[PATCH 0/3] Enable basic MMC support on Allwinner H6

[PATCH 0/3] Enable basic MMC support on Allwinner H6

[Icenowy Zheng]

Allwinner H6 features 3 SD/MMC controllers. The first and second ones
are similar to the ones on the Allwinner A64/H5 SoCs, and the third one
adds the function to use EMCE (EMbedded Crypto Engine) on the Allwinner
H6 chip to do full-disk encryption.

Enable basic MMC support on Allwinner H6. Higher speed bins are not
supported, similar to A64/H5. EMCE is also not supported, and keeps its
bypassed status by default.

Icenowy Zheng (3):
  mmc: sunxi: add support for the MMC controller on H6
  arm64: allwinner: h6: add device tree nodes for MMC controllers
  arm64: allwinner: h6: enable MMC0/2 on Pine H64

 .../devicetree/bindings/mmc/sunxi-mmc.txt          |  2 +
 .../boot/dts/allwinner/sun50i-h6-pine-h64.dts      | 32 +++++++++++++
 arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi       | 56 ++++++++++++++++++++++
 drivers/mmc/host/sunxi-mmc.c                       | 16 +++++++
 4 files changed, 106 insertions(+)

-- 
2.15.1

[PATCH 1/3] mmc: sunxi: add support for the MMC controller on H6

[Icenowy Zheng]

The new Allwinner H6 SoC have 3 MMC controllers. The first and second
ones are similar to the ones on A64, but the third one adds EMCE
(Embedded Crypto Engine) support which does hardware transparent crypto
on the eMMC.

As we still do not have support for EMCE, and the support of it is
disabled by defualt, we just duplicate the A64 mmc configurations and
change the compatible string.

Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
---
 Documentation/devicetree/bindings/mmc/sunxi-mmc.txt |  2 ++
 drivers/mmc/host/sunxi-mmc.c                        | 16 ++++++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/sunxi-mmc.txt b/Documentation/devicetree/bindings/mmc/sunxi-mmc.txt
index 132e0007d7d6..e6aa5c7a5e12 100644
--- a/Documentation/devicetree/bindings/mmc/sunxi-mmc.txt
+++ b/Documentation/devicetree/bindings/mmc/sunxi-mmc.txt
@@ -16,6 +16,8 @@ Required properties:
    * "allwinner,sun9i-a80-mmc"
    * "allwinner,sun50i-a64-emmc"
    * "allwinner,sun50i-a64-mmc"
+   * "allwinner,sun50i-h6-emmc"
+   * "allwinner,sun50i-h6-mmc"
  - reg : mmc controller base registers
  - clocks : a list with 4 phandle + clock specifier pairs
  - clock-names : must contain "ahb", "mmc", "output" and "sample"
diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
index 97c6b79b7d6f..05e2b5fd7aa4 100644
--- a/drivers/mmc/host/sunxi-mmc.c
+++ b/drivers/mmc/host/sunxi-mmc.c
@@ -1168,6 +1168,20 @@ static const struct sunxi_mmc_cfg sun50i_a64_emmc_cfg = {
 	.can_calibrate = true,
 };
 
+static const struct sunxi_mmc_cfg sun50i_h6_cfg = {
+	.idma_des_size_bits = 16,
+	.clk_delays = NULL,
+	.can_calibrate = true,
+	.mask_data0 = true,
+	.needs_new_timings = true,
+};
+
+static const struct sunxi_mmc_cfg sun50i_h6_emmc_cfg = {
+	.idma_des_size_bits = 13,
+	.clk_delays = NULL,
+	.can_calibrate = true,
+};
+
 static const struct of_device_id sunxi_mmc_of_match[] = {
 	{ .compatible = "allwinner,sun4i-a10-mmc", .data = &sun4i_a10_cfg },
 	{ .compatible = "allwinner,sun5i-a13-mmc", .data = &sun5i_a13_cfg },
@@ -1176,6 +1190,8 @@ static const struct of_device_id sunxi_mmc_of_match[] = {
 	{ .compatible = "allwinner,sun9i-a80-mmc", .data = &sun9i_a80_cfg },
 	{ .compatible = "allwinner,sun50i-a64-mmc", .data = &sun50i_a64_cfg },
 	{ .compatible = "allwinner,sun50i-a64-emmc", .data = &sun50i_a64_emmc_cfg },
+	{ .compatible = "allwinner,sun50i-h6-mmc", .data = &sun50i_h6_cfg },
+	{ .compatible = "allwinner,sun50i-h6-emmc", .data = &sun50i_h6_emmc_cfg },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, sunxi_mmc_of_match);
-- 
2.15.1

Re: [linux-sunxi] [PATCH 1/3] mmc: sunxi: add support for the MMC controller on H6

[Andre Przywara]

Hi,

On 26/04/18 15:07, Icenowy Zheng wrote:
> The new Allwinner H6 SoC have 3 MMC controllers. The first and second
> ones are similar to the ones on A64, but the third one adds EMCE
> (Embedded Crypto Engine) support which does hardware transparent crypto
> on the eMMC.
> 
> As we still do not have support for EMCE, and the support of it is
> disabled by defualt, we just duplicate the A64 mmc configurations and
> change the compatible string.

So if the A64 MMC part is compatible, we should express it like this:

> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> ---
>  Documentation/devicetree/bindings/mmc/sunxi-mmc.txt |  2 ++
>  drivers/mmc/host/sunxi-mmc.c                        | 16 ++++++++++++++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/sunxi-mmc.txt b/Documentation/devicetree/bindings/mmc/sunxi-mmc.txt
> index 132e0007d7d6..e6aa5c7a5e12 100644
> --- a/Documentation/devicetree/bindings/mmc/sunxi-mmc.txt
> +++ b/Documentation/devicetree/bindings/mmc/sunxi-mmc.txt
> @@ -16,6 +16,8 @@ Required properties:
>     * "allwinner,sun9i-a80-mmc"
>     * "allwinner,sun50i-a64-emmc"
>     * "allwinner,sun50i-a64-mmc"
> +   * "allwinner,sun50i-h6-emmc"
> +   * "allwinner,sun50i-h6-mmc"

This should be changed to:

      * "allwinner,sun50i-h6-emmc", "allwinner,sun50i-a64-emmc"
      * "allwinner,sun50i-h6-mmc", "allwinner,sun50i-a64-mmc"

>   - reg : mmc controller base registers
>   - clocks : a list with 4 phandle + clock specifier pairs
>   - clock-names : must contain "ahb", "mmc", "output" and "sample"
> diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
> index 97c6b79b7d6f..05e2b5fd7aa4 100644
> --- a/drivers/mmc/host/sunxi-mmc.c
> +++ b/drivers/mmc/host/sunxi-mmc.c
> @@ -1168,6 +1168,20 @@ static const struct sunxi_mmc_cfg sun50i_a64_emmc_cfg = {
>  	.can_calibrate = true,
>  };
>  
> +static const struct sunxi_mmc_cfg sun50i_h6_cfg = {
> +	.idma_des_size_bits = 16,
> +	.clk_delays = NULL,
> +	.can_calibrate = true,
> +	.mask_data0 = true,
> +	.needs_new_timings = true,
> +};
> +
> +static const struct sunxi_mmc_cfg sun50i_h6_emmc_cfg = {
> +	.idma_des_size_bits = 13,
> +	.clk_delays = NULL,
> +	.can_calibrate = true,
> +};
> +

... and then we don't need those changes ...

>  static const struct of_device_id sunxi_mmc_of_match[] = {
>  	{ .compatible = "allwinner,sun4i-a10-mmc", .data = &sun4i_a10_cfg },
>  	{ .compatible = "allwinner,sun5i-a13-mmc", .data = &sun5i_a13_cfg },
> @@ -1176,6 +1190,8 @@ static const struct of_device_id sunxi_mmc_of_match[] = {
>  	{ .compatible = "allwinner,sun9i-a80-mmc", .data = &sun9i_a80_cfg },
>  	{ .compatible = "allwinner,sun50i-a64-mmc", .data = &sun50i_a64_cfg },
>  	{ .compatible = "allwinner,sun50i-a64-emmc", .data = &sun50i_a64_emmc_cfg },
> +	{ .compatible = "allwinner,sun50i-h6-mmc", .data = &sun50i_h6_cfg },
> +	{ .compatible = "allwinner,sun50i-h6-emmc", .data = &sun50i_h6_emmc_cfg },

... and those, at least yet.
Should we ever extend the driver to support the EMCE, we can add them at
this occasion.

So this patch would just add the compatible pairs to the binding doc.

Cheers,
Andre.

>  	{ /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(of, sunxi_mmc_of_match);
> 

Re: [linux-sunxi] [PATCH 1/3] mmc: sunxi: add support for the MMC controller on H6

[Icenowy Zheng]

于 2018年4月27日 GMT+08:00 上午12:45:24, Andre Przywara <andre.przywara@arm.com> 写到:
>Hi,
>
>On 26/04/18 15:07, Icenowy Zheng wrote:
>> The new Allwinner H6 SoC have 3 MMC controllers. The first and second
>> ones are similar to the ones on A64, but the third one adds EMCE
>> (Embedded Crypto Engine) support which does hardware transparent
>crypto
>> on the eMMC.
>> 
>> As we still do not have support for EMCE, and the support of it is
>> disabled by defualt, we just duplicate the A64 mmc configurations and
>> change the compatible string.
>
>So if the A64 MMC part is compatible, we should express it like this:
>
>> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
>> ---
>>  Documentation/devicetree/bindings/mmc/sunxi-mmc.txt |  2 ++
>>  drivers/mmc/host/sunxi-mmc.c                        | 16
>++++++++++++++++
>>  2 files changed, 18 insertions(+)
>> 
>> diff --git a/Documentation/devicetree/bindings/mmc/sunxi-mmc.txt
>b/Documentation/devicetree/bindings/mmc/sunxi-mmc.txt
>> index 132e0007d7d6..e6aa5c7a5e12 100644
>> --- a/Documentation/devicetree/bindings/mmc/sunxi-mmc.txt
>> +++ b/Documentation/devicetree/bindings/mmc/sunxi-mmc.txt
>> @@ -16,6 +16,8 @@ Required properties:
>>     * "allwinner,sun9i-a80-mmc"
>>     * "allwinner,sun50i-a64-emmc"
>>     * "allwinner,sun50i-a64-mmc"
>> +   * "allwinner,sun50i-h6-emmc"
>> +   * "allwinner,sun50i-h6-mmc"
>
>This should be changed to:
>
>      * "allwinner,sun50i-h6-emmc", "allwinner,sun50i-a64-emmc"
>      * "allwinner,sun50i-h6-mmc", "allwinner,sun50i-a64-mmc"

This list shouldn't list pairs, and as I said in the reply,
these pairs are not valid.

>
>>   - reg : mmc controller base registers
>>   - clocks : a list with 4 phandle + clock specifier pairs
>>   - clock-names : must contain "ahb", "mmc", "output" and "sample"
>> diff --git a/drivers/mmc/host/sunxi-mmc.c
>b/drivers/mmc/host/sunxi-mmc.c
>> index 97c6b79b7d6f..05e2b5fd7aa4 100644
>> --- a/drivers/mmc/host/sunxi-mmc.c
>> +++ b/drivers/mmc/host/sunxi-mmc.c
>> @@ -1168,6 +1168,20 @@ static const struct sunxi_mmc_cfg
>sun50i_a64_emmc_cfg = {
>>  	.can_calibrate = true,
>>  };
>>  
>> +static const struct sunxi_mmc_cfg sun50i_h6_cfg = {
>> +	.idma_des_size_bits = 16,
>> +	.clk_delays = NULL,
>> +	.can_calibrate = true,
>> +	.mask_data0 = true,
>> +	.needs_new_timings = true,
>> +};
>> +
>> +static const struct sunxi_mmc_cfg sun50i_h6_emmc_cfg = {
>> +	.idma_des_size_bits = 13,
>> +	.clk_delays = NULL,
>> +	.can_calibrate = true,
>> +};
>> +
>
>... and then we don't need those changes ...
>
>>  static const struct of_device_id sunxi_mmc_of_match[] = {
>>  	{ .compatible = "allwinner,sun4i-a10-mmc", .data = &sun4i_a10_cfg
>},
>>  	{ .compatible = "allwinner,sun5i-a13-mmc", .data = &sun5i_a13_cfg
>},
>> @@ -1176,6 +1190,8 @@ static const struct of_device_id
>sunxi_mmc_of_match[] = {
>>  	{ .compatible = "allwinner,sun9i-a80-mmc", .data = &sun9i_a80_cfg
>},
>>  	{ .compatible = "allwinner,sun50i-a64-mmc", .data = &sun50i_a64_cfg
>},
>>  	{ .compatible = "allwinner,sun50i-a64-emmc", .data =
>&sun50i_a64_emmc_cfg },
>> +	{ .compatible = "allwinner,sun50i-h6-mmc", .data = &sun50i_h6_cfg
>},
>> +	{ .compatible = "allwinner,sun50i-h6-emmc", .data =
>&sun50i_h6_emmc_cfg },
>
>... and those, at least yet.
>Should we ever extend the driver to support the EMCE, we can add them
>at
>this occasion.
>
>So this patch would just add the compatible pairs to the binding doc.
>
>Cheers,
>Andre.
>
>>  	{ /* sentinel */ }
>>  };
>>  MODULE_DEVICE_TABLE(of, sunxi_mmc_of_match);
>> 
>
>_______________________________________________
>linux-arm-kernel mailing list
>linux-arm-kernel@lists.infradead.org
>http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [linux-sunxi] [PATCH 1/3] mmc: sunxi: add support for the MMC controller on H6

[Andre Przywara]

Hi,

On 27/04/18 09:38, Icenowy Zheng wrote:
> 
> 
> 于 2018年4月27日 GMT+08:00 上午12:45:24, Andre Przywara <andre.przywara@arm.com> 写到:
>> Hi,
>>
>> On 26/04/18 15:07, Icenowy Zheng wrote:
>>> The new Allwinner H6 SoC have 3 MMC controllers. The first and second
>>> ones are similar to the ones on A64, but the third one adds EMCE
>>> (Embedded Crypto Engine) support which does hardware transparent
>> crypto
>>> on the eMMC.
>>>
>>> As we still do not have support for EMCE, and the support of it is
>>> disabled by defualt, we just duplicate the A64 mmc configurations and
>>> change the compatible string.
>>
>> So if the A64 MMC part is compatible, we should express it like this:
>>
>>> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
>>> ---
>>>  Documentation/devicetree/bindings/mmc/sunxi-mmc.txt |  2 ++
>>>  drivers/mmc/host/sunxi-mmc.c                        | 16
>> ++++++++++++++++
>>>  2 files changed, 18 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/mmc/sunxi-mmc.txt
>> b/Documentation/devicetree/bindings/mmc/sunxi-mmc.txt
>>> index 132e0007d7d6..e6aa5c7a5e12 100644
>>> --- a/Documentation/devicetree/bindings/mmc/sunxi-mmc.txt
>>> +++ b/Documentation/devicetree/bindings/mmc/sunxi-mmc.txt
>>> @@ -16,6 +16,8 @@ Required properties:
>>>     * "allwinner,sun9i-a80-mmc"
>>>     * "allwinner,sun50i-a64-emmc"
>>>     * "allwinner,sun50i-a64-mmc"
>>> +   * "allwinner,sun50i-h6-emmc"
>>> +   * "allwinner,sun50i-h6-mmc"
>>
>> This should be changed to:
>>
>>      * "allwinner,sun50i-h6-emmc", "allwinner,sun50i-a64-emmc"
>>      * "allwinner,sun50i-h6-mmc", "allwinner,sun50i-a64-mmc"
> 
> This list shouldn't list pairs,

It should, as this is what Rob suggested the other day:
http://lists.infradead.org/pipermail/linux-arm-kernel/2018-March/564752.html

Cheers,
Andre.

> and as I said in the reply,
> these pairs are not valid.
> 
>>
>>>   - reg : mmc controller base registers
>>>   - clocks : a list with 4 phandle + clock specifier pairs
>>>   - clock-names : must contain "ahb", "mmc", "output" and "sample"
>>> diff --git a/drivers/mmc/host/sunxi-mmc.c
>> b/drivers/mmc/host/sunxi-mmc.c
>>> index 97c6b79b7d6f..05e2b5fd7aa4 100644
>>> --- a/drivers/mmc/host/sunxi-mmc.c
>>> +++ b/drivers/mmc/host/sunxi-mmc.c
>>> @@ -1168,6 +1168,20 @@ static const struct sunxi_mmc_cfg
>> sun50i_a64_emmc_cfg = {
>>>  	.can_calibrate = true,
>>>  };
>>>  
>>> +static const struct sunxi_mmc_cfg sun50i_h6_cfg = {
>>> +	.idma_des_size_bits = 16,
>>> +	.clk_delays = NULL,
>>> +	.can_calibrate = true,
>>> +	.mask_data0 = true,
>>> +	.needs_new_timings = true,
>>> +};
>>> +
>>> +static const struct sunxi_mmc_cfg sun50i_h6_emmc_cfg = {
>>> +	.idma_des_size_bits = 13,
>>> +	.clk_delays = NULL,
>>> +	.can_calibrate = true,
>>> +};
>>> +
>>
>> ... and then we don't need those changes ...
>>
>>>  static const struct of_device_id sunxi_mmc_of_match[] = {
>>>  	{ .compatible = "allwinner,sun4i-a10-mmc", .data = &sun4i_a10_cfg
>> },
>>>  	{ .compatible = "allwinner,sun5i-a13-mmc", .data = &sun5i_a13_cfg
>> },
>>> @@ -1176,6 +1190,8 @@ static const struct of_device_id
>> sunxi_mmc_of_match[] = {
>>>  	{ .compatible = "allwinner,sun9i-a80-mmc", .data = &sun9i_a80_cfg
>> },
>>>  	{ .compatible = "allwinner,sun50i-a64-mmc", .data = &sun50i_a64_cfg
>> },
>>>  	{ .compatible = "allwinner,sun50i-a64-emmc", .data =
>> &sun50i_a64_emmc_cfg },
>>> +	{ .compatible = "allwinner,sun50i-h6-mmc", .data = &sun50i_h6_cfg
>> },
>>> +	{ .compatible = "allwinner,sun50i-h6-emmc", .data =
>> &sun50i_h6_emmc_cfg },
>>
>> ... and those, at least yet.
>> Should we ever extend the driver to support the EMCE, we can add them
>> at
>> this occasion.
>>
>> So this patch would just add the compatible pairs to the binding doc.
>>
>> Cheers,
>> Andre.
>>
>>>  	{ /* sentinel */ }
>>>  };
>>>  MODULE_DEVICE_TABLE(of, sunxi_mmc_of_match);
>>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] mmc: sunxi: add support for the MMC controller on H6

[Ulf Hansson]

On 26 April 2018 at 16:07, Icenowy Zheng <icenowy@aosc.io> wrote:
> The new Allwinner H6 SoC have 3 MMC controllers. The first and second
> ones are similar to the ones on A64, but the third one adds EMCE
> (Embedded Crypto Engine) support which does hardware transparent crypto
> on the eMMC.
>
> As we still do not have support for EMCE, and the support of it is
> disabled by defualt, we just duplicate the A64 mmc configurations and
> change the compatible string.
>
> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> ---
>  Documentation/devicetree/bindings/mmc/sunxi-mmc.txt |  2 ++

DT doc changes in separate patches please!

>  drivers/mmc/host/sunxi-mmc.c                        | 16 ++++++++++++++++
>  2 files changed, 18 insertions(+)
>

Kind regards
Uffe

[PATCH 2/3] arm64: allwinner: h6: add device tree nodes for MMC controllers

[Icenowy Zheng]

The Allwinner H6 SoC have 3 MMC controllers.

Add device tree nodes for them.

Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
---
 arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 56 ++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
index 4debc3962830..3cbfc035c979 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
@@ -124,12 +124,68 @@
 			interrupt-controller;
 			#interrupt-cells = <3>;
 
+			mmc0_pins: mmc0-pins {
+				pins = "PF0", "PF1", "PF2", "PF3",
+				       "PF4", "PF5";
+				function = "mmc0";
+				drive-strength = <30>;
+				bias-pull-up;
+			};
+
+			mmc2_pins: mmc2-pins {
+				pins = "PC1", "PC4", "PC5", "PC6",
+				       "PC7", "PC8", "PC9", "PC10",
+				       "PC11", "PC12", "PC13", "PC14";
+				function = "mmc2";
+				drive-strength = <30>;
+				bias-pull-up;
+			};
+
 			uart0_ph_pins: uart0-ph {
 				pins = "PH0", "PH1";
 				function = "uart0";
 			};
 		};
 
+		mmc0: mmc@4020000 {
+			compatible = "allwinner,sun50i-h6-mmc";
+			reg = <0x04020000 0x1000>;
+			clocks = <&ccu CLK_BUS_MMC0>, <&ccu CLK_MMC0>;
+			clock-names = "ahb", "mmc";
+			resets = <&ccu RST_BUS_MMC0>;
+			reset-names = "ahb";
+			interrupts = <GIC_SPI 35 IRQ_TYPE_LEVEL_HIGH>;
+			status = "disabled";
+			#address-cells = <1>;
+			#size-cells = <0>;
+		};
+
+		mmc1: mmc@4021000 {
+			compatible = "allwinner,sun50i-h6-mmc";
+			reg = <0x04021000 0x1000>;
+			clocks = <&ccu CLK_BUS_MMC1>, <&ccu CLK_MMC1>;
+			clock-names = "ahb", "mmc";
+			resets = <&ccu RST_BUS_MMC1>;
+			reset-names = "ahb";
+			interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
+			status = "disabled";
+			#address-cells = <1>;
+			#size-cells = <0>;
+		};
+
+		mmc2: mmc@4022000 {
+			compatible = "allwinner,sun50i-h6-emmc";
+			reg = <0x04022000 0x1000>;
+			clocks = <&ccu CLK_BUS_MMC2>, <&ccu CLK_MMC2>;
+			clock-names = "ahb", "mmc";
+			resets = <&ccu RST_BUS_MMC2>;
+			reset-names = "ahb";
+			interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>;
+			status = "disabled";
+			#address-cells = <1>;
+			#size-cells = <0>;
+		};
+
 		uart0: serial@5000000 {
 			compatible = "snps,dw-apb-uart";
 			reg = <0x05000000 0x400>;
-- 
2.15.1

Re: [linux-sunxi] [PATCH 2/3] arm64: allwinner: h6: add device tree nodes for MMC controllers

[Andre Przywara]

Hi,

On 26/04/18 15:07, Icenowy Zheng wrote:
> The Allwinner H6 SoC have 3 MMC controllers.
> 
> Add device tree nodes for them.
> 
> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> ---
>  arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 56 ++++++++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> index 4debc3962830..3cbfc035c979 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> @@ -124,12 +124,68 @@
>  			interrupt-controller;
>  			#interrupt-cells = <3>;
>  
> +			mmc0_pins: mmc0-pins {
> +				pins = "PF0", "PF1", "PF2", "PF3",
> +				       "PF4", "PF5";
> +				function = "mmc0";
> +				drive-strength = <30>;
> +				bias-pull-up;
> +			};
> +
> +			mmc2_pins: mmc2-pins {
> +				pins = "PC1", "PC4", "PC5", "PC6",
> +				       "PC7", "PC8", "PC9", "PC10",
> +				       "PC11", "PC12", "PC13", "PC14";
> +				function = "mmc2";
> +				drive-strength = <30>;
> +				bias-pull-up;
> +			};
> +
>  			uart0_ph_pins: uart0-ph {
>  				pins = "PH0", "PH1";
>  				function = "uart0";
>  			};
>  		};
>  
> +		mmc0: mmc@4020000 {
> +			compatible = "allwinner,sun50i-h6-mmc";

This should be:
			compatible = "allwinner,sun50i-h6-mmc",
				     "allwinner,sun50i-a64-mmc";

> +			reg = <0x04020000 0x1000>;
> +			clocks = <&ccu CLK_BUS_MMC0>, <&ccu CLK_MMC0>;
> +			clock-names = "ahb", "mmc";
> +			resets = <&ccu RST_BUS_MMC0>;
> +			reset-names = "ahb";
> +			interrupts = <GIC_SPI 35 IRQ_TYPE_LEVEL_HIGH>;
> +			status = "disabled";
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +		};
> +
> +		mmc1: mmc@4021000 {
> +			compatible = "allwinner,sun50i-h6-mmc";

same here

> +			reg = <0x04021000 0x1000>;
> +			clocks = <&ccu CLK_BUS_MMC1>, <&ccu CLK_MMC1>;
> +			clock-names = "ahb", "mmc";
> +			resets = <&ccu RST_BUS_MMC1>;
> +			reset-names = "ahb";
> +			interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
> +			status = "disabled";
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +		};
> +
> +		mmc2: mmc@4022000 {
> +			compatible = "allwinner,sun50i-h6-emmc";

and here:
			compatible = "allwinner,sun50i-h6-emmc",
				     "allwinner,sun50i-a64-emmc";

The rest looks correct to me.

Cheers,
Andre.

> +			reg = <0x04022000 0x1000>;
> +			clocks = <&ccu CLK_BUS_MMC2>, <&ccu CLK_MMC2>;
> +			clock-names = "ahb", "mmc";
> +			resets = <&ccu RST_BUS_MMC2>;
> +			reset-names = "ahb";
> +			interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>;
> +			status = "disabled";
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +		};
> +
>  		uart0: serial@5000000 {
>  			compatible = "snps,dw-apb-uart";
>  			reg = <0x05000000 0x400>;
> 

Re: [linux-sunxi] [PATCH 2/3] arm64: allwinner: h6: add device tree nodes for MMC controllers

[Icenowy Zheng]

于 2018年4月27日 GMT+08:00 上午12:45:38, Andre Przywara <andre.przywara@arm.com> 写到:
>Hi,
>
>On 26/04/18 15:07, Icenowy Zheng wrote:
>> The Allwinner H6 SoC have 3 MMC controllers.
>> 
>> Add device tree nodes for them.
>> 
>> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
>> ---
>>  arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 56
>++++++++++++++++++++++++++++
>>  1 file changed, 56 insertions(+)
>> 
>> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
>b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
>> index 4debc3962830..3cbfc035c979 100644
>> --- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
>> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
>> @@ -124,12 +124,68 @@
>>  			interrupt-controller;
>>  			#interrupt-cells = <3>;
>>  
>> +			mmc0_pins: mmc0-pins {
>> +				pins = "PF0", "PF1", "PF2", "PF3",
>> +				       "PF4", "PF5";
>> +				function = "mmc0";
>> +				drive-strength = <30>;
>> +				bias-pull-up;
>> +			};
>> +
>> +			mmc2_pins: mmc2-pins {
>> +				pins = "PC1", "PC4", "PC5", "PC6",
>> +				       "PC7", "PC8", "PC9", "PC10",
>> +				       "PC11", "PC12", "PC13", "PC14";
>> +				function = "mmc2";
>> +				drive-strength = <30>;
>> +				bias-pull-up;
>> +			};
>> +
>>  			uart0_ph_pins: uart0-ph {
>>  				pins = "PH0", "PH1";
>>  				function = "uart0";
>>  			};
>>  		};
>>  
>> +		mmc0: mmc@4020000 {
>> +			compatible = "allwinner,sun50i-h6-mmc";
>
>This should be:
>			compatible = "allwinner,sun50i-h6-mmc",
>				     "allwinner,sun50i-a64-mmc";

I'm intended to not add A64 compatible, as
H6 is a quite new design
(new process) and there might be different behavior, even on mmc0/1.

>
>> +			reg = <0x04020000 0x1000>;
>> +			clocks = <&ccu CLK_BUS_MMC0>, <&ccu CLK_MMC0>;
>> +			clock-names = "ahb", "mmc";
>> +			resets = <&ccu RST_BUS_MMC0>;
>> +			reset-names = "ahb";
>> +			interrupts = <GIC_SPI 35 IRQ_TYPE_LEVEL_HIGH>;
>> +			status = "disabled";
>> +			#address-cells = <1>;
>> +			#size-cells = <0>;
>> +		};
>> +
>> +		mmc1: mmc@4021000 {
>> +			compatible = "allwinner,sun50i-h6-mmc";
>
>same here
>
>> +			reg = <0x04021000 0x1000>;
>> +			clocks = <&ccu CLK_BUS_MMC1>, <&ccu CLK_MMC1>;
>> +			clock-names = "ahb", "mmc";
>> +			resets = <&ccu RST_BUS_MMC1>;
>> +			reset-names = "ahb";
>> +			interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
>> +			status = "disabled";
>> +			#address-cells = <1>;
>> +			#size-cells = <0>;
>> +		};
>> +
>> +		mmc2: mmc@4022000 {
>> +			compatible = "allwinner,sun50i-h6-emmc";
>
>and here:
>			compatible = "allwinner,sun50i-h6-emmc",
>				     "allwinner,sun50i-a64-emmc";

MMC2 on H6 has EMCE capability, so surely there should
only be H6 compatible, and no A64 one.

>
>The rest looks correct to me.
>
>Cheers,
>Andre.
>
>> +			reg = <0x04022000 0x1000>;
>> +			clocks = <&ccu CLK_BUS_MMC2>, <&ccu CLK_MMC2>;
>> +			clock-names = "ahb", "mmc";
>> +			resets = <&ccu RST_BUS_MMC2>;
>> +			reset-names = "ahb";
>> +			interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>;
>> +			status = "disabled";
>> +			#address-cells = <1>;
>> +			#size-cells = <0>;
>> +		};
>> +
>>  		uart0: serial@5000000 {
>>  			compatible = "snps,dw-apb-uart";
>>  			reg = <0x05000000 0x400>;
>> 

Re: [linux-sunxi] [PATCH 2/3] arm64: allwinner: h6: add device tree nodes for MMC controllers

[Andre Przywara]

Hi,

On 27/04/18 09:36, Icenowy Zheng wrote:
> 
> 
> 于 2018年4月27日 GMT+08:00 上午12:45:38, Andre Przywara <andre.przywara@arm.com> 写到:
>> Hi,
>>
>> On 26/04/18 15:07, Icenowy Zheng wrote:
>>> The Allwinner H6 SoC have 3 MMC controllers.
>>>
>>> Add device tree nodes for them.
>>>
>>> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
>>> ---
>>>  arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 56
>> ++++++++++++++++++++++++++++
>>>  1 file changed, 56 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
>> b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
>>> index 4debc3962830..3cbfc035c979 100644
>>> --- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
>>> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
>>> @@ -124,12 +124,68 @@
>>>  			interrupt-controller;
>>>  			#interrupt-cells = <3>;
>>>  
>>> +			mmc0_pins: mmc0-pins {
>>> +				pins = "PF0", "PF1", "PF2", "PF3",
>>> +				       "PF4", "PF5";
>>> +				function = "mmc0";
>>> +				drive-strength = <30>;
>>> +				bias-pull-up;
>>> +			};
>>> +
>>> +			mmc2_pins: mmc2-pins {
>>> +				pins = "PC1", "PC4", "PC5", "PC6",
>>> +				       "PC7", "PC8", "PC9", "PC10",
>>> +				       "PC11", "PC12", "PC13", "PC14";
>>> +				function = "mmc2";
>>> +				drive-strength = <30>;
>>> +				bias-pull-up;
>>> +			};
>>> +
>>>  			uart0_ph_pins: uart0-ph {
>>>  				pins = "PH0", "PH1";
>>>  				function = "uart0";
>>>  			};
>>>  		};
>>>  
>>> +		mmc0: mmc@4020000 {
>>> +			compatible = "allwinner,sun50i-h6-mmc";
>>
>> This should be:
>> 			compatible = "allwinner,sun50i-h6-mmc",
>> 				     "allwinner,sun50i-a64-mmc";
> 
> I'm intended to not add A64 compatible, as
> H6 is a quite new design
> (new process) and there might be different behavior, even on mmc0/1.

But as your patch proves, it is fully backwards compatible: An A64
driver works with this device.
And this is what this compatible string list says: If your system does
not have a specific H6 driver, you can use an A64 driver.
You might not get all the (potentially) new features, but it covers
everything the A64 has.

And a new silicon process doesn't matter here, since the software
interface is unchanged. *If* we find bugs, we can add quirks matching on
the H6 compatible string - that's why we put it here already, despite
having a matching string in the kernel at the moment.

>>> +			reg = <0x04020000 0x1000>;
>>> +			clocks = <&ccu CLK_BUS_MMC0>, <&ccu CLK_MMC0>;
>>> +			clock-names = "ahb", "mmc";
>>> +			resets = <&ccu RST_BUS_MMC0>;
>>> +			reset-names = "ahb";
>>> +			interrupts = <GIC_SPI 35 IRQ_TYPE_LEVEL_HIGH>;
>>> +			status = "disabled";
>>> +			#address-cells = <1>;
>>> +			#size-cells = <0>;
>>> +		};
>>> +
>>> +		mmc1: mmc@4021000 {
>>> +			compatible = "allwinner,sun50i-h6-mmc";
>>
>> same here
>>
>>> +			reg = <0x04021000 0x1000>;
>>> +			clocks = <&ccu CLK_BUS_MMC1>, <&ccu CLK_MMC1>;
>>> +			clock-names = "ahb", "mmc";
>>> +			resets = <&ccu RST_BUS_MMC1>;
>>> +			reset-names = "ahb";
>>> +			interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
>>> +			status = "disabled";
>>> +			#address-cells = <1>;
>>> +			#size-cells = <0>;
>>> +		};
>>> +
>>> +		mmc2: mmc@4022000 {
>>> +			compatible = "allwinner,sun50i-h6-emmc";
>>
>> and here:
>> 			compatible = "allwinner,sun50i-h6-emmc",
>> 				     "allwinner,sun50i-a64-emmc";
> 
> MMC2 on H6 has EMCE capability, so surely there should
> only be H6 compatible, and no A64 one.

Same as above, the A64 eMMC is a subset of the H6 eMMC, so the A64 eMMC
driver can drive the H6 as well. And again your code proves that,
because it behaves exactly the same as for the A64.
In case we ever get support for the EMCE, we add the new compatible
string to the driver and tie it to the new feature. So newer kernels can
use this feature, older kernel will just not, but can happily use the
eMMC anyway.

Cheers,
Andre.

Re: [linux-sunxi] [PATCH 2/3] arm64: allwinner: h6: add device tree nodes for MMC controllers

[Icenowy Zheng]

于 2018年4月27日 GMT+08:00 下午5:18:23, Andre Przywara <andre.przywara@arm.com> 写到:
>Hi,
>
>On 27/04/18 09:36, Icenowy Zheng wrote:
>> 
>> 
>> 于 2018年4月27日 GMT+08:00 上午12:45:38, Andre Przywara
><andre.przywara@arm.com> 写到:
>>> Hi,
>>>
>>> On 26/04/18 15:07, Icenowy Zheng wrote:
>>>> The Allwinner H6 SoC have 3 MMC controllers.
>>>>
>>>> Add device tree nodes for them.
>>>>
>>>> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
>>>> ---
>>>>  arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 56
>>> ++++++++++++++++++++++++++++
>>>>  1 file changed, 56 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
>>> b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
>>>> index 4debc3962830..3cbfc035c979 100644
>>>> --- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
>>>> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
>>>> @@ -124,12 +124,68 @@
>>>>  			interrupt-controller;
>>>>  			#interrupt-cells = <3>;
>>>>  
>>>> +			mmc0_pins: mmc0-pins {
>>>> +				pins = "PF0", "PF1", "PF2", "PF3",
>>>> +				       "PF4", "PF5";
>>>> +				function = "mmc0";
>>>> +				drive-strength = <30>;
>>>> +				bias-pull-up;
>>>> +			};
>>>> +
>>>> +			mmc2_pins: mmc2-pins {
>>>> +				pins = "PC1", "PC4", "PC5", "PC6",
>>>> +				       "PC7", "PC8", "PC9", "PC10",
>>>> +				       "PC11", "PC12", "PC13", "PC14";
>>>> +				function = "mmc2";
>>>> +				drive-strength = <30>;
>>>> +				bias-pull-up;
>>>> +			};
>>>> +
>>>>  			uart0_ph_pins: uart0-ph {
>>>>  				pins = "PH0", "PH1";
>>>>  				function = "uart0";
>>>>  			};
>>>>  		};
>>>>  
>>>> +		mmc0: mmc@4020000 {
>>>> +			compatible = "allwinner,sun50i-h6-mmc";
>>>
>>> This should be:
>>> 			compatible = "allwinner,sun50i-h6-mmc",
>>> 				     "allwinner,sun50i-a64-mmc";
>> 
>> I'm intended to not add A64 compatible, as
>> H6 is a quite new design
>> (new process) and there might be different behavior, even on mmc0/1.
>
>But as your patch proves, it is fully backwards compatible: An A64
>driver works with this device.

No, my patch only proves "the current A64 driver works
with this device", not "Any A64 driver works with device", as
the current driver doesn't fully use the capability provided
by A64 MMC cobtrollers.

>And this is what this compatible string list says: If your system does
>not have a specific H6 driver, you can use an A64 driver.
>You might not get all the (potentially) new features, but it covers
>everything the A64 has.
>
>And a new silicon process doesn't matter here, since the software
>interface is unchanged. *If* we find bugs, we can add quirks matching

I think there's timing parameters for higher speed bins which
are different among chips. As we have currently no support
for speed bins higher than DDR50, they're not added yet.

>on
>the H6 compatible string - that's why we put it here already, despite
>having a matching string in the kernel at the moment.

Device tree is not driver data but hardware description, so
it should follow "how the device is formed" rather than
"how the device works".

>
>>>> +			reg = <0x04020000 0x1000>;
>>>> +			clocks = <&ccu CLK_BUS_MMC0>, <&ccu CLK_MMC0>;
>>>> +			clock-names = "ahb", "mmc";
>>>> +			resets = <&ccu RST_BUS_MMC0>;
>>>> +			reset-names = "ahb";
>>>> +			interrupts = <GIC_SPI 35 IRQ_TYPE_LEVEL_HIGH>;
>>>> +			status = "disabled";
>>>> +			#address-cells = <1>;
>>>> +			#size-cells = <0>;
>>>> +		};
>>>> +
>>>> +		mmc1: mmc@4021000 {
>>>> +			compatible = "allwinner,sun50i-h6-mmc";
>>>
>>> same here
>>>
>>>> +			reg = <0x04021000 0x1000>;
>>>> +			clocks = <&ccu CLK_BUS_MMC1>, <&ccu CLK_MMC1>;
>>>> +			clock-names = "ahb", "mmc";
>>>> +			resets = <&ccu RST_BUS_MMC1>;
>>>> +			reset-names = "ahb";
>>>> +			interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
>>>> +			status = "disabled";
>>>> +			#address-cells = <1>;
>>>> +			#size-cells = <0>;
>>>> +		};
>>>> +
>>>> +		mmc2: mmc@4022000 {
>>>> +			compatible = "allwinner,sun50i-h6-emmc";
>>>
>>> and here:
>>> 			compatible = "allwinner,sun50i-h6-emmc",
>>> 				     "allwinner,sun50i-a64-emmc";
>> 
>> MMC2 on H6 has EMCE capability, so surely there should
>> only be H6 compatible, and no A64 one.
>
>Same as above, the A64 eMMC is a subset of the H6 eMMC, so the A64 eMMC
>driver can drive the H6 as well. And again your code proves that,
>because it behaves exactly the same as for the A64.
>In case we ever get support for the EMCE, we add the new compatible
>string to the driver and tie it to the new feature. So newer kernels
>can
>use this feature, older kernel will just not, but can happily use the
>eMMC anyway.
>
>Cheers,
>Andre.

Re: [linux-sunxi] [PATCH 2/3] arm64: allwinner: h6: add device tree nodes for MMC controllers

[André Przywara]

On 27/04/18 10:23, Icenowy Zheng wrote:
> 
> 
> 于 2018年4月27日 GMT+08:00 下午5:18:23, Andre Przywara <andre.przywara@arm.com> 写到:
>> Hi,
>>
>> On 27/04/18 09:36, Icenowy Zheng wrote:
>>>
>>>
>>> 于 2018年4月27日 GMT+08:00 上午12:45:38, Andre Przywara
>> <andre.przywara@arm.com> 写到:
>>>> Hi,
>>>>
>>>> On 26/04/18 15:07, Icenowy Zheng wrote:
>>>>> The Allwinner H6 SoC have 3 MMC controllers.
>>>>>
>>>>> Add device tree nodes for them.
>>>>>
>>>>> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
>>>>> ---
>>>>>  arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 56
>>>> ++++++++++++++++++++++++++++
>>>>>  1 file changed, 56 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
>>>> b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
>>>>> index 4debc3962830..3cbfc035c979 100644
>>>>> --- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
>>>>> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
>>>>> @@ -124,12 +124,68 @@
>>>>>  			interrupt-controller;
>>>>>  			#interrupt-cells = <3>;
>>>>>  
>>>>> +			mmc0_pins: mmc0-pins {
>>>>> +				pins = "PF0", "PF1", "PF2", "PF3",
>>>>> +				       "PF4", "PF5";
>>>>> +				function = "mmc0";
>>>>> +				drive-strength = <30>;
>>>>> +				bias-pull-up;
>>>>> +			};
>>>>> +
>>>>> +			mmc2_pins: mmc2-pins {
>>>>> +				pins = "PC1", "PC4", "PC5", "PC6",
>>>>> +				       "PC7", "PC8", "PC9", "PC10",
>>>>> +				       "PC11", "PC12", "PC13", "PC14";
>>>>> +				function = "mmc2";
>>>>> +				drive-strength = <30>;
>>>>> +				bias-pull-up;
>>>>> +			};
>>>>> +
>>>>>  			uart0_ph_pins: uart0-ph {
>>>>>  				pins = "PH0", "PH1";
>>>>>  				function = "uart0";
>>>>>  			};
>>>>>  		};
>>>>>  
>>>>> +		mmc0: mmc@4020000 {
>>>>> +			compatible = "allwinner,sun50i-h6-mmc";
>>>>
>>>> This should be:
>>>> 			compatible = "allwinner,sun50i-h6-mmc",
>>>> 				     "allwinner,sun50i-a64-mmc";
>>>
>>> I'm intended to not add A64 compatible, as
>>> H6 is a quite new design
>>> (new process) and there might be different behavior, even on mmc0/1.
>>
>> But as your patch proves, it is fully backwards compatible: An A64
>> driver works with this device.
> 
> No, my patch only proves "the current A64 driver works
> with this device", not "Any A64 driver works with device", as
> the current driver doesn't fully use the capability provided
> by A64 MMC cobtrollers.

Good point, but I still believe every A64 driver would be capable of
driving an H6 MMC controller, ....

>> And this is what this compatible string list says: If your system does
>> not have a specific H6 driver, you can use an A64 driver.
>> You might not get all the (potentially) new features, but it covers
>> everything the A64 has.
>>
>> And a new silicon process doesn't matter here, since the software
>> interface is unchanged. *If* we find bugs, we can add quirks matching
> 
> I think there's timing parameters for higher speed bins which
> are different among chips. As we have currently no support
> for speed bins higher than DDR50, they're not added yet.

True, but what are those differences? I compared the A64 and H6 manuals
side by side, the differences I found are:
SMHC_FIFOTH[+0x40]:
	BSIZE_OF_TRANS[30:28]:
	- H6 supports 16 transfers for SMHC0 also.
	other parameters:
	- H6 recommends better values for SMHC0 also
SMHC_CSDC[+0x54]:
	- H6 doesn't mention restriction to SMHC2
	(though this might be a mistake)
SMHC_NTSR_REG[+0x5C]:
	- H6 defines fields for bits[24:8]
SMHC_EMCE[+0x64] and SMHC_EMCE_DBG[+0x68]:
	- H6 adds, for EMCE support
EMMC_DDR_SBIT_DET_REG[0x10c]:
	- A64 doesn't mention restriction to SMHC2,
	  but I believe this is a mistake
SMHC_EMCE_BMn[0x150 + 0x4 * 0..31]
	- H6 adds, for EMCE support

All those pieces are only *additions* to the H6 over the A64, so don't
affect backwards compatibility.

>> on
>> the H6 compatible string - that's why we put it here already, despite
>> having a matching string in the kernel at the moment.
> 
> Device tree is not driver data but hardware description, so
> it should follow "how the device is formed" rather than
> "how the device works".

True, but as shown above, the compatibility is really at the device level.
Unless you have any other information ...

Cheers,
Andre.

Re: [linux-sunxi] [PATCH 2/3] arm64: allwinner: h6: add device tree nodes for MMC controllers

[Icenowy Zheng]

在 2018-04-27五的 22:25 +0100，André Przywara写道：
> On 27/04/18 10:23, Icenowy Zheng wrote:
> > 
> > 
> > 于 2018年4月27日 GMT+08:00 下午5:18:23, Andre Przywara <andre.przywara@ar
> > m.com> 写到:
> > > Hi,
> > > 
> > > On 27/04/18 09:36, Icenowy Zheng wrote:
> > > > 
> > > > 
> > > > 于 2018年4月27日 GMT+08:00 上午12:45:38, Andre Przywara
> > > 
> > > <andre.przywara@arm.com> 写到:
> > > > > Hi,
> > > > > 
> > > > > On 26/04/18 15:07, Icenowy Zheng wrote:
> > > > > > The Allwinner H6 SoC have 3 MMC controllers.
> > > > > > 
> > > > > > Add device tree nodes for them.
> > > > > > 
> > > > > > Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> > > > > > ---
> > > > > >  arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 56
> > > > > 
> > > > > ++++++++++++++++++++++++++++
> > > > > >  1 file changed, 56 insertions(+)
> > > > > > 
> > > > > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> > > > > 
> > > > > b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> > > > > > index 4debc3962830..3cbfc035c979 100644
> > > > > > --- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> > > > > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> > > > > > @@ -124,12 +124,68 @@
> > > > > >  			interrupt-controller;
> > > > > >  			#interrupt-cells = <3>;
> > > > > >  
> > > > > > +			mmc0_pins: mmc0-pins {
> > > > > > +				pins = "PF0", "PF1",
> > > > > > "PF2", "PF3",
> > > > > > +				       "PF4", "PF5";
> > > > > > +				function = "mmc0";
> > > > > > +				drive-strength = <30>;
> > > > > > +				bias-pull-up;
> > > > > > +			};
> > > > > > +
> > > > > > +			mmc2_pins: mmc2-pins {
> > > > > > +				pins = "PC1", "PC4",
> > > > > > "PC5", "PC6",
> > > > > > +				       "PC7", "PC8",
> > > > > > "PC9", "PC10",
> > > > > > +				       "PC11", "PC12",
> > > > > > "PC13", "PC14";
> > > > > > +				function = "mmc2";
> > > > > > +				drive-strength = <30>;
> > > > > > +				bias-pull-up;
> > > > > > +			};
> > > > > > +
> > > > > >  			uart0_ph_pins: uart0-ph {
> > > > > >  				pins = "PH0", "PH1";
> > > > > >  				function = "uart0";
> > > > > >  			};
> > > > > >  		};
> > > > > >  
> > > > > > +		mmc0: mmc@4020000 {
> > > > > > +			compatible = "allwinner,sun50i-h6-
> > > > > > mmc";
> > > > > 
> > > > > This should be:
> > > > > 			compatible = "allwinner,sun50i-h6-mmc",
> > > > > 				     "allwinner,sun50i-a64-
> > > > > mmc";
> > > > 
> > > > I'm intended to not add A64 compatible, as
> > > > H6 is a quite new design
> > > > (new process) and there might be different behavior, even on
> > > > mmc0/1.
> > > 
> > > But as your patch proves, it is fully backwards compatible: An
> > > A64
> > > driver works with this device.
> > 
> > No, my patch only proves "the current A64 driver works
> > with this device", not "Any A64 driver works with device", as
> > the current driver doesn't fully use the capability provided
> > by A64 MMC cobtrollers.
> 
> Good point, but I still believe every A64 driver would be capable of
> driving an H6 MMC controller, ....
> 
> > > And this is what this compatible string list says: If your system
> > > does
> > > not have a specific H6 driver, you can use an A64 driver.
> > > You might not get all the (potentially) new features, but it
> > > covers
> > > everything the A64 has.
> > > 
> > > And a new silicon process doesn't matter here, since the software
> > > interface is unchanged. *If* we find bugs, we can add quirks
> > > matching
> > 
> > I think there's timing parameters for higher speed bins which
> > are different among chips. As we have currently no support
> > for speed bins higher than DDR50, they're not added yet.
> 
> True, but what are those differences? I compared the A64 and H6
> manuals
> side by side, the differences I found are:
> SMHC_FIFOTH[+0x40]:
> 	BSIZE_OF_TRANS[30:28]:
> 	- H6 supports 16 transfers for SMHC0 also.
> 	other parameters:
> 	- H6 recommends better values for SMHC0 also
> SMHC_CSDC[+0x54]:
> 	- H6 doesn't mention restriction to SMHC2
> 	(though this might be a mistake)
> SMHC_NTSR_REG[+0x5C]:
> 	- H6 defines fields for bits[24:8]
> SMHC_EMCE[+0x64] and SMHC_EMCE_DBG[+0x68]:
> 	- H6 adds, for EMCE support
> EMMC_DDR_SBIT_DET_REG[0x10c]:
> 	- A64 doesn't mention restriction to SMHC2,
> 	  but I believe this is a mistake
> SMHC_EMCE_BMn[0x150 + 0x4 * 0..31]
> 	- H6 adds, for EMCE support
> 
> All those pieces are only *additions* to the H6 over the A64, so
> don't
> affect backwards compatibility.
> 
> > > on
> > > the H6 compatible string - that's why we put it here already,
> > > despite
> > > having a matching string in the kernel at the moment.
> > 
> > Device tree is not driver data but hardware description, so
> > it should follow "how the device is formed" rather than
> > "how the device works".
> 
> True, but as shown above, the compatibility is really at the device
> level.
> Unless you have any other information ...

Rob, could you answer whether we should add the A64 compatible or not?

> 
> Cheers,
> Andre.
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/3] arm64: allwinner: h6: enable MMC0/2 on Pine H64

[Icenowy Zheng]

The Pine H64 board have a MicroSD slot connected to MMC0 controller of
the H6 SoC and a eMMC slot connected to MMC2.

Enable them in the device tree.

Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
---
 .../boot/dts/allwinner/sun50i-h6-pine-h64.dts      | 32 ++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts b/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts
index d36de5eb81f3..78b1cd54687c 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts
@@ -20,6 +20,38 @@
 	chosen {
 		stdout-path = "serial0:115200n8";
 	};
+
+	reg_vcc3v3: vcc3v3 {
+		compatible = "regulator-fixed";
+		regulator-name = "vcc3v3";
+		regulator-min-microvolt = <3300000>;
+		regulator-max-microvolt = <3300000>;
+	};
+
+	reg_vcc1v8: vcc1v8 {
+		compatible = "regulator-fixed";
+		regulator-name = "vcc1v8";
+		regulator-min-microvolt = <1800000>;
+		regulator-max-microvolt = <1800000>;
+	};
+};
+
+&mmc0 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&mmc0_pins>;
+	vmmc-supply = <&reg_vcc3v3>;
+	cd-gpios = <&pio 5 6 GPIO_ACTIVE_LOW>;
+	status = "okay";
+};
+
+&mmc2 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&mmc2_pins>;
+	vmmc-supply = <&reg_vcc3v3>;
+	vqmmc-supply = <&reg_vcc1v8>;
+	non-removable;
+	cap-mmc-hw-reset;
+	status = "okay";
 };
 
 &uart0 {
-- 
2.15.1

Re: [linux-sunxi] [PATCH 3/3] arm64: allwinner: h6: enable MMC0/2 on Pine H64

[Andre Przywara]

Hi,

On 26/04/18 15:07, Icenowy Zheng wrote:
> The Pine H64 board have a MicroSD slot connected to MMC0 controller of
> the H6 SoC and a eMMC slot connected to MMC2.
> 
> Enable them in the device tree.
> 
> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> ---
>  .../boot/dts/allwinner/sun50i-h6-pine-h64.dts      | 32 ++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts b/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts
> index d36de5eb81f3..78b1cd54687c 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts
> @@ -20,6 +20,38 @@
>  	chosen {
>  		stdout-path = "serial0:115200n8";
>  	};
> +
> +	reg_vcc3v3: vcc3v3 {
> +		compatible = "regulator-fixed";
> +		regulator-name = "vcc3v3";
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +	};
> +
> +	reg_vcc1v8: vcc1v8 {
> +		compatible = "regulator-fixed";
> +		regulator-name = "vcc1v8";
> +		regulator-min-microvolt = <1800000>;
> +		regulator-max-microvolt = <1800000>;
> +	};
> +};
> +
> +&mmc0 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&mmc0_pins>;
> +	vmmc-supply = <&reg_vcc3v3>;

So this is actually CLDO1 on the AXP, correct?


> +	cd-gpios = <&pio 5 6 GPIO_ACTIVE_LOW>;
> +	status = "okay";
> +};
> +
> +&mmc2 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&mmc2_pins>;
> +	vmmc-supply = <&reg_vcc3v3>;
> +	vqmmc-supply = <&reg_vcc1v8>;

And this is BLDO2?

I am just asking because I want to avoid running into the same problem
as with the A64 before: that future DTs become incompatible with older
kernels, because we change the power supply to point to the AXP
regulators, which this kernel does not support yet.

It looks like there are more users of those power rails, so we could
keep those supplies connected to these fixed regulators here, even with
AXP-805 support in the kernel.

Or we keep this back until we get proper AXP support in the kernel? I
guess it's quite close to the existing PMICs, so it might be more a
copy&paste exercise to support the AXP-805?

But apart from this this looks correct to me.

Cheers,
Andre.

> +	non-removable;
> +	cap-mmc-hw-reset;
> +	status = "okay";
>  };
>  
>  &uart0 {
> 

Re: [linux-sunxi] [PATCH 3/3] arm64: allwinner: h6: enable MMC0/2 on Pine H64

[Icenowy Zheng]

于 2018年4月27日 GMT+08:00 上午12:46:26, Andre Przywara <andre.przywara@arm.com> 写到:
>Hi,
>
>On 26/04/18 15:07, Icenowy Zheng wrote:
>> The Pine H64 board have a MicroSD slot connected to MMC0 controller
>of
>> the H6 SoC and a eMMC slot connected to MMC2.
>> 
>> Enable them in the device tree.
>> 
>> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
>> ---
>>  .../boot/dts/allwinner/sun50i-h6-pine-h64.dts      | 32
>++++++++++++++++++++++
>>  1 file changed, 32 insertions(+)
>> 
>> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts
>b/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts
>> index d36de5eb81f3..78b1cd54687c 100644
>> --- a/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts
>> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts
>> @@ -20,6 +20,38 @@
>>  	chosen {
>>  		stdout-path = "serial0:115200n8";
>>  	};
>> +
>> +	reg_vcc3v3: vcc3v3 {
>> +		compatible = "regulator-fixed";
>> +		regulator-name = "vcc3v3";
>> +		regulator-min-microvolt = <3300000>;
>> +		regulator-max-microvolt = <3300000>;
>> +	};
>> +
>> +	reg_vcc1v8: vcc1v8 {
>> +		compatible = "regulator-fixed";
>> +		regulator-name = "vcc1v8";
>> +		regulator-min-microvolt = <1800000>;
>> +		regulator-max-microvolt = <1800000>;
>> +	};
>> +};
>> +
>> +&mmc0 {
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&mmc0_pins>;
>> +	vmmc-supply = <&reg_vcc3v3>;
>
>So this is actually CLDO1 on the AXP, correct?

I remember it's coupled between two LDOs, to provide enough power.

>
>
>> +	cd-gpios = <&pio 5 6 GPIO_ACTIVE_LOW>;
>> +	status = "okay";
>> +};
>> +
>> +&mmc2 {
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&mmc2_pins>;
>> +	vmmc-supply = <&reg_vcc3v3>;
>> +	vqmmc-supply = <&reg_vcc1v8>;
>
>And this is BLDO2?

Yes.

>
>I am just asking because I want to avoid running into the same problem
>as with the A64 before: that future DTs become incompatible with older
>kernels, because we change the power supply to point to the AXP
>regulators, which this kernel does not support yet.

The answer is just not to keep this compatibility, as it's not
supported option to update DT without updating kernel.

P.S. I think the DT will update twice on the kernel side, the
first time keep reg_vcc3v3 (as it's coupled) but use real
regulator for reg_vcc1v8, the second time use the real
coupled regulator for reg_vcc3v3.

>
>It looks like there are more users of those power rails, so we could
>keep those supplies connected to these fixed regulators here, even with
>AXP-805 support in the kernel.

It's not a good choice.

>
>Or we keep this back until we get proper AXP support in the kernel? I
>guess it's quite close to the existing PMICs, so it might be more a
>copy&paste exercise to support the AXP-805?

It's not a reason to keep it back.

>
>But apart from this this looks correct to me.
>
>Cheers,
>Andre.
>
>> +	non-removable;
>> +	cap-mmc-hw-reset;
>> +	status = "okay";
>>  };
>>  
>>  &uart0 {
>> 

Re: [linux-sunxi] [PATCH 3/3] arm64: allwinner: h6: enable MMC0/2 on Pine H64

[Andre Przywara]

Hi Icenowy,

On 27/04/18 08:12, Icenowy Zheng wrote:
> 
> 
> 于 2018年4月27日 GMT+08:00 上午12:46:26, Andre Przywara <andre.przywara@arm.com> 写到:
>> Hi,
>>
>> On 26/04/18 15:07, Icenowy Zheng wrote:
>>> The Pine H64 board have a MicroSD slot connected to MMC0 controller
>> of
>>> the H6 SoC and a eMMC slot connected to MMC2.
>>>
>>> Enable them in the device tree.
>>>
>>> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
>>> ---
>>>  .../boot/dts/allwinner/sun50i-h6-pine-h64.dts      | 32
>> ++++++++++++++++++++++
>>>  1 file changed, 32 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts
>> b/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts
>>> index d36de5eb81f3..78b1cd54687c 100644
>>> --- a/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts
>>> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts
>>> @@ -20,6 +20,38 @@
>>>  	chosen {
>>>  		stdout-path = "serial0:115200n8";
>>>  	};
>>> +
>>> +	reg_vcc3v3: vcc3v3 {
>>> +		compatible = "regulator-fixed";
>>> +		regulator-name = "vcc3v3";
>>> +		regulator-min-microvolt = <3300000>;
>>> +		regulator-max-microvolt = <3300000>;
>>> +	};
>>> +
>>> +	reg_vcc1v8: vcc1v8 {
>>> +		compatible = "regulator-fixed";
>>> +		regulator-name = "vcc1v8";
>>> +		regulator-min-microvolt = <1800000>;
>>> +		regulator-max-microvolt = <1800000>;
>>> +	};
>>> +};
>>> +
>>> +&mmc0 {
>>> +	pinctrl-names = "default";
>>> +	pinctrl-0 = <&mmc0_pins>;
>>> +	vmmc-supply = <&reg_vcc3v3>;
>>
>> So this is actually CLDO1 on the AXP, correct?
> 
> I remember it's coupled between two LDOs, to provide enough power.
> 
>>
>>
>>> +	cd-gpios = <&pio 5 6 GPIO_ACTIVE_LOW>;
>>> +	status = "okay";
>>> +};
>>> +
>>> +&mmc2 {
>>> +	pinctrl-names = "default";
>>> +	pinctrl-0 = <&mmc2_pins>;
>>> +	vmmc-supply = <&reg_vcc3v3>;
>>> +	vqmmc-supply = <&reg_vcc1v8>;
>>
>> And this is BLDO2?
> 
> Yes.
> 
>>
>> I am just asking because I want to avoid running into the same problem
>> as with the A64 before: that future DTs become incompatible with older
>> kernels, because we change the power supply to point to the AXP
>> regulators, which this kernel does not support yet.
> 
> The answer is just not to keep this compatibility, as it's not
> supported option to update DT without updating kernel.

Well, I recognise that statement.. ;-) and I understand that it's far
easier to handle it this way. But:
- Which .dtb are we going to write into the SPI flash? An older one,
which covers all kernels, but lacks features? Or a newer one, which
limits the bootable kernels to recent versions?
- Which DT are we going to give to EFI applications?
- Which DT are the BSDs suspected to take? They don't ship their own DTs
(which is good!).

So I understand that "shipping the DT with the kernel" is the old
(embedded!) way of doing things, but I really believe we should stop
relying on this and try to come up with backwards compatible DTs, which
live in the firmware and get updated there. Because this is what the
distros seem to expect from ARM64 boards these days.

> P.S. I think the DT will update twice on the kernel side, the
> first time keep reg_vcc3v3 (as it's coupled) but use real
> regulator for reg_vcc1v8, the second time use the real
> coupled regulator for reg_vcc3v3.
> 
>>
>> It looks like there are more users of those power rails, so we could
>> keep those supplies connected to these fixed regulators here, even with
>> AXP-805 support in the kernel.
> 
> It's not a good choice.
> 
>>
>> Or we keep this back until we get proper AXP support in the kernel? I
>> guess it's quite close to the existing PMICs, so it might be more a
>> copy&paste exercise to support the AXP-805?
> 
> It's not a reason to keep it back.

So I compared the manuals of the AXP806 and the AXP805, the register
interface looks identical to me. I only have a (somewhat) Chinese
version of the AXP806 manual, so couldn't really find the difference
between the two. Do you know more about it? Is it just maybe the
packaging and the electrical properties (like max current supported)?

If the I2C register interface is really the same, we could just add the
DT nodes for the regulator and be done.

Cheers,
Andre.

> 
>>
>> But apart from this this looks correct to me.
>>
>> Cheers,
>> Andre.
>>
>>> +	non-removable;
>>> +	cap-mmc-hw-reset;
>>> +	status = "okay";
>>>  };
>>>  
>>>  &uart0 {
>>>

Re: [linux-sunxi] [PATCH 3/3] arm64: allwinner: h6: enable MMC0/2 on Pine H64

[Icenowy Zheng]

于 2018年4月30日 GMT+08:00 下午5:47:35, Andre Przywara <andre.przywara@arm.com> 写到:
>Hi Icenowy,
>
>On 27/04/18 08:12, Icenowy Zheng wrote:
>> 
>> 
>> 于 2018年4月27日 GMT+08:00 上午12:46:26, Andre Przywara
><andre.przywara@arm.com> 写到:
>>> Hi,
>>>
>>> On 26/04/18 15:07, Icenowy Zheng wrote:
>>>> The Pine H64 board have a MicroSD slot connected to MMC0 controller
>>> of
>>>> the H6 SoC and a eMMC slot connected to MMC2.
>>>>
>>>> Enable them in the device tree.
>>>>
>>>> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
>>>> ---
>>>>  .../boot/dts/allwinner/sun50i-h6-pine-h64.dts      | 32
>>> ++++++++++++++++++++++
>>>>  1 file changed, 32 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts
>>> b/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts
>>>> index d36de5eb81f3..78b1cd54687c 100644
>>>> --- a/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts
>>>> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts
>>>> @@ -20,6 +20,38 @@
>>>>  	chosen {
>>>>  		stdout-path = "serial0:115200n8";
>>>>  	};
>>>> +
>>>> +	reg_vcc3v3: vcc3v3 {
>>>> +		compatible = "regulator-fixed";
>>>> +		regulator-name = "vcc3v3";
>>>> +		regulator-min-microvolt = <3300000>;
>>>> +		regulator-max-microvolt = <3300000>;
>>>> +	};
>>>> +
>>>> +	reg_vcc1v8: vcc1v8 {
>>>> +		compatible = "regulator-fixed";
>>>> +		regulator-name = "vcc1v8";
>>>> +		regulator-min-microvolt = <1800000>;
>>>> +		regulator-max-microvolt = <1800000>;
>>>> +	};
>>>> +};
>>>> +
>>>> +&mmc0 {
>>>> +	pinctrl-names = "default";
>>>> +	pinctrl-0 = <&mmc0_pins>;
>>>> +	vmmc-supply = <&reg_vcc3v3>;
>>>
>>> So this is actually CLDO1 on the AXP, correct?
>> 
>> I remember it's coupled between two LDOs, to provide enough power.
>> 
>>>
>>>
>>>> +	cd-gpios = <&pio 5 6 GPIO_ACTIVE_LOW>;
>>>> +	status = "okay";
>>>> +};
>>>> +
>>>> +&mmc2 {
>>>> +	pinctrl-names = "default";
>>>> +	pinctrl-0 = <&mmc2_pins>;
>>>> +	vmmc-supply = <&reg_vcc3v3>;
>>>> +	vqmmc-supply = <&reg_vcc1v8>;
>>>
>>> And this is BLDO2?
>> 
>> Yes.
>> 
>>>
>>> I am just asking because I want to avoid running into the same
>problem
>>> as with the A64 before: that future DTs become incompatible with
>older
>>> kernels, because we change the power supply to point to the AXP
>>> regulators, which this kernel does not support yet.
>> 
>> The answer is just not to keep this compatibility, as it's not
>> supported option to update DT without updating kernel.
>
>Well, I recognise that statement.. ;-) and I understand that it's far
>easier to handle it this way. But:
>- Which .dtb are we going to write into the SPI flash? An older one,
>which covers all kernels, but lacks features? Or a newer one, which
>limits the bootable kernels to recent versions?
>- Which DT are we going to give to EFI applications?
>- Which DT are the BSDs suspected to take? They don't ship their own
>DTs
>(which is good!).
>
>So I understand that "shipping the DT with the kernel" is the old
>(embedded!) way of doing things, but I really believe we should stop
>relying on this and try to come up with backwards compatible DTs, which
>live in the firmware and get updated there. Because this is what the
>distros seem to expect from ARM64 boards these days.

I think in this way we should change the way to submit
patches -- let DT binding patch be merged when it's ready,
and do not wait for driver.

>
>> P.S. I think the DT will update twice on the kernel side, the
>> first time keep reg_vcc3v3 (as it's coupled) but use real
>> regulator for reg_vcc1v8, the second time use the real
>> coupled regulator for reg_vcc3v3.
>> 
>>>
>>> It looks like there are more users of those power rails, so we could
>>> keep those supplies connected to these fixed regulators here, even
>with
>>> AXP-805 support in the kernel.
>> 
>> It's not a good choice.
>> 
>>>
>>> Or we keep this back until we get proper AXP support in the kernel?
>I
>>> guess it's quite close to the existing PMICs, so it might be more a
>>> copy&paste exercise to support the AXP-805?
>> 
>> It's not a reason to keep it back.
>
>So I compared the manuals of the AXP806 and the AXP805, the register
>interface looks identical to me. I only have a (somewhat) Chinese
>version of the AXP806 manual, so couldn't really find the difference
>between the two. Do you know more about it? Is it just maybe the
>packaging and the electrical properties (like max current supported)?
>
>If the I2C register interface is really the same, we could just add the
>DT nodes for the regulator and be done.

They're the same. My following patchset adds AXP805
compatible just to use AXP806 code. I have asked Wink
and the answer is that they have only preset difference.

>
>Cheers,
>Andre.
>
>> 
>>>
>>> But apart from this this looks correct to me.
>>>
>>> Cheers,
>>> Andre.
>>>
>>>> +	non-removable;
>>>> +	cap-mmc-hw-reset;
>>>> +	status = "okay";
>>>>  };
>>>>  
>>>>  &uart0 {
>>>>

Re: [linux-sunxi] [PATCH 3/3] arm64: allwinner: h6: enable MMC0/2 on Pine H64

[Andre Przywara]

Hi,

On 30/04/18 10:51, Icenowy Zheng wrote:
> 
> 
> 于 2018年4月30日 GMT+08:00 下午5:47:35, Andre Przywara <andre.przywara@arm.com> 写到:
>> Hi Icenowy,
>>
>> On 27/04/18 08:12, Icenowy Zheng wrote:
>>>
>>>
>>> 于 2018年4月27日 GMT+08:00 上午12:46:26, Andre Przywara
>> <andre.przywara@arm.com> 写到:
>>>> Hi,
>>>>
>>>> On 26/04/18 15:07, Icenowy Zheng wrote:
>>>>> The Pine H64 board have a MicroSD slot connected to MMC0 controller
>>>> of
>>>>> the H6 SoC and a eMMC slot connected to MMC2.
>>>>>
>>>>> Enable them in the device tree.
>>>>>
>>>>> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
>>>>> ---
>>>>>  .../boot/dts/allwinner/sun50i-h6-pine-h64.dts      | 32
>>>> ++++++++++++++++++++++
>>>>>  1 file changed, 32 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts
>>>> b/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts
>>>>> index d36de5eb81f3..78b1cd54687c 100644
>>>>> --- a/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts
>>>>> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts
>>>>> @@ -20,6 +20,38 @@
>>>>>  	chosen {
>>>>>  		stdout-path = "serial0:115200n8";
>>>>>  	};
>>>>> +
>>>>> +	reg_vcc3v3: vcc3v3 {
>>>>> +		compatible = "regulator-fixed";
>>>>> +		regulator-name = "vcc3v3";
>>>>> +		regulator-min-microvolt = <3300000>;
>>>>> +		regulator-max-microvolt = <3300000>;
>>>>> +	};
>>>>> +
>>>>> +	reg_vcc1v8: vcc1v8 {
>>>>> +		compatible = "regulator-fixed";
>>>>> +		regulator-name = "vcc1v8";
>>>>> +		regulator-min-microvolt = <1800000>;
>>>>> +		regulator-max-microvolt = <1800000>;
>>>>> +	};
>>>>> +};
>>>>> +
>>>>> +&mmc0 {
>>>>> +	pinctrl-names = "default";
>>>>> +	pinctrl-0 = <&mmc0_pins>;
>>>>> +	vmmc-supply = <&reg_vcc3v3>;
>>>>
>>>> So this is actually CLDO1 on the AXP, correct?
>>>
>>> I remember it's coupled between two LDOs, to provide enough power.
>>>
>>>>
>>>>
>>>>> +	cd-gpios = <&pio 5 6 GPIO_ACTIVE_LOW>;
>>>>> +	status = "okay";
>>>>> +};
>>>>> +
>>>>> +&mmc2 {
>>>>> +	pinctrl-names = "default";
>>>>> +	pinctrl-0 = <&mmc2_pins>;
>>>>> +	vmmc-supply = <&reg_vcc3v3>;
>>>>> +	vqmmc-supply = <&reg_vcc1v8>;
>>>>
>>>> And this is BLDO2?
>>>
>>> Yes.
>>>
>>>>
>>>> I am just asking because I want to avoid running into the same
>> problem
>>>> as with the A64 before: that future DTs become incompatible with
>> older
>>>> kernels, because we change the power supply to point to the AXP
>>>> regulators, which this kernel does not support yet.
>>>
>>> The answer is just not to keep this compatibility, as it's not
>>> supported option to update DT without updating kernel.
>>
>> Well, I recognise that statement.. ;-) and I understand that it's far
>> easier to handle it this way. But:
>> - Which .dtb are we going to write into the SPI flash? An older one,
>> which covers all kernels, but lacks features? Or a newer one, which
>> limits the bootable kernels to recent versions?
>> - Which DT are we going to give to EFI applications?
>> - Which DT are the BSDs suspected to take? They don't ship their own
>> DTs
>> (which is good!).
>>
>> So I understand that "shipping the DT with the kernel" is the old
>> (embedded!) way of doing things, but I really believe we should stop
>> relying on this and try to come up with backwards compatible DTs, which
>> live in the firmware and get updated there. Because this is what the
>> distros seem to expect from ARM64 boards these days.
> 
> I think in this way we should change the way to submit
> patches -- let DT binding patch be merged when it's ready,
> and do not wait for driver.

Yes, I agree. Ideally we would look at the hardware description, create
a binding just based on that and submit it.

Then the actual DTs and the drivers (for Linux, U-Boot, *BSD,
you-name-it) could be done independently from each other.

I think we should really aim for that. The only question is whether this
is really practical, since the documentation is sometimes lacking and we
may discover missing properties during driver development.
So when we meanwhile do hand-in-hand development, we should make sure we
don't break anything in the future.

>>> P.S. I think the DT will update twice on the kernel side, the
>>> first time keep reg_vcc3v3 (as it's coupled) but use real
>>> regulator for reg_vcc1v8, the second time use the real
>>> coupled regulator for reg_vcc3v3.
>>>
>>>>
>>>> It looks like there are more users of those power rails, so we could
>>>> keep those supplies connected to these fixed regulators here, even
>> with
>>>> AXP-805 support in the kernel.
>>>
>>> It's not a good choice.
>>>
>>>>
>>>> Or we keep this back until we get proper AXP support in the kernel?
>> I
>>>> guess it's quite close to the existing PMICs, so it might be more a
>>>> copy&paste exercise to support the AXP-805?
>>>
>>> It's not a reason to keep it back.
>>
>> So I compared the manuals of the AXP806 and the AXP805, the register
>> interface looks identical to me. I only have a (somewhat) Chinese
>> version of the AXP806 manual, so couldn't really find the difference
>> between the two. Do you know more about it? Is it just maybe the
>> packaging and the electrical properties (like max current supported)?
>>
>> If the I2C register interface is really the same, we could just add the
>> DT nodes for the regulator and be done.
> 
> They're the same. My following patchset adds AXP805
> compatible just to use AXP806 code. I have asked Wink
> and the answer is that they have only preset difference.

Ah, thanks for that, that's good info!
So in this case we don't even need to add the compatible name to the
driver, just add it to the binding doc and create (or copy) the DT
snippets. See last week's discussion ;-)
And we could aim to merge this together with the MMC driver, so that
there would be no regression.
Isn't that doable? I am happy to review patches on short notice (if you
have them already, otherwise I am happy to make them).

So in summary it looks like all changes could be purely binding doc/DT
changes, so any 4.17 kernel would work already, when presented with the
right DT.

Cheers,
Andre.


> 
>>
>> Cheers,
>> Andre.
>>
>>>
>>>>
>>>> But apart from this this looks correct to me.
>>>>
>>>> Cheers,
>>>> Andre.
>>>>
>>>>> +	non-removable;
>>>>> +	cap-mmc-hw-reset;
>>>>> +	status = "okay";
>>>>>  };
>>>>>  
>>>>>  &uart0 {
>>>>>

Re: [linux-sunxi] [PATCH 3/3] arm64: allwinner: h6: enable MMC0/2 on Pine H64

[Chen-Yu Tsai]

On Mon, Apr 30, 2018 at 6:44 PM, Andre Przywara <andre.przywara@arm.com> wrote:
> Hi,
>
> On 30/04/18 10:51, Icenowy Zheng wrote:
>>
>>
>> 于 2018年4月30日 GMT+08:00 下午5:47:35, Andre Przywara <andre.przywara@arm.com> 写到:
>>> Hi Icenowy,
>>>
>>> On 27/04/18 08:12, Icenowy Zheng wrote:
>>>>
>>>>
>>>> 于 2018年4月27日 GMT+08:00 上午12:46:26, Andre Przywara
>>> <andre.przywara@arm.com> 写到:
>>>>> Hi,
>>>>>
>>>>> On 26/04/18 15:07, Icenowy Zheng wrote:
>>>>>> The Pine H64 board have a MicroSD slot connected to MMC0 controller
>>>>> of
>>>>>> the H6 SoC and a eMMC slot connected to MMC2.
>>>>>>
>>>>>> Enable them in the device tree.
>>>>>>
>>>>>> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
>>>>>> ---
>>>>>>  .../boot/dts/allwinner/sun50i-h6-pine-h64.dts      | 32
>>>>> ++++++++++++++++++++++
>>>>>>  1 file changed, 32 insertions(+)
>>>>>>
>>>>>> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts
>>>>> b/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts
>>>>>> index d36de5eb81f3..78b1cd54687c 100644
>>>>>> --- a/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts
>>>>>> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts
>>>>>> @@ -20,6 +20,38 @@
>>>>>>   chosen {
>>>>>>           stdout-path = "serial0:115200n8";
>>>>>>   };
>>>>>> +
>>>>>> + reg_vcc3v3: vcc3v3 {
>>>>>> +         compatible = "regulator-fixed";
>>>>>> +         regulator-name = "vcc3v3";
>>>>>> +         regulator-min-microvolt = <3300000>;
>>>>>> +         regulator-max-microvolt = <3300000>;
>>>>>> + };
>>>>>> +
>>>>>> + reg_vcc1v8: vcc1v8 {
>>>>>> +         compatible = "regulator-fixed";
>>>>>> +         regulator-name = "vcc1v8";
>>>>>> +         regulator-min-microvolt = <1800000>;
>>>>>> +         regulator-max-microvolt = <1800000>;
>>>>>> + };
>>>>>> +};
>>>>>> +
>>>>>> +&mmc0 {
>>>>>> + pinctrl-names = "default";
>>>>>> + pinctrl-0 = <&mmc0_pins>;
>>>>>> + vmmc-supply = <&reg_vcc3v3>;
>>>>>
>>>>> So this is actually CLDO1 on the AXP, correct?
>>>>
>>>> I remember it's coupled between two LDOs, to provide enough power.
>>>>
>>>>>
>>>>>
>>>>>> + cd-gpios = <&pio 5 6 GPIO_ACTIVE_LOW>;
>>>>>> + status = "okay";
>>>>>> +};
>>>>>> +
>>>>>> +&mmc2 {
>>>>>> + pinctrl-names = "default";
>>>>>> + pinctrl-0 = <&mmc2_pins>;
>>>>>> + vmmc-supply = <&reg_vcc3v3>;
>>>>>> + vqmmc-supply = <&reg_vcc1v8>;
>>>>>
>>>>> And this is BLDO2?
>>>>
>>>> Yes.
>>>>
>>>>>
>>>>> I am just asking because I want to avoid running into the same
>>> problem
>>>>> as with the A64 before: that future DTs become incompatible with
>>> older
>>>>> kernels, because we change the power supply to point to the AXP
>>>>> regulators, which this kernel does not support yet.
>>>>
>>>> The answer is just not to keep this compatibility, as it's not
>>>> supported option to update DT without updating kernel.
>>>
>>> Well, I recognise that statement.. ;-) and I understand that it's far
>>> easier to handle it this way. But:
>>> - Which .dtb are we going to write into the SPI flash? An older one,
>>> which covers all kernels, but lacks features? Or a newer one, which
>>> limits the bootable kernels to recent versions?
>>> - Which DT are we going to give to EFI applications?
>>> - Which DT are the BSDs suspected to take? They don't ship their own
>>> DTs
>>> (which is good!).
>>>
>>> So I understand that "shipping the DT with the kernel" is the old
>>> (embedded!) way of doing things, but I really believe we should stop
>>> relying on this and try to come up with backwards compatible DTs, which
>>> live in the firmware and get updated there. Because this is what the
>>> distros seem to expect from ARM64 boards these days.
>>
>> I think in this way we should change the way to submit
>> patches -- let DT binding patch be merged when it's ready,
>> and do not wait for driver.
>
> Yes, I agree. Ideally we would look at the hardware description, create
> a binding just based on that and submit it.
>
> Then the actual DTs and the drivers (for Linux, U-Boot, *BSD,
> you-name-it) could be done independently from each other.
>
> I think we should really aim for that. The only question is whether this
> is really practical, since the documentation is sometimes lacking and we
> may discover missing properties during driver development.
> So when we meanwhile do hand-in-hand development, we should make sure we
> don't break anything in the future.

We could do that, but for critical regulators it's a bit tricky. Like the
issue with vmmc and vqmmc, where the driver for the regulator is missing,
leading to an unusable system.

>>>> P.S. I think the DT will update twice on the kernel side, the
>>>> first time keep reg_vcc3v3 (as it's coupled) but use real
>>>> regulator for reg_vcc1v8, the second time use the real
>>>> coupled regulator for reg_vcc3v3.
>>>>
>>>>>
>>>>> It looks like there are more users of those power rails, so we could
>>>>> keep those supplies connected to these fixed regulators here, even
>>> with
>>>>> AXP-805 support in the kernel.
>>>>
>>>> It's not a good choice.
>>>>
>>>>>
>>>>> Or we keep this back until we get proper AXP support in the kernel?
>>> I
>>>>> guess it's quite close to the existing PMICs, so it might be more a
>>>>> copy&paste exercise to support the AXP-805?
>>>>
>>>> It's not a reason to keep it back.
>>>
>>> So I compared the manuals of the AXP806 and the AXP805, the register
>>> interface looks identical to me. I only have a (somewhat) Chinese
>>> version of the AXP806 manual, so couldn't really find the difference
>>> between the two. Do you know more about it? Is it just maybe the
>>> packaging and the electrical properties (like max current supported)?
>>>
>>> If the I2C register interface is really the same, we could just add the
>>> DT nodes for the regulator and be done.
>>
>> They're the same. My following patchset adds AXP805
>> compatible just to use AXP806 code. I have asked Wink
>> and the answer is that they have only preset difference.
>
> Ah, thanks for that, that's good info!
> So in this case we don't even need to add the compatible name to the
> driver, just add it to the binding doc and create (or copy) the DT
> snippets. See last week's discussion ;-)

We need to add the compatible to the I2C side of the AXP driver. Also
the property for "standalone mode". I believe I already touched on this
before in another discussion with Icenowy.

> And we could aim to merge this together with the MMC driver, so that
> there would be no regression.
> Isn't that doable? I am happy to review patches on short notice (if you
> have them already, otherwise I am happy to make them).
>
> So in summary it looks like all changes could be purely binding doc/DT
> changes, so any 4.17 kernel would work already, when presented with the
> right DT.

No it won't. See above about the I2C driver.

ChenYu

>
> Cheers,
> Andre.
>
>
>>
>>>
>>> Cheers,
>>> Andre.
>>>
>>>>
>>>>>
>>>>> But apart from this this looks correct to me.
>>>>>
>>>>> Cheers,
>>>>> Andre.
>>>>>
>>>>>> + non-removable;
>>>>>> + cap-mmc-hw-reset;
>>>>>> + status = "okay";
>>>>>>  };
>>>>>>
>>>>>>  &uart0 {
>>>>>>
>
> --
> You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

Re: [linux-sunxi] [PATCH 3/3] arm64: allwinner: h6: enable MMC0/2 on Pine H64

[Andre Przywara]

Hi,

On 01/05/18 16:52, Chen-Yu Tsai wrote:
> On Mon, Apr 30, 2018 at 6:44 PM, Andre Przywara <andre.przywara@arm.com> wrote:
>> Hi,
>>
>> On 30/04/18 10:51, Icenowy Zheng wrote:
>>>
>>>
>>> 于 2018年4月30日 GMT+08:00 下午5:47:35, Andre Przywara <andre.przywara@arm.com> 写到:
>>>> Hi Icenowy,
>>>>
>>>> On 27/04/18 08:12, Icenowy Zheng wrote:
>>>>>
>>>>>
>>>>> 于 2018年4月27日 GMT+08:00 上午12:46:26, Andre Przywara
>>>> <andre.przywara@arm.com> 写到:
>>>>>> Hi,
>>>>>>
>>>>>> On 26/04/18 15:07, Icenowy Zheng wrote:
>>>>>>> The Pine H64 board have a MicroSD slot connected to MMC0 controller
>>>>>> of
>>>>>>> the H6 SoC and a eMMC slot connected to MMC2.
>>>>>>>
>>>>>>> Enable them in the device tree.
>>>>>>>
>>>>>>> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
>>>>>>> ---
>>>>>>>  .../boot/dts/allwinner/sun50i-h6-pine-h64.dts      | 32
>>>>>> ++++++++++++++++++++++
>>>>>>>  1 file changed, 32 insertions(+)
>>>>>>>
>>>>>>> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts
>>>>>> b/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts
>>>>>>> index d36de5eb81f3..78b1cd54687c 100644
>>>>>>> --- a/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts
>>>>>>> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts
>>>>>>> @@ -20,6 +20,38 @@
>>>>>>>   chosen {
>>>>>>>           stdout-path = "serial0:115200n8";
>>>>>>>   };
>>>>>>> +
>>>>>>> + reg_vcc3v3: vcc3v3 {
>>>>>>> +         compatible = "regulator-fixed";
>>>>>>> +         regulator-name = "vcc3v3";
>>>>>>> +         regulator-min-microvolt = <3300000>;
>>>>>>> +         regulator-max-microvolt = <3300000>;
>>>>>>> + };
>>>>>>> +
>>>>>>> + reg_vcc1v8: vcc1v8 {
>>>>>>> +         compatible = "regulator-fixed";
>>>>>>> +         regulator-name = "vcc1v8";
>>>>>>> +         regulator-min-microvolt = <1800000>;
>>>>>>> +         regulator-max-microvolt = <1800000>;
>>>>>>> + };
>>>>>>> +};
>>>>>>> +
>>>>>>> +&mmc0 {
>>>>>>> + pinctrl-names = "default";
>>>>>>> + pinctrl-0 = <&mmc0_pins>;
>>>>>>> + vmmc-supply = <&reg_vcc3v3>;
>>>>>>
>>>>>> So this is actually CLDO1 on the AXP, correct?
>>>>>
>>>>> I remember it's coupled between two LDOs, to provide enough power.
>>>>>
>>>>>>
>>>>>>
>>>>>>> + cd-gpios = <&pio 5 6 GPIO_ACTIVE_LOW>;
>>>>>>> + status = "okay";
>>>>>>> +};
>>>>>>> +
>>>>>>> +&mmc2 {
>>>>>>> + pinctrl-names = "default";
>>>>>>> + pinctrl-0 = <&mmc2_pins>;
>>>>>>> + vmmc-supply = <&reg_vcc3v3>;
>>>>>>> + vqmmc-supply = <&reg_vcc1v8>;
>>>>>>
>>>>>> And this is BLDO2?
>>>>>
>>>>> Yes.
>>>>>
>>>>>>
>>>>>> I am just asking because I want to avoid running into the same
>>>> problem
>>>>>> as with the A64 before: that future DTs become incompatible with
>>>> older
>>>>>> kernels, because we change the power supply to point to the AXP
>>>>>> regulators, which this kernel does not support yet.
>>>>>
>>>>> The answer is just not to keep this compatibility, as it's not
>>>>> supported option to update DT without updating kernel.
>>>>
>>>> Well, I recognise that statement.. ;-) and I understand that it's far
>>>> easier to handle it this way. But:
>>>> - Which .dtb are we going to write into the SPI flash? An older one,
>>>> which covers all kernels, but lacks features? Or a newer one, which
>>>> limits the bootable kernels to recent versions?
>>>> - Which DT are we going to give to EFI applications?
>>>> - Which DT are the BSDs suspected to take? They don't ship their own
>>>> DTs
>>>> (which is good!).
>>>>
>>>> So I understand that "shipping the DT with the kernel" is the old
>>>> (embedded!) way of doing things, but I really believe we should stop
>>>> relying on this and try to come up with backwards compatible DTs, which
>>>> live in the firmware and get updated there. Because this is what the
>>>> distros seem to expect from ARM64 boards these days.
>>>
>>> I think in this way we should change the way to submit
>>> patches -- let DT binding patch be merged when it's ready,
>>> and do not wait for driver.
>>
>> Yes, I agree. Ideally we would look at the hardware description, create
>> a binding just based on that and submit it.
>>
>> Then the actual DTs and the drivers (for Linux, U-Boot, *BSD,
>> you-name-it) could be done independently from each other.
>>
>> I think we should really aim for that. The only question is whether this
>> is really practical, since the documentation is sometimes lacking and we
>> may discover missing properties during driver development.
>> So when we meanwhile do hand-in-hand development, we should make sure we
>> don't break anything in the future.
> 
> We could do that, but for critical regulators it's a bit tricky. Like the
> issue with vmmc and vqmmc, where the driver for the regulator is missing,
> leading to an unusable system.

