Hi Abhishek,
>>>>> BCM chips may require configuration of PCM to operate correctly and
>>>>> there is a vendor specific HCI command to do this. Add support in the
>>>>> hci_bcm driver to parse this from devicetree and configure the chip.
>>>>>
>>>>> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@xxxxxxxxxxxx>
>>>>> ---
>>>>>
>>>>> Changes in v4: None
>>>>> Changes in v3: None
>>>>> Changes in v2: None
>>>>>
>>>>> drivers/bluetooth/hci_bcm.c | 32 ++++++++++++++++++++++++++++++++
>>>>> 1 file changed, 32 insertions(+)
>>>>>
>>>>> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
>>>>> index 6134bff58748..4ee0b45be7e2 100644
>>>>> --- a/drivers/bluetooth/hci_bcm.c
>>>>> +++ b/drivers/bluetooth/hci_bcm.c
>>>>> @@ -88,6 +88,8 @@ struct bcm_device_data {
>>>>> * used to disable flow control during runtime suspend and system sleep
>>>>> * @is_suspended: whether flow control is currently disabled
>>>>> * @disallow_set_baudrate: don't allow set_baudrate
>>>>> + * @has_pcm_params: whether PCM parameters need to be configured
>>>>> + * @pcm_params: PCM and routing parameters
>>>>> */
>>>>> struct bcm_device {
>>>>> /* Must be the first member, hci_serdev.c expects this. */
>>>>> @@ -122,6 +124,9 @@ struct bcm_device {
>>>>> bool is_suspended;
>>>>> #endif
>>>>> bool disallow_set_baudrate;
>>>>> +
>>>>> + bool has_pcm_params;
>>>>> + struct bcm_set_pcm_int_params pcm_params;
>>>>> };
>>>>>
>>>>> /* generic bcm uart resources */
>>>>> @@ -596,6 +601,16 @@ static int bcm_setup(struct hci_uart *hu)
>>>>> host_set_baudrate(hu, speed);
>>>>> }
>>>>>
>>>>> + /* PCM parameters if any*/
>>>>> + if (bcm->dev && bcm->dev->has_pcm_params) {
>>>>> + err = btbcm_set_pcm_int_params(hu->hdev, &bcm->dev->pcm_params);
>>>>> +
>>>>> + if (err) {
>>>>> + bt_dev_info(hu->hdev, "BCM: Set pcm params failed (%d)",
>>>>> + err);
>>>>> + }
>>>>> + }
>>>>> +
>>>>> finalize:
>>>>> release_firmware(fw);
>>>>>
>>>>> @@ -1132,7 +1147,24 @@ static int bcm_acpi_probe(struct bcm_device *dev)
>>>>>
>>>>> static int bcm_of_probe(struct bcm_device *bdev)
>>>>> {
>>>>> + int err;
>>>>> +
>>>>> device_property_read_u32(bdev->dev, "max-speed", &bdev->oper_speed);
>>>>> +
>>>>> + err = device_property_read_u8(bdev->dev, "brcm,bt-sco-routing",
>>>>> + &bdev->pcm_params.routing);
>>>>> + if (!err)
>>>>> + bdev->has_pcm_params = true;
>>>>
>>>> I think in case of HCI as routing path, these should be using the default or zero values as defined by Broadcom.
>>>
>>> I'm not sure what these default values should be. Wouldn't it be
>>> reasonable to expect the user/developer to set the various brcm
>>> parameters in device tree?
>>> If unset, it's just 0.
>>
>> if that works with the hardware I am fine with that. The other option is to actually first read the current values. And then only change the ones that are supplied by the DT.
>
> I don't know of a read pcm params command (this would be nice to have).
>
> I think it might be prudent to default the frame_mode and clock_mode
> to master (0x1). I'll test how the hardware responds to 0x0 and update
> the default to 0x1 if things fail badly.
it is either one opcode down or one opcode up. Look at monitor/broadcom.c since we do actually decode some of these.
Regards
Marcel
