gpio/aspeed: Set output latch before changing direction
Subsystems (4)
PATCH 1/6 gpio/aspeed: Set output latch before changing direction
In aspeed_gpio_dir_out(), we need to establish the new output value in the output latch *before* we change the direction to output in order to avoid a glitch on the output line if the previous value of the latch was different.
--- drivers/gpio/gpio-aspeed.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/gpio/gpio-aspeed.c +1 -2
@@ -287,11 +287,10 @@ static int aspeed_gpio_dir_out(struct gpio_chip *gc,spin_lock_irqsave (&gpio->lock, flags); +__aspeed_gpio_set (gc, offset, val); reg =ioread32 (bank_val_reg (gpio, bank,GPIO_DIR ));iowrite32 (reg |GPIO_BIT (offset),bank_val_reg (gpio, bank,GPIO_DIR )); -__aspeed_gpio_set (gc, offset, val); -spin_unlock_irqrestore (&gpio->lock, flags);return 0 ;
Signature · 1 line
2.17.0
PATCH 2/6 gpio/aspeed: Use a cache of output data registers
The current driver does a read/modify/write of the output registers when changing a bit in __aspeed_gpio_set().
This is sub-optimal for a couple of reasons:
- If any of the neighbouring GPIOs (sharing the shared
register) isn't (yet) configured as an output, it will read the current input value, and then apply it to the output latch, which may not be what the user expects. There should be no bug in practice as aspeed_gpio_dir_out() will establish a new value but it's not great either.
- The GPIO block in the aspeed chip is clocked rather
slowly (typically 25Mhz). That extra MMIO read halves the maximum speed at which we can toggle the GPIO.
This provides a significant performance improvement to the GPIO based FSI master.
--- drivers/gpio/gpio-aspeed.c | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-)
drivers/gpio/gpio-aspeed.c +19 -2
@@ -54,6 +54,8 @@ struct aspeed_gpio {u8 *offset_timer;unsigned int timer_users[4 ];struct clk *clk; + +u32 *dcache; };struct aspeed_gpio_bank {
@@ -231,12 +233,13 @@ static void __aspeed_gpio_set(struct gpio_chip *gc, unsigned int offset,u32 reg; addr =bank_val_reg (gpio, bank,GPIO_DATA ); - reg =ioread32 (addr); + reg = gpio->dcache[GPIO_BANK (offset)];if (val) reg |=GPIO_BIT (offset);else reg &= ~GPIO_BIT (offset); + gpio->dcache[GPIO_BANK (offset)] = reg;iowrite32 (reg, addr); }
@@ -848,7 +851,7 @@ static int __init aspeed_gpio_probe(struct platform_device *pdev)const struct of_device_id *gpio_id;struct aspeed_gpio *gpio;struct resource *res; -int rc; +int rc, i, banks; gpio =devm_kzalloc (&pdev->dev,sizeof (*gpio),GFP_KERNEL );if (!gpio)
@@ -889,6 +892,20 @@ static int __init aspeed_gpio_probe(struct platform_device *pdev) gpio->chip.base =-1 ; gpio->chip.irq_need_valid_mask = true; +/* Allocate a cache of the output registers */ + banks = gpio->config->nr_gpios >>5 ; + gpio->dcache =devm_kzalloc (&pdev->dev, +sizeof (u32) * banks,GFP_KERNEL ); +if (!gpio->dcache) +return -ENOMEM ; + +/* Populate it with initial values read from the HW */ +for (i =0 ; i < banks; i++) { +const struct aspeed_gpio_bank *bank = &aspeed_gpio_banks[i]; + gpio->dcache[i] =ioread32 (gpio->base + bank->val_regs + +GPIO_DATA ); + } + rc =devm_gpiochip_add_data (&pdev->dev, &gpio->chip, gpio);if (rc <0 )return rc;
Signature · 1 line
2.17.0
Re: [PATCH linux dev-4.13 2/6] gpio/aspeed: Use a cache of output data registers
Quoted 82 lines · Benjamin Herrenschmidt
The current driver does a read/modify/write of the output registers when changing a bit in
__aspeed_gpio_set().This is sub-optimal for a couple of reasons:
- If any of the neighbouring GPIOs (sharing the shared
register) isn't (yet) configured as an output, it will read the current input value, and then apply it to the output latch, which may not be what the user expects. There should be no bug in practice asaspeed_gpio_dir_out()will establish a new value but it's not great either.- The GPIO block in the aspeed chip is clocked rather
slowly (typically 25Mhz). That extra MMIO read halves the maximum speed at which we can toggle the GPIO.This provides a significant performance improvement to the GPIO based FSI master.
Signed-off-by Benjamin Herrenschmidt <[email protected]>--- drivers/gpio/gpio-aspeed.c | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-)drivers/gpio/gpio-aspeed.c +19
-2@@ -54,6 +54,8 @@ struct aspeed_gpio {u8 *offset_timer;unsigned int timer_users[4 ];struct clk *clk; + +u32 *dcache; };struct aspeed_gpio_bank {@@ -231,12 +233,13 @@ static void __aspeed_gpio_set(struct gpio_chip *gc, unsigned int offset,u32 reg; addr =bank_val_reg (gpio, bank,GPIO_DATA ); - reg =ioread32 (addr); + reg = gpio->dcache[GPIO_BANK (offset)];if (val) reg |=GPIO_BIT (offset);else reg &= ~GPIO_BIT (offset); + gpio->dcache[GPIO_BANK (offset)] = reg;iowrite32 (reg, addr); }@@ -848,7 +851,7 @@ static int __init aspeed_gpio_probe(struct platform_device *pdev)const struct of_device_id *gpio_id;struct aspeed_gpio *gpio;struct resource *res; -int rc; +int rc, i, banks; gpio =devm_kzalloc (&pdev->dev,sizeof (*gpio),GFP_KERNEL );if (!gpio)@@ -889,6 +892,20 @@ static int __init aspeed_gpio_probe(struct platform_device *pdev) gpio->chip.base =-1 ; gpio->chip.irq_need_valid_mask = true; +/* Allocate a cache of the output registers */ + banks = gpio->config->nr_gpios >>5 ; + gpio->dcache =devm_kzalloc (&pdev->dev, +sizeof (u32) * banks,GFP_KERNEL ); +if (!gpio->dcache) +return -ENOMEM ; + +/* Populate it with initial values read from the HW */ +for (i =0 ; i < banks; i++) { +const struct aspeed_gpio_bank *bank = &aspeed_gpio_banks[i]; + gpio->dcache[i] =ioread32 (gpio->base + bank->val_regs + +GPIO_DATA ); + } + rc =devm_gpiochip_add_data (&pdev->dev, &gpio->chip, gpio);if (rc <0 )return rc;
Andrew Jeffery
Reviewed-byRe: [PATCH linux dev-4.13 2/6] gpio/aspeed: Use a cache of output data registers
Quoted 24 lines · Christopher Bostic
Reviewed-by Christopher Bostic <[email protected]>Quoted 20 lines · Benjamin Herrenschmidt
The current driver does a read/modify/write of the output registers when changing a bit in
__aspeed_gpio_set().This is sub-optimal for a couple of reasons:
- If any of the neighbouring GPIOs (sharing the shared
register) isn't (yet) configured as an output, it will read the current input value, and then apply it to the output latch, which may not be what the user expects. There should be no bug in practice asaspeed_gpio_dir_out()will establish a new value but it's not great either.- The GPIO block in the aspeed chip is clocked rather
slowly (typically 25Mhz). That extra MMIO read halves the maximum speed at which we can toggle the GPIO.This provides a significant performance improvement to the GPIO based FSI master.
Signed-off-by Benjamin Herrenschmidt <[email protected]>
Quoted 63 lines
Quoted 62 lines
--- drivers/gpio/gpio-aspeed.c | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-)drivers/gpio/gpio-aspeed.c +19
-2@@ -54,6 +54,8 @@ struct aspeed_gpio {u8 *offset_timer;unsigned int timer_users[4 ];struct clk *clk; + +u32 *dcache; };struct aspeed_gpio_bank {@@ -231,12 +233,13 @@ static void __aspeed_gpio_set(struct gpio_chip *gc, unsigned int offset,u32 reg; addr =bank_val_reg (gpio, bank,GPIO_DATA ); - reg =ioread32 (addr); + reg = gpio->dcache[GPIO_BANK (offset)];if (val) reg |=GPIO_BIT (offset);else reg &= ~GPIO_BIT (offset); + gpio->dcache[GPIO_BANK (offset)] = reg;iowrite32 (reg, addr); }@@ -848,7 +851,7 @@ static int __init aspeed_gpio_probe(struct platform_device *pdev)const struct of_device_id *gpio_id;struct aspeed_gpio *gpio;struct resource *res; -int rc; +int rc, i, banks; gpio =devm_kzalloc (&pdev->dev,sizeof (*gpio),GFP_KERNEL );if (!gpio)@@ -889,6 +892,20 @@ static int __init aspeed_gpio_probe(struct platform_device *pdev) gpio->chip.base =-1 ; gpio->chip.irq_need_valid_mask = true; +/* Allocate a cache of the output registers */ + banks = gpio->config->nr_gpios >>5 ; + gpio->dcache =devm_kzalloc (&pdev->dev, +sizeof (u32) * banks,GFP_KERNEL ); +if (!gpio->dcache) +return -ENOMEM ; + +/* Populate it with initial values read from the HW */ +for (i =0 ; i < banks; i++) { +const struct aspeed_gpio_bank *bank = &aspeed_gpio_banks[i]; + gpio->dcache[i] =ioread32 (gpio->base + bank->val_regs + +GPIO_DATA ); + } + rc =devm_gpiochip_add_data (&pdev->dev, &gpio->chip, gpio);if (rc <0 )return rc;
PATCH 3/6 fsi/fsi-master-gpio: Sample input data on different clock phase
In reply to Benjamin HerrenschmidtWe currently sample the input data right after we toggle the clock low, then high. The slave establishes the data on the rising edge, so this is not ideal. We should sample it on the low phase instead.
This currently works because we have an extra delay, but subsequent patches will remove it.
--- drivers/fsi/fsi-master-gpio.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
drivers/fsi/fsi-master-gpio.c +5 -3
@@ -86,12 +86,15 @@ static void clock_toggle(struct fsi_master_gpio *master, int count) } } -static int sda_in (struct fsi_master_gpio *master) +static int sda_clock_in (struct fsi_master_gpio *master) {int in;ndelay (FSI_GPIO_STD_DLY ); +gpiod_set_value (master->gpio_clk,0 ); in =gpiod_get_value (master->gpio_data); +ndelay (FSI_GPIO_STD_DLY ); +gpiod_set_value (master->gpio_clk,1 );return in ?1 :0 ; }
@@ -126,8 +129,7 @@ static void serial_in(struct fsi_master_gpio *master, struct fsi_gpio_msg *msg,set_sda_input (master);for (bit =0 ; bit < num_bits; bit++) { -clock_toggle (master,1 ); - in_bit =sda_in (master); + in_bit =sda_clock_in (master); msg->msg <<=1 ; msg->msg |= ~in_bit &0x1 ;/* Data is active low */ }
Signature · 1 line
2.17.0
Re: [PATCH linux dev-4.13 3/6] fsi/fsi-master-gpio: Sample input data on different clock phase
Quoted 44 lines · Benjamin Herrenschmidt
We currently sample the input data right after we toggle the clock low, then high. The slave establishes the data on the rising edge, so this is not ideal. We should sample it on the low phase instead.
This currently works because we have an extra delay, but subsequent patches will remove it.
Signed-off-by Benjamin Herrenschmidt <[email protected]>--- drivers/fsi/fsi-master-gpio.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)drivers/fsi/fsi-master-gpio.c +5
-3@@ -86,12 +86,15 @@ static void clock_toggle(struct fsi_master_gpio *master, int count) } } -static int sda_in (struct fsi_master_gpio *master) +static int sda_clock_in (struct fsi_master_gpio *master) {int in;ndelay (FSI_GPIO_STD_DLY ); +gpiod_set_value (master->gpio_clk,0 ); in =gpiod_get_value (master->gpio_data); +ndelay (FSI_GPIO_STD_DLY ); +gpiod_set_value (master->gpio_clk,1 );return in ?1 :0 ; }@@ -126,8 +129,7 @@ static void serial_in(struct fsi_master_gpio *master, struct fsi_gpio_msg *msg,set_sda_input (master);for (bit =0 ; bit < num_bits; bit++) { -clock_toggle (master,1 ); - in_bit =sda_in (master); + in_bit =sda_clock_in (master); msg->msg <<=1 ; msg->msg |= ~in_bit &0x1 ;/* Data is active low */ }
PATCH 4/6 fsi/fsi-master-gpio: Add "no-gpio-delays" option
In reply to Benjamin HerrenschmidtThis 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.
--- 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(-)
arch/arm/boot/dts/aspeed-bmc-opp-romulus.dts +1 -0
@@ -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>;
arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts +1 -0
@@ -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>;
arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts +1 -0
@@ -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>;
drivers/fsi/fsi-master-gpio.c +16 -4
@@ -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;
Signature · 1 line
2.17.0
Re: [PATCH linux dev-4.13 4/6] fsi/fsi-master-gpio: Add "no-gpio-delays" option
Quoted 110 lines · Benjamin Herrenschmidt
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 <[email protected]>--- 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(-)arch/arm/boot/dts/aspeed-bmc-opp-romulus.dts +1
-0@@ -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>;arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts +1
-0@@ -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>;arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts +1
-0@@ -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>;drivers/fsi/fsi-master-gpio.c +16
-4@@ -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;
Re: [PATCH linux dev-4.13 4/6] fsi/fsi-master-gpio: Add "no-gpio-delays" option
Quoted 70 lines · Christopher Bostic
Reviewed-by Christopher Bostic <[email protected]>Quoted 66 lines · Benjamin Herrenschmidt
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 <[email protected]>--- 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(-)arch/arm/boot/dts/aspeed-bmc-opp-romulus.dts +1
-0@@ -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>;arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts +1
-0@@ -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>;arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts +1
-0@@ -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>;drivers/fsi/fsi-master-gpio.c +1
-0@@ -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; };
Quoted 7 lines
drivers/fsi/fsi-master-gpio.c static void clock_toggle(struct fsi_master_gpio *master, int count) · line 83
Quoted 7 lines
drivers/fsi/fsi-master-gpio.c +1
-1#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)
Quoted 27 lines
Quoted 27 lines
drivers/fsi/fsi-master-gpio.c +7
-3+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;
Quoted 6 lines
drivers/fsi/fsi-master-gpio.c static int fsi_master_gpio_probe(struct platform_device *pdev) · line 690
Quoted 6 lines
drivers/fsi/fsi-master-gpio.c +6
-0+/* +* 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
Quoted 5 lines
Quoted 4 lines
drivers/fsi/fsi-master-gpio.c +1
-0+ master->master.n_links =1 ; master->master.flags =FSI_MASTER_FLAG_SWCLOCK ; master->master.read = fsi_master_gpio_read;
Re: [PATCH linux dev-4.13 4/6] fsi/fsi-master-gpio: Add "no-gpio-delays" option
Quoted 11 lines · Andrew Jeffery
Quoted 3 lines
Review
Quoted 3 lines
for (i = 0; i < count; i++) {(quoted diff) +1
-1- 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)Quoted 1 line
Quoted 1 line
+
ndelay(FSI_GPIO_STD_DLY);
I prefer it as "no_delays" personally because delays is the norm, and no delays is the exception for aspeed (or slow GPIO blocks).
Cheers,
Ben.
PATCH 5/6 fsi/fsi-master-gpio: Reduce turnaround clocks
In reply to Benjamin HerrenschmidtFSI_GPIO_PRIME_SLAVE_CLOCKS is the number of clocks if the "idle" phase between the end of a response and the beginning of the next one. It corresponds to tSendDelay in the FSI specification.
The default value in the slave is 16 clocks. 100 is way overkill and significantly reduces the driver performance.
This changes it to 20 (which gives the HW a bit of margin still just in case).
--- drivers/fsi/fsi-master-gpio.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/fsi/fsi-master-gpio.c +1 -1
@@ -49,7 +49,7 @@#define FSI_GPIO_CRC_SIZE 4#define FSI_GPIO_MSG_ID_SIZE 2#define FSI_GPIO_MSG_RESPID_SIZE 2 -#define FSI_GPIO_PRIME_SLAVE_CLOCKS 100 +#define FSI_GPIO_PRIME_SLAVE_CLOCKS 20struct fsi_master_gpio {struct fsi_master master;
Signature · 1 line
2.17.0
Re: [PATCH linux dev-4.13 5/6] fsi/fsi-master-gpio: Reduce turnaround clocks
Quoted 29 lines · Benjamin Herrenschmidt
FSI_GPIO_PRIME_SLAVE_CLOCKSis the number of clocks if the "idle" phase between the end of a response and the beginning of the next one. It corresponds to tSendDelay in the FSI specification.The default value in the slave is 16 clocks. 100 is way overkill and significantly reduces the driver performance.
This changes it to 20 (which gives the HW a bit of margin still just in case).
Signed-off-by Benjamin Herrenschmidt <[email protected]>--- drivers/fsi/fsi-master-gpio.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/fsi/fsi-master-gpio.c +1
-1@@ -49,7 +49,7 @@#define FSI_GPIO_CRC_SIZE 4#define FSI_GPIO_MSG_ID_SIZE 2#define FSI_GPIO_MSG_RESPID_SIZE 2 -#define FSI_GPIO_PRIME_SLAVE_CLOCKS 100 +#define FSI_GPIO_PRIME_SLAVE_CLOCKS 20struct fsi_master_gpio {struct fsi_master master;
PATCH 6/6 fsi/fsi-master-gpio: Reduce dpoll clocks
In reply to Benjamin HerrenschmidtFSI_GPIO_DPOLL_CLOCKS is the number of clocks before sending a DPOLL command after receiving a BUSY status. It should be at least tSendDelay (16 clocks).
According to comments in the code, it needs to also be at least 21 clocks due to HW issues.
It's currently 100 clocks which impacts performances negatively in some cases. Reduces it in half to 50 clocks which seems to still be solid.
--- drivers/fsi/fsi-master-gpio.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/fsi/fsi-master-gpio.c +3 -2
@@ -29,7 +29,8 @@#define FSI_GPIO_CMD_TERM 0x3f#define FSI_GPIO_CMD_ABS_AR 0x4 -#define FSI_GPIO_DPOLL_CLOCKS 100/* < 21 will cause slave to hang */ + +#define FSI_GPIO_DPOLL_CLOCKS 50/* < 21 will cause slave to hang */ /* Bus errors */ #define FSI_GPIO_ERR_BUSY 1/* Slave stuck in busy state */
@@ -43,7 +44,7 @@#define FSI_GPIO_RESP_ACK 0#define FSI_GPIO_RESP_ACKD 4 -#define FSI_GPIO_MAX_BUSY 100 +#define FSI_GPIO_MAX_BUSY 200#define FSI_GPIO_MTOE_COUNT 1000#define FSI_GPIO_DRAIN_BITS 20#define FSI_GPIO_CRC_SIZE 4
Re: [PATCH linux dev-4.13 6/6] fsi/fsi-master-gpio: Reduce dpoll clocks
Quoted 40 lines · Benjamin Herrenschmidt
FSI_GPIO_DPOLL_CLOCKSis the number of clocks before sending a DPOLL command after receiving a BUSY status. It should be at least tSendDelay (16 clocks).According to comments in the code, it needs to also be at least 21 clocks due to HW issues.
It's currently 100 clocks which impacts performances negatively in some cases. Reduces it in half to 50 clocks which seems to still be solid.
Signed-off-by Benjamin Herrenschmidt <[email protected]>--- drivers/fsi/fsi-master-gpio.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)drivers/fsi/fsi-master-gpio.c +3
-2@@ -29,7 +29,8 @@#define FSI_GPIO_CMD_TERM 0x3f#define FSI_GPIO_CMD_ABS_AR 0x4 -#define FSI_GPIO_DPOLL_CLOCKS 100/* < 21 will cause slave to hang */ + +#define FSI_GPIO_DPOLL_CLOCKS 50/* < 21 will cause slave to hang */ /* Bus errors */ #define FSI_GPIO_ERR_BUSY 1/* Slave stuck in busy state */ @@ -43,7 +44,7 @@#define FSI_GPIO_RESP_ACK 0#define FSI_GPIO_RESP_ACKD 4 -#define FSI_GPIO_MAX_BUSY 100 +#define FSI_GPIO_MAX_BUSY 200#define FSI_GPIO_MTOE_COUNT 1000#define FSI_GPIO_DRAIN_BITS 20#define FSI_GPIO_CRC_SIZE 4
Re: [PATCH linux dev-4.13 6/6] fsi/fsi-master-gpio: Reduce dpoll clocks
Quoted 14 lines · Christopher Bostic
Reviewed-by Christopher Bostic <[email protected]>Quoted 10 lines · Benjamin Herrenschmidt
FSI_GPIO_DPOLL_CLOCKSis the number of clocks before sending a DPOLL command after receiving a BUSY status. It should be at least tSendDelay (16 clocks).According to comments in the code, it needs to also be at least 21 clocks due to HW issues.
It's currently 100 clocks which impacts performances negatively in some cases. Reduces it in half to 50 clocks which seems to still be solid.
Out of curiosity, was there any science to 50 beyond the (16/21 cycle) reasoning above? Was there instability at lower values?
Quoted 31 lines
Quoted 30 lines
Signed-off-by Benjamin Herrenschmidt <[email protected]>--- drivers/fsi/fsi-master-gpio.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)drivers/fsi/fsi-master-gpio.c +3
-2@@ -29,7 +29,8 @@#define FSI_GPIO_CMD_TERM 0x3f#define FSI_GPIO_CMD_ABS_AR 0x4 -#define FSI_GPIO_DPOLL_CLOCKS 100/* < 21 will cause slave to hang */ + +#define FSI_GPIO_DPOLL_CLOCKS 50/* < 21 will cause slave to hang */ /* Bus errors */ #define FSI_GPIO_ERR_BUSY 1/* Slave stuck in busy state */ @@ -43,7 +44,7 @@#define FSI_GPIO_RESP_ACK 0#define FSI_GPIO_RESP_ACKD 4 -#define FSI_GPIO_MAX_BUSY 100 +#define FSI_GPIO_MAX_BUSY 200#define FSI_GPIO_MTOE_COUNT 1000#define FSI_GPIO_DRAIN_BITS 20#define FSI_GPIO_CRC_SIZE 4
Re: [PATCH linux dev-4.13 6/6] fsi/fsi-master-gpio: Reduce dpoll clocks
Quoted 12 lines · Andrew Jeffery
Quoted 11 lines · Christopher Bostic
Reviewed-by Christopher Bostic <[email protected]>Quoted 7 lines · Benjamin Herrenschmidt
FSI_GPIO_DPOLL_CLOCKSis the number of clocks before sending a DPOLL command after receiving a BUSY status. It should be at least tSendDelay (16 clocks).According to comments in the code, it needs to also be at least 21 clocks due to HW issues.
Quoted 6 lines
Quoted 3 lines
Quoted 3 lines
It's currently 100 clocks which impacts performances negatively in some cases. Reduces it in half to 50 clocks which seems to still be solid.
Out of curiosity, was there any science to 50 beyond the (16/21 cycle) reasoning above? Was there instability at lower values?
No. The "science" aka spec would need it to be tSendDelay, which is 16 cycles. However there's a comment that says that there are issues below
21. I decided to keep *some* paranoia and cut in half to 50 instead of 100.
Quoted 30 lines
Quoted 30 lines
Quoted 30 lines
Signed-off-by Benjamin Herrenschmidt <[email protected]>--- drivers/fsi/fsi-master-gpio.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)drivers/fsi/fsi-master-gpio.c +3
-2@@ -29,7 +29,8 @@#define FSI_GPIO_CMD_TERM 0x3f#define FSI_GPIO_CMD_ABS_AR 0x4 -#define FSI_GPIO_DPOLL_CLOCKS 100/* < 21 will cause slave to hang */ + +#define FSI_GPIO_DPOLL_CLOCKS 50/* < 21 will cause slave to hang */ /* Bus errors */ #define FSI_GPIO_ERR_BUSY 1/* Slave stuck in busy state */ @@ -43,7 +44,7 @@#define FSI_GPIO_RESP_ACK 0#define FSI_GPIO_RESP_ACKD 4 -#define FSI_GPIO_MAX_BUSY 100 +#define FSI_GPIO_MAX_BUSY 200#define FSI_GPIO_MTOE_COUNT 1000#define FSI_GPIO_DRAIN_BITS 20#define FSI_GPIO_CRC_SIZE 4
PATCH 6/6 fsi/fsi-master-gpio: Reduce dpoll clocks
In reply to Benjamin HerrenschmidtFSI_GPIO_DPOLL_CLOCKS is the number of clocks before sending a DPOLL command after receiving a BUSY status. It should be at least tSendDelay (16 clocks).
According to comments in the code, it needs to also be at least 21 clocks due to HW issues.
It's currently 100 clocks which impacts performances negatively in some cases. Reduces it in half to 50 clocks which seems to still be solid.
--- drivers/fsi/fsi-master-gpio.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/fsi/fsi-master-gpio.c +3 -2
@@ -29,7 +29,8 @@#define FSI_GPIO_CMD_TERM 0x3f#define FSI_GPIO_CMD_ABS_AR 0x4 -#define FSI_GPIO_DPOLL_CLOCKS 100/* < 21 will cause slave to hang */ + +#define FSI_GPIO_DPOLL_CLOCKS 50/* < 21 will cause slave to hang */ /* Bus errors */ #define FSI_GPIO_ERR_BUSY 1/* Slave stuck in busy state */
@@ -43,7 +44,7 @@#define FSI_GPIO_RESP_ACK 0#define FSI_GPIO_RESP_ACKD 4 -#define FSI_GPIO_MAX_BUSY 100 +#define FSI_GPIO_MAX_BUSY 200#define FSI_GPIO_MTOE_COUNT 1000#define FSI_GPIO_DRAIN_BITS 20#define FSI_GPIO_CRC_SIZE 4
Signature · 1 line
2.17.0
Quoted 27 lines · Benjamin Herrenschmidt
In
aspeed_gpio_dir_out(), we need to establish the new output value in the output latch *before* we change the direction to output in order to avoid a glitch on the output line if the previous value of the latch was different.Signed-off-by Benjamin Herrenschmidt <[email protected]>--- drivers/gpio/gpio-aspeed.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)drivers/gpio/gpio-aspeed.c +1
-2@@ -287,11 +287,10 @@ static int aspeed_gpio_dir_out(struct gpio_chip *gc,spin_lock_irqsave (&gpio->lock, flags); +__aspeed_gpio_set (gc, offset, val); reg =ioread32 (bank_val_reg (gpio, bank,GPIO_DIR ));iowrite32 (reg |GPIO_BIT (offset),bank_val_reg (gpio, bank,GPIO_DIR )); -__aspeed_gpio_set (gc, offset, val); -spin_unlock_irqrestore (&gpio->lock, flags);return 0 ;
Andrew Jeffery
Reviewed-byQuoted 9 lines · Christopher Bostic
Reviewed-by Christopher Bostic <[email protected]>Quoted 5 lines · Benjamin Herrenschmidt
In
aspeed_gpio_dir_out(), we need to establish the new output value in the output latch *before* we change the direction to output in order to avoid a glitch on the output line if the previous value of the latch was different.
I think Milton mentioned this in a review way back when I was upstreaming the driver, but looks like I forgot to address it.
Quoted 1 line
Quoted 1 line
Signed-off-by Benjamin Herrenschmidt <[email protected]>
Quoted 22 lines
Quoted 21 lines
--- drivers/gpio/gpio-aspeed.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)drivers/gpio/gpio-aspeed.c +1
-2@@ -287,11 +287,10 @@ static int aspeed_gpio_dir_out(struct gpio_chip *gc,spin_lock_irqsave (&gpio->lock, flags); +__aspeed_gpio_set (gc, offset, val); reg =ioread32 (bank_val_reg (gpio, bank,GPIO_DIR ));iowrite32 (reg |GPIO_BIT (offset),bank_val_reg (gpio, bank,GPIO_DIR )); -__aspeed_gpio_set (gc, offset, val); -spin_unlock_irqrestore (&gpio->lock, flags);return 0 ;
Joel Stanley
maintainerQuoted 11 lines · Andrew Jeffery
Quoted 9 lines · Christopher Bostic
Reviewed-by Christopher Bostic <[email protected]>Quoted 5 lines · Benjamin Herrenschmidt
In
aspeed_gpio_dir_out(), we need to establish the new output value in the output latch *before* we change the direction to output in order to avoid a glitch on the output line if the previous value of the latch was different.
Quoted 5 lines
I think Milton mentioned this in a review way back when I was upstreaming the driver, but looks like I forgot to address it.
Quoted 1 line
Quoted 1 line
Signed-off-by Benjamin Herrenschmidt <[email protected]>Reviewed-by Andrew Jeffery <[email protected]>
Ben, can you please send this upstream as a fix?
Cheers,
Joel
Quoted 11 lines · Andrew Jeffery
Quoted 9 lines · Christopher Bostic
Reviewed-by Christopher Bostic <[email protected]>Quoted 5 lines · Benjamin Herrenschmidt
In
aspeed_gpio_dir_out(), we need to establish the new output value in the output latch *before* we change the direction to output in order to avoid a glitch on the output line if the previous value of the latch was different.
Quoted 5 lines
I think Milton mentioned this in a review way back when I was upstreaming the driver, but looks like I forgot to address it.
Quoted 1 line
Quoted 1 line
Signed-off-by Benjamin Herrenschmidt <[email protected]>Reviewed-by Andrew Jeffery <[email protected]>
I've applied this series to dev-4.13. Thanks for the patches, and thanks to everyone who reviewed.
Cheers,
Joel