Hi Ulf,
2014-11-25 14:48 GMT+01:00 Ulf Hansson <ulf.hansson@xxxxxxxxxx>:
> On 24 November 2014 at 12:06, Johan Rudholm <johan.rudholm@xxxxxxxx> wrote:
>> Move the (e)MMC specific hw_reset code from core.c into mmc.c and call
>> it from the new bus_ops member hw_reset. This also lets us add code
>> for resetting SD cards as well.
>>
>> Signed-off-by: Johan Rudholm <johanru@xxxxxxxx>
>> ---
>> drivers/mmc/core/core.c | 56 +++++++++++++++-------------------------------
>> drivers/mmc/core/core.h | 5 ++++
>> drivers/mmc/core/mmc.c | 40 +++++++++++++++++++++++++++++++++
>> 3 files changed, 63 insertions(+), 38 deletions(-)
>>
>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> index 9584bff..492b3e5 100644
>> --- a/drivers/mmc/core/core.c
>> +++ b/drivers/mmc/core/core.c
>> @@ -2245,67 +2245,47 @@ static void mmc_hw_reset_for_init(struct mmc_host *host)
>> mmc_host_clk_release(host);
>> }
>>
>> -int mmc_can_reset(struct mmc_card *card)
>> -{
>> - u8 rst_n_function;
>> -
>> - if (!mmc_card_mmc(card))
>> - return 0;
>> - rst_n_function = card->ext_csd.rst_n_function;
>> - if ((rst_n_function & EXT_CSD_RST_N_EN_MASK) != EXT_CSD_RST_N_ENABLED)
>> - return 0;
>> - return 1;
>> -}
>> -EXPORT_SYMBOL(mmc_can_reset);
>
> Isn't the mmc_can_reset() function used from mmc_test.c?
Yes. Maybe we should move this stuff to mmc_test instead. We could
also move the code that checks if the reset worked or not to mmc_test,
since this is the only place where the check is performed.
>> -
>> +/* Reset card in a bus-specific way */
>> static int mmc_do_hw_reset(struct mmc_host *host, int check)
>> {
>> - struct mmc_card *card = host->card;
>> -
>> - if (!(host->caps & MMC_CAP_HW_RESET) || !host->ops->hw_reset)
>> - return -EOPNOTSUPP;
>> + int ret;
>>
>> - if (!card)
>> + if (!host->card)
>> return -EINVAL;
>>
>> - if (!mmc_can_reset(card))
>
> You need a mmc_bus_get() here before accessing the host->bus_ops callbacks.
>
> Well, if you would executed this code with the host claimed and from
> the mmc block layer, you can be sure the bus_ops to exist. Now, since
> this is an exported function, I think you need to be more safe to also
> do the mmc_bus_get|put().
Got it, thanks.
>> + if (!host->bus_ops || !host->bus_ops->hw_reset ||
>> + host->bus_ops->hw_reset(host, MMC_HW_RESET_TEST))
>> return -EOPNOTSUPP;
>>
>> - mmc_host_clk_hold(host);
>> - mmc_set_clock(host, host->f_init);
>> + ret = host->bus_ops->hw_reset(host, check);
>
> What's going on here? You are invoking the ->hw_reset() callback
> twice. That seems odd.
>>
>> - host->ops->hw_reset(host);
>> -
>> - /* If the reset has happened, then a status command will fail */
>> - if (check) {
>> - u32 status;
>> -
>> - if (!mmc_send_status(card, &status)) {
>> - mmc_host_clk_release(host);
>> - return -ENOSYS;
>> - }
>> - }
>> -
>> - /* Set initial state and call mmc_set_ios */
>> - mmc_set_initial_state(host);
>> + if (check == MMC_HW_RESET_TEST_CARD)
>> + return ret;
>>
>> - mmc_host_clk_release(host);
>> + pr_warn("%s: tried to reset card (status %d)\n",
>> + mmc_hostname(host), ret);
>>
>> return host->bus_ops->power_restore(host);
>> }
>>
>> int mmc_hw_reset(struct mmc_host *host)
>> {
>> - return mmc_do_hw_reset(host, 0);
>> + return mmc_do_hw_reset(host, MMC_HW_RESET_RESET);
>> }
>> EXPORT_SYMBOL(mmc_hw_reset);
>>
>> int mmc_hw_reset_check(struct mmc_host *host)
>> {
>> - return mmc_do_hw_reset(host, 1);
>> + return mmc_do_hw_reset(host, MMC_HW_RESET_CHECK);
>> }
>> EXPORT_SYMBOL(mmc_hw_reset_check);
>>
>> +int mmc_can_reset(struct mmc_card *card)
>> +{
>> + return mmc_do_hw_reset(card->host, MMC_HW_RESET_TEST_CARD);
>> +}
>> +EXPORT_SYMBOL(mmc_can_reset);
>> +
>> static int mmc_rescan_try_freq(struct mmc_host *host, unsigned freq)
>> {
>> host->f_init = freq;
>> diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
>> index d76597c..f6e0a52 100644
>> --- a/drivers/mmc/core/core.h
>> +++ b/drivers/mmc/core/core.h
>> @@ -27,6 +27,11 @@ struct mmc_bus_ops {
>> int (*power_restore)(struct mmc_host *);
>> int (*alive)(struct mmc_host *);
>> int (*shutdown)(struct mmc_host *);
>> + int (*hw_reset)(struct mmc_host *, int);
>> +#define MMC_HW_RESET_RESET 0
>> +#define MMC_HW_RESET_TEST 1
>> +#define MMC_HW_RESET_CHECK 2
>> +#define MMC_HW_RESET_TEST_CARD 3
>
> Urgh. Is there a way to remove all this? I just don't like all these options.
>
> In fact, I would prefer to have none of them.
If we move the test and check functionality to mmc_test, I think we
can avoid these. What do you think of this approach?
//Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
