[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <365ade7c-bb2c-42ff-a2e7-9a291b1faab7@riscstar.com>
Date: Tue, 30 Sep 2025 21:40:12 -0500
From: Alex Elder <elder@...cstar.com>
To: Manivannan Sadhasivam <mani@...nel.org>
Cc: robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
lpieralisi@...nel.org, kwilczynski@...nel.org, bhelgaas@...gle.com,
vkoul@...nel.org, kishon@...nel.org, dlan@...too.org,
paul.walmsley@...ive.com, palmer@...belt.com, aou@...s.berkeley.edu,
alex@...ti.fr, p.zabel@...gutronix.de, tglx@...utronix.de,
johan+linaro@...nel.org, thippeswamy.havalige@....com, namcao@...utronix.de,
mayank.rana@....qualcomm.com, shradha.t@...sung.com, inochiama@...il.com,
quic_schintav@...cinc.com, fan.ni@...sung.com, devicetree@...r.kernel.org,
linux-phy@...ts.infradead.org, linux-pci@...r.kernel.org,
spacemit@...ts.linux.dev, linux-riscv@...ts.infradead.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/6] dt-bindings: phy: spacemit: introduce PCIe root
complex
On 9/20/25 12:55 AM, Manivannan Sadhasivam wrote:
> On Fri, Sep 19, 2025 at 03:14:05PM -0500, Alex Elder wrote:
>> On 9/15/25 3:14 AM, Manivannan Sadhasivam wrote:
>>> On Wed, Aug 13, 2025 at 01:46:57PM GMT, Alex Elder wrote:
>>>
>>> Subject should have 'pci' prefix, not 'phy'.
>>
>> OK I'll update that in the next version.
I have a new version of this code ready, but I'll delay sending
it until the end of the merge window.
>>>> Add the Device Tree binding for the PCIe root complex found on the
>>>> SpacemiT K1 SoC. This device is derived from the Synopsys Designware
>>>> PCIe IP. It supports up to three PCIe ports operating at PCIe gen 2
>>>> link speeds (5 GT/sec). One of the ports uses a combo PHY, which is
>>>> typically used to support a USB 3 port.
>>>>
>>>> Signed-off-by: Alex Elder <elder@...cstar.com>
>>>> ---
>>>> .../bindings/pci/spacemit,k1-pcie-rc.yaml | 141 ++++++++++++++++++
>>>> 1 file changed, 141 insertions(+)
>>>> create mode 100644 Documentation/devicetree/bindings/pci/spacemit,k1-pcie-rc.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/pci/spacemit,k1-pcie-rc.yaml b/Documentation/devicetree/bindings/pci/spacemit,k1-pcie-rc.yaml
>>>> new file mode 100644
>>>> index 0000000000000..6bcca2f91a6fd
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/pci/spacemit,k1-pcie-rc.yaml
>>>> @@ -0,0 +1,141 @@
. . .
>>>> + interrupts-extended:
>>>> + maxItems: 1
>>>
>>> What is the purpose of this property? Is it for MSI or INTx?
>>
>> It is for MSIs, which are translated into this interrupt.
>> I'll add a short description indicating this.
>>
>> Is there a better way to represent this?
>>
>
> For external MSI controllers, it is recommended to use 'msi-map' property as the
> client often need to pass RID, RID length and other sideband data to the MSI
> controller.
This implementation is using the DWC built-in MSI controller, not an
external controller. It implements 256 MSIs, and they're handled by
dw_handle_msi_irq(). The interrupt will be given name "msi" in the
next version and I hope that at least helps clarify things.
>
>>>> +
>>>> + spacemit,syscon-pmu:
>>>> + description:
>>>> + PHandle that refers to the APMU system controller, whose
>>>> + regmap is used in managing resets and link state.
>>>> + $ref: /schemas/types.yaml#/definitions/phandle
>>>> +
>>>> + device_type:
>>>> + const: pci
>>>> +
>>>> + max-link-speed:
>>>> + const: 2
>>>
>>> Why do you need to limit it to 5 GT/s always?
>>
>> It's what the hardware overview says is the speed
>> of the ports.
>> PCIE PortA Gen2x1
>> PCIE PortB Gen2x2
>> PCIE PortC Gen2x2
>>
>> But I think what you're asking might be "why do you
>> need to specify in DT that the link speed is limited".
>> And in that case, I realize now that it is not needed.
>> I will specify dw_pcie->max_link_speed to 2 before
>> calling dw_pcie_host_init().
>>
>
> You only need to specify this property if you want to limit the link speed to a
> certain data rate by the controller driver than what is supported by the
> hardware.
Yes, this makes sense.
> Looks like your hardware supports 5 GT/s by default for all ports. So you can
> omit this property and also no need to set 'dw_pcie->max_link_speed'. In the
> absence of this property, 'dw_pcie->max_link_speed' will be populated with the
> hardware default value, which will be 2 in your case.
You are correct, and I verified this. I won't specify the link
speed "manually," and the hardware default (which is 2) will be
used.
>> If that's not what you meant, please let me know.
>>
>>>> + num-viewport:
>>>> + const: 8
>>>> +
>>>> +required:
>>>> + - compatible
>>>> + - reg
>>>> + - clocks
>>>> + - clock-names
>>>> + - resets
>>>> + - reset-names
>>>> + - spacemit,syscon-pmu
>>>> + - "#address-cells"
>>>> + - "#size-cells"
>>>> + - device_type
>>>> + - max-link-speed
>>>
>>> Same comment as above.
>>>
>>>> + - bus-range
>>>> + - num-viewport
>>>> +
>>>> +additionalProperties: false
>>>> +
>>>> +examples:
>>>> + - |
>>>> + #include <dt-bindings/clock/spacemit,k1-syscon.h>
>>>> + pcie0: pcie@...00000 {
>>>> + compatible = "spacemit,k1-pcie-rc";
>>>> + reg = <0x0 0xca000000 0x0 0x00001000>,
>>>> + <0x0 0xca300000 0x0 0x0001ff24>,
>>>> + <0x0 0x8f000000 0x0 0x00002000>,
>>>> + <0x0 0xc0b20000 0x0 0x00001000>;
>>>> + reg-names = "dbi",
>>>> + "atu",
>>>> + "config",
>>>> + "link";
>>>> +
>>>> + ranges = <0x01000000 0x8f002000 0x0 0x8f002000 0x0 0x100000>,
>>>
>>> I/O port CPU address starts from 0.
>>
>> First, I'm not sure what this comment means.
>>
>
> Sorry, I meant to say 'PCI address' not 'CPU address'. The second element in
> your 'ranges' example corresponds to the PCI start address of the I/O port and
> it should always start from 0. Also, the encoding for both tuples are wrong. It
> should be:
>
> ranges = <0x01000000 0x0 0x00000000 0x8f002000 0x0 0x100000>,
> <0x02000000 0x0 0x80000000 0x80000000 0x0 0x0f000000>;
Simply switching to what you show works just fine. PCI
addresses show up being zero-based, as they should be.
What I don't fully understand is why it worked before.
Maybe because this is simply defining bounds in the
mapping?
In any case, I am using ranges properties like what you show.
Thanks Mani.
-Alex
> - Mani
Powered by blists - more mailing lists