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

[Rob Herring]

On Thu, Oct 06, 2016 at 08:47:13AM +0200, Robert Jarzmik wrote:
> Robert Jarzmik  writes:
> 
> > Mark Rutland  writes:
> >
> >> On Mon, Oct 03, 2016 at 06:11:23PM +0200, Robert Jarzmik wrote:
> >>> Mark Rutland  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

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

[Rob Herring]

On Thu, Oct 06, 2016 at 08:47:13AM +0200, Robert Jarzmik wrote:
> Robert Jarzmik  writes:
> 
> > Mark Rutland  writes:
> >
> >> On Mon, Oct 03, 2016 at 06:11:23PM +0200, Robert Jarzmik wrote:
> >>> Mark Rutland  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

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

[Rob Herring]

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.

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

[Rob Herring]

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.

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

[Robert Jarzmik]

Robert Jarzmik  writes:

> Mark Rutland  writes:
>
>> On Mon, Oct 03, 2016 at 06:11:23PM +0200, Robert Jarzmik wrote:
>>> Mark Rutland  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

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

[Robert Jarzmik]

Robert Jarzmik  writes:

> Mark Rutland  writes:
>
>> On Mon, Oct 03, 2016 at 06:11:23PM +0200, Robert Jarzmik wrote:
>>> Mark Rutland  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

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

[Robert Jarzmik]

Mark Rutland  writes:

> On Mon, Oct 03, 2016 at 06:11:23PM +0200, Robert Jarzmik wrote:
>> Mark Rutland  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

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

[Robert Jarzmik]

Mark Rutland  writes:

> On Mon, Oct 03, 2016 at 06:11:23PM +0200, Robert Jarzmik wrote:
>> Mark Rutland  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

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

[Mark Rutland]

On Mon, Oct 03, 2016 at 06:11:23PM +0200, Robert Jarzmik wrote:
> Mark Rutland  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 
> >> ---
> >>  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.

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

[Mark Rutland]

On Mon, Oct 03, 2016 at 06:11:23PM +0200, Robert Jarzmik wrote:
> Mark Rutland  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 
> >> ---
> >>  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.

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

[Mark Rutland]

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.

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

[Mark Rutland]

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.

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

[Robert Jarzmik]

Jeremy Linton  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 
>> ---
>>  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

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

[Robert Jarzmik]

Jeremy Linton  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 
>> ---
>>  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

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

[Robert Jarzmik]

Mark Rutland  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 
>> ---
>>  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

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

[Robert Jarzmik]

Mark Rutland  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 
>> ---
>>  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

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

[Russell King - ARM Linux]

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 
> > ---
> >  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.

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

[Russell King - ARM Linux]

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 
> > ---
> >  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.

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

[Mark Rutland]

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 
> ---
>  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
> 

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

[Mark Rutland]

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 
> ---
>  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
> 

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

[Jeremy Linton]

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 
---
 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

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

[Jeremy Linton]

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 
---
 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