Message-ID: <736d0164-b672-4c3b-ad64-89c31482effb@collabora.com>
Date: Thu, 3 Apr 2025 12:57:31 +0200
From: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
To: Axe Yang (杨磊) <Axe.Yang@...iatek.com>,
 Wenbin Mei (梅文彬) <Wenbin.Mei@...iatek.com>,
 "conor+dt@...nel.org" <conor+dt@...nel.org>,
 Chaotian Jing (井朝天)
 <Chaotian.Jing@...iatek.com>, "robh@...nel.org" <robh@...nel.org>,
 "matthias.bgg@...il.com" <matthias.bgg@...il.com>,
 "ulf.hansson@...aro.org" <ulf.hansson@...aro.org>,
 "krzk+dt@...nel.org" <krzk+dt@...nel.org>
Cc: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 "linux-mediatek@...ts.infradead.org" <linux-mediatek@...ts.infradead.org>,
 "linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
 Andy-ld Lu (卢东) <Andy-ld.Lu@...iatek.com>,
 "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
 Yong Mao (毛勇) <yong.mao@...iatek.com>,
 "linux-arm-kernel@...ts.infradead.org"
 <linux-arm-kernel@...ts.infradead.org>,
 Qingliang Li (黎晴亮) <Qingliang.Li@...iatek.com>
Subject: Re: [PATCH 1/2] dt-bindings: mmc: mtk-sd: add single burst switch

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@...iatek.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:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>
>>>>>
>>
>>

Powered by blists - more mailing lists