Devicetree
 help / color / mirror / Atom feed
* [PATCH 2/3] Revert "usb: usb251xb: Add US port lanes inversion property"
From: Lucas Stach @ 2019-07-19  8:44 UTC (permalink / raw)
  To: Richard Leitner, Greg Kroah-Hartman, Rob Herring
  Cc: Mark Rutland, Serge Semin, linux-usb, devicetree, kernel,
	patchwork-lst
In-Reply-To: <20190719084407.28041-1-l.stach@pengutronix.de>

This property isn't needed and not yet used anywhere. The swap-dx-lanes
property is perfectly fine for doing the swap on the upstream port
lanes.

CC: stable@vger.kernel.org #5.2
Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/usb/misc/usb251xb.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
index 4d6ae3795a88..119aeb658c81 100644
--- a/drivers/usb/misc/usb251xb.c
+++ b/drivers/usb/misc/usb251xb.c
@@ -574,8 +574,6 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
 	hub->port_swap = USB251XB_DEF_PORT_SWAP;
 	usb251xb_get_ports_field(hub, "swap-dx-lanes", data->port_cnt,
 				 &hub->port_swap);
-	if (of_get_property(np, "swap-us-lanes", NULL))
-		hub->port_swap |= BIT(0);
 
 	/* The following parameters are currently not exposed to devicetree, but
 	 * may be as soon as needed.
-- 
2.20.1

^ permalink raw reply related

* [PATCH 1/3] Revert "usb: usb251xb: Add US lanes inversion dts-bindings"
From: Lucas Stach @ 2019-07-19  8:44 UTC (permalink / raw)
  To: Richard Leitner, Greg Kroah-Hartman, Rob Herring
  Cc: Mark Rutland, Serge Semin, linux-usb, devicetree, kernel,
	patchwork-lst

This reverts commit 3342ce35a1, as there is no need for this separate
property and it breaks compatibility with existing devicetree files
(arch/arm64/boot/dts/freescale/imx8mq.dtsi).

CC: stable@vger.kernel.org #5.2
Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 Documentation/devicetree/bindings/usb/usb251xb.txt | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/usb251xb.txt b/Documentation/devicetree/bindings/usb/usb251xb.txt
index bc7945e9dbfe..17915f64b8ee 100644
--- a/Documentation/devicetree/bindings/usb/usb251xb.txt
+++ b/Documentation/devicetree/bindings/usb/usb251xb.txt
@@ -64,10 +64,8 @@ Optional properties :
  - power-on-time-ms : Specifies the time it takes from the time the host
 	initiates the power-on sequence to a port until the port has adequate
 	power. The value is given in ms in a 0 - 510 range (default is 100ms).
- - swap-dx-lanes : Specifies the downstream ports which will swap the
-	differential-pair (D+/D-), default is not-swapped.
- - swap-us-lanes : Selects the upstream port differential-pair (D+/D-)
-	swapping (boolean, default is not-swapped)
+ - swap-dx-lanes : Specifies the ports which will swap the differential-pair
+	(D+/D-), default is not-swapped.
 
 Examples:
 	usb2512b@2c {
-- 
2.20.1

^ permalink raw reply related

* RE: [PATCH v2 2/8] PCI: Add ACS quirk for Amazon Annapurna Labs root ports
From: Gustavo Pimentel @ 2019-07-19  8:25 UTC (permalink / raw)
  To: Jonathan Chocron, lorenzo.pieralisi@arm.com, bhelgaas@google.com,
	jingoohan1@gmail.com, gustavo.pimentel@synopsys.com,
	robh+dt@kernel.org, mark.rutland@arm.com
  Cc: dwmw@amazon.co.uk, benh@kernel.crashing.org, alisaidi@amazon.com,
	ronenk@amazon.com, barakw@amazon.com, talel@amazon.com,
	hanochu@amazon.com, hhhawa@amazon.com, linux-pci@vger.kernel.org,
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org
In-Reply-To: <20190718094531.21423-3-jonnyc@amazon.com>

On Thu, Jul 18, 2019 at 10:45:25, Jonathan Chocron <jonnyc@amazon.com> 
wrote:

> From: Ali Saidi <alisaidi@amazon.com>
> 
> The Amazon's Annapurna Labs root ports don't advertise an ACS
> capability, but they don't allow peer-to-peer transactions and do
> validate bus numbers through the SMMU. Additionally, it's not possible
> for one RP to pass traffic to another RP.
> 
> Signed-off-by: Ali Saidi <alisaidi@amazon.com>
> Signed-off-by: Jonathan Chocron <jonnyc@amazon.com>
> ---
>  drivers/pci/quirks.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 208aacf39329..23672680dba7 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -4366,6 +4366,23 @@ static int pci_quirk_qcom_rp_acs(struct pci_dev *dev, u16 acs_flags)
>  	return ret;
>  }
>  
> +static int pci_quirk_al_acs(struct pci_dev *dev, u16 acs_flags)
> +{
> +	/*
> +	 * Amazon's Annapurna Labs root ports don't include an ACS capability,
> +	 * but do include ACS-like functionality. The hardware doesn't support
> +	 * peer-to-peer transactions via the root port and each has a unique
> +	 * segment number.
> +	 * Additionally, the root ports cannot send traffic to each other.
> +	 */
> +	acs_flags &= ~(PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_SV | PCI_ACS_UF);
> +
> +	if (pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT)
> +		return -ENOTTY;
> +
> +	return acs_flags ? 0 : 1;
> +}
> +
>  /*
>   * Sunrise Point PCH root ports implement ACS, but unfortunately as shown in
>   * the datasheet (Intel 100 Series Chipset Family PCH Datasheet, Vol. 2,
> @@ -4559,6 +4576,8 @@ static const struct pci_dev_acs_enabled {
>  	{ PCI_VENDOR_ID_AMPERE, 0xE00A, pci_quirk_xgene_acs },
>  	{ PCI_VENDOR_ID_AMPERE, 0xE00B, pci_quirk_xgene_acs },
>  	{ PCI_VENDOR_ID_AMPERE, 0xE00C, pci_quirk_xgene_acs },
> +	/* Amazon Annapurna Labs */
> +	{ PCI_VENDOR_ID_AMAZON_ANNAPURNA_LABS, 0x0031, pci_quirk_al_acs },
>  	{ 0 }
>  };
>  
> -- 
> 2.17.1

Seems ok.

Reviewed-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>

^ permalink raw reply

* RE: [PATCH v2 3/8] PCI/VPD: Add VPD release quirk for Amazon's Annapurna Labs Root Port
From: Gustavo Pimentel @ 2019-07-19  8:24 UTC (permalink / raw)
  To: Jonathan Chocron, lorenzo.pieralisi@arm.com, bhelgaas@google.com,
	jingoohan1@gmail.com, gustavo.pimentel@synopsys.com,
	robh+dt@kernel.org, mark.rutland@arm.com
  Cc: dwmw@amazon.co.uk, benh@kernel.crashing.org, alisaidi@amazon.com,
	ronenk@amazon.com, barakw@amazon.com, talel@amazon.com,
	hanochu@amazon.com, hhhawa@amazon.com, linux-pci@vger.kernel.org,
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org
In-Reply-To: <20190718094531.21423-4-jonnyc@amazon.com>

On Thu, Jul 18, 2019 at 10:45:26, Jonathan Chocron <jonnyc@amazon.com> 
wrote:

> The Amazon Annapurna Labs PCIe Root Port exposes the VPD capability,
> but there is no actual support for it.
> 
> The reason for not using the already existing quirk_blacklist_vpd()
> is that, although this fails pci_vpd_read/write, the 'vpd' sysfs
> entry still exists. When running lspci -vv, for example, this
> results in the following error:
> 
> pcilib: sysfs_read_vpd: read failed: Input/output error
> 
> This quirk removes the sysfs entry, which avoids the error print.
> 
> Signed-off-by: Jonathan Chocron <jonnyc@amazon.com>
> ---
>  drivers/pci/vpd.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
> index 4963c2e2bd4c..c23a8ec08db9 100644
> --- a/drivers/pci/vpd.c
> +++ b/drivers/pci/vpd.c
> @@ -644,4 +644,20 @@ static void quirk_chelsio_extend_vpd(struct pci_dev *dev)
>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_CHELSIO, PCI_ANY_ID,
>  			quirk_chelsio_extend_vpd);
>  
> +static void quirk_al_vpd_release(struct pci_dev *dev)
> +{
> +	if (dev->vpd) {
> +		pci_vpd_release(dev);
> +		dev->vpd = NULL;
> +		pci_warn(dev, FW_BUG "Releasing VPD capability (No support for VPD read/write transactions)\n");
> +	}
> +}
> +
> +/*
> + * The 0031 device id is reused for other non Root Port device types,
> + * therefore the quirk is registered for the PCI_CLASS_BRIDGE_PCI class.
> + */
> +DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_AMAZON_ANNAPURNA_LABS, 0x0031,
> +			      PCI_CLASS_BRIDGE_PCI, 8, quirk_al_vpd_release);
> +
>  #endif
> -- 
> 2.17.1

Seems ok.

Reviewed-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>

^ permalink raw reply

* Re: [PATCH 0/3] Add DSP node on i.MX8QXP board
From: Daniel Baluta @ 2019-07-19  7:32 UTC (permalink / raw)
  To: Marco Felsch
  Cc: Daniel Baluta, Shawn Guo, Mark Rutland, Aisheng Dong, Peng Fan,
	Anson Huang, Devicetree List, Sascha Hauer, Frank Li,
	Linux Kernel Mailing List, Paul Olaru, Rob Herring, dl-linux-imx,
	Pengutronix Kernel Team, Leonard Crestez, Fabio Estevam,
	S.j. Wang, linux-arm-kernel, sound-open-firmware
In-Reply-To: <20190719070005.mkqvfhjras2jmo52@pengutronix.de>

On Fri, Jul 19, 2019 at 10:00 AM Marco Felsch <m.felsch@pengutronix.de> wrote:
>
> Hi Daniel,
>
> thanks for your patches :) but it's quite common to bundle the driver
> related and the dt related patches. Can you add the firmware related
> patch to this series in your v2?

Sure. Will do that in v2.

^ permalink raw reply

* [PATCH v2 3/3] arm64: dts: freescale: Add support for i.MX8QXP AI_ML board
From: Manivannan Sadhasivam @ 2019-07-19  7:09 UTC (permalink / raw)
  To: shawnguo, s.hauer, robh+dt
  Cc: kernel, festevam, linux-imx, devicetree, linux-arm-kernel,
	linux-kernel, Darshak.Patel, kinjan.patel, prajose.john,
	Manivannan Sadhasivam
In-Reply-To: <20190719070926.29114-1-manivannan.sadhasivam@linaro.org>

Add support for i.MX8QXP AI_ML board from Einfochips. This board is one
of the Consumer Edition boards of the 96Boards family based on i.MX8QXP
SoC from NXP/Freescale.

The initial support includes following peripherals which are tested and
known to be working:

1. Debug serial via UART2
2. uSD
3. WiFi
4. Ethernet

More information about this board can be found in Arrow website:
https://www.arrow.com/en/products/imx8-ai-ml/arrow-development-tools

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 arch/arm64/boot/dts/freescale/Makefile        |   1 +
 .../boot/dts/freescale/imx8qxp-ai_ml.dts      | 249 ++++++++++++++++++
 2 files changed, 250 insertions(+)
 create mode 100644 arch/arm64/boot/dts/freescale/imx8qxp-ai_ml.dts

diff --git a/arch/arm64/boot/dts/freescale/Makefile b/arch/arm64/boot/dts/freescale/Makefile
index 0bd122f60549..bd8460549d1a 100644
--- a/arch/arm64/boot/dts/freescale/Makefile
+++ b/arch/arm64/boot/dts/freescale/Makefile
@@ -24,4 +24,5 @@ dtb-$(CONFIG_ARCH_MXC) += imx8mm-evk.dtb
 dtb-$(CONFIG_ARCH_MXC) += imx8mq-evk.dtb
 dtb-$(CONFIG_ARCH_MXC) += imx8mq-zii-ultra-rmb3.dtb
 dtb-$(CONFIG_ARCH_MXC) += imx8mq-zii-ultra-zest.dtb
+dtb-$(CONFIG_ARCH_MXC) += imx8qxp-ai_ml.dtb
 dtb-$(CONFIG_ARCH_MXC) += imx8qxp-mek.dtb
