Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
Message-ID: <9952fa20-a27f-3240-cc49-5c5109febfc1@quicinc.com>
Date:   Wed, 13 Sep 2023 11:27:25 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.0
Subject: Re: [PATCH v6 4/4] clk: qcom: add clock controller driver for
 qca8386/qca8084
Content-Language: en-US
To:     Stephen Boyd <sboyd@kernel.org>, <agross@kernel.org>,
        <andersson@kernel.org>, <catalin.marinas@arm.com>,
        <conor+dt@kernel.org>, <konrad.dybcio@linaro.org>,
        <krzysztof.kozlowski+dt@linaro.org>, <mturquette@baylibre.com>,
        <p.zabel@pengutronix.de>, <robh+dt@kernel.org>, <will@kernel.org>
CC:     <linux-arm-msm@vger.kernel.org>, <linux-clk@vger.kernel.org>,
        <devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <quic_srichara@quicinc.com>
References: <20230901091823.30242-1-quic_luoj@quicinc.com>
 <20230901091823.30242-5-quic_luoj@quicinc.com>
 <27ae3297ad161fd67706db70b402db04.sboyd@kernel.org>
 <16d09acf-7bdd-04ee-6faf-936c0366df03@quicinc.com>
 <17681a9f756cc70a190c674c51b90140.sboyd@kernel.org>
 <5a4805f7-f802-b1ba-9804-59c0fe6c7f26@quicinc.com>
 <92058c25fb11b75ee0a2298a684825e9.sboyd@kernel.org>
 <f67b354c-8a4b-49f5-6275-66b7d614301a@quicinc.com>
 <82adb75659e0d278e25b65b0e81df99a.sboyd@kernel.org>
From:   Jie Luo <quic_luoj@quicinc.com>
In-Reply-To: <82adb75659e0d278e25b65b0e81df99a.sboyd@kernel.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 9/13/2023 1:18 AM, Stephen Boyd wrote:
> Quoting Jie Luo (2023-09-12 05:07:02)
>>
>>
>> On 9/12/2023 4:11 AM, Stephen Boyd wrote:
>>> Quoting Jie Luo (2023-09-08 04:10:35)
>>>>
>>>> For example, when the uniphy works on PHY_INTERFACE_MODE_2500BASEX, then
>>>> the parent uniphy clock rate is 312.5M, which is decided by hardware and
>>>> can't be changed. when a branch clock requires a 25M clock, the parent
>>>> uniphy clock maybe updated to 125M by clock framework if the flag
>>>> CLK_SET_RATE_PARENT is set here, but the actual hardware clock rate of
>>>> uniphy is still 315.5M since the uniphy still works in the interface
>>>> mode PHY_INTERFACE_MODE_2500BASEX.
>>>>
>>>
>>> If the parent rate can't change because CLK_SET_RATE_PARENT is missing
>>> and the hardware doesn't allow it, then perhaps instead of having a
>>> frequency table we should have rcg clk ops for determine_rate that
>>> simply looks at the parent rates and finds the rate closest to what is
>>> desired. And for the set_rate clk_op we can have it be simple and just
>>> program a fixed divider. The benefit is less frequency tables that don't
>>> do anything and less hard-coding of the frequency. I thought we already
>>> had those rcg clk_ops but I couldn't find them with a quick glance.
>>
>> Thanks Stephen for the suggestion.
>> looks you are saying the clk ops clk_dp_ops for the fix parent rate?
>> which seems not meet the clock requirement of this clock.
> 
> Yeah that is close, but the determine_rate clk_op needs to look at all
> possible parents. With the dp clk_ops we assume that only one parent is
> possible.
> 
>>
>> For the device qca8k, it is also possible to switch the interface modes
>> between PHY_INTERFACE_MODE_2500BASEX(312.5M) and
>> PHY_INTERFACE_MODE_SGMII(125M) during the running time, and there are
>> multiple parent clock source(P_UNIPHY0_RX or P_UNIPHY0_TX) for the RCG
>> clocks to select according to the current work mode. so the parent_map
>> and freq_tbl are necessary to this clock.
> 
> I still don't see why the freq_tbl is necessary.

Hi Stephen,
For clk_rcg2_ops, freq_tbl is used to find the closest rate to decided 
the parent clock, the configuration of clock source and clock divider 
are saved in the freq_tbl to configure the RCG hardware register, the 
mapping of parent clock and hardware register value is decided by the 
freq_tbl for the RCG clock.