[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CANCKTBsm6o9yaSenj-wft+n0uX-HNRjpJjkZaQcn4t474fXtow@mail.gmail.com>
Date: Fri, 31 Jan 2025 11:10:32 -0500
From: Jim Quinlan <jim2101024@...il.com>
To: Stanimir Varbanov <svarbanov@...e.de>
Cc: linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-rpi-kernel@...ts.infradead.org,
linux-pci@...r.kernel.org,
Broadcom internal kernel review list <bcm-kernel-feedback-list@...adcom.com>,
Thomas Gleixner <tglx@...utronix.de>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
Florian Fainelli <florian.fainelli@...adcom.com>, Nicolas Saenz Julienne <nsaenz@...nel.org>,
Bjorn Helgaas <bhelgaas@...gle.com>, Lorenzo Pieralisi <lpieralisi@...nel.org>, kw@...ux.com,
Philipp Zabel <p.zabel@...gutronix.de>, Andrea della Porta <andrea.porta@...e.com>,
Phil Elwell <phil@...pberrypi.com>, Jonathan Bell <jonathan@...pberrypi.com>,
Dave Stevenson <dave.stevenson@...pberrypi.com>
Subject: Re: [PATCH v5 -next 04/11] PCI: brcmstb: Reuse config structure
On Mon, Jan 20, 2025 at 8:01 AM Stanimir Varbanov <svarbanov@...e.de> wrote:
>
> Instead of copying fields from pcie_cfg_data structure to
> brcm_pcie reference it directly.
>
> Signed-off-by: Stanimir Varbanov <svarbanov@...e.de>
> Reviewed-by: Florian Fainelil <florian.fainelli@...adcom.com>
> ---
> 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@...adcom.com>
> --
> 2.47.0
>
Powered by blists - more mailing lists