| 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
Krzysztof Kozlowski <krzk@kernel.org> writes: > On 14/08/2024 10:23, 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> >> --- >> v1 -> v2: >> - update the example > > I don't understand why this patch is no being picked up. The code > correct represents the piece of hardware. The supplies should be > required, because this one particular device - the one described in this > binding - cannot work without them. I have already explained the situation. With supplies changed to optional I'm happy take the patch.
On Thu, 19 Sep 2024 09:48:41 +0200, Kalle Valo <kvalo@kernel.org> said: > Krzysztof Kozlowski <krzk@kernel.org> writes: > >> On 14/08/2024 10:23, 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> >>> --- >>> v1 -> v2: >>> - update the example >> >> I don't understand why this patch is no being picked up. The code >> correct represents the piece of hardware. The supplies should be >> required, because this one particular device - the one described in this >> binding - cannot work without them. > > I have already explained the situation. With supplies changed to > optional I'm happy take the patch. > No, silent NAKing and needless stalling is what you're doing. I responded to your last email with extensive clarifications. You're being told by the experts on the subject matter (Krzysztof and Conor) that the change is correct. The change has no functional impact on the driver code. It's also in line with commit 71839a929d9e ("dt-bindings: net: wireless: qcom,ath11k: describe the ath11k on QCA6390") under which we had literally the same discussion and that you ended up picking up after all. Arnd: I've added you here to bring this to your attention because it's somewhat related to what we discussed yesterday. It's a change that is very much SoC-specific, that has trouble getting upstream due to the driver's maintainer unwilingness to accept it. Is this a case where a change to DT bindings should go through the SoC rather than the driver tree? Best Regards, Bartosz Golaszewski
On 19/09/2024 09:48, Kalle Valo wrote: > Krzysztof Kozlowski <krzk@kernel.org> writes: > >> On 14/08/2024 10:23, 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> >>> --- >>> v1 -> v2: >>> - update the example >> >> I don't understand why this patch is no being picked up. The code >> correct represents the piece of hardware. The supplies should be >> required, because this one particular device - the one described in this >> binding - cannot work without them. > > I have already explained the situation. With supplies changed to > optional I'm happy take the patch. You did not provide any relevant argument to this case. Your concerns described quite different case and are no applicable to DT based platforms. Best regards, Krzysztof
Bartosz Golaszewski <brgl@bgdev.pl> writes: > On Thu, 19 Sep 2024 09:48:41 +0200, Kalle Valo <kvalo@kernel.org> said: >> Krzysztof Kozlowski <krzk@kernel.org> writes: >> >>> On 14/08/2024 10:23, 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> >>>> --- >>>> v1 -> v2: >>>> - update the example >>> >>> I don't understand why this patch is no being picked up. The code >>> correct represents the piece of hardware. The supplies should be >>> required, because this one particular device - the one described in this >>> binding - cannot work without them. >> >> I have already explained the situation. With supplies changed to >> optional I'm happy take the patch. >> > > No, silent NAKing and needless stalling is what you're doing. I responded to > your last email with extensive clarifications. You're being told by the > experts on the subject matter (Krzysztof and Conor) that the change is correct. > > The change has no functional impact on the driver code. Until now it was possible to use qcom,ath11k-calibration-variant DT property with M.2 devices. If your patch is applied that's not possible anymore. > It's also in line with commit 71839a929d9e ("dt-bindings: net: > wireless: qcom,ath11k: describe the ath11k on QCA6390") under which we > had literally the same discussion and that you ended up picking up > after all. I don't care about QCA6390 as it's not really used anywhere anymore. I picked up 71839a929d9e, even though I considered it to be wrong, so that your pwrseq subsystem is not delayed. But WCN6855 is a different matter as it's more widely used. > Arnd: I've added you here to bring this to your attention because it's somewhat > related to what we discussed yesterday. It's a change that is very much > SoC-specific, that has trouble getting upstream due to the driver's maintainer > unwilingness to accept it. Is this a case where a change to DT bindings should > go through the SoC rather than the driver tree? Like I have said, I'm happy to take the patch if the supplies are optional. Why can't we do that?
Krzysztof Kozlowski <krzk@kernel.org> writes: > On 19/09/2024 09:48, Kalle Valo wrote: >> Krzysztof Kozlowski <krzk@kernel.org> writes: >> >>> On 14/08/2024 10:23, 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> >>>> --- >>>> v1 -> v2: >>>> - update the example >>> >>> I don't understand why this patch is no being picked up. The code >>> correct represents the piece of hardware. The supplies should be >>> required, because this one particular device - the one described in this >>> binding - cannot work without them. >> >> I have already explained the situation. With supplies changed to >> optional I'm happy take the patch. > > You did not provide any relevant argument to this case. Your concerns > described quite different case and are no applicable to DT based platforms. Ok, I'll try to explain my concerns one more time. I'll try to be thorough so will be a longer mail. In ath11k we have board files, it's basically board/product specific calibration data which is combined with the calibration data from chip's OTP. Choosing the correct board file is essential as otherwise the performance can be bad or the device doesn't work at all. The board files are stored in board-2.bin file in /lib/firmware. ath11k chooses the correct board file based on the information provided by the ath11k firmware and then transfers the board file to firmware. From board-2.bin the correct board file is search based on strings like this: bus=pci,vendor=17cb,device=1103,subsystem-vendor=105b,subsystem-device=e0ca,qmi-chip-id=2,qmi-board-id=255 bus=pci,vendor=17cb,device=1103,subsystem-vendor=105b,subsystem-device=e0ca,qmi-chip-id=2,qmi-board-id=255,variant=HO_BNM But the firmware does not always provide unique enough information for choosing the correct board file and that's why we added the variant property (the second example above). This variant property gives us the means to name the board files uniquely and not have any conflicts. In x86 systems we retrieve it from SMBIOS and in DT systems using qcom,ath11k-calibration-variant property. If WCN6855 supplies are marked as required, it means that we cannot use qcom,ath11k-calibration-variant DT property anymore with WCN6855 M.2 boards. So if we have devices which don't provide unique information then for those devices it's impossible to automatically to choose the correct board file. So based on this, to me the correct solution here is to make the supplies optional so that qcom,ath11k-calibration-variant DT property can continue to be used with WCN6855 M.2 boards.
On Fri, 20 Sep 2024 08:22:16 +0200, Kalle Valo <kvalo@kernel.org> said: > Bartosz Golaszewski <brgl@bgdev.pl> writes: > >> On Thu, 19 Sep 2024 09:48:41 +0200, Kalle Valo <kvalo@kernel.org> said: >>> Krzysztof Kozlowski <krzk@kernel.org> writes: >>> >>>> On 14/08/2024 10:23, 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> >>>>> --- >>>>> v1 -> v2: >>>>> - update the example >>>> >>>> I don't understand why this patch is no being picked up. The code >>>> correct represents the piece of hardware. The supplies should be >>>> required, because this one particular device - the one described in this >>>> binding - cannot work without them. >>> >>> I have already explained the situation. With supplies changed to >>> optional I'm happy take the patch. >>> >> >> No, silent NAKing and needless stalling is what you're doing. I responded to >> your last email with extensive clarifications. You're being told by the >> experts on the subject matter (Krzysztof and Conor) that the change is correct. >> >> The change has no functional impact on the driver code. > > Until now it was possible to use qcom,ath11k-calibration-variant DT > property with M.2 devices. If your patch is applied that's not possible > anymore. > This is incorrect, why do you keep repeating it? What will be impossible is upstreaming DT sources which don't take these supplies - which is what we want. >> It's also in line with commit 71839a929d9e ("dt-bindings: net: >> wireless: qcom,ath11k: describe the ath11k on QCA6390") under which we >> had literally the same discussion and that you ended up picking up >> after all. > > I don't care about QCA6390 as it's not really used anywhere anymore. I > picked up 71839a929d9e, even though I considered it to be wrong, so that > your pwrseq subsystem is not delayed. But WCN6855 is a different matter > as it's more widely used. > In upstream sources, it's only used in X13s and I added a node for it to sc8280xp-crd but that's not upstream yet. Am I missing anything? As I said several times: for out-of-tree DTS, this change does *not* matter. >> Arnd: I've added you here to bring this to your attention because it's somewhat >> related to what we discussed yesterday. It's a change that is very much >> SoC-specific, that has trouble getting upstream due to the driver's maintainer >> unwilingness to accept it. Is this a case where a change to DT bindings should >> go through the SoC rather than the driver tree? > > Like I have said, I'm happy to take the patch if the supplies are > optional. Why can't we do that? > Because this patch reflects the reality of the chipset. And device-tree is supposed to model the reality. It's not there to configure your firmware loader. Bartosz
On Fri, 20 Sep 2024 08:45:56 +0200, Kalle Valo <kvalo@kernel.org> said: > Krzysztof Kozlowski <krzk@kernel.org> writes: > >> On 19/09/2024 09:48, Kalle Valo wrote: >>> Krzysztof Kozlowski <krzk@kernel.org> writes: >>> >>>> On 14/08/2024 10:23, 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> >>>>> --- >>>>> v1 -> v2: >>>>> - update the example >>>> >>>> I don't understand why this patch is no being picked up. The code >>>> correct represents the piece of hardware. The supplies should be >>>> required, because this one particular device - the one described in this >>>> binding - cannot work without them. >>> >>> I have already explained the situation. With supplies changed to >>> optional I'm happy take the patch. >> >> You did not provide any relevant argument to this case. Your concerns >> described quite different case and are no applicable to DT based platforms. > > Ok, I'll try to explain my concerns one more time. I'll try to be > thorough so will be a longer mail. > > In ath11k we have board files, it's basically board/product specific > calibration data which is combined with the calibration data from chip's > OTP. Choosing the correct board file is essential as otherwise the > performance can be bad or the device doesn't work at all. > > The board files are stored in board-2.bin file in /lib/firmware. ath11k > chooses the correct board file based on the information provided by the > ath11k firmware and then transfers the board file to firmware. From > board-2.bin the correct board file is search based on strings like this: > > bus=pci,vendor=17cb,device=1103,subsystem-vendor=105b,subsystem-device=e0ca,qmi-chip-id=2,qmi-board-id=255 > bus=pci,vendor=17cb,device=1103,subsystem-vendor=105b,subsystem-device=e0ca,qmi-chip-id=2,qmi-board-id=255,variant=HO_BNM > > But the firmware does not always provide unique enough information for > choosing the correct board file and that's why we added the variant > property (the second example above). This variant property gives us the > means to name the board files uniquely and not have any conflicts. In > x86 systems we retrieve it from SMBIOS and in DT systems using > qcom,ath11k-calibration-variant property. > No issues here. > If WCN6855 supplies are marked as required, it means that we cannot use > qcom,ath11k-calibration-variant DT property anymore with WCN6855 M.2 > boards. So if we have devices which don't provide unique information > then for those devices it's impossible to automatically to choose the > correct board file. > What you're really trying to say is: we cannot use the following snippet of DTS anymore: &pcie4_port0 { wifi@0 { compatible = "pci17cb,1103"; reg = <0x10000 0x0 0x0 0x0 0x0>; qcom,ath11k-calibration-variant = "LE_X13S"; }; }; First: it's not true. We are not allowed to break existing device-tree sources and a change to the schema has no power to do so anyway. You will however no longer be able to upstream just this as it will not pass make dtbs_check anymore. Second: this bit is incomplete even if the WCN6855 package is on a detachable M.2 card. When a DT property is defined as optional in schema, it doesn't mean: "the driver will work fine without it". It means: "the *hardware* does not actually need it to function". That's a huge difference. DTS is not a configuration file for your convenience. > So based on this, to me the correct solution here is to make the > supplies optional so that qcom,ath11k-calibration-variant DT property > can continue to be used with WCN6855 M.2 boards. > No, this is the convenient solution. The *correct* solution is to say how the ath11k inside the WCN6855 package is really supplied. The dt-bindings should define the correct representation, not the convenient one. Let me give you an analogy: we don't really need to have always-on, fixed regulators in DTS. The drivers don't really need them. We do it for completeness of the HW description. Bartosz
On 9/20/2024 1:22 AM, Bartosz Golaszewski wrote: > On Fri, 20 Sep 2024 08:45:56 +0200, Kalle Valo <kvalo@kernel.org> said: >> Krzysztof Kozlowski <krzk@kernel.org> writes: >> >>> On 19/09/2024 09:48, Kalle Valo wrote: >>>> Krzysztof Kozlowski <krzk@kernel.org> writes: >>>> >>>>> On 14/08/2024 10:23, 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> >>>>>> --- >>>>>> v1 -> v2: >>>>>> - update the example >>>>> >>>>> I don't understand why this patch is no being picked up. The code >>>>> correct represents the piece of hardware. The supplies should be >>>>> required, because this one particular device - the one described in this >>>>> binding - cannot work without them. >>>> >>>> I have already explained the situation. With supplies changed to >>>> optional I'm happy take the patch. >>> >>> You did not provide any relevant argument to this case. Your concerns >>> described quite different case and are no applicable to DT based platforms. >> >> Ok, I'll try to explain my concerns one more time. I'll try to be >> thorough so will be a longer mail. >> >> In ath11k we have board files, it's basically board/product specific >> calibration data which is combined with the calibration data from chip's >> OTP. Choosing the correct board file is essential as otherwise the >> performance can be bad or the device doesn't work at all. >> >> The board files are stored in board-2.bin file in /lib/firmware. ath11k >> chooses the correct board file based on the information provided by the >> ath11k firmware and then transfers the board file to firmware. From >> board-2.bin the correct board file is search based on strings like this: >> >> bus=pci,vendor=17cb,device=1103,subsystem-vendor=105b,subsystem-device=e0ca,qmi-chip-id=2,qmi-board-id=255 >> bus=pci,vendor=17cb,device=1103,subsystem-vendor=105b,subsystem-device=e0ca,qmi-chip-id=2,qmi-board-id=255,variant=HO_BNM >> >> But the firmware does not always provide unique enough information for >> choosing the correct board file and that's why we added the variant >> property (the second example above). This variant property gives us the >> means to name the board files uniquely and not have any conflicts. In >> x86 systems we retrieve it from SMBIOS and in DT systems using >> qcom,ath11k-calibration-variant property. >> > > No issues here. > >> If WCN6855 supplies are marked as required, it means that we cannot use >> qcom,ath11k-calibration-variant DT property anymore with WCN6855 M.2 >> boards. So if we have devices which don't provide unique information >> then for those devices it's impossible to automatically to choose the >> correct board file. >> > > What you're really trying to say is: we cannot use the following snippet of > DTS anymore: > > &pcie4_port0 { > wifi@0 { > compatible = "pci17cb,1103"; > reg = <0x10000 0x0 0x0 0x0 0x0>; > > qcom,ath11k-calibration-variant = "LE_X13S"; > }; > }; > > First: it's not true. We are not allowed to break existing device-tree sources > and a change to the schema has no power to do so anyway. You will however no > longer be able to upstream just this as it will not pass make dtbs_check > anymore. > > Second: this bit is incomplete even if the WCN6855 package is on a detachable > M.2 card. When a DT property is defined as optional in schema, it doesn't > mean: "the driver will work fine without it". It means: "the *hardware* does > not actually need it to function". That's a huge difference. DTS is not a > configuration file for your convenience. > >> So based on this, to me the correct solution here is to make the >> supplies optional so that qcom,ath11k-calibration-variant DT property >> can continue to be used with WCN6855 M.2 boards. >> > > No, this is the convenient solution. The *correct* solution is to say how the > ath11k inside the WCN6855 package is really supplied. The dt-bindings should > define the correct representation, not the convenient one. > > Let me give you an analogy: we don't really need to have always-on, fixed > regulators in DTS. The drivers don't really need them. We do it for > completeness of the HW description. Again, since I'm a DT n00b: Just to make sure I understand, you are saying that with this change any existing .dts/.dtb files will still work with an updated driver, so the new properties are not required to be populated on existing devices. However a new driver with support for these properties will utilize them when they are present, and the current ath11k .dts files will need to be updated to include these properties for pci17cb,1103, i.e. the following needs updating: arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts &pcie4_port0 { wifi@0 { compatible = "pci17cb,1103"; reg = <0x10000 0x0 0x0 0x0 0x0>; qcom,ath11k-calibration-variant = "LE_X13S"; }; };
On Fri, Sep 20, 2024 at 11:02 PM Jeff Johnson <quic_jjohnson@quicinc.com> wrote: > > > > > Let me give you an analogy: we don't really need to have always-on, fixed > > regulators in DTS. The drivers don't really need them. We do it for > > completeness of the HW description. > > Again, since I'm a DT n00b: > Just to make sure I understand, you are saying that with this change any > existing .dts/.dtb files will still work with an updated driver, so the new > properties are not required to be populated on existing devices. > There are no driver updates. No functional change. > However a new driver with support for these properties will utilize them when > they are present, and the current ath11k .dts files will need to be updated to > include these properties for pci17cb,1103, i.e. the following needs updating: > arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts What new driver? The dts is being updated in a separate series[1]. It makes this platform use the new power sequencing subsystem for wcn6855. All other changes required to make it work are already upstream. There's no change to ath11k. Bart [1] https://lore.kernel.org/all/20240905122023.47251-1-brgl@bgdev.pl/
On 20/09/2024 08:45, Kalle Valo wrote: > Krzysztof Kozlowski <krzk@kernel.org> writes: > >> On 19/09/2024 09:48, Kalle Valo wrote: >>> Krzysztof Kozlowski <krzk@kernel.org> writes: >>> >>>> On 14/08/2024 10:23, 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> >>>>> --- >>>>> v1 -> v2: >>>>> - update the example >>>> >>>> I don't understand why this patch is no being picked up. The code >>>> correct represents the piece of hardware. The supplies should be >>>> required, because this one particular device - the one described in this >>>> binding - cannot work without them. >>> >>> I have already explained the situation. With supplies changed to >>> optional I'm happy take the patch. >> >> You did not provide any relevant argument to this case. Your concerns >> described quite different case and are no applicable to DT based platforms. > > Ok, I'll try to explain my concerns one more time. I'll try to be > thorough so will be a longer mail. > > In ath11k we have board files, it's basically board/product specific > calibration data which is combined with the calibration data from chip's > OTP. Choosing the correct board file is essential as otherwise the > performance can be bad or the device doesn't work at all. > > The board files are stored in board-2.bin file in /lib/firmware. ath11k > chooses the correct board file based on the information provided by the > ath11k firmware and then transfers the board file to firmware. From > board-2.bin the correct board file is search based on strings like this: > > bus=pci,vendor=17cb,device=1103,subsystem-vendor=105b,subsystem-device=e0ca,qmi-chip-id=2,qmi-board-id=255 > bus=pci,vendor=17cb,device=1103,subsystem-vendor=105b,subsystem-device=e0ca,qmi-chip-id=2,qmi-board-id=255,variant=HO_BNM > > But the firmware does not always provide unique enough information for > choosing the correct board file and that's why we added the variant > property (the second example above). This variant property gives us the > means to name the board files uniquely and not have any conflicts. In > x86 systems we retrieve it from SMBIOS and in DT systems using > qcom,ath11k-calibration-variant property. > > If WCN6855 supplies are marked as required, it means that we cannot use > qcom,ath11k-calibration-variant DT property anymore with WCN6855 M.2 > boards. So if we have devices which don't provide unique information No, it does not mean that. > then for those devices it's impossible to automatically to choose the > correct board file. Anyway, only this device must be fully described, because you cannot have pci17cb,1103 without these supplies. It's just electrically not possible, according to our investigation. > > So based on this, to me the correct solution here is to make the > supplies optional so that qcom,ath11k-calibration-variant DT property > can continue to be used with WCN6855 M.2 boards. WCN6855 can still do whatever they want. They are not restricted, not limited. pci17cb,1103 must provide suppplies, because pci17cb,1103 cannot work without them. Best regards, Krzysztof
On 20/09/2024 23:02, Jeff Johnson wrote: > On 9/20/2024 1:22 AM, Bartosz Golaszewski wrote: >> On Fri, 20 Sep 2024 08:45:56 +0200, Kalle Valo <kvalo@kernel.org> said: >>> Krzysztof Kozlowski <krzk@kernel.org> writes: >>> >>>> On 19/09/2024 09:48, Kalle Valo wrote: >>>>> Krzysztof Kozlowski <krzk@kernel.org> writes: >>>>> >>>>>> On 14/08/2024 10:23, 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> >>>>>>> --- >>>>>>> v1 -> v2: >>>>>>> - update the example >>>>>> >>>>>> I don't understand why this patch is no being picked up. The code >>>>>> correct represents the piece of hardware. The supplies should be >>>>>> required, because this one particular device - the one described in this >>>>>> binding - cannot work without them. >>>>> >>>>> I have already explained the situation. With supplies changed to >>>>> optional I'm happy take the patch. >>>> >>>> You did not provide any relevant argument to this case. Your concerns >>>> described quite different case and are no applicable to DT based platforms. >>> >>> Ok, I'll try to explain my concerns one more time. I'll try to be >>> thorough so will be a longer mail. >>> >>> In ath11k we have board files, it's basically board/product specific >>> calibration data which is combined with the calibration data from chip's >>> OTP. Choosing the correct board file is essential as otherwise the >>> performance can be bad or the device doesn't work at all. >>> >>> The board files are stored in board-2.bin file in /lib/firmware. ath11k >>> chooses the correct board file based on the information provided by the >>> ath11k firmware and then transfers the board file to firmware. From >>> board-2.bin the correct board file is search based on strings like this: >>> >>> bus=pci,vendor=17cb,device=1103,subsystem-vendor=105b,subsystem-device=e0ca,qmi-chip-id=2,qmi-board-id=255 >>> bus=pci,vendor=17cb,device=1103,subsystem-vendor=105b,subsystem-device=e0ca,qmi-chip-id=2,qmi-board-id=255,variant=HO_BNM >>> >>> But the firmware does not always provide unique enough information for >>> choosing the correct board file and that's why we added the variant >>> property (the second example above). This variant property gives us the >>> means to name the board files uniquely and not have any conflicts. In >>> x86 systems we retrieve it from SMBIOS and in DT systems using >>> qcom,ath11k-calibration-variant property. >>> >> >> No issues here. >> >>> If WCN6855 supplies are marked as required, it means that we cannot use >>> qcom,ath11k-calibration-variant DT property anymore with WCN6855 M.2 >>> boards. So if we have devices which don't provide unique information >>> then for those devices it's impossible to automatically to choose the >>> correct board file. >>> >> >> What you're really trying to say is: we cannot use the following snippet of >> DTS anymore: >> >> &pcie4_port0 { >> wifi@0 { >> compatible = "pci17cb,1103"; >> reg = <0x10000 0x0 0x0 0x0 0x0>; >> >> qcom,ath11k-calibration-variant = "LE_X13S"; >> }; >> }; >> >> First: it's not true. We are not allowed to break existing device-tree sources >> and a change to the schema has no power to do so anyway. You will however no >> longer be able to upstream just this as it will not pass make dtbs_check >> anymore. >> >> Second: this bit is incomplete even if the WCN6855 package is on a detachable >> M.2 card. When a DT property is defined as optional in schema, it doesn't >> mean: "the driver will work fine without it". It means: "the *hardware* does >> not actually need it to function". That's a huge difference. DTS is not a >> configuration file for your convenience. >> >>> So based on this, to me the correct solution here is to make the >>> supplies optional so that qcom,ath11k-calibration-variant DT property >>> can continue to be used with WCN6855 M.2 boards. >>> >> >> No, this is the convenient solution. The *correct* solution is to say how the >> ath11k inside the WCN6855 package is really supplied. The dt-bindings should >> define the correct representation, not the convenient one. >> >> Let me give you an analogy: we don't really need to have always-on, fixed >> regulators in DTS. The drivers don't really need them. We do it for >> completeness of the HW description. > > Again, since I'm a DT n00b: > Just to make sure I understand, you are saying that with this change any > existing .dts/.dtb files will still work with an updated driver, so the new > properties are not required to be populated on existing devices. Did you folks rejected patches acked by DT maintainers on basis of not understanding DT at all? > > However a new driver with support for these properties will utilize them when > they are present, and the current ath11k .dts files will need to be updated to It is not related to drivers at all. Nothing in this thread is related to drivers. Can we entirely drop the drivers from the discussion? > include these properties for pci17cb,1103, i.e. the following needs updating: > arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts > &pcie4_port0 { > wifi@0 { > compatible = "pci17cb,1103"; > reg = <0x10000 0x0 0x0 0x0 0x0>; > > qcom,ath11k-calibration-variant = "LE_X13S"; > }; > }; > > Best regards, Krzysztof
On 9/24/2024 1:06 AM, Krzysztof Kozlowski wrote: > On 20/09/2024 23:02, Jeff Johnson wrote: >> Again, since I'm a DT n00b: >> Just to make sure I understand, you are saying that with this change any >> existing .dts/.dtb files will still work with an updated driver, so the new >> properties are not required to be populated on existing devices. > > Did you folks rejected patches acked by DT maintainers on basis of not > understanding DT at all? I personally have not rejected any DT patches. Nor have I accepted any. I'm deferring to Kalle. >> However a new driver with support for these properties will utilize them when >> they are present, and the current ath11k .dts files will need to be updated to > > It is not related to drivers at all. Nothing in this thread is related > to drivers. > > Can we entirely drop the drivers from the discussion? I brought up drivers since in the discussion there was concern that this DT change would potentially break existing devices that have a DTS that defines qcom,ath11k-calibration-variant, and I wanted clarification on that point. /jeff
On Tue, Sep 24, 2024 at 6:46 PM Jeff Johnson <quic_jjohnson@quicinc.com> wrote: > > On 9/24/2024 1:06 AM, Krzysztof Kozlowski wrote: > > On 20/09/2024 23:02, Jeff Johnson wrote: > >> Again, since I'm a DT n00b: > >> Just to make sure I understand, you are saying that with this change any > >> existing .dts/.dtb files will still work with an updated driver, so the new > >> properties are not required to be populated on existing devices. > > > > Did you folks rejected patches acked by DT maintainers on basis of not > > understanding DT at all? > > I personally have not rejected any DT patches. Nor have I accepted any. > I'm deferring to Kalle. > > >> However a new driver with support for these properties will utilize them when > >> they are present, and the current ath11k .dts files will need to be updated to > > > > It is not related to drivers at all. Nothing in this thread is related > > to drivers. > > > > Can we entirely drop the drivers from the discussion? > > I brought up drivers since in the discussion there was concern that this DT > change would potentially break existing devices that have a DTS that defines > qcom,ath11k-calibration-variant, and I wanted clarification on that point. > > /jeff How could this happen? DT schema is used to validate device-tree sources but is not used by the kernel code in any way. I've said it several times over the course of this and the previous (qca6390) discussion. Bartosz
Krzysztof Kozlowski <krzk@kernel.org> writes: > On 20/09/2024 08:45, Kalle Valo wrote: > >> Krzysztof Kozlowski <krzk@kernel.org> writes: >> >>> On 19/09/2024 09:48, Kalle Valo wrote: >>>> Krzysztof Kozlowski <krzk@kernel.org> writes: >>>> >>>>> On 14/08/2024 10:23, 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> >>>>>> --- >>>>>> v1 -> v2: >>>>>> - update the example >>>>> >>>>> I don't understand why this patch is no being picked up. The code >>>>> correct represents the piece of hardware. The supplies should be >>>>> required, because this one particular device - the one described in this >>>>> binding - cannot work without them. >>>> >>>> I have already explained the situation. With supplies changed to >>>> optional I'm happy take the patch. >>> >>> You did not provide any relevant argument to this case. Your concerns >>> described quite different case and are no applicable to DT based platforms. >> >> Ok, I'll try to explain my concerns one more time. I'll try to be >> thorough so will be a longer mail. >> >> In ath11k we have board files, it's basically board/product specific >> calibration data which is combined with the calibration data from chip's >> OTP. Choosing the correct board file is essential as otherwise the >> performance can be bad or the device doesn't work at all. >> >> The board files are stored in board-2.bin file in /lib/firmware. ath11k >> chooses the correct board file based on the information provided by the >> ath11k firmware and then transfers the board file to firmware. From >> board-2.bin the correct board file is search based on strings like this: >> >> bus=pci,vendor=17cb,device=1103,subsystem-vendor=105b,subsystem-device=e0ca,qmi-chip-id=2,qmi-board-id=255 >> bus=pci,vendor=17cb,device=1103,subsystem-vendor=105b,subsystem-device=e0ca,qmi-chip-id=2,qmi-board-id=255,variant=HO_BNM >> >> But the firmware does not always provide unique enough information for >> choosing the correct board file and that's why we added the variant >> property (the second example above). This variant property gives us the >> means to name the board files uniquely and not have any conflicts. In >> x86 systems we retrieve it from SMBIOS and in DT systems using >> qcom,ath11k-calibration-variant property. >> >> If WCN6855 supplies are marked as required, it means that we cannot use >> qcom,ath11k-calibration-variant DT property anymore with WCN6855 M.2 >> boards. So if we have devices which don't provide unique information > > No, it does not mean that. > >> then for those devices it's impossible to automatically to choose the >> correct board file. > > Anyway, only this device must be fully described, because you cannot > have pci17cb,1103 without these supplies. It's just electrically not > possible, according to our investigation. Yeah, you have been telling that all along. But on the contrary I have WCN6855 (pci17cb,1103) M.2 board which I installed to my NUC and they haven't needed any supplies (unless BIOS does something automatically). Also I have QCA6390 and WCN7850 M.2 boards, both which you claim needs the supplies, and they just work out-of-box as well. So I have a hard time trusting your spec and believing it's the ultimate authority. To me if reality and spec doesn't match, reality wins. >> So based on this, to me the correct solution here is to make the >> supplies optional so that qcom,ath11k-calibration-variant DT property >> can continue to be used with WCN6855 M.2 boards. > > WCN6855 can still do whatever they want. They are not restricted, not > limited. pci17cb,1103 must provide suppplies, because pci17cb,1103 > cannot work without them. Claiming that WCN6885 can still do whatever they want is confusing to me because WCN6855 is pci17cb,1103, there are no other ids. See ath11k/pci.c: #define WCN6855_DEVICE_ID 0x1103 { PCI_VDEVICE(QCOM, WCN6855_DEVICE_ID) }, But this discussion is going circles and honestly is waste of time. I don't think the patch is right but I'll apply it anyway, let's deal with the problems if/when they come up.
On 25/09/2024 07:58, Kalle Valo wrote: > Krzysztof Kozlowski <krzk@kernel.org> writes: > >> On 20/09/2024 08:45, Kalle Valo wrote: >> >>> Krzysztof Kozlowski <krzk@kernel.org> writes: >>> >>>> On 19/09/2024 09:48, Kalle Valo wrote: >>>>> Krzysztof Kozlowski <krzk@kernel.org> writes: >>>>> >>>>>> On 14/08/2024 10:23, 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> >>>>>>> --- >>>>>>> v1 -> v2: >>>>>>> - update the example >>>>>> >>>>>> I don't understand why this patch is no being picked up. The code >>>>>> correct represents the piece of hardware. The supplies should be >>>>>> required, because this one particular device - the one described in this >>>>>> binding - cannot work without them. >>>>> >>>>> I have already explained the situation. With supplies changed to >>>>> optional I'm happy take the patch. >>>> >>>> You did not provide any relevant argument to this case. Your concerns >>>> described quite different case and are no applicable to DT based platforms. >>> >>> Ok, I'll try to explain my concerns one more time. I'll try to be >>> thorough so will be a longer mail. >>> >>> In ath11k we have board files, it's basically board/product specific >>> calibration data which is combined with the calibration data from chip's >>> OTP. Choosing the correct board file is essential as otherwise the >>> performance can be bad or the device doesn't work at all. >>> >>> The board files are stored in board-2.bin file in /lib/firmware. ath11k >>> chooses the correct board file based on the information provided by the >>> ath11k firmware and then transfers the board file to firmware. From >>> board-2.bin the correct board file is search based on strings like this: >>> >>> bus=pci,vendor=17cb,device=1103,subsystem-vendor=105b,subsystem-device=e0ca,qmi-chip-id=2,qmi-board-id=255 >>> bus=pci,vendor=17cb,device=1103,subsystem-vendor=105b,subsystem-device=e0ca,qmi-chip-id=2,qmi-board-id=255,variant=HO_BNM >>> >>> But the firmware does not always provide unique enough information for >>> choosing the correct board file and that's why we added the variant >>> property (the second example above). This variant property gives us the >>> means to name the board files uniquely and not have any conflicts. In >>> x86 systems we retrieve it from SMBIOS and in DT systems using >>> qcom,ath11k-calibration-variant property. >>> >>> If WCN6855 supplies are marked as required, it means that we cannot use >>> qcom,ath11k-calibration-variant DT property anymore with WCN6855 M.2 >>> boards. So if we have devices which don't provide unique information >> >> No, it does not mean that. >> >>> then for those devices it's impossible to automatically to choose the >>> correct board file. >> >> Anyway, only this device must be fully described, because you cannot >> have pci17cb,1103 without these supplies. It's just electrically not >> possible, according to our investigation. > > Yeah, you have been telling that all along. But on the contrary I have > WCN6855 (pci17cb,1103) M.2 board which I installed to my NUC and they > haven't needed any supplies (unless BIOS does something automatically). That's a ACPI system, so ACPI and complex drivers handle this. > Also I have QCA6390 and WCN7850 M.2 boards, both which you claim needs > the supplies, and they just work out-of-box as well. So I have a hard On DT platform they work by coincidence or "work mostly". Ppwer sequencing work fixes it. > time trusting your spec and believing it's the ultimate authority. To me > if reality and spec doesn't match, reality wins. You miss concepts of DT. Really. You can skip in DT many regulator supplies, which are enabled by default (e.g. by bootloader). And what? Everything will work fine even if Linux kernel requires them. Driver will probe fine, hardware "will work". Still the regulators *are required* by DT rules and we have been telling you this so many times already. That's not a difference case here. > >>> So based on this, to me the correct solution here is to make the >>> supplies optional so that qcom,ath11k-calibration-variant DT property >>> can continue to be used with WCN6855 M.2 boards. >> >> WCN6855 can still do whatever they want. They are not restricted, not >> limited. pci17cb,1103 must provide suppplies, because pci17cb,1103 >> cannot work without them. > > Claiming that WCN6885 can still do whatever they want is confusing to me > because WCN6855 is pci17cb,1103, there are no other ids. See > ath11k/pci.c: > > #define WCN6855_DEVICE_ID 0x1103 > > { PCI_VDEVICE(QCOM, WCN6855_DEVICE_ID) }, > > But this discussion is going circles and honestly is waste of time. I > don't think the patch is right but I'll apply it anyway, let's deal with > the problems if/when they come up. For me, there could be different WCN6885 chips, flavors or PCI cards. The binding speaks only about given PCI device. Best regards, Krzysztof
Bartosz Golaszewski <brgl@bgdev.pl> wrote: > 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> > Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org> > Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com> Patch applied to ath-next branch of ath.git, thanks. 02d697272cc6 dt-bindings: net: ath11k: document the inputs of the ath11k on WCN6855
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"; }; };