| Message ID | 20220416110507.642398-3-pgwipeout@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enable rk356x PCIe controller | expand |
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.
On Sat, Apr 16, 2022 at 8:54 AM Marc Zyngier <maz@kernel.org> wrote: > > 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. Apologies, I'll be sure to be at least one cup of coffee in before doing early morning code. > > 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. There are 8 bits in total, the top four are for the TX interrupts, for which EP mode is not yet supported by the driver. I can constrain this further and let it be expanded when that support is added, if that works for you? > > > + 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. Okay, that makes sense. I'm hitting the entire block when it should be the individual IRQ. I also notice some drivers protect this with a spinlock while others do not, how should this be handled? > > M. > -- > Jazz is not dead. It just smells funny... Thanks Again!
On Sat, 16 Apr 2022 14:24:26 +0100, Peter Geis <pgwipeout@gmail.com> wrote: > > On Sat, Apr 16, 2022 at 8:54 AM Marc Zyngier <maz@kernel.org> wrote: > > > > 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. > > Apologies, I'll be sure to be at least one cup of coffee in before > doing early morning code. Even with a steady intake of coffee, there is a pretty clear policy around the frequency of patch submission, see [1]. [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst#n337 There is no hard enforcement of this process, but that should give you an idea of how to deal with it. In any case, 7 series in less than a week is a clear sign that this series should be *ignored*, as the author is likely to post yet another one in the next few hours. > > > > > 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. > > There are 8 bits in total, the top four are for the TX interrupts, for > which EP mode is not yet supported by the driver. So why aren't they excluded from the set of bits that you look at? > I can constrain this further and let it be expanded when that support > is added, if that works for you? Well, you can't have INTx interrupts in EP mode (that's a TLP going out of the device, and not something that is signalled *to* the CPU). So the two should be mutually exclusive. > > > > > > + 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. > > Okay, that makes sense. I'm hitting the entire block when it should be > the individual IRQ. > I also notice some drivers protect this with a spinlock while others > do not, how should this be handled? It obviously depends on how the HW. works. If this is a shared register using a RMW sequence, then you need some form of mutual exclusion in order to preserve the atomicity of the update. If the HW supports updating the masks using a set of hot bits (with separate clear/set registers), than there is no need for locking. In your case PCIE_CLIENT_INTR_MASK_LEGACY seems to support this odd "write-enable" feature which can probably be used to implement a lockless access, something like: void mask(struct irq_data *d) { u32 val = BIT(d->hwirq + 16) | BIT(d->hwirq); writel_relaxed(val, ...); } void mask(struct irq_data *d) { u32 val = BIT(d->hwirq + 16); writel_relaxed(val, ...); } Another thing is that it is completely unclear to me what initialises these interrupts the first place (INTR_MASK_LEGACY, INTR_EN_LEGACY). Are you relying on the firmware to do that for you? Thanks, M.
On Sun, Apr 17, 2022 at 5:53 AM Marc Zyngier <maz@kernel.org> wrote: > > On Sat, 16 Apr 2022 14:24:26 +0100, > Peter Geis <pgwipeout@gmail.com> wrote: > > > > On Sat, Apr 16, 2022 at 8:54 AM Marc Zyngier <maz@kernel.org> wrote: > > > > > > 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. > > > > Apologies, I'll be sure to be at least one cup of coffee in before > > doing early morning code. > > Even with a steady intake of coffee, there is a pretty clear policy > around the frequency of patch submission, see [1]. > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst#n337 > > There is no hard enforcement of this process, but that should give you > an idea of how to deal with it. In any case, 7 series in less than a > week is a clear sign that this series should be *ignored*, as the > author is likely to post yet another one in the next few hours. Understood. > > > > > > > > > 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. > > > > There are 8 bits in total, the top four are for the TX interrupts, for > > which EP mode is not yet supported by the driver. > > So why aren't they excluded from the set of bits that you look at? > > > I can constrain this further and let it be expanded when that support > > is added, if that works for you? > > Well, you can't have INTx interrupts in EP mode (that's a TLP going > out of the device, and not something that is signalled *to* the > CPU). So the two should be mutually exclusive. Thank you for the explanation, I haven't messed about with EP mode, so my experience is solely with RC mode. > > > > > > > > > > + 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. > > > > Okay, that makes sense. I'm hitting the entire block when it should be > > the individual IRQ. > > I also notice some drivers protect this with a spinlock while others > > do not, how should this be handled? > > It obviously depends on how the HW. works. If this is a shared > register using a RMW sequence, then you need some form of mutual > exclusion in order to preserve the atomicity of the update. > > If the HW supports updating the masks using a set of hot bits (with > separate clear/set registers), than there is no need for locking. In > your case PCIE_CLIENT_INTR_MASK_LEGACY seems to support this odd > "write-enable" feature which can probably be used to implement a > lockless access, something like: > > void mask(struct irq_data *d) > { > u32 val = BIT(d->hwirq + 16) | BIT(d->hwirq); This is what HIWORD_UPDATE_BIT does, it's rather common in Rockchip code. I believe I can safely drop the spinlock when enabling/disabling individual interrupts. > writel_relaxed(val, ...); > } > > void mask(struct irq_data *d) > { > u32 val = BIT(d->hwirq + 16); > writel_relaxed(val, ...); > } > > Another thing is that it is completely unclear to me what initialises > these interrupts the first place (INTR_MASK_LEGACY, INTR_EN_LEGACY). > Are you relying on the firmware to do that for you? There is no dedicated mask or enable/disable for the legacy interrupt line (unless it's undocumented). It appears to be enabled via an "or" function with the emulated interrupts. As far as I can tell this is common for dw-pcie, looking at the other drivers. > > Thanks, > > M. > > -- > Without deviation from the norm, progress is not possible. Thank you for your insight! Peter
On Mon, 18 Apr 2022 12:37:00 +0100, Peter Geis <pgwipeout@gmail.com> wrote: > > On Sun, Apr 17, 2022 at 5:53 AM Marc Zyngier <maz@kernel.org> wrote: > > > > On Sat, 16 Apr 2022 14:24:26 +0100, > > Peter Geis <pgwipeout@gmail.com> wrote: > > > > > > Okay, that makes sense. I'm hitting the entire block when it should be > > > the individual IRQ. > > > I also notice some drivers protect this with a spinlock while others > > > do not, how should this be handled? > > > > It obviously depends on how the HW. works. If this is a shared > > register using a RMW sequence, then you need some form of mutual > > exclusion in order to preserve the atomicity of the update. > > > > If the HW supports updating the masks using a set of hot bits (with > > separate clear/set registers), than there is no need for locking. In > > your case PCIE_CLIENT_INTR_MASK_LEGACY seems to support this odd > > "write-enable" feature which can probably be used to implement a > > lockless access, something like: > > > > void mask(struct irq_data *d) > > { > > u32 val = BIT(d->hwirq + 16) | BIT(d->hwirq); > > This is what HIWORD_UPDATE_BIT does, it's rather common in Rockchip code. > I believe I can safely drop the spinlock when enabling/disabling > individual interrupts. Yes. > > > writel_relaxed(val, ...); > > } > > > > void mask(struct irq_data *d) > > { > > u32 val = BIT(d->hwirq + 16); > > writel_relaxed(val, ...); > > } > > > > Another thing is that it is completely unclear to me what initialises > > these interrupts the first place (INTR_MASK_LEGACY, INTR_EN_LEGACY). > > Are you relying on the firmware to do that for you? > > There is no dedicated mask or enable/disable for the legacy interrupt > line (unless it's undocumented). I'm talking about the INTR_MASK_LEGACY and INTR_EN_LEGACY registers, which control the INTx (although the latter seems to default to some reserved values). I don't see where you initialise them to a state where they are enabled and masked, which should be the initial state once this driver has probed. The output interrupt itself is obviously controlled by the GIC driver. > It appears to be enabled via an "or" function with the emulated interrupts. > As far as I can tell this is common for dw-pcie, looking at the other drivers. I think we're talking past each other. I'm solely concerned with the initialisation of the input control registers, for which I see no code in this patch. M.
On Mon, Apr 18, 2022 at 8:34 AM Marc Zyngier <maz@kernel.org> wrote: > > On Mon, 18 Apr 2022 12:37:00 +0100, > Peter Geis <pgwipeout@gmail.com> wrote: > > > > On Sun, Apr 17, 2022 at 5:53 AM Marc Zyngier <maz@kernel.org> wrote: > > > > > > On Sat, 16 Apr 2022 14:24:26 +0100, > > > Peter Geis <pgwipeout@gmail.com> wrote: > > > > > > > > Okay, that makes sense. I'm hitting the entire block when it should be > > > > the individual IRQ. > > > > I also notice some drivers protect this with a spinlock while others > > > > do not, how should this be handled? > > > > > > It obviously depends on how the HW. works. If this is a shared > > > register using a RMW sequence, then you need some form of mutual > > > exclusion in order to preserve the atomicity of the update. > > > > > > If the HW supports updating the masks using a set of hot bits (with > > > separate clear/set registers), than there is no need for locking. In > > > your case PCIE_CLIENT_INTR_MASK_LEGACY seems to support this odd > > > "write-enable" feature which can probably be used to implement a > > > lockless access, something like: > > > > > > void mask(struct irq_data *d) > > > { > > > u32 val = BIT(d->hwirq + 16) | BIT(d->hwirq); > > > > This is what HIWORD_UPDATE_BIT does, it's rather common in Rockchip code. > > I believe I can safely drop the spinlock when enabling/disabling > > individual interrupts. > > Yes. > > > > > > writel_relaxed(val, ...); > > > } > > > > > > void mask(struct irq_data *d) > > > { > > > u32 val = BIT(d->hwirq + 16); > > > writel_relaxed(val, ...); > > > } > > > > > > Another thing is that it is completely unclear to me what initialises > > > these interrupts the first place (INTR_MASK_LEGACY, INTR_EN_LEGACY). > > > Are you relying on the firmware to do that for you? > > > > There is no dedicated mask or enable/disable for the legacy interrupt > > line (unless it's undocumented). > > I'm talking about the INTR_MASK_LEGACY and INTR_EN_LEGACY registers, > which control the INTx (although the latter seems to default to some > reserved values). I don't see where you initialise them to a state > where they are enabled and masked, which should be the initial state > once this driver has probed. The output interrupt itself is obviously > controlled by the GIC driver. PCIE_CLIENT_INTR_MASK_LEGACY is the register I use here to mask/unmask the interrupts. It defaults to all masked on reset. The current rk3568 trm v1.1 does not reference an INTR_EN_LEGACY register. > > > It appears to be enabled via an "or" function with the emulated interrupts. > > As far as I can tell this is common for dw-pcie, looking at the other drivers. > > I think we're talking past each other. I'm solely concerned with the > initialisation of the input control registers, for which I see no code > in this patch. Downstream points to the mask/unmask functions for the enable/disable functions, which would be superfluous here as mainline defaults to that anyways if they are null. I've double checked and downstream only uses the mask register, enable and routing config appears to be left as is from reset. I'm rather concerned about the lack of any obvious way to control routing, nor an ack mechanism for the irq. I see other implementations reference the core registers or vendor defined registers for these functions. Unfortunately the rk3568 trm does not include the core register definitions, and the designware documentation appears to be behind a paywall/nda. I suspect most of the confusion here boils down to a lack of documentation, but it's entirely possible I am simply not understanding the question. I'm already aware of other functions that I need documentation for that is currently unavailable. > > M. > > -- > Without deviation from the norm, progress is not possible. Thank you for your time, Peter
On Mon, 18 Apr 2022 16:13:39 +0100, Peter Geis <pgwipeout@gmail.com> wrote: > > On Mon, Apr 18, 2022 at 8:34 AM Marc Zyngier <maz@kernel.org> wrote: > > > > On Mon, 18 Apr 2022 12:37:00 +0100, > > Peter Geis <pgwipeout@gmail.com> wrote: > > > > > > On Sun, Apr 17, 2022 at 5:53 AM Marc Zyngier <maz@kernel.org> wrote: > > > > > > > > On Sat, 16 Apr 2022 14:24:26 +0100, > > > > Peter Geis <pgwipeout@gmail.com> wrote: > > > > > > > > > > Okay, that makes sense. I'm hitting the entire block when it should be > > > > > the individual IRQ. > > > > > I also notice some drivers protect this with a spinlock while others > > > > > do not, how should this be handled? > > > > > > > > It obviously depends on how the HW. works. If this is a shared > > > > register using a RMW sequence, then you need some form of mutual > > > > exclusion in order to preserve the atomicity of the update. > > > > > > > > If the HW supports updating the masks using a set of hot bits (with > > > > separate clear/set registers), than there is no need for locking. In > > > > your case PCIE_CLIENT_INTR_MASK_LEGACY seems to support this odd > > > > "write-enable" feature which can probably be used to implement a > > > > lockless access, something like: > > > > > > > > void mask(struct irq_data *d) > > > > { > > > > u32 val = BIT(d->hwirq + 16) | BIT(d->hwirq); > > > > > > This is what HIWORD_UPDATE_BIT does, it's rather common in Rockchip code. > > > I believe I can safely drop the spinlock when enabling/disabling > > > individual interrupts. > > > > Yes. > > > > > > > > > writel_relaxed(val, ...); > > > > } > > > > > > > > void mask(struct irq_data *d) > > > > { > > > > u32 val = BIT(d->hwirq + 16); > > > > writel_relaxed(val, ...); > > > > } > > > > > > > > Another thing is that it is completely unclear to me what initialises > > > > these interrupts the first place (INTR_MASK_LEGACY, INTR_EN_LEGACY). > > > > Are you relying on the firmware to do that for you? > > > > > > There is no dedicated mask or enable/disable for the legacy interrupt > > > line (unless it's undocumented). > > > > I'm talking about the INTR_MASK_LEGACY and INTR_EN_LEGACY registers, > > which control the INTx (although the latter seems to default to some > > reserved values). I don't see where you initialise them to a state > > where they are enabled and masked, which should be the initial state > > once this driver has probed. The output interrupt itself is obviously > > controlled by the GIC driver. > > PCIE_CLIENT_INTR_MASK_LEGACY is the register I use here to mask/unmask > the interrupts. > It defaults to all masked on reset. And? Are your really expecting that the firmware that runs before the kernel will preserve this register and not write anything silly to it because, oh wait, it wants to use interrupts? Or that nobody will kexec a secondary kernel from the first one after having used these interrupts? Rule #1: Initialise the HW to sensible values Rule #2: See Rule #1 > The current rk3568 trm v1.1 does not reference an INTR_EN_LEGACY register. The TRM for RK3588 mentions it, and is the same IP. > > > > > It appears to be enabled via an "or" function with the emulated interrupts. > > > As far as I can tell this is common for dw-pcie, looking at the other drivers. > > > > I think we're talking past each other. I'm solely concerned with the > > initialisation of the input control registers, for which I see no code > > in this patch. > > Downstream points to the mask/unmask functions for the enable/disable > functions, which would be superfluous here as mainline defaults to > that anyways if they are null. Yeah, that's completely dumb. But there is no shortage of dumb stuff in the RK downstream code... > > I've double checked and downstream only uses the mask register, enable > and routing config appears to be left as is from reset. And that's a bug. > I'm rather concerned about the lack of any obvious way to control > routing, nor an ack mechanism for the irq. Which routing? Do you mean the affinity? You can't change it, as this would change the affinity of all interrupts at once. > I see other implementations reference the core registers or vendor > defined registers for these functions. > Unfortunately the rk3568 trm does not include the core register > definitions, and the designware documentation appears to be behind a > paywall/nda. If you use a search engine, you'll find *CONFIDENTIAL* copies of the DW stuff. The whole thing is a laugh anyway. > > I suspect most of the confusion here boils down to a lack of > documentation, but it's entirely possible I am simply not > understanding the question. My only ask is that you properly initialise the HW. This will save countless amount of head-scratching once you have a decent firmware or kexec. M.
On Mon, Apr 18, 2022 at 6:53 PM Marc Zyngier <maz@kernel.org> wrote: > > On Mon, 18 Apr 2022 16:13:39 +0100, > Peter Geis <pgwipeout@gmail.com> wrote: > > > > On Mon, Apr 18, 2022 at 8:34 AM Marc Zyngier <maz@kernel.org> wrote: > > > > > > On Mon, 18 Apr 2022 12:37:00 +0100, > > > Peter Geis <pgwipeout@gmail.com> wrote: > > > > > > > > On Sun, Apr 17, 2022 at 5:53 AM Marc Zyngier <maz@kernel.org> wrote: > > > > > > > > > > On Sat, 16 Apr 2022 14:24:26 +0100, > > > > > Peter Geis <pgwipeout@gmail.com> wrote: > > > > > > > > > > > > Okay, that makes sense. I'm hitting the entire block when it should be > > > > > > the individual IRQ. > > > > > > I also notice some drivers protect this with a spinlock while others > > > > > > do not, how should this be handled? > > > > > > > > > > It obviously depends on how the HW. works. If this is a shared > > > > > register using a RMW sequence, then you need some form of mutual > > > > > exclusion in order to preserve the atomicity of the update. > > > > > > > > > > If the HW supports updating the masks using a set of hot bits (with > > > > > separate clear/set registers), than there is no need for locking. In > > > > > your case PCIE_CLIENT_INTR_MASK_LEGACY seems to support this odd > > > > > "write-enable" feature which can probably be used to implement a > > > > > lockless access, something like: > > > > > > > > > > void mask(struct irq_data *d) > > > > > { > > > > > u32 val = BIT(d->hwirq + 16) | BIT(d->hwirq); > > > > > > > > This is what HIWORD_UPDATE_BIT does, it's rather common in Rockchip code. > > > > I believe I can safely drop the spinlock when enabling/disabling > > > > individual interrupts. > > > > > > Yes. > > > > > > > > > > > > writel_relaxed(val, ...); > > > > > } > > > > > > > > > > void mask(struct irq_data *d) > > > > > { > > > > > u32 val = BIT(d->hwirq + 16); > > > > > writel_relaxed(val, ...); > > > > > } > > > > > > > > > > Another thing is that it is completely unclear to me what initialises > > > > > these interrupts the first place (INTR_MASK_LEGACY, INTR_EN_LEGACY). > > > > > Are you relying on the firmware to do that for you? > > > > > > > > There is no dedicated mask or enable/disable for the legacy interrupt > > > > line (unless it's undocumented). > > > > > > I'm talking about the INTR_MASK_LEGACY and INTR_EN_LEGACY registers, > > > which control the INTx (although the latter seems to default to some > > > reserved values). I don't see where you initialise them to a state > > > where they are enabled and masked, which should be the initial state > > > once this driver has probed. The output interrupt itself is obviously > > > controlled by the GIC driver. > > > > PCIE_CLIENT_INTR_MASK_LEGACY is the register I use here to mask/unmask > > the interrupts. > > It defaults to all masked on reset. > > And? Are your really expecting that the firmware that runs before the > kernel will preserve this register and not write anything silly to it > because, oh wait, it wants to use interrupts? Or that nobody will > kexec a secondary kernel from the first one after having used these > interrupts? > > Rule #1: Initialise the HW to sensible values > Rule #2: See Rule #1 I don't disagree here, there are plenty of examples of bugs that stem from this in Rockchip's code. Working on this series has given me ideas for improvements to the rk3399 controller as well. > > > The current rk3568 trm v1.1 does not reference an INTR_EN_LEGACY register. > > The TRM for RK3588 mentions it, and is the same IP. Unfortunately this assumption doesn't hold up to testing. On rk356x this entire register block is 0x0, which if it was implemented means legacy interrupts would not work, among other issues. Even in the rk3588 it's marked as "reserved" which means there's a good possibility it isn't fully implemented there either. A number of other blocks in the rk3588 trm are labeled as being available only after a specific hardware revision. We are seeing other bugs in the hardware implementation Rockchip has here, so making assumptions based on other implementations of the DW IP is unsafe. > > > > > > > > It appears to be enabled via an "or" function with the emulated interrupts. > > > > As far as I can tell this is common for dw-pcie, looking at the other drivers. > > > > > > I think we're talking past each other. I'm solely concerned with the > > > initialisation of the input control registers, for which I see no code > > > in this patch. > > > > Downstream points to the mask/unmask functions for the enable/disable > > functions, which would be superfluous here as mainline defaults to > > that anyways if they are null. > > Yeah, that's completely dumb. But there is no shortage of dumb stuff > in the RK downstream code... You'll find no argument from me here, I'm merely using it as an example of the vendor's implementation. The only resources I have available are the publically released documentation and the publically released downstream code. > > > > > I've double checked and downstream only uses the mask register, enable > > and routing config appears to be left as is from reset. > > And that's a bug. > > > I'm rather concerned about the lack of any obvious way to control > > routing, nor an ack mechanism for the irq. > > Which routing? Do you mean the affinity? You can't change it, as this > would change the affinity of all interrupts at once. > > > I see other implementations reference the core registers or vendor > > defined registers for these functions. > > Unfortunately the rk3568 trm does not include the core register > > definitions, and the designware documentation appears to be behind a > > paywall/nda. > > If you use a search engine, you'll find *CONFIDENTIAL* copies of the > DW stuff. The whole thing is a laugh anyway. > > > > > I suspect most of the confusion here boils down to a lack of > > documentation, but it's entirely possible I am simply not > > understanding the question. > > My only ask is that you properly initialise the HW. This will save > countless amount of head-scratching once you have a decent firmware or > kexec. The only way to ensure that in a sane way is to trigger the resets at driver probe. Can that be safely done without causing other issues with an already configured card or should I power cycle it as well? This is starting to feature creep from the original intention of this series, since a pre-configured controller would affect more than just interrupts. If you wish, as a compromise I can ensure all INTx interrupts are masked at probe (which would hilariously be the opposite of downstream). > > M. > > -- > Without deviation from the norm, progress is not possible.
On Tue, 19 Apr 2022 01:23:23 +0100, Peter Geis <pgwipeout@gmail.com> wrote: > > > My only ask is that you properly initialise the HW. This will save > > countless amount of head-scratching once you have a decent firmware or > > kexec. > > The only way to ensure that in a sane way is to trigger the resets at > driver probe. If that can be done, that'd be great. > Can that be safely done without causing other issues with an already > configured card or should I power cycle it as well? Well, you are already renegotiating the link anyway, so that's a very moot point. > This is starting to feature creep from the original intention of this > series, since a pre-configured controller would affect more than just > interrupts. Configuring the HW is not exactly a feature creep. If your intention is to keep everything as it was left, then you don't have much of a driver, but instead a time bomb. And we can do without another one in the tree. > If you wish, as a compromise I can ensure all INTx interrupts are > masked at probe (which would hilariously be the opposite of > downstream). As far as I'm concerned, downstream doesn't exist. If someone wants the downstream code, they can use it directly and we don't need to merge this code. If, on the other hand, you want this driver to be useful and to be maintained upstream, initialising the interrupt mask is the absolute bare minimum. M.
On Tue, Apr 19, 2022 at 4:05 AM Marc Zyngier <maz@kernel.org> wrote: > > On Tue, 19 Apr 2022 01:23:23 +0100, > Peter Geis <pgwipeout@gmail.com> wrote: > > > > > My only ask is that you properly initialise the HW. This will save > > > countless amount of head-scratching once you have a decent firmware or > > > kexec. > > > > The only way to ensure that in a sane way is to trigger the resets at > > driver probe. > > If that can be done, that'd be great. Okay, I'll work on implementing this then. > > > Can that be safely done without causing other issues with an already > > configured card or should I power cycle it as well? > > Well, you are already renegotiating the link anyway, so that's a very > moot point. Understood, thank you. > > > This is starting to feature creep from the original intention of this > > series, since a pre-configured controller would affect more than just > > interrupts. > > Configuring the HW is not exactly a feature creep. If your intention > is to keep everything as it was left, then you don't have much of a > driver, but instead a time bomb. And we can do without another one in > the tree. Understood, I apologize if I'm being difficult here, I just want to make sure I completely understand the assignment. Your comment about kexec made it clear for me, thank you. > > > If you wish, as a compromise I can ensure all INTx interrupts are > > masked at probe (which would hilariously be the opposite of > > downstream). > > As far as I'm concerned, downstream doesn't exist. If someone wants > the downstream code, they can use it directly and we don't need to > merge this code. Once again, you'll have no argument from me in this regard. I've had several blocks of hardware enablement sitting out of tree waiting for the phy code to land. As much testing as my branch has seen, it's still only a drop in the bucket compared to finally being mainlined. I appreciate all of your effort and review here and I absolutely want this done correctly. > > If, on the other hand, you want this driver to be useful and to be > maintained upstream, initialising the interrupt mask is the absolute > bare minimum. I think resetting the whole core is the best move, since it's the only way we can guarantee a sane configuration with the documentation we have. > > M. > > -- > Without deviation from the norm, progress is not possible.
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) + 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); +}; + +static void rockchip_intx_unmask(struct irq_data *data) +{ + struct rockchip_pcie *rockchip = irq_data_get_irq_chip_data(data); + unsigned long flags; + u32 val; + + /* enable 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); +}; + +static struct irq_chip rockchip_intx_irq_chip = { + .name = "INTx", + .irq_mask = rockchip_intx_mask, + .irq_unmask = rockchip_intx_unmask, + .flags = IRQCHIP_SKIP_SET_WAKE | IRQCHIP_MASK_ON_SUSPEND, +}; + +static int rockchip_pcie_intx_map(struct irq_domain *domain, unsigned int irq, + irq_hw_number_t hwirq) +{ + irq_set_chip_and_handler(irq, &rockchip_intx_irq_chip, handle_level_irq); + irq_set_chip_data(irq, domain->host_data); + + return 0; +} + +static const struct irq_domain_ops intx_domain_ops = { + .map = rockchip_pcie_intx_map, +}; + +static int rockchip_pcie_init_irq_domain(struct rockchip_pcie *rockchip) +{ + struct device *dev = rockchip->pci.dev; + struct device_node *intc; + + raw_spin_lock_init(&rockchip->irq_lock); + + intc = of_get_child_by_name(dev->of_node, "legacy-interrupt-controller"); + if (!intc) { + dev_err(dev, "missing child interrupt-controller node\n"); + return -EINVAL; + } + + rockchip->irq_domain = irq_domain_add_linear(intc, PCI_NUM_INTX, + &intx_domain_ops, rockchip); + of_node_put(intc); + if (!rockchip->irq_domain) { + dev_err(dev, "failed to get a INTx IRQ domain\n"); + return -EINVAL; + } + + return 0; +} + static void rockchip_pcie_enable_ltssm(struct rockchip_pcie *rockchip) { rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_ENABLE_LTSSM, @@ -111,7 +207,19 @@ static int rockchip_pcie_host_init(struct pcie_port *pp) { struct dw_pcie *pci = to_dw_pcie_from_pp(pp); struct rockchip_pcie *rockchip = to_rockchip_pcie(pci); + struct device *dev = rockchip->pci.dev; u32 val = HIWORD_UPDATE_BIT(PCIE_LTSSM_ENABLE_ENHANCE); + int irq, ret; + + irq = of_irq_get_byname(dev->of_node, "legacy"); + if (irq < 0) + return irq; + + ret = rockchip_pcie_init_irq_domain(rockchip); + if (ret < 0) + dev_err(dev, "failed to init irq domain\n"); + + irq_set_chained_handler_and_data(irq, rockchip_pcie_legacy_int_handler, rockchip); /* LTSSM enable control mode */ rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_HOT_RESET_CTRL);
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(-)