RE: [PATCH 1/2] net: dpaa2: move DPAA2 PTP driver out of staging/

[Y.b. Lu]

Hi Ioana,

> -----Original Message-----
> From: Ioana Ciocoi Radulescu
> Sent: Friday, September 28, 2018 6:21 PM
> To: Y.b. Lu <yangbo.lu@xxxxxxx>; Andrew Lunn <andrew@xxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx; devel@xxxxxxxxxxxxxxxxxxxx;
> netdev@xxxxxxxxxxxxxxx; Richard Cochran <richardcochran@xxxxxxxxx>;
> David S . Miller <davem@xxxxxxxxxxxxx>; Greg Kroah-Hartman
> <gregkh@xxxxxxxxxxxxxxxxxxx>
> Subject: RE: [PATCH 1/2] net: dpaa2: move DPAA2 PTP driver out of staging/
> 
> > -----Original Message-----
> > From: Y.b. Lu
> > Sent: Friday, September 28, 2018 11:04 AM
> > To: Andrew Lunn <andrew@xxxxxxx>
> > Cc: linux-kernel@xxxxxxxxxxxxxxx; devel@xxxxxxxxxxxxxxxxxxxx;
> > netdev@xxxxxxxxxxxxxxx; Richard Cochran <richardcochran@xxxxxxxxx>;
> > David S . Miller <davem@xxxxxxxxxxxxx>; Ioana Ciocoi Radulescu
> > <ruxandra.radulescu@xxxxxxx>; Greg Kroah-Hartman
> > <gregkh@xxxxxxxxxxxxxxxxxxx>
> > Subject: RE: [PATCH 1/2] net: dpaa2: move DPAA2 PTP driver out of
> > staging/
> >
[...]
> > >
> > > It seems like there is a lot of code in dprtc.c which is unused.
> > > rtc.c does
> > nothing
> > > with interrupts for example. Do you plan to make use of this extra
> > > code? Or can it be removed leaving just what is needed?
> >
> > [Y.b. Lu] Currently the ptp/rtc driver is not full-featured. The extra
> > code is being planed to be used.
> 
> Are there any interrupts associated to the real time clock module that will
> actually be used by the driver? Also, I don't think the create/destroy functions
> are meant to be used by the PTP kernel driver, even though MC exposes the
> APIs for them.
> 
> Generally speaking, I think it's better to remove unused code from the current
> driver and re-add it along with the feature actually using it.

[Y.b. Lu] Yes. We need to implement these interrupts to support ptp_clock_event() of common ptp_clock driver.
This is mainly to support 1588 timer external signals.
I get your point, and will remove unused code before using them.

> 
> >
> > >
> > > struct dprtc_cmd_get_irq - Putting pad at the beginning of a struct
> > > seems
> > very
> > > odd. And it is not the only example.
> >
> > [Y.b. Lu] This should depended on MC firmware and APIs I think. Once
> > the MC improves this, the APIs could be updated to fix this.
> 
> These structures map the command format expected by the MC firmware. I
> agree that some of the command layouts are less than inspired, but I'm not
> sure we can expect MC to "improve" them without a good reason, as this
> would break backward compatibility.
> 
> I also want to bring up the question of where the dpaa2 ptp driver should be
> located. The qoriq_ptp driver (which targets previous gen Freescale/NXP
> architectures) is located in drivers/ptp. I'm not sure if the dpaa2 ptp driver
> should be moved there as well or it's better suited for the currently proposed
> location.

[Y.b. Lu] Actually the ptp timer is to provide hw timestamping support for ethernet.
Ptp clock driver together with ethernet hw timestamping driver provide the method to support 1588 software application.
It's ok to put ptp clock driver near to ethernet driver. And most ptp clock drivers in kernel are together with ethernet driver.
You may see there are gianfar_ptp and dpaa_ptp before. Considering they could reuse the code, I created the ptp_qoriq for them to use the one driver.

> 
> Thanks,
> Ioana