From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sean Anderson Date: Mon, 5 Apr 2021 09:16:32 -0400 Subject: [PATCH v8 04/28] spi: spi-mem: add spi_mem_dtr_supports_op() In-Reply-To: <20210405074032.uhuoeegby3f6zn4d@ti.com> References: <20210401193133.18129-1-p.yadav@ti.com> <20210401193133.18129-5-p.yadav@ti.com> <20210405074032.uhuoeegby3f6zn4d@ti.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 4/5/21 3:40 AM, Pratyush Yadav wrote: > On 02/04/21 06:31PM, Sean Anderson wrote: >> On 4/1/21 3:31 PM, Pratyush Yadav wrote: >>> spi_mem_default_supports_op() rejects DTR ops by default to ensure that >>> the controller drivers that haven't been updated with DTR support >>> continue to reject them. It also makes sure that controllers that don't >>> support DTR mode at all (which is most of them at the moment) also >>> reject them. >>> >>> This means that controller drivers that want to support DTR mode can't >>> use spi_mem_default_supports_op(). Driver authors have to roll their own >>> supports_op() function and mimic the buswidth checks. See >>> spi-cadence-quadspi.c for example. Or even worse, driver authors might >>> skip it completely or get it wrong. >>> >>> Add spi_mem_dtr_supports_op(). It provides a basic sanity check for DTR >>> ops and performs the buswidth requirement check. Move the logic for >>> checking buswidth in spi_mem_default_supports_op() to a separate >>> function so the logic is not repeated twice. >>> >>> Signed-off-by: Pratyush Yadav >>> --- >>> drivers/spi/spi-mem.c | 22 +++++++++++++++++++--- >>> include/spi-mem.h | 2 ++ >>> 2 files changed, 21 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c >>> index 541cd0e5a7..be1737a2c6 100644 >>> --- a/drivers/spi/spi-mem.c >>> +++ b/drivers/spi/spi-mem.c >>> @@ -145,8 +145,8 @@ static int spi_check_buswidth_req(struct spi_slave *slave, u8 buswidth, bool tx) >>> return -ENOTSUPP; >>> } >>> -bool spi_mem_default_supports_op(struct spi_slave *slave, >>> - const struct spi_mem_op *op) >>> +static bool spi_mem_check_buswidth(struct spi_slave *slave, >>> + const struct spi_mem_op *op) >>> { >>> if (spi_check_buswidth_req(slave, op->cmd.buswidth, true)) >>> return false; >>> @@ -164,13 +164,29 @@ bool spi_mem_default_supports_op(struct spi_slave *slave, >>> op->data.dir == SPI_MEM_DATA_OUT)) >>> return false; >>> + return true; >>> +} >>> + >>> +bool spi_mem_dtr_supports_op(struct spi_slave *slave, >>> + const struct spi_mem_op *op) >>> +{ >>> + if (op->cmd.nbytes != 2) >>> + return false; >> >> Why does the command bytes need to be 2? > > They need to be 2 if the command buswidth is 8 because otherwise the > command phase will only take up half a cycle. This should have been if > (op->cmd.buswidth == 8 && op->cmd.nbytes != 2). Perhaps the most correct then is int width = cmd.dtr ? cmd.buswidth * 2 : cmd.buswidth; if (cmd.nbytes % width) { ... } >> >> --Sean >> >>> + >>> + return spi_mem_check_buswidth(slave, op); >>> +} >>> +EXPORT_SYMBOL_GPL(spi_mem_dtr_supports_op); >>> + >>> +bool spi_mem_default_supports_op(struct spi_slave *slave, >>> + const struct spi_mem_op *op) >>> +{ >>> if (op->cmd.dtr || op->addr.dtr || op->dummy.dtr || op->data.dtr) >>> return false; >>> if (op->cmd.nbytes != 1) >>> return false;> - return true; >>> + return spi_mem_check_buswidth(slave, op); >>> } >>> EXPORT_SYMBOL_GPL(spi_mem_default_supports_op); >>> diff --git a/include/spi-mem.h b/include/spi-mem.h >>> index dc53b517c1..37a9128c5b 100644 >>> --- a/include/spi-mem.h >>> +++ b/include/spi-mem.h >>> @@ -249,6 +249,8 @@ spi_controller_dma_unmap_mem_op_data(struct spi_controller *ctlr, >>> int spi_mem_adjust_op_size(struct spi_slave *slave, struct spi_mem_op *op); >>> bool spi_mem_supports_op(struct spi_slave *slave, const struct spi_mem_op *op); >>> +bool spi_mem_dtr_supports_op(struct spi_slave *slave, >>> + const struct spi_mem_op *op); >>> bool spi_mem_default_supports_op(struct spi_slave *slave, >>> const struct spi_mem_op *op); >>> >> >