[PATCH v1 0/4] Add qcom,product-variant properties in Qualcomm

[PATCH v1 0/4] Add qcom,product-variant properties in Qualcomm

[Cheng Jiang]

Add a new property in qualcom bluetooth dts to identify the product
information, so the driver can load the proper firmware.

Several Qualcomm projects will use the same Bluetooth chip, each
focusing on different features. For instance, consumer projects
prioritize the A2DP SRC feature, while IoT projects focus on the A2DP
SINK feature. Due to the patch size, it is not feasible to include all
features in a single firmware.

Therefore, the 'product-variant' devicetree property is used to provide
product information for the Bluetooth driver to load the appropriate
firmware.

The driver will parse 'product-variant' to load firmware from different
directories. If it's not defined in dts, the default firmware will be
loaded, which is compatible with the existing implementaion.


Cheng Jiang (4):
  dt-bindings: bluetooth: add 'qcom,product-variant'
  dt-bindings: bluetooth: Add qca6698 compatible string
  arm64: dts: qcom: sa8775p-ride: update BT nodes
  Bluetooth: hci_qca: add qcom,product-variant properties

 .../net/bluetooth/qualcomm-bluetooth.yaml     |   8 +
 arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi    |   3 +-
 drivers/bluetooth/btqca.c                     | 142 ++++++++++++++----
 drivers/bluetooth/btqca.h                     |  11 +-
 drivers/bluetooth/hci_qca.c                   |  73 +++++----
 5 files changed, 174 insertions(+), 63 deletions(-)


base-commit: 6fb2fa9805c501d9ade047fc511961f3273cdcb5
-- 
2.25.1

[PATCH v2 1/4] dt-bindings: bluetooth: add 'qcom,product-variant'

[Cheng Jiang]

Several Qualcomm projects will use the same Bluetooth chip, each
focusing on different features. For instance, consumer projects
prioritize the A2DP SRC feature, while IoT projects focus on the A2DP
SINK feature, which may have more optimizations for coexistence when
acting as a SINK. Due to the patch size, it is not feasible to include
all features in a single firmware.

Therefore, the 'product-variant' devicetree property is used to provide
product information for the Bluetooth driver to load the appropriate
firmware.

If this property is not defined, the default firmware will be loaded,
ensuring there are no backward compatibility issues with older
devicetrees.

The product-variant defines like this:
  0 - 15 (16 bits) are product line specific definitions
  16 - 23 (8 bits) are for the product line.
  24 - 31 (8 bits) are reserved for future use, 0 currently

|---------------------------------------------------------------------|
|                       32 Bits                                       |
|---------------------------------------------------------------------|
|  31 - 24 (bits)   |    23 - 16 (bits)   | 15 - 0 (16 bits)          |
|---------------------------------------------------------------------|
|   Reserved        |    0: default       | 0: default                |
|                   |    1: CE            |                           |
|                   |    2: IoT           |                           |
|                   |    3: Auto          |                           |
|                   |    4: Reserved      |                           |
|---------------------------------------------------------------------|

Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
---
 .../bindings/net/bluetooth/qualcomm-bluetooth.yaml          | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
index 7bb68311c609..9019fe7bcdc6 100644
--- a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
+++ b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
@@ -110,6 +110,12 @@ properties:
     description:
       boot firmware is incorrectly passing the address in big-endian order
 
+  qcom,product-variant:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      specify the product information for driver to load the appropriate firmware
+
+
 required:
   - compatible
 
-- 
2.25.1

Re: [PATCH v2 1/4] dt-bindings: bluetooth: add 'qcom,product-variant'

[Dmitry Baryshkov]

On Wed, Nov 20, 2024 at 05:54:25PM +0800, Cheng Jiang wrote:
> Several Qualcomm projects will use the same Bluetooth chip, each
> focusing on different features. For instance, consumer projects
> prioritize the A2DP SRC feature, while IoT projects focus on the A2DP
> SINK feature, which may have more optimizations for coexistence when
> acting as a SINK. Due to the patch size, it is not feasible to include
> all features in a single firmware.
> 
> Therefore, the 'product-variant' devicetree property is used to provide
> product information for the Bluetooth driver to load the appropriate
> firmware.
> 
> If this property is not defined, the default firmware will be loaded,
> ensuring there are no backward compatibility issues with older
> devicetrees.
> 
> The product-variant defines like this:
>   0 - 15 (16 bits) are product line specific definitions
>   16 - 23 (8 bits) are for the product line.
>   24 - 31 (8 bits) are reserved for future use, 0 currently

Please use text strings instead of encoding this information into random
integers and then using just 3 bits out of 32.

> 
> |---------------------------------------------------------------------|
> |                       32 Bits                                       |
> |---------------------------------------------------------------------|
> |  31 - 24 (bits)   |    23 - 16 (bits)   | 15 - 0 (16 bits)          |
> |---------------------------------------------------------------------|
> |   Reserved        |    0: default       | 0: default                |
> |                   |    1: CE            |                           |
> |                   |    2: IoT           |                           |
> |                   |    3: Auto          |                           |
> |                   |    4: Reserved      |                           |
> |---------------------------------------------------------------------|
> 
> Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
> ---
>  .../bindings/net/bluetooth/qualcomm-bluetooth.yaml          | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
> index 7bb68311c609..9019fe7bcdc6 100644
> --- a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
> +++ b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
> @@ -110,6 +110,12 @@ properties:
>      description:
>        boot firmware is incorrectly passing the address in big-endian order
>  
> +  qcom,product-variant:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      specify the product information for driver to load the appropriate firmware

DT describes hardware. Is this a hardware property?

> +
> +
>  required:
>    - compatible
>  
> -- 
> 2.25.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH v2 1/4] dt-bindings: bluetooth: add 'qcom,product-variant'

[Cheng Jiang]

Hi Dmitry, 

On 11/20/2024 6:43 PM, Dmitry Baryshkov wrote:
> On Wed, Nov 20, 2024 at 05:54:25PM +0800, Cheng Jiang wrote:
>> Several Qualcomm projects will use the same Bluetooth chip, each
>> focusing on different features. For instance, consumer projects
>> prioritize the A2DP SRC feature, while IoT projects focus on the A2DP
>> SINK feature, which may have more optimizations for coexistence when
>> acting as a SINK. Due to the patch size, it is not feasible to include
>> all features in a single firmware.
>>
>> Therefore, the 'product-variant' devicetree property is used to provide
>> product information for the Bluetooth driver to load the appropriate
>> firmware.
>>
>> If this property is not defined, the default firmware will be loaded,
>> ensuring there are no backward compatibility issues with older
>> devicetrees.
>>
>> The product-variant defines like this:
>>   0 - 15 (16 bits) are product line specific definitions
>>   16 - 23 (8 bits) are for the product line.
>>   24 - 31 (8 bits) are reserved for future use, 0 currently
> 
> Please use text strings instead of encoding this information into random
> integers and then using just 3 bits out of 32.
Ack. Originally intended to make it more flexible for future use. It can be 
text strings for current requirement.
> 
>>
>> |---------------------------------------------------------------------|
>> |                       32 Bits                                       |
>> |---------------------------------------------------------------------|
>> |  31 - 24 (bits)   |    23 - 16 (bits)   | 15 - 0 (16 bits)          |
>> |---------------------------------------------------------------------|
>> |   Reserved        |    0: default       | 0: default                |
>> |                   |    1: CE            |                           |
>> |                   |    2: IoT           |                           |
>> |                   |    3: Auto          |                           |
>> |                   |    4: Reserved      |                           |
>> |---------------------------------------------------------------------|
>>
>> Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
>> ---
>>  .../bindings/net/bluetooth/qualcomm-bluetooth.yaml          | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
>> index 7bb68311c609..9019fe7bcdc6 100644
>> --- a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
>> +++ b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
>> @@ -110,6 +110,12 @@ properties:
>>      description:
>>        boot firmware is incorrectly passing the address in big-endian order
>>  
>> +  qcom,product-variant:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description:
>> +      specify the product information for driver to load the appropriate firmware
> 
> DT describes hardware. Is this a hardware property?

It has been added to identify the firmware image for the platform. The driver
parses it, and then the rampatch is selected from a specify directory. Currently, 
there is a 'firmware-name' parameter, but it is only used to specify the NVM
(config) file. We also need to specify the rampatch (TLV file).


Can we re-use the "firmware-name"? add two segments like the following?
firmware-name = "rampatch_xx.tlv",  "nvm_xx.bin";

Or add a new property to specify the rampatch file? 
rampatch-name = "rampatch_xx.tlv";

> 
>> +
>> +
>>  required:
>>    - compatible
>>  
>> -- 
>> 2.25.1
>>
> 

Re: [PATCH v2 1/4] dt-bindings: bluetooth: add 'qcom,product-variant'

[Dmitry Baryshkov]

On Thu, 21 Nov 2024 at 06:02, Cheng Jiang <quic_chejiang@quicinc.com> wrote:
>
> Hi Dmitry,
>
> On 11/20/2024 6:43 PM, Dmitry Baryshkov wrote:
> > On Wed, Nov 20, 2024 at 05:54:25PM +0800, Cheng Jiang wrote:
> >> Several Qualcomm projects will use the same Bluetooth chip, each
> >> focusing on different features. For instance, consumer projects
> >> prioritize the A2DP SRC feature, while IoT projects focus on the A2DP
> >> SINK feature, which may have more optimizations for coexistence when
> >> acting as a SINK. Due to the patch size, it is not feasible to include
> >> all features in a single firmware.
> >>
> >> Therefore, the 'product-variant' devicetree property is used to provide
> >> product information for the Bluetooth driver to load the appropriate
> >> firmware.
> >>
> >> If this property is not defined, the default firmware will be loaded,
> >> ensuring there are no backward compatibility issues with older
> >> devicetrees.
> >>
> >> The product-variant defines like this:
> >>   0 - 15 (16 bits) are product line specific definitions
> >>   16 - 23 (8 bits) are for the product line.
> >>   24 - 31 (8 bits) are reserved for future use, 0 currently
> >
> > Please use text strings instead of encoding this information into random
> > integers and then using just 3 bits out of 32.
> Ack. Originally intended to make it more flexible for future use. It can be
> text strings for current requirement.

No, fixed-format data isn't flexible. Fine-grained properties are.
Please define exactly what is necessary rather than leaving empty
holes "for future expansion".=

> >
> >>
> >> |---------------------------------------------------------------------|
> >> |                       32 Bits                                       |
> >> |---------------------------------------------------------------------|
> >> |  31 - 24 (bits)   |    23 - 16 (bits)   | 15 - 0 (16 bits)          |
> >> |---------------------------------------------------------------------|
> >> |   Reserved        |    0: default       | 0: default                |
> >> |                   |    1: CE            |                           |
> >> |                   |    2: IoT           |                           |
> >> |                   |    3: Auto          |                           |
> >> |                   |    4: Reserved      |                           |
> >> |---------------------------------------------------------------------|
> >>
> >> Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
> >> ---
> >>  .../bindings/net/bluetooth/qualcomm-bluetooth.yaml          | 6 ++++++
> >>  1 file changed, 6 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
> >> index 7bb68311c609..9019fe7bcdc6 100644
> >> --- a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
> >> +++ b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
> >> @@ -110,6 +110,12 @@ properties:
> >>      description:
> >>        boot firmware is incorrectly passing the address in big-endian order
> >>
> >> +  qcom,product-variant:
> >> +    $ref: /schemas/types.yaml#/definitions/uint32
> >> +    description:
> >> +      specify the product information for driver to load the appropriate firmware
> >
> > DT describes hardware. Is this a hardware property?
>
> It has been added to identify the firmware image for the platform. The driver
> parses it, and then the rampatch is selected from a specify directory. Currently,
> there is a 'firmware-name' parameter, but it is only used to specify the NVM
> (config) file. We also need to specify the rampatch (TLV file).
>
>
> Can we re-use the "firmware-name"? add two segments like the following?
> firmware-name = "rampatch_xx.tlv",  "nvm_xx.bin";

I think this is the better solution

>
> Or add a new property to specify the rampatch file?
> rampatch-name = "rampatch_xx.tlv";
>
> >
> >> +
> >> +
> >>  required:
> >>    - compatible
> >>
> >> --
> >> 2.25.1
> >>
> >
>


-- 
With best wishes
Dmitry

Re: [PATCH v2 1/4] dt-bindings: bluetooth: add 'qcom,product-variant'

[Cheng Jiang]

Hi Dmitry,

On 11/21/2024 12:38 PM, Dmitry Baryshkov wrote:
> On Thu, 21 Nov 2024 at 06:02, Cheng Jiang <quic_chejiang@quicinc.com> wrote:
>>
>> Hi Dmitry,
>>
>> On 11/20/2024 6:43 PM, Dmitry Baryshkov wrote:
>>> On Wed, Nov 20, 2024 at 05:54:25PM +0800, Cheng Jiang wrote:
>>>> Several Qualcomm projects will use the same Bluetooth chip, each
>>>> focusing on different features. For instance, consumer projects
>>>> prioritize the A2DP SRC feature, while IoT projects focus on the A2DP
>>>> SINK feature, which may have more optimizations for coexistence when
>>>> acting as a SINK. Due to the patch size, it is not feasible to include
>>>> all features in a single firmware.
>>>>
>>>> Therefore, the 'product-variant' devicetree property is used to provide
>>>> product information for the Bluetooth driver to load the appropriate
>>>> firmware.
>>>>
>>>> If this property is not defined, the default firmware will be loaded,
>>>> ensuring there are no backward compatibility issues with older
>>>> devicetrees.
>>>>
>>>> The product-variant defines like this:
>>>>   0 - 15 (16 bits) are product line specific definitions
>>>>   16 - 23 (8 bits) are for the product line.
>>>>   24 - 31 (8 bits) are reserved for future use, 0 currently
>>>
>>> Please use text strings instead of encoding this information into random
>>> integers and then using just 3 bits out of 32.
>> Ack. Originally intended to make it more flexible for future use. It can be
>> text strings for current requirement.
> 
> No, fixed-format data isn't flexible. Fine-grained properties are.
> Please define exactly what is necessary rather than leaving empty
> holes "for future expansion".=
Got it. Thank you for the guidance. 
> 
>>>
>>>>
>>>> |---------------------------------------------------------------------|
>>>> |                       32 Bits                                       |
>>>> |---------------------------------------------------------------------|
>>>> |  31 - 24 (bits)   |    23 - 16 (bits)   | 15 - 0 (16 bits)          |
>>>> |---------------------------------------------------------------------|
>>>> |   Reserved        |    0: default       | 0: default                |
>>>> |                   |    1: CE            |                           |
>>>> |                   |    2: IoT           |                           |
>>>> |                   |    3: Auto          |                           |
>>>> |                   |    4: Reserved      |                           |
>>>> |---------------------------------------------------------------------|
>>>>
>>>> Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
>>>> ---
>>>>  .../bindings/net/bluetooth/qualcomm-bluetooth.yaml          | 6 ++++++
>>>>  1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
>>>> index 7bb68311c609..9019fe7bcdc6 100644
>>>> --- a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
>>>> +++ b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
>>>> @@ -110,6 +110,12 @@ properties:
>>>>      description:
>>>>        boot firmware is incorrectly passing the address in big-endian order
>>>>
>>>> +  qcom,product-variant:
>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>> +    description:
>>>> +      specify the product information for driver to load the appropriate firmware
>>>
>>> DT describes hardware. Is this a hardware property?
>>
>> It has been added to identify the firmware image for the platform. The driver
>> parses it, and then the rampatch is selected from a specify directory. Currently,
>> there is a 'firmware-name' parameter, but it is only used to specify the NVM
>> (config) file. We also need to specify the rampatch (TLV file).
>>
>>
>> Can we re-use the "firmware-name"? add two segments like the following?
>> firmware-name = "rampatch_xx.tlv",  "nvm_xx.bin";
> 
> I think this is the better solution
Ack. Will submit a new change based on this. 
> 
>>
>> Or add a new property to specify the rampatch file?
>> rampatch-name = "rampatch_xx.tlv";
>>
>>>
>>>> +
>>>> +
>>>>  required:
>>>>    - compatible
>>>>
>>>> --
>>>> 2.25.1
>>>>
>>>
>>
> 
> 

Re: [PATCH v2 1/4] dt-bindings: bluetooth: add 'qcom,product-variant'

[Cheng Jiang (IOE)]

Hi Dmitry,

On 11/21/2024 12:38 PM, Dmitry Baryshkov wrote:
> On Thu, 21 Nov 2024 at 06:02, Cheng Jiang <quic_chejiang@quicinc.com> wrote:
>>
>> Hi Dmitry,
>>
>> On 11/20/2024 6:43 PM, Dmitry Baryshkov wrote:
>>> On Wed, Nov 20, 2024 at 05:54:25PM +0800, Cheng Jiang wrote:
>>>> Several Qualcomm projects will use the same Bluetooth chip, each
>>>> focusing on different features. For instance, consumer projects
>>>> prioritize the A2DP SRC feature, while IoT projects focus on the A2DP
>>>> SINK feature, which may have more optimizations for coexistence when
>>>> acting as a SINK. Due to the patch size, it is not feasible to include
>>>> all features in a single firmware.
>>>>
>>>> Therefore, the 'product-variant' devicetree property is used to provide
>>>> product information for the Bluetooth driver to load the appropriate
>>>> firmware.
>>>>
>>>> If this property is not defined, the default firmware will be loaded,
>>>> ensuring there are no backward compatibility issues with older
>>>> devicetrees.
>>>>
>>>> The product-variant defines like this:
>>>>   0 - 15 (16 bits) are product line specific definitions
>>>>   16 - 23 (8 bits) are for the product line.
>>>>   24 - 31 (8 bits) are reserved for future use, 0 currently
>>>
>>> Please use text strings instead of encoding this information into random
>>> integers and then using just 3 bits out of 32.
>> Ack. Originally intended to make it more flexible for future use. It can be
>> text strings for current requirement.
> 
> No, fixed-format data isn't flexible. Fine-grained properties are.
> Please define exactly what is necessary rather than leaving empty
> holes "for future expansion".=
> 
>>>
>>>>
>>>> |---------------------------------------------------------------------|
>>>> |                       32 Bits                                       |
>>>> |---------------------------------------------------------------------|
>>>> |  31 - 24 (bits)   |    23 - 16 (bits)   | 15 - 0 (16 bits)          |
>>>> |---------------------------------------------------------------------|
>>>> |   Reserved        |    0: default       | 0: default                |
>>>> |                   |    1: CE            |                           |
>>>> |                   |    2: IoT           |                           |
>>>> |                   |    3: Auto          |                           |
>>>> |                   |    4: Reserved      |                           |
>>>> |---------------------------------------------------------------------|
>>>>
>>>> Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
>>>> ---
>>>>  .../bindings/net/bluetooth/qualcomm-bluetooth.yaml          | 6 ++++++
>>>>  1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
>>>> index 7bb68311c609..9019fe7bcdc6 100644
>>>> --- a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
>>>> +++ b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
>>>> @@ -110,6 +110,12 @@ properties:
>>>>      description:
>>>>        boot firmware is incorrectly passing the address in big-endian order
>>>>
>>>> +  qcom,product-variant:
>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>> +    description:
>>>> +      specify the product information for driver to load the appropriate firmware
>>>
>>> DT describes hardware. Is this a hardware property?
>>
>> It has been added to identify the firmware image for the platform. The driver
>> parses it, and then the rampatch is selected from a specify directory. Currently,
>> there is a 'firmware-name' parameter, but it is only used to specify the NVM
>> (config) file. We also need to specify the rampatch (TLV file).
>>
>>
>> Can we re-use the "firmware-name"? add two segments like the following?
>> firmware-name = "rampatch_xx.tlv",  "nvm_xx.bin";
> 
> I think this is the better solution
> 
How about the following logic for handling 'firmware-name' property:
1. If there is only one string in firmware-name, it must be the NVM file, which is used
   for backward compatibility.

2. If there are two strings in firmware-name, the first string is for the rampatch, and 
   the second string is for the NVM.

3. Due to variations in RF performance of chips from different foundries, different NVM
   configurations are used based on the board ID. If the second string ends with boardid,
   the NVM file will be selected according to the board ID.


Here are two examples:

 firmware-name = "qca/QCA6698/hpbtfw21.tlv",  "qca/QCA6698/hpnv21.bin";
In this configuration, the driver will use the two files directly.


 firmware-name = "qca/QCA6698/hpbtfw21.tlv",  "qca/QCA6698/hpnv21.boardid";
In this configuration, the driver will replace boardid with the actual board information.
If the board id is 0x0206, the nvm file name will be qca/QCA6698/hpnv21.b0206

>>
>> Or add a new property to specify the rampatch file?
>> rampatch-name = "rampatch_xx.tlv";
>>
>>>
>>>> +
>>>> +
>>>>  required:
>>>>    - compatible
>>>>
>>>> --
>>>> 2.25.1
>>>>
>>>
>>
> 
> 

Re: [PATCH v2 1/4] dt-bindings: bluetooth: add 'qcom,product-variant'

[Dmitry Baryshkov]

On Sat, 30 Nov 2024 at 05:48, Cheng Jiang (IOE)
<quic_chejiang@quicinc.com> wrote:
>
> Hi Dmitry,
>
> On 11/21/2024 12:38 PM, Dmitry Baryshkov wrote:
> > On Thu, 21 Nov 2024 at 06:02, Cheng Jiang <quic_chejiang@quicinc.com> wrote:
> >>
> >> Hi Dmitry,
> >>
> >> On 11/20/2024 6:43 PM, Dmitry Baryshkov wrote:
> >>> On Wed, Nov 20, 2024 at 05:54:25PM +0800, Cheng Jiang wrote:
> >>>> Several Qualcomm projects will use the same Bluetooth chip, each
> >>>> focusing on different features. For instance, consumer projects
> >>>> prioritize the A2DP SRC feature, while IoT projects focus on the A2DP
> >>>> SINK feature, which may have more optimizations for coexistence when
> >>>> acting as a SINK. Due to the patch size, it is not feasible to include
> >>>> all features in a single firmware.
> >>>>
> >>>> Therefore, the 'product-variant' devicetree property is used to provide
> >>>> product information for the Bluetooth driver to load the appropriate
> >>>> firmware.
> >>>>
> >>>> If this property is not defined, the default firmware will be loaded,
> >>>> ensuring there are no backward compatibility issues with older
> >>>> devicetrees.
> >>>>
> >>>> The product-variant defines like this:
> >>>>   0 - 15 (16 bits) are product line specific definitions
> >>>>   16 - 23 (8 bits) are for the product line.
> >>>>   24 - 31 (8 bits) are reserved for future use, 0 currently
> >>>
> >>> Please use text strings instead of encoding this information into random
> >>> integers and then using just 3 bits out of 32.
> >> Ack. Originally intended to make it more flexible for future use. It can be
> >> text strings for current requirement.
> >
> > No, fixed-format data isn't flexible. Fine-grained properties are.
> > Please define exactly what is necessary rather than leaving empty
> > holes "for future expansion".=
> >
> >>>
> >>>>
> >>>> |---------------------------------------------------------------------|
> >>>> |                       32 Bits                                       |
> >>>> |---------------------------------------------------------------------|
> >>>> |  31 - 24 (bits)   |    23 - 16 (bits)   | 15 - 0 (16 bits)          |
> >>>> |---------------------------------------------------------------------|
> >>>> |   Reserved        |    0: default       | 0: default                |
> >>>> |                   |    1: CE            |                           |
> >>>> |                   |    2: IoT           |                           |
> >>>> |                   |    3: Auto          |                           |
> >>>> |                   |    4: Reserved      |                           |
> >>>> |---------------------------------------------------------------------|
> >>>>
> >>>> Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
> >>>> ---
> >>>>  .../bindings/net/bluetooth/qualcomm-bluetooth.yaml          | 6 ++++++
> >>>>  1 file changed, 6 insertions(+)
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
> >>>> index 7bb68311c609..9019fe7bcdc6 100644
> >>>> --- a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
> >>>> +++ b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
> >>>> @@ -110,6 +110,12 @@ properties:
> >>>>      description:
> >>>>        boot firmware is incorrectly passing the address in big-endian order
> >>>>
> >>>> +  qcom,product-variant:
> >>>> +    $ref: /schemas/types.yaml#/definitions/uint32
> >>>> +    description:
> >>>> +      specify the product information for driver to load the appropriate firmware
> >>>
> >>> DT describes hardware. Is this a hardware property?
> >>
> >> It has been added to identify the firmware image for the platform. The driver
> >> parses it, and then the rampatch is selected from a specify directory. Currently,
> >> there is a 'firmware-name' parameter, but it is only used to specify the NVM
> >> (config) file. We also need to specify the rampatch (TLV file).
> >>
> >>
> >> Can we re-use the "firmware-name"? add two segments like the following?
> >> firmware-name = "rampatch_xx.tlv",  "nvm_xx.bin";
> >
> > I think this is the better solution
> >
> How about the following logic for handling 'firmware-name' property:
> 1. If there is only one string in firmware-name, it must be the NVM file, which is used
>    for backward compatibility.
>
> 2. If there are two strings in firmware-name, the first string is for the rampatch, and
>    the second string is for the NVM.

I'd say, other way around: the first one is always NVM, the second one
is rampatch and it is optional.

>
> 3. Due to variations in RF performance of chips from different foundries, different NVM
>    configurations are used based on the board ID. If the second string ends with boardid,
>    the NVM file will be selected according to the board ID.

Is there a reason why you can not use the exact firmware name? The
firmware name is a part of the board DT file. I assume you know the
board ID that has been used for the board.

>
>
> Here are two examples:
>
>  firmware-name = "qca/QCA6698/hpbtfw21.tlv",  "qca/QCA6698/hpnv21.bin";
> In this configuration, the driver will use the two files directly.
>
>
>  firmware-name = "qca/QCA6698/hpbtfw21.tlv",  "qca/QCA6698/hpnv21.boardid";
> In this configuration, the driver will replace boardid with the actual board information.
> If the board id is 0x0206, the nvm file name will be qca/QCA6698/hpnv21.b0206
>
> >>
> >> Or add a new property to specify the rampatch file?
> >> rampatch-name = "rampatch_xx.tlv";
> >>
> >>>
> >>>> +
> >>>> +
> >>>>  required:
> >>>>    - compatible
> >>>>
> >>>> --
> >>>> 2.25.1
> >>>>
> >>>
> >>
> >
> >
>

--
With best wishes
Dmitry

Re: [PATCH v2 1/4] dt-bindings: bluetooth: add 'qcom,product-variant'

[Cheng Jiang (IOE)]

Hi Dmitry,

On 11/30/2024 4:24 PM, Dmitry Baryshkov wrote:
> On Sat, 30 Nov 2024 at 05:48, Cheng Jiang (IOE)
> <quic_chejiang@quicinc.com> wrote:
>>
>> Hi Dmitry,
>>
>> On 11/21/2024 12:38 PM, Dmitry Baryshkov wrote:
>>> On Thu, 21 Nov 2024 at 06:02, Cheng Jiang <quic_chejiang@quicinc.com> wrote:
>>>>
>>>> Hi Dmitry,
>>>>
>>>> On 11/20/2024 6:43 PM, Dmitry Baryshkov wrote:
>>>>> On Wed, Nov 20, 2024 at 05:54:25PM +0800, Cheng Jiang wrote:
>>>>>> Several Qualcomm projects will use the same Bluetooth chip, each
>>>>>> focusing on different features. For instance, consumer projects
>>>>>> prioritize the A2DP SRC feature, while IoT projects focus on the A2DP
>>>>>> SINK feature, which may have more optimizations for coexistence when
>>>>>> acting as a SINK. Due to the patch size, it is not feasible to include
>>>>>> all features in a single firmware.
>>>>>>
>>>>>> Therefore, the 'product-variant' devicetree property is used to provide
>>>>>> product information for the Bluetooth driver to load the appropriate
>>>>>> firmware.
>>>>>>
>>>>>> If this property is not defined, the default firmware will be loaded,
>>>>>> ensuring there are no backward compatibility issues with older
>>>>>> devicetrees.
>>>>>>
>>>>>> The product-variant defines like this:
>>>>>>   0 - 15 (16 bits) are product line specific definitions
>>>>>>   16 - 23 (8 bits) are for the product line.
>>>>>>   24 - 31 (8 bits) are reserved for future use, 0 currently
>>>>>
>>>>> Please use text strings instead of encoding this information into random
>>>>> integers and then using just 3 bits out of 32.
>>>> Ack. Originally intended to make it more flexible for future use. It can be
>>>> text strings for current requirement.
>>>
>>> No, fixed-format data isn't flexible. Fine-grained properties are.
>>> Please define exactly what is necessary rather than leaving empty
>>> holes "for future expansion".=
>>>
>>>>>
>>>>>>
>>>>>> |---------------------------------------------------------------------|
>>>>>> |                       32 Bits                                       |
>>>>>> |---------------------------------------------------------------------|
>>>>>> |  31 - 24 (bits)   |    23 - 16 (bits)   | 15 - 0 (16 bits)          |
>>>>>> |---------------------------------------------------------------------|
>>>>>> |   Reserved        |    0: default       | 0: default                |
>>>>>> |                   |    1: CE            |                           |
>>>>>> |                   |    2: IoT           |                           |
>>>>>> |                   |    3: Auto          |                           |
>>>>>> |                   |    4: Reserved      |                           |
>>>>>> |---------------------------------------------------------------------|
>>>>>>
>>>>>> Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
>>>>>> ---
>>>>>>  .../bindings/net/bluetooth/qualcomm-bluetooth.yaml          | 6 ++++++
>>>>>>  1 file changed, 6 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
>>>>>> index 7bb68311c609..9019fe7bcdc6 100644
>>>>>> --- a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
>>>>>> +++ b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
>>>>>> @@ -110,6 +110,12 @@ properties:
>>>>>>      description:
>>>>>>        boot firmware is incorrectly passing the address in big-endian order
>>>>>>
>>>>>> +  qcom,product-variant:
>>>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>>>> +    description:
>>>>>> +      specify the product information for driver to load the appropriate firmware
>>>>>
>>>>> DT describes hardware. Is this a hardware property?
>>>>
>>>> It has been added to identify the firmware image for the platform. The driver
>>>> parses it, and then the rampatch is selected from a specify directory. Currently,
>>>> there is a 'firmware-name' parameter, but it is only used to specify the NVM
>>>> (config) file. We also need to specify the rampatch (TLV file).
>>>>
>>>>
>>>> Can we re-use the "firmware-name"? add two segments like the following?
>>>> firmware-name = "rampatch_xx.tlv",  "nvm_xx.bin";
>>>
>>> I think this is the better solution
>>>
>> How about the following logic for handling 'firmware-name' property:
>> 1. If there is only one string in firmware-name, it must be the NVM file, which is used
>>    for backward compatibility.
>>
>> 2. If there are two strings in firmware-name, the first string is for the rampatch, and
>>    the second string is for the NVM.
> 
> I'd say, other way around: the first one is always NVM, the second one
> is rampatch and it is optional.
> 
OK, Got it.
>>
>> 3. Due to variations in RF performance of chips from different foundries, different NVM
>>    configurations are used based on the board ID. If the second string ends with boardid,
>>    the NVM file will be selected according to the board ID.
> 
> Is there a reason why you can not use the exact firmware name? The
> firmware name is a part of the board DT file. I assume you know the
> board ID that has been used for the board.
> 
The boardid is the connectivity board's id. NVM is a board specific configuration file, 
it's related to the connectivity board. We may attach different connectivity board on the
same platform. For example, we have connectivity boards based on the QCA6698 chipset that
can support either a two-antenna or three-antenna solution. Both boards work fine on the
sa8775p-ride platform. 
>>
>>
>> Here are two examples:
>>
>>  firmware-name = "qca/QCA6698/hpbtfw21.tlv",  "qca/QCA6698/hpnv21.bin";
>> In this configuration, the driver will use the two files directly.
>>
>>
>>  firmware-name = "qca/QCA6698/hpbtfw21.tlv",  "qca/QCA6698/hpnv21.boardid";
>> In this configuration, the driver will replace boardid with the actual board information.
>> If the board id is 0x0206, the nvm file name will be qca/QCA6698/hpnv21.b0206
>>
>>>>
>>>> Or add a new property to specify the rampatch file?
>>>> rampatch-name = "rampatch_xx.tlv";
>>>>
>>>>>
>>>>>> +
>>>>>> +
>>>>>>  required:
>>>>>>    - compatible
>>>>>>
>>>>>> --
>>>>>> 2.25.1
>>>>>>
>>>>>
>>>>
>>>
>>>
>>
> 
> --
> With best wishes
> Dmitry

Re: [PATCH v2 1/4] dt-bindings: bluetooth: add 'qcom,product-variant'

[Dmitry Baryshkov]

