| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 24 Jun 2019 08:52:47 +0300
From: Matti Vaittinen <matti.vaittinen@...rohmeurope.com>
To: Andra Danciu <andradanciu1997@...il.com>
Cc: robh+dt@...nel.org, mark.rutland@....com, shawnguo@...nel.org,
leoyang.li@....com, aisheng.dong@....com, sriram.dash@....com,
pramod.kumar_1@....com, bhaskar.upadhaya@....com,
vabhav.sharma@....com, pankaj.bansal@....com,
richard.hu@...hnexion.com, l.stach@...gutronix.de,
ping.bai@....com, manivannan.sadhasivam@...aro.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
daniel.baluta@....com
Subject: Re: [RFC PATCH] arm64: dts: fsl: wandboard: Add a device tree for
the PICO-PI-IMX8M
Hello Richard,
Nice to see you upstreaming this! Thumbs up!
Just few remarks to pmic node from me:
On Thu, Jun 20, 2019 at 04:32:52PM +0300, Andra Danciu wrote:
> From: Richard Hu <richard.hu@...hnexion.com>
>
> The current level of support yields a working console and is able to boot
> userspace from an initial ramdisk copied via u-boot in RAM.
>
> Additional subsystems that are active :
> - Ethernet
> - USB
>
> Cc: Daniel Baluta <daniel.baluta@....com>
> Signed-off-by: Richard Hu <richard.hu@...hnexion.com>
> Signed-off-by: Andra Danciu <andradanciu1997@...il.com>
> ---
> I am using pico-pi-8mxm board to work on my project for Google Summer of Code.
> This is based on patches from https://github.com/wandboard-org.
>
> arch/arm64/boot/dts/freescale/Makefile | 1 +
> arch/arm64/boot/dts/freescale/wand-pi-8m.dts | 590 +++++++++++++++++++++++++++
> 2 files changed, 591 insertions(+)
> create mode 100644 arch/arm64/boot/dts/freescale/wand-pi-8m.dts
>
> diff --git a/arch/arm64/boot/dts/freescale/Makefile b/arch/arm64/boot/dts/freescale/Makefile
> index 984554343c83..5904d6a8a033 100644
> --- a/arch/arm64/boot/dts/freescale/Makefile
> +++ b/arch/arm64/boot/dts/freescale/Makefile
> @@ -23,3 +23,4 @@ dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-lx2160a-rdb.dtb
> dtb-$(CONFIG_ARCH_MXC) += imx8mm-evk.dtb
> dtb-$(CONFIG_ARCH_MXC) += imx8mq-evk.dtb
> dtb-$(CONFIG_ARCH_MXC) += imx8qxp-mek.dtb
> +dtb-$(CONFIG_ARCH_MXC) += wand-pi-8m.dtb
> diff --git a/arch/arm64/boot/dts/freescale/wand-pi-8m.dts b/arch/arm64/boot/dts/freescale/wand-pi-8m.dts
> new file mode 100644
> index 000000000000..9f7121014722
> --- /dev/null
> +++ b/arch/arm64/boot/dts/freescale/wand-pi-8m.dts
> @@ -0,0 +1,590 @@
// snip
> +
> +&i2c1 {
> + clock-frequency = <100000>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_i2c1>;
> + status = "okay";
> +
> + typec_tusb320:tusb320@47 {
> + compatible = "ti,tusb320";
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_tusb320_irq &pinctrl_typec_ss_sel>;
> + reg = <0x47>;
> + vbus-supply = <®_usb_otg_vbus>;
> + ss-sel-gpios = <&gpio3 5 GPIO_ACTIVE_HIGH>;
> + tusb320,int-gpio = <&gpio3 6 GPIO_ACTIVE_LOW>;
> + tusb320,select-mode = <0>;
> + tusb320,dfp-power = <0>;
> + };
> +
> + pmic: bd71837@4b {
I was once told the node names should be generic :] So, I'd suggest
using "pmic@4b".
> + reg = <0x4b>;
> + compatible = "rohm,bd71837";
> + /* PMIC BD71837 PMIC_nINT GPIO1_IO12 */
> + pinctrl-0 = <&pinctrl_pmic>;
> + gpio_intr = <&gpio1 3 GPIO_ACTIVE_LOW>;
> +
> + bd71837,pmic-buck1-uses-i2c-dvs;
> + bd71837,pmic-buck1-dvs-voltage = <900000>, <850000>, <800000>; /* VDD_SOC: Run-Idle-Suspend */
> + bd71837,pmic-buck2-uses-i2c-dvs;
> + bd71837,pmic-buck2-dvs-voltage = <1000000>, <900000>, <0>; /* VDD_ARM: Run-Idle */
> + bd71837,pmic-buck3-uses-i2c-dvs;
> + bd71837,pmic-buck3-dvs-voltage = <1000000>, <0>, <0>; /* VDD_GPU: Run */
> + bd71837,pmic-buck4-uses-i2c-dvs;
> + bd71837,pmic-buck4-dvs-voltage = <1000000>, <0>, <0>; /* VDD_VPU: Run */
These entries should be replaced by proper properties for run-level voltage
configuration. Please see the
Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt and
Documentation/devicetree/bindings/regulator/rohm,bd71837-regulator.txt.
I think you wish to use rohm,dvs-run-voltage, rohm,dvs-idle-voltage,
and rohm,dvs-suspend-voltage instead.
Furthermore, I see you are not specifying rohm,reset-snvs-powered.
I wonder if it is intentional to not use SNVS as reset target. Seeing you
use i.MX8 and seeing used those unsupported run-level configuration properties
which were present only in some very first proprietary driver draft - I
expect this may not be intentional. I think that early driver defaulted
to SNVS while it also failed to provide any regulator enable/disable
control.
> +
> + gpo {
> + rohm,drv = <0x0C>; /* 0b0000_1100 all gpos with cmos output mode */
> + };
What is this?
> +
> + regulators {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + buck1_reg: regulator@0 {
I don't think the node names are correct. As far as I know the regulator
core uses node names - please see the valid names from documentation.
> + reg = <0>;
> + regulator-compatible = "buck1";
I think you shouldn't use regulator-compatible. On the other hand, I
think you should use regulator-name.
> + regulator-min-microvolt = <700000>;
> + regulator-max-microvolt = <1300000>;
> + regulator-boot-on;
> + regulator-always-on;
> + regulator-ramp-delay = <1250>;
> + };
> +
> + buck2_reg: regulator@1 {
> + reg = <1>;
> + regulator-compatible = "buck2";
> + regulator-min-microvolt = <700000>;
> + regulator-max-microvolt = <1300000>;
> + regulator-boot-on;
> + regulator-always-on;
> + regulator-ramp-delay = <1250>;
> + };
> +
> + buck3_reg: regulator@2 {
> + reg = <2>;
> + regulator-compatible = "buck3";
> + regulator-min-microvolt = <700000>;
> + regulator-max-microvolt = <1300000>;
> + regulator-boot-on;
> + regulator-always-on;
In typical BD71837 use-cases the buck 3 is used to power graphichs accelerator.
I wonder if enable/disable control should be allowed to help thermal
issues and power saving? (This comment can be ignored if not applicaple
to your board)
> + };
> +
> + buck4_reg: regulator@3 {
> + reg = <3>;
> + regulator-compatible = "buck4";
> + regulator-min-microvolt = <700000>;
> + regulator-max-microvolt = <1300000>;
> + regulator-boot-on;
> + regulator-always-on;
In typical BD71837 use-cases the buck 4 is used to power VPU.
I wonder if enable/disable control should be allowed to help thermal
issues and power saving? (This comment can be ignored if not applicaple
to your board)
> + };
> +
> + buck5_reg: regulator@4 {
> + reg = <4>;
> + regulator-compatible = "buck5";
> + regulator-min-microvolt = <700000>;
> + regulator-max-microvolt = <1350000>;
> + regulator-boot-on;
> + regulator-always-on;
> + };
> +
> + buck6_reg: regulator@5 {
> + reg = <5>;
> + regulator-compatible = "buck6";
> + regulator-min-microvolt = <3000000>;
> + regulator-max-microvolt = <3300000>;
> + regulator-boot-on;
> + regulator-always-on;
> + };
> +
> + buck7_reg: regulator@6 {
> + reg = <6>;
> + regulator-compatible = "buck7";
> + regulator-min-microvolt = <1605000>;
> + regulator-max-microvolt = <1995000>;
> + regulator-boot-on;
> + regulator-always-on;
> + };
> +
> + buck8_reg: regulator@7 {
> + reg = <7>;
> + regulator-compatible = "buck8";
> + regulator-min-microvolt = <800000>;
> + regulator-max-microvolt = <1400000>;
> + regulator-boot-on;
> + regulator-always-on;
> + };
> +
> + ldo1_reg: regulator@8 {
> + reg = <8>;
> + regulator-compatible = "ldo1";
> + regulator-min-microvolt = <3000000>;
> + regulator-max-microvolt = <3300000>;
> + regulator-boot-on;
> + regulator-always-on;
> + };
> +
> + ldo2_reg: regulator@9 {
> + reg = <9>;
> + regulator-compatible = "ldo2";
> + regulator-min-microvolt = <900000>;
> + regulator-max-microvolt = <900000>;
> + regulator-boot-on;
> + regulator-always-on;
> + };
> +
> + ldo3_reg: regulator@10 {
> + reg = <10>;
> + regulator-compatible = "ldo3";
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <3300000>;
> + regulator-boot-on;
> + regulator-always-on;
> + };
> +
> + ldo4_reg: regulator@11 {
> + reg = <11>;
> + regulator-compatible = "ldo4";
> + regulator-min-microvolt = <900000>;
> + regulator-max-microvolt = <1800000>;
> + regulator-boot-on;
> + regulator-always-on;
> + };
> +
> + ldo5_reg: regulator@12 {
> + reg = <12>;
> + regulator-compatible = "ldo5";
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <3300000>;
> + regulator-boot-on;
> + regulator-always-on;
You may want to mark the BUCK6 as a supply for LDO5.
> + };
> +
> + ldo6_reg: regulator@13 {
> + reg = <13>;
> + regulator-compatible = "ldo6";
> + regulator-min-microvolt = <900000>;
> + regulator-max-microvolt = <1800000>;
> + regulator-boot-on;
> + regulator-always-on;
You may want to mark the BUCK7 as a supply for LDO6.
> + };
> +
> + ldo7_reg: regulator@14 {
> + reg = <14>;
> + regulator-compatible = "ldo7";
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <3300000>;
> + regulator-boot-on;
> + regulator-always-on;
> + };
> + };
> + };
> +};
> +
Best Regards
Matti Vaittinen
--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND
~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =]
Powered by blists - more mailing lists