| Message ID | 20221207131731.1291517-4-william.qiu@starfivetech.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Conor Dooley |
| Headers | show |
| Series | StarFive's SDIO/eMMC driver support | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/tree_selection | fail | Guessing tree name failed |
On 07/12/2022 14:17, William Qiu wrote: > This adds the mmc node for the StarFive JH7110 SoC. > Set sdioo node to emmc and set sdio1 node to sd. > > Signed-off-by: William Qiu <william.qiu@starfivetech.com> > --- > .../jh7110-starfive-visionfive-v2.dts | 25 ++++++++++++ > arch/riscv/boot/dts/starfive/jh7110.dtsi | 38 +++++++++++++++++++ > 2 files changed, 63 insertions(+) > > diff --git a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts > index c8946cf3a268..6ef8e303c2e6 100644 > --- a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts > +++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts > @@ -47,6 +47,31 @@ &clk_rtc { > clock-frequency = <32768>; > }; > > +&sdio0 { > + max-frequency = <100000000>; > + card-detect-delay = <300>; > + bus-width = <8>; > + cap-mmc-highspeed; > + mmc-ddr-1_8v; > + mmc-hs200-1_8v; > + non-removable; > + cap-mmc-hw-reset; > + post-power-on-delay-ms = <200>; > + status = "okay"; > +}; > + > +&sdio1 { > + max-frequency = <100000000>; > + card-detect-delay = <300>; > + bus-width = <4>; > + no-sdio; > + no-mmc; > + broken-cd; > + cap-sd-highspeed; > + post-power-on-delay-ms = <200>; > + status = "okay"; > +}; > + > &gmac0_rmii_refin { > clock-frequency = <50000000>; > }; > diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi > index c22e8f1d2640..e90b085d7e41 100644 > --- a/arch/riscv/boot/dts/starfive/jh7110.dtsi > +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi > @@ -331,6 +331,11 @@ aoncrg: clock-controller@17000000 { > #reset-cells = <1>; > }; > > + sys_syscon: sys_syscon@13030000 { No underscores in node names, generic node names (syscon or system-controller) > + compatible = "syscon"; This is not allowed alone. > + reg = <0x0 0x13030000 0x0 0x1000>; > + }; > + > gpio: gpio@13040000 { > compatible = "starfive,jh7110-sys-pinctrl"; > reg = <0x0 0x13040000 0x0 0x10000>; > @@ -433,5 +438,38 @@ uart5: serial@12020000 { > reg-shift = <2>; > status = "disabled"; > }; > + > + /* unremovable emmc as mmcblk0 */ > + sdio0: mmc@16010000 { > + compatible = "starfive,jh7110-sdio"; > + reg = <0x0 0x16010000 0x0 0x10000>; > + clocks = <&syscrg JH7110_SYSCLK_SDIO0_AHB>, > + <&syscrg JH7110_SYSCLK_SDIO0_SDCARD>; > + clock-names = "biu","ciu"; > + resets = <&syscrg JH7110_SYSRST_SDIO0_AHB>; > + reset-names = "reset"; > + interrupts = <74>; > + fifo-depth = <32>; > + fifo-watermark-aligned; > + data-addr = <0>; > + starfive,sys-syscon = <&sys_syscon 0x14 0x1a 0x7c000000>; This does not match your bindings at all. "&sys_syscon" is a phandle, not a number of tuning retries, as you expect in your bindings. Best regards, Krzysztof
On Wed, Dec 07, 2022 at 04:14:53PM +0100, Krzysztof Kozlowski wrote: > On 07/12/2022 14:17, William Qiu wrote: > > This adds the mmc node for the StarFive JH7110 SoC. > > Set sdioo node to emmc and set sdio1 node to sd. > > > > Signed-off-by: William Qiu <william.qiu@starfivetech.com> > > --- > > .../jh7110-starfive-visionfive-v2.dts | 25 ++++++++++++ > > arch/riscv/boot/dts/starfive/jh7110.dtsi | 38 +++++++++++++++++++ > > 2 files changed, 63 insertions(+) > > > > diff --git a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts > > index c8946cf3a268..6ef8e303c2e6 100644 > > --- a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts > > +++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts > > @@ -47,6 +47,31 @@ &clk_rtc { > > clock-frequency = <32768>; > > }; > > > > +&sdio0 { > > + max-frequency = <100000000>; > > + card-detect-delay = <300>; > > + bus-width = <8>; > > + cap-mmc-highspeed; > > + mmc-ddr-1_8v; > > + mmc-hs200-1_8v; > > + non-removable; > > + cap-mmc-hw-reset; > > + post-power-on-delay-ms = <200>; > > + status = "okay"; > > +}; > > + > > +&sdio1 { > > + max-frequency = <100000000>; > > + card-detect-delay = <300>; > > + bus-width = <4>; > > + no-sdio; > > + no-mmc; > > + broken-cd; > > + cap-sd-highspeed; > > + post-power-on-delay-ms = <200>; > > + status = "okay"; > > +}; > > + > > &gmac0_rmii_refin { > > clock-frequency = <50000000>; > > }; > > diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi > > index c22e8f1d2640..e90b085d7e41 100644 > > --- a/arch/riscv/boot/dts/starfive/jh7110.dtsi > > +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi > > @@ -331,6 +331,11 @@ aoncrg: clock-controller@17000000 { > > #reset-cells = <1>; > > }; > > > > + sys_syscon: sys_syscon@13030000 { > > No underscores in node names, generic node names (syscon or > system-controller) > > > + compatible = "syscon"; > > This is not allowed alone. > > > + reg = <0x0 0x13030000 0x0 0x1000>; > > + }; > > + > > gpio: gpio@13040000 { > > compatible = "starfive,jh7110-sys-pinctrl"; > > reg = <0x0 0x13040000 0x0 0x10000>; > > @@ -433,5 +438,38 @@ uart5: serial@12020000 { > > reg-shift = <2>; > > status = "disabled"; > > }; > > + > > + /* unremovable emmc as mmcblk0 */ > > + sdio0: mmc@16010000 { > > + compatible = "starfive,jh7110-sdio"; > > + reg = <0x0 0x16010000 0x0 0x10000>; > > + clocks = <&syscrg JH7110_SYSCLK_SDIO0_AHB>, > > + <&syscrg JH7110_SYSCLK_SDIO0_SDCARD>; > > + clock-names = "biu","ciu"; > > + resets = <&syscrg JH7110_SYSRST_SDIO0_AHB>; > > + reset-names = "reset"; > > + interrupts = <74>; > > + fifo-depth = <32>; > > + fifo-watermark-aligned; > > + data-addr = <0>; > > + starfive,sys-syscon = <&sys_syscon 0x14 0x1a 0x7c000000>; > > This does not match your bindings at all. "&sys_syscon" is a phandle, > not a number of tuning retries, as you expect in your bindings. Additionally, a Link: to the documentation for where-ever these "random" numbers that are being used would be nice. +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,sys-syscon", 3, 0, &args); + if (ret) { + dev_err(host->dev, "Failed to parse starfive,sys-syscon\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]; Given the driver, the property description just seems incorrect and this is actually the bit of the syscon that is relevant to the tuning process (perhaps where the find the tuning values?). Without public docs or a better description it is hard for (me at least) to know :) + + host->priv = priv; + + return 0; +}
On 2022/12/7 23:14, Krzysztof Kozlowski wrote: > On 07/12/2022 14:17, William Qiu wrote: >> This adds the mmc node for the StarFive JH7110 SoC. >> Set sdioo node to emmc and set sdio1 node to sd. >> >> Signed-off-by: William Qiu <william.qiu@starfivetech.com> >> --- >> .../jh7110-starfive-visionfive-v2.dts | 25 ++++++++++++ >> arch/riscv/boot/dts/starfive/jh7110.dtsi | 38 +++++++++++++++++++ >> 2 files changed, 63 insertions(+) >> >> diff --git a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts >> index c8946cf3a268..6ef8e303c2e6 100644 >> --- a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts >> +++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts >> @@ -47,6 +47,31 @@ &clk_rtc { >> clock-frequency = <32768>; >> }; >> >> +&sdio0 { >> + max-frequency = <100000000>; >> + card-detect-delay = <300>; >> + bus-width = <8>; >> + cap-mmc-highspeed; >> + mmc-ddr-1_8v; >> + mmc-hs200-1_8v; >> + non-removable; >> + cap-mmc-hw-reset; >> + post-power-on-delay-ms = <200>; >> + status = "okay"; >> +}; >> + >> +&sdio1 { >> + max-frequency = <100000000>; >> + card-detect-delay = <300>; >> + bus-width = <4>; >> + no-sdio; >> + no-mmc; >> + broken-cd; >> + cap-sd-highspeed; >> + post-power-on-delay-ms = <200>; >> + status = "okay"; >> +}; >> + >> &gmac0_rmii_refin { >> clock-frequency = <50000000>; >> }; >> diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi >> index c22e8f1d2640..e90b085d7e41 100644 >> --- a/arch/riscv/boot/dts/starfive/jh7110.dtsi >> +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi >> @@ -331,6 +331,11 @@ aoncrg: clock-controller@17000000 { >> #reset-cells = <1>; >> }; >> >> + sys_syscon: sys_syscon@13030000 { > > No underscores in node names, generic node names (syscon or > system-controller) Will fix. > >> + compatible = "syscon"; > > This is not allowed alone. Will fix. > >> + reg = <0x0 0x13030000 0x0 0x1000>; >> + }; >> + >> gpio: gpio@13040000 { >> compatible = "starfive,jh7110-sys-pinctrl"; >> reg = <0x0 0x13040000 0x0 0x10000>; >> @@ -433,5 +438,38 @@ uart5: serial@12020000 { >> reg-shift = <2>; >> status = "disabled"; >> }; >> + >> + /* unremovable emmc as mmcblk0 */ >> + sdio0: mmc@16010000 { >> + compatible = "starfive,jh7110-sdio"; >> + reg = <0x0 0x16010000 0x0 0x10000>; >> + clocks = <&syscrg JH7110_SYSCLK_SDIO0_AHB>, >> + <&syscrg JH7110_SYSCLK_SDIO0_SDCARD>; >> + clock-names = "biu","ciu"; >> + resets = <&syscrg JH7110_SYSRST_SDIO0_AHB>; >> + reset-names = "reset"; >> + interrupts = <74>; >> + fifo-depth = <32>; >> + fifo-watermark-aligned; >> + data-addr = <0>; >> + starfive,sys-syscon = <&sys_syscon 0x14 0x1a 0x7c000000>; > > This does not match your bindings at all. "&sys_syscon" is a phandle, > not a number of tuning retries, as you expect in your bindings. > Hi Krzysztof, The sys_syscon is one of system controller registers, and it is used to be the data strobe delay chain select. I will update its type and description in next version. Thanks, William Qiu > > Best regards, > Krzysztof >
On 2022/12/8 0:31, Conor Dooley wrote: > On Wed, Dec 07, 2022 at 04:14:53PM +0100, Krzysztof Kozlowski wrote: >> On 07/12/2022 14:17, William Qiu wrote: >> > This adds the mmc node for the StarFive JH7110 SoC. >> > Set sdioo node to emmc and set sdio1 node to sd. >> > >> > Signed-off-by: William Qiu <william.qiu@starfivetech.com> >> > --- >> > .../jh7110-starfive-visionfive-v2.dts | 25 ++++++++++++ >> > arch/riscv/boot/dts/starfive/jh7110.dtsi | 38 +++++++++++++++++++ >> > 2 files changed, 63 insertions(+) >> > >> > diff --git a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts >> > index c8946cf3a268..6ef8e303c2e6 100644 >> > --- a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts >> > +++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts >> > @@ -47,6 +47,31 @@ &clk_rtc { >> > clock-frequency = <32768>; >> > }; >> > >> > +&sdio0 { >> > + max-frequency = <100000000>; >> > + card-detect-delay = <300>; >> > + bus-width = <8>; >> > + cap-mmc-highspeed; >> > + mmc-ddr-1_8v; >> > + mmc-hs200-1_8v; >> > + non-removable; >> > + cap-mmc-hw-reset; >> > + post-power-on-delay-ms = <200>; >> > + status = "okay"; >> > +}; >> > + >> > +&sdio1 { >> > + max-frequency = <100000000>; >> > + card-detect-delay = <300>; >> > + bus-width = <4>; >> > + no-sdio; >> > + no-mmc; >> > + broken-cd; >> > + cap-sd-highspeed; >> > + post-power-on-delay-ms = <200>; >> > + status = "okay"; >> > +}; >> > + >> > &gmac0_rmii_refin { >> > clock-frequency = <50000000>; >> > }; >> > diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi >> > index c22e8f1d2640..e90b085d7e41 100644 >> > --- a/arch/riscv/boot/dts/starfive/jh7110.dtsi >> > +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi >> > @@ -331,6 +331,11 @@ aoncrg: clock-controller@17000000 { >> > #reset-cells = <1>; >> > }; >> > >> > + sys_syscon: sys_syscon@13030000 { >> >> No underscores in node names, generic node names (syscon or >> system-controller) >> >> > + compatible = "syscon"; >> >> This is not allowed alone. >> >> > + reg = <0x0 0x13030000 0x0 0x1000>; >> > + }; >> > + >> > gpio: gpio@13040000 { >> > compatible = "starfive,jh7110-sys-pinctrl"; >> > reg = <0x0 0x13040000 0x0 0x10000>; >> > @@ -433,5 +438,38 @@ uart5: serial@12020000 { >> > reg-shift = <2>; >> > status = "disabled"; >> > }; >> > + >> > + /* unremovable emmc as mmcblk0 */ >> > + sdio0: mmc@16010000 { >> > + compatible = "starfive,jh7110-sdio"; >> > + reg = <0x0 0x16010000 0x0 0x10000>; >> > + clocks = <&syscrg JH7110_SYSCLK_SDIO0_AHB>, >> > + <&syscrg JH7110_SYSCLK_SDIO0_SDCARD>; >> > + clock-names = "biu","ciu"; >> > + resets = <&syscrg JH7110_SYSRST_SDIO0_AHB>; >> > + reset-names = "reset"; >> > + interrupts = <74>; >> > + fifo-depth = <32>; >> > + fifo-watermark-aligned; >> > + data-addr = <0>; >> > + starfive,sys-syscon = <&sys_syscon 0x14 0x1a 0x7c000000>; >> >> This does not match your bindings at all. "&sys_syscon" is a phandle, >> not a number of tuning retries, as you expect in your bindings. > > Additionally, a Link: to the documentation for where-ever these "random" > numbers that are being used would be nice. > I will update in next version. > +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,sys-syscon", 3, 0, &args); > + if (ret) { > + dev_err(host->dev, "Failed to parse starfive,sys-syscon\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]; > > Given the driver, the property description just seems incorrect and this > is actually the bit of the syscon that is relevant to the tuning process > (perhaps where the find the tuning values?). Without public docs or a > better description it is hard for (me at least) to know :) > Hi Conor, Thank you for taking time to review and provide helpful comments for this patch. I'll make the description better and more detailed in next version. Best regards, William Qiu > + > + host->priv = priv; > + > + return 0; > +}
Hi William, thanks for your patch! On Wed, Dec 7, 2022 at 2:17 PM William Qiu <william.qiu@starfivetech.com> wrote: > + starfive,sys-syscon = <&sys_syscon 0x14 0x1a 0x7c000000>; (...) > + starfive,sys-syscon = <&sys_syscon 0x9c 0x1 0x3e>; These are registster offsets and shifts. Don't define such stuff in your device tree, use #defines in the code and just provide the phandle. Yours, Linus Walleij
On 2022/12/9 5:15, Linus Walleij wrote: > Hi William, > > thanks for your patch! > > On Wed, Dec 7, 2022 at 2:17 PM William Qiu <william.qiu@starfivetech.com> wrote: > >> + starfive,sys-syscon = <&sys_syscon 0x14 0x1a 0x7c000000>; > (...) >> + starfive,sys-syscon = <&sys_syscon 0x9c 0x1 0x3e>; > > These are registster offsets and shifts. Don't define such stuff in your device > tree, use #defines in the code and just provide the phandle. > Hi Linus, I will use #define in code instead in device tree. Thank you for taking time to review and provide helpful comments for this patch. Best Regards, William Qiu > Yours, > Linus Walleij
On 2022/12/9 5:15, Linus Walleij wrote: > Hi William, > > thanks for your patch! > > On Wed, Dec 7, 2022 at 2:17 PM William Qiu <william.qiu@starfivetech.com> wrote: > >> + starfive,sys-syscon = <&sys_syscon 0x14 0x1a 0x7c000000>; > (...) >> + starfive,sys-syscon = <&sys_syscon 0x9c 0x1 0x3e>; > > These are registster offsets and shifts. Don't define such stuff in your device > tree, use #defines in the code and just provide the phandle. > Hi Linus, I'm sorry to bother you, but as for the definition of syscon, after discussing with my colleagues, we think it is easier to distinguish SDIO0 and SDIO1 by defining it in the device tree, and the code compatibility is better. I expected to discuss this issue with you in V2,, but I am not sure whether you have seen V2, so I can only ask for your advice in V1. Best Regards William Qiu > Yours, > Linus Walleij
diff --git a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts index c8946cf3a268..6ef8e303c2e6 100644 --- a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts +++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts @@ -47,6 +47,31 @@ &clk_rtc { clock-frequency = <32768>; }; +&sdio0 { + max-frequency = <100000000>; + card-detect-delay = <300>; + bus-width = <8>; + cap-mmc-highspeed; + mmc-ddr-1_8v; + mmc-hs200-1_8v; + non-removable; + cap-mmc-hw-reset; + post-power-on-delay-ms = <200>; + status = "okay"; +}; + +&sdio1 { + max-frequency = <100000000>; + card-detect-delay = <300>; + bus-width = <4>; + no-sdio; + no-mmc; + broken-cd; + cap-sd-highspeed; + post-power-on-delay-ms = <200>; + status = "okay"; +}; + &gmac0_rmii_refin { clock-frequency = <50000000>; }; diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi index c22e8f1d2640..e90b085d7e41 100644 --- a/arch/riscv/boot/dts/starfive/jh7110.dtsi +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi @@ -331,6 +331,11 @@ aoncrg: clock-controller@17000000 { #reset-cells = <1>; }; + sys_syscon: sys_syscon@13030000 { + compatible = "syscon"; + reg = <0x0 0x13030000 0x0 0x1000>; + }; + gpio: gpio@13040000 { compatible = "starfive,jh7110-sys-pinctrl"; reg = <0x0 0x13040000 0x0 0x10000>; @@ -433,5 +438,38 @@ uart5: serial@12020000 { reg-shift = <2>; status = "disabled"; }; + + /* unremovable emmc as mmcblk0 */ + sdio0: mmc@16010000 { + compatible = "starfive,jh7110-sdio"; + reg = <0x0 0x16010000 0x0 0x10000>; + clocks = <&syscrg JH7110_SYSCLK_SDIO0_AHB>, + <&syscrg JH7110_SYSCLK_SDIO0_SDCARD>; + clock-names = "biu","ciu"; + resets = <&syscrg JH7110_SYSRST_SDIO0_AHB>; + reset-names = "reset"; + interrupts = <74>; + fifo-depth = <32>; + fifo-watermark-aligned; + data-addr = <0>; + starfive,sys-syscon = <&sys_syscon 0x14 0x1a 0x7c000000>; + status = "disabled"; + }; + + sdio1: mmc@16020000 { + compatible = "starfive,jh7110-sdio"; + reg = <0x0 0x16020000 0x0 0x10000>; + clocks = <&syscrg JH7110_SYSCLK_SDIO1_AHB>, + <&syscrg JH7110_SYSCLK_SDIO1_SDCARD>; + clock-names = "biu","ciu"; + resets = <&syscrg JH7110_SYSRST_SDIO1_AHB>; + reset-names = "reset"; + interrupts = <75>; + fifo-depth = <32>; + fifo-watermark-aligned; + data-addr = <0>; + starfive,sys-syscon = <&sys_syscon 0x9c 0x1 0x3e>; + status = "disabled"; + }; }; };
This adds the mmc node for the StarFive JH7110 SoC. Set sdioo node to emmc and set sdio1 node to sd. Signed-off-by: William Qiu <william.qiu@starfivetech.com> --- .../jh7110-starfive-visionfive-v2.dts | 25 ++++++++++++ arch/riscv/boot/dts/starfive/jh7110.dtsi | 38 +++++++++++++++++++ 2 files changed, 63 insertions(+)