[PATCH] arm64: dts: amlogic: gxl: set i2c bias to pull-up

[PATCH] arm64: dts: amlogic: gxl: set i2c bias to pull-up

[Da Xue]

GXL I2C pins need internal pull-up enabled to operate if there
is no external resistor. The pull-up is 60kohms per the datasheet.

We should set the bias when i2c pinmux is enabled.

Signed-off-by: Da Xue <da@libre.computer>
---
 arch/arm64/boot/dts/amlogic/meson-gxl.dtsi | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi b/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi
index 2dc2fdaecf9f..aed8dbfbb64d 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi
@@ -214,7 +214,7 @@ mux {
 				groups = "i2c_sck_ao",
 				       "i2c_sda_ao";
 				function = "i2c_ao";
-				bias-disable;
+				bias-pull-up;
 			};
 		};
 
@@ -576,7 +576,7 @@ mux {
 				groups = "i2c_sck_a",
 				     "i2c_sda_a";
 				function = "i2c_a";
-				bias-disable;
+				bias-pull-up;
 			};
 		};
 
@@ -585,7 +585,7 @@ mux {
 				groups = "i2c_sck_b",
 				      "i2c_sda_b";
 				function = "i2c_b";
-				bias-disable;
+				bias-pull-up;
 			};
 		};
 
@@ -594,7 +594,7 @@ mux {
 				groups = "i2c_sck_c",
 				      "i2c_sda_c";
 				function = "i2c_c";
-				bias-disable;
+				bias-pull-up;
 			};
 		};
 
@@ -603,7 +603,7 @@ mux {
 				groups = "i2c_sck_c_dv19",
 				      "i2c_sda_c_dv18";
 				function = "i2c_c";
-				bias-disable;
+				bias-pull-up;
 			};
 		};
 
-- 
2.39.5

Re: [PATCH] arm64: dts: amlogic: gxl: set i2c bias to pull-up

[Martin Blumenstingl]

On Fri, Apr 25, 2025 at 10:31 PM Da Xue <da@libre.computer> wrote:
>
> GXL I2C pins need internal pull-up enabled to operate if there
> is no external resistor. The pull-up is 60kohms per the datasheet.
>
> We should set the bias when i2c pinmux is enabled.
>
> Signed-off-by: Da Xue <da@libre.computer>
Reviewed-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

+Xianwei Zhao (who has recently upstreamed Amlogic A4 pinctrl support).
I suspect we need a similar change for all other (Meson8, Meson8b,
GXBB, G12A, ...) SoCs as well.
Can you confirm this? And if not, why does only GXL need this special treatment?


Best regards,
Martin

Re: [PATCH] arm64: dts: amlogic: gxl: set i2c bias to pull-up

[Xianwei Zhao]

Hi Martin,

On 2025/4/28 05:08, Martin Blumenstingl wrote:
> [ EXTERNAL EMAIL ]
> 
> On Fri, Apr 25, 2025 at 10:31 PM Da Xue <da@libre.computer> wrote:
>>
>> GXL I2C pins need internal pull-up enabled to operate if there
>> is no external resistor. The pull-up is 60kohms per the datasheet.
>>
>> We should set the bias when i2c pinmux is enabled.
>>
>> Signed-off-by: Da Xue <da@libre.computer>
> Reviewed-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> 
> +Xianwei Zhao (who has recently upstreamed Amlogic A4 pinctrl support).
> I suspect we need a similar change for all other (Meson8, Meson8b,
> GXBB, G12A, ...) SoCs as well.
> Can you confirm this? And if not, why does only GXL need this special treatment?
> 

The A4 pinctrl driver mainly differs in the implementation method, and 
the differences between chips are primarily described in the DTS, rather 
than requiring the submission of bindings and code each time a new SoC 
pinctrl driver support is added. In theory, previous chips could also be 
supported by new drivers.

> 
> Best regards,
> Martin

Re: [PATCH] arm64: dts: amlogic: gxl: set i2c bias to pull-up

[Da Xue]

On Sun, Apr 27, 2025 at 5:11 PM Martin Blumenstingl
<martin.blumenstingl@googlemail.com> wrote:
>
> On Fri, Apr 25, 2025 at 10:31 PM Da Xue <da@libre.computer> wrote:
> >
> > GXL I2C pins need internal pull-up enabled to operate if there
> > is no external resistor. The pull-up is 60kohms per the datasheet.
> >
> > We should set the bias when i2c pinmux is enabled.
> >
> > Signed-off-by: Da Xue <da@libre.computer>
> Reviewed-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>
> +Xianwei Zhao (who has recently upstreamed Amlogic A4 pinctrl support).
> I suspect we need a similar change for all other (Meson8, Meson8b,
> GXBB, G12A, ...) SoCs as well.
> Can you confirm this? And if not, why does only GXL need this special treatment?

We've only tested this extensively on GXL (S805X, S905D, S905X). I
think we should enable it for all SoCs if this patch doesn't cause any
issues. I don't see most Amlogic boards implement pull-ups but I
haven't tested that as extensively as I've done for these.

>
>
> Best regards,
> Martin
>
> _______________________________________________
> linux-amlogic mailing list
> linux-amlogic@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH] arm64: dts: amlogic: gxl: set i2c bias to pull-up

[Neil Armstrong]

On 25/04/2025 22:31, Da Xue wrote:
> GXL I2C pins need internal pull-up enabled to operate if there
> is no external resistor. The pull-up is 60kohms per the datasheet.
> 
> We should set the bias when i2c pinmux is enabled.

So, yes in some cases when the on-board pull-up is missing, the on-pad
pull-up is required, but the whole idea was to only add the pull-up property
when needed.

So I know the real motivation is again about the 40pin headers, where
some applications don't add a pull-up and still want to have i2c working.

So my question is: why can't the pull-up property be added in overlays ?

Neil

> 
> Signed-off-by: Da Xue <da@libre.computer>
> ---
>   arch/arm64/boot/dts/amlogic/meson-gxl.dtsi | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi b/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi
> index 2dc2fdaecf9f..aed8dbfbb64d 100644
> --- a/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi
> +++ b/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi
> @@ -214,7 +214,7 @@ mux {
>   				groups = "i2c_sck_ao",
>   				       "i2c_sda_ao";
>   				function = "i2c_ao";
> -				bias-disable;
> +				bias-pull-up;
>   			};
>   		};
>   
> @@ -576,7 +576,7 @@ mux {
>   				groups = "i2c_sck_a",
>   				     "i2c_sda_a";
>   				function = "i2c_a";
> -				bias-disable;
> +				bias-pull-up;
>   			};
>   		};
>   
> @@ -585,7 +585,7 @@ mux {
>   				groups = "i2c_sck_b",
>   				      "i2c_sda_b";
>   				function = "i2c_b";
> -				bias-disable;
> +				bias-pull-up;
>   			};
>   		};
>   
> @@ -594,7 +594,7 @@ mux {
>   				groups = "i2c_sck_c",
>   				      "i2c_sda_c";
>   				function = "i2c_c";
> -				bias-disable;
> +				bias-pull-up;
>   			};
>   		};
>   
> @@ -603,7 +603,7 @@ mux {
>   				groups = "i2c_sck_c_dv19",
>   				      "i2c_sda_c_dv18";
>   				function = "i2c_c";
> -				bias-disable;
> +				bias-pull-up;
>   			};
>   		};
>   

Re: [PATCH] arm64: dts: amlogic: gxl: set i2c bias to pull-up

[Da Xue]

On Mon, Apr 28, 2025 at 3:50 AM Neil Armstrong
<neil.armstrong@linaro.org> wrote:
>
> On 25/04/2025 22:31, Da Xue wrote:
> > GXL I2C pins need internal pull-up enabled to operate if there
> > is no external resistor. The pull-up is 60kohms per the datasheet.
> >
> > We should set the bias when i2c pinmux is enabled.
>
> So, yes in some cases when the on-board pull-up is missing, the on-pad
> pull-up is required, but the whole idea was to only add the pull-up property
> when needed.
>
> So I know the real motivation is again about the 40pin headers, where
> some applications don't add a pull-up and still want to have i2c working.
>
> So my question is: why can't the pull-up property be added in overlays ?

The issue is the property types. I wish the bias was bias = <PULL_UP>
instead of bias-disabled, bias-pull-up, bias-pull-down since we have
to hack a bunch of /delete-property/ in the overlays. A lot of the
merging tools ignore /delete-property/. This is a convenience patch
which may cause push-pull times to change by an insignificant amount.
We have been carrying this patch out-of-tree for 5+ years without
issues. I have not seen any design on GXL that had a PU for I2C.
Externally, I've seen threads of people asking why I2C does not work
on other boards.

>
> Neil
>
> >
> > Signed-off-by: Da Xue <da@libre.computer>
> > ---
> >   arch/arm64/boot/dts/amlogic/meson-gxl.dtsi | 10 +++++-----
> >   1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi b/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi
> > index 2dc2fdaecf9f..aed8dbfbb64d 100644
> > --- a/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi
> > +++ b/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi
> > @@ -214,7 +214,7 @@ mux {
> >                               groups = "i2c_sck_ao",
> >                                      "i2c_sda_ao";
> >                               function = "i2c_ao";
> > -                             bias-disable;
> > +                             bias-pull-up;
> >                       };
> >               };
> >
> > @@ -576,7 +576,7 @@ mux {
> >                               groups = "i2c_sck_a",
> >                                    "i2c_sda_a";
> >                               function = "i2c_a";
> > -                             bias-disable;
> > +                             bias-pull-up;
> >                       };
> >               };
> >
> > @@ -585,7 +585,7 @@ mux {
> >                               groups = "i2c_sck_b",
> >                                     "i2c_sda_b";
> >                               function = "i2c_b";
> > -                             bias-disable;
> > +                             bias-pull-up;
> >                       };
> >               };
> >
> > @@ -594,7 +594,7 @@ mux {
> >                               groups = "i2c_sck_c",
> >                                     "i2c_sda_c";
> >                               function = "i2c_c";
> > -                             bias-disable;
> > +                             bias-pull-up;
> >                       };
> >               };
> >
> > @@ -603,7 +603,7 @@ mux {
> >                               groups = "i2c_sck_c_dv19",
> >                                     "i2c_sda_c_dv18";
> >                               function = "i2c_c";
> > -                             bias-disable;
> > +                             bias-pull-up;
> >                       };
> >               };
> >
>
>
> _______________________________________________
> linux-amlogic mailing list
> linux-amlogic@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH] arm64: dts: amlogic: gxl: set i2c bias to pull-up

[Martin Blumenstingl]

On Mon, Apr 28, 2025 at 9:32 AM Neil Armstrong
<neil.armstrong@linaro.org> wrote:
>
> On 25/04/2025 22:31, Da Xue wrote:
> > GXL I2C pins need internal pull-up enabled to operate if there
> > is no external resistor. The pull-up is 60kohms per the datasheet.
> >
> > We should set the bias when i2c pinmux is enabled.
>
> So, yes in some cases when the on-board pull-up is missing, the on-pad
> pull-up is required, but the whole idea was to only add the pull-up property
> when needed.
>
> So I know the real motivation is again about the 40pin headers, where
> some applications don't add a pull-up and still want to have i2c working.
For SD/eMMC, SPI and UART we have pull up/down enabled in meson-gxl.dtsi.
I can't recall if I have asked this before: what's the rule (of thumb)
for having bias-pull-up/down vs. bias-disable in .dtsi files?

In the past I had to track down incorrect bias.
It gets especially tricky when we don't have schematics and there's no
u-boot sources available from the vendor (I know, the latter shouldn't
be possible - but that's the world we live in unfortunately).
It means that we can end up with one of four cases for the bias settings:
- there's an actual resistor on the PCB (pulling the pin up/down)
- hardware (SoC) default for the pin
- vendor u-boot configures bias for the pin (but we don't know because
we don't have the sources)
- vendor Linux configures bias for the pin (at least we can dump the
.dtb and hope that the bias setting is in there)

That's why I lean towards having sane defaults in the SoC.dtsi (it
seems that rockchip does the same?).


Best regards,
Martin

Re: [PATCH] arm64: dts: amlogic: gxl: set i2c bias to pull-up

[neil.armstrong]

On 04/05/2025 23:03, Martin Blumenstingl wrote:
> On Mon, Apr 28, 2025 at 9:32 AM Neil Armstrong
> <neil.armstrong@linaro.org> wrote:
>>
>> On 25/04/2025 22:31, Da Xue wrote:
>>> GXL I2C pins need internal pull-up enabled to operate if there
>>> is no external resistor. The pull-up is 60kohms per the datasheet.
>>>
>>> We should set the bias when i2c pinmux is enabled.
>>
>> So, yes in some cases when the on-board pull-up is missing, the on-pad
>> pull-up is required, but the whole idea was to only add the pull-up property
>> when needed.
>>
>> So I know the real motivation is again about the 40pin headers, where
>> some applications don't add a pull-up and still want to have i2c working.
> For SD/eMMC, SPI and UART we have pull up/down enabled in meson-gxl.dtsi.
> I can't recall if I have asked this before: what's the rule (of thumb)
> for having bias-pull-up/down vs. bias-disable in .dtsi files?
> 
> In the past I had to track down incorrect bias.
> It gets especially tricky when we don't have schematics and there's no
> u-boot sources available from the vendor (I know, the latter shouldn't
> be possible - but that's the world we live in unfortunately).
> It means that we can end up with one of four cases for the bias settings:
> - there's an actual resistor on the PCB (pulling the pin up/down)
> - hardware (SoC) default for the pin
> - vendor u-boot configures bias for the pin (but we don't know because
> we don't have the sources)
> - vendor Linux configures bias for the pin (at least we can dump the
> .dtb and hope that the bias setting is in there)
> 
> That's why I lean towards having sane defaults in the SoC.dtsi (it
> seems that rockchip does the same?).

Right, but in this case the biases needed for i2c is rather easy to figure out,
and the very weak 60kohms on the pad won't affect i2c if a proper PU is on
the PCB. So I'll pick this one, and yeah a similar one should be needed
for G12/SM1 and newer SoCs.

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

Neil

> 
> 
> Best regards,
> Martin

Re: [PATCH] arm64: dts: amlogic: gxl: set i2c bias to pull-up

[Neil Armstrong]

Hi,

On Fri, 25 Apr 2025 16:31:18 -0400, Da Xue wrote:
> GXL I2C pins need internal pull-up enabled to operate if there
> is no external resistor. The pull-up is 60kohms per the datasheet.
> 
> We should set the bias when i2c pinmux is enabled.
> 
> 

Thanks, Applied to https://git.kernel.org/pub/scm/linux/kernel/git/amlogic/linux.git (v6.16/arm64-dt)

[1/1] arm64: dts: amlogic: gxl: set i2c bias to pull-up
      https://git.kernel.org/amlogic/c/16d4daa00ee69a43a4c79000d40f9271c85f7879

These changes has been applied on the intermediate git tree [1].

The v6.16/arm64-dt branch will then be sent via a formal Pull Request to the Linux SoC maintainers
for inclusion in their intermediate git branches in order to be sent to Linus during
the next merge window, or sooner if it's a set of fixes.

In the cases of fixes, those will be merged in the current release candidate
kernel and as soon they appear on the Linux master branch they will be
backported to the previous Stable and Long-Stable kernels [2].

The intermediate git branches are merged daily in the linux-next tree [3],
people are encouraged testing these pre-release kernels and report issues on the
relevant mailing-lists.

If problems are discovered on those changes, please submit a signed-off-by revert
patch followed by a corrective changeset.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/amlogic/linux.git
[2] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git
[3] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git

-- 
Neil