[PATCH v5 0/4] Exynos SROMc configuration and Ethernet support for SMDK5410

[PATCH v5 0/4] Exynos SROMc configuration and Ethernet support for SMDK5410

[Pavel Fedin]

This patch extends Exynos SROM controller driver with ability to configure
controller outputs and enables SMSC9115 Ethernet chip on SMDK5410 board,
which is connected via SROMc bank #3.

With this patchset, support for the whole existing SMDK range can be added.
Actually, only bank number is different.

This patchset also depends on Exynos 5410 pinctrl support, introduced by
patches 0003 and 0004 from this set:
[PATCH v4 0/5] ARM: EXYNOS: ODROID-XU DT and LEDs
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/330862.html

Pinctrl support is necessary in order to correctly configure
multifunctional pins of the Exynos chip.

v4 => v5:
- Some cosmetic changes in the dtsi ("compatible" goes first)
- Use correctly specified ranges for the SROMc node
- Reuse existing properties where possible ("reg" for bank# and
  "reg-io-width" for data width)
- Separated page-mode property from timings array
- More improvements to the documentation

v3 => v4:
- Devices are now added as subnodes, with additional properties. This allows
  to cleary specify dependency. If configuration fails, error will be reported
  and child devices will not be probed.
- These additional properties now have "samsung,srom-XXX" format
- Fixed code style, now better understood.

v2 => v3:
- Fixed up SROMc region size in the device tree
- Reordered patches, documentation goes first now

v1 => v2:
- Fixed some typos and bad labels in device tree
- Improved documentation

Pavel Fedin (4):
  Documentation: dt-bindings: Describe SROMc configuration
  ARM: dts: Add SROMc to Exynos 5410
  drivers: exynos-srom: Add support for bank configuration
  ARM: dts: Add Ethernet chip to SMDK5410

 .../bindings/arm/samsung/exynos-srom.txt           | 68 +++++++++++++++++++++-
 arch/arm/boot/dts/exynos5410-smdk5410.dts          | 40 +++++++++++++
 arch/arm/boot/dts/exynos5410.dtsi                  | 11 ++++
 arch/arm/mach-exynos/Kconfig                       |  2 +-
 drivers/soc/samsung/Kconfig                        |  2 +-
 drivers/soc/samsung/exynos-srom.c                  | 60 ++++++++++++++++++-
 6 files changed, 177 insertions(+), 6 deletions(-)

-- 
2.4.4

[PATCH v5 1/4] Documentation: dt-bindings: Describe SROMc configuration

[Pavel Fedin]

Add documentation for new subnode properties, allowing bank configuration.
Based on u-boot implementation, but heavily reworked.

Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
---
 .../bindings/arm/samsung/exynos-srom.txt           | 68 +++++++++++++++++++++-
 1 file changed, 66 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/arm/samsung/exynos-srom.txt b/Documentation/devicetree/bindings/arm/samsung/exynos-srom.txt
index 33886d5..ee378ca 100644
--- a/Documentation/devicetree/bindings/arm/samsung/exynos-srom.txt
+++ b/Documentation/devicetree/bindings/arm/samsung/exynos-srom.txt
@@ -5,8 +5,72 @@ Required properties:
 
 - reg: offset and length of the register set
 
-Example:
+- #address-cells: Must be set to 2 to allow memory address translation
+
+- #size-cells: Must be set to 1 to allow CS address passing
+
+- ranges: Must be set up to reflect the memory layout with four integer values
+	  per bank:
+		<bank-number> 0 <physical address of bank> <size>
+
+Sub-nodes:
+The SROM controller can be used to attach external peripherials. In this case
+device nodes should be added as subnodes to the SROMc node. These subnodes,
+except regular device specification, should contain the following properties,
+describing configuration of the relevant SROM bank:
+
+Required properties:
+- reg: bank number, base address (relative to start of the bank) and size of
+       the memory mapped for the device. Note that base address will be
+       typically 0 as this is the start of the bank.
+
+- samsung,srom-timing : array of 6 integers, specifying bank timings in the
+                        following order: Tacp, Tcah, Tcoh, Tacc, Tcos, Tacs.
+                        Each value is specified in cycles (0 - 15) and has the
+                        following meaning:
+                        Tacp : Page mode access cycle at Page mode
+                        Tcah : Address holding time after CSn
+                        Tcoh : Chip selection hold on OEn
+                        Tacc : Access cycle
+                        Tcos : Chip selection set-up before OEn
+                        Tacs : Address set-up before CSn
+
+Optional properties:
+- reg-io-width : data width in bytes (1 or 2). If omitted, default of 1 is used.
+
+- samsung,srom-page-mode : page mode configuration for the bank:
+			   0 - normal (one data)
+			   1 - four data
+			   If omitted, default of 0 is used.
+
+Example: basic definition, no banks are configured
+	sromc@12570000 {
+		compatible = "samsung,exynos-srom";
+		reg = <0x12570000 0x14>;
+	};
+
+Example: SROMc with smsc 911x ethernet chip on bank 3
 	sromc@12570000 {
+		#address-cells = <2>;
+		#size-cells = <1>;
+		ranges = <0 0 0x04000000 0x20000   // Bank0
+			  1 0 0x05000000 0x20000   // Bank1
+			  2 0 0x06000000 0x20000   // Bank2
+			  3 0 0x07000000 0x20000>; // Bank3
+
 		compatible = "samsung,exynos-srom";
-		reg = <0x12570000 0x10>;
+		reg = <0x12570000 0x14>;
+
+		ethernet@3 {
+			compatible = "smsc,lan9115";
+			reg = <3 0 0x10000>;	   // Bank 3, offset = 0
+			phy-mode = "mii";
+			interrupt-parent = <&gpx0>;
+			interrupts = <5 8>;
+			reg-io-width = <2>;
+			smsc,irq-push-pull;
+			smsc,force-internal-phy;
+
+			samsung,srom-config = <1 9 12 1 9 1 1>;
+		};
 	};
