| Message ID | 20231214072839.2367-16-minda.chen@starfivetech.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | Refactoring Microchip PCIe driver and add StarFive PCIe | expand |
On 2023/12/14 15:28, Minda Chen wrote: > PolarFire PCIE event IRQs includes PLDA local interrupts and PolarFire > their own IRQs. PolarFire PCIe event irq_chip ops using an event_desc to > unify different IRQ register addresses. On PLDA sides, PLDA irqchip codes > only require to set PLDA local interrupt register. So the PLDA irqchip ops > codes can not be extracted from PolarFire codes. > > To support PLDA its own event IRQ process, implements PLDA irqchip ops and > add event irqchip field to struct pcie_plda_rp. > > Signed-off-by: Minda Chen <minda.chen@starfivetech.com> > --- > .../pci/controller/plda/pcie-microchip-host.c | 65 ++++++++++++++++++- > drivers/pci/controller/plda/pcie-plda.h | 3 + > 2 files changed, 67 insertions(+), 1 deletion(-) > Hi Conor Could you take time to review this patch? For I using event irq chip instead of event ops and the whole patch have been changed. I think it's better And I added the implementation of PLDA event irqchip and make it easier to claim the necessity of the modification. If you approve this, I will add back the review tag. Thanks Hi Lorenzo Have you reviewed this patch？ Does the commit message and the codes are can be approved ？Thanks > diff --git a/drivers/pci/controller/plda/pcie-microchip-host.c b/drivers/pci/controller/plda/pcie-microchip-host.c > index fd0d92c3d03f..ff40c1622173 100644 > --- a/drivers/pci/controller/plda/pcie-microchip-host.c > +++ b/drivers/pci/controller/plda/pcie-microchip-host.c > @@ -771,6 +771,63 @@ static struct irq_chip mc_event_irq_chip = { > .irq_unmask = mc_unmask_event_irq, > }; > > +static u32 plda_hwirq_to_mask(int hwirq) > +{ > + u32 mask; > + > + if (hwirq < EVENT_PM_MSI_INT_INTX) > + mask = BIT(hwirq + A_ATR_EVT_POST_ERR_SHIFT); > + else if (hwirq == EVENT_PM_MSI_INT_INTX) > + mask = PM_MSI_INT_INTX_MASK; > + else > + mask = BIT(hwirq + PM_MSI_TO_MASK_OFFSET); > + > + return mask; > +} > + > +static void plda_ack_event_irq(struct irq_data *data) > +{ > + struct plda_pcie_rp *port = irq_data_get_irq_chip_data(data); > + > + writel_relaxed(plda_hwirq_to_mask(data->hwirq), > + port->bridge_addr + ISTATUS_LOCAL); > +} > + > +static void plda_mask_event_irq(struct irq_data *data) > +{ > + struct plda_pcie_rp *port = irq_data_get_irq_chip_data(data); > + u32 mask, val; > + > + mask = plda_hwirq_to_mask(data->hwirq); > + > + raw_spin_lock(&port->lock); > + val = readl_relaxed(port->bridge_addr + IMASK_LOCAL); > + val &= ~mask; > + writel_relaxed(val, port->bridge_addr + IMASK_LOCAL); > + raw_spin_unlock(&port->lock); > +} > + > +static void plda_unmask_event_irq(struct irq_data *data) > +{ > + struct plda_pcie_rp *port = irq_data_get_irq_chip_data(data); > + u32 mask, val; > + > + mask = plda_hwirq_to_mask(data->hwirq); > + > + raw_spin_lock(&port->lock); > + val = readl_relaxed(port->bridge_addr + IMASK_LOCAL); > + val |= mask; > + writel_relaxed(val, port->bridge_addr + IMASK_LOCAL); > + raw_spin_unlock(&port->lock); > +} > + > +static struct irq_chip plda_event_irq_chip = { > + .name = "PLDA PCIe EVENT", > + .irq_ack = plda_ack_event_irq, > + .irq_mask = plda_mask_event_irq, > + .irq_unmask = plda_unmask_event_irq, > +}; > + > static const struct plda_event_ops plda_event_ops = { > .get_events = plda_get_events, > }; > @@ -778,7 +835,9 @@ static const struct plda_event_ops plda_event_ops = { > static int plda_pcie_event_map(struct irq_domain *domain, unsigned int irq, > irq_hw_number_t hwirq) > { > - irq_set_chip_and_handler(irq, &mc_event_irq_chip, handle_level_irq); > + struct plda_pcie_rp *port = (void *)domain->host_data; > + > + irq_set_chip_and_handler(irq, port->event_irq_chip, handle_level_irq); > irq_set_chip_data(irq, domain->host_data); > > return 0; > @@ -963,6 +1022,9 @@ static int plda_init_interrupts(struct platform_device *pdev, > if (!port->event_ops) > port->event_ops = &plda_event_ops; > > + if (!port->event_irq_chip) > + port->event_irq_chip = &plda_event_irq_chip; > + > ret = plda_pcie_init_irq_domains(port); > if (ret) { > dev_err(dev, "failed creating IRQ domains\n"); > @@ -1040,6 +1102,7 @@ static int mc_platform_init(struct pci_config_window *cfg) > return ret; > > port->plda.event_ops = &mc_event_ops; > + port->plda.event_irq_chip = &mc_event_irq_chip; > > /* Address translation is up; safe to enable interrupts */ > ret = plda_init_interrupts(pdev, &port->plda, &mc_event); > diff --git a/drivers/pci/controller/plda/pcie-plda.h b/drivers/pci/controller/plda/pcie-plda.h > index dd8bc2750bfc..24ac50c458dc 100644 > --- a/drivers/pci/controller/plda/pcie-plda.h > +++ b/drivers/pci/controller/plda/pcie-plda.h > @@ -128,6 +128,8 @@ > * DMA end : reserved for vendor implement > */ > > +#define PM_MSI_TO_MASK_OFFSET 19 > + > struct plda_pcie_rp; > > struct plda_event_ops { > @@ -150,6 +152,7 @@ struct plda_pcie_rp { > raw_spinlock_t lock; > struct plda_msi msi; > const struct plda_event_ops *event_ops; > + const struct irq_chip *event_irq_chip; > void __iomem *bridge_addr; > int num_events; > };
On Thu, Dec 21, 2023 at 06:56:22PM +0800, Minda Chen wrote: > > > On 2023/12/14 15:28, Minda Chen wrote: > > PolarFire PCIE event IRQs includes PLDA local interrupts and PolarFire > > their own IRQs. PolarFire PCIe event irq_chip ops using an event_desc to > > unify different IRQ register addresses. On PLDA sides, PLDA irqchip codes > > only require to set PLDA local interrupt register. So the PLDA irqchip ops > > codes can not be extracted from PolarFire codes. > > > > To support PLDA its own event IRQ process, implements PLDA irqchip ops and > > add event irqchip field to struct pcie_plda_rp. > > > > Signed-off-by: Minda Chen <minda.chen@starfivetech.com> > > --- > > .../pci/controller/plda/pcie-microchip-host.c | 65 ++++++++++++++++++- > > drivers/pci/controller/plda/pcie-plda.h | 3 + > > 2 files changed, 67 insertions(+), 1 deletion(-) > > > Hi Conor > Could you take time to review this patch? For I using event irq chip instead of event ops and the whole patch have been changed. I think it's better > And I added the implementation of PLDA event irqchip and make it easier to claim the necessity of the modification. > If you approve this, I will add back the review tag. Thanks > > Hi Lorenzo > Have you reviewed this patch？ Does the commit message and the codes are can be approved ？Thanks > Please wrap the lines at 75 columns in length. I have not reviewed but I am still struggling to understand the commit log, I apologise, I can try to review the series and figure out what the patch is doing but I would appreciate if commits logs could be made easier to parse. Thanks, Lorenzo > > diff --git a/drivers/pci/controller/plda/pcie-microchip-host.c b/drivers/pci/controller/plda/pcie-microchip-host.c > > index fd0d92c3d03f..ff40c1622173 100644 > > --- a/drivers/pci/controller/plda/pcie-microchip-host.c > > +++ b/drivers/pci/controller/plda/pcie-microchip-host.c > > @@ -771,6 +771,63 @@ static struct irq_chip mc_event_irq_chip = { > > .irq_unmask = mc_unmask_event_irq, > > }; > > > +static u32 plda_hwirq_to_mask(int hwirq) > > +{ > > + u32 mask; > > + > > + if (hwirq < EVENT_PM_MSI_INT_INTX) > > + mask = BIT(hwirq + A_ATR_EVT_POST_ERR_SHIFT); > > + else if (hwirq == EVENT_PM_MSI_INT_INTX) > > + mask = PM_MSI_INT_INTX_MASK; > > + else > > + mask = BIT(hwirq + PM_MSI_TO_MASK_OFFSET); > > + > > + return mask; > > +} > > + > > +static void plda_ack_event_irq(struct irq_data *data) > > +{ > > + struct plda_pcie_rp *port = irq_data_get_irq_chip_data(data); > > + > > + writel_relaxed(plda_hwirq_to_mask(data->hwirq), > > + port->bridge_addr + ISTATUS_LOCAL); > > +} > > + > > +static void plda_mask_event_irq(struct irq_data *data) > > +{ > > + struct plda_pcie_rp *port = irq_data_get_irq_chip_data(data); > > + u32 mask, val; > > + > > + mask = plda_hwirq_to_mask(data->hwirq); > > + > > + raw_spin_lock(&port->lock); > > + val = readl_relaxed(port->bridge_addr + IMASK_LOCAL); > > + val &= ~mask; > > + writel_relaxed(val, port->bridge_addr + IMASK_LOCAL); > > + raw_spin_unlock(&port->lock); > > +} > > + > > +static void plda_unmask_event_irq(struct irq_data *data) > > +{ > > + struct plda_pcie_rp *port = irq_data_get_irq_chip_data(data); > > + u32 mask, val; > > + > > + mask = plda_hwirq_to_mask(data->hwirq); > > + > > + raw_spin_lock(&port->lock); > > + val = readl_relaxed(port->bridge_addr + IMASK_LOCAL); > > + val |= mask; > > + writel_relaxed(val, port->bridge_addr + IMASK_LOCAL); > > + raw_spin_unlock(&port->lock); > > +} > > + > > +static struct irq_chip plda_event_irq_chip = { > > + .name = "PLDA PCIe EVENT", > > + .irq_ack = plda_ack_event_irq, > > + .irq_mask = plda_mask_event_irq, > > + .irq_unmask = plda_unmask_event_irq, > > +}; > > + > > static const struct plda_event_ops plda_event_ops = { > > .get_events = plda_get_events, > > }; > > @@ -778,7 +835,9 @@ static const struct plda_event_ops plda_event_ops = { > > static int plda_pcie_event_map(struct irq_domain *domain, unsigned int irq, > > irq_hw_number_t hwirq) > > { > > - irq_set_chip_and_handler(irq, &mc_event_irq_chip, handle_level_irq); > > + struct plda_pcie_rp *port = (void *)domain->host_data; > > + > > + irq_set_chip_and_handler(irq, port->event_irq_chip, handle_level_irq); > > irq_set_chip_data(irq, domain->host_data); > > > > return 0; > > @@ -963,6 +1022,9 @@ static int plda_init_interrupts(struct platform_device *pdev, > > if (!port->event_ops) > > port->event_ops = &plda_event_ops; > > > > + if (!port->event_irq_chip) > > + port->event_irq_chip = &plda_event_irq_chip; > > + > > ret = plda_pcie_init_irq_domains(port); > > if (ret) { > > dev_err(dev, "failed creating IRQ domains\n"); > > @@ -1040,6 +1102,7 @@ static int mc_platform_init(struct pci_config_window *cfg) > > return ret; > > > > port->plda.event_ops = &mc_event_ops; > > + port->plda.event_irq_chip = &mc_event_irq_chip; > > > > /* Address translation is up; safe to enable interrupts */ > > ret = plda_init_interrupts(pdev, &port->plda, &mc_event); > > diff --git a/drivers/pci/controller/plda/pcie-plda.h b/drivers/pci/controller/plda/pcie-plda.h > > index dd8bc2750bfc..24ac50c458dc 100644 > > --- a/drivers/pci/controller/plda/pcie-plda.h > > +++ b/drivers/pci/controller/plda/pcie-plda.h > > @@ -128,6 +128,8 @@ > > * DMA end : reserved for vendor implement > > */ > > > > +#define PM_MSI_TO_MASK_OFFSET 19 > > + > > struct plda_pcie_rp; > > > > struct plda_event_ops { > > @@ -150,6 +152,7 @@ struct plda_pcie_rp { > > raw_spinlock_t lock; > > struct plda_msi msi; > > const struct plda_event_ops *event_ops; > > + const struct irq_chip *event_irq_chip; > > void __iomem *bridge_addr; > > int num_events; > > };
On 2023/12/21 23:32, Lorenzo Pieralisi wrote: > On Thu, Dec 21, 2023 at 06:56:22PM +0800, Minda Chen wrote: >> >> >> On 2023/12/14 15:28, Minda Chen wrote: >> > PolarFire PCIE event IRQs includes PLDA local interrupts and PolarFire >> > their own IRQs. PolarFire PCIe event irq_chip ops using an event_desc to >> > unify different IRQ register addresses. On PLDA sides, PLDA irqchip codes >> > only require to set PLDA local interrupt register. So the PLDA irqchip ops >> > codes can not be extracted from PolarFire codes. >> > >> > To support PLDA its own event IRQ process, implements PLDA irqchip ops and >> > add event irqchip field to struct pcie_plda_rp. >> > >> > Signed-off-by: Minda Chen <minda.chen@starfivetech.com> >> > --- >> > .../pci/controller/plda/pcie-microchip-host.c | 65 ++++++++++++++++++- >> > drivers/pci/controller/plda/pcie-plda.h | 3 + >> > 2 files changed, 67 insertions(+), 1 deletion(-) >> > >> Hi Conor >> Could you take time to review this patch? For I using event irq chip instead of event ops and the whole patch have been changed. I think it's better >> And I added the implementation of PLDA event irqchip and make it easier to claim the necessity of the modification. >> If you approve this, I will add back the review tag. Thanks >> >> Hi Lorenzo >> Have you reviewed this patch？ Does the commit message and the codes are can be approved ？Thanks >> > > Please wrap the lines at 75 columns in length. > OK > I have not reviewed but I am still struggling to understand the > commit log, I apologise, I can try to review the series and figure > out what the patch is doing but I would appreciate if commits logs > could be made easier to parse. > > Thanks, > Lorenzo > The commit message it is not good. I draw a graph about the PCIe global event interrupt domain (related to patch 10- 16). Actually all these interrupts patches are for extracting the common PLDA codes to pcie-plda-host.c and do not change microchip's codes logic. +----------------------------------------------------------+ | microchip Global event interrupt domain | +-----------------------------------+-----------+----------+ | | microchip | PLDA | | | event num |(StarFive)| | | |event num | +-----------------------------------+-----------+----------+ | | 0 | | | | | | (mc pcie |microchip platform event interrupt | | | int line) | | | | ------------| | | | | |10 | | +-----------------------------------+-----------+----------+ | PLDA host DMA interrupt |11 | | | (int number is not fixed, defined | | | | by vendor) |14 | | +--+-----------------------------------+---------- +----------+-+ | | PLDA event interrupt |15 |0 | | | | (int number is fixed) | | | | ---------|--| | | | | (Starfive| | | | | | pcie int | | +------------------+ | | | | line) | | |INTx event domain | | | | | | | +------------------+ | | | | | | | | | | | | +------------------+ | | | | | | |MSI event domain | | | | | | | +------------------+ | | | | | | |27 |12 | | | +---------------------------------+-+-----------+----------+ | | extract PLDA event part to common PLDA file. | +---------------------------------------------------------------+ >> > diff --git a/drivers/pci/controller/plda/pcie-microchip-host.c b/drivers/pci/controller/plda/pcie-microchip-host.c >> > index fd0d92c3d03f..ff40c1622173 100644 >> > --- a/drivers/pci/controller/plda/pcie-microchip-host.c >> > +++ b/drivers/pci/controller/plda/pcie-microchip-host.c >> > @@ -771,6 +771,63 @@ static struct irq_chip mc_event_irq_chip = { >> > .irq_unmask = mc_unmask_event_irq, >> > }; >> > > +static u32 plda_hwirq_to_mask(int hwirq) >> > +{ >> > + u32 mask; >> > + >> > + if (hwirq < EVENT_PM_MSI_INT_INTX) >> > + mask = BIT(hwirq + A_ATR_EVT_POST_ERR_SHIFT); >> > + else if (hwirq == EVENT_PM_MSI_INT_INTX) >> > + mask = PM_MSI_INT_INTX_MASK; >> > + else >> > + mask = BIT(hwirq + PM_MSI_TO_MASK_OFFSET); >> > + >> > + return mask; >> > +} >> > + >> > +static void plda_ack_event_irq(struct irq_data *data) >> > +{ >> > + struct plda_pcie_rp *port = irq_data_get_irq_chip_data(data); >> > + >> > + writel_relaxed(plda_hwirq_to_mask(data->hwirq), >> > + port->bridge_addr + ISTATUS_LOCAL); >> > +} >> > + >> > +static void plda_mask_event_irq(struct irq_data *data) >> > +{ >> > + struct plda_pcie_rp *port = irq_data_get_irq_chip_data(data); >> > + u32 mask, val; >> > + >> > + mask = plda_hwirq_to_mask(data->hwirq); >> > + >> > + raw_spin_lock(&port->lock); >> > + val = readl_relaxed(port->bridge_addr + IMASK_LOCAL); >> > + val &= ~mask; >> > + writel_relaxed(val, port->bridge_addr + IMASK_LOCAL); >> > + raw_spin_unlock(&port->lock); >> > +} >> > + >> > +static void plda_unmask_event_irq(struct irq_data *data) >> > +{ >> > + struct plda_pcie_rp *port = irq_data_get_irq_chip_data(data); >> > + u32 mask, val; >> > + >> > + mask = plda_hwirq_to_mask(data->hwirq); >> > + >> > + raw_spin_lock(&port->lock); >> > + val = readl_relaxed(port->bridge_addr + IMASK_LOCAL); >> > + val |= mask; >> > + writel_relaxed(val, port->bridge_addr + IMASK_LOCAL); >> > + raw_spin_unlock(&port->lock); >> > +} >> > + >> > +static struct irq_chip plda_event_irq_chip = { >> > + .name = "PLDA PCIe EVENT", >> > + .irq_ack = plda_ack_event_irq, >> > + .irq_mask = plda_mask_event_irq, >> > + .irq_unmask = plda_unmask_event_irq, >> > +}; >> > + >> > static const struct plda_event_ops plda_event_ops = { >> > .get_events = plda_get_events, >> > }; >> > @@ -778,7 +835,9 @@ static const struct plda_event_ops plda_event_ops = { >> > static int plda_pcie_event_map(struct irq_domain *domain, unsigned int irq, >> > irq_hw_number_t hwirq) >> > { >> > - irq_set_chip_and_handler(irq, &mc_event_irq_chip, handle_level_irq); >> > + struct plda_pcie_rp *port = (void *)domain->host_data; >> > + >> > + irq_set_chip_and_handler(irq, port->event_irq_chip, handle_level_irq); >> > irq_set_chip_data(irq, domain->host_data); >> > >> > return 0; >> > @@ -963,6 +1022,9 @@ static int plda_init_interrupts(struct platform_device *pdev, >> > if (!port->event_ops) >> > port->event_ops = &plda_event_ops; >> > >> > + if (!port->event_irq_chip) >> > + port->event_irq_chip = &plda_event_irq_chip; >> > + >> > ret = plda_pcie_init_irq_domains(port); >> > if (ret) { >> > dev_err(dev, "failed creating IRQ domains\n"); >> > @@ -1040,6 +1102,7 @@ static int mc_platform_init(struct pci_config_window *cfg) >> > return ret; >> > >> > port->plda.event_ops = &mc_event_ops; >> > + port->plda.event_irq_chip = &mc_event_irq_chip; >> > >> > /* Address translation is up; safe to enable interrupts */ >> > ret = plda_init_interrupts(pdev, &port->plda, &mc_event); >> > diff --git a/drivers/pci/controller/plda/pcie-plda.h b/drivers/pci/controller/plda/pcie-plda.h >> > index dd8bc2750bfc..24ac50c458dc 100644 >> > --- a/drivers/pci/controller/plda/pcie-plda.h >> > +++ b/drivers/pci/controller/plda/pcie-plda.h >> > @@ -128,6 +128,8 @@ >> > * DMA end : reserved for vendor implement >> > */ >> > >> > +#define PM_MSI_TO_MASK_OFFSET 19 >> > + >> > struct plda_pcie_rp; >> > >> > struct plda_event_ops { >> > @@ -150,6 +152,7 @@ struct plda_pcie_rp { >> > raw_spinlock_t lock; >> > struct plda_msi msi; >> > const struct plda_event_ops *event_ops; >> > + const struct irq_chip *event_irq_chip; >> > void __iomem *bridge_addr; >> > int num_events; >> > };
[+Thomas] On Fri, Dec 22, 2023 at 07:18:48PM +0800, Minda Chen wrote: > > > On 2023/12/21 23:32, Lorenzo Pieralisi wrote: > > On Thu, Dec 21, 2023 at 06:56:22PM +0800, Minda Chen wrote: > >> > >> > >> On 2023/12/14 15:28, Minda Chen wrote: > >> > PolarFire PCIE event IRQs includes PLDA local interrupts and PolarFire > >> > their own IRQs. PolarFire PCIe event irq_chip ops using an event_desc to > >> > unify different IRQ register addresses. On PLDA sides, PLDA irqchip codes > >> > only require to set PLDA local interrupt register. So the PLDA irqchip ops > >> > codes can not be extracted from PolarFire codes. > >> > > >> > To support PLDA its own event IRQ process, implements PLDA irqchip ops and > >> > add event irqchip field to struct pcie_plda_rp. > >> > > >> > Signed-off-by: Minda Chen <minda.chen@starfivetech.com> > >> > --- > >> > .../pci/controller/plda/pcie-microchip-host.c | 65 ++++++++++++++++++- > >> > drivers/pci/controller/plda/pcie-plda.h | 3 + > >> > 2 files changed, 67 insertions(+), 1 deletion(-) > >> > > >> Hi Conor > >> Could you take time to review this patch? For I using event irq chip instead of event ops and the whole patch have been changed. I think it's better > >> And I added the implementation of PLDA event irqchip and make it easier to claim the necessity of the modification. > >> If you approve this, I will add back the review tag. Thanks > >> > >> Hi Lorenzo > >> Have you reviewed this patch？ Does the commit message and the codes are can be approved ？Thanks > >> > > > > Please wrap the lines at 75 columns in length. > > > OK > > I have not reviewed but I am still struggling to understand the > > commit log, I apologise, I can try to review the series and figure > > out what the patch is doing but I would appreciate if commits logs > > could be made easier to parse. > > > > Thanks, > > Lorenzo > > > > The commit message it is not good. > > I draw a graph about the PCIe global event interrupt domain > (related to patch 10- 16). > Actually all these interrupts patches are for extracting the common > PLDA codes to pcie-plda-host.c and do not change microchip's codes logic. s/codes/code (please apply this to the the full series) I will have a look at the code but I can't rewrite the commit log myself (it does not scale I am afraid), as it stands I don't understand it and that's a problem, I am sorry but that's important. I added Thomas (you should CC him for irqchip [only] changes) if he has time to review these irqchip changes to make sure they are proper. Thanks, Lorenzo > +----------------------------------------------------------+ > | microchip Global event interrupt domain | > +-----------------------------------+-----------+----------+ > | | microchip | PLDA | > | | event num |(StarFive)| > | | |event num | > +-----------------------------------+-----------+----------+ > | | 0 | | > | | | | > (mc pcie |microchip platform event interrupt | | | > int line) | | | | > ------------| | | | > | |10 | | > +-----------------------------------+-----------+----------+ > | PLDA host DMA interrupt |11 | | > | (int number is not fixed, defined | | | > | by vendor) |14 | | > +--+-----------------------------------+---------- +----------+-+ > | | PLDA event interrupt |15 |0 | | > | | (int number is fixed) | | | | > ---------|--| | | | | > (Starfive| | | | | | > pcie int | | +------------------+ | | | | > line) | | |INTx event domain | | | | | > | | +------------------+ | | | | > | | | | | | > | | +------------------+ | | | | > | | |MSI event domain | | | | | > | | +------------------+ | | | | > | | |27 |12 | | > | +---------------------------------+-+-----------+----------+ | > | extract PLDA event part to common PLDA file. | > +---------------------------------------------------------------+ > > > >> > diff --git a/drivers/pci/controller/plda/pcie-microchip-host.c b/drivers/pci/controller/plda/pcie-microchip-host.c > >> > index fd0d92c3d03f..ff40c1622173 100644 > >> > --- a/drivers/pci/controller/plda/pcie-microchip-host.c > >> > +++ b/drivers/pci/controller/plda/pcie-microchip-host.c > >> > @@ -771,6 +771,63 @@ static struct irq_chip mc_event_irq_chip = { > >> > .irq_unmask = mc_unmask_event_irq, > >> > }; > >> > > +static u32 plda_hwirq_to_mask(int hwirq) > >> > +{ > >> > + u32 mask; > >> > + > >> > + if (hwirq < EVENT_PM_MSI_INT_INTX) > >> > + mask = BIT(hwirq + A_ATR_EVT_POST_ERR_SHIFT); > >> > + else if (hwirq == EVENT_PM_MSI_INT_INTX) > >> > + mask = PM_MSI_INT_INTX_MASK; > >> > + else > >> > + mask = BIT(hwirq + PM_MSI_TO_MASK_OFFSET); > >> > + > >> > + return mask; > >> > +} > >> > + > >> > +static void plda_ack_event_irq(struct irq_data *data) > >> > +{ > >> > + struct plda_pcie_rp *port = irq_data_get_irq_chip_data(data); > >> > + > >> > + writel_relaxed(plda_hwirq_to_mask(data->hwirq), > >> > + port->bridge_addr + ISTATUS_LOCAL); > >> > +} > >> > + > >> > +static void plda_mask_event_irq(struct irq_data *data) > >> > +{ > >> > + struct plda_pcie_rp *port = irq_data_get_irq_chip_data(data); > >> > + u32 mask, val; > >> > + > >> > + mask = plda_hwirq_to_mask(data->hwirq); > >> > + > >> > + raw_spin_lock(&port->lock); > >> > + val = readl_relaxed(port->bridge_addr + IMASK_LOCAL); > >> > + val &= ~mask; > >> > + writel_relaxed(val, port->bridge_addr + IMASK_LOCAL); > >> > + raw_spin_unlock(&port->lock); > >> > +} > >> > + > >> > +static void plda_unmask_event_irq(struct irq_data *data) > >> > +{ > >> > + struct plda_pcie_rp *port = irq_data_get_irq_chip_data(data); > >> > + u32 mask, val; > >> > + > >> > + mask = plda_hwirq_to_mask(data->hwirq); > >> > + > >> > + raw_spin_lock(&port->lock); > >> > + val = readl_relaxed(port->bridge_addr + IMASK_LOCAL); > >> > + val |= mask; > >> > + writel_relaxed(val, port->bridge_addr + IMASK_LOCAL); > >> > + raw_spin_unlock(&port->lock); > >> > +} > >> > + > >> > +static struct irq_chip plda_event_irq_chip = { > >> > + .name = "PLDA PCIe EVENT", > >> > + .irq_ack = plda_ack_event_irq, > >> > + .irq_mask = plda_mask_event_irq, > >> > + .irq_unmask = plda_unmask_event_irq, > >> > +}; > >> > + > >> > static const struct plda_event_ops plda_event_ops = { > >> > .get_events = plda_get_events, > >> > }; > >> > @@ -778,7 +835,9 @@ static const struct plda_event_ops plda_event_ops = { > >> > static int plda_pcie_event_map(struct irq_domain *domain, unsigned int irq, > >> > irq_hw_number_t hwirq) > >> > { > >> > - irq_set_chip_and_handler(irq, &mc_event_irq_chip, handle_level_irq); > >> > + struct plda_pcie_rp *port = (void *)domain->host_data; > >> > + > >> > + irq_set_chip_and_handler(irq, port->event_irq_chip, handle_level_irq); > >> > irq_set_chip_data(irq, domain->host_data); > >> > > >> > return 0; > >> > @@ -963,6 +1022,9 @@ static int plda_init_interrupts(struct platform_device *pdev, > >> > if (!port->event_ops) > >> > port->event_ops = &plda_event_ops; > >> > > >> > + if (!port->event_irq_chip) > >> > + port->event_irq_chip = &plda_event_irq_chip; > >> > + > >> > ret = plda_pcie_init_irq_domains(port); > >> > if (ret) { > >> > dev_err(dev, "failed creating IRQ domains\n"); > >> > @@ -1040,6 +1102,7 @@ static int mc_platform_init(struct pci_config_window *cfg) > >> > return ret; > >> > > >> > port->plda.event_ops = &mc_event_ops; > >> > + port->plda.event_irq_chip = &mc_event_irq_chip; > >> > > >> > /* Address translation is up; safe to enable interrupts */ > >> > ret = plda_init_interrupts(pdev, &port->plda, &mc_event); > >> > diff --git a/drivers/pci/controller/plda/pcie-plda.h b/drivers/pci/controller/plda/pcie-plda.h > >> > index dd8bc2750bfc..24ac50c458dc 100644 > >> > --- a/drivers/pci/controller/plda/pcie-plda.h > >> > +++ b/drivers/pci/controller/plda/pcie-plda.h > >> > @@ -128,6 +128,8 @@ > >> > * DMA end : reserved for vendor implement > >> > */ > >> > > >> > +#define PM_MSI_TO_MASK_OFFSET 19 > >> > + > >> > struct plda_pcie_rp; > >> > > >> > struct plda_event_ops { > >> > @@ -150,6 +152,7 @@ struct plda_pcie_rp { > >> > raw_spinlock_t lock; > >> > struct plda_msi msi; > >> > const struct plda_event_ops *event_ops; > >> > + const struct irq_chip *event_irq_chip; > >> > void __iomem *bridge_addr; > >> > int num_events; > >> > };
On 2023/12/27 20:43, Lorenzo Pieralisi wrote: > [+Thomas] > > On Fri, Dec 22, 2023 at 07:18:48PM +0800, Minda Chen wrote: >> >> >> On 2023/12/21 23:32, Lorenzo Pieralisi wrote: >> > On Thu, Dec 21, 2023 at 06:56:22PM +0800, Minda Chen wrote: >> >> >> >> >> >> On 2023/12/14 15:28, Minda Chen wrote: >> >> > PolarFire PCIE event IRQs includes PLDA local interrupts and PolarFire >> >> > their own IRQs. PolarFire PCIe event irq_chip ops using an event_desc to >> >> > unify different IRQ register addresses. On PLDA sides, PLDA irqchip codes >> >> > only require to set PLDA local interrupt register. So the PLDA irqchip ops >> >> > codes can not be extracted from PolarFire codes. >> >> > >> >> > To support PLDA its own event IRQ process, implements PLDA irqchip ops and >> >> > add event irqchip field to struct pcie_plda_rp. >> >> > >> >> > Signed-off-by: Minda Chen <minda.chen@starfivetech.com> >> >> > --- >> >> > .../pci/controller/plda/pcie-microchip-host.c | 65 ++++++++++++++++++- >> >> > drivers/pci/controller/plda/pcie-plda.h | 3 + >> >> > 2 files changed, 67 insertions(+), 1 deletion(-) >> >> > >> >> Hi Conor >> >> Could you take time to review this patch? For I using event irq chip instead of event ops and the whole patch have been changed. I think it's better >> >> And I added the implementation of PLDA event irqchip and make it easier to claim the necessity of the modification. >> >> If you approve this, I will add back the review tag. Thanks >> >> >> >> Hi Lorenzo >> >> Have you reviewed this patch？ Does the commit message and the codes are can be approved ？Thanks >> >> >> > >> > Please wrap the lines at 75 columns in length. >> > >> OK >> > I have not reviewed but I am still struggling to understand the >> > commit log, I apologise, I can try to review the series and figure >> > out what the patch is doing but I would appreciate if commits logs >> > could be made easier to parse. >> > >> > Thanks, >> > Lorenzo >> > >> >> The commit message it is not good. >> >> I draw a graph about the PCIe global event interrupt domain >> (related to patch 10- 16). >> Actually all these interrupts patches are for extracting the common >> PLDA codes to pcie-plda-host.c and do not change microchip's codes logic. > > s/codes/code (please apply this to the the full series) > > I will have a look at the code but I can't rewrite the commit log myself > (it does not scale I am afraid), as it stands I don't understand it and > that's a problem, I am sorry but that's important. > > I added Thomas (you should CC him for irqchip [only] changes) if he > has time to review these irqchip changes to make sure they are proper. > > Thanks, > Lorenzo > The interrupt irq_chip ops includes ack/mask/unmask. These ops are for writing the correct register. Microchip Implement their PCIe interrupts and require to write their registers. So the irq_chip ops are different. (List below are the microchip interrupt register base and status/mask register offset. In pcie-microchip-host.c:130) #define PCIE_EVENT(x) \ .base = MC_PCIE_CTRL_ADDR, \ .offset = PCIE_EVENT_INT, \ .mask_offset = PCIE_EVENT_INT, \ .mask_high = 1, \ .mask = PCIE_EVENT_INT_ ## x ## _INT, \ .enb_mask = PCIE_EVENT_INT_ENB_MASK #define SEC_EVENT(x) \ .base = MC_PCIE_CTRL_ADDR, \ .offset = SEC_ERROR_INT, \ .mask_offset = SEC_ERROR_INT_MASK, \ .mask = SEC_ERROR_INT_ ## x ## _INT, \ .mask_high = 1, \ .enb_mask = 0 #define DED_EVENT(x) \ .base = MC_PCIE_CTRL_ADDR, \ .offset = DED_ERROR_INT, \ .mask_offset = DED_ERROR_INT_MASK, \ .mask_high = 1, \ .mask = DED_ERROR_INT_ ## x ## _INT, \ .enb_mask = 0 >> +----------------------------------------------------------+ >> | microchip Global event interrupt domain | >> +-----------------------------------+-----------+----------+ >> | | microchip | PLDA | >> | | event num |(StarFive)| >> | | |event num | >> +-----------------------------------+-----------+----------+ >> | | 0 | | >> | | | | >> (mc pcie |microchip platform event interrupt | | | >> int line) | | | | >> ------------| | | | >> | |10 | | >> +-----------------------------------+-----------+----------+ >> | PLDA host DMA interrupt |11 | | >> | (int number is not fixed, defined | | | >> | by vendor) |14 | | >> +--+-----------------------------------+---------- +----------+-+ >> | | PLDA event interrupt |15 |0 | | >> | | (int number is fixed) | | | | >> ---------|--| | | | | >> (Starfive| | | | | | >> pcie int | | +------------------+ | | | | >> line) | | |INTx event domain | | | | | >> | | +------------------+ | | | | >> | | | | | | >> | | +------------------+ | | | | >> | | |MSI event domain | | | | | >> | | +------------------+ | | | | >> | | |27 |12 | | >> | +---------------------------------+-+-----------+----------+ | >> | extract PLDA event part to common PLDA file. | >> +---------------------------------------------------------------+ >> >> >> >> > diff --git a/drivers/pci/controller/plda/pcie-microchip-host.c b/drivers/pci/controller/plda/pcie-microchip-host.c >> >> > index fd0d92c3d03f..ff40c1622173 100644 >> >> > --- a/drivers/pci/controller/plda/pcie-microchip-host.c >> >> > +++ b/drivers/pci/controller/plda/pcie-microchip-host.c >> >> > @@ -771,6 +771,63 @@ static struct irq_chip mc_event_irq_chip = { >> >> > .irq_unmask = mc_unmask_event_irq, >> >> > }; >> >> > > +static u32 plda_hwirq_to_mask(int hwirq) >> >> > +{ >> >> > + u32 mask; >> >> > + >> >> > + if (hwirq < EVENT_PM_MSI_INT_INTX) >> >> > + mask = BIT(hwirq + A_ATR_EVT_POST_ERR_SHIFT); >> >> > + else if (hwirq == EVENT_PM_MSI_INT_INTX) >> >> > + mask = PM_MSI_INT_INTX_MASK; >> >> > + else >> >> > + mask = BIT(hwirq + PM_MSI_TO_MASK_OFFSET); >> >> > + >> >> > + return mask; >> >> > +} >> >> > + >> >> > +static void plda_ack_event_irq(struct irq_data *data) >> >> > +{ >> >> > + struct plda_pcie_rp *port = irq_data_get_irq_chip_data(data); >> >> > + >> >> > + writel_relaxed(plda_hwirq_to_mask(data->hwirq), >> >> > + port->bridge_addr + ISTATUS_LOCAL); >> >> > +} >> >> > + >> >> > +static void plda_mask_event_irq(struct irq_data *data) >> >> > +{ >> >> > + struct plda_pcie_rp *port = irq_data_get_irq_chip_data(data); >> >> > + u32 mask, val; >> >> > + >> >> > + mask = plda_hwirq_to_mask(data->hwirq); >> >> > + >> >> > + raw_spin_lock(&port->lock); >> >> > + val = readl_relaxed(port->bridge_addr + IMASK_LOCAL); >> >> > + val &= ~mask; >> >> > + writel_relaxed(val, port->bridge_addr + IMASK_LOCAL); >> >> > + raw_spin_unlock(&port->lock); >> >> > +} >> >> > + >> >> > +static void plda_unmask_event_irq(struct irq_data *data) >> >> > +{ >> >> > + struct plda_pcie_rp *port = irq_data_get_irq_chip_data(data); >> >> > + u32 mask, val; >> >> > + >> >> > + mask = plda_hwirq_to_mask(data->hwirq); >> >> > + >> >> > + raw_spin_lock(&port->lock); >> >> > + val = readl_relaxed(port->bridge_addr + IMASK_LOCAL); >> >> > + val |= mask; >> >> > + writel_relaxed(val, port->bridge_addr + IMASK_LOCAL); >> >> > + raw_spin_unlock(&port->lock); >> >> > +} >> >> > + >> >> > +static struct irq_chip plda_event_irq_chip = { >> >> > + .name = "PLDA PCIe EVENT", >> >> > + .irq_ack = plda_ack_event_irq, >> >> > + .irq_mask = plda_mask_event_irq, >> >> > + .irq_unmask = plda_unmask_event_irq, >> >> > +}; >> >> > + >> >> > static const struct plda_event_ops plda_event_ops = { >> >> > .get_events = plda_get_events, >> >> > }; >> >> > @@ -778,7 +835,9 @@ static const struct plda_event_ops plda_event_ops = { >> >> > static int plda_pcie_event_map(struct irq_domain *domain, unsigned int irq, >> >> > irq_hw_number_t hwirq) >> >> > { >> >> > - irq_set_chip_and_handler(irq, &mc_event_irq_chip, handle_level_irq); >> >> > + struct plda_pcie_rp *port = (void *)domain->host_data; >> >> > + >> >> > + irq_set_chip_and_handler(irq, port->event_irq_chip, handle_level_irq); >> >> > irq_set_chip_data(irq, domain->host_data); >> >> > >> >> > return 0; >> >> > @@ -963,6 +1022,9 @@ static int plda_init_interrupts(struct platform_device *pdev, >> >> > if (!port->event_ops) >> >> > port->event_ops = &plda_event_ops; >> >> > >> >> > + if (!port->event_irq_chip) >> >> > + port->event_irq_chip = &plda_event_irq_chip; >> >> > + >> >> > ret = plda_pcie_init_irq_domains(port); >> >> > if (ret) { >> >> > dev_err(dev, "failed creating IRQ domains\n"); >> >> > @@ -1040,6 +1102,7 @@ static int mc_platform_init(struct pci_config_window *cfg) >> >> > return ret; >> >> > >> >> > port->plda.event_ops = &mc_event_ops; >> >> > + port->plda.event_irq_chip = &mc_event_irq_chip; >> >> > >> >> > /* Address translation is up; safe to enable interrupts */ >> >> > ret = plda_init_interrupts(pdev, &port->plda, &mc_event); >> >> > diff --git a/drivers/pci/controller/plda/pcie-plda.h b/drivers/pci/controller/plda/pcie-plda.h >> >> > index dd8bc2750bfc..24ac50c458dc 100644 >> >> > --- a/drivers/pci/controller/plda/pcie-plda.h >> >> > +++ b/drivers/pci/controller/plda/pcie-plda.h >> >> > @@ -128,6 +128,8 @@ >> >> > * DMA end : reserved for vendor implement >> >> > */ >> >> > >> >> > +#define PM_MSI_TO_MASK_OFFSET 19 >> >> > + >> >> > struct plda_pcie_rp; >> >> > >> >> > struct plda_event_ops { >> >> > @@ -150,6 +152,7 @@ struct plda_pcie_rp { >> >> > raw_spinlock_t lock; >> >> > struct plda_msi msi; >> >> > const struct plda_event_ops *event_ops; >> >> > + const struct irq_chip *event_irq_chip; >> >> > void __iomem *bridge_addr; >> >> > int num_events; >> >> > };
diff --git a/drivers/pci/controller/plda/pcie-microchip-host.c b/drivers/pci/controller/plda/pcie-microchip-host.c index fd0d92c3d03f..ff40c1622173 100644 --- a/drivers/pci/controller/plda/pcie-microchip-host.c +++ b/drivers/pci/controller/plda/pcie-microchip-host.c @@ -771,6 +771,63 @@ static struct irq_chip mc_event_irq_chip = { .irq_unmask = mc_unmask_event_irq, }; +static u32 plda_hwirq_to_mask(int hwirq) +{ + u32 mask; + + if (hwirq < EVENT_PM_MSI_INT_INTX) + mask = BIT(hwirq + A_ATR_EVT_POST_ERR_SHIFT); + else if (hwirq == EVENT_PM_MSI_INT_INTX) + mask = PM_MSI_INT_INTX_MASK; + else + mask = BIT(hwirq + PM_MSI_TO_MASK_OFFSET); + + return mask; +} + +static void plda_ack_event_irq(struct irq_data *data) +{ + struct plda_pcie_rp *port = irq_data_get_irq_chip_data(data); + + writel_relaxed(plda_hwirq_to_mask(data->hwirq), + port->bridge_addr + ISTATUS_LOCAL); +} + +static void plda_mask_event_irq(struct irq_data *data) +{ + struct plda_pcie_rp *port = irq_data_get_irq_chip_data(data); + u32 mask, val; + + mask = plda_hwirq_to_mask(data->hwirq); + + raw_spin_lock(&port->lock); + val = readl_relaxed(port->bridge_addr + IMASK_LOCAL); + val &= ~mask; + writel_relaxed(val, port->bridge_addr + IMASK_LOCAL); + raw_spin_unlock(&port->lock); +} + +static void plda_unmask_event_irq(struct irq_data *data) +{ + struct plda_pcie_rp *port = irq_data_get_irq_chip_data(data); + u32 mask, val; + + mask = plda_hwirq_to_mask(data->hwirq); + + raw_spin_lock(&port->lock); + val = readl_relaxed(port->bridge_addr + IMASK_LOCAL); + val |= mask; + writel_relaxed(val, port->bridge_addr + IMASK_LOCAL); + raw_spin_unlock(&port->lock); +} + +static struct irq_chip plda_event_irq_chip = { + .name = "PLDA PCIe EVENT", + .irq_ack = plda_ack_event_irq, + .irq_mask = plda_mask_event_irq, + .irq_unmask = plda_unmask_event_irq, +}; + static const struct plda_event_ops plda_event_ops = { .get_events = plda_get_events, }; @@ -778,7 +835,9 @@ static const struct plda_event_ops plda_event_ops = { static int plda_pcie_event_map(struct irq_domain *domain, unsigned int irq, irq_hw_number_t hwirq) { - irq_set_chip_and_handler(irq, &mc_event_irq_chip, handle_level_irq); + struct plda_pcie_rp *port = (void *)domain->host_data; + + irq_set_chip_and_handler(irq, port->event_irq_chip, handle_level_irq); irq_set_chip_data(irq, domain->host_data); return 0; @@ -963,6 +1022,9 @@ static int plda_init_interrupts(struct platform_device *pdev, if (!port->event_ops) port->event_ops = &plda_event_ops; + if (!port->event_irq_chip) + port->event_irq_chip = &plda_event_irq_chip; + ret = plda_pcie_init_irq_domains(port); if (ret) { dev_err(dev, "failed creating IRQ domains\n"); @@ -1040,6 +1102,7 @@ static int mc_platform_init(struct pci_config_window *cfg) return ret; port->plda.event_ops = &mc_event_ops; + port->plda.event_irq_chip = &mc_event_irq_chip; /* Address translation is up; safe to enable interrupts */ ret = plda_init_interrupts(pdev, &port->plda, &mc_event); diff --git a/drivers/pci/controller/plda/pcie-plda.h b/drivers/pci/controller/plda/pcie-plda.h index dd8bc2750bfc..24ac50c458dc 100644 --- a/drivers/pci/controller/plda/pcie-plda.h +++ b/drivers/pci/controller/plda/pcie-plda.h @@ -128,6 +128,8 @@ * DMA end : reserved for vendor implement */ +#define PM_MSI_TO_MASK_OFFSET 19 + struct plda_pcie_rp; struct plda_event_ops { @@ -150,6 +152,7 @@ struct plda_pcie_rp { raw_spinlock_t lock; struct plda_msi msi; const struct plda_event_ops *event_ops; + const struct irq_chip *event_irq_chip; void __iomem *bridge_addr; int num_events; };
PolarFire PCIE event IRQs includes PLDA local interrupts and PolarFire their own IRQs. PolarFire PCIe event irq_chip ops using an event_desc to unify different IRQ register addresses. On PLDA sides, PLDA irqchip codes only require to set PLDA local interrupt register. So the PLDA irqchip ops codes can not be extracted from PolarFire codes. To support PLDA its own event IRQ process, implements PLDA irqchip ops and add event irqchip field to struct pcie_plda_rp. Signed-off-by: Minda Chen <minda.chen@starfivetech.com> --- .../pci/controller/plda/pcie-microchip-host.c | 65 ++++++++++++++++++- drivers/pci/controller/plda/pcie-plda.h | 3 + 2 files changed, 67 insertions(+), 1 deletion(-)