[PATCH v5 2/3] arm64: dts: imx: Add i.mx8mm nitrogen8mm basic dts support
Adrien Grassein
adrien.grassein at gmail.com
Fri Jan 29 04:49:27 EST 2021
Hi Marco,
Le ven. 29 janv. 2021 à 10:22, Marco Felsch <m.felsch at pengutronix.de> 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 at 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 at 70 {
> > > > + compatible = "nxp,pca9540";
> > > > + reg = <0x70>;
> > > > + #address-cells = <1>;
> > > > + #size-cells = <0>;
> > > > +
> > > > + i2c3 {
> > > > + reg = <0>;
> > > > + #address-cells = <1>;
> > > > + #size-cells = <0>;
> > > > +
> > > > + rtc at 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 = <®_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
More information about the linux-arm-kernel
mailing list