Received-SPF: pass (google.com: domain of linux-kernel+bounces-19089-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
From: JiSheng Teoh <jisheng.teoh@starfivetech.com>
To: "andi.shyti@kernel.org" <andi.shyti@kernel.org>
CC: JiSheng Teoh <jisheng.teoh@starfivetech.com>, Leyfoon Tan
	<leyfoon.tan@starfivetech.com>, "linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>, "linux-i2c@vger.kernel.org"
	<linux-i2c@vger.kernel.org>, "linux-kernel@vger.kernel.org"
	<linux-kernel@vger.kernel.org>, "michal.simek@amd.com" <michal.simek@amd.com>
Subject: RE: [PATCH v2] i2c: cadence: Add system suspend and resume PM support
Thread-Topic: [PATCH v2] i2c: cadence: Add system suspend and resume PM
 support
Thread-Index: AQHaKqHICsIFtOoPykiU4PTXtMLy1rCg6VYAgACMaYCAAG4PAIAASqKAgC0BRBA=
Date: Sun, 7 Jan 2024 23:47:17 +0000
Message-ID:
 <ZQ0PR01MB1160E73BE852925A270DBE95EB64A@ZQ0PR01MB1160.CHNPR01.prod.partner.outlook.cn>
References: <20231210115412.hztjrubacnmenbvz@zenone.zhora.eu>
 <20231210162119.364-1-jisheng.teoh@starfivetech.com>
In-Reply-To: <20231210162119.364-1-jisheng.teoh@starfivetech.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0:
 =?us-ascii?Q?lhBfLWmWMeCJEu2l1id5B8mF01sWaYdf9NxdkvfUYIV8Qt5y49HKIvdcIi5k?=
 =?us-ascii?Q?oFqJa/mL3cfPvGEuHC71AHL5abohKGcDUAfX/CsayXoOsvkuC9T6cGy3OQBE?=
 =?us-ascii?Q?1zZxp3IwDBxHc9aB60jDxllg0TKrm9op8aVCpgRgc6kuZlBoXs3FJGQpyMPh?=
 =?us-ascii?Q?+rSXj41H0rAe7K5XDZ7MCTb0i2WfmiPxh8JSnGP1nANVtbS3F5+o/6n4i/5n?=
 =?us-ascii?Q?HUcR/fCiLxCvIVoJZOduqBLL4e+D9SXFyQ1IWPZbN2qfa0nCgWSb7KEK/rHh?=
 =?us-ascii?Q?6WDZhmRxLtaX5YSi3hO7gIGjZ4fhHThsNOmxChTwzh5x4YD3za8DWezJUSn9?=
 =?us-ascii?Q?nMpwx/eYAHZAbMHkCPSF7NjWHwUYh/ega8xFRre8Y+/UHmfosm1c3+mqNxtn?=
 =?us-ascii?Q?WZOQpXlP62jDIgvls39rA5tRon/AbtONsghzClLlqa/ddR6SmwuqBEEmWQXM?=
 =?us-ascii?Q?Pt8B9pkxU31kQAoh7KwcHKKifravOTKh4Z0RBwp0MLo6utLO/3sHIYDcUmBH?=
 =?us-ascii?Q?SXAOqHEkpHQrvbq6S/gOFfL53xBWfyODkrOCmbzslUP62Fcn/ObPy4GP9vng?=
 =?us-ascii?Q?HMJ88m9hdCTTfpP5jp2KSBDavjDBvY6qVHK016+yXgGSkD7A+mk/aoM97s6w?=
 =?us-ascii?Q?fhhU0pwmyOglNXn3LEjA5u11EbDJQex2Lx8YvXqwNmu1A69sGCwOIjMOWMpt?=
 =?us-ascii?Q?TM8ha4NuIh7dqdr+dZWrElzqIrYy5Rsw27ERCXZz91rD5gRceNPqvBssrMmF?=
 =?us-ascii?Q?nlhMAO/P7utimNeP95Jf9D3QyzhQATSE2TNq4KqMDFBUrsKudlvLg1fnbOxk?=
 =?us-ascii?Q?kOB/xOsW1GfyfT6TIYQe/3Y/WBrlrxLSgC+vA7okPJvFmVag1myOGcN1XsbE?=
 =?us-ascii?Q?Jl1yH5OPUMoknbvxvNwpPxkqdUuQRedI8mU2RSDUrsYnNfmW7hYryhBMcL5g?=
 =?us-ascii?Q?VDqrFxEqlz3lYvuTg61bUfmnmkkWySRlUxu2wjZsRq4CrJ11flFHzNJHHwfD?=
 =?us-ascii?Q?i42RU1QEVU6eh8w3EGeKNND15sHBUDXcShHXj40BuWckDsix6IGhZQNdw88+?=
 =?us-ascii?Q?HQLMKvDveoifHS+/iTQYp4cLlUJ3aj0BEJy9prQ7gDp8P97fsJ6GiD/tNNjV?=
 =?us-ascii?Q?9QmjBuiTnqTdDE1Ii0/DsDFocTFsCVFmEbpcUp5HDGtWYF8ApoKrmSglioTm?=
 =?us-ascii?Q?1NHfZ1uBZpdyOJKmCNQSDK3kXbDLQ+giq7CsvbOPYCCMFzi/+0gzEAtf7umm?=
 =?us-ascii?Q?F+xXLujsi5jDhF9PmChZ107Ew9g1pmCh8Y2VgYRKeZFPnKDL8hxU/HxXCaqp?=
 =?us-ascii?Q?mr5AwV8qAIYG9NsGQfd54dBgAGupSQhfGMr7ORuniSsESlMYSBSEcWZYTXd4?=
 =?us-ascii?Q?ykEroGFBtW3OwK/qPT2XLV1dM+FxL7UoiJnGGov2QMw3YrbtXvNvwFJoHcdn?=
 =?us-ascii?Q?PTjsyae+EDmlnUPpuEOkLX6BPJuQSLLuUXS9mcuGPzUFo9uokhBCj+BH586E?=
 =?us-ascii?Q?xBgR7+eZ8DUD3R2yUIL+JmluIDKMFrNq7Yq2ZRBSIekoDrrhbFFQuVH0PTLa?=
 =?us-ascii?Q?nimAJhDkPvtjWumrJa8S+jjUw81ghEqfNJ+NQ9zAavES1o86/2nTiGDx5I3B?=
 =?us-ascii?Q?zw=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
X-Mailing-List: linux-kernel@vger.kernel.org
List-Id: <linux-kernel.vger.kernel.org>
List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org>
List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org>
MIME-Version: 1.0
X-OriginatorOrg: starfivetech.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: ZQ0PR01MB1160.CHNPR01.prod.partner.outlook.cn
X-MS-Exchange-CrossTenant-Network-Message-Id: e284a4bf-755f-4a45-2744-08dc0fdafb3f
X-MS-Exchange-CrossTenant-originalarrivaltime: 07 Jan 2024 23:47:17.1897
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 06fe3fa3-1221-43d3-861b-5a4ee687a85c
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: fB+lhLyfQ7TSY5YmZhV92G+A1udROa3hf7NtmQQz4zlNKcA1RrT84XoaoREhf9T1GHyWSgPjHUaTTF7aP2a4u+QMlav4+sg96Q7H/SRvS1c=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: ZQ0PR01MB1096

Hi Andi,

> On Sun, 10 Dec 2023 12:54:12 +0100
> Andi Shyti <andi.shyti@kernel.org> wrote:
>=20
> > Hi Ji Sheng,
> >
> > [...]
> >
> > > > > +static int __maybe_unused cdns_i2c_resume(struct device *dev) {
> > > >
> > > > I am not really understanding what you are trying to do here:
> > > >
> > > > > +	struct cdns_i2c *xi2c =3D dev_get_drvdata(dev);
> > > > > +	int err;
> > > > > +
> > > > > +	err =3D cdns_i2c_runtime_resume(dev);
> > > >
> > > > First you try to resume...
> > > >
> > > > > +	if (err)
> > > > > +		return err;
> > > > > +
> > > > > +	if (pm_runtime_status_suspended(dev)) {
> > > >
> > > > ... then you check if you are suspended ...
> > >
> > > This serves as a check and balance to ensure that when the system
> > > resumes with device in runtime suspend state, we disable the clock
> > > enabled in earlier cdns_i2c_runtime_resume() to ensure a balanced
> > > clock reference count for subsequent runtime resume transition.
> > > Similar implementation can be found in this commit:
> > > https://github.com/torvalds/linux/commit/44c99904cf61f945d02ac9976ab
> > > 10dd5ccaea393
> > >
> >
> > OK, this is done purely for clock balancing, but then, I still don't
> > understand the case. I expect the clock counter to be unbalanced when
> > you suspend (because is moving towards '0').
> >
> > While, if you check if the clock is unbalanced when resuming, it means
> > that the clock had a negative counter (which is impossible because the
> > clock counter is unsigned).
> >
> > If there is any unbalancing at this stage, then I recommend you to
> > check what has happened previously.
> >
> > ... Or is there anything I am missing?
> >
> > Thanks,
> > Andi
>=20
> You are right, the clock counter will move towards 0 during system suspen=
d.
> Conversely during system resume, the clock counter is incremented to 1 ea=
rly on in cdns_i2c_runtime_resume(). So the clock counter
> is not negative to start with.
> At this point of time, clock counter is 1. If the device is in runtime su=
spend, we decrement the clock counter back to 0, so the
> subsequent runtime resume could increment it back to 1. In a sense, balan=
cing the clock counter.
> Please help correct me if I've got it wrong.
>=20
Just to check on the status of this patch. Let me know if the above stateme=
nt makes sense.
> >
> > > > > +		err =3D cdns_i2c_runtime_suspend(dev);
> > > >
> > > > ... and suspend again? Shouldn't this be _resume()?
> > > >
> > > > Thanks,
> > > > Andi
> > > >
> > > > > +		if (err)
> > > > > +			return err;
> > > > > +	}
> > > > > +
> > > > > +	i2c_mark_adapter_resumed(&xi2c->adap);
> > > > > +
> > > > > +	return 0;
> > > > > +}
>=20