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

["Gabriel L. Somlo" <gsomlo@gmail.com>]

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