From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752168AbcFAWSy (ORCPT ); Wed, 1 Jun 2016 18:18:54 -0400 Received: from mout.kundenserver.de ([212.227.126.133]:63379 "EHLO mout.kundenserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752099AbcFAWSw (ORCPT ); Wed, 1 Jun 2016 18:18:52 -0400 From: Arnd Bergmann To: Gerd Hoffmann Cc: linux-rpi-kernel@lists.infradead.org, Eric Anholt , Ulf Hansson , Stephen Warren , Lee Jones , open list , "open list:MULTIMEDIA CARD (MMC), SECURE DIGITAL (SD) AND..." , "moderated list:BROADCOM BCM2835 ARM ARCHITECTURE" Subject: Re: [PATCH 21/32] mmc: bcm2835-sdhost: Add new driver for the internal SD controller. Date: Thu, 02 Jun 2016 00:18:52 +0200 Message-ID: <4715245.LFBOAeJdHc@wuerfel> User-Agent: KMail/5.1.3 (Linux/4.4.0-22-generic; KDE/5.18.0; x86_64; ; ) In-Reply-To: <1464817421-8519-22-git-send-email-kraxel@redhat.com> References: <1464817421-8519-1-git-send-email-kraxel@redhat.com> <1464817421-8519-22-git-send-email-kraxel@redhat.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Provags-ID: V03:K0:/HaMwad5xi+dtIe9ukEy1eywDJFyfLd6G3mS/dPd2LYdF0CDL6H 7D7qoIczVbn/3gx+K6rOATcDk5g7PEx63Oapv+oILvOmJjvzTrbd2mh2MyybnuXpUzOyTLp nyKC+YrSfUq60fBZbnMwgEE/AMWb0wDKRY/3X0tBTQmQ22VAP5uXmcgbVuzRQTygCvaCHMm BbK5tPfE6q/OVb+V6/lLw== X-UI-Out-Filterresults: notjunk:1;V01:K0:ZlQVT4Hd7wc=:8nq9UkXFCZaRXShIuRY8VX ve9vYoVtwW20Tocu0qnEXHrBwlPlyk+q6rLnp/kPn4bztuyEELcN7n0tSG16rE/Tzzjnd8a+6 zfpA+Y63mGlaPSc6i5hFkVW21ilFAWaxSUvbVlfXnoGhmWdmvepkZsI1nlt+gXmBntb/6sCIf FzNemnc4kBwERq1EveyWUkericIn3VtHNa3mNiohc7vpkGU0Q2vhCg8euOCSn1c4zV8AlFa8N TPXyJ29pLBipe4AMt37vatKwQDcZwcjCGfa/MtGMuJe73nK1BQsqT5Tx3XBfeW9iPgFujQ30/ JNtsruk50L5xkFXz+SiVaIh8UamQmT79jwbMjoNGlTyPIEAR684r8e2mVfMfCaj7o9u3lE9Cs mdfi6w8Bc8VKFVIs25OMW3lvY8cgY43li18OwlJgaZQK6hVtmEKPQlDt2oWGv7Dqg2tn9Eft/ qBeW/5/aL+n4/dCDnxZIxlv0wXCHYBQSFrGEbPqlv+MqrDJoelD7lKxN8g9M9a34BU/2ez0o5 QSrMlmI5/kIHms9MHcdr7JsGM/15J1rW0PaRUEs11hOgEnjGjTCyDtw6V3Lq5IErQyb7uiH5T yWou5mOhKGQ5P4RvYY2ToQ3e1NDtj0qAjfNuxFtGxUcCVzBbhLqVki9Ukis8d807HEWJeoe1P Ztk2wxYqL+uqPQ/PRMDTE6iLP1dJZ1Q6w84yBcDQ232vPIPXipifyI5nIZ2OqhcBQ4rC9DuzB eNpmoK3FRcFo5f2z Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wednesday, June 1, 2016 11:43:30 PM CEST Gerd Hoffmann wrote: > +static void bcm2835_sdhost_reset_internal(struct bcm2835_host *host) > +{ > + u32 temp; > + > + bcm2835_sdhost_set_power(host, false); > + > + bcm2835_sdhost_write(host, 0, SDCMD); > + bcm2835_sdhost_write(host, 0, SDARG); > + bcm2835_sdhost_write(host, 0xf00000, SDTOUT); > + bcm2835_sdhost_write(host, 0, SDCDIV); > + bcm2835_sdhost_write(host, 0x7f8, SDHSTS); /* Write 1s to clear */ > + bcm2835_sdhost_write(host, 0, SDHCFG); > + bcm2835_sdhost_write(host, 0, SDHBCT); > + bcm2835_sdhost_write(host, 0, SDHBLC); > + > + /* Limit fifo usage due to silicon bug */ > + temp = bcm2835_sdhost_read(host, SDEDM); > + temp &= ~((SDEDM_THRESHOLD_MASK << SDEDM_READ_THRESHOLD_SHIFT) | > + (SDEDM_THRESHOLD_MASK << SDEDM_WRITE_THRESHOLD_SHIFT)); > + temp |= (FIFO_READ_THRESHOLD << SDEDM_READ_THRESHOLD_SHIFT) | > + (FIFO_WRITE_THRESHOLD << SDEDM_WRITE_THRESHOLD_SHIFT); > + bcm2835_sdhost_write(host, temp, SDEDM); > + mdelay(10); > + bcm2835_sdhost_set_power(host, true); > + mdelay(10); Are you sure you cannot use msleep() here? > + host->clock = 0; > + bcm2835_sdhost_write(host, host->hcfg, SDHCFG); > + bcm2835_sdhost_write(host, host->cdiv, SDCDIV); > + mmiowb(); > +} What is the barrier for? Same question for all the other instances > + > +static void bcm2835_sdhost_reset(struct mmc_host *mmc) > +{ > + struct bcm2835_host *host = mmc_priv(mmc); > + unsigned long flags; > + > + spin_lock_irqsave(&host->lock, flags); > + bcm2835_sdhost_reset_internal(host); > + spin_unlock_irqrestore(&host->lock, flags); > +} The spinlock seems to only protect the reset from happening concurrently with bcm2835_sdhost_dma_complete() here. So if you ensure that this function is no longer pending, you can probably use msleep() above. > +static void bcm2835_sdhost_set_ios(struct mmc_host *mmc, struct mmc_ios *ios); Maybe rearrange the order of the functions to avoid forward declarations? > + spin_lock_init(&host->lock); > + > + if (IS_ERR_OR_NULL(host->dma_chan_tx) || > + IS_ERR_OR_NULL(host->dma_chan_rx)) { > + pr_err("%s: unable to initialise DMA channels. Falling back to PIO\n", > + mmc_hostname(mmc)); When are these ever NULL? > + /* Parse OF address directly to get the physical address for > + * DMA to our registers. > + */ > + host->phys_addr = be32_to_cpup(of_get_address(pdev->dev.of_node, 0, > + NULL, NULL)); This looks broken on 64-bit systems when #address-cells=<2>, or if there is an intermediate bus with a non-empty ranges property. What's wrong with platform_get_resource(pdev, IORESOURCE_MEM, 0)? Arnd