diff mbox series

[v2,2/3] interconnect: qcom: sdm660: Add missing a2noc qos clocks

Message ID 20210824043435.23190-3-shawn.guo@linaro.org (mailing list archive)
State Not Applicable
Headers show
Series Add missing A2NoC QoS clocks for SDM660 interconnect driver | expand

Commit Message

Shawn Guo Aug. 24, 2021, 4:34 a.m. UTC 
It adds the missing a2noc clocks required for QoS registers programming
per downstream kernel[1].  Otherwise, qcom_icc_noc_set_qos_priority()
call on mas_ufs or mas_usb_hs node will simply result in a hardware hang
on SDM660 SoC.

[1] https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/arch/arm/boot/dts/qcom/sdm660-bus.dtsi?h=LA.UM.8.2.r1-04800-sdm660.0#n43

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
Tested-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/interconnect/qcom/sdm660.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

Comments

AngeloGioacchino Del Regno Sept. 1, 2021, 3 p.m. UTC | #1 
Il 24/08/21 06:34, Shawn Guo ha scritto:
> It adds the missing a2noc clocks required for QoS registers programming
> per downstream kernel[1].  Otherwise, qcom_icc_noc_set_qos_priority()
> call on mas_ufs or mas_usb_hs node will simply result in a hardware hang
> on SDM660 SoC.
> 
> [1] https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/arch/arm/boot/dts/qcom/sdm660-bus.dtsi?h=LA.UM.8.2.r1-04800-sdm660.0#n43
> 
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> Tested-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>

My Xperia devices are not locking up on any call to qos priority setting, but 
that's surely because such clocks are left on by the bootloader before booting 
Linux... and then after booting my USB is up, which is probably why I still don't 
experience any hang, I guess.

In any case, I do recognize that the a2noc does effectively need these clocks to be 
on in order to be reachable, as also reported in the downstream dtsi.

Thank you!

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
Bjorn Andersson Sept. 24, 2021, 2:33 p.m. UTC | #2 
On Mon 23 Aug 23:34 CDT 2021, Shawn Guo wrote:

> It adds the missing a2noc clocks required for QoS registers programming
> per downstream kernel[1].  Otherwise, qcom_icc_noc_set_qos_priority()
> call on mas_ufs or mas_usb_hs node will simply result in a hardware hang
> on SDM660 SoC.
> 
> [1] https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/arch/arm/boot/dts/qcom/sdm660-bus.dtsi?h=LA.UM.8.2.r1-04800-sdm660.0#n43
> 
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> Tested-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Georgi, do you intend to pull this patch in for v5.15-rc?

I.e. should I pick up the dts change for v5.15 as well.

Regards,
Bjorn

> ---
>  drivers/interconnect/qcom/sdm660.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/interconnect/qcom/sdm660.c b/drivers/interconnect/qcom/sdm660.c
> index c89c991a80a0..661eb3635d21 100644
> --- a/drivers/interconnect/qcom/sdm660.c
> +++ b/drivers/interconnect/qcom/sdm660.c
> @@ -174,6 +174,16 @@ static const struct clk_bulk_data bus_mm_clocks[] = {
>  	{ .id = "iface" },
>  };
>  
> +static const struct clk_bulk_data bus_a2noc_clocks[] = {
> +	{ .id = "bus" },
> +	{ .id = "bus_a" },
> +	{ .id = "ipa" },
> +	{ .id = "ufs_axi" },
> +	{ .id = "aggre2_ufs_axi" },
> +	{ .id = "aggre2_usb3_axi" },
> +	{ .id = "cfg_noc_usb2_axi" },
> +};
> +
>  /**
>   * struct qcom_icc_provider - Qualcomm specific interconnect provider
>   * @provider: generic interconnect provider
> @@ -811,6 +821,10 @@ static int qnoc_probe(struct platform_device *pdev)
>  		qp->bus_clks = devm_kmemdup(dev, bus_mm_clocks,
>  					    sizeof(bus_mm_clocks), GFP_KERNEL);
>  		qp->num_clks = ARRAY_SIZE(bus_mm_clocks);
> +	} else if (of_device_is_compatible(dev->of_node, "qcom,sdm660-a2noc")) {
> +		qp->bus_clks = devm_kmemdup(dev, bus_a2noc_clocks,
> +					    sizeof(bus_a2noc_clocks), GFP_KERNEL);
> +		qp->num_clks = ARRAY_SIZE(bus_a2noc_clocks);
>  	} else {
>  		if (of_device_is_compatible(dev->of_node, "qcom,sdm660-bimc"))
>  			qp->is_bimc_node = true;
> -- 
> 2.17.1
>
Georgi Djakov Sept. 24, 2021, 2:58 p.m. UTC | #3 
On 24.09.21 17:33, Bjorn Andersson wrote:
> On Mon 23 Aug 23:34 CDT 2021, Shawn Guo wrote:
> 
>> It adds the missing a2noc clocks required for QoS registers programming
>> per downstream kernel[1].  Otherwise, qcom_icc_noc_set_qos_priority()
>> call on mas_ufs or mas_usb_hs node will simply result in a hardware hang
>> on SDM660 SoC.
>>
>> [1] https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/arch/arm/boot/dts/qcom/sdm660-bus.dtsi?h=LA.UM.8.2.r1-04800-sdm660.0#n43
>>
>> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
>> Tested-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> 
> Georgi, do you intend to pull this patch in for v5.15-rc?
> 
> I.e. should I pick up the dts change for v5.15 as well.

Yes, i have just sent a pull request with this included, so please
pull the dts change as a fix for v5.15-rc. Sorry for not mentioning
it explicitly.
I assume that not all bootloaders are leaving the qos clocks enabled,
so if this is fixing a hardware hang, should we also backport it into
stable? This is probably more of a question to the people actively
using this board?

Thanks,
Georgi

> Regards,
> Bjorn
> 
>> ---
>>   drivers/interconnect/qcom/sdm660.c | 14 ++++++++++++++
>>   1 file changed, 14 insertions(+)
>>
>> diff --git a/drivers/interconnect/qcom/sdm660.c b/drivers/interconnect/qcom/sdm660.c
>> index c89c991a80a0..661eb3635d21 100644
>> --- a/drivers/interconnect/qcom/sdm660.c
>> +++ b/drivers/interconnect/qcom/sdm660.c
>> @@ -174,6 +174,16 @@ static const struct clk_bulk_data bus_mm_clocks[] = {
>>   	{ .id = "iface" },
>>   };
>>   
>> +static const struct clk_bulk_data bus_a2noc_clocks[] = {
>> +	{ .id = "bus" },
>> +	{ .id = "bus_a" },
>> +	{ .id = "ipa" },
>> +	{ .id = "ufs_axi" },
>> +	{ .id = "aggre2_ufs_axi" },
>> +	{ .id = "aggre2_usb3_axi" },
>> +	{ .id = "cfg_noc_usb2_axi" },
>> +};
>> +
>>   /**
>>    * struct qcom_icc_provider - Qualcomm specific interconnect provider
>>    * @provider: generic interconnect provider
>> @@ -811,6 +821,10 @@ static int qnoc_probe(struct platform_device *pdev)
>>   		qp->bus_clks = devm_kmemdup(dev, bus_mm_clocks,
>>   					    sizeof(bus_mm_clocks), GFP_KERNEL);
>>   		qp->num_clks = ARRAY_SIZE(bus_mm_clocks);
>> +	} else if (of_device_is_compatible(dev->of_node, "qcom,sdm660-a2noc")) {
>> +		qp->bus_clks = devm_kmemdup(dev, bus_a2noc_clocks,
>> +					    sizeof(bus_a2noc_clocks), GFP_KERNEL);
>> +		qp->num_clks = ARRAY_SIZE(bus_a2noc_clocks);
>>   	} else {
>>   		if (of_device_is_compatible(dev->of_node, "qcom,sdm660-bimc"))
>>   			qp->is_bimc_node = true;
>> -- 
>> 2.17.1
>>
Bjorn Andersson Sept. 24, 2021, 3:20 p.m. UTC | #4 
On Fri 24 Sep 07:58 PDT 2021, Georgi Djakov wrote:

> On 24.09.21 17:33, Bjorn Andersson wrote:
> > On Mon 23 Aug 23:34 CDT 2021, Shawn Guo wrote:
> > 
> > > It adds the missing a2noc clocks required for QoS registers programming
> > > per downstream kernel[1].  Otherwise, qcom_icc_noc_set_qos_priority()
> > > call on mas_ufs or mas_usb_hs node will simply result in a hardware hang
> > > on SDM660 SoC.
> > > 
> > > [1] https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/arch/arm/boot/dts/qcom/sdm660-bus.dtsi?h=LA.UM.8.2.r1-04800-sdm660.0#n43
> > > 
> > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > > Tested-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > 
> > Georgi, do you intend to pull this patch in for v5.15-rc?
> > 
> > I.e. should I pick up the dts change for v5.15 as well.
> 
> Yes, i have just sent a pull request with this included, so please
> pull the dts change as a fix for v5.15-rc. Sorry for not mentioning
> it explicitly.
> I assume that not all bootloaders are leaving the qos clocks enabled,
> so if this is fixing a hardware hang, should we also backport it into
> stable? This is probably more of a question to the people actively
> using this board?
> 

Devices that needed this change wouldn't boot on that older kernel
anyways, so I think it's safe to assume that we don't have any users on
an old kernel that would benefit from this change.

So v5.15 is probably sufficient.

Regards,
Bjorn

> Thanks,
> Georgi
> 
> > Regards,
> > Bjorn
> > 
> > > ---
> > >   drivers/interconnect/qcom/sdm660.c | 14 ++++++++++++++
> > >   1 file changed, 14 insertions(+)
> > > 
> > > diff --git a/drivers/interconnect/qcom/sdm660.c b/drivers/interconnect/qcom/sdm660.c
> > > index c89c991a80a0..661eb3635d21 100644
> > > --- a/drivers/interconnect/qcom/sdm660.c
> > > +++ b/drivers/interconnect/qcom/sdm660.c
> > > @@ -174,6 +174,16 @@ static const struct clk_bulk_data bus_mm_clocks[] = {
> > >   	{ .id = "iface" },
> > >   };
> > > +static const struct clk_bulk_data bus_a2noc_clocks[] = {
> > > +	{ .id = "bus" },
> > > +	{ .id = "bus_a" },
> > > +	{ .id = "ipa" },
> > > +	{ .id = "ufs_axi" },
> > > +	{ .id = "aggre2_ufs_axi" },
> > > +	{ .id = "aggre2_usb3_axi" },
> > > +	{ .id = "cfg_noc_usb2_axi" },
> > > +};
> > > +
> > >   /**
> > >    * struct qcom_icc_provider - Qualcomm specific interconnect provider
> > >    * @provider: generic interconnect provider
> > > @@ -811,6 +821,10 @@ static int qnoc_probe(struct platform_device *pdev)
> > >   		qp->bus_clks = devm_kmemdup(dev, bus_mm_clocks,
> > >   					    sizeof(bus_mm_clocks), GFP_KERNEL);
> > >   		qp->num_clks = ARRAY_SIZE(bus_mm_clocks);
> > > +	} else if (of_device_is_compatible(dev->of_node, "qcom,sdm660-a2noc")) {
> > > +		qp->bus_clks = devm_kmemdup(dev, bus_a2noc_clocks,
> > > +					    sizeof(bus_a2noc_clocks), GFP_KERNEL);
> > > +		qp->num_clks = ARRAY_SIZE(bus_a2noc_clocks);
> > >   	} else {
> > >   		if (of_device_is_compatible(dev->of_node, "qcom,sdm660-bimc"))
> > >   			qp->is_bimc_node = true;
> > > -- 
> > > 2.17.1
> > > 
>
diff mbox series

Patch

diff --git a/drivers/interconnect/qcom/sdm660.c b/drivers/interconnect/qcom/sdm660.c
index c89c991a80a0..661eb3635d21 100644
--- a/drivers/interconnect/qcom/sdm660.c
+++ b/drivers/interconnect/qcom/sdm660.c
@@ -174,6 +174,16 @@  static const struct clk_bulk_data bus_mm_clocks[] = {
 	{ .id = "iface" },
 };
 
+static const struct clk_bulk_data bus_a2noc_clocks[] = {
+	{ .id = "bus" },
+	{ .id = "bus_a" },
+	{ .id = "ipa" },
+	{ .id = "ufs_axi" },
+	{ .id = "aggre2_ufs_axi" },
+	{ .id = "aggre2_usb3_axi" },
+	{ .id = "cfg_noc_usb2_axi" },
+};
+
 /**
  * struct qcom_icc_provider - Qualcomm specific interconnect provider
  * @provider: generic interconnect provider
@@ -811,6 +821,10 @@  static int qnoc_probe(struct platform_device *pdev)
 		qp->bus_clks = devm_kmemdup(dev, bus_mm_clocks,
 					    sizeof(bus_mm_clocks), GFP_KERNEL);
 		qp->num_clks = ARRAY_SIZE(bus_mm_clocks);
+	} else if (of_device_is_compatible(dev->of_node, "qcom,sdm660-a2noc")) {
+		qp->bus_clks = devm_kmemdup(dev, bus_a2noc_clocks,
+					    sizeof(bus_a2noc_clocks), GFP_KERNEL);
+		qp->num_clks = ARRAY_SIZE(bus_a2noc_clocks);
 	} else {
 		if (of_device_is_compatible(dev->of_node, "qcom,sdm660-bimc"))
 			qp->is_bimc_node = true;