[PATCH linux dev-5.8] fsi: aspeed: Set poll timeout based on clock divider

[PATCH linux dev-5.8] fsi: aspeed: Set poll timeout based on clock divider

[Eddie James]

The timeout for polling for transfer acknowledgment on the OPB
was very long, occasionally resulting in scheduling problems.
Instead, use a timeout based on the clock divider specified with
the module parameter. In benchmarking, the worst case poll times
didn't increase significantly with increased divider until it
reached 16 and higher. The average poll time increased linearly
with the divider.

div  1: max:150us avg: 2us
div  2: max:155us avg: 3us
div  4: max:149us avg: 7us
div  8: max:153us avg:13us
div 16: max:197us avg:21us
div 32: max:181us avg:50us
div 64: max:262us avg:100us

Signed-off-by: Eddie James <eajames@linux.ibm.com>
---
 drivers/fsi/fsi-master-aspeed.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/fsi/fsi-master-aspeed.c b/drivers/fsi/fsi-master-aspeed.c
index c71d7e9a32b0..13f7e07beacc 100644
--- a/drivers/fsi/fsi-master-aspeed.c
+++ b/drivers/fsi/fsi-master-aspeed.c
@@ -25,6 +25,7 @@ struct fsi_master_aspeed {
 	void __iomem		*base;
 	struct clk		*clk;
 	struct gpio_desc	*cfam_reset_gpio;
+	int			timeout_us;
 };
 
 #define to_fsi_master_aspeed(m) \
@@ -92,8 +93,6 @@ static const u32 fsi_base = 0xa0000000;
 static u16 aspeed_fsi_divisor = FSI_DIVISOR_DEFAULT;
 module_param_named(bus_div,aspeed_fsi_divisor, ushort, 0);
 
-#define OPB_POLL_TIMEOUT		10000
-
 static int __opb_write(struct fsi_master_aspeed *aspeed, u32 addr,
 		       u32 val, u32 transfer_size)
 {
@@ -110,7 +109,7 @@ static int __opb_write(struct fsi_master_aspeed *aspeed, u32 addr,
 
 	ret = readl_poll_timeout(base + OPB_IRQ_STATUS, reg,
 				(reg & OPB0_XFER_ACK_EN) != 0,
-				0, OPB_POLL_TIMEOUT);
+				0, aspeed->timeout_us);
 
 	status = readl(base + OPB0_STATUS);
 
@@ -157,7 +156,7 @@ static int __opb_read(struct fsi_master_aspeed *aspeed, uint32_t addr,
 
 	ret = readl_poll_timeout(base + OPB_IRQ_STATUS, reg,
 			   (reg & OPB0_XFER_ACK_EN) != 0,
-			   0, OPB_POLL_TIMEOUT);
+			   0, aspeed->timeout_us);
 
 	status = readl(base + OPB0_STATUS);
 
@@ -609,6 +608,7 @@ static int fsi_master_aspeed_probe(struct platform_device *pdev)
 	dev_set_drvdata(&pdev->dev, aspeed);
 
 	mutex_init(&aspeed->lock);
+	aspeed->timeout_us = min(10000, max(1, aspeed_fsi_divisor / 8) * 300);
 	aspeed_master_init(aspeed);
 
 	rc = fsi_master_register(&aspeed->master);
-- 
2.27.0

Re: [PATCH linux dev-5.8] fsi: aspeed: Set poll timeout based on clock divider

[Joel Stanley]

On Wed, 10 Feb 2021 at 20:46, Eddie James <eajames@linux.ibm.com> wrote:
>
> The timeout for polling for transfer acknowledgment on the OPB
> was very long, occasionally resulting in scheduling problems.
> Instead, use a timeout based on the clock divider specified with
> the module parameter. In benchmarking, the worst case poll times
> didn't increase significantly with increased divider until it
> reached 16 and higher. The average poll time increased linearly
> with the divider.
>
> div  1: max:150us avg: 2us
> div  2: max:155us avg: 3us
> div  4: max:149us avg: 7us
> div  8: max:153us avg:13us
> div 16: max:197us avg:21us
> div 32: max:181us avg:50us
> div 64: max:262us avg:100us

Nice testing. How did you create these numbers?

As this is an upper bound (normally the operation will complete and
the opb_read/write will return within seconds), I think we should set
the timeout to a constant. Choose something sensible (300us? 500?).
There's no need for it to scale, as in most cases the loop will go
around a few times and exit.

Please mention in your commit message the backtrace you saw that
motivated this patch. I'm not completely sure this fixes the root
cause, but it's a good improvement nonetheless.

Finally, submit your next version on the upstream lists. I can apply
it to the openbmc tree at the same time as the fsi tree.

Cheers,

Joel

>
> Signed-off-by: Eddie James <eajames@linux.ibm.com>
> ---
>  drivers/fsi/fsi-master-aspeed.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/fsi/fsi-master-aspeed.c b/drivers/fsi/fsi-master-aspeed.c
> index c71d7e9a32b0..13f7e07beacc 100644
> --- a/drivers/fsi/fsi-master-aspeed.c
> +++ b/drivers/fsi/fsi-master-aspeed.c
> @@ -25,6 +25,7 @@ struct fsi_master_aspeed {
>         void __iomem            *base;
>         struct clk              *clk;
>         struct gpio_desc        *cfam_reset_gpio;
> +       int                     timeout_us;
>  };
>
>  #define to_fsi_master_aspeed(m) \
> @@ -92,8 +93,6 @@ static const u32 fsi_base = 0xa0000000;
>  static u16 aspeed_fsi_divisor = FSI_DIVISOR_DEFAULT;
>  module_param_named(bus_div,aspeed_fsi_divisor, ushort, 0);
>
> -#define OPB_POLL_TIMEOUT               10000
> -
>  static int __opb_write(struct fsi_master_aspeed *aspeed, u32 addr,
>                        u32 val, u32 transfer_size)
>  {
> @@ -110,7 +109,7 @@ static int __opb_write(struct fsi_master_aspeed *aspeed, u32 addr,
>
>         ret = readl_poll_timeout(base + OPB_IRQ_STATUS, reg,
>                                 (reg & OPB0_XFER_ACK_EN) != 0,
> -                               0, OPB_POLL_TIMEOUT);
> +                               0, aspeed->timeout_us);
>
>         status = readl(base + OPB0_STATUS);
>
> @@ -157,7 +156,7 @@ static int __opb_read(struct fsi_master_aspeed *aspeed, uint32_t addr,
>
>         ret = readl_poll_timeout(base + OPB_IRQ_STATUS, reg,
>                            (reg & OPB0_XFER_ACK_EN) != 0,
> -                          0, OPB_POLL_TIMEOUT);
> +                          0, aspeed->timeout_us);
>
>         status = readl(base + OPB0_STATUS);
>
> @@ -609,6 +608,7 @@ static int fsi_master_aspeed_probe(struct platform_device *pdev)
>         dev_set_drvdata(&pdev->dev, aspeed);
>
>         mutex_init(&aspeed->lock);
> +       aspeed->timeout_us = min(10000, max(1, aspeed_fsi_divisor / 8) * 300);
>         aspeed_master_init(aspeed);
>
>         rc = fsi_master_register(&aspeed->master);
> --
> 2.27.0
>