<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Apr 29, 2015 at 2:45 PM, Arnd Bergmann <span dir="ltr"><<a href="mailto:arnd@arndb.de" target="_blank">arnd@arndb.de</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On Wednesday 29 April 2015 12:34:41 Suman Tripathi wrote:<br>
> On Tue, Apr 28, 2015 at 1:19 AM, Arnd Bergmann <<a href="mailto:arnd@arndb.de">arnd@arndb.de</a>> wrote:<br>
> > On Monday 27 April 2015 21:25:20 Suman Tripathi wrote:<br>
> >> > On Monday 27 April 2015 20:33:25 Suman Tripathi wrote:<br>
> >> > > > On Tuesday 21 April 2015 21:12:39 Suman Tripathi wrote:<br>
> >> > > > > +                       host->quirks |= SDHCI_QUIRK_BROKEN_DMA;<br>
> >> > > > > +<br>
> >> > > > > +               if (of_get_property(np, "no-cmd23", NULL))<br>
> >> > > > > +                       host->quirks2 |= SDHCI_QUIRK2_HOST_NO_CMD23;<br>
> >> > > > ><br>
> >> > > > >                 if (of_get_property(np, "no-1-8-v", NULL))<br>
> >> > > > ><br>
> >> > > > >                         host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V;<br>
> >> > > ><br>
> >> > > > Any property you add needs to be documented in the DT binding.<br>
> >> > > > If possible, add generic properties for each bug you have mmc.txt<br>
> >> > > > rather than the driver specific sdhci.txt, and implement the<br>
> >> > ><br>
> >> > > I will add the binding in mmc.txt. I thought this was present but not.<br>
> >> > ><br>
> >> > > > parsing in a common function that is used for all mmc hosts.<br>
> >> > ><br>
> >> > > As per mine understanding the sdhci_get_of_porperty is a common<br>
> >> > > parsing function  . Am I wrong ??<br>
> >><br>
> ><br>
> > A small side note: please fix your email client to use proper attribution<br>
> > of the citations. The way you reply, nobody knows what you are saying<br>
> > compare to what you quote. Also, reduce the quotation to the parts you<br>
> > are replying to.<br>
> ><br>
><br>
> Okay. Sorry for that. I fixed it.<br>
<br>
</span>Ok, much better.<br>
<span class=""><br>
> >> > No, this is only used for sdhci, not for the other controllers.<br>
> >><br>
> >> But our's is a SHCI variant so I added it in this file.<br>
> ><br>
> > That's my point: a lot of the bugs are independent of the specific<br>
> > host controller and could happen with any one of them. We want to<br>
> > ensure that nobody tries to add another property with similar<br>
> > semantics and a different name just because they are using a<br>
> > different driver.<br>
><br>
> Then I am not finding a reason why we have sdhci_get_of_property function ?? .<br>
>  I added a generic names like broken-adma that everyone can reuse it.<br>
> I made mistake of not adding it in the binding.<br>
><br>
> For eg : broken-cd is not added by me but I can use it. So I added<br>
> something like broken-adma as it was not present.<br>
<br>
</span>The common mmc_of_parse() handles "broken-cd", and the sdhci_get_of_property()<br>
does so too. This is really a mistake we made earlier when it was added<br>
to sdhi instead of the common code. We should remove the parsing for<br>
that property from the sdhci driver and have the core handle it always,<br>
but that require someone to do it and ensure that no subtle ABI changes<br>
are introduced on the way.<br></blockquote><div> </div><div>I was not aware of the mmc_of_parse() . Agree now.</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
For new properties, the right way is to add it to the common function only.<br>
<span class=""><br>
> >> 2. We need to support PIO mode as of now because DMA or ADMA requires<br>
> >> some kind of translation driver that I am working on.<br>
> ><br>
> > But this does not describe the hardware properties. Don't add properties<br>
> > that describe the lack of a kernel driver. If you can't do DMA yet,<br>
> > use a dma-ranges property that lists one empty range to prevent<br>
> > dma_set_mask() from working, so it will fall back to PIO mode. You<br>
> > may have to fix the driver if that doesn't already work.<br>
> ><br>
><br>
> The generic sdhc framework doesn't have this capabiltiy. It uses the<br>
> quirks to identify the broken DMA and ADMA modes even<br>
> if the controller is capable of.<br>
><br>
> > What kind of driver do you need here?<br>
><br>
> For DMA and adma we need some 32 bit to 64 bit translation driver.<br>
> The existing arasan driver only support 32 bit.<br>
<br>
</span>Ok, that sounds like a very simple case: The width of the DMA is determined<br>
from DT by looking at the dma-ranges properties. If it doesn't work, one<br>
of these steps that are supposed to happen are broken and you should<br>
try to find out which one that is and fix it:<br>
<br>
- The parent node of the sdhci device in DT must not claim to support<br>
  64 bit if the bus is only 32-bit wide. A dma-ranges property containing<br>
  "<0 0 1 0>" would describe a bus that has a 32-bit DMA address range that<br>
  is 1:1 mapped to the root bus, which is the default. </blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
- The ARM64 code must check that property in a call to dma_set_mask()<br>
  or dma_set_mask_and_coherent(), and not allow a mask to be set that<br>
  exceeds the size of the dma-ranges property.<br>
<br>
- The sdhci driver must call dma_set_mask() or dma_set_mask_and_coherent()<br>
  with the mask that is claimed by the device (usually 32 bit or 64 bit)<br>
  and check the result.<br>
<br>
- If the call to dma_set_mask() for the 64-bit mask fails, the driver must<br>
  fall back to using the 32-bit mask and not attempt to use the 64-bit<br>
  DMA registers. <br></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
This is the behavior we require anyway, and if this all works, you don't<br>
need the extra quirks.<br>
<br>
The above assumes that the limitation is enforced by the bus (e.g. an<br>
AHB bus can only do 32-bit DMA). It would be a little different if you<br>
have a 64-bit AXI bus and the Arasan device itself is limited to 32-bit<br>
independent of the width of the bus it is connected to. Can you find out<br>
which of these two cases you have? <br></blockquote><div><br></div><div>We have 64 bit AXI and Arasan device or controller is 32 bit . So there is 64 bit AXI to 32 bit AHB translation. </div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
> >> 3. The version of arasan variant we have in our SoC doesn't have the<br>
> >> HISPD  bit field in HI-SPEED SD card. So this makes HI-SPEED sdcard<br>
> >> work.<br>
> >><br>
> >> 4. NO_CMD23 is required for eMMC cards.<br>
> >><br>
> >> These are not new properties.  Only the fact is I am using it for our<br>
> >> SoC from dtb. These quirks are already there in mmc common framework.<br>
> >> Nothing is new.<br>
> ><br>
> > Are you sure that you have version 8.9a of the Arasan SDHCI? This sounds<br>
><br>
> No We are using 4.9a ARASAN SDHCI<br>
<br>
</span>Ok, then add a compatible string for this version to the arasan binding,<br>
and set the quirks flags only for the 4.9a version and not for the 8.9a<br>
version.<br></blockquote><div> </div><div>Okay </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class="HOEnZb"><font color="#888888"><br>
        Arnd<br>
</font></span></blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature"><div dir="ltr"><div>Thanks,</div><div>with regards,</div>Suman Tripathi</div></div>
</div></div>