Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]


Groups > linux.kernel > #1494758 > unrolled thread

[PATCH 3/3] net: smsc911x: add u16 workaround for pxa platforms

Started byRobert Jarzmik <robert.jarzmik@free.fr>
First post2016-10-03 11:10 +0200
Last post2016-10-09 03:40 +0200
Articles 12 — 5 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.


Contents

  [PATCH 3/3] net: smsc911x: add u16 workaround for pxa platforms Robert Jarzmik <robert.jarzmik@free.fr> - 2016-10-03 11:10 +0200
    Re: [PATCH 3/3] net: smsc911x: add u16 workaround for pxa platforms Jeremy Linton <jeremy.linton@arm.com> - 2016-10-03 17:30 +0200
      Re: [PATCH 3/3] net: smsc911x: add u16 workaround for pxa platforms Robert Jarzmik <robert.jarzmik@free.fr> - 2016-10-03 18:20 +0200
    Re: [PATCH 3/3] net: smsc911x: add u16 workaround for pxa platforms Mark Rutland <mark.rutland@arm.com> - 2016-10-03 17:50 +0200
      Re: [PATCH 3/3] net: smsc911x: add u16 workaround for pxa platforms Robert Jarzmik <robert.jarzmik@free.fr> - 2016-10-03 18:20 +0200
        Re: [PATCH 3/3] net: smsc911x: add u16 workaround for pxa platforms Mark Rutland <mark.rutland@arm.com> - 2016-10-03 19:00 +0200
          Re: [PATCH 3/3] net: smsc911x: add u16 workaround for pxa platforms Robert Jarzmik <robert.jarzmik@free.fr> - 2016-10-03 21:20 +0200
            Re: [PATCH 3/3] net: smsc911x: add u16 workaround for pxa platforms Robert Jarzmik <robert.jarzmik@free.fr> - 2016-10-06 08:50 +0200
              Re: [PATCH 3/3] net: smsc911x: add u16 workaround for pxa platforms Rob Herring <robh@kernel.org> - 2016-10-09 03:40 +0200
      Re: [PATCH 3/3] net: smsc911x: add u16 workaround for pxa platforms Russell King - ARM Linux <linux@armlinux.org.uk> - 2016-10-03 18:20 +0200
        Re: [PATCH 3/3] net: smsc911x: add u16 workaround for pxa platforms Mark Rutland <mark.rutland@arm.com> - 2016-10-03 18:50 +0200
          Re: [PATCH 3/3] net: smsc911x: add u16 workaround for pxa platforms Rob Herring <robh@kernel.org> - 2016-10-09 03:40 +0200

#1494758 — [PATCH 3/3] net: smsc911x: add u16 workaround for pxa platforms

FromRobert Jarzmik <robert.jarzmik@free.fr>
Date2016-10-03 11:10 +0200
Subject[PATCH 3/3] net: smsc911x: add u16 workaround for pxa platforms
Message-ID<so7q2-2Wv-27@gated-at.bofh.it>
Add a workaround for mainstone, idp and stargate2 boards, for u16 writes
which must be aligned on 32 bits addresses.

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
 Documentation/devicetree/bindings/net/smsc911x.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/smsc911x.txt b/Documentation/devicetree/bindings/net/smsc911x.txt
index 3fed3c124411..224965b7453c 100644
--- a/Documentation/devicetree/bindings/net/smsc911x.txt
+++ b/Documentation/devicetree/bindings/net/smsc911x.txt
@@ -13,6 +13,8 @@ Optional properties:
 - reg-io-width : Specify the size (in bytes) of the IO accesses that
   should be performed on the device.  Valid value for SMSC LAN is
   2 or 4.  If it's omitted or invalid, the size would be 2.
+- reg-u16-align4 : Boolean, put in place the workaround the force all
+  		   u16 writes to be 32 bits aligned
 - smsc,irq-active-high : Indicates the IRQ polarity is active-high
 - smsc,irq-push-pull : Indicates the IRQ type is push-pull
 - smsc,force-internal-phy : Forces SMSC LAN controller to use
