From: Brendan Higgins <brendanhiggins@google.com> To: Wolfram Sang <wsa@the-dreams.de> Cc: "Rob Herring" <robh+dt@kernel.org>, "Mark Rutland" <mark.rutland@arm.com>, "Thomas Gleixner" <tglx@linutronix.de>, "Jason Cooper" <jason@lakedaemon.net>, "Marc Zyngier" <marc.zyngier@arm.com>, "Joel Stanley" <joel@jms.id.au>, "Vladimir Zapolskiy" <vz@mleia.com>, "Kachalov Anton" <mouse@mayc.ru>, "Cédric Le Goater" <clg@kaod.org>, "Benjamin Herrenschmidt" <benh@kernel.crashing.org>, "Ryan Chen" <ryan_chen@aspeedtech.com>, linux-i2c@vger.kernel.org, devicetree@vger.kernel.org, "Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>, "OpenBMC Maillist" <openbmc@lists.ozlabs.org> Subject: Re: [PATCH v10 4/5] i2c: aspeed: added driver for Aspeed I2C Date: Mon, 19 Jun 2017 21:02:46 -0700 [thread overview] Message-ID: <CAFd5g471kpgskO1MkBPZgFLbwAqncT_NZy=igVRkYJpxDNRVyg@mail.gmail.com> (raw) In-Reply-To: <20170619141322.devvmcbjaeq4jnv5@ninjato> I thought all of the comments made sense and will be addressed in my next revision, except the following: >> +static int aspeed_i2c_recover_bus(struct aspeed_i2c_bus *bus) >> +{ >> + unsigned long time_left, flags; >> + int ret = 0; >> + u32 command; >> + >> + spin_lock_irqsave(&bus->lock, flags); >> + command = readl(bus->base + ASPEED_I2C_CMD_REG); >> + >> + if (command & ASPEED_I2CD_SDA_LINE_STS) { >> + /* Bus is idle: no recovery needed. */ >> + if (command & ASPEED_I2CD_SCL_LINE_STS) >> + goto out; >> + dev_dbg(bus->dev, "bus hung (state %x), attempting recovery\n", >> + command); >> + >> + reinit_completion(&bus->cmd_complete); >> + writel(ASPEED_I2CD_M_STOP_CMD, bus->base + ASPEED_I2C_CMD_REG); >> + spin_unlock_irqrestore(&bus->lock, flags); >> + >> + time_left = wait_for_completion_timeout( >> + &bus->cmd_complete, bus->adap.timeout); >> + >> + spin_lock_irqsave(&bus->lock, flags); >> + if (time_left == 0) >> + goto reset_out; >> + else if (bus->cmd_err) >> + goto reset_out; >> + /* Recovery failed. */ >> + else if (!(readl(bus->base + ASPEED_I2C_CMD_REG) & >> + ASPEED_I2CD_SCL_LINE_STS)) >> + goto reset_out; >> + /* Bus error. */ >> + } else { >> + dev_dbg(bus->dev, "bus hung (state %x), attempting recovery\n", >> + command); > > Same dbg message as in the condition? Move it out of the 'if'? Message is the same; that's true; however, I only want to print this if I am actually doing the recovery. There is a case in the first condition where we actually don't attempt recovery. (See both SDA and SCL high). Nevertheless, now that I am looking at this. I think it might make sense to make the dbg statements different since I can explain what type of recovery I am attempting. > >> + >> + reinit_completion(&bus->cmd_complete); >> + writel(ASPEED_I2CD_BUS_RECOVER_CMD, >> + bus->base + ASPEED_I2C_CMD_REG); > > Out of interest: What does the RECOVER_CMD do? According to the documentation, it attempts to create 1 to 8 SCL cycles to force any stuck device to let go of the SDA line. It then puts the hardware in a sane state once it detects that the bus has been recovered. I will put a comment to this effect. > >> + spin_unlock_irqrestore(&bus->lock, flags); >> + >> + time_left = wait_for_completion_timeout( >> + &bus->cmd_complete, bus->adap.timeout); >> + >> + spin_lock_irqsave(&bus->lock, flags); >> + if (time_left == 0) >> + goto reset_out; >> + else if (bus->cmd_err) >> + goto reset_out; >> + /* Recovery failed. */ >> + else if (!(readl(bus->base + ASPEED_I2C_CMD_REG) & >> + ASPEED_I2CD_SDA_LINE_STS)) >> + goto reset_out; >> + } >> + >> +out: >> + spin_unlock_irqrestore(&bus->lock, flags); >> + >> + return ret; >> + >> +reset_out: >> + spin_unlock_irqrestore(&bus->lock, flags); >> + >> + return aspeed_i2c_reset(bus); >> +} Cheers!
WARNING: multiple messages have this Message-ID (diff)
From: Brendan Higgins <brendanhiggins-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> To: Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> Cc: "Rob Herring" <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>, "Mark Rutland" <mark.rutland-5wv7dgnIgG8@public.gmane.org>, "Thomas Gleixner" <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>, "Jason Cooper" <jason-NLaQJdtUoK4Be96aLqz0jA@public.gmane.org>, "Marc Zyngier" <marc.zyngier-5wv7dgnIgG8@public.gmane.org>, "Joel Stanley" <joel-U3u1mxZcP9KHXe+LvDLADg@public.gmane.org>, "Vladimir Zapolskiy" <vz-ChpfBGZJDbMAvxtiuMwx3w@public.gmane.org>, "Kachalov Anton" <mouse-Pma6HLj0uuo@public.gmane.org>, "Cédric Le Goater" <clg-Bxea+6Xhats@public.gmane.org>, "Benjamin Herrenschmidt" <benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>, "Ryan Chen" <ryan_chen-SAlXDmAnmOAqDJ6do+/SaQ@public.gmane.org>, linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, "Linux Kernel Mailing List" <linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, "OpenBMC Maillist" <openbmc-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org> Subject: Re: [PATCH v10 4/5] i2c: aspeed: added driver for Aspeed I2C Date: Mon, 19 Jun 2017 21:02:46 -0700 [thread overview] Message-ID: <CAFd5g471kpgskO1MkBPZgFLbwAqncT_NZy=igVRkYJpxDNRVyg@mail.gmail.com> (raw) In-Reply-To: <20170619141322.devvmcbjaeq4jnv5@ninjato> I thought all of the comments made sense and will be addressed in my next revision, except the following: >> +static int aspeed_i2c_recover_bus(struct aspeed_i2c_bus *bus) >> +{ >> + unsigned long time_left, flags; >> + int ret = 0; >> + u32 command; >> + >> + spin_lock_irqsave(&bus->lock, flags); >> + command = readl(bus->base + ASPEED_I2C_CMD_REG); >> + >> + if (command & ASPEED_I2CD_SDA_LINE_STS) { >> + /* Bus is idle: no recovery needed. */ >> + if (command & ASPEED_I2CD_SCL_LINE_STS) >> + goto out; >> + dev_dbg(bus->dev, "bus hung (state %x), attempting recovery\n", >> + command); >> + >> + reinit_completion(&bus->cmd_complete); >> + writel(ASPEED_I2CD_M_STOP_CMD, bus->base + ASPEED_I2C_CMD_REG); >> + spin_unlock_irqrestore(&bus->lock, flags); >> + >> + time_left = wait_for_completion_timeout( >> + &bus->cmd_complete, bus->adap.timeout); >> + >> + spin_lock_irqsave(&bus->lock, flags); >> + if (time_left == 0) >> + goto reset_out; >> + else if (bus->cmd_err) >> + goto reset_out; >> + /* Recovery failed. */ >> + else if (!(readl(bus->base + ASPEED_I2C_CMD_REG) & >> + ASPEED_I2CD_SCL_LINE_STS)) >> + goto reset_out; >> + /* Bus error. */ >> + } else { >> + dev_dbg(bus->dev, "bus hung (state %x), attempting recovery\n", >> + command); > > Same dbg message as in the condition? Move it out of the 'if'? Message is the same; that's true; however, I only want to print this if I am actually doing the recovery. There is a case in the first condition where we actually don't attempt recovery. (See both SDA and SCL high). Nevertheless, now that I am looking at this. I think it might make sense to make the dbg statements different since I can explain what type of recovery I am attempting. > >> + >> + reinit_completion(&bus->cmd_complete); >> + writel(ASPEED_I2CD_BUS_RECOVER_CMD, >> + bus->base + ASPEED_I2C_CMD_REG); > > Out of interest: What does the RECOVER_CMD do? According to the documentation, it attempts to create 1 to 8 SCL cycles to force any stuck device to let go of the SDA line. It then puts the hardware in a sane state once it detects that the bus has been recovered. I will put a comment to this effect. > >> + spin_unlock_irqrestore(&bus->lock, flags); >> + >> + time_left = wait_for_completion_timeout( >> + &bus->cmd_complete, bus->adap.timeout); >> + >> + spin_lock_irqsave(&bus->lock, flags); >> + if (time_left == 0) >> + goto reset_out; >> + else if (bus->cmd_err) >> + goto reset_out; >> + /* Recovery failed. */ >> + else if (!(readl(bus->base + ASPEED_I2C_CMD_REG) & >> + ASPEED_I2CD_SDA_LINE_STS)) >> + goto reset_out; >> + } >> + >> +out: >> + spin_unlock_irqrestore(&bus->lock, flags); >> + >> + return ret; >> + >> +reset_out: >> + spin_unlock_irqrestore(&bus->lock, flags); >> + >> + return aspeed_i2c_reset(bus); >> +} Cheers! -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html
next prev parent reply other threads:[~2017-06-20 4:02 UTC|newest] Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-06-03 1:29 [PATCH v10 0/5] i2c: aspeed: added driver for Aspeed I2C Brendan Higgins 2017-06-03 1:29 ` [PATCH v10 1/5] irqchip/aspeed-i2c-ic: binding docs for Aspeed I2C Interrupt Controller Brendan Higgins 2017-06-05 6:25 ` Joel Stanley 2017-06-07 11:31 ` Marc Zyngier 2017-06-03 1:29 ` [PATCH v10 2/5] irqchip/aspeed-i2c-ic: Add I2C IRQ controller for Aspeed Brendan Higgins 2017-06-05 4:56 ` Andrew Jeffery 2017-06-05 4:56 ` Andrew Jeffery 2017-06-03 1:29 ` [PATCH v10 3/5] i2c: aspeed: added documentation for Aspeed I2C driver Brendan Higgins 2017-06-05 4:40 ` Joel Stanley 2017-06-03 1:29 ` [PATCH v10 4/5] i2c: aspeed: added driver for Aspeed I2C Brendan Higgins 2017-06-05 4:40 ` Joel Stanley 2017-06-05 4:40 ` Joel Stanley 2017-06-05 5:02 ` Andrew Jeffery 2017-06-05 5:02 ` Andrew Jeffery 2017-06-15 23:42 ` Brendan Higgins 2017-06-15 23:42 ` Brendan Higgins 2017-06-19 14:13 ` Wolfram Sang 2017-06-19 14:13 ` Wolfram Sang 2017-06-19 14:21 ` Wolfram Sang 2017-06-20 0:58 ` Brendan Higgins 2017-06-20 0:58 ` Brendan Higgins 2017-06-20 7:49 ` Wolfram Sang 2017-06-20 4:02 ` Brendan Higgins [this message] 2017-06-20 4:02 ` Brendan Higgins 2017-06-03 1:29 ` [PATCH v10 5/5] i2c: aspeed: added slave support for Aspeed I2C driver Brendan Higgins 2017-06-07 6:57 ` Joel Stanley 2017-06-19 14:14 ` Wolfram Sang
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CAFd5g471kpgskO1MkBPZgFLbwAqncT_NZy=igVRkYJpxDNRVyg@mail.gmail.com' \ --to=brendanhiggins@google.com \ --cc=benh@kernel.crashing.org \ --cc=clg@kaod.org \ --cc=devicetree@vger.kernel.org \ --cc=jason@lakedaemon.net \ --cc=joel@jms.id.au \ --cc=linux-i2c@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=marc.zyngier@arm.com \ --cc=mark.rutland@arm.com \ --cc=mouse@mayc.ru \ --cc=openbmc@lists.ozlabs.org \ --cc=robh+dt@kernel.org \ --cc=ryan_chen@aspeedtech.com \ --cc=tglx@linutronix.de \ --cc=vz@mleia.com \ --cc=wsa@the-dreams.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.