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

Tue Oct 31 09:33:00 UTC 2017

On 10/31/2017 09:27 AM, Jagan Teki wrote:
> On Mon, Oct 30, 2017 at 5:12 PM, Marek Vasut <marex at 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 at 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 at 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 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.
>>>>>
>>>>> 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