* [PATCH 0/2] sh_eth: don't get the register layout from platform data @ 2013-08-17 23:08 Sergei Shtylyov 2013-08-17 23:11 ` [PATCH 1/2] sh_eth: get register layout from 'struct sh_eth_cpu_data' Sergei Shtylyov 2013-08-17 23:13 ` [PATCH 2/2] sh_eth: remove 'register_type' field from 'struct sh_eth_plat_data' Sergei Shtylyov 0 siblings, 2 replies; 15+ messages in thread From: Sergei Shtylyov @ 2013-08-17 23:08 UTC (permalink / raw) To: netdev, davem; +Cc: lethal, linux-sh Hello. Here's a couple of patches making the 'sh_eth' driver get the register layout from the internal data structures, not the platform data; this is in preparation to adding device tree support to the driver. The patches are against David Miller's 'net-next.git' repo and the couple of SH Ether fixes I've just posted. David, if you decide to merge those SH patches into the 'net.git' repo, then a sync of it to 'net-next.git' would be required before applying this series. [1/2] sh_eth: get register layout from 'struct sh_eth_cpu_data' [2/2] sh_eth: remove 'register_type' field from 'struct sh_eth_plat_data' WBR, Sergei ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/2] sh_eth: get register layout from 'struct sh_eth_cpu_data' 2013-08-17 23:08 [PATCH 0/2] sh_eth: don't get the register layout from platform data Sergei Shtylyov @ 2013-08-17 23:11 ` Sergei Shtylyov 2013-08-21 0:05 ` David Miller 2013-08-17 23:13 ` [PATCH 2/2] sh_eth: remove 'register_type' field from 'struct sh_eth_plat_data' Sergei Shtylyov 1 sibling, 1 reply; 15+ messages in thread From: Sergei Shtylyov @ 2013-08-17 23:11 UTC (permalink / raw) To: netdev, davem; +Cc: linux-sh The register layout is a SoC characteristic, so it's wrong that it's stored in the otherwise board specific platform data. Add 'register_type' field to 'struct sh_eth_cpu_data', initialize it properly for each SoC, and read it from this structure instead of the platfrom data from now on... Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> --- drivers/net/ethernet/renesas/sh_eth.c | 22 +++++++++++++++++++++- drivers/net/ethernet/renesas/sh_eth.h | 1 + 2 files changed, 22 insertions(+), 1 deletion(-) Index: net-next/drivers/net/ethernet/renesas/sh_eth.c =================================================================== --- net-next.orig/drivers/net/ethernet/renesas/sh_eth.c +++ net-next/drivers/net/ethernet/renesas/sh_eth.c @@ -378,6 +378,8 @@ static struct sh_eth_cpu_data r8a777x_da .set_duplex = sh_eth_set_duplex, .set_rate = sh_eth_set_rate_r8a777x, + .register_type = SH_ETH_REG_FAST_RCAR, + .ecsr_value = ECSR_PSRTO | ECSR_LCHNG | ECSR_ICD, .ecsipr_value = ECSIPR_PSRTOIP | ECSIPR_LCHNGIP | ECSIPR_ICDIP, .eesipr_value = 0x01ff009f, @@ -398,6 +400,8 @@ static struct sh_eth_cpu_data r8a7790_da .set_duplex = sh_eth_set_duplex, .set_rate = sh_eth_set_rate_r8a777x, + .register_type = SH_ETH_REG_FAST_RCAR, + .ecsr_value = ECSR_PSRTO | ECSR_LCHNG | ECSR_ICD, .ecsipr_value = ECSIPR_PSRTOIP | ECSIPR_LCHNGIP | ECSIPR_ICDIP, .eesipr_value = 0x01ff009f, @@ -435,6 +439,8 @@ static struct sh_eth_cpu_data sh7724_dat .set_duplex = sh_eth_set_duplex, .set_rate = sh_eth_set_rate_sh7724, + .register_type = SH_ETH_REG_FAST_SH4, + .ecsr_value = ECSR_PSRTO | ECSR_LCHNG | ECSR_ICD, .ecsipr_value = ECSIPR_PSRTOIP | ECSIPR_LCHNGIP | ECSIPR_ICDIP, .eesipr_value = 0x01ff009f, @@ -473,6 +479,8 @@ static struct sh_eth_cpu_data sh7757_dat .set_duplex = sh_eth_set_duplex, .set_rate = sh_eth_set_rate_sh7757, + .register_type = SH_ETH_REG_FAST_SH4, + .eesipr_value = DMAC_M_RFRMER | DMAC_M_ECI | 0x003fffff, .rmcr_value = 0x00000001, @@ -541,6 +549,8 @@ static struct sh_eth_cpu_data sh7757_dat .set_duplex = sh_eth_set_duplex, .set_rate = sh_eth_set_rate_giga, + .register_type = SH_ETH_REG_GIGABIT, + .ecsr_value = ECSR_ICD | ECSR_MPD, .ecsipr_value = ECSIPR_LCHNGIP | ECSIPR_ICDIP | ECSIPR_MPDIP, .eesipr_value = DMAC_M_RFRMER | DMAC_M_ECI | 0x003fffff, @@ -599,6 +609,8 @@ static struct sh_eth_cpu_data sh7734_dat .set_duplex = sh_eth_set_duplex, .set_rate = sh_eth_set_rate_gether, + .register_type = SH_ETH_REG_GIGABIT, + .ecsr_value = ECSR_ICD | ECSR_MPD, .ecsipr_value = ECSIPR_LCHNGIP | ECSIPR_ICDIP | ECSIPR_MPDIP, .eesipr_value = DMAC_M_RFRMER | DMAC_M_ECI | 0x003fffff, @@ -626,6 +638,8 @@ static struct sh_eth_cpu_data sh7763_dat .set_duplex = sh_eth_set_duplex, .set_rate = sh_eth_set_rate_gether, + .register_type = SH_ETH_REG_GIGABIT, + .ecsr_value = ECSR_ICD | ECSR_MPD, .ecsipr_value = ECSIPR_LCHNGIP | ECSIPR_ICDIP | ECSIPR_MPDIP, .eesipr_value = DMAC_M_RFRMER | DMAC_M_ECI | 0x003fffff, @@ -663,6 +677,8 @@ static struct sh_eth_cpu_data r8a7740_da .set_duplex = sh_eth_set_duplex, .set_rate = sh_eth_set_rate_gether, + .register_type = SH_ETH_REG_GIGABIT, + .ecsr_value = ECSR_ICD | ECSR_MPD, .ecsipr_value = ECSIPR_LCHNGIP | ECSIPR_ICDIP | ECSIPR_MPDIP, .eesipr_value = DMAC_M_RFRMER | DMAC_M_ECI | 0x003fffff, @@ -685,6 +701,8 @@ static struct sh_eth_cpu_data r8a7740_da }; static struct sh_eth_cpu_data sh7619_data = { + .register_type = SH_ETH_REG_FAST_SH3_SH2, + .eesipr_value = DMAC_M_RFRMER | DMAC_M_ECI | 0x003fffff, .apr = 1, @@ -694,6 +712,8 @@ static struct sh_eth_cpu_data sh7619_dat }; static struct sh_eth_cpu_data sh771x_data = { + .register_type = SH_ETH_REG_FAST_SH3_SH2, + .eesipr_value = DMAC_M_RFRMER | DMAC_M_ECI | 0x003fffff, .tsu = 1, }; @@ -2643,10 +2663,10 @@ static int sh_eth_drv_probe(struct platf mdp->edmac_endian = pd->edmac_endian; mdp->no_ether_link = pd->no_ether_link; mdp->ether_link_active_low = pd->ether_link_active_low; - mdp->reg_offset = sh_eth_get_register_offset(pd->register_type); /* set cpu data */ mdp->cd = (struct sh_eth_cpu_data *)id->driver_data; + mdp->reg_offset = sh_eth_get_register_offset(mdp->cd->register_type); sh_eth_set_default_cpu_data(mdp->cd); /* set function */ 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 @@ -454,6 +454,7 @@ struct sh_eth_cpu_data { void (*set_rate)(struct net_device *ndev); /* mandatory initialize value */ + int register_type; unsigned long eesipr_value; /* optional initialize value */ ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] sh_eth: get register layout from 'struct sh_eth_cpu_data' 2013-08-17 23:11 ` [PATCH 1/2] sh_eth: get register layout from 'struct sh_eth_cpu_data' Sergei Shtylyov @ 2013-08-21 0:05 ` David Miller 0 siblings, 0 replies; 15+ messages in thread From: David Miller @ 2013-08-21 0:05 UTC (permalink / raw) To: sergei.shtylyov; +Cc: netdev, linux-sh From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> Date: Sun, 18 Aug 2013 03:11:28 +0400 > The register layout is a SoC characteristic, so it's wrong that it's stored > in the otherwise board specific platform data. Add 'register_type' field to > 'struct sh_eth_cpu_data', initialize it properly for each SoC, and read it > from this structure instead of the platfrom data from now on... > > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> Applied. ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/2] sh_eth: remove 'register_type' field from 'struct sh_eth_plat_data' 2013-08-17 23:08 [PATCH 0/2] sh_eth: don't get the register layout from platform data Sergei Shtylyov 2013-08-17 23:11 ` [PATCH 1/2] sh_eth: get register layout from 'struct sh_eth_cpu_data' Sergei Shtylyov @ 2013-08-17 23:13 ` Sergei Shtylyov 2013-08-20 10:51 ` Laurent Pinchart 2013-08-21 0:05 ` David Miller 1 sibling, 2 replies; 15+ messages in thread From: Sergei Shtylyov @ 2013-08-17 23:13 UTC (permalink / raw) To: netdev, davem, lethal, linux-sh 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(-) 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); ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] sh_eth: remove 'register_type' field from 'struct sh_eth_plat_data' 2013-08-17 23:13 ` [PATCH 2/2] sh_eth: remove 'register_type' field from 'struct sh_eth_plat_data' Sergei Shtylyov @ 2013-08-20 10:51 ` Laurent Pinchart 2013-08-20 14:27 ` Sergei Shtylyov 2013-08-21 0:05 ` David Miller 1 sibling, 1 reply; 15+ messages in thread From: Laurent Pinchart @ 2013-08-20 10:51 UTC (permalink / raw) To: Sergei Shtylyov; +Cc: netdev, davem, lethal, linux-sh 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); -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] sh_eth: remove 'register_type' field from 'struct sh_eth_plat_data' 2013-08-20 10:51 ` Laurent Pinchart @ 2013-08-20 14:27 ` Sergei Shtylyov 2013-08-20 22:09 ` Sergei Shtylyov 0 siblings, 1 reply; 15+ messages in thread From: Sergei Shtylyov @ 2013-08-20 14:27 UTC (permalink / raw) To: Laurent Pinchart; +Cc: netdev, davem, lethal, linux-sh 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] sh_eth: remove 'register_type' field from 'struct sh_eth_plat_data' 2013-08-20 14:27 ` Sergei Shtylyov @ 2013-08-20 22:09 ` Sergei Shtylyov 2013-08-20 22:50 ` Laurent Pinchart 0 siblings, 1 reply; 15+ messages in thread From: Sergei Shtylyov @ 2013-08-20 22:09 UTC (permalink / raw) To: Laurent Pinchart; +Cc: netdev, davem, lethal, linux-sh 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] sh_eth: remove 'register_type' field from 'struct sh_eth_plat_data' 2013-08-20 22:09 ` Sergei Shtylyov @ 2013-08-20 22:50 ` Laurent Pinchart 2013-08-20 23:01 ` Sergei Shtylyov 0 siblings, 1 reply; 15+ messages in thread From: Laurent Pinchart @ 2013-08-20 22:50 UTC (permalink / raw) To: Sergei Shtylyov; +Cc: netdev, davem, lethal, linux-sh 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. -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] sh_eth: remove 'register_type' field from 'struct sh_eth_plat_data' 2013-08-20 22:50 ` Laurent Pinchart @ 2013-08-20 23:01 ` Sergei Shtylyov 2013-08-21 0:39 ` Laurent Pinchart 0 siblings, 1 reply; 15+ messages in thread From: Sergei Shtylyov @ 2013-08-20 23:01 UTC (permalink / raw) To: Laurent Pinchart; +Cc: netdev, davem, lethal, linux-sh 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] sh_eth: remove 'register_type' field from 'struct sh_eth_plat_data' 2013-08-20 23:01 ` Sergei Shtylyov @ 2013-08-21 0:39 ` Laurent Pinchart 2013-08-21 12:49 ` Sergei Shtylyov 0 siblings, 1 reply; 15+ messages in thread From: Laurent Pinchart @ 2013-08-21 0:39 UTC (permalink / raw) To: Sergei Shtylyov; +Cc: netdev, davem, lethal, linux-sh 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. -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] sh_eth: remove 'register_type' field from 'struct sh_eth_plat_data' 2013-08-21 0:39 ` Laurent Pinchart @ 2013-08-21 12:49 ` Sergei Shtylyov 2013-08-21 12:58 ` Laurent Pinchart 0 siblings, 1 reply; 15+ messages in thread From: Sergei Shtylyov @ 2013-08-21 12:49 UTC (permalink / raw) To: Laurent Pinchart; +Cc: netdev, davem, lethal, linux-sh 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] sh_eth: remove 'register_type' field from 'struct sh_eth_plat_data' 2013-08-21 12:49 ` Sergei Shtylyov @ 2013-08-21 12:58 ` Laurent Pinchart 2013-08-26 21:30 ` Sergei Shtylyov 0 siblings, 1 reply; 15+ messages in thread From: Laurent Pinchart @ 2013-08-21 12:58 UTC (permalink / raw) To: Sergei Shtylyov; +Cc: netdev, davem, lethal, linux-sh 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. -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] sh_eth: remove 'register_type' field from 'struct sh_eth_plat_data' 2013-08-21 12:58 ` Laurent Pinchart @ 2013-08-26 21:30 ` Sergei Shtylyov 2013-08-27 6:41 ` Laurent Pinchart 0 siblings, 1 reply; 15+ messages in thread From: Sergei Shtylyov @ 2013-08-26 21:30 UTC (permalink / raw) To: Laurent Pinchart; +Cc: netdev, davem, lethal, linux-sh 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] sh_eth: remove 'register_type' field from 'struct sh_eth_plat_data' 2013-08-26 21:30 ` Sergei Shtylyov @ 2013-08-27 6:41 ` Laurent Pinchart 0 siblings, 0 replies; 15+ messages in thread From: Laurent Pinchart @ 2013-08-27 6:41 UTC (permalink / raw) To: Sergei Shtylyov; +Cc: netdev, davem, lethal, linux-sh 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. -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] sh_eth: remove 'register_type' field from 'struct sh_eth_plat_data' 2013-08-17 23:13 ` [PATCH 2/2] sh_eth: remove 'register_type' field from 'struct sh_eth_plat_data' Sergei Shtylyov 2013-08-20 10:51 ` Laurent Pinchart @ 2013-08-21 0:05 ` David Miller 1 sibling, 0 replies; 15+ messages in thread From: David Miller @ 2013-08-21 0:05 UTC (permalink / raw) To: sergei.shtylyov; +Cc: netdev, lethal, linux-sh 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2013-08-27 6:40 UTC | newest] Thread overview: 15+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-08-17 23:08 [PATCH 0/2] sh_eth: don't get the register layout from platform data Sergei Shtylyov 2013-08-17 23:11 ` [PATCH 1/2] sh_eth: get register layout from 'struct sh_eth_cpu_data' Sergei Shtylyov 2013-08-21 0:05 ` David Miller 2013-08-17 23:13 ` [PATCH 2/2] sh_eth: remove 'register_type' field from 'struct sh_eth_plat_data' Sergei Shtylyov 2013-08-20 10:51 ` Laurent Pinchart 2013-08-20 14:27 ` Sergei Shtylyov 2013-08-20 22:09 ` Sergei Shtylyov 2013-08-20 22:50 ` Laurent Pinchart 2013-08-20 23:01 ` Sergei Shtylyov 2013-08-21 0:39 ` Laurent Pinchart 2013-08-21 12:49 ` Sergei Shtylyov 2013-08-21 12:58 ` Laurent Pinchart 2013-08-26 21:30 ` Sergei Shtylyov 2013-08-27 6:41 ` Laurent Pinchart 2013-08-21 0:05 ` David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).