[PATCH 0/2] mmc: mtk-sd: add support for burst type switch

[PATCH 0/2] mmc: mtk-sd: add support for burst type switch

[Axe Yang]

Add a device tree property for MSDC node to switch burst type.
The default burst type is 'single', but for some versions of IP,
the AMBA type is AXI, which do not support 'single' burst type.

This series of changes provides a way to switch the burst type,
and is necessary as the AXI bus is being used more frequently
in the new versions of MSDC IP.

Axe Yang (2):
  dt-bindings: mmc: mtk-sd: add single burst switch
  mmc: mtk-sd: add support to disable single burst

 Documentation/devicetree/bindings/mmc/mtk-sd.yaml |  8 ++++++++
 drivers/mmc/host/mtk-sd.c                         | 10 ++++++++++
 2 files changed, 18 insertions(+)

-- 
2.46.0

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

[Axe Yang]

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 <axe.yang@mediatek.com>
---
 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:
-- 
2.46.0

[PATCH 2/2] mmc: mtk-sd: add support to disable single burst

[Axe Yang]

Add support to disable 'single' burst type if the bus type is AXI.
Since the AMBA within some of the legacy and new designed MSDC IP
is AXI, this switch is necessary.

The burst type is not IC-specific, but host-specific. So we use a
devicetree property 'mediatek,disable-single-burst' to switch burst
type for specific MSDC host.

Signed-off-by: Axe Yang <axe.yang@mediatek.com>
---
 drivers/mmc/host/mtk-sd.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
index 345ea91629e0..ed46c69def1e 100644
--- a/drivers/mmc/host/mtk-sd.c
+++ b/drivers/mmc/host/mtk-sd.c
@@ -249,6 +249,7 @@
 #define MSDC_PATCH_BIT1_CMDTA     GENMASK(5, 3)    /* RW */
 #define MSDC_PB1_BUSY_CHECK_SEL   BIT(7)    /* RW */
 #define MSDC_PATCH_BIT1_STOP_DLY  GENMASK(11, 8)    /* RW */
+#define MSDC_PB1_SINGLE_BURST     BIT(16)   /* RW */
 
 #define MSDC_PATCH_BIT2_CFGRESP   BIT(15)   /* RW */
 #define MSDC_PATCH_BIT2_CFGCRCSTS BIT(28)   /* RW */