On Mon, Dec 02, 2024 at 10:22:52AM +0800, Cheng Jiang (IOE) wrote:
> Hi Dmitry,
> 
> On 11/30/2024 4:24 PM, Dmitry Baryshkov wrote:
> > On Sat, 30 Nov 2024 at 05:48, Cheng Jiang (IOE)
> > <quic_chejiang@quicinc.com> wrote:
> >>
> >> Hi Dmitry,
> >>
> >> On 11/21/2024 12:38 PM, Dmitry Baryshkov wrote:
> >>> On Thu, 21 Nov 2024 at 06:02, Cheng Jiang <quic_chejiang@quicinc.com> wrote:
> >>>>
> >>>> Hi Dmitry,
> >>>>
> >>>> On 11/20/2024 6:43 PM, Dmitry Baryshkov wrote:
> >>>>> On Wed, Nov 20, 2024 at 05:54:25PM +0800, Cheng Jiang wrote:
> >>>>>> Several Qualcomm projects will use the same Bluetooth chip, each
> >>>>>> focusing on different features. For instance, consumer projects
> >>>>>> prioritize the A2DP SRC feature, while IoT projects focus on the A2DP
> >>>>>> SINK feature, which may have more optimizations for coexistence when
> >>>>>> acting as a SINK. Due to the patch size, it is not feasible to include
> >>>>>> all features in a single firmware.
> >>>>>>
> >>>>>> Therefore, the 'product-variant' devicetree property is used to provide
> >>>>>> product information for the Bluetooth driver to load the appropriate
> >>>>>> firmware.
> >>>>>>
> >>>>>> If this property is not defined, the default firmware will be loaded,
> >>>>>> ensuring there are no backward compatibility issues with older
> >>>>>> devicetrees.
> >>>>>>
> >>>>>> The product-variant defines like this:
> >>>>>>   0 - 15 (16 bits) are product line specific definitions
> >>>>>>   16 - 23 (8 bits) are for the product line.
> >>>>>>   24 - 31 (8 bits) are reserved for future use, 0 currently
> >>>>>
> >>>>> Please use text strings instead of encoding this information into random
> >>>>> integers and then using just 3 bits out of 32.
> >>>> Ack. Originally intended to make it more flexible for future use. It can be
> >>>> text strings for current requirement.
> >>>
> >>> No, fixed-format data isn't flexible. Fine-grained properties are.
> >>> Please define exactly what is necessary rather than leaving empty
> >>> holes "for future expansion".=
> >>>
> >>>>>
> >>>>>>
> >>>>>> |---------------------------------------------------------------------|
> >>>>>> |                       32 Bits                                       |
> >>>>>> |---------------------------------------------------------------------|
> >>>>>> |  31 - 24 (bits)   |    23 - 16 (bits)   | 15 - 0 (16 bits)          |
> >>>>>> |---------------------------------------------------------------------|
> >>>>>> |   Reserved        |    0: default       | 0: default                |
> >>>>>> |                   |    1: CE            |                           |
> >>>>>> |                   |    2: IoT           |                           |
> >>>>>> |                   |    3: Auto          |                           |
> >>>>>> |                   |    4: Reserved      |                           |
> >>>>>> |---------------------------------------------------------------------|
> >>>>>>
> >>>>>> Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
> >>>>>> ---
> >>>>>>  .../bindings/net/bluetooth/qualcomm-bluetooth.yaml          | 6 ++++++
> >>>>>>  1 file changed, 6 insertions(+)
> >>>>>>
> >>>>>> diff --git a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
> >>>>>> index 7bb68311c609..9019fe7bcdc6 100644
> >>>>>> --- a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
> >>>>>> +++ b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
> >>>>>> @@ -110,6 +110,12 @@ properties:
> >>>>>>      description:
> >>>>>>        boot firmware is incorrectly passing the address in big-endian order
> >>>>>>
> >>>>>> +  qcom,product-variant:
> >>>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
> >>>>>> +    description:
> >>>>>> +      specify the product information for driver to load the appropriate firmware
> >>>>>
> >>>>> DT describes hardware. Is this a hardware property?
> >>>>
> >>>> It has been added to identify the firmware image for the platform. The driver
> >>>> parses it, and then the rampatch is selected from a specify directory. Currently,
> >>>> there is a 'firmware-name' parameter, but it is only used to specify the NVM
> >>>> (config) file. We also need to specify the rampatch (TLV file).
> >>>>
> >>>>
> >>>> Can we re-use the "firmware-name"? add two segments like the following?
> >>>> firmware-name = "rampatch_xx.tlv",  "nvm_xx.bin";
> >>>
> >>> I think this is the better solution
> >>>
> >> How about the following logic for handling 'firmware-name' property:
> >> 1. If there is only one string in firmware-name, it must be the NVM file, which is used
> >>    for backward compatibility.
> >>
> >> 2. If there are two strings in firmware-name, the first string is for the rampatch, and
> >>    the second string is for the NVM.
> > 
> > I'd say, other way around: the first one is always NVM, the second one
> > is rampatch and it is optional.
> > 
> OK, Got it.
> >>
> >> 3. Due to variations in RF performance of chips from different foundries, different NVM
> >>    configurations are used based on the board ID. If the second string ends with boardid,
> >>    the NVM file will be selected according to the board ID.
> > 
> > Is there a reason why you can not use the exact firmware name? The
> > firmware name is a part of the board DT file. I assume you know the
> > board ID that has been used for the board.
> > 
> The boardid is the connectivity board's id. NVM is a board specific configuration file, 
> it's related to the connectivity board. We may attach different connectivity board on the
> same platform. For example, we have connectivity boards based on the QCA6698 chipset that
> can support either a two-antenna or three-antenna solution. Both boards work fine on the
> sa8775p-ride platform. 

Please add such an info to the commit messages (plural for it being a
generic feedback: please describe the reasons for your design
decisions),

I really don't like the .boardid template. What if we change property
behaviour in the following way: if there is no file extension then .bNN
will be probed, falling back to .bin. This will require reading board ID
for all the platforms that support it (do wcn3990 have board ID?)

> >>
> >>
> >> Here are two examples:
> >>
> >>  firmware-name = "qca/QCA6698/hpbtfw21.tlv",  "qca/QCA6698/hpnv21.bin";
> >> In this configuration, the driver will use the two files directly.
> >>
> >>
> >>  firmware-name = "qca/QCA6698/hpbtfw21.tlv",  "qca/QCA6698/hpnv21.boardid";
> >> In this configuration, the driver will replace boardid with the actual board information.
> >> If the board id is 0x0206, the nvm file name will be qca/QCA6698/hpnv21.b0206
> >>
> >>>>
> >>>> Or add a new property to specify the rampatch file?
> >>>> rampatch-name = "rampatch_xx.tlv";
> >>>>
> >>>>>
> >>>>>> +
> >>>>>> +
> >>>>>>  required:
> >>>>>>    - compatible
> >>>>>>
> >>>>>> --
> >>>>>> 2.25.1
> >>>>>>
> >>>>>
> >>>>
> >>>
> >>>
> >>
> > 
> > --
> > With best wishes
> > Dmitry
> 

-- 
With best wishes
Dmitry

Re: [PATCH v2 1/4] dt-bindings: bluetooth: add 'qcom,product-variant'

[Cheng Jiang (IOE)]

On 12/2/2024 7:38 PM, Dmitry Baryshkov wrote:
> On Mon, Dec 02, 2024 at 10:22:52AM +0800, Cheng Jiang (IOE) wrote:
>> Hi Dmitry,
>>
>> On 11/30/2024 4:24 PM, Dmitry Baryshkov wrote:
>>> On Sat, 30 Nov 2024 at 05:48, Cheng Jiang (IOE)
>>> <quic_chejiang@quicinc.com> wrote:
>>>>
>>>> Hi Dmitry,
>>>>
>>>> On 11/21/2024 12:38 PM, Dmitry Baryshkov wrote:
>>>>> On Thu, 21 Nov 2024 at 06:02, Cheng Jiang <quic_chejiang@quicinc.com> wrote:
>>>>>>
>>>>>> Hi Dmitry,
>>>>>>
>>>>>> On 11/20/2024 6:43 PM, Dmitry Baryshkov wrote:
>>>>>>> On Wed, Nov 20, 2024 at 05:54:25PM +0800, Cheng Jiang wrote:
>>>>>>>> Several Qualcomm projects will use the same Bluetooth chip, each
>>>>>>>> focusing on different features. For instance, consumer projects
>>>>>>>> prioritize the A2DP SRC feature, while IoT projects focus on the A2DP
>>>>>>>> SINK feature, which may have more optimizations for coexistence when
>>>>>>>> acting as a SINK. Due to the patch size, it is not feasible to include
>>>>>>>> all features in a single firmware.
>>>>>>>>
>>>>>>>> Therefore, the 'product-variant' devicetree property is used to provide
>>>>>>>> product information for the Bluetooth driver to load the appropriate
>>>>>>>> firmware.
>>>>>>>>
>>>>>>>> If this property is not defined, the default firmware will be loaded,
>>>>>>>> ensuring there are no backward compatibility issues with older
>>>>>>>> devicetrees.
>>>>>>>>
>>>>>>>> The product-variant defines like this:
>>>>>>>>   0 - 15 (16 bits) are product line specific definitions
>>>>>>>>   16 - 23 (8 bits) are for the product line.
>>>>>>>>   24 - 31 (8 bits) are reserved for future use, 0 currently
>>>>>>>
>>>>>>> Please use text strings instead of encoding this information into random
>>>>>>> integers and then using just 3 bits out of 32.
>>>>>> Ack. Originally intended to make it more flexible for future use. It can be
>>>>>> text strings for current requirement.
>>>>>
>>>>> No, fixed-format data isn't flexible. Fine-grained properties are.
>>>>> Please define exactly what is necessary rather than leaving empty
>>>>> holes "for future expansion".=
>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> |---------------------------------------------------------------------|
>>>>>>>> |                       32 Bits                                       |
>>>>>>>> |---------------------------------------------------------------------|
>>>>>>>> |  31 - 24 (bits)   |    23 - 16 (bits)   | 15 - 0 (16 bits)          |
>>>>>>>> |---------------------------------------------------------------------|
>>>>>>>> |   Reserved        |    0: default       | 0: default                |
>>>>>>>> |                   |    1: CE            |                           |
>>>>>>>> |                   |    2: IoT           |                           |
>>>>>>>> |                   |    3: Auto          |                           |
>>>>>>>> |                   |    4: Reserved      |                           |
>>>>>>>> |---------------------------------------------------------------------|
>>>>>>>>
>>>>>>>> Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
>>>>>>>> ---
>>>>>>>>  .../bindings/net/bluetooth/qualcomm-bluetooth.yaml          | 6 ++++++
>>>>>>>>  1 file changed, 6 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
>>>>>>>> index 7bb68311c609..9019fe7bcdc6 100644
>>>>>>>> --- a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
>>>>>>>> +++ b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
>>>>>>>> @@ -110,6 +110,12 @@ properties:
>>>>>>>>      description:
>>>>>>>>        boot firmware is incorrectly passing the address in big-endian order
>>>>>>>>
>>>>>>>> +  qcom,product-variant:
>>>>>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>>>>>> +    description:
>>>>>>>> +      specify the product information for driver to load the appropriate firmware
>>>>>>>
>>>>>>> DT describes hardware. Is this a hardware property?
>>>>>>
>>>>>> It has been added to identify the firmware image for the platform. The driver
>>>>>> parses it, and then the rampatch is selected from a specify directory. Currently,
>>>>>> there is a 'firmware-name' parameter, but it is only used to specify the NVM
>>>>>> (config) file. We also need to specify the rampatch (TLV file).
>>>>>>
>>>>>>
>>>>>> Can we re-use the "firmware-name"? add two segments like the following?
>>>>>> firmware-name = "rampatch_xx.tlv",  "nvm_xx.bin";
>>>>>
>>>>> I think this is the better solution
>>>>>
>>>> How about the following logic for handling 'firmware-name' property:
>>>> 1. If there is only one string in firmware-name, it must be the NVM file, which is used
>>>>    for backward compatibility.
>>>>
>>>> 2. If there are two strings in firmware-name, the first string is for the rampatch, and
>>>>    the second string is for the NVM.
>>>
>>> I'd say, other way around: the first one is always NVM, the second one
>>> is rampatch and it is optional.
>>>
>> OK, Got it.
>>>>
>>>> 3. Due to variations in RF performance of chips from different foundries, different NVM
>>>>    configurations are used based on the board ID. If the second string ends with boardid,
>>>>    the NVM file will be selected according to the board ID.
>>>
>>> Is there a reason why you can not use the exact firmware name? The
>>> firmware name is a part of the board DT file. I assume you know the
>>> board ID that has been used for the board.
>>>
>> The boardid is the connectivity board's id. NVM is a board specific configuration file, 
>> it's related to the connectivity board. We may attach different connectivity board on the
>> same platform. For example, we have connectivity boards based on the QCA6698 chipset that
>> can support either a two-antenna or three-antenna solution. Both boards work fine on the
>> sa8775p-ride platform. 
> 
> Please add such an info to the commit messages (plural for it being a
> generic feedback: please describe the reasons for your design
> decisions),
> 
Ack.
> I really don't like the .boardid template. What if we change property
> behaviour in the following way: if there is no file extension then .bNN
> will be probed, falling back to .bin. This will require reading board ID
> for all the platforms that support it (do wcn3990 have board ID?)
> 
Ack, this proposal is great. 
Yes, We have board ID for each connectivity card. An NVM file maps to it
if necessary.

Let me provide a new patchset based on this solution. Thank you very much for
the valuable comments. 
>>>>
>>>>
>>>> Here are two examples:
>>>>
>>>>  firmware-name = "qca/QCA6698/hpbtfw21.tlv",  "qca/QCA6698/hpnv21.bin";
>>>> In this configuration, the driver will use the two files directly.
>>>>
>>>>
>>>>  firmware-name = "qca/QCA6698/hpbtfw21.tlv",  "qca/QCA6698/hpnv21.boardid";
>>>> In this configuration, the driver will replace boardid with the actual board information.
>>>> If the board id is 0x0206, the nvm file name will be qca/QCA6698/hpnv21.b0206
>>>>
>>>>>>
>>>>>> Or add a new property to specify the rampatch file?
>>>>>> rampatch-name = "rampatch_xx.tlv";
>>>>>>
>>>>>>>
>>>>>>>> +
>>>>>>>> +
>>>>>>>>  required:
>>>>>>>>    - compatible
>>>>>>>>
>>>>>>>> --
>>>>>>>> 2.25.1
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>> --
>>> With best wishes
>>> Dmitry
>>
> 

Re: [PATCH v2 1/4] dt-bindings: bluetooth: add 'qcom,product-variant'

[Dmitry Baryshkov]

On Mon, 2 Dec 2024 at 16:25, Cheng Jiang (IOE)
<quic_chejiang@quicinc.com> wrote:
>
>
>
> On 12/2/2024 7:38 PM, Dmitry Baryshkov wrote:
> > On Mon, Dec 02, 2024 at 10:22:52AM +0800, Cheng Jiang (IOE) wrote:
> >> Hi Dmitry,
> >>
> >> On 11/30/2024 4:24 PM, Dmitry Baryshkov wrote:
> >>> On Sat, 30 Nov 2024 at 05:48, Cheng Jiang (IOE)
> >>> <quic_chejiang@quicinc.com> wrote:
> >>>>
> >>>> Hi Dmitry,
> >>>>
> >>>> On 11/21/2024 12:38 PM, Dmitry Baryshkov wrote:
> >>>>> On Thu, 21 Nov 2024 at 06:02, Cheng Jiang <quic_chejiang@quicinc.com> wrote:
> >>>>>>
> >>>>>> Hi Dmitry,
> >>>>>>
> >>>>>> On 11/20/2024 6:43 PM, Dmitry Baryshkov wrote:
> >>>>>>> On Wed, Nov 20, 2024 at 05:54:25PM +0800, Cheng Jiang wrote:
> >>>>>>>> Several Qualcomm projects will use the same Bluetooth chip, each
> >>>>>>>> focusing on different features. For instance, consumer projects
> >>>>>>>> prioritize the A2DP SRC feature, while IoT projects focus on the A2DP
> >>>>>>>> SINK feature, which may have more optimizations for coexistence when
> >>>>>>>> acting as a SINK. Due to the patch size, it is not feasible to include
> >>>>>>>> all features in a single firmware.
> >>>>>>>>
> >>>>>>>> Therefore, the 'product-variant' devicetree property is used to provide
> >>>>>>>> product information for the Bluetooth driver to load the appropriate
> >>>>>>>> firmware.
> >>>>>>>>
> >>>>>>>> If this property is not defined, the default firmware will be loaded,
> >>>>>>>> ensuring there are no backward compatibility issues with older
> >>>>>>>> devicetrees.
> >>>>>>>>
> >>>>>>>> The product-variant defines like this:
> >>>>>>>>   0 - 15 (16 bits) are product line specific definitions
> >>>>>>>>   16 - 23 (8 bits) are for the product line.
> >>>>>>>>   24 - 31 (8 bits) are reserved for future use, 0 currently
> >>>>>>>
> >>>>>>> Please use text strings instead of encoding this information into random
> >>>>>>> integers and then using just 3 bits out of 32.
> >>>>>> Ack. Originally intended to make it more flexible for future use. It can be
> >>>>>> text strings for current requirement.
> >>>>>
> >>>>> No, fixed-format data isn't flexible. Fine-grained properties are.
> >>>>> Please define exactly what is necessary rather than leaving empty
> >>>>> holes "for future expansion".=
> >>>>>
> >>>>>>>
> >>>>>>>>
> >>>>>>>> |---------------------------------------------------------------------|
> >>>>>>>> |                       32 Bits                                       |
> >>>>>>>> |---------------------------------------------------------------------|
> >>>>>>>> |  31 - 24 (bits)   |    23 - 16 (bits)   | 15 - 0 (16 bits)          |
> >>>>>>>> |---------------------------------------------------------------------|
> >>>>>>>> |   Reserved        |    0: default       | 0: default                |
> >>>>>>>> |                   |    1: CE            |                           |
> >>>>>>>> |                   |    2: IoT           |                           |
> >>>>>>>> |                   |    3: Auto          |                           |
> >>>>>>>> |                   |    4: Reserved      |                           |
> >>>>>>>> |---------------------------------------------------------------------|
> >>>>>>>>
> >>>>>>>> Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
> >>>>>>>> ---
> >>>>>>>>  .../bindings/net/bluetooth/qualcomm-bluetooth.yaml          | 6 ++++++
> >>>>>>>>  1 file changed, 6 insertions(+)
> >>>>>>>>
> >>>>>>>> diff --git a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
> >>>>>>>> index 7bb68311c609..9019fe7bcdc6 100644
> >>>>>>>> --- a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
> >>>>>>>> +++ b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
> >>>>>>>> @@ -110,6 +110,12 @@ properties:
> >>>>>>>>      description:
> >>>>>>>>        boot firmware is incorrectly passing the address in big-endian order
> >>>>>>>>
> >>>>>>>> +  qcom,product-variant:
> >>>>>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
> >>>>>>>> +    description:
> >>>>>>>> +      specify the product information for driver to load the appropriate firmware
> >>>>>>>
> >>>>>>> DT describes hardware. Is this a hardware property?
> >>>>>>
> >>>>>> It has been added to identify the firmware image for the platform. The driver
> >>>>>> parses it, and then the rampatch is selected from a specify directory. Currently,
> >>>>>> there is a 'firmware-name' parameter, but it is only used to specify the NVM
> >>>>>> (config) file. We also need to specify the rampatch (TLV file).
> >>>>>>
> >>>>>>
> >>>>>> Can we re-use the "firmware-name"? add two segments like the following?
> >>>>>> firmware-name = "rampatch_xx.tlv",  "nvm_xx.bin";
> >>>>>
> >>>>> I think this is the better solution
> >>>>>
> >>>> How about the following logic for handling 'firmware-name' property:
> >>>> 1. If there is only one string in firmware-name, it must be the NVM file, which is used
> >>>>    for backward compatibility.
> >>>>
> >>>> 2. If there are two strings in firmware-name, the first string is for the rampatch, and
> >>>>    the second string is for the NVM.
> >>>
> >>> I'd say, other way around: the first one is always NVM, the second one
> >>> is rampatch and it is optional.
> >>>
> >> OK, Got it.
> >>>>
> >>>> 3. Due to variations in RF performance of chips from different foundries, different NVM
> >>>>    configurations are used based on the board ID. If the second string ends with boardid,
> >>>>    the NVM file will be selected according to the board ID.
> >>>
> >>> Is there a reason why you can not use the exact firmware name? The
> >>> firmware name is a part of the board DT file. I assume you know the
> >>> board ID that has been used for the board.
> >>>
> >> The boardid is the connectivity board's id. NVM is a board specific configuration file,
> >> it's related to the connectivity board. We may attach different connectivity board on the
> >> same platform. For example, we have connectivity boards based on the QCA6698 chipset that
> >> can support either a two-antenna or three-antenna solution. Both boards work fine on the
> >> sa8775p-ride platform.
> >
> > Please add such an info to the commit messages (plural for it being a
> > generic feedback: please describe the reasons for your design
> > decisions),
> >
> Ack.
> > I really don't like the .boardid template. What if we change property
> > behaviour in the following way: if there is no file extension then .bNN
> > will be probed, falling back to .bin. This will require reading board ID
> > for all the platforms that support it (do wcn3990 have board ID?)
> >
> Ack, this proposal is great.
> Yes, We have board ID for each connectivity card. An NVM file maps to it
> if necessary.

The question was about the WiFI generations, not about the NVM cards.
Do wcn3990 also support reading board ID?

>
> Let me provide a new patchset based on this solution. Thank you very much for
> the valuable comments.

:-)

> >>>>
> >>>>
> >>>> Here are two examples:
> >>>>
> >>>>  firmware-name = "qca/QCA6698/hpbtfw21.tlv",  "qca/QCA6698/hpnv21.bin";
> >>>> In this configuration, the driver will use the two files directly.
> >>>>
> >>>>
> >>>>  firmware-name = "qca/QCA6698/hpbtfw21.tlv",  "qca/QCA6698/hpnv21.boardid";
> >>>> In this configuration, the driver will replace boardid with the actual board information.
> >>>> If the board id is 0x0206, the nvm file name will be qca/QCA6698/hpnv21.b0206
> >>>>
> >>>>>>
> >>>>>> Or add a new property to specify the rampatch file?
> >>>>>> rampatch-name = "rampatch_xx.tlv";
> >>>>>>
> >>>>>>>
> >>>>>>>> +
> >>>>>>>> +
> >>>>>>>>  required:
> >>>>>>>>    - compatible
> >>>>>>>>
> >>>>>>>> --
> >>>>>>>> 2.25.1
> >>>>>>>>
> >>>>>>>
> >>>>>>
> >>>>>
> >>>>>
> >>>>
> >>>
> >>> --
> >>> With best wishes
> >>> Dmitry
> >>
> >
>


-- 
With best wishes
Dmitry

Re: [PATCH v2 1/4] dt-bindings: bluetooth: add 'qcom,product-variant'

[Cheng Jiang (IOE)]

Hi Dmitry,

On 12/2/2024 11:14 PM, Dmitry Baryshkov wrote:
> On Mon, 2 Dec 2024 at 16:25, Cheng Jiang (IOE)
> <quic_chejiang@quicinc.com> wrote:
>>
>>
>>
>> On 12/2/2024 7:38 PM, Dmitry Baryshkov wrote:
>>> On Mon, Dec 02, 2024 at 10:22:52AM +0800, Cheng Jiang (IOE) wrote:
>>>> Hi Dmitry,
>>>>
>>>> On 11/30/2024 4:24 PM, Dmitry Baryshkov wrote:
>>>>> On Sat, 30 Nov 2024 at 05:48, Cheng Jiang (IOE)
>>>>> <quic_chejiang@quicinc.com> wrote:
>>>>>>
>>>>>> Hi Dmitry,
>>>>>>
>>>>>> On 11/21/2024 12:38 PM, Dmitry Baryshkov wrote:
>>>>>>> On Thu, 21 Nov 2024 at 06:02, Cheng Jiang <quic_chejiang@quicinc.com> wrote:
>>>>>>>>
>>>>>>>> Hi Dmitry,
>>>>>>>>
>>>>>>>> On 11/20/2024 6:43 PM, Dmitry Baryshkov wrote:
>>>>>>>>> On Wed, Nov 20, 2024 at 05:54:25PM +0800, Cheng Jiang wrote:
>>>>>>>>>> Several Qualcomm projects will use the same Bluetooth chip, each
>>>>>>>>>> focusing on different features. For instance, consumer projects
>>>>>>>>>> prioritize the A2DP SRC feature, while IoT projects focus on the A2DP
>>>>>>>>>> SINK feature, which may have more optimizations for coexistence when
>>>>>>>>>> acting as a SINK. Due to the patch size, it is not feasible to include
>>>>>>>>>> all features in a single firmware.
>>>>>>>>>>
>>>>>>>>>> Therefore, the 'product-variant' devicetree property is used to provide
>>>>>>>>>> product information for the Bluetooth driver to load the appropriate
>>>>>>>>>> firmware.
>>>>>>>>>>
>>>>>>>>>> If this property is not defined, the default firmware will be loaded,
>>>>>>>>>> ensuring there are no backward compatibility issues with older
>>>>>>>>>> devicetrees.
>>>>>>>>>>
>>>>>>>>>> The product-variant defines like this:
>>>>>>>>>>   0 - 15 (16 bits) are product line specific definitions
>>>>>>>>>>   16 - 23 (8 bits) are for the product line.
>>>>>>>>>>   24 - 31 (8 bits) are reserved for future use, 0 currently
>>>>>>>>>
>>>>>>>>> Please use text strings instead of encoding this information into random
>>>>>>>>> integers and then using just 3 bits out of 32.
>>>>>>>> Ack. Originally intended to make it more flexible for future use. It can be
>>>>>>>> text strings for current requirement.
>>>>>>>
>>>>>>> No, fixed-format data isn't flexible. Fine-grained properties are.
>>>>>>> Please define exactly what is necessary rather than leaving empty
>>>>>>> holes "for future expansion".=
>>>>>>>
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> |---------------------------------------------------------------------|
>>>>>>>>>> |                       32 Bits                                       |
>>>>>>>>>> |---------------------------------------------------------------------|
>>>>>>>>>> |  31 - 24 (bits)   |    23 - 16 (bits)   | 15 - 0 (16 bits)          |
>>>>>>>>>> |---------------------------------------------------------------------|
>>>>>>>>>> |   Reserved        |    0: default       | 0: default                |
>>>>>>>>>> |                   |    1: CE            |                           |
>>>>>>>>>> |                   |    2: IoT           |                           |
>>>>>>>>>> |                   |    3: Auto          |                           |
>>>>>>>>>> |                   |    4: Reserved      |                           |
>>>>>>>>>> |---------------------------------------------------------------------|
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
>>>>>>>>>> ---
>>>>>>>>>>  .../bindings/net/bluetooth/qualcomm-bluetooth.yaml          | 6 ++++++
>>>>>>>>>>  1 file changed, 6 insertions(+)
>>>>>>>>>>
>>>>>>>>>> diff --git a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
>>>>>>>>>> index 7bb68311c609..9019fe7bcdc6 100644
>>>>>>>>>> --- a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
>>>>>>>>>> +++ b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
>>>>>>>>>> @@ -110,6 +110,12 @@ properties:
>>>>>>>>>>      description:
>>>>>>>>>>        boot firmware is incorrectly passing the address in big-endian order
>>>>>>>>>>
>>>>>>>>>> +  qcom,product-variant:
>>>>>>>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>>>>>>>> +    description:
>>>>>>>>>> +      specify the product information for driver to load the appropriate firmware
>>>>>>>>>
>>>>>>>>> DT describes hardware. Is this a hardware property?
>>>>>>>>
>>>>>>>> It has been added to identify the firmware image for the platform. The driver
>>>>>>>> parses it, and then the rampatch is selected from a specify directory. Currently,
>>>>>>>> there is a 'firmware-name' parameter, but it is only used to specify the NVM
>>>>>>>> (config) file. We also need to specify the rampatch (TLV file).
>>>>>>>>
>>>>>>>>
>>>>>>>> Can we re-use the "firmware-name"? add two segments like the following?
>>>>>>>> firmware-name = "rampatch_xx.tlv",  "nvm_xx.bin";
>>>>>>>
>>>>>>> I think this is the better solution
>>>>>>>
>>>>>> How about the following logic for handling 'firmware-name' property:
>>>>>> 1. If there is only one string in firmware-name, it must be the NVM file, which is used
>>>>>>    for backward compatibility.
>>>>>>
>>>>>> 2. If there are two strings in firmware-name, the first string is for the rampatch, and
>>>>>>    the second string is for the NVM.
>>>>>
>>>>> I'd say, other way around: the first one is always NVM, the second one
>>>>> is rampatch and it is optional.
>>>>>
>>>> OK, Got it.
>>>>>>
>>>>>> 3. Due to variations in RF performance of chips from different foundries, different NVM
>>>>>>    configurations are used based on the board ID. If the second string ends with boardid,
>>>>>>    the NVM file will be selected according to the board ID.
>>>>>
>>>>> Is there a reason why you can not use the exact firmware name? The
>>>>> firmware name is a part of the board DT file. I assume you know the
>>>>> board ID that has been used for the board.
>>>>>
>>>> The boardid is the connectivity board's id. NVM is a board specific configuration file,
>>>> it's related to the connectivity board. We may attach different connectivity board on the
>>>> same platform. For example, we have connectivity boards based on the QCA6698 chipset that
>>>> can support either a two-antenna or three-antenna solution. Both boards work fine on the
>>>> sa8775p-ride platform.
>>>
>>> Please add such an info to the commit messages (plural for it being a
>>> generic feedback: please describe the reasons for your design
>>> decisions),
>>>
>> Ack.
>>> I really don't like the .boardid template. What if we change property
>>> behaviour in the following way: if there is no file extension then .bNN
>>> will be probed, falling back to .bin. This will require reading board ID
>>> for all the platforms that support it (do wcn3990 have board ID?)
>>>
>> Ack, this proposal is great.
>> Yes, We have board ID for each connectivity card. An NVM file maps to it
>> if necessary.
> 
> The question was about the WiFI generations, not about the NVM cards.
> Do wcn3990 also support reading board ID?
> 
WCN3990 supports reading board id. However, the board ID is only associated with
the connectivity board, not the chipset itself. From a Bluetooth perspective,
all WCN3990 connectivity boards use the same NVM file.

>>
>> Let me provide a new patchset based on this solution. Thank you very much for
>> the valuable comments.
> 
> :-)
> 
>>>>>>
>>>>>>
>>>>>> Here are two examples:
>>>>>>
>>>>>>  firmware-name = "qca/QCA6698/hpbtfw21.tlv",  "qca/QCA6698/hpnv21.bin";
>>>>>> In this configuration, the driver will use the two files directly.
>>>>>>
>>>>>>
>>>>>>  firmware-name = "qca/QCA6698/hpbtfw21.tlv",  "qca/QCA6698/hpnv21.boardid";
>>>>>> In this configuration, the driver will replace boardid with the actual board information.
>>>>>> If the board id is 0x0206, the nvm file name will be qca/QCA6698/hpnv21.b0206
>>>>>>
>>>>>>>>
>>>>>>>> Or add a new property to specify the rampatch file?
>>>>>>>> rampatch-name = "rampatch_xx.tlv";
>>>>>>>>
>>>>>>>>>
>>>>>>>>>> +
>>>>>>>>>> +
>>>>>>>>>>  required:
>>>>>>>>>>    - compatible
>>>>>>>>>>
>>>>>>>>>> --
>>>>>>>>>> 2.25.1
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>> --
>>>>> With best wishes
>>>>> Dmitry
>>>>
>>>
>>
> 
> 

Re: [PATCH v2 1/4] dt-bindings: bluetooth: add 'qcom,product-variant'

[Krzysztof Kozlowski]

On 20/11/2024 10:54, Cheng Jiang wrote:
> Several Qualcomm projects will use the same Bluetooth chip, each
> focusing on different features. For instance, consumer projects
> prioritize the A2DP SRC feature, while IoT projects focus on the A2DP
> SINK feature, which may have more optimizations for coexistence when
> acting as a SINK. Due to the patch size, it is not feasible to include
> all features in a single firmware.
> 
> Therefore, the 'product-variant' devicetree property is used to provide
> product information for the Bluetooth driver to load the appropriate
> firmware.
> 
> If this property is not defined, the default firmware will be loaded,
> ensuring there are no backward compatibility issues with older
> devicetrees.
> 
> The product-variant defines like this:
>   0 - 15 (16 bits) are product line specific definitions
>   16 - 23 (8 bits) are for the product line.
>   24 - 31 (8 bits) are reserved for future use, 0 currently
> 
> |---------------------------------------------------------------------|
> |                       32 Bits                                       |
> |---------------------------------------------------------------------|
> |  31 - 24 (bits)   |    23 - 16 (bits)   | 15 - 0 (16 bits)          |
> |---------------------------------------------------------------------|
> |   Reserved        |    0: default       | 0: default                |
> |                   |    1: CE            |                           |
> |                   |    2: IoT           |                           |
> |                   |    3: Auto          |                           |
> |                   |    4: Reserved      |                           |
> |---------------------------------------------------------------------|
> 
> Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
> ---
>  .../bindings/net/bluetooth/qualcomm-bluetooth.yaml          | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
> index 7bb68311c609..9019fe7bcdc6 100644
> --- a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
> +++ b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
> @@ -110,6 +110,12 @@ properties:
>      description:
>        boot firmware is incorrectly passing the address in big-endian order
>  
> +  qcom,product-variant:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      specify the product information for driver to load the appropriate firmware

Nah, you have firmware-name for this.


> +
> +
No clue why two blank lines...

Best regards,
Krzysztof

Re: [PATCH v2 1/4] dt-bindings: bluetooth: add 'qcom,product-variant'

[Cheng Jiang]

Hi Krzysztof,

On 11/21/2024 12:47 AM, Krzysztof Kozlowski wrote:
> On 20/11/2024 10:54, Cheng Jiang wrote:
>> Several Qualcomm projects will use the same Bluetooth chip, each
>> focusing on different features. For instance, consumer projects
>> prioritize the A2DP SRC feature, while IoT projects focus on the A2DP
>> SINK feature, which may have more optimizations for coexistence when
>> acting as a SINK. Due to the patch size, it is not feasible to include
>> all features in a single firmware.
>>
>> Therefore, the 'product-variant' devicetree property is used to provide
>> product information for the Bluetooth driver to load the appropriate
>> firmware.
>>
>> If this property is not defined, the default firmware will be loaded,
>> ensuring there are no backward compatibility issues with older
>> devicetrees.
>>
>> The product-variant defines like this:
>>   0 - 15 (16 bits) are product line specific definitions
>>   16 - 23 (8 bits) are for the product line.
>>   24 - 31 (8 bits) are reserved for future use, 0 currently
>>
>> |---------------------------------------------------------------------|
>> |                       32 Bits                                       |
>> |---------------------------------------------------------------------|
>> |  31 - 24 (bits)   |    23 - 16 (bits)   | 15 - 0 (16 bits)          |
>> |---------------------------------------------------------------------|
>> |   Reserved        |    0: default       | 0: default                |
>> |                   |    1: CE            |                           |
>> |                   |    2: IoT           |                           |
>> |                   |    3: Auto          |                           |
>> |                   |    4: Reserved      |                           |
>> |---------------------------------------------------------------------|
>>
>> Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
>> ---
>>  .../bindings/net/bluetooth/qualcomm-bluetooth.yaml          | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
>> index 7bb68311c609..9019fe7bcdc6 100644
>> --- a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
>> +++ b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
>> @@ -110,6 +110,12 @@ properties:
>>      description:
>>        boot firmware is incorrectly passing the address in big-endian order
>>  
>> +  qcom,product-variant:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description:
>> +      specify the product information for driver to load the appropriate firmware
> 
> Nah, you have firmware-name for this.
> 
Currently "firmware-name" is used to specifythe nvm (config) file only,
we also need to specify the rampatch file (TLV). 
 
Can we re-use the "firmware-name"? add two segments like the following?
firmware-name = "rampatch_xx.tlv",  "nvm_xx.bin";

Or add a new property to specify the rampatch file? 
rampatch-name = "rampatch_xx.tlv";

Thanks!
> 
>> +
>> +
> No clue why two blank lines...
> 
> Best regards,
> Krzysztof

Re: [PATCH v2 1/4] dt-bindings: bluetooth: add 'qcom,product-variant'

[Krzysztof Kozlowski]

On 21/11/2024 05:06, Cheng Jiang wrote:
> Hi Krzysztof,
> 
> On 11/21/2024 12:47 AM, Krzysztof Kozlowski wrote:
>> On 20/11/2024 10:54, Cheng Jiang wrote:
>>> Several Qualcomm projects will use the same Bluetooth chip, each
>>> focusing on different features. For instance, consumer projects
>>> prioritize the A2DP SRC feature, while IoT projects focus on the A2DP
>>> SINK feature, which may have more optimizations for coexistence when
>>> acting as a SINK. Due to the patch size, it is not feasible to include
>>> all features in a single firmware.
>>>
>>> Therefore, the 'product-variant' devicetree property is used to provide
>>> product information for the Bluetooth driver to load the appropriate
>>> firmware.
>>>
>>> If this property is not defined, the default firmware will be loaded,
>>> ensuring there are no backward compatibility issues with older
>>> devicetrees.
>>>
>>> The product-variant defines like this:
>>>   0 - 15 (16 bits) are product line specific definitions
>>>   16 - 23 (8 bits) are for the product line.
>>>   24 - 31 (8 bits) are reserved for future use, 0 currently
>>>
>>> |---------------------------------------------------------------------|
>>> |                       32 Bits                                       |
>>> |---------------------------------------------------------------------|
>>> |  31 - 24 (bits)   |    23 - 16 (bits)   | 15 - 0 (16 bits)          |
>>> |---------------------------------------------------------------------|
>>> |   Reserved        |    0: default       | 0: default                |
>>> |                   |    1: CE            |                           |
>>> |                   |    2: IoT           |                           |
>>> |                   |    3: Auto          |                           |
>>> |                   |    4: Reserved      |                           |
>>> |---------------------------------------------------------------------|
>>>
>>> Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
>>> ---
>>>  .../bindings/net/bluetooth/qualcomm-bluetooth.yaml          | 6 ++++++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
>>> index 7bb68311c609..9019fe7bcdc6 100644
>>> --- a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
>>> +++ b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
>>> @@ -110,6 +110,12 @@ properties:
>>>      description:
>>>        boot firmware is incorrectly passing the address in big-endian order
>>>  
>>> +  qcom,product-variant:
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    description:
>>> +      specify the product information for driver to load the appropriate firmware
>>
>> Nah, you have firmware-name for this.
>>
> Currently "firmware-name" is used to specifythe nvm (config) file only,
> we also need to specify the rampatch file (TLV). 
>  
> Can we re-use the "firmware-name"? add two segments like the following?
> firmware-name = "rampatch_xx.tlv",  "nvm_xx.bin";
> 
> Or add a new property to specify the rampatch file? 
> rampatch-name = "rampatch_xx.tlv";
You can grow the property, it's a list. Order of items in the list must
be fixed (specific), though. See other Qualcomm remoteproc PAS loaders
which already use two entries.

Best regards,
Krzysztof

Re: [PATCH v2 1/4] dt-bindings: bluetooth: add 'qcom,product-variant'

[Cheng Jiang (IOE)]

Hi Krzysztof,

On 11/21/2024 3:49 PM, Krzysztof Kozlowski wrote:
> On 21/11/2024 05:06, Cheng Jiang wrote:
>> Hi Krzysztof,
>>
>> On 11/21/2024 12:47 AM, Krzysztof Kozlowski wrote:
>>> On 20/11/2024 10:54, Cheng Jiang wrote:
>>>> Several Qualcomm projects will use the same Bluetooth chip, each
>>>> focusing on different features. For instance, consumer projects
>>>> prioritize the A2DP SRC feature, while IoT projects focus on the A2DP
>>>> SINK feature, which may have more optimizations for coexistence when
>>>> acting as a SINK. Due to the patch size, it is not feasible to include
>>>> all features in a single firmware.
>>>>
>>>> Therefore, the 'product-variant' devicetree property is used to provide
>>>> product information for the Bluetooth driver to load the appropriate
>>>> firmware.
>>>>
>>>> If this property is not defined, the default firmware will be loaded,
>>>> ensuring there are no backward compatibility issues with older
>>>> devicetrees.
>>>>
>>>> The product-variant defines like this:
>>>>   0 - 15 (16 bits) are product line specific definitions
>>>>   16 - 23 (8 bits) are for the product line.
>>>>   24 - 31 (8 bits) are reserved for future use, 0 currently
>>>>
>>>> |---------------------------------------------------------------------|
>>>> |                       32 Bits                                       |
>>>> |---------------------------------------------------------------------|
>>>> |  31 - 24 (bits)   |    23 - 16 (bits)   | 15 - 0 (16 bits)          |
>>>> |---------------------------------------------------------------------|
>>>> |   Reserved        |    0: default       | 0: default                |
>>>> |                   |    1: CE            |                           |
>>>> |                   |    2: IoT           |                           |
>>>> |                   |    3: Auto          |                           |
>>>> |                   |    4: Reserved      |                           |
>>>> |---------------------------------------------------------------------|
>>>>
>>>> Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
>>>> ---
>>>>  .../bindings/net/bluetooth/qualcomm-bluetooth.yaml          | 6 ++++++
>>>>  1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
>>>> index 7bb68311c609..9019fe7bcdc6 100644
>>>> --- a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
>>>> +++ b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
>>>> @@ -110,6 +110,12 @@ properties:
>>>>      description:
>>>>        boot firmware is incorrectly passing the address in big-endian order
>>>>  
>>>> +  qcom,product-variant:
>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>> +    description:
>>>> +      specify the product information for driver to load the appropriate firmware
>>>
>>> Nah, you have firmware-name for this.
>>>
>> Currently "firmware-name" is used to specifythe nvm (config) file only,
>> we also need to specify the rampatch file (TLV). 
>>  
>> Can we re-use the "firmware-name"? add two segments like the following?
>> firmware-name = "rampatch_xx.tlv",  "nvm_xx.bin";
>>
>> Or add a new property to specify the rampatch file? 
>> rampatch-name = "rampatch_xx.tlv";
> You can grow the property, it's a list. Order of items in the list must
> be fixed (specific), though. See other Qualcomm remoteproc PAS loaders
> which already use two entries.
Thank you for the guidance. I will follow it to submit a new change. 
> 
> Best regards,
> Krzysztof

Re: [PATCH v2 1/4] dt-bindings: bluetooth: add 'qcom,product-variant'

[Konrad Dybcio]

On 20.11.2024 10:54 AM, Cheng Jiang wrote:
> Several Qualcomm projects will use the same Bluetooth chip, each
> focusing on different features. For instance, consumer projects
> prioritize the A2DP SRC feature, while IoT projects focus on the A2DP
> SINK feature, which may have more optimizations for coexistence when
> acting as a SINK. Due to the patch size, it is not feasible to include
> all features in a single firmware.
> 
> Therefore, the 'product-variant' devicetree property is used to provide
> product information for the Bluetooth driver to load the appropriate
> firmware.
> 
> If this property is not defined, the default firmware will be loaded,
> ensuring there are no backward compatibility issues with older
> devicetrees.
> 
> The product-variant defines like this:
>   0 - 15 (16 bits) are product line specific definitions
>   16 - 23 (8 bits) are for the product line.

Product lines don't exist in the kernel. Please describe the actual
differences between those files instead.

Konrad

Re: [PATCH v2 1/4] dt-bindings: bluetooth: add 'qcom,product-variant'

[Konrad Dybcio]

On 22.11.2024 1:49 PM, Konrad Dybcio wrote:
> On 20.11.2024 10:54 AM, Cheng Jiang wrote:
>> Several Qualcomm projects will use the same Bluetooth chip, each
>> focusing on different features. For instance, consumer projects
>> prioritize the A2DP SRC feature, while IoT projects focus on the A2DP
>> SINK feature, which may have more optimizations for coexistence when
>> acting as a SINK. Due to the patch size, it is not feasible to include
>> all features in a single firmware.
>>
>> Therefore, the 'product-variant' devicetree property is used to provide
>> product information for the Bluetooth driver to load the appropriate
>> firmware.
>>
>> If this property is not defined, the default firmware will be loaded,
>> ensuring there are no backward compatibility issues with older
>> devicetrees.
>>
>> The product-variant defines like this:
>>   0 - 15 (16 bits) are product line specific definitions
>>   16 - 23 (8 bits) are for the product line.
> 
> Product lines don't exist in the kernel. Please describe the actual
> differences between those files instead.

Ok, so it seems to be feature set. This definitely should be
userspace-toggleable. We can always reset the device and reload firmware
at runtime, so I see no point in having to educate the kernel about this.

I'd say A2DP SRC is probably preferred to be the deafult, as we have
people running bare upstream on laptops and other consumer devices.

Konrad

[PATCH v2 2/4] dt-bindings: bluetooth: Add qca6698 compatible string

[Cheng Jiang]

Add QCA6698 qcom,qca6698-bt compatible strings.

Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
---
 .../devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml   | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
index 9019fe7bcdc6..527f947289af 100644
--- a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
+++ b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
@@ -18,6 +18,7 @@ properties:
     enum:
       - qcom,qca2066-bt
       - qcom,qca6174-bt
+      - qcom,qca6698-bt
       - qcom,qca9377-bt
       - qcom,wcn3988-bt
       - qcom,wcn3990-bt
@@ -175,6 +176,7 @@ allOf:
         compatible:
           contains:
             enum:
+              - qcom,qca6698-bt
               - qcom,wcn6855-bt
     then:
       required:
-- 
2.25.1

Re: [PATCH v2 2/4] dt-bindings: bluetooth: Add qca6698 compatible string

[Dmitry Baryshkov]

On Wed, Nov 20, 2024 at 05:54:26PM +0800, Cheng Jiang wrote:
> Add QCA6698 qcom,qca6698-bt compatible strings.

Why? Is it the same chip as WCN6855 or a different chip? Is it
completely compatible?

> 
> Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
> ---
>  .../devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml   | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
> index 9019fe7bcdc6..527f947289af 100644
> --- a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
> +++ b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
> @@ -18,6 +18,7 @@ properties:
>      enum:
>        - qcom,qca2066-bt
>        - qcom,qca6174-bt
> +      - qcom,qca6698-bt
>        - qcom,qca9377-bt
>        - qcom,wcn3988-bt
>        - qcom,wcn3990-bt
> @@ -175,6 +176,7 @@ allOf:
>          compatible:
>            contains:
>              enum:
> +              - qcom,qca6698-bt
>                - qcom,wcn6855-bt
>      then:
>        required:
> -- 
> 2.25.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH v2 2/4] dt-bindings: bluetooth: Add qca6698 compatible string

[Cheng Jiang]

Hi Dmitry,

On 11/20/2024 6:44 PM, Dmitry Baryshkov wrote:
> On Wed, Nov 20, 2024 at 05:54:26PM +0800, Cheng Jiang wrote:
>> Add QCA6698 qcom,qca6698-bt compatible strings.
> 
> Why? Is it the same chip as WCN6855 or a different chip? Is it
> completely compatible?
> 
They are different chips. But it's compatible with WCN6855.
>>
>> Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
>> ---
>>  .../devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml   | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
>> index 9019fe7bcdc6..527f947289af 100644
>> --- a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
>> +++ b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
>> @@ -18,6 +18,7 @@ properties:
>>      enum:
>>        - qcom,qca2066-bt
>>        - qcom,qca6174-bt
>> +      - qcom,qca6698-bt
>>        - qcom,qca9377-bt
>>        - qcom,wcn3988-bt
>>        - qcom,wcn3990-bt
>> @@ -175,6 +176,7 @@ allOf:
>>          compatible:
>>            contains:
>>              enum:
>> +              - qcom,qca6698-bt
>>                - qcom,wcn6855-bt
>>      then:
>>        required:
>> -- 
>> 2.25.1
>>
> 

Re: [PATCH v2 2/4] dt-bindings: bluetooth: Add qca6698 compatible string

[Krzysztof Kozlowski]

On 21/11/2024 05:12, Cheng Jiang wrote:
> Hi Dmitry,
> 
> On 11/20/2024 6:44 PM, Dmitry Baryshkov wrote:
>> On Wed, Nov 20, 2024 at 05:54:26PM +0800, Cheng Jiang wrote:
>>> Add QCA6698 qcom,qca6698-bt compatible strings.
>>
>> Why? Is it the same chip as WCN6855 or a different chip? Is it
>> completely compatible?
>>
> They are different chips. But it's compatible with WCN6855.

That's what the commit msg is for. Do not say what is obvious, visible
from the diff.

Best regards,
Krzysztof

Re: [PATCH v2 2/4] dt-bindings: bluetooth: Add qca6698 compatible string

[Dmitry Baryshkov]

On Thu, 21 Nov 2024 at 06:12, Cheng Jiang <quic_chejiang@quicinc.com> wrote:
>
> Hi Dmitry,
>
> On 11/20/2024 6:44 PM, Dmitry Baryshkov wrote:
> > On Wed, Nov 20, 2024 at 05:54:26PM +0800, Cheng Jiang wrote:
> >> Add QCA6698 qcom,qca6698-bt compatible strings.
> >
> > Why? Is it the same chip as WCN6855 or a different chip? Is it
> > completely compatible?
> >
> They are different chips. But it's compatible with WCN6855.

So, do we really need new compat? Will/can it use the same firmware?

> >>
> >> Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
> >> ---
> >>  .../devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml   | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
> >> index 9019fe7bcdc6..527f947289af 100644
> >> --- a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
> >> +++ b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
> >> @@ -18,6 +18,7 @@ properties:
> >>      enum:
> >>        - qcom,qca2066-bt
> >>        - qcom,qca6174-bt
> >> +      - qcom,qca6698-bt
> >>        - qcom,qca9377-bt
> >>        - qcom,wcn3988-bt
> >>        - qcom,wcn3990-bt
> >> @@ -175,6 +176,7 @@ allOf:
> >>          compatible:
> >>            contains:
> >>              enum:
> >> +              - qcom,qca6698-bt
> >>                - qcom,wcn6855-bt
> >>      then:
> >>        required:
> >> --
> >> 2.25.1
> >>
> >
>


-- 
With best wishes
Dmitry

Re: [PATCH v2 2/4] dt-bindings: bluetooth: Add qca6698 compatible string

[Cheng Jiang (IOE)]

Hi Dmitry,

On 11/22/2024 12:28 AM, Dmitry Baryshkov wrote:
> On Thu, 21 Nov 2024 at 06:12, Cheng Jiang <quic_chejiang@quicinc.com> wrote:
>>
>> Hi Dmitry,
>>
>> On 11/20/2024 6:44 PM, Dmitry Baryshkov wrote:
>>> On Wed, Nov 20, 2024 at 05:54:26PM +0800, Cheng Jiang wrote:
>>>> Add QCA6698 qcom,qca6698-bt compatible strings.
>>>
>>> Why? Is it the same chip as WCN6855 or a different chip? Is it
>>> completely compatible?
>>>
>> They are different chips. But it's compatible with WCN6855.
> 
> So, do we really need new compat? Will/can it use the same firmware?
We need to use a different firmware. Let me check if using 
"firmware-name" allows us to omit the new soc type. 
From the driver's perspective, the only change is the need to load a 
different firmware.

> 
>>>>
>>>> Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
>>>> ---
>>>>  .../devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml   | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
>>>> index 9019fe7bcdc6..527f947289af 100644
>>>> --- a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
>>>> +++ b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
>>>> @@ -18,6 +18,7 @@ properties:
>>>>      enum:
>>>>        - qcom,qca2066-bt
>>>>        - qcom,qca6174-bt
>>>> +      - qcom,qca6698-bt
>>>>        - qcom,qca9377-bt
>>>>        - qcom,wcn3988-bt
>>>>        - qcom,wcn3990-bt
>>>> @@ -175,6 +176,7 @@ allOf:
>>>>          compatible:
>>>>            contains:
>>>>              enum:
>>>> +              - qcom,qca6698-bt
>>>>                - qcom,wcn6855-bt
>>>>      then:
>>>>        required:
>>>> --
>>>> 2.25.1
>>>>
>>>
>>
> 
> 

Re: [PATCH v2 2/4] dt-bindings: bluetooth: Add qca6698 compatible string

[quic_zijuhu]

On 11/22/2024 9:55 AM, Cheng Jiang (IOE) wrote:
> Hi Dmitry,
> 
> On 11/22/2024 12:28 AM, Dmitry Baryshkov wrote:
>> On Thu, 21 Nov 2024 at 06:12, Cheng Jiang <quic_chejiang@quicinc.com> wrote:
>>>
>>> Hi Dmitry,
>>>
>>> On 11/20/2024 6:44 PM, Dmitry Baryshkov wrote:
>>>> On Wed, Nov 20, 2024 at 05:54:26PM +0800, Cheng Jiang wrote:
>>>>> Add QCA6698 qcom,qca6698-bt compatible strings.
>>>>
>>>> Why? Is it the same chip as WCN6855 or a different chip? Is it
>>>> completely compatible?
>>>>
>>> They are different chips. But it's compatible with WCN6855.
>>
>> So, do we really need new compat? Will/can it use the same firmware?
> We need to use a different firmware. Let me check if using 
> "firmware-name" allows us to omit the new soc type. 
> From the driver's perspective, the only change is the need to load a 
> different firmware.
> 

it is a good idea to use existing optional property firmware-name to
specify RAMPATCH additionally. that would simplify logic a lot for your
requirements. (^^)(^^).

>>
>>>>>
>>>>> Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
>>>>> ---
>>>>>  .../devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml   | 2 ++
>>>>>  1 file changed, 2 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
>>>>> index 9019fe7bcdc6..527f947289af 100644
>>>>> --- a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
>>>>> +++ b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
>>>>> @@ -18,6 +18,7 @@ properties:
>>>>>      enum:
>>>>>        - qcom,qca2066-bt
>>>>>        - qcom,qca6174-bt
>>>>> +      - qcom,qca6698-bt
>>>>>        - qcom,qca9377-bt
>>>>>        - qcom,wcn3988-bt
>>>>>        - qcom,wcn3990-bt
>>>>> @@ -175,6 +176,7 @@ allOf:
>>>>>          compatible:
>>>>>            contains:
>>>>>              enum:
>>>>> +              - qcom,qca6698-bt
>>>>>                - qcom,wcn6855-bt
>>>>>      then:
>>>>>        required:
>>>>> --
>>>>> 2.25.1
>>>>>
>>>>
>>>
>>
>>
> 

Re: [PATCH v2 2/4] dt-bindings: bluetooth: Add qca6698 compatible string

[Dmitry Baryshkov]

On Fri, 22 Nov 2024 at 03:55, Cheng Jiang (IOE)
<quic_chejiang@quicinc.com> wrote:
>
> Hi Dmitry,
>
> On 11/22/2024 12:28 AM, Dmitry Baryshkov wrote:
> > On Thu, 21 Nov 2024 at 06:12, Cheng Jiang <quic_chejiang@quicinc.com> wrote:
> >>
> >> Hi Dmitry,
> >>
> >> On 11/20/2024 6:44 PM, Dmitry Baryshkov wrote:
> >>> On Wed, Nov 20, 2024 at 05:54:26PM +0800, Cheng Jiang wrote:
> >>>> Add QCA6698 qcom,qca6698-bt compatible strings.
> >>>
> >>> Why? Is it the same chip as WCN6855 or a different chip? Is it
> >>> completely compatible?
> >>>
> >> They are different chips. But it's compatible with WCN6855.
> >
> > So, do we really need new compat? Will/can it use the same firmware?
> We need to use a different firmware.

Need because of the product needs or need because of the existing
firmware not working with the chip?
Wait... your WiFi colleagues were more helpful and they wrote that "it
has different RF,
IPA, thermal, RAM size and etc, so new firmware files used." ([1]).
Please include that information in your commit messages too to let
reviewers understand  what is going on.

[1] https://lore.kernel.org/linux-arm-msm/20241024002514.92290-1-quic_miaoqing@quicinc.com/

> Let me check if using
> "firmware-name" allows us to omit the new soc type.
> From the driver's perspective, the only change is the need to load a
> different firmware.

If you want to emphasise that it is not just WCN6855, extend schema to
use fallback compatibles:
compat = "qcom,qca6698-bt", "qcom,wcn6855-bt"; No driver changes are
necessary with this approach.


>
> >
> >>>>
> >>>> Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
> >>>> ---
> >>>>  .../devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml   | 2 ++
> >>>>  1 file changed, 2 insertions(+)
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
> >>>> index 9019fe7bcdc6..527f947289af 100644
> >>>> --- a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
> >>>> +++ b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
> >>>> @@ -18,6 +18,7 @@ properties:
> >>>>      enum:
> >>>>        - qcom,qca2066-bt
> >>>>        - qcom,qca6174-bt
> >>>> +      - qcom,qca6698-bt
> >>>>        - qcom,qca9377-bt
> >>>>        - qcom,wcn3988-bt
> >>>>        - qcom,wcn3990-bt
> >>>> @@ -175,6 +176,7 @@ allOf:
> >>>>          compatible:
> >>>>            contains:
> >>>>              enum:
> >>>> +              - qcom,qca6698-bt
> >>>>                - qcom,wcn6855-bt
> >>>>      then:
> >>>>        required:
> >>>> --
> >>>> 2.25.1
> >>>>
> >>>
> >>
> >
> >
>


--
With best wishes
Dmitry

Re: [PATCH v2 2/4] dt-bindings: bluetooth: Add qca6698 compatible string

[Krzysztof Kozlowski]

On 20/11/2024 10:54, Cheng Jiang wrote:
> Add QCA6698 qcom,qca6698-bt compatible strings.
We see this from the diff. Say something useful...

Best regards,
Krzysztof

[PATCH v2 3/4] arm64: dts: qcom: sa8775p-ride: update BT nodes

[Cheng Jiang]

Add product-variant property to specify the IoT product line.
Update the chip soc type, SA8775P-ride platform uses the QCA6698
chip, which is compatible with the WCN6855. It's necessary to use this
new SoC type to distinguish it from projects using WCN chips.

Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi b/arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi
index 3fc62e123689..da52f425c676 100644
--- a/arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi
+++ b/arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi
@@ -856,7 +856,8 @@ &uart17 {
 	status = "okay";
 
 	bluetooth {
-		compatible = "qcom,wcn6855-bt";
+		compatible = "qcom,qca6698-bt";
+		qcom,product-variant = <0x20000>;
 
 		vddrfacmn-supply = <&vreg_pmu_rfa_cmn>;
 		vddaon-supply = <&vreg_pmu_aon_0p59>;
-- 
2.25.1

Re: [PATCH v2 3/4] arm64: dts: qcom: sa8775p-ride: update BT nodes

[Dmitry Baryshkov]

On Wed, Nov 20, 2024 at 05:54:27PM +0800, Cheng Jiang wrote:
> Add product-variant property to specify the IoT product line.
> Update the chip soc type, SA8775P-ride platform uses the QCA6698
> chip, which is compatible with the WCN6855. It's necessary to use this
> new SoC type to distinguish it from projects using WCN chips.

Why?

> 
> Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi b/arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi
> index 3fc62e123689..da52f425c676 100644
> --- a/arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi
> @@ -856,7 +856,8 @@ &uart17 {
>  	status = "okay";
>  
>  	bluetooth {
> -		compatible = "qcom,wcn6855-bt";
> +		compatible = "qcom,qca6698-bt";
> +		qcom,product-variant = <0x20000>;
>  
>  		vddrfacmn-supply = <&vreg_pmu_rfa_cmn>;
>  		vddaon-supply = <&vreg_pmu_aon_0p59>;
> -- 
> 2.25.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH v2 3/4] arm64: dts: qcom: sa8775p-ride: update BT nodes

[Krzysztof Kozlowski]

On 20/11/2024 10:54, Cheng Jiang wrote:
> Add product-variant property to specify the IoT product line.
> Update the chip soc type, SA8775P-ride platform uses the QCA6698
> chip, which is compatible with the WCN6855. It's necessary to use this
> new SoC type to distinguish it from projects using WCN chips.
> 
> Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi b/arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi
> index 3fc62e123689..da52f425c676 100644
> --- a/arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi
> @@ -856,7 +856,8 @@ &uart17 {
>  	status = "okay";
>  
>  	bluetooth {
> -		compatible = "qcom,wcn6855-bt";
> +		compatible = "qcom,qca6698-bt";

Breaks users.

> +		qcom,product-variant = <0x20000>;

Nope.

>  
>  		vddrfacmn-supply = <&vreg_pmu_rfa_cmn>;
>  		vddaon-supply = <&vreg_pmu_aon_0p59>;


Best regards,
Krzysztof

[PATCH v2 4/4] Bluetooth: hci_qca: add qcom,product-variant properties

[Cheng Jiang]

Since different products use the same SoC chip, features cannot
be included in a single patch. Use the qcom,product-variant to
load the appropriate firmware.

The qcom,product-variant provides product line information, which
the driver uses to load firmware from different directories.

If it's not defined in dts, the default firmware will be loaded.

Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
---
 drivers/bluetooth/btqca.c   | 142 +++++++++++++++++++++++++++++-------
 drivers/bluetooth/btqca.h   |  11 ++-
 drivers/bluetooth/hci_qca.c |  73 +++++++++---------
 3 files changed, 164 insertions(+), 62 deletions(-)

diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
index dfbbac92242a..0845e5a60412 100644
--- a/drivers/bluetooth/btqca.c
+++ b/drivers/bluetooth/btqca.c
@@ -700,8 +700,79 @@ static int qca_check_bdaddr(struct hci_dev *hdev, const struct qca_fw_config *co
 	return 0;
 }
 
-static void qca_generate_hsp_nvm_name(char *fwname, size_t max_size,
-		struct qca_btsoc_version ver, u8 rom_ver, u16 bid)
+
+const char *qca_get_soc_name(enum qca_btsoc_type soc_type)
+{
+	const char *soc_name = "";
+
+	switch (soc_type) {
+	case QCA_QCA2066:
+		soc_name = "QCA2066";
+		break;
+
+	case QCA_QCA6698:
+		soc_name = "QCA6698";
+		break;
+
+	case QCA_WCN3988:
+	case QCA_WCN3990:
+	case QCA_WCN3991:
+	case QCA_WCN3998:
+		soc_name = "WCN399x";
+		break;
+
+	case QCA_WCN6750:
+		soc_name = "WCN6750";
+		break;
+
+	case QCA_WCN6855:
+		soc_name = "WCN6855";
+		break;
+
+	case QCA_WCN7850:
+		soc_name = "WCN7850";
+		break;
+
+	default:
+		soc_name = "ROME/QCA6390";
+	}
+
+	return soc_name;
+}
+EXPORT_SYMBOL_GPL(qca_get_soc_name);
+
+static void qca_get_firmware_path(enum qca_btsoc_type soc_type, char *fw_path,
+		size_t max_size, enum qca_product_type product_type)
+{
+	const char *fw_dir = NULL;
+
+	switch (product_type) {
+	case QCA_MCC:
+		fw_dir = "qca";
+		break;
+	case QCA_CE:
+		fw_dir = "qca/ce";
+		break;
+	case QCA_IOT:
+		fw_dir = "qca/iot";
+		break;
+	case QCA_AUTO:
+		fw_dir = "qca/auto";
+		break;
+	default:
+		fw_dir = "qca";
+		break;
+	}
+
+	if (product_type == QCA_IOT)
+		snprintf(fw_path, max_size, "%s/%s", fw_dir, qca_get_soc_name(soc_type));
+	else
+		snprintf(fw_path, max_size, "%s", fw_dir);
+}
+
+static void qca_generate_hsp_nvm_name(enum qca_btsoc_type soc_type, char *fwname,
+		size_t max_size, const char *fw_path, struct qca_btsoc_version ver, u8 rom_ver,
+		u16 bid)
 {
 	const char *variant;
 
@@ -712,33 +783,36 @@ static void qca_generate_hsp_nvm_name(char *fwname, size_t max_size,
 		variant = "";
 
 	if (bid == 0x0)
-		snprintf(fwname, max_size, "qca/hpnv%02x%s.bin", rom_ver, variant);
+		snprintf(fwname, max_size, "%s/hpnv%02x%s.bin", fw_path, rom_ver, variant);
 	else
-		snprintf(fwname, max_size, "qca/hpnv%02x%s.%x", rom_ver, variant, bid);
+		snprintf(fwname, max_size, "%s/hpnv%02x%s.%x", fw_path, rom_ver, variant, bid);
 }
 
-static inline void qca_get_nvm_name_generic(struct qca_fw_config *cfg,
+static inline void qca_get_nvm_name_generic(struct qca_fw_config *cfg, const char *fw_path,
 					    const char *stem, u8 rom_ver, u16 bid)
 {
 	if (bid == 0x0)
-		snprintf(cfg->fwname, sizeof(cfg->fwname), "qca/%snv%02x.bin", stem, rom_ver);
+		snprintf(cfg->fwname, sizeof(cfg->fwname),
+			 "%s/%snv%02x.bin", fw_path, stem, rom_ver);
 	else if (bid & 0xff00)
 		snprintf(cfg->fwname, sizeof(cfg->fwname),
-			 "qca/%snv%02x.b%x", stem, rom_ver, bid);
+			 "%s/%snv%02x.b%x", fw_path, stem, rom_ver, bid);
 	else
 		snprintf(cfg->fwname, sizeof(cfg->fwname),
-			 "qca/%snv%02x.b%02x", stem, rom_ver, bid);
+			 "%s/%snv%02x.b%02x", fw_path, stem, rom_ver, bid);
 }
 
 int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
 		   enum qca_btsoc_type soc_type, struct qca_btsoc_version ver,
-		   const char *firmware_name)
+		   const char *firmware_name, uint32_t product_variant)
 {
 	struct qca_fw_config config = {};
 	int err;
 	u8 rom_ver = 0;
 	u32 soc_ver;
 	u16 boardid = 0;
+	enum qca_product_type product_type;
+	char fw_path[64] = {0};
 
 	bt_dev_dbg(hdev, "QCA setup on UART");
 
@@ -759,6 +833,10 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
 	if (soc_type == QCA_WCN6750)
 		qca_send_patch_config_cmd(hdev);
 
+	/* Get the f/w path based on product variant */
+	product_type = (product_variant >> 16) & 0xff;
+	qca_get_firmware_path(soc_type, fw_path, sizeof(fw_path), product_type);
+
 	/* Download rampatch file */
 	config.type = TLV_TYPE_PATCH;
 	switch (soc_type) {
@@ -766,19 +844,23 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
 	case QCA_WCN3991:
 	case QCA_WCN3998:
 		snprintf(config.fwname, sizeof(config.fwname),
-			 "qca/crbtfw%02x.tlv", rom_ver);
+			 "%s/crbtfw%02x.tlv", fw_path, rom_ver);
 		break;
 	case QCA_WCN3988:
 		snprintf(config.fwname, sizeof(config.fwname),
-			 "qca/apbtfw%02x.tlv", rom_ver);
+			 "%s/apbtfw%02x.tlv", fw_path, rom_ver);
 		break;
 	case QCA_QCA2066:
 		snprintf(config.fwname, sizeof(config.fwname),
-			 "qca/hpbtfw%02x.tlv", rom_ver);
+			 "%s/hpbtfw%02x.tlv", fw_path, rom_ver);
 		break;
 	case QCA_QCA6390:
 		snprintf(config.fwname, sizeof(config.fwname),
-			 "qca/htbtfw%02x.tlv", rom_ver);
+			 "%s/htbtfw%02x.tlv", fw_path, rom_ver);
+		break;
+	case QCA_QCA6698:
+		snprintf(config.fwname, sizeof(config.fwname),
+			 "%s/hpbtfw%02x.tlv", fw_path, rom_ver);
 		break;
 	case QCA_WCN6750:
 		/* Choose mbn file by default.If mbn file is not found
@@ -786,19 +868,19 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
 		 */
 		config.type = ELF_TYPE_PATCH;
 		snprintf(config.fwname, sizeof(config.fwname),
-			 "qca/msbtfw%02x.mbn", rom_ver);
+			 "%s/msbtfw%02x.mbn", fw_path, rom_ver);
 		break;
 	case QCA_WCN6855:
 		snprintf(config.fwname, sizeof(config.fwname),
-			 "qca/hpbtfw%02x.tlv", rom_ver);
+			 "%s/hpbtfw%02x.tlv", fw_path, rom_ver);
 		break;
 	case QCA_WCN7850:
 		snprintf(config.fwname, sizeof(config.fwname),
-			 "qca/hmtbtfw%02x.tlv", rom_ver);
+			 "%s/hmtbtfw%02x.tlv", fw_path, rom_ver);
 		break;
 	default:
 		snprintf(config.fwname, sizeof(config.fwname),
-			 "qca/rampatch_%08x.bin", soc_ver);
+			 "%s/rampatch_%08x.bin", fw_path, soc_ver);
 	}
 
 	err = qca_download_firmware(hdev, &config, soc_type, rom_ver);
@@ -810,7 +892,8 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
 	/* Give the controller some time to get ready to receive the NVM */
 	msleep(10);
 
-	if (soc_type == QCA_QCA2066 || soc_type == QCA_WCN7850)
+	if (soc_type == QCA_QCA2066 || soc_type == QCA_WCN7850 ||
+		soc_type == QCA_QCA6698)
 		qca_read_fw_board_id(hdev, &boardid);
 
 	/* Download NVM configuration */
@@ -825,39 +908,40 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
 		case QCA_WCN3998:
 			if (le32_to_cpu(ver.soc_id) == QCA_WCN3991_SOC_ID) {
 				snprintf(config.fwname, sizeof(config.fwname),
-					 "qca/crnv%02xu.bin", rom_ver);
+					 "%s/crnv%02xu.bin", fw_path, rom_ver);
 			} else {
 				snprintf(config.fwname, sizeof(config.fwname),
-					 "qca/crnv%02x.bin", rom_ver);
+					 "%s/crnv%02x.bin", fw_path, rom_ver);
 			}
 			break;
 		case QCA_WCN3988:
 			snprintf(config.fwname, sizeof(config.fwname),
-				 "qca/apnv%02x.bin", rom_ver);
+				 "%s/apnv%02x.bin", fw_path, rom_ver);
 			break;
 		case QCA_QCA2066:
