RE: [PATCH v2 04/12] driver: clk: imx: Add clock driver for imx6sll

[Jacky Bai <ping.bai@xxxxxxx>]

> Subject: Re: [PATCH v2 04/12] driver: clk: imx: Add clock driver for imx6sll
> 
> > > > +
> > > > +static const char *pll_bypass_src_sels[] = { "osc", "dummy", };
> > >
> > > All these should be const char * const unless something is wrong.
> >
> > If changed to 'const char * const', it vill has argument type mismatch
> > error, as imx_clk_* wrapper function has argument type 'const char *'.
> 
> Hmm that's unfortunate.
> 
> >
> > >
> > > > +
> > > > +	for (i = 0; i < ARRAY_SIZE(clks_init_on); i++)
> > > > +		clk_prepare_enable(clks[clks_init_on[i]]);
> > >
> > > Critical clocks?
> >
> > Yes, these clocks must be always on.
> >
> > >
> > > > +
> > > > +	if (IS_ENABLED(CONFIG_USB_MXS_PHY)) {
> > > > +		clk_prepare_enable(clks[IMX6SLL_CLK_USBPHY1_GATE]);
> > > > +		clk_prepare_enable(clks[IMX6SLL_CLK_USBPHY2_GATE]);
> > >
> > > The phy driver can't enable these?
> >
> > The reason why we enable these two clks here is in below commit commit
> > a5120e89e7e187a91852896f586876c7a2030804
> > Author: Peter Chen <peter.chen@xxxxxxxxxxxxx>
> > Date:   Fri Jan 18 10:38:05 2013 +0800
> >       ARM i.MX6: change mxs usbphy clock usage
> >
> 
> So can we mark these clks with CLK_IS_CRITICAL flag then instead?
> Or are they disabled out of the bootloader?
> 

Sure, using 'CLK_IS_CRITICAL'  should be ok for clks_init_on clocks.  But for  USBPHY*_GATE, it is
only enabled when CONIG_USB_MXC_PHY is true.  And another concern is if we need to add CLK_IS_CRITICAL
flag to clks_init_on clocks, we may need to add new wrapper function to register these critical clock. It is not very good.

 > >
> > > > +	}
> > > > +
> > > > +	/* Lower the AHB clock rate before changing the clock source. */
> > > > +	clk_set_rate(clks[IMX6SLL_CLK_AHB], 99000000);
> > > > +
> > > > +	/* Change periph_pre clock to pll2_bus to adjust AXI rate to 264MHz */
> > > > +	clk_set_parent(clks[IMX6SLL_CLK_PERIPH_CLK2_SEL],
> > > clks[IMX6SLL_CLK_PLL3_USB_OTG]);
> > > > +	clk_set_parent(clks[IMX6SLL_CLK_PERIPH],
> > > clks[IMX6SLL_CLK_PERIPH_CLK2]);
> > > > +	clk_set_parent(clks[IMX6SLL_CLK_PERIPH_PRE],
> > > clks[IMX6SLL_CLK_PLL2_BUS]);
> > > > +	clk_set_parent(clks[IMX6SLL_CLK_PERIPH],
> > > > +clks[IMX6SLL_CLK_PERIPH_PRE]);
> > > > +
> > > > +	clk_set_rate(clks[IMX6SLL_CLK_AHB], 132000000);
> > >
> > > assigned-clocks for rates now? Or perhaps we shouldn't be exposing
> > > these as clks if they have some sort of complicated rate sequence
> > > switch that we can't guarantee with the clk_ops we have today.
> >
> > These clks will be used by some peripherals, so we need to expose these
> clocks.
> > And the above parent and rate swith sequence is not very easy to be
> > handled in assigned-clocks, So we leave it in this place.
> >
> 
> How do we guarantee that the rate switch doesn't happen later on, requiring
> this coordinated sequence of clk operations?
> 

This clock sequence is used for increasing the AXI and AHB bus clock rate. In normal
use, it is very rarely that we need to change them again. If we really need to change
AXI and AHB bus clock later, a similar sequence must be used to do this.

> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux
> Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html