diff --git a/arch/arm64/boot/dts/freescale/imx8qxp-ai_ml.dts b/arch/arm64/boot/dts/freescale/imx8qxp-ai_ml.dts
new file mode 100644
index 000000000000..3dc8757d9c42
--- /dev/null
+++ b/arch/arm64/boot/dts/freescale/imx8qxp-ai_ml.dts
@@ -0,0 +1,249 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2018 Einfochips
+ * Copyright 2019 Linaro Ltd.
+ */
+
+/dts-v1/;
+
+#include "imx8qxp.dtsi"
+
+/ {
+	model = "Einfochips i.MX8QXP AI_ML";
+	compatible = "einfochips,imx8qxp-ai_ml", "fsl,imx8qxp";
+
+	aliases {
+		serial1 = &adma_lpuart1;
+		serial2 = &adma_lpuart2;
+		serial3 = &adma_lpuart3;
+	};
+
+	chosen {
+		stdout-path = &adma_lpuart2;
+	};
+
+	memory@80000000 {
+		device_type = "memory";
+		reg = <0x00000000 0x80000000 0 0x80000000>;
+	};
+
+	leds {
+		compatible = "gpio-leds";
+		pinctrl-names = "default";
+		pinctrl-0 = <&pinctrl_leds>;
+
+		user_led1 {
+			label = "green:user1";
+			gpios = <&lsio_gpio4 16 GPIO_ACTIVE_HIGH>;
+			linux,default-trigger = "heartbeat";
+		};
+
+		user_led2 {
+			label = "green:user2";
+			gpios = <&lsio_gpio0 6 GPIO_ACTIVE_HIGH>;
+			linux,default-trigger = "none";
+		};
+
+		user_led3 {
+			label = "green:user3";
+			gpios = <&lsio_gpio0 7 GPIO_ACTIVE_HIGH>;
+			linux,default-trigger = "mmc1";
+			default-state = "off";
+		};
+
+		user_led4 {
+			label = "green:user4";
+			gpios = <&lsio_gpio4 21 GPIO_ACTIVE_HIGH>;
+			panic-indicator;
+			linux,default-trigger = "none";
+		};
+
+		wlan_active_led {
+			label = "yellow:wlan";
+			gpios = <&lsio_gpio4 17 GPIO_ACTIVE_HIGH>;
+			linux,default-trigger = "phy0tx";
+			default-state = "off";
+		};
+
+		bt_active_led {
+			label = "blue:bt";
+			gpios = <&lsio_gpio4 18 GPIO_ACTIVE_HIGH>;
+			linux,default-trigger = "hci0-power";
+			default-state = "off";
+		};
+	};
+
+	sdio_pwrseq: sdio-pwrseq {
+		compatible = "mmc-pwrseq-simple";
+		pinctrl-names = "default";
+		pinctrl-0 = <&pinctrl_wifi_reg_on>;
+		reset-gpios = <&lsio_gpio3 24 GPIO_ACTIVE_LOW>;
+	};
+};
+
+/* BT */
+&adma_lpuart0 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_lpuart0>;
+	uart-has-rtscts;
+	status = "okay";
+};
+
+/* LS-UART0 */
+&adma_lpuart1 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_lpuart1>;
+	status = "okay";
+};
+
+/* Debug */
+&adma_lpuart2 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_lpuart2>;
+	status = "okay";
+};
+
+/* PCI-E UART */
+&adma_lpuart3 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_lpuart3>;
+	status = "okay";
+};
+
+&fec1 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_fec1>;
+	phy-mode = "rgmii-id";
+	phy-handle = <&ethphy0>;
+	fsl,magic-packet;
+	status = "okay";
+
+	mdio {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		ethphy0: ethernet-phy@0 {
+			compatible = "ethernet-phy-ieee802.3-c22";
+			reg = <0>;
+		};
+	};
+};
+
+/* WiFi */
+&usdhc1 {
+	#address-cells = <1>;
+	#size-cells = <0>;
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_usdhc1>;
+	bus-width = <4>;
+	no-sd;
+	non-removable;
+	mmc-pwrseq = <&sdio_pwrseq>;
+	status = "okay";
+
+	brcmf: wifi@1 {
+		reg = <1>;
+		compatible = "brcm,bcm4329-fmac";
+	};
+};
+
+/* SD */
+&usdhc2 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_usdhc2>;
+	bus-width = <4>;
+	cd-gpios = <&lsio_gpio4 22 GPIO_ACTIVE_LOW>;
+	status = "okay";
+};
+
+&iomuxc {
+	pinctrl_fec1: fec1grp {
+		fsl,pins = <
+			IMX8QXP_ENET0_MDC_CONN_ENET0_MDC			0x06000020
+			IMX8QXP_ENET0_MDIO_CONN_ENET0_MDIO			0x06000020
+			IMX8QXP_ENET0_RGMII_TX_CTL_CONN_ENET0_RGMII_TX_CTL	0x06000020
+			IMX8QXP_ENET0_RGMII_TXC_CONN_ENET0_RGMII_TXC		0x06000020
+			IMX8QXP_ENET0_RGMII_TXD0_CONN_ENET0_RGMII_TXD0		0x06000020
+			IMX8QXP_ENET0_RGMII_TXD1_CONN_ENET0_RGMII_TXD1		0x06000020
+			IMX8QXP_ENET0_RGMII_TXD2_CONN_ENET0_RGMII_TXD2		0x06000020
+			IMX8QXP_ENET0_RGMII_TXD3_CONN_ENET0_RGMII_TXD3		0x06000020
+			IMX8QXP_ENET0_RGMII_RXC_CONN_ENET0_RGMII_RXC		0x06000020
+			IMX8QXP_ENET0_RGMII_RX_CTL_CONN_ENET0_RGMII_RX_CTL	0x06000020
+			IMX8QXP_ENET0_RGMII_RXD0_CONN_ENET0_RGMII_RXD0		0x06000020
+			IMX8QXP_ENET0_RGMII_RXD1_CONN_ENET0_RGMII_RXD1		0x06000020
+			IMX8QXP_ENET0_RGMII_RXD2_CONN_ENET0_RGMII_RXD2		0x06000020
+			IMX8QXP_ENET0_RGMII_RXD3_CONN_ENET0_RGMII_RXD3		0x06000020
+		>;
+	};
+
+	pinctrl_leds: ledsgrp{
+		fsl,pins = <
+			IMX8QXP_ESAI0_TX2_RX3_LSIO_GPIO0_IO06			0x00000021
+			IMX8QXP_ESAI0_TX3_RX2_LSIO_GPIO0_IO07			0x00000021
+			IMX8QXP_EMMC0_DATA7_LSIO_GPIO4_IO16			0x00000021
+			IMX8QXP_USDHC1_WP_LSIO_GPIO4_IO21			0x00000021
+			IMX8QXP_EMMC0_STROBE_LSIO_GPIO4_IO17			0x00000021
+			IMX8QXP_EMMC0_RESET_B_LSIO_GPIO4_IO18			0x00000021
+		>;
+	};
+
+	pinctrl_lpuart0: lpuart0grp {
+		fsl,pins = <
+			IMX8QXP_UART0_RX_ADMA_UART0_RX				0X06000020
+			IMX8QXP_UART0_TX_ADMA_UART0_TX				0X06000020
+			IMX8QXP_FLEXCAN0_TX_ADMA_UART0_CTS_B 			0x06000020
+			IMX8QXP_FLEXCAN0_RX_ADMA_UART0_RTS_B			0x06000020
+		>;
+	};
+
+	pinctrl_lpuart1: lpuart1grp {
+		fsl,pins = <
+			IMX8QXP_UART1_RX_ADMA_UART1_RX				0X06000020
+			IMX8QXP_UART1_TX_ADMA_UART1_TX				0X06000020
+		>;
+	};
+
+	pinctrl_lpuart2: lpuart2grp {
+		fsl,pins = <
+			IMX8QXP_UART2_RX_ADMA_UART2_RX				0X06000020
+			IMX8QXP_UART2_TX_ADMA_UART2_TX				0X06000020
+		>;
+	};
+
+	pinctrl_lpuart3: lpuart3grp {
+		fsl,pins = <
+			IMX8QXP_FLEXCAN2_RX_ADMA_UART3_RX			0X06000020
+			IMX8QXP_FLEXCAN2_TX_ADMA_UART3_TX			0X06000020
+		>;
+	};
+
+	pinctrl_usdhc1: usdhc1grp {
+		fsl,pins = <
+			IMX8QXP_EMMC0_CLK_CONN_EMMC0_CLK			0x06000041
+			IMX8QXP_EMMC0_CMD_CONN_EMMC0_CMD			0x00000021
+			IMX8QXP_EMMC0_DATA0_CONN_EMMC0_DATA0			0x00000021
+			IMX8QXP_EMMC0_DATA1_CONN_EMMC0_DATA1			0x00000021
+			IMX8QXP_EMMC0_DATA2_CONN_EMMC0_DATA2			0x00000021
+			IMX8QXP_EMMC0_DATA3_CONN_EMMC0_DATA3			0x00000021
+		>;
+	};
+
+	pinctrl_usdhc2: usdhc2grp {
+		fsl,pins = <
+			IMX8QXP_USDHC1_CLK_CONN_USDHC1_CLK			0x06000041
+			IMX8QXP_USDHC1_CMD_CONN_USDHC1_CMD			0x00000021
+			IMX8QXP_USDHC1_DATA0_CONN_USDHC1_DATA0			0x00000021
+			IMX8QXP_USDHC1_DATA1_CONN_USDHC1_DATA1			0x00000021
+			IMX8QXP_USDHC1_DATA2_CONN_USDHC1_DATA2			0x00000021
+			IMX8QXP_USDHC1_DATA3_CONN_USDHC1_DATA3			0x00000021
+			IMX8QXP_USDHC1_VSELECT_CONN_USDHC1_VSELECT		0x00000021
+			IMX8QXP_USDHC1_CD_B_LSIO_GPIO4_IO22			0x00000021
+		>;
+	};
+
+	pinctrl_wifi_reg_on: wifiregongrp {
+		fsl,pins = <
+			IMX8QXP_QSPI0B_SS1_B_LSIO_GPIO3_IO24			0x00000021
+		>;
+	};
+};
-- 
2.17.1

^ permalink raw reply related

* [PATCH v2 2/3] dt-bindings: arm: Document i.MX8QXP AI_ML board binding
From: Manivannan Sadhasivam @ 2019-07-19  7:09 UTC (permalink / raw)
  To: shawnguo, s.hauer, robh+dt
  Cc: kernel, festevam, linux-imx, devicetree, linux-arm-kernel,
	linux-kernel, Darshak.Patel, kinjan.patel, prajose.john,
	Manivannan Sadhasivam
In-Reply-To: <20190719070926.29114-1-manivannan.sadhasivam@linaro.org>

Document devicetree binding of i.MX8QXP AI_ML board from Einfochips.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 Documentation/devicetree/bindings/arm/fsl.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/arm/fsl.yaml b/Documentation/devicetree/bindings/arm/fsl.yaml
index 407138ebc0d0..8e9209a75478 100644
--- a/Documentation/devicetree/bindings/arm/fsl.yaml
+++ b/Documentation/devicetree/bindings/arm/fsl.yaml
@@ -180,6 +180,7 @@ properties:
       - description: i.MX8QXP based Boards
         items:
           - enum:
+              - einfochips,imx8qxp-ai_ml  # i.MX8QXP AI_ML Board
               - fsl,imx8qxp-mek           # i.MX8QXP MEK Board
           - const: fsl,imx8qxp
 
-- 
2.17.1

^ permalink raw reply related

* [PATCH v2 1/3] dt-bindings: Add Vendor prefix for Einfochips
From: Manivannan Sadhasivam @ 2019-07-19  7:09 UTC (permalink / raw)
  To: shawnguo, s.hauer, robh+dt
  Cc: kernel, festevam, linux-imx, devicetree, linux-arm-kernel,
	linux-kernel, Darshak.Patel, kinjan.patel, prajose.john,
	Manivannan Sadhasivam
In-Reply-To: <20190719070926.29114-1-manivannan.sadhasivam@linaro.org>

Add devicetree vendor prefix for Einfochips.
https://www.einfochips.com/

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 1acf806b62bf..9b74c4de5676 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -255,6 +255,8 @@ patternProperties:
     description: Emerging Display Technologies
   "^eeti,.*":
     description: eGalax_eMPIA Technology Inc
+  "^einfochips,.*":
+    description: Einfochips
   "^elan,.*":
     description: Elan Microelectronic Corp.
   "^elgin,.*":
-- 
2.17.1

^ permalink raw reply related

* [PATCH v2 0/3] Add support for i.MXQXP AI_ML board
From: Manivannan Sadhasivam @ 2019-07-19  7:09 UTC (permalink / raw)
  To: shawnguo, s.hauer, robh+dt
  Cc: devicetree, kinjan.patel, linux-kernel, Darshak.Patel, linux-imx,
	kernel, Manivannan Sadhasivam, festevam, linux-arm-kernel,
	prajose.john

Hello,

This patchset adds support for i.MXQXP AI_ML board from Einfochips.
This board is one of the Consumer Edition boards of the 96Boards family
based on i.MX8QXP SoC from NXP/Freescale.

The initial support includes following peripherals which are tested and
known to be working:

1. Debug serial via UART2
2. uSD
3. WiFi
4. Ethernet

More information about this board can be found in Arrow website:
https://www.arrow.com/en/products/imx8-ai-ml/arrow-development-tools

Thanks,
Mani

Changes in v2:

* Incorporated review comments from Dong (small cleanups)

Manivannan Sadhasivam (3):
  dt-bindings: Add Vendor prefix for Einfochips
  dt-bindings: arm: Document i.MX8QXP AI_ML board binding
  arm64: dts: freescale: Add support for i.MX8QXP AI_ML board

 .../devicetree/bindings/arm/fsl.yaml          |   1 +
 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 arch/arm64/boot/dts/freescale/Makefile        |   1 +
 .../boot/dts/freescale/imx8qxp-ai_ml.dts      | 249 ++++++++++++++++++
 4 files changed, 253 insertions(+)
 create mode 100644 arch/arm64/boot/dts/freescale/imx8qxp-ai_ml.dts

-- 
2.17.1

^ permalink raw reply

* Re: [PATCH 0/3] Add DSP node on i.MX8QXP board
From: Marco Felsch @ 2019-07-19  7:00 UTC (permalink / raw)
  To: Daniel Baluta
  Cc: shawnguo, mark.rutland, aisheng.dong, peng.fan, anson.huang,
	devicetree, s.hauer, Frank.Li, linux-kernel, paul.olaru, robh+dt,
	linux-imx, kernel, leonard.crestez, festevam, shengjiu.wang,
	linux-arm-kernel, sound-open-firmware
In-Reply-To: <20190718151346.3523-1-daniel.baluta@nxp.com>

Hi Daniel,

thanks for your patches :) but it's quite common to bundle the driver
related and the dt related patches. Can you add the firmware related
patch to this series in your v2?

Regards,
  Marco

On 19-07-18 18:13, Daniel Baluta wrote:
> i.MX8QXP boards feature an Hifi4 DSP from Tensilica. This patch series
> adds the DT node.
> 
> Note that we switched to the new yaml format for bindings documentation.
> 
> The DSP will run SOF Firmware [1]. Patches adding support for Linux DSP
> driver are already sent for review to SOF folks [2].
> 
> This patch series also contains a patch introducing DT related clocks.
> 
> The patch was already reviewed here:
> 	https://lkml.org/lkml/2019/7/17/975
> 
> but I added it in this patch series because it wasn't yet picked by
> Shawn so patches 2/3 will not compiled without patch 1.
> 
> [1] https://github.com/thesofproject/sof
> [2] https://github.com/thesofproject/linux/pull/1048/commits
> 
> Daniel Baluta (3):
>   clk: imx8: Add DSP related clocks
>   arm64: dts: imx8qxp: Add DSP DT node
>   dt-bindings: dsp: fsl: Add DSP core binding support
> 
>  .../devicetree/bindings/dsp/fsl,dsp.yaml      | 87 +++++++++++++++++++
>  arch/arm64/boot/dts/freescale/imx8qxp-mek.dts |  4 +
>  arch/arm64/boot/dts/freescale/imx8qxp.dtsi    | 32 +++++++
>  drivers/clk/imx/clk-imx8qxp-lpcg.c            |  5 ++
>  include/dt-bindings/clock/imx8-clock.h        |  6 +-
>  5 files changed, 133 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/devicetree/bindings/dsp/fsl,dsp.yaml
> 
> -- 
> 2.17.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

^ permalink raw reply

* Re: [PATCH 3/3] dt-bindings: dsp: fsl: Add DSP core binding support
From: Daniel Baluta @ 2019-07-19  6:34 UTC (permalink / raw)
  To: Leonard Crestez
  Cc: Mark Rutland, Aisheng Dong, Peng Fan, Fabio Estevam, Anson Huang,
	Devicetree List, Daniel Baluta, Sascha Hauer, Frank Li,
	linux-kernel@vger.kernel.org, Paul Olaru, Rob Herring,
	dl-linux-imx, Sascha Hauer, Shawn Guo, S.j. Wang,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	sound-open-firmware@alsa-project.org
In-Reply-To: <VI1PR04MB5055597B7C3AC114FEB7E3E5EEC80@VI1PR04MB5055.eurprd04.prod.outlook.com>

