[PATCH] spi: spi-imx: mx51 support for more than 4 gpio chip selects

[PATCH] spi: spi-imx: mx51 support for more than 4 gpio chip selects

[Chris Lesiak]

[-- Attachment #1.1: Type: text/plain, Size: 3849 bytes --]

The MX51_ECSPI_CTRL and MX51_ECSPI_CONFIG registers have bit fields
that only support the four slave select channels.  If we are using
a gpio to support chip_select > 3, we need to be careful not to
write outside the bit fields.  Probably the biggest issue is the
2-bit CHANNEL_SELECT field of MX51_ECSPI_CTRL overflowing into the
BURST_LENGTH field.  That will likely cause a DMA TX timeout.

To prevent this, when chip_select > 3, use slave select = 3.

This should allow up to four channels using the built-in slave
selects, or any of the first three chip selects optionally using
built-in slave selects with gpio used for additional channels.

Signed-off-by: Chris Lesiak <chris.lesiak@licor.com>
---
 drivers/spi/spi-imx.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index 30d82cc7300b..7ed493200951 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -521,6 +521,7 @@ static int mx51_ecspi_prepare_message(struct spi_imx_data *spi_imx,
        u32 testreg, delay;
        u32 cfg = readl(spi_imx->base + MX51_ECSPI_CONFIG);
        u32 current_cfg = cfg;
+       u8 ss = min_t(u8, spi->chip_select, 3);

        /* set Master or Slave mode */
        if (spi_imx->slave_mode)
@@ -535,7 +536,7 @@ static int mx51_ecspi_prepare_message(struct spi_imx_data *spi_imx,
                ctrl |= MX51_ECSPI_CTRL_DRCTL(spi_imx->spi_drctl);

        /* set chip select to use */
-       ctrl |= MX51_ECSPI_CTRL_CS(spi->chip_select);
+       ctrl |= MX51_ECSPI_CTRL_CS(ss);

        /*
         * The ctrl register must be written first, with the EN bit set other
@@ -556,22 +557,22 @@ static int mx51_ecspi_prepare_message(struct spi_imx_data *spi_imx,
         * BURST_LENGTH + 1 bits are received
         */
        if (spi_imx->slave_mode && is_imx53_ecspi(spi_imx))
-               cfg &= ~MX51_ECSPI_CONFIG_SBBCTRL(spi->chip_select);
+               cfg &= ~MX51_ECSPI_CONFIG_SBBCTRL(ss);
        else
-               cfg |= MX51_ECSPI_CONFIG_SBBCTRL(spi->chip_select);
+               cfg |= MX51_ECSPI_CONFIG_SBBCTRL(ss);

        if (spi->mode & SPI_CPOL) {
-               cfg |= MX51_ECSPI_CONFIG_SCLKPOL(spi->chip_select);
-               cfg |= MX51_ECSPI_CONFIG_SCLKCTL(spi->chip_select);
+               cfg |= MX51_ECSPI_CONFIG_SCLKPOL(ss);
+               cfg |= MX51_ECSPI_CONFIG_SCLKCTL(ss);
        } else {
-               cfg &= ~MX51_ECSPI_CONFIG_SCLKPOL(spi->chip_select);
-               cfg &= ~MX51_ECSPI_CONFIG_SCLKCTL(spi->chip_select);
+               cfg &= ~MX51_ECSPI_CONFIG_SCLKPOL(ss);
+               cfg &= ~MX51_ECSPI_CONFIG_SCLKCTL(ss);
        }

        if (spi->mode & SPI_CS_HIGH)
-               cfg |= MX51_ECSPI_CONFIG_SSBPOL(spi->chip_select);
+               cfg |= MX51_ECSPI_CONFIG_SSBPOL(ss);
        else
-               cfg &= ~MX51_ECSPI_CONFIG_SSBPOL(spi->chip_select);
+               cfg &= ~MX51_ECSPI_CONFIG_SSBPOL(ss);

        if (cfg == current_cfg)
                return 0;
@@ -616,14 +617,15 @@ static void mx51_configure_cpha(struct spi_imx_data *spi_imx,
        bool cpha = (spi->mode & SPI_CPHA);
        bool flip_cpha = (spi->mode & SPI_RX_CPHA_FLIP) && spi_imx->rx_only;
        u32 cfg = readl(spi_imx->base + MX51_ECSPI_CONFIG);
+       u8 ss = min_t(u8, spi->chip_select, 3);

        /* Flip cpha logical value iff flip_cpha */
        cpha ^= flip_cpha;

        if (cpha)
-               cfg |= MX51_ECSPI_CONFIG_SCLKPHA(spi->chip_select);
+               cfg |= MX51_ECSPI_CONFIG_SCLKPHA(ss);
        else
-               cfg &= ~MX51_ECSPI_CONFIG_SCLKPHA(spi->chip_select);
+               cfg &= ~MX51_ECSPI_CONFIG_SCLKPHA(ss);

        writel(cfg, spi_imx->base + MX51_ECSPI_CONFIG);
 }
--
2.26.2

Chris Lesiak
Principal Design Engineer, Software
402-467-0693

[-- Attachment #1.2.1: Type: text/html, Size: 10080 bytes --]

[-- Attachment #1.2.2: logo.png --]
[-- Type: image/png, Size: 15564 bytes --]

Re: [PATCH] spi: spi-imx: mx51 support for more than 4 gpio chip selects

[Mark Brown]

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

On Fri, Oct 07, 2022 at 10:28:30AM -0500, Chris Lesiak wrote:
> The MX51_ECSPI_CTRL and MX51_ECSPI_CONFIG registers have bit fields
> that only support the four slave select channels.  If we are using
> a gpio to support chip_select > 3, we need to be careful not to

This doesn't apply against current code, please check and resend.
Please also try to avoid outdated terminology and refer to chip selects.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v2] spi: spi-imx: mx51 support for more than 4 gpio chip selects

[Chris Lesiak]

[-- Attachment #1.1: Type: text/plain, Size: 4893 bytes --]

The MX51_ECSPI_CTRL and MX51_ECSPI_CONFIG registers have bit fields
that only support the four hardware chip select channels.  If we are
using a gpio to support chip_select > 3, we need to be careful not to
write outside the bit fields.  Probably the biggest issue is the
2-bit CHANNEL_SELECT field of MX51_ECSPI_CTRL overflowing into the
BURST_LENGTH field.  That will likely cause a DMA TX timeout.

To prevent this, when chip_select > 3, use hardware chip select = 3.

This should allow up to four channels using the hardware chip
selects, or any of the first three chip selects optionally using
hardware chip selects with gpio used for additional channels.

Signed-off-by: Chris Lesiak <chris.lesiak@licor.com>
---
 drivers/spi/spi-imx.c | 43 ++++++++++++++++++++++++++++++++-----------
 1 file changed, 32 insertions(+), 11 deletions(-)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index 30d82cc7300b..9c4e979ab74d 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -253,6 +253,8 @@ static bool spi_imx_can_dma(struct spi_controller *controller, struct spi_device
        return true;
 }

+#define MX51_ECSPI_MAX_HW_CS           3
+
 #define MX51_ECSPI_CTRL                0x08
 #define MX51_ECSPI_CTRL_ENABLE         (1 <<  0)
 #define MX51_ECSPI_CTRL_XCH            (1 <<  2)
@@ -521,6 +523,17 @@ static int mx51_ecspi_prepare_message(struct spi_imx_data *spi_imx,
        u32 testreg, delay;
        u32 cfg = readl(spi_imx->base + MX51_ECSPI_CONFIG);
        u32 current_cfg = cfg;
+       u8 hw_chip_select;
+
+       if (spi->cs_gpiod) {
+               hw_chip_select = MX51_ECSPI_MAX_HW_CS;
+       } else if (spi->chip_select <= MX51_ECSPI_MAX_HW_CS) {
+               hw_chip_select = spi->chip_select;
+       } else {
+               dev_err(spi_imx->dev, "hardware chip select is out of range: %i\n",
+                       spi->chip_select);
+               return -EINVAL;
+       }

        /* set Master or Slave mode */
        if (spi_imx->slave_mode)
@@ -535,7 +548,7 @@ static int mx51_ecspi_prepare_message(struct spi_imx_data *spi_imx,
                ctrl |= MX51_ECSPI_CTRL_DRCTL(spi_imx->spi_drctl);

        /* set chip select to use */
-       ctrl |= MX51_ECSPI_CTRL_CS(spi->chip_select);
+       ctrl |= MX51_ECSPI_CTRL_CS(hw_chip_select);

        /*
         * The ctrl register must be written first, with the EN bit set other
@@ -556,22 +569,22 @@ static int mx51_ecspi_prepare_message(struct spi_imx_data *spi_imx,
         * BURST_LENGTH + 1 bits are received
         */
        if (spi_imx->slave_mode && is_imx53_ecspi(spi_imx))
-               cfg &= ~MX51_ECSPI_CONFIG_SBBCTRL(spi->chip_select);
+               cfg &= ~MX51_ECSPI_CONFIG_SBBCTRL(hw_chip_select);
        else
-               cfg |= MX51_ECSPI_CONFIG_SBBCTRL(spi->chip_select);
+               cfg |= MX51_ECSPI_CONFIG_SBBCTRL(hw_chip_select);

        if (spi->mode & SPI_CPOL) {
-               cfg |= MX51_ECSPI_CONFIG_SCLKPOL(spi->chip_select);
-               cfg |= MX51_ECSPI_CONFIG_SCLKCTL(spi->chip_select);
+               cfg |= MX51_ECSPI_CONFIG_SCLKPOL(hw_chip_select);
+               cfg |= MX51_ECSPI_CONFIG_SCLKCTL(hw_chip_select);
        } else {
-               cfg &= ~MX51_ECSPI_CONFIG_SCLKPOL(spi->chip_select);
-               cfg &= ~MX51_ECSPI_CONFIG_SCLKCTL(spi->chip_select);
+               cfg &= ~MX51_ECSPI_CONFIG_SCLKPOL(hw_chip_select);
+               cfg &= ~MX51_ECSPI_CONFIG_SCLKCTL(hw_chip_select);
        }

        if (spi->mode & SPI_CS_HIGH)
-               cfg |= MX51_ECSPI_CONFIG_SSBPOL(spi->chip_select);
+               cfg |= MX51_ECSPI_CONFIG_SSBPOL(hw_chip_select);
        else
-               cfg &= ~MX51_ECSPI_CONFIG_SSBPOL(spi->chip_select);
+               cfg &= ~MX51_ECSPI_CONFIG_SSBPOL(hw_chip_select);

        if (cfg == current_cfg)
                return 0;
@@ -616,14 +629,22 @@ static void mx51_configure_cpha(struct spi_imx_data *spi_imx,
        bool cpha = (spi->mode & SPI_CPHA);
        bool flip_cpha = (spi->mode & SPI_RX_CPHA_FLIP) && spi_imx->rx_only;
        u32 cfg = readl(spi_imx->base + MX51_ECSPI_CONFIG);
+       u8 hw_chip_select;
+
+       if (spi->cs_gpiod)
+               hw_chip_select = MX51_ECSPI_MAX_HW_CS;
+       else if (spi->chip_select <= MX51_ECSPI_MAX_HW_CS)
+               hw_chip_select = spi->chip_select;
+       else
+               return;

        /* Flip cpha logical value iff flip_cpha */
        cpha ^= flip_cpha;

        if (cpha)
-               cfg |= MX51_ECSPI_CONFIG_SCLKPHA(spi->chip_select);
+               cfg |= MX51_ECSPI_CONFIG_SCLKPHA(hw_chip_select);
        else
-               cfg &= ~MX51_ECSPI_CONFIG_SCLKPHA(spi->chip_select);
+               cfg &= ~MX51_ECSPI_CONFIG_SCLKPHA(hw_chip_select);

        writel(cfg, spi_imx->base + MX51_ECSPI_CONFIG);
 }
--
2.26.2

Chris Lesiak
Principal Design Engineer, Software


[-- Attachment #1.2.1: Type: text/html, Size: 10506 bytes --]

[-- Attachment #1.2.2: logo.png --]
[-- Type: image/png, Size: 15564 bytes --]

Re: [PATCH v2] spi: spi-imx: mx51 support for more than 4 gpio chip selects

[Mark Brown]

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

On Mon, Oct 17, 2022 at 08:54:03AM -0500, Chris Lesiak wrote:
> The MX51_ECSPI_CTRL and MX51_ECSPI_CONFIG registers have bit fields
> that only support the four hardware chip select channels.  If we are
> using a gpio to support chip_select > 3, we need to be careful not to
> write outside the bit fields.  Probably the biggest issue is the
> 2-bit CHANNEL_SELECT field of MX51_ECSPI_CTRL overflowing into the
> BURST_LENGTH field.  That will likely cause a DMA TX timeout.

This still doesn't apply against current code, please check and resend:

Applying: spi: spi-imx: mx51 support for more than 4 gpio chip selects
Using index info to reconstruct a base tree...
error: patch failed: drivers/spi/spi-imx.c:253
error: drivers/spi/spi-imx.c: patch does not apply
error: Did you hand edit your patch?
It does not apply to blobs recorded in its index.
Patch failed at 0001 spi: spi-imx: mx51 support for more than 4 gpio chip selects

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]