Configure FSL eSPI CSBEF, CSAFT, and whether to send all received data to user

Commit Message

Jane Wan April 16, 2014, 4:39 p.m. UTC

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

--
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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Mark Brown April 16, 2014, 5:28 p.m. UTC | #1

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.

Patch

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;