| Message ID | 20190119091528.11776-7-lukma@denx.de |
|---|---|
| State | Changes Requested |
| Delegated to: | Stefano Babic |
| Headers | show |
| Series | imx: dm: Update mccmon6 board to only use DM/DTS in u-boot proper | expand |
Hi Lukasz, On Sat, Jan 19, 2019 at 7:15 AM Lukasz Majewski <lukma@denx.de> wrote: > +static ulong imx6q_clk_get_rate(struct clk *clk) > +{ > + ulong rate = 0; > + > + debug("%s(#%lu)\n", __func__, clk->id); > + > + switch (clk->id) { > + case IMX6QDL_CLK_ECSPI1: > + case IMX6QDL_CLK_ECSPI2: > + case IMX6QDL_CLK_ECSPI3: > + case IMX6QDL_CLK_ECSPI4: > + return imx6_get_cspi_clk(); > + > + case IMX6QDL_CLK_USDHC1: > + case IMX6QDL_CLK_USDHC2: > + case IMX6QDL_CLK_USDHC3: > + case IMX6QDL_CLK_USDHC4: > + return imx6_get_usdhc_clk(clk->id - IMX6QDL_CLK_USDHC1); I don't think this scales well as this needs to grow for all other peripherals and for each port instance. If we are adding a clock driver for mx6, why don't we add it just like the kernel one? Barebox imports the clock driver from the kernel and it is much cleaner: https://git.pengutronix.de/cgit/barebox/tree/drivers/clk/imx/clk-imx6.c
Hi Fabio, > Hi Lukasz, > > On Sat, Jan 19, 2019 at 7:15 AM Lukasz Majewski <lukma@denx.de> wrote: > > > +static ulong imx6q_clk_get_rate(struct clk *clk) > > +{ > > + ulong rate = 0; > > + > > + debug("%s(#%lu)\n", __func__, clk->id); > > + > > + switch (clk->id) { > > + case IMX6QDL_CLK_ECSPI1: > > + case IMX6QDL_CLK_ECSPI2: > > + case IMX6QDL_CLK_ECSPI3: > > + case IMX6QDL_CLK_ECSPI4: > > + return imx6_get_cspi_clk(); > > + > > + case IMX6QDL_CLK_USDHC1: > > + case IMX6QDL_CLK_USDHC2: > > + case IMX6QDL_CLK_USDHC3: > > + case IMX6QDL_CLK_USDHC4: > > + return imx6_get_usdhc_clk(clk->id - > > IMX6QDL_CLK_USDHC1); > > I don't think this scales well as this needs to grow for all other > peripherals and for each port instance. The rationale regarding this approach: 1. Reuse the clock.c code for iMX6Q as much as possible. 2, This code is based on the clk-imx8q.c file - hence the question why the Linux clock API was not ported for this new SoC?. > > If we are adding a clock driver for mx6, why don't we add it just like > the kernel one? I can try to port the Linux code, but IMHO it would be feasible to port only relevant (ECSPI, USDHC) parts of it (not all as I cannot test it all properly). > > Barebox imports the clock driver from the kernel and it is much > cleaner: > https://git.pengutronix.de/cgit/barebox/tree/drivers/clk/imx/clk-imx6.c Yes, it has been trimmed (...a bit...) when compared to original v4.20 :-) . Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
Hi Lukasz, On Mon, Jan 21, 2019 at 12:19 PM Lukasz Majewski <lukma@denx.de> wrote: > The rationale regarding this approach: > > 1. Reuse the clock.c code for iMX6Q as much as possible. Yes, the problem is that clock.c does not scale well as we have dedicated clock functions for each peripheral: enable_uart_clk(), enable_usdhc_clk(), enable_i2c_clk(), etc If we have a chance to use common clock framework now in U-Boot that would be a much better implementation and easier to maintain in sync with the kernel. > 2, This code is based on the clk-imx8q.c file - hence the question > why the Linux clock API was not ported for this new SoC?. The problem is that up to now no imx clock driver uses the kernel approach. Barebox uses the kernel approach for all imx SoCs, imx8mq included: https://git.pengutronix.de/cgit/barebox/tree/drivers/clk/imx/clk-imx8mq.c > > If we are adding a clock driver for mx6, why don't we add it just like > > the kernel one? > > I can try to port the Linux code, but IMHO it would be feasible to port > only relevant (ECSPI, USDHC) parts of it (not all as I cannot test it > all properly). That would be much appreciated. Thanks
Hi Lukasz, On 21/01/19 15:19, Lukasz Majewski wrote: > Hi Fabio, > >> Hi Lukasz, >> >> On Sat, Jan 19, 2019 at 7:15 AM Lukasz Majewski <lukma@denx.de> wrote: >> >>> +static ulong imx6q_clk_get_rate(struct clk *clk) >>> +{ >>> + ulong rate = 0; >>> + >>> + debug("%s(#%lu)\n", __func__, clk->id); >>> + >>> + switch (clk->id) { >>> + case IMX6QDL_CLK_ECSPI1: >>> + case IMX6QDL_CLK_ECSPI2: >>> + case IMX6QDL_CLK_ECSPI3: >>> + case IMX6QDL_CLK_ECSPI4: >>> + return imx6_get_cspi_clk(); >>> + >>> + case IMX6QDL_CLK_USDHC1: >>> + case IMX6QDL_CLK_USDHC2: >>> + case IMX6QDL_CLK_USDHC3: >>> + case IMX6QDL_CLK_USDHC4: >>> + return imx6_get_usdhc_clk(clk->id - >>> IMX6QDL_CLK_USDHC1); >> >> I don't think this scales well as this needs to grow for all other >> peripherals and for each port instance. > I am hiiting the same doubts as Fabio when I reviewed Peng's patches with clocks for i.MX8M. The current approach was introduced some years ago with i.MX5, but it does not fit well now and it does not scale. > The rationale regarding this approach: > > 1. Reuse the clock.c code for iMX6Q as much as possible. > > 2, This code is based on the clk-imx8q.c file - hence the question > why the Linux clock API was not ported for this new SoC?. Many reasons, first there was already a set of functions to get / set clocks coming from previos i.MX platforms, and this API was simply reused. And nobody was in charge to port the clock API. > >> >> If we are adding a clock driver for mx6, why don't we add it just like >> the kernel one? > > I can try to port the Linux code, but IMHO it would be feasible to port > only relevant (ECSPI, USDHC) parts of it (not all as I cannot test it > all properly). Fully agree. Further parts will be added on demand. Less is better. I disagree to add not tested code. > >> >> Barebox imports the clock driver from the kernel and it is much >> cleaner: >> https://git.pengutronix.de/cgit/barebox/tree/drivers/clk/imx/clk-imx6.c > > Yes, it has been trimmed (...a bit...) when compared to original > v4.20 :-) . > Best regards, Stefano
Hi Stefano, Fabio, > Hi Lukasz, > > On 21/01/19 15:19, Lukasz Majewski wrote: > > Hi Fabio, > > > >> Hi Lukasz, > >> > >> On Sat, Jan 19, 2019 at 7:15 AM Lukasz Majewski <lukma@denx.de> > >> wrote: > >>> +static ulong imx6q_clk_get_rate(struct clk *clk) > >>> +{ > >>> + ulong rate = 0; > >>> + > >>> + debug("%s(#%lu)\n", __func__, clk->id); > >>> + > >>> + switch (clk->id) { > >>> + case IMX6QDL_CLK_ECSPI1: > >>> + case IMX6QDL_CLK_ECSPI2: > >>> + case IMX6QDL_CLK_ECSPI3: > >>> + case IMX6QDL_CLK_ECSPI4: > >>> + return imx6_get_cspi_clk(); > >>> + > >>> + case IMX6QDL_CLK_USDHC1: > >>> + case IMX6QDL_CLK_USDHC2: > >>> + case IMX6QDL_CLK_USDHC3: > >>> + case IMX6QDL_CLK_USDHC4: > >>> + return imx6_get_usdhc_clk(clk->id - > >>> IMX6QDL_CLK_USDHC1); > >> > >> I don't think this scales well as this needs to grow for all other > >> peripherals and for each port instance. > > > > I am hiiting the same doubts as Fabio when I reviewed Peng's patches > with clocks for i.MX8M. The current approach was introduced some years > ago with i.MX5, but it does not fit well now and it does not scale. > > > > The rationale regarding this approach: > > > > 1. Reuse the clock.c code for iMX6Q as much as possible. > > > > 2, This code is based on the clk-imx8q.c file - hence the question > > why the Linux clock API was not ported for this new SoC?. > > Many reasons, first there was already a set of functions to get / set > clocks coming from previos i.MX platforms, and this API was simply > reused. And nobody was in charge to port the clock API. > > > > >> > >> If we are adding a clock driver for mx6, why don't we add it just > >> like the kernel one? > > > > I can try to port the Linux code, but IMHO it would be feasible to > > port only relevant (ECSPI, USDHC) parts of it (not all as I cannot > > test it all properly). > > Fully agree. Further parts will be added on demand. Less is better. I > disagree to add not tested code. The work is in progress - I will add the same directory structure as Linux's Common Clock Framework [CCF]. I shall finish in 2-3 days. There are a few problems to tackle (when porting code from Linux): 1. As it is now - for IMX6Q we need a static table to store clock pointers. It is around 1KiB of SRAM/SDRAM. This is _a_lot_ for SPL. For u-boot proper this is not a huge problem. 2. The SPL shall use OF_PLATDATA, as we do need clock management mostly in SPL. Moreover, I do think that SPL will be bigger (this is a _real_ problem, IMHO). 3. The CCF design needs to be fitted into U-Boot's Driver Model - this is achievable. The most annoying problem is the lack of .get_rate() method in the CCF's API. 4. The CCF driver for imx6q uses clks: ccm@020c4000 { } Node as a "manager". In its probe we create other (proper) clocks (hierarchy). Also, this driver is a starting point for getting access to other clocks. As said above - I will first post the CCF port very similar to Barebox/Linux. Afterwards, we can decide if and how we optimise it. > > > > >> > >> Barebox imports the clock driver from the kernel and it is much > >> cleaner: > >> https://git.pengutronix.de/cgit/barebox/tree/drivers/clk/imx/clk-imx6.c > > > > Yes, it has been trimmed (...a bit...) when compared to original > > v4.20 :-) . > > > Best regards, > Stefano > Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
On Tue, Jan 29, 2019 at 12:46 PM Lukasz Majewski <lukma@denx.de> wrote: > > Hi Stefano, Fabio, > > > Hi Lukasz, > > > > On 21/01/19 15:19, Lukasz Majewski wrote: > > > Hi Fabio, > > > > > >> Hi Lukasz, > > >> > > >> On Sat, Jan 19, 2019 at 7:15 AM Lukasz Majewski <lukma@denx.de> > > >> wrote: > > >>> +static ulong imx6q_clk_get_rate(struct clk *clk) > > >>> +{ > > >>> + ulong rate = 0; > > >>> + > > >>> + debug("%s(#%lu)\n", __func__, clk->id); > > >>> + > > >>> + switch (clk->id) { > > >>> + case IMX6QDL_CLK_ECSPI1: > > >>> + case IMX6QDL_CLK_ECSPI2: > > >>> + case IMX6QDL_CLK_ECSPI3: > > >>> + case IMX6QDL_CLK_ECSPI4: > > >>> + return imx6_get_cspi_clk(); > > >>> + > > >>> + case IMX6QDL_CLK_USDHC1: > > >>> + case IMX6QDL_CLK_USDHC2: > > >>> + case IMX6QDL_CLK_USDHC3: > > >>> + case IMX6QDL_CLK_USDHC4: > > >>> + return imx6_get_usdhc_clk(clk->id - > > >>> IMX6QDL_CLK_USDHC1); > > >> > > >> I don't think this scales well as this needs to grow for all other > > >> peripherals and for each port instance. > > > > > > > I am hiiting the same doubts as Fabio when I reviewed Peng's patches > > with clocks for i.MX8M. The current approach was introduced some years > > ago with i.MX5, but it does not fit well now and it does not scale. > > > > > > > The rationale regarding this approach: > > > > > > 1. Reuse the clock.c code for iMX6Q as much as possible. > > > > > > 2, This code is based on the clk-imx8q.c file - hence the question > > > why the Linux clock API was not ported for this new SoC?. > > > > Many reasons, first there was already a set of functions to get / set > > clocks coming from previos i.MX platforms, and this API was simply > > reused. And nobody was in charge to port the clock API. > > > > > > > >> > > >> If we are adding a clock driver for mx6, why don't we add it just > > >> like the kernel one? > > > > > > I can try to port the Linux code, but IMHO it would be feasible to > > > port only relevant (ECSPI, USDHC) parts of it (not all as I cannot > > > test it all properly). > > > > Fully agree. Further parts will be added on demand. Less is better. I > > disagree to add not tested code. > > The work is in progress - I will add the same directory structure as > Linux's Common Clock Framework [CCF]. I shall finish in 2-3 days. > > There are a few problems to tackle (when porting code from Linux): Just to add my experience with previous CLK support series[1]. Do we really need to port it from Linux, because I'm thinking we can manage it as simple as other SoC support in U-Boot.(I have few clocks addition on this series other than ENET) What do you think? [1] https://patchwork.ozlabs.org/cover/950964/
Hi Jagan, > On Tue, Jan 29, 2019 at 12:46 PM Lukasz Majewski <lukma@denx.de> > wrote: > > > > Hi Stefano, Fabio, > > > > > Hi Lukasz, > > > > > > On 21/01/19 15:19, Lukasz Majewski wrote: > > > > Hi Fabio, > > > > > > > >> Hi Lukasz, > > > >> > > > >> On Sat, Jan 19, 2019 at 7:15 AM Lukasz Majewski <lukma@denx.de> > > > >> wrote: > > > >>> +static ulong imx6q_clk_get_rate(struct clk *clk) > > > >>> +{ > > > >>> + ulong rate = 0; > > > >>> + > > > >>> + debug("%s(#%lu)\n", __func__, clk->id); > > > >>> + > > > >>> + switch (clk->id) { > > > >>> + case IMX6QDL_CLK_ECSPI1: > > > >>> + case IMX6QDL_CLK_ECSPI2: > > > >>> + case IMX6QDL_CLK_ECSPI3: > > > >>> + case IMX6QDL_CLK_ECSPI4: > > > >>> + return imx6_get_cspi_clk(); > > > >>> + > > > >>> + case IMX6QDL_CLK_USDHC1: > > > >>> + case IMX6QDL_CLK_USDHC2: > > > >>> + case IMX6QDL_CLK_USDHC3: > > > >>> + case IMX6QDL_CLK_USDHC4: > > > >>> + return imx6_get_usdhc_clk(clk->id - > > > >>> IMX6QDL_CLK_USDHC1); > > > >> > > > >> I don't think this scales well as this needs to grow for all > > > >> other peripherals and for each port instance. > > > > > > > > > > I am hiiting the same doubts as Fabio when I reviewed Peng's > > > patches with clocks for i.MX8M. The current approach was > > > introduced some years ago with i.MX5, but it does not fit well > > > now and it does not scale. > > > > > > > > > > The rationale regarding this approach: > > > > > > > > 1. Reuse the clock.c code for iMX6Q as much as possible. > > > > > > > > 2, This code is based on the clk-imx8q.c file - hence the > > > > question why the Linux clock API was not ported for this new > > > > SoC?. > > > > > > Many reasons, first there was already a set of functions to get / > > > set clocks coming from previos i.MX platforms, and this API was > > > simply reused. And nobody was in charge to port the clock API. > > > > > > > > > > >> > > > >> If we are adding a clock driver for mx6, why don't we add it > > > >> just like the kernel one? > > > > > > > > I can try to port the Linux code, but IMHO it would be feasible > > > > to port only relevant (ECSPI, USDHC) parts of it (not all as I > > > > cannot test it all properly). > > > > > > Fully agree. Further parts will be added on demand. Less is > > > better. I disagree to add not tested code. > > > > The work is in progress - I will add the same directory structure as > > Linux's Common Clock Framework [CCF]. I shall finish in 2-3 days. > > > > There are a few problems to tackle (when porting code from Linux): > > Just to add my experience with previous CLK support series[1]. Do we > really need to port it from Linux, because I'm thinking we can manage > it as simple as other SoC support in U-Boot.(I have few clocks > addition on this series other than ENET) > > What do you think? This patch series seems to me like the one which reuses the "large" switch/case approach with "fsl,imx6q-ccm" driver [*]. The ENET shall also be handled by the CCF from Linux. Another problem - the U-boot's clock DM API is not supporting passing parent clock rate, which is important in the hierarchical clock tree. The Linux CCF is a hierarchical one built in the driver's [*] probe. Do we need to port the (trimmed) Common Clock Framework [CCF] from Linux? With the first version of this patch I also wanted to re-use the code from clock.c imx6 file. This would be good enough for now. However, in the long term (and taking into consideration the fact that imx6 needs to be converted to DM anyway) it may be beneficial to re-use CCF. The _real_ problem is to fit it and reuse in SPL (to avoid footprint size regression). The CCF port will work correctly in u-boot proper, but for SPL we would need some hacks (as e.g. by default we now strip clock properties from DTS when appending to SPL). Now SPLs for IMX6 use solid code without any unneeded stuff (and hence we didn't need TPL...) . > > [1] https://patchwork.ozlabs.org/cover/950964/ Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
On Tue, Jan 29, 2019 at 1:35 PM Lukasz Majewski <lukma@denx.de> wrote: > > Hi Jagan, > > > On Tue, Jan 29, 2019 at 12:46 PM Lukasz Majewski <lukma@denx.de> > > wrote: > > > > > > Hi Stefano, Fabio, > > > > > > > Hi Lukasz, > > > > > > > > On 21/01/19 15:19, Lukasz Majewski wrote: > > > > > Hi Fabio, > > > > > > > > > >> Hi Lukasz, > > > > >> > > > > >> On Sat, Jan 19, 2019 at 7:15 AM Lukasz Majewski <lukma@denx.de> > > > > >> wrote: > > > > >>> +static ulong imx6q_clk_get_rate(struct clk *clk) > > > > >>> +{ > > > > >>> + ulong rate = 0; > > > > >>> + > > > > >>> + debug("%s(#%lu)\n", __func__, clk->id); > > > > >>> + > > > > >>> + switch (clk->id) { > > > > >>> + case IMX6QDL_CLK_ECSPI1: > > > > >>> + case IMX6QDL_CLK_ECSPI2: > > > > >>> + case IMX6QDL_CLK_ECSPI3: > > > > >>> + case IMX6QDL_CLK_ECSPI4: > > > > >>> + return imx6_get_cspi_clk(); > > > > >>> + > > > > >>> + case IMX6QDL_CLK_USDHC1: > > > > >>> + case IMX6QDL_CLK_USDHC2: > > > > >>> + case IMX6QDL_CLK_USDHC3: > > > > >>> + case IMX6QDL_CLK_USDHC4: > > > > >>> + return imx6_get_usdhc_clk(clk->id - > > > > >>> IMX6QDL_CLK_USDHC1); > > > > >> > > > > >> I don't think this scales well as this needs to grow for all > > > > >> other peripherals and for each port instance. > > > > > > > > > > > > > I am hiiting the same doubts as Fabio when I reviewed Peng's > > > > patches with clocks for i.MX8M. The current approach was > > > > introduced some years ago with i.MX5, but it does not fit well > > > > now and it does not scale. > > > > > > > > > > > > > The rationale regarding this approach: > > > > > > > > > > 1. Reuse the clock.c code for iMX6Q as much as possible. > > > > > > > > > > 2, This code is based on the clk-imx8q.c file - hence the > > > > > question why the Linux clock API was not ported for this new > > > > > SoC?. > > > > > > > > Many reasons, first there was already a set of functions to get / > > > > set clocks coming from previos i.MX platforms, and this API was > > > > simply reused. And nobody was in charge to port the clock API. > > > > > > > > > > > > > >> > > > > >> If we are adding a clock driver for mx6, why don't we add it > > > > >> just like the kernel one? > > > > > > > > > > I can try to port the Linux code, but IMHO it would be feasible > > > > > to port only relevant (ECSPI, USDHC) parts of it (not all as I > > > > > cannot test it all properly). > > > > > > > > Fully agree. Further parts will be added on demand. Less is > > > > better. I disagree to add not tested code. > > > > > > The work is in progress - I will add the same directory structure as > > > Linux's Common Clock Framework [CCF]. I shall finish in 2-3 days. > > > > > > There are a few problems to tackle (when porting code from Linux): > > > > Just to add my experience with previous CLK support series[1]. Do we > > really need to port it from Linux, because I'm thinking we can manage > > it as simple as other SoC support in U-Boot.(I have few clocks > > addition on this series other than ENET) > > > > What do you think? > > This patch series seems to me like the one which reuses the "large" > switch/case approach with "fsl,imx6q-ccm" driver [*]. > > The ENET shall also be handled by the CCF from Linux. > > Another problem - the U-boot's clock DM API is not supporting passing > parent clock rate, which is important in the hierarchical clock tree. > The Linux CCF is a hierarchical one built in the driver's [*] probe. Understand, we need to taken care the parent clock rates as well for set and get rates. In fact we tried similar thing with sunxi, patch[2] implements get_rate by taking parent clock rates in to account by using switch statements. and this change[3] do add same but with recursive calls. Since U-Boot need minimal clocks and most of them run with default parents it better to implement as smooth as possible otherwise it would ended-up size and complex issues, IMHO. I think other platforms like mediatek clocks are also following same. > > > > Do we need to port the (trimmed) Common Clock Framework [CCF] from > Linux? > > With the first version of this patch I also wanted to re-use the code > from clock.c imx6 file. This would be good enough for now. > > However, in the long term (and taking into consideration the fact that > imx6 needs to be converted to DM anyway) it may be beneficial to re-use > CCF. > > The _real_ problem is to fit it and reuse in SPL (to avoid footprint > size regression). The CCF port will work correctly in u-boot proper, > but for SPL we would need some hacks (as e.g. by default we now strip > clock properties from DTS when appending to SPL). > > Now SPLs for IMX6 use solid code without any unneeded stuff (and > hence we didn't need TPL...) . Yes, all these size issues might occur even in future if we port large things from Linux. [2] https://patchwork.ozlabs.org/patch/1019673/ [3] https://gist.github.com/apritzel/db93dd06b4defb46504bccbfe4fc2c20#file-sunxi_clk-c-L86-L112
Hi Jagan, > On Tue, Jan 29, 2019 at 1:35 PM Lukasz Majewski <lukma@denx.de> wrote: > > > > Hi Jagan, > > > > > On Tue, Jan 29, 2019 at 12:46 PM Lukasz Majewski <lukma@denx.de> > > > wrote: > > > > > > > > Hi Stefano, Fabio, > > > > > > > > > Hi Lukasz, > > > > > > > > > > On 21/01/19 15:19, Lukasz Majewski wrote: > > > > > > Hi Fabio, > > > > > > > > > > > >> Hi Lukasz, > > > > > >> > > > > > >> On Sat, Jan 19, 2019 at 7:15 AM Lukasz Majewski > > > > > >> <lukma@denx.de> wrote: > > > > > >>> +static ulong imx6q_clk_get_rate(struct clk *clk) > > > > > >>> +{ > > > > > >>> + ulong rate = 0; > > > > > >>> + > > > > > >>> + debug("%s(#%lu)\n", __func__, clk->id); > > > > > >>> + > > > > > >>> + switch (clk->id) { > > > > > >>> + case IMX6QDL_CLK_ECSPI1: > > > > > >>> + case IMX6QDL_CLK_ECSPI2: > > > > > >>> + case IMX6QDL_CLK_ECSPI3: > > > > > >>> + case IMX6QDL_CLK_ECSPI4: > > > > > >>> + return imx6_get_cspi_clk(); > > > > > >>> + > > > > > >>> + case IMX6QDL_CLK_USDHC1: > > > > > >>> + case IMX6QDL_CLK_USDHC2: > > > > > >>> + case IMX6QDL_CLK_USDHC3: > > > > > >>> + case IMX6QDL_CLK_USDHC4: > > > > > >>> + return imx6_get_usdhc_clk(clk->id - > > > > > >>> IMX6QDL_CLK_USDHC1); > > > > > >> > > > > > >> I don't think this scales well as this needs to grow for > > > > > >> all other peripherals and for each port instance. > > > > > > > > > > > > > > > > I am hiiting the same doubts as Fabio when I reviewed Peng's > > > > > patches with clocks for i.MX8M. The current approach was > > > > > introduced some years ago with i.MX5, but it does not fit well > > > > > now and it does not scale. > > > > > > > > > > > > > > > > The rationale regarding this approach: > > > > > > > > > > > > 1. Reuse the clock.c code for iMX6Q as much as possible. > > > > > > > > > > > > 2, This code is based on the clk-imx8q.c file - hence the > > > > > > question why the Linux clock API was not ported for this new > > > > > > SoC?. > > > > > > > > > > Many reasons, first there was already a set of functions to > > > > > get / set clocks coming from previos i.MX platforms, and this > > > > > API was simply reused. And nobody was in charge to port the > > > > > clock API. > > > > > > > > > > > >> > > > > > >> If we are adding a clock driver for mx6, why don't we add > > > > > >> it just like the kernel one? > > > > > > > > > > > > I can try to port the Linux code, but IMHO it would be > > > > > > feasible to port only relevant (ECSPI, USDHC) parts of it > > > > > > (not all as I cannot test it all properly). > > > > > > > > > > Fully agree. Further parts will be added on demand. Less is > > > > > better. I disagree to add not tested code. > > > > > > > > The work is in progress - I will add the same directory > > > > structure as Linux's Common Clock Framework [CCF]. I shall > > > > finish in 2-3 days. > > > > > > > > There are a few problems to tackle (when porting code from > > > > Linux): > > > > > > Just to add my experience with previous CLK support series[1]. Do > > > we really need to port it from Linux, because I'm thinking we can > > > manage it as simple as other SoC support in U-Boot.(I have few > > > clocks addition on this series other than ENET) > > > > > > What do you think? > > > > This patch series seems to me like the one which reuses the "large" > > switch/case approach with "fsl,imx6q-ccm" driver [*]. > > > > The ENET shall also be handled by the CCF from Linux. > > > > Another problem - the U-boot's clock DM API is not supporting > > passing parent clock rate, which is important in the hierarchical > > clock tree. The Linux CCF is a hierarchical one built in the > > driver's [*] probe. > > Understand, we need to taken care the parent clock rates as well for > set and get rates. In fact we tried similar thing with sunxi, patch[2] > implements get_rate by taking parent clock rates in to account by > using switch statements. and this change[3] do add same but with > recursive calls. Since U-Boot need minimal clocks and most of them run > with default parents it better to implement as smooth as possible > otherwise it would ended-up size and complex issues, IMHO. > > I think other platforms like mediatek clocks are also following same. I've posted some patches today: http://patchwork.ozlabs.org/cover/1034127/ > > > > > > > > > Do we need to port the (trimmed) Common Clock Framework [CCF] from > > Linux? > > > > With the first version of this patch I also wanted to re-use the > > code from clock.c imx6 file. This would be good enough for now. > > > > However, in the long term (and taking into consideration the fact > > that imx6 needs to be converted to DM anyway) it may be beneficial > > to re-use CCF. > > > > The _real_ problem is to fit it and reuse in SPL (to avoid footprint > > size regression). The CCF port will work correctly in u-boot proper, > > but for SPL we would need some hacks (as e.g. by default we now > > strip clock properties from DTS when appending to SPL). > > > > Now SPLs for IMX6 use solid code without any unneeded stuff (and > > hence we didn't need TPL...) . > > Yes, all these size issues might occur even in future if we port large > things from Linux. > > [2] https://patchwork.ozlabs.org/patch/1019673/ > [3] > https://gist.github.com/apritzel/db93dd06b4defb46504bccbfe4fc2c20#file-sunxi_clk-c-L86-L112 Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
diff --git a/drivers/clk/imx/Kconfig b/drivers/clk/imx/Kconfig index a6fb58d6cf..7dc261f23e 100644 --- a/drivers/clk/imx/Kconfig +++ b/drivers/clk/imx/Kconfig @@ -1,3 +1,10 @@ +config CLK_IMX6Q + bool "Clock support for i.MX6Q" + depends on ARCH_MX6 + select CLK + help + This enables support clock driver for i.MX6Q platforms. + config CLK_IMX8 bool "Clock support for i.MX8" depends on ARCH_IMX8 diff --git a/drivers/clk/imx/Makefile b/drivers/clk/imx/Makefile index 5505ae52e2..b32744812f 100644 --- a/drivers/clk/imx/Makefile +++ b/drivers/clk/imx/Makefile @@ -2,4 +2,5 @@ # # SPDX-License-Identifier: GPL-2.0 +obj-$(CONFIG_CLK_IMX6Q) += clk-imx6q.o obj-$(CONFIG_CLK_IMX8) += clk-imx8.o diff --git a/drivers/clk/imx/clk-imx6q.c b/drivers/clk/imx/clk-imx6q.c new file mode 100644 index 0000000000..a0aa1f5f45 --- /dev/null +++ b/drivers/clk/imx/clk-imx6q.c @@ -0,0 +1,176 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2019 DENX Software Engineering + * Lukasz Majewski, DENX Software Engineering, lukma@denx.de + * + * Based on: clk-imx8.c + */ + +#include <common.h> +#include <clk-uclass.h> +#include <dm.h> +#include <asm/arch/clock.h> +#include <dt-bindings/clock/imx6qdl-clock.h> + +struct imx6q_clks { + ulong id; + const char *name; +}; + +static struct imx6q_clks imx6q_clk_names[] = { + { IMX6QDL_CLK_ECSPI1, "ECSPI1" }, + { IMX6QDL_CLK_ECSPI2, "ECSPI2" }, + { IMX6QDL_CLK_ECSPI3, "ECSPI3" }, + { IMX6QDL_CLK_ECSPI4, "ECSPI4" }, + { IMX6QDL_CLK_USDHC1, "USDHC1" }, + { IMX6QDL_CLK_USDHC2, "USDHC2" }, + { IMX6QDL_CLK_USDHC3, "USDHC3" }, + { IMX6QDL_CLK_USDHC4, "USDHC4" }, +}; + +static ulong imx6q_clk_get_rate(struct clk *clk) +{ + ulong rate = 0; + + debug("%s(#%lu)\n", __func__, clk->id); + + switch (clk->id) { + case IMX6QDL_CLK_ECSPI1: + case IMX6QDL_CLK_ECSPI2: + case IMX6QDL_CLK_ECSPI3: + case IMX6QDL_CLK_ECSPI4: + return imx6_get_cspi_clk(); + + case IMX6QDL_CLK_USDHC1: + case IMX6QDL_CLK_USDHC2: + case IMX6QDL_CLK_USDHC3: + case IMX6QDL_CLK_USDHC4: + return imx6_get_usdhc_clk(clk->id - IMX6QDL_CLK_USDHC1); + default: + if (clk->id < IMX6QDL_CLK_DUMMY || clk->id >= IMX6QDL_CLK_END) { + printf("%s(Invalid clk ID #%lu)\n", __func__, clk->id); + return -EINVAL; + } + return -ENOTSUPP; + } + + return rate; +} + +static ulong imx6q_clk_set_rate(struct clk *clk, unsigned long rate) +{ + debug("%s(#%lu), rate: %lu\n", __func__, clk->id, rate); + + return rate; +} + +static int __imx6q_clk_enable(struct clk *clk, bool enable) +{ + debug("%s(#%lu) en: %d\n", __func__, clk->id, enable); + + switch (clk->id) { + case IMX6QDL_CLK_ECSPI1: + case IMX6QDL_CLK_ECSPI2: + case IMX6QDL_CLK_ECSPI3: + case IMX6QDL_CLK_ECSPI4: + return enable_spi_clk(enable, clk->id - IMX6QDL_CLK_ECSPI1); + + case IMX6QDL_CLK_USDHC1: + case IMX6QDL_CLK_USDHC2: + case IMX6QDL_CLK_USDHC3: + case IMX6QDL_CLK_USDHC4: + return enable_usdhc_clk(enable, clk->id - IMX6QDL_CLK_USDHC1); + + default: + if (clk->id < IMX6QDL_CLK_DUMMY || clk->id >= IMX6QDL_CLK_END) { + printf("%s(Invalid clk ID #%lu)\n", __func__, clk->id); + return -EINVAL; + } + return -ENOTSUPP; + } + + return 0; +} + +static int imx6q_clk_disable(struct clk *clk) +{ + return __imx6q_clk_enable(clk, 0); +} + +static int imx6q_clk_enable(struct clk *clk) +{ + return __imx6q_clk_enable(clk, 1); +} + +#if CONFIG_IS_ENABLED(CMD_CLK) +int soc_clk_dump(void) +{ + struct udevice *dev; + struct clk clk; + unsigned long rate; + int i, ret; + + ret = uclass_get_device_by_driver(UCLASS_CLK, + DM_GET_DRIVER(imx6q_clk), &dev); + if (ret) + return ret; + + printf("Clk\t\tHz\n"); + + for (i = 0; i < ARRAY_SIZE(imx6q_clk_names); i++) { + clk.id = imx6q_clk_names[i].id; + ret = clk_request(dev, &clk); + if (ret < 0) { + debug("%s clk_request() failed: %d\n", __func__, ret); + continue; + } + + ret = clk_get_rate(&clk); + rate = ret; + + clk_free(&clk); + + if (ret == -ENOTSUPP) { + printf("clk ID %lu not supported yet\n", + imx6q_clk_names[i].id); + continue; + } + if (ret < 0) { + printf("%s %lu: get_rate err: %d\n", + __func__, imx6q_clk_names[i].id, ret); + continue; + } + + printf("%s(%3lu):\t%lu\n", + imx6q_clk_names[i].name, imx6q_clk_names[i].id, rate); + } + + return 0; +} +#endif + +static struct clk_ops imx6q_clk_ops = { + .set_rate = imx6q_clk_set_rate, + .get_rate = imx6q_clk_get_rate, + .enable = imx6q_clk_enable, + .disable = imx6q_clk_disable, +}; + +static int imx6q_clk_probe(struct udevice *dev) +{ + return 0; +} + +static const struct udevice_id imx6q_clk_ids[] = { + { .compatible = "fsl,imx6q-ccm" }, + { }, +}; + +U_BOOT_DRIVER(imx6q_clk) = { + .name = "clk_imx6q", + .id = UCLASS_CLK, + .of_match = imx6q_clk_ids, + .ops = &imx6q_clk_ops, + .probe = imx6q_clk_probe, + .flags = DM_FLAG_PRE_RELOC, +};
This commit provides support for setting USDHCx/ECSPIx clocks depending on used bus. Moreover, it is agnostic to the alias numbering as the information about the clock is read from device tree. Last but not least - the current IMX6Q clock code in mach-imx/mx6/clock.c has been reused to avoid code duplication. Code from this file will be moved to clk-imx6q.c when other iMX6Q based boards adopt usage of this driver. Signed-off-by: Lukasz Majewski <lukma@denx.de> FIX: clk-imx6q --- drivers/clk/imx/Kconfig | 7 ++ drivers/clk/imx/Makefile | 1 + drivers/clk/imx/clk-imx6q.c | 176 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 184 insertions(+) create mode 100644 drivers/clk/imx/clk-imx6q.c