| Message ID | 55613934b7d14ae4122b648c20351b63b03a1385.1584851536.git.mirq-linux@rere.qmqm.pl |
|---|---|
| State | Under Review |
| Delegated to: | Wolfram Sang |
| Headers | show |
| Series | [v3] i2c: at91: support atomic write xfer | expand |
> + /* FIXME: only single write request supported to 7-bit addr */ Hmm, this is quite limited. Would it be very hard to support multiple messages? Or reads? 10 bits don't matter. > + if (!dev->pdata->has_alt_cmd) > + return -EOPNOTSUPP; We should handle this in probe(), I think: if (dev->pdata->has_alt_cmd) at91_twi_algorithm.master_xfer_atomic = at91_twi_xfer_atomic;
On Sun, Mar 22, 2020 at 03:30:04PM +0100, Wolfram Sang wrote: > > > + /* FIXME: only single write request supported to 7-bit addr */ > > Hmm, this is quite limited. Would it be very hard to support multiple > messages? Or reads? 10 bits don't matter. I don't expect this to be used for much more than a simple write to PMIC to kill the power. So this patch is tailor made for exactly this purpose. Though, if you would go for full support of atomic transfers, then I would suggest to hack the non-atomic path to be usable in atomic mode instead (some I2C drivers do just that, eg. i2c-tegra). BTW, I found this comment in i2c-core.h: * We only allow atomic transfers for very late communication, e.g. to send * the powerdown command to a PMIC. Atomic transfers are a corner case and not * for generic use! I think this covers the idea. > > + if (!dev->pdata->has_alt_cmd) > > + return -EOPNOTSUPP; > > We should handle this in probe(), I think: > > if (dev->pdata->has_alt_cmd) > at91_twi_algorithm.master_xfer_atomic = at91_twi_xfer_atomic; This would mean writable ops structure - something I try hard to avoid. We can use another copy of i2c_algorithm structure if needed, though. Best Regards Michał Mirosław
Hi, > I don't expect this to be used for much more than a simple write to PMIC > to kill the power. So this patch is tailor made for exactly this purpose. Frankly, I don't like it much. The atomic callbacks are supposed to be drop-in replacements of the non-atomic contexts. There may be a need to read a PMIC register before writing something. I considered checking in the core if we can fall back to non-atomic calls if the the atomic ones return -EOPNOTSUPP, though, but I still don't like the idea. I expect that people send me minimal versions then which are extended over time by very personal use cases. Having a proper implementation once-and-for-all (despite bugfixes) sounds much more maintainable to me. > Though, if you would go for full support of atomic transfers, then > I would suggest to hack the non-atomic path to be usable in atomic mode > instead (some I2C drivers do just that, eg. i2c-tegra). Yes, that is what I am aiming for. > BTW, I found this comment in i2c-core.h: > > * We only allow atomic transfers for very late communication, e.g. to send > * the powerdown command to a PMIC. Atomic transfers are a corner case and not > * for generic use! > > I think this covers the idea. Well, since I implemented the atomic_xfer mechanism, I think I am the primary authority of what "covers the idea", so I will fix the comment above :) Note, there is also this comment in the way more user-visible include/linux/i2c.h: 509 * @master_xfer_atomic: same as @master_xfer. Yet, only using atomic context 510 * so e.g. PMICs can be accessed very late before shutdown. Optional. All the best, Wolfram
On Tue, May 05, 2020 at 05:52:28PM +0200, Wolfram Sang wrote: > Hi, > > > I don't expect this to be used for much more than a simple write to PMIC > > to kill the power. So this patch is tailor made for exactly this purpose. > > Frankly, I don't like it much. The atomic callbacks are supposed to be > drop-in replacements of the non-atomic contexts. There may be a need to > read a PMIC register before writing something. I considered checking in > the core if we can fall back to non-atomic calls if the the atomic ones > return -EOPNOTSUPP, though, but I still don't like the idea. I expect > that people send me minimal versions then which are extended over time > by very personal use cases. Having a proper implementation > once-and-for-all (despite bugfixes) sounds much more maintainable to me. Is it really possible to fall back to non-atomic calls? If that would be possible, then I would actually want to use it in this case instead of writing another implementation. I must say, that I'm reluctant in investing a lot of time in doing full-blown implementation for a feature that has only a very limited use. I do understand your point about a proper implementation being better than a special-cased-only one, but I really don't have a use for the extension. The few PMICs I worked with just need a single write to power off. > > Though, if you would go for full support of atomic transfers, then > > I would suggest to hack the non-atomic path to be usable in atomic mode > > instead (some I2C drivers do just that, eg. i2c-tegra). > > Yes, that is what I am aiming for. > > > BTW, I found this comment in i2c-core.h: > > > > * We only allow atomic transfers for very late communication, e.g. to send > > * the powerdown command to a PMIC. Atomic transfers are a corner case and not > > * for generic use! > > > > I think this covers the idea. > > Well, since I implemented the atomic_xfer mechanism, I think I am the > primary authority of what "covers the idea", so I will fix the comment > above :) Note, there is also this comment in the way more user-visible > include/linux/i2c.h: > > 509 * @master_xfer_atomic: same as @master_xfer. Yet, only using atomic context > 510 * so e.g. PMICs can be accessed very late before shutdown. Optional. So, we don't have to wonder what the author had in mind. Lets expand the idea then. :-) Shutdown is kind of special atomic context in that it is ok to do long waits (as I2C requires) because nothing else is there to do. This is very unlike normal atomic context. Do you plan to have it work in other contexts? What are the idea and use cases for atomic-context transfers? I guess we might want it for suspend/resume, but I think there is an early stage (with all non-atomic stuff working) and NOIRQ stage (when most everything is already shutdown). When a PMIC needs a read, I would actually do it ("prepare" the PMIC) in the early stage if possible. Best Regards, Michał Mirosław
On Tue, May 05, 2020 at 06:47:39PM +0200, Michał Mirosław wrote: > On Tue, May 05, 2020 at 05:52:28PM +0200, Wolfram Sang wrote: [...] > > > BTW, I found this comment in i2c-core.h: > > > > > > * We only allow atomic transfers for very late communication, e.g. to send > > > * the powerdown command to a PMIC. Atomic transfers are a corner case and not > > > * for generic use! > > > > > > I think this covers the idea. > > > > Well, since I implemented the atomic_xfer mechanism, I think I am the > > primary authority of what "covers the idea", so I will fix the comment > > above :) Note, there is also this comment in the way more user-visible > > include/linux/i2c.h: > > > > 509 * @master_xfer_atomic: same as @master_xfer. Yet, only using atomic context > > 510 * so e.g. PMICs can be accessed very late before shutdown. Optional. > > So, we don't have to wonder what the author had in mind. Lets expand > the idea then. :-) > > Shutdown is kind of special atomic context in that it is ok to do long > waits (as I2C requires) because nothing else is there to do. This is > very unlike normal atomic context. Do you plan to have it work in other > contexts? What are the idea and use cases for atomic-context transfers? > > I guess we might want it for suspend/resume, but I think there is an > early stage (with all non-atomic stuff working) and NOIRQ stage (when > most everything is already shutdown). When a PMIC needs a read, I would > actually do it ("prepare" the PMIC) in the early stage if possible. For a followup, I did a quick grep for pm_power_off in i2c drivers [1] and looked around how are the shutdown handlers implemented. Mostly I see regmap_update_bits() (almost all with a regcache) and plain writes. No driver checks if the I2C controller provides atomic transfers - all assume it is possible. Coming back to the original patch, I think that WARN on error from the atomic is transfer is missing here. The core tries to use normal master_xfer in atomic context as a fallback, but I'm not sure this actually works (I wrote the patch because it didn't). If the driver API had split submit and wait callbacks, this could be much easier, as there would only be need to implement atomic wait part differently most of the time. Best Regards, Michał Mirosław [1] grep -rl 'i2c\|smbus' $(grep pm_power_off -rl drivers/)
diff --git a/drivers/i2c/busses/i2c-at91-master.c b/drivers/i2c/busses/i2c-at91-master.c index ba6fbb9c7390..10c66809df83 100644 --- a/drivers/i2c/busses/i2c-at91-master.c +++ b/drivers/i2c/busses/i2c-at91-master.c @@ -21,8 +21,10 @@ #include <linux/i2c.h> #include <linux/interrupt.h> #include <linux/io.h> +#include <linux/iopoll.h> #include <linux/of.h> #include <linux/of_device.h> +#include <linux/pinctrl/consumer.h> #include <linux/platform_device.h> #include <linux/platform_data/dma-atmel.h> #include <linux/pm_runtime.h> @@ -709,6 +711,68 @@ static int at91_twi_xfer(struct i2c_adapter *adap, struct i2c_msg *msg, int num) return ret; } +static int at91_twi_xfer_atomic(struct i2c_adapter *adap, struct i2c_msg *msg, int num) +{ + struct at91_twi_dev *dev = i2c_get_adapdata(adap); + struct pinctrl *pins; + __u32 stat; + int ret; + + /* FIXME: only single write request supported to 7-bit addr */ + if (num != 1) + return -EOPNOTSUPP; + if (msg->flags & I2C_M_RD) + return -EOPNOTSUPP; + if (msg->flags & I2C_M_TEN) + return -EOPNOTSUPP; + if (msg->len > dev->fifo_size && msg->len > 1) + return -EOPNOTSUPP; + if (!dev->pdata->has_alt_cmd) + return -EOPNOTSUPP; + + pins = pinctrl_get_select_default(&adap->dev); + + ret = clk_prepare_enable(dev->clk); + if (ret) + goto out; + + /* Clear and disable pending interrupts, such as NACK. */ + at91_twi_read(dev, AT91_TWI_SR); + at91_twi_write(dev, AT91_TWI_IDR, ~0); + + at91_twi_write(dev, AT91_TWI_MMR, msg->addr << 16); + + if (!msg->len) { + at91_twi_write(dev, AT91_TWI_CR, + AT91_TWI_ACMDIS | AT91_TWI_QUICK); + } else { + size_t n = msg->len; + __u8 *p; + + at91_twi_write(dev, AT91_TWI_CR, + AT91_TWI_ACMEN | + AT91_TWI_THRCLR | AT91_TWI_RHRCLR); + at91_twi_write(dev, AT91_TWI_ACR, AT91_TWI_ACR_DATAL(n)); + for (p = msg->buf; n--; ++p) + writeb_relaxed(*p, dev->base + AT91_TWI_THR); + } + + readl_relaxed_poll_timeout_atomic(dev->base + AT91_TWI_SR, stat, + stat & AT91_TWI_TXCOMP, 100, + (2 + msg->len) * 1000); + if (stat & AT91_TWI_NACK) + ret = -EREMOTEIO; + else + ret = num; + + clk_disable_unprepare(dev->clk); +out: + if (!IS_ERR(pins)) + pinctrl_put(pins); + + return ret; +} + /* * The hardware can handle at most two messages concatenated by a * repeated start via it's internal address feature. @@ -725,8 +789,9 @@ static u32 at91_twi_func(struct i2c_adapter *adapter) } static const struct i2c_algorithm at91_twi_algorithm = { - .master_xfer = at91_twi_xfer, - .functionality = at91_twi_func, + .master_xfer = at91_twi_xfer, + .master_xfer_atomic = at91_twi_xfer_atomic, + .functionality = at91_twi_func, }; static int at91_twi_configure_dma(struct at91_twi_dev *dev, u32 phy_addr)
Implement basic support for atomic write - enough to get a simple write to PMIC on shutdown. Only for chips having ALT_CMD register, eg. SAMA5D2. Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> --- v2: remove runtime-PM usage switch to readl*poll*atomic() for transfer completion wait v3: build fixes --- drivers/i2c/busses/i2c-at91-master.c | 69 +++++++++++++++++++++++++++- 1 file changed, 67 insertions(+), 2 deletions(-)