Re: [PATCH v4 08/10] PCI: brcmstb: Adjust PHY PLL setup to use a 54MHz input refclk

[James Quinlan <james.quinlan@broadcom.com>]

On 12/10/24 08:42, Stanimir Varbanov wrote:
> Hi Jim
>
> On 12/10/24 12:52 AM, James Quinlan wrote:
>> On 10/25/24 08:45, Stanimir Varbanov wrote:
>>> The default input reference clock for the PHY PLL is 100Mhz, except for
>>> some devices where it is 54Mhz like bcm2712C1 and bcm2712D0.
>>>
>>> To implement this adjustments introduce a new .post_setup op in
>>> pcie_cfg_data and call it at the end of brcm_pcie_setup function.
>>>
>>> The bcm2712 .post_setup callback implements the required MDIO writes that
>>> switch the PLL refclk and also change PHY PM clock period.
>>>
>>> Without this RPi5 PCIex1 is unable to enumerate endpoint devices on
>>> the expansion connector.
>>>
>>> Signed-off-by: Stanimir Varbanov <svarbanov@suse.de>
>>> ---
>>> v3 -> v4:
>>>    - Improved patch description (Florian)
>>>
>>>    drivers/pci/controller/pcie-brcmstb.c | 42 +++++++++++++++++++++++++++
>>>    1 file changed, 42 insertions(+)
>>>
>>> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/
>>> controller/pcie-brcmstb.c
>>> index d970a76aa9ef..2571dcc14560 100644
>>> --- a/drivers/pci/controller/pcie-brcmstb.c
>>> +++ b/drivers/pci/controller/pcie-brcmstb.c
>>> @@ -55,6 +55,10 @@
>>>    #define PCIE_RC_DL_MDIO_WR_DATA                0x1104
>>>    #define PCIE_RC_DL_MDIO_RD_DATA                0x1108
>>>    +#define PCIE_RC_PL_PHY_CTL_15                0x184c
>>> +#define  PCIE_RC_PL_PHY_CTL_15_DIS_PLL_PD_MASK        0x400000
>>> +#define  PCIE_RC_PL_PHY_CTL_15_PM_CLK_PERIOD_MASK    0xff
>>> +
>>>    #define PCIE_MISC_MISC_CTRL                0x4008
>>>    #define  PCIE_MISC_MISC_CTRL_PCIE_RCB_64B_MODE_MASK    0x80
>>>    #define  PCIE_MISC_MISC_CTRL_PCIE_RCB_MPS_MODE_MASK    0x400
>>> @@ -251,6 +255,7 @@ struct pcie_cfg_data {
>>>        u8 num_inbound_wins;
>>>        int (*perst_set)(struct brcm_pcie *pcie, u32 val);
>>>        int (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
>>> +    int (*post_setup)(struct brcm_pcie *pcie);
>>>    };
>>>      struct subdev_regulators {
>>> @@ -826,6 +831,36 @@ static int brcm_pcie_perst_set_generic(struct
>>> brcm_pcie *pcie, u32 val)
>>>        return 0;
>>>    }
>>>    +static int brcm_pcie_post_setup_bcm2712(struct brcm_pcie *pcie)
>>> +{
>>> +    const u16 data[] = { 0x50b9, 0xbda1, 0x0094, 0x97b4, 0x5030,
>>> 0x5030, 0x0007 };
>>> +    const u8 regs[] = { 0x16, 0x17, 0x18, 0x19, 0x1b, 0x1c, 0x1e };
>>> +    int ret, i;
>>> +    u32 tmp;
>>> +
>>> +    /* Allow a 54MHz (xosc) refclk source */
>>> +    ret = brcm_pcie_mdio_write(pcie->base, MDIO_PORT0,
>>> SET_ADDR_OFFSET, 0x1600);
>>> +    if (ret < 0)
>>> +        return ret;
>>> +
>>> +    for (i = 0; i < ARRAY_SIZE(regs); i++) {
>>> +        ret = brcm_pcie_mdio_write(pcie->base, MDIO_PORT0, regs[i],
>>> data[i]);
>>> +        if (ret < 0)
>>> +            return ret;
>>> +    }
>>> +
>>> +    usleep_range(100, 200);
>>> +
>>> +    /* Fix for L1SS errata */
>>> +    tmp = readl(pcie->base + PCIE_RC_PL_PHY_CTL_15);
>>> +    tmp &= ~PCIE_RC_PL_PHY_CTL_15_PM_CLK_PERIOD_MASK;
>>> +    /* PM clock period is 18.52ns (round down) */
>>> +    tmp |= 0x12;
>>> +    writel(tmp, pcie->base + PCIE_RC_PL_PHY_CTL_15);
>> Hi Stan,
>>
>> Can you please say more about where this errata came from?  I asked the
>> 7712 PCIe HW folks and they said that there best guess was that it was a
>> old workaround for a particular Broadcom Wifi endpoint.  Do you know its
>> origin?
> Unfortunately, I don't know the details. See the comments on previous
> series version [1]. My observation shows that MDIO writes are
> implemented in RPi platform firmware only for pcie2 (where RP1 south
> bridge is connected) but not for pcie1 expansion connector.

Well, I think my concern is more about the comment "Fix for L1SS errata" 
rather than the code.  If this is a bonafide errata it should have an 
identifier and some documentation somewhere. Declaring it to be an 
unknown errata provides little info.

Code-wise, you could use u32p_replace_bits(..., PM_CLK_PERIOD_MASK) to 
do the field value insertion.

All the above being said, I have no objection since this code is 
specific to the RPi platform.

Jim Quinlan  Broadcom STB/CM

>
> ~Stan
>
> [1] https://www.spinics.net/lists/linux-pci/msg160842.html
>