[PATCH v3 RESEND] clk: sifive: Fix W=1 kernel build warning

[Jessica Clarke]

On 13 Jan 2022, at 17:21, Lee Jones <lee.jones at linaro.org> wrote:
> 
> On Thu, 13 Jan 2022, Zong Li wrote:
> 
>> On Wed, Jan 12, 2022 at 5:09 PM Lee Jones <lee.jones at linaro.org> wrote:
>>> 
>>> On Wed, 12 Jan 2022, Zong Li wrote:
>>> 
>>>> On Tue, Jan 11, 2022 at 5:32 PM Lee Jones <lee.jones at linaro.org> wrote:
>>>>> 
>>>>> On Tue, 11 Jan 2022, Zong Li wrote:
>>>>> 
>>>>>> On Mon, Jan 10, 2022 at 5:50 PM Lee Jones <lee.jones at linaro.org> wrote:
>>>>>>> 
>>>>>>> Please improve the subject line.
>>>>>>> 
>>>>>>> If this is a straight revert, the subject line should reflect that.
>>>>>>> 
>>>>>>> If not, you need to give us specific information regarding the purpose
>>>>>>> of this patch.  Please read the Git log for better, more forthcoming
>>>>>>> examples.
>>>>>>> 
>>>>>> 
>>>>>> It seems to me that this patch is not a straight revert, it provides
>>>>>> another way to fix the original build warnings, just like
>>>>>> '487dc7bb6a0c' tried to do. I guess the commit message has described
>>>>>> what the original warnings is and what the root cause is, it also
>>>>>> mentioned what is changed in this patch. I'm a bit confused whether we
>>>>>> need to add fixes tag, it looks like that it might cause some
>>>>>> misunderstanding?
>>>>> 
>>>>> I think it's the patch description and subject that is causing the
>>>>> misunderstanding.
>>>>> 
>>>> 
>>>> Yes, the subject should be made better.
>>>> 
>>>>> Please help me with a couple of points and I'll help you draft
>>>>> something up.
>>>>> 
>>>>> Firstly, what alerted you to the problem you're attempting to solve?
>>>>> 
>>>> 
>>>> I recently noticed the code was changed, I guess that I was missing
>>>> something there. After tracking the log, I found that there is a build
>>>> warning in the original implementation, and it was already fixed, but
>>>> it seems to me that there are still some situations there, please help
>>>> me to see the following illustration.
>>>> 
>>>>>>>> --- a/drivers/clk/sifive/fu540-prci.c
>>>>>>>> +++ b/drivers/clk/sifive/fu540-prci.c
>>>>>>>> @@ -20,7 +20,6 @@
>>>>>>>> 
>>>>>>>> #include <dt-bindings/clock/sifive-fu540-prci.h>
>>>>>>>> 
>>>>>>>> -#include "fu540-prci.h"
>>>>> 
>>>>> How is this related to the issue/patch?
>>>>> 
>>>> 
>>>> Let's go back to the version without '487dc7bb6a0c' fix. The
>>>> prci_clk_fu540 variable is defined in sifive-fu540-prci.h header,
>>>> however, fu540-prci.c includes this header but doesn't use this
>>>> variable, so the warnings happen.
>>>> 
>>>> The easiest way to do it is just removing this line, then the warning
>>>> could be fixed. But as the '487dc7bb6a0c' or this patch does, the code
>>>> should be improved, the prci_clk_fu540 variable shouldn't be defined
>>>> in the header, it should be moved somewhere.
>>>> 
>>>>>>>> +struct prci_clk_desc prci_clk_fu540 = {
>>>>>>>> +     .clks = __prci_init_clocks_fu540,
>>>>>>>> +     .num_clks = ARRAY_SIZE(__prci_init_clocks_fu540),
>>>>>>>> +};
>>>>> 
>>>>>>>> diff --git a/drivers/clk/sifive/fu540-prci.h b/drivers/clk/sifive/fu540-prci.h
>>>>>>>> index c220677dc010..931d6cad8c1c 100644
>>>>>>>> --- a/drivers/clk/sifive/fu540-prci.h
>>>>>>>> +++ b/drivers/clk/sifive/fu540-prci.h
>>>>>>>> @@ -7,10 +7,6 @@
>>>>>>>> +extern struct prci_clk_desc prci_clk_fu540;
>>>>> 
>>>>>>>> diff --git a/drivers/clk/sifive/sifive-prci.c b/drivers/clk/sifive/sifive-prci.c
>>>>>>>> index 80a288c59e56..916d2fc28b9c 100644
>>>>>>>> --- a/drivers/clk/sifive/sifive-prci.c
>>>>>>>> +++ b/drivers/clk/sifive/sifive-prci.c
>>>>>>>> @@ -12,11 +12,6 @@
>>>>>>>> #include "fu540-prci.h"
>>>>>>>> #include "fu740-prci.h"
>>>>>>>> 
>>>>>>>> -static const struct prci_clk_desc prci_clk_fu540 = {
>>>>>>>> -     .clks = __prci_init_clocks_fu540,
>>>>>>>> -     .num_clks = ARRAY_SIZE(__prci_init_clocks_fu540),
>>>>>>>> -};
>>>>>>>> -
>>>>> 
>>>>> I'm not sure if it's you or I that is missing the point here, but
>>>>> prci_clk_fu540 is used within *this* file itself:
>>>>> 
>>>> 
>>>> Here is another situation I mentioned at the beginning, if we'd like
>>>> to put prci_clk_fu540 here, prci_clk_fu740 should be put here as well.
>>>> I guess you didn't do that because there is a bug in the original
>>>> code, fu740-prci.c misused the fu540-prci.h, so there is no build
>>>> warning on fu740. FU740 still works correctly by misusing the
>>>> fu540-prci.h header because fu740-prci.c doesn't actually use the
>>>> prci_clk_fu740 variable, like fu540 we talked about earlier.
>>>> 
>>>>> static const struct of_device_id sifive_prci_of_match[] = {
>>>>>         {.compatible = "sifive,fu540-c000-prci", .data = &prci_clk_fu540},
>>>>>         {.compatible = "sifive,fu740-c000-prci", .data = &prci_clk_fu740},
>>>>>         {}
>>>>> };
>>>>> 
>>>>> So why are you moving it out to somewhere it is *not* used and making
>>>>> it an extern?  This sounds like the opposite to what you'd want?
>>>> 
>>>> The idea is that sifive-prci.c is the core and common part of PRCI,
>>>> and I'd like to separate the SoCs-dependent part into SoCs-dependent
>>>> files, such as fu540-prci.c and fu740-prci.c. The goal is if we add
>>>> new SoCs in the future, we can just put the SoCs-dependent data
>>>> structure in the new C file, and do as minimum modification as
>>>> possible in the core file (i.e. sifive-prci.c). It might also help us
>>>> to see all SoCs-dependent data in one file, then we don't need to
>>>> cross many files. Putting these two variables in sifive-pric.c is the
>>>> right thing to do, but that is why I separate them and make them
>>>> extern in this patch.
>>> 
>>> I can see what you are doing, but I don't think this is the right
>>> thing to do.  Please put the struct in the same location as it's
>>> referenced.
>> 
>> If we decide to move them into sifive-prci.c (i.e. put it in where
>> it's referenced.), I worried that we might need to move all stuff
>> which are in fu540-prci.c and fu740-prci.c. Because 'prci_clk_fu540'
>> is referenced in sifive-prci.c, whereas '__prci_init_clocks_fu540' is
>> used by 'prci_clk_fu540', and the almost things in fu540-prci.c are
>> used by '__prci_init_clocks_fu540'. It seems to be a little bit
>> difficult to clearly decouple it for modularization which I want to
>> do. What this patch does might be a accepted way, I hope that you can
>> consider it again.
>> 
>>> 
>>> And yes that should also be the case for prci_clk_fu740 and yes, it
>>> was over-looked because it wasn't causing warnings at build time for
>>> whatever reason.
>>> 
>>> IMHO, placing 'struct of_device_id' match tables in headers is also
>>> odd and is likely the real cause of this strange situation.
>> 
>> I couldn't see what are you pointing, do you mind give more details
>> about it? It seems to me that the match table is put in C file (i.e.
>> sifive-prci.c).
> 
> Oh, sorry, it's a common source file, rather than a header.
> 
> Okay, so I went and actually looked at the code this time.
> 
> If I were you I would move all of the device specific structs and
> tables into the device specific header files, then delete the device
> specific source (C) files entirely.
> 
> There seems to be no good reason for carrying a common source file as
> well as a source file AND header file for each supported device.
> IMHO, that's over-complicating things for no apparent gain.

The reason it exists the way it does is that the driver uses the header
files shipped and used for the device tree bindings, and they give the
same names to different constants (the first three constants are in
fact the same so don’t clash, but PRCI_CLK_TLCLK is different between
the two), so can’t both be in the same translation unit (at least not
without some gross #undef’ing). In FreeBSD I took the alternate
approach of just defining our own FU540_ and FU740_ namespaced copies
of the constants, as drivers do for most things anyway.

Jess