On Thu, Jul 18, 2019 at 9:40 PM Leonard Crestez <leonard.crestez@nxp.com> wrote:
>
> On 18.07.2019 21:24, Daniel Baluta wrote:
> > On Thu, Jul 18, 2019 at 7:41 PM Rob Herring <robh+dt@kernel.org> wrote:
> >>
> >> On Thu, Jul 18, 2019 at 9:13 AM Daniel Baluta <daniel.baluta@nxp.com> wrote:
> >>>
> >>> This describes the DSP device tree node.
> >>>
> >>> Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
>
> >>> +  power-domains:
> >>> +    description:
> >>> +      List of phandle and PM domain specifier as documented in
> >>> +      Documentation/devicetree/bindings/power/power_domain.txt
> >>
> >> How many? 4?
> >
> > Yes, 4 for i.MX8QXP. Also, the same number is for i.MX8QM. Anyhow, I didn't
> > added added a limit here because I really don't know how many will be
> > in upcoming i.MX platforms.
>
> Which 4? It might help to use power-domain-names explicitly just like
> it's done for clocks and mboxes.
>
> This is very common for phandle lists.

4 like in the example at the bottom of the patch:

+   power-domains = <&pd IMX_SC_R_MU_13A>,
+                        <&pd IMX_SC_R_MU_13B>,
+                        <&pd IMX_SC_R_DSP>,
+                        <&pd IMX_SC_R_DSP_RAM>;

Not sure if it makes sense to use power-domain-names as the driver parses
directly the "power-domains" property.

^ permalink raw reply

* Re: [PATCH v3 0/6] Introduce Bandwidth OPPs for interconnect paths
From: Saravana Kannan @ 2019-07-19  4:12 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Georgi Djakov, Rob Herring, Mark Rutland, Viresh Kumar,
	Nishanth Menon, Stephen Boyd, Rafael J. Wysocki, Vincent Guittot,
	Sweeney, Sean, daidavid1, Rajendra Nayak, Sibi Sankar,
	Bjorn Andersson, Evan Green, Android Kernel Team, Linux PM,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML
In-Reply-To: <20190718053746.64drmonk72vwnt4s@vireshk-i7>

On Wed, Jul 17, 2019 at 10:37 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> I know you have explained lots of things earlier as well, but they are
> available over multiple threads and I don't know where to reply now :)
>
> Lets have proper discussion (once again) here and be done with it.
> Sorry for the trouble of explaining things again.
>
> On 17-07-19, 13:34, Saravana Kannan wrote:
> > On Wed, Jul 17, 2019 at 3:32 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > > On 02-07-19, 18:10, Saravana Kannan wrote:
> > > > gpu_cache_opp_table: gpu_cache_opp_table {
> > > >       compatible = "operating-points-v2";
> > > >
> > > >       gpu_cache_3000: opp-3000 {
> > > >               opp-peak-KBps = <3000>;
> > > >               opp-avg-KBps = <1000>;
> > > >       };
> > > >       gpu_cache_6000: opp-6000 {
> > > >               opp-peak-KBps = <6000>;
> > > >               opp-avg-KBps = <2000>;
> > > >       };
> > > >       gpu_cache_9000: opp-9000 {
> > > >               opp-peak-KBps = <9000>;
> > > >               opp-avg-KBps = <9000>;
> > > >       };
> > > > };
> > > >
> > > > gpu_ddr_opp_table: gpu_ddr_opp_table {
> > > >       compatible = "operating-points-v2";
> > > >
> > > >       gpu_ddr_1525: opp-1525 {
> > > >               opp-peak-KBps = <1525>;
> > > >               opp-avg-KBps = <452>;
> > > >       };
> > > >       gpu_ddr_3051: opp-3051 {
> > > >               opp-peak-KBps = <3051>;
> > > >               opp-avg-KBps = <915>;
> > > >       };
> > > >       gpu_ddr_7500: opp-7500 {
> > > >               opp-peak-KBps = <7500>;
> > > >               opp-avg-KBps = <3000>;
> > > >       };
> > > > };
> > >
> > > Who is going to use the above tables and how ?
> >
> > In this example the GPU driver would use these. It'll go through these
> > and then decide what peak and average bw to pick based on whatever
> > criteria.
>
> Are you saying that the GPU driver will decide which bandwidth to
> choose while running at a particular frequency (say 2 GHz) ? And that
> it can choose 1525 or 3051 or 7500 from the ddr path ?
>
> Will it be possible to publicly share how we derive to these decisions
> ?

GPU is just an example. So I can't really speak for how a random GPU
driver might decide the bandwidth to pick.

But one obvious way is to start at the lowest bandwidth and check the
bus port busy%. If it's > 80% busy, it'll pick the next bandwidth,
etc. So, something like what cpufreq ondemand or conservative governor
used to do.

> The thing is I don't like these separate OPP tables which will not be
> used by anyone else, but just GPU (or a single device).

The BW OPP table isn't always a secondary OPP table. It can be a
primary OPP table too. For example, if you have a bandwidth monitoring
device/HW IP that can measure for a path and make requests for that
path, it'll have a BW OPP table and it'll pick from one of those BW
OPP levels based on the hardware measurements. It will have it's own
device driver. This is basically no different from a device being the
only user of a freq OPP table.

> I would like
> to put this data in the GPU OPP table only. What about putting a
> range in the GPU OPP table for the Bandwidth if it can change so much
> for the same frequency.

I don't think the range is going to work. If a GPU is doing purely
computational work, it's not unreasonable for it to vote for the
lowest bandwidth for any GPU frequency.

>
> > > These are the maximum
> > > BW available over these paths, right ?
> >
> > I wouldn't call them "maximum" because there can't be multiple
> > maximums :) But yes, these are the meaningful bandwidth from the GPU's
> > perspective to use over these paths.
> >
> > >
> > > > gpu_opp_table: gpu_opp_table {
> > > >       compatible = "operating-points-v2";
> > > >       opp-shared;
> > > >
> > > >       opp-200000000 {
> > > >               opp-hz = /bits/ 64 <200000000>;
> > > >       };
> > > >       opp-400000000 {
> > > >               opp-hz = /bits/ 64 <400000000>;
> > > >       };
> > > > };
> > >
> > > Shouldn't this link back to the above tables via required-opp, etc ?
> > > How will we know how much BW is required by the GPU device for all the
> > > paths ?
> >
> > If that's what the GPU driver wants to do, then yes. But the GPU
> > driver could also choose to scale the bandwidth for these paths based
> > on multiple other signals. Eg: bus port busy percentage, measure
> > bandwidth, etc.
>
> Lets say that the GPU is running at 2 GHz right now and based on above
> inputs it wants to increase the bandwidth to 7500 for ddr path, now
> does it make sense to run at 4 GHz instead of 2 so we utilize the
> bandwidth to the best of our ability and waste less power ?

This is kinda hard to explain, but I'll try.

Firstly, the GPU power increase might be so high that you might not
want to do this anyway.

Also, what you are proposing *might* improve the perf/mW (efficiency)
but it doesn't decrease the actual power consumption. So, this doesn't
really work towards saving power for mobile devices.

Also, if the GPU is generating a lot of traffic to DDR and you
increase the GPU frequency, it's only going to generate even more
traffic. So you'll end up in a positive feedback loop that maxes out
the frequency and bandwidth. Definitely not something you want for a
mobile device.

> If something like that is acceptable, then what about keeping the
> bandwidth fixed for frequencies and rather scale the frequency of the
> GPU on the inputs your provided (like bus port busy percentage, etc).

I don't think it's acceptable.

> The current proposal makes me wonder on why should we try to reuse OPP
> tables for providing these bandwidth values as the OPP tables for
> interconnect paths isn't really a lot of data, only bandwidth all the
> time and there is no linking from the device's OPP table as well.

I think everyone is getting too tied up on mapping device frequency to
bandwidth requests. That's useful for a limited set of cases. But it
doesn't work for a lot of use cases.

Couple of benefits of using BW OPPs instead of with listing bandwidth
values as part of frequency OPP tables:
- Works better when the interconnect path has more useful levels that
the device frequency levels. I think this might even be true on the
SDM845 for GPU and DDR. The link from freq OPP to BW OPP could list
the minimum bandwidth level to use for a particular device freq and
then let the hardware monitoring heuristic take it higher from there.
- Works even if no freq to bandwidth mapping heuristic is used but the
device needs to skip certain bandwidth levels based on the platform's
power/perf reasons.
- More scalable as more properties are added to BW OPP levels. Traffic
priority is one natural extension of the BW OPP "rows". Explicit
latency is another possibility.
- Currently devices that have use case specific bandwidth levels
(that's not computed at runtime) have no way of capturing their use
case level bandwidth needs in DT. Everyone is inventing their own
scheme. Having BW OPP table would allow them capture all the use case
specific bandwidth levels in DT and then pick one using the
index/phandle/etc. We could even allow naming OPP rows and pick it
that way. Not saying this is a main reason for BW OPP tables or we
should do this, but this is a possibility to consider.

Long story short, BW OPP tables make a lot of sense for anyone who has
actually done bandwidth scaling on a commercial platform.

If people are getting too tied up about the interconnect-opp-table we
can just drop that. I just added that to avoid having any implicit
ordering of tables in the operation-points-v2 property vs
interconnects property and call it out more explicitly. But it's not a
hill worth dying on.

-Saravana

^ permalink raw reply

* [PATCH AUTOSEL 5.2 091/171] dt-bindings: backlight: lm3630a: correct schema validation
From: Sasha Levin @ 2019-07-19  3:55 UTC (permalink / raw)
  To: linux-kernel, stable
  Cc: Sasha Levin, Daniel Thompson, devicetree, dri-devel, Rob Herring,
	Dan Murphy, linux-leds, Brian Masney
In-Reply-To: <20190719035643.14300-1-sashal@kernel.org>

From: Brian Masney <masneyb@onstation.org>

[ Upstream commit ef4db28c1f45cda6989bc8a8e45294894786d947 ]

The '#address-cells' and '#size-cells' properties were not defined in
the lm3630a bindings and would cause the following error when
attempting to validate the examples against the schema:

Documentation/devicetree/bindings/leds/backlight/lm3630a-backlight.example.dt.yaml:
'#address-cells', '#size-cells' do not match any of the regexes:
'^led@[01]$', 'pinctrl-[0-9]+'

Correct this by adding those two properties.

While we're here, move the ti,linear-mapping-mode property to the
led@[01] child nodes to correct the following validation error:

Documentation/devicetree/bindings/leds/backlight/lm3630a-backlight.example.dt.yaml:
led@0: 'ti,linear-mapping-mode' does not match any of the regexes:
'pinctrl-[0-9]+'

Fixes: 32fcb75c66a0 ("dt-bindings: backlight: Add lm3630a bindings")
Signed-off-by: Brian Masney <masneyb@onstation.org>
Reported-by: Rob Herring <robh+dt@kernel.org>
Acked-by: Daniel Thompson <daniel.thompson@linaro.org>
Acked-by: Dan Murphy <dmurphy@ti.com>
[robh: also drop maxItems from child reg]
Signed-off-by: Rob Herring <robh@kernel.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 .../leds/backlight/lm3630a-backlight.yaml     | 21 ++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/leds/backlight/lm3630a-backlight.yaml b/Documentation/devicetree/bindings/leds/backlight/lm3630a-backlight.yaml
index 4d61fe0a98a4..dc129d9a329e 100644
--- a/Documentation/devicetree/bindings/leds/backlight/lm3630a-backlight.yaml
+++ b/Documentation/devicetree/bindings/leds/backlight/lm3630a-backlight.yaml
@@ -23,16 +23,17 @@ properties:
   reg:
     maxItems: 1
 
-  ti,linear-mapping-mode:
-    description: |
-      Enable linear mapping mode. If disabled, then it will use exponential
-      mapping mode in which the ramp up/down appears to have a more uniform
-      transition to the human eye.
-    type: boolean
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 0
 
 required:
   - compatible
   - reg
+  - '#address-cells'
+  - '#size-cells'
 
 patternProperties:
   "^led@[01]$":
@@ -48,7 +49,6 @@ patternProperties:
           in this property. The two current sinks can be controlled
           independently with both banks, or bank A can be configured to control
           both sinks with the led-sources property.
-        maxItems: 1
         minimum: 0
         maximum: 1
 
@@ -73,6 +73,13 @@ patternProperties:
         minimum: 0
         maximum: 255
 
+      ti,linear-mapping-mode:
+        description: |
+          Enable linear mapping mode. If disabled, then it will use exponential
+          mapping mode in which the ramp up/down appears to have a more uniform
+          transition to the human eye.
+        type: boolean
+
     required:
       - reg
 
-- 
2.20.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related

* [v6,2/2] dt-bindings: rtc: add bindings for FlexTimer Module
From: Biwen Li @ 2019-07-19  3:54 UTC (permalink / raw)
  To: a.zummo, alexandre.belloni, leoyang.li, robh+dt
  Cc: linux-rtc, linux-kernel, xiaobo.xie, jiafei.pan, ran.wang_1,
	mark.rutland, devicetree, Biwen Li
In-Reply-To: <20190719035400.9800-1-biwen.li@nxp.com>

The patch adds bindings for FlexTimer Module

Signed-off-by: Biwen Li <biwen.li@nxp.com>
---
Change in v6:
    - correct subject
    - delete note
    - remove reg property about IP Powerdown exception register

Change in v5:
    - None

Change in v4:
    - add note about dts and kernel options
    - add aliases in example

Change in v3:
	- remove reg-names property
	- correct cells number

Change in v2:
	- replace ls1043a with ls1088a as example
	- add rcpm node and fsl,rcpm-wakeup property

 .../bindings/rtc/rtc-fsl-ftm-alarm.txt        | 41 +++++++++++++++++++
 1 file changed, 41 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rtc/rtc-fsl-ftm-alarm.txt

diff --git a/Documentation/devicetree/bindings/rtc/rtc-fsl-ftm-alarm.txt b/Documentation/devicetree/bindings/rtc/rtc-fsl-ftm-alarm.txt
new file mode 100644
index 000000000000..9291b022ffc7
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/rtc-fsl-ftm-alarm.txt
@@ -0,0 +1,41 @@
+Freescale FlexTimer Module (FTM) Alarm
+
+Required properties:
+- compatible : Should be "fsl,<chip>-ftm-alarm", the
+	       supported chips include
+	       "fsl,ls1012a-ftm-alarm"
+	       "fsl,ls1021a-ftm-alarm"
+	       "fsl,ls1028a-ftm-alarm"
+	       "fsl,ls1043a-ftm-alarm"
+	       "fsl,ls1046a-ftm-alarm"
+	       "fsl,ls1088a-ftm-alarm"
+	       "fsl,ls208xa-ftm-alarm"
+- reg : Specifies base physical address and size of the register sets for the
+  FlexTimer Module.
+- interrupts : Should be the FlexTimer Module interrupt.
+- fsl,rcpm-wakeup property and rcpm node : Please refer
+	Documentation/devicetree/bindings/soc/fsl/rcpm.txt
+
+Optional properties:
+- big-endian: If the host controller is big-endian mode, specify this property.
+  The default endian mode is little-endian.
+
+Example:
+aliases {
+	...
+	rtc1 = ftm_alarm0; // Use FlexTimer Module as /dev/rtc1
+	...
+};
+
+rcpm: rcpm@1e34040 {
+	compatible = "fsl,ls1088a-rcpm", "fsl,qoriq-rcpm-2.1+";
+	reg = <0x0 0x1e34040 0x0 0x18>;
+	#fsl,rcpm-wakeup-cells = <6>;
+};
+
+ftm_alarm0: timer@2800000 {
+	compatible = "fsl,ls1088a-ftm-alarm";
+	reg = <0x0 0x2800000 0x0 0x10000>;
+	fsl,rcpm-wakeup = <&rcpm 0x0 0x0 0x0 0x0 0x4000 0x0>;
+	interrupts = <0 44 4>;
+};
-- 
2.17.1

^ permalink raw reply related

* [v6,1/2] rtc/fsl: add FTM alarm driver as the wakeup source
From: Biwen Li @ 2019-07-19  3:53 UTC (permalink / raw)
  To: a.zummo, alexandre.belloni, leoyang.li, robh+dt
  Cc: linux-rtc, linux-kernel, xiaobo.xie, jiafei.pan, ran.wang_1,
	mark.rutland, devicetree, Biwen Li

For the paltforms including LS1012A, LS1021A, LS1028A, LS1043A,
LS1046A, LS1088A, LS208xA that has the FlexTimer
module, implementing alarm functions within RTC subsystem
to wakeup the system when system going to sleep (work with RCPM driver).

Signed-off-by: Biwen Li <biwen.li@nxp.com>
---
Change in v6:
    - None

Change in v5:
    - replace devm_rtc_device_register with devm_rtc_allocate_device
    and rtc_register_device

Change in v4:
    - clean code
    - correct requesting irq
    - register as a regular RTC driver
    - change return value of ftm_rtc_set_alarm from -EINVAL to -ERANGE
    - replace pr_err with dev_err
    - sort alphabetically
    - auto select RCPM driver
    - correct UTC time in ftm_rtc_read_time

Change in v3:
	- add some comments about clock source and errata
	- adjust format
	- replace endian with big_endian of struct ftm_rtc
	- remove compatible "fsl,ftm-alarm"

Change in v2:
	- remove code about setting rcpm

 drivers/rtc/Kconfig             |  15 ++
 drivers/rtc/Makefile            |   1 +
 drivers/rtc/rtc-fsl-ftm-alarm.c | 336 ++++++++++++++++++++++++++++++++
 3 files changed, 352 insertions(+)
 create mode 100644 drivers/rtc/rtc-fsl-ftm-alarm.c

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 5b9b2eec1435..ac6728ba7895 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -1322,6 +1322,21 @@ config RTC_DRV_IMXDI
 	   This driver can also be built as a module, if so, the module
 	   will be called "rtc-imxdi".
 
+config RTC_DRV_FSL_FTM_ALARM
+	tristate "Freescale FlexTimer alarm timer"
+	depends on ARCH_LAYERSCAPE
+	select FSL_RCPM
+	default y
+	help
+	   For the FlexTimer in LS1012A, LS1021A, LS1028A, LS1043A, LS1046A,
+	   LS1088A, LS208xA, we can use FTM as the wakeup source.
+
+	   Say y here to enable FTM alarm support. The FTM alarm provides
+	   alarm functions for wakeup system from deep sleep.
+
+	   This driver can also be built as a module, if so, the module
+	   will be called "rtc-fsl-ftm-alarm".
+
 config RTC_DRV_MESON
 	tristate "Amlogic Meson RTC"
 	depends on (ARM && ARCH_MESON) || COMPILE_TEST
diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index 84deeddc8f14..5b1fa1d3a7e8 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -71,6 +71,7 @@ obj-$(CONFIG_RTC_DRV_EFI)	+= rtc-efi.o
 obj-$(CONFIG_RTC_DRV_EM3027)	+= rtc-em3027.o
 obj-$(CONFIG_RTC_DRV_EP93XX)	+= rtc-ep93xx.o
 obj-$(CONFIG_RTC_DRV_FM3130)	+= rtc-fm3130.o
+obj-$(CONFIG_RTC_DRV_FSL_FTM_ALARM)	+= rtc-fsl-ftm-alarm.o
 obj-$(CONFIG_RTC_DRV_FTRTC010)	+= rtc-ftrtc010.o
 obj-$(CONFIG_RTC_DRV_GENERIC)	+= rtc-generic.o
 obj-$(CONFIG_RTC_DRV_GOLDFISH)	+= rtc-goldfish.o
diff --git a/drivers/rtc/rtc-fsl-ftm-alarm.c b/drivers/rtc/rtc-fsl-ftm-alarm.c
new file mode 100644
index 000000000000..4f7259c2d6a3
--- /dev/null
+++ b/drivers/rtc/rtc-fsl-ftm-alarm.c
@@ -0,0 +1,336 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Freescale FlexTimer Module (FTM) alarm device driver.
+ *
+ * Copyright 2014 Freescale Semiconductor, Inc.
+ * Copyright 2019 NXP
+ *
+ */
+
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/platform_device.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/module.h>
+#include <linux/fsl/ftm.h>
+#include <linux/rtc.h>
+#include <linux/time.h>
+
+#define FTM_SC_CLK(c)		((c) << FTM_SC_CLK_MASK_SHIFT)
+
+/*
+ * Select Fixed frequency clock (32KHz) as clock source
+ * of FlexTimer Module
+ */
+#define FTM_SC_CLKS_FIXED_FREQ	0x02
+#define FIXED_FREQ_CLK		32000
+
+/* Select 128 (2^7) as divider factor */
+#define MAX_FREQ_DIV		(1 << FTM_SC_PS_MASK)
+
+/* Maximum counter value in FlexTimer's CNT registers */
+#define MAX_COUNT_VAL		0xffff
+
+struct ftm_rtc {
+	struct rtc_device *rtc_dev;
+	void __iomem *base;
+	bool big_endian;
+	u32 alarm_freq;
+};
+
+static inline u32 rtc_readl(struct ftm_rtc *dev, u32 reg)
+{
+	if (dev->big_endian)
+		return ioread32be(dev->base + reg);
+	else
+		return ioread32(dev->base + reg);
+}
+
+static inline void rtc_writel(struct ftm_rtc *dev, u32 reg, u32 val)
+{
+	if (dev->big_endian)
+		iowrite32be(val, dev->base + reg);
+	else
+		iowrite32(val, dev->base + reg);
+}
+
+static inline void ftm_counter_enable(struct ftm_rtc *rtc)
+{
+	u32 val;
+
+	/* select and enable counter clock source */
+	val = rtc_readl(rtc, FTM_SC);
+	val &= ~(FTM_SC_PS_MASK | FTM_SC_CLK_MASK);
+	val |= (FTM_SC_PS_MASK | FTM_SC_CLK(FTM_SC_CLKS_FIXED_FREQ));
+	rtc_writel(rtc, FTM_SC, val);
+}
+
+static inline void ftm_counter_disable(struct ftm_rtc *rtc)
+{
+	u32 val;
+
+	/* disable counter clock source */
+	val = rtc_readl(rtc, FTM_SC);
+	val &= ~(FTM_SC_PS_MASK | FTM_SC_CLK_MASK);
+	rtc_writel(rtc, FTM_SC, val);
+}
+
+static inline void ftm_irq_acknowledge(struct ftm_rtc *rtc)
+{
+	unsigned int timeout = 100;
+
+	/*
+	 *Fix errata A-007728 for flextimer
+	 *	If the FTM counter reaches the FTM_MOD value between
+	 *	the reading of the TOF bit and the writing of 0 to
+	 *	the TOF bit, the process of clearing the TOF bit
+	 *	does not work as expected when FTMx_CONF[NUMTOF] != 0
+	 *	and the current TOF count is less than FTMx_CONF[NUMTOF].
+	 *	If the above condition is met, the TOF bit remains set.
+	 *	If the TOF interrupt is enabled (FTMx_SC[TOIE] = 1),the
+	 *	TOF interrupt also remains asserted.
+	 *
+	 *	Above is the errata discription
+	 *
+	 *	In one word: software clearing TOF bit not works when
+	 *	FTMx_CONF[NUMTOF] was seted as nonzero and FTM counter
+	 *	reaches the FTM_MOD value.
+	 *
+	 *	The workaround is clearing TOF bit until it works
+	 *	(FTM counter doesn't always reache the FTM_MOD anyway),
+	 *	which may cost some cycles.
+	 */
+	while ((FTM_SC_TOF & rtc_readl(rtc, FTM_SC)) && timeout--)
+		rtc_writel(rtc, FTM_SC, rtc_readl(rtc, FTM_SC) & (~FTM_SC_TOF));
+}
+
+static inline void ftm_irq_enable(struct ftm_rtc *rtc)
+{
+	u32 val;
+
+	val = rtc_readl(rtc, FTM_SC);
+	val |= FTM_SC_TOIE;
+	rtc_writel(rtc, FTM_SC, val);
+}
+
+static inline void ftm_irq_disable(struct ftm_rtc *rtc)
+{
+	u32 val;
+
+	val = rtc_readl(rtc, FTM_SC);
+	val &= ~FTM_SC_TOIE;
+	rtc_writel(rtc, FTM_SC, val);
+}
+
+static inline void ftm_reset_counter(struct ftm_rtc *rtc)
+{
+	/*
+	 * The CNT register contains the FTM counter value.
+	 * Reset clears the CNT register. Writing any value to COUNT
+	 * updates the counter with its initial value, CNTIN.
+	 */
+	rtc_writel(rtc, FTM_CNT, 0x00);
+}
+
+static void ftm_clean_alarm(struct ftm_rtc *rtc)
+{
+	ftm_counter_disable(rtc);
+
+	rtc_writel(rtc, FTM_CNTIN, 0x00);
+	rtc_writel(rtc, FTM_MOD, ~0U);
+
+	ftm_reset_counter(rtc);
+}
+
+static irqreturn_t ftm_rtc_alarm_interrupt(int irq, void *dev)
+{
+	struct ftm_rtc *rtc = dev;
+
+	ftm_irq_acknowledge(rtc);
+	ftm_irq_disable(rtc);
+	ftm_clean_alarm(rtc);
+
+	return IRQ_HANDLED;
+}
+
+static int ftm_rtc_alarm_irq_enable(struct device *dev,
+		unsigned int enabled)
+{
+	struct ftm_rtc *rtc = dev_get_drvdata(dev);
+
+	if (enabled)
+		ftm_irq_enable(rtc);
+	else
+		ftm_irq_disable(rtc);
+
+	return 0;
+}
+
+/*
+ * Note:
+ *	The function is not really getting time from the RTC
+ *	since FlexTimer is not a RTC device, but we need to
+ *	get time to setup alarm, so we are using system time
+ *	for now.
+ */
+static int ftm_rtc_read_time(struct device *dev, struct rtc_time *tm)
+{
+	struct timespec64 ts64;
+
+	ktime_get_real_ts64(&ts64);
+	rtc_time_to_tm(ts64.tv_sec, tm);
+
+	return 0;
+}
+
+static int ftm_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alm)
+{
+	return 0;
+}
+
+/*
+ * 1. Select fixed frequency clock (32KHz) as clock source;
+ * 2. Select 128 (2^7) as divider factor;
+ * So clock is 250 Hz (32KHz/128).
+ *
+ * 3. FlexTimer's CNT register is a 32bit register,
+ * but the register's 16 bit as counter value,it's other 16 bit
+ * is reserved.So minimum counter value is 0x0,maximum counter
+ * value is 0xffff.
+ * So max alarm value is 262 (65536 / 250) seconds
+ */
+static int ftm_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alm)
+{
+	struct rtc_time tm;
+	unsigned long now, alm_time, cycle;
+	struct ftm_rtc *rtc = dev_get_drvdata(dev);
+
+	ftm_rtc_read_time(dev, &tm);
+	rtc_tm_to_time(&tm, &now);
+	rtc_tm_to_time(&alm->time, &alm_time);
+
+	ftm_clean_alarm(rtc);
+	cycle = (alm_time - now) * rtc->alarm_freq;
+	if (cycle > MAX_COUNT_VAL) {
+		pr_err("Out of alarm range {0~262} seconds.\n");
+		return -ERANGE;
+	}
+
+	ftm_irq_disable(rtc);
+
+	/*
+	 * The counter increments until the value of MOD is reached,
+	 * at which point the counter is reloaded with the value of CNTIN.
+	 * The TOF (the overflow flag) bit is set when the FTM counter
+	 * changes from MOD to CNTIN. So we should using the cycle - 1.
+	 */
+	rtc_writel(rtc, FTM_MOD, cycle - 1);
+
+	ftm_counter_enable(rtc);
+	ftm_irq_enable(rtc);
+
+	return 0;
+
+}
+
+static const struct rtc_class_ops ftm_rtc_ops = {
+	.read_time		= ftm_rtc_read_time,
+	.read_alarm		= ftm_rtc_read_alarm,
+	.set_alarm		= ftm_rtc_set_alarm,
+	.alarm_irq_enable	= ftm_rtc_alarm_irq_enable,
+};
+
+static int ftm_rtc_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct resource *r;
+	int irq;
+	int ret;
+	struct ftm_rtc *rtc;
+
+	rtc = devm_kzalloc(&pdev->dev, sizeof(*rtc), GFP_KERNEL);
+	if (unlikely(!rtc)) {
+		dev_err(&pdev->dev, "cannot alloc memery for rtc\n");
+		return -ENOMEM;
+	}
+
+	platform_set_drvdata(pdev, rtc);
+
+	rtc->rtc_dev = devm_rtc_allocate_device(&pdev->dev);
+	if (IS_ERR(rtc->rtc_dev))
+		return PTR_ERR(rtc->rtc_dev);
+
+	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!r) {
+		dev_err(&pdev->dev, "cannot get resource for rtc\n");
+		return -ENODEV;
+	}
+
+	rtc->base = devm_ioremap_resource(&pdev->dev, r);
+	if (IS_ERR(rtc->base)) {
+		dev_err(&pdev->dev, "cannot ioremap resource for rtc\n");
+		return PTR_ERR(rtc->base);
+	}
+
+	irq = irq_of_parse_and_map(np, 0);
+	if (irq <= 0) {
+		dev_err(&pdev->dev, "unable to get IRQ from DT, %d\n", irq);
+		return -EINVAL;
+	}
+
+	ret = devm_request_irq(&pdev->dev, irq, ftm_rtc_alarm_interrupt,
+			       IRQF_NO_SUSPEND, dev_name(&pdev->dev), rtc);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to request irq\n");
+		return ret;
+	}
+
+	rtc->big_endian = of_property_read_bool(np, "big-endian");
+	rtc->alarm_freq = (u32)FIXED_FREQ_CLK / (u32)MAX_FREQ_DIV;
+	rtc->rtc_dev->ops = &ftm_rtc_ops;
+
+	device_init_wakeup(&pdev->dev, true);
+
+	ret = rtc_register_device(rtc->rtc_dev);
+	if (ret) {
+		dev_err(&pdev->dev, "can't register rtc device\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static const struct of_device_id ftm_rtc_match[] = {
+	{ .compatible = "fsl,ls1012a-ftm-alarm", },
+	{ .compatible = "fsl,ls1021a-ftm-alarm", },
+	{ .compatible = "fsl,ls1043a-ftm-alarm", },
+	{ .compatible = "fsl,ls1046a-ftm-alarm", },
+	{ .compatible = "fsl,ls1088a-ftm-alarm", },
+	{ .compatible = "fsl,ls208xa-ftm-alarm", },
+	{ .compatible = "fsl,ls1028a-ftm-alarm", },
+	{ },
+};
+
+static struct platform_driver ftm_rtc_driver = {
+	.probe		= ftm_rtc_probe,
+	.driver		= {
+		.name	= "ftm-alarm",
+		.of_match_table = ftm_rtc_match,
+	},
+};
+
+static int __init ftm_alarm_init(void)
+{
+	return platform_driver_register(&ftm_rtc_driver);
+}
+
+device_initcall(ftm_alarm_init);
+
+MODULE_DESCRIPTION("NXP/Freescale FlexTimer alarm driver");
+MODULE_AUTHOR("Biwen Li <biwen.li@nxp.com>");
+MODULE_LICENSE("GPL");
-- 
2.17.1

^ permalink raw reply related

* Re: [PATCH v3 2/4] of/platform: Add functional dependency link from DT bindings
From: Frank Rowand @ 2019-07-19  2:49 UTC (permalink / raw)
  To: Rob Herring
  Cc: Saravana Kannan, Mark Rutland, Greg Kroah-Hartman,
	Rafael J. Wysocki,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	linux-kernel@vger.kernel.org, David Collins, Android Kernel Team
In-Reply-To: <CAL_JsqLySLMLanBJvyWqFGhVzXrEaUP-3t9MDmpnAXhQA_7y=g@mail.gmail.com>

On 7/16/19 3:56 PM, Rob Herring wrote:
> On Mon, Jul 15, 2019 at 7:05 PM Frank Rowand <frowand.list@gmail.com> wrote:
>>
>> On 7/15/19 11:40 AM, Saravana Kannan wrote:
>>> Replying again because the previous email accidentally included HTML.
>>>
>>> Thanks for taking the time to reconsider the wording Frank. Your
>>> intention was clear to me in the first email too.
>>>
>>> A kernel command line option can also completely disable this
>>> functionality easily and cleanly. Can we pick that as an option? I've
>>> an implementation of that in the v5 series I sent out last week.
>>
>> Yes, Rob suggested a command line option for debugging, and I am fine with
>> that.  But even with that, I would like a lot of testing so that we have a
>> chance of finding systems that have trouble with the changes and could
>> potentially be fixed before impacting a large number of users.
> 
> Leaving it in -next for more than a cycle will not help. There's some

I have to agree with your scepticism of the value of -next for this
specific case.  But I think there is a _tiny_ potential of additional
testing if the feature is in more than one -next cycle.

> number of users who test linux-next. Then there's more that test -rc
> kernels. Then there's more that test final releases and/or stable
> kernels. Probably, the more stable the h/w, the more it tends to be
> latter groups. (I don't get reports of breaking PowerMacs with the
> changes sitting in linux-next.)
> 
> My main worry about this being off by default is it won't get tested.
> I'm not sure there's enough interest to drive folks to turn it on and
> test. Maybe it needs to be on until we see breakage.

Agreed, but worried about the potential disruption when breakage
occurs.

-Frank

> 
> Rob
> .
> 

^ permalink raw reply

* Re: [PATCH v14 2/2] dt-bindings: spi: Document Renesas R-Car Gen3 RPC-IF controller bindings
From: masonccyang @ 2019-07-19  2:38 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Boris Brezillon, Mark Brown,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Geert Uytterhoeven, Simon Horman, juliensu, Lee Jones,
	Linux Kernel Mailing List, Linux-Renesas, linux-spi, Marek Vasut,
	Mark Rutland, Miquel Raynal, Rob Herring, Sergei Shtylyov
In-Reply-To: <CAMuHMdUPmj0tAhJ18DhQEbYxP7g4MMBMe0U_2eQFQ7mOq4ZFLQ@mail.gmail.com>


Hi Geert, 
 
Thanks for your review!

Will fix it as 

+Renesas R-Car Gen3 RPC-IF controller Device Tree Bindings
+---------------------------------------------------------
+
+Required properties:
+- compatible: should be an SoC-specific compatible value, followed by
+                                "renesas,rcar-gen3-rpc" as a fallback.
+                                supported SoC-specific values are:
+                                "renesas,r8a77980-rpc"          (R-Car 
V3H)
+                                "renesas,r8a77995-rpc"          (R-Car 
D3)
+- reg: should contain three register areas:
+       first for the base address of RPC-IF registers,
+       second for the direct mapping read mode and
+       third for the write buffer area.
+- reg-names: should contain "regs", "dirmap" and "wbuf"
+- clocks: should contain the clock phandle/specifier pair for the module 
clock.
+- clock-names: should contain "rpc"
+- power-domains: should contain the power domain phandle/secifier pair.
+- resets: should contain the reset controller phandle/specifier pair.
+- #address-cells: should be 1
+- #size-cells: should be 0
+
+  flash: should be represented by a subnode of the RPC-IF node, 
+  which "compatible" property contains "jedec,spi-nor", it presents SPI 
is used.
+
+Example:
+
+                rpc: spi@ee200000 {
+                                compatible = "renesas,r8a77995-rpc", 
"renesas,rcar-gen3-rpc";
+                                reg = <0 0xee200000 0 0x200>, <0 
0x08000000 0 0x4000000>,
+                                      <0 0xee208000 0 0x100>;
+                                reg-names = "regs", "dirmap", "wbuf";
+                                clocks = <&cpg CPG_MOD 917>;
+                                clock-names = "rpc";
+                                power-domains = <&sysc 
R8A77995_PD_ALWAYS_ON>;
+                                resets = <&cpg 917>;
+                                #address-cells = <1>;
+                                #size-cells = <0>;
+
+                                flash@0 {
+                                                compatible = 
"jedec,spi-nor";
+                                                reg = <0>;
+                                                spi-max-frequency = 
<40000000>;
+                                                spi-tx-bus-width = <1>;
+                                                spi-rx-bus-width = <1>;
+                                };
+                };

Is it OK ?

thanks & best regards,
Mason


CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information 
and/or personal data, which is protected by applicable laws. Please be 
reminded that duplication, disclosure, distribution, or use of this e-mail 
(and/or its attachments) or any part thereof is prohibited. If you receive 
this e-mail in error, please notify us immediately and delete this mail as 
well as its attachment(s) from your system. In addition, please be 
informed that collection, processing, and/or use of personal data is 
prohibited unless expressly permitted by personal data protection laws. 
Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================



============================================================================

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================

^ permalink raw reply

* Re: [PATCH v9 04/18] kunit: test: add kunit_stream a std::stream like logger
From: Brendan Higgins @ 2019-07-19  0:08 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Frank Rowand, Greg KH, Josh Poimboeuf, Kees Cook, Kieran Bingham,
	Luis Chamberlain, Peter Zijlstra, Rob Herring, shuah,
	Theodore Ts'o, Masahiro Yamada, devicetree, dri-devel,
	kunit-dev, open list:DOCUMENTATION, linux-fsdevel, linux-kbuild,
	Linux Kernel Mailing List, KERNEL 
In-Reply-To: <CAFd5g46a7C1+R6ZcE_SkqaYqgrH5Rx3M=X7orFyaMgFLDbeYYA@mail.gmail.com>

On Thu, Jul 18, 2019 at 12:22:33PM -0700, Brendan Higgins wrote:
> On Thu, Jul 18, 2019 at 10:50 AM Stephen Boyd <sboyd@kernel.org> wrote:
> >
> > Quoting Brendan Higgins (2019-07-16 11:52:01)
> > > On Tue, Jul 16, 2019 at 10:50 AM Stephen Boyd <sboyd@kernel.org> wrote:
[...]
> > Do you have a link to those earlier patches?
> 
> This is the first patchset:
> 
> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1788057.html
> 
> In particular you can see the code for matching functions here:
> 
> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1788073.html
> 
> And parameter matching code here:
> 
> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1788072.html
> 
> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1788086.html
> 
> My apologies in advance, but the code at this early stage had not
> adopted the kunit_* prefix and was still using the test_* and mock_*
> prefix. (Hence, struct kunit_stream was known as struct test_stream).
[...]
> > The crux of my complaint is that the string stream API is too loosely
> > defined to be usable. It allows tests to build up a string of
> > unstructured information, but with certain calling constraints so we
> > have to tread carefully. If there was more structure to the data that's
> > being recorded then the test case runner could operate on the data
> > without having to do string/stream operations, allocations, etc. This
> > would make the assertion logic much more concrete and specific to kunit,
> > instead of this small kunit wrapper that's been placed on top of string
> > stream.
> 
> Yeah, I can see the point of wanting something that provides more
> structure than the raw `struct kunit_stream` interface. In fact, it is
> something I had already started working on, when I had determined it
> would be a large effort to capture all the variations. I was further
> put off from the idea when I had been asked to convert the KUnit
> intermediate format from what I was using to TAP, because, as it is,
> the current data printed out by KUnit doesn't contain all the data I
> would like to put in it in a way that best takes advantage of the TAP
> specification. One problematic area in particular: TAP already
> provides a way to present a lot of the data I would like to export,
> but it involves JSON serialization which was an idea that some of the
> other reviewers understandably weren't too keen on. TAP also wants to
> report data some time after it is available, which is generally not a
> good idea for test debug information; you want to make it available as
> soon as you can or you risk crashing with the data still inside.
> 
> Hence, I decided we could probably spend a good long while debating
> how I present the information. So the idea of having a loose
> definition seemed attractive to me in its own right since it would
> likely conform to whatever we ended up deciding in the long run. Also,
> all the better that it was what I already had and no one seemed to
> mind too much.
> 
> The only constant I expect is that `struct kunit` will likely need to
> take an abstract object with a `commit` method, or a `format` method
> or whatever so it could control when data was going to be printed out
> to the user. We will probably also use a string builder in there
> somewhere.
> 
> > TL;DR: If we can get rid of the string stream API I'd view that as an
> > improvement because building arbitrary strings in the kernel is complex,
> > error prone and has calling context concerns.
> 
> True. No argument there.
> 
> > Is the intention that other code besides unit tests will use this string
> > stream API to build up strings? Any targets in mind? This would be a
> > good way to get the API merged upstream given that its 2019 and we
> > haven't had such an API in the kernel so far.
> 
> Someone, (was it you?) asked about code sharing with a string builder
> thingy that was used for creating structured human readable files, but
> that seemed like a pretty massive undertaking.
> 
> Aside from that, no. I would kind of prefered that nobody used it for
> anything else because I the issues you described.
> 
> Nevertheless, I think the debate over the usefulness of the
> string_stream and kunit_stream are separate topics. Even if we made
> kunit_stream more structured, I am pretty sure I would want to use
> string_stream or some variation for constructing the message.
> 
> > An "object oriented" (strong quotes!) approach where kunit_fail_msg is
> > the innermost struct in some assertion specific structure might work
> > nicely and allow the test runner to call a generic 'format' function to
> > print out the message based on the type of assertion/expectation it is.
> > It probably would mean less code size too because the strings that are
> > common will be in the common printing function instead of created twice,
> > in the macros/code and then copied to the heap for the string stream.
> >
> >         struct kunit_assert {
> >                 const char *line;
> >                 const char *file;
> >                 const char *func;
> >                 void (*format)(struct kunit_assert *assert);
> >         };
> >
> >         struct kunit_comparison_assert {
> >                 enum operator operator;
> >                 const char *left;
> >                 const char *right;
> >                 struct kunit_assert assert;
> >         };
> >
> >         struct kunit_bool_assert {
> >                 const char *truth;
> >                 const char *statement;
> >                 struct kunit_assert assert;
> >         };
> >
> >         void kunit_format_comparison(struct kunit_assert *assert)
> >         {
> >                 struct kunit_comparison_assert *comp = container_of(assert, ...)
> >
> >                 kunit_printk(...)
> >         }

I started poking around with your suggestion while we are waiting. A
couple early observations:

1) It is actually easier to do than I previously thought and will probably
   help with getting more of the planned TAP output stuff working.

   That being said, this is still a pretty substantial undertaking and
   will likely take *at least* a week to implement and properly review.
   Assuming everything goes extremely well (no unexpected issues on my
   end, very responsive reviewers, etc).

2) It *will* eliminate the need for kunit_stream.

3) ...but, it *will not* eliminate the need for string_stream.

Based on my early observations, I do think it is worth doing, but I
don't think it is worth trying to make it in this patchset (unless I
have already missed the window, or it is going to be open for a while):
I do think it will make things much cleaner, but I don't think it will
achieve your desired goal of getting rid of an unstructured
{kunit|string}_stream style interface; it just adds a layer on top of it
that makes it harder to misuse.

I attached a patch of what I have so far at the end of this email so you
can see what I am talking about. And of course, if you agree with my
assessment, so we can start working on it as a future patch.

A couple things in regard to the patch I attached:

1) I wrote it pretty quickly so there are almost definitely mistakes.
   You should consider it RFC. I did verify it compiles though.

2) Also, I did use kunit_stream in writing it: all occurences should be
   pretty easy to replace with string_stream; nevertheless, the reason
   for this is just to make it easier to play with the current APIs. I
   wanted to have something working before I went through a big tedious
   refactoring. So sorry if it causes any confusion.

3) I also based the patch on all the KUnit patches I have queued up
   (includes things like mocking and such) since I want to see how this
   serialization thing will work with mocks and matchers and things like
   that.

> I started working on something similarish, but by the time I ended up
> coming up with a parent object whose definition was loose enough to
> satisfy all the properties required by the child classes it ended up
> basically being the same as what I have now just with a more complex
> hierarchy of message manipulation logic.
> 
> On the other hand, I didn't have the idea of doing the parent object
> quite the way you did and that would clean up a lot of the duplicated
> first line logic.
> 
> I would like to give it a try, but I am afraid I am going to get
> sucked down a really deep rabbit hole.
> 
> > Maybe other people have opinions here on if you should do it now or
> > later. Future coding is not a great argument because it's hard to
> > predict the future. On the other hand, this patchset is in good shape to
> 
> Yeah, that's kind of why I am afraid to go down this road when I have
> something that works now and I know works with the mocking stuff I
> want to do.
> 
> I would like to try your suggestion, but I want to try to make it work
> with my mocking patches before I commit to it because otherwise I am
> just going to have to back it out in a follow up patchset.
> 
> > merge and I'd like to use it to write unit tests for code I maintain so
> > I don't want to see this stall out. Sorry if I'm opening the can of
> > worms you're talking about.
> 
> Don't be sorry. I agree with you that the kunit_stream stuff is not very pretty.
> 
> Shuah, have we missed the merge window for 5.3?
> 
> I saw you only sent one PR out so far for this release, and there
> wasn't much in it; I imagine you are going to send at least one more?
> 
> I figure, if we still got time to try out your suggestion, Stephen, no
> harm in trying.
> 
> Also if we missed it, then I have another couple months to play around with it.
> 
> What do you think?

I attached the patch mentioned above below. Let me know what you think!

Cheers!

>From 53d475d3d56afcf92b452c6d347dbedfa1a17d34 Mon Sep 17 00:00:00 2001
From: Brendan Higgins <brendanhiggins@google.com>
Date: Thu, 18 Jul 2019 16:08:52 -0700
Subject: [PATCH v1] DO NOT MERGE: started playing around with the
 serialization api

---
 include/kunit/assert.h | 130 ++++++++++++++++++++++++++++++
 include/kunit/mock.h   |   4 +
 kunit/Makefile         |   3 +-
 kunit/assert.c         | 179 +++++++++++++++++++++++++++++++++++++++++
 kunit/mock.c           |   6 +-
 5 files changed, 318 insertions(+), 4 deletions(-)
 create mode 100644 include/kunit/assert.h
 create mode 100644 kunit/assert.c

diff --git a/include/kunit/assert.h b/include/kunit/assert.h
new file mode 100644
index 0000000000000..e054fdff4642f
--- /dev/null
+++ b/include/kunit/assert.h
@@ -0,0 +1,130 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Assertion and expectation serialization API.
+ *
+ * Copyright (C) 2019, Google LLC.
+ * Author: Brendan Higgins <brendanhiggins@google.com>
+ */
+
+#ifndef _KUNIT_ASSERT_H
+#define _KUNIT_ASSERT_H
+
+#include <kunit/test.h>
+#include <kunit/mock.h>
+
+enum kunit_assert_type {
+	KUNIT_ASSERTION,
+	KUNIT_EXPECTATION,
+};
+
+struct kunit_assert {
+	enum kunit_assert_type type;
+	const char *line;
+	const char *file;
+	struct va_format message;
+	void (*format)(struct kunit_assert *assert,
+		       struct kunit_stream *stream);
+};
+
+void kunit_base_assert_format(struct kunit_assert *assert,
+			      struct kunit_stream *stream);
+
+void kunit_assert_print_msg(struct kunit_assert *assert,
+			    struct kunit_stream *stream);
+
+struct kunit_unary_assert {
+	struct kunit_assert assert;
+	const char *condition;
+	bool expected_true;
+};
+
+void kunit_unary_assert_format(struct kunit_assert *assert,
+			       struct kunit_stream *stream);
+
+struct kunit_ptr_not_err_assert {
+	struct kunit_assert assert;
+	const char *text;
+	void *value;
+};
+
+void kunit_ptr_not_err_assert_format(struct kunit_assert *assert,
+				     struct kunit_stream *stream);
+
+struct kunit_binary_assert {
+	struct kunit_assert assert;
+	const char *operation;
+	const char *left_text;
+	long long left_value;
+	const char *right_text;
+	long long right_value;
+};
+
+void kunit_binary_assert_format(struct kunit_assert *assert,
+				struct kunit_stream *stream);
+
+struct kunit_binary_ptr_assert {
+	struct kunit_assert assert;
+	const char *operation;
+	const char *left_text;
+	void *left_value;
+	const char *right_text;
+	void *right_value;
+};
+
+void kunit_binary_ptr_assert_format(struct kunit_assert *assert,
+				    struct kunit_stream *stream);
+
+struct kunit_binary_str_assert {
+	struct kunit_assert assert;
+	const char *operation;
+	const char *left_text;
+	const char *left_value;
+	const char *right_text;
+	const char *right_value;
+};
+
+void kunit_binary_str_assert_format(struct kunit_assert *assert,
+				    struct kunit_stream *stream);
+
+struct kunit_mock_assert {
+	struct kunit_assert assert;
+};
+
+struct kunit_mock_no_expectations {
+	struct kunit_mock_assert assert;
+};
+
+struct kunit_mock_declaration {
+	const char *function_name;
+	const char **type_names;
+	const void **params;
+	int len;
+};
+
+void kunit_mock_declaration_format(struct kunit_mock_declaration *declaration,
+				   struct kunit_stream *stream);
+
+struct kunit_matcher_result {
+	struct kunit_assert assert;
+};
+
+struct kunit_mock_failed_match {
+	struct list_head node;
+	const char *expectation_text;
+	struct kunit_matcher_result *matcher_list;
+	size_t matcher_list_len;
+};
+
+void kunit_mock_failed_match_format(struct kunit_mock_failed_match *match,
+				    struct kunit_stream *stream);
+
+struct kunit_mock_no_match {
+	struct kunit_mock_assert assert;
+	struct kunit_mock_declaration declaration;
+	struct list_head failed_match_list;
+};
+
+void kunit_mock_no_match_format(struct kunit_assert *assert,
+				struct kunit_stream *stream);
+
+#endif /*  _KUNIT_ASSERT_H */
diff --git a/include/kunit/mock.h b/include/kunit/mock.h
index 001b96af62f1e..52c9e427c831b 100644
--- a/include/kunit/mock.h
+++ b/include/kunit/mock.h
@@ -144,6 +144,10 @@ void mock_register_formatter(struct mock_param_formatter *formatter);
 
 void mock_unregister_formatter(struct mock_param_formatter *formatter);
 
+void mock_format_param(struct kunit_stream *stream,
+		       const char *type_name,
+		       const void *param);
+
 struct mock *mock_get_global_mock(void);
 
 #define MOCK(name) name##_mock
diff --git a/kunit/Makefile b/kunit/Makefile
index bbf43fcfb93a9..149d856a30f04 100644
--- a/kunit/Makefile
+++ b/kunit/Makefile
@@ -3,7 +3,8 @@ obj-$(CONFIG_KUNIT) +=			test.o \
 					common-mocks.o \
 					string-stream.o \
 					kunit-stream.o \
-					try-catch.o
+					try-catch.o \
+					assert.o
 
 obj-$(CONFIG_KUNIT_TEST) +=		test-test.o \
 					test-mock.o \
diff --git a/kunit/assert.c b/kunit/assert.c
new file mode 100644
index 0000000000000..75bb6922a994e
--- /dev/null
+++ b/kunit/assert.c
@@ -0,0 +1,179 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Assertion and expectation serialization API.
+ *
+ * Copyright (C) 2019, Google LLC.
+ * Author: Brendan Higgins <brendanhiggins@google.com>
+ */
+#include <kunit/assert.h>
+
+void kunit_base_assert_format(struct kunit_assert *assert,
+			      struct kunit_stream *stream)
+{
+	const char *expect_or_assert;
+
+	if (assert->type == KUNIT_EXPECTATION)
+		expect_or_assert = "EXPECTATION";
+	else
+		expect_or_assert = "ASSERTION";
+
+	kunit_stream_add(stream, "%s FAILED at %s:%s\n",
+			 expect_or_assert, assert->file, assert->line);
+}
+
+void kunit_assert_print_msg(struct kunit_assert *assert,
+			    struct kunit_stream *stream)
+{
+	if (assert->message.fmt)
+		kunit_stream_add(stream, "\n%pV", &assert->message);
+}
+
+void kunit_unary_assert_format(struct kunit_assert *assert,
+			       struct kunit_stream *stream)
+{
+	struct kunit_unary_assert *unary_assert = container_of(
+			assert, struct kunit_unary_assert, assert);
+
+	kunit_base_assert_format(assert, stream);
+	if (unary_assert->expected_true)
+		kunit_stream_add(stream,
+				 "\tExpected %s to be true, but is false\n",
+				 unary_assert->condition);
+	else
+		kunit_stream_add(stream,
+				 "\tExpected %s to be false, but is true\n",
+				 unary_assert->condition);
+	kunit_assert_print_msg(assert, stream);
+}
+
+void kunit_ptr_not_err_assert_format(struct kunit_assert *assert,
+				     struct kunit_stream *stream)
+{
+	struct kunit_ptr_not_err_assert *ptr_assert = container_of(
+			assert, struct kunit_ptr_not_err_assert, assert);
+
+	kunit_base_assert_format(assert, stream);
+	if (!ptr_assert->value) {
+		kunit_stream_add(stream,
+				 "\tExpected %s is not null, but is\n",
+				 ptr_assert->text);
+	} else if (IS_ERR(ptr_assert->value)) {
+		kunit_stream_add(stream,
+				 "\tExpected %s is not error, but is: %ld\n",
+				 ptr_assert->text,
+				 PTR_ERR(ptr_assert->value));
+	}
+	kunit_assert_print_msg(assert, stream);
+}
+
+void kunit_binary_assert_format(struct kunit_assert *assert,
+				struct kunit_stream *stream)
+{
+	struct kunit_binary_assert *binary_assert = container_of(
+			assert, struct kunit_binary_assert, assert);
+
+	kunit_base_assert_format(assert, stream);
+	kunit_stream_add(stream,
+			 "\tExpected %s %s %s, but\n",
+			 binary_assert->left_text,
+			 binary_assert->operation,
+			 binary_assert->right_text);
+	kunit_stream_add(stream, "\t\t%s == %lld\n",
+			 binary_assert->left_text,
+			 binary_assert->left_value);
+	kunit_stream_add(stream, "\t\t%s == %lld",
+			 binary_assert->right_text,
+			 binary_assert->right_value);
+	kunit_assert_print_msg(assert, stream);
+}
+
+void kunit_binary_ptr_assert_format(struct kunit_assert *assert,
+				    struct kunit_stream *stream)
+{
+	struct kunit_binary_ptr_assert *binary_assert = container_of(
+			assert, struct kunit_binary_ptr_assert, assert);
+
+	kunit_base_assert_format(assert, stream);
+	kunit_stream_add(stream,
+			 "\tExpected %s %s %s, but\n",
+			 binary_assert->left_text,
+			 binary_assert->operation,
+			 binary_assert->right_text);
+	kunit_stream_add(stream, "\t\t%s == %pK\n",
+			 binary_assert->left_text,
+			 binary_assert->left_value);
+	kunit_stream_add(stream, "\t\t%s == %pK",
+			 binary_assert->right_text,
+			 binary_assert->right_value);
+	kunit_assert_print_msg(assert, stream);
+}
+
+void kunit_binary_str_assert_format(struct kunit_assert *assert,
+				    struct kunit_stream *stream)
+{
+	struct kunit_binary_str_assert *binary_assert = container_of(
+			assert, struct kunit_binary_str_assert, assert);
+
+	kunit_base_assert_format(assert, stream);
+	kunit_stream_add(stream,
+			 "\tExpected %s %s %s, but\n",
+			 binary_assert->left_text,
+			 binary_assert->operation,
+			 binary_assert->right_text);
+	kunit_stream_add(stream, "\t\t%s == %s\n",
+			 binary_assert->left_text,
+			 binary_assert->left_value);
+	kunit_stream_add(stream, "\t\t%s == %s",
+			 binary_assert->right_text,
+			 binary_assert->right_value);
+	kunit_assert_print_msg(assert, stream);
+}
+
+void kunit_mock_declaration_format(struct kunit_mock_declaration *declaration,
+				   struct kunit_stream *stream)
+{
+	int i;
+
+	kunit_stream_add(stream, "%s(", declaration->function_name);
+	for (i = 0; i < declaration->len; i++) {
+		mock_format_param(stream,
+				  declaration->type_names[i],
+				  declaration->params[i]);
+		if (i < declaration->len - 1)
+			kunit_stream_add(stream, ", ");
+	}
+	kunit_stream_add(stream, ")\n");
+}
+
+void kunit_mock_failed_match_format(struct kunit_mock_failed_match *match,
+				    struct kunit_stream *stream)
+{
+	struct kunit_matcher_result *result;
+	size_t i;
+
+	kunit_stream_add(stream,
+			 "Tried expectation: %s, but\n",
+			 match->expectation_text);
+	for (i = 0; i < match->matcher_list_len; i++) {
+		result = &match->matcher_list[i];
+		kunit_stream_add(stream, "\t");
+		result->assert.format(&result->assert, stream);
+		kunit_stream_add(stream, "\n");
+	}
+}
+
+void kunit_mock_no_match_format(struct kunit_assert *assert,
+				struct kunit_stream *stream)
+{
+	struct kunit_mock_assert *mock_assert = container_of(
+			assert, struct kunit_mock_assert, assert);
+	struct kunit_mock_no_match *no_match = container_of(
+			mock_assert, struct kunit_mock_no_match, assert);
+	struct kunit_mock_failed_match *expectation;
+
+	kunit_base_assert_format(assert, stream);
+	kunit_mock_declaration_format(&no_match->declaration, stream);
+
+	list_for_each_entry(expectation, &no_match->failed_match_list, node)
+		kunit_mock_failed_match_format(expectation, stream);
+}
diff --git a/kunit/mock.c b/kunit/mock.c
index ccb0abe111402..ab441a58a918c 100644
--- a/kunit/mock.c
+++ b/kunit/mock.c
@@ -269,9 +269,9 @@ struct mock_param_formatter *mock_find_formatter(const char *type_name)
 	return NULL;
 }
 
