drivers: ethernet: gd32: GigaDevice ENET support #47227

alsrgv · 2022-07-04T05:17:13Z

Adds GigaDevice ENET support implemented via HAL. Tested on GD32450Z devkit. PTP support tested using #46557.

Signed-off-by: Alex Sergeev asergeev@carbonrobotics.com

alsrgv · 2022-07-04T05:46:26Z

Compliance checks found misspelling on ENET_PTP_SUBSTRACT_FROM_TIME. Unfortunately, it's misspelled in HAL as well - what can I do to suppress it?

nandojve · 2022-07-04T06:19:54Z

Hi @alsrgv,

Thank you for your PR.

CC: @soburi @cameled @feilongfl

gmarull

Thanks for this contribution, a few initial comments

gmarull · 2022-07-04T09:45:33Z

drivers/ethernet/CMakeLists.txt

@@ -28,6 +28,7 @@ zephyr_library_sources_ifdef(CONFIG_ETH_STM32_HAL	eth_stm32_hal.c)
 zephyr_library_sources_ifdef(CONFIG_ETH_W5500		eth_w5500.c)
 zephyr_library_sources_ifdef(CONFIG_ETH_SAM_GMAC	eth_sam_gmac.c)
 zephyr_library_sources_ifdef(CONFIG_DSA_KSZ8XXX		dsa_ksz8xxx.c)
+zephyr_library_sources_ifdef(CONFIG_ETH_GD32_HAL	eth_gd32_hal.c)


let's just call it eth_gd32

Rename just the file(s), or all the defines and function names as well, e.g. ETH_GD32_HAL_RX_THREAD_STACK_SIZE -> ETH_GD32_RX_THREAD_STACK_SIZE, eth_gd32_hal_dev_data -> eth_gd32_dev_data?

gmarull · 2022-07-04T09:46:55Z

drivers/ethernet/Kconfig.gd32_hal

+choice ETH_GD32_HAL_PHY
+	prompt "PHY chip"
+	default ETH_GD32_HAL_PHY_DP83848
+
+config ETH_GD32_HAL_PHY_DP83848
+	bool "DP83848"
+	help
+	  Use DP83848 PHY.
+
+config ETH_GD32_HAL_PHY_LAN8700
+	bool "LAN8700"
+	help
+	  Use LAN8700 series PHY.
+
+endchoice


why should PHYs be hardcoded in Kconfig?

Agreed, why isn't this coming from devicetree (dts/bindings/ethernet/ethernet-phy.yaml)

Please, take look at Atmel ETH driver. It uses the ethernet-phy and MDIO driver.

zephyr/boards/arm/sam_v71_xult/sam_v71_xult-common.dtsi

Lines 220 to 239 in 01909a2

&gmac {

status = "okay";

pinctrl-0 = <&gmac_rmii>;

pinctrl-names = "default";

phy: phy {

compatible = "ethernet-phy";

status = "okay";

address = <0>;

mdio = <&mdio>;

};

};

&mdio {

status = "okay";

pinctrl-0 = <&mdio_default>;

pinctrl-names = "default";

};

Hi @alsrgv, nice to see you have find the time to submit your work!

For the MDIO part you can take a look at what i have done in sbourdelin@183c902

And how i integrate it using the ethernet-phy and DTS in sbourdelin@f558a96

Note that it was still a WIP tho

I see! That looks nice, I wasn't sure how much progress you've made so far. I don't think this approach works with their HAL - they're pretty rigid about PHY interaction.

When do you estimate your register version be ready, and would it have PTP support?

If GD32 HAL is designed with PHYs hardcoded I hardly see how it can be used. Zephyr drivers should not be constrained by how HALs do things internally. Such HALs may be useful in a bare-metal or freertos, but Zephyr has its own mechanisms, like DT.

In general, I think that low level HALs (like STM32 LL or register based) play well with Zephyr. However, higher level ones tend to be inefficient and force design patterns which are incompatible with Zephyr. Such HALs collide easily with Zephyr because they tend to do things like pin control (e.g., nrfx, infineon), clock control (e.g., TI), power management (e.g., TI, partly nrfx), hardware description, etc. It's nice to have this available when e.g. in FreeRTOS where you have nothing else than a scheduler and a few primitives. However, all these areas have a vendor independent solution in Zephyr: pinctrl, clock_control, PM subsys, Devicetree…

I see! That looks nice, I wasn't sure how much progress you've made so far. I don't think this approach works with their HAL - they're pretty rigid about PHY interaction.

When do you estimate your register version be ready, and would it have PTP support?

I'm not working on it right now, i don't know when i will have spare time to continue but i was struggling with the PHY to get the Link Status using my MDIO driver, probably something weird in my code and i'm lacking knowledge on the Ethernet stack so it takes me a lot of time to progress.
But i would be happy to see someone make it works to understand what i m missing, so feel free to re-use what you want in my approach.

It's not entirely true that PHY is hardcoded in this HAL - both PHY address and PHY type can be overridden in the way I'm doing it via Kconfig, and I got it working with 2 PHYs (dev kit and the board we built use different PHYs), it's just not as pretty. STM32 ethernet driver similarly uses Kconfig to specify PHY address.

What do you guys want to do here? Abandon this PR? Or merge it until @sbourdelin PR comes along?

drivers/ethernet/Kconfig.gd32_hal

gmarull · 2022-07-04T09:48:22Z

drivers/ethernet/Kconfig.gd32_hal

+config PTP_CLOCK_GD32_HAL
+	bool "GigaDevice GD32 HAL PTP clock driver support"
+	default y
+	depends on PTP_CLOCK || NET_L2_PTP
+	help
+	  Enable GigaDevice GD32 PTP clock support.
+
+config ETH_GD32_HAL_PTP_CLOCK_SRC_HZ
+	int "Frequency of the clock source for the PTP timer"
+	default 50000000
+	depends on PTP_CLOCK_GD32_HAL
+	help
+	  Set the frequency in Hz sourced to the PTP timer.
+	  If the value is set properly, the timer will be accurate.
+
+config ETH_GD32_HAL_PTP_CLOCK_ADJ_MIN_PCT
+	int "Lower bound of clock frequency adjustment (in percent)"
+	default 90
+	depends on PTP_CLOCK_GD32_HAL
+	help
+	  Specifies lower bound of PTP clock rate adjustment.
+
+config ETH_GD32_HAL_PTP_CLOCK_ADJ_MAX_PCT
+	int "Upper bound of clock frequency adjustment (in percent)"
+	default 110
+	depends on PTP_CLOCK_GD32_HAL
+	help
+	  Specifies upper bound of PTP clock rate adjustment.
+
+config ETH_GD32_HAL_PTP_CLOCK_INIT_PRIO
+	int
+	default 85
+	depends on PTP_CLOCK_GD32_HAL
+	help
+	  GigaDevice GD32 PTP Clock initialization priority level.
+	  There is a dependency from the network stack that this device
+	  initializes before network stack (NET_INIT_PRIO).
+


nit: to ease review, please add PTP support in a new commit

I'll do it once we agree on all the renames that need to happen - prefer to do those over a single commit.

drivers/ethernet/eth_gd32_hal.c

gmarull · 2022-07-04T09:58:57Z

drivers/ethernet/eth_gd32_hal.c

+DEVICE_DEFINE(gd32_ptp_clock_0, PTP_CLOCK_NAME, ptp_gd32_init,
+	      NULL, &ptp_gd32_0_context, NULL, POST_KERNEL,
+	      CONFIG_ETH_GD32_HAL_PTP_CLOCK_INIT_PRIO, &api);


why can't this be a DT device?

I looked around and haven't seen PTP being a device tree device - can you provide an example?

gmarull · 2022-07-04T10:01:05Z

modules/hal_gigadevice/CMakeLists.txt

