References: <20230116074214.2326-1-yu.tu@amlogic.com>
 <20230116074214.2326-4-yu.tu@amlogic.com>
 <1ja62eybrv.fsf@starbuckisacylon.baylibre.com>
 <aedb0764-b5cb-7f49-f279-51dbec070e80@amlogic.com>
 <1jwn5hwn0w.fsf@starbuckisacylon.baylibre.com>
 <a4ad6ac6-60c2-8f7b-fdb0-509de31db282@amlogic.com>
 <1jy1pko0fc.fsf@starbuckisacylon.baylibre.com>
 <e2e9045a-6e35-112f-69a7-15b080571b69@amlogic.com>
User-agent: mu4e 1.8.10; emacs 28.2
From:   Jerome Brunet <jbrunet@baylibre.com>
To:     Yu Tu <yu.tu@amlogic.com>, linux-clk@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org,
        linux-amlogic@lists.infradead.org, linux-kernel@vger.kernel.org,
        devicetree@vger.kernel.org, Rob Herring <robh+dt@kernel.org>,
        Neil Armstrong <neil.armstrong@linaro.org>,
        Kevin Hilman <khilman@baylibre.com>,
        Michael Turquette <mturquette@baylibre.com>,
        Stephen Boyd <sboyd@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Cc:     "kelvin . zhang" <Kelvin.Zhang@amlogic.com>,
        "qi . duan" <qi.duan@amlogic.com>
Subject: Re: [PATCH V6 3/3] clk: meson: s4: add support for Amlogic S4 SoC
 peripheral clock controller
Date:   Mon, 30 Jan 2023 10:47:04 +0100
In-reply-to: <e2e9045a-6e35-112f-69a7-15b080571b69@amlogic.com>
Message-ID: <1jr0vcnyf7.fsf@starbuckisacylon.baylibre.com>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk


On Mon 30 Jan 2023 at 17:41, Yu Tu <yu.tu@amlogic.com> wrote:

> On 2023/1/30 17:06, Jerome Brunet wrote:
>> [ EXTERNAL EMAIL ]
>> On Sat 28 Jan 2023 at 18:17, Yu Tu <yu.tu@amlogic.com> wrote:
>> 
>>> On 2023/1/20 17:47, Jerome Brunet wrote:
>>>> [ EXTERNAL EMAIL ]
>>>> On Fri 20 Jan 2023 at 11:33, Yu Tu <yu.tu@amlogic.com> wrote:
>>>>
>>>>> Hi
>>>>> On 2023/1/19 19:37, Jerome Brunet wrote:
>>>>>> [ EXTERNAL EMAIL ]
>>>>>> On Mon 16 Jan 2023 at 15:42, Yu Tu <yu.tu@amlogic.com> wrote:
>>>>>>
>>>>>>> Add the peripherals clock controller driver in the s4 SoC family.
>>>>>>>
>>>>>>> Signed-off-by: Yu Tu <yu.tu@amlogic.com>
>>>>>> [...]
>>>>>>
>>>>>>> +
>>>>>>> +/* Video Clocks */
>>>>>>> +static struct clk_regmap s4_vid_pll_div = {
>>>>>>> +	.data = &(struct meson_vid_pll_div_data){
>>>>>>> +		.val = {
>>>>>>> +			.reg_off = CLKCTRL_VID_PLL_CLK_DIV,
>>>>>>> +			.shift   = 0,
>>>>>>> +			.width   = 15,
>>>>>>> +		},
>>>>>>> +		.sel = {
>>>>>>> +			.reg_off = CLKCTRL_VID_PLL_CLK_DIV,
>>>>>>> +			.shift   = 16,
>>>>>>> +			.width   = 2,
>>>>>>> +		},
>>>>>>> +	},
>>>>>>> +	.hw.init = &(struct clk_init_data) {
>>>>>>> +		.name = "vid_pll_div",
>>>>>>> +		/*
>>>>>>> +		 * The frequency division from the hdmi_pll clock to the vid_pll_div
>>>>>>> +		 * clock is the default value of this register. When designing the
>>>>>>> +		 * video module of the chip, a default value that can meet the
>>>>>>> +		 * requirements of the video module will be solidified according
>>>>>>> +		 * to the usage requirements of the chip, so as to facilitate chip
>>>>>>> +		 * simulation. So this is ro_ops.
>>>>>>> +		 * It is important to note that this clock is not used on this
>>>>>>> +		 * chip and is described only for the integrity of the clock tree.
>>>>>>> +		 */
>>>>>> If it is reset value and will be applicable to all the design, regarless
>>>>>> of the use-case, then yes RO ops is OK
>>>>>>
>>>>>> >From what I understand here, the value will depend on the use-case requirements.
>>>>>> This is a typical case where the DT prop "assigned-rate" should be used, not RO ops.
>>>>>
>>>>> Check the previous chip history, the actual scene is not used at all,
>>>>> basically is used in simulation. So the previous SOC was "ro_ops" without
>>>>> any problems.  This S4 SOC is not actually useful either.
>>>>>
>>>>> So when you were upstream, you had no problem making "ro_ops". I wonder if
>>>>> I could delete this useless clock, so you don't have to worry about it.
>>>> I don't know what to make of this. What is the point of adding a useless
>>>> clock ?
>>>
>>> As explained earlier this "vid_pll_div" is actually used in chip
>>> emulation. So next I'd like to know what you suggest to do with the clock?
>>>
>> If it does not exist in the actual SoC, please remove it
>> 
>
> If I remove it, the "vid_pll_sel" clock will be missing a parent
> (vid_pll_div). I will use the table method and give the above reasons. Do
> you accept this method?

Either the clock exists or it does not.

If the HW actually exist, it is expected to be properly described.
If it does not, it obviously cannot be an input to another clock.

Please sort this out and make the necessary changes.