-- 
2.4.4

[PATCH v5 2/4] ARM: dts: Add SROMc to Exynos 5410

[Pavel Fedin]

This machine uses own SoC device tree file, add missing part.
We insert the complete description, with ranges, because we are going to
connect devices to it. Values in ranges are SoC-specific, so they go here
in order not to duplicate them for every machine.

Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
---
 arch/arm/boot/dts/exynos5410.dtsi | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm/boot/dts/exynos5410.dtsi b/arch/arm/boot/dts/exynos5410.dtsi
index 4603356..9cfb814 100644
--- a/arch/arm/boot/dts/exynos5410.dtsi
+++ b/arch/arm/boot/dts/exynos5410.dtsi
@@ -101,6 +101,17 @@
 			reg = <0x10000000 0x100>;
 		};
 
+		sromc: sromc@12250000 {
+			compatible = "samsung,exynos-srom";
+			reg = <0x12250000 0x14>;
+			#address-cells = <2>;
+			#size-cells = <1>;
+			ranges = <0 0 0x04000000 0x20000
+				  1 0 0x05000000 0x20000
+				  2 0 0x06000000 0x20000
+				  3 0 0x07000000 0x20000>;
+		};
+
 		pmu_system_controller: system-controller@10040000 {
 			compatible = "samsung,exynos5410-pmu", "syscon";
 			reg = <0x10040000 0x5000>;
-- 
2.4.4

[PATCH v5 3/4] drivers: exynos-srom: Add support for bank configuration

[Pavel Fedin]

Implement handling properties in subnodes and adding child devices to the
system. Child devices will not be added if configuration fails.

Since the driver now does more than suspend-resume support, dependency on
CONFIG_PM is removed.

Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
---
 arch/arm/mach-exynos/Kconfig      |  2 +-
 drivers/soc/samsung/Kconfig       |  2 +-
 drivers/soc/samsung/exynos-srom.c | 60 +++++++++++++++++++++++++++++++++++++--
 3 files changed, 60 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mach-exynos/Kconfig b/arch/arm/mach-exynos/Kconfig
index 83c85f5..c22dc42 100644
--- a/arch/arm/mach-exynos/Kconfig
+++ b/arch/arm/mach-exynos/Kconfig
@@ -16,7 +16,7 @@ menuconfig ARCH_EXYNOS
 	select ARM_GIC
 	select COMMON_CLK_SAMSUNG
 	select EXYNOS_THERMAL
-	select EXYNOS_SROM if PM
+	select EXYNOS_SROM
 	select HAVE_ARM_SCU if SMP
 	select HAVE_S3C2410_I2C if I2C
 	select HAVE_S3C2410_WATCHDOG if WATCHDOG
diff --git a/drivers/soc/samsung/Kconfig b/drivers/soc/samsung/Kconfig
index 2833b5b..ea4bc2a 100644
--- a/drivers/soc/samsung/Kconfig
+++ b/drivers/soc/samsung/Kconfig
@@ -8,6 +8,6 @@ config SOC_SAMSUNG
 
 config EXYNOS_SROM
 	bool
-	depends on ARM && ARCH_EXYNOS && PM
+	depends on ARM && ARCH_EXYNOS
 
 endmenu
diff --git a/drivers/soc/samsung/exynos-srom.c b/drivers/soc/samsung/exynos-srom.c
index 57a232d..49b5c9e 100644
--- a/drivers/soc/samsung/exynos-srom.c
+++ b/drivers/soc/samsung/exynos-srom.c
@@ -14,6 +14,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
+#include <linux/of_platform.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 
@@ -67,11 +68,50 @@ static struct exynos_srom_reg_dump *exynos_srom_alloc_reg_dump(
 	return rd;
 }
 
+static int decode_sromc(struct exynos_srom *srom, struct device_node *np)
+{
+	u32 bank, width, pmc;
+	u32 timing[6];
+	u32 cs, bw;
+
+	if (of_property_read_u32(np, "reg", &bank))
+		return -EINVAL;
+	if (of_property_read_u32(np, "reg-io-width", &width))
+		width = 1;
+	if (of_property_read_u32(np, "samsung,srom-page-mode", &pmc))
+		pmc = 0;
+	if (of_property_read_u32_array(np, "samsung,srom-timing", timing,
+				       ARRAY_SIZE(timing)))
+		return -EINVAL;
+
+	bank *= 4; /* Convert bank into shift/offset */
+
+	cs = 1 << EXYNOS_SROM_BW__BYTEENABLE__SHIFT;
+	if (width == 2)
+		cs |= 1 << EXYNOS_SROM_BW__DATAWIDTH__SHIFT;
+
+	bw = __raw_readl(srom->reg_base + EXYNOS_SROM_BW);
+	bw = (bw & ~(EXYNOS_SROM_BW__CS_MASK << bank)) | (cs << bank);
+	__raw_writel(bw, srom->reg_base + EXYNOS_SROM_BW);
+
+	__raw_writel((pmc << EXYNOS_SROM_BCX__PMC__SHIFT) |
+		    (timing[0] << EXYNOS_SROM_BCX__TACP__SHIFT) |
+		    (timing[1] << EXYNOS_SROM_BCX__TCAH__SHIFT) |
+		    (timing[2] << EXYNOS_SROM_BCX__TCOH__SHIFT) |
+		    (timing[3] << EXYNOS_SROM_BCX__TACC__SHIFT) |
+		    (timing[4] << EXYNOS_SROM_BCX__TCOS__SHIFT) |
+		    (timing[5] << EXYNOS_SROM_BCX__TACS__SHIFT),
+		    srom->reg_base + EXYNOS_SROM_BC0 + bank);
+
+	return 0;
+}
+
 static int exynos_srom_probe(struct platform_device *pdev)
 {
-	struct device_node *np;
+	struct device_node *np, *child;
 	struct exynos_srom *srom;
 	struct device *dev = &pdev->dev;
+	bool error = false;
 
 	np = dev->of_node;
 	if (!np) {
@@ -100,7 +140,23 @@ static int exynos_srom_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
-	return 0;
+	for_each_child_of_node(np, child) {
+		if (decode_sromc(srom, child)) {
+			dev_err(dev,
+				"Could not decode bank configuration for %s\n",
+				child->name);
+			error = true;
+		}
+	}
+
+	/*
+	 * If any bank failed to configure, we still provide suspend/resume,
+	 * but do not probe child devices
+	 */
+	if (error)
+		return 0;
+
+	return of_platform_populate(np, NULL, NULL, dev);
 }
 
 static int exynos_srom_remove(struct platform_device *pdev)
-- 
2.4.4

[PATCH v5 4/4] ARM: dts: Add Ethernet chip to SMDK5410

[Pavel Fedin]

The chip is smsc9115, connected via SROMc bank 3. Additionally, some GPIO
initialization is required.

Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
---
 arch/arm/boot/dts/exynos5410-smdk5410.dts | 40 +++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/arch/arm/boot/dts/exynos5410-smdk5410.dts b/arch/arm/boot/dts/exynos5410-smdk5410.dts
index cebeaab..f41952f 100644
--- a/arch/arm/boot/dts/exynos5410-smdk5410.dts
+++ b/arch/arm/boot/dts/exynos5410-smdk5410.dts
@@ -61,6 +61,27 @@
 	disable-wp;
 };
 
+&pinctrl_0 {
+	srom_ctl: srom-ctl {
+		samsung,pins = "gpy0-3", "gpy0-4", "gpy0-5",
+			       "gpy1-0", "gpy1-1", "gpy1-2", "gpy1-3";
+		samsung,pin-function = <2>;
+		samsung,pin-drv = <0>;
+	};
+
+	srom_ebi: srom-ebi {
+		samsung,pins = "gpy3-0", "gpy3-1", "gpy3-2", "gpy3-3",
+			       "gpy3-4", "gpy3-5", "gpy3-6", "gpy3-7",
+			       "gpy5-0", "gpy5-1", "gpy5-2", "gpy5-3",
+			       "gpy5-4", "gpy5-5", "gpy5-6", "gpy5-7",
+			       "gpy6-0", "gpy6-1", "gpy6-2", "gpy6-3",
+			       "gpy6-4", "gpy6-5", "gpy6-6", "gpy6-7";
+		samsung,pin-function = <2>;
+		samsung,pin-pud = <3>;
+		samsung,pin-drv = <0>;
+	};
+};
+
 &uart0 {
 	status = "okay";
 };
@@ -72,3 +93,22 @@
 &uart2 {
 	status = "okay";
 };
+
+&sromc {
+	pinctrl-names = "default";
+	pinctrl-0 = <&srom_ctl>, <&srom_ebi>;
+
+	ethernet@3 {
+		compatible = "smsc,lan9115";
+		reg = <3 0 0x10000>;
+		phy-mode = "mii";
+		interrupt-parent = <&gpx0>;
+		interrupts = <5 8>;
+		reg-io-width = <2>;
+		smsc,irq-push-pull;
+		smsc,force-internal-phy;
+
+		samsung,srom-page-mode = <1>;
+		samsung,srom-timing = <9 12 1 9 1 1>;
+	};
+};
-- 
2.4.4

Re: [PATCH v5 0/4] Exynos SROMc configuration and Ethernet support for SMDK5410

[Krzysztof Kozlowski]

On 03.11.2015 18:16, Pavel Fedin wrote:
> This patch extends Exynos SROM controller driver with ability to configure
> controller outputs and enables SMSC9115 Ethernet chip on SMDK5410 board,
> which is connected via SROMc bank #3.
> 
> With this patchset, support for the whole existing SMDK range can be added.
> Actually, only bank number is different.
> 
> This patchset also depends on Exynos 5410 pinctrl support, introduced by
> patches 0003 and 0004 from this set:
> [PATCH v4 0/5] ARM: EXYNOS: ODROID-XU DT and LEDs
> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/330862.html

Only the first patch made up to kernel. The rest (2-5) received feedback
and I am waiting for fixing it. Andreas apparently loose the interest...

Best regards,
Krzysztof

Re: [PATCH v5 1/4] Documentation: dt-bindings: Describe SROMc configuration

[Krzysztof Kozlowski]

On 03.11.2015 18:16, Pavel Fedin wrote:
> Add documentation for new subnode properties, allowing bank configuration.
> Based on u-boot implementation, but heavily reworked.

Please mention also fixing the size of mapped memory in <reg> in the
example.

> 
> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
> ---
>  .../bindings/arm/samsung/exynos-srom.txt           | 68 +++++++++++++++++++++-
>  1 file changed, 66 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/arm/samsung/exynos-srom.txt b/Documentation/devicetree/bindings/arm/samsung/exynos-srom.txt
> index 33886d5..ee378ca 100644
> --- a/Documentation/devicetree/bindings/arm/samsung/exynos-srom.txt
> +++ b/Documentation/devicetree/bindings/arm/samsung/exynos-srom.txt
> @@ -5,8 +5,72 @@ Required properties:
>  
>  - reg: offset and length of the register set
>  
> -Example:
> +- #address-cells: Must be set to 2 to allow memory address translation
> +
> +- #size-cells: Must be set to 1 to allow CS address passing
> +
> +- ranges: Must be set up to reflect the memory layout with four integer values
> +	  per bank:
> +		<bank-number> 0 <physical address of bank> <size>
> +

These are mentioned as required but actually they shouldn't be, if there
are no peripherals.

By making them required you would be breaking ABI. I see that actually
the ABI is not broken by the driver so maybe just add a separate
optional section for subdevices.
In that section you would described required attributes and subnodes.

> +Sub-nodes:
> +The SROM controller can be used to attach external peripherials. In this case

s/peripherials/peripherals/

> +device nodes should be added as subnodes to the SROMc node. These subnodes,
> +except regular device specification, should contain the following properties,
> +describing configuration of the relevant SROM bank:
> +
> +Required properties:
> +- reg: bank number, base address (relative to start of the bank) and size of
> +       the memory mapped for the device. Note that base address will be
> +       typically 0 as this is the start of the bank.
> +
> +- samsung,srom-timing : array of 6 integers, specifying bank timings in the
> +                        following order: Tacp, Tcah, Tcoh, Tacc, Tcos, Tacs.
> +                        Each value is specified in cycles (0 - 15) and has the
> +                        following meaning:
> +                        Tacp : Page mode access cycle at Page mode
> +                        Tcah : Address holding time after CSn
> +                        Tcoh : Chip selection hold on OEn
> +                        Tacc : Access cycle

This one has values 1-32 clock cycles

> +                        Tcos : Chip selection set-up before OEn
> +                        Tacs : Address set-up before CSn
> +
> +Optional properties:
> +- reg-io-width : data width in bytes (1 or 2). If omitted, default of 1 is used.
> +
> +- samsung,srom-page-mode : page mode configuration for the bank:
> +			   0 - normal (one data)
> +			   1 - four data
> +			   If omitted, default of 0 is used.
> +
> +Example: basic definition, no banks are configured
> +	sromc@12570000 {
> +		compatible = "samsung,exynos-srom";
> +		reg = <0x12570000 0x14>;
> +	};
> +
> +Example: SROMc with smsc 911x ethernet chip on bank 3

Maybe:
s/smsc 911x ethernet/SMSC911x Ethernet/

Best regards,
Krzysztof

>  	sromc@12570000 {
> +		#address-cells = <2>;
> +		#size-cells = <1>;
> +		ranges = <0 0 0x04000000 0x20000   // Bank0
> +			  1 0 0x05000000 0x20000   // Bank1
> +			  2 0 0x06000000 0x20000   // Bank2
> +			  3 0 0x07000000 0x20000>; // Bank3
> +
>  		compatible = "samsung,exynos-srom";
> -		reg = <0x12570000 0x10>;
> +		reg = <0x12570000 0x14>;
> +
> +		ethernet@3 {
> +			compatible = "smsc,lan9115";
> +			reg = <3 0 0x10000>;	   // Bank 3, offset = 0
> +			phy-mode = "mii";
> +			interrupt-parent = <&gpx0>;
> +			interrupts = <5 8>;
> +			reg-io-width = <2>;
> +			smsc,irq-push-pull;
> +			smsc,force-internal-phy;
> +
> +			samsung,srom-config = <1 9 12 1 9 1 1>;
> +		};
>  	};
> 

Re: [PATCH v5 2/4] ARM: dts: Add SROMc to Exynos 5410

[Krzysztof Kozlowski]

On 03.11.2015 18:16, Pavel Fedin wrote:
> This machine uses own SoC device tree file, add missing part.
> We insert the complete description, with ranges, because we are going to
> connect devices to it. Values in ranges are SoC-specific, so they go here
> in order not to duplicate them for every machine.
> 
> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
> ---
>  arch/arm/boot/dts/exynos5410.dtsi | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/exynos5410.dtsi b/arch/arm/boot/dts/exynos5410.dtsi
> index 4603356..9cfb814 100644
> --- a/arch/arm/boot/dts/exynos5410.dtsi
> +++ b/arch/arm/boot/dts/exynos5410.dtsi
> @@ -101,6 +101,17 @@
>  			reg = <0x10000000 0x100>;
>  		};
>  
> +		sromc: sromc@12250000 {
> +			compatible = "samsung,exynos-srom";
> +			reg = <0x12250000 0x14>;
> +			#address-cells = <2>;
> +			#size-cells = <1>;
> +			ranges = <0 0 0x04000000 0x20000
> +				  1 0 0x05000000 0x20000
> +				  2 0 0x06000000 0x20000
> +				  3 0 0x07000000 0x20000>;

Following my comments for bindings documentation - I think it is better
to add the address-cells, size-cells and ranges in 4th patch.

Because actually in this patch you are adding just basic support for
SROM controller: for saving and restoring registers. It could be merged
even without the rest of patchset.

Best regards,
Krzysztof

> +		};
> +
>  		pmu_system_controller: system-controller@10040000 {
>  			compatible = "samsung,exynos5410-pmu", "syscon";
>  			reg = <0x10040000 0x5000>;
> 

Re: [PATCH v5 3/4] drivers: exynos-srom: Add support for bank configuration

[Krzysztof Kozlowski]

On 03.11.2015 18:16, Pavel Fedin wrote:
> Implement handling properties in subnodes and adding child devices to the
> system. Child devices will not be added if configuration fails.
> 
> Since the driver now does more than suspend-resume support, dependency on
> CONFIG_PM is removed.
> 
> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
> ---
>  arch/arm/mach-exynos/Kconfig      |  2 +-
>  drivers/soc/samsung/Kconfig       |  2 +-
>  drivers/soc/samsung/exynos-srom.c | 60 +++++++++++++++++++++++++++++++++++++--
>  3 files changed, 60 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/mach-exynos/Kconfig b/arch/arm/mach-exynos/Kconfig
> index 83c85f5..c22dc42 100644
> --- a/arch/arm/mach-exynos/Kconfig
> +++ b/arch/arm/mach-exynos/Kconfig
> @@ -16,7 +16,7 @@ menuconfig ARCH_EXYNOS
>  	select ARM_GIC
>  	select COMMON_CLK_SAMSUNG
>  	select EXYNOS_THERMAL
> -	select EXYNOS_SROM if PM
> +	select EXYNOS_SROM
>  	select HAVE_ARM_SCU if SMP
>  	select HAVE_S3C2410_I2C if I2C
>  	select HAVE_S3C2410_WATCHDOG if WATCHDOG
> diff --git a/drivers/soc/samsung/Kconfig b/drivers/soc/samsung/Kconfig
> index 2833b5b..ea4bc2a 100644
> --- a/drivers/soc/samsung/Kconfig
> +++ b/drivers/soc/samsung/Kconfig
> @@ -8,6 +8,6 @@ config SOC_SAMSUNG
>  
>  config EXYNOS_SROM
>  	bool
> -	depends on ARM && ARCH_EXYNOS && PM
> +	depends on ARM && ARCH_EXYNOS
>  
>  endmenu
> diff --git a/drivers/soc/samsung/exynos-srom.c b/drivers/soc/samsung/exynos-srom.c
> index 57a232d..49b5c9e 100644
> --- a/drivers/soc/samsung/exynos-srom.c
> +++ b/drivers/soc/samsung/exynos-srom.c
> @@ -14,6 +14,7 @@
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_address.h>
> +#include <linux/of_platform.h>
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
>  
> @@ -67,11 +68,50 @@ static struct exynos_srom_reg_dump *exynos_srom_alloc_reg_dump(
>  	return rd;
>  }
>  
> +static int decode_sromc(struct exynos_srom *srom, struct device_node *np)

I missed that one previously: add prefix and more descriptive name, like:
exynos_srom_parse_child()

> +{
> +	u32 bank, width, pmc;
> +	u32 timing[6];
> +	u32 cs, bw;
> +
> +	if (of_property_read_u32(np, "reg", &bank))
> +		return -EINVAL;
> +	if (of_property_read_u32(np, "reg-io-width", &width))
> +		width = 1;
> +	if (of_property_read_u32(np, "samsung,srom-page-mode", &pmc))
> +		pmc = 0;
> +	if (of_property_read_u32_array(np, "samsung,srom-timing", timing,
> +				       ARRAY_SIZE(timing)))
> +		return -EINVAL;
> +
> +	bank *= 4; /* Convert bank into shift/offset */
> +
> +	cs = 1 << EXYNOS_SROM_BW__BYTEENABLE__SHIFT;
> +	if (width == 2)
> +		cs |= 1 << EXYNOS_SROM_BW__DATAWIDTH__SHIFT;
> +
> +	bw = __raw_readl(srom->reg_base + EXYNOS_SROM_BW);
> +	bw = (bw & ~(EXYNOS_SROM_BW__CS_MASK << bank)) | (cs << bank);
> +	__raw_writel(bw, srom->reg_base + EXYNOS_SROM_BW);
> +
> +	__raw_writel((pmc << EXYNOS_SROM_BCX__PMC__SHIFT) |
> +		    (timing[0] << EXYNOS_SROM_BCX__TACP__SHIFT) |
> +		    (timing[1] << EXYNOS_SROM_BCX__TCAH__SHIFT) |
> +		    (timing[2] << EXYNOS_SROM_BCX__TCOH__SHIFT) |
> +		    (timing[3] << EXYNOS_SROM_BCX__TACC__SHIFT) |
> +		    (timing[4] << EXYNOS_SROM_BCX__TCOS__SHIFT) |
> +		    (timing[5] << EXYNOS_SROM_BCX__TACS__SHIFT),
> +		    srom->reg_base + EXYNOS_SROM_BC0 + bank);
> +
> +	return 0;
> +}
> +
>  static int exynos_srom_probe(struct platform_device *pdev)
>  {
> -	struct device_node *np;
> +	struct device_node *np, *child;
>  	struct exynos_srom *srom;
>  	struct device *dev = &pdev->dev;
> +	bool error = false;

The 'error' name is misleading - like error for entire probe which is
not true.

Instead split it to separate function like:

+static int exynos_srom_parse_children(....) {
+       int ret = 0;
+
+	for_each_child_of_node(np, child) {
+               ret = exynos_srom_parse_child(srom, child);
+		if (ret) {
+			dev_err(dev,
+				"Could not decode bank configuration for %s: %d\n",
+				child->name, ret);
+			break;
+		}
+	}
+
+       return ret;
+}

Best regards,
Krzysztof

>  
>  	np = dev->of_node;
>  	if (!np) {
> @@ -100,7 +140,23 @@ static int exynos_srom_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  	}
>  
> -	return 0;
> +	for_each_child_of_node(np, child) {
> +		if (decode_sromc(srom, child)) {
> +			dev_err(dev,
> +				"Could not decode bank configuration for %s\n",
> +				child->name);
> +			error = true;
> +		}
> +	}
> +
> +	/*
> +	 * If any bank failed to configure, we still provide suspend/resume,
> +	 * but do not probe child devices
> +	 */
> +	if (error)
> +		return 0;
> +
> +	return of_platform_populate(np, NULL, NULL, dev);
>  }
>  
>  static int exynos_srom_remove(struct platform_device *pdev)
> 

Re: [PATCH v5 4/4] ARM: dts: Add Ethernet chip to SMDK5410

[Krzysztof Kozlowski]

On 03.11.2015 18:16, Pavel Fedin wrote:
> The chip is smsc9115, connected via SROMc bank 3. Additionally, some GPIO
> initialization is required.
> 
> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
> ---
>  arch/arm/boot/dts/exynos5410-smdk5410.dts | 40 +++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/exynos5410-smdk5410.dts b/arch/arm/boot/dts/exynos5410-smdk5410.dts
> index cebeaab..f41952f 100644
> --- a/arch/arm/boot/dts/exynos5410-smdk5410.dts
> +++ b/arch/arm/boot/dts/exynos5410-smdk5410.dts
> @@ -61,6 +61,27 @@
>  	disable-wp;
>  };
>  
> +&pinctrl_0 {
> +	srom_ctl: srom-ctl {
> +		samsung,pins = "gpy0-3", "gpy0-4", "gpy0-5",
> +			       "gpy1-0", "gpy1-1", "gpy1-2", "gpy1-3";
> +		samsung,pin-function = <2>;
> +		samsung,pin-drv = <0>;
> +	};
> +
> +	srom_ebi: srom-ebi {
> +		samsung,pins = "gpy3-0", "gpy3-1", "gpy3-2", "gpy3-3",
> +			       "gpy3-4", "gpy3-5", "gpy3-6", "gpy3-7",
> +			       "gpy5-0", "gpy5-1", "gpy5-2", "gpy5-3",
> +			       "gpy5-4", "gpy5-5", "gpy5-6", "gpy5-7",
> +			       "gpy6-0", "gpy6-1", "gpy6-2", "gpy6-3",
> +			       "gpy6-4", "gpy6-5", "gpy6-6", "gpy6-7";
> +		samsung,pin-function = <2>;
> +		samsung,pin-pud = <3>;
> +		samsung,pin-drv = <0>;
> +	};
> +};
> +
>  &uart0 {
>  	status = "okay";
>  };
> @@ -72,3 +93,22 @@
>  &uart2 {
>  	status = "okay";
>  };
> +
> +&sromc {

Put sromc in alphabetical order.

> +	pinctrl-names = "default";
> +	pinctrl-0 = <&srom_ctl>, <&srom_ebi>;
> +
> +	ethernet@3 {
> +		compatible = "smsc,lan9115";
> +		reg = <3 0 0x10000>;
> +		phy-mode = "mii";
> +		interrupt-parent = <&gpx0>;
> +		interrupts = <5 8>;

s/8/IRQ_TYPE_LEVEL_LOW/
(is this really level low interrupt?)

> +		reg-io-width = <2>;
> +		smsc,irq-push-pull;
> +		smsc,force-internal-phy;
> +
> +		samsung,srom-page-mode = <1>;
> +		samsung,srom-timing = <9 12 1 9 1 1>;

Some other DTS include regulators: vddvario-supply and vdd33a-supply. It
seems that they are not described in SMSC911x bindings but in
GPMC-eth... but the smsc911x driver is requesting them. Could you
investigate that? I think these regulators should be provided (and
SMSC911x bindings should be updated).

> +	};
> +};

