This patch is to move DPAA2 PTP driver out of staging/
since the dpaa2-eth had been moved out.
Signed-off-by: Yangbo Lu <[email protected]>
---
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%)
delete mode 100644 drivers/staging/fsl-dpaa2/rtc/Makefile
diff --git a/drivers/net/ethernet/freescale/Kconfig b/drivers/net/ethernet/freescale/Kconfig
index 7a30276..d3a62bc 100644
--- a/drivers/net/ethernet/freescale/Kconfig
+++ b/drivers/net/ethernet/freescale/Kconfig
@@ -96,13 +96,6 @@ config GIANFAR
on the 8540.
source "drivers/net/ethernet/freescale/dpaa/Kconfig"
-
-config FSL_DPAA2_ETH
- tristate "Freescale DPAA2 Ethernet"
- depends on FSL_MC_BUS && FSL_MC_DPIO
- depends on NETDEVICES && ETHERNET
- ---help---
- Ethernet driver for Freescale DPAA2 SoCs, using the
- Freescale MC bus driver
+source "drivers/net/ethernet/freescale/dpaa2/Kconfig"
endif # NET_VENDOR_FREESCALE
diff --git a/drivers/net/ethernet/freescale/dpaa2/Kconfig b/drivers/net/ethernet/freescale/dpaa2/Kconfig
new file mode 100644
index 0000000..44c5c3a
--- /dev/null
+++ b/drivers/net/ethernet/freescale/dpaa2/Kconfig
@@ -0,0 +1,15 @@
+config FSL_DPAA2_ETH
+ tristate "Freescale DPAA2 Ethernet"
+ depends on FSL_MC_BUS && FSL_MC_DPIO
+ depends on NETDEVICES && ETHERNET
+ help
+ Ethernet driver for Freescale DPAA2 SoCs, using the
+ Freescale MC bus driver
+
+config FSL_DPAA2_PTP_CLOCK
+ tristate "Freescale DPAA2 PTP Clock"
+ depends on FSL_DPAA2_ETH && POSIX_TIMERS
+ select PTP_1588_CLOCK
+ help
+ This driver adds support for using the DPAA2 1588 timer module
+ as a PTP clock.
diff --git a/drivers/net/ethernet/freescale/dpaa2/Makefile b/drivers/net/ethernet/freescale/dpaa2/Makefile
index 9315ecd..312e37f 100644
--- a/drivers/net/ethernet/freescale/dpaa2/Makefile
+++ b/drivers/net/ethernet/freescale/dpaa2/Makefile
@@ -3,9 +3,11 @@
# Makefile for the Freescale DPAA2 Ethernet controller
#
-obj-$(CONFIG_FSL_DPAA2_ETH) += fsl-dpaa2-eth.o
+obj-$(CONFIG_FSL_DPAA2_ETH) += fsl-dpaa2-eth.o
+obj-$(CONFIG_FSL_DPAA2_PTP_CLOCK) += fsl-dpaa2-rtc.o
-fsl-dpaa2-eth-objs := dpaa2-eth.o dpaa2-ethtool.o dpni.o
+fsl-dpaa2-eth-objs := dpaa2-eth.o dpaa2-ethtool.o dpni.o
+fsl-dpaa2-rtc-objs := rtc.o dprtc.o
# Needed by the tracing framework
CFLAGS_dpaa2-eth.o := -I$(src)
diff --git a/drivers/staging/fsl-dpaa2/rtc/dprtc-cmd.h b/drivers/net/ethernet/freescale/dpaa2/dprtc-cmd.h
similarity index 100%
rename from drivers/staging/fsl-dpaa2/rtc/dprtc-cmd.h
rename to drivers/net/ethernet/freescale/dpaa2/dprtc-cmd.h
diff --git a/drivers/staging/fsl-dpaa2/rtc/dprtc.c b/drivers/net/ethernet/freescale/dpaa2/dprtc.c
similarity index 100%
rename from drivers/staging/fsl-dpaa2/rtc/dprtc.c
rename to drivers/net/ethernet/freescale/dpaa2/dprtc.c
diff --git a/drivers/staging/fsl-dpaa2/rtc/dprtc.h b/drivers/net/ethernet/freescale/dpaa2/dprtc.h
similarity index 100%
rename from drivers/staging/fsl-dpaa2/rtc/dprtc.h
rename to drivers/net/ethernet/freescale/dpaa2/dprtc.h
diff --git a/drivers/staging/fsl-dpaa2/rtc/rtc.c b/drivers/net/ethernet/freescale/dpaa2/rtc.c
similarity index 100%
rename from drivers/staging/fsl-dpaa2/rtc/rtc.c
rename to drivers/net/ethernet/freescale/dpaa2/rtc.c
diff --git a/drivers/staging/fsl-dpaa2/rtc/rtc.h b/drivers/net/ethernet/freescale/dpaa2/rtc.h
similarity index 100%
rename from drivers/staging/fsl-dpaa2/rtc/rtc.h
rename to drivers/net/ethernet/freescale/dpaa2/rtc.h
diff --git a/drivers/staging/fsl-dpaa2/Kconfig b/drivers/staging/fsl-dpaa2/Kconfig
index 59aaae7..991e154 100644
--- a/drivers/staging/fsl-dpaa2/Kconfig
+++ b/drivers/staging/fsl-dpaa2/Kconfig
@@ -16,11 +16,3 @@ config FSL_DPAA2_ETHSW
---help---
Driver for Freescale DPAA2 Ethernet Switch. Select
BRIDGE to have support for bridge tools.
-
-config FSL_DPAA2_PTP_CLOCK
- tristate "Freescale DPAA2 PTP Clock"
- depends on FSL_DPAA2_ETH && POSIX_TIMERS
- select PTP_1588_CLOCK
- help
- This driver adds support for using the DPAA2 1588 timer module
- as a PTP clock.
diff --git a/drivers/staging/fsl-dpaa2/Makefile b/drivers/staging/fsl-dpaa2/Makefile
index 464f242..c92ab98 100644
--- a/drivers/staging/fsl-dpaa2/Makefile
+++ b/drivers/staging/fsl-dpaa2/Makefile
@@ -3,4 +3,3 @@
#
obj-$(CONFIG_FSL_DPAA2_ETHSW) += ethsw/
-obj-$(CONFIG_FSL_DPAA2_PTP_CLOCK) += rtc/
diff --git a/drivers/staging/fsl-dpaa2/rtc/Makefile b/drivers/staging/fsl-dpaa2/rtc/Makefile
deleted file mode 100644
index 5468da0..0000000
--- a/drivers/staging/fsl-dpaa2/rtc/Makefile
+++ /dev/null
@@ -1,7 +0,0 @@
-#
-# Makefile for the Freescale DPAA2 PTP clock
-#
-
-obj-$(CONFIG_FSL_DPAA2_PTP_CLOCK) += dpaa2-rtc.o
-
-dpaa2-rtc-objs := rtc.o dprtc.o
--
1.7.1
The files maintained under DPAA2 PTP/ETHERNET needs to
be updated since dpaa2 ptp driver had been moved into
drivers/net/ethernet/freescale/dpaa2/.
Signed-off-by: Yangbo Lu <[email protected]>
---
MAINTAINERS | 11 ++++++++---
1 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/MAINTAINERS b/MAINTAINERS
index 15565de..ba6f441 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4530,7 +4530,11 @@ DPAA2 ETHERNET DRIVER
M: Ioana Radulescu <[email protected]>
L: [email protected]
S: Maintained
-F: drivers/net/ethernet/freescale/dpaa2
+F: drivers/net/ethernet/freescale/dpaa2/dpaa2-eth*
+F: drivers/net/ethernet/freescale/dpaa2/dpni*
+F: drivers/net/ethernet/freescale/dpaa2/dpkg.h
+F: drivers/net/ethernet/freescale/dpaa2/Makefile
+F: drivers/net/ethernet/freescale/dpaa2/Kconfig
DPAA2 ETHERNET SWITCH DRIVER
M: Ioana Radulescu <[email protected]>
@@ -4541,9 +4545,10 @@ F: drivers/staging/fsl-dpaa2/ethsw
DPAA2 PTP CLOCK DRIVER
M: Yangbo Lu <[email protected]>
-L: [email protected]
+L: [email protected]
S: Maintained
-F: drivers/staging/fsl-dpaa2/rtc
+F: drivers/net/ethernet/freescale/dpaa2/rtc*
+F: drivers/net/ethernet/freescale/dpaa2/dprtc*
DPT_I2O SCSI RAID DRIVER
M: Adaptec OEM Raid Solutions <[email protected]>
--
1.7.1
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 <[email protected]>
> ---
> 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
Hi Andrew,
Thanks a lot for your comments.
Please see my comments inline.
Best regards,
Yangbo Lu
> -----Original Message-----
> From: Andrew Lunn <[email protected]>
> Sent: Thursday, September 27, 2018 9:25 PM
> To: Y.b. Lu <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; Richard Cochran <[email protected]>;
> David S . Miller <[email protected]>; Ioana Ciocoi Radulescu
> <[email protected]>; Greg Kroah-Hartman
> <[email protected]>
> 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 <[email protected]>
> > ---
> > 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
> -----Original Message-----
> From: Y.b. Lu
> Sent: Friday, September 28, 2018 11:04 AM
> To: Andrew Lunn <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; Richard Cochran <[email protected]>;
> David S . Miller <[email protected]>; Ioana Ciocoi Radulescu
> <[email protected]>; Greg Kroah-Hartman
> <[email protected]>
> 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 <[email protected]>
> > Sent: Thursday, September 27, 2018 9:25 PM
> > To: Y.b. Lu <[email protected]>
> > Cc: [email protected]; [email protected];
> > [email protected]; Richard Cochran <[email protected]>;
> > David S . Miller <[email protected]>; Ioana Ciocoi Radulescu
> > <[email protected]>; Greg Kroah-Hartman
> > <[email protected]>
> > 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 <[email protected]>
> > > ---
> > > 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
On Fri, Sep 28, 2018 at 10:20:55AM +0000, 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
> > 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
Hi Ioana,
> -----Original Message-----
> From: Ioana Ciocoi Radulescu
> Sent: Friday, September 28, 2018 6:21 PM
> To: Y.b. Lu <[email protected]>; Andrew Lunn <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; Richard Cochran <[email protected]>;
> David S . Miller <[email protected]>; Greg Kroah-Hartman
> <[email protected]>
> 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 <[email protected]>
> > Cc: [email protected]; [email protected];
> > [email protected]; Richard Cochran <[email protected]>;
> > David S . Miller <[email protected]>; Ioana Ciocoi Radulescu
> > <[email protected]>; Greg Kroah-Hartman
> > <[email protected]>
> > 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
Hi Andrew,
> -----Original Message-----
> From: Andrew Lunn <[email protected]>
> Sent: Friday, September 28, 2018 11:18 PM
> To: Y.b. Lu <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; Richard Cochran <[email protected]>;
> David S . Miller <[email protected]>; Ioana Ciocoi Radulescu
> <[email protected]>; Greg Kroah-Hartman
> <[email protected]>
> 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
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' <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; Richard Cochran <[email protected]>;
> David S . Miller <[email protected]>; Ioana Ciocoi Radulescu
> <[email protected]>; Greg Kroah-Hartman
> <[email protected]>
> 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 <[email protected]>
> > Sent: Thursday, September 27, 2018 9:25 PM
> > To: Y.b. Lu <[email protected]>
> > Cc: [email protected]; [email protected];
> > [email protected]; Richard Cochran <[email protected]>;
> > David S . Miller <[email protected]>; Ioana Ciocoi Radulescu
> > <[email protected]>; Greg Kroah-Hartman
> > <[email protected]>
> > 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 <[email protected]>
> > > ---
[...]
>
> > 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.