[PATCH 0/4] Add WCN3680B WiFi/Bluetooth and enable it on Fairphone 3

[PATCH 0/4] Add WCN3680B WiFi/Bluetooth and enable it on Fairphone 3

[Luca Weiss]

Add a new compatible for the WCN3680B found together with some Qualcomm
SoCs. And enable this WiFi/Bluetooth combo on Fairphone 3 smartphone.

Worth noting that I'm pretty sure some phones already in mainline do
have this WCN3680B chip enabled but are using a different compatible,
e.g. qcom,wcn3680 (without the b suffix).

Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
---
Luca Weiss (4):
      dt-bindings: remoteproc: qcom: wcnss: Add WCN3680B compatible
      remoteproc: qcom_wcnss: Add WCN3680B compatible
      wifi: wcn36xx: Add check for WCN3680B
      arm64: dts: qcom: sdm632-fairphone-fp3: Enable WiFi/Bluetooth

 .../devicetree/bindings/remoteproc/qcom,wcnss-pil.yaml    |  1 +
 arch/arm64/boot/dts/qcom/sdm632-fairphone-fp3.dts         | 15 +++++++++++++++
 drivers/net/wireless/ath/wcn36xx/main.c                   |  3 ++-
 drivers/remoteproc/qcom_wcnss_iris.c                      |  1 +
 4 files changed, 19 insertions(+), 1 deletion(-)
---
base-commit: 09eda82818c490a6fb24f1374bd704ea5c3b577a
change-id: 20231015-fp3-wcnss-23e83b8235f5

Best regards,
-- 
Luca Weiss <luca@z3ntu.xyz>

[PATCH 1/4] dt-bindings: remoteproc: qcom: wcnss: Add WCN3680B compatible

[Luca Weiss]

Add a compatible for the iris subnode in the WCNSS PIL.

Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
---
 Documentation/devicetree/bindings/remoteproc/qcom,wcnss-pil.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,wcnss-pil.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,wcnss-pil.yaml
index 45eb42bd3c2c..0e5e0b7a0610 100644
--- a/Documentation/devicetree/bindings/remoteproc/qcom,wcnss-pil.yaml
+++ b/Documentation/devicetree/bindings/remoteproc/qcom,wcnss-pil.yaml
@@ -111,6 +111,7 @@ properties:
           - qcom,wcn3660
           - qcom,wcn3660b
           - qcom,wcn3680
+          - qcom,wcn3680b
 
       clocks:
         minItems: 1

-- 
2.42.0

[PATCH 2/4] remoteproc: qcom_wcnss: Add WCN3680B compatible

[Luca Weiss]

Add a compatible for the WCN3680B chip used with some Qualcomm SoCs.

It shares the same regulator setup as WCN3680, so we can reuse the
driver data for that.

Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
---
 drivers/remoteproc/qcom_wcnss_iris.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/remoteproc/qcom_wcnss_iris.c b/drivers/remoteproc/qcom_wcnss_iris.c
index dd36fd077911..22acc609105f 100644
--- a/drivers/remoteproc/qcom_wcnss_iris.c
+++ b/drivers/remoteproc/qcom_wcnss_iris.c
@@ -99,6 +99,7 @@ static const struct of_device_id iris_of_match[] = {
 	{ .compatible = "qcom,wcn3660", .data = &wcn3660_data },
 	{ .compatible = "qcom,wcn3660b", .data = &wcn3680_data },
 	{ .compatible = "qcom,wcn3680", .data = &wcn3680_data },
+	{ .compatible = "qcom,wcn3680b", .data = &wcn3680_data },
 	{}
 };
 

-- 
2.42.0

[PATCH 3/4] wifi: wcn36xx: Add check for WCN3680B

[Luca Weiss]

Add a check for the WCN3680B compatible next to the WCN3680 compatible.

Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
---
 drivers/net/wireless/ath/wcn36xx/main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/wcn36xx/main.c b/drivers/net/wireless/ath/wcn36xx/main.c
index 2bd1163177f0..74b0b7074760 100644
--- a/drivers/net/wireless/ath/wcn36xx/main.c
+++ b/drivers/net/wireless/ath/wcn36xx/main.c
@@ -1535,7 +1535,8 @@ static int wcn36xx_platform_get_resources(struct wcn36xx *wcn,
 		if (of_device_is_compatible(iris_node, "qcom,wcn3660") ||
 		    of_device_is_compatible(iris_node, "qcom,wcn3660b"))
 			wcn->rf_id = RF_IRIS_WCN3660;
-		if (of_device_is_compatible(iris_node, "qcom,wcn3680"))
+		if (of_device_is_compatible(iris_node, "qcom,wcn3680") ||
+		    of_device_is_compatible(iris_node, "qcom,wcn3680b"))
 			wcn->rf_id = RF_IRIS_WCN3680;
 		of_node_put(iris_node);
 	}

-- 
2.42.0

[PATCH 4/4] arm64: dts: qcom: sdm632-fairphone-fp3: Enable WiFi/Bluetooth

[Luca Weiss]

Configure and enable the WCNSS which provides WiFi and Bluetooth on this
device.

Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
---
 arch/arm64/boot/dts/qcom/sdm632-fairphone-fp3.dts | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm632-fairphone-fp3.dts b/arch/arm64/boot/dts/qcom/sdm632-fairphone-fp3.dts
index 301eca9a4f31..addf56fb7937 100644
--- a/arch/arm64/boot/dts/qcom/sdm632-fairphone-fp3.dts
+++ b/arch/arm64/boot/dts/qcom/sdm632-fairphone-fp3.dts
@@ -239,3 +239,18 @@ &usb3 {
 &usb3_dwc3 {
 	dr_mode = "peripheral";
 };
+
+&wcnss {
+	status = "okay";
+
+	vddpx-supply = <&pm8953_l5>;
+};
+
+&wcnss_iris {
+	compatible = "qcom,wcn3680b";
+
+	vddxo-supply = <&pm8953_l7>;
+	vddrfa-supply = <&pm8953_l19>;
+	vddpa-supply = <&pm8953_l9>;
+	vdddig-supply = <&pm8953_l5>;
+};

-- 
2.42.0

Re: [PATCH 1/4] dt-bindings: remoteproc: qcom: wcnss: Add WCN3680B compatible

[Krzysztof Kozlowski]

On 15/10/2023 22:03, Luca Weiss wrote:
> Add a compatible for the iris subnode in the WCNSS PIL.
> 
> Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
> ---
>  Documentation/devicetree/bindings/remoteproc/qcom,wcnss-pil.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,wcnss-pil.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,wcnss-pil.yaml
> index 45eb42bd3c2c..0e5e0b7a0610 100644
> --- a/Documentation/devicetree/bindings/remoteproc/qcom,wcnss-pil.yaml
> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,wcnss-pil.yaml
> @@ -111,6 +111,7 @@ properties:
>            - qcom,wcn3660
>            - qcom,wcn3660b
>            - qcom,wcn3680
> +          - qcom,wcn3680b

Looks like this should be made as compatible with qcom,wcn3680 (so with
fallback).

Best regards,
Krzysztof

Re: [PATCH 2/4] remoteproc: qcom_wcnss: Add WCN3680B compatible

[Krzysztof Kozlowski]

On 15/10/2023 22:03, Luca Weiss wrote:
> Add a compatible for the WCN3680B chip used with some Qualcomm SoCs.
> 
> It shares the same regulator setup as WCN3680, so we can reuse the
> driver data for that.
> 
> Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
> ---
>  drivers/remoteproc/qcom_wcnss_iris.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/remoteproc/qcom_wcnss_iris.c b/drivers/remoteproc/qcom_wcnss_iris.c
> index dd36fd077911..22acc609105f 100644
> --- a/drivers/remoteproc/qcom_wcnss_iris.c
> +++ b/drivers/remoteproc/qcom_wcnss_iris.c
> @@ -99,6 +99,7 @@ static const struct of_device_id iris_of_match[] = {
>  	{ .compatible = "qcom,wcn3660", .data = &wcn3660_data },
>  	{ .compatible = "qcom,wcn3660b", .data = &wcn3680_data },
>  	{ .compatible = "qcom,wcn3680", .data = &wcn3680_data },
> +	{ .compatible = "qcom,wcn3680b", .data = &wcn3680_data },

Just make devices compatible and no need for this driver change.

Best regards,
Krzysztof

Re: [PATCH 3/4] wifi: wcn36xx: Add check for WCN3680B

[Krzysztof Kozlowski]

On 15/10/2023 22:03, Luca Weiss wrote:
> Add a check for the WCN3680B compatible next to the WCN3680 compatible.
> 
> Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
> ---
>  drivers/net/wireless/ath/wcn36xx/main.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/ath/wcn36xx/main.c b/drivers/net/wireless/ath/wcn36xx/main.c
> index 2bd1163177f0..74b0b7074760 100644
> --- a/drivers/net/wireless/ath/wcn36xx/main.c
> +++ b/drivers/net/wireless/ath/wcn36xx/main.c
> @@ -1535,7 +1535,8 @@ static int wcn36xx_platform_get_resources(struct wcn36xx *wcn,
>  		if (of_device_is_compatible(iris_node, "qcom,wcn3660") ||
>  		    of_device_is_compatible(iris_node, "qcom,wcn3660b"))
>  			wcn->rf_id = RF_IRIS_WCN3660;
> -		if (of_device_is_compatible(iris_node, "qcom,wcn3680"))
> +		if (of_device_is_compatible(iris_node, "qcom,wcn3680") ||
> +		    of_device_is_compatible(iris_node, "qcom,wcn3680b"))

Same comment

Best regards,
Krzysztof

Re: [PATCH 1/4] dt-bindings: remoteproc: qcom: wcnss: Add WCN3680B compatible

[Loic Poulain]

On Mon, 16 Oct 2023 at 07:35, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 15/10/2023 22:03, Luca Weiss wrote:
> > Add a compatible for the iris subnode in the WCNSS PIL.
> >
> > Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
> > ---
> >  Documentation/devicetree/bindings/remoteproc/qcom,wcnss-pil.yaml | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,wcnss-pil.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,wcnss-pil.yaml
> > index 45eb42bd3c2c..0e5e0b7a0610 100644
> > --- a/Documentation/devicetree/bindings/remoteproc/qcom,wcnss-pil.yaml
> > +++ b/Documentation/devicetree/bindings/remoteproc/qcom,wcnss-pil.yaml
> > @@ -111,6 +111,7 @@ properties:
> >            - qcom,wcn3660
> >            - qcom,wcn3660b
> >            - qcom,wcn3680
> > +          - qcom,wcn3680b
>
> Looks like this should be made as compatible with qcom,wcn3680 (so with
> fallback).

Yes, agree, let's do a regular fallback as there is nothing 'b'
specific in the driver:
`compatible = "qcom,wcn3680b", "qcom,wcn3680";`

And yes, we should also have done that for qcom,wcn3660b...

Regards,
Loic

Re: [PATCH 1/4] dt-bindings: remoteproc: qcom: wcnss: Add WCN3680B compatible

[Stephan Gerhold]

On Mon, Oct 16, 2023 at 03:16:14PM +0200, Loic Poulain wrote:
> On Mon, 16 Oct 2023 at 07:35, Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
> >
> > On 15/10/2023 22:03, Luca Weiss wrote:
> > > Add a compatible for the iris subnode in the WCNSS PIL.
> > >
> > > Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
> > > ---
> > >  Documentation/devicetree/bindings/remoteproc/qcom,wcnss-pil.yaml | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,wcnss-pil.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,wcnss-pil.yaml
> > > index 45eb42bd3c2c..0e5e0b7a0610 100644
> > > --- a/Documentation/devicetree/bindings/remoteproc/qcom,wcnss-pil.yaml
> > > +++ b/Documentation/devicetree/bindings/remoteproc/qcom,wcnss-pil.yaml
> > > @@ -111,6 +111,7 @@ properties:
> > >            - qcom,wcn3660
> > >            - qcom,wcn3660b
> > >            - qcom,wcn3680
> > > +          - qcom,wcn3680b
> >
> > Looks like this should be made as compatible with qcom,wcn3680 (so with
> > fallback).
> 
> Yes, agree, let's do a regular fallback as there is nothing 'b'
> specific in the driver:
> `compatible = "qcom,wcn3680b", "qcom,wcn3680";`
> 
> And yes, we should also have done that for qcom,wcn3660b...
> 

I don't think this would have worked properly for qcom,wcn3660b:

 - It's not compatible with "qcom,wcn3660", because they have different
   regulator voltage requirements. wcn3660(a?) needs vddpa with
   2.9-3.0V, but wcn3660b needs 3.3V. That's why wcn3660b uses the
   wcn3680_data in qcom_wcnss.iris.c. Otherwise if you would run an
   older kernel that knows "qcom,wcn3660" but not "qcom,wcn3660b" it
   would apply the wrong voltage.

 - It's not compatible with "qcom,wcn3680" either because that is used
   as indication if 802.11ac is supported (wcn3660b doesn't).

The main question here is: What does the current "qcom,wcn3680"
compatible actually represent? It's defined with vddpa = 3.3V in the
driver, which would suggest that:

 1. It's actually meant to represent WCN3680B, which needs 3.3V vddpa
    like WCN3660B, or

 2. WCN3680(A?) has different requirements than WCN3660(A?) and also
    needs 3.3V vddpa. But then what is the difference between
    WCN3680(A?) and WCN3680B? Is there even a variant without ...B?

There is public documentation for WCN3660B and WCN3680B but the non-B
variants are shrouded in mystery.

Thanks,
Stephan

Re: [PATCH 2/4] remoteproc: qcom_wcnss: Add WCN3680B compatible

[Konrad Dybcio]

On 10/16/23 07:35, Krzysztof Kozlowski wrote:
> On 15/10/2023 22:03, Luca Weiss wrote:
>> Add a compatible for the WCN3680B chip used with some Qualcomm SoCs.
>>
>> It shares the same regulator setup as WCN3680, so we can reuse the
>> driver data for that.
>>
>> Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
>> ---
>>   drivers/remoteproc/qcom_wcnss_iris.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/remoteproc/qcom_wcnss_iris.c b/drivers/remoteproc/qcom_wcnss_iris.c
>> index dd36fd077911..22acc609105f 100644
>> --- a/drivers/remoteproc/qcom_wcnss_iris.c
>> +++ b/drivers/remoteproc/qcom_wcnss_iris.c
>> @@ -99,6 +99,7 @@ static const struct of_device_id iris_of_match[] = {
>>   	{ .compatible = "qcom,wcn3660", .data = &wcn3660_data },
>>   	{ .compatible = "qcom,wcn3660b", .data = &wcn3680_data },
>>   	{ .compatible = "qcom,wcn3680", .data = &wcn3680_data },
>> +	{ .compatible = "qcom,wcn3680b", .data = &wcn3680_data },
> 
> Just make devices compatible and no need for this driver change.
Or reconsider given <ZS1MTAHq6GLW6RAK@gerhold.net>

Konrad

Re: [PATCH 1/4] dt-bindings: remoteproc: qcom: wcnss: Add WCN3680B compatible

[Luca Weiss]

On Montag, 16. Oktober 2023 16:44:28 CET Stephan Gerhold wrote:
> On Mon, Oct 16, 2023 at 03:16:14PM +0200, Loic Poulain wrote:
> > On Mon, 16 Oct 2023 at 07:35, Krzysztof Kozlowski
> > 
> > <krzysztof.kozlowski@linaro.org> wrote:
> > > On 15/10/2023 22:03, Luca Weiss wrote:
> > > > Add a compatible for the iris subnode in the WCNSS PIL.
> > > > 
> > > > Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
> > > > ---
> > > > 
> > > >  Documentation/devicetree/bindings/remoteproc/qcom,wcnss-pil.yaml | 1
> > > >  +
> > > >  1 file changed, 1 insertion(+)
> > > > 
> > > > diff --git
> > > > a/Documentation/devicetree/bindings/remoteproc/qcom,wcnss-pil.yaml
> > > > b/Documentation/devicetree/bindings/remoteproc/qcom,wcnss-pil.yaml
> > > > index 45eb42bd3c2c..0e5e0b7a0610 100644
> > > > --- a/Documentation/devicetree/bindings/remoteproc/qcom,wcnss-pil.yaml
> > > > +++ b/Documentation/devicetree/bindings/remoteproc/qcom,wcnss-pil.yaml
> > > > 
> > > > @@ -111,6 +111,7 @@ properties:
> > > >            - qcom,wcn3660
> > > >            - qcom,wcn3660b
> > > >            - qcom,wcn3680
> > > > 
> > > > +          - qcom,wcn3680b
> > > 
> > > Looks like this should be made as compatible with qcom,wcn3680 (so with
> > > fallback).
> > 
> > Yes, agree, let's do a regular fallback as there is nothing 'b'
> > specific in the driver:
> > `compatible = "qcom,wcn3680b", "qcom,wcn3680";`
> > 
> > And yes, we should also have done that for qcom,wcn3660b...
> 
> I don't think this would have worked properly for qcom,wcn3660b:
> 
>  - It's not compatible with "qcom,wcn3660", because they have different
>    regulator voltage requirements. wcn3660(a?) needs vddpa with
>    2.9-3.0V, but wcn3660b needs 3.3V. That's why wcn3660b uses the
>    wcn3680_data in qcom_wcnss.iris.c. Otherwise if you would run an
>    older kernel that knows "qcom,wcn3660" but not "qcom,wcn3660b" it
>    would apply the wrong voltage.
> 
>  - It's not compatible with "qcom,wcn3680" either because that is used
>    as indication if 802.11ac is supported (wcn3660b doesn't).
> 
> The main question here is: What does the current "qcom,wcn3680"
> compatible actually represent? It's defined with vddpa = 3.3V in the
> driver, which would suggest that:
> 
>  1. It's actually meant to represent WCN3680B, which needs 3.3V vddpa
>     like WCN3660B, or
> 
>  2. WCN3680(A?) has different requirements than WCN3660(A?) and also
>     needs 3.3V vddpa. But then what is the difference between
>     WCN3680(A?) and WCN3680B? Is there even a variant without ...B?
> 
> There is public documentation for WCN3660B and WCN3680B but the non-B
> variants are shrouded in mystery.

Hi Stephan (and everyone),

Do you have a suggestion how to move this patchset forward? Is the fallback 
compatible that was suggested okay for the wcn3680b situation?

  compatible = "qcom,wcn3680b", "qcom,wcn3680";

If so, I'll make v2 with that implemented.

Regards
Luca

> 
> Thanks,
> Stephan

Re: [PATCH 1/4] dt-bindings: remoteproc: qcom: wcnss: Add WCN3680B compatible

[Stephan Gerhold]

On Sat, Nov 25, 2023 at 01:25:22PM +0100, Luca Weiss wrote:
> On Montag, 16. Oktober 2023 16:44:28 CET Stephan Gerhold wrote:
> > On Mon, Oct 16, 2023 at 03:16:14PM +0200, Loic Poulain wrote:
> > > On Mon, 16 Oct 2023 at 07:35, Krzysztof Kozlowski
> > > 
> > > <krzysztof.kozlowski@linaro.org> wrote:
> > > > On 15/10/2023 22:03, Luca Weiss wrote:
> > > > > Add a compatible for the iris subnode in the WCNSS PIL.
> > > > > 
> > > > > Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
> > > > > ---
> > > > > 
> > > > >  Documentation/devicetree/bindings/remoteproc/qcom,wcnss-pil.yaml | 1
> > > > >  +
> > > > >  1 file changed, 1 insertion(+)
> > > > > 
> > > > > diff --git
> > > > > a/Documentation/devicetree/bindings/remoteproc/qcom,wcnss-pil.yaml
> > > > > b/Documentation/devicetree/bindings/remoteproc/qcom,wcnss-pil.yaml
> > > > > index 45eb42bd3c2c..0e5e0b7a0610 100644
> > > > > --- a/Documentation/devicetree/bindings/remoteproc/qcom,wcnss-pil.yaml
> > > > > +++ b/Documentation/devicetree/bindings/remoteproc/qcom,wcnss-pil.yaml
> > > > > 
> > > > > @@ -111,6 +111,7 @@ properties:
> > > > >            - qcom,wcn3660
> > > > >            - qcom,wcn3660b
> > > > >            - qcom,wcn3680
> > > > > 
> > > > > +          - qcom,wcn3680b
> > > > 
> > > > Looks like this should be made as compatible with qcom,wcn3680 (so with
> > > > fallback).
> > > 
> > > Yes, agree, let's do a regular fallback as there is nothing 'b'
> > > specific in the driver:
> > > `compatible = "qcom,wcn3680b", "qcom,wcn3680";`
> > > 
> > > And yes, we should also have done that for qcom,wcn3660b...
> > 
> > I don't think this would have worked properly for qcom,wcn3660b:
> > 
> >  - It's not compatible with "qcom,wcn3660", because they have different
> >    regulator voltage requirements. wcn3660(a?) needs vddpa with
> >    2.9-3.0V, but wcn3660b needs 3.3V. That's why wcn3660b uses the
> >    wcn3680_data in qcom_wcnss.iris.c. Otherwise if you would run an
> >    older kernel that knows "qcom,wcn3660" but not "qcom,wcn3660b" it
> >    would apply the wrong voltage.
> > 
> >  - It's not compatible with "qcom,wcn3680" either because that is used
> >    as indication if 802.11ac is supported (wcn3660b doesn't).
> > 
> > The main question here is: What does the current "qcom,wcn3680"
> > compatible actually represent? It's defined with vddpa = 3.3V in the
> > driver, which would suggest that:
> > 
> >  1. It's actually meant to represent WCN3680B, which needs 3.3V vddpa
> >     like WCN3660B, or
> > 
> >  2. WCN3680(A?) has different requirements than WCN3660(A?) and also
> >     needs 3.3V vddpa. But then what is the difference between
> >     WCN3680(A?) and WCN3680B? Is there even a variant without ...B?
> > 
> > There is public documentation for WCN3660B and WCN3680B but the non-B
> > variants are shrouded in mystery.
> 
> Hi Stephan (and everyone),
> 
> Do you have a suggestion how to move this patchset forward? Is the fallback 
> compatible that was suggested okay for the wcn3680b situation?
> 
>   compatible = "qcom,wcn3680b", "qcom,wcn3680";
> 
> If so, I'll make v2 with that implemented.
> 

Personally, I would just go with what exists already and use the
existing "qcom,wcn3680" compatible as-is. No one has provided evidence
that there is actually something like a WCN3680(A), without a 'B' at the
end. Also, all existing users of the "qcom,wcn3680" compatible upstream
actually seem to refer to WCN3680B (I'm quite sure apq8039-t2 has
WCN3680B, MSM8974 isn't entirely clear but iirc you mentioned there was
some schematic of a MSM8974 device which says WCN3680B as well).

It would be nice to have the compatible perfectly correct and complete,
but I see no point to extend it with additional information that we
can't properly verify.

Or do you actually need a different compatible to customize something in
the driver?

Thanks,
Stephan

Re: [PATCH 1/4] dt-bindings: remoteproc: qcom: wcnss: Add WCN3680B compatible

[Luca Weiss]

On Montag, 27. November 2023 21:07:28 CET Stephan Gerhold wrote:
> On Sat, Nov 25, 2023 at 01:25:22PM +0100, Luca Weiss wrote:
> > On Montag, 16. Oktober 2023 16:44:28 CET Stephan Gerhold wrote:
> > > On Mon, Oct 16, 2023 at 03:16:14PM +0200, Loic Poulain wrote:
> > > > On Mon, 16 Oct 2023 at 07:35, Krzysztof Kozlowski
> > > > 
> > > > <krzysztof.kozlowski@linaro.org> wrote:
> > > > > On 15/10/2023 22:03, Luca Weiss wrote:
> > > > > > Add a compatible for the iris subnode in the WCNSS PIL.
> > > > > > 
> > > > > > Signed-off-by: Luca Weiss <luca@z3ntu.xyz>
> > > > > > ---
> > > > > > 
> > > > > >  Documentation/devicetree/bindings/remoteproc/qcom,wcnss-pil.yaml
> > > > > >  | 1
> > > > > >  +
> > > > > >  1 file changed, 1 insertion(+)
> > > > > > 
> > > > > > diff --git
> > > > > > a/Documentation/devicetree/bindings/remoteproc/qcom,wcnss-pil.yaml
> > > > > > b/Documentation/devicetree/bindings/remoteproc/qcom,wcnss-pil.yaml
> > > > > > index 45eb42bd3c2c..0e5e0b7a0610 100644
> > > > > > ---
> > > > > > a/Documentation/devicetree/bindings/remoteproc/qcom,wcnss-pil.yam
> > > > > > l
> > > > > > +++
> > > > > > b/Documentation/devicetree/bindings/remoteproc/qcom,wcnss-pil.yam
> > > > > > l
> > > > > > 
> > > > > > @@ -111,6 +111,7 @@ properties:
> > > > > >            - qcom,wcn3660
> > > > > >            - qcom,wcn3660b
> > > > > >            - qcom,wcn3680
> > > > > > 
> > > > > > +          - qcom,wcn3680b
> > > > > 
> > > > > Looks like this should be made as compatible with qcom,wcn3680 (so
> > > > > with
> > > > > fallback).
> > > > 
> > > > Yes, agree, let's do a regular fallback as there is nothing 'b'
> > > > specific in the driver:
> > > > `compatible = "qcom,wcn3680b", "qcom,wcn3680";`
> > > > 
> > > > And yes, we should also have done that for qcom,wcn3660b...
> > > 
> > > I don't think this would have worked properly for qcom,wcn3660b:
> > >  - It's not compatible with "qcom,wcn3660", because they have different
> > >  
> > >    regulator voltage requirements. wcn3660(a?) needs vddpa with
> > >    2.9-3.0V, but wcn3660b needs 3.3V. That's why wcn3660b uses the
> > >    wcn3680_data in qcom_wcnss.iris.c. Otherwise if you would run an
> > >    older kernel that knows "qcom,wcn3660" but not "qcom,wcn3660b" it
> > >    would apply the wrong voltage.
> > >  
> > >  - It's not compatible with "qcom,wcn3680" either because that is used
> > >  
> > >    as indication if 802.11ac is supported (wcn3660b doesn't).
> > > 
> > > The main question here is: What does the current "qcom,wcn3680"
> > > compatible actually represent? It's defined with vddpa = 3.3V in the
> > > 
> > > driver, which would suggest that:
> > >  1. It's actually meant to represent WCN3680B, which needs 3.3V vddpa
> > >  
> > >     like WCN3660B, or
> > >  
> > >  2. WCN3680(A?) has different requirements than WCN3660(A?) and also
> > >  
> > >     needs 3.3V vddpa. But then what is the difference between
> > >     WCN3680(A?) and WCN3680B? Is there even a variant without ...B?
> > > 
> > > There is public documentation for WCN3660B and WCN3680B but the non-B
> > > variants are shrouded in mystery.
> > 
> > Hi Stephan (and everyone),
> > 
> > Do you have a suggestion how to move this patchset forward? Is the
> > fallback
> > compatible that was suggested okay for the wcn3680b situation?
> > 
> >   compatible = "qcom,wcn3680b", "qcom,wcn3680";
> > 
> > If so, I'll make v2 with that implemented.
> 
> Personally, I would just go with what exists already and use the
> existing "qcom,wcn3680" compatible as-is. No one has provided evidence
> that there is actually something like a WCN3680(A), without a 'B' at the
> end. Also, all existing users of the "qcom,wcn3680" compatible upstream
> actually seem to refer to WCN3680B (I'm quite sure apq8039-t2 has
> WCN3680B, MSM8974 isn't entirely clear but iirc you mentioned there was
> some schematic of a MSM8974 device which says WCN3680B as well).
> 
> It would be nice to have the compatible perfectly correct and complete,
> but I see no point to extend it with additional information that we
> can't properly verify.
> 
> Or do you actually need a different compatible to customize something in
> the driver?

No it should be fine with qcom,wcn3680.

I'll send v2 in the coming days then, thanks for your input!

Regards
Luca

> 
> Thanks,
> Stephan