> 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
ms...@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/ |
signature.asc
Description: Digital signature
_______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
