mbox series

[v5,00/17] Basic StarFive JH7110 RISC-V SoC support

Message ID 20230329034224.26545-1-yanhong.wang@starfivetech.com
Headers show
Series Basic StarFive JH7110 RISC-V SoC support | expand

Message

Yanhong Wang March 29, 2023, 3:42 a.m. UTC
This series of patches base on the latest branch/master, and add support
for the StarFive JH7110 RISC-V SoC and VisionFive V2 board. In order for
this to be achieved, the respective DT nodes have been added,  and the
required defconfigs have been added to the boards' defconfig. What is more,
the basic required DM drivers have been added, such as reset, clock, pinctrl,
uart, ram etc.

Note that the register base address of reset controller is same with the
clock controller. Therefore, there is no device tree node alone for reset
driver. It binds device node in the clock driver.

The u-boot-spl and u-boot has been tested on the StarFive VisionFive 2 1.2A
and 1.3B boards which equip with JH7110 SoC and works normally.

For more information and support, you can visit RVspace wiki[1].

[1] https://wiki.rvspace.org/

v5:
- Fixed the build errors that came from "make htmldocs".
- Remove the MMU configuration from the S7 device tree node.
- Splitted starfive_visionfive2_defconfig into starfive_visionfive2_12a_defconfig
  and starfive_visionfive2_13b_defconfig.
- Added the implementation of 'get_function' callback function to pinctrl.

v4:
- Replace compatible string "starfive,jh7110-ccache" with "sifive,ccache0".
- Added 'gmac0_tx_inv','gmac1_tx_inv','gmac1_rx' clock registration.
- Added enable_caches() call to enable L2 cache in board_init() function.
- Moved 'S7_0' device node from board dts to SoC -u-boot.dtsi.
- Added 'i2cx' device node to board dts to consistent with linux.
- Renamed device node 'sdcard1_pins' to 'mmc1_pins'.

v3:
- Added doc/board/starfive/visionfive2.rst file.
- Added support booting from SD.
- Added support pinctrl in SPL.
- Reworded dts to consistent with linux.
- Added CFG_EXTRA_ENV_SETTINGS configuration.
- Reworded starfive_visionfive2_defconfig.
- Reworded the clock driver.
- Renamed 'starfive-jh7110.h' to 'starfive,jh7110-crg.h'.
- Separated 'starfive_visionfive2.dts' to 'jh7110-starfive-visionfive-2-v1.3b.dts'
  and 'jh7110-starfive-visionfive-2-v1.2a.dts' to consistent with linux.
- Added pinmux_property_set callback function implementation in 'pinctrl-starfive.c'.

v2:
- Renamed file 'jh7110-regs.h' to 'regs.h'.
- Reworded the clear L2 LIM memory code in C.
- Removed flash init call in 'spl_soc_init' function.
- Reworded the clock driver.
- Rename the macro 'SET_DIV' to 'ASSIGNED_CLOCK_PARENTS' in 'spl.c'.
- Moved the device tree node 'dmc@15700000' from 'jh7110-u-boot.dtsi' to
  'starfive_visionfive2-u-boot.dtsi'

Previous versions:
v1 - https://patchwork.ozlabs.org/project/uboot/cover/20221212025020.23778-1-yanhong.wang@starfivetech.com/
v2 - https://patchwork.ozlabs.org/project/uboot/cover/20230118081132.31403-1-yanhong.wang@starfivetech.com/
v3 - https://patchwork.ozlabs.org/project/uboot/patch/20230303032432.7837-2-yanhong.wang@starfivetech.com/
v4 - https://patchwork.ozlabs.org/project/uboot/cover/20230316025332.3297-1-yanhong.wang@starfivetech.com/

Jianlong Huang (1):
  dt-bindings: pinctrl: Add StarFive JH7110 pinctrl definitions

Kuan Lim Lee (1):
  pinctrl: starfive: Add StarFive JH7110 driver

