Message-ID: <Ya/SbwzYUD3gwwOL@errol.ini.cmu.edu>
Date:   Tue, 7 Dec 2021 16:30:23 -0500
From:   "Gabriel L. Somlo" <gsomlo@...il.com>
To:     Geert Uytterhoeven <geert@...ux-m68k.org>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Rob Herring <robh+dt@...nel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>, Ulf Hansson <ulf.hansson@...aro.org>,
        Linux MMC List <linux-mmc@...r.kernel.org>,
        Karol Gugala <kgugala@...micro.com>,
        Mateusz Holenko <mholenko@...micro.com>,
        Kamil Rakoczy <krakoczy@...micro.com>,
        mdudek@...ernships.antmicro.com,
        Paul Mackerras <paulus@...abs.org>,
        Joel Stanley <joel@....id.au>,
        Stafford Horne <shorne@...il.com>,
        david.abdurachmanov@...ive.com,
        Florent Kermarrec <florent@...oy-digital.fr>,
        Randy Dunlap <rdunlap@...radead.org>
Subject: Re: [PATCH v2 3/3] mmc: Add driver for LiteX's LiteSDCard interface

Hi Geert,

I think I figured out a solution, see below:

On Tue, Dec 07, 2021 at 03:16:22PM +0100, Geert Uytterhoeven wrote:
> On Tue, Dec 7, 2021 at 3:10 PM Gabriel L. Somlo <gsomlo@...il.com> wrote:
> > On Mon, Dec 06, 2021 at 01:24:49PM +0100, Geert Uytterhoeven wrote:
> > > On Sat, Dec 4, 2021 at 9:41 PM Gabriel Somlo <gsomlo@...il.com> wrote:
> > > > 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 <krakoczy@...micro.com>
> > > > Signed-off-by: Kamil Rakoczy <krakoczy@...micro.com>
> > > > Co-developed-by: Maciej Dudek <mdudek@...ernships.antmicro.com>
> > > > Signed-off-by: Maciej Dudek <mdudek@...ernships.antmicro.com>
> > > > Co-developed-by: Paul Mackerras <paulus@...abs.org>
> > > > Signed-off-by: Paul Mackerras <paulus@...abs.org>
> > > > Signed-off-by: Gabriel Somlo <gsomlo@...il.com>
> 
> > > > +       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@...05000 {
                        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>;
                };
		...
        };

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);
                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.

Thanks again,
--Gabriel

Powered by blists - more mailing lists