lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Fri, 17 Feb 2017 17:05:17 +0100 From: Fabrice Gasnier <fabrice.gasnier@...com> To: Jonathan Cameron <jic23@...nel.org>, Linus Walleij <linus.walleij@...aro.org> CC: Russell King <linux@...linux.org.uk>, Rob Herring <robh+dt@...nel.org>, "linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>, "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>, "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>, Mark Rutland <mark.rutland@....com>, Maxime Coquelin <mcoquelin.stm32@...il.com>, Alexandre TORGUE <alexandre.torgue@...com>, Lars-Peter Clausen <lars@...afoo.de>, Hartmut Knaack <knaack.h@....de>, Peter Meerwald <pmeerw@...erw.net>, Benjamin Gaignard <benjamin.gaignard@...aro.org>, Benjamin Gaignard <benjamin.gaignard@...com>, <tglx@...utronix.de> Subject: Re: [PATCH v2 4/5] iio: trigger: add support for STM32 EXTI triggers On 02/11/2017 11:29 AM, Jonathan Cameron wrote: > On 06/02/17 16:01, Fabrice Gasnier wrote: >> On 02/04/2017 12:39 PM, Jonathan Cameron wrote: >>> On 03/02/17 19:40, Linus Walleij wrote: >>>> On Mon, Jan 30, 2017 at 3:33 PM, Fabrice Gasnier <fabrice.gasnier@...com> wrote: >>>> >>>>> EXTi[0..15] gpio signal can be routed internally as trigger source for >>>>> ADC or DAC conversions. Configure them as interrupts to configure >>>>> trigger path in HW. >>>>> >>>>> Note: interrupt handler isn't required here, and corresponding interrupt >>>>> can be kept masked at exti controller level. >>>>> >>>>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@...com> >>>> >>>> But I see nothing STM32-specific about this driver? >>>> >>>> I think we should do everone a service and just create >>>> drivers/iio/trigger/gpio-trigger.c >>>> >>>> I wondered before why we don't have a generic GPIO IIO trigger, >>>> it seems like such an intuitive and useful thing to have. >>> We do, it just got renamed at some point a while back to be >>> iio-trig-interrupt after it became clear that it didn't need >>> to be a gpio either - just an interrupt. Can't remember which >>> part provided a non gpio interrupt pin and hence drove that >>> change. Was quite a while back! >>> d4fd73bf25c3aafc891ef4443fc744d427ec1df0 specifically in 2013 >>> >>> Handling of the gpio stuff should be handled in the interrupt >>> description itself. >>> >>> However, it's a bit different - in that in the below it >>> would be the equivalent of triggering on the unused exti >>> interrupt rather than on the end of conversion. >>> >>> In this case, because of the hardware linkage we can effectively >>> skip the first interrupt. >>> >>> Arguably to make this a general purpose trigger we should enable >>> that interrupt if anything other than the STM devices that can >>> use it in hardware are hooked on to it. >>> >>> So this is an interrupt trigger without the interrupt ever >>> being visible to software. >>> >>> It might be easy enough to add that support to the generic version >>> except that linking said trigger requires some register changes >>> in the STM side. + there is a kicker in the various last bit >>> of this patch - we need a way to find out if it's the interrupt >>> we think it is (i.e. an exti interrupt) >> >> Hi Jonathan, Linus, all, >> >> First, many thanks for reviewing. >> >> In this patch-set, I choose to implement this hardware trigger line >> into separate driver... Thinking out loud: >> If I try to summarize, as you perfectly describe here before, I see >> two items to address: - this is pure HW line, that can either >> generate interrupts, and/or start conversions in HW. This may be hard >> to combine both, an interrupt handler to call iio_trigger_poll() from >> there, for generic devices, but not for stm devices (not sure if this >> can benefit to others?). > Wouldn't have thought it was that hard to do, but fair enough + it > can definitely be added later if anyone cares. Hi Jonathan, I hope I'd give some more trials ;-). > > - there is need to do some register changes >> on stm device side (ADC) as well, when choosing a particular trigger >> (EXTI line for instance) e.g. in validate_trigger(). >> >> I'm starting to wonder if this can be separate driver. Maybe this >> should be part of device driver (e.g. ADC), at least for the time being. > Sure, if it ends up easier that way then do so. We don't really have > support yet for triggered DACs anyway and as you say this could be > modified later - subject to the need to end up with device tree bindings > that make sense in that case as well. I've just posted an RFC, to add device tree bindings for trigger. "iio: trigger: Add OF support and GPIO based trigger" I think this is a prerequisite, trying to get more generic implementation. >> >>>> >>>> Let's see what Jonathan says. >>> >>> >>>> >>>>> +config IIO_STM32_EXTI_TRIGGER >>>>> + tristate "STM32 EXTI Trigger" >>>>> + depends on (ARCH_STM32 && OF) || COMPILE_TEST >>>> >>>> config IIO_GPIO_TRIGGER >>>> depends on GPIOLIB >>>> >>>>> + select STM32_EXTI >>>> >>>> Isn't the dependency actually the other way around? >>>> >>>> default STM32_EXTI makes more sense, or just put it into the >>>> defconfig. >>>> >>>>> +#include <linux/gpio/consumer.h> >>>>> +#include <linux/iio/iio.h> >>>>> +#include <linux/iio/trigger.h> >>>>> +#include <linux/interrupt.h> >>>>> +#include <linux/module.h> >>>>> +#include <linux/platform_device.h> >>>>> + >>>>> +/* STM32 has up to 16 EXTI triggers on GPIOs */ >>>>> +#define STM32_MAX_EXTI_TRIGGER 16 >>>> >>>> Just don't put any restrictions like this so it can be widely >>>> reused. >>>> >>>>> +static irqreturn_t stm32_exti_trigger_handler(int irq, void *data) >>>>> +{ >>>>> + /* Exti handler shouldn't be invoked, and isn't used */ >>>>> + return IRQ_HANDLED; >>>>> +} >>>> >>>> It could be a good idea to capture the timestamp here if we were >>>> actually using this IRQ. >>>> >>>>> +static int stm32_exti_trigger_probe(struct platform_device *pdev) >>>>> +{ >>>>> + int irq, ret; >>>>> + char name[8]; >>>>> + struct gpio_desc *gpio; >>>>> + struct iio_trigger *trig; >>>>> + unsigned int i; >>>>> + >>>>> + for (i = 0; i < STM32_MAX_EXTI_TRIGGER; i++) { >>>> >>>> Why not just run this until devm_gpiod_get() returns -ERRNO >>>> or something? >>>> >>>>> + snprintf(name, sizeof(name), "exti%d", i); >>>>> + >>>>> + gpio = devm_gpiod_get_optional(&pdev->dev, name, GPIOD_IN); >>>> >>>> Why would it be optional? >>>> >>>> Either it is there in the device tree or we get -EINVAL or something >>>> if there is no >>>> such index in the device tree. We can get -EPROBE_DEEER too, and then >>>> we should exit silently or just print that deferring is happening. >>>> >>>>> + if (IS_ERR_OR_NULL(gpio)) { >>>>> + if (IS_ERR(gpio)) { >>>>> + dev_err(&pdev->dev, "gpio %s get error %ld\n", >>>>> + name, PTR_ERR(gpio)); >>>>> + return PTR_ERR(gpio); >>>>> + } >>>>> + dev_dbg(&pdev->dev, "No %s gpio\n", name); >>>>> + continue; >>>>> + } >>>> >>>> Good >>>> >>>>> + irq = gpiod_to_irq(gpio); >>>>> + if (irq < 0) { >>>>> + dev_err(&pdev->dev, "gpio %d to irq failed\n", i); >>>>> + return irq; >>>>> + } >>>>> + >>>>> + ret = devm_request_irq(&pdev->dev, irq, >>>>> + stm32_exti_trigger_handler, >>>>> + 0, dev_name(&pdev->dev), pdev); >>> Hmm. So this is a trick to set the interrupt mapping up inside the device. >>> The whole thing doesn't really exist. >>> >>> Rather feels like there ought to be some generic interface for >>> 'I want to pretend I want a particular interrupt but not actually get one'. >>> >>> But that would only work in this weird case where there is also a real interrupt >>> associated with it - just one we elect not to use. >>>>> + if (ret) { >>>>> + dev_err(&pdev->dev, "request IRQ %d failed\n", irq); >>>>> + return ret; >>>>> + } >>>> >>>> Here you need some elaborate trigger edge handling. >>>> >>>> The flags that you define as "0" here, how do we say that we >>>> want to handle rising or falling edges, for example? >>>> >>>> I think you might want to establish these DT properties for >>>> GPIO triggers: >>>> >>>> gpio-trigger-rising-edge; >>>> gpio-trigger-falling-edge; >>>> >>>> Then: >>>> >>>> int irq_flags = 0; >>>> >>>> if (of_property_read_bool(np, "gpio-trigger-rising-edge") >>>> irq_flags |= IRQF_TRIGGER_RISING; >>>> else if (of_property_read_bool(np, "gpio-trigger-falling-edge") >>>> irq_flags |= IRQF_TRIGGER_FALLING; >>> Should this not all be part of the interrupt specification rather >>> than down here in the specific driver? >>>> >>>> To pass along to the devm_request_irq() function as flags. >>>> >>>> I find it weird that it even works without. Most GPIO interrupts >>>> should require you to set a trigger type. But I guess it is because >>>> of the other weirdness you describe below. >>>> >>>>> + /* >>>>> + * gpios are configured as interrupts, so exti trigger path is >>>>> + * configured in HW, and can now be used as external trigger >>>>> + * source by other IPs. But getting interrupts when trigger >>>>> + * occurs is unused here, so mask irq on exti controller by >>>>> + * default. >>>>> + */ >>>>> + disable_irq(irq); >>>> >>>> Aha. That is not generic. But what about just adding: >>>> >>>> if (of_property_read_bool(np, "gpio-trigger-numb-irq") >>>> disable_irq(); >>>> >>>> (Plus add the binding for that something like "this makes the >>>> GPIO mentioned get requested, translated to an IRQ, get the >>>> IRQ requested, and then immediately just disabled as other >>>> hardware will actually hande the IRQ line".) >>>> >>>> I understand that this is kind of weird: we're making a whole generic >>>> GPIO trigger driver just to use it with hardware that grabs and disabled >>>> the irq immediately. >>>> >>>> But I think that in the long run it makes for more reusable code. >>> I'd go a step further. Whether it is numbed or not will depend on what >>> is downstream. We should be providing this interrupt like normal if >>> we have other devices triggering off it. In that case it becomes a standard >>> interrupt trigger. >> >> Hmm, in case stm device is using this trigger, iio_trigger_poll() >> will be called. If I understand your point here, this interrupt >> should be enabled, when stm device isn't using this trigger (for >> generic devices). > Exactly - or when both STM and generic drivers are using it, then it > would be better to trigger the generic consumer of the trigger at this > interrupt, not on the EOC interrupt from the ADC. I just have one doubt about this. If I summarize: 1 - In case "any device" driver uses a gpio trigger, then, only gpio interrupt shall call trigger_poll. 2 - In case STM32 ADC device uses gpio trigger, only EOC interrupt shall call trigger_poll. 3 - In case both 1 + 2, "any device" and STM32 ADC use same trigger... Shouldn't trigger_poll() be called only once ? (e.g. not twice ?) Then I would rather rely on EOC to call trigger_poll() only once... Please advise on this. > >> May validate_device()/set_trigger_state() be used >> to enable or disable this interrupt ? Then, what criteria may be used >> for that purpose ?> >>> >>> Polling off the back of the dataready interrupt is fine if there is nothing >>> earlier available. Here there is so we should really be triggering other >>> devices off this earlier interrupt. >> >> I fear it can add complexity, because it will depend on user choice to >> select this trigger for an stm32 adc, or another device, or both ? >> Not sure how to distinguish both from within this trigger driver. >> In the end, best maybe to implement this closer in stm32 adc/dac >> driver, and limit trigger usage with .validate_device(). > That is certainly a valid choice. Not as nice in some ways, but you > get to decide on what you work on rather than me ;) > > I'm not entirely sure how we'd do the magic to identify the trigger > and deliberately not hook in the pollfunc if it had nothing to do. I hope exposing the GPIO/EXTI trigger -> ADC HW connection to device tree can help on this. DT should reflect HW. Adding some trigger ops, to inform trigger inhibit the trigger_poll() call (and possibly mask the interrupt as well) may do the job ? >> >>>> >>>>> +static const struct of_device_id stm32_exti_trigger_of_match[] = { >>>>> + { .compatible = "st,stm32-exti-trigger" }, >>>>> + {}, >>>> >>>> "iio-gpio-trigger" >>>> >>>> Should fit anyone, given the above amendments. >>>> >>>>> +#if IS_ENABLED(CONFIG_IIO_STM32_EXTI_TRIGGER) >>>>> +bool is_stm32_exti_trigger(struct iio_trigger *trig); >>>>> +#else >>>>> +static inline bool is_stm32_exti_trigger(struct iio_trigger *trig) >>>>> +{ >>>>> + return false; >>>>> +} >>>>> +#endif >>>> >>>> This seems unnecessary to broadcast to the entire kernel. >>> This one section is the only really non generic element that >>> isn't supported by the existing interrupt trigger. >>> Mind you that doesn't have device tree bindings yet :( Please kindly look at RFC I just sent, and let me know your opinion. >> >> Yes, then I suppose simple approach is to rework this, and put it >> inside stm32 adc core driver ? >> Then, register trigger from there, and read from dt child node. >> Is something like the following suitable ? >> adc@...hhh { >> compatible = "st,stm32f4-adc-core" >> ... >> adc1 { >> ... >> } >> >> trigger { >> gpios = <...>; >> st,trigger-value = <11>; /* e.g. EXTI nb */ >> st,trigger-polarity = <1>; >> } >> } >> >> Please let me know your opinion. > That works for me. Could conceive of more complex situations that > doesn't describe (driving both the ADC and a DAC for example - if > that's possible in the hardware). Can figure out how to do that when it > matters. I hope I can give some more tries, try to do as generic as possible. About your remark on ADC/DAC: Well, I think only possible case is the above 1, 2 & 3. DAC isn't there yet. But anyway, it doesn't share same lines as ADC in hw :-). Best Regards, Fabrice > > Jonathan >> >> Best regards, >> Fabrice >> >>> >>> I wonder if we can add some sort of flag in there to identify hardware >>> blocks for tricks like this. Or at a push provide the interrupt >>> to both bits of kit so they can compare and see if they are looking >>> at the same one? >>>> >>>> Why? (Maybe I can find explanations in later patches. >>>> >>>> Yours, >>>> Linus Walleij >>>> >>> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-iio" in >> the body of a message to majordomo@...r.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >
Powered by blists - more mailing lists