Hello Martin Actually you were right that my patch affects only meson8 family not the all new ones, my bad. It's clear from the driver code meson_saradc.c and dts files. I've made an experiment on a113l soc - changingclock_rate inmeson_sar_adc_param and measuring adc channel many times and with low clockfrequency (priv->adc_clk) time of measurementis high and vice versa. ADC_CLK_DIV field in SAR_ADC_REG3 is always zero. So now I need to get s805 (meson8) board for example and made experiment on it. Maximum value in 6 bits divider should get bigger measurement time then max 5 bits divider. And thanks for sharing your experience with us :) Best regards George On 5/17/23 22:29, Martin Blumenstingl wrote: > 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 >
