Re: [PATCH net-next 2/3] can: mscan-mpc5xxx: add support for the MPC521x processor

[Wolfram Sang <w.sang@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 4101 bytes --]

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/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]