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

[Marek Vasut]

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

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
> 


-- 
Best regards,
Marek Vasut