[U-Boot] [RFC PATCH 03/11] mmc: Add support for Qualcomm SDHCI controller

[Simon Glass]

Hi Mateusz,

On 16 December 2015 at 15:46, Mateusz Kulikowski
<mateusz.kulikowski at gmail.com> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA256
>
> Hi Simon,
>
> On 15.12.2015 19:58, Simon Glass wrote:
>> Hi Mateusz,
>>
>> On 10 December 2015 at 14:41, Mateusz Kulikowski
>> <mateusz.kulikowski at gmail.com> wrote:
> [...]
>>> +DECLARE_GLOBAL_DATA_PTR;
>>> +
>>> +static int msm_sdc_clk_init(struct udevice *dev)
>>> +{
>>> +       uint clk_rate = fdtdec_get_uint(gd->fdt_blob, dev->of_offset,
>>> +                                       "clock-frequency", 400000);
>>> +       uint clkd[2]; /* clk_id and clk_no */
>>> +       fdtdec_get_int_array(gd->fdt_blob, dev->of_offset, "clock", clkd, 2);
>>> +       clkd[0] = fdt_node_offset_by_phandle(gd->fdt_blob, clkd[0]);
>>> +
>>> +       struct udevice *clk = NULL;
>>> +       uclass_get_device_by_of_offset(UCLASS_CLK, clkd[0], &clk);
>>> +       if (clk)
>>> +               clk_set_periph_rate(clk, clkd[1], clk_rate);
>>> +
>>
>> See comments on the previous patch. Also you could move the DT decode
>> code all into your ofdata_to_platdata function (if you like).
>
> But requesting clock must be handled in probe (i.e. when clock is
> already bound) right?

Yes. Although bear in mind that the ofdata_to_platdata() call is made
just before probe() so it is OK to put 'get' calls in there. In
general all devices are bound before any are probed.

>
>>
>>> +       return 0;
>>> +}
>>> +
>>> +static int msm_sdc_probe(struct udevice *dev)
>>> +{
>>> +       struct msm_sdhc *prv = dev_get_priv(dev);
>>> +       struct sdhci_host *host = &prv->host;
>>> +       u32 core_version, core_minor, core_major;
>>> +
>>> +       host->quirks = SDHCI_QUIRK_WAIT_SEND_CMD | SDHCI_QUIRK_BROKEN_R1B;
>>> +
>>> +       /* Init clocks */
>>> +       if (msm_sdc_clk_init(dev))
>>> +               return -EIO;
>>> +
>>> +       /* Reset the core and Enable SDHC mode */
>>> +       writel(readl(prv->base + SDCC_MCI_POWER) | SDCC_MCI_POWER_SW_RST,
>>> +              prv->base + SDCC_MCI_POWER);
>>> +
>>> +       /* SW reset can take upto 10HCLK + 15MCLK cycles. (min 40us) */
>>> +       mdelay(2);
>>
>> So why such a long delay? Perhaps you should put the code immediately
>> below int a timeout loop?
>>
>> start = get_timer(0);
>> while (readl...) {
>>    if (get_timer(start) > 2)
>>       return -ETIMEDOUT;
>> }
>>
> I'm not sure if I can do that - I can test it, perhaps it will even work on
> my board, but it was put explicitly in Linux driver (1-5ms delay).
>
> Documentation says:
> "SW should wait until bit 0 (MCLK_REG_WR_ACTIVE) of MCI_STATUS2 register
> is 0, after writing to MCI_POWER register and before accessing this
> register again."
>
> But this applies to all writes, not to reset request - I'm not sure
> how MCI_STATUS2 will behave during reset (it may get zeroed earlier).
>
> Do you really think I should try to decrease this delay?

Can you change the code to wait until bit 0 is 0, instead of having a delay?

>
> [...]
>>> +}
>>> +
>>> +static int msm_ofdata_to_platdata(struct udevice *dev)
>>> +{
>>> +       struct msm_sdhc *priv = dev_get_priv(dev);
>>> +       struct sdhci_host *host = &priv->host;
>>> +
>>> +       host->name = strdup(dev->name);
>>> +       host->ioaddr = (void *)dev_get_addr(dev);
>>> +       host->bus_width = fdtdec_get_int(gd->fdt_blob, dev->of_offset,
>>> +                                        "bus-width", 4);
>>> +       host->index = fdtdec_get_uint(gd->fdt_blob, dev->of_offset, "index", 0);
>>> +       priv->base = fdtdec_get_addr_size_auto_parent(gd->fdt_blob,
>>> +                                                     dev->parent->of_offset,
>>> +                                                     dev->of_offset, "reg",
>>> +                                                     1, NULL);
>>
>> Odd that you are reading the second cell. How come?
>
> I tried to use as much parts of original qcom Linux dts as possible.
> For mmc controller, there are 2 addresses needed:
> - - SDCC base (i.e. base of IP block with vendor-specific regs etc.)
> - - SDHCI base (i.e. place where SDHCI "standard" registers start)
>
> My assumption is that SDHCI offset may be different on different
> SoCs, and wanted to have generic driver.

OK I see, thanks.

Regards,
Simon