[U-Boot] [PATCH v2] DW SPI: Get clock value from Device Tree

[Marek Vasut]

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 at 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 at synopsys.com> wrote:
>>>>>>>>> Hi Jagan,
>>>>>>>>>
>>>>>>>>>> -----Original Message-----
>>>>>>>>>> From: Eugeniy Paltsev [mailto:paltsev at synopsys.com]
>>>>>>>>>> Sent: Tuesday, October 17, 2017 4:33 PM
>>>>>>>>>> To: jagannadh.teki at gmail.com
>>>>>>>>>> Cc: u-boot at lists.denx.de; uboot-snps-arc at 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 .

-- 
Best regards,
Marek Vasut