Re: [PATCH 3/3] net: allwinner: sun4i-emac: fix emac SRAM mapping

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.

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

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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:

quoted

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.

quoted

Signed-off-by: Jens Kuske <redacted>
---
  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