Re: [PATCH v3 3/3] clk: qcom: add clock controller driver for qca8386/qca8084

[Konrad Dybcio <konrad.dybcio@linaro.org>]

On 11.08.2023 13:49, Jie Luo wrote:
> 
> 
> On 8/10/2023 8:59 PM, Konrad Dybcio wrote:
>> On 10.08.2023 13:54, Luo Jie wrote:
>>> Add clock & reset controller driver for qca8386/qca8084.
>>>
>>> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
>>> ---
>>
>>> +struct qcom_cc {
>>> +    struct qcom_reset_controller reset;
>>> +    struct clk_regmap **rclks;
>>> +    size_t num_rclks;
>>> +};
>> This all, including the probe func, is required because of the MDIO dance,
>> I assume?
>>
>> Commonizing that would make more sense should more clocks like this appear
>> in the future.
>>
> Hi Konrad,
> Thanks for the review.
> the structure qcom_cc is not because of MDIO dance, this is the common structure used by other qcom clock drivers such as gcc-ipq9574 in the probe function.
> 
> seems that we can't make any more commonization on this, please let me know if there is any idea on this.
My main point is, why can't you use qcom_cc_really_probe?

> 
>> [...]
>>
>>> +static struct clk_branch nss_cc_switch_core_clk = {
>>> +    .halt_reg = 0x8,
>>> +    .clkr = {
>>> +        .enable_reg = 0x8,
>>> +        .enable_mask = BIT(0),
>>> +        .hw.init = &(const struct clk_init_data) {
>>> +            .name = "nss_cc_switch_core_clk",
>>> +            .parent_hws = (const struct clk_hw *[]) {
>>> +                &nss_cc_switch_core_clk_src.clkr.hw,
>>> +            },
>>> +            .num_parents = 1,
>>> +            /* Can be disabled in PHY mode for power saving */
>> Well it clearly cannot be disabled if it has the CLK_IS_CRITICAL flag :D
>>
>> What's the "PHY mode" you're talking about?
> Yes, the clock with flag CLK_IS_CRITICAL needs to be enabled, the hardware register needs to be configured to enable it, it is disabled by default.
> 
> this clock branch is necessary for the switch mode that has switch features such routing and fdb forwarding, i will remove the CLK_IS_CRITICAL from the clock that is not needed for the PHY mode, we can enable it later when configuring it as switch mode.
> 
> As for PHY mode, the device works as only PHY device, that includes 4 PHY ports, no switch functions.
It's still not obvious at all what "phy mode" or "switch mode" are.
Are they usecases for a device based on this SoC?

Konrad
> 
>>
>>> +            .flags = CLK_SET_RATE_PARENT | CLK_IS_CRITICAL,
>>> +            .ops = &clk_branch2_mdio_ops,
>>> +        },
>>> +    },
>>> +};
>> I see a whole bunch of CRITICAL clocks.. please make sure these clocks
>> are actually necessary for Linux to know about (i.e. if we don't need
>> to call any operations on them, we might just skip registering them
>> with the driver).
>>
>> Konrad
>>
> yes, Konrad, the clocks with  CLK_IS_CRITICAL need to be enabled manually, all clocks are disabled after reset(by default).