Re: [PATCH v3 5/7] PCI: mediatek-gen3: Integrate new pwrctrl API

[Bjorn Helgaas <helgaas@kernel.org>]

On Mon, Mar 09, 2026 at 01:24:34PM +0800, Chen-Yu Tsai wrote:
> On Sat, Mar 7, 2026 at 4:59 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Mon, Mar 02, 2026 at 01:31:05PM +0800, Chen-Yu Tsai wrote:
> > > With the new PCI pwrctrl API and PCI slot binding and power drivers, we
> > > now have a way to describe and power up WiFi/BT adapters connected
> > > through a PCIe or M.2 slot, or exploded onto the mainboard itself.
> ...

> > So now we have:
> >
> >   mtk_pcie_probe
> >     mtk_pcie_setup
> >       mtk_pcie_startup_port
> >         mtk_pcie_device_power_up      <-- power up
> >         mtk_pcie_device_power_down    # error path
> >       mtk_pcie_setup_irq              # set up controller IRQ
> >       mtk_pcie_device_power_down      # if mtk_pcie_setup_irq() failed
> >     pci_host_probe                    # enumerate downstream devices
> >     mtk_pcie_device_power_down        # if pci_host_probe() failed
> >
> > I think this is kind of a mess because mtk_pcie_device_power_down() is
> > called from so many places, and some of them aren't connected to the
> > mtk_pcie_device_power_up().
> >
> > In mtk_pcie_setup(), mtk_pcie_setup_irq() only deals with the
> > *controller* IRQ and has nothing to do with the downstream devices.  I
> > think mtk_pcie_setup_irq() should be done before
> > mtk_pcie_startup_port() so we can abort before even powering up those
> > devices.
> 
> Makes sense to me. I think it can even be moved outside of mtk_pcie_setup()?
> That way we don't have mtk_pcie_irq_teardown() in two error paths?
> 
> I can send a follow up patch for that.

What if we do those cleanups first?  That would make this patch quite
a bit simpler and we wouldn't have to undo things.  It would be a
shame to have a complicated patch to add this stuff, then another
complicated patch to clean it up.

> > In mtk_pcie_startup_port(), mtk_pcie_enable_msi() and the PCIe
> > translation window setup also don't have anything to do with the
> > downstream devices, so I think they should be done before
> > calling mtk_pcie_device_power_up().  The only thing there that needs
> > the downstream devices powered up is waiting for the link to come up.
> 
> My guess is that if the link up fails, then mtk_pcie_startup_port() is
> going to error out anyway, so it maybe made sense to make sure a device
> is actually present before doing any more work.

Doesn't sound very convincing to me.  mtk_pcie_set_trans_table() and
the window setup are local things with no timeouts or anything.  I
think the logical structure is way more important than some kind of
hand-crafted work avoidance.

Separating the PCIe controller setup from things related to the link
and downstream devices is pretty high on my list.

Bjorn