[PATCH 0/4] arm64: amlogic: mmc: meson-gx: Add core, tx, rx

[PATCH 0/4] arm64: amlogic: mmc: meson-gx: Add core, tx, rx

[Vyacheslav Bocharov]

The mmc driver use the same phase values (core - 180, tx/rx - 0) for all
meson64 platforms. However, some platforms (and even some boards) require
different values (axg for example use 270 degree for core clock). This patch
transfers the values from the code to the variables in the device-tree files.
If not set in dts, use old default values.

Vyacheslav Bocharov (4):
  arm64: amlogic: mmc: meson-gx: Add core, tx, rx eMMC/SD/SDIO phase
    clock settings from devicetree data
  arm64: amlogic: mmc: meson-gx: Add dts binding include for core, tx,
    rx eMMC/SD/SDIO phase clock settings from devicetree data
  arm64: amlogic: dts: meson: update meson-axg device-tree for new core,
    tx, rx phase clock settings.
  arm64: dts: docs: Update mmc meson-gx documentation for new config
    option amlogic,mmc-phase

 .../bindings/mmc/amlogic,meson-gx.txt         |  7 ++++
 arch/arm64/boot/dts/amlogic/meson-axg.dtsi    |  3 ++
 drivers/mmc/host/meson-gx-mmc.c               | 18 +++++++---
 include/dt-bindings/mmc/meson-gx-mmc.h        | 35 +++++++++++++++++++
 4 files changed, 58 insertions(+), 5 deletions(-)
 create mode 100644 include/dt-bindings/mmc/meson-gx-mmc.h

-- 
2.30.2

[PATCH 1/4] arm64: amlogic: mmc: meson-gx: Add core, tx, rx eMMC/SD/SDIO phase clock settings from devicetree data

[Vyacheslav Bocharov]

The mmc driver has the same phase values for all meson platforms. However,
some platforms (and even some boards) require different values. This patch
transfers the values from the set in the code to the variables in the
device-tree file.

Signed-off-by: Vyacheslav Bocharov <adeep@lexina.in>

diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c
index df05e60bed9a..c0f32054e472 100644
--- a/drivers/mmc/host/meson-gx-mmc.c
+++ b/drivers/mmc/host/meson-gx-mmc.c
@@ -27,6 +27,7 @@
 #include <linux/interrupt.h>
 #include <linux/bitfield.h>
 #include <linux/pinctrl/consumer.h>
+#include <dt-bindings/mmc/meson-gx-mmc.h>
 
 #define DRIVER_NAME "meson-gx-mmc"
 
@@ -36,8 +37,6 @@
 #define   CLK_CORE_PHASE_MASK GENMASK(9, 8)
 #define   CLK_TX_PHASE_MASK GENMASK(11, 10)
 #define   CLK_RX_PHASE_MASK GENMASK(13, 12)
-#define   CLK_PHASE_0 0
-#define   CLK_PHASE_180 2
 #define   CLK_V2_TX_DELAY_MASK GENMASK(19, 16)
 #define   CLK_V2_RX_DELAY_MASK GENMASK(23, 20)
 #define   CLK_V2_ALWAYS_ON BIT(24)
@@ -428,13 +427,22 @@ static int meson_mmc_clk_init(struct meson_host *host)
 	const char *mux_parent_names[MUX_CLK_NUM_PARENTS];
 	const char *clk_parent[1];
 	u32 clk_reg;
+	u32 phase[3]; // <core_phase, tx_phase, rx_phase>
+
+	if (!(host->dev && host->dev->of_node) || (device_property_read_u32_array(host->dev,
+	    "amlogic,mmc-phase", phase, 3) < 0)) {
+		dev_dbg(host->dev, "get amlogic,mmc-phase failed, use default phase settings\n");
+		phase[0] = CLK_PHASE_180;
+		phase[1] = CLK_PHASE_0;
+		phase[2] = CLK_PHASE_0;
+	}
 
 	/* init SD_EMMC_CLOCK to sane defaults w/min clock rate */
 	clk_reg = CLK_ALWAYS_ON(host);
 	clk_reg |= CLK_DIV_MASK;
-	clk_reg |= FIELD_PREP(CLK_CORE_PHASE_MASK, CLK_PHASE_180);
-	clk_reg |= FIELD_PREP(CLK_TX_PHASE_MASK, CLK_PHASE_0);
-	clk_reg |= FIELD_PREP(CLK_RX_PHASE_MASK, CLK_PHASE_0);
+	clk_reg |= FIELD_PREP(CLK_CORE_PHASE_MASK, phase[0]);
+	clk_reg |= FIELD_PREP(CLK_TX_PHASE_MASK, phase[1]);
+	clk_reg |= FIELD_PREP(CLK_RX_PHASE_MASK, phase[2]);
 	clk_reg |= CLK_IRQ_SDIO_SLEEP(host);
 	writel(clk_reg, host->regs + SD_EMMC_CLOCK);
 
-- 
2.30.2

Re: [PATCH 1/4] arm64: amlogic: mmc: meson-gx: Add core, tx, rx eMMC/SD/SDIO phase clock settings from devicetree data

[Martin Blumenstingl]

Hi Vyacheslav,

On Thu, Nov 10, 2022 at 4:01 PM Vyacheslav Bocharov <adeep@lexina.in> wrote:
>
> The mmc driver has the same phase values for all meson platforms. However,
> some platforms (and even some boards) require different values.
Could you please elaborate which phases are per platform and which are
per board?
In this series you're only managing the core phase differently for all
AXG boards. IMO this can be achieved by extending struct
meson_mmc_data with the desired core clock phase


Best regards,
Martin

[PATCH 2/4] arm64: amlogic: mmc: meson-gx: Add dts binding include for core, tx, rx eMMC/SD/SDIO phase clock settings from devicetree data

[Vyacheslav Bocharov]

The mmc driver has the same phase values for all meson platforms. However,
some platforms (and even some boards) require different values. This patch
transfers the values from the set in the code to the variables in the
device-tree file.

Signed-off-by: Vyacheslav Bocharov <adeep@lexina.in>

 create mode 100644 include/dt-bindings/mmc/meson-gx-mmc.h

diff --git a/include/dt-bindings/mmc/meson-gx-mmc.h b/include/dt-bindings/mmc/meson-gx-mmc.h
new file mode 100644
index 000000000000..cfc4a9d75b2b
--- /dev/null
+++ b/include/dt-bindings/mmc/meson-gx-mmc.h
@@ -0,0 +1,35 @@
+/* SPDX-License-Identifier: (GPL-2.0+ or MIT) */
+/*
+ * Copyright (c) 2022 JetHome, Vyacheslav Bocharov
+ * Author: Vyacheslav Bocharov <adeep@lexina.in>
+ */
+
+#ifndef _DT_BINDINGS_MESON_GX_MMC_H
+#define _DT_BINDINGS_MESON_GX_MMC_H
+
+/*
+ * Cfg_rx_phase: RX clock phase
+ * bits: 9:8 R/W
+ * default: 0
+ * Recommended value: 0
+ *
+ * Cfg_tx_phase: TX clock phase
+ * bits: 9:8 R/W
+ * default: 0
+ * Recommended value: 2
+ *
+ * Cfg_co_phase: Core clock phase
+ * bits: 9:8 R/W
+ * default: 0
+ * Recommended value: 2
+ *
+ * values: 0: 0 phase, 1: 90 phase, 2: 180 phase, 3: 270 phase.
+ */
+
+#define   CLK_PHASE_0 0
+#define   CLK_PHASE_90 1
+#define   CLK_PHASE_180 2
+#define   CLK_PHASE_270 3
+
+
+#endif
-- 
2.30.2

Re: [PATCH 2/4] arm64: amlogic: mmc: meson-gx: Add dts binding include for core, tx, rx eMMC/SD/SDIO phase clock settings from devicetree data

[Martin Blumenstingl]

Hi Vyacheslav,

On Thu, Nov 10, 2022 at 4:02 PM Vyacheslav Bocharov <adeep@lexina.in> wrote:
[...]
> +/*
> + * Cfg_rx_phase: RX clock phase
> + * bits: 9:8 R/W
Generally register values should not be part of the dt-bindings.
If we need to make the phases configurable through device-tree then I
suggest using the human readable values (0, 90, 180, 270) instead of
these register bits.

That said, if for whatever reason we need to have #defines for this
then they should be added with the dt-bindings patch (and also carry
the dt-bindings subject prefix) instead of a separate patch.


Best regards,
Martin

[PATCH 3/4] arm64: amlogic: dts: meson: update meson-axg device-tree for new core, tx, rx phase clock settings.

[Vyacheslav Bocharov]

Use phase 270 for core MMC clock on axg meson boards.
Tested on JetHub J100/110 devices.

Signed-off-by: Vyacheslav Bocharov <adeep@lexina.in>

diff --git a/arch/arm64/boot/dts/amlogic/meson-axg.dtsi b/arch/arm64/boot/dts/amlogic/meson-axg.dtsi
index 04f797b5a012..0af4784d84c7 100644
--- a/arch/arm64/boot/dts/amlogic/meson-axg.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-axg.dtsi
@@ -13,6 +13,7 @@
 #include <dt-bindings/reset/amlogic,meson-axg-audio-arb.h>
 #include <dt-bindings/reset/amlogic,meson-axg-reset.h>
 #include <dt-bindings/power/meson-axg-power.h>
+#include <dt-bindings/mmc/meson-gx-mmc.h>
 
 / {
 	compatible = "amlogic,meson-axg";
@@ -1891,6 +1892,7 @@ sd_emmc_b: sd@5000 {
 					<&clkc CLKID_SD_EMMC_B_CLK0>,
 					<&clkc CLKID_FCLK_DIV2>;
 				clock-names = "core", "clkin0", "clkin1";
+				amlogic,mmc-phase = <CLK_PHASE_270 CLK_PHASE_0 CLK_PHASE_0>;
 				resets = <&reset RESET_SD_EMMC_B>;
 			};
 
@@ -1904,6 +1906,7 @@ sd_emmc_c: mmc@7000 {
 					<&clkc CLKID_FCLK_DIV2>;
 				clock-names = "core", "clkin0", "clkin1";
 				resets = <&reset RESET_SD_EMMC_C>;
+				amlogic,mmc-phase = <CLK_PHASE_270 CLK_PHASE_0 CLK_PHASE_0>;
 			};
 
 			usb2_phy1: phy@9020 {
-- 
2.30.2

[PATCH 4/4] arm64: dts: docs: Update mmc meson-gx documentation for new config option amlogic,mmc-phase

[Vyacheslav Bocharov]

- amlogic,mmc-phases: 3-element array of clock phases for core, tx, rx
clock with values:
	0: CLK_PHASE_0 - 0 phase
	1: CLK_PHASE_90 - 90 phase
	2: CLK_PHASE_180 - 180 phase
	3: CLK_PHASE_270 - 270 phase
By default driver use <CLK_PHASE_180 CLK_PHASE_0 CLK_PHASE_0> value.

Signed-off-by: Vyacheslav Bocharov <adeep@lexina.in>

diff --git a/Documentation/devicetree/bindings/mmc/amlogic,meson-gx.txt b/Documentation/devicetree/bindings/mmc/amlogic,meson-gx.txt
index ccc5358db131..98c89c5b3455 100644
--- a/Documentation/devicetree/bindings/mmc/amlogic,meson-gx.txt
+++ b/Documentation/devicetree/bindings/mmc/amlogic,meson-gx.txt
@@ -25,6 +25,12 @@ Required properties:
 Optional properties:
 - amlogic,dram-access-quirk: set when controller's internal DMA engine cannot access the
   DRAM memory, like on the G12A dedicated SDIO controller.
+- amlogic,mmc-phases: 3-element array of clock phases for core, tx, rx clock with values:
+	0: CLK_PHASE_0 - 0 phase
+	1: CLK_PHASE_90 - 90 phase
+	2: CLK_PHASE_180 - 180 phase
+	3: CLK_PHASE_270 - 270 phase
+  By default driver use <CLK_PHASE_180 CLK_PHASE_0 CLK_PHASE_0> value.
 
 Example:
 
@@ -36,4 +42,5 @@ Example:
 		clock-names = "core", "clkin0", "clkin1";
 		pinctrl-0 = <&emmc_pins>;
 		resets = <&reset RESET_SD_EMMC_A>;
+		amlogic,mmc-phases = <CLK_PHASE_180 CLK_PHASE_0 CLK_PHASE_0>;
 	};
-- 
2.30.2

Re: [PATCH 4/4] arm64: dts: docs: Update mmc meson-gx documentation for new config option amlogic,mmc-phase

[Martin Blumenstingl]

Hi Vyacheslav,

On Thu, Nov 10, 2022 at 4:01 PM Vyacheslav Bocharov <adeep@lexina.in> wrote:
[...]
> +- amlogic,mmc-phases: 3-element array of clock phases for core, tx, rx clock with values:
> +       0: CLK_PHASE_0 - 0 phase
> +       1: CLK_PHASE_90 - 90 phase
> +       2: CLK_PHASE_180 - 180 phase
> +       3: CLK_PHASE_270 - 270 phase
As mentioned in another patch: I'd go with the human readable values
(0, 90, 180, 270) instead of the register bits.

[...]
> +               amlogic,mmc-phases = <CLK_PHASE_180 CLK_PHASE_0 CLK_PHASE_0>;
Also I *think* the format here is not correct, for an array of three
u32 values this should be:
  amlogic,mmc-phases = <CLK_PHASE_180>, <CLK_PHASE_0>, <CLK_PHASE_0>;


Best regards,
Martin

Re: [PATCH 4/4] arm64: dts: docs: Update mmc meson-gx documentation for new config option amlogic,mmc-phase

[Krzysztof Kozlowski]

On 10/11/2022 16:00, Vyacheslav Bocharov wrote:
> - amlogic,mmc-phases: 3-element array of clock phases for core, tx, rx

Use subject prefixes matching the subsystem (git log --oneline -- ...).

> clock with values:
> 	0: CLK_PHASE_0 - 0 phase
> 	1: CLK_PHASE_90 - 90 phase
> 	2: CLK_PHASE_180 - 180 phase
> 	3: CLK_PHASE_270 - 270 phase
> By default driver use <CLK_PHASE_180 CLK_PHASE_0 CLK_PHASE_0> value.

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC.  It might happen, that command when run on an older
kernel, gives you outdated entries.  Therefore please be sure you base
your patches on recent Linux kernel.

You missed several people.

> 
> Signed-off-by: Vyacheslav Bocharov <adeep@lexina.in>
> 
> diff --git a/Documentation/devicetree/bindings/mmc/amlogic,meson-gx.txt b/Documentation/devicetree/bindings/mmc/amlogic,meson-gx.txt
> index ccc5358db131..98c89c5b3455 100644
> --- a/Documentation/devicetree/bindings/mmc/amlogic,meson-gx.txt
> +++ b/Documentation/devicetree/bindings/mmc/amlogic,meson-gx.txt
> @@ -25,6 +25,12 @@ Required properties:
>  Optional properties:
>  - amlogic,dram-access-quirk: set when controller's internal DMA engine cannot access the
>    DRAM memory, like on the G12A dedicated SDIO controller.
> +- amlogic,mmc-phases: 3-element array of clock phases for core, tx, rx clock with values:
> +	0: CLK_PHASE_0 - 0 phase
> +	1: CLK_PHASE_90 - 90 phase
> +	2: CLK_PHASE_180 - 180 phase
> +	3: CLK_PHASE_270 - 270 phase
> +  By default driver use <CLK_PHASE_180 CLK_PHASE_0 CLK_PHASE_0> value.

No, this has to be converted to DT schema first.

Best regards,
Krzysztof

Re: [PATCH 0/4] arm64: amlogic: mmc: meson-gx: Add core, tx, rx

[Jerome Brunet]

On Thu 10 Nov 2022 at 18:00, Vyacheslav Bocharov <adeep@lexina.in> wrote:

> The mmc driver use the same phase values (core - 180, tx/rx - 0) for all
> meson64 platforms. However, some platforms (and even some boards) require
> different values

Where does it stops ? Trying to solve the instabilities of this
IP/driver by tweaking the phase has proven to be dead-end.

Soon, you'll end up tweaking these settings depending on the on
particular version of the device because it ships with a different eMMC
manufacturer. Then comes multi sourcing, sdio modules, sdcards ...

> (axg for example use 270 degree for core clock).

Where ? Upstream linux does not

u-boot does something of the sort for sm1 and I'm not entirely sure this
appropriate either.

IMO, this setting has more to do with the mode the mmc device is
operating at - not the platform or board.

We had some discussions with the HW designers at AML and they recommended
to keep a phase shift of 180 between the Core and Tx. They also
recommended to leave Rx alone (actually, starting from the v3, the Rx
field has no effect. It is not even wired to actual HW)

Funnily, that is not what the vendor driver does. It also does A LOT of
extremely complex and 'debatable' things, which mostly mask how much the
driver is unstable.

With the upstream drivers, modes up to SDR50 and HS200 have been stable
lately. SDR104 and DDR modes (DDR52 or HS400) remains problematic.

Changing the settings further would require more discussion with AML.
Blindly poking these value until you get something stablish for 1
particular use case is a recipe for disaster.

> This patch
> transfers the values from the code to the variables in the device-tree files.
> If not set in dts, use old default values.

I think going that way is opening a big can of worms. 
I don't think this should be applied

>
> Vyacheslav Bocharov (4):
>   arm64: amlogic: mmc: meson-gx: Add core, tx, rx eMMC/SD/SDIO phase
>     clock settings from devicetree data
>   arm64: amlogic: mmc: meson-gx: Add dts binding include for core, tx,
>     rx eMMC/SD/SDIO phase clock settings from devicetree data
>   arm64: amlogic: dts: meson: update meson-axg device-tree for new core,
>     tx, rx phase clock settings.
>   arm64: dts: docs: Update mmc meson-gx documentation for new config
>     option amlogic,mmc-phase
>
>  .../bindings/mmc/amlogic,meson-gx.txt         |  7 ++++
>  arch/arm64/boot/dts/amlogic/meson-axg.dtsi    |  3 ++
>  drivers/mmc/host/meson-gx-mmc.c               | 18 +++++++---
>  include/dt-bindings/mmc/meson-gx-mmc.h        | 35 +++++++++++++++++++
>  4 files changed, 58 insertions(+), 5 deletions(-)
>  create mode 100644 include/dt-bindings/mmc/meson-gx-mmc.h

Re: [PATCH 0/4] arm64: amlogic: mmc: meson-gx: Add core, tx, rx

[Vyacheslav]

Hi!
Thanks for reply. Sorry for delay.


13.11.2022 23:06, Jerome Brunet wrote:
> 
> On Thu 10 Nov 2022 at 18:00, Vyacheslav Bocharov <adeep@lexina.in> wrote:
> 
>> The mmc driver use the same phase values (core - 180, tx/rx - 0) for all
>> meson64 platforms. However, some platforms (and even some boards) require
>> different values
> 
> Where does it stops ? Trying to solve the instabilities of this
> IP/driver by tweaking the phase has proven to be dead-end.

As a result, there is now a stalemate and various real-world operating 
system projects use patches to change clock phases.


> 
> Soon, you'll end up tweaking these settings depending on the on
> particular version of the device because it ships with a different eMMC
> manufacturer. Then comes multi sourcing, sdio modules, sdcards ...
> 
>> (axg for example use 270 degree for core clock).
> 
> Where ? Upstream linux does not

Armbian/Home Assistant OS use core phase 270 for axg/g12/sm1 boards 
(patches by Neil). On JetHub devices phase 270 is need with eMMC more 
than 16Gb size.

> 
> u-boot does something of the sort for sm1 and I'm not entirely sure this
> appropriate either.

U-boot in Armbian/HAOS use same phase 270 or/and force low speed mode 
for eMMC (limit clock freq).

> 
> IMO, this setting has more to do with the mode the mmc device is
> operating at - not the platform or board.
> 
> We had some discussions with the HW designers at AML and they recommended
> to keep a phase shift of 180 between the Core and Tx. They also
> recommended to leave Rx alone (actually, starting from the v3, the Rx
> field has no effect. It is not even wired to actual HW)

I do not have access to AML engineers :)
I can only test settings on several different boards. And it seems that 
the phase settings depend not only on the board layout, but also on the 
eMMC chip used. What to do about this (if not to use the magic of the 
driver from AML) other than providing the ability to change the value in 
devicetree for each board I can't think of yet.

> 
> Funnily, that is not what the vendor driver does. It also does A LOT of
> extremely complex and 'debatable' things, which mostly mask how much the
> driver is unstable.

As far as I understand they just go through all possible values until 
the first successful attempt to initialize the device.
What do you think of the idea to implement such a variant for the 
meson-gx driver?

> 
> With the upstream drivers, modes up to SDR50 and HS200 have been stable
> lately. SDR104 and DDR modes (DDR52 or HS400) remains problematic.

I have troubles with HS200, for example:
Card Type [CARD_TYPE: 0x57]
  HS200 Single Data Rate eMMC @200MHz 1.8VI/O
  HS Dual Data Rate eMMC @52MHz 1.8V or 3VI/O
  HS eMMC @52MHz - at rated device voltage(s)
  HS eMMC @26MHz - at rated device voltage(s)

> 
> Changing the settings further would require more discussion with AML.
> Blindly poking these value until you get something stablish for 1
> particular use case is a recipe for disaster.

I assumed the idea that the dts are edited by the maintainers or the 
board developers and will be able to choose the values themselves.


> 
>> This patch
>> transfers the values from the code to the variables in the device-tree files.
>> If not set in dts, use old default values.
> 
> I think going that way is opening a big can of worms.
> I don't think this should be applied
> 
>>
>> Vyacheslav Bocharov (4):
>>    arm64: amlogic: mmc: meson-gx: Add core, tx, rx eMMC/SD/SDIO phase
>>      clock settings from devicetree data
>>    arm64: amlogic: mmc: meson-gx: Add dts binding include for core, tx,
>>      rx eMMC/SD/SDIO phase clock settings from devicetree data
>>    arm64: amlogic: dts: meson: update meson-axg device-tree for new core,
>>      tx, rx phase clock settings.
>>    arm64: dts: docs: Update mmc meson-gx documentation for new config
>>      option amlogic,mmc-phase
>>
>>   .../bindings/mmc/amlogic,meson-gx.txt         |  7 ++++
>>   arch/arm64/boot/dts/amlogic/meson-axg.dtsi    |  3 ++
>>   drivers/mmc/host/meson-gx-mmc.c               | 18 +++++++---
>>   include/dt-bindings/mmc/meson-gx-mmc.h        | 35 +++++++++++++++++++
>>   4 files changed, 58 insertions(+), 5 deletions(-)
>>   create mode 100644 include/dt-bindings/mmc/meson-gx-mmc.h
> 
> 
> _______________________________________________
> linux-amlogic mailing list
> linux-amlogic@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-amlogic

-- 
Vyacheslav Bocharov

Re: [PATCH 0/4] arm64: amlogic: mmc: meson-gx: Add core, tx, rx

[Jerome Brunet]

On Thu 24 Nov 2022 at 09:22, Vyacheslav <adeep@lexina.in> wrote:

> Hi!
> Thanks for reply. Sorry for delay.
>
>
> 13.11.2022 23:06, Jerome Brunet wrote:
>> On Thu 10 Nov 2022 at 18:00, Vyacheslav Bocharov <adeep@lexina.in> wrote:
>> 
>>> The mmc driver use the same phase values (core - 180, tx/rx - 0) for all
>>> meson64 platforms. However, some platforms (and even some boards) require
>>> different values
>> Where does it stops ? Trying to solve the instabilities of this
>> IP/driver by tweaking the phase has proven to be dead-end.
>
> As a result, there is now a stalemate and various real-world operating
> system projects use patches to change clock phases.
>

The current setting has seen its fair share of "real world"
testing too, before being applied. 

It does need more work, sure. It does not make what is proposed here
appropriate.

>
>> Soon, you'll end up tweaking these settings depending on the on
>> particular version of the device because it ships with a different eMMC
>> manufacturer. Then comes multi sourcing, sdio modules, sdcards ...
>> 
>>> (axg for example use 270 degree for core clock).
>> Where ? Upstream linux does not
>
> Armbian/Home Assistant OS use core phase 270 for axg/g12/sm1 boards
> (patches by Neil). On JetHub devices phase 270 is need with eMMC more than
> 16Gb size.

Size has nothing to do with this. Few boards electing to do something
else does justify making this a DT config. It just shows the controller
still need work.

>
>> u-boot does something of the sort for sm1 and I'm not entirely sure this
>> appropriate either.
>
> U-boot in Armbian/HAOS use same phase 270 or/and force low speed mode for
> eMMC (limit clock freq).
>
>> IMO, this setting has more to do with the mode the mmc device is
>> operating at - not the platform or board.
>> We had some discussions with the HW designers at AML and they recommended
>> to keep a phase shift of 180 between the Core and Tx. They also
>> recommended to leave Rx alone (actually, starting from the v3, the Rx
>> field has no effect. It is not even wired to actual HW)
>
> I do not have access to AML engineers :)
> I can only test settings on several different boards. And it seems that the
> phase settings depend not only on the board layout, but also on the eMMC
> chip used.

What are you going to do when a manufacturer does multi-sourcing then
? Make one DT per PCB/eMMC chip combination ? It is wrong and does not scale.

> What to do about this (if not to use the magic of the driver
> from AML) other than providing the ability to change the value in
> devicetree for each board I can't think of yet.
>
>> Funnily, that is not what the vendor driver does. It also does A LOT of
>> extremely complex and 'debatable' things, which mostly mask how much the
>> driver is unstable.
>
> As far as I understand they just go through all possible values until the
> first successful attempt to initialize the device.
> What do you think of the idea to implement such a variant for the meson-gx
> driver?

What the amlogic driver does are overly complex computation to get a tuning
value, which is then contiously cycled (in the IRQ handler!!) while silently
retrying failed transaction behind MMC core's back

That's far from being desirable.

>
>> With the upstream drivers, modes up to SDR50 and HS200 have been stable
>> lately. SDR104 and DDR modes (DDR52 or HS400) remains problematic.
>
> I have troubles with HS200, for example:
> Card Type [CARD_TYPE: 0x57]
>  HS200 Single Data Rate eMMC @200MHz 1.8VI/O
>  HS Dual Data Rate eMMC @52MHz 1.8V or 3VI/O
>  HS eMMC @52MHz - at rated device voltage(s)
>  HS eMMC @26MHz - at rated device voltage(s)
>

That does not says with which mode or at which stage the problem occurs

>> Changing the settings further would require more discussion with AML.
>> Blindly poking these value until you get something stablish for 1
>> particular use case is a recipe for disaster.
>
> I assumed the idea that the dts are edited by the maintainers or the board
> developers and will be able to choose the values themselves.
>

And eventually, we'll end-up telling people to adjust the phases
depending of the sdcard they insert ... This does not work for me.

I understand the will to get this working at full speed. I've spent A
LOT more time than would have wanted in this driver, trying to do
exactly that. There is quite an history about that on this list
detailing why changes have been made.

It was stable(ish) for while. Now we are getting more reports of
problems. This (again) shows this need more work. It also shows there
are still things we don't know about this controller and this where it
gets tricky because there is high risk of causing regressions with each
change.

Let's leave Rx (which has no effect) and Tx out for now.

My guess is the core phase might need to be adapted depending on
* Mode: It seems the DDR modes are done by making the controller run
  faster then diving the output clock by 2. This divider might
  mess up the phase shift needed.
* Speed: Maybe there is delay contraint between the input clock and the
  core and we need at adapting the phase shift depending on the rate ?

I think what could be tried - with a LOOONG RFT giving a chance people
to report regressions - is:
* Defaulting the Core phase to 270: Despite AML HW engineer
  recommendation, this is what the vendor code does. Maybe this will help
  with board that seems to require 270 to start.
  I know if stays for all modes, it will cause problems
1) Set 180 when switching to DDR modes
2) Wether switching to 180 for high speed SDR modes (UHS, HS200) is
   required, or not, is a bit unclear. If the problem is the internal
   divider, it should not be required. If the problem is the delay,
   maybe it is.

>
>> 
>>> This patch
>>> transfers the values from the code to the variables in the device-tree files.
>>> If not set in dts, use old default values.
>> I think going that way is opening a big can of worms.
>> I don't think this should be applied
>> 
>>>
>>> Vyacheslav Bocharov (4):
>>>    arm64: amlogic: mmc: meson-gx: Add core, tx, rx eMMC/SD/SDIO phase
>>>      clock settings from devicetree data
>>>    arm64: amlogic: mmc: meson-gx: Add dts binding include for core, tx,
>>>      rx eMMC/SD/SDIO phase clock settings from devicetree data
>>>    arm64: amlogic: dts: meson: update meson-axg device-tree for new core,
>>>      tx, rx phase clock settings.
>>>    arm64: dts: docs: Update mmc meson-gx documentation for new config
>>>      option amlogic,mmc-phase
>>>
>>>   .../bindings/mmc/amlogic,meson-gx.txt         |  7 ++++
>>>   arch/arm64/boot/dts/amlogic/meson-axg.dtsi    |  3 ++
>>>   drivers/mmc/host/meson-gx-mmc.c               | 18 +++++++---
>>>   include/dt-bindings/mmc/meson-gx-mmc.h        | 35 +++++++++++++++++++
>>>   4 files changed, 58 insertions(+), 5 deletions(-)
>>>   create mode 100644 include/dt-bindings/mmc/meson-gx-mmc.h
>> _______________________________________________
>> linux-amlogic mailing list
>> linux-amlogic@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-amlogic