Yes, that was my original point. We can already anticipate that we will
break forward compatibility, so we can try to do something about that
now to avoid that, either by staying with fixed regulators, or by adding
the PMIC early.

>>>>> P.S. I think the DT will update twice on the kernel side, the
>>>>> first time keep reg_vcc3v3 (as it's coupled) but use real
>>>>> regulator for reg_vcc1v8, the second time use the real
>>>>> coupled regulator for reg_vcc3v3.
>>>>>
>>>>>>
>>>>>> It looks like there are more users of those power rails, so we could
>>>>>> keep those supplies connected to these fixed regulators here, even
>>>> with
>>>>>> AXP-805 support in the kernel.
>>>>>
>>>>> It's not a good choice.
>>>>>
>>>>>>
>>>>>> Or we keep this back until we get proper AXP support in the kernel?
>>>> I
>>>>>> guess it's quite close to the existing PMICs, so it might be more a
>>>>>> copy&paste exercise to support the AXP-805?
>>>>>
>>>>> It's not a reason to keep it back.
>>>>
>>>> So I compared the manuals of the AXP806 and the AXP805, the register
>>>> interface looks identical to me. I only have a (somewhat) Chinese
>>>> version of the AXP806 manual, so couldn't really find the difference
>>>> between the two. Do you know more about it? Is it just maybe the
>>>> packaging and the electrical properties (like max current supported)?
>>>>
>>>> If the I2C register interface is really the same, we could just add the
>>>> DT nodes for the regulator and be done.
>>>
>>> They're the same. My following patchset adds AXP805
>>> compatible just to use AXP806 code. I have asked Wink
>>> and the answer is that they have only preset difference.
>>
>> Ah, thanks for that, that's good info!
>> So in this case we don't even need to add the compatible name to the
>> driver, just add it to the binding doc and create (or copy) the DT
>> snippets. See last week's discussion ;-)
> 
> We need to add the compatible to the I2C side of the AXP driver.

Why? If it's really 100% compatible, we just add it to the binding doc
and use compatible = "x-powers,axp805", "x-powers,axp806"; in the DT.
That would immediately enable other OSes, for instance.

> Also the property for "standalone mode".

Are you referring to what the manual refers to as "self-work" mode?
In this case I don't see why we need a property: this mode is set up on
the board side by leaving the MODESET pin floating. And it can be
queried by checking bits[7:6] of REG 00, so doesn't need a DT property.
If we care about this mode (do we?), we can check for this in the driver.

(Curious if you meant something else ...)

Cheers,
Andre.


> I believe I already touched on this
> before in another discussion with Icenowy.
> 
>> And we could aim to merge this together with the MMC driver, so that
>> there would be no regression.
>> Isn't that doable? I am happy to review patches on short notice (if you
>> have them already, otherwise I am happy to make them).
>>
>> So in summary it looks like all changes could be purely binding doc/DT
>> changes, so any 4.17 kernel would work already, when presented with the
>> right DT.
> 
> No it won't. See above about the I2C driver.
> 
> ChenYu
> 
>>
>> Cheers,
>> Andre.
>>
>>
>>>
>>>>
>>>> Cheers,
>>>> Andre.
>>>>
>>>>>
>>>>>>
>>>>>> But apart from this this looks correct to me.
>>>>>>
>>>>>> Cheers,
>>>>>> Andre.
>>>>>>
>>>>>>> + non-removable;
>>>>>>> + cap-mmc-hw-reset;
>>>>>>> + status = "okay";
>>>>>>>  };
>>>>>>>
>>>>>>>  &uart0 {
>>>>>>>
>>
>> --
>> You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
>> To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe@googlegroups.com.
>> For more options, visit https://groups.google.com/d/optout.

Re: [linux-sunxi] [PATCH 3/3] arm64: allwinner: h6: enable MMC0/2 on Pine H64

[Chen-Yu Tsai]

On Wed, May 2, 2018 at 7:01 PM, Andre Przywara <andre.przywara@arm.com> wrote:
> Hi,
>
> On 01/05/18 16:52, Chen-Yu Tsai wrote:
>> On Mon, Apr 30, 2018 at 6:44 PM, Andre Przywara <andre.przywara@arm.com> wrote:
>>> Hi,
>>>
>>> On 30/04/18 10:51, Icenowy Zheng wrote:
>>>>
>>>>
>>>> 于 2018年4月30日 GMT+08:00 下午5:47:35, Andre Przywara <andre.przywara@arm.com> 写到:
>>>>> Hi Icenowy,
>>>>>
>>>>> On 27/04/18 08:12, Icenowy Zheng wrote:
>>>>>>
>>>>>>
>>>>>> 于 2018年4月27日 GMT+08:00 上午12:46:26, Andre Przywara
>>>>> <andre.przywara@arm.com> 写到:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 26/04/18 15:07, Icenowy Zheng wrote:
>>>>>>>> The Pine H64 board have a MicroSD slot connected to MMC0 controller
>>>>>>> of
>>>>>>>> the H6 SoC and a eMMC slot connected to MMC2.
>>>>>>>>
>>>>>>>> Enable them in the device tree.
>>>>>>>>
>>>>>>>> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
>>>>>>>> ---
>>>>>>>>  .../boot/dts/allwinner/sun50i-h6-pine-h64.dts      | 32
>>>>>>> ++++++++++++++++++++++
>>>>>>>>  1 file changed, 32 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts
>>>>>>> b/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts
>>>>>>>> index d36de5eb81f3..78b1cd54687c 100644
>>>>>>>> --- a/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts
>>>>>>>> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts
>>>>>>>> @@ -20,6 +20,38 @@
>>>>>>>>   chosen {
>>>>>>>>           stdout-path = "serial0:115200n8";
>>>>>>>>   };
>>>>>>>> +
>>>>>>>> + reg_vcc3v3: vcc3v3 {
>>>>>>>> +         compatible = "regulator-fixed";
>>>>>>>> +         regulator-name = "vcc3v3";
>>>>>>>> +         regulator-min-microvolt = <3300000>;
>>>>>>>> +         regulator-max-microvolt = <3300000>;
>>>>>>>> + };
>>>>>>>> +
>>>>>>>> + reg_vcc1v8: vcc1v8 {
>>>>>>>> +         compatible = "regulator-fixed";
>>>>>>>> +         regulator-name = "vcc1v8";
>>>>>>>> +         regulator-min-microvolt = <1800000>;
>>>>>>>> +         regulator-max-microvolt = <1800000>;
>>>>>>>> + };
>>>>>>>> +};
>>>>>>>> +
>>>>>>>> +&mmc0 {
>>>>>>>> + pinctrl-names = "default";
>>>>>>>> + pinctrl-0 = <&mmc0_pins>;
>>>>>>>> + vmmc-supply = <&reg_vcc3v3>;
>>>>>>>
>>>>>>> So this is actually CLDO1 on the AXP, correct?
>>>>>>
>>>>>> I remember it's coupled between two LDOs, to provide enough power.
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> + cd-gpios = <&pio 5 6 GPIO_ACTIVE_LOW>;
>>>>>>>> + status = "okay";
>>>>>>>> +};
>>>>>>>> +
>>>>>>>> +&mmc2 {
>>>>>>>> + pinctrl-names = "default";
>>>>>>>> + pinctrl-0 = <&mmc2_pins>;
>>>>>>>> + vmmc-supply = <&reg_vcc3v3>;
>>>>>>>> + vqmmc-supply = <&reg_vcc1v8>;
>>>>>>>
>>>>>>> And this is BLDO2?
>>>>>>
>>>>>> Yes.
>>>>>>
>>>>>>>
>>>>>>> I am just asking because I want to avoid running into the same
>>>>> problem
>>>>>>> as with the A64 before: that future DTs become incompatible with
>>>>> older
>>>>>>> kernels, because we change the power supply to point to the AXP
>>>>>>> regulators, which this kernel does not support yet.
>>>>>>
>>>>>> The answer is just not to keep this compatibility, as it's not
>>>>>> supported option to update DT without updating kernel.
>>>>>
>>>>> Well, I recognise that statement.. ;-) and I understand that it's far
>>>>> easier to handle it this way. But:
>>>>> - Which .dtb are we going to write into the SPI flash? An older one,
>>>>> which covers all kernels, but lacks features? Or a newer one, which
>>>>> limits the bootable kernels to recent versions?
>>>>> - Which DT are we going to give to EFI applications?
>>>>> - Which DT are the BSDs suspected to take? They don't ship their own
>>>>> DTs
>>>>> (which is good!).
>>>>>
>>>>> So I understand that "shipping the DT with the kernel" is the old
>>>>> (embedded!) way of doing things, but I really believe we should stop
>>>>> relying on this and try to come up with backwards compatible DTs, which
>>>>> live in the firmware and get updated there. Because this is what the
>>>>> distros seem to expect from ARM64 boards these days.
>>>>
>>>> I think in this way we should change the way to submit
>>>> patches -- let DT binding patch be merged when it's ready,
>>>> and do not wait for driver.
>>>
>>> Yes, I agree. Ideally we would look at the hardware description, create
>>> a binding just based on that and submit it.
>>>
>>> Then the actual DTs and the drivers (for Linux, U-Boot, *BSD,
>>> you-name-it) could be done independently from each other.
>>>
>>> I think we should really aim for that. The only question is whether this
>>> is really practical, since the documentation is sometimes lacking and we
>>> may discover missing properties during driver development.
>>> So when we meanwhile do hand-in-hand development, we should make sure we
>>> don't break anything in the future.
>>
>> We could do that, but for critical regulators it's a bit tricky. Like the
>> issue with vmmc and vqmmc, where the driver for the regulator is missing,
>> leading to an unusable system.
>
> Yes, that was my original point. We can already anticipate that we will
> break forward compatibility, so we can try to do something about that
> now to avoid that, either by staying with fixed regulators, or by adding
> the PMIC early.
>
>>>>>> P.S. I think the DT will update twice on the kernel side, the
>>>>>> first time keep reg_vcc3v3 (as it's coupled) but use real
>>>>>> regulator for reg_vcc1v8, the second time use the real
>>>>>> coupled regulator for reg_vcc3v3.
>>>>>>
>>>>>>>
>>>>>>> It looks like there are more users of those power rails, so we could
>>>>>>> keep those supplies connected to these fixed regulators here, even
>>>>> with
>>>>>>> AXP-805 support in the kernel.
>>>>>>
>>>>>> It's not a good choice.
>>>>>>
>>>>>>>
>>>>>>> Or we keep this back until we get proper AXP support in the kernel?
>>>>> I
>>>>>>> guess it's quite close to the existing PMICs, so it might be more a
>>>>>>> copy&paste exercise to support the AXP-805?
>>>>>>
>>>>>> It's not a reason to keep it back.
>>>>>
>>>>> So I compared the manuals of the AXP806 and the AXP805, the register
>>>>> interface looks identical to me. I only have a (somewhat) Chinese
>>>>> version of the AXP806 manual, so couldn't really find the difference
>>>>> between the two. Do you know more about it? Is it just maybe the
>>>>> packaging and the electrical properties (like max current supported)?
>>>>>
>>>>> If the I2C register interface is really the same, we could just add the
>>>>> DT nodes for the regulator and be done.
>>>>
>>>> They're the same. My following patchset adds AXP805
>>>> compatible just to use AXP806 code. I have asked Wink
>>>> and the answer is that they have only preset difference.
>>>
>>> Ah, thanks for that, that's good info!
>>> So in this case we don't even need to add the compatible name to the
>>> driver, just add it to the binding doc and create (or copy) the DT
>>> snippets. See last week's discussion ;-)
>>
>> We need to add the compatible to the I2C side of the AXP driver.
>
> Why? If it's really 100% compatible, we just add it to the binding doc
> and use compatible = "x-powers,axp805", "x-powers,axp806"; in the DT.
> That would immediately enable other OSes, for instance.

I meant the implementation side, not the DT. The AXP I2C driver only
binds to the earlier (up to AXP221) PMICs. So just having the device
tree correctly set up does not guarantee a working system for the
current release, or the one in -rc.

>> Also the property for "standalone mode".
>
> Are you referring to what the manual refers to as "self-work" mode?
> In this case I don't see why we need a property: this mode is set up on
> the board side by leaving the MODESET pin floating. And it can be
> queried by checking bits[7:6] of REG 00, so doesn't need a DT property.
> If we care about this mode (do we?), we can check for this in the driver.
>
> (Curious if you meant something else ...)

Yes this is what I was referring to. If you look at the last two registers
you'll see that self-work mode, along with master mode, require the address
extension register be programmed before access to any other register can
happen.

We already have the "x-powers,master-mode" property. The driver will program
the registers correctly when it sees this property. I'm arguing that we
should have another "x-powers,self-work-mode", because these two modes are
not the same: The enable pin works as a power key under self-work mode, while
it acts as a level triggered enable pi, as the name describes. There are
some other minor differences described in the datasheet. The device tree,
being a hardware description, should not conflate the two and leave it up
to the driver to figure things out.

ChenYu

Re: [linux-sunxi] [PATCH 3/3] arm64: allwinner: h6: enable MMC0/2 on Pine H64

[Chen-Yu Tsai]

On Mon, Apr 30, 2018 at 5:47 PM, Andre Przywara <andre.przywara@arm.com> wrote:
> Hi Icenowy,
>
> On 27/04/18 08:12, Icenowy Zheng wrote:
>>
>>
>> 于 2018年4月27日 GMT+08:00 上午12:46:26, Andre Przywara <andre.przywara@arm.com> 写到:
>>> Hi,
>>>
>>> On 26/04/18 15:07, Icenowy Zheng wrote:
>>>> The Pine H64 board have a MicroSD slot connected to MMC0 controller
>>> of
>>>> the H6 SoC and a eMMC slot connected to MMC2.
>>>>
>>>> Enable them in the device tree.
>>>>
>>>> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
>>>> ---
>>>>  .../boot/dts/allwinner/sun50i-h6-pine-h64.dts      | 32
>>> ++++++++++++++++++++++
>>>>  1 file changed, 32 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts
>>> b/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts
>>>> index d36de5eb81f3..78b1cd54687c 100644
>>>> --- a/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts
>>>> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dts
>>>> @@ -20,6 +20,38 @@
>>>>     chosen {
>>>>             stdout-path = "serial0:115200n8";
>>>>     };
>>>> +
>>>> +   reg_vcc3v3: vcc3v3 {
>>>> +           compatible = "regulator-fixed";
>>>> +           regulator-name = "vcc3v3";
>>>> +           regulator-min-microvolt = <3300000>;
>>>> +           regulator-max-microvolt = <3300000>;
>>>> +   };
>>>> +
>>>> +   reg_vcc1v8: vcc1v8 {
>>>> +           compatible = "regulator-fixed";
>>>> +           regulator-name = "vcc1v8";
>>>> +           regulator-min-microvolt = <1800000>;
>>>> +           regulator-max-microvolt = <1800000>;
>>>> +   };
>>>> +};
>>>> +
>>>> +&mmc0 {
>>>> +   pinctrl-names = "default";
>>>> +   pinctrl-0 = <&mmc0_pins>;
>>>> +   vmmc-supply = <&reg_vcc3v3>;
>>>
>>> So this is actually CLDO1 on the AXP, correct?
>>
>> I remember it's coupled between two LDOs, to provide enough power.
>>
>>>
>>>
>>>> +   cd-gpios = <&pio 5 6 GPIO_ACTIVE_LOW>;
>>>> +   status = "okay";
>>>> +};
>>>> +
>>>> +&mmc2 {
>>>> +   pinctrl-names = "default";
>>>> +   pinctrl-0 = <&mmc2_pins>;
>>>> +   vmmc-supply = <&reg_vcc3v3>;
>>>> +   vqmmc-supply = <&reg_vcc1v8>;
>>>
>>> And this is BLDO2?
>>
>> Yes.
>>
>>>
>>> I am just asking because I want to avoid running into the same problem
>>> as with the A64 before: that future DTs become incompatible with older
>>> kernels, because we change the power supply to point to the AXP
>>> regulators, which this kernel does not support yet.
>>
>> The answer is just not to keep this compatibility, as it's not
>> supported option to update DT without updating kernel.
>
> Well, I recognise that statement.. ;-) and I understand that it's far
> easier to handle it this way. But:
> - Which .dtb are we going to write into the SPI flash? An older one,
> which covers all kernels, but lacks features? Or a newer one, which
> limits the bootable kernels to recent versions?
> - Which DT are we going to give to EFI applications?
> - Which DT are the BSDs suspected to take? They don't ship their own DTs
> (which is good!).
>
> So I understand that "shipping the DT with the kernel" is the old
> (embedded!) way of doing things, but I really believe we should stop
> relying on this and try to come up with backwards compatible DTs, which
> live in the firmware and get updated there. Because this is what the
> distros seem to expect from ARM64 boards these days.
>
>> P.S. I think the DT will update twice on the kernel side, the
>> first time keep reg_vcc3v3 (as it's coupled) but use real
>> regulator for reg_vcc1v8, the second time use the real
>> coupled regulator for reg_vcc3v3.
>>
>>>
>>> It looks like there are more users of those power rails, so we could
>>> keep those supplies connected to these fixed regulators here, even with
>>> AXP-805 support in the kernel.
>>
>> It's not a good choice.
>>
>>>
>>> Or we keep this back until we get proper AXP support in the kernel? I
>>> guess it's quite close to the existing PMICs, so it might be more a
>>> copy&paste exercise to support the AXP-805?
>>
>> It's not a reason to keep it back.
>
> So I compared the manuals of the AXP806 and the AXP805, the register
> interface looks identical to me. I only have a (somewhat) Chinese
> version of the AXP806 manual, so couldn't really find the difference
> between the two. Do you know more about it? Is it just maybe the
> packaging and the electrical properties (like max current supported)?

>From what I could tell, they are the same. I'm not sure about max current,
but that's something not described in the device tree right now. FYI,
the AXP221s and AXP221 differ slightly in that one has increased current
capability for one or two of the DC-DCs. We treat them as if they were
the same right now.

> If the I2C register interface is really the same, we could just add the
> DT nodes for the regulator and be done.

It is. However, we currently lack a "standalone operating mode" property.
This is different from "master mode".

ChenYu

> Cheers,
> Andre.
>
>>
>>>
>>> But apart from this this looks correct to me.
>>>
>>> Cheers,
>>> Andre.
>>>
>>>> +   non-removable;
>>>> +   cap-mmc-hw-reset;
>>>> +   status = "okay";
>>>>  };
>>>>
>>>>  &uart0 {
>>>>

Re: [linux-sunxi] [PATCH 3/3] arm64: allwinner: h6: enable MMC0/2 on Pine H64

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2717 bytes --]

