| Message ID | 20200519203419.12369-8-james.quinlan@broadcom.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | PCI: brcmstb: enable PCIe for STB chips | expand |
Hi Jim, On Tue, May 19, 2020 at 04:34:05PM -0400, Jim Quinlan wrote: > From: Jim Quinlan <jquinlan@broadcom.com> > > Some STB chips have a special purpose reset controller named > RESCAL (reset calibration). This commit adds the control > of RESCAL as well as the ability to start and stop its > operation for PCIe HW. > > Signed-off-by: Jim Quinlan <jquinlan@broadcom.com> > --- > drivers/pci/controller/pcie-brcmstb.c | 81 ++++++++++++++++++++++++++- > 1 file changed, 80 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c > index 2c470104ba38..0787e8f6f7e5 100644 > --- a/drivers/pci/controller/pcie-brcmstb.c > +++ b/drivers/pci/controller/pcie-brcmstb.c [...] > @@ -1100,6 +1164,21 @@ static int brcm_pcie_probe(struct platform_device *pdev) > dev_err(&pdev->dev, "could not enable clock\n"); > return ret; > } > + pcie->rescal = devm_reset_control_get_shared(&pdev->dev, "rescal"); > + if (IS_ERR(pcie->rescal)) { > + if (PTR_ERR(pcie->rescal) == -EPROBE_DEFER) > + return -EPROBE_DEFER; > + pcie->rescal = NULL; This is effectively an optional reset control, so it is better to use: ↵ pcie->rescal = devm_reset_control_get_optional_shared(&pdev->dev, "rescal");↵ if (IS_ERR(pcie->rescal)) return PTR_ERR(pcie->rescal); > + } else { > + ret = reset_control_deassert(pcie->rescal); > + if (ret) > + dev_err(&pdev->dev, "failed to deassert 'rescal'\n"); > + } reset_control_* can handle rstc == NULL parameters for optional reset controls, so this can be done unconditionally: ret = reset_control_deassert(pcie->rescal);↵ if (ret)↵ dev_err(&pdev->dev, "failed to deassert 'rescal'\n");↵ Is rescal handled by the reset-brcmstb-rescal driver? Since that only implements the .reset op, I would expect reset_control_reset() here. Otherwise this looks like it'd be missing a reset_control_assert in remove. regards Philipp
On Wed, May 20, 2020 at 3:27 AM Philipp Zabel <pza@pengutronix.de> wrote: > > Hi Jim, > > On Tue, May 19, 2020 at 04:34:05PM -0400, Jim Quinlan wrote: > > From: Jim Quinlan <jquinlan@broadcom.com> > > > > Some STB chips have a special purpose reset controller named > > RESCAL (reset calibration). This commit adds the control > > of RESCAL as well as the ability to start and stop its > > operation for PCIe HW. > > > > Signed-off-by: Jim Quinlan <jquinlan@broadcom.com> > > --- > > drivers/pci/controller/pcie-brcmstb.c | 81 ++++++++++++++++++++++++++- > > 1 file changed, 80 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c > > index 2c470104ba38..0787e8f6f7e5 100644 > > --- a/drivers/pci/controller/pcie-brcmstb.c > > +++ b/drivers/pci/controller/pcie-brcmstb.c > [...] > > @@ -1100,6 +1164,21 @@ static int brcm_pcie_probe(struct platform_device *pdev) > > dev_err(&pdev->dev, "could not enable clock\n"); > > return ret; > > } > > + pcie->rescal = devm_reset_control_get_shared(&pdev->dev, "rescal"); > > + if (IS_ERR(pcie->rescal)) { > > + if (PTR_ERR(pcie->rescal) == -EPROBE_DEFER) > > + return -EPROBE_DEFER; > > + pcie->rescal = NULL; > > This is effectively an optional reset control, so it is better to use: > ↵ > pcie->rescal = devm_reset_control_get_optional_shared(&pdev->dev, > "rescal");↵ > if (IS_ERR(pcie->rescal)) > return PTR_ERR(pcie->rescal); > > > + } else { > > + ret = reset_control_deassert(pcie->rescal); > > + if (ret) > > + dev_err(&pdev->dev, "failed to deassert 'rescal'\n"); > > + } > > reset_control_* can handle rstc == NULL parameters for optional reset > controls, so this can be done unconditionally: > > ret = reset_control_deassert(pcie->rescal);↵ > if (ret)↵ > dev_err(&pdev->dev, "failed to deassert 'rescal'\n");↵ > > Is rescal handled by the reset-brcmstb-rescal driver? Since that only > implements the .reset op, I would expect reset_control_reset() here. Where exactly? "reset.h" says that "Calling reset_control_rese()t is not allowed on a shared reset control." so I'm not sure why you would want me to invoke it. > Otherwise this looks like it'd be missing a reset_control_assert in > remove. I can add this. Thanks, Jim > > regards > Philipp
On 5/21/2020 2:48 PM, Jim Quinlan wrote: > On Wed, May 20, 2020 at 3:27 AM Philipp Zabel <pza@pengutronix.de> wrote: >> >> Hi Jim, >> >> On Tue, May 19, 2020 at 04:34:05PM -0400, Jim Quinlan wrote: >>> From: Jim Quinlan <jquinlan@broadcom.com> >>> >>> Some STB chips have a special purpose reset controller named >>> RESCAL (reset calibration). This commit adds the control >>> of RESCAL as well as the ability to start and stop its >>> operation for PCIe HW. >>> >>> Signed-off-by: Jim Quinlan <jquinlan@broadcom.com> >>> --- >>> drivers/pci/controller/pcie-brcmstb.c | 81 ++++++++++++++++++++++++++- >>> 1 file changed, 80 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c >>> index 2c470104ba38..0787e8f6f7e5 100644 >>> --- a/drivers/pci/controller/pcie-brcmstb.c >>> +++ b/drivers/pci/controller/pcie-brcmstb.c >> [...] >>> @@ -1100,6 +1164,21 @@ static int brcm_pcie_probe(struct platform_device *pdev) >>> dev_err(&pdev->dev, "could not enable clock\n"); >>> return ret; >>> } >>> + pcie->rescal = devm_reset_control_get_shared(&pdev->dev, "rescal"); >>> + if (IS_ERR(pcie->rescal)) { >>> + if (PTR_ERR(pcie->rescal) == -EPROBE_DEFER) >>> + return -EPROBE_DEFER; >>> + pcie->rescal = NULL; >> >> This is effectively an optional reset control, so it is better to use: >> ↵ >> pcie->rescal = devm_reset_control_get_optional_shared(&pdev->dev, >> "rescal");↵ >> if (IS_ERR(pcie->rescal)) >> return PTR_ERR(pcie->rescal); >> >>> + } else { >>> + ret = reset_control_deassert(pcie->rescal); >>> + if (ret) >>> + dev_err(&pdev->dev, "failed to deassert 'rescal'\n"); >>> + } >> >> reset_control_* can handle rstc == NULL parameters for optional reset >> controls, so this can be done unconditionally: >> >> ret = reset_control_deassert(pcie->rescal);↵ >> if (ret)↵ >> dev_err(&pdev->dev, "failed to deassert 'rescal'\n");↵ >> >> Is rescal handled by the reset-brcmstb-rescal driver? Since that only >> implements the .reset op, I would expect reset_control_reset() here. > Where exactly? "reset.h" says that "Calling reset_control_rese()t is > not allowed on a shared reset control." so I'm not sure why you would > want me to invoke it. Yes this is handled by drivers/reset/reset-brcmstb-rescal.c which only implements a .reset() callback, what would be the appropriate API usage here given that this is a shared reset between AHCI and PCIe, should drivers/reset/reset-brcmstb-rescal.c not implement a .reset() callback and .assert() callback instead? >> Otherwise this looks like it'd be missing a reset_control_assert in >> remove. > I can add this. > Thanks, > Jim >> >> regards >> Philipp
diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c index 2c470104ba38..0787e8f6f7e5 100644 --- a/drivers/pci/controller/pcie-brcmstb.c +++ b/drivers/pci/controller/pcie-brcmstb.c @@ -23,6 +23,7 @@ #include <linux/of_platform.h> #include <linux/pci.h> #include <linux/printk.h> +#include <linux/reset.h> #include <linux/sizes.h> #include <linux/slab.h> #include <linux/string.h> @@ -152,7 +153,17 @@ #define SSC_STATUS_SSC_MASK 0x400 #define SSC_STATUS_PLL_LOCK_MASK 0x800 -#define IDX_ADDR(pcie) \ +/* Rescal registers */ +#define PCIE_DVT_PMU_PCIE_PHY_CTRL 0xc700 +#define PCIE_DVT_PMU_PCIE_PHY_CTRL_DAST_NFLDS 0x3 +#define PCIE_DVT_PMU_PCIE_PHY_CTRL_DAST_DIG_RESET_MASK 0x4 +#define PCIE_DVT_PMU_PCIE_PHY_CTRL_DAST_DIG_RESET_SHIFT 0x2 +#define PCIE_DVT_PMU_PCIE_PHY_CTRL_DAST_RESET_MASK 0x2 +#define PCIE_DVT_PMU_PCIE_PHY_CTRL_DAST_RESET_SHIFT 0x1 +#define PCIE_DVT_PMU_PCIE_PHY_CTRL_DAST_PWRDN_MASK 0x1 +#define PCIE_DVT_PMU_PCIE_PHY_CTRL_DAST_PWRDN_SHIFT 0x0 + +#define IDX_ADDR(pcie) \ (pcie->reg_offsets[EXT_CFG_INDEX]) #define DATA_ADDR(pcie) \ (pcie->reg_offsets[EXT_CFG_DATA]) @@ -242,6 +253,7 @@ struct brcm_pcie { const int *reg_offsets; const int *reg_field_info; enum pcie_type type; + struct reset_control *rescal; }; /* @@ -957,6 +969,47 @@ static void brcm_pcie_enter_l23(struct brcm_pcie *pcie) dev_err(pcie->dev, "failed to enter low-power link state\n"); } +static int brcm_phy_cntl(struct brcm_pcie *pcie, const int start) +{ + static const u32 shifts[PCIE_DVT_PMU_PCIE_PHY_CTRL_DAST_NFLDS] = { + PCIE_DVT_PMU_PCIE_PHY_CTRL_DAST_PWRDN_SHIFT, + PCIE_DVT_PMU_PCIE_PHY_CTRL_DAST_RESET_SHIFT, + PCIE_DVT_PMU_PCIE_PHY_CTRL_DAST_DIG_RESET_SHIFT,}; + static const u32 masks[PCIE_DVT_PMU_PCIE_PHY_CTRL_DAST_NFLDS] = { + PCIE_DVT_PMU_PCIE_PHY_CTRL_DAST_PWRDN_MASK, + PCIE_DVT_PMU_PCIE_PHY_CTRL_DAST_RESET_MASK, + PCIE_DVT_PMU_PCIE_PHY_CTRL_DAST_DIG_RESET_MASK,}; + const int beg = start ? 0 : PCIE_DVT_PMU_PCIE_PHY_CTRL_DAST_NFLDS - 1; + const int end = start ? PCIE_DVT_PMU_PCIE_PHY_CTRL_DAST_NFLDS : -1; + u32 tmp, combined_mask = 0; + u32 val = !!start; + void __iomem *base = pcie->base; + int i; + + for (i = beg; i != end; start ? i++ : i--) { + tmp = readl(base + PCIE_DVT_PMU_PCIE_PHY_CTRL); + tmp = (tmp & ~masks[i]) | ((val << shifts[i]) & masks[i]); + writel(tmp, base + PCIE_DVT_PMU_PCIE_PHY_CTRL); + usleep_range(50, 200); + combined_mask |= masks[i]; + } + + tmp = readl(base + PCIE_DVT_PMU_PCIE_PHY_CTRL); + val = start ? combined_mask : 0; + + return (tmp & combined_mask) == val ? 0 : -EIO; +} + +static inline int brcm_phy_start(struct brcm_pcie *pcie) +{ + return pcie->rescal ? brcm_phy_cntl(pcie, 1) : 0; +} + +static inline int brcm_phy_stop(struct brcm_pcie *pcie) +{ + return pcie->rescal ? brcm_phy_cntl(pcie, 0) : 0; +} + static void brcm_pcie_turn_off(struct brcm_pcie *pcie) { void __iomem *base = pcie->base; @@ -987,6 +1040,9 @@ static int brcm_pcie_suspend(struct device *dev) int ret = 0; brcm_pcie_turn_off(pcie); + ret = brcm_phy_stop(pcie); + if (ret) + dev_err(pcie->dev, "failed to stop phy\n"); clk_disable_unprepare(pcie->clk); return ret; @@ -1002,6 +1058,12 @@ static int brcm_pcie_resume(struct device *dev) base = pcie->base; clk_prepare_enable(pcie->clk); + ret = brcm_phy_start(pcie); + if (ret) { + dev_err(pcie->dev, "failed to start phy\n"); + return ret; + } + /* Take bridge out of reset so we can access the SERDES reg */ brcm_pcie_bridge_sw_init_set(pcie, 0); @@ -1028,6 +1090,8 @@ static void __brcm_pcie_remove(struct brcm_pcie *pcie) { brcm_msi_remove(pcie); brcm_pcie_turn_off(pcie); + if (brcm_phy_stop(pcie)) + dev_err(pcie->dev, "failed to stop phy\n"); clk_disable_unprepare(pcie->clk); } @@ -1100,6 +1164,21 @@ static int brcm_pcie_probe(struct platform_device *pdev) dev_err(&pdev->dev, "could not enable clock\n"); return ret; } + pcie->rescal = devm_reset_control_get_shared(&pdev->dev, "rescal"); + if (IS_ERR(pcie->rescal)) { + if (PTR_ERR(pcie->rescal) == -EPROBE_DEFER) + return -EPROBE_DEFER; + pcie->rescal = NULL; + } else { + ret = reset_control_deassert(pcie->rescal); + if (ret) + dev_err(&pdev->dev, "failed to deassert 'rescal'\n"); + } + ret = brcm_phy_start(pcie); + if (ret) { + dev_err(pcie->dev, "failed to start phy\n"); + return ret; + } ret = brcm_pcie_setup(pcie); if (ret)