Skip to contentSkip to conversation

Adding SATA support for Armada 370/XP

PATCH Superseded Gregory CLEMENT 7 participants 22 messages Oct 24, 2012 – Oct 30, 2012

Subsystems (4)
Device tree bindings Arm (32-bit) Arm sub-architectures Arm/Marvell Kirkwood and Armada 370, 375, 38x, 39x, XP, 3700, 7K/8K, CN9130 SoC support

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Gregory CLEMENT

author
#0 lore raw

cover Adding SATA support for Armada 370/XP

Hello,

this small patch set adds the SATA support for Armada 370 and Armada XP.

The evaluation boards for Armada 370 and Armada XP come with 2 SATA ports, and when both are enable the coherent pool for DMA mapping was too short. It was exactly the same issue that was fixed for Kirkwood two months ago. So I used the same fix in the first patch. Later when Kirkwood will be part of mach-mvebu, then this fix will be shared between the 2 SoCs families.

This patch set is based on 3.7-rc2 and depends one the framework clock support (the last version was posted last week:
http://thread.gmane.org/gmane.linux.kernel/1375701). The git branch called mvebu-SATA-for-3.8 is also available at
https://github.com/MISL-EBU-System-SW/mainline-public.git.

Regards,

Gregory

Gregory CLEMENT (1):
arm: mvebu: increase atomic coherent pool size for armada 370/XP

Lior Amsalem (1):
arm: mvebu: adding SATA support: dt binding and config update

 arch/arm/boot/dts/armada-370-db.dts  |    3 +++
 arch/arm/boot/dts/armada-370-xp.dtsi |   10 ++++++++++
 arch/arm/boot/dts/armada-xp-db.dts   |    3 +++
 arch/arm/configs/mvebu_defconfig     |    7 +++++++
 arch/arm/mach-mvebu/armada-370-xp.c  |   12 ++++++++++++
 5 files changed, 35 insertions(+)
Signature · 1 line
1.7.9.5
author
#1 lore raw

PATCH 1/2 arm: mvebu: increase atomic coherent pool size for armada 370/XP

For Armada 370/XP we have the same problem that for the commit cb01b63, so we applied the same solution: "The default 256 KiB coherent pool may be too small for some of the Kirkwood devices, so increase it to make sure that devices will be able to allocate their buffers with GFP_ATOMIC flag"

Signed-off-by Gregory CLEMENT <[email protected]>
Cc Marek Szyprowski <[email protected]>
---
 arch/arm/mach-mvebu/armada-370-xp.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)
arch/arm/mach-mvebu/armada-370-xp.c +12 -0
index 2af6ce5..cbad821 100644
@@ -17,6 +17,7 @@
 #include <linux/of_platform.h>
 #include <linux/io.h>
 #include <linux/time-armada-370-xp.h>
+#include <linux/dma-mapping.h>
 #include <asm/mach/arch.h>
 #include <asm/mach/map.h>
 #include <asm/mach/time.h>

@@ -43,6 +44,16 @@ void __init armada_370_xp_timer_and_clk_init(void)
 	armada_370_xp_timer_init();
 }
 
+void __init armada_370_xp_init_early(void)
+{
+	/*
+	 * Some Armada 370/XP devices allocate their coherent buffers
+	 * from atomic context. Increase size of atomic coherent pool
+	 * to make sure such the allocations won't fail.
+	 */
+	init_dma_coherent_pool_size(SZ_1M);
+}
+
 struct sys_timer armada_370_xp_timer = {
 	.init		= armada_370_xp_timer_and_clk_init,
 };

@@ -61,6 +72,7 @@ static const char * const armada_370_xp_dt_board_dt_compat[] = {
 DT_MACHINE_START(ARMADA_XP_DT, "Marvell Aramada 370/XP (Device Tree)")
 	.init_machine	= armada_370_xp_dt_init,
 	.map_io		= armada_370_xp_map_io,
+	.init_early	= armada_370_xp_init_early,
 	.init_irq	= armada_370_xp_init_irq,
 	.handle_irq     = armada_370_xp_handle_irq,
 	.timer		= &armada_370_xp_timer,
Signature · 1 line
1.7.9.5
Acked-by on v1 1/2
#2 lore raw

Re: [PATCH 1/2] arm: mvebu: increase atomic coherent pool size for armada 370/XP

Hello,

Quoted 9 lines · Gregory CLEMENT

For Armada 370/XP we have the same problem that for the commit cb01b63, so we applied the same solution: "The default 256 KiB coherent pool may be too small for some of the Kirkwood devices, so increase it to make sure that devices will be able to allocate their buffers with GFP_ATOMIC flag"

Signed-off-by Gregory CLEMENT <[email protected]>
Cc Marek Szyprowski <[email protected]>
Acked-by Marek Szyprowski <[email protected]>
Quoted 35 lines
---
  arch/arm/mach-mvebu/armada-370-xp.c |   12 ++++++++++++
  1 file changed, 12 insertions(+)
arch/arm/mach-mvebu/armada-370-xp.c +11 -0
index 2af6ce5..cbad821 100644
@@ -17,6 +17,7 @@
  #include <linux/of_platform.h>
  #include <linux/io.h>
  #include <linux/time-armada-370-xp.h>
+#include <linux/dma-mapping.h>
  #include <asm/mach/arch.h>
  #include <asm/mach/map.h>
  #include <asm/mach/time.h>

@@ -43,6 +44,16 @@ void __init armada_370_xp_timer_and_clk_init(void)
  	armada_370_xp_timer_init();
  }
 
+void __init armada_370_xp_init_early(void)
+{
+	/*
+	 * Some Armada 370/XP devices allocate their coherent buffers
+	 * from atomic context. Increase size of atomic coherent pool
+	 * to make sure such the allocations won't fail.
+	 */
+	init_dma_coherent_pool_size(SZ_1M);
+}
+
  struct sys_timer armada_370_xp_timer = {
  	.init		= armada_370_xp_timer_and_clk_init,
  };

arch/arm/mach-mvebu/armada-370-xp.c static const char * const armada_370_xp_dt_board_dt_compat[] = { · line 78

Quoted 7 lines
arch/arm/mach-mvebu/armada-370-xp.c +1 -0
@@ -61,6 +72,7 @@ static const char * const armada_370_xp_dt_board_dt_compat[] = {
  DT_MACHINE_START(ARMADA_XP_DT, "Marvell Aramada 370/XP (Device Tree)")
  	.init_machine	= armada_370_xp_dt_init,
  	.map_io		= armada_370_xp_map_io,
+	.init_early	= armada_370_xp_init_early,
  	.init_irq	= armada_370_xp_init_irq,
  	.handle_irq     = armada_370_xp_handle_irq,
  	.timer		= &armada_370_xp_timer,

Best regards

Signature · 2 lines
Marek Szyprowski
Samsung Poland R&D Center
#3 lore raw

Re: [PATCH 1/2] arm: mvebu: increase atomic coherent pool size for armada 370/XP

In reply to Gregory CLEMENT
Quoted 7 lines · Gregory CLEMENT

For Armada 370/XP we have the same problem that for the commit cb01b63, so we applied the same solution: "The default 256 KiB coherent pool may be too small for some of the Kirkwood devices, so increase it to make sure that devices will be able to allocate their buffers with GFP_ATOMIC flag"

Signed-off-by Gregory CLEMENT <[email protected]>

Do you know why the ATA driver needs this? I find it surprising that it's necessary, so I'd like to make sure we're not just working around a device driver bug here.

Arnd

#4 lore raw

Re: [PATCH 1/2] arm: mvebu: increase atomic coherent pool size for armada 370/XP

Arnd,

Quoted 7 lines · Arnd Bergmann
Quoted 6 lines · Gregory CLEMENT

For Armada 370/XP we have the same problem that for the commit cb01b63, so we applied the same solution: "The default 256 KiB coherent pool may be too small for some of the Kirkwood devices, so increase it to make sure that devices will be able to allocate their buffers with GFP_ATOMIC flag"

Quoted 5 lines
Quoted 1 line
Signed-off-by Gregory CLEMENT <[email protected]>

Do you know why the ATA driver needs this? I find it surprising that it's necessary, so I'd like to make sure we're not just working around a device driver bug here.

The sata_mv driver create dma_pool and allocate objects from them, and all the memory allocated for dma_pools is allocated using dma_alloc_coherent(), and I guess the driver is using too much of them.

Seems like the driver is too lazy and allocates everything coherent to avoid the hassle of doing dma_map/dma_unmap operations when needed, but I haven't looked in details at the driver yet to see if it would be possible to switch those DMA coherent allocations into non-coherent allocations + appropriate calls to the DMA operations.

That said, that's for sure a larger task than just enabling SATA on Armada 370/XP, so I would advocate to handle this problem separately.

Best regards,

Thomas

Signature · 4 lines
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
#5 lore raw

Re: [PATCH 1/2] arm: mvebu: increase atomic coherent pool size for armada 370/XP

Quoted 18 lines · Thomas Petazzoni
Quoted 7 lines · Arnd Bergmann
Quoted 6 lines · Gregory CLEMENT

For Armada 370/XP we have the same problem that for the commit cb01b63, so we applied the same solution: "The default 256 KiB coherent pool may be too small for some of the Kirkwood devices, so increase it to make sure that devices will be able to allocate their buffers with GFP_ATOMIC flag"

Quoted 5 lines
Quoted 1 line
Signed-off-by Gregory CLEMENT <[email protected]>

Do you know why the ATA driver needs this? I find it surprising that it's necessary, so I'd like to make sure we're not just working around a device driver bug here.

The sata_mv driver create dma_pool and allocate objects from them, and all the memory allocated for dma_pools is allocated using dma_alloc_coherent(), and I guess the driver is using too much of them.

Quoted 5 lines

Seems like the driver is too lazy and allocates everything coherent to avoid the hassle of doing dma_map/dma_unmap operations when needed, but I haven't looked in details at the driver yet to see if it would be possible to switch those DMA coherent allocations into non-coherent allocations + appropriate calls to the DMA operations.

Using coherent allocations is fine, I was wondering whether they need to be atomic or not.

Quoted 2 lines

That said, that's for sure a larger task than just enabling SATA on Armada 370/XP, so I would advocate to handle this problem separately.

Agreed.

Arnd

#6 lore raw

Re: [PATCH 1/2] arm: mvebu: increase atomic coherent pool size for armada 370/XP

Quoted 8 lines · Arnd Bergmann
Quoted 5 lines

Seems like the driver is too lazy and allocates everything coherent to avoid the hassle of doing dma_map/dma_unmap operations when needed, but I haven't looked in details at the driver yet to see if it would be possible to switch those DMA coherent allocations into non-coherent allocations + appropriate calls to the DMA operations.

Using coherent allocations is fine, I was wondering whether they need to be atomic or not.

You're raising a good point here: all the dma_pool_alloc() allocations done by sata_mv are GFP_KERNEL. So why are we having problems with the /atomic/ coherent pool size? Is it the libata core that's doing GFP_ATOMIC DMA coherent allocations. It doesn't seem so. Something's odd.

Thomas

Signature · 4 lines
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
#7 lore raw

PATCH 1/2 arm: mvebu: increase atomic coherent pool size for armada 370/XP

In reply to Arnd Bergmann

Hello,

Quoted 19 lines · Arnd Bergmann
Quoted 18 lines · Thomas Petazzoni
Quoted 7 lines · Arnd Bergmann
Quoted 6 lines · Gregory CLEMENT

For Armada 370/XP we have the same problem that for the commit cb01b63, so we applied the same solution: "The default 256 KiB coherent pool may be too small for some of the Kirkwood devices, so increase it to make sure that devices will be able to allocate their buffers with GFP_ATOMIC flag"

Quoted 5 lines
Quoted 1 line
Signed-off-by Gregory CLEMENT <[email protected]>

Do you know why the ATA driver needs this? I find it surprising that it's necessary, so I'd like to make sure we're not just working around a device driver bug here.

The sata_mv driver create dma_pool and allocate objects from them, and all the memory allocated for dma_pools is allocated using dma_alloc_coherent(), and I guess the driver is using too much of them.

Quoted 8 lines
Quoted 5 lines

Seems like the driver is too lazy and allocates everything coherent to avoid the hassle of doing dma_map/dma_unmap operations when needed, but I haven't looked in details at the driver yet to see if it would be possible to switch those DMA coherent allocations into non-coherent allocations + appropriate calls to the DMA operations.

Using coherent allocations is fine, I was wondering whether they need to be atomic or not.

I've checked the code and it turned out that ALL allocations from dma_pool are made with GFP_ATOMIC flag. GFP flags passed from caller are simply ignored, see mm/dmapool.c dma_pool_alloc() function.

This looks like an ugly hack or workaround from ancient times, because dma_pool_alloc() tries to implement __GFP_WAIT style approach on it's own. I've tried to find the reason for such approach, but it looks that this code comes from pre-git times. IMHO this strange code inside dma_pool_alloc() should be replaced with simple call to pool_alloc_page(pool, mem_flags) and let dma-mapping and core memory subsystems to handle gfp flags correctly.

Arnd: do you have any opinion on this? I'm aware that this change might have some side effects on drivers which don't pass correct gfp flags, but using the emergency memory pools all the time for all dma_pool allocations really doesn't make much sense.

Best regards

Signature · 2 lines
Marek Szyprowski
Samsung Poland R&D Center
author
#8 lore raw

PATCH 2/2 arm: mvebu: adding SATA support: dt binding and config update

In reply to Gregory CLEMENT

From: Lior Amsalem <[email protected]>

Signed-off-by Gregory CLEMENT <[email protected]>
Signed-off-by Lior Amsalem <[email protected]>
---
 arch/arm/boot/dts/armada-370-db.dts  |    3 +++
 arch/arm/boot/dts/armada-370-xp.dtsi |   10 ++++++++++
 arch/arm/boot/dts/armada-xp-db.dts   |    3 +++
 arch/arm/configs/mvebu_defconfig     |    7 +++++++
 4 files changed, 23 insertions(+)
arch/arm/boot/dts/armada-370-db.dts +3 -0
index 4a31b03..2a2aa75 100644
@@ -34,5 +34,8 @@
 			clock-frequency = <200000000>;
 			status = "okay";
 		};
+		sata at d00a0000 {
+			status = "okay";
+		};
 	};
 };
