[PATCH v1 0/3] Change tuning implementation

[PATCH v1 0/3] Change tuning implementation

[William Qiu]

Hi,

This series of patches changes the tuning implementation, from the
previous way of reading and writing system controller registers to
reading and writing UHS_REG_EXT register, thus optimizing the tuning
of obtaining delay-chain.

The patch series is based on master-next.

William Qiu (3):
  dt-bindings: mmc: Drop unused properties
  mmc: starfive: Change tuning implementation
  riscv: dts: starfive: Drop unused properties and limit frquency

 .../bindings/mmc/starfive,jh7110-mmc.yaml     |  15 --
 .../jh7110-starfive-visionfive-2.dtsi         |   4 +
 arch/riscv/boot/dts/starfive/jh7110.dtsi      |   2 -
 drivers/mmc/host/dw_mmc-starfive.c            | 131 +++++-------------
 4 files changed, 41 insertions(+), 111 deletions(-)

--
2.34.1

[PATCH v1 1/3] dt-bindings: mmc: Drop unused properties

[William Qiu]

Due to the change of tuning implementation, it's no longer necessary to
use the "starfive,sysreg" property in dts, so drop the relevant
description in dt-bindings here.

Signed-off-by: William Qiu <william.qiu@starfivetech.com>
---
 .../bindings/mmc/starfive,jh7110-mmc.yaml         | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/Documentation/devicetree/bindings/mmc/starfive,jh7110-mmc.yaml b/Documentation/devicetree/bindings/mmc/starfive,jh7110-mmc.yaml
index 51e1b04e799f..10df41941331 100644
--- a/Documentation/devicetree/bindings/mmc/starfive,jh7110-mmc.yaml
+++ b/Documentation/devicetree/bindings/mmc/starfive,jh7110-mmc.yaml
@@ -36,26 +36,12 @@ properties:
   interrupts:
     maxItems: 1
 
-  starfive,sysreg:
-    $ref: /schemas/types.yaml#/definitions/phandle-array
-    items:
-      - items:
-          - description: phandle to System Register Controller syscon node
-          - description: offset of SYS_SYSCONSAIF__SYSCFG register for MMC controller
-          - description: shift of SYS_SYSCONSAIF__SYSCFG register for MMC controller
-          - description: mask of SYS_SYSCONSAIF__SYSCFG register for MMC controller
-    description:
-      Should be four parameters, the phandle to System Register Controller
-      syscon node and the offset/shift/mask of SYS_SYSCONSAIF__SYSCFG register
-      for MMC controller.
-
 required:
   - compatible
   - reg
   - clocks
   - clock-names
   - interrupts
-  - starfive,sysreg
 
 unevaluatedProperties: false
 
@@ -73,5 +59,4 @@ examples:
         fifo-depth = <32>;
         fifo-watermark-aligned;
         data-addr = <0>;
-        starfive,sysreg = <&sys_syscon 0x14 0x1a 0x7c000000>;
     };
-- 
2.34.1

Re: [PATCH v1 1/3] dt-bindings: mmc: Drop unused properties

[Conor Dooley]

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

On Wed, Aug 30, 2023 at 11:18:44AM +0800, William Qiu wrote:
> Due to the change of tuning implementation, it's no longer necessary to
> use the "starfive,sysreg" property in dts, so drop the relevant
> description in dt-bindings here.

How does changing your software implantation invalidate a description of
the hardware?

> 
> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
> ---
>  .../bindings/mmc/starfive,jh7110-mmc.yaml         | 15 ---------------
>  1 file changed, 15 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/starfive,jh7110-mmc.yaml b/Documentation/devicetree/bindings/mmc/starfive,jh7110-mmc.yaml
> index 51e1b04e799f..10df41941331 100644
> --- a/Documentation/devicetree/bindings/mmc/starfive,jh7110-mmc.yaml
> +++ b/Documentation/devicetree/bindings/mmc/starfive,jh7110-mmc.yaml
> @@ -36,26 +36,12 @@ properties:
>    interrupts:
>      maxItems: 1
>  
> -  starfive,sysreg:
> -    $ref: /schemas/types.yaml#/definitions/phandle-array
> -    items:
> -      - items:
> -          - description: phandle to System Register Controller syscon node
> -          - description: offset of SYS_SYSCONSAIF__SYSCFG register for MMC controller
> -          - description: shift of SYS_SYSCONSAIF__SYSCFG register for MMC controller
> -          - description: mask of SYS_SYSCONSAIF__SYSCFG register for MMC controller
> -    description:
> -      Should be four parameters, the phandle to System Register Controller
> -      syscon node and the offset/shift/mask of SYS_SYSCONSAIF__SYSCFG register
> -      for MMC controller.
> -
>  required:
>    - compatible
>    - reg
>    - clocks
>    - clock-names
>    - interrupts
> -  - starfive,sysreg
>  
>  unevaluatedProperties: false
>  
> @@ -73,5 +59,4 @@ examples:
>          fifo-depth = <32>;
>          fifo-watermark-aligned;
>          data-addr = <0>;
> -        starfive,sysreg = <&sys_syscon 0x14 0x1a 0x7c000000>;
>      };
> -- 
> 2.34.1
> 

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

Re: [PATCH v1 1/3] dt-bindings: mmc: Drop unused properties

[Krzysztof Kozlowski]

On 30/08/2023 08:50, Conor Dooley wrote:
> On Wed, Aug 30, 2023 at 11:18:44AM +0800, William Qiu wrote:
>> Due to the change of tuning implementation, it's no longer necessary to
>> use the "starfive,sysreg" property in dts, so drop the relevant
>> description in dt-bindings here.
> 
> How does changing your software implantation invalidate a description of
> the hardware?
> 

Which is kind of proof that this syscon was just to substitute
incomplete hardware description (e.g. missing clocks and phys). We
should have rejected it. Just like we should reject them in the future.

There are just few cases where syscon is reasonable. All others is just
laziness. It's not only starfivetech, of course. Several other
contributors do the same.

Best regards,
Krzysztof

Re: [PATCH v1 1/3] dt-bindings: mmc: Drop unused properties

[Conor Dooley]

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

On Wed, Aug 30, 2023 at 09:29:20AM +0200, Krzysztof Kozlowski wrote:
> On 30/08/2023 08:50, Conor Dooley wrote:
> > On Wed, Aug 30, 2023 at 11:18:44AM +0800, William Qiu wrote:
> >> Due to the change of tuning implementation, it's no longer necessary to
> >> use the "starfive,sysreg" property in dts, so drop the relevant
> >> description in dt-bindings here.
> > 
> > How does changing your software implantation invalidate a description of
> > the hardware?
> > 
> 
> Which is kind of proof that this syscon was just to substitute
> incomplete hardware description (e.g. missing clocks and phys). We
> should have rejected it. Just like we should reject them in the future.

:s I dunno what to do with this... I'm inclined to say not to remove it
from the binding or dts at all & only change the software.

> There are just few cases where syscon is reasonable. All others is just
> laziness. It's not only starfivetech, of course. Several other
> contributors do the same.

I'm not sure if laziness is fair, lack of understanding is usually more
likely.

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

Re: [PATCH v1 1/3] dt-bindings: mmc: Drop unused properties

[William Qiu]

On 2023/8/30 16:34, Conor Dooley wrote:
> On Wed, Aug 30, 2023 at 09:29:20AM +0200, Krzysztof Kozlowski wrote:
>> On 30/08/2023 08:50, Conor Dooley wrote:
>> > On Wed, Aug 30, 2023 at 11:18:44AM +0800, William Qiu wrote:
>> >> Due to the change of tuning implementation, it's no longer necessary to
>> >> use the "starfive,sysreg" property in dts, so drop the relevant
>> >> description in dt-bindings here.
>> > 
>> > How does changing your software implantation invalidate a description of
>> > the hardware?
>> > 
>> 
>> Which is kind of proof that this syscon was just to substitute
>> incomplete hardware description (e.g. missing clocks and phys). We
>> should have rejected it. Just like we should reject them in the future.
> 
> :s I dunno what to do with this... I'm inclined to say not to remove it
> from the binding or dts at all & only change the software.
> 
>> There are just few cases where syscon is reasonable. All others is just
>> laziness. It's not only starfivetech, of course. Several other
>> contributors do the same.
> 
> I'm not sure if laziness is fair, lack of understanding is usually more
> likely.

For this, I tend to keep it in binding, but remove it from required. Because
we only modify the tuning implementation, it doesn't mean that this property
need to be removed, it's just no longer be the required one.

Best Regards,
William

Re: [PATCH v1 1/3] dt-bindings: mmc: Drop unused properties

[Conor Dooley]

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

On Fri, Sep 01, 2023 at 10:33:13AM +0800, William Qiu wrote:
> 
> 
> On 2023/8/30 16:34, Conor Dooley wrote:
> > On Wed, Aug 30, 2023 at 09:29:20AM +0200, Krzysztof Kozlowski wrote:
> >> On 30/08/2023 08:50, Conor Dooley wrote:
> >> > On Wed, Aug 30, 2023 at 11:18:44AM +0800, William Qiu wrote:
> >> >> Due to the change of tuning implementation, it's no longer necessary to
> >> >> use the "starfive,sysreg" property in dts, so drop the relevant
> >> >> description in dt-bindings here.
> >> > 
> >> > How does changing your software implantation invalidate a description of
> >> > the hardware?
> >> > 
> >> 
> >> Which is kind of proof that this syscon was just to substitute
> >> incomplete hardware description (e.g. missing clocks and phys). We
> >> should have rejected it. Just like we should reject them in the future.
> > 
> > :s I dunno what to do with this... I'm inclined to say not to remove it
> > from the binding or dts at all & only change the software.
> > 
> >> There are just few cases where syscon is reasonable. All others is just
> >> laziness. It's not only starfivetech, of course. Several other
> >> contributors do the same.
> > 
> > I'm not sure if laziness is fair, lack of understanding is usually more
> > likely.
> 
> For this, I tend to keep it in binding, but remove it from required. Because
> we only modify the tuning implementation, it doesn't mean that this property
> need to be removed, it's just no longer be the required one.

Please only remove it from required if the current driver doesn't break
if the regmap is removed.

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

Re: [PATCH v1 1/3] dt-bindings: mmc: Drop unused properties

[Jessica Clarke]

On 1 Sep 2023, at 16:42, Conor Dooley <conor@kernel.org> wrote:
> 
> On Fri, Sep 01, 2023 at 10:33:13AM +0800, William Qiu wrote:
>> 
>> 
>> On 2023/8/30 16:34, Conor Dooley wrote:
>>> On Wed, Aug 30, 2023 at 09:29:20AM +0200, Krzysztof Kozlowski wrote:
>>>> On 30/08/2023 08:50, Conor Dooley wrote:
>>>>> On Wed, Aug 30, 2023 at 11:18:44AM +0800, William Qiu wrote:
>>>>>> Due to the change of tuning implementation, it's no longer necessary to
>>>>>> use the "starfive,sysreg" property in dts, so drop the relevant
>>>>>> description in dt-bindings here.
>>>>> 
>>>>> How does changing your software implantation invalidate a description of
>>>>> the hardware?
>>>>> 
>>>> 
>>>> Which is kind of proof that this syscon was just to substitute
>>>> incomplete hardware description (e.g. missing clocks and phys). We
>>>> should have rejected it. Just like we should reject them in the future.
>>> 
>>> :s I dunno what to do with this... I'm inclined to say not to remove it
>>> from the binding or dts at all & only change the software.
>>> 
>>>> There are just few cases where syscon is reasonable. All others is just
>>>> laziness. It's not only starfivetech, of course. Several other
>>>> contributors do the same.
>>> 
>>> I'm not sure if laziness is fair, lack of understanding is usually more
>>> likely.
>> 
>> For this, I tend to keep it in binding, but remove it from required. Because
>> we only modify the tuning implementation, it doesn't mean that this property
>> need to be removed, it's just no longer be the required one.
> 
> Please only remove it from required if the current driver doesn't break
> if the regmap is removed.

Either way please make sure the documentation clearly states “never use
this, if you’re using it you’re doing it wrong, this only exists
because it was wrongly used in the past”. Otherwise people writing
drivers for other OSes will probably use it too thinking they need to.

Jess

Re: [PATCH v1 1/3] dt-bindings: mmc: Drop unused properties

[Conor Dooley]

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

On Fri, Sep 01, 2023 at 06:20:38PM +0100, Jessica Clarke wrote:
> On 1 Sep 2023, at 16:42, Conor Dooley <conor@kernel.org> wrote:
> > 
> > On Fri, Sep 01, 2023 at 10:33:13AM +0800, William Qiu wrote:
> >> 
> >> 
> >> On 2023/8/30 16:34, Conor Dooley wrote:
> >>> On Wed, Aug 30, 2023 at 09:29:20AM +0200, Krzysztof Kozlowski wrote:
> >>>> On 30/08/2023 08:50, Conor Dooley wrote:
> >>>>> On Wed, Aug 30, 2023 at 11:18:44AM +0800, William Qiu wrote:
> >>>>>> Due to the change of tuning implementation, it's no longer necessary to
> >>>>>> use the "starfive,sysreg" property in dts, so drop the relevant
> >>>>>> description in dt-bindings here.
> >>>>> 
> >>>>> How does changing your software implantation invalidate a description of
> >>>>> the hardware?
> >>>>> 
> >>>> 
> >>>> Which is kind of proof that this syscon was just to substitute
> >>>> incomplete hardware description (e.g. missing clocks and phys). We
> >>>> should have rejected it. Just like we should reject them in the future.
> >>> 
> >>> :s I dunno what to do with this... I'm inclined to say not to remove it
> >>> from the binding or dts at all & only change the software.
> >>> 
> >>>> There are just few cases where syscon is reasonable. All others is just
> >>>> laziness. It's not only starfivetech, of course. Several other
> >>>> contributors do the same.
> >>> 
> >>> I'm not sure if laziness is fair, lack of understanding is usually more
> >>> likely.
> >> 
> >> For this, I tend to keep it in binding, but remove it from required. Because
> >> we only modify the tuning implementation, it doesn't mean that this property
> >> need to be removed, it's just no longer be the required one.
> > 
> > Please only remove it from required if the current driver doesn't break
> > if the regmap is removed.
> 
> Either way please make sure the documentation clearly states “never use
> this, if you’re using it you’re doing it wrong, this only exists
> because it was wrongly used in the past”. Otherwise people writing
> drivers for other OSes will probably use it too thinking they need to.

Maybe we should just delete it if the impact is going to be negligible,
sounds like you're not using it in FreeBSD, which was part of what I was
worried about. Guess it depends on what Emil & the distro heads think.

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

Re: [PATCH v1 1/3] dt-bindings: mmc: Drop unused properties

[Jessica Clarke]

On 1 Sep 2023, at 18:43, Conor Dooley <conor@kernel.org> wrote:
> 
> On Fri, Sep 01, 2023 at 06:20:38PM +0100, Jessica Clarke wrote:
>> On 1 Sep 2023, at 16:42, Conor Dooley <conor@kernel.org> wrote:
>>> 
>>> On Fri, Sep 01, 2023 at 10:33:13AM +0800, William Qiu wrote:
>>>> 
>>>> 
>>>> On 2023/8/30 16:34, Conor Dooley wrote:
>>>>> On Wed, Aug 30, 2023 at 09:29:20AM +0200, Krzysztof Kozlowski wrote:
>>>>>> On 30/08/2023 08:50, Conor Dooley wrote:
>>>>>>> On Wed, Aug 30, 2023 at 11:18:44AM +0800, William Qiu wrote:
>>>>>>>> Due to the change of tuning implementation, it's no longer necessary to
>>>>>>>> use the "starfive,sysreg" property in dts, so drop the relevant
>>>>>>>> description in dt-bindings here.
>>>>>>> 
>>>>>>> How does changing your software implantation invalidate a description of
>>>>>>> the hardware?
>>>>>>> 
>>>>>> 
>>>>>> Which is kind of proof that this syscon was just to substitute
>>>>>> incomplete hardware description (e.g. missing clocks and phys). We
>>>>>> should have rejected it. Just like we should reject them in the future.
>>>>> 
>>>>> :s I dunno what to do with this... I'm inclined to say not to remove it
>>>>> from the binding or dts at all & only change the software.
>>>>> 
>>>>>> There are just few cases where syscon is reasonable. All others is just
>>>>>> laziness. It's not only starfivetech, of course. Several other
>>>>>> contributors do the same.
>>>>> 
>>>>> I'm not sure if laziness is fair, lack of understanding is usually more
>>>>> likely.
>>>> 
>>>> For this, I tend to keep it in binding, but remove it from required. Because
>>>> we only modify the tuning implementation, it doesn't mean that this property
>>>> need to be removed, it's just no longer be the required one.
>>> 
>>> Please only remove it from required if the current driver doesn't break
>>> if the regmap is removed.
>> 
>> Either way please make sure the documentation clearly states “never use
>> this, if you’re using it you’re doing it wrong, this only exists
>> because it was wrongly used in the past”. Otherwise people writing
>> drivers for other OSes will probably use it too thinking they need to.
> 
> Maybe we should just delete it if the impact is going to be negligible,
> sounds like you're not using it in FreeBSD, which was part of what I was
> worried about. Guess it depends on what Emil & the distro heads think.

FreeBSD doesn’t have StarFive drivers yet; I don’t have time to write
them, and a community member has taken it upon themselves as a hobby
but is rather inexperienced and has been struggling for months. OpenBSD
has drivers, including a modified dwmmc, but doesn’t use this property
(in fact its driver doesn’t use the compatible other than to probe the
generic driver). I don’t think anyone else has a serious port; Haiku’s
the closest but also has no StarFive support.

Jess

Re: [PATCH v1 1/3] dt-bindings: mmc: Drop unused properties

[William Qiu]

On 2023/9/2 1:43, Conor Dooley wrote:
> On Fri, Sep 01, 2023 at 06:20:38PM +0100, Jessica Clarke wrote:
>> On 1 Sep 2023, at 16:42, Conor Dooley <conor@kernel.org> wrote:
>> > 
>> > On Fri, Sep 01, 2023 at 10:33:13AM +0800, William Qiu wrote:
>> >> 
>> >> 
>> >> On 2023/8/30 16:34, Conor Dooley wrote:
>> >>> On Wed, Aug 30, 2023 at 09:29:20AM +0200, Krzysztof Kozlowski wrote:
>> >>>> On 30/08/2023 08:50, Conor Dooley wrote:
>> >>>>> On Wed, Aug 30, 2023 at 11:18:44AM +0800, William Qiu wrote:
>> >>>>>> Due to the change of tuning implementation, it's no longer necessary to
>> >>>>>> use the "starfive,sysreg" property in dts, so drop the relevant
>> >>>>>> description in dt-bindings here.
>> >>>>> 
>> >>>>> How does changing your software implantation invalidate a description of
>> >>>>> the hardware?
>> >>>>> 
>> >>>> 
>> >>>> Which is kind of proof that this syscon was just to substitute
>> >>>> incomplete hardware description (e.g. missing clocks and phys). We
>> >>>> should have rejected it. Just like we should reject them in the future.
>> >>> 
>> >>> :s I dunno what to do with this... I'm inclined to say not to remove it
>> >>> from the binding or dts at all & only change the software.
>> >>> 
>> >>>> There are just few cases where syscon is reasonable. All others is just
>> >>>> laziness. It's not only starfivetech, of course. Several other
>> >>>> contributors do the same.
>> >>> 
>> >>> I'm not sure if laziness is fair, lack of understanding is usually more
>> >>> likely.
>> >> 
>> >> For this, I tend to keep it in binding, but remove it from required. Because
>> >> we only modify the tuning implementation, it doesn't mean that this property
>> >> need to be removed, it's just no longer be the required one.
>> > 
>> > Please only remove it from required if the current driver doesn't break
>> > if the regmap is removed.
>> 
>> Either way please make sure the documentation clearly states “never use
>> this, if you’re using it you’re doing it wrong, this only exists
>> because it was wrongly used in the past”. Otherwise people writing
>> drivers for other OSes will probably use it too thinking they need to.
> 
> Maybe we should just delete it if the impact is going to be negligible,
> sounds like you're not using it in FreeBSD, which was part of what I was
> worried about. Guess it depends on what Emil & the distro heads think.
Hi Conor,

After discussing it with our colleagues, we decided that deleting it was the best
course of action. Since there will no longer be a related implementation of
"starfive,sysreg" in future drivers, even if the dt-binding is described, it will
be "never use", so I think it should be deleted.

What do you think?

Best regards,
William

Re: [PATCH v1 1/3] dt-bindings: mmc: Drop unused properties

[Emil Renner Berthing]

