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

Rush, Jason A. Jason.Rush at gd-ms.com
Thu Feb 23 19:22:51 UTC 2017


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 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 */
>>>>
>>>
>>

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


More information about the U-Boot mailing list