Xilinx SYSACE driver and 8-bit attachment

Xilinx SYSACE driver and 8-bit attachment

[John Williams]

Hi Grant (it's your driver :) and David D ( and your DTS generator :)

I've just debugged a failure on a board with an 8-bit sysAce
connection - I had an infinite "kicking stalled FSM" loop on driver
load.

Turns out that the way the driver probes for an 8-bit connection:

if (of_find_property(op->node, "8-bit", NULL))
 ...

doesn't match the properties generated by Xilinx's device tree generator:

               SysACE_CompactFlash: sysace@83600000 {
                        compatible = "xlnx,xps-sysace-1.01.a",
"xlnx,xps-sysace-1.00.a";
                        interrupt-parent = <&Interrupt_Cntlr>;
                        interrupts = < 5 2 >;
                        reg = < 0x83600000 0x10000 >;
                        xlnx,family = "spartan6";
                        xlnx,mem-width = <0x8>;

So, the question is, which should change?

A temporary hard-code to 8-bit width fixes the bug for me, so I'm
certain it's just this OF property binding problem.

Let me know if we should link the DTS list into this as well.

Thanks,

John
--
John Williams, PhD, B.Eng, B.IT
PetaLogix - Linux Solutions for a Reconfigurable World
w: www.petalogix.com  p: +61-7-30090663  f: +61-7-30090663

Re: Xilinx SYSACE driver and 8-bit attachment

[Grant Likely]

[Added devicetree-discuss to cc: list]

On Wed, Oct 7, 2009 at 1:13 AM, John Williams
<john.williams@petalogix.com> wrote:
> Hi Grant (it's your driver :) and David D ( and your DTS generator :)
[...]
> if (of_find_property(op->node, "8-bit", NULL))
[...]
> doesn't match the properties generated by Xilinx's device tree generator:
[...]
>                         xlnx,mem-width = <0x8>;
>
> So, the question is, which should change?

Well, obviously its a device tree generator bug, the driver code is
perfect.  :-P

In all seriousness though, the '8-bit' property has been present for a
while now and needs to be retained to not break existing users.  It
would be okay for the driver to be modified to *also* check for the
xlnx,mem-width property, but the better solution is to modify the
device tree generator.  Plus, the '8-bit' property is the documented
binding in Documentation/powerpc/dts-bindings/xilinx.txt

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

RE: Xilinx SYSACE driver and 8-bit attachment

[John Linn]

I'll look to see if something has changed.  It's not clear to me if it has ever been right.

It look like we normally use 16 bit mode as that's how BSB generates the system by default.

Thanks,
John

> -----Original Message-----
> From: glikely@secretlab.ca [mailto:glikely@secretlab.ca] On Behalf Of Grant Likely
> Sent: Wednesday, October 07, 2009 7:35 AM
> To: John Williams
> Cc: Linux Kernel list; Michal Simek; John Linn; David DeBonis; Stephen Neuendorffer; devicetree-
> discuss
> Subject: Re: Xilinx SYSACE driver and 8-bit attachment
> 
> [Added devicetree-discuss to cc: list]
> 
> On Wed, Oct 7, 2009 at 1:13 AM, John Williams
> <john.williams@petalogix.com> wrote:
> > Hi Grant (it's your driver :) and David D ( and your DTS generator :)
> [...]
> > if (of_find_property(op->node, "8-bit", NULL))
> [...]
> > doesn't match the properties generated by Xilinx's device tree generator:
> [...]
> >                         xlnx,mem-width = <0x8>;
> >
> > So, the question is, which should change?
> 
> Well, obviously its a device tree generator bug, the driver code is
> perfect.  :-P
> 
> In all seriousness though, the '8-bit' property has been present for a
> while now and needs to be retained to not break existing users.  It
> would be okay for the driver to be modified to *also* check for the
> xlnx,mem-width property, but the better solution is to modify the
> device tree generator.  Plus, the '8-bit' property is the documented
> binding in Documentation/powerpc/dts-bindings/xilinx.txt
> 
> Cheers,
> g.
> 
> --
> Grant Likely, B.Sc., P.Eng.
> Secret Lab Technologies Ltd.


This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.

Re: Xilinx SYSACE driver and 8-bit attachment

[John Williams]

Hi John,

On Thu, Oct 8, 2009 at 12:24 AM, John Linn <John.Linn@xilinx.com> wrote:
> I'll look to see if something has changed.  It's not clear to me if it has ever been right.
>
> It look like we normally use 16 bit mode as that's how BSB generates the system by default.

The SP605 reference design uses an 8-bit attachment to the SystemACE -
that's how I found this issue.

John
-- 
John Williams, PhD, B.Eng, B.IT
PetaLogix - Linux Solutions for a Reconfigurable World
w: www.petalogix.com  p: +61-7-30090663  f: +61-7-30090663

RE: Xilinx SYSACE driver and 8-bit attachment

[John Linn]

I fixed the device tree generator and checked into git-dev, I'll push it out soon after a little more test time.

-- John

> -----Original Message-----
> From: John Williams [mailto:john.williams@petalogix.com]
> Sent: Wednesday, October 07, 2009 5:42 PM
> To: John Linn
> Cc: grant.likely@secretlab.ca; Linux Kernel list; Michal Simek; David DeBonis; Stephen Neuendorffer;
> devicetree-discuss
> Subject: Re: Xilinx SYSACE driver and 8-bit attachment
> 
> Hi John,
> 
> On Thu, Oct 8, 2009 at 12:24 AM, John Linn <John.Linn@xilinx.com> wrote:
> > I'll look to see if something has changed.  It's not clear to me if it has ever been right.
> >
> > It look like we normally use 16 bit mode as that's how BSB generates the system by default.
> 
> The SP605 reference design uses an 8-bit attachment to the SystemACE -
> that's how I found this issue.
> 
> John
> --
> John Williams, PhD, B.Eng, B.IT
> PetaLogix - Linux Solutions for a Reconfigurable World
> w: www.petalogix.com  p: +61-7-30090663  f: +61-7-30090663


This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.

Re: Xilinx SYSACE driver and 8-bit attachment

[John Williams]

Hi John,


On Thu, Oct 8, 2009 at 9:44 AM, John Linn <John.Linn@xilinx.com> wrote:
> I fixed the device tree generator and checked into git-dev, I'll push it out soon after a little more test time.

I think we should be careful about adding special per-device handing
in the DTS generator.  Where ever possible I would encourage generic
handling.  Most Xilinx devices are "green fields" with respect to the
device tree binding and these params, SystemACE is a little different
because it's been in mainline longer and has users outside the DTS
generator flow.

John
-- 
John Williams, PhD, B.Eng, B.IT
PetaLogix - Linux Solutions for a Reconfigurable World
w: www.petalogix.com  p: +61-7-30090663  f: +61-7-30090663

RE: Xilinx SYSACE driver and 8-bit attachment

[John Linn]

> -----Original Message-----
> From: John Linn
> Sent: Wednesday, October 07, 2009 8:24 AM
> To: 'Grant Likely'; John Williams
> Cc: Linux Kernel list; Michal Simek; David DeBonis; Stephen Neuendorffer; devicetree-discuss
> Subject: RE: Xilinx SYSACE driver and 8-bit attachment
> 
> I'll look to see if something has changed.  It's not clear to me if it has ever been right.
> 
> It look like we normally use 16 bit mode as that's how BSB generates the system by default.
> 
> Thanks,
> John
> 
> > -----Original Message-----
> > From: glikely@secretlab.ca [mailto:glikely@secretlab.ca] On Behalf Of Grant Likely
> > Sent: Wednesday, October 07, 2009 7:35 AM
> > To: John Williams
> > Cc: Linux Kernel list; Michal Simek; John Linn; David DeBonis; Stephen Neuendorffer; devicetree-
> > discuss
> > Subject: Re: Xilinx SYSACE driver and 8-bit attachment
> >
> > [Added devicetree-discuss to cc: list]
> >
> > On Wed, Oct 7, 2009 at 1:13 AM, John Williams
> > <john.williams@petalogix.com> wrote:
> > > Hi Grant (it's your driver :) and David D ( and your DTS generator :)
> > [...]
> > > if (of_find_property(op->node, "8-bit", NULL))
> > [...]
> > > doesn't match the properties generated by Xilinx's device tree generator:
> > [...]
> > >                         xlnx,mem-width = <0x8>;

Do you know if the 8 bit mode was ever tested with the "8-bit" property in the tree?

I don't see anything in the device tree generator history to say we ever did that "8-bit", but maybe I'm missing something.

We are just generating the parameters from the h/w and the memory width is it.  We can always put something in there special, but it seems silly if it was never used anyway.

Thanks,
John

> > >
> > > So, the question is, which should change?
> >
> > Well, obviously its a device tree generator bug, the driver code is
> > perfect.  :-P
> >
> > In all seriousness though, the '8-bit' property has been present for a
> > while now and needs to be retained to not break existing users.  It
> > would be okay for the driver to be modified to *also* check for the
> > xlnx,mem-width property, but the better solution is to modify the
> > device tree generator.  Plus, the '8-bit' property is the documented
> > binding in Documentation/powerpc/dts-bindings/xilinx.txt
> >
> > Cheers,
> > g.
> >
> > --
> > Grant Likely, B.Sc., P.Eng.
> > Secret Lab Technologies Ltd.


This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.

Re: Xilinx SYSACE driver and 8-bit attachment

[Grant Likely]

On Wed, Oct 7, 2009 at 8:42 AM, John Linn <John.Linn@xilinx.com> wrote:
>> -----Original Message-----
>> From: John Linn
>> Sent: Wednesday, October 07, 2009 8:24 AM
>> To: 'Grant Likely'; John Williams
>> Cc: Linux Kernel list; Michal Simek; David DeBonis; Stephen Neuendorffer; devicetree-discuss
>> Subject: RE: Xilinx SYSACE driver and 8-bit attachment
>>
>> I'll look to see if something has changed.  It's not clear to me if it has ever been right.
>>
>> It look like we normally use 16 bit mode as that's how BSB generates the system by default.
>>
>> Thanks,
>> John
>>
>> > -----Original Message-----
>> > From: glikely@secretlab.ca [mailto:glikely@secretlab.ca] On Behalf Of Grant Likely
>> > Sent: Wednesday, October 07, 2009 7:35 AM
>> > To: John Williams
>> > Cc: Linux Kernel list; Michal Simek; John Linn; David DeBonis; Stephen Neuendorffer; devicetree-
>> > discuss
>> > Subject: Re: Xilinx SYSACE driver and 8-bit attachment
>> >
>> > [Added devicetree-discuss to cc: list]
>> >
>> > On Wed, Oct 7, 2009 at 1:13 AM, John Williams
>> > <john.williams@petalogix.com> wrote:
>> > > Hi Grant (it's your driver :) and David D ( and your DTS generator :)
>> > [...]
>> > > if (of_find_property(op->node, "8-bit", NULL))
>> > [...]
>> > > doesn't match the properties generated by Xilinx's device tree generator:
>> > [...]
>> > >                         xlnx,mem-width = <0x8>;
>
> Do you know if the 8 bit mode was ever tested with the "8-bit" property in the tree?
>
> I don't see anything in the device tree generator history to say we ever did that "8-bit", but maybe I'm missing something.
>
> We are just generating the parameters from the h/w and the memory width is it.  We can always put something in there special, but it seems silly if it was never used anyway.

Don't forget that Virtex and Microblaze are not the only users of this
driver.   There is a AMCC 440 board which uses the sysace as a CF
adapter.  8-bit was definitely tested and is in use.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

RE: Xilinx SYSACE driver and 8-bit attachment

[Stephen Neuendorffer]

> -----Original Message-----
> From: devicetree-discuss-bounces+stephen.neuendorffer=xilinx.com@lists.ozlabs.org [mailto:devicetree-
> discuss-bounces+stephen.neuendorffer=xilinx.com@lists.ozlabs.org] On Behalf Of Grant Likely
> Sent: Wednesday, October 07, 2009 7:53 AM
> To: John Linn
> Cc: David DeBonis; Stephen Neuendorffer; devicetree-discuss; Linux Kernel list; Michal Simek; John
> Linn; John Williams
> Subject: Re: Xilinx SYSACE driver and 8-bit attachment
> 
> On Wed, Oct 7, 2009 at 8:42 AM, John Linn <John.Linn@xilinx.com> wrote:
> >> -----Original Message-----
> >> From: John Linn
> >> Sent: Wednesday, October 07, 2009 8:24 AM
> >> To: 'Grant Likely'; John Williams
> >> Cc: Linux Kernel list; Michal Simek; David DeBonis; Stephen Neuendorffer; devicetree-discuss
> >> Subject: RE: Xilinx SYSACE driver and 8-bit attachment
> >>
> >> I'll look to see if something has changed.  It's not clear to me if it has ever been right.
> >>
> >> It look like we normally use 16 bit mode as that's how BSB generates the system by default.
> >>
> >> Thanks,
> >> John
> >>
> >> > -----Original Message-----
> >> > From: glikely@secretlab.ca [mailto:glikely@secretlab.ca] On Behalf Of Grant Likely
> >> > Sent: Wednesday, October 07, 2009 7:35 AM
> >> > To: John Williams
> >> > Cc: Linux Kernel list; Michal Simek; John Linn; David DeBonis; Stephen Neuendorffer; devicetree-
> >> > discuss
> >> > Subject: Re: Xilinx SYSACE driver and 8-bit attachment
> >> >
> >> > [Added devicetree-discuss to cc: list]
> >> >
> >> > On Wed, Oct 7, 2009 at 1:13 AM, John Williams
> >> > <john.williams@petalogix.com> wrote:
> >> > > Hi Grant (it's your driver :) and David D ( and your DTS generator :)
> >> > [...]
> >> > > if (of_find_property(op->node, "8-bit", NULL))
> >> > [...]
> >> > > doesn't match the properties generated by Xilinx's device tree generator:
> >> > [...]
> >> > >                         xlnx,mem-width = <0x8>;
> >
> > Do you know if the 8 bit mode was ever tested with the "8-bit" property in the tree?
> >
> > I don't see anything in the device tree generator history to say we ever did that "8-bit", but
> maybe I'm missing something.
> >
> > We are just generating the parameters from the h/w and the memory width is it.  We can always put
> something in there special, but it seems silly if it was never used anyway.
> 
> Don't forget that Virtex and Microblaze are not the only users of this
> driver.   There is a AMCC 440 board which uses the sysace as a CF
> adapter.  8-bit was definitely tested and is in use.

In this case, I'm pretty sure this tag was never automatically generated by the DTS generator.
Personally, I think it's bad practice to always assume that the device tree generator should be modified to match what a Linux driver is expecting, however given the precedence in this case,
I agree that it might be reasonable to put a special case in to generate this.

Steve
 

This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.

Re: Xilinx SYSACE driver and 8-bit attachment

[John Williams]

On Wed, Oct 7, 2009 at 11:34 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> [Added devicetree-discuss to cc: list]
>
> On Wed, Oct 7, 2009 at 1:13 AM, John Williams
> <john.williams@petalogix.com> wrote:
>> Hi Grant (it's your driver :) and David D ( and your DTS generator :)
> [...]
>> if (of_find_property(op->node, "8-bit", NULL))
> [...]
>> doesn't match the properties generated by Xilinx's device tree generator:
> [...]
>>                         xlnx,mem-width = <0x8>;
>>
>> So, the question is, which should change?
>
> Well, obviously its a device tree generator bug, the driver code is
> perfect.  :-P

How did I know you'd say that? :)

> In all seriousness though, the '8-bit' property has been present for a
> while now and needs to be retained to not break existing users.  It
> would be okay for the driver to be modified to *also* check for the
> xlnx,mem-width property, but the better solution is to modify the
> device tree generator.  Plus, the '8-bit' property is the documented
> binding in Documentation/powerpc/dts-bindings/xilinx.txt

I tend to agree - but I also tend think that putting funky, arbitrary
output strings in the DTS generator is also a bad idea.

In this case, my vote is leaning towards adding a test for
"xlnx,mem-width = <..>" to the driver, in addition to the existing
"8-bit" tag to keep back compatability.

How would you feel about that?

In future, for new Xilinx drivers I think the default binding should
be inspired by what comes from the DTS generator automatically, with
out any special per-device hacks.  This has a natural tendency towards
minimum pain I think.

John
-- 
John Williams, PhD, B.Eng, B.IT
PetaLogix - Linux Solutions for a Reconfigurable World
w: www.petalogix.com  p: +61-7-30090663  f: +61-7-30090663

RE: Xilinx SYSACE driver and 8-bit attachment

[John Linn]

> -----Original Message-----
> From: John Williams [mailto:john.williams@petalogix.com]
> Sent: Wednesday, October 07, 2009 5:45 PM
> To: grant.likely@secretlab.ca
> Cc: Linux Kernel list; Michal Simek; John Linn; David DeBonis; Stephen Neuendorffer; devicetree-
> discuss
> Subject: Re: Xilinx SYSACE driver and 8-bit attachment
> 
> On Wed, Oct 7, 2009 at 11:34 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> > [Added devicetree-discuss to cc: list]
> >
> > On Wed, Oct 7, 2009 at 1:13 AM, John Williams
> > <john.williams@petalogix.com> wrote:
> >> Hi Grant (it's your driver :) and David D ( and your DTS generator :)
> > [...]
> >> if (of_find_property(op->node, "8-bit", NULL))
> > [...]
> >> doesn't match the properties generated by Xilinx's device tree generator:
> > [...]
> >>                         xlnx,mem-width = <0x8>;
> >>
> >> So, the question is, which should change?
> >
> > Well, obviously its a device tree generator bug, the driver code is
> > perfect.  :-P
> 
> How did I know you'd say that? :)
> 
> > In all seriousness though, the '8-bit' property has been present for a
> > while now and needs to be retained to not break existing users.  It
> > would be okay for the driver to be modified to *also* check for the
> > xlnx,mem-width property, but the better solution is to modify the
> > device tree generator.  Plus, the '8-bit' property is the documented
> > binding in Documentation/powerpc/dts-bindings/xilinx.txt
> 
> I tend to agree - but I also tend think that putting funky, arbitrary
> output strings in the DTS generator is also a bad idea.
> 
> In this case, my vote is leaning towards adding a test for
> "xlnx,mem-width = <..>" to the driver, in addition to the existing
> "8-bit" tag to keep back compatability.
> 
> How would you feel about that?
> 
> In future, for new Xilinx drivers I think the default binding should
> be inspired by what comes from the DTS generator automatically, with
> out any special per-device hacks.  This has a natural tendency towards
> minimum pain I think.

Agreed. This was special legacy is the only reason it warranted it.

> 
> John
> --
> John Williams, PhD, B.Eng, B.IT
> PetaLogix - Linux Solutions for a Reconfigurable World
> w: www.petalogix.com  p: +61-7-30090663  f: +61-7-30090663


This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.

RE: Xilinx SYSACE driver and 8-bit attachment

[Stephen Neuendorffer]

> -----Original Message-----
> From: John Williams [mailto:john.williams@petalogix.com]
> Sent: Wednesday, October 07, 2009 4:45 PM
> To: grant.likely@secretlab.ca
> Cc: Linux Kernel list; Michal Simek; John Linn; David DeBonis; Stephen Neuendorffer; devicetree-
> discuss
> Subject: Re: Xilinx SYSACE driver and 8-bit attachment
> 
> On Wed, Oct 7, 2009 at 11:34 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> > [Added devicetree-discuss to cc: list]
> >
> > On Wed, Oct 7, 2009 at 1:13 AM, John Williams
> > <john.williams@petalogix.com> wrote:
> >> Hi Grant (it's your driver :) and David D ( and your DTS generator :)
> > [...]
> >> if (of_find_property(op->node, "8-bit", NULL))
> > [...]
> >> doesn't match the properties generated by Xilinx's device tree generator:
> > [...]
> >>                         xlnx,mem-width = <0x8>;
> >>
> >> So, the question is, which should change?
> >
> > Well, obviously its a device tree generator bug, the driver code is
> > perfect.  :-P
> 
> How did I know you'd say that? :)
> 
> > In all seriousness though, the '8-bit' property has been present for a
> > while now and needs to be retained to not break existing users.  It
> > would be okay for the driver to be modified to *also* check for the
> > xlnx,mem-width property, but the better solution is to modify the
> > device tree generator.  Plus, the '8-bit' property is the documented
> > binding in Documentation/powerpc/dts-bindings/xilinx.txt
> 
> I tend to agree - but I also tend think that putting funky, arbitrary
> output strings in the DTS generator is also a bad idea.
> 
> In this case, my vote is leaning towards adding a test for
> "xlnx,mem-width = <..>" to the driver, in addition to the existing
> "8-bit" tag to keep back compatability.
> 
> How would you feel about that?
> 
> In future, for new Xilinx drivers I think the default binding should
> be inspired by what comes from the DTS generator automatically, with
> out any special per-device hacks.  This has a natural tendency towards
> minimum pain I think.

I second this wholeheartedly.
Note that by definition, what comes out of the generator 'accurately and exactly describes the hardware', to the extent that the hardware is represented completely by its parameters.

Steve


This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.