Patchwork * mpc8313erdb.dts: Fixed eTSEC interrupt assignment.

login
register
mail settings
Submitter Roland Lezuo
Date Sept. 4, 2009, 10:31 a.m.
Message ID <25940478.1252060285938.JavaMail.root@viefep11.chello.at>
Download mbox | patch
Permalink /patch/32986/
State Deferred
Delegated to: Kumar Gala
Headers show

Comments

Roland Lezuo - Sept. 4, 2009, 10:31 a.m.
The following patch is needed to correctly assign the IRQs for the gianfar driver on the MPC8313ERDB-revc boards. ERR and TX are swapped as well as the interrupt lines for the two devices.

Signed-off-by: Roland Lezuo <roland.lezuo@chello.at>

---
 arch/powerpc/boot/dts/mpc8313erdb.dts |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
Scott Wood - Sept. 9, 2009, 6:22 p.m.
On Fri, Sep 04, 2009 at 12:31:25PM +0200, Roland Lezuo wrote:
> The following patch is needed to correctly assign the IRQs for the
> gianfar driver on the MPC8313ERDB-revc boards. ERR and TX are swapped
> as well as the interrupt lines for the two devices.

And it will incorrectly assign them on older revisions of the chip.

We really should have a u-boot fixup based on SVR.

-Scott
Kumar Gala - Sept. 9, 2009, 8:28 p.m.
On Sep 9, 2009, at 1:22 PM, Scott Wood wrote:

> On Fri, Sep 04, 2009 at 12:31:25PM +0200, Roland Lezuo wrote:
>> The following patch is needed to correctly assign the IRQs for the
>> gianfar driver on the MPC8313ERDB-revc boards. ERR and TX are swapped
>> as well as the interrupt lines for the two devices.
>
> And it will incorrectly assign them on older revisions of the chip.
>
> We really should have a u-boot fixup based on SVR.

I felt like Kim did this.

- k
Kim Phillips - Sept. 9, 2009, 8:35 p.m.
On Wed, 9 Sep 2009 15:28:01 -0500
Kumar Gala <galak@kernel.crashing.org> wrote:

> 
> On Sep 9, 2009, at 1:22 PM, Scott Wood wrote:
> 
> > On Fri, Sep 04, 2009 at 12:31:25PM +0200, Roland Lezuo wrote:
> >> The following patch is needed to correctly assign the IRQs for the
> >> gianfar driver on the MPC8313ERDB-revc boards. ERR and TX are swapped
> >> as well as the interrupt lines for the two devices.
> >
> > And it will incorrectly assign them on older revisions of the chip.
> >
> > We really should have a u-boot fixup based on SVR.

definitely.

> I felt like Kim did this.

I never got my hands on a Rev. C board.  What's the SVR on it?  I think
this is also valid for Rev. B boards, and I don't have one of those
either.

Kim
Mark Bishop - Sept. 9, 2009, 8:49 p.m.
Quoting Scott Wood <scottwood@freescale.com>:

> On Fri, Sep 04, 2009 at 12:31:25PM +0200, Roland Lezuo wrote:
>> The following patch is needed to correctly assign the IRQs for the
>> gianfar driver on the MPC8313ERDB-revc boards. ERR and TX are swapped
>> as well as the interrupt lines for the two devices.
>
> And it will incorrectly assign them on older revisions of the chip.
>
> We really should have a u-boot fixup based on SVR.
>
> -Scott
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
>
>

I have both versions of the boards and I use two versions of the .dts  
file.  Actually, four, two to support the old style uboot .dts that  
comes with Freescale's BSP and 2 that support the newer uboot.
Richard Cochran - Oct. 14, 2009, 7:41 a.m.
>-----Original Message-----
>From: linuxppc-dev-bounces On Behalf Of Scott Wood
>Sent: Wednesday, September 09, 2009 8:22 PM
>To: Roland Lezuo
>Cc: linuxppc-dev@lists.ozlabs.org
>Subject: Re: [PATCH] * mpc8313erdb.dts: Fixed eTSEC interrupt assignment.
>
>On Fri, Sep 04, 2009 at 12:31:25PM +0200, Roland Lezuo wrote:
>> The following patch is needed to correctly assign the IRQs for the
>> gianfar driver on the MPC8313ERDB-revc boards. ERR and TX are swapped
>> as well as the interrupt lines for the two devices.
>
>And it will incorrectly assign them on older revisions of the chip.
>
>We really should have a u-boot fixup based on SVR.

Why not just offer different dts files, one for each board revision?

Something like:
mpc8313erdb-revA.dts
mpc8313erdb-revB.dts
mpc8313erdb-revC.dts

Richard
Scott Wood - Oct. 14, 2009, 3:27 p.m.
On Wed, Oct 14, 2009 at 09:41:33AM +0200, Richard Cochran wrote:
> >-----Original Message-----
> >From: linuxppc-dev-bounces On Behalf Of Scott Wood
> >Sent: Wednesday, September 09, 2009 8:22 PM
> >To: Roland Lezuo
> >Cc: linuxppc-dev@lists.ozlabs.org
> >Subject: Re: [PATCH] * mpc8313erdb.dts: Fixed eTSEC interrupt assignment.
> >
> >On Fri, Sep 04, 2009 at 12:31:25PM +0200, Roland Lezuo wrote:
> >> The following patch is needed to correctly assign the IRQs for the
> >> gianfar driver on the MPC8313ERDB-revc boards. ERR and TX are swapped
> >> as well as the interrupt lines for the two devices.
> >
> >And it will incorrectly assign them on older revisions of the chip.
> >
> >We really should have a u-boot fixup based on SVR.
> 
> Why not just offer different dts files, one for each board revision?
> 
> Something like:
> mpc8313erdb-revA.dts
> mpc8313erdb-revB.dts
> mpc8313erdb-revC.dts

Because that would be three times the device trees to maintain, and a
source of user confusion.

-Scott
Richard Cochran - Oct. 15, 2009, 12:19 p.m.
>-----Original Message----- From: Scott Wood [mailto:scottwood@freescale.com]
>Because that would be three times the device trees to maintain, and a
>source of user confusion.

I wonder which is more confusing for the user:

1. Choosing one of three dts files.
2. Having only one dts for his board, but Ethernet doesn't work.

Richard
Scott Wood - Oct. 15, 2009, 4:27 p.m.
On Thu, Oct 15, 2009 at 02:19:30PM +0200, Richard Cochran wrote:
> >-----Original Message----- From: Scott Wood [mailto:scottwood@freescale.com]
> >Because that would be three times the device trees to maintain, and a
> >source of user confusion.
> 
> I wonder which is more confusing for the user:
> 
> 1. Choosing one of three dts files.

Possibly incorrectly.

> 2. Having only one dts for his board, but Ethernet doesn't work.

The point is to fix u-boot so that it *does* work with only one dts.  If
people not upgrading u-boot is your concern, we could put the fixup in the
Linux platform code instead.

And feel free to ask through official Freescale support channels why the
U-Boot that shipped on these boards does not have such a fixup (or why they
decided it was better to make late-rev 8313's interrupt assignments match
other 83xx than for all revs of the same part number to have the same
interrupt assignments).

-Scott
Richard Cochran - Oct. 16, 2009, 6:31 a.m.
> -----Original Message-----
> From: Scott Wood [mailto:scottwood@freescale.com]
> Subject: Re: [PATCH] * mpc8313erdb.dts: Fixed eTSEC interrupt assignment.
>
> On Thu, Oct 15, 2009 at 02:19:30PM +0200, Richard Cochran wrote:
> > 2. Having only one dts for his board, but Ethernet doesn't work.
>
> The point is to fix u-boot so that it *does* work with only one dts.  If
> people not upgrading u-boot is your concern, we could put the fixup in the
> Linux platform code instead.

Yes, I was thinking that upgrading u-boot can be problematic for
people who don't have a JTAG flash tool, in case things go wrong. The
whole kernel-uboot-dts triangle is a constant source of confusion, IMHO.

But I thought the whole point of the device tree was, that it is easy
to produce a dts that exactly matches a particular physical board
design. If MPC8313-ERDB REVC has a different IRQ routing, then doesn't
it deserve its own dts? Or do only *some* of the REVC boards have this
routing?

> And feel free to ask through official Freescale support channels why the
> U-Boot that shipped on these boards does not have such a fixup (or why they
> decided it was better to make late-rev 8313's interrupt assignments match
> other 83xx than for all revs of the same part number to have the same
> interrupt assignments).

It sounds like you are not overly satisfied with Freescale's handling
of their BSPs ;)

To be fair, I am happy that Freescale appears to take Linux support
seriously, but I do think they drop the ball once a BSP is
published. For example, the MPC8313-ERDB ships with kernel 2.6.23
(with the old style dts) and a fair number of non-mainstream
patches. It is not so easy to get a recent kernel booting on that
board.

I have the LITE5200B, MPC8313-ERDB, MPC8572DS, and the P2020DS in
house, and it is really the same, sad story with each of
them. Wouldn't it be grand if the development boards would boot "out
of the box" when compiling the most recent kernel with default config?

Maybe that's only wishful thinking, but I am happy to contribute what I
can...

