On Wednesday 05 May 2021 09:00:57 Stefan Roese wrote: > Hi Pali, > > On 05.05.21 00:28, Pali Rohár wrote: > > On Thursday 29 April 2021 11:00:17 Stefan Roese wrote: > > > Hi Marek, > > > > > > (Added Tom and Simon to Cc) > > > > > > On 29.04.21 10:27, Marek Behun wrote: > > > > On Thu, 29 Apr 2021 08:46:36 +0200 > > > > Stefan Roese <s...@denx.de> wrote: > > > > > > > > > > phy: marvell: add RX training command > > > > > > > > > > Applied to u-boot-marvell/master > > > > > > > > > > Thanks, > > > > > Stefan > > > > > > > > Stefan, do you think it would make sense to at least create a special > > > > mechanism for these platform commands? For example via a top-level > > > > command "plat", i.e. instead of > > > > > rx_training params > > > > we would call > > > > > plat rx_training params > > > > > > > > The plat command could list all registered platform specific commands... > > > > > > Not sure. If you want to split it up, then we would perhaps also > > > need to add a mechanism for board specific commands as well. E.g. for > > > commands not common for a platform but only for specific boards. My > > > feeling is that this makes things overly complex. And I also don't see > > > a real problem with the current "flat" structure of these commands > > > being "global". Again, I mention the many already existing board and > > > platform specific commands in current mainline. > > > > > > Perhaps other people can comment on this use / introduction of > > > platform specific U-Boot commands? > > > > > > BTW: Again, we can definitely rename this specific "rx_training" > > > command, if you feel this is absolutely misleading. IIRC, then I already > > > made a suggestion for this. > > > > Hello! "rx_training" is definitely ambiguous and I strongly suggest to > > rename this command (if is going to be merged in current form). > > It's already in mainline. I'll probably send a patch to rename it, > please see below... > > > My first impression was that this command is suppose to do some DDR3/4 > > training sequence but it is doing something totally different. > > > > I'm also not a big fan of these custom vendor specific/proprietary > > commands. And I rather would like to see generic command with an API so > > other boards and vendors could implement it too. > > > > But if this comphy rx training code is something specific to Marvell > > platforms and there is no other platform which needs such abstraction > > then lets have it as custom vendor specific command. > > > > I hope that Tom or Simon have better knowledge of U-Boot code and > > hardware on which is U-Boot running and can say if there are other > > platforms for such command or not. > > > > And if "plat" command is too complex for this, what about renaming this > > command to something like "mvebu_comphy_rx_training" or something > > similar? To express that it is Marvell specific and also mention that it > > is for comphy. And not for ddr, uart or ethernet phy. > > No other platform than "MVEBU" can select this command. So adding > "mvebu" seems superflous to me. But it would make it clear that it's > platform specific never the less. I agree that "comphy" makes > perfect sense to avoid confusion (mixup with DDR3/4) here.
I mean if there is another platform for which such command could be implemented in future (not what is currently implemented/supported) or if comphy rx training "feature" is a8k specific. > > Same applies for Marvell command "bubt" which is already present in > > U-Boot codebase. > > "bubt" is special and cannot be changed easily without breaking update > scripts using it AFAICT. As it's pretty old and used in the Marvell code > base for quite some time - including all the documentation about > updating. I see. This needs to say in current form. > > It has totally insane name as abbreviation of Burn > > UBooT and moreover on A3720 is does not even work with U-Boot image but > > rather with "big" image in specific custom format which contains > > concatenation of Cortex-M3 Secure Firmware, Cortex-A53 Trusted Firmware > > and U-Boot. And I think this "bubt" is example of command which should > > not be vendor specific but rather generic U-Boot command as its purpose > > is to update vendor specific boot image stored in nand/eMMC either via > > TFTP or from uSD card. So this command could have been called "fwupdate" > > or similar to express that is updated vendor specific firmware or U-Boot > > bootloader in vendor specific format (if U-Boot needs to have some > > encapsulation) for current board to current "boot device/partition". > > Yes, it would be nice to have a common / generic command for such an > update process with multiple options (storage device etc). > > Thanks, > Stefan