I don't have the board schematics so I couldn't verify the GPIOs.
Overall looks good.

Best regards,
Krzysztof

RE: [PATCH v5 0/4] Exynos SROMc configuration and Ethernet support for SMDK5410

[Pavel Fedin]

 Hello!

> > This patchset also depends on Exynos 5410 pinctrl support, introduced by
> > patches 0003 and 0004 from this set:
> > [PATCH v4 0/5] ARM: EXYNOS: ODROID-XU DT and LEDs
> > http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/330862.html
> 
> Only the first patch made up to kernel. The rest (2-5) received feedback
> and I am waiting for fixing it. Andreas apparently loose the interest...

 I have rechecked the original thread on archives and found nothing new. Where can
i read those feedbacks? Perhaps it makes sense for me to pick them up and include
into my set.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [PATCH v5 0/4] Exynos SROMc configuration and Ethernet support for SMDK5410

[Krzysztof Kozlowski]

On 05.11.2015 16:59, Pavel Fedin wrote:
>  Hello!
> 
>>> This patchset also depends on Exynos 5410 pinctrl support, introduced by
>>> patches 0003 and 0004 from this set:
>>> [PATCH v4 0/5] ARM: EXYNOS: ODROID-XU DT and LEDs
>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/330862.html
>>
>> Only the first patch made up to kernel. The rest (2-5) received feedback
>> and I am waiting for fixing it. Andreas apparently loose the interest...
> 
>  I have rechecked the original thread on archives and found nothing new. Where can
> i read those feedbacks? Perhaps it makes sense for me to pick them up and include
> into my set.

