Re: [PATCH] Configure FSL eSPI CSBEF, CSAFT, and whether to send all received data to user

Re: [PATCH] Configure FSL eSPI CSBEF, CSAFT, and whether to send all received data to user

[Insop Song]

On Mon, Apr 14, 2014 at 09:51:51PM +0100, Mark Brown wrote:
> On Sat, Apr 12, 2014 at 11:48:35AM -0700, Jane Wan wrote:
> > Make FSL eSPI properties configurable through device tree.  The
> > configurable parameters include CSnBEF and CSnAFT in ESPI_SPMODEn
> > registers (n=0,1,2,3), and whether to send all received data to user.
> 
> Don't send cover letters for individual patches, include anything that's
> needed in the patch changelog or after the ---.
> 
> > Test done:
> > - Kernel version 3.8.13
> 
> We're developing v3.15 now, you need to submit against current code.
> 

We will generate a new patch.

> Please also be sure to CC the mailing list on patches, the sourceforge
> list has been unused for quite some time now.

We use scripts/get_maintainer.pl to get the mailing addresses.
We will double check the mailing list from MAINTAINER file.

Regards,

Insop

Re: [PATCH] Configure FSL eSPI CSBEF, CSAFT, and whether to send all received data to user

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 503 bytes --]

On Mon, Apr 14, 2014 at 02:38:53PM -0700, Insop Song wrote:
> On Mon, Apr 14, 2014 at 09:51:51PM +0100, Mark Brown wrote:

> > Please also be sure to CC the mailing list on patches, the sourceforge
> > list has been unused for quite some time now.

> We use scripts/get_maintainer.pl to get the mailing addresses.
> We will double check the mailing list from MAINTAINER file.

This will be part of not using a current kernel - if you're using a v3.8
kernel to check then that's over a year out of date.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] Configure FSL eSPI CSBEF, CSAFT, and whether to send all received data to user

[Insop Song]

On Tue, Apr 15, 2014 at 12:49:49AM +0100, Mark Brown wrote:
> On Mon, Apr 14, 2014 at 02:38:53PM -0700, Insop Song wrote:
> > On Mon, Apr 14, 2014 at 09:51:51PM +0100, Mark Brown wrote:
> 
> > > Please also be sure to CC the mailing list on patches, the sourceforge
> > > list has been unused for quite some time now.
> 
> > We use scripts/get_maintainer.pl to get the mailing addresses.
> > We will double check the mailing list from MAINTAINER file.
> 
> This will be part of not using a current kernel - if you're using a v3.8
> kernel to check then that's over a year out of date.

Mark,

If you were asking that the issue is due to the fact that the kernel we are using is
out of date, then here is our response.

Though, we didn't generate the patch with the latest kernel, we HAVE checked the
latest kernel and the *bugginess* of the freescale espi driver was the same as
the kernel that we are using.

Using the other thread, we will response to your other concerns.

Regards,

Insop
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Configure FSL eSPI CSBEF, CSAFT, and whether to send all received data to user

[Insop Song]

On Tue, Apr 15, 2014 at 12:49:49AM +0100, Mark Brown wrote:
> On Mon, Apr 14, 2014 at 02:38:53PM -0700, Insop Song wrote:
> > On Mon, Apr 14, 2014 at 09:51:51PM +0100, Mark Brown wrote:
> 
> > > Please also be sure to CC the mailing list on patches, the sourceforge
> > > list has been unused for quite some time now.
> 
> > We use scripts/get_maintainer.pl to get the mailing addresses.
> > We will double check the mailing list from MAINTAINER file.
> 
> This will be part of not using a current kernel - if you're using a v3.8
> kernel to check then that's over a year out of date.

I think I sent too soon, you meant for the get_maintainer.pl script.

Regards,

Re: [PATCH] Configure FSL eSPI CSBEF, CSAFT, and whether to send all received data to user

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 607 bytes --]

On Mon, Apr 14, 2014 at 05:00:59PM -0700, Insop Song wrote:
> On Tue, Apr 15, 2014 at 12:49:49AM +0100, Mark Brown wrote:

> > > We use scripts/get_maintainer.pl to get the mailing addresses.
> > > We will double check the mailing list from MAINTAINER file.

> > This will be part of not using a current kernel - if you're using a v3.8
> > kernel to check then that's over a year out of date.

> I think I sent too soon, you meant for the get_maintainer.pl script.

Yup, and for the MAINTAINERS file which it looks things up in - your
copies will be out of date if you're running them from the v3.8 kernel.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

RE: [PATCH] Configure FSL eSPI CSBEF, CSAFT, and whether to send all received data to user

[Jane Wan]

On Mon, Apr 14, 2014 at 09:51:56PM +0100, Mark Brown wrote:
> On Sat, Apr 12, 2014 at 11:48:36AM -0700, Jane Wan wrote:
> > Make FSL eSPI CSnBEF and CSnAFT in ESPI_SPMODEn registers (n=0,1,2,3)
> > configurable through device tree.  FSL eSPI driver hardcodes them to 0.
> > Some device requires different value.
> 
> What do these do?

CSnBEF is the chip select setup time.  It's the delay in bits from the activation 
of chip select pin to the first clock for data frame.

CSnAFT is the chip select hold time.  It's the delay in bits from the last clock 
for data frame to the deactivation of chip select pin.

> > Allow FSL eSPI driver configurable whether to send all received data
> > form slave devices to user.  When user wants to send n_tx bytes and
> > receives n_rx bytes, FSL eSPI driver sends (n_tx + n_rx) bytes on MOSI.
> > For the received (n_tx + n_rx) bytes from MISO, current FSL eSPI
> > driver drops the first n_tx bytes, only passes the last n_rx bytes to user.
> > Some device driver has problem with this.  It requires to know all
> > bytes that the slave device puts on MISO.
> 
> This sounds like a separate patch to the first one, the described behaviour is
> definitely buggy and any correctly implemented Linux driver that does
> bidirectional I/O will have trouble with it.  It should be split out from the new DT
> bindings which are a new feature.
> 
> > ---
> >  drivers/spi/spi-fsl-espi.c       |   68 ++++++++++++++++++++++++++++++++++---
> -
> >  1 files changed, 61 insertions(+), 7 deletions(-)
> 
> All DT binding changes need to be documented in the binding document.

The binding document is updated.  The new changes (for CSnBEF and CSnAFT only)  will 
look like the following.  If this is ok, I'll send it as a separate patch for further discussion.

---
 Documentation/devicetree/bindings/spi/fsl-spi.txt |    6 ++++
 drivers/spi/spi-fsl-espi.c                        |   34 ++++++++++++++++++---
 2 files changed, 36 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/spi/fsl-spi.txt b/Documentation/devicetree/bindings/spi/fsl-spi.txt
index b032dd7..2c7fd41 100644
--- a/Documentation/devicetree/bindings/spi/fsl-spi.txt
+++ b/Documentation/devicetree/bindings/spi/fsl-spi.txt
@@ -42,6 +42,10 @@ Required properties:
 - interrupts : should contain eSPI interrupt, the device has one interrupt.
 - fsl,espi-num-chipselects : the number of the chipselect signals.
 
+Optional properties:
+- fsl,csbef: chip select assertion time in bits before frame starts
+- fsl,csaft: chip select negation time in bits after frame ends
+
 Example:
 	spi@110000 {
 		#address-cells = <1>;
@@ -51,4 +55,6 @@ Example:
 		interrupts = <53 0x2>;
 		interrupt-parent = <&mpic>;
 		fsl,espi-num-chipselects = <4>;
+		fsl,csbef = <1>;
+		fsl,csaft = <1>;
 	};
diff --git a/drivers/spi/spi-fsl-espi.c b/drivers/spi/spi-fsl-espi.c index 428dc7a..7ff9463 100644
--- a/drivers/spi/spi-fsl-espi.c
+++ b/drivers/spi/spi-fsl-espi.c
@@ -590,8 +590,10 @@ static struct spi_master * fsl_espi_probe(struct device *dev,
 	struct spi_master *master;
 	struct mpc8xxx_spi *mpc8xxx_spi;
 	struct fsl_espi_reg *reg_base;
-	u32 regval;
-	int i, ret = 0;
+	struct device_node *nc;
+	const __be32 *prop;
+	u32 regval, csmode;
+	int i, len, ret = 0;
 
 	master = spi_alloc_master(dev, sizeof(struct mpc8xxx_spi));
 	if (!master) {
@@ -638,8 +640,32 @@ static struct spi_master * fsl_espi_probe(struct device *dev,
 	mpc8xxx_spi_write_reg(&reg_base->event, 0xffffffff);
 
 	/* Init eSPI CS mode register */
-	for (i = 0; i < pdata->max_chipselect; i++)
-		mpc8xxx_spi_write_reg(&reg_base->csmode[i], CSMODE_INIT_VAL);
+	for_each_available_child_of_node(master->dev.of_node, nc) {
+		/* get chip select */
+		prop = of_get_property(nc, "reg", &len);
+		if (!prop || len < sizeof(*prop))
+			continue;
+		i = be32_to_cpup(prop);
+		if (i < 0 || i >= pdata->max_chipselect)
+			continue;
+
+		csmode = CSMODE_INIT_VAL;
+		/* check if CSBEF is set in device tree */
+		prop = of_get_property(nc, "fsl,csbef", &len);
+		if (prop && len >= sizeof(*prop)) {
+			csmode &= ~(CSMODE_BEF(0xf));
+			csmode |= CSMODE_BEF(be32_to_cpup(prop));
+		}
+		/* check if CSAFT is set in device tree */
+		prop = of_get_property(nc, "fsl,csaft", &len);
+		if (prop && len >= sizeof(*prop)) {
+			csmode &= ~(CSMODE_AFT(0xf));
+			csmode |= CSMODE_AFT(be32_to_cpup(prop));
+		}
+		mpc8xxx_spi_write_reg(&reg_base->csmode[i], csmode);
+
+		dev_info(dev, "cs=%d, init_csmode=0x%x\n", i, csmode);
+	}
 
 	/* Enable SPI interface */
 	regval = pdata->initial_spmode | SPMODE_INIT_VAL | SPMODE_ENABLE;
--
1.7.9.5

> > +/* whether to send all rx data to user per chip select */ static u8
> > +*spi_raw_rxdata_to_user;
> > +
> 
> No, any data needs to be part of the driver data structure not a global variable.

I will look into this.

> > +		if (spi_raw_rxdata_to_user[m->spi->chip_select])
> > +			espi_trans->len = n_tx;
> > +		else
> > +			espi_trans->len = trans_len + n_tx;
> 
> Why is there even an option for the buggy behaviour?

We have three devices attached to the FSL eSPI interface, with chip select (CS) 
0-2. The device driver for the device at CS #2 requires to know all the data that 
the slave device put on MISO.  But the device drivers for the other two devices 
(at CS #0 and #1) work with the existing FSL eSPI driver.  The device at CS #0 is 
Micron n25q512a compatible.

We make the FSL eSPI driver configurable through device tree.  If we make a fix 
without the DT option, the fix will break other device drivers working with the 
existing FSL eSPI driver.

Could this still be considered as a solution?  If this is ok, I can send it as a separate 
patch.  Otherwise, we will look if this driver can be modified without DT option.

Regards,
Jane
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Configure FSL eSPI CSBEF, CSAFT, and whether to send all received data to user

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1318 bytes --]

On Wed, Apr 16, 2014 at 04:39:47PM +0000, Jane Wan wrote:
> On Mon, Apr 14, 2014 at 09:51:56PM +0100, Mark Brown wrote:

> > > +		if (spi_raw_rxdata_to_user[m->spi->chip_select])
> > > +			espi_trans->len = n_tx;
> > > +		else
> > > +			espi_trans->len = trans_len + n_tx;

> > Why is there even an option for the buggy behaviour?

> We have three devices attached to the FSL eSPI interface, with chip select (CS) 
> 0-2. The device driver for the device at CS #2 requires to know all the data that 
> the slave device put on MISO.  But the device drivers for the other two devices 
> (at CS #0 and #1) work with the existing FSL eSPI driver.  The device at CS #0 is 
> Micron n25q512a compatible.

> We make the FSL eSPI driver configurable through device tree.  If we make a fix 
> without the DT option, the fix will break other device drivers working with the 
> existing FSL eSPI driver.

> Could this still be considered as a solution?  If this is ok, I can send it as a separate 
> patch.  Otherwise, we will look if this driver can be modified without DT option.

No, this is completely unaceptable.  The drivers relying on the buggy
behaviour are broken and must be fixed.  The whole point of DT is to
describe the hardware, not to allow the OS to implement workarounds for
itself.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]