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

[Chen-Yu Tsai]

On Tue, Mar 10, 2026 at 12:49 PM Chen-Yu Tsai <wenst at chromium.org> wrote:
>
> On Tue, Mar 10, 2026 at 5:50 AM Bjorn Helgaas <helgaas at kernel.org> wrote:
> >
> > 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 at 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.
>
> Sure.
>
> > > > 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.
>
> I understand. I merely meant that I have no actual knowledge of why it
> happened, or if there are any concerns from the hardware side.
>
> > Separating the PCIe controller setup from things related to the link
> > and downstream devices is pretty high on my list.
>
> Makes sense to me.
>
> I'll send an updated version.

I'm confused. I see this version in -next with the minor fixups from
Bjorn. Do you want me to do the cleanups on top of them?


ChenYu