Go ahead. The only waiting feedback is at patch 2/5:

http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/330910.html
Javier's pointed use of linux,stdout-path and putting xxti in
exynos5410.dtsi (and customizing it per board if needed).

Patches already got reviews and acks so only 2/5 is the stopper.

Best regards,
Krzysztof

RE: [PATCH v5 0/4] Exynos SROMc configuration and Ethernet support for SMDK5410

[Pavel Fedin]

 Hello!

> >>> This patchset also depends on Exynos 5410 pinctrl support, introduced by
> >>> patches 0003 and 0004 from this set:
> >>> [PATCH v4 0/5] ARM: EXYNOS: ODROID-XU DT and LEDs
> >>> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/330862.html
> >>
> >> Only the first patch made up to kernel. The rest (2-5) received feedback
> >> and I am waiting for fixing it. Andreas apparently loose the interest...
> >
> >  I have rechecked the original thread on archives and found nothing new. Where can
> > i read those feedbacks? Perhaps it makes sense for me to pick them up and include
> > into my set.
> 
> Go ahead. The only waiting feedback is at patch 2/5:
> 
> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/330910.html
> Javier's pointed use of linux,stdout-path and putting xxti in
> exynos5410.dtsi (and customizing it per board if needed).
> 
> Patches already got reviews and acks so only 2/5 is the stopper.

 Ah, it's odroidXU device tree. Sorry, i don't have this particular board, and my patch actually doesn't need it. I could do some