-			qca_generate_hsp_nvm_name(config.fwname,
-				sizeof(config.fwname), ver, rom_ver, boardid);
+		case QCA_QCA6698:
+			qca_generate_hsp_nvm_name(soc_type, config.fwname,
+				sizeof(config.fwname), fw_path, ver, rom_ver, boardid);
 			break;
 		case QCA_QCA6390:
 			snprintf(config.fwname, sizeof(config.fwname),
-				 "qca/htnv%02x.bin", rom_ver);
+				 "%s/htnv%02x.bin", fw_path, rom_ver);
 			break;
 		case QCA_WCN6750:
 			snprintf(config.fwname, sizeof(config.fwname),
-				 "qca/msnv%02x.bin", rom_ver);
+				 "%s/msnv%02x.bin", fw_path, rom_ver);
 			break;
 		case QCA_WCN6855:
 			snprintf(config.fwname, sizeof(config.fwname),
-				 "qca/hpnv%02x.bin", rom_ver);
+				 "%s/hpnv%02x.bin", fw_path, rom_ver);
 			break;
 		case QCA_WCN7850:
-			qca_get_nvm_name_generic(&config, "hmt", rom_ver, boardid);
+			qca_get_nvm_name_generic(&config, "hmt", fw_path, rom_ver, boardid);
 			break;
 
 		default:
 			snprintf(config.fwname, sizeof(config.fwname),
-				 "qca/nvm_%08x.bin", soc_ver);
+				 "%s/nvm_%08x.bin", fw_path, soc_ver);
 		}
 	}
 
@@ -871,6 +955,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
 	case QCA_WCN3991:
 	case QCA_QCA2066:
 	case QCA_QCA6390:
+	case QCA_QCA6698:
 	case QCA_WCN6750:
 	case QCA_WCN6855:
 	case QCA_WCN7850:
@@ -909,6 +994,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
 	case QCA_WCN6750:
 	case QCA_WCN6855:
 	case QCA_WCN7850:
+	case QCA_QCA6698:
 		/* get fw build info */
 		err = qca_read_fw_build_info(hdev);
 		if (err < 0)
diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
index bb5207d7a8c7..baa3f979d017 100644
--- a/drivers/bluetooth/btqca.h
+++ b/drivers/bluetooth/btqca.h
@@ -151,21 +151,30 @@ enum qca_btsoc_type {
 	QCA_WCN3991,
 	QCA_QCA2066,
 	QCA_QCA6390,
+	QCA_QCA6698,
 	QCA_WCN6750,
 	QCA_WCN6855,
 	QCA_WCN7850,
 };
 
+enum qca_product_type {
+	QCA_MCC = 0,
+	QCA_CE,
+	QCA_IOT,
+	QCA_AUTO,
+};
+
 #if IS_ENABLED(CONFIG_BT_QCA)
 
 int qca_set_bdaddr_rome(struct hci_dev *hdev, const bdaddr_t *bdaddr);
 int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
 		   enum qca_btsoc_type soc_type, struct qca_btsoc_version ver,
-		   const char *firmware_name);
+		   const char *firmware_name, uint32_t product_variant);
 int qca_read_soc_version(struct hci_dev *hdev, struct qca_btsoc_version *ver,
 			 enum qca_btsoc_type);
 int qca_set_bdaddr(struct hci_dev *hdev, const bdaddr_t *bdaddr);
 int qca_send_pre_shutdown_cmd(struct hci_dev *hdev);
+const char *qca_get_soc_name(enum qca_btsoc_type soc_type);
 #else
 
 static inline int qca_set_bdaddr_rome(struct hci_dev *hdev, const bdaddr_t *bdaddr)
diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 37129e6cb0eb..69fec890eb8c 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -227,6 +227,7 @@ struct qca_serdev {
 	struct qca_power *bt_power;
 	u32 init_speed;
 	u32 oper_speed;
+	u32 product_variant;
 	bool bdaddr_property_broken;
 	const char *firmware_name;
 };
@@ -1361,6 +1362,7 @@ static int qca_set_baudrate(struct hci_dev *hdev, uint8_t baudrate)
 	case QCA_WCN6750:
 	case QCA_WCN6855:
 	case QCA_WCN7850:
+	case QCA_QCA6698:
 		usleep_range(1000, 10000);
 		break;
 
@@ -1447,6 +1449,7 @@ static int qca_check_speeds(struct hci_uart *hu)
 	case QCA_WCN6750:
 	case QCA_WCN6855:
 	case QCA_WCN7850:
+	case QCA_QCA6698:
 		if (!qca_get_speed(hu, QCA_INIT_SPEED) &&
 		    !qca_get_speed(hu, QCA_OPER_SPEED))
 			return -EINVAL;
@@ -1489,6 +1492,7 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
 		case QCA_WCN6750:
 		case QCA_WCN6855:
 		case QCA_WCN7850:
+		case QCA_QCA6698:
 			hci_uart_set_flow_control(hu, true);
 			break;
 
@@ -1523,6 +1527,7 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
 		case QCA_WCN6750:
 		case QCA_WCN6855:
 		case QCA_WCN7850:
+		case QCA_QCA6698:
 			hci_uart_set_flow_control(hu, false);
 			break;
 
@@ -1803,6 +1808,7 @@ static int qca_power_on(struct hci_dev *hdev)
 	case QCA_WCN6855:
 	case QCA_WCN7850:
 	case QCA_QCA6390:
+	case QCA_QCA6698:
 		ret = qca_regulator_init(hu);
 		break;
 
@@ -1858,7 +1864,6 @@ static int qca_setup(struct hci_uart *hu)
 	int ret;
 	struct qca_btsoc_version ver;
 	struct qca_serdev *qcadev;
-	const char *soc_name;
 
 	ret = qca_check_speeds(hu);
 	if (ret)
@@ -1873,34 +1878,7 @@ static int qca_setup(struct hci_uart *hu)
 	 */
 	set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
 
-	switch (soc_type) {
-	case QCA_QCA2066:
-		soc_name = "qca2066";
-		break;
-
-	case QCA_WCN3988:
-	case QCA_WCN3990:
-	case QCA_WCN3991:
-	case QCA_WCN3998:
-		soc_name = "wcn399x";
-		break;
-
-	case QCA_WCN6750:
-		soc_name = "wcn6750";
-		break;
-
-	case QCA_WCN6855:
-		soc_name = "wcn6855";
-		break;
-
-	case QCA_WCN7850:
-		soc_name = "wcn7850";
-		break;
-
-	default:
-		soc_name = "ROME/QCA6390";
-	}
-	bt_dev_info(hdev, "setting up %s", soc_name);
+	bt_dev_info(hdev, "setting up %s", qca_get_soc_name(soc_type));
 
 	qca->memdump_state = QCA_MEMDUMP_IDLE;
 
@@ -1919,6 +1897,7 @@ static int qca_setup(struct hci_uart *hu)
 	case QCA_WCN6750:
 	case QCA_WCN6855:
 	case QCA_WCN7850:
+	case QCA_QCA6698:
 		qcadev = serdev_device_get_drvdata(hu->serdev);
 		if (qcadev->bdaddr_property_broken)
 			set_bit(HCI_QUIRK_BDADDR_PROPERTY_BROKEN, &hdev->quirks);
@@ -1952,6 +1931,7 @@ static int qca_setup(struct hci_uart *hu)
 	case QCA_WCN6750:
 	case QCA_WCN6855:
 	case QCA_WCN7850:
+	case QCA_QCA6698:
 		break;
 
 	default:
@@ -1963,7 +1943,7 @@ static int qca_setup(struct hci_uart *hu)
 
 	/* Setup patch / NVM configurations */
 	ret = qca_uart_setup(hdev, qca_baudrate, soc_type, ver,
-			firmware_name);
+			firmware_name, qcadev->product_variant);
 	if (!ret) {
 		clear_bit(QCA_IBS_DISABLED, &qca->flags);
 		qca_debugfs_init(hdev);
@@ -2089,6 +2069,20 @@ static const struct qca_device_data qca_soc_data_qca6390 __maybe_unused = {
 	.num_vregs = 0,
 };
 
+static const struct qca_device_data qca_soc_data_qca6698 __maybe_unused = {
+	.soc_type = QCA_QCA6698,
+	.vregs = (struct qca_vreg []) {
+		{ "vddio", 5000 },
+		{ "vddbtcxmx", 126000 },
+		{ "vddrfacmn", 12500 },
+		{ "vddrfa0p8", 102000 },
+		{ "vddrfa1p7", 302000 },
+		{ "vddrfa1p2", 257000 },
+	},
+	.num_vregs = 6,
+	.capabilities = QCA_CAP_WIDEBAND_SPEECH | QCA_CAP_VALID_LE_STATES,
+};
+
 static const struct qca_device_data qca_soc_data_wcn6750 __maybe_unused = {
 	.soc_type = QCA_WCN6750,
 	.vregs = (struct qca_vreg []) {
@@ -2165,7 +2159,7 @@ static void qca_power_shutdown(struct hci_uart *hu)
 		pwrseq_power_off(power->pwrseq);
 		set_bit(QCA_BT_OFF, &qca->flags);
 		return;
-        }
+	}
 
 	switch (soc_type) {
 	case QCA_WCN3988:
@@ -2179,6 +2173,7 @@ static void qca_power_shutdown(struct hci_uart *hu)
 
 	case QCA_WCN6750:
 	case QCA_WCN6855:
+	case QCA_QCA6698:
 		gpiod_set_value_cansleep(qcadev->bt_en, 0);
 		msleep(100);
 		qca_regulator_disable(qcadev);
@@ -2313,6 +2308,12 @@ static int qca_serdev_probe(struct serdev_device *serdev)
 					 &qcadev->firmware_name);
 	device_property_read_u32(&serdev->dev, "max-speed",
 				 &qcadev->oper_speed);
+	device_property_read_u32(&serdev->dev, "qcom,product-variant",
+				 &qcadev->product_variant);
+
+	if (qcadev->product_variant != 0)
+		BT_INFO("QC Product Variant: 0x%08x", qcadev->product_variant);
+
 	if (!qcadev->oper_speed)
 		BT_DBG("UART will pick default operating speed");
 
@@ -2333,6 +2334,7 @@ static int qca_serdev_probe(struct serdev_device *serdev)
 	case QCA_WCN6855:
 	case QCA_WCN7850:
 	case QCA_QCA6390:
+	case QCA_QCA6698:
 		qcadev->bt_power = devm_kzalloc(&serdev->dev,
 						sizeof(struct qca_power),
 						GFP_KERNEL);
@@ -2346,6 +2348,7 @@ static int qca_serdev_probe(struct serdev_device *serdev)
 	switch (qcadev->btsoc_type) {
 	case QCA_WCN6855:
 	case QCA_WCN7850:
+	case QCA_QCA6698:
 		if (!device_property_present(&serdev->dev, "enable-gpios")) {
 			/*
 			 * Backward compatibility with old DT sources. If the
@@ -2380,7 +2383,8 @@ static int qca_serdev_probe(struct serdev_device *serdev)
 					       GPIOD_OUT_LOW);
 		if (IS_ERR(qcadev->bt_en) &&
 		    (data->soc_type == QCA_WCN6750 ||
-		     data->soc_type == QCA_WCN6855)) {
+		     data->soc_type == QCA_WCN6855 ||
+		     data->soc_type == QCA_QCA6698)) {
 			dev_err(&serdev->dev, "failed to acquire BT_EN gpio\n");
 			return PTR_ERR(qcadev->bt_en);
 		}
@@ -2393,7 +2397,8 @@ static int qca_serdev_probe(struct serdev_device *serdev)
 		if (IS_ERR(qcadev->sw_ctrl) &&
 		    (data->soc_type == QCA_WCN6750 ||
 		     data->soc_type == QCA_WCN6855 ||
-		     data->soc_type == QCA_WCN7850)) {
+		     data->soc_type == QCA_WCN7850 ||
+		     data->soc_type == QCA_QCA6698)) {
 			dev_err(&serdev->dev, "failed to acquire SW_CTRL gpio\n");
 			return PTR_ERR(qcadev->sw_ctrl);
 		}
@@ -2475,6 +2480,7 @@ static void qca_serdev_remove(struct serdev_device *serdev)
 	case QCA_WCN6750:
 	case QCA_WCN6855:
 	case QCA_WCN7850:
+	case QCA_QCA6698:
 		if (power->vregs_on)
 			qca_power_shutdown(&qcadev->serdev_hu);
 		break;
@@ -2669,6 +2675,7 @@ static const struct of_device_id qca_bluetooth_of_match[] = {
 	{ .compatible = "qcom,qca2066-bt", .data = &qca_soc_data_qca2066},
 	{ .compatible = "qcom,qca6174-bt" },
 	{ .compatible = "qcom,qca6390-bt", .data = &qca_soc_data_qca6390},
+	{ .compatible = "qcom,qca6698-bt", .data = &qca_soc_data_qca6698},
 	{ .compatible = "qcom,qca9377-bt" },
 	{ .compatible = "qcom,wcn3988-bt", .data = &qca_soc_data_wcn3988},
 	{ .compatible = "qcom,wcn3990-bt", .data = &qca_soc_data_wcn3990},
-- 
2.25.1

Re: [PATCH v2 4/4] Bluetooth: hci_qca: add qcom,product-variant properties

[Dmitry Baryshkov]

On Wed, Nov 20, 2024 at 05:54:28PM +0800, Cheng Jiang wrote:
> Since different products use the same SoC chip, features cannot
> be included in a single patch. Use the qcom,product-variant to
> load the appropriate firmware.

I can not understand this: what kind of features are so different that
you can not include them into a single firmware image? Please enable all
users with all possible features instead of tying them to a product
segment. If I'm missing something, please provide additional
information.

> 
> The qcom,product-variant provides product line information, which
> the driver uses to load firmware from different directories.
> 
> If it's not defined in dts, the default firmware will be loaded.
> 
> Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
> ---
>  drivers/bluetooth/btqca.c   | 142 +++++++++++++++++++++++++++++-------
>  drivers/bluetooth/btqca.h   |  11 ++-
>  drivers/bluetooth/hci_qca.c |  73 +++++++++---------
>  3 files changed, 164 insertions(+), 62 deletions(-)
> 
> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
> index dfbbac92242a..0845e5a60412 100644
> --- a/drivers/bluetooth/btqca.c
> +++ b/drivers/bluetooth/btqca.c
> @@ -700,8 +700,79 @@ static int qca_check_bdaddr(struct hci_dev *hdev, const struct qca_fw_config *co
>  	return 0;
>  }
>  
> -static void qca_generate_hsp_nvm_name(char *fwname, size_t max_size,
> -		struct qca_btsoc_version ver, u8 rom_ver, u16 bid)
> +
> +const char *qca_get_soc_name(enum qca_btsoc_type soc_type)
> +{
> +	const char *soc_name = "";
> +
> +	switch (soc_type) {
> +	case QCA_QCA2066:
> +		soc_name = "QCA2066";
> +		break;
> +
> +	case QCA_QCA6698:
> +		soc_name = "QCA6698";
> +		break;
> +
> +	case QCA_WCN3988:
> +	case QCA_WCN3990:
> +	case QCA_WCN3991:
> +	case QCA_WCN3998:
> +		soc_name = "WCN399x";
> +		break;
> +
> +	case QCA_WCN6750:
> +		soc_name = "WCN6750";
> +		break;
> +
> +	case QCA_WCN6855:
> +		soc_name = "WCN6855";
> +		break;
> +
> +	case QCA_WCN7850:
> +		soc_name = "WCN7850";
> +		break;
> +
> +	default:
> +		soc_name = "ROME/QCA6390";
> +	}
> +
> +	return soc_name;
> +}
> +EXPORT_SYMBOL_GPL(qca_get_soc_name);
> +
> +static void qca_get_firmware_path(enum qca_btsoc_type soc_type, char *fw_path,
> +		size_t max_size, enum qca_product_type product_type)
> +{
> +	const char *fw_dir = NULL;
> +
> +	switch (product_type) {
> +	case QCA_MCC:
> +		fw_dir = "qca";
> +		break;
> +	case QCA_CE:
> +		fw_dir = "qca/ce";
> +		break;
> +	case QCA_IOT:
> +		fw_dir = "qca/iot";
> +		break;
> +	case QCA_AUTO:
> +		fw_dir = "qca/auto";
> +		break;
> +	default:
> +		fw_dir = "qca";
> +		break;
> +	}
> +
> +	if (product_type == QCA_IOT)
> +		snprintf(fw_path, max_size, "%s/%s", fw_dir, qca_get_soc_name(soc_type));

Why do you need even more nesting for IoT products? "qca/iot/ROME/QCA6390"
also looks strange, but perfectly possible with your patch

> +	else
> +		snprintf(fw_path, max_size, "%s", fw_dir);

Without the IoT platform you can just include a static string.

> +}
> +
> +static void qca_generate_hsp_nvm_name(enum qca_btsoc_type soc_type, char *fwname,
> +		size_t max_size, const char *fw_path, struct qca_btsoc_version ver, u8 rom_ver,
> +		u16 bid)
>  {
>  	const char *variant;
>  
> @@ -712,33 +783,36 @@ static void qca_generate_hsp_nvm_name(char *fwname, size_t max_size,
>  		variant = "";
>  
>  	if (bid == 0x0)
> -		snprintf(fwname, max_size, "qca/hpnv%02x%s.bin", rom_ver, variant);
> +		snprintf(fwname, max_size, "%s/hpnv%02x%s.bin", fw_path, rom_ver, variant);
>  	else
> -		snprintf(fwname, max_size, "qca/hpnv%02x%s.%x", rom_ver, variant, bid);
> +		snprintf(fwname, max_size, "%s/hpnv%02x%s.%x", fw_path, rom_ver, variant, bid);
>  }
>  
> -static inline void qca_get_nvm_name_generic(struct qca_fw_config *cfg,
> +static inline void qca_get_nvm_name_generic(struct qca_fw_config *cfg, const char *fw_path,
>  					    const char *stem, u8 rom_ver, u16 bid)
>  {
>  	if (bid == 0x0)
> -		snprintf(cfg->fwname, sizeof(cfg->fwname), "qca/%snv%02x.bin", stem, rom_ver);
> +		snprintf(cfg->fwname, sizeof(cfg->fwname),
> +			 "%s/%snv%02x.bin", fw_path, stem, rom_ver);
>  	else if (bid & 0xff00)
>  		snprintf(cfg->fwname, sizeof(cfg->fwname),
> -			 "qca/%snv%02x.b%x", stem, rom_ver, bid);
> +			 "%s/%snv%02x.b%x", fw_path, stem, rom_ver, bid);
>  	else
>  		snprintf(cfg->fwname, sizeof(cfg->fwname),
> -			 "qca/%snv%02x.b%02x", stem, rom_ver, bid);
> +			 "%s/%snv%02x.b%02x", fw_path, stem, rom_ver, bid);
>  }
>  
>  int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>  		   enum qca_btsoc_type soc_type, struct qca_btsoc_version ver,
> -		   const char *firmware_name)
> +		   const char *firmware_name, uint32_t product_variant)
>  {
>  	struct qca_fw_config config = {};
>  	int err;
>  	u8 rom_ver = 0;
>  	u32 soc_ver;
>  	u16 boardid = 0;
> +	enum qca_product_type product_type;
> +	char fw_path[64] = {0};

No need to init it with lame data.

>  
>  	bt_dev_dbg(hdev, "QCA setup on UART");
>  
> @@ -759,6 +833,10 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>  	if (soc_type == QCA_WCN6750)
>  		qca_send_patch_config_cmd(hdev);
>  
> +	/* Get the f/w path based on product variant */
> +	product_type = (product_variant >> 16) & 0xff;
> +	qca_get_firmware_path(soc_type, fw_path, sizeof(fw_path), product_type);
> +
>  	/* Download rampatch file */
>  	config.type = TLV_TYPE_PATCH;
>  	switch (soc_type) {
> @@ -766,19 +844,23 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>  	case QCA_WCN3991:
>  	case QCA_WCN3998:
>  		snprintf(config.fwname, sizeof(config.fwname),
> -			 "qca/crbtfw%02x.tlv", rom_ver);
> +			 "%s/crbtfw%02x.tlv", fw_path, rom_ver);
>  		break;
>  	case QCA_WCN3988:
>  		snprintf(config.fwname, sizeof(config.fwname),
> -			 "qca/apbtfw%02x.tlv", rom_ver);
> +			 "%s/apbtfw%02x.tlv", fw_path, rom_ver);
>  		break;
>  	case QCA_QCA2066:
>  		snprintf(config.fwname, sizeof(config.fwname),
> -			 "qca/hpbtfw%02x.tlv", rom_ver);
> +			 "%s/hpbtfw%02x.tlv", fw_path, rom_ver);
>  		break;
>  	case QCA_QCA6390:
>  		snprintf(config.fwname, sizeof(config.fwname),
> -			 "qca/htbtfw%02x.tlv", rom_ver);
> +			 "%s/htbtfw%02x.tlv", fw_path, rom_ver);
> +		break;
> +	case QCA_QCA6698:
> +		snprintf(config.fwname, sizeof(config.fwname),
> +			 "%s/hpbtfw%02x.tlv", fw_path, rom_ver);
>  		break;
>  	case QCA_WCN6750:
>  		/* Choose mbn file by default.If mbn file is not found
> @@ -786,19 +868,19 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>  		 */
>  		config.type = ELF_TYPE_PATCH;
>  		snprintf(config.fwname, sizeof(config.fwname),
> -			 "qca/msbtfw%02x.mbn", rom_ver);
> +			 "%s/msbtfw%02x.mbn", fw_path, rom_ver);
>  		break;
>  	case QCA_WCN6855:
>  		snprintf(config.fwname, sizeof(config.fwname),
> -			 "qca/hpbtfw%02x.tlv", rom_ver);
> +			 "%s/hpbtfw%02x.tlv", fw_path, rom_ver);
>  		break;
>  	case QCA_WCN7850:
>  		snprintf(config.fwname, sizeof(config.fwname),
> -			 "qca/hmtbtfw%02x.tlv", rom_ver);
> +			 "%s/hmtbtfw%02x.tlv", fw_path, rom_ver);
>  		break;
>  	default:
>  		snprintf(config.fwname, sizeof(config.fwname),
> -			 "qca/rampatch_%08x.bin", soc_ver);
> +			 "%s/rampatch_%08x.bin", fw_path, soc_ver);
>  	}
>  
>  	err = qca_download_firmware(hdev, &config, soc_type, rom_ver);
> @@ -810,7 +892,8 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>  	/* Give the controller some time to get ready to receive the NVM */
>  	msleep(10);
>  
> -	if (soc_type == QCA_QCA2066 || soc_type == QCA_WCN7850)
> +	if (soc_type == QCA_QCA2066 || soc_type == QCA_WCN7850 ||
> +		soc_type == QCA_QCA6698)
>  		qca_read_fw_board_id(hdev, &boardid);
>  
>  	/* Download NVM configuration */
> @@ -825,39 +908,40 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>  		case QCA_WCN3998:
>  			if (le32_to_cpu(ver.soc_id) == QCA_WCN3991_SOC_ID) {
>  				snprintf(config.fwname, sizeof(config.fwname),
> -					 "qca/crnv%02xu.bin", rom_ver);
> +					 "%s/crnv%02xu.bin", fw_path, rom_ver);
>  			} else {
>  				snprintf(config.fwname, sizeof(config.fwname),
> -					 "qca/crnv%02x.bin", rom_ver);
> +					 "%s/crnv%02x.bin", fw_path, rom_ver);
>  			}
>  			break;
>  		case QCA_WCN3988:
>  			snprintf(config.fwname, sizeof(config.fwname),
> -				 "qca/apnv%02x.bin", rom_ver);
> +				 "%s/apnv%02x.bin", fw_path, rom_ver);
>  			break;
>  		case QCA_QCA2066:
> -			qca_generate_hsp_nvm_name(config.fwname,
> -				sizeof(config.fwname), ver, rom_ver, boardid);
> +		case QCA_QCA6698:
> +			qca_generate_hsp_nvm_name(soc_type, config.fwname,
> +				sizeof(config.fwname), fw_path, ver, rom_ver, boardid);
>  			break;
>  		case QCA_QCA6390:
>  			snprintf(config.fwname, sizeof(config.fwname),
> -				 "qca/htnv%02x.bin", rom_ver);
> +				 "%s/htnv%02x.bin", fw_path, rom_ver);
>  			break;
>  		case QCA_WCN6750:
>  			snprintf(config.fwname, sizeof(config.fwname),
> -				 "qca/msnv%02x.bin", rom_ver);
> +				 "%s/msnv%02x.bin", fw_path, rom_ver);
>  			break;
>  		case QCA_WCN6855:
>  			snprintf(config.fwname, sizeof(config.fwname),
> -				 "qca/hpnv%02x.bin", rom_ver);
> +				 "%s/hpnv%02x.bin", fw_path, rom_ver);
>  			break;
>  		case QCA_WCN7850:
> -			qca_get_nvm_name_generic(&config, "hmt", rom_ver, boardid);
> +			qca_get_nvm_name_generic(&config, "hmt", fw_path, rom_ver, boardid);
>  			break;
>  
>  		default:
>  			snprintf(config.fwname, sizeof(config.fwname),
> -				 "qca/nvm_%08x.bin", soc_ver);
> +				 "%s/nvm_%08x.bin", fw_path, soc_ver);
>  		}
>  	}
>  
> @@ -871,6 +955,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>  	case QCA_WCN3991:
>  	case QCA_QCA2066:
>  	case QCA_QCA6390:
> +	case QCA_QCA6698:

This wasn't mentioned in the commit message. Please separate unrelated
changes into separate patches.

>  	case QCA_WCN6750:
>  	case QCA_WCN6855:
>  	case QCA_WCN7850:
> @@ -909,6 +994,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>  	case QCA_WCN6750:
>  	case QCA_WCN6855:
>  	case QCA_WCN7850:
> +	case QCA_QCA6698:
>  		/* get fw build info */
>  		err = qca_read_fw_build_info(hdev);
>  		if (err < 0)
> diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
> index bb5207d7a8c7..baa3f979d017 100644
> --- a/drivers/bluetooth/btqca.h
> +++ b/drivers/bluetooth/btqca.h
> @@ -151,21 +151,30 @@ enum qca_btsoc_type {
>  	QCA_WCN3991,
>  	QCA_QCA2066,
>  	QCA_QCA6390,
> +	QCA_QCA6698,
>  	QCA_WCN6750,
>  	QCA_WCN6855,
>  	QCA_WCN7850,
>  };
>  
> +enum qca_product_type {
> +	QCA_MCC = 0,
> +	QCA_CE,
> +	QCA_IOT,
> +	QCA_AUTO,

What is MCC? CE?

> +};
> +
>  #if IS_ENABLED(CONFIG_BT_QCA)
>  
>  int qca_set_bdaddr_rome(struct hci_dev *hdev, const bdaddr_t *bdaddr);
>  int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>  		   enum qca_btsoc_type soc_type, struct qca_btsoc_version ver,
> -		   const char *firmware_name);
> +		   const char *firmware_name, uint32_t product_variant);
>  int qca_read_soc_version(struct hci_dev *hdev, struct qca_btsoc_version *ver,
>  			 enum qca_btsoc_type);
>  int qca_set_bdaddr(struct hci_dev *hdev, const bdaddr_t *bdaddr);
>  int qca_send_pre_shutdown_cmd(struct hci_dev *hdev);
> +const char *qca_get_soc_name(enum qca_btsoc_type soc_type);
>  #else
>  
>  static inline int qca_set_bdaddr_rome(struct hci_dev *hdev, const bdaddr_t *bdaddr)
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index 37129e6cb0eb..69fec890eb8c 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -227,6 +227,7 @@ struct qca_serdev {
>  	struct qca_power *bt_power;
>  	u32 init_speed;
>  	u32 oper_speed;
> +	u32 product_variant;
>  	bool bdaddr_property_broken;
>  	const char *firmware_name;
>  };
> @@ -1361,6 +1362,7 @@ static int qca_set_baudrate(struct hci_dev *hdev, uint8_t baudrate)
>  	case QCA_WCN6750:
>  	case QCA_WCN6855:
>  	case QCA_WCN7850:
> +	case QCA_QCA6698:
>  		usleep_range(1000, 10000);
>  		break;
>  
> @@ -1447,6 +1449,7 @@ static int qca_check_speeds(struct hci_uart *hu)
>  	case QCA_WCN6750:
>  	case QCA_WCN6855:
>  	case QCA_WCN7850:
> +	case QCA_QCA6698:
>  		if (!qca_get_speed(hu, QCA_INIT_SPEED) &&
>  		    !qca_get_speed(hu, QCA_OPER_SPEED))
>  			return -EINVAL;
> @@ -1489,6 +1492,7 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
>  		case QCA_WCN6750:
>  		case QCA_WCN6855:
>  		case QCA_WCN7850:
> +		case QCA_QCA6698:
>  			hci_uart_set_flow_control(hu, true);
>  			break;
>  
> @@ -1523,6 +1527,7 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
>  		case QCA_WCN6750:
>  		case QCA_WCN6855:
>  		case QCA_WCN7850:
> +		case QCA_QCA6698:
>  			hci_uart_set_flow_control(hu, false);
>  			break;
>  
> @@ -1803,6 +1808,7 @@ static int qca_power_on(struct hci_dev *hdev)
>  	case QCA_WCN6855:
>  	case QCA_WCN7850:
>  	case QCA_QCA6390:
> +	case QCA_QCA6698:
>  		ret = qca_regulator_init(hu);
>  		break;
>  
> @@ -1858,7 +1864,6 @@ static int qca_setup(struct hci_uart *hu)
>  	int ret;
>  	struct qca_btsoc_version ver;
>  	struct qca_serdev *qcadev;
> -	const char *soc_name;
>  
>  	ret = qca_check_speeds(hu);
>  	if (ret)
> @@ -1873,34 +1878,7 @@ static int qca_setup(struct hci_uart *hu)
>  	 */
>  	set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
>  
> -	switch (soc_type) {
> -	case QCA_QCA2066:
> -		soc_name = "qca2066";
> -		break;
> -
> -	case QCA_WCN3988:
> -	case QCA_WCN3990:
> -	case QCA_WCN3991:
> -	case QCA_WCN3998:
> -		soc_name = "wcn399x";
> -		break;
> -
> -	case QCA_WCN6750:
> -		soc_name = "wcn6750";
> -		break;
> -
> -	case QCA_WCN6855:
> -		soc_name = "wcn6855";
> -		break;
> -
> -	case QCA_WCN7850:
> -		soc_name = "wcn7850";
> -		break;
> -
> -	default:
> -		soc_name = "ROME/QCA6390";
> -	}
> -	bt_dev_info(hdev, "setting up %s", soc_name);
> +	bt_dev_info(hdev, "setting up %s", qca_get_soc_name(soc_type));
>  
>  	qca->memdump_state = QCA_MEMDUMP_IDLE;
>  
> @@ -1919,6 +1897,7 @@ static int qca_setup(struct hci_uart *hu)
>  	case QCA_WCN6750:
>  	case QCA_WCN6855:
>  	case QCA_WCN7850:
> +	case QCA_QCA6698:
>  		qcadev = serdev_device_get_drvdata(hu->serdev);
>  		if (qcadev->bdaddr_property_broken)
>  			set_bit(HCI_QUIRK_BDADDR_PROPERTY_BROKEN, &hdev->quirks);
> @@ -1952,6 +1931,7 @@ static int qca_setup(struct hci_uart *hu)
>  	case QCA_WCN6750:
>  	case QCA_WCN6855:
>  	case QCA_WCN7850:
> +	case QCA_QCA6698:
>  		break;
>  
>  	default:
> @@ -1963,7 +1943,7 @@ static int qca_setup(struct hci_uart *hu)
>  
>  	/* Setup patch / NVM configurations */
>  	ret = qca_uart_setup(hdev, qca_baudrate, soc_type, ver,
> -			firmware_name);
> +			firmware_name, qcadev->product_variant);
>  	if (!ret) {
>  		clear_bit(QCA_IBS_DISABLED, &qca->flags);
>  		qca_debugfs_init(hdev);
> @@ -2089,6 +2069,20 @@ static const struct qca_device_data qca_soc_data_qca6390 __maybe_unused = {
>  	.num_vregs = 0,
>  };
>  
> +static const struct qca_device_data qca_soc_data_qca6698 __maybe_unused = {
> +	.soc_type = QCA_QCA6698,
> +	.vregs = (struct qca_vreg []) {
> +		{ "vddio", 5000 },
> +		{ "vddbtcxmx", 126000 },
> +		{ "vddrfacmn", 12500 },
> +		{ "vddrfa0p8", 102000 },
> +		{ "vddrfa1p7", 302000 },
> +		{ "vddrfa1p2", 257000 },

No need to describe regulators, use PMU and powerseq.

> +	},
> +	.num_vregs = 6,
> +	.capabilities = QCA_CAP_WIDEBAND_SPEECH | QCA_CAP_VALID_LE_STATES,
> +};

Why can't you use the qca_soc_data_wcn6855?

> +
>  static const struct qca_device_data qca_soc_data_wcn6750 __maybe_unused = {
>  	.soc_type = QCA_WCN6750,
>  	.vregs = (struct qca_vreg []) {
> @@ -2165,7 +2159,7 @@ static void qca_power_shutdown(struct hci_uart *hu)
>  		pwrseq_power_off(power->pwrseq);
>  		set_bit(QCA_BT_OFF, &qca->flags);
>  		return;
> -        }
> +	}

Completely unrelated, cleanups go to a separate patch.

>  
>  	switch (soc_type) {
>  	case QCA_WCN3988:
> @@ -2179,6 +2173,7 @@ static void qca_power_shutdown(struct hci_uart *hu)
>  
>  	case QCA_WCN6750:
>  	case QCA_WCN6855:
> +	case QCA_QCA6698:
>  		gpiod_set_value_cansleep(qcadev->bt_en, 0);
>  		msleep(100);
>  		qca_regulator_disable(qcadev);
> @@ -2313,6 +2308,12 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>  					 &qcadev->firmware_name);
>  	device_property_read_u32(&serdev->dev, "max-speed",
>  				 &qcadev->oper_speed);
> +	device_property_read_u32(&serdev->dev, "qcom,product-variant",
> +				 &qcadev->product_variant);
> +
> +	if (qcadev->product_variant != 0)
> +		BT_INFO("QC Product Variant: 0x%08x", qcadev->product_variant);

Don't spam users with useless hex numbers. Printing the sensible string
should be fine though.

> +
>  	if (!qcadev->oper_speed)
>  		BT_DBG("UART will pick default operating speed");
>  
> @@ -2333,6 +2334,7 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>  	case QCA_WCN6855:
>  	case QCA_WCN7850:
>  	case QCA_QCA6390:
> +	case QCA_QCA6698:
>  		qcadev->bt_power = devm_kzalloc(&serdev->dev,
>  						sizeof(struct qca_power),
>  						GFP_KERNEL);
> @@ -2346,6 +2348,7 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>  	switch (qcadev->btsoc_type) {
>  	case QCA_WCN6855:
>  	case QCA_WCN7850:
> +	case QCA_QCA6698:
>  		if (!device_property_present(&serdev->dev, "enable-gpios")) {
>  			/*
>  			 * Backward compatibility with old DT sources. If the
> @@ -2380,7 +2383,8 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>  					       GPIOD_OUT_LOW);
>  		if (IS_ERR(qcadev->bt_en) &&
>  		    (data->soc_type == QCA_WCN6750 ||
> -		     data->soc_type == QCA_WCN6855)) {
> +		     data->soc_type == QCA_WCN6855 ||
> +		     data->soc_type == QCA_QCA6698)) {
>  			dev_err(&serdev->dev, "failed to acquire BT_EN gpio\n");
>  			return PTR_ERR(qcadev->bt_en);
>  		}
> @@ -2393,7 +2397,8 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>  		if (IS_ERR(qcadev->sw_ctrl) &&
>  		    (data->soc_type == QCA_WCN6750 ||
>  		     data->soc_type == QCA_WCN6855 ||
> -		     data->soc_type == QCA_WCN7850)) {
> +		     data->soc_type == QCA_WCN7850 ||
> +		     data->soc_type == QCA_QCA6698)) {
>  			dev_err(&serdev->dev, "failed to acquire SW_CTRL gpio\n");
>  			return PTR_ERR(qcadev->sw_ctrl);
>  		}
> @@ -2475,6 +2480,7 @@ static void qca_serdev_remove(struct serdev_device *serdev)
>  	case QCA_WCN6750:
>  	case QCA_WCN6855:
>  	case QCA_WCN7850:
> +	case QCA_QCA6698:
>  		if (power->vregs_on)
>  			qca_power_shutdown(&qcadev->serdev_hu);
>  		break;
> @@ -2669,6 +2675,7 @@ static const struct of_device_id qca_bluetooth_of_match[] = {
>  	{ .compatible = "qcom,qca2066-bt", .data = &qca_soc_data_qca2066},
>  	{ .compatible = "qcom,qca6174-bt" },
>  	{ .compatible = "qcom,qca6390-bt", .data = &qca_soc_data_qca6390},
> +	{ .compatible = "qcom,qca6698-bt", .data = &qca_soc_data_qca6698},
>  	{ .compatible = "qcom,qca9377-bt" },
>  	{ .compatible = "qcom,wcn3988-bt", .data = &qca_soc_data_wcn3988},
>  	{ .compatible = "qcom,wcn3990-bt", .data = &qca_soc_data_wcn3990},
> -- 
> 2.25.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH v2 4/4] Bluetooth: hci_qca: add qcom,product-variant properties

[Cheng Jiang]

Hi Dmitry,

On 11/20/2024 6:57 PM, Dmitry Baryshkov wrote:
> On Wed, Nov 20, 2024 at 05:54:28PM +0800, Cheng Jiang wrote:
>> Since different products use the same SoC chip, features cannot
>> be included in a single patch. Use the qcom,product-variant to
>> load the appropriate firmware.
> 
> I can not understand this: what kind of features are so different that
> you can not include them into a single firmware image? Please enable all
> users with all possible features instead of tying them to a product
> segment. If I'm missing something, please provide additional
> information.
We have serveral projects for different cusomters, but may use the same 
soc chip (different boards). Customer have different requriements. For
some customer focus on A2DP SINK role, they need a high throughput PKI of
BTC, then firmware requires more optimizaiton for coexistence when acting
as SINK role. While other projects only act as A2DP Source, they need the
optimizaiton for coexistence when acting as SRC role  For basic Bluetooth
functions, they are included, but we can't included all the feature in a
signle firmware. 

For PC projects, the focus is on A2DP SRC and HFP AG roles. Meanwhile,
for IoT projects, the focus is on A2DP SINK and HFP Client roles.
 
> 
>>
>> The qcom,product-variant provides product line information, which
>> the driver uses to load firmware from different directories.
>>
>> If it's not defined in dts, the default firmware will be loaded.
>>
>> Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
>> ---
>>  drivers/bluetooth/btqca.c   | 142 +++++++++++++++++++++++++++++-------
>>  drivers/bluetooth/btqca.h   |  11 ++-
>>  drivers/bluetooth/hci_qca.c |  73 +++++++++---------
>>  3 files changed, 164 insertions(+), 62 deletions(-)
>>
>> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
>> index dfbbac92242a..0845e5a60412 100644
>> --- a/drivers/bluetooth/btqca.c
>> +++ b/drivers/bluetooth/btqca.c
>> @@ -700,8 +700,79 @@ static int qca_check_bdaddr(struct hci_dev *hdev, const struct qca_fw_config *co
>>  	return 0;
>>  }
>>  
>> -static void qca_generate_hsp_nvm_name(char *fwname, size_t max_size,
>> -		struct qca_btsoc_version ver, u8 rom_ver, u16 bid)
>> +
>> +const char *qca_get_soc_name(enum qca_btsoc_type soc_type)
>> +{
>> +	const char *soc_name = "";
>> +
>> +	switch (soc_type) {
>> +	case QCA_QCA2066:
>> +		soc_name = "QCA2066";
>> +		break;
>> +
>> +	case QCA_QCA6698:
>> +		soc_name = "QCA6698";
>> +		break;
>> +
>> +	case QCA_WCN3988:
>> +	case QCA_WCN3990:
>> +	case QCA_WCN3991:
>> +	case QCA_WCN3998:
>> +		soc_name = "WCN399x";
>> +		break;
>> +
>> +	case QCA_WCN6750:
>> +		soc_name = "WCN6750";
>> +		break;
>> +
>> +	case QCA_WCN6855:
>> +		soc_name = "WCN6855";
>> +		break;
>> +
>> +	case QCA_WCN7850:
>> +		soc_name = "WCN7850";
>> +		break;
>> +
>> +	default:
>> +		soc_name = "ROME/QCA6390";
>> +	}
>> +
>> +	return soc_name;
>> +}
>> +EXPORT_SYMBOL_GPL(qca_get_soc_name);
>> +
>> +static void qca_get_firmware_path(enum qca_btsoc_type soc_type, char *fw_path,
>> +		size_t max_size, enum qca_product_type product_type)
>> +{
>> +	const char *fw_dir = NULL;
>> +
>> +	switch (product_type) {
>> +	case QCA_MCC:
>> +		fw_dir = "qca";
>> +		break;
>> +	case QCA_CE:
>> +		fw_dir = "qca/ce";
>> +		break;
>> +	case QCA_IOT:
>> +		fw_dir = "qca/iot";
>> +		break;
>> +	case QCA_AUTO:
>> +		fw_dir = "qca/auto";
>> +		break;
>> +	default:
>> +		fw_dir = "qca";
>> +		break;
>> +	}
>> +
>> +	if (product_type == QCA_IOT)
>> +		snprintf(fw_path, max_size, "%s/%s", fw_dir, qca_get_soc_name(soc_type));
> 
> Why do you need even more nesting for IoT products? "qca/iot/ROME/QCA6390"
> also looks strange, but perfectly possible with your patch
It's intended to separate the firmware from the IoT product and the existing product.
> 
>> +	else
>> +		snprintf(fw_path, max_size, "%s", fw_dir);
> 
> Without the IoT platform you can just include a static string.
Ack.
> 
>> +}
>> +
>> +static void qca_generate_hsp_nvm_name(enum qca_btsoc_type soc_type, char *fwname,
>> +		size_t max_size, const char *fw_path, struct qca_btsoc_version ver, u8 rom_ver,
>> +		u16 bid)
>>  {
>>  	const char *variant;
>>  
>> @@ -712,33 +783,36 @@ static void qca_generate_hsp_nvm_name(char *fwname, size_t max_size,
>>  		variant = "";
>>  
>>  	if (bid == 0x0)
>> -		snprintf(fwname, max_size, "qca/hpnv%02x%s.bin", rom_ver, variant);
>> +		snprintf(fwname, max_size, "%s/hpnv%02x%s.bin", fw_path, rom_ver, variant);
>>  	else
>> -		snprintf(fwname, max_size, "qca/hpnv%02x%s.%x", rom_ver, variant, bid);
>> +		snprintf(fwname, max_size, "%s/hpnv%02x%s.%x", fw_path, rom_ver, variant, bid);
>>  }
>>  
>> -static inline void qca_get_nvm_name_generic(struct qca_fw_config *cfg,
>> +static inline void qca_get_nvm_name_generic(struct qca_fw_config *cfg, const char *fw_path,
>>  					    const char *stem, u8 rom_ver, u16 bid)
>>  {
>>  	if (bid == 0x0)
>> -		snprintf(cfg->fwname, sizeof(cfg->fwname), "qca/%snv%02x.bin", stem, rom_ver);
>> +		snprintf(cfg->fwname, sizeof(cfg->fwname),
>> +			 "%s/%snv%02x.bin", fw_path, stem, rom_ver);
>>  	else if (bid & 0xff00)
>>  		snprintf(cfg->fwname, sizeof(cfg->fwname),
>> -			 "qca/%snv%02x.b%x", stem, rom_ver, bid);
>> +			 "%s/%snv%02x.b%x", fw_path, stem, rom_ver, bid);
>>  	else
>>  		snprintf(cfg->fwname, sizeof(cfg->fwname),
>> -			 "qca/%snv%02x.b%02x", stem, rom_ver, bid);
>> +			 "%s/%snv%02x.b%02x", fw_path, stem, rom_ver, bid);
>>  }
>>  
>>  int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>>  		   enum qca_btsoc_type soc_type, struct qca_btsoc_version ver,
>> -		   const char *firmware_name)
>> +		   const char *firmware_name, uint32_t product_variant)
>>  {
>>  	struct qca_fw_config config = {};
>>  	int err;
>>  	u8 rom_ver = 0;
>>  	u32 soc_ver;
>>  	u16 boardid = 0;
>> +	enum qca_product_type product_type;
>> +	char fw_path[64] = {0};
> 
> No need to init it with lame data.
Ack.
> 
>>  
>>  	bt_dev_dbg(hdev, "QCA setup on UART");
>>  
>> @@ -759,6 +833,10 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>>  	if (soc_type == QCA_WCN6750)
>>  		qca_send_patch_config_cmd(hdev);
>>  
>> +	/* Get the f/w path based on product variant */
>> +	product_type = (product_variant >> 16) & 0xff;
>> +	qca_get_firmware_path(soc_type, fw_path, sizeof(fw_path), product_type);
>> +
>>  	/* Download rampatch file */
>>  	config.type = TLV_TYPE_PATCH;
>>  	switch (soc_type) {
>> @@ -766,19 +844,23 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>>  	case QCA_WCN3991:
>>  	case QCA_WCN3998:
>>  		snprintf(config.fwname, sizeof(config.fwname),
>> -			 "qca/crbtfw%02x.tlv", rom_ver);
>> +			 "%s/crbtfw%02x.tlv", fw_path, rom_ver);
>>  		break;
>>  	case QCA_WCN3988:
>>  		snprintf(config.fwname, sizeof(config.fwname),
>> -			 "qca/apbtfw%02x.tlv", rom_ver);
>> +			 "%s/apbtfw%02x.tlv", fw_path, rom_ver);
>>  		break;
>>  	case QCA_QCA2066:
>>  		snprintf(config.fwname, sizeof(config.fwname),
>> -			 "qca/hpbtfw%02x.tlv", rom_ver);
>> +			 "%s/hpbtfw%02x.tlv", fw_path, rom_ver);
>>  		break;
>>  	case QCA_QCA6390:
>>  		snprintf(config.fwname, sizeof(config.fwname),
>> -			 "qca/htbtfw%02x.tlv", rom_ver);
>> +			 "%s/htbtfw%02x.tlv", fw_path, rom_ver);
>> +		break;
>> +	case QCA_QCA6698:
>> +		snprintf(config.fwname, sizeof(config.fwname),
>> +			 "%s/hpbtfw%02x.tlv", fw_path, rom_ver);
>>  		break;
>>  	case QCA_WCN6750:
>>  		/* Choose mbn file by default.If mbn file is not found
>> @@ -786,19 +868,19 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>>  		 */
>>  		config.type = ELF_TYPE_PATCH;
>>  		snprintf(config.fwname, sizeof(config.fwname),
>> -			 "qca/msbtfw%02x.mbn", rom_ver);
>> +			 "%s/msbtfw%02x.mbn", fw_path, rom_ver);
>>  		break;
>>  	case QCA_WCN6855:
>>  		snprintf(config.fwname, sizeof(config.fwname),
>> -			 "qca/hpbtfw%02x.tlv", rom_ver);
>> +			 "%s/hpbtfw%02x.tlv", fw_path, rom_ver);
>>  		break;
>>  	case QCA_WCN7850:
>>  		snprintf(config.fwname, sizeof(config.fwname),
>> -			 "qca/hmtbtfw%02x.tlv", rom_ver);
>> +			 "%s/hmtbtfw%02x.tlv", fw_path, rom_ver);
>>  		break;
>>  	default:
>>  		snprintf(config.fwname, sizeof(config.fwname),
>> -			 "qca/rampatch_%08x.bin", soc_ver);
>> +			 "%s/rampatch_%08x.bin", fw_path, soc_ver);
>>  	}
>>  
>>  	err = qca_download_firmware(hdev, &config, soc_type, rom_ver);
>> @@ -810,7 +892,8 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>>  	/* Give the controller some time to get ready to receive the NVM */
>>  	msleep(10);
>>  
>> -	if (soc_type == QCA_QCA2066 || soc_type == QCA_WCN7850)
>> +	if (soc_type == QCA_QCA2066 || soc_type == QCA_WCN7850 ||
>> +		soc_type == QCA_QCA6698)
>>  		qca_read_fw_board_id(hdev, &boardid);
>>  
>>  	/* Download NVM configuration */
>> @@ -825,39 +908,40 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>>  		case QCA_WCN3998:
>>  			if (le32_to_cpu(ver.soc_id) == QCA_WCN3991_SOC_ID) {
>>  				snprintf(config.fwname, sizeof(config.fwname),
>> -					 "qca/crnv%02xu.bin", rom_ver);
>> +					 "%s/crnv%02xu.bin", fw_path, rom_ver);
>>  			} else {
>>  				snprintf(config.fwname, sizeof(config.fwname),
>> -					 "qca/crnv%02x.bin", rom_ver);
>> +					 "%s/crnv%02x.bin", fw_path, rom_ver);
>>  			}
>>  			break;
>>  		case QCA_WCN3988:
>>  			snprintf(config.fwname, sizeof(config.fwname),
>> -				 "qca/apnv%02x.bin", rom_ver);
>> +				 "%s/apnv%02x.bin", fw_path, rom_ver);
>>  			break;
>>  		case QCA_QCA2066:
>> -			qca_generate_hsp_nvm_name(config.fwname,
>> -				sizeof(config.fwname), ver, rom_ver, boardid);
>> +		case QCA_QCA6698:
>> +			qca_generate_hsp_nvm_name(soc_type, config.fwname,
>> +				sizeof(config.fwname), fw_path, ver, rom_ver, boardid);
>>  			break;
>>  		case QCA_QCA6390:
>>  			snprintf(config.fwname, sizeof(config.fwname),
>> -				 "qca/htnv%02x.bin", rom_ver);
>> +				 "%s/htnv%02x.bin", fw_path, rom_ver);
>>  			break;
>>  		case QCA_WCN6750:
>>  			snprintf(config.fwname, sizeof(config.fwname),
>> -				 "qca/msnv%02x.bin", rom_ver);
>> +				 "%s/msnv%02x.bin", fw_path, rom_ver);
>>  			break;
>>  		case QCA_WCN6855:
>>  			snprintf(config.fwname, sizeof(config.fwname),
>> -				 "qca/hpnv%02x.bin", rom_ver);
>> +				 "%s/hpnv%02x.bin", fw_path, rom_ver);
>>  			break;
>>  		case QCA_WCN7850:
>> -			qca_get_nvm_name_generic(&config, "hmt", rom_ver, boardid);
>> +			qca_get_nvm_name_generic(&config, "hmt", fw_path, rom_ver, boardid);
>>  			break;
>>  
>>  		default:
>>  			snprintf(config.fwname, sizeof(config.fwname),
>> -				 "qca/nvm_%08x.bin", soc_ver);
>> +				 "%s/nvm_%08x.bin", fw_path, soc_ver);
>>  		}
>>  	}
>>  
>> @@ -871,6 +955,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>>  	case QCA_WCN3991:
>>  	case QCA_QCA2066:
>>  	case QCA_QCA6390:
>> +	case QCA_QCA6698:
> 
> This wasn't mentioned in the commit message. Please separate unrelated
> changes into separate patches.
ACK.
> 
>>  	case QCA_WCN6750:
>>  	case QCA_WCN6855:
>>  	case QCA_WCN7850:
>> @@ -909,6 +994,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>>  	case QCA_WCN6750:
>>  	case QCA_WCN6855:
>>  	case QCA_WCN7850:
>> +	case QCA_QCA6698:
>>  		/* get fw build info */
>>  		err = qca_read_fw_build_info(hdev);
>>  		if (err < 0)
>> diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
>> index bb5207d7a8c7..baa3f979d017 100644
>> --- a/drivers/bluetooth/btqca.h
>> +++ b/drivers/bluetooth/btqca.h
>> @@ -151,21 +151,30 @@ enum qca_btsoc_type {
>>  	QCA_WCN3991,
>>  	QCA_QCA2066,
>>  	QCA_QCA6390,
>> +	QCA_QCA6698,
>>  	QCA_WCN6750,
>>  	QCA_WCN6855,
>>  	QCA_WCN7850,
>>  };
>>  
>> +enum qca_product_type {
>> +	QCA_MCC = 0,
>> +	QCA_CE,
>> +	QCA_IOT,
>> +	QCA_AUTO,
> 
> What is MCC? CE?
> 
>> +};
>> +
>>  #if IS_ENABLED(CONFIG_BT_QCA)
>>  
>>  int qca_set_bdaddr_rome(struct hci_dev *hdev, const bdaddr_t *bdaddr);
>>  int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>>  		   enum qca_btsoc_type soc_type, struct qca_btsoc_version ver,
>> -		   const char *firmware_name);
>> +		   const char *firmware_name, uint32_t product_variant);
>>  int qca_read_soc_version(struct hci_dev *hdev, struct qca_btsoc_version *ver,
>>  			 enum qca_btsoc_type);
>>  int qca_set_bdaddr(struct hci_dev *hdev, const bdaddr_t *bdaddr);
>>  int qca_send_pre_shutdown_cmd(struct hci_dev *hdev);
>> +const char *qca_get_soc_name(enum qca_btsoc_type soc_type);
>>  #else
>>  
>>  static inline int qca_set_bdaddr_rome(struct hci_dev *hdev, const bdaddr_t *bdaddr)
>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> index 37129e6cb0eb..69fec890eb8c 100644
>> --- a/drivers/bluetooth/hci_qca.c
>> +++ b/drivers/bluetooth/hci_qca.c
>> @@ -227,6 +227,7 @@ struct qca_serdev {
>>  	struct qca_power *bt_power;
>>  	u32 init_speed;
>>  	u32 oper_speed;
>> +	u32 product_variant;
>>  	bool bdaddr_property_broken;
>>  	const char *firmware_name;
>>  };
>> @@ -1361,6 +1362,7 @@ static int qca_set_baudrate(struct hci_dev *hdev, uint8_t baudrate)
>>  	case QCA_WCN6750:
>>  	case QCA_WCN6855:
>>  	case QCA_WCN7850:
>> +	case QCA_QCA6698:
>>  		usleep_range(1000, 10000);
>>  		break;
>>  
>> @@ -1447,6 +1449,7 @@ static int qca_check_speeds(struct hci_uart *hu)
>>  	case QCA_WCN6750:
>>  	case QCA_WCN6855:
>>  	case QCA_WCN7850:
>> +	case QCA_QCA6698:
>>  		if (!qca_get_speed(hu, QCA_INIT_SPEED) &&
>>  		    !qca_get_speed(hu, QCA_OPER_SPEED))
>>  			return -EINVAL;
>> @@ -1489,6 +1492,7 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
>>  		case QCA_WCN6750:
>>  		case QCA_WCN6855:
>>  		case QCA_WCN7850:
>> +		case QCA_QCA6698:
>>  			hci_uart_set_flow_control(hu, true);
>>  			break;
>>  
>> @@ -1523,6 +1527,7 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
>>  		case QCA_WCN6750:
>>  		case QCA_WCN6855:
>>  		case QCA_WCN7850:
>> +		case QCA_QCA6698:
>>  			hci_uart_set_flow_control(hu, false);
>>  			break;
>>  
>> @@ -1803,6 +1808,7 @@ static int qca_power_on(struct hci_dev *hdev)
>>  	case QCA_WCN6855:
>>  	case QCA_WCN7850:
>>  	case QCA_QCA6390:
>> +	case QCA_QCA6698:
>>  		ret = qca_regulator_init(hu);
>>  		break;
>>  
>> @@ -1858,7 +1864,6 @@ static int qca_setup(struct hci_uart *hu)
>>  	int ret;
>>  	struct qca_btsoc_version ver;
>>  	struct qca_serdev *qcadev;
>> -	const char *soc_name;
>>  
>>  	ret = qca_check_speeds(hu);
>>  	if (ret)
>> @@ -1873,34 +1878,7 @@ static int qca_setup(struct hci_uart *hu)
>>  	 */
>>  	set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
>>  
>> -	switch (soc_type) {
>> -	case QCA_QCA2066:
>> -		soc_name = "qca2066";
>> -		break;
>> -
>> -	case QCA_WCN3988:
>> -	case QCA_WCN3990:
>> -	case QCA_WCN3991:
>> -	case QCA_WCN3998:
>> -		soc_name = "wcn399x";
>> -		break;
>> -
>> -	case QCA_WCN6750:
>> -		soc_name = "wcn6750";
>> -		break;
>> -
>> -	case QCA_WCN6855:
>> -		soc_name = "wcn6855";
>> -		break;
>> -
>> -	case QCA_WCN7850:
>> -		soc_name = "wcn7850";
>> -		break;
>> -
>> -	default:
>> -		soc_name = "ROME/QCA6390";
>> -	}
>> -	bt_dev_info(hdev, "setting up %s", soc_name);
>> +	bt_dev_info(hdev, "setting up %s", qca_get_soc_name(soc_type));
>>  
>>  	qca->memdump_state = QCA_MEMDUMP_IDLE;
>>  
>> @@ -1919,6 +1897,7 @@ static int qca_setup(struct hci_uart *hu)
>>  	case QCA_WCN6750:
>>  	case QCA_WCN6855:
>>  	case QCA_WCN7850:
>> +	case QCA_QCA6698:
>>  		qcadev = serdev_device_get_drvdata(hu->serdev);
>>  		if (qcadev->bdaddr_property_broken)
>>  			set_bit(HCI_QUIRK_BDADDR_PROPERTY_BROKEN, &hdev->quirks);
>> @@ -1952,6 +1931,7 @@ static int qca_setup(struct hci_uart *hu)
>>  	case QCA_WCN6750:
>>  	case QCA_WCN6855:
>>  	case QCA_WCN7850:
>> +	case QCA_QCA6698:
>>  		break;
>>  
>>  	default:
>> @@ -1963,7 +1943,7 @@ static int qca_setup(struct hci_uart *hu)
>>  
>>  	/* Setup patch / NVM configurations */
>>  	ret = qca_uart_setup(hdev, qca_baudrate, soc_type, ver,
>> -			firmware_name);
>> +			firmware_name, qcadev->product_variant);
>>  	if (!ret) {
>>  		clear_bit(QCA_IBS_DISABLED, &qca->flags);
>>  		qca_debugfs_init(hdev);
>> @@ -2089,6 +2069,20 @@ static const struct qca_device_data qca_soc_data_qca6390 __maybe_unused = {
>>  	.num_vregs = 0,
>>  };
>>  
>> +static const struct qca_device_data qca_soc_data_qca6698 __maybe_unused = {
>> +	.soc_type = QCA_QCA6698,
>> +	.vregs = (struct qca_vreg []) {
>> +		{ "vddio", 5000 },
>> +		{ "vddbtcxmx", 126000 },
>> +		{ "vddrfacmn", 12500 },
>> +		{ "vddrfa0p8", 102000 },
>> +		{ "vddrfa1p7", 302000 },
>> +		{ "vddrfa1p2", 257000 },
> 
> No need to describe regulators, use PMU and powerseq.
> 
>> +	},
>> +	.num_vregs = 6,
>> +	.capabilities = QCA_CAP_WIDEBAND_SPEECH | QCA_CAP_VALID_LE_STATES,
>> +};
> 
> Why can't you use the qca_soc_data_wcn6855?
> 
>> +
>>  static const struct qca_device_data qca_soc_data_wcn6750 __maybe_unused = {
>>  	.soc_type = QCA_WCN6750,
>>  	.vregs = (struct qca_vreg []) {
>> @@ -2165,7 +2159,7 @@ static void qca_power_shutdown(struct hci_uart *hu)
>>  		pwrseq_power_off(power->pwrseq);
>>  		set_bit(QCA_BT_OFF, &qca->flags);
>>  		return;
>> -        }
>> +	}
> 
> Completely unrelated, cleanups go to a separate patch.
Ack.
> 
>>  
>>  	switch (soc_type) {
>>  	case QCA_WCN3988:
>> @@ -2179,6 +2173,7 @@ static void qca_power_shutdown(struct hci_uart *hu)
>>  
>>  	case QCA_WCN6750:
>>  	case QCA_WCN6855:
>> +	case QCA_QCA6698:
>>  		gpiod_set_value_cansleep(qcadev->bt_en, 0);
>>  		msleep(100);
>>  		qca_regulator_disable(qcadev);
>> @@ -2313,6 +2308,12 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>>  					 &qcadev->firmware_name);
>>  	device_property_read_u32(&serdev->dev, "max-speed",
>>  				 &qcadev->oper_speed);
>> +	device_property_read_u32(&serdev->dev, "qcom,product-variant",
>> +				 &qcadev->product_variant);
>> +
>> +	if (qcadev->product_variant != 0)
>> +		BT_INFO("QC Product Variant: 0x%08x", qcadev->product_variant);
> 
> Don't spam users with useless hex numbers. Printing the sensible string
> should be fine though.
> 
>> +
>>  	if (!qcadev->oper_speed)
>>  		BT_DBG("UART will pick default operating speed");
>>  
>> @@ -2333,6 +2334,7 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>>  	case QCA_WCN6855:
>>  	case QCA_WCN7850:
>>  	case QCA_QCA6390:
>> +	case QCA_QCA6698:
>>  		qcadev->bt_power = devm_kzalloc(&serdev->dev,
>>  						sizeof(struct qca_power),
>>  						GFP_KERNEL);
>> @@ -2346,6 +2348,7 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>>  	switch (qcadev->btsoc_type) {
>>  	case QCA_WCN6855:
>>  	case QCA_WCN7850:
>> +	case QCA_QCA6698:
>>  		if (!device_property_present(&serdev->dev, "enable-gpios")) {
>>  			/*
>>  			 * Backward compatibility with old DT sources. If the
>> @@ -2380,7 +2383,8 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>>  					       GPIOD_OUT_LOW);
>>  		if (IS_ERR(qcadev->bt_en) &&
>>  		    (data->soc_type == QCA_WCN6750 ||
>> -		     data->soc_type == QCA_WCN6855)) {
>> +		     data->soc_type == QCA_WCN6855 ||
>> +		     data->soc_type == QCA_QCA6698)) {
>>  			dev_err(&serdev->dev, "failed to acquire BT_EN gpio\n");
>>  			return PTR_ERR(qcadev->bt_en);
>>  		}
>> @@ -2393,7 +2397,8 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>>  		if (IS_ERR(qcadev->sw_ctrl) &&
>>  		    (data->soc_type == QCA_WCN6750 ||
>>  		     data->soc_type == QCA_WCN6855 ||
>> -		     data->soc_type == QCA_WCN7850)) {
>> +		     data->soc_type == QCA_WCN7850 ||
>> +		     data->soc_type == QCA_QCA6698)) {
>>  			dev_err(&serdev->dev, "failed to acquire SW_CTRL gpio\n");
>>  			return PTR_ERR(qcadev->sw_ctrl);
>>  		}
>> @@ -2475,6 +2480,7 @@ static void qca_serdev_remove(struct serdev_device *serdev)
>>  	case QCA_WCN6750:
>>  	case QCA_WCN6855:
>>  	case QCA_WCN7850:
>> +	case QCA_QCA6698:
>>  		if (power->vregs_on)
>>  			qca_power_shutdown(&qcadev->serdev_hu);
>>  		break;
>> @@ -2669,6 +2675,7 @@ static const struct of_device_id qca_bluetooth_of_match[] = {
>>  	{ .compatible = "qcom,qca2066-bt", .data = &qca_soc_data_qca2066},
>>  	{ .compatible = "qcom,qca6174-bt" },
>>  	{ .compatible = "qcom,qca6390-bt", .data = &qca_soc_data_qca6390},
>> +	{ .compatible = "qcom,qca6698-bt", .data = &qca_soc_data_qca6698},
>>  	{ .compatible = "qcom,qca9377-bt" },
>>  	{ .compatible = "qcom,wcn3988-bt", .data = &qca_soc_data_wcn3988},
>>  	{ .compatible = "qcom,wcn3990-bt", .data = &qca_soc_data_wcn3990},
>> -- 
>> 2.25.1
>>
> 

Re: [PATCH v2 4/4] Bluetooth: hci_qca: add qcom,product-variant properties

[Dmitry Baryshkov]

On Thu, Nov 21, 2024 at 12:40:27PM +0800, Cheng Jiang wrote:
> Hi Dmitry,
> 
> On 11/20/2024 6:57 PM, Dmitry Baryshkov wrote:
> > On Wed, Nov 20, 2024 at 05:54:28PM +0800, Cheng Jiang wrote:
> >> Since different products use the same SoC chip, features cannot
> >> be included in a single patch. Use the qcom,product-variant to
> >> load the appropriate firmware.
> > 
> > I can not understand this: what kind of features are so different that
> > you can not include them into a single firmware image? Please enable all
> > users with all possible features instead of tying them to a product
> > segment. If I'm missing something, please provide additional
> > information.
> We have serveral projects for different cusomters, but may use the same 
> soc chip (different boards). Customer have different requriements. For
> some customer focus on A2DP SINK role, they need a high throughput PKI of
> BTC, then firmware requires more optimizaiton for coexistence when acting
> as SINK role. While other projects only act as A2DP Source, they need the
> optimizaiton for coexistence when acting as SRC role  For basic Bluetooth
> functions, they are included, but we can't included all the feature in a
> signle firmware. 

This description corresponds to the use-case / software description
rather than the hardware differences. DT describes the hardware. Please
don't use DT to segment the users. If your customer needs a different
firmware, they can use distro or BSP-specific ways to implement that
instead of hardcoding this information in the hardware description.
Consider somebody using IoT as a low-power PC.

> For PC projects, the focus is on A2DP SRC and HFP AG roles. Meanwhile,
> for IoT projects, the focus is on A2DP SINK and HFP Client roles.

What does that mean for the users? Does that mean that we can not use
RBn boards as SBC devices? Or will audio quality be lowered on such
devices?

>  
> > 
> >>
> >> The qcom,product-variant provides product line information, which
> >> the driver uses to load firmware from different directories.
> >>
> >> If it's not defined in dts, the default firmware will be loaded.
> >>
> >> Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
> >> ---
> >>  drivers/bluetooth/btqca.c   | 142 +++++++++++++++++++++++++++++-------
> >>  drivers/bluetooth/btqca.h   |  11 ++-
> >>  drivers/bluetooth/hci_qca.c |  73 +++++++++---------
> >>  3 files changed, 164 insertions(+), 62 deletions(-)
> >>
> >> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
> >> index dfbbac92242a..0845e5a60412 100644
> >> --- a/drivers/bluetooth/btqca.c
> >> +++ b/drivers/bluetooth/btqca.c
> >> @@ -700,8 +700,79 @@ static int qca_check_bdaddr(struct hci_dev *hdev, const struct qca_fw_config *co
> >>  	return 0;
> >>  }
> >>  
> >> -static void qca_generate_hsp_nvm_name(char *fwname, size_t max_size,
> >> -		struct qca_btsoc_version ver, u8 rom_ver, u16 bid)
> >> +
> >> +const char *qca_get_soc_name(enum qca_btsoc_type soc_type)
> >> +{
> >> +	const char *soc_name = "";
> >> +
> >> +	switch (soc_type) {
> >> +	case QCA_QCA2066:
> >> +		soc_name = "QCA2066";
> >> +		break;
> >> +
> >> +	case QCA_QCA6698:
> >> +		soc_name = "QCA6698";
> >> +		break;
> >> +
> >> +	case QCA_WCN3988:
> >> +	case QCA_WCN3990:
> >> +	case QCA_WCN3991:
> >> +	case QCA_WCN3998:
> >> +		soc_name = "WCN399x";
> >> +		break;
> >> +
> >> +	case QCA_WCN6750:
> >> +		soc_name = "WCN6750";
> >> +		break;
> >> +
> >> +	case QCA_WCN6855:
> >> +		soc_name = "WCN6855";
> >> +		break;
> >> +
> >> +	case QCA_WCN7850:
> >> +		soc_name = "WCN7850";
> >> +		break;
> >> +
> >> +	default:
> >> +		soc_name = "ROME/QCA6390";
> >> +	}
> >> +
> >> +	return soc_name;
> >> +}
> >> +EXPORT_SYMBOL_GPL(qca_get_soc_name);
> >> +
> >> +static void qca_get_firmware_path(enum qca_btsoc_type soc_type, char *fw_path,
> >> +		size_t max_size, enum qca_product_type product_type)
> >> +{
> >> +	const char *fw_dir = NULL;
> >> +
> >> +	switch (product_type) {
> >> +	case QCA_MCC:
> >> +		fw_dir = "qca";
> >> +		break;
> >> +	case QCA_CE:
> >> +		fw_dir = "qca/ce";
> >> +		break;
> >> +	case QCA_IOT:
> >> +		fw_dir = "qca/iot";
> >> +		break;
> >> +	case QCA_AUTO:
> >> +		fw_dir = "qca/auto";
> >> +		break;
> >> +	default:
> >> +		fw_dir = "qca";
> >> +		break;
> >> +	}
> >> +
> >> +	if (product_type == QCA_IOT)
> >> +		snprintf(fw_path, max_size, "%s/%s", fw_dir, qca_get_soc_name(soc_type));
> > 
> > Why do you need even more nesting for IoT products? "qca/iot/ROME/QCA6390"
> > also looks strange, but perfectly possible with your patch
> It's intended to separate the firmware from the IoT product and the existing product.


I asked, why do you need additional nesting. Isn't just qca/iot/XXbtfwYY
enough for your usecase?

> > 
> >> +	else
> >> +		snprintf(fw_path, max_size, "%s", fw_dir);
> > 
> > Without the IoT platform you can just include a static string.
> Ack.
> > 
> >> +}
> >> +
> >> +static void qca_generate_hsp_nvm_name(enum qca_btsoc_type soc_type, char *fwname,
> >> +		size_t max_size, const char *fw_path, struct qca_btsoc_version ver, u8 rom_ver,
> >> +		u16 bid)
> >>  {
> >>  	const char *variant;
> >>  
> >> @@ -712,33 +783,36 @@ static void qca_generate_hsp_nvm_name(char *fwname, size_t max_size,
> >>  		variant = "";
> >>  
> >>  	if (bid == 0x0)
> >> -		snprintf(fwname, max_size, "qca/hpnv%02x%s.bin", rom_ver, variant);
> >> +		snprintf(fwname, max_size, "%s/hpnv%02x%s.bin", fw_path, rom_ver, variant);
> >>  	else
> >> -		snprintf(fwname, max_size, "qca/hpnv%02x%s.%x", rom_ver, variant, bid);
> >> +		snprintf(fwname, max_size, "%s/hpnv%02x%s.%x", fw_path, rom_ver, variant, bid);
> >>  }
> >>  
> >> -static inline void qca_get_nvm_name_generic(struct qca_fw_config *cfg,
> >> +static inline void qca_get_nvm_name_generic(struct qca_fw_config *cfg, const char *fw_path,
> >>  					    const char *stem, u8 rom_ver, u16 bid)
> >>  {
> >>  	if (bid == 0x0)
> >> -		snprintf(cfg->fwname, sizeof(cfg->fwname), "qca/%snv%02x.bin", stem, rom_ver);
> >> +		snprintf(cfg->fwname, sizeof(cfg->fwname),
> >> +			 "%s/%snv%02x.bin", fw_path, stem, rom_ver);
> >>  	else if (bid & 0xff00)
> >>  		snprintf(cfg->fwname, sizeof(cfg->fwname),
> >> -			 "qca/%snv%02x.b%x", stem, rom_ver, bid);
> >> +			 "%s/%snv%02x.b%x", fw_path, stem, rom_ver, bid);
> >>  	else
> >>  		snprintf(cfg->fwname, sizeof(cfg->fwname),
> >> -			 "qca/%snv%02x.b%02x", stem, rom_ver, bid);
> >> +			 "%s/%snv%02x.b%02x", fw_path, stem, rom_ver, bid);
> >>  }
> >>  
> >>  int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
> >>  		   enum qca_btsoc_type soc_type, struct qca_btsoc_version ver,
> >> -		   const char *firmware_name)
> >> +		   const char *firmware_name, uint32_t product_variant)
> >>  {
> >>  	struct qca_fw_config config = {};
> >>  	int err;
> >>  	u8 rom_ver = 0;
> >>  	u32 soc_ver;
> >>  	u16 boardid = 0;
> >> +	enum qca_product_type product_type;
> >> +	char fw_path[64] = {0};
> > 
> > No need to init it with lame data.
> Ack.
> > 
> >>  
> >>  	bt_dev_dbg(hdev, "QCA setup on UART");
> >>  
> >> @@ -759,6 +833,10 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
> >>  	if (soc_type == QCA_WCN6750)
> >>  		qca_send_patch_config_cmd(hdev);
> >>  
> >> +	/* Get the f/w path based on product variant */
> >> +	product_type = (product_variant >> 16) & 0xff;
> >> +	qca_get_firmware_path(soc_type, fw_path, sizeof(fw_path), product_type);
> >> +
> >>  	/* Download rampatch file */
> >>  	config.type = TLV_TYPE_PATCH;
> >>  	switch (soc_type) {
> >> @@ -766,19 +844,23 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
> >>  	case QCA_WCN3991:
> >>  	case QCA_WCN3998:
> >>  		snprintf(config.fwname, sizeof(config.fwname),
> >> -			 "qca/crbtfw%02x.tlv", rom_ver);
> >> +			 "%s/crbtfw%02x.tlv", fw_path, rom_ver);
> >>  		break;
> >>  	case QCA_WCN3988:
> >>  		snprintf(config.fwname, sizeof(config.fwname),
> >> -			 "qca/apbtfw%02x.tlv", rom_ver);
> >> +			 "%s/apbtfw%02x.tlv", fw_path, rom_ver);
> >>  		break;
> >>  	case QCA_QCA2066:
> >>  		snprintf(config.fwname, sizeof(config.fwname),
> >> -			 "qca/hpbtfw%02x.tlv", rom_ver);
> >> +			 "%s/hpbtfw%02x.tlv", fw_path, rom_ver);
> >>  		break;
> >>  	case QCA_QCA6390:
> >>  		snprintf(config.fwname, sizeof(config.fwname),
> >> -			 "qca/htbtfw%02x.tlv", rom_ver);
> >> +			 "%s/htbtfw%02x.tlv", fw_path, rom_ver);
> >> +		break;
> >> +	case QCA_QCA6698:
> >> +		snprintf(config.fwname, sizeof(config.fwname),
> >> +			 "%s/hpbtfw%02x.tlv", fw_path, rom_ver);
> >>  		break;
> >>  	case QCA_WCN6750:
> >>  		/* Choose mbn file by default.If mbn file is not found
> >> @@ -786,19 +868,19 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
> >>  		 */
> >>  		config.type = ELF_TYPE_PATCH;
> >>  		snprintf(config.fwname, sizeof(config.fwname),
> >> -			 "qca/msbtfw%02x.mbn", rom_ver);
> >> +			 "%s/msbtfw%02x.mbn", fw_path, rom_ver);
> >>  		break;
> >>  	case QCA_WCN6855:
> >>  		snprintf(config.fwname, sizeof(config.fwname),
> >> -			 "qca/hpbtfw%02x.tlv", rom_ver);
> >> +			 "%s/hpbtfw%02x.tlv", fw_path, rom_ver);
> >>  		break;
> >>  	case QCA_WCN7850:
> >>  		snprintf(config.fwname, sizeof(config.fwname),
> >> -			 "qca/hmtbtfw%02x.tlv", rom_ver);
> >> +			 "%s/hmtbtfw%02x.tlv", fw_path, rom_ver);
> >>  		break;
> >>  	default:
> >>  		snprintf(config.fwname, sizeof(config.fwname),
> >> -			 "qca/rampatch_%08x.bin", soc_ver);
> >> +			 "%s/rampatch_%08x.bin", fw_path, soc_ver);
> >>  	}
> >>  
> >>  	err = qca_download_firmware(hdev, &config, soc_type, rom_ver);
> >> @@ -810,7 +892,8 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
> >>  	/* Give the controller some time to get ready to receive the NVM */
> >>  	msleep(10);
> >>  
> >> -	if (soc_type == QCA_QCA2066 || soc_type == QCA_WCN7850)
> >> +	if (soc_type == QCA_QCA2066 || soc_type == QCA_WCN7850 ||
> >> +		soc_type == QCA_QCA6698)
> >>  		qca_read_fw_board_id(hdev, &boardid);
> >>  
> >>  	/* Download NVM configuration */
> >> @@ -825,39 +908,40 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
> >>  		case QCA_WCN3998:
> >>  			if (le32_to_cpu(ver.soc_id) == QCA_WCN3991_SOC_ID) {
> >>  				snprintf(config.fwname, sizeof(config.fwname),
> >> -					 "qca/crnv%02xu.bin", rom_ver);
> >> +					 "%s/crnv%02xu.bin", fw_path, rom_ver);
> >>  			} else {
> >>  				snprintf(config.fwname, sizeof(config.fwname),
> >> -					 "qca/crnv%02x.bin", rom_ver);
> >> +					 "%s/crnv%02x.bin", fw_path, rom_ver);
> >>  			}
> >>  			break;
> >>  		case QCA_WCN3988:
> >>  			snprintf(config.fwname, sizeof(config.fwname),
> >> -				 "qca/apnv%02x.bin", rom_ver);
> >> +				 "%s/apnv%02x.bin", fw_path, rom_ver);
> >>  			break;
> >>  		case QCA_QCA2066:
> >> -			qca_generate_hsp_nvm_name(config.fwname,
> >> -				sizeof(config.fwname), ver, rom_ver, boardid);
> >> +		case QCA_QCA6698:
> >> +			qca_generate_hsp_nvm_name(soc_type, config.fwname,
> >> +				sizeof(config.fwname), fw_path, ver, rom_ver, boardid);
> >>  			break;
> >>  		case QCA_QCA6390:
> >>  			snprintf(config.fwname, sizeof(config.fwname),
> >> -				 "qca/htnv%02x.bin", rom_ver);
> >> +				 "%s/htnv%02x.bin", fw_path, rom_ver);
> >>  			break;
> >>  		case QCA_WCN6750:
> >>  			snprintf(config.fwname, sizeof(config.fwname),
> >> -				 "qca/msnv%02x.bin", rom_ver);
> >> +				 "%s/msnv%02x.bin", fw_path, rom_ver);
> >>  			break;
> >>  		case QCA_WCN6855:
> >>  			snprintf(config.fwname, sizeof(config.fwname),
> >> -				 "qca/hpnv%02x.bin", rom_ver);
> >> +				 "%s/hpnv%02x.bin", fw_path, rom_ver);
> >>  			break;
> >>  		case QCA_WCN7850:
> >> -			qca_get_nvm_name_generic(&config, "hmt", rom_ver, boardid);
> >> +			qca_get_nvm_name_generic(&config, "hmt", fw_path, rom_ver, boardid);
> >>  			break;
> >>  
> >>  		default:
> >>  			snprintf(config.fwname, sizeof(config.fwname),
> >> -				 "qca/nvm_%08x.bin", soc_ver);
> >> +				 "%s/nvm_%08x.bin", fw_path, soc_ver);
> >>  		}
> >>  	}
> >>  
> >> @@ -871,6 +955,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
> >>  	case QCA_WCN3991:
> >>  	case QCA_QCA2066:
> >>  	case QCA_QCA6390:
> >> +	case QCA_QCA6698:
> > 
> > This wasn't mentioned in the commit message. Please separate unrelated
> > changes into separate patches.
> ACK.
> > 
> >>  	case QCA_WCN6750:
> >>  	case QCA_WCN6855:
> >>  	case QCA_WCN7850:
> >> @@ -909,6 +994,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
> >>  	case QCA_WCN6750:
> >>  	case QCA_WCN6855:
> >>  	case QCA_WCN7850:
> >> +	case QCA_QCA6698:
> >>  		/* get fw build info */
> >>  		err = qca_read_fw_build_info(hdev);
> >>  		if (err < 0)
> >> diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
> >> index bb5207d7a8c7..baa3f979d017 100644
> >> --- a/drivers/bluetooth/btqca.h
> >> +++ b/drivers/bluetooth/btqca.h
> >> @@ -151,21 +151,30 @@ enum qca_btsoc_type {
> >>  	QCA_WCN3991,
> >>  	QCA_QCA2066,
> >>  	QCA_QCA6390,
> >> +	QCA_QCA6698,
> >>  	QCA_WCN6750,
> >>  	QCA_WCN6855,
> >>  	QCA_WCN7850,
> >>  };
> >>  
> >> +enum qca_product_type {
> >> +	QCA_MCC = 0,
> >> +	QCA_CE,
> >> +	QCA_IOT,
> >> +	QCA_AUTO,
> > 
> > What is MCC? CE?

And the question got ignored. Sad.

> > 
> >> +};
> >> +
> >>  #if IS_ENABLED(CONFIG_BT_QCA)
> >>  
> >>  int qca_set_bdaddr_rome(struct hci_dev *hdev, const bdaddr_t *bdaddr);
> >>  int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
> >>  		   enum qca_btsoc_type soc_type, struct qca_btsoc_version ver,
> >> -		   const char *firmware_name);
> >> +		   const char *firmware_name, uint32_t product_variant);
> >>  int qca_read_soc_version(struct hci_dev *hdev, struct qca_btsoc_version *ver,
> >>  			 enum qca_btsoc_type);
> >>  int qca_set_bdaddr(struct hci_dev *hdev, const bdaddr_t *bdaddr);
> >>  int qca_send_pre_shutdown_cmd(struct hci_dev *hdev);
> >> +const char *qca_get_soc_name(enum qca_btsoc_type soc_type);
> >>  #else
> >>  
> >>  static inline int qca_set_bdaddr_rome(struct hci_dev *hdev, const bdaddr_t *bdaddr)
> >> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> >> index 37129e6cb0eb..69fec890eb8c 100644
> >> --- a/drivers/bluetooth/hci_qca.c
> >> +++ b/drivers/bluetooth/hci_qca.c
> >> @@ -227,6 +227,7 @@ struct qca_serdev {
> >>  	struct qca_power *bt_power;
> >>  	u32 init_speed;
> >>  	u32 oper_speed;
> >> +	u32 product_variant;
> >>  	bool bdaddr_property_broken;
> >>  	const char *firmware_name;
> >>  };
> >> @@ -1361,6 +1362,7 @@ static int qca_set_baudrate(struct hci_dev *hdev, uint8_t baudrate)
> >>  	case QCA_WCN6750:
> >>  	case QCA_WCN6855:
> >>  	case QCA_WCN7850:
> >> +	case QCA_QCA6698:
> >>  		usleep_range(1000, 10000);
> >>  		break;
> >>  
> >> @@ -1447,6 +1449,7 @@ static int qca_check_speeds(struct hci_uart *hu)
> >>  	case QCA_WCN6750:
> >>  	case QCA_WCN6855:
> >>  	case QCA_WCN7850:
> >> +	case QCA_QCA6698:
> >>  		if (!qca_get_speed(hu, QCA_INIT_SPEED) &&
> >>  		    !qca_get_speed(hu, QCA_OPER_SPEED))
> >>  			return -EINVAL;
> >> @@ -1489,6 +1492,7 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
> >>  		case QCA_WCN6750:
> >>  		case QCA_WCN6855:
> >>  		case QCA_WCN7850:
> >> +		case QCA_QCA6698:
> >>  			hci_uart_set_flow_control(hu, true);
> >>  			break;
> >>  
> >> @@ -1523,6 +1527,7 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
> >>  		case QCA_WCN6750:
> >>  		case QCA_WCN6855:
> >>  		case QCA_WCN7850:
> >> +		case QCA_QCA6698:
> >>  			hci_uart_set_flow_control(hu, false);
> >>  			break;
> >>  
> >> @@ -1803,6 +1808,7 @@ static int qca_power_on(struct hci_dev *hdev)
> >>  	case QCA_WCN6855:
> >>  	case QCA_WCN7850:
> >>  	case QCA_QCA6390:
> >> +	case QCA_QCA6698:
> >>  		ret = qca_regulator_init(hu);
> >>  		break;
> >>  
> >> @@ -1858,7 +1864,6 @@ static int qca_setup(struct hci_uart *hu)
> >>  	int ret;
> >>  	struct qca_btsoc_version ver;
> >>  	struct qca_serdev *qcadev;
> >> -	const char *soc_name;
> >>  
> >>  	ret = qca_check_speeds(hu);
> >>  	if (ret)
> >> @@ -1873,34 +1878,7 @@ static int qca_setup(struct hci_uart *hu)
> >>  	 */
> >>  	set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
> >>  
> >> -	switch (soc_type) {
> >> -	case QCA_QCA2066:
> >> -		soc_name = "qca2066";
> >> -		break;
> >> -
> >> -	case QCA_WCN3988:
> >> -	case QCA_WCN3990:
> >> -	case QCA_WCN3991:
> >> -	case QCA_WCN3998:
> >> -		soc_name = "wcn399x";
> >> -		break;
> >> -
> >> -	case QCA_WCN6750:
> >> -		soc_name = "wcn6750";
> >> -		break;
> >> -
> >> -	case QCA_WCN6855:
> >> -		soc_name = "wcn6855";
> >> -		break;
> >> -
> >> -	case QCA_WCN7850:
> >> -		soc_name = "wcn7850";
> >> -		break;
> >> -
> >> -	default:
> >> -		soc_name = "ROME/QCA6390";
> >> -	}
> >> -	bt_dev_info(hdev, "setting up %s", soc_name);
> >> +	bt_dev_info(hdev, "setting up %s", qca_get_soc_name(soc_type));
> >>  
> >>  	qca->memdump_state = QCA_MEMDUMP_IDLE;
> >>  
> >> @@ -1919,6 +1897,7 @@ static int qca_setup(struct hci_uart *hu)
> >>  	case QCA_WCN6750:
> >>  	case QCA_WCN6855:
> >>  	case QCA_WCN7850:
> >> +	case QCA_QCA6698:
> >>  		qcadev = serdev_device_get_drvdata(hu->serdev);
> >>  		if (qcadev->bdaddr_property_broken)
> >>  			set_bit(HCI_QUIRK_BDADDR_PROPERTY_BROKEN, &hdev->quirks);
> >> @@ -1952,6 +1931,7 @@ static int qca_setup(struct hci_uart *hu)
> >>  	case QCA_WCN6750:
> >>  	case QCA_WCN6855:
> >>  	case QCA_WCN7850:
> >> +	case QCA_QCA6698:
> >>  		break;
> >>  
> >>  	default:
> >> @@ -1963,7 +1943,7 @@ static int qca_setup(struct hci_uart *hu)
> >>  
> >>  	/* Setup patch / NVM configurations */
> >>  	ret = qca_uart_setup(hdev, qca_baudrate, soc_type, ver,
> >> -			firmware_name);
> >> +			firmware_name, qcadev->product_variant);
> >>  	if (!ret) {
> >>  		clear_bit(QCA_IBS_DISABLED, &qca->flags);
> >>  		qca_debugfs_init(hdev);
> >> @@ -2089,6 +2069,20 @@ static const struct qca_device_data qca_soc_data_qca6390 __maybe_unused = {
> >>  	.num_vregs = 0,
> >>  };
> >>  
> >> +static const struct qca_device_data qca_soc_data_qca6698 __maybe_unused = {
> >> +	.soc_type = QCA_QCA6698,
> >> +	.vregs = (struct qca_vreg []) {
> >> +		{ "vddio", 5000 },
> >> +		{ "vddbtcxmx", 126000 },
> >> +		{ "vddrfacmn", 12500 },
> >> +		{ "vddrfa0p8", 102000 },
> >> +		{ "vddrfa1p7", 302000 },
> >> +		{ "vddrfa1p2", 257000 },
> > 
> > No need to describe regulators, use PMU and powerseq.

And this one...

> > 
> >> +	},
> >> +	.num_vregs = 6,
> >> +	.capabilities = QCA_CAP_WIDEBAND_SPEECH | QCA_CAP_VALID_LE_STATES,
> >> +};
> > 
> > Why can't you use the qca_soc_data_wcn6855?

And this one...

> > 
> >> +
> >>  static const struct qca_device_data qca_soc_data_wcn6750 __maybe_unused = {
> >>  	.soc_type = QCA_WCN6750,
> >>  	.vregs = (struct qca_vreg []) {
> >> @@ -2165,7 +2159,7 @@ static void qca_power_shutdown(struct hci_uart *hu)
> >>  		pwrseq_power_off(power->pwrseq);
> >>  		set_bit(QCA_BT_OFF, &qca->flags);
> >>  		return;
> >> -        }
> >> +	}
> > 
> > Completely unrelated, cleanups go to a separate patch.
> Ack.
> > 
> >>  
> >>  	switch (soc_type) {
> >>  	case QCA_WCN3988:
> >> @@ -2179,6 +2173,7 @@ static void qca_power_shutdown(struct hci_uart *hu)
> >>  
> >>  	case QCA_WCN6750:
> >>  	case QCA_WCN6855:
> >> +	case QCA_QCA6698:
> >>  		gpiod_set_value_cansleep(qcadev->bt_en, 0);
> >>  		msleep(100);
> >>  		qca_regulator_disable(qcadev);
> >> @@ -2313,6 +2308,12 @@ static int qca_serdev_probe(struct serdev_device *serdev)
> >>  					 &qcadev->firmware_name);
> >>  	device_property_read_u32(&serdev->dev, "max-speed",
> >>  				 &qcadev->oper_speed);
> >> +	device_property_read_u32(&serdev->dev, "qcom,product-variant",
> >> +				 &qcadev->product_variant);
> >> +
> >> +	if (qcadev->product_variant != 0)
> >> +		BT_INFO("QC Product Variant: 0x%08x", qcadev->product_variant);
> > 
> > Don't spam users with useless hex numbers. Printing the sensible string
> > should be fine though.
> > 
> >> +
> >>  	if (!qcadev->oper_speed)
> >>  		BT_DBG("UART will pick default operating speed");
> >>  
> >> @@ -2333,6 +2334,7 @@ static int qca_serdev_probe(struct serdev_device *serdev)
> >>  	case QCA_WCN6855:
> >>  	case QCA_WCN7850:
> >>  	case QCA_QCA6390:
> >> +	case QCA_QCA6698:
> >>  		qcadev->bt_power = devm_kzalloc(&serdev->dev,
> >>  						sizeof(struct qca_power),
> >>  						GFP_KERNEL);
> >> @@ -2346,6 +2348,7 @@ static int qca_serdev_probe(struct serdev_device *serdev)
> >>  	switch (qcadev->btsoc_type) {
> >>  	case QCA_WCN6855:
> >>  	case QCA_WCN7850:
> >> +	case QCA_QCA6698:
> >>  		if (!device_property_present(&serdev->dev, "enable-gpios")) {
> >>  			/*
> >>  			 * Backward compatibility with old DT sources. If the
> >> @@ -2380,7 +2383,8 @@ static int qca_serdev_probe(struct serdev_device *serdev)
> >>  					       GPIOD_OUT_LOW);
> >>  		if (IS_ERR(qcadev->bt_en) &&
> >>  		    (data->soc_type == QCA_WCN6750 ||
> >> -		     data->soc_type == QCA_WCN6855)) {
> >> +		     data->soc_type == QCA_WCN6855 ||
> >> +		     data->soc_type == QCA_QCA6698)) {
> >>  			dev_err(&serdev->dev, "failed to acquire BT_EN gpio\n");
> >>  			return PTR_ERR(qcadev->bt_en);
> >>  		}
> >> @@ -2393,7 +2397,8 @@ static int qca_serdev_probe(struct serdev_device *serdev)
> >>  		if (IS_ERR(qcadev->sw_ctrl) &&
> >>  		    (data->soc_type == QCA_WCN6750 ||
> >>  		     data->soc_type == QCA_WCN6855 ||
> >> -		     data->soc_type == QCA_WCN7850)) {
> >> +		     data->soc_type == QCA_WCN7850 ||
> >> +		     data->soc_type == QCA_QCA6698)) {
> >>  			dev_err(&serdev->dev, "failed to acquire SW_CTRL gpio\n");
> >>  			return PTR_ERR(qcadev->sw_ctrl);
> >>  		}
> >> @@ -2475,6 +2480,7 @@ static void qca_serdev_remove(struct serdev_device *serdev)
> >>  	case QCA_WCN6750:
> >>  	case QCA_WCN6855:
> >>  	case QCA_WCN7850:
> >> +	case QCA_QCA6698:
> >>  		if (power->vregs_on)
> >>  			qca_power_shutdown(&qcadev->serdev_hu);
> >>  		break;
> >> @@ -2669,6 +2675,7 @@ static const struct of_device_id qca_bluetooth_of_match[] = {
> >>  	{ .compatible = "qcom,qca2066-bt", .data = &qca_soc_data_qca2066},
> >>  	{ .compatible = "qcom,qca6174-bt" },
> >>  	{ .compatible = "qcom,qca6390-bt", .data = &qca_soc_data_qca6390},
> >> +	{ .compatible = "qcom,qca6698-bt", .data = &qca_soc_data_qca6698},
> >>  	{ .compatible = "qcom,qca9377-bt" },
> >>  	{ .compatible = "qcom,wcn3988-bt", .data = &qca_soc_data_wcn3988},
> >>  	{ .compatible = "qcom,wcn3990-bt", .data = &qca_soc_data_wcn3990},
> >> -- 
> >> 2.25.1
> >>
> > 
> 

-- 
With best wishes
Dmitry

Re: [PATCH v2 4/4] Bluetooth: hci_qca: add qcom,product-variant properties

[Cheng Jiang (IOE)]

Hi Dmitry,

On 11/22/2024 2:41 AM, Dmitry Baryshkov wrote:
> On Thu, Nov 21, 2024 at 12:40:27PM +0800, Cheng Jiang wrote:
>> Hi Dmitry,
>>
>> On 11/20/2024 6:57 PM, Dmitry Baryshkov wrote:
>>> On Wed, Nov 20, 2024 at 05:54:28PM +0800, Cheng Jiang wrote:
>>>> Since different products use the same SoC chip, features cannot
>>>> be included in a single patch. Use the qcom,product-variant to
>>>> load the appropriate firmware.
>>>
>>> I can not understand this: what kind of features are so different that
>>> you can not include them into a single firmware image? Please enable all
>>> users with all possible features instead of tying them to a product
>>> segment. If I'm missing something, please provide additional
>>> information.
>> We have serveral projects for different cusomters, but may use the same 
>> soc chip (different boards). Customer have different requriements. For
>> some customer focus on A2DP SINK role, they need a high throughput PKI of
>> BTC, then firmware requires more optimizaiton for coexistence when acting
>> as SINK role. While other projects only act as A2DP Source, they need the
>> optimizaiton for coexistence when acting as SRC role  For basic Bluetooth
>> functions, they are included, but we can't included all the feature in a
>> signle firmware. 
> 
> This description corresponds to the use-case / software description
> rather than the hardware differences. DT describes the hardware. Please
> don't use DT to segment the users. If your customer needs a different
> firmware, they can use distro or BSP-specific ways to implement that
> instead of hardcoding this information in the hardware description.
> Consider somebody using IoT as a low-power PC.
> 
>> For PC projects, the focus is on A2DP SRC and HFP AG roles. Meanwhile,
>> for IoT projects, the focus is on A2DP SINK and HFP Client roles.
> 
> What does that mean for the users? Does that mean that we can not use
> RBn boards as SBC devices? Or will audio quality be lowered on such
> devices?
> 
No. It can be used as both SRC and SINK role. Audio quality is the same.
This is just to explain that different customers/products may have 
different requirements. We need both the software and hardware 
optimization to meet the requirements.

What's more, if the platform to which the BT chipsets are attached is 
Qualcomm instead of a third-party option, there may be more Qualcomm-specific 
features available.

That’s why we need to override the BT firmware rampatch sometimes. We will
use the existing property firmware-name for our purpose instead of the current
solution. Thank you for your valuable comments.

>>  
>>>
>>>>
>>>> The qcom,product-variant provides product line information, which
>>>> the driver uses to load firmware from different directories.
>>>>
>>>> If it's not defined in dts, the default firmware will be loaded.
>>>>
>>>> Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
>>>> ---
>>>>  drivers/bluetooth/btqca.c   | 142 +++++++++++++++++++++++++++++-------
>>>>  drivers/bluetooth/btqca.h   |  11 ++-
>>>>  drivers/bluetooth/hci_qca.c |  73 +++++++++---------
>>>>  3 files changed, 164 insertions(+), 62 deletions(-)
>>>>
>>>> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
>>>> index dfbbac92242a..0845e5a60412 100644
>>>> --- a/drivers/bluetooth/btqca.c
>>>> +++ b/drivers/bluetooth/btqca.c
>>>> @@ -700,8 +700,79 @@ static int qca_check_bdaddr(struct hci_dev *hdev, const struct qca_fw_config *co
>>>>  	return 0;
>>>>  }
>>>>  
>>>> -static void qca_generate_hsp_nvm_name(char *fwname, size_t max_size,
>>>> -		struct qca_btsoc_version ver, u8 rom_ver, u16 bid)
>>>> +
>>>> +const char *qca_get_soc_name(enum qca_btsoc_type soc_type)
>>>> +{
>>>> +	const char *soc_name = "";
>>>> +
>>>> +	switch (soc_type) {
>>>> +	case QCA_QCA2066:
>>>> +		soc_name = "QCA2066";
>>>> +		break;
>>>> +
>>>> +	case QCA_QCA6698:
>>>> +		soc_name = "QCA6698";
>>>> +		break;
>>>> +
>>>> +	case QCA_WCN3988:
>>>> +	case QCA_WCN3990:
>>>> +	case QCA_WCN3991:
>>>> +	case QCA_WCN3998:
>>>> +		soc_name = "WCN399x";
>>>> +		break;
>>>> +
>>>> +	case QCA_WCN6750:
>>>> +		soc_name = "WCN6750";
>>>> +		break;
>>>> +
>>>> +	case QCA_WCN6855:
>>>> +		soc_name = "WCN6855";
>>>> +		break;
>>>> +
>>>> +	case QCA_WCN7850:
>>>> +		soc_name = "WCN7850";
>>>> +		break;
>>>> +
>>>> +	default:
>>>> +		soc_name = "ROME/QCA6390";
>>>> +	}
>>>> +
>>>> +	return soc_name;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(qca_get_soc_name);
>>>> +
>>>> +static void qca_get_firmware_path(enum qca_btsoc_type soc_type, char *fw_path,
>>>> +		size_t max_size, enum qca_product_type product_type)
>>>> +{
>>>> +	const char *fw_dir = NULL;
>>>> +
>>>> +	switch (product_type) {
>>>> +	case QCA_MCC:
>>>> +		fw_dir = "qca";
>>>> +		break;
>>>> +	case QCA_CE:
>>>> +		fw_dir = "qca/ce";
>>>> +		break;
>>>> +	case QCA_IOT:
>>>> +		fw_dir = "qca/iot";
>>>> +		break;
>>>> +	case QCA_AUTO:
>>>> +		fw_dir = "qca/auto";
>>>> +		break;
>>>> +	default:
>>>> +		fw_dir = "qca";
>>>> +		break;
>>>> +	}
>>>> +
>>>> +	if (product_type == QCA_IOT)
>>>> +		snprintf(fw_path, max_size, "%s/%s", fw_dir, qca_get_soc_name(soc_type));
>>>
>>> Why do you need even more nesting for IoT products? "qca/iot/ROME/QCA6390"
>>> also looks strange, but perfectly possible with your patch
>> It's intended to separate the firmware from the IoT product and the existing product.
> 
> 
> I asked, why do you need additional nesting. Isn't just qca/iot/XXbtfwYY
> enough for your usecase?
> 
>>>
>>>> +	else
>>>> +		snprintf(fw_path, max_size, "%s", fw_dir);
>>>
>>> Without the IoT platform you can just include a static string.
>> Ack.
>>>
>>>> +}
>>>> +
>>>> +static void qca_generate_hsp_nvm_name(enum qca_btsoc_type soc_type, char *fwname,
>>>> +		size_t max_size, const char *fw_path, struct qca_btsoc_version ver, u8 rom_ver,
>>>> +		u16 bid)
>>>>  {
>>>>  	const char *variant;
>>>>  
>>>> @@ -712,33 +783,36 @@ static void qca_generate_hsp_nvm_name(char *fwname, size_t max_size,
>>>>  		variant = "";
>>>>  
>>>>  	if (bid == 0x0)
>>>> -		snprintf(fwname, max_size, "qca/hpnv%02x%s.bin", rom_ver, variant);
>>>> +		snprintf(fwname, max_size, "%s/hpnv%02x%s.bin", fw_path, rom_ver, variant);
>>>>  	else
>>>> -		snprintf(fwname, max_size, "qca/hpnv%02x%s.%x", rom_ver, variant, bid);
>>>> +		snprintf(fwname, max_size, "%s/hpnv%02x%s.%x", fw_path, rom_ver, variant, bid);
>>>>  }
>>>>  
>>>> -static inline void qca_get_nvm_name_generic(struct qca_fw_config *cfg,
>>>> +static inline void qca_get_nvm_name_generic(struct qca_fw_config *cfg, const char *fw_path,
>>>>  					    const char *stem, u8 rom_ver, u16 bid)
>>>>  {
>>>>  	if (bid == 0x0)
>>>> -		snprintf(cfg->fwname, sizeof(cfg->fwname), "qca/%snv%02x.bin", stem, rom_ver);
>>>> +		snprintf(cfg->fwname, sizeof(cfg->fwname),
>>>> +			 "%s/%snv%02x.bin", fw_path, stem, rom_ver);
>>>>  	else if (bid & 0xff00)
>>>>  		snprintf(cfg->fwname, sizeof(cfg->fwname),
>>>> -			 "qca/%snv%02x.b%x", stem, rom_ver, bid);
>>>> +			 "%s/%snv%02x.b%x", fw_path, stem, rom_ver, bid);
>>>>  	else
>>>>  		snprintf(cfg->fwname, sizeof(cfg->fwname),
>>>> -			 "qca/%snv%02x.b%02x", stem, rom_ver, bid);
>>>> +			 "%s/%snv%02x.b%02x", fw_path, stem, rom_ver, bid);
>>>>  }
>>>>  
>>>>  int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>>>>  		   enum qca_btsoc_type soc_type, struct qca_btsoc_version ver,
>>>> -		   const char *firmware_name)
>>>> +		   const char *firmware_name, uint32_t product_variant)
>>>>  {
>>>>  	struct qca_fw_config config = {};
>>>>  	int err;
>>>>  	u8 rom_ver = 0;
>>>>  	u32 soc_ver;
>>>>  	u16 boardid = 0;
>>>> +	enum qca_product_type product_type;
>>>> +	char fw_path[64] = {0};
>>>
>>> No need to init it with lame data.
>> Ack.
>>>
>>>>  
>>>>  	bt_dev_dbg(hdev, "QCA setup on UART");
>>>>  
>>>> @@ -759,6 +833,10 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>>>>  	if (soc_type == QCA_WCN6750)
>>>>  		qca_send_patch_config_cmd(hdev);
>>>>  
>>>> +	/* Get the f/w path based on product variant */
>>>> +	product_type = (product_variant >> 16) & 0xff;
>>>> +	qca_get_firmware_path(soc_type, fw_path, sizeof(fw_path), product_type);
>>>> +
>>>>  	/* Download rampatch file */
>>>>  	config.type = TLV_TYPE_PATCH;
>>>>  	switch (soc_type) {
>>>> @@ -766,19 +844,23 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>>>>  	case QCA_WCN3991:
>>>>  	case QCA_WCN3998:
>>>>  		snprintf(config.fwname, sizeof(config.fwname),
>>>> -			 "qca/crbtfw%02x.tlv", rom_ver);
>>>> +			 "%s/crbtfw%02x.tlv", fw_path, rom_ver);
>>>>  		break;
>>>>  	case QCA_WCN3988:
>>>>  		snprintf(config.fwname, sizeof(config.fwname),
>>>> -			 "qca/apbtfw%02x.tlv", rom_ver);
>>>> +			 "%s/apbtfw%02x.tlv", fw_path, rom_ver);
>>>>  		break;
>>>>  	case QCA_QCA2066:
>>>>  		snprintf(config.fwname, sizeof(config.fwname),
>>>> -			 "qca/hpbtfw%02x.tlv", rom_ver);
>>>> +			 "%s/hpbtfw%02x.tlv", fw_path, rom_ver);
>>>>  		break;
>>>>  	case QCA_QCA6390:
>>>>  		snprintf(config.fwname, sizeof(config.fwname),
>>>> -			 "qca/htbtfw%02x.tlv", rom_ver);
>>>> +			 "%s/htbtfw%02x.tlv", fw_path, rom_ver);
>>>> +		break;
>>>> +	case QCA_QCA6698:
>>>> +		snprintf(config.fwname, sizeof(config.fwname),
>>>> +			 "%s/hpbtfw%02x.tlv", fw_path, rom_ver);
>>>>  		break;
>>>>  	case QCA_WCN6750:
>>>>  		/* Choose mbn file by default.If mbn file is not found
>>>> @@ -786,19 +868,19 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>>>>  		 */
>>>>  		config.type = ELF_TYPE_PATCH;
>>>>  		snprintf(config.fwname, sizeof(config.fwname),
>>>> -			 "qca/msbtfw%02x.mbn", rom_ver);
>>>> +			 "%s/msbtfw%02x.mbn", fw_path, rom_ver);
>>>>  		break;
>>>>  	case QCA_WCN6855:
>>>>  		snprintf(config.fwname, sizeof(config.fwname),
>>>> -			 "qca/hpbtfw%02x.tlv", rom_ver);
>>>> +			 "%s/hpbtfw%02x.tlv", fw_path, rom_ver);
>>>>  		break;
>>>>  	case QCA_WCN7850:
>>>>  		snprintf(config.fwname, sizeof(config.fwname),
>>>> -			 "qca/hmtbtfw%02x.tlv", rom_ver);
>>>> +			 "%s/hmtbtfw%02x.tlv", fw_path, rom_ver);
>>>>  		break;
>>>>  	default:
>>>>  		snprintf(config.fwname, sizeof(config.fwname),
>>>> -			 "qca/rampatch_%08x.bin", soc_ver);
>>>> +			 "%s/rampatch_%08x.bin", fw_path, soc_ver);
>>>>  	}
>>>>  
>>>>  	err = qca_download_firmware(hdev, &config, soc_type, rom_ver);
>>>> @@ -810,7 +892,8 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>>>>  	/* Give the controller some time to get ready to receive the NVM */
>>>>  	msleep(10);
>>>>  
>>>> -	if (soc_type == QCA_QCA2066 || soc_type == QCA_WCN7850)
>>>> +	if (soc_type == QCA_QCA2066 || soc_type == QCA_WCN7850 ||
>>>> +		soc_type == QCA_QCA6698)
>>>>  		qca_read_fw_board_id(hdev, &boardid);
>>>>  
>>>>  	/* Download NVM configuration */
>>>> @@ -825,39 +908,40 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>>>>  		case QCA_WCN3998:
>>>>  			if (le32_to_cpu(ver.soc_id) == QCA_WCN3991_SOC_ID) {
>>>>  				snprintf(config.fwname, sizeof(config.fwname),
>>>> -					 "qca/crnv%02xu.bin", rom_ver);
>>>> +					 "%s/crnv%02xu.bin", fw_path, rom_ver);
>>>>  			} else {
>>>>  				snprintf(config.fwname, sizeof(config.fwname),
>>>> -					 "qca/crnv%02x.bin", rom_ver);
>>>> +					 "%s/crnv%02x.bin", fw_path, rom_ver);
>>>>  			}
>>>>  			break;
>>>>  		case QCA_WCN3988:
>>>>  			snprintf(config.fwname, sizeof(config.fwname),
>>>> -				 "qca/apnv%02x.bin", rom_ver);
>>>> +				 "%s/apnv%02x.bin", fw_path, rom_ver);
>>>>  			break;
>>>>  		case QCA_QCA2066:
>>>> -			qca_generate_hsp_nvm_name(config.fwname,
>>>> -				sizeof(config.fwname), ver, rom_ver, boardid);
>>>> +		case QCA_QCA6698:
>>>> +			qca_generate_hsp_nvm_name(soc_type, config.fwname,
>>>> +				sizeof(config.fwname), fw_path, ver, rom_ver, boardid);
>>>>  			break;
>>>>  		case QCA_QCA6390:
>>>>  			snprintf(config.fwname, sizeof(config.fwname),
>>>> -				 "qca/htnv%02x.bin", rom_ver);
>>>> +				 "%s/htnv%02x.bin", fw_path, rom_ver);
>>>>  			break;
>>>>  		case QCA_WCN6750:
>>>>  			snprintf(config.fwname, sizeof(config.fwname),
>>>> -				 "qca/msnv%02x.bin", rom_ver);
>>>> +				 "%s/msnv%02x.bin", fw_path, rom_ver);
>>>>  			break;
>>>>  		case QCA_WCN6855:
>>>>  			snprintf(config.fwname, sizeof(config.fwname),
>>>> -				 "qca/hpnv%02x.bin", rom_ver);
>>>> +				 "%s/hpnv%02x.bin", fw_path, rom_ver);
>>>>  			break;
>>>>  		case QCA_WCN7850:
>>>> -			qca_get_nvm_name_generic(&config, "hmt", rom_ver, boardid);
>>>> +			qca_get_nvm_name_generic(&config, "hmt", fw_path, rom_ver, boardid);
>>>>  			break;
>>>>  
>>>>  		default:
>>>>  			snprintf(config.fwname, sizeof(config.fwname),
>>>> -				 "qca/nvm_%08x.bin", soc_ver);
>>>> +				 "%s/nvm_%08x.bin", fw_path, soc_ver);
>>>>  		}
>>>>  	}
>>>>  
>>>> @@ -871,6 +955,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>>>>  	case QCA_WCN3991:
>>>>  	case QCA_QCA2066:
>>>>  	case QCA_QCA6390:
>>>> +	case QCA_QCA6698:
>>>
>>> This wasn't mentioned in the commit message. Please separate unrelated
>>> changes into separate patches.
>> ACK.
>>>
>>>>  	case QCA_WCN6750:
>>>>  	case QCA_WCN6855:
>>>>  	case QCA_WCN7850:
>>>> @@ -909,6 +994,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>>>>  	case QCA_WCN6750:
>>>>  	case QCA_WCN6855:
>>>>  	case QCA_WCN7850:
>>>> +	case QCA_QCA6698:
>>>>  		/* get fw build info */
>>>>  		err = qca_read_fw_build_info(hdev);
>>>>  		if (err < 0)
>>>> diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
>>>> index bb5207d7a8c7..baa3f979d017 100644
>>>> --- a/drivers/bluetooth/btqca.h
>>>> +++ b/drivers/bluetooth/btqca.h
>>>> @@ -151,21 +151,30 @@ enum qca_btsoc_type {
>>>>  	QCA_WCN3991,
>>>>  	QCA_QCA2066,
>>>>  	QCA_QCA6390,
>>>> +	QCA_QCA6698,
>>>>  	QCA_WCN6750,
>>>>  	QCA_WCN6855,
>>>>  	QCA_WCN7850,
>>>>  };
>>>>  
>>>> +enum qca_product_type {
>>>> +	QCA_MCC = 0,
>>>> +	QCA_CE,
>>>> +	QCA_IOT,
>>>> +	QCA_AUTO,
>>>
>>> What is MCC? CE?
> 
> And the question got ignored. Sad.
> 
>>>
>>>> +};
>>>> +
>>>>  #if IS_ENABLED(CONFIG_BT_QCA)
>>>>  
>>>>  int qca_set_bdaddr_rome(struct hci_dev *hdev, const bdaddr_t *bdaddr);
>>>>  int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>>>>  		   enum qca_btsoc_type soc_type, struct qca_btsoc_version ver,
>>>> -		   const char *firmware_name);
>>>> +		   const char *firmware_name, uint32_t product_variant);
>>>>  int qca_read_soc_version(struct hci_dev *hdev, struct qca_btsoc_version *ver,
>>>>  			 enum qca_btsoc_type);
>>>>  int qca_set_bdaddr(struct hci_dev *hdev, const bdaddr_t *bdaddr);
>>>>  int qca_send_pre_shutdown_cmd(struct hci_dev *hdev);
>>>> +const char *qca_get_soc_name(enum qca_btsoc_type soc_type);
>>>>  #else
>>>>  
>>>>  static inline int qca_set_bdaddr_rome(struct hci_dev *hdev, const bdaddr_t *bdaddr)
>>>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>>>> index 37129e6cb0eb..69fec890eb8c 100644
>>>> --- a/drivers/bluetooth/hci_qca.c
>>>> +++ b/drivers/bluetooth/hci_qca.c
>>>> @@ -227,6 +227,7 @@ struct qca_serdev {
>>>>  	struct qca_power *bt_power;
>>>>  	u32 init_speed;
>>>>  	u32 oper_speed;
>>>> +	u32 product_variant;
>>>>  	bool bdaddr_property_broken;
>>>>  	const char *firmware_name;
>>>>  };
>>>> @@ -1361,6 +1362,7 @@ static int qca_set_baudrate(struct hci_dev *hdev, uint8_t baudrate)
>>>>  	case QCA_WCN6750:
>>>>  	case QCA_WCN6855:
>>>>  	case QCA_WCN7850:
>>>> +	case QCA_QCA6698:
>>>>  		usleep_range(1000, 10000);
>>>>  		break;
>>>>  
>>>> @@ -1447,6 +1449,7 @@ static int qca_check_speeds(struct hci_uart *hu)
>>>>  	case QCA_WCN6750:
>>>>  	case QCA_WCN6855:
>>>>  	case QCA_WCN7850:
>>>> +	case QCA_QCA6698:
>>>>  		if (!qca_get_speed(hu, QCA_INIT_SPEED) &&
>>>>  		    !qca_get_speed(hu, QCA_OPER_SPEED))
>>>>  			return -EINVAL;
>>>> @@ -1489,6 +1492,7 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
>>>>  		case QCA_WCN6750:
>>>>  		case QCA_WCN6855:
>>>>  		case QCA_WCN7850:
>>>> +		case QCA_QCA6698:
>>>>  			hci_uart_set_flow_control(hu, true);
>>>>  			break;
>>>>  
>>>> @@ -1523,6 +1527,7 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
>>>>  		case QCA_WCN6750:
>>>>  		case QCA_WCN6855:
>>>>  		case QCA_WCN7850:
>>>> +		case QCA_QCA6698:
>>>>  			hci_uart_set_flow_control(hu, false);
>>>>  			break;
>>>>  
>>>> @@ -1803,6 +1808,7 @@ static int qca_power_on(struct hci_dev *hdev)
>>>>  	case QCA_WCN6855:
>>>>  	case QCA_WCN7850:
>>>>  	case QCA_QCA6390:
>>>> +	case QCA_QCA6698:
>>>>  		ret = qca_regulator_init(hu);
>>>>  		break;
>>>>  
>>>> @@ -1858,7 +1864,6 @@ static int qca_setup(struct hci_uart *hu)
>>>>  	int ret;
>>>>  	struct qca_btsoc_version ver;
>>>>  	struct qca_serdev *qcadev;
>>>> -	const char *soc_name;
>>>>  
>>>>  	ret = qca_check_speeds(hu);
>>>>  	if (ret)
>>>> @@ -1873,34 +1878,7 @@ static int qca_setup(struct hci_uart *hu)
>>>>  	 */
>>>>  	set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
>>>>  
>>>> -	switch (soc_type) {
>>>> -	case QCA_QCA2066:
>>>> -		soc_name = "qca2066";
>>>> -		break;
>>>> -
>>>> -	case QCA_WCN3988:
>>>> -	case QCA_WCN3990:
>>>> -	case QCA_WCN3991:
>>>> -	case QCA_WCN3998:
>>>> -		soc_name = "wcn399x";
>>>> -		break;
>>>> -
>>>> -	case QCA_WCN6750:
>>>> -		soc_name = "wcn6750";
>>>> -		break;
>>>> -
>>>> -	case QCA_WCN6855:
>>>> -		soc_name = "wcn6855";
>>>> -		break;
>>>> -
>>>> -	case QCA_WCN7850:
>>>> -		soc_name = "wcn7850";
>>>> -		break;
>>>> -
>>>> -	default:
>>>> -		soc_name = "ROME/QCA6390";
>>>> -	}
>>>> -	bt_dev_info(hdev, "setting up %s", soc_name);
>>>> +	bt_dev_info(hdev, "setting up %s", qca_get_soc_name(soc_type));
>>>>  
>>>>  	qca->memdump_state = QCA_MEMDUMP_IDLE;
>>>>  
>>>> @@ -1919,6 +1897,7 @@ static int qca_setup(struct hci_uart *hu)
>>>>  	case QCA_WCN6750:
>>>>  	case QCA_WCN6855:
>>>>  	case QCA_WCN7850:
>>>> +	case QCA_QCA6698:
>>>>  		qcadev = serdev_device_get_drvdata(hu->serdev);
>>>>  		if (qcadev->bdaddr_property_broken)
>>>>  			set_bit(HCI_QUIRK_BDADDR_PROPERTY_BROKEN, &hdev->quirks);
>>>> @@ -1952,6 +1931,7 @@ static int qca_setup(struct hci_uart *hu)
>>>>  	case QCA_WCN6750:
>>>>  	case QCA_WCN6855:
>>>>  	case QCA_WCN7850:
>>>> +	case QCA_QCA6698:
>>>>  		break;
>>>>  
>>>>  	default:
>>>> @@ -1963,7 +1943,7 @@ static int qca_setup(struct hci_uart *hu)
>>>>  
>>>>  	/* Setup patch / NVM configurations */
>>>>  	ret = qca_uart_setup(hdev, qca_baudrate, soc_type, ver,
>>>> -			firmware_name);
>>>> +			firmware_name, qcadev->product_variant);
>>>>  	if (!ret) {
>>>>  		clear_bit(QCA_IBS_DISABLED, &qca->flags);
>>>>  		qca_debugfs_init(hdev);
>>>> @@ -2089,6 +2069,20 @@ static const struct qca_device_data qca_soc_data_qca6390 __maybe_unused = {
>>>>  	.num_vregs = 0,
>>>>  };
>>>>  
>>>> +static const struct qca_device_data qca_soc_data_qca6698 __maybe_unused = {
>>>> +	.soc_type = QCA_QCA6698,
>>>> +	.vregs = (struct qca_vreg []) {
>>>> +		{ "vddio", 5000 },
>>>> +		{ "vddbtcxmx", 126000 },
>>>> +		{ "vddrfacmn", 12500 },
>>>> +		{ "vddrfa0p8", 102000 },
>>>> +		{ "vddrfa1p7", 302000 },
>>>> +		{ "vddrfa1p2", 257000 },
>>>
>>> No need to describe regulators, use PMU and powerseq.
> 
> And this one...
> 
>>>
>>>> +	},
>>>> +	.num_vregs = 6,
>>>> +	.capabilities = QCA_CAP_WIDEBAND_SPEECH | QCA_CAP_VALID_LE_STATES,
>>>> +};
>>>
>>> Why can't you use the qca_soc_data_wcn6855?
> 
> And this one...
> 
>>>
>>>> +
>>>>  static const struct qca_device_data qca_soc_data_wcn6750 __maybe_unused = {
>>>>  	.soc_type = QCA_WCN6750,
>>>>  	.vregs = (struct qca_vreg []) {
>>>> @@ -2165,7 +2159,7 @@ static void qca_power_shutdown(struct hci_uart *hu)
>>>>  		pwrseq_power_off(power->pwrseq);
>>>>  		set_bit(QCA_BT_OFF, &qca->flags);
>>>>  		return;
>>>> -        }
>>>> +	}
>>>
>>> Completely unrelated, cleanups go to a separate patch.
>> Ack.
>>>
>>>>  
>>>>  	switch (soc_type) {
>>>>  	case QCA_WCN3988:
>>>> @@ -2179,6 +2173,7 @@ static void qca_power_shutdown(struct hci_uart *hu)
>>>>  
>>>>  	case QCA_WCN6750:
>>>>  	case QCA_WCN6855:
>>>> +	case QCA_QCA6698:
>>>>  		gpiod_set_value_cansleep(qcadev->bt_en, 0);
>>>>  		msleep(100);
>>>>  		qca_regulator_disable(qcadev);
>>>> @@ -2313,6 +2308,12 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>>>>  					 &qcadev->firmware_name);
>>>>  	device_property_read_u32(&serdev->dev, "max-speed",
>>>>  				 &qcadev->oper_speed);
>>>> +	device_property_read_u32(&serdev->dev, "qcom,product-variant",
>>>> +				 &qcadev->product_variant);
>>>> +
>>>> +	if (qcadev->product_variant != 0)
>>>> +		BT_INFO("QC Product Variant: 0x%08x", qcadev->product_variant);
>>>
>>> Don't spam users with useless hex numbers. Printing the sensible string
>>> should be fine though.
>>>
>>>> +
>>>>  	if (!qcadev->oper_speed)
>>>>  		BT_DBG("UART will pick default operating speed");
>>>>  
>>>> @@ -2333,6 +2334,7 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>>>>  	case QCA_WCN6855:
>>>>  	case QCA_WCN7850:
>>>>  	case QCA_QCA6390:
>>>> +	case QCA_QCA6698:
>>>>  		qcadev->bt_power = devm_kzalloc(&serdev->dev,
>>>>  						sizeof(struct qca_power),
>>>>  						GFP_KERNEL);
>>>> @@ -2346,6 +2348,7 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>>>>  	switch (qcadev->btsoc_type) {
>>>>  	case QCA_WCN6855:
>>>>  	case QCA_WCN7850:
>>>> +	case QCA_QCA6698:
>>>>  		if (!device_property_present(&serdev->dev, "enable-gpios")) {
>>>>  			/*
>>>>  			 * Backward compatibility with old DT sources. If the
>>>> @@ -2380,7 +2383,8 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>>>>  					       GPIOD_OUT_LOW);
>>>>  		if (IS_ERR(qcadev->bt_en) &&
>>>>  		    (data->soc_type == QCA_WCN6750 ||
>>>> -		     data->soc_type == QCA_WCN6855)) {
>>>> +		     data->soc_type == QCA_WCN6855 ||
>>>> +		     data->soc_type == QCA_QCA6698)) {
>>>>  			dev_err(&serdev->dev, "failed to acquire BT_EN gpio\n");
>>>>  			return PTR_ERR(qcadev->bt_en);
>>>>  		}
>>>> @@ -2393,7 +2397,8 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>>>>  		if (IS_ERR(qcadev->sw_ctrl) &&
>>>>  		    (data->soc_type == QCA_WCN6750 ||
>>>>  		     data->soc_type == QCA_WCN6855 ||
>>>> -		     data->soc_type == QCA_WCN7850)) {
>>>> +		     data->soc_type == QCA_WCN7850 ||
>>>> +		     data->soc_type == QCA_QCA6698)) {
>>>>  			dev_err(&serdev->dev, "failed to acquire SW_CTRL gpio\n");
>>>>  			return PTR_ERR(qcadev->sw_ctrl);
>>>>  		}
>>>> @@ -2475,6 +2480,7 @@ static void qca_serdev_remove(struct serdev_device *serdev)
>>>>  	case QCA_WCN6750:
>>>>  	case QCA_WCN6855:
>>>>  	case QCA_WCN7850:
>>>> +	case QCA_QCA6698:
>>>>  		if (power->vregs_on)
>>>>  			qca_power_shutdown(&qcadev->serdev_hu);
>>>>  		break;
>>>> @@ -2669,6 +2675,7 @@ static const struct of_device_id qca_bluetooth_of_match[] = {
>>>>  	{ .compatible = "qcom,qca2066-bt", .data = &qca_soc_data_qca2066},
>>>>  	{ .compatible = "qcom,qca6174-bt" },
>>>>  	{ .compatible = "qcom,qca6390-bt", .data = &qca_soc_data_qca6390},
>>>> +	{ .compatible = "qcom,qca6698-bt", .data = &qca_soc_data_qca6698},
>>>>  	{ .compatible = "qcom,qca9377-bt" },
>>>>  	{ .compatible = "qcom,wcn3988-bt", .data = &qca_soc_data_wcn3988},
>>>>  	{ .compatible = "qcom,wcn3990-bt", .data = &qca_soc_data_wcn3990},
>>>> -- 
>>>> 2.25.1
>>>>
>>>
>>
> 

Re: [PATCH v2 4/4] Bluetooth: hci_qca: add qcom,product-variant properties

[neil.armstrong]

On 20/11/2024 10:54, Cheng Jiang wrote:
> Since different products use the same SoC chip, features cannot
> be included in a single patch. Use the qcom,product-variant to
> load the appropriate firmware.
> 
> The qcom,product-variant provides product line information, which
> the driver uses to load firmware from different directories.
> 
> If it's not defined in dts, the default firmware will be loaded.
> 
> Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
> ---
>   drivers/bluetooth/btqca.c   | 142 +++++++++++++++++++++++++++++-------
>   drivers/bluetooth/btqca.h   |  11 ++-
>   drivers/bluetooth/hci_qca.c |  73 +++++++++---------
>   3 files changed, 164 insertions(+), 62 deletions(-)
> 
> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
> index dfbbac92242a..0845e5a60412 100644
> --- a/drivers/bluetooth/btqca.c
> +++ b/drivers/bluetooth/btqca.c
> @@ -700,8 +700,79 @@ static int qca_check_bdaddr(struct hci_dev *hdev, const struct qca_fw_config *co
>   	return 0;
>   }
>   
> -static void qca_generate_hsp_nvm_name(char *fwname, size_t max_size,
> -		struct qca_btsoc_version ver, u8 rom_ver, u16 bid)
> +
> +const char *qca_get_soc_name(enum qca_btsoc_type soc_type)
> +{
> +	const char *soc_name = "";
> +
> +	switch (soc_type) {
> +	case QCA_QCA2066:
> +		soc_name = "QCA2066";
> +		break;
> +
> +	case QCA_QCA6698:
> +		soc_name = "QCA6698";
> +		break;
> +
> +	case QCA_WCN3988:
> +	case QCA_WCN3990:
> +	case QCA_WCN3991:
> +	case QCA_WCN3998:
> +		soc_name = "WCN399x";
> +		break;
> +
> +	case QCA_WCN6750:
> +		soc_name = "WCN6750";
> +		break;
> +
> +	case QCA_WCN6855:
> +		soc_name = "WCN6855";
> +		break;
> +
> +	case QCA_WCN7850:
> +		soc_name = "WCN7850";
> +		break;
> +
> +	default:
> +		soc_name = "ROME/QCA6390";
> +	}
> +
> +	return soc_name;
> +}
> +EXPORT_SYMBOL_GPL(qca_get_soc_name);
> +
> +static void qca_get_firmware_path(enum qca_btsoc_type soc_type, char *fw_path,
> +		size_t max_size, enum qca_product_type product_type)
> +{
> +	const char *fw_dir = NULL;
> +
> +	switch (product_type) {
> +	case QCA_MCC:
> +		fw_dir = "qca";
> +		break;
> +	case QCA_CE:
> +		fw_dir = "qca/ce";
> +		break;
> +	case QCA_IOT:
> +		fw_dir = "qca/iot";
> +		break;
> +	case QCA_AUTO:
> +		fw_dir = "qca/auto";
> +		break;
> +	default:
> +		fw_dir = "qca";
> +		break;
> +	}
> +
> +	if (product_type == QCA_IOT)
> +		snprintf(fw_path, max_size, "%s/%s", fw_dir, qca_get_soc_name(soc_type));
> +	else
> +		snprintf(fw_path, max_size, "%s", fw_dir);
> +}
> +
> +static void qca_generate_hsp_nvm_name(enum qca_btsoc_type soc_type, char *fwname,
> +		size_t max_size, const char *fw_path, struct qca_btsoc_version ver, u8 rom_ver,
> +		u16 bid)
>   {
>   	const char *variant;
>   
> @@ -712,33 +783,36 @@ static void qca_generate_hsp_nvm_name(char *fwname, size_t max_size,
>   		variant = "";
>   
>   	if (bid == 0x0)
> -		snprintf(fwname, max_size, "qca/hpnv%02x%s.bin", rom_ver, variant);
> +		snprintf(fwname, max_size, "%s/hpnv%02x%s.bin", fw_path, rom_ver, variant);
>   	else
> -		snprintf(fwname, max_size, "qca/hpnv%02x%s.%x", rom_ver, variant, bid);
> +		snprintf(fwname, max_size, "%s/hpnv%02x%s.%x", fw_path, rom_ver, variant, bid);
>   }
>   
> -static inline void qca_get_nvm_name_generic(struct qca_fw_config *cfg,
> +static inline void qca_get_nvm_name_generic(struct qca_fw_config *cfg, const char *fw_path,
>   					    const char *stem, u8 rom_ver, u16 bid)
>   {
>   	if (bid == 0x0)
> -		snprintf(cfg->fwname, sizeof(cfg->fwname), "qca/%snv%02x.bin", stem, rom_ver);
> +		snprintf(cfg->fwname, sizeof(cfg->fwname),
> +			 "%s/%snv%02x.bin", fw_path, stem, rom_ver);
>   	else if (bid & 0xff00)
>   		snprintf(cfg->fwname, sizeof(cfg->fwname),
> -			 "qca/%snv%02x.b%x", stem, rom_ver, bid);
> +			 "%s/%snv%02x.b%x", fw_path, stem, rom_ver, bid);
>   	else
>   		snprintf(cfg->fwname, sizeof(cfg->fwname),
> -			 "qca/%snv%02x.b%02x", stem, rom_ver, bid);
> +			 "%s/%snv%02x.b%02x", fw_path, stem, rom_ver, bid);
>   }
>   
>   int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>   		   enum qca_btsoc_type soc_type, struct qca_btsoc_version ver,
> -		   const char *firmware_name)
> +		   const char *firmware_name, uint32_t product_variant)
>   {
>   	struct qca_fw_config config = {};
>   	int err;
>   	u8 rom_ver = 0;
>   	u32 soc_ver;
>   	u16 boardid = 0;
> +	enum qca_product_type product_type;
> +	char fw_path[64] = {0};
>   
>   	bt_dev_dbg(hdev, "QCA setup on UART");
>   
> @@ -759,6 +833,10 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>   	if (soc_type == QCA_WCN6750)
>   		qca_send_patch_config_cmd(hdev);
>   
> +	/* Get the f/w path based on product variant */
> +	product_type = (product_variant >> 16) & 0xff;
> +	qca_get_firmware_path(soc_type, fw_path, sizeof(fw_path), product_type);
> +
>   	/* Download rampatch file */
>   	config.type = TLV_TYPE_PATCH;
>   	switch (soc_type) {
> @@ -766,19 +844,23 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>   	case QCA_WCN3991:
>   	case QCA_WCN3998:
>   		snprintf(config.fwname, sizeof(config.fwname),
> -			 "qca/crbtfw%02x.tlv", rom_ver);
> +			 "%s/crbtfw%02x.tlv", fw_path, rom_ver);

Changing firmware path will break existing platforms, please don't, or add fallbacks.

>   		break;
>   	case QCA_WCN3988:
>   		snprintf(config.fwname, sizeof(config.fwname),
> -			 "qca/apbtfw%02x.tlv", rom_ver);
> +			 "%s/apbtfw%02x.tlv", fw_path, rom_ver);
>   		break;
>   	case QCA_QCA2066:
>   		snprintf(config.fwname, sizeof(config.fwname),
> -			 "qca/hpbtfw%02x.tlv", rom_ver);
> +			 "%s/hpbtfw%02x.tlv", fw_path, rom_ver);
>   		break;
>   	case QCA_QCA6390:
>   		snprintf(config.fwname, sizeof(config.fwname),
> -			 "qca/htbtfw%02x.tlv", rom_ver);
> +			 "%s/htbtfw%02x.tlv", fw_path, rom_ver);
> +		break;
> +	case QCA_QCA6698:
> +		snprintf(config.fwname, sizeof(config.fwname),
> +			 "%s/hpbtfw%02x.tlv", fw_path, rom_ver);
>   		break;
>   	case QCA_WCN6750:
>   		/* Choose mbn file by default.If mbn file is not found
> @@ -786,19 +868,19 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>   		 */
>   		config.type = ELF_TYPE_PATCH;
>   		snprintf(config.fwname, sizeof(config.fwname),
> -			 "qca/msbtfw%02x.mbn", rom_ver);
> +			 "%s/msbtfw%02x.mbn", fw_path, rom_ver);
>   		break;
>   	case QCA_WCN6855:
>   		snprintf(config.fwname, sizeof(config.fwname),
> -			 "qca/hpbtfw%02x.tlv", rom_ver);
> +			 "%s/hpbtfw%02x.tlv", fw_path, rom_ver);
>   		break;
>   	case QCA_WCN7850:
>   		snprintf(config.fwname, sizeof(config.fwname),
> -			 "qca/hmtbtfw%02x.tlv", rom_ver);
> +			 "%s/hmtbtfw%02x.tlv", fw_path, rom_ver);
>   		break;
>   	default:
>   		snprintf(config.fwname, sizeof(config.fwname),
> -			 "qca/rampatch_%08x.bin", soc_ver);
> +			 "%s/rampatch_%08x.bin", fw_path, soc_ver);
>   	}
>   
>   	err = qca_download_firmware(hdev, &config, soc_type, rom_ver);
> @@ -810,7 +892,8 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>   	/* Give the controller some time to get ready to receive the NVM */
>   	msleep(10);
>   
> -	if (soc_type == QCA_QCA2066 || soc_type == QCA_WCN7850)
> +	if (soc_type == QCA_QCA2066 || soc_type == QCA_WCN7850 ||
> +		soc_type == QCA_QCA6698)
>   		qca_read_fw_board_id(hdev, &boardid);
>   
>   	/* Download NVM configuration */
> @@ -825,39 +908,40 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>   		case QCA_WCN3998:
>   			if (le32_to_cpu(ver.soc_id) == QCA_WCN3991_SOC_ID) {
>   				snprintf(config.fwname, sizeof(config.fwname),
> -					 "qca/crnv%02xu.bin", rom_ver);
> +					 "%s/crnv%02xu.bin", fw_path, rom_ver);
>   			} else {
>   				snprintf(config.fwname, sizeof(config.fwname),
> -					 "qca/crnv%02x.bin", rom_ver);
> +					 "%s/crnv%02x.bin", fw_path, rom_ver);
>   			}
>   			break;
>   		case QCA_WCN3988:
>   			snprintf(config.fwname, sizeof(config.fwname),
> -				 "qca/apnv%02x.bin", rom_ver);
> +				 "%s/apnv%02x.bin", fw_path, rom_ver);
>   			break;
>   		case QCA_QCA2066:
> -			qca_generate_hsp_nvm_name(config.fwname,
> -				sizeof(config.fwname), ver, rom_ver, boardid);
> +		case QCA_QCA6698:
> +			qca_generate_hsp_nvm_name(soc_type, config.fwname,
> +				sizeof(config.fwname), fw_path, ver, rom_ver, boardid);
>   			break;
>   		case QCA_QCA6390:
>   			snprintf(config.fwname, sizeof(config.fwname),
> -				 "qca/htnv%02x.bin", rom_ver);
> +				 "%s/htnv%02x.bin", fw_path, rom_ver);
>   			break;
>   		case QCA_WCN6750:
>   			snprintf(config.fwname, sizeof(config.fwname),
> -				 "qca/msnv%02x.bin", rom_ver);
> +				 "%s/msnv%02x.bin", fw_path, rom_ver);
>   			break;
>   		case QCA_WCN6855:
>   			snprintf(config.fwname, sizeof(config.fwname),
> -				 "qca/hpnv%02x.bin", rom_ver);
> +				 "%s/hpnv%02x.bin", fw_path, rom_ver);
>   			break;
>   		case QCA_WCN7850:
> -			qca_get_nvm_name_generic(&config, "hmt", rom_ver, boardid);
> +			qca_get_nvm_name_generic(&config, "hmt", fw_path, rom_ver, boardid);
>   			break;
>   
>   		default:
>   			snprintf(config.fwname, sizeof(config.fwname),
> -				 "qca/nvm_%08x.bin", soc_ver);
> +				 "%s/nvm_%08x.bin", fw_path, soc_ver);
>   		}
>   	}
>   
> @@ -871,6 +955,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>   	case QCA_WCN3991:
>   	case QCA_QCA2066:
>   	case QCA_QCA6390:
> +	case QCA_QCA6698:
>   	case QCA_WCN6750:
>   	case QCA_WCN6855:
>   	case QCA_WCN7850:
> @@ -909,6 +994,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>   	case QCA_WCN6750:
>   	case QCA_WCN6855:
>   	case QCA_WCN7850:
> +	case QCA_QCA6698:
>   		/* get fw build info */
>   		err = qca_read_fw_build_info(hdev);
>   		if (err < 0)
> diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
> index bb5207d7a8c7..baa3f979d017 100644
> --- a/drivers/bluetooth/btqca.h
> +++ b/drivers/bluetooth/btqca.h
> @@ -151,21 +151,30 @@ enum qca_btsoc_type {
>   	QCA_WCN3991,
>   	QCA_QCA2066,
>   	QCA_QCA6390,
> +	QCA_QCA6698,
>   	QCA_WCN6750,
>   	QCA_WCN6855,
>   	QCA_WCN7850,
>   };
>   
> +enum qca_product_type {
> +	QCA_MCC = 0,
> +	QCA_CE,
> +	QCA_IOT,
> +	QCA_AUTO,
> +};
> +
>   #if IS_ENABLED(CONFIG_BT_QCA)
>   
>   int qca_set_bdaddr_rome(struct hci_dev *hdev, const bdaddr_t *bdaddr);
>   int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>   		   enum qca_btsoc_type soc_type, struct qca_btsoc_version ver,
> -		   const char *firmware_name);
> +		   const char *firmware_name, uint32_t product_variant);
>   int qca_read_soc_version(struct hci_dev *hdev, struct qca_btsoc_version *ver,
>   			 enum qca_btsoc_type);
>   int qca_set_bdaddr(struct hci_dev *hdev, const bdaddr_t *bdaddr);
>   int qca_send_pre_shutdown_cmd(struct hci_dev *hdev);
> +const char *qca_get_soc_name(enum qca_btsoc_type soc_type);
>   #else
>   
>   static inline int qca_set_bdaddr_rome(struct hci_dev *hdev, const bdaddr_t *bdaddr)
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index 37129e6cb0eb..69fec890eb8c 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -227,6 +227,7 @@ struct qca_serdev {
>   	struct qca_power *bt_power;
>   	u32 init_speed;
>   	u32 oper_speed;
> +	u32 product_variant;
>   	bool bdaddr_property_broken;
>   	const char *firmware_name;
>   };
> @@ -1361,6 +1362,7 @@ static int qca_set_baudrate(struct hci_dev *hdev, uint8_t baudrate)
>   	case QCA_WCN6750:
>   	case QCA_WCN6855:
>   	case QCA_WCN7850:
> +	case QCA_QCA6698:
>   		usleep_range(1000, 10000);
>   		break;
>   
> @@ -1447,6 +1449,7 @@ static int qca_check_speeds(struct hci_uart *hu)
>   	case QCA_WCN6750:
>   	case QCA_WCN6855:
>   	case QCA_WCN7850:
> +	case QCA_QCA6698:
>   		if (!qca_get_speed(hu, QCA_INIT_SPEED) &&
>   		    !qca_get_speed(hu, QCA_OPER_SPEED))
>   			return -EINVAL;
> @@ -1489,6 +1492,7 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
>   		case QCA_WCN6750:
>   		case QCA_WCN6855:
>   		case QCA_WCN7850:
> +		case QCA_QCA6698:
>   			hci_uart_set_flow_control(hu, true);
>   			break;
>   
> @@ -1523,6 +1527,7 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
>   		case QCA_WCN6750:
>   		case QCA_WCN6855:
>   		case QCA_WCN7850:
> +		case QCA_QCA6698:
>   			hci_uart_set_flow_control(hu, false);
>   			break;
>   
> @@ -1803,6 +1808,7 @@ static int qca_power_on(struct hci_dev *hdev)
>   	case QCA_WCN6855:
>   	case QCA_WCN7850:
>   	case QCA_QCA6390:
> +	case QCA_QCA6698:
>   		ret = qca_regulator_init(hu);
>   		break;
>   
> @@ -1858,7 +1864,6 @@ static int qca_setup(struct hci_uart *hu)
>   	int ret;
>   	struct qca_btsoc_version ver;
>   	struct qca_serdev *qcadev;
> -	const char *soc_name;
>   
>   	ret = qca_check_speeds(hu);
>   	if (ret)
> @@ -1873,34 +1878,7 @@ static int qca_setup(struct hci_uart *hu)
>   	 */
>   	set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
>   
> -	switch (soc_type) {
> -	case QCA_QCA2066:
> -		soc_name = "qca2066";
> -		break;
> -
> -	case QCA_WCN3988:
> -	case QCA_WCN3990:
> -	case QCA_WCN3991:
> -	case QCA_WCN3998:
> -		soc_name = "wcn399x";
> -		break;
> -
> -	case QCA_WCN6750:
> -		soc_name = "wcn6750";
> -		break;
> -
> -	case QCA_WCN6855:
> -		soc_name = "wcn6855";
> -		break;
> -
> -	case QCA_WCN7850:
> -		soc_name = "wcn7850";
> -		break;
> -
> -	default:
> -		soc_name = "ROME/QCA6390";
> -	}
> -	bt_dev_info(hdev, "setting up %s", soc_name);
> +	bt_dev_info(hdev, "setting up %s", qca_get_soc_name(soc_type));

Move this into a new function in a separate patch

>   
>   	qca->memdump_state = QCA_MEMDUMP_IDLE;
>   
> @@ -1919,6 +1897,7 @@ static int qca_setup(struct hci_uart *hu)
>   	case QCA_WCN6750:
>   	case QCA_WCN6855:
>   	case QCA_WCN7850:
> +	case QCA_QCA6698:
>   		qcadev = serdev_device_get_drvdata(hu->serdev);
>   		if (qcadev->bdaddr_property_broken)
>   			set_bit(HCI_QUIRK_BDADDR_PROPERTY_BROKEN, &hdev->quirks);
> @@ -1952,6 +1931,7 @@ static int qca_setup(struct hci_uart *hu)
>   	case QCA_WCN6750:
>   	case QCA_WCN6855:
>   	case QCA_WCN7850:
> +	case QCA_QCA6698:
>   		break;
>   
>   	default:
> @@ -1963,7 +1943,7 @@ static int qca_setup(struct hci_uart *hu)
>   
>   	/* Setup patch / NVM configurations */
>   	ret = qca_uart_setup(hdev, qca_baudrate, soc_type, ver,
> -			firmware_name);
> +			firmware_name, qcadev->product_variant);

Add product variant support separately from adding QCA6698

>   	if (!ret) {
>   		clear_bit(QCA_IBS_DISABLED, &qca->flags);
>   		qca_debugfs_init(hdev);
> @@ -2089,6 +2069,20 @@ static const struct qca_device_data qca_soc_data_qca6390 __maybe_unused = {
>   	.num_vregs = 0,
>   };
>   
> +static const struct qca_device_data qca_soc_data_qca6698 __maybe_unused = {
> +	.soc_type = QCA_QCA6698,
> +	.vregs = (struct qca_vreg []) {
> +		{ "vddio", 5000 },
> +		{ "vddbtcxmx", 126000 },
> +		{ "vddrfacmn", 12500 },
> +		{ "vddrfa0p8", 102000 },
> +		{ "vddrfa1p7", 302000 },
> +		{ "vddrfa1p2", 257000 },
> +	},
> +	.num_vregs = 6,
> +	.capabilities = QCA_CAP_WIDEBAND_SPEECH | QCA_CAP_VALID_LE_STATES,
> +};
> +
>   static const struct qca_device_data qca_soc_data_wcn6750 __maybe_unused = {
>   	.soc_type = QCA_WCN6750,
>   	.vregs = (struct qca_vreg []) {
> @@ -2165,7 +2159,7 @@ static void qca_power_shutdown(struct hci_uart *hu)
>   		pwrseq_power_off(power->pwrseq);
>   		set_bit(QCA_BT_OFF, &qca->flags);
>   		return;
> -        }
> +	}

This is cleanup

>   
>   	switch (soc_type) {
>   	case QCA_WCN3988:
> @@ -2179,6 +2173,7 @@ static void qca_power_shutdown(struct hci_uart *hu)
>   
>   	case QCA_WCN6750:
>   	case QCA_WCN6855:
> +	case QCA_QCA6698:
>   		gpiod_set_value_cansleep(qcadev->bt_en, 0);
>   		msleep(100);
>   		qca_regulator_disable(qcadev);
> @@ -2313,6 +2308,12 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>   					 &qcadev->firmware_name);
>   	device_property_read_u32(&serdev->dev, "max-speed",
>   				 &qcadev->oper_speed);
> +	device_property_read_u32(&serdev->dev, "qcom,product-variant",
> +				 &qcadev->product_variant);
> +
> +	if (qcadev->product_variant != 0)
> +		BT_INFO("QC Product Variant: 0x%08x", qcadev->product_variant);
> +
>   	if (!qcadev->oper_speed)
>   		BT_DBG("UART will pick default operating speed");
>   
> @@ -2333,6 +2334,7 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>   	case QCA_WCN6855:
>   	case QCA_WCN7850:
>   	case QCA_QCA6390:
> +	case QCA_QCA6698:
>   		qcadev->bt_power = devm_kzalloc(&serdev->dev,
>   						sizeof(struct qca_power),
>   						GFP_KERNEL);
> @@ -2346,6 +2348,7 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>   	switch (qcadev->btsoc_type) {
>   	case QCA_WCN6855:
>   	case QCA_WCN7850:
> +	case QCA_QCA6698:
>   		if (!device_property_present(&serdev->dev, "enable-gpios")) {
>   			/*
>   			 * Backward compatibility with old DT sources. If the
> @@ -2380,7 +2383,8 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>   					       GPIOD_OUT_LOW);
>   		if (IS_ERR(qcadev->bt_en) &&
>   		    (data->soc_type == QCA_WCN6750 ||
> -		     data->soc_type == QCA_WCN6855)) {
> +		     data->soc_type == QCA_WCN6855 ||
> +		     data->soc_type == QCA_QCA6698)) {
>   			dev_err(&serdev->dev, "failed to acquire BT_EN gpio\n");
>   			return PTR_ERR(qcadev->bt_en);
>   		}
> @@ -2393,7 +2397,8 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>   		if (IS_ERR(qcadev->sw_ctrl) &&
>   		    (data->soc_type == QCA_WCN6750 ||
>   		     data->soc_type == QCA_WCN6855 ||
> -		     data->soc_type == QCA_WCN7850)) {
> +		     data->soc_type == QCA_WCN7850 ||
> +		     data->soc_type == QCA_QCA6698)) {
>   			dev_err(&serdev->dev, "failed to acquire SW_CTRL gpio\n");
>   			return PTR_ERR(qcadev->sw_ctrl);
>   		}
> @@ -2475,6 +2480,7 @@ static void qca_serdev_remove(struct serdev_device *serdev)
>   	case QCA_WCN6750:
>   	case QCA_WCN6855:
>   	case QCA_WCN7850:
> +	case QCA_QCA6698:
>   		if (power->vregs_on)
>   			qca_power_shutdown(&qcadev->serdev_hu);
>   		break;
> @@ -2669,6 +2675,7 @@ static const struct of_device_id qca_bluetooth_of_match[] = {
>   	{ .compatible = "qcom,qca2066-bt", .data = &qca_soc_data_qca2066},
>   	{ .compatible = "qcom,qca6174-bt" },
>   	{ .compatible = "qcom,qca6390-bt", .data = &qca_soc_data_qca6390},
> +	{ .compatible = "qcom,qca6698-bt", .data = &qca_soc_data_qca6698},
>   	{ .compatible = "qcom,qca9377-bt" },
>   	{ .compatible = "qcom,wcn3988-bt", .data = &qca_soc_data_wcn3988},
>   	{ .compatible = "qcom,wcn3990-bt", .data = &qca_soc_data_wcn3990},

This patch is too long anf has multiple different changes merged together,
please split into multiple small pieces that can be reviewed easier,

Thanks,
Neil

Re: [PATCH v2 4/4] Bluetooth: hci_qca: add qcom,product-variant properties

[Cheng Jiang]

Hi Neil,

On 11/20/2024 6:59 PM, neil.armstrong@linaro.org wrote:
> On 20/11/2024 10:54, Cheng Jiang wrote:
>> Since different products use the same SoC chip, features cannot
>> be included in a single patch. Use the qcom,product-variant to
>> load the appropriate firmware.
>>
>> The qcom,product-variant provides product line information, which
>> the driver uses to load firmware from different directories.
>>
>> If it's not defined in dts, the default firmware will be loaded.
>>
>> Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
>> ---
>>   drivers/bluetooth/btqca.c   | 142 +++++++++++++++++++++++++++++-------
>>   drivers/bluetooth/btqca.h   |  11 ++-
>>   drivers/bluetooth/hci_qca.c |  73 +++++++++---------
>>   3 files changed, 164 insertions(+), 62 deletions(-)
>>
>> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
>> index dfbbac92242a..0845e5a60412 100644
>> --- a/drivers/bluetooth/btqca.c
>> +++ b/drivers/bluetooth/btqca.c
>> @@ -700,8 +700,79 @@ static int qca_check_bdaddr(struct hci_dev *hdev, const struct qca_fw_config *co
>>       return 0;
>>   }
>>   -static void qca_generate_hsp_nvm_name(char *fwname, size_t max_size,
>> -        struct qca_btsoc_version ver, u8 rom_ver, u16 bid)
>> +
>> +const char *qca_get_soc_name(enum qca_btsoc_type soc_type)
>> +{
>> +    const char *soc_name = "";
>> +
>> +    switch (soc_type) {
>> +    case QCA_QCA2066:
>> +        soc_name = "QCA2066";
>> +        break;
>> +
>> +    case QCA_QCA6698:
>> +        soc_name = "QCA6698";
>> +        break;
>> +
>> +    case QCA_WCN3988:
>> +    case QCA_WCN3990:
>> +    case QCA_WCN3991:
>> +    case QCA_WCN3998:
>> +        soc_name = "WCN399x";
>> +        break;
>> +
>> +    case QCA_WCN6750:
>> +        soc_name = "WCN6750";
>> +        break;
>> +
>> +    case QCA_WCN6855:
>> +        soc_name = "WCN6855";
>> +        break;
>> +
>> +    case QCA_WCN7850:
>> +        soc_name = "WCN7850";
>> +        break;
>> +
>> +    default:
>> +        soc_name = "ROME/QCA6390";
>> +    }
>> +
>> +    return soc_name;
>> +}
>> +EXPORT_SYMBOL_GPL(qca_get_soc_name);
>> +
>> +static void qca_get_firmware_path(enum qca_btsoc_type soc_type, char *fw_path,
>> +        size_t max_size, enum qca_product_type product_type)
>> +{
>> +    const char *fw_dir = NULL;
>> +
>> +    switch (product_type) {
>> +    case QCA_MCC:
>> +        fw_dir = "qca";
>> +        break;
>> +    case QCA_CE:
>> +        fw_dir = "qca/ce";
>> +        break;
>> +    case QCA_IOT:
>> +        fw_dir = "qca/iot";
>> +        break;
>> +    case QCA_AUTO:
>> +        fw_dir = "qca/auto";
>> +        break;
>> +    default:
>> +        fw_dir = "qca";
>> +        break;
>> +    }
>> +
>> +    if (product_type == QCA_IOT)
>> +        snprintf(fw_path, max_size, "%s/%s", fw_dir, qca_get_soc_name(soc_type));
>> +    else
>> +        snprintf(fw_path, max_size, "%s", fw_dir);
>> +}
>> +
>> +static void qca_generate_hsp_nvm_name(enum qca_btsoc_type soc_type, char *fwname,
>> +        size_t max_size, const char *fw_path, struct qca_btsoc_version ver, u8 rom_ver,
>> +        u16 bid)
>>   {
>>       const char *variant;
>>   @@ -712,33 +783,36 @@ static void qca_generate_hsp_nvm_name(char *fwname, size_t max_size,
>>           variant = "";
>>         if (bid == 0x0)
>> -        snprintf(fwname, max_size, "qca/hpnv%02x%s.bin", rom_ver, variant);
>> +        snprintf(fwname, max_size, "%s/hpnv%02x%s.bin", fw_path, rom_ver, variant);
>>       else
>> -        snprintf(fwname, max_size, "qca/hpnv%02x%s.%x", rom_ver, variant, bid);
>> +        snprintf(fwname, max_size, "%s/hpnv%02x%s.%x", fw_path, rom_ver, variant, bid);
>>   }
>>   -static inline void qca_get_nvm_name_generic(struct qca_fw_config *cfg,
>> +static inline void qca_get_nvm_name_generic(struct qca_fw_config *cfg, const char *fw_path,
>>                           const char *stem, u8 rom_ver, u16 bid)
>>   {
>>       if (bid == 0x0)
>> -        snprintf(cfg->fwname, sizeof(cfg->fwname), "qca/%snv%02x.bin", stem, rom_ver);
>> +        snprintf(cfg->fwname, sizeof(cfg->fwname),
>> +             "%s/%snv%02x.bin", fw_path, stem, rom_ver);
>>       else if (bid & 0xff00)
>>           snprintf(cfg->fwname, sizeof(cfg->fwname),
>> -             "qca/%snv%02x.b%x", stem, rom_ver, bid);
>> +             "%s/%snv%02x.b%x", fw_path, stem, rom_ver, bid);
>>       else
>>           snprintf(cfg->fwname, sizeof(cfg->fwname),
>> -             "qca/%snv%02x.b%02x", stem, rom_ver, bid);
>> +             "%s/%snv%02x.b%02x", fw_path, stem, rom_ver, bid);
>>   }
>>     int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>>              enum qca_btsoc_type soc_type, struct qca_btsoc_version ver,
>> -           const char *firmware_name)
>> +           const char *firmware_name, uint32_t product_variant)
>>   {
>>       struct qca_fw_config config = {};
>>       int err;
>>       u8 rom_ver = 0;
>>       u32 soc_ver;
>>       u16 boardid = 0;
>> +    enum qca_product_type product_type;
>> +    char fw_path[64] = {0};
>>         bt_dev_dbg(hdev, "QCA setup on UART");
>>   @@ -759,6 +833,10 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>>       if (soc_type == QCA_WCN6750)
>>           qca_send_patch_config_cmd(hdev);
>>   +    /* Get the f/w path based on product variant */
>> +    product_type = (product_variant >> 16) & 0xff;
>> +    qca_get_firmware_path(soc_type, fw_path, sizeof(fw_path), product_type);
>> +
>>       /* Download rampatch file */
>>       config.type = TLV_TYPE_PATCH;
>>       switch (soc_type) {
>> @@ -766,19 +844,23 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>>       case QCA_WCN3991:
>>       case QCA_WCN3998:
>>           snprintf(config.fwname, sizeof(config.fwname),
>> -             "qca/crbtfw%02x.tlv", rom_ver);
>> +             "%s/crbtfw%02x.tlv", fw_path, rom_ver);
> 
> Changing firmware path will break existing platforms, please don't, or add fallbacks.
Ack.
> 
>>           break;
>>       case QCA_WCN3988:
>>           snprintf(config.fwname, sizeof(config.fwname),
>> -             "qca/apbtfw%02x.tlv", rom_ver);
>> +             "%s/apbtfw%02x.tlv", fw_path, rom_ver);
>>           break;
>>       case QCA_QCA2066:
>>           snprintf(config.fwname, sizeof(config.fwname),
>> -             "qca/hpbtfw%02x.tlv", rom_ver);
>> +             "%s/hpbtfw%02x.tlv", fw_path, rom_ver);
>>           break;
>>       case QCA_QCA6390:
>>           snprintf(config.fwname, sizeof(config.fwname),
>> -             "qca/htbtfw%02x.tlv", rom_ver);
>> +             "%s/htbtfw%02x.tlv", fw_path, rom_ver);
>> +        break;
>> +    case QCA_QCA6698:
>> +        snprintf(config.fwname, sizeof(config.fwname),
>> +             "%s/hpbtfw%02x.tlv", fw_path, rom_ver);
>>           break;
>>       case QCA_WCN6750:
>>           /* Choose mbn file by default.If mbn file is not found
>> @@ -786,19 +868,19 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>>            */
>>           config.type = ELF_TYPE_PATCH;
>>           snprintf(config.fwname, sizeof(config.fwname),
>> -             "qca/msbtfw%02x.mbn", rom_ver);
>> +             "%s/msbtfw%02x.mbn", fw_path, rom_ver);
>>           break;
>>       case QCA_WCN6855:
>>           snprintf(config.fwname, sizeof(config.fwname),
>> -             "qca/hpbtfw%02x.tlv", rom_ver);
>> +             "%s/hpbtfw%02x.tlv", fw_path, rom_ver);
>>           break;
>>       case QCA_WCN7850:
>>           snprintf(config.fwname, sizeof(config.fwname),
>> -             "qca/hmtbtfw%02x.tlv", rom_ver);
>> +             "%s/hmtbtfw%02x.tlv", fw_path, rom_ver);
>>           break;
>>       default:
>>           snprintf(config.fwname, sizeof(config.fwname),
>> -             "qca/rampatch_%08x.bin", soc_ver);
>> +             "%s/rampatch_%08x.bin", fw_path, soc_ver);
>>       }
>>         err = qca_download_firmware(hdev, &config, soc_type, rom_ver);
>> @@ -810,7 +892,8 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>>       /* Give the controller some time to get ready to receive the NVM */
>>       msleep(10);
>>   -    if (soc_type == QCA_QCA2066 || soc_type == QCA_WCN7850)
>> +    if (soc_type == QCA_QCA2066 || soc_type == QCA_WCN7850 ||
>> +        soc_type == QCA_QCA6698)
>>           qca_read_fw_board_id(hdev, &boardid);
>>         /* Download NVM configuration */
>> @@ -825,39 +908,40 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>>           case QCA_WCN3998:
>>               if (le32_to_cpu(ver.soc_id) == QCA_WCN3991_SOC_ID) {
>>                   snprintf(config.fwname, sizeof(config.fwname),
>> -                     "qca/crnv%02xu.bin", rom_ver);
>> +                     "%s/crnv%02xu.bin", fw_path, rom_ver);
>>               } else {
>>                   snprintf(config.fwname, sizeof(config.fwname),
>> -                     "qca/crnv%02x.bin", rom_ver);
>> +                     "%s/crnv%02x.bin", fw_path, rom_ver);
>>               }
>>               break;
>>           case QCA_WCN3988:
>>               snprintf(config.fwname, sizeof(config.fwname),
>> -                 "qca/apnv%02x.bin", rom_ver);
>> +                 "%s/apnv%02x.bin", fw_path, rom_ver);
>>               break;
>>           case QCA_QCA2066:
>> -            qca_generate_hsp_nvm_name(config.fwname,
>> -                sizeof(config.fwname), ver, rom_ver, boardid);
>> +        case QCA_QCA6698:
>> +            qca_generate_hsp_nvm_name(soc_type, config.fwname,
>> +                sizeof(config.fwname), fw_path, ver, rom_ver, boardid);
>>               break;
>>           case QCA_QCA6390:
>>               snprintf(config.fwname, sizeof(config.fwname),
>> -                 "qca/htnv%02x.bin", rom_ver);
>> +                 "%s/htnv%02x.bin", fw_path, rom_ver);
>>               break;
>>           case QCA_WCN6750:
>>               snprintf(config.fwname, sizeof(config.fwname),
>> -                 "qca/msnv%02x.bin", rom_ver);
>> +                 "%s/msnv%02x.bin", fw_path, rom_ver);
>>               break;
>>           case QCA_WCN6855:
>>               snprintf(config.fwname, sizeof(config.fwname),
>> -                 "qca/hpnv%02x.bin", rom_ver);
>> +                 "%s/hpnv%02x.bin", fw_path, rom_ver);
>>               break;
>>           case QCA_WCN7850:
>> -            qca_get_nvm_name_generic(&config, "hmt", rom_ver, boardid);
>> +            qca_get_nvm_name_generic(&config, "hmt", fw_path, rom_ver, boardid);
>>               break;
>>             default:
>>               snprintf(config.fwname, sizeof(config.fwname),
>> -                 "qca/nvm_%08x.bin", soc_ver);
>> +                 "%s/nvm_%08x.bin", fw_path, soc_ver);
>>           }
>>       }
>>   @@ -871,6 +955,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>>       case QCA_WCN3991:
>>       case QCA_QCA2066:
>>       case QCA_QCA6390:
>> +    case QCA_QCA6698:
>>       case QCA_WCN6750:
>>       case QCA_WCN6855:
>>       case QCA_WCN7850:
>> @@ -909,6 +994,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>>       case QCA_WCN6750:
>>       case QCA_WCN6855:
>>       case QCA_WCN7850:
>> +    case QCA_QCA6698:
>>           /* get fw build info */
>>           err = qca_read_fw_build_info(hdev);
>>           if (err < 0)
>> diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
>> index bb5207d7a8c7..baa3f979d017 100644
>> --- a/drivers/bluetooth/btqca.h
>> +++ b/drivers/bluetooth/btqca.h
>> @@ -151,21 +151,30 @@ enum qca_btsoc_type {
>>       QCA_WCN3991,
>>       QCA_QCA2066,
>>       QCA_QCA6390,
>> +    QCA_QCA6698,
>>       QCA_WCN6750,
>>       QCA_WCN6855,
>>       QCA_WCN7850,
>>   };
>>   +enum qca_product_type {
>> +    QCA_MCC = 0,
>> +    QCA_CE,
>> +    QCA_IOT,
>> +    QCA_AUTO,
>> +};
>> +
>>   #if IS_ENABLED(CONFIG_BT_QCA)
>>     int qca_set_bdaddr_rome(struct hci_dev *hdev, const bdaddr_t *bdaddr);
>>   int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>>              enum qca_btsoc_type soc_type, struct qca_btsoc_version ver,
>> -           const char *firmware_name);
>> +           const char *firmware_name, uint32_t product_variant);
>>   int qca_read_soc_version(struct hci_dev *hdev, struct qca_btsoc_version *ver,
>>                enum qca_btsoc_type);
>>   int qca_set_bdaddr(struct hci_dev *hdev, const bdaddr_t *bdaddr);
>>   int qca_send_pre_shutdown_cmd(struct hci_dev *hdev);
>> +const char *qca_get_soc_name(enum qca_btsoc_type soc_type);
>>   #else
>>     static inline int qca_set_bdaddr_rome(struct hci_dev *hdev, const bdaddr_t *bdaddr)
>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> index 37129e6cb0eb..69fec890eb8c 100644
>> --- a/drivers/bluetooth/hci_qca.c
>> +++ b/drivers/bluetooth/hci_qca.c
>> @@ -227,6 +227,7 @@ struct qca_serdev {
>>       struct qca_power *bt_power;
>>       u32 init_speed;
>>       u32 oper_speed;
>> +    u32 product_variant;
>>       bool bdaddr_property_broken;
>>       const char *firmware_name;
>>   };
>> @@ -1361,6 +1362,7 @@ static int qca_set_baudrate(struct hci_dev *hdev, uint8_t baudrate)
>>       case QCA_WCN6750:
>>       case QCA_WCN6855:
>>       case QCA_WCN7850:
>> +    case QCA_QCA6698:
>>           usleep_range(1000, 10000);
>>           break;
>>   @@ -1447,6 +1449,7 @@ static int qca_check_speeds(struct hci_uart *hu)
>>       case QCA_WCN6750:
>>       case QCA_WCN6855:
>>       case QCA_WCN7850:
>> +    case QCA_QCA6698:
>>           if (!qca_get_speed(hu, QCA_INIT_SPEED) &&
>>               !qca_get_speed(hu, QCA_OPER_SPEED))
>>               return -EINVAL;
>> @@ -1489,6 +1492,7 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
>>           case QCA_WCN6750:
>>           case QCA_WCN6855:
>>           case QCA_WCN7850:
>> +        case QCA_QCA6698:
>>               hci_uart_set_flow_control(hu, true);
>>               break;
>>   @@ -1523,6 +1527,7 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
>>           case QCA_WCN6750:
>>           case QCA_WCN6855:
>>           case QCA_WCN7850:
>> +        case QCA_QCA6698:
>>               hci_uart_set_flow_control(hu, false);
>>               break;
>>   @@ -1803,6 +1808,7 @@ static int qca_power_on(struct hci_dev *hdev)
>>       case QCA_WCN6855:
>>       case QCA_WCN7850:
>>       case QCA_QCA6390:
>> +    case QCA_QCA6698:
>>           ret = qca_regulator_init(hu);
>>           break;
>>   @@ -1858,7 +1864,6 @@ static int qca_setup(struct hci_uart *hu)
>>       int ret;
>>       struct qca_btsoc_version ver;
>>       struct qca_serdev *qcadev;
>> -    const char *soc_name;
>>         ret = qca_check_speeds(hu);
>>       if (ret)
>> @@ -1873,34 +1878,7 @@ static int qca_setup(struct hci_uart *hu)
>>        */
>>       set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
>>   -    switch (soc_type) {
>> -    case QCA_QCA2066:
>> -        soc_name = "qca2066";
>> -        break;
>> -
>> -    case QCA_WCN3988:
>> -    case QCA_WCN3990:
>> -    case QCA_WCN3991:
>> -    case QCA_WCN3998:
>> -        soc_name = "wcn399x";
>> -        break;
>> -
>> -    case QCA_WCN6750:
>> -        soc_name = "wcn6750";
>> -        break;
>> -
>> -    case QCA_WCN6855:
>> -        soc_name = "wcn6855";
>> -        break;
>> -
>> -    case QCA_WCN7850:
>> -        soc_name = "wcn7850";
>> -        break;
>> -
>> -    default:
>> -        soc_name = "ROME/QCA6390";
>> -    }
>> -    bt_dev_info(hdev, "setting up %s", soc_name);
>> +    bt_dev_info(hdev, "setting up %s", qca_get_soc_name(soc_type));
> 
> Move this into a new function in a separate patch
Ack.
> 
>>         qca->memdump_state = QCA_MEMDUMP_IDLE;
>>   @@ -1919,6 +1897,7 @@ static int qca_setup(struct hci_uart *hu)
>>       case QCA_WCN6750:
>>       case QCA_WCN6855:
>>       case QCA_WCN7850:
>> +    case QCA_QCA6698:
>>           qcadev = serdev_device_get_drvdata(hu->serdev);
>>           if (qcadev->bdaddr_property_broken)
>>               set_bit(HCI_QUIRK_BDADDR_PROPERTY_BROKEN, &hdev->quirks);
>> @@ -1952,6 +1931,7 @@ static int qca_setup(struct hci_uart *hu)
>>       case QCA_WCN6750:
>>       case QCA_WCN6855:
>>       case QCA_WCN7850:
>> +    case QCA_QCA6698:
>>           break;
>>         default:
>> @@ -1963,7 +1943,7 @@ static int qca_setup(struct hci_uart *hu)
>>         /* Setup patch / NVM configurations */
>>       ret = qca_uart_setup(hdev, qca_baudrate, soc_type, ver,
>> -            firmware_name);
>> +            firmware_name, qcadev->product_variant);
> 
> Add product variant support separately from adding QCA6698
Ack.
> 
>>       if (!ret) {
>>           clear_bit(QCA_IBS_DISABLED, &qca->flags);
>>           qca_debugfs_init(hdev);
>> @@ -2089,6 +2069,20 @@ static const struct qca_device_data qca_soc_data_qca6390 __maybe_unused = {
>>       .num_vregs = 0,
>>   };
>>   +static const struct qca_device_data qca_soc_data_qca6698 __maybe_unused = {
>> +    .soc_type = QCA_QCA6698,
>> +    .vregs = (struct qca_vreg []) {
>> +        { "vddio", 5000 },
>> +        { "vddbtcxmx", 126000 },
>> +        { "vddrfacmn", 12500 },
>> +        { "vddrfa0p8", 102000 },
>> +        { "vddrfa1p7", 302000 },
>> +        { "vddrfa1p2", 257000 },
>> +    },
>> +    .num_vregs = 6,
>> +    .capabilities = QCA_CAP_WIDEBAND_SPEECH | QCA_CAP_VALID_LE_STATES,
>> +};
>> +
>>   static const struct qca_device_data qca_soc_data_wcn6750 __maybe_unused = {
>>       .soc_type = QCA_WCN6750,
>>       .vregs = (struct qca_vreg []) {
>> @@ -2165,7 +2159,7 @@ static void qca_power_shutdown(struct hci_uart *hu)
>>           pwrseq_power_off(power->pwrseq);
>>           set_bit(QCA_BT_OFF, &qca->flags);
>>           return;
>> -        }
>> +    }
> 
> This is cleanup
> 
>>         switch (soc_type) {
>>       case QCA_WCN3988:
>> @@ -2179,6 +2173,7 @@ static void qca_power_shutdown(struct hci_uart *hu)
>>         case QCA_WCN6750:
>>       case QCA_WCN6855:
>> +    case QCA_QCA6698:
>>           gpiod_set_value_cansleep(qcadev->bt_en, 0);
>>           msleep(100);
>>           qca_regulator_disable(qcadev);
>> @@ -2313,6 +2308,12 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>>                        &qcadev->firmware_name);
>>       device_property_read_u32(&serdev->dev, "max-speed",
>>                    &qcadev->oper_speed);
>> +    device_property_read_u32(&serdev->dev, "qcom,product-variant",
>> +                 &qcadev->product_variant);
>> +
>> +    if (qcadev->product_variant != 0)
>> +        BT_INFO("QC Product Variant: 0x%08x", qcadev->product_variant);
>> +
>>       if (!qcadev->oper_speed)
>>           BT_DBG("UART will pick default operating speed");
>>   @@ -2333,6 +2334,7 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>>       case QCA_WCN6855:
>>       case QCA_WCN7850:
>>       case QCA_QCA6390:
>> +    case QCA_QCA6698:
>>           qcadev->bt_power = devm_kzalloc(&serdev->dev,
>>                           sizeof(struct qca_power),
>>                           GFP_KERNEL);
>> @@ -2346,6 +2348,7 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>>       switch (qcadev->btsoc_type) {
>>       case QCA_WCN6855:
>>       case QCA_WCN7850:
>> +    case QCA_QCA6698:
>>           if (!device_property_present(&serdev->dev, "enable-gpios")) {
>>               /*
>>                * Backward compatibility with old DT sources. If the
>> @@ -2380,7 +2383,8 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>>                              GPIOD_OUT_LOW);
>>           if (IS_ERR(qcadev->bt_en) &&
>>               (data->soc_type == QCA_WCN6750 ||
>> -             data->soc_type == QCA_WCN6855)) {
>> +             data->soc_type == QCA_WCN6855 ||
>> +             data->soc_type == QCA_QCA6698)) {
>>               dev_err(&serdev->dev, "failed to acquire BT_EN gpio\n");
>>               return PTR_ERR(qcadev->bt_en);
>>           }
>> @@ -2393,7 +2397,8 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>>           if (IS_ERR(qcadev->sw_ctrl) &&
>>               (data->soc_type == QCA_WCN6750 ||
>>                data->soc_type == QCA_WCN6855 ||
>> -             data->soc_type == QCA_WCN7850)) {
>> +             data->soc_type == QCA_WCN7850 ||
>> +             data->soc_type == QCA_QCA6698)) {
>>               dev_err(&serdev->dev, "failed to acquire SW_CTRL gpio\n");
>>               return PTR_ERR(qcadev->sw_ctrl);
>>           }
>> @@ -2475,6 +2480,7 @@ static void qca_serdev_remove(struct serdev_device *serdev)
>>       case QCA_WCN6750:
>>       case QCA_WCN6855:
>>       case QCA_WCN7850:
>> +    case QCA_QCA6698:
>>           if (power->vregs_on)
>>               qca_power_shutdown(&qcadev->serdev_hu);
>>           break;
>> @@ -2669,6 +2675,7 @@ static const struct of_device_id qca_bluetooth_of_match[] = {
>>       { .compatible = "qcom,qca2066-bt", .data = &qca_soc_data_qca2066},
>>       { .compatible = "qcom,qca6174-bt" },
>>       { .compatible = "qcom,qca6390-bt", .data = &qca_soc_data_qca6390},
>> +    { .compatible = "qcom,qca6698-bt", .data = &qca_soc_data_qca6698},
>>       { .compatible = "qcom,qca9377-bt" },
>>       { .compatible = "qcom,wcn3988-bt", .data = &qca_soc_data_wcn3988},
>>       { .compatible = "qcom,wcn3990-bt", .data = &qca_soc_data_wcn3990},
> 
> This patch is too long anf has multiple different changes merged together,
> please split into multiple small pieces that can be reviewed easier,
Ack. Thanks Neil. Will change the patch to samll pieces in furture commits. 
>
> Thanks,
> Neil