From: Bartosz Golaszewski <[email protected]>
Add a PCI compatible for the ATH11K module on QCA6390 and describe the
power inputs from the PMU that it consumes.
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
.../net/wireless/qcom,ath11k-pci.yaml | 46 +++++++++++++++++++
1 file changed, 46 insertions(+)
diff --git a/Documentation/devicetree/bindings/net/wireless/qcom,ath11k-pci.yaml b/Documentation/devicetree/bindings/net/wireless/qcom,ath11k-pci.yaml
index 41d023797d7d..8675d7d0215c 100644
--- a/Documentation/devicetree/bindings/net/wireless/qcom,ath11k-pci.yaml
+++ b/Documentation/devicetree/bindings/net/wireless/qcom,ath11k-pci.yaml
@@ -17,6 +17,7 @@ description: |
properties:
compatible:
enum:
+ - pci17cb,1101 # QCA6390
- pci17cb,1103 # WCN6855
reg:
@@ -28,10 +29,55 @@ properties:
string to uniquely identify variant of the calibration data for designs
with colliding bus and device ids
+ vddrfacmn-supply:
+ description: VDD_RFA_CMN supply regulator handle
+
+ vddaon-supply:
+ description: VDD_AON supply regulator handle
+
+ vddwlcx-supply:
+ description: VDD_WL_CX supply regulator handle
+
+ vddwlmx-supply:
+ description: VDD_WL_MX supply regulator handle
+
+ vddrfa0p8-supply:
+ description: VDD_RFA_0P8 supply regulator handle
+
+ vddrfa1p2-supply:
+ description: VDD_RFA_1P2 supply regulator handle
+
+ vddrfa1p7-supply:
+ description: VDD_RFA_1P7 supply regulator handle
+
+ vddpcie0p9-supply:
+ description: VDD_PCIE_0P9 supply regulator handle
+
+ vddpcie1p8-supply:
+ description: VDD_PCIE_1P8 supply regulator handle
+
required:
- compatible
- reg
+allOf:
+ - if:
+ properties:
+ compatible:
+ contains:
+ const: pci17cb,1101
+ then:
+ required:
+ - vddrfacmn-supply
+ - vddaon-supply
+ - vddwlcx-supply
+ - vddwlmx-supply
+ - vddrfa0p8-supply
+ - vddrfa1p2-supply
+ - vddrfa1p7-supply
+ - vddpcie0p9-supply
+ - vddpcie1p8-supply
+
additionalProperties: false
examples:
--
2.40.1
On Thu, Jun 6, 2024 at 4:02 PM Kalle Valo <[email protected]> wrote:
>
> Bartosz Golaszewski <[email protected]> writes:
>
> > On Thu, Jun 6, 2024 at 3:30 PM Kalle Valo <[email protected]> wrote:
> >
> >>
> >> Bartosz Golaszewski <[email protected]> writes:
> >>
> >> > From: Bartosz Golaszewski <[email protected]>
> >> >
> >> > Add a PCI compatible for the ATH11K module on QCA6390 and describe the
> >> > power inputs from the PMU that it consumes.
> >> >
> >> > Reviewed-by: Krzysztof Kozlowski <[email protected]>
> >> > Signed-off-by: Bartosz Golaszewski <[email protected]>
> >>
> >> [...]
> >>
> >> > +allOf:
> >> > + - if:
> >> > + properties:
> >> > + compatible:
> >> > + contains:
> >> > + const: pci17cb,1101
> >> > + then:
> >> > + required:
> >> > + - vddrfacmn-supply
> >> > + - vddaon-supply
> >> > + - vddwlcx-supply
> >> > + - vddwlmx-supply
> >> > + - vddrfa0p8-supply
> >> > + - vddrfa1p2-supply
> >> > + - vddrfa1p7-supply
> >> > + - vddpcie0p9-supply
> >> > + - vddpcie1p8-supply
> >>
> >> Not sure if we discussed this before, but based on this I understand
> >> that there can't be an DT entry for device pci17cb,1101 without all the
> >> supply properties? But there are QCA6390 devices with PCI id 17cb:1101
> >> which do not need these supplies and already work. For example, my Dell
> >> XPS 13 x86 laptop is one. Or anyone who manually installs QCA6390 board
> >> to their PCI slot and some of them might want to use DT, for example
> >> setting qcom,ath11k-calibration-variant.
> >>
> >> This is not a blocker for me, just making sure that we are not breaking
> >> any existing setups.
> >>
> >
> > If they are already powered up without the need for the PCI pwrctl
> > driver to do it, then they will work alright. Bindings don't affect
> > functionality.
>
> Sure, I'm not worried about functionality. I'm worried that if I
> there's, for example, an ARM based setup which uses DT and wants to use
> a similar QCA6390 board that I have, and set
> qcom,ath11k-calibration-variant in DT. In other words, I'm worried if
> you are looking at this only for Snapdragon family of boards?
>
No, what I'm looking at is the entire QCA6390 package. That means WLAN
*and* Bluetooth *and* the PMU that manages power.
If you're using the QCA6390 on a device-tree system then you should
probably model at least the WLAN node and the PMU and the problem with
supplies is fixed. But if you don't have the supplies, that's alright
for downstream.
> Again, I don't see this as a blocker. I just want to understand how this
> should work for all types of devices there are out there.
>
> > But if you have a QCA6390 then you have its PMU too and the bindings
> > model the real-world hardware.
> >
> > IOW: your laptop should be alright but the supplies are really there
> > which warrants adding them to the bindings.
>
> Sorry, not following here. Can you clarify your comment "the supplies
> are really there"? You mean inside the PCI board? But that's not visible
> to the kernel in anyway, the PCI board just works after I plug it in.
> It's like a regular PCI device. So I don't understand why that should be
> visible in DT, but I can very well be missing something.
>
I think you're thinking about some kind of detachable PCIe board with
this chipset on it. I refer to the QCA6390 chipset itself which is
also more than just PCI. The Bluetooth interface doesn't use PCI at
all. On the boards I'm working on, the chipset is just soldered to the
main board. If your detachable board "just works" then it must be
wired in a way that enables WLAN the moment it's plugged in but this
doesn't happen over PCI. The chipset has a power input and GPIOs to
enable each module.
Also: I doubt you need DT for your detachable board?
Bart
> --
> https://patchwork.kernel.org/project/linux-wireless/list/
>
> https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Bartosz Golaszewski <[email protected]> writes:
> On Thu, Jun 6, 2024 at 4:02 PM Kalle Valo <[email protected]> wrote:
>
>>
>> Bartosz Golaszewski <[email protected]> writes:
>>
>> > On Thu, Jun 6, 2024 at 3:30 PM Kalle Valo <[email protected]> wrote:
>> >
>> >>
>> >> Bartosz Golaszewski <[email protected]> writes:
>> >>
>> >> > From: Bartosz Golaszewski <[email protected]>
>> >> >
>> >> > Add a PCI compatible for the ATH11K module on QCA6390 and describe the
>> >> > power inputs from the PMU that it consumes.
>> >> >
>> >> > Reviewed-by: Krzysztof Kozlowski <[email protected]>
>> >> > Signed-off-by: Bartosz Golaszewski <[email protected]>
>> >>
>> >> [...]
>> >>
>> >> > +allOf:
>> >> > + - if:
>> >> > + properties:
>> >> > + compatible:
>> >> > + contains:
>> >> > + const: pci17cb,1101
>> >> > + then:
>> >> > + required:
>> >> > + - vddrfacmn-supply
>> >> > + - vddaon-supply
>> >> > + - vddwlcx-supply
>> >> > + - vddwlmx-supply
>> >> > + - vddrfa0p8-supply
>> >> > + - vddrfa1p2-supply
>> >> > + - vddrfa1p7-supply
>> >> > + - vddpcie0p9-supply
>> >> > + - vddpcie1p8-supply
>> >>
>> >> Not sure if we discussed this before, but based on this I understand
>> >> that there can't be an DT entry for device pci17cb,1101 without all the
>> >> supply properties? But there are QCA6390 devices with PCI id 17cb:1101
>> >> which do not need these supplies and already work. For example, my Dell
>> >> XPS 13 x86 laptop is one. Or anyone who manually installs QCA6390 board
>> >> to their PCI slot and some of them might want to use DT, for example
>> >> setting qcom,ath11k-calibration-variant.
>> >>
>> >> This is not a blocker for me, just making sure that we are not breaking
>> >> any existing setups.
>> >>
>> >
>> > If they are already powered up without the need for the PCI pwrctl
>> > driver to do it, then they will work alright. Bindings don't affect
>> > functionality.
>>
>> Sure, I'm not worried about functionality. I'm worried that if I
>> there's, for example, an ARM based setup which uses DT and wants to use
>> a similar QCA6390 board that I have, and set
>> qcom,ath11k-calibration-variant in DT. In other words, I'm worried if
>> you are looking at this only for Snapdragon family of boards?
>>
>
> No, what I'm looking at is the entire QCA6390 package. That means WLAN
> *and* Bluetooth *and* the PMU that manages power.
I think we are just looking at this from different point of views. You
are looking at a datasheet (most likely for a Snapdragon based system)
and I'm looking what actual devices there are out in the field.
> If you're using the QCA6390 on a device-tree system then you should
> probably model at least the WLAN node and the PMU and the problem with
> supplies is fixed.
But why? If there are boards out there who don't need any of this why
would they still need to model all this in DT?
Based on the discussions I have heard only Snapdragon systems who
require all this configuration you describe. Of course there can be
other systems but I have not heard about those.
> But if you don't have the supplies, that's alright for downstream.
What do you mean downstream in this context?
>> Again, I don't see this as a blocker. I just want to understand how this
>> should work for all types of devices there are out there.
>>
>> > But if you have a QCA6390 then you have its PMU too and the bindings
>> > model the real-world hardware.
>> >
>> > IOW: your laptop should be alright but the supplies are really there
>> > which warrants adding them to the bindings.
>>
>> Sorry, not following here. Can you clarify your comment "the supplies
>> are really there"? You mean inside the PCI board? But that's not visible
>> to the kernel in anyway, the PCI board just works after I plug it in.
>> It's like a regular PCI device. So I don't understand why that should be
>> visible in DT, but I can very well be missing something.
>>
>
> I think you're thinking about some kind of detachable PCIe board with
> this chipset on it.
Exactly, a lot of WLAN boards are like this.
> I refer to the QCA6390 chipset itself which is also more than just
> PCI. The Bluetooth interface doesn't use PCI at all. On the boards I'm
> working on, the chipset is just soldered to the main board.
And I guess you are looking at Snapdragon boards only?
> If your detachable board "just works" then it must be wired in a way
> that enables WLAN the moment it's plugged in but this doesn't happen
> over PCI. The chipset has a power input and GPIOs to enable each
> module.
I don't know how the boards are implemented but it could be so. But from
host system point of view it's just a regular PCI device.
> Also: I doubt you need DT for your detachable board?
Sure, I don't need DT but that's not my point. My point is why require
these supplies for _all_ devices having PCI id 17cb:1101 (ie. QCA6390)
then clearly there are such devices which don't need it? To me that's
bad design and, if I'm understanding correctly, prevents use of
qcom,ath11k-calibration-variant property. To me having the supplies
optional in DT is more approriate.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Bartosz Golaszewski <[email protected]> writes:
>> >> Sure, I don't need DT but that's not my point. My point is why require
>> >> these supplies for _all_ devices having PCI id 17cb:1101 (ie. QCA6390)
>> >> then clearly there are such devices which don't need it? To me that's
>> >> bad design and, if I'm understanding correctly, prevents use of
>> >> qcom,ath11k-calibration-variant property. To me having the supplies
>> >> optional in DT is more approriate.
>> >>
>> >
>> > We require them because *they are physically there*.
>>
>> I understand that for all known DT QCA6390 hardware, the supplies should
>> be provided thus they should be required. If in the future we have
>> different design or we represent some pluggable PCI card, then:
>> 1. Probably that PCI card does not need power sequencing, thus no DT
>> description,
>> 2. If still needs power sequencing, you can always amend bindings and
>> un-require the supplies.
>>
>>
>> Best regards,
>> Krzysztof
>>
>
> Kalle, does the above answer your questions? Are these bindings good to go?
To me most important is that we are on the same page that in some cases
(eg. with M.2 boards) the supplies can be optional and we can update the
bindings doc once such need arises (but we don't make any changes right
now). Based on point 2 from Krzysztof I think we all agree, right?
Just making sure: if we later change the supplies optional does that
create any problems with backwards compatibility? It's important that
updates go smoothly.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
On Wed, Jun 12, 2024 at 2:49 PM Kalle Valo <[email protected]> wrote:
>
> Bartosz Golaszewski <[email protected]> writes:
>
> >> >> Sure, I don't need DT but that's not my point. My point is why require
> >> >> these supplies for _all_ devices having PCI id 17cb:1101 (ie. QCA6390)
> >> >> then clearly there are such devices which don't need it? To me that's
> >> >> bad design and, if I'm understanding correctly, prevents use of
> >> >> qcom,ath11k-calibration-variant property. To me having the supplies
> >> >> optional in DT is more approriate.
> >> >>
> >> >
> >> > We require them because *they are physically there*.
> >>
> >> I understand that for all known DT QCA6390 hardware, the supplies should
> >> be provided thus they should be required. If in the future we have
> >> different design or we represent some pluggable PCI card, then:
> >> 1. Probably that PCI card does not need power sequencing, thus no DT
> >> description,
> >> 2. If still needs power sequencing, you can always amend bindings and
> >> un-require the supplies.
> >>
> >>
> >> Best regards,
> >> Krzysztof
> >>
> >
> > Kalle, does the above answer your questions? Are these bindings good to go?
>
> To me most important is that we are on the same page that in some cases
> (eg. with M.2 boards) the supplies can be optional and we can update the
> bindings doc once such need arises (but we don't make any changes right
> now). Based on point 2 from Krzysztof I think we all agree, right?
>
> Just making sure: if we later change the supplies optional does that
> create any problems with backwards compatibility? It's important that
> updates go smoothly.
No, you can always relax the requirements alright. It's only when you
make them more strict that you'll run into backward compatibility
issues.
Bart