[PATCH linux dev-4.7 4/5] drivers/fsi: Add FSI bus error cleanup
Christopher Bostic
cbostic at linux.vnet.ibm.com
Sat Feb 18 04:47:13 AEDT 2017
On 2/17/17 6:55 AM, Joel Stanley wrote:
> On Fri, Feb 17, 2017 at 8:34 AM, Christopher Bostic
> <cbostic at linux.vnet.ibm.com> wrote:
>> On detect of bus fails attempt to clean them up and try the
>> command again.
>>
>> Signed-off-by: Christopher Bostic <cbostic at linux.vnet.ibm.com>
>> ---
>> drivers/fsi/fsi-master-gpio.c | 100 +++++++++++++++++++++++++++++++++++-------
>> 1 file changed, 85 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/fsi/fsi-master-gpio.c b/drivers/fsi/fsi-master-gpio.c
>> index 92af53d..bb3983c 100644
>> --- a/drivers/fsi/fsi-master-gpio.c
>> +++ b/drivers/fsi/fsi-master-gpio.c
>> @@ -67,8 +67,16 @@
>>
>> #define FSI_MASTER_GPIO_LINK_SIZE 0x00800000
>>
>> +static int fsi_master_gpio_read(struct fsi_master *_master, int link,
>> + uint8_t slave, uint32_t addr, void *val, size_t size);
>> +static int fsi_master_gpio_write(struct fsi_master *_master, int link,
>> + uint8_t slave, uint32_t addr, const void *val, size_t size);
>> +static int fsi_master_gpio_break(struct fsi_master *_master, int link);
>> +
>> static DEFINE_SPINLOCK(fsi_gpio_cmd_lock); /* lock around fsi commands */
>>
>> +uint8_t in_err_cleanup;
>> +
>> struct fsi_master_gpio {
>> struct fsi_master master;
>> struct gpio_desc *gpio_clk;
>> @@ -358,24 +366,85 @@ static void build_abs_ar_command(struct fsi_gpio_msg *cmd, uint64_t mode,
>> cmd->msg >>= (64 - cmd->bits);
>> }
>>
>> +/*
>> + * Clean up the FSI bus path on detection of error.
>> + * todo: get rid of magic numbers
>> + */
>> +static void fsi_master_gpio_handle_error(struct fsi_master_gpio *master,
>> + uint32_t addr)
>> +{
>> + uint32_t smode = 0xa0ff0100;
> I was wondering what a smode is?
Hi Joel,
smode is the Slave Mode register. I know, its a hack to directly
access the slave registers from the hub code. The next revision I'll
put a bit more thought into how to manage this.
>> +
>> + /* Avoid nested error handling */
>> + if (in_err_cleanup)
>> + return;
>> +
>> + in_err_cleanup = 1;
> This locking is racy. Instead use a atomic type for in_err_cleanup,
> declared in the scope of fsi_master_gpio_handle_error.
Will change.
>> + fsi_master_gpio_break(&master->master, 0);
>> + udelay(200);
>> +
>> + /* Set slave ID in smode */
>> + fsi_master_gpio_write(&master->master, 0, 0, 0x800, &smode,
>> + sizeof(smode));
>> +
>> + /* Reset the MFSI bridge */
>> + smode = 0xc0000000;
>> + fsi_master_gpio_write(&master->master, 0, 0, 0x35D0, &smode,
>> + sizeof(smode));
>> +
>> + if (addr >= 0x80000) {
>> +
>> + /*
>> + * Break to hub link 1
>> + * todo: make this handle more arbitrary link topologies
>> + */
>> + smode = 0xc0de0000;
>> + fsi_master_gpio_write(&master->master, 0, 0, 0x100004, &smode,
>> + sizeof(smode));
>> +
>> + smode = 0xa0ff0100;
>> + fsi_master_gpio_write(&master->master, 0, 0, 0x100800, &smode,
>> + sizeof(smode));
>> + }
>> + in_err_cleanup = 0;
>> +}
>> +
>> +static int send_command(struct fsi_master_gpio *master,
>> + struct fsi_gpio_msg *cmd, uint8_t expected,
>> + size_t size, void *val)
>> +{
>> + unsigned long flags;
>> + int rc;
>> +
>> + spin_lock_irqsave(&fsi_gpio_cmd_lock, flags);
>> + serial_out(master, cmd);
>> + echo_delay(master);
>> + rc = poll_for_response(master, expected, size, val);
>> + spin_unlock_irqrestore(&fsi_gpio_cmd_lock, flags);
>> +
>> + return rc;
>> +}
>> +
>> 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;
>> int rc;
>> - unsigned long flags;
>>
>> if (link != 0)
>> return -ENODEV;
>>
>> build_abs_ar_command(&cmd, FSI_GPIO_CMD_READ, slave, addr, size, NULL);
>> -
>> - spin_lock_irqsave(&fsi_gpio_cmd_lock, flags);
>> - serial_out(master, &cmd);
>> - echo_delay(master);
>> - rc = poll_for_response(master, FSI_GPIO_RESP_ACKD, size, val);
>> - spin_unlock_irqrestore(&fsi_gpio_cmd_lock, flags);
> I see you've pulled the guts of this function out into send_command.
> It would have made it easier to follow if that refactoring was in it's
> own patch, and the error handling was introduced afterwards.
One reason I left the send_command refactor in with the rest of this
patch is that if I didn't then I'd have several locations in the file
that do the same sequence of steps - itself a cause for a review
comment. Guess breaking it out in a separate patch is a preferable
approach in the interest of making the code more review friendly, will
change.
>> + rc = send_command(master, &cmd, FSI_GPIO_RESP_ACKD, size, val);
>> + if (rc) {
>> + fsi_master_gpio_handle_error(master, addr);
>> +
>> + /* Try again */
>> + rc = send_command(master, &cmd, FSI_GPIO_RESP_ACKD, size, val);
>> + if (rc)
>> + fsi_master_gpio_handle_error(master, addr);
>> + }
>>
>> return rc;
>> }
>> @@ -386,18 +455,20 @@ 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;
>> - unsigned long flags;
>>
>> if (link != 0)
>> return -ENODEV;
>>
>> build_abs_ar_command(&cmd, FSI_GPIO_CMD_WRITE, slave, addr, size, val);
>> -
>> - spin_lock_irqsave(&fsi_gpio_cmd_lock, flags);
>> - serial_out(master, &cmd);
>> - echo_delay(master);
>> - rc = poll_for_response(master, FSI_GPIO_RESP_ACK, size, NULL);
>> - spin_unlock_irqrestore(&fsi_gpio_cmd_lock, flags);
>> + rc = send_command(master, &cmd, FSI_GPIO_RESP_ACK, size, NULL);
>> + if (rc) {
>> + fsi_master_gpio_handle_error(master, addr);
>> +
>> + /* Try again */
>> + rc = send_command(master, &cmd, FSI_GPIO_RESP_ACK, size, NULL);
>> + if (rc)
>> + fsi_master_gpio_handle_error(master, addr);
>> + }
>>
>> return rc;
>> }
>> @@ -480,7 +551,6 @@ static int fsi_master_gpio_probe(struct platform_device *pdev)
>> master = devm_kzalloc(&pdev->dev, sizeof(*master), GFP_KERNEL);
>> if (!master)
>> return -ENOMEM;
>> - master->master.dev = &pdev->dev;
> I'm not sure why you're making this change?
This was related to a change earlier in the patch set. It doesn't
belong in this one. Will fix.
Thanks,
Chris
>
>> gpio = devm_gpiod_get(&pdev->dev, "clock", 0);
>> if (IS_ERR(gpio)) {
>> --
>> 1.8.2.2
>>
More information about the openbmc
mailing list