[PATCHv2] ARM: socfpga: dts: Add support for SD/MMC

[dinguyen@altera.com (Dinh Nguyen)]

On Thu, 2013-08-08 at 15:13 -0600, Stephen Warren wrote:
> On 08/08/2013 02:54 PM, Dinh Nguyen wrote:
> > On Thu, 2013-08-08 at 14:37 -0600, Stephen Warren wrote:
> >> On 08/08/2013 02:32 PM, Dinh Nguyen wrote:
> >>> On Thu, 2013-08-08 at 14:14 -0600, Stephen Warren wrote:
> >>>> On 08/05/2013 02:43 PM, dinguyen at altera.com wrote:
> >>>>> From: Dinh Nguyen <dinguyen@altera.com>
> >>>>>
> >>>>> Add bindings for SD/MMC for SOCFPGA.
> >>>>> Add "syscon" to the "altr,sys-mgr" binding.
> >>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt b/Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt
> >>>>> new file mode 100644
> >>>>> index 0000000..dc14922
> >>>>> --- /dev/null
> >>>>> +++ b/Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt
> >>>>> @@ -0,0 +1,48 @@
> >>>>> +* Altera SOCFPGA specific extensions to the Synopsis Designware Mobile
> >>>>> +  Storage Host Controller
> >>>>> +
> >>>>> +Required Properties:
> >>>>> +
> >>>>> +* compatible: should be
> >>>>> +	- "altr,socfpga-dw-mshc": for controllers with Altera SOCFPGA
> >>>>> +	  specific extensions.
> >>>>> +
> >>>>> +* altr,dw-mshc-ciu-div: Specifies the divider value for the card interface
> >>>>> +  unit (ciu) clock. The value should be (n-1). For Altera's SOCFPGA, the divider
> >>>>> +  value is fixed at 3, which means parent_clock/4.
> >>>>
> >>>> This feels like something that should be represented using the common
> >>>> clock API; a driver should query the rate of its input clock, and then
> >>>> calculate the MMC block's internal divider based on that (perhaps also
> >>>> call clk_set_rate() on the input clock?).
> >>>
> >>> This means a change to the dw_mmc driver, which I can look into for the
> >>> next round? I have promised Pawel to consolidate the bindings for both
> >>> exynos and socfpga in the next round already. I will also look into
> >>> using the common clock API for the MMC as well. 
> >>>
> >>> This patch is the only thing that is preventing from SD/MMC working for
> >>> SOCFPGA in the mainline, can I get your Ack if I look into doing this
> >>> for 3.13 for both the exynos and socfpga driver, and address your latter
> >>> comments?
> >>
> >> The problem is that if the binding supports or requires that property
> >> now, it has to at least support it forever. Given that we're getting
> >> serious about DT ABI now, we should be only introducing DT bindings that
> >> we believe are complete and corrrect, rather than bindings which we
> >> actively expect to be temporary and to change incompatibly later.
> >>
> > Right ok. Then I guess I will have to look into consolidating the
> > bindings sooner rather than later.
> > 
> > I went back to look at the driver again, and I think it is doing as you
> > are suggesting:
> > 
> > host->bus_hz is getting its input value from the common clock API.
> > "altr,dw-mshc-ciu-div" is specifying the internal divider that is in the
> > SD/MMC IP itself.
> > 
> > I guess I'm still not clear on how I can represent the SD/MMC divider in
> > in the context of the common clock API.
> 
> Why is there a need to directly represent the divider anywhere? The
> driver can find the rate of the input clock, and I assume it knows what
> rate it wants the clock to run at, so can't it calculate the divider
> based on those two pieces of information?

CC: Chris Ball
> 
> Or, does the driver really not know what rate it wants the clock to be
> after the internal divider? If not, then I think that *rate* should be
> recorded in DT, not the divider to obtain that rate.
> 

I believe that this is the case, that the driver does not know what rate
it will clock the SD card at. I think internally we did have a "bus_hz"
in DT a while back. I guess I don't really understand why it would be
better to have a *rate* DT entry instead of a fixed-divider entry?

Dinh
>