RE: [PATCH 1/2] net: dpaa2: move DPAA2 PTP driver out of staging/
Hi all, Sent out the v2. Please help to review. Thanks a lot. Hi Andrew, > -Original Message- > From: Y.b. Lu > Sent: Friday, September 28, 2018 4:04 PM > To: 'Andrew Lunn' > Cc: linux-ker...@vger.kernel.org; de...@driverdev.osuosl.org; > net...@vger.kernel.org; Richard Cochran ; > David S . Miller ; Ioana Ciocoi Radulescu > ; Greg Kroah-Hartman > > 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 > > Sent: Thursday, September 27, 2018 9:25 PM > > To: Y.b. Lu > > Cc: linux-ker...@vger.kernel.org; de...@driverdev.osuosl.org; > > net...@vger.kernel.org; Richard Cochran ; > > David S . Miller ; Ioana Ciocoi Radulescu > > ; Greg Kroah-Hartman > > > > 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 > > > --- [...] > > > Can rtc_drv be made const? > > [Y.b. Lu] Will use const in next version. > [Y.b. Lu] Sorry, just found it couldn't be made const. Warnings showed when built with const. > > 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. [Y.b. Lu] Sorry, I still kept that header file since dpaa2_phc_index need to be declared. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH 1/2] net: dpaa2: move DPAA2 PTP driver out of staging/
Hi Andrew, > -Original Message- > From: Andrew Lunn > Sent: Friday, September 28, 2018 11:18 PM > To: Y.b. Lu > Cc: linux-ker...@vger.kernel.org; de...@driverdev.osuosl.org; > net...@vger.kernel.org; Richard Cochran ; > David S . Miller ; Ioana Ciocoi Radulescu > ; Greg Kroah-Hartman > > Subject: Re: [PATCH 1/2] net: dpaa2: move DPAA2 PTP driver out of staging/ > > > > 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. > > That is going to be hard to do. Ideally the driver should work with any > firmware version. You don't really want to force the user to upgrade the > driver/kernel and the firmware at the same time. So you cannot for example > remove this pad. What you might be able to do in newer versions is actually > use the space. But you have to be sure the current code is correctly ignoring > it > and setting it to zero. [Y.b. Lu] Thanks a lot, I think I understand now The files dprtc* defining the APIs were provided together with MC firmware. They were tested working fine. MC firmware would also consider the backward compatibility I think. Regarding to the API files, let me remove unused code before using them, and keep the rest. > > Andrew ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH 1/2] net: dpaa2: move DPAA2 PTP driver out of staging/
Hi Ioana, > -Original Message- > From: Ioana Ciocoi Radulescu > Sent: Friday, September 28, 2018 6:21 PM > To: Y.b. Lu ; Andrew Lunn > Cc: linux-ker...@vger.kernel.org; de...@driverdev.osuosl.org; > net...@vger.kernel.org; Richard Cochran ; > David S . Miller ; Greg Kroah-Hartman > > 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 > > Cc: linux-ker...@vger.kernel.org; de...@driverdev.osuosl.org; > > net...@vger.kernel.org; Richard Cochran ; > > David S . Miller ; Ioana Ciocoi Radulescu > > ; Greg Kroah-Hartman > > > > 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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/2] net: dpaa2: move DPAA2 PTP driver out of staging/
> > 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. That is going to be hard to do. Ideally the driver should work with any firmware version. You don't really want to force the user to upgrade the driver/kernel and the firmware at the same time. So you cannot for example remove this pad. What you might be able to do in newer versions is actually use the space. But you have to be sure the current code is correctly ignoring it and setting it to zero. Andrew ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/2] net: dpaa2: move DPAA2 PTP driver out of staging/
On Fri, Sep 28, 2018 at 10:20:55AM +, Ioana Ciocoi Radulescu wrote: > 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. +1 Thanks, Richard ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
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
> Sent: Thursday, September 27, 2018 9:25 PM
> To: Y.b. Lu
> Cc: linux-ker...@vger.kernel.org; de...@driverdev.osuosl.org;
> net...@vger.kernel.org; Richard Cochran ;
> David S . Miller ; Ioana Ciocoi Radulescu
> ; Greg Kroah-Hartman
>
> 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
> > ---
> > 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.
>
> 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.
>
> Andrew
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
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
> Cc: linux-ker...@vger.kernel.org; de...@driverdev.osuosl.org;
> net...@vger.kernel.org; Richard Cochran ;
> David S . Miller ; Ioana Ciocoi Radulescu
> ; Greg Kroah-Hartman
>
> 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
> > Sent: Thursday, September 27, 2018 9:25 PM
> > To: Y.b. Lu
> > Cc: linux-ker...@vger.kernel.org; de...@driverdev.osuosl.org;
> > net...@vger.kernel.org; Richard Cochran ;
> > David S . Miller ; Ioana Ciocoi Radulescu
> > ; Greg Kroah-Hartman
> >
> > 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
> > > ---
> > > 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
&g
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
> ---
> 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.
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.
kzalloc() could be changed to devm_kzalloc() to simplify the cleanup
Can ptp_dpaa2_caps be made const?
dpaa2_phc_index does not appear to be used.
dev_set_drvdata(dev, NULL); is not needed.
Can rtc_drv be made const?
Is rtc.h used by anything other than rtc.c? It seems like it can be removed.
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?
struct dprtc_cmd_get_irq - Putting pad at the beginning of a struct
seems very odd. And it is not the only example.
Andrew
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
