[v2,7/8] PCI: microchip: Rename and refactor mc_pcie_enable_msi()

Commit Message

Daire McNamara June 30, 2023, 3:48 p.m. UTC

From: Daire McNamara <[e-mail]>

After improving driver to get MSI-related information from
configuration registers (set at power on from the Libero FPGA
design), its now clear that mc_pcie_enable_msi() is not a good
name for this function.  The function is better named as
mc_pcie_fixup_ecam() as its purpose is to correct the queue
size of the MSI CAP CTRL.

Signed-off-by: Daire McNamara <[e-mail]>
---
 drivers/pci/controller/pcie-microchip-host.c | 28 +++++++++++---------
 1 file changed, 16 insertions(+), 12 deletions(-)

Comments

Conor Dooley July 1, 2023, 11:13 p.m. UTC | #1

On Fri, Jun 30, 2023 at 04:48:58PM +0100, [e-mail] wrote:
> From: Daire McNamara <[e-mail]>
> 
> After improving driver to get MSI-related information from
> configuration registers (set at power on from the Libero FPGA
> design), its now clear that mc_pcie_enable_msi() is not a good
> name for this function.  The function is better named as
> mc_pcie_fixup_ecam() as its purpose is to correct the queue
> size of the MSI CAP CTRL.
> 
> Signed-off-by: Daire McNamara <[e-mail]>

Acked-by: Conor Dooley <[e-mail]>

Cheers,
Conor.

Bjorn Helgaas July 19, 2023, 5:41 p.m. UTC | #2

On Fri, Jun 30, 2023 at 04:48:58PM +0100, [e-mail] wrote:
> From: Daire McNamara <[e-mail]>
> 
> After improving driver to get MSI-related information from
> configuration registers (set at power on from the Libero FPGA
> design), its now clear that mc_pcie_enable_msi() is not a good

it's (contraction of "it is")

> name for this function.  The function is better named as
> mc_pcie_fixup_ecam() as its purpose is to correct the queue
> size of the MSI CAP CTRL.

> -static void mc_pcie_enable_msi(struct mc_pcie *port, void __iomem *base)
> +static void mc_pcie_fixup_ecam(struct mc_pcie *port, void __iomem *ecam)

Since the purpose of this seems to be to fix stuff in the MSI cap,
removing "msi" from the name seems weird.  The fact that it uses ECAM
to access the registers is incidental.

> -	msg_ctrl &= ~PCI_MSI_FLAGS_QSIZE;
> -	msg_ctrl |= queue_size << 4;
> -	writew_relaxed(msg_ctrl, base + cap_offset + PCI_MSI_FLAGS);
> +	reg &= ~PCI_MSI_FLAGS_QSIZE;
> +	reg |= queue_size << 4;

Could maybe use FIELD_PREP() instead of the shift?  I guess this would
go in the "Gather MSI information" patch.

Bjorn

Lorenzo Pieralisi July 27, 2023, 3:27 p.m. UTC | #3

On Wed, Jul 19, 2023 at 12:41:35PM -0500, Bjorn Helgaas wrote:
> On Fri, Jun 30, 2023 at 04:48:58PM +0100, [e-mail] wrote:
> > From: Daire McNamara <[e-mail]>
> > 
> > After improving driver to get MSI-related information from
> > configuration registers (set at power on from the Libero FPGA
> > design), its now clear that mc_pcie_enable_msi() is not a good
> 
> it's (contraction of "it is")
> 
> > name for this function.  The function is better named as
> > mc_pcie_fixup_ecam() as its purpose is to correct the queue
> > size of the MSI CAP CTRL.
> 
> > -static void mc_pcie_enable_msi(struct mc_pcie *port, void __iomem *base)
> > +static void mc_pcie_fixup_ecam(struct mc_pcie *port, void __iomem *ecam)
> 
> Since the purpose of this seems to be to fix stuff in the MSI cap,
> removing "msi" from the name seems weird.  The fact that it uses ECAM
> to access the registers is incidental.
> 
> > -	msg_ctrl &= ~PCI_MSI_FLAGS_QSIZE;
> > -	msg_ctrl |= queue_size << 4;
> > -	writew_relaxed(msg_ctrl, base + cap_offset + PCI_MSI_FLAGS);
> > +	reg &= ~PCI_MSI_FLAGS_QSIZE;
> > +	reg |= queue_size << 4;
> 
> Could maybe use FIELD_PREP() instead of the shift?  I guess this would
> go in the "Gather MSI information" patch.

Daire,

can you follow up on these review comments please ?

Thanks,
Lorenzo

> 
> Bjorn

Patch

diff --git a/drivers/pci/controller/pcie-microchip-host.c b/drivers/pci/controller/pcie-microchip-host.c
index 20ce21438a7e..5bb467de9cc5 100644
--- a/drivers/pci/controller/pcie-microchip-host.c
+++ b/drivers/pci/controller/pcie-microchip-host.c
@@ -383,25 +383,29 @@  static struct {
 
 static char poss_clks[][5] = { "fic0", "fic1", "fic2", "fic3" };
 
-static void mc_pcie_enable_msi(struct mc_pcie *port, void __iomem *base)
+static void mc_pcie_fixup_ecam(struct mc_pcie *port, void __iomem *ecam)
 {
 	struct mc_msi *msi = &port->msi;
-	u32 cap_offset = MC_MSI_CAP_CTRL_OFFSET;
-	u16 msg_ctrl = readw_relaxed(base + cap_offset + PCI_MSI_FLAGS);
+	u16 reg;
 	u8 queue_size;
 
-	msg_ctrl |= PCI_MSI_FLAGS_ENABLE;
+	/* Fixup MSI enable flag */
+	reg = readw_relaxed(ecam + MC_MSI_CAP_CTRL_OFFSET + PCI_MSI_FLAGS);
+	reg |= PCI_MSI_FLAGS_ENABLE;
+	writew_relaxed(reg, ecam + MC_MSI_CAP_CTRL_OFFSET + PCI_MSI_FLAGS);
+
 	/* Fixup PCI MSI queue flags */
-	queue_size = msg_ctrl & PCI_MSI_FLAGS_QMASK;
+	queue_size = reg & PCI_MSI_FLAGS_QMASK;
 	queue_size >>= 1;
-	msg_ctrl &= ~PCI_MSI_FLAGS_QSIZE;
-	msg_ctrl |= queue_size << 4;
-	writew_relaxed(msg_ctrl, base + cap_offset + PCI_MSI_FLAGS);
+	reg &= ~PCI_MSI_FLAGS_QSIZE;
+	reg |= queue_size << 4;
+	writew_relaxed(reg, ecam + MC_MSI_CAP_CTRL_OFFSET + PCI_MSI_FLAGS);
 
+	/* Fixup MSI addr fields */
 	writel_relaxed(lower_32_bits(msi->vector_phy),
-		       base + cap_offset + PCI_MSI_ADDRESS_LO);
+		       ecam + MC_MSI_CAP_CTRL_OFFSET + PCI_MSI_ADDRESS_LO);
 	writel_relaxed(upper_32_bits(msi->vector_phy),
-		       base + cap_offset + PCI_MSI_ADDRESS_HI);
+		       ecam + MC_MSI_CAP_CTRL_OFFSET + PCI_MSI_ADDRESS_HI);
 }
 
 static void mc_handle_msi(struct irq_desc *desc)
@@ -1137,8 +1141,8 @@  static int mc_platform_init(struct pci_config_window *cfg)
 
 	port->msi.num_vectors = 1 << val;
 
-	/* Hardware doesn't setup MSI by default */
-	mc_pcie_enable_msi(port, cfg->win);
+	/* Need some fixups for MSI in config space */
+	mc_pcie_fixup_ecam(port, cfg->win);
 
 	/* Pick vector address from design */
 	port->msi.vector_phy = readl_relaxed(bridge_base_addr + IMSI_ADDR);