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

[Bjorn Helgaas <helgaas@kernel.org>]

On Fri, Mar 20, 2026 at 02:10:03PM +0800, Chen-Yu Tsai wrote:
> On Tue, Mar 10, 2026 at 12:49 PM Chen-Yu Tsai <wenst@chromium.org> wrote:
> > On Tue, Mar 10, 2026 at 5:50 AM Bjorn Helgaas <helgaas@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@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?

I had intended to defer merging pci/controller/mediatek-gen3 to
pci/next and wait for your updated version.  But I see that I forgot
about that and included the original version.  I rebuilt pci/next
without pci/controller/mediatek-gen3.

Bjorn