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

[PATCH v2 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.

Changes for v2:
- Pass soc_data pointer via .driver_data.
- Drop use of CONFIG_ACPI and ACPI_PTR().
- Add comment for sc8180x_acpi_reserved_gpios[] terminator.
- Add comments for tiles handling.

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     | 27 +++++++++++---
 drivers/pinctrl/qcom/pinctrl-msm.h     |  1 +
 drivers/pinctrl/qcom/pinctrl-sc8180x.c | 49 ++++++++++++++++++++++++--
 4 files changed, 72 insertions(+), 7 deletions(-)

-- 
2.17.1

[PATCH v2 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 | 27 +++++++++++++++++++++++----
 drivers/pinctrl/qcom/pinctrl-msm.h |  1 +
 2 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index d70caecd21d2..af6ed7f43058 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>
@@ -1396,6 +1397,7 @@ int msm_pinctrl_probe(struct platform_device *pdev,
 {
 	struct msm_pinctrl *pctrl;
 	struct resource *res;
+	void __iomem *base;
 	int ret;
 	int i;
 
@@ -1412,7 +1414,8 @@ 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)) {
+		/* DT boot with multiple tile resources */
 		for (i = 0; i < soc_data->ntiles; i++) {
 			res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
 							   soc_data->tiles[i]);
@@ -1421,10 +1424,26 @@ int msm_pinctrl_probe(struct platform_device *pdev,
 				return PTR_ERR(pctrl->regs[i]);
 		}
 	} else {
+		/*
+		 * DT boot with one single resource or ACPI boot (always
+		 * one memory resource in DSDT)
+		 */
 		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) {
+			/*
+			 * ACPI boot with SoC driver using titles like
+			 * pinctrl-sc8180x.
+			 */
+			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 v2 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 | 49 ++++++++++++++++++++++++--
 2 files changed, 48 insertions(+), 3 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..3ec74ea37c1b 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 /* terminator */
+};
+
 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,13 +1602,43 @@ 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);
+	const struct msm_pinctrl_soc_data *soc_data;
+
+	soc_data = device_get_match_data(&pdev->dev);
+	if (!soc_data)
+		return -EINVAL;
+
+	return msm_pinctrl_probe(pdev, soc_data);
 }
 
+static const struct acpi_device_id sc8180x_pinctrl_acpi_match[] = {
+	{
+		.id = "QCOM040D",
+		.driver_data = (kernel_ulong_t) &sc8180x_acpi_pinctrl,
+	},
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, sc8180x_pinctrl_acpi_match);
+
 static const struct of_device_id sc8180x_pinctrl_of_match[] = {
-	{ .compatible = "qcom,sc8180x-tlmm", },
+	{
+		.compatible = "qcom,sc8180x-tlmm",
+		.data = &sc8180x_pinctrl,
+	},
 	{ },
 };
 MODULE_DEVICE_TABLE(of, sc8180x_pinctrl_of_match);
@@ -1603,6 +1647,7 @@ static struct platform_driver sc8180x_pinctrl_driver = {
 	.driver = {
 		.name = "sc8180x-pinctrl",
 		.of_match_table = sc8180x_pinctrl_of_match,
+		.acpi_match_table = sc8180x_pinctrl_acpi_match,
 	},
 	.probe = sc8180x_pinctrl_probe,
 	.remove = msm_pinctrl_remove,
-- 
2.17.1

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

[Andy Shevchenko]

On Wed, Mar 03, 2021 at 09:26:22PM +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.

> +#include <linux/acpi.h>

It's not used here.

property.h
mod_devicetable.h


-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Wed, Mar 03, 2021 at 09:26:21PM +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>

There are at least two possibilities to avoid this:
 - use is_of_node(dev_fwnode(dev)), or in case you need board files support,
   !(fwnode && is_of_fwnode(fwnode))
 - provide the tiles support directly from the driver thru internal data structures

 And to me the second approach seems better, because there is no guarantee that
 tiles support is only defined by the fwnode type.

-- 
With Best Regards,
Andy Shevchenko

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

[Shawn Guo]

On Wed, Mar 03, 2021 at 04:08:09PM +0200, Andy Shevchenko wrote:
> On Wed, Mar 03, 2021 at 09:26:21PM +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>
> 
> There are at least two possibilities to avoid this:

So could you explain why we should avoid including this header?

Shawn

>  - use is_of_node(dev_fwnode(dev)), or in case you need board files support,
>    !(fwnode && is_of_fwnode(fwnode))
>  - provide the tiles support directly from the driver thru internal data structures
> 
>  And to me the second approach seems better, because there is no guarantee that
>  tiles support is only defined by the fwnode type.

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

[Andy Shevchenko]

On Wed, Mar 03, 2021 at 10:45:27PM +0800, Shawn Guo wrote:
> On Wed, Mar 03, 2021 at 04:08:09PM +0200, Andy Shevchenko wrote:
> > On Wed, Mar 03, 2021 at 09:26:21PM +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>
> > 
> > There are at least two possibilities to avoid this:
> 
> So could you explain why we should avoid including this header?

Here you can include it, but it's quite huge in order to have just one little
function out of it. But main point is it seems that relying on firmware type
for the tiles support is fragile.

> >  - use is_of_node(dev_fwnode(dev)), or in case you need board files support,
> >    !(fwnode && is_of_fwnode(fwnode))
> >  - provide the tiles support directly from the driver thru internal data structures
> > 
> >  And to me the second approach seems better, because there is no guarantee that
> >  tiles support is only defined by the fwnode type.

-- 
With Best Regards,
Andy Shevchenko

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

[Shawn Guo]

On Wed, Mar 03, 2021 at 06:14:09PM +0200, Andy Shevchenko wrote:
> On Wed, Mar 03, 2021 at 10:45:27PM +0800, Shawn Guo wrote:
> > On Wed, Mar 03, 2021 at 04:08:09PM +0200, Andy Shevchenko wrote:
> > > On Wed, Mar 03, 2021 at 09:26:21PM +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>
> > > 
> > > There are at least two possibilities to avoid this:
> > 
> > So could you explain why we should avoid including this header?
> 
> Here you can include it, but it's quite huge in order to have just one little
> function out of it. But main point is it seems that relying on firmware type
> for the tiles support is fragile.

Okay, I'm not entirely happy about the tiles checking for so many
conditions, so looked for other way around.  It turns out that
pinctrl-sdm845 driver sets an example for not using tiles at all.  I
will send v3 to drop this tiles thing completely.

Shawn