fsi: Overall improvements and new SBE fifo driver
Subsystems (4)
cover fsi: Overall improvements and new SBE fifo driver
This series brings in a number of improvements to our FSI stack (one of the service interfaces for communicating between a BMC chip and our POWER processors).
The GPIO based "Soft FSI" performance is significantly improved, and it's reliability as well.
The SBE fifo driver provides the interface to the processor "Self Boot Engine"
Some of these patches have been simmering for a while in the OpenBMC tree.
PATCH 1/15 fsi: gpio: Trace busy count
From: Andrew Jeffery <[email protected]>
An observation from trace output of the existing FSI tracepoints was that the remote device was sometimes reporting as busy. Add a new tracepoint reporting the busy count in order to get a better grip on how often this is the case.
Signed-off-by Andrew Jeffery <[email protected]>
Acked-by Eddie James <[email protected]>
Signed-off-by Benjamin Herrenschmidt <[email protected]>
--- drivers/fsi/fsi-master-gpio.c | 3 +++ include/trace/events/fsi_master_gpio.h | 16 ++++++++++++++++ 2 files changed, 19 insertions(+)
drivers/fsi/fsi-master-gpio.c +3 -0
@@ -401,6 +401,9 @@ static int poll_for_response(struct fsi_master_gpio *master,break ; } +if (busy_count >0 ) +trace_fsi_master_gpio_poll_response_busy (master, busy_count); +/* Clock the slave enough to be ready for next operation */ clock_zeros (master,FSI_GPIO_PRIME_SLAVE_CLOCKS );return rc;
include/trace/events/fsi_master_gpio.h +16 -0
@@ -64,6 +64,22 @@ TRACE_EVENT(fsi_master_gpio_break, ) ); + +TRACE_EVENT (fsi_master_gpio_poll_response_busy, + TP_PROTO(const struct fsi_master_gpio *master, int busy), +TP_ARGS (master, busy), +TP_STRUCT__entry ( + __field(int, master_idx) +__field (int, busy) + ), +TP_fast_assign ( + __entry->master_idx = master->master.idx; + __entry->busy = busy; + ), +TP_printk ("fsi-gpio%d: device reported busy %d times" , + __entry->master_idx, __entry->busy) +); +#endif /* _TRACE_FSI_MASTER_GPIO_H */ #include <trace/define_trace.h>
Signature · 1 line
2.17.0
PATCH 2/15 fsi: gpio: Remove unused 'id' variable
In reply to Benjamin HerrenschmidtFrom: Andrew Jeffery <[email protected]>
Signed-off-by Andrew Jeffery <[email protected]>
Signed-off-by Benjamin Herrenschmidt <[email protected]>
--- drivers/fsi/fsi-master-gpio.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/fsi/fsi-master-gpio.c +1 -2
@@ -270,7 +270,7 @@ static int read_one_response(struct fsi_master_gpio *master, uint8_t data_size, struct fsi_gpio_msg *msgp, uint8_t *tagp) { struct fsi_gpio_msg msg; -uint8_t id, tag; +uint8_t tag;uint32_t crc;int i;
@@ -295,7 +295,6 @@ static int read_one_response(struct fsi_master_gpio *master,/* Read slave ID & response tag */ serial_in (master, &msg,4 ); - id = (msg.msg >>FSI_GPIO_MSG_RESPID_SIZE ) &0x3 ; tag = msg.msg &0x3 ;/* If we have an ACK and we're expecting data, clock the data in too */
Signature · 1 line
2.17.0
PATCH 3/15 fsi: gpio: Use a mutex to protect transfers
In reply to Benjamin HerrenschmidtFrom: Jeremy Kerr <[email protected]>
Reduce time spent with interrupts disabled by limiting the critical sections to bitbanging FSI symbols. We only need to ensure exclusive use of the bus for an entire transfer, not that the transfer be performed in atomic context.
Signed-off-by Jeremy Kerr <[email protected]>
Signed-off-by Benjamin Herrenschmidt <[email protected]>
--- drivers/fsi/fsi-master-gpio.c | 86 ++++++++++++++++++++++++++--------- 1 file changed, 64 insertions(+), 22 deletions(-)
drivers/fsi/fsi-master-gpio.c +64 -22
@@ -54,7 +54,8 @@struct fsi_master_gpio {struct fsi_master master;struct device *dev; -spinlock_t cmd_lock;/* Lock for commands */ +struct mutex cmd_lock;/* mutex for command ordering */ +spinlock_t bit_lock;/* lock for clocking bits out */ struct gpio_desc *gpio_clk;struct gpio_desc *gpio_data;struct gpio_desc *gpio_trans;/* Voltage translator */
@@ -270,10 +271,13 @@ static int read_one_response(struct fsi_master_gpio *master, uint8_t data_size, struct fsi_gpio_msg *msgp, uint8_t *tagp) { struct fsi_gpio_msg msg; -uint8_t tag; +unsigned long flags;uint32_t crc; +uint8_t tag;int i; +spin_lock_irqsave (&master->bit_lock, flags); +/* wait for the start bit */ for (i =0 ; i <FSI_GPIO_MTOE_COUNT ; i++) { msg.bits =0 ;
@@ -286,6 +290,7 @@ static int read_one_response(struct fsi_master_gpio *master,dev_dbg (master->dev,"Master time out waiting for response\n" );fsi_master_gpio_error (master,FSI_GPIO_MTOE ); +spin_unlock_irqrestore (&master->bit_lock, flags);return -EIO ; }
@@ -304,6 +309,8 @@ static int read_one_response(struct fsi_master_gpio *master,/* read CRC */ serial_in (master, &msg,FSI_GPIO_CRC_SIZE ); +spin_unlock_irqrestore (&master->bit_lock, flags); +/* we have a whole message now; check CRC */ crc =crc4 (0 ,1 ,1 ); crc =crc4 (crc, msg.msg, msg.bits);
@@ -324,12 +331,16 @@ static int read_one_response(struct fsi_master_gpio *master,static int issue_term (struct fsi_master_gpio *master,uint8_t slave) {struct fsi_gpio_msg cmd; +unsigned long flags;uint8_t tag;int rc;build_term_command (&cmd, slave); + +spin_lock_irqsave (&master->bit_lock, flags);serial_out (master, &cmd);echo_delay (master); +spin_unlock_irqrestore (&master->bit_lock, flags); rc =read_one_response (master,0 ,NULL , &tag);if (rc <0 ) {
@@ -349,6 +360,7 @@ static int poll_for_response(struct fsi_master_gpio *master, {struct fsi_gpio_msg response, cmd;int busy_count =0 , rc, i; +unsigned long flags;uint8_t tag;uint8_t *data_byte = data;
@@ -377,15 +389,20 @@ static int poll_for_response(struct fsi_master_gpio *master,* d-poll, not indicated in the hardware protocol * spec. < 20 clocks causes slave to hang, 21 ok. */ -clock_zeros (master,FSI_GPIO_DPOLL_CLOCKS );if (busy_count++ <FSI_GPIO_MAX_BUSY ) {build_dpoll_command (&cmd, slave); +spin_lock_irqsave (&master->bit_lock, flags); +clock_zeros (master,FSI_GPIO_DPOLL_CLOCKS );serial_out (master, &cmd);echo_delay (master); +spin_unlock_irqrestore (&master->bit_lock, flags); goto retry; }dev_warn (master->dev,"ERR slave is stuck in busy state, issuing TERM\n" ); +spin_lock_irqsave (&master->bit_lock, flags); +clock_zeros (master,FSI_GPIO_DPOLL_CLOCKS ); +spin_unlock_irqrestore (&master->bit_lock, flags);issue_term (master, slave); rc = -EIO ;break ;
@@ -404,27 +421,42 @@ static int poll_for_response(struct fsi_master_gpio *master,trace_fsi_master_gpio_poll_response_busy (master, busy_count);/* Clock the slave enough to be ready for next operation */ +spin_lock_irqsave (&master->bit_lock, flags);clock_zeros (master,FSI_GPIO_PRIME_SLAVE_CLOCKS ); +spin_unlock_irqrestore (&master->bit_lock, flags);return rc; } -static int fsi_master_gpio_xfer (struct fsi_master_gpio *master,uint8_t slave, -struct fsi_gpio_msg *cmd,size_t resp_len,void *resp) +static int send_request (struct fsi_master_gpio *master, +struct fsi_gpio_msg *cmd) {unsigned long flags; -int rc; - -spin_lock_irqsave (&master->cmd_lock, flags); +spin_lock_irqsave (&master->bit_lock, flags);if (master->external_mode) { -spin_unlock_irqrestore (&master->cmd_lock, flags); +spin_unlock_irqrestore (&master->bit_lock, flags);return -EBUSY ; }serial_out (master, cmd);echo_delay (master); - rc =poll_for_response (master, slave, resp_len, resp); -spin_unlock_irqrestore (&master->cmd_lock, flags); +spin_unlock_irqrestore (&master->bit_lock, flags); + +return 0 ; +} + +static int fsi_master_gpio_xfer (struct fsi_master_gpio *master,uint8_t slave, +struct fsi_gpio_msg *cmd,size_t resp_len,void *resp) +{ +int rc; + +mutex_lock (&master->cmd_lock); + + rc =send_request (master, cmd); +if (!rc) + rc =poll_for_response (master, slave, resp_len, resp); + +mutex_unlock (&master->cmd_lock);return rc; }
@@ -478,11 +510,14 @@ static int fsi_master_gpio_break(struct fsi_master *_master, int link)trace_fsi_master_gpio_break (master); -spin_lock_irqsave (&master->cmd_lock, flags); +mutex_lock (&master->cmd_lock);if (master->external_mode) { -spin_unlock_irqrestore (&master->cmd_lock, flags); +mutex_unlock (&master->cmd_lock);return -EBUSY ; } + +spin_lock_irqsave (&master->bit_lock, flags); +set_sda_output (master,1 );sda_out (master,1 );clock_toggle (master,FSI_PRE_BREAK_CLOCKS );
@@ -491,7 +526,9 @@ static int fsi_master_gpio_break(struct fsi_master *_master, int link)echo_delay (master);sda_out (master,1 );clock_toggle (master,FSI_POST_BREAK_CLOCKS ); -spin_unlock_irqrestore (&master->cmd_lock, flags); + +spin_unlock_irqrestore (&master->bit_lock, flags); +mutex_unlock (&master->cmd_lock);/* Wait for logic reset to take effect */ udelay (200 );
@@ -501,6 +538,8 @@ static int fsi_master_gpio_break(struct fsi_master *_master, int link)static void fsi_master_gpio_init (struct fsi_master_gpio *master) { +unsigned long flags; +gpiod_direction_output (master->gpio_mux,1 );gpiod_direction_output (master->gpio_trans,1 );gpiod_direction_output (master->gpio_enable,1 );
@@ -508,7 +547,9 @@ static void fsi_master_gpio_init(struct fsi_master_gpio *master)gpiod_direction_output (master->gpio_data,1 );/* todo: evaluate if clocks can be reduced */ +spin_lock_irqsave (&master->bit_lock, flags);clock_zeros (master,FSI_INIT_CLOCKS ); +spin_unlock_irqrestore (&master->bit_lock, flags); }static void fsi_master_gpio_init_external (struct fsi_master_gpio *master)
@@ -523,18 +564,17 @@ static void fsi_master_gpio_init_external(struct fsi_master_gpio *master)static int fsi_master_gpio_link_enable (struct fsi_master *_master,int link) {struct fsi_master_gpio *master =to_fsi_master_gpio (_master); -unsigned long flags;int rc = -EBUSY ;if (link !=0 )return -ENODEV ; -spin_lock_irqsave (&master->cmd_lock, flags); +mutex_lock (&master->cmd_lock);if (!master->external_mode) {gpiod_set_value (master->gpio_enable,1 ); rc =0 ; } -spin_unlock_irqrestore (&master->cmd_lock, flags); +mutex_unlock (&master->cmd_lock);return rc; }
@@ -552,7 +592,7 @@ static ssize_t external_mode_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t count) { struct fsi_master_gpio *master = dev_get_drvdata(dev); -unsigned long flags, val; +unsigned long val;bool external_mode;int err;
@@ -562,10 +602,10 @@ static ssize_t external_mode_store(struct device *dev, external_mode = !!val; -spin_lock_irqsave (&master->cmd_lock, flags); +mutex_lock (&master->cmd_lock);if (external_mode == master->external_mode) { -spin_unlock_irqrestore (&master->cmd_lock, flags); +mutex_unlock (&master->cmd_lock);return count; }
@@ -574,7 +614,8 @@ static ssize_t external_mode_store(struct device *dev,fsi_master_gpio_init_external (master); elsefsi_master_gpio_init (master ); -spin_unlock_irqrestore (&master->cmd_lock, flags); + +mutex_unlock (&master->cmd_lock);fsi_master_rescan (&master->master);
@@ -642,7 +683,8 @@ static int fsi_master_gpio_probe(struct platform_device *pdev) master->master.send_break = fsi_master_gpio_break; master->master.link_enable = fsi_master_gpio_link_enable;platform_set_drvdata (pdev, master); -spin_lock_init (&master->cmd_lock); +spin_lock_init (&master->bit_lock); +mutex_init (&master->cmd_lock);fsi_master_gpio_init (master);
Signature · 1 line
2.17.0
PATCH 4/15 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.
Signed-off-by Benjamin Herrenschmidt <[email protected]>
Reviewed-by Christopher Bostic <[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 */ }
Signature · 1 line
2.17.0
PATCH 5/15 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.
Signed-off-by Benjamin Herrenschmidt <[email protected]>
Reviewed-by Christopher Bostic <[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
@@ -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>;
arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts +1 -0
@@ -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>;
arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts +1 -0
@@ -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>;
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 05/15] fsi/fsi-master-gpio: Add "no-gpio-delays" option
Quoted 10 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.
Forgot to add the DT binding documentation, I'll send it as a separate patch.
Quoted 100 lines
Signed-off-by Benjamin Herrenschmidt <[email protected]>Reviewed-by Christopher Bostic <[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@@ -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>;arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts +1
-0@@ -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>;arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts +1
-0@@ -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>;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;
PATCH 1/1 dt-bindings: fsi-master-gpio: Document "no-gpio-delays" property
In reply to Benjamin HerrenschmidtSupport for this is being added to the driver but the original patch forgot to add this documentation.
Signed-off-by Benjamin Herrenschmidt <[email protected]>
--- Documentation/devicetree/bindings/fsi/fsi-master-gpio.txt | 4 ++++ 1 file changed, 4 insertions(+)
Documentation/devicetree/bindings/fsi/fsi-master-gpio.txt +4 -0
@@ -11,6 +11,10 @@ Optional properties: - trans-gpios = <gpio-descriptor>; : GPIO for voltage translator enable - mux-gpios = <gpio-descriptor>; : GPIO for pin multiplexing with other functions (eg, external FSI masters) + - no-gpio-delays; : Don't add extra delays between GPIO + accesses. This is useful when the HW + GPIO block is running at a low enough + frequency. Examples:
Re: [PATCH] dt-bindings: fsi-master-gpio: Document "no-gpio-delays" property
Quoted 7 lines · Benjamin Herrenschmidt
Support for this is being added to the driver but the original patch forgot to add this documentation.
Signed-off-by Benjamin Herrenschmidt <[email protected]>--- Documentation/devicetree/bindings/fsi/fsi-master-gpio.txt | 4 ++++ 1 file changed, 4 insertions(+)
Applied.
Rob
PATCH 6/15 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).
Signed-off-by Benjamin Herrenschmidt <[email protected]>
Reviewed-by Christopher Bostic <[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;
Signature · 1 line
2.17.0
PATCH 7/15 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.
Signed-off-by Benjamin Herrenschmidt <[email protected]>
Reviewed-by Christopher Bostic <[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
Signature · 1 line
2.17.0
PATCH 8/15 fsi/fsi-master-gpio: Delay sampling of FSI data input
In reply to Benjamin HerrenschmidtMost SoC GPIO implementations, including the Aspeed one, have synchronizers on the GPIO inputs. This means that the value read from a GPIO is a couple of clocks old, from whatever clock source feeds those synchronizers.
In practice, this means that in no-delay mode, we are using a value that can potentially be a bit too old and too close to the clock edge establishing the data on the other side of the link.
The voltage converters we use on some systems make this worse and sensitive to things like voltage fluctuations etc... This is, we believe, the cause of occasional CRC errors encountered during heavy activity on the LPC bus.
This is fixed by introducing a dummy GPIO read before the actual data read. It slows down SBEFIFO by about 15% (less than any delay primitive) and the end result is so far solid.
Signed-off-by Benjamin Herrenschmidt <[email protected]>
Reviewed-by Christopher Bostic <[email protected]>
--- drivers/fsi/fsi-master-gpio.c | 5 +++++ 1 file changed, 5 insertions(+)
drivers/fsi/fsi-master-gpio.c +5 -0
@@ -97,6 +97,11 @@ static int sda_clock_in(struct fsi_master_gpio *master)if (!master->no_delays)ndelay (FSI_GPIO_STD_DLY );gpiod_set_value (master->gpio_clk,0 ); + +/* Dummy read to feed the synchronizers */ +gpiod_get_value (master->gpio_data); + +/* Actual data read */ in =gpiod_get_value (master->gpio_data);if (!master->no_delays)ndelay (FSI_GPIO_STD_DLY );
Signature · 1 line
2.17.0
PATCH 9/15 fsi/gpio: Include command build in locked section
In reply to Benjamin HerrenschmidtFrom: Jeremy Kerr <[email protected]>
For implementing relative addressing mode, we'll need to build a command that is coherent with CFAM state. To do that, include the build_command_* functions in the locked section of read/write/term.
Signed-off-by Jeremy Kerr <[email protected]>
Signed-off-by Benjamin Herrenschmidt <[email protected]>
--- drivers/fsi/fsi-master-gpio.c | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-)
drivers/fsi/fsi-master-gpio.c +18 -7
@@ -463,14 +463,10 @@ static int fsi_master_gpio_xfer(struct fsi_master_gpio *master, uint8_t slave, {int rc; -mutex_lock (&master->cmd_lock); - rc =send_request (master, cmd);if (!rc) rc =poll_for_response (master, slave, resp_len, resp); -mutex_unlock (&master->cmd_lock); -return rc; }
@@ -479,12 +475,17 @@ static int fsi_master_gpio_read(struct fsi_master *_master, int link, {struct fsi_master_gpio *master =to_fsi_master_gpio (_master);struct fsi_gpio_msg cmd; +int rc;if (link !=0 )return -ENODEV ; +mutex_lock (&master->cmd_lock);build_abs_ar_command (&cmd, id, addr, size,NULL ); -return fsi_master_gpio_xfer (master, id, &cmd, size, val); + rc =fsi_master_gpio_xfer (master, id, &cmd, size, val); +mutex_unlock (&master->cmd_lock); + +return rc; } static int fsi_master_gpio_write(struct fsi_master *_master, int link,
@@ -492,12 +493,17 @@ static int fsi_master_gpio_write(struct fsi_master *_master, int link, {struct fsi_master_gpio *master =to_fsi_master_gpio (_master);struct fsi_gpio_msg cmd; +int rc;if (link !=0 )return -ENODEV ; +mutex_lock (&master->cmd_lock);build_abs_ar_command (&cmd, id, addr, size, val); -return fsi_master_gpio_xfer (master, id, &cmd,0 ,NULL ); + rc =fsi_master_gpio_xfer (master, id, &cmd,0 ,NULL ); +mutex_unlock (&master->cmd_lock); + +return rc; } static int fsi_master_gpio_term(struct fsi_master *_master,
@@ -505,12 +511,17 @@ static int fsi_master_gpio_term(struct fsi_master *_master, {struct fsi_master_gpio *master =to_fsi_master_gpio (_master);struct fsi_gpio_msg cmd; +int rc;if (link !=0 )return -ENODEV ; +mutex_lock (&master->cmd_lock);build_term_command (&cmd, id); -return fsi_master_gpio_xfer (master, id, &cmd,0 ,NULL ); + rc =fsi_master_gpio_xfer (master, id, &cmd,0 ,NULL ); +mutex_unlock (&master->cmd_lock); + +return rc; }static int fsi_master_gpio_break (struct fsi_master *_master,int link)
Signature · 1 line
2.17.0
PATCH 10/15 fsi/gpio: Use relative-addressing commands
In reply to Benjamin HerrenschmidtFrom: Jeremy Kerr <[email protected]>
FSI CFAMs support shorter commands that use a relative (or same) address as the last. This change introduces a last_addr to the master state, and uses it for subsequent reads/writes, and performs relative addressing when a subsequent read/write is in range.
Signed-off-by Jeremy Kerr <[email protected]>
Signed-off-by Benjamin Herrenschmidt <[email protected]>
Reviewed-by Christopher Bostic <[email protected]>
--- drivers/fsi/fsi-master-gpio.c | 102 ++++++++++++++++++++++++++++++---- 1 file changed, 91 insertions(+), 11 deletions(-)
drivers/fsi/fsi-master-gpio.c +91 -11
@@ -28,6 +28,8 @@#define FSI_GPIO_CMD_DPOLL 0x2#define FSI_GPIO_CMD_TERM 0x3f#define FSI_GPIO_CMD_ABS_AR 0x4 +#define FSI_GPIO_CMD_REL_AR 0x5 +#define FSI_GPIO_CMD_SAME_AR 0x3/* but only a 2-bit opcode... */ #define FSI_GPIO_DPOLL_CLOCKS 50/* < 21 will cause slave to hang */
@@ -52,6 +54,8 @@#define FSI_GPIO_MSG_RESPID_SIZE 2#define FSI_GPIO_PRIME_SLAVE_CLOCKS 20 +#define LAST_ADDR_INVALID 0x1 +struct fsi_master_gpio {struct fsi_master master;struct device *dev;
@@ -64,6 +68,7 @@ struct fsi_master_gpio {struct gpio_desc *gpio_mux;/* Mux control */ bool external_mode;bool no_delays; +uint32_t last_addr; };#define CREATE_TRACE_POINTS
@@ -205,22 +210,89 @@ static void msg_push_crc(struct fsi_gpio_msg *msg)msg_push_bits (msg, crc,4 ); } +static bool check_same_address (struct fsi_master_gpio *master,int id, +uint32_t addr) +{ +/* this will also handle LAST_ADDR_INVALID */ +return master->last_addr == (((id &0x3 ) <<21 ) | (addr & ~0x3 )); +} + +static bool check_relative_address (struct fsi_master_gpio *master,int id, +uint32_t addr,uint32_t *rel_addrp) +{ +uint32_t last_addr = master->last_addr; +int32_t rel_addr; + +if (last_addr ==LAST_ADDR_INVALID ) +return false; + +/* We may be in 23-bit addressing mode, which uses the id as the +* top two address bits. So, if we're referencing a different ID, +* use absolute addresses. +*/ +if (((last_addr >>21 ) &0x3 ) != id) +return false; + +/* remove the top two bits from any 23-bit addressing */ + last_addr &= (1 <<21 ) -1 ; + +/* We know that the addresses are limited to 21 bits, so this won't +* overflow the signed rel_addr */ + rel_addr = addr - last_addr; +if (rel_addr >255 || rel_addr <-256 ) +return false; + + *rel_addrp = (uint32_t )rel_addr; + +return true; +} + +static void last_address_update (struct fsi_master_gpio *master, +int id,bool valid,uint32_t addr) +{ +if (!valid) + master->last_addr =LAST_ADDR_INVALID ; +else + master->last_addr = ((id &0x3 ) <<21 ) | (addr & ~0x3 ); +} +/* -* Encode an Absolute Address command +* Encode an Absolute/Relative/Same Address command */ -static void build_abs_ar_command (struct fsi_gpio_msg *cmd, -uint8_t id,uint32_t addr,size_t size,const void *data) +static void build_ar_command (struct fsi_master_gpio *master, +struct fsi_gpio_msg *cmd,uint8_t id, +uint32_t addr,size_t size,const void *data) { +int i, addr_bits, opcode_bits;bool write = !!data; -uint8_t ds; -int i; +uint8_t ds, opcode; +uint32_t rel_addr; cmd->bits =0 ; cmd->msg =0 ; -msg_push_bits (cmd, id,2 ); -msg_push_bits (cmd,FSI_GPIO_CMD_ABS_AR ,3 ); -msg_push_bits (cmd, write ?0 :1 ,1 ); +/* we have 21 bits of address max */ + addr &= ((1 <<21 ) -1 ); + +/* cmd opcodes are variable length - SAME_AR is only two bits */ + opcode_bits =3 ; + +if (check_same_address (master, id, addr)) { +/* we still address the byte offset within the word */ + addr_bits =2 ; + opcode_bits =2 ; + opcode =FSI_GPIO_CMD_SAME_AR ; + + }else if (check_relative_address (master, id, addr, &rel_addr)) { +/* 8 bits plus sign */ + addr_bits =9 ; + addr = rel_addr; + opcode =FSI_GPIO_CMD_REL_AR ; + + }else { + addr_bits =21 ; + opcode =FSI_GPIO_CMD_ABS_AR ; + }/* * The read/write size is encoded in the lower bits of the address
@@ -237,7 +309,10 @@ static void build_abs_ar_command(struct fsi_gpio_msg *cmd,if (size ==4 ) addr |=1 ; -msg_push_bits (cmd, addr & ((1 <<21 ) -1 ),21 ); +msg_push_bits (cmd, id,2 ); +msg_push_bits (cmd, opcode, opcode_bits); +msg_push_bits (cmd, write ?0 :1 ,1 ); +msg_push_bits (cmd, addr, addr_bits);msg_push_bits (cmd, ds,1 );for (i =0 ; write && i < size; i++)msg_push_bits (cmd, ((uint8_t *)data)[i],8 );
@@ -481,8 +556,9 @@ static int fsi_master_gpio_read(struct fsi_master *_master, int link,return -ENODEV ;mutex_lock (&master->cmd_lock); -build_abs_ar_command (&cmd, id, addr, size,NULL ); +build_ar_command (master, &cmd, id, addr, size,NULL ); rc =fsi_master_gpio_xfer (master, id, &cmd, size, val); +last_address_update (master, id, rc ==0 , addr);mutex_unlock (&master->cmd_lock);return rc;
@@ -499,8 +575,9 @@ static int fsi_master_gpio_write(struct fsi_master *_master, int link,return -ENODEV ;mutex_lock (&master->cmd_lock); -build_abs_ar_command (&cmd, id, addr, size, val); +build_ar_command (master, &cmd, id, addr, size, val); rc =fsi_master_gpio_xfer (master, id, &cmd,0 ,NULL ); +last_address_update (master, id, rc ==0 , addr);mutex_unlock (&master->cmd_lock);return rc;
@@ -519,6 +596,7 @@ static int fsi_master_gpio_term(struct fsi_master *_master,mutex_lock (&master->cmd_lock);build_term_command (&cmd, id); rc =fsi_master_gpio_xfer (master, id, &cmd,0 ,NULL ); +last_address_update (master, id, false,0 );mutex_unlock (&master->cmd_lock);return rc;
@@ -552,6 +630,7 @@ static int fsi_master_gpio_break(struct fsi_master *_master, int link)clock_toggle (master,FSI_POST_BREAK_CLOCKS );spin_unlock_irqrestore (&master->bit_lock, flags); +last_address_update (master,0 , false,0 );mutex_unlock (&master->cmd_lock);/* Wait for logic reset to take effect */
@@ -662,6 +741,7 @@ static int fsi_master_gpio_probe(struct platform_device *pdev) master->dev = &pdev->dev; master->master.dev.parent = master->dev; master->master.dev.of_node =of_node_get (dev_of_node (master->dev)); + master->last_addr =LAST_ADDR_INVALID ; gpio =devm_gpiod_get (&pdev->dev,"clock" ,0 );if (IS_ERR (gpio)) {
Signature · 1 line
2.17.0
PATCH 11/15 fsi/fsi-master-gpio: Implement CRC error recovery
In reply to Benjamin HerrenschmidtThe FSI protocol defines two modes of recovery from CRC errors, this implements both:
- If the device returns an ECRC (it detected a CRC error in the command), then we simply issue the command again.
- If the master detects a CRC error in the response, we send an E_POLL command which requests a resend of the response without actually re-executing the command (which could otherwise have unwanted side effects such as dequeuing a FIFO twice).
Signed-off-by Benjamin Herrenschmidt <[email protected]>
Reviewed-by Christopher Bostic <[email protected]>
---
Note: This was actually tested by removing some of my fixes, thus causing us to hit occasional CRC errors during high LPC activity.
--- drivers/fsi/fsi-master-gpio.c | 90 ++++++++++++++++++++------ include/trace/events/fsi_master_gpio.h | 27 ++++++++ 2 files changed, 99 insertions(+), 18 deletions(-)
drivers/fsi/fsi-master-gpio.c +72 -18
@@ -22,20 +22,23 @@#define FSI_BREAK_CLOCKS 256/* Number of clocks to issue break */ #define FSI_POST_BREAK_CLOCKS 16000/* Number clocks to set up cfam */ #define FSI_INIT_CLOCKS 5000/* Clock out any old data */ +#define FSI_GPIO_DPOLL_CLOCKS 50/* < 21 will cause slave to hang */ +#define FSI_GPIO_EPOLL_CLOCKS 50/* Number of clocks for E_POLL retry */ #define FSI_GPIO_STD_DELAY 10/* Standard GPIO delay in nS */ /* todo: adjust down as low as */ /* possible or eliminate */ +#define FSI_CRC_ERR_RETRIES 10 +#define FSI_GPIO_CMD_DPOLL 0x2 +#define FSI_GPIO_CMD_EPOLL 0x3#define FSI_GPIO_CMD_TERM 0x3f#define FSI_GPIO_CMD_ABS_AR 0x4#define FSI_GPIO_CMD_REL_AR 0x5#define FSI_GPIO_CMD_SAME_AR 0x3/* but only a 2-bit opcode... */ - -#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 */ +/* Slave responses */ +#define FSI_GPIO_RESP_ACK 0/* Success */ +#define FSI_GPIO_RESP_BUSY 1/* Slave busy */ #define FSI_GPIO_RESP_ERRA 2/* Any (misc) Error */ #define FSI_GPIO_RESP_ERRC 3/* Slave reports master CRC error */ #define FSI_GPIO_MTOE 4/* Master time out error */
@@ -330,6 +333,16 @@ static void build_dpoll_command(struct fsi_gpio_msg *cmd, uint8_t slave_id)msg_push_crc (cmd); } +static void build_epoll_command (struct fsi_gpio_msg *cmd,uint8_t slave_id) +{ + cmd->bits =0 ; + cmd->msg =0 ; + +msg_push_bits (cmd, slave_id,2 ); +msg_push_bits (cmd,FSI_GPIO_CMD_EPOLL ,3 ); +msg_push_crc (cmd); +} +static void echo_delay (struct fsi_master_gpio *master) {set_sda_output (master,1 );
@@ -355,6 +368,12 @@ static void fsi_master_gpio_error(struct fsi_master_gpio *master, int error) } +/* +* Note: callers rely specifically on this returning -EAGAIN for +* a CRC error detected in the response. Use other error code +* for other situations. It will be converted to something else +* higher up the stack before it reaches userspace. +*/ static int read_one_response (struct fsi_master_gpio *master,uint8_t data_size,struct fsi_gpio_msg *msgp,uint8_t *tagp) {
@@ -379,7 +398,7 @@ static int read_one_response(struct fsi_master_gpio *master, "Master time out waiting for response\n");fsi_master_gpio_error (master,FSI_GPIO_MTOE );spin_unlock_irqrestore (&master->bit_lock, flags); -return -EIO ; +return -ETIMEDOUT ; } msg.bits =0 ;
@@ -405,7 +424,7 @@ static int read_one_response(struct fsi_master_gpio *master,if (crc) {dev_dbg (master->dev,"ERR response CRC\n" );fsi_master_gpio_error (master,FSI_GPIO_CRC_INVAL ); -return -EIO ; +return -EAGAIN ; }if (msgp)
@@ -451,11 +470,33 @@ static int poll_for_response(struct fsi_master_gpio *master,unsigned long flags;uint8_t tag;uint8_t *data_byte = data; - +int crc_err_retries =0 ; retry: rc =read_one_response (master, size, &response, &tag); -if (rc) -return rc; + +/* Handle retries on CRC errors */ +if (rc == -EAGAIN ) { +/* Too many retries ? */ +if (crc_err_retries++ >FSI_CRC_ERR_RETRIES ) { +/* +* Pass it up as a -EIO otherwise upper level will retry +* the whole command which isn't what we want here. +*/ + rc = -EIO ; + goto fail; + } +dev_dbg (master->dev, +"CRC error retry %d\n" , crc_err_retries); +trace_fsi_master_gpio_crc_rsp_error (master); +build_epoll_command (&cmd, slave); +spin_lock_irqsave (&master->bit_lock, flags); +clock_zeros (master,FSI_GPIO_EPOLL_CLOCKS ); +serial_out (master, &cmd); +echo_delay (master); +spin_unlock_irqrestore (&master->bit_lock, flags); + goto retry; + }else if (rc) + goto fail;switch (tag) {case FSI_GPIO_RESP_ACK :
@@ -496,18 +537,21 @@ static int poll_for_response(struct fsi_master_gpio *master,break ;case FSI_GPIO_RESP_ERRA : -case FSI_GPIO_RESP_ERRC : -dev_dbg (master->dev,"ERR%c received: 0x%x\n" , - tag ==FSI_GPIO_RESP_ERRA ?'A' :'C' , - (int )response.msg); +dev_dbg (master->dev,"ERRA received: 0x%x\n" , (int )response.msg);fsi_master_gpio_error (master, response.msg); rc = -EIO ;break ; +case FSI_GPIO_RESP_ERRC : +dev_dbg (master->dev,"ERRC received: 0x%x\n" , (int )response.msg); +fsi_master_gpio_error (master, response.msg); +trace_fsi_master_gpio_crc_cmd_error (master); + rc = -EAGAIN ; +break ; }if (busy_count >0 )trace_fsi_master_gpio_poll_response_busy (master, busy_count); - + fail:/* Clock the slave enough to be ready for next operation */ spin_lock_irqsave (&master->bit_lock, flags);clock_zeros (master,FSI_GPIO_PRIME_SLAVE_CLOCKS );
@@ -536,11 +580,21 @@ static int send_request(struct fsi_master_gpio *master,static int fsi_master_gpio_xfer (struct fsi_master_gpio *master,uint8_t slave,struct fsi_gpio_msg *cmd,size_t resp_len,void *resp) { -int rc; +int rc = -EAGAIN , retries =0 ; - rc =send_request (master, cmd); -if (!rc) +while ((retries++) <FSI_CRC_ERR_RETRIES ) { + rc =send_request (master, cmd); +if (rc) +break ; rc =poll_for_response (master, slave, resp_len, resp); +if (rc != -EAGAIN ) +break ; + rc = -EIO ; +dev_warn (master->dev,"ECRC retry %d\n" , retries); + +/* Pace it a bit before retry */ +msleep (1 ); + }return rc; }
include/trace/events/fsi_master_gpio.h +27 -0
@@ -64,6 +64,33 @@ TRACE_EVENT(fsi_master_gpio_break, ) ); +TRACE_EVENT (fsi_master_gpio_crc_cmd_error, + TP_PROTO(const struct fsi_master_gpio *master), +TP_ARGS (master), +TP_STRUCT__entry ( +__field (int, master_idx) + ), +TP_fast_assign ( + __entry->master_idx = master->master.idx; + ), +TP_printk ("fsi-gpio%d ----CRC command retry---" , + __entry->master_idx + ) +); + +TRACE_EVENT (fsi_master_gpio_crc_rsp_error, + TP_PROTO(const struct fsi_master_gpio *master), +TP_ARGS (master), +TP_STRUCT__entry ( +__field (int, master_idx) + ), +TP_fast_assign ( + __entry->master_idx = master->master.idx; + ), +TP_printk ("fsi-gpio%d ----CRC response---" , + __entry->master_idx + ) +); TRACE_EVENT(fsi_master_gpio_poll_response_busy, TP_PROTO(const struct fsi_master_gpio *master, int busy),
Signature · 1 line
2.17.0
PATCH 12/15 fsi/fsi-master-gpio: More error handling cleanup
In reply to Benjamin HerrenschmidtRemove calls to the empty and useless fsi_master_gpio_error() function, and report CRC errors as "FSI_ERR_NO_SLAVE" when reading an all 1's response.
Signed-off-by Benjamin Herrenschmidt <[email protected]>
--- drivers/fsi/fsi-master-gpio.c | 26 +++++--------------------- 1 file changed, 5 insertions(+), 21 deletions(-)
drivers/fsi/fsi-master-gpio.c +5 -21
@@ -41,13 +41,6 @@#define FSI_GPIO_RESP_BUSY 1/* Slave busy */ #define FSI_GPIO_RESP_ERRA 2/* Any (misc) Error */ #define FSI_GPIO_RESP_ERRC 3/* Slave reports master CRC error */ -#define FSI_GPIO_MTOE 4/* Master time out error */ -#define FSI_GPIO_CRC_INVAL 5/* Master reports slave CRC error */ - -/* Normal slave responses */ -#define FSI_GPIO_RESP_BUSY 1 -#define FSI_GPIO_RESP_ACK 0 -#define FSI_GPIO_RESP_ACKD 4#define FSI_GPIO_MAX_BUSY 200#define FSI_GPIO_MTOE_COUNT 1000
@@ -359,15 +352,6 @@ static void build_term_command(struct fsi_gpio_msg *cmd, uint8_t slave_id)msg_push_crc (cmd); } -/* -* Store information on master errors so handler can detect and clean -* up the bus -*/ -static void fsi_master_gpio_error (struct fsi_master_gpio *master,int error) -{ - -} -/* * Note: callers rely specifically on this returning -EAGAIN for * a CRC error detected in the response. Use other error code
@@ -396,7 +380,6 @@ static int read_one_response(struct fsi_master_gpio *master,if (i ==FSI_GPIO_MTOE_COUNT ) {dev_dbg (master->dev,"Master time out waiting for response\n" ); -fsi_master_gpio_error (master,FSI_GPIO_MTOE );spin_unlock_irqrestore (&master->bit_lock, flags);return -ETIMEDOUT ; }
@@ -422,8 +405,11 @@ static int read_one_response(struct fsi_master_gpio *master, crc =crc4 (0 ,1 ,1 ); crc =crc4 (crc, msg.msg, msg.bits);if (crc) { -dev_dbg (master->dev,"ERR response CRC\n" ); -fsi_master_gpio_error (master,FSI_GPIO_CRC_INVAL ); +/* Check if it's all 1's, that probably means the host is off */ +if (((~msg.msg) & ((1ull << msg.bits) -1 )) ==0 ) +return -ENODEV ; +dev_dbg (master->dev,"ERR response CRC msg: 0x%016llx (%d bits)\n" , + msg.msg, msg.bits);return -EAGAIN ; }
@@ -538,12 +524,10 @@ static int poll_for_response(struct fsi_master_gpio *master,case FSI_GPIO_RESP_ERRA :dev_dbg (master->dev,"ERRA received: 0x%x\n" , (int )response.msg); -fsi_master_gpio_error (master, response.msg); rc = -EIO ;break ;case FSI_GPIO_RESP_ERRC :dev_dbg (master->dev,"ERRC received: 0x%x\n" , (int )response.msg); -fsi_master_gpio_error (master, response.msg);trace_fsi_master_gpio_crc_cmd_error (master); rc = -EAGAIN ;break ;
Signature · 1 line
2.17.0
PATCH 13/15 fsi/master-gpio: Replace bit_bit lock with IRQ disable/enable
In reply to Benjamin HerrenschmidtFrom: Jeremy Kerr <[email protected]>
We currently use a spinlock (bit_lock) around operations that clock bits out of the FSI bus, and a mutex to protect against simultaneous access to the master.
This means that bit_lock isn't needed for mutual exlusion, only to prevent timing issues when clocking bits out.
To reflect this, this change converts bit_lock to just the local_irq_save/restore operation.
Signed-off-by Jeremy Kerr <[email protected]>
Signed-off-by Benjamin Herrenschmidt <[email protected]>
--- drivers/fsi/fsi-master-gpio.c | 48 +++++++++++++++++------------------ 1 file changed, 23 insertions(+), 25 deletions(-)
drivers/fsi/fsi-master-gpio.c +23 -25
@@ -8,11 +8,11 @@#include <linux/fsi.h> #include <linux/gpio/consumer.h> #include <linux/io.h> +#include <linux/irqflags.h> #include <linux/module.h> #include <linux/of.h> #include <linux/platform_device.h> #include <linux/slab.h> -#include <linux/spinlock.h> #include "fsi-master.h"
@@ -56,7 +56,6 @@ struct fsi_master_gpio {struct fsi_master master;struct device *dev;struct mutex cmd_lock;/* mutex for command ordering */ -spinlock_t bit_lock;/* lock for clocking bits out */ struct gpio_desc *gpio_clk;struct gpio_desc *gpio_data;struct gpio_desc *gpio_trans;/* Voltage translator */
@@ -367,7 +366,7 @@ static int read_one_response(struct fsi_master_gpio *master,uint8_t tag;int i; -spin_lock_irqsave (&master->bit_lock, flags); +local_irq_save (flags);/* wait for the start bit */ for (i =0 ; i <FSI_GPIO_MTOE_COUNT ; i++) {
@@ -380,7 +379,7 @@ static int read_one_response(struct fsi_master_gpio *master,if (i ==FSI_GPIO_MTOE_COUNT ) {dev_dbg (master->dev,"Master time out waiting for response\n" ); -spin_unlock_irqrestore (&master->bit_lock, flags); +local_irq_restore (flags);return -ETIMEDOUT ; }
@@ -399,7 +398,7 @@ static int read_one_response(struct fsi_master_gpio *master,/* read CRC */ serial_in (master, &msg,FSI_GPIO_CRC_SIZE ); -spin_unlock_irqrestore (&master->bit_lock, flags); +local_irq_restore (flags);/* we have a whole message now; check CRC */ crc =crc4 (0 ,1 ,1 );
@@ -430,10 +429,10 @@ static int issue_term(struct fsi_master_gpio *master, uint8_t slave)build_term_command (&cmd, slave); -spin_lock_irqsave (&master->bit_lock, flags); +local_irq_save (flags);serial_out (master, &cmd);echo_delay (master); -spin_unlock_irqrestore (&master->bit_lock, flags); +local_irq_restore (flags); rc =read_one_response (master,0 ,NULL , &tag);if (rc <0 ) {
@@ -475,11 +474,11 @@ static int poll_for_response(struct fsi_master_gpio *master, "CRC error retry %d\n", crc_err_retries);trace_fsi_master_gpio_crc_rsp_error (master);build_epoll_command (&cmd, slave); -spin_lock_irqsave (&master->bit_lock, flags); +local_irq_save (flags);clock_zeros (master,FSI_GPIO_EPOLL_CLOCKS );serial_out (master, &cmd);echo_delay (master); -spin_unlock_irqrestore (&master->bit_lock, flags); +local_irq_restore (flags); goto retry; }else if (rc) goto fail;
@@ -506,18 +505,18 @@ static int poll_for_response(struct fsi_master_gpio *master,*/ if (busy_count++ <FSI_GPIO_MAX_BUSY ) {build_dpoll_command (&cmd, slave); -spin_lock_irqsave (&master->bit_lock, flags); +local_irq_save (flags);clock_zeros (master,FSI_GPIO_DPOLL_CLOCKS );serial_out (master, &cmd);echo_delay (master); -spin_unlock_irqrestore (&master->bit_lock, flags); +local_irq_restore (flags); goto retry; }dev_warn (master->dev,"ERR slave is stuck in busy state, issuing TERM\n" ); -spin_lock_irqsave (&master->bit_lock, flags); +local_irq_save (flags);clock_zeros (master,FSI_GPIO_DPOLL_CLOCKS ); -spin_unlock_irqrestore (&master->bit_lock, flags); +local_irq_restore (flags);issue_term (master, slave); rc = -EIO ;break ;
@@ -537,9 +536,10 @@ static int poll_for_response(struct fsi_master_gpio *master,trace_fsi_master_gpio_poll_response_busy (master, busy_count); fail:/* Clock the slave enough to be ready for next operation */ -spin_lock_irqsave (&master->bit_lock, flags); +local_irq_save (flags);clock_zeros (master,FSI_GPIO_PRIME_SLAVE_CLOCKS ); -spin_unlock_irqrestore (&master->bit_lock, flags); +local_irq_restore (flags); +return rc; }
@@ -548,15 +548,13 @@ static int send_request(struct fsi_master_gpio *master, {unsigned long flags; -spin_lock_irqsave (&master->bit_lock, flags); -if (master->external_mode) { -spin_unlock_irqrestore (&master->bit_lock, flags); +if (master->external_mode)return -EBUSY ; - } +local_irq_save (flags);serial_out (master, cmd);echo_delay (master); -spin_unlock_irqrestore (&master->bit_lock, flags); +local_irq_restore (flags);return 0 ; }
@@ -656,7 +654,7 @@ static int fsi_master_gpio_break(struct fsi_master *_master, int link)return -EBUSY ; } -spin_lock_irqsave (&master->bit_lock, flags); +local_irq_save (flags);set_sda_output (master,1 );sda_out (master,1 );
@@ -667,7 +665,8 @@ static int fsi_master_gpio_break(struct fsi_master *_master, int link)sda_out (master,1 );clock_toggle (master,FSI_POST_BREAK_CLOCKS ); -spin_unlock_irqrestore (&master->bit_lock, flags); +local_irq_restore (flags); +last_address_update (master,0 , false,0 );mutex_unlock (&master->cmd_lock);
@@ -688,9 +687,9 @@ static void fsi_master_gpio_init(struct fsi_master_gpio *master)gpiod_direction_output (master->gpio_data,1 );/* todo: evaluate if clocks can be reduced */ -spin_lock_irqsave (&master->bit_lock, flags); +local_irq_save (flags);clock_zeros (master,FSI_INIT_CLOCKS ); -spin_unlock_irqrestore (&master->bit_lock, flags); +local_irq_restore (flags); }static void fsi_master_gpio_init_external (struct fsi_master_gpio *master)
@@ -832,7 +831,6 @@ static int fsi_master_gpio_probe(struct platform_device *pdev) master->master.send_break = fsi_master_gpio_break; master->master.link_enable = fsi_master_gpio_link_enable;platform_set_drvdata (pdev, master); -spin_lock_init (&master->bit_lock);mutex_init (&master->cmd_lock);fsi_master_gpio_init (master);
Signature · 1 line
2.17.0
PATCH 14/15 fsi: scom: Remove PIB reset during probe
In reply to Benjamin HerrenschmidtFrom: Eddie James <[email protected]>
The PIB reset causes problems for the running P9 chip. The reset shouldn't be performed by this driver.
Signed-off-by Eddie James <[email protected]>
Reviewed-by Christopher Bostic <[email protected]>
Signed-off-by Benjamin Herrenschmidt <[email protected]>
--- drivers/fsi/fsi-scom.c | 8 -------- 1 file changed, 8 deletions(-)
drivers/fsi/fsi-scom.c +0 -8
@@ -26,15 +26,11 @@#define FSI_ENGID_SCOM 0x5 -#define SCOM_FSI2PIB_DELAY 50 -/* SCOM engine register set */ #define SCOM_DATA0_REG 0x00#define SCOM_DATA1_REG 0x04#define SCOM_CMD_REG 0x08 -#define SCOM_RESET_REG 0x1C -#define SCOM_RESET_CMD 0x80000000#define SCOM_WRITE_CMD 0x80000000struct scom_device {
@@ -180,7 +176,6 @@ static const struct file_operations scom_fops = {static int scom_probe (struct device *dev) { -uint32_t data;struct fsi_device *fsi_dev =to_fsi_dev (dev);struct scom_device *scom;
@@ -197,9 +192,6 @@ static int scom_probe(struct device *dev) scom->mdev.parent = dev;list_add (&scom->link, &scom_devices); - data =cpu_to_be32 (SCOM_RESET_CMD ); -fsi_device_write (fsi_dev,SCOM_RESET_REG , &data,sizeof (uint32_t )); -return misc_register (&scom->mdev); }
Signature · 1 line
2.17.0
PATCH 15/15 fsi/sbefifo: Add driver for the SBE FIFO
In reply to Benjamin HerrenschmidtThis driver provides an in-kernel and a user API for accessing the command FIFO of the SBE (Self Boot Engine) of the POWER9 processor, via the FSI bus.
It provides an in-kernel interface to submit command and receive responses, along with a helper to locate and analyse the response status block. It's a simple synchronous submit() type API.
The user interface uses the write/read interface that an earlier version of this driver already provided, however it has some specific limitations in order to keep the driver simple and avoid using up a lot of kernel memory:
- The user should perform a single write() with the command and a single read() to get the response (with a buffer big enough to hold the entire response).
- On a write() the command is simply "stored" into a kernel buffer, it is submitted as one operation on the subsequent read(). This allows to have the code write directly from the FIFO into the user buffer and avoid hogging the SBE between the write() and read() syscall as it's critical that the SBE be freed asap to respond to the host. An extra write() will simply replace the previously written command.
- A write of a single 4 bytes containing the value 0x52534554 in big endian will trigger a reset request. No read is necessary, the write() call will return when the reset has been acknowledged or times out.
- The command is limited to 4K bytes.
Signed-off-by Benjamin Herrenschmidt <[email protected]>
---
--- drivers/fsi/Kconfig | 7 + drivers/fsi/Makefile | 1 + drivers/fsi/fsi-sbefifo.c | 1005 +++++++++++++++++++++++++++++++++++ include/linux/fsi-sbefifo.h | 33 ++ 4 files changed, 1046 insertions(+) create mode 100644 drivers/fsi/fsi-sbefifo.c create mode 100644 include/linux/fsi-sbefifo.h
drivers/fsi/Kconfig +7 -0
@@ -32,4 +32,11 @@ config FSI_SCOM ---help--- This option enables an FSI based SCOM device driver. +config FSI_SBEFIFO + tristate "SBEFIFO FSI client device driver" + ---help--- + This option enables an FSI based SBEFIFO device driver. The SBEFIFO is + a pipe-like FSI device for communicating with the self boot engine + (SBE) on POWER processors. + endif
drivers/fsi/Makefile +1 -0
@@ -3,3 +3,4 @@ obj-$(CONFIG_FSI) += fsi-core.o obj-$(CONFIG_FSI_MASTER_HUB) += fsi-master-hub.o obj-$(CONFIG_FSI_MASTER_GPIO) += fsi-master-gpio.o obj-$(CONFIG_FSI_SCOM) += fsi-scom.o +obj-$(CONFIG_FSI_SBEFIFO) += fsi-sbefifo.o
drivers/fsi/fsi-sbefifo.c new file +1005 -0
1,005 lines not on this page · raw message
include/linux/fsi-sbefifo.h new file +33 -0
@@ -0,0 +1,33 @@ +/* +* SBEFIFO FSI Client device driver +* +* Copyright (C) IBM Corporation 2017 +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License version 2 as +* published by the Free Software Foundation. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERGCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +*/ + +#ifndef LINUX_FSI_SBEFIFO_H +#define LINUX_FSI_SBEFIFO_H + +#define SBEFIFO_CMD_PUT_OCC_SRAM 0xa404 +#define SBEFIFO_CMD_GET_OCC_SRAM 0xa403 +#define SBEFIFO_CMD_GET_SBE_FFDC 0xa801 + +#define SBEFIFO_MAX_FFDC_SIZE 0x2000 + +struct device ; + +int sbefifo_submit (struct device *dev,const __be32 *command,size_t cmd_len, +__be32 *response,size_t *resp_len); + +int sbefifo_parse_status (struct device *dev,u16 cmd,__be32 *response, +size_t resp_len,size_t *data_len); + +#endif /* LINUX_FSI_SBEFIFO_H */
Signature · 1 line
2.17.0
Quoted 4 lines · Benjamin Herrenschmidt
This series brings in a number of improvements to our FSI stack (one of the service interfaces for communicating between a BMC chip and our POWER processors).
Quoted 8 lines
The GPIO based "Soft FSI" performance is significantly improved, and it's reliability as well.
The SBE fifo driver provides the interface to the processor "Self Boot Engine"
Some of these patches have been simmering for a while in the OpenBMC tree.
I run this series atop of 4.17-rc7 today, and they look solid.
Tested-by Joel Stanley <[email protected]>
Cheers,
Joel
(Greg, see below)
Quoted 12 lines · Joel Stanley
Quoted 10 lines · Benjamin Herrenschmidt
This series brings in a number of improvements to our FSI stack (one of the service interfaces for communicating between a BMC chip and our POWER processors).
The GPIO based "Soft FSI" performance is significantly improved, and it's reliability as well.
The SBE fifo driver provides the interface to the processor "Self Boot Engine"
Quoted 6 lines
Quoted 2 lines
Some of these patches have been simmering for a while in the OpenBMC tree.
I run this series atop of 4.17-rc7 today, and they look solid.
Tested-by Joel Stanley <[email protected]>
So how do we proceed ? I have more coming on top of this, but we should start with getting this series merged.
Greg, should we create a FSI git repo with a 3-member maintainer team (Jeremy, Chris and myself) and send you pull requests ? Or are you happy to continue picking up patches ?
We have more stuff to come in there, including support for a HW master contoller in a future BMC chip, some more slave drivers, etc..
It also looks like the s390 guys might start using some of that as well (their chips use FSI as well, so far they used our old service processor which uses an ancient non-uptream set of drivers & stack, but that's likely to change).
Cheers,
Ben.
Quoted 3 lines
Cheers,
Joel
Quoted 20 lines · Benjamin Herrenschmidt
(Greg, see below)
Quoted 17 lines · Joel Stanley
Quoted 4 lines · Benjamin Herrenschmidt
This series brings in a number of improvements to our FSI stack (one of the service interfaces for communicating between a BMC chip and our POWER processors).
Quoted 8 lines
The GPIO based "Soft FSI" performance is significantly improved, and it's reliability as well.
The SBE fifo driver provides the interface to the processor "Self Boot Engine"
Some of these patches have been simmering for a while in the OpenBMC tree.
I run this series atop of 4.17-rc7 today, and they look solid.
Quoted 8 lines
Quoted 1 line
Tested-by Joel Stanley <[email protected]>So how do we proceed ? I have more coming on top of this, but we should start with getting this series merged.
Greg, should we create a FSI git repo with a 3-member maintainer team (Jeremy, Chris and myself) and send you pull requests ? Or are you happy to continue picking up patches ?
Which ever is best/easiest for you is fine with me, I can work either way just fine.
Quoted 7 lines
We have more stuff to come in there, including support for a HW master contoller in a future BMC chip, some more slave drivers, etc..
It also looks like the s390 guys might start using some of that as well (their chips use FSI as well, so far they used our old service processor which uses an ancient non-uptream set of drivers & stack, but that's likely to change).
Then maybe a git tree is easiest for all of you to work against?
thanks,
greg k-h