Re: [PATCH V222/7] PCI: dwc: Add msi_ops to allow DBI based MSI register access

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

On Mon, Jul 15, 2024 at 11:13:30AM -0700, Mayank Rana wrote:
> PCIe ECAM driver do not have dw_pcie data structure populated and DBI
> access related APIs. Hence add msi_ops as part of dw_msi structure to
> allow populating DBI based MSI register access.
> 
> Signed-off-by: Mayank Rana <quic_mrana@quicinc.com>
> ---
>  drivers/pci/controller/dwc/pcie-designware-host.c | 20 ++++++++++++-
>  drivers/pci/controller/dwc/pcie-designware-msi.c  | 36 +++++++++++++----------
>  drivers/pci/controller/dwc/pcie-designware-msi.h  | 10 +++++--
>  3 files changed, 47 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index 3dcf88a..7a1eb1f 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -47,6 +47,16 @@ static void dw_pcie_host_request_msg_tlp_res(struct dw_pcie_rp *pp)
>  	}
>  }
>  
> +static u32 dw_pcie_readl_msi_dbi(void *pci, u32 reg)
> +{
> +	return dw_pcie_readl_dbi((struct dw_pcie *)pci, reg);
> +}
> +
> +static void dw_pcie_writel_msi_dbi(void *pci, u32 reg, u32 val)
> +{
> +	dw_pcie_writel_dbi((struct dw_pcie *)pci, reg, val);
> +}
> +

There is nothing MSI specific in dw_pcie_{writel/readl}_msi_dbi(). This just
writes/reads the registers. So this should be called as dw_pcie_write_reg()/
dw_pcie_write_reg().

