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

From: Yu Tu
Date: Mon Jan 30 2023 - 05:01:07 EST




On 2023/1/30 17:47, Jerome Brunet wrote:
[ EXTERNAL EMAIL ]


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

On 2023/1/30 17:06, Jerome Brunet wrote:
[ EXTERNAL EMAIL ]
On Sat 28 Jan 2023 at 18:17, Yu Tu <yu.tu@xxxxxxxxxxx> wrote:

On 2023/1/20 17:47, Jerome Brunet wrote:
[ EXTERNAL EMAIL ]
On Fri 20 Jan 2023 at 11:33, Yu Tu <yu.tu@xxxxxxxxxxx> 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@xxxxxxxxxxx> wrote:

Add the peripherals clock controller driver in the s4 SoC family.

Signed-off-by: Yu Tu <yu.tu@xxxxxxxxxxx>
[...]

+
+/* 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.