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.
> 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
> >
>
Regards
Jonathan
