* [PATCH] ASoC: qcom: soundwire: Enable soundwire bus clock for version 1.6
@ 2021-10-01 16:24 Srinivasa Rao Mandadapu
2021-10-01 17:57 ` Pierre-Louis Bossart
2021-10-05 17:01 ` Bjorn Andersson
0 siblings, 2 replies; 7+ messages in thread
From: Srinivasa Rao Mandadapu @ 2021-10-01 16:24 UTC (permalink / raw)
To: agross, bjorn.andersson, lgirdwood, broonie, robh+dt, plai,
bgoswami, perex, tiwai, srinivas.kandagatla, rohitkr,
linux-arm-msm, alsa-devel, devicetree, linux-kernel, swboyd,
judyhsiao
Cc: Srinivasa Rao Mandadapu, Venkata Prasad Potturu
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>
---
drivers/soundwire/qcom.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
index 0ef79d6..599b3ed 100644
--- a/drivers/soundwire/qcom.c
+++ b/drivers/soundwire/qcom.c
@@ -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;
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.,
is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH] ASoC: qcom: soundwire: Enable soundwire bus clock for version 1.6
2021-10-01 16:24 [PATCH] ASoC: qcom: soundwire: Enable soundwire bus clock for version 1.6 Srinivasa Rao Mandadapu
@ 2021-10-01 17:57 ` Pierre-Louis Bossart
2021-10-05 14:13 ` Srinivasa Rao Mandadapu
2021-10-05 17:01 ` Bjorn Andersson
1 sibling, 1 reply; 7+ messages in thread
From: Pierre-Louis Bossart @ 2021-10-01 17:57 UTC (permalink / raw)
To: Srinivasa Rao Mandadapu, agross, bjorn.andersson, lgirdwood,
broonie, robh+dt, plai, bgoswami, perex, tiwai,
srinivas.kandagatla, rohitkr, linux-arm-msm, alsa-devel,
devicetree, linux-kernel, swboyd, judyhsiao
Cc: Venkata Prasad Potturu
> @@ -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?
> + 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);
if (!ctrl->swrm_hctl_reg)
return -ENODEV;
?
> +
> ret = qcom_swrm_get_port_config(ctrl);
> if (ret)
> goto err_clk;
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] ASoC: qcom: soundwire: Enable soundwire bus clock for version 1.6
2021-10-01 17:57 ` Pierre-Louis Bossart
@ 2021-10-05 14:13 ` Srinivasa Rao Mandadapu
2021-10-05 15:03 ` Pierre-Louis Bossart
0 siblings, 1 reply; 7+ messages in thread
From: Srinivasa Rao Mandadapu @ 2021-10-05 14:13 UTC (permalink / raw)
To: Pierre-Louis Bossart, agross, bjorn.andersson, lgirdwood,
broonie, robh+dt, plai, bgoswami, perex, tiwai,
srinivas.kandagatla, rohitkr, linux-arm-msm, alsa-devel,
devicetree, linux-kernel, swboyd, judyhsiao
Cc: Venkata Prasad Potturu
On 10/1/2021 11:27 PM, Pierre-Louis Bossart wrote:
Thanks for Your time Bossart!!!
>> @@ -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.
>
>> + 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);
> 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.
>> +
>> 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.
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] ASoC: qcom: soundwire: Enable soundwire bus clock for version 1.6
2021-10-05 14:13 ` Srinivasa Rao Mandadapu
@ 2021-10-05 15:03 ` Pierre-Louis Bossart
2021-10-05 15:13 ` Srinivasa Rao Mandadapu
0 siblings, 1 reply; 7+ messages in thread
From: Pierre-Louis Bossart @ 2021-10-05 15:03 UTC (permalink / raw)
To: Srinivasa Rao Mandadapu, agross, bjorn.andersson, lgirdwood,
broonie, robh+dt, plai, bgoswami, perex, tiwai,
srinivas.kandagatla, rohitkr, linux-arm-msm, alsa-devel,
devicetree, linux-kernel, swboyd, judyhsiao
Cc: Venkata Prasad Potturu
>>> + 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...
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ASoC: qcom: soundwire: Enable soundwire bus clock for version 1.6
2021-10-05 15:03 ` Pierre-Louis Bossart
@ 2021-10-05 15:13 ` Srinivasa Rao Mandadapu
0 siblings, 0 replies; 7+ messages in thread
From: Srinivasa Rao Mandadapu @ 2021-10-05 15:13 UTC (permalink / raw)
To: Pierre-Louis Bossart, agross, bjorn.andersson, lgirdwood,
broonie, robh+dt, plai, bgoswami, perex, tiwai,
srinivas.kandagatla, rohitkr, linux-arm-msm, alsa-devel,
devicetree, linux-kernel, swboyd, judyhsiao
Cc: Venkata Prasad Potturu
On 10/5/2021 8:33 PM, Pierre-Louis Bossart wrote:
> Thanks for Your time Bossart!!!
>
>>>> + 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...
Okay. My intention is if offset (swrm_hctl_reg) is zero, devm_ioremap
may return error.
In that case we need to ignore error.
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.,
is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ASoC: qcom: soundwire: Enable soundwire bus clock for version 1.6
2021-10-01 16:24 [PATCH] ASoC: qcom: soundwire: Enable soundwire bus clock for version 1.6 Srinivasa Rao Mandadapu
2021-10-01 17:57 ` Pierre-Louis Bossart
@ 2021-10-05 17:01 ` Bjorn Andersson
2021-10-08 5:30 ` Srinivasa Rao Mandadapu
1 sibling, 1 reply; 7+ messages in thread
From: Bjorn Andersson @ 2021-10-05 17:01 UTC (permalink / raw)
To: Srinivasa Rao Mandadapu
Cc: agross, lgirdwood, broonie, robh+dt, plai, bgoswami, perex,
tiwai, srinivas.kandagatla, rohitkr, linux-arm-msm, alsa-devel,
devicetree, linux-kernel, swboyd, judyhsiao,
Venkata Prasad Potturu
On Fri 01 Oct 09:24 PDT 2021, Srinivasa Rao Mandadapu wrote:
> 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>
> ---
> drivers/soundwire/qcom.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
> index 0ef79d6..599b3ed 100644
> --- a/drivers/soundwire/qcom.c
> +++ b/drivers/soundwire/qcom.c
> @@ -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;
That's a tricky way of saying:
val &= ~BIT(1);
That said, naming bit 1 is still a very good thing.
> + 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);
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
> +
> 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.
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] ASoC: qcom: soundwire: Enable soundwire bus clock for version 1.6
2021-10-05 17:01 ` Bjorn Andersson
@ 2021-10-08 5:30 ` Srinivasa Rao Mandadapu
0 siblings, 0 replies; 7+ messages in thread
From: Srinivasa Rao Mandadapu @ 2021-10-08 5:30 UTC (permalink / raw)
To: Bjorn Andersson
Cc: agross, lgirdwood, broonie, robh+dt, plai, bgoswami, perex,
tiwai, srinivas.kandagatla, rohitkr, linux-arm-msm, alsa-devel,
devicetree, linux-kernel, swboyd, judyhsiao,
Venkata Prasad Potturu
Thanks for Your time Bjorn!!!
On 10/5/2021 10:31 PM, Bjorn Andersson wrote:
> On Fri 01 Oct 09:24 PDT 2021, Srinivasa Rao Mandadapu wrote:
>
>> 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>
>> ---
>> drivers/soundwire/qcom.c | 12 +++++++++++-
>> 1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
>> index 0ef79d6..599b3ed 100644
>> --- a/drivers/soundwire/qcom.c
>> +++ b/drivers/soundwire/qcom.c
>> @@ -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;
> 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.
>
>> + 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);
> 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
Okay. will add error check, as Bossart Suggested.
>
>> +
>> 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.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-10-08 5:31 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-01 16:24 [PATCH] ASoC: qcom: soundwire: Enable soundwire bus clock for version 1.6 Srinivasa Rao Mandadapu
2021-10-01 17:57 ` Pierre-Louis Bossart
2021-10-05 14:13 ` Srinivasa Rao Mandadapu
2021-10-05 15:03 ` Pierre-Louis Bossart
2021-10-05 15:13 ` Srinivasa Rao Mandadapu
2021-10-05 17:01 ` Bjorn Andersson
2021-10-08 5:30 ` Srinivasa Rao Mandadapu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).