ASoC: qcom: soundwire: Enable soundwire bus clock for version 1.6

PATCH Superseded Srinivasa Rao Mandadapu 3 participants 7 messages Oct 1, 2021 – Oct 8, 2021

Subsystems (3)

Arm Qualcomm mailing list Qualcomm audio (ASoC) drivers SoundWire

lore

Share & tools

lore

Srinivasa Rao Mandadapu

author

2021-10-01 16:25 UTC #0 lore raw

PATCH 1/1 ASoC: qcom: soundwire: Enable soundwire bus clock for version 1.6

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

Signed-off-by Venkata Prasad Potturu <[email protected]>

Signed-off-by Srinivasa Rao Mandadapu <[email protected]>

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

drivers/soundwire/qcom.c +11 -1

index 0ef79d6..599b3ed 100644
@@ -127,6 +127,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 +611,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 &= 0xFFFFFFFD;
+		iowrite32(val, ctrl->swrm_hctl_reg);
+	}
+
 	ctrl->reg_write(ctrl, SWRM_MCP_FRAME_CTRL_BANK_ADDR(0), val);
 
 	/* Enable Auto enumeration */

@@ -1200,7 +1207,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 +1258,9 @@ 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);
+
 	ret = qcom_swrm_get_port_config(ctrl);
 	if (ret)
 		goto err_clk;

Signature · 2 lines

Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.,
is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

Pierre-Louis Bossart

2021-10-01 17:57 UTC #1 lore raw

drivers/soundwire/qcom.c static int qcom_swrm_init(struct qcom_swrm_ctrl *ctrl) · line 616

Quoted 8 lines

drivers/soundwire/qcom.c +3 -0

@@ -610,6 +611,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 &= 0xFFFFFFFD;

magic value, use a #define MASK_SOMETHING?

Quoted 16 lines

drivers/soundwire/qcom.c +4 -1

+		iowrite32(val, ctrl->swrm_hctl_reg);
+	}
+
 	ctrl->reg_write(ctrl, SWRM_MCP_FRAME_CTRL_BANK_ADDR(0), val);
 
 	/* Enable Auto enumeration */

@@ -1200,7 +1207,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)

drivers/soundwire/qcom.c static int qcom_swrm_probe(struct platform_device *pdev) · line 1262

Quoted 5 lines

drivers/soundwire/qcom.c +2 -0

@@ -1251,6 +1258,9 @@ 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;

Quoted 5 lines

drivers/soundwire/qcom.c +1 -0

+
 	ret = qcom_swrm_get_port_config(ctrl);
 	if (ret)
 		goto err_clk;

Srinivasa Rao Mandadapu

author

2021-10-05 14:14 UTC #2 lore raw

On 10/1/2021 11:27 PM, Pierre-Louis Bossart wrote:
Thanks for Your time Bossart!!!

Quoted 8 lines

drivers/soundwire/qcom.c static int qcom_swrm_init(struct qcom_swrm_ctrl *ctrl) · line 616
Quoted 7 lines
drivers/soundwire/qcom.c +3 -0
@@ -610,6 +611,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 &= 0xFFFFFFFD;
magic value, use a #define MASK_SOMETHING?

Okay. will update it.

Quoted 20 lines

drivers/soundwire/qcom.c +4 -1

+		iowrite32(val, ctrl->swrm_hctl_reg);
+	}
+
  	ctrl->reg_write(ctrl, SWRM_MCP_FRAME_CTRL_BANK_ADDR(0), val);
 
  	/* Enable Auto enumeration */

@@ -1200,7 +1207,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 +1258,9 @@ 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;

Quoted 6 lines

drivers/soundwire/qcom.c static int qcom_swrm_probe(struct platform_device *pdev) · line 1262
Quoted 2 lines
drivers/soundwire/qcom.c +2 -0
+	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;
?

I think here error check is not required, as this change is required only for soundwire version 1.6 and above.

Quoted 5 lines

drivers/soundwire/qcom.c +1 -0

+
  	ret = qcom_swrm_get_port_config(ctrl);
  	if (ret)
  		goto err_clk;

Signature · 2 lines

Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.,
is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

Pierre-Louis Bossart

2021-10-05 15:05 UTC #3 lore raw

Quoted 10 lines

Quoted 8 lines
Quoted 4 lines
+ ctrl->swrm_hctl_reg = devm_ioremap(&pdev->dev, swrm_hctl_reg, 0x4);
if (!ctrl->swrm_hctl_reg)
return -ENODEV;
?
I think here error check is not required, as this change is required only for soundwire version 1.6 and above.

My comment had nothing to do with versions, it's just that ioremap can fail and in general it's wise to test for errors...

Srinivasa Rao Mandadapu

author

2021-10-05 15:13 UTC #4 lore raw

Quoted 4 lines · Pierre-Louis Bossart

Thanks for Your time Bossart!!!
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
+ ctrl->swrm_hctl_reg = devm_ioremap(&pdev->dev, swrm_hctl_reg, 0x4);

