Re: [PATCH 3/3] net: smsc911x: add u16 workaround for pxa platforms
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
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 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
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
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
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
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
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
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
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
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
[PATCH 3/3] net: smsc911x: add u16 workaround for pxa platforms
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 - 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