| Message ID | 20230509022734.148970-7-jiawenwu@trustnetic.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | TXGBE PHYLINK support | expand |
> +static int txgbe_gpio_get(struct gpio_chip *chip, unsigned int offset) > +{ > + struct wx *wx = gpiochip_get_data(chip); > + struct txgbe *txgbe = wx->priv; > + int val; > + > + val = rd32m(wx, WX_GPIO_EXT, BIT(offset)); > + > + txgbe->gpio_orig &= ~BIT(offset); > + txgbe->gpio_orig |= val; > + > + return !!(val & BIT(offset)); > +} > +static void txgbe_irq_handler(struct irq_desc *desc) > +{ > + struct irq_chip *chip = irq_desc_get_chip(desc); > + struct wx *wx = irq_desc_get_handler_data(desc); > + struct txgbe *txgbe = wx->priv; > + irq_hw_number_t hwirq; > + unsigned long gpioirq; > + struct gpio_chip *gc; > + u32 gpio; > + > + chained_irq_enter(chip, desc); > + > + gpioirq = rd32(wx, WX_GPIO_INTSTATUS); > + > + /* workaround for hysteretic gpio interrupts */ > + gpio = rd32(wx, WX_GPIO_EXT); > + if (!gpioirq) > + gpioirq = txgbe->gpio_orig ^ gpio; Please could you expand on the comment. Are you saying that WX_GPIO_INTSTATUS sometimes does not contain the GPIO which caused the interrupt? If so, you then compare the last gpio_get with the current value and assume that is what caused the interrupt? > + > + gc = txgbe->gpio; > + for_each_set_bit(hwirq, &gpioirq, gc->ngpio) > + generic_handle_domain_irq(gc->irq.domain, hwirq); > + > + chained_irq_exit(chip, desc); > + > + /* unmask interrupt */ > + if (netif_running(wx->netdev)) > + wx_intr_enable(wx, TXGBE_INTR_MISC(wx)); Is that a hardware requirement, that interrupts only work when the interface is running? Interrupts are not normally conditional like this, at least when the SoC provides the GPIO controller. Andrew --- pw-bot: cr
> +static int txgbe_gpio_set_type(struct irq_data *d, unsigned int type) > +{ > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > + irq_hw_number_t hwirq = irqd_to_hwirq(d); > + struct wx *wx = gpiochip_get_data(gc); > + u32 level, polarity; > + > + level = rd32(wx, WX_GPIO_INTTYPE_LEVEL); > + polarity = rd32(wx, WX_GPIO_POLARITY); > + > + switch (type) { > + case IRQ_TYPE_EDGE_BOTH: > + level |= BIT(hwirq); > + break; > + case IRQ_TYPE_EDGE_RISING: > + level |= BIT(hwirq); > + polarity |= BIT(hwirq); > + break; > + case IRQ_TYPE_EDGE_FALLING: > + level |= BIT(hwirq); > + polarity &= ~BIT(hwirq); > + break; > + case IRQ_TYPE_LEVEL_HIGH: > + level &= ~BIT(hwirq); > + polarity |= BIT(hwirq); > + break; > + case IRQ_TYPE_LEVEL_LOW: > + level &= ~BIT(hwirq); > + polarity &= ~BIT(hwirq); > + break; > + } You have two configuration bits, level and polarity, yet handle 5 different types? > + wr32m(wx, WX_GPIO_INTEN, BIT(hwirq), BIT(hwirq)); > + wr32(wx, WX_GPIO_INTTYPE_LEVEL, level); > + if (type != IRQ_TYPE_EDGE_BOTH) > + wr32(wx, WX_GPIO_POLARITY, polarity); If we are interested in both edges, then polarity is meaningless. So i can understand not writing it. But how does the hardware know polarity should not be used? Andrew
Tue, May 09, 2023 at 10:27:31AM +0800, Jiawen Wu kirjoitti: > Register GPIO chip and handle GPIO IRQ for SFP socket. ... > +#include <linux/gpio/consumer.h> What for? > +#include <linux/gpio/machine.h> > +#include <linux/gpio/driver.h> ... > +static int txgbe_gpio_get(struct gpio_chip *chip, unsigned int offset) > +{ > + struct wx *wx = gpiochip_get_data(chip); > + struct txgbe *txgbe = wx->priv; > + int val; > + > + val = rd32m(wx, WX_GPIO_EXT, BIT(offset)); > + > + txgbe->gpio_orig &= ~BIT(offset); > + txgbe->gpio_orig |= val; This can use standard pattern in conjunction with simple rd32() call: txgbe->gpio_orig = (txgbe->gpio_orig & ~BIT(offset)) | (val & BIT(offset)); otherwise it's not immediately obvious that val can have only one bit set. > + return !!(val & BIT(offset)); > +} ... > +static int txgbe_gpio_direction_out(struct gpio_chip *chip, unsigned int offset, > + int val) > +{ > + struct wx *wx = gpiochip_get_data(chip); > + u32 mask; > + > + mask = BIT(offset) | BIT(offset - 1); > + if (val) > + wr32m(wx, WX_GPIO_DR, mask, mask); > + else > + wr32m(wx, WX_GPIO_DR, mask, 0); > + > + wr32m(wx, WX_GPIO_DDR, BIT(offset), BIT(offset)); Can you explain, what prevents to have this flow to be interleaved by other API calls, like ->direction_in()? Didn't you missed proper locking schema? > + return 0; > +} ... > + switch (type) { > + case IRQ_TYPE_EDGE_BOTH: > + level |= BIT(hwirq); > + break; > + case IRQ_TYPE_EDGE_RISING: > + level |= BIT(hwirq); > + polarity |= BIT(hwirq); > + break; > + case IRQ_TYPE_EDGE_FALLING: > + level |= BIT(hwirq); > + polarity &= ~BIT(hwirq); This... > + break; > + case IRQ_TYPE_LEVEL_HIGH: > + level &= ~BIT(hwirq); ...and this can be done outside of the switch-case. Then you simply set certain bits where it's needed. > + polarity |= BIT(hwirq); > + break; > + case IRQ_TYPE_LEVEL_LOW: > + level &= ~BIT(hwirq); > + polarity &= ~BIT(hwirq); > + break; default? > + } ... > + /* workaround for hysteretic gpio interrupts */ GPIO ... > + gc->can_sleep = false; Useless, kzalloc() already sets this to 0. ... > + girq->num_parents = 1; > + girq->parents = devm_kcalloc(&pdev->dev, 1, sizeof(*girq->parents), Use girq->num_parents instead of explicit 1 in this call. > + GFP_KERNEL); Also with struct device *dev = &pdev->dev; this (and others) can be modified as girq->parents = devm_kcalloc(dev, girq->num_parents, sizeof(*girq->parents), > + if (!girq->parents) > + return -ENOMEM; ... > +#define TXGBE_PX_MISC_IEN_MASK ( \ > + TXGBE_PX_MISC_ETH_LKDN | \ > + TXGBE_PX_MISC_DEV_RST | \ > + TXGBE_PX_MISC_ETH_EVENT | \ > + TXGBE_PX_MISC_ETH_LK | \ > + TXGBE_PX_MISC_ETH_AN | \ > + TXGBE_PX_MISC_INT_ERR | \ > + TXGBE_PX_MISC_GPIO) Wouldn't be better #define TXGBE_PX_MISC_IEN_MASK \ (TXGBE_PX_MISC_ETH_LKDN | TXGBE_PX_MISC_ETH_LK | \ TXGBE_PX_MISC_ETH_EVENT | TXGBE_PX_MISC_ETH_AN | \ TXGBE_PX_MISC_DEV_RST | TXGBE_PX_MISC_INT_ERR | \ TXGBE_PX_MISC_GPIO) ?
On Thursday, May 11, 2023 8:39 PM, Andrew Lunn wrote: > > +static int txgbe_gpio_set_type(struct irq_data *d, unsigned int type) > > +{ > > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > > + irq_hw_number_t hwirq = irqd_to_hwirq(d); > > + struct wx *wx = gpiochip_get_data(gc); > > + u32 level, polarity; > > + > > + level = rd32(wx, WX_GPIO_INTTYPE_LEVEL); > > + polarity = rd32(wx, WX_GPIO_POLARITY); > > + > > + switch (type) { > > + case IRQ_TYPE_EDGE_BOTH: > > + level |= BIT(hwirq); > > + break; > > + case IRQ_TYPE_EDGE_RISING: > > + level |= BIT(hwirq); > > + polarity |= BIT(hwirq); > > + break; > > + case IRQ_TYPE_EDGE_FALLING: > > + level |= BIT(hwirq); > > + polarity &= ~BIT(hwirq); > > + break; > > + case IRQ_TYPE_LEVEL_HIGH: > > + level &= ~BIT(hwirq); > > + polarity |= BIT(hwirq); > > + break; > > + case IRQ_TYPE_LEVEL_LOW: > > + level &= ~BIT(hwirq); > > + polarity &= ~BIT(hwirq); > > + break; > > + } > > You have two configuration bits, level and polarity, yet handle 5 different types? > > > + wr32m(wx, WX_GPIO_INTEN, BIT(hwirq), BIT(hwirq)); > > + wr32(wx, WX_GPIO_INTTYPE_LEVEL, level); > > + if (type != IRQ_TYPE_EDGE_BOTH) > > + wr32(wx, WX_GPIO_POLARITY, polarity); > > If we are interested in both edges, then polarity is meaningless. So i can > understand not writing it. But how does the hardware know polarity should not > be used? I will add toggle trigger to set polarity in both edges, to solve the hysteretic interrupts problem that has been bothering me.
On Thursday, May 11, 2023 8:32 PM, Andrew Lunn wrote: > > +static int txgbe_gpio_get(struct gpio_chip *chip, unsigned int > > +offset) { > > + struct wx *wx = gpiochip_get_data(chip); > > + struct txgbe *txgbe = wx->priv; > > + int val; > > + > > + val = rd32m(wx, WX_GPIO_EXT, BIT(offset)); > > + > > + txgbe->gpio_orig &= ~BIT(offset); > > + txgbe->gpio_orig |= val; > > + > > + return !!(val & BIT(offset)); > > +} > > > +static void txgbe_irq_handler(struct irq_desc *desc) { > > + struct irq_chip *chip = irq_desc_get_chip(desc); > > + struct wx *wx = irq_desc_get_handler_data(desc); > > + struct txgbe *txgbe = wx->priv; > > + irq_hw_number_t hwirq; > > + unsigned long gpioirq; > > + struct gpio_chip *gc; > > + u32 gpio; > > + > > + chained_irq_enter(chip, desc); > > + > > + gpioirq = rd32(wx, WX_GPIO_INTSTATUS); > > + > > + /* workaround for hysteretic gpio interrupts */ > > + gpio = rd32(wx, WX_GPIO_EXT); > > + if (!gpioirq) > > + gpioirq = txgbe->gpio_orig ^ gpio; > > Please could you expand on the comment. Are you saying that > WX_GPIO_INTSTATUS sometimes does not contain the GPIO which caused the > interrupt? If so, you then compare the last gpio_get with the current value and > assume that is what caused the interrupt? Yes. Sometime there is a lag in WX_GPIO_INTSTATUS. When the GPIO interrupt cause, the GPIO state has been back to its previous state. So I added this workaround to save some...but only if there are other interrupts at the same time, i.e. txgbe_irq_handler() called. But I will remove it in the next version, because I find a more accurate solution. > > > + > > + gc = txgbe->gpio; > > + for_each_set_bit(hwirq, &gpioirq, gc->ngpio) > > + generic_handle_domain_irq(gc->irq.domain, hwirq); > > + > > + chained_irq_exit(chip, desc); > > + > > + /* unmask interrupt */ > > + if (netif_running(wx->netdev)) > > + wx_intr_enable(wx, TXGBE_INTR_MISC(wx)); > > Is that a hardware requirement, that interrupts only work when the interface is > running? Interrupts are not normally conditional like this, at least when the SoC > provides the GPIO controller. Should we handle the interrupts when interface is not running?
> > +static int txgbe_gpio_direction_out(struct gpio_chip *chip, unsigned int offset, > > + int val) > > +{ > > + struct wx *wx = gpiochip_get_data(chip); > > + u32 mask; > > + > > + mask = BIT(offset) | BIT(offset - 1); > > + if (val) > > + wr32m(wx, WX_GPIO_DR, mask, mask); > > + else > > + wr32m(wx, WX_GPIO_DR, mask, 0); > > + > > + wr32m(wx, WX_GPIO_DDR, BIT(offset), BIT(offset)); > > Can you explain, what prevents to have this flow to be interleaved by other API > calls, like ->direction_in()? Didn't you missed proper locking schema? It's true, I should add spinlock for writing GPIO registers. > > > + return 0; > > +} > > ... > > > + switch (type) { > > + case IRQ_TYPE_EDGE_BOTH: > > + level |= BIT(hwirq); > > + break; > > + case IRQ_TYPE_EDGE_RISING: > > + level |= BIT(hwirq); > > + polarity |= BIT(hwirq); > > + break; > > + case IRQ_TYPE_EDGE_FALLING: > > + level |= BIT(hwirq); > > > + polarity &= ~BIT(hwirq); > > This... > > > + break; > > + case IRQ_TYPE_LEVEL_HIGH: > > + level &= ~BIT(hwirq); > > ...and this can be done outside of the switch-case. Then you simply set certain > bits where it's needed. > > > + polarity |= BIT(hwirq); > > + break; > > + case IRQ_TYPE_LEVEL_LOW: > > + level &= ~BIT(hwirq); > > + polarity &= ~BIT(hwirq); > > + break; > > default? Do you mean that treat IRQ_TYPE_LEVEL_LOW as default case, clear level and polarity firstly, then set the bits in other needed case?
On Tue, May 09, 2023 at 10:27:31AM +0800, Jiawen Wu wrote: > Register GPIO chip and handle GPIO IRQ for SFP socket. > > Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com> > --- > drivers/net/ethernet/wangxun/Kconfig | 2 + > drivers/net/ethernet/wangxun/libwx/wx_lib.c | 3 +- > drivers/net/ethernet/wangxun/libwx/wx_type.h | 2 + > .../net/ethernet/wangxun/txgbe/txgbe_main.c | 20 +- > .../net/ethernet/wangxun/txgbe/txgbe_phy.c | 228 ++++++++++++++++++ > .../net/ethernet/wangxun/txgbe/txgbe_type.h | 27 +++ > 6 files changed, 263 insertions(+), 19 deletions(-) > > diff --git a/drivers/net/ethernet/wangxun/Kconfig b/drivers/net/ethernet/wangxun/Kconfig > index 90812d76181d..73f4492928c0 100644 > --- a/drivers/net/ethernet/wangxun/Kconfig > +++ b/drivers/net/ethernet/wangxun/Kconfig > @@ -41,6 +41,8 @@ config TXGBE > tristate "Wangxun(R) 10GbE PCI Express adapters support" > depends on PCI > select I2C_DESIGNWARE_PLATFORM > + select GPIOLIB_IRQCHIP > + select GPIOLIB > select REGMAP > select COMMON_CLK > select LIBWX > diff --git a/drivers/net/ethernet/wangxun/libwx/wx_lib.c b/drivers/net/ethernet/wangxun/libwx/wx_lib.c > index 1e8d8b7b0c62..590215303d45 100644 > --- a/drivers/net/ethernet/wangxun/libwx/wx_lib.c > +++ b/drivers/net/ethernet/wangxun/libwx/wx_lib.c > @@ -1348,7 +1348,8 @@ void wx_free_irq(struct wx *wx) > free_irq(entry->vector, q_vector); > } > > - free_irq(wx->msix_entries[vector].vector, wx); > + if (wx->mac.type == wx_mac_em) > + free_irq(wx->msix_entries[vector].vector, wx); > } > EXPORT_SYMBOL(wx_free_irq); > > diff --git a/drivers/net/ethernet/wangxun/libwx/wx_type.h b/drivers/net/ethernet/wangxun/libwx/wx_type.h > index 97bce855bc60..d151d6f79022 100644 > --- a/drivers/net/ethernet/wangxun/libwx/wx_type.h > +++ b/drivers/net/ethernet/wangxun/libwx/wx_type.h > @@ -79,7 +79,9 @@ > #define WX_GPIO_INTMASK 0x14834 > #define WX_GPIO_INTTYPE_LEVEL 0x14838 > #define WX_GPIO_POLARITY 0x1483C > +#define WX_GPIO_INTSTATUS 0x14844 > #define WX_GPIO_EOI 0x1484C > +#define WX_GPIO_EXT 0x14850 > > /*********************** Transmit DMA registers **************************/ > /* transmit global control */ > diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c > index e10296abf5b4..ded04e9e136f 100644 > --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c > +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c > @@ -82,6 +82,8 @@ static int txgbe_enumerate_functions(struct wx *wx) > **/ > static void txgbe_irq_enable(struct wx *wx, bool queues) > { > + wr32(wx, WX_PX_MISC_IEN, TXGBE_PX_MISC_IEN_MASK); > + > /* unmask interrupt */ > wx_intr_enable(wx, TXGBE_INTR_MISC(wx)); > if (queues) > @@ -129,17 +131,6 @@ static irqreturn_t txgbe_intr(int __always_unused irq, void *data) > return IRQ_HANDLED; > } > > -static irqreturn_t txgbe_msix_other(int __always_unused irq, void *data) > -{ > - struct wx *wx = data; > - > - /* re-enable the original interrupt state */ > - if (netif_running(wx->netdev)) > - txgbe_irq_enable(wx, false); > - > - return IRQ_HANDLED; > -} > - > /** > * txgbe_request_msix_irqs - Initialize MSI-X interrupts > * @wx: board private structure > @@ -171,13 +162,6 @@ static int txgbe_request_msix_irqs(struct wx *wx) > } > } > > - err = request_irq(wx->msix_entries[vector].vector, > - txgbe_msix_other, 0, netdev->name, wx); > - if (err) { > - wx_err(wx, "request_irq for msix_other failed: %d\n", err); > - goto free_queue_irqs; > - } > - > return 0; > > free_queue_irqs: > diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c > index 23fa7311db45..8085616a9146 100644 > --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c > +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c > @@ -2,6 +2,9 @@ > /* Copyright (c) 2015 - 2023 Beijing WangXun Technology Co., Ltd. */ > > #include <linux/platform_device.h> > +#include <linux/gpio/consumer.h> > +#include <linux/gpio/machine.h> > +#include <linux/gpio/driver.h> > #include <linux/gpio/property.h> > #include <linux/regmap.h> > #include <linux/clkdev.h> > @@ -10,6 +13,7 @@ > #include <linux/pci.h> > > #include "../libwx/wx_type.h" > +#include "../libwx/wx_hw.h" > #include "txgbe_type.h" > #include "txgbe_phy.h" > > @@ -74,6 +78,224 @@ static int txgbe_swnodes_register(struct txgbe *txgbe) > return software_node_register_node_group(nodes->group); > } > > +static int txgbe_gpio_get(struct gpio_chip *chip, unsigned int offset) > +{ > + struct wx *wx = gpiochip_get_data(chip); > + struct txgbe *txgbe = wx->priv; > + int val; > + > + val = rd32m(wx, WX_GPIO_EXT, BIT(offset)); > + > + txgbe->gpio_orig &= ~BIT(offset); > + txgbe->gpio_orig |= val; You seem to be using this as a way to implement triggering interrupts on both levels. Reading the GPIO value using the GPIO functions should not change the interrupt state, so this is wrong. > + > + return !!(val & BIT(offset)); > +} > + > +static int txgbe_gpio_get_direction(struct gpio_chip *chip, unsigned int offset) > +{ > + struct wx *wx = gpiochip_get_data(chip); > + u32 val; > + > + val = rd32(wx, WX_GPIO_DDR); > + if (BIT(offset) & val) > + return GPIO_LINE_DIRECTION_OUT; > + > + return GPIO_LINE_DIRECTION_IN; > +} > + > +static int txgbe_gpio_direction_in(struct gpio_chip *chip, unsigned int offset) > +{ > + struct wx *wx = gpiochip_get_data(chip); > + > + wr32m(wx, WX_GPIO_DDR, BIT(offset), 0); > + > + return 0; > +} > + > +static int txgbe_gpio_direction_out(struct gpio_chip *chip, unsigned int offset, > + int val) > +{ > + struct wx *wx = gpiochip_get_data(chip); > + u32 mask; > + > + mask = BIT(offset) | BIT(offset - 1); > + if (val) > + wr32m(wx, WX_GPIO_DR, mask, mask); > + else > + wr32m(wx, WX_GPIO_DR, mask, 0); Why are you writing two neighbouring bits here? If GPIO 0 is requested to change, offset will be zero, and BIT(-1) is probably not what you want. Moreover, if requesting a change to GPIO 3, BIT(offset - 1) will also hit GPIO 2. Maybe there's a "* 2" missing here? If this code is in fact correct, it needs a comment to explain what's going on here. > + > + wr32m(wx, WX_GPIO_DDR, BIT(offset), BIT(offset)); > + > + return 0; > +} > + > +static void txgbe_gpio_irq_ack(struct irq_data *d) > +{ > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > + irq_hw_number_t hwirq = irqd_to_hwirq(d); > + struct wx *wx = gpiochip_get_data(gc); > + > + wr32(wx, WX_GPIO_EOI, BIT(hwirq)); > +} > + > +static void txgbe_gpio_irq_mask(struct irq_data *d) > +{ > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > + irq_hw_number_t hwirq = irqd_to_hwirq(d); > + struct wx *wx = gpiochip_get_data(gc); > + > + gpiochip_disable_irq(gc, hwirq); > + > + wr32m(wx, WX_GPIO_INTMASK, BIT(hwirq), BIT(hwirq)); > +} > + > +static void txgbe_gpio_irq_unmask(struct irq_data *d) > +{ > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > + irq_hw_number_t hwirq = irqd_to_hwirq(d); > + struct wx *wx = gpiochip_get_data(gc); > + > + gpiochip_enable_irq(gc, hwirq); > + > + wr32m(wx, WX_GPIO_INTMASK, BIT(hwirq), 0); > +} > + > +static int txgbe_gpio_set_type(struct irq_data *d, unsigned int type) > +{ > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > + irq_hw_number_t hwirq = irqd_to_hwirq(d); > + struct wx *wx = gpiochip_get_data(gc); > + u32 level, polarity; > + > + level = rd32(wx, WX_GPIO_INTTYPE_LEVEL); > + polarity = rd32(wx, WX_GPIO_POLARITY); > + > + switch (type) { > + case IRQ_TYPE_EDGE_BOTH: > + level |= BIT(hwirq); > + break; > + case IRQ_TYPE_EDGE_RISING: > + level |= BIT(hwirq); > + polarity |= BIT(hwirq); > + break; > + case IRQ_TYPE_EDGE_FALLING: > + level |= BIT(hwirq); Are you sure you've got this correct. "level" gets set when edge cases are requested and cleared when level cases are requested. It seems that the register really selects edge-mode if the bit is set? Is it described in the documentation as "not level" ? > + polarity &= ~BIT(hwirq); > + break; > + case IRQ_TYPE_LEVEL_HIGH: > + level &= ~BIT(hwirq); > + polarity |= BIT(hwirq); > + break; > + case IRQ_TYPE_LEVEL_LOW: > + level &= ~BIT(hwirq); > + polarity &= ~BIT(hwirq); > + break; > + } > + > + if (type & IRQ_TYPE_LEVEL_MASK) > + irq_set_handler_locked(d, handle_level_irq); > + else if (type & IRQ_TYPE_EDGE_BOTH) > + irq_set_handler_locked(d, handle_edge_irq); So what handler do we end up with if a GPIO is initially requested for a level IRQ, released, and then requested for an edge IRQ? Secondly, a more general comment. You are using the masks here, and we can see from the above that there is a pattern to the setting of level and polarity. Also, IMHO there's a simpler way to do this: u32 level, polarity, mask, val; mask = BIT(hwirq); if (type & IRQ_TYPE_LEVEL_MASK) { level = 0; irq_set_handler_locked(d, handle_level_irq); } else { level = mask; /* fixme: irq_set_handler_locked(handle_edge_irq); ? */ } if (type == IRQ_TYPE_EDGE_RISING || type == IRQ_TYPE_LEVEL_HIGH) polarity = mask; else polarity = 0; /* fixme: what does this register do, and why is it configured * here? */ wr32m(wx, WX_GPIO_INTEN, mask, mask); wr32m(wx, WX_GPIO_INTTYPE_LEVEL, mask, level); if (type != IRQ_TYPE_EDGE_BOTH) wr32m(wx, WX_GPIO_POLARITY, mask, polarity); Now, as for both-edge interrupts, this needs further thought. As I say above, using the _get method to update the internal interrupt state won't be reliable. If the hardware has no way to trigger on both edges, then it's going to take some additional complexity to make this work. Firstly, you need to record which interrupts are using both-edges in the driver, so you know which need extra work in the interrupt handler. Secondly, you still need to configure the polarity as best you can to pick up the first change in state here. That means reading the current GPIO state, and configuring the GPIO polarity correctly here. It's going to be racy with the hardware, so the closer together you can get the GPIO state-to-polarity-set the better in terms of the size of the race window. > +static void txgbe_irq_handler(struct irq_desc *desc) > +{ > + struct irq_chip *chip = irq_desc_get_chip(desc); > + struct wx *wx = irq_desc_get_handler_data(desc); > + struct txgbe *txgbe = wx->priv; > + irq_hw_number_t hwirq; > + unsigned long gpioirq; > + struct gpio_chip *gc; > + u32 gpio; > + > + chained_irq_enter(chip, desc); > + > + gpioirq = rd32(wx, WX_GPIO_INTSTATUS); Does reading INTSTATUS clear down any of the pending status bits? > + > + /* workaround for hysteretic gpio interrupts */ > + gpio = rd32(wx, WX_GPIO_EXT); > + if (!gpioirq) > + gpioirq = txgbe->gpio_orig ^ gpio; This doesn't solve the both-edge case, because this will only get evaluated if some other GPIO also happens to raise an interrupt. For any GPIOs should have both-edge applied, you need to read the current state of the GPIO and program the polarity appropriately, then re-read the GPIO to see if it changed state during that race and handle that as best that can be. The problem is that making both-edge work reliably on hardware that doesn't support both-edge will always be rather racy. > + > + gc = txgbe->gpio; > + for_each_set_bit(hwirq, &gpioirq, gc->ngpio) > + generic_handle_domain_irq(gc->irq.domain, hwirq); > + > + chained_irq_exit(chip, desc); > + > + /* unmask interrupt */ > + if (netif_running(wx->netdev)) > + wx_intr_enable(wx, TXGBE_INTR_MISC(wx)); Why does this depend on whether the network interface is running, and why is it done at the end of the interrupt handler? Maybe this needs a better comment in the code explaining what it's actually doing? Thanks.
On Fri, May 12, 2023 at 11:58 AM Jiawen Wu <jiawenwu@trustnetic.com> wrote: ... > > > + switch (type) { > > > + case IRQ_TYPE_EDGE_BOTH: > > > + level |= BIT(hwirq); > > > + break; > > > + case IRQ_TYPE_EDGE_RISING: > > > + level |= BIT(hwirq); > > > + polarity |= BIT(hwirq); > > > + break; > > > + case IRQ_TYPE_EDGE_FALLING: > > > + level |= BIT(hwirq); > > > > > + polarity &= ~BIT(hwirq); > > > > This... > > > > > + break; > > > + case IRQ_TYPE_LEVEL_HIGH: > > > + level &= ~BIT(hwirq); > > > > ...and this can be done outside of the switch-case. Then you simply set certain > > bits where it's needed. > > > > > + polarity |= BIT(hwirq); > > > + break; > > > + case IRQ_TYPE_LEVEL_LOW: > > > + level &= ~BIT(hwirq); > > > + polarity &= ~BIT(hwirq); > > > + break; > > > > default? > > Do you mean that treat IRQ_TYPE_LEVEL_LOW as default case, clear level and > polarity firstly, then set the bits in other needed case? level &= ... polarity &= ... switch () { case X: level |= ... break; case Y: polarity |= ... break; case Z: ... break; default: ...handle error... }
> > +static int txgbe_gpio_get(struct gpio_chip *chip, unsigned int offset) > > +{ > > + struct wx *wx = gpiochip_get_data(chip); > > + struct txgbe *txgbe = wx->priv; > > + int val; > > + > > + val = rd32m(wx, WX_GPIO_EXT, BIT(offset)); > > + > > + txgbe->gpio_orig &= ~BIT(offset); > > + txgbe->gpio_orig |= val; > > You seem to be using this as a way to implement triggering interrupts on > both levels. Reading the GPIO value using the GPIO functions should not > change the interrupt state, so this is wrong. Yes, I just found the correct way to deal with it. > > +static int txgbe_gpio_direction_out(struct gpio_chip *chip, unsigned int offset, > > + int val) > > +{ > > + struct wx *wx = gpiochip_get_data(chip); > > + u32 mask; > > + > > + mask = BIT(offset) | BIT(offset - 1); > > + if (val) > > + wr32m(wx, WX_GPIO_DR, mask, mask); > > + else > > + wr32m(wx, WX_GPIO_DR, mask, 0); > > Why are you writing two neighbouring bits here? If GPIO 0 is requested > to change, offset will be zero, and BIT(-1) is probably not what you > want. > > Moreover, if requesting a change to GPIO 3, BIT(offset - 1) will also > hit GPIO 2. > > Maybe there's a "* 2" missing here? > > If this code is in fact correct, it needs a comment to explain what's > going on here. GPIO lines description: /* GPIO 0: tx fault * GPIO 1: tx disable * GPIO 2: sfp module absent * GPIO 3: rx signal lost * GPIO 4: rate select 1, 1G(0) 10G(1) * GPIO 5: rate select 0, 1G(0) 10G(1) */ The previous consideration was processing GPIO 0&1, 4&5. The output lines are 1/4/5. Under the persistent misconfiguration of flash, GPIO 0 is treated as the output signal to enable/disable TX laser, together with GPIO 1. And GPIO 4 seems not be used by SFP driver, to change module rate, also this driver does not implement rate switching either. In my understanding, the input GPIO does not call this function, so I put no condition there. But in general, with all GPIO being used correctly, removing these odd codes should work as well. I'll fix it and test it again. > > +static int txgbe_gpio_set_type(struct irq_data *d, unsigned int type) > > +{ > > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > > + irq_hw_number_t hwirq = irqd_to_hwirq(d); > > + struct wx *wx = gpiochip_get_data(gc); > > + u32 level, polarity; > > + > > + level = rd32(wx, WX_GPIO_INTTYPE_LEVEL); > > + polarity = rd32(wx, WX_GPIO_POLARITY); > > + > > + switch (type) { > > + case IRQ_TYPE_EDGE_BOTH: > > + level |= BIT(hwirq); > > + break; > > + case IRQ_TYPE_EDGE_RISING: > > + level |= BIT(hwirq); > > + polarity |= BIT(hwirq); > > + break; > > + case IRQ_TYPE_EDGE_FALLING: > > + level |= BIT(hwirq); > > Are you sure you've got this correct. "level" gets set when edge cases > are requested and cleared when level cases are requested. It seems that > the register really selects edge-mode if the bit is set? Is it described > in the documentation as "not level" ? Yes, the WX_GPIO_INTTYPE_LEVEL register shows that 0 for level-sensitive, 1 for edge-sensitive. > > > + polarity &= ~BIT(hwirq); > > + break; > > + case IRQ_TYPE_LEVEL_HIGH: > > + level &= ~BIT(hwirq); > > + polarity |= BIT(hwirq); > > + break; > > + case IRQ_TYPE_LEVEL_LOW: > > + level &= ~BIT(hwirq); > > + polarity &= ~BIT(hwirq); > > + break; > > + } > > + > > + if (type & IRQ_TYPE_LEVEL_MASK) > > + irq_set_handler_locked(d, handle_level_irq); > > + else if (type & IRQ_TYPE_EDGE_BOTH) > > + irq_set_handler_locked(d, handle_edge_irq); > > So what handler do we end up with if a GPIO is initially requested for > a level IRQ, released, and then requested for an edge IRQ? Sorry I don't know much about it, who controls the IRQ type? In fact, edge IRQ always be requested in my test, and the type is EDGE_BOTH. > > Secondly, a more general comment. You are using the masks here, and we > can see from the above that there is a pattern to the setting of level > and polarity. Also, IMHO there's a simpler way to do this: > > u32 level, polarity, mask, val; > > mask = BIT(hwirq); > > if (type & IRQ_TYPE_LEVEL_MASK) { > level = 0; > irq_set_handler_locked(d, handle_level_irq); > } else { > level = mask; > /* fixme: irq_set_handler_locked(handle_edge_irq); ? */ > } > > if (type == IRQ_TYPE_EDGE_RISING || type == IRQ_TYPE_LEVEL_HIGH) > polarity = mask; > else > polarity = 0; > > /* fixme: what does this register do, and why is it configured > * here? > */ > wr32m(wx, WX_GPIO_INTEN, mask, mask); It enables corresponding GPIO interrupt. > > wr32m(wx, WX_GPIO_INTTYPE_LEVEL, mask, level); > if (type != IRQ_TYPE_EDGE_BOTH) > wr32m(wx, WX_GPIO_POLARITY, mask, polarity); > > Now, as for both-edge interrupts, this needs further thought. As I say > above, using the _get method to update the internal interrupt state > won't be reliable. > > If the hardware has no way to trigger on both edges, then it's going to > take some additional complexity to make this work. Firstly, you need to > record which interrupts are using both-edges in the driver, so you know > which need extra work in the interrupt handler. > > Secondly, you still need to configure the polarity as best you can to > pick up the first change in state here. That means reading the current > GPIO state, and configuring the GPIO polarity correctly here. It's > going to be racy with the hardware, so the closer together you can get > the GPIO state-to-polarity-set the better in terms of the size of the > race window. Thanks for the detailed advice. Hardware can trigger the interrupts on both edges, just set polarity. > > > +static void txgbe_irq_handler(struct irq_desc *desc) > > +{ > > + struct irq_chip *chip = irq_desc_get_chip(desc); > > + struct wx *wx = irq_desc_get_handler_data(desc); > > + struct txgbe *txgbe = wx->priv; > > + irq_hw_number_t hwirq; > > + unsigned long gpioirq; > > + struct gpio_chip *gc; > > + u32 gpio; > > + > > + chained_irq_enter(chip, desc); > > + > > + gpioirq = rd32(wx, WX_GPIO_INTSTATUS); > > Does reading INTSTATUS clear down any of the pending status bits? No, it's read only. Interrupt status bits will be cleared in txgbe_gpio_irq_ack(). > > > + > > + /* workaround for hysteretic gpio interrupts */ > > + gpio = rd32(wx, WX_GPIO_EXT); > > + if (!gpioirq) > > + gpioirq = txgbe->gpio_orig ^ gpio; > > This doesn't solve the both-edge case, because this will only get > evaluated if some other GPIO also happens to raise an interrupt. > > For any GPIOs should have both-edge applied, you need to read the > current state of the GPIO and program the polarity appropriately, > then re-read the GPIO to see if it changed state during that race > and handle that as best that can be. > > The problem is that making both-edge work reliably on hardware that > doesn't support both-edge will always be rather racy. Thanks again, I learned a lot. > > > + > > + gc = txgbe->gpio; > > + for_each_set_bit(hwirq, &gpioirq, gc->ngpio) > > + generic_handle_domain_irq(gc->irq.domain, hwirq); > > + > > + chained_irq_exit(chip, desc); > > + > > + /* unmask interrupt */ > > + if (netif_running(wx->netdev)) > > + wx_intr_enable(wx, TXGBE_INTR_MISC(wx)); > > Why does this depend on whether the network interface is running, and > why is it done at the end of the interrupt handler? Maybe this needs a > better comment in the code explaining what it's actually doing? This register should be written after handling interrupts, otherwise the next Interrupt may not come, by hardware design. And should we handle the interrupts when interface is not running? I didn't think it should do, so I made it conditional.
> > Is that a hardware requirement, that interrupts only work when the interface is > > running? Interrupts are not normally conditional like this, at least when the SoC > > provides the GPIO controller. > > Should we handle the interrupts when interface is not running? You are adding a generic GPIO controller. It could in theory be used for anything. GPIO controlled LEDs, buttons, bit-banging MDIO/SPI/I2C, etc. I would not artificially limit interrupts to just when the interface is up, unless you need to. You should also take a look at the SFP code. When does it enable interrupts? When the SFP device is created, or when phylink_start() is called? SoC GPIO controllers work all the time, so it could be the SFP code just assumes interrupts work as soon as the GPIO is available. With the interface down, try disconnecting/connecting the fibre and see if /sys/kernel/debug/sfpX/state shows it sees LOS change. Andrew
diff --git a/drivers/net/ethernet/wangxun/Kconfig b/drivers/net/ethernet/wangxun/Kconfig index 90812d76181d..73f4492928c0 100644 --- a/drivers/net/ethernet/wangxun/Kconfig +++ b/drivers/net/ethernet/wangxun/Kconfig @@ -41,6 +41,8 @@ config TXGBE tristate "Wangxun(R) 10GbE PCI Express adapters support" depends on PCI select I2C_DESIGNWARE_PLATFORM + select GPIOLIB_IRQCHIP + select GPIOLIB select REGMAP select COMMON_CLK select LIBWX diff --git a/drivers/net/ethernet/wangxun/libwx/wx_lib.c b/drivers/net/ethernet/wangxun/libwx/wx_lib.c index 1e8d8b7b0c62..590215303d45 100644 --- a/drivers/net/ethernet/wangxun/libwx/wx_lib.c +++ b/drivers/net/ethernet/wangxun/libwx/wx_lib.c @@ -1348,7 +1348,8 @@ void wx_free_irq(struct wx *wx) free_irq(entry->vector, q_vector); } - free_irq(wx->msix_entries[vector].vector, wx); + if (wx->mac.type == wx_mac_em) + free_irq(wx->msix_entries[vector].vector, wx); } EXPORT_SYMBOL(wx_free_irq); diff --git a/drivers/net/ethernet/wangxun/libwx/wx_type.h b/drivers/net/ethernet/wangxun/libwx/wx_type.h index 97bce855bc60..d151d6f79022 100644 --- a/drivers/net/ethernet/wangxun/libwx/wx_type.h +++ b/drivers/net/ethernet/wangxun/libwx/wx_type.h @@ -79,7 +79,9 @@ #define WX_GPIO_INTMASK 0x14834 #define WX_GPIO_INTTYPE_LEVEL 0x14838 #define WX_GPIO_POLARITY 0x1483C +#define WX_GPIO_INTSTATUS 0x14844 #define WX_GPIO_EOI 0x1484C +#define WX_GPIO_EXT 0x14850 /*********************** Transmit DMA registers **************************/ /* transmit global control */ diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c index e10296abf5b4..ded04e9e136f 100644 --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c @@ -82,6 +82,8 @@ static int txgbe_enumerate_functions(struct wx *wx) **/ static void txgbe_irq_enable(struct wx *wx, bool queues) { + wr32(wx, WX_PX_MISC_IEN, TXGBE_PX_MISC_IEN_MASK); + /* unmask interrupt */ wx_intr_enable(wx, TXGBE_INTR_MISC(wx)); if (queues) @@ -129,17 +131,6 @@ static irqreturn_t txgbe_intr(int __always_unused irq, void *data) return IRQ_HANDLED; } -static irqreturn_t txgbe_msix_other(int __always_unused irq, void *data) -{ - struct wx *wx = data; - - /* re-enable the original interrupt state */ - if (netif_running(wx->netdev)) - txgbe_irq_enable(wx, false); - - return IRQ_HANDLED; -} - /** * txgbe_request_msix_irqs - Initialize MSI-X interrupts * @wx: board private structure @@ -171,13 +162,6 @@ static int txgbe_request_msix_irqs(struct wx *wx) } } - err = request_irq(wx->msix_entries[vector].vector, - txgbe_msix_other, 0, netdev->name, wx); - if (err) { - wx_err(wx, "request_irq for msix_other failed: %d\n", err); - goto free_queue_irqs; - } - return 0; free_queue_irqs: diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c index 23fa7311db45..8085616a9146 100644 --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c @@ -2,6 +2,9 @@ /* Copyright (c) 2015 - 2023 Beijing WangXun Technology Co., Ltd. */ #include <linux/platform_device.h> +#include <linux/gpio/consumer.h> +#include <linux/gpio/machine.h> +#include <linux/gpio/driver.h> #include <linux/gpio/property.h> #include <linux/regmap.h> #include <linux/clkdev.h> @@ -10,6 +13,7 @@ #include <linux/pci.h> #include "../libwx/wx_type.h" +#include "../libwx/wx_hw.h" #include "txgbe_type.h" #include "txgbe_phy.h" @@ -74,6 +78,224 @@ static int txgbe_swnodes_register(struct txgbe *txgbe) return software_node_register_node_group(nodes->group); } +static int txgbe_gpio_get(struct gpio_chip *chip, unsigned int offset) +{ + struct wx *wx = gpiochip_get_data(chip); + struct txgbe *txgbe = wx->priv; + int val; + + val = rd32m(wx, WX_GPIO_EXT, BIT(offset)); + + txgbe->gpio_orig &= ~BIT(offset); + txgbe->gpio_orig |= val; + + return !!(val & BIT(offset)); +} + +static int txgbe_gpio_get_direction(struct gpio_chip *chip, unsigned int offset) +{ + struct wx *wx = gpiochip_get_data(chip); + u32 val; + + val = rd32(wx, WX_GPIO_DDR); + if (BIT(offset) & val) + return GPIO_LINE_DIRECTION_OUT; + + return GPIO_LINE_DIRECTION_IN; +} + +static int txgbe_gpio_direction_in(struct gpio_chip *chip, unsigned int offset) +{ + struct wx *wx = gpiochip_get_data(chip); + + wr32m(wx, WX_GPIO_DDR, BIT(offset), 0); + + return 0; +} + +static int txgbe_gpio_direction_out(struct gpio_chip *chip, unsigned int offset, + int val) +{ + struct wx *wx = gpiochip_get_data(chip); + u32 mask; + + mask = BIT(offset) | BIT(offset - 1); + if (val) + wr32m(wx, WX_GPIO_DR, mask, mask); + else + wr32m(wx, WX_GPIO_DR, mask, 0); + + wr32m(wx, WX_GPIO_DDR, BIT(offset), BIT(offset)); + + return 0; +} + +static void txgbe_gpio_irq_ack(struct irq_data *d) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + irq_hw_number_t hwirq = irqd_to_hwirq(d); + struct wx *wx = gpiochip_get_data(gc); + + wr32(wx, WX_GPIO_EOI, BIT(hwirq)); +} + +static void txgbe_gpio_irq_mask(struct irq_data *d) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + irq_hw_number_t hwirq = irqd_to_hwirq(d); + struct wx *wx = gpiochip_get_data(gc); + + gpiochip_disable_irq(gc, hwirq); + + wr32m(wx, WX_GPIO_INTMASK, BIT(hwirq), BIT(hwirq)); +} + +static void txgbe_gpio_irq_unmask(struct irq_data *d) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + irq_hw_number_t hwirq = irqd_to_hwirq(d); + struct wx *wx = gpiochip_get_data(gc); + + gpiochip_enable_irq(gc, hwirq); + + wr32m(wx, WX_GPIO_INTMASK, BIT(hwirq), 0); +} + +static int txgbe_gpio_set_type(struct irq_data *d, unsigned int type) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + irq_hw_number_t hwirq = irqd_to_hwirq(d); + struct wx *wx = gpiochip_get_data(gc); + u32 level, polarity; + + level = rd32(wx, WX_GPIO_INTTYPE_LEVEL); + polarity = rd32(wx, WX_GPIO_POLARITY); + + switch (type) { + case IRQ_TYPE_EDGE_BOTH: + level |= BIT(hwirq); + break; + case IRQ_TYPE_EDGE_RISING: + level |= BIT(hwirq); + polarity |= BIT(hwirq); + break; + case IRQ_TYPE_EDGE_FALLING: + level |= BIT(hwirq); + polarity &= ~BIT(hwirq); + break; + case IRQ_TYPE_LEVEL_HIGH: + level &= ~BIT(hwirq); + polarity |= BIT(hwirq); + break; + case IRQ_TYPE_LEVEL_LOW: + level &= ~BIT(hwirq); + polarity &= ~BIT(hwirq); + break; + } + + if (type & IRQ_TYPE_LEVEL_MASK) + irq_set_handler_locked(d, handle_level_irq); + else if (type & IRQ_TYPE_EDGE_BOTH) + irq_set_handler_locked(d, handle_edge_irq); + + wr32m(wx, WX_GPIO_INTEN, BIT(hwirq), BIT(hwirq)); + wr32(wx, WX_GPIO_INTTYPE_LEVEL, level); + if (type != IRQ_TYPE_EDGE_BOTH) + wr32(wx, WX_GPIO_POLARITY, polarity); + + return 0; +} + +static const struct irq_chip txgbe_gpio_irq_chip = { + .name = "txgbe_gpio_irq", + .irq_ack = txgbe_gpio_irq_ack, + .irq_mask = txgbe_gpio_irq_mask, + .irq_unmask = txgbe_gpio_irq_unmask, + .irq_set_type = txgbe_gpio_set_type, + .flags = IRQCHIP_IMMUTABLE, + GPIOCHIP_IRQ_RESOURCE_HELPERS, +}; + +static void txgbe_irq_handler(struct irq_desc *desc) +{ + struct irq_chip *chip = irq_desc_get_chip(desc); + struct wx *wx = irq_desc_get_handler_data(desc); + struct txgbe *txgbe = wx->priv; + irq_hw_number_t hwirq; + unsigned long gpioirq; + struct gpio_chip *gc; + u32 gpio; + + chained_irq_enter(chip, desc); + + gpioirq = rd32(wx, WX_GPIO_INTSTATUS); + + /* workaround for hysteretic gpio interrupts */ + gpio = rd32(wx, WX_GPIO_EXT); + if (!gpioirq) + gpioirq = txgbe->gpio_orig ^ gpio; + + gc = txgbe->gpio; + for_each_set_bit(hwirq, &gpioirq, gc->ngpio) + generic_handle_domain_irq(gc->irq.domain, hwirq); + + chained_irq_exit(chip, desc); + + /* unmask interrupt */ + if (netif_running(wx->netdev)) + wx_intr_enable(wx, TXGBE_INTR_MISC(wx)); +} + +static int txgbe_gpio_init(struct txgbe *txgbe) +{ + struct gpio_irq_chip *girq; + struct wx *wx = txgbe->wx; + struct pci_dev *pdev; + struct gpio_chip *gc; + int ret; + + pdev = wx->pdev; + txgbe->gpio_orig = 0; + + gc = devm_kzalloc(&pdev->dev, sizeof(*gc), GFP_KERNEL); + if (!gc) + return -ENOMEM; + + gc->label = devm_kasprintf(&pdev->dev, GFP_KERNEL, "txgbe_gpio-%x", + (pdev->bus->number << 8) | pdev->devfn); + gc->base = -1; + gc->ngpio = 6; + gc->owner = THIS_MODULE; + gc->parent = &pdev->dev; + gc->fwnode = software_node_fwnode(txgbe->nodes.group[SWNODE_GPIO]); + gc->get = txgbe_gpio_get; + gc->get_direction = txgbe_gpio_get_direction; + gc->direction_input = txgbe_gpio_direction_in; + gc->direction_output = txgbe_gpio_direction_out; + gc->can_sleep = false; + + girq = &gc->irq; + gpio_irq_chip_set_chip(girq, &txgbe_gpio_irq_chip); + girq->parent_handler = txgbe_irq_handler; + girq->parent_handler_data = wx; + girq->num_parents = 1; + girq->parents = devm_kcalloc(&pdev->dev, 1, sizeof(*girq->parents), + GFP_KERNEL); + if (!girq->parents) + return -ENOMEM; + girq->parents[0] = wx->msix_entries[wx->num_q_vectors].vector; + girq->default_type = IRQ_TYPE_NONE; + girq->handler = handle_bad_irq; + + ret = devm_gpiochip_add_data(&pdev->dev, gc, wx); + if (ret) + return ret; + + txgbe->gpio = gc; + + return 0; +} + static int txgbe_clock_register(struct txgbe *txgbe) { struct pci_dev *pdev = txgbe->wx->pdev; @@ -188,6 +410,12 @@ int txgbe_init_phy(struct txgbe *txgbe) return ret; } + ret = txgbe_gpio_init(txgbe); + if (ret) { + wx_err(txgbe->wx, "failed to init gpio\n"); + goto err_unregister_swnode; + } + ret = txgbe_clock_register(txgbe); if (ret) { wx_err(txgbe->wx, "failed to register clock: %d\n", ret); diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h b/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h index fc91e0fc37a6..796f33fe3016 100644 --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h @@ -55,6 +55,31 @@ #define TXGBE_TS_CTL 0x10300 #define TXGBE_TS_CTL_EVAL_MD BIT(31) +/* GPIO register bit */ +#define TXGBE_GPIOBIT_0 BIT(0) /* I:tx fault */ +#define TXGBE_GPIOBIT_1 BIT(1) /* O:tx disabled */ +#define TXGBE_GPIOBIT_2 BIT(2) /* I:sfp module absent */ +#define TXGBE_GPIOBIT_3 BIT(3) /* I:rx signal lost */ +#define TXGBE_GPIOBIT_4 BIT(4) /* O:rate select, 1G(0) 10G(1) */ +#define TXGBE_GPIOBIT_5 BIT(5) /* O:rate select, 1G(0) 10G(1) */ + +/* Extended Interrupt Enable Set */ +#define TXGBE_PX_MISC_ETH_LKDN BIT(8) +#define TXGBE_PX_MISC_DEV_RST BIT(10) +#define TXGBE_PX_MISC_ETH_EVENT BIT(17) +#define TXGBE_PX_MISC_ETH_LK BIT(18) +#define TXGBE_PX_MISC_ETH_AN BIT(19) +#define TXGBE_PX_MISC_INT_ERR BIT(20) +#define TXGBE_PX_MISC_GPIO BIT(26) +#define TXGBE_PX_MISC_IEN_MASK ( \ + TXGBE_PX_MISC_ETH_LKDN | \ + TXGBE_PX_MISC_DEV_RST | \ + TXGBE_PX_MISC_ETH_EVENT | \ + TXGBE_PX_MISC_ETH_LK | \ + TXGBE_PX_MISC_ETH_AN | \ + TXGBE_PX_MISC_INT_ERR | \ + TXGBE_PX_MISC_GPIO) + /* I2C registers */ #define TXGBE_I2C_BASE 0x14900 @@ -153,6 +178,8 @@ struct txgbe { struct platform_device *i2c_dev; struct clk_lookup *clock; struct clk *clk; + struct gpio_chip *gpio; + u32 gpio_orig; }; #endif /* _TXGBE_TYPE_H_ */
Register GPIO chip and handle GPIO IRQ for SFP socket. Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com> --- drivers/net/ethernet/wangxun/Kconfig | 2 + drivers/net/ethernet/wangxun/libwx/wx_lib.c | 3 +- drivers/net/ethernet/wangxun/libwx/wx_type.h | 2 + .../net/ethernet/wangxun/txgbe/txgbe_main.c | 20 +- .../net/ethernet/wangxun/txgbe/txgbe_phy.c | 228 ++++++++++++++++++ .../net/ethernet/wangxun/txgbe/txgbe_type.h | 27 +++ 6 files changed, 263 insertions(+), 19 deletions(-)