On Fri, Jan 31, 2025 at 11:10 AM Jim Quinlan <jim2101024@xxxxxxxxx> wrote:
>
> On Mon, Jan 20, 2025 at 8:01 AM Stanimir Varbanov <svarbanov@xxxxxxx> wrote:
> >
> > Instead of copying fields from pcie_cfg_data structure to
> > brcm_pcie reference it directly.
> >
> > Signed-off-by: Stanimir Varbanov <svarbanov@xxxxxxx>
> > Reviewed-by: Florian Fainelil <florian.fainelli@xxxxxxxxxxxx>
> > ---
> > v4 -> v5:
> > - No changes.
> >
> > drivers/pci/controller/pcie-brcmstb.c | 70 ++++++++++++---------------
> > 1 file changed, 31 insertions(+), 39 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > index e733a27dc8df..48b2747d8c98 100644
> > --- a/drivers/pci/controller/pcie-brcmstb.c
> > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > @@ -191,11 +191,11 @@
> > #define SSC_STATUS_PLL_LOCK_MASK 0x800
> > #define PCIE_BRCM_MAX_MEMC 3
> >
> > -#define IDX_ADDR(pcie) ((pcie)->reg_offsets[EXT_CFG_INDEX])
> > -#define DATA_ADDR(pcie) ((pcie)->reg_offsets[EXT_CFG_DATA])
> > -#define PCIE_RGR1_SW_INIT_1(pcie) ((pcie)->reg_offsets[RGR1_SW_INIT_1])
> > -#define HARD_DEBUG(pcie) ((pcie)->reg_offsets[PCIE_HARD_DEBUG])
> > -#define INTR2_CPU_BASE(pcie) ((pcie)->reg_offsets[PCIE_INTR2_CPU_BASE])
> > +#define IDX_ADDR(pcie) ((pcie)->cfg->offsets[EXT_CFG_INDEX])
> > +#define DATA_ADDR(pcie) ((pcie)->cfg->offsets[EXT_CFG_DATA])
> > +#define PCIE_RGR1_SW_INIT_1(pcie) ((pcie)->cfg->offsets[RGR1_SW_INIT_1])
> > +#define HARD_DEBUG(pcie) ((pcie)->cfg->offsets[PCIE_HARD_DEBUG])
> > +#define INTR2_CPU_BASE(pcie) ((pcie)->cfg->offsets[PCIE_INTR2_CPU_BASE])
> >
> > /* Rescal registers */
> > #define PCIE_DVT_PMU_PCIE_PHY_CTRL 0xc700
> > @@ -276,8 +276,6 @@ struct brcm_pcie {
> > int gen;
> > u64 msi_target_addr;
> > struct brcm_msi *msi;
> > - const int *reg_offsets;
> > - enum pcie_soc_base soc_base;
> > struct reset_control *rescal;
> > struct reset_control *perst_reset;
> > struct reset_control *bridge_reset;
> > @@ -285,17 +283,14 @@ struct brcm_pcie {
> > int num_memc;
> > u64 memc_size[PCIE_BRCM_MAX_MEMC];
> > u32 hw_rev;
> > - 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;
> > - u8 num_inbound_wins;
> > + const struct pcie_cfg_data *cfg;
> > };
> >
> > static inline bool is_bmips(const struct brcm_pcie *pcie)
> > {
> > - return pcie->soc_base == BCM7435 || pcie->soc_base == BCM7425;
> > + return pcie->cfg->soc_base == BCM7435 || pcie->cfg->soc_base == BCM7425;
> > }
> >
> > /*
> > @@ -855,7 +850,7 @@ static int brcm_pcie_get_inbound_wins(struct brcm_pcie *pcie,
> > * security considerations, and is not implemented in our modern
> > * SoCs.
> > */
> > - if (pcie->soc_base != BCM7712)
> > + if (pcie->cfg->soc_base != BCM7712)
> > add_inbound_win(b++, &n, 0, 0, 0);
> >
> > resource_list_for_each_entry(entry, &bridge->dma_ranges) {
> > @@ -872,10 +867,10 @@ static int brcm_pcie_get_inbound_wins(struct brcm_pcie *pcie,
> > * That being said, each BARs size must still be a power of
> > * two.
> > */
> > - if (pcie->soc_base == BCM7712)
> > + if (pcie->cfg->soc_base == BCM7712)
> > add_inbound_win(b++, &n, size, cpu_start, pcie_start);
> >
> > - if (n > pcie->num_inbound_wins)
> > + if (n > pcie->cfg->num_inbound_wins)
> > break;
> > }
> >
> > @@ -889,7 +884,7 @@ static int brcm_pcie_get_inbound_wins(struct brcm_pcie *pcie,
> > * that enables multiple memory controllers. As such, it can return
> > * now w/o doing special configuration.
> > */
> > - if (pcie->soc_base == BCM7712)
> > + if (pcie->cfg->soc_base == BCM7712)
> > return n;
> >
> > ret = of_property_read_variable_u64_array(pcie->np, "brcm,scb-sizes", pcie->memc_size, 1,
> > @@ -1012,7 +1007,7 @@ static void set_inbound_win_registers(struct brcm_pcie *pcie,
> > * 7712:
> > * All of their BARs need to be set.
> > */
> > - if (pcie->soc_base == BCM7712) {
> > + if (pcie->cfg->soc_base == BCM7712) {
> > /* BUS remap register settings */
> > reg_offset = brcm_ubus_reg_offset(i);
> > tmp = lower_32_bits(cpu_addr) & ~0xfff;
> > @@ -1036,15 +1031,15 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
> > int memc, ret;
> >
> > /* Reset the bridge */
> > - ret = pcie->bridge_sw_init_set(pcie, 1);
> > + ret = pcie->cfg->bridge_sw_init_set(pcie, 1);
> > if (ret)
> > return ret;
> >
> > /* Ensure that PERST# is asserted; some bootloaders may deassert it. */
> > - if (pcie->soc_base == BCM2711) {
> > - ret = pcie->perst_set(pcie, 1);
> > + if (pcie->cfg->soc_base == BCM2711) {
> > + ret = pcie->cfg->perst_set(pcie, 1);
> > if (ret) {
> > - pcie->bridge_sw_init_set(pcie, 0);
> > + pcie->cfg->bridge_sw_init_set(pcie, 0);
> > return ret;
> > }
> > }
> > @@ -1052,7 +1047,7 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
> > usleep_range(100, 200);
> >
> > /* Take the bridge out of reset */
> > - ret = pcie->bridge_sw_init_set(pcie, 0);
> > + ret = pcie->cfg->bridge_sw_init_set(pcie, 0);
> > if (ret)
> > return ret;
> >
> > @@ -1072,9 +1067,9 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
> > */
> > if (is_bmips(pcie))
> > burst = 0x1; /* 256 bytes */
> > - else if (pcie->soc_base == BCM2711)
> > + else if (pcie->cfg->soc_base == BCM2711)
> > burst = 0x0; /* 128 bytes */
> > - else if (pcie->soc_base == BCM7278)
> > + else if (pcie->cfg->soc_base == BCM7278)
> > burst = 0x3; /* 512 bytes */
> > else
> > burst = 0x2; /* 512 bytes */
> > @@ -1199,7 +1194,7 @@ static void brcm_extend_rbus_timeout(struct brcm_pcie *pcie)
> > u32 timeout_us = 4000000; /* 4 seconds, our setting for L1SS */
> >
> > /* 7712 does not have this (RGR1) timer */
> > - if (pcie->soc_base == BCM7712)
> > + if (pcie->cfg->soc_base == BCM7712)
> > return;
> >
> > /* Each unit in timeout register is 1/216,000,000 seconds */
> > @@ -1277,7 +1272,7 @@ static int brcm_pcie_start_link(struct brcm_pcie *pcie)
> > int ret, i;
> >
> > /* Unassert the fundamental reset */
> > - ret = pcie->perst_set(pcie, 0);
> > + ret = pcie->cfg->perst_set(pcie, 0);
> > if (ret)
> > return ret;
> >
> > @@ -1463,12 +1458,12 @@ static int brcm_phy_cntl(struct brcm_pcie *pcie, const int start)
> >
> > static inline int brcm_phy_start(struct brcm_pcie *pcie)
> > {
> > - return pcie->has_phy ? brcm_phy_cntl(pcie, 1) : 0;
> > + return pcie->cfg->has_phy ? brcm_phy_cntl(pcie, 1) : 0;
> > }
> >
> > static inline int brcm_phy_stop(struct brcm_pcie *pcie)
> > {
> > - return pcie->has_phy ? brcm_phy_cntl(pcie, 0) : 0;
> > + return pcie->cfg->has_phy ? brcm_phy_cntl(pcie, 0) : 0;
> > }
> >
> > static int brcm_pcie_turn_off(struct brcm_pcie *pcie)
> > @@ -1479,7 +1474,7 @@ static int brcm_pcie_turn_off(struct brcm_pcie *pcie)
> > if (brcm_pcie_link_up(pcie))
> > brcm_pcie_enter_l23(pcie);
> > /* Assert fundamental reset */
> > - ret = pcie->perst_set(pcie, 1);
> > + ret = pcie->cfg->perst_set(pcie, 1);
> > if (ret)
> > return ret;
> >
> > @@ -1582,7 +1577,7 @@ static int brcm_pcie_resume_noirq(struct device *dev)
> > goto err_reset;
> >
> > /* Take bridge out of reset so we can access the SERDES reg */
> > - pcie->bridge_sw_init_set(pcie, 0);
> > + pcie->cfg->bridge_sw_init_set(pcie, 0);
> >
> > /* SERDES_IDDQ = 0 */
> > tmp = readl(base + HARD_DEBUG(pcie));
> > @@ -1803,12 +1798,7 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> > pcie = pci_host_bridge_priv(bridge);
> > pcie->dev = &pdev->dev;
> > pcie->np = np;
> > - pcie->reg_offsets = data->offsets;
> > - pcie->soc_base = data->soc_base;
> > - pcie->perst_set = data->perst_set;
> > - pcie->bridge_sw_init_set = data->bridge_sw_init_set;
> > - pcie->has_phy = data->has_phy;
> > - pcie->num_inbound_wins = data->num_inbound_wins;
> > + pcie->cfg = data;
> >
> > pcie->base = devm_platform_ioremap_resource(pdev, 0);
> > if (IS_ERR(pcie->base))
> > @@ -1843,7 +1833,7 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> > if (ret)
> > return dev_err_probe(&pdev->dev, ret, "could not enable clock\n");
> >
> > - pcie->bridge_sw_init_set(pcie, 0);
> > + pcie->cfg->bridge_sw_init_set(pcie, 0);
> >
> > if (pcie->swinit_reset) {
> > ret = reset_control_assert(pcie->swinit_reset);
> > @@ -1882,7 +1872,8 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> > goto fail;
> >
> > pcie->hw_rev = readl(pcie->base + PCIE_MISC_REVISION);
> > - if (pcie->soc_base == BCM4908 && pcie->hw_rev >= BRCM_PCIE_HW_REV_3_20) {
> > + if (pcie->cfg->soc_base == BCM4908 &&
> > + pcie->hw_rev >= BRCM_PCIE_HW_REV_3_20) {
> > dev_err(pcie->dev, "hardware revision with unsupported PERST# setup\n");
> > ret = -ENODEV;
> > goto fail;
> > @@ -1897,7 +1888,8 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> > }
> > }
> >
> > - bridge->ops = pcie->soc_base == BCM7425 ? &brcm7425_pcie_ops : &brcm_pcie_ops;
> > + bridge->ops = pcie->cfg->soc_base == BCM7425 ?
> > + &brcm7425_pcie_ops : &brcm_pcie_ops;
> > bridge->sysdata = pcie;
> >
> > platform_set_drvdata(pdev, pcie);
>
> Reviewed-by: Jim Quinlan <james.quinlan@xxxxxxxxxxxx>
Hi Stan,
Sorry for the late notice but I get a compilation error on this commit:
drivers/pci/controller/pcie-brcmstb.c: In function 'brcm_pcie_turn_off':
drivers/pci/controller/pcie-brcmstb.c:1492:14: error: 'struct
brcm_pcie' has no member named 'bridge_sw_init_set'; did you mean
'bridge_reset'?
ret = pcie->bridge_sw_init_set(pcie, 1);
^~~~~~~~~~~~~~~~~~
bridge_reset
make[5]: *** [scripts/Makefile.build:194:
drivers/pci/controller/pcie-brcmstb.o] Error 1
It appears to be fixed with the subsequent commit "PCI: brcmstb: Add
bcm2712 support".
Can you please look into this and see if you get the same results?
Regards,
Jim Quinlan
Broadcom STB/CM
> > --
> > 2.47.0
> >
