| Message ID | 20240814082301.8091-1-brgl@bgdev.pl |
|---|---|
| State | New |
| Headers | show |
| Series | [net-next,v2] dt-bindings: net: ath11k: document the inputs of the ath11k on WCN6855 | expand |
On Wed, Aug 14, 2024 at 10:23:01AM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > Describe the inputs from the PMU of the ath11k module on WCN6855. > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> Acked-by: Conor Dooley <conor.dooley@microchip.com>
Bartosz Golaszewski <brgl@bgdev.pl> writes: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > Describe the inputs from the PMU of the ath11k module on WCN6855. > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > --- > v1 -> v2: > - update the example > > .../net/wireless/qcom,ath11k-pci.yaml | 29 +++++++++++++++++++ > 1 file changed, 29 insertions(+) This goes to ath-next, not net-next. > diff --git a/Documentation/devicetree/bindings/net/wireless/qcom,ath11k-pci.yaml b/Documentation/devicetree/bindings/net/wireless/qcom,ath11k-pci.yaml > index 8675d7d0215c..a71fdf05bc1e 100644 > --- a/Documentation/devicetree/bindings/net/wireless/qcom,ath11k-pci.yaml > +++ b/Documentation/devicetree/bindings/net/wireless/qcom,ath11k-pci.yaml > @@ -50,6 +50,9 @@ properties: > vddrfa1p7-supply: > description: VDD_RFA_1P7 supply regulator handle > > + vddrfa1p8-supply: > + description: VDD_RFA_1P8 supply regulator handle > + > vddpcie0p9-supply: > description: VDD_PCIE_0P9 supply regulator handle > > @@ -77,6 +80,22 @@ allOf: > - vddrfa1p7-supply > - vddpcie0p9-supply > - vddpcie1p8-supply > + - if: > + properties: > + compatible: > + contains: > + const: pci17cb,1103 > + then: > + required: > + - vddrfacmn-supply > + - vddaon-supply > + - vddwlcx-supply > + - vddwlmx-supply > + - vddrfa0p8-supply > + - vddrfa1p2-supply > + - vddrfa1p8-supply > + - vddpcie0p9-supply > + - vddpcie1p8-supply Like we discussed before, shouldn't these supplies be optional as not all modules need them?
On Fri, Aug 16, 2024 at 10:26 AM Kalle Valo <kvalo@kernel.org> wrote: > > Bartosz Golaszewski <brgl@bgdev.pl> writes: > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > Describe the inputs from the PMU of the ath11k module on WCN6855. > > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > --- > > v1 -> v2: > > - update the example > > > > .../net/wireless/qcom,ath11k-pci.yaml | 29 +++++++++++++++++++ > > 1 file changed, 29 insertions(+) > > This goes to ath-next, not net-next. > > > diff --git a/Documentation/devicetree/bindings/net/wireless/qcom,ath11k-pci.yaml b/Documentation/devicetree/bindings/net/wireless/qcom,ath11k-pci.yaml > > index 8675d7d0215c..a71fdf05bc1e 100644 > > --- a/Documentation/devicetree/bindings/net/wireless/qcom,ath11k-pci.yaml > > +++ b/Documentation/devicetree/bindings/net/wireless/qcom,ath11k-pci.yaml > > @@ -50,6 +50,9 @@ properties: > > vddrfa1p7-supply: > > description: VDD_RFA_1P7 supply regulator handle > > > > + vddrfa1p8-supply: > > + description: VDD_RFA_1P8 supply regulator handle > > + > > vddpcie0p9-supply: > > description: VDD_PCIE_0P9 supply regulator handle > > > > @@ -77,6 +80,22 @@ allOf: > > - vddrfa1p7-supply > > - vddpcie0p9-supply > > - vddpcie1p8-supply > > + - if: > > + properties: > > + compatible: > > + contains: > > + const: pci17cb,1103 > > + then: > > + required: > > + - vddrfacmn-supply > > + - vddaon-supply > > + - vddwlcx-supply > > + - vddwlmx-supply > > + - vddrfa0p8-supply > > + - vddrfa1p2-supply > > + - vddrfa1p8-supply > > + - vddpcie0p9-supply > > + - vddpcie1p8-supply > > Like we discussed before, shouldn't these supplies be optional as not > all modules need them? > The answer is still the same: the ATH11K inside a WCN6855 does - in fact - always need them. The fact that the X13s doesn't define them is bad representation of HW and I'm fixing it in a subsequent DTS patch. Bart > -- > https://patchwork.kernel.org/project/linux-wireless/list/ > > https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
On Fri, Aug 16, 2024 at 11:10 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > On Fri, Aug 16, 2024 at 10:26 AM Kalle Valo <kvalo@kernel.org> wrote: > > > > Bartosz Golaszewski <brgl@bgdev.pl> writes: > > > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > > > Describe the inputs from the PMU of the ath11k module on WCN6855. > > > > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > --- > > > v1 -> v2: > > > - update the example > > > > > > .../net/wireless/qcom,ath11k-pci.yaml | 29 +++++++++++++++++++ > > > 1 file changed, 29 insertions(+) > > > > This goes to ath-next, not net-next. > > > > > diff --git a/Documentation/devicetree/bindings/net/wireless/qcom,ath11k-pci.yaml b/Documentation/devicetree/bindings/net/wireless/qcom,ath11k-pci.yaml > > > index 8675d7d0215c..a71fdf05bc1e 100644 > > > --- a/Documentation/devicetree/bindings/net/wireless/qcom,ath11k-pci.yaml > > > +++ b/Documentation/devicetree/bindings/net/wireless/qcom,ath11k-pci.yaml > > > @@ -50,6 +50,9 @@ properties: > > > vddrfa1p7-supply: > > > description: VDD_RFA_1P7 supply regulator handle > > > > > > + vddrfa1p8-supply: > > > + description: VDD_RFA_1P8 supply regulator handle > > > + > > > vddpcie0p9-supply: > > > description: VDD_PCIE_0P9 supply regulator handle > > > > > > @@ -77,6 +80,22 @@ allOf: > > > - vddrfa1p7-supply > > > - vddpcie0p9-supply > > > - vddpcie1p8-supply > > > + - if: > > > + properties: > > > + compatible: > > > + contains: > > > + const: pci17cb,1103 > > > + then: > > > + required: > > > + - vddrfacmn-supply > > > + - vddaon-supply > > > + - vddwlcx-supply > > > + - vddwlmx-supply > > > + - vddrfa0p8-supply > > > + - vddrfa1p2-supply > > > + - vddrfa1p8-supply > > > + - vddpcie0p9-supply > > > + - vddpcie1p8-supply > > > > Like we discussed before, shouldn't these supplies be optional as not > > all modules need them? > > > > The answer is still the same: the ATH11K inside a WCN6855 does - in > fact - always need them. The fact that the X13s doesn't define them is > bad representation of HW and I'm fixing it in a subsequent DTS patch. > Gentle ping. Bart
Bartosz Golaszewski <brgl@bgdev.pl> writes: >> > + - if: >> > + properties: >> > + compatible: >> > + contains: >> > + const: pci17cb,1103 >> > + then: >> > + required: >> > + - vddrfacmn-supply >> > + - vddaon-supply >> > + - vddwlcx-supply >> > + - vddwlmx-supply >> > + - vddrfa0p8-supply >> > + - vddrfa1p2-supply >> > + - vddrfa1p8-supply >> > + - vddpcie0p9-supply >> > + - vddpcie1p8-supply >> >> Like we discussed before, shouldn't these supplies be optional as not >> all modules need them? >> > > The answer is still the same: the ATH11K inside a WCN6855 does - in > fact - always need them. The fact that the X13s doesn't define them is > bad representation of HW and I'm fixing it in a subsequent DTS patch. But, like we discussed earlier, M.2 boards don't need these so I think this should be optional.
On Thu, Sep 5, 2024 at 5:47 PM Kalle Valo <kvalo@kernel.org> wrote: > > Bartosz Golaszewski <brgl@bgdev.pl> writes: > > >> > + - if: > >> > + properties: > >> > + compatible: > >> > + contains: > >> > + const: pci17cb,1103 > >> > + then: > >> > + required: > >> > + - vddrfacmn-supply > >> > + - vddaon-supply > >> > + - vddwlcx-supply > >> > + - vddwlmx-supply > >> > + - vddrfa0p8-supply > >> > + - vddrfa1p2-supply > >> > + - vddrfa1p8-supply > >> > + - vddpcie0p9-supply > >> > + - vddpcie1p8-supply > >> > >> Like we discussed before, shouldn't these supplies be optional as not > >> all modules need them? > >> > > > > The answer is still the same: the ATH11K inside a WCN6855 does - in > > fact - always need them. The fact that the X13s doesn't define them is > > bad representation of HW and I'm fixing it in a subsequent DTS patch. > > But, like we discussed earlier, M.2 boards don't need these so I think > this should be optional. > If they are truly dynamic, plug-and-play M.2 boards then they shouldn't need any description in device-tree. If they are M.2 sockets that use custom, vendor-specific pins (like what is the case on sc8280xp-crd and X13s) then the HW they carry needs to be described correctly. We've discussed that before. Bart
Bartosz Golaszewski <brgl@bgdev.pl> writes: > On Thu, Sep 5, 2024 at 5:47 PM Kalle Valo <kvalo@kernel.org> wrote: >> >> Bartosz Golaszewski <brgl@bgdev.pl> writes: >> >> >> > + - if: >> >> > + properties: >> >> > + compatible: >> >> > + contains: >> >> > + const: pci17cb,1103 >> >> > + then: >> >> > + required: >> >> > + - vddrfacmn-supply >> >> > + - vddaon-supply >> >> > + - vddwlcx-supply >> >> > + - vddwlmx-supply >> >> > + - vddrfa0p8-supply >> >> > + - vddrfa1p2-supply >> >> > + - vddrfa1p8-supply >> >> > + - vddpcie0p9-supply >> >> > + - vddpcie1p8-supply >> >> >> >> Like we discussed before, shouldn't these supplies be optional as not >> >> all modules need them? >> >> >> > >> > The answer is still the same: the ATH11K inside a WCN6855 does - in >> > fact - always need them. The fact that the X13s doesn't define them is >> > bad representation of HW and I'm fixing it in a subsequent DTS patch. >> >> But, like we discussed earlier, M.2 boards don't need these so I think >> this should be optional. >> > > If they are truly dynamic, plug-and-play M.2 boards then they > shouldn't need any description in device-tree. If they are M.2 sockets > that use custom, vendor-specific pins (like what is the case on > sc8280xp-crd and X13s) then the HW they carry needs to be described > correctly. We've discussed that before. Sigh. Please reread the previous discussion. In some cases we need to set qcom,ath11k-calibration-variant even for M.2 boards.
On 05/09/2024 20:28, Kalle Valo wrote: > Bartosz Golaszewski <brgl@bgdev.pl> writes: > >> On Thu, Sep 5, 2024 at 5:47 PM Kalle Valo <kvalo@kernel.org> wrote: >>> >>> Bartosz Golaszewski <brgl@bgdev.pl> writes: >>> >>>>>> + - if: >>>>>> + properties: >>>>>> + compatible: >>>>>> + contains: >>>>>> + const: pci17cb,1103 >>>>>> + then: >>>>>> + required: >>>>>> + - vddrfacmn-supply >>>>>> + - vddaon-supply >>>>>> + - vddwlcx-supply >>>>>> + - vddwlmx-supply >>>>>> + - vddrfa0p8-supply >>>>>> + - vddrfa1p2-supply >>>>>> + - vddrfa1p8-supply >>>>>> + - vddpcie0p9-supply >>>>>> + - vddpcie1p8-supply >>>>> >>>>> Like we discussed before, shouldn't these supplies be optional as not >>>>> all modules need them? >>>>> >>>> >>>> The answer is still the same: the ATH11K inside a WCN6855 does - in >>>> fact - always need them. The fact that the X13s doesn't define them is >>>> bad representation of HW and I'm fixing it in a subsequent DTS patch. >>> >>> But, like we discussed earlier, M.2 boards don't need these so I think >>> this should be optional. >>> >> >> If they are truly dynamic, plug-and-play M.2 boards then they >> shouldn't need any description in device-tree. If they are M.2 sockets >> that use custom, vendor-specific pins (like what is the case on >> sc8280xp-crd and X13s) then the HW they carry needs to be described >> correctly. We've discussed that before. > > Sigh. Please reread the previous discussion. In some cases we need to > set qcom,ath11k-calibration-variant even for M.2 boards. M.2 cards also have the same power sequencing troubles because of usage of reserved or custom pins. Whether the properties here are required or optional, depends not on the board but on the M.2 card. Therefore I don't understand why you claim it should be optional, just because it is M.2. Best regards, Krzysztof
On Thu, Sep 5, 2024 at 8:28 PM Kalle Valo <kvalo@kernel.org> wrote: > > Bartosz Golaszewski <brgl@bgdev.pl> writes: > > > On Thu, Sep 5, 2024 at 5:47 PM Kalle Valo <kvalo@kernel.org> wrote: > >> > >> Bartosz Golaszewski <brgl@bgdev.pl> writes: > >> > >> >> > + - if: > >> >> > + properties: > >> >> > + compatible: > >> >> > + contains: > >> >> > + const: pci17cb,1103 > >> >> > + then: > >> >> > + required: > >> >> > + - vddrfacmn-supply > >> >> > + - vddaon-supply > >> >> > + - vddwlcx-supply > >> >> > + - vddwlmx-supply > >> >> > + - vddrfa0p8-supply > >> >> > + - vddrfa1p2-supply > >> >> > + - vddrfa1p8-supply > >> >> > + - vddpcie0p9-supply > >> >> > + - vddpcie1p8-supply > >> >> > >> >> Like we discussed before, shouldn't these supplies be optional as not > >> >> all modules need them? > >> >> > >> > > >> > The answer is still the same: the ATH11K inside a WCN6855 does - in > >> > fact - always need them. The fact that the X13s doesn't define them is > >> > bad representation of HW and I'm fixing it in a subsequent DTS patch. > >> > >> But, like we discussed earlier, M.2 boards don't need these so I think > >> this should be optional. > >> > > > > If they are truly dynamic, plug-and-play M.2 boards then they > > shouldn't need any description in device-tree. If they are M.2 sockets > > that use custom, vendor-specific pins (like what is the case on > > sc8280xp-crd and X13s) then the HW they carry needs to be described > > correctly. We've discussed that before. > > Sigh. Please reread the previous discussion. In some cases we need to > set qcom,ath11k-calibration-variant even for M.2 boards. > Maybe instead of posting patronizing comments and forcing me to reiterate all my previous points, you should reread the discussion as well? DT describes hardware and the WNC6855 package is composed of several modules which we represent as separate DT nodes - currently: PMU, WLAN, Bluetooth. The WLAN module takes inputs from the PMU so it *DOES* need the supplies. The fact that you only want to specify the qcom,ath11k-calibration-variant property is irrelevant because the HW is what it is. Device-tree source is not a configuration file - it's a description of hardware. For upstream - if you're using the WCN6855, you must specify the inputs for the WLAN module so it's only fair they be described as "required". For out-of-tree DTS I couldn't care less. You are not correct saying that "M.2 boards don't need these" because as a matter of fact: the WLAN module on your M.2 card takes these inputs from the PMU inside the WCN6855 package. Bartosz
On 9/6/2024 12:44 AM, Bartosz Golaszewski wrote: > For upstream - if you're using the WCN6855, you must specify the > inputs for the WLAN module so it's only fair they be described as > "required". For out-of-tree DTS I couldn't care less. > > You are not correct saying that "M.2 boards don't need these" because > as a matter of fact: the WLAN module on your M.2 card takes these > inputs from the PMU inside the WCN6855 package. Let me start by saying that DT is one area where I'm a newbie, so I hope I can get some education. I'd like to start with an observation: I've used both WCN6855 with ath11k and WCN7850 with ath12k on an x86 laptop without any device tree, so from that perspective none of the device tree stuff is "required" -- these modules "just work". However I also realize that when these are installed on Qualcomm ARM platforms that there are GPIO pins that control things like XO clock, WLAN enable & Bluetooth enable, as well as voltage regulators, and the device is non-functional without those configured, so the device tree items are required in that environment. So just from that perspective saying something is "required" is confusing when there are platforms where it isn't required. And perhaps that is what is confusing Kalle as well? /jeff
On Fri, Sep 6, 2024 at 8:38 PM Jeff Johnson <quic_jjohnson@quicinc.com> wrote: > > On 9/6/2024 12:44 AM, Bartosz Golaszewski wrote: > > For upstream - if you're using the WCN6855, you must specify the > > inputs for the WLAN module so it's only fair they be described as > > "required". For out-of-tree DTS I couldn't care less. > > > > You are not correct saying that "M.2 boards don't need these" because > > as a matter of fact: the WLAN module on your M.2 card takes these > > inputs from the PMU inside the WCN6855 package. > > Let me start by saying that DT is one area where I'm a newbie, so I hope I can > get some education. > > I'd like to start with an observation: I've used both WCN6855 with ath11k and > WCN7850 with ath12k on an x86 laptop without any device tree, so from that > perspective none of the device tree stuff is "required" -- these modules "just > work". > Yes. This is what I refer to as "fully dynamic" M.2 cards, where the card typically has an on-board PMIC that handles the power-up of the device, respecting all timings etc. No custom pins are used. You don't need device-tree. DT bindings don't concern this case. Even it this was an ARM, DT-based platform, you wouldn't need the DT entry. > However I also realize that when these are installed on Qualcomm ARM platforms > that there are GPIO pins that control things like XO clock, WLAN enable & > Bluetooth enable, as well as voltage regulators, and the device is > non-functional without those configured, so the device tree items are required > in that environment. > > So just from that perspective saying something is "required" is confusing when > there are platforms where it isn't required. And perhaps that is what is > confusing Kalle as well? > The properties are required IF you have a DT representation. Because if you're modeling the physical package, this is what it looks like. The one on your "fully dynamic" M.2 card is the same - it also has the same internal inputs and outputs but you're not modeling the external package in the first place so you don't need to care about them. But if you do represent the chipset and not as a black box WCN6855 in its entirety but its WLAN, BT and PMU modules separately then it warrants making the true inputs of the WLAN module mandatory in the schema. Please let me know if this is enough of an explanation. Bart
On Mon, Sep 9, 2024 at 10:19 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > On Fri, Sep 6, 2024 at 8:38 PM Jeff Johnson <quic_jjohnson@quicinc.com> wrote: > > > > On 9/6/2024 12:44 AM, Bartosz Golaszewski wrote: > > > For upstream - if you're using the WCN6855, you must specify the > > > inputs for the WLAN module so it's only fair they be described as > > > "required". For out-of-tree DTS I couldn't care less. > > > > > > You are not correct saying that "M.2 boards don't need these" because > > > as a matter of fact: the WLAN module on your M.2 card takes these > > > inputs from the PMU inside the WCN6855 package. > > > > Let me start by saying that DT is one area where I'm a newbie, so I hope I can > > get some education. > > > > I'd like to start with an observation: I've used both WCN6855 with ath11k and > > WCN7850 with ath12k on an x86 laptop without any device tree, so from that > > perspective none of the device tree stuff is "required" -- these modules "just > > work". > > > > Yes. This is what I refer to as "fully dynamic" M.2 cards, where the > card typically has an on-board PMIC that handles the power-up of the > device, respecting all timings etc. No custom pins are used. You don't > need device-tree. DT bindings don't concern this case. Even it this > was an ARM, DT-based platform, you wouldn't need the DT entry. > > > However I also realize that when these are installed on Qualcomm ARM platforms > > that there are GPIO pins that control things like XO clock, WLAN enable & > > Bluetooth enable, as well as voltage regulators, and the device is > > non-functional without those configured, so the device tree items are required > > in that environment. > > > > So just from that perspective saying something is "required" is confusing when > > there are platforms where it isn't required. And perhaps that is what is > > confusing Kalle as well? > > > > The properties are required IF you have a DT representation. Because > if you're modeling the physical package, this is what it looks like. > The one on your "fully dynamic" M.2 card is the same - it also has the > same internal inputs and outputs but you're not modeling the external > package in the first place so you don't need to care about them. But > if you do represent the chipset and not as a black box WCN6855 in its > entirety but its WLAN, BT and PMU modules separately then it warrants > making the true inputs of the WLAN module mandatory in the schema. > > Please let me know if this is enough of an explanation. > > Bart One more thing to add. You guys worry about existing device-trees, don't, they will continue to work fine. This change only mandates that upstream DT sources model the inputs of the WLAN module and it's only verified by make_dtbs. This doesn't make the code require them. Whatever works for you now, will keep on working. Bart
diff --git a/Documentation/devicetree/bindings/net/wireless/qcom,ath11k-pci.yaml b/Documentation/devicetree/bindings/net/wireless/qcom,ath11k-pci.yaml index 8675d7d0215c..a71fdf05bc1e 100644 --- a/Documentation/devicetree/bindings/net/wireless/qcom,ath11k-pci.yaml +++ b/Documentation/devicetree/bindings/net/wireless/qcom,ath11k-pci.yaml @@ -50,6 +50,9 @@ properties: vddrfa1p7-supply: description: VDD_RFA_1P7 supply regulator handle + vddrfa1p8-supply: + description: VDD_RFA_1P8 supply regulator handle + vddpcie0p9-supply: description: VDD_PCIE_0P9 supply regulator handle @@ -77,6 +80,22 @@ allOf: - vddrfa1p7-supply - vddpcie0p9-supply - vddpcie1p8-supply + - if: + properties: + compatible: + contains: + const: pci17cb,1103 + then: + required: + - vddrfacmn-supply + - vddaon-supply + - vddwlcx-supply + - vddwlmx-supply + - vddrfa0p8-supply + - vddrfa1p2-supply + - vddrfa1p8-supply + - vddpcie0p9-supply + - vddpcie1p8-supply additionalProperties: false @@ -99,6 +118,16 @@ examples: compatible = "pci17cb,1103"; reg = <0x10000 0x0 0x0 0x0 0x0>; + vddrfacmn-supply = <&vreg_pmu_rfa_cmn_0p8>; + vddaon-supply = <&vreg_pmu_aon_0p8>; + vddwlcx-supply = <&vreg_pmu_wlcx_0p8>; + vddwlmx-supply = <&vreg_pmu_wlmx_0p8>; + vddpcie1p8-supply = <&vreg_pmu_pcie_1p8>; + vddpcie0p9-supply = <&vreg_pmu_pcie_0p9>; + vddrfa0p8-supply = <&vreg_pmu_rfa_0p8>; + vddrfa1p2-supply = <&vreg_pmu_rfa_1p2>; + vddrfa1p8-supply = <&vreg_pmu_rfa_1p7>; + qcom,ath11k-calibration-variant = "LE_X13S"; }; };