* Re: [PATCH v3 RESEND] clk: sifive: Fix W=1 kernel build warning
@ 2022-01-13 17:21 ` Lee Jones
0 siblings, 0 replies; 38+ messages in thread
From: Lee Jones @ 2022-01-13 17:21 UTC (permalink / raw)
To: Zong Li
Cc: Michael Turquette, Stephen Boyd, Palmer Dabbelt, Paul Walmsley,
linux-clk, linux-riscv
On Thu, 13 Jan 2022, Zong Li wrote:
> On Wed, Jan 12, 2022 at 5:09 PM Lee Jones <lee.jones@linaro.org> wrote:
> >
> > On Wed, 12 Jan 2022, Zong Li wrote:
> >
> > > On Tue, Jan 11, 2022 at 5:32 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > >
> > > > On Tue, 11 Jan 2022, Zong Li wrote:
> > > >
> > > > > On Mon, Jan 10, 2022 at 5:50 PM Lee Jones <lee.jones@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.
--
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 RESEND] clk: sifive: Fix W=1 kernel build warning
2022-01-13 17:21 ` Lee Jones
@ 2022-01-13 17:22 ` Lee Jones
-1 siblings, 0 replies; 38+ messages in thread
From: Lee Jones @ 2022-01-13 17:22 UTC (permalink / raw)
To: Zong Li
Cc: Michael Turquette, Stephen Boyd, Palmer Dabbelt, Paul Walmsley,
linux-clk, linux-riscv
On Thu, 13 Jan 2022, Lee Jones wrote:
> On Thu, 13 Jan 2022, Zong Li wrote:
>
> > On Wed, Jan 12, 2022 at 5:09 PM Lee Jones <lee.jones@linaro.org> wrote:
> > >
> > > On Wed, 12 Jan 2022, Zong Li wrote:
> > >
> > > > On Tue, Jan 11, 2022 at 5:32 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > > >
> > > > > On Tue, 11 Jan 2022, Zong Li wrote:
> > > > >
> > > > > > On Mon, Jan 10, 2022 at 5:50 PM Lee Jones <lee.jones@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.
I can even do it for you, if you'd like!
--
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 RESEND] clk: sifive: Fix W=1 kernel build warning
@ 2022-01-13 17:22 ` Lee Jones
0 siblings, 0 replies; 38+ messages in thread
From: Lee Jones @ 2022-01-13 17:22 UTC (permalink / raw)
To: Zong Li
Cc: Michael Turquette, Stephen Boyd, Palmer Dabbelt, Paul Walmsley,
linux-clk, linux-riscv
On Thu, 13 Jan 2022, Lee Jones wrote:
> On Thu, 13 Jan 2022, Zong Li wrote:
>
> > On Wed, Jan 12, 2022 at 5:09 PM Lee Jones <lee.jones@linaro.org> wrote:
> > >
> > > On Wed, 12 Jan 2022, Zong Li wrote:
> > >
> > > > On Tue, Jan 11, 2022 at 5:32 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > > >
> > > > > On Tue, 11 Jan 2022, Zong Li wrote:
> > > > >
> > > > > > On Mon, Jan 10, 2022 at 5:50 PM Lee Jones <lee.jones@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.
I can even do it for you, if you'd like!
--
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 RESEND] clk: sifive: Fix W=1 kernel build warning
2022-01-13 17:21 ` Lee Jones
@ 2022-01-13 17:51 ` Jessica Clarke
-1 siblings, 0 replies; 38+ messages in thread
From: Jessica Clarke @ 2022-01-13 17:51 UTC (permalink / raw)
To: Lee Jones
Cc: Zong Li, Michael Turquette, Stephen Boyd, Palmer Dabbelt,
Paul Walmsley, linux-clk, linux-riscv
On 13 Jan 2022, at 17:21, Lee Jones <lee.jones@linaro.org> wrote:
>
> On Thu, 13 Jan 2022, Zong Li wrote:
>
>> On Wed, Jan 12, 2022 at 5:09 PM Lee Jones <lee.jones@linaro.org> wrote:
>>>
>>> On Wed, 12 Jan 2022, Zong Li wrote:
>>>
>>>> On Tue, Jan 11, 2022 at 5:32 PM Lee Jones <lee.jones@linaro.org> wrote:
>>>>>
>>>>> On Tue, 11 Jan 2022, Zong Li wrote:
>>>>>
>>>>>> On Mon, Jan 10, 2022 at 5:50 PM Lee Jones <lee.jones@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
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 RESEND] clk: sifive: Fix W=1 kernel build warning
@ 2022-01-13 17:51 ` Jessica Clarke
0 siblings, 0 replies; 38+ messages in thread
From: Jessica Clarke @ 2022-01-13 17:51 UTC (permalink / raw)
To: Lee Jones
Cc: Zong Li, Michael Turquette, Stephen Boyd, Palmer Dabbelt,
Paul Walmsley, linux-clk, linux-riscv
On 13 Jan 2022, at 17:21, Lee Jones <lee.jones@linaro.org> wrote:
>
> On Thu, 13 Jan 2022, Zong Li wrote:
>
>> On Wed, Jan 12, 2022 at 5:09 PM Lee Jones <lee.jones@linaro.org> wrote:
>>>
>>> On Wed, 12 Jan 2022, Zong Li wrote:
>>>
>>>> On Tue, Jan 11, 2022 at 5:32 PM Lee Jones <lee.jones@linaro.org> wrote:
>>>>>
>>>>> On Tue, 11 Jan 2022, Zong Li wrote:
>>>>>
>>>>>> On Mon, Jan 10, 2022 at 5:50 PM Lee Jones <lee.jones@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
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 RESEND] clk: sifive: Fix W=1 kernel build warning
2022-01-13 17:51 ` Jessica Clarke
@ 2022-01-13 18:13 ` Lee Jones
-1 siblings, 0 replies; 38+ messages in thread
From: Lee Jones @ 2022-01-13 18:13 UTC (permalink / raw)
To: Jessica Clarke
Cc: Zong Li, Michael Turquette, Stephen Boyd, Palmer Dabbelt,
Paul Walmsley, linux-clk, linux-riscv
On Thu, 13 Jan 2022, Jessica Clarke wrote:
> On 13 Jan 2022, at 17:21, Lee Jones <lee.jones@linaro.org> wrote:
> >
> > On Thu, 13 Jan 2022, Zong Li wrote:
> >
> >> On Wed, Jan 12, 2022 at 5:09 PM Lee Jones <lee.jones@linaro.org> wrote:
> >>>
> >>> On Wed, 12 Jan 2022, Zong Li wrote:
> >>>
> >>>> On Tue, Jan 11, 2022 at 5:32 PM Lee Jones <lee.jones@linaro.org> wrote:
> >>>>>
> >>>>> On Tue, 11 Jan 2022, Zong Li wrote:
> >>>>>
> >>>>>> On Mon, Jan 10, 2022 at 5:50 PM Lee Jones <lee.jones@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.
--
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 RESEND] clk: sifive: Fix W=1 kernel build warning
@ 2022-01-13 18:13 ` Lee Jones
0 siblings, 0 replies; 38+ messages in thread
From: Lee Jones @ 2022-01-13 18:13 UTC (permalink / raw)
To: Jessica Clarke
Cc: Zong Li, Michael Turquette, Stephen Boyd, Palmer Dabbelt,
Paul Walmsley, linux-clk, linux-riscv
On Thu, 13 Jan 2022, Jessica Clarke wrote:
> On 13 Jan 2022, at 17:21, Lee Jones <lee.jones@linaro.org> wrote:
> >
> > On Thu, 13 Jan 2022, Zong Li wrote:
> >
> >> On Wed, Jan 12, 2022 at 5:09 PM Lee Jones <lee.jones@linaro.org> wrote:
> >>>
> >>> On Wed, 12 Jan 2022, Zong Li wrote:
> >>>
> >>>> On Tue, Jan 11, 2022 at 5:32 PM Lee Jones <lee.jones@linaro.org> wrote:
> >>>>>
> >>>>> On Tue, 11 Jan 2022, Zong Li wrote:
> >>>>>
> >>>>>> On Mon, Jan 10, 2022 at 5:50 PM Lee Jones <lee.jones@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.
--
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 RESEND] clk: sifive: Fix W=1 kernel build warning
2022-01-13 18:13 ` Lee Jones
@ 2022-01-14 9:45 ` Zong Li
-1 siblings, 0 replies; 38+ messages in thread
From: Zong Li @ 2022-01-14 9:45 UTC (permalink / raw)
To: Lee Jones
Cc: Jessica Clarke, Michael Turquette, Stephen Boyd, Palmer Dabbelt,
Paul Walmsley, linux-clk, linux-riscv
On Fri, Jan 14, 2022 at 2:13 AM Lee Jones <lee.jones@linaro.org> wrote:
>
> On Thu, 13 Jan 2022, Jessica Clarke wrote:
>
> > On 13 Jan 2022, at 17:21, Lee Jones <lee.jones@linaro.org> wrote:
> > >
> > > On Thu, 13 Jan 2022, Zong Li wrote:
> > >
> > >> On Wed, Jan 12, 2022 at 5:09 PM Lee Jones <lee.jones@linaro.org> wrote:
> > >>>
> > >>> On Wed, 12 Jan 2022, Zong Li wrote:
> > >>>
> > >>>> On Tue, Jan 11, 2022 at 5:32 PM Lee Jones <lee.jones@linaro.org> wrote:
> > >>>>>
> > >>>>> On Tue, 11 Jan 2022, Zong Li wrote:
> > >>>>>
> > >>>>>> On Mon, Jan 10, 2022 at 5:50 PM Lee Jones <lee.jones@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.
>
> --
> Lee Jones [李琼斯]
> Principal Technical Lead - Developer Services
> Linaro.org │ Open source software for Arm SoCs
> Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 RESEND] clk: sifive: Fix W=1 kernel build warning
@ 2022-01-14 9:45 ` Zong Li
0 siblings, 0 replies; 38+ messages in thread
From: Zong Li @ 2022-01-14 9:45 UTC (permalink / raw)
To: Lee Jones
Cc: Jessica Clarke, Michael Turquette, Stephen Boyd, Palmer Dabbelt,
Paul Walmsley, linux-clk, linux-riscv
On Fri, Jan 14, 2022 at 2:13 AM Lee Jones <lee.jones@linaro.org> wrote:
>
> On Thu, 13 Jan 2022, Jessica Clarke wrote:
>
> > On 13 Jan 2022, at 17:21, Lee Jones <lee.jones@linaro.org> wrote:
> > >
> > > On Thu, 13 Jan 2022, Zong Li wrote:
> > >
> > >> On Wed, Jan 12, 2022 at 5:09 PM Lee Jones <lee.jones@linaro.org> wrote:
> > >>>
> > >>> On Wed, 12 Jan 2022, Zong Li wrote:
> > >>>
> > >>>> On Tue, Jan 11, 2022 at 5:32 PM Lee Jones <lee.jones@linaro.org> wrote:
> > >>>>>
> > >>>>> On Tue, 11 Jan 2022, Zong Li wrote:
> > >>>>>
> > >>>>>> On Mon, Jan 10, 2022 at 5:50 PM Lee Jones <lee.jones@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.
>
> --
> Lee Jones [李琼斯]
> Principal Technical Lead - Developer Services
> Linaro.org │ Open source software for Arm SoCs
> Follow Linaro: Facebook | Twitter | Blog
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 RESEND] clk: sifive: Fix W=1 kernel build warning
2022-01-14 9:45 ` Zong Li
@ 2022-01-14 10:07 ` Lee Jones
-1 siblings, 0 replies; 38+ messages in thread
From: Lee Jones @ 2022-01-14 10:07 UTC (permalink / raw)
To: Zong Li
Cc: Jessica Clarke, Michael Turquette, Stephen Boyd, Palmer Dabbelt,
Paul Walmsley, linux-clk, linux-riscv
On Fri, 14 Jan 2022, Zong Li wrote:
> On Fri, Jan 14, 2022 at 2:13 AM Lee Jones <lee.jones@linaro.org> wrote:
> >
> > On Thu, 13 Jan 2022, Jessica Clarke wrote:
> >
> > > On 13 Jan 2022, at 17:21, Lee Jones <lee.jones@linaro.org> wrote:
> > > >
> > > > On Thu, 13 Jan 2022, Zong Li wrote:
> > > >
> > > >> On Wed, Jan 12, 2022 at 5:09 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > >>>
> > > >>> On Wed, 12 Jan 2022, Zong Li wrote:
> > > >>>
> > > >>>> On Tue, Jan 11, 2022 at 5:32 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > >>>>>
> > > >>>>> On Tue, 11 Jan 2022, Zong Li wrote:
> > > >>>>>
> > > >>>>>> On Mon, Jan 10, 2022 at 5:50 PM Lee Jones <lee.jones@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?
--
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 RESEND] clk: sifive: Fix W=1 kernel build warning
@ 2022-01-14 10:07 ` Lee Jones
0 siblings, 0 replies; 38+ messages in thread
From: Lee Jones @ 2022-01-14 10:07 UTC (permalink / raw)
To: Zong Li
Cc: Jessica Clarke, Michael Turquette, Stephen Boyd, Palmer Dabbelt,
Paul Walmsley, linux-clk, linux-riscv
On Fri, 14 Jan 2022, Zong Li wrote:
> On Fri, Jan 14, 2022 at 2:13 AM Lee Jones <lee.jones@linaro.org> wrote:
> >
> > On Thu, 13 Jan 2022, Jessica Clarke wrote:
> >
> > > On 13 Jan 2022, at 17:21, Lee Jones <lee.jones@linaro.org> wrote:
> > > >
> > > > On Thu, 13 Jan 2022, Zong Li wrote:
> > > >
> > > >> On Wed, Jan 12, 2022 at 5:09 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > >>>
> > > >>> On Wed, 12 Jan 2022, Zong Li wrote:
> > > >>>
> > > >>>> On Tue, Jan 11, 2022 at 5:32 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > >>>>>
> > > >>>>> On Tue, 11 Jan 2022, Zong Li wrote:
> > > >>>>>
> > > >>>>>> On Mon, Jan 10, 2022 at 5:50 PM Lee Jones <lee.jones@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?
--
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 RESEND] clk: sifive: Fix W=1 kernel build warning
2022-01-14 10:07 ` Lee Jones
@ 2022-01-14 14:59 ` Zong Li
-1 siblings, 0 replies; 38+ messages in thread
From: Zong Li @ 2022-01-14 14:59 UTC (permalink / raw)
To: Lee Jones
Cc: Jessica Clarke, Michael Turquette, Stephen Boyd, Palmer Dabbelt,
Paul Walmsley, linux-clk, linux-riscv
On Fri, Jan 14, 2022 at 6:07 PM Lee Jones <lee.jones@linaro.org> wrote:
>
> On Fri, 14 Jan 2022, Zong Li wrote:
>
> > On Fri, Jan 14, 2022 at 2:13 AM Lee Jones <lee.jones@linaro.org> wrote:
> > >
> > > On Thu, 13 Jan 2022, Jessica Clarke wrote:
> > >
> > > > On 13 Jan 2022, at 17:21, Lee Jones <lee.jones@linaro.org> wrote:
> > > > >
> > > > > On Thu, 13 Jan 2022, Zong Li wrote:
> > > > >
> > > > >> On Wed, Jan 12, 2022 at 5:09 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > > >>>
> > > > >>> On Wed, 12 Jan 2022, Zong Li wrote:
> > > > >>>
> > > > >>>> On Tue, Jan 11, 2022 at 5:32 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > > >>>>>
> > > > >>>>> On Tue, 11 Jan 2022, Zong Li wrote:
> > > > >>>>>
> > > > >>>>>> On Mon, Jan 10, 2022 at 5:50 PM Lee Jones <lee.jones@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.
> --
> Lee Jones [李琼斯]
> Principal Technical Lead - Developer Services
> Linaro.org │ Open source software for Arm SoCs
> Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 RESEND] clk: sifive: Fix W=1 kernel build warning
@ 2022-01-14 14:59 ` Zong Li
0 siblings, 0 replies; 38+ messages in thread
From: Zong Li @ 2022-01-14 14:59 UTC (permalink / raw)
To: Lee Jones
Cc: Jessica Clarke, Michael Turquette, Stephen Boyd, Palmer Dabbelt,
Paul Walmsley, linux-clk, linux-riscv
On Fri, Jan 14, 2022 at 6:07 PM Lee Jones <lee.jones@linaro.org> wrote:
>
> On Fri, 14 Jan 2022, Zong Li wrote:
>
> > On Fri, Jan 14, 2022 at 2:13 AM Lee Jones <lee.jones@linaro.org> wrote:
> > >
> > > On Thu, 13 Jan 2022, Jessica Clarke wrote:
> > >
> > > > On 13 Jan 2022, at 17:21, Lee Jones <lee.jones@linaro.org> wrote:
> > > > >
> > > > > On Thu, 13 Jan 2022, Zong Li wrote:
> > > > >
> > > > >> On Wed, Jan 12, 2022 at 5:09 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > > >>>
> > > > >>> On Wed, 12 Jan 2022, Zong Li wrote:
> > > > >>>
> > > > >>>> On Tue, Jan 11, 2022 at 5:32 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > > >>>>>
> > > > >>>>> On Tue, 11 Jan 2022, Zong Li wrote:
> > > > >>>>>
> > > > >>>>>> On Mon, Jan 10, 2022 at 5:50 PM Lee Jones <lee.jones@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.
> --
> Lee Jones [李琼斯]
> Principal Technical Lead - Developer Services
> Linaro.org │ Open source software for Arm SoCs
> Follow Linaro: Facebook | Twitter | Blog
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 RESEND] clk: sifive: Fix W=1 kernel build warning
2022-01-14 14:59 ` Zong Li
@ 2022-01-14 16:34 ` Lee Jones
-1 siblings, 0 replies; 38+ messages in thread
From: Lee Jones @ 2022-01-14 16:34 UTC (permalink / raw)
To: Zong Li
Cc: Jessica Clarke, Michael Turquette, Stephen Boyd, Palmer Dabbelt,
Paul Walmsley, linux-clk, linux-riscv
On Fri, 14 Jan 2022, Zong Li wrote:
> On Fri, Jan 14, 2022 at 6:07 PM Lee Jones <lee.jones@linaro.org> wrote:
> >
> > On Fri, 14 Jan 2022, Zong Li wrote:
> >
> > > On Fri, Jan 14, 2022 at 2:13 AM Lee Jones <lee.jones@linaro.org> wrote:
> > > >
> > > > On Thu, 13 Jan 2022, Jessica Clarke wrote:
> > > >
> > > > > On 13 Jan 2022, at 17:21, Lee Jones <lee.jones@linaro.org> wrote:
> > > > > >
> > > > > > On Thu, 13 Jan 2022, Zong Li wrote:
> > > > > >
> > > > > >> On Wed, Jan 12, 2022 at 5:09 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > > > >>>
> > > > > >>> On Wed, 12 Jan 2022, Zong Li wrote:
> > > > > >>>
> > > > > >>>> On Tue, Jan 11, 2022 at 5:32 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > > > >>>>>
> > > > > >>>>> On Tue, 11 Jan 2022, Zong Li wrote:
> > > > > >>>>>
> > > > > >>>>>> On Mon, Jan 10, 2022 at 5:50 PM Lee Jones <lee.jones@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.
--
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 RESEND] clk: sifive: Fix W=1 kernel build warning
@ 2022-01-14 16:34 ` Lee Jones
0 siblings, 0 replies; 38+ messages in thread
From: Lee Jones @ 2022-01-14 16:34 UTC (permalink / raw)
To: Zong Li
Cc: Jessica Clarke, Michael Turquette, Stephen Boyd, Palmer Dabbelt,
Paul Walmsley, linux-clk, linux-riscv
On Fri, 14 Jan 2022, Zong Li wrote:
> On Fri, Jan 14, 2022 at 6:07 PM Lee Jones <lee.jones@linaro.org> wrote:
> >
> > On Fri, 14 Jan 2022, Zong Li wrote:
> >
> > > On Fri, Jan 14, 2022 at 2:13 AM Lee Jones <lee.jones@linaro.org> wrote:
> > > >
> > > > On Thu, 13 Jan 2022, Jessica Clarke wrote:
> > > >
> > > > > On 13 Jan 2022, at 17:21, Lee Jones <lee.jones@linaro.org> wrote:
> > > > > >
> > > > > > On Thu, 13 Jan 2022, Zong Li wrote:
> > > > > >
> > > > > >> On Wed, Jan 12, 2022 at 5:09 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > > > >>>
> > > > > >>> On Wed, 12 Jan 2022, Zong Li wrote:
> > > > > >>>
> > > > > >>>> On Tue, Jan 11, 2022 at 5:32 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > > > >>>>>
> > > > > >>>>> On Tue, 11 Jan 2022, Zong Li wrote:
> > > > > >>>>>
> > > > > >>>>>> On Mon, Jan 10, 2022 at 5:50 PM Lee Jones <lee.jones@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.
--
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 RESEND] clk: sifive: Fix W=1 kernel build warning
2022-01-14 16:34 ` Lee Jones
@ 2022-01-15 6:19 ` Zong Li
-1 siblings, 0 replies; 38+ messages in thread
From: Zong Li @ 2022-01-15 6:19 UTC (permalink / raw)
To: Lee Jones
Cc: Jessica Clarke, Michael Turquette, Stephen Boyd, Palmer Dabbelt,
Paul Walmsley, linux-clk, linux-riscv
On Sat, Jan 15, 2022 at 12:34 AM Lee Jones <lee.jones@linaro.org> wrote:
>
> On Fri, 14 Jan 2022, Zong Li wrote:
>
> > On Fri, Jan 14, 2022 at 6:07 PM Lee Jones <lee.jones@linaro.org> wrote:
> > >
> > > On Fri, 14 Jan 2022, Zong Li wrote:
> > >
> > > > On Fri, Jan 14, 2022 at 2:13 AM Lee Jones <lee.jones@linaro.org> wrote:
> > > > >
> > > > > On Thu, 13 Jan 2022, Jessica Clarke wrote:
> > > > >
> > > > > > On 13 Jan 2022, at 17:21, Lee Jones <lee.jones@linaro.org> wrote:
> > > > > > >
> > > > > > > On Thu, 13 Jan 2022, Zong Li wrote:
> > > > > > >
> > > > > > >> On Wed, Jan 12, 2022 at 5:09 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > > > > >>>
> > > > > > >>> On Wed, 12 Jan 2022, Zong Li wrote:
> > > > > > >>>
> > > > > > >>>> On Tue, Jan 11, 2022 at 5:32 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > > > > >>>>>
> > > > > > >>>>> On Tue, 11 Jan 2022, Zong Li wrote:
> > > > > > >>>>>
> > > > > > >>>>>> On Mon, Jan 10, 2022 at 5:50 PM Lee Jones <lee.jones@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
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 RESEND] clk: sifive: Fix W=1 kernel build warning
@ 2022-01-15 6:19 ` Zong Li
0 siblings, 0 replies; 38+ messages in thread
From: Zong Li @ 2022-01-15 6:19 UTC (permalink / raw)
To: Lee Jones
Cc: Jessica Clarke, Michael Turquette, Stephen Boyd, Palmer Dabbelt,
Paul Walmsley, linux-clk, linux-riscv
On Sat, Jan 15, 2022 at 12:34 AM Lee Jones <lee.jones@linaro.org> wrote:
>
> On Fri, 14 Jan 2022, Zong Li wrote:
>
> > On Fri, Jan 14, 2022 at 6:07 PM Lee Jones <lee.jones@linaro.org> wrote:
> > >
> > > On Fri, 14 Jan 2022, Zong Li wrote:
> > >
> > > > On Fri, Jan 14, 2022 at 2:13 AM Lee Jones <lee.jones@linaro.org> wrote:
> > > > >
> > > > > On Thu, 13 Jan 2022, Jessica Clarke wrote:
> > > > >
> > > > > > On 13 Jan 2022, at 17:21, Lee Jones <lee.jones@linaro.org> wrote:
> > > > > > >
> > > > > > > On Thu, 13 Jan 2022, Zong Li wrote:
> > > > > > >
> > > > > > >> On Wed, Jan 12, 2022 at 5:09 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > > > > >>>
> > > > > > >>> On Wed, 12 Jan 2022, Zong Li wrote:
> > > > > > >>>
> > > > > > >>>> On Tue, Jan 11, 2022 at 5:32 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > > > > >>>>>
> > > > > > >>>>> On Tue, 11 Jan 2022, Zong Li wrote:
> > > > > > >>>>>
> > > > > > >>>>>> On Mon, Jan 10, 2022 at 5:50 PM Lee Jones <lee.jones@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
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 RESEND] clk: sifive: Fix W=1 kernel build warning
2022-01-15 6:19 ` Zong Li
@ 2022-01-19 9:38 ` Zong Li
-1 siblings, 0 replies; 38+ messages in thread
From: Zong Li @ 2022-01-19 9:38 UTC (permalink / raw)
To: Lee Jones
Cc: Jessica Clarke, Michael Turquette, Stephen Boyd, Palmer Dabbelt,
Paul Walmsley, linux-clk, linux-riscv
On Sat, Jan 15, 2022 at 2:19 PM Zong Li <zong.li@sifive.com> wrote:
>
> On Sat, Jan 15, 2022 at 12:34 AM Lee Jones <lee.jones@linaro.org> wrote:
> >
> > On Fri, 14 Jan 2022, Zong Li wrote:
> >
> > > On Fri, Jan 14, 2022 at 6:07 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > >
> > > > On Fri, 14 Jan 2022, Zong Li wrote:
> > > >
> > > > > On Fri, Jan 14, 2022 at 2:13 AM Lee Jones <lee.jones@linaro.org> wrote:
> > > > > >
> > > > > > On Thu, 13 Jan 2022, Jessica Clarke wrote:
> > > > > >
> > > > > > > On 13 Jan 2022, at 17:21, Lee Jones <lee.jones@linaro.org> wrote:
> > > > > > > >
> > > > > > > > On Thu, 13 Jan 2022, Zong Li wrote:
> > > > > > > >
> > > > > > > >> On Wed, Jan 12, 2022 at 5:09 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > > > > > >>>
> > > > > > > >>> On Wed, 12 Jan 2022, Zong Li wrote:
> > > > > > > >>>
> > > > > > > >>>> On Tue, Jan 11, 2022 at 5:32 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > > > > > >>>>>
> > > > > > > >>>>> On Tue, 11 Jan 2022, Zong Li wrote:
> > > > > > > >>>>>
> > > > > > > >>>>>> On Mon, Jan 10, 2022 at 5:50 PM Lee Jones <lee.jones@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.
>
Thanks all for helping me to review and give me the suggestions. I
sent another patch set to refactor the code (i.e. [PATCH 0/4] Refactor
the PRCI driver to reduce the complexity). I'd like to drop this
patch, and move on to the new patch set.
Thanks.
> > --
> > Lee Jones [李琼斯]
> > Principal Technical Lead - Developer Services
> > Linaro.org │ Open source software for Arm SoCs
> > Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v3 RESEND] clk: sifive: Fix W=1 kernel build warning
@ 2022-01-19 9:38 ` Zong Li
0 siblings, 0 replies; 38+ messages in thread
From: Zong Li @ 2022-01-19 9:38 UTC (permalink / raw)
To: Lee Jones
Cc: Jessica Clarke, Michael Turquette, Stephen Boyd, Palmer Dabbelt,
Paul Walmsley, linux-clk, linux-riscv
On Sat, Jan 15, 2022 at 2:19 PM Zong Li <zong.li@sifive.com> wrote:
>
> On Sat, Jan 15, 2022 at 12:34 AM Lee Jones <lee.jones@linaro.org> wrote:
> >
> > On Fri, 14 Jan 2022, Zong Li wrote:
> >
> > > On Fri, Jan 14, 2022 at 6:07 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > >
> > > > On Fri, 14 Jan 2022, Zong Li wrote:
> > > >
> > > > > On Fri, Jan 14, 2022 at 2:13 AM Lee Jones <lee.jones@linaro.org> wrote:
> > > > > >
> > > > > > On Thu, 13 Jan 2022, Jessica Clarke wrote:
> > > > > >
> > > > > > > On 13 Jan 2022, at 17:21, Lee Jones <lee.jones@linaro.org> wrote:
> > > > > > > >
> > > > > > > > On Thu, 13 Jan 2022, Zong Li wrote:
> > > > > > > >
> > > > > > > >> On Wed, Jan 12, 2022 at 5:09 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > > > > > >>>
> > > > > > > >>> On Wed, 12 Jan 2022, Zong Li wrote:
> > > > > > > >>>
> > > > > > > >>>> On Tue, Jan 11, 2022 at 5:32 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > > > > > >>>>>
> > > > > > > >>>>> On Tue, 11 Jan 2022, Zong Li wrote:
> > > > > > > >>>>>
> > > > > > > >>>>>> On Mon, Jan 10, 2022 at 5:50 PM Lee Jones <lee.jones@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.
>
Thanks all for helping me to review and give me the suggestions. I
sent another patch set to refactor the code (i.e. [PATCH 0/4] Refactor
the PRCI driver to reduce the complexity). I'd like to drop this
patch, and move on to the new patch set.
Thanks.
> > --
> > Lee Jones [李琼斯]
> > Principal Technical Lead - Developer Services
> > Linaro.org │ Open source software for Arm SoCs
> > Follow Linaro: Facebook | Twitter | Blog
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 38+ messages in thread