| Message ID | 1540996688-23681-1-git-send-email-aisheng.dong@nxp.com |
|---|---|
| Headers | show |
| Series | ARM: imx: add imx7ulp support | expand |
Hi Dong, On Wed, Oct 31, 2018 at 11:46 AM A.s. Dong <aisheng.dong@nxp.com> wrote: > + reg_vsd_3v3: regulator-vsd-3v3 { > + compatible = "regulator-fixed"; > + regulator-name = "VSD_3V3"; > + regulator-min-microvolt = <3300000>; > + regulator-max-microvolt = <3300000>; > + pinctrl-names = "default"; > + pinctrl-0 = <&pinctrl_usdhc0_rst>; > + gpio = <&gpio_ptd 0 GPIO_ACTIVE_HIGH>; > + enable-active-high; You model this as a regulator... > +&usdhc0 { > + pinctrl-names = "default"; > + pinctrl-0 = <&pinctrl_usdhc0>; > + cd-gpios = <&gpio_ptc 10 GPIO_ACTIVE_LOW>; > + vmmc-supply = <®_vsd_3v3>; but as this pins controls the reset of the SD port it seems that using mmc-pwrseq would be more appropriate. This way you could pass 'reset-gpios' inside the pwrseq node, which would describe the hardware more accurately.
On Thu, Nov 1, 2018 at 7:28 AM Fabio Estevam <festevam@gmail.com> wrote: > > Hi Dong, > > On Wed, Oct 31, 2018 at 11:46 AM A.s. Dong <aisheng.dong@nxp.com> wrote: > > > + reg_vsd_3v3: regulator-vsd-3v3 { > > + compatible = "regulator-fixed"; > > + regulator-name = "VSD_3V3"; > > + regulator-min-microvolt = <3300000>; > > + regulator-max-microvolt = <3300000>; > > + pinctrl-names = "default"; > > + pinctrl-0 = <&pinctrl_usdhc0_rst>; > > + gpio = <&gpio_ptd 0 GPIO_ACTIVE_HIGH>; > > + enable-active-high; > > You model this as a regulator... > > > +&usdhc0 { > > + pinctrl-names = "default"; > > + pinctrl-0 = <&pinctrl_usdhc0>; > > + cd-gpios = <&gpio_ptc 10 GPIO_ACTIVE_LOW>; > > + vmmc-supply = <®_vsd_3v3>; > > but as this pins controls the reset of the SD port it seems that using > mmc-pwrseq would be more appropriate. > > This way you could pass 'reset-gpios' inside the pwrseq node, which > would describe the hardware more accurately. Just looked at the schematics and the SD0_nRST signals is only used when the eMMC is populated. As you are only defining SD0_DATA0-DATA3 it means you are using the microSD option (option 1 as per the schematics). In the microSD option the SD0_nRST is not used, so better not to describe it in device tree.
> -----Original Message----- > From: Fabio Estevam [mailto:festevam@gmail.com] > Sent: Thursday, November 1, 2018 6:28 PM [...] > Hi Dong, > > On Wed, Oct 31, 2018 at 11:46 AM A.s. Dong <aisheng.dong@nxp.com> > wrote: > > > + reg_vsd_3v3: regulator-vsd-3v3 { > > + compatible = "regulator-fixed"; > > + regulator-name = "VSD_3V3"; > > + regulator-min-microvolt = <3300000>; > > + regulator-max-microvolt = <3300000>; > > + pinctrl-names = "default"; > > + pinctrl-0 = <&pinctrl_usdhc0_rst>; > > + gpio = <&gpio_ptd 0 GPIO_ACTIVE_HIGH>; > > + enable-active-high; > > You model this as a regulator... > > > +&usdhc0 { > > + pinctrl-names = "default"; > > + pinctrl-0 = <&pinctrl_usdhc0>; > > + cd-gpios = <&gpio_ptc 10 GPIO_ACTIVE_LOW>; > > + vmmc-supply = <®_vsd_3v3>; > > but as this pins controls the reset of the SD port it seems that using > mmc-pwrseq would be more appropriate. > > This way you could pass 'reset-gpios' inside the pwrseq node, which would > describe the hardware more accurately. This seems a bit confusing to me. That GPIO is used to control the SD card power on/off. So it's naturally a GPIO regulator. Looking at exist IMX dts, you will find all similar board doing like this. Pwrseq seems more like to be used for WiFi/eMMC card. Am I missed something? Regards Dong Aisheg
> -----Original Message----- > From: Fabio Estevam [mailto:festevam@gmail.com] > Sent: Thursday, November 1, 2018 6:36 PM [...] > > On Thu, Nov 1, 2018 at 7:28 AM Fabio Estevam <festevam@gmail.com> > wrote: > > > > Hi Dong, > > > > On Wed, Oct 31, 2018 at 11:46 AM A.s. Dong <aisheng.dong@nxp.com> > wrote: > > > > > + reg_vsd_3v3: regulator-vsd-3v3 { > > > + compatible = "regulator-fixed"; > > > + regulator-name = "VSD_3V3"; > > > + regulator-min-microvolt = <3300000>; > > > + regulator-max-microvolt = <3300000>; > > > + pinctrl-names = "default"; > > > + pinctrl-0 = <&pinctrl_usdhc0_rst>; > > > + gpio = <&gpio_ptd 0 GPIO_ACTIVE_HIGH>; > > > + enable-active-high; > > > > You model this as a regulator... > > > > > +&usdhc0 { > > > + pinctrl-names = "default"; > > > + pinctrl-0 = <&pinctrl_usdhc0>; > > > + cd-gpios = <&gpio_ptc 10 GPIO_ACTIVE_LOW>; > > > + vmmc-supply = <®_vsd_3v3>; > > > > but as this pins controls the reset of the SD port it seems that using > > mmc-pwrseq would be more appropriate. > > > > This way you could pass 'reset-gpios' inside the pwrseq node, which > > would describe the hardware more accurately. > > Just looked at the schematics and the SD0_nRST signals is only used when the > eMMC is populated. > > As you are only defining SD0_DATA0-DATA3 it means you are using the > microSD option (option 1 as per the schematics). > > In the microSD option the SD0_nRST is not used, so better not to describe it in > device tree. Can you tell which version were you looking at? >From my version SPF-29163_A1, it's populated by default. Regards Dong Aisheng
On Thu, Nov 1, 2018 at 12:49 PM A.s. Dong <aisheng.dong@nxp.com> wrote: > This seems a bit confusing to me. > That GPIO is used to control the SD card power on/off. So it's naturally a > GPIO regulator. Looking at exist IMX dts, you will find all similar board doing > like this. Pwrseq seems more like to be used for WiFi/eMMC card. > Am I missed something? As per the board schematics the name of the signal is SD0_nRST, where RST means "reset". There is even this note in the schematics: "ROM Code will reset the SD power during boot up through SD_RST". eMMC reset is better handled by 'reset-gpios' property from pwrseq.
On Thu, Nov 1, 2018 at 12:53 PM A.s. Dong <aisheng.dong@nxp.com> wrote: > Can you tell which version were you looking at? > From my version SPF-29163_A1, it's populated by default. Mine is sch-29163-b and it shows both micro SD card and eMMC on the schematics. On my board I see only the micro SD card populated, not the eMMC. Also, if you are describing the eMMC then why do you pass the card detect property and only 4 data lines? SD0_nRST is only connected to the eMMC card. eMMC does not have card detect signal and it uses 8 data lines, so clearly there is something inconsistent in your dts. What exactly do you want to describe: the SD card or the eMMC?
> -----Original Message----- > From: Fabio Estevam [mailto:festevam@gmail.com] > Sent: Thursday, November 1, 2018 11:57 PM [...] > > On Thu, Nov 1, 2018 at 12:49 PM A.s. Dong <aisheng.dong@nxp.com> wrote: > > > This seems a bit confusing to me. > > That GPIO is used to control the SD card power on/off. So it's > > naturally a GPIO regulator. Looking at exist IMX dts, you will find > > all similar board doing like this. Pwrseq seems more like to be used for > WiFi/eMMC card. > > Am I missed something? > > As per the board schematics the name of the signal is SD0_nRST, where RST > means "reset". > I guess you may also notice that circuit name above that signal is " Power Switch for SD3.0". > There is even this note in the schematics: "ROM Code will reset the SD power > during boot up through SD_RST". > Yes, but it's more like a RESET. > eMMC reset is better handled by 'reset-gpios' property from pwrseq. It's not eMMC. Let's see more in binding doc: Required properties: - compatible : contains "mmc-pwrseq-simple". Optional properties: - reset-gpios : contains a list of GPIO specifiers. The reset GPIOs are asserted at initialization and prior we start the power up procedure of the card. They will be de-asserted right after the power has been provided to the card. That looks more like an auxiliary GPIO reset pin which is used along with normal power up process (see. mmc_power_up ()) which may be commonly used for WiFi cards. But for SD card with a single supply, GPIO regulator seems enough to me. That's also the using in past years. Are things changed? Regards Dong Aisheng
> -----Original Message----- > From: Fabio Estevam [mailto:festevam@gmail.com] > Sent: Friday, November 2, 2018 12:01 AM [...] > > On Thu, Nov 1, 2018 at 12:53 PM A.s. Dong <aisheng.dong@nxp.com> wrote: > > > Can you tell which version were you looking at? > > From my version SPF-29163_A1, it's populated by default. > > Mine is sch-29163-b and it shows both micro SD card and eMMC on the > schematics. > I will check it later. But Rev B may be supported later if changed. > On my board I see only the micro SD card populated, not the eMMC. > > Also, if you are describing the eMMC then why do you pass the card detect > property and only 4 data lines? > > SD0_nRST is only connected to the eMMC card. > > eMMC does not have card detect signal and it uses 8 data lines, so clearly > there is something inconsistent in your dts. > > What exactly do you want to describe: the SD card or the eMMC? I guess we're talking about different schematic versions. The patch is supposed to support Rev A. Regards Dong Aisheng
[...] > > -----Original Message----- > > From: Fabio Estevam [mailto:festevam@gmail.com] > > Sent: Friday, November 2, 2018 12:01 AM > [...] > > > > On Thu, Nov 1, 2018 at 12:53 PM A.s. Dong <aisheng.dong@nxp.com> > wrote: > > > > > Can you tell which version were you looking at? > > > From my version SPF-29163_A1, it's populated by default. > > > > Mine is sch-29163-b and it shows both micro SD card and eMMC on the > > schematics. > > > > I will check it later. But Rev B may be supported later if changed. > > > On my board I see only the micro SD card populated, not the eMMC. > > > > Also, if you are describing the eMMC then why do you pass the card > > detect property and only 4 data lines? > > > > SD0_nRST is only connected to the eMMC card. > > > > eMMC does not have card detect signal and it uses 8 data lines, so > > clearly there is something inconsistent in your dts. > > > > What exactly do you want to describe: the SD card or the eMMC? > > I guess we're talking about different schematic versions. > The patch is supposed to support Rev A. > I just checked RevB schematic and SD card circuit seems the same as Rev A that SD0_nRST is used to control the SD power and there's CD pin. So the SD binding should work for both. Regards Dong Aisheng
On Thu, Nov 1, 2018 at 1:21 PM A.s. Dong <aisheng.dong@nxp.com> wrote: > But for SD card with a single supply, GPIO regulator seems enough to me. > That's also the using in past years. Are things changed? Ok, now it clear you are describing the micro SD port and not the eMMC, so: Reviewed-by: Fabio Estevam <festevam@gmail.com>
On Wed, Oct 31, 2018 at 3:43 PM A.s. Dong <aisheng.dong@nxp.com> wrote: > On some SoCs(e.g. MX7ULP), GPIO clock is gatable and maybe > disabled by default. Users have to make sure it's enabled before > being able to access controller registers, otherwise an external > abort error may occur. Let's add the optional clocks property to > handle this case. > > For ULP GPIO clock, it includes two separate clocks: one is for > GPIO controller Input/Output function clock while another is > GPIO port control clock for interrupt function. > > Cc: Linus Walleij <linus.walleij@linaro.org> > Cc: Mark Rutland <mark.rutland@arm.com> > Cc: Stefan Agner <stefan@agner.ch> > Cc: linux-gpio@vger.kernel.org > Cc: devicetree@vger.kernel.org > Reviewed-by: Rob Herring <robh@kernel.org> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> Patch applied. Yours, Linus Walleij
On Wed, Oct 31, 2018 at 3:43 PM A.s. Dong <aisheng.dong@nxp.com> wrote: > + clk_port = devm_clk_get(&pdev->dev, "port"); > + if (clk_port == ERR_PTR(-EPROBE_DEFER)) > + return -EPROBE_DEFER; > + > + clk_gpio = devm_clk_get(&pdev->dev, "gpio"); > + if (clk_gpio == ERR_PTR(-EPROBE_DEFER)) > + return -EPROBE_DEFER; > + > + if (!IS_ERR_OR_NULL(clk_port)) { > + ret = clk_prepare_enable(clk_port); > + if (ret) > + return ret; > + } > + > + if (!IS_ERR_OR_NULL(clk_gpio)) { > + ret = clk_prepare_enable(clk_gpio); > + if (ret) { > + clk_disable_unprepare(clk_port); > + return ret; > + } > + } I think IS_ERR_OR_NULL() is considered harmful among other things. What about this pattern: clk = devm_clk_get(dev, "foo"); if (!IS_ERR(clk)) { ret = clk_prepare_enable(clk); if (ret) return ret; } else if (PTR_ERR(clk) == -EPROBE_DEFER) { /* * Percolate deferrals, for anything else, * just live without the clocking. */ return PTR_ERR(clk); } You also need to introduce code to disable the clock on .remove() or the clock will always be on after this module has probed. This applies also to builtin drivers, unless you suppress the sysfs attributes and use platform_driver_probe() Yours, Linus Walleij
> -----Original Message----- > From: Linus Walleij [mailto:linus.walleij@linaro.org] > Sent: Friday, November 2, 2018 5:33 PM [...] > On Wed, Oct 31, 2018 at 3:43 PM A.s. Dong <aisheng.dong@nxp.com> wrote: > > > + clk_port = devm_clk_get(&pdev->dev, "port"); > > + if (clk_port == ERR_PTR(-EPROBE_DEFER)) > > + return -EPROBE_DEFER; > > + > > + clk_gpio = devm_clk_get(&pdev->dev, "gpio"); > > + if (clk_gpio == ERR_PTR(-EPROBE_DEFER)) > > + return -EPROBE_DEFER; > > + > > + if (!IS_ERR_OR_NULL(clk_port)) { > > + ret = clk_prepare_enable(clk_port); > > + if (ret) > > + return ret; > > + } > > + > > + if (!IS_ERR_OR_NULL(clk_gpio)) { > > + ret = clk_prepare_enable(clk_gpio); > > + if (ret) { > > + clk_disable_unprepare(clk_port); > > + return ret; > > + } > > + } > > I think IS_ERR_OR_NULL() is considered harmful among other things. > > What about this pattern: > > clk = devm_clk_get(dev, "foo"); > if (!IS_ERR(clk)) { > ret = clk_prepare_enable(clk); > if (ret) > return ret; > } else if (PTR_ERR(clk) == -EPROBE_DEFER) { > /* > * Percolate deferrals, for anything else, > * just live without the clocking. > */ > return PTR_ERR(clk); > } > > You also need to introduce code to disable the clock on > .remove() or the clock will always be on after this module has probed. This > applies also to builtin drivers, unless you suppress the sysfs attributes and use > platform_driver_probe() > Looks good to me. Will update the patch. Thanks for the suggestion. Regards Dong Aisheng > Yours, > Linus Walleij
The i.MX 7ULP family of processors represents NXP’s latest achievement in ultra-low-power processing for use cases demanding long battery life. Targeted towards the growing market of portable devices, the i.MX 7ULP family of processors features NXP's advanced implementation of the Arm® Cortex®-A7 core, the Arm Cortex-M4 core, as well as a 3D and 2D Graphics Processing Units (GPUs). The i.MX 7ULP family provides up to 32-bit LPDDR2/LPDDR3 memory interface and a number of other interfaces for connecting peripherals, such as WLAN, Bluetooth, GPS, displays, and camera sensors. This patch series adds the basic support for imx7ulp. It includes machine level support code and device tree. Note: it depends on clk driver which is still under review. v2->v3: * back to old pinctrl binding according to SoC maintainer's suggestions * use generic node name * error checking updated according to Russell's suggestion: ptr == ERR_PTR(-EPROBE_DEFER) v1->v2: * switch to SPDX license * rebase to latest tree * pad name update * add gpio clk support * minor fix Dong Aisheng (10): dt-bindings: fsl: add compatible for imx7ulp evk dt-bindings: fsl: add imx7ulp pm related components bindings dt-bindings: gpio: vf610: add optional clocks property gpio: vf610: add optional clock support dt-bindings: pinctrl: imx7ulp: back to imx legacy binding for consistency pinctrl: fsl: imx7ulp: change to use imx legacy binding ARM: imx: add initial support for imx7ulp dts: imx: add common imx7ulp dtsi support dts: fsl: add imx7ulp evk support ARM: imx_v6_v7_defconfig: add imx7ulp support .../bindings/arm/freescale/fsl,imx7ulp-pm.txt | 23 ++ Documentation/devicetree/bindings/arm/fsl.txt | 8 + .../devicetree/bindings/gpio/gpio-vf610.txt | 6 + .../bindings/pinctrl/fsl,imx7ulp-pinctrl.txt | 66 ++--- arch/arm/boot/dts/Makefile | 2 + arch/arm/boot/dts/imx7ulp-evk.dts | 77 +++++ arch/arm/boot/dts/imx7ulp.dtsi | 314 +++++++++++++++++++++ arch/arm/configs/imx_v6_v7_defconfig | 1 + arch/arm/mach-imx/Kconfig | 9 + arch/arm/mach-imx/Makefile | 1 + arch/arm/mach-imx/common.h | 1 + arch/arm/mach-imx/cpu.c | 3 + arch/arm/mach-imx/mach-imx7ulp.c | 32 +++ arch/arm/mach-imx/mxc.h | 1 + arch/arm/mach-imx/pm-imx7ulp.c | 28 ++ drivers/gpio/gpio-vf610.c | 24 ++ drivers/pinctrl/freescale/pinctrl-imx7ulp.c | 42 --- 17 files changed, 559 insertions(+), 79 deletions(-) create mode 100644 Documentation/devicetree/bindings/arm/freescale/fsl,imx7ulp-pm.txt create mode 100644 arch/arm/boot/dts/imx7ulp-evk.dts create mode 100644 arch/arm/boot/dts/imx7ulp.dtsi create mode 100644 arch/arm/mach-imx/mach-imx7ulp.c create mode 100644 arch/arm/mach-imx/pm-imx7ulp.c