| Message ID | 1376498884-9199-2-git-send-email-dinguyen@altera.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. That way, the MSHC driver will work fine if a HW designer has dropped the MSHC IP block into a completely different SoC with a different syscon register layout.
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. > > That way, the MSHC driver will work fine if a HW designer has dropped > the MSHC IP block into a completely different SoC with a different > syscon register layout. > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
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.
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 >
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..eaf98cd --- /dev/null +++ b/Documentation/devicetree/bindings/mmc/socfpga-dw-mshc.txt @@ -0,0 +1,56 @@ +* 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. + +* 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. + +Example: + dwmmc0@ff704000 { + compatible = "altr,socfpga-dw-mshc", "snps,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"; + num-slots = <1>; + supports-highspeed; + broken-cd; + altr,sysmgr = <&system_mgr>; + altr,dw-mshc-sdr-timing = <0 3>; + + slot@0 { + reg = <0>; + bus-width = <4>; + }; + }; diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi index c31c8e9..9706767 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"; diff --git a/arch/arm/boot/dts/socfpga_cyclone5.dts b/arch/arm/boot/dts/socfpga_cyclone5.dts index 973999d..698dde9 100644 --- a/arch/arm/boot/dts/socfpga_cyclone5.dts +++ b/arch/arm/boot/dts/socfpga_cyclone5.dts @@ -54,6 +54,19 @@ status = "okay"; }; + dwmmc0@ff704000 { + num-slots = <1>; + supports-highspeed; + broken-cd; + altr,sysmgr = <&system_mgr>; + 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>; };