Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1411626 > unrolled thread

Re: [PATCH 21/32] mmc: bcm2835-sdhost: Add new driver for the internal SD controller.

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 21/32] mmc: bcm2835-sdhost: Add new driver for the internal SD controller. Arnd Bergmann <arnd@arndb.de> - 2016-06-02 00:20 +0200
    Re: [PATCH 21/32] mmc: bcm2835-sdhost: Add new driver for the internal SD controller. Eric Anholt <eric@anholt.net> - 2016-06-02 20:20 +0200
      Re: [PATCH 21/32] mmc: bcm2835-sdhost: Add new driver for the internal SD controller. Arnd Bergmann <arnd@arndb.de> - 2016-06-03 10:50 +0200

#1411626 — Re: [PATCH 21/32] mmc: bcm2835-sdhost: Add new driver for the internal SD controller.

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

[toc] | [next] | [standalone]

#1412480

[Multipart message — attachments visible in raw view] — view raw

Arnd Bergmann <arnd@arndb.de> writes:

> On Wednesday, June 1, 2016 11:43:30 PM CEST Gerd Hoffmann wrote:
>> +	/* 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)?

I'll get to the rest of the review later, but this is a weird pattern
that we have in several bcm2835-related drivers.

We need the address as seen by the DMA controller, not the address from
the ARM's perspective (which is offset by the dma-ranges DT property).
This is the way that people have figured out to get that address.

[toc] | [prev] | [next] | [standalone]

#1412931

On Thursday, June 2, 2016 11:12:38 AM CEST Eric Anholt wrote:
> Arnd Bergmann <arnd@arndb.de> writes:
> 
> > On Wednesday, June 1, 2016 11:43:30 PM CEST Gerd Hoffmann wrote:
> >> +    /* 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)?
> 
> I'll get to the rest of the review later, but this is a weird pattern
> that we have in several bcm2835-related drivers.
> 
> We need the address as seen by the DMA controller, not the address from
> the ARM's perspective (which is offset by the dma-ranges DT property).
> This is the way that people have figured out to get that address.

I see. This is indeed a bit tricky, as there is no easy way to know
how the dmaengine is wired up to the slave. A correct solution is
probably to follow the 'dma-ranges' up from the master to the common
parent and then follow the 'ranges' back to the slave, but that
requires coming up with a new exported function.

It's perhaps good enough in the meantime to read out the address from
the slave directly, but you should not assume that the first
cell has the complete information and instead should at least
evaluate #address-cells.

	Arnd

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web