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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20100104172209.GC4665@pengutronix.de>
Date:	Mon, 4 Jan 2010 18:22:09 +0100
From:	Wolfram Sang <w.sang@...gutronix.de>
To:	Wolfgang Grandegger <wg@...ndegger.com>
Cc:	Socketcan-core@...ts.berlios.de, Netdev@...r.kernel.org,
	Devicetree-discuss@...ts.ozlabs.org, Linuxppc-dev@...ts.ozlabs.org,
	Wolfgang Grandegger <wg@...x.de>
Subject: 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@...0 {
                        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/  |

Download attachment "signature.asc" of type "application/pgp-signature" (198 bytes)