blind modifications, but cannot test them, and this is out of my scope at all.
 As mentioned in the commit msg, i need only 0003 and 0004 from there. Can they be picked up independently? They don't need 0002 by
themselves, only 0005 needs it.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [PATCH v5 0/4] Exynos SROMc configuration and Ethernet support for SMDK5410

[Krzysztof Kozlowski]

On 05.11.2015 17:44, Pavel Fedin wrote:
>  Hello!
> 
>>>>> This patchset also depends on Exynos 5410 pinctrl support, introduced by
>>>>> patches 0003 and 0004 from this set:
>>>>> [PATCH v4 0/5] ARM: EXYNOS: ODROID-XU DT and LEDs
>>>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/330862.html
>>>>
>>>> Only the first patch made up to kernel. The rest (2-5) received feedback
>>>> and I am waiting for fixing it. Andreas apparently loose the interest...
>>>
>>>  I have rechecked the original thread on archives and found nothing new. Where can
>>> i read those feedbacks? Perhaps it makes sense for me to pick them up and include
>>> into my set.
>>
>> Go ahead. The only waiting feedback is at patch 2/5:
>>
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/330910.html
>> Javier's pointed use of linux,stdout-path and putting xxti in
>> exynos5410.dtsi (and customizing it per board if needed).
>>
>> Patches already got reviews and acks so only 2/5 is the stopper.
> 
>  Ah, it's odroidXU device tree. Sorry, i don't have this particular board, and my patch actually doesn't need it. I could do some
> blind modifications, but cannot test them, and this is out of my scope at all.
>  As mentioned in the commit msg, i need only 0003 and 0004 from there. Can they be picked up independently? They don't need 0002 by
> themselves, only 0005 needs it.

Indeed the order of patches is messed up and this makes the confusion. I
was waiting for resend of entire patchset but you are right - this is
not required. 3 and 4 can be applied independently.

If they apply, I will pick up them after merge window.
If they don't, they will need rebasing by someone. Maybe you can take
care of them? You have the SMDK5410 for testing, right?

Best regards,
Krzysztof

RE: [PATCH v5 0/4] Exynos SROMc configuration and Ethernet support for SMDK5410

[Pavel Fedin]

 Hello!

> >>>>> This patchset also depends on Exynos 5410 pinctrl support, introduced by
> >>>>> patches 0003 and 0004 from this set:
> >>>>> [PATCH v4 0/5] ARM: EXYNOS: ODROID-XU DT and LEDs
> >>>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/330862.html
> >>>>
> >>>> Only the first patch made up to kernel. The rest (2-5) received feedback
> >>>> and I am waiting for fixing it. Andreas apparently loose the interest...
> >>>
> >>>  I have rechecked the original thread on archives and found nothing new. Where can
> >>> i read those feedbacks? Perhaps it makes sense for me to pick them up and include
> >>> into my set.
> >>
> >> Go ahead. The only waiting feedback is at patch 2/5:
> >>
> >> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/330910.html
> >> Javier's pointed use of linux,stdout-path and putting xxti in
> >> exynos5410.dtsi (and customizing it per board if needed).
> >>
> >> Patches already got reviews and acks so only 2/5 is the stopper.
> >
> >  Ah, it's odroidXU device tree. Sorry, i don't have this particular board, and my patch
> actually doesn't need it. I could do some
> > blind modifications, but cannot test them, and this is out of my scope at all.
> >  As mentioned in the commit msg, i need only 0003 and 0004 from there. Can they be picked up
> independently? They don't need 0002 by
> > themselves, only 0005 needs it.
> 
> Indeed the order of patches is messed up and this makes the confusion. I
> was waiting for resend of entire patchset but you are right - this is
> not required. 3 and 4 can be applied independently.
> 
> If they apply, I will pick up them after merge window.

 They should. I have quite recent linux-next tree, i've just checked,
0003 applies with a small fuzz and 0004 applies without problems at all.

> If they don't, they will need rebasing by someone. Maybe you can take
> care of them?

 Of course. Please notify me if rebasing will be needed, i'll do it.

> You have the SMDK5410 for testing, right?

 Of course. And you can already get my

Tested-by: Pavel Fedin <p.fedin@samsung.com>

 on those two because my experimental tree already uses them, and
everything works quite well.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

RE: [PATCH v5 3/4] drivers: exynos-srom: Add support for bank configuration

[Pavel Fedin]

 Hello!

> > +static int decode_sromc(struct exynos_srom *srom, struct device_node *np)
> 
> I missed that one previously: add prefix and more descriptive name, like:
> exynos_srom_parse_child()

 exynos_srom_configure_bank(), is this name OK?

> >  static int exynos_srom_probe(struct platform_device *pdev)
> >  {
> > -	struct device_node *np;
> > +	struct device_node *np, *child;
> >  	struct exynos_srom *srom;
> >  	struct device *dev = &pdev->dev;
> > +	bool error = false;
> 
> The 'error' name is misleading - like error for entire probe which is
> not true.
> 
> Instead split it to separate function like:
> 
> +static int exynos_srom_parse_children(....) {
> +       int ret = 0;
> +
> +	for_each_child_of_node(np, child) {
> +               ret = exynos_srom_parse_child(srom, child);
> +		if (ret) {
> +			dev_err(dev,
> +				"Could not decode bank configuration for %s: %d\n",
> +				child->name, ret);
> +			break;
> +		}
> +	}
> +
> +       return ret;
> +}

 Factoring out this loop is unnecessary, because i could just 'return 0' in the loop
instead of 'error = true'. Byt my idea is to go through all banks anyway, just in
case, to diagnose all of them. So that the user will be able to spot and fix all
broken banks at once, instead of doing one-by-one.
 I have renamed the variable to 'bool bad_bank_config', will this be OK?

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [PATCH v5 3/4] drivers: exynos-srom: Add support for bank configuration

[Krzysztof Kozlowski]

W dniu 05.11.2015 o 19:40, Pavel Fedin pisze:
>  Hello!
> 
>>> +static int decode_sromc(struct exynos_srom *srom, struct device_node *np)
>>
>> I missed that one previously: add prefix and more descriptive name, like:
>> exynos_srom_parse_child()
> 
>  exynos_srom_configure_bank(), is this name OK?

Yes, its OK.

> 
>>>  static int exynos_srom_probe(struct platform_device *pdev)
>>>  {
>>> -	struct device_node *np;
>>> +	struct device_node *np, *child;
>>>  	struct exynos_srom *srom;
>>>  	struct device *dev = &pdev->dev;
>>> +	bool error = false;
>>
>> The 'error' name is misleading - like error for entire probe which is
>> not true.
>>
>> Instead split it to separate function like:
>>
>> +static int exynos_srom_parse_children(....) {
>> +       int ret = 0;
>> +
>> +	for_each_child_of_node(np, child) {
>> +               ret = exynos_srom_parse_child(srom, child);
>> +		if (ret) {
>> +			dev_err(dev,
>> +				"Could not decode bank configuration for %s: %d\n",
>> +				child->name, ret);
>> +			break;
>> +		}
>> +	}
>> +
>> +       return ret;
>> +}
> 
>  Factoring out this loop is unnecessary, because i could just 'return 0' in the loop
> instead of 'error = true'. Byt my idea is to go through all banks anyway, just in
> case, to diagnose all of them. So that the user will be able to spot and fix all
> broken banks at once, instead of doing one-by-one.
>  I have renamed the variable to 'bool bad_bank_config', will this be OK?

Yes, that's also OK.

Best regards,
Krzysztof

RE: [PATCH v5 4/4] ARM: dts: Add Ethernet chip to SMDK5410

[Pavel Fedin]

 Hello!

> > +	ethernet@3 {
> > +		compatible = "smsc,lan9115";
> > +		reg = <3 0 0x10000>;
> > +		phy-mode = "mii";
> > +		interrupt-parent = <&gpx0>;
> > +		interrupts = <5 8>;
> 
> s/8/IRQ_TYPE_LEVEL_LOW/
> (is this really level low interrupt?)

 Yes, according to: https://github.com/AndreiLux/Perseus-S3/blob/master/arch/arm/mach-exynos/mach-smdk5250.c#L133

> Some other DTS include regulators: vddvario-supply and vdd33a-supply. It
> seems that they are not described in SMSC911x bindings but in
> GPMC-eth... but the smsc911x driver is requesting them. Could you
> investigate that? I think these regulators should be provided (and
> SMSC911x bindings should be updated).

 Sorry, i cannot. The board has lots of jumpers, which choose between fixed voltage and regulators for different components,
according to board's manual, but the manual is very poor IMHO, and i don't understand how to use them. And i use default,
fixed-voltage configuration.
 One of my colleagues tried to get it working, but failed. It actually requires more time, and IIRC you need to bring up i2c before
you can drive regulators.
 So can we leave it as it is for now? At least it works, and this is much better than no ethernet support at all.

> I don't have the board schematics so I couldn't verify the GPIOs.

 Me neither, i wrote GPIO settings according to chip datasheet i have. They are actually chip-specific, but i wrote them in board
file because on different boards you may use different banks, and therefore different pins. Or, if you don't use SROMc at all, you
can configure all pins to do something else.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [PATCH v5 4/4] ARM: dts: Add Ethernet chip to SMDK5410

[Krzysztof Kozlowski]

W dniu 05.11.2015 o 20:14, Pavel Fedin pisze:
>  Hello!
> 
>>> +	ethernet@3 {
>>> +		compatible = "smsc,lan9115";
>>> +		reg = <3 0 0x10000>;
>>> +		phy-mode = "mii";
>>> +		interrupt-parent = <&gpx0>;
>>> +		interrupts = <5 8>;
>>
>> s/8/IRQ_TYPE_LEVEL_LOW/
>> (is this really level low interrupt?)
> 
>  Yes, according to: https://github.com/AndreiLux/Perseus-S3/blob/master/arch/arm/mach-exynos/mach-smdk5250.c#L133

Although this is different board, but okay.

> 
>> Some other DTS include regulators: vddvario-supply and vdd33a-supply. It
>> seems that they are not described in SMSC911x bindings but in
>> GPMC-eth... but the smsc911x driver is requesting them. Could you
>> investigate that? I think these regulators should be provided (and
>> SMSC911x bindings should be updated).
> 
>  Sorry, i cannot. The board has lots of jumpers, which choose between fixed voltage and regulators for different components,
> according to board's manual, but the manual is very poor IMHO, and i don't understand how to use them. And i use default,
> fixed-voltage configuration.
>  One of my colleagues tried to get it working, but failed. It actually requires more time, and IIRC you need to bring up i2c before
> you can drive regulators.
>  So can we leave it as it is for now? At least it works, and this is much better than no ethernet support at all.

Ok, that is sufficient explanation.

>> I don't have the board schematics so I couldn't verify the GPIOs.
> 
>  Me neither, i wrote GPIO settings according to chip datasheet i have. They are actually chip-specific, but i wrote them in board
> file because on different boards you may use different banks, and therefore different pins. Or, if you don't use SROMc at all, you
> can configure all pins to do something else.
> 

Makes sense.

Best regards,
Krzysztof

RE: [PATCH v5 4/4] ARM: dts: Add Ethernet chip to SMDK5410

[Pavel Fedin]

 Hello!

> >>> +	ethernet@3 {
> >>> +		compatible = "smsc,lan9115";
> >>> +		reg = <3 0 0x10000>;
> >>> +		phy-mode = "mii";
> >>> +		interrupt-parent = <&gpx0>;
> >>> +		interrupts = <5 8>;
> >>
> >> s/8/IRQ_TYPE_LEVEL_LOW/
> >> (is this really level low interrupt?)
> >
> >  Yes, according to: https://github.com/AndreiLux/Perseus-S3/blob/master/arch/arm/mach-
> exynos/mach-smdk5250.c#L133
> 
> Although this is different board, but okay.

 I am curious too, so i examined SMSC datasheet. The IRQ is fully programmable on
chip's side too, and our driver indeed defaults to active-low. You can switch it
to active-high by addint smsc,irq-active-high property. But i just copied
settings from those old Android kernels, and it just works.
 So, cleaning up and posting v6.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia