From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753818AbdDJPBA (ORCPT ); Mon, 10 Apr 2017 11:01:00 -0400 Received: from mail-wr0-f176.google.com ([209.85.128.176]:34391 "EHLO mail-wr0-f176.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751812AbdDJPAU (ORCPT ); Mon, 10 Apr 2017 11:00:20 -0400 Date: Mon, 10 Apr 2017 16:00:15 +0100 From: Lee Jones To: =?utf-8?B?5Yav5LyfbGludXg=?= Cc: "linux-kernel@vger.kernel.org" Subject: Re: [PATCH v3] mfd:rtsx: do retry when dma transfer error Message-ID: <20170410150015.o7d2iueesouzfsuh@dell> References: <1483606902-12762-1-git-send-email-steven_feng@realsil.com.cn> <20170407111316.fcjhydgbnf2yulaw@dell> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.6.2-neo (2016-08-21) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, 10 Apr 2017, 冯伟linux wrote: > > --- a/drivers/mfd/rtsx_pcr.c > > +++ b/drivers/mfd/rtsx_pcr.c > > @@ -30,6 +30,7 @@ > > #include > > #include > > #include > > +#include > > Why is this required? > > > The UHS_SER104_MAX_DTR which is in "card_clock = UHS_SER104_MAX_DTR > - (pcr->dma_error_count *20000000)" is defined in linux/mmc/card.h, so > it is required. Okay. > > spin_lock_irqsave(&pcr->lock, flags); > > - if (pcr->trans_result == TRANS_RESULT_FAIL) > > - err = -EINVAL; > > + if (pcr->trans_result == TRANS_RESULT_FAIL) { > > + err = -EILSEQ; > > "Illegal byte sequence", really? > > > This errno need to be -EILSEQ. You need to explain why. > >>> + if (card_clock == UHS_SDR104_MAX_DTR && > >>> + pcr->dma_error_count && > >>> + PCI_PID(pcr) == RTS5227_DEVICE_ID) > >>> + card_clock = (UHS_SDR104_MAX_DTR - > >>> + pcr->dma_error_count * 20000000); > > ... but won't this only reduce the clock frequency just once? > > > > There is no point bracketing the whole statement. > > > > But you do need to bracket one (the second) section of it. > > > The times of DMA transfer error occurrs recorded in dma_error_count, > When DMA transfer error occurrs, the card_clock is reduced by 20MHz. I think you'll find this logic will only reduce the clock frequency by 20MHz once and only once. After the first: card_clock = (UHS_SDR104_MAX_DTR - pcr->dma_error_count * 20000000) ... happens, the first comparison: card_clock == UHS_SDR104_MAX_DTR ... will fail on subsequent attempts and will not allow it to be reduced any further. Did you test it? -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog