[PATCH v2] ASoC: qcom: soundwire: Add support for controlling audio CGCR from HLOS

[Srinivasa Rao Mandadapu]

On 6/1/2022 6:36 PM, Srinivas Kandagatla wrote:
Thanks for Your time Srini!!
>
>
> On 01/06/2022 13:57, Srinivasa Rao Mandadapu wrote:
>>
>> On 6/1/2022 4:02 AM, Matthias Kaehlcke wrote:
>> Thanks for Your Time Matthias!!!
>>> On Tue, May 24, 2022 at 04:19:47PM +0530, Srinivasa Rao Mandadapu 
>>> wrote:
>>>> On 5/21/2022 8:43 AM, Stephen Boyd wrote:
>>>> Thanks for your time Stephen!!!
>>>>> Quoting Srinivasa Rao Mandadapu (2022-05-18 05:42:35)
>>>>>> diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
>>>>>> index da1ad7e..445e481 100644
>>>>>> --- a/drivers/soundwire/qcom.c
>>>>>> +++ b/drivers/soundwire/qcom.c
>>>>>> @@ -1333,6 +1337,10 @@ static int qcom_swrm_probe(struct 
>>>>>> platform_device *pdev)
>>>>>>           ctrl->bus.compute_params = &qcom_swrm_compute_params;
>>>>>>           ctrl->bus.clk_stop_timeout = 300;
>>>>>>
>>>>>> +       ctrl->audio_cgcr = devm_reset_control_get_exclusive(dev, 
>>>>>> "swr_audio_cgcr");
>>>>>> +       if (IS_ERR(ctrl->audio_cgcr))
>>>>>> +               dev_err(dev, "Failed to get audio_cgcr reset 
>>>>>> required for soundwire-v1.6.0\n");
>>>>> Why is there no return on error here? Is the reset optional?
>>>> Yes it's optional. For older platforms this is not required.
>>> If it's optional then either there should be no error message, or the
>>> error message should only be logged when the version is >= 1.6.0. There
>>> are few things worse than a kernel log riddled with misleading error
>>> messages.
>>
>> In that case, it can be done like below. Kindly let me know your 
>> opinion on this.
>>
>> if (ctrl->version >= 0x01060000) {
>
> This is not true 1.7+ variants do not require anything as such.

I think it applies for all upcoming versions as Qualcomm Hardware team. 
Here is the not from HW Team.

*/Issue: /*

HW CTL on AHB interface to SWR/Codec slave is default ON, the AHB clock 
is ungated for programmable cycles per transaction

(from valid htrans until slave ready or hysteresis count) and therefore 
hclk is not free-running.

Without a free-running HCLK, interrupt, CMD FIFO, slave status and other 
similar logic will have functional issues.

SWR has internal clock gating for address accesses but some logic is 
running on HCLK

*/Fix: /*

Registers to control HW_CTL is accessible via HLOS – so software has to 
always ungate hclk if soundwire is operational and keep it running

*/CSR: /*

SoundWire TX/RX.WSA :

Register: *LPASS_SWR_TX/RX/WSA_CONFIG_CGCR* | 0xXXXC010

	
	
	
	

0x3

	

0x1

*Bits*

	

*Field Name*

	

*Access*

	

*Description*

	

*Current PoR*

	

*SW Update*

1

	

HW_CTL

	

	

HW Dynamic Clock Gating Control Register 1: HW Controlled 0: SW Controlled

	

1

	

0

0

	

CLK_ENABLE

	

	

Enabling the clock when in SW Controlled Mode 1: Clock Enabled 0: Clock 
Disabled

	

1

	

1

>
> Why not add a flag in struct qcom_swrm_data and pass it as part of 
> of_match data specific to this version?
>
> --srini
>>      ctrl->audio_cgcr = devm_reset_control_get_exclusive(dev, 
>> "swr_audio_cgcr");
>>          if (IS_ERR(ctrl->audio_cgcr)) {
>>              dev_err(dev, "Failed to get audio_cgcr reset required 
>> for soundwire-v1.6.0\n");
>>              ret = PTR_ERR(ctrl->audio_cgcr);
>>              goto err_clk;
>>          }
>>      }
>>