[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.rush at 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