Wolfram Sang wrote: >> 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
OK, the the board uses an oscillator clock of 33.333333 MHz. The "ref" or "ip" clock would be a worse choice. >>> 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. OK. >>>>>> + >>>>>> +#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.) Well, yes. The mpc512x Socket-CAN mainline support is already in a much better shape than the base port. I will keep it until the mpc512x port has matured/settled. >>> 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. You are welcome. >> Will come up with a v2 patch soon... > > Please do an s/latetr/latter/ before posting it :) OK. Wolfgang. _______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev