| Message ID | 20240715120936.1150314-4-s-vadapalli@ti.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Krzysztof Wilczyński |
| Headers | show |
| Series | Add support for ACSPCIE refclk output on J784S4-EVM | expand |
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 > Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> > --- > 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 > @@ -44,6 +44,7 @@ enum link_status { > #define J721E_MODE_RC BIT(7) > #define LANE_COUNT(n) ((n) << 8) > > +#define ACSPCIE_PAD_ENABLE_MASK GENMASK(1, 0) > #define GENERATION_SEL_MASK GENMASK(1, 0) > > struct j721e_pcie { > @@ -220,6 +221,30 @@ static int j721e_pcie_set_lane_count(struct j721e_pcie *pcie, > return ret; > } > > +static int j721e_acspcie_pad_enable(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_ENABLE_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) { > + /* 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. 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 */ 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". > + 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? > + syscon = syscon_regmap_lookup_by_phandle_optional(node, "ti,syscon-acspcie-proxy-ctrl"); > + if (syscon) { > + ret = j721e_acspcie_pad_enable(pcie, syscon); > + if (ret) > + return ret; > + } > + > return 0; > } > > -- > 2.40.1 >
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@ti.com> > > --- > > 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.
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 @@ -44,6 +44,7 @@ enum link_status { #define J721E_MODE_RC BIT(7) #define LANE_COUNT(n) ((n) << 8) +#define ACSPCIE_PAD_ENABLE_MASK GENMASK(1, 0) #define GENERATION_SEL_MASK GENMASK(1, 0) struct j721e_pcie { @@ -220,6 +221,30 @@ static int j721e_pcie_set_lane_count(struct j721e_pcie *pcie, return ret; } +static int j721e_acspcie_pad_enable(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_ENABLE_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) { + /* PAD Enable Bits have to be cleared to in order to enable output */ + 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 */ + syscon = syscon_regmap_lookup_by_phandle_optional(node, "ti,syscon-acspcie-proxy-ctrl"); + if (syscon) { + ret = j721e_acspcie_pad_enable(pcie, syscon); + if (ret) + return ret; + } + return 0; }
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. Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> --- drivers/pci/controller/cadence/pci-j721e.c | 33 ++++++++++++++++++++++ 1 file changed, 33 insertions(+)