Re: [PATCH net-next] misc: Add Renesas Synchronization Management Unit (SMU) support

Re: [PATCH net-next] misc: Add Renesas Synchronization Management Unit (SMU) support

[Arnd Bergmann]

On Fri, Feb 12, 2021 at 2:40 AM Min Li <min.li.xe@renesas.com> wrote:
> > There should probably be a description of the purpose of the hardware both
> > here and in the patch description.
> >
> > In particular, please explain how it relates to the existing clockmatrix driver.
>
> I just uploaded v2 patch to provide more background info for this change.

It appears that you accidentally dropped the Cc list, adding them back in the
reply.  Also adding the PTP maintainer, as this clearly needs to be reviewed
in the context of the PTP subsystem. Please keep Richard and the netdev
list on Cc in future submissions.

> This driver is developed to be only used by Renesas PTP Clock Manager for
> Linux (pcm4l) software.
>
> This driver supports 1588/PTP releated functionalities that
> specific to Renesas devices but are not supported by general PHC framework.
> So pcm4l will use both the existing PHC driver and this driver to complete
> 1588/PTP support.

Ah, so if this is for a PTP related driver, it should probably be integrated
into the PTP subsystem rather than being a separate class.

> > A pure list of register values seems neither particular portable nor intuitive.
> > How is a user expected to interpret these, and are you sure that any driver
> > for this class would have the same interpretation at the same register index?
> >
>
> Yes we need a way to dump register values when remote debugging with customers.
> And all the Renesas SMU has similar register layout

A sysfs interface is a poor choice for this though -- how can you guarantee that
even future Renesas devices follow the exact same register layout? By
encoding the current hardware generation into the user interface, you
would end up having to emulate this on other chips you want to support later.

If it's only for debugging, best leave it out of the public interface, and only
have it in your own copy of the driver until the bugs are gone, or add a debugfs
interface.

> > Can you explain the purpose of this restriction? Why is it ok for two threads
> > to access the same file descriptor, but not two different file descriptors for
> > the same device?
> >
>
> The mutex is there to provide synchronization to the device access while PHC driver is accessing the device.
> The atomic count is to make sure only one user space program is using the driver at a time.

Then remove the atomic count, as it clearly doesn't do what you describe
when you can have multiple threads access the driver concurrently.

> > Each of these needs a device tree binding. It's usually better to name the
> > compatible strings according to the chips that contain this hardware, such as
> > "renesas,r8a1234567-rsmucdev" instead of "renesas,rsmu-cdev0".
> >
> > Since you don't seem to about the difference between the devices, the driver
> > can also just bind to one of them (usually the oldest) and then the newer
> > devices contain the string as a fallback, so you don't have to update the
> > driver every time another variant gets made.
> >
> >
>
> Actually the device is not spawned from device tree but from Renesas MFD driver (submitted in a separate thread).
> The MFD driver will call mfd_add_devices to create the platform devices. I am not sure if I still need to create binding
> In this case.

If you have an of_device_id table, it needs a binding. It sounds like you
don't need the of_device_id though.

> > This should probably be part of the .c file, as no other driver needs to
> > interface with it.
> >
>
> We actually run a unit test on the driver that needs to access this structure.
> That is why I need to put it in a header

Unit tests are good, but it's better to have them in the kernel.
Can you add the unit test into the patch then?
We now have the kunit framework for running unit tests.

> > This tells me that you got the abstraction the wrong way: the common files
> > should not need to know anything about the specific implementations.
> >
> > Instead, these should be in separate modules that call exported functions
> > from the common code.
> >
> >
>
> I got what you mean. But so far it only supports small set of functions, which is why
> I don't feet it is worth the effort to over abstract things.

Then maybe pick one of the two hardware variants and drop the abstraction you
have. You can then add more features before you add a proper abstraction
layer and then the second driver.

            Arnd

RE: [PATCH net-next] misc: Add Renesas Synchronization Management Unit (SMU) support

[Min Li]

> 
> Ah, so if this is for a PTP related driver, it should probably be integrated into
> the PTP subsystem rather than being a separate class.
> 

I was trying to add these functions to PHC subsystem but was not accepted because the functions
are specific to Renesas device and there is no place for those functions in PHC driver.

> > > A pure list of register values seems neither particular portable nor
> intuitive.
> > > How is a user expected to interpret these, and are you sure that any
> > > driver for this class would have the same interpretation at the same
> register index?
> > >
> >
> > Yes we need a way to dump register values when remote debugging with
> customers.
> > And all the Renesas SMU has similar register layout
> 
> A sysfs interface is a poor choice for this though -- how can you guarantee
> that even future Renesas devices follow the exact same register layout? By
> encoding the current hardware generation into the user interface, you would
> end up having to emulate this on other chips you want to support later.
> 
> If it's only for debugging, best leave it out of the public interface, and only
> have it in your own copy of the driver until the bugs are gone, or add a
> debugfs interface.
> 

I will drop the sysfs change in the new patch

> 
> Unit tests are good, but it's better to have them in the kernel.
> Can you add the unit test into the patch then?
> We now have the kunit framework for running unit tests.
> 

Our unit test is based on ceedling. But I will definitely look into the kunit and try to
Transfer it to kunit for the next release.

> > > This tells me that you got the abstraction the wrong way: the common
> > > files should not need to know anything about the specific
> implementations.
> > >
> > > Instead, these should be in separate modules that call exported
> > > functions from the common code.
> > >
> > >
> >
> > I got what you mean. But so far it only supports small set of
> > functions, which is why I don't feet it is worth the effort to over abstract
> things.
> 
> Then maybe pick one of the two hardware variants and drop the abstraction
> you have. You can then add more features before you add a proper
> abstraction layer and then the second driver.
> 

If I come up with a new file and move all the abstraction code there, does that work?

Re: [PATCH net-next] misc: Add Renesas Synchronization Management Unit (SMU) support

[Arnd Bergmann]

On Fri, Feb 12, 2021 at 5:19 PM Min Li <min.li.xe@renesas.com> wrote:
>
> >
> > Ah, so if this is for a PTP related driver, it should probably be integrated into
> > the PTP subsystem rather than being a separate class.
> >
>
> I was trying to add these functions to PHC subsystem but was not accepted because the functions
> are specific to Renesas device and there is no place for those functions in PHC driver.

It would be useful to explain that in the patch description and link
to the original
discussion there. What exactly was the objection?

> > > > This tells me that you got the abstraction the wrong way: the common
> > > > files should not need to know anything about the specific
> > implementations.
> > > >
> > > > Instead, these should be in separate modules that call exported
> > > > functions from the common code.
> > > >
> > > >
> > >
> > > I got what you mean. But so far it only supports small set of
> > > functions, which is why I don't feet it is worth the effort to over abstract
> > things.
> >
> > Then maybe pick one of the two hardware variants and drop the abstraction
> > you have. You can then add more features before you add a proper
> > abstraction layer and then the second driver.
> >
>
> If I come up with a new file and move all the abstraction code there,
> does that work?

I think so, but it's more important to figure out a good user space
interface first. The ioctl interfaces should be written on a higher-level
abstraction, to ensure they can work with any hardware implementation
and are not specific to Renesas devices.

Can you describe on an abstract level how a user would use the
character device, and what they achieve by that?

       Arnd

RE: [PATCH net-next] misc: Add Renesas Synchronization Management Unit (SMU) support

[Min Li]

> >
> > If I come up with a new file and move all the abstraction code there,
> > does that work?
> 
> I think so, but it's more important to figure out a good user space interface
> first. The ioctl interfaces should be written on a higher-level abstraction, to
> ensure they can work with any hardware implementation and are not
> specific to Renesas devices.
> 
> Can you describe on an abstract level how a user would use the character
> device, and what they achieve by that?
> 
>        Arnd

