| Message ID | 1422200959-1717-4-git-send-email-jenskuske@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Jens, On Sun, Jan 25, 2015 at 04:49:19PM +0100, Jens Kuske wrote: > The EMAC needs SRAM block A3_A4 being mapped to EMAC peripheral to > work. This is done by the bootloader most of the time, but U-Boot > Falcon Mode, for example, skips emac initialization and SRAM would > stay mapped to the CPU. Thanks for reviving this. > Signed-off-by: Jens Kuske <jenskuske@gmail.com> > --- > drivers/net/ethernet/allwinner/Kconfig | 1 + > drivers/net/ethernet/allwinner/sun4i-emac.c | 18 ++++++++++++++++++ > 2 files changed, 19 insertions(+) > > diff --git a/drivers/net/ethernet/allwinner/Kconfig b/drivers/net/ethernet/allwinner/Kconfig > index d8d95d4..508a288 100644 > --- a/drivers/net/ethernet/allwinner/Kconfig > +++ b/drivers/net/ethernet/allwinner/Kconfig > @@ -28,6 +28,7 @@ config SUN4I_EMAC > select MII > select PHYLIB > select MDIO_SUN4I > + select MFD_SYSCON > ---help--- > Support for Allwinner A10 EMAC ethernet driver. > > diff --git a/drivers/net/ethernet/allwinner/sun4i-emac.c b/drivers/net/ethernet/allwinner/sun4i-emac.c > index 1fcd556..86c891d 100644 > --- a/drivers/net/ethernet/allwinner/sun4i-emac.c > +++ b/drivers/net/ethernet/allwinner/sun4i-emac.c > @@ -18,6 +18,8 @@ > #include <linux/gpio.h> > #include <linux/interrupt.h> > #include <linux/irq.h> > +#include <linux/mfd/syscon.h> > +#include <linux/mfd/syscon/sun4i-sc.h> > #include <linux/mii.h> > #include <linux/module.h> > #include <linux/netdevice.h> > @@ -28,6 +30,7 @@ > #include <linux/of_platform.h> > #include <linux/platform_device.h> > #include <linux/phy.h> > +#include <linux/regmap.h> > > #include "sun4i-emac.h" > > @@ -78,6 +81,7 @@ struct emac_board_info { > > struct phy_device *phy_dev; > struct device_node *phy_node; > + struct regmap *sc; > unsigned int link; > unsigned int speed; > unsigned int duplex; > @@ -862,6 +866,18 @@ static int emac_probe(struct platform_device *pdev) > goto out; > } > > + /* Map SRAM_A3_A4 to EMAC */ > + db->sc = syscon_regmap_lookup_by_compatible( > + "allwinner,sun4i-a10-syscon"); > + if (IS_ERR(db->sc)) { > + dev_err(&pdev->dev, "failed to find syscon regmap\n"); > + ret = PTR_ERR(db->sc); > + goto out; > + } > + > + regmap_update_bits(db->sc, SUN4I_SC1, SUN4I_SC1_SRAM_A3_A4_MAP_MASK, > + SUN4I_SC1_SRAM_A3_A4_MAP_EMAC); > + I don't think that using a syscon is the right solution here. All this SRAM mapping thing is mutually exclusive, and will possibly impact other drivers as well. I think this is a more a case for a small driver in drivers/soc that would take care of this, and make sure that client drivers don't step on each other's toe. Maxime
Hi, On 25/01/15 17:25, Maxime Ripard wrote: > Hi Jens, > > On Sun, Jan 25, 2015 at 04:49:19PM +0100, Jens Kuske wrote: >> The EMAC needs SRAM block A3_A4 being mapped to EMAC peripheral to >> work. This is done by the bootloader most of the time, but U-Boot >> Falcon Mode, for example, skips emac initialization and SRAM would >> stay mapped to the CPU. > > Thanks for reviving this. > >> Signed-off-by: Jens Kuske <jenskuske@gmail.com> >> --- >> drivers/net/ethernet/allwinner/Kconfig | 1 + >> drivers/net/ethernet/allwinner/sun4i-emac.c | 18 ++++++++++++++++++ >> 2 files changed, 19 insertions(+) >> >> diff --git a/drivers/net/ethernet/allwinner/Kconfig b/drivers/net/ethernet/allwinner/Kconfig >> index d8d95d4..508a288 100644 >> --- a/drivers/net/ethernet/allwinner/Kconfig >> +++ b/drivers/net/ethernet/allwinner/Kconfig >> @@ -28,6 +28,7 @@ config SUN4I_EMAC >> select MII >> select PHYLIB >> select MDIO_SUN4I >> + select MFD_SYSCON >> ---help--- >> Support for Allwinner A10 EMAC ethernet driver. >> >> diff --git a/drivers/net/ethernet/allwinner/sun4i-emac.c b/drivers/net/ethernet/allwinner/sun4i-emac.c >> index 1fcd556..86c891d 100644 >> --- a/drivers/net/ethernet/allwinner/sun4i-emac.c >> +++ b/drivers/net/ethernet/allwinner/sun4i-emac.c >> @@ -18,6 +18,8 @@ >> #include <linux/gpio.h> >> #include <linux/interrupt.h> >> #include <linux/irq.h> >> +#include <linux/mfd/syscon.h> >> +#include <linux/mfd/syscon/sun4i-sc.h> >> #include <linux/mii.h> >> #include <linux/module.h> >> #include <linux/netdevice.h> >> @@ -28,6 +30,7 @@ >> #include <linux/of_platform.h> >> #include <linux/platform_device.h> >> #include <linux/phy.h> >> +#include <linux/regmap.h> >> >> #include "sun4i-emac.h" >> >> @@ -78,6 +81,7 @@ struct emac_board_info { >> >> struct phy_device *phy_dev; >> struct device_node *phy_node; >> + struct regmap *sc; >> unsigned int link; >> unsigned int speed; >> unsigned int duplex; >> @@ -862,6 +866,18 @@ static int emac_probe(struct platform_device *pdev) >> goto out; >> } >> >> + /* Map SRAM_A3_A4 to EMAC */ >> + db->sc = syscon_regmap_lookup_by_compatible( >> + "allwinner,sun4i-a10-syscon"); >> + if (IS_ERR(db->sc)) { >> + dev_err(&pdev->dev, "failed to find syscon regmap\n"); >> + ret = PTR_ERR(db->sc); >> + goto out; >> + } >> + >> + regmap_update_bits(db->sc, SUN4I_SC1, SUN4I_SC1_SRAM_A3_A4_MAP_MASK, >> + SUN4I_SC1_SRAM_A3_A4_MAP_EMAC); >> + > > I don't think that using a syscon is the right solution here. > > All this SRAM mapping thing is mutually exclusive, and will possibly > impact other drivers as well. Each single SRAM area can only be mapped to a single peripheral, so as long as the driver only changes bits related to his own area nothing can go wrong I believe. SRAM_C2 looks like it can be mapped do different devices (AE, CE, ACE), but as far as I understand this, they are all related to the ACE device, sharing a common register space, and would have to be handled by a single driver anyway (if that will ever happen without docs) https://linux-sunxi.org/ACE_Register_guide > I think this is a more a case for a small driver in drivers/soc that > would take care of this, and make sure that client drivers don't step > on each other's toe. I'm not convinced this is necessary, but what would this driver do different than a basic regmap? Check if the area is already mapped by any driver and deny mapping it again by a different driver? Which different driver, each area is only interesting for a single device/driver? Except maybe mapping it to CPU as general purpose sram, but that would need some direct agreement with the driver to steal its memory anyway. Jens
Hi, On Mon, Jan 26, 2015 at 03:34:49PM +0100, Jens Kuske wrote: > Hi, > > On 25/01/15 17:25, Maxime Ripard wrote: > >Hi Jens, > > > >On Sun, Jan 25, 2015 at 04:49:19PM +0100, Jens Kuske wrote: > >>The EMAC needs SRAM block A3_A4 being mapped to EMAC peripheral to > >>work. This is done by the bootloader most of the time, but U-Boot > >>Falcon Mode, for example, skips emac initialization and SRAM would > >>stay mapped to the CPU. > > > >Thanks for reviving this. > > > >>Signed-off-by: Jens Kuske <jenskuske@gmail.com> > >>--- > >> drivers/net/ethernet/allwinner/Kconfig | 1 + > >> drivers/net/ethernet/allwinner/sun4i-emac.c | 18 ++++++++++++++++++ > >> 2 files changed, 19 insertions(+) > >> > >>diff --git a/drivers/net/ethernet/allwinner/Kconfig b/drivers/net/ethernet/allwinner/Kconfig > >>index d8d95d4..508a288 100644 > >>--- a/drivers/net/ethernet/allwinner/Kconfig > >>+++ b/drivers/net/ethernet/allwinner/Kconfig > >>@@ -28,6 +28,7 @@ config SUN4I_EMAC > >> select MII > >> select PHYLIB > >> select MDIO_SUN4I > >>+ select MFD_SYSCON > >> ---help--- > >> Support for Allwinner A10 EMAC ethernet driver. > >> > >>diff --git a/drivers/net/ethernet/allwinner/sun4i-emac.c b/drivers/net/ethernet/allwinner/sun4i-emac.c > >>index 1fcd556..86c891d 100644 > >>--- a/drivers/net/ethernet/allwinner/sun4i-emac.c > >>+++ b/drivers/net/ethernet/allwinner/sun4i-emac.c > >>@@ -18,6 +18,8 @@ > >> #include <linux/gpio.h> > >> #include <linux/interrupt.h> > >> #include <linux/irq.h> > >>+#include <linux/mfd/syscon.h> > >>+#include <linux/mfd/syscon/sun4i-sc.h> > >> #include <linux/mii.h> > >> #include <linux/module.h> > >> #include <linux/netdevice.h> > >>@@ -28,6 +30,7 @@ > >> #include <linux/of_platform.h> > >> #include <linux/platform_device.h> > >> #include <linux/phy.h> > >>+#include <linux/regmap.h> > >> > >> #include "sun4i-emac.h" > >> > >>@@ -78,6 +81,7 @@ struct emac_board_info { > >> > >> struct phy_device *phy_dev; > >> struct device_node *phy_node; > >>+ struct regmap *sc; > >> unsigned int link; > >> unsigned int speed; > >> unsigned int duplex; > >>@@ -862,6 +866,18 @@ static int emac_probe(struct platform_device *pdev) > >> goto out; > >> } > >> > >>+ /* Map SRAM_A3_A4 to EMAC */ > >>+ db->sc = syscon_regmap_lookup_by_compatible( > >>+ "allwinner,sun4i-a10-syscon"); > >>+ if (IS_ERR(db->sc)) { > >>+ dev_err(&pdev->dev, "failed to find syscon regmap\n"); > >>+ ret = PTR_ERR(db->sc); > >>+ goto out; > >>+ } > >>+ > >>+ regmap_update_bits(db->sc, SUN4I_SC1, SUN4I_SC1_SRAM_A3_A4_MAP_MASK, > >>+ SUN4I_SC1_SRAM_A3_A4_MAP_EMAC); > >>+ > > > >I don't think that using a syscon is the right solution here. > > > >All this SRAM mapping thing is mutually exclusive, and will possibly > >impact other drivers as well. > > Each single SRAM area can only be mapped to a single peripheral, so as long > as the driver only changes bits related to his own area nothing can go wrong > I believe. Which is exactly my point. This kind of assumption is very fragile. Such mistakes might be made, will slip in under any review and might get un-noticed for a very long time. While adding a simple driver at least would make this obvious when two drivers will contend for the same SRAM mapping. > SRAM_C2 looks like it can be mapped do different devices (AE, CE, ACE), but > as far as I understand this, they are all related to the ACE device, sharing > a common register space, and would have to be handled by a single driver > anyway (if that will ever happen without docs) > https://linux-sunxi.org/ACE_Register_guide > > >I think this is a more a case for a small driver in drivers/soc that > >would take care of this, and make sure that client drivers don't step > >on each other's toe. > > I'm not convinced this is necessary, but what would this driver do different > than a basic regmap? Check if the area is already mapped by any driver and > deny mapping it again by a different driver? Which different driver, each > area is only interesting for a single device/driver? Except maybe mapping it > to CPU as general purpose sram, but that would need some direct agreement > with the driver to steal its memory anyway. Off the top of my head: - Make sure no one step on each other's toe, including the CPU that might require a SRAM for several things (suspend, cpuidle, PSCI, etc.) - Provide a comprehensive status of the various SRAM and what they are mapped to in debugfs - Provide an abstraction to the SRAM mapping IP. You make the assumption that the client drivers will always use on an A10 or an SoC that has the same SRAMs, and the same IP to control which device they are mapped to. This might not be true for other drivers, and other SoCs. - Make any adjustment to these registers that wouldn't fit in any client drivers. Maxime
diff --git a/drivers/net/ethernet/allwinner/Kconfig b/drivers/net/ethernet/allwinner/Kconfig index d8d95d4..508a288 100644 --- a/drivers/net/ethernet/allwinner/Kconfig +++ b/drivers/net/ethernet/allwinner/Kconfig @@ -28,6 +28,7 @@ config SUN4I_EMAC select MII select PHYLIB select MDIO_SUN4I + select MFD_SYSCON ---help--- Support for Allwinner A10 EMAC ethernet driver. diff --git a/drivers/net/ethernet/allwinner/sun4i-emac.c b/drivers/net/ethernet/allwinner/sun4i-emac.c index 1fcd556..86c891d 100644 --- a/drivers/net/ethernet/allwinner/sun4i-emac.c +++ b/drivers/net/ethernet/allwinner/sun4i-emac.c @@ -18,6 +18,8 @@ #include <linux/gpio.h> #include <linux/interrupt.h> #include <linux/irq.h> +#include <linux/mfd/syscon.h> +#include <linux/mfd/syscon/sun4i-sc.h> #include <linux/mii.h> #include <linux/module.h> #include <linux/netdevice.h> @@ -28,6 +30,7 @@ #include <linux/of_platform.h> #include <linux/platform_device.h> #include <linux/phy.h> +#include <linux/regmap.h> #include "sun4i-emac.h" @@ -78,6 +81,7 @@ struct emac_board_info { struct phy_device *phy_dev; struct device_node *phy_node; + struct regmap *sc; unsigned int link; unsigned int speed; unsigned int duplex; @@ -862,6 +866,18 @@ static int emac_probe(struct platform_device *pdev) goto out; } + /* Map SRAM_A3_A4 to EMAC */ + db->sc = syscon_regmap_lookup_by_compatible( + "allwinner,sun4i-a10-syscon"); + if (IS_ERR(db->sc)) { + dev_err(&pdev->dev, "failed to find syscon regmap\n"); + ret = PTR_ERR(db->sc); + goto out; + } + + regmap_update_bits(db->sc, SUN4I_SC1, SUN4I_SC1_SRAM_A3_A4_MAP_MASK, + SUN4I_SC1_SRAM_A3_A4_MAP_EMAC); + /* Read MAC-address from DT */ mac_addr = of_get_mac_address(np); if (mac_addr) @@ -910,7 +926,9 @@ out: static int emac_remove(struct platform_device *pdev) { struct net_device *ndev = platform_get_drvdata(pdev); + struct emac_board_info *db = netdev_priv(ndev); + regmap_update_bits(db->sc, SUN4I_SC1, SUN4I_SC1_SRAM_A3_A4_MAP_MASK, 0); unregister_netdev(ndev); free_netdev(ndev);
The EMAC needs SRAM block A3_A4 being mapped to EMAC peripheral to work. This is done by the bootloader most of the time, but U-Boot Falcon Mode, for example, skips emac initialization and SRAM would stay mapped to the CPU. Signed-off-by: Jens Kuske <jenskuske@gmail.com> --- drivers/net/ethernet/allwinner/Kconfig | 1 + drivers/net/ethernet/allwinner/sun4i-emac.c | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+)