diff mbox series

[v4] pinctrl: qcom: sc8180x: add ACPI probe support

Message ID 20210310111210.1232-1-shawn.guo@linaro.org
State Superseded
Headers show
Series [v4] pinctrl: qcom: sc8180x: add ACPI probe support | expand

Commit Message

Shawn Guo March 10, 2021, 11:12 a.m. UTC
It adds ACPI probe support for pinctrl-sc8180x driver.  We have one
problem with ACPI table, i.e. GIO0 (TLMM) block has one single memory
resource to cover 3 tiles defined by SC8180X.  To follow the hardware
layout of 3 tiles which is already supported DT probe, it adds one
function to replace the original single memory resource with 3 named
ones for tiles.  With that, We can map memory for ACPI in the same way
as DT.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>

---
Changes for 4:
 - Add sc8180x_pinctrl_add_tile_resources() to massage memory resource
   for ACPI probe.

Changes for v3:
- Remove the use of tiles completely.
- Drop unneed include of acpi.h.

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.

 drivers/pinctrl/qcom/Kconfig           |   2 +-
 drivers/pinctrl/qcom/pinctrl-sc8180x.c | 119 ++++++++++++++++++++++++-
 2 files changed, 118 insertions(+), 3 deletions(-)

-- 
2.17.1

Comments

Bjorn Andersson March 10, 2021, 4:53 p.m. UTC | #1
On Wed 10 Mar 06:09 CST 2021, Andy Shevchenko wrote:

> On Wed, Mar 10, 2021 at 07:12:10PM +0800, Shawn Guo wrote:

> > It adds ACPI probe support for pinctrl-sc8180x driver.  We have one

> > problem with ACPI table, i.e. GIO0 (TLMM) block has one single memory

> > resource to cover 3 tiles defined by SC8180X.  To follow the hardware

> > layout of 3 tiles which is already supported DT probe, it adds one

> > function to replace the original single memory resource with 3 named

> > ones for tiles.  With that, We can map memory for ACPI in the same way

> > as DT.

> 

> You are reinventing a wheel, i.e. MFD framework. Can you simple utilize

> devm_mfd_add_devices()?

> 


But wouldn't such driver need to do exactly this, and then set up the
mfd cell and register it? So the new wheel would still be there, just
wrapped in more code?

> Basically you may write an new pure MFD driver (drivers/mfd) that will

> instantiate properly the pin control driver.

> 


In contrast to typical MFDs this would still be a single mfd_cell, just
with different set of resources, derived from the mfd device itself.

Regards,
Bjorn
Andy Shevchenko March 10, 2021, 4:59 p.m. UTC | #2
On Wed, Mar 10, 2021 at 6:55 PM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
> On Wed 10 Mar 06:09 CST 2021, Andy Shevchenko wrote:

> > On Wed, Mar 10, 2021 at 07:12:10PM +0800, Shawn Guo wrote:

> > > It adds ACPI probe support for pinctrl-sc8180x driver.  We have one

> > > problem with ACPI table, i.e. GIO0 (TLMM) block has one single memory

> > > resource to cover 3 tiles defined by SC8180X.  To follow the hardware

> > > layout of 3 tiles which is already supported DT probe, it adds one

> > > function to replace the original single memory resource with 3 named

> > > ones for tiles.  With that, We can map memory for ACPI in the same way

> > > as DT.

> >

> > You are reinventing a wheel, i.e. MFD framework. Can you simple utilize

> > devm_mfd_add_devices()?

> >

>

> But wouldn't such driver need to do exactly this, and then set up the

> mfd cell and register it? So the new wheel would still be there, just

> wrapped in more code?


Are you sure it will take more code?

> > Basically you may write an new pure MFD driver (drivers/mfd) that will

> > instantiate properly the pin control driver.

> >

>

> In contrast to typical MFDs this would still be a single mfd_cell, just

> with different set of resources, derived from the mfd device itself.


Yes, I understand the current case, but in my proposal you will have a
separate driver for all quirks needed for this ACPI HID
implementation.

In any case, I just pointed out that MFD is worth considering.

-- 
With Best Regards,
Andy Shevchenko
Bjorn Andersson March 10, 2021, 7:24 p.m. UTC | #3
On Wed 10 Mar 05:12 CST 2021, Shawn Guo wrote:

> It adds ACPI probe support for pinctrl-sc8180x driver.  We have one

> problem with ACPI table, i.e. GIO0 (TLMM) block has one single memory

> resource to cover 3 tiles defined by SC8180X.  To follow the hardware

> layout of 3 tiles which is already supported DT probe, it adds one

> function to replace the original single memory resource with 3 named

> ones for tiles.  With that, We can map memory for ACPI in the same way

> as DT.

> 

> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>

> ---

> Changes for 4:

>  - Add sc8180x_pinctrl_add_tile_resources() to massage memory resource

>    for ACPI probe.

> 

> Changes for v3:

> - Remove the use of tiles completely.

> - Drop unneed include of acpi.h.

> 

> 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.

> 

>  drivers/pinctrl/qcom/Kconfig           |   2 +-

>  drivers/pinctrl/qcom/pinctrl-sc8180x.c | 119 ++++++++++++++++++++++++-

>  2 files changed, 118 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..4ff80a7a1221 100644

> --- a/drivers/pinctrl/qcom/pinctrl-sc8180x.c

> +++ b/drivers/pinctrl/qcom/pinctrl-sc8180x.c

> @@ -23,6 +23,21 @@ enum {

>  	WEST

>  };

>  

> +/*

> + * ACPI DSDT has one single memory resource for TLMM.  The offsets below are

> + * used to locate different tiles for ACPI probe.

> + */

> +struct tile_info {

> +	u32 offset;

> +	u32 size;

> +};

> +

> +static const struct tile_info sc8180x_tile_info[] = {

> +	{ 0x00d00000, 0x00300000, },

> +	{ 0x00500000, 0x00700000, },

> +	{ 0x00100000, 0x00300000, },

> +};

> +

>  #define FUNCTION(fname)					\

>  	[msm_mux_##fname] = {				\

>  		.name = #fname,				\

> @@ -1557,6 +1572,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 +1610,105 @@ 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),

> +	.pins = sc8180x_pins,

> +	.npins = ARRAY_SIZE(sc8180x_pins),

> +	.groups = sc8180x_groups,

> +	.ngroups = ARRAY_SIZE(sc8180x_groups),

> +	.reserved_gpios = sc8180x_acpi_reserved_gpios,

> +	.ngpios = 191,


As I said before, there really only is 190 things called "GPIO" on this
platform - the 191 thing is a hack of mine, but it's inclusion here
should depend on how the ACPI system would reset the UFS device - if it
does so at all.

> +};

> +


A comment describing the purpose of this function (to split the
IORESOURCE_MEM into three) would be in its place.

> +static int sc8180x_pinctrl_add_tile_resources(struct platform_device *pdev)

> +{

> +	int nres_num = pdev->num_resources + ARRAY_SIZE(sc8180x_tiles) - 1;

> +	struct resource *mres, *nres, *res;

> +	int i, ret;

> +

> +	/*

> +	 * DT already has tiles defined properly, so nothing needs to be done

> +	 * for DT probe.

> +	 */

> +	if (pdev->dev.of_node)

> +		return 0;

> +

> +	/* Allocate for new resources */

> +	nres = devm_kzalloc(&pdev->dev, sizeof(*nres) * nres_num, GFP_KERNEL);

> +	if (!nres)

> +		return -ENOMEM;

> +

> +	res = nres;

> +

> +	for (i = 0; i < pdev->num_resources; i++) {

> +		struct resource *r = &pdev->resource[i];

> +

> +		/* Save memory resource and copy others */

> +		if (resource_type(r) == IORESOURCE_MEM)

> +			mres = r;

> +		else

> +			*res++ = *r;

> +	}

> +

> +	/* Append tile memory resources */

> +	for (i = 0; i < ARRAY_SIZE(sc8180x_tiles); i++, res++) {

> +		const struct tile_info *info = &sc8180x_tile_info[i];

> +

> +		res->start = mres->start + info->offset;

> +		res->end = mres->start + info->offset + info->size - 1;

> +		res->flags = mres->flags;

> +		res->name = sc8180x_tiles[i];

> +

> +		/* Add new MEM to resource tree */

> +		insert_resource(mres->parent, res);

> +	}

> +

> +	/* Remove old MEM from resource tree */

> +	remove_resource(mres);

> +

> +	/* Free old resources and install new ones */

> +	ret = platform_device_add_resources(pdev, nres, nres_num);

> +	if (ret) {

> +		dev_err(&pdev->dev, "failed to add new resources: %d\n", ret);

> +		return ret;

> +	}

> +

> +	return 0;

> +}

> +

>  static int sc8180x_pinctrl_probe(struct platform_device *pdev)

>  {

> -	return msm_pinctrl_probe(pdev, &sc8180x_pinctrl);

> +	const struct msm_pinctrl_soc_data *soc_data;

> +	int ret;

> +

> +	soc_data = device_get_match_data(&pdev->dev);

> +	if (!soc_data)

> +		return -EINVAL;

> +

> +	ret = sc8180x_pinctrl_add_tile_resources(pdev);

> +	if (ret) {

> +		dev_err(&pdev->dev, "failed to add tile resources: %d\n", ret);


sc8180x_pinctrl_add_tile_resources() already printed an error when we
get here, so no need to print another one.


Other than that, I think this looks good!

Regards,
Bjorn

> +		return ret;

> +	}

> +

> +	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 +1717,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

>
Shawn Guo March 11, 2021, 1:29 a.m. UTC | #4
On Wed, Mar 10, 2021 at 01:24:49PM -0600, Bjorn Andersson wrote:
> > @@ -1588,13 +1610,105 @@ 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),

> > +	.pins = sc8180x_pins,

> > +	.npins = ARRAY_SIZE(sc8180x_pins),

> > +	.groups = sc8180x_groups,

> > +	.ngroups = ARRAY_SIZE(sc8180x_groups),

> > +	.reserved_gpios = sc8180x_acpi_reserved_gpios,

> > +	.ngpios = 191,

> 

> As I said before, there really only is 190 things called "GPIO" on this

> platform - the 191 thing is a hack of mine, but it's inclusion here

> should depend on how the ACPI system would reset the UFS device - if it

> does so at all.


No, so far I do not see reset is required for ACPI boot.  I will keep it
as 190 to match hardware spec.

> 

> > +};

> > +

> 

> A comment describing the purpose of this function (to split the

> IORESOURCE_MEM into three) would be in its place.


Okay.

> 

> > +static int sc8180x_pinctrl_add_tile_resources(struct platform_device *pdev)

> > +{

> > +	int nres_num = pdev->num_resources + ARRAY_SIZE(sc8180x_tiles) - 1;

> > +	struct resource *mres, *nres, *res;

> > +	int i, ret;

> > +

> > +	/*

> > +	 * DT already has tiles defined properly, so nothing needs to be done

> > +	 * for DT probe.

> > +	 */

> > +	if (pdev->dev.of_node)

> > +		return 0;

> > +

> > +	/* Allocate for new resources */

> > +	nres = devm_kzalloc(&pdev->dev, sizeof(*nres) * nres_num, GFP_KERNEL);

> > +	if (!nres)

> > +		return -ENOMEM;

> > +

> > +	res = nres;

> > +

> > +	for (i = 0; i < pdev->num_resources; i++) {

> > +		struct resource *r = &pdev->resource[i];

> > +

> > +		/* Save memory resource and copy others */

> > +		if (resource_type(r) == IORESOURCE_MEM)

> > +			mres = r;

> > +		else

> > +			*res++ = *r;

> > +	}

> > +

> > +	/* Append tile memory resources */

> > +	for (i = 0; i < ARRAY_SIZE(sc8180x_tiles); i++, res++) {

> > +		const struct tile_info *info = &sc8180x_tile_info[i];

> > +

> > +		res->start = mres->start + info->offset;

> > +		res->end = mres->start + info->offset + info->size - 1;

> > +		res->flags = mres->flags;

> > +		res->name = sc8180x_tiles[i];

> > +

> > +		/* Add new MEM to resource tree */

> > +		insert_resource(mres->parent, res);

> > +	}

> > +

> > +	/* Remove old MEM from resource tree */

> > +	remove_resource(mres);

> > +

> > +	/* Free old resources and install new ones */

> > +	ret = platform_device_add_resources(pdev, nres, nres_num);

> > +	if (ret) {

> > +		dev_err(&pdev->dev, "failed to add new resources: %d\n", ret);

> > +		return ret;

> > +	}

> > +

> > +	return 0;

> > +}

> > +

> >  static int sc8180x_pinctrl_probe(struct platform_device *pdev)

> >  {

> > -	return msm_pinctrl_probe(pdev, &sc8180x_pinctrl);

> > +	const struct msm_pinctrl_soc_data *soc_data;

> > +	int ret;

> > +

> > +	soc_data = device_get_match_data(&pdev->dev);

> > +	if (!soc_data)

> > +		return -EINVAL;

> > +

> > +	ret = sc8180x_pinctrl_add_tile_resources(pdev);

> > +	if (ret) {

> > +		dev_err(&pdev->dev, "failed to add tile resources: %d\n", ret);

> 

> sc8180x_pinctrl_add_tile_resources() already printed an error when we

> get here, so no need to print another one.


Sure.

Shawn
diff mbox series

Patch

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..4ff80a7a1221 100644
--- a/drivers/pinctrl/qcom/pinctrl-sc8180x.c
+++ b/drivers/pinctrl/qcom/pinctrl-sc8180x.c
@@ -23,6 +23,21 @@  enum {
 	WEST
 };
 
+/*
+ * ACPI DSDT has one single memory resource for TLMM.  The offsets below are
+ * used to locate different tiles for ACPI probe.
+ */
+struct tile_info {
+	u32 offset;
+	u32 size;
+};
+
+static const struct tile_info sc8180x_tile_info[] = {
+	{ 0x00d00000, 0x00300000, },
+	{ 0x00500000, 0x00700000, },
+	{ 0x00100000, 0x00300000, },
+};
+
 #define FUNCTION(fname)					\
 	[msm_mux_##fname] = {				\
 		.name = #fname,				\
@@ -1557,6 +1572,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 +1610,105 @@  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),
+	.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_add_tile_resources(struct platform_device *pdev)
+{
+	int nres_num = pdev->num_resources + ARRAY_SIZE(sc8180x_tiles) - 1;
+	struct resource *mres, *nres, *res;
+	int i, ret;
+
+	/*
+	 * DT already has tiles defined properly, so nothing needs to be done
+	 * for DT probe.
+	 */
+	if (pdev->dev.of_node)
+		return 0;
+
+	/* Allocate for new resources */
+	nres = devm_kzalloc(&pdev->dev, sizeof(*nres) * nres_num, GFP_KERNEL);
+	if (!nres)
+		return -ENOMEM;
+
+	res = nres;
+
+	for (i = 0; i < pdev->num_resources; i++) {
+		struct resource *r = &pdev->resource[i];
+
+		/* Save memory resource and copy others */
+		if (resource_type(r) == IORESOURCE_MEM)
+			mres = r;
+		else
+			*res++ = *r;
+	}
+
+	/* Append tile memory resources */
+	for (i = 0; i < ARRAY_SIZE(sc8180x_tiles); i++, res++) {
+		const struct tile_info *info = &sc8180x_tile_info[i];
+
+		res->start = mres->start + info->offset;
+		res->end = mres->start + info->offset + info->size - 1;
+		res->flags = mres->flags;
+		res->name = sc8180x_tiles[i];
+
+		/* Add new MEM to resource tree */
+		insert_resource(mres->parent, res);
+	}
+
+	/* Remove old MEM from resource tree */
+	remove_resource(mres);
+
+	/* Free old resources and install new ones */
+	ret = platform_device_add_resources(pdev, nres, nres_num);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to add new resources: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
 static int sc8180x_pinctrl_probe(struct platform_device *pdev)
 {
-	return msm_pinctrl_probe(pdev, &sc8180x_pinctrl);
+	const struct msm_pinctrl_soc_data *soc_data;
+	int ret;
+
+	soc_data = device_get_match_data(&pdev->dev);
+	if (!soc_data)
+		return -EINVAL;
+
+	ret = sc8180x_pinctrl_add_tile_resources(pdev);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to add tile resources: %d\n", ret);
+		return ret;
+	}
+
+	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 +1717,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,