Message-ID: <c3394a08-edab-45a4-9ed8-db2a06598a0a@quicinc.com>
Date: Mon, 2 Dec 2024 22:25:03 +0800
From: "Cheng Jiang (IOE)" <quic_chejiang@...cinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
CC: Marcel Holtmann <marcel@...tmann.org>,
        Luiz Augusto von Dentz
	<luiz.dentz@...il.com>,
        Rob Herring <robh@...nel.org>,
        Krzysztof Kozlowski
	<krzk+dt@...nel.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Bjorn Andersson
	<andersson@...nel.org>,
        Konrad Dybcio <konradybcio@...nel.org>,
        "Balakrishna
 Godavarthi" <quic_bgodavar@...cinc.com>,
        Rocky Liao
	<quic_rjliao@...cinc.com>, <quic_zijuhu@...cinc.com>,
        <linux-bluetooth@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>,
        <quic_mohamull@...cinc.com>
Subject: Re: [PATCH v2 1/4] dt-bindings: bluetooth: add 'qcom,product-variant'



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@...cinc.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@...cinc.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@...cinc.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
>>
> 

Powered by blists - more mailing lists