Re: [PATCH v4 0/8] PCI: dwc: Add support for Marvell AC5 SoC

Re: [PATCH v4 0/8] PCI: dwc: Add support for Marvell AC5 SoC

[Bjorn Helgaas]

On Mon, Mar 13, 2023 at 02:40:08PM +0200, Elad Nachman wrote:
> ...
> Elad Nachman (5):
>   dt-bindings: PCI: dwc: Add dma-ranges, region mask
>   PCI: armada8k: support AC5 INTx PCIe interrupts
>   PCI: armada8k: support reg regions according to DT.
>   PCI: dwc: Introduce configurable DMA mask
>   PCI: dwc: Introduce region limit from DT

If you repost for any reason, please capitalize consistently ("Support
..." (twice)) and drop the period at end of subject to avoid
unnecessary line wrapping.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 2/8] PCI: armada8k: Add AC5 SoC support

[Bjorn Helgaas]

On Mon, Mar 13, 2023 at 02:40:10PM +0200, Elad Nachman wrote:
> From: Raz Adashi <raza@marvell.com>
> 
> pcie-armada8k driver is utilized to serve also AC5.
> 
> Driver assumes interrupt mask registers are located
> in the same address inboth CPUs. This assumption is
> incorrect - fix it for AC5.

s/inboth/in both/

Rewrap to fill 75 columns.

> +#define PCIE_VENDOR_REGS_OFFSET		0x8000	/* in ac5 is 0x10000 */

Don't add this comment in patch [2/8] and then update it in patch
[6/8].  Make it what you want the first time.  Also nice if you make
it fit in 80 columns like the rest of the file.

> -static const struct dw_pcie_ops dw_pcie_ops = {
> +static u32 ac5_xlate_dbi_reg(u32 reg)
> +{
> +	/* Handle AC5 ATU access */
> +	if ((reg & ~0xfffff) == PCIE_ATU_ACCESS_MASK_AC5) {
> +		reg &= 0xfffff;
> +		/* ATU registers offset is 0xC00 + 0x200 * n,
> +		 * from RFU registers.
> +		 */
> +		reg = 0xc000 | (0x200 * (reg >> 9)) | (reg & 0xff);
> +	} else if ((reg & 0xfffff000) == PCIE_VENDOR_REGS_OFFSET) {
> +		/* PCIe RFU registers in A8K are at offset 0x8000 from base
> +		 * (0xf2600000) while in AC5 offset is 0x10000 from base
> +		 * (0x800a0000) therefore need the addition of 0x8000.
> +		 */
> +		reg += PCIE_VENDOR_REGS_OFFSET;

This is a hidden assumption that the AC5 offset (0x10000) happens to
be twice PCIE_VENDOR_REGS_OFFSET (0x8000).  I think the AC5 offset
deserves its own separate #define.

Use the conventional multi-line comment style:

  /*
   * ...
   */

> +	pci->ops = data->pcie_ops;
>  	pci->dev = dev;
> -	pci->ops = &dw_pcie_ops;

The diff is easier to read if you don't move the "pci->ops ="
assignment.  Move it if there's a *reason* to move it, but leave it
at the same spot otherwise.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 2/8] PCI: armada8k: Add AC5 SoC support

[Serge Semin]

On Mon, Mar 13, 2023 at 02:40:10PM +0200, Elad Nachman wrote:
> From: Raz Adashi <raza@marvell.com>
> 
> pcie-armada8k driver is utilized to serve also AC5.
> 
> Driver assumes interrupt mask registers are located
> in the same address inboth CPUs. This assumption is
> incorrect - fix it for AC5.
> 
> Co-developed-by: Yuval Shaia <yshaia@marvell.com>
> Signed-off-by: Yuval Shaia <yshaia@marvell.com>
> Signed-off-by: Raz Adashi <raza@marvell.com>
> Signed-off-by: Vadym Kochan <vadym.kochan@plvision.eu>
> ---
> v2:
>    1) fix W1 warnings which caused by unused leftover code
> 
>    2) Use one xlate function to translate ac5 dbi access. Also add
>       mode description in comments about this translation.
> 
>    3) Use correct name of Raz
> 
>    4) Use matching data to pass the SoC specific params (type & ops)
> 
>  drivers/pci/controller/dwc/pcie-armada8k.c | 145 +++++++++++++++++----
>  1 file changed, 120 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-armada8k.c b/drivers/pci/controller/dwc/pcie-armada8k.c
> index 5c999e15c357..b9fb1375dc58 100644
> --- a/drivers/pci/controller/dwc/pcie-armada8k.c
> +++ b/drivers/pci/controller/dwc/pcie-armada8k.c
> @@ -16,6 +16,7 @@
>  #include <linux/kernel.h>
>  #include <linux/init.h>
>  #include <linux/of.h>
> +#include <linux/of_device.h>
>  #include <linux/pci.h>
>  #include <linux/phy/phy.h>
>  #include <linux/platform_device.h>
> @@ -26,15 +27,26 @@
>  
>  #define ARMADA8K_PCIE_MAX_LANES PCIE_LNK_X4
>  
> +enum armada8k_pcie_type {
> +	ARMADA8K_PCIE_TYPE_A8K,
> +	ARMADA8K_PCIE_TYPE_AC5
> +};

