mbox series

[v3,00/17] Improve the MT8365 SoC and EVK board support

Message ID 20230203-evk-board-support-v3-0-0003e80e0095@baylibre.com
Headers show
Series Improve the MT8365 SoC and EVK board support | expand

Message

Alexandre Mergnat March 29, 2023, 8:54 a.m. UTC
This commits are based on the Fabien Parent <fparent@baylibre.com> work.

The purpose of this series is to add the following HWs / IPs support for
the mt8365-evk board:
- Watchdog
- Power Management Integrated Circuit "PMIC" wrapper
  - MT6357 PMIC
- MultiMediaCard "MMC" & Secure Digital "SD" controller
- USB controller
- Ethernet MAC controller

Add CPU Freq & IDLE support for this board.

This series depends to another one which add support for MT8365 SoC and
EVK board [1].

Regards,
Alex

[1]: https://lore.kernel.org/all/20230125143503.1015424-1-bero@baylibre.com/

Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
---
Changes in v3:
- Add trailers and simply resend.
- Link to v2: https://lore.kernel.org/r/20230203-evk-board-support-v2-0-6ec7cdb10ccf@baylibre.com

---
Alexandre Bailon (1):
      arm64: dts: mediatek: Increase the size BL31 reserved memory

Alexandre Mergnat (12):
      dt-bindings: watchdog: mediatek,mtk-wdt: add mt8365
      dt-bindings: pinctrl: mediatek,mt8365-pinctrl: add drive strength property
      arm64: dts: mediatek: add watchdog support for mt8365 SoC
      arm64: dts: mediatek: add pwrap support to mt8365 SoC
      arm64: dts: mediatek: add mt6357 PMIC support for  mt8365-evk
      arm64: dts: mediatek: add mmc support for mt8365 SoC
      arm64: dts: mediatek: add mmc support for mt8365-evk
      arm64: dts: mediatek: add usb controller support for mt8365-evk
      arm64: dts: mediatek: add ethernet support for mt8365 SoC
      arm64: dts: mediatek: add ethernet support for mt8365-evk
      arm64: dts: mediatek: add OPP support for mt8365 SoC
      arm64: dts: mediatek: add cpufreq support for mt8365-evk

Amjad Ouled-Ameur (2):
      arm64: dts: mediatek: fix systimer properties
      arm64: dts: mediatek: Add CPU Idle support

Fabien Parent (2):
      arm64: dts: mediatek: add mt6357 device-tree
      arm64: dts: mediatek: set vmc regulator as always on

 .../bindings/pinctrl/mediatek,mt8365-pinctrl.yaml  |   3 +
 .../bindings/watchdog/mediatek,mtk-wdt.yaml        |   1 +
 arch/arm64/boot/dts/mediatek/mt6357.dtsi           | 282 +++++++++++++++++++++
 arch/arm64/boot/dts/mediatek/mt8365-evk.dts        | 254 ++++++++++++++++++-
 arch/arm64/boot/dts/mediatek/mt8365.dtsi           | 196 +++++++++++++-
 5 files changed, 731 insertions(+), 5 deletions(-)
---
base-commit: 555b3a55823ec063129de4403899203febb58788
change-id: 20230203-evk-board-support-d5b7a839ed7b

Best regards,

Comments

