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

[Ioana Ciocoi Radulescu]

> -----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/
> 
> Hi Andrew,
> 
> Thanks a lot for your comments.
> Please see my comments inline.
> 
> Best regards,
> Yangbo Lu
> 
> > -----Original Message-----
> > From: Andrew Lunn <andrew@xxxxxxx>
> > Sent: Thursday, September 27, 2018 9:25 PM
> > To: Y.b. Lu <yangbo.lu@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/
> >
> > On Thu, Sep 27, 2018 at 07:12:27PM +0800, Yangbo Lu wrote:
> > > This patch is to move DPAA2 PTP driver out of staging/ since the
> > > dpaa2-eth had been moved out.
> > >
> > > Signed-off-by: Yangbo Lu <yangbo.lu@xxxxxxx>
> > > ---
> > >  drivers/net/ethernet/freescale/Kconfig             |    9 +--------
> > >  drivers/net/ethernet/freescale/dpaa2/Kconfig       |   15
> > +++++++++++++++
> > >  drivers/net/ethernet/freescale/dpaa2/Makefile      |    6 ++++--
> > >  .../ethernet/freescale/dpaa2}/dprtc-cmd.h          |    0
> > >  .../rtc => net/ethernet/freescale/dpaa2}/dprtc.c   |    0
> > >  .../rtc => net/ethernet/freescale/dpaa2}/dprtc.h   |    0
> > >  .../rtc => net/ethernet/freescale/dpaa2}/rtc.c     |    0
> > >  .../rtc => net/ethernet/freescale/dpaa2}/rtc.h     |    0
> > >  drivers/staging/fsl-dpaa2/Kconfig                  |    8 --------
> > >  drivers/staging/fsl-dpaa2/Makefile                 |    1 -
> > >  drivers/staging/fsl-dpaa2/rtc/Makefile             |    7 -------
> > >  11 files changed, 20 insertions(+), 26 deletions(-)  create mode
> > > 100644 drivers/net/ethernet/freescale/dpaa2/Kconfig
> > >  rename drivers/{staging/fsl-dpaa2/rtc =>
> > > net/ethernet/freescale/dpaa2}/dprtc-cmd.h (100%)  rename
> > > drivers/{staging/fsl-dpaa2/rtc =>
> > > net/ethernet/freescale/dpaa2}/dprtc.c (100%)  rename
> > > drivers/{staging/fsl-dpaa2/rtc =>
> > > net/ethernet/freescale/dpaa2}/dprtc.h (100%)  rename
> > > drivers/{staging/fsl-dpaa2/rtc => net/ethernet/freescale/dpaa2}/rtc.c
> > > (100%)  rename drivers/{staging/fsl-dpaa2/rtc =>
> > > net/ethernet/freescale/dpaa2}/rtc.h (100%)
> >
> > Hi Yangbo
> >
> > Calling a ptp driver rtc.[ch] seems rather odd. Could you fixup the name,
> > change it to ptp.[ch]. Also, some of the function names, and structures,
> > rtc_probe->ptp_probe, rtc_remove->ptp_remove, rtc_match_id_table->
> > ptp_match_id_table, etc.
> 
> [Y.b. Lu] Indeed, it's odd and confusing...
> For dpaa2, all hardware resources are allocated and configured through the
> Management Complex (MC) portals.
> MC abstracts most of these resources as DPAA2 objects and exposes ABIs
> through which they can be configured and controlled.
> This ptp timer was named as rtc in MC firmware and APIs as you saw in
> dprtc.*.
> So initially I wrote this driver using rtc as name.
> 
> No worries, let me change it in next version.
> 
> >
> > ptp_dpaa2_adjfreq() probably should return err, not 0.
> > ptp_dpaa2_gettime() again does not return the error.
> > If fact, it seems like all the main functions ignore errors.
> 
> [Y.b. Lu] Will fix the returns in next version.
> 
> >
> > kzalloc() could be changed to devm_kzalloc() to simplify the cleanup
> 
> [Y.b. Lu] Will use devm_kzalloc() in next version.
> 
>  Can
> > ptp_dpaa2_caps be made const?
> 
> [Y.b. Lu] Yes. Will change it in next version.
> 
> > dpaa2_phc_index does not appear to be used.
> 
> [Y.b. Lu] It's used in dpaa2-ethtool.c for .get_ts_info interface of
> ethtool_ops.
> 
> > dev_set_drvdata(dev, NULL); is not needed.
> 
> [Y.b. Lu] Will remove it in next version.
> 
> > Can rtc_drv be made const?
> 
> [Y.b. Lu] Will use const in next version.
> 
> > Is rtc.h used by anything other than rtc.c? It seems like it can be removed.
> 
> [Y.b. Lu] Let me remove it in next version.
> 
> >
> > 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.
 
> 
> >
> > 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.

Thanks,
Ioana