From: George Stark <[email protected]>
According to datasheets of supported meson SOCs
length of ADC_CLK_DIV field is 6 bits long
Fixes: 3adbf3427330 ("iio: adc: add a driver for the SAR ADC found in Amlogic Meson SoCs")
Signed-off-by: George Stark <[email protected]>
---
drivers/iio/adc/meson_saradc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c
index 85b6826cc10c..b93ff42b8c19 100644
--- a/drivers/iio/adc/meson_saradc.c
+++ b/drivers/iio/adc/meson_saradc.c
@@ -72,7 +72,7 @@
#define MESON_SAR_ADC_REG3_PANEL_DETECT_COUNT_MASK GENMASK(20, 18)
#define MESON_SAR_ADC_REG3_PANEL_DETECT_FILTER_TB_MASK GENMASK(17, 16)
#define MESON_SAR_ADC_REG3_ADC_CLK_DIV_SHIFT 10
- #define MESON_SAR_ADC_REG3_ADC_CLK_DIV_WIDTH 5
+ #define MESON_SAR_ADC_REG3_ADC_CLK_DIV_WIDTH 6
#define MESON_SAR_ADC_REG3_BLOCK_DLY_SEL_MASK GENMASK(9, 8)
#define MESON_SAR_ADC_REG3_BLOCK_DLY_MASK GENMASK(7, 0)
--
2.38.4
On 15/05/2023 23:05, George Stark wrote:
> From: George Stark <[email protected]>
>
> According to datasheets of supported meson SOCs
> length of ADC_CLK_DIV field is 6 bits long
>
> Fixes: 3adbf3427330 ("iio: adc: add a driver for the SAR ADC found in Amlogic Meson SoCs")
> Signed-off-by: George Stark <[email protected]>
> ---
> drivers/iio/adc/meson_saradc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c
> index 85b6826cc10c..b93ff42b8c19 100644
> --- a/drivers/iio/adc/meson_saradc.c
> +++ b/drivers/iio/adc/meson_saradc.c
> @@ -72,7 +72,7 @@
> #define MESON_SAR_ADC_REG3_PANEL_DETECT_COUNT_MASK GENMASK(20, 18)
> #define MESON_SAR_ADC_REG3_PANEL_DETECT_FILTER_TB_MASK GENMASK(17, 16)
> #define MESON_SAR_ADC_REG3_ADC_CLK_DIV_SHIFT 10
> - #define MESON_SAR_ADC_REG3_ADC_CLK_DIV_WIDTH 5
> + #define MESON_SAR_ADC_REG3_ADC_CLK_DIV_WIDTH 6
> #define MESON_SAR_ADC_REG3_BLOCK_DLY_SEL_MASK GENMASK(9, 8)
> #define MESON_SAR_ADC_REG3_BLOCK_DLY_MASK GENMASK(7, 0)
>
Reviewed-by: Neil Armstrong <[email protected]>
Hi George,
thank you for this patch!
On Mon, May 15, 2023 at 11:06 PM George Stark <[email protected]> wrote:
>
> From: George Stark <[email protected]>
>
> 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?
This divider is only relevant on older SoCs that predate GXBB (S905).
To my knowledge all SoCs from GXBB onwards place the divider in the
main or AO clock controller, so this bitmask is irrelevant there.
> Fixes: 3adbf3427330 ("iio: adc: add a driver for the SAR ADC found in Amlogic Meson SoCs")
> Signed-off-by: George Stark <[email protected]>
Since my question above doesn't affect this patch:
Reviewed-by: Martin Blumenstingl <[email protected]>
Hello Martin,
On Tue, May 16, 2023 at 09:08:26PM +0200, Martin Blumenstingl wrote:
> Hi George,
>
> thank you for this patch!
>
> On Mon, May 15, 2023 at 11:06 PM George Stark <[email protected]> wrote:
> >
> > From: George Stark <[email protected]>
> >
> > 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?
> This divider is only relevant on older SoCs that predate GXBB (S905).
> To my knowledge all SoCs from GXBB onwards place the divider in the
> main or AO clock controller, so this bitmask is irrelevant there.
>
We are currently working with the A1 SoC family, specifically the A113L
variant. According to the datasheet for the A113L, the ADC_CLK_DIV can
be found in the SAR_ADC_REG3 register as a 6-bit value located within
bits 15 through 10.
> > Fixes: 3adbf3427330 ("iio: adc: add a driver for the SAR ADC found in Amlogic Meson SoCs")
> > Signed-off-by: George Stark <[email protected]>
> Since my question above doesn't affect this patch:
> Reviewed-by: Martin Blumenstingl <[email protected]>
--
Thank you,
Dmitry
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 <[email protected]> wrote:
>>
>> From: George Stark <[email protected]>
>>
>> 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 divider is only relevant on older SoCs that predate GXBB (S905).
> To my knowledge all SoCs from GXBB onwards place the divider in the
> main or AO clock controller, so this bitmask is irrelevant there.
>
>> Fixes: 3adbf3427330 ("iio: adc: add a driver for the SAR ADC found in Amlogic Meson SoCs")
>> Signed-off-by: George Stark <[email protected]>
> Since my question above doesn't affect this patch:
> Reviewed-by: Martin Blumenstingl <[email protected]>
>
> _______________________________________________
> linux-amlogic mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-amlogic
--
Vyacheslav Bocharov
On 5/16/23 22:08, Martin Blumenstingl wrote:
> Hi George,
>
> thank you for this patch!
>
> On Mon, May 15, 2023 at 11:06 PM George Stark <[email protected]> wrote:
>> From: George Stark <[email protected]>
>>
>> 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?
> This divider is only relevant on older SoCs that predate GXBB (S905).
> To my knowledge all SoCs from GXBB onwards place the divider in the
> main or AO clock controller, so this bitmask is irrelevant there.
Hello Martin
I've checked datasheets of all chips listed in meson_sar_adc_of_match array in meson_saradc.c and everywhere this field is 6 bits long. According to driver code and existing dts files this patch affects all supported chips except meson8.
Best regards
George
>> Fixes: 3adbf3427330 ("iio: adc: add a driver for the SAR ADC found in Amlogic Meson SoCs")
>> Signed-off-by: George Stark <[email protected]>
> Since my question above doesn't affect this patch:
> Reviewed-by: Martin Blumenstingl <[email protected]>
>
Hello Vyacheslav, George and Dmitry,
On Wed, May 17, 2023 at 1:37 PM Vyacheslav <[email protected]> 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 <[email protected]> wrote:
> >>
> >> From: George Stark <[email protected]>
> >>
> >> 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/[email protected]/T/#u
On Wed, 17 May 2023 16:47:59 +0000
Старк Георгий Николаевич <[email protected]> wrote:
> On 5/16/23 22:08, Martin Blumenstingl wrote:
> > Hi George,
> >
> > thank you for this patch!
> >
> > On Mon, May 15, 2023 at 11:06 PM George Stark <[email protected]> wrote:
> >> From: George Stark <[email protected]>
> >>
> >> 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?
> > This divider is only relevant on older SoCs that predate GXBB (S905).
> > To my knowledge all SoCs from GXBB onwards place the divider in the
> > main or AO clock controller, so this bitmask is irrelevant there.
>
> Hello Martin
>
> I've checked datasheets of all chips listed in meson_sar_adc_of_match array in meson_saradc.c and everywhere this field is 6 bits long. According to driver code and existing dts files this patch affects all supported chips except meson8.
On that note, do we want to add any clarifying text on the scope to the
commit message?
Jonathan
>
> Best regards
> George
>
>
> >> Fixes: 3adbf3427330 ("iio: adc: add a driver for the SAR ADC found in Amlogic Meson SoCs")
> >> Signed-off-by: George Stark <[email protected]>
> > Since my question above doesn't affect this patch:
> > Reviewed-by: Martin Blumenstingl <[email protected]>
> >
>
On Sat, May 20, 2023 at 6:30 PM Jonathan Cameron <[email protected]> wrote:
> On Wed, 17 May 2023 16:47:59 +0000
> Старк Георгий Николаевич <[email protected]> wrote:
> > On 5/16/23 22:08, Martin Blumenstingl wrote:
...
> > I've checked datasheets of all chips listed in meson_sar_adc_of_match array in meson_saradc.c and everywhere this field is 6 bits long. According to driver code and existing dts files this patch affects all supported chips except meson8.
>
> On that note, do we want to add any clarifying text on the scope to the
> commit message?
We should, I think, but according to Martin's message, as I may
interpret it, the datasheet may also be wrong and some experiments on
a real hardware should prepend any change in the code.
--
With Best Regards,
Andy Shevchenko
Martin,
On Wed, May 17, 2023 at 09:28:58PM +0200, Martin Blumenstingl wrote:
> Hello Vyacheslav, George and Dmitry,
>
> On Wed, May 17, 2023 at 1:37 PM Vyacheslav <[email protected]> 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 <[email protected]> wrote:
> > >>
> > >> From: George Stark <[email protected]>
> > >>
> > >> 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/[email protected]/T/#u
Thank you very much for providing such a detailed description of all the
circumstances that could be valuable in this situation. I completely
agree with you that experiments are required here, so we will definitely
run them. Additionally, we will reach out to Amlogic regarding this
strange behavior and share the results with you and the community. I
believe that this will be important as well.
--
Thank you,
Dmitry
Hello Jonathan,
Thank you very much for the review!
On Sat, May 20, 2023 at 04:46:18PM +0100, Jonathan Cameron wrote:
> On Wed, 17 May 2023 16:47:59 +0000
> Старк Георгий Николаевич <[email protected]> wrote:
>
> > On 5/16/23 22:08, Martin Blumenstingl wrote:
> > > Hi George,
> > >
> > > thank you for this patch!
> > >
> > > On Mon, May 15, 2023 at 11:06 PM George Stark <[email protected]> wrote:
> > >> From: George Stark <[email protected]>
> > >>
> > >> 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?
> > > This divider is only relevant on older SoCs that predate GXBB (S905).
> > > To my knowledge all SoCs from GXBB onwards place the divider in the
> > > main or AO clock controller, so this bitmask is irrelevant there.
> >
> > Hello Martin
> >
> > I've checked datasheets of all chips listed in meson_sar_adc_of_match array in meson_saradc.c and everywhere this field is 6 bits long. According to driver code and existing dts files this patch affects all supported chips except meson8.
>
> On that note, do we want to add any clarifying text on the scope to the
> commit message?
To begin with, we will conduct all experiments that Martin mentioned in
another message within this thread. Once completed, we will share the
results in this current thread. If the changes are still relevant, we
will prepare a new patch set with a detailed commit message.
[...]
--
Thank you,
Dmitry
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 <[email protected]> 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 <[email protected]> wrote:
>>>> From: George Stark <[email protected]>
>>>>
>>>> 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/[email protected]/T/#u
>
Hi George,
On Mon, May 22, 2023 at 5:47 PM Старк Георгий Николаевич
<[email protected]> wrote:
>
> 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.
Thanks for sharing your findings!
> I need to get s805 (meson8) board for example and made experiment on it.
If you don't find any Meson8 (S802)/Meson8b (S805) or Meson8m2 (S812)
board then please provide the code that you used for your experiment
as a patch so I can give it a try on my Odroid-C1 (Meson8b).
Best regards,
Martin
On 5/29/23 23:41, Martin Blumenstingl wrote:
> Hi George,
>
> On Mon, May 22, 2023 at 5:47 PM Старк Георгий Николаевич
> <[email protected]> wrote:
>> 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.
> Thanks for sharing your findings!
>
>> I need to get s805 (meson8) board for example and made experiment on it.
> If you don't find any Meson8 (S802)/Meson8b (S805) or Meson8m2 (S812)
> board then please provide the code that you used for your experiment
> as a patch so I can give it a try on my Odroid-C1 (Meson8b).
Hello Martin
Thanks for the help! I managed to get Odroid-C1 myself. I think it'll be
useful to test further patches.
By the way seems like the board and the sock are not popular anymore...
It remains only to build mainline kernel for meson8 to perform tests.
Hope to be back soon.
Best regards
George
>
>
> Best regards,
> Martin
>
On 5/29/23 23:41, Martin Blumenstingl wrote:
> Hi George,
>
> On Mon, May 22, 2023 at 5:47 PM Старк Георгий Николаевич
> <[email protected]> wrote:
>> 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.
> Thanks for sharing your findings!
>
>> I need to get s805 (meson8) board for example and made experiment on it.
> If you don't find any Meson8 (S802)/Meson8b (S805) or Meson8m2 (S812)
> board then please provide the code that you used for your experiment
> as a patch so I can give it a try on my Odroid-C1 (Meson8b).
>
>
> Best regards,
> Martin
>
Hello Martin
Here the test I promised:
Question: what's the real size of clock divder field in SAR_ADC_REG3 register in saradc in meson8 socs?
The current kernel code says 5 bits
The datasheet says 6 bit
The parent clock of adc clock is 24Mhz
I can check it here by:
# cat /sys/kernel/debug/clk/clk_summary
xtal 4 4 1 24000000 0 0 50000 Y
c1108680.adc#adc_div 1 1 0 1142858 0 0 50000 Y
c1108680.adc#adc_en 1 1 0 1142858 0 0 50000 Y
for divider width 5bit min adc clock is 24Mhz / 32 = 750KHZ
for divider width 6bit min adc clock is 24Mhz / 64 = 375KHz
I suppose that the lower adc clock rate the higher measurement time
so I need to get measurement time at both clk freqs and the times differ so
6bit divider is really applied
I performed test at Odroid-C1, kernel 6.2-rc8
Two kernel patches must be applied:
the topic starter patch and the helper patch at the end of the letter
In the helper patch I turn on CLOCK_ALLOW_WRITE_DEBUGFS to change clock rate from she shell
and use ktime_get_raw_ts64 to measure measurement time
So the the test itself:
cat /sys/devices/platform/soc/c1100000.cbus/c1108680.adc/iio:device0/in_voltage3_raw
[ 1781.226309] ==== freq: 1142858 time 42408000
# echo 750000 > /sys/kernel/debug/clk/c1108680.adc#adc_en/clk_rate
# cat /sys/devices/platform/soc/c1100000.cbus/c1108680.adc/iio:device0/in_voltage3_raw
[ 1790.728656] ==== freq: 750000 time 49173000
# echo 375000 > /sys/kernel/debug/clk/c1108680.adc#adc_en/clk_rate
# cat /sys/devices/platform/soc/c1100000.cbus/c1108680.adc/iio:device0/in_voltage3_raw
[ 1816.955477] ==== freq: 375000 time 68245000
# cat /sys/kernel/debug/clk/clk_summary
xtal 4 4 1 24000000 0 0 50000 Y
c1108680.adc#adc_div 1 1 0 375000 0 0 50000 Y
c1108680.adc#adc_en 1 1 0 375000 0 0 50000 Y
Helper patch:
diff --git a/drivers/clk/baikal-t1/ccu-pll.c
b/drivers/clk/baikal-t1/ccu-pll.c
index 13ef28001439..226e0d46a994 100644
--- a/drivers/clk/baikal-t1/ccu-pll.c
+++ b/drivers/clk/baikal-t1/ccu-pll.c
@@ -363,7 +363,7 @@ static const struct ccu_pll_dbgfs_fld ccu_pll_flds[] = {
* therefore we don't provide any kernel config based compile time
option for
* this feature to enable.
*/
-#undef CCU_PLL_ALLOW_WRITE_DEBUGFS
+#define CCU_PLL_ALLOW_WRITE_DEBUGFS
#ifdef CCU_PLL_ALLOW_WRITE_DEBUGFS
static int ccu_pll_dbgfs_bit_set(void *priv, u64 val)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index e62552a75f08..d552acdaa7bc 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3263,7 +3263,7 @@ static int clk_dump_show(struct seq_file *s, void
*data)
}
DEFINE_SHOW_ATTRIBUTE(clk_dump);
-#undef CLOCK_ALLOW_WRITE_DEBUGFS
+#define CLOCK_ALLOW_WRITE_DEBUGFS
#ifdef CLOCK_ALLOW_WRITE_DEBUGFS
/*
* This can be dangerous, therefore don't provide any real compile time
diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c
index 6959a0064551..65132d3ca46a 100644
--- a/drivers/iio/adc/meson_saradc.c
+++ b/drivers/iio/adc/meson_saradc.c
@@ -24,6 +24,8 @@
#include <linux/regulator/consumer.h>
#include <linux/mfd/syscon.h>
+#include <linux/timekeeping.h>
+
#define MESON_SAR_ADC_REG0 0x00
#define MESON_SAR_ADC_REG0_PANEL_DETECT BIT(31)
#define MESON_SAR_ADC_REG0_BUSY_MASK GENMASK(30, 28)
@@ -599,6 +601,8 @@ static int meson_sar_adc_get_sample(struct iio_dev
*indio_dev,
struct device *dev = indio_dev->dev.parent;
int ret;
+ int i;
+
if (chan->type == IIO_TEMP && !priv->temperature_sensor_calibrated)
return -ENOTSUPP;
@@ -606,16 +610,28 @@ static int meson_sar_adc_get_sample(struct iio_dev
*indio_dev,
if (ret)
return ret;
- /* clear the FIFO to make sure we're not reading old values */
- meson_sar_adc_clear_fifo(indio_dev);
- meson_sar_adc_set_averaging(indio_dev, chan, avg_mode, avg_samples);
+ struct timespec64 ts0, ts1;
+ ktime_get_raw_ts64(&ts0);
- meson_sar_adc_enable_channel(indio_dev, chan);
+ for (i = 0; i < 1000; i++) {
- meson_sar_adc_start_sample_engine(indio_dev);
- ret = meson_sar_adc_read_raw_sample(indio_dev, chan, val);
- meson_sar_adc_stop_sample_engine(indio_dev);
+ /* clear the FIFO to make sure we're not reading old
values */
+ meson_sar_adc_clear_fifo(indio_dev);
+
+ meson_sar_adc_set_averaging(indio_dev, chan, avg_mode,
avg_samples);
+
+ meson_sar_adc_enable_channel(indio_dev, chan);
+
+ meson_sar_adc_start_sample_engine(indio_dev);
+ ret = meson_sar_adc_read_raw_sample(indio_dev, chan, val);
+ meson_sar_adc_stop_sample_engine(indio_dev);
+ }
+
+ ktime_get_raw_ts64(&ts1);
+ u64 t0 = NSEC_PER_SEC * ts0.tv_sec + ts0.tv_nsec;
+ u64 t1 = NSEC_PER_SEC * ts1.tv_sec + ts1.tv_nsec;
+ printk("==== freq: %lu time %lld\n",
clk_get_rate(priv->adc_clk), t1 - t0);
meson_sar_adc_unlock(indio_dev);
Best regards
George
Hi George,
On Thu, Jun 1, 2023 at 10:56 PM George Stark <[email protected]> wrote:
[...]
> Here the test I promised:
> Question: what's the real size of clock divder field in SAR_ADC_REG3 register in saradc in meson8 socs?
> The current kernel code says 5 bits
> The datasheet says 6 bit
>
> The parent clock of adc clock is 24Mhz
> I can check it here by:
>
> # cat /sys/kernel/debug/clk/clk_summary
> xtal 4 4 1 24000000 0 0 50000 Y
> c1108680.adc#adc_div 1 1 0 1142858 0 0 50000 Y
> c1108680.adc#adc_en 1 1 0 1142858 0 0 50000 Y
>
> for divider width 5bit min adc clock is 24Mhz / 32 = 750KHZ
> for divider width 6bit min adc clock is 24Mhz / 64 = 375KHz
>
> I suppose that the lower adc clock rate the higher measurement time
> so I need to get measurement time at both clk freqs and the times differ so
> 6bit divider is really applied
>
> I performed test at Odroid-C1, kernel 6.2-rc8
> Two kernel patches must be applied:
>
> the topic starter patch and the helper patch at the end of the letter
> In the helper patch I turn on CLOCK_ALLOW_WRITE_DEBUGFS to change clock rate from she shell
> and use ktime_get_raw_ts64 to measure measurement time
>
> So the the test itself:
> cat /sys/devices/platform/soc/c1100000.cbus/c1108680.adc/iio:device0/in_voltage3_raw
> [ 1781.226309] ==== freq: 1142858 time 42408000
>
> # echo 750000 > /sys/kernel/debug/clk/c1108680.adc#adc_en/clk_rate
> # cat /sys/devices/platform/soc/c1100000.cbus/c1108680.adc/iio:device0/in_voltage3_raw
> [ 1790.728656] ==== freq: 750000 time 49173000
>
> # echo 375000 > /sys/kernel/debug/clk/c1108680.adc#adc_en/clk_rate
> # cat /sys/devices/platform/soc/c1100000.cbus/c1108680.adc/iio:device0/in_voltage3_raw
> [ 1816.955477] ==== freq: 375000 time 68245000
>
> # cat /sys/kernel/debug/clk/clk_summary
> xtal 4 4 1 24000000 0 0 50000 Y
> c1108680.adc#adc_div 1 1 0 375000 0 0 50000 Y
> c1108680.adc#adc_en 1 1 0 375000 0 0 50000 Y
These results looks excellent - thanks for sharing the test results!
Could you please check one last thing:
$ grep -i adc /sys/kernel/debug/meson-clk-msr/measure_summary
It should confirm that the clock rate is 375kHz (or close to it, SoC
internal clock measurement is not 100% precise)
Once we have that confirmation: can you please re-send the patch with
the description updated so it's clear which SoC generations are
affected and by stating that the fix was tested on a Meson8b Odroid-C1
board.
Thank you and best regards,
Martin
On 6/5/23 23:18, Martin Blumenstingl wrote:
> Hi George,
>
> On Thu, Jun 1, 2023 at 10:56 PM George Stark <[email protected]> wrote:
> [...]
>> Here the test I promised:
>> Question: what's the real size of clock divder field in SAR_ADC_REG3 register in saradc in meson8 socs?
>> The current kernel code says 5 bits
>> The datasheet says 6 bit
>>
>> The parent clock of adc clock is 24Mhz
>> I can check it here by:
>>
>> # cat /sys/kernel/debug/clk/clk_summary
>> xtal 4 4 1 24000000 0 0 50000 Y
>> c1108680.adc#adc_div 1 1 0 1142858 0 0 50000 Y
>> c1108680.adc#adc_en 1 1 0 1142858 0 0 50000 Y
>>
>> for divider width 5bit min adc clock is 24Mhz / 32 = 750KHZ
>> for divider width 6bit min adc clock is 24Mhz / 64 = 375KHz
>>
>> I suppose that the lower adc clock rate the higher measurement time
>> so I need to get measurement time at both clk freqs and the times differ so
>> 6bit divider is really applied
>>
>> I performed test at Odroid-C1, kernel 6.2-rc8
>> Two kernel patches must be applied:
>>
>> the topic starter patch and the helper patch at the end of the letter
>> In the helper patch I turn on CLOCK_ALLOW_WRITE_DEBUGFS to change clock rate from she shell
>> and use ktime_get_raw_ts64 to measure measurement time
>>
>> So the the test itself:
>> cat /sys/devices/platform/soc/c1100000.cbus/c1108680.adc/iio:device0/in_voltage3_raw
>> [ 1781.226309] ==== freq: 1142858 time 42408000
>>
>> # echo 750000 > /sys/kernel/debug/clk/c1108680.adc#adc_en/clk_rate
>> # cat /sys/devices/platform/soc/c1100000.cbus/c1108680.adc/iio:device0/in_voltage3_raw
>> [ 1790.728656] ==== freq: 750000 time 49173000
>>
>> # echo 375000 > /sys/kernel/debug/clk/c1108680.adc#adc_en/clk_rate
>> # cat /sys/devices/platform/soc/c1100000.cbus/c1108680.adc/iio:device0/in_voltage3_raw
>> [ 1816.955477] ==== freq: 375000 time 68245000
>>
>> # cat /sys/kernel/debug/clk/clk_summary
>> xtal 4 4 1 24000000 0 0 50000 Y
>> c1108680.adc#adc_div 1 1 0 375000 0 0 50000 Y
>> c1108680.adc#adc_en 1 1 0 375000 0 0 50000 Y
> These results looks excellent - thanks for sharing the test results!
> Could you please check one last thing:
> $ grep -i adc /sys/kernel/debug/meson-clk-msr/measure_summary
> It should confirm that the clock rate is 375kHz (or close to it, SoC
> internal clock measurement is not 100% precise)
Hello Martin
Looks like it works as expected:
# grep -i adc /sys/kernel/debug/meson-clk-msr/measure_summary
sar_adc 1140625 +/-3125Hz
# echo 375000 > /sys/kernel/debug/clk/c1108680.adc#adc_en/clk_rate
# grep -i adc /sys/kernel/debug/meson-clk-msr/measure_summary
sar_adc 371875 +/-3125Hz
So I'm re-sending the patch with fixed commit message
--
Best regards
George
>
> Once we have that confirmation: can you please re-send the patch with
> the description updated so it's clear which SoC generations are
> affected and by stating that the fix was tested on a Meson8b Odroid-C1
> board.
>
>
> Thank you and best regards,
> Martin