[PATCH V6 3/3] clk: meson: s4: add support for Amlogic S4 SoC peripheral clock controller

[Yu Tu]

On 2023/1/30 18:07, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
> 
> 
> On Mon 30 Jan 2023 at 17:59, Yu Tu <yu.tu at amlogic.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 at 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 at 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 at 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 at amlogic.com> wrote:
>>>>>>>>>
>>>>>>>>>> Add the peripherals clock controller driver in the s4 SoC family.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Yu Tu <yu.tu at 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.
>>>
>>
>> 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.