[PATCH] cn913x: add support for iEi Puzzle-M901/Puzzle-M902

Ian Chang ianchang at ieiworld.com
Wed May 26 00:34:10 PDT 2021


Hi 
Thank you for your advice
> This RTC seems to be supported for a long time by upstream rtc-ds1307 driver [1]. What does not work with upstream one that works with this one?
Yes, it is compatible with rtc-1307, I will use rtc-ds1307 instead of uploading the RX8130 driver


> The above armada-*.dtsi are all already present in vanilla Linux.
> Do not overwrite them here. Please amend them with patches, if needed.
Because kernel 5.4.114 does not contain armada-*.dtsi files, I will compare kernel 5.10 which already contains these files.

Regards
Ian Chang

-----Original Message-----
From: Tomasz Maciej Nowak <tmn505 at gmail.com> 
Sent: Tuesday, May 25, 2021 10:05 PM
To: Daniel Golle <daniel at makrotopia.org>; eveans2002 at gmail.com
Cc: openwrt-devel at lists.openwrt.org; ianchang <ianchang at ieiworld.com>
Subject: Re: [PATCH] cn913x: add support for iEi Puzzle-M901/Puzzle-M902

Hi.

W dniu 25.05.2021 o 10:22, Daniel Golle pisze:
> Hi!
> 
> thank you for submitting support for this interesting hardware.
> There are some minor style issues which need to be addressed before 
> the patch can be merged into our tree.
> It would also be better to split this into at least two patches:
> [1/2] Add driver for Epson RX8130 RTC

This RTC seems to be supported for a long time by upstream rtc-ds1307 driver [1]. What does not work with upstream one that works with this one?

> [2/2] Add support for Puzzla M90x
> 
> Other than that, please see my comments inline below:
> 
> On Mon, May 24, 2021 at 01:58:00AM +0800, eveans2002 at gmail.com wrote:
>> From: ianchang <ianchang at ieiworld.com>
>>
>>  Hardware specification
>>  ----------------------
>>  * CN9130 SoC, Quad-core ARMv8 Cortex-72 @ 2200 MHz
>>  * 4 GB DDR
>>  * 4 GB eMMC
>>  * 4 MB (SPI Flash)
>>  * 6 x 2.5 Gigabit  ports (Puzzle-M901)
>>     - External PHY with 6 ports (AQR112R)
>>  * 6 x 2.5 Gigabit ports (Puzzle-M902)
>>     - External PHY with 6 ports (AQR112R)
>>    3 x 10 Gigabit ports (Puzzle-M902)
>>     - External PHY with 3 ports (AQR113R)
>>
>>  * 4 x Front panel LED
>>  * 1 x USB 3.0
>>  * Reset button on Rear panel
>>  * UART (115200 8N1,header on PCB)
>>
>> Signed-off-by: ianchang <ianchang at ieiworld.com>
> 
> Please use your full real name seperated by spaces (here in SoB and in 
> the Form: line above)
> 
>> ---
>>  .../base-files/etc/board.d/02_network         |    9 +-
>>  target/linux/mvebu/cortexa72/config-5.4       |   14 +
>>  .../boot/dts/marvell/armada-ap807-quad.dtsi   |   93 +
>>  .../arm64/boot/dts/marvell/armada-ap807.dtsi  |   30 +
>>  .../arm64/boot/dts/marvell/armada-ap80x.dtsi  |  464 ++++
>>  .../arm64/boot/dts/marvell/armada-cp115.dtsi  |   12 +
>>  .../arm64/boot/dts/marvell/armada-cp11x.dtsi  |  573 +++++
> 
> The above armada-*.dtsi are all already present in vanilla Linux.
> Do not overwrite them here. Please amend them with patches, if needed.
> 
> 
>>  .../dts/marvell/puzzle-armada-common.dtsi     |   11 +
>>  .../boot/dts/marvell/puzzle-armada-cp110.dtsi |   12 +
>>  .../arm64/boot/dts/marvell/puzzle-cn9130.dtsi |   37 +
>>  .../dts/marvell/puzzle-m901-cn9130-db.dts     |  429 ++++
>>  .../dts/marvell/puzzle-m901-cn9131-db.dts     |  243 +++
>>  .../dts/marvell/puzzle-m902-cn9130-db.dts     |  430 ++++
>>  .../dts/marvell/puzzle-m902-cn9131-db.dts     |  247 +++
>>  .../dts/marvell/puzzle-m902-cn9132-db.dts     |  261 +++
>>  target/linux/mvebu/files/drivers/rtc/Kconfig  | 1943 
>> +++++++++++++++++  target/linux/mvebu/files/drivers/rtc/Makefile |  189 ++
>>  .../mvebu/files/drivers/rtc/rtc-rx8130.c      |  807 +++++++
>>  target/linux/mvebu/image/cortexa72.mk         |   20 +
>>  19 files changed, 5823 insertions(+), 1 deletion(-)  create mode 
>> 100644 
>> target/linux/mvebu/files/arch/arm64/boot/dts/marvell/armada-ap807-qua
>> d.dtsi  create mode 100644 
>> target/linux/mvebu/files/arch/arm64/boot/dts/marvell/armada-ap807.dts
>> i  create mode 100644 
>> target/linux/mvebu/files/arch/arm64/boot/dts/marvell/armada-ap80x.dts
>> i  create mode 100644 
>> target/linux/mvebu/files/arch/arm64/boot/dts/marvell/armada-cp115.dts
>> i  create mode 100644 
>> target/linux/mvebu/files/arch/arm64/boot/dts/marvell/armada-cp11x.dts
>> i  create mode 100644 
>> target/linux/mvebu/files/arch/arm64/boot/dts/marvell/puzzle-armada-co
>> mmon.dtsi  create mode 100644 
>> target/linux/mvebu/files/arch/arm64/boot/dts/marvell/puzzle-armada-cp
>> 110.dtsi  create mode 100644 
>> target/linux/mvebu/files/arch/arm64/boot/dts/marvell/puzzle-cn9130.dt
>> si  create mode 100644 
>> target/linux/mvebu/files/arch/arm64/boot/dts/marvell/puzzle-m901-cn91
>> 30-db.dts  create mode 100644 
>> target/linux/mvebu/files/arch/arm64/boot/dts/marvell/puzzle-m901-cn91
>> 31-db.dts  create mode 100644 
>> target/linux/mvebu/files/arch/arm64/boot/dts/marvell/puzzle-m902-cn91
>> 30-db.dts  create mode 100644 
>> target/linux/mvebu/files/arch/arm64/boot/dts/marvell/puzzle-m902-cn91
>> 31-db.dts  create mode 100644 
>> target/linux/mvebu/files/arch/arm64/boot/dts/marvell/puzzle-m902-cn91
>> 32-db.dts  create mode 100644 
>> target/linux/mvebu/files/drivers/rtc/Kconfig
> 
> You should not overwrite existing files in the kernel tree 
> (drivers/rtc/Kconfig does exist already). Use a patch and add that to 
> target/linux/mvevu/patches-*/ instead to modify files which are 
> present in the Linux kernel tree.
> 
>>  create mode 100644 target/linux/mvebu/files/drivers/rtc/Makefile
> 
> Same here.
> 
> 
>>  create mode 100644 target/linux/mvebu/files/drivers/rtc/rtc-rx8130.c
>>
>> diff --git 
>> a/target/linux/mvebu/cortexa72/base-files/etc/board.d/02_network 
>> b/target/linux/mvebu/cortexa72/base-files/etc/board.d/02_network
>> index 9ab3c8174d..9ad043b343 100755
>> --- a/target/linux/mvebu/cortexa72/base-files/etc/board.d/02_network
>> +++ b/target/linux/mvebu/cortexa72/base-files/etc/board.d/02_network
>> @@ -21,8 +21,15 @@ marvell,armada8040-db)
>>  marvell,armada7040-db)
>>  	ucidef_set_interfaces_lan_wan "eth0 eth2" "eth1"
>>  	;;
>> +marvell,cn9131)
>> +        ucidef_set_interfaces_lan_wan "eth1 eth2 eth3 eth4 eth5" "eth0"
>> +        ;;
>> +marvell,cn9132)
>> +        ucidef_set_interfaces_lan_wan "eth1 eth2 eth3 eth4 eth5 eth10 eth11 eth12" "eth0"
>> +        ;;
>>  *)
>> -	ucidef_set_interface_lan "eth0"
>> +#	ucidef_set_interface_lan "eth0"
>> +        ucidef_set_interfaces_lan_wan "eth0 eth3" "eth6"
> 
> Please keep files with one style of indentation (ie. use tabs for 
> indentation in files already using tabs, use 4 or 8 spaces in files 
> already applying this style). Here you are indenting with spaces in a 
> file which is using tabs.

Aside from tabs vs spaces, there's a lot of comented out code with C++ style in dts, it should be removed and all remaining comments converted to C style.

> 
>>  	;;
>>  esac
>>  
>> diff --git a/target/linux/mvebu/cortexa72/config-5.4 
>> b/target/linux/mvebu/cortexa72/config-5.4
>> index 5727ae5918..919f579157 100644
>> --- a/target/linux/mvebu/cortexa72/config-5.4
>> +++ b/target/linux/mvebu/cortexa72/config-5.4
>> @@ -175,3 +175,17 @@ CONFIG_THREAD_INFO_IN_TASK=y  
>> CONFIG_UNMAP_KERNEL_AT_EL0=y  CONFIG_VMAP_STACK=y  
>> CONFIG_ZONE_DMA32=y
>> +# CONFIG_PUZZLE-M90x
>> +CONFIG_RTC_DRV_RX8130=y
>> +# CONFIG_RTC_DRV_MV is not set
>> +# CONFIG_RTC_DRV_ARMADA38X is not set # Character devices 
>> +CONFIG_ARCH_HAS_DEVMEM_IS_ALLOWED=y
>> +CONFIG_DEVMEM=y

Some time ago there was enablement of DEVMEM for other target [2].
it lacked justification for providing it by default, pleas provide one here.

Aside from this, changes to config for kernel 5.10 are lacking.

[snip]

1. https://www.kernel.org/doc/Documentation/devicetree/bindings/rtc/rtc-ds1307.txt
2. http://lists.infradead.org/pipermail/openwrt-devel/2021-February/033702.html

Regards

--
TMN




More information about the openwrt-devel mailing list