linux-kernel - Re: [PATCH v3 2/2] PCI: j721e: Enable ACSPCIE Refclk if "ti,syscon-acspcie-proxy-ctrl" exists

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] [day] [month] [year] [list]

Message-ID: <99e1b482-0a54-4a33-9c59-f9851ef2c1b6@ti.com>
Date: Thu, 29 Aug 2024 11:00:59 +0530
From: Siddharth Vadapalli <s-vadapalli@...com>
To: Bjorn Helgaas <helgaas@...nel.org>
CC: Siddharth Vadapalli <s-vadapalli@...com>, <bhelgaas@...gle.com>,
        <lpieralisi@...nel.org>, <kw@...ux.com>,
        <manivannan.sadhasivam@...aro.org>, <robh@...nel.org>,
        <krzk+dt@...nel.org>, <conor+dt@...nel.org>, <vigneshr@...com>,
        <kishon@...nel.org>, <linux-pci@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>, <srk@...com>
Subject: Re: [PATCH v3 2/2] PCI: j721e: Enable ACSPCIE Refclk if
 "ti,syscon-acspcie-proxy-ctrl" exists

On Wed, Aug 28, 2024 at 04:19:06PM -0500, Bjorn Helgaas wrote:

Hello Bjorn,

> On Tue, Aug 27, 2024 at 11:25:48AM +0530, Siddharth Vadapalli wrote:
> > The ACSPCIE module is capable of driving the reference clock required by
> > the PCIe Endpoint device. It is an alternative to on-board and external
> > reference clock generators. Enabling the output from the ACSPCIE module's
> > PAD IO Buffers requires clearing the "PAD IO disable" bits of the
> > ACSPCIE_PROXY_CTRL register in the CTRL_MMR register space.
> > 
> > Add support to enable the ACSPCIE reference clock output using the optional
> > device-tree property "ti,syscon-acspcie-proxy-ctrl".
> > 
> > Signed-off-by: Siddharth Vadapalli <s-vadapalli@...com>
> > ---
> > 

[...]

> > +
> > +	ret = of_parse_phandle_with_fixed_args(node,
> > +					       "ti,syscon-acspcie-proxy-ctrl",
> > +					       1, 0, &args);
> > +	if (!ret) {
> > +		/* Clear PAD IO disable bits to enable refclk output */
> > +		val = ~(args.args[0]);
> > +		ret = regmap_update_bits(syscon, 0, mask, val);
> > +		if (ret)
> > +			dev_err(dev, "failed to enable ACSPCIE refclk: %d\n",
> > +				ret);
> > +	} else {
> > +		dev_err(dev,
> > +			"ti,syscon-acspcie-proxy-ctrl has invalid arguments\n");
> > +	}
> 
> I should have mentioned this the first time, but this would be easier
> to read if structured as:
> 
>   ret = of_parse_phandle_with_fixed_args(...);
>   if (ret) {
>     dev_err(...);
>     return ret;
>   }
> 
>   /* Clear PAD IO disable bits to enable refclk output */
>   val = ~(args.args[0]);
>   ret = regmap_update_bits(syscon, 0, mask, val);
>   if (ret) {
>     dev_err(...);
>     return ret;
>   }
> 
>   return 0;

Yes, this removes the nested IF conditions and is definitely a cleaner
approach. I will update this in the next version of the patch.

> 
> > +	return ret;
> > +}
> > +
> >  static int j721e_pcie_ctrl_init(struct j721e_pcie *pcie)
> >  {
> >  	struct device *dev = pcie->cdns_pcie->dev;
> > @@ -259,6 +288,15 @@ static int j721e_pcie_ctrl_init(struct j721e_pcie *pcie)
> >  		return ret;
> >  	}
> >  
> > +	/* Enable ACSPCIE refclk output if the optional property exists */
> > +	syscon = syscon_regmap_lookup_by_phandle_optional(node,
> > +						"ti,syscon-acspcie-proxy-ctrl");
> > +	if (syscon) {
> > +		ret = j721e_enable_acspcie_refclk(pcie, syscon);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> >  	return 0;
> 
> Not as dramatic here, but I think the following would be a little
> simpler since the final "return" isn't used for two purposes
> ((1) syscon property absent, (2) syscon present and refclk
> successfully enabled):
> 
>   syscon = syscon_regmap_lookup_by_phandle_optional(...);
>   if (!syscon)
>     return 0;
> 
>   return j721e_enable_acspcie_refclk(...);

Sure. I will implement the above in the next patch. Thank you for
reviewing this patch and sharing your feedback.

Regards,
Siddharth.