[PATCH v1 1/4] pinctrl: intel: Split intel_pinctrl_add_padgroups() for better maintenance

[PATCH v1 1/4] pinctrl: intel: Split intel_pinctrl_add_padgroups() for better maintenance

[Andy Shevchenko]

Currently the intel_pinctrl_add_padgroups() is twisted a bit due to
a different nature of the pin control hardware implementations. Thus,
its maintenance is a bit hard. Besides that some pieces of code
are run on all hardware and make this code slightly inefficient,
and moreover, validation for one case is done in a wrong time in a flow
which makes it even slower.

Split intel_pinctrl_add_padgroups() to two functions, one per hardware
implementation, for better maintenance and readability.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pinctrl/intel/pinctrl-intel.c | 60 ++++++++++++++++++---------
 1 file changed, 40 insertions(+), 20 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index b6ef1911c1dd..ae13e4390935 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -1321,34 +1321,19 @@ static int intel_gpio_probe(struct intel_pinctrl *pctrl, int irq)
 	return 0;
 }
 
-static int intel_pinctrl_add_padgroups(struct intel_pinctrl *pctrl,
-				       struct intel_community *community)
+static int intel_pinctrl_add_padgroups_by_gpps(struct intel_pinctrl *pctrl,
+					       struct intel_community *community)
 {
 	struct intel_padgroup *gpps;
-	unsigned int npins = community->npins;
 	unsigned int padown_num = 0;
-	size_t ngpps, i;
-
-	if (community->gpps)
-		ngpps = community->ngpps;
-	else
-		ngpps = DIV_ROUND_UP(community->npins, community->gpp_size);
+	size_t i, ngpps = community->ngpps;
 
 	gpps = devm_kcalloc(pctrl->dev, ngpps, sizeof(*gpps), GFP_KERNEL);
 	if (!gpps)
 		return -ENOMEM;
 
 	for (i = 0; i < ngpps; i++) {
-		if (community->gpps) {
-			gpps[i] = community->gpps[i];
-		} else {
-			unsigned int gpp_size = community->gpp_size;
-
-			gpps[i].reg_num = i;
-			gpps[i].base = community->pin_base + i * gpp_size;
-			gpps[i].size = min(gpp_size, npins);
-			npins -= gpps[i].size;
-		}
+		gpps[i] = community->gpps[i];
 
 		if (gpps[i].size > 32)
 			return -EINVAL;
@@ -1366,6 +1351,38 @@ static int intel_pinctrl_add_padgroups(struct intel_pinctrl *pctrl,
 				break;
 		}
 
+		gpps[i].padown_num = padown_num;
+		padown_num += DIV_ROUND_UP(gpps[i].size * 4, 32);
+	}
+
+	community->gpps = gpps;
+
+	return 0;
+}
+
+static int intel_pinctrl_add_padgroups_by_size(struct intel_pinctrl *pctrl,
+					       struct intel_community *community)
+{
+	struct intel_padgroup *gpps;
+	unsigned int npins = community->npins;
+	unsigned int padown_num = 0;
+	size_t i, ngpps = DIV_ROUND_UP(npins, community->gpp_size);
+
+	if (community->gpp_size > 32)
+		return -EINVAL;
+
+	gpps = devm_kcalloc(pctrl->dev, ngpps, sizeof(*gpps), GFP_KERNEL);
+	if (!gpps)
+		return -ENOMEM;
+
+	for (i = 0; i < ngpps; i++) {
+		unsigned int gpp_size = community->gpp_size;
+
+		gpps[i].reg_num = i;
+		gpps[i].base = community->pin_base + i * gpp_size;
+		gpps[i].size = min(gpp_size, npins);
+		npins -= gpps[i].size;
+
 		gpps[i].padown_num = padown_num;
 
 		/*
@@ -1483,7 +1500,10 @@ static int intel_pinctrl_probe(struct platform_device *pdev,
 		community->regs = regs;
 		community->pad_regs = regs + padbar;
 
-		ret = intel_pinctrl_add_padgroups(pctrl, community);
+		if (community->gpps)
+			ret = intel_pinctrl_add_padgroups_by_gpps(pctrl, community);
+		else
+			ret = intel_pinctrl_add_padgroups_by_size(pctrl, community);
 		if (ret)
 			return ret;
 	}
-- 
2.29.2

[PATCH v1 2/4] pinctrl: intel: Drop unnecessary check for predefined features

[Andy Shevchenko]

None of the drivers is overriding features. Remove unnecessary check.
While here, rename rev to value to make easier further development.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pinctrl/intel/pinctrl-intel.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index ae13e4390935..1a479112ed85 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -1473,6 +1473,7 @@ static int intel_pinctrl_probe(struct platform_device *pdev,
 		struct intel_community *community = &pctrl->communities[i];
 		void __iomem *regs;
 		u32 padbar;
+		u32 value;
 
 		*community = pctrl->soc->communities[i];
 
@@ -1480,18 +1481,11 @@ static int intel_pinctrl_probe(struct platform_device *pdev,
 		if (IS_ERR(regs))
 			return PTR_ERR(regs);
 
-		/*
-		 * Determine community features based on the revision if
-		 * not specified already.
-		 */
-		if (!community->features) {
-			u32 rev;
-
-			rev = (readl(regs + REVID) & REVID_MASK) >> REVID_SHIFT;
-			if (rev >= 0x94) {
-				community->features |= PINCTRL_FEATURE_DEBOUNCE;
-				community->features |= PINCTRL_FEATURE_1K_PD;
-			}
+		/* Determine community features based on the revision */
+		value = readl(regs + REVID);
+		if (((value & REVID_MASK) >> REVID_SHIFT) >= 0x94) {
+			community->features |= PINCTRL_FEATURE_DEBOUNCE;
+			community->features |= PINCTRL_FEATURE_1K_PD;
 		}
 
 		/* Read offset of the pad configuration registers */
-- 
2.29.2

[PATCH v1 3/4] pinctrl: intel: Convert revision conditional to switch-case

[Andy Shevchenko]

switch-case is slightly better to maintain in case some new features will come
in new revisions of the hardware.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pinctrl/intel/pinctrl-intel.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index 1a479112ed85..00979acb0203 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -1483,9 +1483,13 @@ static int intel_pinctrl_probe(struct platform_device *pdev,
 
 		/* Determine community features based on the revision */
 		value = readl(regs + REVID);
-		if (((value & REVID_MASK) >> REVID_SHIFT) >= 0x94) {
+		switch ((value & REVID_MASK) >> REVID_SHIFT) {
+		case 0x0094 ... 0xffff:
 			community->features |= PINCTRL_FEATURE_DEBOUNCE;
 			community->features |= PINCTRL_FEATURE_1K_PD;
+			break;
+		default:
+			break;
 		}
 
 		/* Read offset of the pad configuration registers */
-- 
2.29.2

[PATCH v1 4/4] pinctrl: intel: Convert capability list to features

[Andy Shevchenko]

Communities can have features provided in the capability list.
Traverse the list and convert to respective features.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pinctrl/intel/pinctrl-intel.c | 41 +++++++++++++++++++++++++--
 drivers/pinctrl/intel/pinctrl-intel.h |  4 +++
 2 files changed, 42 insertions(+), 3 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index 00979acb0203..3d9f22ee987a 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -29,6 +29,16 @@
 #define REVID_SHIFT			16
 #define REVID_MASK			GENMASK(31, 16)
 
+#define CAPLIST				0x004
+#define CAPLIST_ID_SHIFT		16
+#define CAPLIST_ID_MASK			GENMASK(23, 16)
+#define CAPLIST_ID_GPIO_HW_INFO		1
+#define CAPLIST_ID_PWM			2
+#define CAPLIST_ID_BLINK		3
+#define CAPLIST_ID_EXP			4
+#define CAPLIST_NEXT_SHIFT		0
+#define CAPLIST_NEXT_MASK		GENMASK(15, 0)
+
 #define PADBAR				0x00c
 
 #define PADOWN_BITS			4
@@ -1472,7 +1482,7 @@ static int intel_pinctrl_probe(struct platform_device *pdev,
 	for (i = 0; i < pctrl->ncommunities; i++) {
 		struct intel_community *community = &pctrl->communities[i];
 		void __iomem *regs;
-		u32 padbar;
+		u32 offset;
 		u32 value;
 
 		*community = pctrl->soc->communities[i];
@@ -1492,11 +1502,36 @@ static int intel_pinctrl_probe(struct platform_device *pdev,
 			break;
 		}
 
+		/* Determine community features based on the capabilities */
+		offset = CAPLIST;
+		do {
+			value = readl(regs + offset);
+			switch ((value & CAPLIST_ID_MASK) >> CAPLIST_ID_SHIFT) {
+			case CAPLIST_ID_GPIO_HW_INFO:
+				community->features |= PINCTRL_FEATURE_GPIO_HW_INFO;
+				break;
+			case CAPLIST_ID_PWM:
+				community->features |= PINCTRL_FEATURE_PWM;
+				break;
+			case CAPLIST_ID_BLINK:
+				community->features |= PINCTRL_FEATURE_BLINK;
+				break;
+			case CAPLIST_ID_EXP:
+				community->features |= PINCTRL_FEATURE_EXP;
+				break;
+			default:
+				break;
+			}
+			offset = (value & CAPLIST_NEXT_MASK) >> CAPLIST_NEXT_SHIFT;
+		} while (offset);
+
+		dev_dbg(&pdev->dev, "Community%d features: %#08x\n", i, community->features);
+
 		/* Read offset of the pad configuration registers */
-		padbar = readl(regs + PADBAR);
+		offset = readl(regs + PADBAR);
 
 		community->regs = regs;
-		community->pad_regs = regs + padbar;
+		community->pad_regs = regs + offset;
 
 		if (community->gpps)
 			ret = intel_pinctrl_add_padgroups_by_gpps(pctrl, community);
diff --git a/drivers/pinctrl/intel/pinctrl-intel.h b/drivers/pinctrl/intel/pinctrl-intel.h
index ad34b7a3f6ed..c4fef03b663f 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.h
+++ b/drivers/pinctrl/intel/pinctrl-intel.h
@@ -143,6 +143,10 @@ struct intel_community {
 /* Additional features supported by the hardware */
 #define PINCTRL_FEATURE_DEBOUNCE	BIT(0)
 #define PINCTRL_FEATURE_1K_PD		BIT(1)
+#define PINCTRL_FEATURE_GPIO_HW_INFO	BIT(2)
+#define PINCTRL_FEATURE_PWM		BIT(3)
+#define PINCTRL_FEATURE_BLINK		BIT(4)
+#define PINCTRL_FEATURE_EXP		BIT(5)
 
 /**
  * PIN_GROUP - Declare a pin group
-- 
2.29.2

Re: [PATCH v1 3/4] pinctrl: intel: Convert revision conditional to switch-case

[Mika Westerberg]

On Thu, Jan 07, 2021 at 09:01:59PM +0200, Andy Shevchenko wrote:
> switch-case is slightly better to maintain in case some new features will come
> in new revisions of the hardware.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/pinctrl/intel/pinctrl-intel.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
> index 1a479112ed85..00979acb0203 100644
> --- a/drivers/pinctrl/intel/pinctrl-intel.c
> +++ b/drivers/pinctrl/intel/pinctrl-intel.c
> @@ -1483,9 +1483,13 @@ static int intel_pinctrl_probe(struct platform_device *pdev,
>  
>  		/* Determine community features based on the revision */
>  		value = readl(regs + REVID);
> -		if (((value & REVID_MASK) >> REVID_SHIFT) >= 0x94) {
> +		switch ((value & REVID_MASK) >> REVID_SHIFT) {
> +		case 0x0094 ... 0xffff:
>  			community->features |= PINCTRL_FEATURE_DEBOUNCE;
>  			community->features |= PINCTRL_FEATURE_1K_PD;
> +			break;
> +		default:
> +			break;

This adds 4 new lines and I don't think it improves anything so I
suggest we leave it as is for now. Once we have hardware that actually
implements some new features we can reconsider something like this.

Re: [PATCH v1 4/4] pinctrl: intel: Convert capability list to features

[Mika Westerberg]

On Thu, Jan 07, 2021 at 09:02:00PM +0200, Andy Shevchenko wrote:
> Communities can have features provided in the capability list.
> Traverse the list and convert to respective features.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/pinctrl/intel/pinctrl-intel.c | 41 +++++++++++++++++++++++++--
>  drivers/pinctrl/intel/pinctrl-intel.h |  4 +++
>  2 files changed, 42 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
> index 00979acb0203..3d9f22ee987a 100644
> --- a/drivers/pinctrl/intel/pinctrl-intel.c
> +++ b/drivers/pinctrl/intel/pinctrl-intel.c
> @@ -29,6 +29,16 @@
>  #define REVID_SHIFT			16
>  #define REVID_MASK			GENMASK(31, 16)
>  
> +#define CAPLIST				0x004
> +#define CAPLIST_ID_SHIFT		16
> +#define CAPLIST_ID_MASK			GENMASK(23, 16)
> +#define CAPLIST_ID_GPIO_HW_INFO		1
> +#define CAPLIST_ID_PWM			2
> +#define CAPLIST_ID_BLINK		3
> +#define CAPLIST_ID_EXP			4
> +#define CAPLIST_NEXT_SHIFT		0
> +#define CAPLIST_NEXT_MASK		GENMASK(15, 0)
> +
>  #define PADBAR				0x00c
>  
>  #define PADOWN_BITS			4
> @@ -1472,7 +1482,7 @@ static int intel_pinctrl_probe(struct platform_device *pdev,
>  	for (i = 0; i < pctrl->ncommunities; i++) {
>  		struct intel_community *community = &pctrl->communities[i];
>  		void __iomem *regs;
> -		u32 padbar;
> +		u32 offset;
>  		u32 value;
>  
>  		*community = pctrl->soc->communities[i];
> @@ -1492,11 +1502,36 @@ static int intel_pinctrl_probe(struct platform_device *pdev,
>  			break;
>  		}
>  
> +		/* Determine community features based on the capabilities */
> +		offset = CAPLIST;
> +		do {
> +			value = readl(regs + offset);
> +			switch ((value & CAPLIST_ID_MASK) >> CAPLIST_ID_SHIFT) {
> +			case CAPLIST_ID_GPIO_HW_INFO:
> +				community->features |= PINCTRL_FEATURE_GPIO_HW_INFO;
> +				break;
> +			case CAPLIST_ID_PWM:
> +				community->features |= PINCTRL_FEATURE_PWM;
> +				break;
> +			case CAPLIST_ID_BLINK:
> +				community->features |= PINCTRL_FEATURE_BLINK;
> +				break;
> +			case CAPLIST_ID_EXP:
> +				community->features |= PINCTRL_FEATURE_EXP;
> +				break;
> +			default:
> +				break;
> +			}
> +			offset = (value & CAPLIST_NEXT_MASK) >> CAPLIST_NEXT_SHIFT;

I suggest adding some check, like that we visited the previous offset
already, so that we do not loop here forever if we find wrongly
formatted capability list.

Otherwise looks good.

> +		} while (offset);

Re: [PATCH v1 1/4] pinctrl: intel: Split intel_pinctrl_add_padgroups() for better maintenance

[Mika Westerberg]

On Thu, Jan 07, 2021 at 09:01:57PM +0200, Andy Shevchenko wrote:
> Currently the intel_pinctrl_add_padgroups() is twisted a bit due to
> a different nature of the pin control hardware implementations. Thus,
> its maintenance is a bit hard. Besides that some pieces of code
> are run on all hardware and make this code slightly inefficient,
> and moreover, validation for one case is done in a wrong time in a flow
> which makes it even slower.
> 
> Split intel_pinctrl_add_padgroups() to two functions, one per hardware
> implementation, for better maintenance and readability.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH v1 2/4] pinctrl: intel: Drop unnecessary check for predefined features

[Mika Westerberg]

On Thu, Jan 07, 2021 at 09:01:58PM +0200, Andy Shevchenko wrote:
> None of the drivers is overriding features. Remove unnecessary check.
> While here, rename rev to value to make easier further development.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH v1 4/4] pinctrl: intel: Convert capability list to features

[Andy Shevchenko]

On Fri, Jan 8, 2021 at 9:09 AM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Thu, Jan 07, 2021 at 09:02:00PM +0200, Andy Shevchenko wrote:
> > Communities can have features provided in the capability list.
> > Traverse the list and convert to respective features.

...

> > +             /* Determine community features based on the capabilities */
> > +             offset = CAPLIST;
> > +             do {
> > +                     value = readl(regs + offset);
> > +                     switch ((value & CAPLIST_ID_MASK) >> CAPLIST_ID_SHIFT) {
> > +                     case CAPLIST_ID_GPIO_HW_INFO:
> > +                             community->features |= PINCTRL_FEATURE_GPIO_HW_INFO;
> > +                             break;
> > +                     case CAPLIST_ID_PWM:
> > +                             community->features |= PINCTRL_FEATURE_PWM;
> > +                             break;
> > +                     case CAPLIST_ID_BLINK:
> > +                             community->features |= PINCTRL_FEATURE_BLINK;
> > +                             break;
> > +                     case CAPLIST_ID_EXP:
> > +                             community->features |= PINCTRL_FEATURE_EXP;
> > +                             break;
> > +                     default:
> > +                             break;
> > +                     }
> > +                     offset = (value & CAPLIST_NEXT_MASK) >> CAPLIST_NEXT_SHIFT;
>
> I suggest adding some check, like that we visited the previous offset
> already, so that we do not loop here forever if we find wrongly
> formatted capability list.

I don't see how it could be achieved (offsets can be unordered). If
there is such an issue it will mean a silicon bug.
I never heard that we have similar checks in the PCI or xHCI code.
Maybe it's something new, do you know if it has similar code to see?

> Otherwise looks good.
>
> > +             } while (offset);



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 4/4] pinctrl: intel: Convert capability list to features

[Andy Shevchenko]

On Fri, Jan 8, 2021 at 2:22 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Fri, Jan 8, 2021 at 9:09 AM Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > On Thu, Jan 07, 2021 at 09:02:00PM +0200, Andy Shevchenko wrote:

...

> I don't see how it could be achieved (offsets can be unordered). If
> there is such an issue it will mean a silicon bug.

Specification says clearly that one register is a must and its value
defines the behaviour.

"The first Capability List register is located at offset 0x004...  and
contains a pointer/address to the next Capability List register. The
first Capability List register is no different than others... except
for its “Capability Identification” field is always 0. The total
number of Capability List registers... is 1 at the minimum (to
determine if there is any capability)."

So I prefer to stick with my original variant.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 4/4] pinctrl: intel: Convert capability list to features

[Mika Westerberg]

On Fri, Jan 08, 2021 at 02:31:23PM +0200, Andy Shevchenko wrote:
> On Fri, Jan 8, 2021 at 2:22 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Fri, Jan 8, 2021 at 9:09 AM Mika Westerberg
> > <mika.westerberg@linux.intel.com> wrote:
> > > On Thu, Jan 07, 2021 at 09:02:00PM +0200, Andy Shevchenko wrote:
> 
> ...
> 
> > I don't see how it could be achieved (offsets can be unordered). If
> > there is such an issue it will mean a silicon bug.
> 
> Specification says clearly that one register is a must and its value
> defines the behaviour.
> 
> "The first Capability List register is located at offset 0x004...  and
> contains a pointer/address to the next Capability List register. The
> first Capability List register is no different than others... except
> for its “Capability Identification” field is always 0. The total
> number of Capability List registers... is 1 at the minimum (to
> determine if there is any capability)."

This is not the first time something like this is done wrong at silicon
level. IMHO it is always good idea to avoid possible infinite loops
especially in the kernel space.

> So I prefer to stick with my original variant.

OK.

Re: [PATCH v1 4/4] pinctrl: intel: Convert capability list to features

[Andy Shevchenko]

On Fri, Jan 8, 2021 at 2:46 PM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Fri, Jan 08, 2021 at 02:31:23PM +0200, Andy Shevchenko wrote:
> > On Fri, Jan 8, 2021 at 2:22 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > > On Fri, Jan 8, 2021 at 9:09 AM Mika Westerberg
> > > <mika.westerberg@linux.intel.com> wrote:

...

> > > I don't see how it could be achieved (offsets can be unordered). If
> > > there is such an issue it will mean a silicon bug.
> >
> > Specification says clearly that one register is a must and its value
> > defines the behaviour.
> >
> > "The first Capability List register is located at offset 0x004...  and
> > contains a pointer/address to the next Capability List register. The
> > first Capability List register is no different than others... except
> > for its “Capability Identification” field is always 0. The total
> > number of Capability List registers... is 1 at the minimum (to
> > determine if there is any capability)."
>
> This is not the first time something like this is done wrong at silicon
> level.

I agree. What about solving the issue when it comes?

> IMHO it is always good idea to avoid possible infinite loops
> especially in the kernel space.

But do PCI / xHCI (the first two that came to my mind) have something like this?

> > So I prefer to stick with my original variant.
>
> OK.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 4/4] pinctrl: intel: Convert capability list to features

[Mika Westerberg]

On Fri, Jan 08, 2021 at 02:51:09PM +0200, Andy Shevchenko wrote:
> On Fri, Jan 8, 2021 at 2:46 PM Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > On Fri, Jan 08, 2021 at 02:31:23PM +0200, Andy Shevchenko wrote:
> > > On Fri, Jan 8, 2021 at 2:22 PM Andy Shevchenko
> > > <andy.shevchenko@gmail.com> wrote:
> > > > On Fri, Jan 8, 2021 at 9:09 AM Mika Westerberg
> > > > <mika.westerberg@linux.intel.com> wrote:
> 
> ...
> 
> > > > I don't see how it could be achieved (offsets can be unordered). If
> > > > there is such an issue it will mean a silicon bug.
> > >
> > > Specification says clearly that one register is a must and its value
> > > defines the behaviour.
> > >
> > > "The first Capability List register is located at offset 0x004...  and
> > > contains a pointer/address to the next Capability List register. The
> > > first Capability List register is no different than others... except
> > > for its “Capability Identification” field is always 0. The total
> > > number of Capability List registers... is 1 at the minimum (to
> > > determine if there is any capability)."
> >
> > This is not the first time something like this is done wrong at silicon
> > level.
> 
> I agree. What about solving the issue when it comes?

Up to you :)

> > IMHO it is always good idea to avoid possible infinite loops
> > especially in the kernel space.
> 
> But do PCI / xHCI (the first two that came to my mind) have something like this?

Yes they do, at least PCI. I would expect xHCI to have it too as the
hardware can be hot removed in the middle of a capability list walk
returning 1's on subsequent reads.

Re: [PATCH v1 4/4] pinctrl: intel: Convert capability list to features

[Andy Shevchenko]

On Fri, Jan 08, 2021 at 03:11:38PM +0200, Mika Westerberg wrote:
> On Fri, Jan 08, 2021 at 02:51:09PM +0200, Andy Shevchenko wrote:
> > On Fri, Jan 8, 2021 at 2:46 PM Mika Westerberg
> > <mika.westerberg@linux.intel.com> wrote:

...

> > What about solving the issue when it comes?
> 
> Up to you :)

I have sent v2 with dropped patch 3 and added your tag for patches 1 and 2.
Please, review the patch 3 which is basically kept unchanged.
Thanks!

> > > IMHO it is always good idea to avoid possible infinite loops
> > > especially in the kernel space.
> > 
> > But do PCI / xHCI (the first two that came to my mind) have something like this?
> 
> Yes they do, at least PCI. I would expect xHCI to have it too as the
> hardware can be hot removed in the middle of a capability list walk
> returning 1's on subsequent reads.

Good to know.

-- 
With Best Regards,
Andy Shevchenko