Hi, Guennadi-san,
(2013/06/28 16:54), Guennadi Liakhovetski wrote:> Hi Shimoda-san
>
> Thanks for an update. Sorry it took me so long to get down to reviewing
> it. I looked at it originally and I knew, I would need a significant time
> this time to look and think about it, so, I had to postpone. This looks
> much better already, thanks! The flow is already correct, but I think it
> might be possible to improve it further. Please, see below.
Thank you very much for your review!
> On Tue, 18 Jun 2013, Shimoda, Yoshihiro wrote:
< snip >
>> + struct mmc_request mrq_sbc; /* mmc_request for SBC */
>
> I don't think we'll need this eventually.
I got it.
< snip >
>> @@ -936,9 +942,27 @@ static void sh_mmcif_request(struct mmc_host *mmc,
>> struct mmc_request *mrq)
>> break;
>> }
>>
>> - host->mrq = mrq;
>> + if (mrq->sbc) {
>
> Ok, this is an entry point, you have to act here, agree. But how about the
> following: we add a new WAIT state: MMCIF_WAIT_FOR_SBC and use that one
> inside sh_mmcif_start_cmd() instead of MMCIF_WAIT_FOR_CMD in the SBC case?
> Then maybe you won't need to change sh_mmcif_request() at all or almost at
> all:
I will add a new WAIT state.
< snip >
>> + /* Store original mrq to mrq_orig */
>> + host->mrq_orig = mrq;
>> +
>> + /* Copy original mrq data to mrq_sbc */
>> + host->mrq_sbc = *mrq;
>>
>> - sh_mmcif_start_cmd(host, mrq);
>
> this call might change, see later.
>
>> + /* Switch the mrq_sbc.cmd for SBC */
>> + host->mrq_sbc.cmd = mrq->sbc;
>> + host->mrq_sbc.sbc = NULL;
>> + host->mrq_sbc.data = NULL;
>> + host->mrq_sbc.stop = NULL;
>> +
>> + /* Set current mrq pointer to mrq_sbc */
>> + host->mrq = &host->mrq_sbc;
>> + } else {
>> + /* Set current mrq pointer to original mrq */
>> + host->mrq = mrq;
>> + }
>> +
>> + sh_mmcif_start_cmd(host, host->mrq);
>
> This will set "host->wait_for = MMCIF_WAIT_FOR_CMD"
>
>> }
>>
>> static int sh_mmcif_clk_update(struct sh_mmcif_host *host)
>> @@ -1212,13 +1236,35 @@ static irqreturn_t sh_mmcif_irqt(int irq, void
>> *dev_id)
>
> Now, when we enter sh_mmcif_irqt() after an SBC command completion, we
> still have "host->wait_for == MMCIF_WAIT_FOR_CMD" so we enter
> sh_mmcif_end_cmd(), right? But you set mrq->data = NULL above, so, it just
> (possibly) gets a response and returns. So far so good.
Your point is correct.
> With the proposed MMCIF_WAIT_FOR_SBC you'll have something like
>
> case MMCIF_WAIT_FOR_SBC:
> wait = sh_mmcif_end_sbc(host);
> break;
>
> In sh_mmcif_end_sbc() you would do a similar to sh_mmcif_end_cmd() error
> processing, maybe get a response (no idea, whether SBC has MMC_RSP_PRESENT
> set), call sh_mmcif_start_cmd() again, but there now you have to take care
> not to jump to the same state again, but to use MMCIF_WAIT_FOR_CMD this
> time. So, your wait-assignment in sh_mmcif_start_cmd() would look like
>
> if (mrq->sbc && host->wait_for != MMCIF_WAIT_FOR_SBC)
> host->wait_for = MMCIF_WAIT_FOR_SBC;
> else
> host->wait_for = MMCIF_WAIT_FOR_CMD;
>
> and return true.
I got it, I will modify this.
>> return IRQ_HANDLED;
>> }
>>
>> + if (mrq->sbc && (mrq->cmd->opcode == MMC_WRITE_MULTIPLE_BLOCK) &&
>> + (host->wait_for != MMCIF_WAIT_FOR_WRITE_END)) {
>> + /* Wait for end of data phase */
>> + host->wait_for = MMCIF_WAIT_FOR_WRITE_END;
>> + sh_mmcif_bitset(host, MMCIF_CE_INT_MASK, MASK_MDTRANE);
>> + schedule_delayed_work(&host->timeout_work, host->timeout);
>> + mutex_unlock(&host->thread_lock);
>> + return IRQ_HANDLED;
>> + }
>> +
>> + if (mrq->sbc && (mrq->cmd->opcode == MMC_READ_MULTIPLE_BLOCK) &&
>> + (host->wait_for != MMCIF_WAIT_FOR_READ_END)) {
>> + /* Wait for end of data phase */
>> + host->wait_for = MMCIF_WAIT_FOR_READ_END;
>> + sh_mmcif_bitset(host, MMCIF_CE_INT_MASK, MASK_MBUFRE);
>> + schedule_delayed_work(&host->timeout_work, host->timeout);
>> + mutex_unlock(&host->thread_lock);
>> + return IRQ_HANDLED;
>> + }
>
> Hm, this is interesting. Why do you need those? Currently we only wait for
> read- and write-end conditions in single-block read- and write-operations,
> but not in multi-block ones. With SBC you also want to wait for them in
> the multi-block case. Is it really SBC-specific or maybe we have to do
> this always? In either case I wouldn't add it here but to respective
> state-handlers, called from the switch statement above. And if this is
> indeed needed for all multi-block operations, this should be a separate
> patch.
In the previous code, the driver always enables the "Automatic CMD12 Issuance"
function in the multi-block case. So, we don't need wait for read- and write-end
conditions. However, if we use SBC, we disables the "Automatic CMD12 Issuance"
function. So, we have to do this only when we use SBC.
So, Should I separate this code as other patch?
Or, should I remove this code, and add similar code to sh_mmcif_end_cmd() and
sh_mmcif_[mread,mwrite]_block()?
>> +
>> if (host->wait_for != MMCIF_WAIT_FOR_STOP) {
>
> Currently you enter this path also after processing an SBC, which isn't
> needed, but just happens to be harmless. If you use MMCIF_WAIT_FOR_SBC you
> don't get here at all.
Since we have to set the "data->bytes_xfered" below, we need to enter this path
even if we use SBC:
>> struct mmc_data *data = mrq->data;
>> if (!mrq->cmd->error && data && !data->error)
>> data->bytes_xfered =
>> data->blocks * data->blksz;
We need this code.
>> - if (mrq->stop && !mrq->cmd->error && (!data || !data->error)) {
>> + /* If SBC, we don't use CMD12(STOP) */
>> + if (mrq->stop && !mrq->cmd->error && (!data || !data->error) &&
>> + !mrq->sbc) {
>> sh_mmcif_stop_cmd(host, mrq);
>> if (!mrq->stop->error) {
>> schedule_delayed_work(&host->timeout_work,
>> host->timeout);
>> @@ -1228,6 +1274,14 @@ static irqreturn_t sh_mmcif_irqt(int irq, void
>> *dev_id)
>> }
>> }
>>
>> + if ((mrq->cmd->opcode == MMC_SET_BLOCK_COUNT) && !mrq->cmd->error) {
>
> This won't be needed.
I will remove it.
Best regards,
Yoshihiro Shimoda
>> + /* Send the original .request() command */
>> + host->mrq = host->mrq_orig;
>> + sh_mmcif_start_cmd(host, host->mrq);
>> + mutex_unlock(&host->thread_lock);
>> + return IRQ_HANDLED;
>> + }
>> +
>> host->wait_for = MMCIF_WAIT_FOR_REQUEST;
>> host->state = STATE_IDLE;
>> host->mrq = NULL;
>> --
>> 1.7.1
>
> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/
>
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
