[PATCH 0/2] clk: qcom: gpucc-*: stop requesting register resource

[PATCH 0/2] clk: qcom: gpucc-*: stop requesting register resource

[Dmitry Baryshkov]

Testing of [1] pointed out that on all modern Qualcomm platforms GPU
clock controller shares regiser space with the GMU. All gpucc drivers
use (internally) devm_platform_ioremap_resource(), preventing the GPU
driver from using devm_ioremap_resource() on its own. As GMU register
space includes gpucc's one, make gpucc drivers use non-requesting
helper, allowing GPU to take over the bigger memory region.

[1] https://patchwork.freedesktop.org/series/134401/

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
Dmitry Baryshkov (2):
      clk: qocm: add qcom_cc_map_norequest
      clk: qcom: gpucc-*: use qcom_cc_map_norequest

 drivers/clk/qcom/common.c         | 20 ++++++++++++++++++++
 drivers/clk/qcom/common.h         |  2 ++
 drivers/clk/qcom/gpucc-qcm2290.c  |  2 +-
 drivers/clk/qcom/gpucc-sa8775p.c  |  2 +-
 drivers/clk/qcom/gpucc-sc7180.c   |  2 +-
 drivers/clk/qcom/gpucc-sc7280.c   |  2 +-
 drivers/clk/qcom/gpucc-sc8280xp.c |  2 +-
 drivers/clk/qcom/gpucc-sdm845.c   |  2 +-
 drivers/clk/qcom/gpucc-sm6115.c   |  2 +-
 drivers/clk/qcom/gpucc-sm6125.c   |  2 +-
 drivers/clk/qcom/gpucc-sm6350.c   |  2 +-
 drivers/clk/qcom/gpucc-sm6375.c   |  2 +-
 drivers/clk/qcom/gpucc-sm8150.c   |  2 +-
 drivers/clk/qcom/gpucc-sm8250.c   |  2 +-
 drivers/clk/qcom/gpucc-sm8350.c   |  2 +-
 drivers/clk/qcom/gpucc-sm8450.c   |  2 +-
 drivers/clk/qcom/gpucc-sm8550.c   |  2 +-
 drivers/clk/qcom/gpucc-sm8650.c   |  2 +-
 drivers/clk/qcom/gpucc-x1e80100.c |  2 +-
 19 files changed, 39 insertions(+), 17 deletions(-)
---
base-commit: 5d98d5e70f505b7278336de493eba94cde5526b3
change-id: 20240627-gpucc-no-request-cb6b5f72e8da

Best regards,
-- 
Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

[PATCH 1/2] clk: qocm: add qcom_cc_map_norequest

[Dmitry Baryshkov]

The GPU clock controllers use memory region that is a part of the GMU's
memory region. Add qcom_cc_map_norequest() to be used by GPUCC, so that
GPU driver can use devm_ioremap_resource for GMU resources.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/clk/qcom/common.c | 20 ++++++++++++++++++++
 drivers/clk/qcom/common.h |  2 ++
 2 files changed, 22 insertions(+)

diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
index c92e10c60322..dcc73bc22606 100644
--- a/drivers/clk/qcom/common.c
+++ b/drivers/clk/qcom/common.c
@@ -113,6 +113,26 @@ qcom_cc_map(struct platform_device *pdev, const struct qcom_cc_desc *desc)
 }
 EXPORT_SYMBOL_GPL(qcom_cc_map);
 
