| Message ID | 20241025161501.485684-5-miquel.raynal@bootlin.com |
|---|---|
| State | New |
| Headers | show |
| Series | spi-nand/spi-mem DTR support | expand |
On 10/25/24 5:14 PM, Miquel Raynal wrote: > Every ->exec_op() call correctly configures the spi bus speed to the > maximum allowed frequency for the memory using the constant spi default > parameter. Since we can now have per-operation constraints, let's use > the value that comes from the spi-mem operation structure instead. In > case there is no specific limitation for this operation, the default spi > device value will be given anyway. > > The per-operation frequency capability is thus advertised to the spi-mem > core. > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- > drivers/spi/spi-amlogic-spifc-a1.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/spi/spi-amlogic-spifc-a1.c b/drivers/spi/spi-amlogic-spifc-a1.c > index fadf6667cd51..18c9aa2cbc29 100644 > --- a/drivers/spi/spi-amlogic-spifc-a1.c > +++ b/drivers/spi/spi-amlogic-spifc-a1.c > @@ -259,7 +259,7 @@ static int amlogic_spifc_a1_exec_op(struct spi_mem *mem, > size_t data_size = op->data.nbytes; > int ret; > > - ret = amlogic_spifc_a1_set_freq(spifc, mem->spi->max_speed_hz); > + ret = amlogic_spifc_a1_set_freq(spifc, op->max_freq); > if (ret) > return ret; > > @@ -320,6 +320,10 @@ static const struct spi_controller_mem_ops amlogic_spifc_a1_mem_ops = { > .adjust_op_size = amlogic_spifc_a1_adjust_op_size, > }; I see the driver sets ctrl->min_speed_hz = SPIFC_A1_MIN_HZ; Do you want to introduce a struct spi_controller_mem_ops.supports_op and check that the spimem op freq is not below the controller's minimum freq?
Hi Tudor, On 11/11/2024 at 13:42:31 GMT, Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > On 10/25/24 5:14 PM, Miquel Raynal wrote: >> Every ->exec_op() call correctly configures the spi bus speed to the >> maximum allowed frequency for the memory using the constant spi default >> parameter. Since we can now have per-operation constraints, let's use >> the value that comes from the spi-mem operation structure instead. In >> case there is no specific limitation for this operation, the default spi >> device value will be given anyway. >> >> The per-operation frequency capability is thus advertised to the spi-mem >> core. > > I see the driver sets ctrl->min_speed_hz = SPIFC_A1_MIN_HZ; > > Do you want to introduce a struct spi_controller_mem_ops.supports_op and > check that the spimem op freq is not below the controller's minimum freq? I was about to do that but I think we can do better. I am already tuning the max frequency depending on the op. I can just check in the default supports_op hook that the operation max frequency is not below the controller's minimum. So all drivers with this kind of limitation will be covered. Thanks for the heads up! Miquèl
On 12/13/24 11:44 AM, Miquel Raynal wrote: > Hi Tudor, > > On 11/11/2024 at 13:42:31 GMT, Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > >> On 10/25/24 5:14 PM, Miquel Raynal wrote: >>> Every ->exec_op() call correctly configures the spi bus speed to the >>> maximum allowed frequency for the memory using the constant spi default >>> parameter. Since we can now have per-operation constraints, let's use >>> the value that comes from the spi-mem operation structure instead. In >>> case there is no specific limitation for this operation, the default spi >>> device value will be given anyway. >>> >>> The per-operation frequency capability is thus advertised to the spi-mem >>> core. >> >> I see the driver sets ctrl->min_speed_hz = SPIFC_A1_MIN_HZ; >> >> Do you want to introduce a struct spi_controller_mem_ops.supports_op and >> check that the spimem op freq is not below the controller's minimum freq? > > I was about to do that but I think we can do better. I am already tuning > the max frequency depending on the op. I can just check in the default > supports_op hook that the operation max frequency is not below the > controller's minimum. So all drivers with this kind of limitation will > be covered. > Sounds good!
diff --git a/drivers/spi/spi-amlogic-spifc-a1.c b/drivers/spi/spi-amlogic-spifc-a1.c index fadf6667cd51..18c9aa2cbc29 100644 --- a/drivers/spi/spi-amlogic-spifc-a1.c +++ b/drivers/spi/spi-amlogic-spifc-a1.c @@ -259,7 +259,7 @@ static int amlogic_spifc_a1_exec_op(struct spi_mem *mem, size_t data_size = op->data.nbytes; int ret; - ret = amlogic_spifc_a1_set_freq(spifc, mem->spi->max_speed_hz); + ret = amlogic_spifc_a1_set_freq(spifc, op->max_freq); if (ret) return ret; @@ -320,6 +320,10 @@ static const struct spi_controller_mem_ops amlogic_spifc_a1_mem_ops = { .adjust_op_size = amlogic_spifc_a1_adjust_op_size, }; +static const struct spi_controller_mem_caps amlogic_spifc_a1_mem_caps = { + .per_op_freq = true, +}; + static int amlogic_spifc_a1_probe(struct platform_device *pdev) { struct spi_controller *ctrl; @@ -356,6 +360,7 @@ static int amlogic_spifc_a1_probe(struct platform_device *pdev) ctrl->bits_per_word_mask = SPI_BPW_MASK(8); ctrl->auto_runtime_pm = true; ctrl->mem_ops = &amlogic_spifc_a1_mem_ops; + ctrl->mem_caps = &amlogic_spifc_a1_mem_caps; ctrl->min_speed_hz = SPIFC_A1_MIN_HZ; ctrl->max_speed_hz = SPIFC_A1_MAX_HZ; ctrl->mode_bits = (SPI_RX_DUAL | SPI_TX_DUAL |
Every ->exec_op() call correctly configures the spi bus speed to the maximum allowed frequency for the memory using the constant spi default parameter. Since we can now have per-operation constraints, let's use the value that comes from the spi-mem operation structure instead. In case there is no specific limitation for this operation, the default spi device value will be given anyway. The per-operation frequency capability is thus advertised to the spi-mem core. Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- drivers/spi/spi-amlogic-spifc-a1.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)