On Fri, Feb 12, 2021 at 2:40 AM Min Li [off-list ref] wrote:
quoted
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.
quoted
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.
quoted
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.
quoted
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.
quoted
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.
quoted
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
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.
quoted
quoted
A pure list of register values seems neither particular portable nor
intuitive.
quoted
quoted
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?
quoted
quoted
Yes we need a way to dump register values when remote debugging with
customers.
quoted
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.
quoted
quoted
This tells me that you got the abstraction the wrong way: the common
files should not need to know anything about the specific
implementations.
quoted
quoted
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?
On Fri, Feb 12, 2021 at 5:19 PM Min Li [off-list ref] wrote:
quoted
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?
quoted
quoted
quoted
This tells me that you got the abstraction the wrong way: the common
files should not need to know anything about the specific
implementations.
quoted
quoted
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
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
On Tue, Feb 16, 2021 at 6:10 PM Min Li [off-list ref] wrote:
quoted
quoted
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
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
On Tue, Feb 16, 2021 at 11:14 PM Min Li [off-list ref] wrote:
quoted
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