[U-Boot] [PATCH] spi: cadence_qspi_apb: Add trigger-base DT bindings from Linux
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.rush@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 = <&qspi_clk>; ext-decoder = <0>; /* external decoder */ num-cs = <4>; + trigger-base = <0x00000000>; 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 int max_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 */
On Tuesday 21 February 2017 10:20 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.rush@gd-ms.com
Tested on TI K2G EVM
Tested-by: Vignesh R vigneshr@ti.com
Thanks Vignesh
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.rush@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 = <&qspi_clk>; ext-decoder = <0>; /* external decoder */ num-cs = <4>;
trigger-base = <0x00000000>;
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 ?
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 ?
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 int max_hz; void *regbase; void *ahbbase;
Can you remove the AHB base ? I think it's no longer used. Also, I think this should be void __iomem * here , also for regbase .
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 */
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.rush@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 = <&qspi_clk>; ext-decoder = <0>; /* external decoder */ num-cs = <4>;
trigger-base = <0x00000000>;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 int max_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 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 */
Thanks, Jason
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.rush@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 = <&qspi_clk>; ext-decoder = <0>; /* external decoder */ num-cs = <4>;
trigger-base = <0x00000000>;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..394820f308 100644 --- a/drivers/spi/cadence_qspi.h +++ b/drivers/spi/cadence_qspi.h @@ -18,6 +18,7 @@ struct cadence_spi_platdata { unsigned int max_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.
Aaaah, and looking at the Linux bindings, there it comes from the reg property. OK, so much for the bloody confusing naming. If you feel like cleaning this up, separate patch is welcome, if not ... oh well.
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.
Yeah, that'd be brilliant if you could clean this bit too. Thanks!
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 */
Thanks, Jason
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.rush@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 = <&qspi_clk>; ext-decoder = <0>; /* external decoder */ num-cs = <4>;
trigger-base = <0x00000000>;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..394820f308 100644 --- a/drivers/spi/cadence_qspi.h +++ b/drivers/spi/cadence_qspi.h @@ -18,6 +18,7 @@ struct cadence_spi_platdata { unsigned int max_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.
Aaaah, and looking at the Linux bindings, there it comes from the reg property. OK, so much for the bloody confusing naming. If you feel like cleaning this up, separate patch is welcome, if not ... oh well.
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.
Yeah, that'd be brilliant if you could clean this bit too. Thanks!
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 */
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.
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?
Thanks, Jason
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:
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.rush@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 = <&qspi_clk>; ext-decoder = <0>; /* external decoder */ num-cs = <4>;
trigger-base = <0x00000000>;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..394820f308 100644 --- a/drivers/spi/cadence_qspi.h +++ b/drivers/spi/cadence_qspi.h @@ -18,6 +18,7 @@ struct cadence_spi_platdata { unsigned int max_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.
Aaaah, and looking at the Linux bindings, there it comes from the reg property. OK, so much for the bloody confusing naming. If you feel like cleaning this up, separate patch is welcome, if not ... oh well.
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.
Yeah, that'd be brilliant if you could clean this bit too. Thanks!
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 */
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.
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.
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. Please see if delay values are populated correctly in DT.
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
But there were other patches by others in v2017.01-rc1, like spi: cadence_qspi: Fix CS timings which may have impact.
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 0x40000, reading into memory address 0x2000000. 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 0x2000000 0x0 0x40000 device 0 offset 0x0, size 0x40000 SF: 262144 bytes @ 0x0 Read: OK => md.b 0x2000000 0x40 02000000: fe 1f 6e dc dd fb 7b 44 ff dd 49 fe f8 5b ab 1b ..n...{D..I..[.. 02000010: 2f df 38 36 8f b6 47 eb 56 73 9c f9 05 ed 5c e7 /.86..G.Vs..... 02000020: c2 ef ad fe da 6d a8 78 49 fd df 5e 77 f9 66 37 .....m.xI..^w.f7 02000030: 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 0x2000000 0x0 0x40000 device 0 offset 0x0, size 0x40000 SF: 262144 bytes @ 0x0 Read: OK => md.b 0x2000000 0x40 02000000: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................ 02000010: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................ 02000020: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................ 02000030: 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
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?
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:
diff --git a/common/bouncebuf.c b/common/bouncebuf.c index 054d9e0302cc..2878b9eed1ae 100644 --- a/common/bouncebuf.c +++ b/common/bouncebuf.c @@ -55,21 +55,21 @@ int bounce_buffer_start(struct bounce_buffer *state, void *data, * Flush data to RAM so DMA reads can pick it up, * and any CPU writebacks don't race with DMA writes */ - flush_dcache_range((unsigned long)state->bounce_buffer, - (unsigned long)(state->bounce_buffer) + - state->len_aligned); +// flush_dcache_range((unsigned long)state->bounce_buffer, +// (unsigned long)(state->bounce_buffer) + +// state->len_aligned);
return 0; }
int bounce_buffer_stop(struct bounce_buffer *state) { - if (state->flags & GEN_BB_WRITE) { - /* Invalidate cache so that CPU can see any newly DMA'd data */ - invalidate_dcache_range((unsigned long)state->bounce_buffer, - (unsigned long)(state->bounce_buffer) + - state->len_aligned); - } +// if (state->flags & GEN_BB_WRITE) { +// /* Invalidate cache so that CPU can see any newly DMA'd data */ +// invalidate_dcache_range((unsigned long)state->bounce_buffer, +// (unsigned long)(state->bounce_buffer) + +// state->len_aligned); +// }
if (state->bounce_buffer == state->user_buffer) return 0;
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.
participants (4)
-
Marek Vasut -
R, Vignesh -
Rush, Jason A. -
Vignesh R