lkml.org 
[lkml]   [2021]   [Dec]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v2 3/3] mmc: Add driver for LiteX's LiteSDCard interface
Hi Gabriel,

On Tue, Dec 7, 2021 at 10:30 PM Gabriel L. Somlo <gsomlo@gmail.com> wrote:
> 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@gmail.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@gmail.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@antmicro.com>
> > > > > Signed-off-by: Kamil Rakoczy <krakoczy@antmicro.com>
> > > > > Co-developed-by: Maciej Dudek <mdudek@internships.antmicro.com>
> > > > > Signed-off-by: Maciej Dudek <mdudek@internships.antmicro.com>
> > > > > Co-developed-by: Paul Mackerras <paulus@ozlabs.org>
> > > > > Signed-off-by: Paul Mackerras <paulus@ozlabs.org>
> > > > > Signed-off-by: Gabriel Somlo <gsomlo@gmail.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@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

\
 
 \ /
  Last update: 2021-12-08 10:36    [W:0.066 / U:0.232 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site