[U-Boot] [v3 3/4] spi: cadence_qspi: fix base trigger address & transfer start address

vikas vikas.manocha at st.com
Fri Aug 21 00:25:54 CEST 2015


Hi,

On 08/20/2015 02:56 PM, Marek Vasut wrote:
> On Thursday, August 20, 2015 at 06:48:36 PM, vikas wrote:
>> Hi,
>>
>> On 08/19/2015 08:54 PM, Marek Vasut wrote:
>>> On Saturday, August 15, 2015 at 04:15:59 AM, Vikas Manocha wrote:
>>>> This patch is to separate the base trigger from the read/write transfer
>>>> start addresses.
>>>>
>>>> Base trigger register address (0x1c register) corresponds to the address
>>>> which should be put on AHB bus to handle indirect transfer triggered
>>>> before.
>>>>
>>>> To handle indirect transfer we need to issue addresses from (value of
>>>> 0x1c) to (value of 0x1c) + 15*4 ("4" corresponds to size of SRAM
>>>> location). There are no obstacles in issuing const address just equal
>>>> to 0x1c. Important thing to note is that indirect trigger address has
>>>> nothing in common with your physical or mapped NOR Flash address.
>>>>
>>>> Transfer read/write start addresses (offset 0x68/0x78)should be
>>>> programmed with the absolute flash address to be read/written.
>>>>
>>>> plat->ahbbase has been renamed to plat->flashbase for clarity.
>>>> plat->triggerbase is added in device tree for mapped spi flash address.
>>>>
>>>> Signed-off-by: Vikas Manocha <vikas.manocha at st.com>
>>>> ---
>>>>
>>>> Changes in v3: formatted string breaking fixed.
>>>> Changes in v2: Rebased to master
>>>>
>>>>  arch/arm/dts/socfpga.dtsi      |    3 ++-
>>>>  arch/arm/dts/stv0991.dts       |    3 ++-
>>>>  drivers/spi/cadence_qspi.c     |   14 +++++++-------
>>>>  drivers/spi/cadence_qspi.h     |    5 +++--
>>>>  drivers/spi/cadence_qspi_apb.c |   11 +++++------
>>>>  5 files changed, 19 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/arch/arm/dts/socfpga.dtsi b/arch/arm/dts/socfpga.dtsi
>>>> index 9b12420..25053ec 100644
>>>> --- a/arch/arm/dts/socfpga.dtsi
>>>> +++ b/arch/arm/dts/socfpga.dtsi
>>>> @@ -633,7 +633,8 @@
>>>>
>>>>  			#address-cells = <1>;
>>>>  			#size-cells = <0>;
>>>>  			reg = <0xff705000 0x1000>,
>>>>
>>>> -				<0xffa00000 0x1000>;
>>>> +				<0xffa00000 0x1000>,
>>>> +				<0xffa00000 0x0010>;
>>>
>>> Can you please follow the DT bindings Graham and me sent for Linux kernel
>>> instead of adding ad-hoc undocumented stuff ?
>>
>> It is aligned to current DT binding of u-boot.
>> I agree to align it to linux kernel DT binding but that should be a
>> separate patch.
> 
> And this separate patch should come in FIRST, we do not want to support two
> sets of bindings, no way.

As you know linux DT binding is under review & current linux patch needs to be fixed. It does not make sense to
support something which is under review & discussion are ongoing for it.

