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

[Joel Stanley]

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:

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?

> +       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?

> +       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/ ?

> +       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.

> --- 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...

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