arch/arm/boot/dts/armada-370-xp.dtsi +10 -0
index 94b4b9e..3f08233 100644
@@ -69,6 +69,16 @@
 			compatible = "marvell,armada-addr-decoding-controller";
 			reg = <0xd0020000 0x258>;
 		};
+
+		sata at d00a0000 {
+                        compatible = "marvell,orion-sata";
+                        reg = <0xd00a0000 0x2400>;
+                        interrupts = <55>;
+                        nr-ports = <2>;
+			clocks = <&coreclk 0>;//,  <&coreclk 0>;
+                        status = "disabled";
+		};
+
 	};
 };
arch/arm/boot/dts/armada-xp-db.dts +3 -0
index b1fc728..b0db9a3 100644
@@ -46,5 +46,8 @@
 			clock-frequency = <250000000>;
 			status = "okay";
 		};
+		sata at d00a0000 {
+			status = "okay";
+		};
 	};
 };
arch/arm/configs/mvebu_defconfig +7 -0
index 7bcf850..8ac5f3c 100644
@@ -18,6 +18,13 @@ CONFIG_ZBOOT_ROM_BSS=0x0
 CONFIG_ARM_APPENDED_DTB=y
 CONFIG_VFP=y
 CONFIG_UEVENT_HELPER_PATH="/sbin/hotplug"
+CONFIG_BLK_DEV_SD=y
+CONFIG_ATA=y
+CONFIG_SATA_MV=y
+CONFIG_MD=y
+CONFIG_BLK_DEV_MD=y
+# CONFIG_MD_AUTODETECT is not set
+CONFIG_MD_RAID456=y
 CONFIG_SERIAL_8250=y
 CONFIG_SERIAL_8250_CONSOLE=y
 CONFIG_SERIAL_OF_PLATFORM=y
Signature · 1 line
1.7.9.5
#9 lore raw

Re: [PATCH 2/2] arm: mvebu: adding SATA support: dt binding and config update

Hello,

Shouldn't you split into one commit adding the SATA definition in the .dtsi + doing the defconfig change (the "SoC" level modifications), and then another commit for the .dts change? I don't really care personally, it's really up to Jason/Andrew on this.

Another comment below, though.

Quoted 5 lines · Gregory CLEMENT
arch/arm/boot/dts/armada-370-xp.dtsi +0 -0
index 94b4b9e..3f08233 100644

arch/arm/boot/dts/armada-370-xp.dtsi line 78

Quoted 10 lines
arch/arm/boot/dts/armada-370-xp.dtsi +7 -0
@@ -69,6 +69,16 @@
 			compatible = "marvell,armada-addr-decoding-controller";
 			reg = <0xd0020000 0x258>;
 		};
+
+		sata at d00a0000 {
+                        compatible = "marvell,orion-sata";
+                        reg = <0xd00a0000 0x2400>;
+                        interrupts = <55>;
+                        nr-ports = <2>;
+			clocks = <&coreclk 0>;//,  <&coreclk 0>;

Alignment problem + remainings of tests or something like that.

Thomas

Signature · 4 lines
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
author
#10 lore raw

Re: [PATCH 2/2] arm: mvebu: adding SATA support: dt binding and config update

Quoted 20 lines · Thomas Petazzoni

Hello,

Shouldn't you split into one commit adding the SATA definition in the .dtsi + doing the defconfig change (the "SoC" level modifications), and then another commit for the .dts change? I don't really care personally, it's really up to Jason/Andrew on this.

Another comment below, though.

Quoted 9 lines · Gregory CLEMENT
arch/arm/boot/dts/armada-370-xp.dtsi +1 -0
index 94b4b9e..3f08233 100644
@@ -69,6 +69,16 @@
 			compatible = "marvell,armada-addr-decoding-controller";
 			reg = <0xd0020000 0x258>;
 		};
+
Quoted 8 lines

arch/arm/boot/dts/armada-370-xp.dtsi line 78