Yanhong Wang (15):
  riscv: cpu: jh7110: Add support for jh7110 SoC
  cache: starfive: Add StarFive JH7110 support
  dt-bindings: reset: Add StarFive JH7110 reset definitions
  reset: starfive: jh7110: Add reset driver for StarFive JH7110 SoC
  dt-bindings: clock: Add StarFive JH7110 clock definitions
  clk: starfive: Add StarFive JH7110 clock driver
  ram: starfive: add ddr driver
  board: starfive: add StarFive VisionFive v2 board support
  riscv: cpu: jh7110: Add Kconfig for StarFive JH7110 SoC
  board: starfive: Add Kconfig for StarFive VisionFive v2 Board
  board: starfive: Add TARGET_STARFIVE_VISIONFIVE2 to Kconfig
  riscv: dts: jh7110: Add initial StarFive JH7110 device tree
  riscv: dts: jh7110: Add initial u-boot device tree
  riscv: dts: jh7110: Add initial StarFive VisionFive v2 board device
    tree
  configs: starfive: add defconfig for StarFive VisionFvie2 1.2A and
    1.3B

 arch/riscv/Kconfig                            |    5 +
 arch/riscv/cpu/jh7110/Kconfig                 |   28 +
 arch/riscv/cpu/jh7110/Makefile                |   10 +
 arch/riscv/cpu/jh7110/cpu.c                   |   23 +
 arch/riscv/cpu/jh7110/dram.c                  |   38 +
 arch/riscv/cpu/jh7110/spl.c                   |   64 +
 arch/riscv/dts/Makefile                       |    3 +-
 ...10-starfive-visionfive-2-v1.2a-u-boot.dtsi |   69 +
 .../jh7110-starfive-visionfive-2-v1.2a.dts    |   12 +
 ...10-starfive-visionfive-2-v1.3b-u-boot.dtsi |   69 +
 .../jh7110-starfive-visionfive-2-v1.3b.dts    |   12 +
 .../dts/jh7110-starfive-visionfive-2.dtsi     |  319 +++
 arch/riscv/dts/jh7110-u-boot.dtsi             |   99 +
 arch/riscv/dts/jh7110.dtsi                    |  573 +++++
 arch/riscv/include/asm/arch-jh7110/regs.h     |   19 +
 arch/riscv/include/asm/arch-jh7110/spl.h      |   12 +
 board/starfive/visionfive2/Kconfig            |   53 +
 board/starfive/visionfive2/MAINTAINERS        |    7 +
 board/starfive/visionfive2/Makefile           |    7 +
 board/starfive/visionfive2/spl.c              |   87 +
 .../visionfive2/starfive_visionfive2.c        |   40 +
 configs/starfive_visionfive2_12a_defconfig    |   79 +
 configs/starfive_visionfive2_13b_defconfig    |   79 +
 doc/board/index.rst                           |    1 +
 doc/board/starfive/index.rst                  |    9 +
 doc/board/starfive/visionfive2.rst            |  492 +++++
 drivers/cache/cache-sifive-ccache.c           |    1 +
 drivers/clk/Kconfig                           |    1 +
 drivers/clk/Makefile                          |    1 +
 drivers/clk/starfive/Kconfig                  |   17 +
 drivers/clk/starfive/Makefile                 |    4 +
 drivers/clk/starfive/clk-jh7110-pll.c         |  321 +++
 drivers/clk/starfive/clk-jh7110.c             |  603 +++++
 drivers/clk/starfive/clk.h                    |   57 +
 drivers/pinctrl/Kconfig                       |    1 +
 drivers/pinctrl/Makefile                      |    1 +
 drivers/pinctrl/starfive/Kconfig              |   28 +
 drivers/pinctrl/starfive/Makefile             |    6 +
 drivers/pinctrl/starfive/pinctrl-jh7110-aon.c |  113 +
 drivers/pinctrl/starfive/pinctrl-jh7110-sys.c |  399 ++++
 drivers/pinctrl/starfive/pinctrl-starfive.c   |  398 ++++
 drivers/pinctrl/starfive/pinctrl-starfive.h   |   55 +
 drivers/ram/Kconfig                           |    1 +
 drivers/ram/Makefile                          |    4 +-
 drivers/ram/starfive/Kconfig                  |    5 +
 drivers/ram/starfive/Makefile                 |   11 +
 drivers/ram/starfive/ddrcsr_boot.c            |  339 +++
 drivers/ram/starfive/ddrphy_start.c           |  279 +++
 drivers/ram/starfive/ddrphy_train.c           |  383 ++++
 drivers/ram/starfive/ddrphy_utils.c           | 1955 +++++++++++++++++
 drivers/ram/starfive/starfive_ddr.c           |  161 ++
 drivers/ram/starfive/starfive_ddr.h           |   65 +
 drivers/reset/Kconfig                         |   16 +
 drivers/reset/Makefile                        |    1 +
 drivers/reset/reset-jh7110.c                  |  158 ++
 include/configs/starfive-visionfive2.h        |   49 +
 .../dt-bindings/clock/starfive,jh7110-crg.h   |  257 +++
 .../pinctrl/pinctrl-starfive-jh7110.h         |  427 ++++
 .../dt-bindings/reset/starfive,jh7110-crg.h   |  183 ++
 59 files changed, 8507 insertions(+), 2 deletions(-)
 create mode 100644 arch/riscv/cpu/jh7110/Kconfig
 create mode 100644 arch/riscv/cpu/jh7110/Makefile
 create mode 100644 arch/riscv/cpu/jh7110/cpu.c
 create mode 100644 arch/riscv/cpu/jh7110/dram.c
 create mode 100644 arch/riscv/cpu/jh7110/spl.c
 create mode 100644 arch/riscv/dts/jh7110-starfive-visionfive-2-v1.2a-u-boot.dtsi
 create mode 100644 arch/riscv/dts/jh7110-starfive-visionfive-2-v1.2a.dts
 create mode 100644 arch/riscv/dts/jh7110-starfive-visionfive-2-v1.3b-u-boot.dtsi
 create mode 100644 arch/riscv/dts/jh7110-starfive-visionfive-2-v1.3b.dts
 create mode 100644 arch/riscv/dts/jh7110-starfive-visionfive-2.dtsi
 create mode 100644 arch/riscv/dts/jh7110-u-boot.dtsi
 create mode 100644 arch/riscv/dts/jh7110.dtsi
 create mode 100644 arch/riscv/include/asm/arch-jh7110/regs.h
 create mode 100644 arch/riscv/include/asm/arch-jh7110/spl.h
 create mode 100644 board/starfive/visionfive2/Kconfig
 create mode 100644 board/starfive/visionfive2/MAINTAINERS
 create mode 100644 board/starfive/visionfive2/Makefile
 create mode 100644 board/starfive/visionfive2/spl.c
 create mode 100644 board/starfive/visionfive2/starfive_visionfive2.c
 create mode 100644 configs/starfive_visionfive2_12a_defconfig
 create mode 100644 configs/starfive_visionfive2_13b_defconfig
 create mode 100644 doc/board/starfive/index.rst
 create mode 100644 doc/board/starfive/visionfive2.rst
 create mode 100644 drivers/clk/starfive/Kconfig
 create mode 100644 drivers/clk/starfive/Makefile
 create mode 100644 drivers/clk/starfive/clk-jh7110-pll.c
 create mode 100644 drivers/clk/starfive/clk-jh7110.c
 create mode 100644 drivers/clk/starfive/clk.h
 create mode 100644 drivers/pinctrl/starfive/Kconfig
 create mode 100644 drivers/pinctrl/starfive/Makefile
 create mode 100644 drivers/pinctrl/starfive/pinctrl-jh7110-aon.c
 create mode 100644 drivers/pinctrl/starfive/pinctrl-jh7110-sys.c
 create mode 100644 drivers/pinctrl/starfive/pinctrl-starfive.c
 create mode 100644 drivers/pinctrl/starfive/pinctrl-starfive.h
 create mode 100644 drivers/ram/starfive/Kconfig
 create mode 100644 drivers/ram/starfive/Makefile
 create mode 100644 drivers/ram/starfive/ddrcsr_boot.c
 create mode 100644 drivers/ram/starfive/ddrphy_start.c
 create mode 100644 drivers/ram/starfive/ddrphy_train.c
 create mode 100644 drivers/ram/starfive/ddrphy_utils.c
 create mode 100644 drivers/ram/starfive/starfive_ddr.c
 create mode 100644 drivers/ram/starfive/starfive_ddr.h
 create mode 100644 drivers/reset/reset-jh7110.c
 create mode 100644 include/configs/starfive-visionfive2.h
 create mode 100644 include/dt-bindings/clock/starfive,jh7110-crg.h
 create mode 100644 include/dt-bindings/pinctrl/pinctrl-starfive-jh7110.h
 create mode 100644 include/dt-bindings/reset/starfive,jh7110-crg.h


base-commit: 41a88ad529b3943b1e465846eb24fe2c29203e35

Comments

Torsten Duwe March 29, 2023, 9:41 a.m. UTC | #1
On Wed, 29 Mar 2023 11:42:07 +0800
Yanhong Wang <yanhong.wang@starfivetech.com> wrote:

> v5:
[...]
> - Splitted starfive_visionfive2_defconfig into starfive_visionfive2_12a_defconfig
>   and starfive_visionfive2_13b_defconfig.

Is this really necessary? It puts another burden on people building U-Boot,
distribution networks, and last but not least users, who will need to pick the
correct binary blob, after trying to find out which board they actually have.

Even past versions can detect the installed RAM correctly and will modify
the DT accordingly, I assume? Why not make an inquiry on GMAC1_MDIO to tell
whether it's a YT8512C (->v1.2A) or another YT8531C (->v1.3B), in the ethernet
patch set, and likewise update the device tree dynamically then?

	Torsten
Yanhong Wang March 29, 2023, 10:16 a.m. UTC | #2
On 2023/3/29 17:41, Torsten Duwe wrote:
> On Wed, 29 Mar 2023 11:42:07 +0800
> Yanhong Wang <yanhong.wang@starfivetech.com> wrote:
> 
>> v5:
> [...]
>> - Splitted starfive_visionfive2_defconfig into starfive_visionfive2_12a_defconfig
>>   and starfive_visionfive2_13b_defconfig.
> 
> Is this really necessary? It puts another burden on people building U-Boot,
> distribution networks, and last but not least users, who will need to pick the
> correct binary blob, after trying to find out which board they actually have.
> 
> Even past versions can detect the installed RAM correctly and will modify
> the DT accordingly, I assume? Why not make an inquiry on GMAC1_MDIO to tell
> whether it's a YT8512C (->v1.2A) or another YT8531C (->v1.3B), in the ethernet
> patch set, and likewise update the device tree dynamically then?
> 

