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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <631d5a82-7d24-49cd-9868-74c7b3c08bcd@linaro.org>
Date:   Sat, 12 Aug 2023 12:56:06 +0200
From:   Konrad Dybcio <konrad.dybcio@...aro.org>
To:     Jie Luo <quic_luoj@...cinc.com>, andersson@...nel.org,
        agross@...nel.org, mturquette@...libre.com, sboyd@...nel.org,
        robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org,
        conor+dt@...nel.org, catalin.marinas@....com, will@...nel.org,
        p.zabel@...gutronix.de
Cc:     linux-arm-msm@...r.kernel.org, linux-clk@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        quic_srichara@...cinc.com
Subject: Re: [PATCH v3 3/3] clk: qcom: add clock controller driver for
 qca8386/qca8084

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@...cinc.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).