Re: [PATCH v3 4/7] dt-bindings: PCI: dwc: add DMA, region mask bits

Re: [PATCH v3 4/7] dt-bindings: PCI: dwc: add DMA, region mask bits

[Krzysztof Kozlowski]

On 23/02/2023 19:05, Elad Nachman wrote:
> From: Elad Nachman <enachman@marvell.com>
> 
> Add properties to support configurable DMA mask bits
> and region mask bits.
> configurable DMA mask bits is needed for Marvell AC5/AC5X SOCs which
> have their physical DDR memory start at address 0x2_0000_0000.
> 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.

This formatting is so bad it makes difficult to read. Make these proper
sentences with proper wrapping.


> First DT property is called num-dmamask,
> and can range between 33 and 64.

Wrong mapping and we see it in the code. No need to code it again in
commit msg. Especially that you already said it in the first sentence.

> Second DT property is called num-regionmask,
> and can range between 33 and 64.


> 
> Signed-off-by: Elad Nachman <enachman@marvell.com>
> ---
>  .../devicetree/bindings/pci/snps,dw-pcie-common.yaml   | 10 ++++++++++
>  1 file changed, 10 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..a1b06ff19ca7 100644
> --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie-common.yaml
> +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie-common.yaml
> @@ -261,6 +261,16 @@ properties:
>  
>    dma-coherent: true
>  
> +  num-dmamask:
> +    description: |
> +      number of dma mask bits to use, if different than default 32

minimum: 33 (from commit msg)
default: 32... which does not make now sense...

> +    maximum: 64
> +
> +  num-regionmask:
> +    description: |
> +      number of region limit mask bits to use, if different than default 32
> +    maximum: 64
> +
>  additionalProperties: true
>  
>  ...

Best regards,
Krzysztof


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

Re: [PATCH v3 4/7] dt-bindings: PCI: dwc: add DMA, region mask bits

[Rob Herring]

On Thu, Feb 23, 2023 at 08:05:28PM +0200, Elad Nachman wrote:
> From: Elad Nachman <enachman@marvell.com>
> 
> Add properties to support configurable DMA mask bits
> and region mask bits.
> configurable DMA mask bits is needed for Marvell AC5/AC5X SOCs which
> have their physical DDR memory start at address 0x2_0000_0000.
> 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.
> First DT property is called num-dmamask,
> and can range between 33 and 64.
> Second DT property is called num-regionmask,
> and can range between 33 and 64.
> 
> Signed-off-by: Elad Nachman <enachman@marvell.com>
> ---
>  .../devicetree/bindings/pci/snps,dw-pcie-common.yaml   | 10 ++++++++++
>  1 file changed, 10 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..a1b06ff19ca7 100644
> --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie-common.yaml
> +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie-common.yaml
> @@ -261,6 +261,16 @@ properties:
>  
>    dma-coherent: true
>  
> +  num-dmamask:

Nope! There's already a defined way to define DMA/bus addresses and 
sizes in DT. That's dma-ranges.

Rob

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

Re: [PATCH v3 6/7] PCI: dwc: Introduce Configurable DMA mask

[Krzysztof Kozlowski]

On 23/02/2023 19:05, 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

There is no space before full stop and comma. Also fix wrapping.

> coherent allocation succeed later, a different DMA mask is
> required, as defined in the DT file for such SOCs.
> If not defined, fallback to 32-bit as previously done in the code.
> DT property is called num-dmamask , and can range between 33 and 64.
> 
> Signed-off-by: Elad Nachman <enachman@marvell.com>
> ---
>  .../pci/controller/dwc/pcie-designware-host.c | 23 ++++++++++++++-----
>  1 file changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index 9952057c8819..ac851b065325 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -204,7 +204,6 @@ static int dw_pcie_irq_domain_alloc(struct irq_domain *domain,
>  				    pp->msi_irq_chip,
>  				    pp, handle_edge_irq,
>  				    NULL, NULL);
> -

How this is related to the commit?

>  	return 0;
>  }
>  
> @@ -250,7 +249,6 @@ int dw_pcie_allocate_domains(struct dw_pcie_rp *pp)
>  		irq_domain_remove(pp->irq_domain);
>  		return -ENOMEM;
>  	}
> -

Same problem... and later in the code as well.

Best regards,
Krzysztof


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

Re: [PATCH v3 7/7] PCI: dwc: Introduce region limit from DT

[Krzysztof Kozlowski]

On 23/02/2023 19:05, 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.
> 
> Signed-off-by: Elad Nachman <enachman@marvell.com>
> ---
>  drivers/pci/controller/dwc/pcie-designware.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 53a16b8b6ac2..429594e853ae 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -401,7 +401,6 @@ static void dw_pcie_writel_atu(struct dw_pcie *pci, u32 dir, u32 index,
>  	int ret;
>  
>  	base = dw_pcie_select_atu(pci, dir, index);
> -
>  	if (pci->ops && pci->ops->write_dbi) {
>  		pci->ops->write_dbi(pci, base, reg, 4, val);
>  		return;
> @@ -735,10 +734,13 @@ 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, num_region_maskbits;

No need to use num_region_maskbits in function scope.

>  	u64 max;
> +	struct device *dev = pci->dev;
> +	struct device_node *np = dev->of_node;
>  
>  	val = dw_pcie_readl_dbi(pci, PCIE_ATU_VIEWPORT);
> +

You need to fix this random changes in unrelated places...

Best regards,
Krzysztof


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

Re: [PATCH v3 0/7] PCI: dwc: Add support for Marvell AC5 SoC

[Bjorn Helgaas]

On Thu, Feb 23, 2023 at 08:05:24PM +0200, Elad Nachman wrote:
> From: Elad Nachman <enachman@marvell.com>
> 
> Add support for AC5 SoC with MSI and in message emulated legacy mode.
> There are differences in the registers addresses, blocks, DDR location
> for coherent DMA allocation and additional implementation specific registers.
> In addition, support cases of older Designware IP (Armada 7020) which supports
> above 4GB PCIe physical memory window by use of device tree.
> ...

> Elad Nachman (4):
>   dt-bindings: PCI: dwc: add DMA, region mask bits
>   PCI: dwc: support AC5 Legacy PCIe interrupts
>   PCI: dwc: Introduce Configurable DMA mask
>   PCI: dwc: Introduce region limit from DT
> 
> Raz Adashi (1):
>   PCI: armada8k: Add AC5 SoC support
> 
> Vadym Kochan (1):
>   dt-bindings: PCI: armada8k: Add compatible string for AC5 SoC
> 
> Yuval Shaia (1):
>   PCI: armada8k: Add MSI support for AC5 SoC

Capitalize subject consistently.  Use consistent driver tags.  Use
parallel sentence structure.

s/add DMA/Add DMA/
s/PCI: dwc: support/PCI: armada8k: Support/
  (this particular patch only affects armada8k, so don't label it "dwc")
s/support/Support/
s/Configurable/configurable/
s/Add MSI support for AC5 SoC/Add AC5 MSI support/
  (parallel to "Add AC5 SoC support")

The PCIe spec doesn't really use "legacy" when defining the interrupt
model.  I think you're referring to INTx, which it *does* use and is
more specific.  If so, please say "INTx interrupts" instead of "legacy
PCIe interrupts".

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