+/* gpucc shares memory region with GMU, don't request the region */
+struct regmap *
+qcom_cc_map_norequest(struct platform_device *pdev, const struct qcom_cc_desc *desc)
+{
+	struct device *dev = &pdev->dev;
+	struct resource *r;
+	void __iomem *base;
+
+	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!r)
+		return ERR_PTR(dev_err_probe(dev, -EINVAL, "resource not found\n"));
+
+	base = devm_ioremap(dev, r->start, resource_size(r));
+	if (IS_ERR(base))
+		return ERR_CAST(base);
+
+	return devm_regmap_init_mmio(dev, base, desc->config);
+}
+EXPORT_SYMBOL_GPL(qcom_cc_map_norequest);
+
 void
 qcom_pll_set_fsm_mode(struct regmap *map, u32 reg, u8 bias_count, u8 lock_count)
 {
diff --git a/drivers/clk/qcom/common.h b/drivers/clk/qcom/common.h
index d048bdeeba10..6cab7805a92c 100644
--- a/drivers/clk/qcom/common.h
+++ b/drivers/clk/qcom/common.h
@@ -60,6 +60,8 @@ extern int qcom_cc_register_sleep_clk(struct device *dev);
 
 extern struct regmap *qcom_cc_map(struct platform_device *pdev,
 				  const struct qcom_cc_desc *desc);
+extern struct regmap *qcom_cc_map_norequest(struct platform_device *pdev,
+					    const struct qcom_cc_desc *desc);
 extern int qcom_cc_really_probe(struct device *dev,
 				const struct qcom_cc_desc *desc,
 				struct regmap *regmap);

-- 
2.39.2

[PATCH 2/2] clk: qcom: gpucc-*: use qcom_cc_map_norequest

[Dmitry Baryshkov]

On most of the Qualcomm platforms GPU clock controller registers are
located inside the GMU's register space. By using qcom_cc_map() gpucc
drivers mark the region as used, prevening GMU driver from claiming the
bigger region.

Make affected GPU clock controller drivers use qcom_cc_map_norequest(),
allowing GMU driver to use devm_ioremap_resource().

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/clk/qcom/gpucc-qcm2290.c  | 2 +-
 drivers/clk/qcom/gpucc-sa8775p.c  | 2 +-
 drivers/clk/qcom/gpucc-sc7180.c   | 2 +-
 drivers/clk/qcom/gpucc-sc7280.c   | 2 +-
 drivers/clk/qcom/gpucc-sc8280xp.c | 2 +-
 drivers/clk/qcom/gpucc-sdm845.c   | 2 +-
 drivers/clk/qcom/gpucc-sm6115.c   | 2 +-
 drivers/clk/qcom/gpucc-sm6125.c   | 2 +-
 drivers/clk/qcom/gpucc-sm6350.c   | 2 +-
 drivers/clk/qcom/gpucc-sm6375.c   | 2 +-
 drivers/clk/qcom/gpucc-sm8150.c   | 2 +-
 drivers/clk/qcom/gpucc-sm8250.c   | 2 +-
 drivers/clk/qcom/gpucc-sm8350.c   | 2 +-
 drivers/clk/qcom/gpucc-sm8450.c   | 2 +-
 drivers/clk/qcom/gpucc-sm8550.c   | 2 +-
 drivers/clk/qcom/gpucc-sm8650.c   | 2 +-
 drivers/clk/qcom/gpucc-x1e80100.c | 2 +-
 17 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/clk/qcom/gpucc-qcm2290.c b/drivers/clk/qcom/gpucc-qcm2290.c
index dc369dff882e..2a886b3d6ab4 100644
--- a/drivers/clk/qcom/gpucc-qcm2290.c
+++ b/drivers/clk/qcom/gpucc-qcm2290.c
@@ -372,7 +372,7 @@ static int gpu_cc_qcm2290_probe(struct platform_device *pdev)
 	struct regmap *regmap;
 	int ret;
 
-	regmap = qcom_cc_map(pdev, &gpu_cc_qcm2290_desc);
+	regmap = qcom_cc_map_norequest(pdev, &gpu_cc_qcm2290_desc);
 	if (IS_ERR(regmap))
 		return PTR_ERR(regmap);
 
diff --git a/drivers/clk/qcom/gpucc-sa8775p.c b/drivers/clk/qcom/gpucc-sa8775p.c
index f8a8ac343d70..312b45e6fc29 100644
--- a/drivers/clk/qcom/gpucc-sa8775p.c
+++ b/drivers/clk/qcom/gpucc-sa8775p.c
@@ -592,7 +592,7 @@ static int gpu_cc_sa8775p_probe(struct platform_device *pdev)
 {
 	struct regmap *regmap;
 
-	regmap = qcom_cc_map(pdev, &gpu_cc_sa8775p_desc);
+	regmap = qcom_cc_map_norequest(pdev, &gpu_cc_sa8775p_desc);
 	if (IS_ERR(regmap))
 		return PTR_ERR(regmap);
 
diff --git a/drivers/clk/qcom/gpucc-sc7180.c b/drivers/clk/qcom/gpucc-sc7180.c
index 08f3983d016f..03480a2fa78c 100644
--- a/drivers/clk/qcom/gpucc-sc7180.c
+++ b/drivers/clk/qcom/gpucc-sc7180.c
@@ -220,7 +220,7 @@ static int gpu_cc_sc7180_probe(struct platform_device *pdev)
 	struct alpha_pll_config gpu_cc_pll_config = {};
 	unsigned int value, mask;
 
-	regmap = qcom_cc_map(pdev, &gpu_cc_sc7180_desc);
+	regmap = qcom_cc_map_norequest(pdev, &gpu_cc_sc7180_desc);
 	if (IS_ERR(regmap))
 		return PTR_ERR(regmap);
 
diff --git a/drivers/clk/qcom/gpucc-sc7280.c b/drivers/clk/qcom/gpucc-sc7280.c
index bd699a624517..86f89fbb4aec 100644
--- a/drivers/clk/qcom/gpucc-sc7280.c
+++ b/drivers/clk/qcom/gpucc-sc7280.c
@@ -458,7 +458,7 @@ static int gpu_cc_sc7280_probe(struct platform_device *pdev)
 {
 	struct regmap *regmap;
 
-	regmap = qcom_cc_map(pdev, &gpu_cc_sc7280_desc);
+	regmap = qcom_cc_map_norequest(pdev, &gpu_cc_sc7280_desc);
 	if (IS_ERR(regmap))
 		return PTR_ERR(regmap);
 
diff --git a/drivers/clk/qcom/gpucc-sc8280xp.c b/drivers/clk/qcom/gpucc-sc8280xp.c
index c96be61e3f47..519940dc99eb 100644
--- a/drivers/clk/qcom/gpucc-sc8280xp.c
+++ b/drivers/clk/qcom/gpucc-sc8280xp.c
@@ -436,7 +436,7 @@ static int gpu_cc_sc8280xp_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	regmap = qcom_cc_map(pdev, &gpu_cc_sc8280xp_desc);
+	regmap = qcom_cc_map_norequest(pdev, &gpu_cc_sc8280xp_desc);
 	if (IS_ERR(regmap)) {
 		pm_runtime_put(&pdev->dev);
 		return PTR_ERR(regmap);
diff --git a/drivers/clk/qcom/gpucc-sdm845.c b/drivers/clk/qcom/gpucc-sdm845.c
index ef26690cf504..b78f8b632601 100644
--- a/drivers/clk/qcom/gpucc-sdm845.c
+++ b/drivers/clk/qcom/gpucc-sdm845.c
@@ -177,7 +177,7 @@ static int gpu_cc_sdm845_probe(struct platform_device *pdev)
 	struct regmap *regmap;
 	unsigned int value, mask;
 
-	regmap = qcom_cc_map(pdev, &gpu_cc_sdm845_desc);
+	regmap = qcom_cc_map_norequest(pdev, &gpu_cc_sdm845_desc);
 	if (IS_ERR(regmap))
 		return PTR_ERR(regmap);
 
diff --git a/drivers/clk/qcom/gpucc-sm6115.c b/drivers/clk/qcom/gpucc-sm6115.c
index d43c86cf73a5..ab3e33fbe401 100644
--- a/drivers/clk/qcom/gpucc-sm6115.c
+++ b/drivers/clk/qcom/gpucc-sm6115.c
@@ -474,7 +474,7 @@ static int gpu_cc_sm6115_probe(struct platform_device *pdev)
 {
 	struct regmap *regmap;
 
-	regmap = qcom_cc_map(pdev, &gpu_cc_sm6115_desc);
+	regmap = qcom_cc_map_norequest(pdev, &gpu_cc_sm6115_desc);
 	if (IS_ERR(regmap))
 		return PTR_ERR(regmap);
 
diff --git a/drivers/clk/qcom/gpucc-sm6125.c b/drivers/clk/qcom/gpucc-sm6125.c
index ed6a6e505801..14dc75b3771a 100644
--- a/drivers/clk/qcom/gpucc-sm6125.c
+++ b/drivers/clk/qcom/gpucc-sm6125.c
@@ -395,7 +395,7 @@ static int gpu_cc_sm6125_probe(struct platform_device *pdev)
 {
 	struct regmap *regmap;
 
-	regmap = qcom_cc_map(pdev, &gpu_cc_sm6125_desc);
+	regmap = qcom_cc_map_norequest(pdev, &gpu_cc_sm6125_desc);
 	if (IS_ERR(regmap))
 		return PTR_ERR(regmap);
 
diff --git a/drivers/clk/qcom/gpucc-sm6350.c b/drivers/clk/qcom/gpucc-sm6350.c
index 1e12ad8948db..f0a6a6fb693f 100644
--- a/drivers/clk/qcom/gpucc-sm6350.c
+++ b/drivers/clk/qcom/gpucc-sm6350.c
@@ -489,7 +489,7 @@ static int gpu_cc_sm6350_probe(struct platform_device *pdev)
 	struct regmap *regmap;
 	unsigned int value, mask;
 
-	regmap = qcom_cc_map(pdev, &gpu_cc_sm6350_desc);
+	regmap = qcom_cc_map_norequest(pdev, &gpu_cc_sm6350_desc);
 	if (IS_ERR(regmap))
 		return PTR_ERR(regmap);
 
diff --git a/drivers/clk/qcom/gpucc-sm6375.c b/drivers/clk/qcom/gpucc-sm6375.c
index 41f59024143e..4ec7399f8fc4 100644
--- a/drivers/clk/qcom/gpucc-sm6375.c
+++ b/drivers/clk/qcom/gpucc-sm6375.c
@@ -446,7 +446,7 @@ static int gpucc_sm6375_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	regmap = qcom_cc_map(pdev, &gpucc_sm6375_desc);
+	regmap = qcom_cc_map_norequest(pdev, &gpucc_sm6375_desc);
 	if (IS_ERR(regmap)) {
 		pm_runtime_put(&pdev->dev);
 		return PTR_ERR(regmap);
diff --git a/drivers/clk/qcom/gpucc-sm8150.c b/drivers/clk/qcom/gpucc-sm8150.c
index d711464a71b6..b01531ca13d9 100644
--- a/drivers/clk/qcom/gpucc-sm8150.c
+++ b/drivers/clk/qcom/gpucc-sm8150.c
@@ -295,7 +295,7 @@ static int gpu_cc_sm8150_probe(struct platform_device *pdev)
 {
 	struct regmap *regmap;
 
-	regmap = qcom_cc_map(pdev, &gpu_cc_sm8150_desc);
+	regmap = qcom_cc_map_norequest(pdev, &gpu_cc_sm8150_desc);
 	if (IS_ERR(regmap))
 		return PTR_ERR(regmap);
 
diff --git a/drivers/clk/qcom/gpucc-sm8250.c b/drivers/clk/qcom/gpucc-sm8250.c
index 113b486a6d2f..ded2faff96ce 100644
--- a/drivers/clk/qcom/gpucc-sm8250.c
+++ b/drivers/clk/qcom/gpucc-sm8250.c
@@ -305,7 +305,7 @@ static int gpu_cc_sm8250_probe(struct platform_device *pdev)
 	struct regmap *regmap;
 	unsigned int value, mask;
 
-	regmap = qcom_cc_map(pdev, &gpu_cc_sm8250_desc);
+	regmap = qcom_cc_map_norequest_norequest(pdev, &gpu_cc_sm8250_desc);
 	if (IS_ERR(regmap))
 		return PTR_ERR(regmap);
 
diff --git a/drivers/clk/qcom/gpucc-sm8350.c b/drivers/clk/qcom/gpucc-sm8350.c
index f3b6bdc24485..c11ba4c5f254 100644
--- a/drivers/clk/qcom/gpucc-sm8350.c
+++ b/drivers/clk/qcom/gpucc-sm8350.c
@@ -596,7 +596,7 @@ static int gpu_cc_sm8350_probe(struct platform_device *pdev)
 {
 	struct regmap *regmap;
 
-	regmap = qcom_cc_map(pdev, &gpu_cc_sm8350_desc);
+	regmap = qcom_cc_map_norequest(pdev, &gpu_cc_sm8350_desc);
 	if (IS_ERR(regmap)) {
 		dev_err(&pdev->dev, "Failed to map gpu cc registers\n");
 		return PTR_ERR(regmap);
diff --git a/drivers/clk/qcom/gpucc-sm8450.c b/drivers/clk/qcom/gpucc-sm8450.c
index b3c5d6923cd2..34c709baeefa 100644
--- a/drivers/clk/qcom/gpucc-sm8450.c
+++ b/drivers/clk/qcom/gpucc-sm8450.c
@@ -744,7 +744,7 @@ static int gpu_cc_sm8450_probe(struct platform_device *pdev)
 {
 	struct regmap *regmap;
 
-	regmap = qcom_cc_map(pdev, &gpu_cc_sm8450_desc);
+	regmap = qcom_cc_map_norequest(pdev, &gpu_cc_sm8450_desc);
 	if (IS_ERR(regmap))
 		return PTR_ERR(regmap);
 
diff --git a/drivers/clk/qcom/gpucc-sm8550.c b/drivers/clk/qcom/gpucc-sm8550.c
index 7486edf56160..e77c287604e6 100644
--- a/drivers/clk/qcom/gpucc-sm8550.c
+++ b/drivers/clk/qcom/gpucc-sm8550.c
@@ -568,7 +568,7 @@ static int gpu_cc_sm8550_probe(struct platform_device *pdev)
 {
 	struct regmap *regmap;
 
-	regmap = qcom_cc_map(pdev, &gpu_cc_sm8550_desc);
+	regmap = qcom_cc_map_norequest(pdev, &gpu_cc_sm8550_desc);
 	if (IS_ERR(regmap))
 		return PTR_ERR(regmap);
 
diff --git a/drivers/clk/qcom/gpucc-sm8650.c b/drivers/clk/qcom/gpucc-sm8650.c
index f15aeecc512d..f7370ec3bac2 100644
--- a/drivers/clk/qcom/gpucc-sm8650.c
+++ b/drivers/clk/qcom/gpucc-sm8650.c
@@ -640,7 +640,7 @@ static int gpu_cc_sm8650_probe(struct platform_device *pdev)
 {
 	struct regmap *regmap;
 
-	regmap = qcom_cc_map(pdev, &gpu_cc_sm8650_desc);
+	regmap = qcom_cc_map_norequest(pdev, &gpu_cc_sm8650_desc);
 	if (IS_ERR(regmap))
 		return PTR_ERR(regmap);
 
diff --git a/drivers/clk/qcom/gpucc-x1e80100.c b/drivers/clk/qcom/gpucc-x1e80100.c
index 2eec20dd0254..e583a4a96629 100644
--- a/drivers/clk/qcom/gpucc-x1e80100.c
+++ b/drivers/clk/qcom/gpucc-x1e80100.c
@@ -630,7 +630,7 @@ static int gpu_cc_x1e80100_probe(struct platform_device *pdev)
 {
 	struct regmap *regmap;
 
-	regmap = qcom_cc_map(pdev, &gpu_cc_x1e80100_desc);
+	regmap = qcom_cc_map_norequest(pdev, &gpu_cc_x1e80100_desc);
 	if (IS_ERR(regmap))
 		return PTR_ERR(regmap);
 

-- 
2.39.2

Re: [PATCH 1/2] clk: qocm: add qcom_cc_map_norequest

[Stephen Boyd]

Quoting Dmitry Baryshkov (2024-06-27 22:20:22)
> The GPU clock controllers use memory region that is a part of the GMU's
> memory region. Add qcom_cc_map_norequest() to be used by GPUCC, so that
> GPU driver can use devm_ioremap_resource for GMU resources.

Why does GMU map the gpu clk controller? Does it use those registers? We
don't want to allow two different drivers to map the same region because
then they don't coordinate and write over things.

Re: [PATCH 1/2] clk: qocm: add qcom_cc_map_norequest

[Dmitry Baryshkov]

On Tue, 9 Jul 2024 at 01:30, Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Dmitry Baryshkov (2024-06-27 22:20:22)
> > The GPU clock controllers use memory region that is a part of the GMU's
> > memory region. Add qcom_cc_map_norequest() to be used by GPUCC, so that
> > GPU driver can use devm_ioremap_resource for GMU resources.
>
> Why does GMU map the gpu clk controller? Does it use those registers? We
> don't want to allow two different drivers to map the same region because
> then they don't coordinate and write over things.

It's not that GMU maps gpu CC separately. It looks more like gpucc is
a part of the GMU address space. I think GMU manages some of the
clocks or GDSCs directly.

-- 
With best wishes
Dmitry

Re: [PATCH 1/2] clk: qocm: add qcom_cc_map_norequest

[Stephen Boyd]

Quoting Dmitry Baryshkov (2024-07-10 16:32:18)
> On Tue, 9 Jul 2024 at 01:30, Stephen Boyd <sboyd@kernel.org> wrote:
> >
> > Quoting Dmitry Baryshkov (2024-06-27 22:20:22)
> > > The GPU clock controllers use memory region that is a part of the GMU's
> > > memory region. Add qcom_cc_map_norequest() to be used by GPUCC, so that
> > > GPU driver can use devm_ioremap_resource for GMU resources.
> >
> > Why does GMU map the gpu clk controller? Does it use those registers? We
> > don't want to allow two different drivers to map the same region because
> > then they don't coordinate and write over things.
> 
> It's not that GMU maps gpu CC separately. It looks more like gpucc is
> a part of the GMU address space. I think GMU manages some of the
> clocks or GDSCs directly.
> 

I imagine GMU is a collection of stuff, so the register range is large
because it's basically a subsystem unto itself. Can the range in DT be
split up, or changed so that different devices within GMU are split out?
Or maybe the gpu clk controller can be made into a child of some GMU
node, where the GMU node has a driver that populates devices that match
drivers in different subsystems.

Re: [PATCH 1/2] clk: qocm: add qcom_cc_map_norequest

[Dmitry Baryshkov]

On Thu, 11 Jul 2024 at 03:04, Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Dmitry Baryshkov (2024-07-10 16:32:18)
> > On Tue, 9 Jul 2024 at 01:30, Stephen Boyd <sboyd@kernel.org> wrote:
> > >
> > > Quoting Dmitry Baryshkov (2024-06-27 22:20:22)
> > > > The GPU clock controllers use memory region that is a part of the GMU's
> > > > memory region. Add qcom_cc_map_norequest() to be used by GPUCC, so that
> > > > GPU driver can use devm_ioremap_resource for GMU resources.
> > >
> > > Why does GMU map the gpu clk controller? Does it use those registers? We
> > > don't want to allow two different drivers to map the same region because
> > > then they don't coordinate and write over things.
> >
> > It's not that GMU maps gpu CC separately. It looks more like gpucc is
> > a part of the GMU address space. I think GMU manages some of the
> > clocks or GDSCs directly.
> >
>
> I imagine GMU is a collection of stuff, so the register range is large
> because it's basically a subsystem unto itself. Can the range in DT be
> split up, or changed so that different devices within GMU are split out?

No, we have to remain compatible with existing DT. It's not a problem
of a single new platform, the issue has always been present there.

> Or maybe the gpu clk controller can be made into a child of some GMU
> node, where the GMU node has a driver that populates devices that match
> drivers in different subsystems.

Well... Technically yes, but this brings another pack of issues. There
is no separate GMU driver, so we will likely have a chicken-and-egg
problem, as probing of the GPU driver will also create the gpucc
device which is further used by the GPU.

-- 
With best wishes
Dmitry