lkml.org 
[lkml]   [2024]   [Apr]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH net-next] net: phy: micrel: Add support for PTP_PF_EXTTS for lan8814
From
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;
>>
>>
>


\
 
 \ /
  Last update: 2024-05-27 18:02    [W:0.148 / U:0.052 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site