Quoted 6 lines
arch/arm/boot/dts/armada-370-xp.dtsi +6 -0
+		sata at d00a0000 {
+                        compatible = "marvell,orion-sata";
+                        reg = <0xd00a0000 0x2400>;
+                        interrupts = <55>;
+                        nr-ports = <2>;
+			clocks = <&coreclk 0>;//,  <&coreclk 0>;

Alignment problem + remainings of tests or something like that.

True I missed this one.

Jason, Andrew, do you want I split this patch as suggested by Thomas or are you fine with having one single patch?

I will wait your answer before sending the V2.

Thanks,

Gregory

#11 lore raw

Re: [PATCH 2/2] arm: mvebu: adding SATA support: dt binding and config update

Quoted 30 lines · Gregory CLEMENT
Quoted 20 lines · Thomas Petazzoni

Hello,

Shouldn't you split into one commit adding the SATA definition in the .dtsi + doing the defconfig change (the "SoC" level modifications), and then another commit for the .dts change? I don't really care personally, it's really up to Jason/Andrew on this.

Another comment below, though.

Quoted 9 lines · Gregory CLEMENT
arch/arm/boot/dts/armada-370-xp.dtsi +1 -0
index 94b4b9e..3f08233 100644
@@ -69,6 +69,16 @@
 			compatible = "marvell,armada-addr-decoding-controller";
 			reg = <0xd0020000 0x258>;
 		};
+
Quoted 8 lines

arch/arm/boot/dts/armada-370-xp.dtsi line 78

Quoted 6 lines
arch/arm/boot/dts/armada-370-xp.dtsi +6 -0
+		sata at d00a0000 {
+                        compatible = "marvell,orion-sata";
+                        reg = <0xd00a0000 0x2400>;
+                        interrupts = <55>;
+                        nr-ports = <2>;
+			clocks = <&coreclk 0>;//,  <&coreclk 0>;

Alignment problem + remainings of tests or something like that.

Quoted 4 lines

True I missed this one.

Jason, Andrew, do you want I split this patch as suggested by Thomas or are you fine with having one single patch?

Yes, please make the defconfig changes a separate patch. Also, please make sure only the minimum is enabled (eq RAID... isn't needed).

thx,

Jason.

#12 lore raw

Re: [PATCH 2/2] arm: mvebu: adding SATA support: dt binding and config update

Jason,

Quoted 5 lines · Jason Cooper
Quoted 2 lines

Jason, Andrew, do you want I split this patch as suggested by Thomas or are you fine with having one single patch?

Yes, please make the defconfig changes a separate patch. Also, please make sure only the minimum is enabled (eq RAID... isn't needed).

I haven't looked in details at the driver, but is nr-ports = <foo> the right way of doing things? We may have platforms were port 0 is not used, but port 1 is used, and just a number of ports doesn't allow to express this.

Shouldn't the DT property be

ports = <0>, <1>
ports = <1>
ports = <1>, <3>

In order to allow to more precisely enabled SATA ports? Or maybe the SATA ports cannot be enabled/disabled on a per-port basis, in which case I'm obviously wrong here.

Best regards,

Thomas

Signature · 4 lines
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
author
#13 lore raw

Re: [PATCH 2/2] arm: mvebu: adding SATA support: dt binding and config update

Quoted 17 lines · Thomas Petazzoni

Jason,

Quoted 5 lines · Jason Cooper
Quoted 2 lines

Jason, Andrew, do you want I split this patch as suggested by Thomas or are you fine with having one single patch?

Yes, please make the defconfig changes a separate patch. Also, please make sure only the minimum is enabled (eq RAID... isn't needed).

I haven't looked in details at the driver, but is nr-ports = <foo> the right way of doing things? We may have platforms were port 0 is not used, but port 1 is used, and just a number of ports doesn't allow to express this.

Shouldn't the DT property be

Quoted 7 lines

ports = <0>, <1>
ports = <1>
ports = <1>, <3>

In order to allow to more precisely enabled SATA ports? Or maybe the SATA ports cannot be enabled/disabled on a per-port basis, in which case I'm obviously wrong here.

The actual implementation of mv_sata.c doesn't work like this. You can only pass the number of ports supported not the list of the port you want to support. I've checked in the device tree binding documentation _and_ also in the code.

Quoted 5 lines

Best regards,

Thomas

Signature · 4 lines
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
#14 lore raw

Re: [PATCH 2/2] arm: mvebu: adding SATA support: dt binding and config update

Quoted 22 lines · Gregory CLEMENT
Quoted 21 lines · Thomas Petazzoni

Jason,

Quoted 5 lines · Jason Cooper
Quoted 2 lines

Jason, Andrew, do you want I split this patch as suggested by Thomas or are you fine with having one single patch?

Yes, please make the defconfig changes a separate patch. Also, please make sure only the minimum is enabled (eq RAID... isn't needed).

I haven't looked in details at the driver, but is nr-ports = <foo> the right way of doing things? We may have platforms were port 0 is not used, but port 1 is used, and just a number of ports doesn't allow to express this.

Shouldn't the DT property be

ports = <0>, <1>
ports = <1>
ports = <1>, <3>

Quoted 8 lines
Quoted 3 lines

In order to allow to more precisely enabled SATA ports? Or maybe the SATA ports cannot be enabled/disabled on a per-port basis, in which case I'm obviously wrong here.

The actual implementation of mv_sata.c doesn't work like this. You can only pass the number of ports supported not the list of the port you want to support. I've checked in the device tree binding documentation _and_ also in the code.

Is that a statement about the driver or the h/w? It does not matter what the driver does. If the h/w can support skipping a port, then the dts should allow that.

A bitmask would be most appropriate here (and matches how AHCI does the equivalent).

Rob

author
#15 lore raw

Re: [PATCH 2/2] arm: mvebu: adding SATA support: dt binding and config update

Quoted 27 lines · Rob Herring
Quoted 26 lines · Gregory CLEMENT
Quoted 24 lines · Thomas Petazzoni

Jason,

Quoted 5 lines · Jason Cooper
Quoted 2 lines

Jason, Andrew, do you want I split this patch as suggested by Thomas or are you fine with having one single patch?

Yes, please make the defconfig changes a separate patch. Also, please make sure only the minimum is enabled (eq RAID... isn't needed).

I haven't looked in details at the driver, but is nr-ports = <foo> the right way of doing things? We may have platforms were port 0 is not used, but port 1 is used, and just a number of ports doesn't allow to express this.

Shouldn't the DT property be

ports = <0>, <1>
ports = <1>
ports = <1>, <3>

In order to allow to more precisely enabled SATA ports? Or maybe the SATA ports cannot be enabled/disabled on a per-port basis, in which case I'm obviously wrong here.

Quoted 8 lines
Quoted 4 lines

The actual implementation of mv_sata.c doesn't work like this. You can only pass the number of ports supported not the list of the port you want to support. I've checked in the device tree binding documentation _and_ also in the code.

Is that a statement about the driver or the h/w? It does not matter what the driver does. If the h/w can support skipping a port, then the dts should allow that.

Indeed I didn't see anything in the datasheet which would prevent to use only port 2. I agree to add this feature if needed, but currently there is no Marvell based board whith this kind of configuration. Can we keep this series as a simple series to enable SATA on Armada 370/XP, and prepare an other series for this new feature?

Quoted 3 lines

A bitmask would be most appropriate here (and matches how AHCI does the equivalent).

I didn't see any bitmask for AHCI, just an optional list of phandle on combophy.

Quoted 9 lines

Rob

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Signature · 4 lines
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
#16 lore raw

Re: [PATCH 2/2] arm: mvebu: adding SATA support: dt binding and config update

In reply to Thomas Petazzoni
Quoted 7 lines · Thomas Petazzoni

Jason,

Quoted 3 lines · Jason Cooper
Quoted 2 lines

Jason, Andrew, do you want I split this patch as suggested by Thomas or are you fine with having one single patch?

Quoted 7 lines
Quoted 2 lines

Yes, please make the defconfig changes a separate patch. Also, please make sure only the minimum is enabled (eq RAID... isn't needed).

I haven't looked in details at the driver, but is nr-ports = <foo> the right way of doing things? We may have platforms were port 0 is not used, but port 1 is used, and just a number of ports doesn't allow to express this.

Do you have an example of this?

thx,

Jason.

#17 lore raw

Re: [PATCH 2/2] arm: mvebu: adding SATA support: dt binding and config update

In reply to Jason Cooper
Quoted 31 lines · Jason Cooper
Quoted 30 lines · Gregory CLEMENT
Quoted 28 lines · Thomas Petazzoni

Hello,

Shouldn't you split into one commit adding the SATA definition in the .dtsi + doing the defconfig change (the "SoC" level modifications), and then another commit for the .dts change? I don't really care personally, it's really up to Jason/Andrew on this.

Another comment below, though.

Quoted 5 lines · Gregory CLEMENT
arch/arm/boot/dts/armada-370-xp.dtsi +0 -0
index 94b4b9e..3f08233 100644

arch/arm/boot/dts/armada-370-xp.dtsi line 78

Quoted 10 lines
arch/arm/boot/dts/armada-370-xp.dtsi +7 -0
@@ -69,6 +69,16 @@
 			compatible = "marvell,armada-addr-decoding-controller";
 			reg = <0xd0020000 0x258>;
 		};
+
+		sata at d00a0000 {
+                        compatible = "marvell,orion-sata";
+                        reg = <0xd00a0000 0x2400>;
+                        interrupts = <55>;
+                        nr-ports = <2>;
+			clocks = <&coreclk 0>;//,  <&coreclk 0>;

Alignment problem + remainings of tests or something like that.

Quoted 7 lines
Quoted 4 lines

True I missed this one.

Jason, Andrew, do you want I split this patch as suggested by Thomas or are you fine with having one single patch?

Yes, please make the defconfig changes a separate patch. Also, please make sure only the minimum is enabled (eq RAID... isn't needed).

What about updating multi_v7_defconfig instead?

Rob

author
#18 lore raw

Re: [PATCH 2/2] arm: mvebu: adding SATA support: dt binding and config update

Quoted 34 lines · Rob Herring
Quoted 33 lines · Jason Cooper
Quoted 32 lines · Gregory CLEMENT
Quoted 20 lines · Thomas Petazzoni

Hello,

Shouldn't you split into one commit adding the SATA definition in the .dtsi + doing the defconfig change (the "SoC" level modifications), and then another commit for the .dts change? I don't really care personally, it's really up to Jason/Andrew on this.

Another comment below, though.

Quoted 9 lines · Gregory CLEMENT
arch/arm/boot/dts/armada-370-xp.dtsi +1 -0
index 94b4b9e..3f08233 100644
@@ -69,6 +69,16 @@
 			compatible = "marvell,armada-addr-decoding-controller";
 			reg = <0xd0020000 0x258>;
 		};
+
Quoted 8 lines

arch/arm/boot/dts/armada-370-xp.dtsi line 78

Quoted 6 lines
arch/arm/boot/dts/armada-370-xp.dtsi +6 -0
+		sata at d00a0000 {
+                        compatible = "marvell,orion-sata";
+                        reg = <0xd00a0000 0x2400>;
+                        interrupts = <55>;
+                        nr-ports = <2>;
+			clocks = <&coreclk 0>;//,  <&coreclk 0>;

Alignment problem + remainings of tests or something like that.

True I missed this one.

Quoted 7 lines
Quoted 5 lines
Quoted 2 lines

Jason, Andrew, do you want I split this patch as suggested by Thomas or are you fine with having one single patch?

Yes, please make the defconfig changes a separate patch. Also, please make sure only the minimum is enabled (eq RAID... isn't needed).

What about updating multi_v7_defconfig instead?

About the _instead_, when I proposed to removed mvebu_defconfig Thomas argued that it was more convenient to build a mvebu-only kernel when doing kernel development, and Andrew also pointed that this defconfig should be useful for kisskb.

And about updated both mvebu_defconfig and multi_v7_defconfig, I'm fine with it.

Gregory

#19 lore raw

Re: [PATCH 2/2] arm: mvebu: adding SATA support: dt binding and config update

Quoted 41 lines · Gregory CLEMENT
Quoted 40 lines · Rob Herring
Quoted 38 lines · Jason Cooper
Quoted 30 lines · Gregory CLEMENT
Quoted 20 lines · Thomas Petazzoni

Hello,

Shouldn't you split into one commit adding the SATA definition in the .dtsi + doing the defconfig change (the "SoC" level modifications), and then another commit for the .dts change? I don't really care personally, it's really up to Jason/Andrew on this.

Another comment below, though.

Quoted 9 lines · Gregory CLEMENT
arch/arm/boot/dts/armada-370-xp.dtsi +1 -0
index 94b4b9e..3f08233 100644
@@ -69,6 +69,16 @@
 			compatible = "marvell,armada-addr-decoding-controller";
 			reg = <0xd0020000 0x258>;
 		};
+
Quoted 8 lines

arch/arm/boot/dts/armada-370-xp.dtsi line 78

Quoted 6 lines
arch/arm/boot/dts/armada-370-xp.dtsi +6 -0
+		sata at d00a0000 {
+                        compatible = "marvell,orion-sata";
+                        reg = <0xd00a0000 0x2400>;
+                        interrupts = <55>;
+                        nr-ports = <2>;
+			clocks = <&coreclk 0>;//,  <&coreclk 0>;

Alignment problem + remainings of tests or something like that.

Quoted 4 lines

True I missed this one.

Jason, Andrew, do you want I split this patch as suggested by Thomas or are you fine with having one single patch?

Yes, please make the defconfig changes a separate patch. Also, please make sure only the minimum is enabled (eq RAID... isn't needed).

Quoted 6 lines
Quoted 1 line

What about updating multi_v7_defconfig instead?

About the _instead_, when I proposed to removed mvebu_defconfig Thomas argued that it was more convenient to build a mvebu-only kernel when doing kernel development, and Andrew also pointed that this defconfig should be useful for kisskb.

Okay, missed that discussion.

Quoted 2 lines

And about updated both mvebu_defconfig and multi_v7_defconfig, I'm fine with it.

Yes, then please update both.

Rob

maintainer
#20 lore raw

Re: [PATCH 2/2] arm: mvebu: adding SATA support: dt binding and config update

In reply to Gregory CLEMENT
Quoted 50 lines · Gregory CLEMENT

From: Lior Amsalem <[email protected]>

Signed-off-by Gregory CLEMENT <[email protected]>
Signed-off-by Lior Amsalem <[email protected]>
---
 arch/arm/boot/dts/armada-370-db.dts  |    3 +++
 arch/arm/boot/dts/armada-370-xp.dtsi |   10 ++++++++++
 arch/arm/boot/dts/armada-xp-db.dts   |    3 +++
 arch/arm/configs/mvebu_defconfig     |    7 +++++++
 4 files changed, 23 insertions(+)
arch/arm/boot/dts/armada-370-db.dts +3 -0
index 4a31b03..2a2aa75 100644
@@ -34,5 +34,8 @@
 			clock-frequency = <200000000>;
 			status = "okay";
 		};
+		sata at d00a0000 {
+			status = "okay";
+		};
 	};
 };
arch/arm/boot/dts/armada-370-xp.dtsi +10 -0
index 94b4b9e..3f08233 100644
@@ -69,6 +69,16 @@
 			compatible = "marvell,armada-addr-decoding-controller";
 			reg = <0xd0020000 0x258>;
 		};
+
+		sata at d00a0000 {
+                        compatible = "marvell,orion-sata";
+                        reg = <0xd00a0000 0x2400>;
+                        interrupts = <55>;
+                        nr-ports = <2>;
+			clocks = <&coreclk 0>;//,  <&coreclk 0>;
+                        status = "disabled";
+		};
+
 	};
 };
arch/arm/boot/dts/armada-xp-db.dts +0 -0
index b1fc728..b0db9a3 100644

arch/arm/boot/dts/armada-xp-db.dts line 53

Quoted 8 lines
arch/arm/boot/dts/armada-xp-db.dts +3 -0
@@ -46,5 +46,8 @@
 			clock-frequency = <250000000>;
 			status = "okay";
 		};
+		sata at d00a0000 {
+			status = "okay";
+		};
 	};
 };

Hi Gregory

Should there be some pinctrl setup somewhere, to ensure the pins used for SATA are really setup up for SATA?

Also, for kirkwood, the number of SATA ports varies. So we don't define it in the base kirkwood.dtsi and leave each board to set it. Do we want to be consistent between kirkwood and 370/xp?

Thanks
Andrew

author
#21 lore raw

Re: [PATCH 2/2] arm: mvebu: adding SATA support: dt binding and config update

Quoted 62 lines · Andrew Lunn
Quoted 50 lines · Gregory CLEMENT

From: Lior Amsalem <[email protected]>

Signed-off-by Gregory CLEMENT <[email protected]>
Signed-off-by Lior Amsalem <[email protected]>
---
 arch/arm/boot/dts/armada-370-db.dts  |    3 +++
 arch/arm/boot/dts/armada-370-xp.dtsi |   10 ++++++++++
 arch/arm/boot/dts/armada-xp-db.dts   |    3 +++
 arch/arm/configs/mvebu_defconfig     |    7 +++++++
 4 files changed, 23 insertions(+)
arch/arm/boot/dts/armada-370-db.dts +3 -0
index 4a31b03..2a2aa75 100644
@@ -34,5 +34,8 @@
 			clock-frequency = <200000000>;
 			status = "okay";
 		};
+		sata at d00a0000 {
+			status = "okay";
+		};
 	};
 };
arch/arm/boot/dts/armada-370-xp.dtsi +10 -0
index 94b4b9e..3f08233 100644
@@ -69,6 +69,16 @@
 			compatible = "marvell,armada-addr-decoding-controller";
 			reg = <0xd0020000 0x258>;
 		};
+
+		sata at d00a0000 {
+                        compatible = "marvell,orion-sata";
+                        reg = <0xd00a0000 0x2400>;
+                        interrupts = <55>;
+                        nr-ports = <2>;
+			clocks = <&coreclk 0>;//,  <&coreclk 0>;
+                        status = "disabled";
+		};
+
 	};
 };
arch/arm/boot/dts/armada-xp-db.dts +0 -0
index b1fc728..b0db9a3 100644

arch/arm/boot/dts/armada-xp-db.dts line 53

Quoted 8 lines
arch/arm/boot/dts/armada-xp-db.dts +3 -0
@@ -46,5 +46,8 @@
 			clock-frequency = <250000000>;
 			status = "okay";
 		};
+		sata at d00a0000 {
+			status = "okay";
+		};
 	};
 };

Hi Gregory

Quoted 2 lines

Should there be some pinctrl setup somewhere, to ensure the pins used for SATA are really setup up for SATA?

Yes you're right we should not depend of the bootloader configuration.

Quoted 4 lines

Also, for kirkwood, the number of SATA ports varies. So we don't define it in the base kirkwood.dtsi and leave each board to set it. Do we want to be consistent between kirkwood and 370/xp?

Yes sure. I will move it from dtsi to dts.

Quoted 9 lines

Thanks
Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Signature · 4 lines
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com