[PATCH v2 2/2] mmc: host: sunxi: add support for A64 mmc controller

Hans de Goede hdegoede at redhat.com
Mon Aug 1 00:29:40 PDT 2016


Hi,

On 01-08-16 01:48, Icenowy Zheng wrote:
>
> Hi,
> 31.07.2016, 22:30, "Hans de Goede" <hdegoede at redhat.com>:
>> Hi,
>>
>> On 31-07-16 13:02, Icenowy Zheng wrote:
>>>  A64 SoC features a MMC controller which need only the mod clock, and can
>>>  calibrate delay by itself. This patch adds support for the new MMC
>>>  controller IP core.
>>>
>>>  Based on work by Andre Przywara <andre.przywara at arm.com>.
>>>
>>>  Signed-off-by: Icenowy Zheng <icenowy at aosc.xyz>
>>
>> Looks good, some minor remarks (see comments inline) after those
>> are addressed this is ready to be merged IMHO.
>>
>>>  ---
>>>   drivers/mmc/host/sunxi-mmc.c | 106 ++++++++++++++++++++++++++++++++++++-------
>>>   1 file changed, 90 insertions(+), 16 deletions(-)
>>>
>>>  diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
>>>  index 2ec91ce..aa2abf3 100644
>>>  --- a/drivers/mmc/host/sunxi-mmc.c
>>>  +++ b/drivers/mmc/host/sunxi-mmc.c
>>>  @@ -72,6 +72,14 @@
>>>   #define SDXC_REG_CHDA (0x90)
>>>   #define SDXC_REG_CBDA (0x94)
>>>
>>>  +/* New registers introduced in A64 */
>>>  +#define SDXC_REG_A12A 0x058 /* SMC Auto Command 12 Register */
>>>  +#define SDXC_REG_SD_NTSR 0x05C /* SMC New Timing Set Register */
>>>  +#define SDXC_REG_DRV_DL 0x140 /* Drive Delay Control Register */
>>>  +#define SDXC_REG_SAMP_DL_REG 0x144 /* SMC sample delay control */
>>>  +#define SDXC_REG_DS_DL_REG 0x148 /* SMC data strobe delay control */
>>>  +
>>>  +
>>
>> Please drop 1 empty line here.
> Thanks for you notice... I forgot to checkout the empty line...
>>
>>>   #define mmc_readl(host, reg) \
>>>           readl((host)->reg_base + SDXC_##reg)
>>>   #define mmc_writel(host, reg, value) \
>>>  @@ -217,6 +225,15 @@
>>>   #define SDXC_CLK_50M_DDR 3
>>>   #define SDXC_CLK_50M_DDR_8BIT 4
>>>
>>>  +#define SDXC_2X_TIMING_MODE BIT(31)
>>>  +
>>>  +#define SDXC_CAL_START BIT(15)
>>>  +#define SDXC_CAL_DONE BIT(14)
>>>  +#define SDXC_CAL_DL_SHIFT 8
>>>  +#define SDXC_CAL_DL_SW_EN BIT(7)
>>>  +#define SDXC_CAL_DL_SW_SHIFT 0
>>>  +#define SDXC_CAL_DL_MASK 0x3f
>>>  +
>>>   struct sunxi_mmc_clk_delay {
>>>           u32 output;
>>>           u32 sample;
>>>  @@ -232,6 +249,9 @@ struct sunxi_idma_des {
>>>   struct sunxi_mmc_cfg {
>>>           u32 idma_des_size_bits;
>>>           const struct sunxi_mmc_clk_delay *clk_delays;
>>>  +
>>
>> I would not insert an empty line here.
> I'm only feeling that a empty before such a comment line is a
> kind of separator...
> (Refer to struct sunxi_mmc_host)

Ok.

>>
>>>  + /* does the IP block support autocalibration? */
>>>  + bool can_calibrate;
>>>   };
>>>
>>>   struct sunxi_mmc_host {
>>>  @@ -657,6 +677,34 @@ static int sunxi_mmc_oclk_onoff(struct sunxi_mmc_host *host, u32 oclk_en)
>>>           return 0;
>>>   }
>>>
>>>  +static int sunxi_mmc_calibrate(struct sunxi_mmc_host *host,
>>>  + struct mmc_ios *ios, int reg_off)
>>>  +{
>>>  + u32 reg = readl(host->reg_base + reg_off);
>>>  + u32 delay;
>>>  +
>>
>> I would add:
>>
>>         if (!host->cfg->can_calibrate)
>>                 return 0;
>>
>> Here; and ...
> I agree with it. It's some kind of error checking.
>>
>>>  + reg &= ~(SDXC_CAL_DL_MASK << SDXC_CAL_DL_SW_SHIFT);
>>>  + reg &= ~SDXC_CAL_DL_SW_EN;
>>>  +
>>>  + writel(reg | SDXC_CAL_START, host->reg_base + reg_off);
>>>  +
>>>  + dev_dbg(mmc_dev(host->mmc), "calibration started\n");
>>>  +
>>>  + while (!((reg = readl(host->reg_base + reg_off)) & SDXC_CAL_DONE))
>>>  + cpu_relax();
>>>  +
>>>  + delay = (reg >> SDXC_CAL_DL_SHIFT) & SDXC_CAL_DL_MASK;
>>>  +
>>>  + reg &= ~SDXC_CAL_START;
>>>  + reg |= (delay << SDXC_CAL_DL_SW_SHIFT) | SDXC_CAL_DL_SW_EN;
>>>  +
>>>  + writel(reg, host->reg_base + reg_off);
>>>  +
>>>  + dev_dbg(mmc_dev(host->mmc), "calibration ended, res is 0x%x\n", reg);
>>
>> Add:
>>
>>         /* TODO: enable calibrate on sdc2 SDXC_REG_DS_DL_REG of A64 */
>>
>> here; and ...
> The function only received a register address as a parameter...
> It do not know the difference between different regs to calibrate.

We can change the parameters the function gets, or even introduce
a new function once we fix the TODO.

For now we just need a place to put the TODO and this seems like
the best place.

>>
>>>  +
>>>  + return 0;
>>>  +}
>>>  +
>>>   static int sunxi_mmc_clk_set_phase(struct sunxi_mmc_host *host,
>>>                                      struct mmc_ios *ios, u32 rate)
>>>   {
>>>  @@ -727,9 +775,17 @@ static int sunxi_mmc_clk_set_rate(struct sunxi_mmc_host *host,
>>>           }
>>>           mmc_writel(host, REG_CLKCR, rval);
>>>
>>>  - ret = sunxi_mmc_clk_set_phase(host, ios, rate);
>>>  - if (ret)
>>>  - return ret;
>>
>> Keep this as is; and add:
>>
>>                 ret = sunxi_mmc_calibrate(host, ios, SDXC_REG_SAMP_DL_REG);
>>                 if (ret)
>>                         return ret;
>>
> It's a good idea :-)
>> Instead of:
>>
>>>  + if (host->cfg->can_calibrate) {
>>>  + ret = sunxi_mmc_calibrate(host, ios, SDXC_REG_SAMP_DL_REG);
>>>  + if (ret)
>>>  + return ret;
>>>  +
>>>  + /* TODO: enable calibrate on sdc2 SDXC_REG_DS_DL_REG of A64 */
>>>  + } else {
>>>  + ret = sunxi_mmc_clk_set_phase(host, ios, rate);
>>>  + if (ret)
>>>  + return ret;
>>>  + }
>>>
>>>           return sunxi_mmc_oclk_onoff(host, 1);
>>>   }
>>>  @@ -982,21 +1038,31 @@ static const struct sunxi_mmc_clk_delay sun9i_mmc_clk_delays[] = {
>>>   static const struct sunxi_mmc_cfg sun4i_a10_cfg = {
>>>           .idma_des_size_bits = 13,
>>>           .clk_delays = NULL,
>>>  + .can_calibrate = false,
>>>   };
>>>
>>>   static const struct sunxi_mmc_cfg sun5i_a13_cfg = {
>>>           .idma_des_size_bits = 16,
>>>           .clk_delays = NULL,
>>>  + .can_calibrate = false,
>>>   };
>>>
>>>   static const struct sunxi_mmc_cfg sun7i_a20_cfg = {
>>>           .idma_des_size_bits = 16,
>>>           .clk_delays = sunxi_mmc_clk_delays,
>>>  + .can_calibrate = false,
>>>   };
>>>
>>>   static const struct sunxi_mmc_cfg sun9i_a80_cfg = {
>>>           .idma_des_size_bits = 16,
>>>           .clk_delays = sun9i_mmc_clk_delays,
>>>  + .can_calibrate = false,
>>>  +};
>>>  +
>>>  +static const struct sunxi_mmc_cfg sun50i_a64_cfg = {
>>>  + .idma_des_size_bits = 16,
>>>  + .clk_delays = NULL,
>>>  + .can_calibrate = true,
>>>   };
>>>
>>>   static const struct of_device_id sunxi_mmc_of_match[] = {
>>>  @@ -1004,6 +1070,7 @@ static const struct of_device_id sunxi_mmc_of_match[] = {
>>>           { .compatible = "allwinner,sun5i-a13-mmc", .data = &sun5i_a13_cfg },
>>>           { .compatible = "allwinner,sun7i-a20-mmc", .data = &sun7i_a20_cfg },
>>>           { .compatible = "allwinner,sun9i-a80-mmc", .data = &sun9i_a80_cfg },
>>>  + { .compatible = "allwinner,sun50i-a64-mmc", .data = &sun50i_a64_cfg },
>>>           { /* sentinel */ }
>>>   };
>>>   MODULE_DEVICE_TABLE(of, sunxi_mmc_of_match);
>>
>> Not sure why all the below hunks are here, they certainly do not belong
>> in this patch; and they are not necessary. As I mentioned in the
>> commit msg of "mmc: sunxi: sun4i / sun5i do not have sample clocks" :
>>
>>     "Note this patch leaves the clk_prepare_enable() / clk_disable_unprepare()
>>      calls to the sample clks as-is, without adding checks for them being
>>      NULL. All the clk_foo calls accept a NULL clk and will return success when
>>      called with a NULL clk."
>>
>> So please drop these.
>>
> Thanks! I will drop these!
> (I don't know that the clk functions can accept NULL...)

Great.

Regards,

Hans



>
>> Thanks & Regards,
>>
>> Hans
>>
>>>  @@ -1071,16 +1138,18 @@ static int sunxi_mmc_resource_request(struct sunxi_mmc_host *host,
>>>                   goto error_disable_clk_ahb;
>>>           }
>>>
>>>  - ret = clk_prepare_enable(host->clk_output);
>>>  - if (ret) {
>>>  - dev_err(&pdev->dev, "Enable output clk err %d\n", ret);
>>>  - goto error_disable_clk_mmc;
>>>  - }
>>>  + if (host->cfg->clk_delays) {
>>>  + ret = clk_prepare_enable(host->clk_output);
>>>  + if (ret) {
>>>  + dev_err(&pdev->dev, "Enable output clk err %d\n", ret);
>>>  + goto error_disable_clk_mmc;
>>>  + }
>>>
>>>  - ret = clk_prepare_enable(host->clk_sample);
>>>  - if (ret) {
>>>  - dev_err(&pdev->dev, "Enable sample clk err %d\n", ret);
>>>  - goto error_disable_clk_output;
>>>  + ret = clk_prepare_enable(host->clk_sample);
>>>  + if (ret) {
>>>  + dev_err(&pdev->dev, "Enable sample clk err %d\n", ret);
>>>  + goto error_disable_clk_output;
>>>  + }
>>>           }
>>>
>>>           if (!IS_ERR(host->reset)) {
>>>  @@ -1107,9 +1176,11 @@ error_assert_reset:
>>>           if (!IS_ERR(host->reset))
>>>                   reset_control_assert(host->reset);
>>>   error_disable_clk_sample:
>>>  - clk_disable_unprepare(host->clk_sample);
>>>  + if (host->cfg->clk_delays)
>>>  + clk_disable_unprepare(host->clk_sample);
>>>   error_disable_clk_output:
>>>  - clk_disable_unprepare(host->clk_output);
>>>  + if (host->cfg->clk_delays)
>>>  + clk_disable_unprepare(host->clk_output);
>>>   error_disable_clk_mmc:
>>>           clk_disable_unprepare(host->clk_mmc);
>>>   error_disable_clk_ahb:
>>>  @@ -1191,8 +1262,11 @@ static int sunxi_mmc_remove(struct platform_device *pdev)
>>>           if (!IS_ERR(host->reset))
>>>                   reset_control_assert(host->reset);
>>>
>>>  - clk_disable_unprepare(host->clk_sample);
>>>  - clk_disable_unprepare(host->clk_output);
>>>  + if (host->cfg->clk_delays) {
>>>  + clk_disable_unprepare(host->clk_sample);
>>>  + clk_disable_unprepare(host->clk_output);
>>>  + }
>>>  +
>>>           clk_disable_unprepare(host->clk_mmc);
>>>           clk_disable_unprepare(host->clk_ahb);
>
> Thanks,
> Icenowy
>



More information about the linux-arm-kernel mailing list