-- 
2.1.4

[toc] | [next] | [standalone]


#1494914

FromJeremy Linton <jeremy.linton@arm.com>
Date2016-10-03 17:30 +0200
Message-ID<sodlL-6F3-3@gated-at.bofh.it>
In reply to#1494758
Hi Robert,

On 10/03/2016 04:05 AM, Robert Jarzmik wrote:
> Add a workaround for mainstone, idp and stargate2 boards, for u16 writes
> which must be aligned on 32 bits addresses.
>
> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
> ---
>  Documentation/devicetree/bindings/net/smsc911x.txt | 2 ++
>  1 file changed, 2 insertions(+)

I think this might be the wrong doc file. I think you want the 
smsc-lan91c111.txt file.


Thanks,

>
> diff --git a/Documentation/devicetree/bindings/net/smsc911x.txt b/Documentation/devicetree/bindings/net/smsc911x.txt
> index 3fed3c124411..224965b7453c 100644
> --- a/Documentation/devicetree/bindings/net/smsc911x.txt
> +++ b/Documentation/devicetree/bindings/net/smsc911x.txt
> @@ -13,6 +13,8 @@ Optional properties:
>  - reg-io-width : Specify the size (in bytes) of the IO accesses that
>    should be performed on the device.  Valid value for SMSC LAN is
>    2 or 4.  If it's omitted or invalid, the size would be 2.
> +- reg-u16-align4 : Boolean, put in place the workaround the force all
> +  		   u16 writes to be 32 bits aligned
>  - smsc,irq-active-high : Indicates the IRQ polarity is active-high
>  - smsc,irq-push-pull : Indicates the IRQ type is push-pull
>  - smsc,force-internal-phy : Forces SMSC LAN controller to use
>

[toc] | [prev] | [next] | [standalone]


#1494954

FromRobert Jarzmik <robert.jarzmik@free.fr>
Date2016-10-03 18:20 +0200
Message-ID<soe8a-7gD-5@gated-at.bofh.it>
In reply to#1494914
Jeremy Linton <jeremy.linton@arm.com> writes:

> Hi Robert,
>
> On 10/03/2016 04:05 AM, Robert Jarzmik wrote:
>> Add a workaround for mainstone, idp and stargate2 boards, for u16 writes
>> which must be aligned on 32 bits addresses.
>>
>> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
>> ---
>>  Documentation/devicetree/bindings/net/smsc911x.txt | 2 ++
>>  1 file changed, 2 insertions(+)
>
> I think this might be the wrong doc file. I think you want the
> smsc-lan91c111.txt file.
Ah yes, thanks for spoting that.

Cheers.

--
Robert

[toc] | [prev] | [next] | [standalone]


#1494921

FromMark Rutland <mark.rutland@arm.com>
Date2016-10-03 17:50 +0200
Message-ID<sodF7-6Lm-9@gated-at.bofh.it>
In reply to#1494758
On Mon, Oct 03, 2016 at 11:05:53AM +0200, Robert Jarzmik wrote:
> Add a workaround for mainstone, idp and stargate2 boards, for u16 writes
> which must be aligned on 32 bits addresses.
> 
> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
> ---
>  Documentation/devicetree/bindings/net/smsc911x.txt | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/smsc911x.txt b/Documentation/devicetree/bindings/net/smsc911x.txt
> index 3fed3c124411..224965b7453c 100644
> --- a/Documentation/devicetree/bindings/net/smsc911x.txt
> +++ b/Documentation/devicetree/bindings/net/smsc911x.txt
> @@ -13,6 +13,8 @@ Optional properties:
>  - reg-io-width : Specify the size (in bytes) of the IO accesses that
>    should be performed on the device.  Valid value for SMSC LAN is
>    2 or 4.  If it's omitted or invalid, the size would be 2.
> +- reg-u16-align4 : Boolean, put in place the workaround the force all
> +  		   u16 writes to be 32 bits aligned

This property name and description is confusing.

How exactly does this differ from having reg-io-width = <4>, which is
documented immediately above?

Thanks,
Mark.

>  - smsc,irq-active-high : Indicates the IRQ polarity is active-high
>  - smsc,irq-push-pull : Indicates the IRQ type is push-pull
>  - smsc,force-internal-phy : Forces SMSC LAN controller to use
> -- 
> 2.1.4
> 

[toc] | [prev] | [next] | [standalone]


#1494956

FromRobert Jarzmik <robert.jarzmik@free.fr>
Date2016-10-03 18:20 +0200
Message-ID<soe8a-7gD-9@gated-at.bofh.it>
In reply to#1494921
Mark Rutland <mark.rutland@arm.com> writes:

> On Mon, Oct 03, 2016 at 11:05:53AM +0200, Robert Jarzmik wrote:
>> Add a workaround for mainstone, idp and stargate2 boards, for u16 writes
>> which must be aligned on 32 bits addresses.
>> 
>> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
>> ---
>>  Documentation/devicetree/bindings/net/smsc911x.txt | 2 ++
>>  1 file changed, 2 insertions(+)
>> 
>> diff --git a/Documentation/devicetree/bindings/net/smsc911x.txt b/Documentation/devicetree/bindings/net/smsc911x.txt
>> index 3fed3c124411..224965b7453c 100644
>> --- a/Documentation/devicetree/bindings/net/smsc911x.txt
>> +++ b/Documentation/devicetree/bindings/net/smsc911x.txt
>> @@ -13,6 +13,8 @@ Optional properties:
>>  - reg-io-width : Specify the size (in bytes) of the IO accesses that
>>    should be performed on the device.  Valid value for SMSC LAN is
>>    2 or 4.  If it's omitted or invalid, the size would be 2.
>> +- reg-u16-align4 : Boolean, put in place the workaround the force all
>> +  		   u16 writes to be 32 bits aligned
>
> This property name and description is confusing.
>
> How exactly does this differ from having reg-io-width = <4>, which is
> documented immediately above?

reg-io-width specifies the IO size, ie. how many data lines are physically
connected from the system bus to the lan adapter.

reg-u16-align4 tells that a specific hardware doesn't support 16 bit writes not
being 32 bits aligned, or said differently that a "store" 16 bits wide on an
address of the format 4*n + 2 deserves a special handling in the driver, while a
store 16 bits wide on an address of the format 4*n can follow the simple casual
case.

I'm pretty open to any name you might suggest, these 3 hardwares I know of are
really crazy, you can see them in patch 1/3, in the _SMC_outw_align4() function
...

Cheers.

--
Robert

[toc] | [prev] | [next] | [standalone]


#1494968

FromMark Rutland <mark.rutland@arm.com>
Date2016-10-03 19:00 +0200
Message-ID<soeKR-7tO-3@gated-at.bofh.it>
In reply to#1494956
On Mon, Oct 03, 2016 at 06:11:23PM +0200, Robert Jarzmik wrote:
> Mark Rutland <mark.rutland@arm.com> writes:
> 
> > On Mon, Oct 03, 2016 at 11:05:53AM +0200, Robert Jarzmik wrote:
> >> Add a workaround for mainstone, idp and stargate2 boards, for u16 writes
> >> which must be aligned on 32 bits addresses.
> >> 
> >> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
> >> ---
> >>  Documentation/devicetree/bindings/net/smsc911x.txt | 2 ++
> >>  1 file changed, 2 insertions(+)
> >> 
> >> diff --git a/Documentation/devicetree/bindings/net/smsc911x.txt b/Documentation/devicetree/bindings/net/smsc911x.txt
> >> index 3fed3c124411..224965b7453c 100644
> >> --- a/Documentation/devicetree/bindings/net/smsc911x.txt
> >> +++ b/Documentation/devicetree/bindings/net/smsc911x.txt
> >> @@ -13,6 +13,8 @@ Optional properties:
> >>  - reg-io-width : Specify the size (in bytes) of the IO accesses that
> >>    should be performed on the device.  Valid value for SMSC LAN is
> >>    2 or 4.  If it's omitted or invalid, the size would be 2.
> >> +- reg-u16-align4 : Boolean, put in place the workaround the force all
> >> +  		   u16 writes to be 32 bits aligned
> >
> > This property name and description is confusing.
> >
> > How exactly does this differ from having reg-io-width = <4>, which is
> > documented immediately above?
> 
> reg-io-width specifies the IO size, ie. how many data lines are physically
> connected from the system bus to the lan adapter.
> 
> reg-u16-align4 tells that a specific hardware doesn't support 16 bit writes not
> being 32 bits aligned, or said differently that a "store" 16 bits wide on an
> address of the format 4*n + 2 deserves a special handling in the driver, while a
> store 16 bits wide on an address of the format 4*n can follow the simple casual
> case.

If I've understood correctly, effectively the low 2 address lines to the
device are hard-wired to zero, e.g. a 16-bit access to 4*n + 2 would go
to 4*n + 0 on the device? Or is the failure case distinct from that?

Do we have other platforms where similar is true? e.g. u8 accesses
requiring 16-bit alignment?

Thanks,
Mark.

[toc] | [prev] | [next] | [standalone]


#1495004

FromRobert Jarzmik <robert.jarzmik@free.fr>
Date2016-10-03 21:20 +0200
Message-ID<sogWl-uQ-17@gated-at.bofh.it>
In reply to#1494968
Mark Rutland <mark.rutland@arm.com> writes:

> On Mon, Oct 03, 2016 at 06:11:23PM +0200, Robert Jarzmik wrote:
>> Mark Rutland <mark.rutland@arm.com> writes:
>> 
>> reg-u16-align4 tells that a specific hardware doesn't support 16 bit writes not
>> being 32 bits aligned, or said differently that a "store" 16 bits wide on an
>> address of the format 4*n + 2 deserves a special handling in the driver, while a
>> store 16 bits wide on an address of the format 4*n can follow the simple casual
>> case.
>
> If I've understood correctly, effectively the low 2 address lines to the
> device are hard-wired to zero, e.g. a 16-bit access to 4*n + 2 would go
> to 4*n + 0 on the device? Or is the failure case distinct from that?
It is distinct.

The "awful truth" is that an FPGA lies between the system bus and the
smc91c111. And this FPGA cannot handle correctly the 4*n + 2 u16 writes.

> Do we have other platforms where similar is true? e.g. u8 accesses
> requiring 16-bit alignment?

Not really, ie. not with a alignement requirement.

But there are of course these ones are handled by reg-io-width and the
SMC_USE_xxx_BITS flags as far as I understand it. These cases are when a
platform declares SMC91X_USE_16BIT or SMC91X_USE_32BIT, but not SMC91X_USE_8BIT,
which would make me think of :
 - CONFIG_SH_SH4202_MICRODEV,
 - CONFIG_M32R
 - several omap1 boards
 - 1 sa1100 board
 - several MMP and realview boards

With all these platforms, each u8 access is replaced with a u16 access and a
mask / shift + mask.

Cheers.

-- 
Robert

[toc] | [prev] | [next] | [standalone]


#1496180

FromRobert Jarzmik <robert.jarzmik@free.fr>
Date2016-10-06 08:50 +0200
Message-ID<spaFb-4Sw-3@gated-at.bofh.it>
In reply to#1495004
Robert Jarzmik <robert.jarzmik@free.fr> writes:

