2021-05-16 17:03:17

by Dmitry Osipenko

[permalink] [raw]
Subject: [PATCH v2 0/4] Enable compile-testing of Tegra memory drivers

This series enables compile-testing for all of NVIDIA Tegra memory
drivers.

Changelog:

v2: - Added patch which should fix compilation warning of tegra124-emc driver
on 64bit platforms that was reported by kernel build robot. Thanks
to Nathan Chancellor for the suggested fix.

memory: tegra124: Fix compilation warnings on 64bit platforms

- Added missing stubs to the tegra-clk header in another new patch. This
was also reported by kernel build robot for v1.

clk: tegra: Add stubs needed for compile-testing

- The memory/tegra/Kconfig now uses `if TEGRA_MC`, which was suggested
by Krzysztof Kozlowski to v1. This makes Tegra Kconfig to look consistent
with the Exynos Kconfig.

Dmitry Osipenko (4):
soc/tegra: fuse: Add missing stubs
clk: tegra: Add stubs needed for compile-testing
memory: tegra124-emc: Fix compilation warnings on 64bit platforms
memory: tegra: Enable compile testing for all drivers

drivers/memory/tegra/Kconfig | 16 ++++++++++------
drivers/memory/tegra/tegra124-emc.c | 4 ++--
include/linux/clk/tegra.h | 28 ++++++++++++++++++++++++----
include/soc/tegra/fuse.h | 20 +++++++++++++++++---
4 files changed, 53 insertions(+), 15 deletions(-)

--
2.30.2



2021-05-16 17:05:55

by Dmitry Osipenko

[permalink] [raw]
Subject: [PATCH v2 3/4] memory: tegra124-emc: Fix compilation warnings on 64bit platforms

Fix compilation warning on 64bit platforms caused by implicit promotion
of 32bit signed integer to a 64bit unsigned value which happens after
enabling compile-testing of the driver.

Suggested-by: Nathan Chancellor <[email protected]>
Signed-off-by: Dmitry Osipenko <[email protected]>
---
drivers/memory/tegra/tegra124-emc.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/memory/tegra/tegra124-emc.c b/drivers/memory/tegra/tegra124-emc.c
index 5699d909abc2..c9eb948cf4df 100644
--- a/drivers/memory/tegra/tegra124-emc.c
+++ b/drivers/memory/tegra/tegra124-emc.c
@@ -272,8 +272,8 @@
#define EMC_PUTERM_ADJ 0x574

#define DRAM_DEV_SEL_ALL 0
-#define DRAM_DEV_SEL_0 (2 << 30)
-#define DRAM_DEV_SEL_1 (1 << 30)
+#define DRAM_DEV_SEL_0 (2u << 30)
+#define DRAM_DEV_SEL_1 (1u << 30)

#define EMC_CFG_POWER_FEATURES_MASK \
(EMC_CFG_DYN_SREF | EMC_CFG_DRAM_ACPD | EMC_CFG_DRAM_CLKSTOP_SR | \
--
2.30.2


2021-05-16 17:08:36

by Dmitry Osipenko

[permalink] [raw]
Subject: [PATCH v2 4/4] memory: tegra: Enable compile testing for all drivers

Enable compile testing for all Tegra memory drivers.

Signed-off-by: Dmitry Osipenko <[email protected]>
---
drivers/memory/tegra/Kconfig | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/memory/tegra/Kconfig b/drivers/memory/tegra/Kconfig
index a70967a56e52..c63ffa74ab94 100644
--- a/drivers/memory/tegra/Kconfig
+++ b/drivers/memory/tegra/Kconfig
@@ -2,16 +2,18 @@
config TEGRA_MC
bool "NVIDIA Tegra Memory Controller support"
default y
- depends on ARCH_TEGRA
+ depends on ARCH_TEGRA || COMPILE_TEST
select INTERCONNECT
help
This driver supports the Memory Controller (MC) hardware found on
NVIDIA Tegra SoCs.

+if TEGRA_MC
+
config TEGRA20_EMC
tristate "NVIDIA Tegra20 External Memory Controller driver"
default y
- depends on TEGRA_MC && ARCH_TEGRA_2x_SOC
+ depends on ARCH_TEGRA_2x_SOC || COMPILE_TEST
select DEVFREQ_GOV_SIMPLE_ONDEMAND
select PM_DEVFREQ
help
@@ -23,7 +25,7 @@ config TEGRA20_EMC
config TEGRA30_EMC
tristate "NVIDIA Tegra30 External Memory Controller driver"
default y
- depends on TEGRA_MC && ARCH_TEGRA_3x_SOC
+ depends on ARCH_TEGRA_3x_SOC || COMPILE_TEST
select PM_OPP
help
This driver is for the External Memory Controller (EMC) found on
@@ -34,8 +36,8 @@ config TEGRA30_EMC
config TEGRA124_EMC
tristate "NVIDIA Tegra124 External Memory Controller driver"
default y
- depends on TEGRA_MC && ARCH_TEGRA_124_SOC
- select TEGRA124_CLK_EMC
+ depends on ARCH_TEGRA_124_SOC || COMPILE_TEST
+ select TEGRA124_CLK_EMC if ARCH_TEGRA
select PM_OPP
help
This driver is for the External Memory Controller (EMC) found on
@@ -49,10 +51,12 @@ config TEGRA210_EMC_TABLE

config TEGRA210_EMC
tristate "NVIDIA Tegra210 External Memory Controller driver"
- depends on TEGRA_MC && ARCH_TEGRA_210_SOC
+ depends on ARCH_TEGRA_210_SOC || COMPILE_TEST
select TEGRA210_EMC_TABLE
help
This driver is for the External Memory Controller (EMC) found on
Tegra210 chips. The EMC controls the external DRAM on the board.
This driver is required to change memory timings / clock rate for
external memory.
+
+endif
--
2.30.2


2021-05-16 21:22:47

by Dmitry Osipenko

[permalink] [raw]
Subject: [PATCH v2 2/4] clk: tegra: Add stubs needed for compile-testing

Add stubs needed for compile-testing of Tegra memory drivers.

Signed-off-by: Dmitry Osipenko <[email protected]>
---
include/linux/clk/tegra.h | 28 ++++++++++++++++++++++++----
1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/include/linux/clk/tegra.h b/include/linux/clk/tegra.h
index f7ff722a03dd..d540b2879c26 100644
--- a/include/linux/clk/tegra.h
+++ b/include/linux/clk/tegra.h
@@ -144,17 +144,37 @@ typedef long (tegra20_clk_emc_round_cb)(unsigned long rate,
unsigned long min_rate,
unsigned long max_rate,
void *arg);
+typedef int (tegra124_emc_prepare_timing_change_cb)(struct tegra_emc *emc,
+ unsigned long rate);
+typedef void (tegra124_emc_complete_timing_change_cb)(struct tegra_emc *emc,
+ unsigned long rate);

+#ifdef CONFIG_ARCH_TEGRA
void tegra20_clk_set_emc_round_callback(tegra20_clk_emc_round_cb *round_cb,
void *cb_arg);
int tegra20_clk_prepare_emc_mc_same_freq(struct clk *emc_clk, bool same);

-typedef int (tegra124_emc_prepare_timing_change_cb)(struct tegra_emc *emc,
- unsigned long rate);
-typedef void (tegra124_emc_complete_timing_change_cb)(struct tegra_emc *emc,
- unsigned long rate);
void tegra124_clk_set_emc_callbacks(tegra124_emc_prepare_timing_change_cb *prep_cb,
tegra124_emc_complete_timing_change_cb *complete_cb);
+#else
+static inline void
+tegra20_clk_set_emc_round_callback(tegra20_clk_emc_round_cb *round_cb,
+ void *cb_arg)
+{
+}
+
+static inline int
+tegra20_clk_prepare_emc_mc_same_freq(struct clk *emc_clk, bool same)
+{
+ return 0;
+}
+
+static inline void
+tegra124_clk_set_emc_callbacks(tegra124_emc_prepare_timing_change_cb *prep_cb,
+ tegra124_emc_complete_timing_change_cb *complete_cb)
+{
+}
+#endif

struct tegra210_clk_emc_config {
unsigned long rate;
--
2.30.2


2021-05-16 21:47:06

by Dmitry Osipenko

[permalink] [raw]
Subject: [PATCH v2 1/4] soc/tegra: fuse: Add missing stubs

Add missing stubs that will allow Tegra memory driver to be compile-tested
by kernel build bots.

Signed-off-by: Dmitry Osipenko <[email protected]>
---
include/soc/tegra/fuse.h | 20 +++++++++++++++++---
1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/include/soc/tegra/fuse.h b/include/soc/tegra/fuse.h
index 78cbc787a4dc..990701f788bc 100644
--- a/include/soc/tegra/fuse.h
+++ b/include/soc/tegra/fuse.h
@@ -52,14 +52,28 @@ struct tegra_sku_info {
enum tegra_revision revision;
};

+#ifdef CONFIG_ARCH_TEGRA
+extern struct tegra_sku_info tegra_sku_info;
u32 tegra_read_straps(void);
u32 tegra_read_ram_code(void);
int tegra_fuse_readl(unsigned long offset, u32 *value);
-
-#ifdef CONFIG_ARCH_TEGRA
-extern struct tegra_sku_info tegra_sku_info;
#else
static struct tegra_sku_info tegra_sku_info __maybe_unused;
+
+static inline u32 tegra_read_straps(void)
+{
+ return 0;
+}
+
+static inline u32 tegra_read_ram_code(void)
+{
+ return 0;
+}
+
+static inline int tegra_fuse_readl(unsigned long offset, u32 *value)
+{
+ return -ENODEV;
+}
#endif

struct device *tegra_soc_device_register(void);
--
2.30.2


2021-05-17 11:33:43

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v2 0/4] Enable compile-testing of Tegra memory drivers

On 16/05/2021 12:12, Dmitry Osipenko wrote:
> This series enables compile-testing for all of NVIDIA Tegra memory
> drivers.
>
> Changelog:
>
> v2: - Added patch which should fix compilation warning of tegra124-emc driver
> on 64bit platforms that was reported by kernel build robot. Thanks
> to Nathan Chancellor for the suggested fix.
>
> memory: tegra124: Fix compilation warnings on 64bit platforms
>
> - Added missing stubs to the tegra-clk header in another new patch. This
> was also reported by kernel build robot for v1.
>
> clk: tegra: Add stubs needed for compile-testing
>
> - The memory/tegra/Kconfig now uses `if TEGRA_MC`, which was suggested
> by Krzysztof Kozlowski to v1. This makes Tegra Kconfig to look consistent
> with the Exynos Kconfig.
>

Hi Thierry,

The memory drivers part depends on soc and clk patches. There is also
another series from Dmitry (devm_tegra_core_dev_init_opp_table()) with
memory-soc dependency. I guess it makes sense you take everything via
soc-tegra, but just in case, can you keep the memory drivers on
dedicated branch?


Best regards,
Krzysztof

2021-05-17 11:35:10

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v2 4/4] memory: tegra: Enable compile testing for all drivers

On 16/05/2021 12:12, Dmitry Osipenko wrote:
> Enable compile testing for all Tegra memory drivers.
>
> Signed-off-by: Dmitry Osipenko <[email protected]>
> ---
> drivers/memory/tegra/Kconfig | 16 ++++++++++------
> 1 file changed, 10 insertions(+), 6 deletions(-)
>
This depends on clk/soc changes, so I am fine to take it via other tree:

Reviewed-by: Krzysztof Kozlowski <[email protected]>

Best regards,
Krzysztof

2021-05-17 12:11:28

by Nathan Chancellor

[permalink] [raw]
Subject: Re: [PATCH v2 3/4] memory: tegra124-emc: Fix compilation warnings on 64bit platforms

On 5/16/2021 9:12 AM, Dmitry Osipenko wrote:
> Fix compilation warning on 64bit platforms caused by implicit promotion
> of 32bit signed integer to a 64bit unsigned value which happens after
> enabling compile-testing of the driver.
>
> Suggested-by: Nathan Chancellor <[email protected]>
> Signed-off-by: Dmitry Osipenko <[email protected]>

Reviewed-by: Nathan Chancellor <[email protected]>

> ---
> drivers/memory/tegra/tegra124-emc.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/memory/tegra/tegra124-emc.c b/drivers/memory/tegra/tegra124-emc.c
> index 5699d909abc2..c9eb948cf4df 100644
> --- a/drivers/memory/tegra/tegra124-emc.c
> +++ b/drivers/memory/tegra/tegra124-emc.c
> @@ -272,8 +272,8 @@
> #define EMC_PUTERM_ADJ 0x574
>
> #define DRAM_DEV_SEL_ALL 0
> -#define DRAM_DEV_SEL_0 (2 << 30)
> -#define DRAM_DEV_SEL_1 (1 << 30)
> +#define DRAM_DEV_SEL_0 (2u << 30)
> +#define DRAM_DEV_SEL_1 (1u << 30)
>
> #define EMC_CFG_POWER_FEATURES_MASK \
> (EMC_CFG_DYN_SREF | EMC_CFG_DRAM_ACPD | EMC_CFG_DRAM_CLKSTOP_SR | \
>


2021-05-17 14:10:55

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v2 3/4] memory: tegra124-emc: Fix compilation warnings on 64bit platforms

On 16/05/2021 12:12, Dmitry Osipenko wrote:
> Fix compilation warning on 64bit platforms caused by implicit promotion
> of 32bit signed integer to a 64bit unsigned value which happens after
> enabling compile-testing of the driver.
>
> Suggested-by: Nathan Chancellor <[email protected]>
> Signed-off-by: Dmitry Osipenko <[email protected]>
> ---
> drivers/memory/tegra/tegra124-emc.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/memory/tegra/tegra124-emc.c b/drivers/memory/tegra/tegra124-emc.c
> index 5699d909abc2..c9eb948cf4df 100644
> --- a/drivers/memory/tegra/tegra124-emc.c
> +++ b/drivers/memory/tegra/tegra124-emc.c
> @@ -272,8 +272,8 @@
> #define EMC_PUTERM_ADJ 0x574
>
> #define DRAM_DEV_SEL_ALL 0
> -#define DRAM_DEV_SEL_0 (2 << 30)
> -#define DRAM_DEV_SEL_1 (1 << 30)
> +#define DRAM_DEV_SEL_0 (2u << 30)
> +#define DRAM_DEV_SEL_1 (1u << 30)

Why not using BIT()? This would make even this 2<<30 less awkard...

Best regards,
Krzysztof

2021-05-17 14:20:24

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v2 3/4] memory: tegra124-emc: Fix compilation warnings on 64bit platforms

On 17/05/2021 09:35, Dmitry Osipenko wrote:
> 17.05.2021 14:28, Krzysztof Kozlowski пишет:
>> On 16/05/2021 12:12, Dmitry Osipenko wrote:
>>> Fix compilation warning on 64bit platforms caused by implicit promotion
>>> of 32bit signed integer to a 64bit unsigned value which happens after
>>> enabling compile-testing of the driver.
>>>
>>> Suggested-by: Nathan Chancellor <[email protected]>
>>> Signed-off-by: Dmitry Osipenko <[email protected]>
>>> ---
>>> drivers/memory/tegra/tegra124-emc.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/memory/tegra/tegra124-emc.c b/drivers/memory/tegra/tegra124-emc.c
>>> index 5699d909abc2..c9eb948cf4df 100644
>>> --- a/drivers/memory/tegra/tegra124-emc.c
>>> +++ b/drivers/memory/tegra/tegra124-emc.c
>>> @@ -272,8 +272,8 @@
>>> #define EMC_PUTERM_ADJ 0x574
>>>
>>> #define DRAM_DEV_SEL_ALL 0
>>> -#define DRAM_DEV_SEL_0 (2 << 30)
>>> -#define DRAM_DEV_SEL_1 (1 << 30)
>>> +#define DRAM_DEV_SEL_0 (2u << 30)
>>> +#define DRAM_DEV_SEL_1 (1u << 30)
>>
>> Why not using BIT()? This would make even this 2<<30 less awkard...
>
> The bitfield 31:30 is a enum, 3 is a wrong value. Formally it's
> incorrect to use the BIT() macro here.

