On 27 November 2014 at 10:05, Johan Rudholm <johan.rudholm@xxxxxxxx> wrote:
> 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?
I like that approach.
In principle I think we should have only one API for hardware reset,
typically that should be mmc_hw_reset(). Then, convert
mmc_test_hw_reset() into invoking the mmc_hw_reset() API and let it
handle further tests by itself.
Kind regards
Uffe
--
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
