Re: [U-Boot] cadence_qspi_apb: cache issues on mach-socfpga

[Rush, Jason A]

Goldschmidt Simon Wrote:
>Marek Vasut wrote:
 I don't believe the patchset I submitted for DT bindings were merged in.
>>>
>>> I can confirm that. I'd strongly vote for them to get in as cadence_qspi
>>> is otherwise not usable on mach socfpga.
>>>
>>> How can I ensure a tested-by from me gets related to that patch set?
>>> I can't reply to it as I was not subscribed to the list at that time.
>>
>> You should rebase that patch anyway and then resend it.
> 
> I could do that work but I guess it would be up to Jason to do that,
> regarding authorship?
> 
> Simon

I can probably rebase and send it again later this week.

-- Jason
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] cadence_qspi_apb: cache issues on mach-socfpga

[Rush, Jason A]

Goldschmidt Simon wrote:
> Hi,
> 
> I ran into the same issue with the cadence qspi driver and dcache as Jason
> reported (in febuary, I think - I started to monitor U-Boot in july only).
> 
> May I ask what's the status here? I do need fixes for this to keep
> mach-socfpga running with qspi. I currently set dcache off globally and
> it works, but I still need the trigger-address fix (which is fixed by Jason's
> patch to use linux DT bindings).

I don't believe the patchset I submitted for DT bindings were merged in.

> Regarding the bounce buffer change: isn't it wrong to use the generic bounce
> buffer here? Given the dcache calls in it, it seems like this one is for DMA
> but we're doing cpu copy in the cadence qspi driver.

I tend to agree for the socfpga architecture; however, I'm not very familiar 
with
the bounce buffer and dcache.  From what I recall, it looked like the data is 
copied
from the QSPI by the CPU on the socfpga so the data itself might be in the 
dcache?
Then the call to bounce_buffer_stop invalidates the dcache, so the QSPI data 
that
was there is now invalidated, and whatever random data in higher level memory
is copied over the QSPI data.

> Anything I can do to get this pushed?
> 
> Thanks,
> Simon

- Jason
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH 3/4] dts: k2g: Add trigger-address property to QSPI device

[Rush, Jason A.]

Add the 'cdns,trigger-address' property to the cadence QSPI device
node for Texas Instruments K2G SoC devices.

Signed-off-by: Jason A. Rush 
---
 arch/arm/dts/keystone-k2g.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/dts/keystone-k2g.dtsi b/arch/arm/dts/keystone-k2g.dtsi
index 191e3f167a..0380911f4d 100644
--- a/arch/arm/dts/keystone-k2g.dtsi
+++ b/arch/arm/dts/keystone-k2g.dtsi
@@ -98,6 +98,7 @@
num-cs = <4>;
fifo-depth = <256>;
sram-size = <256>;
+   cdns,trigger-address = <0x2400>;
status = "disabled";
};
 
-- 
2.11.0

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

[U-Boot] [PATCH 4/4] dts: stv0991: Add trigger-address property to QSPI device

[Rush, Jason A.]

Add the 'cdns,trigger-address' property to the cadence QSPI device
node for the ST STV0991 application board.

Signed-off-by: Jason A. Rush 
---
 arch/arm/dts/stv0991.dts | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/dts/stv0991.dts b/arch/arm/dts/stv0991.dts
index fa3fd641b2..6bc5372c7f 100644
--- a/arch/arm/dts/stv0991.dts
+++ b/arch/arm/dts/stv0991.dts
@@ -33,6 +33,7 @@
<0x4000 0x100>;
clocks = <375>;
sram-size = <256>;
+   cdns,trigger-address = <0x4000>;
status = "okay";
 
flash0: n25q32@0 {
-- 
2.11.0

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

[U-Boot] [PATCH 0/4] spi: cadence_spi: Add cdns, trigger-address DT property

[Rush, Jason A.]

This patch series addresses an issue with the indaddrtrig
register on the Cadence QSPI device being programmed with
the wrong value for the socfpga arch.  Adopting the
trigger-address DT bindings from the Linux kernel allows
the indaddrtrig register to be set independently from the
ahbbase as required by the socfpga arch.

This patch series is a subset of a previous patch series.
The subset was requested to be submitted as a separate
patch[1].

[1]https://lists.denx.de/pipermail/u-boot/2017-March/283897.html

Tested on Terasic SoCKit dev board (Altera Cyclone V)

Jason A. Rush (4):
  spi: cadence_spi: Add cdns,trigger-address DT property
  dts: socfpga: Add trigger-address property to QSPI device
  dts: k2g: Add trigger-address property to QSPI device
  dts: stv0991: Add trigger-address property to QSPI device

 arch/arm/dts/keystone-k2g.dtsi | 1 +
 arch/arm/dts/socfpga.dtsi  | 1 +
 arch/arm/dts/stv0991.dts   | 1 +
 drivers/spi/cadence_qspi.c | 2 ++
 drivers/spi/cadence_qspi.h | 1 +
 drivers/spi/cadence_qspi_apb.c | 4 ++--
 6 files changed, 8 insertions(+), 2 deletions(-)

-- 
2.11.0

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

[U-Boot] [PATCH 1/4] spi: cadence_spi: Add cdns, trigger-address DT property

[Rush, Jason A.]

The socfpga arch uses a different value for the indaddrtrig reg than
the ahbbase address. Adopting the cdns,trigger-address DT bindings
from the Linux kernel allows the trigger-address to be set correctly
on the socfpga arch.

Tested on Terasic SoCKit dev board (Altera Cyclone V)

Signed-off-by: Jason A. Rush 
---
 drivers/spi/cadence_qspi.c | 2 ++
 drivers/spi/cadence_qspi.h | 1 +
 drivers/spi/cadence_qspi_apb.c | 4 ++--
 3 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c
index 9a6e41f330..b7bed4d4fc 100644
--- a/drivers/spi/cadence_qspi.c
+++ b/drivers/spi/cadence_qspi.c
@@ -297,6 +297,8 @@ static int cadence_spi_ofdata_to_platdata(struct udevice 
*bus)
plat->regbase = (void *)data[0];
plat->ahbbase = (void *)data[2];
plat->sram_size = fdtdec_get_int(blob, node, "sram-size", 128);
+   plat->trigger_address = fdtdec_get_uint(blob, node,
+   "cdns,trigger-address", 0);
 
/* All other paramters are embedded in the child node */
subnode = fdt_first_subnode(blob, node);
diff --git a/drivers/spi/cadence_qspi.h b/drivers/spi/cadence_qspi.h
index d1927a4003..20fc7a7d9d 100644
--- a/drivers/spi/cadence_qspi.h
+++ b/drivers/spi/cadence_qspi.h
@@ -26,6 +26,7 @@ struct cadence_spi_platdata {
u32 tchsh_ns;
u32 tslch_ns;
u32 sram_size;
+   u32 trigger_address;
 };
 
 struct cadence_spi_priv {
diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/cadence_qspi_apb.c
index e02f2217f4..e4ad84c9b1 100644
--- a/drivers/spi/cadence_qspi_apb.c
+++ b/drivers/spi/cadence_qspi_apb.c
@@ -560,7 +560,7 @@ int cadence_qspi_apb_indirect_read_setup(struct 
cadence_spi_platdata *plat,
addr_bytes = cmdlen - 1;
 
/* Setup the indirect trigger address */
-   writel((u32)plat->ahbbase,
+   writel(plat->trigger_address,
   plat->regbase + CQSPI_REG_INDIRECTTRIGGER);
 
/* Configure the opcode */
@@ -710,7 +710,7 @@ int cadence_qspi_apb_indirect_write_setup(struct 
cadence_spi_platdata *plat,
return -EINVAL;
}
/* Setup the indirect trigger address */
-   writel((u32)plat->ahbbase,
+   writel(plat->trigger_address,
   plat->regbase + CQSPI_REG_INDIRECTTRIGGER);
 
/* Configure the opcode */
-- 
2.11.0

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

Re: [U-Boot] [PATCH v2 2/8] Revert "spi: cadence_qspi_apb: Use 32 bit indirect read transaction when possible"

[Rush, Jason A.]

Marek Vasut wrote:
> On 03/14/2017 03:23 PM, Rush, Jason A. wrote:
>> Marek Vasut wrote:
>>> On 03/07/2017 04:18 PM, Rush, Jason A. wrote:
>>>> Marek Vasut wrote:
>>>>> On 03/03/2017 04:17 PM, Rush, Jason A. wrote:
>>>>>> Marek Vasut wrote:
>>>>>>> On 03/01/2017 05:36 PM, Rush, Jason A. wrote:
>>>>>>>> This reverts commit b63b46313ed29e9b0c36b3d6b9407f6eade40c8f.
>>>>>>>>
>>>>>>>> The Cadence QSPI device does not work with caching (introduced with
>>>>>>>> the bounce buffer in this commit) on the Altera SoC platform.
>>>>>>>>
>>>>>>>> Signed-off-by: Jason A. Rush <jason.r...@gd-ms.com>
>>>>>>>
>>>>>>> Do we really need the reverts or can we just fix the commit(s) up 
>>>>>>> somehow ?
>>>>>>>
>>>>>>
>>>>>> Would you prefer I squash the 2 reverts and the subsequent patch together
>>>>>> as a single commit?
>>>>>
>>>>> I would prefer if you answered my question :) So let me re-iterate, can
>>>>> we incrementally fix the driver instead of doing the revert(s) ?
>>>>
>>>> I think I misunderstood your question.  Could you clarify what you mean by
>>>> incrementally fix the driver?
>>>
>>> That means change it to the final form without reverting patches.
>>
>> Yes, I can definitely change this so it does not revert the patches.
>
> OK
>
>>>> Are you asking if there is a way to fix the cache issue with the CQSPI on 
>>>> the
>>>> Altera SoC platform?  If so, I don't know if I have the knowledge to 
>>>> answer that.
>>>> Do you have any suggestions on where one would start looking to fix the
>>>> caching problem?
>>>
>>> Uh, there is a cache problem on socfpga ? Is this series working around
>>> it then ?
>>>
>>
>> Yes, there seems to be a cache issue with the CQSPI on the socfpga.  And yes,
>> this patch series is working around this cache issue.
>
> OK, that means I'm not accepting this series. Let's fix the cache issue
> properly.
>
>> In addition, this series
>> introduces the trigger-address DT that Linux introduced and is required by
>> the CQSPI on socfpga.
>
> Can we split this part from the patchset, so it can go in separately ?
>
>> Here's a quick recap of the cache issue and this patch series... The original
>> commits used a bounce buffer implementation to fix 32-bit read alignment
>> issues with the CQSPI on a TI SoC device.  The bounce buffer implementation
>> was a clean solution to the 32-bit alignment issue, but this implementation
>> returned random data from the QSPI flash on socfpga.  You suggested I try
>> disabling the dcache, as you recalled some caching problem on the CQSPI in
>> the past.  Running 'dcache off' solved the random data issue on socfpga
>> while using the bounce buffer implementation.
>>
>> That's when Vignesh and I decided to work around the cache problem by
>> reverting the two original commits and using a previous patch Vignesh had
>> written to fix the alignment problem.
>>
>> If I clean the series up to change it to the final form and not revert the
>> patches, is this an acceptable approach?
>
> No, if we know there is a bug and we only pile workarounds, we will end
> up with shitty code. Let's fix this properly please.
>

I agree a workaround is no good.  I have no knowledge or experience with
fixing a cache problem though, so I think someone else is going to have to
tackle this bug.

If you'd like, I can submit a separate patch for adopting the Linux DT
trigger-address property, but the CQSPI will still be broken for the socfpga
until someone fixes the cache problem with the CQSPI.

--
Regards,
Jason
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v2 2/8] Revert "spi: cadence_qspi_apb: Use 32 bit indirect read transaction when possible"

[Rush, Jason A.]

Marek Vasut wrote:
> On 03/07/2017 04:18 PM, Rush, Jason A. wrote:
>> Marek Vasut wrote:
>>> On 03/03/2017 04:17 PM, Rush, Jason A. wrote:
>>>> Marek Vasut wrote:
>>>>> On 03/01/2017 05:36 PM, Rush, Jason A. wrote:
>>>>>> This reverts commit b63b46313ed29e9b0c36b3d6b9407f6eade40c8f.
>>>>>>
>>>>>> The Cadence QSPI device does not work with caching (introduced with
>>>>>> the bounce buffer in this commit) on the Altera SoC platform.
>>>>>>
>>>>>> Signed-off-by: Jason A. Rush <jason.r...@gd-ms.com>
>>>>>
>>>>> Do we really need the reverts or can we just fix the commit(s) up somehow 
>>>>> ?
>>>>>
>>>>
>>>> Would you prefer I squash the 2 reverts and the subsequent patch together
>>>> as a single commit?
>>>
>>> I would prefer if you answered my question :) So let me re-iterate, can
>>> we incrementally fix the driver instead of doing the revert(s) ?
>>
>> I think I misunderstood your question.  Could you clarify what you mean by
>> incrementally fix the driver?
>
> That means change it to the final form without reverting patches.

Yes, I can definitely change this so it does not revert the patches.

>
>> Are you asking if there is a way to fix the cache issue with the CQSPI on the
>> Altera SoC platform?  If so, I don't know if I have the knowledge to answer 
>> that.
>> Do you have any suggestions on where one would start looking to fix the
>> caching problem?
>
> Uh, there is a cache problem on socfpga ? Is this series working around
> it then ?
>

Yes, there seems to be a cache issue with the CQSPI on the socfpga.  And yes,
this patch series is working around this cache issue.  In addition, this series
introduces the trigger-address DT that Linux introduced and is required by
the CQSPI on socfpga.

Here's a quick recap of the cache issue and this patch series... The original
commits used a bounce buffer implementation to fix 32-bit read alignment
issues with the CQSPI on a TI SoC device.  The bounce buffer implementation
was a clean solution to the 32-bit alignment issue, but this implementation
returned random data from the QSPI flash on socfpga.  You suggested I try
disabling the dcache, as you recalled some caching problem on the CQSPI in
the past.  Running 'dcache off' solved the random data issue on socfpga
while using the bounce buffer implementation.

That's when Vignesh and I decided to work around the cache problem by
reverting the two original commits and using a previous patch Vignesh had
written to fix the alignment problem.

If I clean the series up to change it to the final form and not revert the
patches, is this an acceptable approach?

--
Regards,
Jason

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

Re: [U-Boot] [PATCH v2 2/8] Revert "spi: cadence_qspi_apb: Use 32 bit indirect read transaction when possible"

[Rush, Jason A.]

Marek Vasut wrote:
> On 03/03/2017 04:17 PM, Rush, Jason A. wrote:
>> Marek Vasut wrote:
>>> On 03/01/2017 05:36 PM, Rush, Jason A. wrote:
>>>> This reverts commit b63b46313ed29e9b0c36b3d6b9407f6eade40c8f.
>>>>
>>>> The Cadence QSPI device does not work with caching (introduced with
>>>> the bounce buffer in this commit) on the Altera SoC platform.
>>>>
>>>> Signed-off-by: Jason A. Rush <jason.r...@gd-ms.com>
>>>
>>> Do we really need the reverts or can we just fix the commit(s) up somehow ?
>>>
>>
>> Would you prefer I squash the 2 reverts and the subsequent patch together
>> as a single commit?
> 
> I would prefer if you answered my question :) So let me re-iterate, can
> we incrementally fix the driver instead of doing the revert(s) ?

I think I misunderstood your question.  Could you clarify what you mean by
incrementally fix the driver?

Are you asking if there is a way to fix the cache issue with the CQSPI on the
Altera SoC platform?  If so, I don't know if I have the knowledge to answer 
that.
Do you have any suggestions on where one would start looking to fix the
caching problem?

Just so we have a common understanding, the reverts along with the
subsequent patches get the CQSPI device working again on the Altera SoC as
it used to in previous versions of U-Boot, and the CQSPI should continue to
work for the TI devices Vignesh's patches were intended to fix.

Regards,
Jason

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

Re: [U-Boot] [PATCH v2 2/8] Revert "spi: cadence_qspi_apb: Use 32 bit indirect read transaction when possible"

[Rush, Jason A.]

Marek Vasut wrote:
> On 03/01/2017 05:36 PM, Rush, Jason A. wrote:
>> This reverts commit b63b46313ed29e9b0c36b3d6b9407f6eade40c8f.
>>
>> The Cadence QSPI device does not work with caching (introduced with
>> the bounce buffer in this commit) on the Altera SoC platform.
>>
>> Signed-off-by: Jason A. Rush <jason.r...@gd-ms.com>
> 
> Do we really need the reverts or can we just fix the commit(s) up somehow ?
> 

Would you prefer I squash the 2 reverts and the subsequent patch together
as a single commit?

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

Re: [U-Boot] [PATCH v2 6/8] dts: socfpga: Add trigger-address property to QSPI device

[Rush, Jason A.]

Dinh Nguyen wrote:
> Hi Jason,
> 
> On 03/01/2017 10:38 AM, Rush, Jason A. wrote:
>> Add the 'cdns,trigger-address' property to the cadence QSPI device
>> node for Altera SoC devices.
>>
>> Signed-off-by: Jason A. Rush <jason.r...@gd-ms.com>
>> ---
>> Changed in v2:
>>  - renamed trigger-base to trigger-address
>>
>>  arch/arm/dts/socfpga.dtsi | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/arch/arm/dts/socfpga.dtsi b/arch/arm/dts/socfpga.dtsi
>> index 8588221e57..ac26d05722 100644
>> --- a/arch/arm/dts/socfpga.dtsi
>> +++ b/arch/arm/dts/socfpga.dtsi
>> @@ -646,6 +646,7 @@
>>  num-cs = <4>;
>>  fifo-depth = <128>;
>>  sram-size = <128>;
>> +trigger-address = <0x>;
> 
> According to the documentation I see, the indaddrtrig on SoCFPGA is at
> offset of 0x1C.
> 

Yes, the indaddrtrig register is at offset 0x1C, but the 'trigger-address'
property is the value written to that address.

In older versions of U-Boot (where the CQSPI worked) the value was
Written into the indaddrtrig register with the following code:

  ((u32)plat->ahbbase & CQSPI_INDIRECTTRIGGER_ADDR_MASK)

ahbbase is 0xffa0 and the mask is 0xF, which resulted in writing
a 0x0 into the indaddrtrig register.

Later changes to the CQSPI code removed that mask, so the indaddrtrig
register was programmed with 0xffa0 which resulted in the CPU
resetting whenever a read/write was performed.

You can see this same change has been incorporated into the 4.10 kernel:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/socfpga.dtsi?id=refs/tags/v4.10#n727
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/mtd/spi-nor/cadence-quadspi.c?id=refs/tags/v4.10#n1054

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

[U-Boot] [PATCH v2 8/8] dts: stv0991: Add trigger-address property to QSPI device

[Rush, Jason A.]

Add the 'cdns,trigger-address' property to the cadence QSPI device
node for the ST STV0991 application board.

Signed-off-by: Jason A. Rush 
---
Changed in v2:
 - renamed trigger-base to trigger-address

 arch/arm/dts/stv0991.dts | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/dts/stv0991.dts b/arch/arm/dts/stv0991.dts
index fa3fd641b2..f83b1efd9d 100644
--- a/arch/arm/dts/stv0991.dts
+++ b/arch/arm/dts/stv0991.dts
@@ -33,6 +33,7 @@
<0x4000 0x100>;
clocks = <375>;
sram-size = <256>;
+   trigger-address = <0x4000>;
status = "okay";
 
flash0: n25q32@0 {
-- 
2.11.0

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

[U-Boot] [PATCH v2 7/8] dts: k2g: Add trigger-address property to QSPI device

[Rush, Jason A.]

Add the 'cdns,trigger-address' property to the cadence QSPI device
node for Texas Instruments K2G SoC devices.

Signed-off-by: Jason A. Rush 
---
Changed in v2:
 - renamed trigger-base to trigger-address

 arch/arm/dts/keystone-k2g.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/dts/keystone-k2g.dtsi b/arch/arm/dts/keystone-k2g.dtsi
index 2193f9fa21..d49d419b05 100644
--- a/arch/arm/dts/keystone-k2g.dtsi
+++ b/arch/arm/dts/keystone-k2g.dtsi
@@ -95,6 +95,7 @@
num-cs = <4>;
fifo-depth = <256>;
sram-size = <256>;
+   trigger-address = <0x2400>;
status = "disabled";
};
 
-- 
2.11.0

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

[U-Boot] [PATCH v2 6/8] dts: socfpga: Add trigger-address property to QSPI device

[Rush, Jason A.]

Add the 'cdns,trigger-address' property to the cadence QSPI device
node for Altera SoC devices.

Signed-off-by: Jason A. Rush 
---
Changed in v2:
 - renamed trigger-base to trigger-address

 arch/arm/dts/socfpga.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/dts/socfpga.dtsi b/arch/arm/dts/socfpga.dtsi
index 8588221e57..ac26d05722 100644
--- a/arch/arm/dts/socfpga.dtsi
+++ b/arch/arm/dts/socfpga.dtsi
@@ -646,6 +646,7 @@
num-cs = <4>;
fifo-depth = <128>;
sram-size = <128>;
+   trigger-address = <0x>;
bus-num = <2>;
status = "disabled";
};
-- 
2.11.0

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

[U-Boot] [PATCH v2 5/8] spi: cadence_spi: Add cdns, trigger-address DT property

[Rush, Jason A.]

The socfpga arch uses a different value for the indaddrtrig reg than
the ahbbase address. Adopting the trigger-address DT bindings from
the Linux kernel allows the indaddrtrig reg to be set correctly on
the socfpga arch.

Tested on Terasic SoCKit dev board (Altera Cyclone V)

Signed-off-by: Jason A. Rush 
---
Changed in v2:
 - renamed DT property to trigger-address
 - load trigger-address as a u32

 drivers/spi/cadence_qspi.c | 1 +
 drivers/spi/cadence_qspi.h | 1 +
 drivers/spi/cadence_qspi_apb.c | 4 ++--
 3 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c
index 09f2ec3528..92d08eb3c6 100644
--- a/drivers/spi/cadence_qspi.c
+++ b/drivers/spi/cadence_qspi.c
@@ -300,6 +300,7 @@ static int cadence_spi_ofdata_to_platdata(struct udevice 
*bus)
}
 
plat->sram_size = fdtdec_get_int(blob, node, "sram-size", 128);
+   plat->trigger_address = fdtdec_get_uint(blob, node, "trigger-address", 
0);
 
/* All other paramters are embedded in the child node */
subnode = fdt_first_subnode(blob, node);
diff --git a/drivers/spi/cadence_qspi.h b/drivers/spi/cadence_qspi.h
index da21b1346d..d607974cbe 100644
--- a/drivers/spi/cadence_qspi.h
+++ b/drivers/spi/cadence_qspi.h
@@ -26,6 +26,7 @@ struct cadence_spi_platdata {
u32 tchsh_ns;
u32 tslch_ns;
u32 sram_size;
+   u32 trigger_address;
 };
 
 struct cadence_spi_priv {
diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/cadence_qspi_apb.c
index 52ec892c4a..89fa3eaa21 100644
--- a/drivers/spi/cadence_qspi_apb.c
+++ b/drivers/spi/cadence_qspi_apb.c
@@ -559,7 +559,7 @@ int cadence_qspi_apb_indirect_read_setup(struct 
cadence_spi_platdata *plat,
addr_bytes = cmdlen - 1;
 
/* Setup the indirect trigger address */
-   writel((u32)plat->ahbbase,
+   writel(plat->trigger_address,
   plat->regbase + CQSPI_REG_INDIRECTTRIGGER);
 
/* Configure the opcode */
@@ -699,7 +699,7 @@ int cadence_qspi_apb_indirect_write_setup(struct 
cadence_spi_platdata *plat,
return -EINVAL;
}
/* Setup the indirect trigger address */
-   writel((u32)plat->ahbbase,
+   writel(plat->trigger_address,
   plat->regbase + CQSPI_REG_INDIRECTTRIGGER);
 
/* Configure the opcode */
-- 
2.11.0

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

[U-Boot] [PATCH v2 4/8] spi: cadence_qspi: Cleanup register loading/accesses

[Rush, Jason A.]

Load the regbase/ahbbase 'reg' DT properties using the standard
dev_get_addr_index function, and add __iomem to all register variables
declarations.

Signed-off-by: Jason A. Rush 
---
Changed in v2: None

 drivers/spi/cadence_qspi.c | 25 ++---
 drivers/spi/cadence_qspi.h | 28 ++--
 drivers/spi/cadence_qspi_apb.c | 24 
 3 files changed, 40 insertions(+), 37 deletions(-)

diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c
index 9a6e41f330..09f2ec3528 100644
--- a/drivers/spi/cadence_qspi.c
+++ b/drivers/spi/cadence_qspi.c
@@ -40,7 +40,7 @@ static int cadence_spi_write_speed(struct udevice *bus, uint 
hz)
 static int spi_calibration(struct udevice *bus, uint hz)
 {
struct cadence_spi_priv *priv = dev_get_priv(bus);
-   void *base = priv->regbase;
+   void __iomem *base = priv->regbase;
u8 opcode_rdid = 0x9F;
unsigned int idcode = 0, temp = 0;
int err = 0, i, range_lo = -1, range_hi = -1;
@@ -190,7 +190,7 @@ static int cadence_spi_xfer(struct udevice *dev, unsigned 
int bitlen,
struct cadence_spi_platdata *plat = bus->platdata;
struct cadence_spi_priv *priv = dev_get_priv(bus);
struct dm_spi_slave_platdata *dm_plat = dev_get_parent_platdata(dev);
-   void *base = priv->regbase;
+   void __iomem *base = priv->regbase;
u8 *cmd_buf = priv->cmd_buf;
size_t data_bytes;
int err = 0;
@@ -284,18 +284,21 @@ static int cadence_spi_ofdata_to_platdata(struct udevice 
*bus)
const void *blob = gd->fdt_blob;
int node = dev_of_offset(bus);
int subnode;
-   u32 data[4];
-   int ret;
 
-   /* 2 base addresses are needed, lets get them from the DT */
-   ret = fdtdec_get_int_array(blob, node, "reg", data, ARRAY_SIZE(data));
-   if (ret) {
-   printf("Error: Can't get base addresses (ret=%d)!\n", ret);
-   return -ENODEV;
+   /* Load the regbase from the first 'reg' property */
+   plat->regbase = (void __iomem *)dev_get_addr_index(bus, 0);
+   if (IS_ERR(plat->regbase)) {
+   printf("Error: Can't get regbase address!\n");
+   return PTR_ERR(plat->regbase);
+   }
+
+   /* Load the ahbbase from the second 'reg' property */
+   plat->ahbbase = (void __iomem *)dev_get_addr_index(bus, 1);
+   if (IS_ERR(plat->ahbbase)) {
+   printf("Error: Can't get ahbbase address!\n");
+   return PTR_ERR(plat->ahbbase);
}
 
-   plat->regbase = (void *)data[0];
-   plat->ahbbase = (void *)data[2];
plat->sram_size = fdtdec_get_int(blob, node, "sram-size", 128);
 
/* All other paramters are embedded in the child node */
diff --git a/drivers/spi/cadence_qspi.h b/drivers/spi/cadence_qspi.h
index d1927a4003..da21b1346d 100644
--- a/drivers/spi/cadence_qspi.h
+++ b/drivers/spi/cadence_qspi.h
@@ -16,8 +16,8 @@
 
 struct cadence_spi_platdata {
unsigned intmax_hz;
-   void*regbase;
-   void*ahbbase;
+   void __iomem*regbase;
+   void __iomem*ahbbase;
 
u32 page_size;
u32 block_size;
@@ -29,8 +29,8 @@ struct cadence_spi_platdata {
 };
 
 struct cadence_spi_priv {
-   void*regbase;
-   void*ahbbase;
+   void __iomem*regbase;
+   void __iomem*ahbbase;
size_t  cmd_len;
u8  cmd_buf[32];
size_t  data_len;
@@ -43,12 +43,12 @@ struct cadence_spi_priv {
 
 /* Functions call declaration */
 void cadence_qspi_apb_controller_init(struct cadence_spi_platdata *plat);
-void cadence_qspi_apb_controller_enable(void *reg_base_addr);
-void cadence_qspi_apb_controller_disable(void *reg_base_addr);
+void cadence_qspi_apb_controller_enable(void __iomem *reg_base_addr);
+void cadence_qspi_apb_controller_disable(void __iomem *reg_base_addr);
 
-int cadence_qspi_apb_command_read(void *reg_base_addr,
+int cadence_qspi_apb_command_read(void __iomem *reg_base_addr,
unsigned int cmdlen, const u8 *cmdbuf, unsigned int rxlen, u8 *rxbuf);
-int cadence_qspi_apb_command_write(void *reg_base_addr,
+int cadence_qspi_apb_command_write(void __iomem *reg_base_addr,
unsigned int cmdlen, const u8 *cmdbuf,
unsigned int txlen,  const u8 *txbuf);
 
@@ -61,17 +61,17 @@ int cadence_qspi_apb_indirect_write_setup(struct 
cadence_spi_platdata *plat,
 int cadence_qspi_apb_indirect_write_execute(struct cadence_spi_platdata *plat,
unsigned int txlen, const u8 *txbuf);
 
-void cadence_qspi_apb_chipselect(void *reg_base,
+void cadence_qspi_apb_chipselect(void __iomem *reg_base,
unsigned int chip_select, unsigned int decoder_enable);
-void cadence_qspi_apb_set_clk_mode(void *reg_base, uint mode);
-void cadence_qspi_apb_config_baudrate_div(void *reg_base,
+void 

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

[Rush, Jason A.]

From: 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.

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

Signed-off-by: Vignesh R 
Signed-off-by: Jason A. Rush 
---
Changed in v2:
 - changed reg name to match current reg names

 drivers/spi/cadence_qspi_apb.c | 41 +++--
 1 file changed, 31 insertions(+), 10 deletions(-)

diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/cadence_qspi_apb.c
index df6a91fc9f..907c6edf0a 100644
--- a/drivers/spi/cadence_qspi_apb.c
+++ b/drivers/spi/cadence_qspi_apb.c
@@ -722,23 +722,44 @@ int cadence_qspi_apb_indirect_write_execute(struct 
cadence_spi_platdata *plat,
 {
unsigned int page_size = plat->page_size;
unsigned int remaining = n_tx;
-   unsigned int write_bytes;
+   unsigned int write_bytes = 0;
int ret;
 
+   /* Handle non-4-byte aligned access to avoid data abort. */
+   if ((uintptr_t) txbuf % 4) {
+   write_bytes = n_tx > 3 ? 4 - ((uintptr_t)txbuf % 4) : n_tx;
+   /* Configure the indirect read transfer bytes */
+   writel(write_bytes, plat->regbase + CQSPI_REG_INDIRECTWRBYTES);
+   /* Start the indirect write transfer */
+   writel(CQSPI_REG_INDIRECTWR_START,
+  plat->regbase + CQSPI_REG_INDIRECTWR);
+   writesb(plat->ahbbase, txbuf, write_bytes);
+   txbuf += write_bytes;
+   remaining -= write_bytes;
+   if (remaining > 0) {
+   u32 start_addr = readl(plat->regbase +
+  CQSPI_REG_INDIRECTWRSTARTADDR);
+   start_addr += write_bytes;
+   writel(start_addr, plat->regbase +
+  CQSPI_REG_INDIRECTWRSTARTADDR);
+   }
+   }
+
/* Configure the indirect read transfer bytes */
-   writel(n_tx, plat->regbase + CQSPI_REG_INDIRECTWRBYTES);
+   writel(n_tx - write_bytes, plat->regbase + CQSPI_REG_INDIRECTWRBYTES);
 
-   /* Start the indirect write transfer */
-   writel(CQSPI_REG_INDIRECTWR_START,
-  plat->regbase + CQSPI_REG_INDIRECTWR);
+   if (remaining > 0)
+   writel(CQSPI_REG_INDIRECTWR_START,
+  plat->regbase + CQSPI_REG_INDIRECTWR);
 
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, txbuf, write_bytes >> 2);
+   if (write_bytes % 4)
+   writesb(plat->ahbbase,
+   txbuf + rounddown(write_bytes, 4),
+   write_bytes % 4);
 
ret = wait_for_bit("QSPI", plat->regbase + CQSPI_REG_SDRAMLEVEL,
   CQSPI_REG_SDRAMLEVEL_WR_MASK <<
-- 
2.11.0

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

[U-Boot] [PATCH v2 2/8] Revert "spi: cadence_qspi_apb: Use 32 bit indirect read transaction when possible"

[Rush, Jason A.]

This reverts commit b63b46313ed29e9b0c36b3d6b9407f6eade40c8f.

The Cadence QSPI device does not work with caching (introduced with
the bounce buffer in this commit) on the Altera SoC platform.

Signed-off-by: Jason A. Rush 
---
Changed in v2: None

 drivers/spi/cadence_qspi_apb.c | 22 ++
 1 file changed, 6 insertions(+), 16 deletions(-)

diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/cadence_qspi_apb.c
index a6787c3b47..df6a91fc9f 100644
--- a/drivers/spi/cadence_qspi_apb.c
+++ b/drivers/spi/cadence_qspi_apb.c
@@ -633,8 +633,6 @@ int cadence_qspi_apb_indirect_read_execute(struct 
cadence_spi_platdata *plat,
 {
unsigned int remaining = n_rx;
unsigned int bytes_to_read = 0;
-   struct bounce_buffer bb;
-   u8 *bb_rxbuf;
int ret;
 
writel(n_rx, plat->regbase + CQSPI_REG_INDIRECTRDBYTES);
@@ -643,11 +641,6 @@ int cadence_qspi_apb_indirect_read_execute(struct 
cadence_spi_platdata *plat,
writel(CQSPI_REG_INDIRECTRD_START,
   plat->regbase + CQSPI_REG_INDIRECTRD);
 
-   ret = bounce_buffer_start(, (void *)rxbuf, n_rx, GEN_BB_WRITE);
-   if (ret)
-   return ret;
-   bb_rxbuf = bb.bounce_buffer;
-
while (remaining > 0) {
ret = cadence_qspi_wait_for_data(plat);
if (ret < 0) {
@@ -661,13 +654,12 @@ int cadence_qspi_apb_indirect_read_execute(struct 
cadence_spi_platdata *plat,
bytes_to_read *= CQSPI_FIFO_WIDTH;
bytes_to_read = bytes_to_read > remaining ?
remaining : bytes_to_read;
-   readsl(plat->ahbbase, bb_rxbuf, bytes_to_read >> 2);
-   if (bytes_to_read % 4)
-   readsb(plat->ahbbase,
-  bb_rxbuf + rounddown(bytes_to_read, 4),
-  bytes_to_read % 4);
-
-   bb_rxbuf += bytes_to_read;
+   /* Handle non-4-byte aligned access to avoid data 
abort. */
+   if (((uintptr_t)rxbuf % 4) || (bytes_to_read % 4))
+   readsb(plat->ahbbase, rxbuf, bytes_to_read);
+   else
+   readsl(plat->ahbbase, rxbuf, bytes_to_read >> 
2);
+   rxbuf += bytes_to_read;
remaining -= bytes_to_read;
bytes_to_read = cadence_qspi_get_rd_sram_level(plat);
}
@@ -684,7 +676,6 @@ int cadence_qspi_apb_indirect_read_execute(struct 
cadence_spi_platdata *plat,
/* Clear indirect completion status */
writel(CQSPI_REG_INDIRECTRD_DONE,
   plat->regbase + CQSPI_REG_INDIRECTRD);
-   bounce_buffer_stop();
 
return 0;
 
@@ -692,7 +683,6 @@ failrd:
/* Cancel the indirect read */
writel(CQSPI_REG_INDIRECTRD_CANCEL,
   plat->regbase + CQSPI_REG_INDIRECTRD);
-   bounce_buffer_stop();
return ret;
 }
 
-- 
2.11.0

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

[U-Boot] [PATCH v2 1/8] Revert "spi: cadence_qspi_apb: Use 32 bit indirect write transaction when possible"

[Rush, Jason A.]

This reverts commit 57897c13de03ac0136d64641a3eab526c6810387.

The Cadence QSPI device does not work with caching (introduced with
the bounce buffer in this commit) on the Altera SoC platform.

Signed-off-by: Jason A. Rush 
---
Changed in v2: None

 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, 6 insertions(+), 23 deletions(-)

diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/cadence_qspi_apb.c
index e02f2217f4..a6787c3b47 100644
--- a/drivers/spi/cadence_qspi_apb.c
+++ b/drivers/spi/cadence_qspi_apb.c
@@ -30,7 +30,6 @@
 #include 
 #include 
 #include 
-#include 
 #include "cadence_qspi.h"
 
 #define CQSPI_REG_POLL_US  1 /* 1us */
@@ -735,17 +734,6 @@ 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);
@@ -756,11 +744,11 @@ int cadence_qspi_apb_indirect_write_execute(struct 
cadence_spi_platdata *plat,
 
while (remaining > 0) {
write_bytes = remaining > page_size ? page_size : remaining;
-   writesl(plat->ahbbase, bb_txbuf, write_bytes >> 2);
-   if (write_bytes % 4)
-   writesb(plat->ahbbase,
-   bb_txbuf + rounddown(write_bytes, 4),
-   write_bytes % 4);
+   /* 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);
 
ret = wait_for_bit("QSPI", plat->regbase + CQSPI_REG_SDRAMLEVEL,
   CQSPI_REG_SDRAMLEVEL_WR_MASK <<
@@ -770,7 +758,7 @@ int cadence_qspi_apb_indirect_write_execute(struct 
cadence_spi_platdata *plat,
goto failwr;
}
 
-   bb_txbuf += write_bytes;
+   txbuf += write_bytes;
remaining -= write_bytes;
}
 
@@ -781,7 +769,6 @@ 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,
@@ -792,7 +779,6 @@ failwr:
/* Cancel the indirect write */
writel(CQSPI_REG_INDIRECTWR_CANCEL,
   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 bd252312a2..a5e72a331b 100644
--- a/include/configs/k2g_evm.h
+++ b/include/configs/k2g_evm.h
@@ -73,7 +73,6 @@
 #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 582b04af3d..a3fa28c791 100644
--- a/include/configs/socfpga_common.h
+++ b/include/configs/socfpga_common.h
@@ -200,7 +200,6 @@ 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 24dd81856b..5d180d7419 100644
--- a/include/configs/stv0991.h
+++ b/include/configs/stv0991.h
@@ -71,7 +71,6 @@
 #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.11.0

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

[U-Boot] [PATCH v2 0/8] spi: cadence: Fix read/write on socfpga

[Rush, Jason A.]

This patch series addresses two problems with reads/writes not
working with the Cadence QSPI device on the socfpga arch.

The first issue is reads/writes to the Cadence QSPI device
do not work correctly with caching enabled on the socfpga.
This problem was introduced with previous commits that use
a bounce buffer to support unaligned read/writes. Patches 1-3
address this issue by replacing the bounce buffer with a
non-cached version.

The second issue with the Cadence QSPI device on the socfpga
device is that the indaddrtrig register needs a different value
than the ahbbase address.  Adopting the trigger-address DT
bindings from the Linux kernel allows the indaddrtrig reg to
be set correctly for the socfpga arch.

Jason A. Rush (7):
  Revert "spi: cadence_qspi_apb: Use 32 bit indirect write transaction
when possible"
  Revert "spi: cadence_qspi_apb: Use 32 bit indirect read transaction
when possible"
  spi: cadence_qspi: Cleanup register loading/accesses
  spi: cadence_spi: Add cdns,trigger-address DT property
  dts: socfpga: Add trigger-address property to QSPI device
  dts: k2g: Add trigger-address property to QSPI device
  dts: stv0991: Add trigger-address property to QSPI device

Vignesh R (1):
  spi: cadence_qspi_apb: Use 32 bit indirect write transaction when
possible

 arch/arm/dts/keystone-k2g.dtsi   |  1 +
 arch/arm/dts/socfpga.dtsi|  1 +
 arch/arm/dts/stv0991.dts |  1 +
 drivers/spi/cadence_qspi.c   | 26 ++-
 drivers/spi/cadence_qspi.h   | 29 ++--
 drivers/spi/cadence_qspi_apb.c   | 99 +++-
 include/configs/k2g_evm.h|  1 -
 include/configs/socfpga_common.h |  1 -
 include/configs/stv0991.h|  1 -
 9 files changed, 81 insertions(+), 79 deletions(-)

-- 
2.11.0

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

Re: [U-Boot] [PATCH] spi: cadence_qspi_apb: Add trigger-base DT bindings from Linux

[Rush, Jason A.]

R, Vignesh wrote:
> On 2/28/2017 8:38 PM, Rush, Jason A. wrote:
> [...]
>>
>> This also works.
>>
>> Marek - how do you feel about a patch series with the following:
>>
>> 1. revert commit 57897c13de03ac0136d64641a3eab526c6810387
>>  spi: cadence_qspi_apb: Use 32 bit indirect write transaction when 
>> possible
>> 2. revert commit b63b46313ed29e9b0c36b3d6b9407f6eade40c8f
>>  spi: cadence_qspi_apb: Use 32 bit indirect read transaction when 
>> possible
>> 3. Apply my slightly modified version of 
>> https://patchwork.ozlabs.org/patch/693069/
>>  (should I keep Vignesh as the signed-off?)
> 
> Depends on how much you have changed the code. If the change is
> significant then change the authorship to you and drop my signed-off.
> Else, keep the authorship and signed-off.
> If you are adding something new to the patch like adding code to make
> sure that only 32bit data reads are issued, then I suggest you to submit
> that change as separate patch.

Very minimal.  Another commit changed a #define from
CQSPI_REG_INDIRECTWR_START_MASK to CQSPI_REG_INDIRECTWR_START,
so I had to modify the patch to drop the _MASK so it would apply.  Other
than that, it's identical in content.

> 
>> 4. Clean-up loading the reg variables to use fdtdec_get_addr_xxx(...) and 
>> mark
>>  them all as __iomem
>> 5. Load the indirect-trigger property from the DT as a u32.  I think this 
>> still
>>  needs to be a u32 because it's used in a writel(...) 32-bit write call 
>> in
>>  cadence_qspi_apb.c
>> 6. Add indirect-trigger to dts files that use cadence driver
>>
>> I think that captures all the changes needed.
>>
>> Also, for the naming of the DT property, Linux has uses a 'cdns,' prefix
>> (e.g. cdns,trigger-address) for a number of their properties (cdns,tshsl-ns,
>> cdns,tsd2d-ns, etc.), but u-boot does not currently use the 'cdns,' prefix 
>> for
>> the same properties.  Do you want the 'cdns,' prefix on trigger-address?
>> If so, do you want me to rename all the other properties to align them with
>> the Linux DT as an additional patch?
>>

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

Re: [U-Boot] [PATCH] spi: cadence_qspi_apb: Add trigger-base DT bindings from Linux

[Rush, Jason A.]

I don't know if this message successfully sent last weekend.  My mail server 
was undergoing maintenance, and I didn't see my original message on the u-boot 
mailing list archive.  So I'm resending, my apologies if this is a repost.

R, Vignesh wrote:
> On 2/25/2017 1:25 AM, Rush, Jason A. wrote:
>> R, Vignesh wrote:
>>> On 2/24/2017 12:55 AM, Marek Vasut wrote:
>>>> On 02/23/2017 08:22 PM, Rush, Jason A. wrote:
>>>>> Marek Vasut wrote:
>>>>>> On 02/22/2017 06:37 PM, Rush, Jason A. wrote:
>>>>>>> Marek Vasut wrote:
>>>>>>>> On 02/21/2017 05:50 PM, Rush, Jason A. wrote:
>>> [...]
>>>>>
>>>
>>> You could try reverting my commits:
>>> commit 57897c13de03ac0136d64641a3eab526c6810387 spi: cadence_qspi_apb:
>>> Use 32 bit indirect write transaction when possible
>>> commit b63b46313ed29e9b0c36b3d6b9407f6eade40c8f spi: cadence_qspi_apb:
>>> Use 32 bit indirect read transaction when possible
>>>
>>
>> When I reverted these two commits and added my patch for the indirect
>> trigger_address, it works correctly.
>>
> 
> Oops, these patches are required as Cadence QSPI controller(I am not
> sure if all versions of IP are newer versions only) has a limitation
> that the external master is only permitted to issue 32-bit data
> interface reads until the last word of an indirect transfer.
> 
> 
>> Also, when I disabled the dcache (dcache off) as Marek suggested, it works
>> correctly when running from the master branch (again with my indirect
>> trigger_address patch).
>>
> 
> Just that I understand correctly, with latest master(with no patches
> reverted) + your patch for indirect trigger_address + dcache off, you
> don't see any problem?
> 

Correct.

>>>
>>> But there were other patches by others in v2017.01-rc1, like
>>> spi: cadence_qspi: Fix CS timings which may have impact.
>>>
>>
>> I left all other commits in except the two Vignesh suggested to revert, so
>> it seems to be related to those two commits and caching.  As another data
>> point, I can load and boot linux with caching on from another source (MMC).
>> So I don't think it's a problem with memory/caching in general.
>>
>> Any suggestions on how to proceed from here?
>>
> 
> My patches use common bounce_buffer implementation which does dcache
> flush/invalidate and if dcache has issues then I guess those operations
> may be causing data corruption. Could you do a bit more research for me?
> 
> 1. As a hack, could you just disable dcache operations in bounce_buffer
> implementation? Here is the diff for the same:
> 

This works.  So it does seem to be an issue with the CQSPI and dcache on
the Altera SoC.

> 
> diff --git a/common/bouncebuf.c b/common/bouncebuf.c
> index 054d9e0302cc..2878b9eed1ae 100644
> --- a/common/bouncebuf.c
> +++ b/common/bouncebuf.c
[...]
> 
> 2. If that works, I guess there is some issue wrt CQSPI and dcache on your 
> platform,
> I suggest you to revert my above two patches and try non bounce buffer 
> version of
> my changes here: https://patchwork.ozlabs.org/patch/693069/.
> This patch takes care of indirect write. I don't have similar patch for
> indirect read but that wasn't required.
> 

This also works.

Marek - how do you feel about a patch series with the following:

1. revert commit 57897c13de03ac0136d64641a3eab526c6810387
 spi: cadence_qspi_apb: Use 32 bit indirect write transaction when possible
2. revert commit b63b46313ed29e9b0c36b3d6b9407f6eade40c8f
 spi: cadence_qspi_apb: Use 32 bit indirect read transaction when possible
3. Apply my slightly modified version of 
https://patchwork.ozlabs.org/patch/693069/
 (should I keep Vignesh as the signed-off?)
4. Clean-up loading the reg variables to use fdtdec_get_addr_xxx(...) and mark
 them all as __iomem
5. Load the indirect-trigger property from the DT as a u32.  I think this still
 needs to be a u32 because it's used in a writel(...) 32-bit write call in
 cadence_qspi_apb.c
6. Add indirect-trigger to dts files that use cadence driver

I think that captures all the changes needed.

Also, for the naming of the DT property, Linux has uses a 'cdns,' prefix
(e.g. cdns,trigger-address) for a number of their properties (cdns,tshsl-ns,
cdns,tsd2d-ns, etc.), but u-boot does not currently use the 'cdns,' prefix for
the same properties.  Do you want the 'cdns,' prefix on trigger-address? 
If so, do you want me to rename all the other properties to align them with
the Linux DT as an additional patch?

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

Re: [U-Boot] [PATCH] spi: cadence_qspi_apb: Add trigger-base DT bindings from Linux

[Rush, Jason A.]

R, Vignesh wrote:
> On 2/24/2017 12:55 AM, Marek Vasut wrote:
>> On 02/23/2017 08:22 PM, Rush, Jason A. wrote:
>>> Marek Vasut wrote:
>>>> On 02/22/2017 06:37 PM, Rush, Jason A. wrote:
>>>>> Marek Vasut wrote:
>>>>>> On 02/21/2017 05:50 PM, Rush, Jason A. wrote:
> [...]
>>>
>>> While I was debugging some of my changes, I noticed that the data being 
>>> read from the
>>> QSPI flash device appears to be random.  The CPU no longer resets while 
>>> performing a
>>> read when the indirect trigger address is setup correctly for the Altrera 
>>> SoC, but there
>>> appears to be a larger problem with reading data in general.
>>>
>
> How random is it? Is the problem seen only when unaligned read/write are
> done or when length of transfer is not a multiple of word(4 byte)?
> If the data is really random in all cases, then I suspect timing issues.

I've been doing reads starting at NOR flash address 0x0, with a size of
0x4, reading into memory address 0x200.  So I don't think it's an
alignment issue.  The NOR flash is erased, so it should be all 0xFF, but
here's a memory dump of what I get running the latest from the master branch
(with my patch for indirect trigger_address):

=> sf probe
SF: Detected n25q1024 with page size 256 Bytes, erase size 64 KiB, total 128 MiB
=> sf read 0x200 0x0 0x4
device 0 offset 0x0, size 0x4
SF: 262144 bytes @ 0x0 Read: OK
=> md.b 0x200 0x40
0200: fe 1f 6e dc dd fb 7b 44 ff dd 49 fe f8 5b ab 1b..n...{D..I..[..
0210: 2f df 38 36 8f b6 47 eb 56 73 9c f9 05 ed 5c e7/.86..G.Vs\.
0220: c2 ef ad fe da 6d a8 78 49 fd df 5e 77 f9 66 37.m.xI..^w.f7
0230: cd 7f 7f eb 8d be 73 bf de c8 35 d5 10 bf a5 f6..s...5.

When I run from the v2016.11 tag, I get the following (expected results):

=> sf probe
SF: Detected n25q1024 with page size 256 Bytes, erase size 64 KiB, total 128 MiB
=> sf read 0x200 0x0 0x4
device 0 offset 0x0, size 0x4
SF: 262144 bytes @ 0x0 Read: OK
=> md.b 0x200 0x40
0200: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
0210: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
0220: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
0230: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff

> Please see if delay values are populated correctly in DT.

All the delay values, as well as all other CQSPI properties, in the DT match
the values at runtime.

>
>>> When I apply my patch to the v2016.11 release, reads appear correct.  
>>> However, when I
>>> apply my patch to the v2017.01 release, the data read from the QSPI device 
>>> appear to be
>>> random/corrupt.  I noticed the cadence_spi_apb.c file changed significantly 
>>> between
>>> v2016.11 and v2017.01, possibly a change in this file is causing the 
>>> problem on the Altera
>>> SoC.
>>>
>>> I'm not really that familiar with the cadence device, so this issue is 
>>> getting a little beyond a
>>> simple patch to setup the indirect trigger address correctly for the 
>>> Altrera SoC.  Is there
>>> anyone more familiar with the cadence device on the Altera SoC that could 
>>> take a look
>>> into this?
>>
>> Vignesh did those changes, so I think he can assist you. In the
>> meantime, you can try git bisect . Another thing you can try is
>> disabling the dcache and see if that fixes things (dcache off),
>> I recall seeing some caching issues with CQSPI.
>>
>
> You could try reverting my commits:
> commit 57897c13de03ac0136d64641a3eab526c6810387 spi: cadence_qspi_apb:
> Use 32 bit indirect write transaction when possible
> commit b63b46313ed29e9b0c36b3d6b9407f6eade40c8f spi: cadence_qspi_apb:
> Use 32 bit indirect read transaction when possible
>

When I reverted these two commits and added my patch for the indirect
trigger_address, it works correctly.

Also, when I disabled the dcache (dcache off) as Marek suggested, it works
correctly when running from the master branch (again with my indirect
trigger_address patch).

>
> But there were other patches by others in v2017.01-rc1, like
> spi: cadence_qspi: Fix CS timings which may have impact.
>

I left all other commits in except the two Vignesh suggested to revert, so
it seems to be related to those two commits and caching.  As another data
point, I can load and boot linux with caching on from another source (MMC).
So I don't think it's a problem with memory/caching in general.

Any suggestions on how to proceed from here?

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

Re: [U-Boot] [PATCH] spi: cadence_qspi_apb: Add trigger-base DT bindings from Linux

[Rush, Jason A.]

Marek Vasut wrote:
> On 02/22/2017 06:37 PM, Rush, Jason A. wrote:
>> Marek Vasut wrote:
>>> On 02/21/2017 05:50 PM, Rush, Jason A. wrote:
>>>> The socfpga arch uses a different value for the indaddrtrig reg than
>>>> the ahbbase address. Adopting the Linux DT bindings separates the
>>>> ahbbase and trigger-base addresses, allowing the trigger-base to be+
>>>> set correctly on the socfpga arch.
>>>>
>>>> Tested on Terasic SoCkit dev board (Altera Cyclone V)
>>>>
>>>> Signed-off-by: Jason A. Rush <jason.r...@gd-ms.com>
>>>> ---
>>>>  arch/arm/dts/socfpga.dtsi  | 1 +
>>>>  drivers/spi/cadence_qspi.c | 2 ++
>>>>  drivers/spi/cadence_qspi.h | 1 +
>>>>  drivers/spi/cadence_qspi_apb.c | 4 ++--
>>>>  4 files changed, 6 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/arch/arm/dts/socfpga.dtsi b/arch/arm/dts/socfpga.dtsi
>>>> index 8588221e57..2aff0c2419 100644
>>>> --- a/arch/arm/dts/socfpga.dtsi
>>>> +++ b/arch/arm/dts/socfpga.dtsi
>>>> @@ -644,6 +644,7 @@
>>>>clocks = <_clk>;
>>>>ext-decoder = <0>;  /* external decoder */
>>>>num-cs = <4>;
>>>> +  trigger-base = <0x>;
>>>
>>> Can you separate the DT patch from the driver patch ? Also, can you check 
>>> the other users of the CQSPI driver to see if they define the
>>> trigger base ?
>>>
>>
>> Yes, I will separate into two patches.
> 
> Thanks
> 
>> I default the trigger_base to the same value as the ahbbase if the 
>> trigger-base
>> was not defined in the DT.  That way, the driver code works as before for
>> architectures that expect the trigger_base to equal the value of the ahbbase.
>>  (e.g. TI K2G SoC).  I updated only the Altera SoC dtsi file since that 
>> architecture
>> needs a different value for the trigger_base.
> 
> In fact, the Linux DT bindings have the following and no AHB base, so
> please stick to that:
> 
> - cdns,trigger-address : 32-bit indirect AHB trigger address.
> 
> For details, see
> Documentation/devicetree/bindings/mtd/cadence-quadspi.txt in Linux 4.8
> or so and newer.
> 
>> Should I change this behavior to default the value to 0x0, and patch the 3 
>> dts/dtsi
>> files that use the cadence driver to explicitly include the trigger-base?
> 
> Yeah, looks sensible.
> 
>>>
>>>>fifo-depth = <128>;
>>>>sram-size = <128>;
>>>>bus-num = <2>;
>>>> diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c
>>>> index 9a6e41f330..a18b331f6c 100644
>>>> --- a/drivers/spi/cadence_qspi.c
>>>> +++ b/drivers/spi/cadence_qspi.c
>>>> @@ -296,6 +296,8 @@ static int cadence_spi_ofdata_to_platdata(struct
>>>> udevice *bus)
>>>>
>>>>plat->regbase = (void *)data[0];
>>>>plat->ahbbase = (void *)data[2];
>>>> +  plat->trigger_base = (void *)fdtdec_get_int(blob, node, "trigger-base",
>>>> +  (int)plat->ahbbase);
>>>
>>> Probably get u32 , but what about 64-bit systems ? Don't we have some 
>>> fdtdec_get.*addr ?
>>
>> You're right, this should be a u32.  I don't think I should have made 
>> trigger_base
>> a void* in the first place, but instead it should be a u32.  Looking at the 
>> Linux
>> kernel, which I just realized they call it trigger_address not trigger_base, 
>> it is just
>> a 32-bit value that is written into a 32-bit wide register, not an iomem 
>> memory
>> mapped pointer.
> 
> Ah right, the reg is 32bit . Is it possible that on aargh64, someone
> will pass 64bit trigger base in ?
> 
>> What if I change it to a u32 and rename it to trigger_address (which I should
>> have done the first time)?  That would align us correctly with the Linux 
>> kernel.
> 
> See above /wrt the naming. void __iomem * works on both 32 and 64bit
> systems, so I'd prefer to see that.
> 
>>>
>>>>plat->sram_size = fdtdec_get_int(blob, node, "sram-size", 128);
>>>>
>>>>/* All other paramters are embedded in the child node */ diff --git
>>>> a/drivers/spi/cadence_qspi.h b/drivers/spi/cadence_qspi.h index
>>>> d1927a4003..3948

Re: [U-Boot] [PATCH] spi: cadence_qspi_apb: Add trigger-base DT bindings from Linux

[Rush, Jason A.]

Marek Vasut wrote:
> On 02/21/2017 05:50 PM, Rush, Jason A. wrote:
>> The socfpga arch uses a different value for the indaddrtrig reg than
>> the ahbbase address. Adopting the Linux DT bindings separates the
>> ahbbase and trigger-base addresses, allowing the trigger-base to be+
>> set correctly on the socfpga arch.
>>
>> Tested on Terasic SoCkit dev board (Altera Cyclone V)
>>
>> Signed-off-by: Jason A. Rush <jason.r...@gd-ms.com>
>> ---
>>  arch/arm/dts/socfpga.dtsi  | 1 +
>>  drivers/spi/cadence_qspi.c | 2 ++
>>  drivers/spi/cadence_qspi.h | 1 +
>>  drivers/spi/cadence_qspi_apb.c | 4 ++--
>>  4 files changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm/dts/socfpga.dtsi b/arch/arm/dts/socfpga.dtsi
>> index 8588221e57..2aff0c2419 100644
>> --- a/arch/arm/dts/socfpga.dtsi
>> +++ b/arch/arm/dts/socfpga.dtsi
>> @@ -644,6 +644,7 @@
>>  clocks = <_clk>;
>>  ext-decoder = <0>;  /* external decoder */
>>  num-cs = <4>;
>> +trigger-base = <0x>;
> 
> Can you separate the DT patch from the driver patch ? Also, can you check the 
> other users of the CQSPI driver to see if they define the
> trigger base ?
>

Yes, I will separate into two patches.

I default the trigger_base to the same value as the ahbbase if the trigger-base
was not defined in the DT.  That way, the driver code works as before for
architectures that expect the trigger_base to equal the value of the ahbbase.
 (e.g. TI K2G SoC).  I updated only the Altera SoC dtsi file since that 
architecture
needs a different value for the trigger_base.

Should I change this behavior to default the value to 0x0, and patch the 3 
dts/dtsi
files that use the cadence driver to explicitly include the trigger-base?

> 
>>  fifo-depth = <128>;
>>  sram-size = <128>;
>>  bus-num = <2>;
>> diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c
>> index 9a6e41f330..a18b331f6c 100644
>> --- a/drivers/spi/cadence_qspi.c
>> +++ b/drivers/spi/cadence_qspi.c
>> @@ -296,6 +296,8 @@ static int cadence_spi_ofdata_to_platdata(struct
>> udevice *bus)
>>
>>  plat->regbase = (void *)data[0];
>>  plat->ahbbase = (void *)data[2];
>> +plat->trigger_base = (void *)fdtdec_get_int(blob, node, "trigger-base",
>> +(int)plat->ahbbase);
> 
> Probably get u32 , but what about 64-bit systems ? Don't we have some 
> fdtdec_get.*addr ?

You're right, this should be a u32.  I don't think I should have made 
trigger_base
a void* in the first place, but instead it should be a u32.  Looking at the 
Linux
kernel, which I just realized they call it trigger_address not trigger_base, it 
is just
a 32-bit value that is written into a 32-bit wide register, not an iomem memory
mapped pointer.

What if I change it to a u32 and rename it to trigger_address (which I should
have done the first time)?  That would align us correctly with the Linux kernel.

> 
>>  plat->sram_size = fdtdec_get_int(blob, node, "sram-size", 128);
>>
>>  /* All other paramters are embedded in the child node */ diff --git
>> a/drivers/spi/cadence_qspi.h b/drivers/spi/cadence_qspi.h index
>> d1927a4003..394820f308 100644
>> --- a/drivers/spi/cadence_qspi.h
>> +++ b/drivers/spi/cadence_qspi.h
>> @@ -18,6 +18,7 @@ struct cadence_spi_platdata {
>>  unsigned intmax_hz;
>>  void*regbase;
>>  void*ahbbase;
> 
> Can you remove the AHB base ? I think it's no longer used.

ahbbase is still used in cadence_qspi_apb.c, it's the register that the QSPI 
data
is read from, so it's still needed.

> Also, I think this should be void __iomem * here , also for regbase .
>

This is probably true, regbase and ahbbase should both be __iomem *, but
that feels like a different clean-up patch.  If you'd like me to, I could update
both of these as part of this patch though.

> 
>> +void*trigger_base;
>>
>>  u32 page_size;
>>  u32 block_size;
>> diff --git a/drivers/spi/cadence_qspi_apb.c
>> b/drivers/spi/cadence_qspi_apb.c index e02f2217f4..0e66d5fd82 100644
>> --- a/drivers/spi/cadence_qspi_apb.c
>> +++ b/drivers/spi/cadence_qspi_apb.c
>> @@ -560,7 +560,7 @@ int cadence_qspi_apb_indirect_read_setup(struct 
>> cadence_spi_platdata *plat,
>>  addr_bytes = cmdlen - 1;
>>
>>  /* Setup the indirect trigger addres

[U-Boot] [PATCH] spi: cadence_qspi_apb: Add trigger-base DT bindings from Linux

[Rush, Jason A.]

The socfpga arch uses a different value for the indaddrtrig reg than
the ahbbase address. Adopting the Linux DT bindings separates the
ahbbase and trigger-base addresses, allowing the trigger-base to be+
set correctly on the socfpga arch.

Tested on Terasic SoCkit dev board (Altera Cyclone V)

Signed-off-by: Jason A. Rush 
---
 arch/arm/dts/socfpga.dtsi  | 1 +
 drivers/spi/cadence_qspi.c | 2 ++
 drivers/spi/cadence_qspi.h | 1 +
 drivers/spi/cadence_qspi_apb.c | 4 ++--
 4 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/arm/dts/socfpga.dtsi b/arch/arm/dts/socfpga.dtsi
index 8588221e57..2aff0c2419 100644
--- a/arch/arm/dts/socfpga.dtsi
+++ b/arch/arm/dts/socfpga.dtsi
@@ -644,6 +644,7 @@
clocks = <_clk>;
ext-decoder = <0>;  /* external decoder */
num-cs = <4>;
+   trigger-base = <0x>;
fifo-depth = <128>;
sram-size = <128>;
bus-num = <2>;
diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c
index 9a6e41f330..a18b331f6c 100644
--- a/drivers/spi/cadence_qspi.c
+++ b/drivers/spi/cadence_qspi.c
@@ -296,6 +296,8 @@ static int cadence_spi_ofdata_to_platdata(struct udevice 
*bus)
 
plat->regbase = (void *)data[0];
plat->ahbbase = (void *)data[2];
+   plat->trigger_base = (void *)fdtdec_get_int(blob, node, "trigger-base",
+   (int)plat->ahbbase);
plat->sram_size = fdtdec_get_int(blob, node, "sram-size", 128);
 
/* All other paramters are embedded in the child node */
diff --git a/drivers/spi/cadence_qspi.h b/drivers/spi/cadence_qspi.h
index d1927a4003..394820f308 100644
--- a/drivers/spi/cadence_qspi.h
+++ b/drivers/spi/cadence_qspi.h
@@ -18,6 +18,7 @@ struct cadence_spi_platdata {
unsigned intmax_hz;
void*regbase;
void*ahbbase;
+   void*trigger_base;
 
u32 page_size;
u32 block_size;
diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/cadence_qspi_apb.c
index e02f2217f4..0e66d5fd82 100644
--- a/drivers/spi/cadence_qspi_apb.c
+++ b/drivers/spi/cadence_qspi_apb.c
@@ -560,7 +560,7 @@ int cadence_qspi_apb_indirect_read_setup(struct 
cadence_spi_platdata *plat,
addr_bytes = cmdlen - 1;
 
/* Setup the indirect trigger address */
-   writel((u32)plat->ahbbase,
+   writel((u32)plat->trigger_base,
   plat->regbase + CQSPI_REG_INDIRECTTRIGGER);
 
/* Configure the opcode */
@@ -710,7 +710,7 @@ int cadence_qspi_apb_indirect_write_setup(struct 
cadence_spi_platdata *plat,
return -EINVAL;
}
/* Setup the indirect trigger address */
-   writel((u32)plat->ahbbase,
+   writel((u32)plat->trigger_base,
   plat->regbase + CQSPI_REG_INDIRECTTRIGGER);
 
/* Configure the opcode */
-- 
2.11.0

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

Re: [U-Boot] socfpga qspi issues on SoCKit devkit

[Rush, Jason A.]

Marek Vasut wrote:
> On 02/20/2017 05:53 PM, Rush, Jason A. wrote:
>> Marek Vasut wrote:
>>> On 02/20/2017 05:25 AM, Vignesh R wrote:
>>>> + Marek
>>>
>>> Thanks, +CC Dinh and Ley
>>>
>>>> On Friday 17 February 2017 05:02 AM, Rush, Jason A. wrote:
>>>>> The QSPI NOR interface on the Altera Cyclone V SoCKit devkit (Rev
>>>>> B) appears to be broken in the current release.  I've tracked it
>>>>> down to working in the
>>>>> v2016.07 release, but broken in the the v2016.09 release.  With the
>>>>> help of git bisect, I tracked down the commit that breaks the QSPI to the 
>>>>> following:
>>>>>
>>>>>   commit dac3bf20fb2c9b03476be0d73db620f62ab3cee1
>>>>>   Author: Vignesh R <vigne...@ti.com>
>>>>>   Date:   Wed Jul 6 10:20:55 2016 +0530
>>>>>
>>>>>   spi: cadence_qspi_apb: Support 32 bit AHB address
>>>>>
>>>>>   AHB address can be as long as 32 bit, hence remove the
>>>>>   CQSPI_REG_INDIRECTRDSTARTADDR mask. Since AHB address is passed 
>>>>> from DT
>>>>>   and read as u32 value, it anyway does not make sense to mask upper 
>>>>> bits.
>>>>>
>>>>>   Signed-off-by: Vignesh R <vigne...@ti.com>
>>>>>   Tested-by: Marek Vasut <ma...@denx.de>
>>>>>   Acked-by: Marek Vasut <ma...@denx.de>
>>>>>   Reviewed-by: Jagan Teki <jt...@openedev.com>
>>>>>
>>>>> When I try and read anything from the QSPI on the
>>>>> dac3bf20fb2c9b03476be0d73db620f62ab3cee1 commit I get a "data
>>>>> abort" and the CPU resets.  Example output below is from a clean
>>>>> build of U-Boot configured with socfpga_sockit_defconfig:
>>>>>
>>>>>  => sf probe
>>>>>   SF: Detected N25Q1024 with page size 256 Bytes, erase size 64 KiB, 
>>>>> total 128 MiB
>>>>>   => sf read 0x200 0x5 0x5000
>>>>>   device 0 offset 0x5, size 0x5000
>>>>>   data abort
>>>>>   pc : [<3ff7a9bc>]  lr : [<3ff98359>]
>>>>>   reloc pc : [<010249fc>]lr : [<01042399>]
>>>>>   sp : 3bf4fde8  ip : 3ff7a371 fp : 0002
>>>>>   r10:   r9 : 3bf54ee8 r8 : 3bf55530
>>>>>   r7 : 270d  r6 : 0200 r5 : 5000  r4 : 3bf55530
>>>>>   r3 : 0004  r2 :  r1 : 0200  r0 : ffa0
>>>>>   Flags: nZCv  IRQs off  FIQs off  Mode SVC_32
>>>>>   Resetting CPU ...
>>>>>
>>>>> When I run the same commands with the previous commit in the git
>>>>> log
>>>>> (fdf02a36c52cb96717b64113775c4251ecd49596) I get the following output:
>>>>>
>>>>>   => sf probe
>>>>>   SF: Detected N25Q1024 with page size 256 Bytes, erase size 64 KiB, 
>>>>> total 128 MiB
>>>>>   => sf read 0x200 0x5 0x5000
>>>>>   device 0 offset 0x5, size 0x5000
>>>>>   SF: 20480 bytes @ 0x5 Read: OK
>>>>>   =>
>>>>>
>>>>> I've done some investigation, and previously the ahbbase was masked
>>>>> so the value
>>>>> 0xFFA0_ results in 0x0 when writing the CQSPI_REG_INDIRECTTRIGGER 
>>>>> register.
>>>>> I assume the above commit works on some boards, but it appears to
>>>>> break the socfpga arch.
>>>>>
>>>>> Is there someone that could help investigate or confirm this?
>>>>
>>>> I am looking at cyclone5_handbook.pdf [1] and it says indaddrtrig
>>>> field is 32 bit wide. So, I wonder why masking is needed. If you
>>>> specify ahbbase as  0xFFA0_ in DT(reg property) then the
>>>> expectation is
>>>> 0xFFA0_ gets written to indaddrtrig reg. It looks like the
>>>> trigger address is 0x0 and not 0xFFA0_, therefore you may have
>>>> to update reg property in DT to reflect the same.
>>>>
>>>> [1]https://www.altera.com/en_US/pdfs/literature/hb/cyclone-v/cyclone
>>>> 5_
>>>> handbook.pdf
>>>>
>>
>> I agree, according to the handbook the indaddrtrig register is 32-bits
>> wide, but writing the value 0xFFA0_ to indaddrtrig causes the
>> above data abort.  It looks like a similar issue has been discussed before:
>> http://lists.denx.de/pipermail/u-boot/2015-August/224649.html
>>
>> In that proposed patch, it looks like the address to use for the
>> indaddrtrig reg was being separated from the ahbbase and the Altera
>> SoC needed a value of 0x_.  I'm not very familiar with the
>> Cadence device on the Altera SoC, but it looks like it needs a
>> different value than the ahbbase written to the indaddrtrig reg.
>>
>> I noticed the Cadence QSPI driver in the Linux tree has a separate
>> trigger_address that is loaded from the DT, and the socfpga.dtsi sets
>> the trigger_address to 0x_.
> 
> So let's just adopt the Linux bindings and get this fixed. Can you send a 
> patch ? Thanks

I'll work on a patch.

> btw where did this 0xffa0_ come from, Dinh ?
> 

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

Re: [U-Boot] socfpga qspi issues on SoCKit devkit

[Rush, Jason A.]

Marek Vasut wrote:
> On 02/20/2017 05:25 AM, Vignesh R wrote:
>> + Marek
>
> Thanks, +CC Dinh and Ley
>
>> On Friday 17 February 2017 05:02 AM, Rush, Jason A. wrote:
>>> The QSPI NOR interface on the Altera Cyclone V SoCKit devkit (Rev B) appears
>>> to be broken in the current release.  I've tracked it down to working in the
>>> v2016.07 release, but broken in the the v2016.09 release.  With the help of 
>>> git
>>> bisect, I tracked down the commit that breaks the QSPI to the following:
>>>
>>>   commit dac3bf20fb2c9b03476be0d73db620f62ab3cee1
>>>   Author: Vignesh R <vigne...@ti.com>
>>>   Date:   Wed Jul 6 10:20:55 2016 +0530
>>>
>>>   spi: cadence_qspi_apb: Support 32 bit AHB address
>>>
>>>   AHB address can be as long as 32 bit, hence remove the
>>>   CQSPI_REG_INDIRECTRDSTARTADDR mask. Since AHB address is passed from 
>>> DT
>>>   and read as u32 value, it anyway does not make sense to mask upper 
>>> bits.
>>>
>>>   Signed-off-by: Vignesh R <vigne...@ti.com>
>>>   Tested-by: Marek Vasut <ma...@denx.de>
>>>   Acked-by: Marek Vasut <ma...@denx.de>
>>>   Reviewed-by: Jagan Teki <jt...@openedev.com>
>>>
>>> When I try and read anything from the QSPI on the
>>> dac3bf20fb2c9b03476be0d73db620f62ab3cee1 commit I get a "data abort" and the
>>> CPU resets.  Example output below is from a clean build of U-Boot configured
>>> with socfpga_sockit_defconfig:
>>>
>>>  => sf probe
>>>   SF: Detected N25Q1024 with page size 256 Bytes, erase size 64 KiB, total 
>>> 128 MiB
>>>   => sf read 0x200 0x5 0x5000
>>>   device 0 offset 0x5, size 0x5000
>>>   data abort
>>>   pc : [<3ff7a9bc>]  lr : [<3ff98359>]
>>>   reloc pc : [<010249fc>]lr : [<01042399>]
>>>   sp : 3bf4fde8  ip : 3ff7a371 fp : 0002
>>>   r10:   r9 : 3bf54ee8 r8 : 3bf55530
>>>   r7 : 270d  r6 : 0200 r5 : 5000  r4 : 3bf55530
>>>   r3 : 0004  r2 :  r1 : 0200  r0 : ffa0
>>>   Flags: nZCv  IRQs off  FIQs off  Mode SVC_32
>>>   Resetting CPU ...
>>>
>>> When I run the same commands with the previous commit in the git log
>>> (fdf02a36c52cb96717b64113775c4251ecd49596) I get the following output:
>>>
>>>   => sf probe
>>>   SF: Detected N25Q1024 with page size 256 Bytes, erase size 64 KiB, total 
>>> 128 MiB
>>>   => sf read 0x200 0x5 0x5000
>>>   device 0 offset 0x5, size 0x5000
>>>   SF: 20480 bytes @ 0x5 Read: OK
>>>   =>
>>>
>>> I've done some investigation, and previously the ahbbase was masked so the 
>>> value
>>> 0xFFA0_ results in 0x0 when writing the CQSPI_REG_INDIRECTTRIGGER 
>>> register.
>>> I assume the above commit works on some boards, but it appears to break the
>>> socfpga arch.
>>>
>>> Is there someone that could help investigate or confirm this?
>>
>> I am looking at cyclone5_handbook.pdf [1] and it says indaddrtrig
>> field is 32 bit wide. So, I wonder why masking is needed. If you
>> specify ahbbase as  0xFFA0_ in DT(reg property) then the
>> expectation is
>> 0xFFA0_ gets written to indaddrtrig reg. It looks like the trigger
>> address is 0x0 and not 0xFFA0_, therefore you may have to update
>> reg property in DT to reflect the same.
>>
>> [1]https://www.altera.com/en_US/pdfs/literature/hb/cyclone-v/cyclone5_
>> handbook.pdf
>>

I agree, according to the handbook the indaddrtrig register is 32-bits wide, but
writing the value 0xFFA0_ to indaddrtrig causes the above data abort.  It
looks like a similar issue has been discussed before:
http://lists.denx.de/pipermail/u-boot/2015-August/224649.html

In that proposed patch, it looks like the address to use for the indaddrtrig reg
was being separated from the ahbbase and the Altera SoC needed a value of
0x_.  I'm not very familiar with the Cadence device on the Altera SoC,
but it looks like it needs a different value than the ahbbase written to the
indaddrtrig reg.

I noticed the Cadence QSPI driver in the Linux tree has a separate
trigger_address that is loaded from the DT, and the socfpga.dtsi sets the
trigger_address to 0x_.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] socfpga qspi issues on SoCKit devkit

[Rush, Jason A.]

The QSPI NOR interface on the Altera Cyclone V SoCKit devkit (Rev B) appears to 
be broken in the current release.  I've tracked it down to working in the 
v2016.07 release, but broken in the the v2016.09 release.  With the help of git 
bisect, I tracked down the commit that breaks the QSPI to the following:

  commit dac3bf20fb2c9b03476be0d73db620f62ab3cee1
  Author: Vignesh R 
  Date:   Wed Jul 6 10:20:55 2016 +0530

  spi: cadence_qspi_apb: Support 32 bit AHB address

  AHB address can be as long as 32 bit, hence remove the
  CQSPI_REG_INDIRECTRDSTARTADDR mask. Since AHB address is passed from DT
  and read as u32 value, it anyway does not make sense to mask upper bits.

  Signed-off-by: Vignesh R 
  Tested-by: Marek Vasut 
  Acked-by: Marek Vasut 
  Reviewed-by: Jagan Teki 

When I try and read anything from the QSPI on the 
dac3bf20fb2c9b03476be0d73db620f62ab3cee1 commit I get a "data abort" and the 
CPU resets.  Example output below is from a clean build of U-Boot configured 
with socfpga_sockit_defconfig:

 => sf probe
  SF: Detected N25Q1024 with page size 256 Bytes, erase size 64 KiB, total 128 
MiB
  => sf read 0x200 0x5 0x5000
  device 0 offset 0x5, size 0x5000
  data abort
  pc : [<3ff7a9bc>]  lr : [<3ff98359>]
  reloc pc : [<010249fc>]lr : [<01042399>]
  sp : 3bf4fde8  ip : 3ff7a371 fp : 0002
  r10:   r9 : 3bf54ee8 r8 : 3bf55530
  r7 : 270d  r6 : 0200 r5 : 5000  r4 : 3bf55530
  r3 : 0004  r2 :  r1 : 0200  r0 : ffa0
  Flags: nZCv  IRQs off  FIQs off  Mode SVC_32
  Resetting CPU ...

When I run the same commands with the previous commit in the git log 
(fdf02a36c52cb96717b64113775c4251ecd49596) I get the following output:

  => sf probe
  SF: Detected N25Q1024 with page size 256 Bytes, erase size 64 KiB, total 128 
MiB
  => sf read 0x200 0x5 0x5000
  device 0 offset 0x5, size 0x5000
  SF: 20480 bytes @ 0x5 Read: OK
  =>

I've done some investigation, and previously the ahbbase was masked so the 
value 0xFFA0_ results in 0x0 when writing the CQSPI_REG_INDIRECTTRIGGER 
register.  I assume the above commit works on some boards, but it appears to 
break the socfpga arch.

Is there someone that could help investigate or confirm this?

Thanks,
Jason

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