Why "3"? BIT(31) is the same as 2<<30. It's common to use BIT for
register fields which do not accept all possible values. Now you
basically reimplement BIT() which is error-prone.


Best regards,
Krzysztof

2021-05-17 19:30:37

by Dmitry Osipenko

[permalink] [raw]
Subject: Re: [PATCH v2 3/4] memory: tegra124-emc: Fix compilation warnings on 64bit platforms

17.05.2021 14:28, Krzysztof Kozlowski пишет:
> On 16/05/2021 12:12, Dmitry Osipenko wrote:
>> Fix compilation warning on 64bit platforms caused by implicit promotion
>> of 32bit signed integer to a 64bit unsigned value which happens after
>> enabling compile-testing of the driver.
>>
>> Suggested-by: Nathan Chancellor <[email protected]>
>> Signed-off-by: Dmitry Osipenko <[email protected]>
>> ---
>> drivers/memory/tegra/tegra124-emc.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/memory/tegra/tegra124-emc.c b/drivers/memory/tegra/tegra124-emc.c
>> index 5699d909abc2..c9eb948cf4df 100644
>> --- a/drivers/memory/tegra/tegra124-emc.c
>> +++ b/drivers/memory/tegra/tegra124-emc.c
>> @@ -272,8 +272,8 @@
>> #define EMC_PUTERM_ADJ 0x574
>>
>> #define DRAM_DEV_SEL_ALL 0
>> -#define DRAM_DEV_SEL_0 (2 << 30)
>> -#define DRAM_DEV_SEL_1 (1 << 30)
>> +#define DRAM_DEV_SEL_0 (2u << 30)
>> +#define DRAM_DEV_SEL_1 (1u << 30)
>
> Why not using BIT()? This would make even this 2<<30 less awkard...

The bitfield 31:30 is a enum, 3 is a wrong value. Formally it's
incorrect to use the BIT() macro here.

2021-05-17 19:32:35

by Dmitry Osipenko

[permalink] [raw]
Subject: Re: [PATCH v2 3/4] memory: tegra124-emc: Fix compilation warnings on 64bit platforms

17.05.2021 16:39, Krzysztof Kozlowski пишет:
> On 17/05/2021 09:35, Dmitry Osipenko wrote:
>> 17.05.2021 14:28, Krzysztof Kozlowski пишет:
>>> On 16/05/2021 12:12, Dmitry Osipenko wrote:
>>>> Fix compilation warning on 64bit platforms caused by implicit promotion
>>>> of 32bit signed integer to a 64bit unsigned value which happens after
>>>> enabling compile-testing of the driver.
>>>>
>>>> Suggested-by: Nathan Chancellor <[email protected]>
>>>> Signed-off-by: Dmitry Osipenko <[email protected]>
>>>> ---
>>>> drivers/memory/tegra/tegra124-emc.c | 4 ++--
>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/memory/tegra/tegra124-emc.c b/drivers/memory/tegra/tegra124-emc.c
>>>> index 5699d909abc2..c9eb948cf4df 100644
>>>> --- a/drivers/memory/tegra/tegra124-emc.c
>>>> +++ b/drivers/memory/tegra/tegra124-emc.c
>>>> @@ -272,8 +272,8 @@
>>>> #define EMC_PUTERM_ADJ 0x574
>>>>
>>>> #define DRAM_DEV_SEL_ALL 0
>>>> -#define DRAM_DEV_SEL_0 (2 << 30)
>>>> -#define DRAM_DEV_SEL_1 (1 << 30)
>>>> +#define DRAM_DEV_SEL_0 (2u << 30)
>>>> +#define DRAM_DEV_SEL_1 (1u << 30)
>>>
>>> Why not using BIT()? This would make even this 2<<30 less awkard...
>>
>> The bitfield 31:30 is a enum, 3 is a wrong value. Formally it's
>> incorrect to use the BIT() macro here.
>
> Why "3"? BIT(31) is the same as 2<<30.

By 3 I meant BIT(31)|BIT(30). This bitfield is explicitly designated as
a enum in the hardware documentation.

> It's common to use BIT for
> register fields which do not accept all possible values. Now you
> basically reimplement BIT() which is error-prone.

Could you please show couple examples? The common practice today is to
use FIELD_PREP helpers, but this driver was written before these helpers
existed.

2021-05-17 21:10:44

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v2 3/4] memory: tegra124-emc: Fix compilation warnings on 64bit platforms

On 17/05/2021 09:47, Dmitry Osipenko wrote:
> 17.05.2021 16:39, Krzysztof Kozlowski пишет:
>>>>> #define DRAM_DEV_SEL_ALL 0
>>>>> -#define DRAM_DEV_SEL_0 (2 << 30)
>>>>> -#define DRAM_DEV_SEL_1 (1 << 30)
>>>>> +#define DRAM_DEV_SEL_0 (2u << 30)
>>>>> +#define DRAM_DEV_SEL_1 (1u << 30)
>>>>
>>>> Why not using BIT()? This would make even this 2<<30 less awkard...
>>>
>>> The bitfield 31:30 is a enum, 3 is a wrong value. Formally it's
>>> incorrect to use the BIT() macro here.
>>
>> Why "3"? BIT(31) is the same as 2<<30.
>
> By 3 I meant BIT(31)|BIT(30). This bitfield is explicitly designated as
> a enum in the hardware documentation.

I understand it and using BIT() here does not mean someone has to set
both of them. BIT() is a helper pointing out that you want to toggle one
bit. It does not mean that it is allowed to do so always!

>
>> It's common to use BIT for
>> register fields which do not accept all possible values. Now you
>> basically reimplement BIT() which is error-prone.
>
> Could you please show couple examples? The common practice today is to
> use FIELD_PREP helpers, but this driver was written before these helpers
> existed.


There are plenty of such examples so I guess it would be easier to ask
you to provide counter ones. Few IT for enum-like registers found within 2 minutes:

https://elixir.bootlin.com/linux/latest/C/ident/MAX77620_CNFG_GPIO_INT_MASK
https://elixir.bootlin.com/linux/v5.13-rc2/source/drivers/regulator/max77650-regulator.c#L18
https://elixir.bootlin.com/linux/v5.13-rc2/source/drivers/regulator/tps6524x-regulator.c#L62
https://elixir.bootlin.com/linux/v5.13-rc2/source/drivers/regulator/tps80031-regulator.c#L39
https://elixir.bootlin.com/linux/v5.13-rc2/source/drivers/regulator/da9121-regulator.h#L200
https://elixir.bootlin.com/linux/v5.13-rc2/source/drivers/regulator/da9121-regulator.h#L231

Best regards,
Krzysztof

2021-05-18 02:26:03

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v2 3/4] memory: tegra124-emc: Fix compilation warnings on 64bit platforms

On 16/05/2021 12:12, Dmitry Osipenko wrote:
> Fix compilation warning on 64bit platforms caused by implicit promotion
> of 32bit signed integer to a 64bit unsigned value which happens after
> enabling compile-testing of the driver.
>
> Suggested-by: Nathan Chancellor <[email protected]>
> Signed-off-by: Dmitry Osipenko <[email protected]>
The patch was not suggested by Nathan but it was:
Reported-by: kernel test robot <[email protected]>

Nathan however provided analysis and proper solution, so co-developed or
his SoB fits better. This is not that important as comment above -
including robot's credits.

Best regards,
Krzysztof

2021-05-18 10:13:30

by Dmitry Osipenko

[permalink] [raw]
Subject: Re: [PATCH v2 3/4] memory: tegra124-emc: Fix compilation warnings on 64bit platforms

17.05.2021 17:04, Krzysztof Kozlowski пишет:
> On 17/05/2021 09:47, Dmitry Osipenko wrote:
>> 17.05.2021 16:39, Krzysztof Kozlowski пишет:
>>>>>> #define DRAM_DEV_SEL_ALL 0
>>>>>> -#define DRAM_DEV_SEL_0 (2 << 30)
>>>>>> -#define DRAM_DEV_SEL_1 (1 << 30)
>>>>>> +#define DRAM_DEV_SEL_0 (2u << 30)
>>>>>> +#define DRAM_DEV_SEL_1 (1u << 30)
>>>>>
>>>>> Why not using BIT()? This would make even this 2<<30 less awkard...
>>>>
>>>> The bitfield 31:30 is a enum, 3 is a wrong value. Formally it's
>>>> incorrect to use the BIT() macro here.
>>>
>>> Why "3"? BIT(31) is the same as 2<<30.
>>
>> By 3 I meant BIT(31)|BIT(30). This bitfield is explicitly designated as
>> a enum in the hardware documentation.
>
> I understand it and using BIT() here does not mean someone has to set
> both of them. BIT() is a helper pointing out that you want to toggle one
> bit. It does not mean that it is allowed to do so always!
>
>>
>>> It's common to use BIT for
>>> register fields which do not accept all possible values. Now you
>>> basically reimplement BIT() which is error-prone.
>>
>> Could you please show couple examples? The common practice today is to
>> use FIELD_PREP helpers, but this driver was written before these helpers
>> existed.
>
>
> There are plenty of such examples so I guess it would be easier to ask
> you to provide counter ones. Few IT for enum-like registers found within 2 minutes:
>
> https://elixir.bootlin.com/linux/latest/C/ident/MAX77620_CNFG_GPIO_INT_MASK
> https://elixir.bootlin.com/linux/v5.13-rc2/source/drivers/regulator/max77650-regulator.c#L18
> https://elixir.bootlin.com/linux/v5.13-rc2/source/drivers/regulator/tps6524x-regulator.c#L62
> https://elixir.bootlin.com/linux/v5.13-rc2/source/drivers/regulator/tps80031-regulator.c#L39
> https://elixir.bootlin.com/linux/v5.13-rc2/source/drivers/regulator/da9121-regulator.h#L200
> https://elixir.bootlin.com/linux/v5.13-rc2/source/drivers/regulator/da9121-regulator.h#L231

Alright, I'll use the BIT macro in the v3.

I also realized now that the tegra30-emc drivers needs the same change.

Thank you for the review.

2021-05-18 19:21:23

by Dmitry Osipenko

[permalink] [raw]
Subject: Re: [PATCH v2 3/4] memory: tegra124-emc: Fix compilation warnings on 64bit platforms

17.05.2021 17:24, Krzysztof Kozlowski пишет:
> On 16/05/2021 12:12, Dmitry Osipenko wrote:
>> Fix compilation warning on 64bit platforms caused by implicit promotion
>> of 32bit signed integer to a 64bit unsigned value which happens after
>> enabling compile-testing of the driver.
>>
>> Suggested-by: Nathan Chancellor <[email protected]>
>> Signed-off-by: Dmitry Osipenko <[email protected]>
> The patch was not suggested by Nathan but it was:
> Reported-by: kernel test robot <[email protected]>
>
> Nathan however provided analysis and proper solution, so co-developed or
> his SoB fits better. This is not that important as comment above -
> including robot's credits.

I'll update the tags in v3, thank you.