| Message ID | 201308180313.26607.sergei.shtylyov@cogentembedded.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
Hi Sergei, Thank you for the patch. On Sunday 18 August 2013 03:13:26 Sergei Shtylyov wrote: > Now that the 'register_type' field of the 'sh_eth' driver's platform data is > not used by the driver anymore, it's time to remove it and its > initializers from the SH platform code. Also move *enum* declaring values > for this field from <linux/sh_eth.h> to the local driver's header file > as they're only needed by the driver itself now... > > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > > --- > arch/arm/mach-shmobile/board-armadillo800eva.c | 1 - > arch/arm/mach-shmobile/board-bockw.c | 1 - > arch/sh/boards/board-espt.c | 1 - > arch/sh/boards/board-sh7757lcr.c | 4 ---- > arch/sh/boards/mach-ecovec24/setup.c | 1 - > arch/sh/boards/mach-se/7724/setup.c | 1 - > arch/sh/boards/mach-sh7763rdp/setup.c | 1 - > arch/sh/kernel/cpu/sh2/setup-sh7619.c | 1 - > drivers/net/ethernet/renesas/sh_eth.h | 7 +++++++ > include/linux/sh_eth.h | 7 ------- > 10 files changed, 7 insertions(+), 18 deletions(-) [snip] > Index: net-next/drivers/net/ethernet/renesas/sh_eth.h > =================================================================== > --- net-next.orig/drivers/net/ethernet/renesas/sh_eth.h > +++ net-next/drivers/net/ethernet/renesas/sh_eth.h > @@ -157,6 +157,13 @@ enum { > SH_ETH_MAX_REGISTER_OFFSET, > }; > > +enum { > + SH_ETH_REG_GIGABIT, > + SH_ETH_REG_FAST_RCAR, > + SH_ETH_REG_FAST_SH4, > + SH_ETH_REG_FAST_SH3_SH2 > +}; > + Would it make sense to move this change and the one below to a separate patch to be merged through the net tree ? > /* Driver's parameters */ > #if defined(CONFIG_CPU_SH4) || defined(CONFIG_ARCH_SHMOBILE) > #define SH4_SKB_RX_ALIGN 32 > Index: net-next/include/linux/sh_eth.h > =================================================================== > --- net-next.orig/include/linux/sh_eth.h > +++ net-next/include/linux/sh_eth.h > @@ -5,17 +5,10 @@ > #include <linux/if_ether.h> > > enum {EDMAC_LITTLE_ENDIAN, EDMAC_BIG_ENDIAN}; > -enum { > - SH_ETH_REG_GIGABIT, > - SH_ETH_REG_FAST_RCAR, > - SH_ETH_REG_FAST_SH4, > - SH_ETH_REG_FAST_SH3_SH2 > -}; > > struct sh_eth_plat_data { > int phy; > int edmac_endian; Wouldn't it make sense to move the edmac_endian field to sh_eth_cpu_data as well ? > - int register_type; > phy_interface_t phy_interface; > void (*set_mdio_gate)(void *addr);
Hello. On 20-08-2013 14:51, Laurent Pinchart wrote: >> Now that the 'register_type' field of the 'sh_eth' driver's platform data is >> not used by the driver anymore, it's time to remove it and its >> initializers from the SH platform code. Also move *enum* declaring values >> for this field from <linux/sh_eth.h> to the local driver's header file >> as they're only needed by the driver itself now... >> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> >> --- >> arch/arm/mach-shmobile/board-armadillo800eva.c | 1 - >> arch/arm/mach-shmobile/board-bockw.c | 1 - >> arch/sh/boards/board-espt.c | 1 - >> arch/sh/boards/board-sh7757lcr.c | 4 ---- >> arch/sh/boards/mach-ecovec24/setup.c | 1 - >> arch/sh/boards/mach-se/7724/setup.c | 1 - >> arch/sh/boards/mach-sh7763rdp/setup.c | 1 - >> arch/sh/kernel/cpu/sh2/setup-sh7619.c | 1 - >> drivers/net/ethernet/renesas/sh_eth.h | 7 +++++++ >> include/linux/sh_eth.h | 7 ------- >> 10 files changed, 7 insertions(+), 18 deletions(-) > [snip] >> Index: net-next/drivers/net/ethernet/renesas/sh_eth.h >> =================================================================== >> --- net-next.orig/drivers/net/ethernet/renesas/sh_eth.h >> +++ net-next/drivers/net/ethernet/renesas/sh_eth.h >> @@ -157,6 +157,13 @@ enum { >> SH_ETH_MAX_REGISTER_OFFSET, >> }; >> >> +enum { >> + SH_ETH_REG_GIGABIT, >> + SH_ETH_REG_FAST_RCAR, >> + SH_ETH_REG_FAST_SH4, >> + SH_ETH_REG_FAST_SH3_SH2 >> +}; >> + > Would it make sense to move this change and the one below to a separate patch > to be merged through the net tree ? I'm intending to merge these patches thru the net-next tree. >> /* Driver's parameters */ >> #if defined(CONFIG_CPU_SH4) || defined(CONFIG_ARCH_SHMOBILE) >> #define SH4_SKB_RX_ALIGN 32 >> Index: net-next/include/linux/sh_eth.h >> =================================================================== >> --- net-next.orig/include/linux/sh_eth.h >> +++ net-next/include/linux/sh_eth.h >> @@ -5,17 +5,10 @@ >> #include <linux/if_ether.h> >> >> enum {EDMAC_LITTLE_ENDIAN, EDMAC_BIG_ENDIAN}; >> -enum { >> - SH_ETH_REG_GIGABIT, >> - SH_ETH_REG_FAST_RCAR, >> - SH_ETH_REG_FAST_SH4, >> - SH_ETH_REG_FAST_SH3_SH2 >> -}; >> >> struct sh_eth_plat_data { >> int phy; >> int edmac_endian; > Wouldn't it make sense to move the edmac_endian field to sh_eth_cpu_data as > well ? No, it depends on the SoC endianness which is determined by power-on pin strapping -- which is board specific. WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. On 08/20/2013 06:27 PM, Sergei Shtylyov wrote: >>> Now that the 'register_type' field of the 'sh_eth' driver's platform data is >>> not used by the driver anymore, it's time to remove it and its >>> initializers from the SH platform code. Also move *enum* declaring values >>> for this field from <linux/sh_eth.h> to the local driver's header file >>> as they're only needed by the driver itself now... >>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> [...] >>> /* Driver's parameters */ >>> #if defined(CONFIG_CPU_SH4) || defined(CONFIG_ARCH_SHMOBILE) >>> #define SH4_SKB_RX_ALIGN 32 >>> Index: net-next/include/linux/sh_eth.h >>> =================================================================== >>> --- net-next.orig/include/linux/sh_eth.h >>> +++ net-next/include/linux/sh_eth.h >>> @@ -5,17 +5,10 @@ >>> #include <linux/if_ether.h> >>> >>> enum {EDMAC_LITTLE_ENDIAN, EDMAC_BIG_ENDIAN}; >>> -enum { >>> - SH_ETH_REG_GIGABIT, >>> - SH_ETH_REG_FAST_RCAR, >>> - SH_ETH_REG_FAST_SH4, >>> - SH_ETH_REG_FAST_SH3_SH2 >>> -}; >>> >>> struct sh_eth_plat_data { >>> int phy; >>> int edmac_endian; >> Wouldn't it make sense to move the edmac_endian field to sh_eth_cpu_data as >> well ? > No, it depends on the SoC endianness which is determined by power-on pin > strapping -- which is board specific. BTW, I don't think the driver works correctly in the BE case since it uses io{read|write}32() to access the registers and those functions assume LE ordering on MMIO. WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Sergei, On Wednesday 21 August 2013 02:09:49 Sergei Shtylyov wrote: > On 08/20/2013 06:27 PM, Sergei Shtylyov wrote: > >>> Now that the 'register_type' field of the 'sh_eth' driver's platform > >>> data is not used by the driver anymore, it's time to remove it and its > >>> initializers from the SH platform code. Also move *enum* declaring > >>> values for this field from <linux/sh_eth.h> to the local driver's > >>> header file as they're only needed by the driver itself now... > >>> > >>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > > [...] > > >>> /* Driver's parameters */ > >>> #if defined(CONFIG_CPU_SH4) || defined(CONFIG_ARCH_SHMOBILE) > >>> #define SH4_SKB_RX_ALIGN 32 > >>> > >>> Index: net-next/include/linux/sh_eth.h > >>> =================================================================== > >>> --- net-next.orig/include/linux/sh_eth.h > >>> +++ net-next/include/linux/sh_eth.h > >>> @@ -5,17 +5,10 @@ > >>> > >>> #include <linux/if_ether.h> > >>> > >>> enum {EDMAC_LITTLE_ENDIAN, EDMAC_BIG_ENDIAN}; > >>> > >>> -enum { > >>> - SH_ETH_REG_GIGABIT, > >>> - SH_ETH_REG_FAST_RCAR, > >>> - SH_ETH_REG_FAST_SH4, > >>> - SH_ETH_REG_FAST_SH3_SH2 > >>> -}; > >>> > >>> struct sh_eth_plat_data { > >>> > >>> int phy; > >>> int edmac_endian; > >> > >> Wouldn't it make sense to move the edmac_endian field to sh_eth_cpu_data > >> as well ? > >> > > No, it depends on the SoC endianness which is determined by power-on pin > > strapping -- which is board specific. Does SoC endianness affect the ARM core endianness, the ethernet registers endianness, or both ? If it affects the ARM core endianness only, the kernel needs to be compiled in little-endian or big-endian mode anyway, and the sh_eth driver should use cpu_to_le32() unconditionally. If it affects both the ARM core and the ethernet controller there's not need to care about the endianness, as it will always be good. We only need to care about it if it affects the ethernet controller registers only, which would seem weird to me. > BTW, I don't think the driver works correctly in the BE case since it uses > io{read|write}32() to access the registers and those functions assume LE > ordering on MMIO.
On 08/21/2013 02:50 AM, Laurent Pinchart wrote: >>>>> Now that the 'register_type' field of the 'sh_eth' driver's platform >>>>> data is not used by the driver anymore, it's time to remove it and its >>>>> initializers from the SH platform code. Also move *enum* declaring >>>>> values for this field from <linux/sh_eth.h> to the local driver's >>>>> header file as they're only needed by the driver itself now... >>>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> >> [...] >>>>> /* Driver's parameters */ >>>>> #if defined(CONFIG_CPU_SH4) || defined(CONFIG_ARCH_SHMOBILE) >>>>> #define SH4_SKB_RX_ALIGN 32 >>>>> >>>>> Index: net-next/include/linux/sh_eth.h >>>>> =================================================================== >>>>> --- net-next.orig/include/linux/sh_eth.h >>>>> +++ net-next/include/linux/sh_eth.h >>>>> @@ -5,17 +5,10 @@ >>>>> >>>>> #include <linux/if_ether.h> >>>>> >>>>> enum {EDMAC_LITTLE_ENDIAN, EDMAC_BIG_ENDIAN}; >>>>> >>>>> -enum { >>>>> - SH_ETH_REG_GIGABIT, >>>>> - SH_ETH_REG_FAST_RCAR, >>>>> - SH_ETH_REG_FAST_SH4, >>>>> - SH_ETH_REG_FAST_SH3_SH2 >>>>> -}; >>>>> >>>>> struct sh_eth_plat_data { >>>>> >>>>> int phy; >>>>> int edmac_endian; >>>> Wouldn't it make sense to move the edmac_endian field to sh_eth_cpu_data >>>> as well ? >>> No, it depends on the SoC endianness which is determined by power-on pin >>> strapping -- which is board specific. > Does SoC endianness affect the ARM core endianness, the ethernet registers > endianness, or both ? Both, AFAIK. > If it affects the ARM core endianness only, the kernel > needs to be compiled in little-endian or big-endian mode anyway, and the > sh_eth driver should use cpu_to_le32() unconditionally. If it affects both the > ARM core and the ethernet controller there's not need to care about the > endianness, as it will always be good. No, it won't unless you're using __raw_{readl|writel}() accessors. The driver doesn't do it. {readl|writel}() and io{read|write}32() that use them always assume LE ordering of memory. > We only need to care about it if it > affects the ethernet controller registers only, which would seem weird to me. Unfortunately, you are wrong. >> BTW, I don't think the driver works correctly in the BE case since it uses >> io{read|write}32() to access the registers and those functions assume LE >> ordering on MMIO. WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> Date: Sun, 18 Aug 2013 03:13:26 +0400 > Now that the 'register_type' field of the 'sh_eth' driver's platform data is not > used by the driver anymore, it's time to remove it and its initializers from > the SH platform code. Also move *enum* declaring values for this field from > <linux/sh_eth.h> to the local driver's header file as they're only needed > by the driver itself now... > > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> Applied. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Sergei, On Wednesday 21 August 2013 03:01:28 Sergei Shtylyov wrote: > On 08/21/2013 02:50 AM, Laurent Pinchart wrote: > >>>>> Now that the 'register_type' field of the 'sh_eth' driver's platform > >>>>> data is not used by the driver anymore, it's time to remove it and > >>>>> its initializers from the SH platform code. Also move *enum* > >>>>> declaring values for this field from <linux/sh_eth.h> to the local > >>>>> driver's header file as they're only needed by the driver itself > >>>>> now... > >>>>> > >>>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > >> > >> [...] > >> > >>>>> /* Driver's parameters */ > >>>>> #if defined(CONFIG_CPU_SH4) || defined(CONFIG_ARCH_SHMOBILE) > >>>>> #define SH4_SKB_RX_ALIGN 32 > >>>>> > >>>>> Index: net-next/include/linux/sh_eth.h > >>>>> =================================================================== > >>>>> --- net-next.orig/include/linux/sh_eth.h > >>>>> +++ net-next/include/linux/sh_eth.h > >>>>> @@ -5,17 +5,10 @@ > >>>>> > >>>>> #include <linux/if_ether.h> > >>>>> > >>>>> enum {EDMAC_LITTLE_ENDIAN, EDMAC_BIG_ENDIAN}; > >>>>> > >>>>> -enum { > >>>>> - SH_ETH_REG_GIGABIT, > >>>>> - SH_ETH_REG_FAST_RCAR, > >>>>> - SH_ETH_REG_FAST_SH4, > >>>>> - SH_ETH_REG_FAST_SH3_SH2 > >>>>> -}; > >>>>> > >>>>> struct sh_eth_plat_data { > >>>>> > >>>>> int phy; > >>>>> int edmac_endian; > >>>> > >>>> Wouldn't it make sense to move the edmac_endian field to > >>>> sh_eth_cpu_data as well ? > >>> > >>> No, it depends on the SoC endianness which is determined by power-on pin > >>> strapping -- which is board specific. > > > > Does SoC endianness affect the ARM core endianness, the ethernet registers > > endianness, or both ? > > Both, AFAIK. > > > If it affects the ARM core endianness only, the kernel needs to be > > compiled in little-endian or big-endian mode anyway, and the sh_eth driver > > should use cpu_to_le32() unconditionally. If it affects both the ARM core > > and the ethernet controller there's not need to care about the endianness, > > as it will always be good. > > No, it won't unless you're using __raw_{readl|writel}() accessors. The > driver doesn't do it. {readl|writel}() and io{read|write}32() that use them > always assume LE ordering of memory. > > > We only need to care about it if it affects the ethernet controller > > registers only, which would seem weird to me. > > Unfortunately, you are wrong. Care to explain *why* ? There might be bugs in the driver (such as using the wrong I/O accessors), but I don't see why we need to configure the endianness through platform data. > >> BTW, I don't think the driver works correctly in the BE case since it > >> uses io{read|write}32() to access the registers and those functions > >> assume LE ordering on MMIO.
Hello. On 21-08-2013 4:39, Laurent Pinchart wrote: >>>>>>> Now that the 'register_type' field of the 'sh_eth' driver's platform >>>>>>> data is not used by the driver anymore, it's time to remove it and >>>>>>> its initializers from the SH platform code. Also move *enum* >>>>>>> declaring values for this field from <linux/sh_eth.h> to the local >>>>>>> driver's header file as they're only needed by the driver itself >>>>>>> now... >>>>>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> >>>> [...] >>>>>>> /* Driver's parameters */ >>>>>>> #if defined(CONFIG_CPU_SH4) || defined(CONFIG_ARCH_SHMOBILE) >>>>>>> #define SH4_SKB_RX_ALIGN 32 >>>>>>> >>>>>>> Index: net-next/include/linux/sh_eth.h >>>>>>> =================================================================== >>>>>>> --- net-next.orig/include/linux/sh_eth.h >>>>>>> +++ net-next/include/linux/sh_eth.h >>>>>>> @@ -5,17 +5,10 @@ >>>>>>> >>>>>>> #include <linux/if_ether.h> >>>>>>> >>>>>>> enum {EDMAC_LITTLE_ENDIAN, EDMAC_BIG_ENDIAN}; >>>>>>> >>>>>>> -enum { >>>>>>> - SH_ETH_REG_GIGABIT, >>>>>>> - SH_ETH_REG_FAST_RCAR, >>>>>>> - SH_ETH_REG_FAST_SH4, >>>>>>> - SH_ETH_REG_FAST_SH3_SH2 >>>>>>> -}; >>>>>>> >>>>>>> struct sh_eth_plat_data { >>>>>>> >>>>>>> int phy; >>>>>>> int edmac_endian; >>>>>> Wouldn't it make sense to move the edmac_endian field to >>>>>> sh_eth_cpu_data as well ? >>>>> No, it depends on the SoC endianness which is determined by power-on pin >>>>> strapping -- which is board specific. >>> Does SoC endianness affect the ARM core endianness, the ethernet registers >>> endianness, or both ? >> Both, AFAIK. >>> If it affects the ARM core endianness only, the kernel needs to be >>> compiled in little-endian or big-endian mode anyway, and the sh_eth driver >>> should use cpu_to_le32() unconditionally. If it affects both the ARM core >>> and the ethernet controller there's not need to care about the endianness, >>> as it will always be good. >> No, it won't unless you're using __raw_{readl|writel}() accessors. The >> driver doesn't do it. {readl|writel}() and io{read|write}32() that use them >> always assume LE ordering of memory. >>> We only need to care about it if it affects the ethernet controller >>> registers only, which would seem weird to me. >> Unfortunately, you are wrong. > Care to explain *why* ? There might be bugs in the driver (such as using the > wrong I/O accessors), but I don't see why we need to configure the endianness > through platform data. Re-read my reply about the power-on pin strapping please. The SoC endianness setting gets read from the external source to the SoC, i.e. it's determined by the board. WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Sergei, On Wednesday 21 August 2013 16:49:52 Sergei Shtylyov wrote: > On 21-08-2013 4:39, Laurent Pinchart wrote: > >>>>>>> Now that the 'register_type' field of the 'sh_eth' driver's platform > >>>>>>> data is not used by the driver anymore, it's time to remove it and > >>>>>>> its initializers from the SH platform code. Also move *enum* > >>>>>>> declaring values for this field from <linux/sh_eth.h> to the > >>>>>>> local driver's header file as they're only needed by the driver > >>>>>>> itself now... > >>>>>>> > >>>>>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > >>>> > >>>> [...] > >>>> > >>>>>>> /* Driver's parameters */ > >>>>>>> #if defined(CONFIG_CPU_SH4) || defined(CONFIG_ARCH_SHMOBILE) > >>>>>>> #define SH4_SKB_RX_ALIGN 32 > >>>>>>> > >>>>>>> Index: net-next/include/linux/sh_eth.h > >>>>>>> =================================================================== > >>>>>>> --- net-next.orig/include/linux/sh_eth.h > >>>>>>> +++ net-next/include/linux/sh_eth.h > >>>>>>> @@ -5,17 +5,10 @@ > >>>>>>> > >>>>>>> #include <linux/if_ether.h> > >>>>>>> > >>>>>>> enum {EDMAC_LITTLE_ENDIAN, EDMAC_BIG_ENDIAN}; > >>>>>>> > >>>>>>> -enum { > >>>>>>> - SH_ETH_REG_GIGABIT, > >>>>>>> - SH_ETH_REG_FAST_RCAR, > >>>>>>> - SH_ETH_REG_FAST_SH4, > >>>>>>> - SH_ETH_REG_FAST_SH3_SH2 > >>>>>>> -}; > >>>>>>> > >>>>>>> struct sh_eth_plat_data { > >>>>>>> > >>>>>>> int phy; > >>>>>>> int edmac_endian; > >>>>>> > >>>>>> Wouldn't it make sense to move the edmac_endian field to > >>>>>> sh_eth_cpu_data as well ? > >>>>> > >>>>> No, it depends on the SoC endianness which is determined by power-on > >>>>> pin strapping -- which is board specific. > >>> > >>> Does SoC endianness affect the ARM core endianness, the ethernet > >>> registers endianness, or both ? > >> > >> Both, AFAIK. > >> > >>> If it affects the ARM core endianness only, the kernel needs to be > >>> compiled in little-endian or big-endian mode anyway, and the sh_eth > >>> driver should use cpu_to_le32() unconditionally. If it affects both the > >>> ARM core and the ethernet controller there's not need to care about the > >>> endianness, as it will always be good. > >> > >> No, it won't unless you're using __raw_{readl|writel}() accessors. The > >> driver doesn't do it. {readl|writel}() and io{read|write}32() that use > >> them always assume LE ordering of memory. > >> > >>> We only need to care about it if it affects the ethernet controller > >>> registers only, which would seem weird to me. > >> > >> Unfortunately, you are wrong. > > > > Care to explain *why* ? There might be bugs in the driver (such as using > > the wrong I/O accessors), but I don't see why we need to configure the > > endianness through platform data. > > Re-read my reply about the power-on pin strapping please. The SoC endianness > setting gets read from the external source to the SoC, i.e. it's determined > by the board. Unless that pin doesn't affect the CPU core (in which case I wonder what it's used for), the kernel will need to be compiled for the specified endianness anyway. Endianness conversion can thus be performed with cpu_to_* (if the registers endianness is fixed) or skipped completely (if the registers endianness is also configured by the bootstrap pin) by using raw I/O accessors. Modifications will be need in the sh_eth driver, but I don't see why the driver would need to receive endianness information from platform data or DT.
Hello.
On 08/21/2013 04:58 PM, Laurent Pinchart wrote:
Sorry for the belated reply.
>>>>>>>>> Now that the 'register_type' field of the 'sh_eth' driver's platform
>>>>>>>>> data is not used by the driver anymore, it's time to remove it and
>>>>>>>>> its initializers from the SH platform code. Also move *enum*
>>>>>>>>> declaring values for this field from <linux/sh_eth.h> to the
>>>>>>>>> local driver's header file as they're only needed by the driver
>>>>>>>>> itself now...
>>>>>>>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>>>>>> [...]
>>>>>>>>> /* Driver's parameters */
>>>>>>>>> #if defined(CONFIG_CPU_SH4) || defined(CONFIG_ARCH_SHMOBILE)
>>>>>>>>> #define SH4_SKB_RX_ALIGN 32
>>>>>>>>>
>>>>>>>>> Index: net-next/include/linux/sh_eth.h
>>>>>>>>> ===================================================================
>>>>>>>>> --- net-next.orig/include/linux/sh_eth.h
>>>>>>>>> +++ net-next/include/linux/sh_eth.h
>>>>>>>>> @@ -5,17 +5,10 @@
>>>>>>>>>
>>>>>>>>> #include <linux/if_ether.h>
>>>>>>>>>
>>>>>>>>> enum {EDMAC_LITTLE_ENDIAN, EDMAC_BIG_ENDIAN};
>>>>>>>>>
>>>>>>>>> -enum {
>>>>>>>>> - SH_ETH_REG_GIGABIT,
>>>>>>>>> - SH_ETH_REG_FAST_RCAR,
>>>>>>>>> - SH_ETH_REG_FAST_SH4,
>>>>>>>>> - SH_ETH_REG_FAST_SH3_SH2
>>>>>>>>> -};
>>>>>>>>>
>>>>>>>>> struct sh_eth_plat_data {
>>>>>>>>>
>>>>>>>>> int phy;
>>>>>>>>> int edmac_endian;
>>>>>>>> Wouldn't it make sense to move the edmac_endian field to
>>>>>>>> sh_eth_cpu_data as well ?
>>>>>>> No, it depends on the SoC endianness which is determined by power-on
>>>>>>> pin strapping -- which is board specific.
>>>>> Does SoC endianness affect the ARM core endianness, the ethernet
>>>>> registers endianness, or both ?
>>>> Both, AFAIK.
>>>>> If it affects the ARM core endianness only, the kernel needs to be
>>>>> compiled in little-endian or big-endian mode anyway, and the sh_eth
>>>>> driver should use cpu_to_le32() unconditionally. If it affects both the
>>>>> ARM core and the ethernet controller there's not need to care about the
>>>>> endianness, as it will always be good.
>>>> No, it won't unless you're using __raw_{readl|writel}() accessors. The
>>>> driver doesn't do it. {readl|writel}() and io{read|write}32() that use
>>>> them always assume LE ordering of memory.
>>>>> We only need to care about it if it affects the ethernet controller
>>>>> registers only, which would seem weird to me.
>>>> Unfortunately, you are wrong.
>>> Care to explain *why* ? There might be bugs in the driver (such as using
>>> the wrong I/O accessors), but I don't see why we need to configure the
>>> endianness through platform data.
>> Re-read my reply about the power-on pin strapping please. The SoC endianness
>> setting gets read from the external source to the SoC, i.e. it's determined
>> by the board.
> Unless that pin doesn't affect the CPU core (in which case I wonder what it's
> used for),
The thing is I don't have the necessary information about SH SoCs for
which the 'edmac_endian' field was first added. I'm basing my assumptions on
the R-Car manuals which claim that both register and DMA descriptor endianness
depend on the SoC endianness (which is selected by the MD8 pin at power-on).
So I don't even understand why it's called 'edmac_endian' based on this info,
as it apparently determines only DMA descriptor endianness.
> the kernel will need to be compiled for the specified endianness
> anyway. Endianness conversion can thus be performed with cpu_to_* (if the
> registers endianness is fixed) or skipped completely (if the registers
> endianness is also configured by the bootstrap pin) by using raw I/O
> accessors.
Unfortunately, the raw accessors also miss the barriers which might be
necessary.
> Modifications will be need in the sh_eth driver, but I don't see
> why the driver would need to receive endianness information from platform data
> or DT.
So you suggest that we use __LITTLE_ENDIAN/__BIG_ENDIAN pre-defined macros
as when we're programing the EDMR.EL bit to enable automatic data swapping
feature (it doesn't swap register or descriptors, only the raw data)?
WBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Sergei, On Tuesday 27 August 2013 01:30:02 Sergei Shtylyov wrote: > On 08/21/2013 04:58 PM, Laurent Pinchart wrote: > > Sorry for the belated reply. No worries. > >>>>>>>>> Now that the 'register_type' field of the 'sh_eth' driver's > >>>>>>>>> platform > >>>>>>>>> data is not used by the driver anymore, it's time to remove it and > >>>>>>>>> its initializers from the SH platform code. Also move *enum* > >>>>>>>>> declaring values for this field from <linux/sh_eth.h> to the > >>>>>>>>> local driver's header file as they're only needed by the driver > >>>>>>>>> itself now... > >>>>>>>>> > >>>>>>>>> Signed-off-by: Sergei Shtylyov > >>>>>>>>> <sergei.shtylyov@cogentembedded.com> > >>>>>> > >>>>>> [...] > >>>>>> > >>>>>>>>> /* Driver's parameters */ > >>>>>>>>> #if defined(CONFIG_CPU_SH4) || defined(CONFIG_ARCH_SHMOBILE) > >>>>>>>>> #define SH4_SKB_RX_ALIGN 32 > >>>>>>>>> > >>>>>>>>> Index: net-next/include/linux/sh_eth.h > >>>>>>>>> ================================================================== > >>>>>>>>> = > >>>>>>>>> --- net-next.orig/include/linux/sh_eth.h > >>>>>>>>> +++ net-next/include/linux/sh_eth.h > >>>>>>>>> @@ -5,17 +5,10 @@ > >>>>>>>>> > >>>>>>>>> #include <linux/if_ether.h> > >>>>>>>>> > >>>>>>>>> enum {EDMAC_LITTLE_ENDIAN, EDMAC_BIG_ENDIAN}; > >>>>>>>>> > >>>>>>>>> -enum { > >>>>>>>>> - SH_ETH_REG_GIGABIT, > >>>>>>>>> - SH_ETH_REG_FAST_RCAR, > >>>>>>>>> - SH_ETH_REG_FAST_SH4, > >>>>>>>>> - SH_ETH_REG_FAST_SH3_SH2 > >>>>>>>>> -}; > >>>>>>>>> > >>>>>>>>> struct sh_eth_plat_data { > >>>>>>>>> > >>>>>>>>> int phy; > >>>>>>>>> int edmac_endian; > >>>>>>>> > >>>>>>>> Wouldn't it make sense to move the edmac_endian field to > >>>>>>>> sh_eth_cpu_data as well ? > >>>>>>> > >>>>>>> No, it depends on the SoC endianness which is determined by power-on > >>>>>>> pin strapping -- which is board specific. > >>>>> > >>>>> Does SoC endianness affect the ARM core endianness, the ethernet > >>>>> registers endianness, or both ? > >>>> > >>>> Both, AFAIK. > >>>> > >>>>> If it affects the ARM core endianness only, the kernel needs to be > >>>>> compiled in little-endian or big-endian mode anyway, and the sh_eth > >>>>> driver should use cpu_to_le32() unconditionally. If it affects both > >>>>> the ARM core and the ethernet controller there's not need to care > >>>>> about the endianness, as it will always be good. > >>>> > >>>> No, it won't unless you're using __raw_{readl|writel}() accessors. The > >>>> driver doesn't do it. {readl|writel}() and io{read|write}32() that use > >>>> them always assume LE ordering of memory. > >>>> > >>>>> We only need to care about it if it affects the ethernet controller > >>>>> registers only, which would seem weird to me. > >>>> > >>>> Unfortunately, you are wrong. > >>> > >>> Care to explain *why* ? There might be bugs in the driver (such as using > >>> the wrong I/O accessors), but I don't see why we need to configure the > >>> endianness through platform data. > >> > >> Re-read my reply about the power-on pin strapping please. The SoC > >> endianness setting gets read from the external source to the SoC, i.e. > >> it's determined by the board. > > > > Unless that pin doesn't affect the CPU core (in which case I wonder what > > it's used for), > > The thing is I don't have the necessary information about SH SoCs for which > the 'edmac_endian' field was first added. I'm basing my assumptions on the > R-Car manuals which claim that both register and DMA descriptor endianness > depend on the SoC endianness (which is selected by the MD8 pin at power-on). > So I don't even understand why it's called 'edmac_endian' based on this > info, as it apparently determines only DMA descriptor endianness. Neither do I. Reading the documentation I just had doubts, hence my questions. I doubt that the MD8 pin modifies the endianness of all registers, as that would probably be very costly from a silicon point of view, but everything is possible. > > the kernel will need to be compiled for the specified endianness > > anyway. Endianness conversion can thus be performed with cpu_to_* (if the > > registers endianness is fixed) or skipped completely (if the registers > > endianness is also configured by the bootstrap pin) by using raw I/O > > accessors. > > Unfortunately, the raw accessors also miss the barriers which might be > necessary. But it would be pretty trivial if needed to create and use in the driver accessors with barriers based on the raw accessors. > > Modifications will be need in the sh_eth driver, but I don't see > > why the driver would need to receive endianness information from platform > > data or DT. > > So you suggest that we use __LITTLE_ENDIAN/__BIG_ENDIAN pre-defined macros > as when we're programing the EDMR.EL bit to enable automatic data swapping > feature (it doesn't swap register or descriptors, only the raw data)? Based on my understanding, I believe that we either - don't need different register access codes paths for little and big endian (for a variety of reasons, as explained in the previous e-mails in this thread) - need different code paths, but can make the decision at compile time instead of runtime because the CPU core endianness changes as well, which forces a recompilation anyway I would thus suggest to use the right combination of cpu_to_[bl]e*, raw accessors (with added barriers), #ifdef __LITTLE_ENDIAN/__BIG_ENDIAN and EDMR.EL settings to remove the edmac_endian field. What the right combination is isn't know yet, we can experiment later when we'll get a big-endian system. I'm fine with keeping the field in the platform data structure for reference until then, but let's not add it to the DT bindings.
Index: net-next/arch/arm/mach-shmobile/board-armadillo800eva.c =================================================================== --- net-next.orig/arch/arm/mach-shmobile/board-armadillo800eva.c +++ net-next/arch/arm/mach-shmobile/board-armadillo800eva.c @@ -358,7 +358,6 @@ static struct platform_device usbhsf_dev static struct sh_eth_plat_data sh_eth_platdata = { .phy = 0x00, /* LAN8710A */ .edmac_endian = EDMAC_LITTLE_ENDIAN, - .register_type = SH_ETH_REG_GIGABIT, .phy_interface = PHY_INTERFACE_MODE_MII, }; Index: net-next/arch/arm/mach-shmobile/board-bockw.c =================================================================== --- net-next.orig/arch/arm/mach-shmobile/board-bockw.c +++ net-next/arch/arm/mach-shmobile/board-bockw.c @@ -89,7 +89,6 @@ static struct sh_mobile_sdhi_info sdhi0_ static struct sh_eth_plat_data ether_platform_data __initdata = { .phy = 0x01, .edmac_endian = EDMAC_LITTLE_ENDIAN, - .register_type = SH_ETH_REG_FAST_RCAR, .phy_interface = PHY_INTERFACE_MODE_RMII, /* * Although the LINK signal is available on the board, it's connected to Index: net-next/arch/sh/boards/board-espt.c =================================================================== --- net-next.orig/arch/sh/boards/board-espt.c +++ net-next/arch/sh/boards/board-espt.c @@ -80,7 +80,6 @@ static struct resource sh_eth_resources[ static struct sh_eth_plat_data sh7763_eth_pdata = { .phy = 0, .edmac_endian = EDMAC_LITTLE_ENDIAN, - .register_type = SH_ETH_REG_GIGABIT, .phy_interface = PHY_INTERFACE_MODE_MII, }; Index: net-next/arch/sh/boards/board-sh7757lcr.c =================================================================== --- net-next.orig/arch/sh/boards/board-sh7757lcr.c +++ net-next/arch/sh/boards/board-sh7757lcr.c @@ -77,7 +77,6 @@ static struct resource sh_eth0_resources static struct sh_eth_plat_data sh7757_eth0_pdata = { .phy = 1, .edmac_endian = EDMAC_LITTLE_ENDIAN, - .register_type = SH_ETH_REG_FAST_SH4, .set_mdio_gate = sh7757_eth_set_mdio_gate, }; @@ -106,7 +105,6 @@ static struct resource sh_eth1_resources static struct sh_eth_plat_data sh7757_eth1_pdata = { .phy = 1, .edmac_endian = EDMAC_LITTLE_ENDIAN, - .register_type = SH_ETH_REG_FAST_SH4, .set_mdio_gate = sh7757_eth_set_mdio_gate, }; @@ -151,7 +149,6 @@ static struct resource sh_eth_giga0_reso static struct sh_eth_plat_data sh7757_eth_giga0_pdata = { .phy = 18, .edmac_endian = EDMAC_LITTLE_ENDIAN, - .register_type = SH_ETH_REG_GIGABIT, .set_mdio_gate = sh7757_eth_giga_set_mdio_gate, .phy_interface = PHY_INTERFACE_MODE_RGMII_ID, }; @@ -186,7 +183,6 @@ static struct resource sh_eth_giga1_reso static struct sh_eth_plat_data sh7757_eth_giga1_pdata = { .phy = 19, .edmac_endian = EDMAC_LITTLE_ENDIAN, - .register_type = SH_ETH_REG_GIGABIT, .set_mdio_gate = sh7757_eth_giga_set_mdio_gate, .phy_interface = PHY_INTERFACE_MODE_RGMII_ID, }; Index: net-next/arch/sh/boards/mach-ecovec24/setup.c =================================================================== --- net-next.orig/arch/sh/boards/mach-ecovec24/setup.c +++ net-next/arch/sh/boards/mach-ecovec24/setup.c @@ -159,7 +159,6 @@ static struct resource sh_eth_resources[ static struct sh_eth_plat_data sh_eth_plat = { .phy = 0x1f, /* SMSC LAN8700 */ .edmac_endian = EDMAC_LITTLE_ENDIAN, - .register_type = SH_ETH_REG_FAST_SH4, .phy_interface = PHY_INTERFACE_MODE_MII, .ether_link_active_low = 1 }; Index: net-next/arch/sh/boards/mach-se/7724/setup.c =================================================================== --- net-next.orig/arch/sh/boards/mach-se/7724/setup.c +++ net-next/arch/sh/boards/mach-se/7724/setup.c @@ -377,7 +377,6 @@ static struct resource sh_eth_resources[ static struct sh_eth_plat_data sh_eth_plat = { .phy = 0x1f, /* SMSC LAN8187 */ .edmac_endian = EDMAC_LITTLE_ENDIAN, - .register_type = SH_ETH_REG_FAST_SH4, .phy_interace = PHY_INTERFACE_MODE_MII, }; Index: net-next/arch/sh/boards/mach-sh7763rdp/setup.c =================================================================== --- net-next.orig/arch/sh/boards/mach-sh7763rdp/setup.c +++ net-next/arch/sh/boards/mach-sh7763rdp/setup.c @@ -88,7 +88,6 @@ static struct resource sh_eth_resources[ static struct sh_eth_plat_data sh7763_eth_pdata = { .phy = 1, .edmac_endian = EDMAC_LITTLE_ENDIAN, - .register_type = SH_ETH_REG_GIGABIT, .phy_interface = PHY_INTERFACE_MODE_MII, }; Index: net-next/arch/sh/kernel/cpu/sh2/setup-sh7619.c =================================================================== --- net-next.orig/arch/sh/kernel/cpu/sh2/setup-sh7619.c +++ net-next/arch/sh/kernel/cpu/sh2/setup-sh7619.c @@ -114,7 +114,6 @@ static struct platform_device scif2_devi static struct sh_eth_plat_data eth_platform_data = { .phy = 1, .edmac_endian = EDMAC_LITTLE_ENDIAN, - .register_type = SH_ETH_REG_FAST_SH3_SH2, .phy_interace = PHY_INTERFACE_MODE_MII, }; Index: net-next/drivers/net/ethernet/renesas/sh_eth.h =================================================================== --- net-next.orig/drivers/net/ethernet/renesas/sh_eth.h +++ net-next/drivers/net/ethernet/renesas/sh_eth.h @@ -157,6 +157,13 @@ enum { SH_ETH_MAX_REGISTER_OFFSET, }; +enum { + SH_ETH_REG_GIGABIT, + SH_ETH_REG_FAST_RCAR, + SH_ETH_REG_FAST_SH4, + SH_ETH_REG_FAST_SH3_SH2 +}; + /* Driver's parameters */ #if defined(CONFIG_CPU_SH4) || defined(CONFIG_ARCH_SHMOBILE) #define SH4_SKB_RX_ALIGN 32 Index: net-next/include/linux/sh_eth.h =================================================================== --- net-next.orig/include/linux/sh_eth.h +++ net-next/include/linux/sh_eth.h @@ -5,17 +5,10 @@ #include <linux/if_ether.h> enum {EDMAC_LITTLE_ENDIAN, EDMAC_BIG_ENDIAN}; -enum { - SH_ETH_REG_GIGABIT, - SH_ETH_REG_FAST_RCAR, - SH_ETH_REG_FAST_SH4, - SH_ETH_REG_FAST_SH3_SH2 -}; struct sh_eth_plat_data { int phy; int edmac_endian; - int register_type; phy_interface_t phy_interface; void (*set_mdio_gate)(void *addr);
Now that the 'register_type' field of the 'sh_eth' driver's platform data is not used by the driver anymore, it's time to remove it and its initializers from the SH platform code. Also move *enum* declaring values for this field from <linux/sh_eth.h> to the local driver's header file as they're only needed by the driver itself now... Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> --- arch/arm/mach-shmobile/board-armadillo800eva.c | 1 - arch/arm/mach-shmobile/board-bockw.c | 1 - arch/sh/boards/board-espt.c | 1 - arch/sh/boards/board-sh7757lcr.c | 4 ---- arch/sh/boards/mach-ecovec24/setup.c | 1 - arch/sh/boards/mach-se/7724/setup.c | 1 - arch/sh/boards/mach-sh7763rdp/setup.c | 1 - arch/sh/kernel/cpu/sh2/setup-sh7619.c | 1 - drivers/net/ethernet/renesas/sh_eth.h | 7 +++++++ include/linux/sh_eth.h | 7 ------- 10 files changed, 7 insertions(+), 18 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html