Re: [PATCH 1/2] dt-bindings: mmc: mtk-sd: add single burst switch

Il 25/03/25 03:41, Axe Yang (杨磊) ha scritto:

Hi Angelo,

Any comment on this :D

Check inline reply below....

Regards,
Axe

On Wed, 2025-03-12 at 14:30 +0800, axe.yang wrote:

quoted

On Tue, 2025-03-11 at 10:47 +0100, AngeloGioacchino Del Regno wrote:

quoted

External email : Please do not click links or open attachments
until
you have verified the sender or the content.


Il 07/03/25 07:59, Axe Yang (杨磊) ha scritto:

quoted

On Thu, 2025-03-06 at 10:19 +0100, AngeloGioacchino Del Regno
wrote:

quoted

External email : Please do not click links or open attachments
until
you have verified the sender or the content.


Il 06/03/25 09:48, Axe Yang ha scritto:

quoted

Add 'mediatek,disable-single-burst' setting. This property
can
be
used to switch bus burst type, from single burst to INCR,
which
is
determined by the bus type within the IP. Some versions of
the
IP
are using AXI bus, thus this switch is necessary as 'single'
is
not
the burst type supported by the bus.

Signed-off-by: Axe Yang <redacted>

I am mostly sure that this is not something to put in
devicetree,
but
as
platform data for specific SoC(s), as much as I'm mostly sure
that
all of
the instances of the MSDC IP in one SoC will be *all* using
either
single
or INCR.

No, actually MSDC IPs in one SoC are using different versions.
Usually MSDC1 (index from 1) is used as eMMC host, the left hosts
are
used as SD/SDIO hosts. They have similar designs, but there are
still
difference.

quoted

So, I think I know the answer but I'll still ask just to be
extremely
sure:

is there any MediaTek SoC that has different IP versions for
different MSDC
instances, and that hence require single burst on one instance
and
INCR on
another instance?

Yes. Actually every SoC has different IP versions for eMMC and
SD/SDIO
host as I said.
e.g. For MT8168, signel burst bit should be set to 1 for eMMC
Host,
but
0 for SD/SDIO Host.

quoted

And if there is - is there a pattern? Is it always SDIO
requiring
INCR or
always eMMC/SD requiring it?

No, there is no pattern. Both eMMC and SD/SDIO hosts need to be
configured base on IP version. There is no binding relationship
between
eMMC/SD/SDIO and the burst type. eMMC burst type might be INCR or
single, same as SD/SDIO.

Okay but if there are different IP versions, and AXI/AHB is
determined
by the IP version, why aren't you parsing the MAIN_VER/ECO_VER
registers of
the MSDC IP to check whether to use INCR or SINGLE?

To address your concerns, I had further discussions with the
designer.
Their response was that the bus type and IP version are not bound
together. This contradicts my previous statements, and I apologize
for
that.
According to the designer's feedback, I must say that the single
burst
setting is indeed tied to the IC, but the granularity is such that it
needs to be set individually for each host.
Given the large number of ICs Mediatek currently has, adding burst
type
information for each host to the driver's compatible structure would
mean adding hundreds(maybe thousands :() of lines to the driver for
the
compatible structures for *all previous SoCs* (currently there are
only
13 compatible structures, and they can be reuse by new SoC). This
approach seems very cumbersome.

So I still believe that placing this setting in the DTS is a more
appropriate approach.

Hello Axe,

sorry for the wait - this email fell through the cracks and I didn't see
it at all, so thank you for the ping.

Unfortunately, I don't think that this would be acceptable from a devicetree and/or
bindings standpoint, but then you don't really need to modify the pdata for all of
the currently supported SoCs to declare false, as false==0, which is the default.

But maybe there's another way out of this.

You said that this modification is done because some controllers are under AXI and
some others are under AHB... I was doing some cleanups to this driver and doing so
made me check a couple of things....

When a MSDC controller is under AXI, there will be configuration for that in other
registers - specifically, I'm wondering if the EMMC50_CFG2 register can be used to
check if we are under an AHB to AXI wrapper or not.

The idea is to read this register (offset 0x21c), [27:24] AXI_SET_LEN contains the
number of beats per burst (from 1 to 16), and also [23:19] AXI_RX_OUTSTANDING_NUM
contains the number of outstanding transfers (1 to 13).

If a controller does not have an AXI2AHB Wrapper, or if it does not use the AXI bus
this register should read zero I think?

Especially the two fields that I mentioned before, those should read zero.

That, especially because the hwaddr for the controllers is anyway and always long
0x1000 - and I think that the extra registers space, on controllers that don't have
the EMMC50 registers (msdc1 and msdc2) should be still reserved to those and never
used for anything else.

Would that detection way work?

If it would, we'd be again autodetecting whether to set or not the AXI single burst
option in the patch bits...without relying on specifying anything manually, not in
the devicetree, and not in the platform data :-)

Cheers,
Angelo

quoted

Regards,
Axe

quoted

Cheers,
Angelo

quoted

Regards,
Axe

quoted

quoted

---
    Documentation/devicetree/bindings/mmc/mtk-sd.yaml | 8
++++++++
    1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/mtk-

sd.yaml
b/Documentation/devicetree/bindings/mmc/mtk-sd.yaml
index 0debccbd6519..6076aff0a689 100644

--- a/Documentation/devicetree/bindings/mmc/mtk-sd.yaml
+++ b/Documentation/devicetree/bindings/mmc/mtk-sd.yaml

@@ -100,6 +100,14 @@ properties:
        minimum: 0
        maximum: 0xffffffff

+  mediatek,disable-single-burst:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description:
+      Burst type setting. For some versions of the IP that

do
not
use
+      AHB bus, the burst type need to be switched to INCR.
+      If present, use INCR burst type.
+      If not present, use single burst type.
+
      mediatek,hs200-cmd-int-delay:
        $ref: /schemas/types.yaml#/definitions/uint32
        description: