diff mbox series

[v5,10/12] PCI: brcmstb: Check return value of all reset_control_xxx calls

Message ID 20240731222831.14895-11-james.quinlan@broadcom.com (mailing list archive)
State Superseded
Headers show
Series PCI: brcnstb: Enable STB 7712 SOC | expand

Commit Message

Jim Quinlan July 31, 2024, 10:28 p.m. UTC 
Always check the return value for invocations of reset_control_xxx() and
propagate the error to the next level.  Although the current functions
in reset-brcmstb.c cannot fail, this may someday change.

Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
Reviewed-by: Stanimir Varbanov <svarbanov@suse.de>
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/pci/controller/pcie-brcmstb.c | 102 ++++++++++++++++++--------
 1 file changed, 73 insertions(+), 29 deletions(-)

Comments

Manivannan Sadhasivam Aug. 7, 2024, 2:11 p.m. UTC | #1 
On Wed, Jul 31, 2024 at 06:28:24PM -0400, Jim Quinlan wrote:
> Always check the return value for invocations of reset_control_xxx() and
> propagate the error to the next level.  Although the current functions
> in reset-brcmstb.c cannot fail, this may someday change.
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>

One comment below. With that addressed,

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

> Reviewed-by: Stanimir Varbanov <svarbanov@suse.de>
> Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
> ---
>  drivers/pci/controller/pcie-brcmstb.c | 102 ++++++++++++++++++--------
>  1 file changed, 73 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index 0ecca3d9576f..c4ceb1823a79 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c

[...]

>  static int pci_dev_may_wakeup(struct pci_dev *dev, void *data)
> @@ -1478,9 +1514,12 @@ static int brcm_pcie_suspend_noirq(struct device *dev)
>  {
>  	struct brcm_pcie *pcie = dev_get_drvdata(dev);
>  	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
> -	int ret;
> +	int ret, rret;
> +
> +	ret = brcm_pcie_turn_off(pcie);
> +	if (ret)
> +		return ret;
>  
> -	brcm_pcie_turn_off(pcie);
>  	/*
>  	 * If brcm_phy_stop() returns an error, just dev_err(). If we
>  	 * return the error it will cause the suspend to fail and this is a
> @@ -1509,7 +1548,10 @@ static int brcm_pcie_suspend_noirq(struct device *dev)
>  						     pcie->sr->supplies);
>  			if (ret) {
>  				dev_err(dev, "Could not turn off regulators\n");
> -				reset_control_reset(pcie->rescal);
> +				rret = reset_control_reset(pcie->rescal);
> +				if (rret)
> +					dev_err(dev, "failed to reset 'rascal' controller ret=%d\n",
> +						rret);

I don't think it is really necessary to capture the return value in err path.
Unable to turn off the regulator itself is fatal, so we could just assert reset
and hope for the best.

- Mani
Jim Quinlan Aug. 12, 2024, 6:20 p.m. UTC | #2 
On Wed, Aug 7, 2024 at 10:11 AM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> On Wed, Jul 31, 2024 at 06:28:24PM -0400, Jim Quinlan wrote:
> > Always check the return value for invocations of reset_control_xxx() and
> > propagate the error to the next level.  Although the current functions
> > in reset-brcmstb.c cannot fail, this may someday change.
> >
> > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
>
> One comment below. With that addressed,
>
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>
> > Reviewed-by: Stanimir Varbanov <svarbanov@suse.de>
> > Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
> > ---
> >  drivers/pci/controller/pcie-brcmstb.c | 102 ++++++++++++++++++--------
> >  1 file changed, 73 insertions(+), 29 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > index 0ecca3d9576f..c4ceb1823a79 100644
> > --- a/drivers/pci/controller/pcie-brcmstb.c
> > +++ b/drivers/pci/controller/pcie-brcmstb.c
>
> [...]
>
> >  static int pci_dev_may_wakeup(struct pci_dev *dev, void *data)
> > @@ -1478,9 +1514,12 @@ static int brcm_pcie_suspend_noirq(struct device *dev)
> >  {
> >       struct brcm_pcie *pcie = dev_get_drvdata(dev);
> >       struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
> > -     int ret;
> > +     int ret, rret;
> > +
> > +     ret = brcm_pcie_turn_off(pcie);
> > +     if (ret)
> > +             return ret;
> >
> > -     brcm_pcie_turn_off(pcie);
> >       /*
> >        * If brcm_phy_stop() returns an error, just dev_err(). If we
> >        * return the error it will cause the suspend to fail and this is a
> > @@ -1509,7 +1548,10 @@ static int brcm_pcie_suspend_noirq(struct device *dev)
> >                                                    pcie->sr->supplies);
> >                       if (ret) {
> >                               dev_err(dev, "Could not turn off regulators\n");
> > -                             reset_control_reset(pcie->rescal);
> > +                             rret = reset_control_reset(pcie->rescal);
> > +                             if (rret)
> > +                                     dev_err(dev, "failed to reset 'rascal' controller ret=%d\n",
> > +                                             rret);
>
> I don't think it is really necessary to capture the return value in err path.
> Unable to turn off the regulator itself is fatal, so we could just assert reset
> and hope for the best.

Hi Mani,

I'm not sure what you would like changed.  Failure to turn off the
regulators isn't  necessarily fatal, it's when they cannot be turned
on that is fatal.

There can be two failures here: failure to turn off regulators and
failure to reset the "rascal" reset.  Since I can only return one
error value, I print the other value via dev_err().

Regards,
Jim Quinlan
Broadcom STB/CM


>
> - Mani
>
> --
> மணிவண்ணன் சதாசிவம்
diff mbox series

Patch

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 0ecca3d9576f..c4ceb1823a79 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -239,8 +239,8 @@  struct pcie_cfg_data {
 	const enum pcie_type type;
 	const bool has_phy;
 	unsigned int num_inbound_wins;
-	void (*perst_set)(struct brcm_pcie *pcie, u32 val);
-	void (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
+	int (*perst_set)(struct brcm_pcie *pcie, u32 val);
+	int (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
 };
 
 struct subdev_regulators {
@@ -285,8 +285,8 @@  struct brcm_pcie {
 	int			num_memc;
 	u64			memc_size[PCIE_BRCM_MAX_MEMC];
 	u32			hw_rev;
-	void			(*perst_set)(struct brcm_pcie *pcie, u32 val);
-	void			(*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
+	int			(*perst_set)(struct brcm_pcie *pcie, u32 val);
+	int			(*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
 	struct subdev_regulators *sr;
 	bool			ep_wakeup_capable;
 	bool			has_phy;
@@ -749,12 +749,18 @@  static void __iomem *brcm7425_pcie_map_bus(struct pci_bus *bus,
 	return base + DATA_ADDR(pcie);
 }
 
-static void brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie *pcie, u32 val)
+static int brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie *pcie, u32 val)
 {
+	int ret = 0;
+
 	if (val)
-		reset_control_assert(pcie->bridge_reset);
+		ret = reset_control_assert(pcie->bridge_reset);
 	else
-		reset_control_deassert(pcie->bridge_reset);
+		ret = reset_control_deassert(pcie->bridge_reset);
+
+	if (ret)
+		dev_err(pcie->dev, "failed to %s 'bridge' reset, err=%d\n",
+			val ? "assert" : "deassert", ret);
 
 	if (!pcie->bridge_reset) {
 		u32 tmp, mask =  RGR1_SW_INIT_1_INIT_GENERIC_MASK;
@@ -764,9 +770,11 @@  static void brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie *pcie, u32 val
 		tmp = (tmp & ~mask) | ((val << shift) & mask);
 		writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
 	}
+
+	return ret;
 }
 
-static void brcm_pcie_bridge_sw_init_set_7278(struct brcm_pcie *pcie, u32 val)
+static int brcm_pcie_bridge_sw_init_set_7278(struct brcm_pcie *pcie, u32 val)
 {
 	u32 tmp, mask =  RGR1_SW_INIT_1_INIT_7278_MASK;
 	u32 shift = RGR1_SW_INIT_1_INIT_7278_SHIFT;
@@ -774,20 +782,29 @@  static void brcm_pcie_bridge_sw_init_set_7278(struct brcm_pcie *pcie, u32 val)
 	tmp = readl(pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
 	tmp = (tmp & ~mask) | ((val << shift) & mask);
 	writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
+
+	return 0;
 }
 
-static void brcm_pcie_perst_set_4908(struct brcm_pcie *pcie, u32 val)
+static int brcm_pcie_perst_set_4908(struct brcm_pcie *pcie, u32 val)
 {
+	int ret;
+
 	if (WARN_ONCE(!pcie->perst_reset, "missing PERST# reset controller\n"))
-		return;
+		return -EINVAL;
 
 	if (val)
-		reset_control_assert(pcie->perst_reset);
+		ret = reset_control_assert(pcie->perst_reset);
 	else
-		reset_control_deassert(pcie->perst_reset);
+		ret = reset_control_deassert(pcie->perst_reset);
+
+	if (ret)
+		dev_err(pcie->dev, "failed to %s 'perst' reset, err=%d\n",
+			val ? "assert" : "deassert", ret);
+	return ret;
 }
 
-static void brcm_pcie_perst_set_7278(struct brcm_pcie *pcie, u32 val)
+static int brcm_pcie_perst_set_7278(struct brcm_pcie *pcie, u32 val)
 {
 	u32 tmp;
 
@@ -795,15 +812,19 @@  static void brcm_pcie_perst_set_7278(struct brcm_pcie *pcie, u32 val)
 	tmp = readl(pcie->base + PCIE_MISC_PCIE_CTRL);
 	u32p_replace_bits(&tmp, !val, PCIE_MISC_PCIE_CTRL_PCIE_PERSTB_MASK);
 	writel(tmp, pcie->base +  PCIE_MISC_PCIE_CTRL);
+
+	return 0;
 }
 
-static void brcm_pcie_perst_set_generic(struct brcm_pcie *pcie, u32 val)
+static int brcm_pcie_perst_set_generic(struct brcm_pcie *pcie, u32 val)
 {
 	u32 tmp;
 
 	tmp = readl(pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
 	u32p_replace_bits(&tmp, val, PCIE_RGR1_SW_INIT_1_PERST_MASK);
 	writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
+
+	return 0;
 }
 
 static inline void set_bar(struct inbound_win *b, int *count, u64 size,
@@ -1016,19 +1037,28 @@  static int brcm_pcie_setup(struct brcm_pcie *pcie)
 	struct resource_entry *entry;
 	u32 tmp, burst, aspm_support;
 	int num_out_wins = 0, num_rc_bars = 0;
-	int memc;
+	int memc, ret;
 
 	/* Reset the bridge */
-	pcie->bridge_sw_init_set(pcie, 1);
+	ret = pcie->bridge_sw_init_set(pcie, 1);
+	if (ret)
+		return ret;
 
 	/* Ensure that PERST# is asserted; some bootloaders may deassert it. */
-	if (pcie->type == BCM2711)
-		pcie->perst_set(pcie, 1);
+	if (pcie->type == BCM2711) {
+		ret = pcie->perst_set(pcie, 1);
+		if (ret) {
+			pcie->bridge_sw_init_set(pcie, 0);
+			return ret;
+		}
+	}
 
 	usleep_range(100, 200);
 
 	/* Take the bridge out of reset */
-	pcie->bridge_sw_init_set(pcie, 0);
+	ret = pcie->bridge_sw_init_set(pcie, 0);
+	if (ret)
+		return ret;
 
 	tmp = readl(base + HARD_DEBUG(pcie));
 	if (is_bmips(pcie))
@@ -1247,7 +1277,9 @@  static int brcm_pcie_start_link(struct brcm_pcie *pcie)
 	int ret, i;
 
 	/* Unassert the fundamental reset */
-	pcie->perst_set(pcie, 0);
+	ret = pcie->perst_set(pcie, 0);
+	if (ret)
+		return ret;
 
 	/*
 	 * Wait for 100ms after PERST# deassertion; see PCIe CEM specification
@@ -1439,15 +1471,17 @@  static inline int brcm_phy_stop(struct brcm_pcie *pcie)
 	return pcie->has_phy ? brcm_phy_cntl(pcie, 0) : 0;
 }
 
-static void brcm_pcie_turn_off(struct brcm_pcie *pcie)
+static int brcm_pcie_turn_off(struct brcm_pcie *pcie)
 {
 	void __iomem *base = pcie->base;
-	int tmp;
+	int tmp, ret;
 
 	if (brcm_pcie_link_up(pcie))
 		brcm_pcie_enter_l23(pcie);
 	/* Assert fundamental reset */
-	pcie->perst_set(pcie, 1);
+	ret = pcie->perst_set(pcie, 1);
+	if (ret)
+		return ret;
 
 	/* Deassert request for L23 in case it was asserted */
 	tmp = readl(base + PCIE_MISC_PCIE_CTRL);
@@ -1460,7 +1494,9 @@  static void brcm_pcie_turn_off(struct brcm_pcie *pcie)
 	writel(tmp, base + HARD_DEBUG(pcie));
 
 	/* Shutdown PCIe bridge */
-	pcie->bridge_sw_init_set(pcie, 1);
+	ret = pcie->bridge_sw_init_set(pcie, 1);
+
+	return ret;
 }
 
 static int pci_dev_may_wakeup(struct pci_dev *dev, void *data)
