diff mbox series

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

Message ID 20241120095428.1122935-2-quic_chejiang@quicinc.com
State New
Headers show
Series Add qcom,product-variant properties in Qualcomm | expand

Commit Message

Cheng Jiang (IOE) Nov. 20, 2024, 9:54 a.m. UTC
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(+)

Comments

Dmitry Baryshkov Nov. 20, 2024, 10:43 a.m. UTC | #1
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
>
Krzysztof Kozlowski Nov. 20, 2024, 4:47 p.m. UTC | #2
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
Cheng Jiang (IOE) Nov. 21, 2024, 4:02 a.m. UTC | #3
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
>>
>
Cheng Jiang (IOE) Nov. 21, 2024, 4:06 a.m. UTC | #4
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
Dmitry Baryshkov Nov. 21, 2024, 4:38 a.m. UTC | #5
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
> >>
> >
>
Cheng Jiang (IOE) Nov. 21, 2024, 4:44 a.m. UTC | #6
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
>>>>
>>>
>>
> 
>
Krzysztof Kozlowski Nov. 21, 2024, 7:49 a.m. UTC | #7
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
Cheng Jiang (IOE) Nov. 21, 2024, 3:53 p.m. UTC | #8
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
Konrad Dybcio Nov. 22, 2024, 12:49 p.m. UTC | #9
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
Konrad Dybcio Nov. 22, 2024, 1:08 p.m. UTC | #10
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
Cheng Jiang (IOE) Nov. 30, 2024, 3:47 a.m. UTC | #11
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
>>>>
>>>
>>
> 
>
Dmitry Baryshkov Nov. 30, 2024, 8:24 a.m. UTC | #12
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
Cheng Jiang (IOE) Dec. 2, 2024, 2:22 a.m. UTC | #13
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
Dmitry Baryshkov Dec. 2, 2024, 11:38 a.m. UTC | #14
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
>
Cheng Jiang (IOE) Dec. 2, 2024, 2:25 p.m. UTC | #15
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
>>
>
Dmitry Baryshkov Dec. 2, 2024, 3:14 p.m. UTC | #16
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
> >>
> >
>
Cheng Jiang (IOE) Dec. 3, 2024, 3:04 a.m. UTC | #17
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
>>>>
>>>
>>
> 
>
diff mbox series

Patch

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