Re: [PATCH 1/2] PCI: rockchip: Make some regulators non-optional

[Robin Murphy <robin.murphy@xxxxxxx>]

On 2020-11-07 11:36, Qu Wenruo wrote:
> On 2019/11/21 上午1:05, Lorenzo Pieralisi wrote:
> > On Sat, Nov 16, 2019 at 12:54:19PM +0000, Robin Murphy wrote:
> > > The 0V9 and 1V8 supplies power the PCIe block in the SoC itself, and
> > > are thus fundamental to PCIe being usable at all. As such, it makes
> > > sense to treat them as non-optional and rely on dummy regulators if
> > > not explicitly described.
> > >
> > > Signed-off-by: Robin Murphy <robin.murphy@xxxxxxx>
> > > ---
> > >   drivers/pci/controller/pcie-rockchip-host.c | 69 ++++++++-------------
> > >   1 file changed, 25 insertions(+), 44 deletions(-)
> >
> > Applied to pci/rockchip, thanks.
>
> Sorry, this commit is cause regression for RK3399 boards unable to
> detect the controller anymore.
>
> The 1v8 (and 0v9) is causing -517 and reject the controller initialization.

That's -EPROBE_DEFER, which must mean that a regulator *is* described, 
but you're missing the relevant driver - that's an issue with your 
config/initrd. Being optional should only change the behaviour if the 
supply is totally absent (i.e. you get -ENODEV instead of a dummy 
regulator), so I don't see that it would make any difference in this 
situation anyway :/

> I'm not a PCI guy, but a quick google search shows these two voltages
> are not related to PCIE core functionality, especially considering the
> controller used in RK3399 are mostly to provide NVME support.

Unlike the 12V and 3V3 supplies to the slot, these supplies are to the 
PCIE_AVDD_0V9 and PCIE_AVDD_1V8 pins on the SoC itself, which the 
datasheet describe as "Supply voltage for PCIE". Having power is kind of 
important for the I/O circuits on all the signal pins to work.

Now it's almost certainly true that these supplies technically belong to 
the phy rather than the controller, but it's a bit late to change the 
bindings for the sake of semantics.

> This bug makes all RK3399 users who put root fs into NVME driver unable
> to boot the device.
>
> I really hope some one could test the patch before affecting the end
> users or at least try to understand how most users would use the PCIE
> interface for.

I *am* that end user in this case - I use an M.2 NVME on my board, which 
prompted me to take a look at the regulator handling here in the first 
place, to see if it might be possible to shut up the annoying message 
about a 12V supply that is entirely irrelevant to a board without a 
full-size PCIe slot. I use a mainline-based distro, so I've been running 
this change for nearly a year since it landed in v5.5, and I'm sure many 
others have too. I've not heard of any other complaints in that time...

Robin.

> Thanks,
> Qu
>
> > Lorenzo
> >
> > > diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
> > > index ef8e677ce9d1..68525f8ac4d9 100644
> > > --- a/drivers/pci/controller/pcie-rockchip-host.c
> > > +++ b/drivers/pci/controller/pcie-rockchip-host.c
> > > @@ -620,19 +620,13 @@ static int rockchip_pcie_parse_host_dt(struct rockchip_pcie *rockchip)
> > >   		dev_info(dev, "no vpcie3v3 regulator found\n");
> > >   	}
> > >   
> > > -	rockchip->vpcie1v8 = devm_regulator_get_optional(dev, "vpcie1v8");
> > > -	if (IS_ERR(rockchip->vpcie1v8)) {
> > > -		if (PTR_ERR(rockchip->vpcie1v8) != -ENODEV)
> > > -			return PTR_ERR(rockchip->vpcie1v8);
> > > -		dev_info(dev, "no vpcie1v8 regulator found\n");
> > > -	}
> > > +	rockchip->vpcie1v8 = devm_regulator_get(dev, "vpcie1v8");
> > > +	if (IS_ERR(rockchip->vpcie1v8))
> > > +		return PTR_ERR(rockchip->vpcie1v8);
> > >   
> > > -	rockchip->vpcie0v9 = devm_regulator_get_optional(dev, "vpcie0v9");
> > > -	if (IS_ERR(rockchip->vpcie0v9)) {
> > > -		if (PTR_ERR(rockchip->vpcie0v9) != -ENODEV)
> > > -			return PTR_ERR(rockchip->vpcie0v9);
> > > -		dev_info(dev, "no vpcie0v9 regulator found\n");
> > > -	}
> > > +	rockchip->vpcie0v9 = devm_regulator_get(dev, "vpcie0v9");
> > > +	if (IS_ERR(rockchip->vpcie0v9))
> > > +		return PTR_ERR(rockchip->vpcie0v9);
> > >   
> > >   	return 0;
> > >   }
> > > @@ -658,27 +652,22 @@ static int rockchip_pcie_set_vpcie(struct rockchip_pcie *rockchip)
> > >   		}
> > >   	}
> > >   
> > > -	if (!IS_ERR(rockchip->vpcie1v8)) {
> > > -		err = regulator_enable(rockchip->vpcie1v8);
> > > -		if (err) {
> > > -			dev_err(dev, "fail to enable vpcie1v8 regulator\n");
> > > -			goto err_disable_3v3;
> > > -		}
> > > +	err = regulator_enable(rockchip->vpcie1v8);
> > > +	if (err) {
> > > +		dev_err(dev, "fail to enable vpcie1v8 regulator\n");
> > > +		goto err_disable_3v3;
> > >   	}
> > >   
> > > -	if (!IS_ERR(rockchip->vpcie0v9)) {
> > > -		err = regulator_enable(rockchip->vpcie0v9);
> > > -		if (err) {
> > > -			dev_err(dev, "fail to enable vpcie0v9 regulator\n");
> > > -			goto err_disable_1v8;
> > > -		}
> > > +	err = regulator_enable(rockchip->vpcie0v9);
> > > +	if (err) {
> > > +		dev_err(dev, "fail to enable vpcie0v9 regulator\n");
> > > +		goto err_disable_1v8;
> > >   	}
> > >   
> > >   	return 0;
> > >   
> > >   err_disable_1v8:
> > > -	if (!IS_ERR(rockchip->vpcie1v8))
> > > -		regulator_disable(rockchip->vpcie1v8);
> > > +	regulator_disable(rockchip->vpcie1v8);
> > >   err_disable_3v3:
> > >   	if (!IS_ERR(rockchip->vpcie3v3))
> > >   		regulator_disable(rockchip->vpcie3v3);
> > > @@ -897,8 +886,7 @@ static int __maybe_unused rockchip_pcie_suspend_noirq(struct device *dev)
> > >   
> > >   	rockchip_pcie_disable_clocks(rockchip);
> > >   
> > > -	if (!IS_ERR(rockchip->vpcie0v9))
> > > -		regulator_disable(rockchip->vpcie0v9);
> > > +	regulator_disable(rockchip->vpcie0v9);
> > >   
> > >   	return ret;
> > >   }
> > > @@ -908,12 +896,10 @@ static int __maybe_unused rockchip_pcie_resume_noirq(struct device *dev)
> > >   	struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
> > >   	int err;
> > >   
> > > -	if (!IS_ERR(rockchip->vpcie0v9)) {
> > > -		err = regulator_enable(rockchip->vpcie0v9);
> > > -		if (err) {
> > > -			dev_err(dev, "fail to enable vpcie0v9 regulator\n");
> > > -			return err;
> > > -		}
> > > +	err = regulator_enable(rockchip->vpcie0v9);
> > > +	if (err) {
> > > +		dev_err(dev, "fail to enable vpcie0v9 regulator\n");
> > > +		return err;
> > >   	}
> > >   
> > >   	err = rockchip_pcie_enable_clocks(rockchip);
> > > @@ -939,8 +925,7 @@ static int __maybe_unused rockchip_pcie_resume_noirq(struct device *dev)
> > >   err_pcie_resume:
> > >   	rockchip_pcie_disable_clocks(rockchip);
> > >   err_disable_0v9:
> > > -	if (!IS_ERR(rockchip->vpcie0v9))
> > > -		regulator_disable(rockchip->vpcie0v9);
> > > +	regulator_disable(rockchip->vpcie0v9);
> > >   	return err;
> > >   }
> > >   
> > > @@ -1081,10 +1066,8 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
> > >   		regulator_disable(rockchip->vpcie12v);
> > >   	if (!IS_ERR(rockchip->vpcie3v3))
> > >   		regulator_disable(rockchip->vpcie3v3);
> > > -	if (!IS_ERR(rockchip->vpcie1v8))
> > > -		regulator_disable(rockchip->vpcie1v8);
> > > -	if (!IS_ERR(rockchip->vpcie0v9))
> > > -		regulator_disable(rockchip->vpcie0v9);
> > > +	regulator_disable(rockchip->vpcie1v8);
> > > +	regulator_disable(rockchip->vpcie0v9);
> > >   err_set_vpcie:
> > >   	rockchip_pcie_disable_clocks(rockchip);
> > >   	return err;
> > > @@ -1108,10 +1091,8 @@ static int rockchip_pcie_remove(struct platform_device *pdev)
> > >   		regulator_disable(rockchip->vpcie12v);
> > >   	if (!IS_ERR(rockchip->vpcie3v3))
> > >   		regulator_disable(rockchip->vpcie3v3);
> > > -	if (!IS_ERR(rockchip->vpcie1v8))
> > > -		regulator_disable(rockchip->vpcie1v8);
> > > -	if (!IS_ERR(rockchip->vpcie0v9))
> > > -		regulator_disable(rockchip->vpcie0v9);
> > > +	regulator_disable(rockchip->vpcie1v8);
> > > +	regulator_disable(rockchip->vpcie0v9);
> > >   
> > >   	return 0;
> > >   }
> > > --
> > > 2.17.1
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel