This thread has been locked.

If you have a related question, please click the "Ask a related question" button in the top right corner. The newly created question will be automatically linked to this question.

TMS570LC4357: race condition in code generated by HalCoGen for the DP83640 chip

Part Number: TMS570LC4357
Other Parts Discussed in Thread: HALCOGEN, DP83640

Hi

file source/HL_phy_dp83640.c generated by HalCoGen (all versions include latest 4.6.0) contain this code

uint32 Dp83640IDGet(uint32 mdioBaseAddr, uint32 phyAddr)
{
    uint32 id = 0U;
    uint16 data = 0U;

    /* read the ID1 register */
    (void)MDIOPhyRegRead(mdioBaseAddr, phyAddr, (uint32)PHY_ID1, &data);

    /* update the ID1 value */
	  id = (uint32)data;
	  id = (uint32)((uint32)id << PHY_ID_SHIFT);
 
    /* read the ID2 register */
    (void)MDIOPhyRegRead(mdioBaseAddr, phyAddr, (uint32)PHY_ID2, &data);

    /* update the ID2 value */
    id |= data; 

    /* return the ID in ID1:ID2 format */
    return id;
}

This code is sensitive to race condition with chip reset. First MDIOPhyRegRead could fail when chip reset isn't finished. Chip reset can be finished immediately after. In this conditions second MDIOPhyRegRead didn't fail. And function returns invalid PHY_ID.

  • Hi Jiri,

    Are you referring to one of these two timing specs being violated?  OR something else?

  • Yes I mean any of this two reset times. It isn't significant which. Problem is when timing of this end of reset fits perfectly between two reading of ID value.

    In this case upped 16 bit of ID will be 0x0000 and lower bits correcly 0x5CE1

    Final invalid 32 bit result is 0x00005CE1. Expected value is 0x20005CE1

    Better code is:

    uint32 Dp83640IDGet(uint32 mdioBaseAddr, uint32 phyAddr)
    {
        uint32 id = 0U;
        uint16 data = 0U;
    
        /* read the ID1 register */
        boolean status = MDIOPhyRegRead(mdioBaseAddr, phyAddr, (uint32)PHY_ID1, &data);
    
        if (status == TRUE)
        {
          /* update the ID1 value */
          id = (uint32)data;
          id = (uint32)((uint32)id << PHY_ID_SHIFT);
    
          /* read the ID2 register */
          status = MDIOPhyRegRead(mdioBaseAddr, phyAddr, (uint32)PHY_ID2, &data);
          if (status == TRUE)
          {
            /* update the ID2 value */
            id |= data;
          }
          else
          {
            id = 0U; // as error value
          }
        } else {}
    
        /* return the ID in ID1:ID2 format */
        return id;
    }
    

  • Hi Jiri,

    I don't see a good way to handle this actually, at least inside any of the PHY API calls.

    We went through some debug with another customer and when one of these phy timings wasn't being met the PHY would never respond. (not just respond w. the wrong ID). I think that issue occurred on power up reset which has a longer time. The phy team told us something to the effect that the time is needed to reset the management interface & registers on the phy and if that time is violated the phy may not initialized.

    So the problem is a bit more serious than I think should be fixed in a condition in the Dp83640IDGet() because this shouldn't be called until the timings in the phy datasheet are met.

    What's really needed is a delay time from releasing reset to starting the PHY initialization and this doesn't lend itself too well to the simple generated APIs that come w. HalCoGen. It would be something that should show up in an application example I think, like part of the initialization of the TCP/IP stack. And it is really specific to the phy.

    These type of timing race conditions especially from a power up show up quite often between chips on the same board.
    And so I would just categorize this as one of the many conditions HALCoGen doesn't account for and become part of the application code. I think the lwIP example just 'got lucky' because there was enough init stuff that the delay is just met but it's met 'accidentally' rather than explicitly.