| Message ID | 20231027145422.40265-2-nks@flawful.org |
|---|---|
| State | New |
| Headers | show |
| Series | rockchip DWC PCIe improvements | expand |
On Fri, Oct 27, 2023 at 9:56 AM Niklas Cassel <nks@flawful.org> wrote: > > From: Niklas Cassel <niklas.cassel@wdc.com> > > Even though rockchip-dw-pcie.yaml inherits snps,dw-pcie.yaml > using: > > allOf: > - $ref: /schemas/pci/snps,dw-pcie.yaml# > > and snps,dw-pcie.yaml does have the atu reg defined, in order to be > able to use this reg, while still making sure 'make CHECK_DTBS=y' > pass, we need to add this reg to rockchip-dw-pcie.yaml. > > All compatible strings (rockchip,rk3568-pcie and rockchip,rk3588-pcie) > should have this reg. > > The regs in the example are updated to actually match pcie3x2 on rk3568. Breaking compatibility on these platforms is okay because ...? > > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com> > --- > .../devicetree/bindings/pci/rockchip-dw-pcie.yaml | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-)
Hello Rob, On Fri, Oct 27, 2023 at 10:58:28AM -0500, Rob Herring wrote: > On Fri, Oct 27, 2023 at 9:56 AM Niklas Cassel <nks@flawful.org> wrote: > > > > From: Niklas Cassel <niklas.cassel@wdc.com> > > > > Even though rockchip-dw-pcie.yaml inherits snps,dw-pcie.yaml > > using: > > > > allOf: > > - $ref: /schemas/pci/snps,dw-pcie.yaml# > > > > and snps,dw-pcie.yaml does have the atu reg defined, in order to be > > able to use this reg, while still making sure 'make CHECK_DTBS=y' > > pass, we need to add this reg to rockchip-dw-pcie.yaml. > > > > All compatible strings (rockchip,rk3568-pcie and rockchip,rk3588-pcie) > > should have this reg. > > > > The regs in the example are updated to actually match pcie3x2 on rk3568. > > Breaking compatibility on these platforms is okay because ...? I don't follow, could you please elaborate? Kind regards, Niklas
On Fri, Oct 27, 2023 at 11:15 AM Niklas Cassel <Niklas.Cassel@wdc.com> wrote: > > Hello Rob, > > On Fri, Oct 27, 2023 at 10:58:28AM -0500, Rob Herring wrote: > > On Fri, Oct 27, 2023 at 9:56 AM Niklas Cassel <nks@flawful.org> wrote: > > > > > > From: Niklas Cassel <niklas.cassel@wdc.com> > > > > > > Even though rockchip-dw-pcie.yaml inherits snps,dw-pcie.yaml > > > using: > > > > > > allOf: > > > - $ref: /schemas/pci/snps,dw-pcie.yaml# > > > > > > and snps,dw-pcie.yaml does have the atu reg defined, in order to be > > > able to use this reg, while still making sure 'make CHECK_DTBS=y' > > > pass, we need to add this reg to rockchip-dw-pcie.yaml. > > > > > > All compatible strings (rockchip,rk3568-pcie and rockchip,rk3588-pcie) > > > should have this reg. > > > > > > The regs in the example are updated to actually match pcie3x2 on rk3568. > > > > Breaking compatibility on these platforms is okay because ...? > > I don't follow, could you please elaborate? A DT which was valid without 'atu' region is now not valid with this change. If I'm reading this schema with the change, then not having 'atu' is an error and the OS can treat it as an error. If it does, then it wouldn't work with existing DTs. You cannot add new required properties or required entries. If you can say no users of the affected platforms care (e.g. only you have a board) or the binding is not yet in use, then it's fine. But you have to state that justification. I suspect that is not the case here. Rob
Hi Niklas, Am Freitag, 27. Oktober 2023, 18:14:32 CEST schrieb Niklas Cassel: > On Fri, Oct 27, 2023 at 10:58:28AM -0500, Rob Herring wrote: > > On Fri, Oct 27, 2023 at 9:56 AM Niklas Cassel <nks@flawful.org> wrote: > > > > > > From: Niklas Cassel <niklas.cassel@wdc.com> > > > > > > Even though rockchip-dw-pcie.yaml inherits snps,dw-pcie.yaml > > > using: > > > > > > allOf: > > > - $ref: /schemas/pci/snps,dw-pcie.yaml# > > > > > > and snps,dw-pcie.yaml does have the atu reg defined, in order to be > > > able to use this reg, while still making sure 'make CHECK_DTBS=y' > > > pass, we need to add this reg to rockchip-dw-pcie.yaml. > > > > > > All compatible strings (rockchip,rk3568-pcie and rockchip,rk3588-pcie) > > > should have this reg. > > > > > > The regs in the example are updated to actually match pcie3x2 on rk3568. > > > > Breaking compatibility on these platforms is okay because ...? > > I don't follow, could you please elaborate? you're adding the atu reg unconditionally as required element. Newer kernel versions (strongly) _should_ work with older devicetrees. So a kernel with that change should also work with a dtb build from the old style. DTBs are essentially part of the device firmware, so while some devices can update theirs easily, you can't really require a dtb update. I guess you could something like: reg-names: oneOf: - deprecated: true items: - const: dbi - const: apb - const: config - items: - const: dbi - const: apb - const: config - const: atu (may not be accurate and to spec yet)
On Fri, Oct 27, 2023 at 12:03:15PM -0500, Rob Herring wrote: > On Fri, Oct 27, 2023 at 11:15 AM Niklas Cassel <Niklas.Cassel@wdc.com> wrote: > > > > Hello Rob, > > > > On Fri, Oct 27, 2023 at 10:58:28AM -0500, Rob Herring wrote: > > > On Fri, Oct 27, 2023 at 9:56 AM Niklas Cassel <nks@flawful.org> wrote: > > > > > > > > From: Niklas Cassel <niklas.cassel@wdc.com> > > > > > > > > Even though rockchip-dw-pcie.yaml inherits snps,dw-pcie.yaml > > > > using: > > > > > > > > allOf: > > > > - $ref: /schemas/pci/snps,dw-pcie.yaml# > > > > > > > > and snps,dw-pcie.yaml does have the atu reg defined, in order to be > > > > able to use this reg, while still making sure 'make CHECK_DTBS=y' > > > > pass, we need to add this reg to rockchip-dw-pcie.yaml. > > > > > > > > All compatible strings (rockchip,rk3568-pcie and rockchip,rk3588-pcie) > > > > should have this reg. > > > > > > > > The regs in the example are updated to actually match pcie3x2 on rk3568. > > > > > > Breaking compatibility on these platforms is okay because ...? > > > > I don't follow, could you please elaborate? > > A DT which was valid without 'atu' region is now not valid with this > change. If I'm reading this schema with the change, then not having > 'atu' is an error and the OS can treat it as an error. If it does, > then it wouldn't work with existing DTs. You cannot add new required > properties or required entries. > > If you can say no users of the affected platforms care (e.g. only you > have a board) or the binding is not yet in use, then it's fine. But > you have to state that justification. I suspect that is not the case > here. FWIW, I had "atu" as optional in v2 of this series. Since I made the effort in v3 to add "atu" to all the existing users of the rockchip binding, I thought that marking it required in the rockchip binding would help ensure that any future rockchip platform does not forget to add "atu". I know that DT has to be backwards compatible, but the device driver works fine with a DT that lacks "atu" (even though you will not be able to detect all iATUs), so an old DT would still work. But yes, running make CHECK_DTBS=y with the new binding, would not pass for an old DT. I get your point, you can never add a required property or entries to an existing compatible (this is in use). If we look at snps,dw-pcie.yaml, "atu" is optional, so that correlates to the device driver being able to work without "atu" specified. Since the rockchip driver doesn't get "atu" itself, but relies on the common code to get it, it should obviously be optional also for the rockchip binding. I guess my problem is that I just want to inherit the entry from the common binding... Is there no DT keyword to extend an existing binding, so that you inherit all the properties/entries from that common binding, while still allowing you to define (or overload) additional properties/entries? Even if there is no way to inherit all properties/entries from a common binding, it would be nice to be able to inherit a specific property/entry using a "inherit" keyword, such that you get the same definition (optional/required) as the parent binding. Kind regards, Niklas
On Sat, Oct 28, 2023 at 12:11:02PM +0000, Niklas Cassel wrote: > On Fri, Oct 27, 2023 at 12:03:15PM -0500, Rob Herring wrote: > > On Fri, Oct 27, 2023 at 11:15 AM Niklas Cassel <Niklas.Cassel@wdc.com> wrote: > > > > > > Hello Rob, > > > > > > On Fri, Oct 27, 2023 at 10:58:28AM -0500, Rob Herring wrote: > > > > On Fri, Oct 27, 2023 at 9:56 AM Niklas Cassel <nks@flawful.org> wrote: > > > > > > > > > > From: Niklas Cassel <niklas.cassel@wdc.com> > > > > > > > > > > Even though rockchip-dw-pcie.yaml inherits snps,dw-pcie.yaml > > > > > using: > > > > > > > > > > allOf: > > > > > - $ref: /schemas/pci/snps,dw-pcie.yaml# > > > > > > > > > > and snps,dw-pcie.yaml does have the atu reg defined, in order to be > > > > > able to use this reg, while still making sure 'make CHECK_DTBS=y' > > > > > pass, we need to add this reg to rockchip-dw-pcie.yaml. > > > > > > > > > > All compatible strings (rockchip,rk3568-pcie and rockchip,rk3588-pcie) > > > > > should have this reg. > > > > > > > > > > The regs in the example are updated to actually match pcie3x2 on rk3568. > > > > > > > > Breaking compatibility on these platforms is okay because ...? > > > > > > I don't follow, could you please elaborate? > > > > A DT which was valid without 'atu' region is now not valid with this > > change. If I'm reading this schema with the change, then not having > > 'atu' is an error and the OS can treat it as an error. If it does, > > then it wouldn't work with existing DTs. You cannot add new required > > properties or required entries. > > > > If you can say no users of the affected platforms care (e.g. only you > > have a board) or the binding is not yet in use, then it's fine. But > > you have to state that justification. I suspect that is not the case > > here. > > FWIW, I had "atu" as optional in v2 of this series. Right, that was "correct". > Since I made the effort in v3 to add "atu" to all the existing users of the > rockchip binding, I thought that marking it required in the rockchip binding > would help ensure that any future rockchip platform does not forget to add > "atu". It would nice to test both this kind of thing and compatibility. The only way I know of to do that is with 'deprecated' keyword which still needs support in dtschema to remove all the deprecated schemas (which would then cause warnings). That's not hard, I just haven't done it yet. To use 'deprecated' here, you could do something like this: items: - ... - ... - ... - ... allOf: - deprecated: true minItems: 3 However, that would also not work because we implicitly add 'minItems: 4', but that could be fixed. For 'required', we'd need a 'oneOf' with 2 lists of required properties which is kind of ugly. > I know that DT has to be backwards compatible, but the device driver works > fine with a DT that lacks "atu" (even though you will not be able to detect > all iATUs), so an old DT would still work. > > But yes, running make CHECK_DTBS=y with the new binding, would not pass for > an old DT. > > I get your point, you can never add a required property or entries to an > existing compatible (this is in use). We may test for this at some point. I want to be able to test for ABI breaks rather than catch them in reviews. Right now I'm just kicking around ideas in my head on how to do that... > If we look at snps,dw-pcie.yaml, "atu" is optional, so that correlates to > the device driver being able to work without "atu" specified. > > Since the rockchip driver doesn't get "atu" itself, but relies on the common > code to get it, it should obviously be optional also for the rockchip binding. The way this binding works is snps,dw-pcie.yaml defines the set of common properties. The SoC specific binding still has to define which ones it uses. It's some duplication, but there's not really any way around it on these licensed IPs unless the bindings are complete up front. > I guess my problem is that I just want to inherit the entry from the common > binding... > > Is there no DT keyword to extend an existing binding, so that you inherit all > the properties/entries from that common binding, while still allowing you to > define (or overload) additional properties/entries? > > Even if there is no way to inherit all properties/entries from a common binding, > it would be nice to be able to inherit a specific property/entry using a > "inherit" keyword, such that you get the same definition (optional/required) as > the parent binding. The only way to inherit is via a $ref. We can only add more constraints to a parent/referenced binding. You could have a $ref to "snps,dw-pcie.yaml#/properties/reg-names/items/oneOf/3" to get the 4th reg-name in the list of common properties. I wouldn't recommend doing that because I think it will be fragile and difficult to maintain. Rob
diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml index 1ae8dcfa072c..6ca87ff4ae20 100644 --- a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml @@ -33,12 +33,14 @@ properties: - description: Data Bus Interface (DBI) registers - description: Rockchip designed configuration registers - description: Config registers + - description: iATU/eDMA registers reg-names: items: - const: dbi - const: apb - const: config + - const: atu clocks: minItems: 5 @@ -171,10 +173,11 @@ examples: pcie3x2: pcie@fe280000 { compatible = "rockchip,rk3568-pcie"; - reg = <0x3 0xc0800000 0x0 0x390000>, - <0x0 0xfe280000 0x0 0x10000>, - <0x3 0x80000000 0x0 0x100000>; - reg-names = "dbi", "apb", "config"; + reg = <0x3 0xc0800000 0x0 0x00300000>, + <0x0 0xfe280000 0x0 0x00010000>, + <0x0 0xf0000000 0x0 0x00100000>, + <0x3 0xc0b00000 0x0 0x00100000>; + reg-names = "dbi", "apb", "config", "atu"; bus-range = <0x20 0x2f>; clocks = <&cru 143>, <&cru 144>, <&cru 145>, <&cru 146>,