Messages in this thread | | | Date | Thu, 25 Apr 2024 01:10:01 +0100 | Subject | Re: [PATCH net-next] net: phy: micrel: Add support for PTP_PF_EXTTS for lan8814 | From | Vadim Fedorenko <> |
| |
On 24/04/2024 20:12, Horatiu Vultur wrote: > 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@microchip.com> >> >> 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.
Ah, I see, sorry for the mess..
> > ... > >> >>> +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.
ah, I see, PTP_PIN_SETFUNC + PTP_EXTTS_REQUEST ioctls will do the configuration. Maybe make GPIO 3 as PTP_PF_EXTTS function by default?
> ... > > > } >>> @@ -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.
I have checked it while checking enable part. Conditions in ptp_ioctl give no options to limit lowest number of pin which supports EXTTS.
I think that the ptp_clock_info documentation is misleading here:
* @n_ext_ts: The number of external time stamp channels.
should be replaced to something like "max index of external time stamp channel".
With all above the patch LGTM!
Reviewed-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>
> >> >>> 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; >> >> >
| |