Hi Ulf,
2014-12-01 11:50 GMT+01:00 Ulf Hansson <ulf.hansson@xxxxxxxxxx>:
> On 28 November 2014 at 16:54, Johan Rudholm <johan.rudholm@xxxxxxxx> wrote:
>> 2014-11-27 10:50 GMT+01:00 Ulf Hansson <ulf.hansson@xxxxxxxxxx>:
>>> 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.
>>
>> The trouble is that mmc_test_hw_reset() needs to check if the reset
>> was a success after calling host_ops->hw_reset, but before calling
>
> How about always do the CMD13 check when host_ops->hw_reset() exist
> and returns suceess? The error handling for CMD13 check could be print
> an error. It shouldn't prevent us from proceeding with the rest of
> power cycle operations.
Sorry for having dropped this conversation for so long. I've thought
about the best approach for a while now and I'll soon send out a patch
set with your suggestions incorporated.
Kind regards, 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