Hi Arnd

This driver is meant to be used by Renesas PTP Clock Manager for
Linux (pcm4l) software for Renesas device only.

About how pcm4l uses the char device, pcm4l will open the device
and do the supported ioctl cmds on the device, simple like that.

At the same time, pcm4l will also open ptp hardware clock device,
which is /dev/ptp[x], to do clock adjustments.

Min 

Re: [PATCH net-next] misc: Add Renesas Synchronization Management Unit (SMU) support

[Arnd Bergmann]

On Tue, Feb 16, 2021 at 6:10 PM Min Li <min.li.xe@renesas.com> wrote:
> > > If I come up with a new file and move all the abstraction code there,
> > > does that work?
> >
> > I think so, but it's more important to figure out a good user space interface
> > first. The ioctl interfaces should be written on a higher-level abstraction, to
> > ensure they can work with any hardware implementation and are not
> > specific to Renesas devices.
> >
> > Can you describe on an abstract level how a user would use the character
> > device, and what they achieve by that?
>
> This driver is meant to be used by Renesas PTP Clock Manager for
> Linux (pcm4l) software for Renesas device only.
>
> About how pcm4l uses the char device, pcm4l will open the device
> and do the supported ioctl cmds on the device, simple like that.
>
> At the same time, pcm4l will also open ptp hardware clock device,
> which is /dev/ptp[x], to do clock adjustments.

I can't help but think you are evading my question I asked. If there is no
specific action that this pcm4l tool needs to perform, then I'd think
we should better not provide any interface for it at all.

I also found a reference to only closed source software at
https://www.renesas.com/us/en/software-tool/ptp-clock-manager-linux
We don't add low-level interfaces to the kernel that are only
usable by closed-source software.

Once you are able to describe the requirements for what pcm4l
actually needs from the hardware, we can start discussing what
a high-level interface would look like that can be used to replace
the your current interface, in a way that would work across vendors
and with both pcm4l and open-source tools that do the same job.

      Arnd

RE: [PATCH net-next] misc: Add Renesas Synchronization Management Unit (SMU) support

[Min Li]

> 
> I can't help but think you are evading my question I asked. If there is no
> specific action that this pcm4l tool needs to perform, then I'd think we
> should better not provide any interface for it at all.
> 
> I also found a reference to only closed source software at
> https://www.renesas.com/us/en/software-tool/ptp-clock-manager-linux
> We don't add low-level interfaces to the kernel that are only usable by
> closed-source software.
> 
> Once you are able to describe the requirements for what pcm4l actually
> needs from the hardware, we can start discussing what a high-level
> interface would look like that can be used to replace the your current
> interface, in a way that would work across vendors and with both pcm4l and
> open-source tools that do the same job.
> 
>       Arnd

Hi Arnd

This driver is used by pcm4l to access functionalities that cannot be accessed through PHC(ptp hardware clock) interface.

All these functions are kind of specific to Renesas SMU device and I have never heard other devices offering similar functions

The 3 functions currently provided are (more to be added in the future)

- set combomode

In Telecom Boundary Clock (T-BC) and Telecom Time Slave Clock (T-TSC) applications per ITU-T G.8275.2, two DPLLs can be used:
one DPLL is configured as a DCO to synthesize PTP clocks, and the other DPLL is configured as an EEC(Ethernet Equipment Clock)
to generate physical layer clocks. Combo mode provides physical layer frequency support from the EEC/SEC to the PTP clock.

- read DPLL's FFO

Read fractional frequency offset (FFO) from a DPLL. 

For a DPLL channel, a Frequency Control Word (FCW) is used to adjust the frequency output of the DCO. A positive value will
increase the output frequency and a negative one will decrease the output frequency.

This function will read FCW first and convert it to FFO.

-read DPLL's state

The DPLLs support four primary operating modes: Free-Run, Locked, Holdover, and DCO. In Free-Run mode the DPLLs synthesize
clocks based on the system clock alone. In Locked mode the DPLLs filter reference clock jitter with the selected bandwidth. Additionally
in Locked mode, the long-term output frequency accuracy is the same as the long-term frequency accuracy of the selected input
reference. In Holdover mode, the DPLL uses frequency data acquired while in Locked mode to generate accurate frequencies when input
references are not available. In DCO mode, the DPLL control loop is opened and the DCO can be controlled by a PTP clock recovery
servo running on an external processor to synthesize PTP clocks.

Again, at the bottom, these function are just reading/writing certain registers through I2C/SPI interface.

I am making this driver to support pcm4l since my my company, Renesas, wants to abstract hw details into the kernel. But I can not figure out
how to make this universally applied interface and I find misc is the best place to hold driver like this. On the other hand, if you have better ideas,
I am all ears.

Min

Re: [PATCH net-next] misc: Add Renesas Synchronization Management Unit (SMU) support

[Arnd Bergmann]

On Tue, Feb 16, 2021 at 11:14 PM Min Li <min.li.xe@renesas.com> wrote:
> > I can't help but think you are evading my question I asked. If there is no
> > specific action that this pcm4l tool needs to perform, then I'd think we
> > should better not provide any interface for it at all.
> >
> > I also found a reference to only closed source software at
> > https://www.renesas.com/us/en/software-tool/ptp-clock-manager-linux
> > We don't add low-level interfaces to the kernel that are only usable by
> > closed-source software.
> >
> > Once you are able to describe the requirements for what pcm4l actually
> > needs from the hardware, we can start discussing what a high-level
> > interface would look like that can be used to replace the your current
> > interface, in a way that would work across vendors and with both pcm4l and
> > open-source tools that do the same job.
>
> This driver is used by pcm4l to access functionalities that cannot be accessed through PHC(ptp hardware clock) interface.
>
> All these functions are kind of specific to Renesas SMU device and I have never heard other devices offering similar functions
>
> The 3 functions currently provided are (more to be added in the future)
>
> - set combomode
>
> In Telecom Boundary Clock (T-BC) and Telecom Time Slave Clock (T-TSC) applications
> per ITU-T G.8275.2, two DPLLs can be used:
> one DPLL is configured as a DCO to synthesize PTP clocks, and the other DPLL is
> configured as an EEC(Ethernet Equipment Clock) to generate physical layer clocks.
> Combo mode provides physical layer frequency support from the EEC/SEC to the PTP
> clock.

Thank you for the explanation. Now, to take the question to an even
higher level, is it useful to leave it up to the user to pick one of the two
modes explicitly, or can the kernel make that decision based on some
other information that it already has, or that can be supplied to it
using a more abstract interface?

In other words, when would a user pick combomode over non-combomode
or vice versa? Would it make sense to have this configured according to
the hardware platform, e.g. in a device tree property of the device, rather
than having the user choose a mode?

Which of the two possible modes do other PTP devices use that support
DCO and EEC but are not configurable?

> - read DPLL's FFO
>
> Read fractional frequency offset (FFO) from a DPLL.
>
> For a DPLL channel, a Frequency Control Word (FCW) is used to adjust the
> frequency output of the DCO. A positive value will increase the output frequency
> and a negative one will decrease the output frequency.
>
> This function will read FCW first and convert it to FFO.

Is this related to the information in the timex->freq field? It sounds
like this would already be accessible through the existing
clock_adjtime() interface.

> -read DPLL's state
>
> The DPLLs support four primary operating modes: Free-Run, Locked,
> Holdover, and DCO. In Free-Run mode the DPLLs synthesize clocks
>  based on the system clock alone. In Locked mode the DPLLs filter
> reference clock jitter with the selected bandwidth. Additionally in
> Locked mode, the long-term output frequency accuracy is the same
> as the long-term frequency accuracy of the selected input reference.
> In Holdover mode, the DPLL uses frequency data acquired while in
> Locked mode to generate accurate frequencies when input
> references are not available. In DCO mode, the DPLL control loop
> is opened and the DCO can be controlled by a PTP clock recovery
> servo running on an external processor to synthesize PTP clocks.

