Re: [PATCH 3/3] PCI: j721e: Add support for enabling ACSPCIE PAD IO Buffer output

[Siddharth Vadapalli]

On Thu, Jul 25, 2024 at 04:18:41PM -0500, Bjorn Helgaas wrote:

Hello Bjorn,

> On Mon, Jul 15, 2024 at 05:39:36PM +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.
> 
> And I guess this patch actually *does* enable the ACSPCIE PAD IO
> Buffer output?
> 
> This commit log tells me what is *required* to enable the output, but
> it doesn't actually say whether the patch *does* enable the output.
> 
> Similarly, if this patch enables ACSPCIE PAD IO Buffer output, I would
> make the subject be:
> 
>   PCI: j721e: Enable ACSPCIE Refclk output when DT property is present

I will update the commit message and the $subject to clearly indicate
that the patch enables the reference clock output from the ACSPCIE module.

> 
> > Signed-off-by: Siddharth Vadapalli <s-vadapalli@xxxxxx>
> > ---
> >  drivers/pci/controller/cadence/pci-j721e.c | 33 ++++++++++++++++++++++
> >  1 file changed, 33 insertions(+)
> > 
> > diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
> > index 85718246016b..2fa0eff68a8a 100644
> > --- a/drivers/pci/controller/cadence/pci-j721e.c
> > +++ b/drivers/pci/controller/cadence/pci-j721e.c

[...]

> > +
> > +	ret = of_parse_phandle_with_fixed_args(node, "ti,syscon-acspcie-proxy-ctrl",
> > +					       1, 0, &args);
> > +	if (!ret) {
> > +		/* PAD Enable Bits have to be cleared to in order to enable output */
> 
> Most of this file fits in 80 columns (printf strings are an exception
> so they're easier to find with grep).  It'd be nice if your new code
> and comments fit in 80 columns as well.

I will wrap the lines to the 80 character limit.

> 
> An easy fix for the comment would be:
> 
>   /* Clear PAD Enable bits to enable output */
> 
> Although it sounds non-sensical to *clear* enable bits to enable
> something, and the commit log talks about clearing PAD IO *disable*
> bits, so maybe you meant this instead?
> 
>   /* Clear PAD IO disable bits to enable output */

Thank you for the suggestion. This is much better and I will update the
comment.

> 
> If the logical operation here is to enable driving Refclk, I think the
> function name and error messages might be more informative if they
> mentioned "refclk" instead of "PAD".

While the Hardware terminology is "PAD", looking at it again, I agree
that using "refclk" will be a better choice for describing the objective
of the function, as well as the outcome in case of a failure.

> 
> > +		val = ~(args.args[0]);
> > +		ret = regmap_update_bits(syscon, 0, mask, val);
> > +		if (ret)
> > +			dev_err(dev, "Enabling ACSPCIE PAD output failed: %d\n", ret);
> > +	} else {
> > +		dev_err(dev, "ti,syscon-acspcie-proxy-ctrl has invalid parameters\n");
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> >  static int j721e_pcie_ctrl_init(struct j721e_pcie *pcie)
> >  {
> >  	struct device *dev = pcie->cdns_pcie->dev;
> > @@ -259,6 +284,14 @@ static int j721e_pcie_ctrl_init(struct j721e_pcie *pcie)
> >  		return ret;
> >  	}
> >  
> > +	/* Enable ACSPCIe PAD IO Buffers if the optional property exists */
> 
> Is the canonical name "ACSPCIE" or "ACSPCIe"?  You used "ACSPCIE"
> above?

It is "ACSPCIE" and I have mentioned it that way consistently at all
places including the dt-bindings patches but have accidentally written
"ACSPCIe" above. I will fix this.

Thank you for reviewing this patch.

Regards,
Siddharth.