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