> Mark Rutland <mark.rutland@arm.com> writes:
>
>> On Mon, Oct 03, 2016 at 06:11:23PM +0200, Robert Jarzmik wrote:
>>> Mark Rutland <mark.rutland@arm.com> writes:
>>> 
>>> reg-u16-align4 tells that a specific hardware doesn't support 16 bit writes not
>>> being 32 bits aligned, or said differently that a "store" 16 bits wide on an
>>> address of the format 4*n + 2 deserves a special handling in the driver, while a
>>> store 16 bits wide on an address of the format 4*n can follow the simple casual
>>> case.
>>
>> If I've understood correctly, effectively the low 2 address lines to the
>> device are hard-wired to zero, e.g. a 16-bit access to 4*n + 2 would go
>> to 4*n + 0 on the device? Or is the failure case distinct from that?
> It is distinct.
>
> The "awful truth" is that an FPGA lies between the system bus and the
> smc91c111. And this FPGA cannot handle correctly the 4*n + 2 u16 writes.
>
>> Do we have other platforms where similar is true? e.g. u8 accesses
>> requiring 16-bit alignment?
>
> Not really, ie. not with a alignement requirement.
>
> But there are of course these ones are handled by reg-io-width and the
> SMC_USE_xxx_BITS flags as far as I understand it. These cases are when a
> platform declares SMC91X_USE_16BIT or SMC91X_USE_32BIT, but not SMC91X_USE_8BIT,
> which would make me think of :
>  - CONFIG_SH_SH4202_MICRODEV,
>  - CONFIG_M32R
>  - several omap1 boards
>  - 1 sa1100 board
>  - several MMP and realview boards
>
> With all these platforms, each u8 access is replaced with a u16 access and a
> mask / shift + mask.

Or so what should I call this entry if reg-u16-align4 is not a good candidate ?

Cheers.

-- 
Robert

[toc] | [prev] | [next] | [standalone]


#1497843

FromRob Herring <robh@kernel.org>
Date2016-10-09 03:40 +0200
Message-ID<sqbfQ-6a9-7@gated-at.bofh.it>
In reply to#1496180
On Thu, Oct 06, 2016 at 08:47:13AM +0200, Robert Jarzmik wrote:
> Robert Jarzmik <robert.jarzmik@free.fr> writes:
> 
> > Mark Rutland <mark.rutland@arm.com> writes:
> >
> >> On Mon, Oct 03, 2016 at 06:11:23PM +0200, Robert Jarzmik wrote:
> >>> Mark Rutland <mark.rutland@arm.com> writes:
> >>> 
> >>> reg-u16-align4 tells that a specific hardware doesn't support 16 bit writes not
> >>> being 32 bits aligned, or said differently that a "store" 16 bits wide on an
> >>> address of the format 4*n + 2 deserves a special handling in the driver, while a
> >>> store 16 bits wide on an address of the format 4*n can follow the simple casual
> >>> case.
> >>
> >> If I've understood correctly, effectively the low 2 address lines to the
> >> device are hard-wired to zero, e.g. a 16-bit access to 4*n + 2 would go
> >> to 4*n + 0 on the device? Or is the failure case distinct from that?
> > It is distinct.
> >
> > The "awful truth" is that an FPGA lies between the system bus and the
> > smc91c111. And this FPGA cannot handle correctly the 4*n + 2 u16 writes.
> >
> >> Do we have other platforms where similar is true? e.g. u8 accesses
> >> requiring 16-bit alignment?
> >
> > Not really, ie. not with a alignement requirement.
> >
> > But there are of course these ones are handled by reg-io-width and the
> > SMC_USE_xxx_BITS flags as far as I understand it. These cases are when a
> > platform declares SMC91X_USE_16BIT or SMC91X_USE_32BIT, but not SMC91X_USE_8BIT,
> > which would make me think of :
> >  - CONFIG_SH_SH4202_MICRODEV,
> >  - CONFIG_M32R
> >  - several omap1 boards
> >  - 1 sa1100 board
> >  - several MMP and realview boards
> >
> > With all these platforms, each u8 access is replaced with a u16 access and a
> > mask / shift + mask.
> 
> Or so what should I call this entry if reg-u16-align4 is not a good candidate ?

This seems broken in a very platform specific way, so perhaps something 
named based on the platform.

Rob

[toc] | [prev] | [next] | [standalone]


#1494959

