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

On Thu, 6 Jun 2019 at 04:50, Samuel Mendoza-Jonas sam@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.

Signed-off-by: Samuel Mendoza-Jonas sam@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