Wouldn't any PTP clock run in one of these modes? If this is just
informational, it might be appropriate to have another sysfs attribute
for each PTP clock that shows the state of the DPLL, and then
have the PTP driver either fill in the current value in 'struct ptp_clock',
or provide a callback to report the state when a user reads the sysfs
attribute.

      Arnd

Re: [PATCH net-next] misc: Add Renesas Synchronization Management Unit (SMU) support

[Arnd Bergmann]

On Wed, Feb 17, 2021 at 9:20 PM Min Li <min.li.xe@renesas.com> wrote:
>
> I attached the G.8273.2 document, where chapter 6 is about supporting physical layer
> frequency. And combo mode is Renesas way to support this requirement. Other companies
> may come up with different ways to support it.
>
> When EEC quality is below certain level, we would wanna turn off combo mode.

Maybe this is something that could be handled inside of the device driver then?

If the driver can use the same algorithm that is in your user space software
today, that would seem to be a nicer way to handle it than requiring a separate
application.

> > > This function will read FCW first and convert it to FFO.
> >
> > Is this related to the information in the timex->freq field? It sounds like this
> > would already be accessible through the existing
> > clock_adjtime() interface.
> >
>
> They are related, but dealing with timex->freq has limitations
>
> 1) Renesas SMU has up to 8 DPLLs and only one of the them would be ptp
> clock and we want to be able to read any DPLL's FFO or state

Is this necessarily unique to Renesas SMU though? Not sure what
makes sense in terms of the phc/ptp interface. Could there just be
a separate instance for each DPLL in the phc subsystem even if it's
actually a ptp clock, or would that be an incorrect use?

> 2) timex->freq's unit is ppb and we want to read more precise ffo in smaller unit of ppqt

This also sounds like something that would not be vendor specific. If you
need a higher resolution, then at some point others would need it as well.
There is already precedence in 'struct timex' to redefine the resolution of
some fields based on a flag -- 'time.tv_usec' can either refer to microseconds
to nanoseconds.

If the range of the 'freq' field is sufficient to encode ppqt, you could add
another flag for that, otherwise another reserved field can be used.

> 3) there is no interface in the current ptp hardware clock infrastructure to read ffo back from hardware

Adding an internal interface is the easy part here, the hard part is defining
the user interface.

> > Wouldn't any PTP clock run in one of these modes? If this is just
> > informational, it might be appropriate to have another sysfs attribute for
> > each PTP clock that shows the state of the DPLL, and then have the PTP
> > driver either fill in the current value in 'struct ptp_clock', or provide a
> > callback to report the state when a user reads the sysfs attribute.
> >
>
> What you propose can work. But DPLL operating mode is not standardized
> so different vendor may have different explanation for various modes.

If it's a string, that could easily be extended to further modes, as long
as the kernel documents which names are allowed. If multiple vendors
refer to the same mode by different names, someone will have to decide
what to call it in the kernel, and everyone afterwards would use the same
name.

> Also, I thought sysfs is only for debug or informational purpose.
> Production software is not supposed to use it for critical tasks?

No, you are probably thinking of debugfs. sysfs is one of multiple
common ways to exchange this kind of data in a reliable way.

An ioctl would probably work just as well though, usually sysfs
is better when the information makes sense to human operators
or simple shell scripts, while an ioctl interface is better if performance
is important, or if the information is primarily used in C programs.

        Arnd

Re: [PATCH net-next] misc: Add Renesas Synchronization Management Unit (SMU) support

[Jakub Kicinski]

