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

[Zong Li]

On Sat, Jan 15, 2022 at 12:34 AM Lee Jones <lee.jones at linaro.org> wrote:
>
> On Fri, 14 Jan 2022, Zong Li wrote:
>
> > On Fri, Jan 14, 2022 at 6:07 PM Lee Jones <lee.jones at linaro.org> wrote:
> > >
> > > On Fri, 14 Jan 2022, Zong Li wrote:
> > >
> > > > On Fri, Jan 14, 2022 at 2:13 AM Lee Jones <lee.jones at linaro.org> wrote:
> > > > >
> > > > > On Thu, 13 Jan 2022, Jessica Clarke wrote:
> > > > >
> > > > > > 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.
> > > > >
> > > > > That's a sensible approach.
> > > > >
> > > > > One which we use in Linux extensively.
> > > >
> > > > Thanks all for the review and suggestions, it is great to me to move
> > > > all stuff into the specific headers, I only have one question there,
> > > > is it ok to put the definition of those data structures in header
> > > > files? That is one of the changes we had done in v2 patch. If it's
> > > > good to you, I will do it in the next version. Thanks.
> > >
> > > Can you give me an example please?
> > >
> >
> > Yes, for the simplest example, we don't usually define 'int a = 1' in
> > header, we might just declare 'extern int a' in header files. If I
> > understand correctly, we are going to move all stuff in fu540-prci.c
> > into fu540-prci.h, so there will be many definitions of variable in
> > fu540-prci.h. These headers will be used only in one file (i.e.
> > sifive-prci.c), it might not cause strange behavior, but I'd like to
> > make sure if it could be accepted and ok to all you guys before I
> > sending the next version.
>
> Everything in fu540-prci.c is suitable for inclusion into a header
> file.  The data there is just made up of populated tables.
>

Thanks for your reply. I will change them in the next version.

> --
> Lee Jones [李琼斯]
> Principal Technical Lead - Developer Services
> Linaro.org │ Open source software for Arm SoCs
> Follow Linaro: Facebook | Twitter | Blog