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

[Jisheng Zhang]

On Fri, 11 Nov 2016 11:22:43 +0800 Jisheng Zhang wrote:

> 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.

Personally, I prefer solA: use the IP name as compatible string. This is IP
specific rather than SoC specific. The HW itself supports two clks

Thanks,
Jisheng

> 
> 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