[PATCH 0/2] Add ACPI support for SC8180X pinctrl driver

[PATCH 0/2] Add ACPI support for SC8180X pinctrl driver

[Shawn Guo]

This is a couple of patches that enable ACPI probe for SC8180X pinctrl
driver.  The msm core driver needs a bit change to handle tiles mapping
differently between DT and ACPI.

Shawn Guo (2):
  pinctrl: qcom: handle tiles for ACPI boot
  pinctrl: qcom: sc8180x: add ACPI probe support

 drivers/pinctrl/qcom/Kconfig           |  2 +-
 drivers/pinctrl/qcom/pinctrl-msm.c     | 18 +++++++---
 drivers/pinctrl/qcom/pinctrl-msm.h     |  1 +
 drivers/pinctrl/qcom/pinctrl-sc8180x.c | 48 +++++++++++++++++++++++++-
 4 files changed, 63 insertions(+), 6 deletions(-)

-- 
2.17.1

[PATCH 1/2] pinctrl: qcom: handle tiles for ACPI boot

[Shawn Guo]

It's not always the case that DT and ACPI describe hardware resource in
the same schema, even for a single platform.  For example, on SC8180X,
DT uses the tiles schema while ACPI describe memory resource as a single
region.  It patches msm_pinctrl_probe() function to map tiles regions
only for DT.  While for ACPI, it maps the single memory resource and
calculate tile bases with offsets passed from SoC data.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/pinctrl/qcom/pinctrl-msm.c | 18 ++++++++++++++----
 drivers/pinctrl/qcom/pinctrl-msm.h |  1 +
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 40256663264f..2526f299bdce 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -4,6 +4,7 @@
  * Copyright (c) 2013, The Linux Foundation. All rights reserved.
  */
 
+#include <linux/acpi.h>
 #include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/io.h>
