Re: [PATCH v3 0/2] Add support for Allwinner A64 mmc controller

Re: [PATCH v3 0/2] Add support for Allwinner A64 mmc controller

[Hans de Goede]

Hi,

On 01-08-16 17:13, Icenowy Zheng wrote:
> Allwinner A64 has a slightly modified version of the Allwinner MMC controller.
>
> It do not have "output" or "sample" clock input, instead, it can calibrate the
> delay internally.
>
> This series of patch added the calibrate feature to the sunxi-mmc driver. It
> is based on works by Andre Przywara <andre.przywara@arm.com>, and now it
> depends on the patch set sent recently by Hans de Goede <hdegoede@redhat.com>,
> which disabled "output" and "sample" clock on A10/13.
> ( http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/446187.html )
>
> As the clock and device tree patch for Allwinner A64 is not yet merged, the
> patch set contains no patch to enable it.

Series looks good to me and is:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans

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

[Jaehoon Chung]

Hi Icenowy,

On 08/02/2016 12:13 AM, 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.
> 
> Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
> ---
> Changes in v2:
> - Rebased on Hans de Goede's patchset.
> Changes in v3:
> - Tidy up based on Hans de Goede's opinions.
> 
>  drivers/mmc/host/sunxi-mmc.c | 66 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 66 insertions(+)
> 
> diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
> index 2ec91ce..3c26180 100644
> --- a/drivers/mmc/host/sunxi-mmc.c
> +++ b/drivers/mmc/host/sunxi-mmc.c
> @@ -72,6 +72,13 @@
>  #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 */
> +
>  #define mmc_readl(host, reg) \
>  	readl((host)->reg_base + SDXC_##reg)
>  #define mmc_writel(host, reg, value) \
> @@ -217,6 +224,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 +248,9 @@ struct sunxi_idma_des {
>  struct sunxi_mmc_cfg {
>  	u32 idma_des_size_bits;
>  	const struct sunxi_mmc_clk_delay *clk_delays;
> +
> +	/* does the IP block support autocalibration? */
> +	bool can_calibrate;
>  };
>  
>  struct sunxi_mmc_host {
> @@ -657,6 +676,38 @@ 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)

Where is mmc_ios structure used in this function?
And why passing the reg_off?

> +{
> +	u32 reg = readl(host->reg_base + reg_off);
> +	u32 delay;

delay doesn't need to use u32..
reg_off is only passed with SDXC_REG_SAMP_DL_REG..this function doesn't reuse anywhere.

> +
> +	if (!host->cfg->can_calibrate)
> +		return 0;
> +
> +	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();

If never hit this condition, infinite loop? 

> +
> +	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;

Something is strange. It seems to maintain the  delay value.

reg &= ~(SDXC_CAL_START | (SDXC_CAL_DL_MASK << SDXC_CAL_DL_SHIFT));
reg |= SDXC_CAL_DL_SW_EN; 

is it same thing?

> +
> +	writel(reg, host->reg_base + reg_off);
> +
> +	dev_dbg(mmc_dev(host->mmc), "calibration ended, res is 0x%x\n", reg);

s/res is/reg is ?

> +
> +	/* TODO: enable calibrate on sdc2 SDXC_REG_DS_DL_REG of A64 */
> +	return 0;
> +}
> +
>  static int sunxi_mmc_clk_set_phase(struct sunxi_mmc_host *host,
>  				   struct mmc_ios *ios, u32 rate)
>  {
> @@ -731,6 +782,10 @@ static int sunxi_mmc_clk_set_rate(struct sunxi_mmc_host *host,
>  	if (ret)
>  		return ret;
>  
> +	ret = sunxi_mmc_calibrate(host, ios, SDXC_REG_SAMP_DL_REG);
> +	if (ret)
> +		return ret;

Never enter this condition. sunxi_mmc_calibrate() is always returned 0.


Best Regards,
Jaehoon Chung

> +
>  	return sunxi_mmc_oclk_onoff(host, 1);
>  }
>  
> @@ -982,21 +1037,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 +1069,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);
> 

Re: [PATCH v3 1/2] Documentation: dt: Add new compatible to sunxi mmc driver bindings

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1154 bytes --]

Hi Icenowy,

On Mon, Aug 01, 2016 at 11:13:50PM +0800, Icenowy Zheng wrote:
> Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
> ---
> Changes in v2:
> - Rebased on Hans de Goede's patchset.
> 
>  Documentation/devicetree/bindings/mmc/sunxi-mmc.txt | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/sunxi-mmc.txt b/Documentation/devicetree/bindings/mmc/sunxi-mmc.txt
> index 904ff9f..55cdd80 100644
> --- a/Documentation/devicetree/bindings/mmc/sunxi-mmc.txt
> +++ b/Documentation/devicetree/bindings/mmc/sunxi-mmc.txt
> @@ -13,6 +13,7 @@ Required properties:
>     * "allwinner,sun5i-a13-mmc"
>     * "allwinner,sun7i-a20-mmc"
>     * "allwinner,sun9i-a80-mmc"
> +   * "allwinner,sun50i-a64-mmc"
>   - reg : mmc controller base registers
>   - clocks : a list with 4 phandle + clock specifier pairs
>   - clock-names : must contain "ahb", "mmc", "output" and "sample"

You need to document that the output and sample clocks are not
mandatory in the A64 case.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]