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

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

quoted

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.

quoted

quoted

quoted

quoted

+
+#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.)

quoted

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