| Message ID | E1oTkeg-003t9k-Mc@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add Apple Mac System Management Controller GPIOs | expand |
On Thu, Sep 1, 2022 at 5:18 PM Russell King <rmk+kernel@armlinux.org.uk> wrote: > > From: Hector Martin <marcan@marcan.st> > > Add IRQ support to the macsmc driver. This patch has updates from Joey > Gouly and Russell King. ... > + u16 type = event >> 16; > + u8 offset = (event >> 8) & 0xff; The ' & 0xff' part is redundant. ... > + return (ret == 0) ? NOTIFY_OK : NOTIFY_DONE; Parentheses and ' == 0' parts are redundant. You may swap ternary operands. ... > +static void macsmc_gpio_irq_enable(struct irq_data *d) > +{ > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > + struct macsmc_gpio *smcgp = gpiochip_get_data(gc); > + > + gpiochip_enable_irq(gc, irqd_to_hwirq(d)); > + set_bit(irqd_to_hwirq(d), smcgp->irq_enable_shadow); You may use temporary variable for hwirq irq_hw_number_t hwirq = irdq_to_hwirq(d); > +} > + > +static void macsmc_gpio_irq_disable(struct irq_data *d) > +{ > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > + struct macsmc_gpio *smcgp = gpiochip_get_data(gc); > + > + clear_bit(irqd_to_hwirq(d), smcgp->irq_enable_shadow); > + gpiochip_disable_irq(gc, irqd_to_hwirq(d)); Ditto. > +} > + > +static int macsmc_gpio_irq_set_type(struct irq_data *d, unsigned int type) > +{ > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > + struct macsmc_gpio *smcgp = gpiochip_get_data(gc); > + int offset = irqd_to_hwirq(d); As above. > + u32 mode; > + if (!test_bit(offset, smcgp->irq_supported)) > + return -EINVAL; We have a valid mask for IRQs. Can it be used here instead? > + switch (type & IRQ_TYPE_SENSE_MASK) { > + case IRQ_TYPE_LEVEL_HIGH: > + mode = IRQ_MODE_HIGH; > + break; > + case IRQ_TYPE_LEVEL_LOW: > + mode = IRQ_MODE_LOW; > + break; > + case IRQ_TYPE_EDGE_RISING: > + mode = IRQ_MODE_RISING; > + break; > + case IRQ_TYPE_EDGE_FALLING: > + mode = IRQ_MODE_FALLING; > + break; > + case IRQ_TYPE_EDGE_BOTH: > + mode = IRQ_MODE_BOTH; > + break; > + default: > + return -EINVAL; > } > > + smcgp->irq_mode_shadow[offset] = mode; Usually we want to have handle_bad_irq() handler by default and in ->set_type() we lock a handler depending on the flags. Why is this not the case in this driver? > return 0; > } ... > +static void macsmc_gpio_irq_bus_sync_unlock(struct irq_data *d) > +{ > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > + struct macsmc_gpio *smcgp = gpiochip_get_data(gc); > + smc_key key = macsmc_gpio_key(irqd_to_hwirq(d)); > + int offset = irqd_to_hwirq(d); As above. > + bool val; > + > + if (smcgp->irq_mode_shadow[offset] != smcgp->irq_mode[offset]) { > + u32 cmd = CMD_IRQ_MODE | smcgp->irq_mode_shadow[offset]; > + if (apple_smc_write_u32(smcgp->smc, key, cmd) < 0) > + dev_err(smcgp->dev, "GPIO IRQ config failed for %p4ch = 0x%x\n", &key, cmd); > + else > + smcgp->irq_mode_shadow[offset] = smcgp->irq_mode[offset]; > + } > + > + val = test_bit(offset, smcgp->irq_enable_shadow); > + if (test_bit(offset, smcgp->irq_enable) != val) { > + if (apple_smc_write_u32(smcgp->smc, key, CMD_IRQ_ENABLE | val) < 0) > + dev_err(smcgp->dev, "GPIO IRQ en/disable failed for %p4ch\n", &key); > + else > + change_bit(offset, smcgp->irq_enable); > + } > + > + mutex_unlock(&smcgp->irq_mutex); > +} -- With Best Regards, Andy Shevchenko
On Thu, Sep 1, 2022 at 3:54 PM Russell King <rmk+kernel@armlinux.org.uk> wrote: > From: Hector Martin <marcan@marcan.st> > > Add IRQ support to the macsmc driver. This patch has updates from Joey > Gouly and Russell King. > > Signed-off-by: Hector Martin <marcan@marcan.st> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> Overall looks very good, again some detailed remarks from Andy should be addressed, apart from that you need to add: select GPIOLIB_IRQCHIP to the Kconfig entry for the driver, or else the compile robots are going to hit some configuration that doesn't compile. > + struct mutex irq_mutex; > + DECLARE_BITMAP(irq_supported, MAX_GPIO); If you can use the .init_valid_mask from struct gpio_irq_chip instead, it will allocate this mask dynamically for the irqchip. (Further comment below.) > + DECLARE_BITMAP(irq_enable_shadow, MAX_GPIO); Please rename irq_unmasked_shadow as it is tracking this and not what the irqchip core calls enabled/disabled. > + DECLARE_BITMAP(irq_enable, MAX_GPIO); I think this state should be possible to set/get from the irqchip core. !irqd_irq_masked(d) on the descriptor, correct me if I'm wrong. > + u32 irq_mode_shadow[MAX_GPIO]; > + u32 irq_mode[MAX_GPIO]; > > int first_index; > }; > @@ -161,6 +172,7 @@ static int macsmc_gpio_init_valid_mask(struct gpio_chip *gc, > for (i = 0; i < count; i++) { > smc_key key; > int gpio_nr; > + u32 val; > int ret = apple_smc_get_key_by_index(smcgp->smc, smcgp->first_index + i, &key); > > if (ret < 0) > @@ -176,11 +188,143 @@ static int macsmc_gpio_init_valid_mask(struct gpio_chip *gc, > } > > set_bit(gpio_nr, valid_mask); > + > + /* Check for IRQ support */ > + ret = apple_smc_rw_u32(smcgp->smc, key, CMD_IRQ_MODE, &val); > + if (!ret) > + set_bit(gpio_nr, smcgp->irq_supported); > + } > + > + return 0; > +} This gets initialized from the struct gpio_chip .init_valid_mask, but struct gpio_irq_chip has its own callback with the same name, which is preferred to be used for this, check if you can use that instead, it makes the use more obvious. > +static int macsmc_gpio_event(struct notifier_block *nb, unsigned long event, void *data) > +{ > + struct macsmc_gpio *smcgp = container_of(nb, struct macsmc_gpio, nb); > + u16 type = event >> 16; > + u8 offset = (event >> 8) & 0xff; > + smc_key key = macsmc_gpio_key(offset); > + unsigned long flags; > + int ret; > + > + if (type != SMC_EV_GPIO) > + return NOTIFY_DONE; > + > + if (offset > MAX_GPIO) { > + dev_err(smcgp->dev, "GPIO event index %d out of range\n", offset); > + return NOTIFY_BAD; > + } > + > + local_irq_save(flags); > + ret = generic_handle_domain_irq(smcgp->gc.irq.domain, offset); > + local_irq_restore(flags); Isn't irq_bus_lock/unlock protecting us here already? (I might be getting it wrong...) Since this is coming from a notifier and not an IRQ or threaded IRQ I actually am a bit puzzled on how to handle it... you probably know it better than me, maybe ask Marc Z if anything is unclear. > + if (apple_smc_write_u32(smcgp->smc, key, CMD_IRQ_ACK | 1) < 0) > + dev_err(smcgp->dev, "GPIO IRQ ack failed for %p4ch\n", &key); isn't this one of those cases where we should implement the irqchip callback .irq_ack() specifically for this? That callback will only be used by edge triggered IRQs but I guess that would realistically be all we support anyway? (See comment below on .set_type) > +static void macsmc_gpio_irq_enable(struct irq_data *d) > +{ > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > + struct macsmc_gpio *smcgp = gpiochip_get_data(gc); > + > + gpiochip_enable_irq(gc, irqd_to_hwirq(d)); > + set_bit(irqd_to_hwirq(d), smcgp->irq_enable_shadow); > +} > + > +static void macsmc_gpio_irq_disable(struct irq_data *d) > +{ > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > + struct macsmc_gpio *smcgp = gpiochip_get_data(gc); > + > + clear_bit(irqd_to_hwirq(d), smcgp->irq_enable_shadow); > + gpiochip_disable_irq(gc, irqd_to_hwirq(d)); > +} I would rename these unmask/mask to match the callback hooks they are implementing, since there are irqchips callbacks with these names I get a but confused. > +static int macsmc_gpio_irq_set_type(struct irq_data *d, unsigned int type) > +{ > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > + struct macsmc_gpio *smcgp = gpiochip_get_data(gc); > + int offset = irqd_to_hwirq(d); > + u32 mode; > + > + if (!test_bit(offset, smcgp->irq_supported)) > + return -EINVAL; > + > + switch (type & IRQ_TYPE_SENSE_MASK) { > + case IRQ_TYPE_LEVEL_HIGH: > + mode = IRQ_MODE_HIGH; > + break; > + case IRQ_TYPE_LEVEL_LOW: > + mode = IRQ_MODE_LOW; > + break; > + case IRQ_TYPE_EDGE_RISING: > + mode = IRQ_MODE_RISING; > + break; > + case IRQ_TYPE_EDGE_FALLING: > + mode = IRQ_MODE_FALLING; > + break; > + case IRQ_TYPE_EDGE_BOTH: > + mode = IRQ_MODE_BOTH; > + break; > + default: > + return -EINVAL; I don't know how level IRQs would work on this essentially message-passing process context interrupt. Maybe I am getting it all wrong, but for level the line should be held low/high until the IRQ is serviced, it would be possible to test if this actually works by *not* servicing an IRQ and see if the SMC then sends another message notifier for the same IRQ. I strongly suspect that actually only edges are supported, but there might be semantics I don't understand here. > } > > + smcgp->irq_mode_shadow[offset] = mode; Hm yeah I guess this shadow mode is necessary for the sync to work. > return 0; > } > > +static void macsmc_gpio_irq_bus_lock(struct irq_data *d) > +{ > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > + struct macsmc_gpio *smcgp = gpiochip_get_data(gc); > + > + mutex_lock(&smcgp->irq_mutex); > +} > + > +static void macsmc_gpio_irq_bus_sync_unlock(struct irq_data *d) > +{ > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > + struct macsmc_gpio *smcgp = gpiochip_get_data(gc); > + smc_key key = macsmc_gpio_key(irqd_to_hwirq(d)); > + int offset = irqd_to_hwirq(d); > + bool val; > + > + if (smcgp->irq_mode_shadow[offset] != smcgp->irq_mode[offset]) { > + u32 cmd = CMD_IRQ_MODE | smcgp->irq_mode_shadow[offset]; > + if (apple_smc_write_u32(smcgp->smc, key, cmd) < 0) > + dev_err(smcgp->dev, "GPIO IRQ config failed for %p4ch = 0x%x\n", &key, cmd); > + else > + smcgp->irq_mode_shadow[offset] = smcgp->irq_mode[offset]; > + } > + > + val = test_bit(offset, smcgp->irq_enable_shadow); > + if (test_bit(offset, smcgp->irq_enable) != val) { So what you want to know for each line is (correct me if I'm wrong): - Is it enabled (unmasked) or not? - Did it get changed enabled->disabled, disabled->enabled since macsmc_gpio_irq_bus_lock()? Doesn't the irqchip core track the first part of this for you? irqd_irq_masked(d) should tell you the same thing as irq_enable, just inverted. irq_enable_shadow is a bit tricker, I guess you might need that since the irqchip doesn't track state changes. > static int macsmc_gpio_probe(struct platform_device *pdev) > { > struct macsmc_gpio *smcgp; > @@ -221,6 +365,18 @@ static int macsmc_gpio_probe(struct platform_device *pdev) > smcgp->gc.base = -1; > smcgp->gc.parent = &pdev->dev; > > + gpio_irq_chip_set_chip(&smcgp->gc.irq, &macsmc_gpio_irqchip); > + smcgp->gc.irq.parent_handler = NULL; > + smcgp->gc.irq.num_parents = 0; > + smcgp->gc.irq.parents = NULL; > + smcgp->gc.irq.default_type = IRQ_TYPE_NONE; > + smcgp->gc.irq.handler = handle_simple_irq; I would consider setting this to handle_edge_irq() and implement .irq_ack(). I might be wrong. But overall since this IRQ is driven by a notifier I feel a bit lost. Yours, Linus Walleij
On Thu, Sep 01, 2022 at 09:03:49PM +0300, Andy Shevchenko wrote: > On Thu, Sep 1, 2022 at 5:18 PM Russell King <rmk+kernel@armlinux.org.uk> wrote: > > From: Hector Martin <marcan@marcan.st> > > > > Add IRQ support to the macsmc driver. This patch has updates from Joey > > Gouly and Russell King. > > ... > > > + u16 type = event >> 16; > > + u8 offset = (event >> 8) & 0xff; > > The ' & 0xff' part is redundant. It's probably also more logical to call this "hwirq". > > +static int macsmc_gpio_irq_set_type(struct irq_data *d, unsigned int type) > > +{ > > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > > + struct macsmc_gpio *smcgp = gpiochip_get_data(gc); ... > > + if (!test_bit(offset, smcgp->irq_supported)) > > + return -EINVAL; > > We have a valid mask for IRQs. Can it be used here instead? Looks like we can, thanks for the suggestion. > > + smcgp->irq_mode_shadow[offset] = mode; > > Usually we want to have handle_bad_irq() handler by default and in > ->set_type() we lock a handler depending on the flags. Why is this not > the case in this driver? "lock a handler" ? I guess you mean select a handler. I don't see a reason why we couldn't switch between handle_bad_irq() and handle_simple_irq(). I would guess (I don't know the implementation details of the Apple platform) that the SMC forwards a message when the IRQ happens, but I'm guessing that this is well tested on the platform with the simple flow handler. Changing it to something else would need discussion with the Asahi Linux folk.
On Fri, Sep 02, 2022 at 03:21:31PM +0200, Linus Walleij wrote: > On Thu, Sep 1, 2022 at 3:54 PM Russell King <rmk+kernel@armlinux.org.uk> wrote: > > + DECLARE_BITMAP(irq_enable_shadow, MAX_GPIO); > > Please rename irq_unmasked_shadow as it is tracking > this and not what the irqchip core calls enabled/disabled. > > > + DECLARE_BITMAP(irq_enable, MAX_GPIO); > > I think this state should be possible to set/get from the irqchip > core. !irqd_irq_masked(d) on the descriptor, correct me if I'm wrong. I think you're getting the two mixed up. irq_enable_shadow (irq_unmasked_shadow) is updated from the ->irq_mask and ->irq_unmask callbacaks, and will track !irqd_irq_masked(d) state. So, I think we can get rid of irq_enable_shadow and just use !irqd_irq_masked(d). The irq_enable bit array tracks the state on the SMC, and is used to indicate whether we need to update that state when we unlock the bus (which is when the driver talks to the SMC to reconfigure it.) So, I think killing irq_enable_shadow and replacing irq_enable with irq_unmasked would be correct - and going a bit further, irq_smc_unmasked to show that it's the SMC's status. > > +static int macsmc_gpio_event(struct notifier_block *nb, unsigned long event, void *data) > > +{ > > + struct macsmc_gpio *smcgp = container_of(nb, struct macsmc_gpio, nb); > > + u16 type = event >> 16; > > + u8 offset = (event >> 8) & 0xff; > > + smc_key key = macsmc_gpio_key(offset); > > + unsigned long flags; > > + int ret; > > + > > + if (type != SMC_EV_GPIO) > > + return NOTIFY_DONE; > > + > > + if (offset > MAX_GPIO) { > > + dev_err(smcgp->dev, "GPIO event index %d out of range\n", offset); > > + return NOTIFY_BAD; > > + } > > + > > + local_irq_save(flags); > > + ret = generic_handle_domain_irq(smcgp->gc.irq.domain, offset); > > + local_irq_restore(flags); > > Isn't irq_bus_lock/unlock protecting us here already? > (I might be getting it wrong...) Hmm, where does irq_bus_lock get called? Given this function is called while running a blocking notifier chain, interrupts will not be disabled on entry to this function. I haven't found a place in the maze of irq handling code that generic_handle_domain_irq() would end up using the bus lock/unlock functions - and if they did, with the above IRQ saving, the kernel would WARN() about calling mutex_lock() with IRQs disabled. So it doesn't. This actually entirely negates any benefit of the kernel trying to mask or unmask an interrupt in "hardware" while running the handler - since macsmc_gpio_irq_bus_sync_unlock() won't be called, the state on the SMC won't get touched. > Since this is coming from a notifier and not an IRQ or threaded > IRQ I actually am a bit puzzled on how to handle it... you probably > know it better than me, maybe ask Marc Z if anything is > unclear. It's been years since I did any real platform porting work, so deep knowledge of the IRQ subsystem has evaporated. > > + if (apple_smc_write_u32(smcgp->smc, key, CMD_IRQ_ACK | 1) < 0) > > + dev_err(smcgp->dev, "GPIO IRQ ack failed for %p4ch\n", &key); > > isn't this one of those cases where we should implement the > irqchip callback .irq_ack() specifically for this? > > That callback will only be used by edge triggered IRQs but > I guess that would realistically be all we support anyway? > (See comment below on .set_type) I would imagine it depends on how the SMC GPIO interrupt works - whether the ACK is ACK as we know it in Linux (x86 PIC) or whether it's ACK as in a notification to the SMC that we have finished handling the interrupt and it can send us the next interrupt. I suspect we don't know that level of detail of the platform, so given that this is what the Asahi kernel does, that's the best we have. > > +static int macsmc_gpio_irq_set_type(struct irq_data *d, unsigned int type) > > +{ > > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > > + struct macsmc_gpio *smcgp = gpiochip_get_data(gc); > > + int offset = irqd_to_hwirq(d); > > + u32 mode; > > + > > + if (!test_bit(offset, smcgp->irq_supported)) > > + return -EINVAL; > > + > > + switch (type & IRQ_TYPE_SENSE_MASK) { > > + case IRQ_TYPE_LEVEL_HIGH: > > + mode = IRQ_MODE_HIGH; > > + break; > > + case IRQ_TYPE_LEVEL_LOW: > > + mode = IRQ_MODE_LOW; > > + break; > > + case IRQ_TYPE_EDGE_RISING: > > + mode = IRQ_MODE_RISING; > > + break; > > + case IRQ_TYPE_EDGE_FALLING: > > + mode = IRQ_MODE_FALLING; > > + break; > > + case IRQ_TYPE_EDGE_BOTH: > > + mode = IRQ_MODE_BOTH; > > + break; > > + default: > > + return -EINVAL; > > I don't know how level IRQs would work on this essentially > message-passing process context interrupt. Maybe I am getting > it all wrong, but for level the line should be held low/high until > the IRQ is serviced, it would be possible to test if this actually > works by *not* servicing an IRQ and see if the SMC then sends > another message notifier for the same IRQ. If level IRQs are not supported, then it's strange that the Asahi folk have been able to reverse engineer the CMD_IRQ_MODE codes for these states. Maybe the SMC issues another message for a level IRQ after it receives a CMD_IRQ_ACK message if the level interrupt is still asserted? > I strongly suspect that actually only edges are supported, but > there might be semantics I don't understand here. > > > } > > > > + smcgp->irq_mode_shadow[offset] = mode; > > Hm yeah I guess this shadow mode is necessary for the sync > to work. Ineed. > > +static void macsmc_gpio_irq_bus_sync_unlock(struct irq_data *d) > > +{ > > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > > + struct macsmc_gpio *smcgp = gpiochip_get_data(gc); > > + smc_key key = macsmc_gpio_key(irqd_to_hwirq(d)); > > + int offset = irqd_to_hwirq(d); > > + bool val; > > + > > + if (smcgp->irq_mode_shadow[offset] != smcgp->irq_mode[offset]) { > > + u32 cmd = CMD_IRQ_MODE | smcgp->irq_mode_shadow[offset]; > > + if (apple_smc_write_u32(smcgp->smc, key, cmd) < 0) > > + dev_err(smcgp->dev, "GPIO IRQ config failed for %p4ch = 0x%x\n", &key, cmd); > > + else > > + smcgp->irq_mode_shadow[offset] = smcgp->irq_mode[offset]; > > + } > > + > > + val = test_bit(offset, smcgp->irq_enable_shadow); > > + if (test_bit(offset, smcgp->irq_enable) != val) { > > So what you want to know for each line is (correct me if I'm wrong): > - Is it enabled (unmasked) or not? > - Did it get changed enabled->disabled, disabled->enabled since > macsmc_gpio_irq_bus_lock()? I think you mean here "did the 'enable' state change between macsmc_gpio_irq_bus_lock() and macsmc_gpio_irq_bus_unlock". > Doesn't the irqchip core track the first part of this for you? > irqd_irq_masked(d) should tell you the same thing as > irq_enable, just inverted. > > irq_enable_shadow is a bit tricker, I guess you might need that since > the irqchip doesn't track state changes. Yep, which is what I've said above in this reply where these bitmaps are declared. > > static int macsmc_gpio_probe(struct platform_device *pdev) > > { > > struct macsmc_gpio *smcgp; > > @@ -221,6 +365,18 @@ static int macsmc_gpio_probe(struct platform_device *pdev) > > smcgp->gc.base = -1; > > smcgp->gc.parent = &pdev->dev; > > > > + gpio_irq_chip_set_chip(&smcgp->gc.irq, &macsmc_gpio_irqchip); > > + smcgp->gc.irq.parent_handler = NULL; > > + smcgp->gc.irq.num_parents = 0; > > + smcgp->gc.irq.parents = NULL; > > + smcgp->gc.irq.default_type = IRQ_TYPE_NONE; > > + smcgp->gc.irq.handler = handle_simple_irq; > > I would consider setting this to handle_edge_irq() and implement > .irq_ack(). I might be wrong. I don't think that's suitable, because then we'll be calling irq_ack() before the handler has run - and we won't be notifying the SMC that the interrupt has been masked. So it could send another notification for the same IRQ while it's still being handled. Then there's the question about level IRQs as well. I think, given that I don't know how the SMC works (presumably the Asahi folk have a bit more of an idea, but that will be based on reverse engineering effort) that I am not going to modify this driver's behaviour drastically by changing the flow handler to the edge flow handler from the simple flow. To me, that could well be a disaster for this driver. That would be something for the Asahi folk to look at.
(Replied privately to Russell by a mistake) ---------- Forwarded message --------- From: Russell King (Oracle) <linux@armlinux.org.uk> Date: Mon, Sep 5, 2022 at 3:50 PM Subject: Re: [PATCH 6/6] gpio: macsmc: Add IRQ support To: Linus Walleij <linus.walleij@linaro.org> Cc: Arnd Bergmann <arnd@arndb.de>, Lee Jones <lee@kernel.org>, Alyssa Rosenzweig <alyssa@rosenzweig.io>, <asahi@lists.linux.dev>, Bartosz Golaszewski <brgl@bgdev.pl>, Hector Martin <marcan@marcan.st>, <linux-arm-kernel@lists.infradead.org>, <linux-gpio@vger.kernel.org>, Sven Peter <sven@svenpeter.dev> On Fri, Sep 02, 2022 at 03:21:31PM +0200, Linus Walleij wrote: > On Thu, Sep 1, 2022 at 3:54 PM Russell King <rmk+kernel@armlinux.org.uk> wrote: > > + DECLARE_BITMAP(irq_enable_shadow, MAX_GPIO); > > Please rename irq_unmasked_shadow as it is tracking > this and not what the irqchip core calls enabled/disabled. > > > + DECLARE_BITMAP(irq_enable, MAX_GPIO); > > I think this state should be possible to set/get from the irqchip > core. !irqd_irq_masked(d) on the descriptor, correct me if I'm wrong. I think you're getting the two mixed up. irq_enable_shadow (irq_unmasked_shadow) is updated from the ->irq_mask and ->irq_unmask callbacaks, and will track !irqd_irq_masked(d) state. So, I think we can get rid of irq_enable_shadow and just use !irqd_irq_masked(d). The irq_enable bit array tracks the state on the SMC, and is used to indicate whether we need to update that state when we unlock the bus (which is when the driver talks to the SMC to reconfigure it.) So, I think killing irq_enable_shadow and replacing irq_enable with irq_unmasked would be correct - and going a bit further, irq_smc_unmasked to show that it's the SMC's status. > > +static int macsmc_gpio_event(struct notifier_block *nb, unsigned long event, void *data) > > +{ > > + struct macsmc_gpio *smcgp = container_of(nb, struct macsmc_gpio, nb); > > + u16 type = event >> 16; > > + u8 offset = (event >> 8) & 0xff; > > + smc_key key = macsmc_gpio_key(offset); > > + unsigned long flags; > > + int ret; > > + > > + if (type != SMC_EV_GPIO) > > + return NOTIFY_DONE; > > + > > + if (offset > MAX_GPIO) { > > + dev_err(smcgp->dev, "GPIO event index %d out of range\n", offset); > > + return NOTIFY_BAD; > > + } > > + > > + local_irq_save(flags); > > + ret = generic_handle_domain_irq(smcgp->gc.irq.domain, offset); > > + local_irq_restore(flags); > > Isn't irq_bus_lock/unlock protecting us here already? > (I might be getting it wrong...) Hmm, where does irq_bus_lock get called? Given this function is called while running a blocking notifier chain, interrupts will not be disabled on entry to this function. I haven't found a place in the maze of irq handling code that generic_handle_domain_irq() would end up using the bus lock/unlock functions - and if they did, with the above IRQ saving, the kernel would WARN() about calling mutex_lock() with IRQs disabled. So it doesn't. This actually entirely negates any benefit of the kernel trying to mask or unmask an interrupt in "hardware" while running the handler - since macsmc_gpio_irq_bus_sync_unlock() won't be called, the state on the SMC won't get touched. > Since this is coming from a notifier and not an IRQ or threaded > IRQ I actually am a bit puzzled on how to handle it... you probably > know it better than me, maybe ask Marc Z if anything is > unclear. It's been years since I did any real platform porting work, so deep knowledge of the IRQ subsystem has evaporated. > > + if (apple_smc_write_u32(smcgp->smc, key, CMD_IRQ_ACK | 1) < 0) > > + dev_err(smcgp->dev, "GPIO IRQ ack failed for %p4ch\n", &key); > > isn't this one of those cases where we should implement the > irqchip callback .irq_ack() specifically for this? > > That callback will only be used by edge triggered IRQs but > I guess that would realistically be all we support anyway? > (See comment below on .set_type) I would imagine it depends on how the SMC GPIO interrupt works - whether the ACK is ACK as we know it in Linux (x86 PIC) or whether it's ACK as in a notification to the SMC that we have finished handling the interrupt and it can send us the next interrupt. I suspect we don't know that level of detail of the platform, so given that this is what the Asahi kernel does, that's the best we have. > > +static int macsmc_gpio_irq_set_type(struct irq_data *d, unsigned int type) > > +{ > > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > > + struct macsmc_gpio *smcgp = gpiochip_get_data(gc); > > + int offset = irqd_to_hwirq(d); > > + u32 mode; > > + > > + if (!test_bit(offset, smcgp->irq_supported)) > > + return -EINVAL; > > + > > + switch (type & IRQ_TYPE_SENSE_MASK) { > > + case IRQ_TYPE_LEVEL_HIGH: > > + mode = IRQ_MODE_HIGH; > > + break; > > + case IRQ_TYPE_LEVEL_LOW: > > + mode = IRQ_MODE_LOW; > > + break; > > + case IRQ_TYPE_EDGE_RISING: > > + mode = IRQ_MODE_RISING; > > + break; > > + case IRQ_TYPE_EDGE_FALLING: > > + mode = IRQ_MODE_FALLING; > > + break; > > + case IRQ_TYPE_EDGE_BOTH: > > + mode = IRQ_MODE_BOTH; > > + break; > > + default: > > + return -EINVAL; > > I don't know how level IRQs would work on this essentially > message-passing process context interrupt. Maybe I am getting > it all wrong, but for level the line should be held low/high until > the IRQ is serviced, it would be possible to test if this actually > works by *not* servicing an IRQ and see if the SMC then sends > another message notifier for the same IRQ. If level IRQs are not supported, then it's strange that the Asahi folk have been able to reverse engineer the CMD_IRQ_MODE codes for these states. Maybe the SMC issues another message for a level IRQ after it receives a CMD_IRQ_ACK message if the level interrupt is still asserted? > I strongly suspect that actually only edges are supported, but > there might be semantics I don't understand here. > > > } > > > > + smcgp->irq_mode_shadow[offset] = mode; > > Hm yeah I guess this shadow mode is necessary for the sync > to work. Ineed. > > +static void macsmc_gpio_irq_bus_sync_unlock(struct irq_data *d) > > +{ > > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > > + struct macsmc_gpio *smcgp = gpiochip_get_data(gc); > > + smc_key key = macsmc_gpio_key(irqd_to_hwirq(d)); > > + int offset = irqd_to_hwirq(d); > > + bool val; > > + > > + if (smcgp->irq_mode_shadow[offset] != smcgp->irq_mode[offset]) { > > + u32 cmd = CMD_IRQ_MODE | smcgp->irq_mode_shadow[offset]; > > + if (apple_smc_write_u32(smcgp->smc, key, cmd) < 0) > > + dev_err(smcgp->dev, "GPIO IRQ config failed for %p4ch = 0x%x\n", &key, cmd); > > + else > > + smcgp->irq_mode_shadow[offset] = smcgp->irq_mode[offset]; > > + } > > + > > + val = test_bit(offset, smcgp->irq_enable_shadow); > > + if (test_bit(offset, smcgp->irq_enable) != val) { > > So what you want to know for each line is (correct me if I'm wrong): > - Is it enabled (unmasked) or not? > - Did it get changed enabled->disabled, disabled->enabled since > macsmc_gpio_irq_bus_lock()? I think you mean here "did the 'enable' state change between macsmc_gpio_irq_bus_lock() and macsmc_gpio_irq_bus_unlock". > Doesn't the irqchip core track the first part of this for you? > irqd_irq_masked(d) should tell you the same thing as > irq_enable, just inverted. > > irq_enable_shadow is a bit tricker, I guess you might need that since > the irqchip doesn't track state changes. Yep, which is what I've said above in this reply where these bitmaps are declared. > > static int macsmc_gpio_probe(struct platform_device *pdev) > > { > > struct macsmc_gpio *smcgp; > > @@ -221,6 +365,18 @@ static int macsmc_gpio_probe(struct platform_device *pdev) > > smcgp->gc.base = -1; > > smcgp->gc.parent = &pdev->dev; > > > > + gpio_irq_chip_set_chip(&smcgp->gc.irq, &macsmc_gpio_irqchip); > > + smcgp->gc.irq.parent_handler = NULL; > > + smcgp->gc.irq.num_parents = 0; > > + smcgp->gc.irq.parents = NULL; > > + smcgp->gc.irq.default_type = IRQ_TYPE_NONE; > > + smcgp->gc.irq.handler = handle_simple_irq; > > I would consider setting this to handle_edge_irq() and implement > .irq_ack(). I might be wrong. I don't think that's suitable, because then we'll be calling irq_ack() before the handler has run - and we won't be notifying the SMC that the interrupt has been masked. So it could send another notification for the same IRQ while it's still being handled. Then there's the question about level IRQs as well. I think, given that I don't know how the SMC works (presumably the Asahi folk have a bit more of an idea, but that will be based on reverse engineering effort) that I am not going to modify this driver's behaviour drastically by changing the flow handler to the edge flow handler from the simple flow. To me, that could well be a disaster for this driver. That would be something for the Asahi folk to look at. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Mon, Sep 5, 2022 at 4:04 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Mon, Sep 5, 2022 at 2:54 PM Russell King (Oracle) > <linux@armlinux.org.uk> wrote: > > On Thu, Sep 01, 2022 at 09:03:49PM +0300, Andy Shevchenko wrote: > > > On Thu, Sep 1, 2022 at 5:18 PM Russell King <rmk+kernel@armlinux.org.uk> wrote: > > ... > > > > Usually we want to have handle_bad_irq() handler by default and in > > > ->set_type() we lock a handler depending on the flags. Why is this not > > > the case in this driver? > > > > "lock a handler" ? I guess you mean select a handler. > > Yes, I was a bit confused by API naming (irq_set_handler_locked() was it). > > > I don't see a reason why we couldn't switch between handle_bad_irq() > > and handle_simple_irq(). I would guess (I don't know the implementation > > details of the Apple platform) that the SMC forwards a message when the > > IRQ happens, but I'm guessing that this is well tested on the platform > > with the simple flow handler. > > I have had a real case where it makes quite a difference: > eb441337c714 ("gpio: pca953x: Set IRQ type when handle Intel Galileo Gen 2") > > The missed ->irq_set_type() call shows a bug in the IRQ flow in the > code. This became visible due to the switch to handle_bad_irq() and > was hidden with other default handlers. After this I ask people to use > the default handler to be a "bad" one. > > > Changing it to something else would need > > discussion with the Asahi Linux folk. > > > > -- > With Best Regards, > Andy Shevchenko
On Mon, Sep 5, 2022 at 2:47 PM Russell King (Oracle) <linux@armlinux.org.uk> wrote: > On Fri, Sep 02, 2022 at 03:21:31PM +0200, Linus Walleij wrote: > > > + local_irq_save(flags); > > > + ret = generic_handle_domain_irq(smcgp->gc.irq.domain, offset); > > > + local_irq_restore(flags); > > > > Isn't irq_bus_lock/unlock protecting us here already? > > (I might be getting it wrong...) > > Hmm, where does irq_bus_lock get called? Given this function is called > while running a blocking notifier chain, interrupts will not be > disabled on entry to this function. I haven't found a place in the maze > of irq handling code that generic_handle_domain_irq() would end up using > the bus lock/unlock functions - and if they did, with the above IRQ > saving, the kernel would WARN() about calling mutex_lock() with IRQs > disabled. So it doesn't. Ah I get it now, the notification mechanism goes entirely orthogonal to the irqchip, that's what got me confused. You're right, keep this. > > That callback will only be used by edge triggered IRQs but > > I guess that would realistically be all we support anyway? > > (See comment below on .set_type) > > I would imagine it depends on how the SMC GPIO interrupt works - > whether the ACK is ACK as we know it in Linux (x86 PIC) or whether > it's ACK as in a notification to the SMC that we have finished > handling the interrupt and it can send us the next interrupt. > > I suspect we don't know that level of detail of the platform, so > given that this is what the Asahi kernel does, that's the best we > have. OK > > > +static int macsmc_gpio_irq_set_type(struct irq_data *d, unsigned int type) > > > +{ > > > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > > > + struct macsmc_gpio *smcgp = gpiochip_get_data(gc); > > > + int offset = irqd_to_hwirq(d); > > > + u32 mode; > > > + > > > + if (!test_bit(offset, smcgp->irq_supported)) > > > + return -EINVAL; > > > + > > > + switch (type & IRQ_TYPE_SENSE_MASK) { > > > + case IRQ_TYPE_LEVEL_HIGH: > > > + mode = IRQ_MODE_HIGH; > > > + break; > > > + case IRQ_TYPE_LEVEL_LOW: > > > + mode = IRQ_MODE_LOW; > > > + break; > > > + case IRQ_TYPE_EDGE_RISING: > > > + mode = IRQ_MODE_RISING; > > > + break; > > > + case IRQ_TYPE_EDGE_FALLING: > > > + mode = IRQ_MODE_FALLING; > > > + break; > > > + case IRQ_TYPE_EDGE_BOTH: > > > + mode = IRQ_MODE_BOTH; > > > + break; > > > + default: > > > + return -EINVAL; > > > > I don't know how level IRQs would work on this essentially > > message-passing process context interrupt. Maybe I am getting > > it all wrong, but for level the line should be held low/high until > > the IRQ is serviced, it would be possible to test if this actually > > works by *not* servicing an IRQ and see if the SMC then sends > > another message notifier for the same IRQ. > > If level IRQs are not supported, then it's strange that the Asahi > folk have been able to reverse engineer the CMD_IRQ_MODE codes for > these states. > > Maybe the SMC issues another message for a level IRQ after it receives > a CMD_IRQ_ACK message if the level interrupt is still asserted? (...) > > > + gpio_irq_chip_set_chip(&smcgp->gc.irq, &macsmc_gpio_irqchip); > > > + smcgp->gc.irq.parent_handler = NULL; > > > + smcgp->gc.irq.num_parents = 0; > > > + smcgp->gc.irq.parents = NULL; > > > + smcgp->gc.irq.default_type = IRQ_TYPE_NONE; > > > + smcgp->gc.irq.handler = handle_simple_irq; > > > > I would consider setting this to handle_edge_irq() and implement > > .irq_ack(). I might be wrong. > > I don't think that's suitable, because then we'll be calling irq_ack() > before the handler has run - and we won't be notifying the SMC that > the interrupt has been masked. So it could send another notification > for the same IRQ while it's still being handled. Then there's the > question about level IRQs as well. > > I think, given that I don't know how the SMC works (presumably the > Asahi folk have a bit more of an idea, but that will be based on > reverse engineering effort) that I am not going to modify this driver's > behaviour drastically by changing the flow handler to the edge flow > handler from the simple flow. To me, that could well be a disaster > for this driver. That would be something for the Asahi folk to look > at. Fair enough. From my end this will be fine to merge after you considered the things brought up and it is certainly not necessary to have any "perfect" solution, to me it is clear that what we need to do is enable the target so that people can use it and then we/Asahi can comb through it and reexamine things like this once the whole system is usable as a whole. I've seen that Konrad has even started using the M1 infrastructure to kickstart a few iPhone/iPad devices, so given how much hardware this is (in absolute units) I think it's pretty important we get to usable ASAP. Yours, Linus Walleij
On Mon, Sep 05, 2022 at 04:19:22PM +0300, Andy Shevchenko wrote: > (Replied privately to Russell by a mistake) What are you doing? This email you've forwarded is not your own email that you sent to me privately but my reply to LinusW that was on the list. Are you okay, or is this a result of you rushing and not making sure that you're doing what you intend to do? Maybe you need to slow down a bit? > ---------- Forwarded message --------- > From: Russell King (Oracle) <linux@armlinux.org.uk> > Date: Mon, Sep 5, 2022 at 3:50 PM > Subject: Re: [PATCH 6/6] gpio: macsmc: Add IRQ support > To: Linus Walleij <linus.walleij@linaro.org> > Cc: Arnd Bergmann <arnd@arndb.de>, Lee Jones <lee@kernel.org>, Alyssa > Rosenzweig <alyssa@rosenzweig.io>, <asahi@lists.linux.dev>, Bartosz > Golaszewski <brgl@bgdev.pl>, Hector Martin <marcan@marcan.st>, > <linux-arm-kernel@lists.infradead.org>, <linux-gpio@vger.kernel.org>, > Sven Peter <sven@svenpeter.dev> > > > On Fri, Sep 02, 2022 at 03:21:31PM +0200, Linus Walleij wrote: > > On Thu, Sep 1, 2022 at 3:54 PM Russell King <rmk+kernel@armlinux.org.uk> wrote: > > > + DECLARE_BITMAP(irq_enable_shadow, MAX_GPIO); > > > > Please rename irq_unmasked_shadow as it is tracking > > this and not what the irqchip core calls enabled/disabled. > > > > > + DECLARE_BITMAP(irq_enable, MAX_GPIO); > > > > I think this state should be possible to set/get from the irqchip > > core. !irqd_irq_masked(d) on the descriptor, correct me if I'm wrong. > > I think you're getting the two mixed up. irq_enable_shadow > (irq_unmasked_shadow) is updated from the ->irq_mask and ->irq_unmask > callbacaks, and will track !irqd_irq_masked(d) state. So, I think we > can get rid of irq_enable_shadow and just use !irqd_irq_masked(d). > > The irq_enable bit array tracks the state on the SMC, and is used to > indicate whether we need to update that state when we unlock the bus > (which is when the driver talks to the SMC to reconfigure it.) > > So, I think killing irq_enable_shadow and replacing irq_enable with > irq_unmasked would be correct - and going a bit further, > irq_smc_unmasked to show that it's the SMC's status. > > > > +static int macsmc_gpio_event(struct notifier_block *nb, unsigned long event, void *data) > > > +{ > > > + struct macsmc_gpio *smcgp = container_of(nb, struct macsmc_gpio, nb); > > > + u16 type = event >> 16; > > > + u8 offset = (event >> 8) & 0xff; > > > + smc_key key = macsmc_gpio_key(offset); > > > + unsigned long flags; > > > + int ret; > > > + > > > + if (type != SMC_EV_GPIO) > > > + return NOTIFY_DONE; > > > + > > > + if (offset > MAX_GPIO) { > > > + dev_err(smcgp->dev, "GPIO event index %d out of range\n", offset); > > > + return NOTIFY_BAD; > > > + } > > > + > > > + local_irq_save(flags); > > > + ret = generic_handle_domain_irq(smcgp->gc.irq.domain, offset); > > > + local_irq_restore(flags); > > > > Isn't irq_bus_lock/unlock protecting us here already? > > (I might be getting it wrong...) > > Hmm, where does irq_bus_lock get called? Given this function is called > while running a blocking notifier chain, interrupts will not be > disabled on entry to this function. I haven't found a place in the maze > of irq handling code that generic_handle_domain_irq() would end up using > the bus lock/unlock functions - and if they did, with the above IRQ > saving, the kernel would WARN() about calling mutex_lock() with IRQs > disabled. So it doesn't. > > This actually entirely negates any benefit of the kernel trying to mask > or unmask an interrupt in "hardware" while running the handler - since > macsmc_gpio_irq_bus_sync_unlock() won't be called, the state on the SMC > won't get touched. > > > Since this is coming from a notifier and not an IRQ or threaded > > IRQ I actually am a bit puzzled on how to handle it... you probably > > know it better than me, maybe ask Marc Z if anything is > > unclear. > > It's been years since I did any real platform porting work, so deep > knowledge of the IRQ subsystem has evaporated. > > > > + if (apple_smc_write_u32(smcgp->smc, key, CMD_IRQ_ACK | 1) < 0) > > > + dev_err(smcgp->dev, "GPIO IRQ ack failed for %p4ch\n", &key); > > > > isn't this one of those cases where we should implement the > > irqchip callback .irq_ack() specifically for this? > > > > That callback will only be used by edge triggered IRQs but > > I guess that would realistically be all we support anyway? > > (See comment below on .set_type) > > I would imagine it depends on how the SMC GPIO interrupt works - > whether the ACK is ACK as we know it in Linux (x86 PIC) or whether > it's ACK as in a notification to the SMC that we have finished > handling the interrupt and it can send us the next interrupt. > > I suspect we don't know that level of detail of the platform, so > given that this is what the Asahi kernel does, that's the best we > have. > > > > +static int macsmc_gpio_irq_set_type(struct irq_data *d, unsigned int type) > > > +{ > > > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > > > + struct macsmc_gpio *smcgp = gpiochip_get_data(gc); > > > + int offset = irqd_to_hwirq(d); > > > + u32 mode; > > > + > > > + if (!test_bit(offset, smcgp->irq_supported)) > > > + return -EINVAL; > > > + > > > + switch (type & IRQ_TYPE_SENSE_MASK) { > > > + case IRQ_TYPE_LEVEL_HIGH: > > > + mode = IRQ_MODE_HIGH; > > > + break; > > > + case IRQ_TYPE_LEVEL_LOW: > > > + mode = IRQ_MODE_LOW; > > > + break; > > > + case IRQ_TYPE_EDGE_RISING: > > > + mode = IRQ_MODE_RISING; > > > + break; > > > + case IRQ_TYPE_EDGE_FALLING: > > > + mode = IRQ_MODE_FALLING; > > > + break; > > > + case IRQ_TYPE_EDGE_BOTH: > > > + mode = IRQ_MODE_BOTH; > > > + break; > > > + default: > > > + return -EINVAL; > > > > I don't know how level IRQs would work on this essentially > > message-passing process context interrupt. Maybe I am getting > > it all wrong, but for level the line should be held low/high until > > the IRQ is serviced, it would be possible to test if this actually > > works by *not* servicing an IRQ and see if the SMC then sends > > another message notifier for the same IRQ. > > If level IRQs are not supported, then it's strange that the Asahi > folk have been able to reverse engineer the CMD_IRQ_MODE codes for > these states. > > Maybe the SMC issues another message for a level IRQ after it receives > a CMD_IRQ_ACK message if the level interrupt is still asserted? > > > I strongly suspect that actually only edges are supported, but > > there might be semantics I don't understand here. > > > > > } > > > > > > + smcgp->irq_mode_shadow[offset] = mode; > > > > Hm yeah I guess this shadow mode is necessary for the sync > > to work. > > Ineed. > > > > +static void macsmc_gpio_irq_bus_sync_unlock(struct irq_data *d) > > > +{ > > > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > > > + struct macsmc_gpio *smcgp = gpiochip_get_data(gc); > > > + smc_key key = macsmc_gpio_key(irqd_to_hwirq(d)); > > > + int offset = irqd_to_hwirq(d); > > > + bool val; > > > + > > > + if (smcgp->irq_mode_shadow[offset] != smcgp->irq_mode[offset]) { > > > + u32 cmd = CMD_IRQ_MODE | smcgp->irq_mode_shadow[offset]; > > > + if (apple_smc_write_u32(smcgp->smc, key, cmd) < 0) > > > + dev_err(smcgp->dev, "GPIO IRQ config failed for %p4ch = 0x%x\n", &key, cmd); > > > + else > > > + smcgp->irq_mode_shadow[offset] = smcgp->irq_mode[offset]; > > > + } > > > + > > > + val = test_bit(offset, smcgp->irq_enable_shadow); > > > + if (test_bit(offset, smcgp->irq_enable) != val) { > > > > So what you want to know for each line is (correct me if I'm wrong): > > - Is it enabled (unmasked) or not? > > - Did it get changed enabled->disabled, disabled->enabled since > > macsmc_gpio_irq_bus_lock()? > > I think you mean here "did the 'enable' state change between > macsmc_gpio_irq_bus_lock() and macsmc_gpio_irq_bus_unlock". > > > Doesn't the irqchip core track the first part of this for you? > > irqd_irq_masked(d) should tell you the same thing as > > irq_enable, just inverted. > > > > irq_enable_shadow is a bit tricker, I guess you might need that since > > the irqchip doesn't track state changes. > > Yep, which is what I've said above in this reply where these bitmaps > are declared. > > > > static int macsmc_gpio_probe(struct platform_device *pdev) > > > { > > > struct macsmc_gpio *smcgp; > > > @@ -221,6 +365,18 @@ static int macsmc_gpio_probe(struct platform_device *pdev) > > > smcgp->gc.base = -1; > > > smcgp->gc.parent = &pdev->dev; > > > > > > + gpio_irq_chip_set_chip(&smcgp->gc.irq, &macsmc_gpio_irqchip); > > > + smcgp->gc.irq.parent_handler = NULL; > > > + smcgp->gc.irq.num_parents = 0; > > > + smcgp->gc.irq.parents = NULL; > > > + smcgp->gc.irq.default_type = IRQ_TYPE_NONE; > > > + smcgp->gc.irq.handler = handle_simple_irq; > > > > I would consider setting this to handle_edge_irq() and implement > > .irq_ack(). I might be wrong. > > I don't think that's suitable, because then we'll be calling irq_ack() > before the handler has run - and we won't be notifying the SMC that > the interrupt has been masked. So it could send another notification > for the same IRQ while it's still being handled. Then there's the > question about level IRQs as well. > > I think, given that I don't know how the SMC works (presumably the > Asahi folk have a bit more of an idea, but that will be based on > reverse engineering effort) that I am not going to modify this driver's > behaviour drastically by changing the flow handler to the edge flow > handler from the simple flow. To me, that could well be a disaster > for this driver. That would be something for the Asahi folk to look > at. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! > > > -- > With Best Regards, > Andy Shevchenko > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
On 02/09/2022 22.21, Linus Walleij wrote: >> + switch (type & IRQ_TYPE_SENSE_MASK) { >> + case IRQ_TYPE_LEVEL_HIGH: >> + mode = IRQ_MODE_HIGH; >> + break; >> + case IRQ_TYPE_LEVEL_LOW: >> + mode = IRQ_MODE_LOW; >> + break; >> + case IRQ_TYPE_EDGE_RISING: >> + mode = IRQ_MODE_RISING; >> + break; >> + case IRQ_TYPE_EDGE_FALLING: >> + mode = IRQ_MODE_FALLING; >> + break; >> + case IRQ_TYPE_EDGE_BOTH: >> + mode = IRQ_MODE_BOTH; >> + break; >> + default: >> + return -EINVAL; > > I don't know how level IRQs would work on this essentially > message-passing process context interrupt. Maybe I am getting > it all wrong, but for level the line should be held low/high until > the IRQ is serviced, it would be possible to test if this actually > works by *not* servicing an IRQ and see if the SMC then sends > another message notifier for the same IRQ. > > I strongly suspect that actually only edges are supported, but > there might be semantics I don't understand here. IIRC that is exactly what happens - the SMC will re-fire the IRQ after the ACK if it is set to level mode and still at the active level. I do remember testing all the modes carefully when implementing this to figure out what the precise semantics are, and I *think* I agonized over the flow handlers quite a bit and decided this way would work properly for all the modes, but it's been a while so I'd have to take a look again to convince myself again :) - Hector
On Tue, Sep 06, 2022 at 04:00:31PM +0900, Hector Martin wrote: > On 02/09/2022 22.21, Linus Walleij wrote: > >> + switch (type & IRQ_TYPE_SENSE_MASK) { > >> + case IRQ_TYPE_LEVEL_HIGH: > >> + mode = IRQ_MODE_HIGH; > >> + break; > >> + case IRQ_TYPE_LEVEL_LOW: > >> + mode = IRQ_MODE_LOW; > >> + break; > >> + case IRQ_TYPE_EDGE_RISING: > >> + mode = IRQ_MODE_RISING; > >> + break; > >> + case IRQ_TYPE_EDGE_FALLING: > >> + mode = IRQ_MODE_FALLING; > >> + break; > >> + case IRQ_TYPE_EDGE_BOTH: > >> + mode = IRQ_MODE_BOTH; > >> + break; > >> + default: > >> + return -EINVAL; > > > > I don't know how level IRQs would work on this essentially > > message-passing process context interrupt. Maybe I am getting > > it all wrong, but for level the line should be held low/high until > > the IRQ is serviced, it would be possible to test if this actually > > works by *not* servicing an IRQ and see if the SMC then sends > > another message notifier for the same IRQ. > > > > I strongly suspect that actually only edges are supported, but > > there might be semantics I don't understand here. > > IIRC that is exactly what happens - the SMC will re-fire the IRQ after > the ACK if it is set to level mode and still at the active level. > > I do remember testing all the modes carefully when implementing this to > figure out what the precise semantics are, and I *think* I agonized over > the flow handlers quite a bit and decided this way would work properly > for all the modes, but it's been a while so I'd have to take a look > again to convince myself again :) Thanks for the clarification - I think it would be useful to put some of that as comments before the CMD_IRQ_ACK write to head off any questions about this in the future. Something like this maybe? /* * This is not an "ack" int he i8253 PIC sense - it is used for level * interrupts as well. The SMC will re-fire the interrupt event after * this ACK if the level interrupt is still active. */ if (apple_smc_write_u32(smcgp->smc, key, CMD_IRQ_ACK | 1) < 0) dev_err(smcgp->dev, "GPIO IRQ ack failed for %p4ch\n", &key);
On 06/09/2022 16.47, Russell King (Oracle) wrote: > On Tue, Sep 06, 2022 at 04:00:31PM +0900, Hector Martin wrote: >> On 02/09/2022 22.21, Linus Walleij wrote: >>>> + switch (type & IRQ_TYPE_SENSE_MASK) { >>>> + case IRQ_TYPE_LEVEL_HIGH: >>>> + mode = IRQ_MODE_HIGH; >>>> + break; >>>> + case IRQ_TYPE_LEVEL_LOW: >>>> + mode = IRQ_MODE_LOW; >>>> + break; >>>> + case IRQ_TYPE_EDGE_RISING: >>>> + mode = IRQ_MODE_RISING; >>>> + break; >>>> + case IRQ_TYPE_EDGE_FALLING: >>>> + mode = IRQ_MODE_FALLING; >>>> + break; >>>> + case IRQ_TYPE_EDGE_BOTH: >>>> + mode = IRQ_MODE_BOTH; >>>> + break; >>>> + default: >>>> + return -EINVAL; >>> >>> I don't know how level IRQs would work on this essentially >>> message-passing process context interrupt. Maybe I am getting >>> it all wrong, but for level the line should be held low/high until >>> the IRQ is serviced, it would be possible to test if this actually >>> works by *not* servicing an IRQ and see if the SMC then sends >>> another message notifier for the same IRQ. >>> >>> I strongly suspect that actually only edges are supported, but >>> there might be semantics I don't understand here. >> >> IIRC that is exactly what happens - the SMC will re-fire the IRQ after >> the ACK if it is set to level mode and still at the active level. >> >> I do remember testing all the modes carefully when implementing this to >> figure out what the precise semantics are, and I *think* I agonized over >> the flow handlers quite a bit and decided this way would work properly >> for all the modes, but it's been a while so I'd have to take a look >> again to convince myself again :) > > Thanks for the clarification - I think it would be useful to put some > of that as comments before the CMD_IRQ_ACK write to head off any > questions about this in the future. Something like this maybe? > > /* > * This is not an "ack" int he i8253 PIC sense - it is used for level > * interrupts as well. The SMC will re-fire the interrupt event after > * this ACK if the level interrupt is still active. > */ > if (apple_smc_write_u32(smcgp->smc, key, CMD_IRQ_ACK | 1) < 0) > dev_err(smcgp->dev, "GPIO IRQ ack failed for %p4ch\n", &key); > Sounds good to me! (nit: typo on "int he") - Hector
diff --git a/drivers/gpio/gpio-macsmc.c b/drivers/gpio/gpio-macsmc.c index ff9950afb69a..939e2dc33c6f 100644 --- a/drivers/gpio/gpio-macsmc.c +++ b/drivers/gpio/gpio-macsmc.c @@ -10,6 +10,7 @@ #include <linux/bitmap.h> #include <linux/device.h> #include <linux/gpio/driver.h> +#include <linux/irq.h> #include <linux/mfd/core.h> #include <linux/mfd/macsmc.h> @@ -68,10 +69,20 @@ * 3 = ? */ +#define SMC_EV_GPIO 0x7202 + struct macsmc_gpio { struct device *dev; struct apple_smc *smc; struct gpio_chip gc; + struct notifier_block nb; + + struct mutex irq_mutex; + DECLARE_BITMAP(irq_supported, MAX_GPIO); + DECLARE_BITMAP(irq_enable_shadow, MAX_GPIO); + DECLARE_BITMAP(irq_enable, MAX_GPIO); + u32 irq_mode_shadow[MAX_GPIO]; + u32 irq_mode[MAX_GPIO]; int first_index; }; @@ -161,6 +172,7 @@ static int macsmc_gpio_init_valid_mask(struct gpio_chip *gc, for (i = 0; i < count; i++) { smc_key key; int gpio_nr; + u32 val; int ret = apple_smc_get_key_by_index(smcgp->smc, smcgp->first_index + i, &key); if (ret < 0) @@ -176,11 +188,143 @@ static int macsmc_gpio_init_valid_mask(struct gpio_chip *gc, } set_bit(gpio_nr, valid_mask); + + /* Check for IRQ support */ + ret = apple_smc_rw_u32(smcgp->smc, key, CMD_IRQ_MODE, &val); + if (!ret) + set_bit(gpio_nr, smcgp->irq_supported); + } + + return 0; +} + +static int macsmc_gpio_event(struct notifier_block *nb, unsigned long event, void *data) +{ + struct macsmc_gpio *smcgp = container_of(nb, struct macsmc_gpio, nb); + u16 type = event >> 16; + u8 offset = (event >> 8) & 0xff; + smc_key key = macsmc_gpio_key(offset); + unsigned long flags; + int ret; + + if (type != SMC_EV_GPIO) + return NOTIFY_DONE; + + if (offset > MAX_GPIO) { + dev_err(smcgp->dev, "GPIO event index %d out of range\n", offset); + return NOTIFY_BAD; + } + + local_irq_save(flags); + ret = generic_handle_domain_irq(smcgp->gc.irq.domain, offset); + local_irq_restore(flags); + + if (apple_smc_write_u32(smcgp->smc, key, CMD_IRQ_ACK | 1) < 0) + dev_err(smcgp->dev, "GPIO IRQ ack failed for %p4ch\n", &key); + + return (ret == 0) ? NOTIFY_OK : NOTIFY_DONE; +} + +static void macsmc_gpio_irq_enable(struct irq_data *d) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + struct macsmc_gpio *smcgp = gpiochip_get_data(gc); + + gpiochip_enable_irq(gc, irqd_to_hwirq(d)); + set_bit(irqd_to_hwirq(d), smcgp->irq_enable_shadow); +} + +static void macsmc_gpio_irq_disable(struct irq_data *d) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + struct macsmc_gpio *smcgp = gpiochip_get_data(gc); + + clear_bit(irqd_to_hwirq(d), smcgp->irq_enable_shadow); + gpiochip_disable_irq(gc, irqd_to_hwirq(d)); +} + +static int macsmc_gpio_irq_set_type(struct irq_data *d, unsigned int type) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + struct macsmc_gpio *smcgp = gpiochip_get_data(gc); + int offset = irqd_to_hwirq(d); + u32 mode; + + if (!test_bit(offset, smcgp->irq_supported)) + return -EINVAL; + + switch (type & IRQ_TYPE_SENSE_MASK) { + case IRQ_TYPE_LEVEL_HIGH: + mode = IRQ_MODE_HIGH; + break; + case IRQ_TYPE_LEVEL_LOW: + mode = IRQ_MODE_LOW; + break; + case IRQ_TYPE_EDGE_RISING: + mode = IRQ_MODE_RISING; + break; + case IRQ_TYPE_EDGE_FALLING: + mode = IRQ_MODE_FALLING; + break; + case IRQ_TYPE_EDGE_BOTH: + mode = IRQ_MODE_BOTH; + break; + default: + return -EINVAL; } + smcgp->irq_mode_shadow[offset] = mode; return 0; } +static void macsmc_gpio_irq_bus_lock(struct irq_data *d) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + struct macsmc_gpio *smcgp = gpiochip_get_data(gc); + + mutex_lock(&smcgp->irq_mutex); +} + +static void macsmc_gpio_irq_bus_sync_unlock(struct irq_data *d) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + struct macsmc_gpio *smcgp = gpiochip_get_data(gc); + smc_key key = macsmc_gpio_key(irqd_to_hwirq(d)); + int offset = irqd_to_hwirq(d); + bool val; + + if (smcgp->irq_mode_shadow[offset] != smcgp->irq_mode[offset]) { + u32 cmd = CMD_IRQ_MODE | smcgp->irq_mode_shadow[offset]; + if (apple_smc_write_u32(smcgp->smc, key, cmd) < 0) + dev_err(smcgp->dev, "GPIO IRQ config failed for %p4ch = 0x%x\n", &key, cmd); + else + smcgp->irq_mode_shadow[offset] = smcgp->irq_mode[offset]; + } + + val = test_bit(offset, smcgp->irq_enable_shadow); + if (test_bit(offset, smcgp->irq_enable) != val) { + if (apple_smc_write_u32(smcgp->smc, key, CMD_IRQ_ENABLE | val) < 0) + dev_err(smcgp->dev, "GPIO IRQ en/disable failed for %p4ch\n", &key); + else + change_bit(offset, smcgp->irq_enable); + } + + mutex_unlock(&smcgp->irq_mutex); +} + +static const struct irq_chip macsmc_gpio_irqchip = { + .name = "macsmc-pmu-gpio", + .irq_mask = macsmc_gpio_irq_disable, + .irq_unmask = macsmc_gpio_irq_enable, + .irq_set_type = macsmc_gpio_irq_set_type, + .irq_bus_lock = macsmc_gpio_irq_bus_lock, + .irq_bus_sync_unlock = macsmc_gpio_irq_bus_sync_unlock, + .irq_set_type = macsmc_gpio_irq_set_type, + .flags = IRQCHIP_SET_TYPE_MASKED | + IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_IMMUTABLE, + GPIOCHIP_IRQ_RESOURCE_HELPERS, +}; + static int macsmc_gpio_probe(struct platform_device *pdev) { struct macsmc_gpio *smcgp; @@ -221,6 +365,18 @@ static int macsmc_gpio_probe(struct platform_device *pdev) smcgp->gc.base = -1; smcgp->gc.parent = &pdev->dev; + gpio_irq_chip_set_chip(&smcgp->gc.irq, &macsmc_gpio_irqchip); + smcgp->gc.irq.parent_handler = NULL; + smcgp->gc.irq.num_parents = 0; + smcgp->gc.irq.parents = NULL; + smcgp->gc.irq.default_type = IRQ_TYPE_NONE; + smcgp->gc.irq.handler = handle_simple_irq; + + mutex_init(&smcgp->irq_mutex); + + smcgp->nb.notifier_call = macsmc_gpio_event; + apple_smc_register_notifier(smc, &smcgp->nb); + return devm_gpiochip_add_data(&pdev->dev, &smcgp->gc, smcgp); }