Re: [PATCH v2 4/6] drm/exynos: dsi: add support for Exynos5433 SoC

[Hyungwon Hwang <human.hwang@samsung.com>]

Dear Andrej,

On Mon, 23 Mar 2015 10:31:58 +0100
Andrzej Hajda <a.hajda@samsung.com> wrote:

> On 03/20/2015 06:15 AM, Hyungwon Hwang wrote:
> > Dear Andrej,
> >
> > On Thu, 19 Mar 2015 10:32:10 +0100
> > Andrzej Hajda <a.hajda@samsung.com> wrote:
> >
> >> On 03/19/2015 02:18 AM, Hyungwon Hwang wrote:
> >>> Dear Daniel,
> >>>
> >>> On Thu, 19 Mar 2015 01:13:21 +0000
> >>> Daniel Stone <daniel-rLtY4a/8tF1rovVCs/uTlw@public.gmane.org>
> >>> wrote:
> >>>
> >>>> Hi Hyungwon,
> >>>>
> >>>> On 19 March 2015 at 01:02, Hyungwon Hwang
> >>>> <human.hwang-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
> >>>>>>> +       /*
> >>>>>>> +        * The input PLL clock for MIPI DSI in Exynos5433
> >>>>>>> seems to be fixed
> >>>>>>> +        * by OSC CLK.
> >>>>>>> +        */
> >>>>>>> +       fin = 24 * MHZ;
> >>>>>> Er, is this always true on other platforms as well? Shouldn't
> >>>>>> this be a part of the DeviceTree description?
> >>>>> I forgot to change the comment in development. Finally it is
> >>>>> found that all exynos mipi dsi's fin is OSC clk which is 24
> >>>>> MHz. So I will remove the comment, but remain the code as it is.
> >>>> Fair enough. Should pll_clk be removed from the DT description
> >>>> then, if it's fixed to the oscillator?
> >>> Yes. It is redundant to represent pll_clk in DT, and it should be
> >>> removed.
> >> Why do you think OSC clk determines value of pll_clk?
> >> pll_clk is mapped to SCLK_MIPI[01] or SCLK_DSIM0 gate with few
> >> dividers and muxes above. So at least in theory it can differ from
> >> osc clk. Additionally this gate should be enabled so you cannot
> >> just remove it from DT.
> >>
> >> Regards
> >> Andrzej
> > As I found, pll clk is not SCLK_MIPI[01] but OSC CLK. SCLK_DSIM0
> > must be controlled in this driver as it has been, as a gate clock
> > of MIPI DSI block, but not as a pll clk. SCLK_DSIM0 is not the input
> > clock of MIPI DPHY which provides fin in this code. So clock setting
> > and getting code was wrong, and must be removed.
> >
> > OSC CLK is not soc-depedendant but board-dependant, even though I
> > have not seen any board which does not use OSC CLK by 24 MHz. It
> > must be parsed from board DT file, which in this case, we can use
> > the value in pll_clk_rate (the variable name must be renamed also).
> >
> > Because ambiguous description in the technical document, I can be
> > wrong. Please let me know if I do not understand something. Thanks
> > for your comment.
> 
> After some digging I agree that documentation is quite confusing and
> current code could be wrong. Anyway I wonder if it wouldn't be better
> to explicitly provide input clock for DSIM, or more precisely for its
> PLL instead of hardcoding 24MHz into the driver.

OK. I agree. It will be more explicit to get the clock rate from DT.

> 
> Another thing that bothers me is relation of DPHY_PLL in clock
> controller to MIPI_DPHY in Exynos7420. There are two clocks used by
> MIPI_DPHY:
> - "Ref Clock" pinned to SCLK_MIPIDPHY_M4 connected to OSCCLK,
> - "PHY Clock" pinned to I_FOUT_DPHY_PLL connected to DPHY_PLL,
> 
> The first clock seems to be your osc clock, but what is the role of
> the 2nd one?

Hmm, I couldn't find similar clock in Exynos5433, also I don't have
the manual for Exynos7420.

Best regards,
Hyungwon Hwang


> 
> Regards
> Andrzej
> 
> > Best regards,
> > Hyungwon Hwang
> >
> >>>>> Thanks for your review. I will send it again with the changes
> >>>>> you suggested.
> >>>> Thanks very much!
> >>>>
> >>>> Cheers,
> >>>> Daniel
> >>> Best regards,
> >>> Hyungwon Hwang
> >>> --
> >>> To unsubscribe from this list: send the line "unsubscribe
> >>> devicetree" in the body of a message to
> >>> majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo
> >>> info at  http://vger.kernel.org/majordomo-info.html
> >>>
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel