| Message ID | 20210317160410.2097178-1-m.tretter@pengutronix.de (mailing list archive) |
|---|---|
| Headers | show |
| Series | soc: xilinx: pm_domains: cleanup and fix PM_INIT_FINALIZE | expand |
Hi Michael, Thanks for the patch. > -----Original Message----- > From: Michael Tretter <m.tretter@pengutronix.de> > Sent: 17 March 2021 09:34 PM > To: linux-arm-kernel@lists.infradead.org > Cc: Michal Simek <michals@xilinx.com>; Rajan Vaja <RAJANV@xilinx.com>; Jolly > Shah <JOLLYS@xilinx.com>; m.tretter@pengutronix.de > Subject: [PATCH 0/4] soc: xilinx: pm_domains: cleanup and fix PM_INIT_FINALIZE > > CAUTION: This message has originated from an External Source. Please use proper > judgment and caution when opening attachments, clicking links, or responding to > this email. > > > Hi, > > Patch 1 of this series fixes the ZynqMP PMU FW power management > initialization, which was done by the wrong driver. PM_INIT_FINALIZE must be > called from the zynqmp_pm_domains driver, which handles power domains, instead > of the zynmp_power driver, which is responsible for suspend and shutdown. [Rajan] I am fine with moving to genpd but zynqmp_pm_init_finalize() needs to be late call. zynqmp_pm_init_finalize() should be called when Linux has requested all the devices through genpd driver. Making it late call will make sure it. > > Patches 2 to 4 are various cleanup patches to improve the readability and > debugging experience of the zynqmp_pm_domains driver. > > Michael > > Michael Tretter (4): > soc: xilinx: move PM_INIT_FINALIZE to zynqmp_pm_domains driver > soc: xilinx: cleanup debug and error messages > soc: xilinx: use a properly named field instead of flags > soc: xilinx: add a to_zynqmp_pm_domain macro > > drivers/soc/xilinx/zynqmp_pm_domains.c | 79 +++++++++++++------------- > drivers/soc/xilinx/zynqmp_power.c | 1 - > 2 files changed, 38 insertions(+), 42 deletions(-) > > -- > 2.29.2
Hi Rajan, On Thu, 15 Apr 2021 16:27:58 +0000, Rajan Vaja wrote: > Thanks for the patch. > > > -----Original Message----- > > From: Michael Tretter <m.tretter@pengutronix.de> > > Sent: 17 March 2021 09:34 PM > > To: linux-arm-kernel@lists.infradead.org > > Cc: Michal Simek <michals@xilinx.com>; Rajan Vaja <RAJANV@xilinx.com>; Jolly > > Shah <JOLLYS@xilinx.com>; m.tretter@pengutronix.de > > Subject: [PATCH 0/4] soc: xilinx: pm_domains: cleanup and fix PM_INIT_FINALIZE > > > > Patch 1 of this series fixes the ZynqMP PMU FW power management > > initialization, which was done by the wrong driver. PM_INIT_FINALIZE must be > > called from the zynqmp_pm_domains driver, which handles power domains, instead > > of the zynmp_power driver, which is responsible for suspend and shutdown. > [Rajan] I am fine with moving to genpd but zynqmp_pm_init_finalize() needs to be late call. > zynqmp_pm_init_finalize() should be called when Linux has requested all the devices through > genpd driver. Making it late call will make sure it. What is the reason why all devices have to be requested before calling zynqmp_pm_init_finalize()? I was expecting that calling PM_INIT_FINALIZE only would tell the PMU_FW that Linux is using the PM API and the PMU_FW should power down/up PM slaves as requested by Linux. It is somewhat surprising that this isn't the case and all PM slaves have to be powered up before calling PM_INIT_FINALIZE. What would happen if some driver is built as a module? In that case, the module would be loaded and request the pm node only after PM_INIT_FINALIZE was called. Do we have to avoid/disallow such cases? For USB, I am actually observing a similar situation: If I do not request the USB PM slave before I call PM_INIT_FINALIZE, I see communication errors with connected USB devices. Could this be related? Thanks, Michael > > > > > Patches 2 to 4 are various cleanup patches to improve the readability and > > debugging experience of the zynqmp_pm_domains driver. > > > > Michael > > > > Michael Tretter (4): > > soc: xilinx: move PM_INIT_FINALIZE to zynqmp_pm_domains driver > > soc: xilinx: cleanup debug and error messages > > soc: xilinx: use a properly named field instead of flags > > soc: xilinx: add a to_zynqmp_pm_domain macro > > > > drivers/soc/xilinx/zynqmp_pm_domains.c | 79 +++++++++++++------------- > > drivers/soc/xilinx/zynqmp_power.c | 1 - > > 2 files changed, 38 insertions(+), 42 deletions(-) > > > > -- > > 2.29.2
Hi Michal, > -----Original Message----- > From: Michael Tretter <m.tretter@pengutronix.de> > Sent: 19 April 2021 01:03 PM > To: Rajan Vaja <RAJANV@xilinx.com> > Cc: linux-arm-kernel@lists.infradead.org; Michal Simek <michals@xilinx.com>; Jolly > Shah <JOLLYS@xilinx.com> > Subject: Re: [PATCH 0/4] soc: xilinx: pm_domains: cleanup and fix > PM_INIT_FINALIZE > > Hi Rajan, > > On Thu, 15 Apr 2021 16:27:58 +0000, Rajan Vaja wrote: > > Thanks for the patch. > > > > > -----Original Message----- > > > From: Michael Tretter <m.tretter@pengutronix.de> > > > Sent: 17 March 2021 09:34 PM > > > To: linux-arm-kernel@lists.infradead.org > > > Cc: Michal Simek <michals@xilinx.com>; Rajan Vaja <RAJANV@xilinx.com>; Jolly > > > Shah <JOLLYS@xilinx.com>; m.tretter@pengutronix.de > > > Subject: [PATCH 0/4] soc: xilinx: pm_domains: cleanup and fix > PM_INIT_FINALIZE > > > > > > Patch 1 of this series fixes the ZynqMP PMU FW power management > > > initialization, which was done by the wrong driver. PM_INIT_FINALIZE must be > > > called from the zynqmp_pm_domains driver, which handles power domains, > instead > > > of the zynmp_power driver, which is responsible for suspend and shutdown. > > [Rajan] I am fine with moving to genpd but zynqmp_pm_init_finalize() needs to > be late call. > > zynqmp_pm_init_finalize() should be called when Linux has requested all the > devices through > > genpd driver. Making it late call will make sure it. > > What is the reason why all devices have to be requested before calling > zynqmp_pm_init_finalize()? [Rajan] This is required if device is not to be powered down. If zynqmp_pm_init_finalize() Is called before requesting, device may go power down. So settings done in probe() may be lost. In this case, driver needs to do config during runtime resume. However, some driver may not do all config during runtime resume which may not work. > > I was expecting that calling PM_INIT_FINALIZE only would tell the PMU_FW that > Linux is using the PM API and the PMU_FW should power down/up PM slaves as > requested by Linux. It is somewhat surprising that this isn't the case and all > PM slaves have to be powered up before calling PM_INIT_FINALIZE. [Rajan] Basically PMUFW will do power up/down as per request. However, device config may be lost due to this power down. So device driver mat need to take care during runtime suspend/resume > > What would happen if some driver is built as a module? In that case, the > module would be loaded and request the pm node only after PM_INIT_FINALIZE > was > called. Do we have to avoid/disallow such cases? [Rajan] We allow that. When driver is init, it needs to setup device which will be fine As probe do most of settings. > > For USB, I am actually observing a similar situation: If I do not request the > USB PM slave before I call PM_INIT_FINALIZE, I see communication errors with > connected USB devices. Could this be related? [Rajan] Yes, device config might have lost due to power down because of init finalize call. Thanks, Rajan > > Thanks, > > Michael > > > > > > > > > Patches 2 to 4 are various cleanup patches to improve the readability and > > > debugging experience of the zynqmp_pm_domains driver. > > > > > > Michael > > > > > > Michael Tretter (4): > > > soc: xilinx: move PM_INIT_FINALIZE to zynqmp_pm_domains driver > > > soc: xilinx: cleanup debug and error messages > > > soc: xilinx: use a properly named field instead of flags > > > soc: xilinx: add a to_zynqmp_pm_domain macro > > > > > > drivers/soc/xilinx/zynqmp_pm_domains.c | 79 +++++++++++++------------- > > > drivers/soc/xilinx/zynqmp_power.c | 1 - > > > 2 files changed, 38 insertions(+), 42 deletions(-) > > > > > > -- > > > 2.29.2
On Mon, Apr 19, 2021 at 09:32:39AM +0200, Michael Tretter wrote: Sorry for chiming in randomly. I always though the way PM_INIT_FINALIZE is designed has issues(e.g. racy). I was involved in discussion with Xilinx when we will designing more generic version of EEMI - SCMI which is now supported in upstream. EEMI was in production already when we started on SCMI 3-4 years back and wanted to get feedback. [...] > > What is the reason why all devices have to be requested before calling > zynqmp_pm_init_finalize()? > Yes that is wrong assumption/expectation from the firmware. > I was expecting that calling PM_INIT_FINALIZE only would tell the PMU_FW that > Linux is using the PM API and the PMU_FW should power down/up PM slaves as > requested by Linux. It is somewhat surprising that this isn't the case and all > PM slaves have to be powered up before calling PM_INIT_FINALIZE. > Agreed that was my understanding too. > What would happen if some driver is built as a module? In that case, the > module would be loaded and request the pm node only after PM_INIT_FINALIZE was > called. Do we have to avoid/disallow such cases? > I was told it will work. But it will be always racy if there are multiple channels to talk to firmware. My argument firmware can turn off all the devices before giving control to OS and no need for that. But there is some boot time optimisation possible I am told which I could well be. But this interface for too racy IMO, just happens to be fine with limited configurations it operates in. -- Regards, Sudeep
Hi, On 4/20/21 4:18 PM, Sudeep Holla wrote: > On Mon, Apr 19, 2021 at 09:32:39AM +0200, Michael Tretter wrote: > > Sorry for chiming in randomly. I always though the way PM_INIT_FINALIZE > is designed has issues(e.g. racy). I was involved in discussion with > Xilinx when we will designing more generic version of EEMI - SCMI > which is now supported in upstream. EEMI was in production already when > we started on SCMI 3-4 years back and wanted to get feedback. > > [...] > >> >> What is the reason why all devices have to be requested before calling >> zynqmp_pm_init_finalize()? >> > > Yes that is wrong assumption/expectation from the firmware. > >> I was expecting that calling PM_INIT_FINALIZE only would tell the PMU_FW that >> Linux is using the PM API and the PMU_FW should power down/up PM slaves as >> requested by Linux. It is somewhat surprising that this isn't the case and all >> PM slaves have to be powered up before calling PM_INIT_FINALIZE. >> > > Agreed that was my understanding too. > >> What would happen if some driver is built as a module? In that case, the >> module would be loaded and request the pm node only after PM_INIT_FINALIZE was >> called. Do we have to avoid/disallow such cases? >> > > I was told it will work. But it will be always racy if there are multiple > channels to talk to firmware. > > My argument firmware can turn off all the devices before giving control > to OS and no need for that. But there is some boot time optimisation > possible I am told which I could well be. But this interface for too > racy IMO, just happens to be fine with limited configurations it operates > in. Rajan: Can you please do deep dive to this in pmufw and try to figured it out how to fix this on firmware side? Thanks, Michal
Hi Rajan, On Wed, 28 Apr 2021 15:17:25 +0200, Michal Simek wrote: > On 4/20/21 4:18 PM, Sudeep Holla wrote: > > On Mon, Apr 19, 2021 at 09:32:39AM +0200, Michael Tretter wrote: > > > > Sorry for chiming in randomly. I always though the way PM_INIT_FINALIZE > > is designed has issues(e.g. racy). I was involved in discussion with > > Xilinx when we will designing more generic version of EEMI - SCMI > > which is now supported in upstream. EEMI was in production already when > > we started on SCMI 3-4 years back and wanted to get feedback. Is it possible to use SCMI on the ZynqMP? I guess no, as I couldn't find any code that would make this possible. Correct? > > > > [...] > > > >> > >> What is the reason why all devices have to be requested before calling > >> zynqmp_pm_init_finalize()? > >> > > > > Yes that is wrong assumption/expectation from the firmware. > > > >> I was expecting that calling PM_INIT_FINALIZE only would tell the PMU_FW that > >> Linux is using the PM API and the PMU_FW should power down/up PM slaves as > >> requested by Linux. It is somewhat surprising that this isn't the case and all > >> PM slaves have to be powered up before calling PM_INIT_FINALIZE. > >> > > > > Agreed that was my understanding too. > > > >> What would happen if some driver is built as a module? In that case, the > >> module would be loaded and request the pm node only after PM_INIT_FINALIZE was > >> called. Do we have to avoid/disallow such cases? > >> > > > > I was told it will work. But it will be always racy if there are multiple > > channels to talk to firmware. > > > > My argument firmware can turn off all the devices before giving control > > to OS and no need for that. But there is some boot time optimisation > > possible I am told which I could well be. But this interface for too > > racy IMO, just happens to be fine with limited configurations it operates > > in. > > Rajan: Can you please do deep dive to this in pmufw and try to figured > it out how to fix this on firmware side? Did you have time to look into this? There are 3 more cleanup patches in this series. Are there any objections against these patches? I think the other patches are still useful by themselves. Michael
Hi Michael, > -----Original Message----- > From: Michael Tretter <m.tretter@pengutronix.de> > Sent: 01 June 2021 02:19 PM > To: Rajan Vaja <RAJANV@xilinx.com>; Michal Simek <michals@xilinx.com> > Cc: Sudeep Holla <sudeep.holla@arm.com>; linux-arm-kernel@lists.infradead.org; > kernel@pengutronix.de > Subject: Re: [PATCH 0/4] soc: xilinx: pm_domains: cleanup and fix > PM_INIT_FINALIZE > > Hi Rajan, > > On Wed, 28 Apr 2021 15:17:25 +0200, Michal Simek wrote: > > On 4/20/21 4:18 PM, Sudeep Holla wrote: > > > On Mon, Apr 19, 2021 at 09:32:39AM +0200, Michael Tretter wrote: > > > > > > Sorry for chiming in randomly. I always though the way PM_INIT_FINALIZE > > > is designed has issues(e.g. racy). I was involved in discussion with > > > Xilinx when we will designing more generic version of EEMI - SCMI > > > which is now supported in upstream. EEMI was in production already when > > > we started on SCMI 3-4 years back and wanted to get feedback. > > Is it possible to use SCMI on the ZynqMP? I guess no, as I couldn't find any > code that would make this possible. Correct? [Rajan] Yes. > > > > > > > [...] > > > > > >> > > >> What is the reason why all devices have to be requested before calling > > >> zynqmp_pm_init_finalize()? > > >> > > > > > > Yes that is wrong assumption/expectation from the firmware. > > > > > >> I was expecting that calling PM_INIT_FINALIZE only would tell the PMU_FW > that > > >> Linux is using the PM API and the PMU_FW should power down/up PM slaves > as > > >> requested by Linux. It is somewhat surprising that this isn't the case and all > > >> PM slaves have to be powered up before calling PM_INIT_FINALIZE. > > >> > > > > > > Agreed that was my understanding too. > > > > > >> What would happen if some driver is built as a module? In that case, the > > >> module would be loaded and request the pm node only after > PM_INIT_FINALIZE was > > >> called. Do we have to avoid/disallow such cases? > > >> > > > > > > I was told it will work. But it will be always racy if there are multiple > > > channels to talk to firmware. > > > > > > My argument firmware can turn off all the devices before giving control > > > to OS and no need for that. But there is some boot time optimisation > > > possible I am told which I could well be. But this interface for too > > > racy IMO, just happens to be fine with limited configurations it operates > > > in. > > > > Rajan: Can you please do deep dive to this in pmufw and try to figured > > it out how to fix this on firmware side? > > Did you have time to look into this? > > There are 3 more cleanup patches in this series. Are there any objections > against these patches? I think the other patches are still useful by > themselves. [Rajan] I am okay with this however, pm_init_finalize() needs to be late call. It can be taken later as it is not late call right now in mainline. > > Michael