@@ -485,6 +486,7 @@ struct msdc_host {
 	u32 src_clk_freq;	/* source clock frequency */
 	unsigned char timing;
 	bool vqmmc_enabled;
+	bool disable_single_burst;
 	u32 latch_ck;
 	u32 hs400_ds_delay;
 	u32 hs400_ds_dly3;
@@ -1874,6 +1876,10 @@ static void msdc_init_hw(struct msdc_host *host)
 	writel(0xffff4089, host->base + MSDC_PATCH_BIT1);
 	sdr_set_bits(host->base + EMMC50_CFG0, EMMC50_CFG_CFCSTS_SEL);
 
+	if (host->disable_single_burst)
+		sdr_clr_bits(host->base + MSDC_PATCH_BIT1,
+			     MSDC_PB1_SINGLE_BURST);
+
 	if (host->dev_comp->stop_clk_fix) {
 		if (host->dev_comp->stop_dly_sel)
 			sdr_set_field(host->base + MSDC_PATCH_BIT1,
@@ -2820,6 +2826,10 @@ static void msdc_of_property_parse(struct platform_device *pdev,
 		host->cqhci = true;
 	else
 		host->cqhci = false;
+
+	host->disable_single_burst =
+		of_property_read_bool(pdev->dev.of_node,
+				      "mediatek,disable-single-burst");
 }
 
 static int msdc_of_clock_parse(struct platform_device *pdev,
-- 
2.46.0

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

[AngeloGioacchino Del Regno]

Il 06/03/25 09:48, Axe Yang ha scritto:
> 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 <axe.yang@mediatek.com>

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.

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?

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

Regards,
Angelo

> ---
>   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:

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

[Axe Yang (杨磊)]

On Thu, 2025-03-06 at 10:19 +0100, AngeloGioacchino Del Regno wrote:
> 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:
> > 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 <axe.yang@mediatek.com>
> 
> 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.

> 
> 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.

> 
> 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.


Regards,
Axe


> 
> > ---
> >   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:
> 
> 
> 

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

[Conor Dooley]

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

On Fri, Mar 07, 2025 at 06:59:03AM +0000, Axe Yang (杨磊) wrote:
> On Thu, 2025-03-06 at 10:19 +0100, AngeloGioacchino Del Regno wrote:
> > 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:
> > > 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 <axe.yang@mediatek.com>
> > 
> > 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.
> 
> > 
> > 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.
> 

Sounds like two different IPs that really should have different
compatibles to me...

> > 
> > 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.
> 
> 
> Regards,
> Axe
> 
> 
> > 
> > > ---
> > >   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:
> > 
> > 
> > 

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

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

[Axe Yang (杨磊)]

On Fri, 2025-03-07 at 15:48 +0000, Conor Dooley wrote:
> On Fri, Mar 07, 2025 at 06:59:03AM +0000, Axe Yang (杨磊) wrote:
> > On Thu, 2025-03-06 at 10:19 +0100, AngeloGioacchino Del Regno
> > wrote:
> > > 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:
> > > > 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 <axe.yang@mediatek.com>
> > > 
> > > 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.
> > 
> > > 
> > > 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.
> > 
> 
> Sounds like two different IPs that really should have different
> compatibles to me...

Yes, so considering all factors, adding a device tree property might be
the most appropriate approach. This method is more flexible and
convenient.
Another reason, which I didn't mention in the previous reply, is that
MediaTek has too many SoCs using MSDC. Currently, there are already 13
mtXXXX compatible structures in the msdc driver, and a significant
portion of the SoC settings can be reused by the new SoC (without
adding a compatible for new SoC). 
If we add a variable in the compatible to represent the single burst
setting for eMMC/SD/SDIO (maybe using a u8 type variable as bitmap for
different hosts, bit0 for msdc0, bit1 for msdc1, bit2 for msdc2, ...),
we might have to add a compatible for each Soc, leading to a
significant increase in the number of compatibles and making it more
complex.

Regards,
Axe

> > > 
> > > 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.
> > 
> > 
> > Regards,
> > Axe
> > 
> > 
> > > 
> > > > ---
> > > >   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:
> > > 
> > > 
> > > 

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

[AngeloGioacchino Del Regno]

Il 07/03/25 07:59, Axe Yang (杨磊) ha scritto:
> On Thu, 2025-03-06 at 10:19 +0100, AngeloGioacchino Del Regno wrote:
>> 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:
>>> 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 <axe.yang@mediatek.com>
>>
>> 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.
> 
>>
>> 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.
> 
>>
>> 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?

Cheers,
Angelo

> 
> Regards,
> Axe
> 
> 
>>
>>> ---
>>>    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:
>>
>>
>>

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

[Axe Yang (杨磊)]

On Tue, 2025-03-11 at 10:47 +0100, AngeloGioacchino Del Regno wrote:
> 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:
> > On Thu, 2025-03-06 at 10:19 +0100, AngeloGioacchino Del Regno
> > wrote:
> > > 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:
> > > > 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 <axe.yang@mediatek.com>
> > > 
> > > 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.
> > 
> > > 
> > > 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.
> > 
> > > 
> > > 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.

Regards,
Axe

> 
> Cheers,
> Angelo
> 
> > 
> > Regards,
> > Axe
> > 
> > 
> > > 
> > > > ---
> > > >    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:
> > > 
> > > 
> > > 
> 
> 

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

[Axe Yang (杨磊)]

Hi Angelo,

Any comment on this :D 

Regards,
Axe

On Wed, 2025-03-12 at 14:30 +0800, axe.yang wrote:
> On Tue, 2025-03-11 at 10:47 +0100, AngeloGioacchino Del Regno wrote:
> > 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:
> > > On Thu, 2025-03-06 at 10:19 +0100, AngeloGioacchino Del Regno
> > > wrote:
> > > > 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:
> > > > > 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 <axe.yang@mediatek.com>
> > > > 
> > > > 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.
> > > 
> > > > 
> > > > 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.
> > > 
> > > > 
> > > > 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.
> 
> Regards,
> Axe
> 
> > 
> > Cheers,
> > Angelo
> > 
> > > 
> > > Regards,
> > > Axe
> > > 
> > > 
> > > > 
> > > > > ---
> > > > >    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:
> > > > 
> > > > 
> > > > 
> > 
> > 

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

[AngeloGioacchino Del Regno]

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:
>> On Tue, 2025-03-11 at 10:47 +0100, AngeloGioacchino Del Regno wrote:
>>> 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:
>>>> On Thu, 2025-03-06 at 10:19 +0100, AngeloGioacchino Del Regno
>>>> wrote:
>>>>> 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:
>>>>>> 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 <axe.yang@mediatek.com>
>>>>>
>>>>> 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.
>>>>
>>>>>
>>>>> 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.
>>>>
>>>>>
>>>>> 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

>> Regards,
>> Axe
>>
>>>
>>> Cheers,
>>> Angelo
>>>
>>>>
>>>> Regards,
>>>> Axe
>>>>
>>>>
>>>>>
>>>>>> ---
>>>>>>     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:
>>>>>
>>>>>
>>>>>
>>>
>>>

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

[Axe Yang (杨磊)]

On Tue, 2025-03-25 at 11:20 +0100, AngeloGioacchino Del Regno wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> 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:
> > > On Tue, 2025-03-11 at 10:47 +0100, AngeloGioacchino Del Regno
> > > wrote:
> > > > 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:
> > > > > On Thu, 2025-03-06 at 10:19 +0100, AngeloGioacchino Del Regno
> > > > > wrote:
> > > > > > 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:
> > > > > > > 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 <axe.yang@mediatek.com>
> > > > > > 
> > > > > > 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.
> > > > > 
> > > > > > 
> > > > > > 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.
> > > > > 
> > > > > > 
> > > > > > 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?

Confirmed that this approach will work for all Soc and IP version. Thx.

Will send v2 after your register cleanup series accepted.

Regards,
Axe

> 
> 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
> 
> > > Regards,
> > > Axe
> > > 
> > > > 
> > > > Cheers,
> > > > Angelo
> > > > 
> > > > > 
> > > > > Regards,
> > > > > Axe
> > > > > 
> > > > > 
> > > > > > 
> > > > > > > ---
> > > > > > >     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:
> > > > > > 
> > > > > > 
> > > > > > 
> > > > 
> > > > 
> 
> 

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

[AngeloGioacchino Del Regno]

Il 27/03/25 03:48, Axe Yang (杨磊) ha scritto:
> On Tue, 2025-03-25 at 11:20 +0100, AngeloGioacchino Del Regno wrote:
>> External email : Please do not click links or open attachments until
>> you have verified the sender or the content.
>>
>>
>> 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:
>>>> On Tue, 2025-03-11 at 10:47 +0100, AngeloGioacchino Del Regno
>>>> wrote:
>>>>> 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:
>>>>>> On Thu, 2025-03-06 at 10:19 +0100, AngeloGioacchino Del Regno
>>>>>> wrote:
>>>>>>> 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:
>>>>>>>> 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 <axe.yang@mediatek.com>
>>>>>>>
>>>>>>> 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.
>>>>>>
>>>>>>>
>>>>>>> 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.
>>>>>>
>>>>>>>
>>>>>>> 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?
> 
> Confirmed that this approach will work for all Soc and IP version. Thx.
> 

That's great to hear.

Makes things much simpler and at this point that will even fix some other
MediaTek SoCs at this point ;-)


> Will send v2 after your register cleanup series accepted.

Please feel free to send your v2 even right now, just add a note (not in the
commit description) saying that your patch is based on top of my series.

That will be fine.

Also, if you could provide a review on my series, that would help speeding up
things :-)

Thanks,
Angelo

> 
> Regards,
> Axe
> 
>>
>> 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
>>
>>>> Regards,
>>>> Axe
>>>>
>>>>>
>>>>> Cheers,
>>>>> Angelo
>>>>>
>>>>>>
>>>>>> Regards,
>>>>>> Axe
>>>>>>
>>>>>>
>>>>>>>
>>>>>>>> ---
>>>>>>>>      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:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>
>>>>>
>>
>>