>  int dw_pcie_host_init(struct dw_pcie_rp *pp)
>  {
>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> @@ -55,6 +65,7 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
>  	struct platform_device *pdev = to_platform_device(dev);
>  	struct resource_entry *win;
>  	struct pci_host_bridge *bridge;
> +	struct dw_msi_ops *msi_ops;
>  	struct resource *res;
>  	bool has_msi_ctrl;
>  	int ret;
> @@ -124,7 +135,14 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
>  			if (ret < 0)
>  				goto err_deinit_host;
>  		} else if (has_msi_ctrl) {
> -			pp->msi = dw_pcie_msi_host_init(pdev, pp, pp->num_vectors);
> +			msi_ops = devm_kzalloc(dev, sizeof(*msi_ops), GFP_KERNEL);
> +			if (msi_ops == NULL)
> +				goto err_deinit_host;
> +
> +			msi_ops->pp = pci;

Stuffing private data inside ops structure looks weird. Also the fact that
allocating memory for ops...

> +			msi_ops->readl_msi = dw_pcie_readl_msi_dbi,
> +			msi_ops->writel_msi = dw_pcie_writel_msi_dbi,

Same for the callback name.

> +			pp->msi = dw_pcie_msi_host_init(pdev, msi_ops, pp->num_vectors);
>  			if (IS_ERR(pp->msi)) {
>  				ret = PTR_ERR(pp->msi);
>  				goto err_deinit_host;
> diff --git a/drivers/pci/controller/dwc/pcie-designware-msi.c b/drivers/pci/controller/dwc/pcie-designware-msi.c
> index 39fe5be..dbfffce 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-msi.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-msi.c
> @@ -44,6 +44,16 @@ static struct msi_domain_info dw_pcie_msi_domain_info = {
>  	.chip	= &dw_pcie_msi_irq_chip,
>  };
>  
> +static u32 dw_msi_readl(struct dw_msi *msi, u32 reg)
> +{
> +	return msi->msi_ops->readl_msi(msi->msi_ops->pp, reg);
> +}
> +
> +static void dw_msi_writel(struct dw_msi *msi, u32 reg, u32 val)
> +{
> +	msi->msi_ops->writel_msi(msi->msi_ops->pp, reg, val);
> +}
> +

These could be:

dw_msi_read_reg()
dw_msi_write_reg()

- Mani

>  /* MSI int handler */
>  irqreturn_t dw_handle_msi_irq(struct dw_msi *msi)
>  {
> @@ -51,13 +61,11 @@ irqreturn_t dw_handle_msi_irq(struct dw_msi *msi)
>  	unsigned long val;
>  	u32 status, num_ctrls;
>  	irqreturn_t ret = IRQ_NONE;
> -	struct dw_pcie *pci = to_dw_pcie_from_pp(msi->pp);
>  
>  	num_ctrls = msi->num_vectors / MAX_MSI_IRQS_PER_CTRL;
>  
>  	for (i = 0; i < num_ctrls; i++) {
> -		status = dw_pcie_readl_dbi(pci, PCIE_MSI_INTR0_STATUS +
> -					   (i * MSI_REG_CTRL_BLOCK_SIZE));
> +		status = dw_msi_readl(msi, PCIE_MSI_INTR0_STATUS + (i * MSI_REG_CTRL_BLOCK_SIZE));
>  		if (!status)
>  			continue;
>  
> @@ -115,7 +123,6 @@ static int dw_pci_msi_set_affinity(struct irq_data *d,
>  static void dw_pci_bottom_mask(struct irq_data *d)
>  {
>  	struct dw_msi *msi = irq_data_get_irq_chip_data(d);
> -	struct dw_pcie *pci = to_dw_pcie_from_pp(msi->pp);
>  	unsigned int res, bit, ctrl;
>  	unsigned long flags;
>  
> @@ -126,7 +133,7 @@ static void dw_pci_bottom_mask(struct irq_data *d)
>  	bit = d->hwirq % MAX_MSI_IRQS_PER_CTRL;
>  
>  	msi->irq_mask[ctrl] |= BIT(bit);
> -	dw_pcie_writel_dbi(pci, PCIE_MSI_INTR0_MASK + res, msi->irq_mask[ctrl]);
> +	dw_msi_writel(msi, PCIE_MSI_INTR0_MASK + res, msi->irq_mask[ctrl]);
>  
>  	raw_spin_unlock_irqrestore(&msi->lock, flags);
>  }
> @@ -134,7 +141,6 @@ static void dw_pci_bottom_mask(struct irq_data *d)
>  static void dw_pci_bottom_unmask(struct irq_data *d)
>  {
>  	struct dw_msi *msi = irq_data_get_irq_chip_data(d);
> -	struct dw_pcie *pci = to_dw_pcie_from_pp(msi->pp);
>  	unsigned int res, bit, ctrl;
>  	unsigned long flags;
>  
> @@ -145,7 +151,7 @@ static void dw_pci_bottom_unmask(struct irq_data *d)
>  	bit = d->hwirq % MAX_MSI_IRQS_PER_CTRL;
>  
>  	msi->irq_mask[ctrl] &= ~BIT(bit);
> -	dw_pcie_writel_dbi(pci, PCIE_MSI_INTR0_MASK + res, msi->irq_mask[ctrl]);
> +	dw_msi_writel(msi, PCIE_MSI_INTR0_MASK + res, msi->irq_mask[ctrl]);
>  
>  	raw_spin_unlock_irqrestore(&msi->lock, flags);
>  }
> @@ -153,14 +159,13 @@ static void dw_pci_bottom_unmask(struct irq_data *d)
>  static void dw_pci_bottom_ack(struct irq_data *d)
>  {
>  	struct dw_msi *msi  = irq_data_get_irq_chip_data(d);
> -	struct dw_pcie *pci = to_dw_pcie_from_pp(msi->pp);
>  	unsigned int res, bit, ctrl;
>  
>  	ctrl = d->hwirq / MAX_MSI_IRQS_PER_CTRL;
>  	res = ctrl * MSI_REG_CTRL_BLOCK_SIZE;
>  	bit = d->hwirq % MAX_MSI_IRQS_PER_CTRL;
>  
> -	dw_pcie_writel_dbi(pci, PCIE_MSI_INTR0_STATUS + res, BIT(bit));
> +	dw_msi_writel(msi, PCIE_MSI_INTR0_STATUS + res, BIT(bit));
>  }
>  
>  static struct irq_chip dw_pci_msi_bottom_irq_chip = {
> @@ -262,7 +267,6 @@ void dw_pcie_free_msi(struct dw_msi *msi)
>  
>  void dw_pcie_msi_init(struct dw_msi *msi)
>  {
> -	struct dw_pcie *pci = to_dw_pcie_from_pp(msi->pp);
>  	u32 ctrl, num_ctrls;
>  	u64 msi_target;
>  
> @@ -273,16 +277,16 @@ void dw_pcie_msi_init(struct dw_msi *msi)
>  	num_ctrls = msi->num_vectors / MAX_MSI_IRQS_PER_CTRL;
>  	/* Initialize IRQ Status array */
>  	for (ctrl = 0; ctrl < num_ctrls; ctrl++) {
> -		dw_pcie_writel_dbi(pci, PCIE_MSI_INTR0_MASK +
> +		dw_msi_writel(msi, PCIE_MSI_INTR0_MASK +
>  				(ctrl * MSI_REG_CTRL_BLOCK_SIZE),
>  				msi->irq_mask[ctrl]);
> -		dw_pcie_writel_dbi(pci, PCIE_MSI_INTR0_ENABLE +
> +		dw_msi_writel(msi, PCIE_MSI_INTR0_ENABLE +
>  				(ctrl * MSI_REG_CTRL_BLOCK_SIZE), ~0);
>  	}
>  
>  	/* Program the msi_data */
> -	dw_pcie_writel_dbi(pci, PCIE_MSI_ADDR_LO, lower_32_bits(msi_target));
> -	dw_pcie_writel_dbi(pci, PCIE_MSI_ADDR_HI, upper_32_bits(msi_target));
> +	dw_msi_writel(msi, PCIE_MSI_ADDR_LO, lower_32_bits(msi_target));
> +	dw_msi_writel(msi, PCIE_MSI_ADDR_HI, upper_32_bits(msi_target));
>  }
>  
>  static int dw_pcie_parse_split_msi_irq(struct dw_msi *msi, struct platform_device *pdev)
> @@ -324,7 +328,7 @@ static int dw_pcie_parse_split_msi_irq(struct dw_msi *msi, struct platform_devic
>  }
>  
>  struct dw_msi *dw_pcie_msi_host_init(struct platform_device *pdev,
> -				void *pp, u32 num_vectors)
> +				struct dw_msi_ops *ops, u32 num_vectors)
>  {
>  	struct device *dev = &pdev->dev;
>  	u64 *msi_vaddr = NULL;
> @@ -341,7 +345,7 @@ struct dw_msi *dw_pcie_msi_host_init(struct platform_device *pdev,
>  
>  	raw_spin_lock_init(&msi->lock);
>  	msi->dev = dev;
> -	msi->pp = pp;
> +	msi->msi_ops = ops;
>  	msi->has_msi_ctrl = true;
>  	msi->num_vectors = num_vectors;
>  
> diff --git a/drivers/pci/controller/dwc/pcie-designware-msi.h b/drivers/pci/controller/dwc/pcie-designware-msi.h
> index 633156e..cf5c612 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-msi.h
> +++ b/drivers/pci/controller/dwc/pcie-designware-msi.h
> @@ -18,8 +18,15 @@
>  #define MSI_REG_CTRL_BLOCK_SIZE		12
>  #define MSI_DEF_NUM_VECTORS		32
>  
> +struct dw_msi_ops {
> +	void	*pp;
> +	u32	(*readl_msi)(void *pp, u32 reg);
> +	void	(*writel_msi)(void *pp, u32 reg, u32 val);
> +};
> +
>  struct dw_msi {
>  	struct device		*dev;
> +	struct dw_msi_ops	*msi_ops;
>  	struct irq_domain	*irq_domain;
>  	struct irq_domain	*msi_domain;
>  	struct irq_chip		*msi_irq_chip;
> @@ -31,11 +38,10 @@ struct dw_msi {
>  	DECLARE_BITMAP(msi_irq_in_use, MAX_MSI_IRQS);
>  	bool                    has_msi_ctrl;
>  	void			*private_data;
> -	void			*pp;
>  };
>  
>  struct dw_msi *dw_pcie_msi_host_init(struct platform_device *pdev,
> -			void *pp, u32 num_vectors);
> +			struct dw_msi_ops *ops, u32 num_vectors);
>  int dw_pcie_allocate_domains(struct dw_msi *msi);
>  void dw_pcie_msi_init(struct dw_msi *msi);
>  void dw_pcie_free_msi(struct dw_msi *msi);
> -- 
> 2.7.4
> 

-- 
மணிவண்ணன் சதாசிவம்