| Message ID | 20240102-j7200-pcie-s2r-v2-6-8e4f7d228ec2@bootlin.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add suspend to ram support for PCIe on J7200 | expand |
On Fri, Jan 26, 2024 at 4:37 PM Thomas Richard <thomas.richard@bootlin.com> wrote: > > For suspend and resume support, wiz_clock_init needs to be called multiple wiz_clock_init() > times. > > Add a parameter to wiz_clock_init to be able to skip clocks registration. Ditto. ...to skip the registration of the clocks. ... > -static int wiz_clock_init(struct wiz *wiz, struct device_node *node) > +static int wiz_clock_init(struct wiz *wiz, struct device_node *node, bool probe) So, why not refactor this to two functions first? ... > + clk = devm_clk_get(dev, "core_ref_clk"); > + if (IS_ERR(clk)) { > + dev_err(dev, "core_ref_clk clock not found\n"); > + ret = PTR_ERR(clk); > + return ret; This is wrong and has to be fixed in the original code. The correct approach is to use `return dev_err_probe(...)` or open code it, but since we have a helper, open coding is highly discouraged. > + } ... > + if (wiz->data->pma_cmn_refclk1_int_mode) { > + clk = devm_clk_get(dev, "core_ref1_clk"); > + if (IS_ERR(clk)) { > + dev_err(dev, "core_ref1_clk clock not found\n"); > + ret = PTR_ERR(clk); > + return ret; Ditto. > + } ... > + clk = devm_clk_get(dev, "ext_ref_clk"); > + if (IS_ERR(clk)) { > + dev_err(dev, "ext_ref_clk clock not found\n"); > + ret = PTR_ERR(clk); > + return ret; Ditto. > + } ... > + /* What follows is about registering clocks. */ > + if (!probe) > + return 0; Just refactor properly, no need to have this ugly mix of devm_*() for probe stage, parameter and HW related things.
Hi Thomas, On Fri, Jan 26, 2024 at 03:36:48PM +0100, Thomas Richard wrote: > For suspend and resume support, wiz_clock_init needs to be called multiple > times. > > Add a parameter to wiz_clock_init to be able to skip clocks registration. > > Based on the work of Théo Lebrun <theo.lebrun@bootlin.com> > > Signed-off-by: Thomas Richard <thomas.richard@bootlin.com> > --- > drivers/phy/ti/phy-j721e-wiz.c | 60 +++++++++++++++++++++++++----------------- > 1 file changed, 36 insertions(+), 24 deletions(-) > > diff --git a/drivers/phy/ti/phy-j721e-wiz.c b/drivers/phy/ti/phy-j721e-wiz.c > index fc3cd98c60ff..09f7edf16562 100644 > --- a/drivers/phy/ti/phy-j721e-wiz.c > +++ b/drivers/phy/ti/phy-j721e-wiz.c > @@ -1076,7 +1076,7 @@ static int wiz_clock_register(struct wiz *wiz) > return ret; > } > > -static int wiz_clock_init(struct wiz *wiz, struct device_node *node) > +static int wiz_clock_init(struct wiz *wiz, struct device_node *node, bool probe) the 'bool probe' is ugly. When someone reads something like "ret = wiz_clock_init(wiz, node, true);" wonders what the true/false is and needs to follow through the function to understand. Please, split this function (and below) into two parts: wiz_clock_probe() and wiz_clock_init(). The former calls the latter. Later on you can just call wiz_clock_init(). Andi > { > const struct wiz_clk_mux_sel *clk_mux_sel = wiz->clk_mux_sel; > struct device *dev = wiz->dev; > @@ -1087,14 +1087,36 @@ static int wiz_clock_init(struct wiz *wiz, struct device_node *node) > int ret; > int i; > > - clk = devm_clk_get(dev, "core_ref_clk"); > - if (IS_ERR(clk)) { > - dev_err(dev, "core_ref_clk clock not found\n"); > - ret = PTR_ERR(clk); > - return ret; > + if (probe) { > + clk = devm_clk_get(dev, "core_ref_clk"); > + if (IS_ERR(clk)) { > + dev_err(dev, "core_ref_clk clock not found\n"); > + ret = PTR_ERR(clk); > + return ret; > + } > + wiz->input_clks[WIZ_CORE_REFCLK] = clk; > + > + if (wiz->data->pma_cmn_refclk1_int_mode) { > + clk = devm_clk_get(dev, "core_ref1_clk"); > + if (IS_ERR(clk)) { > + dev_err(dev, "core_ref1_clk clock not found\n"); > + ret = PTR_ERR(clk); > + return ret; > + } > + wiz->input_clks[WIZ_CORE_REFCLK1] = clk; > + } > + > + clk = devm_clk_get(dev, "ext_ref_clk"); > + if (IS_ERR(clk)) { > + dev_err(dev, "ext_ref_clk clock not found\n"); > + ret = PTR_ERR(clk); > + return ret; > + } > + wiz->input_clks[WIZ_EXT_REFCLK] = clk; > } > - wiz->input_clks[WIZ_CORE_REFCLK] = clk; > > + > + clk = wiz->input_clks[WIZ_CORE_REFCLK]; > rate = clk_get_rate(clk); > if (rate >= 100000000) > regmap_field_write(wiz->pma_cmn_refclk_int_mode, 0x1); > @@ -1121,14 +1143,7 @@ static int wiz_clock_init(struct wiz *wiz, struct device_node *node) > } > > if (wiz->data->pma_cmn_refclk1_int_mode) { > - clk = devm_clk_get(dev, "core_ref1_clk"); > - if (IS_ERR(clk)) { > - dev_err(dev, "core_ref1_clk clock not found\n"); > - ret = PTR_ERR(clk); > - return ret; > - } > - wiz->input_clks[WIZ_CORE_REFCLK1] = clk; > - > + clk = wiz->input_clks[WIZ_CORE_REFCLK1]; > rate = clk_get_rate(clk); > if (rate >= 100000000) > regmap_field_write(wiz->pma_cmn_refclk1_int_mode, 0x1); > @@ -1136,20 +1151,17 @@ static int wiz_clock_init(struct wiz *wiz, struct device_node *node) > regmap_field_write(wiz->pma_cmn_refclk1_int_mode, 0x3); > } > > - clk = devm_clk_get(dev, "ext_ref_clk"); > - if (IS_ERR(clk)) { > - dev_err(dev, "ext_ref_clk clock not found\n"); > - ret = PTR_ERR(clk); > - return ret; > - } > - wiz->input_clks[WIZ_EXT_REFCLK] = clk; > - > + clk = wiz->input_clks[WIZ_EXT_REFCLK]; > rate = clk_get_rate(clk); > if (rate >= 100000000) > regmap_field_write(wiz->pma_cmn_refclk_mode, 0x0); > else > regmap_field_write(wiz->pma_cmn_refclk_mode, 0x2); > > + /* What follows is about registering clocks. */ > + if (!probe) > + return 0; > + > switch (wiz->type) { > case AM64_WIZ_10G: > case J7200_WIZ_10G: > @@ -1592,7 +1604,7 @@ static int wiz_probe(struct platform_device *pdev) > goto err_get_sync; > } > > - ret = wiz_clock_init(wiz, node); > + ret = wiz_clock_init(wiz, node, true); > if (ret < 0) { > dev_warn(dev, "Failed to initialize clocks\n"); > goto err_get_sync; > > -- > 2.39.2 >
diff --git a/drivers/phy/ti/phy-j721e-wiz.c b/drivers/phy/ti/phy-j721e-wiz.c index fc3cd98c60ff..09f7edf16562 100644 --- a/drivers/phy/ti/phy-j721e-wiz.c +++ b/drivers/phy/ti/phy-j721e-wiz.c @@ -1076,7 +1076,7 @@ static int wiz_clock_register(struct wiz *wiz) return ret; } -static int wiz_clock_init(struct wiz *wiz, struct device_node *node) +static int wiz_clock_init(struct wiz *wiz, struct device_node *node, bool probe) { const struct wiz_clk_mux_sel *clk_mux_sel = wiz->clk_mux_sel; struct device *dev = wiz->dev; @@ -1087,14 +1087,36 @@ static int wiz_clock_init(struct wiz *wiz, struct device_node *node) int ret; int i; - clk = devm_clk_get(dev, "core_ref_clk"); - if (IS_ERR(clk)) { - dev_err(dev, "core_ref_clk clock not found\n"); - ret = PTR_ERR(clk); - return ret; + if (probe) { + clk = devm_clk_get(dev, "core_ref_clk"); + if (IS_ERR(clk)) { + dev_err(dev, "core_ref_clk clock not found\n"); + ret = PTR_ERR(clk); + return ret; + } + wiz->input_clks[WIZ_CORE_REFCLK] = clk; + + if (wiz->data->pma_cmn_refclk1_int_mode) { + clk = devm_clk_get(dev, "core_ref1_clk"); + if (IS_ERR(clk)) { + dev_err(dev, "core_ref1_clk clock not found\n"); + ret = PTR_ERR(clk); + return ret; + } + wiz->input_clks[WIZ_CORE_REFCLK1] = clk; + } + + clk = devm_clk_get(dev, "ext_ref_clk"); + if (IS_ERR(clk)) { + dev_err(dev, "ext_ref_clk clock not found\n"); + ret = PTR_ERR(clk); + return ret; + } + wiz->input_clks[WIZ_EXT_REFCLK] = clk; } - wiz->input_clks[WIZ_CORE_REFCLK] = clk; + + clk = wiz->input_clks[WIZ_CORE_REFCLK]; rate = clk_get_rate(clk); if (rate >= 100000000) regmap_field_write(wiz->pma_cmn_refclk_int_mode, 0x1); @@ -1121,14 +1143,7 @@ static int wiz_clock_init(struct wiz *wiz, struct device_node *node) } if (wiz->data->pma_cmn_refclk1_int_mode) { - clk = devm_clk_get(dev, "core_ref1_clk"); - if (IS_ERR(clk)) { - dev_err(dev, "core_ref1_clk clock not found\n"); - ret = PTR_ERR(clk); - return ret; - } - wiz->input_clks[WIZ_CORE_REFCLK1] = clk; - + clk = wiz->input_clks[WIZ_CORE_REFCLK1]; rate = clk_get_rate(clk); if (rate >= 100000000) regmap_field_write(wiz->pma_cmn_refclk1_int_mode, 0x1); @@ -1136,20 +1151,17 @@ static int wiz_clock_init(struct wiz *wiz, struct device_node *node) regmap_field_write(wiz->pma_cmn_refclk1_int_mode, 0x3); } - clk = devm_clk_get(dev, "ext_ref_clk"); - if (IS_ERR(clk)) { - dev_err(dev, "ext_ref_clk clock not found\n"); - ret = PTR_ERR(clk); - return ret; - } - wiz->input_clks[WIZ_EXT_REFCLK] = clk; - + clk = wiz->input_clks[WIZ_EXT_REFCLK]; rate = clk_get_rate(clk); if (rate >= 100000000) regmap_field_write(wiz->pma_cmn_refclk_mode, 0x0); else regmap_field_write(wiz->pma_cmn_refclk_mode, 0x2); + /* What follows is about registering clocks. */ + if (!probe) + return 0; + switch (wiz->type) { case AM64_WIZ_10G: case J7200_WIZ_10G: @@ -1592,7 +1604,7 @@ static int wiz_probe(struct platform_device *pdev) goto err_get_sync; } - ret = wiz_clock_init(wiz, node); + ret = wiz_clock_init(wiz, node, true); if (ret < 0) { dev_warn(dev, "Failed to initialize clocks\n"); goto err_get_sync;
For suspend and resume support, wiz_clock_init needs to be called multiple times. Add a parameter to wiz_clock_init to be able to skip clocks registration. Based on the work of Théo Lebrun <theo.lebrun@bootlin.com> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com> --- drivers/phy/ti/phy-j721e-wiz.c | 60 +++++++++++++++++++++++++----------------- 1 file changed, 36 insertions(+), 24 deletions(-)