Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Forward-port downstream pcie-brcmstb patches to 6.14 #6675

Open
wants to merge 9 commits into
base: rpi-6.14.y
Choose a base branch
from

Conversation

P33M
Copy link
Contributor

@P33M P33M commented Feb 18, 2025

This applies on top of the upstream bcm2712-aware pcie-brcmstb driver and restores downstream bugfixes and compatibility features.

The intent is to keep all the conflated 2712/platform-specific adjustments relatively self-contained, hopefully avoiding merge conflicts until they land upstream.

@popcornmix
Copy link
Collaborator

This booted for me, with working ssd/m2hat and ethernet.

Copy link
Contributor

@6by9 6by9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seeing as much of this should be destined for upstream, I'm being picky.

And welcome to the lovely world of dt-bindings.

the time between internal release of fundamental reset and
the deassertion of the external PERST# pin. This has the
effect of increasing the Tperst_clk phase of link init.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Describe the units and range, eg

    $ref: /schemas/types.yaml#/definitions/uint32
    maximum: 5000

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strangely, the bindings checker complains if I define this. But not for uint8 (?)

brcm,fifo-qos-map:
description: 2712 only. Each nibble assigns all per-TC FIFOs an AXI
priority based on fullness (backpressure signalling).
Mutually exclusive with vdm-qos-map.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Units and range.

Do we have to encode it within DT, or could it be an array of N values?

Is it mandatory to have one or other of these, or is neither acceptable?

The bit you'll really love is that commenting "2712 only" doesn't wash in bindings. YAML has lots of lovely syntax to denote which compatibles support which properties.
eg https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml#L95-L126

make dt_binding_check checks whether your bindings are valid.
make CHECK_DTBS=y broadcom/bcm2712-rpi-5-b.dts checks whether your dtb file complies with the requirements of the binding.
(Add the relevant environment vars if cross-compiling).

Copy link
Contributor Author

@P33M P33M Mar 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where it gets fun: downstream uses nibbles. Would the upstream preference be a u32 array? Bit wasteful to encode 4 bits in 32.

Also, the useful combinations of these properties are either nothing, or "brcm,fifo-qos-map" or "brcm,vdm-qos-map". Can optional properties be specified as mutually exclusive?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have expected upstream to want an array of u8 or u32, but the only way of finding out would be to ask them.
Wasting <1kB isn't really worth worrying about.

You can convert the DT config directly back into the relevant nibbles for the hardware during probe, so it only throws a small amount extra on the stack during probe.

Yes I believe you can encode mutually exclusion on optional properties. Don't ask me for the exact runes at the moment though.

description: 2712 only. Each nibble assigns each per-TC FIFO a base AXI
priority with elevation depending on Vendor Messages from the EP -
specifically, RP1.
Mutually exclusive with fifo-qos-map.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

P33M added 4 commits March 3, 2025 16:55
These chips use a UBUS-AXI bridge component that has configurable
timeout and error response handling.

Suppress AXI error responses to CPU requests, otherwise these are fatal
if they reach the ARM cluster, and set reasonably large timeouts for
both Mem and Cfg requests.

Signed-off-by: Jonathan Bell <jonathan@raspberrypi.com>
In commit b478e16 ("PCI/ASPM: Consolidate link state defines")
PCIE_LINK_STATE_L1 and PCIE_LINK_STATE_L0s grew some bits for more
granular control of ASPM.

This broke the aspm-no-l0s override, instead disabling link ASPM
completely if this DT property was specified.

Specify the field bits in the driver.

Fixes: caab002 ("PCI: brcmstb: Disable L0s component of ASPM if requested")
Fixes: 0693b42 ("PCI: brcmstb: Split post-link up initialization to brcm_pcie_start_link()")
Signed-off-by: Jonathan Bell <jonathan@raspberrypi.com>
It appears that bits in the Root Control Register are reset with
perst_n, which means the PCI layer's call to enable CRS prior to
adding/scanning the bus has no effect. Open-code the enable in
brcm_pcie_start_link as a workaround.

Without CRS visibility, configuration reads issued by the CPU don't
retire if the endpoint returns a CRS response - the RC will poll until a
(large) timeout is reached. This means the core can stall for a long
time during boot.

Signed-off-by: Jonathan Bell <jonathan@raspberrypi.com>
The PHY MDIO register map is different on BCM2712, and as the PHY input
clock is 54MHz not 100MHz, enabling refclk SSC is both broken and
unfixable.

Mask out attempts to enable SSC with a controller quirk.

Signed-off-by: Jonathan Bell <jonathan@raspberrypi.com>
P33M added 5 commits March 5, 2025 10:29
There is configurable priority forwarding hardware in this variant of the
Root Complex controller. Add optional properties to configure FIFO
backpressure or Vendor-Defined Message priority forwarding.

Signed-off-by: Jonathan Bell <jonathan@raspberrypi.com>
The BCM2712 root complexes can interpret priority signalling in two
different ways, based on the incoming Traffic Class of a TLP.

The TLP TCs are assigned to separate internal request/response queues,
and assigned different AXI IDs. These queues can have outgoing AXI
transactions tagged based on:

- Static QoS values
- Dynamic QoS through internal backpressure
- Dynamic QoS with elevation based on Vendor Messages received by the RC

The VDM mechanism is of limited use due to implementation bugs, but the
implicit reordering due to separate ID assignment allows higher-priority
traffic from an EP to overtake other traffic in the RC and rest of the
system.

RP1 assigns TCs based on its internal bus managers, and internally tags
read requests to allow out-of-order completions, so these two features
operate in concert to provide priority service to e.g. MIPI camera or
display traffic.

Signed-off-by: Jonathan Bell <jonathan@raspberrypi.com>
Some platforms may require an extended time with refclk active before
PERST# is released. Add a property to let the RC driver know how long to
wait.

Signed-off-by: Jonathan Bell <jonathan@raspberrypi.com>
Some endpoints need longer than the minimum Tperst_clk time of 100us
that the PCIe specification allows for, as they may need to sequence
internal resets off the stable output of internal PLLs prior to removal
of fundamental reset. PCIe switches are an especially bad case, in some
cases requiring up to 100 milliseconds for stable downstream link
behaviour.

Parse the DT property brcm,tperst-clk-ms and use this to hold PERST# low
during brcm_pcie_start_link().

The BRCM RC typically outputs 200us of stable refclk before deasserting
PERST#. By masking/forcing the output signal while deasserting the
internal reset, the effect is to extend the length of time that the
refclk is active and stable before PERST# is released.

The TX lanes will enter the Polling state before PERST# is released, but
this appears to be harmless.

Signed-off-by: Jonathan Bell <jonathan@raspberrypi.com>
pcie1 should use the FIFO threshold property as the RC should not
pay attention to Vendor Messages from incompatible endpoint hardware.

Also drop the downstream MPS property, it's no longer needed.

Signed-off-by: Jonathan Bell <jonathan@raspberrypi.com>
@P33M P33M force-pushed the pcie-6.14-bugfixes_1 branch from 1bb63cb to 837e80a Compare March 5, 2025 10:56
@P33M P33M requested a review from 6by9 March 5, 2025 10:57
@6by9
Copy link
Contributor

6by9 commented Mar 5, 2025

Minor heads up that there's another pcie-brcmstb patch set just queued up for merging
https://lore.kernel.org/linux-arm-kernel/20250214173944.47506-1-james.quinlan@broadcom.com/

@P33M
Copy link
Contributor Author

P33M commented Mar 5, 2025

https://lore.kernel.org/linux-arm-kernel/20250214173944.47506-2-james.quinlan@broadcom.com/ obsoletes commit 5ae7f04 but that's a problem for future us.

@6by9
Copy link
Contributor

6by9 commented Mar 5, 2025

https://lore.kernel.org/linux-arm-kernel/20250214173944.47506-2-james.quinlan@broadcom.com/ obsoletes commit 5ae7f04 but that's a problem for future us.

Ish.
These appear to have been accepted, so will be in 6.15/6.16. Whilst we're working on this already, it'd be better to backport those to 6.14 first and fix the problems once, rather than having to revisit it in 3 months time.

It also means that the your patches should apply upstream for when we try submitting them.

(Quickest way to apply the series to the current branch is b4 shazam 20250214173944.47506-1-james.quinlan@broadcom.com/ - see b4 docs)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants