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

Gabriel L. Somlo
2021-12-07T14:10:24Z

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

Good idea, done and ready for v3.

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