There is only one defconfig in V4, and dts is separate for versions 1.2a and 1.3b.
Andreas Schwab suggested that defconfig is also defined separately, so the definition 
of defconfig in V5 is also separated. 

The discussion process  as follows:

https://patchwork.ozlabs.org/project/uboot/patch/20230316025332.3297-18-yanhong.wang@starfivetech.com/

Do you have any better suggestion on whether defconfig is defined separately?

> 	Torsten
Torsten Duwe March 29, 2023, 11:46 a.m. UTC | #3
On Wed, 29 Mar 2023 18:16:20 +0800
yanhong wang <yanhong.wang@starfivetech.com> wrote:

> 
> 
> On 2023/3/29 17:41, Torsten Duwe wrote:
> > On Wed, 29 Mar 2023 11:42:07 +0800
> > Yanhong Wang <yanhong.wang@starfivetech.com> wrote:
> > 
> >> v5:
> > [...]
> >> - Splitted starfive_visionfive2_defconfig into starfive_visionfive2_12a_defconfig
> >>   and starfive_visionfive2_13b_defconfig.
> > 
> > Is this really necessary? It puts another burden on people building U-Boot,
> > distribution networks, and last but not least users, who will need to pick the
> > correct binary blob, after trying to find out which board they actually have.
> > 
> > Even past versions can detect the installed RAM correctly and will modify
> > the DT accordingly, I assume? Why not make an inquiry on GMAC1_MDIO to tell
> > whether it's a YT8512C (->v1.2A) or another YT8531C (->v1.3B), in the ethernet
> > patch set, and likewise update the device tree dynamically then?
> > 
> 
> There is only one defconfig in V4, and dts is separate for versions 1.2a and 1.3b.
> Andreas Schwab suggested that defconfig is also defined separately, so the definition 
> of defconfig in V5 is also separated. 
> 
> The discussion process  as follows:
> 
> https://patchwork.ozlabs.org/project/uboot/patch/20230316025332.3297-18-yanhong.wang@starfivetech.com/

Very well. I'll talk to Andreas. Sorry for the confusion.

> Do you have any better suggestion on whether defconfig is defined separately?

See above. Roughly, define a superset, like a node YT8512C attached to GMAC1, and another
node with a YT8531C attached to GMAC1, both "disabled", and at runtime mark the detected one
"okay", maybe patching the board name or not. But I'll discuss this with Andreas first.

	Torsten
Torsten Duwe April 12, 2023, 5:50 p.m. UTC | #4
On Wed, 29 Mar 2023 18:16:20 +0800
yanhong wang <yanhong.wang@starfivetech.com> wrote:

> 
> 
> On 2023/3/29 17:41, Torsten Duwe wrote:
> > On Wed, 29 Mar 2023 11:42:07 +0800
> > Yanhong Wang <yanhong.wang@starfivetech.com> wrote:
> > 
> >> v5:
> > [...]
> >> - Splitted starfive_visionfive2_defconfig into starfive_visionfive2_12a_defconfig
> >>   and starfive_visionfive2_13b_defconfig.
> > 
> > Is this really necessary? It puts another burden on people building U-Boot,
> > distribution networks, and last but not least users, who will need to pick the
> > correct binary blob, after trying to find out which board they actually have.
> > 
> > Even past versions can detect the installed RAM correctly and will modify
> > the DT accordingly, I assume? Why not make an inquiry on GMAC1_MDIO to tell
> > whether it's a YT8512C (->v1.2A) or another YT8531C (->v1.3B), in the ethernet
> > patch set, and likewise update the device tree dynamically then?

At a second look, this is a bit tricky: a device tree is already needed for the network
initialisation. That one would need to be good enough to get at the PHYs, and flexible
enough to be patched into shape later. But see below...
 
> There is only one defconfig in V4, and dts is separate for versions 1.2a and 1.3b.
> Andreas Schwab suggested that defconfig is also defined separately, so the definition 
> of defconfig in V5 is also separated. 
> 
> The discussion process  as follows:
> 
> https://patchwork.ozlabs.org/project/uboot/patch/20230316025332.3297-18-yanhong.wang@starfivetech.com/
> 
> Do you have any better suggestion on whether defconfig is defined separately?

Andreas' concern is the match between the device tree and the actual hardware,
as far as it matters for (driver) software. So, different hardware => different DT.

However, AFAICT there is no difference until network comes into play, right? And even
then, it is only the types of PHYs and their wiring, correct?

From the other thread: can we enable the EEPROM reading code first, to get the proper
MAC addresses for the hardware, and also read the board revision, similar to
get_pcb_revision_from_eeprom() from the HiFive unmatched? And then use fixup functions
from common/fdt_support.c to adapt the device tree details to the detected board?

	Torsten
Yanhong Wang April 13, 2023, 2:05 a.m. UTC | #5
On 2023/4/13 1:50, Torsten Duwe wrote:
> On Wed, 29 Mar 2023 18:16:20 +0800
> yanhong wang <yanhong.wang@starfivetech.com> wrote:
> 
>> 
>> 
>> On 2023/3/29 17:41, Torsten Duwe wrote:
>> > On Wed, 29 Mar 2023 11:42:07 +0800
>> > Yanhong Wang <yanhong.wang@starfivetech.com> wrote:
>> > 
>> >> v5:
>> > [...]
>> >> - Splitted starfive_visionfive2_defconfig into starfive_visionfive2_12a_defconfig
>> >>   and starfive_visionfive2_13b_defconfig.
>> > 
>> > Is this really necessary? It puts another burden on people building U-Boot,
>> > distribution networks, and last but not least users, who will need to pick the
>> > correct binary blob, after trying to find out which board they actually have.
>> > 
>> > Even past versions can detect the installed RAM correctly and will modify
>> > the DT accordingly, I assume? Why not make an inquiry on GMAC1_MDIO to tell
>> > whether it's a YT8512C (->v1.2A) or another YT8531C (->v1.3B), in the ethernet
>> > patch set, and likewise update the device tree dynamically then?
> 
> At a second look, this is a bit tricky: a device tree is already needed for the network
> initialisation. That one would need to be good enough to get at the PHYs, and flexible
> enough to be patched into shape later. But see below...
>  
>> There is only one defconfig in V4, and dts is separate for versions 1.2a and 1.3b.
>> Andreas Schwab suggested that defconfig is also defined separately, so the definition 
>> of defconfig in V5 is also separated. 
>> 
>> The discussion process  as follows:
>> 
>> https://patchwork.ozlabs.org/project/uboot/patch/20230316025332.3297-18-yanhong.wang@starfivetech.com/
>> 
>> Do you have any better suggestion on whether defconfig is defined separately?
> 
> Andreas' concern is the match between the device tree and the actual hardware,
> as far as it matters for (driver) software. So, different hardware => different DT.
> 
> However, AFAICT there is no difference until network comes into play, right? And even
> then, it is only the types of PHYs and their wiring, correct?
> 

Yes, before gmac and phy were added, everything was the same except for 'model', but
the definition of DT refers to Linux and is consistent with the definition framework of Linux.

The difference between 1.2A and 1.3B is the PHY type and phy clock delay configuration, 
which are reflected in DT, and the difference in defconfig is the configuration of the DT file.

Is defconfig defined separately or merged?

> From the other thread: can we enable the EEPROM reading code first, to get the proper
> MAC addresses for the hardware, and also read the board revision, similar to
> get_pcb_revision_from_eeprom() from the HiFive unmatched? And then use fixup functions
> from common/fdt_support.c to adapt the device tree details to the detected board?
> 

The EEPROM is being prepared and will be submitted as soon as possible. Is it necessary to 
incorporate EEPROM into this submission?

When eeprom is supported, the MAC address will be read from eeprom. The board reversion 
can be read from eeprom, but phy clock delay configuration cannot be read from eeprom, only in DT.

> 	Torsten
Torsten Duwe April 13, 2023, 9:03 a.m. UTC | #6
On Thu, 13 Apr 2023 10:05:28 +0800
yanhong wang <yanhong.wang@starfivetech.com> wrote:

> the definition of DT refers to Linux and is consistent with the definition framework of Linux.

