Message ID | 1512170904-4749-5-git-send-email-timur@codeaurora.org (mailing list archive) |
---|---|
State | Not Applicable, archived |
Delegated to: | Andy Gross |
Headers | show |
On Fri, 2017-12-01 at 17:28 -0600, 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. > +static const struct acpi_device_id qdf2xxx_acpi_ids[] = { > + {"QCOM8001", QDF2XXX_V1}, > + {"QCOM8002", QDF2XXX_V2}, > + {}, > +}; > +MODULE_DEVICE_TABLE(acpi, qdf2xxx_acpi_ids); > > + const struct acpi_device_id *id = > + acpi_match_device(qdf2xxx_acpi_ids, &pdev->dev); JFYI: there is no need to move IDs like this. Use members of struct device_driver wisely. > -static const struct acpi_device_id qdf2xxx_acpi_ids[] = { > - {"QCOM8001"}, > - {}, > -}; > -MODULE_DEVICE_TABLE(acpi, qdf2xxx_acpi_ids);
On Sat, Dec 2, 2017 at 12:28 AM, Timur Tabi <timur@codeaurora.org> wrote: > /* 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; > } It's unfortunate that this driver uses the undocumented "num-gpios" when the device tree bindings already has standardized "ngpios" as the name for this. Maybe it was not standardized back in 2015 when this driver was merged. Or we were all sloppy :/ > + /* 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; > + } This is in direct conflict with the existing "gpios" binding in device tree. Where is this name coming from? ACPI standards? If device tree and ACPI start defining things which are in direct conflict we can just shut down this device_property() business altogether, it will never work that way. I would try to merge a DT bindings doc defining "valid-gpios" or something like this, can we proceed like that? Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2017-12-12 at 11:42 +0100, Linus Walleij wrote: > On Sat, Dec 2, 2017 at 12:28 AM, Timur Tabi <timur@codeaurora.org> > wrote: > > + /* 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; > > + } > > This is in direct conflict with the existing "gpios" binding in device > tree. > > Where is this name coming from? ACPI standards? Not ACPI standards as of my knowledge. ACPI standard defines a common scheme how to define properties, it doesn't tell anything about property names or any mappings between names to values or names to "OS subsystem"). As for GPIO we just follow *de facto* what DT has right now, i.e. "xxx- gpio" or "xxx-gpios" pattern is used to map ACPI standard resource to a GPIO name. That's how GPIO ACPI lib is being developed. > If device tree and ACPI start defining things which are in direct > conflict > we can just shut down this device_property() business altogether, > it will never work that way. This is fully understandable. Also it works in other direction, i.e. if DT will break the established thing it will break also ACPI and built-in device properties. We are keeping an eye on this not to happen as much as we can in any direction. So, summarize above, I don't see any impediments (except maybe very broken ARM64 firmware that is already on devices on market) to make it properly from the beginning.
On 12/12/2017 04:05 AM, Andy Shevchenko wrote: >> +static const struct acpi_device_id qdf2xxx_acpi_ids[] = { >> + {"QCOM8001", QDF2XXX_V1}, >> + {"QCOM8002", QDF2XXX_V2}, >> + {}, >> +}; >> +MODULE_DEVICE_TABLE(acpi, qdf2xxx_acpi_ids); >> >> + const struct acpi_device_id *id = >> + acpi_match_device(qdf2xxx_acpi_ids, &pdev->dev); > JFYI: there is no need to move IDs like this. > Use members of struct device_driver wisely. I have to move it, otherwise I get: drivers/pinctrl/qcom/pinctrl-qdf2xxx.c:49:21: error: 'qdf2xxx_acpi_ids' undeclared (first use in this function); did you mean 'qdf2xxx_pinctrl'? I reference the structure in qdf2xxx_pinctrl_probe().
On 12/12/2017 05:07 AM, Andy Shevchenko wrote: > Not ACPI standards as of my knowledge. ACPI standard defines a common > scheme how to define properties, it doesn't tell anything about property > names or any mappings between names to values or names to "OS > subsystem"). There was an attempt a while back to standardize this like we do for device tree, but it fell apart. Device-specific ACPI-only properties are not standarized. This driver is initialized only on ACPI systems. It has no device tree binding. > As for GPIO we just follow *de facto* what DT has right now, i.e. "xxx- > gpio" or "xxx-gpios" pattern is used to map ACPI standard resource to a > GPIO name. That's how GPIO ACPI lib is being developed. GPIOs in device tree are defined completely differently than in ACPI. On DT, the kernel controls the pin muxing. On ACPI, pins are muxed by firmware and never re-muxed by the operating system. So all this driver does is expose a few pins as simple GPIOs.
On Tue, 2017-12-12 at 14:17 -0600, Timur Tabi wrote: > On 12/12/2017 04:05 AM, Andy Shevchenko wrote: > > > +static const struct acpi_device_id qdf2xxx_acpi_ids[] = { > > > + {"QCOM8001", QDF2XXX_V1}, > > > + {"QCOM8002", QDF2XXX_V2}, > > > + {}, > > > +}; > > > +MODULE_DEVICE_TABLE(acpi, qdf2xxx_acpi_ids); > > > > > > + const struct acpi_device_id *id = > > > + acpi_match_device(qdf2xxx_acpi_ids, &pdev->dev); > > > > JFYI: there is no need to move IDs like this. > > Use members of struct device_driver wisely. > > I have to move it, otherwise I get: > > drivers/pinctrl/qcom/pinctrl-qdf2xxx.c:49:21: error: > 'qdf2xxx_acpi_ids' > undeclared (first use in this function); did you mean > 'qdf2xxx_pinctrl'? > > I reference the structure in qdf2xxx_pinctrl_probe(). Please, read my comment again. The key part of the phrase: "Use members of struct device_driver" So, do not move the IDs. There are examples in the kernel how to access it.
On Tue, 2017-12-12 at 14:27 -0600, Timur Tabi wrote: > On 12/12/2017 05:07 AM, Andy Shevchenko wrote: > > > Not ACPI standards as of my knowledge. ACPI standard defines a > > common > > scheme how to define properties, it doesn't tell anything about > > property > > names or any mappings between names to values or names to "OS > > subsystem"). > > There was an attempt a while back to standardize this like we do for > device tree, but it fell apart. Device-specific ACPI-only properties > are not standarized. This driver is initialized only on ACPI > systems. > It has no device tree binding. It should follow DT *de facto* standard bindings like "ngpios" (though it's not needed in ACPI case IIRC) and other properties. > > As for GPIO we just follow *de facto* what DT has right now, i.e. > > "xxx- > > gpio" or "xxx-gpios" pattern is used to map ACPI standard resource > > to a > > GPIO name. That's how GPIO ACPI lib is being developed. > > GPIOs in device tree are defined completely differently than in ACPI. > On DT, the kernel controls the pin muxing. On ACPI, pins are muxed > by > firmware and never re-muxed by the operating system. So all this > driver > does is expose a few pins as simple GPIOs. Wait, runtime muxing is a matter of requesting another function (usually GPIO) and putting it back afterwards. Do you really need anything like this at *runtime*? Pin control design is not compatible with hardware (too abstract), but that is the problem of DT as well: I'm referring here to not carefully designed so called "pin states". This is another story and has nothing specific for ACPI.
On 12/13/2017 08:32 AM, Andy Shevchenko wrote: > Please, read my comment again. The key part of the phrase: > "Use members of struct device_driver" > > So, do not move the IDs. There are examples in the kernel how to access > it. Sorry, I don't understand what you're talking about. I don't see how I can call const struct acpi_device_id *id = acpi_match_device(qdf2xxx_acpi_ids, &pdev->dev); without having qdf2xxx_acpi_ids already defined previously. And without the 'id', I can't figure out whether I've probed via QCOM8001 or QCOM8002.
On 12/13/2017 08:36 AM, Andy Shevchenko wrote: > Wait, runtime muxing is a matter of requesting another function (usually > GPIO) and putting it back afterwards. Do you really need anything like > this at*runtime*? No, there is no runtime muxing on ACPI platforms.
On 12/13/2017 08:46 AM, Timur Tabi wrote: > >> Please, read my comment again. The key part of the phrase: >> "Use members of struct device_driver" >> >> So, do not move the IDs. There are examples in the kernel how to access >> it. > > Sorry, I don't understand what you're talking about. I don't see how I > can call > > const struct acpi_device_id *id = > acpi_match_device(qdf2xxx_acpi_ids, &pdev->dev); > > without having qdf2xxx_acpi_ids already defined previously. And without > the 'id', I can't figure out whether I've probed via QCOM8001 or QCOM8002. I think I found it. Are you talking about doing this instead: id = acpi_match_device(pdev->dev.driver->acpi_match_table, &pdev->dev);
On Wed, 2017-12-13 at 09:18 -0600, Timur Tabi wrote: > On 12/13/2017 08:46 AM, Timur Tabi wrote: > I think I found it. Are you talking about doing this instead: > > id = acpi_match_device(pdev->dev.driver->acpi_match_table, &pdev- > >dev); Precisely!
diff --git a/drivers/pinctrl/qcom/pinctrl-qdf2xxx.c b/drivers/pinctrl/qcom/pinctrl-qdf2xxx.c index bb3ce5c3e18b..fa39b0eb329d 100644 --- a/drivers/pinctrl/qcom/pinctrl-qdf2xxx.c +++ b/drivers/pinctrl/qcom/pinctrl-qdf2xxx.c @@ -38,68 +38,151 @@ /* maximum size of each gpio name (enough room for "gpioXXX" + null) */ #define NAME_SIZE 8 +enum { + QDF2XXX_V1, + QDF2XXX_V2, +}; + +static const struct acpi_device_id qdf2xxx_acpi_ids[] = { + {"QCOM8001", QDF2XXX_V1}, + {"QCOM8002", QDF2XXX_V2}, + {}, +}; +MODULE_DEVICE_TABLE(acpi, qdf2xxx_acpi_ids); + static int qdf2xxx_pinctrl_probe(struct platform_device *pdev) { + const struct acpi_device_id *id = + acpi_match_device(qdf2xxx_acpi_ids, &pdev->dev); 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. + */ + 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; + } + + /* + * Because we have a specific list of GPIOs, the GPIO map + * is 'sparse'. + */ + qdf2xxx_pinctrl.sparse = true; + } + 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; @@ -109,12 +192,6 @@ static int qdf2xxx_pinctrl_probe(struct platform_device *pdev) return msm_pinctrl_probe(pdev, &qdf2xxx_pinctrl); } -static const struct acpi_device_id qdf2xxx_acpi_ids[] = { - {"QCOM8001"}, - {}, -}; -MODULE_DEVICE_TABLE(acpi, qdf2xxx_acpi_ids); - static struct platform_driver qdf2xxx_pinctrl_driver = { .driver = { .name = "qdf2xxx-pinctrl",
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 | 149 +++++++++++++++++++++++++-------- 1 file changed, 113 insertions(+), 36 deletions(-)