Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 27 Aug 2022 00:36:29 +0300
From:   Serge Semin <fancer.lancer@gmail.com>
To:     Sudip Mukherjee <sudip.mukherjee@sifive.com>
Cc:     Serge Semin <Sergey.Semin@baikalelectronics.ru>,
        Mark Brown <broonie@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        greentime.hu@sifive.com, jude.onyenegecha@sifive.com,
        william.salmon@sifive.com, adnan.chowdhury@sifive.com,
        ben.dooks@sifive.com, linux-spi@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        jeegar.lakhani@sifive.com
Subject: Re: [PATCH 02/11] spi: dw: add check for support of dual/quad/octal
Message-ID: <20220826213629.axf7dnlwwkah7d43@mobilestation>
References: <20220802175755.6530-1-sudip.mukherjee@sifive.com>
 <20220802175755.6530-3-sudip.mukherjee@sifive.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220802175755.6530-3-sudip.mukherjee@sifive.com>
Precedence: bulk

On Tue, Aug 02, 2022 at 06:57:46PM +0100, Sudip Mukherjee wrote:
> Before doing the mem op spi controller will be queried about the
> buswidths it supports. Add the dual/quad/octal if the controller
> has the DW_SPI_CAP_EXT_SPI capability.
> 
> Signed-off-by: Sudip Mukherjee <sudip.mukherjee@sifive.com>
> ---
>  drivers/spi/spi-dw-core.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
> index 97e72da7c120..77529e359b6d 100644
> --- a/drivers/spi/spi-dw-core.c
> +++ b/drivers/spi/spi-dw-core.c
> @@ -488,8 +488,23 @@ static int dw_spi_adjust_mem_op_size(struct spi_mem *mem, struct spi_mem_op *op)
>  static bool dw_spi_supports_mem_op(struct spi_mem *mem,
>  				   const struct spi_mem_op *op)
>  {
> -	if (op->data.buswidth > 1 || op->addr.buswidth > 1 ||
> -	    op->dummy.buswidth > 1 || op->cmd.buswidth > 1)

> +	struct dw_spi *dws = spi_controller_get_devdata(mem->spi->controller);
> +
> +	/*
> +	 * Only support TT0 mode in enhanced SPI for now.
> +	 * TT0 = Instruction and Address will be sent in
> +	 * Standard SPI Mode.
> +	 */
> +	if (op->addr.buswidth > 1 || op->dummy.buswidth > 1 ||
> +	    op->cmd.buswidth > 1)
> +		return false;
> +
> +	/* In enhanced SPI 1, 2, 4, 8 all are valid modes. */
> +	if (op->data.buswidth > 1 && (!(dws->caps & DW_SPI_CAP_EXT_SPI)))
> +		return false;
> +
> +	/* Only support upto 32 bit address in enhanced SPI for now. */
> +	if (op->data.buswidth > 1 && op->addr.nbytes > 4)

Sorry, it can't be done as you suggest. First you get to update the
supported mem-ops method with no adding actual ops support. Thus the
driver will permit submitting the enhanced SPI memory operations, but
won't handle them correctly. So the driver will be left broken after
this commit. Second the dummy bytes are supported for the Rx
operations only. Thirdly you don't check the length of the cmd field,
but you need to unless you add the 2-bytes long opcode support. The
conditional statement for the address data length is incorrect. Since
you get to fix the field length to 4 bytes the address will be always
sent as 4 bytes. Fifthly you haven't fixed the
dw_spi_adjust_mem_op_size() method, which permits SPI MEM Tx-transfers
of any size. It isn't true in your case. Finally you can easily add
the cmd and address sent via the same buswidth as specified for the
data by means of the SPI_CTRL0.TRANS_TYPE field settings, thus
supporting all the possible modes.

Instead I suggest for you to do the next things in this case:
1. Create separate dw_spi_supports_enh_mem_op() and
dw_spi_adjust_enh_mem_op_size() for the case of the Enhanced SPI
modes.
2. Return false if the dummy cycle is requested for the
Tx-operation.
3. Return false if:
   (op->addr.nbytes != 0 && op->addr.buswidth != 1 && op->addr.buswidth != op->data.buswidth) ||
   (op->cmd.buswidth != 1 && op->cmd.buswidth != op->addr.buswidth && op->cmd.buswidth != op->data.buswidth)
4. Return false if:
   (op->dummy.nbytes != 0 && op->dummy.nbytes / op->dummy.buswidth > 4)
5. Make sure the dw_spi_adjust_enh_mem_op_size() method fixes both Rx
and Tx transfer lengths.

All of the above needs to be added either in the framework of the
patch with the new SPI MEM ops functionality or in a ways so the
currently unsupported operations wouldn't be available for now.

-Sergey

>  		return false;
>  
>  	return spi_mem_default_supports_op(mem, op);
> -- 
> 2.30.2
>