linux-kernel - Re: [PATCH 05/15] fsi/fsi-master-gpio: Add "no-gpio-delays" option

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f2091b1df25fcfa1e8073e2eb3281f4c7b1247d7.camel@kernel.crashing.org>
Date:   Tue, 29 May 2018 11:42:11 +1000
From:   Benjamin Herrenschmidt <benh@...nel.crashing.org>
To:     linux-kernel@...r.kernel.org
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jeremy Kerr <jk@...abs.org>, Joel Stanley <joel@....id.au>,
        Christopher Bostic <cbostic@...ux.vnet.ibm.com>
Subject: Re: [PATCH 05/15] fsi/fsi-master-gpio: Add "no-gpio-delays" option

On Tue, 2018-05-29 at 11:30 +1000, 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.

Forgot to add the DT binding documentation, I'll send it as a separate
patch.

> Signed-off-by: Benjamin Herrenschmidt <benh@...nel.crashing.org>
> Reviewed-by: Christopher Bostic <cbostic@...ux.vnet.ibm.com>
> ---
>  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 3a93ae77cde3..71cb9317d624 100644
> --- a/arch/arm/boot/dts/aspeed-bmc-opp-romulus.dts
> +++ b/arch/arm/boot/dts/aspeed-bmc-opp-romulus.dts
> @@ -53,6 +53,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 dab467463c99..97cf819309b5 100644
> --- a/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
> +++ b/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
> @@ -126,6 +126,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 f3bc89a4262f..71a64730f080 100644
> --- a/arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts
> +++ b/arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts
> @@ -86,6 +86,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)
> +			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");
> +
>  	master->master.n_links = 1;
>  	master->master.flags = FSI_MASTER_FLAG_SWCLOCK;
>  	master->master.read = fsi_master_gpio_read;