[PATCH net-next v2] dt-bindings: net: ath11k: document the inputs of the ath11k on WCN6855

[PATCH net-next v2] dt-bindings: net: ath11k: document the inputs of the ath11k on WCN6855

[Bartosz Golaszewski]

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(+)

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";
             };
         };
-- 
2.43.0

Re: [PATCH net-next v2] dt-bindings: net: ath11k: document the inputs of the ath11k on WCN6855

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 335 bytes --]

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>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH net-next v2] dt-bindings: net: ath11k: document the inputs of the ath11k on WCN6855

[Kalle Valo]

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?

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH net-next v2] dt-bindings: net: ath11k: document the inputs of the ath11k on WCN6855

[Bartosz Golaszewski]

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

Re: [PATCH net-next v2] dt-bindings: net: ath11k: document the inputs of the ath11k on WCN6855

[Bartosz Golaszewski]

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

Re: [PATCH net-next v2] dt-bindings: net: ath11k: document the inputs of the ath11k on WCN6855

[Kalle Valo]

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.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH net-next v2] dt-bindings: net: ath11k: document the inputs of the ath11k on WCN6855

[Bartosz Golaszewski]

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

Re: [PATCH net-next v2] dt-bindings: net: ath11k: document the inputs of the ath11k on WCN6855

[Kalle Valo]

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.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH net-next v2] dt-bindings: net: ath11k: document the inputs of the ath11k on WCN6855

[Krzysztof Kozlowski]

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

Re: [PATCH net-next v2] dt-bindings: net: ath11k: document the inputs of the ath11k on WCN6855

[Bartosz Golaszewski]

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

Re: [PATCH net-next v2] dt-bindings: net: ath11k: document the inputs of the ath11k on WCN6855

[Jeff Johnson]

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

Re: [PATCH net-next v2] dt-bindings: net: ath11k: document the inputs of the ath11k on WCN6855

[Bartosz Golaszewski]

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

Re: [PATCH net-next v2] dt-bindings: net: ath11k: document the inputs of the ath11k on WCN6855

[Bartosz Golaszewski]

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

Re: [PATCH net-next v2] dt-bindings: net: ath11k: document the inputs of the ath11k on WCN6855

[Krzysztof Kozlowski]

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.

Therefore to clarify, as DT maintainer this counts as full approval:

Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof

Re: [PATCH net-next v2] dt-bindings: net: ath11k: document the inputs of the ath11k on WCN6855

[Kalle Valo]

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.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH net-next v2] dt-bindings: net: ath11k: document the inputs of the ath11k on WCN6855

[Bartosz Golaszewski]

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

Re: [PATCH net-next v2] dt-bindings: net: ath11k: document the inputs of the ath11k on WCN6855

[Kalle Valo]

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?

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH net-next v2] dt-bindings: net: ath11k: document the inputs of the ath11k on WCN6855

[Bartosz Golaszewski]

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

Re: [PATCH net-next v2] dt-bindings: net: ath11k: document the inputs of the ath11k on WCN6855

[Krzysztof Kozlowski]

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

Re: [PATCH net-next v2] dt-bindings: net: ath11k: document the inputs of the ath11k on WCN6855

[Kalle Valo]

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.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH net-next v2] dt-bindings: net: ath11k: document the inputs of the ath11k on WCN6855

[Bartosz Golaszewski]

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

Re: [PATCH net-next v2] dt-bindings: net: ath11k: document the inputs of the ath11k on WCN6855

[Jeff Johnson]

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";
	};
};

Re: [PATCH net-next v2] dt-bindings: net: ath11k: document the inputs of the ath11k on WCN6855

[Bartosz Golaszewski]

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/

Re: [PATCH net-next v2] dt-bindings: net: ath11k: document the inputs of the ath11k on WCN6855

[Krzysztof Kozlowski]

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

Re: [PATCH net-next v2] dt-bindings: net: ath11k: document the inputs of the ath11k on WCN6855

[Jeff Johnson]

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

Re: [PATCH net-next v2] dt-bindings: net: ath11k: document the inputs of the ath11k on WCN6855

[Bartosz Golaszewski]

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

Re: [PATCH net-next v2] dt-bindings: net: ath11k: document the inputs of the ath11k on WCN6855

[Krzysztof Kozlowski]

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

Re: [PATCH net-next v2] dt-bindings: net: ath11k: document the inputs of the ath11k on WCN6855

[Kalle Valo]

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.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH net-next v2] dt-bindings: net: ath11k: document the inputs of the ath11k on WCN6855

[Krzysztof Kozlowski]

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

Re: [PATCH net-next v2] dt-bindings: net: ath11k: document the inputs of the ath11k on WCN6855

[Kalle Valo]

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

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20240814082301.8091-1-brgl@bgdev.pl/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
https://docs.kernel.org/process/submitting-patches.html