On Fri, 8 Sept 2023 at 12:03, William Qiu <william.qiu@starfivetech.com> wrote:
> On 2023/9/2 1:43, Conor Dooley wrote:
> > On Fri, Sep 01, 2023 at 06:20:38PM +0100, Jessica Clarke wrote:
> >> On 1 Sep 2023, at 16:42, Conor Dooley <conor@kernel.org> wrote:
> >> >
> >> > On Fri, Sep 01, 2023 at 10:33:13AM +0800, William Qiu wrote:
> >> >>
> >> >>
> >> >> On 2023/8/30 16:34, Conor Dooley wrote:
> >> >>> On Wed, Aug 30, 2023 at 09:29:20AM +0200, Krzysztof Kozlowski wrote:
> >> >>>> On 30/08/2023 08:50, Conor Dooley wrote:
> >> >>>>> On Wed, Aug 30, 2023 at 11:18:44AM +0800, William Qiu wrote:
> >> >>>>>> Due to the change of tuning implementation, it's no longer necessary to
> >> >>>>>> use the "starfive,sysreg" property in dts, so drop the relevant
> >> >>>>>> description in dt-bindings here.
> >> >>>>>
> >> >>>>> How does changing your software implantation invalidate a description of
> >> >>>>> the hardware?
> >> >>>>>
> >> >>>>
> >> >>>> Which is kind of proof that this syscon was just to substitute
> >> >>>> incomplete hardware description (e.g. missing clocks and phys). We
> >> >>>> should have rejected it. Just like we should reject them in the future.
> >> >>>
> >> >>> :s I dunno what to do with this... I'm inclined to say not to remove it
> >> >>> from the binding or dts at all & only change the software.
> >> >>>
> >> >>>> There are just few cases where syscon is reasonable. All others is just
> >> >>>> laziness. It's not only starfivetech, of course. Several other
> >> >>>> contributors do the same.
> >> >>>
> >> >>> I'm not sure if laziness is fair, lack of understanding is usually more
> >> >>> likely.
> >> >>
> >> >> For this, I tend to keep it in binding, but remove it from required. Because
> >> >> we only modify the tuning implementation, it doesn't mean that this property
> >> >> need to be removed, it's just no longer be the required one.
> >> >
> >> > Please only remove it from required if the current driver doesn't break
> >> > if the regmap is removed.
> >>
> >> Either way please make sure the documentation clearly states “never use
> >> this, if you’re using it you’re doing it wrong, this only exists
> >> because it was wrongly used in the past”. Otherwise people writing
> >> drivers for other OSes will probably use it too thinking they need to.
> >
> > Maybe we should just delete it if the impact is going to be negligible,
> > sounds like you're not using it in FreeBSD, which was part of what I was
> > worried about. Guess it depends on what Emil & the distro heads think.
> Hi Conor,
>
> After discussing it with our colleagues, we decided that deleting it was the best
> course of action. Since there will no longer be a related implementation of
> "starfive,sysreg" in future drivers, even if the dt-binding is described, it will
> be "never use", so I think it should be deleted.
>
> What do you think?

The device tree should be a description of the hardware and there
really is a 'u0_sdio_data_strobe_phase_ctrl' field in the sysreg
registers[1] on the JH7110 that seems to do _something_ related to the
sdio interface. So I don't think the fact that the Linux driver no
longer uses it is a good reason to remove it, but if there are some
other pragmatic reasons to do so then I'm fine with it. Removing it
from the list of required properties should be fine though.

/Emil

[1]: https://doc-en.rvspace.org/JH7110/TRM/JH7110_TRM/sys_syscon.html

Re: [PATCH v1 1/3] dt-bindings: mmc: Drop unused properties

[Conor Dooley]

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

On Fri, Sep 08, 2023 at 03:32:36PM +0200, Emil Renner Berthing wrote:
> On Fri, 8 Sept 2023 at 12:03, William Qiu <william.qiu@starfivetech.com> wrote:
> > On 2023/9/2 1:43, Conor Dooley wrote:
> > > On Fri, Sep 01, 2023 at 06:20:38PM +0100, Jessica Clarke wrote:
> > >> On 1 Sep 2023, at 16:42, Conor Dooley <conor@kernel.org> wrote:
> > >> >
> > >> > On Fri, Sep 01, 2023 at 10:33:13AM +0800, William Qiu wrote:
> > >> >>
> > >> >>
> > >> >> On 2023/8/30 16:34, Conor Dooley wrote:
> > >> >>> On Wed, Aug 30, 2023 at 09:29:20AM +0200, Krzysztof Kozlowski wrote:
> > >> >>>> On 30/08/2023 08:50, Conor Dooley wrote:
> > >> >>>>> On Wed, Aug 30, 2023 at 11:18:44AM +0800, William Qiu wrote:
> > >> >>>>>> Due to the change of tuning implementation, it's no longer necessary to
> > >> >>>>>> use the "starfive,sysreg" property in dts, so drop the relevant
> > >> >>>>>> description in dt-bindings here.
> > >> >>>>>
> > >> >>>>> How does changing your software implantation invalidate a description of
> > >> >>>>> the hardware?
> > >> >>>>>
> > >> >>>>
> > >> >>>> Which is kind of proof that this syscon was just to substitute
> > >> >>>> incomplete hardware description (e.g. missing clocks and phys). We
> > >> >>>> should have rejected it. Just like we should reject them in the future.
> > >> >>>
> > >> >>> :s I dunno what to do with this... I'm inclined to say not to remove it
> > >> >>> from the binding or dts at all & only change the software.
> > >> >>>
> > >> >>>> There are just few cases where syscon is reasonable. All others is just
> > >> >>>> laziness. It's not only starfivetech, of course. Several other
> > >> >>>> contributors do the same.
> > >> >>>
> > >> >>> I'm not sure if laziness is fair, lack of understanding is usually more
> > >> >>> likely.
> > >> >>
> > >> >> For this, I tend to keep it in binding, but remove it from required. Because
> > >> >> we only modify the tuning implementation, it doesn't mean that this property
> > >> >> need to be removed, it's just no longer be the required one.
> > >> >
> > >> > Please only remove it from required if the current driver doesn't break
> > >> > if the regmap is removed.
> > >>
> > >> Either way please make sure the documentation clearly states “never use
> > >> this, if you’re using it you’re doing it wrong, this only exists
> > >> because it was wrongly used in the past”. Otherwise people writing
> > >> drivers for other OSes will probably use it too thinking they need to.
> > >
> > > Maybe we should just delete it if the impact is going to be negligible,
> > > sounds like you're not using it in FreeBSD, which was part of what I was
> > > worried about. Guess it depends on what Emil & the distro heads think.
> > Hi Conor,
> >
> > After discussing it with our colleagues, we decided that deleting it was the best
> > course of action. Since there will no longer be a related implementation of
> > "starfive,sysreg" in future drivers, even if the dt-binding is described, it will
> > be "never use", so I think it should be deleted.
> >
> > What do you think?
> 
> The device tree should be a description of the hardware and there
> really is a 'u0_sdio_data_strobe_phase_ctrl' field in the sysreg
> registers[1] on the JH7110 that seems to do _something_ related to the
> sdio interface. So I don't think the fact that the Linux driver no
> longer uses it is a good reason to remove it, but if there are some
> other pragmatic reasons to do so then I'm fine with it. Removing it
> from the list of required properties should be fine though.

SGTM. Can you update the patch to do that please William?

Thanks,
Conor.

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

Re: [PATCH v1 1/3] dt-bindings: mmc: Drop unused properties

[William Qiu]

On 2023/9/12 0:14, Conor Dooley wrote:
> On Fri, Sep 08, 2023 at 03:32:36PM +0200, Emil Renner Berthing wrote:
>> On Fri, 8 Sept 2023 at 12:03, William Qiu <william.qiu@starfivetech.com> wrote:
>> > On 2023/9/2 1:43, Conor Dooley wrote:
>> > > On Fri, Sep 01, 2023 at 06:20:38PM +0100, Jessica Clarke wrote:
>> > >> On 1 Sep 2023, at 16:42, Conor Dooley <conor@kernel.org> wrote:
>> > >> >
>> > >> > On Fri, Sep 01, 2023 at 10:33:13AM +0800, William Qiu wrote:
>> > >> >>
>> > >> >>
>> > >> >> On 2023/8/30 16:34, Conor Dooley wrote:
>> > >> >>> On Wed, Aug 30, 2023 at 09:29:20AM +0200, Krzysztof Kozlowski wrote:
>> > >> >>>> On 30/08/2023 08:50, Conor Dooley wrote:
>> > >> >>>>> On Wed, Aug 30, 2023 at 11:18:44AM +0800, William Qiu wrote:
>> > >> >>>>>> Due to the change of tuning implementation, it's no longer necessary to
>> > >> >>>>>> use the "starfive,sysreg" property in dts, so drop the relevant
>> > >> >>>>>> description in dt-bindings here.
>> > >> >>>>>
>> > >> >>>>> How does changing your software implantation invalidate a description of
>> > >> >>>>> the hardware?
>> > >> >>>>>
>> > >> >>>>
>> > >> >>>> Which is kind of proof that this syscon was just to substitute
>> > >> >>>> incomplete hardware description (e.g. missing clocks and phys). We
>> > >> >>>> should have rejected it. Just like we should reject them in the future.
>> > >> >>>
>> > >> >>> :s I dunno what to do with this... I'm inclined to say not to remove it
>> > >> >>> from the binding or dts at all & only change the software.
>> > >> >>>
>> > >> >>>> There are just few cases where syscon is reasonable. All others is just
>> > >> >>>> laziness. It's not only starfivetech, of course. Several other
>> > >> >>>> contributors do the same.
>> > >> >>>
>> > >> >>> I'm not sure if laziness is fair, lack of understanding is usually more
>> > >> >>> likely.
>> > >> >>
>> > >> >> For this, I tend to keep it in binding, but remove it from required. Because
>> > >> >> we only modify the tuning implementation, it doesn't mean that this property
>> > >> >> need to be removed, it's just no longer be the required one.
>> > >> >
>> > >> > Please only remove it from required if the current driver doesn't break
>> > >> > if the regmap is removed.
>> > >>
>> > >> Either way please make sure the documentation clearly states “never use
>> > >> this, if you’re using it you’re doing it wrong, this only exists
>> > >> because it was wrongly used in the past”. Otherwise people writing
>> > >> drivers for other OSes will probably use it too thinking they need to.
>> > >
>> > > Maybe we should just delete it if the impact is going to be negligible,
>> > > sounds like you're not using it in FreeBSD, which was part of what I was
>> > > worried about. Guess it depends on what Emil & the distro heads think.
>> > Hi Conor,
>> >
>> > After discussing it with our colleagues, we decided that deleting it was the best
>> > course of action. Since there will no longer be a related implementation of
>> > "starfive,sysreg" in future drivers, even if the dt-binding is described, it will
>> > be "never use", so I think it should be deleted.
>> >
>> > What do you think?
>> 
>> The device tree should be a description of the hardware and there
>> really is a 'u0_sdio_data_strobe_phase_ctrl' field in the sysreg
>> registers[1] on the JH7110 that seems to do _something_ related to the
>> sdio interface. So I don't think the fact that the Linux driver no
>> longer uses it is a good reason to remove it, but if there are some
>> other pragmatic reasons to do so then I'm fine with it. Removing it
>> from the list of required properties should be fine though.
> 
> SGTM. Can you update the patch to do that please William?
> 
> Thanks,
> Conor.

OK, I will update the patch as suggested by Emil.

Best Regards,
William

[PATCH v1 2/3] mmc: starfive: Change tuning implementation

[William Qiu]

Before, we used syscon to achieve tuning, but the actual measurement
showed little effect, so the tuning implementation was modified here,
and it was realized by reading and writing the UHS_REG_EXT register.

Signed-off-by: William Qiu <william.qiu@starfivetech.com>
---
 drivers/mmc/host/dw_mmc-starfive.c | 131 ++++++++---------------------
 1 file changed, 37 insertions(+), 94 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc-starfive.c b/drivers/mmc/host/dw_mmc-starfive.c
index fd05a648a8bb..593c995e49f5 100644
--- a/drivers/mmc/host/dw_mmc-starfive.c
+++ b/drivers/mmc/host/dw_mmc-starfive.c
@@ -20,14 +20,6 @@
 #define ALL_INT_CLR		0x1ffff
 #define MAX_DELAY_CHAIN		32
 
-struct starfive_priv {
-	struct device *dev;
-	struct regmap *reg_syscon;
-	u32 syscon_offset;
-	u32 syscon_shift;
-	u32 syscon_mask;
-};
-
 static void dw_mci_starfive_set_ios(struct dw_mci *host, struct mmc_ios *ios)
 {
 	int ret;
@@ -44,117 +36,68 @@ static void dw_mci_starfive_set_ios(struct dw_mci *host, struct mmc_ios *ios)
 	}
 }
 
+static void dw_mci_starfive_hs_set_bits(struct dw_mci *host, u32 smpl_phase)
+{
+	/* change driver phase and sample phase */
+	u32 mask = 0x1f;
+	u32 reg_value;
+
+	reg_value = mci_readl(host, UHS_REG_EXT);
+
+	/* In UHS_REG_EXT, only 5 bits valid in DRV_PHASE and SMPL_PHASE */
+	reg_value &= ~(mask << 16);
+	reg_value |= (smpl_phase << 16);
+	mci_writel(host, UHS_REG_EXT, reg_value);
+
+	/* We should delay 1ms wait for timing setting finished. */
+	mdelay(1);
+}
+
 static int dw_mci_starfive_execute_tuning(struct dw_mci_slot *slot,
 					     u32 opcode)
 {
 	static const int grade  = MAX_DELAY_CHAIN;
 	struct dw_mci *host = slot->host;
-	struct starfive_priv *priv = host->priv;
-	int rise_point = -1, fall_point = -1;
-	int err, prev_err = 0;
+	int err = -1;
+	int smpl_phase, smpl_raise = -1, smpl_fall = -1;
 	int i;
-	bool found = 0;
-	u32 regval;
-
-	/*
-	 * Use grade as the max delay chain, and use the rise_point and
-	 * fall_point to ensure the best sampling point of a data input
-	 * signals.
-	 */
+
 	for (i = 0; i < grade; i++) {
-		regval = i << priv->syscon_shift;
-		err = regmap_update_bits(priv->reg_syscon, priv->syscon_offset,
-						priv->syscon_mask, regval);
-		if (err)
-			return err;
+		smpl_phase = i;
+		dw_mci_starfive_hs_set_bits(host, smpl_phase);
 		mci_writel(host, RINTSTS, ALL_INT_CLR);
 
 		err = mmc_send_tuning(slot->mmc, opcode, NULL);
-		if (!err)
-			found = 1;
-
-		if (i > 0) {
-			if (err && !prev_err)
-				fall_point = i - 1;
-			if (!err && prev_err)
-				rise_point = i;
-		}
 
-		if (rise_point != -1 && fall_point != -1)
-			goto tuning_out;
-
-		prev_err = err;
-		err = 0;
-	}
-
-tuning_out:
-	if (found) {
-		if (rise_point == -1)
-			rise_point = 0;
-		if (fall_point == -1)
-			fall_point = grade - 1;
-		if (fall_point < rise_point) {
-			if ((rise_point + fall_point) >
-			    (grade - 1))
-				i = fall_point / 2;
-			else
-				i = (rise_point + grade - 1) / 2;
-		} else {
-			i = (rise_point + fall_point) / 2;
+		if (!err && smpl_raise < 0) {
+			smpl_raise = i;
+		} else if (err && smpl_raise >= 0) {
+			smpl_fall = i - 1;
+			break;
 		}
+	}
 
-		regval = i << priv->syscon_shift;
-		err = regmap_update_bits(priv->reg_syscon, priv->syscon_offset,
-						priv->syscon_mask, regval);
-		if (err)
-			return err;
-		mci_writel(host, RINTSTS, ALL_INT_CLR);
+	if (i >= grade && smpl_raise >= 0)
+		smpl_fall = grade - 1;
 
-		dev_info(host->dev, "Found valid delay chain! use it [delay=%d]\n", i);
-	} else {
+	if (smpl_raise < 0) {
 		dev_err(host->dev, "No valid delay chain! use default\n");
+		dw_mci_starfive_hs_set_bits(host, 0);
 		err = -EINVAL;
+	} else {
+		smpl_phase = (smpl_raise + smpl_fall) / 2;
+		dw_mci_starfive_hs_set_bits(host, smpl_phase);
+		dev_dbg(host->dev, "Found valid delay chain! use it [delay=%d]\n", smpl_phase);
+		err = 0;
 	}
 
 	mci_writel(host, RINTSTS, ALL_INT_CLR);
 	return err;
 }
 
-static int dw_mci_starfive_parse_dt(struct dw_mci *host)
-{
-	struct of_phandle_args args;
-	struct starfive_priv *priv;
-	int ret;
-
-	priv = devm_kzalloc(host->dev, sizeof(*priv), GFP_KERNEL);
-	if (!priv)
-		return -ENOMEM;
-
-	ret = of_parse_phandle_with_fixed_args(host->dev->of_node,
-						"starfive,sysreg", 3, 0, &args);
-	if (ret) {
-		dev_err(host->dev, "Failed to parse starfive,sysreg\n");
-		return -EINVAL;
-	}
-
-	priv->reg_syscon = syscon_node_to_regmap(args.np);
-	of_node_put(args.np);
-	if (IS_ERR(priv->reg_syscon))
-		return PTR_ERR(priv->reg_syscon);
-
-	priv->syscon_offset = args.args[0];
-	priv->syscon_shift  = args.args[1];
-	priv->syscon_mask   = args.args[2];
-
-	host->priv = priv;
-
-	return 0;
-}
-
 static const struct dw_mci_drv_data starfive_data = {
 	.common_caps		= MMC_CAP_CMD23,
 	.set_ios		= dw_mci_starfive_set_ios,
-	.parse_dt		= dw_mci_starfive_parse_dt,
 	.execute_tuning		= dw_mci_starfive_execute_tuning,
 };
 
-- 
2.34.1

Re: [PATCH v1 2/3] mmc: starfive: Change tuning implementation

[Emil Renner Berthing]

On Wed, 30 Aug 2023 at 05:21, William Qiu <william.qiu@starfivetech.com> wrote:
>
> Before, we used syscon to achieve tuning, but the actual measurement
> showed little effect, so the tuning implementation was modified here,
> and it was realized by reading and writing the UHS_REG_EXT register.
>
> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
> ---
>  drivers/mmc/host/dw_mmc-starfive.c | 131 ++++++++---------------------
>  1 file changed, 37 insertions(+), 94 deletions(-)
>
> diff --git a/drivers/mmc/host/dw_mmc-starfive.c b/drivers/mmc/host/dw_mmc-starfive.c
> index fd05a648a8bb..593c995e49f5 100644
> --- a/drivers/mmc/host/dw_mmc-starfive.c
> +++ b/drivers/mmc/host/dw_mmc-starfive.c
> @@ -20,14 +20,6 @@
>  #define ALL_INT_CLR            0x1ffff
>  #define MAX_DELAY_CHAIN                32
>
> -struct starfive_priv {
> -       struct device *dev;
> -       struct regmap *reg_syscon;
> -       u32 syscon_offset;
> -       u32 syscon_shift;
> -       u32 syscon_mask;
> -};
> -
>  static void dw_mci_starfive_set_ios(struct dw_mci *host, struct mmc_ios *ios)
>  {
>         int ret;
> @@ -44,117 +36,68 @@ static void dw_mci_starfive_set_ios(struct dw_mci *host, struct mmc_ios *ios)
>         }
>  }
>
> +static void dw_mci_starfive_hs_set_bits(struct dw_mci *host, u32 smpl_phase)

"set bits" is very generic. Maybe dw_mci_starfive_set_sample_phase()
or something more descriptive.

> +{
> +       /* change driver phase and sample phase */
> +       u32 mask = 0x1f;
> +       u32 reg_value;
> +
> +       reg_value = mci_readl(host, UHS_REG_EXT);
> +
> +       /* In UHS_REG_EXT, only 5 bits valid in DRV_PHASE and SMPL_PHASE */
> +       reg_value &= ~(mask << 16);
> +       reg_value |= (smpl_phase << 16);
> +       mci_writel(host, UHS_REG_EXT, reg_value);
> +
> +       /* We should delay 1ms wait for timing setting finished. */
> +       mdelay(1);
> +}

This implementation could use some cleanup. Eg. why do we need the
mask variable?
How about something like this:

#define STARFIVE_SMPL_PHASE     GENMASK(20, 16)

u32 reg_value = mci_read(host, UHS_REG_EXT);
reg_value &= ~STARFIVE_SMPL_PHASE;
reg_value |= FIELD_PREP(STARFIVE_SMPL_PHASE, smpl_phase);
mci_writel(host, UHS_REG_EXT, reg_value);
...

>  static int dw_mci_starfive_execute_tuning(struct dw_mci_slot *slot,
>                                              u32 opcode)
>  {
>         static const int grade  = MAX_DELAY_CHAIN;
>         struct dw_mci *host = slot->host;
> -       struct starfive_priv *priv = host->priv;
> -       int rise_point = -1, fall_point = -1;
> -       int err, prev_err = 0;
> +       int err = -1;

This variable is always set later so doesn't need initialization and
is better called 'ret' as it's the return value of the function, and
not necessarily an error.

> +       int smpl_phase, smpl_raise = -1, smpl_fall = -1;
>         int i;
> -       bool found = 0;
> -       u32 regval;
> -
> -       /*
> -        * Use grade as the max delay chain, and use the rise_point and
> -        * fall_point to ensure the best sampling point of a data input
> -        * signals.
> -        */
> +
>         for (i = 0; i < grade; i++) {
> -               regval = i << priv->syscon_shift;
> -               err = regmap_update_bits(priv->reg_syscon, priv->syscon_offset,
> -                                               priv->syscon_mask, regval);
> -               if (err)
> -                       return err;
> +               smpl_phase = i;

This can now be written

for (sampl_phase = 0; sampl_phase < grade; sampl_phase++)

> +               dw_mci_starfive_hs_set_bits(host, smpl_phase);
>                 mci_writel(host, RINTSTS, ALL_INT_CLR);
>
>                 err = mmc_send_tuning(slot->mmc, opcode, NULL);
> -               if (!err)
> -                       found = 1;
> -
> -               if (i > 0) {
> -                       if (err && !prev_err)
> -                               fall_point = i - 1;
> -                       if (!err && prev_err)
> -                               rise_point = i;
> -               }
>
> -               if (rise_point != -1 && fall_point != -1)
> -                       goto tuning_out;
> -
> -               prev_err = err;
> -               err = 0;
> -       }
> -
> -tuning_out:
> -       if (found) {
> -               if (rise_point == -1)
> -                       rise_point = 0;
> -               if (fall_point == -1)
> -                       fall_point = grade - 1;
> -               if (fall_point < rise_point) {
> -                       if ((rise_point + fall_point) >
> -                           (grade - 1))
> -                               i = fall_point / 2;
> -                       else
> -                               i = (rise_point + grade - 1) / 2;
> -               } else {
> -                       i = (rise_point + fall_point) / 2;
> +               if (!err && smpl_raise < 0) {
> +                       smpl_raise = i;
> +               } else if (err && smpl_raise >= 0) {
> +                       smpl_fall = i - 1;
> +                       break;
>                 }
> +       }
>
> -               regval = i << priv->syscon_shift;
> -               err = regmap_update_bits(priv->reg_syscon, priv->syscon_offset,
> -                                               priv->syscon_mask, regval);
> -               if (err)
> -                       return err;
> -               mci_writel(host, RINTSTS, ALL_INT_CLR);
> +       if (i >= grade && smpl_raise >= 0)
> +               smpl_fall = grade - 1;
>
> -               dev_info(host->dev, "Found valid delay chain! use it [delay=%d]\n", i);
> -       } else {
> +       if (smpl_raise < 0) {
>                 dev_err(host->dev, "No valid delay chain! use default\n");
> +               dw_mci_starfive_hs_set_bits(host, 0);
>                 err = -EINVAL;
> +       } else {
> +               smpl_phase = (smpl_raise + smpl_fall) / 2;
> +               dw_mci_starfive_hs_set_bits(host, smpl_phase);
> +               dev_dbg(host->dev, "Found valid delay chain! use it [delay=%d]\n", smpl_phase);
> +               err = 0;
>         }

Maybe something like:

  if (smpl_raise < 0) {
    smpl_phase = 0;
    dev_err(host->dev, "No valid delay chain, using default\n");
    ret = -EINVAL;
    goto out;
  }

  smpl_phase = (smpl_raise + smpl_fall) / 2;
  dev_dbg(...);
  ret = 0;

out:
  dw_mci_starfive_hs_set_bits(host, smpl_phase);
  mci_writel(host, RINTSTS, ALL_INT_CLR);
  return ret;
>  }
>
> -static int dw_mci_starfive_parse_dt(struct dw_mci *host)
> -{
> -       struct of_phandle_args args;
> -       struct starfive_priv *priv;
> -       int ret;
> -
> -       priv = devm_kzalloc(host->dev, sizeof(*priv), GFP_KERNEL);
> -       if (!priv)
> -               return -ENOMEM;
> -
> -       ret = of_parse_phandle_with_fixed_args(host->dev->of_node,
> -                                               "starfive,sysreg", 3, 0, &args);
> -       if (ret) {
> -               dev_err(host->dev, "Failed to parse starfive,sysreg\n");
> -               return -EINVAL;
> -       }
> -
> -       priv->reg_syscon = syscon_node_to_regmap(args.np);
> -       of_node_put(args.np);
> -       if (IS_ERR(priv->reg_syscon))
> -               return PTR_ERR(priv->reg_syscon);
> -
> -       priv->syscon_offset = args.args[0];
> -       priv->syscon_shift  = args.args[1];
> -       priv->syscon_mask   = args.args[2];
> -
> -       host->priv = priv;
> -
> -       return 0;
> -}
> -
>  static const struct dw_mci_drv_data starfive_data = {
>         .common_caps            = MMC_CAP_CMD23,
>         .set_ios                = dw_mci_starfive_set_ios,
> -       .parse_dt               = dw_mci_starfive_parse_dt,
>         .execute_tuning         = dw_mci_starfive_execute_tuning,
>  };
>
> --
> 2.34.1
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v1 2/3] mmc: starfive: Change tuning implementation

[William Qiu]

On 2023/8/30 18:28, Emil Renner Berthing wrote:
> On Wed, 30 Aug 2023 at 05:21, William Qiu <william.qiu@starfivetech.com> wrote:
>>
>> Before, we used syscon to achieve tuning, but the actual measurement
>> showed little effect, so the tuning implementation was modified here,
>> and it was realized by reading and writing the UHS_REG_EXT register.
>>
>> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
>> ---
>>  drivers/mmc/host/dw_mmc-starfive.c | 131 ++++++++---------------------
>>  1 file changed, 37 insertions(+), 94 deletions(-)
>>
>> diff --git a/drivers/mmc/host/dw_mmc-starfive.c b/drivers/mmc/host/dw_mmc-starfive.c
>> index fd05a648a8bb..593c995e49f5 100644
>> --- a/drivers/mmc/host/dw_mmc-starfive.c
>> +++ b/drivers/mmc/host/dw_mmc-starfive.c
>> @@ -20,14 +20,6 @@
>>  #define ALL_INT_CLR            0x1ffff
>>  #define MAX_DELAY_CHAIN                32
>>
>> -struct starfive_priv {
>> -       struct device *dev;
>> -       struct regmap *reg_syscon;
>> -       u32 syscon_offset;
>> -       u32 syscon_shift;
>> -       u32 syscon_mask;
>> -};
>> -
>>  static void dw_mci_starfive_set_ios(struct dw_mci *host, struct mmc_ios *ios)
>>  {
>>         int ret;
>> @@ -44,117 +36,68 @@ static void dw_mci_starfive_set_ios(struct dw_mci *host, struct mmc_ios *ios)
>>         }
>>  }
>>
>> +static void dw_mci_starfive_hs_set_bits(struct dw_mci *host, u32 smpl_phase)
> 
> "set bits" is very generic. Maybe dw_mci_starfive_set_sample_phase()
> or something more descriptive.
> 
Will update.
>> +{
>> +       /* change driver phase and sample phase */
>> +       u32 mask = 0x1f;
>> +       u32 reg_value;
>> +
>> +       reg_value = mci_readl(host, UHS_REG_EXT);
>> +
>> +       /* In UHS_REG_EXT, only 5 bits valid in DRV_PHASE and SMPL_PHASE */
>> +       reg_value &= ~(mask << 16);
>> +       reg_value |= (smpl_phase << 16);
>> +       mci_writel(host, UHS_REG_EXT, reg_value);
>> +
>> +       /* We should delay 1ms wait for timing setting finished. */
>> +       mdelay(1);
>> +}
> 
> This implementation could use some cleanup. Eg. why do we need the
> mask variable?
> How about something like this:
> 
> #define STARFIVE_SMPL_PHASE     GENMASK(20, 16)
> 
> u32 reg_value = mci_read(host, UHS_REG_EXT);
> reg_value &= ~STARFIVE_SMPL_PHASE;
> reg_value |= FIELD_PREP(STARFIVE_SMPL_PHASE, smpl_phase);
> mci_writel(host, UHS_REG_EXT, reg_value);
> ...
> 
I'll try it.
>>  static int dw_mci_starfive_execute_tuning(struct dw_mci_slot *slot,
>>                                              u32 opcode)
>>  {
>>         static const int grade  = MAX_DELAY_CHAIN;
>>         struct dw_mci *host = slot->host;
>> -       struct starfive_priv *priv = host->priv;
>> -       int rise_point = -1, fall_point = -1;
>> -       int err, prev_err = 0;
>> +       int err = -1;
> 
> This variable is always set later so doesn't need initialization and
> is better called 'ret' as it's the return value of the function, and
> not necessarily an error.
Will update.
> 
>> +       int smpl_phase, smpl_raise = -1, smpl_fall = -1;
>>         int i;
>> -       bool found = 0;
>> -       u32 regval;
>> -
>> -       /*
>> -        * Use grade as the max delay chain, and use the rise_point and
>> -        * fall_point to ensure the best sampling point of a data input
>> -        * signals.
>> -        */
>> +
>>         for (i = 0; i < grade; i++) {
>> -               regval = i << priv->syscon_shift;
>> -               err = regmap_update_bits(priv->reg_syscon, priv->syscon_offset,
>> -                                               priv->syscon_mask, regval);
>> -               if (err)
>> -                       return err;
>> +               smpl_phase = i;
> 
> This can now be written
> 
> for (sampl_phase = 0; sampl_phase < grade; sampl_phase++)
> 
Will update.
>> +               dw_mci_starfive_hs_set_bits(host, smpl_phase);
>>                 mci_writel(host, RINTSTS, ALL_INT_CLR);
>>
>>                 err = mmc_send_tuning(slot->mmc, opcode, NULL);
>> -               if (!err)
>> -                       found = 1;
>> -
>> -               if (i > 0) {
>> -                       if (err && !prev_err)
>> -                               fall_point = i - 1;
>> -                       if (!err && prev_err)
>> -                               rise_point = i;
>> -               }
>>
>> -               if (rise_point != -1 && fall_point != -1)
>> -                       goto tuning_out;
>> -
>> -               prev_err = err;
>> -               err = 0;
>> -       }
>> -
>> -tuning_out:
>> -       if (found) {
>> -               if (rise_point == -1)
>> -                       rise_point = 0;
>> -               if (fall_point == -1)
>> -                       fall_point = grade - 1;
>> -               if (fall_point < rise_point) {
>> -                       if ((rise_point + fall_point) >
>> -                           (grade - 1))
>> -                               i = fall_point / 2;
>> -                       else
>> -                               i = (rise_point + grade - 1) / 2;
>> -               } else {
>> -                       i = (rise_point + fall_point) / 2;
>> +               if (!err && smpl_raise < 0) {
>> +                       smpl_raise = i;
>> +               } else if (err && smpl_raise >= 0) {
>> +                       smpl_fall = i - 1;
>> +                       break;
>>                 }
>> +       }
>>
>> -               regval = i << priv->syscon_shift;
>> -               err = regmap_update_bits(priv->reg_syscon, priv->syscon_offset,
>> -                                               priv->syscon_mask, regval);
>> -               if (err)
>> -                       return err;
>> -               mci_writel(host, RINTSTS, ALL_INT_CLR);
>> +       if (i >= grade && smpl_raise >= 0)
>> +               smpl_fall = grade - 1;
>>
>> -               dev_info(host->dev, "Found valid delay chain! use it [delay=%d]\n", i);
>> -       } else {
>> +       if (smpl_raise < 0) {
>>                 dev_err(host->dev, "No valid delay chain! use default\n");
>> +               dw_mci_starfive_hs_set_bits(host, 0);
>>                 err = -EINVAL;
>> +       } else {
>> +               smpl_phase = (smpl_raise + smpl_fall) / 2;
>> +               dw_mci_starfive_hs_set_bits(host, smpl_phase);
>> +               dev_dbg(host->dev, "Found valid delay chain! use it [delay=%d]\n", smpl_phase);
>> +               err = 0;
>>         }
> 
> Maybe something like:
> 
>   if (smpl_raise < 0) {
>     smpl_phase = 0;
>     dev_err(host->dev, "No valid delay chain, using default\n");
>     ret = -EINVAL;
>     goto out;
>   }
> 
>   smpl_phase = (smpl_raise + smpl_fall) / 2;
>   dev_dbg(...);
>   ret = 0;
> 
> out:
>   dw_mci_starfive_hs_set_bits(host, smpl_phase);
>   mci_writel(host, RINTSTS, ALL_INT_CLR);
>   return ret;
>>  }
>>
I'll try it.

Thanks for taking time to review this patch series and giving so much
helpful suggestions.

Best regards,
William
>> -static int dw_mci_starfive_parse_dt(struct dw_mci *host)
>> -{
>> -       struct of_phandle_args args;
>> -       struct starfive_priv *priv;
>> -       int ret;
>> -
>> -       priv = devm_kzalloc(host->dev, sizeof(*priv), GFP_KERNEL);
>> -       if (!priv)
>> -               return -ENOMEM;
>> -
>> -       ret = of_parse_phandle_with_fixed_args(host->dev->of_node,
>> -                                               "starfive,sysreg", 3, 0, &args);
>> -       if (ret) {
>> -               dev_err(host->dev, "Failed to parse starfive,sysreg\n");
>> -               return -EINVAL;
>> -       }
>> -
>> -       priv->reg_syscon = syscon_node_to_regmap(args.np);
>> -       of_node_put(args.np);
>> -       if (IS_ERR(priv->reg_syscon))
>> -               return PTR_ERR(priv->reg_syscon);
>> -
>> -       priv->syscon_offset = args.args[0];
>> -       priv->syscon_shift  = args.args[1];
>> -       priv->syscon_mask   = args.args[2];
>> -
>> -       host->priv = priv;
>> -
>> -       return 0;
>> -}
>> -
>>  static const struct dw_mci_drv_data starfive_data = {
>>         .common_caps            = MMC_CAP_CMD23,
>>         .set_ios                = dw_mci_starfive_set_ios,
>> -       .parse_dt               = dw_mci_starfive_parse_dt,
>>         .execute_tuning         = dw_mci_starfive_execute_tuning,
>>  };
>>
>> --
>> 2.34.1
>>
>>
>> _______________________________________________
>> linux-riscv mailing list
>> linux-riscv@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v1 3/3] riscv: dts: starfive: Drop unused properties and limit frquency

[William Qiu]

Drop unused properties and limit cclk_in to 50M, thus cancelling the
internal frequency and adopting the by-pass mode.

Signed-off-by: William Qiu <william.qiu@starfivetech.com>
---
 .../riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi | 4 ++++
 arch/riscv/boot/dts/starfive/jh7110.dtsi                      | 2 --
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
index d79f94432b27..d1f2ec308bca 100644
--- a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
+++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
@@ -205,6 +205,8 @@ &i2c6 {
 
 &mmc0 {
 	max-frequency = <100000000>;
+	assigned-clocks = <&syscrg JH7110_SYSCLK_SDIO0_SDCARD>;
+	assigned-clock-rates = <50000000>;
 	bus-width = <8>;
 	cap-mmc-highspeed;
 	mmc-ddr-1_8v;
@@ -221,6 +223,8 @@ &mmc0 {
 
 &mmc1 {
 	max-frequency = <100000000>;
+	assigned-clocks = <&syscrg JH7110_SYSCLK_SDIO1_SDCARD>;
+	assigned-clock-rates = <50000000>;
 	bus-width = <4>;
 	no-sdio;
 	no-mmc;
diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
index e85464c328d0..7b8e841aeef8 100644
--- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
+++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
@@ -870,7 +870,6 @@ mmc0: mmc@16010000 {
 			fifo-depth = <32>;
 			fifo-watermark-aligned;
 			data-addr = <0>;
-			starfive,sysreg = <&sys_syscon 0x14 0x1a 0x7c000000>;
 			status = "disabled";
 		};
 
@@ -886,7 +885,6 @@ mmc1: mmc@16020000 {
 			fifo-depth = <32>;
 			fifo-watermark-aligned;
 			data-addr = <0>;
-			starfive,sysreg = <&sys_syscon 0x9c 0x1 0x3e>;
 			status = "disabled";
 		};
 
-- 
2.34.1