Hi Geert,

Thanks for the feedback! Some replies and follow-up questions inline,
below:

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

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

--- /dev/null
+++ b/drivers/mmc/host/litex_mmc.c

quoted

+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.

Good idea, done and ready for v3.

quoted

+       }
+}

quoted

+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?

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.

Thanks,
--Gabriel