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(+)
--
1.7.9.5
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(+)
diff --git a/arch/arm/mach-mvebu/armada-370-xp.c b/arch/arm/mach-mvebu/armada-370-xp.c
index 2af6ce5..cbad821 100644
--- a/arch/arm/mach-mvebu/armada-370-xp.c
+++ b/arch/arm/mach-mvebu/armada-370-xp.c
@@ -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,
--
1.7.9.5
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(+)
diff --git a/arch/arm/boot/dts/armada-370-db.dts b/arch/arm/boot/dts/armada-370-db.dts
index 4a31b03..2a2aa75 100644
--- a/arch/arm/boot/dts/armada-370-db.dts
+++ b/arch/arm/boot/dts/armada-370-db.dts
@@ -34,5 +34,8 @@
clock-frequency = <200000000>;
status = "okay";
};
+ sata@d00a0000 {
+ status = "okay";
+ };
};
};
diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
index 94b4b9e..3f08233 100644
--- a/arch/arm/boot/dts/armada-370-xp.dtsi
+++ b/arch/arm/boot/dts/armada-370-xp.dtsi
@@ -69,6 +69,16 @@
compatible = "marvell,armada-addr-decoding-controller";
reg = <0xd0020000 0x258>;
};
+
+ sata@d00a0000 {
+ compatible = "marvell,orion-sata";
+ reg = <0xd00a0000 0x2400>;
+ interrupts = <55>;
+ nr-ports = <2>;
+ clocks = <&coreclk 0>;//, <&coreclk 0>;
+ status = "disabled";
+ };
+
};
};
diff --git a/arch/arm/boot/dts/armada-xp-db.dts b/arch/arm/boot/dts/armada-xp-db.dts
index b1fc728..b0db9a3 100644
--- a/arch/arm/boot/dts/armada-xp-db.dts
+++ b/arch/arm/boot/dts/armada-xp-db.dts
@@ -46,5 +46,8 @@
clock-frequency = <250000000>;
status = "okay";
};
+ sata@d00a0000 {
+ status = "okay";
+ };
};
};
diff --git a/arch/arm/configs/mvebu_defconfig b/arch/arm/configs/mvebu_defconfig
index 7bcf850..8ac5f3c 100644
--- a/arch/arm/configs/mvebu_defconfig
+++ b/arch/arm/configs/mvebu_defconfig
@@ -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
--
1.7.9.5
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.
On Wed, 24 Oct 2012 15:49:21 +0200, Gregory CLEMENT wrote:
> diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
> index 94b4b9e..3f08233 100644
> --- a/arch/arm/boot/dts/armada-370-xp.dtsi
> +++ b/arch/arm/boot/dts/armada-370-xp.dtsi
> @@ -69,6 +69,16 @@
> compatible = "marvell,armada-addr-decoding-controller";
> reg = <0xd0020000 0x258>;
> };
> +
> + sata@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
--
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
On 10/24/2012 04:01 PM, Thomas Petazzoni wrote:
> 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.
>
> On Wed, 24 Oct 2012 15:49:21 +0200, Gregory CLEMENT wrote:
>
>> diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
>> index 94b4b9e..3f08233 100644
>> --- a/arch/arm/boot/dts/armada-370-xp.dtsi
>> +++ b/arch/arm/boot/dts/armada-370-xp.dtsi
>> @@ -69,6 +69,16 @@
>> compatible = "marvell,armada-addr-decoding-controller";
>> reg = <0xd0020000 0x258>;
>> };
>> +
>> + sata@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
On Wed, Oct 24, 2012 at 03:49:21PM +0200, Gregory CLEMENT wrote:
> 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(+)
>
> diff --git a/arch/arm/boot/dts/armada-370-db.dts b/arch/arm/boot/dts/armada-370-db.dts
> index 4a31b03..2a2aa75 100644
> --- a/arch/arm/boot/dts/armada-370-db.dts
> +++ b/arch/arm/boot/dts/armada-370-db.dts
> @@ -34,5 +34,8 @@
> clock-frequency = <200000000>;
> status = "okay";
> };
> + sata@d00a0000 {
> + status = "okay";
> + };
> };
> };
> diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
> index 94b4b9e..3f08233 100644
> --- a/arch/arm/boot/dts/armada-370-xp.dtsi
> +++ b/arch/arm/boot/dts/armada-370-xp.dtsi
> @@ -69,6 +69,16 @@
> compatible = "marvell,armada-addr-decoding-controller";
> reg = <0xd0020000 0x258>;
> };
> +
> + sata@d00a0000 {
> + compatible = "marvell,orion-sata";
> + reg = <0xd00a0000 0x2400>;
> + interrupts = <55>;
> + nr-ports = <2>;
> + clocks = <&coreclk 0>;//, <&coreclk 0>;
> + status = "disabled";
> + };
> +
> };
> };
>
> diff --git a/arch/arm/boot/dts/armada-xp-db.dts b/arch/arm/boot/dts/armada-xp-db.dts
> index b1fc728..b0db9a3 100644
> --- a/arch/arm/boot/dts/armada-xp-db.dts
> +++ b/arch/arm/boot/dts/armada-xp-db.dts
> @@ -46,5 +46,8 @@
> clock-frequency = <250000000>;
> status = "okay";
> };
> + sata@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
On 10/24/2012 04:08 PM, Andrew Lunn wrote:
> On Wed, Oct 24, 2012 at 03:49:21PM +0200, Gregory CLEMENT wrote:
>> 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(+)
>>
>> diff --git a/arch/arm/boot/dts/armada-370-db.dts b/arch/arm/boot/dts/armada-370-db.dts
>> index 4a31b03..2a2aa75 100644
>> --- a/arch/arm/boot/dts/armada-370-db.dts
>> +++ b/arch/arm/boot/dts/armada-370-db.dts
>> @@ -34,5 +34,8 @@
>> clock-frequency = <200000000>;
>> status = "okay";
>> };
>> + sata@d00a0000 {
>> + status = "okay";
>> + };
>> };
>> };
>> diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
>> index 94b4b9e..3f08233 100644
>> --- a/arch/arm/boot/dts/armada-370-xp.dtsi
>> +++ b/arch/arm/boot/dts/armada-370-xp.dtsi
>> @@ -69,6 +69,16 @@
>> compatible = "marvell,armada-addr-decoding-controller";
>> reg = <0xd0020000 0x258>;
>> };
>> +
>> + sata@d00a0000 {
>> + compatible = "marvell,orion-sata";
>> + reg = <0xd00a0000 0x2400>;
>> + interrupts = <55>;
>> + nr-ports = <2>;
>> + clocks = <&coreclk 0>;//, <&coreclk 0>;
>> + status = "disabled";
>> + };
>> +
>> };
>> };
>>
>> diff --git a/arch/arm/boot/dts/armada-xp-db.dts b/arch/arm/boot/dts/armada-xp-db.dts
>> index b1fc728..b0db9a3 100644
>> --- a/arch/arm/boot/dts/armada-xp-db.dts
>> +++ b/arch/arm/boot/dts/armada-xp-db.dts
>> @@ -46,5 +46,8 @@
>> clock-frequency = <250000000>;
>> status = "okay";
>> };
>> + sata@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?
Yes you're right we should not depend of the bootloader configuration.
>
> 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.
>
> Thanks
> Andrew
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
Hello,
On 10/24/2012 3:49 PM, Gregory CLEMENT wrote:
> 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]>
> ---
> arch/arm/mach-mvebu/armada-370-xp.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/arch/arm/mach-mvebu/armada-370-xp.c b/arch/arm/mach-mvebu/armada-370-xp.c
> index 2af6ce5..cbad821 100644
> --- a/arch/arm/mach-mvebu/armada-370-xp.c
> +++ b/arch/arm/mach-mvebu/armada-370-xp.c
> @@ -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,
>
Best regards
--
Marek Szyprowski
Samsung Poland R&D Center
On Wednesday 24 October 2012, Gregory CLEMENT wrote:
> 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
Arnd,
On Thu, 25 Oct 2012 11:27:36 +0000, Arnd Bergmann wrote:
> On Wednesday 24 October 2012, Gregory CLEMENT wrote:
> > 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.
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
--
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
On Wed, Oct 24, 2012 at 04:05:45PM +0200, Gregory CLEMENT wrote:
> On 10/24/2012 04:01 PM, Thomas Petazzoni wrote:
> > 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.
> >
> > On Wed, 24 Oct 2012 15:49:21 +0200, Gregory CLEMENT wrote:
> >
> >> diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
> >> index 94b4b9e..3f08233 100644
> >> --- a/arch/arm/boot/dts/armada-370-xp.dtsi
> >> +++ b/arch/arm/boot/dts/armada-370-xp.dtsi
> >> @@ -69,6 +69,16 @@
> >> compatible = "marvell,armada-addr-decoding-controller";
> >> reg = <0xd0020000 0x258>;
> >> };
> >> +
> >> + sata@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?
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.
Jason,
On Thu, 25 Oct 2012 09:18:18 -0400, Jason Cooper wrote:
> > 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
--
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
On 10/25/2012 03:21 PM, Thomas Petazzoni wrote:
> Jason,
>
> On Thu, 25 Oct 2012 09:18:18 -0400, Jason Cooper wrote:
>
>>> 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.
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.
>
> Best regards,
>
> Thomas
>
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
On Thu, Oct 25, 2012 at 03:21:14PM +0200, Thomas Petazzoni wrote:
> Jason,
>
> On Thu, 25 Oct 2012 09:18:18 -0400, Jason Cooper wrote:
>
> > > 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.
Do you have an example of this?
thx,
Jason.
On Thursday 25 October 2012, Thomas Petazzoni wrote:
> On Thu, 25 Oct 2012 11:27:36 +0000, Arnd Bergmann wrote:
> > On Wednesday 24 October 2012, Gregory CLEMENT wrote:
> > > 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.
>
> 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.
Using coherent allocations is fine, I was wondering whether they need
to be atomic or not.
> 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
On 10/25/2012 08:18 AM, Jason Cooper wrote:
> On Wed, Oct 24, 2012 at 04:05:45PM +0200, Gregory CLEMENT wrote:
>> On 10/24/2012 04:01 PM, Thomas Petazzoni wrote:
>>> 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.
>>>
>>> On Wed, 24 Oct 2012 15:49:21 +0200, Gregory CLEMENT wrote:
>>>
>>>> diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
>>>> index 94b4b9e..3f08233 100644
>>>> --- a/arch/arm/boot/dts/armada-370-xp.dtsi
>>>> +++ b/arch/arm/boot/dts/armada-370-xp.dtsi
>>>> @@ -69,6 +69,16 @@
>>>> compatible = "marvell,armada-addr-decoding-controller";
>>>> reg = <0xd0020000 0x258>;
>>>> };
>>>> +
>>>> + sata@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?
>
> 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
On 10/25/2012 08:34 AM, Gregory CLEMENT wrote:
> On 10/25/2012 03:21 PM, Thomas Petazzoni wrote:
>> Jason,
>>
>> On Thu, 25 Oct 2012 09:18:18 -0400, Jason Cooper wrote:
>>
>>>> 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.
>
> 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
On Thu, 25 Oct 2012 13:46:41 +0000, Arnd Bergmann wrote:
> > 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
--
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
On 10/25/2012 03:53 PM, Rob Herring wrote:
> On 10/25/2012 08:18 AM, Jason Cooper wrote:
>> On Wed, Oct 24, 2012 at 04:05:45PM +0200, Gregory CLEMENT wrote:
>>> On 10/24/2012 04:01 PM, Thomas Petazzoni wrote:
>>>> 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.
>>>>
>>>> On Wed, 24 Oct 2012 15:49:21 +0200, Gregory CLEMENT wrote:
>>>>
>>>>> diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
>>>>> index 94b4b9e..3f08233 100644
>>>>> --- a/arch/arm/boot/dts/armada-370-xp.dtsi
>>>>> +++ b/arch/arm/boot/dts/armada-370-xp.dtsi
>>>>> @@ -69,6 +69,16 @@
>>>>> compatible = "marvell,armada-addr-decoding-controller";
>>>>> reg = <0xd0020000 0x258>;
>>>>> };
>>>>> +
>>>>> + sata@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?
>>
>> 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
On 10/25/2012 09:11 AM, Gregory CLEMENT wrote:
> On 10/25/2012 03:53 PM, Rob Herring wrote:
>> On 10/25/2012 08:18 AM, Jason Cooper wrote:
>>> On Wed, Oct 24, 2012 at 04:05:45PM +0200, Gregory CLEMENT wrote:
>>>> On 10/24/2012 04:01 PM, Thomas Petazzoni wrote:
>>>>> 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.
>>>>>
>>>>> On Wed, 24 Oct 2012 15:49:21 +0200, Gregory CLEMENT wrote:
>>>>>
>>>>>> diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
>>>>>> index 94b4b9e..3f08233 100644
>>>>>> --- a/arch/arm/boot/dts/armada-370-xp.dtsi
>>>>>> +++ b/arch/arm/boot/dts/armada-370-xp.dtsi
>>>>>> @@ -69,6 +69,16 @@
>>>>>> compatible = "marvell,armada-addr-decoding-controller";
>>>>>> reg = <0xd0020000 0x258>;
>>>>>> };
>>>>>> +
>>>>>> + sata@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?
>>>
>>> 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.
Okay, missed that discussion.
> And about updated both mvebu_defconfig and multi_v7_defconfig, I'm
> fine with it.
Yes, then please update both.
Rob
On 10/25/2012 03:57 PM, Rob Herring wrote:
> On 10/25/2012 08:34 AM, Gregory CLEMENT wrote:
>> On 10/25/2012 03:21 PM, Thomas Petazzoni wrote:
>>> Jason,
>>>
>>> On Thu, 25 Oct 2012 09:18:18 -0400, Jason Cooper wrote:
>>>
>>>>> 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.
>>
>> 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?
>
> 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.
>
> Rob
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com