Re: [PATCH v1] meson saradc: fix clock divider mask length

[Martin Blumenstingl]

Hello Vyacheslav, George and Dmitry,

On Wed, May 17, 2023 at 1:37 PM Vyacheslav <adeep@xxxxxxxxx> wrote:
>
> Hi, Martin,
>
> On 16.05.2023 22:08, Martin Blumenstingl wrote:
> > Hi George,
> >
> > thank you for this patch!
> >
> > On Mon, May 15, 2023 at 11:06 PM George Stark <gnstark@xxxxxxxxxxxxxx> wrote:
> >>
> >> From: George Stark <GNStark@xxxxxxxxxxxxxx>
> >>
> >> According to datasheets of supported meson SOCs
> >> length of ADC_CLK_DIV field is 6 bits long
> > I have a question about this sentence which doesn't affect this patch
> > - it's only about managing expectations:
> > Which SoC are you referring to?
>
> I checked the 905x, 905x3, a113x datasheets - there is the same register
> with 6 bits for  ADC_CLK_DIV
This highlights a common issue I have seen with Amlogic datasheets:
parts of the datasheet are outdated (or incorrect in one way or
another).
For my following explanation I will refer to the public S905X3
datasheet from [0].

The documentation for SAR_ADC_REG3 on page 1065 states that this
register contains:
- bit 30: SAR ADC_CLK_EN: 1 = enable the SAR ADC clock
- bits 10-15: ADC_CLK_DIV: The ADC clock is derived by dividing the
27Mhz crystal by N+1. This value divides the 27Mhz clock to generate
an ADC clock. A value of 20 for example divides the 27Mhz clock by 21
to generate an equivalent 1.28Mhz clock

The first problem with this part of the documentation is that there's
no 27MHz crystal on the Amlogic SoCs listed (S905X, S905X3), only a
24MHz one.
I'm also human, I'm not perfect so typos and mistakes happen. If you
look at the S805 datasheet (from year 2015) on page 116/117 you'll see
that even back then it said 27MHz - and even that SoC generation
(Meson8b) had a 24MHz crystal, not a 27MHz one. In over five years
that typo has not been fixed.

Let's focus on the S905X3 datasheet again, this time page 101 where it
has "Figure 7-8 AO Clock Sources".
Note that the register offsets listed in that section need to be
multiplied by 4 to get the actual offset in IO memory.
It describes the "sar_adc_clk" with:
- first mux at register 0x90 (= 0x24 * 4) bits [10:9] (inputs are: 0 =
XTAL, 1 = clk81, 2 and 3 are grounded)
- gate at register 0x90 (= 0x24 * 4) bit 8
- divider at register 0x90 (= 0x24 * 4) bits [7:0]
- second mux at register 0x90 (= 0x24 * 4) bit 0 (inputs are: 0 =
divider from above, 1 = XTAL)

Looking at drivers/clk/meson/g12a-aoclk.c this is what we implement
(apart form the second mux, which seems to be missing).
But this now gets confusing: why are there now two dividers and two
gates (one the SAR ADC registers and another on in the AO clock
controller registers)?

Looking at my board (G12A X96 Max in this case, but it's uses the same
clock controller drivers as SM1/S905X3) where &saradc is not enabled
(meaning: it uses SoC defaults or values initialized by the vendor
u-boot/TF-A):
$ grep adc /sys/kernel/debug/clk/clk_summary
   g12a_ao_saradc_mux                0        0        0    24000000
      g12a_ao_saradc_div             0        0        0     1142858
         g12a_ao_saradc_gate         0        0        0     1142858
                        g12a_ao_saradc       0        0        0   166666664
                        g12a_adc       0        0        0   166666664
(output is shortened to make it easier to read)

1142858Hz is 24MHz divided by 21 (as described in the SAR ADC register
space - but these values are from the AO clock controller registers).
So my thought is: if the clock has been programmed in the AO clock
register space then the divider and gate from the SAR ADC register
space are not used (anymore) on this SoC generation.

My understanding so far (matching experiments I made long time ago) is:
- the gate and divider within the SAR ADC register space are only
relevant for SoCs that predate the GXBB generation
- SoCs starting from the GXBB SoC generation (that includes GXL, SM1,
...) use a dedicated SAR ADC clock provided by some clock controller
(see the output of $ git grep -E "sar[_]?adc" drivers/clk/meson/ |
grep name | grep -v _div | grep -v _mux | grep -v _sel)
-- I think this even applies to the A1 SoC, looking at "clk: meson:
a1: add Amlogic A1 Peripherals clock controller driver" [2] from
Dmitry the peripheral clock controller has a "saradc" clock tree (with
mux, divider, gate)

As result of my understanding meson_saradc.c will only register the
divider and gate (using meson_sar_adc_clk_init()) if no ADC clock is
provided via the .dtb.
On GXBB and newer SoCs meson_sar_adc_clk_init() is not called and the
divider and gate from the SAR ADC registers are not used.

Amlogic has debug tool IP block in these SoCs called "clock  measurer"
which can measure various clocks.
We provide a debugfs interface in
/sys/kernel/debug/meson-clk-msr/measure_summary
My suggestion is to play around with the SAR ADC clock (both, the one
from the peripheral clock controller on your SoC and the one inside
the SAR ADC registers) and see which clock has an impact on the
measured clock rate.

PS: I apologize for this long mail. I want to make clear that it's not
a rant towards you.
My thought is to share some of the experiences I made in the past.
I'm always hoping that the quality of the datasheets improves over
time. In some regards they do (a lot more IPs are documented compared
to older generations).
Missing details in the datasheet or incorrect descriptions have cost
me a lot of time previously.
My ultimate suggestion is to double check (for example with the clock
measurer, a scope, ...) what's written in the datasheet so you're not
wasting time like I did.


Best regards,
Martin


[0] https://dn.odroid.com/S905X3/ODROID-C4/Docs/S905X3_Public_Datasheet_Hardkernel.pdf
[1] https://dn.odroid.com/S805/Datasheet/S805_Datasheet%20V0.8%2020150126.pdf
[2] https://lore.kernel.org/linux-amlogic/20230517133309.9874-7-ddrokosov@xxxxxxxxxxxxxx/T/#u