[PATCH v1] spi: spi-mtk-nor: Optimize timeout for dma read

[PATCH v1] spi: spi-mtk-nor: Optimize timeout for dma read

[Bayi Cheng]

bayi cheng (1):
  spi: spi-mtk-nor: Optimize timeout for dma read

 drivers/spi/spi-mtk-nor.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

-- 
2.25.1

[PATCH v1] spi: spi-mtk-nor: Optimize timeout for dma read

[Bayi Cheng]

bayi cheng (1):
  spi: spi-mtk-nor: Optimize timeout for dma read

 drivers/spi/spi-mtk-nor.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

-- 
2.25.1



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1] spi: spi-mtk-nor: Optimize timeout for dma read

[Bayi Cheng]

From: bayi cheng <bayi.cheng@mediatek.com>

The timeout value of the current dma read is unreasonable. For example,
If the spi flash clock is 26Mhz, It will takes about 1.3ms to read a
4KB data in spi mode. But the actual measurement exceeds 50s when a
dma read timeout is encountered.

In order to be more accurately, It is necessary to use msecs_to_jiffies,
After modification, the measured timeout value is about 130ms.

Signed-off-by: bayi cheng <bayi.cheng@mediatek.com>
---
 drivers/spi/spi-mtk-nor.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-nor.c
index d167699a1a96..3d989db80ee9 100644
--- a/drivers/spi/spi-mtk-nor.c
+++ b/drivers/spi/spi-mtk-nor.c
@@ -354,7 +354,7 @@ static int mtk_nor_dma_exec(struct mtk_nor *sp, u32 from, unsigned int length,
 			    dma_addr_t dma_addr)
 {
 	int ret = 0;
-	ulong delay;
+	ulong delay, timeout;
 	u32 reg;
 
 	writel(from, sp->base + MTK_NOR_REG_DMA_FADR);
@@ -376,15 +376,16 @@ static int mtk_nor_dma_exec(struct mtk_nor *sp, u32 from, unsigned int length,
 	mtk_nor_rmw(sp, MTK_NOR_REG_DMA_CTL, MTK_NOR_DMA_START, 0);
 
 	delay = CLK_TO_US(sp, (length + 5) * BITS_PER_BYTE);
+	timeout = (delay + 1) * 100;
 
 	if (sp->has_irq) {
 		if (!wait_for_completion_timeout(&sp->op_done,
-						 (delay + 1) * 100))
+		    msecs_to_jiffies(max_t(size_t, timeout / 1000, 10))))
 			ret = -ETIMEDOUT;
 	} else {
 		ret = readl_poll_timeout(sp->base + MTK_NOR_REG_DMA_CTL, reg,
 					 !(reg & MTK_NOR_DMA_START), delay / 3,
-					 (delay + 1) * 100);
+					 timeout);
 	}
 
 	if (ret < 0)
-- 
2.25.1

[PATCH v1] spi: spi-mtk-nor: Optimize timeout for dma read

[Bayi Cheng]

From: bayi cheng <bayi.cheng@mediatek.com>

The timeout value of the current dma read is unreasonable. For example,
If the spi flash clock is 26Mhz, It will takes about 1.3ms to read a
4KB data in spi mode. But the actual measurement exceeds 50s when a
dma read timeout is encountered.

In order to be more accurately, It is necessary to use msecs_to_jiffies,
After modification, the measured timeout value is about 130ms.

Signed-off-by: bayi cheng <bayi.cheng@mediatek.com>
---
 drivers/spi/spi-mtk-nor.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-nor.c
index d167699a1a96..3d989db80ee9 100644
--- a/drivers/spi/spi-mtk-nor.c
+++ b/drivers/spi/spi-mtk-nor.c
@@ -354,7 +354,7 @@ static int mtk_nor_dma_exec(struct mtk_nor *sp, u32 from, unsigned int length,
 			    dma_addr_t dma_addr)
 {
 	int ret = 0;
-	ulong delay;
+	ulong delay, timeout;
 	u32 reg;
 
 	writel(from, sp->base + MTK_NOR_REG_DMA_FADR);
@@ -376,15 +376,16 @@ static int mtk_nor_dma_exec(struct mtk_nor *sp, u32 from, unsigned int length,
 	mtk_nor_rmw(sp, MTK_NOR_REG_DMA_CTL, MTK_NOR_DMA_START, 0);
 
 	delay = CLK_TO_US(sp, (length + 5) * BITS_PER_BYTE);
+	timeout = (delay + 1) * 100;
 
 	if (sp->has_irq) {
 		if (!wait_for_completion_timeout(&sp->op_done,
-						 (delay + 1) * 100))
+		    msecs_to_jiffies(max_t(size_t, timeout / 1000, 10))))
 			ret = -ETIMEDOUT;
 	} else {
 		ret = readl_poll_timeout(sp->base + MTK_NOR_REG_DMA_CTL, reg,
 					 !(reg & MTK_NOR_DMA_START), delay / 3,
-					 (delay + 1) * 100);
+					 timeout);
 	}
 
 	if (ret < 0)
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1] spi: spi-mtk-nor: Optimize timeout for dma read

[Allen-KH Cheng (程冠勳)]

Hi Bayi,

Please don't send cover letters for single patches [1], Thanks.

[1] https://lore.kernel.org/all/YuJsDI8rqkHuysIT@sirena.org.uk/

BRs,
Allen

On Thu, 2022-11-03 at 13:28 +0800, Bayi Cheng wrote:
> bayi cheng (1):
>   spi: spi-mtk-nor: Optimize timeout for dma read
> 
>  drivers/spi/spi-mtk-nor.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 

Re: [PATCH v1] spi: spi-mtk-nor: Optimize timeout for dma read

[Allen-KH Cheng (程冠勳)]

Hi Bayi,

Please don't send cover letters for single patches [1], Thanks.

[1] https://lore.kernel.org/all/YuJsDI8rqkHuysIT@sirena.org.uk/

BRs,
Allen

On Thu, 2022-11-03 at 13:28 +0800, Bayi Cheng wrote:
> bayi cheng (1):
>   spi: spi-mtk-nor: Optimize timeout for dma read
> 
>  drivers/spi/spi-mtk-nor.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1] spi: spi-mtk-nor: Optimize timeout for dma read

[AngeloGioacchino Del Regno]

Il 03/11/22 06:28, Bayi Cheng ha scritto:
> From: bayi cheng <bayi.cheng@mediatek.com>
> 
> The timeout value of the current dma read is unreasonable. For example,
> If the spi flash clock is 26Mhz, It will takes about 1.3ms to read a
> 4KB data in spi mode. But the actual measurement exceeds 50s when a
> dma read timeout is encountered.
> 
> In order to be more accurately, It is necessary to use msecs_to_jiffies,
> After modification, the measured timeout value is about 130ms.
> 
> Signed-off-by: bayi cheng <bayi.cheng@mediatek.com>
> ---
>   drivers/spi/spi-mtk-nor.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-nor.c
> index d167699a1a96..3d989db80ee9 100644
> --- a/drivers/spi/spi-mtk-nor.c
> +++ b/drivers/spi/spi-mtk-nor.c
> @@ -354,7 +354,7 @@ static int mtk_nor_dma_exec(struct mtk_nor *sp, u32 from, unsigned int length,
>   			    dma_addr_t dma_addr)
>   {
>   	int ret = 0;
> -	ulong delay;
> +	ulong delay, timeout;
>   	u32 reg;
>   
>   	writel(from, sp->base + MTK_NOR_REG_DMA_FADR);
> @@ -376,15 +376,16 @@ static int mtk_nor_dma_exec(struct mtk_nor *sp, u32 from, unsigned int length,
>   	mtk_nor_rmw(sp, MTK_NOR_REG_DMA_CTL, MTK_NOR_DMA_START, 0);
>   
>   	delay = CLK_TO_US(sp, (length + 5) * BITS_PER_BYTE);
> +	timeout = (delay + 1) * 100;
>   
>   	if (sp->has_irq) {
>   		if (!wait_for_completion_timeout(&sp->op_done,
> -						 (delay + 1) * 100))
> +		    msecs_to_jiffies(max_t(size_t, timeout / 1000, 10))))

You're giving a `size_t` variable to msecs_to_jiffies(), but checking `jiffies.h`,
this function takes a `const unsigned int` param.
Please change the type to match that.

Aside from that, your `timeout` variable contains a timeout in microseconds and
this means that actually using msecs_to_jiffies() is suboptimal here.

Please use usecs_to_jiffies() instead.

Regards,
Angelo

Re: [PATCH v1] spi: spi-mtk-nor: Optimize timeout for dma read

[AngeloGioacchino Del Regno]

Il 03/11/22 06:28, Bayi Cheng ha scritto:
> From: bayi cheng <bayi.cheng@mediatek.com>
> 
> The timeout value of the current dma read is unreasonable. For example,
> If the spi flash clock is 26Mhz, It will takes about 1.3ms to read a
> 4KB data in spi mode. But the actual measurement exceeds 50s when a
> dma read timeout is encountered.
> 
> In order to be more accurately, It is necessary to use msecs_to_jiffies,
> After modification, the measured timeout value is about 130ms.
> 
> Signed-off-by: bayi cheng <bayi.cheng@mediatek.com>
> ---
>   drivers/spi/spi-mtk-nor.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-nor.c
> index d167699a1a96..3d989db80ee9 100644
> --- a/drivers/spi/spi-mtk-nor.c
> +++ b/drivers/spi/spi-mtk-nor.c
> @@ -354,7 +354,7 @@ static int mtk_nor_dma_exec(struct mtk_nor *sp, u32 from, unsigned int length,
>   			    dma_addr_t dma_addr)
>   {
>   	int ret = 0;
> -	ulong delay;
> +	ulong delay, timeout;
>   	u32 reg;
>   
>   	writel(from, sp->base + MTK_NOR_REG_DMA_FADR);
> @@ -376,15 +376,16 @@ static int mtk_nor_dma_exec(struct mtk_nor *sp, u32 from, unsigned int length,
>   	mtk_nor_rmw(sp, MTK_NOR_REG_DMA_CTL, MTK_NOR_DMA_START, 0);
>   
>   	delay = CLK_TO_US(sp, (length + 5) * BITS_PER_BYTE);
> +	timeout = (delay + 1) * 100;
>   
>   	if (sp->has_irq) {
>   		if (!wait_for_completion_timeout(&sp->op_done,
> -						 (delay + 1) * 100))
> +		    msecs_to_jiffies(max_t(size_t, timeout / 1000, 10))))

You're giving a `size_t` variable to msecs_to_jiffies(), but checking `jiffies.h`,
this function takes a `const unsigned int` param.
Please change the type to match that.

Aside from that, your `timeout` variable contains a timeout in microseconds and
this means that actually using msecs_to_jiffies() is suboptimal here.

Please use usecs_to_jiffies() instead.

Regards,
Angelo


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v1] spi: spi-mtk-nor: Optimize timeout for dma read

[David Laight]

From: AngeloGioacchino Del Regno
> Sent: 03 November 2022 09:44
> 
> Il 03/11/22 06:28, Bayi Cheng ha scritto:
> > From: bayi cheng <bayi.cheng@mediatek.com>
> >
> > The timeout value of the current dma read is unreasonable. For example,
> > If the spi flash clock is 26Mhz, It will takes about 1.3ms to read a
> > 4KB data in spi mode. But the actual measurement exceeds 50s when a
> > dma read timeout is encountered.
> >
> > In order to be more accurately, It is necessary to use msecs_to_jiffies,
> > After modification, the measured timeout value is about 130ms.
> >
> > Signed-off-by: bayi cheng <bayi.cheng@mediatek.com>
> > ---
> >   drivers/spi/spi-mtk-nor.c | 7 ++++---
> >   1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-nor.c
> > index d167699a1a96..3d989db80ee9 100644
> > --- a/drivers/spi/spi-mtk-nor.c
> > +++ b/drivers/spi/spi-mtk-nor.c
> > @@ -354,7 +354,7 @@ static int mtk_nor_dma_exec(struct mtk_nor *sp, u32 from, unsigned int length,
> >   			    dma_addr_t dma_addr)
> >   {
> >   	int ret = 0;
> > -	ulong delay;
> > +	ulong delay, timeout;
> >   	u32 reg;
> >
> >   	writel(from, sp->base + MTK_NOR_REG_DMA_FADR);
> > @@ -376,15 +376,16 @@ static int mtk_nor_dma_exec(struct mtk_nor *sp, u32 from, unsigned int length,
> >   	mtk_nor_rmw(sp, MTK_NOR_REG_DMA_CTL, MTK_NOR_DMA_START, 0);
> >
> >   	delay = CLK_TO_US(sp, (length + 5) * BITS_PER_BYTE);
> > +	timeout = (delay + 1) * 100;
> >
> >   	if (sp->has_irq) {
> >   		if (!wait_for_completion_timeout(&sp->op_done,
> > -						 (delay + 1) * 100))
> > +		    msecs_to_jiffies(max_t(size_t, timeout / 1000, 10))))
> 
> You're giving a `size_t` variable to msecs_to_jiffies(), but checking `jiffies.h`,
> this function takes a `const unsigned int` param.
> Please change the type to match that.

The type shouldn't matter at all.
What matters is the domain of the value.

Quite why you need to use max_t(size_t, ...) is another matter.
timeout is ulong so max(timeout/1000, 10ul) should be fine.

But is ulong even right?
The domain of the value is almost certainly the same on 32bit and 64bit.
So you almost certainly need u32 or u64.

	David

> 
> Aside from that, your `timeout` variable contains a timeout in microseconds and
> this means that actually using msecs_to_jiffies() is suboptimal here.
> 
> Please use usecs_to_jiffies() instead.
> 
> Regards,
> Angelo

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

RE: [PATCH v1] spi: spi-mtk-nor: Optimize timeout for dma read

[David Laight]

From: AngeloGioacchino Del Regno
> Sent: 03 November 2022 09:44
> 
> Il 03/11/22 06:28, Bayi Cheng ha scritto:
> > From: bayi cheng <bayi.cheng@mediatek.com>
> >
> > The timeout value of the current dma read is unreasonable. For example,
> > If the spi flash clock is 26Mhz, It will takes about 1.3ms to read a
> > 4KB data in spi mode. But the actual measurement exceeds 50s when a
> > dma read timeout is encountered.
> >
> > In order to be more accurately, It is necessary to use msecs_to_jiffies,
> > After modification, the measured timeout value is about 130ms.
> >
> > Signed-off-by: bayi cheng <bayi.cheng@mediatek.com>
> > ---
> >   drivers/spi/spi-mtk-nor.c | 7 ++++---
> >   1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-nor.c
> > index d167699a1a96..3d989db80ee9 100644
> > --- a/drivers/spi/spi-mtk-nor.c
> > +++ b/drivers/spi/spi-mtk-nor.c
> > @@ -354,7 +354,7 @@ static int mtk_nor_dma_exec(struct mtk_nor *sp, u32 from, unsigned int length,
> >   			    dma_addr_t dma_addr)
> >   {
> >   	int ret = 0;
> > -	ulong delay;
> > +	ulong delay, timeout;
> >   	u32 reg;
> >
> >   	writel(from, sp->base + MTK_NOR_REG_DMA_FADR);
> > @@ -376,15 +376,16 @@ static int mtk_nor_dma_exec(struct mtk_nor *sp, u32 from, unsigned int length,
> >   	mtk_nor_rmw(sp, MTK_NOR_REG_DMA_CTL, MTK_NOR_DMA_START, 0);
> >
> >   	delay = CLK_TO_US(sp, (length + 5) * BITS_PER_BYTE);
> > +	timeout = (delay + 1) * 100;
> >
> >   	if (sp->has_irq) {
> >   		if (!wait_for_completion_timeout(&sp->op_done,
> > -						 (delay + 1) * 100))
> > +		    msecs_to_jiffies(max_t(size_t, timeout / 1000, 10))))
> 
> You're giving a `size_t` variable to msecs_to_jiffies(), but checking `jiffies.h`,
> this function takes a `const unsigned int` param.
> Please change the type to match that.

The type shouldn't matter at all.
What matters is the domain of the value.

Quite why you need to use max_t(size_t, ...) is another matter.
timeout is ulong so max(timeout/1000, 10ul) should be fine.

But is ulong even right?
The domain of the value is almost certainly the same on 32bit and 64bit.
So you almost certainly need u32 or u64.

	David

> 
> Aside from that, your `timeout` variable contains a timeout in microseconds and
> this means that actually using msecs_to_jiffies() is suboptimal here.
> 
> Please use usecs_to_jiffies() instead.
> 
> Regards,
> Angelo

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1] spi: spi-mtk-nor: Optimize timeout for dma read

[Bayi Cheng (程八意)]

On Thu, 2022-11-03 at 22:35 +0000, David Laight wrote:
> From: AngeloGioacchino Del Regno
> > Sent: 03 November 2022 09:44
> > 
> > Il 03/11/22 06:28, Bayi Cheng ha scritto:
> > > From: bayi cheng <bayi.cheng@mediatek.com>
> > > 
> > > The timeout value of the current dma read is unreasonable. For
> > > example,
> > > If the spi flash clock is 26Mhz, It will takes about 1.3ms to
> > > read a
> > > 4KB data in spi mode. But the actual measurement exceeds 50s when
> > > a
> > > dma read timeout is encountered.
> > > 
> > > In order to be more accurately, It is necessary to use
> > > msecs_to_jiffies,
> > > After modification, the measured timeout value is about 130ms.
> > > 
> > > Signed-off-by: bayi cheng <bayi.cheng@mediatek.com>
> > > ---
> > >   drivers/spi/spi-mtk-nor.c | 7 ++++---
> > >   1 file changed, 4 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-
> > > nor.c
> > > index d167699a1a96..3d989db80ee9 100644
> > > --- a/drivers/spi/spi-mtk-nor.c
> > > +++ b/drivers/spi/spi-mtk-nor.c
> > > @@ -354,7 +354,7 @@ static int mtk_nor_dma_exec(struct mtk_nor
> > > *sp, u32 from, unsigned int length,
> > >   			    dma_addr_t dma_addr)
> > >   {
> > >   	int ret = 0;
> > > -	ulong delay;
> > > +	ulong delay, timeout;
> > >   	u32 reg;
> > > 
> > >   	writel(from, sp->base + MTK_NOR_REG_DMA_FADR);
> > > @@ -376,15 +376,16 @@ static int mtk_nor_dma_exec(struct mtk_nor
> > > *sp, u32 from, unsigned int length,
> > >   	mtk_nor_rmw(sp, MTK_NOR_REG_DMA_CTL, MTK_NOR_DMA_START,
> > > 0);
> > > 
> > >   	delay = CLK_TO_US(sp, (length + 5) * BITS_PER_BYTE);
> > > +	timeout = (delay + 1) * 100;
> > > 
> > >   	if (sp->has_irq) {
> > >   		if (!wait_for_completion_timeout(&sp->op_done,
> > > -						 (delay + 1) * 100))
> > > +		    msecs_to_jiffies(max_t(size_t, timeout / 1000,
> > > 10))))
> > 
> > You're giving a `size_t` variable to msecs_to_jiffies(), but
> > checking `jiffies.h`,
> > this function takes a `const unsigned int` param.
> > Please change the type to match that.
> 
> The type shouldn't matter at all.
> What matters is the domain of the value.
> 
> Quite why you need to use max_t(size_t, ...) is another matter.
> timeout is ulong so max(timeout/1000, 10ul) should be fine.
> 
> But is ulong even right?
> The domain of the value is almost certainly the same on 32bit and
> 64bit.
> So you almost certainly need u32 or u64.
> 
> 	David
> 
Hi David & Angelo

Thank you for your comments!
To sum up, I think the next version will make the following two
changes:
1, The timeout value will not exceed u32, so the type of timeout will
be changed from ulong to u32.
2, Change msecs_to_jiffies(max_t(size_t, timeout / 1000, 10)) to be
msecs_to_jiffies(max(timeout/1000, 10ul)).

If you think these changes are not enough, please let me know, Thanks！

Best Regards,
Bayi

> > 
> > Aside from that, your `timeout` variable contains a timeout in
> > microseconds and
> > this means that actually using msecs_to_jiffies() is suboptimal
> > here.
> > 
> > Please use usecs_to_jiffies() instead.
> > 
> > Regards,
> > Angelo
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes,
> MK1 1PT, UK
> Registration No: 1397386 (Wales)

Re: [PATCH v1] spi: spi-mtk-nor: Optimize timeout for dma read

[Bayi Cheng (程八意)]

On Thu, 2022-11-03 at 22:35 +0000, David Laight wrote:
> From: AngeloGioacchino Del Regno
> > Sent: 03 November 2022 09:44
> > 
> > Il 03/11/22 06:28, Bayi Cheng ha scritto:
> > > From: bayi cheng <bayi.cheng@mediatek.com>
> > > 
> > > The timeout value of the current dma read is unreasonable. For
> > > example,
> > > If the spi flash clock is 26Mhz, It will takes about 1.3ms to
> > > read a
> > > 4KB data in spi mode. But the actual measurement exceeds 50s when
> > > a
> > > dma read timeout is encountered.
> > > 
> > > In order to be more accurately, It is necessary to use
> > > msecs_to_jiffies,
> > > After modification, the measured timeout value is about 130ms.
> > > 
> > > Signed-off-by: bayi cheng <bayi.cheng@mediatek.com>
> > > ---
> > >   drivers/spi/spi-mtk-nor.c | 7 ++++---
> > >   1 file changed, 4 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-
> > > nor.c
> > > index d167699a1a96..3d989db80ee9 100644
> > > --- a/drivers/spi/spi-mtk-nor.c
> > > +++ b/drivers/spi/spi-mtk-nor.c
> > > @@ -354,7 +354,7 @@ static int mtk_nor_dma_exec(struct mtk_nor
> > > *sp, u32 from, unsigned int length,
> > >   			    dma_addr_t dma_addr)
> > >   {
> > >   	int ret = 0;
> > > -	ulong delay;
> > > +	ulong delay, timeout;
> > >   	u32 reg;
> > > 
> > >   	writel(from, sp->base + MTK_NOR_REG_DMA_FADR);
> > > @@ -376,15 +376,16 @@ static int mtk_nor_dma_exec(struct mtk_nor
> > > *sp, u32 from, unsigned int length,
> > >   	mtk_nor_rmw(sp, MTK_NOR_REG_DMA_CTL, MTK_NOR_DMA_START,
> > > 0);
> > > 
> > >   	delay = CLK_TO_US(sp, (length + 5) * BITS_PER_BYTE);
> > > +	timeout = (delay + 1) * 100;
> > > 
> > >   	if (sp->has_irq) {
> > >   		if (!wait_for_completion_timeout(&sp->op_done,
> > > -						 (delay + 1) * 100))
> > > +		    msecs_to_jiffies(max_t(size_t, timeout / 1000,
> > > 10))))
> > 
> > You're giving a `size_t` variable to msecs_to_jiffies(), but
> > checking `jiffies.h`,
> > this function takes a `const unsigned int` param.
> > Please change the type to match that.
> 
> The type shouldn't matter at all.
> What matters is the domain of the value.
> 
> Quite why you need to use max_t(size_t, ...) is another matter.
> timeout is ulong so max(timeout/1000, 10ul) should be fine.
> 
> But is ulong even right?
> The domain of the value is almost certainly the same on 32bit and
> 64bit.
> So you almost certainly need u32 or u64.
> 
> 	David
> 
Hi David & Angelo

Thank you for your comments!
To sum up, I think the next version will make the following two
changes:
1, The timeout value will not exceed u32, so the type of timeout will
be changed from ulong to u32.
2, Change msecs_to_jiffies(max_t(size_t, timeout / 1000, 10)) to be
msecs_to_jiffies(max(timeout/1000, 10ul)).

If you think these changes are not enough, please let me know, Thanks！

Best Regards,
Bayi

> > 
> > Aside from that, your `timeout` variable contains a timeout in
> > microseconds and
> > this means that actually using msecs_to_jiffies() is suboptimal
> > here.
> > 
> > Please use usecs_to_jiffies() instead.
> > 
> > Regards,
> > Angelo
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes,
> MK1 1PT, UK
> Registration No: 1397386 (Wales)
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1] spi: spi-mtk-nor: Optimize timeout for dma read

[Bayi Cheng (程八意)]

On Fri, 2022-11-04 at 07:53 +0000, Bayi Cheng (程八意) wrote:
> On Thu, 2022-11-03 at 22:35 +0000, David Laight wrote:
> > From: AngeloGioacchino Del Regno
> > > Sent: 03 November 2022 09:44
> > > 
> > > Il 03/11/22 06:28, Bayi Cheng ha scritto:
> > > > From: bayi cheng <bayi.cheng@mediatek.com>
> > > > 
> > > > The timeout value of the current dma read is unreasonable. For
> > > > example,
> > > > If the spi flash clock is 26Mhz, It will takes about 1.3ms to
> > > > read a
> > > > 4KB data in spi mode. But the actual measurement exceeds 50s
> > > > when
> > > > a
> > > > dma read timeout is encountered.
> > > > 
> > > > In order to be more accurately, It is necessary to use
> > > > msecs_to_jiffies,
> > > > After modification, the measured timeout value is about 130ms.
> > > > 
> > > > Signed-off-by: bayi cheng <bayi.cheng@mediatek.com>
> > > > ---
> > > >   drivers/spi/spi-mtk-nor.c | 7 ++++---
> > > >   1 file changed, 4 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-
> > > > nor.c
> > > > index d167699a1a96..3d989db80ee9 100644
> > > > --- a/drivers/spi/spi-mtk-nor.c
> > > > +++ b/drivers/spi/spi-mtk-nor.c
> > > > @@ -354,7 +354,7 @@ static int mtk_nor_dma_exec(struct mtk_nor
> > > > *sp, u32 from, unsigned int length,
> > > >   			    dma_addr_t dma_addr)
> > > >   {
> > > >   	int ret = 0;
> > > > -	ulong delay;
> > > > +	ulong delay, timeout;
> > > >   	u32 reg;
> > > > 
> > > >   	writel(from, sp->base + MTK_NOR_REG_DMA_FADR);
> > > > @@ -376,15 +376,16 @@ static int mtk_nor_dma_exec(struct
> > > > mtk_nor
> > > > *sp, u32 from, unsigned int length,
> > > >   	mtk_nor_rmw(sp, MTK_NOR_REG_DMA_CTL, MTK_NOR_DMA_START,
> > > > 0);
> > > > 
> > > >   	delay = CLK_TO_US(sp, (length + 5) * BITS_PER_BYTE);
> > > > +	timeout = (delay + 1) * 100;
> > > > 
> > > >   	if (sp->has_irq) {
> > > >   		if (!wait_for_completion_timeout(&sp->op_done,
> > > > -						 (delay + 1) *
> > > > 100))
> > > > +		    msecs_to_jiffies(max_t(size_t, timeout /
> > > > 1000,
> > > > 10))))
> > > 
> > > You're giving a `size_t` variable to msecs_to_jiffies(), but
> > > checking `jiffies.h`,
> > > this function takes a `const unsigned int` param.
> > > Please change the type to match that.
> > 
> > The type shouldn't matter at all.
> > What matters is the domain of the value.
> > 
> > Quite why you need to use max_t(size_t, ...) is another matter.
> > timeout is ulong so max(timeout/1000, 10ul) should be fine.
> > 
> > But is ulong even right?
> > The domain of the value is almost certainly the same on 32bit and
> > 64bit.
> > So you almost certainly need u32 or u64.
> > 
> > 	David
> > 
> 
> Hi David & Angelo
> 
> Thank you for your comments!
> To sum up, I think the next version will make the following two
> changes:
> 1, The timeout value will not exceed u32, so the type of timeout will
> be changed from ulong to u32.
> 2, Change msecs_to_jiffies(max_t(size_t, timeout / 1000, 10)) to be
> msecs_to_jiffies(max(timeout / 1000, 10U)).
> 
> If you think these changes are not enough, please let me know,
> Thanks！
> 
> Best Regards,
> Bayi
> 

Hi Angelo, Hi David,

Just a gentle ping on this.
Could you please review this patch and give us some suggestion?

PS: With your permission, I will make the following changes in the next
version:

Change in v2:
  -Change the type of "timeout" from ulong to u32.
  -Replace max_t with max.


Thanks.

BRs,
Bayi Cheng

> > > 
> > > Aside from that, your `timeout` variable contains a timeout in
> > > microseconds and
> > > this means that actually using msecs_to_jiffies() is suboptimal
> > > here.
> > > 
> > > Please use usecs_to_jiffies() instead.
> > > 
> > > Regards,
> > > Angelo
> > 
> > -
> > Registered Address Lakeside, Bramley Road, Mount Farm, Milton
> > Keynes,
> > MK1 1PT, UK
> > Registration No: 1397386 (Wales)

Re: [PATCH v1] spi: spi-mtk-nor: Optimize timeout for dma read

[Bayi Cheng (程八意)]

On Fri, 2022-11-04 at 07:53 +0000, Bayi Cheng (程八意) wrote:
> On Thu, 2022-11-03 at 22:35 +0000, David Laight wrote:
> > From: AngeloGioacchino Del Regno
> > > Sent: 03 November 2022 09:44
> > > 
> > > Il 03/11/22 06:28, Bayi Cheng ha scritto:
> > > > From: bayi cheng <bayi.cheng@mediatek.com>
> > > > 
> > > > The timeout value of the current dma read is unreasonable. For
> > > > example,
> > > > If the spi flash clock is 26Mhz, It will takes about 1.3ms to
> > > > read a
> > > > 4KB data in spi mode. But the actual measurement exceeds 50s
> > > > when
> > > > a
> > > > dma read timeout is encountered.
> > > > 
> > > > In order to be more accurately, It is necessary to use
> > > > msecs_to_jiffies,
> > > > After modification, the measured timeout value is about 130ms.
> > > > 
> > > > Signed-off-by: bayi cheng <bayi.cheng@mediatek.com>
> > > > ---
> > > >   drivers/spi/spi-mtk-nor.c | 7 ++++---
> > > >   1 file changed, 4 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-
> > > > nor.c
> > > > index d167699a1a96..3d989db80ee9 100644
> > > > --- a/drivers/spi/spi-mtk-nor.c
> > > > +++ b/drivers/spi/spi-mtk-nor.c
> > > > @@ -354,7 +354,7 @@ static int mtk_nor_dma_exec(struct mtk_nor
> > > > *sp, u32 from, unsigned int length,
> > > >   			    dma_addr_t dma_addr)
> > > >   {
> > > >   	int ret = 0;
> > > > -	ulong delay;
> > > > +	ulong delay, timeout;
> > > >   	u32 reg;
> > > > 
> > > >   	writel(from, sp->base + MTK_NOR_REG_DMA_FADR);
> > > > @@ -376,15 +376,16 @@ static int mtk_nor_dma_exec(struct
> > > > mtk_nor
> > > > *sp, u32 from, unsigned int length,
> > > >   	mtk_nor_rmw(sp, MTK_NOR_REG_DMA_CTL, MTK_NOR_DMA_START,
> > > > 0);
> > > > 
> > > >   	delay = CLK_TO_US(sp, (length + 5) * BITS_PER_BYTE);
> > > > +	timeout = (delay + 1) * 100;
> > > > 
> > > >   	if (sp->has_irq) {
> > > >   		if (!wait_for_completion_timeout(&sp->op_done,
> > > > -						 (delay + 1) *
> > > > 100))
> > > > +		    msecs_to_jiffies(max_t(size_t, timeout /
> > > > 1000,
> > > > 10))))
> > > 
> > > You're giving a `size_t` variable to msecs_to_jiffies(), but
> > > checking `jiffies.h`,
> > > this function takes a `const unsigned int` param.
> > > Please change the type to match that.
> > 
> > The type shouldn't matter at all.
> > What matters is the domain of the value.
> > 
> > Quite why you need to use max_t(size_t, ...) is another matter.
> > timeout is ulong so max(timeout/1000, 10ul) should be fine.
> > 
> > But is ulong even right?
> > The domain of the value is almost certainly the same on 32bit and
> > 64bit.
> > So you almost certainly need u32 or u64.
> > 
> > 	David
> > 
> 
> Hi David & Angelo
> 
> Thank you for your comments!
> To sum up, I think the next version will make the following two
> changes:
> 1, The timeout value will not exceed u32, so the type of timeout will
> be changed from ulong to u32.
> 2, Change msecs_to_jiffies(max_t(size_t, timeout / 1000, 10)) to be
> msecs_to_jiffies(max(timeout / 1000, 10U)).
> 
> If you think these changes are not enough, please let me know,
> Thanks！
> 
> Best Regards,
> Bayi
> 

Hi Angelo, Hi David,

Just a gentle ping on this.
Could you please review this patch and give us some suggestion?

PS: With your permission, I will make the following changes in the next
version:

Change in v2:
  -Change the type of "timeout" from ulong to u32.
  -Replace max_t with max.


Thanks.

BRs,
Bayi Cheng

> > > 
> > > Aside from that, your `timeout` variable contains a timeout in
> > > microseconds and
> > > this means that actually using msecs_to_jiffies() is suboptimal
> > > here.
> > > 
> > > Please use usecs_to_jiffies() instead.
> > > 
> > > Regards,
> > > Angelo
> > 
> > -
> > Registered Address Lakeside, Bramley Road, Mount Farm, Milton
> > Keynes,
> > MK1 1PT, UK
> > Registration No: 1397386 (Wales)
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1] spi: spi-mtk-nor: Optimize timeout for dma read

[AngeloGioacchino Del Regno]

Il 11/11/22 05:16, Bayi Cheng (程八意) ha scritto:
> On Fri, 2022-11-04 at 07:53 +0000, Bayi Cheng (程八意) wrote:
>> On Thu, 2022-11-03 at 22:35 +0000, David Laight wrote:
>>> From: AngeloGioacchino Del Regno
>>>> Sent: 03 November 2022 09:44
>>>>
>>>> Il 03/11/22 06:28, Bayi Cheng ha scritto:
>>>>> From: bayi cheng <bayi.cheng@mediatek.com>
>>>>>
>>>>> The timeout value of the current dma read is unreasonable. For
>>>>> example,
>>>>> If the spi flash clock is 26Mhz, It will takes about 1.3ms to
>>>>> read a
>>>>> 4KB data in spi mode. But the actual measurement exceeds 50s
>>>>> when
>>>>> a
>>>>> dma read timeout is encountered.
>>>>>
>>>>> In order to be more accurately, It is necessary to use
>>>>> msecs_to_jiffies,
>>>>> After modification, the measured timeout value is about 130ms.
>>>>>
>>>>> Signed-off-by: bayi cheng <bayi.cheng@mediatek.com>
>>>>> ---
>>>>>    drivers/spi/spi-mtk-nor.c | 7 ++++---
>>>>>    1 file changed, 4 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-
>>>>> nor.c
>>>>> index d167699a1a96..3d989db80ee9 100644
>>>>> --- a/drivers/spi/spi-mtk-nor.c
>>>>> +++ b/drivers/spi/spi-mtk-nor.c
>>>>> @@ -354,7 +354,7 @@ static int mtk_nor_dma_exec(struct mtk_nor
>>>>> *sp, u32 from, unsigned int length,
>>>>>    			    dma_addr_t dma_addr)
>>>>>    {
>>>>>    	int ret = 0;
>>>>> -	ulong delay;
>>>>> +	ulong delay, timeout;
>>>>>    	u32 reg;
>>>>>
>>>>>    	writel(from, sp->base + MTK_NOR_REG_DMA_FADR);
>>>>> @@ -376,15 +376,16 @@ static int mtk_nor_dma_exec(struct
>>>>> mtk_nor
>>>>> *sp, u32 from, unsigned int length,
>>>>>    	mtk_nor_rmw(sp, MTK_NOR_REG_DMA_CTL, MTK_NOR_DMA_START,
>>>>> 0);
>>>>>
>>>>>    	delay = CLK_TO_US(sp, (length + 5) * BITS_PER_BYTE);
>>>>> +	timeout = (delay + 1) * 100;
>>>>>
>>>>>    	if (sp->has_irq) {
>>>>>    		if (!wait_for_completion_timeout(&sp->op_done,
>>>>> -						 (delay + 1) *
>>>>> 100))
>>>>> +		    msecs_to_jiffies(max_t(size_t, timeout /
>>>>> 1000,
>>>>> 10))))
>>>>
>>>> You're giving a `size_t` variable to msecs_to_jiffies(), but
>>>> checking `jiffies.h`,
>>>> this function takes a `const unsigned int` param.
>>>> Please change the type to match that.
>>>
>>> The type shouldn't matter at all.
>>> What matters is the domain of the value.
>>>
>>> Quite why you need to use max_t(size_t, ...) is another matter.
>>> timeout is ulong so max(timeout/1000, 10ul) should be fine.
>>>
>>> But is ulong even right?
>>> The domain of the value is almost certainly the same on 32bit and
>>> 64bit.
>>> So you almost certainly need u32 or u64.
>>>
>>> 	David
>>>
>>
>> Hi David & Angelo
>>
>> Thank you for your comments!
>> To sum up, I think the next version will make the following two
>> changes:
>> 1, The timeout value will not exceed u32, so the type of timeout will
>> be changed from ulong to u32.
>> 2, Change msecs_to_jiffies(max_t(size_t, timeout / 1000, 10)) to be
>> msecs_to_jiffies(max(timeout / 1000, 10U)).
>>
>> If you think these changes are not enough, please let me know,
>> Thanks！
>>
>> Best Regards,
>> Bayi
>>
> 
> Hi Angelo, Hi David,
> 
> Just a gentle ping on this.
> Could you please review this patch and give us some suggestion?
> 
> PS: With your permission, I will make the following changes in the next
> version:
> 
> Change in v2:
>    -Change the type of "timeout" from ulong to u32.
>    -Replace max_t with max.
> 

I still recommend to use usecs_to_jiffies() when appropriate, instead of
converting usecs to msecs and using msecs_to_jiffies().

As for the rest in your list: yes, please.

Regards,
Angelo

> 
> Thanks.
> 
> BRs,
> Bayi Cheng
> 
>>>>
>>>> Aside from that, your `timeout` variable contains a timeout in
>>>> microseconds and
>>>> this means that actually using msecs_to_jiffies() is suboptimal
>>>> here.
>>>>
>>>> Please use usecs_to_jiffies() instead.
>>>>
>>>> Regards,
>>>> Angelo
>>>
>>> -
>>> Registered Address Lakeside, Bramley Road, Mount Farm, Milton
>>> Keynes,
>>> MK1 1PT, UK
>>> Registration No: 1397386 (Wales)

-- 
AngeloGioacchino Del Regno
Software Engineer

Collabora Ltd.
Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK
Registered in England & Wales, no. 5513718

Re: [PATCH v1] spi: spi-mtk-nor: Optimize timeout for dma read

[AngeloGioacchino Del Regno]

Il 11/11/22 05:16, Bayi Cheng (程八意) ha scritto:
> On Fri, 2022-11-04 at 07:53 +0000, Bayi Cheng (程八意) wrote:
>> On Thu, 2022-11-03 at 22:35 +0000, David Laight wrote:
>>> From: AngeloGioacchino Del Regno
>>>> Sent: 03 November 2022 09:44
>>>>
>>>> Il 03/11/22 06:28, Bayi Cheng ha scritto:
>>>>> From: bayi cheng <bayi.cheng@mediatek.com>
>>>>>
>>>>> The timeout value of the current dma read is unreasonable. For
>>>>> example,
>>>>> If the spi flash clock is 26Mhz, It will takes about 1.3ms to
>>>>> read a
>>>>> 4KB data in spi mode. But the actual measurement exceeds 50s
>>>>> when
>>>>> a
>>>>> dma read timeout is encountered.
>>>>>
>>>>> In order to be more accurately, It is necessary to use
>>>>> msecs_to_jiffies,
>>>>> After modification, the measured timeout value is about 130ms.
>>>>>
>>>>> Signed-off-by: bayi cheng <bayi.cheng@mediatek.com>
>>>>> ---
>>>>>    drivers/spi/spi-mtk-nor.c | 7 ++++---
>>>>>    1 file changed, 4 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-
>>>>> nor.c
>>>>> index d167699a1a96..3d989db80ee9 100644
>>>>> --- a/drivers/spi/spi-mtk-nor.c
>>>>> +++ b/drivers/spi/spi-mtk-nor.c
>>>>> @@ -354,7 +354,7 @@ static int mtk_nor_dma_exec(struct mtk_nor
>>>>> *sp, u32 from, unsigned int length,
>>>>>    			    dma_addr_t dma_addr)
>>>>>    {
>>>>>    	int ret = 0;
>>>>> -	ulong delay;
>>>>> +	ulong delay, timeout;
>>>>>    	u32 reg;
>>>>>
>>>>>    	writel(from, sp->base + MTK_NOR_REG_DMA_FADR);
>>>>> @@ -376,15 +376,16 @@ static int mtk_nor_dma_exec(struct
>>>>> mtk_nor
>>>>> *sp, u32 from, unsigned int length,
>>>>>    	mtk_nor_rmw(sp, MTK_NOR_REG_DMA_CTL, MTK_NOR_DMA_START,
>>>>> 0);
>>>>>
>>>>>    	delay = CLK_TO_US(sp, (length + 5) * BITS_PER_BYTE);
>>>>> +	timeout = (delay + 1) * 100;
>>>>>
>>>>>    	if (sp->has_irq) {
>>>>>    		if (!wait_for_completion_timeout(&sp->op_done,
>>>>> -						 (delay + 1) *
>>>>> 100))
>>>>> +		    msecs_to_jiffies(max_t(size_t, timeout /
>>>>> 1000,
>>>>> 10))))
>>>>
>>>> You're giving a `size_t` variable to msecs_to_jiffies(), but
>>>> checking `jiffies.h`,
>>>> this function takes a `const unsigned int` param.
>>>> Please change the type to match that.
>>>
>>> The type shouldn't matter at all.
>>> What matters is the domain of the value.
>>>
>>> Quite why you need to use max_t(size_t, ...) is another matter.
>>> timeout is ulong so max(timeout/1000, 10ul) should be fine.
>>>
>>> But is ulong even right?
>>> The domain of the value is almost certainly the same on 32bit and
>>> 64bit.
>>> So you almost certainly need u32 or u64.
>>>
>>> 	David
>>>
>>
>> Hi David & Angelo
>>
>> Thank you for your comments!
>> To sum up, I think the next version will make the following two
>> changes:
>> 1, The timeout value will not exceed u32, so the type of timeout will
>> be changed from ulong to u32.
>> 2, Change msecs_to_jiffies(max_t(size_t, timeout / 1000, 10)) to be
>> msecs_to_jiffies(max(timeout / 1000, 10U)).
>>
>> If you think these changes are not enough, please let me know,
>> Thanks！
>>
>> Best Regards,
>> Bayi
>>
> 
> Hi Angelo, Hi David,
> 
> Just a gentle ping on this.
> Could you please review this patch and give us some suggestion?
> 
> PS: With your permission, I will make the following changes in the next
> version:
> 
> Change in v2:
>    -Change the type of "timeout" from ulong to u32.
>    -Replace max_t with max.
> 

I still recommend to use usecs_to_jiffies() when appropriate, instead of
converting usecs to msecs and using msecs_to_jiffies().

As for the rest in your list: yes, please.

Regards,
Angelo

> 
> Thanks.
> 
> BRs,
> Bayi Cheng
> 
>>>>
>>>> Aside from that, your `timeout` variable contains a timeout in
>>>> microseconds and
>>>> this means that actually using msecs_to_jiffies() is suboptimal
>>>> here.
>>>>
>>>> Please use usecs_to_jiffies() instead.
>>>>
>>>> Regards,
>>>> Angelo
>>>
>>> -
>>> Registered Address Lakeside, Bramley Road, Mount Farm, Milton
>>> Keynes,
>>> MK1 1PT, UK
>>> Registration No: 1397386 (Wales)

-- 
AngeloGioacchino Del Regno
Software Engineer

Collabora Ltd.
Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK
Registered in England & Wales, no. 5513718


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1] spi: spi-mtk-nor: Optimize timeout for dma read

[Bayi Cheng (程八意)]

On Fri, 2022-11-11 at 10:16 +0100, AngeloGioacchino Del Regno wrote:
> Il 11/11/22 05:16, Bayi Cheng (程八意) ha scritto:
> > On Fri, 2022-11-04 at 07:53 +0000, Bayi Cheng (程八意) wrote:
> > > On Thu, 2022-11-03 at 22:35 +0000, David Laight wrote:
> > > > From: AngeloGioacchino Del Regno
> > > > > Sent: 03 November 2022 09:44
> > > > > 
> > > > > Il 03/11/22 06:28, Bayi Cheng ha scritto:
> > > > > > From: bayi cheng <bayi.cheng@mediatek.com>
> > > > > > 
> > > > > > The timeout value of the current dma read is unreasonable.
> > > > > > For
> > > > > > example,
> > > > > > If the spi flash clock is 26Mhz, It will takes about 1.3ms
> > > > > > to
> > > > > > read a
> > > > > > 4KB data in spi mode. But the actual measurement exceeds
> > > > > > 50s
> > > > > > when
> > > > > > a
> > > > > > dma read timeout is encountered.
> > > > > > 
> > > > > > In order to be more accurately, It is necessary to use
> > > > > > msecs_to_jiffies,
> > > > > > After modification, the measured timeout value is about
> > > > > > 130ms.
> > > > > > 
> > > > > > Signed-off-by: bayi cheng <bayi.cheng@mediatek.com>
> > > > > > ---
> > > > > >    drivers/spi/spi-mtk-nor.c | 7 ++++---
> > > > > >    1 file changed, 4 insertions(+), 3 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-
> > > > > > mtk-
> > > > > > nor.c
> > > > > > index d167699a1a96..3d989db80ee9 100644
> > > > > > --- a/drivers/spi/spi-mtk-nor.c
> > > > > > +++ b/drivers/spi/spi-mtk-nor.c
> > > > > > @@ -354,7 +354,7 @@ static int mtk_nor_dma_exec(struct
> > > > > > mtk_nor
> > > > > > *sp, u32 from, unsigned int length,
> > > > > >    			    dma_addr_t dma_addr)
> > > > > >    {
> > > > > >    	int ret = 0;
> > > > > > -	ulong delay;
> > > > > > +	ulong delay, timeout;
> > > > > >    	u32 reg;
> > > > > > 
> > > > > >    	writel(from, sp->base + MTK_NOR_REG_DMA_FADR);
> > > > > > @@ -376,15 +376,16 @@ static int mtk_nor_dma_exec(struct
> > > > > > mtk_nor
> > > > > > *sp, u32 from, unsigned int length,
> > > > > >    	mtk_nor_rmw(sp, MTK_NOR_REG_DMA_CTL, MTK_NOR_DMA_START,
> > > > > > 0);
> > > > > > 
> > > > > >    	delay = CLK_TO_US(sp, (length + 5) * BITS_PER_BYTE);
> > > > > > +	timeout = (delay + 1) * 100;
> > > > > > 
> > > > > >    	if (sp->has_irq) {
> > > > > >    		if (!wait_for_completion_timeout(&sp->op_done,
> > > > > > -						 (delay + 1) *
> > > > > > 100))
> > > > > > +		    msecs_to_jiffies(max_t(size_t, timeout /
> > > > > > 1000,
> > > > > > 10))))
> > > > > 
> > > > > You're giving a `size_t` variable to msecs_to_jiffies(), but
> > > > > checking `jiffies.h`,
> > > > > this function takes a `const unsigned int` param.
> > > > > Please change the type to match that.
> > > > 
> > > > The type shouldn't matter at all.
> > > > What matters is the domain of the value.
> > > > 
> > > > Quite why you need to use max_t(size_t, ...) is another matter.
> > > > timeout is ulong so max(timeout/1000, 10ul) should be fine.
> > > > 
> > > > But is ulong even right?
> > > > The domain of the value is almost certainly the same on 32bit
> > > > and
> > > > 64bit.
> > > > So you almost certainly need u32 or u64.
> > > > 
> > > > 	David
> > > > 
> > > 
> > > Hi David & Angelo
> > > 
> > > Thank you for your comments!
> > > To sum up, I think the next version will make the following two
> > > changes:
> > > 1, The timeout value will not exceed u32, so the type of timeout
> > > will
> > > be changed from ulong to u32.
> > > 2, Change msecs_to_jiffies(max_t(size_t, timeout / 1000, 10)) to
> > > be
> > > msecs_to_jiffies(max(timeout / 1000, 10U)).
> > > 
> > > If you think these changes are not enough, please let me know,
> > > Thanks！
> > > 
> > > Best Regards,
> > > Bayi
> > > 
> > 
> > Hi Angelo, Hi David,
> > 
> > Just a gentle ping on this.
> > Could you please review this patch and give us some suggestion?
> > 
> > PS: With your permission, I will make the following changes in the
> > next
> > version:
> > 
> > Change in v2:
> >    -Change the type of "timeout" from ulong to u32.
> >    -Replace max_t with max.
> > 
> 
> I still recommend to use usecs_to_jiffies() when appropriate, instead
> of
> converting usecs to msecs and using msecs_to_jiffies().
> 
> As for the rest in your list: yes, please.
> 
> Regards,
> Angelo
> 

Hi Angelo,

Thanks for your comments! 
I will change it to be usecs_to_jiffies(max(timeout, 10000U)) in the
next patch.

BRs,
Bayi Cheng


> > 
> > Thanks.
> > 
> > BRs,
> > Bayi Cheng
> > 
> > > > > 
> > > > > Aside from that, your `timeout` variable contains a timeout
> > > > > in
> > > > > microseconds and
> > > > > this means that actually using msecs_to_jiffies() is
> > > > > suboptimal
> > > > > here.
> > > > > 
> > > > > Please use usecs_to_jiffies() instead.
> > > > > 
> > > > > Regards,
> > > > > Angelo
> > > > 
> > > > -
> > > > Registered Address Lakeside, Bramley Road, Mount Farm, Milton
> > > > Keynes,
> > > > MK1 1PT, UK
> > > > Registration No: 1397386 (Wales)
> 
> 

Re: [PATCH v1] spi: spi-mtk-nor: Optimize timeout for dma read

[Bayi Cheng (程八意)]

On Fri, 2022-11-11 at 10:16 +0100, AngeloGioacchino Del Regno wrote:
> Il 11/11/22 05:16, Bayi Cheng (程八意) ha scritto:
> > On Fri, 2022-11-04 at 07:53 +0000, Bayi Cheng (程八意) wrote:
> > > On Thu, 2022-11-03 at 22:35 +0000, David Laight wrote:
> > > > From: AngeloGioacchino Del Regno
> > > > > Sent: 03 November 2022 09:44
> > > > > 
> > > > > Il 03/11/22 06:28, Bayi Cheng ha scritto:
> > > > > > From: bayi cheng <bayi.cheng@mediatek.com>
> > > > > > 
> > > > > > The timeout value of the current dma read is unreasonable.
> > > > > > For
> > > > > > example,
> > > > > > If the spi flash clock is 26Mhz, It will takes about 1.3ms
> > > > > > to
> > > > > > read a
> > > > > > 4KB data in spi mode. But the actual measurement exceeds
> > > > > > 50s
> > > > > > when
> > > > > > a
> > > > > > dma read timeout is encountered.
> > > > > > 
> > > > > > In order to be more accurately, It is necessary to use
> > > > > > msecs_to_jiffies,
> > > > > > After modification, the measured timeout value is about
> > > > > > 130ms.
> > > > > > 
> > > > > > Signed-off-by: bayi cheng <bayi.cheng@mediatek.com>
> > > > > > ---
> > > > > >    drivers/spi/spi-mtk-nor.c | 7 ++++---
> > > > > >    1 file changed, 4 insertions(+), 3 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-
> > > > > > mtk-
> > > > > > nor.c
> > > > > > index d167699a1a96..3d989db80ee9 100644
> > > > > > --- a/drivers/spi/spi-mtk-nor.c
> > > > > > +++ b/drivers/spi/spi-mtk-nor.c
> > > > > > @@ -354,7 +354,7 @@ static int mtk_nor_dma_exec(struct
> > > > > > mtk_nor
> > > > > > *sp, u32 from, unsigned int length,
> > > > > >    			    dma_addr_t dma_addr)
> > > > > >    {
> > > > > >    	int ret = 0;
> > > > > > -	ulong delay;
> > > > > > +	ulong delay, timeout;
> > > > > >    	u32 reg;
> > > > > > 
> > > > > >    	writel(from, sp->base + MTK_NOR_REG_DMA_FADR);
> > > > > > @@ -376,15 +376,16 @@ static int mtk_nor_dma_exec(struct
> > > > > > mtk_nor
> > > > > > *sp, u32 from, unsigned int length,
> > > > > >    	mtk_nor_rmw(sp, MTK_NOR_REG_DMA_CTL, MTK_NOR_DMA_START,
> > > > > > 0);
> > > > > > 
> > > > > >    	delay = CLK_TO_US(sp, (length + 5) * BITS_PER_BYTE);
> > > > > > +	timeout = (delay + 1) * 100;
> > > > > > 
> > > > > >    	if (sp->has_irq) {
> > > > > >    		if (!wait_for_completion_timeout(&sp->op_done,
> > > > > > -						 (delay + 1) *
> > > > > > 100))
> > > > > > +		    msecs_to_jiffies(max_t(size_t, timeout /
> > > > > > 1000,
> > > > > > 10))))
> > > > > 
> > > > > You're giving a `size_t` variable to msecs_to_jiffies(), but
> > > > > checking `jiffies.h`,
> > > > > this function takes a `const unsigned int` param.
> > > > > Please change the type to match that.
> > > > 
> > > > The type shouldn't matter at all.
> > > > What matters is the domain of the value.
> > > > 
> > > > Quite why you need to use max_t(size_t, ...) is another matter.
> > > > timeout is ulong so max(timeout/1000, 10ul) should be fine.
> > > > 
> > > > But is ulong even right?
> > > > The domain of the value is almost certainly the same on 32bit
> > > > and
> > > > 64bit.
> > > > So you almost certainly need u32 or u64.
> > > > 
> > > > 	David
> > > > 
> > > 
> > > Hi David & Angelo
> > > 
> > > Thank you for your comments!
> > > To sum up, I think the next version will make the following two
> > > changes:
> > > 1, The timeout value will not exceed u32, so the type of timeout
> > > will
> > > be changed from ulong to u32.
> > > 2, Change msecs_to_jiffies(max_t(size_t, timeout / 1000, 10)) to
> > > be
> > > msecs_to_jiffies(max(timeout / 1000, 10U)).
> > > 
> > > If you think these changes are not enough, please let me know,
> > > Thanks！
> > > 
> > > Best Regards,
> > > Bayi
> > > 
> > 
> > Hi Angelo, Hi David,
> > 
> > Just a gentle ping on this.
> > Could you please review this patch and give us some suggestion?
> > 
> > PS: With your permission, I will make the following changes in the
> > next
> > version:
> > 
> > Change in v2:
> >    -Change the type of "timeout" from ulong to u32.
> >    -Replace max_t with max.
> > 
> 
> I still recommend to use usecs_to_jiffies() when appropriate, instead
> of
> converting usecs to msecs and using msecs_to_jiffies().
> 
> As for the rest in your list: yes, please.
> 
> Regards,
> Angelo
> 

Hi Angelo,

Thanks for your comments! 
I will change it to be usecs_to_jiffies(max(timeout, 10000U)) in the
next patch.

BRs,
Bayi Cheng


> > 
> > Thanks.
> > 
> > BRs,
> > Bayi Cheng
> > 
> > > > > 
> > > > > Aside from that, your `timeout` variable contains a timeout
> > > > > in
> > > > > microseconds and
> > > > > this means that actually using msecs_to_jiffies() is
> > > > > suboptimal
> > > > > here.
> > > > > 
> > > > > Please use usecs_to_jiffies() instead.
> > > > > 
> > > > > Regards,
> > > > > Angelo
> > > > 
> > > > -
> > > > Registered Address Lakeside, Bramley Road, Mount Farm, Milton
> > > > Keynes,
> > > > MK1 1PT, UK
> > > > Registration No: 1397386 (Wales)
> 
> 
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel