| Message ID | 20210107190200.41221-4-andriy.shevchenko@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1,1/4] pinctrl: intel: Split intel_pinctrl_add_padgroups() for better maintenance | expand |
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);
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);
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.
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.
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.
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.
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.
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
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(-)