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@xxxxxxxxx> 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@xxxxxxxxxxxx> > > Signed-off-by: Kamil Rakoczy <krakoczy@xxxxxxxxxxxx> > > Co-developed-by: Maciej Dudek <mdudek@xxxxxxxxxxxxxxxxxxxxxxxx> > > Signed-off-by: Maciej Dudek <mdudek@xxxxxxxxxxxxxxxxxxxxxxxx> > > Co-developed-by: Paul Mackerras <paulus@xxxxxxxxxx> > > Signed-off-by: Paul Mackerras <paulus@xxxxxxxxxx> > > Signed-off-by: Gabriel Somlo <gsomlo@xxxxxxxxx> > > > --- /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
