| Message ID | 20240102-j7200-pcie-s2r-v3-8-5c2e4a3fac1f@bootlin.com |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Add suspend to ram support for PCIe on J7200 | expand |
On Thu, Feb 15, 2024 at 04:17:53PM +0100, Thomas Richard wrote: > The wiz_clock_init() function mixes probe and hardware configuration. > Rename the wiz_clock_init() to wiz_clock_probe() and move the hardware > configuration part in a new function named wiz_clock_init(). > > This hardware configuration sequence must be called during the resume > stage of the driver. ... (Side note, as this can be done later) > if (rate >= 100000000) > + if (rate >= 100000000) > + if (rate >= 100000000) I would make local definition and use it, we may get the global one as there are users. #define HZ_PER_GHZ 1000000000UL
On 15-02-24, 17:43, Andy Shevchenko wrote: > On Thu, Feb 15, 2024 at 04:17:53PM +0100, Thomas Richard wrote: > > The wiz_clock_init() function mixes probe and hardware configuration. > > Rename the wiz_clock_init() to wiz_clock_probe() and move the hardware > > configuration part in a new function named wiz_clock_init(). > > > > This hardware configuration sequence must be called during the resume > > stage of the driver. > > ... > > (Side note, as this can be done later) > > > if (rate >= 100000000) > > > + if (rate >= 100000000) > > > + if (rate >= 100000000) > > I would make local definition and use it, we may get the global one as there > are users. > > #define HZ_PER_GHZ 1000000000UL Better to define as: #define HZ_PER_GHZ 1 * GIGA
On 24/02/16 11:32AM, Vinod Koul wrote: > On 15-02-24, 17:43, Andy Shevchenko wrote: > > On Thu, Feb 15, 2024 at 04:17:53PM +0100, Thomas Richard wrote: > > > The wiz_clock_init() function mixes probe and hardware configuration. > > > Rename the wiz_clock_init() to wiz_clock_probe() and move the hardware > > > configuration part in a new function named wiz_clock_init(). > > > > > > This hardware configuration sequence must be called during the resume > > > stage of the driver. > > > > ... > > > > (Side note, as this can be done later) > > > > > if (rate >= 100000000) > > > > > + if (rate >= 100000000) > > > > > + if (rate >= 100000000) > > > > I would make local definition and use it, we may get the global one as there > > are users. > > > > #define HZ_PER_GHZ 1000000000UL > > Better to define as: > #define HZ_PER_GHZ 1 * GIGA The variable "rate" is being compared against 100 MHz and not 1 GHz. The driver already has the following macros defined: #define REF_CLK_19_2MHZ 19200000 #define REF_CLK_25MHZ 25000000 #define REF_CLK_100MHZ 100000000 #define REF_CLK_156_25MHZ 156250000 So would it be acceptable to change it to: if (rate >= REF_CLK_100MHZ) instead? Regards, Siddharth.
On Fri, Feb 16, 2024 at 11:32:31AM +0530, Vinod Koul wrote: > On 15-02-24, 17:43, Andy Shevchenko wrote: > > On Thu, Feb 15, 2024 at 04:17:53PM +0100, Thomas Richard wrote: ... > > (Side note, as this can be done later) > > > > > if (rate >= 100000000) > > > > > + if (rate >= 100000000) > > > > > + if (rate >= 100000000) > > > > I would make local definition and use it, we may get the global one as there > > are users. > > > > #define HZ_PER_GHZ 1000000000UL > > Better to define as: > #define HZ_PER_GHZ 1 * GIGA (with parentheses) Maybe here, but when it appears in units.h it will be defined as I wrote to be aligned with the rest of definitions.
On Fri, Feb 16, 2024 at 02:34:39PM +0530, Siddharth Vadapalli wrote: > On 24/02/16 11:32AM, Vinod Koul wrote: > > On 15-02-24, 17:43, Andy Shevchenko wrote: > > > On Thu, Feb 15, 2024 at 04:17:53PM +0100, Thomas Richard wrote: ... > > > (Side note, as this can be done later) > > > > > > > if (rate >= 100000000) > > > > > > > + if (rate >= 100000000) > > > > > > > + if (rate >= 100000000) > > > > > > I would make local definition and use it, we may get the global one as there > > > are users. > > > > > > #define HZ_PER_GHZ 1000000000UL > > > > Better to define as: > > #define HZ_PER_GHZ 1 * GIGA > > The variable "rate" is being compared against 100 MHz and not 1 GHz. Extremely good point why constant definitions are better (to avoid missing or extra 0, etc)! > The driver already has the following macros defined: > #define REF_CLK_19_2MHZ 19200000 > #define REF_CLK_25MHZ 25000000 > #define REF_CLK_100MHZ 100000000 > #define REF_CLK_156_25MHZ 156250000 > > So would it be acceptable to change it to: > if (rate >= REF_CLK_100MHZ) > instead? Sounds like a good idea to me.
diff --git a/drivers/phy/ti/phy-j721e-wiz.c b/drivers/phy/ti/phy-j721e-wiz.c index ce8a99801a4c..45c5a4e9cd12 100644 --- a/drivers/phy/ti/phy-j721e-wiz.c +++ b/drivers/phy/ti/phy-j721e-wiz.c @@ -1076,25 +1076,11 @@ static int wiz_clock_register(struct wiz *wiz) return ret; } -static int wiz_clock_init(struct wiz *wiz, struct device_node *node) +static void wiz_clock_init(struct wiz *wiz) { - const struct wiz_clk_mux_sel *clk_mux_sel = wiz->clk_mux_sel; - struct device *dev = wiz->dev; - struct device_node *clk_node; - const char *node_name; unsigned long rate; - struct clk *clk; - int ret; - int i; - - clk = devm_clk_get(dev, "core_ref_clk"); - if (IS_ERR(clk)) - return dev_err_probe(dev, PTR_ERR(clk), - "core_ref_clk clock not found\n"); - - wiz->input_clks[WIZ_CORE_REFCLK] = clk; - rate = clk_get_rate(clk); + rate = clk_get_rate(wiz->input_clks[WIZ_CORE_REFCLK]); if (rate >= 100000000) regmap_field_write(wiz->pma_cmn_refclk_int_mode, 0x1); else @@ -1119,6 +1105,39 @@ static int wiz_clock_init(struct wiz *wiz, struct device_node *node) break; } + if (wiz->input_clks[WIZ_CORE_REFCLK1]) { + rate = clk_get_rate(wiz->input_clks[WIZ_CORE_REFCLK1]); + if (rate >= 100000000) + regmap_field_write(wiz->pma_cmn_refclk1_int_mode, 0x1); + else + regmap_field_write(wiz->pma_cmn_refclk1_int_mode, 0x3); + + } + + rate = clk_get_rate(wiz->input_clks[WIZ_EXT_REFCLK]); + if (rate >= 100000000) + regmap_field_write(wiz->pma_cmn_refclk_mode, 0x0); + else + regmap_field_write(wiz->pma_cmn_refclk_mode, 0x2); +} + +static int wiz_clock_probe(struct wiz *wiz, struct device_node *node) +{ + const struct wiz_clk_mux_sel *clk_mux_sel = wiz->clk_mux_sel; + struct device *dev = wiz->dev; + struct device_node *clk_node; + const char *node_name; + struct clk *clk; + int ret; + int i; + + clk = devm_clk_get(dev, "core_ref_clk"); + if (IS_ERR(clk)) + return dev_err_probe(dev, PTR_ERR(clk), + "core_ref_clk clock not found\n"); + + 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)) @@ -1126,12 +1145,6 @@ static int wiz_clock_init(struct wiz *wiz, struct device_node *node) "core_ref1_clk clock not found\n"); wiz->input_clks[WIZ_CORE_REFCLK1] = clk; - - rate = clk_get_rate(clk); - if (rate >= 100000000) - regmap_field_write(wiz->pma_cmn_refclk1_int_mode, 0x1); - else - regmap_field_write(wiz->pma_cmn_refclk1_int_mode, 0x3); } clk = devm_clk_get(dev, "ext_ref_clk"); @@ -1141,11 +1154,7 @@ static int wiz_clock_init(struct wiz *wiz, struct device_node *node) wiz->input_clks[WIZ_EXT_REFCLK] = clk; - 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); + wiz_clock_init(wiz); switch (wiz->type) { case AM64_WIZ_10G: @@ -1589,7 +1598,7 @@ static int wiz_probe(struct platform_device *pdev) goto err_get_sync; } - ret = wiz_clock_init(wiz, node); + ret = wiz_clock_probe(wiz, node); if (ret < 0) { dev_warn(dev, "Failed to initialize clocks\n"); goto err_get_sync;
The wiz_clock_init() function mixes probe and hardware configuration. Rename the wiz_clock_init() to wiz_clock_probe() and move the hardware configuration part in a new function named wiz_clock_init(). This hardware configuration sequence must be called during the resume stage of the driver. Signed-off-by: Thomas Richard <thomas.richard@bootlin.com> --- drivers/phy/ti/phy-j721e-wiz.c | 65 ++++++++++++++++++++++++------------------ 1 file changed, 37 insertions(+), 28 deletions(-)