> +static int
> +mlxbf2_gpio_irq_set_type(struct irq_data *irqd, unsigned int type)
> +{
> + struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
> + struct mlxbf2_gpio_context *gs = gpiochip_get_data(gc);
> + int offset = irqd_to_hwirq(irqd);
> + unsigned long flags;
> + bool fall = false;
> + bool rise = false;
> + u32 val;
> +
> + switch (type & IRQ_TYPE_SENSE_MASK) {
> + case IRQ_TYPE_EDGE_BOTH:
> + fall = true;
> + rise = true;
> + break;
> + case IRQ_TYPE_EDGE_RISING:
> + rise = true;
> + break;
> + case IRQ_TYPE_EDGE_FALLING:
> + fall = true;
> + break;
> + default:
> + return -EINVAL;
> + }
What PHY are you using? I think every one i've looked at are level
triggered, not edge. Using an edge interrupt might work 99% of the
time, but when the timing is just wrong, you can loose an interrupt.
Which might mean phylib thinks the link is down, when it fact it is
up. You will need to unplug and replug to recover from that.
Andrew
> +static int
> +mlxbf2_gpio_irq_set_type(struct irq_data *irqd, unsigned int type) {
> +
> + switch (type & IRQ_TYPE_SENSE_MASK) {
> + case IRQ_TYPE_EDGE_BOTH:
> + fall = true;
> + rise = true;
> + break;
> + case IRQ_TYPE_EDGE_RISING:
> + rise = true;
> + break;
> + case IRQ_TYPE_EDGE_FALLING:
> + fall = true;
> + break;
> + default:
> + return -EINVAL;
> + }
> What PHY are you using? I think every one i've looked at are
> level triggered, not edge. Using an edge interrupt might work 99%
> of the time, but when the timing is just wrong, you can loose an interrupt.
> Which might mean phylib thinks the link is down, when it fact it is up.
> You will need to unplug and replug to recover from that.
It is the micrel PHY KSZ9031 so it is an active low level interrupt.
Here, IRQ_TYPE_EDGE* macros are mainly used to decide whether to write the
YU_GPIO_CAUSE_FALL_EN register vs the YU_GPIO_CAUSE_RISE_EN register.
These 2 registers are used in both LEVEL/EDGE interrupts.
So I will add back the IRQ_TYPE_LEVEL_LOW and IRQ_TYPE_LEVEL_HIGH as I
did before to configure YU_GPIO_CAUSE_FALL_EN and
YU_GPIO_CAUSE_HIGH_EN respectively. The PHY interrupt signal is physically
Connected to an open drain GPIO pin so software only needs to set
YU_GPIO_CAUSE_FALL_EN register in this case.
Thanks.
Asmaa
On Wed, Sep 22, 2021 at 02:16:40PM +0000, Asmaa Mnebhi wrote:
>
> > +static int
> > +mlxbf2_gpio_irq_set_type(struct irq_data *irqd, unsigned int type) {
> > +
> > + switch (type & IRQ_TYPE_SENSE_MASK) {
> > + case IRQ_TYPE_EDGE_BOTH:
> > + fall = true;
> > + rise = true;
> > + break;
> > + case IRQ_TYPE_EDGE_RISING:
> > + rise = true;
> > + break;
> > + case IRQ_TYPE_EDGE_FALLING:
> > + fall = true;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
>
> > What PHY are you using? I think every one i've looked at are
> > level triggered, not edge. Using an edge interrupt might work 99%
> > of the time, but when the timing is just wrong, you can loose an interrupt.
> > Which might mean phylib thinks the link is down, when it fact it is up.
> > You will need to unplug and replug to recover from that.
>
> It is the micrel PHY KSZ9031 so it is an active low level interrupt.
> Here, IRQ_TYPE_EDGE* macros are mainly used to decide whether to write the
> YU_GPIO_CAUSE_FALL_EN register vs the YU_GPIO_CAUSE_RISE_EN register.
> These 2 registers are used in both LEVEL/EDGE interrupts.
I assume you also have an YU_GPIO_CAUSE_LOW_EN and
YU_GPIO_CAUSE_HIGH_EN registers? These registers need to be set for
IRQ_TYPE_LEVEL_LOW and IRQ_TYPE_LEVEL_HIGH.
Andrew
> > +static int
> > +mlxbf2_gpio_irq_set_type(struct irq_data *irqd, unsigned int type)
> > +{
> > +
> > + switch (type & IRQ_TYPE_SENSE_MASK) {
> > + case IRQ_TYPE_EDGE_BOTH:
> > + fall = true;
> > + rise = true;
> > + break;
> > + case IRQ_TYPE_EDGE_RISING:
> > + rise = true;
> > + break;
> > + case IRQ_TYPE_EDGE_FALLING:
> > + fall = true;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
>
> > What PHY are you using? I think every one i've looked at are level
> > triggered, not edge. Using an edge interrupt might work 99% of the
> > time, but when the timing is just wrong, you can loose an interrupt.
> > Which might mean phylib thinks the link is down, when it fact it is up.
> > You will need to unplug and replug to recover from that.
>
> It is the micrel PHY KSZ9031 so it is an active low level interrupt.
> Here, IRQ_TYPE_EDGE* macros are mainly used to decide whether to write
> the YU_GPIO_CAUSE_FALL_EN register vs the YU_GPIO_CAUSE_RISE_EN register.
> These 2 registers are used in both LEVEL/EDGE interrupts.
> I assume you also have an YU_GPIO_CAUSE_LOW_EN and
> YU_GPIO_CAUSE_HIGH_EN registers? These registers need to
> be set for IRQ_TYPE_LEVEL_LOW and IRQ_TYPE_LEVEL_HIGH.
No we don't. I double checked with the HW team and they confirmed that
YU_GPIO_CAUSE_FALL_EN and YU_GPIO_CAUSE_RISE_EN are used in
Both level and edge interrupts cases.
Thanks.
Asnaa
> No we don't. I double checked with the HW team and they confirmed that
> YU_GPIO_CAUSE_FALL_EN and YU_GPIO_CAUSE_RISE_EN are used in
> Both level and edge interrupts cases.
How? They are different things.
I suggest you test this. Make sure a level interrupt real does fire on
level. One simple test is use a resistor to force the interrupt pin
low. Your machine should then die in an interrupt storm, until the
kernel declares the interrupt broken and disables it.
Andrew