[U-Boot] [PATCH 1/4] phy: Add support for the NC-SI protocol

Samuel Mendoza-Jonas sam at mendozajonas.com
Wed Jun 12 05:42:22 UTC 2019


On Wed, 2019-06-12 at 02:40 +0000, Joel Stanley wrote:
> On Thu, 6 Jun 2019 at 04:50, Samuel Mendoza-Jonas <sam at mendozajonas.com> wrote:
> > This introduces support for the NC-SI protocol, modelled as a phy driver
> > for other ethernet drivers to consume.
> > 
> > NC-SI (Network Controller Sideband Interface) is a protocol to manage a
> > sideband connection to a proper network interface, for example a BMC
> > (Baseboard Management Controller) sharing the NIC of the host system.
> > Probing and configuration occurs by communicating with the "remote" NIC
> > via NC-SI control frames (Ethernet header 0x88f8).
> > 
> > This implementation is roughly based on the upstream Linux
> > implementation[0], with a reduced feature set and an emphasis on getting
> > a link up as fast as possible rather than probing the full possible
> > topology of the bus.
> > The current phy model relies on the network being "up", sending NC-SI
> > command frames via net_send_packet() and receiving them from the
> > net_loop() loop (added in a following patch).
> > 
> > The ncsi-pkt.h header[1] is copied from the Linux kernel for consistent
> > field definitions.
> > 
> > [0]: https://github.com/torvalds/linux/tree/master/net/ncsi
> > [1]: https://github.com/torvalds/linux/blob/master/net/ncsi/ncsi-pkt.h
> > 
> > Signed-off-by: Samuel Mendoza-Jonas <sam at mendozajonas.com>
> 
> Looks good. Some comments below.
> 
> > +static int ncsi_validate_rsp(struct ncsi_rsp_pkt *pkt, int payload)
> > +{
> > +       struct ncsi_rsp_pkt_hdr *hdr = &pkt->rsp;
> > +       __be32 pchecksum;
> > +       u32 checksum;
> > +       if (ntohs(hdr->common.length) != payload) {
> > +               printf("NCSI: 0x%02x response has incorrect length %d\n",
> > +                      hdr->common.type, hdr->common.length);
> > +               return -1;
> > +       }
> > +
> > +       pchecksum = get_unaligned_be32((void *)(hdr + 1) + payload - 4);
> 
> Wheee. So the checksum is the last 4-bytes of the payload. I assume
> it's always longer than 4?
> 
> A clarifying comment might help, or try to write it in a different way:

"Wheee" indeed. I kept this roughly as it is in the kernel so I could
more easily verify it was doing the right thing; now that it's more
tested I'll do something like below so it's less traumatic.

> 
> endp = (void *)hdr + sizeof(hdr) + payload;
> pchecksum = get_unaligned_be32(endp - sizeof(checksum));
> 
> or
> 
>     checksum_offset = sizeof(hdr) + payload - sizeof(checksum);
>     pchecksum = get_unaligned_be32(payload + checksum_offset);
> 
> > +       if (pchecksum != 0) {
> > +               checksum = ncsi_calculate_checksum((unsigned char *)hdr,
> > +                                                  sizeof(*hdr) + payload - 4);
> 
> And then this can be:
> 
>     checksum = ((unsigned char *)hdr, checksum_offset);
> 
> > +               if (pchecksum != checksum) {
> > +                       printf("NCSI: 0x%02x response has invalid checksum\n",
> > +                              hdr->common.type);
> > +                       return -1;
> > +               }
> > +       }
> > +static void ncsi_send_sma(unsigned int np, unsigned int nc)
> > +{
> > +       struct ncsi_cmd_sma_pkt cmd;
> > +       unsigned char *addr;
> > +
> > +       addr = eth_get_ethaddr();
> > +       if (!addr) {
> > +               printf("NCSI: no MAC address configured\n");
> > +               return;
> > +       }
> > +
> > +       memset(&cmd, 0, sizeof(cmd));
> > +       memcpy(cmd.mac, addr, 6);
> 
> Are there endianness issues with addr here?

Aha, will fixup.

> 
> > +       cmd.index = 1;
> > +       cmd.at_e = 1;
> > +
> > +       ncsi_send_command(np, nc, NCSI_PKT_CMD_SMA,
> > +                         ((unsigned char *)&cmd)
> > +                         + sizeof(struct ncsi_cmd_pkt_hdr),
> > +                         cmd_payload(NCSI_PKT_CMD_SMA), true);
> > +}
> > +
> > +int ncsi_probe(struct phy_device *phydev)
> > +{
> > +       // TODO Associate per device
> 
> Is this required before we can support multiple NICs?

Yes, I'll chase this up.

> 
> > +       if (!ncsi_priv) {
> > +               ncsi_priv = malloc(sizeof(struct ncsi));
> > +               if (!ncsi_priv)
> > +                       return -ENOMEM;
> > +               memset(ncsi_priv, 0, sizeof(struct ncsi));
> > +       }
> > +
> > +       phydev->priv = ncsi_priv;
> > +
> > +       return 0;
> > +}
> > +
> > +int ncsi_startup(struct phy_device *phydev)
> > +{
> > +       /* Set phydev parameters */
> > +       phydev->speed = SPEED_100;
> > +       phydev->duplex = DUPLEX_FULL;
> > +       /* Normal phy reset is N/A */
> > +       phydev->flags |= PHY_FLAG_BROKEN_RESET;
> > +
> > +       /* Set initial probe state */
> > +       ncsi_priv->state = NCSI_PROBE_PACKAGE_SP;
> > +
> > +       /* No active package/channel yet */
> > +       ncsi_priv->current_package = NCSI_PACKAGE_MAX;
> > +       ncsi_priv->current_channel = NCSI_CHANNEL_MAX;
> > +
> > +       /* Pretend link works so ftgmac100 sets final bits up */
> 
> s/ftgmac100/mac driver/ ?

Ack

> 
> > +       phydev->link = true;
> > +
> > +       return 0;
> > +}
> > +
> > +int ncsi_shutdown(struct phy_device *phydev)
> > +{
> > +       printf("NCSI: Disabling package %d\n", ncsi_priv->current_package);
> > +       ncsi_send_dp(ncsi_priv->current_package);
> > +       return 0;
> > +}
> > +
> > +static struct phy_driver ncsi_driver = {
> > +       .uid            = PHY_NCSI_ID,
> > +       .mask           = 0xffffffff,
> > +       .name           = "NC-SI",
> > +       .features       = PHY_100BT_FEATURES | PHY_DEFAULT_FEATURES | SUPPORTED_100baseT_Full | SUPPORTED_MII,
> > +       .probe          = ncsi_probe,
> > +       .startup        = ncsi_startup,
> > +       .shutdown       = ncsi_shutdown,
> > +};
> > +
> > +int phy_ncsi_init(void)
> > +{
> > +       phy_register(&ncsi_driver);
> > +       return 0;
> > +}
> > --- /dev/null
> > +++ b/include/net/ncsi-pkt.h
> > @@ -0,0 +1,415 @@
> > +/*
> > + * Copyright Gavin Shan, IBM Corporation 2016.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + */
> 
> If you grab the version from 5.2-rc3 it has been SPDXified.

Cheers, will do.

> 
> > --- a/include/phy.h
> > +++ b/include/phy.h
> > @@ -17,6 +17,7 @@
> >  #include <phy_interface.h>
> > 
> >  #define PHY_FIXED_ID           0xa5a55a5a
> > +#define PHY_NCSI_ID            0xbeefcafe
> 
> hmmm...

Haha - suggestions welcome?

> 
> >  #define PHY_MAX_ADDR 32
> > 
> > --
> > 2.21.0
> > 




More information about the U-Boot mailing list