Re: [PATCH u-boot-marvell 8/8] arm: mvebu: turris_omnia: Add support for USB3.0 mode in WWAN MiniPCIe slot

["Marek Behún" <marek.behun@nic.cz>]

On Fri, 22 Apr 2022 13:47:33 +0200
Pali Rohár <pali@kernel.org> wrote:

> On Wednesday 02 March 2022 13:46:07 Marek Behún wrote:
> > On Wed,  2 Mar 2022 12:47:58 +0100
> > Pali Rohár <pali@kernel.org> wrote:
> >   
> > > PCIe Mini CEM 2.1 spec added support for USB3.0 mode on MiniPCIe cards.
> > > USB3.0 and PCIe share same pins and only one function can be active at the
> > > same time. PCIe Mini CEM 2.1 spec says that determining function is
> > > platform specific and spec does not define any dedicated pin which could
> > > say if card is USB3.0-based or PCIe-based.
> > > 
> > > Implement this platform specific decision (USB3.0 vs PCIe) for WWAN
> > > MiniPCIe slot on Turris Omnia via U-Boot env variable "omnia_wwan_slot",
> > > similarly like is implemented forced mode for MiniPCIe/mSATA slot via
> > > "omnia_msata_slot" env variable. Value "usb3" for "omnia_wwan_slot" would
> > > mean to set USB3.0 mode and value "pcie" original PCIe mode.
> > > 
> > > A385 SoC on Turris Omnia has configurable fifth SerDes line (exported to
> > > MiniPCIe WWAN slot with SIM card) either to USB3.0 or PCIe functionality,
> > > so implementation of this new PCIe Mini CEM 2.1 feature is simple, by just
> > > configuring SerDes to USB 3.0 mode.
> > > 
> > > Other twos MiniPCIe slots on Turris Omnia do not have this new
> > > functionality as their SerDes lines cannot be switched to USB3.0
> > > functionality.
> > > 
> > > Note that A385 SoC does not have too many USB3.0 blocks, so activating
> > > USB3.0 in MiniPCIe cause that one external USB3.0 USB-A port would loose
> > > USB3.0 functionality and would be downgraded just to USB2.0.
> > > 
> > > By default this MiniPCIe WWAN slot is in PCIe mode, like before.
> > > 
> > > To set this MiniPCIe WWAN slot to USB3.0 mode, call U-Boot commands:
> > >   
> > >   => setenv omnia_wwan_slot usb3
> > >   => saveenv
> > >   => reset    
> > > 
> > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > ---
> > >  board/CZ.NIC/turris_omnia/turris_omnia.c | 57 ++++++++++++++++++++++++
> > >  1 file changed, 57 insertions(+)
> > > 
> > > diff --git a/board/CZ.NIC/turris_omnia/turris_omnia.c b/board/CZ.NIC/turris_omnia/turris_omnia.c
> > > index e2f4daa827ed..83cfc80d1930 100644
> > > --- a/board/CZ.NIC/turris_omnia/turris_omnia.c
> > > +++ b/board/CZ.NIC/turris_omnia/turris_omnia.c
> > > @@ -246,6 +246,22 @@ static bool omnia_detect_sata(const char *msata_slot)
> > >  	return stsword & MSATA_IND_STSBIT ? true : false;
> > >  }
> > >  
> > > +static bool omnia_detect_wwan_usb3(const char *wwan_slot)
> > > +{
> > > +	puts("WWAN slot configuration... ");
> > > +
> > > +	if (wwan_slot && strcmp(wwan_slot, "usb3") == 0) {
> > > +		puts("USB3.0\n");
> > > +		return true;
> > > +	}
> > > +
> > > +	if (wwan_slot && strcmp(wwan_slot, "pcie") != 0)
> > > +		printf("unsupported env value '%s', fallback to... ", wwan_slot);  
> > 
> > If  I recall correctly, Linux' and U-Boot's code style (in contrast to
> > TF-A) normally wants
> >   if (expr)        instead of       if (expr != 0)
> > and
> >   if (!expr)       instead of       if (expr == 0)  
> 
> I guess that this applies for functions which return boolean value. And
> not for strcmp() function which is not failure expression. But instead
> it returns comparison value.

Again, this was an unimportant nitpick from me, feel free to ignore
this. But since you replied, I shall entertain you with an opinion I
have to share.

I am sure that I remember people from TF-A requesting to use
(x == 0) or (x != 0), while people from Linux requesting (x) or (!x),
not only for functions which return boolean value. I am not sure now
for what kind of function it may have been exactly, but I think it was
someting like
  err = func();
  if (err == 0)
    ..
and the request was to change it to
  if (!err)

I guess that functions which return 0 on success and negative error
code on failure may be considered returning "boolean" in a sense of
success/failure boolean.

But anyway if seems that in the usage of strcmp(), Linux uses both
variants, although the one with the comparison operator is used a
little bit less:
  $ git grep 'strcmp' | wc -l
  6971
  $ git grep 'strcmp.* [=!]= 0' | wc -l
  2100

Mostly the strcmp() function is used to determine whether two strings
are equal or not, which is a binary/boolean decision, and so I prefer to
check the result as such. But strcmp() can be also used to compare
strings for sorting, and that is the case where ==, < and > operators
would make sense to me.

Again, feel free to ignore, since this is a matter of personal
preference.

Marek