subject:"\[U\-Boot\] \[PATCH v2\] spi\: cadence_qspi_apb\: Use 32 bit indirect write transaction when possible"

Re: [U-Boot] [PATCH v2] spi: cadence_qspi_apb: Use 32 bit indirect write transaction when possible

2016-12-07 Thread Vignesh R



[...]

>> Did you get a chance to test this patch? There is also a similar
>> patch
>> for indirect read as well[1], it would be great if you could give
>> your
>> Tested-by for both the patches. Thanks!
>>
>> [1]https://patchwork.ozlabs.org/patch/700990/
> 
> Actually I was bumping into sf probe error. This is happen at mainline
> even without your patch. Let me continue the troubelshooting at my side

Is it a micron flash on the board? If yes, this might be the issue:
https://www.mail-archive.com/u-boot@lists.denx.de/msg233629.html

> and test out your patch asap.
> 

Thanks!

-- 
Regards
Vignesh
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v2] spi: cadence_qspi_apb: Use 32 bit indirect write transaction when possible

2016-12-07 Thread See, Chin Liang

On Sel, 2016-12-06 at 10:54 +0530, Vignesh R wrote:
> 
> Hi,
> 
> On Thursday 01 December 2016 09:41 AM, Vignesh R wrote:
> [...]
> > 
> > 
> > > 
> > > 
> > > > 
> > > > 
> > > > Data slave port does accept byte, half-word and word access,
> > > > there
> > > > are
> > > > no data aborts. But indirect write controller seems to have
> > > > limitation(as documented in section 11.15.4.9.2) couping with
> > > > non 32-
> > > > bit
> > > > data writes on TI platform. For example with current driver if
> > > > I try:
> > > > 
> > > > fatload mmc 0 0x8200 zImage
> > > > sf erase 0x0 0x50
> > > > sf write 0x8200 0x0 0x35
> > > > sf read 0xA000 0x0 0x35
> > > > 
> > > > 
> > > > md.b 0xA000
> > > > a000: 00 00 a0 00 00 00 a0 00 00 00 a0 00 00 00 a0 00
> > > > a010: 00 00 a0 00 00 00 a0 00 00 00 a0 00 00 00 a0 00
> > > > a020: 03 00 00 00 18 28 6f 00 00 00 00 00 d8 5b 35 00
> > > > a030: 01 02 03 00 00 00 00 00 00 00 00 00 00 00 00 00
> > > > md.b 0x8200
> > > > 8200: 00 00 a0 e1 00 00 a0 e1 00 00 a0 e1 00 00 a0 e1
> > > > 8210: 00 00 a0 e1 00 00 a0 e1 00 00 a0 e1 00 00 a0 e1
> > > > 8220: 03 00 00 ea 18 28 6f 01 00 00 00 00 d8 5b 35 00
> > > > 8230: 01 02 03 04 00 90 0f e1 88 07 00 eb 01 70 a0 e1
> > > > 
> > > > 
> > > > As you can see, every fourth byte turn out to be 0x00.
> > > > Therefore this
> > > > patch is required.
> > > Thanks Vignesh
> > > 
> > > Interesting to know that the newer version of controller has this
> > > constrain. Let me pull out my board to ensure this patch doesn't
> > > break
> > > the SOCFPGA
> > > 
> Did you get a chance to test this patch? There is also a similar
> patch
> for indirect read as well[1], it would be great if you could give
> your
> Tested-by for both the patches. Thanks!
> 
> [1]https://patchwork.ozlabs.org/patch/700990/

Actually I was bumping into sf probe error. This is happen at mainline
even without your patch. Let me continue the troubelshooting at my side
and test out your patch asap.

Thanks
Chin Liang
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v2] spi: cadence_qspi_apb: Use 32 bit indirect write transaction when possible

2016-12-05 Thread Vignesh R

Hi,

On Thursday 01 December 2016 09:41 AM, Vignesh R wrote:
[...]
>>> Data slave port does accept byte, half-word and word access, there
>>> are
>>> no data aborts. But indirect write controller seems to have
>>> limitation(as documented in section 11.15.4.9.2) couping with non 32-
>>> bit
>>> data writes on TI platform. For example with current driver if I try:
>>>
>>> fatload mmc 0 0x8200 zImage
>>> sf erase 0x0 0x50
>>> sf write 0x8200 0x0 0x35
>>> sf read 0xA000 0x0 0x35
>>>
>>>
>>> md.b 0xA000
>>> a000: 00 00 a0 00 00 00 a0 00 00 00 a0 00 00 00 a0 00
>>> a010: 00 00 a0 00 00 00 a0 00 00 00 a0 00 00 00 a0 00
>>> a020: 03 00 00 00 18 28 6f 00 00 00 00 00 d8 5b 35 00
>>> a030: 01 02 03 00 00 00 00 00 00 00 00 00 00 00 00 00
>>> md.b 0x8200
>>> 8200: 00 00 a0 e1 00 00 a0 e1 00 00 a0 e1 00 00 a0 e1
>>> 8210: 00 00 a0 e1 00 00 a0 e1 00 00 a0 e1 00 00 a0 e1
>>> 8220: 03 00 00 ea 18 28 6f 01 00 00 00 00 d8 5b 35 00
>>> 8230: 01 02 03 04 00 90 0f e1 88 07 00 eb 01 70 a0 e1
>>>
>>>
>>> As you can see, every fourth byte turn out to be 0x00. Therefore this
>>> patch is required.
>>
>> Thanks Vignesh
>>
>> Interesting to know that the newer version of controller has this
>> constrain. Let me pull out my board to ensure this patch doesn't break
>> the SOCFPGA
>>

Did you get a chance to test this patch? There is also a similar patch
for indirect read as well[1], it would be great if you could give your
Tested-by for both the patches. Thanks!

[1]https://patchwork.ozlabs.org/patch/700990/
-- 
Regards
Vignesh
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v2] spi: cadence_qspi_apb: Use 32 bit indirect write transaction when possible

2016-11-30 Thread Vignesh R

[...]
>>>
>>> Hmmm... From 11.15.4.1.1, the data slave port should able to accept
>>> only byte, half-word and word access. This should not create any
>>> data
>>> abort but probably bad performance. But it should insignificant as
>>> access time for the flash is longer than the data port access
>>> itself.
>>>
>> Data slave port does accept byte, half-word and word access, there
>> are
>> no data aborts. But indirect write controller seems to have
>> limitation(as documented in section 11.15.4.9.2) couping with non 32-
>> bit
>> data writes on TI platform. For example with current driver if I try:
>>
>> fatload mmc 0 0x8200 zImage
>> sf erase 0x0 0x50
>> sf write 0x8200 0x0 0x35
>> sf read 0xA000 0x0 0x35
>>
>>
>> md.b 0xA000
>> a000: 00 00 a0 00 00 00 a0 00 00 00 a0 00 00 00 a0 00
>> a010: 00 00 a0 00 00 00 a0 00 00 00 a0 00 00 00 a0 00
>> a020: 03 00 00 00 18 28 6f 00 00 00 00 00 d8 5b 35 00
>> a030: 01 02 03 00 00 00 00 00 00 00 00 00 00 00 00 00
>> md.b 0x8200
>> 8200: 00 00 a0 e1 00 00 a0 e1 00 00 a0 e1 00 00 a0 e1
>> 8210: 00 00 a0 e1 00 00 a0 e1 00 00 a0 e1 00 00 a0 e1
>> 8220: 03 00 00 ea 18 28 6f 01 00 00 00 00 d8 5b 35 00
>> 8230: 01 02 03 04 00 90 0f e1 88 07 00 eb 01 70 a0 e1
>>
>>
>> As you can see, every fourth byte turn out to be 0x00. Therefore this
>> patch is required.
> 
> Thanks Vignesh
> 
> Interesting to know that the newer version of controller has this
> constrain. Let me pull out my board to ensure this patch doesn't break
> the SOCFPGA
> 

Not sure if this constraint is due to newer version of controller or due
to how the IP is integration with SoC.


-- 
Regards
Vignesh
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v2] spi: cadence_qspi_apb: Use 32 bit indirect write transaction when possible

2016-11-30 Thread Chin Liang See

On Sel, 2016-11-29 at 10:55 +0530, Vignesh R wrote:
> 
> On Monday 28 November 2016 07:45 PM, See, Chin Liang wrote:
> > 
> > On Jum, 2016-11-25 at 17:51 +0100, Marek Vasut wrote:
> > > 
> > > On 11/24/2016 06:35 AM, Vignesh R wrote:
> > > > 
> > > > 
> > > > According to Section 11.15.4.9.2 Indirect Write Controller of
> > > > K2G
> > > > SoC
> > > > TRM SPRUHY8D[1], the external master is only permitted to issue
> > > > 32-
> > > > bit
> > > > data interface writes until the last word of an indirect
> > > > transfer
> > > > otherwise indirect writes is known to fails sometimes. So, make
> > > > sure
> > > > that QSPI indirect writes are 32 bit sized except for the last
> > > > write. If
> > > > the txbuf is unaligned then use bounce buffer to avoid data
> > > > aborts.
> > > > 
> > > > So, now that the driver uses bounce_buffer, enable
> > > > CONFIG_BOUNCE_BUFFER
> > > > for all boards that use Cadence QSPI driver.
> > > > 
> > > > [1]www.ti.com/lit/ug/spruhy8d/spruhy8d.pdf
> > > > 
> > > > Signed-off-by: Vignesh R 
> > > > ---
> > > Reviewed-by: Marek Vasut 
> > > 
> > > I'd like to have at least Dinh's/Chin's ack on this.
> > THanks Marek
> > 
> > Hmmm... From 11.15.4.1.1, the data slave port should able to accept
> > only byte, half-word and word access. This should not create any
> > data
> > abort but probably bad performance. But it should insignificant as
> > access time for the flash is longer than the data port access
> > itself.
> > 
> Data slave port does accept byte, half-word and word access, there
> are
> no data aborts. But indirect write controller seems to have
> limitation(as documented in section 11.15.4.9.2) couping with non 32-
> bit
> data writes on TI platform. For example with current driver if I try:
> 
> fatload mmc 0 0x8200 zImage
> sf erase 0x0 0x50
> sf write 0x8200 0x0 0x35
> sf read 0xA000 0x0 0x35
> 
> 
> md.b 0xA000
> a000: 00 00 a0 00 00 00 a0 00 00 00 a0 00 00 00 a0 00
> a010: 00 00 a0 00 00 00 a0 00 00 00 a0 00 00 00 a0 00
> a020: 03 00 00 00 18 28 6f 00 00 00 00 00 d8 5b 35 00
> a030: 01 02 03 00 00 00 00 00 00 00 00 00 00 00 00 00
> md.b 0x8200
> 8200: 00 00 a0 e1 00 00 a0 e1 00 00 a0 e1 00 00 a0 e1
> 8210: 00 00 a0 e1 00 00 a0 e1 00 00 a0 e1 00 00 a0 e1
> 8220: 03 00 00 ea 18 28 6f 01 00 00 00 00 d8 5b 35 00
> 8230: 01 02 03 04 00 90 0f e1 88 07 00 eb 01 70 a0 e1
> 
> 
> As you can see, every fourth byte turn out to be 0x00. Therefore this
> patch is required.

Thanks Vignesh

Interesting to know that the newer version of controller has this
constrain. Let me pull out my board to ensure this patch doesn't break
the SOCFPGA

Thanks
Chin Liang

> 
> 
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v2] spi: cadence_qspi_apb: Use 32 bit indirect write transaction when possible

2016-11-29 Thread Vignesh R

[...]
>>>
>>> I'd like to have at least Dinh's/Chin's ack on this.
>>>
>>> btw don't you need BB for READ as well ?
>>>
>>
>> I don't see any issue with READ due to non word size accesses ATM,
>
> Like user does sf read ... 0x1003 0x100 , you'll likely have a problem, 
> no?

 No issues with that either. The above limitation seems to be only wrt sf
 write (indirect write).
>>>
>>> Because indirect read already uses readsb to handle the alignment , right ?
>>>
>>
>> Alignment is not the problem here, even indirect write uses writesb to
>> handle alignment. But the problem is, driver uses readsb/writesb (which
>> perform byte wise accesses) for reading/writing, if txbuf/rxbuf is
>> unaligned or data length is not a multiple of word size. As per the TI
>> K2G SoC TRM, external master is not permitted to do non 32 bit indirect
>> read/write accesses except for last read/write.
>> So, if the driver writes say 25 bytes, one byte at a time using writesb,
>> then some bytes are do not get written to flash correctly (instead 0x0
>> is written).
> 
> Well ok, then we have a problem on READ as well.
> 
>> What my patch is doing is to use bounce buffer so that txbuf is always
>> aligned and writesl can be used instead of writesb. And also make sure
>> that writesb is only to right last trailing bytes in case data length is
>> not a multiple of word size.
> 
> Right.
> 
>> But wrt indirect read, I don't see any such data corruption or other
>> issues if driver reads, say 25 bytes, one byte at a time using readsb
>> (though the TRM advises against this)
> 
> Correct, so fix the READ path too to be extra sure.
> 

Yes, I will submit a separate patch for that shortly.

-- 
Regards
Vignesh
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v2] spi: cadence_qspi_apb: Use 32 bit indirect write transaction when possible

2016-11-29 Thread Marek Vasut

On 11/29/2016 01:08 PM, Vignesh R wrote:
> 
> 
> On Tuesday 29 November 2016 04:23 PM, Marek Vasut wrote:
>> On 11/29/2016 05:58 AM, Vignesh R wrote:
>>>
>>>
>>> On Monday 28 November 2016 06:11 PM, Marek Vasut wrote:
 On 11/28/2016 10:37 AM, Vignesh R wrote:
>
>
> On Friday 25 November 2016 10:21 PM, Marek Vasut wrote:
>> On 11/24/2016 06:35 AM, Vignesh R wrote:
>>> According to Section 11.15.4.9.2 Indirect Write Controller of K2G SoC
>>> TRM SPRUHY8D[1], the external master is only permitted to issue 32-bit
>>> data interface writes until the last word of an indirect transfer
>>> otherwise indirect writes is known to fails sometimes. So, make sure
>>> that QSPI indirect writes are 32 bit sized except for the last write. If
>>> the txbuf is unaligned then use bounce buffer to avoid data aborts.
>>>
>>> So, now that the driver uses bounce_buffer, enable CONFIG_BOUNCE_BUFFER
>>> for all boards that use Cadence QSPI driver.
>>>
>>> [1]www.ti.com/lit/ug/spruhy8d/spruhy8d.pdf
>>>
>>> Signed-off-by: Vignesh R 
>>> ---
>>
>> Reviewed-by: Marek Vasut 
>>
>> I'd like to have at least Dinh's/Chin's ack on this.
>>
>> btw don't you need BB for READ as well ?
>>
>
> I don't see any issue with READ due to non word size accesses ATM,

 Like user does sf read ... 0x1003 0x100 , you'll likely have a problem, no?
>>>
>>> No issues with that either. The above limitation seems to be only wrt sf
>>> write (indirect write).
>>
>> Because indirect read already uses readsb to handle the alignment , right ?
>>
> 
> Alignment is not the problem here, even indirect write uses writesb to
> handle alignment. But the problem is, driver uses readsb/writesb (which
> perform byte wise accesses) for reading/writing, if txbuf/rxbuf is
> unaligned or data length is not a multiple of word size. As per the TI
> K2G SoC TRM, external master is not permitted to do non 32 bit indirect
> read/write accesses except for last read/write.
> So, if the driver writes say 25 bytes, one byte at a time using writesb,
> then some bytes are do not get written to flash correctly (instead 0x0
> is written).

Well ok, then we have a problem on READ as well.

> What my patch is doing is to use bounce buffer so that txbuf is always
> aligned and writesl can be used instead of writesb. And also make sure
> that writesb is only to right last trailing bytes in case data length is
> not a multiple of word size.

Right.

> But wrt indirect read, I don't see any such data corruption or other
> issues if driver reads, say 25 bytes, one byte at a time using readsb
> (though the TRM advises against this)

Correct, so fix the READ path too to be extra sure.

-- 
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v2] spi: cadence_qspi_apb: Use 32 bit indirect write transaction when possible

2016-11-29 Thread Vignesh R

On Tuesday 29 November 2016 04:23 PM, Marek Vasut wrote:
> On 11/29/2016 05:58 AM, Vignesh R wrote:
>>
>>
>> On Monday 28 November 2016 06:11 PM, Marek Vasut wrote:
>>> On 11/28/2016 10:37 AM, Vignesh R wrote:

 On Friday 25 November 2016 10:21 PM, Marek Vasut wrote:
> On 11/24/2016 06:35 AM, Vignesh R wrote:
>> According to Section 11.15.4.9.2 Indirect Write Controller of K2G SoC
>> TRM SPRUHY8D[1], the external master is only permitted to issue 32-bit
>> data interface writes until the last word of an indirect transfer
>> otherwise indirect writes is known to fails sometimes. So, make sure
>> that QSPI indirect writes are 32 bit sized except for the last write. If
>> the txbuf is unaligned then use bounce buffer to avoid data aborts.
>>
>> So, now that the driver uses bounce_buffer, enable CONFIG_BOUNCE_BUFFER
>> for all boards that use Cadence QSPI driver.
>>
>> [1]www.ti.com/lit/ug/spruhy8d/spruhy8d.pdf
>>
>> Signed-off-by: Vignesh R 
>> ---
>
> Reviewed-by: Marek Vasut 
>
> I'd like to have at least Dinh's/Chin's ack on this.
>
> btw don't you need BB for READ as well ?
>

 I don't see any issue with READ due to non word size accesses ATM,
>>>
>>> Like user does sf read ... 0x1003 0x100 , you'll likely have a problem, no?
>>
>> No issues with that either. The above limitation seems to be only wrt sf
>> write (indirect write).
> 
> Because indirect read already uses readsb to handle the alignment , right ?
> 

Alignment is not the problem here, even indirect write uses writesb to
handle alignment. But the problem is, driver uses readsb/writesb (which
perform byte wise accesses) for reading/writing, if txbuf/rxbuf is
unaligned or data length is not a multiple of word size. As per the TI
K2G SoC TRM, external master is not permitted to do non 32 bit indirect
read/write accesses except for last read/write.
So, if the driver writes say 25 bytes, one byte at a time using writesb,
then some bytes are do not get written to flash correctly (instead 0x0
is written).

What my patch is doing is to use bounce buffer so that txbuf is always
aligned and writesl can be used instead of writesb. And also make sure
that writesb is only to right last trailing bytes in case data length is
not a multiple of word size.

But wrt indirect read, I don't see any such data corruption or other
issues if driver reads, say 25 bytes, one byte at a time using readsb
(though the TRM advises against this)

-- 
Regards
Vignesh
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v2] spi: cadence_qspi_apb: Use 32 bit indirect write transaction when possible

2016-11-29 Thread Marek Vasut

On 11/29/2016 05:58 AM, Vignesh R wrote:
> 
> 
> On Monday 28 November 2016 06:11 PM, Marek Vasut wrote:
>> On 11/28/2016 10:37 AM, Vignesh R wrote:
>>>
>>>
>>> On Friday 25 November 2016 10:21 PM, Marek Vasut wrote:
 On 11/24/2016 06:35 AM, Vignesh R wrote:
> According to Section 11.15.4.9.2 Indirect Write Controller of K2G SoC
> TRM SPRUHY8D[1], the external master is only permitted to issue 32-bit
> data interface writes until the last word of an indirect transfer
> otherwise indirect writes is known to fails sometimes. So, make sure
> that QSPI indirect writes are 32 bit sized except for the last write. If
> the txbuf is unaligned then use bounce buffer to avoid data aborts.
>
> So, now that the driver uses bounce_buffer, enable CONFIG_BOUNCE_BUFFER
> for all boards that use Cadence QSPI driver.
>
> [1]www.ti.com/lit/ug/spruhy8d/spruhy8d.pdf
>
> Signed-off-by: Vignesh R 
> ---

 Reviewed-by: Marek Vasut 

 I'd like to have at least Dinh's/Chin's ack on this.

 btw don't you need BB for READ as well ?

>>>
>>> I don't see any issue with READ due to non word size accesses ATM,
>>
>> Like user does sf read ... 0x1003 0x100 , you'll likely have a problem, no?
> 
> No issues with that either. The above limitation seems to be only wrt sf
> write (indirect write).

Because indirect read already uses readsb to handle the alignment , right ?

-- 
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v2] spi: cadence_qspi_apb: Use 32 bit indirect write transaction when possible

2016-11-28 Thread Vignesh R



On Monday 28 November 2016 07:45 PM, See, Chin Liang wrote:
> On Jum, 2016-11-25 at 17:51 +0100, Marek Vasut wrote:
>> On 11/24/2016 06:35 AM, Vignesh R wrote:
>>>
>>> According to Section 11.15.4.9.2 Indirect Write Controller of K2G
>>> SoC
>>> TRM SPRUHY8D[1], the external master is only permitted to issue 32-
>>> bit
>>> data interface writes until the last word of an indirect transfer
>>> otherwise indirect writes is known to fails sometimes. So, make
>>> sure
>>> that QSPI indirect writes are 32 bit sized except for the last
>>> write. If
>>> the txbuf is unaligned then use bounce buffer to avoid data aborts.
>>>
>>> So, now that the driver uses bounce_buffer, enable
>>> CONFIG_BOUNCE_BUFFER
>>> for all boards that use Cadence QSPI driver.
>>>
>>> [1]www.ti.com/lit/ug/spruhy8d/spruhy8d.pdf
>>>
>>> Signed-off-by: Vignesh R 
>>> ---
>> Reviewed-by: Marek Vasut 
>>
>> I'd like to have at least Dinh's/Chin's ack on this.
> 
> THanks Marek
> 
> Hmmm... From 11.15.4.1.1, the data slave port should able to accept
> only byte, half-word and word access. This should not create any data
> abort but probably bad performance. But it should insignificant as
> access time for the flash is longer than the data port access itself.
> 

Data slave port does accept byte, half-word and word access, there are
no data aborts. But indirect write controller seems to have
limitation(as documented in section 11.15.4.9.2) couping with non 32-bit
data writes on TI platform. For example with current driver if I try:

fatload mmc 0 0x8200 zImage
sf erase 0x0 0x50
sf write 0x8200 0x0 0x35
sf read 0xA000 0x0 0x35


md.b 0xA000
a000: 00 00 a0 00 00 00 a0 00 00 00 a0 00 00 00 a0 00
a010: 00 00 a0 00 00 00 a0 00 00 00 a0 00 00 00 a0 00
a020: 03 00 00 00 18 28 6f 00 00 00 00 00 d8 5b 35 00
a030: 01 02 03 00 00 00 00 00 00 00 00 00 00 00 00 00
md.b 0x8200
8200: 00 00 a0 e1 00 00 a0 e1 00 00 a0 e1 00 00 a0 e1
8210: 00 00 a0 e1 00 00 a0 e1 00 00 a0 e1 00 00 a0 e1
8220: 03 00 00 ea 18 28 6f 01 00 00 00 00 d8 5b 35 00
8230: 01 02 03 04 00 90 0f e1 88 07 00 eb 01 70 a0 e1


As you can see, every fourth byte turn out to be 0x00. Therefore this
patch is required.


-- 
Regards
Vignesh
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v2] spi: cadence_qspi_apb: Use 32 bit indirect write transaction when possible

2016-11-28 Thread Vignesh R



On Monday 28 November 2016 06:11 PM, Marek Vasut wrote:
> On 11/28/2016 10:37 AM, Vignesh R wrote:
>>
>>
>> On Friday 25 November 2016 10:21 PM, Marek Vasut wrote:
>>> On 11/24/2016 06:35 AM, Vignesh R wrote:
 According to Section 11.15.4.9.2 Indirect Write Controller of K2G SoC
 TRM SPRUHY8D[1], the external master is only permitted to issue 32-bit
 data interface writes until the last word of an indirect transfer
 otherwise indirect writes is known to fails sometimes. So, make sure
 that QSPI indirect writes are 32 bit sized except for the last write. If
 the txbuf is unaligned then use bounce buffer to avoid data aborts.

 So, now that the driver uses bounce_buffer, enable CONFIG_BOUNCE_BUFFER
 for all boards that use Cadence QSPI driver.

 [1]www.ti.com/lit/ug/spruhy8d/spruhy8d.pdf

 Signed-off-by: Vignesh R 
 ---
>>>
>>> Reviewed-by: Marek Vasut 
>>>
>>> I'd like to have at least Dinh's/Chin's ack on this.
>>>
>>> btw don't you need BB for READ as well ?
>>>
>>
>> I don't see any issue with READ due to non word size accesses ATM,
> 
> Like user does sf read ... 0x1003 0x100 , you'll likely have a problem, no?

No issues with that either. The above limitation seems to be only wrt sf
write (indirect write).


-- 
Regards
Vignesh
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v2] spi: cadence_qspi_apb: Use 32 bit indirect write transaction when possible

2016-11-28 Thread See, Chin Liang

On Jum, 2016-11-25 at 17:51 +0100, Marek Vasut wrote:
> On 11/24/2016 06:35 AM, Vignesh R wrote:
> > 
> > According to Section 11.15.4.9.2 Indirect Write Controller of K2G
> > SoC
> > TRM SPRUHY8D[1], the external master is only permitted to issue 32-
> > bit
> > data interface writes until the last word of an indirect transfer
> > otherwise indirect writes is known to fails sometimes. So, make
> > sure
> > that QSPI indirect writes are 32 bit sized except for the last
> > write. If
> > the txbuf is unaligned then use bounce buffer to avoid data aborts.
> > 
> > So, now that the driver uses bounce_buffer, enable
> > CONFIG_BOUNCE_BUFFER
> > for all boards that use Cadence QSPI driver.
> > 
> > [1]www.ti.com/lit/ug/spruhy8d/spruhy8d.pdf
> > 
> > Signed-off-by: Vignesh R 
> > ---
> Reviewed-by: Marek Vasut 
> 
> I'd like to have at least Dinh's/Chin's ack on this.

THanks Marek

Hmmm... From 11.15.4.1.1, the data slave port should able to accept
only byte, half-word and word access. This should not create any data
abort but probably bad performance. But it should insignificant as
access time for the flash is longer than the data port access itself.

Thanks
Chin Liang

> 
> btw don't you need BB for READ as well ?
> 
> > 
> > v2:
> >  - Use bounce buffer
> >  - Link to v1: https://patchwork.ozlabs.org/patch/693069/
> > 
> >  drivers/spi/cadence_qspi_apb.c   | 26 --
> >  include/configs/k2g_evm.h|  1 +
> >  include/configs/socfpga_common.h |  1 +
> >  include/configs/stv0991.h|  1 +
> >  4 files changed, 23 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/spi/cadence_qspi_apb.c
> > b/drivers/spi/cadence_qspi_apb.c
> > index e285d3c1e761..6ce98acf747d 100644
> > --- a/drivers/spi/cadence_qspi_apb.c
> > +++ b/drivers/spi/cadence_qspi_apb.c
> > @@ -30,6 +30,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #include "cadence_qspi.h"
> > 
> >  #define CQSPI_REG_POLL_US(1) /* 1us */
> > @@ -741,6 +742,17 @@ int
> > cadence_qspi_apb_indirect_write_execute(struct cadence_spi_platdata
> > *plat,
> >   unsigned int remaining = n_tx;
> >   unsigned int write_bytes;
> >   int ret;
> > + struct bounce_buffer bb;
> > + u8 *bb_txbuf;
> > +
> > + /*
> > +  * Handle non-4-byte aligned accesses via bounce buffer to
> > +  * avoid data abort.
> > +  */
> > + ret = bounce_buffer_start(, (void *)txbuf, n_tx,
> > GEN_BB_READ);
> > + if (ret)
> > + return ret;
> > + bb_txbuf = bb.bounce_buffer;
> > 
> >   /* Configure the indirect read transfer bytes */
> >   writel(n_tx, plat->regbase + CQSPI_REG_INDIRECTWRBYTES);
> > @@ -751,11 +763,11 @@ int
> > cadence_qspi_apb_indirect_write_execute(struct cadence_spi_platdata
> > *plat,
> > 
> >   while (remaining > 0) {
> >   write_bytes = remaining > page_size ? page_size :
> > remaining;
> > - /* Handle non-4-byte aligned access to avoid data
> > abort. */
> > - if (((uintptr_t)txbuf % 4) || (write_bytes % 4))
> > - writesb(plat->ahbbase, txbuf, write_bytes);
> > - else
> > - writesl(plat->ahbbase, txbuf, write_bytes >>
> > 2);
> > + writesl(plat->ahbbase, bb_txbuf, write_bytes >> 2);
> > + if (write_bytes % 4)
> > + writesb(plat->ahbbase,
> > + bb_txbuf + rounddown(write_bytes, 4),
> > + write_bytes % 4);
> > 
> >   ret = wait_for_bit("QSPI", plat->regbase +
> > CQSPI_REG_SDRAMLEVEL,
> >  CQSPI_REG_SDRAMLEVEL_WR_MASK <<
> > @@ -765,7 +777,7 @@ int
> > cadence_qspi_apb_indirect_write_execute(struct cadence_spi_platdata
> > *plat,
> >   goto failwr;
> >   }
> > 
> > - txbuf += write_bytes;
> > + bb_txbuf += write_bytes;
> >   remaining -= write_bytes;
> >   }
> > 
> > @@ -776,6 +788,7 @@ int
> > cadence_qspi_apb_indirect_write_execute(struct cadence_spi_platdata
> > *plat,
> >   printf("Indirect write completion error (%i)\n",
> > ret);
> >   goto failwr;
> >   }
> > + bounce_buffer_stop();
> > 
> >   /* Clear indirect completion status */
> >   writel(CQSPI_REG_INDIRECTWR_DONE_MASK,
> > @@ -786,6 +799,7 @@ failwr:
> >   /* Cancel the indirect write */
> >   writel(CQSPI_REG_INDIRECTWR_CANCEL_MASK,
> >  plat->regbase + CQSPI_REG_INDIRECTWR);
> > + bounce_buffer_stop();
> >   return ret;
> >  }
> > 
> > diff --git a/include/configs/k2g_evm.h b/include/configs/k2g_evm.h
> > index a14544526c71..1d603e0c002f 100644
> > --- a/include/configs/k2g_evm.h
> > +++ b/include/configs/k2g_evm.h
> > @@ -79,6 +79,7 @@
> >  #define CONFIG_CADENCE_QSPI
> >  #define CONFIG_CQSPI_REF_CLK 38400
> >  #define CONFIG_CQSPI_DECODER 0x0
> > +#define

Re: [U-Boot] [PATCH v2] spi: cadence_qspi_apb: Use 32 bit indirect write transaction when possible

2016-11-28 Thread Marek Vasut

On 11/28/2016 10:37 AM, Vignesh R wrote:
> 
> 
> On Friday 25 November 2016 10:21 PM, Marek Vasut wrote:
>> On 11/24/2016 06:35 AM, Vignesh R wrote:
>>> According to Section 11.15.4.9.2 Indirect Write Controller of K2G SoC
>>> TRM SPRUHY8D[1], the external master is only permitted to issue 32-bit
>>> data interface writes until the last word of an indirect transfer
>>> otherwise indirect writes is known to fails sometimes. So, make sure
>>> that QSPI indirect writes are 32 bit sized except for the last write. If
>>> the txbuf is unaligned then use bounce buffer to avoid data aborts.
>>>
>>> So, now that the driver uses bounce_buffer, enable CONFIG_BOUNCE_BUFFER
>>> for all boards that use Cadence QSPI driver.
>>>
>>> [1]www.ti.com/lit/ug/spruhy8d/spruhy8d.pdf
>>>
>>> Signed-off-by: Vignesh R 
>>> ---
>>
>> Reviewed-by: Marek Vasut 
>>
>> I'd like to have at least Dinh's/Chin's ack on this.
>>
>> btw don't you need BB for READ as well ?
>>
> 
> I don't see any issue with READ due to non word size accesses ATM,

Like user does sf read ... 0x1003 0x100 , you'll likely have a problem, no?

> anyways I am working on patch to use BB for READ. Will post that separately.
> 
> Thanks for the review!
> 


-- 
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v2] spi: cadence_qspi_apb: Use 32 bit indirect write transaction when possible

2016-11-28 Thread Vignesh R



On Friday 25 November 2016 11:18 PM, Jagan Teki wrote:
 >>> diff --git a/include/configs/k2g_evm.h b/include/configs/k2g_evm.h
 >>> index a14544526c71..1d603e0c002f 100644
 >>> --- a/include/configs/k2g_evm.h
 >>> +++ b/include/configs/k2g_evm.h
 >>> @@ -79,6 +79,7 @@
 >>>  #define CONFIG_CADENCE_QSPI
 >>>  #define CONFIG_CQSPI_REF_CLK 38400
 >>>  #define CONFIG_CQSPI_DECODER 0x0
 >>> +#define CONFIG_BOUNCE_BUFFER
>>> >>
>>> >> Better define this on Kconfig and add it on defconfig.
>> >
>> > Such change is unrelated to this patch and should be fixed in a
>> > separate/subsequent one.
> Agreed it should be a separate patch.

Yes, that should be separate series. There are a bunch of drivers and
couple of architectures using CONFIG_BOUNCE_BUFFER, hence the change is
not trivial.

-- 
Regards
Vignesh
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v2] spi: cadence_qspi_apb: Use 32 bit indirect write transaction when possible

2016-11-28 Thread Vignesh R



On Friday 25 November 2016 10:21 PM, Marek Vasut wrote:
> On 11/24/2016 06:35 AM, Vignesh R wrote:
>> According to Section 11.15.4.9.2 Indirect Write Controller of K2G SoC
>> TRM SPRUHY8D[1], the external master is only permitted to issue 32-bit
>> data interface writes until the last word of an indirect transfer
>> otherwise indirect writes is known to fails sometimes. So, make sure
>> that QSPI indirect writes are 32 bit sized except for the last write. If
>> the txbuf is unaligned then use bounce buffer to avoid data aborts.
>>
>> So, now that the driver uses bounce_buffer, enable CONFIG_BOUNCE_BUFFER
>> for all boards that use Cadence QSPI driver.
>>
>> [1]www.ti.com/lit/ug/spruhy8d/spruhy8d.pdf
>>
>> Signed-off-by: Vignesh R 
>> ---
> 
> Reviewed-by: Marek Vasut 
> 
> I'd like to have at least Dinh's/Chin's ack on this.
> 
> btw don't you need BB for READ as well ?
> 

I don't see any issue with READ due to non word size accesses ATM,
anyways I am working on patch to use BB for READ. Will post that separately.

Thanks for the review!

-- 
Regards
Vignesh
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v2] spi: cadence_qspi_apb: Use 32 bit indirect write transaction when possible

2016-11-25 Thread Jagan Teki

On Fri, Nov 25, 2016 at 11:16 PM, Marek Vasut  wrote:
> On 11/25/2016 06:42 PM, Jagan Teki wrote:
>> On Thu, Nov 24, 2016 at 11:05 AM, Vignesh R  wrote:
>>> According to Section 11.15.4.9.2 Indirect Write Controller of K2G SoC
>>> TRM SPRUHY8D[1], the external master is only permitted to issue 32-bit
>>> data interface writes until the last word of an indirect transfer
>>> otherwise indirect writes is known to fails sometimes. So, make sure
>>> that QSPI indirect writes are 32 bit sized except for the last write. If
>>> the txbuf is unaligned then use bounce buffer to avoid data aborts.
>>>
>>> So, now that the driver uses bounce_buffer, enable CONFIG_BOUNCE_BUFFER
>>> for all boards that use Cadence QSPI driver.
>>>
>>> [1]www.ti.com/lit/ug/spruhy8d/spruhy8d.pdf
>>>
>>> Signed-off-by: Vignesh R 
>>> ---
>>>
>>> v2:
>>>  - Use bounce buffer
>>>  - Link to v1: https://patchwork.ozlabs.org/patch/693069/
>>>
>>>  drivers/spi/cadence_qspi_apb.c   | 26 --
>>>  include/configs/k2g_evm.h|  1 +
>>>  include/configs/socfpga_common.h |  1 +
>>>  include/configs/stv0991.h|  1 +
>>>  4 files changed, 23 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/cadence_qspi_apb.c
>>> index e285d3c1e761..6ce98acf747d 100644
>>> --- a/drivers/spi/cadence_qspi_apb.c
>>> +++ b/drivers/spi/cadence_qspi_apb.c
>>> @@ -30,6 +30,7 @@
>>>  #include 
>>>  #include 
>>>  #include 
>>> +#include 
>>>  #include "cadence_qspi.h"
>>>
>>>  #define CQSPI_REG_POLL_US  (1) /* 1us */
>>> @@ -741,6 +742,17 @@ int cadence_qspi_apb_indirect_write_execute(struct 
>>> cadence_spi_platdata *plat,
>>> unsigned int remaining = n_tx;
>>> unsigned int write_bytes;
>>> int ret;
>>> +   struct bounce_buffer bb;
>>> +   u8 *bb_txbuf;
>>> +
>>> +   /*
>>> +* Handle non-4-byte aligned accesses via bounce buffer to
>>> +* avoid data abort.
>>> +*/
>>> +   ret = bounce_buffer_start(, (void *)txbuf, n_tx, GEN_BB_READ);
>>> +   if (ret)
>>> +   return ret;
>>> +   bb_txbuf = bb.bounce_buffer;
>>>
>>> /* Configure the indirect read transfer bytes */
>>> writel(n_tx, plat->regbase + CQSPI_REG_INDIRECTWRBYTES);
>>> @@ -751,11 +763,11 @@ int cadence_qspi_apb_indirect_write_execute(struct 
>>> cadence_spi_platdata *plat,
>>>
>>> while (remaining > 0) {
>>> write_bytes = remaining > page_size ? page_size : remaining;
>>> -   /* Handle non-4-byte aligned access to avoid data abort. */
>>> -   if (((uintptr_t)txbuf % 4) || (write_bytes % 4))
>>> -   writesb(plat->ahbbase, txbuf, write_bytes);
>>> -   else
>>> -   writesl(plat->ahbbase, txbuf, write_bytes >> 2);
>>> +   writesl(plat->ahbbase, bb_txbuf, write_bytes >> 2);
>>> +   if (write_bytes % 4)
>>> +   writesb(plat->ahbbase,
>>> +   bb_txbuf + rounddown(write_bytes, 4),
>>> +   write_bytes % 4);
>>>
>>> ret = wait_for_bit("QSPI", plat->regbase + 
>>> CQSPI_REG_SDRAMLEVEL,
>>>CQSPI_REG_SDRAMLEVEL_WR_MASK <<
>>> @@ -765,7 +777,7 @@ int cadence_qspi_apb_indirect_write_execute(struct 
>>> cadence_spi_platdata *plat,
>>> goto failwr;
>>> }
>>>
>>> -   txbuf += write_bytes;
>>> +   bb_txbuf += write_bytes;
>>> remaining -= write_bytes;
>>> }
>>>
>>> @@ -776,6 +788,7 @@ int cadence_qspi_apb_indirect_write_execute(struct 
>>> cadence_spi_platdata *plat,
>>> printf("Indirect write completion error (%i)\n", ret);
>>> goto failwr;
>>> }
>>> +   bounce_buffer_stop();
>>>
>>> /* Clear indirect completion status */
>>> writel(CQSPI_REG_INDIRECTWR_DONE_MASK,
>>> @@ -786,6 +799,7 @@ failwr:
>>> /* Cancel the indirect write */
>>> writel(CQSPI_REG_INDIRECTWR_CANCEL_MASK,
>>>plat->regbase + CQSPI_REG_INDIRECTWR);
>>> +   bounce_buffer_stop();
>>> return ret;
>>>  }
>>>
>>> diff --git a/include/configs/k2g_evm.h b/include/configs/k2g_evm.h
>>> index a14544526c71..1d603e0c002f 100644
>>> --- a/include/configs/k2g_evm.h
>>> +++ b/include/configs/k2g_evm.h
>>> @@ -79,6 +79,7 @@
>>>  #define CONFIG_CADENCE_QSPI
>>>  #define CONFIG_CQSPI_REF_CLK 38400
>>>  #define CONFIG_CQSPI_DECODER 0x0
>>> +#define CONFIG_BOUNCE_BUFFER
>>
>> Better define this on Kconfig and add it on defconfig.
>
> Such change is unrelated to this patch and should be fixed in a
> separate/subsequent one.

Agreed it should be a separate patch.

thanks!
-- 
Jagan Teki
Free Software Engineer | www.openedev.com
U-Boot, Linux | Upstream Maintainer
Hyderabad, India.

Re: [U-Boot] [PATCH v2] spi: cadence_qspi_apb: Use 32 bit indirect write transaction when possible

2016-11-25 Thread Marek Vasut

On 11/25/2016 06:42 PM, Jagan Teki wrote:
> On Thu, Nov 24, 2016 at 11:05 AM, Vignesh R  wrote:
>> According to Section 11.15.4.9.2 Indirect Write Controller of K2G SoC
>> TRM SPRUHY8D[1], the external master is only permitted to issue 32-bit
>> data interface writes until the last word of an indirect transfer
>> otherwise indirect writes is known to fails sometimes. So, make sure
>> that QSPI indirect writes are 32 bit sized except for the last write. If
>> the txbuf is unaligned then use bounce buffer to avoid data aborts.
>>
>> So, now that the driver uses bounce_buffer, enable CONFIG_BOUNCE_BUFFER
>> for all boards that use Cadence QSPI driver.
>>
>> [1]www.ti.com/lit/ug/spruhy8d/spruhy8d.pdf
>>
>> Signed-off-by: Vignesh R 
>> ---
>>
>> v2:
>>  - Use bounce buffer
>>  - Link to v1: https://patchwork.ozlabs.org/patch/693069/
>>
>>  drivers/spi/cadence_qspi_apb.c   | 26 --
>>  include/configs/k2g_evm.h|  1 +
>>  include/configs/socfpga_common.h |  1 +
>>  include/configs/stv0991.h|  1 +
>>  4 files changed, 23 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/cadence_qspi_apb.c
>> index e285d3c1e761..6ce98acf747d 100644
>> --- a/drivers/spi/cadence_qspi_apb.c
>> +++ b/drivers/spi/cadence_qspi_apb.c
>> @@ -30,6 +30,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>  #include "cadence_qspi.h"
>>
>>  #define CQSPI_REG_POLL_US  (1) /* 1us */
>> @@ -741,6 +742,17 @@ int cadence_qspi_apb_indirect_write_execute(struct 
>> cadence_spi_platdata *plat,
>> unsigned int remaining = n_tx;
>> unsigned int write_bytes;
>> int ret;
>> +   struct bounce_buffer bb;
>> +   u8 *bb_txbuf;
>> +
>> +   /*
>> +* Handle non-4-byte aligned accesses via bounce buffer to
>> +* avoid data abort.
>> +*/
>> +   ret = bounce_buffer_start(, (void *)txbuf, n_tx, GEN_BB_READ);
>> +   if (ret)
>> +   return ret;
>> +   bb_txbuf = bb.bounce_buffer;
>>
>> /* Configure the indirect read transfer bytes */
>> writel(n_tx, plat->regbase + CQSPI_REG_INDIRECTWRBYTES);
>> @@ -751,11 +763,11 @@ int cadence_qspi_apb_indirect_write_execute(struct 
>> cadence_spi_platdata *plat,
>>
>> while (remaining > 0) {
>> write_bytes = remaining > page_size ? page_size : remaining;
>> -   /* Handle non-4-byte aligned access to avoid data abort. */
>> -   if (((uintptr_t)txbuf % 4) || (write_bytes % 4))
>> -   writesb(plat->ahbbase, txbuf, write_bytes);
>> -   else
>> -   writesl(plat->ahbbase, txbuf, write_bytes >> 2);
>> +   writesl(plat->ahbbase, bb_txbuf, write_bytes >> 2);
>> +   if (write_bytes % 4)
>> +   writesb(plat->ahbbase,
>> +   bb_txbuf + rounddown(write_bytes, 4),
>> +   write_bytes % 4);
>>
>> ret = wait_for_bit("QSPI", plat->regbase + 
>> CQSPI_REG_SDRAMLEVEL,
>>CQSPI_REG_SDRAMLEVEL_WR_MASK <<
>> @@ -765,7 +777,7 @@ int cadence_qspi_apb_indirect_write_execute(struct 
>> cadence_spi_platdata *plat,
>> goto failwr;
>> }
>>
>> -   txbuf += write_bytes;
>> +   bb_txbuf += write_bytes;
>> remaining -= write_bytes;
>> }
>>
>> @@ -776,6 +788,7 @@ int cadence_qspi_apb_indirect_write_execute(struct 
>> cadence_spi_platdata *plat,
>> printf("Indirect write completion error (%i)\n", ret);
>> goto failwr;
>> }
>> +   bounce_buffer_stop();
>>
>> /* Clear indirect completion status */
>> writel(CQSPI_REG_INDIRECTWR_DONE_MASK,
>> @@ -786,6 +799,7 @@ failwr:
>> /* Cancel the indirect write */
>> writel(CQSPI_REG_INDIRECTWR_CANCEL_MASK,
>>plat->regbase + CQSPI_REG_INDIRECTWR);
>> +   bounce_buffer_stop();
>> return ret;
>>  }
>>
>> diff --git a/include/configs/k2g_evm.h b/include/configs/k2g_evm.h
>> index a14544526c71..1d603e0c002f 100644
>> --- a/include/configs/k2g_evm.h
>> +++ b/include/configs/k2g_evm.h
>> @@ -79,6 +79,7 @@
>>  #define CONFIG_CADENCE_QSPI
>>  #define CONFIG_CQSPI_REF_CLK 38400
>>  #define CONFIG_CQSPI_DECODER 0x0
>> +#define CONFIG_BOUNCE_BUFFER
> 
> Better define this on Kconfig and add it on defconfig.

Such change is unrelated to this patch and should be fixed in a
separate/subsequent one.


-- 
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v2] spi: cadence_qspi_apb: Use 32 bit indirect write transaction when possible

2016-11-25 Thread Jagan Teki

On Thu, Nov 24, 2016 at 11:05 AM, Vignesh R  wrote:
> According to Section 11.15.4.9.2 Indirect Write Controller of K2G SoC
> TRM SPRUHY8D[1], the external master is only permitted to issue 32-bit
> data interface writes until the last word of an indirect transfer
> otherwise indirect writes is known to fails sometimes. So, make sure
> that QSPI indirect writes are 32 bit sized except for the last write. If
> the txbuf is unaligned then use bounce buffer to avoid data aborts.
>
> So, now that the driver uses bounce_buffer, enable CONFIG_BOUNCE_BUFFER
> for all boards that use Cadence QSPI driver.
>
> [1]www.ti.com/lit/ug/spruhy8d/spruhy8d.pdf
>
> Signed-off-by: Vignesh R 
> ---
>
> v2:
>  - Use bounce buffer
>  - Link to v1: https://patchwork.ozlabs.org/patch/693069/
>
>  drivers/spi/cadence_qspi_apb.c   | 26 --
>  include/configs/k2g_evm.h|  1 +
>  include/configs/socfpga_common.h |  1 +
>  include/configs/stv0991.h|  1 +
>  4 files changed, 23 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/cadence_qspi_apb.c
> index e285d3c1e761..6ce98acf747d 100644
> --- a/drivers/spi/cadence_qspi_apb.c
> +++ b/drivers/spi/cadence_qspi_apb.c
> @@ -30,6 +30,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include "cadence_qspi.h"
>
>  #define CQSPI_REG_POLL_US  (1) /* 1us */
> @@ -741,6 +742,17 @@ int cadence_qspi_apb_indirect_write_execute(struct 
> cadence_spi_platdata *plat,
> unsigned int remaining = n_tx;
> unsigned int write_bytes;
> int ret;
> +   struct bounce_buffer bb;
> +   u8 *bb_txbuf;
> +
> +   /*
> +* Handle non-4-byte aligned accesses via bounce buffer to
> +* avoid data abort.
> +*/
> +   ret = bounce_buffer_start(, (void *)txbuf, n_tx, GEN_BB_READ);
> +   if (ret)
> +   return ret;
> +   bb_txbuf = bb.bounce_buffer;
>
> /* Configure the indirect read transfer bytes */
> writel(n_tx, plat->regbase + CQSPI_REG_INDIRECTWRBYTES);
> @@ -751,11 +763,11 @@ int cadence_qspi_apb_indirect_write_execute(struct 
> cadence_spi_platdata *plat,
>
> while (remaining > 0) {
> write_bytes = remaining > page_size ? page_size : remaining;
> -   /* Handle non-4-byte aligned access to avoid data abort. */
> -   if (((uintptr_t)txbuf % 4) || (write_bytes % 4))
> -   writesb(plat->ahbbase, txbuf, write_bytes);
> -   else
> -   writesl(plat->ahbbase, txbuf, write_bytes >> 2);
> +   writesl(plat->ahbbase, bb_txbuf, write_bytes >> 2);
> +   if (write_bytes % 4)
> +   writesb(plat->ahbbase,
> +   bb_txbuf + rounddown(write_bytes, 4),
> +   write_bytes % 4);
>
> ret = wait_for_bit("QSPI", plat->regbase + 
> CQSPI_REG_SDRAMLEVEL,
>CQSPI_REG_SDRAMLEVEL_WR_MASK <<
> @@ -765,7 +777,7 @@ int cadence_qspi_apb_indirect_write_execute(struct 
> cadence_spi_platdata *plat,
> goto failwr;
> }
>
> -   txbuf += write_bytes;
> +   bb_txbuf += write_bytes;
> remaining -= write_bytes;
> }
>
> @@ -776,6 +788,7 @@ int cadence_qspi_apb_indirect_write_execute(struct 
> cadence_spi_platdata *plat,
> printf("Indirect write completion error (%i)\n", ret);
> goto failwr;
> }
> +   bounce_buffer_stop();
>
> /* Clear indirect completion status */
> writel(CQSPI_REG_INDIRECTWR_DONE_MASK,
> @@ -786,6 +799,7 @@ failwr:
> /* Cancel the indirect write */
> writel(CQSPI_REG_INDIRECTWR_CANCEL_MASK,
>plat->regbase + CQSPI_REG_INDIRECTWR);
> +   bounce_buffer_stop();
> return ret;
>  }
>
> diff --git a/include/configs/k2g_evm.h b/include/configs/k2g_evm.h
> index a14544526c71..1d603e0c002f 100644
> --- a/include/configs/k2g_evm.h
> +++ b/include/configs/k2g_evm.h
> @@ -79,6 +79,7 @@
>  #define CONFIG_CADENCE_QSPI
>  #define CONFIG_CQSPI_REF_CLK 38400
>  #define CONFIG_CQSPI_DECODER 0x0
> +#define CONFIG_BOUNCE_BUFFER

Better define this on Kconfig and add it on defconfig.

thanks!
-- 
Jagan Teki
Free Software Engineer | www.openedev.com
U-Boot, Linux | Upstream Maintainer
Hyderabad, India.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v2] spi: cadence_qspi_apb: Use 32 bit indirect write transaction when possible

2016-11-25 Thread Marek Vasut

On 11/24/2016 06:35 AM, Vignesh R wrote:
> According to Section 11.15.4.9.2 Indirect Write Controller of K2G SoC
> TRM SPRUHY8D[1], the external master is only permitted to issue 32-bit
> data interface writes until the last word of an indirect transfer
> otherwise indirect writes is known to fails sometimes. So, make sure
> that QSPI indirect writes are 32 bit sized except for the last write. If
> the txbuf is unaligned then use bounce buffer to avoid data aborts.
> 
> So, now that the driver uses bounce_buffer, enable CONFIG_BOUNCE_BUFFER
> for all boards that use Cadence QSPI driver.
> 
> [1]www.ti.com/lit/ug/spruhy8d/spruhy8d.pdf
> 
> Signed-off-by: Vignesh R 
> ---

Reviewed-by: Marek Vasut 

I'd like to have at least Dinh's/Chin's ack on this.

btw don't you need BB for READ as well ?

> v2:
>  - Use bounce buffer
>  - Link to v1: https://patchwork.ozlabs.org/patch/693069/
> 
>  drivers/spi/cadence_qspi_apb.c   | 26 --
>  include/configs/k2g_evm.h|  1 +
>  include/configs/socfpga_common.h |  1 +
>  include/configs/stv0991.h|  1 +
>  4 files changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/cadence_qspi_apb.c
> index e285d3c1e761..6ce98acf747d 100644
> --- a/drivers/spi/cadence_qspi_apb.c
> +++ b/drivers/spi/cadence_qspi_apb.c
> @@ -30,6 +30,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include "cadence_qspi.h"
>  
>  #define CQSPI_REG_POLL_US(1) /* 1us */
> @@ -741,6 +742,17 @@ int cadence_qspi_apb_indirect_write_execute(struct 
> cadence_spi_platdata *plat,
>   unsigned int remaining = n_tx;
>   unsigned int write_bytes;
>   int ret;
> + struct bounce_buffer bb;
> + u8 *bb_txbuf;
> +
> + /*
> +  * Handle non-4-byte aligned accesses via bounce buffer to
> +  * avoid data abort.
> +  */
> + ret = bounce_buffer_start(, (void *)txbuf, n_tx, GEN_BB_READ);
> + if (ret)
> + return ret;
> + bb_txbuf = bb.bounce_buffer;
>  
>   /* Configure the indirect read transfer bytes */
>   writel(n_tx, plat->regbase + CQSPI_REG_INDIRECTWRBYTES);
> @@ -751,11 +763,11 @@ int cadence_qspi_apb_indirect_write_execute(struct 
> cadence_spi_platdata *plat,
>  
>   while (remaining > 0) {
>   write_bytes = remaining > page_size ? page_size : remaining;
> - /* Handle non-4-byte aligned access to avoid data abort. */
> - if (((uintptr_t)txbuf % 4) || (write_bytes % 4))
> - writesb(plat->ahbbase, txbuf, write_bytes);
> - else
> - writesl(plat->ahbbase, txbuf, write_bytes >> 2);
> + writesl(plat->ahbbase, bb_txbuf, write_bytes >> 2);
> + if (write_bytes % 4)
> + writesb(plat->ahbbase,
> + bb_txbuf + rounddown(write_bytes, 4),
> + write_bytes % 4);
>  
>   ret = wait_for_bit("QSPI", plat->regbase + CQSPI_REG_SDRAMLEVEL,
>  CQSPI_REG_SDRAMLEVEL_WR_MASK <<
> @@ -765,7 +777,7 @@ int cadence_qspi_apb_indirect_write_execute(struct 
> cadence_spi_platdata *plat,
>   goto failwr;
>   }
>  
> - txbuf += write_bytes;
> + bb_txbuf += write_bytes;
>   remaining -= write_bytes;
>   }
>  
> @@ -776,6 +788,7 @@ int cadence_qspi_apb_indirect_write_execute(struct 
> cadence_spi_platdata *plat,
>   printf("Indirect write completion error (%i)\n", ret);
>   goto failwr;
>   }
> + bounce_buffer_stop();
>  
>   /* Clear indirect completion status */
>   writel(CQSPI_REG_INDIRECTWR_DONE_MASK,
> @@ -786,6 +799,7 @@ failwr:
>   /* Cancel the indirect write */
>   writel(CQSPI_REG_INDIRECTWR_CANCEL_MASK,
>  plat->regbase + CQSPI_REG_INDIRECTWR);
> + bounce_buffer_stop();
>   return ret;
>  }
>  
> diff --git a/include/configs/k2g_evm.h b/include/configs/k2g_evm.h
> index a14544526c71..1d603e0c002f 100644
> --- a/include/configs/k2g_evm.h
> +++ b/include/configs/k2g_evm.h
> @@ -79,6 +79,7 @@
>  #define CONFIG_CADENCE_QSPI
>  #define CONFIG_CQSPI_REF_CLK 38400
>  #define CONFIG_CQSPI_DECODER 0x0
> +#define CONFIG_BOUNCE_BUFFER
>  #endif
>  
>  #endif /* __CONFIG_K2G_EVM_H */
> diff --git a/include/configs/socfpga_common.h 
> b/include/configs/socfpga_common.h
> index d37e5958b586..2de57b063334 100644
> --- a/include/configs/socfpga_common.h
> +++ b/include/configs/socfpga_common.h
> @@ -208,6 +208,7 @@ unsigned int cm_get_qspi_controller_clk_hz(void);
>  #define CONFIG_CQSPI_REF_CLK cm_get_qspi_controller_clk_hz()
>  #endif
>  #define CONFIG_CQSPI_DECODER 0
> +#define CONFIG_BOUNCE_BUFFER
>  
>  /*
>   * Designware SPI support
> diff --git a/include/configs/stv0991.h b/include/configs/stv0991.h
> index bfd1bd719285..09a3064bd6d6 100644
> ---

[U-Boot] [PATCH v2] spi: cadence_qspi_apb: Use 32 bit indirect write transaction when possible

2016-11-23 Thread Vignesh R

According to Section 11.15.4.9.2 Indirect Write Controller of K2G SoC
TRM SPRUHY8D[1], the external master is only permitted to issue 32-bit
data interface writes until the last word of an indirect transfer
otherwise indirect writes is known to fails sometimes. So, make sure
that QSPI indirect writes are 32 bit sized except for the last write. If
the txbuf is unaligned then use bounce buffer to avoid data aborts.

So, now that the driver uses bounce_buffer, enable CONFIG_BOUNCE_BUFFER
for all boards that use Cadence QSPI driver.

[1]www.ti.com/lit/ug/spruhy8d/spruhy8d.pdf

Signed-off-by: Vignesh R 
---

v2:
 - Use bounce buffer
 - Link to v1: https://patchwork.ozlabs.org/patch/693069/

 drivers/spi/cadence_qspi_apb.c   | 26 --
 include/configs/k2g_evm.h|  1 +
 include/configs/socfpga_common.h |  1 +
 include/configs/stv0991.h|  1 +
 4 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/cadence_qspi_apb.c
index e285d3c1e761..6ce98acf747d 100644
--- a/drivers/spi/cadence_qspi_apb.c
+++ b/drivers/spi/cadence_qspi_apb.c
@@ -30,6 +30,7 @@
 #include 
 #include 
 #include 
+#include 
 #include "cadence_qspi.h"
 
 #define CQSPI_REG_POLL_US  (1) /* 1us */
@@ -741,6 +742,17 @@ int cadence_qspi_apb_indirect_write_execute(struct 
cadence_spi_platdata *plat,
unsigned int remaining = n_tx;
unsigned int write_bytes;
int ret;
+   struct bounce_buffer bb;
+   u8 *bb_txbuf;
+
+   /*
+* Handle non-4-byte aligned accesses via bounce buffer to
+* avoid data abort.
+*/
+   ret = bounce_buffer_start(, (void *)txbuf, n_tx, GEN_BB_READ);
+   if (ret)
+   return ret;
+   bb_txbuf = bb.bounce_buffer;
 
/* Configure the indirect read transfer bytes */
writel(n_tx, plat->regbase + CQSPI_REG_INDIRECTWRBYTES);
@@ -751,11 +763,11 @@ int cadence_qspi_apb_indirect_write_execute(struct 
cadence_spi_platdata *plat,
 
while (remaining > 0) {
write_bytes = remaining > page_size ? page_size : remaining;
-   /* Handle non-4-byte aligned access to avoid data abort. */
-   if (((uintptr_t)txbuf % 4) || (write_bytes % 4))
-   writesb(plat->ahbbase, txbuf, write_bytes);
-   else
-   writesl(plat->ahbbase, txbuf, write_bytes >> 2);
+   writesl(plat->ahbbase, bb_txbuf, write_bytes >> 2);
+   if (write_bytes % 4)
+   writesb(plat->ahbbase,
+   bb_txbuf + rounddown(write_bytes, 4),
+   write_bytes % 4);
 
ret = wait_for_bit("QSPI", plat->regbase + CQSPI_REG_SDRAMLEVEL,
   CQSPI_REG_SDRAMLEVEL_WR_MASK <<
@@ -765,7 +777,7 @@ int cadence_qspi_apb_indirect_write_execute(struct 
cadence_spi_platdata *plat,
goto failwr;
}
 
-   txbuf += write_bytes;
+   bb_txbuf += write_bytes;
remaining -= write_bytes;
}
 
@@ -776,6 +788,7 @@ int cadence_qspi_apb_indirect_write_execute(struct 
cadence_spi_platdata *plat,
printf("Indirect write completion error (%i)\n", ret);
goto failwr;
}
+   bounce_buffer_stop();
 
/* Clear indirect completion status */
writel(CQSPI_REG_INDIRECTWR_DONE_MASK,
@@ -786,6 +799,7 @@ failwr:
/* Cancel the indirect write */
writel(CQSPI_REG_INDIRECTWR_CANCEL_MASK,
   plat->regbase + CQSPI_REG_INDIRECTWR);
+   bounce_buffer_stop();
return ret;
 }
 
diff --git a/include/configs/k2g_evm.h b/include/configs/k2g_evm.h
index a14544526c71..1d603e0c002f 100644
--- a/include/configs/k2g_evm.h
+++ b/include/configs/k2g_evm.h
@@ -79,6 +79,7 @@
 #define CONFIG_CADENCE_QSPI
 #define CONFIG_CQSPI_REF_CLK 38400
 #define CONFIG_CQSPI_DECODER 0x0
+#define CONFIG_BOUNCE_BUFFER
 #endif
 
 #endif /* __CONFIG_K2G_EVM_H */
diff --git a/include/configs/socfpga_common.h b/include/configs/socfpga_common.h
index d37e5958b586..2de57b063334 100644
--- a/include/configs/socfpga_common.h
+++ b/include/configs/socfpga_common.h
@@ -208,6 +208,7 @@ unsigned int cm_get_qspi_controller_clk_hz(void);
 #define CONFIG_CQSPI_REF_CLK   cm_get_qspi_controller_clk_hz()
 #endif
 #define CONFIG_CQSPI_DECODER   0
+#define CONFIG_BOUNCE_BUFFER
 
 /*
  * Designware SPI support
diff --git a/include/configs/stv0991.h b/include/configs/stv0991.h
index bfd1bd719285..09a3064bd6d6 100644
--- a/include/configs/stv0991.h
+++ b/include/configs/stv0991.h
@@ -74,6 +74,7 @@
 #ifdef CONFIG_OF_CONTROL   /* QSPI is controlled via DT */
 #define CONFIG_CQSPI_DECODER   0
 #define CONFIG_CQSPI_REF_CLK   ((30/4)/2)*1000*1000
+#define CONFIG_BOUNCE_BUFFER
 
 #endif
 
-- 
2.10.2

Re: [U-Boot] [PATCH v2] spi: cadence_qspi_apb: Use 32 bit indirect write transaction when possible

Re: [U-Boot] [PATCH v2] spi: cadence_qspi_apb: Use 32 bit indirect write transaction when possible

Re: [U-Boot] [PATCH v2] spi: cadence_qspi_apb: Use 32 bit indirect write transaction when possible

Re: [U-Boot] [PATCH v2] spi: cadence_qspi_apb: Use 32 bit indirect write transaction when possible

Re: [U-Boot] [PATCH v2] spi: cadence_qspi_apb: Use 32 bit indirect write transaction when possible

Re: [U-Boot] [PATCH v2] spi: cadence_qspi_apb: Use 32 bit indirect write transaction when possible

Re: [U-Boot] [PATCH v2] spi: cadence_qspi_apb: Use 32 bit indirect write transaction when possible

Re: [U-Boot] [PATCH v2] spi: cadence_qspi_apb: Use 32 bit indirect write transaction when possible

Re: [U-Boot] [PATCH v2] spi: cadence_qspi_apb: Use 32 bit indirect write transaction when possible

Re: [U-Boot] [PATCH v2] spi: cadence_qspi_apb: Use 32 bit indirect write transaction when possible

Re: [U-Boot] [PATCH v2] spi: cadence_qspi_apb: Use 32 bit indirect write transaction when possible

Re: [U-Boot] [PATCH v2] spi: cadence_qspi_apb: Use 32 bit indirect write transaction when possible

Re: [U-Boot] [PATCH v2] spi: cadence_qspi_apb: Use 32 bit indirect write transaction when possible

Re: [U-Boot] [PATCH v2] spi: cadence_qspi_apb: Use 32 bit indirect write transaction when possible

Re: [U-Boot] [PATCH v2] spi: cadence_qspi_apb: Use 32 bit indirect write transaction when possible

Re: [U-Boot] [PATCH v2] spi: cadence_qspi_apb: Use 32 bit indirect write transaction when possible

Re: [U-Boot] [PATCH v2] spi: cadence_qspi_apb: Use 32 bit indirect write transaction when possible

Re: [U-Boot] [PATCH v2] spi: cadence_qspi_apb: Use 32 bit indirect write transaction when possible

Re: [U-Boot] [PATCH v2] spi: cadence_qspi_apb: Use 32 bit indirect write transaction when possible

[U-Boot] [PATCH v2] spi: cadence_qspi_apb: Use 32 bit indirect write transaction when possible

20 matches

Site Navigation

Mail list logo

Footer information