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:
 <SN7PR12MB72012F5EB73E0C315467770F8B042@SN7PR12MB7201.namprd12.prod.outlook.com>
Date: Tue, 17 Dec 2024 10:16:01 +0000
From: "Havalige, Thippeswamy" <thippeswamy.havalige@....com>
To: Bjorn Helgaas <helgaas@...nel.org>
CC: "bhelgaas@...gle.com" <bhelgaas@...gle.com>, "lpieralisi@...nel.org"
	<lpieralisi@...nel.org>, "kw@...ux.com" <kw@...ux.com>,
	"manivannan.sadhasivam@...aro.org" <manivannan.sadhasivam@...aro.org>,
	"robh@...nel.org" <robh@...nel.org>, "krzk+dt@...nel.org"
	<krzk+dt@...nel.org>, "conor+dt@...nel.org" <conor+dt@...nel.org>,
	"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"jingoohan1@...il.com" <jingoohan1@...il.com>, "Simek, Michal"
	<michal.simek@....com>, "Gogada, Bharat Kumar" <bharat.kumar.gogada@....com>
Subject: RE: [RESEND PATCH v5 3/3] PCI: amd-mdb: Add AMD MDB Root Port driver

Hi Bjorn,

Thanks for review comments, will fix all below comments in next patch.

Regards,
Thippeswamy H

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@...nel.org>
> Sent: Friday, December 13, 2024 10:58 PM
> To: Havalige, Thippeswamy <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; Simek, Michal
> <michal.simek@....com>; Gogada, Bharat Kumar
> <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