On 10/31/2017 09:27 AM, Jagan Teki wrote: > On Mon, Oct 30, 2017 at 5:12 PM, Marek Vasut <ma...@denx.de> wrote: >> On 10/30/2017 12:36 PM, Jagan Teki wrote: >>> On Mon, Oct 30, 2017 at 4:24 PM, Marek Vasut <ma...@denx.de> wrote: >>>> 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. >>> >>> 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. -- Best regards, Marek Vasut _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
