| Message ID | 20220810093215.794977-2-patrice.chotard@foss.st.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | spi: stm32_qspi: use QSPI bus as 8 lines communication channel | expand |
On Wed, Aug 10, 2022 at 11:32:14AM +0200, patrice.chotard@foss.st.com wrote: > Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com> > > v2: _ use parallel-memories property > _ set auto_runtime_pm to true > _ remove pm_runtime_*() usage in transfer_one_message() callback > --- The changelog should come after the --- so that it gets automatically stripped out by tooling. No need to resend just for this. > + /* > + * Dual flash mode is only enable in case "parallel-memories" and > + * "cs-gpios" properties are found in DT > + */ > + if (of_property_read_bool(dev->of_node, "parallel-memories") && > + of_gpio_named_count(dev->of_node, "cs-gpios")) { > + qspi->cr_reg = CR_DFM; > + dev_dbg(dev, "Dual flash mode enable"); > + } Do we need to add something to the DT bindings to indicate that parallel-memories is valid?
Hi Mark On 8/10/22 15:06, Mark Brown wrote: > On Wed, Aug 10, 2022 at 11:32:14AM +0200, patrice.chotard@foss.st.com wrote: > >> Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com> >> >> v2: _ use parallel-memories property >> _ set auto_runtime_pm to true >> _ remove pm_runtime_*() usage in transfer_one_message() callback >> --- > > The changelog should come after the --- so that it gets automatically > stripped out by tooling. No need to resend just for this. > >> + /* >> + * Dual flash mode is only enable in case "parallel-memories" and >> + * "cs-gpios" properties are found in DT >> + */ >> + if (of_property_read_bool(dev->of_node, "parallel-memories") && >> + of_gpio_named_count(dev->of_node, "cs-gpios")) { >> + qspi->cr_reg = CR_DFM; >> + dev_dbg(dev, "Dual flash mode enable"); >> + } > > Do we need to add something to the DT bindings to indicate that > parallel-memories is valid? You mean in the st,stm32-qspi.yaml DT binding file ? Right i think it could be preferable to add it. Patrice
On Wed, Aug 10, 2022 at 03:15:08PM +0200, Patrice CHOTARD wrote: > On 8/10/22 15:06, Mark Brown wrote: > > Do we need to add something to the DT bindings to indicate that > > parallel-memories is valid? > You mean in the st,stm32-qspi.yaml DT binding file ? Right i think it could be preferable to add it. Yes. Though I'm not clear if the bindings actually want to enforce it there, it's a device level property not a controller level one so it might not be something where controller support gets validated.
On 8/10/22 15:23, Mark Brown wrote: > On Wed, Aug 10, 2022 at 03:15:08PM +0200, Patrice CHOTARD wrote: >> On 8/10/22 15:06, Mark Brown wrote: > >>> Do we need to add something to the DT bindings to indicate that >>> parallel-memories is valid? > >> You mean in the st,stm32-qspi.yaml DT binding file ? Right i think it could be preferable to add it. > > Yes. Though I'm not clear if the bindings actually want to enforce it > there, it's a device level property not a controller level one so it > might not be something where controller support gets validated. Ah yes, i see, parallel-memories should not be used in our qspi controller node. So i can't reuse parallel-memories for my purpose. So i need to add a new proprietary property at controller level as done in the v1 ?
On Wed, Aug 10, 2022 at 03:31:59PM +0200, Patrice CHOTARD wrote: > On 8/10/22 15:23, Mark Brown wrote: > > On Wed, Aug 10, 2022 at 03:15:08PM +0200, Patrice CHOTARD wrote: > > Yes. Though I'm not clear if the bindings actually want to enforce it > > there, it's a device level property not a controller level one so it > > might not be something where controller support gets validated. > Ah yes, i see, parallel-memories should not be used in our qspi controller node. > So i can't reuse parallel-memories for my purpose. > So i need to add a new proprietary property at controller level as done in the v1 ? Can't the controller figure this out by looking at the properties of the connected devices? You'd need to just return an error if we ever triggered transfer_one_message() on a device that can't support the operation.
On 8/10/22 15:40, Mark Brown wrote: > On Wed, Aug 10, 2022 at 03:31:59PM +0200, Patrice CHOTARD wrote: >> On 8/10/22 15:23, Mark Brown wrote: >>> On Wed, Aug 10, 2022 at 03:15:08PM +0200, Patrice CHOTARD wrote: > >>> Yes. Though I'm not clear if the bindings actually want to enforce it >>> there, it's a device level property not a controller level one so it >>> might not be something where controller support gets validated. > >> Ah yes, i see, parallel-memories should not be used in our qspi controller node. >> So i can't reuse parallel-memories for my purpose. > >> So i need to add a new proprietary property at controller level as done in the v1 ? > > Can't the controller figure this out by looking at the properties of the > connected devices? You'd need to just return an error if we ever > triggered transfer_one_message() on a device that can't support the > operation. It should be a solution. I just noticed another point, property parallel-memories is an array of uint64 which represent device's size. In case a FPGA is connected to the qspi 8 line bus, parallel-memories property will be set with what ? simply random value to make dtbs_check happy ? IMHO, adding a new proprietary property would be cleaner.
On Wed, Aug 10, 2022 at 03:52:39PM +0200, Patrice CHOTARD wrote: > On 8/10/22 15:40, Mark Brown wrote: > > Can't the controller figure this out by looking at the properties of the > > connected devices? You'd need to just return an error if we ever > > triggered transfer_one_message() on a device that can't support the > > operation. > It should be a solution. > I just noticed another point, property parallel-memories is an array of uint64 which represent device's size. > In case a FPGA is connected to the qspi 8 line bus, parallel-memories property will be set with what ? > simply random value to make dtbs_check happy ? > IMHO, adding a new proprietary property would be cleaner. I tend to agree that this is all rather unclear for things that aren't actually storage.
diff --git a/drivers/spi/spi-stm32-qspi.c b/drivers/spi/spi-stm32-qspi.c index f3fe92300639..a649b28ab111 100644 --- a/drivers/spi/spi-stm32-qspi.c +++ b/drivers/spi/spi-stm32-qspi.c @@ -15,6 +15,7 @@ #include <linux/mutex.h> #include <linux/of.h> #include <linux/of_device.h> +#include <linux/of_gpio.h> #include <linux/pinctrl/consumer.h> #include <linux/pm_runtime.h> #include <linux/platform_device.h> @@ -355,10 +356,10 @@ static int stm32_qspi_get_mode(u8 buswidth) return buswidth; } -static int stm32_qspi_send(struct spi_mem *mem, const struct spi_mem_op *op) +static int stm32_qspi_send(struct spi_device *spi, const struct spi_mem_op *op) { - struct stm32_qspi *qspi = spi_controller_get_devdata(mem->spi->master); - struct stm32_qspi_flash *flash = &qspi->flash[mem->spi->chip_select]; + struct stm32_qspi *qspi = spi_controller_get_devdata(spi->master); + struct stm32_qspi_flash *flash = &qspi->flash[spi->chip_select]; u32 ccr, cr; int timeout, err = 0, err_poll_status = 0; @@ -465,7 +466,7 @@ static int stm32_qspi_poll_status(struct spi_mem *mem, const struct spi_mem_op * qspi->fmode = CCR_FMODE_APM; qspi->status_timeout = timeout_ms; - ret = stm32_qspi_send(mem, op); + ret = stm32_qspi_send(mem->spi, op); mutex_unlock(&qspi->lock); pm_runtime_mark_last_busy(qspi->dev); @@ -489,7 +490,7 @@ static int stm32_qspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op) else qspi->fmode = CCR_FMODE_INDW; - ret = stm32_qspi_send(mem, op); + ret = stm32_qspi_send(mem->spi, op); mutex_unlock(&qspi->lock); pm_runtime_mark_last_busy(qspi->dev); @@ -545,7 +546,7 @@ static ssize_t stm32_qspi_dirmap_read(struct spi_mem_dirmap_desc *desc, else qspi->fmode = CCR_FMODE_INDR; - ret = stm32_qspi_send(desc->mem, &op); + ret = stm32_qspi_send(desc->mem->spi, &op); mutex_unlock(&qspi->lock); pm_runtime_mark_last_busy(qspi->dev); @@ -554,6 +555,81 @@ static ssize_t stm32_qspi_dirmap_read(struct spi_mem_dirmap_desc *desc, return ret ?: len; } +static int stm32_qspi_transfer_one_message(struct spi_controller *ctrl, + struct spi_message *msg) +{ + struct stm32_qspi *qspi = spi_controller_get_devdata(ctrl); + struct spi_transfer *transfer; + struct spi_device *spi = msg->spi; + struct spi_mem_op op; + int ret; + + if (!spi->cs_gpiod) + return -EOPNOTSUPP; + + mutex_lock(&qspi->lock); + + gpiod_set_value_cansleep(spi->cs_gpiod, true); + + list_for_each_entry(transfer, &msg->transfers, transfer_list) { + u8 dummy_bytes = 0; + + memset(&op, 0, sizeof(op)); + + dev_dbg(qspi->dev, "tx_buf:%p tx_nbits:%d rx_buf:%p rx_nbits:%d len:%d dummy_data:%d\n", + transfer->tx_buf, transfer->tx_nbits, + transfer->rx_buf, transfer->rx_nbits, + transfer->len, transfer->dummy_data); + + /* + * QSPI hardware supports dummy bytes transfer. + * If current transfer is dummy byte, merge it with the next + * transfer in order to take into account QSPI block constraint + */ + if (transfer->dummy_data) { + op.dummy.buswidth = transfer->tx_nbits; + op.dummy.nbytes = transfer->len; + dummy_bytes = transfer->len; + + /* if happens, means that message is not correctly built */ + if (list_is_last(&transfer->transfer_list, &msg->transfers)) + goto end_of_transfer; + + transfer = list_next_entry(transfer, transfer_list); + } + + op.data.nbytes = transfer->len; + + if (transfer->rx_buf) { + qspi->fmode = CCR_FMODE_INDR; + op.data.buswidth = transfer->rx_nbits; + op.data.dir = SPI_MEM_DATA_IN; + op.data.buf.in = transfer->rx_buf; + } else { + qspi->fmode = CCR_FMODE_INDW; + op.data.buswidth = transfer->tx_nbits; + op.data.dir = SPI_MEM_DATA_OUT; + op.data.buf.out = transfer->tx_buf; + } + + ret = stm32_qspi_send(spi, &op); + if (ret) + goto end_of_transfer; + + msg->actual_length += transfer->len + dummy_bytes; + } + +end_of_transfer: + gpiod_set_value_cansleep(spi->cs_gpiod, false); + + mutex_unlock(&qspi->lock); + + msg->status = ret; + spi_finalize_current_message(ctrl); + + return ret; +} + static int stm32_qspi_setup(struct spi_device *spi) { struct spi_controller *ctrl = spi->master; @@ -579,7 +655,7 @@ static int stm32_qspi_setup(struct spi_device *spi) flash->presc = presc; mutex_lock(&qspi->lock); - qspi->cr_reg = CR_APMS | 3 << CR_FTHRES_SHIFT | CR_SSHIFT | CR_EN; + qspi->cr_reg |= CR_APMS | 3 << CR_FTHRES_SHIFT | CR_SSHIFT | CR_EN; writel_relaxed(qspi->cr_reg, qspi->io_base + QSPI_CR); /* set dcr fsize to max address */ @@ -741,11 +817,24 @@ static int stm32_qspi_probe(struct platform_device *pdev) mutex_init(&qspi->lock); + /* + * Dual flash mode is only enable in case "parallel-memories" and + * "cs-gpios" properties are found in DT + */ + if (of_property_read_bool(dev->of_node, "parallel-memories") && + of_gpio_named_count(dev->of_node, "cs-gpios")) { + qspi->cr_reg = CR_DFM; + dev_dbg(dev, "Dual flash mode enable"); + } + ctrl->mode_bits = SPI_RX_DUAL | SPI_RX_QUAD | SPI_TX_DUAL | SPI_TX_QUAD; ctrl->setup = stm32_qspi_setup; ctrl->bus_num = -1; ctrl->mem_ops = &stm32_qspi_mem_ops; + ctrl->use_gpio_descriptors = true; + ctrl->transfer_one_message = stm32_qspi_transfer_one_message; + ctrl->auto_runtime_pm = true; ctrl->num_chipselect = STM32_QSPI_MAX_NORCHIP; ctrl->dev.of_node = dev->of_node;