diff mbox series

[v2,6/9] PCI: dwc: Add support to use non default msi_irq_chip

Message ID 20190207110924.30716-7-kishon@ti.com
State Superseded
Headers show
Series PCI: DWC/Keystone: MSI configuration cleanup | expand

Commit Message

Kishon Vijay Abraham I Feb. 7, 2019, 11:09 a.m. UTC 
Platforms using Designware IP uses dw_pci_msi_bottom_irq_chip for
configuring the MSI controller logic within the Designware IP. However
certain platforms like Keystone (K2G) which uses Desingware IP has
it's own MSI controller logic. For handling such platforms,
the irqchip ops uses msi_irq_ack, msi_set_irq, msi_clear_irq callback
functions.

Add support to use different msi_irq_chip with default as
dw_pci_msi_bottom_irq_chip. This is in preparation to get rid off
msi_irq_ack, msi_set_irq, msi_clear_irq and other Keystone specific
dw_pcie_host_ops. This will also help to get rid of get_msi_addr and
get_msi_data ops.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>

---
 drivers/pci/controller/dwc/pcie-designware-host.c | 5 ++++-
 drivers/pci/controller/dwc/pcie-designware.h      | 1 +
 2 files changed, 5 insertions(+), 1 deletion(-)

-- 
2.17.1

Comments

Lorenzo Pieralisi Feb. 7, 2019, 4:48 p.m. UTC | #1 
On Thu, Feb 07, 2019 at 04:39:21PM +0530, Kishon Vijay Abraham I wrote:
> Platforms using Designware IP uses dw_pci_msi_bottom_irq_chip for

> configuring the MSI controller logic within the Designware IP. However

> certain platforms like Keystone (K2G) which uses Desingware IP has

> it's own MSI controller logic. For handling such platforms,

> the irqchip ops uses msi_irq_ack, msi_set_irq, msi_clear_irq callback

> functions.

> 

> Add support to use different msi_irq_chip with default as

> dw_pci_msi_bottom_irq_chip. This is in preparation to get rid off

> msi_irq_ack, msi_set_irq, msi_clear_irq and other Keystone specific

> dw_pcie_host_ops. This will also help to get rid of get_msi_addr and

> get_msi_data ops.

> 

> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>

> ---

>  drivers/pci/controller/dwc/pcie-designware-host.c | 5 ++++-

>  drivers/pci/controller/dwc/pcie-designware.h      | 1 +

>  2 files changed, 5 insertions(+), 1 deletion(-)

> 

> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c

> index 721d60a5d9e4..042de09b0451 100644

> --- a/drivers/pci/controller/dwc/pcie-designware-host.c

> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c

> @@ -245,7 +245,7 @@ static int dw_pcie_irq_domain_alloc(struct irq_domain *domain,

>  

>  	for (i = 0; i < nr_irqs; i++)

>  		irq_domain_set_info(domain, virq + i, bit + i,

> -				    &dw_pci_msi_bottom_irq_chip,

> +				    pp->msi_irq_chip,

>  				    pp, handle_edge_irq,

>  				    NULL, NULL);

>  

> @@ -277,6 +277,9 @@ int dw_pcie_allocate_domains(struct pcie_port *pp)

>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);

>  	struct fwnode_handle *fwnode = of_node_to_fwnode(pci->dev->of_node);

>  

> +	if (!pp->msi_irq_chip)

> +		pp->msi_irq_chip = &dw_pci_msi_bottom_irq_chip;


I think it is better to initialize pp->msi_irq_chip outside
dw_pcie_allocate_domains(), it makes things clearer.

In:

dw_pcie_host_init() for dwc

or

msi_host_init() for platforms with that hook implemented.

Is there any gotcha I am missing ?

Lorenzo

> +

>  	pp->irq_domain = irq_domain_create_linear(fwnode, pp->num_vectors,

>  					       &dw_pcie_msi_domain_ops, pp);

>  	if (!pp->irq_domain) {

> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h

> index 1f56e6ae34ff..95e0c3c93f48 100644

> --- a/drivers/pci/controller/dwc/pcie-designware.h

> +++ b/drivers/pci/controller/dwc/pcie-designware.h

> @@ -176,6 +176,7 @@ struct pcie_port {

>  	struct irq_domain	*irq_domain;

>  	struct irq_domain	*msi_domain;

>  	dma_addr_t		msi_data;

> +	struct irq_chip		*msi_irq_chip;

>  	u32			num_vectors;

>  	u32			irq_status[MAX_MSI_CTRLS];

>  	raw_spinlock_t		lock;

> -- 

> 2.17.1

>
Bjorn Helgaas Feb. 7, 2019, 8:56 p.m. UTC | #2 
On Thu, Feb 07, 2019 at 04:39:21PM +0530, Kishon Vijay Abraham I wrote:
> Platforms using Designware IP uses dw_pci_msi_bottom_irq_chip for

> configuring the MSI controller logic within the Designware IP. However

> certain platforms like Keystone (K2G) which uses Desingware IP has

> it's own MSI controller logic. For handling such platforms,

> the irqchip ops uses msi_irq_ack, msi_set_irq, msi_clear_irq callback

> functions.


s/IP uses/IP use/
s/Desingware/DesignWare/
s/Designware/DesignWare/ (several)
s/IP has/IP have/
s/it's own/their own/
s/ops uses/ops use/

> Add support to use different msi_irq_chip with default as

> dw_pci_msi_bottom_irq_chip. This is in preparation to get rid off

> msi_irq_ack, msi_set_irq, msi_clear_irq and other Keystone specific

> dw_pcie_host_ops. This will also help to get rid of get_msi_addr and

> get_msi_data ops.


s/rid off/rid of/

> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>

> ---

>  drivers/pci/controller/dwc/pcie-designware-host.c | 5 ++++-

>  drivers/pci/controller/dwc/pcie-designware.h      | 1 +

>  2 files changed, 5 insertions(+), 1 deletion(-)

> 

> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c

> index 721d60a5d9e4..042de09b0451 100644

> --- a/drivers/pci/controller/dwc/pcie-designware-host.c

> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c

> @@ -245,7 +245,7 @@ static int dw_pcie_irq_domain_alloc(struct irq_domain *domain,

>  

>  	for (i = 0; i < nr_irqs; i++)

>  		irq_domain_set_info(domain, virq + i, bit + i,

> -				    &dw_pci_msi_bottom_irq_chip,

> +				    pp->msi_irq_chip,

>  				    pp, handle_edge_irq,

>  				    NULL, NULL);

>  

> @@ -277,6 +277,9 @@ int dw_pcie_allocate_domains(struct pcie_port *pp)

>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);

>  	struct fwnode_handle *fwnode = of_node_to_fwnode(pci->dev->of_node);

>  

> +	if (!pp->msi_irq_chip)

> +		pp->msi_irq_chip = &dw_pci_msi_bottom_irq_chip;

> +

>  	pp->irq_domain = irq_domain_create_linear(fwnode, pp->num_vectors,

>  					       &dw_pcie_msi_domain_ops, pp);

>  	if (!pp->irq_domain) {

> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h

> index 1f56e6ae34ff..95e0c3c93f48 100644

> --- a/drivers/pci/controller/dwc/pcie-designware.h

> +++ b/drivers/pci/controller/dwc/pcie-designware.h

> @@ -176,6 +176,7 @@ struct pcie_port {

>  	struct irq_domain	*irq_domain;

>  	struct irq_domain	*msi_domain;

>  	dma_addr_t		msi_data;

> +	struct irq_chip		*msi_irq_chip;

>  	u32			num_vectors;

>  	u32			irq_status[MAX_MSI_CTRLS];

>  	raw_spinlock_t		lock;

> -- 

> 2.17.1

>
Kishon Vijay Abraham I Feb. 8, 2019, 4:46 a.m. UTC | #3 
Hi Lorenzo,

On 07/02/19 10:18 PM, Lorenzo Pieralisi wrote:
> On Thu, Feb 07, 2019 at 04:39:21PM +0530, Kishon Vijay Abraham I wrote:

>> Platforms using Designware IP uses dw_pci_msi_bottom_irq_chip for

>> configuring the MSI controller logic within the Designware IP. However

>> certain platforms like Keystone (K2G) which uses Desingware IP has

>> it's own MSI controller logic. For handling such platforms,

>> the irqchip ops uses msi_irq_ack, msi_set_irq, msi_clear_irq callback

>> functions.

>>

>> Add support to use different msi_irq_chip with default as

>> dw_pci_msi_bottom_irq_chip. This is in preparation to get rid off

>> msi_irq_ack, msi_set_irq, msi_clear_irq and other Keystone specific

>> dw_pcie_host_ops. This will also help to get rid of get_msi_addr and

>> get_msi_data ops.

>>

>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>

>> ---

>>  drivers/pci/controller/dwc/pcie-designware-host.c | 5 ++++-

>>  drivers/pci/controller/dwc/pcie-designware.h      | 1 +

>>  2 files changed, 5 insertions(+), 1 deletion(-)

>>

>> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c

>> index 721d60a5d9e4..042de09b0451 100644

>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c

>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c

>> @@ -245,7 +245,7 @@ static int dw_pcie_irq_domain_alloc(struct irq_domain *domain,

>>  

>>  	for (i = 0; i < nr_irqs; i++)

>>  		irq_domain_set_info(domain, virq + i, bit + i,

>> -				    &dw_pci_msi_bottom_irq_chip,

>> +				    pp->msi_irq_chip,

>>  				    pp, handle_edge_irq,

>>  				    NULL, NULL);

>>  

>> @@ -277,6 +277,9 @@ int dw_pcie_allocate_domains(struct pcie_port *pp)

>>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);

>>  	struct fwnode_handle *fwnode = of_node_to_fwnode(pci->dev->of_node);

>>  

>> +	if (!pp->msi_irq_chip)

>> +		pp->msi_irq_chip = &dw_pci_msi_bottom_irq_chip;

> 

> I think it is better to initialize pp->msi_irq_chip outside

> dw_pcie_allocate_domains(), it makes things clearer.

> 

> In:

> 

> dw_pcie_host_init() for dwc

> 

> or

> 

> msi_host_init() for platforms with that hook implemented.

> 

> Is there any gotcha I am missing ?


I added here only to avoid breaking "git bisect". Next patch adds
ks_pcie_msi_irq_chip in msi_host_init of keystone. However till then it has to
use dw_pci_msi_bottom_irq_chip. Adding anywhere else in dw_pcie_host_init would
mean msi_irq_chip is uninitialized for keystone.

Maybe I can add that in the commit log and move it to dw_pcie_host_init?

Thanks
Kishon
Lorenzo Pieralisi Feb. 8, 2019, 10:22 a.m. UTC | #4 
On Fri, Feb 08, 2019 at 10:16:59AM +0530, Kishon Vijay Abraham I wrote:
> Hi Lorenzo,

> 

> On 07/02/19 10:18 PM, Lorenzo Pieralisi wrote:

> > On Thu, Feb 07, 2019 at 04:39:21PM +0530, Kishon Vijay Abraham I wrote:

> >> Platforms using Designware IP uses dw_pci_msi_bottom_irq_chip for

> >> configuring the MSI controller logic within the Designware IP. However

> >> certain platforms like Keystone (K2G) which uses Desingware IP has

> >> it's own MSI controller logic. For handling such platforms,

> >> the irqchip ops uses msi_irq_ack, msi_set_irq, msi_clear_irq callback

> >> functions.

> >>

> >> Add support to use different msi_irq_chip with default as

> >> dw_pci_msi_bottom_irq_chip. This is in preparation to get rid off

> >> msi_irq_ack, msi_set_irq, msi_clear_irq and other Keystone specific

> >> dw_pcie_host_ops. This will also help to get rid of get_msi_addr and

> >> get_msi_data ops.

> >>

> >> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>

> >> ---

> >>  drivers/pci/controller/dwc/pcie-designware-host.c | 5 ++++-

> >>  drivers/pci/controller/dwc/pcie-designware.h      | 1 +

> >>  2 files changed, 5 insertions(+), 1 deletion(-)

> >>

> >> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c

> >> index 721d60a5d9e4..042de09b0451 100644

> >> --- a/drivers/pci/controller/dwc/pcie-designware-host.c

> >> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c

> >> @@ -245,7 +245,7 @@ static int dw_pcie_irq_domain_alloc(struct irq_domain *domain,

> >>  

> >>  	for (i = 0; i < nr_irqs; i++)

> >>  		irq_domain_set_info(domain, virq + i, bit + i,

> >> -				    &dw_pci_msi_bottom_irq_chip,

> >> +				    pp->msi_irq_chip,

> >>  				    pp, handle_edge_irq,

> >>  				    NULL, NULL);

> >>  

> >> @@ -277,6 +277,9 @@ int dw_pcie_allocate_domains(struct pcie_port *pp)

> >>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);

> >>  	struct fwnode_handle *fwnode = of_node_to_fwnode(pci->dev->of_node);

> >>  

> >> +	if (!pp->msi_irq_chip)

> >> +		pp->msi_irq_chip = &dw_pci_msi_bottom_irq_chip;

> > 

> > I think it is better to initialize pp->msi_irq_chip outside

> > dw_pcie_allocate_domains(), it makes things clearer.

> > 

> > In:

> > 

> > dw_pcie_host_init() for dwc

> > 

> > or

> > 

> > msi_host_init() for platforms with that hook implemented.

> > 

> > Is there any gotcha I am missing ?

> 

> I added here only to avoid breaking "git bisect". Next patch adds

> ks_pcie_msi_irq_chip in msi_host_init of keystone. However till then it has to

> use dw_pci_msi_bottom_irq_chip. Adding anywhere else in dw_pcie_host_init would

> mean msi_irq_chip is uninitialized for keystone.

> 

> Maybe I can add that in the commit log and move it to dw_pcie_host_init?


I do not think you need to mention that in the commit log but move the
initialization of pp->msi_irq_chip in dw_pcie_host_init() in the next
patch, yes.

It is correct to keep bisectability, I should have read ahead.

Lorenzo
diff mbox series

Patch

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index 721d60a5d9e4..042de09b0451 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -245,7 +245,7 @@  static int dw_pcie_irq_domain_alloc(struct irq_domain *domain,
 
 	for (i = 0; i < nr_irqs; i++)
 		irq_domain_set_info(domain, virq + i, bit + i,
-				    &dw_pci_msi_bottom_irq_chip,
+				    pp->msi_irq_chip,
 				    pp, handle_edge_irq,
 				    NULL, NULL);
 
@@ -277,6 +277,9 @@  int dw_pcie_allocate_domains(struct pcie_port *pp)
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
 	struct fwnode_handle *fwnode = of_node_to_fwnode(pci->dev->of_node);
 
+	if (!pp->msi_irq_chip)
+		pp->msi_irq_chip = &dw_pci_msi_bottom_irq_chip;
+
 	pp->irq_domain = irq_domain_create_linear(fwnode, pp->num_vectors,
 					       &dw_pcie_msi_domain_ops, pp);
 	if (!pp->irq_domain) {
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 1f56e6ae34ff..95e0c3c93f48 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -176,6 +176,7 @@  struct pcie_port {
 	struct irq_domain	*irq_domain;
 	struct irq_domain	*msi_domain;
 	dma_addr_t		msi_data;
+	struct irq_chip		*msi_irq_chip;
 	u32			num_vectors;
 	u32			irq_status[MAX_MSI_CTRLS];
 	raw_spinlock_t		lock;