FromRussell King - ARM Linux <linux@armlinux.org.uk>
Date2016-10-03 18:20 +0200
Message-ID<soe8a-7gD-11@gated-at.bofh.it>
In reply to#1494921
On Mon, Oct 03, 2016 at 04:46:25PM +0100, Mark Rutland wrote:
> On Mon, Oct 03, 2016 at 11:05:53AM +0200, Robert Jarzmik wrote:
> > Add a workaround for mainstone, idp and stargate2 boards, for u16 writes
> > which must be aligned on 32 bits addresses.
> > 
> > Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
> > ---
> >  Documentation/devicetree/bindings/net/smsc911x.txt | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/net/smsc911x.txt b/Documentation/devicetree/bindings/net/smsc911x.txt
> > index 3fed3c124411..224965b7453c 100644
> > --- a/Documentation/devicetree/bindings/net/smsc911x.txt
> > +++ b/Documentation/devicetree/bindings/net/smsc911x.txt
> > @@ -13,6 +13,8 @@ Optional properties:
> >  - reg-io-width : Specify the size (in bytes) of the IO accesses that
> >    should be performed on the device.  Valid value for SMSC LAN is
> >    2 or 4.  If it's omitted or invalid, the size would be 2.
> > +- reg-u16-align4 : Boolean, put in place the workaround the force all
> > +  		   u16 writes to be 32 bits aligned
> 
> This property name and description is confusing.
> 
> How exactly does this differ from having reg-io-width = <4>, which is
> documented immediately above?

Please note that the binding doc for smsc,lan91c111.txt is slightly wrong
on two counts:

1) compatible property:

compatible = "smsc,lan91c111";

vs the code:

static const struct of_device_id smc91x_match[] = {
        { .compatible = "smsc,lan91c94", },
        { .compatible = "smsc,lan91c111", },
        {},
};
MODULE_DEVICE_TABLE(of, smc91x_match);

So the binding document needs to mention that smsc,lan91c94 is a valid
compatible for this device.

2) reg-io-width property:

- reg-io-width : Mask of sizes (in bytes) of the IO accesses that
  are supported on the device.  Valid value for SMSC LAN91c111 are
  1, 2 or 4.  If it's omitted or invalid, the size would be 2 meaning
  16-bit access only.

The SMC requires at least one of byte or 16-bit access sizes, with
32-bit access sizes being optional on top.  So, the legal values here
are: 1, 2, 3, 5, 6, and 7.  4 is illegal, and has never been supported
by the driver.

Note that the driver will always use byte accesses if '1' is specified
and emulate 16-bit accesses.  If '2' is specified, the driver will
always use 16-bit accesses, and emulate byte accesses for the 8-bit
registers using a read-modify-write scheme.  If '3' is specified, the
driver will use both 16-bit and byte accesses as appropriate for the
register being accessed with no emulation.  Byte or 16-bit access are
required for non-data register access.

Including 32-bit accesses on top of this allows the packet transfer
(iow, data register accesses) to use 32-bit access instructions, which
is a performance boost.

Moreover, look at the property name vs the binding description.  It's
property name says it's a width, but the description says it's a mask
of sizes - these really aren't the same thing.  Once you start
specifying these other legal masks, it makes a nonsense of the "width"
part of the name.  It's too late to try and fix this now though.

The binding document really needs to get fixed - I'll try to cook up a
patch during this week to correct these points, but it probably needs
coordination if others are going to be changing this as well.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[toc] | [prev] | [next] | [standalone]


#1494965

FromMark Rutland <mark.rutland@arm.com>
Date2016-10-03 18:50 +0200
Message-ID<soeBb-7qE-9@gated-at.bofh.it>
In reply to#1494959
On Mon, Oct 03, 2016 at 05:09:13PM +0100, Russell King - ARM Linux wrote:
> Please note that the binding doc for smsc,lan91c111.txt is slightly wrong
> on two counts:
> 
> 1) compatible property:
> 
> compatible = "smsc,lan91c111";
> 
> vs the code:
> 
> static const struct of_device_id smc91x_match[] = {
>         { .compatible = "smsc,lan91c94", },
>         { .compatible = "smsc,lan91c111", },
>         {},
> };
> MODULE_DEVICE_TABLE(of, smc91x_match);
> 
> So the binding document needs to mention that smsc,lan91c94 is a valid
> compatible for this device.

Yes, it should.

> 2) reg-io-width property:
> 
> - reg-io-width : Mask of sizes (in bytes) of the IO accesses that
>   are supported on the device.  Valid value for SMSC LAN91c111 are
>   1, 2 or 4.  If it's omitted or invalid, the size would be 2 meaning
>   16-bit access only.
 
> Moreover, look at the property name vs the binding description.  It's
> property name says it's a width, but the description says it's a mask
> of sizes - these really aren't the same thing.  Once you start
> specifying these other legal masks, it makes a nonsense of the "width"
> part of the name.  It's too late to try and fix this now though.

Indeed, as-is this is nonsense. :(

The best we can do here is to add a big fat notice regarding the
misnaming; adding a new property is only giong to cause more confusion.

> The binding document really needs to get fixed - I'll try to cook up a
> patch during this week to correct these points, but it probably needs
> coordination if others are going to be changing this as well.

Thanks for handling both of these.

Given the historical rate of change of the binding document, I suspect
the stuff for pxa platforms is going to be the only potential conflict.

Either all of that can go via the DT tree (independent of any new code),
or we can ack the whole lot and it can all go via the net tree in one
go.

Thanks,
Mark.

[toc] | [prev] | [next] | [standalone]


#1497841

FromRob Herring <robh@kernel.org>
Date2016-10-09 03:40 +0200
Message-ID<sqbfQ-6a9-9@gated-at.bofh.it>
In reply to#1494965
On Mon, Oct 03, 2016 at 05:42:29PM +0100, Mark Rutland wrote:
> On Mon, Oct 03, 2016 at 05:09:13PM +0100, Russell King - ARM Linux wrote:
> > Please note that the binding doc for smsc,lan91c111.txt is slightly wrong
> > on two counts:
> > 
> > 1) compatible property:
> > 
> > compatible = "smsc,lan91c111";
> > 
> > vs the code:
> > 
> > static const struct of_device_id smc91x_match[] = {
> >         { .compatible = "smsc,lan91c94", },
> >         { .compatible = "smsc,lan91c111", },
> >         {},
> > };
> > MODULE_DEVICE_TABLE(of, smc91x_match);
> > 
> > So the binding document needs to mention that smsc,lan91c94 is a valid
> > compatible for this device.
> 
> Yes, it should.
> 
> > 2) reg-io-width property:
> > 
> > - reg-io-width : Mask of sizes (in bytes) of the IO accesses that
> >   are supported on the device.  Valid value for SMSC LAN91c111 are
> >   1, 2 or 4.  If it's omitted or invalid, the size would be 2 meaning
> >   16-bit access only.
>  
> > Moreover, look at the property name vs the binding description.  It's
> > property name says it's a width, but the description says it's a mask
> > of sizes - these really aren't the same thing.  Once you start
> > specifying these other legal masks, it makes a nonsense of the "width"
> > part of the name.  It's too late to try and fix this now though.
> 
> Indeed, as-is this is nonsense. :(
> 
> The best we can do here is to add a big fat notice regarding the
> misnaming; adding a new property is only giong to cause more confusion.

Just fix the text here removing the mask part. This is a common property 
and not a mask. The rest saying 1, 2, 4 being valid is correct. There 
are no occurences using this as a mask in kernel dts files either.

> 
> > The binding document really needs to get fixed - I'll try to cook up a
> > patch during this week to correct these points, but it probably needs
> > coordination if others are going to be changing this as well.
> 
> Thanks for handling both of these.
> 
> Given the historical rate of change of the binding document, I suspect
> the stuff for pxa platforms is going to be the only potential conflict.
> 
> Either all of that can go via the DT tree (independent of any new code),
> or we can ack the whole lot and it can all go via the net tree in one
> go.
> 
> Thanks,
> Mark.

[toc] | [prev] | [standalone]


Back to top | Article view | linux.kernel


csiph-web