Re: [PATCH 5/10] dt: bindings: Add bindings for Marvell Xenon SD Host Controller

[Jisheng Zhang]

Hi Rob, Ziji,

On Thu, 10 Nov 2016 19:44:19 +0800 Ziji Hu wrote:

> Hi Rob,
> 
> On 2016/11/10 2:24, Rob Herring wrote:
> > On Mon, Oct 31, 2016 at 12:09:54PM +0100, Gregory CLEMENT wrote:  
> >> From: Ziji Hu <huziji@xxxxxxxxxxx>
> >>
> >> Marvell Xenon SDHC can support eMMC/SD/SDIO.
> >> Add Xenon-specific properties.
> >> Also add properties for Xenon PHY setting.
> >>
> >> Signed-off-by: Hu Ziji <huziji@xxxxxxxxxxx>
> >> Signed-off-by: Gregory CLEMENT <gregory.clement@xxxxxxxxxxxxxxxxxx>
> >> ---
> >>  Documentation/devicetree/bindings/mmc/marvell,xenon-sdhci.txt | 161 +++++++-
> >>  MAINTAINERS                                                   |   1 +-
> >>  2 files changed, 162 insertions(+), 0 deletions(-)
> >>  create mode 100644 Documentation/devicetree/bindings/mmc/marvell,xenon-sdhci.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/mmc/marvell,xenon-sdhci.txt b/Documentation/devicetree/bindings/mmc/marvell,xenon-sdhci.txt
> >> new file mode 100644
> >> index 000000000000..0d2d139494d3
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/mmc/marvell,xenon-sdhci.txt
> >> @@ -0,0 +1,161 @@
> >> +Marvell's Xenon SDHCI Controller device tree bindings
> >> +This file documents differences between the core mmc properties
> >> +described by mmc.txt and the properties used by the Xenon implementation.
> >> +
> >> +A single Xenon IP can support multiple slots.
> >> +Each slot acts as an independent SDHC. It owns independent resources, such
> >> +as register sets clock and PHY.
> >> +Each slot should have an independent device tree node.
> >> +
> >> +Required Properties:
> >> +- compatible: should be one of the following
> >> +  - "marvell,armada-3700-sdhci": For controllers on Armada-3700 SOC.
> >> +  Must provide a second register area and marvell,pad-type.
> >> +  - "marvell,xenon-sdhci": For controllers on all the SOCs, other than
> >> +  Armada-3700.  
> > 
> > Need SoC specific compatible strings.
> >   
> 
> 	Xenon SDHC is a common IP for all Marvell SOCs.
> 	It is difficult to use a single SOC specific compatible to represent Xenon SDHC.
> 	There will be so many SOC compatible strings in list if each specific SOC owns a compatible.
> 	Actually only few SOCs require special properties.
> 	Any suggestion please?
> 
> >> +
> >> +- clocks:
> >> +  Array of clocks required for SDHCI.
> >> +  Requires at least one for Xenon IP core.
> >> +  Some SOCs require additional clock for AXI bus.
> >> +
> >> +- clock-names:
> >> +  Array of names corresponding to clocks property.
> >> +  The input clock for Xenon IP core should be named as "core".
> >> +  The optional AXI clock should be named as "axi".  
> > 
> > When is AXI clock optional? This should be required for ?? compatible 
> > strings.
> >   
> 	It is required on some SOCs.
> 	I will double check if a suitable compatible string can be determined for those SOCs.

Besides the core clk, berlin SoCs have one AXI clock. Usually, we have two
solutions:

solA: as current patch does, take "marvell,xenon-sdhci" as compatible string
and make the AXI clock property optional. Usually for berlin SoCs, we don't need
special properties.

PS: this solution is also what sdhci-pxav3.c takes

solB: As Rob said, add extra SoC compatible strings, so we'll have
something like:

static const struct of_device_id sdhci_xenon_of_match[] = {
	{ .compatible = "marvell,armada-3700-sdhci", },
	{ .compatible = "marvell,berlin4ct-sdhci", },
	...
	{ .compatible = "marvell,berlinxxx-mmc", },
}

then we take care the AXI clk for berlin SoCs in the code.


Which solution do you prefer?

Thanks,
Jisheng