| Message ID | 1513189818-7384-4-git-send-email-timur@codeaurora.org |
|---|---|
| State | New |
| Headers | show |
| Series | pinctrl: qcom: add support for sparse GPIOs | expand |
On 12/13, Timur Tabi wrote: > Newer versions of the firmware for the Qualcomm Datacenter Technologies > QDF2400 restricts access to a subset of the GPIOs on the TLMM. To > prevent older kernels from accidentally accessing the restricted GPIOs, > we change the ACPI HID for the TLMM block from QCOM8001 to QCOM8002, > and introduce a new property "gpios". This property is an array of > specific GPIOs that are accessible. When an older kernel boots on > newer (restricted) firmware, it will fail to probe. > > To implement the sparse GPIO map, we register all of the GPIOs, but set > the pin count for the unavailable GPIOs to zero. The pinctrl-msm > driver will block those unavailable GPIOs from being accessed. > > To allow newer kernels to support older firmware, the driver retains > support for QCOM8001. > > Signed-off-by: Timur Tabi <timur@codeaurora.org> > --- > drivers/pinctrl/qcom/pinctrl-qdf2xxx.c | 134 +++++++++++++++++++++++++-------- > 1 file changed, 103 insertions(+), 31 deletions(-) > > diff --git a/drivers/pinctrl/qcom/pinctrl-qdf2xxx.c b/drivers/pinctrl/qcom/pinctrl-qdf2xxx.c > index bb3ce5c3e18b..deb08e08e86d 100644 > --- a/drivers/pinctrl/qcom/pinctrl-qdf2xxx.c > +++ b/drivers/pinctrl/qcom/pinctrl-qdf2xxx.c > @@ -38,68 +38,139 @@ > /* maximum size of each gpio name (enough room for "gpioXXX" + null) */ > #define NAME_SIZE 8 > > +enum { > + QDF2XXX_V1, > + QDF2XXX_V2, > +}; > + > static int qdf2xxx_pinctrl_probe(struct platform_device *pdev) > { > + const struct acpi_device_id *id; > struct pinctrl_pin_desc *pins; > struct msm_pingroup *groups; > char (*names)[NAME_SIZE]; > unsigned int i; > u32 num_gpios; > + unsigned int avail_gpios; /* The number of GPIOs we support */ > + u16 *gpios; /* An array of supported GPIOs */ > int ret; > > /* Query the number of GPIOs from ACPI */ > ret = device_property_read_u32(&pdev->dev, "num-gpios", &num_gpios); > if (ret < 0) { > - dev_warn(&pdev->dev, "missing num-gpios property\n"); > + dev_err(&pdev->dev, "missing 'num-gpios' property\n"); > return ret; > } > - > if (!num_gpios || num_gpios > MAX_GPIOS) { Given that we have MAX_GPIOS, it would be better to declare a bitmap of available gpios of that size on the stack and then iterate through the bitmap and set bits for the available ones. In the QCOM8001 case, that would be setting all bits up to num_gpios, and in the QCOM8002 case it would be iterating through the list of gpios from the DSD property and setting the bit for that gpio number. This avoids explicitly allocating a list of numbers that is freed almost immediately. Instead we just stack 256 / sizeof(unsigned long) words and set bits. Hopefully we could lift the same logic into the core pinctrl msm driver for usage on non-ACPI systems.
On 12/13/2017 05:01 PM, Stephen Boyd wrote: > Given that we have MAX_GPIOS, it would be better to declare a > bitmap of available gpios of that size on the stack and then > iterate through the bitmap and set bits for the available ones. > In the QCOM8001 case, that would be setting all bits up to > num_gpios, and in the QCOM8002 case it would be iterating through > the list of gpios from the DSD property and setting the bit for > that gpio number. This avoids explicitly allocating a list of > numbers that is freed almost immediately. Instead we just stack > 256 / sizeof(unsigned long) words and set bits. I'm not sure I understand. The only think I'm allocating temporarily is the 'gpios' array, which is an array of shorts. Each element stores the gpio number. It's not a bit array, so "256 / sizeof(unsigned long)" doesn't apply. I need that array to read the DSD. You can't iterate through an DSD property without reading it completely first. > Hopefully we could lift the same logic into the core pinctrl msm > driver for usage on non-ACPI systems. There is no new memory allocation being done in pinctrl-msm, so I don't understand this either.
Stephen, any follow-up to this? I'd like to get these patches into 4.16 if at all possible. Thanks. On 12/13/17 5:09 PM, Timur Tabi wrote: > On 12/13/2017 05:01 PM, Stephen Boyd wrote: >> Given that we have MAX_GPIOS, it would be better to declare a >> bitmap of available gpios of that size on the stack and then >> iterate through the bitmap and set bits for the available ones. >> In the QCOM8001 case, that would be setting all bits up to >> num_gpios, and in the QCOM8002 case it would be iterating through >> the list of gpios from the DSD property and setting the bit for >> that gpio number. This avoids explicitly allocating a list of >> numbers that is freed almost immediately. Instead we just stack >> 256 / sizeof(unsigned long) words and set bits. > > I'm not sure I understand. The only think I'm allocating temporarily is > the 'gpios' array, which is an array of shorts. Each element stores the > gpio number. It's not a bit array, so "256 / sizeof(unsigned long)" > doesn't apply. I need that array to read the DSD. You can't iterate > through an DSD property without reading it completely first. > >> Hopefully we could lift the same logic into the core pinctrl msm >> driver for usage on non-ACPI systems. > > There is no new memory allocation being done in pinctrl-msm, so I don't > understand this either. >
On 12/18, Timur Tabi wrote: > Stephen, any follow-up to this? I'd like to get these patches into > 4.16 if at all possible. Thanks. > Ah I missed that the u16 array can't be iterated through. Any chance the ACPI tables can be changed to list pin ranges, like <33 3>, <90 2>, to indicate that pins 33, 34, 35 and pins 90, 91 are available? That would allow us to put that into the core pinctrl-msm.c file a little better and then only expose pins on the gpiochip when call gpiochip_add_pin_range(). If we want to support this in DT, I think we would have a DT property like available-gpios = <33 3>, <90 2>, <100 34> that we can then iterate through and add only these pins to the gpiochip. That's better than a bitmap in DT and is still compressed somewhat. Without going all the way down into that path, here's my patch to make your patch smaller, but perhaps we can just look for the ACPI property or the DT property in the pinctrl-msm.c core and then add pin ranges directly. Then this ACPI driver doesn't really need to change besides for the ID update. We can expose all the pins and offsets, etc. from the hardware driver but cut out gpios in the core layer in a generic way. ---8<--- From: Timur Tabi <timur@codeaurora.org> Newer versions of the firmware for the Qualcomm Datacenter Technologies QDF2400 restricts access to a subset of the GPIOs on the TLMM. To prevent older kernels from accidentally accessing the restricted GPIOs, we change the ACPI HID for the TLMM block from QCOM8001 to QCOM8002, and introduce a new property "gpios". This property is an array of specific GPIOs that are accessible. When an older kernel boots on newer (restricted) firmware, it will fail to probe. To implement the sparse GPIO map, we register all of the GPIOs, but set the pin count for the unavailable GPIOs to zero. The pinctrl-msm driver will block those unavailable GPIOs from being accessed. To allow newer kernels to support older firmware, the driver retains support for QCOM8001. Signed-off-by: Timur Tabi <timur@codeaurora.org> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org> --- drivers/pinctrl/qcom/pinctrl-qdf2xxx.c | 89 +++++++++++++++++++++++++++++----- 1 file changed, 77 insertions(+), 12 deletions(-) diff --git a/drivers/pinctrl/qcom/pinctrl-qdf2xxx.c b/drivers/pinctrl/qcom/pinctrl-qdf2xxx.c index bb3ce5c3e18b..2ca2f40719b3 100644 --- a/drivers/pinctrl/qcom/pinctrl-qdf2xxx.c +++ b/drivers/pinctrl/qcom/pinctrl-qdf2xxx.c @@ -38,45 +38,107 @@ static struct msm_pinctrl_soc_data qdf2xxx_pinctrl; /* maximum size of each gpio name (enough room for "gpioXXX" + null) */ #define NAME_SIZE 8 +enum { + QDF2XXX_V1, + QDF2XXX_V2, +}; + static int qdf2xxx_pinctrl_probe(struct platform_device *pdev) { + const struct acpi_device_id *id; struct pinctrl_pin_desc *pins; struct msm_pingroup *groups; char (*names)[NAME_SIZE]; - unsigned int i; + unsigned int i, j; u32 num_gpios; + unsigned int avail_gpios; /* The number of GPIOs we support */ + u16 *gpios = NULL; /* An array of supported GPIOs */ int ret; /* Query the number of GPIOs from ACPI */ ret = device_property_read_u32(&pdev->dev, "num-gpios", &num_gpios); if (ret < 0) { - dev_warn(&pdev->dev, "missing num-gpios property\n"); + dev_err(&pdev->dev, "missing 'num-gpios' property\n"); return ret; } - if (!num_gpios || num_gpios > MAX_GPIOS) { - dev_warn(&pdev->dev, "invalid num-gpios property\n"); + dev_err(&pdev->dev, "invalid 'num-gpios' property\n"); return -ENODEV; } + /* + * The QCOM8001 HID contains only the number of GPIOs, and assumes + * that all of them are available. avail_gpios is the same as num_gpios. + * + * The QCOM8002 HID introduces the 'gpios' DSD, which lists + * specific GPIOs that the driver is allowed to access. + * + * The make the common code simpler, in both cases we create an + * array of GPIOs that are accessible. So for QCOM8001, that would + * be all of the GPIOs. + */ + id = acpi_match_device(pdev->dev.driver->acpi_match_table, &pdev->dev); + + if (id->driver_data == QDF2XXX_V1) { + avail_gpios = num_gpios; + } else { + /* The number of GPIOs in the approved list */ + ret = device_property_read_u16_array(&pdev->dev, "gpios", + NULL, 0); + if (ret < 0) { + dev_err(&pdev->dev, "missing 'gpios' property\n"); + return ret; + } + /* + * The number of available GPIOs should be non-zero, and no + * more than the total number of GPIOS. + */ + if (!ret || ret > num_gpios) { + dev_err(&pdev->dev, "invalid 'gpios' property\n"); + return -ENODEV; + } + avail_gpios = ret; + + gpios = devm_kmalloc_array(&pdev->dev, avail_gpios, + sizeof(gpios[0]), GFP_KERNEL); + if (!gpios) + return -ENOMEM; + + /* Assume the array of available GPIOs is sorted */ + ret = device_property_read_u16_array(&pdev->dev, "gpios", gpios, + avail_gpios); + if (ret < 0) { + dev_err(&pdev->dev, "could not read list of GPIOs\n"); + return ret; + } + } + pins = devm_kcalloc(&pdev->dev, num_gpios, sizeof(struct pinctrl_pin_desc), GFP_KERNEL); groups = devm_kcalloc(&pdev->dev, num_gpios, sizeof(struct msm_pingroup), GFP_KERNEL); - names = devm_kcalloc(&pdev->dev, num_gpios, NAME_SIZE, GFP_KERNEL); + names = devm_kcalloc(&pdev->dev, avail_gpios, NAME_SIZE, GFP_KERNEL); if (!pins || !groups || !names) return -ENOMEM; - for (i = 0; i < num_gpios; i++) { - snprintf(names[i], NAME_SIZE, "gpio%u", i); - + /* + * Initialize the array. GPIOs not listed in the 'gpios' bitmap + * still need a number, but nothing else. + */ + for (i = 0, j = 0; i < num_gpios; i++) { pins[i].number = i; - pins[i].name = names[i]; + groups[i].pins = &pins[i].number; + + /* Only expose GPIOs that are available */ + if (gpios && gpios[j] != i) + continue; groups[i].npins = 1; - groups[i].name = names[i]; - groups[i].pins = &pins[i].number; + snprintf(names[j], NAME_SIZE, "gpio%u", i); + pins[i].name = names[j]; + groups[i].name = names[j]; + j++; groups[i].ctl_reg = 0x10000 * i; groups[i].io_reg = 0x04 + 0x10000 * i; @@ -100,6 +162,8 @@ static int qdf2xxx_pinctrl_probe(struct platform_device *pdev) groups[i].intr_detection_width = 2; } + devm_kfree(&pdev->dev, gpios); + qdf2xxx_pinctrl.pins = pins; qdf2xxx_pinctrl.groups = groups; qdf2xxx_pinctrl.npins = num_gpios; @@ -110,7 +174,8 @@ static int qdf2xxx_pinctrl_probe(struct platform_device *pdev) } static const struct acpi_device_id qdf2xxx_acpi_ids[] = { - {"QCOM8001"}, + {"QCOM8001", QDF2XXX_V1}, + {"QCOM8002", QDF2XXX_V2}, {}, }; MODULE_DEVICE_TABLE(acpi, qdf2xxx_acpi_ids);
On 12/18/17 8:39 PM, Stephen Boyd wrote: > Ah I missed that the u16 array can't be iterated through. Any > chance the ACPI tables can be changed to list pin ranges, like > <33 3>, <90 2>, to indicate that pins 33, 34, 35 and pins 90, 91 > are available? It's too late. Firmware is already shipping with the current layout. Unfortunately, there's no good peer review process for DSDs that don't have a DT equivalent. > That would allow us to put that into the core > pinctrl-msm.c file a little better and then only expose pins on > the gpiochip when call gpiochip_add_pin_range(). If we want to > support this in DT, I think we would have a DT property like > available-gpios = <33 3>, <90 2>, <100 34> that we can then > iterate through and add only these pins to the gpiochip. That's > better than a bitmap in DT and is still compressed somewhat. Keep in mind that all this ACPI junk is localized to pinctrl-qdf2xxx. pinctrl-msm does not define any new data structures, it just reuses the existing one. You can still define your DT properties any way you want in your client drivers. pinctrl-qdf2xxx is specific to the Centriq chips. > Without going all the way down into that path, here's my patch to > make your patch smaller, but perhaps we can just look for the > ACPI property or the DT property in the pinctrl-msm.c core and > then add pin ranges directly. Then this ACPI driver doesn't > really need to change besides for the ID update. We can expose > all the pins and offsets, etc. from the hardware driver but cut > out gpios in the core layer in a generic way. Ok, let me review this. I don't think there's any gain in moving the ACPI processing to pinctrl-msm, however.
On 12/18, Timur Tabi wrote: > On 12/18/17 8:39 PM, Stephen Boyd wrote: > >Ah I missed that the u16 array can't be iterated through. Any > >chance the ACPI tables can be changed to list pin ranges, like > ><33 3>, <90 2>, to indicate that pins 33, 34, 35 and pins 90, 91 > >are available? > > It's too late. Firmware is already shipping with the current > layout. Unfortunately, there's no good peer review process for DSDs > that don't have a DT equivalent. Alright! > > >That would allow us to put that into the core > >pinctrl-msm.c file a little better and then only expose pins on > >the gpiochip when call gpiochip_add_pin_range(). If we want to > >support this in DT, I think we would have a DT property like > >available-gpios = <33 3>, <90 2>, <100 34> that we can then > >iterate through and add only these pins to the gpiochip. That's > >better than a bitmap in DT and is still compressed somewhat. > > Keep in mind that all this ACPI junk is localized to > pinctrl-qdf2xxx. pinctrl-msm does not define any new data > structures, it just reuses the existing one. You can still define > your DT properties any way you want in your client drivers. > pinctrl-qdf2xxx is specific to the Centriq chips. Of course. > > >Without going all the way down into that path, here's my patch to > >make your patch smaller, but perhaps we can just look for the > >ACPI property or the DT property in the pinctrl-msm.c core and > >then add pin ranges directly. Then this ACPI driver doesn't > >really need to change besides for the ID update. We can expose > >all the pins and offsets, etc. from the hardware driver but cut > >out gpios in the core layer in a generic way. > > Ok, let me review this. I don't think there's any gain in moving > the ACPI processing to pinctrl-msm, however. > I will attempt to implement the DT part today. It may make the get_direction() revert irrelevant if the gpios aren't even exposed to gpiolib in the first place.
On 12/18/2017 08:39 PM, Stephen Boyd wrote: > + for (i = 0, j = 0; i < num_gpios; i++) { > pins[i].number = i; > - pins[i].name = names[i]; > + groups[i].pins = &pins[i].number; > + > + /* Only expose GPIOs that are available */ > + if (gpios && gpios[j] != i) > + continue; I don't know if I would say this is an improvement. For one thing, QCOM8001 systems are deprecated and don't really exist any more. At the time I originally wrote this patch, they were still in the wild, but they're all gone now. So it's no longer efficient to treat QCOM8001 as the default case. This means that the for-loop will iterate over the full range now, instead of the partial range that it does with my v10 patch. If I post another version of this patch, I'm just going to remove support for QCOM8001. If you want to avoid kmalloc'ing the GPIOs array, we can put it on the stack with a dynamic size, since it will be no more than MAX_GPIOS * 2 (i.e. 512) bytes in size. u16 gpios[avail_gpios]; It would be a little hackish since it needs to be defined at the beginning of a code block, so I would probably put into its own function, but I still fail to see what's wrong with using kmalloc to allocate that array for short-term use temporarily.
On 12/19, Timur Tabi wrote: > On 12/18/2017 08:39 PM, Stephen Boyd wrote: > >+ for (i = 0, j = 0; i < num_gpios; i++) { > > pins[i].number = i; > >- pins[i].name = names[i]; > >+ groups[i].pins = &pins[i].number; > >+ > >+ /* Only expose GPIOs that are available */ > >+ if (gpios && gpios[j] != i) > >+ continue; > > I don't know if I would say this is an improvement. For one thing, > QCOM8001 systems are deprecated and don't really exist any more. At > the time I originally wrote this patch, they were still in the wild, > but they're all gone now. So it's no longer efficient to treat > QCOM8001 as the default case. This means that the for-loop will > iterate over the full range now, instead of the partial range that > it does with my v10 patch. > > If I post another version of this patch, I'm just going to remove > support for QCOM8001. That sounds good too. The diff was really noisy because all the foo[i] became foo[gpio] which causes the diff to increase for no real purpose. My patch was rewriting that stuff so it doesn't come into the diff and we can concentrate on what's actually changing. We already iterate over the full range to fill in the two fields anyway, so I'm not sure what you're getting at with your for-loop comment. Seems like a micro-optimization on probe that probably isn't going to be noticed. > > If you want to avoid kmalloc'ing the GPIOs array, we can put it on > the stack with a dynamic size, since it will be no more than > MAX_GPIOS * 2 (i.e. 512) bytes in size. > > u16 gpios[avail_gpios]; > > It would be a little hackish since it needs to be defined at the > beginning of a code block, so I would probably put into its own > function, but I still fail to see what's wrong with using kmalloc to > allocate that array for short-term use temporarily. > Yeah I wouldn't do that. I'm not trying to avoid allocating the array anymore. Dynamically sized arrays on the stack are not a great idea in the kernel where we have limited stack sizes.
On 12/19/2017 02:30 PM, Stephen Boyd wrote: > Yeah I wouldn't do that. I'm not trying to avoid allocating the > array anymore. Dynamically sized arrays on the stack are not a > great idea in the kernel where we have limited stack sizes. So do you just want to see a v11 that drops support for QCOM8001?
On 12/18/2017 08:39 PM, Stephen Boyd wrote: > + if (gpios && gpios[j] != i) > + continue; ... > + j++; Doesn't this assume that the available GPIOs are listed in numerical order in the ACPI table? If so, then this patch won't work because that order is not guaranteed.
On 12/19, Timur Tabi wrote: > On 12/18/2017 08:39 PM, Stephen Boyd wrote: > >+ if (gpios && gpios[j] != i) > >+ continue; > ... > > + j++; > > Doesn't this assume that the available GPIOs are listed in numerical > order in the ACPI table? If so, then this patch won't work because > that order is not guaranteed. > Yes, I added a comment in the patch about that assumption. It's simple enough to sort the array in place with sort() though. I was looking at the gpiochip_add_pin_ranges() code to try and understand how to only expose pins that are supported as gpios into gpiolib, and then I looked at the history of these patches and wrote some code around pin ranges, got super confused and started thinking about adding gpiochips for each range (ugh), talked to Bjorn, and now I'm writing this mail. The approach of multiple ranges or chips looks like a dead-end that you've already gone down so let's not do that. The thing that I don't like about this patch is that we're modifying npins to indicate what gpios are available or not and then having a huge diff in this patch to do the 's/i/gpio/'. Ideally, everything would flow directly from the request callback and the SoC specific pinctrl driver would just tell the core code what all pins exist in hardware even if they're locked away and in use by something non-linux. That way, we don't have to rejig things in the SoC specific driver when the system configuration changes. I'm hoping we can do the valid mask part generically in the core pinctrl-msm.c file once so that things aren't spread around the SoC specific drivers and we solve ACPI and DT at the same time. We will want irq lines to be unallocated for things that aren't GPIOs, I'm not sure about ACPI and if it cares here, and we have a one to one mapping between irqs, GPIOs, and pinctrl pins with this hardware. Furthermore, we have the irq_valid_mask support in place already, so it seems that we can at least use the mask to be the one place where we indicate which pins are allowed to be used. And it seems like the simplest solution is to set the irq valid mask to be the GPIOs from the device property and then test that bitmask in the pinmux_ops structure's request callback so we cover both gpio (via the gpiochip_generic_request() -> pinmux_request_gpio() -> pin_request() path) and pinctrl (via the pin_request() path). Debugfs will need to test the mask too, but that should be simple. I believe you don't care about pin muxing, but it would make things work in both cases and will help if we want to limit access on platforms that use pin muxing. Let's resolve this by the end of this week. If this plan works we can have the revert patch for get_direction() and the pinctrl-msm.c patch to update the valid mask on gpiochip registration.
On 12/19/17 8:26 PM, Stephen Boyd wrote: > The thing that I don't like about this patch is that we're > modifying npins to indicate what gpios are available or not and > then having a huge diff in this patch to do the 's/i/gpio/'. Considering how small the driver is, I'm not sure if I'd say it's a "huge" diff. Frankly, I think this is a very elegant re-purposing of 'npins'. > Ideally, everything would flow directly from the request callback > and the SoC specific pinctrl driver would just tell the core code > what all pins exist in hardware even if they're locked away and > in use by something non-linux. So you want the request callback to propagated to the SOC driver? I guess that could work. > That way, we don't have to rejig > things in the SoC specific driver when the system configuration > changes. I'm hoping we can do the valid mask part generically in > the core pinctrl-msm.c file once so that things aren't spread > around the SoC specific drivers and we solve ACPI and DT at the > same time. Well, now I'm confused. First I thought you wanted to move the valid check into pinctrl-qdf2xxx, but now you say you want it done in pinctrl-msm, but isn't that what my patches are doing now? > We will want irq lines to be unallocated for things that aren't > GPIOs, I'm not sure about ACPI and if it cares here, and we have > a one to one mapping between irqs, GPIOs, and pinctrl pins with > this hardware. If the pin can't be requested, doesn't that take care of IRQ lines automatically? I don't touch the irq_valid_mask code. > Furthermore, we have the irq_valid_mask support in > place already, so it seems that we can at least use the mask to > be the one place where we indicate which pins are allowed to be > used. Well, I really didn't want to do that. Keep in mind that the root problem is getting pinctrl-qdf2xxx to be able to tell pinctrl-msm what pins are valid. That is the bulk of my code. I'm understanding you less and less the more I read. >And it seems like the simplest solution is to set the irq > valid mask to be the GPIOs from the device property and then test > that bitmask in the pinmux_ops structure's request callback so we > cover both gpio (via the gpiochip_generic_request() -> > pinmux_request_gpio() -> pin_request() path) and pinctrl (via the > pin_request() path). I do not understand that. To be honest, I think I already have the simplest solution, at least for ACPI. I don't really want to complicate my patches to support DT, since I don't really know what the DT-specific problems are. > Debugfs will need to test the mask too, but > that should be simple. I believe you don't care about pin muxing, > but it would make things work in both cases and will help if we > want to limit access on platforms that use pin muxing. I don't care about pin muxing, but my patches already take care of debugfs. > Let's resolve this by the end of this week. If this plan works we > can have the revert patch for get_direction() and the > pinctrl-msm.c patch to update the valid mask on gpiochip > registration. Frankly, I thought I had everything resolved already, and it sounds like you want me to start over from scratch anyway.
On 12/19, Timur Tabi wrote: > Frankly, I thought I had everything resolved already, and it sounds > like you want me to start over from scratch anyway. > Here's the patch. I get a hang when dumping debugfs, but at least sysfs expose fails when trying to request blocked gpios. I need to check if we need to say "yes" to pins that are above the gpio max for pinctrl. I'll do that tomorrow. --- drivers/gpio/gpiolib.c | 4 +- drivers/pinctrl/qcom/pinctrl-msm.c | 98 ++++++++++++++++++++++++++++++++++++-- include/linux/gpio/driver.h | 3 ++ 3 files changed, 99 insertions(+), 6 deletions(-) diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 8db2680bf872..5f118f044caa 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -1475,8 +1475,8 @@ static void gpiochip_irqchip_free_valid_mask(struct gpio_chip *gpiochip) gpiochip->irq_valid_mask = NULL; } -static bool gpiochip_irqchip_irq_valid(const struct gpio_chip *gpiochip, - unsigned int offset) +bool gpiochip_irqchip_irq_valid(const struct gpio_chip *gpiochip, + unsigned int offset) { /* No mask means all valid */ if (likely(!gpiochip->irq_valid_mask)) diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c index 273badd92561..4c2ce1f7d449 100644 --- a/drivers/pinctrl/qcom/pinctrl-msm.c +++ b/drivers/pinctrl/qcom/pinctrl-msm.c @@ -105,6 +105,17 @@ static const struct pinctrl_ops msm_pinctrl_ops = { .dt_free_map = pinctrl_utils_free_map, }; +static int msm_pinmux_request(struct pinctrl_dev *pctldev, unsigned offset) +{ + struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev); + struct gpio_chip *chip = &pctrl->chip; + + if (gpiochip_irqchip_irq_valid(chip, offset)) + return 0; + + return -EINVAL; +} + static int msm_get_functions_count(struct pinctrl_dev *pctldev) { struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev); @@ -166,6 +177,7 @@ static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev, } static const struct pinmux_ops msm_pinmux_ops = { + .request = msm_pinmux_request, .get_functions_count = msm_get_functions_count, .get_function_name = msm_get_function_name, .get_function_groups = msm_get_function_groups, @@ -493,6 +505,9 @@ static void msm_gpio_dbg_show_one(struct seq_file *s, "pull up" }; + if (!gpiochip_irqchip_irq_valid(chip, offset)) + return; + g = &pctrl->soc->groups[offset]; ctl_reg = readl(pctrl->regs + g->ctl_reg); @@ -503,7 +518,7 @@ static void msm_gpio_dbg_show_one(struct seq_file *s, seq_printf(s, " %-8s: %-3s %d", g->name, is_out ? "out" : "in", func); seq_printf(s, " %dmA", msm_regval_to_drive(drive)); - seq_printf(s, " %s", pulls[pull]); + seq_printf(s, " %s\n", pulls[pull]); } static void msm_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip) @@ -511,10 +526,8 @@ static void msm_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip) unsigned gpio = chip->base; unsigned i; - for (i = 0; i < chip->ngpio; i++, gpio++) { + for (i = 0; i < chip->ngpio; i++, gpio++) msm_gpio_dbg_show_one(s, NULL, chip, i, gpio); - seq_puts(s, "\n"); - } } #else @@ -795,6 +808,76 @@ static void msm_gpio_irq_handler(struct irq_desc *desc) chained_irq_exit(chip, desc); } +static int msm_gpio_init_irq_valid_mask(struct gpio_chip *chip, + struct msm_pinctrl *pctrl) +{ + int ret; + unsigned int len, i; + unsigned int max_gpios = pctrl->soc->ngpios; + struct device_node *np = pctrl->dev->of_node; + + /* The number of GPIOs in the ACPI tables */ + ret = device_property_read_u16_array(pctrl->dev, "gpios", NULL, 0); + if (ret > 0 && ret < max_gpios) { + u16 *tmp; + + len = ret; + tmp = kmalloc_array(len, sizeof(tmp[0]), GFP_KERNEL); + if (!tmp) + return -ENOMEM; + + ret = device_property_read_u16_array(pctrl->dev, "gpios", tmp, + len); + if (ret < 0) { + dev_err(pctrl->dev, "could not read list of GPIOs\n"); + kfree(tmp); + return ret; + } + + bitmap_zero(chip->irq_valid_mask, max_gpios); + for (i = 0; i < len; i++) + set_bit(tmp[i], chip->irq_valid_mask); + + return 0; + } + + /* If there's a DT ngpios-ranges property then add those ranges */ + ret = of_property_count_u32_elems(np, "ngpios-ranges"); + if (ret > 0 && ret % 2 == 0 && ret / 2 < max_gpios) { + u32 start; + u32 count; + + len = ret / 2; + bitmap_zero(chip->irq_valid_mask, max_gpios); + + for (i = 0; i < len; i++) { + of_property_read_u32_index(np, "ngpios-ranges", + i * 2, &start); + of_property_read_u32_index(np, "ngpios-ranges", + i * 2 + 1, &count); + bitmap_set(chip->irq_valid_mask, start, count); + } + } + + return 0; +} + +static bool msm_gpio_needs_irq_valid_mask(struct msm_pinctrl *pctrl) +{ + int ret; + struct device_node *np = pctrl->dev->of_node; + + ret = device_property_read_u16_array(pctrl->dev, "gpios", NULL, 0); + if (ret > 0) + return true; + + ret = of_property_count_u32_elems(np, "ngpios-ranges"); + if (ret > 0 && ret % 2 == 0) + return true; + + return false; +} + static int msm_gpio_init(struct msm_pinctrl *pctrl) { struct gpio_chip *chip; @@ -811,6 +894,7 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl) chip->parent = pctrl->dev; chip->owner = THIS_MODULE; chip->of_node = pctrl->dev->of_node; + chip->irq_need_valid_mask = msm_gpio_needs_irq_valid_mask(pctrl); ret = gpiochip_add_data(&pctrl->chip, pctrl); if (ret) { @@ -818,6 +902,12 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl) return ret; } + ret = msm_gpio_init_irq_valid_mask(chip, pctrl); + if (ret) { + dev_err(pctrl->dev, "Failed to setup irq valid bits\n"); + return ret; + } + ret = gpiochip_add_pin_range(&pctrl->chip, dev_name(pctrl->dev), 0, 0, chip->ngpio); if (ret) { dev_err(pctrl->dev, "Failed to add pin range\n"); diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h index af20369ec8e7..be977c1c7498 100644 --- a/include/linux/gpio/driver.h +++ b/include/linux/gpio/driver.h @@ -262,6 +262,9 @@ int gpiochip_irqchip_add_key(struct gpio_chip *gpiochip, bool nested, struct lock_class_key *lock_key); +bool gpiochip_irqchip_irq_valid(const struct gpio_chip *gpiochip, + unsigned int offset); + #ifdef CONFIG_LOCKDEP /*
On 12/20/2017 02:15 AM, Stephen Boyd wrote: > Here's the patch. I get a hang when dumping debugfs, but at least > sysfs expose fails when trying to request blocked gpios. I need > to check if we need to say "yes" to pins that are above the gpio > max for pinctrl. I'll do that tomorrow. Sorry, I just don't see how this is better than my patches. I don't understand the need for involving the IRQ valid mask. I also don't see the value in adding code to look for a property that exists only in one ACPI HID (QCOM8002) as if it were generic. The "num-gpios" and "gpios" DSDs are not supposed to exist in any other HID, so there should be no code that reads it in pinctrl-msm. I'm going on vacation soon. I will post a v11 that eliminates support for QCOM8001. Maybe that version is "good enough" for now and you can add DT and/or IRQ support on top of it.
On 12/20, Timur Tabi wrote: > On 12/20/2017 02:15 AM, Stephen Boyd wrote: > >Here's the patch. I get a hang when dumping debugfs, but at least > >sysfs expose fails when trying to request blocked gpios. I need > >to check if we need to say "yes" to pins that are above the gpio > >max for pinctrl. I'll do that tomorrow. > > Sorry, I just don't see how this is better than my patches. I don't > understand the need for involving the IRQ valid mask. I also don't > see the value in adding code to look for a property that exists only > in one ACPI HID (QCOM8002) as if it were generic. The "num-gpios" > and "gpios" DSDs are not supposed to exist in any other HID, so > there should be no code that reads it in pinctrl-msm. I don't see how it hurts to treat it generically. Presumably that's the way it will be done on ACPI platforms going forward? No need to tie it to some ACPI HID. I'm trying to resolve everything at once: gpios, pinctrl pins, and irqs exposed by the TLMM hardware. The value is that we solve it all, once, now. The DT binding can also be resolved at the same time, so when we need to express this in DT it's already done. Otherwise, something can request irqs from the irqdomain even if the irq can't be enabled, or it can try to mux the pin to some other function, even if the function selection can't be configured. Boiling everything down into the irq valid mask should cover all these cases, and not require us to strip const from all the data in the non-ACPI pinctrl drivers to replace the value in the npins field at runtime.
On 12/20/17 6:39 PM, Stephen Boyd wrote: > I don't see how it hurts to treat it generically. Presumably > that's the way it will be done on ACPI platforms going forward? > No need to tie it to some ACPI HID. But it is tied to a HID. The "num-gpios" and "gpios" properties belong to a specific HID. Someone could create a new HID with different properties, and then what? That's why I want all the ACPI stuff in the client driver. At this point I don't really care any more about what the patches look like, but I really do think that putting the ACPI code in pinctrl-msm is a bad idea. We're debating adding support for multiple TLMMs, and we may create a new HID for that, so that we can define all pins on all TLMMs in one device. We would need to create a new HID and new DSDs to go with it. > I'm trying to resolve everything at once: gpios, pinctrl pins, > and irqs exposed by the TLMM hardware. The value is that we solve > it all, once, now. Keep in mind that I am now in vacation, and so I won't be able to submit any more patches for a while. > The DT binding can also be resolved at the > same time, so when we need to express this in DT it's already > done. Ok. > Otherwise, something can request irqs from the irqdomain > even if the irq can't be enabled, or it can try to mux the pin to > some other function, even if the function selection can't be > configured. Is it possible to request an IRQ for a pin if the pin itself can't be requested? > Boiling everything down into the irq valid mask should cover all > these cases, and not require us to strip const from all the data > in the non-ACPI pinctrl drivers to replace the value in the npins > field at runtime. Ok.
On 12/20, Timur Tabi wrote: > On 12/20/17 6:39 PM, Stephen Boyd wrote: > >I don't see how it hurts to treat it generically. Presumably > >that's the way it will be done on ACPI platforms going forward? > >No need to tie it to some ACPI HID. > > But it is tied to a HID. The "num-gpios" and "gpios" properties > belong to a specific HID. Someone could create a new HID with > different properties, and then what? That's why I want all the ACPI > stuff in the client driver. > > At this point I don't really care any more about what the patches > look like, but I really do think that putting the ACPI code in > pinctrl-msm is a bad idea. > > We're debating adding support for multiple TLMMs, and we may create > a new HID for that, so that we can define all pins on all TLMMs in > one device. We would need to create a new HID and new DSDs to go > with it. Ok. That's testable with acpi_match_device_ids() though. I can add that into pinctrl-msm.c so we don't have to pass info about available gpios from ACPI specific driver into the pinctrl-msm core driver. That's why I'm trying to avoid doing it in the ACPI specific driver. Do it close to where the gpiochip is created instead. Maybe future HIDs could follow the DT design and then we can look for the same device property name in both firmwares. Parsing ranges is simpler. > > >I'm trying to resolve everything at once: gpios, pinctrl pins, > >and irqs exposed by the TLMM hardware. The value is that we solve > >it all, once, now. > > Keep in mind that I am now in vacation, and so I won't be able to > submit any more patches for a while. > > >The DT binding can also be resolved at the > >same time, so when we need to express this in DT it's already > >done. > > Ok. > > >Otherwise, something can request irqs from the irqdomain > >even if the irq can't be enabled, or it can try to mux the pin to > >some other function, even if the function selection can't be > >configured. > > Is it possible to request an IRQ for a pin if the pin itself can't > be requested? I don't see any place blocking GPIOs turning into IRQs once we setup the irqdomain with interrupts. Maybe I missed something, but I think you can request an IRQ once the domain has the hwirqs associated with it. I will test it out.
On 12/21/2017 07:46 PM, Stephen Boyd wrote: > Ok. That's testable with acpi_match_device_ids() though. I can > add that into pinctrl-msm.c so we don't have to pass info about > available gpios from ACPI specific driver into the pinctrl-msm > core driver. That's why I'm trying to avoid doing it in the ACPI > specific driver. Do it close to where the gpiochip is created > instead. I guess we're just going to have to agree to disagree. I see the DSDs as being SOC-specific, and therefore belong in the SOC driver. I'm still going to need an SOC driver to define the TLMM register layout. But as I said earlier, I've already spent way too much time working on a driver that, in all likelihood, never be used in any production system anyway. I look forward to reviewing the next version of your patch. > Maybe future HIDs could follow the DT design and then we can look > for the same device property name in both firmwares. DSDs generally don't have the vendor prefix that DT properties do. > Parsing > ranges is simpler. I'm not sure I agree with that.
On Thu, 2018-01-04 at 09:46 -0600, Timur Tabi wrote: > On 12/21/2017 07:46 PM, Stephen Boyd wrote: > > > > Maybe future HIDs could follow the DT design and then we can look > > for the same device property name in both firmwares. > > DSDs generally don't have the vendor prefix that DT properties do. There are more means to check hardware revisions: HID - Hardware ID CID - Compatible ID UID - Unique ID (good to distinguish instances of the same device on the board) _HRV - Hardware Revision (6.1.6 describes this one) Everything is described in the spec. Does anybody care to read? P.S. More I reading this thread more I become thinking that people screw ACPI use in many ways...
On Thu, Jan 4, 2018 at 5:04 PM, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Thu, 2018-01-04 at 09:46 -0600, Timur Tabi wrote: >> On 12/21/2017 07:46 PM, Stephen Boyd wrote: >> > >> > Maybe future HIDs could follow the DT design and then we can look >> > for the same device property name in both firmwares. >> >> DSDs generally don't have the vendor prefix that DT properties do. > > There are more means to check hardware revisions: > HID - Hardware ID > CID - Compatible ID > UID - Unique ID (good to distinguish instances of the same device on the > board) > _HRV - Hardware Revision (6.1.6 describes this one) Thanks Andy. I expect a patch using these features, also include Andy on CC as it appears he knows how this should be done. (In difference from me...) I'm pretty much relying on other people to understand ACPI for me, maybe I should attend a course or something. Yours, Linus Walleij -- 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
diff --git a/drivers/pinctrl/qcom/pinctrl-qdf2xxx.c b/drivers/pinctrl/qcom/pinctrl-qdf2xxx.c index bb3ce5c3e18b..deb08e08e86d 100644 --- a/drivers/pinctrl/qcom/pinctrl-qdf2xxx.c +++ b/drivers/pinctrl/qcom/pinctrl-qdf2xxx.c @@ -38,68 +38,139 @@ /* maximum size of each gpio name (enough room for "gpioXXX" + null) */ #define NAME_SIZE 8 +enum { + QDF2XXX_V1, + QDF2XXX_V2, +}; + static int qdf2xxx_pinctrl_probe(struct platform_device *pdev) { + const struct acpi_device_id *id; struct pinctrl_pin_desc *pins; struct msm_pingroup *groups; char (*names)[NAME_SIZE]; unsigned int i; u32 num_gpios; + unsigned int avail_gpios; /* The number of GPIOs we support */ + u16 *gpios; /* An array of supported GPIOs */ int ret; /* Query the number of GPIOs from ACPI */ ret = device_property_read_u32(&pdev->dev, "num-gpios", &num_gpios); if (ret < 0) { - dev_warn(&pdev->dev, "missing num-gpios property\n"); + dev_err(&pdev->dev, "missing 'num-gpios' property\n"); return ret; } - if (!num_gpios || num_gpios > MAX_GPIOS) { - dev_warn(&pdev->dev, "invalid num-gpios property\n"); + dev_err(&pdev->dev, "invalid 'num-gpios' property\n"); return -ENODEV; } + /* + * The QCOM8001 HID contains only the number of GPIOs, and assumes + * that all of them are available. avail_gpios is the same as num_gpios. + * + * The QCOM8002 HID introduces the 'gpios' DSD, which lists + * specific GPIOs that the driver is allowed to access. + * + * The make the common code simpler, in both cases we create an + * array of GPIOs that are accessible. So for QCOM8001, that would + * be all of the GPIOs. + */ + id = acpi_match_device(pdev->dev.driver->acpi_match_table, &pdev->dev); + + if (id->driver_data == QDF2XXX_V1) { + avail_gpios = num_gpios; + + gpios = devm_kmalloc_array(&pdev->dev, avail_gpios, + sizeof(gpios[0]), GFP_KERNEL); + if (!gpios) + return -ENOMEM; + + for (i = 0; i < avail_gpios; i++) + gpios[i] = i; + } else { + /* The number of GPIOs in the approved list */ + ret = device_property_read_u16_array(&pdev->dev, "gpios", + NULL, 0); + if (ret < 0) { + dev_err(&pdev->dev, "missing 'gpios' property\n"); + return ret; + } + /* + * The number of available GPIOs should be non-zero, and no + * more than the total number of GPIOS. + */ + if (!ret || ret > num_gpios) { + dev_err(&pdev->dev, "invalid 'gpios' property\n"); + return -ENODEV; + } + avail_gpios = ret; + + gpios = devm_kmalloc_array(&pdev->dev, avail_gpios, + sizeof(gpios[0]), GFP_KERNEL); + if (!gpios) + return -ENOMEM; + + ret = device_property_read_u16_array(&pdev->dev, "gpios", gpios, + avail_gpios); + if (ret < 0) { + dev_err(&pdev->dev, "could not read list of GPIOs\n"); + return ret; + } + } + pins = devm_kcalloc(&pdev->dev, num_gpios, sizeof(struct pinctrl_pin_desc), GFP_KERNEL); groups = devm_kcalloc(&pdev->dev, num_gpios, sizeof(struct msm_pingroup), GFP_KERNEL); - names = devm_kcalloc(&pdev->dev, num_gpios, NAME_SIZE, GFP_KERNEL); + names = devm_kcalloc(&pdev->dev, avail_gpios, NAME_SIZE, GFP_KERNEL); if (!pins || !groups || !names) return -ENOMEM; + /* + * Initialize the array. GPIOs not listed in the 'gpios' array + * still need a number, but nothing else. + */ for (i = 0; i < num_gpios; i++) { - snprintf(names[i], NAME_SIZE, "gpio%u", i); - pins[i].number = i; - pins[i].name = names[i]; - - groups[i].npins = 1; - groups[i].name = names[i]; groups[i].pins = &pins[i].number; + } - groups[i].ctl_reg = 0x10000 * i; - groups[i].io_reg = 0x04 + 0x10000 * i; - groups[i].intr_cfg_reg = 0x08 + 0x10000 * i; - groups[i].intr_status_reg = 0x0c + 0x10000 * i; - groups[i].intr_target_reg = 0x08 + 0x10000 * i; - - groups[i].mux_bit = 2; - groups[i].pull_bit = 0; - groups[i].drv_bit = 6; - groups[i].oe_bit = 9; - groups[i].in_bit = 0; - groups[i].out_bit = 1; - groups[i].intr_enable_bit = 0; - groups[i].intr_status_bit = 0; - groups[i].intr_target_bit = 5; - groups[i].intr_target_kpss_val = 1; - groups[i].intr_raw_status_bit = 4; - groups[i].intr_polarity_bit = 1; - groups[i].intr_detection_bit = 2; - groups[i].intr_detection_width = 2; + /* Populate the entries that are meant to be exposes as GPIOs. */ + for (i = 0; i < avail_gpios; i++) { + unsigned int gpio = gpios[i]; + + groups[gpio].npins = 1; + snprintf(names[i], NAME_SIZE, "gpio%u", gpio); + pins[gpio].name = names[i]; + groups[gpio].name = names[i]; + + groups[gpio].ctl_reg = 0x10000 * gpio; + groups[gpio].io_reg = 0x04 + 0x10000 * gpio; + groups[gpio].intr_cfg_reg = 0x08 + 0x10000 * gpio; + groups[gpio].intr_status_reg = 0x0c + 0x10000 * gpio; + groups[gpio].intr_target_reg = 0x08 + 0x10000 * gpio; + + groups[gpio].mux_bit = 2; + groups[gpio].pull_bit = 0; + groups[gpio].drv_bit = 6; + groups[gpio].oe_bit = 9; + groups[gpio].in_bit = 0; + groups[gpio].out_bit = 1; + groups[gpio].intr_enable_bit = 0; + groups[gpio].intr_status_bit = 0; + groups[gpio].intr_target_bit = 5; + groups[gpio].intr_target_kpss_val = 1; + groups[gpio].intr_raw_status_bit = 4; + groups[gpio].intr_polarity_bit = 1; + groups[gpio].intr_detection_bit = 2; + groups[gpio].intr_detection_width = 2; } + devm_kfree(&pdev->dev, gpios); + qdf2xxx_pinctrl.pins = pins; qdf2xxx_pinctrl.groups = groups; qdf2xxx_pinctrl.npins = num_gpios; @@ -110,7 +181,8 @@ static int qdf2xxx_pinctrl_probe(struct platform_device *pdev) } static const struct acpi_device_id qdf2xxx_acpi_ids[] = { - {"QCOM8001"}, + {"QCOM8001", QDF2XXX_V1}, + {"QCOM8002", QDF2XXX_V2}, {}, }; MODULE_DEVICE_TABLE(acpi, qdf2xxx_acpi_ids);
Newer versions of the firmware for the Qualcomm Datacenter Technologies QDF2400 restricts access to a subset of the GPIOs on the TLMM. To prevent older kernels from accidentally accessing the restricted GPIOs, we change the ACPI HID for the TLMM block from QCOM8001 to QCOM8002, and introduce a new property "gpios". This property is an array of specific GPIOs that are accessible. When an older kernel boots on newer (restricted) firmware, it will fail to probe. To implement the sparse GPIO map, we register all of the GPIOs, but set the pin count for the unavailable GPIOs to zero. The pinctrl-msm driver will block those unavailable GPIOs from being accessed. To allow newer kernels to support older firmware, the driver retains support for QCOM8001. Signed-off-by: Timur Tabi <timur@codeaurora.org> --- drivers/pinctrl/qcom/pinctrl-qdf2xxx.c | 134 +++++++++++++++++++++++++-------- 1 file changed, 103 insertions(+), 31 deletions(-)