[PATCH] arm: mvebu: Add SPI flash on Armada XP-GP board

[PATCH] arm: mvebu: Add SPI flash on Armada XP-GP board

[Ezequiel Garcia]

This patch adds an SPI master device node for Armada XP-GP board.
This master node is an SPI flash controller 'n25q128a13'.

Since there is no 'partitions' node declared, one full sized
partition named as the device will be created.

Cc: Gregory Clement <gregory.clement@free-electrons.com>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Lior Amsalem <alior@marvell.com>
Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
This patch depends on:

 1. Gregory's patch for Armada XP GP board:
      arm: mvebu: support for the new Armada XP development board(DB-MV784MP-GP)

 2. My previous patch for SPI on Armada 370/XP:
      arm: mvebu: Add support for SPI controller in Armada 370/XP

And don't forget to compile the SPI flash driver, CONFIG_MTD_M25P80=y

 arch/arm/boot/dts/armada-xp-gp.dts |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/armada-xp-gp.dts b/arch/arm/boot/dts/armada-xp-gp.dts
index 3eea531..1c8afe2 100644
--- a/arch/arm/boot/dts/armada-xp-gp.dts
+++ b/arch/arm/boot/dts/armada-xp-gp.dts
@@ -97,5 +97,17 @@
 			phy = <&phy3>;
 			phy-mode = "rgmii-id";
 		};
+
+		spi0: spi at d0010600 {
+			status = "okay";
+
+			spi-flash at 0 {
+				#address-cells = <1>;
+				#size-cells = <1>;
+				compatible = "n25q128a13";
+				reg = <0>; /* Chip select 0 */
+				spi-max-frequency = <108000000>;
+			};
+		};
 	};
 };
-- 
1.7.8.6

[PATCH] arm: mvebu: Add SPI flash on Armada XP-GP board

[Jason Cooper]

On Tue, Feb 05, 2013 at 08:24:33AM -0300, Ezequiel Garcia wrote:
> This patch adds an SPI master device node for Armada XP-GP board.
> This master node is an SPI flash controller 'n25q128a13'.
> 
> Since there is no 'partitions' node declared, one full sized
> partition named as the device will be created.
> 
> Cc: Gregory Clement <gregory.clement@free-electrons.com>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Lior Amsalem <alior@marvell.com>
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> ---
> This patch depends on:
> 
>  1. Gregory's patch for Armada XP GP board:
>       arm: mvebu: support for the new Armada XP development board(DB-MV784MP-GP)
> 
>  2. My previous patch for SPI on Armada 370/XP:
>       arm: mvebu: Add support for SPI controller in Armada 370/XP
> 
> And don't forget to compile the SPI flash driver, CONFIG_MTD_M25P80=y
> 
>  arch/arm/boot/dts/armada-xp-gp.dts |   12 ++++++++++++
>  1 files changed, 12 insertions(+), 0 deletions(-)

Very nice, thanks for listing the dependencies.  That makes things a lot
easier on this end.

thx,

Jason.

[PATCH] arm: mvebu: Add SPI flash on Armada XP-GP board

[Andrew Lunn]

> And don't forget to compile the SPI flash driver, CONFIG_MTD_M25P80=y

Hi Ezequiel

Maybe it makes sense to provide a patch to mvebu_defconfig to add 
CONFIG_MTD_M25P80=y ?

	Andrew

[PATCH] arm: mvebu: Add SPI flash on Armada XP-GP board

[Jason Cooper]

Morning-ish Andrew,

On Tue, Feb 05, 2013 at 01:38:27PM +0100, Andrew Lunn wrote:
> > And don't forget to compile the SPI flash driver, CONFIG_MTD_M25P80=y
> 
> Maybe it makes sense to provide a patch to mvebu_defconfig to add 
> CONFIG_MTD_M25P80=y ?

I thought this as well, then reconsidered when he said there was nothing
on it.  So it's not really needed to boot.  I don't have a strong
opinion on it though, so =y, =m, or =n.

thx,

Jason.

[PATCH] arm: mvebu: Add SPI flash on Armada XP-GP board

[Ezequiel Garcia]

On Tue, Feb 05, 2013 at 07:48:33AM -0500, Jason Cooper wrote:
> Morning-ish Andrew,
> 
> On Tue, Feb 05, 2013 at 01:38:27PM +0100, Andrew Lunn wrote:
> > > And don't forget to compile the SPI flash driver, CONFIG_MTD_M25P80=y
> > 
> > Maybe it makes sense to provide a patch to mvebu_defconfig to add 
> > CONFIG_MTD_M25P80=y ?
> 
> I thought this as well, then reconsidered when he said there was nothing
> on it.  So it's not really needed to boot.  I don't have a strong
> opinion on it though, so =y, =m, or =n.
> 

Well, the SPI flash may not be empty, if that's what you meant.
For instnace, my Armada XP GP board has the bootloader on that SPI,
and boots from there. After booting, the SPI flash is no longer needed.

IMHO, I don't think we should include the option CONFIG_MTD_M25P80
as it is just a particular SPI master device present in a particular board.

On the other hand, I don't have a strong opinion on this.
I've always wondered the *true* meaning of defconfigs,
so feel free to enligthen me :)

-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH] arm: mvebu: Add SPI flash on Armada XP-GP board

[Jason Cooper]

On Tue, Feb 05, 2013 at 11:27:21AM -0300, Ezequiel Garcia wrote:
> On Tue, Feb 05, 2013 at 07:48:33AM -0500, Jason Cooper wrote:
> > Morning-ish Andrew,
> > 
> > On Tue, Feb 05, 2013 at 01:38:27PM +0100, Andrew Lunn wrote:
> > > > And don't forget to compile the SPI flash driver, CONFIG_MTD_M25P80=y
> > > 
> > > Maybe it makes sense to provide a patch to mvebu_defconfig to add 
> > > CONFIG_MTD_M25P80=y ?
> > 
> > I thought this as well, then reconsidered when he said there was nothing
> > on it.  So it's not really needed to boot.  I don't have a strong
> > opinion on it though, so =y, =m, or =n.
> > 
> 
> Well, the SPI flash may not be empty, if that's what you meant.
> For instnace, my Armada XP GP board has the bootloader on that SPI,
> and boots from there. After booting, the SPI flash is no longer needed.

Is the bootloader environment on there?  Is there room for the dtb once
u-boot is upgraded to support fdt?

> IMHO, I don't think we should include the option CONFIG_MTD_M25P80
> as it is just a particular SPI master device present in a particular board.
> 
> On the other hand, I don't have a strong opinion on this.
> I've always wondered the *true* meaning of defconfigs,
> so feel free to enligthen me :)

Hmm, a quick look through Documentation/* didn't produce anything
authoritative.  There's Documentation/sh/new-machine.txt which says

"
Now all that's left to do is providing a defconfig for your new board.
This way, other people who end up with this board can simply use this
config for reference instead of trying to guess what settings are
supposed to be used on it.
"

One thing we've been doing a lot of with mvebu is using it for
build-testing.  For that use case, since the bootloader is there, I'd
recommend setting it.  This will reduce the likelyhood of nasty build
breakages futher down the line as we'll catch them before pushing to
arm-soc.

thx,

Jason.

[PATCH] arm: mvebu: Add SPI flash on Armada XP-GP board

[Thomas Petazzoni]

Dear Jason Cooper,

On Tue, 5 Feb 2013 10:31:35 -0500, Jason Cooper wrote:

> One thing we've been doing a lot of with mvebu is using it for
> build-testing.  For that use case, since the bootloader is there, I'd
> recommend setting it.  This will reduce the likelyhood of nasty build
> breakages futher down the line as we'll catch them before pushing to
> arm-soc.

To me, the defconfig should enable all the options needed for the
drivers of on-board devices. So we should enable the driver for this
SPI flash in the mvebu_defconfig.

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH] arm: mvebu: Add SPI flash on Armada XP-GP board

[Gregory CLEMENT]

On 02/05/2013 05:17 PM, Thomas Petazzoni wrote:
> Dear Jason Cooper,
> 
> On Tue, 5 Feb 2013 10:31:35 -0500, Jason Cooper wrote:
> 
>> One thing we've been doing a lot of with mvebu is using it for
>> build-testing.  For that use case, since the bootloader is there, I'd
>> recommend setting it.  This will reduce the likelyhood of nasty build
>> breakages futher down the line as we'll catch them before pushing to
>> arm-soc.
> 
> To me, the defconfig should enable all the options needed for the
> drivers of on-board devices. So we should enable the driver for this
> SPI flash in the mvebu_defconfig.
> 

So that means we should enable the drivers for all the devices present
on all the boards?

> Best regards,
> 
> Thomas
> 


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

[PATCH] arm: mvebu: Add SPI flash on Armada XP-GP board

[Thomas Petazzoni]

Dear Gregory CLEMENT,

On Tue, 05 Feb 2013 17:20:02 +0100, Gregory CLEMENT wrote:

> So that means we should enable the drivers for all the devices present
> on all the boards?

Yes. For example my mvsdio patch set enables some Wifi driver that
corresponds to a particular SDIO Wifi device that is present in the
Mirabox. But not on other boards.

Of course, it doesn't make sense to have drivers for pluggable devices
like PCIe devices that are not soldered, USB devices and things like
that. But for all the rest, I would say it should be in the defconfig.

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH] arm: mvebu: Add SPI flash on Armada XP-GP board

[Jason Cooper]

On Tue, Feb 05, 2013 at 06:07:05PM +0100, Thomas Petazzoni wrote:
> Of course, it doesn't make sense to have drivers for pluggable devices
> like PCIe devices that are not soldered, USB devices and things like
> that. But for all the rest, I would say it should be in the defconfig.

hmmm, so for the new devboard we should set CONFIG_RAM=n? ;-)

thx,

Jason.

[PATCH] arm: mvebu: Add SPI flash on Armada XP-GP board

[Lior Amsalem]

> From: Jason Cooper [mailto:jason at lakedaemon.net]
> Sent: Tuesday, February 05, 2013 8:09 PM
> 
> On Tue, Feb 05, 2013 at 06:07:05PM +0100, Thomas Petazzoni wrote:
> > Of course, it doesn't make sense to have drivers for pluggable devices
> > like PCIe devices that are not soldered, USB devices and things like
> > that. But for all the rest, I would say it should be in the defconfig.
> 
> hmmm, so for the new devboard we should set CONFIG_RAM=n? ;-)

I think something like CONFIG_RAM=m suit better :)

> 
> thx,
> 
> Jason.

Regards,
Lior Amsalem

[PATCH] arm: mvebu: Add SPI flash on Armada XP-GP board

[Ezequiel Garcia]

On Tue, Feb 05, 2013 at 06:07:05PM +0100, Thomas Petazzoni wrote:
> Dear Gregory CLEMENT,
> 
> On Tue, 05 Feb 2013 17:20:02 +0100, Gregory CLEMENT wrote:
> 
> > So that means we should enable the drivers for all the devices present
> > on all the boards?
> 
> Yes. For example my mvsdio patch set enables some Wifi driver that
> corresponds to a particular SDIO Wifi device that is present in the
> Mirabox. But not on other boards.
> 

Okey, I'll add everything needed to access and test the SPI flash,
including MTD_CHAR.

Thanks,

-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH] arm: mvebu: Add SPI flash on Armada XP-GP board

[Jason Cooper]

On Tue, Feb 05, 2013 at 05:17:01PM +0100, Thomas Petazzoni wrote:
> Dear Jason Cooper,
> 
> On Tue, 5 Feb 2013 10:31:35 -0500, Jason Cooper wrote:
> 
> > One thing we've been doing a lot of with mvebu is using it for
> > build-testing.  For that use case, since the bootloader is there, I'd
> > recommend setting it.  This will reduce the likelyhood of nasty build
> > breakages futher down the line as we'll catch them before pushing to
> > arm-soc.
> 
> To me, the defconfig should enable all the options needed for the
> drivers of on-board devices. So we should enable the driver for this
> SPI flash in the mvebu_defconfig.

I agree.

thx,

Jason.

[PATCH] arm: mvebu: Add SPI flash on Armada XP-GP board

[Gregory CLEMENT]

Hi Ezequiel,

On 02/05/2013 12:24 PM, Ezequiel Garcia wrote:
> This patch adds an SPI master device node for Armada XP-GP board.
> This master node is an SPI flash controller 'n25q128a13'.
> 
> Since there is no 'partitions' node declared, one full sized
> partition named as the device will be created.
> 
> Cc: Gregory Clement <gregory.clement@free-electrons.com>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Lior Amsalem <alior@marvell.com>
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> ---
> This patch depends on:
> 
>  1. Gregory's patch for Armada XP GP board:
>       arm: mvebu: support for the new Armada XP development board(DB-MV784MP-GP)
> 
>  2. My previous patch for SPI on Armada 370/XP:
>       arm: mvebu: Add support for SPI controller in Armada 370/XP
> 
> And don't forget to compile the SPI flash driver, CONFIG_MTD_M25P80=y
> 
>  arch/arm/boot/dts/armada-xp-gp.dts |   12 ++++++++++++
>  1 files changed, 12 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/armada-xp-gp.dts b/arch/arm/boot/dts/armada-xp-gp.dts
> index 3eea531..1c8afe2 100644
> --- a/arch/arm/boot/dts/armada-xp-gp.dts
> +++ b/arch/arm/boot/dts/armada-xp-gp.dts
> @@ -97,5 +97,17 @@
>  			phy = <&phy3>;
>  			phy-mode = "rgmii-id";
>  		};
> +
> +		spi0: spi at d0010600 {
> +			status = "okay";
> +
> +			spi-flash at 0 {
> +				#address-cells = <1>;
> +				#size-cells = <1>;
> +				compatible = "n25q128a13";
> +				reg = <0>; /* Chip select 0 */
> +				spi-max-frequency = <108000000>;
> +			};
> +		};
>  	};
>  };
> 

I applied, compiled and try this patch it worked well, at least
for reading, I didn't try to write on the flash where my boolader
is stored. So you can add my:

Tested-by: Gregory Clement <gregory.clement@free-electrons.com>


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

[PATCH] arm: mvebu: Add SPI flash on Armada XP-GP board

[Gregory CLEMENT]

On 02/05/2013 05:28 PM, Gregory CLEMENT wrote:
> Hi Ezequiel,
> 
> On 02/05/2013 12:24 PM, Ezequiel Garcia wrote:
>> This patch adds an SPI master device node for Armada XP-GP board.
>> This master node is an SPI flash controller 'n25q128a13'.
>>
>> Since there is no 'partitions' node declared, one full sized
>> partition named as the device will be created.
>>
>> Cc: Gregory Clement <gregory.clement@free-electrons.com>
>> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
>> Cc: Lior Amsalem <alior@marvell.com>
>> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
>> ---
>> This patch depends on:
>>
>>  1. Gregory's patch for Armada XP GP board:
>>       arm: mvebu: support for the new Armada XP development board(DB-MV784MP-GP)
>>
>>  2. My previous patch for SPI on Armada 370/XP:
>>       arm: mvebu: Add support for SPI controller in Armada 370/XP
>>
>> And don't forget to compile the SPI flash driver, CONFIG_MTD_M25P80=y
>>
>>  arch/arm/boot/dts/armada-xp-gp.dts |   12 ++++++++++++
>>  1 files changed, 12 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/arm/boot/dts/armada-xp-gp.dts b/arch/arm/boot/dts/armada-xp-gp.dts
>> index 3eea531..1c8afe2 100644
>> --- a/arch/arm/boot/dts/armada-xp-gp.dts
>> +++ b/arch/arm/boot/dts/armada-xp-gp.dts
>> @@ -97,5 +97,17 @@
>>  			phy = <&phy3>;
>>  			phy-mode = "rgmii-id";
>>  		};
>> +
>> +		spi0: spi at d0010600 {
>> +			status = "okay";
>> +
>> +			spi-flash at 0 {
>> +				#address-cells = <1>;
>> +				#size-cells = <1>;
>> +				compatible = "n25q128a13";
>> +				reg = <0>; /* Chip select 0 */
>> +				spi-max-frequency = <108000000>;

I had a remark about it, according to the datasheet, 108MHz is the
maximum frequency for the all the instructions but the READ
instruction. For the READ the maximum frequency is 54MHz. So I wonder
if we shouldn't use 54000000 here.



>> +			};
>> +		};
>>  	};
>>  };
>>
> 
> I applied, compiled and try this patch it worked well, at least
> for reading, I didn't try to write on the flash where my boolader
> is stored. So you can add my:
> 
> Tested-by: Gregory Clement <gregory.clement@free-electrons.com>
> 
> 


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

[PATCH] arm: mvebu: Add SPI flash on Armada XP-GP board

[Ezequiel Garcia]

Hi Gregory,

On Tue, Feb 05, 2013 at 09:17:02PM +0100, Gregory CLEMENT wrote:
> On 02/05/2013 05:28 PM, Gregory CLEMENT wrote:
> > Hi Ezequiel,
> > 
> > On 02/05/2013 12:24 PM, Ezequiel Garcia wrote:
> >> This patch adds an SPI master device node for Armada XP-GP board.
> >> This master node is an SPI flash controller 'n25q128a13'.
> >>
> >> Since there is no 'partitions' node declared, one full sized
> >> partition named as the device will be created.
> >>
> >> Cc: Gregory Clement <gregory.clement@free-electrons.com>
> >> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> >> Cc: Lior Amsalem <alior@marvell.com>
> >> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> >> ---
> >> This patch depends on:
> >>
> >>  1. Gregory's patch for Armada XP GP board:
> >>       arm: mvebu: support for the new Armada XP development board(DB-MV784MP-GP)
> >>
> >>  2. My previous patch for SPI on Armada 370/XP:
> >>       arm: mvebu: Add support for SPI controller in Armada 370/XP
> >>
> >> And don't forget to compile the SPI flash driver, CONFIG_MTD_M25P80=y
> >>
> >>  arch/arm/boot/dts/armada-xp-gp.dts |   12 ++++++++++++
> >>  1 files changed, 12 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/arch/arm/boot/dts/armada-xp-gp.dts b/arch/arm/boot/dts/armada-xp-gp.dts
> >> index 3eea531..1c8afe2 100644
> >> --- a/arch/arm/boot/dts/armada-xp-gp.dts
> >> +++ b/arch/arm/boot/dts/armada-xp-gp.dts
> >> @@ -97,5 +97,17 @@
> >>  			phy = <&phy3>;
> >>  			phy-mode = "rgmii-id";
> >>  		};
> >> +
> >> +		spi0: spi at d0010600 {
> >> +			status = "okay";
> >> +
> >> +			spi-flash at 0 {
> >> +				#address-cells = <1>;
> >> +				#size-cells = <1>;
> >> +				compatible = "n25q128a13";
> >> +				reg = <0>; /* Chip select 0 */
> >> +				spi-max-frequency = <108000000>;
> 
> I had a remark about it, according to the datasheet, 108MHz is the
> maximum frequency for the all the instructions but the READ
> instruction. For the READ the maximum frequency is 54MHz. So I wonder
> if we shouldn't use 54000000 here.
> 

Mmm... nice catch.

The mtd driver for the spi flash (m25p80) will use FAST_READ opcode
if CONFIG_M25PXX_USE_FAST_READ is selected, and this option
is selected by default.
However we cannot count on this option being selected, of course.

On the other side, after some testing with spi-max-frequency = 50 MHz
and also with spi-max-frequency = 108 MHz I'm seeing the flash often
(not always) stalls when trying to read the full device through dd:

/ # dd if=/dev/mtd0ro of=/dev/null

This happens regardless of CONFIG_M25PXX_USE_FAST_READ, and regardless
of spi-max-frequency = 50 MHz or 54 MHz or 108 MHz.

Note that setting 108 MHz is useless in our case, because there's
an upper limit of 62.5 MHz set by the core clock of the SoC.

Using a read block size of 1M, the read completes OK -- always:

/ # dd if=/dev/mtd0 of=/dev/null bs=1M
16+0 records in
16+0 records out
16777216 bytes (16.0MB) copied, 27.306048 seconds, 600.0KB/s

Moreover, when the read completes, the achieved bandwidth
is the same for 40, 50, 54, or 108 MHz.

If we set spi-max-frequency to 27 MHz (108 MHz / 4), it looks like the stalling
disappear and the read always completes OK, at almost half the speed:

/ # dd if=/dev/mtd0ro of=/dev/null
32768+0 records in
32768+0 records out
16777216 bytes (16.0MB) copied, 49.005122 seconds, 334.3KB/s

So according to this analysis, we whould set 27 MHz as the max
frequency, based on nothing but in these tests.

I'm not sure the stalling is due to the clocking or due to
some driver bug (I don't like the tricks pulled by the busy loop
in orion_spi_wait_till_ready() but probably this is completely unrelated).

Adding some SPI people in Cc, hoping they can shed a light on this.

What do you think?

-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH] arm: mvebu: Add SPI flash on Armada XP-GP board

[Gregory CLEMENT]

On 02/06/2013 11:54 AM, Ezequiel Garcia wrote:
> Hi Gregory,
> 
> On Tue, Feb 05, 2013 at 09:17:02PM +0100, Gregory CLEMENT wrote:
>> On 02/05/2013 05:28 PM, Gregory CLEMENT wrote:
>>> Hi Ezequiel,
>>>
>>> On 02/05/2013 12:24 PM, Ezequiel Garcia wrote:
>>>> This patch adds an SPI master device node for Armada XP-GP board.
>>>> This master node is an SPI flash controller 'n25q128a13'.
>>>>
>>>> Since there is no 'partitions' node declared, one full sized
>>>> partition named as the device will be created.
>>>>
>>>> Cc: Gregory Clement <gregory.clement@free-electrons.com>
>>>> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
>>>> Cc: Lior Amsalem <alior@marvell.com>
>>>> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
>>>> ---
>>>> This patch depends on:
>>>>
>>>>  1. Gregory's patch for Armada XP GP board:
>>>>       arm: mvebu: support for the new Armada XP development board(DB-MV784MP-GP)
>>>>
>>>>  2. My previous patch for SPI on Armada 370/XP:
>>>>       arm: mvebu: Add support for SPI controller in Armada 370/XP
>>>>
>>>> And don't forget to compile the SPI flash driver, CONFIG_MTD_M25P80=y
>>>>
>>>>  arch/arm/boot/dts/armada-xp-gp.dts |   12 ++++++++++++
>>>>  1 files changed, 12 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/arch/arm/boot/dts/armada-xp-gp.dts b/arch/arm/boot/dts/armada-xp-gp.dts
>>>> index 3eea531..1c8afe2 100644
>>>> --- a/arch/arm/boot/dts/armada-xp-gp.dts
>>>> +++ b/arch/arm/boot/dts/armada-xp-gp.dts
>>>> @@ -97,5 +97,17 @@
>>>>  			phy = <&phy3>;
>>>>  			phy-mode = "rgmii-id";
>>>>  		};
>>>> +
>>>> +		spi0: spi at d0010600 {
>>>> +			status = "okay";
>>>> +
>>>> +			spi-flash at 0 {
>>>> +				#address-cells = <1>;
>>>> +				#size-cells = <1>;
>>>> +				compatible = "n25q128a13";
>>>> +				reg = <0>; /* Chip select 0 */
>>>> +				spi-max-frequency = <108000000>;
>>
>> I had a remark about it, according to the datasheet, 108MHz is the
>> maximum frequency for the all the instructions but the READ
>> instruction. For the READ the maximum frequency is 54MHz. So I wonder
>> if we shouldn't use 54000000 here.
>>
> 
> Mmm... nice catch.
> 
> The mtd driver for the spi flash (m25p80) will use FAST_READ opcode
> if CONFIG_M25PXX_USE_FAST_READ is selected, and this option
> is selected by default.

OK I didn't notice that thre wre a FAST_READ which was supported by
Linux. So I think we can keep 108000000 as spi-max-frequency.

> However we cannot count on this option being selected, of course.
> 
> On the other side, after some testing with spi-max-frequency = 50 MHz
> and also with spi-max-frequency = 108 MHz I'm seeing the flash often
> (not always) stalls when trying to read the full device through dd:
> 
> / # dd if=/dev/mtd0ro of=/dev/null
> 
> This happens regardless of CONFIG_M25PXX_USE_FAST_READ, and regardless
> of spi-max-frequency = 50 MHz or 54 MHz or 108 MHz.
> 
> Note that setting 108 MHz is useless in our case, because there's
> an upper limit of 62.5 MHz set by the core clock of the SoC.
> 
> Using a read block size of 1M, the read completes OK -- always:
> 
> / # dd if=/dev/mtd0 of=/dev/null bs=1M
> 16+0 records in
> 16+0 records out
> 16777216 bytes (16.0MB) copied, 27.306048 seconds, 600.0KB/s
> 
> Moreover, when the read completes, the achieved bandwidth
> is the same for 40, 50, 54, or 108 MHz.
> 
> If we set spi-max-frequency to 27 MHz (108 MHz / 4), it looks like the stalling
> disappear and the read always completes OK, at almost half the speed:
> 
> / # dd if=/dev/mtd0ro of=/dev/null
> 32768+0 records in
> 32768+0 records out
> 16777216 bytes (16.0MB) copied, 49.005122 seconds, 334.3KB/s
> 
> So according to this analysis, we whould set 27 MHz as the max
> frequency, based on nothing but in these tests.
> 
> I'm not sure the stalling is due to the clocking or due to
> some driver bug (I don't like the tricks pulled by the busy loop
> in orion_spi_wait_till_ready() but probably this is completely unrelated).
> 
> Adding some SPI people in Cc, hoping they can shed a light on this.
> 
> What do you think?

We have just seen this on IRC, so it seemed it was actually a problem in
userspace not in kernel space.

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

[PATCH] arm: mvebu: Add SPI flash on Armada XP-GP board

[Ezequiel Garcia]

(Adding mtd in Cc)

On Wed, Feb 06, 2013 at 07:54:31AM -0300, Ezequiel Garcia wrote:
> Hi Gregory,
> 
> On Tue, Feb 05, 2013 at 09:17:02PM +0100, Gregory CLEMENT wrote:
> > On 02/05/2013 05:28 PM, Gregory CLEMENT wrote:
> > > Hi Ezequiel,
> > > 
> > > On 02/05/2013 12:24 PM, Ezequiel Garcia wrote:
> > >> This patch adds an SPI master device node for Armada XP-GP board.
> > >> This master node is an SPI flash controller 'n25q128a13'.
> > >>
> > >> Since there is no 'partitions' node declared, one full sized
> > >> partition named as the device will be created.
> > >>
> > >> Cc: Gregory Clement <gregory.clement@free-electrons.com>
> > >> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> > >> Cc: Lior Amsalem <alior@marvell.com>
> > >> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> > >> ---
> > >> This patch depends on:
> > >>
> > >>  1. Gregory's patch for Armada XP GP board:
> > >>       arm: mvebu: support for the new Armada XP development board(DB-MV784MP-GP)
> > >>
> > >>  2. My previous patch for SPI on Armada 370/XP:
> > >>       arm: mvebu: Add support for SPI controller in Armada 370/XP
> > >>
> > >> And don't forget to compile the SPI flash driver, CONFIG_MTD_M25P80=y
> > >>
> > >>  arch/arm/boot/dts/armada-xp-gp.dts |   12 ++++++++++++
> > >>  1 files changed, 12 insertions(+), 0 deletions(-)
> > >>
> > >> diff --git a/arch/arm/boot/dts/armada-xp-gp.dts b/arch/arm/boot/dts/armada-xp-gp.dts
> > >> index 3eea531..1c8afe2 100644
> > >> --- a/arch/arm/boot/dts/armada-xp-gp.dts
> > >> +++ b/arch/arm/boot/dts/armada-xp-gp.dts
> > >> @@ -97,5 +97,17 @@
> > >>  			phy = <&phy3>;
> > >>  			phy-mode = "rgmii-id";
> > >>  		};
> > >> +
> > >> +		spi0: spi at d0010600 {
> > >> +			status = "okay";
> > >> +
> > >> +			spi-flash at 0 {
> > >> +				#address-cells = <1>;
> > >> +				#size-cells = <1>;
> > >> +				compatible = "n25q128a13";
> > >> +				reg = <0>; /* Chip select 0 */
> > >> +				spi-max-frequency = <108000000>;
> > 
> > I had a remark about it, according to the datasheet, 108MHz is the
> > maximum frequency for the all the instructions but the READ
> > instruction. For the READ the maximum frequency is 54MHz. So I wonder
> > if we shouldn't use 54000000 here.
> > 
> 
> Mmm... nice catch.
> 
> The mtd driver for the spi flash (m25p80) will use FAST_READ opcode
> if CONFIG_M25PXX_USE_FAST_READ is selected, and this option
> is selected by default.
> However we cannot count on this option being selected, of course.
> 
> On the other side, after some testing with spi-max-frequency = 50 MHz
> and also with spi-max-frequency = 108 MHz I'm seeing the flash often
> (not always) stalls when trying to read the full device through dd:
> 
> / # dd if=/dev/mtd0ro of=/dev/null
> 

FWIW, using CONFIG_DETECT_HUNG_TASK we can see the hung happens inside spi_sync().
It seems the completion handler is never called,
though I still don't understand under what circumstances that can happen.

dd              D c02d79c8     0   964    961 0x00000000
[<c02d79c8>] (__schedule+0x1c8/0x504) from [<c02d61d0>] (schedule_timeout+0x158/0x200)
[<c02d61d0>] (schedule_timeout+0x158/0x200) from [<c02d7504>] (wait_for_common+0xe0/0x194)
[<c02d7504>] (wait_for_common+0xe0/0x194) from [<c0215b44>] (spi_sync+0x74/0x90)
[<c0215b44>] (spi_sync+0x74/0x90) from [<c0214c80>] (m25p80_read+0x100/0x17c)
[<c0214c80>] (m25p80_read+0x100/0x17c) from [<c020edac>] (mtd_read+0x8c/0xc0)
[<c020edac>] (mtd_read+0x8c/0xc0) from [<c021404c>] (mtdchar_read+0xe0/0x224)
[<c021404c>] (mtdchar_read+0xe0/0x224) from [<c00c757c>] (vfs_read+0xa4/0x148)
[<c00c757c>] (vfs_read+0xa4/0x148) from [<c00c7664>] (sys_read+0x44/0x78)
[<c00c7664>] (sys_read+0x44/0x78) from [<c000ede0>] (ret_fast_syscall+0x0/0x30)

-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com