Message ID | 20230329034224.26545-1-yanhong.wang@starfivetech.com |
---|---|
Headers | show |
Series | Basic StarFive JH7110 RISC-V SoC support | expand |
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
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
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
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
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
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
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
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
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
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.
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
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.
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
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