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: 3693 bytes --]


> Nice. Just for curiosity, what clock and frequency does it select on
> your board? It should be listed when the driver is loaded.

Using this simple dts-snipplet

                mscan@1300 {
                        compatible = "fsl,mpc5121-mscan";
                        cell-index = <0>;
                        interrupts = <12 8>;
                        interrupt-parent = < &ipic >;
                        reg = <0x1300 0x80>;
                };

I get this output:

mpc5xxx_can 80001300.mscan: using 'sys_clk' with frequency divider 25
mpc5xxx_can 80001300.mscan: MSCAN at 0xe1064300, irq 16, clock 16000000 Hz

mpc5xxx_can 80001380.mscan: using 'sys_clk' with frequency divider 25
mpc5xxx_can 80001380.mscan: MSCAN at 0xe106c380, irq 17, clock 16000000 Hz


> > 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.
> 
> You mean like in the i2c-mpc driver:
> 
> http://lxr.linux.no/#linux+v2.6.32/drivers/i2c/busses/i2c-mpc.c#L585

Yes.

> No problem, if I handle the regs property inside the mpc5121-specific
> function. The #ifdef's are for *space saving*. If nobody else than me
> cares, I will remove them.

I'd be fine with keeping them.

> >>>> +
> >>>> +#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?
> 
> I was thinking about that as well but mpc5xxx.h seems not (yet) to be a
> popular place to store mpc5xxx related definitions.

Probably because the mpc5121 is not very popular when it comes to mainline
activity. (BTW, I also wondered why it is not named mpc512x.h as I'd think
mpc5xxx is for common stuff between mpc5200 and mpc5121. But this is another
issue, not relevant here.)

> > 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.
> 
> Do we have rev1 support in the mainline kernel? I also understood that
> there are only a few devel boards out there with v1 CPUs. If necessary,
> this could be fixed later on demand. But it should be documented, e.g.
> in the KConfig and dts bindings doc, of course.

Yup, documenting it will do.

> Did you have a chance to test bus-off recovery? I just realized one
> issue if the device is stopped while in bus-off.

Sorry, I just did basic transfer-tests as I am currently busy with other
projects.

> Will come up with a v2 patch soon...

Please do an s/latetr/latter/ before posting it :)

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