[RFC PATCH v1 0/2] iio: adc: meson: add MPLL clock workaround for GXLX

[RFC PATCH v1 0/2] iio: adc: meson: add MPLL clock workaround for GXLX

[Martin Blumenstingl]

Hello,

Amlogic GXLX SoCs seem to be mostly the same silicon as GXL. The only
known differences are:
- one less Mali-450 GPU core
- no VP9 codec
- and an odd one: the three MPLL clocks need a bit toggled in the SAR
  ADC register space

This series attempt to fix audio output (which relies on the MPLL
clocks) on the GXLX boards. Unfortunately all we have is a downstream
commit [0] without any further explanation (or anyone who wants to
provide details on this). Since it's not clear if this is a gate, a
reset or some other hardware fix: the driver side includes a warning
for users to update their .dtb along with kernel images in case we
ever figure out what these bits do and how to model them properly.


[0] https://github.com/khadas/linux/commit/d1d98f2ed8c83eb42af8880ed8e206aa402dd70a#diff-c5aaf54323ef93777c5083de37f933058ea8d0af79a1941e0b5a0667dc0f89b3


Martin Blumenstingl (2):
  dt-bindings: iio: adc: amlogic,meson-saradc: Add GXLX SoC compatible
  iio: adc: meson: add support for the GXLX SoC

 .../iio/adc/amlogic,meson-saradc.yaml         |  1 +
 drivers/iio/adc/meson_saradc.c                | 34 +++++++++++++++++++
 2 files changed, 35 insertions(+)

-- 
2.47.1

[RFC PATCH v1 1/2] dt-bindings: iio: adc: amlogic,meson-saradc: Add GXLX SoC compatible

[Martin Blumenstingl]

Add a compatible string for the GXLX SoC. It's very similar to GXL but
has three additional bits in MESON_SAR_ADC_REG12 for the three MPLL
clocks.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 .../devicetree/bindings/iio/adc/amlogic,meson-saradc.yaml        | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/amlogic,meson-saradc.yaml b/Documentation/devicetree/bindings/iio/adc/amlogic,meson-saradc.yaml
index b0962a4583ac..bb9825e7346d 100644
--- a/Documentation/devicetree/bindings/iio/adc/amlogic,meson-saradc.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/amlogic,meson-saradc.yaml
@@ -23,6 +23,7 @@ properties:
               - amlogic,meson8m2-saradc
               - amlogic,meson-gxbb-saradc
               - amlogic,meson-gxl-saradc
+              - amlogic,meson-gxlx-saradc
               - amlogic,meson-gxm-saradc
               - amlogic,meson-axg-saradc
               - amlogic,meson-g12a-saradc
-- 
2.47.1

Re: [RFC PATCH v1 1/2] dt-bindings: iio: adc: amlogic,meson-saradc: Add GXLX SoC compatible

[Krzysztof Kozlowski]

On Tue, Dec 31, 2024 at 08:42:06PM +0100, Martin Blumenstingl wrote:
> Add a compatible string for the GXLX SoC. It's very similar to GXL but
> has three additional bits in MESON_SAR_ADC_REG12 for the three MPLL
> clocks.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  .../devicetree/bindings/iio/adc/amlogic,meson-saradc.yaml        | 1 +
>  1 file changed, 1 insertion(+)

Acked-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof

[RFC PATCH v1 2/2] iio: adc: meson: add support for the GXLX SoC

[Martin Blumenstingl]

The SARADC IP on the GXLX SoC itself is identical to the one found on
GXL SoCs. However, GXLX SoCs require poking the first three bits in the
MESON_SAR_ADC_REG12 register to get the three MPLL clocks (used as clock
generators for the audio frequencies) to work.

The reason why there are MPLL clock bits in the ADC register space is
entirely unknown and it seems that nobody is able to comment on this.
So clearly mark this as a workaround and add a warning so users are
notified that this workaround can change (once we know what these bits
actually do).

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/iio/adc/meson_saradc.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c
index 997def4a4d2f..df15a2566ec7 100644
--- a/drivers/iio/adc/meson_saradc.c
+++ b/drivers/iio/adc/meson_saradc.c
@@ -160,6 +160,11 @@
 	#define MESON_SAR_ADC_REG11_EOC				BIT(1)
 	#define MESON_SAR_ADC_REG11_VREF_SEL			BIT(0)
 
+#define MESON_SAR_ADC_REG12					0x30
+	#define MESON_SAR_ADC_REG12_MPLL0_UNKNOWN		BIT(0)
+	#define MESON_SAR_ADC_REG12_MPLL1_UNKNOWN		BIT(1)
+	#define MESON_SAR_ADC_REG12_MPLL2_UNKNOWN		BIT(2)
+
 #define MESON_SAR_ADC_REG13					0x34
 	#define MESON_SAR_ADC_REG13_12BIT_CALIBRATION_MASK	GENMASK(13, 8)
 
@@ -326,6 +331,7 @@ struct meson_sar_adc_param {
 	u8					cmv_select;
 	u8					adc_eoc;
 	enum meson_sar_adc_vref_sel		vref_voltage;
+	bool					enable_mpll_clock_workaround;
 };
 
 struct meson_sar_adc_data {
@@ -995,6 +1001,15 @@ static int meson_sar_adc_init(struct iio_dev *indio_dev)
 				    priv->param->cmv_select);
 		regmap_update_bits(priv->regmap, MESON_SAR_ADC_REG11,
 				   MESON_SAR_ADC_REG11_CMV_SEL, regval);
+
+		if (priv->param->enable_mpll_clock_workaround) {
+			dev_warn(dev,
+				 "Enabling unknown bits to make the MPLL clocks work. This may change so always update dtbs and kernel together\n");
+			regmap_write(priv->regmap, MESON_SAR_ADC_REG12,
+				     MESON_SAR_ADC_REG12_MPLL0_UNKNOWN |
+				     MESON_SAR_ADC_REG12_MPLL1_UNKNOWN |
+				     MESON_SAR_ADC_REG12_MPLL2_UNKNOWN);
+		}
 	}
 
 	ret = clk_set_parent(priv->adc_sel_clk, priv->clkin);
@@ -1219,6 +1234,17 @@ static const struct meson_sar_adc_param meson_sar_adc_gxl_param = {
 	.cmv_select = 1,
 };
 
+static const struct meson_sar_adc_param meson_sar_adc_gxlx_param = {
+	.has_bl30_integration = true,
+	.clock_rate = 1200000,
+	.regmap_config = &meson_sar_adc_regmap_config_gxbb,
+	.resolution = 12,
+	.disable_ring_counter = 1,
+	.vref_voltage = VREF_VOLTAGE_1V8,
+	.cmv_select = true,
+	.enable_mpll_clock_workaround = true,
+};
+
 static const struct meson_sar_adc_param meson_sar_adc_axg_param = {
 	.has_bl30_integration = true,
 	.clock_rate = 1200000,
@@ -1267,6 +1293,11 @@ static const struct meson_sar_adc_data meson_sar_adc_gxl_data = {
 	.name = "meson-gxl-saradc",
 };
 
+static const struct meson_sar_adc_data meson_sar_adc_gxlx_data = {
+	.param = &meson_sar_adc_gxlx_param,
+	.name = "meson-gxlx-saradc",
+};
+
 static const struct meson_sar_adc_data meson_sar_adc_gxm_data = {
 	.param = &meson_sar_adc_gxl_param,
 	.name = "meson-gxm-saradc",
@@ -1298,6 +1329,9 @@ static const struct of_device_id meson_sar_adc_of_match[] = {
 	}, {
 		.compatible = "amlogic,meson-gxl-saradc",
 		.data = &meson_sar_adc_gxl_data,
+	}, {
+		.compatible = "amlogic,meson-gxlx-saradc",
+		.data = &meson_sar_adc_gxlx_data,
 	}, {
 		.compatible = "amlogic,meson-gxm-saradc",
 		.data = &meson_sar_adc_gxm_data,
-- 
2.47.1

Re: [RFC PATCH v1 2/2] iio: adc: meson: add support for the GXLX SoC

[Jonathan Cameron]

On Tue, 31 Dec 2024 20:42:07 +0100
Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:

> The SARADC IP on the GXLX SoC itself is identical to the one found on
> GXL SoCs. However, GXLX SoCs require poking the first three bits in the
> MESON_SAR_ADC_REG12 register to get the three MPLL clocks (used as clock
> generators for the audio frequencies) to work.
> 
> The reason why there are MPLL clock bits in the ADC register space is
> entirely unknown and it seems that nobody is able to comment on this.
> So clearly mark this as a workaround and add a warning so users are
> notified that this workaround can change (once we know what these bits
> actually do).

So IIUC this is to make some non ADC component work.
How are you handling dependencies?  The ADC driver might not be loaded or
is there some reason it definitely is at the point where the audio driver
loads?

Jonathan

> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  drivers/iio/adc/meson_saradc.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c
> index 997def4a4d2f..df15a2566ec7 100644
> --- a/drivers/iio/adc/meson_saradc.c
> +++ b/drivers/iio/adc/meson_saradc.c
> @@ -160,6 +160,11 @@
>  	#define MESON_SAR_ADC_REG11_EOC				BIT(1)
>  	#define MESON_SAR_ADC_REG11_VREF_SEL			BIT(0)
>  
> +#define MESON_SAR_ADC_REG12					0x30
> +	#define MESON_SAR_ADC_REG12_MPLL0_UNKNOWN		BIT(0)
> +	#define MESON_SAR_ADC_REG12_MPLL1_UNKNOWN		BIT(1)
> +	#define MESON_SAR_ADC_REG12_MPLL2_UNKNOWN		BIT(2)
> +
>  #define MESON_SAR_ADC_REG13					0x34
>  	#define MESON_SAR_ADC_REG13_12BIT_CALIBRATION_MASK	GENMASK(13, 8)
>  
> @@ -326,6 +331,7 @@ struct meson_sar_adc_param {
>  	u8					cmv_select;
>  	u8					adc_eoc;
>  	enum meson_sar_adc_vref_sel		vref_voltage;
> +	bool					enable_mpll_clock_workaround;
>  };
>  
>  struct meson_sar_adc_data {
> @@ -995,6 +1001,15 @@ static int meson_sar_adc_init(struct iio_dev *indio_dev)
>  				    priv->param->cmv_select);
>  		regmap_update_bits(priv->regmap, MESON_SAR_ADC_REG11,
>  				   MESON_SAR_ADC_REG11_CMV_SEL, regval);
> +
> +		if (priv->param->enable_mpll_clock_workaround) {
> +			dev_warn(dev,
> +				 "Enabling unknown bits to make the MPLL clocks work. This may change so always update dtbs and kernel together\n");
> +			regmap_write(priv->regmap, MESON_SAR_ADC_REG12,
> +				     MESON_SAR_ADC_REG12_MPLL0_UNKNOWN |
> +				     MESON_SAR_ADC_REG12_MPLL1_UNKNOWN |
> +				     MESON_SAR_ADC_REG12_MPLL2_UNKNOWN);
> +		}
>  	}
>  
>  	ret = clk_set_parent(priv->adc_sel_clk, priv->clkin);
> @@ -1219,6 +1234,17 @@ static const struct meson_sar_adc_param meson_sar_adc_gxl_param = {
>  	.cmv_select = 1,
>  };
>  
> +static const struct meson_sar_adc_param meson_sar_adc_gxlx_param = {
> +	.has_bl30_integration = true,
> +	.clock_rate = 1200000,
> +	.regmap_config = &meson_sar_adc_regmap_config_gxbb,
> +	.resolution = 12,
> +	.disable_ring_counter = 1,
> +	.vref_voltage = VREF_VOLTAGE_1V8,
> +	.cmv_select = true,
> +	.enable_mpll_clock_workaround = true,
> +};
> +
>  static const struct meson_sar_adc_param meson_sar_adc_axg_param = {
>  	.has_bl30_integration = true,
>  	.clock_rate = 1200000,
> @@ -1267,6 +1293,11 @@ static const struct meson_sar_adc_data meson_sar_adc_gxl_data = {
>  	.name = "meson-gxl-saradc",
>  };
>  
> +static const struct meson_sar_adc_data meson_sar_adc_gxlx_data = {
> +	.param = &meson_sar_adc_gxlx_param,
> +	.name = "meson-gxlx-saradc",
> +};
> +
>  static const struct meson_sar_adc_data meson_sar_adc_gxm_data = {
>  	.param = &meson_sar_adc_gxl_param,
>  	.name = "meson-gxm-saradc",
> @@ -1298,6 +1329,9 @@ static const struct of_device_id meson_sar_adc_of_match[] = {
>  	}, {
>  		.compatible = "amlogic,meson-gxl-saradc",
>  		.data = &meson_sar_adc_gxl_data,
> +	}, {
> +		.compatible = "amlogic,meson-gxlx-saradc",
> +		.data = &meson_sar_adc_gxlx_data,
>  	}, {
>  		.compatible = "amlogic,meson-gxm-saradc",
>  		.data = &meson_sar_adc_gxm_data,

Re: [RFC PATCH v1 2/2] iio: adc: meson: add support for the GXLX SoC

[Martin Blumenstingl]

Hi Jonathan,

On Sat, Jan 4, 2025 at 2:59 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Tue, 31 Dec 2024 20:42:07 +0100
> Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:
>
> > The SARADC IP on the GXLX SoC itself is identical to the one found on
> > GXL SoCs. However, GXLX SoCs require poking the first three bits in the
> > MESON_SAR_ADC_REG12 register to get the three MPLL clocks (used as clock
> > generators for the audio frequencies) to work.
> >
> > The reason why there are MPLL clock bits in the ADC register space is
> > entirely unknown and it seems that nobody is able to comment on this.
> > So clearly mark this as a workaround and add a warning so users are
> > notified that this workaround can change (once we know what these bits
> > actually do).
>
> So IIUC this is to make some non ADC component work.
That's correct

> How are you handling dependencies?  The ADC driver might not be loaded or
> is there some reason it definitely is at the point where the audio driver
> loads?
Unfortunately there are no dependencies at the moment.
To me it's not even 100% clear if those bits are a dependency for the
audio IP or if they are instead linked with the clock controller (more
background info: some of the MPLL clocks are - at least in theory, in
practice we don't use that - are also used as input for the Mali GPU
and video subsystem. The only practical usage that I'm aware of is the
audio controller).

Christian and I have both tried with all of our contacts at Amlogic
but did not get any answers.
If I knew the purpose of these bits I'd model them as whatever they
are (resets, clock gates, ...) and provide proper dt-bindings.


Best regards,
Martin

Re: [RFC PATCH v1 2/2] iio: adc: meson: add support for the GXLX SoC

[Christian Hewitt]

> On 5 Jan 2025, at 7:49 pm, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:
> 
> Hi Jonathan,
> 
> On Sat, Jan 4, 2025 at 2:59 PM Jonathan Cameron <jic23@kernel.org> wrote:
>> 
>> On Tue, 31 Dec 2024 20:42:07 +0100
>> Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:
>> 
>>> The SARADC IP on the GXLX SoC itself is identical to the one found on
>>> GXL SoCs. However, GXLX SoCs require poking the first three bits in the
>>> MESON_SAR_ADC_REG12 register to get the three MPLL clocks (used as clock
>>> generators for the audio frequencies) to work.
>>> 
>>> The reason why there are MPLL clock bits in the ADC register space is
>>> entirely unknown and it seems that nobody is able to comment on this.
>>> So clearly mark this as a workaround and add a warning so users are
>>> notified that this workaround can change (once we know what these bits
>>> actually do).
>> 
>> So IIUC this is to make some non ADC component work.
> That's correct
> 
>> How are you handling dependencies?  The ADC driver might not be loaded or
>> is there some reason it definitely is at the point where the audio driver
>> loads?
> Unfortunately there are no dependencies at the moment.
> To me it's not even 100% clear if those bits are a dependency for the
> audio IP or if they are instead linked with the clock controller (more
> background info: some of the MPLL clocks are - at least in theory, in
> practice we don't use that - are also used as input for the Mali GPU
> and video subsystem. The only practical usage that I'm aware of is the
> audio controller).

In my testing it makes no difference to the audio driver when the adc
bit poke is done. The audio driver probes and loads and you can play
media without generating any visible errors. There’s just no audible
output on GXLX until the poke - it’s like hitting an un-mute button.

For the series:

Tested-by: Christian Hewitt <christianshewitt@gmail.com>

Christian

> Christian and I have both tried with all of our contacts at Amlogic
> but did not get any answers.
> If I knew the purpose of these bits I'd model them as whatever they
> are (resets, clock gates, ...) and provide proper dt-bindings.
> 
> 
> Best regards,
> Martin

Re: [RFC PATCH v1 2/2] iio: adc: meson: add support for the GXLX SoC

[Martin Blumenstingl]

On Tue, Dec 31, 2024 at 8:42 PM Martin Blumenstingl
<martin.blumenstingl@googlemail.com> wrote:
[...]
> +static const struct meson_sar_adc_param meson_sar_adc_gxlx_param = {
> +       .has_bl30_integration = true,
> +       .clock_rate = 1200000,
> +       .regmap_config = &meson_sar_adc_regmap_config_gxbb,
> +       .resolution = 12,
> +       .disable_ring_counter = 1,
> +       .vref_voltage = VREF_VOLTAGE_1V8,
I just noticed that I did not properly rebase this locally.
VREF_VOLTAGE_1V8 only exists in a local patch of mine (to be sent
later, I assume Christian also has it in his tree which is why it
wasn't noticed).
Before this can be applied I need to change it to 1.
I'll do this once the discussion is over and then send a v2.

In other words:
Jonathan, please don't merge v1.

Re: [RFC PATCH v1 0/2] iio: adc: meson: add MPLL clock workaround for GXLX

[neil.armstrong]

On 31/12/2024 20:42, Martin Blumenstingl wrote:
> Hello,
> 
> Amlogic GXLX SoCs seem to be mostly the same silicon as GXL. The only
> known differences are:
> - one less Mali-450 GPU core
> - no VP9 codec
> - and an odd one: the three MPLL clocks need a bit toggled in the SAR
>    ADC register space
> 
> This series attempt to fix audio output (which relies on the MPLL
> clocks) on the GXLX boards. Unfortunately all we have is a downstream
> commit [0] without any further explanation (or anyone who wants to
> provide details on this). Since it's not clear if this is a gate, a
> reset or some other hardware fix: the driver side includes a warning
> for users to update their .dtb along with kernel images in case we
> ever figure out what these bits do and how to model them properly.
> 
> 
> [0] https://github.com/khadas/linux/commit/d1d98f2ed8c83eb42af8880ed8e206aa402dd70a#diff-c5aaf54323ef93777c5083de37f933058ea8d0af79a1941e0b5a0667dc0f89b3
> 
> 
> Martin Blumenstingl (2):
>    dt-bindings: iio: adc: amlogic,meson-saradc: Add GXLX SoC compatible
>    iio: adc: meson: add support for the GXLX SoC
> 
>   .../iio/adc/amlogic,meson-saradc.yaml         |  1 +
>   drivers/iio/adc/meson_saradc.c                | 34 +++++++++++++++++++
>   2 files changed, 35 insertions(+)
> 

LGTM

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>