mbox series

[v13,0/21] Refactoring Microchip PCIe driver and add StarFive PCIe

Message ID 20231214072839.2367-1-minda.chen@starfivetech.com
Headers show
Series Refactoring Microchip PCIe driver and add StarFive PCIe | expand

Message

Minda Chen Dec. 14, 2023, 7:28 a.m. UTC
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.

For re-use the PLDA controller codes, I request refactoring microchip
codes, move PLDA common codes to PLDA files.
Desigware and Cadence is good example for refactoring codes.

----------------------------------------------------------
The refactoring patches total number is 16,(patch 1-16)
which do NOT contain changing logic of codes.

These patches just contain three type basic operations.
(rename, modify codes to support starfive platform, and moving to common file)
If these patched are all be reviewed. They can be accepted first.

Refactoring patches can be devided to different groups
1. (patch 1- 3 is the prepare work of refactoring)
patch1 is move PLDA XpressRICH PCIe host common properties dt-binding
       docs from microchip,pcie-host.yaml
patch2 is move PolarFire codes to PLDA directory.
patch3 is move PLDA IP register macros to plda-pcie.h

2. (patch4 - 6 is processing and re-use PCIe host instance)
patch4 is add bridge_addr field to PCIe host instance.
patch5 is rename data structure in microchip codes.
patch6 is moving two data structures to head file

3. (patch 7 - 9 are for re-use two PCIe setup function)
patch7 is rename two setup functions in microchip codes, prepare to move
to common file.
patch8 is change the arguments of plda_pcie_setup_iomems()
patch9 is move the two setup functions to common file pcie-plda-host.c

4.(patch 10 - 16 are for re-use interupt processing codes)
patch10 is rename the IRQ related functions, prepare to move to
pcie-plda-host.c
patch 11 - 15 is modify the interrupt event codes, preparing for support starfive
and microchip two platforms.
patch16 is move IRQ related functions to pcie-plda-host.c

------------------------------------------------------------
The remainder patches (patch 17 -21) are not refactoring patch.
They are for adding StarFive codes and dont modify the microchip's
codes.

patch17 is Add host init/deinit functions.
patch18 is add StarFive JH7110 PCIe dt-binding doc.
patch19 is Add a PCIe delay time macro
patch20 is add StarFive JH7110 Soc PCIe codes.
patch21 is Starfive dts config

This patchset is base on v6.7-rc4

previous version:
v6:https://patchwork.kernel.org/project/linux-pci/cover/20230915102243.59775-1-minda.chen@starfivetech.com/
v7:https://patchwork.kernel.org/project/linux-pci/cover/20230927100802.46620-1-minda.chen@starfivetech.com/
v8:https://patchwork.kernel.org/project/linux-pci/cover/20231011110514.107528-1-minda.chen@starfivetech.com/
v9:https://patchwork.kernel.org/project/linux-pci/cover/20231020104341.63157-1-minda.chen@starfivetech.com/
v10:https://patchwork.kernel.org/project/linux-pci/cover/20231031115430.113586-1-minda.chen@starfivetech.com/
v11:https://patchwork.kernel.org/project/linux-pci/cover/20231115114912.71448-1-minda.chen@starfivetech.com/
v12:https://patchwork.kernel.org/project/linux-pci/cover/20231206105839.25805-1-minda.chen@starfivetech.com/

change:
  v13:
   patch14: 1. Add plda_get_event() function. This can be compare with mc_get_event() easily and 
		track the codes changes in case in the future..
            2. The host event_ops is directly set to plda host port.
	    3. Setting default host event_ops instead of checking event ops.
   patch15:1. Add PLDA event irq_chip instead of event_domain_ops, The
		event_domain_ops can be re-used.
            2. The host event irq_chip is directly set to plda host port.
	    3. Add PLDA event irqchip ops codes.
            4. Remove Conor's review tag due to whole patch have been changed.
   patch16: Also move the new added PLDA event codes.
   patch17: Add plda host init and deinit function only.

  v12:
   patch17: modify the commit message and add starfive review tag.
   Add PCIE_RESET_CONFIG_DEVICE_WAIT_MS to patch 19.
   patch20: Add disable runtime pm function in starfive_pcie_remove()
            Add "depens on ARCH_STARFIVE || COMPILE_TEST" in Starfive PCie Kconfig

  v11:
     check and modify some commit messages again.
     All the codes are the same with v10.   

  v10:
   All the commit message set to fit in 75 columns.
   All the codes fit in less than 80 colunms.
   patch 14: 
	Commit message changes suggested by Conor.
   patch 20:
        Add 100 ms delay macro to pci.h
	generic phy pointer related codes moving to pcie-starfive.c
	This patch Change pcie-starfive only, bus_ops move to patch 16.
	Some Codes changes suggested by Bjorn.

  v9:
   v8 patch 10 squash to v9 patch 12, v8 patch 18 squash to v9 patch 16.
   patch 4 - 16: Add new review tags and add more accurate commit messages.
   patch 17: move the plda_pcie_host_init/deinit from patch 19. Make
             plda driver become to whole driver.

  v8:
    The patch description in cover-letter has been changed.

    v7 patch 4 split to v8 patch 4 - 6.
        (It is patches about re-use pcie host data structure, new patches just contain one
	function modification. It is more reguluar and easier to review).

    patch 7- 9: modify the commit messages and add reason of
		modifcation.
    patch10- 16 :
             Add review tag and add more commit messages to declear the
	     reason of modifying the codes.
    patch17: plda_handle_events() using bit mask macro. The function are
	     easier to read.

  v7:
    patch17: fix the build warning.
    patch20: Some format changes (Emil's comment)
    patch21: change the pcie node sequences by alphabetical
             delete the "interupt-parent" in pcie node.

  v6:
    v5 patch 4 split to patch 4 -9. New patches just contain one function modification. It is more reguluar.

    patch 9: Just move the two setup functions only
    patch 19 : draw a graph of PLDA local register, make it easier to
               review the codes.
    v5 patch 7 split to patch 10- 16. Each patch just contain one
                function modification. It is more regular.
    patch 10: rename IRQ related functions.
    patch 11 - 15 : modify the events codes, total five patch.
    patch 16: move IRQ related functions to pcie-plda-host.c
    patch 20- 21 using "linux,pci-domain" dts setting.

Kevin Xie (1):
  PCI: Add PCIE_RESET_CONFIG_DEVICE_WAIT_MS waiting time value

Minda Chen (20):
  dt-bindings: PCI: Add PLDA XpressRICH PCIe host common properties
  PCI: microchip: Move pcie-microchip-host.c to plda directory
  PCI: microchip: Move PLDA IP register macros to pcie-plda.h
  PCI: microchip: Add bridge_addr field to struct mc_pcie
  PCI: microchip: Rename two PCIe data structures
  PCI: microchip: Move PCIe host data structures to plda-pcie.h
  PCI: microchip: Rename two setup functions
  PCI: microchip: Change the argument of plda_pcie_setup_iomems()
  PCI: microchip: Move setup functions to pcie-plda-host.c
  PCI: microchip: Rename interrupt related functions
  PCI: microchip: Add num_events field to struct plda_pcie_rp
  PCI: microchip: Add request_event_irq() callback function
  PCI: microchip: Add INTx and MSI event num to struct plda_event
  PCI: microchip: Add get_events() callback and add PLDA get_event()
  PCI: microchip: Add event irqchip field to host port and add PLDA
    irqchip
  PCI: microchip: Move IRQ functions to pcie-plda-host.c
  PCI: plda: Add host init/deinit and map bus functions
  dt-bindings: PCI: Add StarFive JH7110 PCIe controller
  PCI: starfive: Add JH7110 PCIe controller
  riscv: dts: starfive: add PCIe dts configuration for JH7110

 .../bindings/pci/microchip,pcie-host.yaml     |  55 +-
 .../pci/plda,xpressrich3-axi-common.yaml      |  75 ++
 .../bindings/pci/starfive,jh7110-pcie.yaml    | 120 ++++
 MAINTAINERS                                   |  19 +-
 .../jh7110-starfive-visionfive-2.dtsi         |  64 ++
 arch/riscv/boot/dts/starfive/jh7110.dtsi      |  86 +++
 drivers/pci/controller/Kconfig                |   9 +-
 drivers/pci/controller/Makefile               |   2 +-
 drivers/pci/controller/plda/Kconfig           |  30 +
 drivers/pci/controller/plda/Makefile          |   4 +
 .../{ => plda}/pcie-microchip-host.c          | 614 ++--------------
 drivers/pci/controller/plda/pcie-plda-host.c  | 656 ++++++++++++++++++
 drivers/pci/controller/plda/pcie-plda.h       | 265 +++++++
 drivers/pci/controller/plda/pcie-starfive.c   | 462 ++++++++++++
 drivers/pci/pci.h                             |  16 +
 15 files changed, 1861 insertions(+), 616 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pci/plda,xpressrich3-axi-common.yaml
 create mode 100644 Documentation/devicetree/bindings/pci/starfive,jh7110-pcie.yaml
 create mode 100644 drivers/pci/controller/plda/Kconfig
 create mode 100644 drivers/pci/controller/plda/Makefile
 rename drivers/pci/controller/{ => plda}/pcie-microchip-host.c (54%)
 create mode 100644 drivers/pci/controller/plda/pcie-plda-host.c
 create mode 100644 drivers/pci/controller/plda/pcie-plda.h
 create mode 100644 drivers/pci/controller/plda/pcie-starfive.c


base-commit: 33cc938e65a98f1d29d0a18403dbbee050dcad9a

Comments

Lorenzo Pieralisi Dec. 21, 2023, 3:32 p.m. UTC | #1
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;
> >  };
Minda Chen Dec. 22, 2023, 11:18 a.m. UTC | #2
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;
>> >  };
Lorenzo Pieralisi Dec. 27, 2023, 12:43 p.m. UTC | #3
[+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;
> >> >  };
Lorenzo Pieralisi Dec. 27, 2023, 3:49 p.m. UTC | #4
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
>
Lorenzo Pieralisi Dec. 27, 2023, 3:55 p.m. UTC | #5
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
>
Minda Chen Dec. 28, 2023, 9:46 a.m. UTC | #6
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
>>
Minda Chen Dec. 28, 2023, 11:25 a.m. UTC | #7
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;
>> >> >  };
Minda Chen Dec. 29, 2023, 3:46 a.m. UTC | #8
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
>>
Kevin Xie Jan. 5, 2024, 2:35 a.m. UTC | #9
> 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 Hilman Jan. 5, 2024, 5:28 p.m. UTC | #10
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