Re: [PATCH v2 0/5] Introduce IEP driver and packet timestamping support

[Md Danish Anwar <a0501179@ti.com>]

Hi Conor,

On 09/08/23 10:31 am, Md Danish Anwar wrote:
> On 08/08/23 6:15 pm, Conor Dooley wrote:
>> On Tue, Aug 08, 2023 at 06:06:11PM +0530, Md Danish Anwar wrote:
>>> On 08/08/23 5:52 pm, Roger Quadros wrote:
>>>>
>>>>
>>>> On 08/08/2023 15:18, Md Danish Anwar wrote:
>>>>> On 08/08/23 5:38 pm, Conor Dooley wrote:
>>>>>> On Mon, Aug 07, 2023 at 04:30:43PM +0530, MD Danish Anwar wrote:
>>>>>>> This series introduces Industrial Ethernet Peripheral (IEP) driver to
>>>>>>> support timestamping of ethernet packets and thus support PTP and PPS
>>>>>>> for PRU ICSSG ethernet ports.
>>>>>>>
>>>>>>> This series also adds 10M full duplex support for ICSSG ethernet driver.
>>>>>>>
>>>>>>> There are two IEP instances. IEP0 is used for packet timestamping while IEP1
>>>>>>> is used for 10M full duplex support.
>>>>>>>
>>>>>>> This is v2 of the series [v1]. It addresses comments made on [v1].
>>>>>>> This series is based on linux-next(#next-20230807). 
>>>>>>>
>>>>>>> Changes from v1 to v2:
>>>>>>> *) Addressed Simon's comment to fix reverse xmas tree declaration. Some APIs
>>>>>>>    in patch 3 and 4 were not following reverse xmas tree variable declaration.
>>>>>>>    Fixed it in this version.
>>>>>>> *) Addressed Conor's comments and removed unsupported SoCs from compatible
>>>>>>>    comment in patch 1. 
>>>>>>
>>>>>> I'm sorry I missed responding there before you sent v2, it was a bank
>>>>>> holiday yesterday. I'm curious why you removed them, rather than just
>>>>>> added them with a fallback to the ti,am654-icss-iep compatible, given
>>>>>> your comment that "the same compatible currently works for all these
>>>>>> 3 SoCs".
>>>>>
>>>>> I removed them as currently the driver is being upstreamed only for AM654x,
>>>>> once I start up-streaming the ICSSG driver for AM64 and any other SoC. I will
>>>>> add them here. If at that time we are still using same compatible, then I will
>>>>> modify the comment otherwise add new compatible.
>>>>>
>>>>> As of now, I don't see the need of adding other SoCs in iep binding as IEP
>>>>> driver up-streaming is only planned for AM654x as of now.
>>>>
>>>> But, is there any difference in IEP hardware/driver for the other SoCs?
>>>> AFAIK the same IP is used on all SoCs.
>>>>
>>>> If there is no hardware/code change then we don't need to introduce a new compatible.
>>>> The comment for all SoCs can already be there right from the start.
>>>>
>>>
>>> There is no code change. The same compatible is used for other SoCs. Even if
>>> the code is same I was thinking to keep the compatible as below now
>>>
>>> - ti,am654-icss-iep   # for K3 AM65x SoCs
>>>
>>> and once other SoCs are introduced, I will just modify the comment,
>>>
>>> - ti,am654-icss-iep   # for K3 AM65x, AM64x SoCs
>>>
>>> But we can also keep the all SoCs in comment right from start as well. I am
>>> fine with both.
>>
>>> Conor / Roger, Please let me know which approach should I go with in next revision?
>>
>> IMO, "ti,am564-icss-iep" goes in the driver and the other SoCs get
>> specific compatibles in the binding with "ti,am564-icss-iep" as a
>> fallback.
> 
> Sure. Then as for now, "ti,am654-icss-iep" goes in the driver, I will keep the
> dt binding compatible as below (as it was earlier in v1.)
> 
> - ti,am654-icss-iep   # for K3 AM65x, J721E and AM64x SoCs
> 
> When new SoCs are introduced I can add specific bindings for them with
> "ti,am654-icss-iep" being the fallback.
> 

I checked internally and IEP hardware / driver is same across all TI K3 SoCs.
Compatible "ti,am654-icss-iep" will be same for all SoCs. I don't think we need
to introduce different compatibles for different SoCs in future as they will be
using same hardware / driver. For now I will have below as compatible in dt
bindings. This will not change in future. When new SoCs are added, they can
just use this compatible itself. The driver will always use "ti,am654-icss-iep"
as compatible.

	- ti,am654-icss-iep   # for all TI K3 SoCs

-- 
Thanks and Regards,
Danish.