+# PHY configuration.
+zephyr_compile_definitions(_PHY_H_=1)
+# PHY address determined by the hardware
+zephyr_compile_definitions(PHY_ADDRESS=${CONFIG_ETH_GD32_HAL_PHY_ADDRESS})
+# PHY read write timeouts
+zephyr_compile_definitions(PHY_READ_TO=0x0004FFFFU)
+zephyr_compile_definitions(PHY_WRITE_TO=0x0004FFFFU)
+# PHY delay
+zephyr_compile_definitions(PHY_RESETDELAY=0x008FFFFFU)
+zephyr_compile_definitions(PHY_CONFIGDELAY=0x00FFFFFFU)
+# PHY register address
+zephyr_compile_definitions(PHY_REG_BCR=0U)
+zephyr_compile_definitions(PHY_REG_BSR=1U)
+# PHY basic control register
+zephyr_compile_definitions(PHY_RESET=0x8000U)
+zephyr_compile_definitions(PHY_LOOPBACK=0x4000U)
+zephyr_compile_definitions(PHY_FULLDUPLEX_100M=0x2100U)
+zephyr_compile_definitions(PHY_HALFDUPLEX_100M=0x2000U)
+zephyr_compile_definitions(PHY_FULLDUPLEX_10M=0x0100U)
+zephyr_compile_definitions(PHY_HALFDUPLEX_10M=0x0000U)
+zephyr_compile_definitions(PHY_AUTONEGOTIATION=0x1000U)
+zephyr_compile_definitions(PHY_RESTART_AUTONEGOTIATION=0x0200U)
+zephyr_compile_definitions(PHY_POWERDOWN=0x0800U)
+zephyr_compile_definitions(PHY_ISOLATE=0x0400U)
+# PHY basic status register
+zephyr_compile_definitions(PHY_AUTONEGO_COMPLETE=0x0020U)
+zephyr_compile_definitions(PHY_LINKED_STATUS=0x0004U)
+zephyr_compile_definitions(PHY_JABBER_DETECTION=0x0002U)
+# PHY transceiver status register
+if(CONFIG_ETH_GD32_HAL_PHY_LAN8700)
+  zephyr_compile_definitions(PHY_SR=31U)
+  zephyr_compile_definitions(PHY_SPEED_STATUS=0x0004U)
+  zephyr_compile_definitions(PHY_DUPLEX_STATUS=0x0010U)
+elseif(CONFIG_ETH_GD32_HAL_PHY_DP83848)
+  zephyr_compile_definitions(PHY_SR=16U)
+  zephyr_compile_definitions(PHY_SPEED_STATUS=0x0002U)
+  zephyr_compile_definitions(PHY_DUPLEX_STATUS=0x0004U)
+endif()


why are all these hardcodings required? I'm worried that all these are needed because of a poor HAL design. I'll review this part more in detail.

Yes, unfortunately this is due to HAL design. This motivated me to put PHY selection into Kconfig. If I move PHY selection to device tree, is there a way for me to reference device tree information in CMake to inject it like this?

galak · 2022-07-04T12:08:13Z

dts/arm/gigadevice/gd32f4xx/gd32f4xx.dtsi

