| Message ID | 20230515210545.2100161-1-gnstark@sberdevices.ru (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v1] meson saradc: fix clock divider mask length | expand |
On 15/05/2023 23:05, George Stark wrote: > From: George Stark <GNStark@sberdevices.ru> > > 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 <GNStark@sberdevices.ru> > --- > 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 <neil.armstrong@linaro.org>
Hi George, thank you for this patch! On Mon, May 15, 2023 at 11:06 PM George Stark <gnstark@sberdevices.ru> wrote: > > From: George Stark <GNStark@sberdevices.ru> > > 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 <GNStark@sberdevices.ru> Since my question above doesn't affect this patch: Reviewed-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
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 <gnstark@sberdevices.ru> wrote: > > > > From: George Stark <GNStark@sberdevices.ru> > > > > 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 <GNStark@sberdevices.ru> > Since my question above doesn't affect this patch: > Reviewed-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
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@sberdevices.ru> wrote: >> >> From: George Stark <GNStark@sberdevices.ru> >> >> 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 <GNStark@sberdevices.ru> > Since my question above doesn't affect this patch: > Reviewed-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > > _______________________________________________ > linux-amlogic mailing list > linux-amlogic@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-amlogic
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 <gnstark@sberdevices.ru> wrote: >> From: George Stark <GNStark@sberdevices.ru> >> >> 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 <GNStark@sberdevices.ru> > Since my question above doesn't affect this patch: > Reviewed-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> >
Hello Vyacheslav, George and Dmitry, On Wed, May 17, 2023 at 1:37 PM Vyacheslav <adeep@lexina.in> 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@sberdevices.ru> wrote: > >> > >> From: George Stark <GNStark@sberdevices.ru> > >> > >> 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@sberdevices.ru/T/#u
On Wed, 17 May 2023 16:47:59 +0000 Старк Георгий Николаевич <GNStark@sberdevices.ru> 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 <gnstark@sberdevices.ru> wrote: > >> From: George Stark <GNStark@sberdevices.ru> > >> > >> 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 <GNStark@sberdevices.ru> > > Since my question above doesn't affect this patch: > > Reviewed-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > > >
On Sat, May 20, 2023 at 6:30 PM Jonathan Cameron <jic23@kernel.org> wrote: > On Wed, 17 May 2023 16:47:59 +0000 > Старк Георгий Николаевич <GNStark@sberdevices.ru> 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.
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 <adeep@lexina.in> 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@sberdevices.ru> wrote: > > >> > > >> From: George Stark <GNStark@sberdevices.ru> > > >> > > >> 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@sberdevices.ru/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.
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 > Старк Георгий Николаевич <GNStark@sberdevices.ru> 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 <gnstark@sberdevices.ru> wrote: > > >> From: George Stark <GNStark@sberdevices.ru> > > >> > > >> 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. [...]
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@lexina.in> 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@sberdevices.ru> wrote: >>>> From: George Stark <GNStark@sberdevices.ru> >>>> >>>> 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@sberdevices.ru/T/#u >
Hi George, On Mon, May 22, 2023 at 5:47 PM Старк Георгий Николаевич <GNStark@sberdevices.ru> 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 Старк Георгий Николаевич > <GNStark@sberdevices.ru> 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 Старк Георгий Николаевич > <GNStark@sberdevices.ru> 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 <gnstark@sberdevices.ru> 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 <gnstark@sberdevices.ru> 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
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)