Message ID | 20231214072839.2367-1-minda.chen@starfivetech.com |
---|---|
Headers | show |
Series | Refactoring Microchip PCIe driver and add StarFive PCIe | expand |
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 Thu, Dec 14, 2023 at 03:28:27PM +0800, Minda Chen wrote: > Move setup functions to common pcie-plda-host.c. So these two functions > can be re-used. > > Signed-off-by: Minda Chen <minda.chen@starfivetech.com> > Reviewed-by: Conor Dooley <conor.dooley@microchip.com> > --- > drivers/pci/controller/plda/Kconfig | 4 + > drivers/pci/controller/plda/Makefile | 1 + > .../pci/controller/plda/pcie-microchip-host.c | 59 -------------- > drivers/pci/controller/plda/pcie-plda-host.c | 80 +++++++++++++++++++ > drivers/pci/controller/plda/pcie-plda.h | 5 ++ > 5 files changed, 90 insertions(+), 59 deletions(-) > create mode 100644 drivers/pci/controller/plda/pcie-plda-host.c > > diff --git a/drivers/pci/controller/plda/Kconfig b/drivers/pci/controller/plda/Kconfig > index 5cb3be4fc98c..e54a82ee94f5 100644 > --- a/drivers/pci/controller/plda/Kconfig > +++ b/drivers/pci/controller/plda/Kconfig > @@ -3,10 +3,14 @@ > menu "PLDA-based PCIe controllers" > depends on PCI > > +config PCIE_PLDA_HOST > + bool > + > config PCIE_MICROCHIP_HOST > tristate "Microchip AXI PCIe controller" > depends on PCI_MSI && OF > select PCI_HOST_COMMON > + select PCIE_PLDA_HOST > help > Say Y here if you want kernel to support the Microchip AXI PCIe > Host Bridge driver. > diff --git a/drivers/pci/controller/plda/Makefile b/drivers/pci/controller/plda/Makefile > index e1a265cbf91c..4340ab007f44 100644 > --- a/drivers/pci/controller/plda/Makefile > +++ b/drivers/pci/controller/plda/Makefile > @@ -1,2 +1,3 @@ > # SPDX-License-Identifier: GPL-2.0 > +obj-$(CONFIG_PCIE_PLDA_HOST) += pcie-plda-host.o > obj-$(CONFIG_PCIE_MICROCHIP_HOST) += pcie-microchip-host.o > diff --git a/drivers/pci/controller/plda/pcie-microchip-host.c b/drivers/pci/controller/plda/pcie-microchip-host.c > index 31ca8d44ee2a..2e79bcc7c0a5 100644 > --- a/drivers/pci/controller/plda/pcie-microchip-host.c > +++ b/drivers/pci/controller/plda/pcie-microchip-host.c > @@ -838,65 +838,6 @@ static int mc_pcie_init_irq_domains(struct plda_pcie_rp *port) > return mc_allocate_msi_domains(port); > } > > -static void plda_pcie_setup_window(void __iomem *bridge_base_addr, u32 index, > - phys_addr_t axi_addr, phys_addr_t pci_addr, > - size_t size) > -{ > - u32 atr_sz = ilog2(size) - 1; > - u32 val; > - > - if (index == 0) > - val = PCIE_CONFIG_INTERFACE; > - else > - val = PCIE_TX_RX_INTERFACE; > - > - writel(val, bridge_base_addr + (index * ATR_ENTRY_SIZE) + > - ATR0_AXI4_SLV0_TRSL_PARAM); > - > - val = lower_32_bits(axi_addr) | (atr_sz << ATR_SIZE_SHIFT) | > - ATR_IMPL_ENABLE; > - writel(val, bridge_base_addr + (index * ATR_ENTRY_SIZE) + > - ATR0_AXI4_SLV0_SRCADDR_PARAM); > - > - val = upper_32_bits(axi_addr); > - writel(val, bridge_base_addr + (index * ATR_ENTRY_SIZE) + > - ATR0_AXI4_SLV0_SRC_ADDR); > - > - val = lower_32_bits(pci_addr); > - writel(val, bridge_base_addr + (index * ATR_ENTRY_SIZE) + > - ATR0_AXI4_SLV0_TRSL_ADDR_LSB); > - > - val = upper_32_bits(pci_addr); > - writel(val, bridge_base_addr + (index * ATR_ENTRY_SIZE) + > - ATR0_AXI4_SLV0_TRSL_ADDR_UDW); > - > - val = readl(bridge_base_addr + ATR0_PCIE_WIN0_SRCADDR_PARAM); > - val |= (ATR0_PCIE_ATR_SIZE << ATR0_PCIE_ATR_SIZE_SHIFT); > - writel(val, bridge_base_addr + ATR0_PCIE_WIN0_SRCADDR_PARAM); > - writel(0, bridge_base_addr + ATR0_PCIE_WIN0_SRC_ADDR); > -} > - > -static int plda_pcie_setup_iomems(struct pci_host_bridge *bridge, > - struct plda_pcie_rp *port) > -{ > - void __iomem *bridge_base_addr = port->bridge_addr; > - struct resource_entry *entry; > - u64 pci_addr; > - u32 index = 1; > - > - resource_list_for_each_entry(entry, &bridge->windows) { > - if (resource_type(entry->res) == IORESOURCE_MEM) { > - pci_addr = entry->res->start - entry->offset; > - plda_pcie_setup_window(bridge_base_addr, index, > - entry->res->start, pci_addr, > - resource_size(entry->res)); > - index++; > - } > - } > - > - return 0; > -} > - > static inline void mc_clear_secs(struct mc_pcie *port) > { > void __iomem *ctrl_base_addr = port->axi_base_addr + MC_PCIE_CTRL_ADDR; > diff --git a/drivers/pci/controller/plda/pcie-plda-host.c b/drivers/pci/controller/plda/pcie-plda-host.c > new file mode 100644 > index 000000000000..19131181897f > --- /dev/null > +++ b/drivers/pci/controller/plda/pcie-plda-host.c > @@ -0,0 +1,80 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * PLDA PCIe XpressRich host controller driver > + * > + * Copyright (C) 2023 Microchip Co. Ltd > + * > + * Author: Daire McNamara <daire.mcnamara@microchip.com> > + */ > + > +#include <linux/irqchip/chained_irq.h> > +#include <linux/irqdomain.h> > +#include <linux/msi.h> > +#include <linux/of_address.h> > +#include <linux/of_pci.h> > +#include <linux/pci_regs.h> > +#include <linux/pci-ecam.h> > +#include <linux/platform_device.h> Do you really require these headers ? Not in this patch, in later patches and that's why every header should be added when it is really needed. Lorenzo > + > +#include "pcie-plda.h" > + > +void plda_pcie_setup_window(void __iomem *bridge_base_addr, u32 index, > + phys_addr_t axi_addr, phys_addr_t pci_addr, > + size_t size) > +{ > + u32 atr_sz = ilog2(size) - 1; > + u32 val; > + > + if (index == 0) > + val = PCIE_CONFIG_INTERFACE; > + else > + val = PCIE_TX_RX_INTERFACE; > + > + writel(val, bridge_base_addr + (index * ATR_ENTRY_SIZE) + > + ATR0_AXI4_SLV0_TRSL_PARAM); > + > + val = lower_32_bits(axi_addr) | (atr_sz << ATR_SIZE_SHIFT) | > + ATR_IMPL_ENABLE; > + writel(val, bridge_base_addr + (index * ATR_ENTRY_SIZE) + > + ATR0_AXI4_SLV0_SRCADDR_PARAM); > + > + val = upper_32_bits(axi_addr); > + writel(val, bridge_base_addr + (index * ATR_ENTRY_SIZE) + > + ATR0_AXI4_SLV0_SRC_ADDR); > + > + val = lower_32_bits(pci_addr); > + writel(val, bridge_base_addr + (index * ATR_ENTRY_SIZE) + > + ATR0_AXI4_SLV0_TRSL_ADDR_LSB); > + > + val = upper_32_bits(pci_addr); > + writel(val, bridge_base_addr + (index * ATR_ENTRY_SIZE) + > + ATR0_AXI4_SLV0_TRSL_ADDR_UDW); > + > + val = readl(bridge_base_addr + ATR0_PCIE_WIN0_SRCADDR_PARAM); > + val |= (ATR0_PCIE_ATR_SIZE << ATR0_PCIE_ATR_SIZE_SHIFT); > + writel(val, bridge_base_addr + ATR0_PCIE_WIN0_SRCADDR_PARAM); > + writel(0, bridge_base_addr + ATR0_PCIE_WIN0_SRC_ADDR); > +} > +EXPORT_SYMBOL_GPL(plda_pcie_setup_window); > + > +int plda_pcie_setup_iomems(struct pci_host_bridge *bridge, > + struct plda_pcie_rp *port) > +{ > + void __iomem *bridge_base_addr = port->bridge_addr; > + struct resource_entry *entry; > + u64 pci_addr; > + u32 index = 1; > + > + resource_list_for_each_entry(entry, &bridge->windows) { > + if (resource_type(entry->res) == IORESOURCE_MEM) { > + pci_addr = entry->res->start - entry->offset; > + plda_pcie_setup_window(bridge_base_addr, index, > + entry->res->start, pci_addr, > + resource_size(entry->res)); > + index++; > + } > + } > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(plda_pcie_setup_iomems); > diff --git a/drivers/pci/controller/plda/pcie-plda.h b/drivers/pci/controller/plda/pcie-plda.h > index 363fcbbaf6ec..3deefd35fa5a 100644 > --- a/drivers/pci/controller/plda/pcie-plda.h > +++ b/drivers/pci/controller/plda/pcie-plda.h > @@ -120,4 +120,9 @@ struct plda_pcie_rp { > void __iomem *bridge_addr; > }; > > +void plda_pcie_setup_window(void __iomem *bridge_base_addr, u32 index, > + phys_addr_t axi_addr, phys_addr_t pci_addr, > + size_t size); > +int plda_pcie_setup_iomems(struct pci_host_bridge *bridge, > + struct plda_pcie_rp *port); > #endif > -- > 2.17.1 >
On Thu, Dec 14, 2023 at 03:28:29PM +0800, Minda Chen wrote: > The event num is different in other platform. For re-using interrupt > process codes, replace macros with variable. "The number of events is different across platforms. In order to share interrupt processing code, add a variable that defines the number of events so that it can be set per-platform instead of hardcoding it". Lorenzo > > Signed-off-by: Minda Chen <minda.chen@starfivetech.com> > Reviewed-by: Conor Dooley <conor.dooley@microchip.com> > --- > drivers/pci/controller/plda/pcie-microchip-host.c | 8 +++++--- > drivers/pci/controller/plda/pcie-plda.h | 1 + > 2 files changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/pci/controller/plda/pcie-microchip-host.c b/drivers/pci/controller/plda/pcie-microchip-host.c > index 506e6eeadc76..7b3f4f74745d 100644 > --- a/drivers/pci/controller/plda/pcie-microchip-host.c > +++ b/drivers/pci/controller/plda/pcie-microchip-host.c > @@ -654,7 +654,7 @@ static void plda_handle_event(struct irq_desc *desc) > > events = mc_get_events(port); > > - for_each_set_bit(bit, &events, NUM_EVENTS) > + for_each_set_bit(bit, &events, port->num_events) > generic_handle_domain_irq(port->event_domain, bit); > > chained_irq_exit(chip, desc); > @@ -817,7 +817,8 @@ static int plda_pcie_init_irq_domains(struct plda_pcie_rp *port) > return -EINVAL; > } > > - port->event_domain = irq_domain_add_linear(pcie_intc_node, NUM_EVENTS, > + port->event_domain = irq_domain_add_linear(pcie_intc_node, > + port->num_events, > &plda_event_domain_ops, > port); > if (!port->event_domain) { > @@ -921,7 +922,7 @@ static int plda_init_interrupts(struct platform_device *pdev, struct plda_pcie_r > if (irq < 0) > return -ENODEV; > > - for (i = 0; i < NUM_EVENTS; i++) { > + for (i = 0; i < port->num_events; i++) { > event_irq = irq_create_mapping(port->event_domain, i); > if (!event_irq) { > dev_err(dev, "failed to map hwirq %d\n", i); > @@ -1013,6 +1014,7 @@ static int mc_host_probe(struct platform_device *pdev) > > bridge_base_addr = port->axi_base_addr + MC_PCIE_BRIDGE_ADDR; > plda->bridge_addr = bridge_base_addr; > + plda->num_events = NUM_EVENTS; > > /* Allow enabling MSI by disabling MSI-X */ > val = readl(bridge_base_addr + PCIE_PCI_IRQ_DW0); > diff --git a/drivers/pci/controller/plda/pcie-plda.h b/drivers/pci/controller/plda/pcie-plda.h > index 3deefd35fa5a..e3d35cef9894 100644 > --- a/drivers/pci/controller/plda/pcie-plda.h > +++ b/drivers/pci/controller/plda/pcie-plda.h > @@ -118,6 +118,7 @@ struct plda_pcie_rp { > raw_spinlock_t lock; > struct plda_msi msi; > void __iomem *bridge_addr; > + int num_events; > }; > > void plda_pcie_setup_window(void __iomem *bridge_base_addr, u32 index, > -- > 2.17.1 >
On 2023/12/27 23:49, Lorenzo Pieralisi wrote: > On Thu, Dec 14, 2023 at 03:28:27PM +0800, Minda Chen wrote: >> Move setup functions to common pcie-plda-host.c. So these two functions >> can be re-used. >> >> Signed-off-by: Minda Chen <minda.chen@starfivetech.com> >> Reviewed-by: Conor Dooley <conor.dooley@microchip.com> >> --- >> drivers/pci/controller/plda/Kconfig | 4 + >> drivers/pci/controller/plda/Makefile | 1 + >> .../pci/controller/plda/pcie-microchip-host.c | 59 -------------- >> drivers/pci/controller/plda/pcie-plda-host.c | 80 +++++++++++++++++++ >> drivers/pci/controller/plda/pcie-plda.h | 5 ++ >> 5 files changed, 90 insertions(+), 59 deletions(-) >> create mode 100644 drivers/pci/controller/plda/pcie-plda-host.c >> >> diff --git a/drivers/pci/controller/plda/Kconfig b/drivers/pci/controller/plda/Kconfig >> index 5cb3be4fc98c..e54a82ee94f5 100644 >> --- a/drivers/pci/controller/plda/Kconfig >> +++ b/drivers/pci/controller/plda/Kconfig >> @@ -3,10 +3,14 @@ >> menu "PLDA-based PCIe controllers" >> depends on PCI >> >> +config PCIE_PLDA_HOST >> + bool >> + >> config PCIE_MICROCHIP_HOST >> tristate "Microchip AXI PCIe controller" >> depends on PCI_MSI && OF >> select PCI_HOST_COMMON >> + select PCIE_PLDA_HOST >> help >> Say Y here if you want kernel to support the Microchip AXI PCIe >> Host Bridge driver. >> diff --git a/drivers/pci/controller/plda/Makefile b/drivers/pci/controller/plda/Makefile >> index e1a265cbf91c..4340ab007f44 100644 >> --- a/drivers/pci/controller/plda/Makefile >> +++ b/drivers/pci/controller/plda/Makefile >> @@ -1,2 +1,3 @@ >> # SPDX-License-Identifier: GPL-2.0 >> +obj-$(CONFIG_PCIE_PLDA_HOST) += pcie-plda-host.o >> obj-$(CONFIG_PCIE_MICROCHIP_HOST) += pcie-microchip-host.o >> diff --git a/drivers/pci/controller/plda/pcie-microchip-host.c b/drivers/pci/controller/plda/pcie-microchip-host.c >> index 31ca8d44ee2a..2e79bcc7c0a5 100644 >> --- a/drivers/pci/controller/plda/pcie-microchip-host.c >> +++ b/drivers/pci/controller/plda/pcie-microchip-host.c >> @@ -838,65 +838,6 @@ static int mc_pcie_init_irq_domains(struct plda_pcie_rp *port) >> return mc_allocate_msi_domains(port); >> } >> >> -static void plda_pcie_setup_window(void __iomem *bridge_base_addr, u32 index, >> - phys_addr_t axi_addr, phys_addr_t pci_addr, >> - size_t size) >> -{ >> - u32 atr_sz = ilog2(size) - 1; >> - u32 val; >> - >> - if (index == 0) >> - val = PCIE_CONFIG_INTERFACE; >> - else >> - val = PCIE_TX_RX_INTERFACE; >> - >> - writel(val, bridge_base_addr + (index * ATR_ENTRY_SIZE) + >> - ATR0_AXI4_SLV0_TRSL_PARAM); >> - >> - val = lower_32_bits(axi_addr) | (atr_sz << ATR_SIZE_SHIFT) | >> - ATR_IMPL_ENABLE; >> - writel(val, bridge_base_addr + (index * ATR_ENTRY_SIZE) + >> - ATR0_AXI4_SLV0_SRCADDR_PARAM); >> - >> - val = upper_32_bits(axi_addr); >> - writel(val, bridge_base_addr + (index * ATR_ENTRY_SIZE) + >> - ATR0_AXI4_SLV0_SRC_ADDR); >> - >> - val = lower_32_bits(pci_addr); >> - writel(val, bridge_base_addr + (index * ATR_ENTRY_SIZE) + >> - ATR0_AXI4_SLV0_TRSL_ADDR_LSB); >> - >> - val = upper_32_bits(pci_addr); >> - writel(val, bridge_base_addr + (index * ATR_ENTRY_SIZE) + >> - ATR0_AXI4_SLV0_TRSL_ADDR_UDW); >> - >> - val = readl(bridge_base_addr + ATR0_PCIE_WIN0_SRCADDR_PARAM); >> - val |= (ATR0_PCIE_ATR_SIZE << ATR0_PCIE_ATR_SIZE_SHIFT); >> - writel(val, bridge_base_addr + ATR0_PCIE_WIN0_SRCADDR_PARAM); >> - writel(0, bridge_base_addr + ATR0_PCIE_WIN0_SRC_ADDR); >> -} >> - >> -static int plda_pcie_setup_iomems(struct pci_host_bridge *bridge, >> - struct plda_pcie_rp *port) >> -{ >> - void __iomem *bridge_base_addr = port->bridge_addr; >> - struct resource_entry *entry; >> - u64 pci_addr; >> - u32 index = 1; >> - >> - resource_list_for_each_entry(entry, &bridge->windows) { >> - if (resource_type(entry->res) == IORESOURCE_MEM) { >> - pci_addr = entry->res->start - entry->offset; >> - plda_pcie_setup_window(bridge_base_addr, index, >> - entry->res->start, pci_addr, >> - resource_size(entry->res)); >> - index++; >> - } >> - } >> - >> - return 0; >> -} >> - >> static inline void mc_clear_secs(struct mc_pcie *port) >> { >> void __iomem *ctrl_base_addr = port->axi_base_addr + MC_PCIE_CTRL_ADDR; >> diff --git a/drivers/pci/controller/plda/pcie-plda-host.c b/drivers/pci/controller/plda/pcie-plda-host.c >> new file mode 100644 >> index 000000000000..19131181897f >> --- /dev/null >> +++ b/drivers/pci/controller/plda/pcie-plda-host.c >> @@ -0,0 +1,80 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * PLDA PCIe XpressRich host controller driver >> + * >> + * Copyright (C) 2023 Microchip Co. Ltd >> + * >> + * Author: Daire McNamara <daire.mcnamara@microchip.com> >> + */ >> + >> +#include <linux/irqchip/chained_irq.h> >> +#include <linux/irqdomain.h> >> +#include <linux/msi.h> >> +#include <linux/of_address.h> >> +#include <linux/of_pci.h> >> +#include <linux/pci_regs.h> >> +#include <linux/pci-ecam.h> >> +#include <linux/platform_device.h> > > Do you really require these headers ? Not in this patch, > in later patches and that's why every header should be > added when it is really needed. > > Lorenzo > OK, I will change this. thanks. >> + >> +#include "pcie-plda.h" >> + >> +void plda_pcie_setup_window(void __iomem *bridge_base_addr, u32 index, >> + phys_addr_t axi_addr, phys_addr_t pci_addr, >> + size_t size) >> +{ >> + u32 atr_sz = ilog2(size) - 1; >> + u32 val; >> + >> + if (index == 0) >> + val = PCIE_CONFIG_INTERFACE; >> + else >> + val = PCIE_TX_RX_INTERFACE; >> + >> + writel(val, bridge_base_addr + (index * ATR_ENTRY_SIZE) + >> + ATR0_AXI4_SLV0_TRSL_PARAM); >> + >> + val = lower_32_bits(axi_addr) | (atr_sz << ATR_SIZE_SHIFT) | >> + ATR_IMPL_ENABLE; >> + writel(val, bridge_base_addr + (index * ATR_ENTRY_SIZE) + >> + ATR0_AXI4_SLV0_SRCADDR_PARAM); >> + >> + val = upper_32_bits(axi_addr); >> + writel(val, bridge_base_addr + (index * ATR_ENTRY_SIZE) + >> + ATR0_AXI4_SLV0_SRC_ADDR); >> + >> + val = lower_32_bits(pci_addr); >> + writel(val, bridge_base_addr + (index * ATR_ENTRY_SIZE) + >> + ATR0_AXI4_SLV0_TRSL_ADDR_LSB); >> + >> + val = upper_32_bits(pci_addr); >> + writel(val, bridge_base_addr + (index * ATR_ENTRY_SIZE) + >> + ATR0_AXI4_SLV0_TRSL_ADDR_UDW); >> + >> + val = readl(bridge_base_addr + ATR0_PCIE_WIN0_SRCADDR_PARAM); >> + val |= (ATR0_PCIE_ATR_SIZE << ATR0_PCIE_ATR_SIZE_SHIFT); >> + writel(val, bridge_base_addr + ATR0_PCIE_WIN0_SRCADDR_PARAM); >> + writel(0, bridge_base_addr + ATR0_PCIE_WIN0_SRC_ADDR); >> +} >> +EXPORT_SYMBOL_GPL(plda_pcie_setup_window); >> + >> +int plda_pcie_setup_iomems(struct pci_host_bridge *bridge, >> + struct plda_pcie_rp *port) >> +{ >> + void __iomem *bridge_base_addr = port->bridge_addr; >> + struct resource_entry *entry; >> + u64 pci_addr; >> + u32 index = 1; >> + >> + resource_list_for_each_entry(entry, &bridge->windows) { >> + if (resource_type(entry->res) == IORESOURCE_MEM) { >> + pci_addr = entry->res->start - entry->offset; >> + plda_pcie_setup_window(bridge_base_addr, index, >> + entry->res->start, pci_addr, >> + resource_size(entry->res)); >> + index++; >> + } >> + } >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(plda_pcie_setup_iomems); >> diff --git a/drivers/pci/controller/plda/pcie-plda.h b/drivers/pci/controller/plda/pcie-plda.h >> index 363fcbbaf6ec..3deefd35fa5a 100644 >> --- a/drivers/pci/controller/plda/pcie-plda.h >> +++ b/drivers/pci/controller/plda/pcie-plda.h >> @@ -120,4 +120,9 @@ struct plda_pcie_rp { >> void __iomem *bridge_addr; >> }; >> >> +void plda_pcie_setup_window(void __iomem *bridge_base_addr, u32 index, >> + phys_addr_t axi_addr, phys_addr_t pci_addr, >> + size_t size); >> +int plda_pcie_setup_iomems(struct pci_host_bridge *bridge, >> + struct plda_pcie_rp *port); >> #endif >> -- >> 2.17.1 >>
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; >> >> > };
On 2023/12/27 23:55, Lorenzo Pieralisi wrote: > On Thu, Dec 14, 2023 at 03:28:29PM +0800, Minda Chen wrote: >> The event num is different in other platform. For re-using interrupt >> process codes, replace macros with variable. > > "The number of events is different across platforms. In order to > share interrupt processing code, add a variable that defines the > number of events so that it can be set per-platform instead of > hardcoding it". > > Lorenzo > Thanks for reviewing. Happy new year! All the maintainers: Happy new year! >> >> Signed-off-by: Minda Chen <minda.chen@starfivetech.com> >> Reviewed-by: Conor Dooley <conor.dooley@microchip.com> >> --- >> drivers/pci/controller/plda/pcie-microchip-host.c | 8 +++++--- >> drivers/pci/controller/plda/pcie-plda.h | 1 + >> 2 files changed, 6 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/pci/controller/plda/pcie-microchip-host.c b/drivers/pci/controller/plda/pcie-microchip-host.c >> index 506e6eeadc76..7b3f4f74745d 100644 >> --- a/drivers/pci/controller/plda/pcie-microchip-host.c >> +++ b/drivers/pci/controller/plda/pcie-microchip-host.c >> @@ -654,7 +654,7 @@ static void plda_handle_event(struct irq_desc *desc) >> >> events = mc_get_events(port); >> >> - for_each_set_bit(bit, &events, NUM_EVENTS) >> + for_each_set_bit(bit, &events, port->num_events) >> generic_handle_domain_irq(port->event_domain, bit); >> >> chained_irq_exit(chip, desc); >> @@ -817,7 +817,8 @@ static int plda_pcie_init_irq_domains(struct plda_pcie_rp *port) >> return -EINVAL; >> } >> >> - port->event_domain = irq_domain_add_linear(pcie_intc_node, NUM_EVENTS, >> + port->event_domain = irq_domain_add_linear(pcie_intc_node, >> + port->num_events, >> &plda_event_domain_ops, >> port); >> if (!port->event_domain) { >> @@ -921,7 +922,7 @@ static int plda_init_interrupts(struct platform_device *pdev, struct plda_pcie_r >> if (irq < 0) >> return -ENODEV; >> >> - for (i = 0; i < NUM_EVENTS; i++) { >> + for (i = 0; i < port->num_events; i++) { >> event_irq = irq_create_mapping(port->event_domain, i); >> if (!event_irq) { >> dev_err(dev, "failed to map hwirq %d\n", i); >> @@ -1013,6 +1014,7 @@ static int mc_host_probe(struct platform_device *pdev) >> >> bridge_base_addr = port->axi_base_addr + MC_PCIE_BRIDGE_ADDR; >> plda->bridge_addr = bridge_base_addr; >> + plda->num_events = NUM_EVENTS; >> >> /* Allow enabling MSI by disabling MSI-X */ >> val = readl(bridge_base_addr + PCIE_PCI_IRQ_DW0); >> diff --git a/drivers/pci/controller/plda/pcie-plda.h b/drivers/pci/controller/plda/pcie-plda.h >> index 3deefd35fa5a..e3d35cef9894 100644 >> --- a/drivers/pci/controller/plda/pcie-plda.h >> +++ b/drivers/pci/controller/plda/pcie-plda.h >> @@ -118,6 +118,7 @@ struct plda_pcie_rp { >> raw_spinlock_t lock; >> struct plda_msi msi; >> void __iomem *bridge_addr; >> + int num_events; >> }; >> >> void plda_pcie_setup_window(void __iomem *bridge_base_addr, u32 index, >> -- >> 2.17.1 >>
> Minda Chen <minda.chen@starfivetech.com> writes: > > > This patchset final purpose is add PCIe driver for StarFive JH7110 SoC. > > JH7110 using PLDA XpressRICH PCIe IP. Microchip PolarFire Using the > > same IP and have commit their codes, which are mixed with PLDA > > controller codes and Microchip platform codes. > > Thank you for this series. > > I tested this on a VisionFive v2 board, and it seems to probe and find my > M.2 NVMe SSD, but then gets timeouts when trying to use the NVMe (e.g. > 'blkid' command) > Hi, Kevin: Could you please provide the manufacturer and model of the M.2 NVMe SSD you tested? > Kernel logs below. > > Kevin > > [ 15.131094] pcie-starfive 9c0000000.pcie: host bridge > /soc/pcie@9c0000000 ranges: > [ 15.138637] pcie-starfive 9c0000000.pcie: MEM > 0x0038000000..0x003fffffff -> 0x0038000000 > [ 15.147180] pcie-starfive 9c0000000.pcie: MEM > 0x0980000000..0x09bfffffff -> 0x0980000000 > [ 15.368040] pcie-starfive 9c0000000.pcie: port link up > [ 15.374219] pcie-starfive 9c0000000.pcie: PCI host bridge to bus 0001:00 > [ 15.380944] pci_bus 0001:00: root bus resource [bus 00-ff] > [ 15.386443] pci_bus 0001:00: root bus resource [mem > 0x38000000-0x3fffffff] > [ 15.393330] pci_bus 0001:00: root bus resource [mem > 0x980000000-0x9bfffffff pref] > [ 15.400882] pci 0001:00:00.0: [1556:1111] type 01 class 0x060400 > [ 15.407165] pci 0001:00:00.0: supports D1 D2 > [ 15.411447] pci 0001:00:00.0: PME# supported from D0 D1 D2 D3hot > D3cold > [ 15.419964] pci 0001:00:00.0: bridge configuration invalid ([bus 00-00]), > reconfiguring > [ 15.428245] pci 0001:01:00.0: [126f:2263] type 00 class 0x010802 > [ 15.434331] pci 0001:01:00.0: reg 0x10: [mem 0x00000000-0x00003fff > 64bit] > [ 15.441578] pci 0001:01:00.0: 4.000 Gb/s available PCIe bandwidth, > limited by 5.0 GT/s PCIe x1 link at 0001:00:00.0 (capable of 31.504 Gb/s with > 8 > .0 GT/s PCIe x4 link) > [ 15.456910] pci_bus 0001:01: busn_res: [bus 01-ff] end is updated to 01 > [ 15.463553] pci 0001:00:00.0: BAR 8: assigned [mem > 0x38000000-0x380fffff] > [ 15.470352] pci 0001:01:00.0: BAR 0: assigned [mem > 0x38000000-0x38003fff 64bit] > [ 15.477699] pci 0001:00:00.0: PCI bridge to [bus 01] > [ 15.482686] pci 0001:00:00.0: bridge window [mem > 0x38000000-0x380fffff] > [ 15.489632] pcieport 0001:00:00.0: enabling device (0000 -> 0002) > [ 15.496038] pcieport 0001:00:00.0: PME: Signaling with IRQ 56 > [ 15.502472] usb 1-1: new high-speed USB device number 2 using xhci_hcd > [ 15.509755] usb usb2-port2: over-current condition > [ 15.515883] nvme nvme0: pci function 0001:01:00.0 > [ 15.520615] nvme 0001:01:00.0: enabling device (0000 -> 0002) > [ 15.532685] nvme nvme0: allocated 64 MiB host memory buffer. > [ 15.550070] nvme nvme0: 4/0/0 default/read/poll queues > [ 15.562992] nvme nvme0: Ignoring bogus Namespace Identifiers > [ 15.663327] hub 1-1:1.0: USB hub found > [ 15.667320] hub 1-1:1.0: 4 ports detected > > [ 46.064052] nvme nvme0: I/O 424 QID 3 timeout, completion polled > > [ 76.784046] nvme nvme0: I/O 425 (I/O Cmd) QID 3 timeout, aborting > [ 76.790179] nvme nvme0: I/O 426 (I/O Cmd) QID 3 timeout, aborting > [ 76.796294] nvme nvme0: I/O 427 (I/O Cmd) QID 3 timeout, aborting > [ 76.802411] nvme nvme0: I/O 428 (I/O Cmd) QID 3 timeout, aborting > [ 76.808525] nvme nvme0: I/O 429 (I/O Cmd) QID 3 timeout, aborting
Kevin Xie <kevin.xie@starfivetech.com> writes: >> Minda Chen <minda.chen@starfivetech.com> writes: >> >> > This patchset final purpose is add PCIe driver for StarFive JH7110 SoC. >> > JH7110 using PLDA XpressRICH PCIe IP. Microchip PolarFire Using the >> > same IP and have commit their codes, which are mixed with PLDA >> > controller codes and Microchip platform codes. >> >> Thank you for this series. >> >> I tested this on a VisionFive v2 board, and it seems to probe and find my >> M.2 NVMe SSD, but then gets timeouts when trying to use the NVMe (e.g. >> 'blkid' command) >> > > Hi, Kevin: > Could you please provide the manufacturer and model of the M.2 NVMe SSD > you tested? I have a 256 Gb Silicon Power P34A60 M.2 NVMe SSD (part number: sp256gbp34a60m28) Also for reference, I tested the same SSD on another arm platform (Khadas VIM3) and it works fine. Kevin