+		mac: ethernet@40028000 {
+			compatible = "gd,gd32-ethernet";
+			reg = <0x40028000 0x8000>;
+			label = "ETH_0";


remove label property.

Can you explain the reason? Many of devices above have labels, e.g. TIMER_13.

drivers/ethernet/eth_gd32_hal.c

boards/arm/gd32f450z_eval/gd32f450z_eval-pinctrl.dtsi

boards/arm/gd32f450z_eval/gd32f450z_eval.dts

nandojve · 2022-07-04T17:40:49Z

drivers/ethernet/Kconfig.gd32_hal

+choice ETH_GD32_HAL_PHY
+	prompt "PHY chip"
+	default ETH_GD32_HAL_PHY_DP83848
+
+config ETH_GD32_HAL_PHY_DP83848
+	bool "DP83848"
+	help
+	  Use DP83848 PHY.
+
+config ETH_GD32_HAL_PHY_LAN8700
+	bool "LAN8700"
+	help
+	  Use LAN8700 series PHY.
+
+endchoice


Please, take look at Atmel ETH driver. It uses the ethernet-phy and MDIO driver.

zephyr/boards/arm/sam_v71_xult/sam_v71_xult-common.dtsi

Lines 220 to 239 in 01909a2

&gmac {

status = "okay";

pinctrl-0 = <&gmac_rmii>;

pinctrl-names = "default";

phy: phy {

compatible = "ethernet-phy";

status = "okay";

address = <0>;

mdio = <&mdio>;

};

};

&mdio {

status = "okay";

pinctrl-0 = <&mdio_default>;

pinctrl-names = "default";

};

drivers/ethernet/eth_gd32_hal_priv.h

drivers/ethernet/eth_gd32_hal.c

Adds GigaDevice ENET support implemented via HAL. Tested on GD32450Z devkit. Signed-off-by: Alex Sergeev <asergeev@carbonrobotics.com>

alsrgv · 2022-07-04T20:37:43Z

Thanks for the quick feedback! I resolved most of the easy ones, the main remaining uncertainty appears to be PHY. GD32 HAL encapsulates PHY and it's configured via a set of #define's in gd32f4xx_enet.h. These can be overridden externally via -D as long as -D_PHY_H is specified. I'm not sure how to make this work with PHY settings specified in a device tree, any suggestions?

nandojve · 2022-08-01T10:56:54Z

Hi @alsrgv ,

Thanks for the quick feedback! I resolved most of the easy ones, the main remaining uncertainty appears to be PHY. GD32 HAL encapsulates PHY and it's configured via a set of #define's in gd32f4xx_enet.h. These can be overridden externally via -D as long as -D_PHY_H is specified. I'm not sure how to make this work with PHY settings specified in a device tree, any suggestions?

Do you think is possible bring to zephyr driver the enet_init method from gigadevice original driver? I think if it is possible the part that initializes phy can be handled by mdio driver. If you look code rx_thread exists only because of phy itself and that will be handled by mdio part. Is that make sense somehow?

github-actions · 2022-10-01T00:46:20Z

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

alsrgv requested review from nandojve, tbursztyka and pfalcon as code owners July 4, 2022 05:17

zephyrbot added area: Devicetree platform: GD32 GigaDevice area: Ethernet manifest-hal_gigadevice labels Jul 4, 2022

zephyrbot requested review from galak, gmarull and mbolivar-nordic July 4, 2022 05:17

zephyrbot assigned nandojve Jul 4, 2022

alsrgv force-pushed the asergeev/gd32_enet branch 2 times, most recently from 5036307 to dcdeff7 Compare July 4, 2022 05:37

nandojve mentioned this pull request Jul 4, 2022

[RFC] GigaDevice Vendor Support #38657

Open

55 tasks

gmarull reviewed Jul 4, 2022

View reviewed changes

galak reviewed Jul 4, 2022

View reviewed changes

drivers/ethernet/eth_gd32_hal.c Outdated Show resolved Hide resolved

nandojve requested changes Jul 4, 2022

View reviewed changes

alsrgv force-pushed the asergeev/gd32_enet branch 7 times, most recently from b299516 to e16194c Compare July 4, 2022 19:49

drivers: ethernet: gd32: GigaDevice ENET support

6352454

Adds GigaDevice ENET support implemented via HAL. Tested on GD32450Z devkit. Signed-off-by: Alex Sergeev <asergeev@carbonrobotics.com>

alsrgv force-pushed the asergeev/gd32_enet branch from e16194c to 6352454 Compare July 4, 2022 20:28

carlescufi requested review from nandojve, gmarull and galak July 15, 2022 11:42

nandojve requested a review from cameled August 1, 2022 10:35

github-actions bot added the Stale label Oct 1, 2022

github-actions bot closed this Oct 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: ethernet: gd32: GigaDevice ENET support #47227

drivers: ethernet: gd32: GigaDevice ENET support #47227

alsrgv commented Jul 4, 2022 •

edited

Loading

alsrgv commented Jul 4, 2022

nandojve commented Jul 4, 2022

gmarull left a comment

gmarull Jul 4, 2022

alsrgv Jul 4, 2022

gmarull Jul 4, 2022

galak Jul 4, 2022

nandojve Jul 4, 2022

sbourdelin Jul 6, 2022

alsrgv Jul 6, 2022

gmarull Jul 7, 2022

sbourdelin Jul 7, 2022

alsrgv Jul 7, 2022

gmarull Jul 4, 2022

alsrgv Jul 4, 2022

gmarull Jul 4, 2022

alsrgv Jul 4, 2022

gmarull Jul 4, 2022

alsrgv Jul 4, 2022

alsrgv Jul 4, 2022

galak Jul 4, 2022

alsrgv Jul 4, 2022

nandojve Jul 4, 2022

alsrgv commented Jul 4, 2022

nandojve commented Aug 1, 2022

github-actions bot commented Oct 1, 2022

	&gmac {
	status = "okay";

	pinctrl-0 = <&gmac_rmii>;
	pinctrl-names = "default";

	phy: phy {
	compatible = "ethernet-phy";
	status = "okay";
	address = <0>;
	mdio = <&mdio>;
	};
	};

	&mdio {
	status = "okay";

	pinctrl-0 = <&mdio_default>;
	pinctrl-names = "default";
	};

drivers: ethernet: gd32: GigaDevice ENET support #47227

drivers: ethernet: gd32: GigaDevice ENET support #47227

Conversation

alsrgv commented Jul 4, 2022 • edited Loading

alsrgv commented Jul 4, 2022

nandojve commented Jul 4, 2022

gmarull left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alsrgv commented Jul 4, 2022

nandojve commented Aug 1, 2022

github-actions bot commented Oct 1, 2022

alsrgv commented Jul 4, 2022 •

edited

Loading