On 01/07/2024 11:01, Jerome Brunet wrote:
On Mon 01 Jul 2024 at 09:41, Neil Armstrong <neil.armstrong@xxxxxxxxxx> wrote:
On 25/06/2024 15:51, Jerome Brunet wrote:
On Tue 25 Jun 2024 at 15:18, Neil Armstrong <neil.armstrong@xxxxxxxxxx> wrote:
On 25/06/2024 11:53, Jerome Brunet wrote:
On Tue 25 Jun 2024 at 11:38, Neil Armstrong <neil.armstrong@xxxxxxxxxx> wrote:
Hi,
[+cc people from linux-msm]
On 24/06/2024 19:31, Jerome Brunet wrote:
Add support for the HW found in most Amlogic SoC dedicated to measure
system clocks.
This drivers aims to replace the one found in
drivers/soc/amlogic/meson-clk-measure.c with following improvements:
* Access to the measurements through the IIO API:
Easier re-use of the results in userspace and other drivers
* Controllable scale with raw measurements
* Higher precision with processed measurements
Jerome Brunet (2):
dt-bindings: iio: frequency: add clock measure support
iio: frequency: add amlogic clock measure support
.../iio/frequency/amlogic,clk-msr-io.yaml | 50 ++
drivers/iio/frequency/Kconfig | 15 +
drivers/iio/frequency/Makefile | 1 +
drivers/iio/frequency/amlogic-clk-msr-io.c | 802 ++++++++++++++++++
4 files changed, 868 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/frequency/amlogic,clk-msr-io.yaml
create mode 100644 drivers/iio/frequency/amlogic-clk-msr-io.c
While I really appreciate the effort, and the code looks cool, the clkmsr is really
a debug tool, and I'm not sure IIO is the right place for such debug tool ?
The reason why I went through the trouble of doing an IIO port is
because I need that for other purposes than debug. I need to to be able
to check a frequency from another driver. I don't see a reason to invent
another API when IIO provide a perfectly good one.
The HW does measurements. IIO seems like the best place for it.
For the record, I need this for a eARC support.
eARC has a PLL that locks on incoming stream. eARC registers show wether
the PLL is locked or not, but not at which rate. That information is
needed in ASoC. Fortunately the eARC PLL is one of measured clock, which
is a life saver in that case.
This is a very interesting use-case, and quite weird nothing is provided
on the eARC side.
Indeed.
So yes it's definitely a valid use-case, but:
- we should keep the debugfs interface, perhaps move it in the iio driver ?
I considered this initially but it would add a lot of boiler plate
code to provide over debugfs exactly what iio already provides over
sysfs. As you pointed out, the previous driver only provided debug
information, the debugfs interface it provided is hardly a
critical/stable one.
I still don't see why it could add so much boilerplate, all the tables and
calculation fonction would be shared, only the debugfs clk_msr_show() and
clk_msr_summary_show() would be kept, all the rest would be common.
I insist, please keep the debugfs interface for debug purposes. You don't
want to mess with IIO when you bring up new platforms with bare minimum
kernels.
I don't think that is going to change anything. It's not like IIO brings
any complexity or will be compiled out.
But since you insist, I'll add it in the next version as a separate patch.
- we should keep a single compatible, so simply update the current bindings with iio cells
Using a new compatible allows to split the memory region, making the
interface between DT and driver a lot easier to implement seemlessly
between old and new SoCs. Eventually it may allow to implement the duty
part too.
It's a problem for new platforms, you can introduce the split only for the
new ones, the impact on code won't high enough to justify new bindings.
What you are requesting will introduce two drivers providing the same
compatible, unless you plan on removing the old one in a coordinated
way.
That's an unncessary churn. The old driver could stay there for a
while and platform slowly migrate. What you are requesting forcefully
migrates every consumer, assuming the old driver is compiled out.
This is an opportunity to more correctly describe the interface.
It does not break any DT rules, that is enough of a justification IMO.
DT describes the Hardware, I don't see how the new bindings describes better
the current hardware... tying the new bindings to a new driver is actually
against the DT rules, the bindings thing is actually to avoid that.
For PWM, bindings architecture was clearly wrong, but here, not really.
I still don't see the problem of migrating current users to the new driver
using the current compatible, really, please explain what would be the problem ?
In any case you'll only need to add the #io-channel-cells to boards that would
require frequency monitoring for eARC, for all the other boards you'll won't need it.
So this property can safely be added as optional to the current bindings.
Neil
Neil
- for s4 & c3, it's ok to either add a second reg entry in the bindings
Doing that for s4 and c3 only would still make a mess of offset handling
the region because duty prepend the region on old SoC. The goal is to
have an interface that seemlessly support both old and new SoCs.
Neil
Everything that was available through the old driver still is, with more
precision and more control.
There's almost the same interface on qcom SoCs (https://github.com/linux-msm/debugcc) but
they chose to keep it in userspace until we find an appropriate way to expose
this from the kernel the right way.
If it enabled us to monitor a frequency input for a product use-case, IIO would be
the appropriate interface, but AFAIK it's only internal clocks and thus I'm worried
it's not the best way to expose those clocks.
Neil
