| Message ID | 20221025064623.22808-3-mika.westerberg@linux.intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 8a9a784fb337cfd07f305faf5358335d4c12a788 |
| Headers | show |
| Series | spi: intel: Add support for SFDP opcode | expand |
Hi Mika, On 10/25/2022 12:16 PM, Mika Westerberg wrote: > This allows us to get rid of the checks in the intel_spi_[sh]w_cycle() > and makes it possible for the SPI-NOR core to split the transaction into > smaller chunks as needed. If I understand correctly, the controller ops are called from spi-mem.c, and spi_mem_adjust_op does exist and on it's own does "split a data transfer operation into multiple ones" So is this really something that you require the controller to do and can we not rely on spi-mem.c to do it's thing instead? I would like to point you to another controller spi-cadence-quadspi.c that also doesn't have it's own adjust_op_size but I haven't seen any issues as such inspite. This is because everything first goes through spi-mem.c then onto the controllers drivers. This atleast is my understanding, please correct me if I am mistaken. > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > --- > drivers/spi/spi-intel.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/drivers/spi/spi-intel.c b/drivers/spi/spi-intel.c > index b3685460d848..13a3a61239d2 100644 > --- a/drivers/spi/spi-intel.c > +++ b/drivers/spi/spi-intel.c > @@ -363,10 +363,6 @@ static int intel_spi_hw_cycle(struct intel_spi *ispi, > > val = readl(ispi->base + HSFSTS_CTL); > val &= ~(HSFSTS_CTL_FCYCLE_MASK | HSFSTS_CTL_FDBC_MASK); > - > - if (len > INTEL_SPI_FIFO_SZ) > - return -EINVAL; > - > val |= (len - 1) << HSFSTS_CTL_FDBC_SHIFT; > val |= HSFSTS_CTL_FCERR | HSFSTS_CTL_FDONE; > val |= HSFSTS_CTL_FGO; > @@ -397,9 +393,6 @@ static int intel_spi_sw_cycle(struct intel_spi *ispi, u8 opcode, size_t len, > if (ret < 0) > return ret; > > - if (len > INTEL_SPI_FIFO_SZ) > - return -EINVAL; > - > /* > * Always clear it after each SW sequencer operation regardless > * of whether it is successful or not. > @@ -704,6 +697,12 @@ static int intel_spi_erase(struct intel_spi *ispi, const struct spi_mem *mem, > return 0; > } > > +static int intel_spi_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op) > +{ > + op->data.nbytes = clamp_val(op->data.nbytes, 0, INTEL_SPI_FIFO_SZ); > + return 0; > +} > + > static bool intel_spi_cmp_mem_op(const struct intel_spi_mem_op *iop, > const struct spi_mem_op *op) > { > @@ -844,6 +843,7 @@ static ssize_t intel_spi_dirmap_write(struct spi_mem_dirmap_desc *desc, u64 offs > } > > static const struct spi_controller_mem_ops intel_spi_mem_ops = { > + .adjust_op_size = intel_spi_adjust_op_size, > .supports_op = intel_spi_supports_mem_op, > .exec_op = intel_spi_exec_mem_op, > .get_name = intel_spi_get_name,
Hi, On Fri, Oct 28, 2022 at 11:42:09AM +0530, Gole, Dhruva wrote: > Hi Mika, > > On 10/25/2022 12:16 PM, Mika Westerberg wrote: > > This allows us to get rid of the checks in the intel_spi_[sh]w_cycle() > > and makes it possible for the SPI-NOR core to split the transaction into > > smaller chunks as needed. > > If I understand correctly, the controller ops are called from spi-mem.c, and > > spi_mem_adjust_op does exist and on it's own does "split a data transfer > operation into multiple ones" > > So is this really something that you require the controller to do and > > can we not rely on spi-mem.c to do it's thing instead? How does it know the size supported by the hardware? I think this is the reason spi_mem_adjust_op was introduced but I may be mistaken. > I would like to point you to another controller spi-cadence-quadspi.c > > that also doesn't have it's own adjust_op_size but I haven't seen any issues > as such > > inspite. This is because everything first goes through spi-mem.c then onto > the controllers drivers. Well Intel SPI driver did not not have any issues previously either because it handled the read/write split internally but SFDP read is done through "register read side" which only supported max 64-byte read until now :-)
On 10/28/2022 11:55 AM, Mika Westerberg wrote: > Hi, > > On Fri, Oct 28, 2022 at 11:42:09AM +0530, Gole, Dhruva wrote: >> Hi Mika, >> >> On 10/25/2022 12:16 PM, Mika Westerberg wrote: >>> This allows us to get rid of the checks in the intel_spi_[sh]w_cycle() >>> and makes it possible for the SPI-NOR core to split the transaction into >>> smaller chunks as needed. >> If I understand correctly, the controller ops are called from spi-mem.c, and >> >> spi_mem_adjust_op does exist and on it's own does "split a data transfer >> operation into multiple ones" >> >> So is this really something that you require the controller to do and >> >> can we not rely on spi-mem.c to do it's thing instead? > How does it know the size supported by the hardware? I think this is the > reason spi_mem_adjust_op was introduced but I may be mistaken.' The following piece of code might help: op->data.nbytes = min3((size_t)op->data.nbytes, spi_max_transfer_size(mem->spi), spi_max_message_size(mem->spi) I believe this will help it know the size supported by the hardware. and on the controller side: in case of cadence I have seen it pickup the fifo depth from DTSI, for eg. arch/arm64/boot/dts/ti/k3-am62-main.dtsi: cdns,fifo-depth = <256>; > >> I would like to point you to another controller spi-cadence-quadspi.c >> >> that also doesn't have it's own adjust_op_size but I haven't seen any issues >> as such >> >> inspite. This is because everything first goes through spi-mem.c then onto >> the controllers drivers. > Well Intel SPI driver did not not have any issues previously either > because it handled the read/write split internally but SFDP read is done > through "register read side" which only supported max 64-byte read until > now :-)
Hi, On Fri, Oct 28, 2022 at 12:16:43PM +0530, Gole, Dhruva wrote: > > On 10/28/2022 11:55 AM, Mika Westerberg wrote: > > Hi, > > > > On Fri, Oct 28, 2022 at 11:42:09AM +0530, Gole, Dhruva wrote: > > > Hi Mika, > > > > > > On 10/25/2022 12:16 PM, Mika Westerberg wrote: > > > > This allows us to get rid of the checks in the intel_spi_[sh]w_cycle() > > > > and makes it possible for the SPI-NOR core to split the transaction into > > > > smaller chunks as needed. > > > If I understand correctly, the controller ops are called from spi-mem.c, and > > > > > > spi_mem_adjust_op does exist and on it's own does "split a data transfer > > > operation into multiple ones" > > > > > > So is this really something that you require the controller to do and > > > > > > can we not rely on spi-mem.c to do it's thing instead? > > How does it know the size supported by the hardware? I think this is the > > reason spi_mem_adjust_op was introduced but I may be mistaken.' > > The following piece of code might help: > > op->data.nbytes = min3((size_t)op->data.nbytes, > spi_max_transfer_size(mem->spi), > spi_max_message_size(mem->spi) > > I believe this will help it know the size supported by the hardware. > > and on the controller side: > > in case of cadence I have seen it pickup the fifo depth from DTSI, for eg. > > arch/arm64/boot/dts/ti/k3-am62-main.dtsi: cdns,fifo-depth = <256>; I'm not entirely sure I understand what you are suggesting to be honest? ;-) For example the Intel SPI controller does not have any sort of device tree description, it does not really understand anything about SPI either (internally yes but the operations it exposes to software are higher level), and it has fixed 64 byte FIFO. Can I get the SFDP opcode supported with that without implementing custom spi_mem_adjust_op() and if yes, how? Thanks!
diff --git a/drivers/spi/spi-intel.c b/drivers/spi/spi-intel.c index b3685460d848..13a3a61239d2 100644 --- a/drivers/spi/spi-intel.c +++ b/drivers/spi/spi-intel.c @@ -363,10 +363,6 @@ static int intel_spi_hw_cycle(struct intel_spi *ispi, val = readl(ispi->base + HSFSTS_CTL); val &= ~(HSFSTS_CTL_FCYCLE_MASK | HSFSTS_CTL_FDBC_MASK); - - if (len > INTEL_SPI_FIFO_SZ) - return -EINVAL; - val |= (len - 1) << HSFSTS_CTL_FDBC_SHIFT; val |= HSFSTS_CTL_FCERR | HSFSTS_CTL_FDONE; val |= HSFSTS_CTL_FGO; @@ -397,9 +393,6 @@ static int intel_spi_sw_cycle(struct intel_spi *ispi, u8 opcode, size_t len, if (ret < 0) return ret; - if (len > INTEL_SPI_FIFO_SZ) - return -EINVAL; - /* * Always clear it after each SW sequencer operation regardless * of whether it is successful or not. @@ -704,6 +697,12 @@ static int intel_spi_erase(struct intel_spi *ispi, const struct spi_mem *mem, return 0; } +static int intel_spi_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op) +{ + op->data.nbytes = clamp_val(op->data.nbytes, 0, INTEL_SPI_FIFO_SZ); + return 0; +} + static bool intel_spi_cmp_mem_op(const struct intel_spi_mem_op *iop, const struct spi_mem_op *op) { @@ -844,6 +843,7 @@ static ssize_t intel_spi_dirmap_write(struct spi_mem_dirmap_desc *desc, u64 offs } static const struct spi_controller_mem_ops intel_spi_mem_ops = { + .adjust_op_size = intel_spi_adjust_op_size, .supports_op = intel_spi_supports_mem_op, .exec_op = intel_spi_exec_mem_op, .get_name = intel_spi_get_name,
This allows us to get rid of the checks in the intel_spi_[sh]w_cycle() and makes it possible for the SPI-NOR core to split the transaction into smaller chunks as needed. Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> --- drivers/spi/spi-intel.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)