diff mbox series

[v12,15/21] PCI: microchip: Add event IRQ domain ops to struct plda_event

Message ID 20231206105839.25805-16-minda.chen@starfivetech.com (mailing list archive)
State Superseded
Headers show
Series Refactoring Microchip PCIe driver and add StarFive PCIe | expand

Commit Message

Minda Chen Dec. 6, 2023, 10:58 a.m. UTC 
For lack of an MSI controller, The new added PCIe interrupts have to be
added to global interrupt event field. PolarFire event domain ops can not
be re-used.

PLDA event domain ops instances will be implemented in later patch.

Signed-off-by: Minda Chen <minda.chen@starfivetech.com>
Acked-by: Conor Dooley <conor.dooley@microchip.com>
---
 drivers/pci/controller/plda/pcie-microchip-host.c | 6 ++++--
 drivers/pci/controller/plda/pcie-plda.h           | 1 +
 2 files changed, 5 insertions(+), 2 deletions(-)

Comments

Lorenzo Pieralisi Dec. 12, 2023, 11:19 a.m. UTC | #1 
On Wed, Dec 06, 2023 at 06:58:33PM +0800, Minda Chen wrote:
> For lack of an MSI controller, The new added PCIe interrupts have to be
> added to global interrupt event field. PolarFire event domain ops can not
> be re-used.

I don't understand what this means, please explain and I will
add it to the commit log.

> PLDA event domain ops instances will be implemented in later patch.

Future patches don't exist, each commit log is a logical change
that must make sense on its own, I will remove this sentence.

Lorenzo

