[PATCH linux dev-4.13 4/6] fsi/fsi-master-gpio: Add "no-gpio-delays" option

[Andrew Jeffery]

On Wed, 9 May 2018, at 05:24, Christopher Bostic wrote:
> Reviewed-by: Christopher Bostic <cbostic at linux.vnet.ibm.com>
> 
> 
> On 5/7/18 8:06 PM, Benjamin Herrenschmidt wrote:
> > This adds support for an optional device-tree property that
> > makes the driver skip all the delays around clocking the
> > GPIOs and set it in the device-tree of common POWER9 based
> > OpenPower platforms.
> >
> > This useful on chips like the AST2500 where the GPIO block is
> > running at a fairly low clock frequency (25Mhz typically). In
> > this case, the delays are unnecessary and due to the low
> > precision of the timers, actually quite harmful in terms of
> > performance.
> >
> > Signed-off-by: Benjamin Herrenschmidt <benh at kernel.crashing.org>
> > ---
> >   arch/arm/boot/dts/aspeed-bmc-opp-romulus.dts  |  1 +
> >   .../boot/dts/aspeed-bmc-opp-witherspoon.dts   |  1 +
> >   arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts    |  1 +
> >   drivers/fsi/fsi-master-gpio.c                 | 20 +++++++++++++++----
> >   4 files changed, 19 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-romulus.dts b/arch/arm/boot/dts/aspeed-bmc-opp-romulus.dts
> > index d746e562e8fd..0fdf08077ed3 100644
> > --- a/arch/arm/boot/dts/aspeed-bmc-opp-romulus.dts
> > +++ b/arch/arm/boot/dts/aspeed-bmc-opp-romulus.dts
> > @@ -59,6 +59,7 @@
> >   		compatible = "fsi-master-gpio", "fsi-master";
> >   		#address-cells = <2>;
> >   		#size-cells = <0>;
> > +		no-gpio-delays;
> >
> >   		clock-gpios = <&gpio ASPEED_GPIO(AA, 0) GPIO_ACTIVE_HIGH>;
> >   		data-gpios = <&gpio ASPEED_GPIO(AA, 2) GPIO_ACTIVE_HIGH>;
> > diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts b/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
> > index 596608f4b936..214718b7784d 100644
> > --- a/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
> > +++ b/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
> > @@ -160,6 +160,7 @@
> >   		compatible = "fsi-master-gpio", "fsi-master";
> >   		#address-cells = <2>;
> >   		#size-cells = <0>;
> > +		no-gpio-delays;
> >
> >   		clock-gpios = <&gpio ASPEED_GPIO(AA, 0) GPIO_ACTIVE_HIGH>;
> >   		data-gpios = <&gpio ASPEED_GPIO(E, 0) GPIO_ACTIVE_HIGH>;
> > diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts b/arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts
> > index 7f5a29566801..085926935574 100644
> > --- a/arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts
> > +++ b/arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts
> > @@ -91,6 +91,7 @@
> >   		compatible = "fsi-master-gpio", "fsi-master";
> >   		#address-cells = <2>;
> >   		#size-cells = <0>;
> > +		no-gpio-delays;
> >
> >   		trans-gpios = <&gpio ASPEED_GPIO(O, 6) GPIO_ACTIVE_HIGH>;
> >   		enable-gpios = <&gpio ASPEED_GPIO(D, 0) GPIO_ACTIVE_HIGH>;
> > diff --git a/drivers/fsi/fsi-master-gpio.c b/drivers/fsi/fsi-master-gpio.c
> > index d6508bbad1fb..c82bbd35276e 100644
> > --- a/drivers/fsi/fsi-master-gpio.c
> > +++ b/drivers/fsi/fsi-master-gpio.c
> > @@ -62,6 +62,7 @@ struct fsi_master_gpio {
> >   	struct gpio_desc	*gpio_enable;	/* FSI enable */
> >   	struct gpio_desc	*gpio_mux;	/* Mux control */
> >   	bool			external_mode;
> > +	bool			no_delays;
> >   };
> >
> >   #define CREATE_TRACE_POINTS
> > @@ -79,9 +80,11 @@ static void clock_toggle(struct fsi_master_gpio *master, int count)
> >   	int i;
> >
> >   	for (i = 0; i < count; i++) {
> > -		ndelay(FSI_GPIO_STD_DLY);
> > +		if (!master->no_delays)

This is a bit of a nit, but we don't have to have the member name reflect the devicetree property. I reckon the code would be mildly easier to read (avoiding the double-negative) if we inverted it:

                        if (master->do_delays)
 
> > +			ndelay(FSI_GPIO_STD_DLY);
> >   		gpiod_set_value(master->gpio_clk, 0);
> > -		ndelay(FSI_GPIO_STD_DLY);
> > +		if (!master->no_delays)
> > +			ndelay(FSI_GPIO_STD_DLY);
> >   		gpiod_set_value(master->gpio_clk, 1);
> >   	}
> >   }
> > @@ -90,10 +93,12 @@ static int sda_clock_in(struct fsi_master_gpio *master)
> >   {
> >   	int in;
> >
> > -	ndelay(FSI_GPIO_STD_DLY);
> > +	if (!master->no_delays)
> > +		ndelay(FSI_GPIO_STD_DLY);
> >   	gpiod_set_value(master->gpio_clk, 0);
> >   	in = gpiod_get_value(master->gpio_data);
> > -	ndelay(FSI_GPIO_STD_DLY);
> > +	if (!master->no_delays)
> > +		ndelay(FSI_GPIO_STD_DLY);
> >   	gpiod_set_value(master->gpio_clk, 1);
> >   	return in ? 1 : 0;
> >   }
> > @@ -677,6 +682,13 @@ static int fsi_master_gpio_probe(struct platform_device *pdev)
> >   	}
> >   	master->gpio_mux = gpio;
> >
> > +	/*
> > +	 * Check if GPIO block is slow enought that no extra delays
> > +	 * are necessary. This improves performance on ast2500 by
> > +	 * an order of magnitude.
> > +	 */
> > +	master->no_delays = device_property_present(&pdev->dev, "no-gpio-delays");

Obviously negate the expression here as well if you agree with the comment above.

Cheers,

Andrew

> > +
> >   	master->master.n_links = 1;
> >   	master->master.flags = FSI_MASTER_FLAG_SWCLOCK;
> >   	master->master.read = fsi_master_gpio_read;
>