diff mbox series

[v3,10/11] memory: tegra: handle no BWMGR MRQ support in BPMP

Message ID 20230320182441.11904-11-sumitg@nvidia.com
State New
Headers
Series Tegra234 Memory interconnect support |

Commit Message

Sumit Gupta March 20, 2023, 6:24 p.m. UTC 
  If BPMP-FW doesn't support 'MRQ_BWMGR_INT', then the MC Client driver
probe fails with 'EPROBE_DEFER' which it receives on calling the func
'devm_of_icc_get()'. Fix this by initializing the ICC even if the MRQ
is missing and return 'EINVAL' from 'icc_set_bw()' instead of passing
the request to BPMP-FW later when the BW request is made by client.

Fixes: ("memory: tegra: add interconnect support for DRAM scaling in Tegra234")
Signed-off-by: Sumit Gupta <sumitg@nvidia.com>
---
 drivers/memory/tegra/tegra186-emc.c | 13 +++++++------
 drivers/memory/tegra/tegra234.c     |  6 ++++++
 include/soc/tegra/mc.h              |  2 +-
 3 files changed, 14 insertions(+), 7 deletions(-)

Comments

Krzysztof Kozlowski March 22, 2023, 5:50 p.m. UTC | #1 
On 20/03/2023 19:24, Sumit Gupta wrote:
> If BPMP-FW doesn't support 'MRQ_BWMGR_INT', then the MC Client driver
> probe fails with 'EPROBE_DEFER' which it receives on calling the func
> 'devm_of_icc_get()'. Fix this by initializing the ICC even if the MRQ
> is missing and return 'EINVAL' from 'icc_set_bw()' instead of passing
> the request to BPMP-FW later when the BW request is made by client.
> 
> Fixes: ("memory: tegra: add interconnect support for DRAM scaling in Tegra234")

That's not correct tag.

Anyway, send fixes separately.

Best regards,
Krzysztof
Thierry Reding March 23, 2023, 9:55 a.m. UTC | #2 
On Wed, Mar 22, 2023 at 06:50:23PM +0100, Krzysztof Kozlowski wrote:
> On 20/03/2023 19:24, Sumit Gupta wrote:
> > If BPMP-FW doesn't support 'MRQ_BWMGR_INT', then the MC Client driver
> > probe fails with 'EPROBE_DEFER' which it receives on calling the func
> > 'devm_of_icc_get()'. Fix this by initializing the ICC even if the MRQ
> > is missing and return 'EINVAL' from 'icc_set_bw()' instead of passing
> > the request to BPMP-FW later when the BW request is made by client.
> > 
> > Fixes: ("memory: tegra: add interconnect support for DRAM scaling in Tegra234")
> 
> That's not correct tag.
> 
> Anyway, send fixes separately.

I think this was a bit confusing. This fixes an issue that was
introduced in a patch earlier in this series, so it's probably better to
squash it into that patch rather than have a separate fix patch in the
same series.

Thierry
Krzysztof Kozlowski March 23, 2023, 9:58 a.m. UTC | #3 
On 23/03/2023 10:55, Thierry Reding wrote:
> On Wed, Mar 22, 2023 at 06:50:23PM +0100, Krzysztof Kozlowski wrote:
>> On 20/03/2023 19:24, Sumit Gupta wrote:
>>> If BPMP-FW doesn't support 'MRQ_BWMGR_INT', then the MC Client driver
>>> probe fails with 'EPROBE_DEFER' which it receives on calling the func
>>> 'devm_of_icc_get()'. Fix this by initializing the ICC even if the MRQ
>>> is missing and return 'EINVAL' from 'icc_set_bw()' instead of passing
>>> the request to BPMP-FW later when the BW request is made by client.
>>>
>>> Fixes: ("memory: tegra: add interconnect support for DRAM scaling in Tegra234")
>>
>> That's not correct tag.
>>
>> Anyway, send fixes separately.
> 
> I think this was a bit confusing. This fixes an issue that was
> introduced in a patch earlier in this series, so it's probably better to
> squash it into that patch rather than have a separate fix patch in the
> same series.
> 

Yeah, it is quite confusing to send buggy code and immediately fix it...
Introducing known bugs is actually non-bisectable and harming.

Best regards,
Krzysztof
Thierry Reding March 23, 2023, 10:02 a.m. UTC | #4 
On Thu, Mar 23, 2023 at 10:58:18AM +0100, Krzysztof Kozlowski wrote:
> On 23/03/2023 10:55, Thierry Reding wrote:
> > On Wed, Mar 22, 2023 at 06:50:23PM +0100, Krzysztof Kozlowski wrote:
> >> On 20/03/2023 19:24, Sumit Gupta wrote:
> >>> If BPMP-FW doesn't support 'MRQ_BWMGR_INT', then the MC Client driver
> >>> probe fails with 'EPROBE_DEFER' which it receives on calling the func
> >>> 'devm_of_icc_get()'. Fix this by initializing the ICC even if the MRQ
> >>> is missing and return 'EINVAL' from 'icc_set_bw()' instead of passing
> >>> the request to BPMP-FW later when the BW request is made by client.
> >>>
> >>> Fixes: ("memory: tegra: add interconnect support for DRAM scaling in Tegra234")
> >>
> >> That's not correct tag.
> >>
> >> Anyway, send fixes separately.
> > 
> > I think this was a bit confusing. This fixes an issue that was
> > introduced in a patch earlier in this series, so it's probably better to
> > squash it into that patch rather than have a separate fix patch in the
> > same series.
> > 
> 
> Yeah, it is quite confusing to send buggy code and immediately fix it...
> Introducing known bugs is actually non-bisectable and harming.

