| Message ID | 20201130083223.32594-3-zajec5@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | brcmstb: initial work on BCM4908 | expand |
On 11/30/2020 12:32 AM, Rafał Miłecki wrote: > From: Rafał Miłecki <rafal@milecki.pl> > > BCM4908 uses external MISC block for controlling PERST# signal. Use it > as a reset controller. > > Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > --- > V2: Reorder BCM4908 in the enum pcie_type > Use devm_reset_control_get_optional_exclusive() > Don't move hw_rev read up in the code > --- > drivers/pci/controller/Kconfig | 2 +- > drivers/pci/controller/pcie-brcmstb.c | 32 +++++++++++++++++++++++++++ > 2 files changed, 33 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig > index 64e2f5e379aa..d44c70bb88f6 100644 > --- a/drivers/pci/controller/Kconfig > +++ b/drivers/pci/controller/Kconfig > @@ -273,7 +273,7 @@ config VMD > > config PCIE_BRCMSTB > tristate "Broadcom Brcmstb PCIe host controller" > - depends on ARCH_BRCMSTB || ARCH_BCM2835 || COMPILE_TEST > + depends on ARCH_BRCMSTB || ARCH_BCM2835 || ARCH_BCM4908 || COMPILE_TEST > depends on OF > depends on PCI_MSI_IRQ_DOMAIN > default ARCH_BRCMSTB > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c > index 9c3d2982248d..98536cf3af58 100644 > --- a/drivers/pci/controller/pcie-brcmstb.c > +++ b/drivers/pci/controller/pcie-brcmstb.c > @@ -96,6 +96,7 @@ > > #define PCIE_MISC_REVISION 0x406c > #define BRCM_PCIE_HW_REV_33 0x0303 > +#define BRCM_PCIE_HW_REV_3_20 0x0320 > > #define PCIE_MISC_CPU_2_PCIE_MEM_WIN0_BASE_LIMIT 0x4070 > #define PCIE_MISC_CPU_2_PCIE_MEM_WIN0_BASE_LIMIT_LIMIT_MASK 0xfff00000 > @@ -190,6 +191,7 @@ > struct brcm_pcie; > static inline void brcm_pcie_bridge_sw_init_set_7278(struct brcm_pcie *pcie, u32 val); > static inline void brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie *pcie, u32 val); > +static inline void brcm_pcie_perst_set_4908(struct brcm_pcie *pcie, u32 val); > static inline void brcm_pcie_perst_set_7278(struct brcm_pcie *pcie, u32 val); > static inline void brcm_pcie_perst_set_generic(struct brcm_pcie *pcie, u32 val); > > @@ -206,6 +208,7 @@ enum { > > enum pcie_type { > GENERIC, > + BCM4908, > BCM7278, > BCM2711, > }; > @@ -230,6 +233,13 @@ static const struct pcie_cfg_data generic_cfg = { > .bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic, > }; > > +static const struct pcie_cfg_data bcm4908_cfg = { > + .offsets = pcie_offsets, > + .type = BCM4908, > + .perst_set = brcm_pcie_perst_set_4908, > + .bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic, > +}; > + > static const int pcie_offset_bcm7278[] = { > [RGR1_SW_INIT_1] = 0xc010, > [EXT_CFG_INDEX] = 0x9000, > @@ -282,6 +292,7 @@ struct brcm_pcie { > const int *reg_offsets; > enum pcie_type type; > struct reset_control *rescal; > + struct reset_control *perst_reset; > int num_memc; > u64 memc_size[PCIE_BRCM_MAX_MEMC]; > u32 hw_rev; > @@ -747,6 +758,17 @@ static inline void brcm_pcie_bridge_sw_init_set_7278(struct brcm_pcie *pcie, u32 > writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1(pcie)); > } > > +static inline void brcm_pcie_perst_set_4908(struct brcm_pcie *pcie, u32 val) > +{ > + if (WARN_ONCE(!pcie->perst_reset, "missing PERST# reset controller\n")) > + return; > + > + if (val) > + reset_control_assert(pcie->perst_reset); > + else > + reset_control_deassert(pcie->perst_reset); This looks good to me now, just one nit, you probably do not support suspend/resume on the 4908, likely never will, but you should probably pulse the PERST# during PCIe resume, too. With that fixed: Acked-by: Florian Fainelli <f.fainelli@gmail.com> -- Florian
On 04.12.2020 19:21, Florian Fainelli wrote: > On 11/30/2020 12:32 AM, Rafał Miłecki wrote: >> From: Rafał Miłecki <rafal@milecki.pl> >> >> BCM4908 uses external MISC block for controlling PERST# signal. Use it >> as a reset controller. >> >> Signed-off-by: Rafał Miłecki <rafal@milecki.pl> >> --- >> V2: Reorder BCM4908 in the enum pcie_type >> Use devm_reset_control_get_optional_exclusive() >> Don't move hw_rev read up in the code >> --- >> drivers/pci/controller/Kconfig | 2 +- >> drivers/pci/controller/pcie-brcmstb.c | 32 +++++++++++++++++++++++++++ >> 2 files changed, 33 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig >> index 64e2f5e379aa..d44c70bb88f6 100644 >> --- a/drivers/pci/controller/Kconfig >> +++ b/drivers/pci/controller/Kconfig >> @@ -273,7 +273,7 @@ config VMD >> >> config PCIE_BRCMSTB >> tristate "Broadcom Brcmstb PCIe host controller" >> - depends on ARCH_BRCMSTB || ARCH_BCM2835 || COMPILE_TEST >> + depends on ARCH_BRCMSTB || ARCH_BCM2835 || ARCH_BCM4908 || COMPILE_TEST >> depends on OF >> depends on PCI_MSI_IRQ_DOMAIN >> default ARCH_BRCMSTB >> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c >> index 9c3d2982248d..98536cf3af58 100644 >> --- a/drivers/pci/controller/pcie-brcmstb.c >> +++ b/drivers/pci/controller/pcie-brcmstb.c >> @@ -96,6 +96,7 @@ >> >> #define PCIE_MISC_REVISION 0x406c >> #define BRCM_PCIE_HW_REV_33 0x0303 >> +#define BRCM_PCIE_HW_REV_3_20 0x0320 >> >> #define PCIE_MISC_CPU_2_PCIE_MEM_WIN0_BASE_LIMIT 0x4070 >> #define PCIE_MISC_CPU_2_PCIE_MEM_WIN0_BASE_LIMIT_LIMIT_MASK 0xfff00000 >> @@ -190,6 +191,7 @@ >> struct brcm_pcie; >> static inline void brcm_pcie_bridge_sw_init_set_7278(struct brcm_pcie *pcie, u32 val); >> static inline void brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie *pcie, u32 val); >> +static inline void brcm_pcie_perst_set_4908(struct brcm_pcie *pcie, u32 val); >> static inline void brcm_pcie_perst_set_7278(struct brcm_pcie *pcie, u32 val); >> static inline void brcm_pcie_perst_set_generic(struct brcm_pcie *pcie, u32 val); >> >> @@ -206,6 +208,7 @@ enum { >> >> enum pcie_type { >> GENERIC, >> + BCM4908, >> BCM7278, >> BCM2711, >> }; >> @@ -230,6 +233,13 @@ static const struct pcie_cfg_data generic_cfg = { >> .bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic, >> }; >> >> +static const struct pcie_cfg_data bcm4908_cfg = { >> + .offsets = pcie_offsets, >> + .type = BCM4908, >> + .perst_set = brcm_pcie_perst_set_4908, >> + .bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic, >> +}; >> + >> static const int pcie_offset_bcm7278[] = { >> [RGR1_SW_INIT_1] = 0xc010, >> [EXT_CFG_INDEX] = 0x9000, >> @@ -282,6 +292,7 @@ struct brcm_pcie { >> const int *reg_offsets; >> enum pcie_type type; >> struct reset_control *rescal; >> + struct reset_control *perst_reset; >> int num_memc; >> u64 memc_size[PCIE_BRCM_MAX_MEMC]; >> u32 hw_rev; >> @@ -747,6 +758,17 @@ static inline void brcm_pcie_bridge_sw_init_set_7278(struct brcm_pcie *pcie, u32 >> writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1(pcie)); >> } >> >> +static inline void brcm_pcie_perst_set_4908(struct brcm_pcie *pcie, u32 val) >> +{ >> + if (WARN_ONCE(!pcie->perst_reset, "missing PERST# reset controller\n")) >> + return; >> + >> + if (val) >> + reset_control_assert(pcie->perst_reset); >> + else >> + reset_control_deassert(pcie->perst_reset); > > This looks good to me now, just one nit, you probably do not support > suspend/resume on the 4908, likely never will, but you should probably > pulse the PERST# during PCIe resume, too. With that fixed: > > Acked-by: Florian Fainelli <f.fainelli@gmail.com> Driver already does that. Suspend forward trace: brcm_pcie_suspend() brcm_pcie_turn_off() pcie->perst_set(pcie, 1) Resume forward trace: brcm_pcie_resume() brcm_pcie_setup() pcie->perst_set(pcie, 0) Correct me if I'm wrong please.
On 12/10/2020 12:46 AM, Rafał Miłecki wrote: >> This looks good to me now, just one nit, you probably do not support >> suspend/resume on the 4908, likely never will, but you should probably >> pulse the PERST# during PCIe resume, too. With that fixed: >> >> Acked-by: Florian Fainelli <f.fainelli@gmail.com> > > Driver already does that. > > Suspend forward trace: > brcm_pcie_suspend() > brcm_pcie_turn_off() > pcie->perst_set(pcie, 1) > > Resume forward trace: > brcm_pcie_resume() > brcm_pcie_setup() > pcie->perst_set(pcie, 0) > > Correct me if I'm wrong please. Nope, it's all good, my memory was incorrectly serving me, since perst_set() has been moved to a callback you have it called indeed. Acked-by: Florian Fainelli <f.fainelli@gmail.com> Thanks! -- Florian
diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig index 64e2f5e379aa..d44c70bb88f6 100644 --- a/drivers/pci/controller/Kconfig +++ b/drivers/pci/controller/Kconfig @@ -273,7 +273,7 @@ config VMD config PCIE_BRCMSTB tristate "Broadcom Brcmstb PCIe host controller" - depends on ARCH_BRCMSTB || ARCH_BCM2835 || COMPILE_TEST + depends on ARCH_BRCMSTB || ARCH_BCM2835 || ARCH_BCM4908 || COMPILE_TEST depends on OF depends on PCI_MSI_IRQ_DOMAIN default ARCH_BRCMSTB diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c index 9c3d2982248d..98536cf3af58 100644 --- a/drivers/pci/controller/pcie-brcmstb.c +++ b/drivers/pci/controller/pcie-brcmstb.c @@ -96,6 +96,7 @@ #define PCIE_MISC_REVISION 0x406c #define BRCM_PCIE_HW_REV_33 0x0303 +#define BRCM_PCIE_HW_REV_3_20 0x0320 #define PCIE_MISC_CPU_2_PCIE_MEM_WIN0_BASE_LIMIT 0x4070 #define PCIE_MISC_CPU_2_PCIE_MEM_WIN0_BASE_LIMIT_LIMIT_MASK 0xfff00000 @@ -190,6 +191,7 @@ struct brcm_pcie; static inline void brcm_pcie_bridge_sw_init_set_7278(struct brcm_pcie *pcie, u32 val); static inline void brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie *pcie, u32 val); +static inline void brcm_pcie_perst_set_4908(struct brcm_pcie *pcie, u32 val); static inline void brcm_pcie_perst_set_7278(struct brcm_pcie *pcie, u32 val); static inline void brcm_pcie_perst_set_generic(struct brcm_pcie *pcie, u32 val); @@ -206,6 +208,7 @@ enum { enum pcie_type { GENERIC, + BCM4908, BCM7278, BCM2711, }; @@ -230,6 +233,13 @@ static const struct pcie_cfg_data generic_cfg = { .bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic, }; +static const struct pcie_cfg_data bcm4908_cfg = { + .offsets = pcie_offsets, + .type = BCM4908, + .perst_set = brcm_pcie_perst_set_4908, + .bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic, +}; + static const int pcie_offset_bcm7278[] = { [RGR1_SW_INIT_1] = 0xc010, [EXT_CFG_INDEX] = 0x9000, @@ -282,6 +292,7 @@ struct brcm_pcie { const int *reg_offsets; enum pcie_type type; struct reset_control *rescal; + struct reset_control *perst_reset; int num_memc; u64 memc_size[PCIE_BRCM_MAX_MEMC]; u32 hw_rev; @@ -747,6 +758,17 @@ static inline void brcm_pcie_bridge_sw_init_set_7278(struct brcm_pcie *pcie, u32 writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1(pcie)); } +static inline void brcm_pcie_perst_set_4908(struct brcm_pcie *pcie, u32 val) +{ + if (WARN_ONCE(!pcie->perst_reset, "missing PERST# reset controller\n")) + return; + + if (val) + reset_control_assert(pcie->perst_reset); + else + reset_control_deassert(pcie->perst_reset); +} + static inline void brcm_pcie_perst_set_7278(struct brcm_pcie *pcie, u32 val) { u32 tmp; @@ -1206,6 +1228,7 @@ static int brcm_pcie_remove(struct platform_device *pdev) static const struct of_device_id brcm_pcie_match[] = { { .compatible = "brcm,bcm2711-pcie", .data = &bcm2711_cfg }, + { .compatible = "brcm,bcm4908-pcie", .data = &bcm4908_cfg }, { .compatible = "brcm,bcm7211-pcie", .data = &generic_cfg }, { .compatible = "brcm,bcm7278-pcie", .data = &bcm7278_cfg }, { .compatible = "brcm,bcm7216-pcie", .data = &bcm7278_cfg }, @@ -1262,6 +1285,11 @@ static int brcm_pcie_probe(struct platform_device *pdev) clk_disable_unprepare(pcie->clk); return PTR_ERR(pcie->rescal); } + pcie->perst_reset = devm_reset_control_get_optional_exclusive(&pdev->dev, "perst"); + if (IS_ERR(pcie->perst_reset)) { + clk_disable_unprepare(pcie->clk); + return PTR_ERR(pcie->perst_reset); + } ret = reset_control_deassert(pcie->rescal); if (ret) @@ -1279,6 +1307,10 @@ static int brcm_pcie_probe(struct platform_device *pdev) goto fail; pcie->hw_rev = readl(pcie->base + PCIE_MISC_REVISION); + if (pcie->type == BCM4908 && pcie->hw_rev >= BRCM_PCIE_HW_REV_3_20) { + dev_err(pcie->dev, "hardware revision with unsupported PERST# setup\n"); + goto fail; + } msi_np = of_parse_phandle(pcie->np, "msi-parent", 0); if (pci_msi_enabled() && msi_np == pcie->np) {