Re: [PATCH V13 05/10] mmc: cqhci: support for command queue enabled host

From: Adrian Hunter
Date: Thu Nov 09 2017 - 07:55:56 EST


On 09/11/17 14:26, Linus Walleij wrote:
> On Wed, Nov 8, 2017 at 3:14 PM, Adrian Hunter <adrian.hunter@xxxxxxxxx> wrote:
>> On 08/11/17 11:22, Linus Walleij wrote:
>>> On Fri, Nov 3, 2017 at 2:20 PM, Adrian Hunter <adrian.hunter@xxxxxxxxx> wrote:
>
>>> (...)
>
>>>> +EXPORT_SYMBOL(cqhci_resume);
>>>
>>> Why would the CQE case require special suspend/resume
>>> functionality?
>>
>> Seems like a very strange question.
>
> Please realize that patch review is partly about education.
>
> Educating me or anyone about your patch set involves
> being humbe and not seeing your peers as lesser.
>
> Making your reviewer feel stupid by saying the ask
> "strange" or outright stupid questions is not helping
> your cause.

Please try to forgive me for being rude, it is just frustration.

>
>> Obviously CQHCI has to be configured
>> after suspend.
>
> Yeah. I think I misunderstood is such that:
>
>> Also please don't confuse CQE and CQHCI. CQHCI is an implementation of a
>> CQE. We currently do not expect to have another implementation, but it is
>> not impossible.
>
> OK now you educated me, see it's not that hard without
> using belitteling language.
>
>>> This seems two much like on the side CQE-silo engineering,
>>> just use the device .[runtime]_suspend/resume callbacks like
>>> everyone else, make it possible for the host to figure out
>>> if it is in CQE mode or not (I guess it should know already
>>> since cqhci .enable() has been called?) and handle it
>>> from there.
>>
>> That is how it works! The host controller has to decide how to handle
>> suspend / resume.
>
> OK.
>
>> cqhci_suspend() / cqhci_resume() are helper functions that the host
>> controller can use, but doesn't have to.
>
> OK.
>
>>> Why would CQE hosts need special accessors and the rest
>>> of the host not need it?
>>
>> Special accessors can be used to fix up registers that don't work exactly
>> the way the standard specified.
>
> Yeah this is fine as it is for CQHCI, I didn't get that
> part :)
>
>>> ->enable and ->disable() for just CQE seem reasonable.
>>> But that leaves just two new ops.
>>>
>>> So why not just put .cqe_enable() and .cqe_disable()
>>> ops into mmc_host_ops as optional and be done with it?
>>
>> Ok so you are not understanding this at all.
>
> No I did not get it. But I do now (I think).
>
>> As a CQE implementation, CQHCI interfaces with the upper layers through the
>> CQE ops etc.
>>
>> But CQHCI also has to work with any host controller driver, so it needs an
>> interface for that, which is what cqhci_host_ops is for. All the ops serve
>> useful purposes.
> (...)
>> The whole point is to prove a library that can work with any host controller
>> driver. That means it must provide functions and callbacks.
>
> OK
>
>>> I think the above approach to put any CQE-specific callbacks
>>> directly into the struct mmc_host_ops is way more viable.
>>
>> Nothing to do with CQE. This is CQHCI. Please try to get the difference.
>
> I am trying, please try to think about your language.

I strongly disapprove of being rude but sadly it seems to get results.