Hi Gabriel,

On Sat, Dec 4, 2021 at 9:41 PM Gabriel Somlo [off-list ref] 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 <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>

+static void
+litex_set_clk(struct litex_mmc_host *host, unsigned int clk_freq)
+{
+       u32 div = clk_freq ? host->freq / clk_freq : 256;
+
+       div = roundup_pow_of_two(div);
+       div = min_t(u32, max_t(u32, div, 2), 256);
+       dev_info(&host->dev->dev, "sdclk_freq=%d: set to %d via div=%d\n",
+                clk_freq, host->freq / div, div);
+       litex_write16(host->sdphy + LITEX_PHY_CLOCKERDIV, div);
+}
+
+static void
+litex_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
+{
+       struct litex_mmc_host *host = mmc_priv(mmc);
+
+       /* NOTE: Ignore any ios->bus_width updates; they occur right after
+        * the mmc core sends its own acmd6 bus-width change notification,
+        * which is redundant since we snoop on the command flow and inject
+        * an early acmd6 before the first data transfer command is sent!
+        */
+
+       /* update sdcard clock */
+       if (ios->clock != host->clock) {
+               litex_set_clk(host, ios->clock);
+               host->clock = ios->clock;

It might be better to move the assignment to host->clock into the callee,
to avoid it becoming out-of-sync when a second caller is introduced.

+       }
+}

+static int
+litex_mmc_probe(struct platform_device *pdev)
+{
+       struct litex_mmc_host *host;
+       struct mmc_host *mmc;
+       struct device_node *cpu;
+       int ret;
+
+       mmc = mmc_alloc_host(sizeof(struct litex_mmc_host), &pdev->dev);
+       /* NOTE: defaults to max_[req,seg]_size=PAGE_SIZE, max_blk_size=512,
+        * and max_blk_count accordingly set to 8;
+        * If for some reason we need to modify max_blk_count, we must also
+        * re-calculate `max_[req,seg]_size = max_blk_size * max_blk_count;`
+        */
+       if (!mmc)
+               return -ENOMEM;
+
+       host = mmc_priv(mmc);
+       host->mmc = mmc;
+       host->dev = pdev;
+
+       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?


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