@@ -1399,6 +1400,7 @@ int msm_pinctrl_probe(struct platform_device *pdev,
 {
 	struct msm_pinctrl *pctrl;
 	struct resource *res;
+	void __iomem *base;
 	int ret;
 	int i;
 
@@ -1415,7 +1417,7 @@ int msm_pinctrl_probe(struct platform_device *pdev,
 
 	raw_spin_lock_init(&pctrl->lock);
 
-	if (soc_data->tiles) {
+	if (soc_data->tiles && !has_acpi_companion(&pdev->dev)) {
 		for (i = 0; i < soc_data->ntiles; i++) {
 			res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
 							   soc_data->tiles[i]);
@@ -1425,9 +1427,17 @@ int msm_pinctrl_probe(struct platform_device *pdev,
 		}
 	} else {
 		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-		pctrl->regs[0] = devm_ioremap_resource(&pdev->dev, res);
-		if (IS_ERR(pctrl->regs[0]))
-			return PTR_ERR(pctrl->regs[0]);
+		base = devm_ioremap_resource(&pdev->dev, res);
+		if (IS_ERR(base))
+			return PTR_ERR(base);
+
+		if (soc_data->tiles) {
+			for (i = 0; i < soc_data->ntiles; i++)
+				pctrl->regs[i] = base +
+						 soc_data->tile_offsets[i];
+		} else {
+			pctrl->regs[0] = base;
+		}
 
 		pctrl->phys_base[0] = res->start;
 	}
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.h b/drivers/pinctrl/qcom/pinctrl-msm.h
index e31a5167c91e..91333942d53c 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.h
+++ b/drivers/pinctrl/qcom/pinctrl-msm.h
@@ -131,6 +131,7 @@ struct msm_pinctrl_soc_data {
 	bool pull_no_keeper;
 	const char *const *tiles;
 	unsigned int ntiles;
+	const u32 *tile_offsets;
 	const int *reserved_gpios;
 	const struct msm_gpio_wakeirq_map *wakeirq_map;
 	unsigned int nwakeirq_map;
-- 
2.17.1

[PATCH 2/2] pinctrl: qcom: sc8180x: add ACPI probe support

[Shawn Guo]

It adds ACPI probe support with tile offsets passed over to msm core
driver via sc8180x_tile_offsets, as TLMM is described a single memory
region in ACPI DSDT.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/pinctrl/qcom/Kconfig           |  2 +-
 drivers/pinctrl/qcom/pinctrl-sc8180x.c | 48 +++++++++++++++++++++++++-
 2 files changed, 48 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/qcom/Kconfig b/drivers/pinctrl/qcom/Kconfig
index 6853a896c476..9f0218c4f9b3 100644
--- a/drivers/pinctrl/qcom/Kconfig
+++ b/drivers/pinctrl/qcom/Kconfig
@@ -222,7 +222,7 @@ config PINCTRL_SC7280
 
 config PINCTRL_SC8180X
 	tristate "Qualcomm Technologies Inc SC8180x pin controller driver"
-	depends on GPIOLIB && OF
+	depends on GPIOLIB && (OF || ACPI)
 	select PINCTRL_MSM
 	help
 	  This is the pinctrl, pinmux, pinconf and gpiolib driver for the
diff --git a/drivers/pinctrl/qcom/pinctrl-sc8180x.c b/drivers/pinctrl/qcom/pinctrl-sc8180x.c
index b765bf667574..38117ceb4d8f 100644
--- a/drivers/pinctrl/qcom/pinctrl-sc8180x.c
+++ b/drivers/pinctrl/qcom/pinctrl-sc8180x.c
@@ -4,6 +4,7 @@
  * Copyright (c) 2020-2021, Linaro Ltd.
  */
 
+#include <linux/acpi.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
@@ -17,6 +18,12 @@ static const char * const sc8180x_tiles[] = {
 	"west"
 };
 
+static const u32 sc8180x_tile_offsets[] = {
+	0x00d00000,
+	0x00500000,
+	0x00100000
+};
+
 enum {
 	SOUTH,
 	EAST,
@@ -1557,6 +1564,13 @@ static const struct msm_pingroup sc8180x_groups[] = {
 	[193] = SDC_QDSD_PINGROUP(sdc2_data, 0x4b2000, 9, 0),
 };
 
+static const int sc8180x_acpi_reserved_gpios[] = {
+	0, 1, 2, 3,
+	47, 48, 49, 50,
+	126, 127, 128, 129,
+	-1
+};
+
 static const struct msm_gpio_wakeirq_map sc8180x_pdc_map[] = {
 	{ 3, 31 }, { 5, 32 }, { 8, 33 }, { 9, 34 }, { 10, 100 }, { 12, 104 },
 	{ 24, 37 }, { 26, 38 }, { 27, 41 }, { 28, 42 }, { 30, 39 }, { 36, 43 },
@@ -1588,11 +1602,42 @@ static struct msm_pinctrl_soc_data sc8180x_pinctrl = {
 	.nwakeirq_map = ARRAY_SIZE(sc8180x_pdc_map),
 };
 
+static const struct msm_pinctrl_soc_data sc8180x_acpi_pinctrl = {
+	.tiles = sc8180x_tiles,
+	.ntiles = ARRAY_SIZE(sc8180x_tiles),
+	.tile_offsets = sc8180x_tile_offsets,
+	.pins = sc8180x_pins,
+	.npins = ARRAY_SIZE(sc8180x_pins),
+	.groups = sc8180x_groups,
+	.ngroups = ARRAY_SIZE(sc8180x_groups),
+	.reserved_gpios = sc8180x_acpi_reserved_gpios,
+	.ngpios = 191,
+};
+
 static int sc8180x_pinctrl_probe(struct platform_device *pdev)
 {
-	return msm_pinctrl_probe(pdev, &sc8180x_pinctrl);
+	int ret;
+
+	if (pdev->dev.of_node) {
+		ret = msm_pinctrl_probe(pdev, &sc8180x_pinctrl);
+	} else if (has_acpi_companion(&pdev->dev)) {
+		ret = msm_pinctrl_probe(pdev, &sc8180x_acpi_pinctrl);
+	} else {
+		dev_err(&pdev->dev, "DT and ACPI disabled\n");
+		ret = -EINVAL;
+	}
+
+	return ret;
 }
 
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id sc8180x_pinctrl_acpi_match[] = {
+	{ "QCOM040D"},
+	{ },
+};
+MODULE_DEVICE_TABLE(acpi, sc8180x_pinctrl_acpi_match);
+#endif
+
 static const struct of_device_id sc8180x_pinctrl_of_match[] = {
 	{ .compatible = "qcom,sc8180x-tlmm", },
 	{ },
@@ -1603,6 +1648,7 @@ static struct platform_driver sc8180x_pinctrl_driver = {
 	.driver = {
 		.name = "sc8180x-pinctrl",
 		.of_match_table = sc8180x_pinctrl_of_match,
+		.acpi_match_table = ACPI_PTR(sc8180x_pinctrl_acpi_match),
 	},
 	.probe = sc8180x_pinctrl_probe,
 	.remove = msm_pinctrl_remove,
-- 
2.17.1

Re: [PATCH 0/2] Add ACPI support for SC8180X pinctrl driver

[Andy Shevchenko]

On Mon, Mar 01, 2021 at 09:43:27AM +0800, Shawn Guo wrote:
> This is a couple of patches that enable ACPI probe for SC8180X pinctrl
> driver.  The msm core driver needs a bit change to handle tiles mapping
> differently between DT and ACPI.

Please, Cc me for this series.

Meanwhile I think we have to understand the numbering scheme used by ACPI
firmware on the machine in question.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] pinctrl: qcom: handle tiles for ACPI boot

[Andy Shevchenko]

On Mon, Mar 01, 2021 at 09:43:28AM +0800, Shawn Guo wrote:
> It's not always the case that DT and ACPI describe hardware resource in
> the same schema, even for a single platform.  For example, on SC8180X,
> DT uses the tiles schema while ACPI describe memory resource as a single
> region.  It patches msm_pinctrl_probe() function to map tiles regions
> only for DT.  While for ACPI, it maps the single memory resource and
> calculate tile bases with offsets passed from SoC data.

...

> +#include <linux/acpi.h>

No use of this header. See below.
(Perhaps you meant mod_devicetable.h)

...

> -	if (soc_data->tiles) {
> +	if (soc_data->tiles && !has_acpi_companion(&pdev->dev)) {

Any documentation to understand this change?

...

> +		if (soc_data->tiles) {
> +			for (i = 0; i < soc_data->ntiles; i++)
> +				pctrl->regs[i] = base +
> +						 soc_data->tile_offsets[i];
> +		} else {
> +			pctrl->regs[0] = base;
> +		}

And so this?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] pinctrl: qcom: sc8180x: add ACPI probe support

[Andy Shevchenko]

On Mon, Mar 01, 2021 at 09:43:29AM +0800, Shawn Guo wrote:
> It adds ACPI probe support with tile offsets passed over to msm core
> driver via sc8180x_tile_offsets, as TLMM is described a single memory
> region in ACPI DSDT.

...

>  config PINCTRL_SC8180X
>  	tristate "Qualcomm Technologies Inc SC8180x pin controller driver"
> -	depends on GPIOLIB && OF
> +	depends on GPIOLIB && (OF || ACPI)

Can you consider dropping OF dependency completely?

> +#include <linux/acpi.h>

No use of this header, see below.

(Perhaps you meant mod_devicetable.h)

...

> +static const u32 sc8180x_tile_offsets[] = {
> +	0x00d00000,
> +	0x00500000,
> +	0x00100000

Leave comma here.

> +};

...

> +static const int sc8180x_acpi_reserved_gpios[] = {
> +	0, 1, 2, 3,
> +	47, 48, 49, 50,
> +	126, 127, 128, 129,

> +	-1

-1?
Is it kinda terminator?

> +};

...

> +	if (pdev->dev.of_node) {
> +		ret = msm_pinctrl_probe(pdev, &sc8180x_pinctrl);
> +	} else if (has_acpi_companion(&pdev->dev)) {
> +		ret = msm_pinctrl_probe(pdev, &sc8180x_acpi_pinctrl);
> +	} else {
> +		dev_err(&pdev->dev, "DT and ACPI disabled\n");
> +		ret = -EINVAL;
> +	}

Use driver_data field for this and device_get_match_data() instead of above.

...

> +#ifdef CONFIG_ACPI

Drop this ugly ifdeffery.

> +static const struct acpi_device_id sc8180x_pinctrl_acpi_match[] = {
> +	{ "QCOM040D"},

> +	{ },

No comma for terminator line.

> +};
> +MODULE_DEVICE_TABLE(acpi, sc8180x_pinctrl_acpi_match);
> +#endif

...

> +		.acpi_match_table = ACPI_PTR(sc8180x_pinctrl_acpi_match),

No ACPI_PTR(), please.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] pinctrl: qcom: handle tiles for ACPI boot

[Shawn Guo]

On Mon, Mar 01, 2021 at 04:34:30PM +0200, Andy Shevchenko wrote:
> On Mon, Mar 01, 2021 at 09:43:28AM +0800, Shawn Guo wrote:
> > It's not always the case that DT and ACPI describe hardware resource in
> > the same schema, even for a single platform.  For example, on SC8180X,
> > DT uses the tiles schema while ACPI describe memory resource as a single
> > region.  It patches msm_pinctrl_probe() function to map tiles regions
> > only for DT.  While for ACPI, it maps the single memory resource and
> > calculate tile bases with offsets passed from SoC data.
> 
> ...
> 
> > +#include <linux/acpi.h>
> 
> No use of this header. See below.
> (Perhaps you meant mod_devicetable.h)

has_acpi_companion() call needs the header.

> 
> ...
> 
> > -	if (soc_data->tiles) {
> > +	if (soc_data->tiles && !has_acpi_companion(&pdev->dev)) {
> 
> Any documentation to understand this change?

Well, !has_acpi_companion() is just to rule out ACPI boot and ensure
this is a DT boot with tiles. 

> 
> ...
> 
> > +		if (soc_data->tiles) {
> > +			for (i = 0; i < soc_data->ntiles; i++)
> > +				pctrl->regs[i] = base +
> > +						 soc_data->tile_offsets[i];
> > +		} else {
> > +			pctrl->regs[0] = base;
> > +		}
> 
> And so this?

For ACPI boot or DT without tiles, there is only one single memory
resource to map.  But for SoC driver like pinctrl-sc8180x that defines
pins with tiles, even with ACPI boot, we need to have multiple regs[]
to hold bases for tiles.

I will add comment to make it easier for understanding.

Shawn

Re: [PATCH 2/2] pinctrl: qcom: sc8180x: add ACPI probe support

[Shawn Guo]

On Mon, Mar 01, 2021 at 04:37:50PM +0200, Andy Shevchenko wrote:
> On Mon, Mar 01, 2021 at 09:43:29AM +0800, Shawn Guo wrote:
> > It adds ACPI probe support with tile offsets passed over to msm core
> > driver via sc8180x_tile_offsets, as TLMM is described a single memory
> > region in ACPI DSDT.
> 
> ...
> 
> >  config PINCTRL_SC8180X
> >  	tristate "Qualcomm Technologies Inc SC8180x pin controller driver"
> > -	depends on GPIOLIB && OF
> > +	depends on GPIOLIB && (OF || ACPI)
> 
> Can you consider dropping OF dependency completely?

Not sure.  Looking at those driver options in drivers/pinctrl/qcom/Kconfig,
I think it's a global thing, and should be addressed separately anyway.

> 
> > +#include <linux/acpi.h>
> 
> No use of this header, see below.

has_acpi_companion() and ACPI_PTR use it.

> 
> (Perhaps you meant mod_devicetable.h)
> 
> ...
> 
> > +static const u32 sc8180x_tile_offsets[] = {
> > +	0x00d00000,
> > +	0x00500000,
> > +	0x00100000
> 
> Leave comma here.

Well, this is to respect the taste of original author of the driver, if
you take a look at sc8180x_tiles[] above and enum after.

> 
> > +};
> 
> ...
> 
> > +static const int sc8180x_acpi_reserved_gpios[] = {
> > +	0, 1, 2, 3,
> > +	47, 48, 49, 50,
> > +	126, 127, 128, 129,
> 
> > +	-1
> 
> -1?
> Is it kinda terminator?

Yes, it is.  I will add a comment there.

> 
> > +};
> 
> ...
> 
> > +	if (pdev->dev.of_node) {
> > +		ret = msm_pinctrl_probe(pdev, &sc8180x_pinctrl);
> > +	} else if (has_acpi_companion(&pdev->dev)) {
> > +		ret = msm_pinctrl_probe(pdev, &sc8180x_acpi_pinctrl);
> > +	} else {
> > +		dev_err(&pdev->dev, "DT and ACPI disabled\n");
> > +		ret = -EINVAL;
> > +	}
> 
> Use driver_data field for this and device_get_match_data() instead of above.

Good suggestion, thanks!

> 
> ...
> 
> > +#ifdef CONFIG_ACPI
> 
> Drop this ugly ifdeffery.
> 
> > +static const struct acpi_device_id sc8180x_pinctrl_acpi_match[] = {
> > +	{ "QCOM040D"},
> 
> > +	{ },
> 
> No comma for terminator line.
> 
> > +};
> > +MODULE_DEVICE_TABLE(acpi, sc8180x_pinctrl_acpi_match);
> > +#endif
> 
> ...
> 
> > +		.acpi_match_table = ACPI_PTR(sc8180x_pinctrl_acpi_match),
> 
> No ACPI_PTR(), please.

Sounds good.

Shawn