| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <a2f6ab00-7b51-483e-ad10-0ea7ef9bfd90@suse.de>
Date: Fri, 21 Feb 2025 18:44:07 +0200
From: Stanimir Varbanov <svarbanov@...e.de>
To: Jim Quinlan <jim2101024@...il.com>, Stanimir Varbanov
<svarbanov@...e.de>, Krzysztof Wilczyński <kw@...ux.com>
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
Hi Jim,
On 2/21/25 5:36 PM, Jim Quinlan wrote:
> On Fri, Jan 31, 2025 at 11:10 AM Jim Quinlan <jim2101024@...il.com> wrote:
>>
>> 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>
>
> 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?
Ah, it is my fault. Thanks for spotting this. This must have happened
when moving this patch earlier in the series.
Krzystof,
I could send a new version of the series or the other option could be to
rework those two patches in controller/brcmstb?
I will post later the fixes here if you choose the second option.
~Stan
Powered by blists - more mailing lists