[PATCH v1 00/23] phy: marvell: Sync Armada 3k/7k/8k SERDES code with Marvell version

[Pali Rohár]

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 <sr at 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