[PATCH 0/2] sh_eth: don't get the register layout from platform data

[PATCH 0/2] sh_eth: don't get the register layout from platform data

[Sergei Shtylyov]

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

[PATCH 0/2] sh_eth: don't get the register layout from platform data

[Sergei Shtylyov]

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

[PATCH 1/2] sh_eth: get register layout from 'struct sh_eth_cpu_data'

[Sergei Shtylyov]

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

[PATCH 1/2] sh_eth: get register layout from 'struct sh_eth_cpu_data'

[Sergei Shtylyov]

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

[PATCH 2/2] sh_eth: remove 'register_type' field from 'struct sh_eth_plat_data'

[Sergei Shtylyov]

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

[PATCH 2/2] sh_eth: remove 'register_type' field from 'struct sh_eth_plat_data'

[Sergei Shtylyov]

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

Re: [PATCH 2/2] sh_eth: remove 'register_type' field from 'struct sh_eth_plat_data'

[Laurent Pinchart]

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

Re: [PATCH 2/2] sh_eth: remove 'register_type' field from 'struct sh_eth_plat_data'

[Laurent Pinchart]

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

Re: [PATCH 2/2] sh_eth: remove 'register_type' field from 'struct sh_eth_plat_data'

[Sergei Shtylyov]

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

Re: [PATCH 2/2] sh_eth: remove 'register_type' field from 'struct sh_eth_plat_data'

[Sergei Shtylyov]

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

Re: [PATCH 2/2] sh_eth: remove 'register_type' field from 'struct sh_eth_plat_data'

[Sergei Shtylyov]

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

Re: [PATCH 2/2] sh_eth: remove 'register_type' field from 'struct sh_eth_plat_data'

[Sergei Shtylyov]

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

Re: [PATCH 2/2] sh_eth: remove 'register_type' field from 'struct sh_eth_plat_data'

[Laurent Pinchart]

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

Re: [PATCH 2/2] sh_eth: remove 'register_type' field from 'struct sh_eth_plat_data'

[Laurent Pinchart]

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

Re: [PATCH 2/2] sh_eth: remove 'register_type' field from 'struct sh_eth_plat_data'

[Sergei Shtylyov]

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

Re: [PATCH 2/2] sh_eth: remove 'register_type' field from 'struct sh_eth_plat_data'

[Sergei Shtylyov]

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

Re: [PATCH 1/2] sh_eth: get register layout from 'struct sh_eth_cpu_data'

[David Miller]

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.

Re: [PATCH 1/2] sh_eth: get register layout from 'struct sh_eth_cpu_data'

[David Miller]

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.

Re: [PATCH 2/2] sh_eth: remove 'register_type' field from 'struct sh_eth_plat_data'

[David Miller]

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.

Re: [PATCH 2/2] sh_eth: remove 'register_type' field from 'struct sh_eth_plat_data'

[David Miller]

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.

Re: [PATCH 2/2] sh_eth: remove 'register_type' field from 'struct sh_eth_plat_data'

[Laurent Pinchart]

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

Re: [PATCH 2/2] sh_eth: remove 'register_type' field from 'struct sh_eth_plat_data'

[Laurent Pinchart]

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

Re: [PATCH 2/2] sh_eth: remove 'register_type' field from 'struct sh_eth_plat_data'

[Sergei Shtylyov]

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

Re: [PATCH 2/2] sh_eth: remove 'register_type' field from 'struct sh_eth_plat_data'

[Sergei Shtylyov]

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

Re: [PATCH 2/2] sh_eth: remove 'register_type' field from 'struct sh_eth_plat_data'

[Laurent Pinchart]

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

Re: [PATCH 2/2] sh_eth: remove 'register_type' field from 'struct sh_eth_plat_data'

[Laurent Pinchart]

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

Re: [PATCH 2/2] sh_eth: remove 'register_type' field from 'struct sh_eth_plat_data'

[Sergei Shtylyov]

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

Re: [PATCH 2/2] sh_eth: remove 'register_type' field from 'struct sh_eth_plat_data'

[Sergei Shtylyov]

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

Re: [PATCH 2/2] sh_eth: remove 'register_type' field from 'struct sh_eth_plat_data'

[Laurent Pinchart]

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

Re: [PATCH 2/2] sh_eth: remove 'register_type' field from 'struct sh_eth_plat_data'

[Laurent Pinchart]

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