I don't think this was done purposefully but rather as a way of more
clearly showing what was changed. This is also an issue that can only
happen on certain boards, so it's easy to miss.

But yeah, this is bad for bisectibility.

Sumit, please merge this into the patch that you reference in the Fixes:
tag along with the fix in patch 11.

Thierry
Sumit Gupta March 23, 2023, 12:46 p.m. UTC | #5 
On 23/03/23 15:32, Thierry Reding wrote:
> On Thu, Mar 23, 2023 at 10:58:18AM +0100, Krzysztof Kozlowski wrote:
>> On 23/03/2023 10:55, Thierry Reding wrote:
>>> On Wed, Mar 22, 2023 at 06:50:23PM +0100, Krzysztof Kozlowski wrote:
>>>> On 20/03/2023 19:24, Sumit Gupta wrote:
>>>>> If BPMP-FW doesn't support 'MRQ_BWMGR_INT', then the MC Client driver
>>>>> probe fails with 'EPROBE_DEFER' which it receives on calling the func
>>>>> 'devm_of_icc_get()'. Fix this by initializing the ICC even if the MRQ
>>>>> is missing and return 'EINVAL' from 'icc_set_bw()' instead of passing
>>>>> the request to BPMP-FW later when the BW request is made by client.
>>>>>
>>>>> Fixes: ("memory: tegra: add interconnect support for DRAM scaling in Tegra234")
>>>>
>>>> That's not correct tag.
>>>>
>>>> Anyway, send fixes separately.
>>>
>>> I think this was a bit confusing. This fixes an issue that was
>>> introduced in a patch earlier in this series, so it's probably better to
>>> squash it into that patch rather than have a separate fix patch in the
>>> same series.
>>>
>>
>> Yeah, it is quite confusing to send buggy code and immediately fix it...
>> Introducing known bugs is actually non-bisectable and harming.
> 
> I don't think this was done purposefully but rather as a way of more
> clearly showing what was changed. This is also an issue that can only
> happen on certain boards, so it's easy to miss.
> 
> But yeah, this is bad for bisectibility.
> 
> Sumit, please merge this into the patch that you reference in the Fixes:
> tag along with the fix in patch 11.
> 
> Thierry

Sure, I will merge this change in parent patch and send v4.

Thanks,
Sumit
diff mbox series

Patch

diff --git a/drivers/memory/tegra/tegra186-emc.c b/drivers/memory/tegra/tegra186-emc.c
index 978be8f54de4..0d68a20fd376 100644
--- a/drivers/memory/tegra/tegra186-emc.c
+++ b/drivers/memory/tegra/tegra186-emc.c
@@ -343,13 +343,14 @@  static int tegra186_emc_probe(struct platform_device *pdev)
 
 	mc = dev_get_drvdata(emc->dev->parent);
 	if (mc && mc->soc->icc_ops) {
-		if (tegra_bpmp_mrq_is_supported(emc->bpmp, MRQ_BWMGR_INT)) {
-			err = tegra_emc_interconnect_init(emc);
-			if (err)
-				goto put_bpmp;
-		} else {
+		err = tegra_emc_interconnect_init(emc);
+		if (err)
+			goto put_bpmp;
+
+		if (tegra_bpmp_mrq_is_supported(emc->bpmp, MRQ_BWMGR_INT))
+			mc->bwmgr_mrq_supported = true;
+		else
 			dev_info(&pdev->dev, "MRQ_BWMGR_INT not present\n");
-		}
 	}
 
 	return 0;
diff --git a/drivers/memory/tegra/tegra234.c b/drivers/memory/tegra/tegra234.c
index 0b608b820b28..5b4b359f7c71 100644
--- a/drivers/memory/tegra/tegra234.c
+++ b/drivers/memory/tegra/tegra234.c
@@ -828,6 +828,9 @@  static int tegra234_mc_icc_set(struct icc_node *src, struct icc_node *dst)
 	if (src->id == dst->id)
 		return 0;
 
+	if (!mc->bwmgr_mrq_supported)
+		return -EINVAL;
+
 	bpmp = of_tegra_bpmp_get();
 	if (IS_ERR(bpmp)) {
 		ret = PTR_ERR(bpmp);
@@ -874,6 +877,9 @@  static int tegra234_mc_icc_aggregate(struct icc_node *node, u32 tag, u32 avg_bw,
 	struct icc_provider *p = node->provider;
 	struct tegra_mc *mc = icc_provider_to_tegra_mc(p);
 
+	if (!mc->bwmgr_mrq_supported)
+		return -EINVAL;
+
 	if (node->id == TEGRA_ICC_MC_CPU_CLUSTER0 ||
 	    node->id == TEGRA_ICC_MC_CPU_CLUSTER1 ||
 	    node->id == TEGRA_ICC_MC_CPU_CLUSTER2) {
diff --git a/include/soc/tegra/mc.h b/include/soc/tegra/mc.h
index 2fe6f0217a39..522b7679500c 100644
--- a/include/soc/tegra/mc.h
+++ b/include/soc/tegra/mc.h
@@ -234,7 +234,7 @@  struct tegra_mc {
 	struct tegra_mc_timing *timings;
 	unsigned int num_timings;
 	unsigned int num_channels;
-
+	bool bwmgr_mrq_supported;
 	struct reset_controller_dev reset;
 
 	struct icc_provider provider;