[PATCH 0/3] net: allwinner: sun4i-emac: add missing SRAM mapping

[PATCH 0/3] net: allwinner: sun4i-emac: add missing SRAM mapping

[Jens Kuske]

Hi all,

The current sun4i-emac kernel driver does not set the needed SRAM mapping.

This hasn't been noticed because normally the bootloader already did this.
But in case the bootloader skips initializing the EMAC, like in U-Boot's
Falcon Mode for example, the kernel driver has to take care of it.

Patch 1-2 add a mfd/syscon for the shared registers responsible for SRAM
mapping. These patches are taken from Chen-Yu Tsai's sunxi-musb tree [1] and
I added a copy of the device-tree nodes to sun4i and sun5i.

Patch 3 adds the code to map SRAM to EMAC using the syscon.

Regards,
Jens

[1] https://github.com/wens/linux/commits/wip/sunxi-musb


Chen-Yu Tsai (2):
  ARM: dts: sunxi: Add syscon node for controlling SRAM mapping
  ARM: sunxi: Add register bit definitions for SRAM mapping syscon

Jens Kuske (1):
  net: allwinner: sun4i-emac: fix emac SRAM mapping

 arch/arm/boot/dts/sun4i-a10.dtsi            |  5 ++++
 arch/arm/boot/dts/sun5i-a10s.dtsi           |  5 ++++
 arch/arm/boot/dts/sun5i-a13.dtsi            |  5 ++++
 arch/arm/boot/dts/sun7i-a20.dtsi            |  5 ++++
 drivers/net/ethernet/allwinner/Kconfig      |  1 +
 drivers/net/ethernet/allwinner/sun4i-emac.c | 18 +++++++++++++
 include/linux/mfd/syscon/sun4i-sc.h         | 42 +++++++++++++++++++++++++++++
 7 files changed, 81 insertions(+)
 create mode 100644 include/linux/mfd/syscon/sun4i-sc.h

-- 
2.2.2

[PATCH 1/3] ARM: dts: sunxi: Add syscon node for controlling SRAM mapping

[Jens Kuske]

From: Chen-Yu Tsai <wens@csie.org>

Allwinner SoCs have a system controller module that controls whether
SRAM blocks are mapped to the CPU memory or specific peripherals.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
[jenskuske at gmail.com: duplicate syscon node to sun4i and sun5i]
Signed-off-by: Jens Kuske <jenskuske@gmail.com>
---
 arch/arm/boot/dts/sun4i-a10.dtsi  | 5 +++++
 arch/arm/boot/dts/sun5i-a10s.dtsi | 5 +++++
 arch/arm/boot/dts/sun5i-a13.dtsi  | 5 +++++
 arch/arm/boot/dts/sun7i-a20.dtsi  | 5 +++++
 4 files changed, 20 insertions(+)

diff --git a/arch/arm/boot/dts/sun4i-a10.dtsi b/arch/arm/boot/dts/sun4i-a10.dtsi
index 380f914..8a90e48 100644
--- a/arch/arm/boot/dts/sun4i-a10.dtsi
+++ b/arch/arm/boot/dts/sun4i-a10.dtsi
@@ -339,6 +339,11 @@
 		#size-cells = <1>;
 		ranges;
 
+		syscon: syscon at 01c00000 {
+			compatible = "allwinner,sun4i-a10-syscon", "syscon";
+			reg = <0x01c00000 0x8>;
+		};
+
 		dma: dma-controller at 01c02000 {
 			compatible = "allwinner,sun4i-a10-dma";
 			reg = <0x01c02000 0x1000>;
diff --git a/arch/arm/boot/dts/sun5i-a10s.dtsi b/arch/arm/boot/dts/sun5i-a10s.dtsi
index 531272c..21df7b4 100644
--- a/arch/arm/boot/dts/sun5i-a10s.dtsi
+++ b/arch/arm/boot/dts/sun5i-a10s.dtsi
@@ -300,6 +300,11 @@
 		#size-cells = <1>;
 		ranges;
 
+		syscon: syscon at 01c00000 {
+			compatible = "allwinner,sun4i-a10-syscon", "syscon";
+			reg = <0x01c00000 0x8>;
+		};
+
 		dma: dma-controller at 01c02000 {
 			compatible = "allwinner,sun4i-a10-dma";
 			reg = <0x01c02000 0x1000>;
diff --git a/arch/arm/boot/dts/sun5i-a13.dtsi b/arch/arm/boot/dts/sun5i-a13.dtsi
index b131068..b69b7b1 100644
--- a/arch/arm/boot/dts/sun5i-a13.dtsi
+++ b/arch/arm/boot/dts/sun5i-a13.dtsi
@@ -298,6 +298,11 @@
 		#size-cells = <1>;
 		ranges;
 
+		syscon: syscon at 01c00000 {
+			compatible = "allwinner,sun4i-a10-syscon", "syscon";
+			reg = <0x01c00000 0x8>;
+		};
+
 		dma: dma-controller at 01c02000 {
 			compatible = "allwinner,sun4i-a10-dma";
 			reg = <0x01c02000 0x1000>;
diff --git a/arch/arm/boot/dts/sun7i-a20.dtsi b/arch/arm/boot/dts/sun7i-a20.dtsi
index 82097c9..af9c7e6 100644
--- a/arch/arm/boot/dts/sun7i-a20.dtsi
+++ b/arch/arm/boot/dts/sun7i-a20.dtsi
@@ -451,6 +451,11 @@
 		#size-cells = <1>;
 		ranges;
 
+		syscon: syscon at 01c00000 {
+			compatible = "allwinner,sun4i-a10-syscon", "syscon";
+			reg = <0x01c00000 0x8>;
+		};
+
 		nmi_intc: interrupt-controller at 01c00030 {
 			compatible = "allwinner,sun7i-a20-sc-nmi";
 			interrupt-controller;
-- 
2.2.2

[PATCH 2/3] ARM: sunxi: Add register bit definitions for SRAM mapping syscon

[Jens Kuske]

From: Chen-Yu Tsai <wens@csie.org>

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
Signed-off-by: Jens Kuske <jenskuske@gmail.com>
---
 include/linux/mfd/syscon/sun4i-sc.h | 42 +++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)
 create mode 100644 include/linux/mfd/syscon/sun4i-sc.h

diff --git a/include/linux/mfd/syscon/sun4i-sc.h b/include/linux/mfd/syscon/sun4i-sc.h
new file mode 100644
index 0000000..fb970d9
--- /dev/null
+++ b/include/linux/mfd/syscon/sun4i-sc.h
@@ -0,0 +1,42 @@
+/**
+ * sun4i-sc.h - Allwinner sun4i system control register bit definitions
+ *
+ * Copyright (C) 2014 Chen-Yu Tsai
+ *
+ * Chen-Yu Tsai  <wens@csie.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef __LINUX_SUN4I_SC_H
+#define __LINUX_SUN4I_SC_H
+
+#include <linux/bitops.h>
+
+/* Registers */
+#define SUN4I_SC0	0x00
+#define SUN4I_SC1	0x04
+
+/* SRAM control register 0 bits */
+#define SUN4I_SC0_SRAM_C1_MAP_VE	0x7fffffff
+
+/* SRAM control register 1 bits */
+#define SUN4I_SC1_BIST_NDMA_CTRL_SEL	BIT(31)
+#define SUN4I_SC1_SRAM_C3_MAP_ISP	BIT(12)
+#define SUN4I_SC1_SRAM_C2_MAP_MASK	0x0300
+#define SUN4I_SC1_SRAM_C2_MAP_AE	0x0100
+#define SUN4I_SC1_SRAM_C2_MAP_CE	0x0200
+#define SUN4I_SC1_SRAM_C2_MAP_ACE	0x0300
+#define SUN4I_SC1_SRAM_A3_A4_MAP_MASK	0x0030
+#define SUN4I_SC1_SRAM_A3_A4_MAP_EMAC	0x0010
+#define SUN4I_SC1_SRAM_D_MAP_USB0	BIT(0)
+
+#endif /* __LINUX_SUN4I_SC_H */
-- 
2.2.2

[PATCH 2/3] ARM: sunxi: Add register bit definitions for SRAM mapping syscon

[Lee Jones]

On Sun, 25 Jan 2015, Jens Kuske wrote:

> From: Chen-Yu Tsai <wens@csie.org>
> 
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> Signed-off-by: Jens Kuske <jenskuske@gmail.com>
> ---
>  include/linux/mfd/syscon/sun4i-sc.h | 42 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
>  create mode 100644 include/linux/mfd/syscon/sun4i-sc.h

Acked-by: Lee Jones <lee.jones@linaro.org>

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Jens Kuske]

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(+)

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

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

[Maxime Ripard]

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

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150125/c2a153bc/attachment.sig>

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

[Jens Kuske]

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

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

[Maxime Ripard]

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

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150127/bf56b85f/attachment-0001.sig>