On Mon, Apr 30, 2018 at 10:47:35AM +0100, Andre Przywara wrote:
> >> I am just asking because I want to avoid running into the same problem
> >> as with the A64 before: that future DTs become incompatible with older
> >> kernels, because we change the power supply to point to the AXP
> >> regulators, which this kernel does not support yet.
> > 
> > The answer is just not to keep this compatibility, as it's not
> > supported option to update DT without updating kernel.
> 
> Well, I recognise that statement.. ;-) and I understand that it's far
> easier to handle it this way. But:
> - Which .dtb are we going to write into the SPI flash? An older one,
> which covers all kernels, but lacks features? Or a newer one, which
> limits the bootable kernels to recent versions?
> - Which DT are we going to give to EFI applications?
> - Which DT are the BSDs suspected to take? They don't ship their own DTs
> (which is good!).
> 
> So I understand that "shipping the DT with the kernel" is the old
> (embedded!) way of doing things, but I really believe we should stop
> relying on this and try to come up with backwards compatible DTs, which
> live in the firmware and get updated there. Because this is what the
> distros seem to expect from ARM64 boards these days.

You're not talking about backward compatibility, you're talking about
forward compatibility. All the changes in this patch and the one
predicted by Icenowy are backward compatible.

> >> It looks like there are more users of those power rails, so we could
> >> keep those supplies connected to these fixed regulators here, even with
> >> AXP-805 support in the kernel.
> > 
> > It's not a good choice.
> > 
> >>
> >> Or we keep this back until we get proper AXP support in the kernel? I
> >> guess it's quite close to the existing PMICs, so it might be more a
> >> copy&paste exercise to support the AXP-805?
> > 
> > It's not a reason to keep it back.
> 
> So I compared the manuals of the AXP806 and the AXP805, the register
> interface looks identical to me. I only have a (somewhat) Chinese
> version of the AXP806 manual, so couldn't really find the difference
> between the two. Do you know more about it? Is it just maybe the
> packaging and the electrical properties (like max current supported)?
>
> If the I2C register interface is really the same, we could just add the
> DT nodes for the regulator and be done.

And that argument is only valid if you 100% trust the fact that both
datasheet are complete and accurate.

And experience show that you can't.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [linux-sunxi] [PATCH 3/3] arm64: allwinner: h6: enable MMC0/2 on Pine H64

[Andre Przywara]

Hi,

On 02/05/18 10:36, Maxime Ripard wrote:
> On Mon, Apr 30, 2018 at 10:47:35AM +0100, Andre Przywara wrote:
>>>> I am just asking because I want to avoid running into the same problem
>>>> as with the A64 before: that future DTs become incompatible with older
>>>> kernels, because we change the power supply to point to the AXP
>>>> regulators, which this kernel does not support yet.
>>>
>>> The answer is just not to keep this compatibility, as it's not
>>> supported option to update DT without updating kernel.
>>
>> Well, I recognise that statement.. ;-) and I understand that it's far
>> easier to handle it this way. But:
>> - Which .dtb are we going to write into the SPI flash? An older one,
>> which covers all kernels, but lacks features? Or a newer one, which
>> limits the bootable kernels to recent versions?
>> - Which DT are we going to give to EFI applications?
>> - Which DT are the BSDs suspected to take? They don't ship their own DTs
>> (which is good!).
>>
>> So I understand that "shipping the DT with the kernel" is the old
>> (embedded!) way of doing things, but I really believe we should stop
>> relying on this and try to come up with backwards compatible DTs, which
>> live in the firmware and get updated there. Because this is what the
>> distros seem to expect from ARM64 boards these days.
> 
> You're not talking about backward compatibility, you're talking about
> forward compatibility. All the changes in this patch and the one
> predicted by Icenowy are backward compatible.

Argh, yes, I meant forward compatibility, sorry!
Shouldn't write those emails while actually doing something completely
different ;-)

>>>> It looks like there are more users of those power rails, so we could
>>>> keep those supplies connected to these fixed regulators here, even with
>>>> AXP-805 support in the kernel.
>>>
>>> It's not a good choice.
>>>
>>>>
>>>> Or we keep this back until we get proper AXP support in the kernel? I
>>>> guess it's quite close to the existing PMICs, so it might be more a
>>>> copy&paste exercise to support the AXP-805?
>>>
>>> It's not a reason to keep it back.
>>
>> So I compared the manuals of the AXP806 and the AXP805, the register
>> interface looks identical to me. I only have a (somewhat) Chinese
>> version of the AXP806 manual, so couldn't really find the difference
>> between the two. Do you know more about it? Is it just maybe the
>> packaging and the electrical properties (like max current supported)?
>>
>> If the I2C register interface is really the same, we could just add the
>> DT nodes for the regulator and be done.
> 
> And that argument is only valid if you 100% trust the fact that both
> datasheet are complete and accurate.
> 
> And experience show that you can't.

Well, but I wonder how paranoid we are going to be? And in this case we
have confirmation from Wink that they are the same.
So I think we can go with just a DT addition, given that we test it and
confirm that it works for our use case. Should we discover something odd
or undocumented later, I'd consider this a bug fix, which we then (and
only then!) could fix by adding the compatible string to the driver. Any
DT would be fine already, because we list both compatible strings in there.

Cheers,
Andre.

Re: [linux-sunxi] [PATCH 3/3] arm64: allwinner: h6: enable MMC0/2 on Pine H64

[Maxime Ripard]

On Wed, May 02, 2018 at 12:01:53PM +0100, Andre Przywara wrote:
> >>>> It looks like there are more users of those power rails, so we could
> >>>> keep those supplies connected to these fixed regulators here, even with
> >>>> AXP-805 support in the kernel.
> >>>
> >>> It's not a good choice.
> >>>
> >>>>
> >>>> Or we keep this back until we get proper AXP support in the kernel? I
> >>>> guess it's quite close to the existing PMICs, so it might be more a
> >>>> copy&paste exercise to support the AXP-805?
> >>>
> >>> It's not a reason to keep it back.
> >>
> >> So I compared the manuals of the AXP806 and the AXP805, the register
> >> interface looks identical to me. I only have a (somewhat) Chinese
> >> version of the AXP806 manual, so couldn't really find the difference
> >> between the two. Do you know more about it? Is it just maybe the
> >> packaging and the electrical properties (like max current supported)?
> >>
> >> If the I2C register interface is really the same, we could just add the
> >> DT nodes for the regulator and be done.
> > 
> > And that argument is only valid if you 100% trust the fact that both
> > datasheet are complete and accurate.
> > 
> > And experience show that you can't.
> 
> Well, but I wonder how paranoid we are going to be? And in this case we
> have confirmation from Wink that they are the same.

Paranoid enough so that we don't blindly trust that the reviewer had a
coffee, no interruptions or moment of distraction, or that the
datasheet is correct.

But not so paranoid that having the driver running on a kernel is
enough.

> So I think we can go with just a DT addition, given that we test it
> and confirm that it works for our use case. Should we discover
> something odd or undocumented later, I'd consider this a bug fix,
> which we then (and only then!) could fix by adding the compatible
> string to the driver. Any DT would be fine already, because we list
> both compatible strings in there.

In this particular case, yeah, it seems reasonable.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com