| Message ID | 20210210204613.49560-1-eajames@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [linux,dev-5.8] fsi: aspeed: Set poll timeout based on clock divider | expand |
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 >
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);
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(-)