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.
