[PATCH v3] ASoC: qcom: soundwire: Enable soundwire bus clock for version 1.6

[PATCH v3] ASoC: qcom: soundwire: Enable soundwire bus clock for version 1.6

[Srinivasa Rao Mandadapu]

Add support for soundwire 1.6 version to gate RX/TX bus clock.

Signed-off-by: Venkata Prasad Potturu <potturu@codeaurora.org>
Signed-off-by: Srinivasa Rao Mandadapu <srivasam@codeaurora.org>
---
Changes since v2:
    -- Update error check after ioremap.
Changes since v1:
    -- Add const name to mask value.

 drivers/soundwire/qcom.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
index 0ef79d6..bd6fabd 100644
--- a/drivers/soundwire/qcom.c
+++ b/drivers/soundwire/qcom.c
@@ -109,6 +109,7 @@
 #define SWR_MAX_CMD_ID	14
 #define MAX_FIFO_RD_RETRY 3
 #define SWR_OVERFLOW_RETRY_COUNT 30
+#define SWRM_HCTL_REG_MASK ~BIT(1)
 
 struct qcom_swrm_port_config {
 	u8 si;
@@ -127,6 +128,7 @@ struct qcom_swrm_ctrl {
 	struct device *dev;
 	struct regmap *regmap;
 	void __iomem *mmio;
+	char __iomem *swrm_hctl_reg;
 	struct completion broadcast;
 	struct completion enumeration;
 	struct work_struct slave_work;
@@ -610,6 +612,12 @@ static int qcom_swrm_init(struct qcom_swrm_ctrl *ctrl)
 	val = FIELD_PREP(SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_BMSK, ctrl->rows_index);
 	val |= FIELD_PREP(SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_BMSK, ctrl->cols_index);
 
+	if (ctrl->swrm_hctl_reg) {
+		val = ioread32(ctrl->swrm_hctl_reg);
+		val &= SWRM_HCTL_REG_MASK;
+		iowrite32(val, ctrl->swrm_hctl_reg);
+	}
+
 	ctrl->reg_write(ctrl, SWRM_MCP_FRAME_CTRL_BANK_ADDR(0), val);
 
 	/* Enable Auto enumeration */
@@ -1200,7 +1208,7 @@ static int qcom_swrm_probe(struct platform_device *pdev)
 	struct qcom_swrm_ctrl *ctrl;
 	const struct qcom_swrm_data *data;
 	int ret;
-	u32 val;
+	int val, swrm_hctl_reg = 0;
 
 	ctrl = devm_kzalloc(dev, sizeof(*ctrl), GFP_KERNEL);
 	if (!ctrl)
@@ -1251,6 +1259,11 @@ static int qcom_swrm_probe(struct platform_device *pdev)
 	ctrl->bus.port_ops = &qcom_swrm_port_ops;
 	ctrl->bus.compute_params = &qcom_swrm_compute_params;
 
+	if (!of_property_read_u32(dev->of_node, "qcom,swrm-hctl-reg", &swrm_hctl_reg)) {
+		ctrl->swrm_hctl_reg = devm_ioremap(&pdev->dev, swrm_hctl_reg, 0x4);
+		if (!ctrl->swrm_hctl_reg)
+			return -ENODEV;
+	}
 	ret = qcom_swrm_get_port_config(ctrl);
 	if (ret)
 		goto err_clk;
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.,
is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH v3] ASoC: qcom: soundwire: Enable soundwire bus clock for version 1.6

[Bjorn Andersson]

On Thu 07 Oct 22:33 PDT 2021, Srinivasa Rao Mandadapu wrote:

> Add support for soundwire 1.6 version to gate RX/TX bus clock.
> 

Are you really adding soundwire 1.6 support in order to gate RX/TX bus
clock?

Could it be that you're ungating the bus clock so that soundwire 1.6
starts working? The commit message should properly describe why you're
doing your change.

> Signed-off-by: Venkata Prasad Potturu <potturu@codeaurora.org>
> Signed-off-by: Srinivasa Rao Mandadapu <srivasam@codeaurora.org>

Venkata is the first who certified the origin of this patch, yet you're
the author. Either this should be From Venkata (i.e. git commit
--author) or perhaps you need a Co-developed-by here to say that you
collaborated on this and both certify its origin.

> ---
> Changes since v2:
>     -- Update error check after ioremap.

What about the other things I noted in v2?

> Changes since v1:
>     -- Add const name to mask value.
> 
>  drivers/soundwire/qcom.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
> index 0ef79d6..bd6fabd 100644
> --- a/drivers/soundwire/qcom.c
> +++ b/drivers/soundwire/qcom.c
> @@ -109,6 +109,7 @@
>  #define SWR_MAX_CMD_ID	14
>  #define MAX_FIFO_RD_RETRY 3
>  #define SWR_OVERFLOW_RETRY_COUNT 30
> +#define SWRM_HCTL_REG_MASK ~BIT(1)
>  
>  struct qcom_swrm_port_config {
>  	u8 si;
> @@ -127,6 +128,7 @@ struct qcom_swrm_ctrl {
>  	struct device *dev;
>  	struct regmap *regmap;
>  	void __iomem *mmio;
> +	char __iomem *swrm_hctl_reg;
>  	struct completion broadcast;
>  	struct completion enumeration;
>  	struct work_struct slave_work;
> @@ -610,6 +612,12 @@ static int qcom_swrm_init(struct qcom_swrm_ctrl *ctrl)
>  	val = FIELD_PREP(SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_BMSK, ctrl->rows_index);
>  	val |= FIELD_PREP(SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_BMSK, ctrl->cols_index);
>  
> +	if (ctrl->swrm_hctl_reg) {
> +		val = ioread32(ctrl->swrm_hctl_reg);
> +		val &= SWRM_HCTL_REG_MASK;

Make a define with a name that clarifies what BIT(1) is and use that
here, hiding a magic number in an empty define isn't making this more
maintainable.

Essentially put the name of the bit in the register description in a
define and use that here.

> +		iowrite32(val, ctrl->swrm_hctl_reg);
> +	}
> +
>  	ctrl->reg_write(ctrl, SWRM_MCP_FRAME_CTRL_BANK_ADDR(0), val);
>  
>  	/* Enable Auto enumeration */
> @@ -1200,7 +1208,7 @@ static int qcom_swrm_probe(struct platform_device *pdev)
>  	struct qcom_swrm_ctrl *ctrl;
>  	const struct qcom_swrm_data *data;
>  	int ret;
> -	u32 val;
> +	int val, swrm_hctl_reg = 0;

Don't you get a warning from passing val as an int to a function that
takes a u32 pointer?

Also there's no reason to zero-initialize swrm_hctl_reg.

>  
>  	ctrl = devm_kzalloc(dev, sizeof(*ctrl), GFP_KERNEL);
>  	if (!ctrl)
> @@ -1251,6 +1259,11 @@ static int qcom_swrm_probe(struct platform_device *pdev)
>  	ctrl->bus.port_ops = &qcom_swrm_port_ops;
>  	ctrl->bus.compute_params = &qcom_swrm_compute_params;
>  
> +	if (!of_property_read_u32(dev->of_node, "qcom,swrm-hctl-reg", &swrm_hctl_reg)) {

As I said in my feedback of v2, this property is not documented in the
DT binding.


But more important, upstream we do not approve of the downstream
methodology of having properties pointing to single registers in some
memory block somewhere.

Describe the hardware block that you reference fully in devicetree and
make a proper reference to it.

Unfortunately your patch lacks details necessary to know where this
register lives, so it's not possible for me to recommend a proper
design.

Regards,
Bjorn

> +		ctrl->swrm_hctl_reg = devm_ioremap(&pdev->dev, swrm_hctl_reg, 0x4);
> +		if (!ctrl->swrm_hctl_reg)
> +			return -ENODEV;
> +	}
>  	ret = qcom_swrm_get_port_config(ctrl);
>  	if (ret)
>  		goto err_clk;
> -- 
> Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.,
> is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
> 

Re: [PATCH v3] ASoC: qcom: soundwire: Enable soundwire bus clock for version 1.6

[Srinivasa Rao Mandadapu]

On 10/8/2021 8:05 PM, Bjorn Andersson wrote:
Thanks for Your time Bjorn!!!
> On Thu 07 Oct 22:33 PDT 2021, Srinivasa Rao Mandadapu wrote:
>
>> Add support for soundwire 1.6 version to gate RX/TX bus clock.
>>
> Are you really adding soundwire 1.6 support in order to gate RX/TX bus
> clock?
>
> Could it be that you're ungating the bus clock so that soundwire 1.6
> starts working? The commit message should properly describe why you're
> doing your change.
Yes. After updating RX/TX CGCR Register so it's started working. Will 
update proper commit message.
>> Signed-off-by: Venkata Prasad Potturu <potturu@codeaurora.org>
>> Signed-off-by: Srinivasa Rao Mandadapu <srivasam@codeaurora.org>
> Venkata is the first who certified the origin of this patch, yet you're
> the author. Either this should be From Venkata (i.e. git commit
> --author) or perhaps you need a Co-developed-by here to say that you
> collaborated on this and both certify its origin.
Okay. Actually Venakta is the Co developer. Will change accordingly.
>> ---
>> Changes since v2:
>>      -- Update error check after ioremap.
> What about the other things I noted in v2?
Okay. Will update.
>
>> Changes since v1:
>>      -- Add const name to mask value.
>>
>>   drivers/soundwire/qcom.c | 15 ++++++++++++++-
>>   1 file changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
>> index 0ef79d6..bd6fabd 100644
>> --- a/drivers/soundwire/qcom.c
>> +++ b/drivers/soundwire/qcom.c
>> @@ -109,6 +109,7 @@
>>   #define SWR_MAX_CMD_ID	14
>>   #define MAX_FIFO_RD_RETRY 3
>>   #define SWR_OVERFLOW_RETRY_COUNT 30
>> +#define SWRM_HCTL_REG_MASK ~BIT(1)
>>   
>>   struct qcom_swrm_port_config {
>>   	u8 si;
>> @@ -127,6 +128,7 @@ struct qcom_swrm_ctrl {
>>   	struct device *dev;
>>   	struct regmap *regmap;
>>   	void __iomem *mmio;
>> +	char __iomem *swrm_hctl_reg;
>>   	struct completion broadcast;
>>   	struct completion enumeration;
>>   	struct work_struct slave_work;
>> @@ -610,6 +612,12 @@ static int qcom_swrm_init(struct qcom_swrm_ctrl *ctrl)
>>   	val = FIELD_PREP(SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_BMSK, ctrl->rows_index);
>>   	val |= FIELD_PREP(SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_BMSK, ctrl->cols_index);
>>   
>> +	if (ctrl->swrm_hctl_reg) {
>> +		val = ioread32(ctrl->swrm_hctl_reg);
>> +		val &= SWRM_HCTL_REG_MASK;
> Make a define with a name that clarifies what BIT(1) is and use that
> here, hiding a magic number in an empty define isn't making this more
> maintainable.
>
> Essentially put the name of the bit in the register description in a
> define and use that here.
Okay. Will change name appropriately.
>
>> +		iowrite32(val, ctrl->swrm_hctl_reg);
>> +	}
>> +
>>   	ctrl->reg_write(ctrl, SWRM_MCP_FRAME_CTRL_BANK_ADDR(0), val);
>>   
>>   	/* Enable Auto enumeration */
>> @@ -1200,7 +1208,7 @@ static int qcom_swrm_probe(struct platform_device *pdev)
>>   	struct qcom_swrm_ctrl *ctrl;
>>   	const struct qcom_swrm_data *data;
>>   	int ret;
>> -	u32 val;
>> +	int val, swrm_hctl_reg = 0;
> Don't you get a warning from passing val as an int to a function that
> takes a u32 pointer?
Yeah. Will revert val variable type change.
> Also there's no reason to zero-initialize swrm_hctl_reg.
Okay. Will change.
>>   
>>   	ctrl = devm_kzalloc(dev, sizeof(*ctrl), GFP_KERNEL);
>>   	if (!ctrl)
>> @@ -1251,6 +1259,11 @@ static int qcom_swrm_probe(struct platform_device *pdev)
>>   	ctrl->bus.port_ops = &qcom_swrm_port_ops;
>>   	ctrl->bus.compute_params = &qcom_swrm_compute_params;
>>   
>> +	if (!of_property_read_u32(dev->of_node, "qcom,swrm-hctl-reg", &swrm_hctl_reg)) {
> As I said in my feedback of v2, this property is not documented in the
> DT binding.
Okay. Will update dt bindings.
>
> But more important, upstream we do not approve of the downstream
> methodology of having properties pointing to single registers in some
> memory block somewhere.
>
> Describe the hardware block that you reference fully in devicetree and
> make a proper reference to it.
>
> Unfortunately your patch lacks details necessary to know where this
> register lives, so it's not possible for me to recommend a proper
> design.

These Registers lies in LPASS_AUDIO_LPASS_AUDIO_CSR | 0x032A9000 Range.

Register description:

LPASS_AUDIO_SWR_RX_CGCR(0x032A90A0) & LPASS_AUDIO_SWR_TX_CGCR (0x032A90A8)

Bits  Field Name    Description
1        HW_CTL        HW Dynamic Clock Gating Control Register
                                  1: HW Controlled
                                  0: SW Controlled
0        CLK_ENABLE    Enabling the clock when in SW Controlled Mode
                                     1: Clock Enabled
                                      0: Clock Disabled
> Regards,
> Bjorn
>
>> +		ctrl->swrm_hctl_reg = devm_ioremap(&pdev->dev, swrm_hctl_reg, 0x4);
>> +		if (!ctrl->swrm_hctl_reg)
>> +			return -ENODEV;
>> +	}
>>   	ret = qcom_swrm_get_port_config(ctrl);
>>   	if (ret)
>>   		goto err_clk;
>> -- 
>> Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.,
>> is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
>>
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.,
is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.