Messages in this thread |  | | Date | Sat, 16 Apr 2022 13:54:49 +0100 | From | Marc Zyngier <> | Subject | Re: [PATCH v7 2/4] PCI: dwc: rockchip: add legacy interrupt support |
| |
Peter,
May I suggest that you slow down on the number of versions you send? This is the 7th in 5 days, the 3rd today.
At this stage, this is entirely counterproductive.
On 2022-04-16 12:05, Peter Geis wrote: > The legacy interrupts on the rk356x pcie controller are handled by a > single muxed interrupt. Add irq domain support to the pcie-dw-rockchip > driver to support the virtual domain. > > Signed-off-by: Peter Geis <pgwipeout@gmail.com> > --- > drivers/pci/controller/dwc/pcie-dw-rockchip.c | 112 +++++++++++++++++- > 1 file changed, 110 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c > b/drivers/pci/controller/dwc/pcie-dw-rockchip.c > index c9b341e55cbb..863374604fb1 100644 > --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c > +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c > @@ -10,9 +10,12 @@ > > #include <linux/clk.h> > #include <linux/gpio/consumer.h> > +#include <linux/irqchip/chained_irq.h> > +#include <linux/irqdomain.h> > #include <linux/mfd/syscon.h> > #include <linux/module.h> > #include <linux/of_device.h> > +#include <linux/of_irq.h> > #include <linux/phy/phy.h> > #include <linux/platform_device.h> > #include <linux/regmap.h> > @@ -36,10 +39,13 @@ > #define PCIE_LINKUP (PCIE_SMLH_LINKUP | PCIE_RDLH_LINKUP) > #define PCIE_L0S_ENTRY 0x11 > #define PCIE_CLIENT_GENERAL_CONTROL 0x0 > +#define PCIE_CLIENT_INTR_STATUS_LEGACY 0x8 > +#define PCIE_CLIENT_INTR_MASK_LEGACY 0x1c > #define PCIE_CLIENT_GENERAL_DEBUG 0x104 > -#define PCIE_CLIENT_HOT_RESET_CTRL 0x180 > +#define PCIE_CLIENT_HOT_RESET_CTRL 0x180 > #define PCIE_CLIENT_LTSSM_STATUS 0x300 > -#define PCIE_LTSSM_ENABLE_ENHANCE BIT(4) > +#define PCIE_LEGACY_INT_ENABLE GENMASK(3, 0) > +#define PCIE_LTSSM_ENABLE_ENHANCE BIT(4) > #define PCIE_LTSSM_STATUS_MASK GENMASK(5, 0) > > struct rockchip_pcie { > @@ -51,6 +57,8 @@ struct rockchip_pcie { > struct reset_control *rst; > struct gpio_desc *rst_gpio; > struct regulator *vpcie3v3; > + struct irq_domain *irq_domain; > + raw_spinlock_t irq_lock; > }; > > static int rockchip_pcie_readl_apb(struct rockchip_pcie *rockchip, > @@ -65,6 +73,94 @@ static void rockchip_pcie_writel_apb(struct > rockchip_pcie *rockchip, > writel_relaxed(val, rockchip->apb_base + reg); > } > > +static void rockchip_pcie_legacy_int_handler(struct irq_desc *desc) > +{ > + struct irq_chip *chip = irq_desc_get_chip(desc); > + struct rockchip_pcie *rockchip = irq_desc_get_handler_data(desc); > + unsigned long reg, hwirq; > + > + chained_irq_enter(chip, desc); > + > + reg = rockchip_pcie_readl_apb(rockchip, > PCIE_CLIENT_INTR_STATUS_LEGACY); > + > + for_each_set_bit(hwirq, ®, 8)
8? And yet:
#define PCI_NUM_INTX 4
So whatever bits are set above bit 3, you are feeding garbage to the irqdomain code.
> + generic_handle_domain_irq(rockchip->irq_domain, hwirq); > + > + chained_irq_exit(chip, desc); > +} > + > +static void rockchip_intx_mask(struct irq_data *data) > +{ > + struct rockchip_pcie *rockchip = irq_data_get_irq_chip_data(data); > + unsigned long flags; > + u32 val; > + > + /* disable legacy interrupts */ > + raw_spin_lock_irqsave(&rockchip->irq_lock, flags); > + val = HIWORD_UPDATE_BIT(PCIE_LEGACY_INT_ENABLE); > + val |= PCIE_LEGACY_INT_ENABLE; > + rockchip_pcie_writel_apb(rockchip, val, > PCIE_CLIENT_INTR_MASK_LEGACY); > + raw_spin_unlock_irqrestore(&rockchip->irq_lock, flags);
This is completely busted. INTx lines must be controlled individually. If I disable one device's INTx output, I don't want to see the interrupt firing because another one has had its own enabled.
M. -- Jazz is not dead. It just smells funny...
|  |