Re: [PATCH v2 2/2] PCI: microchip: rework reg region handing to support using either instance 1 or 2

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Jun 12, 2024 at 11:57:56AM +0100, Conor Dooley wrote:
> From: Conor Dooley <conor.dooley@xxxxxxxxxxxxx>
> 
> The PCI host controller on PolarFire SoC has multiple "instances", each
> with their own bridge and ctrl address spaces. The original binding has
> an "apb" register region, and it is expected to be set to the base
> address of the host controllers register space. Defines in the driver
> were used to compute the addresses of the bridge and ctrl address ranges
> corresponding to instance1. Some customers want to use instance0 however
> and that requires changing the defines in the driver, which is clearly
> not a portable solution.
> 
> The binding has been changed from a single register region to a pair,
> corresponding to the bridge and ctrl regions respectively, so modify the
> driver to read these regions directly from the devicetree rather than
> compute them from the base address of the abp region.
> 
> To maintain backwards compatibility with the existing binding, the
> driver retains code to handle the "abp" reg and computes the base
> address of the bridge and ctrl regions using the defines if it is
> present. reg-names has always been a required property, so this is
> safe to do.
> 
> Signed-off-by: Conor Dooley <conor.dooley@xxxxxxxxxxxxx>
> ---
>  drivers/pci/controller/pcie-microchip-host.c | 155 +++++++++----------
>  1 file changed, 73 insertions(+), 82 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-microchip-host.c b/drivers/pci/controller/pcie-microchip-host.c
> index 137fb8570ba2..2580a5bd2743 100644
> --- a/drivers/pci/controller/pcie-microchip-host.c
> +++ b/drivers/pci/controller/pcie-microchip-host.c
> @@ -27,9 +27,6 @@
>  #define MC_PCIE1_BRIDGE_ADDR			0x00008000u
>  #define MC_PCIE1_CTRL_ADDR			0x0000a000u
>  
> -#define MC_PCIE_BRIDGE_ADDR			(MC_PCIE1_BRIDGE_ADDR)
> -#define MC_PCIE_CTRL_ADDR			(MC_PCIE1_CTRL_ADDR)
> -
>  /* PCIe Bridge Phy Regs */
>  #define PCIE_PCI_IRQ_DW0			0xa8
>  #define  MSIX_CAP_MASK				BIT(31)
> @@ -207,7 +204,6 @@
>  	[EVENT_LOCAL_ ## x] = { __stringify(x), s }
>  
>  #define PCIE_EVENT(x) \
> -	.base = MC_PCIE_CTRL_ADDR, \
>  	.offset = PCIE_EVENT_INT, \
>  	.mask_offset = PCIE_EVENT_INT, \
>  	.mask_high = 1, \
> @@ -215,7 +211,6 @@
>  	.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, \
> @@ -223,7 +218,6 @@
>  	.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, \
> @@ -231,7 +225,6 @@
>  	.enb_mask = 0
>  
>  #define LOCAL_EVENT(x) \
> -	.base = MC_PCIE_BRIDGE_ADDR, \
>  	.offset = ISTATUS_LOCAL, \
>  	.mask_offset = IMASK_LOCAL, \
>  	.mask_high = 0, \
> @@ -265,7 +258,8 @@ struct mc_msi {
>  };
>  
>  struct mc_pcie {
> -	void __iomem *axi_base_addr;
> +	void __iomem *bridge_base_addr;
> +	void __iomem *ctrl_base_addr;
>  	struct device *dev;
>  	struct irq_domain *intx_domain;
>  	struct irq_domain *event_domain;
> @@ -344,7 +338,6 @@ static struct event_map local_status_to_event[] = {
>  };
>  
>  static struct {
> -	u32 base;
>  	u32 offset;
>  	u32 mask;
>  	u32 shift;
> @@ -415,18 +408,17 @@ static void mc_handle_msi(struct irq_desc *desc)
>  	struct irq_chip *chip = irq_desc_get_chip(desc);
>  	struct device *dev = port->dev;
>  	struct mc_msi *msi = &port->msi;
> -	void __iomem *bridge_base_addr =
> -		port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
>  	unsigned long status;
>  	u32 bit;
>  	int ret;
>  
>  	chained_irq_enter(chip, desc);
>  
> -	status = readl_relaxed(bridge_base_addr + ISTATUS_LOCAL);
> +	status = readl_relaxed(port->bridge_base_addr + ISTATUS_LOCAL);
>  	if (status & PM_MSI_INT_MSI_MASK) {
> -		writel_relaxed(status & PM_MSI_INT_MSI_MASK, bridge_base_addr + ISTATUS_LOCAL);
> -		status = readl_relaxed(bridge_base_addr + ISTATUS_MSI);
> +		writel_relaxed(status & PM_MSI_INT_MSI_MASK,
> +			       port->bridge_base_addr + ISTATUS_LOCAL);
> +		status = readl_relaxed(port->bridge_base_addr + ISTATUS_MSI);
>  		for_each_set_bit(bit, &status, msi->num_vectors) {
>  			ret = generic_handle_domain_irq(msi->dev_domain, bit);
>  			if (ret)
> @@ -441,11 +433,9 @@ static void mc_handle_msi(struct irq_desc *desc)
>  static void mc_msi_bottom_irq_ack(struct irq_data *data)
>  {
>  	struct mc_pcie *port = irq_data_get_irq_chip_data(data);
> -	void __iomem *bridge_base_addr =
> -		port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
>  	u32 bitpos = data->hwirq;
>  
> -	writel_relaxed(BIT(bitpos), bridge_base_addr + ISTATUS_MSI);
> +	writel_relaxed(BIT(bitpos), port->bridge_base_addr + ISTATUS_MSI);
>  }
>  
>  static void mc_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
> @@ -564,15 +554,13 @@ static void mc_handle_intx(struct irq_desc *desc)
>  	struct mc_pcie *port = irq_desc_get_handler_data(desc);
>  	struct irq_chip *chip = irq_desc_get_chip(desc);
>  	struct device *dev = port->dev;
> -	void __iomem *bridge_base_addr =
> -		port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
>  	unsigned long status;
>  	u32 bit;
>  	int ret;
>  
>  	chained_irq_enter(chip, desc);
>  
> -	status = readl_relaxed(bridge_base_addr + ISTATUS_LOCAL);
> +	status = readl_relaxed(port->bridge_base_addr + ISTATUS_LOCAL);
>  	if (status & PM_MSI_INT_INTX_MASK) {
>  		status &= PM_MSI_INT_INTX_MASK;
>  		status >>= PM_MSI_INT_INTX_SHIFT;
> @@ -590,42 +578,36 @@ static void mc_handle_intx(struct irq_desc *desc)
>  static void mc_ack_intx_irq(struct irq_data *data)
>  {
>  	struct mc_pcie *port = irq_data_get_irq_chip_data(data);
> -	void __iomem *bridge_base_addr =
> -		port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
>  	u32 mask = BIT(data->hwirq + PM_MSI_INT_INTX_SHIFT);
>  
> -	writel_relaxed(mask, bridge_base_addr + ISTATUS_LOCAL);
> +	writel_relaxed(mask, port->bridge_base_addr + ISTATUS_LOCAL);
>  }
>  
>  static void mc_mask_intx_irq(struct irq_data *data)
>  {
>  	struct mc_pcie *port = irq_data_get_irq_chip_data(data);
> -	void __iomem *bridge_base_addr =
> -		port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
>  	unsigned long flags;
>  	u32 mask = BIT(data->hwirq + PM_MSI_INT_INTX_SHIFT);
>  	u32 val;
>  
>  	raw_spin_lock_irqsave(&port->lock, flags);
> -	val = readl_relaxed(bridge_base_addr + IMASK_LOCAL);
> +	val = readl_relaxed(port->bridge_base_addr + IMASK_LOCAL);
>  	val &= ~mask;
> -	writel_relaxed(val, bridge_base_addr + IMASK_LOCAL);
> +	writel_relaxed(val, port->bridge_base_addr + IMASK_LOCAL);
>  	raw_spin_unlock_irqrestore(&port->lock, flags);
>  }
>  
>  static void mc_unmask_intx_irq(struct irq_data *data)
>  {
>  	struct mc_pcie *port = irq_data_get_irq_chip_data(data);
> -	void __iomem *bridge_base_addr =
> -		port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
>  	unsigned long flags;
>  	u32 mask = BIT(data->hwirq + PM_MSI_INT_INTX_SHIFT);
>  	u32 val;
>  
>  	raw_spin_lock_irqsave(&port->lock, flags);
> -	val = readl_relaxed(bridge_base_addr + IMASK_LOCAL);
> +	val = readl_relaxed(port->bridge_base_addr + IMASK_LOCAL);
>  	val |= mask;
> -	writel_relaxed(val, bridge_base_addr + IMASK_LOCAL);
> +	writel_relaxed(val, port->bridge_base_addr + IMASK_LOCAL);
>  	raw_spin_unlock_irqrestore(&port->lock, flags);
>  }
>  
> @@ -656,8 +638,7 @@ static inline u32 reg_to_event(u32 reg, struct event_map field)
>  
>  static u32 pcie_events(struct mc_pcie *port)
>  {
> -	void __iomem *ctrl_base_addr = port->axi_base_addr + MC_PCIE_CTRL_ADDR;
> -	u32 reg = readl_relaxed(ctrl_base_addr + PCIE_EVENT_INT);
> +	u32 reg = readl_relaxed(port->ctrl_base_addr + PCIE_EVENT_INT);
>  	u32 val = 0;
>  	int i;
>  
> @@ -669,8 +650,7 @@ static u32 pcie_events(struct mc_pcie *port)
>  
>  static u32 sec_errors(struct mc_pcie *port)
>  {
> -	void __iomem *ctrl_base_addr = port->axi_base_addr + MC_PCIE_CTRL_ADDR;
> -	u32 reg = readl_relaxed(ctrl_base_addr + SEC_ERROR_INT);
> +	u32 reg = readl_relaxed(port->ctrl_base_addr + SEC_ERROR_INT);
>  	u32 val = 0;
>  	int i;
>  
> @@ -682,8 +662,7 @@ static u32 sec_errors(struct mc_pcie *port)
>  
>  static u32 ded_errors(struct mc_pcie *port)
>  {
> -	void __iomem *ctrl_base_addr = port->axi_base_addr + MC_PCIE_CTRL_ADDR;
> -	u32 reg = readl_relaxed(ctrl_base_addr + DED_ERROR_INT);
> +	u32 reg = readl_relaxed(port->ctrl_base_addr + DED_ERROR_INT);
>  	u32 val = 0;
>  	int i;
>  
> @@ -695,8 +674,7 @@ static u32 ded_errors(struct mc_pcie *port)
>  
>  static u32 local_events(struct mc_pcie *port)
>  {
> -	void __iomem *bridge_base_addr = port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
> -	u32 reg = readl_relaxed(bridge_base_addr + ISTATUS_LOCAL);
> +	u32 reg = readl_relaxed(port->bridge_base_addr + ISTATUS_LOCAL);
>  	u32 val = 0;
>  	int i;
>  
> @@ -758,8 +736,12 @@ static void mc_ack_event_irq(struct irq_data *data)
>  	void __iomem *addr;
>  	u32 mask;
>  
> -	addr = port->axi_base_addr + event_descs[event].base +
> -		event_descs[event].offset;
> +	if (event_descs[event].offset == ISTATUS_LOCAL)
> +		addr = port->bridge_base_addr;
> +	else
> +		addr = port->ctrl_base_addr;
> +
> +	addr += event_descs[event].offset;
>  	mask = event_descs[event].mask;
>  	mask |= event_descs[event].enb_mask;
>  
> @@ -774,8 +756,12 @@ static void mc_mask_event_irq(struct irq_data *data)
>  	u32 mask;
>  	u32 val;
>  
> -	addr = port->axi_base_addr + event_descs[event].base +
> -		event_descs[event].mask_offset;
> +	if (event_descs[event].offset == ISTATUS_LOCAL)
> +		addr = port->bridge_base_addr;
> +	else
> +		addr = port->ctrl_base_addr;
> +
> +	addr += event_descs[event].mask_offset;
>  	mask = event_descs[event].mask;
>  	if (event_descs[event].enb_mask) {
>  		mask <<= PCIE_EVENT_INT_ENB_SHIFT;
> @@ -804,8 +790,12 @@ static void mc_unmask_event_irq(struct irq_data *data)
>  	u32 mask;
>  	u32 val;
>  
> -	addr = port->axi_base_addr + event_descs[event].base +
> -		event_descs[event].mask_offset;
> +	if (event_descs[event].offset == ISTATUS_LOCAL)
> +		addr = port->bridge_base_addr;
> +	else
> +		addr = port->ctrl_base_addr;
> +
> +	addr += event_descs[event].mask_offset;
>  	mask = event_descs[event].mask;
>  
>  	if (event_descs[event].enb_mask)
> @@ -972,8 +962,6 @@ static void mc_pcie_setup_window(void __iomem *bridge_base_addr, u32 index,
>  static int mc_pcie_setup_windows(struct platform_device *pdev,
>  				 struct mc_pcie *port)
>  {
> -	void __iomem *bridge_base_addr =
> -		port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
>  	struct pci_host_bridge *bridge = platform_get_drvdata(pdev);
>  	struct resource_entry *entry;
>  	u64 pci_addr;
> @@ -982,7 +970,7 @@ static int mc_pcie_setup_windows(struct platform_device *pdev,
>  	resource_list_for_each_entry(entry, &bridge->windows) {
>  		if (resource_type(entry->res) == IORESOURCE_MEM) {
>  			pci_addr = entry->res->start - entry->offset;
> -			mc_pcie_setup_window(bridge_base_addr, index,
> +			mc_pcie_setup_window(port->bridge_base_addr, index,
>  					     entry->res->start, pci_addr,
>  					     resource_size(entry->res));
>  			index++;
> @@ -994,26 +982,20 @@ static int mc_pcie_setup_windows(struct platform_device *pdev,
>  
>  static inline void mc_clear_secs(struct mc_pcie *port)
>  {
> -	void __iomem *ctrl_base_addr = port->axi_base_addr + MC_PCIE_CTRL_ADDR;
> -
> -	writel_relaxed(SEC_ERROR_INT_ALL_RAM_SEC_ERR_INT, ctrl_base_addr +
> +	writel_relaxed(SEC_ERROR_INT_ALL_RAM_SEC_ERR_INT, port->ctrl_base_addr +
>  		       SEC_ERROR_INT);
> -	writel_relaxed(0, ctrl_base_addr + SEC_ERROR_EVENT_CNT);
> +	writel_relaxed(0, port->ctrl_base_addr + SEC_ERROR_EVENT_CNT);
>  }
>  
>  static inline void mc_clear_deds(struct mc_pcie *port)
>  {
> -	void __iomem *ctrl_base_addr = port->axi_base_addr + MC_PCIE_CTRL_ADDR;
> -
> -	writel_relaxed(DED_ERROR_INT_ALL_RAM_DED_ERR_INT, ctrl_base_addr +
> +	writel_relaxed(DED_ERROR_INT_ALL_RAM_DED_ERR_INT, port->ctrl_base_addr +
>  		       DED_ERROR_INT);
> -	writel_relaxed(0, ctrl_base_addr + DED_ERROR_EVENT_CNT);
> +	writel_relaxed(0, port->ctrl_base_addr + DED_ERROR_EVENT_CNT);
>  }
>  
>  static void mc_disable_interrupts(struct mc_pcie *port)
>  {
> -	void __iomem *bridge_base_addr = port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
> -	void __iomem *ctrl_base_addr = port->axi_base_addr + MC_PCIE_CTRL_ADDR;
>  	u32 val;
>  
>  	/* Ensure ECC bypass is enabled */
> @@ -1021,22 +1003,22 @@ static void mc_disable_interrupts(struct mc_pcie *port)
>  	      ECC_CONTROL_RX_RAM_ECC_BYPASS |
>  	      ECC_CONTROL_PCIE2AXI_RAM_ECC_BYPASS |
>  	      ECC_CONTROL_AXI2PCIE_RAM_ECC_BYPASS;
> -	writel_relaxed(val, ctrl_base_addr + ECC_CONTROL);
> +	writel_relaxed(val, port->ctrl_base_addr + ECC_CONTROL);
>  
>  	/* Disable SEC errors and clear any outstanding */
> -	writel_relaxed(SEC_ERROR_INT_ALL_RAM_SEC_ERR_INT, ctrl_base_addr +
> +	writel_relaxed(SEC_ERROR_INT_ALL_RAM_SEC_ERR_INT, port->ctrl_base_addr +
>  		       SEC_ERROR_INT_MASK);
>  	mc_clear_secs(port);
>  
>  	/* Disable DED errors and clear any outstanding */
> -	writel_relaxed(DED_ERROR_INT_ALL_RAM_DED_ERR_INT, ctrl_base_addr +
> +	writel_relaxed(DED_ERROR_INT_ALL_RAM_DED_ERR_INT, port->ctrl_base_addr +
>  		       DED_ERROR_INT_MASK);
>  	mc_clear_deds(port);
>  
>  	/* Disable local interrupts and clear any outstanding */
> -	writel_relaxed(0, bridge_base_addr + IMASK_LOCAL);
> -	writel_relaxed(GENMASK(31, 0), bridge_base_addr + ISTATUS_LOCAL);
> -	writel_relaxed(GENMASK(31, 0), bridge_base_addr + ISTATUS_MSI);
> +	writel_relaxed(0, port->bridge_base_addr + IMASK_LOCAL);
> +	writel_relaxed(GENMASK(31, 0), port->bridge_base_addr + ISTATUS_LOCAL);
> +	writel_relaxed(GENMASK(31, 0), port->bridge_base_addr + ISTATUS_MSI);
>  
>  	/* Disable PCIe events and clear any outstanding */
>  	val = PCIE_EVENT_INT_L2_EXIT_INT |
> @@ -1045,11 +1027,11 @@ static void mc_disable_interrupts(struct mc_pcie *port)
>  	      PCIE_EVENT_INT_L2_EXIT_INT_MASK |
>  	      PCIE_EVENT_INT_HOTRST_EXIT_INT_MASK |
>  	      PCIE_EVENT_INT_DLUP_EXIT_INT_MASK;
> -	writel_relaxed(val, ctrl_base_addr + PCIE_EVENT_INT);
> +	writel_relaxed(val, port->ctrl_base_addr + PCIE_EVENT_INT);
>  
>  	/* Disable host interrupts and clear any outstanding */
> -	writel_relaxed(0, bridge_base_addr + IMASK_HOST);
> -	writel_relaxed(GENMASK(31, 0), bridge_base_addr + ISTATUS_HOST);
> +	writel_relaxed(0, port->bridge_base_addr + IMASK_HOST);
> +	writel_relaxed(GENMASK(31, 0), port->bridge_base_addr + ISTATUS_HOST);
>  }
>  
>  static int mc_init_interrupts(struct platform_device *pdev, struct mc_pcie *port)
> @@ -1112,14 +1094,11 @@ static int mc_platform_init(struct pci_config_window *cfg)
>  {
>  	struct device *dev = cfg->parent;
>  	struct platform_device *pdev = to_platform_device(dev);
> -	void __iomem *bridge_base_addr =
> -		port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
>  	int ret;
>  
>  	/* Configure address translation table 0 for PCIe config space */
> -	mc_pcie_setup_window(bridge_base_addr, 0, cfg->res.start,
> -			     cfg->res.start,
> -			     resource_size(&cfg->res));
> +	mc_pcie_setup_window(port->bridge_base_addr, 0, cfg->res.start,
> +			     cfg->res.start, resource_size(&cfg->res));
>  
>  	/* Need some fixups in config space */
>  	mc_pcie_enable_msi(port, cfg->win);
> @@ -1140,7 +1119,7 @@ static int mc_platform_init(struct pci_config_window *cfg)
>  static int mc_host_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> -	void __iomem *bridge_base_addr;
> +	void __iomem *axi_base_addr;
This is an apb bus. The variable name should be apb_base_addr, not axi_base_addr
>  	int ret;
>  	u32 val;
>  
> @@ -1150,28 +1129,40 @@ static int mc_host_probe(struct platform_device *pdev)
>  
>  	port->dev = dev;
>  
> -	port->axi_base_addr = devm_platform_ioremap_resource(pdev, 1);
> -	if (IS_ERR(port->axi_base_addr))
> -		return PTR_ERR(port->axi_base_addr);
> +	port->bridge_base_addr = devm_platform_ioremap_resource_byname(pdev, "bridge");
> +	port->ctrl_base_addr = devm_platform_ioremap_resource_byname(pdev, "ctrl");
> +	if(!IS_ERR(port->bridge_base_addr) && !IS_ERR(port->ctrl_base_addr))
> +		goto addrs_set;
>  
> +	/*
> +	 * The original, incorrect, binding that lumped the control and
> +	 * bridge addresses together still needs to be handled by the driver.
> +	 */
> +	axi_base_addr = devm_platform_ioremap_resource_byname(pdev, "apb");
> +	if (IS_ERR(axi_base_addr))
> +		return dev_err_probe(dev, PTR_ERR(port->bridge_base_addr),
> +				     "both legacy apb register and ctrl/bridge regions missing");
> +
> +	port->bridge_base_addr = axi_base_addr + MC_PCIE1_BRIDGE_ADDR;
> +	port->ctrl_base_addr = axi_base_addr + MC_PCIE1_CTRL_ADDR;
> +
> +addrs_set:
>  	mc_disable_interrupts(port);
>  
> -	bridge_base_addr = port->axi_base_addr + MC_PCIE_BRIDGE_ADDR;
> -
>  	/* Allow enabling MSI by disabling MSI-X */
> -	val = readl(bridge_base_addr + PCIE_PCI_IRQ_DW0);
> +	val = readl(port->bridge_base_addr + PCIE_PCI_IRQ_DW0);
>  	val &= ~MSIX_CAP_MASK;
> -	writel(val, bridge_base_addr + PCIE_PCI_IRQ_DW0);
> +	writel(val, port->bridge_base_addr + PCIE_PCI_IRQ_DW0);
>  
>  	/* Pick num vectors from bitfile programmed onto FPGA fabric */
> -	val = readl(bridge_base_addr + PCIE_PCI_IRQ_DW0);
> +	val = readl(port->bridge_base_addr + PCIE_PCI_IRQ_DW0);
>  	val &= NUM_MSI_MSGS_MASK;
>  	val >>= NUM_MSI_MSGS_SHIFT;
>  
>  	port->msi.num_vectors = 1 << val;
>  
>  	/* Pick vector address from design */
> -	port->msi.vector_phy = readl_relaxed(bridge_base_addr + IMSI_ADDR);
> +	port->msi.vector_phy = readl_relaxed(port->bridge_base_addr + IMSI_ADDR);
>  
>  	ret = mc_pcie_init_clks(dev);
>  	if (ret) {
> -- 
> 2.43.0
> 
> 




[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux