[U-Boot] [PATCH 1/4] spi: cadence_qspi: Ensure spi_calibration is run when sclk change

[U-Boot] [PATCH 1/4] spi: cadence_qspi: Ensure spi_calibration is run when sclk change

[Chin Liang See]

Ensuring spi_calibration is run when there is a change of sclk
frequency. This will ensure the qspi flash access works for high
sclk frequency

Signed-off-by: Chin Liang See <clsee@altera.com>
Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
Cc: Dinh Nguyen <dinh.linux@gmail.com>
Cc: Marek Vasut <marex@denx.de>
Cc: Stefan Roese <sr@denx.de>
Cc: Vikas Manocha <vikas.manocha@st.com>
Cc: Jagannadh Teki <jteki@openedev.com>
Cc: Pavel Machek <pavel@denx.de>
---
 drivers/spi/cadence_qspi.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c
index 34a0f46..300934e 100644
--- a/drivers/spi/cadence_qspi.c
+++ b/drivers/spi/cadence_qspi.c
@@ -37,7 +37,7 @@ static int cadence_spi_write_speed(struct udevice *bus, uint hz)
 }
 
 /* Calibration sequence to determine the read data capture delay register */
-static int spi_calibration(struct udevice *bus)
+static int spi_calibration(struct udevice *bus, uint hz)
 {
 	struct cadence_spi_platdata *plat = bus->platdata;
 	struct cadence_spi_priv *priv = dev_get_priv(bus);
@@ -64,7 +64,7 @@ static int spi_calibration(struct udevice *bus)
 	}
 
 	/* use back the intended clock and find low range */
-	cadence_spi_write_speed(bus, plat->max_hz);
+	cadence_spi_write_speed(bus, hz);
 	for (i = 0; i < CQSPI_READ_CAPTURE_MAX_DELAY; i++) {
 		/* Disable QSPI */
 		cadence_qspi_apb_controller_disable(base);
@@ -111,7 +111,7 @@ static int spi_calibration(struct udevice *bus)
 	      (range_hi + range_lo) / 2, range_lo, range_hi);
 
 	/* just to ensure we do once only when speed or chip select change */
-	priv->qspi_calibrated_hz = plat->max_hz;
+	priv->qspi_calibrated_hz = hz;
 	priv->qspi_calibrated_cs = spi_chip_select(bus);
 
 	return 0;
@@ -131,7 +131,7 @@ static int cadence_spi_set_speed(struct udevice *bus, uint hz)
 	/* Calibration required for different SCLK speed or chip select */
 	if (priv->qspi_calibrated_hz != plat->max_hz ||
 	    priv->qspi_calibrated_cs != spi_chip_select(bus)) {
-		err = spi_calibration(bus);
+		err = spi_calibration(bus, hz);
 		if (err)
 			return err;
 	}
-- 
1.7.7.4

[U-Boot] [PATCH 2/4] spi: cadence_qspi: Fix fdt read of spi-max-frequency

[Chin Liang See]

Fix the fdt read for spi-max-frequency as it's contained
in the child node. Current state of code is always
returning default value.

Signed-off-by: Chin Liang See <clsee@altera.com>
Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
Cc: Dinh Nguyen <dinh.linux@gmail.com>
Cc: Marek Vasut <marex@denx.de>
Cc: Stefan Roese <sr@denx.de>
Cc: Vikas Manocha <vikas.manocha@st.com>
Cc: Jagannadh Teki <jteki@openedev.com>
Cc: Pavel Machek <pavel@denx.de>
---
 drivers/spi/cadence_qspi.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c
index 300934e..a00af87 100644
--- a/drivers/spi/cadence_qspi.c
+++ b/drivers/spi/cadence_qspi.c
@@ -291,10 +291,6 @@ static int cadence_spi_ofdata_to_platdata(struct udevice *bus)
 	plat->regbase = (void *)data[0];
 	plat->ahbbase = (void *)data[2];
 
-	/* Use 500KHz as a suitable default */
-	plat->max_hz = fdtdec_get_int(blob, node, "spi-max-frequency",
-				      500000);
-
 	/* All other paramters are embedded in the child node */
 	subnode = fdt_first_subnode(blob, node);
 	if (subnode < 0) {
@@ -302,6 +298,10 @@ static int cadence_spi_ofdata_to_platdata(struct udevice *bus)
 		return -ENODEV;
 	}
 
+	/* Use 500KHz as a suitable default */
+	plat->max_hz = fdtdec_get_int(blob, subnode, "spi-max-frequency",
+				      500000);
+
 	/* Read other parameters from DT */
 	plat->page_size = fdtdec_get_int(blob, subnode, "page-size", 256);
 	plat->block_size = fdtdec_get_int(blob, subnode, "block-size", 16);
-- 
1.7.7.4

[U-Boot] [PATCH 3/4] spi: cadence_qspi: Ensure check for max frequency in place

[Chin Liang See]

Ensure the intended SCLK frequency not exceeding the maximum
frequency. If that happen, SCLK will set to maximum frequency.

Signed-off-by: Chin Liang See <clsee@altera.com>
Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
Cc: Dinh Nguyen <dinh.linux@gmail.com>
Cc: Marek Vasut <marex@denx.de>
Cc: Stefan Roese <sr@denx.de>
Cc: Vikas Manocha <vikas.manocha@st.com>
Cc: Jagannadh Teki <jteki@openedev.com>
Cc: Pavel Machek <pavel@denx.de>
---
 drivers/spi/cadence_qspi.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c
index a00af87..52c29d5 100644
--- a/drivers/spi/cadence_qspi.c
+++ b/drivers/spi/cadence_qspi.c
@@ -123,6 +123,11 @@ static int cadence_spi_set_speed(struct udevice *bus, uint hz)
 	struct cadence_spi_priv *priv = dev_get_priv(bus);
 	int err;
 
+	if (hz > plat->max_hz) {
+		hz = plat->max_hz;
+		puts("SF: Default to maximum supported SCLK frequency\n");
+	}
+
 	/* Disable QSPI */
 	cadence_qspi_apb_controller_disable(priv->regbase);
 
-- 
1.7.7.4

[U-Boot] [PATCH 4/4] arm: dts: socfpga: Increase the spi-max-frequency for QSPI flash

[Chin Liang See]

With a working QSPI calibration, the SCLK can now run up to 100MHz

Signed-off-by: Chin Liang See <clsee@altera.com>
Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
Cc: Dinh Nguyen <dinh.linux@gmail.com>
Cc: Marek Vasut <marex@denx.de>
Cc: Stefan Roese <sr@denx.de>
Cc: Vikas Manocha <vikas.manocha@st.com>
Cc: Jagannadh Teki <jteki@openedev.com>
Cc: Pavel Machek <pavel@denx.de>
---
 arch/arm/dts/socfpga_cyclone5_socdk.dts |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/arm/dts/socfpga_cyclone5_socdk.dts b/arch/arm/dts/socfpga_cyclone5_socdk.dts
index 9650eb0..04e5695 100644
--- a/arch/arm/dts/socfpga_cyclone5_socdk.dts
+++ b/arch/arm/dts/socfpga_cyclone5_socdk.dts
@@ -86,7 +86,7 @@
 		#size-cells = <1>;
 		compatible = "n25q00";
 		reg = <0>;      /* chip select */
-		spi-max-frequency = <50000000>;
+		spi-max-frequency = <100000000>;
 		m25p,fast-read;
 		page-size = <256>;
 		block-size = <16>; /* 2^16, 64KB */
-- 
1.7.7.4

[U-Boot] [PATCH 1/4] spi: cadence_qspi: Ensure spi_calibration is run when sclk change

[Marek Vasut]

On Thursday, September 03, 2015 at 03:41:59 PM, Chin Liang See wrote:
> Ensuring spi_calibration is run when there is a change of sclk
> frequency. This will ensure the qspi flash access works for high
> sclk frequency
> 
> Signed-off-by: Chin Liang See <clsee@altera.com>
> Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
> Cc: Dinh Nguyen <dinh.linux@gmail.com>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Stefan Roese <sr@denx.de>
> Cc: Vikas Manocha <vikas.manocha@st.com>
> Cc: Jagannadh Teki <jteki@openedev.com>
> Cc: Pavel Machek <pavel@denx.de>
> ---
>  drivers/spi/cadence_qspi.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c
> index 34a0f46..300934e 100644
> --- a/drivers/spi/cadence_qspi.c
> +++ b/drivers/spi/cadence_qspi.c
> @@ -37,7 +37,7 @@ static int cadence_spi_write_speed(struct udevice *bus,
> uint hz) }
> 
>  /* Calibration sequence to determine the read data capture delay register
> */ -static int spi_calibration(struct udevice *bus)
> +static int spi_calibration(struct udevice *bus, uint hz)
>  {
>  	struct cadence_spi_platdata *plat = bus->platdata;
>  	struct cadence_spi_priv *priv = dev_get_priv(bus);
> @@ -64,7 +64,7 @@ static int spi_calibration(struct udevice *bus)
>  	}
> 
>  	/* use back the intended clock and find low range */
> -	cadence_spi_write_speed(bus, plat->max_hz);
> +	cadence_spi_write_speed(bus, hz);
>  	for (i = 0; i < CQSPI_READ_CAPTURE_MAX_DELAY; i++) {
>  		/* Disable QSPI */
>  		cadence_qspi_apb_controller_disable(base);
> @@ -111,7 +111,7 @@ static int spi_calibration(struct udevice *bus)
>  	      (range_hi + range_lo) / 2, range_lo, range_hi);
> 
>  	/* just to ensure we do once only when speed or chip select change */
> -	priv->qspi_calibrated_hz = plat->max_hz;
> +	priv->qspi_calibrated_hz = hz;
>  	priv->qspi_calibrated_cs = spi_chip_select(bus);
> 
>  	return 0;
> @@ -131,7 +131,7 @@ static int cadence_spi_set_speed(struct udevice *bus,

Hi,

My impression is that the logic here should be like this:

if (hz > plat->max_hz) {
    printf("error, freq. too high");
    return -EINVAL;
}
if (priv->previous_hz != hz) /* Bus frequency changed, re-calibrate */
    spi_calibrate(bus, hz)
cadence_spi_write_speed(bus, priv->qspi_calibrated_hz);
priv->previous_hz = hz;

Note that you need a new private variable, previous_hz, to hold the previous
value of "hz". This is needed since the calibrated frequency might not be
equal to requested frequency.

> uint hz) /* Calibration required for different SCLK speed or chip select
> */ if (priv->qspi_calibrated_hz != plat->max_hz ||
>  	    priv->qspi_calibrated_cs != spi_chip_select(bus)) {
> -		err = spi_calibration(bus);
> +		err = spi_calibration(bus, hz);

This is called after the frequency is configured in this function, this looks
really backwards.

>  		if (err)
>  			return err;
>  	}

Best regards,
Marek Vasut

[U-Boot] [PATCH 2/4] spi: cadence_qspi: Fix fdt read of spi-max-frequency

[Marek Vasut]

On Thursday, September 03, 2015 at 03:42:00 PM, Chin Liang See wrote:
> Fix the fdt read for spi-max-frequency as it's contained
> in the child node. Current state of code is always
> returning default value.
> 
> Signed-off-by: Chin Liang See <clsee@altera.com>
> Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
> Cc: Dinh Nguyen <dinh.linux@gmail.com>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Stefan Roese <sr@denx.de>
> Cc: Vikas Manocha <vikas.manocha@st.com>
> Cc: Jagannadh Teki <jteki@openedev.com>
> Cc: Pavel Machek <pavel@denx.de>
> ---
>  drivers/spi/cadence_qspi.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c
> index 300934e..a00af87 100644
> --- a/drivers/spi/cadence_qspi.c
> +++ b/drivers/spi/cadence_qspi.c
> @@ -291,10 +291,6 @@ static int cadence_spi_ofdata_to_platdata(struct
> udevice *bus) plat->regbase = (void *)data[0];
>  	plat->ahbbase = (void *)data[2];
> 
> -	/* Use 500KHz as a suitable default */
> -	plat->max_hz = fdtdec_get_int(blob, node, "spi-max-frequency",
> -				      500000);
> -
>  	/* All other paramters are embedded in the child node */
>  	subnode = fdt_first_subnode(blob, node);
>  	if (subnode < 0) {
> @@ -302,6 +298,10 @@ static int cadence_spi_ofdata_to_platdata(struct
> udevice *bus) return -ENODEV;
>  	}
> 
> +	/* Use 500KHz as a suitable default */
> +	plat->max_hz = fdtdec_get_int(blob, subnode, "spi-max-frequency",
> +				      500000);

Use fdtdec_get_u32() or such, since the value is unsigned int (have you ever
seen negative frequency ? ;-) ). Then check the fdtdec_get_u32() return value.

>  	/* Read other parameters from DT */
>  	plat->page_size = fdtdec_get_int(blob, subnode, "page-size", 256);
>  	plat->block_size = fdtdec_get_int(blob, subnode, "block-size", 16);

Best regards,
Marek Vasut

[U-Boot] [PATCH 4/4] arm: dts: socfpga: Increase the spi-max-frequency for QSPI flash

[Marek Vasut]

On Thursday, September 03, 2015 at 03:42:02 PM, Chin Liang See wrote:
> With a working QSPI calibration, the SCLK can now run up to 100MHz
> 
> Signed-off-by: Chin Liang See <clsee@altera.com>
> Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
> Cc: Dinh Nguyen <dinh.linux@gmail.com>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Stefan Roese <sr@denx.de>
> Cc: Vikas Manocha <vikas.manocha@st.com>
> Cc: Jagannadh Teki <jteki@openedev.com>
> Cc: Pavel Machek <pavel@denx.de>

Reviewed-by: Marek Vasut <marex@denx.de>

Best regards,
Marek Vasut

[U-Boot] [PATCH 1/4] spi: cadence_qspi: Ensure spi_calibration is run when sclk change

[Chin Liang See]

Hi,

On Thu, 2015-09-03 at 16:17 +0200, marex at denx.de wrote:
> On Thursday, September 03, 2015 at 03:41:59 PM, Chin Liang See wrote:
> > Ensuring spi_calibration is run when there is a change of sclk
> > frequency. This will ensure the qspi flash access works for high
> > sclk frequency
> > 
> > Signed-off-by: Chin Liang See <clsee@altera.com>
> > Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
> > Cc: Dinh Nguyen <dinh.linux@gmail.com>
> > Cc: Marek Vasut <marex@denx.de>
> > Cc: Stefan Roese <sr@denx.de>
> > Cc: Vikas Manocha <vikas.manocha@st.com>
> > Cc: Jagannadh Teki <jteki@openedev.com>
> > Cc: Pavel Machek <pavel@denx.de>
> > ---
> >  drivers/spi/cadence_qspi.c |    8 ++++----
> >  1 files changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c
> > index 34a0f46..300934e 100644
> > --- a/drivers/spi/cadence_qspi.c
> > +++ b/drivers/spi/cadence_qspi.c
> > @@ -37,7 +37,7 @@ static int cadence_spi_write_speed(struct udevice *bus,
> > uint hz) }
> > 
> >  /* Calibration sequence to determine the read data capture delay register
> > */ -static int spi_calibration(struct udevice *bus)
> > +static int spi_calibration(struct udevice *bus, uint hz)
> >  {
> >  	struct cadence_spi_platdata *plat = bus->platdata;
> >  	struct cadence_spi_priv *priv = dev_get_priv(bus);
> > @@ -64,7 +64,7 @@ static int spi_calibration(struct udevice *bus)
> >  	}
> > 
> >  	/* use back the intended clock and find low range */
> > -	cadence_spi_write_speed(bus, plat->max_hz);
> > +	cadence_spi_write_speed(bus, hz);
> >  	for (i = 0; i < CQSPI_READ_CAPTURE_MAX_DELAY; i++) {
> >  		/* Disable QSPI */
> >  		cadence_qspi_apb_controller_disable(base);
> > @@ -111,7 +111,7 @@ static int spi_calibration(struct udevice *bus)
> >  	      (range_hi + range_lo) / 2, range_lo, range_hi);
> > 
> >  	/* just to ensure we do once only when speed or chip select change */
> > -	priv->qspi_calibrated_hz = plat->max_hz;
> > +	priv->qspi_calibrated_hz = hz;
> >  	priv->qspi_calibrated_cs = spi_chip_select(bus);
> > 
> >  	return 0;
> > @@ -131,7 +131,7 @@ static int cadence_spi_set_speed(struct udevice *bus,
> 
> Hi,
> 
> My impression is that the logic here should be like this:
> 
> if (hz > plat->max_hz) {
>     printf("error, freq. too high");
>     return -EINVAL;
> }
> if (priv->previous_hz != hz) /* Bus frequency changed, re-calibrate */
>     spi_calibrate(bus, hz)
> cadence_spi_write_speed(bus, priv->qspi_calibrated_hz);
> priv->previous_hz = hz;
> 
> Note that you need a new private variable, previous_hz, to hold the previous
> value of "hz". This is needed since the calibrated frequency might not be
> equal to requested frequency.
> 

I know where you coming from. 
Yah, calibration can be skipped when requested frequency same as
previous or calibrated one.
Let me add that.

> > uint hz) /* Calibration required for different SCLK speed or chip select
> > */ if (priv->qspi_calibrated_hz != plat->max_hz ||
> >  	    priv->qspi_calibrated_cs != spi_chip_select(bus)) {
> > -		err = spi_calibration(bus);
> > +		err = spi_calibration(bus, hz);
> 
> This is called after the frequency is configured in this function, this looks
> really backwards.

Yah, let me remove the old code.

Thanks
Chin Liang

> 
> >  		if (err)
> >  			return err;
> >  	}
> 
> Best regards,
> Marek Vasut

[U-Boot] [PATCH 2/4] spi: cadence_qspi: Fix fdt read of spi-max-frequency

[Chin Liang See]

Hi,

On Thu, 2015-09-03 at 16:19 +0200, marex at denx.de wrote:
> On Thursday, September 03, 2015 at 03:42:00 PM, Chin Liang See wrote:
> > Fix the fdt read for spi-max-frequency as it's contained
> > in the child node. Current state of code is always
> > returning default value.
> > 
> > Signed-off-by: Chin Liang See <clsee@altera.com>
> > Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
> > Cc: Dinh Nguyen <dinh.linux@gmail.com>
> > Cc: Marek Vasut <marex@denx.de>
> > Cc: Stefan Roese <sr@denx.de>
> > Cc: Vikas Manocha <vikas.manocha@st.com>
> > Cc: Jagannadh Teki <jteki@openedev.com>
> > Cc: Pavel Machek <pavel@denx.de>
> > ---
> >  drivers/spi/cadence_qspi.c |    8 ++++----
> >  1 files changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c
> > index 300934e..a00af87 100644
> > --- a/drivers/spi/cadence_qspi.c
> > +++ b/drivers/spi/cadence_qspi.c
> > @@ -291,10 +291,6 @@ static int cadence_spi_ofdata_to_platdata(struct
> > udevice *bus) plat->regbase = (void *)data[0];
> >  	plat->ahbbase = (void *)data[2];
> > 
> > -	/* Use 500KHz as a suitable default */
> > -	plat->max_hz = fdtdec_get_int(blob, node, "spi-max-frequency",
> > -				      500000);
> > -
> >  	/* All other paramters are embedded in the child node */
> >  	subnode = fdt_first_subnode(blob, node);
> >  	if (subnode < 0) {
> > @@ -302,6 +298,10 @@ static int cadence_spi_ofdata_to_platdata(struct
> > udevice *bus) return -ENODEV;
> >  	}
> > 
> > +	/* Use 500KHz as a suitable default */
> > +	plat->max_hz = fdtdec_get_int(blob, subnode, "spi-max-frequency",
> > +				      500000);
> 
> Use fdtdec_get_u32() or such, since the value is unsigned int (have you ever
> seen negative frequency ? ;-) ). Then check the fdtdec_get_u32() return value.
> 

I git grep and no unsigned version.
But I can add unsigned casting to avoid that.

Thanks
Chin Liang

> >  	/* Read other parameters from DT */
> >  	plat->page_size = fdtdec_get_int(blob, subnode, "page-size", 256);
> >  	plat->block_size = fdtdec_get_int(blob, subnode, "block-size", 16);
> 
> Best regards,
> Marek Vasut

[U-Boot] [PATCH 2/4] spi: cadence_qspi: Fix fdt read of spi-max-frequency

[Chin Liang See]

Fix the fdt read for spi-max-frequency as it's contained
in the child node. Current state of code is always
returning default value.

Signed-off-by: Chin Liang See <clsee@altera.com>
Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
Cc: Dinh Nguyen <dinh.linux@gmail.com>
Cc: Marek Vasut <marex@denx.de>
Cc: Stefan Roese <sr@denx.de>
Cc: Vikas Manocha <vikas.manocha@st.com>
Cc: Jagannadh Teki <jteki@openedev.com>
Cc: Pavel Machek <pavel@denx.de>
---
Changes for v2
- Add unsigned casting for plat->max_hz
---
 drivers/spi/cadence_qspi.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c
index 23c88d5..8c0f7dd 100644
--- a/drivers/spi/cadence_qspi.c
+++ b/drivers/spi/cadence_qspi.c
@@ -296,10 +296,6 @@ static int cadence_spi_ofdata_to_platdata(struct udevice *bus)
 	plat->regbase = (void *)data[0];
 	plat->ahbbase = (void *)data[2];
 
-	/* Use 500KHz as a suitable default */
-	plat->max_hz = fdtdec_get_int(blob, node, "spi-max-frequency",
-				      500000);
-
 	/* All other paramters are embedded in the child node */
 	subnode = fdt_first_subnode(blob, node);
 	if (subnode < 0) {
@@ -307,6 +303,10 @@ static int cadence_spi_ofdata_to_platdata(struct udevice *bus)
 		return -ENODEV;
 	}
 
+	/* Use 500KHz as a suitable default */
+	plat->max_hz = (unsigned int)fdtdec_get_int(blob, subnode,
+						"spi-max-frequency", 500000);
+
 	/* Read other parameters from DT */
 	plat->page_size = fdtdec_get_int(blob, subnode, "page-size", 256);
 	plat->block_size = fdtdec_get_int(blob, subnode, "block-size", 16);
-- 
1.7.7.4

[U-Boot] [PATCH 2/4] spi: cadence_qspi: Fix fdt read of spi-max-frequency

[Marek Vasut]

On Tuesday, September 08, 2015 at 03:16:58 AM, Chin Liang See wrote:
> Hi,

Hi!

> On Thu, 2015-09-03 at 16:19 +0200, marex at denx.de wrote:
> > On Thursday, September 03, 2015 at 03:42:00 PM, Chin Liang See wrote:
> > > Fix the fdt read for spi-max-frequency as it's contained
> > > in the child node. Current state of code is always
> > > returning default value.
> > > 
> > > Signed-off-by: Chin Liang See <clsee@altera.com>
> > > Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
> > > Cc: Dinh Nguyen <dinh.linux@gmail.com>
> > > Cc: Marek Vasut <marex@denx.de>
> > > Cc: Stefan Roese <sr@denx.de>
> > > Cc: Vikas Manocha <vikas.manocha@st.com>
> > > Cc: Jagannadh Teki <jteki@openedev.com>
> > > Cc: Pavel Machek <pavel@denx.de>
> > > ---
> > > 
> > >  drivers/spi/cadence_qspi.c |    8 ++++----
> > >  1 files changed, 4 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c
> > > index 300934e..a00af87 100644
> > > --- a/drivers/spi/cadence_qspi.c
> > > +++ b/drivers/spi/cadence_qspi.c
> > > @@ -291,10 +291,6 @@ static int cadence_spi_ofdata_to_platdata(struct
> > > udevice *bus) plat->regbase = (void *)data[0];
> > > 
> > >  	plat->ahbbase = (void *)data[2];
> > > 
> > > -	/* Use 500KHz as a suitable default */
> > > -	plat->max_hz = fdtdec_get_int(blob, node, "spi-max-frequency",
> > > -				      500000);
> > > -
> > > 
> > >  	/* All other paramters are embedded in the child node */
> > >  	subnode = fdt_first_subnode(blob, node);
> > >  	if (subnode < 0) {
> > > 
> > > @@ -302,6 +298,10 @@ static int cadence_spi_ofdata_to_platdata(struct
> > > udevice *bus) return -ENODEV;
> > > 
> > >  	}
> > > 
> > > +	/* Use 500KHz as a suitable default */
> > > +	plat->max_hz = fdtdec_get_int(blob, subnode, "spi-max-frequency",
> > > +				      500000);
> > 
> > Use fdtdec_get_u32() or such, since the value is unsigned int (have you
> > ever seen negative frequency ? ;-) ). Then check the fdtdec_get_u32()
> > return value.
> 
> I git grep and no unsigned version.
> But I can add unsigned casting to avoid that.

I think fdt_getprop_u32() is what you're looking for.

Best regards,
Marek Vasut

[U-Boot] [PATCH 2/4] spi: cadence_qspi: Fix fdt read of spi-max-frequency

[Jagan Teki]

On 8 September 2015 at 06:48, Chin Liang See <clsee@altera.com> wrote:
> Fix the fdt read for spi-max-frequency as it's contained
> in the child node. Current state of code is always
> returning default value.
>
> Signed-off-by: Chin Liang See <clsee@altera.com>
> Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
> Cc: Dinh Nguyen <dinh.linux@gmail.com>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Stefan Roese <sr@denx.de>
> Cc: Vikas Manocha <vikas.manocha@st.com>
> Cc: Jagannadh Teki <jteki@openedev.com>
> Cc: Pavel Machek <pavel@denx.de>
> ---
> Changes for v2
> - Add unsigned casting for plat->max_hz
> ---
>  drivers/spi/cadence_qspi.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c
> index 23c88d5..8c0f7dd 100644
> --- a/drivers/spi/cadence_qspi.c
> +++ b/drivers/spi/cadence_qspi.c
> @@ -296,10 +296,6 @@ static int cadence_spi_ofdata_to_platdata(struct udevice *bus)
>         plat->regbase = (void *)data[0];
>         plat->ahbbase = (void *)data[2];
>
> -       /* Use 500KHz as a suitable default */
> -       plat->max_hz = fdtdec_get_int(blob, node, "spi-max-frequency",
> -                                     500000);
> -
>         /* All other paramters are embedded in the child node */
>         subnode = fdt_first_subnode(blob, node);
>         if (subnode < 0) {
> @@ -307,6 +303,10 @@ static int cadence_spi_ofdata_to_platdata(struct udevice *bus)
>                 return -ENODEV;
>         }
>
> +       /* Use 500KHz as a suitable default */
> +       plat->max_hz = (unsigned int)fdtdec_get_int(blob, subnode,
> +                                               "spi-max-frequency", 500000);
> +

fdtdec_get_int will return default_val (say 500000 here) or property
value from dts which is usually "unsigned", unless wrongly set as -ve
value ie reason there is no typecasting used in rest of the code.

But, if still worried, better to add unsigned fdtdec_get_* instead of
type casting.

>         /* Read other parameters from DT */
>         plat->page_size = fdtdec_get_int(blob, subnode, "page-size", 256);
>         plat->block_size = fdtdec_get_int(blob, subnode, "block-size", 16);
> --
> 1.7.7.4
>

thanks!
-- 
Jagan | openedev.

[U-Boot] [PATCH 2/4] spi: cadence_qspi: Fix fdt read of spi-max-frequency

[Marek Vasut]

On Tuesday, September 08, 2015 at 03:18:24 AM, Chin Liang See wrote:
> Fix the fdt read for spi-max-frequency as it's contained
> in the child node. Current state of code is always
> returning default value.
> 
> Signed-off-by: Chin Liang See <clsee@altera.com>
> Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
> Cc: Dinh Nguyen <dinh.linux@gmail.com>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Stefan Roese <sr@denx.de>
> Cc: Vikas Manocha <vikas.manocha@st.com>
> Cc: Jagannadh Teki <jteki@openedev.com>
> Cc: Pavel Machek <pavel@denx.de>
> ---
> Changes for v2
> - Add unsigned casting for plat->max_hz
> ---
>  drivers/spi/cadence_qspi.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c
> index 23c88d5..8c0f7dd 100644
> --- a/drivers/spi/cadence_qspi.c
> +++ b/drivers/spi/cadence_qspi.c
> @@ -296,10 +296,6 @@ static int cadence_spi_ofdata_to_platdata(struct
> udevice *bus) plat->regbase = (void *)data[0];
>  	plat->ahbbase = (void *)data[2];
> 
> -	/* Use 500KHz as a suitable default */
> -	plat->max_hz = fdtdec_get_int(blob, node, "spi-max-frequency",
> -				      500000);
> -
>  	/* All other paramters are embedded in the child node */
>  	subnode = fdt_first_subnode(blob, node);
>  	if (subnode < 0) {
> @@ -307,6 +303,10 @@ static int cadence_spi_ofdata_to_platdata(struct
> udevice *bus) return -ENODEV;
>  	}
> 
> +	/* Use 500KHz as a suitable default */
> +	plat->max_hz = (unsigned int)fdtdec_get_int(blob, subnode,
> +						"spi-max-frequency", 500000);
> +

fdt_getprop_u32() might be the right thing to do here :)

Best regards,
Marek Vasut

[U-Boot] [PATCH 2/4] spi: cadence_qspi: Fix fdt read of spi-max-frequency

[Pavel Machek]

On Mon 2015-09-07 20:18:24, Chin Liang See wrote:
> Fix the fdt read for spi-max-frequency as it's contained
> in the child node. Current state of code is always
> returning default value.
> 
> Signed-off-by: Chin Liang See <clsee@altera.com>
> Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
> Cc: Dinh Nguyen <dinh.linux@gmail.com>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Stefan Roese <sr@denx.de>
> Cc: Vikas Manocha <vikas.manocha@st.com>
> Cc: Jagannadh Teki <jteki@openedev.com>
> Cc: Pavel Machek <pavel@denx.de>
> ---
> Changes for v2
> - Add unsigned casting for plat->max_hz
> ---
>  drivers/spi/cadence_qspi.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c
> index 23c88d5..8c0f7dd 100644
> --- a/drivers/spi/cadence_qspi.c
> +++ b/drivers/spi/cadence_qspi.c
> @@ -296,10 +296,6 @@ static int cadence_spi_ofdata_to_platdata(struct udevice *bus)
>  	plat->regbase = (void *)data[0];
>  	plat->ahbbase = (void *)data[2];
>  
> -	/* Use 500KHz as a suitable default */
> -	plat->max_hz = fdtdec_get_int(blob, node, "spi-max-frequency",
> -				      500000);
> -
>  	/* All other paramters are embedded in the child node */
>  	subnode = fdt_first_subnode(blob, node);
>  	if (subnode < 0) {
> @@ -307,6 +303,10 @@ static int cadence_spi_ofdata_to_platdata(struct udevice *bus)
>  		return -ENODEV;
>  	}
>  
> +	/* Use 500KHz as a suitable default */

And as you'll be updating it... it should be "500 kHz", AFAICT.


> +	plat->max_hz = (unsigned int)fdtdec_get_int(blob, subnode,
> +						"spi-max-frequency", 500000);
> +
>  	/* Read other parameters from DT */
>  	plat->page_size = fdtdec_get_int(blob, subnode, "page-size", 256);
>  	plat->block_size = fdtdec_get_int(blob, subnode, "block-size",
> 16);

									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[U-Boot] [PATCH 2/4] spi: cadence_qspi: Fix fdt read of spi-max-frequency

[Chin Liang See]

Hi,

On Tue, 2015-09-08 at 13:19 +0200, marex at denx.de wrote:
> On Tuesday, September 08, 2015 at 03:18:24 AM, Chin Liang See wrote:
> > Fix the fdt read for spi-max-frequency as it's contained
> > in the child node. Current state of code is always
> > returning default value.
> > 
> > Signed-off-by: Chin Liang See <clsee@altera.com>
> > Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
> > Cc: Dinh Nguyen <dinh.linux@gmail.com>
> > Cc: Marek Vasut <marex@denx.de>
> > Cc: Stefan Roese <sr@denx.de>
> > Cc: Vikas Manocha <vikas.manocha@st.com>
> > Cc: Jagannadh Teki <jteki@openedev.com>
> > Cc: Pavel Machek <pavel@denx.de>
> > ---
> > Changes for v2
> > - Add unsigned casting for plat->max_hz
> > ---
> >  drivers/spi/cadence_qspi.c |    8 ++++----
> >  1 files changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c
> > index 23c88d5..8c0f7dd 100644
> > --- a/drivers/spi/cadence_qspi.c
> > +++ b/drivers/spi/cadence_qspi.c
> > @@ -296,10 +296,6 @@ static int cadence_spi_ofdata_to_platdata(struct
> > udevice *bus) plat->regbase = (void *)data[0];
> >  	plat->ahbbase = (void *)data[2];
> > 
> > -	/* Use 500KHz as a suitable default */
> > -	plat->max_hz = fdtdec_get_int(blob, node, "spi-max-frequency",
> > -				      500000);
> > -
> >  	/* All other paramters are embedded in the child node */
> >  	subnode = fdt_first_subnode(blob, node);
> >  	if (subnode < 0) {
> > @@ -307,6 +303,10 @@ static int cadence_spi_ofdata_to_platdata(struct
> > udevice *bus) return -ENODEV;
> >  	}
> > 
> > +	/* Use 500KHz as a suitable default */
> > +	plat->max_hz = (unsigned int)fdtdec_get_int(blob, subnode,
> > +						"spi-max-frequency", 500000);
> > +
> 
> fdt_getprop_u32() might be the right thing to do here :)

Cool, yup, I am using fdt_getprop_u32_default_node. But need additional
patch to enable this and more FDT related function in SPL

Thanks
Chin Liang

> 
> Best regards,
> Marek Vasut

[U-Boot] [PATCH 2/4] spi: cadence_qspi: Fix fdt read of spi-max-frequency

[Chin Liang See]

Hi,

On Tue, 2015-09-08 at 15:39 +0530, Jagan Teki wrote:
> On 8 September 2015 at 06:48, Chin Liang See <clsee@altera.com> wrote:
> > Fix the fdt read for spi-max-frequency as it's contained
> > in the child node. Current state of code is always
> > returning default value.
> >
> > Signed-off-by: Chin Liang See <clsee@altera.com>
> > Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
> > Cc: Dinh Nguyen <dinh.linux@gmail.com>
> > Cc: Marek Vasut <marex@denx.de>
> > Cc: Stefan Roese <sr@denx.de>
> > Cc: Vikas Manocha <vikas.manocha@st.com>
> > Cc: Jagannadh Teki <jteki@openedev.com>
> > Cc: Pavel Machek <pavel@denx.de>
> > ---
> > Changes for v2
> > - Add unsigned casting for plat->max_hz
> > ---
> >  drivers/spi/cadence_qspi.c |    8 ++++----
> >  1 files changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c
> > index 23c88d5..8c0f7dd 100644
> > --- a/drivers/spi/cadence_qspi.c
> > +++ b/drivers/spi/cadence_qspi.c
> > @@ -296,10 +296,6 @@ static int cadence_spi_ofdata_to_platdata(struct udevice *bus)
> >         plat->regbase = (void *)data[0];
> >         plat->ahbbase = (void *)data[2];
> >
> > -       /* Use 500KHz as a suitable default */
> > -       plat->max_hz = fdtdec_get_int(blob, node, "spi-max-frequency",
> > -                                     500000);
> > -
> >         /* All other paramters are embedded in the child node */
> >         subnode = fdt_first_subnode(blob, node);
> >         if (subnode < 0) {
> > @@ -307,6 +303,10 @@ static int cadence_spi_ofdata_to_platdata(struct udevice *bus)
> >                 return -ENODEV;
> >         }
> >
> > +       /* Use 500KHz as a suitable default */
> > +       plat->max_hz = (unsigned int)fdtdec_get_int(blob, subnode,
> > +                                               "spi-max-frequency", 500000);
> > +
> 
> fdtdec_get_int will return default_val (say 500000 here) or property
> value from dts which is usually "unsigned", unless wrongly set as -ve
> value ie reason there is no typecasting used in rest of the code.
> 
> But, if still worried, better to add unsigned fdtdec_get_* instead of
> type casting.

Thanks to Marek, I will use fdt_getprop_u32_default_node to achieve
this :)

Thanks
Chin Liang

> 
> >         /* Read other parameters from DT */
> >         plat->page_size = fdtdec_get_int(blob, subnode, "page-size", 256);
> >         plat->block_size = fdtdec_get_int(blob, subnode, "block-size", 16);
> > --
> > 1.7.7.4
> >
> 
> thanks!

[U-Boot] [PATCH 2/4] spi: cadence_qspi: Fix fdt read of spi-max-frequency

[Chin Liang See]

Hi,

On Wed, 2015-09-09 at 09:16 +0200, ZY - pavel wrote:
> On Mon 2015-09-07 20:18:24, Chin Liang See wrote:
> > Fix the fdt read for spi-max-frequency as it's contained
> > in the child node. Current state of code is always
> > returning default value.
> > 
> > Signed-off-by: Chin Liang See <clsee@altera.com>
> > Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
> > Cc: Dinh Nguyen <dinh.linux@gmail.com>
> > Cc: Marek Vasut <marex@denx.de>
> > Cc: Stefan Roese <sr@denx.de>
> > Cc: Vikas Manocha <vikas.manocha@st.com>
> > Cc: Jagannadh Teki <jteki@openedev.com>
> > Cc: Pavel Machek <pavel@denx.de>
> > ---
> > Changes for v2
> > - Add unsigned casting for plat->max_hz
> > ---
> >  drivers/spi/cadence_qspi.c |    8 ++++----
> >  1 files changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c
> > index 23c88d5..8c0f7dd 100644
> > --- a/drivers/spi/cadence_qspi.c
> > +++ b/drivers/spi/cadence_qspi.c
> > @@ -296,10 +296,6 @@ static int cadence_spi_ofdata_to_platdata(struct udevice *bus)
> >  	plat->regbase = (void *)data[0];
> >  	plat->ahbbase = (void *)data[2];
> >  
> > -	/* Use 500KHz as a suitable default */
> > -	plat->max_hz = fdtdec_get_int(blob, node, "spi-max-frequency",
> > -				      500000);
> > -
> >  	/* All other paramters are embedded in the child node */
> >  	subnode = fdt_first_subnode(blob, node);
> >  	if (subnode < 0) {
> > @@ -307,6 +303,10 @@ static int cadence_spi_ofdata_to_platdata(struct udevice *bus)
> >  		return -ENODEV;
> >  	}
> >  
> > +	/* Use 500KHz as a suitable default */
> 
> And as you'll be updating it... it should be "500 kHz", AFAICT.

That sounds easy fix :)

Thanks
Chin Liang

> 
> 
> > +	plat->max_hz = (unsigned int)fdtdec_get_int(blob, subnode,
> > +						"spi-max-frequency", 500000);
> > +
> >  	/* Read other parameters from DT */
> >  	plat->page_size = fdtdec_get_int(blob, subnode, "page-size", 256);
> >  	plat->block_size = fdtdec_get_int(blob, subnode, "block-size",
> > 16);
> 
> 									Pavel
> 

[U-Boot] [PATCH 2/4] spi: cadence_qspi: Fix fdt read of spi-max-frequency

[Marek Vasut]

On Thursday, September 10, 2015 at 06:58:10 AM, Chin Liang See wrote:
> Hi,
> 
> On Tue, 2015-09-08 at 13:19 +0200, marex at denx.de wrote:
> > On Tuesday, September 08, 2015 at 03:18:24 AM, Chin Liang See wrote:
> > > Fix the fdt read for spi-max-frequency as it's contained
> > > in the child node. Current state of code is always
> > > returning default value.
> > > 
> > > Signed-off-by: Chin Liang See <clsee@altera.com>
> > > Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
> > > Cc: Dinh Nguyen <dinh.linux@gmail.com>
> > > Cc: Marek Vasut <marex@denx.de>
> > > Cc: Stefan Roese <sr@denx.de>
> > > Cc: Vikas Manocha <vikas.manocha@st.com>
> > > Cc: Jagannadh Teki <jteki@openedev.com>
> > > Cc: Pavel Machek <pavel@denx.de>
> > > ---
> > > Changes for v2
> > > - Add unsigned casting for plat->max_hz
> > > ---
> > > 
> > >  drivers/spi/cadence_qspi.c |    8 ++++----
> > >  1 files changed, 4 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c
> > > index 23c88d5..8c0f7dd 100644
> > > --- a/drivers/spi/cadence_qspi.c
> > > +++ b/drivers/spi/cadence_qspi.c
> > > @@ -296,10 +296,6 @@ static int cadence_spi_ofdata_to_platdata(struct
> > > udevice *bus) plat->regbase = (void *)data[0];
> > > 
> > >  	plat->ahbbase = (void *)data[2];
> > > 
> > > -	/* Use 500KHz as a suitable default */
> > > -	plat->max_hz = fdtdec_get_int(blob, node, "spi-max-frequency",
> > > -				      500000);
> > > -
> > > 
> > >  	/* All other paramters are embedded in the child node */
> > >  	subnode = fdt_first_subnode(blob, node);
> > >  	if (subnode < 0) {
> > > 
> > > @@ -307,6 +303,10 @@ static int cadence_spi_ofdata_to_platdata(struct
> > > udevice *bus) return -ENODEV;
> > > 
> > >  	}
> > > 
> > > +	/* Use 500KHz as a suitable default */
> > > +	plat->max_hz = (unsigned int)fdtdec_get_int(blob, subnode,
> > > +						"spi-max-frequency", 500000);
> > > +
> > 
> > fdt_getprop_u32() might be the right thing to do here :)
> 
> Cool, yup, I am using fdt_getprop_u32_default_node. But need additional
> patch to enable this and more FDT related function in SPL

All right, be careful not to blow the SPL size too much btw.

Best regards,
Marek Vasut