Re: [PATCH v1 1/4] clk: tegra20: Add DEV1/DEV2 OSC dividers

[Marcel Ziswiler]

On Fri, 2018-04-27 at 15:54 +0300, Dmitry Osipenko wrote:
> Hi Marcel,
> 
> On 27.04.2018 15:33,  Ziswiler wrote:
> > Hi Dmitry
> > 
> > Isn't the CLK_RST_CONTROLLER_MISC_CLK_ENB_0 the other way around
> > e.g.
> > DEV1_OSC_DIV_SEL at bit 23:22 and DEV2_OSC_DIV_SEL at 21:20?
> > 
> > On Fri, 2018-04-27 at 02:58 +0300, Dmitry Osipenko wrote:
> > > CDEV1/CDEV2 clocks could have corresponding oscillator clock
> > > divider
> > > as
> > > a parent. Add these dividers in order to be able to provide that
> > > parent
> > > option.
> > > 
> > > Signed-off-by: Dmitry Osipenko <digetx@xxxxxxxxx>
> > > ---
> > >  drivers/clk/tegra/clk-tegra20.c | 12 ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > > 
> > > diff --git a/drivers/clk/tegra/clk-tegra20.c
> > > b/drivers/clk/tegra/clk-
> > > tegra20.c
> > > index 0ee56dd04cec..16cf4108f2ff 100644
> > > --- a/drivers/clk/tegra/clk-tegra20.c
> > > +++ b/drivers/clk/tegra/clk-tegra20.c
> > > @@ -26,6 +26,8 @@
> > >  #include "clk.h"
> > >  #include "clk-id.h"
> > >  
> > > +#define MISC_CLK_ENB 0x48
> > > +
> > >  #define OSC_CTRL 0x50
> > >  #define OSC_CTRL_OSC_FREQ_MASK (3<<30)
> > >  #define OSC_CTRL_OSC_FREQ_13MHZ (0<<30)
> > > @@ -831,6 +833,16 @@ static void __init
> > > tegra20_periph_clk_init(void)
> > >  				    periph_clk_enb_refcnt);
> > >  	clks[TEGRA20_CLK_PEX] = clk;
> > >  
> > > +	/* cdev1 OSC divider */
> > > +	clk_register_divider(NULL, "cdev1_osc_div", "clk_m",
> > > +			     0, clk_base + MISC_CLK_ENB, 20, 2,
> > 
> > So it would be:
> > 
> > +			     0, clk_base + MISC_CLK_ENB, 22, 2,
> > 
> > > +			     CLK_DIVIDER_POWER_OF_TWO, NULL);
> > > +
> > > +	/* cdev2 OSC divider */
> > > +	clk_register_divider(NULL, "cdev2_osc_div", "clk_m",
> > > +			     0, clk_base + MISC_CLK_ENB, 22, 2,
> > 
> > And:
> > 
> > +			     0, clk_base + MISC_CLK_ENB, 20, 2,
> > 
> > > +			     CLK_DIVIDER_POWER_OF_TWO, NULL);
> > > +
> > >  	/* cdev1 */
> > >  	clk = clk_register_fixed_rate(NULL, "cdev1_fixed", NULL,
> > > 0,
> > > 26000000);
> > >  	clk = tegra_clk_register_periph_gate("cdev1",
> > > "cdev1_fixed",
> > > 0,
> 
> Indeed, good catch! I'll wait for more comments and then re-spin
> patchset with
> the fix. Thank you.

You are very welcome. Thank you!

Other than that it all looks proper and works fine at least in the
configuration we use on Colibri T20. So you may add my reviewed and
tested bys to the whole series:

Reviewed-by: Marcel Ziswiler <marcel@xxxxxxxxxxxx>
Tested-by: Marcel Ziswiler <marcel@xxxxxxxxxxxx>