Re: [PATCHv4 2/3] ARM: socfpga: dts: Add support for SD/MMC

[Dinh Nguyen <dinguyen@altera.com>]

On Thu, 2013-08-22 at 14:13 -0600, Stephen Warren wrote:
> On 08/21/2013 01:48 PM, Dinh Nguyen wrote:
> > On Fri, 2013-08-16 at 16:36 -0600, Stephen Warren wrote:
> >> On 08/14/2013 10:48 AM, dinguyen@altera.com wrote:
> >>> From: Dinh Nguyen <dinguyen@altera.com>
> >>>
> >>> Add bindings for SD/MMC for SOCFPGA.
> >>
> >>> diff --git a/Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt b/Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt
> >>
> >>> +* altr,sysmgr: Should be the phandle to the system_mgr node. As this is where
> >>> +		this where the register that controls the CIU clock phases
> >>> +		reside.
> >>
> >> On the surface, this binding series seems OK, but I do have a question:
> >> how is the sysmgr phandle used?
> >>
> >> I assume there's some register in this syscon device that resets or
> >> enables or otherwise controls this MSHC module. How does the code know
> >> which register it is? The phandle in the altr,sysmgr property would
> >> usually be followed by a/some cell(s) that encode this information, so
> >> that the MSHC driver doesn't have to know anything about the layout of
> >> the syscon registers, and so the sysconf driver doesn't have to know
> >> anything about the identity of the MSHC client device.
> > 
> > There is a #define SYSMGR_SDMMCGRP_CTRL_OFFSET that is in
> > dw_mmc-socfpga.c. This defines the offset from the base address that the
> > sysmgr phandle will give me.
> 
> Hmmm. That doesn't sound good. That means that the SDMMC driver knows
> internal details about some other HW module. It'd be better if either:
> 
> a) The sysmgr driver was required to provide an API to the SDMMC driver
> to set up the CIU register as requested.
> 
> or:
> 
> b) The CIU register details were represented in DT.
> 
> Either of these would allow the SDMMC driver to operate unchanged on an
> SoC with a different sysmgr register layout.

Thanks Stephen...will rework accordingly.

Dinh
>