Re: [PATCH v2 5/5] clk: scmi: register scmi clocks with CCF

[Sean Anderson <seanga2@gmail.com>]

On 3/7/22 5:17 AM, Etienne Carriere wrote:
> Hello Sean,
> 
> Thanks for the feedback.
> Sorry I missed your post end Feb.
> 
> On Fri, 25 Feb 2022 at 07:33, Sean Anderson <seanga2@gmail.com> wrote:
>>
>> Hi Etienne,
>>
>>
>> On 2/21/22 3:22 AM, Etienne Carriere wrote:
>>> Implements SCMI APIs to retrieve the number exposed SCMI clocks using
>>> SCMI_PROTOCOL_ATTRIBUTES messages and the names of the clocks using
>>> SCMI_CLOCK_ATTRIBUTES messages.
>>>
>>> This change updates sandbox SCMI clock test driver to manage these
>>> 2 new message IDs.
>>>
>>> Cc: Lukasz Majewski <lukma@denx.de>
>>> Cc: Sean Anderson <seanga2@gmail.com>
>>> Cc: Clement Leger <clement.leger@bootlin.com>
>>> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
>>> Reviewed-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
>>> Signed-off-by: Gabriel Fernandez <gabriel.fernandez@st.com>
>>> Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
>>> ---
>>> Changes since v1:
>>> - Remove Series-links tag and apply R-b tag.
>>> ---
>>>    drivers/clk/clk_scmi.c                     | 90 ++++++++++++++++++++++
>>>    drivers/firmware/scmi/sandbox-scmi_agent.c | 53 +++++++++++++
>>>    include/scmi_protocols.h                   | 43 +++++++++++
>>>    3 files changed, 186 insertions(+)
>>>
>>> diff --git a/drivers/clk/clk_scmi.c b/drivers/clk/clk_scmi.c
>>> index 42fbab0d21..57022685e2 100644
>>> --- a/drivers/clk/clk_scmi.c
>>> +++ b/drivers/clk/clk_scmi.c
>>> @@ -11,6 +11,52 @@
>>>    #include <scmi_agent.h>
>>>    #include <scmi_protocols.h>
>>>    #include <asm/types.h>
>>> +#include <linux/clk-provider.h>
>>> +
>>> +static int scmi_clk_get_num_clock(struct udevice *dev, size_t *num_clocks)
>>> +{
>>> +     struct scmi_clk_protocol_attr_out out;
>>> +     struct scmi_msg msg = {
>>> +             .protocol_id = SCMI_PROTOCOL_ID_CLOCK,
>>> +             .message_id = SCMI_PROTOCOL_ATTRIBUTES,
>>> +             .out_msg = (u8 *)&out,
>>> +             .out_msg_sz = sizeof(out),
>>> +     };
>>> +     int ret;
>>> +
>>> +     ret = devm_scmi_process_msg(dev, &msg);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     *num_clocks = out.attributes & SCMI_CLK_PROTO_ATTR_COUNT_MASK;
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static int scmi_clk_get_attibute(struct udevice *dev, int clkid, char **name)
>>
>> nit: scmi_clk_get_attribute
> 
> right!
> 
>>
>>> +{
>>> +     struct scmi_clk_attribute_in in = {
>>> +             .clock_id = clkid,
>>> +     };
>>> +     struct scmi_clk_attribute_out out;
>>> +     struct scmi_msg msg = {
>>> +             .protocol_id = SCMI_PROTOCOL_ID_CLOCK,
>>> +             .message_id = SCMI_CLOCK_ATTRIBUTES,
>>> +             .in_msg = (u8 *)&in,
>>> +             .in_msg_sz = sizeof(in),
>>> +             .out_msg = (u8 *)&out,
>>> +             .out_msg_sz = sizeof(out),
>>> +     };
>>> +     int ret;
>>> +
>>> +     ret = devm_scmi_process_msg(dev, &msg);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     *name = out.clock_name;
>>> +
>>> +     return 0;
>>> +}
>>>
>>>    static int scmi_clk_gate(struct clk *clk, int enable)
>>>    {
>>> @@ -88,6 +134,49 @@ static ulong scmi_clk_set_rate(struct clk *clk, ulong rate)
>>>        return scmi_clk_get_rate(clk);
>>>    }
>>>
>>> +static int scmi_clk_probe(struct udevice *dev)
>>> +{
>>> +     struct clk *clk;
>>> +     size_t num_clocks, i;
>>> +     int ret;
>>> +
>>> +     if (!CONFIG_IS_ENABLED(CLK_CCF))
>>> +             return 0;
>>> +
>>> +     /* register CCF children: CLK UCLASS, no probed again */
>>> +     if (device_get_uclass_id(dev->parent) == UCLASS_CLK)
>>> +             return 0;
>>> +
>>> +     ret = scmi_clk_get_num_clock(dev, &num_clocks);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     for (i = 0; i < num_clocks; i++) {
>>> +             char *name;
>>> +
>>> +             if (!scmi_clk_get_attibute(dev, i, &name)) {
>>> +                     char *clock_name = strdup(name);
>>> +
>>> +                     clk = kzalloc(sizeof(*clk), GFP_KERNEL);
>>> +                     if (!clk || !clock_name)
>>> +                             ret = -ENOMEM;
>>> +                     else
>>> +                             ret = clk_register(clk, dev->driver->name,
>>> +                                                clock_name, dev->name);
>>> +
>>> +                     if (ret) {
>>> +                             free(clk);
>>> +                             free(clock_name);
>>> +                             return ret;
>>> +                     }
>>> +
>>> +                     clk_dm(i, clk);
>>> +             }
>>> +     }
>>> +
>>> +     return 0;
>>> +}
>>> +
>>
>> So why not call scmi_clk_get_num_clock during probe() and then check against
>> it in xlate? I don't really see why we need CCF. This also means you don't
>> have a driver with copies of itself as children, and is lighter on memory.
> 
> There is no specific need for CCF for the scmi clock driver itself.
> The goal of these changes is rather to allow platforms that do already
> enable CCF (for an u-boot native clock driver)
> to be able to also embed SCMI clocks support, for clocks controlled
> from an external firmware.

Sure, but you can still use non-CCF clocks in a CCF clock.

--Sean