AFAICS instead of if-else-based pattern you can just split up the
implementation into the dedicated functions.

> +
>  struct armada8k_pcie {
>  	struct dw_pcie *pci;
>  	struct clk *clk;
>  	struct clk *clk_reg;
>  	struct phy *phy[ARMADA8K_PCIE_MAX_LANES];
>  	unsigned int phy_count;
> +	enum armada8k_pcie_type pcie_type;
>  };
>  
> -#define PCIE_VENDOR_REGS_OFFSET		0x8000
> +struct armada8k_pcie_of_data {
> +	enum armada8k_pcie_type pcie_type;
> +	const struct dw_pcie_ops *pcie_ops;
> +};
> +
> +#define PCIE_VENDOR_REGS_OFFSET		0x8000	/* in ac5 is 0x10000 */
>  
>  #define PCIE_GLOBAL_CONTROL_REG		(PCIE_VENDOR_REGS_OFFSET + 0x0)
>  #define PCIE_APP_LTSSM_EN		BIT(2)
> @@ -48,10 +60,17 @@ struct armada8k_pcie {
>  
>  #define PCIE_GLOBAL_INT_CAUSE1_REG	(PCIE_VENDOR_REGS_OFFSET + 0x1C)
>  #define PCIE_GLOBAL_INT_MASK1_REG	(PCIE_VENDOR_REGS_OFFSET + 0x20)
> +#define PCIE_GLOBAL_INT_MASK2_REG	(PCIE_VENDOR_REGS_OFFSET + 0x28)
>  #define PCIE_INT_A_ASSERT_MASK		BIT(9)
>  #define PCIE_INT_B_ASSERT_MASK		BIT(10)
>  #define PCIE_INT_C_ASSERT_MASK		BIT(11)
>  #define PCIE_INT_D_ASSERT_MASK		BIT(12)
> +#define PCIE_INT_A_ASSERT_MASK_AC5	BIT(12)
> +#define PCIE_INT_B_ASSERT_MASK_AC5	BIT(13)
> +#define PCIE_INT_C_ASSERT_MASK_AC5	BIT(14)
> +#define PCIE_INT_D_ASSERT_MASK_AC5	BIT(15)
> +
> +#define PCIE_ATU_ACCESS_MASK_AC5	GENMASK(21, 20)
>  
>  #define PCIE_ARCACHE_TRC_REG		(PCIE_VENDOR_REGS_OFFSET + 0x50)
>  #define PCIE_AWCACHE_TRC_REG		(PCIE_VENDOR_REGS_OFFSET + 0x54)
> @@ -169,6 +188,7 @@ static int armada8k_pcie_host_init(struct dw_pcie_rp *pp)
>  {
>  	u32 reg;
>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	struct armada8k_pcie *pcie = to_armada8k_pcie(pci);
>  
>  	if (!dw_pcie_link_up(pci)) {
>  		/* Disable LTSSM state machine to enable configuration */
> @@ -177,32 +197,41 @@ static int armada8k_pcie_host_init(struct dw_pcie_rp *pp)
>  		dw_pcie_writel_dbi(pci, PCIE_GLOBAL_CONTROL_REG, reg);
>  	}
>  
> -	/* Set the device to root complex mode */
> -	reg = dw_pcie_readl_dbi(pci, PCIE_GLOBAL_CONTROL_REG);
> -	reg &= ~(PCIE_DEVICE_TYPE_MASK << PCIE_DEVICE_TYPE_SHIFT);
> -	reg |= PCIE_DEVICE_TYPE_RC << PCIE_DEVICE_TYPE_SHIFT;
> -	dw_pcie_writel_dbi(pci, PCIE_GLOBAL_CONTROL_REG, reg);
> +	if (pcie->pcie_type == ARMADA8K_PCIE_TYPE_A8K) {
> +		/* Set the device to root complex mode */
> +		reg = dw_pcie_readl_dbi(pci, PCIE_GLOBAL_CONTROL_REG);
> +		reg &= ~(PCIE_DEVICE_TYPE_MASK << PCIE_DEVICE_TYPE_SHIFT);
> +		reg |= PCIE_DEVICE_TYPE_RC << PCIE_DEVICE_TYPE_SHIFT;
> +		dw_pcie_writel_dbi(pci, PCIE_GLOBAL_CONTROL_REG, reg);
>  
> -	/* Set the PCIe master AxCache attributes */
> -	dw_pcie_writel_dbi(pci, PCIE_ARCACHE_TRC_REG, ARCACHE_DEFAULT_VALUE);
> -	dw_pcie_writel_dbi(pci, PCIE_AWCACHE_TRC_REG, AWCACHE_DEFAULT_VALUE);
> +		/* Set the PCIe master AxCache attributes */
> +		dw_pcie_writel_dbi(pci, PCIE_ARCACHE_TRC_REG, ARCACHE_DEFAULT_VALUE);
> +		dw_pcie_writel_dbi(pci, PCIE_AWCACHE_TRC_REG, AWCACHE_DEFAULT_VALUE);
>  
> -	/* Set the PCIe master AxDomain attributes */
> -	reg = dw_pcie_readl_dbi(pci, PCIE_ARUSER_REG);
> -	reg &= ~(AX_USER_DOMAIN_MASK << AX_USER_DOMAIN_SHIFT);
> -	reg |= DOMAIN_OUTER_SHAREABLE << AX_USER_DOMAIN_SHIFT;
> -	dw_pcie_writel_dbi(pci, PCIE_ARUSER_REG, reg);
> +		/* Set the PCIe master AxDomain attributes */
> +		reg = dw_pcie_readl_dbi(pci, PCIE_ARUSER_REG);
> +		reg &= ~(AX_USER_DOMAIN_MASK << AX_USER_DOMAIN_SHIFT);
> +		reg |= DOMAIN_OUTER_SHAREABLE << AX_USER_DOMAIN_SHIFT;
> +		dw_pcie_writel_dbi(pci, PCIE_ARUSER_REG, reg);
>  
> -	reg = dw_pcie_readl_dbi(pci, PCIE_AWUSER_REG);
> -	reg &= ~(AX_USER_DOMAIN_MASK << AX_USER_DOMAIN_SHIFT);
> -	reg |= DOMAIN_OUTER_SHAREABLE << AX_USER_DOMAIN_SHIFT;
> -	dw_pcie_writel_dbi(pci, PCIE_AWUSER_REG, reg);
> +		reg = dw_pcie_readl_dbi(pci, PCIE_AWUSER_REG);
> +		reg &= ~(AX_USER_DOMAIN_MASK << AX_USER_DOMAIN_SHIFT);
> +		reg |= DOMAIN_OUTER_SHAREABLE << AX_USER_DOMAIN_SHIFT;
> +		dw_pcie_writel_dbi(pci, PCIE_AWUSER_REG, reg);
> +	}
>  
>  	/* Enable INT A-D interrupts */
> -	reg = dw_pcie_readl_dbi(pci, PCIE_GLOBAL_INT_MASK1_REG);
> -	reg |= PCIE_INT_A_ASSERT_MASK | PCIE_INT_B_ASSERT_MASK |
> -	       PCIE_INT_C_ASSERT_MASK | PCIE_INT_D_ASSERT_MASK;
> -	dw_pcie_writel_dbi(pci, PCIE_GLOBAL_INT_MASK1_REG, reg);
> +	if (pcie->pcie_type == ARMADA8K_PCIE_TYPE_AC5) {
> +		reg = dw_pcie_readl_dbi(pci, PCIE_GLOBAL_INT_MASK2_REG);
> +		reg |= PCIE_INT_A_ASSERT_MASK_AC5 | PCIE_INT_B_ASSERT_MASK_AC5 |
> +		       PCIE_INT_C_ASSERT_MASK_AC5 | PCIE_INT_D_ASSERT_MASK_AC5;
> +		dw_pcie_writel_dbi(pci, PCIE_GLOBAL_INT_MASK2_REG, reg);
> +	} else {
> +		reg = dw_pcie_readl_dbi(pci, PCIE_GLOBAL_INT_MASK1_REG);
> +		reg |= PCIE_INT_A_ASSERT_MASK | PCIE_INT_B_ASSERT_MASK |
> +		       PCIE_INT_C_ASSERT_MASK | PCIE_INT_D_ASSERT_MASK;
> +		dw_pcie_writel_dbi(pci, PCIE_GLOBAL_INT_MASK1_REG, reg);
> +	}
>  
>  	return 0;
>  }
> @@ -258,9 +287,61 @@ static int armada8k_add_pcie_port(struct armada8k_pcie *pcie,
>  	return 0;
>  }
>  
> -static const struct dw_pcie_ops dw_pcie_ops = {
> +static u32 ac5_xlate_dbi_reg(u32 reg)
> +{
> +	/* Handle AC5 ATU access */
> +	if ((reg & ~0xfffff) == PCIE_ATU_ACCESS_MASK_AC5) {
> +		reg &= 0xfffff;
> +		/* ATU registers offset is 0xC00 + 0x200 * n,
> +		 * from RFU registers.
> +		 */
> +		reg = 0xc000 | (0x200 * (reg >> 9)) | (reg & 0xff);

A custom ATU-base address can be specified instead of this
brain-cracking hack.

> +	} else if ((reg & 0xfffff000) == PCIE_VENDOR_REGS_OFFSET) {
> +		/* PCIe RFU registers in A8K are at offset 0x8000 from base
> +		 * (0xf2600000) while in AC5 offset is 0x10000 from base
> +		 * (0x800a0000) therefore need the addition of 0x8000.
> +		 */
> +		reg += PCIE_VENDOR_REGS_OFFSET;
> +	}

app/ulbi reg-space could be defined instead.

-Serge(y)

> +
> +	return reg;
> +}
> +
> +static u32 ac5_pcie_read_dbi(struct dw_pcie *pci, void __iomem *base,
> +			     u32 reg, size_t size)
> +{
> +	u32 val;
> +
> +	dw_pcie_read(base + ac5_xlate_dbi_reg(reg), size, &val);
> +	return val;
> +}
> +
> +static void ac5_pcie_write_dbi(struct dw_pcie *pci, void __iomem *base,
> +			       u32 reg, size_t size, u32 val)
> +{
> +	dw_pcie_write(base + ac5_xlate_dbi_reg(reg), size, val);
> +}
> +
> +static const struct dw_pcie_ops armada8k_dw_pcie_ops = {
> +	.link_up = armada8k_pcie_link_up,
> +	.start_link = armada8k_pcie_start_link,
> +};
> +
> +static const struct dw_pcie_ops ac5_dw_pcie_ops = {
>  	.link_up = armada8k_pcie_link_up,
>  	.start_link = armada8k_pcie_start_link,
> +	.read_dbi = ac5_pcie_read_dbi,
> +	.write_dbi = ac5_pcie_write_dbi,
> +};
> +
> +static const struct armada8k_pcie_of_data a8k_pcie_of_data = {
> +	.pcie_type = ARMADA8K_PCIE_TYPE_A8K,
> +	.pcie_ops = &armada8k_dw_pcie_ops,
> +};
> +
> +static const struct armada8k_pcie_of_data ac5_pcie_of_data = {
> +	.pcie_type = ARMADA8K_PCIE_TYPE_AC5,
> +	.pcie_ops = &ac5_dw_pcie_ops,
>  };
>  
>  static int armada8k_pcie_probe(struct platform_device *pdev)
> @@ -268,9 +349,15 @@ static int armada8k_pcie_probe(struct platform_device *pdev)
>  	struct dw_pcie *pci;
>  	struct armada8k_pcie *pcie;
>  	struct device *dev = &pdev->dev;
> +	const struct armada8k_pcie_of_data *data;
>  	struct resource *base;
>  	int ret;
>  
> +	data = of_device_get_match_data(dev);
> +	if (!data)
> +		return -EINVAL;
> +
> +
>  	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
>  	if (!pcie)
>  		return -ENOMEM;
> @@ -279,9 +366,10 @@ static int armada8k_pcie_probe(struct platform_device *pdev)
>  	if (!pci)
>  		return -ENOMEM;
>  
> +	pci->ops = data->pcie_ops;
>  	pci->dev = dev;
> -	pci->ops = &dw_pcie_ops;
>  
> +	pcie->pcie_type = data->pcie_type;
>  	pcie->pci = pci;
>  
>  	pcie->clk = devm_clk_get(dev, NULL);
> @@ -334,7 +422,14 @@ static int armada8k_pcie_probe(struct platform_device *pdev)
>  }
>  
>  static const struct of_device_id armada8k_pcie_of_match[] = {
> -	{ .compatible = "marvell,armada8k-pcie", },
> +	{
> +		.compatible = "marvell,armada8k-pcie",
> +		.data = &a8k_pcie_of_data,
> +	},
> +	{
> +		.compatible = "marvell,ac5-pcie",
> +		.data = &ac5_pcie_of_data,
> +	},
>  	{},
>  };
>  
> -- 
> 2.17.1
> 
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 7/8] PCI: dwc: Introduce configurable DMA mask

[Rob Herring]

+Robin

On Mon, Mar 13, 2023 at 02:40:15PM +0200, Elad Nachman wrote:
> From: Elad Nachman <enachman@marvell.com>
> 
> Some devices, such as AC5 and AC5X have their physical DDR memory
> start at address 0x2_0000_0000. In order to have the DMA coherent
> allocation succeed later, a different DMA mask is required, as
> defined in the DT file for such SOCs, using dma-ranges.

I'm afraid this is not right. 'dma-ranges' in the PCI host bridge node 
applies to PCI devices (i.e. child node), not the host bridge itself. 
It's 'dma-ranges' in the parent node of the host bridge that applies 
here. The core code will set masks (ranges really now) based on bus 
restrictions. The mask for the device should only be based on the 
device's limits (i.e. the device is 32-bit only). 

I think you will need whatever solution comes out of this thread[1].

Rob

[1] https://lore.kernel.org/all/c014b074-6d7f-773b-533a-c0500e239ab8@arm.com/

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 7/8] PCI: dwc: Introduce configurable DMA mask

[Robin Murphy]

On 2023-03-17 18:23, Rob Herring wrote:
> +Robin
> 
> On Mon, Mar 13, 2023 at 02:40:15PM +0200, Elad Nachman wrote:
>> From: Elad Nachman <enachman@marvell.com>
>>
>> Some devices, such as AC5 and AC5X have their physical DDR memory
>> start at address 0x2_0000_0000. In order to have the DMA coherent
>> allocation succeed later, a different DMA mask is required, as
>> defined in the DT file for such SOCs, using dma-ranges.
> 
> I'm afraid this is not right. 'dma-ranges' in the PCI host bridge node
> applies to PCI devices (i.e. child node), not the host bridge itself.
> It's 'dma-ranges' in the parent node of the host bridge that applies
> here. The core code will set masks (ranges really now) based on bus
> restrictions. The mask for the device should only be based on the
> device's limits (i.e. the device is 32-bit only).
> 
> I think you will need whatever solution comes out of this thread[1].

