linux-kernel - Re: ARC dw-mshc binding compat string

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Mon, 28 Mar 2016 10:55:37 +0000
From:	Alexey Brodkin <Alexey.Brodkin@...opsys.com>
To:	"jh80.chung@...sung.com" <jh80.chung@...sung.com>
CC:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
	"marex@...x.de" <marex@...x.de>,
	"vladimir_zapolskiy@...tor.com" <vladimir_zapolskiy@...tor.com>,
	"robh+dt@...nel.org" <robh+dt@...nel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
Subject: Re: ARC dw-mshc binding compat string

Hi Jaehoon,

On Mon, 2016-03-28 at 19:34 +0900, Jaehoon Chung wrote:
> Hi,

[snip]

> > > > > > > > 
> > > > > > > > That said, I would rather prefer to see "snps,dw-mshc" prefix on description
> > > > > > > > of an MMC controller found on SoCFPGA series, "altr,socfpga-dw-mshc" seems
> > > > > > > > to be redundant.
> Yes..it's redundant..i should be combined to "snps,dw-mshc".

So for socfpga platform compat string should be something like "snps,dw-mshc-socfpga" then?

> > 
> > > 
> > > > 
> > > > > 
> > > > > > 
> > > > > > > 
> > > > > > > According to drivers/mmc/host/dw_mmc-pltfm.c , the Altera SoCFPGA one
> > > > > > > "altr,socfpga-dw-mshc" and also Imagination Technology Pistacio one
> > > > > > > "img,pistachio-dw-mshc" need specialty bit (SDMMC_CMD_USE_HOLD_REG),
> > > > > > > while the stock one "snps,dw-mshc" does not. I am not sure if the ARC
> > > > > > > one needs it as well, but most likely yes.
> > > > > > > 
> > > > > > > I wonder if that bit is needed on some particular version of the DWMMC
> > > > > > > core. In that case, should we have "snps,dw-mshc" and "snps,dw-mshc-vN"
> > > > > > > binding ? Or should we use DT property to discern the need for this bit ?
> > > > > > > 
> > > > > > That's the most common way to take into account peculiarities, add
> > > > > > a property and handle it from the driver.
> > > > > And by "that" you mean which of those two I listed , the
> > > > > "snps,dw-mshc-vN" or adding new DT prop ?
> > > > > 
> > > > I meant to add a new property, not a new compatible, but that's just
> > > > my experience.
> > > > 
> > > > Let me say it __might__ happen that a particular change you need is
> > > > specific to a particular version of the DWMMC IP (query Synopsys
> > > > by the way), but more probably it might be e.g. the same IP version with
> > > > a different reduced or extended configuration or a minor fix/improvement
> > > > to the IP block without resulting version number bump.
> > > > 
> > > > For example I don't remember that errata fixes in IP blocks result in
> > > > a new compatible, instead there are quite common optional "quirk"
> > > > properties for broken IPs -- e.g. check bindings/usb/dwc3.txt :)
> > > Right, this very much matches how I see it as well. Thanks for confirming.
> > > 
> > > Alexey, can you tell us if the requirement for setting
> > > SDMMC_CMD_USE_HOLD_REG came with some new revision of the core or
> > > disappeared with some revision OR if this is some configuration
> > > option of the core during synthesis ?
> > Sorry for not following that discussion during my weekend but I'll try
> > to address all questions now.
> SDMMC_CMD_USE_HOLD_REG didn't come with new revision..It's using continuously.
> But it's difficult to use the generic feature..because it's considered the below things.
> 
> If Card is SDR50/SDR104/DDR50 mode..
> 	1) and phase shift of cclk_in_drv is 0 then SDMMC_CMD_USE_HOLD_REG bit is set to 0,
> 	2) and phase shift of cclk_in_drv > 0 then SDMMC_CMD_USE_HOLD_REG bit is set to 1,
> If Card is SDR12/SDR25 mode, then this bit is set to 1.

So card type is also important here and for certain card type we don't need to
set SDMMC_CMD_USE_HOLD_REG, right?

> We need to check phase shift scheme..but as i knew, each SoC have been implemented differently for phase shift.
> (Phase shift have dependency to SoC.)

Given my assumption above we need to check 2 things:
 * Card type
 * SoC-specific implementation detail (phase shift scheme)

> And it have to check HCON register..there is IMPLEMENT_HOLD_REG(bit[22]).
> (It described whether IP have hold register or not)

Ah actually 3 things
 + IMPLEMENT_HOLD_REG

> I didn't read this thread entirely.
> I'm not sure what you have discussed..but my understanding is right..i recommend to use "snps,dw-mshc" for ARC compat
> string.
> Otherwise it need to add "dw_mmc-<SoC>.c". dw_mmc-pltfm.c should provide the basic dw-mmc controller functionality.

Hm, interesting looks like you already made some changes here:
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=aaaaeb7a933471f6413ca44dd36efd57f2fa9429

So now driver checks if SoC has HOLD REG then SDMMC_CMD_USE_HOLD_REG will be set
(regardless card type).

And what's interesting and connected to this discussion since mentioned commit
there's no point in having both "altr,socfpga-dw-mshc" and "img,pistachio-dw-mshc"
compat strings because the do nothing now. I.e. it's time to replace both mentioned
compat strings with generic "snps,dw-mshc".

Anybody volunteers for that .dts* cleanup?

-Alexey