Hello Wolfgang, first the good news: Your patches also work with our MPC5121-board.
> >> +#else /* !CONFIG_PPC_MPC5200 */
> >> +static u32 __devinit mpc52xx_can_get_clock(struct of_device *ofdev,
> >> + const char *clock_name,
> >> + int *mscan_clksrc)
> >> +{
> >> + return 0;
> >> +}
> >> +#endif /* CONFIG_PPC_MPC5200 */
> >
> > Hmmm, I don't really like those empty functions. I once used the data-field
> > of
> > struct of_device_id, which carried a function pointer to a specific
> > init-function for the matched device. What do you think about such an
> > approach?
>
> Often the problem is that the function will not compile on the other MPC
> arch. This is not true here. So, the main reason for the #ifdefs is
> space saving. Your approach will not help in both cases.
My idea was: it might be nice to save both #else-branches and the if-clause in
probe() which calls this get_clock() or the other (and then another in case
there will be a new mpc5xyz-user in the future). And replace it with some
mpc5xxx_custom_init() which is taken from of_device_id->data. No big issue,
though; no show-stopper.
> >> +
> >> +#ifdef CONFIG_PPC_MPC512x
> >> +struct mpc512x_clockctl {
> >> + u32 spmr; /* System PLL Mode Reg */
> >> + u32 sccr[2]; /* System Clk Ctrl Reg 1 & 2 */
> >> + u32 scfr1; /* System Clk Freq Reg 1 */
> >> + u32 scfr2; /* System Clk Freq Reg 2 */
> >> + u32 reserved;
> >> + u32 bcr; /* Bread Crumb Reg */
> >> + u32 pccr[12]; /* PSC Clk Ctrl Reg 0-11 */
> >> + u32 spccr; /* SPDIF Clk Ctrl Reg */
> >> + u32 cccr; /* CFM Clk Ctrl Reg */
> >> + u32 dccr; /* DIU Clk Cnfg Reg */
> >> + u32 mccr[4]; /* MSCAN Clk Ctrl Reg 1-3 */
> >> +};
I wonder if this (and the occurence in clock.c) should be factored out and
moved to asm/mpc5xxx.h?
> >> +
> >> +static struct of_device_id mpc512x_clock_ids[] __devinitdata = {
> >> + { .compatible = "fsl,mpc5121-clock", },
> >> + {}
> >> +};
> >> +
> >> +static u32 __devinit mpc512x_can_get_clock(struct of_device *ofdev,
> >> + const char *clock_name,
> >> + int *mscan_clksrc,
> >> + ssize_t mscan_addr)
> >> +{
> >> + struct mpc512x_clockctl __iomem *clockctl;
> >> + struct device_node *np_clock;
> >> + struct clk *sys_clk, *ref_clk;
> >> + int plen, clockidx, clocksrc = -1;
> >> + u32 sys_freq, val, clockdiv = 1, freq = 0;
> >> + const u32 *pval;
> >> +
> >> + np_clock = of_find_matching_node(NULL, mpc512x_clock_ids);
> >> + if (!np_clock) {
> >> + dev_err(&ofdev->dev, "couldn't find clock node\n");
> >> + return -ENODEV;
> >> + }
> >> + clockctl = of_iomap(np_clock, 0);
> >> + if (!clockctl) {
> >> + dev_err(&ofdev->dev, "couldn't map clock registers\n");
> >> + return 0;
> >> + }
> >> +
> >> + /* Determine the MSCAN device index from the physical address */
> >> + clockidx = (mscan_addr & 0x80) ? 1 : 0;
> >> + if (mscan_addr & 0x2000)
> >> + clockidx += 2;
> >
> > The PSCs use 'cell-index', here we use mscan_addr to derive the index. This
> > is
> > not consistent, but should be IMHO. Now, which is the preferred way? I think
> > I'd go for 'cell-index', as other processors might have mscan_addr shuffled.
> > Also, we could use 'of_iomap' again in the probe_routine.
>
> I understood that "cell-index" is deprecated and it has been removed
> from many nodes. That's why I used the address to derive the index.
Well, the arguments in your other mail make sense to me, so keep it this way.
As not only the index-issue, but also the clock-handling is different for the
PSCs, it is at least consistently inconsistent :D
One further note: I couldn't spot any code handling Rev1 of the MPC5121? Do you
plan to add such code? If not, we should at least put a comment that it is
missing. The binding documentation should be updated as well, as you can't use
all options on such revisions.
Regards,
Wolfram
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
signature.asc
Description: Digital signature
_______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
