diff mbox series

[v3] i2c: at91: support atomic write xfer

Message ID 55613934b7d14ae4122b648c20351b63b03a1385.1584851536.git.mirq-linux@rere.qmqm.pl
State Under Review
Headers show
Series [v3] i2c: at91: support atomic write xfer | expand

Commit Message

Michał Mirosław March 22, 2020, 4:33 a.m. UTC
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(-)

Comments

Wolfram Sang March 22, 2020, 2:30 p.m. UTC | #1
> +	/* 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;
Michał Mirosław March 22, 2020, 4:30 p.m. UTC | #2
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
Wolfram Sang May 5, 2020, 3:52 p.m. UTC | #3
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
Michał Mirosław May 5, 2020, 4:47 p.m. UTC | #4
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
Michał Mirosław May 6, 2020, 5:17 p.m. UTC | #5
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 mbox series

Patch

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)