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

[Marek Vasut]

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.

> >>  			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 :)

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

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

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