| Message ID | 20210107190200.41221-1-andriy.shevchenko@linux.intel.com |
|---|---|
| State | Superseded |
| 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 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>
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>
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
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
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. -- With Best Regards, Andy Shevchenko
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. -- With Best Regards, Andy Shevchenko
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; }
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(-)