linux-kernel - Re: [RESEND PATCH v5 3/3] PCI: amd-mdb: Add AMD MDB Root Port driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241213172759.GA3418116@bhelgaas>
Date: Fri, 13 Dec 2024 11:27:59 -0600
From: Bjorn Helgaas <helgaas@...nel.org>
To: Thippeswamy Havalige <thippeswamy.havalige@....com>
Cc: bhelgaas@...gle.com, lpieralisi@...nel.org, kw@...ux.com,
	manivannan.sadhasivam@...aro.org, robh@...nel.org,
	krzk+dt@...nel.org, conor+dt@...nel.org, linux-pci@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	jingoohan1@...il.com, michal.simek@....com,
	bharat.kumar.gogada@....com
Subject: Re: [RESEND PATCH v5 3/3] PCI: amd-mdb: Add AMD MDB Root Port driver

On Fri, Dec 13, 2024 at 12:10:35PM +0530, Thippeswamy Havalige wrote:
> Add support for AMD MDB(Multimedia DMA Bridge) IP core as Root Port.

Add space before "(".

> +config PCIE_AMD_MDB
> +	bool "AMD PCIe controller (host mode)"

Seems too generic to describe this as "the AMD PCIe controller".
Perhaps at least "AMD MDB PCIe controller"?  And/or include "Versal2"?

> +	depends on OF || COMPILE_TEST
> +	depends on PCI && PCI_MSI
> +	select PCIE_DW_HOST
> +	help
> +	  Say Y here to enable PCIe controller support on AMD SoCs. The
> +	  PCIe controller is based on DesignWare Hardware and uses AMD
> +	  hardware wrappers.

Make this help text a little more specific, too.

> + * struct amd_mdb_pcie - PCIe port information
> + * @pci: DesignWare PCIe controller structure
> + * @mdb_base: MDB System Level Control and Status Register(SLCR) Base

Add space before "(".

Thanks for expanding this initialism.  Capitalize it in the text of
other patches so it's obvious that it's an initialism, not a word.

> + * @intx_domain: Legacy IRQ domain pointer

Just say "INTx IRQ domain pointer".  No point in using two terms when
we use "INTx" everywhere else.

> + * @mdb_domain: MDB IRQ domain pointer
> + */
> +struct amd_mdb_pcie {
> +	struct dw_pcie			pci;
> +	void __iomem			*mdb_base;
> +	struct irq_domain		*intx_domain;
> +	struct irq_domain		*mdb_domain;
> +};

> + * amd_mdb_pcie_init_port - Initialize hardware
> + * @pcie: PCIe port information
> + * @pdev: platform device
> + */
> +static int amd_mdb_pcie_init_port(struct amd_mdb_pcie *pcie,
> +				  struct platform_device *pdev)

"pdev" is unused, why include it?

> +static irqreturn_t amd_mdb_pcie_event_flow(int irq, void *args)
> +{
> +	struct amd_mdb_pcie *pcie = args;
> +	unsigned long val;
> +	int i;
> +
> +	val =  pcie_read(pcie, AMD_MDB_TLP_IR_STATUS_MISC);

Spurious extra space.

> +static int amd_mdb_pcie_init_irq_domains(struct amd_mdb_pcie *pcie,
> +					 struct platform_device *pdev)
> +{
> +	struct dw_pcie *pci = &pcie->pci;
> +	struct dw_pcie_rp *pp = &pci->pp;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *node = dev->of_node;
> +	struct device_node *pcie_intc_node;
> +
> +	/* Setup INTx */
> +	pcie_intc_node = of_get_next_child(node, NULL);
> +	if (!pcie_intc_node) {
> +		dev_err(dev, "No PCIe Intc node found\n");
> +		return -EINVAL;
> +	}
> +
> +	pcie->mdb_domain = irq_domain_add_linear(pcie_intc_node, 32,
> +						 &event_domain_ops,
> +					       pcie);

Fix whitespace.  "pcie" would fit on the previous line.

Bjorn