> Signed-off-by: Minda Chen <minda.chen@starfivetech.com>
> Acked-by: Conor Dooley <conor.dooley@microchip.com>
> ---
>  drivers/pci/controller/plda/pcie-microchip-host.c | 6 ++++--
>  drivers/pci/controller/plda/pcie-plda.h           | 1 +
>  2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/plda/pcie-microchip-host.c b/drivers/pci/controller/plda/pcie-microchip-host.c
> index f5e7da242aec..e6dcc572b65b 100644
> --- a/drivers/pci/controller/plda/pcie-microchip-host.c
> +++ b/drivers/pci/controller/plda/pcie-microchip-host.c
> @@ -821,13 +821,15 @@ static const struct plda_event_ops mc_event_ops = {
>  };
>  
>  static const struct plda_event mc_event = {
> +	.domain_ops        = &mc_event_domain_ops,
>  	.event_ops         = &mc_event_ops,
>  	.request_event_irq = mc_request_event_irq,
>  	.intx_event        = EVENT_LOCAL_PM_MSI_INT_INTX,
>  	.msi_event         = EVENT_LOCAL_PM_MSI_INT_MSI,
>  };
>  
> -static int plda_pcie_init_irq_domains(struct plda_pcie_rp *port)
> +static int plda_pcie_init_irq_domains(struct plda_pcie_rp *port,
> +				      const struct irq_domain_ops *ops)
>  {
>  	struct device *dev = port->dev;
>  	struct device_node *node = dev->of_node;
> @@ -941,7 +943,7 @@ static int plda_init_interrupts(struct platform_device *pdev,
>  		return -EINVAL;
>  	}
>  
> -	ret = plda_pcie_init_irq_domains(port);
> +	ret = plda_pcie_init_irq_domains(port, event->domain_ops);
>  	if (ret) {
>  		dev_err(dev, "failed creating IRQ domains\n");
>  		return ret;
> diff --git a/drivers/pci/controller/plda/pcie-plda.h b/drivers/pci/controller/plda/pcie-plda.h
> index df1729095952..820ea16855b5 100644
> --- a/drivers/pci/controller/plda/pcie-plda.h
> +++ b/drivers/pci/controller/plda/pcie-plda.h
> @@ -129,6 +129,7 @@ struct plda_pcie_rp {
>  };
>  
>  struct plda_event {
> +	const struct irq_domain_ops *domain_ops;
>  	const struct plda_event_ops *event_ops;
>  	int (*request_event_irq)(struct plda_pcie_rp *pcie,
>  				 int event_irq, int event);
> -- 
> 2.17.1
>
Minda Chen Dec. 13, 2023, 8:15 a.m. UTC | #2 
On 2023/12/12 19:19, Lorenzo Pieralisi wrote:
> On Wed, Dec 06, 2023 at 06:58:33PM +0800, Minda Chen wrote:
>> For lack of an MSI controller, The new added PCIe interrupts have to be
>> added to global interrupt event field. PolarFire event domain ops can not
>> be re-used.
> 
> I don't understand what this means, please explain and I will
> add it to the commit log.
> 
Sorry.
Microchip Polarfire PCIe adds 11 PCIe interrupts to PCIe global event domain.(Total 28 PCIe interrupts)
The microchip event domain and event irqchip will handle these interrupts.
But PLDA host contain 13 fixed PCIe interrupts. PLDA codes just process these
13 interrupts. Microchip the event irq codes are quite different and can't be used by PLDA codes.
So add an event domain field support microchip and other vendor who just using the PLDA interrupts.
>> PLDA event domain ops instances will be implemented in later patch.
> 
> Future patches don't exist, each commit log is a logical change
> that must make sense on its own, I will remove this sentence.
> 
> Lorenzo

OK, Thanks.

>> Signed-off-by: Minda Chen <minda.chen@starfivetech.com>
>> Acked-by: Conor Dooley <conor.dooley@microchip.com>
>> ---
>>  drivers/pci/controller/plda/pcie-microchip-host.c | 6 ++++--
>>  drivers/pci/controller/plda/pcie-plda.h           | 1 +
>>  2 files changed, 5 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/pci/controller/plda/pcie-microchip-host.c b/drivers/pci/controller/plda/pcie-microchip-host.c
>> index f5e7da242aec..e6dcc572b65b 100644
>> --- a/drivers/pci/controller/plda/pcie-microchip-host.c
>> +++ b/drivers/pci/controller/plda/pcie-microchip-host.c
>> @@ -821,13 +821,15 @@ static const struct plda_event_ops mc_event_ops = {
>>  };
>>  
>>  static const struct plda_event mc_event = {
>> +	.domain_ops        = &mc_event_domain_ops,
>>  	.event_ops         = &mc_event_ops,
>>  	.request_event_irq = mc_request_event_irq,
>>  	.intx_event        = EVENT_LOCAL_PM_MSI_INT_INTX,
>>  	.msi_event         = EVENT_LOCAL_PM_MSI_INT_MSI,
>>  };
>>  
>> -static int plda_pcie_init_irq_domains(struct plda_pcie_rp *port)
>> +static int plda_pcie_init_irq_domains(struct plda_pcie_rp *port,
>> +				      const struct irq_domain_ops *ops)
>>  {
>>  	struct device *dev = port->dev;
>>  	struct device_node *node = dev->of_node;
>> @@ -941,7 +943,7 @@ static int plda_init_interrupts(struct platform_device *pdev,
>>  		return -EINVAL;
>>  	}
>>  
>> -	ret = plda_pcie_init_irq_domains(port);
>> +	ret = plda_pcie_init_irq_domains(port, event->domain_ops);
>>  	if (ret) {
>>  		dev_err(dev, "failed creating IRQ domains\n");
>>  		return ret;
>> diff --git a/drivers/pci/controller/plda/pcie-plda.h b/drivers/pci/controller/plda/pcie-plda.h
>> index df1729095952..820ea16855b5 100644
>> --- a/drivers/pci/controller/plda/pcie-plda.h
>> +++ b/drivers/pci/controller/plda/pcie-plda.h
>> @@ -129,6 +129,7 @@ struct plda_pcie_rp {
>>  };
>>  
>>  struct plda_event {
>> +	const struct irq_domain_ops *domain_ops;
>>  	const struct plda_event_ops *event_ops;
>>  	int (*request_event_irq)(struct plda_pcie_rp *pcie,
>>  				 int event_irq, int event);
>> -- 
>> 2.17.1
>>
Lorenzo Pieralisi Dec. 13, 2023, 9:50 a.m. UTC | #3 
On Wed, Dec 13, 2023 at 04:15:39PM +0800, Minda Chen wrote:
> 
> 
> On 2023/12/12 19:19, Lorenzo Pieralisi wrote:
> > On Wed, Dec 06, 2023 at 06:58:33PM +0800, Minda Chen wrote:
> >> For lack of an MSI controller, The new added PCIe interrupts have to be
> >> added to global interrupt event field. PolarFire event domain ops can not
> >> be re-used.
> > 
> > I don't understand what this means, please explain and I will
> > add it to the commit log.
> > 
> Sorry.
> Microchip Polarfire PCIe adds 11 PCIe interrupts to PCIe global event domain.(Total 28 PCIe interrupts)
> The microchip event domain and event irqchip will handle these interrupts.
> But PLDA host contain 13 fixed PCIe interrupts. PLDA codes just process these
> 13 interrupts. Microchip the event irq codes are quite different and can't be used by PLDA codes.
> So add an event domain field support microchip and other vendor who just using the PLDA interrupts.
> >> PLDA event domain ops instances will be implemented in later patch.
> > 
> > Future patches don't exist, each commit log is a logical change
> > that must make sense on its own, I will remove this sentence.
> > 
> > Lorenzo
> 
> OK, Thanks.
> 
> >> Signed-off-by: Minda Chen <minda.chen@starfivetech.com>
> >> Acked-by: Conor Dooley <conor.dooley@microchip.com>

I am sorry folks but I still don't get what this patch is supposed
to do. To start with it looks like a preparation patch (it does
not have any net effect), that's not OK since it has to be merged
with the patches that actually apply significant changes on top
of this.

Then I need an explanation of what the problem is and what is the
code actually changing in irqdomain/irqchip specific terms.

I can't merge it as-is because I don't understand what it changes
and it is not clear by reading the commit log.

Thanks,
Lorenzo

> >> ---
> >>  drivers/pci/controller/plda/pcie-microchip-host.c | 6 ++++--
> >>  drivers/pci/controller/plda/pcie-plda.h           | 1 +
> >>  2 files changed, 5 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/drivers/pci/controller/plda/pcie-microchip-host.c b/drivers/pci/controller/plda/pcie-microchip-host.c
> >> index f5e7da242aec..e6dcc572b65b 100644
> >> --- a/drivers/pci/controller/plda/pcie-microchip-host.c
> >> +++ b/drivers/pci/controller/plda/pcie-microchip-host.c
> >> @@ -821,13 +821,15 @@ static const struct plda_event_ops mc_event_ops = {
> >>  };
> >>  
> >>  static const struct plda_event mc_event = {
> >> +	.domain_ops        = &mc_event_domain_ops,
> >>  	.event_ops         = &mc_event_ops,
> >>  	.request_event_irq = mc_request_event_irq,
> >>  	.intx_event        = EVENT_LOCAL_PM_MSI_INT_INTX,
> >>  	.msi_event         = EVENT_LOCAL_PM_MSI_INT_MSI,
> >>  };
> >>  
> >> -static int plda_pcie_init_irq_domains(struct plda_pcie_rp *port)
> >> +static int plda_pcie_init_irq_domains(struct plda_pcie_rp *port,
> >> +				      const struct irq_domain_ops *ops)
> >>  {
> >>  	struct device *dev = port->dev;
> >>  	struct device_node *node = dev->of_node;
> >> @@ -941,7 +943,7 @@ static int plda_init_interrupts(struct platform_device *pdev,
> >>  		return -EINVAL;
> >>  	}
> >>  
> >> -	ret = plda_pcie_init_irq_domains(port);
> >> +	ret = plda_pcie_init_irq_domains(port, event->domain_ops);
> >>  	if (ret) {
> >>  		dev_err(dev, "failed creating IRQ domains\n");
> >>  		return ret;
> >> diff --git a/drivers/pci/controller/plda/pcie-plda.h b/drivers/pci/controller/plda/pcie-plda.h
> >> index df1729095952..820ea16855b5 100644
> >> --- a/drivers/pci/controller/plda/pcie-plda.h
> >> +++ b/drivers/pci/controller/plda/pcie-plda.h
> >> @@ -129,6 +129,7 @@ struct plda_pcie_rp {
> >>  };
> >>  
> >>  struct plda_event {
> >> +	const struct irq_domain_ops *domain_ops;
> >>  	const struct plda_event_ops *event_ops;
> >>  	int (*request_event_irq)(struct plda_pcie_rp *pcie,
> >>  				 int event_irq, int event);
> >> -- 
> >> 2.17.1
> >>
Minda Chen Dec. 13, 2023, 10:21 a.m. UTC | #4 
On 2023/12/13 17:50, Lorenzo Pieralisi wrote:
> On Wed, Dec 13, 2023 at 04:15:39PM +0800, Minda Chen wrote:
>> 
>> 
>> On 2023/12/12 19:19, Lorenzo Pieralisi wrote:
>> > On Wed, Dec 06, 2023 at 06:58:33PM +0800, Minda Chen wrote:
>> >> For lack of an MSI controller, The new added PCIe interrupts have to be
>> >> added to global interrupt event field. PolarFire event domain ops can not
>> >> be re-used.
>> > 
>> > I don't understand what this means, please explain and I will
>> > add it to the commit log.
>> > 
>> Sorry.
>> Microchip Polarfire PCIe adds 11 PCIe interrupts to PCIe global event domain.(Total 28 PCIe interrupts)
>> The microchip event domain and event irqchip will handle these interrupts.
>> But PLDA host contain 13 fixed PCIe interrupts. PLDA codes just process these
>> 13 interrupts. Microchip the event irq codes are quite different and can't be used by PLDA codes.
>> So add an event domain field support microchip and other vendor who just using the PLDA interrupts.
>> >> PLDA event domain ops instances will be implemented in later patch.
>> > 
>> > Future patches don't exist, each commit log is a logical change
>> > that must make sense on its own, I will remove this sentence.
>> > 
>> > Lorenzo
>> 
>> OK, Thanks.
>> 
>> >> Signed-off-by: Minda Chen <minda.chen@starfivetech.com>
>> >> Acked-by: Conor Dooley <conor.dooley@microchip.com>
> 
> I am sorry folks but I still don't get what this patch is supposed
> to do. To start with it looks like a preparation patch (it does
> not have any net effect), that's not OK since it has to be merged
> with the patches that actually apply significant changes on top
> of this.
> 
> Then I need an explanation of what the problem is and what is the
> code actually changing in irqdomain/irqchip specific terms.
> 
> I can't merge it as-is because I don't understand what it changes
> and it is not clear by reading the commit log.
> 
> Thanks,
> Lorenzo
> 
Actually I am  not explain this clear. Sorry. I will changed this patch in next version and  add more commit messages.
The added parameter in struct plda_event is event IRQ chip instead  of event IRQ domain ops. Actually the different configuration is irqchip.
And I will add PLDA event irqchip and its functions to this patch, which can be compared with microchip's event irqchip functions. 
>> >> ---
>> >>  drivers/pci/controller/plda/pcie-microchip-host.c | 6 ++++--
>> >>  drivers/pci/controller/plda/pcie-plda.h           | 1 +
>> >>  2 files changed, 5 insertions(+), 2 deletions(-)
>> >> 
>> >> diff --git a/drivers/pci/controller/plda/pcie-microchip-host.c b/drivers/pci/controller/plda/pcie-microchip-host.c
>> >> index f5e7da242aec..e6dcc572b65b 100644
>> >> --- a/drivers/pci/controller/plda/pcie-microchip-host.c
>> >> +++ b/drivers/pci/controller/plda/pcie-microchip-host.c
>> >> @@ -821,13 +821,15 @@ static const struct plda_event_ops mc_event_ops = {
>> >>  };
>> >>  
>> >>  static const struct plda_event mc_event = {
>> >> +	.domain_ops        = &mc_event_domain_ops,
>> >>  	.event_ops         = &mc_event_ops,
>> >>  	.request_event_irq = mc_request_event_irq,
>> >>  	.intx_event        = EVENT_LOCAL_PM_MSI_INT_INTX,
>> >>  	.msi_event         = EVENT_LOCAL_PM_MSI_INT_MSI,
>> >>  };
>> >>  
>> >> -static int plda_pcie_init_irq_domains(struct plda_pcie_rp *port)
>> >> +static int plda_pcie_init_irq_domains(struct plda_pcie_rp *port,
>> >> +				      const struct irq_domain_ops *ops)
>> >>  {
>> >>  	struct device *dev = port->dev;
>> >>  	struct device_node *node = dev->of_node;
>> >> @@ -941,7 +943,7 @@ static int plda_init_interrupts(struct platform_device *pdev,
>> >>  		return -EINVAL;
>> >>  	}
>> >>  
>> >> -	ret = plda_pcie_init_irq_domains(port);
>> >> +	ret = plda_pcie_init_irq_domains(port, event->domain_ops);
>> >>  	if (ret) {
>> >>  		dev_err(dev, "failed creating IRQ domains\n");
>> >>  		return ret;
>> >> diff --git a/drivers/pci/controller/plda/pcie-plda.h b/drivers/pci/controller/plda/pcie-plda.h
>> >> index df1729095952..820ea16855b5 100644
>> >> --- a/drivers/pci/controller/plda/pcie-plda.h
>> >> +++ b/drivers/pci/controller/plda/pcie-plda.h
>> >> @@ -129,6 +129,7 @@ struct plda_pcie_rp {
>> >>  };
>> >>  
>> >>  struct plda_event {
>> >> +	const struct irq_domain_ops *domain_ops;
>> >>  	const struct plda_event_ops *event_ops;
>> >>  	int (*request_event_irq)(struct plda_pcie_rp *pcie,
>> >>  				 int event_irq, int event);
>> >> -- 
>> >> 2.17.1
>> >>
diff mbox series

Patch

diff --git a/drivers/pci/controller/plda/pcie-microchip-host.c b/drivers/pci/controller/plda/pcie-microchip-host.c
index f5e7da242aec..e6dcc572b65b 100644
--- a/drivers/pci/controller/plda/pcie-microchip-host.c
+++ b/drivers/pci/controller/plda/pcie-microchip-host.c
@@ -821,13 +821,15 @@  static const struct plda_event_ops mc_event_ops = {
 };
 
 static const struct plda_event mc_event = {
+	.domain_ops        = &mc_event_domain_ops,
 	.event_ops         = &mc_event_ops,
 	.request_event_irq = mc_request_event_irq,
 	.intx_event        = EVENT_LOCAL_PM_MSI_INT_INTX,
 	.msi_event         = EVENT_LOCAL_PM_MSI_INT_MSI,
 };
 
-static int plda_pcie_init_irq_domains(struct plda_pcie_rp *port)
+static int plda_pcie_init_irq_domains(struct plda_pcie_rp *port,
+				      const struct irq_domain_ops *ops)
 {
 	struct device *dev = port->dev;
 	struct device_node *node = dev->of_node;
@@ -941,7 +943,7 @@  static int plda_init_interrupts(struct platform_device *pdev,
 		return -EINVAL;
 	}
 
-	ret = plda_pcie_init_irq_domains(port);
+	ret = plda_pcie_init_irq_domains(port, event->domain_ops);
 	if (ret) {
 		dev_err(dev, "failed creating IRQ domains\n");
 		return ret;
diff --git a/drivers/pci/controller/plda/pcie-plda.h b/drivers/pci/controller/plda/pcie-plda.h
index df1729095952..820ea16855b5 100644
--- a/drivers/pci/controller/plda/pcie-plda.h
+++ b/drivers/pci/controller/plda/pcie-plda.h
@@ -129,6 +129,7 @@  struct plda_pcie_rp {
 };
 
 struct plda_event {
+	const struct irq_domain_ops *domain_ops;
 	const struct plda_event_ops *event_ops;
 	int (*request_event_irq)(struct plda_pcie_rp *pcie,
 				 int event_irq, int event);