[RFC,net-next,0/6] net: dsa: microchip: add gPTP support for LAN937x switch

  The LAN937x switch has capable for supporting IEEE 1588 PTP protocol. This
patch series add gPTP profile support and tested using the ptp4l application.
LAN937x has the same PTP register set similar to KSZ9563, hence the
implementation has been made common for the ksz switches. But the testing is
done only for lan937x switch.

Arun Ramadoss (6):
  net: dsa: microchip: adding the posix clock support
  net: dsa: microchip: Initial hardware time stamping support
  net: dsa: microchip: Manipulating absolute time using ptp hw clock
  net: dsa: microchip: enable the ptp interrupt for timestamping
  net: dsa: microchip: Adding the ptp packet reception logic
  net: dsa: microchip: add the transmission tstamp logic

 drivers/net/dsa/microchip/Kconfig       |  10 +
 drivers/net/dsa/microchip/Makefile      |   1 +
 drivers/net/dsa/microchip/ksz_common.c  |  43 +-
 drivers/net/dsa/microchip/ksz_common.h  |  31 +
 drivers/net/dsa/microchip/ksz_ptp.c     | 755 ++++++++++++++++++++++++
 drivers/net/dsa/microchip/ksz_ptp.h     |  84 +++
 drivers/net/dsa/microchip/ksz_ptp_reg.h |  68 +++
 include/linux/dsa/ksz_common.h          |  53 ++
 net/dsa/tag_ksz.c                       | 156 ++++-
 9 files changed, 1192 insertions(+), 9 deletions(-)
 create mode 100644 drivers/net/dsa/microchip/ksz_ptp.c
 create mode 100644 drivers/net/dsa/microchip/ksz_ptp.h
 create mode 100644 drivers/net/dsa/microchip/ksz_ptp_reg.h
 create mode 100644 include/linux/dsa/ksz_common.h


base-commit: 66ae04368efbe20eb8951c9a76158f99ce672f25
  

Hi Arun,

On Fri, Oct 14, 2022 at 08:58:51PM +0530, Arun Ramadoss wrote:
> The LAN937x switch has capable for supporting IEEE 1588 PTP protocol. This
> patch series add gPTP profile support and tested using the ptp4l application.
> LAN937x has the same PTP register set similar to KSZ9563, hence the
> implementation has been made common for the ksz switches. But the testing is
> done only for lan937x switch.

Would it be possible to actually test these patches on KSZ9563?

Christian Eggers tried to add PTP support for this switch a while ago,
and he claims that two-step TX timestamping was de-featured for KSZ95xx
due to hardware errata.
https://patchwork.ozlabs.org/project/netdev/patch/20201019172435.4416-8-ceggers@arri.de/

> 
> Arun Ramadoss (6):
>   net: dsa: microchip: adding the posix clock support
>   net: dsa: microchip: Initial hardware time stamping support
>   net: dsa: microchip: Manipulating absolute time using ptp hw clock
>   net: dsa: microchip: enable the ptp interrupt for timestamping
>   net: dsa: microchip: Adding the ptp packet reception logic
>   net: dsa: microchip: add the transmission tstamp logic
> 
>  drivers/net/dsa/microchip/Kconfig       |  10 +
>  drivers/net/dsa/microchip/Makefile      |   1 +
>  drivers/net/dsa/microchip/ksz_common.c  |  43 +-
>  drivers/net/dsa/microchip/ksz_common.h  |  31 +
>  drivers/net/dsa/microchip/ksz_ptp.c     | 755 ++++++++++++++++++++++++
>  drivers/net/dsa/microchip/ksz_ptp.h     |  84 +++
>  drivers/net/dsa/microchip/ksz_ptp_reg.h |  68 +++
>  include/linux/dsa/ksz_common.h          |  53 ++
>  net/dsa/tag_ksz.c                       | 156 ++++-
>  9 files changed, 1192 insertions(+), 9 deletions(-)
>  create mode 100644 drivers/net/dsa/microchip/ksz_ptp.c
>  create mode 100644 drivers/net/dsa/microchip/ksz_ptp.h
>  create mode 100644 drivers/net/dsa/microchip/ksz_ptp_reg.h
>  create mode 100644 include/linux/dsa/ksz_common.h
> 
> 
> base-commit: 66ae04368efbe20eb8951c9a76158f99ce672f25
> -- 
> 2.36.1
>
  

On Fri, Oct 14, 2022 at 08:58:51PM +0530, Arun Ramadoss wrote:
> The LAN937x switch has capable for supporting IEEE 1588 PTP protocol. This
> patch series add gPTP profile support and tested using the ptp4l application.
> LAN937x has the same PTP register set similar to KSZ9563, hence the
> implementation has been made common for the ksz switches. But the testing is
> done only for lan937x switch.

Unrelated to the proposed implementation. What user space stack do you
use for gPTP bridging?
  

On Mon, 2022-10-17 at 21:46 +0300, Vladimir Oltean wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
Hi Vladimir,

> On Fri, Oct 14, 2022 at 08:58:51PM +0530, Arun Ramadoss wrote:
> > The LAN937x switch has capable for supporting IEEE 1588 PTP
> > protocol. This
> > patch series add gPTP profile support and tested using the ptp4l
> > application.
> > LAN937x has the same PTP register set similar to KSZ9563, hence the
> > implementation has been made common for the ksz switches. But the
> > testing is
> > done only for lan937x switch.
> 
> Unrelated to the proposed implementation. What user space stack do
> you
> use for gPTP bridging?
I had used LinuxPTP stack for testing this patch set and in specific
linuxptp/configs/gptp.cfg 

Test Setup is of
LAN9370 DUT1 <LAN1> --- LAN9370 DUT2 <LAN1>

Ran the below command in both DUTS
#ptp4l -f ~/linuxptp/configs/gptp.cfg -i lan1

-
Arun
  

On Mon, 2022-10-17 at 20:19 +0300, Vladimir Oltean wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 

Hi Vladimir,
Thanks for the comment.

> Hi Arun,
> 
> On Fri, Oct 14, 2022 at 08:58:51PM +0530, Arun Ramadoss wrote:
> > The LAN937x switch has capable for supporting IEEE 1588 PTP
> > protocol. This
> > patch series add gPTP profile support and tested using the ptp4l
> > application.
> > LAN937x has the same PTP register set similar to KSZ9563, hence the
> > implementation has been made common for the ksz switches. But the
> > testing is
> > done only for lan937x switch.
> 
> Would it be possible to actually test these patches on KSZ9563?
> 
> Christian Eggers tried to add PTP support for this switch a while
> ago,
> and he claims that two-step TX timestamping was de-featured for
> KSZ95xx
> due to hardware errata.
> 
https://patchwork.ozlabs.org/project/netdev/patch/20201019172435.4416-8-ceggers@arri.de/

I had developed this patch set to add gPTP support for LAN937x based on
the Christian eggers patch for KSZ9563. Initially I thought of keeping
implementation specific to LAN937x through lan937x_ptp.c files. Since
the register sets are same for LAN937x/KSZ9563, I developed using
ksz_ptp.c so that in future Christain eggers patch can be merged to it
to support the 1 step clock support.
I read the Hardware errata of KSZ95xx on 2 step clock and found that it
was fixed in LAN937x switches. If this is case, Do I need to move this
2 step timestamping specific to LAN937x as LAN937x_ptp.c & not claim
for ksz9563 or common implementation in ksz_ptp.c & export the
functionality based on chip-id in get_ts_info dsa hooks.

--
Arun

> 
> > 
> > Arun Ramadoss (6):
> >   net: dsa: microchip: adding the posix clock support
> >   net: dsa: microchip: Initial hardware time stamping support
> >   net: dsa: microchip: Manipulating absolute time using ptp hw
> > clock
> >   net: dsa: microchip: enable the ptp interrupt for timestamping
> >   net: dsa: microchip: Adding the ptp packet reception logic
> >   net: dsa: microchip: add the transmission tstamp logic
> > 
> >  drivers/net/dsa/microchip/Kconfig       |  10 +
> >  drivers/net/dsa/microchip/Makefile      |   1 +
> >  drivers/net/dsa/microchip/ksz_common.c  |  43 +-
> >  drivers/net/dsa/microchip/ksz_common.h  |  31 +
> >  drivers/net/dsa/microchip/ksz_ptp.c     | 755
> > ++++++++++++++++++++++++
> >  drivers/net/dsa/microchip/ksz_ptp.h     |  84 +++
> >  drivers/net/dsa/microchip/ksz_ptp_reg.h |  68 +++
> >  include/linux/dsa/ksz_common.h          |  53 ++
> >  net/dsa/tag_ksz.c                       | 156 ++++-
> >  9 files changed, 1192 insertions(+), 9 deletions(-)
> >  create mode 100644 drivers/net/dsa/microchip/ksz_ptp.c
> >  create mode 100644 drivers/net/dsa/microchip/ksz_ptp.h
> >  create mode 100644 drivers/net/dsa/microchip/ksz_ptp_reg.h
> >  create mode 100644 include/linux/dsa/ksz_common.h
> > 
> > 
> > base-commit: 66ae04368efbe20eb8951c9a76158f99ce672f25
> > --
> > 2.36.1
> >
  

On Tue, Oct 18, 2022 at 06:29:17AM +0000, Arun.Ramadoss@microchip.com wrote:
> > On Fri, Oct 14, 2022 at 08:58:51PM +0530, Arun Ramadoss wrote:
> > > The LAN937x switch has capable for supporting IEEE 1588 PTP protocol. This
> > > patch series add gPTP profile support and tested using the ptp4l application.
> > > LAN937x has the same PTP register set similar to KSZ9563, hence the
> > > implementation has been made common for the ksz switches. But the testing is
> > > done only for lan937x switch.
> > 
> > Unrelated to the proposed implementation. What user space stack do you
> > use for gPTP bridging?
> 
> I had used LinuxPTP stack for testing this patch set and in specific
> linuxptp/configs/gptp.cfg 
> 
> Test Setup is of
> LAN9370 DUT1 <LAN1> --- LAN9370 DUT2 <LAN1>
> 
> Ran the below command in both DUTS
> #ptp4l -f ~/linuxptp/configs/gptp.cfg -i lan1

gPTP bridges are when you specify "-i" more than once, similar to how
1588 boundary clocks are created in ptp4l. Linuxptp does not support
gPTP bridges; the time information needs to be relayed differently
compared to both a BC and a TC, and there is also a Follow_Up information
TLV which needs to be updated.

So I guess the answer is, you don't test gPTP bridging, ok.
  

On Tue, Oct 18, 2022 at 06:44:04AM +0000, Arun.Ramadoss@microchip.com wrote:
> I had developed this patch set to add gPTP support for LAN937x based on
> the Christian eggers patch for KSZ9563. Initially I thought of keeping
> implementation specific to LAN937x through lan937x_ptp.c files. Since
> the register sets are same for LAN937x/KSZ9563, I developed using
> ksz_ptp.c so that in future Christain eggers patch can be merged to it
> to support the 1 step clock support.
> I read the Hardware errata of KSZ95xx on 2 step clock and found that it
> was fixed in LAN937x switches. If this is case, Do I need to move this
> 2 step timestamping specific to LAN937x as LAN937x_ptp.c & not claim
> for ksz9563 or common implementation in ksz_ptp.c & export the
> functionality based on chip-id in get_ts_info dsa hooks.

