Implement stop_link() and  start_link() function op for dwc drivers.

Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
---
 drivers/pci/controller/dwc/pcie-designware-host.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index 952f8594b501254d2b2de5d5e056e16d2aa8d4b7..bcdc4a0e4b4747f2d62e1b67bc1aeda16e35acdd 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -722,10 +722,28 @@ void __iomem *dw_pcie_own_conf_map_bus(struct pci_bus *bus, unsigned int devfn,
 }
 EXPORT_SYMBOL_GPL(dw_pcie_own_conf_map_bus);
 
+static int dw_pcie_op_start_link(struct pci_bus *bus)
+{
+	struct dw_pcie_rp *pp = bus->sysdata;
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+
+	return dw_pcie_host_start_link(pci);
+}
+
+static void dw_pcie_op_stop_link(struct pci_bus *bus)
+{
+	struct dw_pcie_rp *pp = bus->sysdata;
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+
+	dw_pcie_host_stop_link(pci);
+}
+
 static struct pci_ops dw_pcie_ops = {
 	.map_bus = dw_pcie_own_conf_map_bus,
 	.read = pci_generic_config_read,
 	.write = pci_generic_config_write,
+	.start_link = dw_pcie_op_start_link,
+	.stop_link = dw_pcie_op_stop_link,
 };
 
 static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)

-- 
2.34.1

On Thu, Aug 28, 2025 at 05:39:02PM +0530, Krishna Chaitanya Chundru wrote:
> Implement stop_link() and  start_link() function op for dwc drivers.
> 
> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> ---
>  drivers/pci/controller/dwc/pcie-designware-host.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index 952f8594b501254d2b2de5d5e056e16d2aa8d4b7..bcdc4a0e4b4747f2d62e1b67bc1aeda16e35acdd 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -722,10 +722,28 @@ void __iomem *dw_pcie_own_conf_map_bus(struct pci_bus *bus, unsigned int devfn,
>  }
>  EXPORT_SYMBOL_GPL(dw_pcie_own_conf_map_bus);
>  
> +static int dw_pcie_op_start_link(struct pci_bus *bus)
> +{
> +	struct dw_pcie_rp *pp = bus->sysdata;
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +
> +	return dw_pcie_host_start_link(pci);

This takes a pci_bus *, which could be any PCI bus, but this only
works for root buses because it affects the link from a Root Port.

I know the TC9563 is directly below the Root Port in the current
topology, but it seems like the ability to configure a Switch with I2C
or similar is potentially of general interest, even if the switch is
deeper in the hierarchy.

Is there a generic way to inhibit link training, e.g., with the Link
Disable bit in the Link Control register?  If so, this could
potentially be done in a way that would work for any vendor and for
any Downstream Port, including Root Ports and Switch Downstream Ports.

> +}
> +
> +static void dw_pcie_op_stop_link(struct pci_bus *bus)
> +{
> +	struct dw_pcie_rp *pp = bus->sysdata;
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +
> +	dw_pcie_host_stop_link(pci);
> +}
> +
>  static struct pci_ops dw_pcie_ops = {
>  	.map_bus = dw_pcie_own_conf_map_bus,
>  	.read = pci_generic_config_read,
>  	.write = pci_generic_config_write,
> +	.start_link = dw_pcie_op_start_link,
> +	.stop_link = dw_pcie_op_stop_link,
>  };
>  
>  static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
> 
> -- 
> 2.34.1
>

On Thu, Sep 25, 2025 at 09:54:16AM -0500, Bjorn Helgaas wrote:
> On Thu, Aug 28, 2025 at 05:39:02PM +0530, Krishna Chaitanya Chundru wrote:
> > Implement stop_link() and  start_link() function op for dwc drivers.
> > 
> > Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> > ---
> >  drivers/pci/controller/dwc/pcie-designware-host.c | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > index 952f8594b501254d2b2de5d5e056e16d2aa8d4b7..bcdc4a0e4b4747f2d62e1b67bc1aeda16e35acdd 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > @@ -722,10 +722,28 @@ void __iomem *dw_pcie_own_conf_map_bus(struct pci_bus *bus, unsigned int devfn,
> >  }
> >  EXPORT_SYMBOL_GPL(dw_pcie_own_conf_map_bus);
> >  
> > +static int dw_pcie_op_start_link(struct pci_bus *bus)
> > +{
> > +	struct dw_pcie_rp *pp = bus->sysdata;
> > +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > +
> > +	return dw_pcie_host_start_link(pci);
> 
> This takes a pci_bus *, which could be any PCI bus, but this only
> works for root buses because it affects the link from a Root Port.
> 
> I know the TC9563 is directly below the Root Port in the current
> topology, but it seems like the ability to configure a Switch with I2C
> or similar is potentially of general interest, even if the switch is
> deeper in the hierarchy.
> 
> Is there a generic way to inhibit link training, e.g., with the Link
> Disable bit in the Link Control register?  If so, this could
> potentially be done in a way that would work for any vendor and for
> any Downstream Port, including Root Ports and Switch Downstream Ports.
> 

FWIW, the link should not be stopped for a single device, since it could affect
other devices in the bus. Imagine if this switch is connected to one of the
downstream port of another switch. Then stopping and starting the link will
affect other devices connected to the upstream switch as well.

This driver is doing it right now just because, there is no other way to
control the switch state machine. Ideally, we would want the PERST# to be in
asserted stage to keep the device from starting the state machine, then program
the registers over I2C and deassert PERST#. This will work across all of the
host controller drivers (if they support pwrctrl framework).

But since we don't have PERST# support for the pwrctrl framework, we can have
this as an adhoc solution. I don't think we should try to generalize it.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

On Thu, Sep 25, 2025 at 09:49:16PM +0530, Manivannan Sadhasivam wrote:
> On Thu, Sep 25, 2025 at 09:54:16AM -0500, Bjorn Helgaas wrote:
> > On Thu, Aug 28, 2025 at 05:39:02PM +0530, Krishna Chaitanya Chundru wrote:
> > > Implement stop_link() and  start_link() function op for dwc drivers.
> > > 
> > > Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> > > ---
> > >  drivers/pci/controller/dwc/pcie-designware-host.c | 18 ++++++++++++++++++
> > >  1 file changed, 18 insertions(+)
> > > 
> > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > index 952f8594b501254d2b2de5d5e056e16d2aa8d4b7..bcdc4a0e4b4747f2d62e1b67bc1aeda16e35acdd 100644
> > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > @@ -722,10 +722,28 @@ void __iomem *dw_pcie_own_conf_map_bus(struct pci_bus *bus, unsigned int devfn,
> > >  }
> > >  EXPORT_SYMBOL_GPL(dw_pcie_own_conf_map_bus);
> > >  
> > > +static int dw_pcie_op_start_link(struct pci_bus *bus)
> > > +{
> > > +	struct dw_pcie_rp *pp = bus->sysdata;
> > > +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > > +
> > > +	return dw_pcie_host_start_link(pci);
> > 
> > This takes a pci_bus *, which could be any PCI bus, but this only
> > works for root buses because it affects the link from a Root Port.
> > 
> > I know the TC9563 is directly below the Root Port in the current
> > topology, but it seems like the ability to configure a Switch with
> > I2C or similar is potentially of general interest, even if the
> > switch is deeper in the hierarchy.
> > 
> > Is there a generic way to inhibit link training, e.g., with the
> > Link Disable bit in the Link Control register?  If so, this could
> > potentially be done in a way that would work for any vendor and
> > for any Downstream Port, including Root Ports and Switch
> > Downstream Ports.
> 
> FWIW, the link should not be stopped for a single device, since it
> could affect other devices in the bus. Imagine if this switch is
> connected to one of the downstream port of another switch. Then
> stopping and starting the link will affect other devices connected
> to the upstream switch as well.

Link Disable would affect all devices downstream of the bridge where
it is set, same as dw_pcie_op_stop_link().

> This driver is doing it right now just because, there is no other
> way to control the switch state machine. Ideally, we would want the
> PERST# to be in asserted stage to keep the device from starting the
> state machine, then program the registers over I2C and deassert
> PERST#. This will work across all of the host controller drivers (if
> they support pwrctrl framework).

I don't think there's a way to implement .start_link() and
.stop_link() for ACPI unless it's by using Link Disable, which is why
I asked about this.  If Link Disable *does* work, it would be a very
generic way to do this because it's part of the PCIe base spec.

Bjorn

On 9/25/2025 10:55 PM, Bjorn Helgaas wrote:
> On Thu, Sep 25, 2025 at 09:49:16PM +0530, Manivannan Sadhasivam wrote:
>> On Thu, Sep 25, 2025 at 09:54:16AM -0500, Bjorn Helgaas wrote:
>>> On Thu, Aug 28, 2025 at 05:39:02PM +0530, Krishna Chaitanya Chundru wrote:
>>>> Implement stop_link() and  start_link() function op for dwc drivers.
>>>>
>>>> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
>>>> ---
>>>>   drivers/pci/controller/dwc/pcie-designware-host.c | 18 ++++++++++++++++++
>>>>   1 file changed, 18 insertions(+)
>>>>
>>>> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
>>>> index 952f8594b501254d2b2de5d5e056e16d2aa8d4b7..bcdc4a0e4b4747f2d62e1b67bc1aeda16e35acdd 100644
>>>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
>>>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
>>>> @@ -722,10 +722,28 @@ void __iomem *dw_pcie_own_conf_map_bus(struct pci_bus *bus, unsigned int devfn,
>>>>   }
>>>>   EXPORT_SYMBOL_GPL(dw_pcie_own_conf_map_bus);
>>>>   
>>>> +static int dw_pcie_op_start_link(struct pci_bus *bus)
>>>> +{
>>>> +	struct dw_pcie_rp *pp = bus->sysdata;
>>>> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>>>> +
>>>> +	return dw_pcie_host_start_link(pci);
>>>
>>> This takes a pci_bus *, which could be any PCI bus, but this only
>>> works for root buses because it affects the link from a Root Port.
>>>
>>> I know the TC9563 is directly below the Root Port in the current
>>> topology, but it seems like the ability to configure a Switch with
>>> I2C or similar is potentially of general interest, even if the
>>> switch is deeper in the hierarchy.
>>>
>>> Is there a generic way to inhibit link training, e.g., with the
>>> Link Disable bit in the Link Control register?  If so, this could
>>> potentially be done in a way that would work for any vendor and
>>> for any Downstream Port, including Root Ports and Switch
>>> Downstream Ports.
>>
>> FWIW, the link should not be stopped for a single device, since it
>> could affect other devices in the bus. Imagine if this switch is
>> connected to one of the downstream port of another switch. Then
>> stopping and starting the link will affect other devices connected
>> to the upstream switch as well.
> 
> Link Disable would affect all devices downstream of the bridge where
> it is set, same as dw_pcie_op_stop_link().
> 
>> This driver is doing it right now just because, there is no other
>> way to control the switch state machine. Ideally, we would want the
>> PERST# to be in asserted stage to keep the device from starting the
>> state machine, then program the registers over I2C and deassert
>> PERST#. This will work across all of the host controller drivers (if
>> they support pwrctrl framework).
> 
> I don't think there's a way to implement .start_link() and
> .stop_link() for ACPI unless it's by using Link Disable, which is why
> I asked about this.  If Link Disable *does* work, it would be a very
> generic way to do this because it's part of the PCIe base spec.
>
Hi Bjorn,

We did test as you suggested but unfortunately the setting are not
getting reflected we need to explicitly assert perst to make sure
pcie is in reset state while applying these settings.

- Krishna Chaitanya.

> Bjorn
>

On Fri, Sep 26, 2025 at 07:09:17PM +0530, Krishna Chaitanya Chundru wrote:
> On 9/25/2025 10:55 PM, Bjorn Helgaas wrote:
> > On Thu, Sep 25, 2025 at 09:49:16PM +0530, Manivannan Sadhasivam wrote:
> > > On Thu, Sep 25, 2025 at 09:54:16AM -0500, Bjorn Helgaas wrote:
> > > > On Thu, Aug 28, 2025 at 05:39:02PM +0530, Krishna Chaitanya Chundru wrote:
> > > > > Implement stop_link() and  start_link() function op for dwc drivers.
> > > > > 
> > > > > Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> > > > > ---
> > > > >   drivers/pci/controller/dwc/pcie-designware-host.c | 18 ++++++++++++++++++
> > > > >   1 file changed, 18 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > index 952f8594b501254d2b2de5d5e056e16d2aa8d4b7..bcdc4a0e4b4747f2d62e1b67bc1aeda16e35acdd 100644
> > > > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > @@ -722,10 +722,28 @@ void __iomem *dw_pcie_own_conf_map_bus(struct pci_bus *bus, unsigned int devfn,
> > > > >   }
> > > > >   EXPORT_SYMBOL_GPL(dw_pcie_own_conf_map_bus);
> > > > > +static int dw_pcie_op_start_link(struct pci_bus *bus)
> > > > > +{
> > > > > +	struct dw_pcie_rp *pp = bus->sysdata;
> > > > > +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > > > > +
> > > > > +	return dw_pcie_host_start_link(pci);
> > > > 
> > > > This takes a pci_bus *, which could be any PCI bus, but this only
> > > > works for root buses because it affects the link from a Root Port.
> > > > 
> > > > I know the TC9563 is directly below the Root Port in the current
> > > > topology, but it seems like the ability to configure a Switch with
> > > > I2C or similar is potentially of general interest, even if the
> > > > switch is deeper in the hierarchy.
> > > > 
> > > > Is there a generic way to inhibit link training, e.g., with the
> > > > Link Disable bit in the Link Control register?  If so, this could
> > > > potentially be done in a way that would work for any vendor and
> > > > for any Downstream Port, including Root Ports and Switch
> > > > Downstream Ports.
> > > 
> > > FWIW, the link should not be stopped for a single device, since it
> > > could affect other devices in the bus. Imagine if this switch is
> > > connected to one of the downstream port of another switch. Then
> > > stopping and starting the link will affect other devices connected
> > > to the upstream switch as well.
> > 
> > Link Disable would affect all devices downstream of the bridge where
> > it is set, same as dw_pcie_op_stop_link().
> > 
> > > This driver is doing it right now just because, there is no other
> > > way to control the switch state machine. Ideally, we would want the
> > > PERST# to be in asserted stage to keep the device from starting the
> > > state machine, then program the registers over I2C and deassert
> > > PERST#. This will work across all of the host controller drivers (if
> > > they support pwrctrl framework).
> > 
> > I don't think there's a way to implement .start_link() and
> > .stop_link() for ACPI unless it's by using Link Disable, which is why
> > I asked about this.  If Link Disable *does* work, it would be a very
> > generic way to do this because it's part of the PCIe base spec.
> 
> We did test as you suggested but unfortunately the setting are not
> getting reflected we need to explicitly assert perst to make sure
> pcie is in reset state while applying these settings.

Maybe ".stop_link()" is the wrong name if what's actually required is
PERST#?

This feels like the kind of problem we are likely to see again in
different topologies, e.g., a switch in an external enclosure that
needs configuration.

On Fri, Sep 26, 2025 at 03:39:16PM -0500, Bjorn Helgaas wrote:
> On Fri, Sep 26, 2025 at 07:09:17PM +0530, Krishna Chaitanya Chundru wrote:
> > On 9/25/2025 10:55 PM, Bjorn Helgaas wrote:
> > > On Thu, Sep 25, 2025 at 09:49:16PM +0530, Manivannan Sadhasivam wrote:
> > > > On Thu, Sep 25, 2025 at 09:54:16AM -0500, Bjorn Helgaas wrote:
> > > > > On Thu, Aug 28, 2025 at 05:39:02PM +0530, Krishna Chaitanya Chundru wrote:
> > > > > > Implement stop_link() and  start_link() function op for dwc drivers.
> > > > > > 
> > > > > > Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> > > > > > ---
> > > > > >   drivers/pci/controller/dwc/pcie-designware-host.c | 18 ++++++++++++++++++
> > > > > >   1 file changed, 18 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > > index 952f8594b501254d2b2de5d5e056e16d2aa8d4b7..bcdc4a0e4b4747f2d62e1b67bc1aeda16e35acdd 100644
> > > > > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > > @@ -722,10 +722,28 @@ void __iomem *dw_pcie_own_conf_map_bus(struct pci_bus *bus, unsigned int devfn,
> > > > > >   }
> > > > > >   EXPORT_SYMBOL_GPL(dw_pcie_own_conf_map_bus);
> > > > > > +static int dw_pcie_op_start_link(struct pci_bus *bus)
> > > > > > +{
> > > > > > +	struct dw_pcie_rp *pp = bus->sysdata;
> > > > > > +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > > > > > +
> > > > > > +	return dw_pcie_host_start_link(pci);
> > > > > 
> > > > > This takes a pci_bus *, which could be any PCI bus, but this only
> > > > > works for root buses because it affects the link from a Root Port.
> > > > > 
> > > > > I know the TC9563 is directly below the Root Port in the current
> > > > > topology, but it seems like the ability to configure a Switch with
> > > > > I2C or similar is potentially of general interest, even if the
> > > > > switch is deeper in the hierarchy.
> > > > > 
> > > > > Is there a generic way to inhibit link training, e.g., with the
> > > > > Link Disable bit in the Link Control register?  If so, this could
> > > > > potentially be done in a way that would work for any vendor and
> > > > > for any Downstream Port, including Root Ports and Switch
> > > > > Downstream Ports.
> > > > 
> > > > FWIW, the link should not be stopped for a single device, since it
> > > > could affect other devices in the bus. Imagine if this switch is
> > > > connected to one of the downstream port of another switch. Then
> > > > stopping and starting the link will affect other devices connected
> > > > to the upstream switch as well.
> > > 
> > > Link Disable would affect all devices downstream of the bridge where
> > > it is set, same as dw_pcie_op_stop_link().
> > > 
> > > > This driver is doing it right now just because, there is no other
> > > > way to control the switch state machine. Ideally, we would want the
> > > > PERST# to be in asserted stage to keep the device from starting the
> > > > state machine, then program the registers over I2C and deassert
> > > > PERST#. This will work across all of the host controller drivers (if
> > > > they support pwrctrl framework).
> > > 
> > > I don't think there's a way to implement .start_link() and
> > > .stop_link() for ACPI unless it's by using Link Disable, which is why
> > > I asked about this.  If Link Disable *does* work, it would be a very
> > > generic way to do this because it's part of the PCIe base spec.
> > 
> > We did test as you suggested but unfortunately the setting are not
> > getting reflected we need to explicitly assert perst to make sure
> > pcie is in reset state while applying these settings.
> 
> Maybe ".stop_link()" is the wrong name if what's actually required is
> PERST#?
> 

If we rename this callback to foo_perst(), then it will be similar to my Pwrctrl
PERST# integration series [1]. I'm wondering why shouldn't we merge it instead
and get rid of this callback from this series for good?

- Mani

[1] https://lore.kernel.org/linux-pci/20250912-pci-pwrctrl-perst-v3-0-3c0ac62b032c@oss.qualcomm.com/

-- 
மணிவண்ணன் சதாசிவம்