Re: [PATCH V3 1/2] pinctrl: intel: fix offset calculation issue of register PAD_OWN

Re: [PATCH V3 1/2] pinctrl: intel: fix offset calculation issue of register PAD_OWN

[Mika Westerberg]

On Thu, Nov 26, 2015 at 01:09:50AM +0800, Qipeng Zha wrote:
> The equation of offset calculation is not correct for PAD_OWN,
> verified this fix will get right offset.
> 
> Signed-off-by: Qi Zheng <qi.zheng@intel.com>
> Signed-off-by: Qipeng Zha <qipeng.zha@intel.com>
> ---
>  drivers/pinctrl/intel/pinctrl-intel.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
> index 392e28d..e3f580b 100644
> --- a/drivers/pinctrl/intel/pinctrl-intel.c
> +++ b/drivers/pinctrl/intel/pinctrl-intel.c
> @@ -37,6 +37,7 @@
>  #define PADOWN_BITS			4
>  #define PADOWN_SHIFT(p)			((p) % 8 * PADOWN_BITS)
>  #define PADOWN_MASK(p)			(0xf << PADOWN_SHIFT(p))
> +#define PADOWN_GPP(p)			((p) / 8)
>  
>  /* Offset from pad_regs */
>  #define PADCFG0				0x000
> @@ -142,7 +143,7 @@ static void __iomem *intel_get_padcfg(struct intel_pinctrl *pctrl, unsigned pin,
>  static bool intel_pad_owned_by_host(struct intel_pinctrl *pctrl, unsigned pin)
>  {
>  	const struct intel_community *community;
> -	unsigned padno, gpp, gpp_offset, offset;
> +	unsigned padno, gpp, offset;
>  	void __iomem *padown;
>  
>  	community = intel_get_community(pctrl, pin);
> @@ -152,9 +153,8 @@ static bool intel_pad_owned_by_host(struct intel_pinctrl *pctrl, unsigned pin)
>  		return true;
>  
>  	padno = pin_to_padno(community, pin);
> -	gpp = padno / NPADS_IN_GPP;
> -	gpp_offset = padno % NPADS_IN_GPP;
> -	offset = community->padown_offset + gpp * 16 + (gpp_offset / 8) * 4;
> +	gpp = PADOWN_GPP(padno);
> +	offset = community->padown_offset + gpp * 4;

Actually have you verified this on Sunrisepoint? 

If you check the public datasheet (6th Generation Intel Core processor
I/O Datasheet vol2) the GPP_B starts from 0x30.

First pad in GPP_B is 24 so we get:

  current code: 0x30
  your code:    0x2c

So this needs to take the gpp_size in account as well.

>  	padown = community->regs + offset;
>  
>  	return !(readl(padown) & PADOWN_MASK(padno));
> -- 
> 1.8.3.2

Re: [PATCH V3 2/2] pinctrl: intel: fix bug of register offset calculation

[Mika Westerberg]

On Thu, Nov 26, 2015 at 01:09:51AM +0800, Qipeng Zha wrote:
> The group size for registers PADCFGLOCK, HOSTSW_OWN, GPI_IS,
> GPI_IE, are not 24 for Broxton, Add a parameter to allow
> different platform to set correct value.
> 
> ---
> change in v3:
>  use community->gpp_size directly, not macro.
> 
> change in v2:
>  define gpp_size for sunrisepoint in its private structure.
> 
> Signed-off-by: Qi Zheng <qi.zheng@intel.com>
> Signed-off-by: Qipeng Zha <qipeng.zha@intel.com>

My previous ack still applies to this patch.

[PATCH V3 1/2] pinctrl: intel: fix offset calculation issue of register PAD_OWN

[Qipeng Zha]

The equation of offset calculation is not correct for PAD_OWN,
verified this fix will get right offset.

Signed-off-by: Qi Zheng <qi.zheng@intel.com>
Signed-off-by: Qipeng Zha <qipeng.zha@intel.com>
---
 drivers/pinctrl/intel/pinctrl-intel.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index 392e28d..e3f580b 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -37,6 +37,7 @@
 #define PADOWN_BITS			4
 #define PADOWN_SHIFT(p)			((p) % 8 * PADOWN_BITS)
 #define PADOWN_MASK(p)			(0xf << PADOWN_SHIFT(p))
+#define PADOWN_GPP(p)			((p) / 8)
 
 /* Offset from pad_regs */
 #define PADCFG0				0x000
@@ -142,7 +143,7 @@ static void __iomem *intel_get_padcfg(struct intel_pinctrl *pctrl, unsigned pin,
 static bool intel_pad_owned_by_host(struct intel_pinctrl *pctrl, unsigned pin)
 {
 	const struct intel_community *community;
-	unsigned padno, gpp, gpp_offset, offset;
+	unsigned padno, gpp, offset;
 	void __iomem *padown;
 
 	community = intel_get_community(pctrl, pin);
@@ -152,9 +153,8 @@ static bool intel_pad_owned_by_host(struct intel_pinctrl *pctrl, unsigned pin)
 		return true;
 
 	padno = pin_to_padno(community, pin);
-	gpp = padno / NPADS_IN_GPP;
-	gpp_offset = padno % NPADS_IN_GPP;
-	offset = community->padown_offset + gpp * 16 + (gpp_offset / 8) * 4;
+	gpp = PADOWN_GPP(padno);
+	offset = community->padown_offset + gpp * 4;
 	padown = community->regs + offset;
 
 	return !(readl(padown) & PADOWN_MASK(padno));
-- 
1.8.3.2

[PATCH V3 2/2] pinctrl: intel: fix bug of register offset calculation

[Qipeng Zha]

The group size for registers PADCFGLOCK, HOSTSW_OWN, GPI_IS,
GPI_IE, are not 24 for Broxton, Add a parameter to allow
different platform to set correct value.

---
change in v3:
 use community->gpp_size directly, not macro.

change in v2:
 define gpp_size for sunrisepoint in its private structure.

Signed-off-by: Qi Zheng <qi.zheng@intel.com>
Signed-off-by: Qipeng Zha <qipeng.zha@intel.com>
---
 drivers/pinctrl/intel/pinctrl-broxton.c      |  1 +
 drivers/pinctrl/intel/pinctrl-intel.c        | 32 +++++++++++++---------------
 drivers/pinctrl/intel/pinctrl-intel.h        |  3 +++
 drivers/pinctrl/intel/pinctrl-sunrisepoint.c |  1 +
 4 files changed, 20 insertions(+), 17 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-broxton.c b/drivers/pinctrl/intel/pinctrl-broxton.c
index e42d5d4..5979d38 100644
--- a/drivers/pinctrl/intel/pinctrl-broxton.c
+++ b/drivers/pinctrl/intel/pinctrl-broxton.c
@@ -28,6 +28,7 @@
 		.padcfglock_offset = BXT_PADCFGLOCK,	\
 		.hostown_offset = BXT_HOSTSW_OWN,	\
 		.ie_offset = BXT_GPI_IE,		\
+		.gpp_size = 32,                         \
 		.pin_base = (s),			\
 		.npins = ((e) - (s) + 1),		\
 	}
diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index e3f580b..d88cbcf 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -25,9 +25,6 @@
 
 #include "pinctrl-intel.h"
 
-/* Maximum number of pads in each group */
-#define NPADS_IN_GPP			24
-
 /* Offset from regs */
 #define PADBAR				0x00c
 #define GPI_IS				0x100
@@ -173,11 +170,11 @@ static bool intel_pad_acpi_mode(struct intel_pinctrl *pctrl, unsigned pin)
 		return false;
 
 	padno = pin_to_padno(community, pin);
-	gpp = padno / NPADS_IN_GPP;
+	gpp = padno / community->gpp_size;
 	offset = community->hostown_offset + gpp * 4;
 	hostown = community->regs + offset;
 
-	return !(readl(hostown) & BIT(padno % NPADS_IN_GPP));
+	return !(readl(hostown) & BIT(padno % community->gpp_size));
 }
 
 static bool intel_pad_locked(struct intel_pinctrl *pctrl, unsigned pin)
@@ -193,7 +190,7 @@ static bool intel_pad_locked(struct intel_pinctrl *pctrl, unsigned pin)
 		return false;
 
 	padno = pin_to_padno(community, pin);
-	gpp = padno / NPADS_IN_GPP;
+	gpp = padno / community->gpp_size;
 
 	/*
 	 * If PADCFGLOCK and PADCFGLOCKTX bits are both clear for this pad,
@@ -202,12 +199,12 @@ static bool intel_pad_locked(struct intel_pinctrl *pctrl, unsigned pin)
 	 */
 	offset = community->padcfglock_offset + gpp * 8;
 	value = readl(community->regs + offset);
-	if (value & BIT(pin % NPADS_IN_GPP))
+	if (value & BIT(pin % community->gpp_size))
 		return true;
 
 	offset = community->padcfglock_offset + 4 + gpp * 8;
 	value = readl(community->regs + offset);
-	if (value & BIT(pin % NPADS_IN_GPP))
+	if (value & BIT(pin % community->gpp_size))
 		return true;
 
 	return false;
@@ -663,8 +660,8 @@ static void intel_gpio_irq_ack(struct irq_data *d)
 	community = intel_get_community(pctrl, pin);
 	if (community) {
 		unsigned padno = pin_to_padno(community, pin);
-		unsigned gpp_offset = padno % NPADS_IN_GPP;
-		unsigned gpp = padno / NPADS_IN_GPP;
+		unsigned gpp_offset = padno % community->gpp_size;
+		unsigned gpp = padno / community->gpp_size;
 
 		writel(BIT(gpp_offset), community->regs + GPI_IS + gpp * 4);
 	}
@@ -685,8 +682,8 @@ static void intel_gpio_irq_mask_unmask(struct irq_data *d, bool mask)
 	community = intel_get_community(pctrl, pin);
 	if (community) {
 		unsigned padno = pin_to_padno(community, pin);
-		unsigned gpp_offset = padno % NPADS_IN_GPP;
-		unsigned gpp = padno / NPADS_IN_GPP;
+		unsigned gpp_offset = padno % community->gpp_size;
+		unsigned gpp = padno / community->gpp_size;
 		void __iomem *reg;
 		u32 value;
 
@@ -780,8 +777,8 @@ static int intel_gpio_irq_wake(struct irq_data *d, unsigned int on)
 		return -EINVAL;
 
 	padno = pin_to_padno(community, pin);
-	gpp = padno / NPADS_IN_GPP;
-	gpp_offset = padno % NPADS_IN_GPP;
+	gpp = padno / community->gpp_size;
+	gpp_offset = padno % community->gpp_size;
 
 	/* Clear the existing wake status */
 	writel(BIT(gpp_offset), community->regs + GPI_GPE_STS + gpp * 4);
@@ -819,14 +816,14 @@ static irqreturn_t intel_gpio_community_irq_handler(struct intel_pinctrl *pctrl,
 		/* Only interrupts that are enabled */
 		pending &= enabled;
 
-		for_each_set_bit(gpp_offset, &pending, NPADS_IN_GPP) {
+		for_each_set_bit(gpp_offset, &pending, community->gpp_size) {
 			unsigned padno, irq;
 
 			/*
 			 * The last group in community can have less pins
 			 * than NPADS_IN_GPP.
 			 */
-			padno = gpp_offset + gpp * NPADS_IN_GPP;
+			padno = gpp_offset + gpp * community->gpp_size;
 			if (padno >= community->npins)
 				break;
 
@@ -1002,7 +999,8 @@ int intel_pinctrl_probe(struct platform_device *pdev,
 
 		community->regs = regs;
 		community->pad_regs = regs + padbar;
-		community->ngpps = DIV_ROUND_UP(community->npins, NPADS_IN_GPP);
+		community->ngpps = DIV_ROUND_UP(community->npins,
+						community->gpp_size);
 	}
 
 	irq = platform_get_irq(pdev, 0);
diff --git a/drivers/pinctrl/intel/pinctrl-intel.h b/drivers/pinctrl/intel/pinctrl-intel.h
index 4ec8b57..b602157 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.h
+++ b/drivers/pinctrl/intel/pinctrl-intel.h
@@ -55,6 +55,8 @@ struct intel_function {
  *                  ACPI).
  * @ie_offset: Register offset of GPI_IE from @regs.
  * @pin_base: Starting pin of pins in this community
+ * @gpp_size: Maximum number of pads in each group, such as PADCFGLOCK,
+ *            HOSTSW_OWN,  GPI_IS, GPI_IE, etc.
  * @npins: Number of pins in this community
  * @regs: Community specific common registers (reserved for core driver)
  * @pad_regs: Community specific pad registers (reserved for core driver)
@@ -68,6 +70,7 @@ struct intel_community {
 	unsigned hostown_offset;
 	unsigned ie_offset;
 	unsigned pin_base;
+	unsigned gpp_size;
 	size_t npins;
 	void __iomem *regs;
 	void __iomem *pad_regs;
diff --git a/drivers/pinctrl/intel/pinctrl-sunrisepoint.c b/drivers/pinctrl/intel/pinctrl-sunrisepoint.c
index 1de9ae5..c725a53 100644
--- a/drivers/pinctrl/intel/pinctrl-sunrisepoint.c
+++ b/drivers/pinctrl/intel/pinctrl-sunrisepoint.c
@@ -30,6 +30,7 @@
 		.padcfglock_offset = SPT_PADCFGLOCK,	\
 		.hostown_offset = SPT_HOSTSW_OWN,	\
 		.ie_offset = SPT_GPI_IE,		\
+		.gpp_size = 24,				\
 		.pin_base = (s),			\
 		.npins = ((e) - (s) + 1),		\
 	}
-- 
1.8.3.2

Re: [PATCH V3 2/2] pinctrl: intel: fix bug of register offset calculation

[Linus Walleij]

On Wed, Nov 25, 2015 at 6:09 PM, Qipeng Zha <qipeng.zha@intel.com> wrote:

> The group size for registers PADCFGLOCK, HOSTSW_OWN, GPI_IS,
> GPI_IE, are not 24 for Broxton, Add a parameter to allow
> different platform to set correct value.
>
> ---
> change in v3:
>  use community->gpp_size directly, not macro.
>
> change in v2:
>  define gpp_size for sunrisepoint in its private structure.
>
> Signed-off-by: Qi Zheng <qi.zheng@intel.com>
> Signed-off-by: Qipeng Zha <qipeng.zha@intel.com>

Patch applied. For 4.5 since I can't figure out if this is
a regression or not.

Yours,
Linus Walleij

Re: [PATCH V3 2/2] pinctrl: intel: fix bug of register offset calculation

[Westerberg, Mika]

On Tue, Dec 01, 2015 at 11:16:45AM +0100, Linus Walleij wrote:
> On Wed, Nov 25, 2015 at 6:09 PM, Qipeng Zha <qipeng.zha@intel.com> wrote:
> 
> > The group size for registers PADCFGLOCK, HOSTSW_OWN, GPI_IS,
> > GPI_IE, are not 24 for Broxton, Add a parameter to allow
> > different platform to set correct value.
> >
> > ---
> > change in v3:
> >  use community->gpp_size directly, not macro.
> >
> > change in v2:
> >  define gpp_size for sunrisepoint in its private structure.
> >
> > Signed-off-by: Qi Zheng <qi.zheng@intel.com>
> > Signed-off-by: Qipeng Zha <qipeng.zha@intel.com>
> 
> Patch applied. For 4.5 since I can't figure out if this is
> a regression or not.

Broxton support was added in v4.4 so it would be nice if these patches
could get still into v4.4.

Thanks.

Re: [PATCH V3 2/2] pinctrl: intel: fix bug of register offset calculation

[Linus Walleij]

On Thu, Dec 10, 2015 at 10:04 AM, Westerberg, Mika
<mika.westerberg@intel.com> wrote:
> On Tue, Dec 01, 2015 at 11:16:45AM +0100, Linus Walleij wrote:
>> On Wed, Nov 25, 2015 at 6:09 PM, Qipeng Zha <qipeng.zha@intel.com> wrote:
>>
>> > The group size for registers PADCFGLOCK, HOSTSW_OWN, GPI_IS,
>> > GPI_IE, are not 24 for Broxton, Add a parameter to allow
>> > different platform to set correct value.
>> >
>> > ---
>> > change in v3:
>> >  use community->gpp_size directly, not macro.
>> >
>> > change in v2:
>> >  define gpp_size for sunrisepoint in its private structure.
>> >
>> > Signed-off-by: Qi Zheng <qi.zheng@intel.com>
>> > Signed-off-by: Qipeng Zha <qipeng.zha@intel.com>
>>
>> Patch applied. For 4.5 since I can't figure out if this is
>> a regression or not.
>
> Broxton support was added in v4.4 so it would be nice if these patches
> could get still into v4.4.

Yeah I queued it for fixes, but missed to apply 1/2, fixing it up now.

Yours,
Linus Walleij

Re: [PATCH V3 2/2] pinctrl: intel: fix bug of register offset calculation

[Westerberg, Mika]

On Thu, Dec 10, 2015 at 10:58:44PM +0100, Linus Walleij wrote:
> On Thu, Dec 10, 2015 at 10:04 AM, Westerberg, Mika
> <mika.westerberg@intel.com> wrote:
> > On Tue, Dec 01, 2015 at 11:16:45AM +0100, Linus Walleij wrote:
> >> On Wed, Nov 25, 2015 at 6:09 PM, Qipeng Zha <qipeng.zha@intel.com> wrote:
> >>
> >> > The group size for registers PADCFGLOCK, HOSTSW_OWN, GPI_IS,
> >> > GPI_IE, are not 24 for Broxton, Add a parameter to allow
> >> > different platform to set correct value.
> >> >
> >> > ---
> >> > change in v3:
> >> >  use community->gpp_size directly, not macro.
> >> >
> >> > change in v2:
> >> >  define gpp_size for sunrisepoint in its private structure.
> >> >
> >> > Signed-off-by: Qi Zheng <qi.zheng@intel.com>
> >> > Signed-off-by: Qipeng Zha <qipeng.zha@intel.com>
> >>
> >> Patch applied. For 4.5 since I can't figure out if this is
> >> a regression or not.
> >
> > Broxton support was added in v4.4 so it would be nice if these patches
> > could get still into v4.4.
> 
> Yeah I queued it for fixes, but missed to apply 1/2, fixing it up now.

Thanks!