* Re: [PATCH v2 2/2] mmc: host: sunxi: add support for A64 mmc controller
[not found] ` <20160731110234.3593-3-icenowy@aosc.xyz>
@ 2016-07-31 14:30 ` Hans de Goede
[not found] ` <24941470008912@web11j.yandex.ru>
0 siblings, 1 reply; 2+ messages in thread
From: Hans de Goede @ 2016-07-31 14:30 UTC (permalink / raw)
To: Icenowy Zheng, Rob Herring, Maxime Ripard, Chen-Yu Tsai, Ulf Hansson
Cc: Mark Rutland, Jaehoon Chung, Michal Suchanek, devicetree,
linux-arm-kernel, linux-kernel, linux-mmc
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@arm.com>.
>
> Signed-off-by: Icenowy Zheng <icenowy@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.
> #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.
> + /* 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 ...
> + 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 ...
> +
> + 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;
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 & 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);
>
>
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [PATCH v2 2/2] mmc: host: sunxi: add support for A64 mmc controller
[not found] ` <24941470008912@web11j.yandex.ru>
@ 2016-08-01 7:29 ` Hans de Goede
0 siblings, 0 replies; 2+ messages in thread
From: Hans de Goede @ 2016-08-01 7:29 UTC (permalink / raw)
To: Icenowy Zheng, Rob Herring, Maxime Ripard, Chen-Yu Tsai, Ulf Hansson
Cc: Mark Rutland, Jaehoon Chung, Michal Suchanek, devicetree,
linux-arm-kernel, linux-kernel, linux-mmc
Hi,
On 01-08-16 01:48, Icenowy Zheng wrote:
>
> Hi,
> 31.07.2016, 22:30, "Hans de Goede" <hdegoede@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@arm.com>.
>>>
>>> Signed-off-by: Icenowy Zheng <icenowy@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
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2016-08-01 7:30 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20160731110234.3593-1-icenowy@aosc.xyz>
[not found] ` <20160731110234.3593-3-icenowy@aosc.xyz>
2016-07-31 14:30 ` [PATCH v2 2/2] mmc: host: sunxi: add support for A64 mmc controller Hans de Goede
[not found] ` <24941470008912@web11j.yandex.ru>
2016-08-01 7:29 ` Hans de Goede
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).