[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <ad1f6ea5-f506-22f7-1f88-0291167fb7fd@amlogic.com>
Date: Tue, 31 Jan 2023 11:29:04 +0800
From: Yu Tu <yu.tu@...ogic.com>
To: Jerome Brunet <jbrunet@...libre.com>, <linux-clk@...r.kernel.org>,
<linux-arm-kernel@...ts.infradead.org>,
<linux-amlogic@...ts.infradead.org>,
<linux-kernel@...r.kernel.org>, <devicetree@...r.kernel.org>,
Rob Herring <robh+dt@...nel.org>,
Neil Armstrong <neil.armstrong@...aro.org>,
Kevin Hilman <khilman@...libre.com>,
Michael Turquette <mturquette@...libre.com>,
Stephen Boyd <sboyd@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Martin Blumenstingl <martin.blumenstingl@...glemail.com>
CC: "kelvin . zhang" <Kelvin.Zhang@...ogic.com>,
"qi . duan" <qi.duan@...ogic.com>
Subject: Re: [PATCH V6 3/3] clk: meson: s4: add support for Amlogic S4 SoC
peripheral clock controller
On 2023/1/30 18:07, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
>
>
> On Mon 30 Jan 2023 at 17:59, Yu Tu <yu.tu@...ogic.com> wrote:
>
>> On 2023/1/30 17:47, Jerome Brunet wrote:
>>> [ EXTERNAL EMAIL ]
>>> On Mon 30 Jan 2023 at 17:41, Yu Tu <yu.tu@...ogic.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@...ogic.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@...ogic.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@...ogic.com> wrote:
>>>>>>>>>
>>>>>>>>>> Add the peripherals clock controller driver in the s4 SoC family.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Yu Tu <yu.tu@...ogic.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.
>>>
>>
>> The CLKCTRL_VID_PLL_CLK_DIV register is actually described, but it is not
>> used in the actual board. According to your reply just now, description is
>> required, but I want to know how to describe it to meet your requirements.
>>
>> Please give me some suggestions.
>
> Implementing things is NOT about usage, it is about correctness.
> Either there is actually a clock in the silicon you are producing at the
> Amlogic factory, or there is not.
>
> If the clock is there in the actual HW should be properly
> described/implemented, as it "might" be used as an input to other clocks
> - even if you personnaly don't.
>
> If clock does not exists (nothing behind the registers, or broken, etc
> ...) then, yes you'll need to use parent tables and document this.
>
According to your suggestion, we need to describe the clock
(vid_pll_div).So it seems like we need to implement
"meson_vid_pll_div_ops" and do a commit first. Then submit the S4 SOC
clock driver. So change whether you take it or not?
Or if you have a better idea, let me know.
Powered by blists - more mailing lists