Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
References: <20230404155332.9571-1-ddrokosov@sberdevices.ru>
 <20230404155332.9571-7-ddrokosov@sberdevices.ru>
User-agent: mu4e 1.8.13; emacs 28.2
From:   Jerome Brunet <jbrunet@baylibre.com>
To:     Dmitry Rokosov <ddrokosov@sberdevices.ru>,
        neil.armstrong@linaro.org, mturquette@baylibre.com,
        sboyd@kernel.org, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, khilman@baylibre.com,
        martin.blumenstingl@googlemail.com
Cc:     jian.hu@amlogic.com, kernel@sberdevices.ru, rockosov@gmail.com,
        linux-amlogic@lists.infradead.org, linux-clk@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v12 6/6] clk: meson: a1: add Amlogic A1 Peripherals
 clock controller driver
Date:   Wed, 05 Apr 2023 16:13:44 +0200
In-reply-to: <20230404155332.9571-7-ddrokosov@sberdevices.ru>
Message-ID: <1j8rf6flk5.fsf@starbuckisacylon.baylibre.com>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk


On Tue 04 Apr 2023 at 18:53, Dmitry Rokosov <ddrokosov@sberdevices.ru> wrote:

> +/* Array of all clocks registered by this provider */
> +static struct clk_hw_onecell_data a1_periphs_clks = {
> +	.hws = {
> +		/* DT exposed clocks */
> +		[CLKID_FIXPLL_IN]		= &fixpll_in.hw,
> +		[CLKID_USB_PHY_IN]		= &usb_phy_in.hw,
> +		[CLKID_USB_CTRL_IN]		= &usb_ctrl_in.hw,
> +		[CLKID_HIFIPLL_IN]		= &hifipll_in.hw,
> +		[CLKID_SYSPLL_IN]		= &syspll_in.hw,
> +		[CLKID_DDS_IN]			= &dds_in.hw,
> +		[CLKID_SYS]			= &sys.hw,
> +		[CLKID_CLKTREE]			= &clktree.hw,
> +		[CLKID_RESET_CTRL]		= &reset_ctrl.hw,
> +		[CLKID_ANALOG_CTRL]		= &analog_ctrl.hw,
> +		[CLKID_PWR_CTRL]		= &pwr_ctrl.hw,
> +		[CLKID_PAD_CTRL]		= &pad_ctrl.hw,
> +		[CLKID_SYS_CTRL]		= &sys_ctrl.hw,
> +		[CLKID_TEMP_SENSOR]		= &temp_sensor.hw,
> +		[CLKID_AM2AXI_DIV]		= &am2axi_dev.hw,
> +		[CLKID_SPICC_B]			= &spicc_b.hw,
> +		[CLKID_SPICC_A]			= &spicc_a.hw,
> +		[CLKID_MSR]			= &msr.hw,
> +		[CLKID_AUDIO]			= &audio.hw,
> +		[CLKID_JTAG_CTRL]		= &jtag_ctrl.hw,
> +		[CLKID_SARADC_EN]		= &saradc_en.hw,
> +		[CLKID_PWM_EF]			= &pwm_ef.hw,
> +		[CLKID_PWM_CD]			= &pwm_cd.hw,
> +		[CLKID_PWM_AB]			= &pwm_ab.hw,
> +		[CLKID_CEC]			= &cec.hw,
> +		[CLKID_I2C_S]			= &i2c_s.hw,
> +		[CLKID_IR_CTRL]			= &ir_ctrl.hw,
> +		[CLKID_I2C_M_D]			= &i2c_m_d.hw,
> +		[CLKID_I2C_M_C]			= &i2c_m_c.hw,
> +		[CLKID_I2C_M_B]			= &i2c_m_b.hw,
> +		[CLKID_I2C_M_A]			= &i2c_m_a.hw,
> +		[CLKID_ACODEC]			= &acodec.hw,
> +		[CLKID_OTP]			= &otp.hw,
> +		[CLKID_SD_EMMC_A]		= &sd_emmc_a.hw,
> +		[CLKID_USB_PHY]			= &usb_phy.hw,
> +		[CLKID_USB_CTRL]		= &usb_ctrl.hw,
> +		[CLKID_SYS_DSPB]		= &sys_dspb.hw,
> +		[CLKID_SYS_DSPA]		= &sys_dspa.hw,
> +		[CLKID_DMA]			= &dma.hw,
> +		[CLKID_IRQ_CTRL]		= &irq_ctrl.hw,
> +		[CLKID_NIC]			= &nic.hw,
> +		[CLKID_GIC]			= &gic.hw,
> +		[CLKID_UART_C]			= &uart_c.hw,
> +		[CLKID_UART_B]			= &uart_b.hw,
> +		[CLKID_UART_A]			= &uart_a.hw,
> +		[CLKID_SYS_PSRAM]		= &sys_psram.hw,
> +		[CLKID_RSA]			= &rsa.hw,
> +		[CLKID_CORESIGHT]		= &coresight.hw,
> +		[CLKID_AM2AXI_VAD]		= &am2axi_vad.hw,
> +		[CLKID_AUDIO_VAD]		= &audio_vad.hw,
> +		[CLKID_AXI_DMC]			= &axi_dmc.hw,
> +		[CLKID_AXI_PSRAM]		= &axi_psram.hw,
> +		[CLKID_RAMB]			= &ramb.hw,
> +		[CLKID_RAMA]			= &rama.hw,
> +		[CLKID_AXI_SPIFC]		= &axi_spifc.hw,
> +		[CLKID_AXI_NIC]			= &axi_nic.hw,
> +		[CLKID_AXI_DMA]			= &axi_dma.hw,
> +		[CLKID_CPU_CTRL]		= &cpu_ctrl.hw,
> +		[CLKID_ROM]			= &rom.hw,
> +		[CLKID_PROC_I2C]		= &prod_i2c.hw,
> +		[CLKID_DSPA_EN]			= &dspa_en.hw,
> +		[CLKID_DSPA_EN_NIC]		= &dspa_en_nic.hw,
> +		[CLKID_DSPB_EN]			= &dspb_en.hw,
> +		[CLKID_DSPB_EN_NIC]		= &dspb_en_nic.hw,
> +		[CLKID_RTC]			= &rtc.hw,
> +		[CLKID_CECA_32K]		= &ceca_32k_out.hw,
> +		[CLKID_CECB_32K]		= &cecb_32k_out.hw,
> +		[CLKID_24M]			= &clk_24m.hw,
> +		[CLKID_12M]			= &clk_12m.hw,
> +		[CLKID_FCLK_DIV2_DIVN]		= &fclk_div2_divn.hw,
> +		[CLKID_GEN]			= &gen.hw,
> +		[CLKID_SARADC]			= &saradc.hw,
> +		[CLKID_PWM_A]			= &pwm_a.hw,
> +		[CLKID_PWM_B]			= &pwm_b.hw,
> +		[CLKID_PWM_C]			= &pwm_c.hw,
> +		[CLKID_PWM_D]			= &pwm_d.hw,
> +		[CLKID_PWM_E]			= &pwm_e.hw,
> +		[CLKID_PWM_F]			= &pwm_f.hw,
> +		[CLKID_SPICC]			= &spicc.hw,
> +		[CLKID_TS]			= &ts.hw,
> +		[CLKID_SPIFC]			= &spifc.hw,
> +		[CLKID_USB_BUS]			= &usb_bus.hw,
> +		[CLKID_SD_EMMC]			= &sd_emmc.hw,
> +		[CLKID_PSRAM]			= &psram.hw,
> +		[CLKID_DMC]			= &dmc.hw,
> +		[CLKID_GEN_SEL]			= &gen_sel.hw,
> +		[CLKID_PWM_A_SEL]		= &pwm_a_sel.hw,
> +		[CLKID_PWM_B_SEL]		= &pwm_b_sel.hw,
> +		[CLKID_PWM_C_SEL]		= &pwm_c_sel.hw,
> +		[CLKID_PWM_D_SEL]		= &pwm_d_sel.hw,
> +		[CLKID_PWM_E_SEL]		= &pwm_e_sel.hw,
> +		[CLKID_PWM_F_SEL]		= &pwm_f_sel.hw,
> +		[CLKID_DSPA_A_SEL]		= &dspa_a_sel.hw,
> +		[CLKID_DSPA_B_SEL]		= &dspa_b_sel.hw,
> +		[CLKID_DSPB_A_SEL]		= &dspb_a_sel.hw,
> +		[CLKID_DSPB_B_SEL]		= &dspb_b_sel.hw,
> +		[CLKID_CECA_32K_SEL]		= &ceca_32k_sel.hw,
> +		[CLKID_CECA_32K_SEL_PRE]	= &ceca_32k_sel_pre.hw,
> +		[CLKID_CECB_32K_SEL]		= &cecb_32k_sel.hw,
> +		[CLKID_CECB_32K_SEL_PRE]	= &cecb_32k_sel_pre.hw,
> +
> +		/* Internal clocks */

There is no need to make such groups within the table.
I don't think such change was requested in the previous reviews.

Also grouping the IDs the way you did is a bad idea.

1) Since the clocks are registered in the order of the table, this will
make a lot of orphans during the registration of the controller. It does
work but it is not ideal performance wise. Whenever possible, it is
better to register the clocks from the roots to the leafs.

2) These 2 nice groups are going away the minute you add another clock which
was not part of the original submission. This is why IDs have no meaning,
no even groups.

