| Message ID | 20220606112607.20800-2-amit.kumar-mahapatra@xilinx.com |
|---|---|
| State | New |
| Headers | show |
| Series | spi: Add support for stacked/parallel memories | expand |
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.
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 <broonie@kernel.org> > Sent: Thursday, June 23, 2022 5:37 PM > To: Mahapatra, Amit Kumar <amit.kumar-mahapatra@amd.com> > Cc: Amit Kumar Mahapatra <amit.kumar-mahapatra@xilinx.com>; > p.yadav@ti.com; miquel.raynal@bootlin.com; richard@nod.at; > vigneshr@ti.com; git@xilinx.com; michal.simek@xilinx.com; linux- > spi@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux- > kernel@vger.kernel.org; michael@walle.cc; linux-mtd@lists.infradead.org; > git (AMD-Xilinx) <git@amd.com> > 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 <broonie@kernel.org> > Sent: Monday, July 11, 2022 8:23 PM > To: Michal Simek <michal.simek@xilinx.com> > Cc: Amit Kumar Mahapatra <amit.kumar-mahapatra@xilinx.com>; Krzysztof > Kozlowski <krzysztof.kozlowski+dt@linaro.org>; p.yadav@ti.com; > miquel.raynal@bootlin.com; richard@nod.at; vigneshr@ti.com; > git@xilinx.com; linux-spi@vger.kernel.org; linux-arm- > kernel@lists.infradead.org; linux-kernel@vger.kernel.org; michael@walle.cc; > linux-mtd@lists.infradead.org > 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 <broonie@kernel.org> > Sent: Friday, July 15, 2022 9:24 PM > To: Mahapatra, Amit Kumar <amit.kumar-mahapatra@amd.com> > Cc: Amit Kumar Mahapatra <amit.kumar-mahapatra@xilinx.com>; > p.yadav@ti.com; miquel.raynal@bootlin.com; richard@nod.at; > vigneshr@ti.com; git@xilinx.com; michal.simek@xilinx.com; linux- > spi@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux- > kernel@vger.kernel.org; michael@walle.cc; linux-mtd@lists.infradead.org; > git (AMD-Xilinx) <git@amd.com> > 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 <broonie@kernel.org> > Sent: Tuesday, July 19, 2022 11:23 PM > To: Mahapatra, Amit Kumar <amit.kumar-mahapatra@amd.com> > Cc: Amit Kumar Mahapatra <amit.kumar-mahapatra@xilinx.com>; > p.yadav@ti.com; miquel.raynal@bootlin.com; richard@nod.at; > vigneshr@ti.com; git@xilinx.com; michal.simek@xilinx.com; linux- > spi@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux- > kernel@vger.kernel.org; michael@walle.cc; linux-mtd@lists.infradead.org; > git (AMD-Xilinx) <git@amd.com> > 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
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:
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 <amit.kumar-mahapatra@xilinx.com> --- 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(-)