-static void mock_format_param(struct kunit_stream *stream,
-			      const char *type_name,
-			      const void *param)
+void mock_format_param(struct kunit_stream *stream,
+		       const char *type_name,
+		       const void *param)
 {
 	struct mock_param_formatter *formatter;
 
-- 
2.22.0.657.g960e92d24f-goog

^ permalink raw reply related

* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Dmitry Osipenko @ 2019-07-18 23:52 UTC (permalink / raw)
  To: Sowjanya Komatineni
  Cc: Peter De Schrijver, sboyd, Michael Turquette, Joseph Lo,
	thierry.reding, jonathanh, tglx, jason, marc.zyngier,
	linus.walleij, stefan, mark.rutland, pgaikwad, linux-clk,
	linux-gpio, jckuo, talho, linux-tegra, linux-kernel, mperttunen,
	spatra, robh+dt, devicetree
In-Reply-To: <f664f161-9b6b-6446-e2f9-6373e654dfc3@nvidia.com>

В Thu, 18 Jul 2019 16:08:48 -0700
Sowjanya Komatineni <skomatineni@nvidia.com> пишет:

> On 7/18/19 1:36 PM, Sowjanya Komatineni wrote:
> >
> > On 7/18/19 1:26 PM, Dmitry Osipenko wrote:  
> >> 18.07.2019 22:42, Peter De Schrijver пишет:  
> >>> On Thu, Jul 18, 2019 at 02:44:56AM +0300, Dmitry Osipenko wrote:  
> >>>>> dependencies I am referring are dfll_ref, dfll_soc, and DVFS 
> >>>>> peripheral
> >>>>> clocks which need to be restored prior to DFLL reinit.  
> >>>> Okay, but that shouldn't be a problem if clock dependencies are
> >>>> set up properly.
> >>>>  
> >>>>>>> reverse list order during restore might not work as all other 
> >>>>>>> clocks are
> >>>>>>> in proper order no with any ref clocks for plls getting
> >>>>>>> restored prior
> >>>>>>> to their clients  
> >>>>>> Why? The ref clocks should be registered first and be the
> >>>>>> roots for PLLs
> >>>>>> and the rest. If it's not currently the case, then this need
> >>>>>> to be fixed. You need to ensure that each clock is modeled
> >>>>>> properly. If some
> >>>>>> child clock really depends on multiple parents, then the
> >>>>>> parents need to
> >>>>>> in the correct order or CCF need to be taught about such
> >>>>>> multi-dependencies.
> >>>>>>
> >>>>>> If some required feature is missed, then you have to implement
> >>>>>> it properly and for all, that's how things are done in
> >>>>>> upstream. Sometimes
> >>>>>> it's quite a lot of extra work that everyone are benefiting
> >>>>>> from in the end.
> >>>>>>
> >>>>>> [snip]  
> >>>>> Yes, we should register ref/parents before their clients.
> >>>>>
> >>>>> cclk_g clk is registered last after all pll and peripheral
> >>>>> clocks are registers during clock init.
> >>>>>
> >>>>> dfllCPU_out clk is registered later during dfll-fcpu driver
> >>>>> probe and gets added to the clock list.
> >>>>>
> >>>>> Probably the issue seems to be not linking dfll_ref and dfll_soc
> >>>>> dependencies for dfllCPU_out thru clock list.
> >>>>>
> >>>>> clk-dfll driver during dfll_init_clks gets ref_clk and soc_clk 
> >>>>> reference
> >>>>> thru DT.  
> >>> The dfll does not have any parents. It has some clocks which are
> >>> needed for the logic part of the dfll to function, but there's no
> >>> parent clock as such unlike for peripheral clocks or PLLs where
> >>> the parent is at least used as a reference. The I2C controller of
> >>> the DFLL shares the lines with a normal I2C controller using some
> >>> arbitration logic. That logic only works if the clock for the
> >>> normal I2C controller is enabled. So you need probably 3 clocks
> >>> enabled to initialize the dfll in that case. I don't think it
> >>> makes sense to add complicated logic to the clock
> >>> core to deal with this rather strange case. To me it makes more 
> >>> sense to
> >>> use pmops and open code the sequence there.  
> >> It looks to me that dfllCPU is a PLL and dfll_ref is its reference
> >> parent, while dfll_soc clocks the logic that dynamically
> >> reconfigures dfllCPU in background. I see that PLLP is defined as
> >> a parent for dfll_ref and dfll_soc in the code. Hence seems
> >> dfll_ref should be set as a parent for dfllCPU, no?  
> >
> > dfll_soc will not be restored by the time dfllCPU resume happens
> > after dfll_ref.
> >
> > without dfll_soc, dfllCPU cannot be resumed either. So if we decide
> > to use parent we should use dfll_soc.
> >  
> >> Either way is good to me, given that DFLL will be disabled during
> >> suspend. Resetting DFLL on DFLL's driver resume using PM ops
> >> should be good. And then it also will be better to error out if
> >> DFLL is active during suspend on the DFLL's driver suspend.  
> >
> > Doing in dfll-fcpu pm_ops is much better as it happens right after
> > all clocks are restored and unlike other clock enables, dfll need
> > dfll controller programming as well and is actually registered in
> > dfll-fcpu driver.
> >
> > With this, below is the sequence:
> >
> > CPUFreq suspend switches CPU to PLLP and disables dfll
> >
> > Will add dfll_suspend/resume in dfll-fcpu driver and in dfll
> > suspend will check for dfll active and will error out suspend.
> >
> > dfll resume does dfll reinit.
> >
> > CPUFreq resume enables dfll and switches CPU to dfll.
> >
> >
> > Will go with doing in dfll-fcpu pm_ops rather than parenting 
> > dfllCPU_OUT...
> >  
> Does is make sense to return error EBUSY if dfll is not disabled by
> the time dfll-fcpu suspend happens?

Yes

> Or should I use ETIMEOUT?

No

^ permalink raw reply

* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Sowjanya Komatineni @ 2019-07-18 23:08 UTC (permalink / raw)
  To: Dmitry Osipenko, Peter De Schrijver
  Cc: sboyd, Michael Turquette, Joseph Lo, thierry.reding, jonathanh,
	tglx, jason, marc.zyngier, linus.walleij, stefan, mark.rutland,
	pgaikwad, linux-clk, linux-gpio, jckuo, talho, linux-tegra,
	linux-kernel, mperttunen, spatra, robh+dt, devicetree
In-Reply-To: <1c85cb35-ce7c-1dd1-f637-0c91b2b36db3@nvidia.com>


On 7/18/19 1:36 PM, Sowjanya Komatineni wrote:
>
> On 7/18/19 1:26 PM, Dmitry Osipenko wrote:
>> 18.07.2019 22:42, Peter De Schrijver пишет:
>>> On Thu, Jul 18, 2019 at 02:44:56AM +0300, Dmitry Osipenko wrote:
>>>>> dependencies I am referring are dfll_ref, dfll_soc, and DVFS 
>>>>> peripheral
>>>>> clocks which need to be restored prior to DFLL reinit.
>>>> Okay, but that shouldn't be a problem if clock dependencies are set up
>>>> properly.
>>>>
>>>>>>> reverse list order during restore might not work as all other 
>>>>>>> clocks are
>>>>>>> in proper order no with any ref clocks for plls getting restored 
>>>>>>> prior
>>>>>>> to their clients
>>>>>> Why? The ref clocks should be registered first and be the roots 
>>>>>> for PLLs
>>>>>> and the rest. If it's not currently the case, then this need to be
>>>>>> fixed. You need to ensure that each clock is modeled properly. If 
>>>>>> some
>>>>>> child clock really depends on multiple parents, then the parents 
>>>>>> need to
>>>>>> in the correct order or CCF need to be taught about such
>>>>>> multi-dependencies.
>>>>>>
>>>>>> If some required feature is missed, then you have to implement it
>>>>>> properly and for all, that's how things are done in upstream. 
>>>>>> Sometimes
>>>>>> it's quite a lot of extra work that everyone are benefiting from in
>>>>>> the end.
>>>>>>
>>>>>> [snip]
>>>>> Yes, we should register ref/parents before their clients.
>>>>>
>>>>> cclk_g clk is registered last after all pll and peripheral clocks are
>>>>> registers during clock init.
>>>>>
>>>>> dfllCPU_out clk is registered later during dfll-fcpu driver probe and
>>>>> gets added to the clock list.
>>>>>
>>>>> Probably the issue seems to be not linking dfll_ref and dfll_soc
>>>>> dependencies for dfllCPU_out thru clock list.
>>>>>
>>>>> clk-dfll driver during dfll_init_clks gets ref_clk and soc_clk 
>>>>> reference
>>>>> thru DT.
>>> The dfll does not have any parents. It has some clocks which are needed
>>> for the logic part of the dfll to function, but there's no parent clock
>>> as such unlike for peripheral clocks or PLLs where the parent is at
>>> least used as a reference. The I2C controller of the DFLL shares the
>>> lines with a normal I2C controller using some arbitration logic. That
>>> logic only works if the clock for the normal I2C controller is enabled.
>>> So you need probably 3 clocks enabled to initialize the dfll in that
>>> case. I don't think it makes sense to add complicated logic to the 
>>> clock
>>> core to deal with this rather strange case. To me it makes more 
>>> sense to
>>> use pmops and open code the sequence there.
>> It looks to me that dfllCPU is a PLL and dfll_ref is its reference
>> parent, while dfll_soc clocks the logic that dynamically reconfigures
>> dfllCPU in background. I see that PLLP is defined as a parent for
>> dfll_ref and dfll_soc in the code. Hence seems dfll_ref should be set as
>> a parent for dfllCPU, no?
>
> dfll_soc will not be restored by the time dfllCPU resume happens after 
> dfll_ref.
>
> without dfll_soc, dfllCPU cannot be resumed either. So if we decide to 
> use parent we should use dfll_soc.
>
>> Either way is good to me, given that DFLL will be disabled during
>> suspend. Resetting DFLL on DFLL's driver resume using PM ops should be
>> good. And then it also will be better to error out if DFLL is active
>> during suspend on the DFLL's driver suspend.
>
> Doing in dfll-fcpu pm_ops is much better as it happens right after all 
> clocks are restored and unlike other clock enables, dfll need dfll 
> controller programming as well and is actually registered in dfll-fcpu 
> driver.
>
> With this, below is the sequence:
>
> CPUFreq suspend switches CPU to PLLP and disables dfll
>
> Will add dfll_suspend/resume in dfll-fcpu driver and in dfll suspend 
> will check for dfll active and will error out suspend.
>
> dfll resume does dfll reinit.
>
> CPUFreq resume enables dfll and switches CPU to dfll.
>
>
> Will go with doing in dfll-fcpu pm_ops rather than parenting 
> dfllCPU_OUT...
>
Does is make sense to return error EBUSY if dfll is not disabled by the 
time dfll-fcpu suspend happens?

Or should I use ETIMEOUT?

^ permalink raw reply

* Re: [PATCH v3 1/2] arm64: dts: fsl: pico-pi: Add a device tree for the PICO-PI-IMX8M
From: Fabio Estevam @ 2019-07-18 23:05 UTC (permalink / raw)
  To: Andra Danciu
  Cc: Shawn Guo, Rob Herring, Mark Rutland, Manivannan Sadhasivam,
	Andrey Smirnov, Uwe Kleine-König, Dong Aisheng, Li Yang,
	sriram.dash, Lucas Stach, pankaj.bansal, Ping Bai, Pramod Kumar,
	Bhaskar Upadhaya, Richard Hu,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	linux-kernel
In-Reply-To: <20190718121628.23991-2-andradanciu1997@gmail.com>

Hi Andra,

On Thu, Jul 18, 2019 at 9:16 AM Andra Danciu <andradanciu1997@gmail.com> wrote:
>
> From: Richard Hu <richard.hu@technexion.com>

Please put a few words about the board and a link to its webpage, if available.

> The current level of support yields a working console and is able to boot
> userspace from NFS or init ramdisk.
>
> Additional subsystems that are active :
>         - Ethernet
>         - USB
>
> Cc: Daniel Baluta <daniel.baluta@nxp.com>
> Signed-off-by: Richard Hu <richard.hu@technexion.com>
> Signed-off-by: Andra Danciu <andradanciu1997@gmail.com>
> ---
>  arch/arm64/boot/dts/freescale/Makefile       |   1 +
>  arch/arm64/boot/dts/freescale/pico-pi-8m.dts | 417 +++++++++++++++++++++++++++
>  2 files changed, 418 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/freescale/pico-pi-8m.dts
>
> diff --git a/arch/arm64/boot/dts/freescale/Makefile b/arch/arm64/boot/dts/freescale/Makefile
> index c043aca66572..538422903e8a 100644
> --- a/arch/arm64/boot/dts/freescale/Makefile
> +++ b/arch/arm64/boot/dts/freescale/Makefile
> @@ -26,3 +26,4 @@ dtb-$(CONFIG_ARCH_MXC) += imx8mq-librem5-devkit.dtb
>  dtb-$(CONFIG_ARCH_MXC) += imx8mq-zii-ultra-rmb3.dtb
>  dtb-$(CONFIG_ARCH_MXC) += imx8mq-zii-ultra-zest.dtb
>  dtb-$(CONFIG_ARCH_MXC) += imx8qxp-mek.dtb
> +dtb-$(CONFIG_ARCH_MXC) += pico-pi-8m.dtb

The convention we use with imx dtbs is to put the SoC name first, so
that would become:

imx8mq-pico-pi.dtb


> +&iomuxc {

Please place iomuxc node as the last one.

> +&fec1 {
> +       pinctrl-names = "default";
> +       pinctrl-0 = <&pinctrl_fec1 &pinctrl_enet_3v3>;
> +       phy-mode = "rgmii-id";
> +       pinctrl-assert-gpios = <&gpio1 0 GPIO_ACTIVE_HIGH>;

This property does not exist.

> +       phy-handle = <&ethphy0>;
> +       fsl,magic-packet;
> +       status = "okay";
> +
> +       mdio {
> +               #address-cells = <1>;
> +               #size-cells = <0>;
> +
> +               ethphy0: ethernet-phy@1 {
> +                       compatible = "ethernet-phy-ieee802.3-c22";
> +                       reg = <1>;
> +                       at803x,led-act-blind-workaround;
> +                       at803x,eee-disabled;

These two properties do not exist.

> +&i2c1 {
> +       clock-frequency = <100000>;
> +       pinctrl-names = "default";
> +       pinctrl-0 = <&pinctrl_i2c1>;
> +       status = "okay";
> +
> +       pmic: pmic@4b {
> +               reg = <0x4b>;
> +               compatible = "rohm,bd71837";
> +               /* PMIC BD71837 PMIC_nINT GPIO1_IO12 */
> +               pinctrl-0 = <&pinctrl_pmic>;

pinctrl-names = "default" is missing

> +               gpio_intr = <&gpio1 3 GPIO_ACTIVE_LOW>;

This is not documented.

Please look at Documentation/devicetree/bindings/regulator/rohm,bd71837-regulator.txt
for the valid bindings and also at
arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts for a
reference for adding the BD71837 support.

> +&A53_0 {
> +       operating-points = <
> +               /* kHz    uV */
> +               1500000 1000000
> +               1300000 1000000
> +               1000000 900000
> +               800000  900000

This is not needed as these operating points are already specified at
imx8mq.dtsi.

^ permalink raw reply

* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Dmitry Osipenko @ 2019-07-18 22:52 UTC (permalink / raw)
  To: Sowjanya Komatineni
  Cc: Peter De Schrijver, sboyd, Michael Turquette, Joseph Lo,
	thierry.reding, jonathanh, tglx, jason, marc.zyngier,
	linus.walleij, stefan, mark.rutland, pgaikwad, linux-clk,
	linux-gpio, jckuo, talho, linux-tegra, linux-kernel, mperttunen,
	spatra, robh+dt, devicetree
In-Reply-To: <1c85cb35-ce7c-1dd1-f637-0c91b2b36db3@nvidia.com>

В Thu, 18 Jul 2019 13:36:35 -0700
Sowjanya Komatineni <skomatineni@nvidia.com> пишет:

> On 7/18/19 1:26 PM, Dmitry Osipenko wrote:
> > 18.07.2019 22:42, Peter De Schrijver пишет:  
> >> On Thu, Jul 18, 2019 at 02:44:56AM +0300, Dmitry Osipenko wrote:  
> >>>> dependencies I am referring are dfll_ref, dfll_soc, and DVFS
> >>>> peripheral clocks which need to be restored prior to DFLL
> >>>> reinit.  
> >>> Okay, but that shouldn't be a problem if clock dependencies are
> >>> set up properly.
> >>>  
> >>>>>> reverse list order during restore might not work as all other
> >>>>>> clocks are in proper order no with any ref clocks for plls
> >>>>>> getting restored prior to their clients  
> >>>>> Why? The ref clocks should be registered first and be the roots
> >>>>> for PLLs and the rest. If it's not currently the case, then
> >>>>> this need to be fixed. You need to ensure that each clock is
> >>>>> modeled properly. If some child clock really depends on
> >>>>> multiple parents, then the parents need to in the correct order
> >>>>> or CCF need to be taught about such multi-dependencies.
> >>>>>
> >>>>> If some required feature is missed, then you have to implement
> >>>>> it properly and for all, that's how things are done in
> >>>>> upstream. Sometimes it's quite a lot of extra work that
> >>>>> everyone are benefiting from in the end.
> >>>>>
> >>>>> [snip]  
> >>>> Yes, we should register ref/parents before their clients.
> >>>>
> >>>> cclk_g clk is registered last after all pll and peripheral
> >>>> clocks are registers during clock init.
> >>>>
> >>>> dfllCPU_out clk is registered later during dfll-fcpu driver
> >>>> probe and gets added to the clock list.
> >>>>
> >>>> Probably the issue seems to be not linking dfll_ref and dfll_soc
> >>>> dependencies for dfllCPU_out thru clock list.
> >>>>
> >>>> clk-dfll driver during dfll_init_clks gets ref_clk and soc_clk
> >>>> reference thru DT.  
> >> The dfll does not have any parents. It has some clocks which are
> >> needed for the logic part of the dfll to function, but there's no
> >> parent clock as such unlike for peripheral clocks or PLLs where
> >> the parent is at least used as a reference. The I2C controller of
> >> the DFLL shares the lines with a normal I2C controller using some
> >> arbitration logic. That logic only works if the clock for the
> >> normal I2C controller is enabled. So you need probably 3 clocks
> >> enabled to initialize the dfll in that case. I don't think it
> >> makes sense to add complicated logic to the clock core to deal
> >> with this rather strange case. To me it makes more sense to use
> >> pmops and open code the sequence there.  
> > It looks to me that dfllCPU is a PLL and dfll_ref is its reference
> > parent, while dfll_soc clocks the logic that dynamically
> > reconfigures dfllCPU in background. I see that PLLP is defined as a
> > parent for dfll_ref and dfll_soc in the code. Hence seems dfll_ref
> > should be set as a parent for dfllCPU, no?  
> 
> dfll_soc will not be restored by the time dfllCPU resume happens
> after dfll_ref.
> 
> without dfll_soc, dfllCPU cannot be resumed either. So if we decide
> to use parent we should use dfll_soc.

Okay, my point is that the parents should be properly specified any
ways.

> > Either way is good to me, given that DFLL will be disabled during
> > suspend. Resetting DFLL on DFLL's driver resume using PM ops should
> > be good. And then it also will be better to error out if DFLL is
> > active during suspend on the DFLL's driver suspend.  
> 
> Doing in dfll-fcpu pm_ops is much better as it happens right after
> all clocks are restored and unlike other clock enables, dfll need
> dfll controller programming as well and is actually registered in
> dfll-fcpu driver.
> 
> With this, below is the sequence:
> 
> CPUFreq suspend switches CPU to PLLP and disables dfll
> 
> Will add dfll_suspend/resume in dfll-fcpu driver and in dfll suspend 
> will check for dfll active and will error out suspend.
> 
> dfll resume does dfll reinit.
> 
> CPUFreq resume enables dfll and switches CPU to dfll.
> 
> 
> Will go with doing in dfll-fcpu pm_ops rather than parenting
> dfllCPU_OUT...
> 

Sounds good.

^ permalink raw reply

* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Sowjanya Komatineni @ 2019-07-18 20:36 UTC (permalink / raw)
  To: Dmitry Osipenko, Peter De Schrijver
  Cc: sboyd, Michael Turquette, Joseph Lo, thierry.reding, jonathanh,
	tglx, jason, marc.zyngier, linus.walleij, stefan, mark.rutland,
	pgaikwad, linux-clk, linux-gpio, jckuo, talho, linux-tegra,
	linux-kernel, mperttunen, spatra, robh+dt, devicetree
In-Reply-To: <056496ed-9abf-6907-c61c-a99ccf23b834@gmail.com>


On 7/18/19 1:26 PM, Dmitry Osipenko wrote:
> 18.07.2019 22:42, Peter De Schrijver пишет:
>> On Thu, Jul 18, 2019 at 02:44:56AM +0300, Dmitry Osipenko wrote:
>>>> dependencies I am referring are dfll_ref, dfll_soc, and DVFS peripheral
>>>> clocks which need to be restored prior to DFLL reinit.
>>> Okay, but that shouldn't be a problem if clock dependencies are set up
>>> properly.
>>>
>>>>>> reverse list order during restore might not work as all other clocks are
>>>>>> in proper order no with any ref clocks for plls getting restored prior
>>>>>> to their clients
>>>>> Why? The ref clocks should be registered first and be the roots for PLLs
>>>>> and the rest. If it's not currently the case, then this need to be
>>>>> fixed. You need to ensure that each clock is modeled properly. If some
>>>>> child clock really depends on multiple parents, then the parents need to
>>>>> in the correct order or CCF need to be taught about such
>>>>> multi-dependencies.
>>>>>
>>>>> If some required feature is missed, then you have to implement it
>>>>> properly and for all, that's how things are done in upstream. Sometimes
>>>>> it's quite a lot of extra work that everyone are benefiting from in
>>>>> the end.
>>>>>
>>>>> [snip]
>>>> Yes, we should register ref/parents before their clients.
>>>>
>>>> cclk_g clk is registered last after all pll and peripheral clocks are
>>>> registers during clock init.
>>>>
>>>> dfllCPU_out clk is registered later during dfll-fcpu driver probe and
>>>> gets added to the clock list.
>>>>
>>>> Probably the issue seems to be not linking dfll_ref and dfll_soc
>>>> dependencies for dfllCPU_out thru clock list.
>>>>
>>>> clk-dfll driver during dfll_init_clks gets ref_clk and soc_clk reference
>>>> thru DT.
>> The dfll does not have any parents. It has some clocks which are needed
>> for the logic part of the dfll to function, but there's no parent clock
>> as such unlike for peripheral clocks or PLLs where the parent is at
>> least used as a reference. The I2C controller of the DFLL shares the
>> lines with a normal I2C controller using some arbitration logic. That
>> logic only works if the clock for the normal I2C controller is enabled.
>> So you need probably 3 clocks enabled to initialize the dfll in that
>> case. I don't think it makes sense to add complicated logic to the clock
>> core to deal with this rather strange case. To me it makes more sense to
>> use pmops and open code the sequence there.
> It looks to me that dfllCPU is a PLL and dfll_ref is its reference
> parent, while dfll_soc clocks the logic that dynamically reconfigures
> dfllCPU in background. I see that PLLP is defined as a parent for
> dfll_ref and dfll_soc in the code. Hence seems dfll_ref should be set as
> a parent for dfllCPU, no?

dfll_soc will not be restored by the time dfllCPU resume happens after 
dfll_ref.

without dfll_soc, dfllCPU cannot be resumed either. So if we decide to 
use parent we should use dfll_soc.

> Either way is good to me, given that DFLL will be disabled during
> suspend. Resetting DFLL on DFLL's driver resume using PM ops should be
> good. And then it also will be better to error out if DFLL is active
> during suspend on the DFLL's driver suspend.

Doing in dfll-fcpu pm_ops is much better as it happens right after all 
clocks are restored and unlike other clock enables, dfll need dfll 
controller programming as well and is actually registered in dfll-fcpu 
driver.

With this, below is the sequence:

CPUFreq suspend switches CPU to PLLP and disables dfll

Will add dfll_suspend/resume in dfll-fcpu driver and in dfll suspend 
will check for dfll active and will error out suspend.

dfll resume does dfll reinit.

CPUFreq resume enables dfll and switches CPU to dfll.


Will go with doing in dfll-fcpu pm_ops rather than parenting dfllCPU_OUT...

^ permalink raw reply

* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Dmitry Osipenko @ 2019-07-18 20:32 UTC (permalink / raw)
  To: Sowjanya Komatineni, Peter De Schrijver
  Cc: Joseph Lo, thierry.reding, jonathanh, tglx, jason, marc.zyngier,
	linus.walleij, stefan, mark.rutland, pgaikwad, sboyd, linux-clk,
	linux-gpio, jckuo, talho, linux-tegra, linux-kernel, mperttunen,
	spatra, robh+dt, devicetree
In-Reply-To: <5c6e17ed-bdbd-fa27-87e7-aa95dfa7c591@gmail.com>

18.07.2019 23:11, Dmitry Osipenko пишет:
> 18.07.2019 22:24, Sowjanya Komatineni пишет:
>>
>> On 7/18/19 12:18 PM, Peter De Schrijver wrote:
>>> On Tue, Jul 16, 2019 at 09:43:16PM +0300, Dmitry Osipenko wrote:
>>>>> CPU parents are PLL_X, PLL_P, and dfll. PLL_X always runs at higher
>>>>> rate
>>>>> so switching to PLL_P during CPUFreq probe prior to dfll clock enable
>>>>> should be safe.
>>>> AFAIK, PLLX could run at ~200MHz. There is also a divided output of PLLP
>>>> which CCLKG supports, the PLLP_OUT4.
>>>>
>>>> Probably, realistically, CPU is always running off a fast PLLX during
>>>> boot, but I'm wondering what may happen on KEXEC. I guess ideally
>>>> CPUFreq driver should also have a 'shutdown' callback to teardown DFLL
>>>> on a reboot, but likely that there are other clock-related problems as
>>>> well that may break KEXEC and thus it is not very important at the
>>>> moment.
>>>>
>>> If you turn off the DFLL, you have to be aware that the voltage margins
>>> for DFLL use are lower than for PLL use. So you either need to be sure
>>> to switch to a frequency below fmax @ Vmin or you program the boot
>>> voltage and then you can use PLLX as setup by the bootloader. For OVR
>>> regulators you can't program a voltage without the DFLL, so you have to
>>> tristate the PWM output which will give you a hardwired boot voltage.
>>>
>>> Peter.
>>
>> Yes, we switch CPU to PLLP and then disable DFLL during suspend.
> 
> I'm wondering what happens to T124 on resume from suspend, given that it
> switches CPU to PLLX [1]. I imagine that CPU voltage could be lower than
> needed if suspend happened on DFLL. I'm also now vaguely recalling that
> CPUFreq driver was disabled for T124 because of some problems.
> 
> Or maybe warmboot code is actually touching the voltage regulators?

Oh wait, warmboot is even irrelevant LP1.

> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/arch/arm/mach-tegra/sleep-tegra30.S#n389
> 
> That is also should be a problem for T30 if voltage scaling is happening
> and I have some patches in works that are switching CPU to PLLP instead
> of PLLX on suspend/resume.
> 

^ permalink raw reply

* Re: [PATCH V5 11/18] clk: tegra210: Add support for Tegra210 clocks
From: Dmitry Osipenko @ 2019-07-18 20:26 UTC (permalink / raw)
  To: Peter De Schrijver
  Cc: Sowjanya Komatineni, sboyd, Michael Turquette, Joseph Lo,
	thierry.reding, jonathanh, tglx, jason, marc.zyngier,
	linus.walleij, stefan, mark.rutland, pgaikwad, linux-clk,
	linux-gpio, jckuo, talho, linux-tegra, linux-kernel, mperttunen,
	spatra, robh+dt, devicetree
In-Reply-To: <20190718194222.GH12715@pdeschrijver-desktop.Nvidia.com>

18.07.2019 22:42, Peter De Schrijver пишет:
> On Thu, Jul 18, 2019 at 02:44:56AM +0300, Dmitry Osipenko wrote:
>>>
>>> dependencies I am referring are dfll_ref, dfll_soc, and DVFS peripheral
>>> clocks which need to be restored prior to DFLL reinit.
>>
>> Okay, but that shouldn't be a problem if clock dependencies are set up
>> properly.
>>
>>>>> reverse list order during restore might not work as all other clocks are
>>>>> in proper order no with any ref clocks for plls getting restored prior
>>>>> to their clients
>>>> Why? The ref clocks should be registered first and be the roots for PLLs
>>>> and the rest. If it's not currently the case, then this need to be
>>>> fixed. You need to ensure that each clock is modeled properly. If some
>>>> child clock really depends on multiple parents, then the parents need to
>>>> in the correct order or CCF need to be taught about such
>>>> multi-dependencies.
>>>>
>>>> If some required feature is missed, then you have to implement it
>>>> properly and for all, that's how things are done in upstream. Sometimes
>>>> it's quite a lot of extra work that everyone are benefiting from in
>>>> the end.
>>>>
>>>> [snip]
>>>
>>> Yes, we should register ref/parents before their clients.
>>>
>>> cclk_g clk is registered last after all pll and peripheral clocks are
>>> registers during clock init.
>>>
>>> dfllCPU_out clk is registered later during dfll-fcpu driver probe and
>>> gets added to the clock list.
>>>
>>> Probably the issue seems to be not linking dfll_ref and dfll_soc
>>> dependencies for dfllCPU_out thru clock list.
>>>
>>> clk-dfll driver during dfll_init_clks gets ref_clk and soc_clk reference
>>> thru DT.
> 
> The dfll does not have any parents. It has some clocks which are needed
> for the logic part of the dfll to function, but there's no parent clock
> as such unlike for peripheral clocks or PLLs where the parent is at
> least used as a reference. The I2C controller of the DFLL shares the
> lines with a normal I2C controller using some arbitration logic. That
> logic only works if the clock for the normal I2C controller is enabled.
> So you need probably 3 clocks enabled to initialize the dfll in that
> case. I don't think it makes sense to add complicated logic to the clock
> core to deal with this rather strange case. To me it makes more sense to
> use pmops and open code the sequence there.

It looks to me that dfllCPU is a PLL and dfll_ref is its reference
parent, while dfll_soc clocks the logic that dynamically reconfigures
dfllCPU in background. I see that PLLP is defined as a parent for
dfll_ref and dfll_soc in the code. Hence seems dfll_ref should be set as
a parent for dfllCPU, no?

Either way is good to me, given that DFLL will be disabled during
suspend. Resetting DFLL on DFLL's driver resume using PM ops should be
good. And then it also will be better to error out if DFLL is active
during suspend on the DFLL's driver suspend.

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox