[U-Boot] [PATCH] ARM: at91: sama5d2: Wrap cpu detection to fix macb driver

[U-Boot] [PATCH] ARM: at91: sama5d2: Wrap cpu detection to fix macb driver

[Alexander Dahl]

When introducing the SAMA5D27 SoCs, the SAMA5D2 series got an additional
chip id. The check if the cpu is sama5d2 was changed from a preprocessor
definition (inlining a call to 'get_chip_id()') to a C function,
probably to not call get_chip_id twice?

That however broke a check in the macb ethernet driver. That driver is
more generic and also used for other platforms. I suppose this solution
was implemented to use it in 'gem_is_gigabit_capable()', without having
to stricly depend on the at91 platform:

	#ifndef cpu_is_sama5d2
	#define cpu_is_sama5d2() 0
	#endif

That only works as long as cpu_is_sama5d2 is a preprocessor definition.
(The same is still true for sama5d4 by the way.) So this is a straight
forward fix for the workaround.

The not working check on the SAMA5D2 CPU lead to an issue on a custom
board with a LAN8720A ethernet phy connected to the SoC:

	=> dhcp
	ethernet at f8008000: PHY present at 1
	ethernet at f8008000: Starting autonegotiation...
	ethernet at f8008000: Autonegotiation complete
	ethernet at f8008000: link up, 1000Mbps full-duplex (lpa: 0xffff)
	BOOTP broadcast 1
	BOOTP broadcast 2
	BOOTP broadcast 3
	BOOTP broadcast 4
	BOOTP broadcast 5
	BOOTP broadcast 6
	BOOTP broadcast 7
	BOOTP broadcast 8
	BOOTP broadcast 9
	BOOTP broadcast 10
	BOOTP broadcast 11
	BOOTP broadcast 12
	BOOTP broadcast 13
	BOOTP broadcast 14
	BOOTP broadcast 15
	BOOTP broadcast 16
	BOOTP broadcast 17

	Retry time exceeded; starting again

Notice the wrong reported link speed, although both SoC and phy only
support 100 MBit/s!

The real issue on reliably detecting the features of that cadence
ethernet mac IP block, is probably more complicated, though.

Fixes: 245cbc583db7c1ca52aa32428b8e86f3449d4af2
Signed-off-by: Alexander Dahl <ada@thorsis.com>
---
 arch/arm/mach-at91/armv7/sama5d2_devices.c | 2 +-
 arch/arm/mach-at91/include/mach/sama5d2.h  | 5 ++++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-at91/armv7/sama5d2_devices.c b/arch/arm/mach-at91/armv7/sama5d2_devices.c
index 6432f66c82..59a0c44913 100644
--- a/arch/arm/mach-at91/armv7/sama5d2_devices.c
+++ b/arch/arm/mach-at91/armv7/sama5d2_devices.c
@@ -9,7 +9,7 @@
 #include <asm/arch/clk.h>
 #include <asm/arch/sama5d2.h>
 
-int cpu_is_sama5d2(void)
+int _cpu_is_sama5d2(void)
 {
 	unsigned int chip_id = get_chip_id();
 
diff --git a/arch/arm/mach-at91/include/mach/sama5d2.h b/arch/arm/mach-at91/include/mach/sama5d2.h
index 37806cbf34..c7d9bb5ad3 100644
--- a/arch/arm/mach-at91/include/mach/sama5d2.h
+++ b/arch/arm/mach-at91/include/mach/sama5d2.h
@@ -222,6 +222,9 @@
 #define ARCH_EXID_SAMA5D27C_D1G		0x00000033
 #define ARCH_EXID_SAMA5D28C_D1G		0x00000013
 
+/* Checked if defined in ethernet driver macb */
+#define cpu_is_sama5d2	_cpu_is_sama5d2
+
 /* PIT Timer(PIT_PIIR) */
 #define CONFIG_SYS_TIMER_COUNTER	0xf804803c
 
@@ -231,7 +234,7 @@
 #ifndef __ASSEMBLY__
 unsigned int get_chip_id(void);
 unsigned int get_extension_chip_id(void);
-int cpu_is_sama5d2(void);
+int _cpu_is_sama5d2(void);
 unsigned int has_lcdc(void);
 char *get_cpu_name(void);
 #endif
-- 
2.11.0

[U-Boot] [PATCH] ARM: at91: sama5d2: Wrap cpu detection to fix macb driver

[Alexander Dahl]

Hei hei,

my use of git send-email messed up my original cc list, so I add Eugen
as maintainer for at91 here. I have some additional comments below.

On Fri, Mar 22, 2019 at 02:25:54PM +0100, Alexander Dahl wrote:
> The not working check on the SAMA5D2 CPU lead to an issue on a custom
> board with a LAN8720A ethernet phy connected to the SoC:

I'm not sure if those macros for checking on certain cpu types are a
good solution at all, other code parts seem to distinguish things by
CONFIG_* variables.

> The real issue on reliably detecting the features of that cadence
> ethernet mac IP block, is probably more complicated, though.

The Linux kernel driver sets cap flags based on the dt compat strings.
I consider that a good approach in general, but I don't know if that's
a way for the U-Boot driver model, too?

Greets
Alex

-- 
/"\ ASCII RIBBON | »With the first link, the chain is forged. The first
\ / CAMPAIGN     | speech censured, the first thought forbidden, the
 X  AGAINST      | first freedom denied, chains us all irrevocably.«
/ \ HTML MAIL    | (Jean-Luc Picard, quoting Judge Aaron Satie)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190323/d2688930/attachment.sig>

[U-Boot] [PATCH] ARM: at91: sama5d2: Wrap cpu detection to fix macb driver

[Bin Meng]

Greetings Alex and everyone, 

Currently I'm trying to block all some PC applications that is appearing as
hidden, system files as well as unknown files on my mobile phones, and any
related to my laptop. It's blocking me from writing proper commands. And
upon every restore, reinstall etc, I will have 4 windows update security
files related to arm devices even though I didn't initiate the update exe.
I'm guessing its trying to force a update on my device?

And after very clean reinstall, reset pc, or repairing etc, I will still
have those hidden files scattered all around each folder. As for any
portable devices, mobile phones including sdcards, portable ssd are affected
as well when they are plugged in. Antivirus software seemed to allow these
files as non virus. McAfee however drops some hints when full scan was
applied but not detailed enough but it states something that a device needs
update etc. 

I'm now at my last attempt to delete all activities that are stored in
cloud, e.g. onedrive, windows activities, etc. Or changing another pc pr
even try to attempt against a different location where the IP address is
different?

Thanks and best regards 
Bin Meng



--
Sent from: http://u-boot.10912.n7.nabble.com/

[U-Boot] [PATCH] ARM: at91: sama5d2: Wrap cpu detection to fix macb driver

[Alexander Dahl]

Hei hei,

one thing still puzzled me, why did it work on SAMA5D27-SOM1-EK1 board, but 
not on our custom board? I think I know the reason now. 

As Nicolas Ferre replied on my confusion on that MID register, on SAMA5D2 that 
register reads 0x00020203, so the value extracted for determining if that mac 
hardware block is of type GEM or not reads 2 on this SoC. Without the fix 
fbcaa260e5cdaeb0cc153c823e034076ac6a6902 the macb driver wouldn't recognize 
the SAMA5D2 as having the GEM variant of the mac block and that must have been 
the case here. 

My test setup for the EK board was based on U-Boot v2019.01, which does not 
have that fix (it's in U-Boot master with v2019.04-rc3), but I backported it 
to my tree for our custom board. ¯\_(ツ)_/¯

I would propose to add an additional Fixes line, see below.

Greets
Alex

Am Freitag, 22. März 2019, 14:25:54 CET schrieb Alexander Dahl:
> When introducing the SAMA5D27 SoCs, the SAMA5D2 series got an additional
> chip id. The check if the cpu is sama5d2 was changed from a preprocessor
> definition (inlining a call to 'get_chip_id()') to a C function,
> probably to not call get_chip_id twice?
> 
> That however broke a check in the macb ethernet driver. That driver is
> more generic and also used for other platforms. I suppose this solution
> was implemented to use it in 'gem_is_gigabit_capable()', without having
> to stricly depend on the at91 platform:
> 
> 	#ifndef cpu_is_sama5d2
> 	#define cpu_is_sama5d2() 0
> 	#endif
> 
> That only works as long as cpu_is_sama5d2 is a preprocessor definition.
> (The same is still true for sama5d4 by the way.) So this is a straight
> forward fix for the workaround.
> 
> The not working check on the SAMA5D2 CPU lead to an issue on a custom
> board with a LAN8720A ethernet phy connected to the SoC:
> 
> 	=> dhcp
> 	ethernet at f8008000: PHY present at 1
> 	ethernet at f8008000: Starting autonegotiation...
> 	ethernet at f8008000: Autonegotiation complete
> 	ethernet at f8008000: link up, 1000Mbps full-duplex (lpa: 0xffff)
> 	BOOTP broadcast 1
> 	BOOTP broadcast 2
> 	BOOTP broadcast 3
> 	BOOTP broadcast 4
> 	BOOTP broadcast 5
> 	BOOTP broadcast 6
> 	BOOTP broadcast 7
> 	BOOTP broadcast 8
> 	BOOTP broadcast 9
> 	BOOTP broadcast 10
> 	BOOTP broadcast 11
> 	BOOTP broadcast 12
> 	BOOTP broadcast 13
> 	BOOTP broadcast 14
> 	BOOTP broadcast 15
> 	BOOTP broadcast 16
> 	BOOTP broadcast 17
> 
> 	Retry time exceeded; starting again
> 
> Notice the wrong reported link speed, although both SoC and phy only
> support 100 MBit/s!
> 
> The real issue on reliably detecting the features of that cadence
> ethernet mac IP block, is probably more complicated, though.
> 
> Fixes: 245cbc583db7c1ca52aa32428b8e86f3449d4af2
> Signed-off-by: Alexander Dahl <ada@thorsis.com>

Fixes: fbcaa260e5cdaeb0cc153c823e034076ac6a6902

> ---
>  arch/arm/mach-at91/armv7/sama5d2_devices.c | 2 +-
>  arch/arm/mach-at91/include/mach/sama5d2.h  | 5 ++++-
>  2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/mach-at91/armv7/sama5d2_devices.c
> b/arch/arm/mach-at91/armv7/sama5d2_devices.c index 6432f66c82..59a0c44913
> 100644
> --- a/arch/arm/mach-at91/armv7/sama5d2_devices.c
> +++ b/arch/arm/mach-at91/armv7/sama5d2_devices.c
> @@ -9,7 +9,7 @@
>  #include <asm/arch/clk.h>
>  #include <asm/arch/sama5d2.h>
> 
> -int cpu_is_sama5d2(void)
> +int _cpu_is_sama5d2(void)
>  {
>  	unsigned int chip_id = get_chip_id();
> 
> diff --git a/arch/arm/mach-at91/include/mach/sama5d2.h
> b/arch/arm/mach-at91/include/mach/sama5d2.h index 37806cbf34..c7d9bb5ad3
> 100644
> --- a/arch/arm/mach-at91/include/mach/sama5d2.h
> +++ b/arch/arm/mach-at91/include/mach/sama5d2.h
> @@ -222,6 +222,9 @@
>  #define ARCH_EXID_SAMA5D27C_D1G		0x00000033
>  #define ARCH_EXID_SAMA5D28C_D1G		0x00000013
> 
> +/* Checked if defined in ethernet driver macb */
> +#define cpu_is_sama5d2	_cpu_is_sama5d2
> +
>  /* PIT Timer(PIT_PIIR) */
>  #define CONFIG_SYS_TIMER_COUNTER	0xf804803c
> 
> @@ -231,7 +234,7 @@
>  #ifndef __ASSEMBLY__
>  unsigned int get_chip_id(void);
>  unsigned int get_extension_chip_id(void);
> -int cpu_is_sama5d2(void);
> +int _cpu_is_sama5d2(void);
>  unsigned int has_lcdc(void);
>  char *get_cpu_name(void);
>  #endif

[U-Boot] [PATCH] ARM: at91: sama5d2: Wrap cpu detection to fix macb driver

[Eugen.Hristev at microchip.com]

On 25.03.2019 11:17, Alexander Dahl wrote:

> Hei hei,
> 
> one thing still puzzled me, why did it work on SAMA5D27-SOM1-EK1 board, but
> not on our custom board? I think I know the reason now.
> 
> As Nicolas Ferre replied on my confusion on that MID register, on SAMA5D2 that
> register reads 0x00020203, so the value extracted for determining if that mac
> hardware block is of type GEM or not reads 2 on this SoC. Without the fix
> fbcaa260e5cdaeb0cc153c823e034076ac6a6902 the macb driver wouldn't recognize
> the SAMA5D2 as having the GEM variant of the mac block and that must have been
> the case here.
> 
> My test setup for the EK board was based on U-Boot v2019.01, which does not
> have that fix (it's in U-Boot master with v2019.04-rc3), but I backported it
> to my tree for our custom board. ¯\_(ツ)_/¯
> 
> I would propose to add an additional Fixes line, see below.
> 
> Greets
> Alex
> 
> Am Freitag, 22. März 2019, 14:25:54 CET schrieb Alexander Dahl:
>> When introducing the SAMA5D27 SoCs, the SAMA5D2 series got an additional
>> chip id. The check if the cpu is sama5d2 was changed from a preprocessor
>> definition (inlining a call to 'get_chip_id()') to a C function,
>> probably to not call get_chip_id twice?
>>
>> That however broke a check in the macb ethernet driver. That driver is
>> more generic and also used for other platforms. I suppose this solution
>> was implemented to use it in 'gem_is_gigabit_capable()', without having
>> to stricly depend on the at91 platform:
>>
>> 	#ifndef cpu_is_sama5d2
>> 	#define cpu_is_sama5d2() 0
>> 	#endif
>>
>> That only works as long as cpu_is_sama5d2 is a preprocessor definition.
>> (The same is still true for sama5d4 by the way.) So this is a straight
>> forward fix for the workaround.
>>
>> The not working check on the SAMA5D2 CPU lead to an issue on a custom
>> board with a LAN8720A ethernet phy connected to the SoC:
>>
>> 	=> dhcp
>> 	ethernet at f8008000: PHY present at 1
>> 	ethernet at f8008000: Starting autonegotiation...
>> 	ethernet at f8008000: Autonegotiation complete
>> 	ethernet at f8008000: link up, 1000Mbps full-duplex (lpa: 0xffff)
>> 	BOOTP broadcast 1
>> 	BOOTP broadcast 2
>> 	BOOTP broadcast 3
>> 	BOOTP broadcast 4
>> 	BOOTP broadcast 5
>> 	BOOTP broadcast 6
>> 	BOOTP broadcast 7
>> 	BOOTP broadcast 8
>> 	BOOTP broadcast 9
>> 	BOOTP broadcast 10
>> 	BOOTP broadcast 11
>> 	BOOTP broadcast 12
>> 	BOOTP broadcast 13
>> 	BOOTP broadcast 14
>> 	BOOTP broadcast 15
>> 	BOOTP broadcast 16
>> 	BOOTP broadcast 17
>>
>> 	Retry time exceeded; starting again
>>
>> Notice the wrong reported link speed, although both SoC and phy only
>> support 100 MBit/s!
>>
>> The real issue on reliably detecting the features of that cadence
>> ethernet mac IP block, is probably more complicated, though.
>>
>> Fixes: 245cbc583db7c1ca52aa32428b8e86f3449d4af2
>> Signed-off-by: Alexander Dahl <ada@thorsis.com>
> 
> Fixes: fbcaa260e5cdaeb0cc153c823e034076ac6a6902

Hi,

Are you sure there is a problem with this commit itself:

net: macb: Fix GEM hardware detection

as it looks , this commit fixes the interpretation of the MID register 
w.r.t. the gem capability... not an issue there that your patch fixes.

More likely your patch fixes the fact that the sama5d2 soc was not 
properly defining that macro, which was turned into a function, so your 
patch fixes the code that changed the macro into a function, which broke 
things... is my understanding correct ?

Eugen

> 
>> ---
>>   arch/arm/mach-at91/armv7/sama5d2_devices.c | 2 +-
>>   arch/arm/mach-at91/include/mach/sama5d2.h  | 5 ++++-
>>   2 files changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm/mach-at91/armv7/sama5d2_devices.c
>> b/arch/arm/mach-at91/armv7/sama5d2_devices.c index 6432f66c82..59a0c44913
>> 100644
>> --- a/arch/arm/mach-at91/armv7/sama5d2_devices.c
>> +++ b/arch/arm/mach-at91/armv7/sama5d2_devices.c
>> @@ -9,7 +9,7 @@
>>   #include <asm/arch/clk.h>
>>   #include <asm/arch/sama5d2.h>
>>
>> -int cpu_is_sama5d2(void)
>> +int _cpu_is_sama5d2(void)
>>   {
>>   	unsigned int chip_id = get_chip_id();
>>
>> diff --git a/arch/arm/mach-at91/include/mach/sama5d2.h
>> b/arch/arm/mach-at91/include/mach/sama5d2.h index 37806cbf34..c7d9bb5ad3
>> 100644
>> --- a/arch/arm/mach-at91/include/mach/sama5d2.h
>> +++ b/arch/arm/mach-at91/include/mach/sama5d2.h
>> @@ -222,6 +222,9 @@
>>   #define ARCH_EXID_SAMA5D27C_D1G		0x00000033
>>   #define ARCH_EXID_SAMA5D28C_D1G		0x00000013
>>
>> +/* Checked if defined in ethernet driver macb */
>> +#define cpu_is_sama5d2	_cpu_is_sama5d2
>> +
>>   /* PIT Timer(PIT_PIIR) */
>>   #define CONFIG_SYS_TIMER_COUNTER	0xf804803c
>>
>> @@ -231,7 +234,7 @@
>>   #ifndef __ASSEMBLY__
>>   unsigned int get_chip_id(void);
>>   unsigned int get_extension_chip_id(void);
>> -int cpu_is_sama5d2(void);
>> +int _cpu_is_sama5d2(void);
>>   unsigned int has_lcdc(void);
>>   char *get_cpu_name(void);
>>   #endif
> 
> 
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot
> 

[U-Boot] [PATCH] ARM: at91: sama5d2: Wrap cpu detection to fix macb driver

[Alexander Dahl]

Hello Eugen,

Am Montag, 25. März 2019, 09:46:02 CET schrieb Eugen.Hristev at microchip.com:
> Are you sure there is a problem with this commit itself:
> 
> net: macb: Fix GEM hardware detection
> 
> as it looks , this commit fixes the interpretation of the MID register 
> w.r.t. the gem capability... not an issue there that your patch fixes.

Well, without the changeset "net: macb: Fix GEM hardware detection" the actual 
problem I had, would not trigger, because the SAMA5D2 is not recognized having 
the gem capability. That's why I asked if that additional fixes line is 
correct.

> More likely your patch fixes the fact that the sama5d2 soc was not 
> properly defining that macro, which was turned into a function, so your 
> patch fixes the code that changed the macro into a function, which broke 
> things... is my understanding correct ?

My patch fixes that soc detection only. The behaviour on SAMA5D2 however is as 
follows:

1) Without both patches (as in v2019.01): SAMA5D2 mac is not detected as gem → 
soc detection in function 'gem_is_gigabit_capable()' is not relevant for 
gigabit detection -> ethernet works (maybe accidentally, gem actually used 
like old mac?)

2) With my patch only: mac is not detected as gem AND soc is detected as 
SAMA5D2 → no gigabit detected → ethernet might work as in 1), not tested 
though

3) With the gem hw detect fix only (as in v2019.04-rc3 already): mac is 
detected as gem, soc is not detected as SAMA5D2 → falsely detected gigabit 
capability → ethernet broken (can you reproduce this?)

4) With both patches: hw recognized as gem, soc recognized as SAMA5D2 → mac 
used as gem, but without gigabit caps → works for me on custom board

Greets
Alex

[U-Boot] [PATCH] ARM: at91: sama5d2: Wrap cpu detection to fix macb driver

[Eugen.Hristev at microchip.com]

On 22.03.2019 15:25, Alexander Dahl wrote:

> When introducing the SAMA5D27 SoCs, the SAMA5D2 series got an additional
> chip id. The check if the cpu is sama5d2 was changed from a preprocessor
> definition (inlining a call to 'get_chip_id()') to a C function,
> probably to not call get_chip_id twice?
> 
> That however broke a check in the macb ethernet driver. That driver is
> more generic and also used for other platforms. I suppose this solution
> was implemented to use it in 'gem_is_gigabit_capable()', without having
> to stricly depend on the at91 platform:
> 
> 	#ifndef cpu_is_sama5d2
> 	#define cpu_is_sama5d2() 0
> 	#endif
> 
> That only works as long as cpu_is_sama5d2 is a preprocessor definition.
> (The same is still true for sama5d4 by the way.) So this is a straight
> forward fix for the workaround.
> 
> The not working check on the SAMA5D2 CPU lead to an issue on a custom
> board with a LAN8720A ethernet phy connected to the SoC:
> 
> 	=> dhcp
> 	ethernet at f8008000: PHY present at 1
> 	ethernet at f8008000: Starting autonegotiation...
> 	ethernet at f8008000: Autonegotiation complete
> 	ethernet at f8008000: link up, 1000Mbps full-duplex (lpa: 0xffff)
> 	BOOTP broadcast 1
> 	BOOTP broadcast 2
> 	BOOTP broadcast 3
> 	BOOTP broadcast 4
> 	BOOTP broadcast 5
> 	BOOTP broadcast 6
> 	BOOTP broadcast 7
> 	BOOTP broadcast 8
> 	BOOTP broadcast 9
> 	BOOTP broadcast 10
> 	BOOTP broadcast 11
> 	BOOTP broadcast 12
> 	BOOTP broadcast 13
> 	BOOTP broadcast 14
> 	BOOTP broadcast 15
> 	BOOTP broadcast 16
> 	BOOTP broadcast 17
> 
> 	Retry time exceeded; starting again
> 
> Notice the wrong reported link speed, although both SoC and phy only
> support 100 MBit/s!
> 
> The real issue on reliably detecting the features of that cadence
> ethernet mac IP block, is probably more complicated, though.
> 
> Fixes: 245cbc583db7c1ca52aa32428b8e86f3449d4af2
> Signed-off-by: Alexander Dahl <ada@thorsis.com>
> ---

Applied to u-boot-atmel/next , with a minor tweak on fixes tag
Thanks !