| Message ID | 20171013151817.4588-1-Eugeniy.Paltsev@synopsys.com |
|---|---|
| State | Superseded |
| Delegated to: | Jagannadha Sutradharudu Teki |
| Headers | show |
| Series | [U-Boot,v2] DW SPI: Get clock value from Device Tree | expand |
On Fri, Oct 13, 2017 at 8:48 PM, Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com> wrote: > Add option to set spi controller clock frequency via device tree > using standard clock bindings. > Old way of setting spi controller clock frequency (via implementation > of 'cm_get_spi_controller_clk_hz' function in platform specific code) > remains supported for backward compatibility. > > Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com> > --- > Changes v1->v2: > * disable clock if we can't get the rate. > * get rid of cm_get_spi_controller_clk_hz weak declaration. > > drivers/spi/designware_spi.c | 65 +++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 64 insertions(+), 1 deletion(-) > > diff --git a/drivers/spi/designware_spi.c b/drivers/spi/designware_spi.c > index 5aa507b..9eb5b1c 100644 > --- a/drivers/spi/designware_spi.c > +++ b/drivers/spi/designware_spi.c > @@ -11,6 +11,7 @@ > */ > > #include <common.h> > +#include <clk.h> > #include <dm.h> > #include <errno.h> > #include <malloc.h> > @@ -18,7 +19,10 @@ > #include <fdtdec.h> > #include <linux/compat.h> > #include <asm/io.h> > +/* Only SOCFPGA_GEN5 and SOCFPGA_ARRIA10 uses their clock_manager functions */ > +#if defined(CONFIG_TARGET_SOCFPGA_GEN5) || defined(CONFIG_TARGET_SOCFPGA_ARRIA10) > #include <asm/arch/clock_manager.h> > +#endif How hard it is to make others to use clock manager? do you have any list? thanks!
Hi Jagan, On Mon, 2017-10-16 at 22:37 +0530, Jagan Teki wrote: > On Fri, Oct 13, 2017 at 8:48 PM, Eugeniy Paltsev > <Eugeniy.Paltsev@synopsys.com> wrote: > > Add option to set spi controller clock frequency via device tree > > using standard clock bindings. > > Old way of setting spi controller clock frequency (via implementation > > of 'cm_get_spi_controller_clk_hz' function in platform specific code) > > remains supported for backward compatibility. > > > > Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com> > > --- > > Changes v1->v2: > > * disable clock if we can't get the rate. > > * get rid of cm_get_spi_controller_clk_hz weak declaration. > > > > drivers/spi/designware_spi.c | 65 +++++++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 64 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/spi/designware_spi.c b/drivers/spi/designware_spi.c > > index 5aa507b..9eb5b1c 100644 > > --- a/drivers/spi/designware_spi.c > > +++ b/drivers/spi/designware_spi.c > > @@ -11,6 +11,7 @@ > > */ > > > > #include <common.h> > > +#include <clk.h> > > #include <dm.h> > > #include <errno.h> > > #include <malloc.h> > > @@ -18,7 +19,10 @@ > > #include <fdtdec.h> > > #include <linux/compat.h> > > #include <asm/io.h> > > +/* Only SOCFPGA_GEN5 and SOCFPGA_ARRIA10 uses their clock_manager functions */ > > +#if defined(CONFIG_TARGET_SOCFPGA_GEN5) || defined(CONFIG_TARGET_SOCFPGA_ARRIA10) > > #include <asm/arch/clock_manager.h> > > +#endif > > How hard it is to make others to use clock manager? do you have any list? clock_manager.h is an old (and non-generic) way to deal with different clocks. For example in SOCFPGA_GEN5 and SOCFPGA_ARRIA10 clock_manager.h provides cm_get_spi_controller_clk_hz function to deal with spi controller clock. But today we have another, linux-like alternative: to bind clocks via device tree and manipulate with clocks via generic functions provided by clk.h In this patch I added option to get clock via device tree using standard bindings and restrict clock_manager.h functions usage only to targets which still use it, so new targets can simply bind clock via device tree and they do not need to implement/define something in clock_manager.h So we don't need to make others to use clock manager :) > thanks! -- Eugeniy Paltsev
Hi Jagan, > -----Original Message----- > From: Eugeniy Paltsev [mailto:paltsev@synopsys.com] > Sent: Tuesday, October 17, 2017 4:33 PM > To: jagannadh.teki@gmail.com > Cc: u-boot@lists.denx.de; uboot-snps-arc@synopsys.com > Subject: [uboot-snps-arc] Re: [PATCH v2] DW SPI: Get clock value from Device Tree > > > > How hard it is to make others to use clock manager? do you have any list? > > clock_manager.h is an old (and non-generic) way to deal with different clocks. > For example in SOCFPGA_GEN5 and SOCFPGA_ARRIA10 clock_manager.h provides > cm_get_spi_controller_clk_hz function to deal with spi controller clock. > > But today we have another, linux-like alternative: to bind clocks via device tree > and manipulate with clocks via generic functions provided by clk.h > > In this patch I added option to get clock via device tree using standard bindings > and restrict clock_manager.h functions usage only to targets which still use it, > so new targets can simply bind clock via device tree and they do not need to > implement/define something in clock_manager.h > > So we don't need to make others to use clock manager :) Maybe it worth trying the other way around and think about switching SOCFPGA platforms to generic clk framework? Marek, any plans for that? -Alexey
On Tue, Oct 17, 2017 at 8:27 PM, Alexey Brodkin <Alexey.Brodkin@synopsys.com> wrote: > Hi Jagan, > >> -----Original Message----- >> From: Eugeniy Paltsev [mailto:paltsev@synopsys.com] >> Sent: Tuesday, October 17, 2017 4:33 PM >> To: jagannadh.teki@gmail.com >> Cc: u-boot@lists.denx.de; uboot-snps-arc@synopsys.com >> Subject: [uboot-snps-arc] Re: [PATCH v2] DW SPI: Get clock value from Device Tree >> > >> > How hard it is to make others to use clock manager? do you have any list? >> >> clock_manager.h is an old (and non-generic) way to deal with different clocks. >> For example in SOCFPGA_GEN5 and SOCFPGA_ARRIA10 clock_manager.h provides >> cm_get_spi_controller_clk_hz function to deal with spi controller clock. >> >> But today we have another, linux-like alternative: to bind clocks via device tree >> and manipulate with clocks via generic functions provided by clk.h >> >> In this patch I added option to get clock via device tree using standard bindings >> and restrict clock_manager.h functions usage only to targets which still use it, >> so new targets can simply bind clock via device tree and they do not need to >> implement/define something in clock_manager.h >> >> So we don't need to make others to use clock manager :) > > Maybe it worth trying the other way around and think about switching SOCFPGA platforms to > generic clk framework? Yes, ie what exactly I thought of, thanks!
On 10/17/2017 04:57 PM, Alexey Brodkin wrote: > Hi Jagan, > >> -----Original Message----- >> From: Eugeniy Paltsev [mailto:paltsev@synopsys.com] >> Sent: Tuesday, October 17, 2017 4:33 PM >> To: jagannadh.teki@gmail.com >> Cc: u-boot@lists.denx.de; uboot-snps-arc@synopsys.com >> Subject: [uboot-snps-arc] Re: [PATCH v2] DW SPI: Get clock value from Device Tree >>> >>> How hard it is to make others to use clock manager? do you have any list? >> >> clock_manager.h is an old (and non-generic) way to deal with different clocks. >> For example in SOCFPGA_GEN5 and SOCFPGA_ARRIA10 clock_manager.h provides >> cm_get_spi_controller_clk_hz function to deal with spi controller clock. >> >> But today we have another, linux-like alternative: to bind clocks via device tree >> and manipulate with clocks via generic functions provided by clk.h >> >> In this patch I added option to get clock via device tree using standard bindings >> and restrict clock_manager.h functions usage only to targets which still use it, >> so new targets can simply bind clock via device tree and they do not need to >> implement/define something in clock_manager.h >> >> So we don't need to make others to use clock manager :) > > Maybe it worth trying the other way around and think about switching SOCFPGA platforms to > generic clk framework? That'd be real neat. > Marek, any plans for that? Patches welcome, +CC Dinh. > -Alexey >
On Tue, 2017-10-17 at 20:32 +0530, Jagan Teki wrote: > On Tue, Oct 17, 2017 at 8:27 PM, Alexey Brodkin > <Alexey.Brodkin@synopsys.com> wrote: > > Hi Jagan, > > > > > -----Original Message----- > > > From: Eugeniy Paltsev [mailto:paltsev@synopsys.com] > > > Sent: Tuesday, October 17, 2017 4:33 PM > > > To: jagannadh.teki@gmail.com > > > Cc: u-boot@lists.denx.de; uboot-snps-arc@synopsys.com > > > Subject: [uboot-snps-arc] Re: [PATCH v2] DW SPI: Get clock value from Device Tree > > > > > > > > How hard it is to make others to use clock manager? do you have any list? > > > > > > clock_manager.h is an old (and non-generic) way to deal with different clocks. > > > For example in SOCFPGA_GEN5 and SOCFPGA_ARRIA10 clock_manager.h provides > > > cm_get_spi_controller_clk_hz function to deal with spi controller clock. > > > > > > But today we have another, linux-like alternative: to bind clocks via device tree > > > and manipulate with clocks via generic functions provided by clk.h > > > > > > In this patch I added option to get clock via device tree using standard bindings > > > and restrict clock_manager.h functions usage only to targets which still use it, > > > so new targets can simply bind clock via device tree and they do not need to > > > implement/define something in clock_manager.h > > > > > > So we don't need to make others to use clock manager :) > > > > Maybe it worth trying the other way around and think about switching SOCFPGA platforms to > > generic clk framework? > > Yes, ie what exactly I thought of, thanks! I checked cm_get_spi_controller_clk_hz implementation in SOCFPGA_GEN5 and SOCFPGA_ARRIA10: we can't simply replace it with "fixed-clock" driver as it manipulate with real hardware. The only way to do it is to replace SOCFPGA* clock manager functions by real clock driver. And given I don't have mentioned hardware so I barely can help with those improvements on SOCFPGA. That said if there're no short-term plans to switch SOCFPGA to clk framework maybe we'll be OK with my workaround with #ifdefs? -- Eugeniy Paltsev
On 10/19/2017 05:36 PM, Eugeniy Paltsev wrote: > On Tue, 2017-10-17 at 20:32 +0530, Jagan Teki wrote: >> On Tue, Oct 17, 2017 at 8:27 PM, Alexey Brodkin >> <Alexey.Brodkin@synopsys.com> wrote: >>> Hi Jagan, >>> >>>> -----Original Message----- >>>> From: Eugeniy Paltsev [mailto:paltsev@synopsys.com] >>>> Sent: Tuesday, October 17, 2017 4:33 PM >>>> To: jagannadh.teki@gmail.com >>>> Cc: u-boot@lists.denx.de; uboot-snps-arc@synopsys.com >>>> Subject: [uboot-snps-arc] Re: [PATCH v2] DW SPI: Get clock value from Device Tree >>>>> >>>>> How hard it is to make others to use clock manager? do you have any list? >>>> >>>> clock_manager.h is an old (and non-generic) way to deal with different clocks. >>>> For example in SOCFPGA_GEN5 and SOCFPGA_ARRIA10 clock_manager.h provides >>>> cm_get_spi_controller_clk_hz function to deal with spi controller clock. >>>> >>>> But today we have another, linux-like alternative: to bind clocks via device tree >>>> and manipulate with clocks via generic functions provided by clk.h >>>> >>>> In this patch I added option to get clock via device tree using standard bindings >>>> and restrict clock_manager.h functions usage only to targets which still use it, >>>> so new targets can simply bind clock via device tree and they do not need to >>>> implement/define something in clock_manager.h >>>> >>>> So we don't need to make others to use clock manager :) >>> >>> Maybe it worth trying the other way around and think about switching SOCFPGA platforms to >>> generic clk framework? >> >> Yes, ie what exactly I thought of, thanks! > > I checked cm_get_spi_controller_clk_hz implementation in SOCFPGA_GEN5 and > SOCFPGA_ARRIA10: we can't simply replace it with "fixed-clock" driver as it > manipulate with real hardware. > The only way to do it is to replace SOCFPGA* clock manager functions by real > clock driver. > > And given I don't have mentioned hardware so I barely can help with > those improvements on SOCFPGA. That said if there're no short-term plans to > switch SOCFPGA to clk framework maybe we'll be OK with my workaround with #ifdefs? Wait for Dinh's reply ...
On 10/19/2017 10:51 AM, Marek Vasut wrote: > On 10/19/2017 05:36 PM, Eugeniy Paltsev wrote: >> On Tue, 2017-10-17 at 20:32 +0530, Jagan Teki wrote: >>> On Tue, Oct 17, 2017 at 8:27 PM, Alexey Brodkin >>> <Alexey.Brodkin@synopsys.com> wrote: >>>> Hi Jagan, >>>> >>>>> -----Original Message----- >>>>> From: Eugeniy Paltsev [mailto:paltsev@synopsys.com] >>>>> Sent: Tuesday, October 17, 2017 4:33 PM >>>>> To: jagannadh.teki@gmail.com >>>>> Cc: u-boot@lists.denx.de; uboot-snps-arc@synopsys.com >>>>> Subject: [uboot-snps-arc] Re: [PATCH v2] DW SPI: Get clock value from Device Tree >>>>>> >>>>>> How hard it is to make others to use clock manager? do you have any list? >>>>> >>>>> clock_manager.h is an old (and non-generic) way to deal with different clocks. >>>>> For example in SOCFPGA_GEN5 and SOCFPGA_ARRIA10 clock_manager.h provides >>>>> cm_get_spi_controller_clk_hz function to deal with spi controller clock. >>>>> >>>>> But today we have another, linux-like alternative: to bind clocks via device tree >>>>> and manipulate with clocks via generic functions provided by clk.h >>>>> >>>>> In this patch I added option to get clock via device tree using standard bindings >>>>> and restrict clock_manager.h functions usage only to targets which still use it, >>>>> so new targets can simply bind clock via device tree and they do not need to >>>>> implement/define something in clock_manager.h >>>>> >>>>> So we don't need to make others to use clock manager :) >>>> >>>> Maybe it worth trying the other way around and think about switching SOCFPGA platforms to >>>> generic clk framework? >>> >>> Yes, ie what exactly I thought of, thanks! >> >> I checked cm_get_spi_controller_clk_hz implementation in SOCFPGA_GEN5 and >> SOCFPGA_ARRIA10: we can't simply replace it with "fixed-clock" driver as it >> manipulate with real hardware. >> The only way to do it is to replace SOCFPGA* clock manager functions by real >> clock driver. >> >> And given I don't have mentioned hardware so I barely can help with >> those improvements on SOCFPGA. That said if there're no short-term plans to >> switch SOCFPGA to clk framework maybe we'll be OK with my workaround with #ifdefs? > > Wait for Dinh's reply ... > Honestly, I don't have too much time to work on this right now. So I really don't when it can get done. But it'll go on my to-do list. Dinh
On Thu, 2017-10-19 at 13:20 -0500, Dinh Nguyen wrote: > > On 10/19/2017 10:51 AM, Marek Vasut wrote: > > On 10/19/2017 05:36 PM, Eugeniy Paltsev wrote: > > > On Tue, 2017-10-17 at 20:32 +0530, Jagan Teki wrote: > > > > On Tue, Oct 17, 2017 at 8:27 PM, Alexey Brodkin > > > > <Alexey.Brodkin@synopsys.com> wrote: > > > > > Hi Jagan, > > > > > > > > > > > -----Original Message----- > > > > > > From: Eugeniy Paltsev [mailto:paltsev@synopsys.com] > > > > > > Sent: Tuesday, October 17, 2017 4:33 PM > > > > > > To: jagannadh.teki@gmail.com > > > > > > Cc: u-boot@lists.denx.de; uboot-snps-arc@synopsys.com > > > > > > Subject: [uboot-snps-arc] Re: [PATCH v2] DW SPI: Get clock value from Device Tree > > > > > > > > > > > > > > How hard it is to make others to use clock manager? do you have any list? > > > > > > > > > > > > clock_manager.h is an old (and non-generic) way to deal with different clocks. > > > > > > For example in SOCFPGA_GEN5 and SOCFPGA_ARRIA10 clock_manager.h provides > > > > > > cm_get_spi_controller_clk_hz function to deal with spi controller clock. > > > > > > > > > > > > But today we have another, linux-like alternative: to bind clocks via device tree > > > > > > and manipulate with clocks via generic functions provided by clk.h > > > > > > > > > > > > In this patch I added option to get clock via device tree using standard bindings > > > > > > and restrict clock_manager.h functions usage only to targets which still use it, > > > > > > so new targets can simply bind clock via device tree and they do not need to > > > > > > implement/define something in clock_manager.h > > > > > > > > > > > > So we don't need to make others to use clock manager :) > > > > > > > > > > Maybe it worth trying the other way around and think about switching SOCFPGA platforms to > > > > > generic clk framework? > > > > > > > > Yes, ie what exactly I thought of, thanks! > > > > > > I checked cm_get_spi_controller_clk_hz implementation in SOCFPGA_GEN5 and > > > SOCFPGA_ARRIA10: we can't simply replace it with "fixed-clock" driver as it > > > manipulate with real hardware. > > > The only way to do it is to replace SOCFPGA* clock manager functions by real > > > clock driver. > > > > > > And given I don't have mentioned hardware so I barely can help with > > > those improvements on SOCFPGA. That said if there're no short-term plans to > > > switch SOCFPGA to clk framework maybe we'll be OK with my workaround with #ifdefs? > > > > Wait for Dinh's reply ... > > > > Honestly, I don't have too much time to work on this right now. So I > really don't when it can get done. But it'll go on my to-do list. > > Dinh Yep, thanks for your comments. So, Jagan, given Dinh's reply, could you please apply this patch? Thanks. -- Eugeniy Paltsev
On 10/23/2017 01:43 PM, Eugeniy Paltsev wrote: > On Thu, 2017-10-19 at 13:20 -0500, Dinh Nguyen wrote: >> >> On 10/19/2017 10:51 AM, Marek Vasut wrote: >>> On 10/19/2017 05:36 PM, Eugeniy Paltsev wrote: >>>> On Tue, 2017-10-17 at 20:32 +0530, Jagan Teki wrote: >>>>> On Tue, Oct 17, 2017 at 8:27 PM, Alexey Brodkin >>>>> <Alexey.Brodkin@synopsys.com> wrote: >>>>>> Hi Jagan, >>>>>> >>>>>>> -----Original Message----- >>>>>>> From: Eugeniy Paltsev [mailto:paltsev@synopsys.com] >>>>>>> Sent: Tuesday, October 17, 2017 4:33 PM >>>>>>> To: jagannadh.teki@gmail.com >>>>>>> Cc: u-boot@lists.denx.de; uboot-snps-arc@synopsys.com >>>>>>> Subject: [uboot-snps-arc] Re: [PATCH v2] DW SPI: Get clock value from Device Tree >>>>>>>> >>>>>>>> How hard it is to make others to use clock manager? do you have any list? >>>>>>> >>>>>>> clock_manager.h is an old (and non-generic) way to deal with different clocks. >>>>>>> For example in SOCFPGA_GEN5 and SOCFPGA_ARRIA10 clock_manager.h provides >>>>>>> cm_get_spi_controller_clk_hz function to deal with spi controller clock. >>>>>>> >>>>>>> But today we have another, linux-like alternative: to bind clocks via device tree >>>>>>> and manipulate with clocks via generic functions provided by clk.h >>>>>>> >>>>>>> In this patch I added option to get clock via device tree using standard bindings >>>>>>> and restrict clock_manager.h functions usage only to targets which still use it, >>>>>>> so new targets can simply bind clock via device tree and they do not need to >>>>>>> implement/define something in clock_manager.h >>>>>>> >>>>>>> So we don't need to make others to use clock manager :) >>>>>> >>>>>> Maybe it worth trying the other way around and think about switching SOCFPGA platforms to >>>>>> generic clk framework? >>>>> >>>>> Yes, ie what exactly I thought of, thanks! >>>> >>>> I checked cm_get_spi_controller_clk_hz implementation in SOCFPGA_GEN5 and >>>> SOCFPGA_ARRIA10: we can't simply replace it with "fixed-clock" driver as it >>>> manipulate with real hardware. >>>> The only way to do it is to replace SOCFPGA* clock manager functions by real >>>> clock driver. >>>> >>>> And given I don't have mentioned hardware so I barely can help with >>>> those improvements on SOCFPGA. That said if there're no short-term plans to >>>> switch SOCFPGA to clk framework maybe we'll be OK with my workaround with #ifdefs? >>> >>> Wait for Dinh's reply ... >>> >> >> Honestly, I don't have too much time to work on this right now. So I >> really don't when it can get done. But it'll go on my to-do list. >> >> Dinh > > Yep, thanks for your comments. > > So, Jagan, > given Dinh's reply, could you please apply this patch? I'd really hate it to start seeing soc-specific ifdefs in drivers, that's IMO not acceptable. A __weak override might be a temporary solution I'd be willing to live with though.
On Tue, Oct 24, 2017 at 11:38 AM, Marek Vasut <marex@denx.de> wrote: > On 10/23/2017 01:43 PM, Eugeniy Paltsev wrote: >> On Thu, 2017-10-19 at 13:20 -0500, Dinh Nguyen wrote: >>> >>> On 10/19/2017 10:51 AM, Marek Vasut wrote: >>>> On 10/19/2017 05:36 PM, Eugeniy Paltsev wrote: >>>>> On Tue, 2017-10-17 at 20:32 +0530, Jagan Teki wrote: >>>>>> On Tue, Oct 17, 2017 at 8:27 PM, Alexey Brodkin >>>>>> <Alexey.Brodkin@synopsys.com> wrote: >>>>>>> Hi Jagan, >>>>>>> >>>>>>>> -----Original Message----- >>>>>>>> From: Eugeniy Paltsev [mailto:paltsev@synopsys.com] >>>>>>>> Sent: Tuesday, October 17, 2017 4:33 PM >>>>>>>> To: jagannadh.teki@gmail.com >>>>>>>> Cc: u-boot@lists.denx.de; uboot-snps-arc@synopsys.com >>>>>>>> Subject: [uboot-snps-arc] Re: [PATCH v2] DW SPI: Get clock value from Device Tree >>>>>>>>> >>>>>>>>> How hard it is to make others to use clock manager? do you have any list? >>>>>>>> >>>>>>>> clock_manager.h is an old (and non-generic) way to deal with different clocks. >>>>>>>> For example in SOCFPGA_GEN5 and SOCFPGA_ARRIA10 clock_manager.h provides >>>>>>>> cm_get_spi_controller_clk_hz function to deal with spi controller clock. >>>>>>>> >>>>>>>> But today we have another, linux-like alternative: to bind clocks via device tree >>>>>>>> and manipulate with clocks via generic functions provided by clk.h >>>>>>>> >>>>>>>> In this patch I added option to get clock via device tree using standard bindings >>>>>>>> and restrict clock_manager.h functions usage only to targets which still use it, >>>>>>>> so new targets can simply bind clock via device tree and they do not need to >>>>>>>> implement/define something in clock_manager.h >>>>>>>> >>>>>>>> So we don't need to make others to use clock manager :) >>>>>>> >>>>>>> Maybe it worth trying the other way around and think about switching SOCFPGA platforms to >>>>>>> generic clk framework? >>>>>> >>>>>> Yes, ie what exactly I thought of, thanks! >>>>> >>>>> I checked cm_get_spi_controller_clk_hz implementation in SOCFPGA_GEN5 and >>>>> SOCFPGA_ARRIA10: we can't simply replace it with "fixed-clock" driver as it >>>>> manipulate with real hardware. >>>>> The only way to do it is to replace SOCFPGA* clock manager functions by real >>>>> clock driver. >>>>> >>>>> And given I don't have mentioned hardware so I barely can help with >>>>> those improvements on SOCFPGA. That said if there're no short-term plans to >>>>> switch SOCFPGA to clk framework maybe we'll be OK with my workaround with #ifdefs? >>>> >>>> Wait for Dinh's reply ... >>>> >>> >>> Honestly, I don't have too much time to work on this right now. So I >>> really don't when it can get done. But it'll go on my to-do list. >>> >>> Dinh >> >> Yep, thanks for your comments. >> >> So, Jagan, >> given Dinh's reply, could you please apply this patch? > > I'd really hate it to start seeing soc-specific ifdefs in drivers, > that's IMO not acceptable. A __weak override might be a temporary > solution I'd be willing to live with though. I would rather like to see some check on clock manager itself whether CONFIG_IS_ENABLED(CLK) is using or not? this can tends not to use __weak as well soc #ifdefs in driver. thanks!
On 10/24/2017 11:52 AM, Jagan Teki wrote: > On Tue, Oct 24, 2017 at 11:38 AM, Marek Vasut <marex@denx.de> wrote: >> On 10/23/2017 01:43 PM, Eugeniy Paltsev wrote: >>> On Thu, 2017-10-19 at 13:20 -0500, Dinh Nguyen wrote: >>>> >>>> On 10/19/2017 10:51 AM, Marek Vasut wrote: >>>>> On 10/19/2017 05:36 PM, Eugeniy Paltsev wrote: >>>>>> On Tue, 2017-10-17 at 20:32 +0530, Jagan Teki wrote: >>>>>>> On Tue, Oct 17, 2017 at 8:27 PM, Alexey Brodkin >>>>>>> <Alexey.Brodkin@synopsys.com> wrote: >>>>>>>> Hi Jagan, >>>>>>>> >>>>>>>>> -----Original Message----- >>>>>>>>> From: Eugeniy Paltsev [mailto:paltsev@synopsys.com] >>>>>>>>> Sent: Tuesday, October 17, 2017 4:33 PM >>>>>>>>> To: jagannadh.teki@gmail.com >>>>>>>>> Cc: u-boot@lists.denx.de; uboot-snps-arc@synopsys.com >>>>>>>>> Subject: [uboot-snps-arc] Re: [PATCH v2] DW SPI: Get clock value from Device Tree >>>>>>>>>> >>>>>>>>>> How hard it is to make others to use clock manager? do you have any list? >>>>>>>>> >>>>>>>>> clock_manager.h is an old (and non-generic) way to deal with different clocks. >>>>>>>>> For example in SOCFPGA_GEN5 and SOCFPGA_ARRIA10 clock_manager.h provides >>>>>>>>> cm_get_spi_controller_clk_hz function to deal with spi controller clock. >>>>>>>>> >>>>>>>>> But today we have another, linux-like alternative: to bind clocks via device tree >>>>>>>>> and manipulate with clocks via generic functions provided by clk.h >>>>>>>>> >>>>>>>>> In this patch I added option to get clock via device tree using standard bindings >>>>>>>>> and restrict clock_manager.h functions usage only to targets which still use it, >>>>>>>>> so new targets can simply bind clock via device tree and they do not need to >>>>>>>>> implement/define something in clock_manager.h >>>>>>>>> >>>>>>>>> So we don't need to make others to use clock manager :) >>>>>>>> >>>>>>>> Maybe it worth trying the other way around and think about switching SOCFPGA platforms to >>>>>>>> generic clk framework? >>>>>>> >>>>>>> Yes, ie what exactly I thought of, thanks! >>>>>> >>>>>> I checked cm_get_spi_controller_clk_hz implementation in SOCFPGA_GEN5 and >>>>>> SOCFPGA_ARRIA10: we can't simply replace it with "fixed-clock" driver as it >>>>>> manipulate with real hardware. >>>>>> The only way to do it is to replace SOCFPGA* clock manager functions by real >>>>>> clock driver. >>>>>> >>>>>> And given I don't have mentioned hardware so I barely can help with >>>>>> those improvements on SOCFPGA. That said if there're no short-term plans to >>>>>> switch SOCFPGA to clk framework maybe we'll be OK with my workaround with #ifdefs? >>>>> >>>>> Wait for Dinh's reply ... >>>>> >>>> >>>> Honestly, I don't have too much time to work on this right now. So I >>>> really don't when it can get done. But it'll go on my to-do list. >>>> >>>> Dinh >>> >>> Yep, thanks for your comments. >>> >>> So, Jagan, >>> given Dinh's reply, could you please apply this patch? >> >> I'd really hate it to start seeing soc-specific ifdefs in drivers, >> that's IMO not acceptable. A __weak override might be a temporary >> solution I'd be willing to live with though. > > I would rather like to see some check on clock manager itself whether > CONFIG_IS_ENABLED(CLK) is using or not? this can tends not to use > __weak as well soc #ifdefs in driver. I don't understand what you mean by this , can you elucidate ?
On Tue, 2017-10-24 at 15:22 +0530, Jagan Teki wrote: > On Tue, Oct 24, 2017 at 11:38 AM, Marek Vasut <marex@denx.de> wrote: > > On 10/23/2017 01:43 PM, Eugeniy Paltsev wrote: > > > On Thu, 2017-10-19 at 13:20 -0500, Dinh Nguyen wrote: > > > > > > > > On 10/19/2017 10:51 AM, Marek Vasut wrote: > > > > > On 10/19/2017 05:36 PM, Eugeniy Paltsev wrote: > > > > > > On Tue, 2017-10-17 at 20:32 +0530, Jagan Teki wrote: > > > > > > > On Tue, Oct 17, 2017 at 8:27 PM, Alexey Brodkin > > > > > > > <Alexey.Brodkin@synopsys.com> wrote: > > > > > > > > Hi Jagan, > > > > > > > > > > > > > > > > > -----Original Message----- > > > > > > > > > From: Eugeniy Paltsev [mailto:paltsev@synopsys.com] > > > > > > > > > Sent: Tuesday, October 17, 2017 4:33 PM > > > > > > > > > To: jagannadh.teki@gmail.com > > > > > > > > > Cc: u-boot@lists.denx.de; uboot-snps-arc@synopsys.com > > > > > > > > > Subject: [uboot-snps-arc] Re: [PATCH v2] DW SPI: Get clock value from Device Tree > > > > > > > > > > > > > > > > > > > > How hard it is to make others to use clock manager? do you have any list? > > > > > > > > > > > > > > > > > > clock_manager.h is an old (and non-generic) way to deal with different clocks. > > > > > > > > > For example in SOCFPGA_GEN5 and SOCFPGA_ARRIA10 clock_manager.h provides > > > > > > > > > cm_get_spi_controller_clk_hz function to deal with spi controller clock. > > > > > > > > > > > > > > > > > > But today we have another, linux-like alternative: to bind clocks via device tree > > > > > > > > > and manipulate with clocks via generic functions provided by clk.h > > > > > > > > > > > > > > > > > > In this patch I added option to get clock via device tree using standard bindings > > > > > > > > > and restrict clock_manager.h functions usage only to targets which still use it, > > > > > > > > > so new targets can simply bind clock via device tree and they do not need to > > > > > > > > > implement/define something in clock_manager.h > > > > > > > > > > > > > > > > > > So we don't need to make others to use clock manager :) > > > > > > > > > > > > > > > > Maybe it worth trying the other way around and think about switching SOCFPGA platforms to > > > > > > > > generic clk framework? > > > > > > > > > > > > > > Yes, ie what exactly I thought of, thanks! > > > > > > > > > > > > I checked cm_get_spi_controller_clk_hz implementation in SOCFPGA_GEN5 and > > > > > > SOCFPGA_ARRIA10: we can't simply replace it with "fixed-clock" driver as it > > > > > > manipulate with real hardware. > > > > > > The only way to do it is to replace SOCFPGA* clock manager functions by real > > > > > > clock driver. > > > > > > > > > > > > And given I don't have mentioned hardware so I barely can help with > > > > > > those improvements on SOCFPGA. That said if there're no short-term plans to > > > > > > switch SOCFPGA to clk framework maybe we'll be OK with my workaround with #ifdefs? > > > > > > > > > > Wait for Dinh's reply ... > > > > > > > > > > > > > Honestly, I don't have too much time to work on this right now. So I > > > > really don't when it can get done. But it'll go on my to-do list. > > > > > > > > Dinh > > > > > > Yep, thanks for your comments. > > > > > > So, Jagan, > > > given Dinh's reply, could you please apply this patch? > > > > I'd really hate it to start seeing soc-specific ifdefs in drivers, > > that's IMO not acceptable. A __weak override might be a temporary > > solution I'd be willing to live with though. > > I would rather like to see some check on clock manager itself whether > CONFIG_IS_ENABLED(CLK) is using or not? this can tends not to use > __weak as well soc #ifdefs in driver. > Actually I don't understand what do you mean. Even if I add any #ifdefs to the clock_manager.h I still need to wrap clock_manager.h include with #ifdefs as clock_manager.h is defined only for two targets - SOCFPGA_GEN5 and SOCFPGA_ARRIA10. #if defined(CONFIG_TARGET_SOCFPGA_GEN5) || defined(CONFIG_TARGET_SOCFPGA_ARRIA10) #include <asm/arch/clock_manager.h> #endif And I think it is better to add this #ifdef in driver than create empty clock_manager.h file for every new target which uses this driver. -- Eugeniy Paltsev
On 10/27/2017 03:54 PM, Eugeniy Paltsev wrote: > On Tue, 2017-10-24 at 15:22 +0530, Jagan Teki wrote: >> On Tue, Oct 24, 2017 at 11:38 AM, Marek Vasut <marex@denx.de> wrote: >>> On 10/23/2017 01:43 PM, Eugeniy Paltsev wrote: >>>> On Thu, 2017-10-19 at 13:20 -0500, Dinh Nguyen wrote: >>>>> >>>>> On 10/19/2017 10:51 AM, Marek Vasut wrote: >>>>>> On 10/19/2017 05:36 PM, Eugeniy Paltsev wrote: >>>>>>> On Tue, 2017-10-17 at 20:32 +0530, Jagan Teki wrote: >>>>>>>> On Tue, Oct 17, 2017 at 8:27 PM, Alexey Brodkin >>>>>>>> <Alexey.Brodkin@synopsys.com> wrote: >>>>>>>>> Hi Jagan, >>>>>>>>> >>>>>>>>>> -----Original Message----- >>>>>>>>>> From: Eugeniy Paltsev [mailto:paltsev@synopsys.com] >>>>>>>>>> Sent: Tuesday, October 17, 2017 4:33 PM >>>>>>>>>> To: jagannadh.teki@gmail.com >>>>>>>>>> Cc: u-boot@lists.denx.de; uboot-snps-arc@synopsys.com >>>>>>>>>> Subject: [uboot-snps-arc] Re: [PATCH v2] DW SPI: Get clock value from Device Tree >>>>>>>>>>> >>>>>>>>>>> How hard it is to make others to use clock manager? do you have any list? >>>>>>>>>> >>>>>>>>>> clock_manager.h is an old (and non-generic) way to deal with different clocks. >>>>>>>>>> For example in SOCFPGA_GEN5 and SOCFPGA_ARRIA10 clock_manager.h provides >>>>>>>>>> cm_get_spi_controller_clk_hz function to deal with spi controller clock. >>>>>>>>>> >>>>>>>>>> But today we have another, linux-like alternative: to bind clocks via device tree >>>>>>>>>> and manipulate with clocks via generic functions provided by clk.h >>>>>>>>>> >>>>>>>>>> In this patch I added option to get clock via device tree using standard bindings >>>>>>>>>> and restrict clock_manager.h functions usage only to targets which still use it, >>>>>>>>>> so new targets can simply bind clock via device tree and they do not need to >>>>>>>>>> implement/define something in clock_manager.h >>>>>>>>>> >>>>>>>>>> So we don't need to make others to use clock manager :) >>>>>>>>> >>>>>>>>> Maybe it worth trying the other way around and think about switching SOCFPGA platforms to >>>>>>>>> generic clk framework? >>>>>>>> >>>>>>>> Yes, ie what exactly I thought of, thanks! >>>>>>> >>>>>>> I checked cm_get_spi_controller_clk_hz implementation in SOCFPGA_GEN5 and >>>>>>> SOCFPGA_ARRIA10: we can't simply replace it with "fixed-clock" driver as it >>>>>>> manipulate with real hardware. >>>>>>> The only way to do it is to replace SOCFPGA* clock manager functions by real >>>>>>> clock driver. >>>>>>> >>>>>>> And given I don't have mentioned hardware so I barely can help with >>>>>>> those improvements on SOCFPGA. That said if there're no short-term plans to >>>>>>> switch SOCFPGA to clk framework maybe we'll be OK with my workaround with #ifdefs? >>>>>> >>>>>> Wait for Dinh's reply ... >>>>>> >>>>> >>>>> Honestly, I don't have too much time to work on this right now. So I >>>>> really don't when it can get done. But it'll go on my to-do list. >>>>> >>>>> Dinh >>>> >>>> Yep, thanks for your comments. >>>> >>>> So, Jagan, >>>> given Dinh's reply, could you please apply this patch? >>> >>> I'd really hate it to start seeing soc-specific ifdefs in drivers, >>> that's IMO not acceptable. A __weak override might be a temporary >>> solution I'd be willing to live with though. >> >> I would rather like to see some check on clock manager itself whether >> CONFIG_IS_ENABLED(CLK) is using or not? this can tends not to use >> __weak as well soc #ifdefs in driver. >> > > Actually I don't understand what do you mean. > Even if I add any #ifdefs to the clock_manager.h I still need to wrap clock_manager.h > include with #ifdefs as clock_manager.h is defined only for two > targets - SOCFPGA_GEN5 and SOCFPGA_ARRIA10. > > #if defined(CONFIG_TARGET_SOCFPGA_GEN5) || defined(CONFIG_TARGET_SOCFPGA_ARRIA10) > #include <asm/arch/clock_manager.h> > #endif > > And I think it is better to add this #ifdef in driver than create empty > clock_manager.h file for every new target which uses this driver. If you have weak default implementation in the DW SPI driver of some function to get the clock rate and override it in the socfpga clock manager, then you don't need to use ifdefs .
On Sat, Oct 28, 2017 at 5:09 PM, Marek Vasut <marex@denx.de> wrote: > On 10/27/2017 03:54 PM, Eugeniy Paltsev wrote: >> On Tue, 2017-10-24 at 15:22 +0530, Jagan Teki wrote: >>> On Tue, Oct 24, 2017 at 11:38 AM, Marek Vasut <marex@denx.de> wrote: >>>> On 10/23/2017 01:43 PM, Eugeniy Paltsev wrote: >>>>> On Thu, 2017-10-19 at 13:20 -0500, Dinh Nguyen wrote: >>>>>> >>>>>> On 10/19/2017 10:51 AM, Marek Vasut wrote: >>>>>>> On 10/19/2017 05:36 PM, Eugeniy Paltsev wrote: >>>>>>>> On Tue, 2017-10-17 at 20:32 +0530, Jagan Teki wrote: >>>>>>>>> On Tue, Oct 17, 2017 at 8:27 PM, Alexey Brodkin >>>>>>>>> <Alexey.Brodkin@synopsys.com> wrote: >>>>>>>>>> Hi Jagan, >>>>>>>>>> >>>>>>>>>>> -----Original Message----- >>>>>>>>>>> From: Eugeniy Paltsev [mailto:paltsev@synopsys.com] >>>>>>>>>>> Sent: Tuesday, October 17, 2017 4:33 PM >>>>>>>>>>> To: jagannadh.teki@gmail.com >>>>>>>>>>> Cc: u-boot@lists.denx.de; uboot-snps-arc@synopsys.com >>>>>>>>>>> Subject: [uboot-snps-arc] Re: [PATCH v2] DW SPI: Get clock value from Device Tree >>>>>>>>>>>> >>>>>>>>>>>> How hard it is to make others to use clock manager? do you have any list? >>>>>>>>>>> >>>>>>>>>>> clock_manager.h is an old (and non-generic) way to deal with different clocks. >>>>>>>>>>> For example in SOCFPGA_GEN5 and SOCFPGA_ARRIA10 clock_manager.h provides >>>>>>>>>>> cm_get_spi_controller_clk_hz function to deal with spi controller clock. >>>>>>>>>>> >>>>>>>>>>> But today we have another, linux-like alternative: to bind clocks via device tree >>>>>>>>>>> and manipulate with clocks via generic functions provided by clk.h >>>>>>>>>>> >>>>>>>>>>> In this patch I added option to get clock via device tree using standard bindings >>>>>>>>>>> and restrict clock_manager.h functions usage only to targets which still use it, >>>>>>>>>>> so new targets can simply bind clock via device tree and they do not need to >>>>>>>>>>> implement/define something in clock_manager.h >>>>>>>>>>> >>>>>>>>>>> So we don't need to make others to use clock manager :) >>>>>>>>>> >>>>>>>>>> Maybe it worth trying the other way around and think about switching SOCFPGA platforms to >>>>>>>>>> generic clk framework? >>>>>>>>> >>>>>>>>> Yes, ie what exactly I thought of, thanks! >>>>>>>> >>>>>>>> I checked cm_get_spi_controller_clk_hz implementation in SOCFPGA_GEN5 and >>>>>>>> SOCFPGA_ARRIA10: we can't simply replace it with "fixed-clock" driver as it >>>>>>>> manipulate with real hardware. >>>>>>>> The only way to do it is to replace SOCFPGA* clock manager functions by real >>>>>>>> clock driver. >>>>>>>> >>>>>>>> And given I don't have mentioned hardware so I barely can help with >>>>>>>> those improvements on SOCFPGA. That said if there're no short-term plans to >>>>>>>> switch SOCFPGA to clk framework maybe we'll be OK with my workaround with #ifdefs? >>>>>>> >>>>>>> Wait for Dinh's reply ... >>>>>>> >>>>>> >>>>>> Honestly, I don't have too much time to work on this right now. So I >>>>>> really don't when it can get done. But it'll go on my to-do list. >>>>>> >>>>>> Dinh >>>>> >>>>> Yep, thanks for your comments. >>>>> >>>>> So, Jagan, >>>>> given Dinh's reply, could you please apply this patch? >>>> >>>> I'd really hate it to start seeing soc-specific ifdefs in drivers, >>>> that's IMO not acceptable. A __weak override might be a temporary >>>> solution I'd be willing to live with though. >>> >>> I would rather like to see some check on clock manager itself whether >>> CONFIG_IS_ENABLED(CLK) is using or not? this can tends not to use >>> __weak as well soc #ifdefs in driver. >>> >> >> Actually I don't understand what do you mean. >> Even if I add any #ifdefs to the clock_manager.h I still need to wrap clock_manager.h >> include with #ifdefs as clock_manager.h is defined only for two >> targets - SOCFPGA_GEN5 and SOCFPGA_ARRIA10. >> >> #if defined(CONFIG_TARGET_SOCFPGA_GEN5) || defined(CONFIG_TARGET_SOCFPGA_ARRIA10) >> #include <asm/arch/clock_manager.h> >> #endif >> >> And I think it is better to add this #ifdef in driver than create empty >> clock_manager.h file for every new target which uses this driver. Check CLK in clock-manager like below, of-course cm_get_l4_noc_hz used in other IP's as well. we need to carefully adding these check and if require add CLK for remaining drivers as well. the reason for adding checks in clock-manager is it may be removable code if all use CLK. --- a/arch/arm/mach-socfpga/clock_manager_arria10.c +++ b/arch/arm/mach-socfpga/clock_manager_arria10.c @@ -1066,10 +1066,17 @@ unsigned int cm_get_mmc_controller_clk_hz(void) return clk_hz / 4; } +#if CONFIG_IS_ENABLED(OF_CONTROL) && CONFIG_IS_ENABLED(CLK) +unsigned int cm_get_spi_controller_clk_hz(void) +{ + return 0; +} +#else unsigned int cm_get_spi_controller_clk_hz(void) { return cm_get_l4_noc_hz(CLKMGR_MAINPLL_NOCDIV_L4MPCLK_LSB); } +#endif thanks!
On 10/30/2017 07:04 AM, Jagan Teki wrote: > On Sat, Oct 28, 2017 at 5:09 PM, Marek Vasut <marex@denx.de> wrote: >> On 10/27/2017 03:54 PM, Eugeniy Paltsev wrote: >>> On Tue, 2017-10-24 at 15:22 +0530, Jagan Teki wrote: >>>> On Tue, Oct 24, 2017 at 11:38 AM, Marek Vasut <marex@denx.de> wrote: >>>>> On 10/23/2017 01:43 PM, Eugeniy Paltsev wrote: >>>>>> On Thu, 2017-10-19 at 13:20 -0500, Dinh Nguyen wrote: >>>>>>> >>>>>>> On 10/19/2017 10:51 AM, Marek Vasut wrote: >>>>>>>> On 10/19/2017 05:36 PM, Eugeniy Paltsev wrote: >>>>>>>>> On Tue, 2017-10-17 at 20:32 +0530, Jagan Teki wrote: >>>>>>>>>> On Tue, Oct 17, 2017 at 8:27 PM, Alexey Brodkin >>>>>>>>>> <Alexey.Brodkin@synopsys.com> wrote: >>>>>>>>>>> Hi Jagan, >>>>>>>>>>> >>>>>>>>>>>> -----Original Message----- >>>>>>>>>>>> From: Eugeniy Paltsev [mailto:paltsev@synopsys.com] >>>>>>>>>>>> Sent: Tuesday, October 17, 2017 4:33 PM >>>>>>>>>>>> To: jagannadh.teki@gmail.com >>>>>>>>>>>> Cc: u-boot@lists.denx.de; uboot-snps-arc@synopsys.com >>>>>>>>>>>> Subject: [uboot-snps-arc] Re: [PATCH v2] DW SPI: Get clock value from Device Tree >>>>>>>>>>>>> >>>>>>>>>>>>> How hard it is to make others to use clock manager? do you have any list? >>>>>>>>>>>> >>>>>>>>>>>> clock_manager.h is an old (and non-generic) way to deal with different clocks. >>>>>>>>>>>> For example in SOCFPGA_GEN5 and SOCFPGA_ARRIA10 clock_manager.h provides >>>>>>>>>>>> cm_get_spi_controller_clk_hz function to deal with spi controller clock. >>>>>>>>>>>> >>>>>>>>>>>> But today we have another, linux-like alternative: to bind clocks via device tree >>>>>>>>>>>> and manipulate with clocks via generic functions provided by clk.h >>>>>>>>>>>> >>>>>>>>>>>> In this patch I added option to get clock via device tree using standard bindings >>>>>>>>>>>> and restrict clock_manager.h functions usage only to targets which still use it, >>>>>>>>>>>> so new targets can simply bind clock via device tree and they do not need to >>>>>>>>>>>> implement/define something in clock_manager.h >>>>>>>>>>>> >>>>>>>>>>>> So we don't need to make others to use clock manager :) >>>>>>>>>>> >>>>>>>>>>> Maybe it worth trying the other way around and think about switching SOCFPGA platforms to >>>>>>>>>>> generic clk framework? >>>>>>>>>> >>>>>>>>>> Yes, ie what exactly I thought of, thanks! >>>>>>>>> >>>>>>>>> I checked cm_get_spi_controller_clk_hz implementation in SOCFPGA_GEN5 and >>>>>>>>> SOCFPGA_ARRIA10: we can't simply replace it with "fixed-clock" driver as it >>>>>>>>> manipulate with real hardware. >>>>>>>>> The only way to do it is to replace SOCFPGA* clock manager functions by real >>>>>>>>> clock driver. >>>>>>>>> >>>>>>>>> And given I don't have mentioned hardware so I barely can help with >>>>>>>>> those improvements on SOCFPGA. That said if there're no short-term plans to >>>>>>>>> switch SOCFPGA to clk framework maybe we'll be OK with my workaround with #ifdefs? >>>>>>>> >>>>>>>> Wait for Dinh's reply ... >>>>>>>> >>>>>>> >>>>>>> Honestly, I don't have too much time to work on this right now. So I >>>>>>> really don't when it can get done. But it'll go on my to-do list. >>>>>>> >>>>>>> Dinh >>>>>> >>>>>> Yep, thanks for your comments. >>>>>> >>>>>> So, Jagan, >>>>>> given Dinh's reply, could you please apply this patch? >>>>> >>>>> I'd really hate it to start seeing soc-specific ifdefs in drivers, >>>>> that's IMO not acceptable. A __weak override might be a temporary >>>>> solution I'd be willing to live with though. >>>> >>>> I would rather like to see some check on clock manager itself whether >>>> CONFIG_IS_ENABLED(CLK) is using or not? this can tends not to use >>>> __weak as well soc #ifdefs in driver. >>>> >>> >>> Actually I don't understand what do you mean. >>> Even if I add any #ifdefs to the clock_manager.h I still need to wrap clock_manager.h >>> include with #ifdefs as clock_manager.h is defined only for two >>> targets - SOCFPGA_GEN5 and SOCFPGA_ARRIA10. >>> >>> #if defined(CONFIG_TARGET_SOCFPGA_GEN5) || defined(CONFIG_TARGET_SOCFPGA_ARRIA10) >>> #include <asm/arch/clock_manager.h> >>> #endif >>> >>> And I think it is better to add this #ifdef in driver than create empty >>> clock_manager.h file for every new target which uses this driver. > > Check CLK in clock-manager like below, of-course cm_get_l4_noc_hz used > in other IP's as well. we need to carefully adding these check and if > require add CLK for remaining drivers as well. the reason for adding > checks in clock-manager is it may be removable code if all use CLK. I do not understand what you're trying to tell me, but the patch below makes no sense. What I would like to see is a weak function in the driver and that function be overriden in the socfpga clock manager. > --- a/arch/arm/mach-socfpga/clock_manager_arria10.c > +++ b/arch/arm/mach-socfpga/clock_manager_arria10.c > @@ -1066,10 +1066,17 @@ unsigned int cm_get_mmc_controller_clk_hz(void) > return clk_hz / 4; > } > > +#if CONFIG_IS_ENABLED(OF_CONTROL) && CONFIG_IS_ENABLED(CLK) > +unsigned int cm_get_spi_controller_clk_hz(void) > +{ > + return 0; > +} > +#else > unsigned int cm_get_spi_controller_clk_hz(void) > { > return cm_get_l4_noc_hz(CLKMGR_MAINPLL_NOCDIV_L4MPCLK_LSB); > } > +#endif > > thanks! >
On Mon, Oct 30, 2017 at 4:24 PM, Marek Vasut <marex@denx.de> wrote: > On 10/30/2017 07:04 AM, Jagan Teki wrote: >> On Sat, Oct 28, 2017 at 5:09 PM, Marek Vasut <marex@denx.de> wrote: >>> On 10/27/2017 03:54 PM, Eugeniy Paltsev wrote: >>>> On Tue, 2017-10-24 at 15:22 +0530, Jagan Teki wrote: >>>>> On Tue, Oct 24, 2017 at 11:38 AM, Marek Vasut <marex@denx.de> wrote: >>>>>> On 10/23/2017 01:43 PM, Eugeniy Paltsev wrote: >>>>>>> On Thu, 2017-10-19 at 13:20 -0500, Dinh Nguyen wrote: >>>>>>>> >>>>>>>> On 10/19/2017 10:51 AM, Marek Vasut wrote: >>>>>>>>> On 10/19/2017 05:36 PM, Eugeniy Paltsev wrote: >>>>>>>>>> On Tue, 2017-10-17 at 20:32 +0530, Jagan Teki wrote: >>>>>>>>>>> On Tue, Oct 17, 2017 at 8:27 PM, Alexey Brodkin >>>>>>>>>>> <Alexey.Brodkin@synopsys.com> wrote: >>>>>>>>>>>> Hi Jagan, >>>>>>>>>>>> >>>>>>>>>>>>> -----Original Message----- >>>>>>>>>>>>> From: Eugeniy Paltsev [mailto:paltsev@synopsys.com] >>>>>>>>>>>>> Sent: Tuesday, October 17, 2017 4:33 PM >>>>>>>>>>>>> To: jagannadh.teki@gmail.com >>>>>>>>>>>>> Cc: u-boot@lists.denx.de; uboot-snps-arc@synopsys.com >>>>>>>>>>>>> Subject: [uboot-snps-arc] Re: [PATCH v2] DW SPI: Get clock value from Device Tree >>>>>>>>>>>>>> >>>>>>>>>>>>>> How hard it is to make others to use clock manager? do you have any list? >>>>>>>>>>>>> >>>>>>>>>>>>> clock_manager.h is an old (and non-generic) way to deal with different clocks. >>>>>>>>>>>>> For example in SOCFPGA_GEN5 and SOCFPGA_ARRIA10 clock_manager.h provides >>>>>>>>>>>>> cm_get_spi_controller_clk_hz function to deal with spi controller clock. >>>>>>>>>>>>> >>>>>>>>>>>>> But today we have another, linux-like alternative: to bind clocks via device tree >>>>>>>>>>>>> and manipulate with clocks via generic functions provided by clk.h >>>>>>>>>>>>> >>>>>>>>>>>>> In this patch I added option to get clock via device tree using standard bindings >>>>>>>>>>>>> and restrict clock_manager.h functions usage only to targets which still use it, >>>>>>>>>>>>> so new targets can simply bind clock via device tree and they do not need to >>>>>>>>>>>>> implement/define something in clock_manager.h >>>>>>>>>>>>> >>>>>>>>>>>>> So we don't need to make others to use clock manager :) >>>>>>>>>>>> >>>>>>>>>>>> Maybe it worth trying the other way around and think about switching SOCFPGA platforms to >>>>>>>>>>>> generic clk framework? >>>>>>>>>>> >>>>>>>>>>> Yes, ie what exactly I thought of, thanks! >>>>>>>>>> >>>>>>>>>> I checked cm_get_spi_controller_clk_hz implementation in SOCFPGA_GEN5 and >>>>>>>>>> SOCFPGA_ARRIA10: we can't simply replace it with "fixed-clock" driver as it >>>>>>>>>> manipulate with real hardware. >>>>>>>>>> The only way to do it is to replace SOCFPGA* clock manager functions by real >>>>>>>>>> clock driver. >>>>>>>>>> >>>>>>>>>> And given I don't have mentioned hardware so I barely can help with >>>>>>>>>> those improvements on SOCFPGA. That said if there're no short-term plans to >>>>>>>>>> switch SOCFPGA to clk framework maybe we'll be OK with my workaround with #ifdefs? >>>>>>>>> >>>>>>>>> Wait for Dinh's reply ... >>>>>>>>> >>>>>>>> >>>>>>>> Honestly, I don't have too much time to work on this right now. So I >>>>>>>> really don't when it can get done. But it'll go on my to-do list. >>>>>>>> >>>>>>>> Dinh >>>>>>> >>>>>>> Yep, thanks for your comments. >>>>>>> >>>>>>> So, Jagan, >>>>>>> given Dinh's reply, could you please apply this patch? >>>>>> >>>>>> I'd really hate it to start seeing soc-specific ifdefs in drivers, >>>>>> that's IMO not acceptable. A __weak override might be a temporary >>>>>> solution I'd be willing to live with though. >>>>> >>>>> I would rather like to see some check on clock manager itself whether >>>>> CONFIG_IS_ENABLED(CLK) is using or not? this can tends not to use >>>>> __weak as well soc #ifdefs in driver. >>>>> >>>> >>>> Actually I don't understand what do you mean. >>>> Even if I add any #ifdefs to the clock_manager.h I still need to wrap clock_manager.h >>>> include with #ifdefs as clock_manager.h is defined only for two >>>> targets - SOCFPGA_GEN5 and SOCFPGA_ARRIA10. >>>> >>>> #if defined(CONFIG_TARGET_SOCFPGA_GEN5) || defined(CONFIG_TARGET_SOCFPGA_ARRIA10) >>>> #include <asm/arch/clock_manager.h> >>>> #endif >>>> >>>> And I think it is better to add this #ifdef in driver than create empty >>>> clock_manager.h file for every new target which uses this driver. >> >> Check CLK in clock-manager like below, of-course cm_get_l4_noc_hz used >> in other IP's as well. we need to carefully adding these check and if >> require add CLK for remaining drivers as well. the reason for adding >> checks in clock-manager is it may be removable code if all use CLK. > > I do not understand what you're trying to tell me, but the patch below > makes no sense. What I would like to see is a weak function in the > driver and that function be overriden in the socfpga clock manager. I'm trying to avoid __weak or #ifdef on driver instead make changes in clock manager which will remove in future(once all moved with CLK) thanks!
On 10/30/2017 12:36 PM, Jagan Teki wrote: > On Mon, Oct 30, 2017 at 4:24 PM, Marek Vasut <marex@denx.de> wrote: >> On 10/30/2017 07:04 AM, Jagan Teki wrote: >>> On Sat, Oct 28, 2017 at 5:09 PM, Marek Vasut <marex@denx.de> wrote: >>>> On 10/27/2017 03:54 PM, Eugeniy Paltsev wrote: >>>>> On Tue, 2017-10-24 at 15:22 +0530, Jagan Teki wrote: >>>>>> On Tue, Oct 24, 2017 at 11:38 AM, Marek Vasut <marex@denx.de> wrote: >>>>>>> On 10/23/2017 01:43 PM, Eugeniy Paltsev wrote: >>>>>>>> On Thu, 2017-10-19 at 13:20 -0500, Dinh Nguyen wrote: >>>>>>>>> >>>>>>>>> On 10/19/2017 10:51 AM, Marek Vasut wrote: >>>>>>>>>> On 10/19/2017 05:36 PM, Eugeniy Paltsev wrote: >>>>>>>>>>> On Tue, 2017-10-17 at 20:32 +0530, Jagan Teki wrote: >>>>>>>>>>>> On Tue, Oct 17, 2017 at 8:27 PM, Alexey Brodkin >>>>>>>>>>>> <Alexey.Brodkin@synopsys.com> wrote: >>>>>>>>>>>>> Hi Jagan, >>>>>>>>>>>>> >>>>>>>>>>>>>> -----Original Message----- >>>>>>>>>>>>>> From: Eugeniy Paltsev [mailto:paltsev@synopsys.com] >>>>>>>>>>>>>> Sent: Tuesday, October 17, 2017 4:33 PM >>>>>>>>>>>>>> To: jagannadh.teki@gmail.com >>>>>>>>>>>>>> Cc: u-boot@lists.denx.de; uboot-snps-arc@synopsys.com >>>>>>>>>>>>>> Subject: [uboot-snps-arc] Re: [PATCH v2] DW SPI: Get clock value from Device Tree >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> How hard it is to make others to use clock manager? do you have any list? >>>>>>>>>>>>>> >>>>>>>>>>>>>> clock_manager.h is an old (and non-generic) way to deal with different clocks. >>>>>>>>>>>>>> For example in SOCFPGA_GEN5 and SOCFPGA_ARRIA10 clock_manager.h provides >>>>>>>>>>>>>> cm_get_spi_controller_clk_hz function to deal with spi controller clock. >>>>>>>>>>>>>> >>>>>>>>>>>>>> But today we have another, linux-like alternative: to bind clocks via device tree >>>>>>>>>>>>>> and manipulate with clocks via generic functions provided by clk.h >>>>>>>>>>>>>> >>>>>>>>>>>>>> In this patch I added option to get clock via device tree using standard bindings >>>>>>>>>>>>>> and restrict clock_manager.h functions usage only to targets which still use it, >>>>>>>>>>>>>> so new targets can simply bind clock via device tree and they do not need to >>>>>>>>>>>>>> implement/define something in clock_manager.h >>>>>>>>>>>>>> >>>>>>>>>>>>>> So we don't need to make others to use clock manager :) >>>>>>>>>>>>> >>>>>>>>>>>>> Maybe it worth trying the other way around and think about switching SOCFPGA platforms to >>>>>>>>>>>>> generic clk framework? >>>>>>>>>>>> >>>>>>>>>>>> Yes, ie what exactly I thought of, thanks! >>>>>>>>>>> >>>>>>>>>>> I checked cm_get_spi_controller_clk_hz implementation in SOCFPGA_GEN5 and >>>>>>>>>>> SOCFPGA_ARRIA10: we can't simply replace it with "fixed-clock" driver as it >>>>>>>>>>> manipulate with real hardware. >>>>>>>>>>> The only way to do it is to replace SOCFPGA* clock manager functions by real >>>>>>>>>>> clock driver. >>>>>>>>>>> >>>>>>>>>>> And given I don't have mentioned hardware so I barely can help with >>>>>>>>>>> those improvements on SOCFPGA. That said if there're no short-term plans to >>>>>>>>>>> switch SOCFPGA to clk framework maybe we'll be OK with my workaround with #ifdefs? >>>>>>>>>> >>>>>>>>>> Wait for Dinh's reply ... >>>>>>>>>> >>>>>>>>> >>>>>>>>> Honestly, I don't have too much time to work on this right now. So I >>>>>>>>> really don't when it can get done. But it'll go on my to-do list. >>>>>>>>> >>>>>>>>> Dinh >>>>>>>> >>>>>>>> Yep, thanks for your comments. >>>>>>>> >>>>>>>> So, Jagan, >>>>>>>> given Dinh's reply, could you please apply this patch? >>>>>>> >>>>>>> I'd really hate it to start seeing soc-specific ifdefs in drivers, >>>>>>> that's IMO not acceptable. A __weak override might be a temporary >>>>>>> solution I'd be willing to live with though. >>>>>> >>>>>> I would rather like to see some check on clock manager itself whether >>>>>> CONFIG_IS_ENABLED(CLK) is using or not? this can tends not to use >>>>>> __weak as well soc #ifdefs in driver. >>>>>> >>>>> >>>>> Actually I don't understand what do you mean. >>>>> Even if I add any #ifdefs to the clock_manager.h I still need to wrap clock_manager.h >>>>> include with #ifdefs as clock_manager.h is defined only for two >>>>> targets - SOCFPGA_GEN5 and SOCFPGA_ARRIA10. >>>>> >>>>> #if defined(CONFIG_TARGET_SOCFPGA_GEN5) || defined(CONFIG_TARGET_SOCFPGA_ARRIA10) >>>>> #include <asm/arch/clock_manager.h> >>>>> #endif >>>>> >>>>> And I think it is better to add this #ifdef in driver than create empty >>>>> clock_manager.h file for every new target which uses this driver. >>> >>> Check CLK in clock-manager like below, of-course cm_get_l4_noc_hz used >>> in other IP's as well. we need to carefully adding these check and if >>> require add CLK for remaining drivers as well. the reason for adding >>> checks in clock-manager is it may be removable code if all use CLK. >> >> I do not understand what you're trying to tell me, but the patch below >> makes no sense. What I would like to see is a weak function in the >> driver and that function be overriden in the socfpga clock manager. > > I'm trying to avoid __weak or #ifdef on driver instead make changes in > clock manager which will remove in future(once all moved with CLK) Well, if you do the proposed change, socfpga will break. The __weak is the least possible evil here and I can live with that for now.
On Mon, Oct 30, 2017 at 5:12 PM, Marek Vasut <marex@denx.de> wrote: > On 10/30/2017 12:36 PM, Jagan Teki wrote: >> On Mon, Oct 30, 2017 at 4:24 PM, Marek Vasut <marex@denx.de> wrote: >>> On 10/30/2017 07:04 AM, Jagan Teki wrote: >>>> On Sat, Oct 28, 2017 at 5:09 PM, Marek Vasut <marex@denx.de> wrote: >>>>> On 10/27/2017 03:54 PM, Eugeniy Paltsev wrote: >>>>>> On Tue, 2017-10-24 at 15:22 +0530, Jagan Teki wrote: >>>>>>> On Tue, Oct 24, 2017 at 11:38 AM, Marek Vasut <marex@denx.de> wrote: >>>>>>>> On 10/23/2017 01:43 PM, Eugeniy Paltsev wrote: >>>>>>>>> On Thu, 2017-10-19 at 13:20 -0500, Dinh Nguyen wrote: >>>>>>>>>> >>>>>>>>>> On 10/19/2017 10:51 AM, Marek Vasut wrote: >>>>>>>>>>> On 10/19/2017 05:36 PM, Eugeniy Paltsev wrote: >>>>>>>>>>>> On Tue, 2017-10-17 at 20:32 +0530, Jagan Teki wrote: >>>>>>>>>>>>> On Tue, Oct 17, 2017 at 8:27 PM, Alexey Brodkin >>>>>>>>>>>>> <Alexey.Brodkin@synopsys.com> wrote: >>>>>>>>>>>>>> Hi Jagan, >>>>>>>>>>>>>> >>>>>>>>>>>>>>> -----Original Message----- >>>>>>>>>>>>>>> From: Eugeniy Paltsev [mailto:paltsev@synopsys.com] >>>>>>>>>>>>>>> Sent: Tuesday, October 17, 2017 4:33 PM >>>>>>>>>>>>>>> To: jagannadh.teki@gmail.com >>>>>>>>>>>>>>> Cc: u-boot@lists.denx.de; uboot-snps-arc@synopsys.com >>>>>>>>>>>>>>> Subject: [uboot-snps-arc] Re: [PATCH v2] DW SPI: Get clock value from Device Tree >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> How hard it is to make others to use clock manager? do you have any list? >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> clock_manager.h is an old (and non-generic) way to deal with different clocks. >>>>>>>>>>>>>>> For example in SOCFPGA_GEN5 and SOCFPGA_ARRIA10 clock_manager.h provides >>>>>>>>>>>>>>> cm_get_spi_controller_clk_hz function to deal with spi controller clock. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> But today we have another, linux-like alternative: to bind clocks via device tree >>>>>>>>>>>>>>> and manipulate with clocks via generic functions provided by clk.h >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> In this patch I added option to get clock via device tree using standard bindings >>>>>>>>>>>>>>> and restrict clock_manager.h functions usage only to targets which still use it, >>>>>>>>>>>>>>> so new targets can simply bind clock via device tree and they do not need to >>>>>>>>>>>>>>> implement/define something in clock_manager.h >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> So we don't need to make others to use clock manager :) >>>>>>>>>>>>>> >>>>>>>>>>>>>> Maybe it worth trying the other way around and think about switching SOCFPGA platforms to >>>>>>>>>>>>>> generic clk framework? >>>>>>>>>>>>> >>>>>>>>>>>>> Yes, ie what exactly I thought of, thanks! >>>>>>>>>>>> >>>>>>>>>>>> I checked cm_get_spi_controller_clk_hz implementation in SOCFPGA_GEN5 and >>>>>>>>>>>> SOCFPGA_ARRIA10: we can't simply replace it with "fixed-clock" driver as it >>>>>>>>>>>> manipulate with real hardware. >>>>>>>>>>>> The only way to do it is to replace SOCFPGA* clock manager functions by real >>>>>>>>>>>> clock driver. >>>>>>>>>>>> >>>>>>>>>>>> And given I don't have mentioned hardware so I barely can help with >>>>>>>>>>>> those improvements on SOCFPGA. That said if there're no short-term plans to >>>>>>>>>>>> switch SOCFPGA to clk framework maybe we'll be OK with my workaround with #ifdefs? >>>>>>>>>>> >>>>>>>>>>> Wait for Dinh's reply ... >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Honestly, I don't have too much time to work on this right now. So I >>>>>>>>>> really don't when it can get done. But it'll go on my to-do list. >>>>>>>>>> >>>>>>>>>> Dinh >>>>>>>>> >>>>>>>>> Yep, thanks for your comments. >>>>>>>>> >>>>>>>>> So, Jagan, >>>>>>>>> given Dinh's reply, could you please apply this patch? >>>>>>>> >>>>>>>> I'd really hate it to start seeing soc-specific ifdefs in drivers, >>>>>>>> that's IMO not acceptable. A __weak override might be a temporary >>>>>>>> solution I'd be willing to live with though. >>>>>>> >>>>>>> I would rather like to see some check on clock manager itself whether >>>>>>> CONFIG_IS_ENABLED(CLK) is using or not? this can tends not to use >>>>>>> __weak as well soc #ifdefs in driver. >>>>>>> >>>>>> >>>>>> Actually I don't understand what do you mean. >>>>>> Even if I add any #ifdefs to the clock_manager.h I still need to wrap clock_manager.h >>>>>> include with #ifdefs as clock_manager.h is defined only for two >>>>>> targets - SOCFPGA_GEN5 and SOCFPGA_ARRIA10. >>>>>> >>>>>> #if defined(CONFIG_TARGET_SOCFPGA_GEN5) || defined(CONFIG_TARGET_SOCFPGA_ARRIA10) >>>>>> #include <asm/arch/clock_manager.h> >>>>>> #endif >>>>>> >>>>>> And I think it is better to add this #ifdef in driver than create empty >>>>>> clock_manager.h file for every new target which uses this driver. >>>> >>>> Check CLK in clock-manager like below, of-course cm_get_l4_noc_hz used >>>> in other IP's as well. we need to carefully adding these check and if >>>> require add CLK for remaining drivers as well. the reason for adding >>>> checks in clock-manager is it may be removable code if all use CLK. >>> >>> I do not understand what you're trying to tell me, but the patch below >>> makes no sense. What I would like to see is a weak function in the >>> driver and that function be overriden in the socfpga clock manager. >> >> I'm trying to avoid __weak or #ifdef on driver instead make changes in >> clock manager which will remove in future(once all moved with CLK) > > Well, if you do the proposed change, socfpga will break. The __weak is > the least possible evil here and I can live with that for now. Yes, __weak can be possible evil here but since I'm planning to apply this in next version, I'm hoping possible change that shouldn't effect the driver. I see some __weak's last from many releases that doesn't resolve properly and they are still __weak functions only. thanks!
On 10/31/2017 09:27 AM, Jagan Teki wrote: > On Mon, Oct 30, 2017 at 5:12 PM, Marek Vasut <marex@denx.de> wrote: >> On 10/30/2017 12:36 PM, Jagan Teki wrote: >>> On Mon, Oct 30, 2017 at 4:24 PM, Marek Vasut <marex@denx.de> wrote: >>>> On 10/30/2017 07:04 AM, Jagan Teki wrote: >>>>> On Sat, Oct 28, 2017 at 5:09 PM, Marek Vasut <marex@denx.de> wrote: >>>>>> On 10/27/2017 03:54 PM, Eugeniy Paltsev wrote: >>>>>>> On Tue, 2017-10-24 at 15:22 +0530, Jagan Teki wrote: >>>>>>>> On Tue, Oct 24, 2017 at 11:38 AM, Marek Vasut <marex@denx.de> wrote: >>>>>>>>> On 10/23/2017 01:43 PM, Eugeniy Paltsev wrote: >>>>>>>>>> On Thu, 2017-10-19 at 13:20 -0500, Dinh Nguyen wrote: >>>>>>>>>>> >>>>>>>>>>> On 10/19/2017 10:51 AM, Marek Vasut wrote: >>>>>>>>>>>> On 10/19/2017 05:36 PM, Eugeniy Paltsev wrote: >>>>>>>>>>>>> On Tue, 2017-10-17 at 20:32 +0530, Jagan Teki wrote: >>>>>>>>>>>>>> On Tue, Oct 17, 2017 at 8:27 PM, Alexey Brodkin >>>>>>>>>>>>>> <Alexey.Brodkin@synopsys.com> wrote: >>>>>>>>>>>>>>> Hi Jagan, >>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> -----Original Message----- >>>>>>>>>>>>>>>> From: Eugeniy Paltsev [mailto:paltsev@synopsys.com] >>>>>>>>>>>>>>>> Sent: Tuesday, October 17, 2017 4:33 PM >>>>>>>>>>>>>>>> To: jagannadh.teki@gmail.com >>>>>>>>>>>>>>>> Cc: u-boot@lists.denx.de; uboot-snps-arc@synopsys.com >>>>>>>>>>>>>>>> Subject: [uboot-snps-arc] Re: [PATCH v2] DW SPI: Get clock value from Device Tree >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> How hard it is to make others to use clock manager? do you have any list? >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> clock_manager.h is an old (and non-generic) way to deal with different clocks. >>>>>>>>>>>>>>>> For example in SOCFPGA_GEN5 and SOCFPGA_ARRIA10 clock_manager.h provides >>>>>>>>>>>>>>>> cm_get_spi_controller_clk_hz function to deal with spi controller clock. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> But today we have another, linux-like alternative: to bind clocks via device tree >>>>>>>>>>>>>>>> and manipulate with clocks via generic functions provided by clk.h >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> In this patch I added option to get clock via device tree using standard bindings >>>>>>>>>>>>>>>> and restrict clock_manager.h functions usage only to targets which still use it, >>>>>>>>>>>>>>>> so new targets can simply bind clock via device tree and they do not need to >>>>>>>>>>>>>>>> implement/define something in clock_manager.h >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> So we don't need to make others to use clock manager :) >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Maybe it worth trying the other way around and think about switching SOCFPGA platforms to >>>>>>>>>>>>>>> generic clk framework? >>>>>>>>>>>>>> >>>>>>>>>>>>>> Yes, ie what exactly I thought of, thanks! >>>>>>>>>>>>> >>>>>>>>>>>>> I checked cm_get_spi_controller_clk_hz implementation in SOCFPGA_GEN5 and >>>>>>>>>>>>> SOCFPGA_ARRIA10: we can't simply replace it with "fixed-clock" driver as it >>>>>>>>>>>>> manipulate with real hardware. >>>>>>>>>>>>> The only way to do it is to replace SOCFPGA* clock manager functions by real >>>>>>>>>>>>> clock driver. >>>>>>>>>>>>> >>>>>>>>>>>>> And given I don't have mentioned hardware so I barely can help with >>>>>>>>>>>>> those improvements on SOCFPGA. That said if there're no short-term plans to >>>>>>>>>>>>> switch SOCFPGA to clk framework maybe we'll be OK with my workaround with #ifdefs? >>>>>>>>>>>> >>>>>>>>>>>> Wait for Dinh's reply ... >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Honestly, I don't have too much time to work on this right now. So I >>>>>>>>>>> really don't when it can get done. But it'll go on my to-do list. >>>>>>>>>>> >>>>>>>>>>> Dinh >>>>>>>>>> >>>>>>>>>> Yep, thanks for your comments. >>>>>>>>>> >>>>>>>>>> So, Jagan, >>>>>>>>>> given Dinh's reply, could you please apply this patch? >>>>>>>>> >>>>>>>>> I'd really hate it to start seeing soc-specific ifdefs in drivers, >>>>>>>>> that's IMO not acceptable. A __weak override might be a temporary >>>>>>>>> solution I'd be willing to live with though. >>>>>>>> >>>>>>>> I would rather like to see some check on clock manager itself whether >>>>>>>> CONFIG_IS_ENABLED(CLK) is using or not? this can tends not to use >>>>>>>> __weak as well soc #ifdefs in driver. >>>>>>>> >>>>>>> >>>>>>> Actually I don't understand what do you mean. >>>>>>> Even if I add any #ifdefs to the clock_manager.h I still need to wrap clock_manager.h >>>>>>> include with #ifdefs as clock_manager.h is defined only for two >>>>>>> targets - SOCFPGA_GEN5 and SOCFPGA_ARRIA10. >>>>>>> >>>>>>> #if defined(CONFIG_TARGET_SOCFPGA_GEN5) || defined(CONFIG_TARGET_SOCFPGA_ARRIA10) >>>>>>> #include <asm/arch/clock_manager.h> >>>>>>> #endif >>>>>>> >>>>>>> And I think it is better to add this #ifdef in driver than create empty >>>>>>> clock_manager.h file for every new target which uses this driver. >>>>> >>>>> Check CLK in clock-manager like below, of-course cm_get_l4_noc_hz used >>>>> in other IP's as well. we need to carefully adding these check and if >>>>> require add CLK for remaining drivers as well. the reason for adding >>>>> checks in clock-manager is it may be removable code if all use CLK. >>>> >>>> I do not understand what you're trying to tell me, but the patch below >>>> makes no sense. What I would like to see is a weak function in the >>>> driver and that function be overriden in the socfpga clock manager. >>> >>> I'm trying to avoid __weak or #ifdef on driver instead make changes in >>> clock manager which will remove in future(once all moved with CLK) >> >> Well, if you do the proposed change, socfpga will break. The __weak is >> the least possible evil here and I can live with that for now. > > Yes, __weak can be possible evil here but since I'm planning to apply > this in next version, I'm hoping possible change that shouldn't effect > the driver. I see some __weak's last from many releases that doesn't > resolve properly and they are still __weak functions only. The idea here would be to have the default __weak implementation use clock framework in the driver and override it only for socfpga in the socfpga clock manager. So for this patch, that's a NAK, I'd like a V3 with the __weak.
diff --git a/drivers/spi/designware_spi.c b/drivers/spi/designware_spi.c index 5aa507b..9eb5b1c 100644 --- a/drivers/spi/designware_spi.c +++ b/drivers/spi/designware_spi.c @@ -11,6 +11,7 @@ */ #include <common.h> +#include <clk.h> #include <dm.h> #include <errno.h> #include <malloc.h> @@ -18,7 +19,10 @@ #include <fdtdec.h> #include <linux/compat.h> #include <asm/io.h> +/* Only SOCFPGA_GEN5 and SOCFPGA_ARRIA10 uses their clock_manager functions */ +#if defined(CONFIG_TARGET_SOCFPGA_GEN5) || defined(CONFIG_TARGET_SOCFPGA_ARRIA10) #include <asm/arch/clock_manager.h> +#endif DECLARE_GLOBAL_DATA_PTR; @@ -94,6 +98,7 @@ struct dw_spi_priv { void __iomem *regs; unsigned int freq; /* Default frequency */ unsigned int mode; + unsigned long bus_clk_rate; int bits_per_word; u8 cs; /* chip select pin */ @@ -176,14 +181,72 @@ static void spi_hw_init(struct dw_spi_priv *priv) debug("%s: fifo_len=%d\n", __func__, priv->fifo_len); } +static int dw_spi_of_get_clk(struct udevice *bus) +{ +#if CONFIG_IS_ENABLED(OF_CONTROL) && CONFIG_IS_ENABLED(CLK) + struct dw_spi_priv *priv = dev_get_priv(bus); + struct clk clk; + int ret; + + ret = clk_get_by_index(bus, 0, &clk); + if (ret) + return -EINVAL; + + ret = clk_enable(&clk); + if (ret && ret != -ENOSYS) + return ret; + + priv->bus_clk_rate = clk_get_rate(&clk); + if (!priv->bus_clk_rate) { + clk_disable(&clk); + return -EINVAL; + } + + clk_free(&clk); + + return 0; +#else + return -ENOSYS; +#endif +} + +static int dw_spi_get_clk(struct udevice *bus) +{ + struct dw_spi_priv *priv = dev_get_priv(bus); + + /* Firstly try to get clock frequency from device tree */ + if (!dw_spi_of_get_clk(bus)) + return 0; + + /* + * SOCFPGA_GEN5 and SOCFPGA_ARRIA10 uses cm_get_spi_controller_clk_hz + * function (defined in asm/arch/clock_manager.h) to get spi controller + * clock frequency. So in case of get clock frequency from device + * tree failure rollback to cm_get_spi_controller_clk_hz + */ +#if defined(CONFIG_TARGET_SOCFPGA_GEN5) || defined(CONFIG_TARGET_SOCFPGA_ARRIA10) + priv->bus_clk_rate = cm_get_spi_controller_clk_hz(); +#endif + + if (!priv->bus_clk_rate) + return -EINVAL; + + return 0; +} + static int dw_spi_probe(struct udevice *bus) { struct dw_spi_platdata *plat = dev_get_platdata(bus); struct dw_spi_priv *priv = dev_get_priv(bus); + int ret; priv->regs = plat->regs; priv->freq = plat->frequency; + ret = dw_spi_get_clk(bus); + if (ret) + return ret; + /* Currently only bits_per_word == 8 supported */ priv->bits_per_word = 8; @@ -369,7 +432,7 @@ static int dw_spi_set_speed(struct udevice *bus, uint speed) spi_enable_chip(priv, 0); /* clk_div doesn't support odd number */ - clk_div = cm_get_spi_controller_clk_hz() / speed; + clk_div = priv->bus_clk_rate / speed; clk_div = (clk_div + 1) & 0xfffe; dw_writel(priv, DW_SPI_BAUDR, clk_div);
Add option to set spi controller clock frequency via device tree using standard clock bindings. Old way of setting spi controller clock frequency (via implementation of 'cm_get_spi_controller_clk_hz' function in platform specific code) remains supported for backward compatibility. Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com> --- Changes v1->v2: * disable clock if we can't get the rate. * get rid of cm_get_spi_controller_clk_hz weak declaration. drivers/spi/designware_spi.c | 65 +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 64 insertions(+), 1 deletion(-)