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

Hi Chukun,

Sorry, I missed your mail due recent problem of my client..
On 11:05 Mon 02 Mar     , Chukun Pan wrote:

Hi,

quoted

&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

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)