On Wed, 17 Feb 2021 22:30:14 +0100 Arnd Bergmann wrote:
> On Wed, Feb 17, 2021 at 9:20 PM Min Li <min.li.xe@renesas.com> wrote:
> > I attached the G.8273.2 document, where chapter 6 is about supporting physical layer
> > frequency. And combo mode is Renesas way to support this requirement. Other companies
> > may come up with different ways to support it.
> >
> > When EEC quality is below certain level, we would wanna turn off combo mode.  
> 
> Maybe this is something that could be handled inside of the device driver then?
> 
> If the driver can use the same algorithm that is in your user space software
> today, that would seem to be a nicer way to handle it than requiring a separate
> application.

Other points sound more time than networking, so no suggestions
from me, but on using PHC for L1 freq - that seems like a good 
fit for ethtool?

RE: [PATCH net-next] misc: Add Renesas Synchronization Management Unit (SMU) support

[Min Li]

> -----Original Message-----
> From: Arnd Bergmann <arnd@kernel.org>
> Sent: February 17, 2021 4:30 PM
> To: Min Li <min.li.xe@renesas.com>
> Cc: Derek Kiernan <derek.kiernan@xilinx.com>; Dragan Cvetic
> <dragan.cvetic@xilinx.com>; Arnd Bergmann <arnd@arndb.de>; gregkh
> <gregkh@linuxfoundation.org>; linux-kernel@vger.kernel.org; Networking
> <netdev@vger.kernel.org>; Richard Cochran <richardcochran@gmail.com>
> Subject: Re: [PATCH net-next] misc: Add Renesas Synchronization
> Management Unit (SMU) support
> 
> On Wed, Feb 17, 2021 at 9:20 PM Min Li <min.li.xe@renesas.com> wrote:
> >
> > I attached the G.8273.2 document, where chapter 6 is about supporting
> > physical layer frequency. And combo mode is Renesas way to support
> > this requirement. Other companies may come up with different ways to
> support it.
> >
> > When EEC quality is below certain level, we would wanna turn off combo
> mode.
> 
> Maybe this is something that could be handled inside of the device driver
> then?
> 
> If the driver can use the same algorithm that is in your user space software
> today, that would seem to be a nicer way to handle it than requiring a
> separate application.
> 

Hi Arnd


What is the device driver that you are referring here?

In summary of your reviews, are you suggesting me to discard this change and go back to PTP subsystem
to find a better place for things that I wanna do here?

Min

Re: [PATCH net-next] misc: Add Renesas Synchronization Management Unit (SMU) support

[Arnd Bergmann]

On Thu, Feb 18, 2021 at 4:28 AM Min Li <min.li.xe@renesas.com> wrote:
> > If the driver can use the same algorithm that is in your user space software
> > today, that would seem to be a nicer way to handle it than requiring a
> > separate application.
> >
>
> Hi Arnd
>
>
> What is the device driver that you are referring here?
>
> In summary of your reviews, are you suggesting me to discard this change
> and go back to PTP subsystem to find a better place for things that I wanna
> do here?

Yes, I mean doing it all in the PTP driver.

        Arnd

RE: [PATCH net-next] misc: Add Renesas Synchronization Management Unit (SMU) support

[Min Li]

> -----Original Message-----
> From: Arnd Bergmann <arnd@kernel.org>
> Sent: February 18, 2021 5:51 AM
> To: Min Li <min.li.xe@renesas.com>
> Cc: Derek Kiernan <derek.kiernan@xilinx.com>; Dragan Cvetic
> <dragan.cvetic@xilinx.com>; Arnd Bergmann <arnd@arndb.de>; gregkh
> <gregkh@linuxfoundation.org>; linux-kernel@vger.kernel.org; Networking
> <netdev@vger.kernel.org>; Richard Cochran <richardcochran@gmail.com>
> Subject: Re: [PATCH net-next] misc: Add Renesas Synchronization
> Management Unit (SMU) support
> 
> On Thu, Feb 18, 2021 at 4:28 AM Min Li <min.li.xe@renesas.com> wrote:
> > > If the driver can use the same algorithm that is in your user space
> > > software today, that would seem to be a nicer way to handle it than
> > > requiring a separate application.
> > >
> >
> > Hi Arnd
> >
> >
> > What is the device driver that you are referring here?
> >
> > In summary of your reviews, are you suggesting me to discard this
> > change and go back to PTP subsystem to find a better place for things
> > that I wanna do here?
> 
> Yes, I mean doing it all in the PTP driver.
> 
>         Arnd

