| Message ID | 20190826073143.4582-7-vidyas@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | PCI: tegra: Enable PCIe C5 controller of Tegra194 in p2972-0000 platform | expand |
On Mon, Aug 26, 2019 at 01:01:43PM +0530, Vidya Sagar wrote: > Add support to get regulator information of 3.3V and 12V supplies of a PCIe > slot from the respective controller's device-tree node and enable those > supplies. This is required in platforms like p2972-0000 where the supplies > to x16 slot owned by C5 controller need to be enabled before attempting to > enumerate the devices. > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > --- > drivers/pci/controller/dwc/pcie-tegra194.c | 65 ++++++++++++++++++++++ > 1 file changed, 65 insertions(+) > > diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c > index 8a27b25893c9..97de2151a738 100644 > --- a/drivers/pci/controller/dwc/pcie-tegra194.c > +++ b/drivers/pci/controller/dwc/pcie-tegra194.c > @@ -278,6 +278,8 @@ struct tegra_pcie_dw { > u32 aspm_l0s_enter_lat; > > struct regulator *pex_ctl_supply; > + struct regulator *slot_ctl_3v3; > + struct regulator *slot_ctl_12v; > > unsigned int phy_count; > struct phy **phys; > @@ -1047,6 +1049,59 @@ static void tegra_pcie_downstream_dev_to_D0(struct tegra_pcie_dw *pcie) > } > } > > +static void tegra_pcie_get_slot_regulators(struct tegra_pcie_dw *pcie) > +{ > + pcie->slot_ctl_3v3 = devm_regulator_get_optional(pcie->dev, "vpcie3v3"); > + if (IS_ERR(pcie->slot_ctl_3v3)) > + pcie->slot_ctl_3v3 = NULL; > + > + pcie->slot_ctl_12v = devm_regulator_get_optional(pcie->dev, "vpcie12v"); > + if (IS_ERR(pcie->slot_ctl_12v)) > + pcie->slot_ctl_12v = NULL; Do these need to take into consideration -EPROBE_DEFER? Thanks, Andrew Murray > +} > + > +static int tegra_pcie_enable_slot_regulators(struct tegra_pcie_dw *pcie) > +{ > + int ret; > + > + if (pcie->slot_ctl_3v3) { > + ret = regulator_enable(pcie->slot_ctl_3v3); > + if (ret < 0) { > + dev_err(pcie->dev, > + "Failed to enable 3V3 slot supply: %d\n", ret); > + return ret; > + } > + } > + > + if (pcie->slot_ctl_12v) { > + ret = regulator_enable(pcie->slot_ctl_12v); > + if (ret < 0) { > + dev_err(pcie->dev, > + "Failed to enable 12V slot supply: %d\n", ret); > + if (pcie->slot_ctl_3v3) > + regulator_disable(pcie->slot_ctl_3v3); > + return ret; > + } > + } > + > + /* > + * According to PCI Express Card Electromechanical Specification > + * Revision 1.1, Table-2.4, T_PVPERL (Power stable to PERST# inactive) > + * should be a minimum of 100ms. > + */ > + msleep(100); > + > + return 0; > +} > + > +static void tegra_pcie_disable_slot_regulators(struct tegra_pcie_dw *pcie) > +{ > + if (pcie->slot_ctl_12v) > + regulator_disable(pcie->slot_ctl_12v); > + if (pcie->slot_ctl_3v3) > + regulator_disable(pcie->slot_ctl_3v3); > +} > + > static int tegra_pcie_config_controller(struct tegra_pcie_dw *pcie, > bool en_hw_hot_rst) > { > @@ -1060,6 +1115,10 @@ static int tegra_pcie_config_controller(struct tegra_pcie_dw *pcie, > return ret; > } > > + ret = tegra_pcie_enable_slot_regulators(pcie); > + if (ret < 0) > + goto fail_slot_reg_en; > + > ret = regulator_enable(pcie->pex_ctl_supply); > if (ret < 0) { > dev_err(pcie->dev, "Failed to enable regulator: %d\n", ret); > @@ -1142,6 +1201,8 @@ static int tegra_pcie_config_controller(struct tegra_pcie_dw *pcie, > fail_core_clk: > regulator_disable(pcie->pex_ctl_supply); > fail_reg_en: > + tegra_pcie_disable_slot_regulators(pcie); > +fail_slot_reg_en: > tegra_pcie_bpmp_set_ctrl_state(pcie, false); > > return ret; > @@ -1174,6 +1235,8 @@ static int __deinit_controller(struct tegra_pcie_dw *pcie) > return ret; > } > > + tegra_pcie_disable_slot_regulators(pcie); > + > ret = tegra_pcie_bpmp_set_ctrl_state(pcie, false); > if (ret) { > dev_err(pcie->dev, "Failed to disable controller %d: %d\n", > @@ -1372,6 +1435,8 @@ static int tegra_pcie_dw_probe(struct platform_device *pdev) > return ret; > } > > + tegra_pcie_get_slot_regulators(pcie); > + > pcie->pex_ctl_supply = devm_regulator_get(dev, "vddio-pex-ctl"); > if (IS_ERR(pcie->pex_ctl_supply)) { > dev_err(dev, "Failed to get regulator: %ld\n", > -- > 2.17.1 >
On 8/27/2019 9:17 PM, Andrew Murray wrote: > On Mon, Aug 26, 2019 at 01:01:43PM +0530, Vidya Sagar wrote: >> Add support to get regulator information of 3.3V and 12V supplies of a PCIe >> slot from the respective controller's device-tree node and enable those >> supplies. This is required in platforms like p2972-0000 where the supplies >> to x16 slot owned by C5 controller need to be enabled before attempting to >> enumerate the devices. >> >> Signed-off-by: Vidya Sagar <vidyas@nvidia.com> >> --- >> drivers/pci/controller/dwc/pcie-tegra194.c | 65 ++++++++++++++++++++++ >> 1 file changed, 65 insertions(+) >> >> diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c >> index 8a27b25893c9..97de2151a738 100644 >> --- a/drivers/pci/controller/dwc/pcie-tegra194.c >> +++ b/drivers/pci/controller/dwc/pcie-tegra194.c >> @@ -278,6 +278,8 @@ struct tegra_pcie_dw { >> u32 aspm_l0s_enter_lat; >> >> struct regulator *pex_ctl_supply; >> + struct regulator *slot_ctl_3v3; >> + struct regulator *slot_ctl_12v; >> >> unsigned int phy_count; >> struct phy **phys; >> @@ -1047,6 +1049,59 @@ static void tegra_pcie_downstream_dev_to_D0(struct tegra_pcie_dw *pcie) >> } >> } >> >> +static void tegra_pcie_get_slot_regulators(struct tegra_pcie_dw *pcie) >> +{ >> + pcie->slot_ctl_3v3 = devm_regulator_get_optional(pcie->dev, "vpcie3v3"); >> + if (IS_ERR(pcie->slot_ctl_3v3)) >> + pcie->slot_ctl_3v3 = NULL; >> + >> + pcie->slot_ctl_12v = devm_regulator_get_optional(pcie->dev, "vpcie12v"); >> + if (IS_ERR(pcie->slot_ctl_12v)) >> + pcie->slot_ctl_12v = NULL; > > Do these need to take into consideration -EPROBE_DEFER? Since these are devm_* APIs, isn't it taken care of automatically? > > Thanks, > > Andrew Murray > >> +} >> + >> +static int tegra_pcie_enable_slot_regulators(struct tegra_pcie_dw *pcie) >> +{ >> + int ret; >> + >> + if (pcie->slot_ctl_3v3) { >> + ret = regulator_enable(pcie->slot_ctl_3v3); >> + if (ret < 0) { >> + dev_err(pcie->dev, >> + "Failed to enable 3V3 slot supply: %d\n", ret); >> + return ret; >> + } >> + } >> + >> + if (pcie->slot_ctl_12v) { >> + ret = regulator_enable(pcie->slot_ctl_12v); >> + if (ret < 0) { >> + dev_err(pcie->dev, >> + "Failed to enable 12V slot supply: %d\n", ret); >> + if (pcie->slot_ctl_3v3) >> + regulator_disable(pcie->slot_ctl_3v3); >> + return ret; >> + } >> + } >> + >> + /* >> + * According to PCI Express Card Electromechanical Specification >> + * Revision 1.1, Table-2.4, T_PVPERL (Power stable to PERST# inactive) >> + * should be a minimum of 100ms. >> + */ >> + msleep(100); >> + >> + return 0; >> +} >> + >> +static void tegra_pcie_disable_slot_regulators(struct tegra_pcie_dw *pcie) >> +{ >> + if (pcie->slot_ctl_12v) >> + regulator_disable(pcie->slot_ctl_12v); >> + if (pcie->slot_ctl_3v3) >> + regulator_disable(pcie->slot_ctl_3v3); >> +} >> + >> static int tegra_pcie_config_controller(struct tegra_pcie_dw *pcie, >> bool en_hw_hot_rst) >> { >> @@ -1060,6 +1115,10 @@ static int tegra_pcie_config_controller(struct tegra_pcie_dw *pcie, >> return ret; >> } >> >> + ret = tegra_pcie_enable_slot_regulators(pcie); >> + if (ret < 0) >> + goto fail_slot_reg_en; >> + >> ret = regulator_enable(pcie->pex_ctl_supply); >> if (ret < 0) { >> dev_err(pcie->dev, "Failed to enable regulator: %d\n", ret); >> @@ -1142,6 +1201,8 @@ static int tegra_pcie_config_controller(struct tegra_pcie_dw *pcie, >> fail_core_clk: >> regulator_disable(pcie->pex_ctl_supply); >> fail_reg_en: >> + tegra_pcie_disable_slot_regulators(pcie); >> +fail_slot_reg_en: >> tegra_pcie_bpmp_set_ctrl_state(pcie, false); >> >> return ret; >> @@ -1174,6 +1235,8 @@ static int __deinit_controller(struct tegra_pcie_dw *pcie) >> return ret; >> } >> >> + tegra_pcie_disable_slot_regulators(pcie); >> + >> ret = tegra_pcie_bpmp_set_ctrl_state(pcie, false); >> if (ret) { >> dev_err(pcie->dev, "Failed to disable controller %d: %d\n", >> @@ -1372,6 +1435,8 @@ static int tegra_pcie_dw_probe(struct platform_device *pdev) >> return ret; >> } >> >> + tegra_pcie_get_slot_regulators(pcie); >> + >> pcie->pex_ctl_supply = devm_regulator_get(dev, "vddio-pex-ctl"); >> if (IS_ERR(pcie->pex_ctl_supply)) { >> dev_err(dev, "Failed to get regulator: %ld\n", >> -- >> 2.17.1 >>
On Tue, Aug 27, 2019 at 09:54:17PM +0530, Vidya Sagar wrote: > On 8/27/2019 9:17 PM, Andrew Murray wrote: > > On Mon, Aug 26, 2019 at 01:01:43PM +0530, Vidya Sagar wrote: > > > Add support to get regulator information of 3.3V and 12V supplies of a PCIe > > > slot from the respective controller's device-tree node and enable those > > > supplies. This is required in platforms like p2972-0000 where the supplies > > > to x16 slot owned by C5 controller need to be enabled before attempting to > > > enumerate the devices. > > > > > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > > > --- > > > drivers/pci/controller/dwc/pcie-tegra194.c | 65 ++++++++++++++++++++++ > > > 1 file changed, 65 insertions(+) > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c > > > index 8a27b25893c9..97de2151a738 100644 > > > --- a/drivers/pci/controller/dwc/pcie-tegra194.c > > > +++ b/drivers/pci/controller/dwc/pcie-tegra194.c > > > @@ -278,6 +278,8 @@ struct tegra_pcie_dw { > > > u32 aspm_l0s_enter_lat; > > > struct regulator *pex_ctl_supply; > > > + struct regulator *slot_ctl_3v3; > > > + struct regulator *slot_ctl_12v; > > > unsigned int phy_count; > > > struct phy **phys; > > > @@ -1047,6 +1049,59 @@ static void tegra_pcie_downstream_dev_to_D0(struct tegra_pcie_dw *pcie) > > > } > > > } > > > +static void tegra_pcie_get_slot_regulators(struct tegra_pcie_dw *pcie) > > > +{ > > > + pcie->slot_ctl_3v3 = devm_regulator_get_optional(pcie->dev, "vpcie3v3"); > > > + if (IS_ERR(pcie->slot_ctl_3v3)) > > > + pcie->slot_ctl_3v3 = NULL; > > > + > > > + pcie->slot_ctl_12v = devm_regulator_get_optional(pcie->dev, "vpcie12v"); > > > + if (IS_ERR(pcie->slot_ctl_12v)) > > > + pcie->slot_ctl_12v = NULL; > > > > Do these need to take into consideration -EPROBE_DEFER? > Since these are devm_* APIs, isn't it taken care of automatically? devm_regulator_get_optional can still return -EPROBE_DEFER - for times when "lookup could succeed in the future". It's probably helpful here for your driver to distinguish between there not being a regulator specified in the DT, and there being a regulator but there is no device for it yet. For the latter case - your driver would probe but nothing would enumerate. See pcie-rockchip-host.c for an example of where this is handled. Of course if, for whatever reason it is unlikely you'll ever get -EPROBE_DEFER then maybe it's OK as it is. Thanks, Andrew Murray > > > > > Thanks, > > > > Andrew Murray > > > > > +} > > > + > > > +static int tegra_pcie_enable_slot_regulators(struct tegra_pcie_dw *pcie) > > > +{ > > > + int ret; > > > + > > > + if (pcie->slot_ctl_3v3) { > > > + ret = regulator_enable(pcie->slot_ctl_3v3); > > > + if (ret < 0) { > > > + dev_err(pcie->dev, > > > + "Failed to enable 3V3 slot supply: %d\n", ret); > > > + return ret; > > > + } > > > + } > > > + > > > + if (pcie->slot_ctl_12v) { > > > + ret = regulator_enable(pcie->slot_ctl_12v); > > > + if (ret < 0) { > > > + dev_err(pcie->dev, > > > + "Failed to enable 12V slot supply: %d\n", ret); > > > + if (pcie->slot_ctl_3v3) > > > + regulator_disable(pcie->slot_ctl_3v3); > > > + return ret; > > > + } > > > + } > > > + > > > + /* > > > + * According to PCI Express Card Electromechanical Specification > > > + * Revision 1.1, Table-2.4, T_PVPERL (Power stable to PERST# inactive) > > > + * should be a minimum of 100ms. > > > + */ > > > + msleep(100); > > > + > > > + return 0; > > > +} > > > + > > > +static void tegra_pcie_disable_slot_regulators(struct tegra_pcie_dw *pcie) > > > +{ > > > + if (pcie->slot_ctl_12v) > > > + regulator_disable(pcie->slot_ctl_12v); > > > + if (pcie->slot_ctl_3v3) > > > + regulator_disable(pcie->slot_ctl_3v3); > > > +} > > > + > > > static int tegra_pcie_config_controller(struct tegra_pcie_dw *pcie, > > > bool en_hw_hot_rst) > > > { > > > @@ -1060,6 +1115,10 @@ static int tegra_pcie_config_controller(struct tegra_pcie_dw *pcie, > > > return ret; > > > } > > > + ret = tegra_pcie_enable_slot_regulators(pcie); > > > + if (ret < 0) > > > + goto fail_slot_reg_en; > > > + > > > ret = regulator_enable(pcie->pex_ctl_supply); > > > if (ret < 0) { > > > dev_err(pcie->dev, "Failed to enable regulator: %d\n", ret); > > > @@ -1142,6 +1201,8 @@ static int tegra_pcie_config_controller(struct tegra_pcie_dw *pcie, > > > fail_core_clk: > > > regulator_disable(pcie->pex_ctl_supply); > > > fail_reg_en: > > > + tegra_pcie_disable_slot_regulators(pcie); > > > +fail_slot_reg_en: > > > tegra_pcie_bpmp_set_ctrl_state(pcie, false); > > > return ret; > > > @@ -1174,6 +1235,8 @@ static int __deinit_controller(struct tegra_pcie_dw *pcie) > > > return ret; > > > } > > > + tegra_pcie_disable_slot_regulators(pcie); > > > + > > > ret = tegra_pcie_bpmp_set_ctrl_state(pcie, false); > > > if (ret) { > > > dev_err(pcie->dev, "Failed to disable controller %d: %d\n", > > > @@ -1372,6 +1435,8 @@ static int tegra_pcie_dw_probe(struct platform_device *pdev) > > > return ret; > > > } > > > + tegra_pcie_get_slot_regulators(pcie); > > > + > > > pcie->pex_ctl_supply = devm_regulator_get(dev, "vddio-pex-ctl"); > > > if (IS_ERR(pcie->pex_ctl_supply)) { > > > dev_err(dev, "Failed to get regulator: %ld\n", > > > -- > > > 2.17.1 > > > >
On Tue, Aug 27, 2019 at 06:13:34PM +0100, Andrew Murray wrote: > On Tue, Aug 27, 2019 at 09:54:17PM +0530, Vidya Sagar wrote: > > On 8/27/2019 9:17 PM, Andrew Murray wrote: > > > On Mon, Aug 26, 2019 at 01:01:43PM +0530, Vidya Sagar wrote: > > > > Add support to get regulator information of 3.3V and 12V supplies of a PCIe > > > > slot from the respective controller's device-tree node and enable those > > > > supplies. This is required in platforms like p2972-0000 where the supplies > > > > to x16 slot owned by C5 controller need to be enabled before attempting to > > > > enumerate the devices. > > > > > > > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > > > > --- > > > > drivers/pci/controller/dwc/pcie-tegra194.c | 65 ++++++++++++++++++++++ > > > > 1 file changed, 65 insertions(+) > > > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c > > > > index 8a27b25893c9..97de2151a738 100644 > > > > --- a/drivers/pci/controller/dwc/pcie-tegra194.c > > > > +++ b/drivers/pci/controller/dwc/pcie-tegra194.c > > > > @@ -278,6 +278,8 @@ struct tegra_pcie_dw { > > > > u32 aspm_l0s_enter_lat; > > > > struct regulator *pex_ctl_supply; > > > > + struct regulator *slot_ctl_3v3; > > > > + struct regulator *slot_ctl_12v; > > > > unsigned int phy_count; > > > > struct phy **phys; > > > > @@ -1047,6 +1049,59 @@ static void tegra_pcie_downstream_dev_to_D0(struct tegra_pcie_dw *pcie) > > > > } > > > > } > > > > +static void tegra_pcie_get_slot_regulators(struct tegra_pcie_dw *pcie) > > > > +{ > > > > + pcie->slot_ctl_3v3 = devm_regulator_get_optional(pcie->dev, "vpcie3v3"); > > > > + if (IS_ERR(pcie->slot_ctl_3v3)) > > > > + pcie->slot_ctl_3v3 = NULL; > > > > + > > > > + pcie->slot_ctl_12v = devm_regulator_get_optional(pcie->dev, "vpcie12v"); > > > > + if (IS_ERR(pcie->slot_ctl_12v)) > > > > + pcie->slot_ctl_12v = NULL; > > > > > > Do these need to take into consideration -EPROBE_DEFER? > > Since these are devm_* APIs, isn't it taken care of automatically? > > devm_regulator_get_optional can still return -EPROBE_DEFER - for times when > "lookup could succeed in the future". > > It's probably helpful here for your driver to distinguish between there not > being a regulator specified in the DT, and there being a regulator but there > is no device for it yet. For the latter case - your driver would probe but > nothing would enumerate. > > See pcie-rockchip-host.c for an example of where this is handled. > > Of course if, for whatever reason it is unlikely you'll ever get -EPROBE_DEFER > then maybe it's OK as it is. Let's not assume that. We've just recently encountered a case where we did not handle -EPROBE_DEFER because we had assumed too much, and that turned into a bit of a hassle to fix. Vidya, I think what Andrew is saying is that you need to propagate the -EPROBE_DEFER error to the caller (i.e. the ->probe() callback) so that the PCI controller driver can be properly added to the defer queue in case the regulator isn't ready yet. I think what we want here is something like: pcie->slot_ctl_3v3 = devm_regulator_get_optional(pcie->dev, "vpcie3v3"); if (IS_ERR(pcie->slot_ctl_3v3)) { if (PTR_ERR(pcie->slot_ctl_3v3) != -ENODEV) return PTR_ERR(pcie->slot_ctl_3v3); pcie->slot_ctl_3v3 = NULL; } Andrew, I'm not sure the handling in rockchip_pcie_parse_host_dt() is correct. It singles out -EPROBE_DEFER, which I think is the wrong way around. We should be special-casing -ENODEV, because regulator_get() can return a wide array of error cases, not all of which we actually want to consider successes. For example we could be getting -ENOMEM, which, I would argue, is something that we should propagate. I think it'd be very confusing to take that as meaning "optional regulator wasn't specified", because in that case the DTS file would've had the regulator hooked up (we have to assume that it is needed in that case) but we won't be enabling it, so it's unlikely that devices will enumerate. Thierry
On Wed, Aug 28, 2019 at 11:07:57AM +0200, Thierry Reding wrote: > On Tue, Aug 27, 2019 at 06:13:34PM +0100, Andrew Murray wrote: > > On Tue, Aug 27, 2019 at 09:54:17PM +0530, Vidya Sagar wrote: > > > On 8/27/2019 9:17 PM, Andrew Murray wrote: > > > > On Mon, Aug 26, 2019 at 01:01:43PM +0530, Vidya Sagar wrote: > > > > > Add support to get regulator information of 3.3V and 12V supplies of a PCIe > > > > > slot from the respective controller's device-tree node and enable those > > > > > supplies. This is required in platforms like p2972-0000 where the supplies > > > > > to x16 slot owned by C5 controller need to be enabled before attempting to > > > > > enumerate the devices. > > > > > > > > > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > > > > > --- > > > > > drivers/pci/controller/dwc/pcie-tegra194.c | 65 ++++++++++++++++++++++ > > > > > 1 file changed, 65 insertions(+) > > > > > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c > > > > > index 8a27b25893c9..97de2151a738 100644 > > > > > --- a/drivers/pci/controller/dwc/pcie-tegra194.c > > > > > +++ b/drivers/pci/controller/dwc/pcie-tegra194.c > > > > > @@ -278,6 +278,8 @@ struct tegra_pcie_dw { > > > > > u32 aspm_l0s_enter_lat; > > > > > struct regulator *pex_ctl_supply; > > > > > + struct regulator *slot_ctl_3v3; > > > > > + struct regulator *slot_ctl_12v; > > > > > unsigned int phy_count; > > > > > struct phy **phys; > > > > > @@ -1047,6 +1049,59 @@ static void tegra_pcie_downstream_dev_to_D0(struct tegra_pcie_dw *pcie) > > > > > } > > > > > } > > > > > +static void tegra_pcie_get_slot_regulators(struct tegra_pcie_dw *pcie) > > > > > +{ > > > > > + pcie->slot_ctl_3v3 = devm_regulator_get_optional(pcie->dev, "vpcie3v3"); > > > > > + if (IS_ERR(pcie->slot_ctl_3v3)) > > > > > + pcie->slot_ctl_3v3 = NULL; > > > > > + > > > > > + pcie->slot_ctl_12v = devm_regulator_get_optional(pcie->dev, "vpcie12v"); > > > > > + if (IS_ERR(pcie->slot_ctl_12v)) > > > > > + pcie->slot_ctl_12v = NULL; > > > > > > > > Do these need to take into consideration -EPROBE_DEFER? > > > Since these are devm_* APIs, isn't it taken care of automatically? > > > > devm_regulator_get_optional can still return -EPROBE_DEFER - for times when > > "lookup could succeed in the future". > > > > It's probably helpful here for your driver to distinguish between there not > > being a regulator specified in the DT, and there being a regulator but there > > is no device for it yet. For the latter case - your driver would probe but > > nothing would enumerate. > > > > See pcie-rockchip-host.c for an example of where this is handled. > > > > Of course if, for whatever reason it is unlikely you'll ever get -EPROBE_DEFER > > then maybe it's OK as it is. > > Let's not assume that. We've just recently encountered a case where we > did not handle -EPROBE_DEFER because we had assumed too much, and that > turned into a bit of a hassle to fix. > > Vidya, I think what Andrew is saying is that you need to propagate the > -EPROBE_DEFER error to the caller (i.e. the ->probe() callback) so that > the PCI controller driver can be properly added to the defer queue in > case the regulator isn't ready yet. Indeed. > > I think what we want here is something like: > > pcie->slot_ctl_3v3 = devm_regulator_get_optional(pcie->dev, "vpcie3v3"); > if (IS_ERR(pcie->slot_ctl_3v3)) { > if (PTR_ERR(pcie->slot_ctl_3v3) != -ENODEV) > return PTR_ERR(pcie->slot_ctl_3v3); > > pcie->slot_ctl_3v3 = NULL; > } > > Andrew, I'm not sure the handling in rockchip_pcie_parse_host_dt() is > correct. It singles out -EPROBE_DEFER, which I think is the wrong way > around. We should be special-casing -ENODEV, because regulator_get() > can return a wide array of error cases, not all of which we actually > want to consider successes. For example we could be getting -ENOMEM, > which, I would argue, is something that we should propagate. Yes I completely agree, given that the regulator is optional: we only want to proceed if we find the regulator or if a regulator wasn't specified in the DT. We should fail upon any other error, in case a regulator was specified but the error prevented it from being returned. > I think > it'd be very confusing to take that as meaning "optional regulator > wasn't specified", because in that case the DTS file would've had the > regulator hooked up (we have to assume that it is needed in that case) > but we won't be enabling it, so it's unlikely that devices will > enumerate. Thanks, Andrew Murray > > Thierry
diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c index 8a27b25893c9..97de2151a738 100644 --- a/drivers/pci/controller/dwc/pcie-tegra194.c +++ b/drivers/pci/controller/dwc/pcie-tegra194.c @@ -278,6 +278,8 @@ struct tegra_pcie_dw { u32 aspm_l0s_enter_lat; struct regulator *pex_ctl_supply; + struct regulator *slot_ctl_3v3; + struct regulator *slot_ctl_12v; unsigned int phy_count; struct phy **phys; @@ -1047,6 +1049,59 @@ static void tegra_pcie_downstream_dev_to_D0(struct tegra_pcie_dw *pcie) } } +static void tegra_pcie_get_slot_regulators(struct tegra_pcie_dw *pcie) +{ + pcie->slot_ctl_3v3 = devm_regulator_get_optional(pcie->dev, "vpcie3v3"); + if (IS_ERR(pcie->slot_ctl_3v3)) + pcie->slot_ctl_3v3 = NULL; + + pcie->slot_ctl_12v = devm_regulator_get_optional(pcie->dev, "vpcie12v"); + if (IS_ERR(pcie->slot_ctl_12v)) + pcie->slot_ctl_12v = NULL; +} + +static int tegra_pcie_enable_slot_regulators(struct tegra_pcie_dw *pcie) +{ + int ret; + + if (pcie->slot_ctl_3v3) { + ret = regulator_enable(pcie->slot_ctl_3v3); + if (ret < 0) { + dev_err(pcie->dev, + "Failed to enable 3V3 slot supply: %d\n", ret); + return ret; + } + } + + if (pcie->slot_ctl_12v) { + ret = regulator_enable(pcie->slot_ctl_12v); + if (ret < 0) { + dev_err(pcie->dev, + "Failed to enable 12V slot supply: %d\n", ret); + if (pcie->slot_ctl_3v3) + regulator_disable(pcie->slot_ctl_3v3); + return ret; + } + } + + /* + * According to PCI Express Card Electromechanical Specification + * Revision 1.1, Table-2.4, T_PVPERL (Power stable to PERST# inactive) + * should be a minimum of 100ms. + */ + msleep(100); + + return 0; +} + +static void tegra_pcie_disable_slot_regulators(struct tegra_pcie_dw *pcie) +{ + if (pcie->slot_ctl_12v) + regulator_disable(pcie->slot_ctl_12v); + if (pcie->slot_ctl_3v3) + regulator_disable(pcie->slot_ctl_3v3); +} + static int tegra_pcie_config_controller(struct tegra_pcie_dw *pcie, bool en_hw_hot_rst) { @@ -1060,6 +1115,10 @@ static int tegra_pcie_config_controller(struct tegra_pcie_dw *pcie, return ret; } + ret = tegra_pcie_enable_slot_regulators(pcie); + if (ret < 0) + goto fail_slot_reg_en; + ret = regulator_enable(pcie->pex_ctl_supply); if (ret < 0) { dev_err(pcie->dev, "Failed to enable regulator: %d\n", ret); @@ -1142,6 +1201,8 @@ static int tegra_pcie_config_controller(struct tegra_pcie_dw *pcie, fail_core_clk: regulator_disable(pcie->pex_ctl_supply); fail_reg_en: + tegra_pcie_disable_slot_regulators(pcie); +fail_slot_reg_en: tegra_pcie_bpmp_set_ctrl_state(pcie, false); return ret; @@ -1174,6 +1235,8 @@ static int __deinit_controller(struct tegra_pcie_dw *pcie) return ret; } + tegra_pcie_disable_slot_regulators(pcie); + ret = tegra_pcie_bpmp_set_ctrl_state(pcie, false); if (ret) { dev_err(pcie->dev, "Failed to disable controller %d: %d\n", @@ -1372,6 +1435,8 @@ static int tegra_pcie_dw_probe(struct platform_device *pdev) return ret; } + tegra_pcie_get_slot_regulators(pcie); + pcie->pex_ctl_supply = devm_regulator_get(dev, "vddio-pex-ctl"); if (IS_ERR(pcie->pex_ctl_supply)) { dev_err(dev, "Failed to get regulator: %ld\n",
Add support to get regulator information of 3.3V and 12V supplies of a PCIe slot from the respective controller's device-tree node and enable those supplies. This is required in platforms like p2972-0000 where the supplies to x16 slot owned by C5 controller need to be enabled before attempting to enumerate the devices. Signed-off-by: Vidya Sagar <vidyas@nvidia.com> --- drivers/pci/controller/dwc/pcie-tegra194.c | 65 ++++++++++++++++++++++ 1 file changed, 65 insertions(+)