Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
To:     Helmut Grohne <helmut.grohne@intenta.de>,
        Lee Jones <lee.jones@linaro.org>,
        Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
CC:     Support Opensource <Support.Opensource@diasemi.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH 1/2] mfd: da9062: enable being a system-power-controller
Thread-Topic: [PATCH 1/2] mfd: da9062: enable being a system-power-controller
Thread-Index: AQHVxVLd8ZSwK+YMLk+kKi398FPZzKf4e+UAgAEg3QCAABbyAA==
Date:   Fri, 24 Jan 2020 10:17:19 +0000
Message-ID: <AM6PR10MB22636902FCF576272AC8F373800E0@AM6PR10MB2263.EURPRD10.PROD.OUTLOOK.COM>
References: <20200107120559.GA700@laureti-dev>
 <AM6PR10MB226306BDE8575CED80071148800F0@AM6PR10MB2263.EURPRD10.PROD.OUTLOOK.COM>
 <20200124085310.GA27231@laureti-dev>
In-Reply-To: <20200124085310.GA27231@laureti-dev>
Accept-Language: en-GB, en-US
Content-Language: en-US
x-ms-exchange-messagesentrepresentingtype: 1
x-originating-ip: [165.225.80.228]
x-ms-publictraffictype: Email
x-ms-office365-filtering-correlation-id: 443898b1-66c8-4b73-993f-08d7a0b6981d
x-ms-traffictypediagnostic: AM6PR10MB3365:
x-ms-exchange-sharedmailbox-routingagent-processed: True
x-ms-exchange-transport-forked: True
x-microsoft-antispam-prvs: <AM6PR10MB33652DB8AC1E858B019523E5A70E0@AM6PR10MB3365.EURPRD10.PROD.OUTLOOK.COM>
x-ms-oob-tlc-oobclassifiers: OLM:10000;
x-forefront-prvs: 02929ECF07
x-forefront-antispam-report: SFV:NSPM;SFS:(10009020)(4636009)(396003)(136003)(366004)(346002)(376002)(39850400004)(199004)(189003)(33656002)(54906003)(110136005)(55016002)(7696005)(76116006)(5660300002)(8676002)(9686003)(81166006)(81156014)(66946007)(316002)(52536014)(86362001)(66476007)(66446008)(64756008)(66556008)(186003)(2906002)(8936002)(55236004)(71200400001)(53546011)(478600001)(26005)(6506007)(4326008);DIR:OUT;SFP:1101;SCL:1;SRVR:AM6PR10MB3365;H:AM6PR10MB2263.EURPRD10.PROD.OUTLOOK.COM;FPR:;SPF:None;LANG:en;PTR:InfoNoRecords;A:0;MX:1;
received-spf: None (protection.outlook.com: diasemi.com does not designate
 permitted sender hosts)
x-ms-exchange-senderadcheck: 1
x-microsoft-antispam: BCL:0;
x-microsoft-antispam-message-info: LY6//+jDGaOGKI5y56oPivKPF2tj/r2Y2a4D31u0f9p7cF06nZUApoO4V1clD+440f7JUbzsf9KOP9ME3lMM37vEJolsjZZl3pIvpQYw+X48+xhtYmdn6xHXkbTB0yreu0iWebLRzN+IvWj0NdJnf31cZDuDgIMLbFKMTC5ocrwfXPt/Lvpo46dAfgc/nSaz+5Gb9SxnXnVA8mPv99Q+ju0pqRE/PYXQPafgFHrkbdCvlO5s41JphamD74d/k/Dy7DacIBcJghfyDhoOxdrosbx1Q7XEz1zuDYO6IaC8sNpyLkzo4DcIrQvHDLzxsxEXcwN2BHivZuRl30gPVtUpQ67aIoWUKvsoyVL/xlCFgYZsQwmMd/Lu8yzbBpd1JbfP5bjFPBuN6lWwUvS8iKeocAZOaLSgT4xWsAl7ZEsUL6/Btj89TjZTS6T28CmC/U37
x-ms-exchange-antispam-messagedata: w/Uv0ayL1d1c40VUrwdc20OXe7N17cJFI8nS3Sr25RwF8VedMUqQnBeltBN1ETgtTEG+JJ4ZHLtTbiCnRKk7pJ/6aNECVTHRaq8C0aYtKh7N/CIlqjDEdBHJkj5naOdmp4nn78kMiGww5tP+3DFqFA==
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: diasemi.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 443898b1-66c8-4b73-993f-08d7a0b6981d
X-MS-Exchange-CrossTenant-originalarrivaltime: 24 Jan 2020 10:17:19.2193
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 511e3c0e-ee96-486e-a2ec-e272ffa37b7c
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: X4UboVo4ZsA9dK6NzYZWWUcuN+2ZM9EsSC5KA7oMX6c9GOsLxtSX7OOqEZ8NTwPJTnLMK++EtNikKx7LnDe30+h+wWp65QTWOv8EdDk2CRc=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM6PR10MB3365
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 24 January 2020 08:53, Helmut Grohne wrote:

> Hi,
>=20
> Thank you for reviewing the code.
>=20
> 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() callba=
ck
> > function although I am aware there are other examples of this in the ke=
rnel. At
> > the point that is called I believe IRQs are disabled so it would requir=
e 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 may=
be it's
> > something worth adding? That way we can then only support the
> pm_power_off()
> > approach on systems which can actually do it.
>=20
> 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.
>=20
> 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.
>=20
> 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.
>=20
> So I could check for i2c_dev->adapter->algo->master_xfer_atomic !=3D 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.

>=20
> 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)
>=20
> 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 ti=
me
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 mis=
takes
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 prob=
lem.

Lee, do you have any further insight into any of this? Am I barking up the
wrong tree here?

>=20
> I think that requiring atomic transfers for pm_power_off would be
> relatively easy to implement (for all mfds). However, I also think that
> it would break a fair number of boards, because so few buses implement
> atomic transfers. As such, I don't think we can actually require it
> before requiring all buses to implement atomic transfers. At that point,
> the check becomes useless, because the i2c core will automatically use
> atomic transfers during pm_power_off.
>=20
> Given these reasons (consistency with other drivers, testing and "don't
> break"), I think that including the functionality without an additional
> check is a reasonable thing to do.
>=20
> Helmut