| Message ID | 1376322589-17606-1-git-send-email-dinguyen@altera.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 08/12/2013 09:49 AM, dinguyen@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 I think it's reasonable to define one binding as being based on another plus some additions, so, the binding, Acked-by: Stephen Warren <swarren@nvidia.com> > +Example: > + dwmmc0@ff704000 { > + compatible = "altr,socfpga-dw-mshc", "snps,dw-mshc"; > + altr,dw-mshc-sdr-timing = <0 3>; > + }; It'd be nice to provide a complete example though, rather than only including the properties that this binding adds to the base binding. > diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi > sysmgr@ffd08000 { > - compatible = "altr,sys-mgr"; > + compatible = "altr,sys-mgr", "syscon"; That seems like an unrelated change?
On Tue, 2013-08-13 at 13:52 -0600, Stephen Warren wrote: > On 08/12/2013 09:49 AM, dinguyen@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 > > I think it's reasonable to define one binding as being based on another > plus some additions, so, the binding, > Acked-by: Stephen Warren <swarren@nvidia.com> Thanks Stephen! > > > +Example: > > + dwmmc0@ff704000 { > > + compatible = "altr,socfpga-dw-mshc", "snps,dw-mshc"; > > + altr,dw-mshc-sdr-timing = <0 3>; > > + }; > > It'd be nice to provide a complete example though, rather than only > including the properties that this binding adds to the base binding. I'll add a complete binding. > > > diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi > > > sysmgr@ffd08000 { > > - compatible = "altr,sys-mgr"; > > + compatible = "altr,sys-mgr", "syscon"; > > That seems like an unrelated change? This patch is to enable SD/MMC for SOCFPGA. The "syscon" is needed in dw_mmc-socfpga.c as the clock phases for the SD IP is controlled by registers in "altr,sys-mgr". Thanks, Dinh >
On 08/13/2013 01:58 PM, Dinh Nguyen wrote: > On Tue, 2013-08-13 at 13:52 -0600, Stephen Warren wrote: >> On 08/12/2013 09:49 AM, dinguyen@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/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi >> >>> sysmgr@ffd08000 { >>> - compatible = "altr,sys-mgr"; >>> + compatible = "altr,sys-mgr", "syscon"; >> >> That seems like an unrelated change? > > This patch is to enable SD/MMC for SOCFPGA. The "syscon" is needed in > dw_mmc-socfpga.c as the clock phases for the SD IP is controlled by > registers in "altr,sys-mgr". But nothing in the dwmmc nodes refers to the syscon node, so how can the syscon driver possibly provide services to the MMC driver; the MMC driver won't be able to find it. I hope there's no back-door assumption that some syscon exists, and that there's only one of them...
On Tue, 2013-08-13 at 14:10 -0600, Stephen Warren wrote: > On 08/13/2013 01:58 PM, Dinh Nguyen wrote: > > On Tue, 2013-08-13 at 13:52 -0600, Stephen Warren wrote: > >> On 08/12/2013 09:49 AM, dinguyen@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/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi > >> > >>> sysmgr@ffd08000 { > >>> - compatible = "altr,sys-mgr"; > >>> + compatible = "altr,sys-mgr", "syscon"; > >> > >> That seems like an unrelated change? > > > > This patch is to enable SD/MMC for SOCFPGA. The "syscon" is needed in > > dw_mmc-socfpga.c as the clock phases for the SD IP is controlled by > > registers in "altr,sys-mgr". > > But nothing in the dwmmc nodes refers to the syscon node, so how can the > syscon driver possibly provide services to the MMC driver; the MMC > driver won't be able to find it. I hope there's no back-door assumption > that some syscon exists, and that there's only one of them... In dw_mmc-socfpga.c: priv->sysreg = syscon_regmap_lookup_by_compatible("altr,sys-mgr"); then, uses regmap_write() to set the correct values from "altr,dw-mshc-sdr-timing". This was a suggestion from Arnd. It seems that this part of patch is causing alot of confusion, I should just split the patches out? Dinh >
On 08/13/2013 02:17 PM, Dinh Nguyen wrote: > On Tue, 2013-08-13 at 14:10 -0600, Stephen Warren wrote: >> On 08/13/2013 01:58 PM, Dinh Nguyen wrote: >>> On Tue, 2013-08-13 at 13:52 -0600, Stephen Warren wrote: >>>> On 08/12/2013 09:49 AM, dinguyen@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/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi >>>> >>>>> sysmgr@ffd08000 { >>>>> - compatible = "altr,sys-mgr"; >>>>> + compatible = "altr,sys-mgr", "syscon"; >>>> >>>> That seems like an unrelated change? >>> >>> This patch is to enable SD/MMC for SOCFPGA. The "syscon" is needed in >>> dw_mmc-socfpga.c as the clock phases for the SD IP is controlled by >>> registers in "altr,sys-mgr". >> >> But nothing in the dwmmc nodes refers to the syscon node, so how can the >> syscon driver possibly provide services to the MMC driver; the MMC >> driver won't be able to find it. I hope there's no back-door assumption >> that some syscon exists, and that there's only one of them... > > In dw_mmc-socfpga.c: > > priv->sysreg = syscon_regmap_lookup_by_compatible("altr,sys-mgr"); > > then, uses regmap_write() to set the correct values from > "altr,dw-mshc-sdr-timing". Uggh. There should really be a phandle from the MMC node to the syscon node to specify the connectivity. What if there ends up being 2 HW modules that are sys-mgr? At the very least, the binding for any device that's affected by the sys-mgr should specify that some other node must exist with that compatible value, to provide XXX services for it. But, I wonder if the altr,dw-mshc-sdr-timing property shouldn't be part of the syscon node itself? (Probably best to ignore my ack for now given this new information) > This was a suggestion from Arnd. It seems that this part of patch is > causing alot of confusion, I should just split the patches out? A separate patch to change that compatible value probably would make sense; by the sound of it, that change would also be required for a lot of other devices on the SoC; it's not just specific to MMC?
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..d588bf9 --- /dev/null +++ b/Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt @@ -0,0 +1,36 @@ +* Altera SOCFPGA specific extensions to the Synopsis Designware Mobile + Storage Host Controller + +The Synopsis designware mobile storage host controller is used to interface +a SoC with storage medium such as eMMC or SD/MMC cards. This file documents +differences between the core Synopsis dw mshc controller properties described +by synopsis-dw-mshc.txt and the properties used by the 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-sdr-timing: Specifies the value of CIU clock phase shift value + in transmit mode and CIU clock phase shift value in receive mode for single + data rate mode operation. Refer to notes below for the order of the cells and the + valid values. + + Notes for the sdr-timing values: + + The order of the cells should be + - First Cell: CIU clock phase shift value for RX mode, smplsel bits in + the system manager SDMMC control group. + - Second Cell: CIU clock phase shift value for TX mode, drvsel bits in + the system manager SDMMC control group. + + Valid values for SDR CIU clock timing for SOCFPGA: + - valid value for tx phase shift and rx phase shift is 0 to 7. + +Example: + dwmmc0@ff704000 { + compatible = "altr,socfpga-dw-mshc", "snps,dw-mshc"; + altr,dw-mshc-sdr-timing = <0 3>; + }; diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi index bee62a2..dbf7f22 100644 --- a/arch/arm/boot/dts/socfpga.dtsi +++ b/arch/arm/boot/dts/socfpga.dtsi @@ -468,6 +468,17 @@ cache-level = <2>; }; + mmc: dwmmc0@ff704000 { + compatible = "altr,socfpga-dw-mshc"; + reg = <0xff704000 0x1000>; + interrupts = <0 139 4>; + fifo-depth = <0x400>; + #address-cells = <1>; + #size-cells = <0>; + clocks = <&l4_mp_clk>, <&sdmmc_clk>; + clock-names = "biu", "ciu"; + }; + /* Local timer */ timer@fffec600 { compatible = "arm,cortex-a9-twd-timer"; @@ -521,7 +532,7 @@ }; sysmgr@ffd08000 { - compatible = "altr,sys-mgr"; + compatible = "altr,sys-mgr", "syscon"; reg = <0xffd08000 0x4000>; }; }; diff --git a/arch/arm/boot/dts/socfpga_cyclone5.dts b/arch/arm/boot/dts/socfpga_cyclone5.dts index 973999d..526df6f 100644 --- a/arch/arm/boot/dts/socfpga_cyclone5.dts +++ b/arch/arm/boot/dts/socfpga_cyclone5.dts @@ -54,6 +54,18 @@ status = "okay"; }; + dwmmc0@ff704000 { + num-slots = <1>; + supports-highspeed; + broken-cd; + altr,dw-mshc-sdr-timing = <0 3>; + + slot@0 { + reg = <0>; + bus-width = <4>; + }; + }; + timer0@ffc08000 { clock-frequency = <100000000>; }; diff --git a/arch/arm/boot/dts/socfpga_vt.dts b/arch/arm/boot/dts/socfpga_vt.dts index d1ec0ca..6f23121 100644 --- a/arch/arm/boot/dts/socfpga_vt.dts +++ b/arch/arm/boot/dts/socfpga_vt.dts @@ -46,6 +46,17 @@ status = "okay"; }; + dwmmc0@ff704000 { + num-slots = <1>; + supports-highspeed; + broken-cd; + + slot@0 { + reg = <0>; + bus-width = <4>; + }; + }; + timer0@ffc08000 { clock-frequency = <7000000>; }; diff --git a/drivers/mmc/host/dw_mmc-socfpga.c b/drivers/mmc/host/dw_mmc-socfpga.c index 14b5961..0cff75d 100644 --- a/drivers/mmc/host/dw_mmc-socfpga.c +++ b/drivers/mmc/host/dw_mmc-socfpga.c @@ -31,7 +31,6 @@ /* SOCFPGA implementation specific driver private data */ struct dw_mci_socfpga_priv_data { - u8 ciu_div; /* card interface unit divisor */ u32 hs_timing; /* bitmask for CIU clock phase shift */ struct regmap *sysreg; /* regmap for system manager register */ }; @@ -64,8 +63,6 @@ static int dw_mci_socfpga_setup_clock(struct dw_mci *host) regmap_write(priv->sysreg, SYSMGR_SDMMCGRP_CTRL_OFFSET, priv->hs_timing); clk_prepare_enable(host->ciu_clk); - - host->bus_hz /= (priv->ciu_div + 1); return 0; } @@ -82,14 +79,8 @@ static int dw_mci_socfpga_parse_dt(struct dw_mci *host) struct dw_mci_socfpga_priv_data *priv = host->priv; struct device_node *np = host->dev->of_node; u32 timing[2]; - u32 div = 0; int ret; - ret = of_property_read_u32(np, "altr,dw-mshc-ciu-div", &div); - if (ret) - dev_info(host->dev, "No dw-mshc-ciu-div specified, assuming 1"); - priv->ciu_div = div; - ret = of_property_read_u32_array(np, "altr,dw-mshc-sdr-timing", timing, 2); if (ret)