Re: [PATCH v3 2/2] mmc: host: meson-mx-sdhc: new driver for the Amlogic Meson SDHC host

[Martin Blumenstingl]

Hi Ulf,

On Thu, Dec 19, 2019 at 3:02 PM Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
[...]
> > +struct meson_mx_sdhc_host {
> > +       struct mmc_host                 *mmc;
> > +
> > +       struct mmc_request              *mrq;
> > +       struct mmc_command              *cmd;
> > +       int                             error;
> > +
> > +       void __iomem                    *base;
> > +
> > +       struct clk_divider              clkc_clk_div;
> > +       struct clk_gate                 clkc_tx_clk_on;
> > +       struct clk_gate                 clkc_rx_clk_on;
> > +       struct clk_gate                 clkc_sd_clk_on;
> > +       struct clk_gate                 clkc_mod_clk_on;
> > +       struct clk_mux                  clkc_clk_src_sel;
> > +
> > +       struct clk                      *pclk;
> > +
> > +       struct clk                      *tx_clk;
> > +       struct clk                      *rx_clk;
> > +       struct clk                      *sd_clk;
> > +       struct clk                      *mod_clk;
>
> This is crazy. It's looks like the driver is a driver for a clock
> provider rather than an mmc host driver. :-)
that's the signature of many Amlogic IP blocks:
they build a clock mux/divider/gate into the actual consumer IP block

> Can you please elaborate a bit on what all these different clocks are
> needed for? Are really all of them needed?
actually I don't know - the public S805 datasheet doesn't list these bits
the vendor driver just calls the first three TX_CLK, RX_CLK, SD_CLK
(that's bits[14:12])
the last one (mod_clk) is documented as:
  Clock Module Enable, Should set before bit[14:12] switch on,
  and after bit[14:12] switch off

so all I know is that the order is important

[...]
> > +static void meson_mx_sdhc_wait_cmd_ready(struct mmc_host *mmc)
> > +{
> > +       struct meson_mx_sdhc_host *host = mmc_priv(mmc);
> > +       u32 stat, esta;
> > +       int ret;
> > +
> > +       ret = readl_poll_timeout(host->base + MESON_SDHC_STAT, stat,
> > +                                !(stat & MESON_SDHC_STAT_CMD_BUSY), 1,
> > +                                100000);
>
> This looks quite inefficient. Isn't there a corresponding IRQ that you
> can look for instead, no?
not that I am aware of :-(

> Can you perhaps also elaborate a bit on what kind of busy check you
> are doing here? Maybe also add some comment about that in the code.
the vendor driver documents this: [2]
  wait sdhc controller cmd send
and the public S805 datasheet states:
  (0:Ready for command,1:busy)

> > +       if (ret) {
> > +               dev_warn(mmc_dev(mmc),
> > +                        "Failed to poll for CMD_BUSY while processing CMD%d\n",
> > +                        host->cmd->opcode);
> > +               meson_mx_sdhc_hw_reset(mmc);
> > +       }
> > +
> > +       ret = readl_poll_timeout(host->base + MESON_SDHC_ESTA, esta,
> > +                                !(esta & MESON_SDHC_ESTA_11_13), 1, 100000);
>
> Another polling. Not possible to wait for an IRQ instead?
unfortunately this is another "not that I'm aware of"

> What are the polling about?
and I don't know yet again :-(

> > +       if (ret) {
> > +               dev_warn(mmc_dev(mmc),
> > +                        "Failed to poll for ESTA[13:11] while processing CMD%d\n",
> > +                        host->cmd->opcode);
>
> What is ESTA[13:11]? If you are going to log a message, please make it
> more understandable.
the only other explanation I found in the vendor driver (which I used
as reference) is this error print:
  sdhc_err("Warning: DMA state is wrong! SDHC_ESTA=0x%x\n", esta);

[...]
> > +static void meson_mx_sdhc_disable_clks(struct mmc_host *mmc)
> > +{
> > +       struct meson_mx_sdhc_host *host = mmc_priv(mmc);
> > +
> > +       if (!host->clocks_enabled)
> > +               return;
> > +
> > +       clk_disable_unprepare(host->tx_clk);
> > +       clk_disable_unprepare(host->rx_clk);
> > +       clk_disable_unprepare(host->sd_clk);
> > +
> > +       clk_disable_unprepare(host->mod_clk);
>
> clk_bulk_disable_unprepare() seems like a better option to use here.
good point, thank you
I'll update that while keeping the order of "mod_clk" right (see [0] and [1])

[...]
> > +static int meson_mx_sdhc_enable_clks(struct mmc_host *mmc)
> > +{
> > +       struct meson_mx_sdhc_host *host = mmc_priv(mmc);
> > +       struct clk *clocks[] = {
> > +               host->mod_clk,
> > +               host->sd_clk,
> > +               host->tx_clk,
> > +               host->rx_clk,
> > +       };
> > +       int i, ret;
> > +
> > +       if (host->clocks_enabled)
> > +               return 0;
> > +
> > +       for (i = 0; i < ARRAY_SIZE(clocks); i++) {
> > +               ret = clk_prepare_enable(clocks[i]);
> > +               if (ret) {
> > +                       dev_err(mmc_dev(mmc), "Failed to enable clock %s\n",
> > +                               __clk_get_name(clocks[i]));
> > +                       goto err;
> > +               }
> > +       }
>
> clk_bulk_prepare_enable() seems like a better option to use here.
ACK, same as above

[...]
> > +static int meson_mx_sdhc_set_clk(struct mmc_host *mmc, struct mmc_ios *ios)
> > +{
> > +       struct meson_mx_sdhc_host *host = mmc_priv(mmc);
> > +       u32 rx_clk_phase, val;
> > +       int ret;
> > +
> > +       meson_mx_sdhc_disable_clks(mmc);
> > +
> > +       if (ios->clock) {
> > +               ret = clk_set_rate(host->sd_clk, ios->clock);
> > +               if (ret) {
> > +                       dev_warn(mmc_dev(mmc),
> > +                                "Failed to set MMC clock to %uHz: %d\n",
> > +                                ios->clock, host->error);
> > +                       return ret;
> > +               }
> > +
> > +               ret = meson_mx_sdhc_enable_clks(mmc);
> > +               if (ret)
> > +                       return ret;
> > +
> > +               mmc->actual_clock = clk_get_rate(host->sd_clk);
> > +
> > +               /*
> > +                * according to Amlogic the following latching points are
> > +                * selected with empirical values, there is no (known) formula
> > +                * to calculate these.
> > +                */
> > +               if (mmc->actual_clock > 100000000) {
> > +                       rx_clk_phase = 1;
> > +               } else if (mmc->actual_clock > 45000000) {
> > +                       if (ios->signal_voltage == MMC_SIGNAL_VOLTAGE_330)
> > +                               rx_clk_phase = 15;
> > +                       else
> > +                               rx_clk_phase = 11;
> > +               } else if (mmc->actual_clock >= 25000000) {
> > +                       rx_clk_phase = 15;
> > +               } else if (mmc->actual_clock > 5000000) {
> > +                       rx_clk_phase = 23;
> > +               } else if (mmc->actual_clock > 1000000) {
> > +                       rx_clk_phase = 55;
> > +               } else {
> > +                       rx_clk_phase = 1061;
> > +               }
>
> The latching points (in frequency) looks closely related to the bus
> speed timing frequency.
>
> Perhaps that doesn't matter, but I am wondering whether you may want
> to check "ios.timing" in conjunction with the clock rate?
the Amlogic internal datasheet extract I got has them in a table with
the clock settings (mux, divider). I rephrased this table into my own
words - you can find it in the patch description
what's implemented here is basically a translation of the datasheet into C
if it's more consistent with the Linux MMC framework to check for
"ios.timing" (either only checking the timing, or checking it in
addition to the bus speed) then please let me know.

[...]
> > +static struct clk *meson_mx_sdhc_register_clk(struct device *dev,
> > +                                             struct clk_hw *hw,
> > +                                             const char *name,
> > +                                             int num_parents,
> > +                                             const struct clk_parent_data *pd,
> > +                                             unsigned long flags,
> > +                                             const struct clk_ops *ops)
> > +{
> > +       struct clk_init_data init;
> > +
> > +       init.name = devm_kasprintf(dev, GFP_KERNEL, "%s#%s", dev_name(dev),
> > +                                  name);
> > +       if (!init.name)
> > +               return ERR_PTR(-ENOMEM);
> > +
> > +       init.num_parents = num_parents;
> > +       init.parent_data = pd;
> > +       init.flags = flags;
> > +       init.ops = ops;
> > +
> > +       hw->init = &init;
> > +
> > +       return devm_clk_register(dev, hw);
>
> devm_clk_register() is deprecated. Please convert to
> devm_clk_hw_register() instead.
>
> Note that, this may also lead to that you need to update the
> corresponding DT bindings for the clocks.
>
> Additionally, in regards to the deployment of the clock support, this
> leads to quite some more changes. In principle, the code managing the
> clock provider parts should be quite independent of the clock consumer
> part. I didn't look more closely, maybe this is already the case, then
> the conversion is simple.
just to confirm I understand you correctly:
the dt-bindings should look similar to what others have been working
on for the arasan SDHCI controller: [3]
based on that I'll then go forward and implement the driver part(s)

[...]
> Besides the above comments, I think overall the code looks quite okay to me.
great, thank you for taking the time to review this driver!


Martin


[0] https://github.com/endlessm/linux-meson/blob/d6e13c220931110fe676ede6da69fc61a7cb04b6/drivers/amlogic/mmc/aml_sdhc_m8.c#L1911
[1] https://github.com/endlessm/linux-meson/blob/d6e13c220931110fe676ede6da69fc61a7cb04b6/drivers/amlogic/mmc/aml_sdhc_m8.c#L1933
[2] https://github.com/endlessm/linux-meson/blob/d6e13c220931110fe676ede6da69fc61a7cb04b6/drivers/amlogic/mmc/aml_sdhc_m8.c#L577
[3] https://lkml.org/lkml/2019/7/1/27