Dear Andrej, On Mon, 23 Mar 2015 10:31:58 +0100 Andrzej Hajda <a.hajda at 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 at 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 at public.gmane.org> > >>> wrote: > >>> > >>>> Hi Hyungwon, > >>>> > >>>> On 19 March 2015 at 01:02, Hyungwon Hwang > >>>> <human.hwang-Sze3O3UU22JBDgjK7y7TUQ at 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 at public.gmane.org More majordomo > >>> info at http://vger.kernel.org/majordomo-info.html > >>> >
