[PATCH 0/3] gpio: Intel Baytrail support

[PATCH 0/3] gpio: Intel Baytrail support

[Antoine Tenart]

This series introduces the GPIO Intel Baytrail support, using both the
lpc ich and the gpio ich drivers.

Antoine Tenart (3):
  gpio: gpio-ich: add support for Intel Baytrail
  mfd: lpc_ich: use a correct mask for the GPIO base address
  mfd: lpc_ich: add GPIO support for Baytrail

 drivers/gpio/gpio-ich.c     | 12 ++++++++++++
 drivers/mfd/lpc_ich.c       |  6 +++++-
 include/linux/mfd/lpc_ich.h |  1 +
 3 files changed, 18 insertions(+), 1 deletion(-)

-- 
2.7.0

[PATCH 1/3] gpio: gpio-ich: add support for Intel Baytrail

[Antoine Tenart]

This patch adds support for the Braytrail series, with the hardware
blink capability.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 drivers/gpio/gpio-ich.c     | 12 ++++++++++++
 include/linux/mfd/lpc_ich.h |  1 +
 2 files changed, 13 insertions(+)

diff --git a/drivers/gpio/gpio-ich.c b/drivers/gpio/gpio-ich.c
index 4ba7ed502131..6f5fcae32855 100644
--- a/drivers/gpio/gpio-ich.c
+++ b/drivers/gpio/gpio-ich.c
@@ -384,6 +384,15 @@ static struct ichx_desc avoton_desc = {
 	.use_outlvl_cache = true,
 };
 
+/* Baytrail */
+static struct ichx_desc baytrail_desc = {
+	.ngpio = 96,
+	.regs = ichx_regs,
+	.reglen = ichx_reglen,
+	.have_blink = true,
+	.use_outlvl_cache = true,
+};
+
 static int ichx_gpio_request_regions(struct resource *res_base,
 						const char *name, u8 use_gpio)
 {
@@ -461,6 +470,9 @@ static int ichx_gpio_probe(struct platform_device *pdev)
 	case AVOTON_GPIO:
 		ichx_priv.desc = &avoton_desc;
 		break;
+	case ICH_BAYTRAIL_GPIO:
+		ichx_priv.desc = &baytrail_desc;
+		break;
 	default:
 		return -ENODEV;
 	}
diff --git a/include/linux/mfd/lpc_ich.h b/include/linux/mfd/lpc_ich.h
index 2b300b44f994..659a1cd875d8 100644
--- a/include/linux/mfd/lpc_ich.h
+++ b/include/linux/mfd/lpc_ich.h
@@ -34,6 +34,7 @@ enum {
 	ICH_V10CORP_GPIO,
 	ICH_V10CONS_GPIO,
 	AVOTON_GPIO,
+	ICH_BAYTRAIL_GPIO,
 };
 
 struct lpc_ich_info {
-- 
2.7.0

[PATCH 2/3] mfd: lpc_ich: use a correct mask for the GPIO base address

[Antoine Tenart]

The GPIO base address is read from the GPIOBASE register. The first
bit must be cleared as it can be hardwired to 1 to represent the i/o
space. Other bits are either containing the base address of are
reserved. They should not be cleared as all the chipsets do not have
the same reserved bits.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 drivers/mfd/lpc_ich.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
index b514f3cf140d..f13a5ded3958 100644
--- a/drivers/mfd/lpc_ich.c
+++ b/drivers/mfd/lpc_ich.c
@@ -921,7 +921,10 @@ static int lpc_ich_init_gpio(struct pci_dev *dev)
 gpe0_done:
 	/* Setup GPIO base register */
 	pci_read_config_dword(dev, priv->gbase, &base_addr_cfg);
-	base_addr = base_addr_cfg & 0x0000ff80;
+
+	/* Clear the i/o flag */
+	base_addr = base_addr_cfg & ~BIT(0);
+
 	if (!base_addr) {
 		dev_notice(&dev->dev, "I/O space for GPIO uninitialized\n");
 		ret = -ENODEV;
-- 
2.7.0

[PATCH 3/3] mfd: lpc_ich: add GPIO support for Baytrail

[Antoine Tenart]

This patch adds the GPIO support for the Baytrail family.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 drivers/mfd/lpc_ich.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
index f13a5ded3958..d86b496874df 100644
--- a/drivers/mfd/lpc_ich.c
+++ b/drivers/mfd/lpc_ich.c
@@ -508,6 +508,7 @@ static struct lpc_ich_info lpc_chipset_info[] = {
 	[LPC_BAYTRAIL] = {
 		.name = "Bay Trail SoC",
 		.iTCO_version = 3,
+		.gpio_version = ICH_BAYTRAIL_GPIO,
 	},
 	[LPC_COLETO] = {
 		.name = "Coleto Creek",
-- 
2.7.0

Re: [PATCH 0/3] gpio: Intel Baytrail support

[Mika Westerberg]

On Sat, Jan 23, 2016 at 05:32:06PM +0100, Antoine Tenart wrote:
> This series introduces the GPIO Intel Baytrail support, using both the
> lpc ich and the gpio ich drivers.
> 
> Antoine Tenart (3):
>   gpio: gpio-ich: add support for Intel Baytrail

Intel Baytrail support has been in mainline kernel for quite some time
already. The driver is drivers/pintctrl/intel/pinctrl-baytrail.c.

Also the ICH GPIO block is not compatible to Baytrail which is why there
is a separate driver in the first place. Have you tested this series on
Baytrail?

>   mfd: lpc_ich: use a correct mask for the GPIO base address
>   mfd: lpc_ich: add GPIO support for Baytrail

All Baytrails I've seen expose the GPIO device via ACPI namespace so
there should be no need for the above two patches.

Re: [PATCH 1/3] gpio: gpio-ich: add support for Intel Baytrail

[Lee Jones]

On Sat, 23 Jan 2016, Antoine Tenart wrote:

> This patch adds support for the Braytrail series, with the hardware
> blink capability.
> 
> Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
> ---
>  drivers/gpio/gpio-ich.c     | 12 ++++++++++++
>  include/linux/mfd/lpc_ich.h |  1 +

Acked-by: Lee Jones <lee.jones@linaro.org>

>  2 files changed, 13 insertions(+)
> 
> diff --git a/drivers/gpio/gpio-ich.c b/drivers/gpio/gpio-ich.c
> index 4ba7ed502131..6f5fcae32855 100644
> --- a/drivers/gpio/gpio-ich.c
> +++ b/drivers/gpio/gpio-ich.c
> @@ -384,6 +384,15 @@ static struct ichx_desc avoton_desc = {
>  	.use_outlvl_cache = true,
>  };
>  
> +/* Baytrail */
> +static struct ichx_desc baytrail_desc = {
> +	.ngpio = 96,
> +	.regs = ichx_regs,
> +	.reglen = ichx_reglen,
> +	.have_blink = true,
> +	.use_outlvl_cache = true,
> +};
> +
>  static int ichx_gpio_request_regions(struct resource *res_base,
>  						const char *name, u8 use_gpio)
>  {
> @@ -461,6 +470,9 @@ static int ichx_gpio_probe(struct platform_device *pdev)
>  	case AVOTON_GPIO:
>  		ichx_priv.desc = &avoton_desc;
>  		break;
> +	case ICH_BAYTRAIL_GPIO:
> +		ichx_priv.desc = &baytrail_desc;
> +		break;
>  	default:
>  		return -ENODEV;
>  	}
> diff --git a/include/linux/mfd/lpc_ich.h b/include/linux/mfd/lpc_ich.h
> index 2b300b44f994..659a1cd875d8 100644
> --- a/include/linux/mfd/lpc_ich.h
> +++ b/include/linux/mfd/lpc_ich.h
> @@ -34,6 +34,7 @@ enum {
>  	ICH_V10CORP_GPIO,
>  	ICH_V10CONS_GPIO,
>  	AVOTON_GPIO,
> +	ICH_BAYTRAIL_GPIO,
>  };
>  
>  struct lpc_ich_info {

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] mfd: lpc_ich: use a correct mask for the GPIO base address

[Lee Jones]

On Sat, 23 Jan 2016, Antoine Tenart wrote:

> The GPIO base address is read from the GPIOBASE register. The first
> bit must be cleared as it can be hardwired to 1 to represent the i/o
> space. Other bits are either containing the base address of are
> reserved. They should not be cleared as all the chipsets do not have
> the same reserved bits.
> 
> Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
> ---
>  drivers/mfd/lpc_ich.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)

Applied, thanks.

> diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
> index b514f3cf140d..f13a5ded3958 100644
> --- a/drivers/mfd/lpc_ich.c
> +++ b/drivers/mfd/lpc_ich.c
> @@ -921,7 +921,10 @@ static int lpc_ich_init_gpio(struct pci_dev *dev)
>  gpe0_done:
>  	/* Setup GPIO base register */
>  	pci_read_config_dword(dev, priv->gbase, &base_addr_cfg);
> -	base_addr = base_addr_cfg & 0x0000ff80;
> +
> +	/* Clear the i/o flag */
> +	base_addr = base_addr_cfg & ~BIT(0);
> +
>  	if (!base_addr) {
>  		dev_notice(&dev->dev, "I/O space for GPIO uninitialized\n");
>  		ret = -ENODEV;

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 3/3] mfd: lpc_ich: add GPIO support for Baytrail

[Lee Jones]

On Sat, 23 Jan 2016, Antoine Tenart wrote:

> This patch adds the GPIO support for the Baytrail family.
> 
> Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
> ---
>  drivers/mfd/lpc_ich.c | 1 +
>  1 file changed, 1 insertion(+)

I can't take this yet, as it has a dependency on the GPIO patch.

For my own reference:
  Acked-by: Lee Jones <lee.jones@linaro.org>

> diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
> index f13a5ded3958..d86b496874df 100644
> --- a/drivers/mfd/lpc_ich.c
> +++ b/drivers/mfd/lpc_ich.c
> @@ -508,6 +508,7 @@ static struct lpc_ich_info lpc_chipset_info[] = {
>  	[LPC_BAYTRAIL] = {
>  		.name = "Bay Trail SoC",
>  		.iTCO_version = 3,
> +		.gpio_version = ICH_BAYTRAIL_GPIO,
>  	},
>  	[LPC_COLETO] = {
>  		.name = "Coleto Creek",

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] mfd: lpc_ich: use a correct mask for the GPIO base address

[Peter Tyser]

On Mon, 2016-01-25 at 12:44 +0000, Lee Jones wrote:
> On Sat, 23 Jan 2016, Antoine Tenart wrote:
> 
> > The GPIO base address is read from the GPIOBASE register. The first
> > bit must be cleared as it can be hardwired to 1 to represent the i/o
> > space. Other bits are either containing the base address of are
> > reserved. They should not be cleared as all the chipsets do not have
> > the same reserved bits.
> > 
> > Signed-off-by: Antoine Tenart tenart@free-electrons.com>
> > ---
> >  drivers/mfd/lpc_ich.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> Applied, thanks.

Is it possible to hold off on the application of the change Lee?

> > diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
> > index b514f3cf140d..f13a5ded3958 100644
> > --- a/drivers/mfd/lpc_ich.c
> > +++ b/drivers/mfd/lpc_ich.c
> > @@ -921,7 +921,10 @@ static int lpc_ich_init_gpio(struct pci_dev *dev)
> >  gpe0_done:
> >         /* Setup GPIO base register */
> >         pci_read_config_dword(dev, priv->gbase, &base_addr_cfg);
> > -       base_addr = base_addr_cfg & 0x0000ff80;
> > +
> > +       /* Clear the i/o flag */
> > +       base_addr = base_addr_cfg & ~BIT(0);
> > +


Does this patch work around an issue you are seeing?  Looking at the Bay 
Trail EDS, the GPIO base address register looks like it should work fine 
with the original code (it uses 0xff00 as a mask for the address, and 
reserves 0x80 which reads as a 0).  Also, Bay Trail bit 1 is an enable 
flag, which this patch wouldn't mask off.  Eg if the BIOS enables the GPIO 
controller and sets the enable bit, I think things would break with this 
patch.

It's also scary to not mask off the reserved bits on other Intel chipsets -
you're assuming they all read as 0 and I'm not sure if this is true or 
not.  The patch also doesn't make the same change to the other base 
register reads either, and ideally they'd be kept in sync.

Seems like things should be left as-is, or use an accurate chip-specific 
mask.

I'd leave as-is personally.  Like Mika mentioned, Baytrail GPIO should 
already be supported elsewhere, which should make this change unnecessary.

Regards,
Peter

Re: [PATCH 3/3] mfd: lpc_ich: add GPIO support for Baytrail

[Linus Walleij]

On Sat, Jan 23, 2016 at 5:32 PM, Antoine Tenart
<antoine.tenart@free-electrons.com> wrote:

> This patch adds the GPIO support for the Baytrail family.
>
> Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>

I'm confused by this and Mika's comment on patch 0.

Is the BayTrail an ICH variant or not? Or does it have
two GPIO controllers? Or um ....

Yours,
Linus Walleij

Re: [PATCH 3/3] mfd: lpc_ich: add GPIO support for Baytrail

[Mika Westerberg]

On Thu, Jan 28, 2016 at 11:34:40AM +0100, Linus Walleij wrote:
> On Sat, Jan 23, 2016 at 5:32 PM, Antoine Tenart
> <antoine.tenart@free-electrons.com> wrote:
> 
> > This patch adds the GPIO support for the Baytrail family.
> >
> > Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
> 
> I'm confused by this and Mika's comment on patch 0.
> 
> Is the BayTrail an ICH variant or not? Or does it have
> two GPIO controllers? Or um ....

It is not compatible with ICH and there is only one GPIO controller in
the SoC.

Re: [PATCH 2/3] mfd: lpc_ich: use a correct mask for the GPIO base address

[Lee Jones]

On Mon, 25 Jan 2016, Peter Tyser wrote:

> 
> On Mon, 2016-01-25 at 12:44 +0000, Lee Jones wrote:
> > On Sat, 23 Jan 2016, Antoine Tenart wrote:
> > 
> > > The GPIO base address is read from the GPIOBASE register. The first
> > > bit must be cleared as it can be hardwired to 1 to represent the i/o
> > > space. Other bits are either containing the base address of are
> > > reserved. They should not be cleared as all the chipsets do not have
> > > the same reserved bits.
> > > 
> > > Signed-off-by: Antoine Tenart tenart@free-electrons.com>
> > > ---
> > >  drivers/mfd/lpc_ich.c | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > Applied, thanks.
> 
> Is it possible to hold off on the application of the change Lee?

Patch unapplied.

> > > diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
> > > index b514f3cf140d..f13a5ded3958 100644
> > > --- a/drivers/mfd/lpc_ich.c
> > > +++ b/drivers/mfd/lpc_ich.c
> > > @@ -921,7 +921,10 @@ static int lpc_ich_init_gpio(struct pci_dev *dev)
> > >  gpe0_done:
> > >         /* Setup GPIO base register */
> > >         pci_read_config_dword(dev, priv->gbase, &base_addr_cfg);
> > > -       base_addr = base_addr_cfg & 0x0000ff80;
> > > +
> > > +       /* Clear the i/o flag */
> > > +       base_addr = base_addr_cfg & ~BIT(0);
> > > +
> 
> 
> Does this patch work around an issue you are seeing?  Looking at the Bay 
> Trail EDS, the GPIO base address register looks like it should work fine 
> with the original code (it uses 0xff00 as a mask for the address, and 
> reserves 0x80 which reads as a 0).  Also, Bay Trail bit 1 is an enable 
> flag, which this patch wouldn't mask off.  Eg if the BIOS enables the GPIO 
> controller and sets the enable bit, I think things would break with this 
> patch.
> 
> It's also scary to not mask off the reserved bits on other Intel chipsets -
> you're assuming they all read as 0 and I'm not sure if this is true or 
> not.  The patch also doesn't make the same change to the other base 
> register reads either, and ideally they'd be kept in sync.
> 
> Seems like things should be left as-is, or use an accurate chip-specific 
> mask.
> 
> I'd leave as-is personally.  Like Mika mentioned, Baytrail GPIO should 
> already be supported elsewhere, which should make this change unnecessary.
> 
> Regards,
> Peter

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html