Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Sender: Guenter Roeck <groeck7@gmail.com>
Subject: Re: [PATCH] spi: mediatek: Fix fifo transfer
To:     Mark Brown <broonie@kernel.org>
Cc:     Matthias Brugger <matthias.bgg@gmail.com>,
        linux-spi@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-mediatek@lists.infradead.org, linux-kernel@vger.kernel.org,
        Peter Hess <peter.hess@ph-home.de>,
        Frank Wunderlich <frank-w@public-files.de>,
        Tzung-Bi Shih <tzungbi@google.com>,
        Hsin-Yi Wang <hsinyi@google.com>
References: <20210802030023.1748777-1-linux@roeck-us.net>
From:   Guenter Roeck <linux@roeck-us.net>
Message-ID: <836776f1-5b8d-ba18-2bd7-3b21673a8b82@roeck-us.net>
Date:   Mon, 2 Aug 2021 06:57:40 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.11.0
MIME-Version: 1.0
In-Reply-To: <20210802030023.1748777-1-linux@roeck-us.net>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 8/1/21 8:00 PM, Guenter Roeck wrote:
> Commit 3a70dd2d0503 ("spi: mediatek: fix fifo rx mode") claims that
> fifo RX mode was never handled, and adds the presumably missing code
> to the FIFO transfer function. However, the claim that receive data
> was not handled is incorrect. It was handled as part of interrupt
> handling after the transfer was complete. The code added with the above
> mentioned commit reads data from the receive FIFO before the transfer
> is started, which is wrong. This results in an actual transfer error
> on a Hayato Chromebook.
> 
> Remove the code trying to handle receive data before the transfer is
> started to fix the problem.
> 
> Fixes: 3a70dd2d0503 ("spi: mediatek: fix fifo rx mode")
> Cc: Peter Hess <peter.hess@ph-home.de>
> Cc: Frank Wunderlich <frank-w@public-files.de>
> Cc: Tzung-Bi Shih <tzungbi@google.com>
> Cc: Hsin-Yi Wang <hsinyi@google.com>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---

I should have added here: If this patch isn't acceptable for some reason,
commit 3a70dd2d0503 should be reverted because it is obviously wrong
and introduces a severe regression.

Thanks,
Guenter

>   drivers/spi/spi-mt65xx.c | 19 +++++--------------
>   1 file changed, 5 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/spi/spi-mt65xx.c b/drivers/spi/spi-mt65xx.c
> index 68dca8ceb3ad..7914255521c3 100644
> --- a/drivers/spi/spi-mt65xx.c
> +++ b/drivers/spi/spi-mt65xx.c
> @@ -426,24 +426,15 @@ static int mtk_spi_fifo_transfer(struct spi_master *master,
>   	mtk_spi_prepare_transfer(master, xfer);
>   	mtk_spi_setup_packet(master);
>   
> -	cnt = xfer->len / 4;
> -	if (xfer->tx_buf)
> +	if (xfer->tx_buf) {
> +		cnt = xfer->len / 4;
>   		iowrite32_rep(mdata->base + SPI_TX_DATA_REG, xfer->tx_buf, cnt);
> -
> -	if (xfer->rx_buf)
> -		ioread32_rep(mdata->base + SPI_RX_DATA_REG, xfer->rx_buf, cnt);
> -
> -	remainder = xfer->len % 4;
> -	if (remainder > 0) {
> -		reg_val = 0;
> -		if (xfer->tx_buf) {
> +		remainder = xfer->len % 4;
> +		if (remainder > 0) {
> +			reg_val = 0;
>   			memcpy(&reg_val, xfer->tx_buf + (cnt * 4), remainder);
>   			writel(reg_val, mdata->base + SPI_TX_DATA_REG);
>   		}
> -		if (xfer->rx_buf) {
> -			reg_val = readl(mdata->base + SPI_RX_DATA_REG);
> -			memcpy(xfer->rx_buf + (cnt * 4), &reg_val, remainder);
> -		}
>   	}
>   
>   	mtk_spi_enable_transfer(master);
>