Re: [PATCH net-next] net: phy: micrel: Add support for PTP_PF_EXTTS for lan8814

[Horatiu Vultur]

The 04/24/2024 11:57, Vadim Fedorenko wrote:

Hi Vadim,

> 
> On 23/04/2024 20:57, Horatiu Vultur wrote:
> > Extend the PTP programmable gpios to implement also PTP_PF_EXTTS
> > function. The pins can be configured to capture both of rising
> > and falling edge. Once the event is seen, then an interrupt is
> > generated and the LTC is saved in the registers.
> > On lan8814 only GPIO 3 can be configured for this.
> > 
> > This was tested using:
> > ts2phc -m -l 7 -s generic -f ts2phc.cfg
> > 
> > Where the configuration was the following:
> > ---
> > [global]
> > ts2phc.pin_index  3
> > 
> > [eth0]
> > ---
> > 
> > Signed-off-by: Horatiu Vultur <horatiu.vultur@xxxxxxxxxxxxx>
> 
> I'm not sure what happened to (fac63186f116 net: phy: micrel: Add
> support for PTP_PF_EXTTS for lan8841), looks like this patch is the
> rework previous with the limit to GPIO 3 only. In this case comments
> below are applicable.

These are two different PHYs:
1. lan8814 which is a quad PHY and the patch is this PHY
2. lan8841 which is a single PHY. And the commit that you mention it was
   for that PHY.
So this commit is not rework of the commit that you mention.

..

> 
> > +static int lan8814_ptp_extts(struct ptp_clock_info *ptpci,
> > +                          struct ptp_clock_request *rq, int on)
> > +{
> > +     struct lan8814_shared_priv *shared = container_of(ptpci, struct lan8814_shared_priv,
> > +                                                       ptp_clock_info);
> > +     struct phy_device *phydev = shared->phydev;
> > +     int pin;
> > +
> > +     if (rq->extts.flags & ~(PTP_ENABLE_FEATURE |
> > +                             PTP_EXTTS_EDGES |
> > +                             PTP_STRICT_FLAGS))
> > +             return -EOPNOTSUPP;
> > +
> > +     pin = ptp_find_pin(shared->ptp_clock, PTP_PF_EXTTS,
> > +                        rq->extts.index);
> > +     if (pin == -1 || pin != LAN8814_PTP_EXTTS_NUM)
> > +             return -EINVAL;
> 
> I'm not sure how will enable request pass this check?
> In lan8814_ptp_probe_once pins are initialized with PTP_PF_NONE,
> and ptp_find_pin will always return -1, which will end up with
> -EINVAL here and never hit lan8814_ptp_extts_on/lan8814_ptp_extts_off
> 

Why ptp_find_pin will always return -1? Because we can set the function
of the pin.

..

 >       }
> > @@ -3148,6 +3263,10 @@ static int lan8814_ptpci_verify(struct ptp_clock_info *ptp, unsigned int pin,
> >               if (pin >= LAN8814_PTP_PEROUT_NUM || pin != chan)
> >                       return -1;
> >               break;
> > +     case PTP_PF_EXTTS:
> > +             if (pin != LAN8814_PTP_EXTTS_NUM)
> 
> Here the check states that exactly GPIO 3 can have EXTTS function, but
> later in the config...

..
> 
> > +                     return -1;
> > +             break;
> >       default:
> >               return -1;
> >       }
> > 
> > @@ -3541,7 +3721,7 @@ static int lan8814_ptp_probe_once(struct phy_device *phydev)
> >       snprintf(shared->ptp_clock_info.name, 30, "%s", phydev->drv->name);
> >       shared->ptp_clock_info.max_adj = 31249999;
> >       shared->ptp_clock_info.n_alarm = 0;
> > -     shared->ptp_clock_info.n_ext_ts = 0;
> > +     shared->ptp_clock_info.n_ext_ts = LAN8814_PTP_EXTTS_NUM;
> 
> Here ptp_clock is configured to have 3 pins supporting EXTTS.
> Looks like it should be n_ext_ts = 1;

Good point, let me have a look at this.


> 
> >       shared->ptp_clock_info.n_pins = LAN8814_PTP_GPIO_NUM;
> >       shared->ptp_clock_info.pps = 0;
> >       shared->ptp_clock_info.pin_config = shared->pin_config;
> 
> 

-- 
/Horatiu