> +		[CLKID_XTAL_IN]			= &xtal_in.hw,
> +		[CLKID_DSPA_SEL]		= &dspa_sel.hw,
> +		[CLKID_DSPB_SEL]		= &dspb_sel.hw,
> +		[CLKID_SARADC_SEL]		= &saradc_sel.hw,
> +		[CLKID_SYS_A_SEL]		= &sys_a_sel.hw,
> +		[CLKID_SYS_A_DIV]		= &sys_a_div.hw,
> +		[CLKID_SYS_A]			= &sys_a.hw,
> +		[CLKID_SYS_B_SEL]		= &sys_b_sel.hw,
> +		[CLKID_SYS_B_DIV]		= &sys_b_div.hw,
> +		[CLKID_SYS_B]			= &sys_b.hw,
> +		[CLKID_DSPA_A_DIV]		= &dspa_a_div.hw,
> +		[CLKID_DSPA_A]			= &dspa_a.hw,
> +		[CLKID_DSPA_B_DIV]		= &dspa_b_div.hw,
> +		[CLKID_DSPA_B]			= &dspa_b.hw,
> +		[CLKID_DSPB_A_DIV]		= &dspb_a_div.hw,
> +		[CLKID_DSPB_A]			= &dspb_a.hw,
> +		[CLKID_DSPB_B_DIV]		= &dspb_b_div.hw,
> +		[CLKID_DSPB_B]			= &dspb_b.hw,
> +		[CLKID_RTC_32K_IN]		= &rtc_32k_in.hw,
> +		[CLKID_RTC_32K_DIV]		= &rtc_32k_div.hw,
> +		[CLKID_RTC_32K_XTAL]		= &rtc_32k_xtal.hw,
> +		[CLKID_RTC_32K_SEL]		= &rtc_32k_sel.hw,
> +		[CLKID_CECB_32K_IN]		= &cecb_32k_in.hw,
> +		[CLKID_CECB_32K_DIV]		= &cecb_32k_div.hw,
> +		[CLKID_CECA_32K_IN]		= &ceca_32k_in.hw,
> +		[CLKID_CECA_32K_DIV]		= &ceca_32k_div.hw,
> +		[CLKID_DIV2_PRE]		= &fclk_div2_divn_pre.hw,
> +		[CLKID_24M_DIV2]		= &clk_24m_div2.hw,
> +		[CLKID_GEN_DIV]			= &gen_div.hw,
> +		[CLKID_SARADC_DIV]		= &saradc_div.hw,
> +		[CLKID_PWM_A_DIV]		= &pwm_a_div.hw,
> +		[CLKID_PWM_B_DIV]		= &pwm_b_div.hw,
> +		[CLKID_PWM_C_DIV]		= &pwm_c_div.hw,
> +		[CLKID_PWM_D_DIV]		= &pwm_d_div.hw,
> +		[CLKID_PWM_E_DIV]		= &pwm_e_div.hw,
> +		[CLKID_PWM_F_DIV]		= &pwm_f_div.hw,
> +		[CLKID_SPICC_SEL]		= &spicc_sel.hw,
> +		[CLKID_SPICC_DIV]		= &spicc_div.hw,
> +		[CLKID_SPICC_SEL2]		= &spicc_sel2.hw,
> +		[CLKID_TS_DIV]			= &ts_div.hw,
> +		[CLKID_SPIFC_SEL]		= &spifc_sel.hw,
> +		[CLKID_SPIFC_DIV]		= &spifc_div.hw,
> +		[CLKID_SPIFC_SEL2]		= &spifc_sel2.hw,
> +		[CLKID_USB_BUS_SEL]		= &usb_bus_sel.hw,
> +		[CLKID_USB_BUS_DIV]		= &usb_bus_div.hw,
> +		[CLKID_SD_EMMC_SEL]		= &sd_emmc_sel.hw,
> +		[CLKID_SD_EMMC_DIV]		= &sd_emmc_div.hw,
> +		[CLKID_SD_EMMC_SEL2]		= &sd_emmc_sel2.hw,
> +		[CLKID_PSRAM_SEL]		= &psram_sel.hw,
> +		[CLKID_PSRAM_DIV]		= &psram_div.hw,
> +		[CLKID_PSRAM_SEL2]		= &psram_sel2.hw,
> +		[CLKID_DMC_SEL]			= &dmc_sel.hw,
> +		[CLKID_DMC_DIV]			= &dmc_div.hw,
> +		[CLKID_DMC_SEL2]		= &dmc_sel2.hw,
> +
> +		[NR_CLKS]			= NULL,
> +	},
> +	.num = NR_CLKS,
> +};