linux-kernel - Re: [PATCH v1 1/6] ARM: davinci: clean up map

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <0dad9d38-3645-c295-212b-6b66733f633b@ti.com>
Date:   Fri, 8 Dec 2017 21:04:23 +0530
From:   Sekhar Nori <nsekhar@...com>
To:     David Lechner <david@...hnology.com>,
        <linux-arm-kernel@...ts.infradead.org>
CC:     Kevin Hilman <khilman@...nel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v1 1/6] ARM: davinci: clean up map_io functions

On Thursday 07 December 2017 10:44 PM, David Lechner wrote:
> On 12/07/2017 08:52 AM, Sekhar Nori wrote:
>> On Saturday 02 December 2017 08:04 AM, David Lechner wrote:
>>> This cleans up the map_io functions in the board init files for
>>> mach-davinci.
>>>
>>> Most of the boards had a wrapper function around <board>_init(). This
>>> wrapper is removed and the function is used directly. Additionally, the
>>> <board>_init() functions are renamed to <board>_map_io() to match the
>>> field name.
>>>
>>> Signed-off-by: David Lechner <david@...hnology.com>
>>
>>> diff --git a/arch/arm/mach-davinci/board-dm646x-evm.c
>>> b/arch/arm/mach-davinci/board-dm646x-evm.c
>>> index cb0a41e..f0e2762 100644
>>> --- a/arch/arm/mach-davinci/board-dm646x-evm.c
>>> +++ b/arch/arm/mach-davinci/board-dm646x-evm.c
>>> @@ -716,16 +716,6 @@ static void __init evm_init_i2c(void)
>>>   }
>>>   #endif
>>>   -#define DM6467T_EVM_REF_FREQ        33000000
>>> -
>>> -static void __init davinci_map_io(void)
>>> -{
>>> -    dm646x_init();
>>
>> The call to dm646x_init() is dropped here, but I don't see it added
>> back, at least in this patch.
> 
> dm646x_init() is renamed to dm646x_map_io(), which is used directly in
> MACHINE_START().

Ah, I missed that. But I think its a symptom of too many things going on
in the patch. How about splitting the patch to:

a) Remove trivial <board>_map_io() wrappers and use <soc>_init()
directly to initialize .map_io

b) Rename <soc>_init() to <soc>_map_io

> 
>>
>>> diff --git a/arch/arm/mach-davinci/dm646x.c
>>> b/arch/arm/mach-davinci/dm646x.c
>>> index da21353..b3be5c8 100644
>>> --- a/arch/arm/mach-davinci/dm646x.c
>>> +++ b/arch/arm/mach-davinci/dm646x.c
>>> @@ -17,6 +17,7 @@
>>>   #include <linux/platform_data/edma.h>
>>>   #include <linux/platform_data/gpio-davinci.h>
>>>   +#include <asm/mach-types.h>
>>>   #include <asm/mach/map.h>
>>>     #include <mach/cputype.h>
>>> @@ -952,11 +953,16 @@ int __init dm646x_init_edma(struct
>>> edma_rsv_info *rsv)
>>>       return IS_ERR(edma_pdev) ? PTR_ERR(edma_pdev) : 0;
>>>   }
>>>   -void __init dm646x_init(void)
>>> +#define DM6467T_EVM_REF_FREQ        33000000
>>> +
>>> +void __init dm646x_map_io(void)
>>>   {
>>>       davinci_common_init(&davinci_soc_info_dm646x);
>>>       davinci_map_sysmod();
>>>       davinci_clk_init(davinci_soc_info_dm646x.cpu_clks);
>>> +
>>> +    if (machine_is_davinci_dm6467tevm())
>>> +        davinci_set_refclk_rate(DM6467T_EVM_REF_FREQ);
>>>   }
>>
>> I think we should leave the DM646x case out of this since there are
>> additional issues like introducing these EVM specific defines in a file
>> meant for SoC.
> 
> I agree with the sentiment. This quirk gets moved around several times
> in this series just to keep things working for a git bisect even if it
> is not the ideal place for it to be.
> 
> Currently, all boards use a common reference frequency from the common
> SoC files instead of the board file. I have not done so in this version
> of the series, but I could rework it so that this happens, which would
> remove the need for this quirk altogether. But even then, it would
> probably get shuffled around a bit before being eliminated.

We should avoid shuffling and modifying the same code repeatedly if we
can. I think, it will be easier to read if patch 2 and 1 are interchanged.

There is a lot going on and I wasn't sure how that will look so I tried
it briefly and pushed a branch.

Do have a look, but this seems better to me.

https://git.kernel.org/pub/scm/linux/kernel/git/nsekhar/linux-davinci.git/log/?h=for-davidl

Thanks,
Sekhar