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 <redacted>
Cc: Marek Szyprowski <m.szyprowski@samsung.com>
---
arch/arm/mach-mvebu/armada-370-xp.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
From: Thomas Petazzoni <hidden> Date: 2012-10-24 14:01:54
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:
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
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:
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
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
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?
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
From: Marek Szyprowski <m.szyprowski@samsung.com> Date: 2012-10-25 05:29:50
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 <redacted>
Cc: Marek Szyprowski <m.szyprowski@samsung.com>
Acked-by: Marek Szyprowski <m.szyprowski@samsung.com>
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 <redacted>
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
From: Thomas Petazzoni <hidden> Date: 2012-10-25 11:44:06
Arnd,
On Thu, 25 Oct 2012 11:27:36 +0000, Arnd Bergmann wrote:
On Wednesday 24 October 2012, Gregory CLEMENT wrote:
quoted
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 <redacted>
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
From: Jason Cooper <hidden> Date: 2012-10-25 13:18:51
On Wed, Oct 24, 2012 at 04:05:45PM +0200, Gregory CLEMENT wrote:
On 10/24/2012 04:01 PM, Thomas Petazzoni wrote:
quoted
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:
From: Thomas Petazzoni <hidden> Date: 2012-10-25 13:21:30
Jason,
On Thu, 25 Oct 2012 09:18:18 -0400, Jason Cooper wrote:
quoted
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
Jason,
On Thu, 25 Oct 2012 09:18:18 -0400, Jason Cooper wrote:
quoted
quoted
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
From: Jason Cooper <hidden> Date: 2012-10-25 13:36:22
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:
quoted
quoted
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.
On Thursday 25 October 2012, Thomas Petazzoni wrote:
On Thu, 25 Oct 2012 11:27:36 +0000, Arnd Bergmann wrote:
quoted
On Wednesday 24 October 2012, Gregory CLEMENT wrote:
quoted
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 <redacted>
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.
From: Rob Herring <hidden> Date: 2012-10-25 13:54:06
On 10/25/2012 08:18 AM, Jason Cooper wrote:
On Wed, Oct 24, 2012 at 04:05:45PM +0200, Gregory CLEMENT wrote:
quoted
On 10/24/2012 04:01 PM, Thomas Petazzoni wrote:
quoted
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:
From: Rob Herring <hidden> Date: 2012-10-25 13:57:24
On 10/25/2012 08:34 AM, Gregory CLEMENT wrote:
On 10/25/2012 03:21 PM, Thomas Petazzoni wrote:
quoted
Jason,
On Thu, 25 Oct 2012 09:18:18 -0400, Jason Cooper wrote:
quoted
quoted
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
From: Thomas Petazzoni <hidden> Date: 2012-10-25 14:09:42
On Thu, 25 Oct 2012 13:46:41 +0000, Arnd Bergmann wrote:
quoted
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 Wed, Oct 24, 2012 at 04:05:45PM +0200, Gregory CLEMENT wrote:
quoted
On 10/24/2012 04:01 PM, Thomas Petazzoni wrote:
quoted
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:
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
From: Rob Herring <hidden> Date: 2012-10-25 14:27:19
On 10/25/2012 09:11 AM, Gregory CLEMENT wrote:
On 10/25/2012 03:53 PM, Rob Herring wrote:
quoted
On 10/25/2012 08:18 AM, Jason Cooper wrote:
quoted
On Wed, Oct 24, 2012 at 04:05:45PM +0200, Gregory CLEMENT wrote:
quoted
On 10/24/2012 04:01 PM, Thomas Petazzoni wrote:
quoted
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:
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.
Jason,
On Thu, 25 Oct 2012 09:18:18 -0400, Jason Cooper wrote:
quoted
quoted
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.
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
Hello,
On 10/25/2012 3:46 PM, Arnd Bergmann wrote:
On Thursday 25 October 2012, Thomas Petazzoni wrote:
quoted
On Thu, 25 Oct 2012 11:27:36 +0000, Arnd Bergmann wrote:
quoted
On Wednesday 24 October 2012, Gregory CLEMENT wrote:
quoted
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 <redacted>
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.
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
--
Marek Szyprowski
Samsung Poland R&D Center