The high-level visible behavior needs to be that the kernel denies
hardware timestamping from being enabled on the platforms on which it
does not work (this includes platforms on which it is conveniently
"not tested" by Microchip engineers, despite there being published
errata stating it doesn't work). Then, the code organization needs to be
such that if anyone wants to add one step TX timestamping to KSZ9477/KSZ9563
as a workaround later, the code reuse is close to maximal without
further refactoring. And there should be plenty of reuse beyond the TX
timestamping procedure.

I expect that Christian will also be able to find some time to review
this RFC and propose some changes/ask some questions based on his prior
observations, at least so he said privately.
  

On Tue, 2022-10-18 at 13:29 +0300, Vladimir Oltean wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On Tue, Oct 18, 2022 at 06:44:04AM +0000, Arun.Ramadoss@microchip.com
>  wrote:
> > I had developed this patch set to add gPTP support for LAN937x
> > based on
> > the Christian eggers patch for KSZ9563. Initially I thought of
> > keeping
> > implementation specific to LAN937x through lan937x_ptp.c files.
> > Since
> > the register sets are same for LAN937x/KSZ9563, I developed using
> > ksz_ptp.c so that in future Christain eggers patch can be merged to
> > it
> > to support the 1 step clock support.
> > I read the Hardware errata of KSZ95xx on 2 step clock and found
> > that it
> > was fixed in LAN937x switches. If this is case, Do I need to move
> > this
> > 2 step timestamping specific to LAN937x as LAN937x_ptp.c & not
> > claim
> > for ksz9563 or common implementation in ksz_ptp.c & export the
> > functionality based on chip-id in get_ts_info dsa hooks.
> 
> The high-level visible behavior needs to be that the kernel denies
> hardware timestamping from being enabled on the platforms on which it
> does not work (this includes platforms on which it is conveniently
> "not tested" by Microchip engineers, despite there being published
> errata stating it doesn't work). Then, the code organization needs to
> be
> such that if anyone wants to add one step TX timestamping to
> KSZ9477/KSZ9563
> as a workaround later, the code reuse is close to maximal without
> further refactoring. And there should be plenty of reuse beyond the
> TX
> timestamping procedure.
> 
> I expect that Christian will also be able to find some time to review
> this RFC and propose some changes/ask some questions based on his
> prior
> observations, at least so he said privately.

Thanks Vladimir. I will wait for Christian feedback.

Hi Christian,
To test this patch on KSZ9563, we need to increase the number of
interrupts port_nirqs in KSZ9893 from 2 to 3. Since the chip id of
KSZ9893 and KSZ9563 are same, I had reused the ksz_chip_data same for
both chips. But this chip differ with number of port interrupts. So we
need to update it. We are generating a new patch for adding the new
element in the ksz_chip_data for KSZ9563.
For now, you can update the code as below for testing the patch

-- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -1266,7 +1266,7 @@ const struct ksz_chip_data ksz_switch_chips[] =
{         
                 .num_statics = 16,
                 .cpu_ports = 0x07,      /* can be configured as cpu
port */
                 .port_cnt = 3,          /* total port count */
 -               .port_nirqs = 2,
 +               .port_nirqs = 3,
                 .ops = &ksz9477_dev_ops,
                 .mib_names = ksz9477_mib_names,
                 .mib_cnt = ARRAY_SIZE(ksz9477_mib_names),

--
Arun
  

Hi Arun, hi Vladimir,

On Tuesday, 18 October 2022, 15:42:41 CEST, Arun.Ramadoss@microchip.com wrote:
> Thanks Vladimir. I will wait for Christian feedback.
> 
> Hi Christian,
> To test this patch on KSZ9563, we need to increase the number of
> interrupts port_nirqs in KSZ9893 from 2 to 3. Since the chip id of
> KSZ9893 and KSZ9563 are same, I had reused the ksz_chip_data same for
> both chips. But this chip differ with number of port interrupts. So we
> need to update it. We are generating a new patch for adding the new
> element in the ksz_chip_data for KSZ9563.
> For now, you can update the code as below for testing the patch
> 
> -- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -1266,7 +1266,7 @@ const struct ksz_chip_data ksz_switch_chips[] =
> {         
>                  .num_statics = 16,
>                  .cpu_ports = 0x07,      /* can be configured as cpu
> port */
>                  .port_cnt = 3,          /* total port count */
>  -               .port_nirqs = 2,
>  +               .port_nirqs = 3,
>                  .ops = &ksz9477_dev_ops,
>                  .mib_names = ksz9477_mib_names,
>                  .mib_cnt = ARRAY_SIZE(ksz9477_mib_names),
> 
> --

sorry for the delay. I'm currently quite busy with my own work and
my kids' school stuff. Additionally I had to update my internal kernel tree
from 5.15.y-stable-rt to the latest netdev which took longer than I
expected. (Preempt-RT patches tend to become smaller, my ones are only
getting larger).

Prior applying the patches, everything seems to build and run fine.

After applying the patch series, I had some trouble with linking. I had
configured nearly everything as a module:

CONFIG_PTP_1588_CLOCK=m
CONFIG_NET_DSA=m
CONFIG_NET_DSA_MICROCHIP_KSZ_COMMON=m    [ksz_switch.ko]
CONFIG_NET_DSA_MICROCHIP_KSZ9477_I2C=m
CONFIG_NET_DSA_MICROCHIP_KSZ_PTP=y       [builtin]

With this configuration, kbuild doesn't even try to compile ksz_ptp.c:

ERROR: modpost: "ksz_hwtstamp_get" [drivers/net/dsa/microchip/ksz_switch.ko] undefined!
ERROR: modpost: "ksz_hwtstamp_set" [drivers/net/dsa/microchip/ksz_switch.ko] undefined!
ERROR: modpost: "ksz_port_txtstamp" [drivers/net/dsa/microchip/ksz_switch.ko] undefined!
ERROR: modpost: "ksz_ptp_clock_register" [drivers/net/dsa/microchip/ksz_switch.ko] undefined!
ERROR: modpost: "ksz_port_deferred_xmit" [drivers/net/dsa/microchip/ksz_switch.ko] undefined!
ERROR: modpost: "ksz_ptp_clock_unregister" [drivers/net/dsa/microchip/ksz_switch.ko] undefined!
ERROR: modpost: "ksz_ptp_irq_free" [drivers/net/dsa/microchip/ksz_switch.ko] undefined!
ERROR: modpost: "ksz_tstamp_reconstruct" [drivers/net/dsa/microchip/ksz_switch.ko] undefined!
ERROR: modpost: "ksz_get_ts_info" [drivers/net/dsa/microchip/ksz_switch.ko] undefined!
ERROR: modpost: "ksz_ptp_irq_setup" [drivers/net/dsa/microchip/ksz_switch.ko] undefined!

After setting all of the above to 'y', the build process works (but I would prefer
being able to build as modules). At startup I get a NULL pointer dereference (see below),
but I haven't tried to track down the source yet (will continue tomorrow).

regards,
Christian

[   17.749629] ksz9477-switch 0-005f: Port2: using phy mode rmii instead of rgmii
[   17.785998] 8<--- cut here ---
[   17.789732] Unable to handle kernel NULL pointer dereference at virtual address 00000000
[   17.798006] [00000000] *pgd=00000000
[   17.801573] Internal error: Oops: 805 [#1] THUMB2
[   17.806331] Modules linked in: st_magn_i2c st_sensors_i2c st_magn as73211 usb_storage st_sensors industrialio_triggered_buffer ksz9477_i2c(+) btusb rtc_rv3028 at24 kfifo_b
 spidev leds_gpio leds_pwm led_class iio_trig_sysfs imx6sx_adc industrialio micrel fec imx_napi at25 spi_imx i2c_imx nfsv3 nfs lockd grace sunrpc usb_f_ncm u_ether libcomposi
[   17.847335] CPU: 0 PID: 201 Comm: udevd Not tainted 6.1.0-rc1+ #198
[   17.853768] Hardware name: Freescale i.MX6 Ultralite (Device Tree)
[   17.860060] PC is at ksz_connect_tag_protocol+0x6/0x18
[   17.865286] LR is at dsa_register_switch+0x5a9/0x780
[   17.870336] pc : [<c02cc6be>]    lr : [<c03a4f45>]    psr: 60000033
[   17.876774] sp : c22abc30  ip : ffffffff  fp : 00000000
[   17.882095] r10: c047660c  r9 : c0476e70  r8 : c5dcb400
[   17.887412] r7 : c4f31808  r6 : c1ba1f40  r5 : c4f31800  r4 : c1ba1f40
[   17.894058] r3 : 00000000  r2 : c02d1325  r1 : 00000007  r0 : 00000000
[   17.900766] Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA Thumb  Segment none
[   17.908211] Control: 50c53c7d  Table: 822b0059  DAC: 00000051
[   17.914055] Register r0 information: NULL pointer
[   17.918929] Register r1 information: non-paged memory
[   17.924068] Register r2 information: non-slab/vmalloc memory
[   17.929828] Register r3 information: NULL pointer
[   17.934613] Register r4 information: slab kmalloc-192 start c1ba1f00 pointer offset 64 size 192
[   17.943535] Register r5 information: slab kmalloc-64 start c4f31800 pointer offset 0 size 64
[   17.952131] Register r6 information: slab kmalloc-192 start c1ba1f00 pointer offset 64 size 192
[   17.961071] Register r7 information: slab kmalloc-64 start c4f31800 pointer offset 8 size 64
[   17.969665] Register r8 information: slab kmalloc-512 start c5dcb400 pointer offset 0 size 512
[   17.978434] Register r9 information: non-slab/vmalloc memory
[   17.984254] Register r10 information: non-slab/vmalloc memory
[   17.990103] Register r11 information: NULL pointer
[   17.994977] Register r12 information: non-paged memory
[   18.000206] Process udevd (pid: 201, stack limit = 0x4afbccb6)
[   18.006223] Stack: (0xc22abc30 to 0xc22ac000)
[   18.010658] bc20:                                     00000000 c22abc48 c0c01300 ff8064a4
[   18.018992] bc40: ffffffff 00000002 c7eee0e4 00000000 00000168 bf981240 bf9810b8 c5dcbc00
[   18.027527] bc60: 40000113 00000004 c22abc94 c0306423 40000113 c22f8cd0 c7ef70a8 c0306423
[   18.035857] bc80: c22abc9c c030823d c7ef73cc c22f8cd0 00000007 c2249240 00000000 c7ef70a8
[   18.044186] bca0: 000008e0 00000007 c051acdd c05414e4 c05414f9 c02cd2fb 00000000 00000000
[   18.052718] bcc0: 00000000 00000002 ffffffff ffffffff c0fdf020 000000c4 c2249240 c0fdf000
[   18.061047] bce0: 00000003 c0fdf020 c224928c bf981240 bf9810b8 bf9800ab c22abd24 bf9810b3
[   18.069375] bd00: 00000010 00000001 00000000 00000000 00000000 00000020 00000000 00000000
[   18.077893] bd20: 00000000 00000000 00000000 00000000 00000000 bf98002f bf98002b c2249258
[   18.086222] bd40: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[   18.094611] bd60: 0000ffff 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[   18.102940] bd80: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[   18.111270] bda0: 00000001 00000001 00000000 00000000 00000000 00000000 00000000 00000000
[   18.119684] bdc0: c32bffc0 c0fdf000 bf980033 bf98201c c0fdf020 c081f0c0 00000026 0000017b
[   18.128019] bde0: 00000000 c02f8a8d c02f894b 00000000 c0fdf020 bf98201c 00000000 c02993f5
[   18.136349] be00: 00000001 c0877380 c087748c bf98201c c0fdf020 c0299557 c0fdf064 00000000
[   18.144739] be20: c0fdf020 bf98201c c080abd8 c081f0c0 c3035d34 c0299853 00000000 c0fdf020
[   18.153068] be40: bf98201c c02997c7 c080abd8 c029880b c0d17dcc c1878ab0 bf98201c c3035d00
[   18.153068] be40: bf98201c c02997c7 c080abd8 c029880b c0d17dcc c1878ab0 bf98201c c3035d00
[   18.161477] be60: 00000000 c0298a93 bf98109f bf9810a0 0000006b bf98201c b6e6a290 c23dba00
[   18.169808] be80: 00000000 c081f0c0 c23dba00 c0299c1b bf982074 bf982000 b6e6a290 c02f8875
[   18.178137] bea0: 00000000 bf9b0001 b6e6a290 c0101907 c0c01100 00000cc0 ffffffff 00000008
[   18.186525] bec0: 00000cc0 c01c1aa7 00006c65 00000000 00000000 c32bfc80 c0138a27 c32bfc80
[   18.194853] bee0: 00000008 00000040 c32bfc80 c01ab021 00000cc0 ffffffff bf982080 b6e6a290
[   18.203182] bf00: c32bfc80 0000017b c010027c c0138a41 bf982080 c7fb50e0 00000000 b6e6a290
[   18.211592] bf20: 00000009 c0139cd1 c22abf38 7fffffff 00000000 00000002 c95aa000 c95ac958
[   18.219922] bf40: c95aca40 c95aa000 001de8b0 c97764d0 c97762c8 c9733218 00003000 00003080
[   18.228311] bf60: 00012340 000030e3 00000000 00000000 00000000 00000000 00000000 00012330
[   18.236640] bf80: 00000749 0000074a 00000279 00000000 00000272 00000000 00000000 b6e7b670
[   18.244971] bfa0: 0000017b c0100041 00000000 b6e7b670 00000009 b6e6a290 00000000 00000000
[   18.253373] bfc0: 00000000 b6e7b670 0000017b 0000017b 00000000 b6e7b670 00020000 00000000
[   18.261710] bfe0: b6e6a290 befaa918 b6e66a23 b6ed4c22 40000030 00000009 00000000 00000000
[   18.270240]  ksz_connect_tag_protocol from dsa_register_switch+0x5a9/0x780
[   18.277243]  dsa_register_switch from ksz_switch_register+0x417/0x48c
[   18.283797]  ksz_switch_register from ksz9477_i2c_probe+0x79/0xfce [ksz9477_i2c]
[   18.291583]  ksz9477_i2c_probe [ksz9477_i2c] from i2c_device_probe+0x143/0x15a
[   18.299016]  i2c_device_probe from really_probe+0xb1/0x188
[   18.304597]  really_probe from driver_probe_device+0x2b/0x88
[   18.310354]  driver_probe_device from __driver_attach+0x8d/0x9e
[   18.316438]  __driver_attach from bus_for_each_dev+0x2b/0x42
[   18.322196]  bus_for_each_dev from bus_add_driver+0x6f/0x128
[   18.327955]  bus_add_driver from driver_register+0x59/0x8e
[   18.333537]  driver_register from i2c_register_driver+0x35/0x54
[   18.339642]  i2c_register_driver from do_one_initcall+0x2b/0xbc
[   18.345677]  do_one_initcall from do_init_module+0x2d/0x1a0
[   18.351347]  do_init_module from sys_finit_module+0x65/0x6c
[   18.357018]  sys_finit_module from ret_fast_syscall+0x1/0x5c
[   18.362838] Exception stack(0xc22abfa8 to 0xc22abff0)
[   18.367982] bfa0:                   00000000 b6e7b670 00000009 b6e6a290 00000000 00000000
[   18.376311] bfc0: 00000000 b6e7b670 0000017b 0000017b 00000000 b6e7b670 00020000 00000000
[   18.384717] bfe0: b6e6a290 befaa918 b6e66a23 b6ed4c22
[   18.389862] Code: 0093 6a03 2000 4a02 (601a) 4a02
[   18.394903] ---[ end trace 0000000000000000 ]---
  

Hi Christian,
Thanks for the feedback.

On Sun, 2022-10-23 at 22:15 +0200, Christian Eggers wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> Hi Arun, hi Vladimir,
> 
> On Tuesday, 18 October 2022, 15:42:41 CEST, 
> Arun.Ramadoss@microchip.com wrote:
> > Thanks Vladimir. I will wait for Christian feedback.
> > 
> > Hi Christian,
> > To test this patch on KSZ9563, we need to increase the number of
> > interrupts port_nirqs in KSZ9893 from 2 to 3. Since the chip id of
> > KSZ9893 and KSZ9563 are same, I had reused the ksz_chip_data same
> for
> > both chips. But this chip differ with number of port interrupts. So
> we
> > need to update it. We are generating a new patch for adding the new
> > element in the ksz_chip_data for KSZ9563.
> > For now, you can update the code as below for testing the patch
> > 
> > -- a/drivers/net/dsa/microchip/ksz_common.c
> > +++ b/drivers/net/dsa/microchip/ksz_common.c
> > @@ -1266,7 +1266,7 @@ const struct ksz_chip_data ksz_switch_chips[]
> =
> > { 
> > .num_statics = 16,
> > .cpu_ports = 0x07, /* can be configured as cpu
> > port */
> > .port_cnt = 3, /* total port count */
> > - .port_nirqs = 2,
> > + .port_nirqs = 3,
> > .ops = &ksz9477_dev_ops,
> > .mib_names = ksz9477_mib_names,
> > .mib_cnt = ARRAY_SIZE(ksz9477_mib_names),
> > 
> > --
> 
> sorry for the delay. I'm currently quite busy with my own work and
> my kids' school stuff. Additionally I had to update my internal
> kernel tree
> from 5.15.y-stable-rt to the latest netdev which took longer than I
> expected. (Preempt-RT patches tend to become smaller, my ones are
> only
> getting larger).
> 
> Prior applying the patches, everything seems to build and run fine.
> 
> After applying the patch series, I had some trouble with linking. I
> had
> configured nearly everything as a module:
> 
> CONFIG_PTP_1588_CLOCK=m
> CONFIG_NET_DSA=m
> CONFIG_NET_DSA_MICROCHIP_KSZ_COMMON=m [ksz_switch.ko]
> CONFIG_NET_DSA_MICROCHIP_KSZ9477_I2C=m
> CONFIG_NET_DSA_MICROCHIP_KSZ_PTP=y [builtin]
> 
> With this configuration, kbuild doesn't even try to compile
> ksz_ptp.c:
> 
> ERROR: modpost: "ksz_hwtstamp_get"
> [drivers/net/dsa/microchip/ksz_switch.ko] undefined!
> ERROR: modpost: "ksz_hwtstamp_set"
> [drivers/net/dsa/microchip/ksz_switch.ko] undefined!
> ERROR: modpost: "ksz_port_txtstamp"
> [drivers/net/dsa/microchip/ksz_switch.ko] undefined!
> ERROR: modpost: "ksz_ptp_clock_register"
> [drivers/net/dsa/microchip/ksz_switch.ko] undefined!
> ERROR: modpost: "ksz_port_deferred_xmit"
> [drivers/net/dsa/microchip/ksz_switch.ko] undefined!
> ERROR: modpost: "ksz_ptp_clock_unregister"
> [drivers/net/dsa/microchip/ksz_switch.ko] undefined!
> ERROR: modpost: "ksz_ptp_irq_free"
> [drivers/net/dsa/microchip/ksz_switch.ko] undefined!
> ERROR: modpost: "ksz_tstamp_reconstruct"
> [drivers/net/dsa/microchip/ksz_switch.ko] undefined!
> ERROR: modpost: "ksz_get_ts_info"
> [drivers/net/dsa/microchip/ksz_switch.ko] undefined!
> ERROR: modpost: "ksz_ptp_irq_setup"
> [drivers/net/dsa/microchip/ksz_switch.ko] undefined!
> 
> After setting all of the above to 'y', the build process works (but I
> would prefer
> being able to build as modules). 

May be this is due to kconfig of config_ksz_ptp  defined bool instead
of tristate. Do I need to change the config_ksz_ptp to tristate in
order to compile as modules?

> At startup I get a NULL pointer dereference (see below),
> but I haven't tried to track down the source yet (will continue
> tomorrow).

Not sure of Null pointer exception. Actually the
ksz_connect_tag_protocol is used only to assign the call back of
ksz_port_deferred_xmit and ksz_tstamp_reconstruct to the ds-
>tagger_data. The memory for ds->tagger_data is allocated using the
ksz_connect function in net/dsa/tag_ksz.c file. As per the flow, memory
is allocated in ksz_connect and then connect_tag dsa_hook in the
ksz_common.c is called in order to assign ptp call backs.

> 
> regards,
> Christian
> 
> [ 17.749629] ksz9477-switch 0-005f: Port2: using phy mode rmii
> instead of rgmii
> [ 17.785998] 8<--- cut here ---
> [ 17.789732] Unable to handle kernel NULL pointer dereference at
> virtual address 00000000
> [ 17.798006] [00000000] *pgd=00000000
> [ 17.801573] Internal error: Oops: 805 [#1] THUMB2
> [ 17.806331] Modules linked in: st_magn_i2c st_sensors_i2c st_magn
> as73211 usb_storage st_sensors industrialio_triggered_buffer
> ksz9477_i2c(+) btusb rtc_rv3028 at24 kfifo_b
> spidev leds_gpio leds_pwm led_class iio_trig_sysfs imx6sx_adc
> industrialio micrel fec imx_napi at25 spi_imx i2c_imx nfsv3 nfs lockd
> grace sunrpc usb_f_ncm u_ether libcomposi
> [ 17.847335] CPU: 0 PID: 201 Comm: udevd Not tainted 6.1.0-rc1+ #198
> [ 17.853768] Hardware name: Freescale i.MX6 Ultralite (Device Tree)
> [ 17.860060] PC is at ksz_connect_tag_protocol+0x6/0x18
> [ 17.865286] LR is at dsa_register_switch+0x5a9/0x780
> [ 17.870336] pc : [<c02cc6be>] lr : [<c03a4f45>] psr: 60000033
> [ 17.876774] sp : c22abc30 ip : ffffffff fp : 00000000
> [ 17.882095] r10: c047660c r9 : c0476e70 r8 : c5dcb400
> [ 17.887412] r7 : c4f31808 r6 : c1ba1f40 r5 : c4f31800 r4 : c1ba1f40
> [ 17.894058] r3 : 00000000 r2 : c02d1325 r1 : 00000007 r0 : 00000000
> [ 17.900766] Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA Thumb
> Segment none
> [ 17.908211] Control: 50c53c7d Table: 822b0059 DAC: 00000051
> [ 17.914055] Register r0 information: NULL pointer
> [ 17.918929] Register r1 information: non-paged memory
> [ 17.924068] Register r2 information: non-slab/vmalloc memory
> [ 17.929828] Register r3 information: NULL pointer
> [ 17.934613] Register r4 information: slab kmalloc-192 start c1ba1f00
> pointer offset 64 size 192
> [ 17.943535] Register r5 information: slab kmalloc-64 start c4f31800
> pointer offset 0 size 64
> [ 17.952131] Register r6 information: slab kmalloc-192 start c1ba1f00
> pointer offset 64 size 192
> [ 17.961071] Register r7 information: slab kmalloc-64 start c4f31800
> pointer offset 8 size 64
> [ 17.969665] Register r8 information: slab kmalloc-512 start c5dcb400
> pointer offset 0 size 512
> [ 17.978434] Register r9 information: non-slab/vmalloc memory
> [ 17.984254] Register r10 information: non-slab/vmalloc memory
> [ 17.990103] Register r11 information: NULL pointer
> [ 17.994977] Register r12 information: non-paged memory
> [ 18.000206] Process udevd (pid: 201, stack limit = 0x4afbccb6)
> [ 18.006223] Stack: (0xc22abc30 to 0xc22ac000)
> [ 18.010658] bc20: 00000000 c22abc48 c0c01300 ff8064a4
> [ 18.018992] bc40: ffffffff 00000002 c7eee0e4 00000000 00000168
> bf981240 bf9810b8 c5dcbc00
> [ 18.027527] bc60: 40000113 00000004 c22abc94 c0306423 40000113
> c22f8cd0 c7ef70a8 c0306423
> [ 18.035857] bc80: c22abc9c c030823d c7ef73cc c22f8cd0 00000007
> c2249240 00000000 c7ef70a8
> [ 18.044186] bca0: 000008e0 00000007 c051acdd c05414e4 c05414f9
> c02cd2fb 00000000 00000000
> [ 18.052718] bcc0: 00000000 00000002 ffffffff ffffffff c0fdf020
> 000000c4 c2249240 c0fdf000
> [ 18.061047] bce0: 00000003 c0fdf020 c224928c bf981240 bf9810b8
> bf9800ab c22abd24 bf9810b3
> [ 18.069375] bd00: 00000010 00000001 00000000 00000000 00000000
> 00000020 00000000 00000000
> [ 18.077893] bd20: 00000000 00000000 00000000 00000000 00000000
> bf98002f bf98002b c2249258
> [ 18.086222] bd40: 00000000 00000000 00000000 00000000 00000000
> 00000000 00000000 00000000
> [ 18.094611] bd60: 0000ffff 00000000 00000000 00000000 00000000
> 00000000 00000000 00000000
> [ 18.102940] bd80: 00000000 00000000 00000000 00000000 00000000
> 00000000 00000000 00000000
> [ 18.111270] bda0: 00000001 00000001 00000000 00000000 00000000
> 00000000 00000000 00000000
> [ 18.119684] bdc0: c32bffc0 c0fdf000 bf980033 bf98201c c0fdf020
> c081f0c0 00000026 0000017b
> [ 18.128019] bde0: 00000000 c02f8a8d c02f894b 00000000 c0fdf020
> bf98201c 00000000 c02993f5
> [ 18.136349] be00: 00000001 c0877380 c087748c bf98201c c0fdf020
> c0299557 c0fdf064 00000000
> [ 18.144739] be20: c0fdf020 bf98201c c080abd8 c081f0c0 c3035d34
> c0299853 00000000 c0fdf020
> [ 18.153068] be40: bf98201c c02997c7 c080abd8 c029880b c0d17dcc
> c1878ab0 bf98201c c3035d00
> [ 18.153068] be40: bf98201c c02997c7 c080abd8 c029880b c0d17dcc
> c1878ab0 bf98201c c3035d00
> [ 18.161477] be60: 00000000 c0298a93 bf98109f bf9810a0 0000006b
> bf98201c b6e6a290 c23dba00
> [ 18.169808] be80: 00000000 c081f0c0 c23dba00 c0299c1b bf982074
> bf982000 b6e6a290 c02f8875
> [ 18.178137] bea0: 00000000 bf9b0001 b6e6a290 c0101907 c0c01100
> 00000cc0 ffffffff 00000008
> [ 18.186525] bec0: 00000cc0 c01c1aa7 00006c65 00000000 00000000
> c32bfc80 c0138a27 c32bfc80
> [ 18.194853] bee0: 00000008 00000040 c32bfc80 c01ab021 00000cc0
> ffffffff bf982080 b6e6a290
> [ 18.203182] bf00: c32bfc80 0000017b c010027c c0138a41 bf982080
> c7fb50e0 00000000 b6e6a290
> [ 18.211592] bf20: 00000009 c0139cd1 c22abf38 7fffffff 00000000
> 00000002 c95aa000 c95ac958
> [ 18.219922] bf40: c95aca40 c95aa000 001de8b0 c97764d0 c97762c8
> c9733218 00003000 00003080
> [ 18.228311] bf60: 00012340 000030e3 00000000 00000000 00000000
> 00000000 00000000 00012330
> [ 18.236640] bf80: 00000749 0000074a 00000279 00000000 00000272
> 00000000 00000000 b6e7b670
> [ 18.244971] bfa0: 0000017b c0100041 00000000 b6e7b670 00000009
> b6e6a290 00000000 00000000
> [ 18.253373] bfc0: 00000000 b6e7b670 0000017b 0000017b 00000000
> b6e7b670 00020000 00000000
> [ 18.261710] bfe0: b6e6a290 befaa918 b6e66a23 b6ed4c22 40000030
> 00000009 00000000 00000000
> [ 18.270240] ksz_connect_tag_protocol from
> dsa_register_switch+0x5a9/0x780
> [ 18.277243] dsa_register_switch from ksz_switch_register+0x417/0x48c
> [ 18.283797] ksz_switch_register from ksz9477_i2c_probe+0x79/0xfce
> [ksz9477_i2c]
> [ 18.291583] ksz9477_i2c_probe [ksz9477_i2c] from
> i2c_device_probe+0x143/0x15a
> [ 18.299016] i2c_device_probe from really_probe+0xb1/0x188
> [ 18.304597] really_probe from driver_probe_device+0x2b/0x88
> [ 18.310354] driver_probe_device from __driver_attach+0x8d/0x9e
> [ 18.316438] __driver_attach from bus_for_each_dev+0x2b/0x42
> [ 18.322196] bus_for_each_dev from bus_add_driver+0x6f/0x128
> [ 18.327955] bus_add_driver from driver_register+0x59/0x8e
> [ 18.333537] driver_register from i2c_register_driver+0x35/0x54
> [ 18.339642] i2c_register_driver from do_one_initcall+0x2b/0xbc
> [ 18.345677] do_one_initcall from do_init_module+0x2d/0x1a0
> [ 18.351347] do_init_module from sys_finit_module+0x65/0x6c
> [ 18.357018] sys_finit_module from ret_fast_syscall+0x1/0x5c
> [ 18.362838] Exception stack(0xc22abfa8 to 0xc22abff0)
> [ 18.367982] bfa0: 00000000 b6e7b670 00000009 b6e6a290 00000000
> 00000000
> [ 18.376311] bfc0: 00000000 b6e7b670 0000017b 0000017b 00000000
> b6e7b670 00020000 00000000
> [ 18.384717] bfe0: b6e6a290 befaa918 b6e66a23 b6ed4c22
> [ 18.389862] Code: 0093 6a03 2000 4a02 (601a) 4a02
> [ 18.394903] ---[ end trace 0000000000000000 ]---
> 
> 
> 
>      _______________________________________________________	     Ch
> ristian	  
>  Eggers 	   Software Engineer	   	    ARRI	Arnol
> d & Richter Cine Technik GmbH & Co. Betriebs KG 
>  	  Arriweg 17	, 
> 83071	 
> Stephanskirchen  	    www.arri.com 
> 
>   	 +49 8036 3009-3118						
>  CEggers@arri.de   	 
>  		 Get all the latest information from www.arri.com,, Fac
> ebook, Twitter, Instagram, LinkedIn and YouTube.
>   
> Arnold & Richter Cine Technik GmbH & Co. Betriebs KG
> Sitz: München ‑ Registergericht: Amtsgericht München ‑ Handelsregiste
> rnummer: HRA 57918
> Persönlich haftender Gesellschafter: Arnold & Richter Cine Technik Gm
> bH
> Sitz: München ‑ Registergericht: Amtsgericht München ‑ Handelsregiste
> rnummer: HRB 54477
> Geschäftsführer: Dr. Matthias Erb (Chairman); Dr. Michael Neuhäuser; 
> Stephan Schenk; Walter Trauninger
>  
> 
>
  

Hi Arun, hi Vladimir,

On Tuesday, 18 October 2022, 15:42:41 CEST, Arun.Ramadoss@microchip.com wrote:
> ...
> Thanks Vladimir. I will wait for Christian feedback.
> 
> Hi Christian,
> To test this patch on KSZ9563, we need to increase the number of
> interrupts port_nirqs in KSZ9893 from 2 to 3. Since the chip id of
> KSZ9893 and KSZ9563 are same, I had reused the ksz_chip_data same for
> both chips. But this chip differ with number of port interrupts. So we
> need to update it. We are generating a new patch for adding the new
> element in the ksz_chip_data for KSZ9563.
> For now, you can update the code as below for testing the patch

today I hard first success with your patch series on KSZ9563! ptp4l reported
delay measurements between switch port 1 and the connected Meinberg clock:

> ptp4l[75.590]: port 2: new foreign master ec4670.fffe.0a9dcc-1
> ptp4l[79.590]: selected best master clock ec4670.fffe.0a9dcc
> ptp4l[79.590]: updating UTC offset to 37
> ptp4l[79.591]: port 2: LISTENING to UNCALIBRATED on RS_SLAVE
> ptp4l[81.114]: port 2: delay timeout
> ptp4l[81.117]: delay   filtered        338   raw        338
> ptp4l[81.118]: port 2: minimum delay request interval 2^1
> ptp4l[81.434]: port 1: announce timeout
> ptp4l[81.434]: config item lan0.udp_ttl is 1
> ptp4l[81.436]: config item (null).dscp_event is 0
> ptp4l[81.437]: config item (null).dscp_general is 0
> ptp4l[81.437]: selected best master clock ec4670.fffe.0a9dcc
> ptp4l[81.438]: updating UTC offset to 37
> ptp4l[81.843]: master offset         33 s0 freq   +6937 path delay       338
> ptp4l[82.844]: master offset         26 s2 freq   +6930 path delay       338
> ptp4l[82.844]: port 2: UNCALIBRATED to SLAVE on MASTER_CLOCK_SELECTED
> ptp4l[83.844]: master offset         32 s2 freq   +6962 path delay       338
> ptp4l[84.844]: master offset          3 s2 freq   +6943 path delay       338
> ptp4l[85.844]: master offset        -14 s2 freq   +6927 path delay       338
> ptp4l[86.042]: port 2: delay timeout
> ptp4l[86.045]: delay   filtered        336   raw        335
> ptp4l[86.211]: port 2: delay timeout
> ptp4l[86.213]: delay   filtered        335   raw        331
> ptp4l[86.844]: master offset          3 s2 freq   +6939 path delay       335
> ptp4l[87.847]: master offset         -7 s2 freq   +6930 path delay       335

As a next step I'll try to configure the external output for 1PPS. Is this
already implemented in your patches? The file /sys/class/ptp/ptp2/n_periodic_outputs
shows '0' on my system.

BTW: Which is the preferred delay measurement which I shall test (E2E/P2P)? I
started with E2E is this was configured in the hardware and needs no 1-step
time stamping, but I had to add PTP_MSGTYPE_DELAY_REQ in ksz_port_txtstamp().

On Tuesday, 25 October 2022, 05:41:26 CEST, Arun.Ramadoss@microchip.com wrote:
> On Sun, 2022-10-23 at 22:15 +0200, Christian Eggers wrote:
> > ...
> > After applying the patch series, I had some trouble with linking. I
> > had
> > configured nearly everything as a module:
> > 
> > CONFIG_PTP_1588_CLOCK=m
> > CONFIG_NET_DSA=m
> > CONFIG_NET_DSA_MICROCHIP_KSZ_COMMON=m [ksz_switch.ko]
> > CONFIG_NET_DSA_MICROCHIP_KSZ9477_I2C=m
> > CONFIG_NET_DSA_MICROCHIP_KSZ_PTP=y [builtin]
> > 
> > With this configuration, kbuild doesn't even try to compile
> > ksz_ptp.c:
> > 
> > ERROR: modpost: "ksz_hwtstamp_get"
> > [drivers/net/dsa/microchip/ksz_switch.ko] undefined!
> > ERROR: modpost: "ksz_hwtstamp_set"
> > [drivers/net/dsa/microchip/ksz_switch.ko] undefined!
> > ERROR: modpost: "ksz_port_txtstamp"
> > [drivers/net/dsa/microchip/ksz_switch.ko] undefined!
> > ERROR: modpost: "ksz_ptp_clock_register"
> > [drivers/net/dsa/microchip/ksz_switch.ko] undefined!
> > ERROR: modpost: "ksz_port_deferred_xmit"
> > [drivers/net/dsa/microchip/ksz_switch.ko] undefined!
> > ERROR: modpost: "ksz_ptp_clock_unregister"
> > [drivers/net/dsa/microchip/ksz_switch.ko] undefined!
> > ERROR: modpost: "ksz_ptp_irq_free"
> > [drivers/net/dsa/microchip/ksz_switch.ko] undefined!
> > ERROR: modpost: "ksz_tstamp_reconstruct"
> > [drivers/net/dsa/microchip/ksz_switch.ko] undefined!
> > ERROR: modpost: "ksz_get_ts_info"
> > [drivers/net/dsa/microchip/ksz_switch.ko] undefined!
> > ERROR: modpost: "ksz_ptp_irq_setup"
> > [drivers/net/dsa/microchip/ksz_switch.ko] undefined!
> > 
> > After setting all of the above to 'y', the build process works (but I
> > would prefer
> > being able to build as modules). 
> 
> May be this is due to kconfig of config_ksz_ptp  defined bool instead
> of tristate. Do I need to change the config_ksz_ptp to tristate in
> order to compile as modules?
I'm not an expert for kbuild and cannot tell whether it's allowed to use
bool options which depend on tristate options. At least ksz_ptp.c is compiled
by kbuild if tristate is used. But I needed to add additional EXPORT_SYMBOL()
statements for all non-static functions (see below) for successful linking.

I'm unsure whether it makes sense to build ksz_ptp as a separate module.
Perhaps it should be (conditionally) added to ksz_switch.ko.

On Tuesday, 18 October 2022, 08:44:04 CEST, Arun.Ramadoss@microchip.com wrote:
> I had developed this patch set to add gPTP support for LAN937x based on
> the Christian eggers patch for KSZ9563.
Maybe this could be mentioned somewhere (e.g. extra line in file header of
ksz_ptp.c).  It took a lot of effort (for me) to get this initially running
(e.g. due to limited documentation / support by Microchip).  But I'm quite happy
that this is continued now as it is likely that I'll need PTP support for the
KSZ9563 soon.

For KSZ9563, we will need support for 1-step time stamping as two-step
is not possible.

I've stashed all my local changes into an additional patch (see below).
Please feel free to integrate this into your series.  As soon I get 1PPS
running, I'll continue testing.  Note that I'll be unavailable between Friday
and next Tuesday.

regards,
Christian
---
 drivers/net/dsa/microchip/Kconfig       |  2 +-
 drivers/net/dsa/microchip/ksz9477_i2c.c |  2 +
 drivers/net/dsa/microchip/ksz_common.c  |  2 +-
 drivers/net/dsa/microchip/ksz_ptp.c     | 13 +++++-
 net/dsa/tag_ksz.c                       | 60 +++++++++++++++++++++++--
 5 files changed, 73 insertions(+), 6 deletions(-)

diff --git a/drivers/net/dsa/microchip/Kconfig b/drivers/net/dsa/microchip/Kconfig
index 1e9712ff64e2..ac34c01f39a6 100644
--- a/drivers/net/dsa/microchip/Kconfig
+++ b/drivers/net/dsa/microchip/Kconfig
@@ -22,7 +22,7 @@ config NET_DSA_MICROCHIP_KSZ_SPI
 	  Select to enable support for registering switches configured through SPI.
 
 config NET_DSA_MICROCHIP_KSZ_PTP
-	bool "Support for the PTP clock on the KSZ9563/LAN937x Ethernet Switch"
+	tristate "Support for the PTP clock on the KSZ9563/LAN937x Ethernet Switch"
 	depends on NET_DSA_MICROCHIP_KSZ_COMMON && PTP_1588_CLOCK
 	help
 	  This enables support for timestamping & PTP clock manipulation
diff --git a/drivers/net/dsa/microchip/ksz9477_i2c.c b/drivers/net/dsa/microchip/ksz9477_i2c.c
index 3763930dc6fc..7eb7d887bf43 100644
--- a/drivers/net/dsa/microchip/ksz9477_i2c.c
+++ b/drivers/net/dsa/microchip/ksz9477_i2c.c
@@ -41,6 +41,8 @@ static int ksz9477_i2c_probe(struct i2c_client *i2c,
 	if (i2c->dev.platform_data)
 		dev->pdata = i2c->dev.platform_data;
 
+	dev->irq = i2c->irq;
+
 	ret = ksz_switch_register(dev);
 
 	/* Main DSA driver may not be started yet. */
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 889b3d398def..679c66f1e420 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -1266,7 +1266,7 @@ const struct ksz_chip_data ksz_switch_chips[] = {
 		.num_statics = 16,
 		.cpu_ports = 0x07,	/* can be configured as cpu port */
 		.port_cnt = 3,		/* total port count */
-		.port_nirqs = 2,
+		.port_nirqs = 3,
 		.ops = &ksz9477_dev_ops,
 		.mib_names = ksz9477_mib_names,
 		.mib_cnt = ARRAY_SIZE(ksz9477_mib_names),
diff --git a/drivers/net/dsa/microchip/ksz_ptp.c b/drivers/net/dsa/microchip/ksz_ptp.c
index d11a490a6c87..6e6814286dec 100644
--- a/drivers/net/dsa/microchip/ksz_ptp.c
+++ b/drivers/net/dsa/microchip/ksz_ptp.c
@@ -68,6 +68,7 @@ int ksz_get_ts_info(struct dsa_switch *ds, int port, struct ethtool_ts_info *ts)
 
 	return 0;
 }
+EXPORT_SYMBOL(ksz_get_ts_info);
 
 int ksz_hwtstamp_get(struct dsa_switch *ds, int port, struct ifreq *ifr)
 {
@@ -90,6 +91,7 @@ int ksz_hwtstamp_get(struct dsa_switch *ds, int port, struct ifreq *ifr)
 	return copy_to_user(ifr->ifr_data, &config, sizeof(config)) ?
 		-EFAULT : 0;
 }
+EXPORT_SYMBOL(ksz_hwtstamp_get);
 
 static int ksz_set_hwtstamp_config(struct ksz_device *dev, int port,
 				   struct hwtstamp_config *config)
@@ -106,7 +108,7 @@ static int ksz_set_hwtstamp_config(struct ksz_device *dev, int port,
 	case HWTSTAMP_TX_OFF:
 		prt->hwts_tx_en = false;
 		break;
-	case HWTSTAMP_TX_ON:
+	case HWTSTAMP_TX_ONESTEP_P2P:
 		prt->hwts_tx_en = true;
 		break;
 	default:
@@ -162,6 +164,7 @@ int ksz_hwtstamp_set(struct dsa_switch *ds, int port, struct ifreq *ifr)
 	mutex_unlock(&ptp_data->lock);
 	return ret;
 }
+EXPORT_SYMBOL(ksz_hwtstamp_set);
 
 void ksz_port_txtstamp(struct dsa_switch *ds, int port,
 		       struct sk_buff *skb)
@@ -187,6 +190,7 @@ void ksz_port_txtstamp(struct dsa_switch *ds, int port,
 	ptp_msg_type = ptp_get_msgtype(hdr, type);
 
 	switch (ptp_msg_type) {
+	case PTP_MSGTYPE_DELAY_REQ:
 	case PTP_MSGTYPE_PDELAY_REQ:
 	case PTP_MSGTYPE_PDELAY_RESP:
 	case PTP_MSGTYPE_SYNC:
@@ -202,6 +206,7 @@ void ksz_port_txtstamp(struct dsa_switch *ds, int port,
 
 	KSZ_SKB_CB(skb)->clone = clone;
 }
+EXPORT_SYMBOL(ksz_port_txtstamp);
 
 /* These are function related to the ptp clock info */
 static int _ksz_ptp_gettime(struct ksz_device *dev, struct timespec64 *ts)
@@ -436,6 +441,7 @@ ktime_t ksz_tstamp_reconstruct(struct dsa_switch *ds, ktime_t tstamp)
 
 	return timespec64_to_ktime(ts);
 }
+EXPORT_SYMBOL(ksz_tstamp_reconstruct);
 
 static void ksz_ptp_txtstamp_skb(struct ksz_device *dev,
 				 struct ksz_port *prt, struct sk_buff *skb)
@@ -479,6 +485,7 @@ void ksz_port_deferred_xmit(struct kthread_work *work)
 
 	kfree(xmit_work);
 }
+EXPORT_SYMBOL(ksz_port_deferred_xmit);
 
 static const struct ptp_clock_info ksz_ptp_caps = {
 	.owner		= THIS_MODULE,
@@ -524,6 +531,7 @@ int ksz_ptp_clock_register(struct dsa_switch *ds)
 	ptp_clock_unregister(ptp_data->clock);
 	return ret;
 }
+EXPORT_SYMBOL(ksz_ptp_clock_register);
 
 void ksz_ptp_clock_unregister(struct dsa_switch *ds)
 {
@@ -535,6 +543,7 @@ void ksz_ptp_clock_unregister(struct dsa_switch *ds)
 
 	ptp_clock_unregister(ptp_data->clock);
 }
+EXPORT_SYMBOL(ksz_ptp_clock_unregister);
 
 static irqreturn_t ksz_ptp_msg_thread_fn(int irq, void *dev_id)
 {
@@ -734,6 +743,7 @@ int ksz_ptp_irq_setup(struct dsa_switch *ds, u8 p)
 
 	return ret;
 }
+EXPORT_SYMBOL(ksz_ptp_irq_setup);
 
 void ksz_ptp_irq_free(struct dsa_switch *ds, u8 p)
 {
@@ -749,6 +759,7 @@ void ksz_ptp_irq_free(struct dsa_switch *ds, u8 p)
 
 	irq_domain_remove(ptpirq->domain);
 }
+EXPORT_SYMBOL(ksz_ptp_irq_free);
 
 MODULE_AUTHOR("Arun Ramadoss <arun.ramadoss@microchip.com>");
 MODULE_DESCRIPTION("PTP support for KSZ switch");
diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c
index 582add3398d3..e7680718b478 100644
--- a/net/dsa/tag_ksz.c
+++ b/net/dsa/tag_ksz.c
@@ -251,17 +251,69 @@ MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_KSZ9477);
 #define KSZ9893_TAIL_TAG_OVERRIDE	BIT(5)
 #define KSZ9893_TAIL_TAG_LOOKUP		BIT(6)
 
+/* Time stamp tag is only inserted if PTP is enabled in hardware. */
+static void ksz9893_xmit_timestamp(struct sk_buff *skb)
+{
+//	struct sk_buff *clone = KSZ9477_SKB_CB(skb)->clone;
+//	struct ptp_header *ptp_hdr;
+//	unsigned int ptp_type;
+	u32 tstamp_raw = 0;
+	put_unaligned_be32(tstamp_raw, skb_put(skb, KSZ9477_PTP_TAG_LEN));
+}
+
+/* Defer transmit if waiting for egress time stamp is required.  */
+static struct sk_buff *ksz9893_defer_xmit(struct dsa_port *dp,
+					  struct sk_buff *skb)
+{
+	struct ksz_tagger_data *tagger_data = ksz_tagger_data(dp->ds);
+	struct ksz_tagger_private *priv = ksz_tagger_private(dp->ds);
+	void (*xmit_work_fn)(struct kthread_work *work);
+	struct sk_buff *clone = KSZ_SKB_CB(skb)->clone;
+	struct ksz_deferred_xmit_work *xmit_work;
+	struct kthread_worker *xmit_worker;
+
+	if (!clone)
+		return skb;  /* no deferred xmit for this packet */
+
+	xmit_work_fn = tagger_data->xmit_work_fn;
+	xmit_worker = priv->xmit_worker;
+
+	if (!xmit_work_fn || !xmit_worker)
+		return NULL;
+
+	xmit_work = kzalloc(sizeof(*xmit_work), GFP_ATOMIC);
+	if (!xmit_work)
+		return NULL;
+
+	kthread_init_work(&xmit_work->work, xmit_work_fn);
+	/* Increase refcount so the kfree_skb in dsa_slave_xmit
+	 * won't really free the packet.
+	 */
+	xmit_work->dp = dp;
+	xmit_work->skb = skb_get(skb);
+
+	kthread_queue_work(xmit_worker, &xmit_work->work);
+
+	return NULL;
+}
+
 static struct sk_buff *ksz9893_xmit(struct sk_buff *skb,
 				    struct net_device *dev)
 {
 	struct dsa_port *dp = dsa_slave_to_port(dev);
+	struct ksz_tagger_private *priv;
 	u8 *addr;
 	u8 *tag;
 
+	priv = ksz_tagger_private(dp->ds);
+
+	/* Tag encoding */
+	if (test_bit(KSZ_HWTS_EN, &priv->state))
+		ksz9893_xmit_timestamp(skb);
+
 	if (skb->ip_summed == CHECKSUM_PARTIAL && skb_checksum_help(skb))
 		return NULL;
 
-	/* Tag encoding */
 	tag = skb_put(skb, KSZ_INGRESS_TAG_LEN);
 	addr = skb_mac_header(skb);
 
@@ -270,7 +322,7 @@ static struct sk_buff *ksz9893_xmit(struct sk_buff *skb,
 	if (is_link_local_ether_addr(addr))
 		*tag |= KSZ9893_TAIL_TAG_OVERRIDE;
 
-	return skb;
+	return ksz9893_defer_xmit(dp, skb);
 }
 
 static const struct dsa_device_ops ksz9893_netdev_ops = {
@@ -278,7 +330,9 @@ static const struct dsa_device_ops ksz9893_netdev_ops = {
 	.proto	= DSA_TAG_PROTO_KSZ9893,
 	.xmit	= ksz9893_xmit,
 	.rcv	= ksz9477_rcv,
-	.needed_tailroom = KSZ_INGRESS_TAG_LEN,
+	.connect = ksz_connect,
+	.disconnect = ksz_disconnect,
+	.needed_tailroom = KSZ_INGRESS_TAG_LEN + KSZ9477_PTP_TAG_LEN,
 };
 
 DSA_TAG_DRIVER(ksz9893_netdev_ops);
  

Hi Arun,

On Tue, Oct 25, 2022 at 03:41:26AM +0000, Arun.Ramadoss@microchip.com wrote:
> > CONFIG_PTP_1588_CLOCK=m
> > CONFIG_NET_DSA=m
> > CONFIG_NET_DSA_MICROCHIP_KSZ_COMMON=m [ksz_switch.ko]
> > CONFIG_NET_DSA_MICROCHIP_KSZ9477_I2C=m
> > CONFIG_NET_DSA_MICROCHIP_KSZ_PTP=y [builtin]
> > 
> > With this configuration, kbuild doesn't even try to compile
> > ksz_ptp.c:
> > 
> > ERROR: modpost: "ksz_hwtstamp_get"
> > [drivers/net/dsa/microchip/ksz_switch.ko] undefined!
> > ERROR: modpost: "ksz_hwtstamp_set"
> > [drivers/net/dsa/microchip/ksz_switch.ko] undefined!
> > ERROR: modpost: "ksz_port_txtstamp"
> > [drivers/net/dsa/microchip/ksz_switch.ko] undefined!
> > ERROR: modpost: "ksz_ptp_clock_register"
> > [drivers/net/dsa/microchip/ksz_switch.ko] undefined!
> > ERROR: modpost: "ksz_port_deferred_xmit"
> > [drivers/net/dsa/microchip/ksz_switch.ko] undefined!
> > ERROR: modpost: "ksz_ptp_clock_unregister"
> > [drivers/net/dsa/microchip/ksz_switch.ko] undefined!
> > ERROR: modpost: "ksz_ptp_irq_free"
> > [drivers/net/dsa/microchip/ksz_switch.ko] undefined!
> > ERROR: modpost: "ksz_tstamp_reconstruct"
> > [drivers/net/dsa/microchip/ksz_switch.ko] undefined!
> > ERROR: modpost: "ksz_get_ts_info"
> > [drivers/net/dsa/microchip/ksz_switch.ko] undefined!
> > ERROR: modpost: "ksz_ptp_irq_setup"
> > [drivers/net/dsa/microchip/ksz_switch.ko] undefined!
> > 
> > After setting all of the above to 'y', the build process works (but
> > I would prefer being able to build as modules). 
> 
> May be this is due to kconfig of config_ksz_ptp  defined bool instead
> of tristate. Do I need to change the config_ksz_ptp to tristate in
> order to compile as modules?

You don't want a separate kernel module for PTP support, so no, you
don't want to make CONFIG_NET_DSA_MICROCHIP_KSZ_PTP tristate.

But what you want is for the ksz_ptp.o object file to be included into
ksz_switch-objs when CONFIG_NET_DSA_MICROCHIP_KSZ_PTP is enabled.

See how sja1105 does it:

ifdef CONFIG_NET_DSA_SJA1105_PTP
sja1105-objs += sja1105_ptp.o
endif

You'll also want to make NET_DSA_MICROCHIP_KSZ9477_I2C and
NET_DSA_MICROCHIP_KSZ_SPI to depend on PTP_1588_CLOCK_OPTIONAL, because
CONFIG_PTP_1588_CLOCK can be compiled as module (as Christian shows),
and in that case, you'll want the KSZ drivers to also be built as
modules (otherwise they're built-in, and built-in code cannot depend on
symbols exported from modules, because the modules may never be inserted).

It's best to actually experiment with this, you cannot get it right if
you don't experiment.
  

On Wed, Oct 26, 2022 at 06:47:53PM +0200, Christian Eggers wrote:
> Hi Arun, hi Vladimir,
> 
> On Tuesday, 18 October 2022, 15:42:41 CEST, Arun.Ramadoss@microchip.com wrote:
> > ...
> > Thanks Vladimir. I will wait for Christian feedback.
> >
> > Hi Christian,
> > To test this patch on KSZ9563, we need to increase the number of
> > interrupts port_nirqs in KSZ9893 from 2 to 3. Since the chip id of
> > KSZ9893 and KSZ9563 are same, I had reused the ksz_chip_data same for
> > both chips. But this chip differ with number of port interrupts. So we
> > need to update it. We are generating a new patch for adding the new
> > element in the ksz_chip_data for KSZ9563.
> > For now, you can update the code as below for testing the patch
> 
> today I hard first success with your patch series on KSZ9563! ptp4l reported
> delay measurements between switch port 1 and the connected Meinberg clock:
> 
> > ptp4l[75.590]: port 2: new foreign master ec4670.fffe.0a9dcc-1
> > ptp4l[79.590]: selected best master clock ec4670.fffe.0a9dcc
> > ptp4l[79.590]: updating UTC offset to 37
> > ptp4l[79.591]: port 2: LISTENING to UNCALIBRATED on RS_SLAVE
> > ptp4l[81.114]: port 2: delay timeout
> > ptp4l[81.117]: delay   filtered        338   raw        338
> > ptp4l[81.118]: port 2: minimum delay request interval 2^1
> > ptp4l[81.434]: port 1: announce timeout
> > ptp4l[81.434]: config item lan0.udp_ttl is 1
> > ptp4l[81.436]: config item (null).dscp_event is 0
> > ptp4l[81.437]: config item (null).dscp_general is 0
> > ptp4l[81.437]: selected best master clock ec4670.fffe.0a9dcc
> > ptp4l[81.438]: updating UTC offset to 37
> > ptp4l[81.843]: master offset         33 s0 freq   +6937 path delay       338
> > ptp4l[82.844]: master offset         26 s2 freq   +6930 path delay       338
> > ptp4l[82.844]: port 2: UNCALIBRATED to SLAVE on MASTER_CLOCK_SELECTED
> > ptp4l[83.844]: master offset         32 s2 freq   +6962 path delay       338
> > ptp4l[84.844]: master offset          3 s2 freq   +6943 path delay       338
> > ptp4l[85.844]: master offset        -14 s2 freq   +6927 path delay       338
> > ptp4l[86.042]: port 2: delay timeout
> > ptp4l[86.045]: delay   filtered        336   raw        335
> > ptp4l[86.211]: port 2: delay timeout
> > ptp4l[86.213]: delay   filtered        335   raw        331
> > ptp4l[86.844]: master offset          3 s2 freq   +6939 path delay       335
> > ptp4l[87.847]: master offset         -7 s2 freq   +6930 path delay       335
> 
> As a next step I'll try to configure the external output for 1PPS. Is this
> already implemented in your patches? The file /sys/class/ptp/ptp2/n_periodic_outputs
> shows '0' on my system.

Arun didn't share the PPS output patch publicly, so I don't know why
we're discussing this here. Anyway, in it, Arun (incorrectly)
implemented support for PTP_CLK_REQ_PPS rather than PTP_CLK_REQ_PEROUT,
so there will not be any n_periodic_outputs visible in sysfs. For now,
try via pps_available and pps_enable.

> 
> BTW: Which is the preferred delay measurement which I shall test (E2E/P2P)?

As this time around there is somebody from Microchip finally on the
line, I will not interfere in this part. I tried once, and failed to
understand the KSZ PTP philosophy. I hope you get some answers from
Arun. Just one question below.

> I started with E2E is this was configured in the hardware and needs no 1-step
> time stamping, but I had to add PTP_MSGTYPE_DELAY_REQ in ksz_port_txtstamp().

Hm? So if E2E "doesn't need" 1-step TX timestamping and KSZ9563 doesn't
support 2-step TX timestamping, then what kind of TX timestamping is
used here for Delay_Req messages?

Perhaps you mean that E2E doesn't need moving the RX timestamp of the
Pdelay_Req (t2) into the KSZ TX timestamp trailer of the Pdelay_Resp (t3)?

> > May be this is due to kconfig of config_ksz_ptp  defined bool instead
> > of tristate. Do I need to change the config_ksz_ptp to tristate in
> > order to compile as modules?
> 
> I'm not an expert for kbuild and cannot tell whether it's allowed to use
> bool options which depend on tristate options. At least ksz_ptp.c is compiled
> by kbuild if tristate is used. But I needed to add additional EXPORT_SYMBOL()
> statements for all non-static functions (see below) for successful linking.

If ksz_ptp.o gets linked into ksz_ptp.ko, then yes. But this probably
doesn't make sense, as you point out. So EXPORT_SYMBOL() should not be
needed.

> I'm unsure whether it makes sense to build ksz_ptp as a separate module.
> Perhaps it should be (conditionally) added to ksz_switch.ko.
> 
> On Tuesday, 18 October 2022, 08:44:04 CEST, Arun.Ramadoss@microchip.com wrote:
> > I had developed this patch set to add gPTP support for LAN937x based on
> > the Christian eggers patch for KSZ9563.
> 
> Maybe this could be mentioned somewhere (e.g. extra line in file header of
> ksz_ptp.c).  It took a lot of effort (for me) to get this initially running
> (e.g. due to limited documentation / support by Microchip).  But I'm quite happy
> that this is continued now as it is likely that I'll need PTP support for the
> KSZ9563 soon.
> 
> For KSZ9563, we will need support for 1-step time stamping as two-step
> is not possible.
> 
> I've stashed all my local changes into an additional patch (see below).
> Please feel free to integrate this into your series.  As soon I get 1PPS
> running, I'll continue testing.  Note that I'll be unavailable between Friday
> and next Tuesday.
> 
> regards,
> Christian
>  static int ksz_set_hwtstamp_config(struct ksz_device *dev, int port,
>  				   struct hwtstamp_config *config)
> @@ -106,7 +108,7 @@ static int ksz_set_hwtstamp_config(struct ksz_device *dev, int port,
>  	case HWTSTAMP_TX_OFF:
>  		prt->hwts_tx_en = false;
>  		break;
> -	case HWTSTAMP_TX_ON:
> +	case HWTSTAMP_TX_ONESTEP_P2P:

One shouldn't replace the other; this implementation is simplistic, of course.

Also, why did you choose HWTSTAMP_TX_ONESTEP_P2P and not HWTSTAMP_TX_ONESTEP_SYNC?

>  		prt->hwts_tx_en = true;
>  		break;
>  	default:
> @@ -162,6 +164,7 @@ int ksz_hwtstamp_set(struct dsa_switch *ds, int port, struct ifreq *ifr)
>  	mutex_unlock(&ptp_data->lock);
>  	return ret;
>  }
> +EXPORT_SYMBOL(ksz_hwtstamp_set);
> diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c
> index 582add3398d3..e7680718b478 100644
> --- a/net/dsa/tag_ksz.c
> +++ b/net/dsa/tag_ksz.c
> @@ -251,17 +251,69 @@ MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_KSZ9477);
>  #define KSZ9893_TAIL_TAG_OVERRIDE	BIT(5)
>  #define KSZ9893_TAIL_TAG_LOOKUP		BIT(6)
> 
> +/* Time stamp tag is only inserted if PTP is enabled in hardware. */
> +static void ksz9893_xmit_timestamp(struct sk_buff *skb)
> +{
> +//	struct sk_buff *clone = KSZ9477_SKB_CB(skb)->clone;
> +//	struct ptp_header *ptp_hdr;
> +//	unsigned int ptp_type;
> +	u32 tstamp_raw = 0;
> +	put_unaligned_be32(tstamp_raw, skb_put(skb, KSZ9477_PTP_TAG_LEN));
> +}

This is needed for one-step TX timestamping, ok.

> +
> +/* Defer transmit if waiting for egress time stamp is required.  */
> +static struct sk_buff *ksz9893_defer_xmit(struct dsa_port *dp,
> +					  struct sk_buff *skb)

No need to duplicate, can rename lan937x_defer_xmit() and call that.

Although I'm not exactly clear *which* packets will need deferred
transmission on ksz9xxx. To my knowledge, such a procedure is only
necessary for 2-step TX timestamping, when the TX timestamp must be
propagated back to the socket error queue via skb_complete_tx_timestamp().
For one-step, AFAIK*, this isn't needed.

This is not used, right? Because the function call is shortcircuited by
the "if (test_bit(KSZ_HWTS_EN, &priv->state))" test earlier.

*Or is this intended to be used for the "Software Two-Step Simulation
Mode in hardware 1-Step Mode" that was suggested in the errata sheet,
where one-step Sync messages still get their TX timestamp reported to
user space as if they were two-step?
http://ww1.microchip.com/downloads/en/DeviceDoc/KSZ9563R-Errata-80000786B.pdf
  

Hi Christian,
On Thu, 2022-10-27 at 00:44 +0300, Vladimir Oltean wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On Wed, Oct 26, 2022 at 06:47:53PM +0200, Christian Eggers wrote:
> > Hi Arun, hi Vladimir,
> > 
> > On Tuesday, 18 October 2022, 15:42:41 CEST, 
> > Arun.Ramadoss@microchip.com wrote:
> > > ...
> > > Thanks Vladimir. I will wait for Christian feedback.
> > > 
> > > Hi Christian,
> > > To test this patch on KSZ9563, we need to increase the number of
> > > interrupts port_nirqs in KSZ9893 from 2 to 3. Since the chip id
> > > of
> > > KSZ9893 and KSZ9563 are same, I had reused the ksz_chip_data same
> > > for
> > > both chips. But this chip differ with number of port interrupts.
> > > So we
> > > need to update it. We are generating a new patch for adding the
> > > new
> > > element in the ksz_chip_data for KSZ9563.
> > > For now, you can update the code as below for testing the patch
> > 
> > today I hard first success with your patch series on KSZ9563! ptp4l
> > reported
> > delay measurements between switch port 1 and the connected Meinberg
> > clock:

I tried to bring up the KSZ9563 setup following are my observation
- With this patch series, I am getting the Null pointer exception.
- After applying the patch provided by you, switch probe is successful.

Usually I test the gPTP using the following command
# ptp4l -f ~/ptp4l/gPTP.cfg -i lan1

How did you test this PTP in your setup, so that I can also get the
same result as below.

> > 
> > > ptp4l[75.590]: port 2: new foreign master ec4670.fffe.0a9dcc-1
> > > ptp4l[79.590]: selected best master clock ec4670.fffe.0a9dcc
> > > ptp4l[79.590]: updating UTC offset to 37
> > > ptp4l[79.591]: port 2: LISTENING to UNCALIBRATED on RS_SLAVE
> > > ptp4l[81.114]: port 2: delay timeout
> > > ptp4l[81.117]: delay   filtered        338   raw        338
> > > ptp4l[81.118]: port 2: minimum delay request interval 2^1
> > > ptp4l[81.434]: port 1: announce timeout
> > > ptp4l[81.434]: config item lan0.udp_ttl is 1
> > > ptp4l[81.436]: config item (null).dscp_event is 0
> > > ptp4l[81.437]: config item (null).dscp_general is 0
> > > ptp4l[81.437]: selected best master clock ec4670.fffe.0a9dcc
> > > ptp4l[81.438]: updating UTC offset to 37
> > > ptp4l[81.843]: master offset         33 s0 freq   +6937 path
> > > delay       338
> > > ptp4l[82.844]: master offset         26 s2 freq   +6930 path
> > > delay       338
> > > ptp4l[82.844]: port 2: UNCALIBRATED to SLAVE on
> > > MASTER_CLOCK_SELECTED
> > > ptp4l[83.844]: master offset         32 s2 freq   +6962 path
> > > delay       338
> > > ptp4l[84.844]: master offset          3 s2 freq   +6943 path
> > > delay       338
> > > ptp4l[85.844]: master offset        -14 s2 freq   +6927 path
> > > delay       338
> > > ptp4l[86.042]: port 2: delay timeout
> > > ptp4l[86.045]: delay   filtered        336   raw        335
> > > ptp4l[86.211]: port 2: delay timeout
> > > ptp4l[86.213]: delay   filtered        335   raw        331
> > > ptp4l[86.844]: master offset          3 s2 freq   +6939 path
> > > delay       335
> > > ptp4l[87.847]: master offset         -7 s2 freq   +6930 path
> > > delay       335
> > 
> > As a next step I'll try to configure the external output for 1PPS.
> > Is this
> > already implemented in your patches? The file
> > /sys/class/ptp/ptp2/n_periodic_outputs
> > shows '0' on my system.
> 
> Arun didn't share the PPS output patch publicly, so I don't know why
> we're discussing this here. Anyway, in it, Arun (incorrectly)
> implemented support for PTP_CLK_REQ_PPS rather than
> PTP_CLK_REQ_PEROUT,
> so there will not be any n_periodic_outputs visible in sysfs. For
> now,
> try via pps_available and pps_enable.
> 
> > 
> > BTW: Which is the preferred delay measurement which I shall test
> > (E2E/P2P)?
> 
> As this time around there is somebody from Microchip finally on the
> line, I will not interfere in this part. I tried once, and failed to
> understand the KSZ PTP philosophy. I hope you get some answers from
> Arun. Just one question below.
> 
> > I started with E2E is this was configured in the hardware and needs
> > no 1-step
> > time stamping, but I had to add PTP_MSGTYPE_DELAY_REQ in
> > ksz_port_txtstamp().
> 
> Hm? So if E2E "doesn't need" 1-step TX timestamping and KSZ9563
> doesn't
> support 2-step TX timestamping, then what kind of TX timestamping is
> used here for Delay_Req messages?
> 
> Perhaps you mean that E2E doesn't need moving the RX timestamp of the
> Pdelay_Req (t2) into the KSZ TX timestamp trailer of the Pdelay_Resp
> (t3)?
> 
> > > May be this is due to kconfig of config_ksz_ptp  defined bool
> > > instead
> > > of tristate. Do I need to change the config_ksz_ptp to tristate
> > > in
> > > order to compile as modules?
> > 
> > I'm not an expert for kbuild and cannot tell whether it's allowed
> > to use
> > bool options which depend on tristate options. At least ksz_ptp.c
> > is compiled
> > by kbuild if tristate is used. But I needed to add additional
> > EXPORT_SYMBOL()
> > statements for all non-static functions (see below) for successful
> > linking.
> 
> If ksz_ptp.o gets linked into ksz_ptp.ko, then yes. But this probably
> doesn't make sense, as you point out. So EXPORT_SYMBOL() should not
> be
> needed.
> 
> > I'm unsure whether it makes sense to build ksz_ptp as a separate
> > module.
> > Perhaps it should be (conditionally) added to ksz_switch.ko.
> > 
> > On Tuesday, 18 October 2022, 08:44:04 CEST, 
> > Arun.Ramadoss@microchip.com wrote:
> > > I had developed this patch set to add gPTP support for LAN937x
> > > based on
> > > the Christian eggers patch for KSZ9563.
> > 
> > Maybe this could be mentioned somewhere (e.g. extra line in file
> > header of
> > ksz_ptp.c). 

Sure, I will add it in the File Header in the next version.

> >  It took a lot of effort (for me) to get this initially running
> > (e.g. due to limited documentation / support by Microchip).  But
> > I'm quite happy
> > that this is continued now as it is likely that I'll need PTP
> > support for the
> > KSZ9563 soon.
> > 
> > For KSZ9563, we will need support for 1-step time stamping as two-
> > step
> > is not possible.
> > 
> > I've stashed all my local changes into an additional patch (see
> > below).
> > Please feel free to integrate this into your series.  As soon I get
> > 1PPS
> > running, I'll continue testing. 

I thought 1PPS and periodic output are same, So I sent the 1PPS patch.
I need to look into periodic output.

> >  Note that I'll be unavailable between Friday
> > and next Tuesday.

If you can elaborate the test need to be done in KSZ9563, I can try to
do during your Vacation.

> > 
> > regards,
> > Christian
> >  static int ksz_set_hwtstamp_config(struct ksz_device *dev, int
> > port,
> >                                  struct hwtstamp_config *config)
> > @@ -106,7 +108,7 @@ static int ksz_set_hwtstamp_config(struct
> > ksz_device *dev, int port,
> >       case HWTSTAMP_TX_OFF:
> >               prt->hwts_tx_en = false;
> >               break;
> > -     case HWTSTAMP_TX_ON:
> > +     case HWTSTAMP_TX_ONESTEP_P2P:
> 
> One shouldn't replace the other; this implementation is simplistic,
> of course.
> 
> Also, why did you choose HWTSTAMP_TX_ONESTEP_P2P and not
> HWTSTAMP_TX_ONESTEP_SYNC?
> 
> >               prt->hwts_tx_en = true;
> >               break;
> >       default:
> > @@ -162,6 +164,7 @@ int ksz_hwtstamp_set(struct dsa_switch *ds, int
> > port, struct ifreq *ifr)
> >       mutex_unlock(&ptp_data->lock);
> >       return ret;
> >  }
> > +EXPORT_SYMBOL(ksz_hwtstamp_set);
> > diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c
> > index 582add3398d3..e7680718b478 100644
> > --- a/net/dsa/tag_ksz.c
> > +++ b/net/dsa/tag_ksz.c
> > @@ -251,17 +251,69 @@
> > MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_KSZ9477);
> >  #define KSZ9893_TAIL_TAG_OVERRIDE    BIT(5)
> >  #define KSZ9893_TAIL_TAG_LOOKUP              BIT(6)
> > 
> > +/* Time stamp tag is only inserted if PTP is enabled in hardware.
> > */
> > +static void ksz9893_xmit_timestamp(struct sk_buff *skb)
> > +{
> > +//   struct sk_buff *clone = KSZ9477_SKB_CB(skb)->clone;
> > +//   struct ptp_header *ptp_hdr;
> > +//   unsigned int ptp_type;
> > +     u32 tstamp_raw = 0;
> > +     put_unaligned_be32(tstamp_raw, skb_put(skb,
> > KSZ9477_PTP_TAG_LEN));
> > +}
> 
> This is needed for one-step TX timestamping, ok.
> 
> > +
> > +/* Defer transmit if waiting for egress time stamp is
> > required.  */
> > +static struct sk_buff *ksz9893_defer_xmit(struct dsa_port *dp,
> > +                                       struct sk_buff *skb)
> 
> No need to duplicate, can rename lan937x_defer_xmit() and call that.
> 
> Although I'm not exactly clear *which* packets will need deferred
> transmission on ksz9xxx. To my knowledge, such a procedure is only
> necessary for 2-step TX timestamping, when the TX timestamp must be
> propagated back to the socket error queue via
> skb_complete_tx_timestamp().
> For one-step, AFAIK*, this isn't needed.
> 
> This is not used, right? Because the function call is shortcircuited
> by
> the "if (test_bit(KSZ_HWTS_EN, &priv->state))" test earlier.
> 
> *Or is this intended to be used for the "Software Two-Step Simulation
> Mode in hardware 1-Step Mode" that was suggested in the errata sheet,
> where one-step Sync messages still get their TX timestamp reported to
> user space as if they were two-step?
> 
http://ww1.microchip.com/downloads/en/DeviceDoc/KSZ9563R-Errata-80000786B.pdf
  

Hi Vladimir,

On Wednesday, 26 October 2022, 23:44:55 CEST, Vladimir Oltean wrote:
> Arun didn't share the PPS output patch publicly, so I don't know why
> we're discussing this here. Anyway, in it, Arun (incorrectly)
> implemented support for PTP_CLK_REQ_PPS rather than PTP_CLK_REQ_PEROUT,
> so there will not be any n_periodic_outputs visible in sysfs. For now,
> try via pps_available and pps_enable.
I can continue testing this week.  I can either try with the (incorrect)
PTP_CLK_REQ_PPS or I can try to forward port my earlier patches.

> > BTW: Which is the preferred delay measurement which I shall test (E2E/P2P)?
> 
> As this time around there is somebody from Microchip finally on the
> line, I will not interfere in this part. I tried once, and failed to
> understand the KSZ PTP philosophy. I hope you get some answers from
> Arun. Just one question below.
> 
> > I started with E2E is this was configured in the hardware and needs no 1-step
> > time stamping, but I had to add PTP_MSGTYPE_DELAY_REQ in ksz_port_txtstamp().
> 
> Hm? So if E2E "doesn't need" 1-step TX timestamping and KSZ9563 doesn't
> support 2-step TX timestamping, then what kind of TX timestamping is
> used here for Delay_Req messages?
> Perhaps you mean that E2E doesn't need moving the RX timestamp of the
> Pdelay_Req (t2) into the KSZ TX timestamp trailer of the Pdelay_Resp (t3)?
I think that Delay_Req is not related to 1-step / 2-step time stamping. As far
as I understand, this is only relevant for SYNC and PDelay_Resp.

> > > May be this is due to kconfig of config_ksz_ptp  defined bool instead
> > > of tristate. Do I need to change the config_ksz_ptp to tristate in
> > > order to compile as modules?
> > 
> > I'm not an expert for kbuild and cannot tell whether it's allowed to use
> > bool options which depend on tristate options. At least ksz_ptp.c is compiled
> > by kbuild if tristate is used. But I needed to add additional EXPORT_SYMBOL()
> > statements for all non-static functions (see below) for successful linking.
> 
> If ksz_ptp.o gets linked into ksz_ptp.ko, then yes. But this probably
> doesn't make sense, as you point out. So EXPORT_SYMBOL() should not be
> needed.
> 
> > I'm unsure whether it makes sense to build ksz_ptp as a separate module.
> > Perhaps it should be (conditionally) added to ksz_switch.ko.

So let's conditionally add ksz_ptp.o to ksz_switch.ko.

> >  static int ksz_set_hwtstamp_config(struct ksz_device *dev, int port,
> >  				   struct hwtstamp_config *config)
> > @@ -106,7 +108,7 @@ static int ksz_set_hwtstamp_config(struct ksz_device *dev, int port,
> >  	case HWTSTAMP_TX_OFF:
> >  		prt->hwts_tx_en = false;
> >  		break;
> > -	case HWTSTAMP_TX_ON:
> > +	case HWTSTAMP_TX_ONESTEP_P2P:
> 
> One shouldn't replace the other; this implementation is simplistic, of course.
> 
> Also, why did you choose HWTSTAMP_TX_ONESTEP_P2P and not HWTSTAMP_TX_ONESTEP_SYNC?
Because my (old) ptp4l.conf was configured for P2P+p2p1step.  When I started working on
KSZ9563 PTP two years ago, you suggested doing P2P first, because E2E is affected by nasty
packet filters on the switch hardware...  But for the first tests now I switched to E2E
for the reasons you mentioned above.

Probably HWTSTAMP_TX_ON needs to be rejected for KSZ9563 and only HWTSTAMP_TX_ONESTEP_SYNC
(for E2E) and HWTSTAMP_TX_ONESTEP_P2P (for P2P) should be accepted.

> 
> >  		prt->hwts_tx_en = true;
> >  		break;
> >  	default:
> > @@ -162,6 +164,7 @@ int ksz_hwtstamp_set(struct dsa_switch *ds, int port, struct ifreq *ifr)
> >  	mutex_unlock(&ptp_data->lock);
> >  	return ret;
> >  }
> > +EXPORT_SYMBOL(ksz_hwtstamp_set);
> > diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c
> > index 582add3398d3..e7680718b478 100644
> > --- a/net/dsa/tag_ksz.c
> > +++ b/net/dsa/tag_ksz.c
> > @@ -251,17 +251,69 @@ MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_KSZ9477);
> >  #define KSZ9893_TAIL_TAG_OVERRIDE	BIT(5)
> >  #define KSZ9893_TAIL_TAG_LOOKUP		BIT(6)
> > 
> > +/* Time stamp tag is only inserted if PTP is enabled in hardware. */
> > +static void ksz9893_xmit_timestamp(struct sk_buff *skb)
> > +{
> > +//	struct sk_buff *clone = KSZ9477_SKB_CB(skb)->clone;
> > +//	struct ptp_header *ptp_hdr;
> > +//	unsigned int ptp_type;
> > +	u32 tstamp_raw = 0;
> > +	put_unaligned_be32(tstamp_raw, skb_put(skb, KSZ9477_PTP_TAG_LEN));
> > +}
> 
> This is needed for one-step TX timestamping, ok.
Yes, here is some work left for 1-step PDelay_Resp.

> 
> > +
> > +/* Defer transmit if waiting for egress time stamp is required.  */
> > +static struct sk_buff *ksz9893_defer_xmit(struct dsa_port *dp,
> > +					  struct sk_buff *skb)
> 
> No need to duplicate, can rename lan937x_defer_xmit() and call that.
I wanted only "to make it run", I let the details for Arun.

> 
> Although I'm not exactly clear *which* packets will need deferred
> transmission on ksz9xxx. To my knowledge, such a procedure is only
> necessary for 2-step TX timestamping, when the TX timestamp must be
> propagated back to the socket error queue via skb_complete_tx_timestamp().
> For one-step, AFAIK*, this isn't needed.
As far as I remember, deferred xmit is needed for outgoing time stamps,
which are reported back to the application via the socket's error queue.
Because the KSZ time stamp unit only reports the time stamp itself (but
no sequence number or similar), only a single outgoing packet is allowed
to pass the TS unit at once. Otherwise no mapping between the TS and the
skb would be impossible.

> 
> This is not used, right? Because the function call is shortcircuited by
> the "if (test_bit(KSZ_HWTS_EN, &priv->state))" test earlier.
I am, quite sure that ksz9893_defer_xmit() is actually required.

> 
> *Or is this intended to be used for the "Software Two-Step Simulation
> Mode in hardware 1-Step Mode" that was suggested in the errata sheet,
> where one-step Sync messages still get their TX timestamp reported to
> user space as if they were two-step?
> http://ww1.microchip.com/downloads/en/DeviceDoc/KSZ9563R-Errata-80000786B.pdf
No, I didn't try to got that way.

regards,
Christian
  

Hi Arun,

On Thursday, 27 October 2022, 17:51:54 CEST, Arun.Ramadoss@microchip.com wrote:
> I tried to bring up the KSZ9563 setup following are my observation
> - With this patch series, I am getting the Null pointer exception.
> - After applying the patch provided by you, switch probe is successful.
> 
> Usually I test the gPTP using the following command
> # ptp4l -f ~/ptp4l/gPTP.cfg -i lan1
> 
> How did you test this PTP in your setup, so that I can also get the
> same result as below.

as the KSZ9563 supports no 2-step time stamping, I use 1-step with E2E.
The problem with E2E is, that the "master/slave" filter
on the KSZ9563 filters out some message types.  In order to circumvent
this, I use the KSZ as slave_only for the first tests.  Later I would like
to use P2P, but this requires additional implementation in the driver. 

> > > Maybe this could be mentioned somewhere (e.g. extra line in file
> > > header of
> > > ksz_ptp.c). 
> 
> Sure, I will add it in the File Header in the next version.
thanks.

> 
> I thought 1PPS and periodic output are same, So I sent the 1PPS patch.
> I need to look into periodic output.
I fell the same when I first started with PTP... But maybe you can use
same code for periodic output from my original patches.

regards,
Christian
  

Hi Christian,
I updated the n_per_out code and tested with lan9370 board, it is
working in LED_0 pin.

Is there any testing pending from your side. If no can I generate next
version of patch with your changes which you shared in this thread.

Attached the code for reference

diff --git a/drivers/net/dsa/microchip/ksz_common.h
b/drivers/net/dsa/microchip/ksz_common.h
index 3a640ca4f7e1..f6a1a7acda07 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -472,6 +472,19 @@ static inline int ksz_rmw16(struct ksz_device
*dev, u32 reg, u16 mask,
 	return ret;
 }
 
+static inline int ksz_rmw32(struct ksz_device *dev, u32 reg, u32 mask,
+			    u32 value)
+{
+	int ret;
+
+	ret = regmap_update_bits(dev->regmap[2], reg, mask, value);
+	if (ret)
+		dev_err(dev->dev, "can't rmw 32bit reg: 0x%x %pe\n",
reg,
+			ERR_PTR(ret));
+
+	return ret;
+}
+
 static inline int ksz_write64(struct ksz_device *dev, u32 reg, u64
value)
 {
 	u32 val[2];
diff --git a/drivers/net/dsa/microchip/ksz_ptp.c
b/drivers/net/dsa/microchip/ksz_ptp.c
index d11a490a6c87..54527754a50f 100644
--- a/drivers/net/dsa/microchip/ksz_ptp.c
+++ b/drivers/net/dsa/microchip/ksz_ptp.c
@@ -26,6 +26,147 @@
 
 #define KSZ_PTP_INT_START 13
 
+static int ksz_ptp_restart_perout(struct ksz_device *dev);
+
+static int ksz_ptp_tou_gpio(struct ksz_device *dev)
+{
+	int ret;
+
+	/* Set the Led Override register */
+	ret = ksz_rmw32(dev, REG_SW_GLOBAL_LED_OVR__4, LED_OVR_1,
LED_OVR_1);
+	if (ret)
+		return ret;
+
+	/* Set the Led Source register */
+	return ksz_rmw32(dev, REG_SW_GLOBAL_LED_SRC__4,
LED_SRC_PTP_GPIO_1,
+			 LED_SRC_PTP_GPIO_1);
+}
+
+/* Shared register access routines (Trigger Output Unit) */
+static int ksz_ptp_tou_reset(struct ksz_device *dev, unsigned int
unit)
+{
+	u32 data;
+	int ret;
+
+	/* Reset trigger unit (clears TRIGGER_EN, but not GPIOSTATx) */
+	ret = ksz_rmw32(dev, REG_PTP_CTRL_STAT__4, TRIG_RESET,
TRIG_RESET);
+
+	/* Clear DONE */
+	data = 1 << (unit + TRIG_DONE_S);
+	ret = ksz_write32(dev, REG_PTP_TRIG_STATUS__4, data);
+	if (ret)
+		return ret;
+
+	/* Clear IRQ */
+	data = 1 << (unit + TRIG_INT_S);
+	ret = ksz_write32(dev, REG_PTP_INT_STATUS__4, data);
+	if (ret)
+		return ret;
+
+	/* Clear reset and set GPIO direction */
+	return ksz_rmw32(dev, REG_PTP_CTRL_STAT__4, (TRIG_RESET |
TRIG_ENABLE),
+			 0);
+}
+
+static int ksz_ptp_tou_cycle_width_set(struct ksz_device *dev, u32
width_ns)
+{
+	int ret;
+
+	ret = ksz_write32(dev, REG_TRIG_CYCLE_WIDTH, width_ns);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int ksz_ptp_tou_cycle_count_set(struct ksz_device *dev, u16
count)
+{
+	u32 data;
+	int ret;
+
+	ret = ksz_read32(dev, REG_TRIG_CYCLE_CNT, &data);
+	if (ret)
+		return ret;
+
+	data &= ~(TRIG_CYCLE_CNT_M << TRIG_CYCLE_CNT_S);
+	data |= (count & TRIG_CYCLE_CNT_M) << TRIG_CYCLE_CNT_S;
+
+	ret = ksz_write32(dev, REG_TRIG_CYCLE_CNT, data);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int ksz_ptp_tou_pulse_verify(u64 pulse_ns)
+{
+	u32 data;
+
+	if (pulse_ns & 0x3)
+		return -EINVAL;
+
+	data = (pulse_ns / 8);
+	if (data != (data & TRIG_PULSE_WIDTH_M))
+		return -ERANGE;
+
+	return 0;
+}
+
+static int ksz_ptp_tou_pulse_set(struct ksz_device *dev, u32 pulse_ns)
+{
+	u32 data;
+
+	data = (pulse_ns / 8);
+
+	return ksz_write32(dev, REG_TRIG_PULSE_WIDTH__4, data);
+}
+
+static int ksz_ptp_tou_target_time_set(struct ksz_device *dev,
+				       struct timespec64 const *ts)
+{
+	int ret;
+
+	/* Hardware has only 32 bit */
+	if ((ts->tv_sec & 0xffffffff) != ts->tv_sec)
+		return -EINVAL;
+
+	ret = ksz_write32(dev, REG_TRIG_TARGET_NANOSEC, ts->tv_nsec);
+	if (ret)
+		return ret;
+
+	ret = ksz_write32(dev, REG_TRIG_TARGET_SEC, ts->tv_sec);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int ksz_ptp_tou_start(struct ksz_device *dev, u32 *ctrl_stat_)
+{
+	u32 ctrl_stat;
+	int ret;
+
+	/* Configure GPIO pins */
+	ret = ksz_ptp_tou_gpio(dev);
+	if (ret)
+		return ret;
+
+	ret = ksz_read32(dev, REG_PTP_CTRL_STAT__4, &ctrl_stat);
+	if (ret)
+		return ret;
+
+	ctrl_stat |= (TRIG_ENABLE | GPIO_OUT);
+
+	ret = ksz_write32(dev, REG_PTP_CTRL_STAT__4, ctrl_stat);
+	if (ret)
+		return ret;
+
+	if (ctrl_stat_)
+		*ctrl_stat_ = ctrl_stat;
+
+	return 0;
+}
+
 static int ksz_ptp_enable_mode(struct ksz_device *dev, bool enable)
 {
 	struct ksz_ptp_data *ptp_data = &dev->ptp_data;
@@ -277,6 +418,20 @@ static int ksz_ptp_settime(struct ptp_clock_info
*ptp,
 	/* Load PTP clock from shadow registers */
 	ret = ksz_rmw16(dev, REG_PTP_CLK_CTRL, PTP_LOAD_TIME,
PTP_LOAD_TIME);
 
+	switch (ptp_data->tou_mode) {
+	case KSZ_PTP_TOU_IDLE:
+		break;
+
+	case KSZ_PTP_TOU_PEROUT:
+		dev_info(dev->dev, "Restarting periodic output
signal\n");
+
+		ret = ksz_ptp_restart_perout(dev);
+		if (ret)
+			goto error_return;
+
+		break;
+	}
+
 	spin_lock_bh(&ptp_data->clock_lock);
 	ptp_data->clock_time = *ts;
 	spin_unlock_bh(&ptp_data->clock_lock);
@@ -368,6 +523,20 @@ static int ksz_ptp_adjtime(struct ptp_clock_info
*ptp, s64 delta)
 
 	ret = ksz_write16(dev, REG_PTP_CLK_CTRL, data16);
 
+	switch (ptp_data->tou_mode) {
+	case KSZ_PTP_TOU_IDLE:
+		break;
+
+	case KSZ_PTP_TOU_PEROUT:
+		dev_info(dev->dev, "Restarting periodic output
signal\n");
+
+		ret = ksz_ptp_restart_perout(dev);
+		if (ret)
+			goto error_return;
+
+		break;
+	}
+
 	spin_lock_bh(&ptp_data->clock_lock);
 	ptp_data->clock_time = timespec64_add(ptp_data->clock_time,
delta64);
 	spin_unlock_bh(&ptp_data->clock_lock);
@@ -377,6 +546,201 @@ static int ksz_ptp_adjtime(struct ptp_clock_info
*ptp, s64 delta)
 	return ret;
 }
 
+static int ksz_ptp_configure_perout(struct ksz_device *dev,
+				    u32 cycle_width_ns, u16
cycle_count,
+				    u32 pulse_width_ns,
+				    struct timespec64 const
*target_time)
+{
+	u32 trig_ctrl;
+	int ret;
+
+	/* Enable notify, set rising edge, set periodic pattern */
+	trig_ctrl = TRIG_NOTIFY | (TRIG_POS_PERIOD << TRIG_PATTERN_S);
+	ret = ksz_write32(dev, REG_TRIG_CTRL__4, trig_ctrl);
+	if (ret)
+		return ret;
+
+	ret = ksz_ptp_tou_cycle_width_set(dev, cycle_width_ns);
+	if (ret)
+		return ret;
+
+	ksz_ptp_tou_cycle_count_set(dev, cycle_count);
+	if (ret)
+		return ret;
+
+	ret = ksz_ptp_tou_pulse_set(dev, pulse_width_ns);
+	if (ret)
+		return ret;
+
+	ret = ksz_ptp_tou_target_time_set(dev, target_time);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+#define KSZ_PEROUT_VALID_FLAGS ( \
+				 PTP_PEROUT_DUTY_CYCLE \
+				 )
+
+static int ksz_ptp_enable_perout(struct ksz_device *dev,
+				 struct ptp_perout_request const
*perout_request,
+				 int on)
+{
+	struct ksz_ptp_data *ptp_data = &dev->ptp_data;
+	u64 cycle_width_ns;
+	u64 pulse_width_ns;
+	u32 gpio_stat0;
+	int ret;
+
+	if (perout_request->flags & ~KSZ_PEROUT_VALID_FLAGS)
+		return -EINVAL;
+
+	if (ptp_data->tou_mode != KSZ_PTP_TOU_PEROUT &&
+	    ptp_data->tou_mode != KSZ_PTP_TOU_IDLE)
+		return -EBUSY;
+
+	ret = ksz_ptp_tou_reset(dev, 0);
+	if (ret)
+		return ret;
+
+	if (!on) {
+		ptp_data->tou_mode = KSZ_PTP_TOU_IDLE;
+		return 0;  /* success */
+	}
+
+	ptp_data->perout_target_time_first.tv_sec  = perout_request-
>start.sec;
+	ptp_data->perout_target_time_first.tv_nsec = perout_request-
>start.nsec;
+
+	ptp_data->perout_period.tv_sec = perout_request->period.sec;
+	ptp_data->perout_period.tv_nsec = perout_request->period.nsec;
+
+	cycle_width_ns = timespec64_to_ns(&ptp_data->perout_period);
+	if ((cycle_width_ns & GENMASK(31, 0)) != cycle_width_ns)
+		return -EINVAL;
+
+	if (perout_request->flags & PTP_PEROUT_DUTY_CYCLE)
+		pulse_width_ns = perout_request->on.sec * NSEC_PER_SEC
+
+			perout_request->on.nsec;
+
+	else
+		/* Use a duty cycle of 50%. Maximum pulse width
supported by the
+		 * hardware is a little bit more than 125 ms.
+		 */
+		pulse_width_ns = min_t(u64,
+				       (perout_request->period.sec *
NSEC_PER_SEC
+					+ perout_request->period.nsec)
/ 2
+				       / 8 * 8,
+				       125000000LL);
+
+	ret = ksz_ptp_tou_pulse_verify(pulse_width_ns);
+	if (ret)
+		return ret;
+
+	ret = ksz_ptp_configure_perout(dev, cycle_width_ns,
+				       0,
+				       pulse_width_ns,
+				       &ptp_data-
>perout_target_time_first);
+	if (ret)
+		return ret;
+
+	/* Activate trigger unit */
+	ret = ksz_ptp_tou_start(dev, NULL);
+	if (ret)
+		return ret;
+
+	/* Check error flag:
+	 * - the ACTIVE flag is NOT cleared an error!
+	 */
+	ret = ksz_read32(dev, REG_PTP_TRIG_STATUS__4, &gpio_stat0);
+	if (ret)
+		return ret;
+
+	if (gpio_stat0 & (1 << (0 + TRIG_ERROR_S))) {
+		dev_err(dev->dev, "%s: Trigger unit0 error!\n",
__func__);
+		ret = -EIO;
+		/* Unit will be reset on next access */
+		return ret;
+	}
+
+	ptp_data->tou_mode = KSZ_PTP_TOU_PEROUT;
+
+	return 0;
+}
+
+static int ksz_ptp_restart_perout(struct ksz_device *dev)
+{
+	struct ksz_ptp_data *ptp_data = &dev->ptp_data;
+	s64 now_ns, first_ns, period_ns, next_ns;
+	struct timespec64 now;
+	unsigned int count;
+	int ret;
+
+	ret = _ksz_ptp_gettime(dev, &now);
+	if (ret)
+		return ret;
+
+	now_ns = timespec64_to_ns(&now);
+	first_ns = timespec64_to_ns(&ptp_data-
>perout_target_time_first);
+
+	/* Calculate next perout event based on start time and period
*/
+	period_ns = timespec64_to_ns(&ptp_data->perout_period);
+
+	if (first_ns < now_ns) {
+		count = div_u64(now_ns - first_ns, period_ns);
+		next_ns = first_ns + count * period_ns;
+	} else {
+		next_ns = first_ns;
+	}
+
+	/* Ensure 100 ms guard time prior next event */
+	while (next_ns < now_ns + 100000000)
+		next_ns += period_ns;
+
+	/* Restart periodic output signal */
+	{
+		struct timespec64 next = ns_to_timespec64(next_ns);
+		struct ptp_perout_request perout_request = {
+			.start = {
+				.sec  = next.tv_sec,
+				.nsec = next.tv_nsec
+			},
+			.period = {
+				.sec  = ptp_data->perout_period.tv_sec,
+				.nsec = ptp_data->perout_period.tv_nsec
+			},
+			.index = 0,
+			.flags = 0,  /* keep current values */
+		};
+		ret = ksz_ptp_enable_perout(dev, &perout_request, 1);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int ksz_ptp_enable(struct ptp_clock_info *ptp,
+			  struct ptp_clock_request *req, int on)
+{
+	struct ksz_ptp_data *ptp_data = ptp_caps_to_data(ptp);
+	struct ksz_device *dev = ptp_data_to_ksz_dev(ptp_data);
+	struct ptp_perout_request *perout_request = &req->perout;
+	int ret;
+
+	switch (req->type) {
+	case PTP_CLK_REQ_PEROUT:
+		mutex_lock(&ptp_data->lock);
+		ret = ksz_ptp_enable_perout(dev, perout_request, on);
+		mutex_unlock(&ptp_data->lock);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return ret;
+}
+
 /*  Function is pointer to the do_aux_work in the ptp_clock capability
*/
 static long ksz_ptp_do_aux_work(struct ptp_clock_info *ptp)
 {
@@ -489,6 +853,8 @@ static const struct ptp_clock_info ksz_ptp_caps = {
 	.adjfine	= ksz_ptp_adjfine,
 	.adjtime	= ksz_ptp_adjtime,
 	.do_aux_work	= ksz_ptp_do_aux_work,
+	.enable		= ksz_ptp_enable,
+	.n_per_out	= 1,
 };
 
 int ksz_ptp_clock_register(struct dsa_switch *ds)
diff --git a/drivers/net/dsa/microchip/ksz_ptp.h
b/drivers/net/dsa/microchip/ksz_ptp.h
index b2035a0bcbb2..160c05e573bd 100644
--- a/drivers/net/dsa/microchip/ksz_ptp.h
+++ b/drivers/net/dsa/microchip/ksz_ptp.h
@@ -8,6 +8,11 @@
 
 #if IS_ENABLED(CONFIG_NET_DSA_MICROCHIP_KSZ_PTP)
 
+enum ksz_ptp_tou_mode {
+	KSZ_PTP_TOU_IDLE,
+	KSZ_PTP_TOU_PEROUT,
+};
+
 struct ksz_ptp_data {
 	struct ptp_clock_info caps;
 	struct ptp_clock *clock;
@@ -18,6 +23,9 @@ struct ksz_ptp_data {
 	/* lock for accessing the clock_time */
 	spinlock_t clock_lock;
 	struct timespec64 clock_time;
+	enum ksz_ptp_tou_mode tou_mode;
+	struct timespec64 perout_target_time_first;  /* start of first
perout pulse */
+	struct timespec64 perout_period;
 };
 
 int ksz_ptp_clock_register(struct dsa_switch *ds);
diff --git a/drivers/net/dsa/microchip/ksz_ptp_reg.h
b/drivers/net/dsa/microchip/ksz_ptp_reg.h
index 2ae6c8b01b00..0f391a01a7f7 100644
--- a/drivers/net/dsa/microchip/ksz_ptp_reg.h
+++ b/drivers/net/dsa/microchip/ksz_ptp_reg.h
@@ -3,6 +3,14 @@
  * Copyright (C) 2019-2021 Microchip Technology Inc.
  */
 
+#define REG_SW_GLOBAL_LED_OVR__4	0x0120
+#define LED_OVR_2			BIT(1)
+#define LED_OVR_1			BIT(0)
+
+#define REG_SW_GLOBAL_LED_SRC__4	0x0128
+#define LED_SRC_PTP_GPIO_1		BIT(3)
+#define LED_SRC_PTP_GPIO_2		BIT(2)
+
 /* 5 - PTP Clock */
 #define REG_PTP_CLK_CTRL		0x0500
 
@@ -51,6 +59,77 @@
 #define PTP_MASTER			BIT(1)
 #define PTP_1STEP			BIT(0)
 
+#define REG_PTP_UNIT_INDEX__4          0x0520
+
+#define PTP_UNIT_M                     0xF
+
+#define PTP_GPIO_INDEX_S               16
+#define PTP_TSI_INDEX_S                8
+#define PTP_TOU_INDEX_S                0
+
+#define REG_PTP_TRIG_STATUS__4		0x0524
+
+#define TRIG_ERROR_S			16
+#define TRIG_DONE_S			0
+
+#define REG_PTP_INT_STATUS__4		0x0528
+
+#define TRIG_INT_S			16
+#define TS_INT_S			0
+
+#define TRIG_UNIT_M			0x7
+#define TS_UNIT_M			0x3
+
+#define REG_PTP_CTRL_STAT__4           0x052C
+
+#define GPIO_IN                        BIT(7)
+#define GPIO_OUT                       BIT(6)
+#define TS_INT_ENABLE                  BIT(5)
+#define TRIG_ACTIVE                    BIT(4)
+#define TRIG_ENABLE                    BIT(3)
+#define TRIG_RESET                     BIT(2)
+#define TS_ENABLE                      BIT(1)
+#define TS_RESET                       BIT(0)
+
+#define REG_TRIG_TARGET_NANOSEC        0x0530
+#define REG_TRIG_TARGET_SEC            0x0534
+
+#define REG_TRIG_CTRL__4               0x0538
+
+#define TRIG_CASCADE_ENABLE            BIT(31)
+#define TRIG_CASCADE_TAIL              BIT(30)
+#define TRIG_CASCADE_UPS_M             0xF
+#define TRIG_CASCADE_UPS_S             26
+#define TRIG_NOW                       BIT(25)
+#define TRIG_NOTIFY                    BIT(24)
+#define TRIG_EDGE                      BIT(23)
+#define TRIG_PATTERN_S                 20
+#define TRIG_PATTERN_M                 0x7
+#define TRIG_NEG_EDGE                  0
+#define TRIG_POS_EDGE                  1
+#define TRIG_NEG_PULSE                 2
+#define TRIG_POS_PULSE                 3
+#define TRIG_NEG_PERIOD                4
+#define TRIG_POS_PERIOD                5
+#define TRIG_REG_OUTPUT                6
+#define TRIG_GPO_S                     16
+#define TRIG_GPO_M                     0xF
+#define TRIG_CASCADE_ITERATE_CNT_M     0xFFFF
+
+#define REG_TRIG_CYCLE_WIDTH           0x053C
+
+#define REG_TRIG_CYCLE_CNT             0x0540
+
+#define TRIG_CYCLE_CNT_M               0xFFFF
+#define TRIG_CYCLE_CNT_S               16
+#define TRIG_BIT_PATTERN_M             0xFFFF
+
+#define REG_TRIG_ITERATE_TIME          0x0544
+
+#define REG_TRIG_PULSE_WIDTH__4        0x0548
+
+#define TRIG_PULSE_WIDTH_M             0x00FFFFFF
+
 /* Port PTP Register */
 #define REG_PTP_PORT_RX_DELAY__2	0x0C00
 #define REG_PTP_PORT_TX_DELAY__2	0x0C02

On Wed, 2022-11-02 at 14:11 +0100, Christian Eggers wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> Hi Vladimir,
> 
> On Wednesday, 26 October 2022, 23:44:55 CEST, Vladimir Oltean wrote:
> > Arun didn't share the PPS output patch publicly, so I don't know
> why
> > we're discussing this here. Anyway, in it, Arun (incorrectly)
> > implemented support for PTP_CLK_REQ_PPS rather than
> PTP_CLK_REQ_PEROUT,
> > so there will not be any n_periodic_outputs visible in sysfs. For
> now,
> > try via pps_available and pps_enable.
> I can continue testing this week. I can either try with the
> (incorrect)
> PTP_CLK_REQ_PPS or I can try to forward port my earlier patches.
> 
> > > BTW: Which is the preferred delay measurement which I shall test
> (E2E/P2P)?
> > 
> > As this time around there is somebody from Microchip finally on the
> > line, I will not interfere in this part. I tried once, and failed
> to
> > understand the KSZ PTP philosophy. I hope you get some answers from
> > Arun. Just one question below.
> > 
> > > I started with E2E is this was configured in the hardware and
> needs no 1-step
> > > time stamping, but I had to add PTP_MSGTYPE_DELAY_REQ in
> ksz_port_txtstamp().
> > 
> > Hm? So if E2E "doesn't need" 1-step TX timestamping and KSZ9563
> doesn't
> > support 2-step TX timestamping, then what kind of TX timestamping
> is
> > used here for Delay_Req messages?
> > Perhaps you mean that E2E doesn't need moving the RX timestamp of
> the
> > Pdelay_Req (t2) into the KSZ TX timestamp trailer of the
> Pdelay_Resp (t3)?
> I think that Delay_Req is not related to 1-step / 2-step time
> stamping. As far
> as I understand, this is only relevant for SYNC and PDelay_Resp.
> 
> > > > May be this is due to kconfig of config_ksz_ptp defined bool
> instead
> > > > of tristate. Do I need to change the config_ksz_ptp to tristate
> in
> > > > order to compile as modules?
> > > 
> > > I'm not an expert for kbuild and cannot tell whether it's allowed
> to use
> > > bool options which depend on tristate options. At least ksz_ptp.c
> is compiled
> > > by kbuild if tristate is used. But I needed to add additional
> EXPORT_SYMBOL()
> > > statements for all non-static functions (see below) for
> successful linking.
> > 
> > If ksz_ptp.o gets linked into ksz_ptp.ko, then yes. But this
> probably
> > doesn't make sense, as you point out. So EXPORT_SYMBOL() should not
> be
> > needed.
> > 
> > > I'm unsure whether it makes sense to build ksz_ptp as a separate
> module.
> > > Perhaps it should be (conditionally) added to ksz_switch.ko.
> 
> So let's conditionally add ksz_ptp.o to ksz_switch.ko.
> 
> > > static int ksz_set_hwtstamp_config(struct ksz_device *dev, int
> port,
> > > struct hwtstamp_config *config)
> > > @@ -106,7 +108,7 @@ static int ksz_set_hwtstamp_config(struct
> ksz_device *dev, int port,
> > > case HWTSTAMP_TX_OFF:
> > > prt->hwts_tx_en = false;
> > > break;
> > > - case HWTSTAMP_TX_ON:
> > > + case HWTSTAMP_TX_ONESTEP_P2P:
> > 
> > One shouldn't replace the other; this implementation is simplistic,
> of course.
> > 
> > Also, why did you choose HWTSTAMP_TX_ONESTEP_P2P and not
> HWTSTAMP_TX_ONESTEP_SYNC?
> Because my (old) ptp4l.conf was configured for P2P+p2p1step. When I
> started working on
> KSZ9563 PTP two years ago, you suggested doing P2P first, because E2E
> is affected by nasty
> packet filters on the switch hardware... But for the first tests now
> I switched to E2E
> for the reasons you mentioned above.
> 
> Probably HWTSTAMP_TX_ON needs to be rejected for KSZ9563 and only
> HWTSTAMP_TX_ONESTEP_SYNC
>