Stm32H7 PTP timestamp retrieval issue in HAL

[chris1seto]

Hi,

I started a github issue for this problem: https://github.com/STMicroelectronics/stm32h7xx_hal_driver/issues/63

Realistically, the entire HAL_ETH_ReadData() function just needs to be rewritten. There are multiple flaws in it:

Take for example here: https://github.com/STMicroelectronics/stm32h7xx_hal_driver/blob/21b29aa338c68333bb5e489b03a04553f8fc2c12/Src/stm32h7xx_hal_eth.c#L1074

What's going on there is the driver examines to check if a descriptor is the context format, and then processes the timestamp data, but then it immediately moves on and treats the descriptor as if it was a normal descriptor. 

It's really obvious that the PTP support was not tested (or else ST would have immediately noticed this issue)

Also note that the function does not consider the timestamp dropped or timestamp available bits. This also needs to be fixed.ST, can you please rework this function to correctly handle retrieving the timestamp data from a context descriptor in such a way where one call to this function correctly processes normal descriptors as well as their associated context descriptor?

[chris1seto]

I wrote a patch to the normal hal ethernet driver which will read a timestamp if available. It's at the end of this post. I'm worried though, that I'm not handling something here quite right. Because of the difficulty in getting correct information about the Ethernet MAC, I'm not really sure what I'm not doing right. This does work and even in high traffic cases, does not have issues allocating or RBUS. If timestamping is not enabled, I can run the lwip iperf server and get about 80Mb/s When I enable timestamping, with this patch, it throws RBUs and it seems like there is some kind of issue with the handling of the buffers. I know that's a vague description of why I suspect there is an issue, but I haven't yet been able to figure out why there is an issue there just yet. 

ST, are there any issues with this patch? I don't *think* that the context buffer needs to be rebuilt with the pointer to the allocated buffer. because while the descriptor is read back by DMA with the context information, internally it still has the buffer address it was previously given when a buffer was allocated for it. Is this correct?

The full file is attached, here is the main part of the patch

HAL_StatusTypeDef HAL_ETH_ReadData(ETH_HandleTypeDef *heth, void **pAppBuff)
{
  uint32_t descidx;
  ETH_DMADescTypeDef *dmarxdesc;
  uint32_t desccnt = 0U;
  uint32_t desccntmax;
  uint32_t bufflength;
  uint8_t rxdataready = 0U;

  if (pAppBuff == NULL)
  {
    heth->ErrorCode |= HAL_ETH_ERROR_PARAM;
    return HAL_ERROR;
  }

  if (heth->gState != HAL_ETH_STATE_STARTED)
  {
    return HAL_ERROR;
  }

  descidx = heth->RxDescList.RxDescIdx;
  dmarxdesc = (ETH_DMADescTypeDef *)heth->RxDescList.RxDesc[descidx];
  desccntmax = ETH_RX_DESC_CNT - heth->RxDescList.RxBuildDescCnt;

  /* Check if descriptor is not owned by DMA */
  while (!READ_BIT(dmarxdesc->DESC3, ETH_DMARXNDESCWBF_OWN)
    && desccnt < desccntmax
    && !rxdataready)
  {
    if (READ_BIT(dmarxdesc->DESC3,  ETH_DMARXNDESCWBF_CTXT)  != (uint32_t)RESET)
    {
      /* Get timestamp high */
      heth->RxDescList.TimeStamp.TimeStampHigh = dmarxdesc->DESC1;
      /* Get timestamp low */
      heth->RxDescList.TimeStamp.TimeStampLow  = dmarxdesc->DESC0;
      rxdataready = 1;
    }
    else
    {
      if ((READ_BIT(dmarxdesc->DESC3, ETH_DMARXNDESCWBF_FD) != (uint32_t)RESET) || (heth->RxDescList.pRxStart != NULL))
      {
        /* Check if first descriptor */
        if (READ_BIT(dmarxdesc->DESC3, ETH_DMARXNDESCWBF_FD) != (uint32_t)RESET)
        {
          heth->RxDescList.RxDataLength = 0;
        }

        /* Check if last descriptor */
        bufflength = heth->Init.RxBuffLen;
        if (READ_BIT(dmarxdesc->DESC3, ETH_DMARXNDESCWBF_LD) != (uint32_t)RESET)
        {
          bufflength = READ_BIT(dmarxdesc->DESC3, ETH_DMARXNDESCWBF_PL) - heth->RxDescList.RxDataLength;

          /* Save Last descriptor index */
          heth->RxDescList.pRxLastRxDesc = dmarxdesc->DESC3;

          /* Packet ready */
          if (!READ_BIT(dmarxdesc->DESC1, (1 << 14)))
          {
            rxdataready = 1;
          }
        }
      }

      /* Link callback */
      HAL_ETH_RxLinkCallback(&heth->RxDescList.pRxStart,
        &heth->RxDescList.pRxEnd,
        (uint8_t *)dmarxdesc->BackupAddr0,
        (uint16_t) bufflength);

      heth->RxDescList.RxDataLength += bufflength;

      /* Clear buffer pointer */
      dmarxdesc->BackupAddr0 = 0;
    }

    /* Increment current rx descriptor index */
    INCR_RX_DESC_INDEX(descidx, 1U);

    /* Get current descriptor address */
    dmarxdesc = (ETH_DMADescTypeDef *)heth->RxDescList.RxDesc[descidx];
    desccnt++;
  }

  heth->RxDescList.RxBuildDescCnt += desccnt;

  if ((heth->RxDescList.RxBuildDescCnt) != 0U)
  {
    /* Update Descriptors */
    ETH_UpdateDescriptor(heth);
  }

  heth->RxDescList.RxDescIdx = descidx;

  if (rxdataready == 1U)
  {
    /* Return received packet */
    *pAppBuff = heth->RxDescList.pRxStart;
    /* Reset first element */
    heth->RxDescList.pRxStart = NULL;

    return HAL_OK;
  }

  /* Packet not ready */
  return HAL_ERROR;
}

[Pavel A.]

Maybe a complete project with a fix for PTP can be added here https://github.com/stm32-hotspot/STM32H7-LwIP-Examples  for feedback