diff mbox series

[v2,12/14] spi: bcm63xx-hsspi: Disable spi mem dual io

Message ID 20230124221218.341511-13-william.zhang@broadcom.com
State New
Headers
Series spi: bcm63xx-hsspi: driver and doc updates |

Commit Message

William Zhang Jan. 24, 2023, 10:12 p.m. UTC 
  In general the controller supports SPI dual mode operation but the
particular SPI flash dual io read op switches from single mode in cmd
phase to dual mode in address and data phase. This is not compatible
with prepend operation where cmd and address are sent out through the
prepend buffer and they must use same the number of io pins.

This patch disables these SPI flash dual io read ops through the mem_ops
supports_op interface. This makes sure the SPI flash driver selects the
compatible read ops at run time.

Signed-off-by: William Zhang <william.zhang@broadcom.com>

---

Changes in v2:
- Remove the code that uses the deprecated flag use_cs_workaround
- Always disable dual io read ops as prepend is the default mode

 drivers/spi/spi-bcm63xx-hsspi.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

Comments

Jonas Gorski Jan. 26, 2023, 3:15 p.m. UTC | #1 
On Tue, 24 Jan 2023 at 23:33, William Zhang <william.zhang@broadcom.com> wrote:
>
> In general the controller supports SPI dual mode operation but the
> particular SPI flash dual io read op switches from single mode in cmd
> phase to dual mode in address and data phase. This is not compatible
> with prepend operation where cmd and address are sent out through the
> prepend buffer and they must use same the number of io pins.
>
> This patch disables these SPI flash dual io read ops through the mem_ops
> supports_op interface. This makes sure the SPI flash driver selects the
> compatible read ops at run time.
>
> Signed-off-by: William Zhang <william.zhang@broadcom.com>
>
> ---
>
> Changes in v2:
> - Remove the code that uses the deprecated flag use_cs_workaround
> - Always disable dual io read ops as prepend is the default mode
>
>  drivers/spi/spi-bcm63xx-hsspi.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/drivers/spi/spi-bcm63xx-hsspi.c b/drivers/spi/spi-bcm63xx-hsspi.c
> index 2a0bef943967..dd320fda7611 100644
> --- a/drivers/spi/spi-bcm63xx-hsspi.c
> +++ b/drivers/spi/spi-bcm63xx-hsspi.c
> @@ -20,6 +20,7 @@
>  #include <linux/spi/spi.h>
>  #include <linux/mutex.h>
>  #include <linux/of.h>
> +#include <linux/spi/spi-mem.h>
>  #include <linux/reset.h>
>  #include <linux/pm_runtime.h>
>
> @@ -663,6 +664,23 @@ static int bcm63xx_hsspi_transfer_one(struct spi_master *master,
>         return 0;
>  }
>
> +static bool bcm63xx_hsspi_mem_supports_op(struct spi_mem *mem,
> +                           const struct spi_mem_op *op)
> +{
> +       if (!spi_mem_default_supports_op(mem, op))
> +               return false;
> +
> +       /* Controller doesn't support spi mem dual/quad read cmd in prepend mode */
> +       if ((op->cmd.opcode == 0xbb) || (op->cmd.opcode == 0xeb))

There are defines in linux/mtd/spi-nor.h you can use:

if ((op->cmd.opcode == SPINOR_OP_READ_1_2_2) || (op->cmd.opcode ==
SPINOR_OP_READ_1_4_4))

Though SPINOR_OP_READ_1_4_4 seems to be redundant, since the
controller does not support quad mode, and does not advertise it, so
it should never even be an option.

Looking at that file, instead what is missing is
SPINOR_OP_READ_1_2_2_4B (0xbc) which shouldn't be usable either.


> +               return false;
> +
> +       return true;
> +}
> +
> +static const struct spi_controller_mem_ops bcm63xx_hsspi_mem_ops = {
> +       .supports_op = bcm63xx_hsspi_mem_supports_op,
> +};
> +
>  static irqreturn_t bcm63xx_hsspi_interrupt(int irq, void *dev_id)
>  {
>         struct bcm63xx_hsspi *bs = (struct bcm63xx_hsspi *)dev_id;
> @@ -760,6 +778,7 @@ static int bcm63xx_hsspi_probe(struct platform_device *pdev)
>         mutex_init(&bs->msg_mutex);
>         init_completion(&bs->done);
>
> +       master->mem_ops = &bcm63xx_hsspi_mem_ops;
>         master->dev.of_node = dev->of_node;
>         if (!dev->of_node)
>                 master->bus_num = HSSPI_BUS_NUM;
> --
> 2.37.3
>
William Zhang Jan. 27, 2023, 2:04 a.m. UTC | #2 
On 01/26/2023 07:15 AM, Jonas Gorski wrote:
>>
>> +static bool bcm63xx_hsspi_mem_supports_op(struct spi_mem *mem,
>> +                           const struct spi_mem_op *op)
>> +{
>> +       if (!spi_mem_default_supports_op(mem, op))
>> +               return false;
>> +
>> +       /* Controller doesn't support spi mem dual/quad read cmd in prepend mode */
>> +       if ((op->cmd.opcode == 0xbb) || (op->cmd.opcode == 0xeb))
> 
> There are defines in linux/mtd/spi-nor.h you can use:
> 
> if ((op->cmd.opcode == SPINOR_OP_READ_1_2_2) || (op->cmd.opcode ==
> SPINOR_OP_READ_1_4_4))
> 
> Though SPINOR_OP_READ_1_4_4 seems to be redundant, since the
> controller does not support quad mode, and does not advertise it, so
> it should never even be an option.
> 
> Looking at that file, instead what is missing is
> SPINOR_OP_READ_1_2_2_4B (0xbc) which shouldn't be usable either.
> 

You are right.  I was only looking at the spi nand header which does not 
have this clear definition.  Will use them and drop the quad IO opcode 
for the reason you mentioned.
diff mbox series

Patch

diff --git a/drivers/spi/spi-bcm63xx-hsspi.c b/drivers/spi/spi-bcm63xx-hsspi.c
index 2a0bef943967..dd320fda7611 100644
--- a/drivers/spi/spi-bcm63xx-hsspi.c
+++ b/drivers/spi/spi-bcm63xx-hsspi.c
@@ -20,6 +20,7 @@ 
 #include <linux/spi/spi.h>
 #include <linux/mutex.h>
 #include <linux/of.h>
+#include <linux/spi/spi-mem.h>
 #include <linux/reset.h>
 #include <linux/pm_runtime.h>
 
@@ -663,6 +664,23 @@  static int bcm63xx_hsspi_transfer_one(struct spi_master *master,
 	return 0;
 }
 
+static bool bcm63xx_hsspi_mem_supports_op(struct spi_mem *mem,
+			    const struct spi_mem_op *op)
+{
+	if (!spi_mem_default_supports_op(mem, op))
+		return false;
+
+	/* Controller doesn't support spi mem dual/quad read cmd in prepend mode */
+	if ((op->cmd.opcode == 0xbb) || (op->cmd.opcode == 0xeb))
+		return false;
+
+	return true;
+}
+
+static const struct spi_controller_mem_ops bcm63xx_hsspi_mem_ops = {
+	.supports_op = bcm63xx_hsspi_mem_supports_op,
+};
+
 static irqreturn_t bcm63xx_hsspi_interrupt(int irq, void *dev_id)
 {
 	struct bcm63xx_hsspi *bs = (struct bcm63xx_hsspi *)dev_id;
@@ -760,6 +778,7 @@  static int bcm63xx_hsspi_probe(struct platform_device *pdev)
 	mutex_init(&bs->msg_mutex);
 	init_completion(&bs->done);
 
+	master->mem_ops = &bcm63xx_hsspi_mem_ops;
 	master->dev.of_node = dev->of_node;
 	if (!dev->of_node)
 		master->bus_num = HSSPI_BUS_NUM;