Hi Wolfram, Comments below
On Mon, Nov 2, 2009 at 8:17 AM, Wolfram Sang <w.s...@pengutronix.de> wrote: > Signed-off-by: John Rigby <jri...@freescale.com> > Signed-off-by: Chen Hongjun <hong-jun.c...@freescale.com> > Signed-off-by: Wolfram Sang <w.s...@pengutronix.de> > Cc: Wolfgang Denk <w...@denx.de> > Cc: Grant Likely <grant.lik...@secretlab.ca> > --- > > Should come after the fix for clk_get to be usable for the upcoming CAN > driver: > http://patchwork.ozlabs.org/patch/37342/ > > arch/powerpc/platforms/512x/clock.c | 74 > +++++++++++++++++++++++++++++++++++ > 1 files changed, 74 insertions(+), 0 deletions(-) > > diff --git a/arch/powerpc/platforms/512x/clock.c > b/arch/powerpc/platforms/512x/clock.c > index 4168457..2d3a5ef 100644 > --- a/arch/powerpc/platforms/512x/clock.c > +++ b/arch/powerpc/platforms/512x/clock.c > @@ -50,6 +50,8 @@ struct clk { > static LIST_HEAD(clocks); > static DEFINE_MUTEX(clocks_mutex); > > +struct clk mscan_clks[4]; > + I'd rather not have more globals. If really needed, should at the very least be static and prefixed with mpc5121_. > static struct clk *mpc5121_clk_get(struct device *dev, const char *id) > { > struct clk *p, *clk = ERR_PTR(-ENOENT); > @@ -119,6 +121,8 @@ struct mpc512x_clockctl { > u32 spccr; /* SPDIF Clk Ctrl Reg */ > u32 cccr; /* CFM Clk Ctrl Reg */ > u32 dccr; /* DIU Clk Cnfg Reg */ > + /* rev2+ only regs */ > + u32 mccr[4]; /* MSCAN Clk Ctrl Reg 1-4 */ > }; > > struct mpc512x_clockctl __iomem *clockctl; > @@ -688,6 +692,72 @@ static void psc_clks_init(void) > } > } > > + > +/* > + * mscan clock rate calculation > + */ > +static unsigned long mscan_calc_rate(struct device_node *np, int mscannum) > +{ > + unsigned long mscanclk_src, mscanclk_div; > + u32 *mccr = &clockctl->mccr[mscannum]; > + > + /* > + * If the divider is the reset default of all 1's then > + * we know u-boot and/or board setup has not > + * done anything so set up a sane default > + */ > + if (((in_be32(mccr) >> 17) & 0x7fff) == 0x7fff) { > + /* disable */ > + out_be32(mccr, 0); > + /* src is sysclk, divider is 4 */ > + out_be32(mccr, (0x3 << 17) | 0x10000); > + } > + > + switch ((in_be32(mccr) >> 14) & 0x3) { > + case 0: > + mscanclk_src = sys_clk.rate; > + break; > + case 1: > + mscanclk_src = ref_clk.rate; > + break; > + case 2: > + mscanclk_src = psc_mclk_in.rate; > + break; > + case 3: > + mscanclk_src = spdif_txclk.rate; > + break; > + } Nit: Table lookup perhaps? > + > + mscanclk_src = roundup(mscanclk_src, 1000000); > + mscanclk_div = ((in_be32(mccr) >> 17) & 0x7fff) + 1; > + return mscanclk_src / mscanclk_div; > +} > + > +/* > + * Find all silicon rev2 mscan nodes in device tree and assign a clock > + * with name "mscan%d_clk" and dev pointing at the device > + * returned from of_find_device_by_node > + */ Comment block doesn't really help me understand what the function does. > +static void mscan_clks_init(void) > +{ > + struct device_node *np; > + struct of_device *ofdev; > + const u32 *cell_index; > + > + for_each_compatible_node(np, NULL, "fsl,mpc5121-mscan") { > + cell_index = of_get_property(np, "cell-index", NULL); > + if (cell_index) { > + struct clk *clk = &mscan_clks[*cell_index]; > + clk->flags = CLK_HAS_RATE; > + ofdev = of_find_device_by_node(np); > + clk->dev = &ofdev->dev; > + clk->rate = mscan_calc_rate(np, *cell_index); > + sprintf(clk->name, "mscan%d_clk", *cell_index); > + clk_register(clk); > + } > + } > +} These clock controllers are 1:1 dedicated to the CAN devices, correct? Wouldn't it make more sense to put this code directly into the CAN bus device driver instead of in common code? And allocated the clk structure at driver probe time? It seems like the only shared bit seems to be access to the mccr registers. g. -- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. _______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
