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