Hi Gabriel,

On Tue, Dec 7, 2021 at 10:30 PM Gabriel L. Somlo [off-list ref] wrote:

On Tue, Dec 07, 2021 at 03:16:22PM +0100, Geert Uytterhoeven wrote:

quoted

On Tue, Dec 7, 2021 at 3:10 PM Gabriel L. Somlo [off-list ref] wrote:

quoted

On Mon, Dec 06, 2021 at 01:24:49PM +0100, Geert Uytterhoeven wrote:

quoted

On Sat, Dec 4, 2021 at 9:41 PM Gabriel Somlo [off-list ref] wrote:

quoted

LiteX (https://github.com/enjoy-digital/litex) is a SoC framework
that targets FPGAs. LiteSDCard is a small footprint, configurable
SDCard core commonly used in LiteX designs.

The driver was first written in May 2020 and has been maintained
cooperatively by the LiteX community. Thanks to all contributors!

Co-developed-by: Kamil Rakoczy <redacted>
Signed-off-by: Kamil Rakoczy <redacted>
Co-developed-by: Maciej Dudek <redacted>
Signed-off-by: Maciej Dudek <redacted>
Co-developed-by: Paul Mackerras <redacted>
Signed-off-by: Paul Mackerras <redacted>
Signed-off-by: Gabriel Somlo <gsomlo@gmail.com>

quoted

quoted

quoted

+       host->clock = 0;
+       cpu = of_get_next_cpu_node(NULL);
+       ret = of_property_read_u32(cpu, "clock-frequency", &host->freq);
+       of_node_put(cpu);
+       if (ret) {
+               dev_err(&pdev->dev, "No \"clock-frequency\" property in DT\n");
+               goto err_free_host;
+       }

This looks fragile.
Shouldn't the clock be obtained from a clock property in the mmc
device node, pointing to a clock provider?
How does the real clock tree look like?

In a full LiteX SoC, the main sys_clock is used for cpu, buses, and as a
input source for peripherals such as LiteSDCard (which then further
subdivides it to obtain a 12.5--50.0 MHz sd_clock.

But since we're considering supporting LiteSDCard as an independent IP
block, the "source clock" frequency should indeed be specified as a DT
property in the MMC device node. (I'll have to add that to the list of
updates for litex_json2dts_linux.py as well, once we settle on what it
will look like -- I'll try to make the change and corresponding update
to the devicetree bindings doc for v3).

LMK what you think.

Ideally there should be a "clocks" property with a phandle pointing to a
clock controller node (compatible with "litex,clk").

How does drivers/tty/serial/liteuart.c handle this? Oh, it doesn't ;-)

Assuming LiteX's `litex_json2dts_linux.py` is modified to include:

        ...
        clocks {
                sys_clk: litex_sys_clk {
                        #clock-cells = <0>;
                        compatible = "fixed-clock";
                        clock-frequency = <50000000>;
                };
        };
        ...

in the generated .dts (where `clock-frequency` is whatever the sys_clk
happens to be for that particular SoC gateware), we can then write the
mmc node like so:

        soc {
                ...
                mmc0: mmc@12005000 {
                        compatible = "litex,mmc";
                        reg = <0x12005000 0x100>,
                                <0x12003800 0x100>,
                                <0x12003000 0x100>,
                                <0x12004800 0x100>,
                                <0x12004000 0x100>;
                        reg-names = "phy", "core", "reader", "writer", "irq";
                        clocks = <&sys_clk>;
                        interrupt-parent = <&L1>;
                        interrupts = <4>;
                };
                ...
        };

LGTM.

The LiteSDCard clock initialization can then look like this:

        ...
        /* initialize clock source */
        clk = devm_clk_get(&pdev->dev, NULL);
        if (IS_ERR(clk)) {
                ret = PTR_ERR(clk);
                dev_err(&pdev->dev, "can't get clock: %d\n", ret);

Please use

    ret = dev_err_probe(&pdev->dev, PTR_ERR(clk), "can't get clock\n");

to avoid printing the error in case of probe deferral.

                goto err;
        }
        host->freq = clk_get_rate(clk); /* source (sys_clock) frequency */
        host->clock = 0;                /* calculated sdclock frequency */
        ...

As discussed before, I'll post a `litex_json2dts_linux.py` patch once
we've settled on a mutually agreeable solution, but I think this might
be it. If you agree, I'll also update the DT binding document to
require `clocks` as part of the mmc node.

I tested this in my v3 candidate, and it seems to be working OK.

LMK what you think, and/or if you have any suggestions for additional
improvements.

Nice, that's a good solution, until the full clock tree is exposed in
the DTS. Thanks a lot!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds