On Wed, 2019-06-12 at 02:40 +0000, Joel Stanley wrote:
> On Thu, 6 Jun 2019 at 04:50, Samuel Mendoza-Jonas <s...@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 <s...@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
> >
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
