[PATCHv2 3/5] mmc: shdci-bcm2835: add efficient back-to-back write workaround

Scott Branden sbranden at broadcom.com
Fri Nov 7 10:30:03 PST 2014

On 14-11-05 08:48 PM, Stephen Warren wrote:
> On 11/04/2014 11:55 PM, Scott Branden wrote:
>> On 14-11-04 08:57 PM, Stephen Warren wrote:
>>> On 10/30/2014 12:36 AM, Scott Branden wrote:
>>>> The bcm2835 has clock domain issues when back to back writes to certain
>>>> registers are written.  The existing driver works around this issue with
>>>> udelay.  A more efficient method is to store the 8 and 16 bit writes
>>>> to the registers affected and then write them as 32 bits at the
>>>> appropriate
>>>> time.
>>>> diff --git a/drivers/mmc/host/sdhci-bcm2835.c
>>>> b/drivers/mmc/host/sdhci-bcm2835.c
>>>>    static void bcm2835_sdhci_writew(struct sdhci_host *host, u16 val,
>>>> int reg)
>>>>    {
>>>>        struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>> -    struct bcm2835_sdhci *bcm2835_host = pltfm_host->priv;
>>>> -    u32 oldval = (reg == SDHCI_COMMAND) ? bcm2835_host->shadow :
>>>> -        bcm2835_sdhci_readl(host, reg & ~3);
>>>> +    struct bcm2835_sdhci_host *bcm2835_host = pltfm_host->priv;
>>> Is that type change for bcm2835_host really correct?
>> Yes - at the top of the patch the structure has been expanded and named
>> appropriately.
>> -struct bcm2835_sdhci {
>> -    u32 shadow;
>> +struct bcm2835_sdhci_host {
>> +    u32 shadow_cmd;
>> +    u32 shadow_blk;
>>   };
> Ah yes, sorry for missing that.
>>>> +    } else {
>>>> +        /* Read reg, all other registers are not shadowed */
>>>> +        oldval = readl(host->ioaddr + (reg & ~3));
>>> Is there any reason to use readl() directly here rather than calling
>>> bcm2835_readl()? ...
>> Yes, bcm2835_readl does not need to be called in read-modify-write and
>> shadow register situations and just adds overhead.  All that needs to be
>> called is readl.  bcm2835_readl has some existing ugly code in it to
>> modify the capabilities register on a read function.  This info never
>> needs to be for write as you can't overwrite the capabilities register.
> To be honest, it seems better to do all the read/write through
> consistent functions. One advantage of bcm2835_readl() is that it
> consistently adds on the base address internally so you don't have to
> write it out every time manually. Still, the code ought to work fine
> after this change, so I guess it's OK.
>> I hope to get rid of the capabilities hack in a future patch as this
>> should never have been acceptable in upstreamed code to begin with.  The
>> capabilities override should have been passed in through a device tree
>> entry.
> It's a pretty common technique with precedent. I certainly don't agree
> that it should be configured by DT. Arguably, DT makes sense to describe
> board-to-board variations, but there's almost zero point putting data
> into DT that is SoC description rather than board description; just put
> it into the driver to avoid continually parsing the same data over and
> over from DT just to get back to the same data that could have been
> encoded into the driver. If the data varies between similar controllers,
> an of_match table can easily be used to parameterize it based on
> compatible value.
There is work to be done here or I will be unable to use this driver in 
our chipsets.  Perhaps it will be easier having another driver 
actually... as the DMA seems quite different on RPI.

More information about the linux-rpi-kernel mailing list