On 10/30/2017 07:04 AM, Jagan Teki wrote: > On Sat, Oct 28, 2017 at 5:09 PM, Marek Vasut <ma...@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 <ma...@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.brod...@synopsys.com> wrote: >>>>>>>>>>> Hi Jagan, >>>>>>>>>>> >>>>>>>>>>>> -----Original Message----- >>>>>>>>>>>> From: Eugeniy Paltsev [mailto:palt...@synopsys.com] >>>>>>>>>>>> Sent: Tuesday, October 17, 2017 4:33 PM >>>>>>>>>>>> To: jagannadh.t...@gmail.com >>>>>>>>>>>> Cc: u-boot@lists.denx.de; uboot-snps-...@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! > -- Best regards, Marek Vasut _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot