On Tue, 3 Sept 2024 at 17:00, Jie Luo <quic_luoj@xxxxxxxxxxx> wrote: > > > > On 9/3/2024 2:39 AM, Dmitry Baryshkov wrote: > > On Mon, Sep 02, 2024 at 11:33:57PM GMT, Jie Luo wrote: > >> > >> > >> On 8/31/2024 6:24 AM, Stephen Boyd wrote: > >>> Quoting Jie Luo (2024-08-30 09:14:28) > >>>> 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. > >> > >>> > >>>> On 8/28/2024 1:44 PM, Jie Luo wrote: > >>>>> On 8/28/2024 7:50 AM, Stephen Boyd wrote: > >>>>>> Quoting Luo Jie (2024-08-27 05:46:00) > >>>>>>> + 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