@@ -1478,9 +1514,12 @@  static int brcm_pcie_suspend_noirq(struct device *dev)
 {
 	struct brcm_pcie *pcie = dev_get_drvdata(dev);
 	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
-	int ret;
+	int ret, rret;
+
+	ret = brcm_pcie_turn_off(pcie);
+	if (ret)
+		return ret;
 
-	brcm_pcie_turn_off(pcie);
 	/*
 	 * If brcm_phy_stop() returns an error, just dev_err(). If we
 	 * return the error it will cause the suspend to fail and this is a
@@ -1509,7 +1548,10 @@  static int brcm_pcie_suspend_noirq(struct device *dev)
 						     pcie->sr->supplies);
 			if (ret) {
 				dev_err(dev, "Could not turn off regulators\n");
-				reset_control_reset(pcie->rescal);
+				rret = reset_control_reset(pcie->rescal);
+				if (rret)
+					dev_err(dev, "failed to reset 'rascal' controller ret=%d\n",
+						rret);
 				return ret;
 			}
 		}
@@ -1524,7 +1566,7 @@  static int brcm_pcie_resume_noirq(struct device *dev)
 	struct brcm_pcie *pcie = dev_get_drvdata(dev);
 	void __iomem *base;
 	u32 tmp;
-	int ret;
+	int ret, rret;
 
 	base = pcie->base;
 	ret = clk_prepare_enable(pcie->clk);
@@ -1586,7 +1628,9 @@  static int brcm_pcie_resume_noirq(struct device *dev)
 	if (pcie->sr)
 		regulator_bulk_disable(pcie->sr->num_supplies, pcie->sr->supplies);
 err_reset:
-	reset_control_rearm(pcie->rescal);
+	rret = reset_control_rearm(pcie->rescal);
+	if (rret)
+		dev_err(pcie->dev, "failed to rearm 'rescal' reset, err=%d\n", rret);
 err_disable_clk:
 	clk_disable_unprepare(pcie->clk);
 	return ret;