Message ID | 20250325102610.2073863-2-maz@kernel.org (mailing list archive) |
---|---|
State | Superseded |
Delegated to: | Krzysztof Wilczyński |
Headers | show |
Series | PCI: apple: Add support for t6020 | expand |
> From: Marc Zyngier <maz@kernel.org> > Date: Tue, 25 Mar 2025 10:25:58 +0000 Hi Marc, Sorry for not spotting this in the earlier versions, but: > From: Alyssa Rosenzweig <alyssa@rosenzweig.io> > > t6020 adds some register ranges compared to t8103, so requires > a new compatible as well as the new PHY registers themselves. > > Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io> > [maz: added PHY registers] > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > Documentation/devicetree/bindings/pci/apple,pcie.yaml | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/pci/apple,pcie.yaml b/Documentation/devicetree/bindings/pci/apple,pcie.yaml > index c8775f9cb0713..77554899b9420 100644 > --- a/Documentation/devicetree/bindings/pci/apple,pcie.yaml > +++ b/Documentation/devicetree/bindings/pci/apple,pcie.yaml > @@ -17,6 +17,10 @@ description: | > implements its root ports. But the ATU found on most DesignWare > PCIe host bridges is absent. > > + On systems derived from T602x, the PHY registers are in a region > + separate from the port registers. In that case, there is one PHY > + register range per port register range. > + > All root ports share a single ECAM space, but separate GPIOs are > used to take the PCI devices on those ports out of reset. Therefore > the standard "reset-gpios" and "max-link-speed" properties appear on > @@ -35,11 +39,12 @@ properties: > - apple,t8103-pcie > - apple,t8112-pcie > - apple,t6000-pcie > + - apple,t6020-pcie > - const: apple,pcie Since the T602x PCIe controller has a different register layout, it isn't compatible with the others, so it should not include the "apple,pcie" compatible. The "downstream" device trees for T602x-based devices do indeed not list "apple,pcie" as a compatible. So I think this needs to be written as: compatible: oneOf: - items: - enum: - apple,t8103-pcie - apple,t8112-pcie - apple,t6000-pcie - const: apple,pcie - const: apple,t6020-pcie > > reg: > minItems: 3 > - maxItems: 6 > + maxItems: 10 > > reg-names: > minItems: 3 > @@ -50,6 +55,10 @@ properties: > - const: port1 > - const: port2 > - const: port3 > + - const: phy0 > + - const: phy1 > + - const: phy2 > + - const: phy3 > > ranges: > minItems: 2 > -- > 2.39.2 > > >
Hi Mark, On Tue, 25 Mar 2025 10:50:18 +0000, Mark Kettenis <mark.kettenis@xs4all.nl> wrote: > > > From: Marc Zyngier <maz@kernel.org> > > Date: Tue, 25 Mar 2025 10:25:58 +0000 > > Hi Marc, > > Sorry for not spotting this in the earlier versions, but: No worries -- I expected issues in that department. > > > From: Alyssa Rosenzweig <alyssa@rosenzweig.io> > > > > t6020 adds some register ranges compared to t8103, so requires > > a new compatible as well as the new PHY registers themselves. > > > > Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io> > > [maz: added PHY registers] > > Signed-off-by: Marc Zyngier <maz@kernel.org> > > --- > > Documentation/devicetree/bindings/pci/apple,pcie.yaml | 11 ++++++++++- > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > diff --git a/Documentation/devicetree/bindings/pci/apple,pcie.yaml b/Documentation/devicetree/bindings/pci/apple,pcie.yaml > > index c8775f9cb0713..77554899b9420 100644 > > --- a/Documentation/devicetree/bindings/pci/apple,pcie.yaml > > +++ b/Documentation/devicetree/bindings/pci/apple,pcie.yaml > > @@ -17,6 +17,10 @@ description: | > > implements its root ports. But the ATU found on most DesignWare > > PCIe host bridges is absent. > > > > + On systems derived from T602x, the PHY registers are in a region > > + separate from the port registers. In that case, there is one PHY > > + register range per port register range. > > + > > All root ports share a single ECAM space, but separate GPIOs are > > used to take the PCI devices on those ports out of reset. Therefore > > the standard "reset-gpios" and "max-link-speed" properties appear on > > @@ -35,11 +39,12 @@ properties: > > - apple,t8103-pcie > > - apple,t8112-pcie > > - apple,t6000-pcie > > + - apple,t6020-pcie > > - const: apple,pcie > > Since the T602x PCIe controller has a different register layout, it > isn't compatible with the others, so it should not include the > "apple,pcie" compatible. The "downstream" device trees for > T602x-based devices do indeed not list "apple,pcie" as a compatible. > So I think this needs to be written as: > > compatible: > oneOf: > - items: > - enum: > - apple,t8103-pcie > - apple,t8112-pcie > - apple,t6000-pcie > - const: apple,pcie > - const: apple,t6020-pcie Ah, indeed, that's a good point. Thanks for that. Whilst I have your attention, how about my question below: > > > > > reg: > > minItems: 3 > > - maxItems: 6 > > + maxItems: 10 > > > > reg-names: > > minItems: 3 > > @@ -50,6 +55,10 @@ properties: > > - const: port1 > > - const: port2 > > - const: port3 > > + - const: phy0 > > + - const: phy1 > > + - const: phy2 > > + - const: phy3 Do we need to make this t6020 specific? Obviously, separate PHY registers do not make much sense before t6020, but I couldn't find a way to describe that. I don't even know if that's a desirable outcome. Thanks, M.
> Date: Tue, 25 Mar 2025 11:02:30 +0000 > From: Marc Zyngier <maz@kernel.org> Hi Marc, > Hi Mark, > > On Tue, 25 Mar 2025 10:50:18 +0000, > Mark Kettenis <mark.kettenis@xs4all.nl> wrote: > > > > > From: Marc Zyngier <maz@kernel.org> > > > Date: Tue, 25 Mar 2025 10:25:58 +0000 > > > > Hi Marc, > > > > Sorry for not spotting this in the earlier versions, but: > > No worries -- I expected issues in that department. > > > > > > From: Alyssa Rosenzweig <alyssa@rosenzweig.io> > > > > > > t6020 adds some register ranges compared to t8103, so requires > > > a new compatible as well as the new PHY registers themselves. > > > > > > Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io> > > > [maz: added PHY registers] > > > Signed-off-by: Marc Zyngier <maz@kernel.org> > > > --- > > > Documentation/devicetree/bindings/pci/apple,pcie.yaml | 11 ++++++++++- > > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > > > diff --git a/Documentation/devicetree/bindings/pci/apple,pcie.yaml b/Documentation/devicetree/bindings/pci/apple,pcie.yaml > > > index c8775f9cb0713..77554899b9420 100644 > > > --- a/Documentation/devicetree/bindings/pci/apple,pcie.yaml > > > +++ b/Documentation/devicetree/bindings/pci/apple,pcie.yaml > > > @@ -17,6 +17,10 @@ description: | > > > implements its root ports. But the ATU found on most DesignWare > > > PCIe host bridges is absent. > > > > > > + On systems derived from T602x, the PHY registers are in a region > > > + separate from the port registers. In that case, there is one PHY > > > + register range per port register range. > > > + > > > All root ports share a single ECAM space, but separate GPIOs are > > > used to take the PCI devices on those ports out of reset. Therefore > > > the standard "reset-gpios" and "max-link-speed" properties appear on > > > @@ -35,11 +39,12 @@ properties: > > > - apple,t8103-pcie > > > - apple,t8112-pcie > > > - apple,t6000-pcie > > > + - apple,t6020-pcie > > > - const: apple,pcie > > > > Since the T602x PCIe controller has a different register layout, it > > isn't compatible with the others, so it should not include the > > "apple,pcie" compatible. The "downstream" device trees for > > T602x-based devices do indeed not list "apple,pcie" as a compatible. > > So I think this needs to be written as: > > > > compatible: > > oneOf: > > - items: > > - enum: > > - apple,t8103-pcie > > - apple,t8112-pcie > > - apple,t6000-pcie > > - const: apple,pcie > > - const: apple,t6020-pcie > > Ah, indeed, that's a good point. Thanks for that. > > Whilst I have your attention, how about my question below: > > > > > > > > > reg: > > > minItems: 3 > > > - maxItems: 6 > > > + maxItems: 10 > > > > > > reg-names: > > > minItems: 3 > > > @@ -50,6 +55,10 @@ properties: > > > - const: port1 > > > - const: port2 > > > - const: port3 > > > + - const: phy0 > > > + - const: phy1 > > > + - const: phy2 > > > + - const: phy3 > > Do we need to make this t6020 specific? > > Obviously, separate PHY registers do not make much sense before t6020, > but I couldn't find a way to describe that. I don't even know if > that's a desirable outcome. I don't think there is a way to do that other than creating a separate binding for t6020. But I'm far from a dt-schema expert. Maybe robh has some advice here.
On Tue, 25 Mar 2025 15:41:15 +0000, Mark Kettenis <mark.kettenis@xs4all.nl> wrote: > > > Date: Tue, 25 Mar 2025 11:02:30 +0000 > > From: Marc Zyngier <maz@kernel.org> > > Hi Marc, > > > Hi Mark, > > > > On Tue, 25 Mar 2025 10:50:18 +0000, > > Mark Kettenis <mark.kettenis@xs4all.nl> wrote: > > > > > > > From: Marc Zyngier <maz@kernel.org> > > > > Date: Tue, 25 Mar 2025 10:25:58 +0000 > > > > > > > @@ -50,6 +55,10 @@ properties: > > > > - const: port1 > > > > - const: port2 > > > > - const: port3 > > > > + - const: phy0 > > > > + - const: phy1 > > > > + - const: phy2 > > > > + - const: phy3 > > > > Do we need to make this t6020 specific? > > > > Obviously, separate PHY registers do not make much sense before t6020, > > but I couldn't find a way to describe that. I don't even know if > > that's a desirable outcome. > > I don't think there is a way to do that other than creating a separate > binding for t6020. But I'm far from a dt-schema expert. Maybe robh > has some advice here. Huh, I'd rather not create another binding. The only thing this would buy us is a stricter checking of the register ranges. But it isn't like this block is going to find its way in random HW, and this is only described in a handful of core dtsi files anyway. Unless someone screams (and provides a reasonable alternative), I will leave it as is. Thanks, M.
On Tue, Mar 25, 2025 at 11:49 AM Marc Zyngier <maz@kernel.org> wrote: > > On Tue, 25 Mar 2025 15:41:15 +0000, > Mark Kettenis <mark.kettenis@xs4all.nl> wrote: > > > > > Date: Tue, 25 Mar 2025 11:02:30 +0000 > > > From: Marc Zyngier <maz@kernel.org> > > > > Hi Marc, > > > > > Hi Mark, > > > > > > On Tue, 25 Mar 2025 10:50:18 +0000, > > > Mark Kettenis <mark.kettenis@xs4all.nl> wrote: > > > > > > > > > From: Marc Zyngier <maz@kernel.org> > > > > > Date: Tue, 25 Mar 2025 10:25:58 +0000 > > > > > > > > > @@ -50,6 +55,10 @@ properties: > > > > > - const: port1 > > > > > - const: port2 > > > > > - const: port3 > > > > > + - const: phy0 > > > > > + - const: phy1 > > > > > + - const: phy2 > > > > > + - const: phy3 > > > > > > Do we need to make this t6020 specific? > > > > > > Obviously, separate PHY registers do not make much sense before t6020, > > > but I couldn't find a way to describe that. I don't even know if > > > that's a desirable outcome. > > > > I don't think there is a way to do that other than creating a separate > > binding for t6020. But I'm far from a dt-schema expert. Maybe robh > > has some advice here. > > Huh, I'd rather not create another binding. The only thing this would > buy us is a stricter checking of the register ranges. But it isn't > like this block is going to find its way in random HW, and this is > only described in a handful of core dtsi files anyway. > > Unless someone screams (and provides a reasonable alternative), I will > leave it as is. The simplest thing to do here is (under the 'allOf'): if: properties: compatible: contains: const: apple,t6020-pcie then: properties: reg-names: minItems: 10 If/when we start having different number of ports with phy entries, then it gets messy and each variant has to list out the names or we give up on defining the order. Rob
diff --git a/Documentation/devicetree/bindings/pci/apple,pcie.yaml b/Documentation/devicetree/bindings/pci/apple,pcie.yaml index c8775f9cb0713..77554899b9420 100644 --- a/Documentation/devicetree/bindings/pci/apple,pcie.yaml +++ b/Documentation/devicetree/bindings/pci/apple,pcie.yaml @@ -17,6 +17,10 @@ description: | implements its root ports. But the ATU found on most DesignWare PCIe host bridges is absent. + On systems derived from T602x, the PHY registers are in a region + separate from the port registers. In that case, there is one PHY + register range per port register range. + All root ports share a single ECAM space, but separate GPIOs are used to take the PCI devices on those ports out of reset. Therefore the standard "reset-gpios" and "max-link-speed" properties appear on @@ -35,11 +39,12 @@ properties: - apple,t8103-pcie - apple,t8112-pcie - apple,t6000-pcie + - apple,t6020-pcie - const: apple,pcie reg: minItems: 3 - maxItems: 6 + maxItems: 10 reg-names: minItems: 3 @@ -50,6 +55,10 @@ properties: - const: port1 - const: port2 - const: port3 + - const: phy0 + - const: phy1 + - const: phy2 + - const: phy3 ranges: minItems: 2