Richard
Scott Wood - Oct. 16, 2009, 4:01 p.m.
On Fri, Oct 16, 2009 at 08:31:19AM +0200, Richard Cochran wrote:
> > -----Original Message-----
> > From: Scott Wood [mailto:scottwood@freescale.com]
> > Subject: Re: [PATCH] * mpc8313erdb.dts: Fixed eTSEC interrupt assignment.
> >
> > On Thu, Oct 15, 2009 at 02:19:30PM +0200, Richard Cochran wrote:
> > > 2. Having only one dts for his board, but Ethernet doesn't work.
> >
> > The point is to fix u-boot so that it *does* work with only one dts.  If
> > people not upgrading u-boot is your concern, we could put the fixup in the
> > Linux platform code instead.
> 
> Yes, I was thinking that upgrading u-boot can be problematic for
> people who don't have a JTAG flash tool, in case things go wrong.

Right, so we could put a fixup in Linux as well to handle that case (make
sure not to blindly reverse, though, in case u-boot has already fixed it
up).

> But I thought the whole point of the device tree was, that it is easy
> to produce a dts that exactly matches a particular physical board
> design.

It's the dtb that the kernel sees (possibly after platform fixups, though
that's not the ideal case) that is supposed to exactly match the hardware. 
The dts is just one source (albeit the most significant one) of the data
that goes into the final dtb.

There are boards that have several orthogonal configuration options that
can't be probed -- we'd have to provide dozens (or worse) of .dts files in
order to cover all of them without any runtime fixup.  Not to mention things
like memory size and clock frequency...

> If MPC8313-ERDB REVC has a different IRQ routing, then doesn't
> it deserve its own dts? 

I'm inclined to say no, though I wouldn't object too hard in this case if
people really want it.  But we should still put in a u-boot fixup so that if
you have a new u-boot it doesn't matter which dts you use.

> Or do only *some* of the REVC boards have this
> routing?

They should all have it.

> > And feel free to ask through official Freescale support channels why the
> > U-Boot that shipped on these boards does not have such a fixup (or why they
> > decided it was better to make late-rev 8313's interrupt assignments match
> > other 83xx than for all revs of the same part number to have the same
> > interrupt assignments).
> 
> It sounds like you are not overly satisfied with Freescale's handling
> of their BSPs ;)

:-)

Things are slowly getting better, but it'd be good for the BSP teams to hear
from customers (and not just other internal groups) saying they want more
up-to-date software that doesn't diverge as much from mainline.

And getting u-boot right before shipping it on a board is particularly
important.

> To be fair, I am happy that Freescale appears to take Linux support
> seriously, but I do think they drop the ball once a BSP is
> published. For example, the MPC8313-ERDB ships with kernel 2.6.23
> (with the old style dts) and a fair number of non-mainstream
> patches. It is not so easy to get a recent kernel booting on that
> board.

What problems have you been having with upstream kernels on mpc8313erdb,
other than this IRQ issue?  It should work, though the BSP may have extra
features that haven't been pushed upstream.

> I have the LITE5200B, MPC8313-ERDB, MPC8572DS, and the P2020DS in
> house, and it is really the same, sad story with each of
> them. Wouldn't it be grand if the development boards would boot "out
> of the box" when compiling the most recent kernel with default config?

That's what the group I'm in tries to make sure happens (for the
mpc8xxx/qoriq chips) -- but we're outnumbered by the BSP developers, so some
BSP features tend to be missing.  Basic board support should be there,
though.

I just booted 2.6.32-rc4 straight from Linus on an mpc8572ds yesterday. 
I've booted various upstream kernels on mpc8313erdb (typically an older rev,
though).

-Scott
Kumar Gala - Oct. 16, 2009, 4:30 p.m.
On Oct 16, 2009, at 11:01 AM, Scott Wood wrote:

>>
>> I have the LITE5200B, MPC8313-ERDB, MPC8572DS, and the P2020DS in
>> house, and it is really the same, sad story with each of
>> them. Wouldn't it be grand if the development boards would boot "out
>> of the box" when compiling the most recent kernel with default  
>> config?
>
> That's what the group I'm in tries to make sure happens (for the
> mpc8xxx/qoriq chips) -- but we're outnumbered by the BSP developers,  
> so some
> BSP features tend to be missing.  Basic board support should be there,
> though.
>
> I just booted 2.6.32-rc4 straight from Linus on an mpc8572ds  
> yesterday.
> I've booted various upstream kernels on mpc8313erdb (typically an  
> older rev,
> though).

Also, if they don't boot out of the box report an issue on this list.   
That is part of the community testing aspect of open source.

- k
Richard Cochran - Oct. 20, 2009, 10:01 a.m.
> -----Original Message-----
> From: Scott Wood [mailto:scottwood@freescale.com]
>
> What problems have you been having with upstream kernels on mpc8313erdb,
> other than this IRQ issue?  It should work, though the BSP may have extra
> features that haven't been pushed upstream.

I have been working from kernel 2.6.30 (although the very latest
kernel is just the same WRT these problems, AFAICT). I had to patch in
order to sovle the following three problems:

1. The flash layout in the DTS does not match the default partitioning
   from Freescale. In the current dts, the NAND partitioning is wrong,
   and there is no partitioning for the NOR flash given.

2. The eTSEC interrupt issue that started this thread.

3. The PTP IO signals are not configured in a vanilla linux. For this,
   you need parts of a Freescale patch. [1] Their PTP implementation
   gianfar driver is horrible, but still, the IO configuration in
   these two files in the patch is necessary to get any external
   signals from the PTP clock:

   arch/powerpc/platforms/83xx/mpc8313_rdb.c
   arch/powerpc/platforms/83xx/mpc83xx.h

It may be that point 1 is not so important, since the flash layout is
perhaps a very personal thing, but I would still expect the default
dts to match the default partitioning from Freescale.

Point 2 is obviously essential, and if you buy the mpc8313 for its PTP
capabilities, as I did, then point 3 is a show stopper, as well.

> I just booted 2.6.32-rc4 straight from Linus on an mpc8572ds yesterday.
> I've booted various upstream kernels on mpc8313erdb (typically an older rev,
> though).

Okay, you are right, the mpc8572ds does seem to work pretty well.

Perhaps we can fix up the above three issues?

Richard

[1] http://www.bitshrine.org/gpp_info/linux-fsl-2.6.23-MPC8313ERDB-IEEE-1588.patch.html
Scott Wood - Oct. 20, 2009, 3:56 p.m.
On Tue, Oct 20, 2009 at 12:01:19PM +0200, Richard Cochran wrote:
> > -----Original Message-----
> > From: Scott Wood [mailto:scottwood@freescale.com]
> >
> > What problems have you been having with upstream kernels on mpc8313erdb,
> > other than this IRQ issue?  It should work, though the BSP may have extra
> > features that haven't been pushed upstream.
> 
> I have been working from kernel 2.6.30 (although the very latest
> kernel is just the same WRT these problems, AFAICT). I had to patch in
> order to sovle the following three problems:
> 
> 1. The flash layout in the DTS does not match the default partitioning
>    from Freescale. In the current dts, the NAND partitioning is wrong,
>    and there is no partitioning for the NOR flash given.

OK, I wasn't aware of that -- that kind of thing is an artifact of the BSP
development process being fairly separate from upstream development.  The
"open source team" works on upstream Linux and upstream U-Boot.  We don't
run the BSP u-boot (since we're developing the newer u-boot), and it's easy
to miss when things like this diverge.

In this case, I'd be surprised if the NAND partitioning that is upstream
came from anywhere but an early BSP's Linux tree.  There's not much we (the
upstream-focused developers) can do if different BSPs have different layouts
(other than not specify a layout at all)...  What is the layout you see in
your BSP?

> 2. The eTSEC interrupt issue that started this thread.

Well, yes. :-)

> 3. The PTP IO signals are not configured in a vanilla linux. For this,
>    you need parts of a Freescale patch. [1] Their PTP implementation
>    gianfar driver is horrible, but still, the IO configuration in
>    these two files in the patch is necessary to get any external
>    signals from the PTP clock:
> 
>    arch/powerpc/platforms/83xx/mpc8313_rdb.c
>    arch/powerpc/platforms/83xx/mpc83xx.h

Yes, this is one of those features that is currently BSP-only (partly due to
it needing work, as you note above).  That's different than the board simply
not being supported upstream, but if you need it, you need it.  I'll raise
the issue in my team, but I suggest also letting Freescale support (or other
official feedback channels) know what you'd like to see in terms of
improvements in that code and getting it upstream.

-Scott

Patch

diff --git a/arch/powerpc/boot/dts/mpc8313erdb.dts b/arch/powerpc/boot/dts/mpc8313erdb.dts
index 761faa7..907a445 100644
--- a/arch/powerpc/boot/dts/mpc8313erdb.dts
+++ b/arch/powerpc/boot/dts/mpc8313erdb.dts
@@ -188,7 +188,7 @@ 
 			compatible = "gianfar";
 			reg = <0x24000 0x1000>;
 			local-mac-address = [ 00 00 00 00 00 00 ];
-			interrupts = <37 0x8 36 0x8 35 0x8>;
+			interrupts = <32 0x8 33 0x8 34 0x8>;
 			interrupt-parent = <&ipic>;
 			tbi-handle = < &tbi0 >;
 			/* Vitesse 7385 isn't on the MDIO bus */
@@ -223,7 +223,7 @@ 
 			reg = <0x25000 0x1000>;
 			ranges = <0x0 0x25000 0x1000>;
 			local-mac-address = [ 00 00 00 00 00 00 ];
-			interrupts = <34 0x8 33 0x8 32 0x8>;
+			interrupts = <35 0x8 36 0x8 37 0x8>;
 			interrupt-parent = <&ipic>;
 			tbi-handle = < &tbi1 >;
 			phy-handle = < &phy4 >;