Re: [PATCH v2 3/3] mmc: Add driver for LiteX's LiteSDCard interface

Geert Uytterhoeven
2021-12-06T12:25:06Z

Hi Gabriel,

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>

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

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