This RFC is the continuation to the discussion which happened on
'commit f89504300e94 ("spi: Stacked/parallel memories bindings")' for
adding dtbinding support for stacked/parallel memories.
The purpose of this patch series is to demonstrate the changes in spi-nor,
spi core and ZynqMP GQSPI driver w.r.t to stacked/parallel memories
support.Please go through the series and share you comments.
To support stacked/parallel configuration following changes are done to spi
core and spi-nor.
- The chip select member (chip_select) of the spi_device structure is changed
to an array (chip_select[2]). This array is used to store the CS values coming
form the "reg" DT property.
- Added a new member (cs_index_mask) in the spi_device structure to hold the
index information of above chip_select array. SPI-NOR is not aware of the
chip_select values, For any incoming request SPI-NOR will decide the flash
index with the help of individual flash size and the configuration type
(single/stacked/parallel). SPI-NOR will pass on the flash index information
to the SPI core by setting the appropriate bit(s) of "cs_index_mask".
For example if nth bit of "cs_index_mask" is set then the driver would
assert chip_slect[n].
- The flash parameter member(*params) of the spi_nor structure is changed
to an array (*params[2]). The array is used to store the parameters of each
flash connected in stacked/parallel configuration.
This patch series targets flashes of same make connected in stacked
configuration and for parallel configuration both the flashes should be
identical.
---
BRANCH: mtd/next
---
Amit Kumar Mahapatra (2):
spi: Add multiple CS support for a single SPI device
mtd: spi-nor: Add support for stacked/parallel memories
drivers/mtd/spi-nor/core.c | 104 +++++++++++++++++++++++++++++----
drivers/mtd/spi-nor/core.h | 5 ++
drivers/spi/spi-zynqmp-gqspi.c | 30 ++++++++--
drivers/spi/spi.c | 10 +++-
include/linux/mtd/spi-nor.h | 8 ++-
include/linux/spi/spi.h | 10 +++-
6 files changed, 146 insertions(+), 21 deletions(-)
--
2.17.1
For supporting multiple CS the SPI device need to be aware of all the CS
values. So the "chip_select" member in the spi_device structure is now an
array that holds all the CS values.
spi_device structure now has a "cs_index_mask" member. This acts as an
index to the chip_select array. If nth bit of spi->cs_index_mask is set then
the driver would assert spi->chip_slect[n].
When flashes are connected in stacked mode SPI-NOR will enable the required
chip select by setting the appropriate bit in spi->cs_index_mask.
In parallel connection both the flashes need to be asserted simultaneously,
this can be achieved by enabling 0th(CS0) & 1st(CS1) bit in spi->cs_index_mask.
Signed-off-by: Amit Kumar Mahapatra <[email protected]>
---
drivers/spi/spi-zynqmp-gqspi.c | 30 ++++++++++++++++++++++++++----
drivers/spi/spi.c | 10 +++++++---
include/linux/spi/spi.h | 10 +++++++++-
3 files changed, 42 insertions(+), 8 deletions(-)
diff --git a/drivers/spi/spi-zynqmp-gqspi.c b/drivers/spi/spi-zynqmp-gqspi.c
index c760aac070e5..2535a8bca4da 100644
--- a/drivers/spi/spi-zynqmp-gqspi.c
+++ b/drivers/spi/spi-zynqmp-gqspi.c
@@ -136,6 +136,11 @@
#define GQSPI_MAX_NUM_CS 2 /* Maximum number of chip selects */
+#define GQSPI_SELECT_LOWER_CS BIT(0)
+#define GQSPI_SELECT_UPPER_CS BIT(1)
+#define GQSPI_SELECT_BOTH_CS (GQSPI_SELECT_LOWER_CS | \
+ GQSPI_SELECT_UPPER_CS)
+
#define SPI_AUTOSUSPEND_TIMEOUT 3000
enum mode_type {GQSPI_MODE_IO, GQSPI_MODE_DMA};
@@ -361,16 +366,33 @@ static void zynqmp_qspi_chipselect(struct spi_device *qspi, bool is_high)
struct zynqmp_qspi *xqspi = spi_master_get_devdata(qspi->master);
ulong timeout;
u32 genfifoentry = 0, statusreg;
+ u8 cs_index_mask = qspi->cs_index_mask;
genfifoentry |= GQSPI_GENFIFO_MODE_SPI;
if (!is_high) {
- if (!qspi->chip_select) {
- xqspi->genfifobus = GQSPI_GENFIFO_BUS_LOWER;
- xqspi->genfifocs = GQSPI_GENFIFO_CS_LOWER;
- } else {
+ /*
+ * GQSPI controller only supports two chip selects,
+ * CS0 and CS1
+ */
+ if (cs_index_mask & GQSPI_SELECT_BOTH_CS) {
+
+ xqspi->genfifobus = GQSPI_GENFIFO_BUS_LOWER |
+ GQSPI_GENFIFO_BUS_UPPER;
+ xqspi->genfifocs = GQSPI_GENFIFO_CS_LOWER |
+ GQSPI_GENFIFO_CS_UPPER;
+
+ } else if ((cs_index_mask & GQSPI_SELECT_UPPER_CS) &&
+ (qspi->chip_select[GQSPI_SELECT_UPPER_CS - 1])) {
+
xqspi->genfifobus = GQSPI_GENFIFO_BUS_UPPER;
xqspi->genfifocs = GQSPI_GENFIFO_CS_UPPER;
+
+ } else if ((cs_index_mask & GQSPI_SELECT_LOWER_CS) &&
+ (!qspi->chip_select[GQSPI_SELECT_LOWER_CS - 1])) {
+
+ xqspi->genfifobus = GQSPI_GENFIFO_BUS_LOWER;
+ xqspi->genfifocs = GQSPI_GENFIFO_CS_LOWER;
}
genfifoentry |= xqspi->genfifobus;
genfifoentry |= xqspi->genfifocs;
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 2e6d6bbeb784..d3077874e6e8 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -2082,6 +2082,8 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi,
{
u32 value;
int rc;
+ u32 cs[SPI_CS_CNT_MAX];
+ u8 idx;
/* Mode (clock phase/polarity/etc.) */
if (of_property_read_bool(nc, "spi-cpha"))
@@ -2154,13 +2156,15 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi,
}
/* Device address */
- rc = of_property_read_u32(nc, "reg", &value);
- if (rc) {
+ rc = of_property_read_variable_u32_array(nc, "reg", &cs[0],
+ 1, SPI_CS_CNT_MAX);
+ if (rc < 0) {
dev_err(&ctlr->dev, "%pOF has no valid 'reg' property (%d)\n",
nc, rc);
return rc;
}
- spi->chip_select = value;
+ for(idx = 0; idx < rc; idx++)
+ spi->chip_select[idx] = cs[idx];
/* Device speed */
if (!of_property_read_u32(nc, "spi-max-frequency", &value))
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 5f8c063ddff4..e930d987f3c2 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -18,6 +18,9 @@
#include <uapi/linux/spi/spi.h>
#include <linux/acpi.h>
+/* Max no. of CS supported per spi device */
+#define SPI_CS_CNT_MAX 2
+
struct dma_chan;
struct software_node;
struct ptp_system_timestamp;
@@ -148,6 +151,7 @@ extern int spi_delay_exec(struct spi_delay *_delay, struct spi_transfer *xfer);
* deasserted. If @cs_change_delay is used from @spi_transfer, then the
* two delays will be added up.
* @statistics: statistics for the spi_device
+ * @cs_index_mask: Bit mask of the active chipselect(s) in the chipselect array
*
* A @spi_device is used to interchange data between an SPI slave
* (usually a discrete chip) and CPU memory.
@@ -163,7 +167,7 @@ struct spi_device {
struct spi_controller *controller;
struct spi_controller *master; /* compatibility layer */
u32 max_speed_hz;
- u8 chip_select;
+ u8 chip_select[SPI_CS_CNT_MAX];
u8 bits_per_word;
bool rt;
#define SPI_NO_TX BIT(31) /* no transmit wire */
@@ -194,6 +198,10 @@ struct spi_device {
/* the statistics */
struct spi_statistics statistics;
+ /* Bit mask of the chipselect(s) that the driver
+ * need to use form the chipselect array.
+ */
+ u8 cs_index_mask : 2;
/*
* likely need more hooks for more protocol options affecting how
* the controller talks to each chip, like:
--
2.17.1
While initializing the flash parameter structure, size of each flash is
updated with the values coming from "stacked-memories" and
"parallel-memories" DT properties.
Two new nor->flags (SNOR_F_HAS_STACKED and SNOR_F_HAS_PARALLEL) are added
to distinguish between the stacked and parallel configuration.
In parallel configuration all the operations need to be performed on both
the flashes simultaneously, So during each operation SPI-NOR needs to set
0th bit(CS0) & 1st bit(CS1) in nor->spimem->spi->cs_index_mask. The GQSPI
driver will then assert CS0 & CS1.
In stacked configuration the SPI-NOR, with individual flash size information,
will determining the flash on which the operation need to be performed and
it will set the appropriate CS bit in nor->spimem->spi->cs_index_mask.
Signed-off-by: Amit Kumar Mahapatra <[email protected]>
---
drivers/mtd/spi-nor/core.c | 104 +++++++++++++++++++++++++++++++-----
drivers/mtd/spi-nor/core.h | 5 ++
include/linux/mtd/spi-nor.h | 8 ++-
3 files changed, 104 insertions(+), 13 deletions(-)
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 502967c76c5f..5d9bbb28659a 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -2463,6 +2463,9 @@ static void spi_nor_init_fixup_flags(struct spi_nor *nor)
*/
static void spi_nor_late_init_params(struct spi_nor *nor)
{
+ struct device_node *np = spi_nor_get_flash_node(nor);
+ u64 flash_size[SNOR_FLASH_CNT_MAX];
+
if (nor->manufacturer && nor->manufacturer->fixups &&
nor->manufacturer->fixups->late_init)
nor->manufacturer->fixups->late_init(nor);
@@ -2479,6 +2482,27 @@ static void spi_nor_late_init_params(struct spi_nor *nor)
*/
if (nor->flags & SNOR_F_HAS_LOCK && !nor->params->locking_ops)
spi_nor_init_default_locking_ops(nor);
+
+ /*
+ * The flashes that are connected in stacked mode should be of same make.
+ * Except the flash size all other properties are identical for all the
+ * flashes connected in stacked mode.
+ * The flashes that are connected in parallel mode should be identical.
+ */
+ if (!of_property_read_u64_array(np, "stacked-memories", &flash_size[0], SNOR_FLASH_CNT_MAX)) {
+ nor->flags |= SNOR_F_HAS_STACKED;
+ } else if (!of_property_read_u64_array(np, "parallel-memories", &flash_size[0], SNOR_FLASH_CNT_MAX)){
+ nor->flags |= SNOR_F_HAS_PARALLEL;
+ }
+
+ if (nor->flags & (SNOR_F_HAS_STACKED | SNOR_F_HAS_PARALLEL)) {
+ nor->params[1] = devm_kzalloc(nor->dev, sizeof(*nor->params[0]), GFP_KERNEL);
+ if (!nor->params[1])
+ return -ENOMEM;
+ memcpy(nor->params[1], nor->params[0], sizeof(*nor->params[0]));
+ nor->params[1]->size = flash_size[1];
+ }
+ return 0;
}
/**
@@ -2614,8 +2638,8 @@ static int spi_nor_init_params(struct spi_nor *nor)
{
int ret;
- nor->params = devm_kzalloc(nor->dev, sizeof(*nor->params), GFP_KERNEL);
- if (!nor->params)
+ nor->params[0] = devm_kzalloc(nor->dev, sizeof(*nor->params[0]), GFP_KERNEL);
+ if (!nor->params[0])
return -ENOMEM;
spi_nor_init_default_params(nor);
@@ -2677,19 +2701,51 @@ static int spi_nor_octal_dtr_enable(struct spi_nor *nor, bool enable)
*/
static int spi_nor_quad_enable(struct spi_nor *nor)
{
- if (!nor->params->quad_enable)
- return 0;
+ u8 idx;
+ int err;
- if (!(spi_nor_get_protocol_width(nor->read_proto) == 4 ||
- spi_nor_get_protocol_width(nor->write_proto) == 4))
- return 0;
+ if (nor->flags & SNOR_F_HAS_PARALLEL) {
+ if (!nor->params[0]->quad_enable)
+ return 0;
- return nor->params->quad_enable(nor);
+ if (!(spi_nor_get_protocol_width(nor->read_proto) == 4 ||
+ spi_nor_get_protocol_width(nor->write_proto) == 4))
+ return 0;
+ /*
+ * In parallel mode both chip selects i.e., CS0 &
+ * CS1 need to be asserted simulatneously.
+ */
+ nor->spimem->spi->cs_index_mask = SPI_NOR_ENABLE_BOTH_CS;
+ err = nor->params[0]->quad_enable(nor);
+ } else {
+ for (idx = 0; idx < SNOR_FLASH_CNT_MAX; idx++) {
+ if (nor->params[idx] != NULL) {
+
+ if (!nor->params[idx]->quad_enable)
+ return 0;
+
+ if (!(spi_nor_get_protocol_width(nor->read_proto) == 4 ||
+ spi_nor_get_protocol_width(nor->write_proto) == 4))
+ return 0;
+
+ /*
+ * Set the appropriate CS index before
+ * issuing the command.
+ */
+ nor->spimem->spi->cs_index_mask = 0x01 << idx;
+ err = nor->params[idx]->quad_enable(nor);
+ if (err)
+ return err;
+ }
+ }
+ }
+ return err;
}
static int spi_nor_init(struct spi_nor *nor)
{
int err;
+ int idx;
err = spi_nor_octal_dtr_enable(nor, true);
if (err) {
@@ -2730,7 +2786,25 @@ static int spi_nor_init(struct spi_nor *nor)
*/
WARN_ONCE(nor->flags & SNOR_F_BROKEN_RESET,
"enabling reset hack; may not recover from unexpected reboots\n");
- nor->params->set_4byte_addr_mode(nor, true);
+ if (nor->flags & SNOR_F_HAS_PARALLEL) {
+ /*
+ * In parallel mode both chip selects i.e., CS0 &
+ * CS1 need to be asserted simulatneously.
+ */
+ nor->spimem->spi->cs_index_mask = SPI_NOR_ENABLE_BOTH_CS;
+ nor->params[0]->set_4byte_addr_mode(nor, true);
+ } else {
+ for (idx = 0; idx < SNOR_FLASH_CNT_MAX; idx++) {
+ if (nor->params[idx] != NULL) {
+ /*
+ * Select the appropriate CS index before
+ * issuing the command.
+ */
+ nor->spimem->spi->cs_index_mask = 0x01 << idx;
+ nor->params[idx]->set_4byte_addr_mode(nor, true);
+ }
+ }
+ }
}
return 0;
@@ -2913,6 +2987,8 @@ static void spi_nor_set_mtd_info(struct spi_nor *nor)
{
struct mtd_info *mtd = &nor->mtd;
struct device *dev = nor->dev;
+ u64 total_sz =0;
+ int idx;
spi_nor_set_mtd_locking_ops(nor);
spi_nor_set_mtd_otp_ops(nor);
@@ -2926,9 +3002,13 @@ static void spi_nor_set_mtd_info(struct spi_nor *nor)
mtd->flags |= MTD_NO_ERASE;
else
mtd->_erase = spi_nor_erase;
- mtd->writesize = nor->params->writesize;
- mtd->writebufsize = nor->params->page_size;
- mtd->size = nor->params->size;
+ mtd->writesize = nor->params[0]->writesize;
+ mtd->writebufsize = nor->params[0]->page_size;
+ for (idx = 0; idx < SNOR_FLASH_CNT_MAX; idx++) {
+ if (nor->params[idx] != NULL)
+ total_sz += nor->params[idx]->size;
+ }
+ mtd->size = total_sz;
mtd->_read = spi_nor_read;
/* Might be already set by some SST flashes. */
if (!mtd->_write)
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index 3f841ec36e56..4e8bf3cbe331 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -11,6 +11,9 @@
#define SPI_NOR_MAX_ID_LEN 6
+/* In parallel configuration enable both CS */
+#define SPI_NOR_ENABLE_BOTH_CS (BIT(0) | BIT(1))
+
/* Standard SPI NOR flash operations. */
#define SPI_NOR_READID_OP(naddr, ndummy, buf, len) \
SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RDID, 0), \
@@ -130,6 +133,8 @@ enum spi_nor_option_flags {
SNOR_F_IO_MODE_EN_VOLATILE = BIT(11),
SNOR_F_SOFT_RESET = BIT(12),
SNOR_F_SWP_IS_VOLATILE = BIT(13),
+ SNOR_F_HAS_STACKED = BIT(14),
+ SNOR_F_HAS_PARALLEL = BIT(15),
};
struct spi_nor_read_command {
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 1ede4c89805a..40800e7235ee 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -128,6 +128,12 @@
#define SR2_LB3 BIT(5) /* Security Register Lock Bit 3 */
#define SR2_QUAD_EN_BIT7 BIT(7)
+/*
+ * Maximum number of flashes that can be connected
+ * in stacked/parallel configuration
+ */
+#define SNOR_FLASH_CNT_MAX 2
+
/* Supported SPI protocols */
#define SNOR_PROTO_INST_MASK GENMASK(23, 16)
#define SNOR_PROTO_INST_SHIFT 16
@@ -397,7 +403,7 @@ struct spi_nor {
const struct spi_nor_controller_ops *controller_ops;
- struct spi_nor_flash_parameter *params;
+ struct spi_nor_flash_parameter *params[SNOR_FLASH_CNT_MAX];
struct {
struct spi_mem_dirmap_desc *rdesc;
--
2.17.1
On Mon, Jun 06, 2022 at 04:56:06PM +0530, Amit Kumar Mahapatra wrote:
> ---
> drivers/spi/spi-zynqmp-gqspi.c | 30 ++++++++++++++++++++++++++----
> drivers/spi/spi.c | 10 +++++++---
> include/linux/spi/spi.h | 10 +++++++++-
> 3 files changed, 42 insertions(+), 8 deletions(-)
Please split the core and driver support into separate patches, they are
separate things.
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -2082,6 +2082,8 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi,
> {
> u32 value;
> int rc;
> + u32 cs[SPI_CS_CNT_MAX];
> + u8 idx;
>
> /* Mode (clock phase/polarity/etc.) */
> if (of_property_read_bool(nc, "spi-cpha"))
This is changing the DT binding but doesn't have any updates to the
binding document. The binding code also doesn't validate that we don't
have too many chip selects.
> + /* Bit mask of the chipselect(s) that the driver
> + * need to use form the chipselect array.
> + */
> + u8 cs_index_mask : 2;
Why make this a bitfield?
I'm also not seeing anything here that checks that the driver supports
multiple chip selects - it seems like something that's going to cause
issues and we should probably have something to handle that situation.
Hello Mark,
> -----Original Message-----
> From: linux-arm-kernel <[email protected]> On
> Behalf Of Mark Brown
> Sent: Thursday, June 9, 2022 5:24 PM
> To: Amit Kumar Mahapatra <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]; linux-
> [email protected]; [email protected]; linux-
> [email protected]; [email protected]; [email protected]
> Subject: Re: [RFC PATCH 1/2] spi: Add multiple CS support for a single SPI
> device
>
> On Mon, Jun 06, 2022 at 04:56:06PM +0530, Amit Kumar Mahapatra wrote:
>
> > ---
> > drivers/spi/spi-zynqmp-gqspi.c | 30 ++++++++++++++++++++++++++----
> > drivers/spi/spi.c | 10 +++++++---
> > include/linux/spi/spi.h | 10 +++++++++-
> > 3 files changed, 42 insertions(+), 8 deletions(-)
>
> Please split the core and driver support into separate patches, they are
> separate things.
Ok, I will split the patches.
>
> > --- a/drivers/spi/spi.c
> > +++ b/drivers/spi/spi.c
> > @@ -2082,6 +2082,8 @@ static int of_spi_parse_dt(struct spi_controller
> > *ctlr, struct spi_device *spi, {
> > u32 value;
> > int rc;
> > + u32 cs[SPI_CS_CNT_MAX];
> > + u8 idx;
> >
> > /* Mode (clock phase/polarity/etc.) */
> > if (of_property_read_bool(nc, "spi-cpha"))
>
> This is changing the DT binding but doesn't have any updates to the binding
> document. The binding code also doesn't validate that we don't have too
> many chip selects.
The following updates are done in the binding documents for adding multiple
CS support:
In jedec,spi-nor.yaml file " maxItems " of the "reg" DT property has been
updated to accommodate two CS per SPI device.
https://github.com/torvalds/linux/blob/de5c208d533a46a074eb46ea17f672cc005a7269/Documentation/devicetree/bindings/mtd/jedec%2Cspi-nor.yaml#L49
An example of a flash node with two CS has been added in spi-controller.yaml
https://github.com/torvalds/linux/blob/de5c208d533a46a074eb46ea17f672cc005a7269/Documentation/devicetree/bindings/spi/spi-controller.yaml#L141
>
> > + /* Bit mask of the chipselect(s) that the driver
> > + * need to use form the chipselect array.
> > + */
> > + u8 cs_index_mask : 2;
>
> Why make this a bitfield?
https://github.com/torvalds/linux/blob/de5c208d533a46a074eb46ea17f672cc005a7269/Documentation/devicetree/bindings/mtd/jedec%2Cspi-nor.yaml#L49
As per the DT bindings we are supporting max 2 chip selects per SPI device
that is the reason I had taken it as an bitfield of 2. But now I think that in
future when the number of chip selects per device would increase i.e.,
more than 2, then we have to again increase the bitfield allocation for
accommodating the increase in the number of chip selects per SPI device,
So I think it's better to drop the bitfield for now and use cs_index_mask
as an u8
>
> I'm also not seeing anything here that checks that the driver supports
> multiple chip selects - it seems like something that's going to cause issues
> and we should probably have something to handle that situation.
In my approach the chip select member (chip_select) of the spi_device structure
is changed to an array (chip_select[2]). This array is used to store the CS values
coming from the "reg" DT property.
In case of multiple chip selects spi->chip_slect[0] will hold CS0 value &
spi->chip_select[1] wil hold CS1 value.
In case of single chip select the spi->chip_select[0] will hold the chip select value.
As per the current implementation all the drivers fetch their chip select value form
spi->chip_select, but now the driver code needs to be modified to fetch the value
from spi->chip_select[0] instead and by this approach we do not need to check if the
driver supports single or multiple CS.
Hope I addressed all your concerns and please let us know what you think.
Regards,
Amit
On Thu, Jun 23, 2022 at 11:39:19AM +0000, Mahapatra, Amit Kumar wrote:
> > > /* Mode (clock phase/polarity/etc.) */
> > > if (of_property_read_bool(nc, "spi-cpha"))
> > This is changing the DT binding but doesn't have any updates to the binding
> > document. The binding code also doesn't validate that we don't have too
> > many chip selects.
> The following updates are done in the binding documents for adding multiple
> CS support:
> In jedec,spi-nor.yaml file " maxItems " of the "reg" DT property has been
> updated to accommodate two CS per SPI device.
This is a change to a binding for a specific driver, this is changing
the SPI core.
> > I'm also not seeing anything here that checks that the driver supports
> > multiple chip selects - it seems like something that's going to cause issues
> > and we should probably have something to handle that situation.
> In my approach the chip select member (chip_select) of the spi_device structure
> is changed to an array (chip_select[2]). This array is used to store the CS values
> coming from the "reg" DT property.
> In case of multiple chip selects spi->chip_slect[0] will hold CS0 value &
> spi->chip_select[1] wil hold CS1 value.
> In case of single chip select the spi->chip_select[0] will hold the chip select value.
That doesn't address the issue, the issue is checking that the driver
can support multiple chip selects.
Hi Mark,
On 6/9/22 13:54, Mark Brown wrote:
> On Mon, Jun 06, 2022 at 04:56:06PM +0530, Amit Kumar Mahapatra wrote:
>
>> ---
>> drivers/spi/spi-zynqmp-gqspi.c | 30 ++++++++++++++++++++++++++----
>> drivers/spi/spi.c | 10 +++++++---
>> include/linux/spi/spi.h | 10 +++++++++-
>> 3 files changed, 42 insertions(+), 8 deletions(-)
>
> Please split the core and driver support into separate patches, they are
> separate things.
>
>> --- a/drivers/spi/spi.c
>> +++ b/drivers/spi/spi.c
>> @@ -2082,6 +2082,8 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi,
>> {
>> u32 value;
>> int rc;
>> + u32 cs[SPI_CS_CNT_MAX];
>> + u8 idx;
>>
>> /* Mode (clock phase/polarity/etc.) */
>> if (of_property_read_bool(nc, "spi-cpha"))
>
> This is changing the DT binding but doesn't have any updates to the
> binding document. The binding code also doesn't validate that we don't
> have too many chip selects.
I would like to better understand your request here in connection to change in
the binding code for validation.
What exactly do you want to validate?
That child reg property is not bigger than num-cs in controller node?
Adding also Krzysztof because I was talking to him over IRC about this.
Thanks,
Michal
On Mon, Jul 11, 2022 at 02:47:54PM +0200, Michal Simek wrote:
> On 6/9/22 13:54, Mark Brown wrote:
> > On Mon, Jun 06, 2022 at 04:56:06PM +0530, Amit Kumar Mahapatra wrote:
> > > + u32 cs[SPI_CS_CNT_MAX];
> > > + u8 idx;
> > > /* Mode (clock phase/polarity/etc.) */
> > > if (of_property_read_bool(nc, "spi-cpha"))
> > This is changing the DT binding but doesn't have any updates to the
> > binding document. The binding code also doesn't validate that we don't
> > have too many chip selects.
> I would like to better understand your request here in connection to change
> in the binding code for validation.
> What exactly do you want to validate?
> That child reg property is not bigger than num-cs in controller node?
If you are adding support for multiple chip selects in the driver then
there must be some mechanism for expressing that in the bindings which I
would expect to see appear as a change to the binding document.
Hello Mark,
> -----Original Message-----
> From: Mark Brown <[email protected]>
> Sent: Thursday, June 23, 2022 5:37 PM
> To: Mahapatra, Amit Kumar <[email protected]>
> Cc: Amit Kumar Mahapatra <[email protected]>;
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]; linux-
> [email protected]; [email protected]; linux-
> [email protected]; [email protected]; [email protected];
> git (AMD-Xilinx) <[email protected]>
> Subject: Re: [RFC PATCH 1/2] spi: Add multiple CS support for a single SPI
> device
>
> On Thu, Jun 23, 2022 at 11:39:19AM +0000, Mahapatra, Amit Kumar wrote:
>
> > > > /* Mode (clock phase/polarity/etc.) */
> > > > if (of_property_read_bool(nc, "spi-cpha"))
>
> > > This is changing the DT binding but doesn't have any updates to the
> > > binding document. The binding code also doesn't validate that we
> > > don't have too many chip selects.
>
> > The following updates are done in the binding documents for adding
> > multiple CS support:
> > In jedec,spi-nor.yaml file " maxItems " of the "reg" DT property has
> > been updated to accommodate two CS per SPI device.
>
> This is a change to a binding for a specific driver, this is changing the SPI
> core.
>
> > > I'm also not seeing anything here that checks that the driver
> > > supports multiple chip selects - it seems like something that's
> > > going to cause issues and we should probably have something to handle
> that situation.
>
> > In my approach the chip select member (chip_select) of the spi_device
> > structure is changed to an array (chip_select[2]). This array is used
> > to store the CS values coming from the "reg" DT property.
> > In case of multiple chip selects spi->chip_slect[0] will hold CS0
> > value &
> > spi->chip_select[1] wil hold CS1 value.
> > In case of single chip select the spi->chip_select[0] will hold the chip select
> value.
>
> That doesn't address the issue, the issue is checking that the driver can
> support multiple chip selects.
To address this issue, in spi core we will check the number of items
in the "reg" property of the flash node(which is nothing but the
number of chip selects) against the "num-cs" property of the spi
controller(which is total number of chip selects supported by the
controller). If the number of items mentioned in the "reg" property
is greater than "num-cs" value then we error out.
For eg.,
rc = of_property_read_variable_u32_array(nc, "reg", &cs[0], 1,
SPI_CS_CNT_MAX);
if(rc > ctlr->num_chipselect) {
dev_err(&ctlr->dev, "%pOF has invalid 'reg' property (%d)\n",
nc, rc);
return -EINVAL;
}
Please let us know if you have any other approach in mind.
Regards,
Amit
Hello Mark,
> -----Original Message-----
> From: Mark Brown <[email protected]>
> Sent: Monday, July 11, 2022 8:23 PM
> To: Michal Simek <[email protected]>
> Cc: Amit Kumar Mahapatra <[email protected]>; Krzysztof
> Kozlowski <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; linux-arm-
> [email protected]; [email protected]; [email protected];
> [email protected]
> Subject: Re: [RFC PATCH 1/2] spi: Add multiple CS support for a single SPI
> device
>
> On Mon, Jul 11, 2022 at 02:47:54PM +0200, Michal Simek wrote:
> > On 6/9/22 13:54, Mark Brown wrote:
> > > On Mon, Jun 06, 2022 at 04:56:06PM +0530, Amit Kumar Mahapatra
> wrote:
>
> > > > + u32 cs[SPI_CS_CNT_MAX];
> > > > + u8 idx;
> > > > /* Mode (clock phase/polarity/etc.) */
> > > > if (of_property_read_bool(nc, "spi-cpha"))
>
> > > This is changing the DT binding but doesn't have any updates to the
> > > binding document. The binding code also doesn't validate that we
> > > don't have too many chip selects.
>
> > I would like to better understand your request here in connection to
> > change in the binding code for validation.
> > What exactly do you want to validate?
> > That child reg property is not bigger than num-cs in controller node?
>
> If you are adding support for multiple chip selects in the driver then there
> must be some mechanism for expressing that in the bindings which I would
> expect to see appear as a change to the binding document.
Thank you for the clarification.
As per my understanding we would not be needing a new DT
property for expressing multi chip select support in the driver.
We can use "num-cs" property of the spi controller for
broadcasting multi chip select support of the driver.
If "num-cs" value is greater than 1 then the driver can support
multiple chip selects.
Please let us know if you have any other approach in mind.
Regards,
Amit
On Fri, Jul 15, 2022 at 03:35:49PM +0000, Mahapatra, Amit Kumar wrote:
> > That doesn't address the issue, the issue is checking that the driver can
> > support multiple chip selects.
> To address this issue, in spi core we will check the number of items
> in the "reg" property of the flash node(which is nothing but the
> number of chip selects) against the "num-cs" property of the spi
> controller(which is total number of chip selects supported by the
> controller). If the number of items mentioned in the "reg" property
> is greater than "num-cs" value then we error out.
> For eg.,
> rc = of_property_read_variable_u32_array(nc, "reg", &cs[0], 1,
> SPI_CS_CNT_MAX);
> if(rc > ctlr->num_chipselect) {
> dev_err(&ctlr->dev, "%pOF has invalid 'reg' property (%d)\n",
> nc, rc);
> return -EINVAL;
> }
This would check that the controller has at least the number of chip
selects specified but it would not check that the controller is actually
capable of using more than one chip select at once. We should be
validating both that the chip selects are available and that the
controller can do something useful with them (and probably have an
implementation in the core for doing so via GPIO).
On Fri, Jul 15, 2022 at 03:36:23PM +0000, Mahapatra, Amit Kumar wrote:
> Thank you for the clarification.
> As per my understanding we would not be needing a new DT
> property for expressing multi chip select support in the driver.
> We can use "num-cs" property of the spi controller for
> broadcasting multi chip select support of the driver.
> If "num-cs" value is greater than 1 then the driver can support
> multiple chip selects.
That's true in some sense but not in the sense of being actually able to
use more than one chip select for a single device. If the controller
hardware manages the chip selects it may not be physically possible for
it to control more than one chip select.
While it's not an issue for the bindings even hardware that can
physically do it is going to need a driver update (in the core for GPIO
stuff) to do so.
Hello Mark,
> -----Original Message-----
> From: Mark Brown <[email protected]>
> Sent: Friday, July 15, 2022 9:24 PM
> To: Mahapatra, Amit Kumar <[email protected]>
> Cc: Amit Kumar Mahapatra <[email protected]>;
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]; linux-
> [email protected]; [email protected]; linux-
> [email protected]; [email protected]; [email protected];
> git (AMD-Xilinx) <[email protected]>
> Subject: Re: [RFC PATCH 1/2] spi: Add multiple CS support for a single SPI
> device
>
> On Fri, Jul 15, 2022 at 03:35:49PM +0000, Mahapatra, Amit Kumar wrote:
>
> > > That doesn't address the issue, the issue is checking that the
> > > driver can support multiple chip selects.
>
> > To address this issue, in spi core we will check the number of items
> > in the "reg" property of the flash node(which is nothing but the
> > number of chip selects) against the "num-cs" property of the spi
> > controller(which is total number of chip selects supported by the
> > controller). If the number of items mentioned in the "reg" property is
> > greater than "num-cs" value then we error out.
>
> > For eg.,
>
> > rc = of_property_read_variable_u32_array(nc, "reg", &cs[0], 1,
> > SPI_CS_CNT_MAX);
> > if(rc > ctlr->num_chipselect) {
> > dev_err(&ctlr->dev, "%pOF has invalid 'reg' property (%d)\n",
> > nc, rc);
> > return -EINVAL;
> > }
>
> This would check that the controller has at least the number of chip selects
> specified but it would not check that the controller is actually capable of
> using more than one chip select at once. We should be validating both that
I agree, so for checking the controller multiple chip select capability(using
more than one chip select at once) we can define a new spi controller DT
property like "multi-cs-cap"(please suggest a better name).
The controller that can support multiple chip selects should have this property
in the spi controller DT node. The spi core will check ctlr->multi-cs-cap to
operate multiple chip select in parallel.
> the chip selects are available and that the controller can do something useful
> with them (and probably have an implementation in the core for doing so via
> GPIO).
Here are you referring to the usecase in which a controller implementing multi CS
support using GPIO?
Regards,
Amit
On Tue, Jul 19, 2022 at 01:21:41PM +0000, Mahapatra, Amit Kumar wrote:
> I agree, so for checking the controller multiple chip select capability(using
> more than one chip select at once) we can define a new spi controller DT
> property like "multi-cs-cap"(please suggest a better name).
> The controller that can support multiple chip selects should have this property
> in the spi controller DT node. The spi core will check ctlr->multi-cs-cap to
> operate multiple chip select in parallel.
I'm not sure this needs to be a DT property, it's more just something we
infer from the compatible. The name seems fine, as does the flag in the
controller data.
> > the chip selects are available and that the controller can do something useful
> > with them (and probably have an implementation in the core for doing so via
> > GPIO).
> Here are you referring to the usecase in which a controller implementing multi CS
> support using GPIO?
Yes, we probably ought to.
Hello Mark,
> -----Original Message-----
> From: Mark Brown <[email protected]>
> Sent: Tuesday, July 19, 2022 11:23 PM
> To: Mahapatra, Amit Kumar <[email protected]>
> Cc: Amit Kumar Mahapatra <[email protected]>;
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]; linux-
> [email protected]; [email protected]; linux-
> [email protected]; [email protected]; [email protected];
> git (AMD-Xilinx) <[email protected]>
> Subject: Re: [RFC PATCH 1/2] spi: Add multiple CS support for a single SPI
> device
>
> On Tue, Jul 19, 2022 at 01:21:41PM +0000, Mahapatra, Amit Kumar wrote:
>
> > I agree, so for checking the controller multiple chip select
> > capability(using more than one chip select at once) we can define a
> > new spi controller DT property like "multi-cs-cap"(please suggest a better
> name).
> > The controller that can support multiple chip selects should have this
> > property in the spi controller DT node. The spi core will check
> > ctlr->multi-cs-cap to operate multiple chip select in parallel.
>
> I'm not sure this needs to be a DT property, it's more just something we infer
> from the compatible. The name seems fine, as does the flag in the controller
> data.
I agree that we can infer this from the compatible and set the flag in the controller data.
>
> > > the chip selects are available and that the controller can do
> > > something useful with them (and probably have an implementation in
> > > the core for doing so via GPIO).
>
> > Here are you referring to the usecase in which a controller
> > implementing multi CS support using GPIO?
>
> Yes, we probably ought to.
In my next version I will add the implementation in the spi core for multi CS support using GPIO, but I will not be able test it as I don't have the necessary hardware setup .
Regards,
Amit