From: Wolfgang Grandegger <wg-5Yr1BZd7O62+XT7JhA+gdA@public•gmane.org>
To: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public•gmane.org>
Cc: Socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public•gmane.org,
Netdev-u79uwXL29TY76Z2rM5mHXA@public•gmane.org,
Devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public•gmane.org,
Linuxppc-dev-uLR06cmDAlY/bJ5BZ2RsiQ@public•gmane.org,
Wolfgang Grandegger <wg-ynQEQJNshbs@public•gmane.org>
Subject: Re: [PATCH net-next 2/3] can: mscan-mpc5xxx: add support for the MPC521x processor
Date: Mon, 04 Jan 2010 18:35:17 +0100 [thread overview]
Message-ID: <4B4226D5.6000702@grandegger.com> (raw)
In-Reply-To: <20100104172209.GC4665-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
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
>
> 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
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.
next prev parent reply other threads:[~2010-01-04 17:35 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-01-02 8:17 [PATCH net-next 0/3] can: mscan-mpc5xxx: add support for the Freescale MPC512x Wolfgang Grandegger
2010-01-02 8:17 ` [PATCH net-next 1/3] can: mscan: fix improper return if dlc < 8 in start_xmit function Wolfgang Grandegger
2010-01-02 8:17 ` [PATCH net-next 2/3] can: mscan-mpc5xxx: add support for the MPC521x processor Wolfgang Grandegger
[not found] ` <1262420274-16586-3-git-send-email-wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>
2010-01-02 8:17 ` [PATCH net-next 3/3] powerpc/mpc5xxx: add OF platform binding doc for FSL MSCAN devices Wolfgang Grandegger
[not found] ` <1262420274-16586-4-git-send-email-wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>
2010-01-02 14:05 ` Wolfram Sang
2010-01-02 13:57 ` [PATCH net-next 2/3] can: mscan-mpc5xxx: add support for the MPC521x processor Wolfram Sang
[not found] ` <20100102135734.GC2239-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
2010-01-02 16:08 ` Wolfgang Grandegger
2010-01-04 12:49 ` Wolfgang Grandegger
2010-01-04 16:24 ` Wolfram Sang
[not found] ` <20100104162441.GA4665-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
2010-01-04 16:52 ` Wolfgang Grandegger
2010-01-04 17:22 ` Wolfram Sang
[not found] ` <20100104172209.GC4665-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
2010-01-04 17:35 ` Wolfgang Grandegger [this message]
2010-01-04 19:40 ` Wolfgang Denk
[not found] ` <20100104194027.CB2053F6FD-C2Gvrrd9BC/j/ljBK/0BTg@public.gmane.org>
2010-01-04 20:23 ` Wolfram Sang
[not found] ` <1262420274-16586-2-git-send-email-wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>
2010-01-02 13:30 ` [PATCH net-next 1/3] can: mscan: fix improper return if dlc < 8 in start_xmit function Wolfram Sang
[not found] ` <20100102133041.GB2239-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
2010-01-02 15:34 ` Wolfgang Grandegger
[not found] ` <1262420274-16586-1-git-send-email-wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>
2010-01-02 13:27 ` [PATCH net-next 0/3] can: mscan-mpc5xxx: add support for the Freescale MPC512x Wolfram Sang
[not found] ` <20100102132722.GA2239-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
2010-01-02 15:32 ` Wolfgang Grandegger
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4B4226D5.6000702@grandegger.com \
--to=wg-5yr1bzd7o62+xt7jha+gda@public$(echo .)gmane.org \
--cc=Devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public$(echo .)gmane.org \
--cc=Linuxppc-dev-uLR06cmDAlY/bJ5BZ2RsiQ@public$(echo .)gmane.org \
--cc=Netdev-u79uwXL29TY76Z2rM5mHXA@public$(echo .)gmane.org \
--cc=Socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public$(echo .)gmane.org \
--cc=w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public$(echo .)gmane.org \
--cc=wg-ynQEQJNshbs@public$(echo .)gmane.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox