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:   Wed, 19 Apr 2023 18:03:20 +0200
From:   Andi Shyti <andi.shyti@kernel.org>
To:     Jaewon Kim <jaewon02.kim@samsung.com>
Cc:     Mark Brown <broonie@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
        Andi Shyti <andi@etezian.org>,
        Alim Akhtar <alim.akhtar@samsung.com>,
        linux-spi@vger.kernel.org, linux-samsung-soc@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        Chanho Park <chanho61.park@samsung.com>
Subject: Re: [PATCH v2 4/4] spi: s3c64xx: support interrupt based pio mode
Message-ID: <20230419160320.ydgqqftsfn3y33p4@intel.intel>
References: <20230419060639.38853-1-jaewon02.kim@samsung.com>
 <CGME20230419062755epcas2p43a1127f4bb28cf1cf3f42e5d3cc597cd@epcas2p4.samsung.com>
 <20230419060639.38853-5-jaewon02.kim@samsung.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="zygm74jtlgdew5uq"
Content-Disposition: inline
In-Reply-To: <20230419060639.38853-5-jaewon02.kim@samsung.com>
Precedence: bulk


--zygm74jtlgdew5uq
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Hi Jaewon,

On Wed, Apr 19, 2023 at 03:06:39PM +0900, Jaewon Kim wrote:
> Interrupt based pio mode is supported to reduce CPU load.
> If transfer size is larger than 32 byte, it is processed using interrupt.
>=20
> Signed-off-by: Jaewon Kim <jaewon02.kim@samsung.com>
> ---
>  drivers/spi/spi-s3c64xx.c | 82 ++++++++++++++++++++++++++++++++-------
>  1 file changed, 67 insertions(+), 15 deletions(-)
>=20
> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> index cf3060b2639b..ce1afb9a4ed4 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -58,6 +58,8 @@
>  #define S3C64XX_SPI_MODE_BUS_TSZ_HALFWORD	(1<<17)
>  #define S3C64XX_SPI_MODE_BUS_TSZ_WORD		(2<<17)
>  #define S3C64XX_SPI_MODE_BUS_TSZ_MASK		(3<<17)
> +#define S3C64XX_SPI_MODE_RX_RDY_LVL		GENMASK(16, 11)
> +#define S3C64XX_SPI_MODE_RX_RDY_LVL_SHIFT	11
>  #define S3C64XX_SPI_MODE_SELF_LOOPBACK		(1<<3)
>  #define S3C64XX_SPI_MODE_RXDMA_ON		(1<<2)
>  #define S3C64XX_SPI_MODE_TXDMA_ON		(1<<1)
> @@ -114,6 +116,8 @@
> =20
>  #define S3C64XX_SPI_TRAILCNT		S3C64XX_SPI_MAX_TRAILCNT
> =20
> +#define S3C64XX_SPI_POLLING_SIZE	32
> +
>  #define msecs_to_loops(t) (loops_per_jiffy / 1000 * HZ * t)
>  #define is_polling(x)	(x->cntrlr_info->polling)
> =20
> @@ -552,10 +556,11 @@ static int s3c64xx_wait_for_dma(struct s3c64xx_spi_=
driver_data *sdd,
>  }
> =20
>  static int s3c64xx_wait_for_pio(struct s3c64xx_spi_driver_data *sdd,
> -				struct spi_transfer *xfer)
> +				struct spi_transfer *xfer, int use_irq)

bool use_irq

>  {
>  	void __iomem *regs =3D sdd->regs;
>  	unsigned long val;
> +	unsigned long time;

this time is used only in "if (use_irq)" can you move its
declaration under the if?

>  	u32 status;
>  	int loops;
>  	u32 cpy_len;
> @@ -563,17 +568,24 @@ static int s3c64xx_wait_for_pio(struct s3c64xx_spi_=
driver_data *sdd,
>  	int ms;
>  	u32 tx_time;
> =20
> -	/* sleep during signal transfer time */
> -	status =3D readl(regs + S3C64XX_SPI_STATUS);
> -	if (RX_FIFO_LVL(status, sdd) < xfer->len) {
> -		tx_time =3D (xfer->len * 8 * 1000 * 1000) / sdd->cur_speed;
> -		usleep_range(tx_time / 2, tx_time);
> -	}
> -
>  	/* millisecs to xfer 'len' bytes @ 'cur_speed' */
>  	ms =3D xfer->len * 8 * 1000 / sdd->cur_speed;
>  	ms +=3D 10; /* some tolerance */
> =20
> +	if (use_irq) {
> +		val =3D msecs_to_jiffies(ms);
> +		time =3D wait_for_completion_timeout(&sdd->xfer_completion, val);
> +		if (!time)
> +			return -EIO;
> +	} else {
> +		/* sleep during signal transfer time */
> +		status =3D readl(regs + S3C64XX_SPI_STATUS);
> +		if (RX_FIFO_LVL(status, sdd) < xfer->len) {
> +			tx_time =3D (xfer->len * 8 * 1000 * 1000) / sdd->cur_speed;
> +			usleep_range(tx_time / 2, tx_time);

yeah... just use 'ms'.

> +		}
> +	}
> +
>  	val =3D msecs_to_loops(ms);
>  	do {
>  		cpu_relax();
> @@ -737,10 +749,13 @@ static int s3c64xx_spi_transfer_one(struct spi_mast=
er *master,
>  	void *rx_buf =3D NULL;
>  	int target_len =3D 0, origin_len =3D 0;
>  	int use_dma =3D 0;
> +	int use_irq =3D 0;
>  	int status;
>  	u32 speed;
>  	u8 bpw;
>  	unsigned long flags;
> +	u32 rdy_lv;
> +	u32 val;
> =20
>  	reinit_completion(&sdd->xfer_completion);
> =20
> @@ -761,17 +776,46 @@ static int s3c64xx_spi_transfer_one(struct spi_mast=
er *master,
>  	    sdd->rx_dma.ch && sdd->tx_dma.ch) {
>  		use_dma =3D 1;
> =20
> -	} else if (xfer->len > fifo_len) {
> +	} else if (xfer->len >=3D fifo_len) {
>  		tx_buf =3D xfer->tx_buf;
>  		rx_buf =3D xfer->rx_buf;
>  		origin_len =3D xfer->len;
> -
>  		target_len =3D xfer->len;
> -		if (xfer->len > fifo_len)
> -			xfer->len =3D fifo_len;
> +		xfer->len =3D fifo_len - 1;
>  	}

Is this change related to this patch?

The rest looks good.

Andi

--zygm74jtlgdew5uq
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYIAB0WIQScDfrjQa34uOld1VLaeAVmJtMtbgUCZEAQyAAKCRDaeAVmJtMt
bs2eAQDotXXRybKKzzGuqYu7pgJoJR7ZiCFr79RgsoeNXuSlJQD+LIZqpkkESIpj
EeRs8M1GsIXlaUNp0rU4y9e1QTDS5w8=
=5+Vw
-----END PGP SIGNATURE-----

--zygm74jtlgdew5uq--