Re: [PATCH v3 2/4] clk: qcom: Add CMN PLL clock controller driver for IPQ SoC

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux