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

Message ID	20220107090715.2601-1-zong.li@sifive.com (mailing list archive)
State	New, archived
Headers	show Return-Path: <linux-riscv-bounces+linux-riscv=archiver.kernel.org@lists.infradead.org> From: Zong Li <zong.li@sifive.com> To: mturquette@baylibre.com, sboyd@kernel.org, palmer@dabbelt.com, paul.walmsley@sifive.com, lee.jones@linaro.org, linux-clk@vger.kernel.org, linux-riscv@lists.infradead.org Cc: Zong Li <zong.li@sifive.com> Subject: [PATCH v3 RESEND] clk: sifive: Fix W=1 kernel build warning Date: Fri, 7 Jan 2022 17:07:15 +0800 Message-Id: <20220107090715.2601-1-zong.li@sifive.com> MIME-Version: 1.0 Precedence: list Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Sender: "linux-riscv" <linux-riscv-bounces@lists.infradead.org> Errors-To: linux-riscv-bounces+linux-riscv=archiver.kernel.org@lists.infradead.org
Series	[v3,RESEND] clk: sifive: Fix W=1 kernel build warning \| expand [v3,RESEND] clk: sifive: Fix W=1 kernel build warning

Zong Li Jan. 7, 2022, 9:07 a.m. UTC

This commit reverts commit 487dc7bb6a0c ("clk: sifive: fu540-prci:
Declare static const variable 'prci_clk_fu540' where it's used").
For fixing W=1 kernel build warning(s) about ‘prci_clk_fu540’ defined
but not used [-Wunused-const-variable=], the problem is that the C file
of fu540 and fu740 doesn't use these variables, but they includes the
header files. We could refine the code by moving the definition of these
variables into fu540 and fu740 implementation respectively instead of
common core code, then we could still separate the SoCs-dependent data
in their own implementation.

Fixes: 487dc7bb6a0c ("clk: sifive: fu540-prci: Declare static
const variable 'prci_clk_fu540' where it's used")
Signed-off-by: Zong Li <zong.li@sifive.com>

---
Changed in v3:
 - Rebase on v5.16-rc8
 - Add fixes tag

Changed in v2:
 - Move definition of variable to C file from header
---
 drivers/clk/sifive/fu540-prci.c  |  6 +++++-
 drivers/clk/sifive/fu540-prci.h  |  6 +-----
 drivers/clk/sifive/fu740-prci.c  |  6 +++++-
 drivers/clk/sifive/fu740-prci.h  | 11 +----------
 drivers/clk/sifive/sifive-prci.c |  5 -----
 5 files changed, 12 insertions(+), 22 deletions(-)

Stephen Boyd Jan. 8, 2022, 12:45 a.m. UTC | #1

Quoting Zong Li (2022-01-07 01:07:15)
> This commit reverts commit 487dc7bb6a0c ("clk: sifive: fu540-prci:
> Declare static const variable 'prci_clk_fu540' where it's used").
> For fixing W=1 kernel build warning(s) about ‘prci_clk_fu540’ defined
> but not used [-Wunused-const-variable=], the problem is that the C file
> of fu540 and fu740 doesn't use these variables, but they includes the
> header files. We could refine the code by moving the definition of these
> variables into fu540 and fu740 implementation respectively instead of
> common core code, then we could still separate the SoCs-dependent data
> in their own implementation.
> 
> Fixes: 487dc7bb6a0c ("clk: sifive: fu540-prci: Declare static
> const variable 'prci_clk_fu540' where it's used")

The fixes tag should be on one line, not split across two.

> Signed-off-by: Zong Li <zong.li@sifive.com>
>

Zong Li Jan. 10, 2022, 1:52 a.m. UTC | #2

On Sat, Jan 8, 2022 at 8:45 AM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Zong Li (2022-01-07 01:07:15)
> > This commit reverts commit 487dc7bb6a0c ("clk: sifive: fu540-prci:
> > Declare static const variable 'prci_clk_fu540' where it's used").
> > For fixing W=1 kernel build warning(s) about ‘prci_clk_fu540’ defined
> > but not used [-Wunused-const-variable=], the problem is that the C file
> > of fu540 and fu740 doesn't use these variables, but they includes the
> > header files. We could refine the code by moving the definition of these
> > variables into fu540 and fu740 implementation respectively instead of
> > common core code, then we could still separate the SoCs-dependent data
> > in their own implementation.
> >
> > Fixes: 487dc7bb6a0c ("clk: sifive: fu540-prci: Declare static
> > const variable 'prci_clk_fu540' where it's used")
>
> The fixes tag should be on one line, not split across two.
>

Ok, let me fix it in the v4 patch. Thanks

> > Signed-off-by: Zong Li <zong.li@sifive.com>
> >

Lee Jones Jan. 10, 2022, 9:50 a.m. UTC | #3

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.

On Fri, 07 Jan 2022, Zong Li wrote:

> This commit reverts commit 487dc7bb6a0c ("clk: sifive: fu540-prci:
> Declare static const variable 'prci_clk_fu540' where it's used").
> For fixing W=1 kernel build warning(s) about ‘prci_clk_fu540’ defined
> but not used [-Wunused-const-variable=], the problem is that the C file
> of fu540 and fu740 doesn't use these variables, but they includes the
> header files.

What exactly does this patch fix?  Does it fix a build warning?

If so, please provide the line you are seeing.

> We could refine the code by moving the definition of these
> variables into fu540 and fu740 implementation respectively instead of
> common core code, then we could still separate the SoCs-dependent data
> in their own implementation.
> 
> Fixes: 487dc7bb6a0c ("clk: sifive: fu540-prci: Declare static
> const variable 'prci_clk_fu540' where it's used")

This should be on one line.

What exactly does it fix though?  Please provide more details.

What about the warning that this patch was designed to fix?  Doesn't
that return after this patch has been applied?

> Signed-off-by: Zong Li <zong.li@sifive.com>
> 
> ---
> Changed in v3:
>  - Rebase on v5.16-rc8
>  - Add fixes tag
> 
> Changed in v2:
>  - Move definition of variable to C file from header
> ---
>  drivers/clk/sifive/fu540-prci.c  |  6 +++++-
>  drivers/clk/sifive/fu540-prci.h  |  6 +-----
>  drivers/clk/sifive/fu740-prci.c  |  6 +++++-
>  drivers/clk/sifive/fu740-prci.h  | 11 +----------
>  drivers/clk/sifive/sifive-prci.c |  5 -----
>  5 files changed, 12 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/clk/sifive/fu540-prci.c b/drivers/clk/sifive/fu540-prci.c
> index 29bab915003c..5568bc26e36f 100644
> --- 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"
>  #include "sifive-prci.h"
>  
>  /* PRCI integration data for each WRPLL instance */
> @@ -87,3 +86,8 @@ struct __prci_clock __prci_init_clocks_fu540[] = {
>  		.ops = &sifive_fu540_prci_tlclksel_clk_ops,
>  	},
>  };
> +
> +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 @@
>  #ifndef __SIFIVE_CLK_FU540_PRCI_H
>  #define __SIFIVE_CLK_FU540_PRCI_H
>  
> -#include "sifive-prci.h"
> -
> -#define NUM_CLOCK_FU540	4
> -
> -extern struct __prci_clock __prci_init_clocks_fu540[NUM_CLOCK_FU540];
> +extern struct prci_clk_desc prci_clk_fu540;
>  
>  #endif /* __SIFIVE_CLK_FU540_PRCI_H */
> diff --git a/drivers/clk/sifive/fu740-prci.c b/drivers/clk/sifive/fu740-prci.c
> index 53f6e00a03b9..0ade3dcd24ed 100644
> --- a/drivers/clk/sifive/fu740-prci.c
> +++ b/drivers/clk/sifive/fu740-prci.c
> @@ -8,7 +8,6 @@
>  
>  #include <dt-bindings/clock/sifive-fu740-prci.h>
>  
> -#include "fu540-prci.h"
>  #include "sifive-prci.h"
>  
>  /* PRCI integration data for each WRPLL instance */
> @@ -132,3 +131,8 @@ struct __prci_clock __prci_init_clocks_fu740[] = {
>  		.ops = &sifive_fu740_prci_pcie_aux_clk_ops,
>  	},
>  };
> +
> +struct prci_clk_desc prci_clk_fu740 = {
> +	.clks = __prci_init_clocks_fu740,
> +	.num_clks = ARRAY_SIZE(__prci_init_clocks_fu740),
> +};
> diff --git a/drivers/clk/sifive/fu740-prci.h b/drivers/clk/sifive/fu740-prci.h
> index 511a0bf7ba2b..5bc0e18f058c 100644
> --- a/drivers/clk/sifive/fu740-prci.h
> +++ b/drivers/clk/sifive/fu740-prci.h
> @@ -7,15 +7,6 @@
>  #ifndef __SIFIVE_CLK_FU740_PRCI_H
>  #define __SIFIVE_CLK_FU740_PRCI_H
>  
> -#include "sifive-prci.h"
> -
> -#define NUM_CLOCK_FU740	9
> -
> -extern struct __prci_clock __prci_init_clocks_fu740[NUM_CLOCK_FU740];
> -
> -static const struct prci_clk_desc prci_clk_fu740 = {
> -	.clks = __prci_init_clocks_fu740,
> -	.num_clks = ARRAY_SIZE(__prci_init_clocks_fu740),
> -};
> +extern struct prci_clk_desc prci_clk_fu740;
>  
>  #endif /* __SIFIVE_CLK_FU740_PRCI_H */
> 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),
> -};
> -
>  /*
>   * Private functions
>   */

Zong Li Jan. 11, 2022, 3:21 a.m. UTC | #4

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?

> On Fri, 07 Jan 2022, Zong Li wrote:
>
> > This commit reverts commit 487dc7bb6a0c ("clk: sifive: fu540-prci:
> > Declare static const variable 'prci_clk_fu540' where it's used").
> > For fixing W=1 kernel build warning(s) about ‘prci_clk_fu540’ defined
> > but not used [-Wunused-const-variable=], the problem is that the C file
> > of fu540 and fu740 doesn't use these variables, but they includes the
> > header files.
>
> What exactly does this patch fix?  Does it fix a build warning?
>
> If so, please provide the line you are seeing.
>
> > We could refine the code by moving the definition of these
> > variables into fu540 and fu740 implementation respectively instead of
> > common core code, then we could still separate the SoCs-dependent data
> > in their own implementation.
> >
> > Fixes: 487dc7bb6a0c ("clk: sifive: fu540-prci: Declare static
> > const variable 'prci_clk_fu540' where it's used")
>
> This should be on one line.
>
> What exactly does it fix though?  Please provide more details.
>
> What about the warning that this patch was designed to fix?  Doesn't
> that return after this patch has been applied?
>
> > Signed-off-by: Zong Li <zong.li@sifive.com>
> >
> > ---
> > Changed in v3:
> >  - Rebase on v5.16-rc8
> >  - Add fixes tag
> >
> > Changed in v2:
> >  - Move definition of variable to C file from header
> > ---
> >  drivers/clk/sifive/fu540-prci.c  |  6 +++++-
> >  drivers/clk/sifive/fu540-prci.h  |  6 +-----
> >  drivers/clk/sifive/fu740-prci.c  |  6 +++++-
> >  drivers/clk/sifive/fu740-prci.h  | 11 +----------
> >  drivers/clk/sifive/sifive-prci.c |  5 -----
> >  5 files changed, 12 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/clk/sifive/fu540-prci.c b/drivers/clk/sifive/fu540-prci.c
> > index 29bab915003c..5568bc26e36f 100644
> > --- 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"
> >  #include "sifive-prci.h"
> >
> >  /* PRCI integration data for each WRPLL instance */
> > @@ -87,3 +86,8 @@ struct __prci_clock __prci_init_clocks_fu540[] = {
> >               .ops = &sifive_fu540_prci_tlclksel_clk_ops,
> >       },
> >  };
> > +
> > +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 @@
> >  #ifndef __SIFIVE_CLK_FU540_PRCI_H
> >  #define __SIFIVE_CLK_FU540_PRCI_H
> >
> > -#include "sifive-prci.h"
> > -
> > -#define NUM_CLOCK_FU540      4
> > -
> > -extern struct __prci_clock __prci_init_clocks_fu540[NUM_CLOCK_FU540];
> > +extern struct prci_clk_desc prci_clk_fu540;
> >
> >  #endif /* __SIFIVE_CLK_FU540_PRCI_H */
> > diff --git a/drivers/clk/sifive/fu740-prci.c b/drivers/clk/sifive/fu740-prci.c
> > index 53f6e00a03b9..0ade3dcd24ed 100644
> > --- a/drivers/clk/sifive/fu740-prci.c
> > +++ b/drivers/clk/sifive/fu740-prci.c
> > @@ -8,7 +8,6 @@
> >
> >  #include <dt-bindings/clock/sifive-fu740-prci.h>
> >
> > -#include "fu540-prci.h"
> >  #include "sifive-prci.h"
> >
> >  /* PRCI integration data for each WRPLL instance */
> > @@ -132,3 +131,8 @@ struct __prci_clock __prci_init_clocks_fu740[] = {
> >               .ops = &sifive_fu740_prci_pcie_aux_clk_ops,
> >       },
> >  };
> > +
> > +struct prci_clk_desc prci_clk_fu740 = {
> > +     .clks = __prci_init_clocks_fu740,
> > +     .num_clks = ARRAY_SIZE(__prci_init_clocks_fu740),
> > +};
> > diff --git a/drivers/clk/sifive/fu740-prci.h b/drivers/clk/sifive/fu740-prci.h
> > index 511a0bf7ba2b..5bc0e18f058c 100644
> > --- a/drivers/clk/sifive/fu740-prci.h
> > +++ b/drivers/clk/sifive/fu740-prci.h
> > @@ -7,15 +7,6 @@
> >  #ifndef __SIFIVE_CLK_FU740_PRCI_H
> >  #define __SIFIVE_CLK_FU740_PRCI_H
> >
> > -#include "sifive-prci.h"
> > -
> > -#define NUM_CLOCK_FU740      9
> > -
> > -extern struct __prci_clock __prci_init_clocks_fu740[NUM_CLOCK_FU740];
> > -
> > -static const struct prci_clk_desc prci_clk_fu740 = {
> > -     .clks = __prci_init_clocks_fu740,
> > -     .num_clks = ARRAY_SIZE(__prci_init_clocks_fu740),
> > -};
> > +extern struct prci_clk_desc prci_clk_fu740;
> >
> >  #endif /* __SIFIVE_CLK_FU740_PRCI_H */
> > 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),
> > -};
> > -
> >  /*
> >   * Private functions
> >   */
>
> --
> Lee Jones [李琼斯]
> Principal Technical Lead - Developer Services
> Linaro.org │ Open source software for Arm SoCs
> Follow Linaro: Facebook | Twitter | Blog

Lee Jones Jan. 11, 2022, 9:32 a.m. UTC | #5

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.

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?

> > > --- 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?

> > > +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:

 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?

Zong Li Jan. 12, 2022, 2:47 a.m. UTC | #6

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.

Many thanks for your reply.

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

Lee Jones Jan. 12, 2022, 9:09 a.m. UTC | #7

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.

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.

Zong Li Jan. 13, 2022, 6:47 a.m. UTC | #8

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).

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

Lee Jones Jan. 13, 2022, 5:21 p.m. UTC | #9

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 Jan. 13, 2022, 5:22 p.m. UTC | #10

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!

Jessica Clarke Jan. 13, 2022, 5:51 p.m. UTC | #11

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

Lee Jones Jan. 13, 2022, 6:13 p.m. UTC | #12

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.

Zong Li Jan. 14, 2022, 9:45 a.m. UTC | #13

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

Lee Jones Jan. 14, 2022, 10:07 a.m. UTC | #14

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?

Zong Li Jan. 14, 2022, 2:59 p.m. UTC | #15

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

Lee Jones Jan. 14, 2022, 4:34 p.m. UTC | #16

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.

Zong Li Jan. 15, 2022, 6:19 a.m. UTC | #17

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

Zong Li Jan. 19, 2022, 9:38 a.m. UTC | #18

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

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

Commit Message

Comments

Patch