Right, "make the allocation succeed later" is entirely missing the point 
of this code. The only reason we're doing that allocation at all is to 
reserve a 32-bit bus address. If it fails, it means we can't reliably 
support endpoints with only a 32-bit MSI capability. Using a bigger mask 
in order to successfully reserve a >32-bit bus address basically 
*guarantees* that you can't support endpoints with only a 32-bit MSI 
capability.

Thanks Rob for digging up that thread; the original idea there should 
still be fine, but the alternative suggestion from Serge at the end is 
potentially even better for this situation where it's down to the SoC's 
memory map rather than the kernel config. It just needs somebody with 
sufficient motivation and resources to write and test a patch :)

Robin.

> 
> Rob
> 
> [1] https://lore.kernel.org/all/c014b074-6d7f-773b-533a-c0500e239ab8@arm.com/

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 4/8] dt-bindings: PCI: dwc: Add dma-ranges, region mask

[Rob Herring]

On Mon, Mar 13, 2023 at 02:40:12PM +0200, Elad Nachman wrote:
> From: Elad Nachman <enachman@marvell.com>
> 
> Add properties to support configurable DMA mask bits and region mask bits:
> 
>  1. configurable dma-ranges is needed for Marvell AC5/AC5X SOCs which
>     have their physical DDR memory start at address 0x2_0000_0000.
> 
>  2. Configurable region mask bits is needed for the Marvell Armada
>     7020/7040/8040 SOCs when the DT file places the PCIe window above the 4GB region.
>     The Synopsis Designware PCIe IP in these SOCs is too old to specify the
>     highest memory location supported by the PCIe, but practically supports
>     such locations. Allow these locations to be specified in the DT file.
> 
> Signed-off-by: Elad Nachman <enachman@marvell.com>
> ---
> v4:
>    1) Fix commit message and its formatting
> 
>    2) Replace num-dmamask with dma-ranges
> 
>  .../devicetree/bindings/pci/snps,dw-pcie-common.yaml        | 5 +++++
>  Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml     | 6 ++++++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie-common.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie-common.yaml
> index d87e13496834..3cb9af1aefeb 100644
> --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie-common.yaml
> +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie-common.yaml
> @@ -261,6 +261,11 @@ properties:
>  
>    dma-coherent: true
>  
> +  num-regionmask:
> +    description: |
> +      number of region limit mask bits to use, if different than default 32
> +    maximum: 64

This should be implied from the compatible string.

> +
>  additionalProperties: true
>  
>  ...
> diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> index 1a83f0f65f19..ed7ae2a14804 100644
> --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> @@ -197,6 +197,12 @@ properties:
>        - contains:
>            const: msi
>  
> +  dma-ranges:
> +    description:
> +      Defines the DMA mask for devices which due to non-standard HW address
> +      assignment have their RAM starting address above the lower 32-bit region.
> +      Since this is a mask, only the size attribute of the dma-ranges is used.
> +

No need for this, it is already defined in pci-bus.yaml.

The description is wrong here anyways. The purpose is to translate 
inbound PCI addresses to parent bus addresses (and eventually CPU 
addresses).

Rob

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 3/8] PCI: armada8k: Add AC5 MSI support

[Serge Semin]

On Mon, Mar 13, 2023 at 02:40:11PM +0200, Elad Nachman wrote:
> From: Yuval Shaia <yshaia@marvell.com>
> 
> AC5 requires different handling for MSI as with armada8k.
> Fix it by:
> 
> 1. Enabling the relevant bits in init phase
> 2. Dispatch virtual IRQ handlers when MSI interrupts are received
> 
> Also enable/disable PCIE_APP_LTSSM for AC5.
> 
> Signed-off-by: Yuval Shaia <yshaia@marvell.com>
> Signed-off-by: Vadym Kochan <vadym.kochan@plvision.eu>
> ---
> v4:
>    Fix commit subject to be aligned with previous patch in series
> 
> v2:
>    1) fix W1 warnings which caused by unused leftover code
> 
>    2) fix type in "requieres" word in the description
> 
>  drivers/pci/controller/dwc/pcie-armada8k.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-armada8k.c b/drivers/pci/controller/dwc/pcie-armada8k.c
> index b9fb1375dc58..02481ecadd25 100644
> --- a/drivers/pci/controller/dwc/pcie-armada8k.c
> +++ b/drivers/pci/controller/dwc/pcie-armada8k.c
> @@ -50,6 +50,7 @@ struct armada8k_pcie_of_data {
>  
>  #define PCIE_GLOBAL_CONTROL_REG		(PCIE_VENDOR_REGS_OFFSET + 0x0)
>  #define PCIE_APP_LTSSM_EN		BIT(2)
> +#define PCIE_APP_LTSSM_EN_AC5		BIT(24)
>  #define PCIE_DEVICE_TYPE_SHIFT		4
>  #define PCIE_DEVICE_TYPE_MASK		0xF
>  #define PCIE_DEVICE_TYPE_RC		0x4 /* Root complex */
> @@ -69,6 +70,7 @@ struct armada8k_pcie_of_data {
>  #define PCIE_INT_B_ASSERT_MASK_AC5	BIT(13)
>  #define PCIE_INT_C_ASSERT_MASK_AC5	BIT(14)
>  #define PCIE_INT_D_ASSERT_MASK_AC5	BIT(15)
> +#define PCIE_MSI_MASK_AC5		BIT(11)
>  
>  #define PCIE_ATU_ACCESS_MASK_AC5	GENMASK(21, 20)
>  
> @@ -184,6 +186,16 @@ static int armada8k_pcie_start_link(struct dw_pcie *pci)
>  	return 0;
>  }
>  
> +static void ac5_pcie_msi_init(struct dw_pcie *pci)
> +{
> +	u32 val;
> +
> +	/* Set MSI bit in interrupt mask */
> +	val = dw_pcie_readl_dbi(pci, PCIE_GLOBAL_INT_MASK1_REG);
> +	val |= PCIE_MSI_MASK_AC5;
> +	dw_pcie_writel_dbi(pci, PCIE_GLOBAL_INT_MASK1_REG, val);
> +}
> +
>  static int armada8k_pcie_host_init(struct dw_pcie_rp *pp)
>  {
>  	u32 reg;
> @@ -193,7 +205,10 @@ static int armada8k_pcie_host_init(struct dw_pcie_rp *pp)
>  	if (!dw_pcie_link_up(pci)) {
>  		/* Disable LTSSM state machine to enable configuration */
>  		reg = dw_pcie_readl_dbi(pci, PCIE_GLOBAL_CONTROL_REG);

> -		reg &= ~(PCIE_APP_LTSSM_EN);
> +		if (pcie->pcie_type == ARMADA8K_PCIE_TYPE_AC5)
> +			reg &= ~(PCIE_APP_LTSSM_EN_AC5);
> +		else
> +			reg &= ~(PCIE_APP_LTSSM_EN);

This has nothing to do with MSIs.

-Serge(y)

>  		dw_pcie_writel_dbi(pci, PCIE_GLOBAL_CONTROL_REG, reg);
>  	}
>  
> @@ -233,6 +248,9 @@ static int armada8k_pcie_host_init(struct dw_pcie_rp *pp)
>  		dw_pcie_writel_dbi(pci, PCIE_GLOBAL_INT_MASK1_REG, reg);
>  	}
>  
> +	if (IS_ENABLED(CONFIG_PCI_MSI) && (pcie->pcie_type == ARMADA8K_PCIE_TYPE_AC5))
> +		ac5_pcie_msi_init(pci);
> +
>  	return 0;
>  }
>  
> @@ -249,6 +267,8 @@ static irqreturn_t armada8k_pcie_irq_handler(int irq, void *arg)
>  	 */
>  	val = dw_pcie_readl_dbi(pci, PCIE_GLOBAL_INT_CAUSE1_REG);
>  	dw_pcie_writel_dbi(pci, PCIE_GLOBAL_INT_CAUSE1_REG, val);
> +	if ((PCIE_MSI_MASK_AC5 & val) && (pcie->pcie_type == ARMADA8K_PCIE_TYPE_AC5))
> +		dw_handle_msi_irq(&pci->pp);
>  
>  	return IRQ_HANDLED;
>  }
> -- 
> 2.17.1
> 
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 8/8] PCI: dwc: Introduce region limit from DT

[Bjorn Helgaas]

[+cc Serge, who has done most of the recent work in this file]

On Mon, Mar 13, 2023 at 02:40:16PM +0200, Elad Nachman wrote:
> From: Elad Nachman <enachman@marvell.com>
> 
> Allow dts override of region limit for SOCs with older Synopsis
> Designware PCIe IP but with greater than 32-bit address range support,
> such as the Armada 7020/7040/8040 family of SOCs by Marvell,
> when the DT file places the PCIe window above the 4GB region.
> The Synopsis Designware PCIe IP in these SOCs is too old to specify the
> highest memory location supported by the PCIe, but practically supports
> such locations. Allow these locations to be specified in the DT file.
> DT property is called num-regionmask , and can range between 33 and 64.

s/Synopsis/Synopsys/ (several occurrences)

s/Designware/DesignWare/ (several occurrences)

Remove space before comma.

> Signed-off-by: Elad Nachman <enachman@marvell.com>
> ---
> v4:
>    1) Fix blank lines removal / addition
> 
>    2) Remove usage of variable with same name as dt binding property
> 
>  drivers/pci/controller/dwc/pcie-designware.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 53a16b8b6ac2..9773c110c733 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -735,8 +735,10 @@ static void dw_pcie_link_set_max_speed(struct dw_pcie *pci, u32 link_gen)
>  void dw_pcie_iatu_detect(struct dw_pcie *pci)
>  {
>  	int max_region, ob, ib;
> -	u32 val, min, dir;
> +	u32 val, min, dir, ret;
>  	u64 max;
> +	struct device *dev = pci->dev;
> +	struct device_node *np = dev->of_node;
>  
>  	val = dw_pcie_readl_dbi(pci, PCIE_ATU_VIEWPORT);
>  	if (val == 0xFFFFFFFF) {
> @@ -781,7 +783,13 @@ void dw_pcie_iatu_detect(struct dw_pcie *pci)
>  		dw_pcie_writel_atu(pci, dir, 0, PCIE_ATU_UPPER_LIMIT, 0xFFFFFFFF);
>  		max = dw_pcie_readl_atu(pci, dir, 0, PCIE_ATU_UPPER_LIMIT);
>  	} else {
> -		max = 0;
> +		/* Allow dts override of region limit for older IP with above 32-bit support: */

Reflow comment to fit in 80 columns.

> +		ret = of_property_read_u32(np, "num-regionmask", &val);
> +		if (!ret && val > 32) {
> +			max = GENMASK(val - 33, 0);
> +			dev_info(pci->dev, "Overriding region limit to %u bits\n", val);
> +		} else
> +			max = 0;
>  	}
>  
>  	pci->num_ob_windows = ob;
> -- 
> 2.17.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 8/8] PCI: dwc: Introduce region limit from DT

[Serge Semin]

Hi Bjorn

On Mon, Mar 13, 2023 at 02:48:02PM -0500, Bjorn Helgaas wrote:
> [+cc Serge, who has done most of the recent work in this file]
> 

Thanks for sending copy to me. I'll have a look at the series on
this week.

-Serge(y)

> On Mon, Mar 13, 2023 at 02:40:16PM +0200, Elad Nachman wrote:
> > From: Elad Nachman <enachman@marvell.com>
> > 
> > Allow dts override of region limit for SOCs with older Synopsis
> > Designware PCIe IP but with greater than 32-bit address range support,
> > such as the Armada 7020/7040/8040 family of SOCs by Marvell,
> > when the DT file places the PCIe window above the 4GB region.
> > The Synopsis Designware PCIe IP in these SOCs is too old to specify the
> > highest memory location supported by the PCIe, but practically supports
> > such locations. Allow these locations to be specified in the DT file.
> > DT property is called num-regionmask , and can range between 33 and 64.
> 
> s/Synopsis/Synopsys/ (several occurrences)
> 
> s/Designware/DesignWare/ (several occurrences)
> 
> Remove space before comma.
> 
> > Signed-off-by: Elad Nachman <enachman@marvell.com>
> > ---
> > v4:
> >    1) Fix blank lines removal / addition
> > 
> >    2) Remove usage of variable with same name as dt binding property
> > 
> >  drivers/pci/controller/dwc/pcie-designware.c | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > index 53a16b8b6ac2..9773c110c733 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > @@ -735,8 +735,10 @@ static void dw_pcie_link_set_max_speed(struct dw_pcie *pci, u32 link_gen)
> >  void dw_pcie_iatu_detect(struct dw_pcie *pci)
> >  {
> >  	int max_region, ob, ib;
> > -	u32 val, min, dir;
> > +	u32 val, min, dir, ret;
> >  	u64 max;
> > +	struct device *dev = pci->dev;
> > +	struct device_node *np = dev->of_node;
> >  
> >  	val = dw_pcie_readl_dbi(pci, PCIE_ATU_VIEWPORT);
> >  	if (val == 0xFFFFFFFF) {
> > @@ -781,7 +783,13 @@ void dw_pcie_iatu_detect(struct dw_pcie *pci)
> >  		dw_pcie_writel_atu(pci, dir, 0, PCIE_ATU_UPPER_LIMIT, 0xFFFFFFFF);
> >  		max = dw_pcie_readl_atu(pci, dir, 0, PCIE_ATU_UPPER_LIMIT);
> >  	} else {
> > -		max = 0;
> > +		/* Allow dts override of region limit for older IP with above 32-bit support: */
> 
> Reflow comment to fit in 80 columns.
> 
> > +		ret = of_property_read_u32(np, "num-regionmask", &val);
> > +		if (!ret && val > 32) {
> > +			max = GENMASK(val - 33, 0);
> > +			dev_info(pci->dev, "Overriding region limit to %u bits\n", val);
> > +		} else
> > +			max = 0;
> >  	}
> >  
> >  	pci->num_ob_windows = ob;
> > -- 
> > 2.17.1
> > 
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 8/8] PCI: dwc: Introduce region limit from DT

