| Message ID | 1476918586-13475-16-git-send-email-christopher.lee.bostic@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
Hi Chris, > diff --git a/Documentation/devicetree/bindings/fsi/fsi-master-gpio.txt b/Documentation/devicetree/bindings/fsi/fsi-master-gpio.txt > new file mode 100644 > index 0000000..4cb2c81 > --- /dev/null > +++ b/Documentation/devicetree/bindings/fsi/fsi-master-gpio.txt > @@ -0,0 +1,21 @@ > +Device-tree bindings for gpio-based FSI master driver > +----------------------------------------------------- > + > +Required properties: > + - compatible = "ibm,fsi-master-gpio"; > + - clk-gpio; > + - data-gpio; > + > +Optional properties: > + - enable-gpio; > + - trans-gpio; > + - mux-gpio; > + > +fsi-master { > + compatible = "ibm,fsi-master", "ibm,fsi-master-gpio"; > + clk-gpio = <&gpio 0>; > + data-gpio = <&gpio 1>; > + enable-gpio = <&gpio 2>; > + trans-gpio = <&gpio 3>; > + mux-gpio = <&gpio 4>; > +} Looks good. Might be good to include descriptions for the enable/trans/mux lines here. We should also think about what we want to do for multiple links. There are two main options here: - list them as separate masters in the DT (ie, two fsi-master nodes, completely independent); or - enable multiple GPIO descriptors in the gpio properties, eg: fsi-master { compatible = "ibm,fsi-master", "ibm,fsi-master-gpio"; clk-gpios = <&gpio 0 &gpio 6>; data-gpios = <&gpio 1 &gpio 7>; } which describes a master with two links If we use the latter case, we'd want the property name to be plural (*-gpios) to indicate this. > diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts b/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts > index 21619fd..1875313 100644 > --- a/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts > +++ b/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts > @@ -167,6 +167,36 @@ > output-low; > line-name = "func_mode2"; > }; > + > + pin_fsi_clk { > + gpios = <ASPEED_GPIO(A, 4) GPIO_ACTIVE_HIGH>; > + output-low; > + line-name = "fsi_clk"; > + }; > + > + pin_fsi_data { > + gpios = <ASPEED_GPIO(A, 5) GPIO_ACTIVE_HIGH>; > + output-low; > + line-name = "fsi_data"; > + }; > + > + pin_fsi_trans { > + gpios = <ASPEED_GPIO(H, 6) GPIO_ACTIVE_HIGH>; > + output-low; > + line-name = "fsi_trans"; > + }; > + > + pin_fsi_enable { > + gpios = <ASPEED_GPIO(D, 0) GPIO_ACTIVE_HIGH>; > + output-low; > + line-name = "fsi_enable"; > + }; > + > + pin_fsi_mux { > + gpios = <ASPEED_GPIO(A, 6) GPIO_ACTIVE_HIGH>; > + output-low; > + line-name = "fsi_mux"; > + }; > }; Do we want to add a master node too? > +static void set_clock(struct fsi_master_gpio *master) > +{ > + ndelay(FSI_GPIO_STD_DELAY); > + gpiod_set_value(master->gpio_clk, 1); > +} > + > +static void clear_clock(struct fsi_master_gpio *master) > +{ > + ndelay(FSI_GPIO_STD_DELAY); > + gpiod_set_value(master->gpio_clk, 0); > +} > + > +static void clock_toggle(struct fsi_master_gpio *master, int count) > +{ > + int i; > + > + for (i = 0; i < count; i++) { > + clear_clock(master); > + set_clock(master); > + } > +} To me, this seems a bit awkward, since the set_clock and clear_clock have their pre-delays, but are only used in one context (clock_toggle). Perhaps this would be cleaner just doing the gpiod_set_value()s and delays in clock_toggle would be cleaner? > + > +static int sda_in(struct fsi_master_gpio *master) > +{ > + return gpiod_get_value(master->gpio_data); > +} > + > +static void sda_out(struct fsi_master_gpio *master, int value) > +{ > + gpiod_set_value(master->gpio_data, value); > + ndelay(FSI_GPIO_STD_DELAY); > +} So an output bit that changes sda will take twice as long? Won't the delays in clock_toggle handle this? > + > +static void set_sda_input(struct fsi_master_gpio *master) > +{ > + gpiod_direction_input(master->gpio_data); > + if (master->gpio_trans) > + gpiod_direction_output(master->gpio_trans, 0); > +} > +static void set_sda_output(struct fsi_master_gpio *master, > + int value) > +{ > + gpiod_direction_output(master->gpio_data, value); > + if (master->gpio_trans) > + gpiod_direction_output(master->gpio_trans, 1); > +} Isn't trans always an output? Shouldn't we set the direction to output once during init, and do gpiod_set_value() here? > + > +static void serial_in(struct fsi_master_gpio *master, struct fsi_gpio_msg *cmd, > + uint8_t num_bits) > +{ > + uint8_t bit; > + uint64_t msg = 0; > + uint8_t in_bit = 0; > + > + set_sda_input(master); > + cmd->bits = 0; > + > + for (bit = 0; bit < num_bits; bit++) { > + clock_toggle(master, 1); > + in_bit = sda_in(master); > + cmd->bits++; > + msg <<= 1; > + msg |= ~in_bit & 0x1; /* Data is negative active */ > + } > + cmd->msg = msg; > +} You can shortcut all of the increments of ->num_bits (and initialisation to zero) by just setting it to num_bits (probably after the for-loop, where you set ->msg); > +static int poll_for_response(struct fsi_master_gpio *master, uint8_t expected, > + uint8_t size, void *data) > +{ > + int busy_count = 0, i; > + struct fsi_gpio_msg response, cmd; > + int bits_remaining = 0; > + uint64_t resp = 0; > + uint8_t bits_received = 1 + FSI_GPIO_MSG_ID_SIZE + > + FSI_GPIO_MSG_RESPID_SIZE; > + uint8_t crc_in; > + > + do { > + for (i = 0; i < FSI_GPIO_MTOE_COUNT; i++) { > + serial_in(master, &response, 1); > + resp = response.msg; > + if (response.msg) > + break; > + } > + if (i >= FSI_GPIO_MTOE_COUNT) { > + dev_info(master->master.dev, > + "Master time out waiting for response\n"); > + drain_response(master); > + fsi_master_gpio_error(master, FSI_GPIO_MTOE); > + return FSI_GPIO_MTOE; > + } > + > + /* Response received */ > + serial_in(master, &response, FSI_GPIO_MSG_ID_SIZE); > + dev_info(master->master.dev, "cfam id:%d\n", (int)response.msg); > + resp <<= FSI_GPIO_MSG_ID_SIZE; > + resp |= response.msg; > + > + serial_in(master, &response, FSI_GPIO_MSG_RESPID_SIZE); > + dev_info(master->master.dev, "response id:%d\n", > + (int)response.msg); > + resp <<= FSI_GPIO_MSG_RESPID_SIZE; > + resp |= response.msg; Those dev_info() calls are going to make things very noisy. For printouts in non-error paths, use dev_dbg(). However, you have two serial_in() calls in the single path, can you consolidate these? > + > + switch (response.msg) { > + case FSI_GPIO_RESP_ACK: > + if (expected == FSI_GPIO_RESP_ACKD) > + bits_remaining = 8 * size; > + break; > + > + case FSI_GPIO_RESP_BUSY: > + /* > + * Its necessary to clock slave before issuing > + * d-poll, not indicated in the hardware protocol > + * spec. < 20 clocks causes slave to hang, 21 ok. > + */ > + set_sda_output(master, 0); > + clock_toggle(master, FSI_GPIO_DPOLL_CLOCKS); > + cmd.msg = FSI_GPIO_CMD_DPOLL; > + cmd.bits = FSI_GPIO_CMD_DPOLL_SIZE; > + serial_out(master, &cmd); > + continue; > + > + case FSI_GPIO_RESP_ERRA: > + case FSI_GPIO_RESP_ERRC: > + dev_info(master->master.dev, "ERR received: %d\n", > + (int)response.msg); > + drain_response(master); > + fsi_master_gpio_error(master, response.msg); > + return response.msg; > + } > + > + /* Read in the data field if applicable */ > + if (bits_remaining) { > + serial_in(master, &response, bits_remaining); > + resp <<= bits_remaining; > + resp |= response.msg; > + bits_received += bits_remaining; > + } > + > + /* Read in the crc and check it */ > + serial_in(master, &response, FSI_GPIO_CRC_SIZE); > + > + crc_in = fsi_crc4(0, 1, 1); > + crc_in = fsi_crc4(crc_in, resp, bits_received); > + if (crc_in != response.msg) { > + /* CRC's don't match */ > + dev_info(master->master.dev, "ERR response CRC\n"); > + fsi_master_gpio_error(master, FSI_GPIO_CRC_INVAL); > + return FSI_GPIO_CRC_INVAL; > + } > + return 0; > + > + } while (busy_count++ < FSI_GPIO_MAX_BUSY); > + > + return FSI_GPIO_MAX_BUSY; > +} While you've explained the return values to me, it isn't obvious from the code. From reading the poll_response: - we return 0 on success - we return 1 if we don't see a start bit (what's MTOE?) - we return 100 if we got 100 busy responses (after dpoll) - we return 2 or 3 if there was a checksum error on the slave - we return 5 if there was a checksum error on the master Now, this *may* be okay if we were interpreting the return values within this one file (although we'd want to change busy into something more in line with the others), but this is returned directly to the fsi core through the read & write callbacks. Without any explicit definition of the return values, we'd expect a standard kernel scheme (0 on success, negative errno (eg -EIO) on failure), particularly on the master->read/write interface. I assume you're going to handle the various return values as part of the future error recovery changes, but we should at least (at this stage) compress this into -EIO from the read/write callbacks (possibly in the actual read/write implementations). In future, when we have conditions that we can potentially recover from, I'd suggest using an enum to represent the full set of errors, and define this function to return that type. That allows us to describe the possible set of errors to future developers, and allows the compiler to warn us if we're not handling a particular case. Until the error recovery code is added, we won't know what the best strategy is - but in the meantime, we should stick to the zero/negative-errno convention. > +static int fsi_master_gpio_probe(struct platform_device *pdev) > +{ > + struct fsi_master_gpio *master; > + struct gpio_desc *gpio; > + > + master = devm_kzalloc(&pdev->dev, sizeof(*master), GFP_KERNEL); > + if (!master) > + return -ENOMEM; > + > + gpio = devm_gpiod_get(&pdev->dev, "fsi_clk", 0); You've changed these unnecessarily; this name string is used to look up the device tree property, not the pinctl descriptor. Cheers, Jeremy
On Wed, Oct 19, 2016 at 8:26 PM, Jeremy Kerr <jk@ozlabs.org> wrote: > Hi Chris, > >> diff --git a/Documentation/devicetree/bindings/fsi/fsi-master-gpio.txt b/Documentation/devicetree/bindings/fsi/fsi-master-gpio.txt >> new file mode 100644 >> index 0000000..4cb2c81 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/fsi/fsi-master-gpio.txt >> @@ -0,0 +1,21 @@ >> +Device-tree bindings for gpio-based FSI master driver >> +----------------------------------------------------- >> + >> +Required properties: >> + - compatible = "ibm,fsi-master-gpio"; >> + - clk-gpio; >> + - data-gpio; >> + >> +Optional properties: >> + - enable-gpio; >> + - trans-gpio; >> + - mux-gpio; >> + >> +fsi-master { >> + compatible = "ibm,fsi-master", "ibm,fsi-master-gpio"; >> + clk-gpio = <&gpio 0>; >> + data-gpio = <&gpio 1>; >> + enable-gpio = <&gpio 2>; >> + trans-gpio = <&gpio 3>; >> + mux-gpio = <&gpio 4>; >> +} > > Looks good. Might be good to include descriptions for the > enable/trans/mux lines here. > Hi Jeremy, OK, will add that. > We should also think about what we want to do for multiple links. There > are two main options here: > > - list them as separate masters in the DT (ie, two fsi-master nodes, > completely independent); or > > - enable multiple GPIO descriptors in the gpio properties, eg: > > fsi-master { > compatible = "ibm,fsi-master", "ibm,fsi-master-gpio"; > clk-gpios = <&gpio 0 &gpio 6>; > data-gpios = <&gpio 1 &gpio 7>; > } > > which describes a master with two links > > If we use the latter case, we'd want the property name to be plural > (*-gpios) to indicate this. Its difficult to say how many links we'll eventually need a priori. This assumes two links but it could eventually be way more than that. To be extensible and prevent us from having to modify the device tree later I'd lean towards the idea of having separate masters where each owns its own single FSI link. We're still left with the issue of guessing how many we'll eventually need. How problematic is it to define one gpio master now and then add others to the device tree later? If we need to do it now we'll have to pick a number out of a hat. > >> diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts b/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts >> index 21619fd..1875313 100644 >> --- a/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts >> +++ b/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts >> @@ -167,6 +167,36 @@ >> output-low; >> line-name = "func_mode2"; >> }; >> + >> + pin_fsi_clk { >> + gpios = <ASPEED_GPIO(A, 4) GPIO_ACTIVE_HIGH>; >> + output-low; >> + line-name = "fsi_clk"; >> + }; >> + >> + pin_fsi_data { >> + gpios = <ASPEED_GPIO(A, 5) GPIO_ACTIVE_HIGH>; >> + output-low; >> + line-name = "fsi_data"; >> + }; >> + >> + pin_fsi_trans { >> + gpios = <ASPEED_GPIO(H, 6) GPIO_ACTIVE_HIGH>; >> + output-low; >> + line-name = "fsi_trans"; >> + }; >> + >> + pin_fsi_enable { >> + gpios = <ASPEED_GPIO(D, 0) GPIO_ACTIVE_HIGH>; >> + output-low; >> + line-name = "fsi_enable"; >> + }; >> + >> + pin_fsi_mux { >> + gpios = <ASPEED_GPIO(A, 6) GPIO_ACTIVE_HIGH>; >> + output-low; >> + line-name = "fsi_mux"; >> + }; >> }; > > Do we want to add a master node too? > Don't follow what you mean by a master node. What does that help with? Also how would that be defined in the dev tree? The discussion above about choosing the number of masters we'll eventually need would impact this as well I assume. >> +static void set_clock(struct fsi_master_gpio *master) >> +{ >> + ndelay(FSI_GPIO_STD_DELAY); >> + gpiod_set_value(master->gpio_clk, 1); >> +} >> + >> +static void clear_clock(struct fsi_master_gpio *master) >> +{ >> + ndelay(FSI_GPIO_STD_DELAY); >> + gpiod_set_value(master->gpio_clk, 0); >> +} >> + >> +static void clock_toggle(struct fsi_master_gpio *master, int count) >> +{ >> + int i; >> + >> + for (i = 0; i < count; i++) { >> + clear_clock(master); >> + set_clock(master); >> + } >> +} > > To me, this seems a bit awkward, since the set_clock and clear_clock > have their pre-delays, but are only used in one context (clock_toggle). > Perhaps this would be cleaner just doing the gpiod_set_value()s and > delays in clock_toggle would be cleaner? OK, will change. > >> + >> +static int sda_in(struct fsi_master_gpio *master) >> +{ >> + return gpiod_get_value(master->gpio_data); >> +} >> + >> +static void sda_out(struct fsi_master_gpio *master, int value) >> +{ >> + gpiod_set_value(master->gpio_data, value); >> + ndelay(FSI_GPIO_STD_DELAY); >> +} > > So an output bit that changes sda will take twice as long? Won't the > delays in clock_toggle handle this? Yes that's true, the delays in clock_toggle would be enough I think. Will change. > >> + >> +static void set_sda_input(struct fsi_master_gpio *master) >> +{ >> + gpiod_direction_input(master->gpio_data); >> + if (master->gpio_trans) >> + gpiod_direction_output(master->gpio_trans, 0); >> +} >> +static void set_sda_output(struct fsi_master_gpio *master, >> + int value) >> +{ >> + gpiod_direction_output(master->gpio_data, value); >> + if (master->gpio_trans) >> + gpiod_direction_output(master->gpio_trans, 1); >> +} > > Isn't trans always an output? Shouldn't we set the direction to output > once during init, and do gpiod_set_value() here? > Will change. >> + >> +static void serial_in(struct fsi_master_gpio *master, struct fsi_gpio_msg *cmd, >> + uint8_t num_bits) >> +{ >> + uint8_t bit; >> + uint64_t msg = 0; >> + uint8_t in_bit = 0; >> + >> + set_sda_input(master); >> + cmd->bits = 0; >> + >> + for (bit = 0; bit < num_bits; bit++) { >> + clock_toggle(master, 1); >> + in_bit = sda_in(master); >> + cmd->bits++; >> + msg <<= 1; >> + msg |= ~in_bit & 0x1; /* Data is negative active */ >> + } >> + cmd->msg = msg; >> +} > > You can shortcut all of the increments of ->num_bits (and initialisation > to zero) by just setting it to num_bits (probably after the for-loop, > where you set ->msg); > Will change. >> +static int poll_for_response(struct fsi_master_gpio *master, uint8_t expected, >> + uint8_t size, void *data) >> +{ >> + int busy_count = 0, i; >> + struct fsi_gpio_msg response, cmd; >> + int bits_remaining = 0; >> + uint64_t resp = 0; >> + uint8_t bits_received = 1 + FSI_GPIO_MSG_ID_SIZE + >> + FSI_GPIO_MSG_RESPID_SIZE; >> + uint8_t crc_in; >> + >> + do { >> + for (i = 0; i < FSI_GPIO_MTOE_COUNT; i++) { >> + serial_in(master, &response, 1); >> + resp = response.msg; >> + if (response.msg) >> + break; >> + } >> + if (i >= FSI_GPIO_MTOE_COUNT) { >> + dev_info(master->master.dev, >> + "Master time out waiting for response\n"); >> + drain_response(master); >> + fsi_master_gpio_error(master, FSI_GPIO_MTOE); >> + return FSI_GPIO_MTOE; >> + } >> + >> + /* Response received */ >> + serial_in(master, &response, FSI_GPIO_MSG_ID_SIZE); >> + dev_info(master->master.dev, "cfam id:%d\n", (int)response.msg); >> + resp <<= FSI_GPIO_MSG_ID_SIZE; >> + resp |= response.msg; >> + >> + serial_in(master, &response, FSI_GPIO_MSG_RESPID_SIZE); >> + dev_info(master->master.dev, "response id:%d\n", >> + (int)response.msg); >> + resp <<= FSI_GPIO_MSG_RESPID_SIZE; >> + resp |= response.msg; > > Those dev_info() calls are going to make things very noisy. For > printouts in non-error paths, use dev_dbg(). > OK will change, > However, you have two serial_in() calls in the single path, can you > consolidate these? > Ok, will change. This type of change I would agree offers some minor performance gain but it does add a few lines of code to decode the slave id that weren't needed before. Since I'm making changes to this file already it makes sense to add this one. If all that remain though are minor changes like this one I'd like to be able to address those after the first core patch set is approved. Would this be acceptable? I agree with the desire to get it all right the first time but I'm also concerned with getting the openfsi code available for bringup purposes. >> + >> + switch (response.msg) { >> + case FSI_GPIO_RESP_ACK: >> + if (expected == FSI_GPIO_RESP_ACKD) >> + bits_remaining = 8 * size; >> + break; >> + >> + case FSI_GPIO_RESP_BUSY: >> + /* >> + * Its necessary to clock slave before issuing >> + * d-poll, not indicated in the hardware protocol >> + * spec. < 20 clocks causes slave to hang, 21 ok. >> + */ >> + set_sda_output(master, 0); >> + clock_toggle(master, FSI_GPIO_DPOLL_CLOCKS); >> + cmd.msg = FSI_GPIO_CMD_DPOLL; >> + cmd.bits = FSI_GPIO_CMD_DPOLL_SIZE; >> + serial_out(master, &cmd); >> + continue; >> + >> + case FSI_GPIO_RESP_ERRA: >> + case FSI_GPIO_RESP_ERRC: >> + dev_info(master->master.dev, "ERR received: %d\n", >> + (int)response.msg); >> + drain_response(master); >> + fsi_master_gpio_error(master, response.msg); >> + return response.msg; >> + } >> + >> + /* Read in the data field if applicable */ >> + if (bits_remaining) { >> + serial_in(master, &response, bits_remaining); >> + resp <<= bits_remaining; >> + resp |= response.msg; >> + bits_received += bits_remaining; >> + } >> + >> + /* Read in the crc and check it */ >> + serial_in(master, &response, FSI_GPIO_CRC_SIZE); >> + >> + crc_in = fsi_crc4(0, 1, 1); >> + crc_in = fsi_crc4(crc_in, resp, bits_received); >> + if (crc_in != response.msg) { >> + /* CRC's don't match */ >> + dev_info(master->master.dev, "ERR response CRC\n"); >> + fsi_master_gpio_error(master, FSI_GPIO_CRC_INVAL); >> + return FSI_GPIO_CRC_INVAL; >> + } >> + return 0; >> + >> + } while (busy_count++ < FSI_GPIO_MAX_BUSY); >> + >> + return FSI_GPIO_MAX_BUSY; >> +} > > While you've explained the return values to me, it isn't obvious from > the code. From reading the poll_response: > > - we return 0 on success > > - we return 1 if we don't see a start bit (what's MTOE?) > > - we return 100 if we got 100 busy responses (after dpoll) > > - we return 2 or 3 if there was a checksum error on the slave > > - we return 5 if there was a checksum error on the master > > Now, this *may* be okay if we were interpreting the return values within > this one file (although we'd want to change busy into something more in > line with the others), but this is returned directly to the fsi core > through the read & write callbacks. > > Without any explicit definition of the return values, we'd expect a > standard kernel scheme (0 on success, negative errno (eg -EIO) on > failure), particularly on the master->read/write interface. > > I assume you're going to handle the various return values as part > of the future error recovery changes, but we should at least (at this > stage) compress this into -EIO from the read/write callbacks (possibly > in the actual read/write implementations). > > In future, when we have conditions that we can potentially recover from, > I'd suggest using an enum to represent the full set of errors, and > define this function to return that type. That allows us to describe the > possible set of errors to future developers, and allows the compiler to > warn us if we're not handling a particular case. > > Until the error recovery code is added, we won't know what the best > strategy is - but in the meantime, we should stick to the > zero/negative-errno convention. I'll compress those return codes to -EIO. The specific bus error information is to only consumed by the master error handler anyway. > >> +static int fsi_master_gpio_probe(struct platform_device *pdev) >> +{ >> + struct fsi_master_gpio *master; >> + struct gpio_desc *gpio; >> + >> + master = devm_kzalloc(&pdev->dev, sizeof(*master), GFP_KERNEL); >> + if (!master) >> + return -ENOMEM; >> + >> + gpio = devm_gpiod_get(&pdev->dev, "fsi_clk", 0); > > You've changed these unnecessarily; this name string is used to look up > the device tree property, not the pinctl descriptor. > Will back that out. > Cheers, > > > Jeremy
Hi Chris, >>> diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts b/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts >>> index 21619fd..1875313 100644 >>> --- a/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts >>> +++ b/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts >>> @@ -167,6 +167,36 @@ >>> output-low; >>> line-name = "func_mode2"; >>> }; >>> + >>> + pin_fsi_clk { >>> + gpios = <ASPEED_GPIO(A, 4) GPIO_ACTIVE_HIGH>; >>> + output-low; >>> + line-name = "fsi_clk"; >>> + }; >>> + >>> + pin_fsi_data { >>> + gpios = <ASPEED_GPIO(A, 5) GPIO_ACTIVE_HIGH>; >>> + output-low; >>> + line-name = "fsi_data"; >>> + }; >>> + >>> + pin_fsi_trans { >>> + gpios = <ASPEED_GPIO(H, 6) GPIO_ACTIVE_HIGH>; >>> + output-low; >>> + line-name = "fsi_trans"; >>> + }; >>> + >>> + pin_fsi_enable { >>> + gpios = <ASPEED_GPIO(D, 0) GPIO_ACTIVE_HIGH>; >>> + output-low; >>> + line-name = "fsi_enable"; >>> + }; >>> + >>> + pin_fsi_mux { >>> + gpios = <ASPEED_GPIO(A, 6) GPIO_ACTIVE_HIGH>; >>> + output-low; >>> + line-name = "fsi_mux"; >>> + }; >>> }; >> >> Do we want to add a master node too? > > Don't follow what you mean by a master node. What does that help with? The master node is what describes the fsi master "device" (in this case, the usage of a set of GPIOs as a functional unit). The properties above just define some pinctl configuration to describe a bunch of GPIOs. This doesn't actually describe anything fsi-master related. To do that, we need the master node too; that's the binding specification that we're adding to Documentation/device-tree, and is the thing that looks like: fsi-master { compatible = "ibm,fsi-master", "ibm,fsi-master-gpio"; clk-gpios = <&gpio 0> data-gpios = <&gpio 1> }; or, for one of the fake masters for testing: fsi-master { compatible = "ibm,fsi-master", "ibm,fsi-master-fake"; }; That master node is what causes the master drivers to be "bound" to a device (and multiple masters mean multiple devices instances & probes). Does this mean you haven't yet tested with either the fake or gpio masters? If that's the case, that should be your priority. We're at the stage that any further code review would be redundant, as that code is fairly likely to need changes as a result of testing. >> We should also think about what we want to do for multiple links. There >> are two main options here: >> >> - list them as separate masters in the DT (ie, two fsi-master nodes, >> completely independent); or >> >> - enable multiple GPIO descriptors in the gpio properties, eg: >> >> fsi-master { >> compatible = "ibm,fsi-master", "ibm,fsi-master-gpio"; >> clk-gpios = <&gpio 0 &gpio 6>; >> data-gpios = <&gpio 1 &gpio 7>; >> } >> >> which describes a master with two links >> >> If we use the latter case, we'd want the property name to be plural >> (*-gpios) to indicate this. > > Its difficult to say how many links we'll eventually need a priori. We don't need to - this isn't prescribed by the binding. These *-gpios properties contain one or more "descriptors" of individual GPIOs. While the format of a descriptor depends on how the GPIO controller describes each GPIO, the notation we've used in that example is fairly common, and is: clk-gpio = <(pointer to gpio controller) (index within controller)> in this case, each gpio descriptor takes two cells (a cell is a u32), and so the GPIO controller will have a #gpio-cells = <2> property. Our vanilla master with one link would just be something like: fsi-master { compatible = "ibm,fsi-master", "ibm,fsi-master-gpio"; clk-gpios = <&gpio 0> data-gpios = <&gpio 1> } - this describes a single clock and single data line, as each *-gpios property is one descriptor. Say if we have a FSI master with two links, we would have four cells for all the *-gpios properties: fsi-master { compatible = "ibm,fsi-master", "ibm,fsi-master-gpio"; clk-gpios = <&gpio 0 &gpio 6>; data-gpios = <&gpio 1 &gpio 7>; } - and the binding would remain as is (just that we should use the plural of gpios for the property names, to make this a little more sensible). This could also be represented as two separate master nodes: fsi-master { compatible = "ibm,fsi-master", "ibm,fsi-master-gpio"; clk-gpios = <&gpio 0>; data-gpios = <&gpio 1>; } fsi-master { compatible = "ibm,fsi-master", "ibm,fsi-master-gpio"; clk-gpios = <&gpio 6>; data-gpios = <&gpio 7>; } Our choice there would depend on the hardware layout. Does is make more sense to represent this as two independent masters? Or a single one? Would there ever be any shared state between those two masters that a driver needs to handle? Once we've settled that, we will need the device tree binding people to take a look, which just involves CCing devicetree-discuss@ozlabs.org for patches that modify Documentation/devicetree/*. >> However, you have two serial_in() calls in the single path, can you >> consolidate these? >> > > Ok, will change. > > This type of change I would agree offers some minor performance > gain but it does add a few lines of code to decode the slave id that weren't > needed before. This isn't for performance gain, it's purely for readability. > Since I'm making changes to this file already it makes > sense to add this one. If all that remain though are minor changes like > this one I'd like to be able to address those after the first core patch set > is approved. Would this be acceptable? That can be a tough call. If it's a cleanup post-submission, then you've lost the opportunity to roll that cleanup into the original inclusion, and will need an extra patch for that cleanup (it's bad form to include a cleanup change in something unrelated). However, if there's minor things that don't warrant resubmission, then that should be fine - just be aware that upstream folks may request that you change it before submission *anyway*. Cheers, Jeremy
> > Say if we have a FSI master with two links, we would have four cells > for all the *-gpios properties: > > fsi-master { > compatible = "ibm,fsi-master", "ibm,fsi-master-gpio"; > clk-gpios = <&gpio 0 &gpio 6>; > data-gpios = <&gpio 1 &gpio 7>; > } > Hi Jeremy, One thing I don't understand is the numbering conventions. What does 6 and 7 represent in this case? There are five pins total 0-4 for the existing gpio master. I've followed the numbering as is listed here in the latest patch set I'm submitting this afternoon. > - and the binding would remain as is (just that we should use the plural > of gpios for the property names, to make this a little more sensible). > > This could also be represented as two separate master nodes: > > fsi-master { > compatible = "ibm,fsi-master", "ibm,fsi-master-gpio"; > clk-gpios = <&gpio 0>; > data-gpios = <&gpio 1>; > } > > fsi-master { > compatible = "ibm,fsi-master", "ibm,fsi-master-gpio"; > clk-gpios = <&gpio 6>; > data-gpios = <&gpio 7>; > } > > Our choice there would depend on the hardware layout. Does is make more > sense to represent this as two independent masters? Or a single one? > Would there ever be any shared state between those two masters that a > driver needs to handle? Up until now, all available FSI masters in hardware, FSP, P8 "hub", P8 cascade, P7 cascade, have all had multiple links controlled by one master. It would make sense I think to continue this. There is no direct sharing of information between masters other than what's required by the firmware to start at the beginning master in the chain and walk down each link and checking 'sub' master states to determine where an interrupt or error was sourced. General flow: Identify on which link interrupt / error was reported. Look at each CFAM on that link to see if it was reported locally or downstream if downstream, Look at the cascaded master's owned links to see which reported. Etc.. In my opinion that type of master to master information is better kept separated as it is in the interests of keep a general design common between real hardware FSI masters such as the FSP's and the gpio type master implementations. > > Once we've settled that, we will need the device tree binding people to > take a look, which just involves CCing devicetree-discuss@ozlabs.org for > patches that modify Documentation/devicetree/*. > Will keep them off the cc list for now until we all agree which way we go. Thanks, Chris
Hi Chris, >> Say if we have a FSI master with two links, we would have four cells >> for all the *-gpios properties: >> >> fsi-master { >> compatible = "ibm,fsi-master", "ibm,fsi-master-gpio"; >> clk-gpios = <&gpio 0 &gpio 6>; >> data-gpios = <&gpio 1 &gpio 7>; >> } >> > > Hi Jeremy, > > One thing I don't understand is the numbering conventions. What does > 6 and 7 represent in this case? > There are five pins total 0-4 for the existing gpio master. This is only an example node, and the format of those example *-gpios properties isn't really relevant here - what I'm illustrating is that we could have multiple GPIOs described by each property. The actual format of those *-gpios properties depends on the GPIO controller that they're referring to, and is completely opaque to the consumer of those nodes (in our case, that's the fsi master driver). Here's how it works: The GPIO controller driver has its own way of referencing an individual GPIO, as a tuple of cells. This could be something as simple as: (gpio-number) or, typically they may have flags (like active-low, etc) too: (gpio-number,flags) but more complex controllers can use any format, which could better reference their hardware layout. To make up an example: (controller-bank,bank-index,flags) The device tree node for the GPIO controller will have a #gpio-cells property. This tells us how many cells are required for a tuple that describes an individual GPIO. When we want to reference an individual GPIO from elsewhere in the device tree, we use one of these *-gpios properties, which contains: 1) a pointer to the controller node 2) the tuple describing the GPIO within that controller (1) is always a phandle (a single cell that references the phandle property of another node). This is how we know which controller we're referring to. Once we have the controller, we can look at its #gpio-cells property - this tells us how many cells are needed for (2), which allows the GPIO core code to construct that tuple from a DT property. We request GPIOs using this tuple - the controller driver (and only that driver) knows how to interpret the tuple, and returns the selected tuple to our fsi master. [The term "tuple" isn't standard here - I'm just using it for the purposes of this email. The whole construct (ie, (1) and (2)) is called a descriptor though]. However, the fsi driver doesn't need to care about *any* of that - we just do a devm_gpiod_get() and out pops the correct GPIO. We do need to know that format when constructing the device tree though, and that will vary from platform to platform. Using the palmetto board as an example, we would have something that looks like: fsi-master { compatible = "ibm,fsi-master", "ibm,fsi-master-gpio"; clk-gpios = <&gpio ASPEED_GPIO(A,4) GPIO_ACTIVE_LOW>; data-gpios = <&gpio ASPEED_GPIO(A,5) GPIO_ACTIVE_LOW>; } - using GPIOs A4 and A5, Assuming my reading of the schematic is correct. > Up until now, all available FSI masters in hardware, FSP, P8 "hub", > P8 cascade, P7 cascade, have all had multiple links controlled by one > master. It would make sense I think to continue this. I'm only referring to separate "top-level" masters here (ie, simply the groups of GPIOs themselves). There are a couple of considerations here: a) Using one-link-per-GPIO-master keeps the driver code a little simpler; it doesn't need to manage multiple links b) There is a little more scope for complexity & error in the *-gpios properties, if multiple clk/data/etc lines are described (say, if one FSI link of a multi-master doesn't have a separate mux line..) - so I'm slightly preferring the single-link-per-master idea at present (of course, that could cascade to other masters behind slave engines, but the master driver doesn't need to care about that). > There is no direct sharing of information between masters other than > what's required by the firmware to start at the beginning master in > the chain and walk down each link and checking 'sub' master states to > determine where an interrupt or error was sourced. Again, this only references how we handle the top-level masters. Regards, Jeremy
diff --git a/Documentation/devicetree/bindings/fsi/fsi-master-gpio.txt b/Documentation/devicetree/bindings/fsi/fsi-master-gpio.txt new file mode 100644 index 0000000..4cb2c81 --- /dev/null +++ b/Documentation/devicetree/bindings/fsi/fsi-master-gpio.txt @@ -0,0 +1,21 @@ +Device-tree bindings for gpio-based FSI master driver +----------------------------------------------------- + +Required properties: + - compatible = "ibm,fsi-master-gpio"; + - clk-gpio; + - data-gpio; + +Optional properties: + - enable-gpio; + - trans-gpio; + - mux-gpio; + +fsi-master { + compatible = "ibm,fsi-master", "ibm,fsi-master-gpio"; + clk-gpio = <&gpio 0>; + data-gpio = <&gpio 1>; + enable-gpio = <&gpio 2>; + trans-gpio = <&gpio 3>; + mux-gpio = <&gpio 4>; +} diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts b/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts index 21619fd..1875313 100644 --- a/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts +++ b/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts @@ -167,6 +167,36 @@ output-low; line-name = "func_mode2"; }; + + pin_fsi_clk { + gpios = <ASPEED_GPIO(A, 4) GPIO_ACTIVE_HIGH>; + output-low; + line-name = "fsi_clk"; + }; + + pin_fsi_data { + gpios = <ASPEED_GPIO(A, 5) GPIO_ACTIVE_HIGH>; + output-low; + line-name = "fsi_data"; + }; + + pin_fsi_trans { + gpios = <ASPEED_GPIO(H, 6) GPIO_ACTIVE_HIGH>; + output-low; + line-name = "fsi_trans"; + }; + + pin_fsi_enable { + gpios = <ASPEED_GPIO(D, 0) GPIO_ACTIVE_HIGH>; + output-low; + line-name = "fsi_enable"; + }; + + pin_fsi_mux { + gpios = <ASPEED_GPIO(A, 6) GPIO_ACTIVE_HIGH>; + output-low; + line-name = "fsi_mux"; + }; }; &vuart { diff --git a/drivers/fsi/Kconfig b/drivers/fsi/Kconfig index f065dbe..69e7ee8 100644 --- a/drivers/fsi/Kconfig +++ b/drivers/fsi/Kconfig @@ -17,6 +17,13 @@ config FSI_MASTER_FAKE depends on FSI ---help--- This option enables a fake FSI master driver for debugging. + +config FSI_MASTER_GPIO + tristate "GPIO-based FSI master" + depends on FSI + select GPIO_DEVRES + ---help--- + This option enables a FSI master driver, using GPIO lines directly. endif endmenu diff --git a/drivers/fsi/Makefile b/drivers/fsi/Makefile index 847c00c..2021ce5 100644 --- a/drivers/fsi/Makefile +++ b/drivers/fsi/Makefile @@ -1,3 +1,4 @@ obj-$(CONFIG_FSI) += fsi-core.o obj-$(CONFIG_FSI_MASTER_FAKE) += fsi-master-fake.o +obj-$(CONFIG_FSI_MASTER_GPIO) += fsi-master-gpio.o diff --git a/drivers/fsi/fsi-master-gpio.c b/drivers/fsi/fsi-master-gpio.c new file mode 100644 index 0000000..a9ee36c --- /dev/null +++ b/drivers/fsi/fsi-master-gpio.c @@ -0,0 +1,464 @@ +/* + * A FSI master controller, using a simple GPIO bit-banging interface + */ + +#include <linux/platform_device.h> +#include <linux/gpio/consumer.h> +#include <linux/module.h> +#include <linux/delay.h> + +#include "fsi-master.h" + +#define FSI_ECHO_DELAY_CLOCKS 16 /* Number clocks for echo delay */ +#define FSI_PRE_BREAK_CLOCKS 50 /* Number clocks to prep for break */ +#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_STD_DELAY 10 /* Standard GPIO delay in nS */ + /* todo: adjust down as low as */ + /* possible or eliminate */ +#define FSI_GPIO_CMD_DPOLL 0x0000808A +#define FSI_GPIO_CMD_DPOLL_SIZE 10 +#define FSI_GPIO_DPOLL_CLOCKS 24 /* < 21 will cause slave to hang */ +#define FSI_GPIO_CMD_DEFAULT 0x2000000000000000ULL +#define FSI_GPIO_CMD_WRITE 0 +#define FSI_GPIO_CMD_READ 0x0400000000000000ULL +#define FSI_GPIO_CMD_SLAVE_MASK 0xC000000000000000ULL +#define FSI_GPIO_CMD_ADDR_SHIFT 3 +#define FSI_GPIO_CMD_SIZE_16 0x0000001000000000ULL +#define FSI_GPIO_CMD_SIZE_32 0x0000003000000000ULL +#define FSI_GPIO_CMD_DATA_SHIFT 28 +#define FSI_GPIO_CMD_DFLT_LEN 32 +#define FSI_GPIO_RESP_BUSY 1 +#define FSI_GPIO_RESP_ERRA 2 +#define FSI_GPIO_RESP_ERRC 3 +#define FSI_GPIO_RESP_ACK 0 +#define FSI_GPIO_RESP_ACKD 4 +#define FSI_GPIO_MAX_BUSY 100 +#define FSI_GPIO_MTOE_COUNT 1000 +#define FSI_GPIO_MTOE 1 +#define FSI_GPIO_DRAIN_BITS 20 +#define FSI_GPIO_CRC_SIZE 4 +#define FSI_GPIO_MSG_ID_SIZE 2 +#define FSI_GPIO_MSG_RESPID_SIZE 2 +#define FSI_GPIO_CRC_INVAL 5 + +struct fsi_master_gpio { + struct fsi_master master; + struct gpio_desc *gpio_clk; + struct gpio_desc *gpio_data; + struct gpio_desc *gpio_trans; /* Voltage translator */ + struct gpio_desc *gpio_enable; /* FSI enable */ + struct gpio_desc *gpio_mux; /* Mux control */ +}; + +#define to_fsi_master_gpio(m) container_of(m, struct fsi_master_gpio, master) + +struct fsi_gpio_msg { + uint64_t msg; + uint8_t bits; +}; + +static void set_clock(struct fsi_master_gpio *master) +{ + ndelay(FSI_GPIO_STD_DELAY); + gpiod_set_value(master->gpio_clk, 1); +} + +static void clear_clock(struct fsi_master_gpio *master) +{ + ndelay(FSI_GPIO_STD_DELAY); + gpiod_set_value(master->gpio_clk, 0); +} + +static void clock_toggle(struct fsi_master_gpio *master, int count) +{ + int i; + + for (i = 0; i < count; i++) { + clear_clock(master); + set_clock(master); + } +} + +static int sda_in(struct fsi_master_gpio *master) +{ + return gpiod_get_value(master->gpio_data); +} + +static void sda_out(struct fsi_master_gpio *master, int value) +{ + gpiod_set_value(master->gpio_data, value); + ndelay(FSI_GPIO_STD_DELAY); +} + +static void set_sda_input(struct fsi_master_gpio *master) +{ + gpiod_direction_input(master->gpio_data); + if (master->gpio_trans) + gpiod_direction_output(master->gpio_trans, 0); +} +static void set_sda_output(struct fsi_master_gpio *master, + int value) +{ + gpiod_direction_output(master->gpio_data, value); + if (master->gpio_trans) + gpiod_direction_output(master->gpio_trans, 1); +} + +static void serial_in(struct fsi_master_gpio *master, struct fsi_gpio_msg *cmd, + uint8_t num_bits) +{ + uint8_t bit; + uint64_t msg = 0; + uint8_t in_bit = 0; + + set_sda_input(master); + cmd->bits = 0; + + for (bit = 0; bit < num_bits; bit++) { + clock_toggle(master, 1); + in_bit = sda_in(master); + cmd->bits++; + msg <<= 1; + msg |= ~in_bit & 0x1; /* Data is negative active */ + } + cmd->msg = msg; +} + +static void serial_out(struct fsi_master_gpio *master, + const struct fsi_gpio_msg *cmd) +{ + uint8_t bit; + uint64_t msg = ~cmd->msg; /* Data is negative active */ + uint64_t sda_mask; + uint64_t last_bit = ~0; + int next_bit; + + if (!cmd->bits) { + dev_warn(master->master.dev, "trying to output 0 bits\n"); + return; + } + sda_mask = BIT(cmd->bits - 1); + set_sda_output(master, 0); + + /* Send the start bit */ + sda_out(master, 1); + clock_toggle(master, 1); + + /* Send the message */ + for (bit = 0; bit < cmd->bits; bit++) { + next_bit = (msg & sda_mask) >> (cmd->bits - 1); + if (last_bit ^ next_bit) { + sda_out(master, next_bit); + last_bit = next_bit; + } + clock_toggle(master, 1); + msg <<= 1; + } +} + +/* + * Clock out some 0's after every message to ride out line reflections + */ +static void echo_delay(struct fsi_master_gpio *master) +{ + set_sda_output(master, 0); + clock_toggle(master, FSI_ECHO_DELAY_CLOCKS); +} + +/* + * Used in bus error cases only. Clears out any remaining data the slave + * is attempting to send + */ +static void drain_response(struct fsi_master_gpio *master) +{ + struct fsi_gpio_msg msg; + + serial_in(master, &msg, FSI_GPIO_DRAIN_BITS); +} + +/* + * 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) +{ + +} + +static int poll_for_response(struct fsi_master_gpio *master, uint8_t expected, + uint8_t size, void *data) +{ + int busy_count = 0, i; + struct fsi_gpio_msg response, cmd; + int bits_remaining = 0; + uint64_t resp = 0; + uint8_t bits_received = 1 + FSI_GPIO_MSG_ID_SIZE + + FSI_GPIO_MSG_RESPID_SIZE; + uint8_t crc_in; + + do { + for (i = 0; i < FSI_GPIO_MTOE_COUNT; i++) { + serial_in(master, &response, 1); + resp = response.msg; + if (response.msg) + break; + } + if (i >= FSI_GPIO_MTOE_COUNT) { + dev_info(master->master.dev, + "Master time out waiting for response\n"); + drain_response(master); + fsi_master_gpio_error(master, FSI_GPIO_MTOE); + return FSI_GPIO_MTOE; + } + + /* Response received */ + serial_in(master, &response, FSI_GPIO_MSG_ID_SIZE); + dev_info(master->master.dev, "cfam id:%d\n", (int)response.msg); + resp <<= FSI_GPIO_MSG_ID_SIZE; + resp |= response.msg; + + serial_in(master, &response, FSI_GPIO_MSG_RESPID_SIZE); + dev_info(master->master.dev, "response id:%d\n", + (int)response.msg); + resp <<= FSI_GPIO_MSG_RESPID_SIZE; + resp |= response.msg; + + switch (response.msg) { + case FSI_GPIO_RESP_ACK: + if (expected == FSI_GPIO_RESP_ACKD) + bits_remaining = 8 * size; + break; + + case FSI_GPIO_RESP_BUSY: + /* + * Its necessary to clock slave before issuing + * d-poll, not indicated in the hardware protocol + * spec. < 20 clocks causes slave to hang, 21 ok. + */ + set_sda_output(master, 0); + clock_toggle(master, FSI_GPIO_DPOLL_CLOCKS); + cmd.msg = FSI_GPIO_CMD_DPOLL; + cmd.bits = FSI_GPIO_CMD_DPOLL_SIZE; + serial_out(master, &cmd); + continue; + + case FSI_GPIO_RESP_ERRA: + case FSI_GPIO_RESP_ERRC: + dev_info(master->master.dev, "ERR received: %d\n", + (int)response.msg); + drain_response(master); + fsi_master_gpio_error(master, response.msg); + return response.msg; + } + + /* Read in the data field if applicable */ + if (bits_remaining) { + serial_in(master, &response, bits_remaining); + resp <<= bits_remaining; + resp |= response.msg; + bits_received += bits_remaining; + } + + /* Read in the crc and check it */ + serial_in(master, &response, FSI_GPIO_CRC_SIZE); + + crc_in = fsi_crc4(0, 1, 1); + crc_in = fsi_crc4(crc_in, resp, bits_received); + if (crc_in != response.msg) { + /* CRC's don't match */ + dev_info(master->master.dev, "ERR response CRC\n"); + fsi_master_gpio_error(master, FSI_GPIO_CRC_INVAL); + return FSI_GPIO_CRC_INVAL; + } + return 0; + + } while (busy_count++ < FSI_GPIO_MAX_BUSY); + + return FSI_GPIO_MAX_BUSY; +} + +static void build_abs_ar_command(struct fsi_gpio_msg *cmd, uint64_t mode, + uint8_t slave, uint32_t addr, size_t size, + const void *data) +{ + uint8_t crc; + uint64_t msg_with_start; + + cmd->bits = FSI_GPIO_CMD_DFLT_LEN; + cmd->msg = FSI_GPIO_CMD_DEFAULT; + cmd->msg |= mode; + + /* todo: handle more than just slave id 0 */ + cmd->msg &= ~FSI_GPIO_CMD_SLAVE_MASK; + + cmd->msg |= (addr << FSI_GPIO_CMD_ADDR_SHIFT); + if (size == sizeof(uint8_t)) { + if (data) + cmd->msg |= + *((uint8_t *)data) >> FSI_GPIO_CMD_DATA_SHIFT; + } else if (size == sizeof(uint16_t)) { + cmd->msg |= FSI_GPIO_CMD_SIZE_16; + if (data) + cmd->msg |= + *((uint16_t *)data) >> FSI_GPIO_CMD_DATA_SHIFT; + } else { + cmd->msg |= FSI_GPIO_CMD_SIZE_32; + if (data) + cmd->msg |= + *((uint32_t *)data) >> FSI_GPIO_CMD_DATA_SHIFT; + } + + if (mode == FSI_GPIO_CMD_WRITE) + cmd->bits += (8 * size); + + /* Start bit isn't considered part of command but we need to */ + /* account for it in crc calcs */ + msg_with_start = 0x1 << cmd->bits; + msg_with_start |= cmd->msg; + crc = fsi_crc4(0, 1, 1); + crc = fsi_crc4(crc, msg_with_start, cmd->bits); + cmd->msg |= crc >> cmd->bits; + cmd->bits += FSI_GPIO_CRC_SIZE; +} + +static int fsi_master_gpio_read(struct fsi_master *_master, int link, + uint8_t slave, uint32_t addr, void *val, size_t size) +{ + struct fsi_master_gpio *master = to_fsi_master_gpio(_master); + struct fsi_gpio_msg cmd; + + if (link != 0) + return -ENODEV; + + build_abs_ar_command(&cmd, FSI_GPIO_CMD_READ, slave, addr, size, NULL); + serial_out(master, &cmd); + echo_delay(master); + + return poll_for_response(master, FSI_GPIO_RESP_ACKD, size, val); +} + +static int fsi_master_gpio_write(struct fsi_master *_master, int link, + uint8_t slave, uint32_t addr, const void *val, size_t size) +{ + struct fsi_master_gpio *master = to_fsi_master_gpio(_master); + struct fsi_gpio_msg cmd; + + if (link != 0) + return -ENODEV; + + build_abs_ar_command(&cmd, FSI_GPIO_CMD_WRITE, slave, addr, size, val); + serial_out(master, &cmd); + echo_delay(master); + + return poll_for_response(master, FSI_GPIO_RESP_ACK, size, NULL); +} + +/* + * Issue a break command on link + */ +static int fsi_master_gpio_break(struct fsi_master *_master, int link) +{ + struct fsi_master_gpio *master = to_fsi_master_gpio(_master); + + if (link != 0) + return -ENODEV; + + set_sda_output(master, 0); + clock_toggle(master, FSI_PRE_BREAK_CLOCKS); + sda_out(master, 1); + clock_toggle(master, FSI_BREAK_CLOCKS); + echo_delay(master); + sda_out(master, 0); + clock_toggle(master, FSI_POST_BREAK_CLOCKS); + + /* Wait for logic reset to take effect */ + udelay(200); + + return 0; +} + +static void fsi_master_gpio_init(struct fsi_master_gpio *master) +{ + if (master->gpio_mux) + gpiod_direction_output(master->gpio_mux, 1); + gpiod_direction_output(master->gpio_clk, 1); + set_sda_output(master, 1); + if (master->gpio_enable) + gpiod_direction_output(master->gpio_enable, 0); + + /* todo: evaluate if clocks can be reduced */ + clock_toggle(master, FSI_INIT_CLOCKS); +} + +static int fsi_master_gpio_link_enable(struct fsi_master *_master, int link) +{ + struct fsi_master_gpio *master = to_fsi_master_gpio(_master); + + if (link != 0) + return -ENODEV; + + if (master->gpio_enable) + gpiod_set_value(master->gpio_enable, 1); + + return 0; +} + +static int fsi_master_gpio_probe(struct platform_device *pdev) +{ + struct fsi_master_gpio *master; + struct gpio_desc *gpio; + + master = devm_kzalloc(&pdev->dev, sizeof(*master), GFP_KERNEL); + if (!master) + return -ENOMEM; + + gpio = devm_gpiod_get(&pdev->dev, "fsi_clk", 0); + if (IS_ERR(gpio)) + return PTR_ERR(gpio); + master->gpio_clk = gpio; + + gpio = devm_gpiod_get(&pdev->dev, "fsi_data", 0); + if (IS_ERR(gpio)) + return PTR_ERR(gpio); + master->gpio_data = gpio; + + /* Optional pins */ + + gpio = devm_gpiod_get(&pdev->dev, "fsi_trans", 0); + if (!IS_ERR(gpio)) + master->gpio_trans = gpio; + + gpio = devm_gpiod_get(&pdev->dev, "fsi_enable", 0); + if (!IS_ERR(gpio)) + master->gpio_enable = gpio; + + gpio = devm_gpiod_get(&pdev->dev, "fsi_mux", 0); + if (!IS_ERR(gpio)) + master->gpio_mux = gpio; + + master->master.read = fsi_master_gpio_read; + master->master.write = fsi_master_gpio_write; + master->master.send_break = fsi_master_gpio_break; + master->master.link_enable = fsi_master_gpio_link_enable; + + fsi_master_gpio_init(master); + + return fsi_master_register(&master->master); +} + +static const struct of_device_id fsi_master_gpio_match[] = { + { .compatible = "ibm,fsi-master-gpio" }, + { }, +}; + +static struct platform_driver fsi_master_gpio_driver = { + .driver = { + .name = "fsi-master-gpio", + .of_match_table = fsi_master_gpio_match, + }, + .probe = fsi_master_gpio_probe, +}; + +module_platform_driver(fsi_master_gpio_driver);