Message-ID: <20200124110018.GR15507@dell>
Date:   Fri, 24 Jan 2020 11:00:18 +0000
From:   Lee Jones <lee.jones@...aro.org>
To:     Adam Thomson <Adam.Thomson.Opensource@...semi.com>,
        Mark Brown <broonie@...nel.org>,
        Wolfram Sang <wsa@...-dreams.de>
Cc:     Helmut Grohne <helmut.grohne@...enta.de>,
        Support Opensource <Support.Opensource@...semi.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] mfd: da9062: enable being a system-power-controller

On Fri, 24 Jan 2020, Adam Thomson wrote:
> On 24 January 2020 08:53, Helmut Grohne wrote:
> > On Thu, Jan 23, 2020 at 04:51:37PM +0100, Adam Thomson wrote:
> > > I have concerns about using regmap/I2C within the pm_power_off() callback
> > > function although I am aware there are other examples of this in the kernel. At
> > > the point that is called I believe IRQs are disabled so it would require a
> > > platform to have an atomic version of the I2C bus's xfer function. Don't know
> > > if there's a check to see if the bus supports this, but if not then maybe it's
> > > something worth adding? That way we can then only support the
> > pm_power_off()
> > > approach on systems which can actually do it.
> > 
> > On arm, machine_power_off calls the pm_power_off callback after issuing
> > local_irq_disable() and smp_send_stop(). So I think your intuition is
> > correct that we are running with only one CPU left with IRQs disabled.
> > 
> > I have tested this code on a board with an i2c-cadence bus. This driver
> > seems to use IRQs for completion tracking with no fallback to polling.
> > I'm now puzzled as to why this works at all. Given that I'm using
> > regmap_update_bits on a volatile register, it would have to complete the
> > read before performing the relevant write. Nevertheless, it reliably
> > turns off here. So I'm starting to wonder whether there is a flaw in the
> > analysis.
> > 
> > I also looked into whether linux/i2c.h would tell us about the
> > availability of an atomic xfer function. Indeed, the i2c_algorithm
> > structure has a master_xfer_atomic specifically for this purpose. The
> > i2c core will automatically use this function when irqs are disabled.
> > Unfortunately, very few buses implement this function. In particular,
> > i2c-cadence lacks it.
> > 
> > So I could check for i2c_dev->adapter->algo->master_xfer_atomic != NULL
> > indeed. Possibly this could be wrapped in a central inline function.
> 
> Yes, I'd be tempted to make this a nice wrapper function to hide the
> particulars, were someone to implement this.
> 
> > 
> > I concur that quite a few other drivers use a regmap/i2c from their
> > pm_power_off hook. Examples include:
> >  * arch/powerpc/platforms/83xx/mcu_mpc8349emitx.c (i2c without regmap)
> >  * drivers/mfd/axp20x.c (regmap without i2c)
> >  * drivers/mfd/dm355evm_msp.c (i2c without regmap)
> >  * drivers/mfd/max77620.c (regmap and i2c)
> >  * drivers/mfd/max8907.c (regmap and i2c)
> >  * drivers/mfd/palmas.c (regmap and i2c)
> >  * drivers/mfd/retu-mfd.c (regmap and i2c)
> >  * drivers/mfd/rn5t618.c (regmap and i2c)
> >  * drivers/mfd/tps6586x.c (regmap and i2c)
> >  * drivers/mfd/tps65910.c (regmap and i2c)
> >  * drivers/mfd/tps80031.c (regmap and i2c)
> >  * drivers/mfd/twl4030-power.c (i2c without regmap)
> >  * drivers/regulator/act8865-regulator.c (regmap and i2c)
> > 
> > For this reason, I think the practice of using regmap/i2c within
> > pm_power_off is well-established and should not be questioned for an
> > individual device. In addition, the relevant functionality must be
> > explicitly requested by modifying a board-specific device-tree. It can
> > be assumed that an integrator would test whether the mfd actually works
> > as a power controller when adding the relevant property. Given that we
> > turned off other CPUs and IRQs, the behaviour should be fairly reliable.
> 
> I never like assumptions and they tend to catch people out. A lot of the time
> driver developers will use another as a template/example and so the same
> possible mistakes can be duplicated. I don't know for certain these are mistakes
> but the code seems to indicate that could be the case, and there's a good
> reason that atomic I2C transfer code has been added into the kernel. I also
> prefer code that helps people identify where a problem might lie so having a
> check for I2C atomic support would be useful to indicate if there is a problem.
> 
> Lee, do you have any further insight into any of this? Am I barking up the
> wrong tree here?

Not off-hand, sorry.  I would have to do a deep-dive to figure it
out for myself.

Maybe this is where Mark and/or Wolfram (Cc'ed) have some knowledge.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Powered by blists - more mailing lists