> 
>>>>  			interrupts = <0 151 4>;
>>>>  			clocks = <&qspi_clk>;
>>>>  			ext-decoder = <0>;  /* external decoder */
>>>>
>>>> diff --git a/arch/arm/dts/stv0991.dts b/arch/arm/dts/stv0991.dts
>>>> index fa3fd64..e23d4fd 100644
>>>> --- a/arch/arm/dts/stv0991.dts
>>>> +++ b/arch/arm/dts/stv0991.dts
>>>> @@ -30,7 +30,8 @@
>>>>
>>>>  			#address-cells = <1>;
>>>>  			#size-cells = <0>;
>>>>  			reg = <0x80203000 0x100>,
>>>>
>>>> -				<0x40000000 0x1000000>;
>>>> +				<0x40000000 0x1000000>,
>>>> +				<0x40000000 0x0000010>;
>>>>
>>>>  			clocks = <3750000>;
>>>>  			sram-size = <256>;
>>>>  			status = "okay";
>>>>
>>>> diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c
>>>> index 34a0f46..01eff71 100644
>>>> --- a/drivers/spi/cadence_qspi.c
>>>> +++ b/drivers/spi/cadence_qspi.c
>>>> @@ -150,7 +150,7 @@ static int cadence_spi_probe(struct udevice *bus)
>>>>
>>>>  	struct cadence_spi_priv *priv = dev_get_priv(bus);
>>>>  	
>>>>  	priv->regbase = plat->regbase;
>>>>
>>>> -	priv->ahbbase = plat->ahbbase;
>>>> +	priv->flashbase = plat->flashbase;
>>>
>>> If you need to rename anything, it should be in a separate patch. Please
>>> keep it separate.
>>
>> this renaming is done here for more clarity as the earlier ahbbase usage is
>> split between "flashbase" & "trigger base". ahbbase is not relevant
>> anymore.
> 
> Which is why it should happen in a separate patch :)

Hmmm.. Not adding any value but i will do it.

> 
>>>>  	if (!priv->qspi_is_init) {
>>>>  	
>>>>  		cadence_qspi_apb_controller_init(plat);
>>>>
>>>> @@ -278,7 +278,7 @@ static int cadence_spi_ofdata_to_platdata(struct
>>>> udevice *bus) const void *blob = gd->fdt_blob;
>>>>
>>>>  	int node = bus->of_offset;
>>>>  	int subnode;
>>>>
>>>> -	u32 data[4];
>>>> +	u32 data[6];
>>>>
>>>>  	int ret;
>>>>  	
>>>>  	/* 2 base addresses are needed, lets get them from the DT */
>>>>
>>>> @@ -289,7 +289,8 @@ static int cadence_spi_ofdata_to_platdata(struct
>>>> udevice *bus) }
>>>>
>>>>  	plat->regbase = (void *)data[0];
>>>>
>>>> -	plat->ahbbase = (void *)data[2];
>>>> +	plat->flashbase = (void *)data[2];
>>>> +	plat->trigger_base = (void *)data[4];
>>>
>>> Please switch this to fdtdec_get_addr() instead ; if you use the Linux
>>> binding, you will be able to do this easily.
>>
>> As above, should be separate patch to align to Linux binding.
> 
> Correct ; and it should go in before this one. We don't want to add crap
> we know about first and fix it afterward, we do it the other way around.
> First we remove the crap which is in the way, then we add features.

ditto.

> 
>>>>  	/* Use 500KHz as a suitable default */
>>>>  	plat->max_hz = fdtdec_get_int(blob, node, "spi-max-frequency",
>>>>
>>>> @@ -311,10 +312,9 @@ static int cadence_spi_ofdata_to_platdata(struct
>>>> udevice *bus) plat->tslch_ns = fdtdec_get_int(blob, subnode, "tslch-ns",
>>>> 20);
>>>>
>>>>  	plat->sram_size = fdtdec_get_int(blob, node, "sram-size", 128);
>>>>
>>>> -	debug("%s: regbase=%p ahbbase=%p max-frequency=%d page-size=%d\n",
>>>> -	      __func__, plat->regbase, plat->ahbbase, plat->max_hz,
>>>> -	      plat->page_size);
>>>> -
>>>> +	debug("%s: regbase=%p flashbase=%p trigger_base=%p max-frequency=%d
>>>> page-size=%d\n", +		__func__, plat->regbase, plat->flashbase,
>>>> plat->trigger_base,
>>>> +		plat->max_hz, plat->page_size);
>>>
>>> Please don't break the indent of the args. The original debug() statement
>>> had the args correctly indented ; this patch breaks the indent.
>>
>> I think it looks like this here as the line length is more than 80 chars
>> for args. After applying patch it looks ok. If not, please let me know how
>> to fix it.
> 
> I'm talking about the change from 6 spaces to one tab in front of the args.

ok.

Cheers,
Vikas

> 
>>>>  	return 0;
>>>>  
>>>>  }
>>>>
>>>> diff --git a/drivers/spi/cadence_qspi.h b/drivers/spi/cadence_qspi.h
>>>> index 98e57aa..7341339 100644
>>>> --- a/drivers/spi/cadence_qspi.h
>>>> +++ b/drivers/spi/cadence_qspi.h
>>>> @@ -17,7 +17,8 @@
>>>>
>>>>  struct cadence_spi_platdata {
>>>>  
>>>>  	unsigned int	max_hz;
>>>>  	void		*regbase;
>>>>
>>>> -	void		*ahbbase;
>>>> +	void		*flashbase;
>>>> +	void		*trigger_base;
>>>>
>>>>  	u32		page_size;
>>>>  	u32		block_size;
>>>>
>>>> @@ -30,7 +31,7 @@ struct cadence_spi_platdata {
>>>>
>>>>  struct cadence_spi_priv {
>>>>  
>>>>  	void		*regbase;
>>>>
>>>> -	void		*ahbbase;
>>>> +	void		*flashbase;
>>>>
>>>>  	size_t		cmd_len;
>>>>  	u8		cmd_buf[32];
>>>>  	size_t		data_len;
>>>>
>>>> diff --git a/drivers/spi/cadence_qspi_apb.c
>>>> b/drivers/spi/cadence_qspi_apb.c index 6b5ae30..25a7ed2 100644
>>>> --- a/drivers/spi/cadence_qspi_apb.c
>>>> +++ b/drivers/spi/cadence_qspi_apb.c
>>>> @@ -44,7 +44,6 @@
>>>>
>>>>  #define CQSPI_INST_TYPE_QUAD			(2)
>>>>  
>>>>  #define CQSPI_STIG_DATA_LEN_MAX			(8)
>>>>
>>>> -#define CQSPI_INDIRECTTRIGGER_ADDR_MASK		(0xFFFFF)
>>>>
>>>>  #define CQSPI_DUMMY_CLKS_PER_BYTE		(8)
>>>>  #define CQSPI_DUMMY_BYTES_MAX			(4)
>>>>
>>>> @@ -281,7 +280,7 @@ static int qpsi_write_sram_fifo_push(struct
>>>> cadence_spi_platdata *plat, const void *src_addr, unsigned int
>>>> num_bytes)
>>>>
>>>>  {
>>>>  
>>>>  	const void *reg_base = plat->regbase;
>>>>
>>>> -	void *dest_addr = plat->ahbbase;
>>>> +	void *dest_addr = plat->trigger_base;
>>>>
>>>>  	unsigned int retry = CQSPI_REG_RETRY;
>>>>  	unsigned int sram_level;
>>>>  	unsigned int wr_bytes;
>>>>
>>>> @@ -534,7 +533,7 @@ void cadence_qspi_apb_controller_init(struct
>>>> cadence_spi_platdata *plat)
>>>>
>>>>  	/* Indirect mode configurations */
>>>>  	writel((plat->sram_size/2), plat->regbase + CQSPI_REG_SRAMPARTITION);
>>>>
>>>> -	writel((u32)plat->ahbbase & CQSPI_INDIRECTTRIGGER_ADDR_MASK,
>>>> +	writel((u32)plat->trigger_base,
>>>>
>>>>  				plat->regbase + CQSPI_REG_INDIRECTTRIGGER);
>>>
>>> Here you actually changed to logic of the code, which breaks it for
>>> SoCFPGA. plat->ahbbase & CQSPI_INDIRECTTRIGGER_ADDR_MASK = 0x0 for
>>> SoCFPGA, but now you changed it such that 0xffa00000 is written into the
>>> register. Same does apply for all your changes below.
>>
>> If we pass 0x00 from device tree, 0x0 would be written in the register. We
>> have to check on SOCFpga platform is 0xFFA0_0000 is correct value or only
>> 0x0. Anyways i will change the DT value to 0x0 for SOCFpga in next
>> version.
> 
> Excellent.
> .
> 


More information about the U-Boot mailing list