AngeloGioacchino Del Regno March 29, 2023, 1:17 p.m. UTC | #1
Il 29/03/23 10:54, Alexandre Mergnat ha scritto:
> Add watchdog support.
> 
> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
> ---
>   arch/arm64/boot/dts/mediatek/mt8365.dtsi | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/mediatek/mt8365.dtsi b/arch/arm64/boot/dts/mediatek/mt8365.dtsi
> index 553c7516406a..e018df6844f6 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8365.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt8365.dtsi
> @@ -162,6 +162,13 @@ syscfg_pctl: syscfg-pctl@10005000 {
>   			reg = <0 0x10005000 0 0x1000>;
>   		};
>   
> +		watchdog: watchdog@10007000 {
> +			compatible = "mediatek,mt8365-wdt",
> +				     "mediatek,mt6589-wdt";

This fits in one line, 83 columns is *definitely* fine.
Can you please compress that?

After which:

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
AngeloGioacchino Del Regno March 29, 2023, 1:17 p.m. UTC | #2
Il 29/03/23 10:54, amergnat@baylibre.com ha scritto:
> From: Alexandre Bailon <abailon@baylibre.com>
> 
> The reserved size for BL31 is too small.
> This has been highlighted by the MPU that now restrict access to BL31
> memory to secure world only.
> This increase the size of the reserved memory.
> 
> Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
AngeloGioacchino Del Regno March 29, 2023, 1:17 p.m. UTC | #3
Il 29/03/23 10:54, amergnat@baylibre.com ha scritto:
> From: Fabien Parent <fparent@baylibre.com>
> 
> This new device-tree add the regulators, rtc and keys support
> for the MT6357 PMIC.
> 
> Signed-off-by: Fabien Parent <fparent@baylibre.com>
> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
> ---
>   arch/arm64/boot/dts/mediatek/mt6357.dtsi | 282 +++++++++++++++++++++++++++++++
>   1 file changed, 282 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/mediatek/mt6357.dtsi b/arch/arm64/boot/dts/mediatek/mt6357.dtsi
> new file mode 100644
> index 000000000000..3330a03c2f74
> --- /dev/null
> +++ b/arch/arm64/boot/dts/mediatek/mt6357.dtsi
> @@ -0,0 +1,282 @@

..snip..

> +
> +		keys {
> +			compatible = "mediatek,mt6357-keys";
> +
> +			key-power {
> +				linux,keycodes = <KEY_POWER>;
> +				wakeup-source;
> +			};
> +
> +			key-home {
> +				linux,keycodes = <KEY_HOME>;
> +				wakeup-source;

KEY_HOME is a wakeup-source?! are you sure?!

(P.S.: I'm just checking if that was intended)

Regards,
Angelo
AngeloGioacchino Del Regno March 29, 2023, 1:19 p.m. UTC | #4
Il 29/03/23 10:54, Alexandre Mergnat ha scritto:
> In order to use the PMIC, the pwrap support should be added
> to allow communication between the SoC and the PMIC.
> 
> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
> ---
>   arch/arm64/boot/dts/mediatek/mt8365.dtsi | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/mediatek/mt8365.dtsi b/arch/arm64/boot/dts/mediatek/mt8365.dtsi
> index e018df6844f6..687011353f69 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8365.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt8365.dtsi
> @@ -186,6 +186,18 @@ apmixedsys: syscon@1000c000 {
>   			#clock-cells = <1>;
>   		};
>   
> +		pwrap: pwrap@1000d000 {
> +			compatible = "mediatek,mt8365-pwrap";
> +			reg = <0 0x1000d000 0 0x1000>;
> +			reg-names = "pwrap";
> +			interrupts = <GIC_SPI 123 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&infracfg CLK_IFR_PWRAP_SPI>,
> +				 <&infracfg CLK_IFR_PMIC_AP>,
> +				 <&infracfg CLK_IFR_PWRAP_SYS>,
> +				 <&infracfg CLK_IFR_PWRAP_TMR>;

I would prefer:

clocks = <&infracfg CLK_IFR_PWRAP_SPI>, <&infracfg CLK_IFR_PMIC_AP>,
	 <&infracfg CLK_IFR_PWRAP_SYS>, <&infracfg CLK_IFR_PWRAP_TMR>;

....but I'll leave this choice to you, as I don't have really strong opinions on
this one, so, with or without the proposed change, you still get my:

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
AngeloGioacchino Del Regno March 29, 2023, 1:20 p.m. UTC | #5
Il 29/03/23 10:54, Alexandre Mergnat ha scritto:
> This power management system chip integration helps to manage regulators
> and keys.
> 
> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
> ---
>   arch/arm64/boot/dts/mediatek/mt8365-evk.dts | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/mediatek/mt8365-evk.dts b/arch/arm64/boot/dts/mediatek/mt8365-evk.dts
> index fc7f6d8ae173..2f88562c638a 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8365-evk.dts
> +++ b/arch/arm64/boot/dts/mediatek/mt8365-evk.dts
> @@ -12,6 +12,7 @@
>   #include <dt-bindings/input/input.h>
>   #include <dt-bindings/pinctrl/mt8365-pinfunc.h>
>   #include "mt8365.dtsi"
> +#include "mt6357.dtsi"
>   
>   / {
>   	model = "MediaTek MT8365 Open Platform EVK";
> @@ -96,6 +97,13 @@ &i2c0 {
>   	#size-cells = <0>;
>   };
>   
> +&mt6357_pmic {
> +	interrupt-parent = <&pio>;
> +	interrupts = <145 IRQ_TYPE_LEVEL_HIGH>;

Please... use:
	interrupts-extended = <&pio 145 IRQ_TYPE_LEVEL_HIGH>;

Cheers,
Angelo
AngeloGioacchino Del Regno March 29, 2023, 1:21 p.m. UTC | #6
Il 29/03/23 10:54, Alexandre Mergnat ha scritto:
> There are three ports of MSDC (MMC and SD Controller), which are:
> - MSDC0: EMMC5.1
> - MSDC1: SD3.0/SDIO3.0
> - MSDC2: SDIO3.0+
> 
> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
> ---
>   arch/arm64/boot/dts/mediatek/mt8365.dtsi | 39 ++++++++++++++++++++++++++++++++
>   1 file changed, 39 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/mediatek/mt8365.dtsi b/arch/arm64/boot/dts/mediatek/mt8365.dtsi
> index 687011353f69..a67eeca28da5 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8365.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt8365.dtsi
> @@ -399,6 +399,45 @@ usb_host: usb@11200000 {
>   			};
>   		};
>   
> +		mmc0: mmc@11230000 {
> +			compatible = "mediatek,mt8365-mmc", "mediatek,mt8183-mmc";
> +			reg = <0 0x11230000 0 0x1000>,
> +			      <0 0x11cd0000 0 0x1000>;
> +			interrupts = <GIC_SPI 23 IRQ_TYPE_LEVEL_LOW>;
> +			clocks = <&topckgen CLK_TOP_MSDC50_0_SEL>,
> +				 <&infracfg CLK_IFR_MSDC0_HCLK>,
> +				 <&infracfg CLK_IFR_MSDC0_SRC>;
> +			clock-names = "source", "hclk", "source_cg";
> +			status = "disabled";
> +		};
> +
> +		mmc1: mmc@11240000 {
> +			compatible = "mediatek,mt8365-mmc", "mediatek,mt8183-mmc";
> +			reg = <0 0x11240000 0 0x1000>,
> +			      <0 0x11c90000 0 0x1000>;
> +			interrupts = <GIC_SPI 24 IRQ_TYPE_LEVEL_LOW>;
> +			clocks = <&topckgen CLK_TOP_MSDC30_1_SEL>,
> +				 <&infracfg CLK_IFR_MSDC1_HCLK>,
> +				 <&infracfg CLK_IFR_MSDC1_SRC>;
> +			clock-names = "source", "hclk", "source_cg";
> +			status = "disabled";
> +		};
> +
> +		mmc2: mmc@11250000 {
> +			compatible = "mediatek,mt8365-mmc", "mediatek,mt8183-mmc";
> +			reg = <0 0x11250000 0 0x1000>,
> +			      <0 0x11c60000 0 0x1000>;
> +			interrupts = <GIC_SPI 68 IRQ_TYPE_LEVEL_LOW>;
> +			clocks = <&topckgen CLK_TOP_MSDC50_2_SEL>,
> +				 <&infracfg CLK_IFR_MSDC2_HCLK>,
> +				 <&infracfg CLK_IFR_MSDC2_SRC>,
> +				 <&infracfg CLK_IFR_MSDC2_BK>,
> +				 <&infracfg CLK_IFR_AP_MSDC0>;
> +			clock-names = "source", "hclk", "source_cg",
> +				      "bus_clk", "sys_cg";

clock-names for this do fit in one 90 columns line.

After compressing it,

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
AngeloGioacchino Del Regno March 29, 2023, 1:24 p.m. UTC | #7
Il 29/03/23 10:54, Alexandre Mergnat ha scritto:
> - Add EMMC support on mmc0 (internal memory)
> - Add SD-UHS support on mmc1 (external memory)
> 
> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
> ---
>   arch/arm64/boot/dts/mediatek/mt8365-evk.dts | 138 ++++++++++++++++++++++++++++
>   1 file changed, 138 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/mediatek/mt8365-evk.dts b/arch/arm64/boot/dts/mediatek/mt8365-evk.dts
> index 2f88562c638a..b5d018686cbe 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8365-evk.dts
> +++ b/arch/arm64/boot/dts/mediatek/mt8365-evk.dts
> @@ -97,6 +97,42 @@ &i2c0 {
>   	#size-cells = <0>;
>   };
>   
> +&mmc0 {
> +	pinctrl-names = "default", "state_uhs";
> +	pinctrl-0 = <&mmc0_default_pins>;
> +	pinctrl-1 = <&mmc0_uhs_pins>;
> +	bus-width = <8>;
> +	max-frequency = <200000000>;
> +	cap-mmc-highspeed;
> +	mmc-hs200-1_8v;
> +	mmc-hs400-1_8v;
> +	cap-mmc-hw-reset;
> +	no-sdio;
> +	no-sd;
> +	hs400-ds-delay = <0x12012>;
> +	vmmc-supply = <&mt6357_vemc_reg>;
> +	vqmmc-supply = <&mt6357_vio18_reg>;
> +	assigned-clocks = <&topckgen CLK_TOP_MSDC50_0_SEL>;
> +	assigned-clock-parents = <&topckgen CLK_TOP_MSDCPLL>;
> +	non-removable;

That's a bit messy :-)
Can we please reorder this by name?

	assigned-clocks....
	bus-width
	cap....
	hs400-ds-delay
	max...
	mmc-hs...
	no...
	non-rem...
	pinctrl...
	vxxxx-supply

	status ....

...Actually the same comment also applies to mmc1.

Cheers,
Angelo
AngeloGioacchino Del Regno March 29, 2023, 1:25 p.m. UTC | #8
Il 29/03/23 10:54, amergnat@baylibre.com ha scritto:
> From: Fabien Parent <fparent@baylibre.com>
> 
> MSDC1 IP block is powered by VMC. Make sure it is always on.

Why always on?
Can't you just set mt6357_vmc_reg as VIN of mt6357_vmch_reg? :-)

Regards,
Angelo

> 
> Signed-off-by: Fabien Parent <fparent@baylibre.com>
> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
> ---
>   arch/arm64/boot/dts/mediatek/mt8365-evk.dts | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/mediatek/mt8365-evk.dts b/arch/arm64/boot/dts/mediatek/mt8365-evk.dts
> index b5d018686cbe..22ec332fe9c9 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8365-evk.dts
> +++ b/arch/arm64/boot/dts/mediatek/mt8365-evk.dts
> @@ -140,6 +140,11 @@ &mt6357_pmic {
>   	#interrupt-cells = <2>;
>   };
>   
> +/* Needed by MSDC1 */
> +&mt6357_vmc_reg {
> +	regulator-always-on;
> +};
> +
>   &pio {
>   	gpio_keys: gpio-keys-pins {
>   		pins {
>
AngeloGioacchino Del Regno March 29, 2023, 1:26 p.m. UTC | #9
Il 29/03/23 10:54, Alexandre Mergnat ha scritto:
> This patch add support for SuperSpeed USB, in OTG mode, on micro connector.
> It also add support for the Extensible Host Controller Interface USB.
> 
> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
> ---
>   arch/arm64/boot/dts/mediatek/mt8365-evk.dts | 22 ++++++++++++++++++++++
>   1 file changed, 22 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/mediatek/mt8365-evk.dts b/arch/arm64/boot/dts/mediatek/mt8365-evk.dts
> index 22ec332fe9c9..868ee0d160e4 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8365-evk.dts
> +++ b/arch/arm64/boot/dts/mediatek/mt8365-evk.dts
> @@ -319,6 +319,28 @@ &pwm {
>   	status = "okay";
>   };
>   
> +&ssusb {
> +	pinctrl-0 = <&usb_pins>;
> +	pinctrl-names = "default";
> +	maximum-speed = "high-speed";
> +	usb-role-switch;
> +	dr_mode = "otg";
> +	vusb33-supply = <&mt6357_vusb33_reg>;
> +	status = "okay";

Order by name please.

P.S.: status can go at the bottom, even if v < s :-)

Thanks,
Angelo
AngeloGioacchino Del Regno March 29, 2023, 1:27 p.m. UTC | #10
Il 29/03/23 10:54, Alexandre Mergnat ha scritto:
> This IP is a 10/100 MAC controller compliant with IEEE 802.3 standards.
> It supports power management with Energy Efficient Ethernet and Wake-on-LAN
> specification. Flow control is provided for half-duplex and full-duplex
> mode. For packet transmission and reception, the controller supports
> IPv4/UDP/TCP checksum offload and VLAN tag insertion.
> 
> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
AngeloGioacchino Del Regno March 29, 2023, 1:28 p.m. UTC | #11
Il 29/03/23 10:54, Alexandre Mergnat ha scritto:
> In order to have cpufreq support, this patch adds generic Operating
> Performance Points support.
> 
> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
> ---
>   arch/arm64/boot/dts/mediatek/mt8365.dtsi | 85 ++++++++++++++++++++++++++++++++
>   1 file changed, 85 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/mediatek/mt8365.dtsi b/arch/arm64/boot/dts/mediatek/mt8365.dtsi
> index 394a5a61be59..c3ea3cc97a47 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8365.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt8365.dtsi
> @@ -20,6 +20,75 @@ cpus {
>   		#address-cells = <1>;
>   		#size-cells = <0>;
>   
> +	cluster0_opp: opp-table-0 {
> +		compatible = "operating-points-v2";
> +		opp-shared;

One blank line here please, makes it slightly more readable.

> +		opp-850000000 {
> +			opp-hz = /bits/ 64 <850000000>;
> +			opp-microvolt = <650000>;
> +		};

Also, my personal preference is to also leave a blank line between
each opp-xxxx subnode, but that's your choice.

Everything else looks good.
AngeloGioacchino Del Regno March 29, 2023, 1:28 p.m. UTC | #12
Il 29/03/23 10:54, Alexandre Mergnat ha scritto:
> In order to have cpufreq support, this patch adds proc-supply and sram-supply
> for each CPU.
> 
> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
AngeloGioacchino Del Regno March 29, 2023, 1:29 p.m. UTC | #13
Il 29/03/23 10:54, amergnat@baylibre.com ha scritto:
> From: Amjad Ouled-Ameur <aouledameur@baylibre.com>
> 
> MT8365 has a SYST timer (System Timer), therefore the compatible node
> should be "mediatek,mt6765-timer" instead of "mediatek,mt6795-systimer"
> (which corresponds to ARM/ARM64 System Timer).
> 
> Plus, register range should be 0x100 instead of 0x10.
> 
> Finally, interrupt polarity of systimer is LEVEL_HIGH.
> 
> Fix the above properties accordingly.
> 
> Signed-off-by: Amjad Ouled-Ameur <aouledameur@baylibre.com>
> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
AngeloGioacchino Del Regno March 29, 2023, 1:31 p.m. UTC | #14
Il 29/03/23 10:54, amergnat@baylibre.com ha scritto:
> From: Amjad Ouled-Ameur <aouledameur@baylibre.com>
> 
> MT8365 has 3 CPU Idle states:
> - MCDI_CPU. (Multi-Core-Deep-Idle)
> - MCDI_CLUSTER.
> - DPIDLE. (Deep-Idle)
> 
> Signed-off-by: Amjad Ouled-Ameur <aouledameur@baylibre.com>
> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Kevin Hilman March 30, 2023, 12:09 a.m. UTC | #15
Alexandre Mergnat <amergnat@baylibre.com> writes:

> This commits are based on the Fabien Parent <fparent@baylibre.com> work.
>
> The purpose of this series is to add the following HWs / IPs support for
> the mt8365-evk board:
> - Watchdog
> - Power Management Integrated Circuit "PMIC" wrapper
>   - MT6357 PMIC
> - MultiMediaCard "MMC" & Secure Digital "SD" controller
> - USB controller
> - Ethernet MAC controller
>
> Add CPU Freq & IDLE support for this board.
>
> This series depends to another one which add support for MT8365 SoC and
> EVK board [1].

It seems to depend on more than that series.  In order to test this, I
tried applying this series on top of Bero's minimal support (now in
linux-next), and it does not apply cleanly.

Could you please list all the dependencies that are not yet upstream.

Thanks,

Kevin
Alexandre Mergnat March 30, 2023, 9:54 a.m. UTC | #16
Le jeu. 30 mars 2023 à 02:09, Kevin Hilman <khilman@baylibre.com> a écrit :
>
> Alexandre Mergnat <amergnat@baylibre.com> writes:
>
> > This commits are based on the Fabien Parent <fparent@baylibre.com> work.
> >
> > The purpose of this series is to add the following HWs / IPs support for
> > the mt8365-evk board:
> > - Watchdog
> > - Power Management Integrated Circuit "PMIC" wrapper
> >   - MT6357 PMIC
> > - MultiMediaCard "MMC" & Secure Digital "SD" controller
> > - USB controller
> > - Ethernet MAC controller
> >
> > Add CPU Freq & IDLE support for this board.
> >
> > This series depends to another one which add support for MT8365 SoC and
> > EVK board [1].
>
> It seems to depend on more than that series.  In order to test this, I
> tried applying this series on top of Bero's minimal support (now in
> linux-next), and it does not apply cleanly.
>
> Could you please list all the dependencies that are not yet upstream.

Hi Kevin,
You're right, it also depend to
https://lore.kernel.org/all/20221122-mt8365-i2c-support-v6-0-e1009c8afd53@baylibre.com/

Regards,
Alex
Matthias Brugger March 30, 2023, 5:20 p.m. UTC | #17
On 29/03/2023 10:54, amergnat@baylibre.com wrote:
> From: Alexandre Bailon <abailon@baylibre.com>
> 
> The reserved size for BL31 is too small.
> This has been highlighted by the MPU that now restrict access to BL31
> memory to secure world only.
> This increase the size of the reserved memory.
> 
> Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>

Applied, thanks!

> ---
>   arch/arm64/boot/dts/mediatek/mt8365-evk.dts | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/mediatek/mt8365-evk.dts b/arch/arm64/boot/dts/mediatek/mt8365-evk.dts
> index b68aee8f229f..fc7f6d8ae173 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8365-evk.dts
> +++ b/arch/arm64/boot/dts/mediatek/mt8365-evk.dts
> @@ -65,10 +65,10 @@ reserved-memory {
>   		#size-cells = <2>;
>   		ranges;
>   
> -		/* 128 KiB reserved for ARM Trusted Firmware (BL31) */
> +		/* 192 KiB reserved for ARM Trusted Firmware (BL31) */
>   		bl31_secmon_reserved: secmon@43000000 {
>   			no-map;
> -			reg = <0 0x43000000 0 0x20000>;
> +			reg = <0 0x43000000 0 0x30000>;
>   		};
>   
>   		/* 12 MiB reserved for OP-TEE (BL32)
>
Matthias Brugger March 30, 2023, 5:22 p.m. UTC | #18
On 29/03/2023 15:17, AngeloGioacchino Del Regno wrote:
> Il 29/03/23 10:54, amergnat@baylibre.com ha scritto:
>> From: Fabien Parent <fparent@baylibre.com>
>>
>> This new device-tree add the regulators, rtc and keys support
>> for the MT6357 PMIC.
>>
>> Signed-off-by: Fabien Parent <fparent@baylibre.com>
>> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
>> ---
>>   arch/arm64/boot/dts/mediatek/mt6357.dtsi | 282 +++++++++++++++++++++++++++++++
>>   1 file changed, 282 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/mediatek/mt6357.dtsi 
>> b/arch/arm64/boot/dts/mediatek/mt6357.dtsi
>> new file mode 100644
>> index 000000000000..3330a03c2f74
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/mediatek/mt6357.dtsi
>> @@ -0,0 +1,282 @@
> 
> ..snip..
> 
>> +
>> +        keys {
>> +            compatible = "mediatek,mt6357-keys";
>> +
>> +            key-power {
>> +                linux,keycodes = <KEY_POWER>;
>> +                wakeup-source;
>> +            };
>> +
>> +            key-home {
>> +                linux,keycodes = <KEY_HOME>;
>> +                wakeup-source;
> 
> KEY_HOME is a wakeup-source?! are you sure?!

I think it could make sense to do that, so I just queued the patch. If that was 
an oversight, then please send a follow-up patch.

Regards,
Matthias

> 
> (P.S.: I'm just checking if that was intended)
> 
> Regards,
> Angelo
Matthias Brugger March 30, 2023, 5:22 p.m. UTC | #19
On 29/03/2023 10:54, Alexandre Mergnat wrote:
> In order to use the PMIC, the pwrap support should be added
> to allow communication between the SoC and the PMIC.
> 
> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>

Applied, thanks.

> ---
>   arch/arm64/boot/dts/mediatek/mt8365.dtsi | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/mediatek/mt8365.dtsi b/arch/arm64/boot/dts/mediatek/mt8365.dtsi
> index e018df6844f6..687011353f69 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8365.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt8365.dtsi
> @@ -186,6 +186,18 @@ apmixedsys: syscon@1000c000 {
>   			#clock-cells = <1>;
>   		};
>   
> +		pwrap: pwrap@1000d000 {
> +			compatible = "mediatek,mt8365-pwrap";
> +			reg = <0 0x1000d000 0 0x1000>;
> +			reg-names = "pwrap";
> +			interrupts = <GIC_SPI 123 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&infracfg CLK_IFR_PWRAP_SPI>,
> +				 <&infracfg CLK_IFR_PMIC_AP>,
> +				 <&infracfg CLK_IFR_PWRAP_SYS>,
> +				 <&infracfg CLK_IFR_PWRAP_TMR>;
> +			clock-names = "spi", "wrap", "sys", "tmr";
> +		};
> +
>   		keypad: keypad@10010000 {
>   			compatible = "mediatek,mt6779-keypad";
>   			reg = <0 0x10010000 0 0x1000>;
>
Matthias Brugger March 30, 2023, 5:28 p.m. UTC | #20
On 29/03/2023 15:21, AngeloGioacchino Del Regno wrote:
> Il 29/03/23 10:54, Alexandre Mergnat ha scritto:
>> There are three ports of MSDC (MMC and SD Controller), which are:
>> - MSDC0: EMMC5.1
>> - MSDC1: SD3.0/SDIO3.0
>> - MSDC2: SDIO3.0+
>>
>> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
>> ---
>>   arch/arm64/boot/dts/mediatek/mt8365.dtsi | 39 ++++++++++++++++++++++++++++++++
>>   1 file changed, 39 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/mediatek/mt8365.dtsi 
>> b/arch/arm64/boot/dts/mediatek/mt8365.dtsi
>> index 687011353f69..a67eeca28da5 100644
>> --- a/arch/arm64/boot/dts/mediatek/mt8365.dtsi
>> +++ b/arch/arm64/boot/dts/mediatek/mt8365.dtsi
>> @@ -399,6 +399,45 @@ usb_host: usb@11200000 {
>>               };
>>           };
>> +        mmc0: mmc@11230000 {
>> +            compatible = "mediatek,mt8365-mmc", "mediatek,mt8183-mmc";
>> +            reg = <0 0x11230000 0 0x1000>,
>> +                  <0 0x11cd0000 0 0x1000>;
>> +            interrupts = <GIC_SPI 23 IRQ_TYPE_LEVEL_LOW>;
>> +            clocks = <&topckgen CLK_TOP_MSDC50_0_SEL>,
>> +                 <&infracfg CLK_IFR_MSDC0_HCLK>,
>> +                 <&infracfg CLK_IFR_MSDC0_SRC>;
>> +            clock-names = "source", "hclk", "source_cg";
>> +            status = "disabled";
>> +        };
>> +
>> +        mmc1: mmc@11240000 {
>> +            compatible = "mediatek,mt8365-mmc", "mediatek,mt8183-mmc";
>> +            reg = <0 0x11240000 0 0x1000>,
>> +                  <0 0x11c90000 0 0x1000>;
>> +            interrupts = <GIC_SPI 24 IRQ_TYPE_LEVEL_LOW>;
>> +            clocks = <&topckgen CLK_TOP_MSDC30_1_SEL>,
>> +                 <&infracfg CLK_IFR_MSDC1_HCLK>,
>> +                 <&infracfg CLK_IFR_MSDC1_SRC>;
>> +            clock-names = "source", "hclk", "source_cg";
>> +            status = "disabled";
>> +        };
>> +
>> +        mmc2: mmc@11250000 {
>> +            compatible = "mediatek,mt8365-mmc", "mediatek,mt8183-mmc";
>> +            reg = <0 0x11250000 0 0x1000>,
>> +                  <0 0x11c60000 0 0x1000>;
>> +            interrupts = <GIC_SPI 68 IRQ_TYPE_LEVEL_LOW>;
>> +            clocks = <&topckgen CLK_TOP_MSDC50_2_SEL>,
>> +                 <&infracfg CLK_IFR_MSDC2_HCLK>,
>> +                 <&infracfg CLK_IFR_MSDC2_SRC>,
>> +                 <&infracfg CLK_IFR_MSDC2_BK>,
>> +                 <&infracfg CLK_IFR_AP_MSDC0>;
>> +            clock-names = "source", "hclk", "source_cg",
>> +                      "bus_clk", "sys_cg";
> 
> clock-names for this do fit in one 90 columns line.
> 
> After compressing it,
> 
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> 

I applied the patch and dropped you Reviewed-by tag. I think it depends on the 
taste of everybody how you prefer to structure this. I've also seen that on 
boards you care (mt8183 in that case) there a clocks that don't adhere to the 
criteria you mention here ;-)

Anyway many thanks for reviewing this and all the great work in general. 
Honestly I feel I didn't made justice by dropping your tag, but as you stated so 
explicitly... :)

Regards,
Matthias
Matthias Brugger March 30, 2023, 5:30 p.m. UTC | #21
On 29/03/2023 15:26, AngeloGioacchino Del Regno wrote:
> Il 29/03/23 10:54, Alexandre Mergnat ha scritto:
>> This patch add support for SuperSpeed USB, in OTG mode, on micro connector.
>> It also add support for the Extensible Host Controller Interface USB.
>>
>> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
>> ---
>>   arch/arm64/boot/dts/mediatek/mt8365-evk.dts | 22 ++++++++++++++++++++++
>>   1 file changed, 22 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/mediatek/mt8365-evk.dts 
>> b/arch/arm64/boot/dts/mediatek/mt8365-evk.dts
>> index 22ec332fe9c9..868ee0d160e4 100644
>> --- a/arch/arm64/boot/dts/mediatek/mt8365-evk.dts
>> +++ b/arch/arm64/boot/dts/mediatek/mt8365-evk.dts
>> @@ -319,6 +319,28 @@ &pwm {
>>       status = "okay";
>>   };
>> +&ssusb {
>> +    pinctrl-0 = <&usb_pins>;
>> +    pinctrl-names = "default";
>> +    maximum-speed = "high-speed";
>> +    usb-role-switch;
>> +    dr_mode = "otg";
>> +    vusb33-supply = <&mt6357_vusb33_reg>;
>> +    status = "okay";
> 
> Order by name please.
> 
> P.S.: status can go at the bottom, even if v < s :-)

You mean v > s ;-)

Yes please reorder keep the status to the bottom that's somehow identical 
throughout the kernel.

Regards,
Matthias

> 
> Thanks,
> Angelo
Matthias Brugger March 30, 2023, 5:31 p.m. UTC | #22
On 29/03/2023 10:54, Alexandre Mergnat wrote:
> This IP is a 10/100 MAC controller compliant with IEEE 802.3 standards.
> It supports power management with Energy Efficient Ethernet and Wake-on-LAN
> specification. Flow control is provided for half-duplex and full-duplex
> mode. For packet transmission and reception, the controller supports
> IPv4/UDP/TCP checksum offload and VLAN tag insertion.
> 
> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>

Applied thanks!

> ---
>   arch/arm64/boot/dts/mediatek/mt8365.dtsi | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/mediatek/mt8365.dtsi b/arch/arm64/boot/dts/mediatek/mt8365.dtsi
> index a67eeca28da5..394a5a61be59 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8365.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt8365.dtsi
> @@ -438,6 +438,18 @@ mmc2: mmc@11250000 {
>   			status = "disabled";
>   		};
>   
> +		ethernet: ethernet@112a0000 {
> +			compatible = "mediatek,mt8365-eth";
> +			reg = <0 0x112a0000 0 0x1000>;
> +			mediatek,pericfg = <&infracfg>;
> +			interrupts = <GIC_SPI 25 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&topckgen CLK_TOP_ETH_SEL>,
> +				 <&infracfg CLK_IFR_NIC_AXI>,
> +				 <&infracfg CLK_IFR_NIC_SLV_AXI>;
> +			clock-names = "core", "reg", "trans";
> +			status = "disabled";
> +		};
> +
>   		u3phy: t-phy@11cc0000 {
>   			compatible = "mediatek,mt8365-tphy", "mediatek,generic-tphy-v2";
>   			#address-cells = <1>;
>
Matthias Brugger March 30, 2023, 5:34 p.m. UTC | #23
On 29/03/2023 10:54, amergnat@baylibre.com wrote:
> From: Amjad Ouled-Ameur <aouledameur@baylibre.com>
> 
> MT8365 has a SYST timer (System Timer), therefore the compatible node
> should be "mediatek,mt6765-timer" instead of "mediatek,mt6795-systimer"
> (which corresponds to ARM/ARM64 System Timer).
> 
> Plus, register range should be 0x100 instead of 0x10.
> 
> Finally, interrupt polarity of systimer is LEVEL_HIGH.
> 
> Fix the above properties accordingly.
> 
> Signed-off-by: Amjad Ouled-Ameur <aouledameur@baylibre.com>
> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>

Patch looks good but does not apply cleanly because of previous patches that I 
didn't take and need rework. Please resend the patches I didn't queue with the 
comments addressed.

Regards,
Matthias

> ---
>   arch/arm64/boot/dts/mediatek/mt8365.dtsi | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/mediatek/mt8365.dtsi b/arch/arm64/boot/dts/mediatek/mt8365.dtsi
> index c3ea3cc97a47..959d8533c24c 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8365.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt8365.dtsi
> @@ -575,9 +575,9 @@ system_clk: dummy13m {
>   	};
>   
>   	systimer: timer@10017000 {
> -		compatible = "mediatek,mt8365-systimer", "mediatek,mt6795-systimer";
> -		reg = <0 0x10017000 0 0x10>;
> -		interrupts = <GIC_SPI 138 IRQ_TYPE_LEVEL_LOW>;
> +		compatible = "mediatek,mt8365-systimer", "mediatek,mt6765-timer";
> +		reg = <0 0x10017000 0 0x100>;
> +		interrupts = <GIC_SPI 138 IRQ_TYPE_LEVEL_HIGH>;
>   		clocks = <&system_clk>;
>   		clock-names = "clk13m";
>   	};
>
Kevin Hilman March 30, 2023, 8:56 p.m. UTC | #24
Alexandre Mergnat <amergnat@baylibre.com> writes:

> Le jeu. 30 mars 2023 à 02:09, Kevin Hilman <khilman@baylibre.com> a écrit :
>>
>> Alexandre Mergnat <amergnat@baylibre.com> writes:
>>
>> > This commits are based on the Fabien Parent <fparent@baylibre.com> work.
>> >
>> > The purpose of this series is to add the following HWs / IPs support for
>> > the mt8365-evk board:
>> > - Watchdog
>> > - Power Management Integrated Circuit "PMIC" wrapper
>> >   - MT6357 PMIC
>> > - MultiMediaCard "MMC" & Secure Digital "SD" controller
>> > - USB controller
>> > - Ethernet MAC controller
>> >
>> > Add CPU Freq & IDLE support for this board.
>> >
>> > This series depends to another one which add support for MT8365 SoC and
>> > EVK board [1].
>>
>> It seems to depend on more than that series.  In order to test this, I
>> tried applying this series on top of Bero's minimal support (now in
>> linux-next), and it does not apply cleanly.
>>
>> Could you please list all the dependencies that are not yet upstream.
>
> Hi Kevin,
> You're right, it also depend to
> https://lore.kernel.org/all/20221122-mt8365-i2c-support-v6-0-e1009c8afd53@baylibre.com/

Nope. Something else is missing too.  I tried this series on top of
Bero's series + i2c series and still doesn't apply cleanly.  Look like
some pinctrl stuff is also missing[1].

Kevin

[1]
 Link: https://lore.kernel.org/r/20230203-evk-board-support-v3-0-0003e80e0095@baylibre.com
 Base: base-commit 555b3a55823ec063129de4403899203febb58788 not known, ignoring
 Base: not specified
       git am /ssd/work/tmp/b4.mbx
Applying: dt-bindings: watchdog: mediatek,mtk-wdt: add mt8365
Applying: dt-bindings: pinctrl: mediatek,mt8365-pinctrl: add drive strength property
error: Documentation/devicetree/bindings/pinctrl/mediatek,mt8365-pinctrl.yaml: does not exist in index
Patch failed at 0002 dt-bindings: pinctrl: mediatek,mt8365-pinctrl: add drive strength property
Alexandre Mergnat March 31, 2023, 9:41 a.m. UTC | #25
Le mer. 29 mars 2023 à 15:17, AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> a écrit :
>
> Il 29/03/23 10:54, amergnat@baylibre.com ha scritto:
> > From: Fabien Parent <fparent@baylibre.com>
> >
> > This new device-tree add the regulators, rtc and keys support
> > for the MT6357 PMIC.
> >
> > Signed-off-by: Fabien Parent <fparent@baylibre.com>
> > Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
> > ---
> >   arch/arm64/boot/dts/mediatek/mt6357.dtsi | 282 +++++++++++++++++++++++++++++++
> >   1 file changed, 282 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/mediatek/mt6357.dtsi b/arch/arm64/boot/dts/mediatek/mt6357.dtsi
> > new file mode 100644
> > index 000000000000..3330a03c2f74
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/mediatek/mt6357.dtsi
> > @@ -0,0 +1,282 @@
>
> ..snip..
>
> > +
> > +             keys {
> > +                     compatible = "mediatek,mt6357-keys";
> > +
> > +                     key-power {
> > +                             linux,keycodes = <KEY_POWER>;
> > +                             wakeup-source;
> > +                     };
> > +
> > +                     key-home {
> > +                             linux,keycodes = <KEY_HOME>;
> > +                             wakeup-source;
>
> KEY_HOME is a wakeup-source?! are you sure?!
>
> (P.S.: I'm just checking if that was intended)

Yes it's the setup used in the official Mediatek Rity project.

Regards,
Alex
Alexandre Mergnat March 31, 2023, 11:08 a.m. UTC | #26
Le mer. 29 mars 2023 à 15:25, AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> a écrit :
>
> Il 29/03/23 10:54, amergnat@baylibre.com ha scritto:
> > From: Fabien Parent <fparent@baylibre.com>
> >
> > MSDC1 IP block is powered by VMC. Make sure it is always on.
>
> Why always on?
> Can't you just set mt6357_vmc_reg as VIN of mt6357_vmch_reg? :-)

I'm not sure to get it. mt6357_vmc_reg & mt6357_vmch_reg come from
PMIC and are supposed to be independent.
You suggest to link them in the mt8365-evk dts file using something like:
&mt6357_vmch_reg {
    vin-supply = <&mt6357_vmc_reg>;
};

Also, regulator binding probably needs change to support that.

Regards,
Alex
Alexandre Mergnat March 31, 2023, 11:23 a.m. UTC | #27
Le ven. 31 mars 2023 à 13:08, Alexandre Mergnat
<amergnat@baylibre.com> a écrit :
>
> Le mer. 29 mars 2023 à 15:25, AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com> a écrit :
> >
> > Il 29/03/23 10:54, amergnat@baylibre.com ha scritto:
> > > From: Fabien Parent <fparent@baylibre.com>
> > >
> > > MSDC1 IP block is powered by VMC. Make sure it is always on.
> >
> > Why always on?
> > Can't you just set mt6357_vmc_reg as VIN of mt6357_vmch_reg? :-)
>
> I'm not sure to get it. mt6357_vmc_reg & mt6357_vmch_reg come from
> PMIC and are supposed to be independent.
> You suggest to link them in the mt8365-evk dts file using something like:
> &mt6357_vmch_reg {
>     vin-supply = <&mt6357_vmc_reg>;
> };

Additionally, vin-supply is supported by fixed-regulator and
gpio-regulator, which is not the case here.
I think it isn't doable.

Regards,
Alex
Alexandre Mergnat March 31, 2023, 12:05 p.m. UTC | #28
Hi Matthias

Le jeu. 30 mars 2023 à 19:34, Matthias Brugger
<matthias.bgg@gmail.com> a écrit :
>
>
>
> On 29/03/2023 10:54, amergnat@baylibre.com wrote:
> > From: Amjad Ouled-Ameur <aouledameur@baylibre.com>
> >
> > MT8365 has a SYST timer (System Timer), therefore the compatible node
> > should be "mediatek,mt6765-timer" instead of "mediatek,mt6795-systimer"
> > (which corresponds to ARM/ARM64 System Timer).
> >
> > Plus, register range should be 0x100 instead of 0x10.
> >
> > Finally, interrupt polarity of systimer is LEVEL_HIGH.
> >
> > Fix the above properties accordingly.
> >
> > Signed-off-by: Amjad Ouled-Ameur <aouledameur@baylibre.com>
> > Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
>
> Patch looks good but does not apply cleanly because of previous patches that I
> didn't take and need rework. Please resend the patches I didn't queue with the
> comments addressed.

Sorry for that, I forgot to drop this patch since it has been already
fixed in the Bero's series [1]
I will drop it for the next version

[1]: https://lore.kernel.org/all/20230309213501.794764-4-bero@baylibre.com/

Regards,
Alex