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