Re: [PATCH v4 7/7] clk: qcom: add the driver for the MSM8996 APCS clocks

[Konrad Dybcio]

On 25.01.2023 22:56, Stephen Boyd wrote:
> Quoting Konrad Dybcio (2023-01-25 13:48:54)
>>
>>
>> On 25.01.2023 22:38, Stephen Boyd wrote:
>>> Quoting Dmitry Baryshkov (2023-01-18 05:22:54)
>>>> diff --git a/drivers/clk/qcom/apcs-msm8996.c b/drivers/clk/qcom/apcs-msm8996.c
>>>> new file mode 100644
>>>> index 000000000000..7e46ea8ed444
>>>> --- /dev/null
>>>> +++ b/drivers/clk/qcom/apcs-msm8996.c
>>>> @@ -0,0 +1,76 @@
>>> [...]
>>>> +
>>>> +static int qcom_apcs_msm8996_clk_probe(struct platform_device *pdev)
>>>> +{
>>>> +       struct device *dev = &pdev->dev;
>>>> +       struct device *parent = dev->parent;
>>>> +       struct regmap *regmap;
>>>> +       struct clk_hw *hw;
>>>> +       unsigned int val;
>>>> +       int ret = -ENODEV;
>>>> +
>>>> +       regmap = dev_get_regmap(parent, NULL);
>>>> +       if (!regmap) {
>>>> +               dev_err(dev, "failed to get regmap: %d\n", ret);
>>>> +               return ret;
>>>> +       }
>>>> +
>>>> +       regmap_read(regmap, APCS_AUX_OFFSET, &val);
>>>> +       regmap_update_bits(regmap, APCS_AUX_OFFSET, APCS_AUX_DIV_MASK,
>>>> +                          FIELD_PREP(APCS_AUX_DIV_MASK, APCS_AUX_DIV_2));
>>>> +
>>>> +       /* Hardware mandated delay */
>>>
>>> Delay for what? Setting the divider? What if the register value didn't
>>> change at all? Can you skip the delay in that case?
>> Waiting 5 us unconditionally in exchange for ensured CPU clock
>> source stability sounds like a rather fair deal.. Checking if
>> the register value changed would not save us much time..
> 
> So it is waiting for the CPU clk to be stable? The comment is not clear.
Okay, so perhaps this is just a misunderstanding because of a lackluster
comment.. This SYS_APCS_AUX (provided by this driver) is one of the CPU
clock sources (and probably the "safest" of them all, as it's fed by
GPLL0 and not the CPU PLLs) the delay is there to ensure it can
stabilize after setting the divider to DIV2. In a theoretical case, the
big 8996 cpucc driver could select this clock as a target for one (or
both) of the per-cluster muxes and it could put the CPUs in a weird state.

As unlikely as that would be, especially considering 8996 (AFAIK) doesn't
use this clock source coming out of reset / bootloader, this lets us
ensure one less thing can break.

Konrad