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

[Bjorn Helgaas <helgaas@kernel.org>]

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@ti.com>
> ---
> 
> v2:
> https://lore.kernel.org/r/20240729092855.1945700-3-s-vadapalli@ti.com/
> Changes since v2:
> - Rebased patch on next-20240826.
> 
> v1:
> https://lore.kernel.org/r/20240715120936.1150314-4-s-vadapalli@ti.com/
> Changes since v1:
> - Addressed Bjorn's feedback at:
>   https://lore.kernel.org/r/20240725211841.GA859405@bhelgaas/
>   with the following changes:
>   1) Updated $subject and commit message to indicate that this patch
>   enables ACSPCIE reference clock output if the DT property is present.
>   2) Updated macro and comments to indicate that the BITS correspond to
>   disabling ACSPCIE output, due to which clearing them enables the
>   reference clock output.
>   3) Replaced "PAD" with "refclk" both in the function name and in the
>   error prints.
>   4) Wrapped lines to be within the 80 character limit to match the rest
>   of the driver.
> 
>  drivers/pci/controller/cadence/pci-j721e.c | 38 ++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
> index 85718246016b..ed42b2229483 100644
> --- a/drivers/pci/controller/cadence/pci-j721e.c
> +++ b/drivers/pci/controller/cadence/pci-j721e.c
> @@ -44,6 +44,7 @@ enum link_status {
>  #define J721E_MODE_RC			BIT(7)
>  #define LANE_COUNT(n)			((n) << 8)
>  
> +#define ACSPCIE_PAD_DISABLE_MASK	GENMASK(1, 0)
>  #define GENERATION_SEL_MASK		GENMASK(1, 0)
>  
>  struct j721e_pcie {
> @@ -220,6 +221,34 @@ static int j721e_pcie_set_lane_count(struct j721e_pcie *pcie,
>  	return ret;
>  }
>  
> +static int j721e_enable_acspcie_refclk(struct j721e_pcie *pcie,
> +				       struct regmap *syscon)
> +{
> +	struct device *dev = pcie->cdns_pcie->dev;
> +	struct device_node *node = dev->of_node;
> +	u32 mask = ACSPCIE_PAD_DISABLE_MASK;
> +	struct of_phandle_args args;
> +	u32 val;
> +	int ret;
> +
> +	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;

> +	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(...);

>  }
>  
> -- 
> 2.40.1
>