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

[Joel Stanley <joel@jms.id.au>]

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
>