Re: [PATCH 3/3] ARM: dts: Add support for Samsung Chagallwifi

From: Henrik Grimler
Date: Sat Jan 15 2022 - 15:28:29 EST

Next message: kernel test robot: "arch/arm/mach-ixp4xx/common-pci.c:143:5: warning: no previous prototype for 'ixp4xx_pci_write'"
Previous message: Sergey Shtylyov: "Re: [PATCH 1/2] platform: make platform_get_irq_optional() optional"
In reply to: Krzysztof Kozlowski: "Re: [PATCH 3/3] ARM: dts: Add support for Samsung Chagallwifi"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Krzysztof,

Thanks for the feedback!

On Sat, Jan 15, 2022 at 05:34:28PM +0100, Krzysztof Kozlowski wrote:
> On 13/01/2022 16:40, Henrik Grimler wrote:
> > Chagallwifi, with product name Samsung Galaxy Tab S 10.5", is based on
> > Exynos 5420. This device is one of several tablet models released in
> > 2013 - 2014 based on Exynos 5420.
> >
> > The device tree added here contains support for:
> >
> > - UART
> > - eMMC
> > - SD card
> > - USB
> >
>
> Thanks for the patches. It is a really nice work, good job!
>
> Some comments below.
>
> > CCI has been disabled in the hardware, enabling it would require
> > (de-)soldering a resistor on the board. Trying to boot with it
> > enabled in kernel makes the device hang when CCI is probed.
> > Exynos5420-arndale-octa also has had CCI disabled due to issues [1].
> >
> > To successfully boot the mainline kernel with the stock bootloader
> > (SBOOT), an additional hack is needed [2]. The same hack is also
> > needed to boot exynos4412-i9300 with stock bootloader, and probably
> > other Samsung devices of similar age.
> >
> > [1] https://marc.info/?l=linux-arm-kernel&m=141718639200624
>
> Commits should use 'commit SHA_12char ("subject")' format instead of
> links to mailing lists.

Will fix in v3.

[ ... ]

> > diff --git a/arch/arm/boot/dts/exynos5420-chagallwifi.dts b/arch/arm/boot/dts/exynos5420-chagallwifi.dts
> > new file mode 100644
> > index 000000000000..51eb2bbe6bf6
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/exynos5420-chagallwifi.dts
> > @@ -0,0 +1,57 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Samsung's Exynos5420 Chagallwifi board device tree source
> > + *
> > + * Copyright (c) 2012-2013 Samsung Electronics Co., Ltd.
> > + * http://www.samsung.com
> > + * Copyright (c) 2022 Henrik Grimler
> > + */
> > +
> > +/dts-v1/;
> > +#include "exynos5420-galaxy-tab-common.dtsi"
>
> Do you plan to have more Galaxy Tab versions?

Yes. One more ("klimt-wifi") as soon as issues here have been ironed
out, and possibly LTE-version(s) later if I actually bring it up
to a point where the hardware differences matter for the kernel.

I am also hoping that someone else with access to other tablet
variants wants to work on addng support for additional models, through
the PostmarketOS project.

> > +
> > +/ {
> > + model = "Samsung Chagallwifi based on exynos5420";
>
> "Chagall WiFi"?

Will change filename and descriptions to something like that.

> > + compatible = "samsung,chagallwifi", "samsung,exynos5420", \
> > + "samsung,exynos5";
> > +};
> > +
> > +&s2mps11 {
> > + regulators {
> > + ldo15_reg: LDO15 {
>
> Please define these regulator nodes in DTSI (only name needed and
> comment like you have there) and override them by label (&ldo15_reg { ...).

Will do.

[ ... ]

> > +/dts-v1/;
> > +#include "exynos5420.dtsi"
> > +#include "exynos5420-cpus.dtsi"
> > +#include <dt-bindings/input/input.h>
> > +#include <dt-bindings/gpio/gpio.h>
> > +#include <dt-bindings/clock/samsung,s2mps11.h>
> > +
> > +/ {
> > + compatible = "samsung,exynos5420", "samsung,exynos5";

> Skip the compatible. It duplicates exynos5420.dtsi and does not bring
> any more information here.

Will do.

[ ... ]

> > + fixed-rate-clocks {
> > + oscclk {
> > + compatible = "samsung,exynos5420-oscclk";
> > + clock-frequency = <24000000>;
> > + };
> > +
> > + xxti {
> > + compatible = "samsung,clock-xxti";
> > + clock-frequency = <24000000>;
> > + };
> > +
> > + xusbxti {
> > + compatible = "samsung,clock-xusbxti";
> > + clock-frequency = <24000000>;
> > + };
>
> Just keep one clock - oscclk. We treat it as alias of xxti, even though
> it might not be exactly alias. I am not sure about real differences, but
> anyway the driver does not care about xxti and xusbxti. xusbxti appears
> in Exynos5420 only partially, e.g. not in all places like ballmap. Other
> boards have only oscclk, so let's do the same.

Will do.

> > + };
> > +
> > + gpio-keys {
> > + compatible = "gpio-keys";
> > + pinctrl-names = "default";
> > +
> > + power {
>
> key-power and so on.

Will do.

> > + debounce-interval = <10>;
> > + gpios = <&gpx2 2 GPIO_ACTIVE_LOW>;
> > + label = "Power";
> > + linux,code = <KEY_POWER>;
> > + wakeup-source;
> > + };
> > +
> > + home {
> > + debounce-interval = <10>;
> > + gpios = <&gpx0 5 GPIO_ACTIVE_LOW>;
> > + label = "Home";
> > + linux,code = <KEY_HOME>;
> > + wakeup-source;
> > + };
> > +
> > + volume-up {
> > + debounce-interval = <10>;
> > + gpios = <&gpx0 2 GPIO_ACTIVE_LOW>;
> > + label = "Volume Up";
> > + linux,code = <KEY_VOLUMEUP>;
> > + };
> > +
> > + volume-down {
> > + debounce-interval = <10>;
> > + gpios = <&gpx0 3 GPIO_ACTIVE_LOW>;
> > + label = "Volume Down";
> > + linux,code = <KEY_VOLUMEDOWN>;
> > + };
> > + };
> > +};

[ ... ]

> > +&hsi2c_7 {
> > + status = "okay";
> > +
> > + s2mps11: pmic@66 {
> > + compatible = "samsung,s2mps11-pmic";
> > + reg = <0x66>;
> > +
> > + interrupt-parent = <&gpx3>;
> > + interrupts = <2 IRQ_TYPE_LEVEL_LOW>;
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&s2mps11_irq>;
> > +
> > + s2mps11_osc: clocks {
> > + compatible = "samsung,s2mps11-clk";
> > + #clock-cells = <1>;
> > + clock-output-names = "s2mps11_ap", "s2mps11_cp",
> > + "s2mps11_bt";
> > + };
> > +
> > + buck1_reg: BUCK1 {
>
> All regulators should be under node "regulators". It should not work
> properly without it, e.g. check
> /sys/kernel/debug/regulator/regulator_summary if values match ones you
> define.

Error on my part when re-arranging, will fix, and investigate issue
with usbdrd supplies.

> > + regulator-name = "VDD_MIF_1V1";
> > + regulator-min-microvolt = <700000>;
> > + regulator-max-microvolt = <1300000>;
> > + regulator-always-on;
> > + regulator-boot-on;
> > +
> > + regulator-state-mem {
> > + regulator-off-in-suspend;
> > + regulator-suspend-microvolt = <1100000>;
>
> Property is deprecated and I am not sure what is it's purpose since, the
> regulator is off in suspend.

Will remove it in v3.

> > + };
> > + };
> > +
> > + buck2_reg: BUCK2 {
> > + regulator-name = "VDD_ARM_1V0";
> > + regulator-min-microvolt = <800000>;
> > + regulator-max-microvolt = <1500000>;
> > + regulator-always-on;
> > + regulator-boot-on;
> > + regulator-ramp-delay = <12500>;
> > +
> > + regulator-state-mem {
> > + regulator-off-in-suspend;
> > + };
> > + };
> > +
> > + buck3_reg: BUCK3 {
> > + regulator-name = "VDD_INT_1V0";
> > + regulator-min-microvolt = <800000>;
> > + regulator-max-microvolt = <1400000>;
> > + regulator-always-on;
> > + regulator-boot-on;
> > +
> > + regulator-state-mem {
> > + regulator-off-in-suspend;
> > + regulator-suspend-microvolt = <1100000>;
>
> Same.

[ ... ]

> > + /*
> > + * LDO15 varies between devices and is
> > + * specified in the device dts
> > + */
>
> Do it like ldo25 in arch/arm/boot/dts/exynos4412-midas.dtsi. Comment is
> good.

Will do.

[ ... ]

> > +
> > + ldo30_reg: LDO30 {
> > + regulator-name = "VDD_TOUCH_1V8";
> > + regulator-min-microvolt = <1900000>;
> > + regulator-max-microvolt = <1900000>;
>
> Name is 1.8V, voltage is 1.9V. Double check this one :)

It does indeed look a bit strange. In Samsung's vendor kernel, two
tablet variants uses 1900000, and two 1800000. The two I have access
to both uses 1900000.

> > +
> > + regulator-state-mem {
> > + regulator-off-in-suspend;
> > + };
> > + };
> > +

[ ... ]

> > +/* Internal storage */
> > +&mmc_0 {
> > + status = "okay";
> > + non-removable;
> > + card-detect-delay = <200>;
> > + samsung,dw-mshc-ciu-div = <3>;
> > + samsung,dw-mshc-sdr-timing = <0 4>;
> > + samsung,dw-mshc-ddr-timing = <0 2>;
> > + pinctrl-names = "default", "output";
> > + clk_pin = &sd0_clk;
> > + clk_val = <0x3>;
>
> I think these two are not supported properties.

Indeed, fixed in next version.

> > + pinctrl-0 = <&sd0_clk &sd0_cmd &sd0_bus1 &sd0_bus4 &sd0_bus8>;
> > + vmmc-supply = <&ldo3_reg>;
> > + vqmmc-supply = <&ldo13_reg>;
> > + bus-width = <8>;
> > + cap-mmc-highspeed;
> > + mmc-hs200-1_8v;
> > +};
> > +
> > +/* External sdcard */
> > +&mmc_2 {
> > + status = "okay";
> > + card-detect-delay = <200>;
> > + samsung,dw-mshc-ciu-div = <3>;
> > + samsung,dw-mshc-sdr-timing = <0 4>;
> > + samsung,dw-mshc-ddr-timing = <0 2>;
> > + bus-width = <4>;
> > + cap-sd-highspeed;
> > + sd-uhs-sdr50;
>
> You should define here also pins, something like Arndale Octa has.

Will do.

> > +};
> > +
> > +&pinctrl_0 {
> > + s2mps11_irq: s2mps11-irq-pins {
> > + samsung,pins = "gpx3-2";
> > + samsung,pin-function = <EXYNOS_PIN_FUNC_F>;
> > + samsung,pin-pud = <EXYNOS_PIN_PULL_NONE>;
> > + samsung,pin-drv = <EXYNOS5420_PIN_DRV_LV1>;
> > + };
> > +};
> > +
>
>
> Best regards,
> Krzysztof

Best regards,
Henrik Grimler

Next message: kernel test robot: "arch/arm/mach-ixp4xx/common-pci.c:143:5: warning: no previous prototype for 'ixp4xx_pci_write'"
Previous message: Sergey Shtylyov: "Re: [PATCH 1/2] platform: make platform_get_irq_optional() optional"
In reply to: Krzysztof Kozlowski: "Re: [PATCH 3/3] ARM: dts: Add support for Samsung Chagallwifi"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]