Re: [PATCH] riscv: dts: spacemit: pcie: fix missing power regulator

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3582 bytes --]

On Mon, Mar 02, 2026 at 11:36:55AM +0800, Yixun Lan wrote:
> Hi Chukun,
> 
> Sorry, I missed your mail due recent problem of my client..
> On 11:05 Mon 02 Mar     , Chukun Pan wrote:
> > Hi,
> > 
> > > &pcie1_port {
> > >  	phys = <&pcie1_phy>;
> > > +	vpcie3v3-supply = <&pcie_vcc_3v3>;
> > >  };
> > > 
> > >  &pcie1 {
> > > @@ -320,6 +321,7 @@ &pcie2_phy {
> > > 
> > >  &pcie2_port {
> > >  	phys = <&pcie2_phy>;
> > > +	vpcie3v3-supply = <&pcie_vcc_3v3>;
> > >  };
> > 
> > ```
> > &pcie1 {
> > 	vpcie3v3-supply = <&pcie_vcc_3v3>;
> > 	status = "okay";
> > };
> > ```
> > 
> > According to DT binding, the vpcie3v3-supply of the &pciex node should
> > be moved to the &pciex_port node. This is simply a duplication of the
> > property.
> > 
> I have confidence that pcie port need to add a regulator to provide 
> supply for devices..
> 
> But, I'm not sure whether it's ok to remove regulator from &pciex node,
> it's possibly a 'yes' answer, but to convince me, I'd like to see a real
> test case to prove it: e.g, power supply for pciex is actually off before 
> the driver initialization, then run regular procedure as it should, if
> all works fine

I'm not really sure what this is about. Whether the supply can be off
before driver probe has nothing to do with what node the supply should
be in. If you're concerned about removing it from the pcie node causing
it to not be enabled during probe, then the driver should probably reach
into the port node, get the regulator and call enable() on it?
As I said when reporting this, either you need to change the driver and
dts, or change the binding. If the supply actually is provided to the
controller and port, then you need to change the binding to have a
supply in the controller node and change the driver to make sure that
both the controller node supply and the port node supply are enabled as
someone could opt to provide them from different regulators.

Looking at the bpif3 schematic
https://drive.google.com/file/d/19iLJ5xnCB_oK8VeQjkPGjzAn39WYyylv/view
I see nowhere where that 3v3 supply is actually provided to the k1,
which I would expect if it were the supply for the controller itself.
Instead, the m2 and minipcie slots are where I see that supply provided
in the schematic. To me, that sounds like the port is what needs the
supply, not the controller, but I'm not super familiar with pci
devicetree stuff unfortunately.

> Btw, different drivers request same regulator is ok, and is quite normal,
> can't draw a conclusion that it's a duplication, as they may be used for
> different reasons
> 
> > But do we really need this pcie_port (PCIe bridge)?
> > 
> > The PCIe bridge node (pcie@0) was treated as a platform device, but it
> > did not define the interrupts property, which resulted in the following
> > warning: `[    2.897980] irq: no irq domain found for pcie@0 !`
> > 
> > Would it be better to submit a patch to remove this pcie_port?
> > 
> > ```
> > -       ret = k1_pcie_parse_port(k1);
> > -       if (ret)
> > -               return dev_err_probe(dev, ret, "failed to parse root port\n");
> > +       k1->phy = devm_phy_get(dev, "pcie-phy");
> > +       if (IS_ERR(k1->phy))
> > +               return dev_err_probe(dev, PTR_ERR(k1->phy), "missing PHY\n");
> > ```
> 
> I've not really looked at this, and not an expert on this area, so will
> leave this to Alex or PCIe maintainers..
> 
> > I have tested this change and it works.
> > 
> > Thanks,
> > Chukun
> > 
> 
> -- 
> Yixun Lan (dlan)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]