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

Thu Feb 23 19:25:32 UTC 2017

On 02/23/2017 08:22 PM, Rush, Jason A. wrote:
> 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?

Vignesh did those changes, so I think he can assist you. In the
meantime, you can try git bisect . Another thing you can try is
disabling the dcache and see if that fixes things (dcache off),
I recall seeing some caching issues with CQSPI.

-- 
Best regards,
Marek Vasut