[Serge Semin]

On Mon, Mar 13, 2023 at 02:40:16PM +0200, Elad Nachman wrote:
> From: Elad Nachman <enachman@marvell.com>
> 
> Allow dts override of region limit for SOCs with older Synopsis
> Designware PCIe IP but with greater than 32-bit address range support,
> such as the Armada 7020/7040/8040 family of SOCs by Marvell,
> when the DT file places the PCIe window above the 4GB region.
> The Synopsis Designware PCIe IP in these SOCs is too old to specify the
> highest memory location supported by the PCIe, but practically supports
> such locations. Allow these locations to be specified in the DT file.
> DT property is called num-regionmask , and can range between 33 and 64.

The implemented algorithm doesn't prevents you from specifying the
outbound MW base above 4GB. It prevents you from overflowing the limit
address which is of the 32-bits width only for the chips older v4.60a
and if the INCREASE_REGION_SIZE IP-core synthesize parameter is set to
zero.

In other words you must make sure that dma-ranges/ranges entries size
when combined with the source address doesn't cause the limit address
overflow (4GB boundary cross in your case). For instance, you want to
map 0x1F0000000 CPU-address region of 512MB size to 0x0 PCIe address. In
that case you'd specify the ranges property like this:
< ranges = <0x82000000 0 0 0x1 0xf0000000 0 0x20000000>;
The CPU-base address is ok since iATU always supports 64-bit base
addresses. But after you add 0x20000000 to 0x1f0000000 you'll get the
4GB boundary overflow (0x210000000) which can't be described with the
32-bit limit address CSR. In this particular case the maximum range
size you can specify is 0x10000000 (256MB).

Anyway judging by the patch content you do nothing but hacking the
ranges entries sanity check procedure which I don't think is a good
thing to do.

-Serge(y)

> 
> Signed-off-by: Elad Nachman <enachman@marvell.com>
> ---
> v4:
>    1) Fix blank lines removal / addition
> 
>    2) Remove usage of variable with same name as dt binding property
> 
>  drivers/pci/controller/dwc/pcie-designware.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 53a16b8b6ac2..9773c110c733 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -735,8 +735,10 @@ static void dw_pcie_link_set_max_speed(struct dw_pcie *pci, u32 link_gen)
>  void dw_pcie_iatu_detect(struct dw_pcie *pci)
>  {
>  	int max_region, ob, ib;
> -	u32 val, min, dir;
> +	u32 val, min, dir, ret;
>  	u64 max;
> +	struct device *dev = pci->dev;
> +	struct device_node *np = dev->of_node;
>  
>  	val = dw_pcie_readl_dbi(pci, PCIE_ATU_VIEWPORT);
>  	if (val == 0xFFFFFFFF) {
> @@ -781,7 +783,13 @@ void dw_pcie_iatu_detect(struct dw_pcie *pci)
>  		dw_pcie_writel_atu(pci, dir, 0, PCIE_ATU_UPPER_LIMIT, 0xFFFFFFFF);
>  		max = dw_pcie_readl_atu(pci, dir, 0, PCIE_ATU_UPPER_LIMIT);
>  	} else {
> -		max = 0;
> +		/* Allow dts override of region limit for older IP with above 32-bit support: */
> +		ret = of_property_read_u32(np, "num-regionmask", &val);
> +		if (!ret && val > 32) {
> +			max = GENMASK(val - 33, 0);
> +			dev_info(pci->dev, "Overriding region limit to %u bits\n", val);
> +		} else
> +			max = 0;
>  	}
>  
>  	pci->num_ob_windows = ob;
> -- 
> 2.17.1
> 
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel