Message-ID: <aXzmN2JiopDi+INX@lizhi-Precision-Tower-5810>
Date: Fri, 30 Jan 2026 12:11:19 -0500
From: Frank Li <Frank.li@....com>
To: Adrian Hunter <adrian.hunter@...el.com>
Cc: alexandre.belloni@...tlin.com, linux-i3c@...ts.infradead.org,
	linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org
Subject: Re: [PATCH 5/7] i3c: mipi-i3c-hci: Allow parent to manage runtime PM

On Fri, Jan 30, 2026 at 06:34:37PM +0200, Adrian Hunter wrote:
> On 30/01/2026 17:04, Frank Li wrote:
> > On Fri, Jan 30, 2026 at 09:00:33AM +0200, Adrian Hunter wrote:
> >> On 29/01/2026 23:00, Frank Li wrote:
> >>> On Thu, Jan 29, 2026 at 10:28:14PM +0200, Adrian Hunter wrote:
> >>>> On 29/01/2026 22:00, Frank Li wrote:
> >>>>> On Thu, Jan 29, 2026 at 08:18:39PM +0200, Adrian Hunter wrote:
> >>>>>> Some platforms implement the MIPI I3C HCI Multi-Bus Instance capability,
> >>>>>> where a single parent device hosts multiple I3C controller instances.  In
> >>>>>> such designs, the parent - not the individual child instances - may need to
> >>>>>> coordinate runtime PM so that all controllers enter low-power states
> >>>>>> together, and all runtime suspend callbacks are invoked in a controlled
> >>>>>> and synchronized manner.
> >>>>>>
> >>>>>> For example, if the parent enables IBI-wakeup when transitioning into a
> >>>>>> low-power state,
> >>>>>
> >>>>> Does your hardware support recieve IBI when runtime suspend?
> >>>>
> >>>> When runtime suspended (in D3), the hardware first triggers a Power Management
> >>>> Event (PME) when the SDA line is pulled low to signal the START condition of an IBI.
> >>>> The PCI subsystem will then runtime-resume the device.  When the bus is enabled,
> >>>> the clock is started and the IBI is received.
> >>>
> >>> It align my assumption, why need complex solution.
> >>>
> >>> SDA->PME->IRQ should handle by hardware, so irq handle queue IBI to working
> >>> queue.
> >>>
> >>> IBI work will try do transfer, which will call runtime resume(), then
> >>> transfer data.
> >>>
> >>> What's issue?
> >>
> >> The PME indicates I3C START (SDA line pulled low).  The controller is
> >> in a low power state unable to operate the bus.  At this point it is not
> >> known what I3C device has pulled down the SDA line, or even if it is an
> >> IBI since it is indistinguishable from hot-join at this point.
> >>
> >> The PCI PME IRQ is not the device's IRQ.  It is handled by acpi_irq()
> >> which ultimately informs the PCI subsystem to wake the PCI device.
> >> The PCI subsystem performs pm_request_resume(), refer pci_acpi_wake_dev().
> >>
> >> When the controller is resumed, it enables the I3C bus and the IBI is
> >> finally delivered normally.
> >>
> >> However, none of that is related to this patch.
> >>
> >> This patch is because the PCI device has 2 I3C bus instances and only 1 PME
> >> wakeup.  The PME becomes active when the PCI device is put to a low
> >> power state.
> >
> > One instance 1 suspend, instance 2 running, PME is inactive, what's happen
> > if instance 1 request IBI?
>
> Nothing will happen.  Instance 1 I3C bus is not operational and there can
> be no PME when the PCI device is not in a low power state (D3hot)
>
> >
> > IBI will be missed?
>
> Possibly not if instance 1 is eventually resumed and the I3C device
> requesting the IBI has not yet given up.
>
> >
> >> Both I3C bus instances must be runtime suspended then.
> >> Similarly, upon resume the PME is no longer active, so both I3C bus instances
> >> must make their buses operational - we don't know which may have received
> >> an IBI.
> >
> > Does PME active auto by hardware or need software config?
>
> PCI devices (hardware) advertise their PME capability in terms of
> which states are capable of PMEs.  Currently the Intel LPSS I3C
> device lists only D3hot.
>
> The PCI subsystem (software) automatically enables the PME before
> runtime suspend if the target power state allows it.

Okay, I think I understand your situation, let me check patch again.

Frank
>
> >
> > Frank
> >> And there may be further IBIs which can't be received unless the
> >> associated bus is operational.  The PCI device is no longer in a low power
> >> state, so there will be no PME in that case.
> >>
> >>>
> >>> Frank
> >>>
> >>>>
> >>>>>
> >>>>> Frank
> >>>>>
> >>>>>> every bus instance must remain able to receive IBIs up
> >>>>>> until that point.  This requires deferring the individual controllers’
> >>>>>> runtime suspend callbacks (which disable bus activity) until the parent
> >>>>>> decides it is safe for all instances to suspend together.
> >>>>>>
> >>>>>> To support this usage model:
> >>>>>>
> >>>>>>   * Export the controller's runtime PM suspend/resume callbacks so that
> >>>>>>     the parent can invoke them directly.
> >>>>>>
> >>>>>>   * Add a new quirk, HCI_QUIRK_RPM_PARENT_MANAGED, which designates the
> >>>>>>     parent device as the controller’s runtime PM device (rpm_dev).  When
> >>>>>>     used without HCI_QUIRK_RPM_ALLOWED, this also prevents the child
> >>>>>>     instance’s system-suspend callbacks from using
> >>>>>>     pm_runtime_force_suspend()/pm_runtime_force_resume(), since runtime
> >>>>>>     PM is managed entirely by the parent.
> >>>>>>
> >>>>>>   * Move DEFAULT_AUTOSUSPEND_DELAY_MS into the header so it can be shared
> >>>>>>     by parent-managed PM implementations.
> >>>>>>
> >>>>>> The new quirk allows platforms with multi-bus parent-managed PM
> >>>>>> infrastructure to correctly coordinate runtime PM across all I3C HCI
> >>>>>> instances.
> >>>>>>
> >>>>>> Signed-off-by: Adrian Hunter <adrian.hunter@...el.com>
> >>>>>> ---
> >>>>>>  drivers/i3c/master/mipi-i3c-hci/core.c | 25 ++++++++++++++++---------
> >>>>>>  drivers/i3c/master/mipi-i3c-hci/hci.h  |  6 ++++++
> >>>>>>  2 files changed, 22 insertions(+), 9 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
> >>>>>> index ec4dbe64c35e..cb974b0f9e17 100644
> >>>>>> --- a/drivers/i3c/master/mipi-i3c-hci/core.c
> >>>>>> +++ b/drivers/i3c/master/mipi-i3c-hci/core.c
> >>>>>> @@ -733,7 +733,7 @@ static int i3c_hci_reset_and_init(struct i3c_hci *hci)
> >>>>>>  	return 0;
> >>>>>>  }
> >>>>>>
> >>>>>> -static int i3c_hci_runtime_suspend(struct device *dev)
> >>>>>> +int i3c_hci_runtime_suspend(struct device *dev)
> >>>>>>  {
> >>>>>>  	struct i3c_hci *hci = dev_get_drvdata(dev);
> >>>>>>  	int ret;
> >>>>>> @@ -746,8 +746,9 @@ static int i3c_hci_runtime_suspend(struct device *dev)
> >>>>>>
> >>>>>>  	return 0;
> >>>>>>  }
> >>>>>> +EXPORT_SYMBOL_GPL(i3c_hci_runtime_suspend);
> >>>>>>
> >>>>>> -static int i3c_hci_runtime_resume(struct device *dev)
> >>>>>> +int i3c_hci_runtime_resume(struct device *dev)
> >>>>>>  {
> >>>>>>  	struct i3c_hci *hci = dev_get_drvdata(dev);
> >>>>>>  	int ret;
> >>>>>> @@ -768,6 +769,7 @@ static int i3c_hci_runtime_resume(struct device *dev)
> >>>>>>
> >>>>>>  	return 0;
> >>>>>>  }
> >>>>>> +EXPORT_SYMBOL_GPL(i3c_hci_runtime_resume);
> >>>>>>
> >>>>>>  static int i3c_hci_suspend(struct device *dev)
> >>>>>>  {
> >>>>>> @@ -784,12 +786,14 @@ static int i3c_hci_resume_common(struct device *dev, bool rstdaa)
> >>>>>>  	struct i3c_hci *hci = dev_get_drvdata(dev);
> >>>>>>  	int ret;
> >>>>>>
> >>>>>> -	if (!(hci->quirks & HCI_QUIRK_RPM_ALLOWED))
> >>>>>> -		return 0;
> >>>>>> +	if (!(hci->quirks & HCI_QUIRK_RPM_PARENT_MANAGED)) {
> >>>>>> +		if (!(hci->quirks & HCI_QUIRK_RPM_ALLOWED))
> >>>>>> +			return 0;
> >>>>>>
> >>>>>> -	ret = pm_runtime_force_resume(dev);
> >>>>>> -	if (ret)
> >>>>>> -		return ret;
> >>>>>> +		ret = pm_runtime_force_resume(dev);
> >>>>>> +		if (ret)
> >>>>>> +			return ret;
> >>>>>> +	}
> >>>>>>
> >>>>>>  	ret = i3c_master_do_daa_ext(&hci->master, rstdaa);
> >>>>>>  	if (ret)
> >>>>>> @@ -812,8 +816,6 @@ static int i3c_hci_restore(struct device *dev)
> >>>>>>  	return i3c_hci_resume_common(dev, true);
> >>>>>>  }
> >>>>>>
> >>>>>> -#define DEFAULT_AUTOSUSPEND_DELAY_MS 1000
> >>>>>> -
> >>>>>>  static void i3c_hci_rpm_enable(struct device *dev)
> >>>>>>  {
> >>>>>>  	struct i3c_hci *hci = dev_get_drvdata(dev);
> >>>>>> @@ -962,6 +964,11 @@ static int i3c_hci_probe(struct platform_device *pdev)
> >>>>>>  	if (hci->quirks & HCI_QUIRK_RPM_IBI_ALLOWED)
> >>>>>>  		hci->master.rpm_ibi_allowed = true;
> >>>>>>
> >>>>>> +	if (hci->quirks & HCI_QUIRK_RPM_PARENT_MANAGED) {
> >>>>>> +		hci->master.rpm_dev = pdev->dev.parent;
> >>>>>> +		hci->master.rpm_allowed = true;
> >>>>>> +	}
> >>>>>> +
> >>>>>>  	return i3c_master_register(&hci->master, &pdev->dev, &i3c_hci_ops, false);
> >>>>>>  }
> >>>>>>
> >>>>>> diff --git a/drivers/i3c/master/mipi-i3c-hci/hci.h b/drivers/i3c/master/mipi-i3c-hci/hci.h
> >>>>>> index 819328a85b84..d0e7ad58ac15 100644
> >>>>>> --- a/drivers/i3c/master/mipi-i3c-hci/hci.h
> >>>>>> +++ b/drivers/i3c/master/mipi-i3c-hci/hci.h
> >>>>>> @@ -147,6 +147,7 @@ struct i3c_hci_dev_data {
> >>>>>>  #define HCI_QUIRK_RESP_BUF_THLD		BIT(4)  /* Set resp buf thld to 0 for AMD platforms */
> >>>>>>  #define HCI_QUIRK_RPM_ALLOWED		BIT(5)  /* Runtime PM allowed */
> >>>>>>  #define HCI_QUIRK_RPM_IBI_ALLOWED	BIT(6)  /* IBI and Hot-Join allowed while runtime suspended */
> >>>>>> +#define HCI_QUIRK_RPM_PARENT_MANAGED	BIT(7)  /* Runtime PM managed by parent device */
> >>>>>>
> >>>>>>  /* global functions */
> >>>>>>  void mipi_i3c_hci_resume(struct i3c_hci *hci);
> >>>>>> @@ -156,4 +157,9 @@ void amd_set_od_pp_timing(struct i3c_hci *hci);
> >>>>>>  void amd_set_resp_buf_thld(struct i3c_hci *hci);
> >>>>>>  void i3c_hci_sync_irq_inactive(struct i3c_hci *hci);
> >>>>>>
> >>>>>> +#define DEFAULT_AUTOSUSPEND_DELAY_MS 1000
> >>>>>> +
> >>>>>> +int i3c_hci_runtime_suspend(struct device *dev);
> >>>>>> +int i3c_hci_runtime_resume(struct device *dev);
> >>>>>> +
> >>>>>>  #endif
> >>>>>> --
> >>>>>> 2.51.0
> >>>>>>
> >>>>
> >>
>

Powered by blists - more mailing lists