This is one of the desired goals, to avoid confusion, usually. But note there are already the
-u-boot.dtsi files; in this case it would be vice-versa: U-Boot could be simple, the kernel
required a different treatment. As long as the resulting tree matches the hardware!

> The difference between 1.2A and 1.3B is the PHY type and phy clock delay configuration, 
> which are reflected in DT, and the difference in defconfig is the configuration of the DT file.
> 
> Is defconfig defined separately or merged?

You are the implementer, this is your decision. You make a proposal, and it will get accepted
or not. We only make suggestions, with the intention to improve the code.

> The EEPROM is being prepared and will be submitted as soon as possible. Is it necessary to 
> incorporate EEPROM into this submission?
>
> When eeprom is supported, the MAC address will be read from eeprom. The board reversion 
> can be read from eeprom, but phy clock delay configuration cannot be read from eeprom, only in DT.

But the board revision number in EEPROM can be used to differentiate between 1.2 and 1.3, right?

When I look at the code and my test results, this is my proposal to pull this in, in order to
simplify things and avoid duplication. Whether you do so is up to you, see above. Let me recap:

* the device tree *must* match the hardware at hand.

* the differences are minor and could be patched, Copy&Waste is error prone and causes extra work.

It is my firm conviction that this patch set does not introduce hardware variants, and it would be
the task of the ethernet driver patch set to split the code (DT+defconfig) OR to provide a patching
method. Maybe I find a few cycles to look at the EEPROM.

	Torsten
Yanhong Wang April 13, 2023, 10:05 a.m. UTC | #7
On 2023/4/13 17:03, Torsten Duwe wrote:
> On Thu, 13 Apr 2023 10:05:28 +0800
> yanhong wang <yanhong.wang@starfivetech.com> wrote:
> 
>> the definition of DT refers to Linux and is consistent with the definition framework of Linux.
> 
> This is one of the desired goals, to avoid confusion, usually. But note there are already the
> -u-boot.dtsi files; in this case it would be vice-versa: U-Boot could be simple, the kernel
> required a different treatment. As long as the resulting tree matches the hardware!
> 
>> The difference between 1.2A and 1.3B is the PHY type and phy clock delay configuration, 
>> which are reflected in DT, and the difference in defconfig is the configuration of the DT file.
>> 
>> Is defconfig defined separately or merged?
> 
> You are the implementer, this is your decision. You make a proposal, and it will get accepted
> or not. We only make suggestions, with the intention to improve the code.
> 

Thanks. A defconfig matches a piece of hardware, which is more developer-friendly and less confusing, 
so defconfig is better defined separately.

>> The EEPROM is being prepared and will be submitted as soon as possible. Is it necessary to 
>> incorporate EEPROM into this submission?
>>
>> When eeprom is supported, the MAC address will be read from eeprom. The board reversion 
>> can be read from eeprom, but phy clock delay configuration cannot be read from eeprom, only in DT.
> 
> But the board revision number in EEPROM can be used to differentiate between 1.2 and 1.3, right?
> 

Yes, board reversion read from eeprom can distinguish between 1.2A and 1.3B.

1.2A and 1.3B are two sets of hardware, and the differences between the hardware are defined
by DT, which is more concise and clear.

> When I look at the code and my test results, this is my proposal to pull this in, in order to
> simplify things and avoid duplication. Whether you do so is up to you, see above. Let me recap:
> 
> * the device tree *must* match the hardware at hand.
> 
> * the differences are minor and could be patched, Copy&Waste is error prone and causes extra work.
> 
> It is my firm conviction that this patch set does not introduce hardware variants, and it would be
> the task of the ethernet driver patch set to split the code (DT+defconfig) OR to provide a patching
> method. Maybe I find a few cycles to look at the EEPROM.
> 
> 	Torsten
Matthias Brugger April 14, 2023, 3:05 p.m. UTC | #8
On 13/04/2023 12:05, yanhong wang wrote:
> 
> 
> On 2023/4/13 17:03, Torsten Duwe wrote:
>> On Thu, 13 Apr 2023 10:05:28 +0800
>> yanhong wang <yanhong.wang@starfivetech.com> wrote:
>>
>>> the definition of DT refers to Linux and is consistent with the definition framework of Linux.
>>
>> This is one of the desired goals, to avoid confusion, usually. But note there are already the
>> -u-boot.dtsi files; in this case it would be vice-versa: U-Boot could be simple, the kernel
>> required a different treatment. As long as the resulting tree matches the hardware!
>>
>>> The difference between 1.2A and 1.3B is the PHY type and phy clock delay configuration,
>>> which are reflected in DT, and the difference in defconfig is the configuration of the DT file.
>>>
>>> Is defconfig defined separately or merged?
>>
>> You are the implementer, this is your decision. You make a proposal, and it will get accepted
>> or not. We only make suggestions, with the intention to improve the code.
>>
> 
> Thanks. A defconfig matches a piece of hardware, which is more developer-friendly and less confusing,
> so defconfig is better defined separately.
> 

My opinion isIn my opinion user-friendlyness is more important then developer 
friendly that from an end-user point of view it's much easier to have one binary 
that works on all different board versions. If not users will have to know which 
board version they have to flash the correct U-Boot.

For the RaspberryPi we even went further and put effort into U-Boot development 
to have one U-Boot binary which can boot RPi3 and RPi4 boards.

In that sense I would advise you to revisit your decision to put a 
developer-friendly approach over an end-user-friendly one.

As Torsten said, it shouldn't be too difficult to update the device-tree at boot 
time to fit the actual board you are running U-Boot on.

Just my thoughts about the issue :)

Best regards,
Matthias

>>> The EEPROM is being prepared and will be submitted as soon as possible. Is it necessary to
>>> incorporate EEPROM into this submission?
>>>
>>> When eeprom is supported, the MAC address will be read from eeprom. The board reversion
>>> can be read from eeprom, but phy clock delay configuration cannot be read from eeprom, only in DT.
>>
>> But the board revision number in EEPROM can be used to differentiate between 1.2 and 1.3, right?
>>
> 
> Yes, board reversion read from eeprom can distinguish between 1.2A and 1.3B.
> 
> 1.2A and 1.3B are two sets of hardware, and the differences between the hardware are defined
> by DT, which is more concise and clear.
> 
>> When I look at the code and my test results, this is my proposal to pull this in, in order to
>> simplify things and avoid duplication. Whether you do so is up to you, see above. Let me recap:
>>
>> * the device tree *must* match the hardware at hand.
>>
>> * the differences are minor and could be patched, Copy&Waste is error prone and causes extra work.
>>
>> It is my firm conviction that this patch set does not introduce hardware variants, and it would be
>> the task of the ethernet driver patch set to split the code (DT+defconfig) OR to provide a patching
>> method. Maybe I find a few cycles to look at the EEPROM.
>>
>> 	Torsten
Leo Liang April 20, 2023, 3:45 a.m. UTC | #9
Hi YanHong, Torsten, Matthias,

On Thu, Apr 13, 2023 at 06:05:56PM +0800, yanhong wang wrote:
> 
> 
> On 2023/4/13 17:03, Torsten Duwe wrote:
> > On Thu, 13 Apr 2023 10:05:28 +0800
> > yanhong wang <yanhong.wang@starfivetech.com> wrote:
> > 
> >> the definition of DT refers to Linux and is consistent with the definition framework of Linux.
> > 
> > This is one of the desired goals, to avoid confusion, usually. But note there are already the
> > -u-boot.dtsi files; in this case it would be vice-versa: U-Boot could be simple, the kernel
> > required a different treatment. As long as the resulting tree matches the hardware!
> > 
> >> The difference between 1.2A and 1.3B is the PHY type and phy clock delay configuration, 
> >> which are reflected in DT, and the difference in defconfig is the configuration of the DT file.
> >> 
> >> Is defconfig defined separately or merged?
> > 
> > You are the implementer, this is your decision. You make a proposal, and it will get accepted
> > or not. We only make suggestions, with the intention to improve the code.
> > 
> 
> Thanks. A defconfig matches a piece of hardware, which is more developer-friendly and less confusing, 
> so defconfig is better defined separately.
> 
> >> The EEPROM is being prepared and will be submitted as soon as possible. Is it necessary to 
> >> incorporate EEPROM into this submission?
> >>
> >> When eeprom is supported, the MAC address will be read from eeprom. The board reversion 
> >> can be read from eeprom, but phy clock delay configuration cannot be read from eeprom, only in DT.
> > 
> > But the board revision number in EEPROM can be used to differentiate between 1.2 and 1.3, right?
> > 
> 
> Yes, board reversion read from eeprom can distinguish between 1.2A and 1.3B.
> 
> 1.2A and 1.3B are two sets of hardware, and the differences between the hardware are defined
> by DT, which is more concise and clear.
> 
> > When I look at the code and my test results, this is my proposal to pull this in, in order to
> > simplify things and avoid duplication. Whether you do so is up to you, see above. Let me recap:
> > 
> > * the device tree *must* match the hardware at hand.
> > 
> > * the differences are minor and could be patched, Copy&Waste is error prone and causes extra work.
> > 
> > It is my firm conviction that this patch set does not introduce hardware variants, and it would be
> > the task of the ethernet driver patch set to split the code (DT+defconfig) OR to provide a patching
> > method. Maybe I find a few cycles to look at the EEPROM.
> > 
> > 	Torsten

Agree with Torsten.

I too IMHO think it makes much sense that 
whether "to split the (DT + defconfig)" or "patching DT"
should be the task of ethernet driver patch.

However, this patch set is rather complete and stay on the ML
for quite a time. And also Torsten has sent out the RFC patch 
that is going for the patching method.

In that sense, I think I could probably merge this v5 patch set
with [v4 17/17] patch that only introduces a single defconfig,
and wait for Torsten's DT patches if you guys agree.

Any thoughts?

Best regards,
Leo
Andreas Schwab May 2, 2023, 12:41 p.m. UTC | #10
On Apr 14 2023, Matthias Brugger wrote:

> My opinion isIn my opinion user-friendlyness is more important then
> developer friendly that from an end-user point of view it's much easier to
> have one binary that works on all different board versions. If not users
> will have to know which board version they have to flash the correct
> U-Boot.

As long as the kernel uses separate device trees, U-Boot needs to know
which one to load.  As it currently stands, U-Boot uses the wrong name
that does not match either of the names used by the kernel, which means
it will not be able find any of them.
Matthias Brugger May 2, 2023, 12:46 p.m. UTC | #11
On 02/05/2023 14:41, Andreas Schwab wrote:
> On Apr 14 2023, Matthias Brugger wrote:
> 
>> My opinion isIn my opinion user-friendlyness is more important then
>> developer friendly that from an end-user point of view it's much easier to
>> have one binary that works on all different board versions. If not users
>> will have to know which board version they have to flash the correct
>> U-Boot.
> 
> As long as the kernel uses separate device trees, U-Boot needs to know
> which one to load.  As it currently stands, U-Boot uses the wrong name
> that does not match either of the names used by the kernel, which means
> it will not be able find any of them.
> 

I'm not sure I get your point. The devicetree will be passed to the kernel via a 
pointer in a register, the kernel does not need to load the devicetree into 
memory, it will use the one passed by U-Boot.

Regards,
Matthias
Andreas Schwab May 2, 2023, 1:11 p.m. UTC | #12
On Mai 02 2023, Matthias Brugger wrote:

> I'm not sure I get your point. The devicetree will be passed to the kernel
> via a pointer in a register, the kernel does not need to load the
> devicetree into memory, it will use the one passed by U-Boot.

But U-Boot needs to load it, and the kernel is the authority in
providing it.
Heinrich Schuchardt May 2, 2023, 1:24 p.m. UTC | #13
On 5/2/23 15:11, Andreas Schwab wrote:
> On Mai 02 2023, Matthias Brugger wrote:
> 
>> I'm not sure I get your point. The devicetree will be passed to the kernel
>> via a pointer in a register, the kernel does not need to load the
>> devicetree into memory, it will use the one passed by U-Boot.
> 
> But U-Boot needs to load it, and the kernel is the authority in
> providing it.
> 

To make it a bit clearer:

Several U-Boot boot methods use the value of environment variable 
$fdtfile to load a device-tree from file. The same holds true for the 
boot.scr file created by Debian's flash-kernel package.

A good solution would be to read the EEPROM to determine the exact board 
version, to set $fdtfile accordingly and update U-Boots control dtb as 
needed.

Best regards

Heinrich
Yanhong Wang May 4, 2023, 2:35 a.m. UTC | #14
On 2023/5/2 21:24, Heinrich Schuchardt wrote:
> On 5/2/23 15:11, Andreas Schwab wrote:
>> On Mai 02 2023, Matthias Brugger wrote:
>>
>>> I'm not sure I get your point. The devicetree will be passed to the kernel
>>> via a pointer in a register, the kernel does not need to load the
>>> devicetree into memory, it will use the one passed by U-Boot.
>>
>> But U-Boot needs to load it, and the kernel is the authority in
>> providing it.
>>
> 
> To make it a bit clearer:
> 
> Several U-Boot boot methods use the value of environment variable $fdtfile to load a device-tree from file. The same holds true for the boot.scr file created by Debian's flash-kernel package.
> 
> A good solution would be to read the EEPROM to determine the exact board version, to set $fdtfile accordingly and update U-Boots control dtb as needed.
> 

The patcheset have implemented the reading of PCB versions from EEPROM and the 
configuration of gmac device tree nodes according to different PCB versions, 
which can achieve a bin file compatible with both versions 1.2A and 1.3B.

The patcheset link:
https://patchwork.ozlabs.org/project/uboot/cover/20230428022515.29393-1-yanhong.wang@starfivetech.com/

> Best regards
> 
> Heinrich