On Tue, 3 Sept 2024 at 17:00, Jie Luo [off-list ref] wrote:

On 9/3/2024 2:39 AM, Dmitry Baryshkov wrote:

quoted

On Mon, Sep 02, 2024 at 11:33:57PM GMT, Jie Luo wrote:

quoted

On 8/31/2024 6:24 AM, Stephen Boyd wrote:

quoted

Quoting Jie Luo (2024-08-30 09:14:28)

quoted

Hi Stephen,
Please find below a minor update to my earlier message on clk_ops usage.

Ok. Next time you can trim the reply to save me time.

OK.

quoted

quoted

On 8/28/2024 1:44 PM, Jie Luo wrote:

quoted

On 8/28/2024 7:50 AM, Stephen Boyd wrote:

quoted

Quoting Luo Jie (2024-08-27 05:46:00)

quoted

+       case 48000000:
+               val |= FIELD_PREP(CMN_PLL_REFCLK_INDEX, 7);
+               break;
+       case 50000000:
+               val |= FIELD_PREP(CMN_PLL_REFCLK_INDEX, 8);
+               break;
+       case 96000000:
+               val |= FIELD_PREP(CMN_PLL_REFCLK_INDEX, 7);
+               val &= ~CMN_PLL_REFCLK_DIV;
+               val |= FIELD_PREP(CMN_PLL_REFCLK_DIV, 2);
+               break;
+       default:
+               return -EINVAL;
+       }

Why isn't this done with struct clk_ops::set_rate() or clk_ops::init()?

OK, I will move this code into the clk_ops::init().

This code is expected to be executed once for initializing the CMN PLL
to enable output clocks, and requires the parent clock rate to be
available. However the parent clock rate is not available in the
clk_ops::init(). Hence clk_ops::set_rate() seems to be the right option
for this. Please let us know if this approach is fine. Thanks.

Sure. It actually sounds like the PLL has a mux to select different
reference clks. Is that right? If so, it seems like there should be
multiple 'clocks' for the DT property and many parents possible. If
that's the case then it should be possible to have something like

    clocks = <0>, <&refclk>, <0>;

in the DT node and then have clk_set_rate() from the consumer actually
set the parent index in hardware. If that's all static then it can be
done with assigned-clock-parents or assigned-clock-rates.

Thanks Stephen. The CMN PLL block always uses a single input reference
clock pin on any given IPQ SoC, however its rate may be different on
different IPQ SoC. For example, its rate is 48MHZ on IPQ9574 and 96MHZ
on IPQ5018.

Your second suggestion seems more apt for this device. I can define the
DT property 'assigned-clock-parents' to configure the clock parent of
CMN PLL. The code for reference clock selection will be added in
clk_ops::set_parent(). Please let us know if this approach is fine.

What is the source of this clock? Can you call clk_get_rate() on this
input?

The source (parent clock) for CMN PLL is always from on-board Wi-Fi
block for any given IPQ SoC.

 From the discussion so far, it seems there are two approaches possible
which I would like to summarize below to be clear. Please let us know
if this understanding or approach needs correction. Thanks.

1. clk_get_rate() requires the parent clock instance to be acquired by
devm_clk_get(). Per our understanding from Stephen's previous comment,
it is preferred that a clock provider driver (this) does not use the
_get_ APIs on the parent clock to get the rate. Instead the parent rate
should be passed to the clk_ops using parent data. So the parent clock
should be specified in the DT using assigned-clock-parents property, and
can be accessed from the clk_ops::set_parent(). This seems like a more
reasonable method.

assigned-clock-parents is necessary if there are multiple possible
parents. As you wrote that there is just one possible parent, then
there is no need to use it.
Stephen, your opinion?

2. Alternatively, if it is architecturally acceptable to use
devm_clk_get() and clk_get_rate() in this clock provider driver, we can
save this parent clock rate into a local driver data structure and then
access it from clk_ops::init() for configuring the PLL.

-- 
With best wishes
Dmitry