* [PATCH 0/2] sh_eth: don't get the register layout from platform data
@ 2013-08-17 23:08 ` Sergei Shtylyov
0 siblings, 0 replies; 30+ 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] 30+ messages in thread
* [PATCH 1/2] sh_eth: get register layout from 'struct sh_eth_cpu_data'
2013-08-17 23:08 ` Sergei Shtylyov
@ 2013-08-17 23:11 ` Sergei Shtylyov
-1 siblings, 0 replies; 30+ 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] 30+ messages in thread* [PATCH 1/2] sh_eth: get register layout from 'struct sh_eth_cpu_data'
@ 2013-08-17 23:11 ` Sergei Shtylyov
0 siblings, 0 replies; 30+ 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] 30+ messages in thread* Re: [PATCH 1/2] sh_eth: get register layout from 'struct sh_eth_cpu_data'
2013-08-17 23:11 ` Sergei Shtylyov
@ 2013-08-21 0:05 ` David Miller
-1 siblings, 0 replies; 30+ 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] 30+ messages in thread
* Re: [PATCH 1/2] sh_eth: get register layout from 'struct sh_eth_cpu_data'
@ 2013-08-21 0:05 ` David Miller
0 siblings, 0 replies; 30+ 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] 30+ messages in thread
* [PATCH 2/2] sh_eth: remove 'register_type' field from 'struct sh_eth_plat_data'
2013-08-17 23:08 ` Sergei Shtylyov
@ 2013-08-17 23:13 ` Sergei Shtylyov
-1 siblings, 0 replies; 30+ 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] 30+ messages in thread* [PATCH 2/2] sh_eth: remove 'register_type' field from 'struct sh_eth_plat_data'
@ 2013-08-17 23:13 ` Sergei Shtylyov
0 siblings, 0 replies; 30+ 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] 30+ messages in thread* Re: [PATCH 2/2] sh_eth: remove 'register_type' field from 'struct sh_eth_plat_data'
2013-08-17 23:13 ` Sergei Shtylyov
@ 2013-08-20 10:51 ` Laurent Pinchart
-1 siblings, 0 replies; 30+ 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] 30+ 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
0 siblings, 0 replies; 30+ 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] 30+ 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
-1 siblings, 0 replies; 30+ 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] 30+ 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
0 siblings, 0 replies; 30+ 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] 30+ 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
-1 siblings, 0 replies; 30+ 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] 30+ 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
0 siblings, 0 replies; 30+ 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] 30+ 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
-1 siblings, 0 replies; 30+ 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] 30+ 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
0 siblings, 0 replies; 30+ 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] 30+ 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
-1 siblings, 0 replies; 30+ 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] 30+ 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
0 siblings, 0 replies; 30+ 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] 30+ 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
-1 siblings, 0 replies; 30+ 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] 30+ 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
0 siblings, 0 replies; 30+ 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] 30+ 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
-1 siblings, 0 replies; 30+ 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] 30+ 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
0 siblings, 0 replies; 30+ 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] 30+ 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
-1 siblings, 0 replies; 30+ 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] 30+ 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
0 siblings, 0 replies; 30+ 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] 30+ 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
-1 siblings, 0 replies; 30+ 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] 30+ 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
0 siblings, 0 replies; 30+ 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] 30+ 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
-1 siblings, 0 replies; 30+ 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] 30+ messages in thread* Re: [PATCH 2/2] sh_eth: remove 'register_type' field from 'struct sh_eth_plat_data'
@ 2013-08-27 6:41 ` Laurent Pinchart
0 siblings, 0 replies; 30+ 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] 30+ messages in thread
* Re: [PATCH 2/2] sh_eth: remove 'register_type' field from 'struct sh_eth_plat_data'
2013-08-17 23:13 ` Sergei Shtylyov
@ 2013-08-21 0:05 ` David Miller
-1 siblings, 0 replies; 30+ 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] 30+ messages in thread
* Re: [PATCH 2/2] sh_eth: remove 'register_type' field from 'struct sh_eth_plat_data'
@ 2013-08-21 0:05 ` David Miller
0 siblings, 0 replies; 30+ 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] 30+ messages in thread