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