Quoted 8 lines

Quoted 6 lines
Quoted 4 lines
if (!ctrl->swrm_hctl_reg)
return -ENODEV;
?
I think here error check is not required, as this change is required only for soundwire version 1.6 and above.
My comment had nothing to do with versions, it's just that ioremap can fail and in general it's wise to test for errors...

Okay. My intention is if offset (swrm_hctl_reg) is zero, devm_ioremap may return error.

In that case we need to ignore error.

Signature · 2 lines

Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.,
is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

Bjorn Andersson

NAK

2021-10-05 17:00 UTC #5 lore raw

In reply to Srinivasa Rao Mandadapu

Quoted 21 lines · Srinivasa Rao Mandadapu

Add support for soundwire 1.6 version to gate RX/TX bus clock.
Signed-off-by Venkata Prasad Potturu <[email protected]>
Signed-off-by Srinivasa Rao Mandadapu <[email protected]>
---
 drivers/soundwire/qcom.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)
drivers/soundwire/qcom.c +1 -0
index 0ef79d6..599b3ed 100644
@@ -127,6 +127,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;

drivers/soundwire/qcom.c static int qcom_swrm_init(struct qcom_swrm_ctrl *ctrl) · line 616

Quoted 6 lines

drivers/soundwire/qcom.c +3 -0

@@ -610,6 +611,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 &= 0xFFFFFFFD;

That's a tricky way of saying:

	val &= ~BIT(1);

That said, naming bit 1 is still a very good thing.

Quoted 16 lines

drivers/soundwire/qcom.c +4 -1

+		iowrite32(val, ctrl->swrm_hctl_reg);
+	}
+
 	ctrl->reg_write(ctrl, SWRM_MCP_FRAME_CTRL_BANK_ADDR(0), val);
 
 	/* Enable Auto enumeration */

@@ -1200,7 +1207,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)

drivers/soundwire/qcom.c static int qcom_swrm_probe(struct platform_device *pdev) · line 1262

Quoted 5 lines

drivers/soundwire/qcom.c +2 -0

@@ -1251,6 +1258,9 @@ 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);

Nack.

You may not pull an address to a single register out of an undocumented DT property and blindly ioremap that.

And you surely should check for errors here, to avoid magical errors caused by this ioremap failing and your bit not being cleared.

Thanks,
Bjorn

Quoted 8 lines

drivers/soundwire/qcom.c +1 -0

+
 	ret = qcom_swrm_get_port_config(ctrl);
 	if (ret)
 		goto err_clk;

Signature · 2 lines

Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.,
is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

Srinivasa Rao Mandadapu

author

2021-10-08 05:31 UTC #6 lore raw

Thanks for Your time Bjorn!!!

Quoted 26 lines · Bjorn Andersson

Quoted 24 lines · Srinivasa Rao Mandadapu

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

Signed-off-by Venkata Prasad Potturu <[email protected]>

Signed-off-by Srinivasa Rao Mandadapu <[email protected]>

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

drivers/soundwire/qcom.c +1 -0

index 0ef79d6..599b3ed 100644
@@ -127,6 +127,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 +611,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);

Quoted 8 lines

drivers/soundwire/qcom.c static int qcom_swrm_init(struct qcom_swrm_ctrl *ctrl) · line 616
Quoted 3 lines
drivers/soundwire/qcom.c +3 -0
+	if (ctrl->swrm_hctl_reg) {
+		val = ioread32(ctrl->swrm_hctl_reg);
+		val &= 0xFFFFFFFD;
That's a tricky way of saying:
	val &= ~BIT(1);
That said, naming bit 1 is still a very good thing.

Okay. will change accordingly.

Quoted 24 lines

Quoted 17 lines

drivers/soundwire/qcom.c +4 -1

+		iowrite32(val, ctrl->swrm_hctl_reg);
+	}
+
  	ctrl->reg_write(ctrl, SWRM_MCP_FRAME_CTRL_BANK_ADDR(0), val);
 
  	/* Enable Auto enumeration */

@@ -1200,7 +1207,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)

drivers/soundwire/qcom.c static int qcom_swrm_probe(struct platform_device *pdev) · line 1262

Quoted 5 lines

drivers/soundwire/qcom.c +2 -0

@@ -1251,6 +1258,9 @@ 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);

Nack.

Quoted 8 lines

You may not pull an address to a single register out of an undocumented DT property and blindly ioremap that.
And you surely should check for errors here, to avoid magical errors caused by this ioremap failing and your bit not being cleared.
Thanks,
Bjorn

Okay. will add error check, as Bossart Suggested.

Quoted 9 lines

drivers/soundwire/qcom.c +1 -0

+
  	ret = qcom_swrm_get_port_config(ctrl);
  	if (ret)
  		goto err_clk;

Signature · 2 lines

Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.,
is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

Signature · 2 lines

Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.,
is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.