| Message ID | 20230320104658.22186-2-johan+linaro@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | arm64: dts: qcom: sc8280xp-x13s: add wifi calibration variant | expand |
Johan Hovold <johan+linaro@kernel.org> writes: > Add devicetree bindings for Qualcomm ath11k PCIe devices such as WCN6856 > for which the calibration data variant may need to be described. > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > --- > .../bindings/net/wireless/pci17cb,1103.yaml | 56 +++++++++++++++++++ > 1 file changed, 56 insertions(+) > create mode 100644 Documentation/devicetree/bindings/net/wireless/pci17cb,1103.yaml I'm confused (as usual), how does this differ from bindings/net/wireless/qcom,ath11k.yaml? Why we need two .yaml files?
On Mon, Mar 20, 2023 at 02:22:12PM +0200, Kalle Valo wrote: > Johan Hovold <johan+linaro@kernel.org> writes: > > > Add devicetree bindings for Qualcomm ath11k PCIe devices such as WCN6856 > > for which the calibration data variant may need to be described. > > > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > > --- > > .../bindings/net/wireless/pci17cb,1103.yaml | 56 +++++++++++++++++++ > > 1 file changed, 56 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/net/wireless/pci17cb,1103.yaml > > I'm confused (as usual), how does this differ from > bindings/net/wireless/qcom,ath11k.yaml? Why we need two .yaml files? Almost none of bindings/net/wireless/qcom,ath11k.yaml applies to WCN6856 when using PCIe (e.g. as most properties are then discoverable). We could try to encode everything in one file, but that would likely just result in a big mess of a schema with conditionals all over. Johan
+ ath11k list Johan Hovold <johan@kernel.org> writes: > On Mon, Mar 20, 2023 at 02:22:12PM +0200, Kalle Valo wrote: >> Johan Hovold <johan+linaro@kernel.org> writes: >> >> > Add devicetree bindings for Qualcomm ath11k PCIe devices such as WCN6856 >> > for which the calibration data variant may need to be described. >> > >> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> >> > --- >> > .../bindings/net/wireless/pci17cb,1103.yaml | 56 +++++++++++++++++++ >> > 1 file changed, 56 insertions(+) >> > create mode 100644 >> > Documentation/devicetree/bindings/net/wireless/pci17cb,1103.yaml >> >> I'm confused (as usual), how does this differ from >> bindings/net/wireless/qcom,ath11k.yaml? Why we need two .yaml files? > > Almost none of bindings/net/wireless/qcom,ath11k.yaml applies to WCN6856 > when using PCIe (e.g. as most properties are then discoverable). > > We could try to encode everything in one file, but that would likely > just result in a big mess of a schema with conditionals all over. Ah, so the current qcom,ath11k.yaml would be only for ath11k AHB devices and this new file is only for ath11k PCI devices? But why still the odd name pci17cb,1103.yaml? It's not really descriptive and I'm for sure will not remember that pci17cb,1103.yaml is for ath11k :) Also it doesn't look good that we have qcom,ath11k-calibration-variant documented twice now. I'm no DT expert but isn't there any other way? Is it possible to include other files? For example, if we would have three files: qcom,ath11k.yaml qcom,ath11k-ahb.yaml qcom,ath11k-pci.yaml Then have the common properties like ath11k-calibration-variant in the first file and ahb/pci files would include that.
On 20/03/2023 11:46, Johan Hovold wrote: > Add devicetree bindings for Qualcomm ath11k PCIe devices such as WCN6856 > for which the calibration data variant may need to be described. > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > --- > .../bindings/net/wireless/pci17cb,1103.yaml | 56 +++++++++++++++++++ > 1 file changed, 56 insertions(+) > create mode 100644 Documentation/devicetree/bindings/net/wireless/pci17cb,1103.yaml > > diff --git a/Documentation/devicetree/bindings/net/wireless/pci17cb,1103.yaml b/Documentation/devicetree/bindings/net/wireless/pci17cb,1103.yaml > new file mode 100644 > index 000000000000..df67013822c6 > --- /dev/null > +++ b/Documentation/devicetree/bindings/net/wireless/pci17cb,1103.yaml PCI devices are kind of exception in the naming, so this should be qcom,ath11k-pci.yaml or qcom,wcn6856.yaml (or something similar) > @@ -0,0 +1,56 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +# Copyright (c) 2023 Linaro Limited > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/net/wireless/pci17cb,1103.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Qualcomm Technologies ath11k wireless devices (PCIe) > + > +maintainers: > + - Kalle Valo <kvalo@kernel.org> > + > +description: | > + Qualcomm Technologies IEEE 802.11ax PCIe devices. > + > +properties: > + compatible: > + enum: > + - pci17cb,1103 # WCN6856 > + > + reg: > + maxItems: 1 > + > + qcom,ath11k-calibration-variant: qcom,calibration-variant > + $ref: /schemas/types.yaml#/definitions/string > + description: calibration data variant Your description copies the name of property. Instead say something more... > + Best regards, Krzysztof
On 21/03/2023 09:14, Krzysztof Kozlowski wrote: > On 20/03/2023 11:46, Johan Hovold wrote: >> Add devicetree bindings for Qualcomm ath11k PCIe devices such as WCN6856 >> for which the calibration data variant may need to be described. >> >> Signed-off-by: Johan Hovold <johan+linaro@kernel.org> >> --- >> .../bindings/net/wireless/pci17cb,1103.yaml | 56 +++++++++++++++++++ >> 1 file changed, 56 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/net/wireless/pci17cb,1103.yaml >> >> diff --git a/Documentation/devicetree/bindings/net/wireless/pci17cb,1103.yaml b/Documentation/devicetree/bindings/net/wireless/pci17cb,1103.yaml >> new file mode 100644 >> index 000000000000..df67013822c6 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/net/wireless/pci17cb,1103.yaml > > PCI devices are kind of exception in the naming, so this should be > qcom,ath11k-pci.yaml or qcom,wcn6856.yaml (or something similar) > > >> @@ -0,0 +1,56 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +# Copyright (c) 2023 Linaro Limited >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/net/wireless/pci17cb,1103.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Qualcomm Technologies ath11k wireless devices (PCIe) >> + >> +maintainers: >> + - Kalle Valo <kvalo@kernel.org> >> + >> +description: | >> + Qualcomm Technologies IEEE 802.11ax PCIe devices. >> + >> +properties: >> + compatible: >> + enum: >> + - pci17cb,1103 # WCN6856 >> + >> + reg: >> + maxItems: 1 >> + >> + qcom,ath11k-calibration-variant: > > qcom,calibration-variant Ah, so there is already property with ath11k, then let's go with existing name. Best regards, Krzysztof
On Mon, Mar 20, 2023 at 08:41:21PM +0200, Kalle Valo wrote: > + ath11k list > > Johan Hovold <johan@kernel.org> writes: > > > On Mon, Mar 20, 2023 at 02:22:12PM +0200, Kalle Valo wrote: > >> Johan Hovold <johan+linaro@kernel.org> writes: > >> > >> > Add devicetree bindings for Qualcomm ath11k PCIe devices such as WCN6856 > >> > for which the calibration data variant may need to be described. > >> > > >> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > >> > --- > >> > .../bindings/net/wireless/pci17cb,1103.yaml | 56 +++++++++++++++++++ > >> > 1 file changed, 56 insertions(+) > >> > create mode 100644 > >> > Documentation/devicetree/bindings/net/wireless/pci17cb,1103.yaml > >> > >> I'm confused (as usual), how does this differ from > >> bindings/net/wireless/qcom,ath11k.yaml? Why we need two .yaml files? > > > > Almost none of bindings/net/wireless/qcom,ath11k.yaml applies to WCN6856 > > when using PCIe (e.g. as most properties are then discoverable). > > > > We could try to encode everything in one file, but that would likely > > just result in a big mess of a schema with conditionals all over. > > Ah, so the current qcom,ath11k.yaml would be only for ath11k AHB devices > and this new file is only for ath11k PCI devices? Right, there would two separate schema files for the two device classes. > But why still the odd > name pci17cb,1103.yaml? It's not really descriptive and I'm for sure > will not remember that pci17cb,1103.yaml is for ath11k :) Yeah, it's not the best name from that perspective, but it follows the current convention of naming the schema files after the first compatible added. That said, we don't have many schemas for PCI devices so perhaps we can establish a new convention for those. Perhaps by replacing the numerical ids with what we'd use if these were platform devices (e.g. 'qcom,wcn6855.yaml'). As long as the DT maintainers are OK with it, I'd also be happy with something like you suggest below: qcom,ath11k-ahb.yaml qcom,ath11k-pci.yaml (or simply not renaming the current file 'qcom,ath11k.yaml') but I have gotten push back on that in the past. > Also it doesn't look good that we have qcom,ath11k-calibration-variant > documented twice now. I'm no DT expert but isn't there any other way? Is > it possible to include other files? For example, if we would have three > files: > > qcom,ath11k.yaml > qcom,ath11k-ahb.yaml > qcom,ath11k-pci.yaml > > Then have the common properties like ath11k-calibration-variant in the > first file and ahb/pci files would include that. That should be possible, but it's not necessarily better as you'd then have to look up two files to see the bindings for either device class (and as far as I can tell there would not be much sharing beyond this single property). Note that the property could just have well have been named 'qcom,calibration-variant' and then it would be shared also with the ath10k set of devices which currently holds another definition of what is essentially the same property ('qcom,ath10k-calibration-variant'). Johan
On Tue, Mar 21, 2023 at 09:14:15AM +0100, Krzysztof Kozlowski wrote: > On 20/03/2023 11:46, Johan Hovold wrote: > > Add devicetree bindings for Qualcomm ath11k PCIe devices such as WCN6856 > > for which the calibration data variant may need to be described. > > > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > > --- > > .../bindings/net/wireless/pci17cb,1103.yaml | 56 +++++++++++++++++++ > > 1 file changed, 56 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/net/wireless/pci17cb,1103.yaml > > > > diff --git a/Documentation/devicetree/bindings/net/wireless/pci17cb,1103.yaml b/Documentation/devicetree/bindings/net/wireless/pci17cb,1103.yaml > > new file mode 100644 > > index 000000000000..df67013822c6 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/net/wireless/pci17cb,1103.yaml > > PCI devices are kind of exception in the naming, so this should be > qcom,ath11k-pci.yaml or qcom,wcn6856.yaml (or something similar) Heh, I suggested something similar in my reply to Kalle. Let's go with 'qcom,ath11k-pci.yaml' then as he first suggested (and keeping the current schema file unchanged?). > > @@ -0,0 +1,56 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +# Copyright (c) 2023 Linaro Limited > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/net/wireless/pci17cb,1103.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Qualcomm Technologies ath11k wireless devices (PCIe) > > + > > +maintainers: > > + - Kalle Valo <kvalo@kernel.org> > > + > > +description: | > > + Qualcomm Technologies IEEE 802.11ax PCIe devices. > > + > > +properties: > > + compatible: > > + enum: > > + - pci17cb,1103 # WCN6856 > > + > > + reg: > > + maxItems: 1 > > + > > + qcom,ath11k-calibration-variant: > > qcom,calibration-variant This one is already in use as you noticed. > > + $ref: /schemas/types.yaml#/definitions/string > > + description: calibration data variant > > Your description copies the name of property. Instead say something more... Yeah, I was actively avoiding trying to say too much (e.g. mentioning the name of the current firmware file). See the definition in qcom,ath11k.yaml. I can try to find some middle ground unless you prefer copying the current definition. Johan
On 21/03/2023 09:27, Johan Hovold wrote: > >>> + $ref: /schemas/types.yaml#/definitions/string >>> + description: calibration data variant >> >> Your description copies the name of property. Instead say something more... > > Yeah, I was actively avoiding trying to say too much (e.g. mentioning > the name of the current firmware file). See the definition in > qcom,ath11k.yaml. > > I can try to find some middle ground unless you prefer copying the > current definition. So just copy the description or its parts. Best regards, Krzysztof
Johan Hovold <johan@kernel.org> writes: > On Mon, Mar 20, 2023 at 08:41:21PM +0200, Kalle Valo wrote: > >> + ath11k list >> >> Johan Hovold <johan@kernel.org> writes: >> >> > On Mon, Mar 20, 2023 at 02:22:12PM +0200, Kalle Valo wrote: >> >> Johan Hovold <johan+linaro@kernel.org> writes: >> >> >> >> > Add devicetree bindings for Qualcomm ath11k PCIe devices such as WCN6856 >> >> > for which the calibration data variant may need to be described. >> >> > >> >> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> >> >> > --- >> >> > .../bindings/net/wireless/pci17cb,1103.yaml | 56 +++++++++++++++++++ >> >> > 1 file changed, 56 insertions(+) >> >> > create mode 100644 >> >> > Documentation/devicetree/bindings/net/wireless/pci17cb,1103.yaml >> >> >> >> I'm confused (as usual), how does this differ from >> >> bindings/net/wireless/qcom,ath11k.yaml? Why we need two .yaml files? >> > >> > Almost none of bindings/net/wireless/qcom,ath11k.yaml applies to WCN6856 >> > when using PCIe (e.g. as most properties are then discoverable). >> > >> > We could try to encode everything in one file, but that would likely >> > just result in a big mess of a schema with conditionals all over. >> >> Ah, so the current qcom,ath11k.yaml would be only for ath11k AHB devices >> and this new file is only for ath11k PCI devices? > > Right, there would two separate schema files for the two device classes. > >> But why still the odd >> name pci17cb,1103.yaml? It's not really descriptive and I'm for sure >> will not remember that pci17cb,1103.yaml is for ath11k :) > > Yeah, it's not the best name from that perspective, but it follows the > current convention of naming the schema files after the first compatible > added. > > That said, we don't have many schemas for PCI devices so perhaps we can > establish a new convention for those. Perhaps by replacing the numerical > ids with what we'd use if these were platform devices (e.g. > 'qcom,wcn6855.yaml'). > > As long as the DT maintainers are OK with it, I'd also be happy with > something like you suggest below: > > qcom,ath11k-ahb.yaml > qcom,ath11k-pci.yaml > > (or simply not renaming the current file 'qcom,ath11k.yaml') but I have > gotten push back on that in the past. Ok, maybe it's then better not to try renaming qcom,ath11k.yaml and keep it as is. >> Also it doesn't look good that we have qcom,ath11k-calibration-variant >> documented twice now. I'm no DT expert but isn't there any other way? Is >> it possible to include other files? For example, if we would have three >> files: >> >> qcom,ath11k.yaml >> qcom,ath11k-ahb.yaml >> qcom,ath11k-pci.yaml >> >> Then have the common properties like ath11k-calibration-variant in the >> first file and ahb/pci files would include that. > > That should be possible, but it's not necessarily better as you'd then > have to look up two files to see the bindings for either device class > (and as far as I can tell there would not be much sharing beyond this > single property). > > Note that the property could just have well have been named > 'qcom,calibration-variant' and then it would be shared also with the > ath10k set of devices which currently holds another definition of what > is essentially the same property ('qcom,ath10k-calibration-variant'). Oh man, having it as 'qcom,calibration-variant' would have been so much better. Oh well, too late now :( Thanks for explaining all this.
diff --git a/Documentation/devicetree/bindings/net/wireless/pci17cb,1103.yaml b/Documentation/devicetree/bindings/net/wireless/pci17cb,1103.yaml new file mode 100644 index 000000000000..df67013822c6 --- /dev/null +++ b/Documentation/devicetree/bindings/net/wireless/pci17cb,1103.yaml @@ -0,0 +1,56 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +# Copyright (c) 2023 Linaro Limited +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/net/wireless/pci17cb,1103.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Qualcomm Technologies ath11k wireless devices (PCIe) + +maintainers: + - Kalle Valo <kvalo@kernel.org> + +description: | + Qualcomm Technologies IEEE 802.11ax PCIe devices. + +properties: + compatible: + enum: + - pci17cb,1103 # WCN6856 + + reg: + maxItems: 1 + + qcom,ath11k-calibration-variant: + $ref: /schemas/types.yaml#/definitions/string + description: calibration data variant + +required: + - compatible + - reg + +additionalProperties: false + +examples: + - | + pcie { + #address-cells = <3>; + #size-cells = <2>; + + pcie@0 { + device_type = "pci"; + reg = <0x0 0x0 0x0 0x0 0x0>; + #address-cells = <3>; + #size-cells = <2>; + ranges; + + bus-range = <0x01 0xff>; + + wifi@0 { + compatible = "pci17cb,1103"; + reg = <0x10000 0x0 0x0 0x0 0x0>; + + qcom,ath11k-calibration-variant = "LE_X13S"; + }; + }; + };
Add devicetree bindings for Qualcomm ath11k PCIe devices such as WCN6856 for which the calibration data variant may need to be described. Signed-off-by: Johan Hovold <johan+linaro@kernel.org> --- .../bindings/net/wireless/pci17cb,1103.yaml | 56 +++++++++++++++++++ 1 file changed, 56 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/wireless/pci17cb,1103.yaml