| Message ID | 20190207110924.30716-9-kishon@ti.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI: DWC/Keystone: MSI configuration cleanup | expand |
On Thu, 2019-02-07 at 16:39 +0530, Kishon Vijay Abraham I wrote: > Now that Keystone started using it's own msi_irq_chip, remove > Keystone specific callback function defined in dw_pcie_host_ops. > > @@ -209,9 +195,6 @@ static void dw_pci_bottom_ack(struct irq_data *d) > > dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_STATUS + res, 4, 1 << bit); > > - if (pp->ops->msi_irq_ack) > - pp->ops->msi_irq_ack(d->hwirq, pp); > - > raw_spin_unlock_irqrestore(&pp->lock, flags); > } After this patch, the entire function now looks like: dw_pci_bottom_ack(struct irq_data *d) { struct pcie_port *pp = irq_data_get_irq_chip_data(d); unsigned int res, bit, ctrl; unsigned long flags; ctrl = d->hwirq / MAX_MSI_IRQS_PER_CTRL; res = ctrl * MSI_REG_CTRL_BLOCK_SIZE; bit = d->hwirq % MAX_MSI_IRQS_PER_CTRL; raw_spin_lock_irqsave(&pp->lock, flags); dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_STATUS + res, 4, 1 << bit); raw_spin_unlock_irqrestore(&pp->lock, flags); } The spin lock isn't necessary anymore since there is nothing but a single register write inside the critical section.
On Thu, Feb 07, 2019 at 04:39:23PM +0530, Kishon Vijay Abraham I wrote: > Now that Keystone started using it's own msi_irq_chip, remove > Keystone specific callback function defined in dw_pcie_host_ops. s/it's/its/ s/callback function/callback functions/ > Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> > Acked-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com> > --- > .../pci/controller/dwc/pcie-designware-host.c | 45 ++++++------------- > drivers/pci/controller/dwc/pcie-designware.h | 5 --- > 2 files changed, 14 insertions(+), 36 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c > index 042de09b0451..9492b05e8ff0 100644 > --- a/drivers/pci/controller/dwc/pcie-designware-host.c > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c > @@ -126,18 +126,12 @@ static void dw_pci_setup_msi_msg(struct irq_data *data, struct msi_msg *msg) > struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > u64 msi_target; > > - if (pp->ops->get_msi_addr) > - msi_target = pp->ops->get_msi_addr(pp); > - else > - msi_target = (u64)pp->msi_data; > + msi_target = (u64)pp->msi_data; > > msg->address_lo = lower_32_bits(msi_target); > msg->address_hi = upper_32_bits(msi_target); > > - if (pp->ops->get_msi_data) > - msg->data = pp->ops->get_msi_data(pp, data->hwirq); > - else > - msg->data = data->hwirq; > + msg->data = data->hwirq; > > dev_dbg(pci->dev, "msi#%d address_hi %#x address_lo %#x\n", > (int)data->hwirq, msg->address_hi, msg->address_lo); > @@ -157,17 +151,13 @@ static void dw_pci_bottom_mask(struct irq_data *data) > > raw_spin_lock_irqsave(&pp->lock, flags); > > - if (pp->ops->msi_clear_irq) { > - pp->ops->msi_clear_irq(pp, data->hwirq); > - } else { > - ctrl = data->hwirq / MAX_MSI_IRQS_PER_CTRL; > - res = ctrl * MSI_REG_CTRL_BLOCK_SIZE; > - bit = data->hwirq % MAX_MSI_IRQS_PER_CTRL; > + ctrl = data->hwirq / MAX_MSI_IRQS_PER_CTRL; > + res = ctrl * MSI_REG_CTRL_BLOCK_SIZE; > + bit = data->hwirq % MAX_MSI_IRQS_PER_CTRL; > > - pp->irq_status[ctrl] &= ~(1 << bit); > - dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_MASK + res, 4, > - ~pp->irq_status[ctrl]); > - } > + pp->irq_status[ctrl] &= ~(1 << bit); > + dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_MASK + res, 4, > + ~pp->irq_status[ctrl]); > > raw_spin_unlock_irqrestore(&pp->lock, flags); > } > @@ -180,17 +170,13 @@ static void dw_pci_bottom_unmask(struct irq_data *data) > > raw_spin_lock_irqsave(&pp->lock, flags); > > - if (pp->ops->msi_set_irq) { > - pp->ops->msi_set_irq(pp, data->hwirq); > - } else { > - ctrl = data->hwirq / MAX_MSI_IRQS_PER_CTRL; > - res = ctrl * MSI_REG_CTRL_BLOCK_SIZE; > - bit = data->hwirq % MAX_MSI_IRQS_PER_CTRL; > + ctrl = data->hwirq / MAX_MSI_IRQS_PER_CTRL; > + res = ctrl * MSI_REG_CTRL_BLOCK_SIZE; > + bit = data->hwirq % MAX_MSI_IRQS_PER_CTRL; > > - pp->irq_status[ctrl] |= 1 << bit; > - dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_MASK + res, 4, > - ~pp->irq_status[ctrl]); > - } > + pp->irq_status[ctrl] |= 1 << bit; > + dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_MASK + res, 4, > + ~pp->irq_status[ctrl]); > > raw_spin_unlock_irqrestore(&pp->lock, flags); > } > @@ -209,9 +195,6 @@ static void dw_pci_bottom_ack(struct irq_data *d) > > dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_STATUS + res, 4, 1 << bit); > > - if (pp->ops->msi_irq_ack) > - pp->ops->msi_irq_ack(d->hwirq, pp); > - > raw_spin_unlock_irqrestore(&pp->lock, flags); > } > > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h > index 95e0c3c93f48..ea4b215b605d 100644 > --- a/drivers/pci/controller/dwc/pcie-designware.h > +++ b/drivers/pci/controller/dwc/pcie-designware.h > @@ -142,14 +142,9 @@ struct dw_pcie_host_ops { > int (*wr_other_conf)(struct pcie_port *pp, struct pci_bus *bus, > unsigned int devfn, int where, int size, u32 val); > int (*host_init)(struct pcie_port *pp); > - void (*msi_set_irq)(struct pcie_port *pp, int irq); > - void (*msi_clear_irq)(struct pcie_port *pp, int irq); > - phys_addr_t (*get_msi_addr)(struct pcie_port *pp); > - u32 (*get_msi_data)(struct pcie_port *pp, int pos); I don't see the whole series on linux-pci (I only see patches 2, 6, 8, 9), but I expected to somewhere see the removal of assignments to these pointers. It would be easier to review if the removal of assignments and the removal of the function pointers from the structure were in the same patch, but maybe that's not really feasible. > void (*scan_bus)(struct pcie_port *pp); > void (*set_num_vectors)(struct pcie_port *pp); > int (*msi_host_init)(struct pcie_port *pp); > - void (*msi_irq_ack)(int irq, struct pcie_port *pp); > }; > > struct pcie_port { > -- > 2.17.1 >
Hi, On 08/02/19 2:56 AM, Bjorn Helgaas wrote: > On Thu, Feb 07, 2019 at 04:39:23PM +0530, Kishon Vijay Abraham I wrote: >> Now that Keystone started using it's own msi_irq_chip, remove >> Keystone specific callback function defined in dw_pcie_host_ops. > > s/it's/its/ > s/callback function/callback functions/ okay. > >> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> >> Acked-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com> >> --- >> .../pci/controller/dwc/pcie-designware-host.c | 45 ++++++------------- >> drivers/pci/controller/dwc/pcie-designware.h | 5 --- >> 2 files changed, 14 insertions(+), 36 deletions(-) >> >> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c >> index 042de09b0451..9492b05e8ff0 100644 >> --- a/drivers/pci/controller/dwc/pcie-designware-host.c >> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c >> @@ -126,18 +126,12 @@ static void dw_pci_setup_msi_msg(struct irq_data *data, struct msi_msg *msg) >> struct dw_pcie *pci = to_dw_pcie_from_pp(pp); >> u64 msi_target; >> >> - if (pp->ops->get_msi_addr) >> - msi_target = pp->ops->get_msi_addr(pp); >> - else >> - msi_target = (u64)pp->msi_data; >> + msi_target = (u64)pp->msi_data; >> >> msg->address_lo = lower_32_bits(msi_target); >> msg->address_hi = upper_32_bits(msi_target); >> >> - if (pp->ops->get_msi_data) >> - msg->data = pp->ops->get_msi_data(pp, data->hwirq); >> - else >> - msg->data = data->hwirq; >> + msg->data = data->hwirq; >> >> dev_dbg(pci->dev, "msi#%d address_hi %#x address_lo %#x\n", >> (int)data->hwirq, msg->address_hi, msg->address_lo); >> @@ -157,17 +151,13 @@ static void dw_pci_bottom_mask(struct irq_data *data) >> >> raw_spin_lock_irqsave(&pp->lock, flags); >> >> - if (pp->ops->msi_clear_irq) { >> - pp->ops->msi_clear_irq(pp, data->hwirq); >> - } else { >> - ctrl = data->hwirq / MAX_MSI_IRQS_PER_CTRL; >> - res = ctrl * MSI_REG_CTRL_BLOCK_SIZE; >> - bit = data->hwirq % MAX_MSI_IRQS_PER_CTRL; >> + ctrl = data->hwirq / MAX_MSI_IRQS_PER_CTRL; >> + res = ctrl * MSI_REG_CTRL_BLOCK_SIZE; >> + bit = data->hwirq % MAX_MSI_IRQS_PER_CTRL; >> >> - pp->irq_status[ctrl] &= ~(1 << bit); >> - dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_MASK + res, 4, >> - ~pp->irq_status[ctrl]); >> - } >> + pp->irq_status[ctrl] &= ~(1 << bit); >> + dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_MASK + res, 4, >> + ~pp->irq_status[ctrl]); >> >> raw_spin_unlock_irqrestore(&pp->lock, flags); >> } >> @@ -180,17 +170,13 @@ static void dw_pci_bottom_unmask(struct irq_data *data) >> >> raw_spin_lock_irqsave(&pp->lock, flags); >> >> - if (pp->ops->msi_set_irq) { >> - pp->ops->msi_set_irq(pp, data->hwirq); >> - } else { >> - ctrl = data->hwirq / MAX_MSI_IRQS_PER_CTRL; >> - res = ctrl * MSI_REG_CTRL_BLOCK_SIZE; >> - bit = data->hwirq % MAX_MSI_IRQS_PER_CTRL; >> + ctrl = data->hwirq / MAX_MSI_IRQS_PER_CTRL; >> + res = ctrl * MSI_REG_CTRL_BLOCK_SIZE; >> + bit = data->hwirq % MAX_MSI_IRQS_PER_CTRL; >> >> - pp->irq_status[ctrl] |= 1 << bit; >> - dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_MASK + res, 4, >> - ~pp->irq_status[ctrl]); >> - } >> + pp->irq_status[ctrl] |= 1 << bit; >> + dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_MASK + res, 4, >> + ~pp->irq_status[ctrl]); >> >> raw_spin_unlock_irqrestore(&pp->lock, flags); >> } >> @@ -209,9 +195,6 @@ static void dw_pci_bottom_ack(struct irq_data *d) >> >> dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_STATUS + res, 4, 1 << bit); >> >> - if (pp->ops->msi_irq_ack) >> - pp->ops->msi_irq_ack(d->hwirq, pp); >> - >> raw_spin_unlock_irqrestore(&pp->lock, flags); >> } >> >> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h >> index 95e0c3c93f48..ea4b215b605d 100644 >> --- a/drivers/pci/controller/dwc/pcie-designware.h >> +++ b/drivers/pci/controller/dwc/pcie-designware.h >> @@ -142,14 +142,9 @@ struct dw_pcie_host_ops { >> int (*wr_other_conf)(struct pcie_port *pp, struct pci_bus *bus, >> unsigned int devfn, int where, int size, u32 val); >> int (*host_init)(struct pcie_port *pp); >> - void (*msi_set_irq)(struct pcie_port *pp, int irq); >> - void (*msi_clear_irq)(struct pcie_port *pp, int irq); >> - phys_addr_t (*get_msi_addr)(struct pcie_port *pp); >> - u32 (*get_msi_data)(struct pcie_port *pp, int pos); > > I don't see the whole series on linux-pci (I only see patches 2, 6, 8, 9), I do see all the patches here https://patchwork.kernel.org/project/linux-pci/list/. Maybe it arrived little late? > but I expected to somewhere see the removal of assignments to these > pointers. It would be easier to review if the removal of assignments and > the removal of the function pointers from the structure were in the same > patch, but maybe that's not really feasible. The removal of the assignments are in 7th patch of the series (PCI: keystone: Use Keystone specific msi_irq_chip). It's feasible but just wanted to keep the keystone changes and DesignWare changes in separate patches. Thanks Kishon
On Fri, Feb 08, 2019 at 10:43:40AM +0530, Kishon Vijay Abraham I wrote: > > I don't see the whole series on linux-pci (I only see patches 2, 6, 8, 9), > > I do see all the patches here > https://patchwork.kernel.org/project/linux-pci/list/. Maybe it arrived little late? For some reason Gmail decided many of your recent messages were spam. Sorry for the noise. > > but I expected to somewhere see the removal of assignments to these > > pointers. It would be easier to review if the removal of assignments and > > the removal of the function pointers from the structure were in the same > > patch, but maybe that's not really feasible. > > The removal of the assignments are in 7th patch of the series (PCI: keystone: > Use Keystone specific msi_irq_chip). It's feasible but just wanted to keep the > keystone changes and DesignWare changes in separate patches. OK, makes sense. Bjorn
diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c index 042de09b0451..9492b05e8ff0 100644 --- a/drivers/pci/controller/dwc/pcie-designware-host.c +++ b/drivers/pci/controller/dwc/pcie-designware-host.c @@ -126,18 +126,12 @@ static void dw_pci_setup_msi_msg(struct irq_data *data, struct msi_msg *msg) struct dw_pcie *pci = to_dw_pcie_from_pp(pp); u64 msi_target; - if (pp->ops->get_msi_addr) - msi_target = pp->ops->get_msi_addr(pp); - else - msi_target = (u64)pp->msi_data; + msi_target = (u64)pp->msi_data; msg->address_lo = lower_32_bits(msi_target); msg->address_hi = upper_32_bits(msi_target); - if (pp->ops->get_msi_data) - msg->data = pp->ops->get_msi_data(pp, data->hwirq); - else - msg->data = data->hwirq; + msg->data = data->hwirq; dev_dbg(pci->dev, "msi#%d address_hi %#x address_lo %#x\n", (int)data->hwirq, msg->address_hi, msg->address_lo); @@ -157,17 +151,13 @@ static void dw_pci_bottom_mask(struct irq_data *data) raw_spin_lock_irqsave(&pp->lock, flags); - if (pp->ops->msi_clear_irq) { - pp->ops->msi_clear_irq(pp, data->hwirq); - } else { - ctrl = data->hwirq / MAX_MSI_IRQS_PER_CTRL; - res = ctrl * MSI_REG_CTRL_BLOCK_SIZE; - bit = data->hwirq % MAX_MSI_IRQS_PER_CTRL; + ctrl = data->hwirq / MAX_MSI_IRQS_PER_CTRL; + res = ctrl * MSI_REG_CTRL_BLOCK_SIZE; + bit = data->hwirq % MAX_MSI_IRQS_PER_CTRL; - pp->irq_status[ctrl] &= ~(1 << bit); - dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_MASK + res, 4, - ~pp->irq_status[ctrl]); - } + pp->irq_status[ctrl] &= ~(1 << bit); + dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_MASK + res, 4, + ~pp->irq_status[ctrl]); raw_spin_unlock_irqrestore(&pp->lock, flags); } @@ -180,17 +170,13 @@ static void dw_pci_bottom_unmask(struct irq_data *data) raw_spin_lock_irqsave(&pp->lock, flags); - if (pp->ops->msi_set_irq) { - pp->ops->msi_set_irq(pp, data->hwirq); - } else { - ctrl = data->hwirq / MAX_MSI_IRQS_PER_CTRL; - res = ctrl * MSI_REG_CTRL_BLOCK_SIZE; - bit = data->hwirq % MAX_MSI_IRQS_PER_CTRL; + ctrl = data->hwirq / MAX_MSI_IRQS_PER_CTRL; + res = ctrl * MSI_REG_CTRL_BLOCK_SIZE; + bit = data->hwirq % MAX_MSI_IRQS_PER_CTRL; - pp->irq_status[ctrl] |= 1 << bit; - dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_MASK + res, 4, - ~pp->irq_status[ctrl]); - } + pp->irq_status[ctrl] |= 1 << bit; + dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_MASK + res, 4, + ~pp->irq_status[ctrl]); raw_spin_unlock_irqrestore(&pp->lock, flags); } @@ -209,9 +195,6 @@ static void dw_pci_bottom_ack(struct irq_data *d) dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_STATUS + res, 4, 1 << bit); - if (pp->ops->msi_irq_ack) - pp->ops->msi_irq_ack(d->hwirq, pp); - raw_spin_unlock_irqrestore(&pp->lock, flags); } diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h index 95e0c3c93f48..ea4b215b605d 100644 --- a/drivers/pci/controller/dwc/pcie-designware.h +++ b/drivers/pci/controller/dwc/pcie-designware.h @@ -142,14 +142,9 @@ struct dw_pcie_host_ops { int (*wr_other_conf)(struct pcie_port *pp, struct pci_bus *bus, unsigned int devfn, int where, int size, u32 val); int (*host_init)(struct pcie_port *pp); - void (*msi_set_irq)(struct pcie_port *pp, int irq); - void (*msi_clear_irq)(struct pcie_port *pp, int irq); - phys_addr_t (*get_msi_addr)(struct pcie_port *pp); - u32 (*get_msi_data)(struct pcie_port *pp, int pos); void (*scan_bus)(struct pcie_port *pp); void (*set_num_vectors)(struct pcie_port *pp); int (*msi_host_init)(struct pcie_port *pp); - void (*msi_irq_ack)(int irq, struct pcie_port *pp); }; struct pcie_port {