| Message ID | cover.1615221459.git.cristian.ciocaltea@gmail.com |
|---|---|
| Headers | show |
| Series | Improve clock support for Actions S500 SoC | expand |
On Mon, Mar 08, 2021 at 07:18:26PM +0200, Cristian Ciocaltea wrote: > Use correct divider registers for the Actions Semi Owl S500 SoC's UART > clocks. > > Fixes: ed6b4795ece4 ("clk: actions: Add clock driver for S500 SoC") > Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@gmail.com> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> Thanks, Mani > --- > drivers/clk/actions/owl-s500.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/clk/actions/owl-s500.c b/drivers/clk/actions/owl-s500.c > index 61bb224f6330..75b7186185b0 100644 > --- a/drivers/clk/actions/owl-s500.c > +++ b/drivers/clk/actions/owl-s500.c > @@ -305,7 +305,7 @@ static OWL_COMP_FIXED_FACTOR(i2c3_clk, "i2c3_clk", "ethernet_pll_clk", > static OWL_COMP_DIV(uart0_clk, "uart0_clk", uart_clk_mux_p, > OWL_MUX_HW(CMU_UART0CLK, 16, 1), > OWL_GATE_HW(CMU_DEVCLKEN1, 6, 0), > - OWL_DIVIDER_HW(CMU_UART1CLK, 0, 8, CLK_DIVIDER_ROUND_CLOSEST, NULL), > + OWL_DIVIDER_HW(CMU_UART0CLK, 0, 8, CLK_DIVIDER_ROUND_CLOSEST, NULL), > CLK_IGNORE_UNUSED); > > static OWL_COMP_DIV(uart1_clk, "uart1_clk", uart_clk_mux_p, > @@ -317,31 +317,31 @@ static OWL_COMP_DIV(uart1_clk, "uart1_clk", uart_clk_mux_p, > static OWL_COMP_DIV(uart2_clk, "uart2_clk", uart_clk_mux_p, > OWL_MUX_HW(CMU_UART2CLK, 16, 1), > OWL_GATE_HW(CMU_DEVCLKEN1, 8, 0), > - OWL_DIVIDER_HW(CMU_UART1CLK, 0, 8, CLK_DIVIDER_ROUND_CLOSEST, NULL), > + OWL_DIVIDER_HW(CMU_UART2CLK, 0, 8, CLK_DIVIDER_ROUND_CLOSEST, NULL), > CLK_IGNORE_UNUSED); > > static OWL_COMP_DIV(uart3_clk, "uart3_clk", uart_clk_mux_p, > OWL_MUX_HW(CMU_UART3CLK, 16, 1), > OWL_GATE_HW(CMU_DEVCLKEN1, 19, 0), > - OWL_DIVIDER_HW(CMU_UART1CLK, 0, 8, CLK_DIVIDER_ROUND_CLOSEST, NULL), > + OWL_DIVIDER_HW(CMU_UART3CLK, 0, 8, CLK_DIVIDER_ROUND_CLOSEST, NULL), > CLK_IGNORE_UNUSED); > > static OWL_COMP_DIV(uart4_clk, "uart4_clk", uart_clk_mux_p, > OWL_MUX_HW(CMU_UART4CLK, 16, 1), > OWL_GATE_HW(CMU_DEVCLKEN1, 20, 0), > - OWL_DIVIDER_HW(CMU_UART1CLK, 0, 8, CLK_DIVIDER_ROUND_CLOSEST, NULL), > + OWL_DIVIDER_HW(CMU_UART4CLK, 0, 8, CLK_DIVIDER_ROUND_CLOSEST, NULL), > CLK_IGNORE_UNUSED); > > static OWL_COMP_DIV(uart5_clk, "uart5_clk", uart_clk_mux_p, > OWL_MUX_HW(CMU_UART5CLK, 16, 1), > OWL_GATE_HW(CMU_DEVCLKEN1, 21, 0), > - OWL_DIVIDER_HW(CMU_UART1CLK, 0, 8, CLK_DIVIDER_ROUND_CLOSEST, NULL), > + OWL_DIVIDER_HW(CMU_UART5CLK, 0, 8, CLK_DIVIDER_ROUND_CLOSEST, NULL), > CLK_IGNORE_UNUSED); > > static OWL_COMP_DIV(uart6_clk, "uart6_clk", uart_clk_mux_p, > OWL_MUX_HW(CMU_UART6CLK, 16, 1), > OWL_GATE_HW(CMU_DEVCLKEN1, 18, 0), > - OWL_DIVIDER_HW(CMU_UART1CLK, 0, 8, CLK_DIVIDER_ROUND_CLOSEST, NULL), > + OWL_DIVIDER_HW(CMU_UART6CLK, 0, 8, CLK_DIVIDER_ROUND_CLOSEST, NULL), > CLK_IGNORE_UNUSED); > > static OWL_COMP_DIV(i2srx_clk, "i2srx_clk", i2s_clk_mux_p, > -- > 2.30.1 >
On Mon, Mar 08, 2021 at 07:18:27PM +0200, Cristian Ciocaltea wrote: > Drop the unsupported entries in the factor table used for the SD[0-2] > clocks definitions on the Actions Semi Owl S500 SoC. > > Fixes: ed6b4795ece4 ("clk: actions: Add clock driver for S500 SoC") > Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@gmail.com> > --- > drivers/clk/actions/owl-s500.c | 4 ---- > 1 file changed, 4 deletions(-) > > diff --git a/drivers/clk/actions/owl-s500.c b/drivers/clk/actions/owl-s500.c > index 75b7186185b0..69cd959205f5 100644 > --- a/drivers/clk/actions/owl-s500.c > +++ b/drivers/clk/actions/owl-s500.c > @@ -127,8 +127,6 @@ static struct clk_factor_table sd_factor_table[] = { > { 12, 1, 13 }, { 13, 1, 14 }, { 14, 1, 15 }, { 15, 1, 16 }, > { 16, 1, 17 }, { 17, 1, 18 }, { 18, 1, 19 }, { 19, 1, 20 }, > { 20, 1, 21 }, { 21, 1, 22 }, { 22, 1, 23 }, { 23, 1, 24 }, > - { 24, 1, 25 }, { 25, 1, 26 }, { 26, 1, 27 }, { 27, 1, 28 }, > - { 28, 1, 29 }, { 29, 1, 30 }, { 30, 1, 31 }, { 31, 1, 32 }, How did you determine that these values are not supported? I've seen cases where the datasheet has the incomplete information about the supported ranges but the downstream driver has everything. Thanks, Mani > > /* bit8: /128 */ > { 256, 1, 1 * 128 }, { 257, 1, 2 * 128 }, { 258, 1, 3 * 128 }, { 259, 1, 4 * 128 }, > @@ -137,8 +135,6 @@ static struct clk_factor_table sd_factor_table[] = { > { 268, 1, 13 * 128 }, { 269, 1, 14 * 128 }, { 270, 1, 15 * 128 }, { 271, 1, 16 * 128 }, > { 272, 1, 17 * 128 }, { 273, 1, 18 * 128 }, { 274, 1, 19 * 128 }, { 275, 1, 20 * 128 }, > { 276, 1, 21 * 128 }, { 277, 1, 22 * 128 }, { 278, 1, 23 * 128 }, { 279, 1, 24 * 128 }, > - { 280, 1, 25 * 128 }, { 281, 1, 26 * 128 }, { 282, 1, 27 * 128 }, { 283, 1, 28 * 128 }, > - { 284, 1, 29 * 128 }, { 285, 1, 30 * 128 }, { 286, 1, 31 * 128 }, { 287, 1, 32 * 128 }, > { 0, 0, 0 }, > }; > > -- > 2.30.1 >
On Mon, Mar 08, 2021 at 07:18:28PM +0200, Cristian Ciocaltea wrote: > The following clocks of the Actions Semi Owl S500 SoC have been defined > to use a shared clock factor table 'bisp_factor_table[]': DE[1-2], VCE, > VDE, BISP, SENSOR[0-1] > > There are several issues involved in this approach: > > * 'bisp_factor_table[]' describes the configuration of a regular 8-rates > divider, so its usage is redundant. Additionally, judging by the BISP > clock context, it is incomplete since it maps only 8 out of 12 > possible entries. > > * The clocks mentioned above are not identical in terms of the available > rates, therefore cannot rely on the same factor table. Specifically, > BISP and SENSOR* are standard 12-rate dividers so their configuration > should rely on a proper clock div table, while VCE and VDE require a > factor table that is a actually a subset of the one needed for DE[1-2] > clocks. > > Let's fix this by implementing the following: > > * Add new factor tables 'de_factor_table' and 'hde_factor_table' to > properly handle DE[1-2], VCE and VDE clocks. > > * Add a common div table 'std12rate_div_table' for BISP and SENSOR[0-1] > clocks converted to OWL_COMP_DIV. > > * Drop the now unused 'bisp_factor_table[]'. > Nice! > Additionally, since SENSOR[0-1] are not gated, unset the OWL_GATE_HW > configuration and drop the CLK_IGNORE_UNUSED flag in their definitions. > No. You should not screen the functionality exposed by the hw, that's what the purpose of these CLK_ flags. Other than that, this patch looks good to me. > Fixes: ed6b4795ece4 ("clk: actions: Add clock driver for S500 SoC") > Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@gmail.com> > --- > drivers/clk/actions/owl-s500.c | 48 ++++++++++++++++++++++------------ > 1 file changed, 31 insertions(+), 17 deletions(-) > > diff --git a/drivers/clk/actions/owl-s500.c b/drivers/clk/actions/owl-s500.c > index 69cd959205f5..abe8874353de 100644 > --- a/drivers/clk/actions/owl-s500.c > +++ b/drivers/clk/actions/owl-s500.c > @@ -138,9 +138,16 @@ static struct clk_factor_table sd_factor_table[] = { > { 0, 0, 0 }, > }; > > -static struct clk_factor_table bisp_factor_table[] = { > - { 0, 1, 1 }, { 1, 1, 2 }, { 2, 1, 3 }, { 3, 1, 4 }, > - { 4, 1, 5 }, { 5, 1, 6 }, { 6, 1, 7 }, { 7, 1, 8 }, > +static struct clk_factor_table de_factor_table[] = { > + { 0, 1, 1 }, { 1, 2, 3 }, { 2, 1, 2 }, { 3, 2, 5 }, > + { 4, 1, 3 }, { 5, 1, 4 }, { 6, 1, 6 }, { 7, 1, 8 }, > + { 8, 1, 12 }, > + { 0, 0, 0 }, > +}; > + > +static struct clk_factor_table hde_factor_table[] = { > + { 0, 1, 1 }, { 1, 2, 3 }, { 2, 1, 2 }, { 3, 2, 5 }, > + { 4, 1, 3 }, { 5, 1, 4 }, { 6, 1, 6 }, { 7, 1, 8 }, > { 0, 0, 0 }, > }; > > @@ -154,6 +161,13 @@ static struct clk_div_table rmii_ref_div_table[] = { > { 0, 0 }, > }; > > +static struct clk_div_table std12rate_div_table[] = { > + { 0, 1 }, { 1, 2 }, { 2, 3 }, { 3, 4 }, > + { 4, 5 }, { 5, 6 }, { 6, 7 }, { 7, 8 }, > + { 8, 9 }, { 9, 10 }, { 10, 11 }, { 11, 12 }, > + { 0, 0 }, > +}; > + > static struct clk_div_table i2s_div_table[] = { > { 0, 1 }, { 1, 2 }, { 2, 3 }, { 3, 4 }, > { 4, 6 }, { 5, 8 }, { 6, 12 }, { 7, 16 }, > @@ -189,39 +203,39 @@ static OWL_DIVIDER(rmii_ref_clk, "rmii_ref_clk", "ethernet_pll_clk", CMU_ETHERNE > > /* factor clocks */ > static OWL_FACTOR(ahb_clk, "ahb_clk", "h_clk", CMU_BUSCLK1, 2, 2, ahb_factor_table, 0, 0); > -static OWL_FACTOR(de1_clk, "de_clk1", "de_clk", CMU_DECLK, 0, 3, bisp_factor_table, 0, 0); > -static OWL_FACTOR(de2_clk, "de_clk2", "de_clk", CMU_DECLK, 4, 3, bisp_factor_table, 0, 0); > +static OWL_FACTOR(de1_clk, "de_clk1", "de_clk", CMU_DECLK, 0, 4, de_factor_table, 0, 0); > +static OWL_FACTOR(de2_clk, "de_clk2", "de_clk", CMU_DECLK, 4, 4, de_factor_table, 0, 0); > > /* composite clocks */ > static OWL_COMP_FACTOR(vce_clk, "vce_clk", hde_clk_mux_p, > OWL_MUX_HW(CMU_VCECLK, 4, 2), > OWL_GATE_HW(CMU_DEVCLKEN0, 26, 0), > - OWL_FACTOR_HW(CMU_VCECLK, 0, 3, 0, bisp_factor_table), > + OWL_FACTOR_HW(CMU_VCECLK, 0, 3, 0, hde_factor_table), > 0); > > static OWL_COMP_FACTOR(vde_clk, "vde_clk", hde_clk_mux_p, > OWL_MUX_HW(CMU_VDECLK, 4, 2), > OWL_GATE_HW(CMU_DEVCLKEN0, 25, 0), > - OWL_FACTOR_HW(CMU_VDECLK, 0, 3, 0, bisp_factor_table), > + OWL_FACTOR_HW(CMU_VDECLK, 0, 3, 0, hde_factor_table), > 0); > > -static OWL_COMP_FACTOR(bisp_clk, "bisp_clk", bisp_clk_mux_p, > +static OWL_COMP_DIV(bisp_clk, "bisp_clk", bisp_clk_mux_p, > OWL_MUX_HW(CMU_BISPCLK, 4, 1), > OWL_GATE_HW(CMU_DEVCLKEN0, 14, 0), > - OWL_FACTOR_HW(CMU_BISPCLK, 0, 3, 0, bisp_factor_table), > + OWL_DIVIDER_HW(CMU_BISPCLK, 0, 4, 0, std12rate_div_table), > 0); > > -static OWL_COMP_FACTOR(sensor0_clk, "sensor0_clk", sensor_clk_mux_p, > +static OWL_COMP_DIV(sensor0_clk, "sensor0_clk", sensor_clk_mux_p, > OWL_MUX_HW(CMU_SENSORCLK, 4, 1), > - OWL_GATE_HW(CMU_DEVCLKEN0, 14, 0), > - OWL_FACTOR_HW(CMU_SENSORCLK, 0, 3, 0, bisp_factor_table), > - CLK_IGNORE_UNUSED); > + { 0 }, > + OWL_DIVIDER_HW(CMU_SENSORCLK, 0, 4, 0, std12rate_div_table), > + 0); > > -static OWL_COMP_FACTOR(sensor1_clk, "sensor1_clk", sensor_clk_mux_p, > +static OWL_COMP_DIV(sensor1_clk, "sensor1_clk", sensor_clk_mux_p, > OWL_MUX_HW(CMU_SENSORCLK, 4, 1), > - OWL_GATE_HW(CMU_DEVCLKEN0, 14, 0), > - OWL_FACTOR_HW(CMU_SENSORCLK, 8, 3, 0, bisp_factor_table), > - CLK_IGNORE_UNUSED); > + { 0 }, > + OWL_DIVIDER_HW(CMU_SENSORCLK, 8, 4, 0, std12rate_div_table), > + 0); > > static OWL_COMP_FACTOR(sd0_clk, "sd0_clk", sd_clk_mux_p, > OWL_MUX_HW(CMU_SD0CLK, 9, 1), > -- > 2.30.1 >
On Mon, Mar 08, 2021 at 07:18:31PM +0200, Cristian Ciocaltea wrote: > Add support for the missing NIC and ETHERNET clocks in the Actions Semi > Owl S500 SoC clock driver. > > Additionally, change APB clock parent from AHB to the newly added NIC. > > Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@gmail.com> > --- > drivers/clk/actions/owl-s500.c | 17 ++++++++++++++++- > 1 file changed, 16 insertions(+), 1 deletion(-) > > diff --git a/drivers/clk/actions/owl-s500.c b/drivers/clk/actions/owl-s500.c > index b9e434173b4f..0ccc9619b302 100644 > --- a/drivers/clk/actions/owl-s500.c > +++ b/drivers/clk/actions/owl-s500.c > @@ -112,6 +112,7 @@ static const char * const bisp_clk_mux_p[] = { "display_pll_clk", "dev_clk" }; > static const char * const sensor_clk_mux_p[] = { "hosc", "bisp_clk" }; > static const char * const sd_clk_mux_p[] = { "dev_clk", "nand_pll_clk" }; > static const char * const pwm_clk_mux_p[] = { "losc", "hosc" }; > +static const char * const nic_clk_mux_p[] = { "dev_clk", "display_pll_clk", "nand_pll_clk", "ddr_pll_clk" }; As per the reg field order, this should come after "ahbprediv_clk_mux_p" Rest looks good. Thanks, Mani > static const char * const ahbprediv_clk_mux_p[] = { "dev_clk", "display_pll_clk", "nand_pll_clk", "ddr_pll_clk" }; > static const char * const uart_clk_mux_p[] = { "hosc", "dev_pll_clk" }; > static const char * const de_clk_mux_p[] = { "display_pll_clk", "dev_clk" }; > @@ -197,7 +198,7 @@ static OWL_GATE(hdmi_clk, "hdmi_clk", "hosc", CMU_DEVCLKEN1, 3, 0, 0); > > /* divider clocks */ > static OWL_DIVIDER(h_clk, "h_clk", "ahbprediv_clk", CMU_BUSCLK1, 2, 2, h_div_table, 0, 0); > -static OWL_DIVIDER(apb_clk, "apb_clk", "ahb_clk", CMU_BUSCLK1, 14, 2, NULL, 0, 0); > +static OWL_DIVIDER(apb_clk, "apb_clk", "nic_clk", CMU_BUSCLK1, 14, 2, NULL, 0, 0); > static OWL_DIVIDER(rmii_ref_clk, "rmii_ref_clk", "ethernet_pll_clk", CMU_ETHERNETPLL, 1, 1, rmii_ref_div_table, 0, 0); > > /* factor clocks */ > @@ -205,6 +206,12 @@ static OWL_FACTOR(de1_clk, "de_clk1", "de_clk", CMU_DECLK, 0, 4, de_factor_table > static OWL_FACTOR(de2_clk, "de_clk2", "de_clk", CMU_DECLK, 4, 4, de_factor_table, 0, 0); > > /* composite clocks */ > +static OWL_COMP_DIV(nic_clk, "nic_clk", nic_clk_mux_p, > + OWL_MUX_HW(CMU_BUSCLK1, 4, 3), > + { 0 }, > + OWL_DIVIDER_HW(CMU_BUSCLK1, 16, 2, 0, NULL), > + 0); > + > static OWL_COMP_DIV(ahbprediv_clk, "ahbprediv_clk", ahbprediv_clk_mux_p, > OWL_MUX_HW(CMU_BUSCLK1, 8, 3), > { 0 }, > @@ -320,6 +327,10 @@ static OWL_COMP_FIXED_FACTOR(i2c3_clk, "i2c3_clk", "ethernet_pll_clk", > OWL_GATE_HW(CMU_DEVCLKEN1, 31, 0), > 1, 5, 0); > > +static OWL_COMP_FIXED_FACTOR(ethernet_clk, "ethernet_clk", "ethernet_pll_clk", > + OWL_GATE_HW(CMU_DEVCLKEN1, 22, 0), > + 1, 20, 0); > + > static OWL_COMP_DIV(uart0_clk, "uart0_clk", uart_clk_mux_p, > OWL_MUX_HW(CMU_UART0CLK, 16, 1), > OWL_GATE_HW(CMU_DEVCLKEN1, 6, 0), > @@ -454,6 +465,8 @@ static struct owl_clk_common *s500_clks[] = { > &apb_clk.common, > &dmac_clk.common, > &gpio_clk.common, > + &nic_clk.common, > + ðernet_clk.common, > }; > > static struct clk_hw_onecell_data s500_hw_clks = { > @@ -513,6 +526,8 @@ static struct clk_hw_onecell_data s500_hw_clks = { > [CLK_APB] = &apb_clk.common.hw, > [CLK_DMAC] = &dmac_clk.common.hw, > [CLK_GPIO] = &gpio_clk.common.hw, > + [CLK_NIC] = &nic_clk.common.hw, > + [CLK_ETHERNET] = ðernet_clk.common.hw, > }, > .num = CLK_NR_CLKS, > }; > -- > 2.30.1 >
Hi Mani, Thanks for reviewing this patch series! On Tue, Mar 16, 2021 at 09:28:45AM +0530, Manivannan Sadhasivam wrote: > On Mon, Mar 08, 2021 at 07:18:27PM +0200, Cristian Ciocaltea wrote: > > Drop the unsupported entries in the factor table used for the SD[0-2] > > clocks definitions on the Actions Semi Owl S500 SoC. > > > > Fixes: ed6b4795ece4 ("clk: actions: Add clock driver for S500 SoC") > > Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@gmail.com> > > --- > > drivers/clk/actions/owl-s500.c | 4 ---- > > 1 file changed, 4 deletions(-) > > > > diff --git a/drivers/clk/actions/owl-s500.c b/drivers/clk/actions/owl-s500.c > > index 75b7186185b0..69cd959205f5 100644 > > --- a/drivers/clk/actions/owl-s500.c > > +++ b/drivers/clk/actions/owl-s500.c > > @@ -127,8 +127,6 @@ static struct clk_factor_table sd_factor_table[] = { > > { 12, 1, 13 }, { 13, 1, 14 }, { 14, 1, 15 }, { 15, 1, 16 }, > > { 16, 1, 17 }, { 17, 1, 18 }, { 18, 1, 19 }, { 19, 1, 20 }, > > { 20, 1, 21 }, { 21, 1, 22 }, { 22, 1, 23 }, { 23, 1, 24 }, > > - { 24, 1, 25 }, { 25, 1, 26 }, { 26, 1, 27 }, { 27, 1, 28 }, > > - { 28, 1, 29 }, { 29, 1, 30 }, { 30, 1, 31 }, { 31, 1, 32 }, > > How did you determine that these values are not supported? > > I've seen cases where the datasheet has the incomplete information about the > supported ranges but the downstream driver has everything. My primary source of information is the xapp-le kernel source code: https://github.com/xapp-le/kernel I always try to double check the implementation with the information in the datasheet, but sometimes, as you already pointed out, it is incomplete. For the SD clocks, it is even worse: there is absolutely no information related to the CMU_SD[0-2]CLK registers. Therefore I had to rely exclusively on the downstream driver code. Hence, for the SD1 clock, I identified the following code snippets: static struct owl_clkreq divbit_PRESD0_CLK = BITMAP(CMU_SD0CLK, 0x0000001f, 0); static struct owl_clkreq divbit_SD0_CLK_2X = BITMAP(CMU_SD0CLK, 0x00000100, 8); static struct owl_refertab T_sdx2 = {{1, 128, -1}, 0}; static struct owl_div divider_PRESD0_CLK = { .type = DIV_T_NATURE, .range_from = 0, .range_to = 24, .reg = &divbit_PRESD0_CLK, }; static struct owl_div divider_SD0_CLK_2X = { .type = DIV_T_TABLE, .range_from = 0, .range_to = 1, .ext = {.tab = &T_sdx2,}, .reg = &divbit_SD0_CLK_2X, }; This is basically what gets translated to sd_factor_table and I removed the extra entries 25..31. Actually I also dropped the 24th one, since that would give us an odd number of items, although I'm not quite sure this is a bug in the xapp-le code or the HW is really supposed to work like that. Kind regards, Cristi > Thanks, > Mani > > > > > /* bit8: /128 */ > > { 256, 1, 1 * 128 }, { 257, 1, 2 * 128 }, { 258, 1, 3 * 128 }, { 259, 1, 4 * 128 }, > > @@ -137,8 +135,6 @@ static struct clk_factor_table sd_factor_table[] = { > > { 268, 1, 13 * 128 }, { 269, 1, 14 * 128 }, { 270, 1, 15 * 128 }, { 271, 1, 16 * 128 }, > > { 272, 1, 17 * 128 }, { 273, 1, 18 * 128 }, { 274, 1, 19 * 128 }, { 275, 1, 20 * 128 }, > > { 276, 1, 21 * 128 }, { 277, 1, 22 * 128 }, { 278, 1, 23 * 128 }, { 279, 1, 24 * 128 }, > > - { 280, 1, 25 * 128 }, { 281, 1, 26 * 128 }, { 282, 1, 27 * 128 }, { 283, 1, 28 * 128 }, > > - { 284, 1, 29 * 128 }, { 285, 1, 30 * 128 }, { 286, 1, 31 * 128 }, { 287, 1, 32 * 128 }, > > { 0, 0, 0 }, > > }; > > > > -- > > 2.30.1 > >
On Tue, Mar 16, 2021 at 09:47:39AM +0530, Manivannan Sadhasivam wrote: > On Mon, Mar 08, 2021 at 07:18:28PM +0200, Cristian Ciocaltea wrote: > > The following clocks of the Actions Semi Owl S500 SoC have been defined > > to use a shared clock factor table 'bisp_factor_table[]': DE[1-2], VCE, > > VDE, BISP, SENSOR[0-1] > > > > There are several issues involved in this approach: > > > > * 'bisp_factor_table[]' describes the configuration of a regular 8-rates > > divider, so its usage is redundant. Additionally, judging by the BISP > > clock context, it is incomplete since it maps only 8 out of 12 > > possible entries. > > > > * The clocks mentioned above are not identical in terms of the available > > rates, therefore cannot rely on the same factor table. Specifically, > > BISP and SENSOR* are standard 12-rate dividers so their configuration > > should rely on a proper clock div table, while VCE and VDE require a > > factor table that is a actually a subset of the one needed for DE[1-2] > > clocks. > > > > Let's fix this by implementing the following: > > > > * Add new factor tables 'de_factor_table' and 'hde_factor_table' to > > properly handle DE[1-2], VCE and VDE clocks. > > > > * Add a common div table 'std12rate_div_table' for BISP and SENSOR[0-1] > > clocks converted to OWL_COMP_DIV. > > > > * Drop the now unused 'bisp_factor_table[]'. > > > > Nice! > > > Additionally, since SENSOR[0-1] are not gated, unset the OWL_GATE_HW > > configuration and drop the CLK_IGNORE_UNUSED flag in their definitions. > > > > No. You should not screen the functionality exposed by the hw, that's what the > purpose of these CLK_ flags. I'm not sure I get this, or maybe I wasn't clear enough with my explanation regarding the changes to SENSOR clocks: they are not gated in hardware, hence the statement 'OWL_GATE_HW(CMU_DEVCLKEN0, 14, 0)' was invalid and I replaced it with '{ 0 }'. Additionally, I assumed the 'CLK_IGNORE_UNUSED' flag makes sense only for the gated clocks. Do I miss something? > Other than that, this patch looks good to me. Thanks, Cristi [...]
On Tue, Mar 16, 2021 at 11:22:40AM +0530, Manivannan Sadhasivam wrote: > On Mon, Mar 08, 2021 at 07:18:31PM +0200, Cristian Ciocaltea wrote: > > Add support for the missing NIC and ETHERNET clocks in the Actions Semi > > Owl S500 SoC clock driver. > > > > Additionally, change APB clock parent from AHB to the newly added NIC. > > > > Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@gmail.com> > > --- > > drivers/clk/actions/owl-s500.c | 17 ++++++++++++++++- > > 1 file changed, 16 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/clk/actions/owl-s500.c b/drivers/clk/actions/owl-s500.c > > index b9e434173b4f..0ccc9619b302 100644 > > --- a/drivers/clk/actions/owl-s500.c > > +++ b/drivers/clk/actions/owl-s500.c > > @@ -112,6 +112,7 @@ static const char * const bisp_clk_mux_p[] = { "display_pll_clk", "dev_clk" }; > > static const char * const sensor_clk_mux_p[] = { "hosc", "bisp_clk" }; > > static const char * const sd_clk_mux_p[] = { "dev_clk", "nand_pll_clk" }; > > static const char * const pwm_clk_mux_p[] = { "losc", "hosc" }; > > +static const char * const nic_clk_mux_p[] = { "dev_clk", "display_pll_clk", "nand_pll_clk", "ddr_pll_clk" }; > > As per the reg field order, this should come after "ahbprediv_clk_mux_p" Fixed, thanks! > Rest looks good. > > Thanks, > Mani > [...]
On Tue, Mar 16, 2021 at 08:14:37PM +0200, Cristian Ciocaltea wrote: > Hi Mani, > > Thanks for reviewing this patch series! > > On Tue, Mar 16, 2021 at 09:28:45AM +0530, Manivannan Sadhasivam wrote: > > On Mon, Mar 08, 2021 at 07:18:27PM +0200, Cristian Ciocaltea wrote: > > > Drop the unsupported entries in the factor table used for the SD[0-2] > > > clocks definitions on the Actions Semi Owl S500 SoC. > > > > > > Fixes: ed6b4795ece4 ("clk: actions: Add clock driver for S500 SoC") > > > Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@gmail.com> > > > --- > > > drivers/clk/actions/owl-s500.c | 4 ---- > > > 1 file changed, 4 deletions(-) > > > > > > diff --git a/drivers/clk/actions/owl-s500.c b/drivers/clk/actions/owl-s500.c > > > index 75b7186185b0..69cd959205f5 100644 > > > --- a/drivers/clk/actions/owl-s500.c > > > +++ b/drivers/clk/actions/owl-s500.c > > > @@ -127,8 +127,6 @@ static struct clk_factor_table sd_factor_table[] = { > > > { 12, 1, 13 }, { 13, 1, 14 }, { 14, 1, 15 }, { 15, 1, 16 }, > > > { 16, 1, 17 }, { 17, 1, 18 }, { 18, 1, 19 }, { 19, 1, 20 }, > > > { 20, 1, 21 }, { 21, 1, 22 }, { 22, 1, 23 }, { 23, 1, 24 }, > > > - { 24, 1, 25 }, { 25, 1, 26 }, { 26, 1, 27 }, { 27, 1, 28 }, > > > - { 28, 1, 29 }, { 29, 1, 30 }, { 30, 1, 31 }, { 31, 1, 32 }, > > [...] > This is basically what gets translated to sd_factor_table and I removed > the extra entries 25..31. Actually I also dropped the 24th one, since > that would give us an odd number of items, although I'm not quite sure > this is a bug in the xapp-le code or the HW is really supposed to work > like that. > In my datasheet I can see the factor values till 24. So let's remove the entries from 25-31. Thanks, Mani > Kind regards, > Cristi > > > Thanks, > > Mani > > > > > > > > /* bit8: /128 */ > > > { 256, 1, 1 * 128 }, { 257, 1, 2 * 128 }, { 258, 1, 3 * 128 }, { 259, 1, 4 * 128 }, > > > @@ -137,8 +135,6 @@ static struct clk_factor_table sd_factor_table[] = { > > > { 268, 1, 13 * 128 }, { 269, 1, 14 * 128 }, { 270, 1, 15 * 128 }, { 271, 1, 16 * 128 }, > > > { 272, 1, 17 * 128 }, { 273, 1, 18 * 128 }, { 274, 1, 19 * 128 }, { 275, 1, 20 * 128 }, > > > { 276, 1, 21 * 128 }, { 277, 1, 22 * 128 }, { 278, 1, 23 * 128 }, { 279, 1, 24 * 128 }, > > > - { 280, 1, 25 * 128 }, { 281, 1, 26 * 128 }, { 282, 1, 27 * 128 }, { 283, 1, 28 * 128 }, > > > - { 284, 1, 29 * 128 }, { 285, 1, 30 * 128 }, { 286, 1, 31 * 128 }, { 287, 1, 32 * 128 }, > > > { 0, 0, 0 }, > > > }; > > > > > > -- > > > 2.30.1 > > >
On Tue, Mar 16, 2021 at 08:37:53PM +0200, Cristian Ciocaltea wrote: > On Tue, Mar 16, 2021 at 09:47:39AM +0530, Manivannan Sadhasivam wrote: > > On Mon, Mar 08, 2021 at 07:18:28PM +0200, Cristian Ciocaltea wrote: > > > The following clocks of the Actions Semi Owl S500 SoC have been defined > > > to use a shared clock factor table 'bisp_factor_table[]': DE[1-2], VCE, > > > VDE, BISP, SENSOR[0-1] > > > > > > There are several issues involved in this approach: > > > > > > * 'bisp_factor_table[]' describes the configuration of a regular 8-rates > > > divider, so its usage is redundant. Additionally, judging by the BISP > > > clock context, it is incomplete since it maps only 8 out of 12 > > > possible entries. > > > > > > * The clocks mentioned above are not identical in terms of the available > > > rates, therefore cannot rely on the same factor table. Specifically, > > > BISP and SENSOR* are standard 12-rate dividers so their configuration > > > should rely on a proper clock div table, while VCE and VDE require a > > > factor table that is a actually a subset of the one needed for DE[1-2] > > > clocks. > > > > > > Let's fix this by implementing the following: > > > > > > * Add new factor tables 'de_factor_table' and 'hde_factor_table' to > > > properly handle DE[1-2], VCE and VDE clocks. > > > > > > * Add a common div table 'std12rate_div_table' for BISP and SENSOR[0-1] > > > clocks converted to OWL_COMP_DIV. > > > > > > * Drop the now unused 'bisp_factor_table[]'. > > > > > > > Nice! > > > > > Additionally, since SENSOR[0-1] are not gated, unset the OWL_GATE_HW > > > configuration and drop the CLK_IGNORE_UNUSED flag in their definitions. > > > > > > > No. You should not screen the functionality exposed by the hw, that's what the > > purpose of these CLK_ flags. > > I'm not sure I get this, or maybe I wasn't clear enough with my > explanation regarding the changes to SENSOR clocks: they are not gated > in hardware, hence the statement 'OWL_GATE_HW(CMU_DEVCLKEN0, 14, 0)' > was invalid and I replaced it with '{ 0 }'. > This clock is gated in hw as per the datasheet. Again, please don't make judgements based on the vendor code as it is not upto date with HW. I know it is silly but that's how things are... > Additionally, I assumed the 'CLK_IGNORE_UNUSED' flag makes sense only > for the gated clocks. Do I miss something? > CLK_IGNORE_UNUSED is used by the clk framework to essentially skip gating the clocks which are turned ON by the bootloader and there is no other driver using it. But I think you can remove this flag because there is no reason to leave this specific clock to be ON always. Thanks, Mani > > Other than that, this patch looks good to me. > > Thanks, > Cristi > > [...]
On Wed, May 26, 2021 at 03:37:43PM +0530, Manivannan Sadhasivam wrote: > On Tue, Mar 16, 2021 at 08:14:37PM +0200, Cristian Ciocaltea wrote: > > Hi Mani, > > > > Thanks for reviewing this patch series! > > > > On Tue, Mar 16, 2021 at 09:28:45AM +0530, Manivannan Sadhasivam wrote: > > > On Mon, Mar 08, 2021 at 07:18:27PM +0200, Cristian Ciocaltea wrote: > > > > Drop the unsupported entries in the factor table used for the SD[0-2] > > > > clocks definitions on the Actions Semi Owl S500 SoC. > > > > > > > > Fixes: ed6b4795ece4 ("clk: actions: Add clock driver for S500 SoC") > > > > Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@gmail.com> > > > > --- > > > > drivers/clk/actions/owl-s500.c | 4 ---- > > > > 1 file changed, 4 deletions(-) > > > > > > > > diff --git a/drivers/clk/actions/owl-s500.c b/drivers/clk/actions/owl-s500.c > > > > index 75b7186185b0..69cd959205f5 100644 > > > > --- a/drivers/clk/actions/owl-s500.c > > > > +++ b/drivers/clk/actions/owl-s500.c > > > > @@ -127,8 +127,6 @@ static struct clk_factor_table sd_factor_table[] = { > > > > { 12, 1, 13 }, { 13, 1, 14 }, { 14, 1, 15 }, { 15, 1, 16 }, > > > > { 16, 1, 17 }, { 17, 1, 18 }, { 18, 1, 19 }, { 19, 1, 20 }, > > > > { 20, 1, 21 }, { 21, 1, 22 }, { 22, 1, 23 }, { 23, 1, 24 }, > > > > - { 24, 1, 25 }, { 25, 1, 26 }, { 26, 1, 27 }, { 27, 1, 28 }, > > > > - { 28, 1, 29 }, { 29, 1, 30 }, { 30, 1, 31 }, { 31, 1, 32 }, > > > > > [...] > > > This is basically what gets translated to sd_factor_table and I removed > > the extra entries 25..31. Actually I also dropped the 24th one, since > > that would give us an odd number of items, although I'm not quite sure > > this is a bug in the xapp-le code or the HW is really supposed to work > > like that. > > > > In my datasheet I can see the factor values till 24. So let's remove the > entries from 25-31. I got an updated datasheet and I confirm 24 is a valid selector. Applied the correction in v2: https://lore.kernel.org/lkml/cover.1622119892.git.cristian.ciocaltea@gmail.com/ Thanks, Cristi > Thanks, > Mani > > > Kind regards, > > Cristi > > > > > Thanks, > > > Mani > > > > > > > > > > > /* bit8: /128 */ > > > > { 256, 1, 1 * 128 }, { 257, 1, 2 * 128 }, { 258, 1, 3 * 128 }, { 259, 1, 4 * 128 }, > > > > @@ -137,8 +135,6 @@ static struct clk_factor_table sd_factor_table[] = { > > > > { 268, 1, 13 * 128 }, { 269, 1, 14 * 128 }, { 270, 1, 15 * 128 }, { 271, 1, 16 * 128 }, > > > > { 272, 1, 17 * 128 }, { 273, 1, 18 * 128 }, { 274, 1, 19 * 128 }, { 275, 1, 20 * 128 }, > > > > { 276, 1, 21 * 128 }, { 277, 1, 22 * 128 }, { 278, 1, 23 * 128 }, { 279, 1, 24 * 128 }, > > > > - { 280, 1, 25 * 128 }, { 281, 1, 26 * 128 }, { 282, 1, 27 * 128 }, { 283, 1, 28 * 128 }, > > > > - { 284, 1, 29 * 128 }, { 285, 1, 30 * 128 }, { 286, 1, 31 * 128 }, { 287, 1, 32 * 128 }, > > > > { 0, 0, 0 }, > > > > }; > > > > > > > > -- > > > > 2.30.1 > > > >
On Wed, May 26, 2021 at 03:48:30PM +0530, Manivannan Sadhasivam wrote: > On Tue, Mar 16, 2021 at 08:37:53PM +0200, Cristian Ciocaltea wrote: > > On Tue, Mar 16, 2021 at 09:47:39AM +0530, Manivannan Sadhasivam wrote: > > > On Mon, Mar 08, 2021 at 07:18:28PM +0200, Cristian Ciocaltea wrote: > > > > The following clocks of the Actions Semi Owl S500 SoC have been defined > > > > to use a shared clock factor table 'bisp_factor_table[]': DE[1-2], VCE, > > > > VDE, BISP, SENSOR[0-1] > > > > > > > > There are several issues involved in this approach: > > > > > > > > * 'bisp_factor_table[]' describes the configuration of a regular 8-rates > > > > divider, so its usage is redundant. Additionally, judging by the BISP > > > > clock context, it is incomplete since it maps only 8 out of 12 > > > > possible entries. > > > > > > > > * The clocks mentioned above are not identical in terms of the available > > > > rates, therefore cannot rely on the same factor table. Specifically, > > > > BISP and SENSOR* are standard 12-rate dividers so their configuration > > > > should rely on a proper clock div table, while VCE and VDE require a > > > > factor table that is a actually a subset of the one needed for DE[1-2] > > > > clocks. > > > > > > > > Let's fix this by implementing the following: > > > > > > > > * Add new factor tables 'de_factor_table' and 'hde_factor_table' to > > > > properly handle DE[1-2], VCE and VDE clocks. > > > > > > > > * Add a common div table 'std12rate_div_table' for BISP and SENSOR[0-1] > > > > clocks converted to OWL_COMP_DIV. > > > > > > > > * Drop the now unused 'bisp_factor_table[]'. > > > > > > > > > > Nice! > > > > > > > Additionally, since SENSOR[0-1] are not gated, unset the OWL_GATE_HW > > > > configuration and drop the CLK_IGNORE_UNUSED flag in their definitions. > > > > > > > > > > No. You should not screen the functionality exposed by the hw, that's what the > > > purpose of these CLK_ flags. > > > > I'm not sure I get this, or maybe I wasn't clear enough with my > > explanation regarding the changes to SENSOR clocks: they are not gated > > in hardware, hence the statement 'OWL_GATE_HW(CMU_DEVCLKEN0, 14, 0)' > > was invalid and I replaced it with '{ 0 }'. > > > > This clock is gated in hw as per the datasheet. Again, please don't make > judgements based on the vendor code as it is not upto date with HW. I > know it is silly but that's how things are... Indeed, a newer datasheet states the clock is gated. I fixed the patch accordingly in v2. > > Additionally, I assumed the 'CLK_IGNORE_UNUSED' flag makes sense only > > for the gated clocks. Do I miss something? > > > > CLK_IGNORE_UNUSED is used by the clk framework to essentially skip > gating the clocks which are turned ON by the bootloader and there is no > other driver using it. But I think you can remove this flag because > there is no reason to leave this specific clock to be ON always. Thanks for the explanation, I kept the flag removed in v2. Regards, Cristi > Thanks, > Mani > > > > Other than that, this patch looks good to me. > > > > Thanks, > > Cristi > > > > [...]