Hi Arnd

The APIs I am adding here is for our development of assisted partial timing support (APTS),
which is a Global Navigation Satellite System (GNSS) backed by Precision Time Protocol (PTP).
So it is not part of PTP but they can work together for network timing solution.

What I am trying to say is the things that I am adding here doesn't really belong to the PTP world.
For example, timex->freq is different from the ffo that I am reading from this driver since the DPLL is
Working in different mode. For PTP, DPLL is working in DCO mode. In DCO mode, the DPLL 
control loop is opened and the DCO can be controlled by a PTP clock recovery servo running on an 
external processor to synthesize PTP clocks. On the other hand for GNSS timing, the ffo I am reading here is when DPLL is
in locked mode. In Locked the long-term output frequency accuracy is the same as the long-term
frequency accuracy of the selected input reference.

For our GNSS APTS development, we have 2 DPLL channels, one channel is locked to GNSS and another channel is PTP channel.
If GNSS channel is locked, we use GNSS's channel to support network timing. Otherwise, we switch to PTP channel. 

To think about it, our device is really an multi functional device (MFD), which is why I am submitting another review for our MFD driver
on the side. We have our PTP driver and we have this for GNSS APTS and other misc functions. 

So can you take a look at this again and see if it makes sense to keep this change simply because the change is not part of PTP subsystem.
They sound like they are related. But when it comes to technicality, there is really no place in PTP to hold stuff that I am doing here.

Thanks

Min

FW: [PATCH net-next] misc: Add Renesas Synchronization Management Unit (SMU) support

[Min Li]

-----Original Message-----
From: Min Li 
Sent: February 18, 2021 11:14 AM
To: 'Arnd Bergmann' <arnd@kernel.org>
Cc: Derek Kiernan <derek.kiernan@xilinx.com>; Dragan Cvetic <dragan.cvetic@xilinx.com>; Arnd Bergmann <arnd@arndb.de>; gregkh <gregkh@linuxfoundation.org>; linux-kernel@vger.kernel.org; Networking <netdev@vger.kernel.org>; Richard Cochran <richardcochran@gmail.com>
Subject: RE: [PATCH net-next] misc: Add Renesas Synchronization Management Unit (SMU) support



> -----Original Message-----
> From: Arnd Bergmann <arnd@kernel.org>
> Sent: February 18, 2021 5:51 AM
> To: Min Li <min.li.xe@renesas.com>
> Cc: Derek Kiernan <derek.kiernan@xilinx.com>; Dragan Cvetic 
> <dragan.cvetic@xilinx.com>; Arnd Bergmann <arnd@arndb.de>; gregkh 
> <gregkh@linuxfoundation.org>; linux-kernel@vger.kernel.org; Networking 
> <netdev@vger.kernel.org>; Richard Cochran <richardcochran@gmail.com>
> Subject: Re: [PATCH net-next] misc: Add Renesas Synchronization 
> Management Unit (SMU) support
> 
> On Thu, Feb 18, 2021 at 4:28 AM Min Li <min.li.xe@renesas.com> wrote:
> > > If the driver can use the same algorithm that is in your user 
> > > space software today, that would seem to be a nicer way to handle 
> > > it than requiring a separate application.
> > >
> >
> > Hi Arnd
> >
> >
> > What is the device driver that you are referring here?
> >
> > In summary of your reviews, are you suggesting me to discard this 
> > change and go back to PTP subsystem to find a better place for 
> > things that I wanna do here?
> 
> Yes, I mean doing it all in the PTP driver.
> 
>         Arnd

Hi Arnd

The APIs I am adding here is for our development of assisted partial timing support (APTS), which is a Global Navigation Satellite System (GNSS) backed by Precision Time Protocol (PTP).
So it is not part of PTP but they can work together for network timing solution.

What I am trying to say is the things that I am adding here doesn't really belong to the PTP world.
For example, timex->freq is different from the ffo that I am reading from this driver since the DPLL is Working in different mode. For PTP, DPLL is working in DCO mode. In DCO mode, the DPLL control loop is opened and the DCO can be controlled by a PTP clock recovery servo running on an external processor to synthesize PTP clocks. On the other hand for GNSS timing, the ffo I am reading here is when DPLL is in locked mode. In Locked the long-term output frequency accuracy is the same as the long-term frequency accuracy of the selected input reference.

For our GNSS APTS development, we have 2 DPLL channels, one channel is locked to GNSS and another channel is PTP channel.
If GNSS channel is locked, we use GNSS's channel to support network timing. Otherwise, we switch to PTP channel. 

To think about it, our device is really an multi functional device (MFD), which is why I am submitting another review for our MFD driver on the side. We have our PTP driver and we have this for GNSS APTS and other misc functions. 

So can you take a look at this again and see if it makes sense to keep this change simply because the change is not part of PTP subsystem.
They sound like they are related. But when it comes to technicality, there is really no place in PTP to hold stuff that I am doing here.

Thanks

Min

RE: [PATCH net-next] misc: Add Renesas Synchronization Management Unit (SMU) support

[Min Li]

[-- Attachment #1: Type: text/plain, Size: 2109 bytes --]

Hi Arnd

This is Min again.

I just got off the meeting with our Architect and I would like to correct some confusing information that I delivered before

I said this driver supports 1588/PTP related functionalities. This is not correct. In fact, this driver is developed to support our 
"GNSS Assisted Partial Timing Support" feature, which is not in PTP domain.

So I would categorize Renesas SMU (synchronization management unit) device as a multi-functional device and we are actually having a
separate review with Lee Jones for the MFD driver alone. Above the MFD driver, we have PHC driver to support PTP and this
driver for miscellaneous functions like GNSS APTS stuff.

I also attached the diagram drawing, where left column is the old way we are doing things and right column is how we wanna do it
through MFD now.

Hopefully, this will help you reconsider my proposal. Please do not hesitate to get back to me for this issue.

Thanks 

Min  

> -----Original Message-----
> From: Arnd Bergmann <arnd@kernel.org>
> Sent: February 18, 2021 5:51 AM
> To: Min Li <min.li.xe@renesas.com>
> Cc: Derek Kiernan <derek.kiernan@xilinx.com>; Dragan Cvetic
> <dragan.cvetic@xilinx.com>; Arnd Bergmann <arnd@arndb.de>; gregkh
> <gregkh@linuxfoundation.org>; linux-kernel@vger.kernel.org; Networking
> <netdev@vger.kernel.org>; Richard Cochran <richardcochran@gmail.com>
> Subject: Re: [PATCH net-next] misc: Add Renesas Synchronization
> Management Unit (SMU) support
> 
> On Thu, Feb 18, 2021 at 4:28 AM Min Li <min.li.xe@renesas.com> wrote:
> > > If the driver can use the same algorithm that is in your user space
> > > software today, that would seem to be a nicer way to handle it than
> > > requiring a separate application.
> > >
> >
> > Hi Arnd
> >
> >
> > What is the device driver that you are referring here?
> >
> > In summary of your reviews, are you suggesting me to discard this
> > change and go back to PTP subsystem to find a better place for things
> > that I wanna do here?
> 
> Yes, I mean doing it all in the PTP driver.
> 
>         Arnd

[-- Attachment #2: rsmu.pdf --]
[-- Type: application/pdf, Size: 91967 bytes --]