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

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

[Rush, Jason A.]

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>;
 			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);
 	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;
+	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 */
-- 
2.11.0

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

[Vignesh R]

On Tuesday 21 February 2017 10:20 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>
> ---

Tested on TI K2G EVM

Tested-by: Vignesh R <vigneshr@ti.com>

Thanks
Vignesh

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

[Marek Vasut]

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 ?

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

>  	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.
Also, I think this should be void __iomem * here , also for
regbase .

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


-- 
Best regards,
Marek Vasut

[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@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

[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@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

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

[Rush, Jason A.]

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

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

[Marek Vasut]

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

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

[R, Vignesh]

On 2/24/2017 12:55 AM, Marek Vasut wrote:
> 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:
[...]
>>
>> 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.
>>

How random is it? Is the problem seen only when unaligned read/write are
done or when length of transfer is not a multiple of word(4 byte)?
If the data is really random in all cases, then I suspect timing issues.
Please see if delay values are populated correctly in DT.

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

You could try reverting my commits:
commit 57897c13de03ac0136d64641a3eab526c6810387 spi: cadence_qspi_apb:
Use 32 bit indirect write transaction when possible
commit b63b46313ed29e9b0c36b3d6b9407f6eade40c8f spi: cadence_qspi_apb:
Use 32 bit indirect read transaction when possible

But there were other patches by others in v2017.01-rc1, like
spi: cadence_qspi: Fix CS timings which may have impact.


-- 
Regards
Vignesh

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

[Rush, Jason A.]

R, Vignesh wrote:
> On 2/24/2017 12:55 AM, Marek Vasut wrote:
>> 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:
> [...]
>>>
>>> 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.
>>>
>
> How random is it? Is the problem seen only when unaligned read/write are
> done or when length of transfer is not a multiple of word(4 byte)?
> If the data is really random in all cases, then I suspect timing issues.

I've been doing reads starting at NOR flash address 0x0, with a size of
0x40000, reading into memory address 0x2000000.  So I don't think it's an
alignment issue.  The NOR flash is erased, so it should be all 0xFF, but
here's a memory dump of what I get running the latest from the master branch
(with my patch for indirect trigger_address):

=> sf probe
SF: Detected n25q1024 with page size 256 Bytes, erase size 64 KiB, total 128 MiB
=> sf read 0x2000000 0x0 0x40000
device 0 offset 0x0, size 0x40000
SF: 262144 bytes @ 0x0 Read: OK
=> md.b 0x2000000 0x40
02000000: fe 1f 6e dc dd fb 7b 44 ff dd 49 fe f8 5b ab 1b    ..n...{D..I..[..
02000010: 2f df 38 36 8f b6 47 eb 56 73 9c f9 05 ed 5c e7    /.86..G.Vs....\.
02000020: c2 ef ad fe da 6d a8 78 49 fd df 5e 77 f9 66 37    .....m.xI..^w.f7
02000030: cd 7f 7f eb 8d be 73 bf de c8 35 d5 10 bf a5 f6    ......s...5.....

When I run from the v2016.11 tag, I get the following (expected results):

=> sf probe
SF: Detected n25q1024 with page size 256 Bytes, erase size 64 KiB, total 128 MiB
=> sf read 0x2000000 0x0 0x40000
device 0 offset 0x0, size 0x40000
SF: 262144 bytes @ 0x0 Read: OK
=> md.b 0x2000000 0x40
02000000: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
02000010: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
02000020: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
02000030: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................

> Please see if delay values are populated correctly in DT.

All the delay values, as well as all other CQSPI properties, in the DT match
the values at runtime.

>
>>> 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.
>>
>
> You could try reverting my commits:
> commit 57897c13de03ac0136d64641a3eab526c6810387 spi: cadence_qspi_apb:
> Use 32 bit indirect write transaction when possible
> commit b63b46313ed29e9b0c36b3d6b9407f6eade40c8f spi: cadence_qspi_apb:
> Use 32 bit indirect read transaction when possible
>

When I reverted these two commits and added my patch for the indirect
trigger_address, it works correctly.

Also, when I disabled the dcache (dcache off) as Marek suggested, it works
correctly when running from the master branch (again with my indirect
trigger_address patch).

>
> But there were other patches by others in v2017.01-rc1, like
> spi: cadence_qspi: Fix CS timings which may have impact.
>

I left all other commits in except the two Vignesh suggested to revert, so
it seems to be related to those two commits and caching.  As another data
point, I can load and boot linux with caching on from another source (MMC).
So I don't think it's a problem with memory/caching in general.

Any suggestions on how to proceed from here?

---
Regards,
Jason

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

[R, Vignesh]

On 2/25/2017 1:25 AM, Rush, Jason A. wrote:
> R, Vignesh wrote:
>> On 2/24/2017 12:55 AM, Marek Vasut wrote:
>>> 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:
>> [...]
>>>>
>>
>> You could try reverting my commits:
>> commit 57897c13de03ac0136d64641a3eab526c6810387 spi: cadence_qspi_apb:
>> Use 32 bit indirect write transaction when possible
>> commit b63b46313ed29e9b0c36b3d6b9407f6eade40c8f spi: cadence_qspi_apb:
>> Use 32 bit indirect read transaction when possible
>>
> 
> When I reverted these two commits and added my patch for the indirect
> trigger_address, it works correctly.
>

Oops, these patches are required as Cadence QSPI controller(I am not
sure if all versions of IP are newer versions only) has a limitation
that the external master is only permitted to issue 32-bit data
interface reads until the last word of an indirect transfer.


> Also, when I disabled the dcache (dcache off) as Marek suggested, it works
> correctly when running from the master branch (again with my indirect
> trigger_address patch).
> 

Just that I understand correctly, with latest master(with no patches
reverted) + your patch for indirect trigger_address + dcache off, you
don't see any problem?

>>
>> But there were other patches by others in v2017.01-rc1, like
>> spi: cadence_qspi: Fix CS timings which may have impact.
>>
> 
> I left all other commits in except the two Vignesh suggested to revert, so
> it seems to be related to those two commits and caching.  As another data
> point, I can load and boot linux with caching on from another source (MMC).
> So I don't think it's a problem with memory/caching in general.
> 
> Any suggestions on how to proceed from here?
> 

My patches use common bounce_buffer implementation which does dcache
flush/invalidate and if dcache has issues then I guess those operations
may be causing data corruption. Could you do a bit more research for me?

1. As a hack, could you just disable dcache operations in bounce_buffer
implementation? Here is the diff for the same:


diff --git a/common/bouncebuf.c b/common/bouncebuf.c
index 054d9e0302cc..2878b9eed1ae 100644
--- a/common/bouncebuf.c
+++ b/common/bouncebuf.c
@@ -55,21 +55,21 @@ int bounce_buffer_start(struct bounce_buffer *state, void *data,
         * Flush data to RAM so DMA reads can pick it up,
         * and any CPU writebacks don't race with DMA writes
         */
-       flush_dcache_range((unsigned long)state->bounce_buffer,
-                               (unsigned long)(state->bounce_buffer) +
-                                       state->len_aligned);
+//     flush_dcache_range((unsigned long)state->bounce_buffer,
+//                             (unsigned long)(state->bounce_buffer) +
+//                                     state->len_aligned);

        return 0;
 }

 int bounce_buffer_stop(struct bounce_buffer *state)
 {
-       if (state->flags & GEN_BB_WRITE) {
-               /* Invalidate cache so that CPU can see any newly DMA'd data */
-               invalidate_dcache_range((unsigned long)state->bounce_buffer,
-                                       (unsigned long)(state->bounce_buffer) +
-                                               state->len_aligned);
-       }
+//     if (state->flags & GEN_BB_WRITE) {
+//             /* Invalidate cache so that CPU can see any newly DMA'd data */
+//             invalidate_dcache_range((unsigned long)state->bounce_buffer,
+//                                     (unsigned long)(state->bounce_buffer) +
+//                                             state->len_aligned);
+//     }

        if (state->bounce_buffer == state->user_buffer)
                return 0;
>
>
>

2. If that works, I guess there is some issue wrt CQSPI and dcache on your platform,
I suggest you to revert my above two patches and try non bounce buffer version of 
my changes here: https://patchwork.ozlabs.org/patch/693069/.
This patch takes care of indirect write. I don't have similar patch for
indirect read but that wasn't required.

-- 
Regards
Vignesh

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

[Rush, Jason A.]

I don't know if this message successfully sent last weekend.  My mail server was undergoing maintenance, and I didn't see my original message on the u-boot mailing list archive.  So I'm resending, my apologies if this is a repost.

R, Vignesh wrote:
> On 2/25/2017 1:25 AM, Rush, Jason A. wrote:
>> R, Vignesh wrote:
>>> On 2/24/2017 12:55 AM, Marek Vasut wrote:
>>>> 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:
>>> [...]
>>>>>
>>>
>>> You could try reverting my commits:
>>> commit 57897c13de03ac0136d64641a3eab526c6810387 spi: cadence_qspi_apb:
>>> Use 32 bit indirect write transaction when possible
>>> commit b63b46313ed29e9b0c36b3d6b9407f6eade40c8f spi: cadence_qspi_apb:
>>> Use 32 bit indirect read transaction when possible
>>>
>>
>> When I reverted these two commits and added my patch for the indirect
>> trigger_address, it works correctly.
>>
> 
> Oops, these patches are required as Cadence QSPI controller(I am not
> sure if all versions of IP are newer versions only) has a limitation
> that the external master is only permitted to issue 32-bit data
> interface reads until the last word of an indirect transfer.
> 
> 
>> Also, when I disabled the dcache (dcache off) as Marek suggested, it works
>> correctly when running from the master branch (again with my indirect
>> trigger_address patch).
>>
> 
> Just that I understand correctly, with latest master(with no patches
> reverted) + your patch for indirect trigger_address + dcache off, you
> don't see any problem?
> 

Correct.

>>>
>>> But there were other patches by others in v2017.01-rc1, like
>>> spi: cadence_qspi: Fix CS timings which may have impact.
>>>
>>
>> I left all other commits in except the two Vignesh suggested to revert, so
>> it seems to be related to those two commits and caching.  As another data
>> point, I can load and boot linux with caching on from another source (MMC).
>> So I don't think it's a problem with memory/caching in general.
>>
>> Any suggestions on how to proceed from here?
>>
> 
> My patches use common bounce_buffer implementation which does dcache
> flush/invalidate and if dcache has issues then I guess those operations
> may be causing data corruption. Could you do a bit more research for me?
> 
> 1. As a hack, could you just disable dcache operations in bounce_buffer
> implementation? Here is the diff for the same:
> 

This works.  So it does seem to be an issue with the CQSPI and dcache on
the Altera SoC.

> 
> diff --git a/common/bouncebuf.c b/common/bouncebuf.c
> index 054d9e0302cc..2878b9eed1ae 100644
> --- a/common/bouncebuf.c
> +++ b/common/bouncebuf.c
[...]
> 
> 2. If that works, I guess there is some issue wrt CQSPI and dcache on your platform,
> I suggest you to revert my above two patches and try non bounce buffer version of
> my changes here: https://patchwork.ozlabs.org/patch/693069/.
> This patch takes care of indirect write. I don't have similar patch for
> indirect read but that wasn't required.
> 

This also works.

Marek - how do you feel about a patch series with the following:

1. revert commit 57897c13de03ac0136d64641a3eab526c6810387
     spi: cadence_qspi_apb: Use 32 bit indirect write transaction when possible
2. revert commit b63b46313ed29e9b0c36b3d6b9407f6eade40c8f
     spi: cadence_qspi_apb: Use 32 bit indirect read transaction when possible
3. Apply my slightly modified version of https://patchwork.ozlabs.org/patch/693069/
     (should I keep Vignesh as the signed-off?)
4. Clean-up loading the reg variables to use fdtdec_get_addr_xxx(...) and mark
     them all as __iomem
5. Load the indirect-trigger property from the DT as a u32.  I think this still
     needs to be a u32 because it's used in a writel(...) 32-bit write call in
     cadence_qspi_apb.c
6. Add indirect-trigger to dts files that use cadence driver

I think that captures all the changes needed.

Also, for the naming of the DT property, Linux has uses a 'cdns,' prefix
(e.g. cdns,trigger-address) for a number of their properties (cdns,tshsl-ns,
cdns,tsd2d-ns, etc.), but u-boot does not currently use the 'cdns,' prefix for
the same properties.  Do you want the 'cdns,' prefix on trigger-address? 
If so, do you want me to rename all the other properties to align them with
the Linux DT as an additional patch?

--
Regards,
Jason

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

[R, Vignesh]

On 2/28/2017 8:38 PM, Rush, Jason A. wrote:
[...]
> 
> This also works.
> 
> Marek - how do you feel about a patch series with the following:
> 
> 1. revert commit 57897c13de03ac0136d64641a3eab526c6810387
>      spi: cadence_qspi_apb: Use 32 bit indirect write transaction when possible
> 2. revert commit b63b46313ed29e9b0c36b3d6b9407f6eade40c8f
>      spi: cadence_qspi_apb: Use 32 bit indirect read transaction when possible
> 3. Apply my slightly modified version of https://patchwork.ozlabs.org/patch/693069/
>      (should I keep Vignesh as the signed-off?)

Depends on how much you have changed the code. If the change is
significant then change the authorship to you and drop my signed-off.
Else, keep the authorship and signed-off.
If you are adding something new to the patch like adding code to make
sure that only 32bit data reads are issued, then I suggest you to submit
that change as separate patch.

> 4. Clean-up loading the reg variables to use fdtdec_get_addr_xxx(...) and mark
>      them all as __iomem
> 5. Load the indirect-trigger property from the DT as a u32.  I think this still
>      needs to be a u32 because it's used in a writel(...) 32-bit write call in
>      cadence_qspi_apb.c
> 6. Add indirect-trigger to dts files that use cadence driver
> 
> I think that captures all the changes needed.
> 
> Also, for the naming of the DT property, Linux has uses a 'cdns,' prefix
> (e.g. cdns,trigger-address) for a number of their properties (cdns,tshsl-ns,
> cdns,tsd2d-ns, etc.), but u-boot does not currently use the 'cdns,' prefix for
> the same properties.  Do you want the 'cdns,' prefix on trigger-address? 
> If so, do you want me to rename all the other properties to align them with
> the Linux DT as an additional patch?
> 
> --
> Regards,
> Jason
> 

-- 
Regards
Vignesh

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

[Rush, Jason A.]

R, Vignesh wrote:
> On 2/28/2017 8:38 PM, Rush, Jason A. wrote:
> [...]
>>
>> This also works.
>>
>> Marek - how do you feel about a patch series with the following:
>>
>> 1. revert commit 57897c13de03ac0136d64641a3eab526c6810387
>>      spi: cadence_qspi_apb: Use 32 bit indirect write transaction when possible
>> 2. revert commit b63b46313ed29e9b0c36b3d6b9407f6eade40c8f
>>      spi: cadence_qspi_apb: Use 32 bit indirect read transaction when possible
>> 3. Apply my slightly modified version of https://patchwork.ozlabs.org/patch/693069/
>>      (should I keep Vignesh as the signed-off?)
> 
> Depends on how much you have changed the code. If the change is
> significant then change the authorship to you and drop my signed-off.
> Else, keep the authorship and signed-off.
> If you are adding something new to the patch like adding code to make
> sure that only 32bit data reads are issued, then I suggest you to submit
> that change as separate patch.

Very minimal.  Another commit changed a #define from
CQSPI_REG_INDIRECTWR_START_MASK to CQSPI_REG_INDIRECTWR_START,
so I had to modify the patch to drop the _MASK so it would apply.  Other
than that, it's identical in content.

> 
>> 4. Clean-up loading the reg variables to use fdtdec_get_addr_xxx(...) and mark
>>      them all as __iomem
>> 5. Load the indirect-trigger property from the DT as a u32.  I think this still
>>      needs to be a u32 because it's used in a writel(...) 32-bit write call in
>>      cadence_qspi_apb.c
>> 6. Add indirect-trigger to dts files that use cadence driver
>>
>> I think that captures all the changes needed.
>>
>> Also, for the naming of the DT property, Linux has uses a 'cdns,' prefix
>> (e.g. cdns,trigger-address) for a number of their properties (cdns,tshsl-ns,
>> cdns,tsd2d-ns, etc.), but u-boot does not currently use the 'cdns,' prefix for
>> the same properties.  Do you want the 'cdns,' prefix on trigger-address?
>> If so, do you want me to rename all the other properties to align them with
>> the Linux DT as an additional patch?
>>

--
Regards,
Jason

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

[Vignesh R]

On Tuesday 28 February 2017 10:36 PM, Rush, Jason A. wrote:
> R, Vignesh wrote:
>> On 2/28/2017 8:38 PM, Rush, Jason A. wrote:
>> [...]
>>>
>>> This also works.
>>>
>>> Marek - how do you feel about a patch series with the following:
>>>
>>> 1. revert commit 57897c13de03ac0136d64641a3eab526c6810387
>>>      spi: cadence_qspi_apb: Use 32 bit indirect write transaction when possible
>>> 2. revert commit b63b46313ed29e9b0c36b3d6b9407f6eade40c8f
>>>      spi: cadence_qspi_apb: Use 32 bit indirect read transaction when possible
>>> 3. Apply my slightly modified version of https://patchwork.ozlabs.org/patch/693069/
>>>      (should I keep Vignesh as the signed-off?)
>>
>> Depends on how much you have changed the code. If the change is
>> significant then change the authorship to you and drop my signed-off.
>> Else, keep the authorship and signed-off.
>> If you are adding something new to the patch like adding code to make
>> sure that only 32bit data reads are issued, then I suggest you to submit
>> that change as separate patch.
> 
> Very minimal.  Another commit changed a #define from
> CQSPI_REG_INDIRECTWR_START_MASK to CQSPI_REG_INDIRECTWR_START,
> so I had to modify the patch to drop the _MASK so it would apply.  Other
> than that, it's identical in content.
> 

In that case you will have to retain my authorship and signed-off by and
add your signed-off by below that. Thanks!


-- 
Regards
Vignesh