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

[Marek Vasut]

On Friday, August 21, 2015 at 12:25:54 AM, vikas wrote:
> Hi,

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.

So you propose that in the meantime, we grow our own, completely different,
in-house set of binding instead of using the ones which are currently being
reviewed for inclusion into Linux kernel. The only comments on those bindings
are from you and are about wording of some piece of text, not even about the
binding themselves.

No, sorry, I'm not buying this.

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

The value is in keeping separate changes separate, reviewable and bisectable.
You might want to read up on this.

[...]