Re: [PATCH v5 2/3] arm64: dts: imx: Add i.mx8mm nitrogen8mm basic dts support

quoted

Hi Marco,

Le ven. 29 janv. 2021 à 10:22, Marco Felsch [off-list ref] a écrit :
Hi Adrien,

On 21-01-28 18:23, Adrien Grassein wrote:

+&i2c1 {
+     clock-frequency = <400000>;
                            ^
                Is the i2c errata fixed on the imx8?
I don't know. What is this errata?
Should I set a lower speed for the particular i2c?
The max. clock on iMX6 is 375kHz due to errate ERR007805
https://www.nxp.com/docs/en/errata/IMX6DQCE.pdf
It's an imx8. So I guess that this errata will not affect my board.

+     pinctrl-names = "default", "gpio";
                                     ^
                        no pinctrl for gpio.
Yes, it's a bug, thanks

+     pinctrl-0 = <&pinctrl_i2c1>;
+     status = "okay";
+
+     pmic@8 {
+             compatible = "nxp,pf8121a";
+             reg = <0x8>;
+
+             regulators {
+                 reg_ldo1: ldo1 {
                        ^
                   alignment
OK

+                             regulator-min-microvolt = <1500000>;
+                             regulator-max-microvolt = <5000000>;
+                             regulator-boot-on;
+                             regulator-always-on;
+                     };
+
+                     reg_ldo2: ldo2 {
+                             regulator-min-microvolt = <1500000>;
+                             regulator-max-microvolt = <5000000>;
+                             regulator-boot-on;
+                             regulator-always-on;
+                     };
+
+                     reg_ldo3: ldo3 {
+                             regulator-min-microvolt = <1500000>;
+                             regulator-max-microvolt = <5000000>;
+                             regulator-boot-on;
+                             regulator-always-on;
+                     };
+
+                     reg_ldo4: ldo4 {
+                             regulator-min-microvolt = <1500000>;
+                             regulator-max-microvolt = <5000000>;
+                             regulator-boot-on;
+                             regulator-always-on;
+                     };
+
+                     reg_buck1: buck1 {
+                             regulator-min-microvolt = <400000>;
+                             regulator-max-microvolt = <1800000>;
+                             regulator-boot-on;
+                             regulator-always-on;
+                     };
+
+                     reg_buck2: buck2 {
+                             regulator-min-microvolt = <400000>;
+                             regulator-max-microvolt = <1800000>;
+                             regulator-boot-on;
+                             regulator-always-on;
+                     };
+
+                     reg_sw3: buck3 {
+                             regulator-min-microvolt = <400000>;
+                             regulator-max-microvolt = <1800000>;
+                             regulator-boot-on;
+                             regulator-always-on;
+                     };
+
+                     reg_buck4: buck4 {
+                             regulator-min-microvolt = <400000>;
+                             regulator-max-microvolt = <1800000>;
+                             regulator-boot-on;
+                             regulator-always-on;
+                     };
+
+                     reg_buck5: buck5 {
+                             regulator-min-microvolt = <400000>;
+                             regulator-max-microvolt = <1800000>;
+                             regulator-boot-on;
+                             regulator-always-on;
+                     };
+
+                     reg_buck6: buck6 {
+                             regulator-min-microvolt = <400000>;
+                             regulator-max-microvolt = <1800000>;
+                             regulator-boot-on;
+                             regulator-always-on;
+                     };
+
+                     reg_buck7: buck7 {
+                             regulator-min-microvolt = <3300000>;
+                             regulator-max-microvolt = <3300000>;
+                             regulator-boot-on;
+                             regulator-always-on;
+                     };
+
+                     reg_vsnvs: vsnvs {
+                             regulator-min-microvolt = <1800000>;
+                             regulator-max-microvolt = <3300000>;
+                             regulator-boot-on;
+                             regulator-always-on;
+                     };
Do we really need to have all regulators marked as always-on?
I used the definition present on the example.
I will remove this (for the one I don't use).
Can you test to remove all always-on except for those really needed e.g.
the vddr-ref. Regulators are obtained on demand by the devices. This
allows us to save power e.g. in suspend-to-ram case.
Ok, thanks,

+             };
+     };
+};
+
+&i2c3 {
+     clock-frequency = <100000>;
+     pinctrl-names = "default", "gpio";
+     pinctrl-0 = <&pinctrl_i2c3>;
+     status = "okay";
+
+     i2cmux@70 {
+             compatible = "nxp,pca9540";
+             reg = <0x70>;
+             #address-cells = <1>;
+             #size-cells = <0>;
+
+             i2c3 {
+                     reg = <0>;
+                     #address-cells = <1>;
+                     #size-cells = <0>;
+
+                     rtc@68 {
+                             compatible = "microcrystal,rv4162";
+                             pinctrl-names = "default";
+                             pinctrl-0 = <&pinctrl_i2c3a_rv4162>;
+                             reg = <0x68>;
reg should be the 2nd property, after the compatible.
OK.

+                             interrupts-extended = <&gpio4 22 IRQ_TYPE_LEVEL_LOW>;
+                             wakeup-source;
+                     };
+             };
+     };
+};
+
+/* console */
+&uart2 {
+     pinctrl-names = "default";
+     pinctrl-0 = <&pinctrl_uart2>;
+     assigned-clocks = <&clk IMX8MM_CLK_UART2>;
+     assigned-clock-parents = <&clk IMX8MM_CLK_24M>;
+     status = "okay";
+};
+
+/* eMMC */
+&usdhc1 {
+     bus-width = <8>;
+     sdhci-caps-mask = <0x80000000 0x0>;
                ^
This is a SD host controller property according the doc.
Yes, I don't understand the point, sorry.
This property is read by the host driver, but should be present here
(like in some other dts).
I've never seen this property. I said "This is a SD host controller"
because your comment says that this usdhc controller is used for eMMC
which is not an SD controller.
The eMMC is connected to this controller. Without this, my eMMC is not
recognized by the kernel.

+     non-removable;
+     pinctrl-names = "default", "state_100mhz", "state_200mhz";
+     pinctrl-0 = <&pinctrl_usdhc1>;
+     pinctrl-1 = <&pinctrl_usdhc1_100mhz>;
+     pinctrl-2 = <&pinctrl_usdhc1_200mhz>;
+     status = "okay";
+};
+
+/* sdcard */
+&usdhc2 {
+     bus-width = <4>;
+     cd-gpios = <&gpio2 12 GPIO_ACTIVE_LOW>;
+     pinctrl-names = "default", "state_100mhz", "state_200mhz";
+     pinctrl-0 = <&pinctrl_usdhc2>;
+     pinctrl-1 = <&pinctrl_usdhc2_100mhz>;
+     pinctrl-2 = <&pinctrl_usdhc2_200mhz>;
+     vqmmc-supply = <&reg_ldo2>;
+     status = "okay";
+};
+
+&wdog1 {
+     pinctrl-names = "default";
+     pinctrl-0 = <&pinctrl_wdog>;
+     fsl,ext-reset-output;
+     status = "okay";
+};
+
+&iomuxc {
+     pinctrl-names = "default";
+     pinctrl-0 = <&pinctrl_hog>;
It would be nice to avoid such hog's. Instead those gpios should get
configured by the device(s) using those.
Once again (sorry), I don't understand the point.
I did this like any other imx8 board (imx8mq-nitrogem for example).
Question is where are those pins used and for what purpose. It is common
to specify the muxing within the device nodes like you did for the wdog1
device. This mechanism here is something like: "Oh I don't wanna setup
the muxing on the correct places, so let's mux it here in glob". Your
hog group isn't really large but it would be nice to avoid it since day
one.
Ok, no problem with this, but how can I do this?
Do you have an example?

Regards,
  Marco
Moreover, yesterday I did another version of this patchset taking in
account mot of your review exept:
    - errata;
    - SD Host property;
    - hog group.

Thanks again,
Adrien

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help