Hi
On 12/11/24 10:54 PM, Jonathan Bell wrote:
> On Wed, 11 Dec 2024 at 19:39, James Quinlan <james.quinlan@xxxxxxxxxxxx> wrote:
>>
>> 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@xxxxxxx>
>>>>> ---
>>>>> 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.
>
> I'm the originator of this thunk - erratum is perhaps the wrong description.
> If the reference clock provided to the RC is 54MHz and not 100MHz, as
> is the case on BCM2712, then many of the L1 sub-state timers run
> slower which means state transitions are unnecessarily lengthened.
>
> This change, and the MDIO manipulation above, should be applied
> regardless of the RC instance and/or connected EP.
>
Thank you Jonathan.
I guess you are fine to take this description in the next version of the
series?
~Stan
