Devicetree
 help / color / mirror / Atom feed
* Re: [PATCH v5 2/4] drm/bridge: add lvds controller support for sam9x7
From: Hari.PrasathGE @ 2024-04-07  6:28 UTC (permalink / raw)
  To: Dharma.B, andrzej.hajda, neil.armstrong, rfoss, Laurent.pinchart,
	jonas, jernej.skrabec, maarten.lankhorst, mripard, tzimmermann,
	airlied, daniel, robh+dt, krzysztof.kozlowski+dt, conor+dt, linux,
	Nicolas.Ferre, alexandre.belloni, claudiu.beznea, Manikandan.M,
	arnd, geert+renesas, Jason, mpe, gerg, rdunlap, vbabka, dri-devel,
	devicetree, linux-kernel, linux-arm-kernel, akpm, deller
In-Reply-To: <20240405043536.274220-3-dharma.b@microchip.com>

On 4/5/24 10:05 AM, Dharma Balasubiramani wrote:
> Add a new LVDS controller driver for sam9x7 which does the following:
> - Prepares and enables the LVDS Peripheral clock
> - Defines its connector type as DRM_MODE_CONNECTOR_LVDS and adds itself
> to the global bridge list.
> - Identifies its output endpoint as panel and adds it to the encoder
> display pipeline
> - Enables the LVDS serializer


Acked-by: Hari Prasath Gujulan Elango <hari.prasathge@microchip.com>

> 
> Signed-off-by: Manikandan Muralidharan <manikandan.m@microchip.com>
> Signed-off-by: Dharma Balasubiramani <dharma.b@microchip.com>
> ---
> Changelog
> v4 -> v5
> - Drop the unused variable 'format'.
> - Use DRM wrapper for dev_err() to maintain uniformity.
> - return -ENODEV instead of -EINVAL to maintain consistency with other DRM
>    bridge drivers.
> v3 -> v4
> - No changes.
> v2 ->v3
> - Correct Typo error "serializer".
> - Consolidate get() and prepare() functions and use devm_clk_get_prepared().
> - Remove unused variable 'ret' in probe().
> - Use devm_pm_runtime_enable() and drop the mchp_lvds_remove().
> v1 -> v2
> - Drop 'res' variable and combine two lines into one.
> - Handle deferred probe properly, use dev_err_probe().
> - Don't print anything on deferred probe. Dropped print.
> - Remove the MODULE_ALIAS and add MODULE_DEVICE_TABLE().
> - symbol 'mchp_lvds_driver' was not declared. It should be static.
> ---
>   drivers/gpu/drm/bridge/Kconfig          |   7 +
>   drivers/gpu/drm/bridge/Makefile         |   1 +
>   drivers/gpu/drm/bridge/microchip-lvds.c | 228 ++++++++++++++++++++++++
>   3 files changed, 236 insertions(+)
>   create mode 100644 drivers/gpu/drm/bridge/microchip-lvds.c
> 
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index efd996f6c138..889098e2d65f 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -190,6 +190,13 @@ config DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW
>   	  to DP++. This is used with the i.MX6 imx-ldb
>   	  driver. You are likely to say N here.
>   
> +config DRM_MICROCHIP_LVDS_SERIALIZER
> +	tristate "Microchip LVDS serializer support"
> +	depends on OF
> +	depends on DRM_ATMEL_HLCDC
> +	help
> +	  Support for Microchip's LVDS serializer.
> +
>   config DRM_NWL_MIPI_DSI
>   	tristate "Northwest Logic MIPI DSI Host controller"
>   	depends on DRM
> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> index 017b5832733b..7df87b582dca 100644
> --- a/drivers/gpu/drm/bridge/Makefile
> +++ b/drivers/gpu/drm/bridge/Makefile
> @@ -13,6 +13,7 @@ obj-$(CONFIG_DRM_LONTIUM_LT9611) += lontium-lt9611.o
>   obj-$(CONFIG_DRM_LONTIUM_LT9611UXC) += lontium-lt9611uxc.o
>   obj-$(CONFIG_DRM_LVDS_CODEC) += lvds-codec.o
>   obj-$(CONFIG_DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW) += megachips-stdpxxxx-ge-b850v3-fw.o
> +obj-$(CONFIG_DRM_MICROCHIP_LVDS_SERIALIZER) += microchip-lvds.o
>   obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o
>   obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
>   obj-$(CONFIG_DRM_PARADE_PS8640) += parade-ps8640.o
> diff --git a/drivers/gpu/drm/bridge/microchip-lvds.c b/drivers/gpu/drm/bridge/microchip-lvds.c
> new file mode 100644
> index 000000000000..149704f498a6
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/microchip-lvds.c
> @@ -0,0 +1,228 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2023 Microchip Technology Inc. and its subsidiaries
> + *
> + * Author: Manikandan Muralidharan <manikandan.m@microchip.com>
> + * Author: Dharma Balasubiramani <dharma.b@microchip.com>
> + *
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/component.h>
> +#include <linux/delay.h>
> +#include <linux/jiffies.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/of_graph.h>
> +#include <linux/pinctrl/devinfo.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +#include <linux/reset.h>
> +
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_bridge.h>
> +#include <drm/drm_of.h>
> +#include <drm/drm_panel.h>
> +#include <drm/drm_print.h>
> +#include <drm/drm_probe_helper.h>
> +#include <drm/drm_simple_kms_helper.h>
> +
> +#define LVDS_POLL_TIMEOUT_MS 1000
> +
> +/* LVDSC register offsets */
> +#define LVDSC_CR	0x00
> +#define LVDSC_CFGR	0x04
> +#define LVDSC_SR	0x0C
> +#define LVDSC_WPMR	0xE4
> +
> +/* Bitfields in LVDSC_CR (Control Register) */
> +#define LVDSC_CR_SER_EN	BIT(0)
> +
> +/* Bitfields in LVDSC_CFGR (Configuration Register) */
> +#define LVDSC_CFGR_PIXSIZE_24BITS	0
> +#define LVDSC_CFGR_DEN_POL_HIGH		0
> +#define LVDSC_CFGR_DC_UNBALANCED	0
> +#define LVDSC_CFGR_MAPPING_JEIDA	BIT(6)
> +
> +/*Bitfields in LVDSC_SR */
> +#define LVDSC_SR_CS	BIT(0)
> +
> +/* Bitfields in LVDSC_WPMR (Write Protection Mode Register) */
> +#define LVDSC_WPMR_WPKEY_MASK	GENMASK(31, 8)
> +#define LVDSC_WPMR_WPKEY_PSSWD	0x4C5644
> +
> +struct mchp_lvds {
> +	struct device *dev;
> +	void __iomem *regs;
> +	struct clk *pclk;
> +	struct drm_panel *panel;
> +	struct drm_bridge bridge;
> +	struct drm_bridge *panel_bridge;
> +};
> +
> +static inline struct mchp_lvds *bridge_to_lvds(struct drm_bridge *bridge)
> +{
> +	return container_of(bridge, struct mchp_lvds, bridge);
> +}
> +
> +static inline u32 lvds_readl(struct mchp_lvds *lvds, u32 offset)
> +{
> +	return readl_relaxed(lvds->regs + offset);
> +}
> +
> +static inline void lvds_writel(struct mchp_lvds *lvds, u32 offset, u32 val)
> +{
> +	writel_relaxed(val, lvds->regs + offset);
> +}
> +
> +static void lvds_serialiser_on(struct mchp_lvds *lvds)
> +{
> +	unsigned long timeout = jiffies + msecs_to_jiffies(LVDS_POLL_TIMEOUT_MS);
> +
> +	/* The LVDSC registers can only be written if WPEN is cleared */
> +	lvds_writel(lvds, LVDSC_WPMR, (LVDSC_WPMR_WPKEY_PSSWD &
> +				LVDSC_WPMR_WPKEY_MASK));
> +
> +	/* Wait for the status of configuration registers to be changed */
> +	while (lvds_readl(lvds, LVDSC_SR) & LVDSC_SR_CS) {
> +		if (time_after(jiffies, timeout)) {
> +			DRM_DEV_ERROR(lvds->dev, "%s: timeout error\n",
> +				      __func__);
> +			return;
> +		}
> +		usleep_range(1000, 2000);
> +	}
> +
> +	/* Configure the LVDSC */
> +	lvds_writel(lvds, LVDSC_CFGR, (LVDSC_CFGR_MAPPING_JEIDA |
> +				LVDSC_CFGR_DC_UNBALANCED |
> +				LVDSC_CFGR_DEN_POL_HIGH |
> +				LVDSC_CFGR_PIXSIZE_24BITS));
> +
> +	/* Enable the LVDS serializer */
> +	lvds_writel(lvds, LVDSC_CR, LVDSC_CR_SER_EN);
> +}
> +
> +static int mchp_lvds_attach(struct drm_bridge *bridge,
> +			    enum drm_bridge_attach_flags flags)
> +{
> +	struct mchp_lvds *lvds = bridge_to_lvds(bridge);
> +
> +	bridge->encoder->encoder_type = DRM_MODE_ENCODER_LVDS;
> +
> +	return drm_bridge_attach(bridge->encoder, lvds->panel_bridge,
> +				 bridge, flags);
> +}
> +
> +static void mchp_lvds_enable(struct drm_bridge *bridge)
> +{
> +	struct mchp_lvds *lvds = bridge_to_lvds(bridge);
> +	int ret;
> +
> +	ret = clk_enable(lvds->pclk);
> +	if (ret < 0) {
> +		DRM_DEV_ERROR(lvds->dev, "failed to enable lvds pclk %d\n", ret);
> +		return;
> +	}
> +
> +	ret = pm_runtime_get_sync(lvds->dev);
> +	if (ret < 0) {
> +		DRM_DEV_ERROR(lvds->dev, "failed to get pm runtime: %d\n", ret);
> +		clk_disable(lvds->pclk);
> +		return;
> +	}
> +
> +	lvds_serialiser_on(lvds);
> +}
> +
> +static void mchp_lvds_disable(struct drm_bridge *bridge)
> +{
> +	struct mchp_lvds *lvds = bridge_to_lvds(bridge);
> +
> +	pm_runtime_put(lvds->dev);
> +	clk_disable(lvds->pclk);
> +}
> +
> +static const struct drm_bridge_funcs mchp_lvds_bridge_funcs = {
> +	.attach = mchp_lvds_attach,
> +	.enable = mchp_lvds_enable,
> +	.disable = mchp_lvds_disable,
> +};
> +
> +static int mchp_lvds_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct mchp_lvds *lvds;
> +	struct device_node *port;
> +
> +	if (!dev->of_node)
> +		return -ENODEV;
> +
> +	lvds = devm_kzalloc(&pdev->dev, sizeof(*lvds), GFP_KERNEL);
> +	if (!lvds)
> +		return -ENOMEM;
> +
> +	lvds->dev = dev;
> +
> +	lvds->regs = devm_ioremap_resource(lvds->dev,
> +			platform_get_resource(pdev, IORESOURCE_MEM, 0));
> +	if (IS_ERR(lvds->regs))
> +		return PTR_ERR(lvds->regs);
> +
> +	lvds->pclk = devm_clk_get_prepared(lvds->dev, "pclk");
> +	if (IS_ERR(lvds->pclk))
> +		return dev_err_probe(lvds->dev, PTR_ERR(lvds->pclk),
> +				"could not get pclk_lvds prepared\n");
> +
> +	port = of_graph_get_remote_node(dev->of_node, 1, 0);
> +	if (!port) {
> +		DRM_DEV_ERROR(dev,
> +			      "can't find port point, please init lvds panel port!\n");
> +		return -ENODEV;
> +	}
> +
> +	lvds->panel = of_drm_find_panel(port);
> +	of_node_put(port);
> +
> +	if (IS_ERR(lvds->panel))
> +		return -EPROBE_DEFER;
> +
> +	lvds->panel_bridge = devm_drm_panel_bridge_add(dev, lvds->panel);
> +
> +	if (IS_ERR(lvds->panel_bridge))
> +		return PTR_ERR(lvds->panel_bridge);
> +
> +	lvds->bridge.of_node = dev->of_node;
> +	lvds->bridge.type = DRM_MODE_CONNECTOR_LVDS;
> +	lvds->bridge.funcs = &mchp_lvds_bridge_funcs;
> +
> +	dev_set_drvdata(dev, lvds);
> +	devm_pm_runtime_enable(dev);
> +
> +	drm_bridge_add(&lvds->bridge);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id mchp_lvds_dt_ids[] = {
> +	{
> +		.compatible = "microchip,sam9x75-lvds",
> +	},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, mchp_lvds_dt_ids);
> +
> +static struct platform_driver mchp_lvds_driver = {
> +	.probe = mchp_lvds_probe,
> +	.driver = {
> +		   .name = "microchip-lvds",
> +		   .of_match_table = mchp_lvds_dt_ids,
> +	},
> +};
> +module_platform_driver(mchp_lvds_driver);
> +
> +MODULE_AUTHOR("Manikandan Muralidharan <manikandan.m@microchip.com>");
> +MODULE_AUTHOR("Dharma Balasubiramani <dharma.b@microchip.com>");
> +MODULE_DESCRIPTION("Low Voltage Differential Signaling Controller Driver");
> +MODULE_LICENSE("GPL");


^ permalink raw reply

* [PATCH v4 2/2] arm64: dts: qcom: sm8550: Add support for Samsung Galaxy Z Fold5
From: Alexandru Marc Serdeliuc via B4 Relay @ 2024-04-07  5:38 UTC (permalink / raw)
  To: Bjorn Andersson, Konrad Dybcio, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley
  Cc: linux-arm-msm, devicetree, linux-kernel, Alexandru Marc Serdeliuc
In-Reply-To: <20240407-samsung-galaxy-zfold5-q5q-v4-0-8b67b1813653@yahoo.com>

From: Alexandru Marc Serdeliuc <serdeliuk@yahoo.com>

Add support for Samsung Galaxy Z Fold5 (q5q) foldable phone based on sm8550

Currently working features:
- Framebuffer
- UFS
- i2c
- Buttons

Signed-off-by: Alexandru Marc Serdeliuc <serdeliuk@yahoo.com>
---
 arch/arm64/boot/dts/qcom/Makefile               |   1 +
 arch/arm64/boot/dts/qcom/sm8550-samsung-q5q.dts | 593 ++++++++++++++++++++++++
 2 files changed, 594 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile
index 7d40ec5e7d21..a7503fd35b6c 100644
--- a/arch/arm64/boot/dts/qcom/Makefile
+++ b/arch/arm64/boot/dts/qcom/Makefile
@@ -241,6 +241,7 @@ dtb-$(CONFIG_ARCH_QCOM)	+= sm8450-sony-xperia-nagara-pdx224.dtb
 dtb-$(CONFIG_ARCH_QCOM)	+= sm8550-hdk.dtb
 dtb-$(CONFIG_ARCH_QCOM)	+= sm8550-mtp.dtb
 dtb-$(CONFIG_ARCH_QCOM)	+= sm8550-qrd.dtb
+dtb-$(CONFIG_ARCH_QCOM)	+= sm8550-samsung-q5q.dtb
 dtb-$(CONFIG_ARCH_QCOM)	+= sm8650-mtp.dtb
 dtb-$(CONFIG_ARCH_QCOM)	+= sm8650-qrd.dtb
 dtb-$(CONFIG_ARCH_QCOM)	+= x1e80100-crd.dtb
diff --git a/arch/arm64/boot/dts/qcom/sm8550-samsung-q5q.dts b/arch/arm64/boot/dts/qcom/sm8550-samsung-q5q.dts
new file mode 100644
index 000000000000..4654ae1364ba
--- /dev/null
+++ b/arch/arm64/boot/dts/qcom/sm8550-samsung-q5q.dts
@@ -0,0 +1,593 @@
+// SPDX-License-cdsp_memIdentifier: BSD-3-Clause
+/*
+ * Copyright (c) 2024, Alexandru Marc Serdeliuc <serdeliuk@yahoo.com>
+ * Copyright (c) 2024, David Wronek <david@mainlining.org>
+ * Copyright (c) 2022, Linaro Limited
+ */
+
+/dts-v1/;
+
+#include <dt-bindings/pinctrl/qcom,pmic-gpio.h>
+#include <dt-bindings/regulator/qcom,rpmh-regulator.h>
+#include "sm8550.dtsi"
+#include "pm8550.dtsi"
+#include "pm8550vs.dtsi"
+#include "pmk8550.dtsi"
+
+/delete-node/ &adspslpi_mem;
+/delete-node/ &cdsp_mem;
+/delete-node/ &mpss_dsm_mem;
+/delete-node/ &mpss_mem;
+/delete-node/ &rmtfs_mem;
+
+/ {
+	model = "Samsung Galaxy Z Fold5";
+	compatible = "samsung,q5q", "qcom,sm8550";
+	chassis-type = "handset";
+
+	aliases {
+		serial0 = &uart7;
+	};
+
+	chosen {
+		#address-cells = <2>;
+		#size-cells = <2>;
+		ranges;
+
+		framebuffer: framebuffer@b8000000 {
+			compatible = "simple-framebuffer";
+			reg = <0x0 0xb8000000 0x0 0x2b00000>;
+			width = <2176>;
+			height = <1812>;
+			stride = <(2176 * 4)>;
+			format = "a8r8g8b8";
+		};
+	};
+
+	gpio-keys {
+		compatible = "gpio-keys";
+		pinctrl-0 = <&volume_up_n>;
+		pinctrl-names = "default";
+
+		key-volume-up {
+			label = "Volume Up";
+			linux,code = <KEY_VOLUMEUP>;
+			gpios = <&pm8550_gpios 6 GPIO_ACTIVE_LOW>;
+			debounce-interval = <15>;
+			linux,can-disable;
+			wakeup-source;
+		};
+	};
+
+	vph_pwr: vph-pwr-regulator {
+		compatible = "regulator-fixed";
+		regulator-name = "vph_pwr";
+		regulator-min-microvolt = <3700000>;
+		regulator-max-microvolt = <3700000>;
+		regulator-always-on;
+		regulator-boot-on;
+	};
+
+	reserved-memory {
+		adspslpi_mem: adspslpi@9ea00000 {
+			reg = <0x0 0x9ea00000 0x0 0x59b4000>;
+			no-map;
+		};
+
+		cdsp_mem: cdsp-region@9c900000 {
+			reg = <0 0x9c900000 0 0x2000000>;
+			no-map;
+		};
+
+		mpss_dsm_mem: mpss-dsm@d4d00000 {
+			reg = <0x0 0xd4d00000 0x0 0x3300000>;
+			no-map;
+		};
+
+		mpss_mem: mpss@8b400000 {
+			reg = <0x0 0x8b400000 0x0 0xfc00000>;
+			no-map;
+		};
+
+		rmtfs_mem: rmtfs-region@d4a80000 {
+			reg = <0x0 0xd4a80000 0x0 0x280000>;
+			no-map;
+		};
+
+		/*
+		 * The bootloader will only keep display hardware enabled
+		 * if this memory region is named exactly 'splash_region'
+		 */
+		splash_region@b8000000 {
+			reg = <0x0 0xb8000000 0x0 0x2b00000>;
+			no-map;
+		};
+	};
+};
+
+&apps_rsc {
+	regulators-0 {
+		compatible = "qcom,pm8550-rpmh-regulators";
+		qcom,pmic-id = "b";
+
+		vreg_bob1: bob1 {
+			regulator-name = "vreg_bob1";
+			regulator-min-microvolt = <3296000>;
+			regulator-max-microvolt = <3960000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+		};
+
+		vreg_bob2: bob2 {
+			regulator-name = "vreg_bob2";
+			regulator-min-microvolt = <2720000>;
+			regulator-max-microvolt = <3960000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+		};
+
+		vreg_l1b_1p8: ldo1 {
+			regulator-name = "vreg_l1b_1p8";
+			regulator-min-microvolt = <1800000>;
+			regulator-max-microvolt = <1800000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+		};
+
+		vreg_l2b_3p0: ldo2 {
+			regulator-name = "vreg_l2b_3p0";
+			regulator-min-microvolt = <3008000>;
+			regulator-max-microvolt = <3008000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+		};
+
+		vreg_l5b_3p1: ldo5 {
+			regulator-name = "vreg_l5b_3p1";
+			regulator-min-microvolt = <3104000>;
+			regulator-max-microvolt = <3104000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+		};
+
+		vreg_l6b_1p8: ldo6 {
+			regulator-name = "vreg_l6b_1p8";
+			regulator-min-microvolt = <1800000>;
+			regulator-max-microvolt = <3008000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+		};
+
+		vreg_l7b_1p8: ldo7 {
+			regulator-name = "vreg_l7b_1p8";
+			regulator-min-microvolt = <1800000>;
+			regulator-max-microvolt = <3008000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+		};
+
+		vreg_l8b_1p8: ldo8 {
+			regulator-name = "vreg_l8b_1p8";
+			regulator-min-microvolt = <1800000>;
+			regulator-max-microvolt = <3008000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+		};
+
+		vreg_l9b_2p9: ldo9 {
+			regulator-name = "vreg_l9b_2p9";
+			regulator-min-microvolt = <2960000>;
+			regulator-max-microvolt = <3008000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+		};
+
+		vreg_l11b_1p2: ldo11 {
+			regulator-name = "vreg_l11b_1p2";
+			regulator-min-microvolt = <1200000>;
+			regulator-max-microvolt = <1504000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+		};
+
+		vreg_l12b_1p8: ldo12 {
+			regulator-name = "vreg_l12b_1p8";
+			regulator-min-microvolt = <1800000>;
+			regulator-max-microvolt = <1800000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+		};
+
+		vreg_l13b_3p0: ldo13 {
+			regulator-name = "vreg_l13b_3p0";
+			regulator-min-microvolt = <3000000>;
+			regulator-max-microvolt = <3000000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+		};
+
+		vreg_l14b_3p2: ldo14 {
+			regulator-name = "vreg_l14b_3p2";
+			regulator-min-microvolt = <3200000>;
+			regulator-max-microvolt = <3200000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+		};
+
+		vreg_l15b_1p8: ldo15 {
+			regulator-name = "vreg_l15b_1p8";
+			regulator-min-microvolt = <1800000>;
+			regulator-max-microvolt = <1800000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+			regulator-allow-set-load;
+			regulator-allowed-modes = <RPMH_REGULATOR_MODE_LPM
+						   RPMH_REGULATOR_MODE_HPM>;
+			regulator-always-on;
+		};
+
+		vreg_l16b_2p8: ldo16 {
+			regulator-name = "vreg_l16b_2p8";
+			regulator-min-microvolt = <2800000>;
+			regulator-max-microvolt = <2800000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+		};
+
+		vreg_l17b_2p5: ldo17 {
+			regulator-name = "vreg_l17b_2p5";
+			regulator-min-microvolt = <2504000>;
+			regulator-max-microvolt = <2504000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+		};
+	};
+
+	regulators-1 {
+		compatible = "qcom,pm8550vs-rpmh-regulators";
+		qcom,pmic-id = "c";
+
+		vreg_l3c_0p91: ldo3 {
+			regulator-name = "vreg_l3c_0p9";
+			regulator-min-microvolt = <880000>;
+			regulator-max-microvolt = <912000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+		};
+	};
+
+	regulators-2 {
+		compatible = "qcom,pm8550vs-rpmh-regulators";
+		qcom,pmic-id = "d";
+
+		vreg_l1d_0p88: ldo1 {
+			regulator-name = "vreg_l1d_0p88";
+			regulator-min-microvolt = <880000>;
+			regulator-max-microvolt = <920000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+		};
+	};
+
+	regulators-3 {
+		compatible = "qcom,pm8550vs-rpmh-regulators";
+		qcom,pmic-id = "e";
+
+		vreg_s4e_0p9: smps4 {
+			regulator-name = "vreg_s4e_0p9";
+			regulator-min-microvolt = <904000>;
+			regulator-max-microvolt = <984000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+		};
+
+		vreg_s5e_1p1: smps5 {
+			regulator-name = "vreg_s5e_1p1";
+			regulator-min-microvolt = <1080000>;
+			regulator-max-microvolt = <1120000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+		};
+
+		vreg_l1e_0p88: ldo1 {
+			regulator-name = "vreg_l1e_0p88";
+			regulator-min-microvolt = <880000>;
+			regulator-max-microvolt = <880000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+		};
+
+		vreg_l2e_0p9: ldo2 {
+			regulator-name = "vreg_l2e_0p9";
+			regulator-min-microvolt = <904000>;
+			regulator-max-microvolt = <970000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+		};
+
+		vreg_l3e_1p2: ldo3 {
+			regulator-name = "vreg_l3e_1p2";
+			regulator-min-microvolt = <1200000>;
+			regulator-max-microvolt = <1200000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+		};
+	};
+
+	regulators-4 {
+		compatible = "qcom,pm8550ve-rpmh-regulators";
+		qcom,pmic-id = "f";
+
+		vreg_s4f_0p5: smps4 {
+			regulator-name = "vreg_s4f_0p5";
+			regulator-min-microvolt = <500000>;
+			regulator-max-microvolt = <700000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+		};
+
+		vreg_l1f_0p9: ldo1 {
+			regulator-name = "vreg_l1f_0p9";
+			regulator-min-microvolt = <912000>;
+			regulator-max-microvolt = <912000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+		};
+
+		vreg_l2f_0p88: ldo2 {
+			regulator-name = "vreg_l2f_0p88";
+			regulator-min-microvolt = <880000>;
+			regulator-max-microvolt = <912000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+		};
+
+		vreg_l3f_0p91: ldo3 {
+			regulator-name = "vreg_l3f_0p91";
+			regulator-min-microvolt = <880000>;
+			regulator-max-microvolt = <912000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+		};
+	};
+
+	regulators-5 {
+		compatible = "qcom,pm8550vs-rpmh-regulators";
+		qcom,pmic-id = "g";
+
+		vreg_s1g_1p2: smps1 {
+			regulator-name = "vreg_s1g_1p2";
+			regulator-min-microvolt = <1200000>;
+			regulator-max-microvolt = <1300000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+		};
+
+		vreg_s2g_0p8: smps2 {
+			regulator-name = "vreg_s2g_0p8";
+			regulator-min-microvolt = <800000>;
+			regulator-max-microvolt = <1000000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+		};
+
+		vreg_s3g_0p7: smps3 {
+			regulator-name = "vreg_s3g_0p7";
+			regulator-min-microvolt = <300000>;
+			regulator-max-microvolt = <1004000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+		};
+
+		vreg_s4g_1p3: smps4 {
+			regulator-name = "vreg_s4g_1p3";
+			regulator-min-microvolt = <1200000>;
+			regulator-max-microvolt = <1352000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+		};
+
+		vreg_s5g_0p8: smps5 {
+			regulator-name = "vreg_s5g_0p8";
+			regulator-min-microvolt = <500000>;
+			regulator-max-microvolt = <1004000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+		};
+
+		vreg_s6g_1p8: smps6 {
+			regulator-name = "vreg_s6g_1p8";
+			regulator-min-microvolt = <1800000>;
+			regulator-max-microvolt = <2000000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+		};
+
+		vreg_l1g_1p2: ldo1 {
+			regulator-name = "vreg_l1g_1p2";
+			regulator-min-microvolt = <1200000>;
+			regulator-max-microvolt = <1200000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+		};
+
+		vreg_l2g_1p2: ldo2 {
+			regulator-name = "vreg_l2g_1p2";
+			regulator-min-microvolt = <1200000>;
+			regulator-max-microvolt = <1200000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+		};
+
+		vreg_l3g_1p2: ldo3 {
+			regulator-name = "vreg_l3g_1p2";
+			regulator-min-microvolt = <1200000>;
+			regulator-max-microvolt = <1200000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+		};
+	};
+
+	regulators-6 {
+		compatible = "qcom,pm8010-rpmh-regulators";
+		qcom,pmic-id = "m";
+
+		vreg_l1m_1p056: ldo1 {
+			regulator-name = "vreg_l1m_1p056";
+			regulator-min-microvolt = <1056000>;
+			regulator-max-microvolt = <1056000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+		};
+
+		vreg_l2m_1p056: ldo2 {
+			regulator-name = "vreg_l2m_1p056";
+			regulator-min-microvolt = <1056000>;
+			regulator-max-microvolt = <1056000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+		};
+
+		vreg_l3m_2p8: ldo3 {
+			regulator-name = "vreg_l3m_2p8";
+			regulator-min-microvolt = <2800000>;
+			regulator-max-microvolt = <2800000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+		};
+
+		vreg_l4m_2p8: ldo4 {
+			regulator-name = "vreg_l4m_2p8";
+			regulator-min-microvolt = <2800000>;
+			regulator-max-microvolt = <2800000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+		};
+
+		vreg_l5m_1p8: ldo5 {
+			regulator-name = "vreg_l5m_1p8";
+			regulator-min-microvolt = <1800000>;
+			regulator-max-microvolt = <1800000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+		};
+
+		vreg_l6m_1p8: ldo6 {
+			regulator-name = "vreg_l6m_1p8";
+			regulator-min-microvolt = <1800000>;
+			regulator-max-microvolt = <1800000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+		};
+
+		vreg_l7m_2p9: ldo7 {
+			regulator-name = "vreg_l7m_2p9";
+			regulator-min-microvolt = <2800000>;
+			regulator-max-microvolt = <2904000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+		};
+	};
+
+	regulators-7 {
+		compatible = "qcom,pm8010-rpmh-regulators";
+		qcom,pmic-id = "n";
+
+		vreg_l1n_1p1: ldo1 {
+			regulator-name = "vreg_l1n_1p1";
+			regulator-min-microvolt = <1104000>;
+			regulator-max-microvolt = <1200000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+		};
+
+		vreg_l2n_1p1: ldo2 {
+			regulator-name = "vreg_l2n_1p1";
+			regulator-min-microvolt = <1104000>;
+			regulator-max-microvolt = <1200000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+		};
+
+		vreg_l3n_2p8: ldo3 {
+			regulator-name = "vreg_l3n_2p8";
+			regulator-min-microvolt = <2800000>;
+			regulator-max-microvolt = <3000000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+		};
+
+		vreg_l4n_2p8: ldo4 {
+			regulator-name = "vreg_l4n_2p8";
+			regulator-min-microvolt = <2800000>;
+			regulator-max-microvolt = <3300000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+		};
+
+		vreg_l5n_1p8: ldo5 {
+			regulator-name = "vreg_l5n_1p8";
+			regulator-min-microvolt = <1800000>;
+			regulator-max-microvolt = <1800000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+		};
+
+		vreg_l6n_3p3: ldo6 {
+			regulator-name = "vreg_l6n_3p3";
+			regulator-min-microvolt = <2800000>;
+			regulator-max-microvolt = <3304000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+		};
+
+		vreg_l7n_2p96: ldo7 {
+			regulator-name = "vreg_l7n_2p96";
+			regulator-min-microvolt = <2800000>;
+			regulator-max-microvolt = <2960000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+		};
+	};
+};
+
+&dispcc {
+	status = "disabled";
+};
+
+&i2c_master_hub_0 {
+	status = "okay";
+};
+
+&pcie0 {
+	wake-gpios = <&tlmm 96 GPIO_ACTIVE_HIGH>;
+	perst-gpios = <&tlmm 94 GPIO_ACTIVE_LOW>;
+	pinctrl-0 = <&pcie0_default_state>;
+	pinctrl-names = "default";
+	status = "okay";
+};
+
+&pcie0_phy {
+	vdda-phy-supply = <&vreg_l1e_0p88>;
+	vdda-pll-supply = <&vreg_l3e_1p2>;
+	status = "okay";
+};
+
+&pm8550_gpios {
+	volume_up_n: volume-up-n-state {
+		pins = "gpio6";
+		function = "normal";
+		power-source = <1>;
+		bias-pull-up;
+		input-enable;
+	};
+};
+
+&pon_pwrkey {
+	status = "okay";
+};
+
+&pon_resin {
+	status = "okay";
+	linux,code = <KEY_VOLUMEDOWN>;
+};
+
+&qupv3_id_0 {
+	status = "okay";
+};
+
+&remoteproc_adsp {
+	firmware-name = "qcom/sm8550/adsp.mdt",
+			"qcom/sm8550/adsp_dtb.mdt";
+	status = "okay";
+};
+
+&remoteproc_cdsp {
+	firmware-name = "qcom/sm8550/cdsp.mdt",
+			"qcom/sm8550/cdsp_dtb.mdt";
+	status = "okay";
+};
+
+&remoteproc_mpss {
+	firmware-name = "qcom/sm8550/modem.mdt",
+			"qcom/sm8550/modem_dtb.mdt";
+	status = "okay";
+};
+
+&sleep_clk {
+	clock-frequency = <32000>;
+};
+
+&tlmm {
+	gpio-reserved-ranges = <36 4>, <50 2>;
+};
+
+&ufs_mem_hc {
+	reset-gpios = <&tlmm 210 GPIO_ACTIVE_LOW>;
+	vcc-supply = <&vreg_l17b_2p5>;
+	vcc-max-microamp = <1300000>;
+	vccq-supply = <&vreg_l1g_1p2>;
+	vccq-max-microamp = <1200000>;
+	vdd-hba-supply = <&vreg_l3g_1p2>;
+	status = "okay";
+};
+
+&ufs_mem_phy {
+	vdda-phy-supply = <&vreg_l1d_0p88>;
+	vdda-pll-supply = <&vreg_l3e_1p2>;
+	status = "okay";
+};
+
+&xo_board {
+	clock-frequency = <76800000>;
+};

-- 
2.34.1



^ permalink raw reply related

* [PATCH v4 0/2] Samsung Galaxy Z Fold5 initial support
From: Alexandru Marc Serdeliuc via B4 Relay @ 2024-04-07  5:38 UTC (permalink / raw)
  To: Bjorn Andersson, Konrad Dybcio, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley
  Cc: linux-arm-msm, devicetree, linux-kernel, Alexandru Marc Serdeliuc

- removed extraneous new line
- removed pcie_1_phy_aux_clk
- removed extranous pcie1

This documents and add intial dts support for Samsung Galaxy Z Fold5 (samsung,q5q)
which is a foldable phone by Samsung based on the sm8550 SoC.

Currently working features:
- Framebuffer
- UFS
- i2c
- Buttons

Signed-off-by: Alexandru Marc Serdeliuc <serdeliuk@yahoo.com>
---
Alexandru Marc Serdeliuc (2):
      dt-bindings: arm: qcom: Document the Samsung Galaxy Z Fold5
      arm64: dts: qcom: sm8550: Add support for Samsung Galaxy Z Fold5

 Documentation/devicetree/bindings/arm/qcom.yaml |   1 +
 arch/arm64/boot/dts/qcom/Makefile               |   1 +
 arch/arm64/boot/dts/qcom/sm8550-samsung-q5q.dts | 593 ++++++++++++++++++++++++
 3 files changed, 595 insertions(+)
---
base-commit: 39cd87c4eb2b893354f3b850f916353f2658ae6f
change-id: 20240407-samsung-galaxy-zfold5-q5q-ab1fdb3df966

Best regards,
-- 
Alexandru Marc Serdeliuc <serdeliuk@yahoo.com>



^ permalink raw reply

* [PATCH v4 1/2] dt-bindings: arm: qcom: Document the Samsung Galaxy Z Fold5
From: Alexandru Marc Serdeliuc via B4 Relay @ 2024-04-07  5:38 UTC (permalink / raw)
  To: Bjorn Andersson, Konrad Dybcio, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley
  Cc: linux-arm-msm, devicetree, linux-kernel, Alexandru Marc Serdeliuc
In-Reply-To: <20240407-samsung-galaxy-zfold5-q5q-v4-0-8b67b1813653@yahoo.com>

From: Alexandru Marc Serdeliuc <serdeliuk@yahoo.com>

This documents Samsung Galaxy Z Fold5 (samsung,q5q)
which is a foldable phone by Samsung based on the sm8550 SoC.

Signed-off-by: Alexandru Marc Serdeliuc <serdeliuk@yahoo.com>
---
 Documentation/devicetree/bindings/arm/qcom.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/arm/qcom.yaml b/Documentation/devicetree/bindings/arm/qcom.yaml
index 66beaac60e1d..dea2a23b8fc2 100644
--- a/Documentation/devicetree/bindings/arm/qcom.yaml
+++ b/Documentation/devicetree/bindings/arm/qcom.yaml
@@ -1003,6 +1003,7 @@ properties:
               - qcom,sm8550-hdk
               - qcom,sm8550-mtp
               - qcom,sm8550-qrd
+              - samsung,q5q
           - const: qcom,sm8550
 
       - items:

-- 
2.34.1



^ permalink raw reply related

* [PATCH v1 1/1] arm64: dts: imx93-11x11-evk: add rtc PCF2131 support
From: Joy Zou @ 2024-04-07  5:19 UTC (permalink / raw)
  To: ping.bai, robh+dt, krzysztof.kozlowski+dt, conor+dt, shawnguo,
	s.hauer
  Cc: kernel, festevam, linux-imx, devicetree, imx, linux-arm-kernel,
	linux-kernel
In-Reply-To: <20240407051913.1989364-1-joy.zou@nxp.com>

Support rtc PCF2131 on imx93-11x11-evk.

Signed-off-by: Joy Zou <joy.zou@nxp.com>
---
 .../boot/dts/freescale/imx93-11x11-evk.dts    | 25 +++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/imx93-11x11-evk.dts b/arch/arm64/boot/dts/freescale/imx93-11x11-evk.dts
index 07e85a30a25f..065fa3390791 100644
--- a/arch/arm64/boot/dts/freescale/imx93-11x11-evk.dts
+++ b/arch/arm64/boot/dts/freescale/imx93-11x11-evk.dts
@@ -281,6 +281,24 @@ ldo5: LDO5 {
 	};
 };
 
+&lpi2c3 {
+	#address-cells = <1>;
+	#size-cells = <0>;
+	clock-frequency = <400000>;
+	pinctrl-names = "default", "sleep";
+	pinctrl-0 = <&pinctrl_lpi2c3>;
+	pinctrl-1 = <&pinctrl_lpi2c3>;
+	status = "okay";
+
+	pcf2131: rtc@53 {
+			compatible = "nxp,pcf2131";
+			reg = <0x53>;
+			interrupt-parent = <&pcal6524>;
+			interrupts = <1 IRQ_TYPE_EDGE_FALLING>;
+			status = "okay";
+	};
+};
+
 &iomuxc {
 	pinctrl_eqos: eqosgrp {
 		fsl,pins = <
@@ -343,6 +361,13 @@ MX93_PAD_I2C2_SDA__LPI2C2_SDA			0x40000b9e
 		>;
 	};
 
+	pinctrl_lpi2c3: lpi2c3grp {
+		fsl,pins = <
+			MX93_PAD_GPIO_IO28__LPI2C3_SDA			0x40000b9e
+			MX93_PAD_GPIO_IO29__LPI2C3_SCL			0x40000b9e
+		>;
+	};
+
 	pinctrl_pcal6524: pcal6524grp {
 		fsl,pins = <
 			MX93_PAD_CCM_CLKO2__GPIO3_IO27			0x31e
-- 
2.37.1


^ permalink raw reply related

* [PATCH v1 0/1] Add rtc PCF2131 support
From: Joy Zou @ 2024-04-07  5:19 UTC (permalink / raw)
  To: ping.bai, robh+dt, krzysztof.kozlowski+dt, conor+dt, shawnguo,
	s.hauer
  Cc: kernel, festevam, linux-imx, devicetree, imx, linux-arm-kernel,
	linux-kernel

The patchset supports RTC PCF2131 on board dts.
For the details, please check the patch commit log.

Joy Zou (1):
  arm64: dts: imx93-11x11-evk: add rtc PCF2131 support

 .../boot/dts/freescale/imx93-11x11-evk.dts    | 25 +++++++++++++++++++
 1 file changed, 25 insertions(+)

-- 
2.37.1


^ permalink raw reply

* [PATCH v9 6/6] PCI: qcom: Add OPP support to scale performance state of power domain
From: Krishna chaitanya chundru @ 2024-04-07  4:37 UTC (permalink / raw)
  To: Bjorn Andersson, Konrad Dybcio, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Manivannan Sadhasivam, Lorenzo Pieralisi,
	Krzysztof Wilczyński, Bjorn Helgaas, johan+linaro, bmasney,
	djakov
  Cc: linux-arm-msm, devicetree, linux-kernel, linux-pci, vireshk,
	quic_vbadigan, quic_skananth, quic_nitegupt, quic_parass,
	quic_krichai, krzysztof.kozlowski
In-Reply-To: <20240407-opp_support-v9-0-496184dc45d7@quicinc.com>

QCOM Resource Power Manager-hardened (RPMh) is a hardware block which
maintains hardware state of a regulator by performing max aggregation of
the requests made by all of the clients.

PCIe controller can operate on different RPMh performance state of power
domain based on the speed of the link. And this performance state varies
from target to target, like some controllers support GEN3 in NOM (Nominal)
voltage corner, while some other supports GEN3 in low SVS (static voltage
scaling).

The SoC can be more power efficient if we scale the performance state
based on the aggregate PCIe link bandwidth.

Add Operating Performance Points (OPP) support to vote for RPMh state based
on the aggregate link bandwidth.

OPP can handle ICC bw voting also, so move ICC bw voting through OPP
framework if OPP entries are present.

Different link configurations may share the same aggregate bandwidth,
e.g., a 2.5 GT/s x2 link and a 5.0 GT/s x1 link have the same bandwidth
and share the same OPP entry.

As we are moving ICC voting as part of OPP, don't initialize ICC if OPP
is supported.

Before PCIe link is initialized vote for highest OPP in the OPP table,
so that we are voting for maximum voltage corner for the link to come up
in maximum supported speed.

Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 72 +++++++++++++++++++++++++++-------
 1 file changed, 58 insertions(+), 14 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index b4893214b2d3..4ad5ef3bf8fc 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -22,6 +22,7 @@
 #include <linux/of.h>
 #include <linux/of_gpio.h>
 #include <linux/pci.h>
+#include <linux/pm_opp.h>
 #include <linux/pm_runtime.h>
 #include <linux/platform_device.h>
 #include <linux/phy/pcie.h>
@@ -1442,15 +1443,13 @@ static int qcom_pcie_icc_init(struct qcom_pcie *pcie)
 	return 0;
 }
 
-static void qcom_pcie_icc_update(struct qcom_pcie *pcie)
+static void qcom_pcie_icc_opp_update(struct qcom_pcie *pcie)
 {
 	struct dw_pcie *pci = pcie->pci;
-	u32 offset, status;
+	u32 offset, status, freq;
+	struct dev_pm_opp *opp;
 	int speed, width;
-	int ret;
-
-	if (!pcie->icc_mem)
-		return;
+	int ret, mbps;
 
 	offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
 	status = readw(pci->dbi_base + offset + PCI_EXP_LNKSTA);
@@ -1462,10 +1461,26 @@ static void qcom_pcie_icc_update(struct qcom_pcie *pcie)
 	speed = FIELD_GET(PCI_EXP_LNKSTA_CLS, status);
 	width = FIELD_GET(PCI_EXP_LNKSTA_NLW, status);
 
-	ret = icc_set_bw(pcie->icc_mem, 0, width * QCOM_PCIE_LINK_SPEED_TO_BW(speed));
-	if (ret) {
-		dev_err(pci->dev, "failed to set interconnect bandwidth for PCIe-MEM: %d\n",
-			ret);
+	if (pcie->icc_mem) {
+		ret = icc_set_bw(pcie->icc_mem, 0, width * QCOM_PCIE_LINK_SPEED_TO_BW(speed));
+		if (ret) {
+			dev_err(pci->dev, "failed to set interconnect bandwidth for PCIe-MEM: %d\n",
+				ret);
+		}
+	} else {
+		mbps = pcie_link_speed_to_mbps(pcie_link_speed[speed]);
+		if (mbps < 0)
+			return;
+
+		freq = mbps * 1000;
+		opp = dev_pm_opp_find_freq_exact(pci->dev, freq * width, true);
+		if (!IS_ERR(opp)) {
+			ret = dev_pm_opp_set_opp(pci->dev, opp);
+			if (ret)
+				dev_err(pci->dev, "Failed to set opp: freq %ld ret %d\n",
+					dev_pm_opp_get_freq(opp), ret);
+			dev_pm_opp_put(opp);
+		}
 	}
 }
 
@@ -1509,8 +1524,10 @@ static void qcom_pcie_init_debugfs(struct qcom_pcie *pcie)
 static int qcom_pcie_probe(struct platform_device *pdev)
 {
 	const struct qcom_pcie_cfg *pcie_cfg;
+	unsigned long max_freq = INT_MAX;
 	struct device *dev = &pdev->dev;
 	struct qcom_pcie *pcie;
+	struct dev_pm_opp *opp;
 	struct dw_pcie_rp *pp;
 	struct resource *res;
 	struct dw_pcie *pci;
@@ -1577,9 +1594,33 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 		goto err_pm_runtime_put;
 	}
 
-	ret = qcom_pcie_icc_init(pcie);
-	if (ret)
+	/* OPP table is optional */
+	ret = devm_pm_opp_of_add_table(dev);
+	if (ret && ret != -ENODEV) {
+		dev_err_probe(dev, ret, "Failed to add OPP table\n");
 		goto err_pm_runtime_put;
+	}
+
+	/*
+	 * Use highest OPP here if the OPP table is present. At the end of
+	 * the probe(), OPP will be updated using qcom_pcie_icc_opp_update().
+	 */
+	if (!ret) {
+		opp = dev_pm_opp_find_freq_floor(dev, &max_freq);
+		if (!IS_ERR(opp)) {
+			ret = dev_pm_opp_set_opp(dev, opp);
+			if (ret)
+				dev_err_probe(pci->dev, ret,
+					      "Failed to set OPP: freq %ld\n",
+					      dev_pm_opp_get_freq(opp));
+			dev_pm_opp_put(opp);
+		}
+	} else {
+		/* Skip ICC init if OPP is supported as it is handled by OPP */
+		ret = qcom_pcie_icc_init(pcie);
+		if (ret)
+			goto err_pm_runtime_put;
+	}
 
 	ret = pcie->cfg->ops->get_resources(pcie);
 	if (ret)
@@ -1599,7 +1640,7 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 		goto err_phy_exit;
 	}
 
-	qcom_pcie_icc_update(pcie);
+	qcom_pcie_icc_opp_update(pcie);
 
 	if (pcie->mhi)
 		qcom_pcie_init_debugfs(pcie);
@@ -1658,6 +1699,9 @@ static int qcom_pcie_suspend_noirq(struct device *dev)
 	if (ret)
 		dev_err(dev, "failed to disable icc path of CPU-PCIe: %d\n", ret);
 
+	if (!pcie->icc_mem)
+		dev_pm_opp_set_opp(pcie->pci->dev, NULL);
+
 	return ret;
 }
 
@@ -1680,7 +1724,7 @@ static int qcom_pcie_resume_noirq(struct device *dev)
 		pcie->suspended = false;
 	}
 
-	qcom_pcie_icc_update(pcie);
+	qcom_pcie_icc_opp_update(pcie);
 
 	return 0;
 }

-- 
2.42.0


^ permalink raw reply related

* [PATCH v9 5/6] PCI: Bring the PCIe speed to MBps logic to new pcie_link_speed_to_mbps()
From: Krishna chaitanya chundru @ 2024-04-07  4:37 UTC (permalink / raw)
  To: Bjorn Andersson, Konrad Dybcio, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Manivannan Sadhasivam, Lorenzo Pieralisi,
	Krzysztof Wilczyński, Bjorn Helgaas, johan+linaro, bmasney,
	djakov
  Cc: linux-arm-msm, devicetree, linux-kernel, linux-pci, vireshk,
	quic_vbadigan, quic_skananth, quic_nitegupt, quic_parass,
	quic_krichai, krzysztof.kozlowski
In-Reply-To: <20240407-opp_support-v9-0-496184dc45d7@quicinc.com>

Bring the switch case in pcie_link_speed_mbps() to new function to
the header file so that it can be used in other places like
in controller driver.

Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/pci/pci.c | 19 +------------------
 drivers/pci/pci.h | 22 ++++++++++++++++++++++
 2 files changed, 23 insertions(+), 18 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index e5f243dd4288..40487b86a75e 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5922,24 +5922,7 @@ int pcie_link_speed_mbps(struct pci_dev *pdev)
 	if (err)
 		return err;
 
-	switch (to_pcie_link_speed(lnksta)) {
-	case PCIE_SPEED_2_5GT:
-		return 2500;
-	case PCIE_SPEED_5_0GT:
-		return 5000;
-	case PCIE_SPEED_8_0GT:
-		return 8000;
-	case PCIE_SPEED_16_0GT:
-		return 16000;
-	case PCIE_SPEED_32_0GT:
-		return 32000;
-	case PCIE_SPEED_64_0GT:
-		return 64000;
-	default:
-		break;
-	}
-
-	return -EINVAL;
+	return pcie_link_speed_to_mbps(to_pcie_link_speed(lnksta));
 }
 EXPORT_SYMBOL(pcie_link_speed_mbps);
 
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 17fed1846847..4de10087523e 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -290,6 +290,28 @@ void pci_bus_put(struct pci_bus *bus);
 	 (speed) == PCIE_SPEED_2_5GT  ?  2500*8/10 : \
 	 0)
 
+static inline int pcie_link_speed_to_mbps(enum pci_bus_speed speed)
+{
+	switch (speed) {
+	case PCIE_SPEED_2_5GT:
+		return 2500;
+	case PCIE_SPEED_5_0GT:
+		return 5000;
+	case PCIE_SPEED_8_0GT:
+		return 8000;
+	case PCIE_SPEED_16_0GT:
+		return 16000;
+	case PCIE_SPEED_32_0GT:
+		return 32000;
+	case PCIE_SPEED_64_0GT:
+		return 64000;
+	default:
+		break;
+	}
+
+	return -EINVAL;
+}
+
 const char *pci_speed_string(enum pci_bus_speed speed);
 enum pci_bus_speed pcie_get_speed_cap(struct pci_dev *dev);
 enum pcie_link_width pcie_get_width_cap(struct pci_dev *dev);

-- 
2.42.0


^ permalink raw reply related

* [PATCH v9 4/6] arm64: dts: qcom: sm8450: Add opp table support to PCIe
From: Krishna chaitanya chundru @ 2024-04-07  4:37 UTC (permalink / raw)
  To: Bjorn Andersson, Konrad Dybcio, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Manivannan Sadhasivam, Lorenzo Pieralisi,
	Krzysztof Wilczyński, Bjorn Helgaas, johan+linaro, bmasney,
	djakov
  Cc: linux-arm-msm, devicetree, linux-kernel, linux-pci, vireshk,
	quic_vbadigan, quic_skananth, quic_nitegupt, quic_parass,
	quic_krichai, krzysztof.kozlowski
In-Reply-To: <20240407-opp_support-v9-0-496184dc45d7@quicinc.com>

PCIe needs to choose the appropriate performance state of RPMH power
domain and interconnect bandwidth based up on the PCIe gen speed.

Add the OPP table support to specify RPMH performance states and
interconnect peak bandwidth.

Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sm8450.dtsi | 77 ++++++++++++++++++++++++++++++++++++
 1 file changed, 77 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi
index 615296e13c43..881e5339cfff 100644
--- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
@@ -1855,7 +1855,35 @@ &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
 			pinctrl-names = "default";
 			pinctrl-0 = <&pcie0_default_state>;
 
+			operating-points-v2 = <&pcie0_opp_table>;
+
 			status = "disabled";
+
+			pcie0_opp_table: opp-table {
+				compatible = "operating-points-v2";
+
+				/* GEN 1x1 */
+				opp-2500000 {
+					opp-hz = /bits/ 64 <2500000>;
+					required-opps = <&rpmhpd_opp_low_svs>;
+					opp-peak-kBps = <250000 1>;
+				};
+
+				/* GEN 2x1 */
+				opp-5000000 {
+					opp-hz = /bits/ 64 <5000000>;
+					required-opps = <&rpmhpd_opp_low_svs>;
+					opp-peak-kBps = <500000 1>;
+				};
+
+				/* GEN 3x1 */
+				opp-8000000 {
+					opp-hz = /bits/ 64 <8000000>;
+					required-opps = <&rpmhpd_opp_nom>;
+					opp-peak-kBps = <984500 1>;
+				};
+			};
+
 		};
 
 		pcie0_phy: phy@1c06000 {
@@ -1982,7 +2010,56 @@ &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
 			pinctrl-names = "default";
 			pinctrl-0 = <&pcie1_default_state>;
 
+			operating-points-v2 = <&pcie1_opp_table>;
+
 			status = "disabled";
+
+			pcie1_opp_table: opp-table {
+				compatible = "operating-points-v2";
+
+				/* GEN 1x1 */
+				opp-2500000 {
+					opp-hz = /bits/ 64 <2500000>;
+					required-opps = <&rpmhpd_opp_low_svs>;
+					opp-peak-kBps = <250000 1>;
+				};
+
+				/* GEN 1x2 GEN 2x1 */
+				opp-5000000 {
+					opp-hz = /bits/ 64 <5000000>;
+					required-opps = <&rpmhpd_opp_low_svs>;
+					opp-peak-kBps = <500000 1>;
+				};
+
+				/* GEN 2x2 */
+				opp-10000000 {
+					opp-hz = /bits/ 64 <10000000>;
+					required-opps = <&rpmhpd_opp_low_svs>;
+					opp-peak-kBps = <1000000 1>;
+				};
+
+				/* GEN 3x1 */
+				opp-8000000 {
+					opp-hz = /bits/ 64 <8000000>;
+					required-opps = <&rpmhpd_opp_nom>;
+					opp-peak-kBps = <984500 1>;
+				};
+
+				/* GEN 3x2 GEN 4x1 */
+				opp-16000000 {
+					opp-hz = /bits/ 64 <16000000>;
+					required-opps = <&rpmhpd_opp_nom>;
+					opp-peak-kBps = <1969000 1>;
+				};
+
+				/* GEN 4x2 */
+				opp-32000000 {
+					opp-hz = /bits/ 64 <32000000>;
+					required-opps = <&rpmhpd_opp_nom>;
+					opp-peak-kBps = <3938000 1>;
+				};
+			};
+
 		};
 
 		pcie1_phy: phy@1c0e000 {

-- 
2.42.0


^ permalink raw reply related

* [PATCH v9 3/6] dt-bindings: pci: qcom: Add opp table
From: Krishna chaitanya chundru @ 2024-04-07  4:37 UTC (permalink / raw)
  To: Bjorn Andersson, Konrad Dybcio, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Manivannan Sadhasivam, Lorenzo Pieralisi,
	Krzysztof Wilczyński, Bjorn Helgaas, johan+linaro, bmasney,
	djakov
  Cc: linux-arm-msm, devicetree, linux-kernel, linux-pci, vireshk,
	quic_vbadigan, quic_skananth, quic_nitegupt, quic_parass,
	quic_krichai, krzysztof.kozlowski
In-Reply-To: <20240407-opp_support-v9-0-496184dc45d7@quicinc.com>

PCIe needs to choose the appropriate performance state of RPMH power
domain based upon the PCIe gen speed.

Adding the Operating Performance Points table allows to adjust power
domain performance state and icc peak bw, depending on the PCIe gen
speed and width.

Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
---
 Documentation/devicetree/bindings/pci/qcom,pcie-sm8450.yaml | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie-sm8450.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie-sm8450.yaml
index 1496d6993ab4..d8c0afaa4b19 100644
--- a/Documentation/devicetree/bindings/pci/qcom,pcie-sm8450.yaml
+++ b/Documentation/devicetree/bindings/pci/qcom,pcie-sm8450.yaml
@@ -69,6 +69,10 @@ properties:
       - const: msi6
       - const: msi7
 
+  operating-points-v2: true
+  opp-table:
+    type: object
+
   resets:
     maxItems: 1
 

-- 
2.42.0


^ permalink raw reply related

* [PATCH v9 2/6] PCI: qcom: Add ICC bandwidth vote for CPU to PCIe path
From: Krishna chaitanya chundru @ 2024-04-07  4:37 UTC (permalink / raw)
  To: Bjorn Andersson, Konrad Dybcio, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Manivannan Sadhasivam, Lorenzo Pieralisi,
	Krzysztof Wilczyński, Bjorn Helgaas, johan+linaro, bmasney,
	djakov
  Cc: linux-arm-msm, devicetree, linux-kernel, linux-pci, vireshk,
	quic_vbadigan, quic_skananth, quic_nitegupt, quic_parass,
	quic_krichai, krzysztof.kozlowski, Bryan O'Donoghue
In-Reply-To: <20240407-opp_support-v9-0-496184dc45d7@quicinc.com>

To access PCIe registers, PCIe BAR space, config space the CPU-PCIe
ICC (interconnect consumers) path should be voted otherwise it may
lead to NoC (Network on chip) timeout. We are surviving because of
other driver vote for this path.

As there is less access on this path compared to PCIe to mem path
add minimum vote i.e 1KBps bandwidth always which is recommended
by HW team.

When suspending, disable this path after register space access
is done.

Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 38 ++++++++++++++++++++++++++++++----
 1 file changed, 34 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 14772edcf0d3..b4893214b2d3 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -245,6 +245,7 @@ struct qcom_pcie {
 	struct phy *phy;
 	struct gpio_desc *reset;
 	struct icc_path *icc_mem;
+	struct icc_path *icc_cpu;
 	const struct qcom_pcie_cfg *cfg;
 	struct dentry *debugfs;
 	bool suspended;
@@ -1409,6 +1410,9 @@ static int qcom_pcie_icc_init(struct qcom_pcie *pcie)
 	if (IS_ERR(pcie->icc_mem))
 		return PTR_ERR(pcie->icc_mem);
 
+	pcie->icc_cpu = devm_of_icc_get(pci->dev, "cpu-pcie");
+	if (IS_ERR(pcie->icc_cpu))
+		return PTR_ERR(pcie->icc_cpu);
 	/*
 	 * Some Qualcomm platforms require interconnect bandwidth constraints
 	 * to be set before enabling interconnect clocks.
@@ -1418,7 +1422,19 @@ static int qcom_pcie_icc_init(struct qcom_pcie *pcie)
 	 */
 	ret = icc_set_bw(pcie->icc_mem, 0, QCOM_PCIE_LINK_SPEED_TO_BW(1));
 	if (ret) {
-		dev_err(pci->dev, "failed to set interconnect bandwidth: %d\n",
+		dev_err(pci->dev, "failed to set interconnect bandwidth for PCIe-MEM: %d\n",
+			ret);
+		return ret;
+	}
+
+	/*
+	 * Since the CPU-PCIe path is only used for activities like register
+	 * access, Config/BAR space access, HW team has recommended to use a
+	 * minimal bandwidth of 1KBps just to keep the link active.
+	 */
+	ret = icc_set_bw(pcie->icc_cpu, 0, kBps_to_icc(1));
+	if (ret) {
+		dev_err(pci->dev, "failed to set interconnect bandwidth for CPU-PCIe: %d\n",
 			ret);
 		return ret;
 	}
@@ -1448,7 +1464,7 @@ static void qcom_pcie_icc_update(struct qcom_pcie *pcie)
 
 	ret = icc_set_bw(pcie->icc_mem, 0, width * QCOM_PCIE_LINK_SPEED_TO_BW(speed));
 	if (ret) {
-		dev_err(pci->dev, "failed to set interconnect bandwidth: %d\n",
+		dev_err(pci->dev, "failed to set interconnect bandwidth for PCIe-MEM: %d\n",
 			ret);
 	}
 }
@@ -1610,7 +1626,7 @@ static int qcom_pcie_suspend_noirq(struct device *dev)
 	 */
 	ret = icc_set_bw(pcie->icc_mem, 0, kBps_to_icc(1));
 	if (ret) {
-		dev_err(dev, "Failed to set interconnect bandwidth: %d\n", ret);
+		dev_err(dev, "Failed to set interconnect bandwidth for PCIe-MEM: %d\n", ret);
 		return ret;
 	}
 
@@ -1634,7 +1650,15 @@ static int qcom_pcie_suspend_noirq(struct device *dev)
 		pcie->suspended = true;
 	}
 
-	return 0;
+	/*
+	 * Remove the vote for CPU-PCIe path now, since at this point onwards,
+	 * no register access will be done.
+	 */
+	ret = icc_disable(pcie->icc_cpu);
+	if (ret)
+		dev_err(dev, "failed to disable icc path of CPU-PCIe: %d\n", ret);
+
+	return ret;
 }
 
 static int qcom_pcie_resume_noirq(struct device *dev)
@@ -1642,6 +1666,12 @@ static int qcom_pcie_resume_noirq(struct device *dev)
 	struct qcom_pcie *pcie = dev_get_drvdata(dev);
 	int ret;
 
+	ret = icc_enable(pcie->icc_cpu);
+	if (ret) {
+		dev_err(dev, "failed to enable icc path of CPU-PCIe: %d\n", ret);
+		return ret;
+	}
+
 	if (pcie->suspended) {
 		ret = qcom_pcie_host_init(&pcie->pci->pp);
 		if (ret)

-- 
2.42.0


^ permalink raw reply related

* [PATCH v9 1/6] arm64: dts: qcom: sm8450: Add interconnect path to PCIe node
From: Krishna chaitanya chundru @ 2024-04-07  4:37 UTC (permalink / raw)
  To: Bjorn Andersson, Konrad Dybcio, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Manivannan Sadhasivam, Lorenzo Pieralisi,
	Krzysztof Wilczyński, Bjorn Helgaas, johan+linaro, bmasney,
	djakov
  Cc: linux-arm-msm, devicetree, linux-kernel, linux-pci, vireshk,
	quic_vbadigan, quic_skananth, quic_nitegupt, quic_parass,
	quic_krichai, krzysztof.kozlowski
In-Reply-To: <20240407-opp_support-v9-0-496184dc45d7@quicinc.com>

Add pcie-mem & cpu-pcie interconnect path to the PCIe nodes.

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sm8450.dtsi | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi
index b86be34a912b..615296e13c43 100644
--- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
@@ -1807,6 +1807,12 @@ pcie0: pcie@1c00000 {
 					<0 0 0 3 &intc 0 0 0 151 IRQ_TYPE_LEVEL_HIGH>, /* int_c */
 					<0 0 0 4 &intc 0 0 0 152 IRQ_TYPE_LEVEL_HIGH>; /* int_d */
 
+			interconnects = <&pcie_noc MASTER_PCIE_0 QCOM_ICC_TAG_ALWAYS
+					 &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
+					<&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ALWAYS
+					 &config_noc SLAVE_PCIE_0 QCOM_ICC_TAG_ALWAYS>;
+			interconnect-names = "pcie-mem", "cpu-pcie";
+
 			clocks = <&gcc GCC_PCIE_0_PIPE_CLK>,
 				 <&gcc GCC_PCIE_0_PIPE_CLK_SRC>,
 				 <&pcie0_phy>,
@@ -1930,6 +1936,12 @@ pcie1: pcie@1c08000 {
 					<0 0 0 3 &intc 0 0 0 438 IRQ_TYPE_LEVEL_HIGH>, /* int_c */
 					<0 0 0 4 &intc 0 0 0 439 IRQ_TYPE_LEVEL_HIGH>; /* int_d */
 
+			interconnects = <&pcie_noc MASTER_PCIE_1 QCOM_ICC_TAG_ALWAYS
+					 &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
+					<&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ALWAYS
+					 &config_noc SLAVE_PCIE_1 QCOM_ICC_TAG_ALWAYS>;
+			interconnect-names = "pcie-mem", "cpu-pcie";
+
 			clocks = <&gcc GCC_PCIE_1_PIPE_CLK>,
 				 <&gcc GCC_PCIE_1_PIPE_CLK_SRC>,
 				 <&pcie1_phy>,

-- 
2.42.0


^ permalink raw reply related

* [PATCH v9 0/6] PCI: qcom: Add support for OPP
From: Krishna chaitanya chundru @ 2024-04-07  4:37 UTC (permalink / raw)
  To: Bjorn Andersson, Konrad Dybcio, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Manivannan Sadhasivam, Lorenzo Pieralisi,
	Krzysztof Wilczyński, Bjorn Helgaas, johan+linaro, bmasney,
	djakov
  Cc: linux-arm-msm, devicetree, linux-kernel, linux-pci, vireshk,
	quic_vbadigan, quic_skananth, quic_nitegupt, quic_parass,
	quic_krichai, krzysztof.kozlowski, Bryan O'Donoghue

This patch adds support for OPP to vote for the performance state of RPMH
power domain based upon PCIe speed it got enumerated.

QCOM Resource Power Manager-hardened (RPMh) is a hardware block which
maintains hardware state of a regulator by performing max aggregation of
the requests made by all of the processors.

PCIe controller can operate on different RPMh performance state of power
domain based up on the speed of the link. And this performance state varies
from target to target.

It is manadate to scale the performance state based up on the PCIe speed
link operates so that SoC can run under optimum power conditions.

Add Operating Performance Points(OPP) support to vote for RPMh state based
upon GEN speed link is operating.

Before link up PCIe driver will vote for the maximum performance state.

As now we are adding ICC BW vote in OPP, the ICC BW voting depends both
GEN speed and link width using opp-level to indicate the opp entry table
will be difficult.

In PCIe certain gen speeds like GEN1x2 & GEN2X1 or GEN3x2 & GEN4x1 use
same icc bw if we use freq in the OPP table to represent the PCIe Gen
speed number of PCIe entries can reduced.

So going back to use freq in the OPP table instead of level.

Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
---
Changes from v8:
	- Removed the ack-by and reviewed by on dt-bindings as dt-bindings moved to new files.
	- Removed dt-binding patch for interconnects as it is added in the common file.
	- Added tags for interconnect as suggested by konrad
	- Added the comments as suggested by mani
	- In ICC BW vote for CPU to PCIe path if icc_disable() fails log error and return instead of re-init.
	- Link to v8: https://lore.kernel.org/linux-arm-msm/20240302-opp_support-v8-0-158285b86b10@quicinc.com/
Changes from v7:
	- Fix the compilation issue in patch3
	- Change the commit text and wrap the comments to 80 columns as suggested by bjorn
	- remove PCIE_MBS2FREQ macro as this is being used by only qcom drivers.
	- Link to v7: https://lore.kernel.org/r/20240223-opp_support-v7-0-10b4363d7e71@quicinc.com
Changes from v6:
	- change CPU-PCIe bandwidth to 1KBps as suggested by HW team.
	- Create a new API to get frequency based upon PCIe speed as suggested
	  by mani.
	- Updated few commit texts and comments.
	- Setting opp to NULL in suspend to remove any votes.
	- Link for v6: https://lore.kernel.org/linux-arm-msm/20240112-opp_support-v6-0-77bbf7d0cc37@quicinc.com/
Changes from v5:
	- Add ICC BW voting as part of OPP, rebase the latest kernel, and only
	- either OPP or ICC BW voting will supported we removed the patch to
	- return error for icc opp update patch.
	- As we added the icc bw voting in opp table I am not including reviewed
	- by tags given in previous patch.
	- Use opp freq to find opp entries as now we need to include pcie link
	- also in to considerations.
	- Add CPU-PCIe BW voting which is not present till now.
	- Drop  PCI: qcom: Return error from 'qcom_pcie_icc_update' as either opp or icc bw
	- only one executes and there is no need to fail if opp or icc update fails.
	- Link for v5: https://lore.kernel.org/linux-arm-msm/20231101063323.GH2897@thinkpad/T/
Changes from v4:
	- Added a separate patch for returning error from the qcom_pcie_upadate
	  and moved opp update logic to icc_update and used a bool variable to 
	  update the opp.
	- Addressed comments made by pavan.
changes from v3:
	- Removing the opp vote on suspend when the link is not up and link is not
	  up and add debug prints as suggested by pavan.
	- Added dev_pm_opp_find_level_floor API to find the highest opp to vote.
changes from v2:
	- Instead of using the freq based opp search use level based as suggested
	  by Dmitry Baryshkov.
Changes from v1:
        - Addressed comments from Krzysztof Kozlowski.
        - Added the rpmhpd_opp_xxx phandle as suggested by pavan.
        - Added dev_pm_opp_set_opp API call which was missed on previous patch.
---

---
Krishna chaitanya chundru (6):
      arm64: dts: qcom: sm8450: Add interconnect path to PCIe node
      PCI: qcom: Add ICC bandwidth vote for CPU to PCIe path
      dt-bindings: pci: qcom: Add opp table
      arm64: dts: qcom: sm8450: Add opp table support to PCIe
      PCI: Bring the PCIe speed to MBps logic to new pcie_link_speed_to_mbps()
      PCI: qcom: Add OPP support to scale performance state of power domain

 .../devicetree/bindings/pci/qcom,pcie-sm8450.yaml  |   4 +
 arch/arm64/boot/dts/qcom/sm8450.dtsi               |  89 +++++++++++++++++
 drivers/pci/controller/dwc/pcie-qcom.c             | 108 +++++++++++++++++----
 drivers/pci/pci.c                                  |  19 +---
 drivers/pci/pci.h                                  |  22 +++++
 5 files changed, 207 insertions(+), 35 deletions(-)
---
base-commit: 6c6e47d69d821047097909288b6d7f1aafb3b9b1
change-id: 20240406-opp_support-ca095eb032b4

Best regards,
-- 
Krishna chaitanya chundru <quic_krichai@quicinc.com>


^ permalink raw reply

* RE: [PATCH v8 4/4] pinctrl: Implementation of the generic scmi-pinctrl driver
From: Peng Fan @ 2024-04-07  3:17 UTC (permalink / raw)
  To: Peng Fan (OSS), Sudeep Holla, Cristian Marussi, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Linus Walleij, Dan Carpenter
  Cc: Andy Shevchenko, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
	linux-gpio@vger.kernel.org, Oleksii Moisieiev
In-Reply-To: <20240405-pinctrl-scmi-v8-4-5fc8e33871bf@nxp.com>

> Subject: [PATCH v8 4/4] pinctrl: Implementation of the generic scmi-pinctrl
> driver
> 
> From: Peng Fan <peng.fan@nxp.com>
> 
> scmi-pinctrl driver implements pinctrl driver interface and using SCMI
> protocol to redirect messages from pinctrl subsystem SDK to SCMI platform
> firmware, which does the changes in HW.

Considering pinctrl expects ENOTSUPP, so I would add this in v9.

diff --git a/drivers/pinctrl/pinctrl-scmi.c b/drivers/pinctrl/pinctrl-scmi.c
index 0f55f000a679..accebe3f0706 100644
--- a/drivers/pinctrl/pinctrl-scmi.c
+++ b/drivers/pinctrl/pinctrl-scmi.c
@@ -296,8 +296,12 @@ static int pinctrl_scmi_pinconf_get(struct pinctrl_dev *pctldev,
 
        ret = pinctrl_ops->settings_get_one(pmx->ph, pin, PIN_TYPE, type,
                                            &config_value);
-       if (ret)
+       if (ret) {
+               /* Convert SCMI error code to PINCTRL expected error code */
+               if (ret == -EOPNOTSUPP)
+                       ret = -ENOTSUPP;
                return ret;
+       }
 
        *config = pinconf_to_config_packed(config_type, config_value);
 
@@ -445,8 +449,12 @@ static int pinctrl_scmi_pinconf_group_get(struct pinctrl_dev *pctldev,
 
        ret = pinctrl_ops->settings_get_one(pmx->ph, group, GROUP_TYPE, type,
                                            &config_value);
-       if (ret)
+       if (ret) {
+               /* Convert SCMI error code to PINCTRL expected error code */
+               if (ret == -EOPNOTSUPP)
+                       ret = -ENOTSUPP;
                return ret;
+       }
 
        *config = pinconf_to_config_packed(config_type, config_value);

I will collect more comments before post v9. Just post out upper diff
to see any disagreements.

Thanks,
Peng.
> 
> Co-developed-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com>
> Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  MAINTAINERS                    |   1 +
>  drivers/pinctrl/Kconfig        |  11 +
>  drivers/pinctrl/Makefile       |   1 +
>  drivers/pinctrl/pinctrl-scmi.c | 564
> +++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 577 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 4b511a55101c..d8270ac6651a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -21457,6 +21457,7 @@ F:	drivers/cpufreq/sc[mp]i-cpufreq.c
>  F:	drivers/firmware/arm_scmi/
>  F:	drivers/firmware/arm_scpi.c
>  F:	drivers/hwmon/scmi-hwmon.c
> +F:	drivers/pinctrl/pinctrl-scmi.c
>  F:	drivers/pmdomain/arm/
>  F:	drivers/powercap/arm_scmi_powercap.c
>  F:	drivers/regulator/scmi-regulator.c
> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig index
> d45657aa986a..4e6f65cf0e76 100644
> --- a/drivers/pinctrl/Kconfig
> +++ b/drivers/pinctrl/Kconfig
> @@ -450,6 +450,17 @@ config PINCTRL_ROCKCHIP
>  	help
>            This support pinctrl and GPIO driver for Rockchip SoCs.
> 
> +config PINCTRL_SCMI
> +	tristate "Pinctrl driver using SCMI protocol interface"
> +	depends on ARM_SCMI_PROTOCOL || COMPILE_TEST
> +	select PINMUX
> +	select GENERIC_PINCONF
> +	help
> +	  This driver provides support for pinctrl which is controlled
> +	  by firmware that implements the SCMI interface.
> +	  It uses SCMI Message Protocol to interact with the
> +	  firmware providing all the pinctrl controls.
> +
>  config PINCTRL_SINGLE
>  	tristate "One-register-per-pin type device tree based pinctrl driver"
>  	depends on OF
> diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile index
> 2152539b53d5..cc809669405a 100644
> --- a/drivers/pinctrl/Makefile
> +++ b/drivers/pinctrl/Makefile
> @@ -45,6 +45,7 @@ obj-$(CONFIG_PINCTRL_PIC32)	+= pinctrl-pic32.o
>  obj-$(CONFIG_PINCTRL_PISTACHIO)	+= pinctrl-pistachio.o
>  obj-$(CONFIG_PINCTRL_RK805)	+= pinctrl-rk805.o
>  obj-$(CONFIG_PINCTRL_ROCKCHIP)	+= pinctrl-rockchip.o
> +obj-$(CONFIG_PINCTRL_SCMI)	+= pinctrl-scmi.o
>  obj-$(CONFIG_PINCTRL_SINGLE)	+= pinctrl-single.o
>  obj-$(CONFIG_PINCTRL_ST) 	+= pinctrl-st.o
>  obj-$(CONFIG_PINCTRL_STMFX) 	+= pinctrl-stmfx.o
> diff --git a/drivers/pinctrl/pinctrl-scmi.c b/drivers/pinctrl/pinctrl-scmi.c new
> file mode 100644 index 000000000000..0f55f000a679
> --- /dev/null
> +++ b/drivers/pinctrl/pinctrl-scmi.c
> @@ -0,0 +1,564 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * System Control and Power Interface (SCMI) Protocol based pinctrl
> +driver
> + *
> + * Copyright (C) 2024 EPAM
> + * Copyright 2024 NXP
> + */
> +
> +#include <linux/device.h>
> +#include <linux/dev_printk.h>
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/scmi_protocol.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +
> +#include <linux/pinctrl/machine.h>
> +#include <linux/pinctrl/pinconf.h>
> +#include <linux/pinctrl/pinconf-generic.h> #include
> +<linux/pinctrl/pinctrl.h> #include <linux/pinctrl/pinmux.h>
> +
> +#include "pinctrl-utils.h"
> +#include "core.h"
> +#include "pinconf.h"
> +
> +#define DRV_NAME "scmi-pinctrl"
> +
> +/* Define num configs, if not large than 4 use stack, else use kcalloc */
> +#define SCMI_NUM_CONFIGS	4
> +
> +static const struct scmi_pinctrl_proto_ops *pinctrl_ops;
> +
> +struct scmi_pinctrl {
> +	struct device *dev;
> +	struct scmi_protocol_handle *ph;
> +	struct pinctrl_dev *pctldev;
> +	struct pinctrl_desc pctl_desc;
> +	struct pinfunction *functions;
> +	unsigned int nr_functions;
> +	struct pinctrl_pin_desc *pins;
> +	unsigned int nr_pins;
> +};
> +
> +static int pinctrl_scmi_get_groups_count(struct pinctrl_dev *pctldev) {
> +	struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> +	return pinctrl_ops->count_get(pmx->ph, GROUP_TYPE); }
> +
> +static const char *pinctrl_scmi_get_group_name(struct pinctrl_dev *pctldev,
> +					       unsigned int selector)
> +{
> +	int ret;
> +	const char *name;
> +	struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> +	ret = pinctrl_ops->name_get(pmx->ph, selector, GROUP_TYPE,
> &name);
> +	if (ret) {
> +		dev_err(pmx->dev, "get name failed with err %d", ret);
> +		return NULL;
> +	}
> +
> +	return name;
> +}
> +
> +static int pinctrl_scmi_get_group_pins(struct pinctrl_dev *pctldev,
> +				       unsigned int selector,
> +				       const unsigned int **pins,
> +				       unsigned int *num_pins)
> +{
> +	struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> +	return pinctrl_ops->group_pins_get(pmx->ph, selector, pins,
> num_pins);
> +}
> +
> +static const struct pinctrl_ops pinctrl_scmi_pinctrl_ops = {
> +	.get_groups_count = pinctrl_scmi_get_groups_count,
> +	.get_group_name = pinctrl_scmi_get_group_name,
> +	.get_group_pins = pinctrl_scmi_get_group_pins, #ifdef CONFIG_OF
> +	.dt_node_to_map = pinconf_generic_dt_node_to_map_all,
> +	.dt_free_map = pinconf_generic_dt_free_map, #endif };
> +
> +static int pinctrl_scmi_get_functions_count(struct pinctrl_dev
> +*pctldev) {
> +	struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> +	return pinctrl_ops->count_get(pmx->ph, FUNCTION_TYPE); }
> +
> +static const char *pinctrl_scmi_get_function_name(struct pinctrl_dev
> *pctldev,
> +						  unsigned int selector)
> +{
> +	int ret;
> +	const char *name;
> +	struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> +	ret = pinctrl_ops->name_get(pmx->ph, selector, FUNCTION_TYPE,
> &name);
> +	if (ret) {
> +		dev_err(pmx->dev, "get name failed with err %d", ret);
> +		return NULL;
> +	}
> +
> +	return name;
> +}
> +
> +static int pinctrl_scmi_get_function_groups(struct pinctrl_dev *pctldev,
> +					    unsigned int selector,
> +					    const char * const **p_groups,
> +					    unsigned int * const
> p_num_groups) {
> +	struct pinfunction *func;
> +	const unsigned int *group_ids;
> +	unsigned int num_groups;
> +	const char **groups;
> +	int ret, i;
> +	struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> +	if (!p_groups || !p_num_groups)
> +		return -EINVAL;
> +
> +	if (selector >= pmx->nr_functions)
> +		return -EINVAL;
> +
> +	func = &pmx->functions[selector];
> +	if (func->ngroups)
> +		goto done;
> +
> +	ret = pinctrl_ops->function_groups_get(pmx->ph, selector,
> &num_groups,
> +					       &group_ids);
> +	if (ret) {
> +		dev_err(pmx->dev, "Unable to get function groups, err %d",
> ret);
> +		return ret;
> +	}
> +	if (!num_groups)
> +		return -EINVAL;
> +
> +	groups = kcalloc(num_groups, sizeof(*groups), GFP_KERNEL);
> +	if (!groups)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < num_groups; i++) {
> +		groups[i] = pinctrl_scmi_get_group_name(pctldev,
> group_ids[i]);
> +		if (!groups[i]) {
> +			ret = -EINVAL;
> +			goto err_free;
> +		}
> +	}
> +
> +	func->ngroups = num_groups;
> +	func->groups = groups;
> +done:
> +	*p_groups = func->groups;
> +	*p_num_groups = func->ngroups;
> +
> +	return 0;
> +
> +err_free:
> +	kfree(groups);
> +
> +	return ret;
> +}
> +
> +static int pinctrl_scmi_func_set_mux(struct pinctrl_dev *pctldev,
> +				     unsigned int selector, unsigned int group)
> {
> +	struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> +	return pinctrl_ops->mux_set(pmx->ph, selector, group); }
> +
> +static int pinctrl_scmi_request(struct pinctrl_dev *pctldev,
> +				unsigned int offset)
> +{
> +	struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> +	return pinctrl_ops->pin_request(pmx->ph, offset); }
> +
> +static int pinctrl_scmi_free(struct pinctrl_dev *pctldev, unsigned int
> +offset) {
> +	struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> +	return pinctrl_ops->pin_free(pmx->ph, offset); }
> +
> +static const struct pinmux_ops pinctrl_scmi_pinmux_ops = {
> +	.request = pinctrl_scmi_request,
> +	.free = pinctrl_scmi_free,
> +	.get_functions_count = pinctrl_scmi_get_functions_count,
> +	.get_function_name = pinctrl_scmi_get_function_name,
> +	.get_function_groups = pinctrl_scmi_get_function_groups,
> +	.set_mux = pinctrl_scmi_func_set_mux,
> +};
> +
> +static int pinctrl_scmi_map_pinconf_type(enum pin_config_param param,
> +					 enum scmi_pinctrl_conf_type
> *type) {
> +	u32 arg = param;
> +
> +	switch (arg) {
> +	case PIN_CONFIG_BIAS_BUS_HOLD:
> +		*type = SCMI_PIN_BIAS_BUS_HOLD;
> +		break;
> +	case PIN_CONFIG_BIAS_DISABLE:
> +		*type = SCMI_PIN_BIAS_DISABLE;
> +		break;
> +	case PIN_CONFIG_BIAS_HIGH_IMPEDANCE:
> +		*type = SCMI_PIN_BIAS_HIGH_IMPEDANCE;
> +		break;
> +	case PIN_CONFIG_BIAS_PULL_DOWN:
> +		*type = SCMI_PIN_BIAS_PULL_DOWN;
> +		break;
> +	case PIN_CONFIG_BIAS_PULL_PIN_DEFAULT:
> +		*type = SCMI_PIN_BIAS_PULL_DEFAULT;
> +		break;
> +	case PIN_CONFIG_BIAS_PULL_UP:
> +		*type = SCMI_PIN_BIAS_PULL_UP;
> +		break;
> +	case PIN_CONFIG_DRIVE_OPEN_DRAIN:
> +		*type = SCMI_PIN_DRIVE_OPEN_DRAIN;
> +		break;
> +	case PIN_CONFIG_DRIVE_OPEN_SOURCE:
> +		*type = SCMI_PIN_DRIVE_OPEN_SOURCE;
> +		break;
> +	case PIN_CONFIG_DRIVE_PUSH_PULL:
> +		*type = SCMI_PIN_DRIVE_PUSH_PULL;
> +		break;
> +	case PIN_CONFIG_DRIVE_STRENGTH:
> +		*type = SCMI_PIN_DRIVE_STRENGTH;
> +		break;
> +	case PIN_CONFIG_DRIVE_STRENGTH_UA:
> +		*type = SCMI_PIN_DRIVE_STRENGTH;
> +		break;
> +	case PIN_CONFIG_INPUT_DEBOUNCE:
> +		*type = SCMI_PIN_INPUT_DEBOUNCE;
> +		break;
> +	case PIN_CONFIG_INPUT_ENABLE:
> +		*type = SCMI_PIN_INPUT_MODE;
> +		break;
> +	case PIN_CONFIG_INPUT_SCHMITT:
> +		*type = SCMI_PIN_INPUT_SCHMITT;
> +		break;
> +	case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
> +		*type = SCMI_PIN_INPUT_MODE;
> +		break;
> +	case PIN_CONFIG_MODE_LOW_POWER:
> +		*type = SCMI_PIN_LOW_POWER_MODE;
> +		break;
> +	case PIN_CONFIG_OUTPUT:
> +		*type = SCMI_PIN_OUTPUT_VALUE;
> +		break;
> +	case PIN_CONFIG_OUTPUT_ENABLE:
> +		*type = SCMI_PIN_OUTPUT_MODE;
> +		break;
> +	case PIN_CONFIG_OUTPUT_IMPEDANCE_OHMS:
> +		*type = SCMI_PIN_OUTPUT_VALUE;
> +		break;
> +	case PIN_CONFIG_POWER_SOURCE:
> +		*type = SCMI_PIN_POWER_SOURCE;
> +		break;
> +	case PIN_CONFIG_SLEW_RATE:
> +		*type = SCMI_PIN_SLEW_RATE;
> +		break;
> +	case SCMI_PIN_OEM_START ... SCMI_PIN_OEM_END:
> +		*type = arg;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int pinctrl_scmi_pinconf_get(struct pinctrl_dev *pctldev,
> +				    unsigned int pin, unsigned long *config) {
> +	int ret;
> +	struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
> +	enum pin_config_param config_type;
> +	enum scmi_pinctrl_conf_type type;
> +	u32 config_value;
> +
> +	if (!config)
> +		return -EINVAL;
> +
> +	config_type = pinconf_to_config_param(*config);
> +
> +	ret = pinctrl_scmi_map_pinconf_type(config_type, &type);
> +	if (ret)
> +		return ret;
> +
> +	ret = pinctrl_ops->settings_get_one(pmx->ph, pin, PIN_TYPE, type,
> +					    &config_value);
> +	if (ret)
> +		return ret;
> +
> +	*config = pinconf_to_config_packed(config_type, config_value);
> +
> +	return 0;
> +}
> +
> +static int
> +pinctrl_scmi_alloc_configs(struct pinctrl_dev *pctldev, u32 num_configs,
> +			   u32 **p_config_value,
> +			   enum scmi_pinctrl_conf_type **p_config_type) {
> +	if (num_configs <= SCMI_NUM_CONFIGS)
> +		return 0;
> +
> +	*p_config_value = kcalloc(num_configs, sizeof(**p_config_value),
> GFP_KERNEL);
> +	if (!*p_config_value)
> +		return -ENOMEM;
> +
> +	*p_config_type = kcalloc(num_configs, sizeof(**p_config_type),
> GFP_KERNEL);
> +	if (!*p_config_type) {
> +		kfree(*p_config_value);
> +		return -ENOMEM;
> +	}
> +
> +	return 0;
> +}
> +
> +static void
> +pinctrl_scmi_free_configs(struct pinctrl_dev *pctldev, u32 num_configs,
> +			  u32 **p_config_value,
> +			  enum scmi_pinctrl_conf_type **p_config_type) {
> +	if (num_configs <= SCMI_NUM_CONFIGS)
> +		return;
> +
> +	kfree(*p_config_value);
> +	kfree(*p_config_type);
> +}
> +
> +static int pinctrl_scmi_pinconf_set(struct pinctrl_dev *pctldev,
> +				    unsigned int pin,
> +				    unsigned long *configs,
> +				    unsigned int num_configs)
> +{
> +	int i, ret;
> +	struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
> +	enum scmi_pinctrl_conf_type config_type[SCMI_NUM_CONFIGS];
> +	u32 config_value[SCMI_NUM_CONFIGS];
> +	enum scmi_pinctrl_conf_type *p_config_type = config_type;
> +	u32 *p_config_value = config_value;
> +	enum pin_config_param param;
> +
> +	if (!configs || !num_configs)
> +		return -EINVAL;
> +
> +	ret = pinctrl_scmi_alloc_configs(pctldev, num_configs,
> &p_config_type,
> +					 &p_config_value);
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < num_configs; i++) {
> +		param = pinconf_to_config_param(configs[i]);
> +		ret = pinctrl_scmi_map_pinconf_type(param,
> &p_config_type[i]);
> +		if (ret) {
> +			dev_err(pmx->dev, "Error map pinconf_type %d\n",
> ret);
> +			goto free_config;
> +		}
> +		p_config_value[i] = pinconf_to_config_argument(configs[i]);
> +	}
> +
> +	ret = pinctrl_ops->settings_conf(pmx->ph, pin, PIN_TYPE,
> num_configs,
> +					 p_config_type,  p_config_value);
> +	if (ret)
> +		dev_err(pmx->dev, "Error parsing config %d\n", ret);
> +
> +free_config:
> +	pinctrl_scmi_free_configs(pctldev, num_configs, &p_config_type,
> +				  &p_config_value);
> +	return ret;
> +}
> +
> +static int pinctrl_scmi_pinconf_group_set(struct pinctrl_dev *pctldev,
> +					  unsigned int group,
> +					  unsigned long *configs,
> +					  unsigned int num_configs)
> +{
> +	int i, ret;
> +	struct scmi_pinctrl *pmx =  pinctrl_dev_get_drvdata(pctldev);
> +	enum scmi_pinctrl_conf_type config_type[SCMI_NUM_CONFIGS];
> +	u32 config_value[SCMI_NUM_CONFIGS];
> +	enum scmi_pinctrl_conf_type *p_config_type = config_type;
> +	u32 *p_config_value = config_value;
> +	enum pin_config_param param;
> +
> +	if (!configs || !num_configs)
> +		return -EINVAL;
> +
> +	ret = pinctrl_scmi_alloc_configs(pctldev, num_configs,
> &p_config_type,
> +					 &p_config_value);
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < num_configs; i++) {
> +		param = pinconf_to_config_param(configs[i]);
> +		ret = pinctrl_scmi_map_pinconf_type(param,
> &p_config_type[i]);
> +		if (ret) {
> +			dev_err(pmx->dev, "Error map pinconf_type %d\n",
> ret);
> +			goto free_config;
> +		}
> +
> +		p_config_value[i] = pinconf_to_config_argument(configs[i]);
> +	}
> +
> +	ret = pinctrl_ops->settings_conf(pmx->ph, group, GROUP_TYPE,
> +					 num_configs, p_config_type,
> +					 p_config_value);
> +	if (ret)
> +		dev_err(pmx->dev, "Error parsing config %d", ret);
> +
> +free_config:
> +	pinctrl_scmi_free_configs(pctldev, num_configs, &p_config_type,
> +				  &p_config_value);
> +	return ret;
> +};
> +
> +static int pinctrl_scmi_pinconf_group_get(struct pinctrl_dev *pctldev,
> +					  unsigned int group,
> +					  unsigned long *config)
> +{
> +	int ret;
> +	struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
> +	enum pin_config_param config_type;
> +	enum scmi_pinctrl_conf_type type;
> +	u32 config_value;
> +
> +	if (!config)
> +		return -EINVAL;
> +
> +	config_type = pinconf_to_config_param(*config);
> +	ret = pinctrl_scmi_map_pinconf_type(config_type, &type);
> +	if (ret) {
> +		dev_err(pmx->dev, "Error map pinconf_type %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = pinctrl_ops->settings_get_one(pmx->ph, group, GROUP_TYPE,
> type,
> +					    &config_value);
> +	if (ret)
> +		return ret;
> +
> +	*config = pinconf_to_config_packed(config_type, config_value);
> +
> +	return 0;
> +}
> +
> +static const struct pinconf_ops pinctrl_scmi_pinconf_ops = {
> +	.is_generic = true,
> +	.pin_config_get = pinctrl_scmi_pinconf_get,
> +	.pin_config_set = pinctrl_scmi_pinconf_set,
> +	.pin_config_group_set = pinctrl_scmi_pinconf_group_set,
> +	.pin_config_group_get = pinctrl_scmi_pinconf_group_get,
> +	.pin_config_config_dbg_show = pinconf_generic_dump_config, };
> +
> +static int pinctrl_scmi_get_pins(struct scmi_pinctrl *pmx,
> +				 struct pinctrl_desc *desc)
> +{
> +	struct pinctrl_pin_desc *pins;
> +	unsigned int npins;
> +	int ret, i;
> +
> +	npins = pinctrl_ops->count_get(pmx->ph, PIN_TYPE);
> +	/*
> +	 * npins will never be zero, the scmi pinctrl driver has bailed out
> +	 * if npins is zero.
> +	 */
> +	pins = devm_kmalloc_array(pmx->dev, npins, sizeof(*pins),
> GFP_KERNEL);
> +	if (!pins)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < npins; i++) {
> +		pins[i].number = i;
> +		/*
> +		 * The memory for name is handled by the scmi firmware
> driver,
> +		 * no need free here
> +		 */
> +		ret = pinctrl_ops->name_get(pmx->ph, i, PIN_TYPE,
> &pins[i].name);
> +		if (ret)
> +			return dev_err_probe(pmx->dev, ret,
> +					     "Can't get name for pin %d", i);
> +	}
> +
> +	desc->npins = npins;
> +	desc->pins = pins;
> +	dev_dbg(pmx->dev, "got pins %u", npins);
> +
> +	return 0;
> +}
> +
> +static int scmi_pinctrl_probe(struct scmi_device *sdev) {
> +	int ret;
> +	struct device *dev = &sdev->dev;
> +	struct scmi_pinctrl *pmx;
> +	const struct scmi_handle *handle;
> +	struct scmi_protocol_handle *ph;
> +
> +	if (!sdev->handle)
> +		return -EINVAL;
> +
> +	handle = sdev->handle;
> +
> +	pinctrl_ops = handle->devm_protocol_get(sdev,
> SCMI_PROTOCOL_PINCTRL, &ph);
> +	if (IS_ERR(pinctrl_ops))
> +		return PTR_ERR(pinctrl_ops);
> +
> +	pmx = devm_kzalloc(dev, sizeof(*pmx), GFP_KERNEL);
> +	if (!pmx)
> +		return -ENOMEM;
> +
> +	pmx->ph = ph;
> +
> +	pmx->dev = dev;
> +	pmx->pctl_desc.name = DRV_NAME;
> +	pmx->pctl_desc.owner = THIS_MODULE;
> +	pmx->pctl_desc.pctlops = &pinctrl_scmi_pinctrl_ops;
> +	pmx->pctl_desc.pmxops = &pinctrl_scmi_pinmux_ops;
> +	pmx->pctl_desc.confops = &pinctrl_scmi_pinconf_ops;
> +
> +	ret = pinctrl_scmi_get_pins(pmx, &pmx->pctl_desc);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_pinctrl_register_and_init(dev, &pmx->pctl_desc, pmx,
> +					     &pmx->pctldev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to register pinctrl\n");
> +
> +	pmx->nr_functions = pinctrl_scmi_get_functions_count(pmx-
> >pctldev);
> +	pmx->functions = devm_kcalloc(dev, pmx->nr_functions,
> +				      sizeof(*pmx->functions), GFP_KERNEL);
> +	if (!pmx->functions)
> +		return -ENOMEM;
> +
> +	return pinctrl_enable(pmx->pctldev);
> +}
> +
> +static const struct scmi_device_id scmi_id_table[] = {
> +	{ SCMI_PROTOCOL_PINCTRL, "pinctrl" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(scmi, scmi_id_table);
> +
> +static struct scmi_driver scmi_pinctrl_driver = {
> +	.name = DRV_NAME,
> +	.probe = scmi_pinctrl_probe,
> +	.id_table = scmi_id_table,
> +};
> +module_scmi_driver(scmi_pinctrl_driver);
> +
> +MODULE_AUTHOR("Oleksii Moisieiev <oleksii_moisieiev@epam.com>");
> +MODULE_AUTHOR("Peng Fan <peng.fan@nxp.com>");
> MODULE_DESCRIPTION("ARM
> +SCMI pin controller driver"); MODULE_LICENSE("GPL");
> 
> --
> 2.37.1


^ permalink raw reply related

* RE: [PATCH v2 2/6] dt-bindings: firmware: add i.MX SCMI Extension protocol
From: Peng Fan @ 2024-04-07  1:50 UTC (permalink / raw)
  To: Krzysztof Kozlowski, Peng Fan (OSS), Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, Sudeep Holla,
	Cristian Marussi
  Cc: devicetree@vger.kernel.org, imx@lists.linux.dev,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org
In-Reply-To: <DU0PR04MB941719A18C3F749E7D180FD888012@DU0PR04MB9417.eurprd04.prod.outlook.com>

> Subject: RE: [PATCH v2 2/6] dt-bindings: firmware: add i.MX SCMI Extension
> protocol
> 
> > Subject: Re: [PATCH v2 2/6] dt-bindings: firmware: add i.MX SCMI
> > Extension protocol
> >
> > On 05/04/2024 14:39, Peng Fan (OSS) wrote:
> > > From: Peng Fan <peng.fan@nxp.com>
> > >
> > > Add i.MX SCMI Extension protocols bindings for:
> > >  - Battery Backed Secure Module(BBSM)
> >
> > Which is what?
> 
> I should say BBM(BBSM + BBNSM), BBM has RTC and ON/OFF key features,
> but BBM is managed by SCMI firmware and exported to agent by BBM
> protocol. So add bindings for i.MX BBM protocol.
> 
> Is this ok?
> 
> >
> > >  - MISC settings such as General Purpose Registers settings.
> > >
> > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > ---
> > >  .../devicetree/bindings/firmware/imx,scmi.yaml     | 80
> > ++++++++++++++++++++++
> > >  1 file changed, 80 insertions(+)
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/firmware/imx,scmi.yaml
> > > b/Documentation/devicetree/bindings/firmware/imx,scmi.yaml
> > > new file mode 100644
> > > index 000000000000..7ee19a661d83
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/firmware/imx,scmi.yaml
> > > @@ -0,0 +1,80 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) # Copyright
> > > +2024 NXP %YAML 1.2
> > > +---
> > > +$id:
> > > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fde
> > > +vi
> > >
> >
> +cetree.org%2Fschemas%2Ffirmware%2Fimx%2Cscmi.yaml%23&data=05%7
> > C02%7Cp
> > >
> >
> +eng.fan%40nxp.com%7C5d16781d3eca425a342508dc562910b7%7C686ea
> > 1d3bc2b4c
> > >
> > +6fa92cd99c5c301635%7C0%7C0%7C638479981570959816%7CUnknown%
> > 7CTWFpbGZsb
> > >
> >
> +3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn
> > 0%3D
> > >
> >
> +%7C0%7C%7C%7C&sdata=mWNwPvu2eyF18MroVOBHb%2Fjeo%2BIHfV5V
> > h%2F9ebdx65MM
> > > +%3D&reserved=0
> > > +$schema:
> > > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fde
> > > +vi
> > > +cetree.org%2Fmeta-
> > schemas%2Fcore.yaml%23&data=05%7C02%7Cpeng.fan%40nx
> > >
> >
> +p.com%7C5d16781d3eca425a342508dc562910b7%7C686ea1d3bc2b4c6fa
> > 92cd99c5c
> > >
> >
> +301635%7C0%7C0%7C638479981570971949%7CUnknown%7CTWFpbGZs
> > b3d8eyJWIjoiM
> > >
> >
> +C4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7
> > C%7C%7
> > >
> >
> +C&sdata=v4XnGG00D4I8j5MJvDUVYMRTm7yRrvz0V3fUyc5KAAA%3D&reser
> > ved=0
> > > +
> > > +title: i.MX System Control and Management Interface(SCMI) Vendor
> > > +Protocols Extension
> > > +
> > > +maintainers:
> > > +  - Peng Fan <peng.fan@nxp.com>
> > > +
> > > +allOf:
> > > +  - $ref: arm,scmi.yaml#
> >
> > Sorry, but arm,scmi is a final schema. Is your plan to define some
> > common part?
> 
> No. I just wanna add vendor extension per SCMI spec.
> 
> 0x80-0xFF:
> Reserved for vendor or platform-specific extensions to this interface
> 
> Each vendor may have different usage saying id 0x81, so I add i.MX dt-
> schema file.
> 
> >
> > > +
> > > +properties:
> > > +  protocol@81:
> > > +    $ref: 'arm,scmi.yaml#/$defs/protocol-node'
> > > +    unevaluatedProperties: false
> > > +    description:
> > > +      The BBM Protocol is for managing Battery Backed Secure Module
> > (BBSM) RTC
> > > +      and the ON/OFF Key
> > > +
> > > +    properties:
> > > +      reg:
> > > +        const: 0x81
> > > +
> > > +    required:
> > > +      - reg
> > > +
> > > +  protocol@84:
> > > +    $ref: 'arm,scmi.yaml#/$defs/protocol-node'
> > > +    unevaluatedProperties: false
> > > +    description:
> > > +      The MISC Protocol is for managing SoC Misc settings, such as
> > > + GPR settings
> >
> > Genera register is not a setting... this is a pleonasm. Please be more
> > specific what is the GPR, MISC protocol etc.
> 
> The MISC Protocol is for managing SoC Misc settings, such as SAI MCLK/MQS
> in Always On domain BLK CTRL,  SAI_CLK_SEL in WAKEUP BLK CTRL, gpio
> expanders which is under control of SCMI firmware.
> 
> > > +
> > > +    properties:
> > > +      reg:
> > > +        const: 0x84
> > > +
> > > +      wakeup-sources:
> > > +        description:
> > > +          Each entry consists of 2 integers, represents the source
> > > + and electric signal edge
> >
> > Can you answer questions from reviewers?
> 
> Sorry. Is this ok?
> minItems: 1
> maxItems: 32
> 
> >
> > > +        items:
> > > +          items:
> > > +            - description: the wakeup source
> > > +            - description: the wakeup electric signal edge
> > > +        $ref: /schemas/types.yaml#/definitions/uint32-matrix
> > > +
> > > +    required:
> > > +      - reg
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +    firmware {
> > > +        scmi {
> > > +            compatible = "arm,scmi";
> >
> > > +            mboxes = <&mu2 5 0>, <&mu2 3 0>, <&mu2 3 1>;
> > > +            shmem = <&scmi_buf0>, <&scmi_buf1>;
> > > +
> > > +            #address-cells = <1>;
> > > +            #size-cells = <0>;
> > > +
> > > +            protocol@81 {
> > > +                reg = <0x81>;
> > > +            };
> > > +
> > > +            protocol@84 {
> > > +                reg = <0x84>;
> > > +                wakeup-sources = <0x8000 1
> > > +                                  0x8001 1
> > > +                                  0x8002 1
> > > +                                  0x8003 1
> > > +                                  0x8004 1>;
> >
> > Nothing improved... If you are going to ignore reviews, then you will
> > only get NAKed.
> 
> Sorry, you mean the examples, or the whole dt-schema?

Missed Rob's comment, will use
< > for each entry.

Thanks,
Peng.

> 
> Thanks,
> Peng.
> >
> > Best regards,
> > Krzysztof


^ permalink raw reply

* RE: [PATCH v2 4/6] firmware: arm_scmi: add initial support for i.MX MISC protocol
From: Peng Fan @ 2024-04-07  1:03 UTC (permalink / raw)
  To: Marco Felsch, Peng Fan (OSS)
  Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Shawn Guo,
	Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
	Sudeep Holla, Cristian Marussi, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, imx@lists.linux.dev
In-Reply-To: <20240405164414.t3fqhnwudxxt2ozv@pengutronix.de>

> Subject: Re: [PATCH v2 4/6] firmware: arm_scmi: add initial support for i.MX
> MISC protocol
> 
> Hi Peng,
> 
> On 24-04-05, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > The i.MX MISC protocol is for misc settings, such as gpio expander
> > wakeup.
> 
> Can you elaborate a bit more please?

The gpio expander is under M33(SCMI firmware used core) I2C control,
But the gpio expander supports board function such as PCIE_WAKEUP,
BTN_WAKEUP. So these are managed by MISC protocol.

SAI_CLK_MSEL in WAKEUP BLK CTRL is also managed by MISC Protocol.

And etc...

I will add more info in commit log in next version later, after I get more
reviews on the patchset.

Thanks,
Peng.

> 
> Regards,
>   Marco
> 
> 
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  drivers/firmware/arm_scmi/Kconfig       |  10 ++
> >  drivers/firmware/arm_scmi/Makefile      |   1 +
> >  drivers/firmware/arm_scmi/imx-sm-misc.c | 305
> ++++++++++++++++++++++++++++++++
> >  include/linux/scmi_imx_protocol.h       |  17 ++
> >  4 files changed, 333 insertions(+)
> >
> > diff --git a/drivers/firmware/arm_scmi/Kconfig
> > b/drivers/firmware/arm_scmi/Kconfig
> > index 56d11c9d9f47..bfeae92f6420 100644
> > --- a/drivers/firmware/arm_scmi/Kconfig
> > +++ b/drivers/firmware/arm_scmi/Kconfig
> > @@ -191,3 +191,13 @@ config IMX_SCMI_BBM_EXT
> >  	  and BUTTON.
> >
> >  	  This driver can also be built as a module.
> > +
> > +config IMX_SCMI_MISC_EXT
> > +	tristate "i.MX SCMI MISC EXTENSION"
> > +	depends on ARM_SCMI_PROTOCOL || (COMPILE_TEST && OF)
> > +	default y if ARCH_MXC
> > +	help
> > +	  This enables i.MX System MISC control logic such as gpio expander
> > +	  wakeup
> > +
> > +	  This driver can also be built as a module.
> > diff --git a/drivers/firmware/arm_scmi/Makefile
> > b/drivers/firmware/arm_scmi/Makefile
> > index 327687acf857..a23fde721222 100644
> > --- a/drivers/firmware/arm_scmi/Makefile
> > +++ b/drivers/firmware/arm_scmi/Makefile
> > @@ -12,6 +12,7 @@ scmi-transport-
> $(CONFIG_ARM_SCMI_TRANSPORT_VIRTIO)
> > += virtio.o
> >  scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_OPTEE) += optee.o
> > scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o
> > system.o voltage.o powercap.o
> >  scmi-protocols-$(CONFIG_IMX_SCMI_BBM_EXT) += imx-sm-bbm.o
> > +scmi-protocols-$(CONFIG_IMX_SCMI_MISC_EXT) += imx-sm-misc.o
> >  scmi-module-objs := $(scmi-driver-y) $(scmi-protocols-y)
> > $(scmi-transport-y)
> >
> >  obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-core.o diff --git
> > a/drivers/firmware/arm_scmi/imx-sm-misc.c
> > b/drivers/firmware/arm_scmi/imx-sm-misc.c
> > new file mode 100644
> > index 000000000000..1b0ec2281518
> > --- /dev/null
> > +++ b/drivers/firmware/arm_scmi/imx-sm-misc.c
> > @@ -0,0 +1,305 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * System control and Management Interface (SCMI) NXP MISC Protocol
> > + *
> > + * Copyright 2024 NXP
> > + */
> > +
> > +#define pr_fmt(fmt) "SCMI Notifications MISC - " fmt
> > +
> > +#include <linux/bits.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/scmi_protocol.h>
> > +#include <linux/scmi_imx_protocol.h>
> > +
> > +#include "protocols.h"
> > +#include "notify.h"
> > +
> > +#define SCMI_PROTOCOL_SUPPORTED_VERSION		0x10000
> > +
> > +enum scmi_imx_misc_protocol_cmd {
> > +	SCMI_IMX_MISC_CTRL_SET	= 0x3,
> > +	SCMI_IMX_MISC_CTRL_GET	= 0x4,
> > +	SCMI_IMX_MISC_CTRL_NOTIFY = 0x8,
> > +};
> > +
> > +struct scmi_imx_misc_info {
> > +	u32 version;
> > +	u32 nr_dev_ctrl;
> > +	u32 nr_brd_ctrl;
> > +	u32 nr_reason;
> > +};
> > +
> > +struct scmi_msg_imx_misc_protocol_attributes {
> > +	__le32 attributes;
> > +};
> > +
> > +#define GET_BRD_CTRLS_NR(x)	le32_get_bits((x), GENMASK(31,
> 24))
> > +#define GET_REASONS_NR(x)	le32_get_bits((x), GENMASK(23, 16))
> > +#define GET_DEV_CTRLS_NR(x)	le32_get_bits((x), GENMASK(15, 0))
> > +#define BRD_CTRL_START_ID	BIT(15)
> > +
> > +struct scmi_imx_misc_ctrl_set_in {
> > +	__le32 id;
> > +	__le32 num;
> > +	__le32 value[MISC_MAX_VAL];
> > +};
> > +
> > +struct scmi_imx_misc_ctrl_notify_in {
> > +	__le32 ctrl_id;
> > +	__le32 flags;
> > +};
> > +
> > +struct scmi_imx_misc_ctrl_notify_payld {
> > +	__le32 ctrl_id;
> > +	__le32 flags;
> > +};
> > +
> > +struct scmi_imx_misc_ctrl_get_out {
> > +	__le32 num;
> > +	__le32 *val;
> > +};
> > +
> > +static int scmi_imx_misc_attributes_get(const struct scmi_protocol_handle
> *ph,
> > +					struct scmi_imx_misc_info *mi)
> > +{
> > +	int ret;
> > +	struct scmi_xfer *t;
> > +	struct scmi_msg_imx_misc_protocol_attributes *attr;
> > +
> > +	ret = ph->xops->xfer_get_init(ph, PROTOCOL_ATTRIBUTES, 0,
> > +				      sizeof(*attr), &t);
> > +	if (ret)
> > +		return ret;
> > +
> > +	attr = t->rx.buf;
> > +
> > +	ret = ph->xops->do_xfer(ph, t);
> > +	if (!ret) {
> > +		mi->nr_dev_ctrl = GET_DEV_CTRLS_NR(attr->attributes);
> > +		mi->nr_brd_ctrl = GET_BRD_CTRLS_NR(attr->attributes);
> > +		mi->nr_reason = GET_REASONS_NR(attr->attributes);
> > +		dev_info(ph->dev, "i.MX MISC NUM DEV CTRL: %d, NUM
> BRD CTRL: %d,NUM Reason: %d\n",
> > +			 mi->nr_dev_ctrl, mi->nr_brd_ctrl, mi->nr_reason);
> > +	}
> > +
> > +	ph->xops->xfer_put(ph, t);
> > +
> > +	return ret;
> > +}
> > +
> > +static int scmi_imx_misc_ctrl_validate_id(const struct
> scmi_protocol_handle *ph,
> > +					  u32 ctrl_id)
> > +{
> > +	struct scmi_imx_misc_info *mi = ph->get_priv(ph);
> > +
> > +	if ((ctrl_id < BRD_CTRL_START_ID) && (ctrl_id > mi->nr_dev_ctrl))
> > +		return -EINVAL;
> > +	if (ctrl_id >= BRD_CTRL_START_ID + mi->nr_brd_ctrl)
> > +		return -EINVAL;
> > +
> > +	return 0;
> > +}
> > +
> > +static int scmi_imx_misc_ctrl_notify(const struct scmi_protocol_handle
> *ph,
> > +				     u32 ctrl_id, u32 flags)
> > +{
> > +	struct scmi_imx_misc_ctrl_notify_in *in;
> > +	struct scmi_xfer *t;
> > +	int ret;
> > +
> > +	ret = scmi_imx_misc_ctrl_validate_id(ph, ctrl_id);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = ph->xops->xfer_get_init(ph, SCMI_IMX_MISC_CTRL_NOTIFY,
> > +				      sizeof(*in), 0, &t);
> > +	if (ret)
> > +		return ret;
> > +
> > +	in = t->tx.buf;
> > +	in->ctrl_id = cpu_to_le32(ctrl_id);
> > +	in->flags = cpu_to_le32(flags);
> > +
> > +	ret = ph->xops->do_xfer(ph, t);
> > +
> > +	ph->xops->xfer_put(ph, t);
> > +
> > +	return ret;
> > +}
> > +
> > +static int
> > +scmi_imx_misc_ctrl_set_notify_enabled(const struct
> scmi_protocol_handle *ph,
> > +				      u8 evt_id, u32 src_id, bool enable) {
> > +	int ret;
> > +
> > +	ret = scmi_imx_misc_ctrl_notify(ph, src_id, enable ? evt_id : 0);
> > +	if (ret)
> > +		dev_err(ph->dev, "FAIL_ENABLED - evt[%X] src[%d] -
> ret:%d\n",
> > +			evt_id, src_id, ret);
> > +
> > +	return ret;
> > +}
> > +
> > +static int scmi_imx_misc_ctrl_get_num_sources(const struct
> > +scmi_protocol_handle *ph) {
> > +	return GENMASK(15, 0);
> > +}
> > +
> > +static void *
> > +scmi_imx_misc_ctrl_fill_custom_report(const struct scmi_protocol_handle
> *ph,
> > +				      u8 evt_id, ktime_t timestamp,
> > +				      const void *payld, size_t payld_sz,
> > +				      void *report, u32 *src_id)
> > +{
> > +	const struct scmi_imx_misc_ctrl_notify_payld *p = payld;
> > +	struct scmi_imx_misc_ctrl_notify_report *r = report;
> > +
> > +	if (sizeof(*p) != payld_sz)
> > +		return NULL;
> > +
> > +	r->timestamp = timestamp;
> > +	r->ctrl_id = p->ctrl_id;
> > +	r->flags = p->flags;
> > +	*src_id = r->ctrl_id;
> > +	dev_dbg(ph->dev, "%s: ctrl_id: %d flags: %d\n", __func__,
> > +		r->ctrl_id, r->flags);
> > +
> > +	return r;
> > +}
> > +
> > +static const struct scmi_event_ops scmi_imx_misc_event_ops = {
> > +	.get_num_sources = scmi_imx_misc_ctrl_get_num_sources,
> > +	.set_notify_enabled = scmi_imx_misc_ctrl_set_notify_enabled,
> > +	.fill_custom_report = scmi_imx_misc_ctrl_fill_custom_report,
> > +};
> > +
> > +static const struct scmi_event scmi_imx_misc_events[] = {
> > +	{
> > +		.id = SCMI_EVENT_IMX_MISC_CONTROL_DISABLED,
> > +		.max_payld_sz = sizeof(struct
> scmi_imx_misc_ctrl_notify_payld),
> > +		.max_report_sz = sizeof(struct
> scmi_imx_misc_ctrl_notify_report),
> > +	},
> > +	{
> > +		.id = SCMI_EVENT_IMX_MISC_CONTROL_FALLING_EDGE,
> > +		.max_payld_sz = sizeof(struct
> scmi_imx_misc_ctrl_notify_payld),
> > +		.max_report_sz = sizeof(struct
> scmi_imx_misc_ctrl_notify_report),
> > +	},
> > +	{
> > +		.id = SCMI_EVENT_IMX_MISC_CONTROL_RISING_EDGE,
> > +		.max_payld_sz = sizeof(struct
> scmi_imx_misc_ctrl_notify_payld),
> > +		.max_report_sz = sizeof(struct
> scmi_imx_misc_ctrl_notify_report),
> > +	}
> > +};
> > +
> > +static struct scmi_protocol_events scmi_imx_misc_protocol_events = {
> > +	.queue_sz = SCMI_PROTO_QUEUE_SZ,
> > +	.ops = &scmi_imx_misc_event_ops,
> > +	.evts = scmi_imx_misc_events,
> > +	.num_events = ARRAY_SIZE(scmi_imx_misc_events), };
> > +
> > +static int scmi_imx_misc_protocol_init(const struct
> > +scmi_protocol_handle *ph) {
> > +	struct scmi_imx_misc_info *minfo;
> > +	u32 version;
> > +	int ret;
> > +
> > +	ret = ph->xops->version_get(ph, &version);
> > +	if (ret)
> > +		return ret;
> > +
> > +	dev_info(ph->dev, "NXP SM MISC Version %d.%d\n",
> > +		 PROTOCOL_REV_MAJOR(version),
> PROTOCOL_REV_MINOR(version));
> > +
> > +	minfo = devm_kzalloc(ph->dev, sizeof(*minfo), GFP_KERNEL);
> > +	if (!minfo)
> > +		return -ENOMEM;
> > +
> > +	ret = scmi_imx_misc_attributes_get(ph, minfo);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return ph->set_priv(ph, minfo, version); }
> > +
> > +static int scmi_imx_misc_ctrl_get(const struct scmi_protocol_handle *ph,
> > +				  u32 ctrl_id, u32 *num, u32 *val) {
> > +	struct scmi_imx_misc_ctrl_get_out *out;
> > +	struct scmi_xfer *t;
> > +	int ret, i;
> > +
> > +	ret = scmi_imx_misc_ctrl_validate_id(ph, ctrl_id);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = ph->xops->xfer_get_init(ph, SCMI_IMX_MISC_CTRL_GET,
> sizeof(u32),
> > +				      0, &t);
> > +	if (ret)
> > +		return ret;
> > +
> > +	put_unaligned_le32(ctrl_id, t->tx.buf);
> > +	ret = ph->xops->do_xfer(ph, t);
> > +	if (!ret) {
> > +		out = t->rx.buf;
> > +		*num = le32_to_cpu(out->num);
> > +		for (i = 0; i < *num && i < MISC_MAX_VAL; i++)
> > +			val[i] = le32_to_cpu(out->val[i]);
> > +	}
> > +
> > +	ph->xops->xfer_put(ph, t);
> > +
> > +	return ret;
> > +}
> > +
> > +static int scmi_imx_misc_ctrl_set(const struct scmi_protocol_handle *ph,
> > +				  u32 ctrl_id, u32 num, u32 *val) {
> > +	struct scmi_imx_misc_ctrl_set_in *in;
> > +	struct scmi_xfer *t;
> > +	int ret, i;
> > +
> > +	ret = scmi_imx_misc_ctrl_validate_id(ph, ctrl_id);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (num > MISC_MAX_VAL)
> > +		return -EINVAL;
> > +
> > +	ret = ph->xops->xfer_get_init(ph, SCMI_IMX_MISC_CTRL_SET,
> sizeof(*in),
> > +				      0, &t);
> > +	if (ret)
> > +		return ret;
> > +
> > +	in = t->tx.buf;
> > +	in->id = cpu_to_le32(ctrl_id);
> > +	in->num = cpu_to_le32(num);
> > +	for (i = 0; i < num; i++)
> > +		in->value[i] = cpu_to_le32(val[i]);
> > +
> > +	ret = ph->xops->do_xfer(ph, t);
> > +
> > +	ph->xops->xfer_put(ph, t);
> > +
> > +	return ret;
> > +}
> > +
> > +static const struct scmi_imx_misc_proto_ops scmi_imx_misc_proto_ops =
> {
> > +	.misc_ctrl_set = scmi_imx_misc_ctrl_set,
> > +	.misc_ctrl_get = scmi_imx_misc_ctrl_get, };
> > +
> > +static const struct scmi_protocol scmi_imx_misc = {
> > +	.id = SCMI_PROTOCOL_IMX_MISC,
> > +	.owner = THIS_MODULE,
> > +	.instance_init = &scmi_imx_misc_protocol_init,
> > +	.ops = &scmi_imx_misc_proto_ops,
> > +	.events = &scmi_imx_misc_protocol_events,
> > +	.supported_version = SCMI_PROTOCOL_SUPPORTED_VERSION, };
> > +module_scmi_protocol(scmi_imx_misc);
> > diff --git a/include/linux/scmi_imx_protocol.h
> > b/include/linux/scmi_imx_protocol.h
> > index 90ce011a4429..a69bd4a20f0f 100644
> > --- a/include/linux/scmi_imx_protocol.h
> > +++ b/include/linux/scmi_imx_protocol.h
> > @@ -13,8 +13,14 @@
> >  #include <linux/notifier.h>
> >  #include <linux/types.h>
> >
> > +#define SCMI_PAYLOAD_LEN	100
> > +
> > +#define SCMI_ARRAY(X, Y)	((SCMI_PAYLOAD_LEN - (X)) / sizeof(Y))
> > +#define MISC_MAX_VAL		SCMI_ARRAY(8, uint32_t)
> > +
> >  enum scmi_nxp_protocol {
> >  	SCMI_PROTOCOL_IMX_BBM = 0x81,
> > +	SCMI_PROTOCOL_IMX_MISC = 0x84,
> >  };
> >
> >  struct scmi_imx_bbm_proto_ops {
> > @@ -42,4 +48,15 @@ struct scmi_imx_bbm_notif_report {
> >  	unsigned int		rtc_id;
> >  	unsigned int		rtc_evt;
> >  };
> > +
> > +struct scmi_imx_misc_ctrl_notify_report {
> > +	ktime_t			timestamp;
> > +	unsigned int		ctrl_id;
> > +	unsigned int		flags;
> > +};
> > +
> > +struct scmi_imx_misc_proto_ops {
> > +	int (*misc_ctrl_set)(const struct scmi_protocol_handle *ph, u32 id,
> u32 num, u32 *val);
> > +	int (*misc_ctrl_get)(const struct scmi_protocol_handle *ph, u32 id,
> > +u32 *num, u32 *val); };
> >  #endif
> >
> > --
> > 2.37.1
> >
> >
> >

^ permalink raw reply

* RE: [PATCH v2 2/6] dt-bindings: firmware: add i.MX SCMI Extension protocol
From: Peng Fan @ 2024-04-07  0:51 UTC (permalink / raw)
  To: Krzysztof Kozlowski, Peng Fan (OSS), Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, Sudeep Holla,
	Cristian Marussi
  Cc: devicetree@vger.kernel.org, imx@lists.linux.dev,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org
In-Reply-To: <9aab8dab-27c4-40b6-b1d5-0a2babe0700d@kernel.org>

> Subject: Re: [PATCH v2 2/6] dt-bindings: firmware: add i.MX SCMI Extension
> protocol
> 
> On 05/04/2024 14:39, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > Add i.MX SCMI Extension protocols bindings for:
> >  - Battery Backed Secure Module(BBSM)
> 
> Which is what?

I should say BBM(BBSM + BBNSM), BBM has RTC and ON/OFF
key features, but BBM is managed by SCMI firmware and exported
to agent by BBM protocol. So add bindings for i.MX BBM protocol.

Is this ok?

> 
> >  - MISC settings such as General Purpose Registers settings.
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  .../devicetree/bindings/firmware/imx,scmi.yaml     | 80
> ++++++++++++++++++++++
> >  1 file changed, 80 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/firmware/imx,scmi.yaml
> > b/Documentation/devicetree/bindings/firmware/imx,scmi.yaml
> > new file mode 100644
> > index 000000000000..7ee19a661d83
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/firmware/imx,scmi.yaml
> > @@ -0,0 +1,80 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) # Copyright 2024
> > +NXP %YAML 1.2
> > +---
> > +$id:
> > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi
> >
> +cetree.org%2Fschemas%2Ffirmware%2Fimx%2Cscmi.yaml%23&data=05%7
> C02%7Cp
> >
> +eng.fan%40nxp.com%7C5d16781d3eca425a342508dc562910b7%7C686ea
> 1d3bc2b4c
> >
> +6fa92cd99c5c301635%7C0%7C0%7C638479981570959816%7CUnknown%
> 7CTWFpbGZsb
> >
> +3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn
> 0%3D
> >
> +%7C0%7C%7C%7C&sdata=mWNwPvu2eyF18MroVOBHb%2Fjeo%2BIHfV5V
> h%2F9ebdx65MM
> > +%3D&reserved=0
> > +$schema:
> > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi
> > +cetree.org%2Fmeta-
> schemas%2Fcore.yaml%23&data=05%7C02%7Cpeng.fan%40nx
> >
> +p.com%7C5d16781d3eca425a342508dc562910b7%7C686ea1d3bc2b4c6fa
> 92cd99c5c
> >
> +301635%7C0%7C0%7C638479981570971949%7CUnknown%7CTWFpbGZs
> b3d8eyJWIjoiM
> >
> +C4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7
> C%7C%7
> >
> +C&sdata=v4XnGG00D4I8j5MJvDUVYMRTm7yRrvz0V3fUyc5KAAA%3D&reser
> ved=0
> > +
> > +title: i.MX System Control and Management Interface(SCMI) Vendor
> > +Protocols Extension
> > +
> > +maintainers:
> > +  - Peng Fan <peng.fan@nxp.com>
> > +
> > +allOf:
> > +  - $ref: arm,scmi.yaml#
> 
> Sorry, but arm,scmi is a final schema. Is your plan to define some common
> part?

No. I just wanna add vendor extension per SCMI spec. 

0x80-0xFF:
Reserved for vendor or platform-specific extensions to this interface

Each vendor may have different usage saying id 0x81, so I add
i.MX dt-schema file.

> 
> > +
> > +properties:
> > +  protocol@81:
> > +    $ref: 'arm,scmi.yaml#/$defs/protocol-node'
> > +    unevaluatedProperties: false
> > +    description:
> > +      The BBM Protocol is for managing Battery Backed Secure Module
> (BBSM) RTC
> > +      and the ON/OFF Key
> > +
> > +    properties:
> > +      reg:
> > +        const: 0x81
> > +
> > +    required:
> > +      - reg
> > +
> > +  protocol@84:
> > +    $ref: 'arm,scmi.yaml#/$defs/protocol-node'
> > +    unevaluatedProperties: false
> > +    description:
> > +      The MISC Protocol is for managing SoC Misc settings, such as
> > + GPR settings
> 
> Genera register is not a setting... this is a pleonasm. Please be more specific
> what is the GPR, MISC protocol etc.

The MISC Protocol is for managing SoC Misc settings, such as SAI MCLK/MQS in
Always On domain BLK CTRL,  SAI_CLK_SEL in WAKEUP BLK CTRL, gpio
expanders which is under control of SCMI firmware.

> > +
> > +    properties:
> > +      reg:
> > +        const: 0x84
> > +
> > +      wakeup-sources:
> > +        description:
> > +          Each entry consists of 2 integers, represents the source
> > + and electric signal edge
> 
> Can you answer questions from reviewers?

Sorry. Is this ok?
minItems: 1
maxItems: 32

> 
> > +        items:
> > +          items:
> > +            - description: the wakeup source
> > +            - description: the wakeup electric signal edge
> > +        $ref: /schemas/types.yaml#/definitions/uint32-matrix
> > +
> > +    required:
> > +      - reg
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    firmware {
> > +        scmi {
> > +            compatible = "arm,scmi";
> 
> > +            mboxes = <&mu2 5 0>, <&mu2 3 0>, <&mu2 3 1>;
> > +            shmem = <&scmi_buf0>, <&scmi_buf1>;
> > +
> > +            #address-cells = <1>;
> > +            #size-cells = <0>;
> > +
> > +            protocol@81 {
> > +                reg = <0x81>;
> > +            };
> > +
> > +            protocol@84 {
> > +                reg = <0x84>;
> > +                wakeup-sources = <0x8000 1
> > +                                  0x8001 1
> > +                                  0x8002 1
> > +                                  0x8003 1
> > +                                  0x8004 1>;
> 
> Nothing improved... If you are going to ignore reviews, then you will only get
> NAKed.

Sorry, you mean the examples, or the whole dt-schema?

Thanks,
Peng.
> 
> Best regards,
> Krzysztof


^ permalink raw reply

* RE: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set additionalProperties to true
From: Peng Fan @ 2024-04-07  0:37 UTC (permalink / raw)
  To: Krzysztof Kozlowski, Peng Fan (OSS), Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, Sudeep Holla,
	Cristian Marussi
  Cc: devicetree@vger.kernel.org, imx@lists.linux.dev,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org
In-Reply-To: <614b5107-656d-4d41-99c1-77941c48342c@kernel.org>

> Subject: Re: [PATCH v2 1/6] dt-bindings: firmware: arm,scmi: set
> additionalProperties to true
> 
> On 05/04/2024 14:39, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > When adding vendor extension protocols, there is dt-schema warning:
> > "
> > imx,scmi.example.dtb: scmi: 'protocol@81', 'protocol@84' do not match
> > any of the regexes: 'pinctrl-[0-9]+'
> > "
> >
> > Set additionalProperties to true to address the issue.
> 
> I do not see anything addressed here, except making the binding accepting
> anything anywhere...

I not wanna add vendor protocols in arm,scmi.yaml, so will introduce
a new yaml imx.scmi.yaml which add i.MX SCMI protocol extension.

With additionalProperties set to false, I not know how, please suggest.

Thanks,
Peng.
> 
> Best regards,
> Krzysztof


^ permalink raw reply

* Re: [PATCH net-next v6 11/17] dt-bindings: net: pse-pd: Add another way of describing several PSE PIs
From: Kory Maincent @ 2024-04-06 19:37 UTC (permalink / raw)
  To: Rob Herring
  Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Jonathan Corbet, Luis Chamberlain, Russ Weight,
	Greg Kroah-Hartman, Rafael J. Wysocki, Krzysztof Kozlowski,
	Conor Dooley, Oleksij Rempel, Mark Brown, Frank Rowand,
	Andrew Lunn, Heiner Kallweit, Russell King, Thomas Petazzoni,
	netdev, linux-kernel, linux-doc, devicetree, Dent Project,
	Maxime Chevallier
In-Reply-To: <20240404103854.29ef418c@kmaincent-XPS-13-7390>

On Thu, 4 Apr 2024 10:38:54 +0200
Kory Maincent <kory.maincent@bootlin.com> wrote:

> On Wed, 3 Apr 2024 09:31:42 -0500
> Rob Herring <robh@kernel.org> wrote:
> 
> > On Wed, Apr 03, 2024 at 11:15:48AM +0200, Kory Maincent wrote:  
> > > On Tue, 2 Apr 2024 08:26:37 -0500
> > > Rob Herring <robh@kernel.org> wrote:
> > >     
> > > > > +          pairset-names:
> > > > > +            $ref: /schemas/types.yaml#/definitions/string-array
> > > > > +            description:
> > > > > +              Names of the pairsets as per IEEE 802.3-2022, Section
> > > > > 145.2.4.
> > > > > +              Valid values are "alternative-a" and "alternative-b".
> > > > > Each name      
> > > > 
> > > > Don't state constraints in prose which are defined as schema 
> > > > constraints.    
> > > 
> > > Ok, I will remove the line.
> > >     
> > > > > +          pairsets:
> > > > > +            $ref: /schemas/types.yaml#/definitions/phandle-array
> > > > > +            description:
> > > > > +              List of phandles, each pointing to the power supply for
> > > > > the
> > > > > +              corresponding pairset named in 'pairset-names'. This
> > > > > property
> > > > > +              aligns with IEEE 802.3-2022, Section 33.2.3 and
> > > > > 145.2.4.
> > > > > +              PSE Pinout Alternatives (as per IEEE 802.3-2022 Table
> > > > > 145\u20133)
> > > > > +
> > > > > |-----------|---------------|---------------|---------------|---------------|
> > > > > +              | Conductor | Alternative A | Alternative A |
> > > > > Alternative B | Alternative B |
> > > > > +              |           |    (MDI-X)    |     (MDI)     |      (X)
> > > > > |      (S)      |
> > > > > +
> > > > > |-----------|---------------|---------------|---------------|---------------|
> > > > > +              | 1         | Negative VPSE | Positive VPSE | \u2014
> > > > >     | \u2014             |
> > > > > +              | 2         | Negative VPSE | Positive VPSE | \u2014
> > > > >     | \u2014             |
> > > > > +              | 3         | Positive VPSE | Negative VPSE | \u2014
> > > > >     | \u2014             |
> > > > > +              | 4         | \u2014             | \u2014             |
> > > > > Negative VPSE | Positive VPSE |
> > > > > +              | 5         | \u2014             | \u2014             |
> > > > > Negative VPSE | Positive VPSE |
> > > > > +              | 6         | Positive VPSE | Negative VPSE | \u2014
> > > > >     | \u2014             |
> > > > > +              | 7         | \u2014             | \u2014             |
> > > > > Positive VPSE | Negative VPSE |
> > > > > +              | 8         | \u2014             | \u2014             |
> > > > > Positive VPSE | Negative VPSE |
> > > > > +            minItems: 1
> > > > > +            maxItems: 2      
> > > > 
> > > > "pairsets" does not follow the normal design pattern of foos,
> > > > foo-names, and #foo-cells. You could add #foo-cells I suppose, but what
> > > > would cells convey? I don't think it's a good fit for what you need.
> > > > 
> > > > The other oddity is the number of entries and the names are fixed. That 
> > > > is usually defined per consumer.     
> > > 
> > > Theoretically if the RJ45 port binding was supported it would make more
> > > sense, but in reality it's not feasible as the PSE controller need this
> > > information in its init process.
> > > The PSE controller reset all its port to apply a configuration so we can't
> > > do it when the consumer (RJ45) probe. It would reset the other ports if
> > > one consumer is probed later in the process.    
> > 
> > There is no reason other than convenience that all information some 
> > driver needs has to be in one node or one hierarchy of nodes. You can 
> > fetch anything from anywhere in the DT. It does feel like some of this 
> > belongs in a connector node. We often haven't described connectors in DT 
> > and stick connector properties in the controller node associated with 
> > the connector. Then as things get more complicated, it becomes a mess.   
> 
> Right, we could indeed put all the informations of the pse_pi node in the
> future RJ45 port abstraction node. Then, this series will be put aside until
> we manage to have the port abstraction get merged.
> I am not glad about this as it will stuck my work until then, but indeed
> removing this pse_pi wrapper node which is between the pse_controller node and
> the connector node seems cleaner.

After some new thought, I thinks it is quite similar on the devicetree side to
have it in a pse_pi node or in the connector node.
Here are my agruments to continue using this pse_pi binding description:
- The connector abstraction is in its early work and won't really see a v1 soon
  while the PoE series got mainly all reviewed-by thanks to Andrew.
  This would stuck the PoE series until maybe one or two Linux version.
- It allows to use the "Power Interface" name like described in the standards.
- Even if this is in the PSE controller node, it is generic to all PSEs so it
  shouldn't become a mess.
- It allows to have the PSE controller and Power Interfaces parameters grouped
  together and it will be easier to read. May not really be an argument! ;)
- It will keep the logic of PoDL with the PHY using a single reference to the
  PSE PI through the pses parameter. 

Is it okay for you to continue with it?

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

^ permalink raw reply

* Re: [PATCH v2] dt-bindings: ti,pcm1681: Convert to dtschema
From: Krzysztof Kozlowski @ 2024-04-06 18:43 UTC (permalink / raw)
  To: Animesh Agarwal
  Cc: Shenghao Ding, Kevin Lu, Baojun Xu, Liam Girdwood, Mark Brown,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, alsa-devel,
	devicetree, linux-kernel
In-Reply-To: <20240406141454.45529-1-animeshagarwal28@gmail.com>

On 06/04/2024 16:14, Animesh Agarwal wrote:
> Convert the Texas Instruments PCM1681 bindings to DT schema.
> Make bindings complete by adding #sound-dai-cells.
> 
> Signed-off-by: Animesh Agarwal <animeshagarwal28@gmail.com>
> 
> ---

Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof


^ permalink raw reply

* [PATCH] arm64: dts: rockchip: rk3588s: Fix ordering of nodes
From: Diederik de Haas @ 2024-04-06 17:28 UTC (permalink / raw)
  To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Heiko Stuebner
  Cc: devicetree, linux-arm-kernel, linux-rockchip, Diederik de Haas

Fix the ordering of the main nodes by sorting them alphabetically and
then the ones with a memory address sequentially by that address.

Signed-off-by: Diederik de Haas <didi.debian@cknow.org>
---
 arch/arm64/boot/dts/rockchip/rk3588s.dtsi | 304 +++++++++++-----------
 1 file changed, 152 insertions(+), 152 deletions(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
index 89d40cff635f..ac5bd630f155 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
@@ -347,6 +347,11 @@ l3_cache: l3-cache {
 		};
 	};
 
+	display_subsystem: display-subsystem {
+		compatible = "rockchip,display-subsystem";
+		ports = <&vop_out>;
+	};
+
 	firmware {
 		optee: optee {
 			compatible = "linaro,optee-tz";
@@ -394,11 +399,6 @@ spll: clock-0 {
 		#clock-cells = <0>;
 	};
 
-	display_subsystem: display-subsystem {
-		compatible = "rockchip,display-subsystem";
-		ports = <&vop_out>;
-	};
-
 	timer {
 		compatible = "arm,armv8-timer";
 		interrupts = <GIC_PPI 13 IRQ_TYPE_LEVEL_HIGH 0>,
@@ -436,6 +436,62 @@ scmi_shmem: sram@0 {
 		};
 	};
 
+	gpu: gpu@fb000000 {
+		compatible = "rockchip,rk3588-mali", "arm,mali-valhall-csf";
+		reg = <0x0 0xfb000000 0x0 0x200000>;
+		#cooling-cells = <2>;
+		assigned-clocks = <&scmi_clk SCMI_CLK_GPU>;
+		assigned-clock-rates = <200000000>;
+		clocks = <&cru CLK_GPU>, <&cru CLK_GPU_COREGROUP>,
+			 <&cru CLK_GPU_STACKS>;
+		clock-names = "core", "coregroup", "stacks";
+		dynamic-power-coefficient = <2982>;
+		interrupts = <GIC_SPI 92 IRQ_TYPE_LEVEL_HIGH 0>,
+			     <GIC_SPI 93 IRQ_TYPE_LEVEL_HIGH 0>,
+			     <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH 0>;
+		interrupt-names = "job", "mmu", "gpu";
+		operating-points-v2 = <&gpu_opp_table>;
+		power-domains = <&power RK3588_PD_GPU>;
+		status = "disabled";
+
+		gpu_opp_table: opp-table {
+			compatible = "operating-points-v2";
+
+			opp-300000000 {
+				opp-hz = /bits/ 64 <300000000>;
+				opp-microvolt = <675000 675000 850000>;
+			};
+			opp-400000000 {
+				opp-hz = /bits/ 64 <400000000>;
+				opp-microvolt = <675000 675000 850000>;
+			};
+			opp-500000000 {
+				opp-hz = /bits/ 64 <500000000>;
+				opp-microvolt = <675000 675000 850000>;
+			};
+			opp-600000000 {
+				opp-hz = /bits/ 64 <600000000>;
+				opp-microvolt = <675000 675000 850000>;
+			};
+			opp-700000000 {
+				opp-hz = /bits/ 64 <700000000>;
+				opp-microvolt = <700000 700000 850000>;
+			};
+			opp-800000000 {
+				opp-hz = /bits/ 64 <800000000>;
+				opp-microvolt = <750000 750000 850000>;
+			};
+			opp-900000000 {
+				opp-hz = /bits/ 64 <900000000>;
+				opp-microvolt = <800000 800000 850000>;
+			};
+			opp-1000000000 {
+				opp-hz = /bits/ 64 <1000000000>;
+				opp-microvolt = <850000 850000 850000>;
+			};
+		};
+	};
+
 	usb_host0_ehci: usb@fc800000 {
 		compatible = "rockchip,rk3588-ehci", "generic-ehci";
 		reg = <0x0 0xfc800000 0x0 0x40000>;
@@ -501,62 +557,6 @@ usb_host2_xhci: usb@fcd00000 {
 		status = "disabled";
 	};
 
-	gpu: gpu@fb000000 {
-		compatible = "rockchip,rk3588-mali", "arm,mali-valhall-csf";
-		reg = <0x0 0xfb000000 0x0 0x200000>;
-		#cooling-cells = <2>;
-		assigned-clocks = <&scmi_clk SCMI_CLK_GPU>;
-		assigned-clock-rates = <200000000>;
-		clocks = <&cru CLK_GPU>, <&cru CLK_GPU_COREGROUP>,
-			 <&cru CLK_GPU_STACKS>;
-		clock-names = "core", "coregroup", "stacks";
-		dynamic-power-coefficient = <2982>;
-		interrupts = <GIC_SPI 92 IRQ_TYPE_LEVEL_HIGH 0>,
-			     <GIC_SPI 93 IRQ_TYPE_LEVEL_HIGH 0>,
-			     <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH 0>;
-		interrupt-names = "job", "mmu", "gpu";
-		operating-points-v2 = <&gpu_opp_table>;
-		power-domains = <&power RK3588_PD_GPU>;
-		status = "disabled";
-
-		gpu_opp_table: opp-table {
-			compatible = "operating-points-v2";
-
-			opp-300000000 {
-				opp-hz = /bits/ 64 <300000000>;
-				opp-microvolt = <675000 675000 850000>;
-			};
-			opp-400000000 {
-				opp-hz = /bits/ 64 <400000000>;
-				opp-microvolt = <675000 675000 850000>;
-			};
-			opp-500000000 {
-				opp-hz = /bits/ 64 <500000000>;
-				opp-microvolt = <675000 675000 850000>;
-			};
-			opp-600000000 {
-				opp-hz = /bits/ 64 <600000000>;
-				opp-microvolt = <675000 675000 850000>;
-			};
-			opp-700000000 {
-				opp-hz = /bits/ 64 <700000000>;
-				opp-microvolt = <700000 700000 850000>;
-			};
-			opp-800000000 {
-				opp-hz = /bits/ 64 <800000000>;
-				opp-microvolt = <750000 750000 850000>;
-			};
-			opp-900000000 {
-				opp-hz = /bits/ 64 <900000000>;
-				opp-microvolt = <800000 800000 850000>;
-			};
-			opp-1000000000 {
-				opp-hz = /bits/ 64 <1000000000>;
-				opp-microvolt = <850000 850000 850000>;
-			};
-		};
-	};
-
 	pmu1grf: syscon@fd58a000 {
 		compatible = "rockchip,rk3588-pmugrf", "syscon", "simple-mfd";
 		reg = <0x0 0xfd58a000 0x0 0x10000>;
@@ -702,74 +702,6 @@ i2c0: i2c@fd880000 {
 		status = "disabled";
 	};
 
-	vop: vop@fdd90000 {
-		compatible = "rockchip,rk3588-vop";
-		reg = <0x0 0xfdd90000 0x0 0x4200>, <0x0 0xfdd95000 0x0 0x1000>;
-		reg-names = "vop", "gamma-lut";
-		interrupts = <GIC_SPI 156 IRQ_TYPE_LEVEL_HIGH 0>;
-		clocks = <&cru ACLK_VOP>,
-			 <&cru HCLK_VOP>,
-			 <&cru DCLK_VOP0>,
-			 <&cru DCLK_VOP1>,
-			 <&cru DCLK_VOP2>,
-			 <&cru DCLK_VOP3>,
-			 <&cru PCLK_VOP_ROOT>;
-		clock-names = "aclk",
-			      "hclk",
-			      "dclk_vp0",
-			      "dclk_vp1",
-			      "dclk_vp2",
-			      "dclk_vp3",
-			      "pclk_vop";
-		iommus = <&vop_mmu>;
-		power-domains = <&power RK3588_PD_VOP>;
-		rockchip,grf = <&sys_grf>;
-		rockchip,vop-grf = <&vop_grf>;
-		rockchip,vo1-grf = <&vo1_grf>;
-		rockchip,pmu = <&pmu>;
-		status = "disabled";
-
-		vop_out: ports {
-			#address-cells = <1>;
-			#size-cells = <0>;
-
-			vp0: port@0 {
-				#address-cells = <1>;
-				#size-cells = <0>;
-				reg = <0>;
-			};
-
-			vp1: port@1 {
-				#address-cells = <1>;
-				#size-cells = <0>;
-				reg = <1>;
-			};
-
-			vp2: port@2 {
-				#address-cells = <1>;
-				#size-cells = <0>;
-				reg = <2>;
-			};
-
-			vp3: port@3 {
-				#address-cells = <1>;
-				#size-cells = <0>;
-				reg = <3>;
-			};
-		};
-	};
-
-	vop_mmu: iommu@fdd97e00 {
-		compatible = "rockchip,rk3588-iommu", "rockchip,rk3568-iommu";
-		reg = <0x0 0xfdd97e00 0x0 0x100>, <0x0 0xfdd97f00 0x0 0x100>;
-		interrupts = <GIC_SPI 156 IRQ_TYPE_LEVEL_HIGH 0>;
-		clocks = <&cru ACLK_VOP>, <&cru HCLK_VOP>;
-		clock-names = "aclk", "iface";
-		#iommu-cells = <0>;
-		power-domains = <&power RK3588_PD_VOP>;
-		status = "disabled";
-	};
-
 	uart0: serial@fd890000 {
 		compatible = "rockchip,rk3588-uart", "snps,dw-apb-uart";
 		reg = <0x0 0xfd890000 0x0 0x100>;
@@ -1140,6 +1072,87 @@ power-domain@RK3588_PD_SDMMC {
 		};
 	};
 
+	av1d: video-codec@fdc70000 {
+		compatible = "rockchip,rk3588-av1-vpu";
+		reg = <0x0 0xfdc70000 0x0 0x800>;
+		interrupts = <GIC_SPI 108 IRQ_TYPE_LEVEL_HIGH 0>;
+		interrupt-names = "vdpu";
+		assigned-clocks = <&cru ACLK_AV1>, <&cru PCLK_AV1>;
+		assigned-clock-rates = <400000000>, <400000000>;
+		clocks = <&cru ACLK_AV1>, <&cru PCLK_AV1>;
+		clock-names = "aclk", "hclk";
+		power-domains = <&power RK3588_PD_AV1>;
+		resets = <&cru SRST_A_AV1>, <&cru SRST_P_AV1>, <&cru SRST_A_AV1_BIU>, <&cru SRST_P_AV1_BIU>;
+	};
+
+	vop: vop@fdd90000 {
+		compatible = "rockchip,rk3588-vop";
+		reg = <0x0 0xfdd90000 0x0 0x4200>, <0x0 0xfdd95000 0x0 0x1000>;
+		reg-names = "vop", "gamma-lut";
+		interrupts = <GIC_SPI 156 IRQ_TYPE_LEVEL_HIGH 0>;
+		clocks = <&cru ACLK_VOP>,
+			 <&cru HCLK_VOP>,
+			 <&cru DCLK_VOP0>,
+			 <&cru DCLK_VOP1>,
+			 <&cru DCLK_VOP2>,
+			 <&cru DCLK_VOP3>,
+			 <&cru PCLK_VOP_ROOT>;
+		clock-names = "aclk",
+			      "hclk",
+			      "dclk_vp0",
+			      "dclk_vp1",
+			      "dclk_vp2",
+			      "dclk_vp3",
+			      "pclk_vop";
+		iommus = <&vop_mmu>;
+		power-domains = <&power RK3588_PD_VOP>;
+		rockchip,grf = <&sys_grf>;
+		rockchip,vop-grf = <&vop_grf>;
+		rockchip,vo1-grf = <&vo1_grf>;
+		rockchip,pmu = <&pmu>;
+		status = "disabled";
+
+		vop_out: ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			vp0: port@0 {
+				#address-cells = <1>;
+				#size-cells = <0>;
+				reg = <0>;
+			};
+
+			vp1: port@1 {
+				#address-cells = <1>;
+				#size-cells = <0>;
+				reg = <1>;
+			};
+
+			vp2: port@2 {
+				#address-cells = <1>;
+				#size-cells = <0>;
+				reg = <2>;
+			};
+
+			vp3: port@3 {
+				#address-cells = <1>;
+				#size-cells = <0>;
+				reg = <3>;
+			};
+		};
+	};
+
+	vop_mmu: iommu@fdd97e00 {
+		compatible = "rockchip,rk3588-iommu", "rockchip,rk3568-iommu";
+		reg = <0x0 0xfdd97e00 0x0 0x100>, <0x0 0xfdd97f00 0x0 0x100>;
+		interrupts = <GIC_SPI 156 IRQ_TYPE_LEVEL_HIGH 0>;
+		clocks = <&cru ACLK_VOP>, <&cru HCLK_VOP>;
+		clock-names = "aclk", "iface";
+		#iommu-cells = <0>;
+		power-domains = <&power RK3588_PD_VOP>;
+		status = "disabled";
+	};
+
 	i2s4_8ch: i2s@fddc0000 {
 		compatible = "rockchip,rk3588-i2s-tdm";
 		reg = <0x0 0xfddc0000 0x0 0x1000>;
@@ -1431,6 +1444,16 @@ qos_vop_m1: qos@fdf82200 {
 		reg = <0x0 0xfdf82200 0x0 0x20>;
 	};
 
+	dfi: dfi@fe060000 {
+		reg = <0x00 0xfe060000 0x00 0x10000>;
+		compatible = "rockchip,rk3588-dfi";
+		interrupts = <GIC_SPI 28 IRQ_TYPE_LEVEL_HIGH 0>,
+			     <GIC_SPI 38 IRQ_TYPE_LEVEL_HIGH 0>,
+			     <GIC_SPI 48 IRQ_TYPE_LEVEL_HIGH 0>,
+			     <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH 0>;
+		rockchip,pmu = <&pmu1grf>;
+	};
+
 	pcie2x1l1: pcie@fe180000 {
 		compatible = "rockchip,rk3588-pcie", "rockchip,rk3568-pcie";
 		bus-range = <0x30 0x3f>;
@@ -1533,16 +1556,6 @@ pcie2x1l2_intc: legacy-interrupt-controller {
 		};
 	};
 
-	dfi: dfi@fe060000 {
-		reg = <0x00 0xfe060000 0x00 0x10000>;
-		compatible = "rockchip,rk3588-dfi";
-		interrupts = <GIC_SPI 28 IRQ_TYPE_LEVEL_HIGH 0>,
-			     <GIC_SPI 38 IRQ_TYPE_LEVEL_HIGH 0>,
-			     <GIC_SPI 48 IRQ_TYPE_LEVEL_HIGH 0>,
-			     <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH 0>;
-		rockchip,pmu = <&pmu1grf>;
-	};
-
 	gmac1: ethernet@fe1c0000 {
 		compatible = "rockchip,rk3588-gmac", "snps,dwmac-4.20a";
 		reg = <0x0 0xfe1c0000 0x0 0x10000>;
@@ -2543,19 +2556,6 @@ gpio4: gpio@fec50000 {
 			#interrupt-cells = <2>;
 		};
 	};
-
-	av1d: video-codec@fdc70000 {
-		compatible = "rockchip,rk3588-av1-vpu";
-		reg = <0x0 0xfdc70000 0x0 0x800>;
-		interrupts = <GIC_SPI 108 IRQ_TYPE_LEVEL_HIGH 0>;
-		interrupt-names = "vdpu";
-		assigned-clocks = <&cru ACLK_AV1>, <&cru PCLK_AV1>;
-		assigned-clock-rates = <400000000>, <400000000>;
-		clocks = <&cru ACLK_AV1>, <&cru PCLK_AV1>;
-		clock-names = "aclk", "hclk";
-		power-domains = <&power RK3588_PD_AV1>;
-		resets = <&cru SRST_A_AV1>, <&cru SRST_P_AV1>, <&cru SRST_A_AV1_BIU>, <&cru SRST_P_AV1_BIU>;
-	};
 };
 
 #include "rk3588s-pinctrl.dtsi"

base-commit: c1b1f340dd7db11f273e426e110697551c9f501f
-- 
2.43.0


^ permalink raw reply related

* Re: [PATCH v8 2/7] arm64: dts: qcom: sm8450: Add interconnect path to PCIe node
From: Krishna Chaitanya Chundru @ 2024-04-06 17:18 UTC (permalink / raw)
  To: Manivannan Sadhasivam, Konrad Dybcio
  Cc: Bjorn Andersson, Lorenzo Pieralisi, Krzysztof Wilczyński,
	Rob Herring, Bjorn Helgaas, Krzysztof Kozlowski, Conor Dooley,
	Rob Herring, Johan Hovold, Brian Masney, Georgi Djakov,
	linux-arm-msm, linux-pci, devicetree, linux-kernel, vireshk,
	quic_vbadigan, quic_skananth, quic_nitegupt, quic_parass
In-Reply-To: <20240405074044.GC2953@thinkpad>



On 4/5/2024 1:10 PM, Manivannan Sadhasivam wrote:
> On Wed, Mar 06, 2024 at 05:04:54PM +0100, Konrad Dybcio wrote:
>>
>>
>> On 3/2/24 04:59, Krishna chaitanya chundru wrote:
>>> Add pcie-mem & cpu-pcie interconnect path to the PCIe nodes.
>>>
>>> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
>>> ---
>>>    arch/arm64/boot/dts/qcom/sm8450.dtsi | 8 ++++++++
>>>    1 file changed, 8 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi
>>> index 01e4dfc4babd..6b1d2e0d9d14 100644
>>> --- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
>>> +++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
>>> @@ -1781,6 +1781,10 @@ pcie0: pcie@1c00000 {
>>>    					<0 0 0 3 &intc 0 0 0 151 IRQ_TYPE_LEVEL_HIGH>, /* int_c */
>>>    					<0 0 0 4 &intc 0 0 0 152 IRQ_TYPE_LEVEL_HIGH>; /* int_d */
>>> +			interconnects = <&pcie_noc MASTER_PCIE_0 0 &mc_virt SLAVE_EBI1 0>,
>>
>> Please use QCOM_ICC_TAG_ALWAYS.
>>
>>> +					<&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_PCIE_0 0>;
>>
>> And this path could presumably be demoted to QCOM_ICC_TAG_ACTIVE_ONLY?
>>
> 
> I think it should be fine since there would be no register access done while the
> RPMh is put into sleep state. Krishna, can you confirm that by executing the CX
> shutdown with QCOM_ICC_TAG_ACTIVE_ONLY vote for cpu-pcie path on any supported
> platform?
> 
> But if we do such change, then it should also be applied to other SoCs.
> 
> - Mani
>
we don't a have platform to test this now, we will keep
QCOM_ICC_TAG_ALWAYS for now.

- Krishna Chaitanya.


^ permalink raw reply

* Re: [PATCH 3/3] of: Use scope based of_node_put() cleanups
From: Jonathan Cameron @ 2024-04-06 17:17 UTC (permalink / raw)
  To: Rob Herring; +Cc: Saravana Kannan, devicetree, linux-kernel
In-Reply-To: <20240404-dt-cleanup-free-v1-3-c60e6cba8da9@kernel.org>

On Thu, 04 Apr 2024 09:15:12 -0500
Rob Herring <robh@kernel.org> wrote:

> Use the relatively new scope based of_node_put() cleanup to simplify
> function exit handling. Doing so reduces the chances of forgetting an
> of_node_put() and simplifies error paths by avoiding the need for goto
> statements.
> 
> Signed-off-by: Rob Herring <robh@kernel.org>

Hi Rob,

Looks good in general. A few suggestions inline.
First one is a little more complex, but I think will give you a better end result

The others are readability improvements enabled by the changes you've made
that I think you could validly change in this same patch.

Jonathan

> ---
>  drivers/of/address.c  | 60 ++++++++++++++++-----------------------------------
>  drivers/of/property.c | 22 ++++++-------------
>  2 files changed, 26 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index ae46a3605904..f7b2d535a6d1 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -491,7 +491,6 @@ static u64 __of_translate_address(struct device_node *dev,
>  				  const __be32 *in_addr, const char *rprop,
>  				  struct device_node **host)
>  {
> -	struct device_node *parent = NULL;
>  	struct of_bus *bus, *pbus;
>  	__be32 addr[OF_MAX_ADDR_CELLS];
>  	int na, ns, pna, pns;
> @@ -504,7 +503,7 @@ static u64 __of_translate_address(struct device_node *dev,
>  
>  	*host = NULL;
>  	/* Get parent & match bus type */
> -	parent = get_parent(dev);
> +	struct device_node *parent __free(device_node) = get_parent(dev);

This is an order change as we'll put this node after dev

Whilst probably fine, I'd call that out in the patch description or just throw
in an earlier
struct device_node *dev_node __free(device_node) = of_node_get(dev);


Obviously the release order doesn't actually matter but a reader has to think
about it briefly to be sure.

Bonus if you do that is that you get to do direct returns at the error condition
and potentially in the breaks out of the loop. To my eye that's a readability
advantage.

In at least one place you could also steal the pointer rather than
getting another reference to dev (taking it to at least 2) then dropping
one of them in the exit path. Can use no_free_ptr() for that.

From readability point of view you'd only want to do that with a direct return.
 		
			*host = no_free_ptr(dev_node);
			return result;
		}




>  	if (parent == NULL)
>  		goto bail;
>  	bus = of_match_bus(parent);
> @@ -573,7 +572,6 @@ static u64 __of_translate_address(struct device_node *dev,
>  		of_dump_addr("one level translation:", addr, na);
>  	}
>   bail:
> -	of_node_put(parent);
>  	of_node_put(dev);
>  


> @@ -1016,11 +999,9 @@ phys_addr_t __init of_dma_get_max_cpu_address(struct device_node *np)
>   */
>  bool of_dma_is_coherent(struct device_node *np)
>  {
> -	struct device_node *node;
> +	struct device_node *node __free(device_node) = of_node_get(np);
>  	bool is_coherent = dma_default_coherent;
>  
> -	node = of_node_get(np);
> -
>  	while (node) {
>  		if (of_property_read_bool(node, "dma-coherent")) {
>  			is_coherent = true;

			return true;

and same in the other branch given you break out the loop and return it directly.


> @@ -1032,7 +1013,6 @@ bool of_dma_is_coherent(struct device_node *np)
>  		}
>  		node = of_get_next_dma_parent(node);
>  	}
> -	of_node_put(node);
>  	return is_coherent;
With above early returns

	return dma_default_coherent;

and drop the is_coherent variable as no longer used.

This sort of related cleanup is one reason I really like the cleanup.h magic.


>  }
>  EXPORT_SYMBOL_GPL(of_dma_is_coherent);
> @@ -1049,19 +1029,17 @@ EXPORT_SYMBOL_GPL(of_dma_is_coherent);
>   */
>  static bool of_mmio_is_nonposted(struct device_node *np)
>  {
> -	struct device_node *parent;
>  	bool nonposted;
>  
>  	if (!IS_ENABLED(CONFIG_ARCH_APPLE))
>  		return false;
>  
> -	parent = of_get_parent(np);
> +	struct device_node *parent __free(device_node) = of_get_parent(np);
>  	if (!parent)
>  		return false;
>  
>  	nonposted = of_property_read_bool(parent, "nonposted-mmio");
>  
> -	of_node_put(parent);
>  	return nonposted;

	return of_property_read_bool()

>  }
>  

^ permalink raw reply

* Re: [PATCH 2/3] of: Use scope based kfree() cleanups
From: Jonathan Cameron @ 2024-04-06 16:51 UTC (permalink / raw)
  To: Rob Herring; +Cc: Saravana Kannan, devicetree, linux-kernel
In-Reply-To: <20240404-dt-cleanup-free-v1-2-c60e6cba8da9@kernel.org>

On Thu, 04 Apr 2024 09:15:11 -0500
Rob Herring <robh@kernel.org> wrote:

> Use the relatively new scope based kfree() cleanup to simplify error
> handling. Doing so reduces the chances of memory leaks and simplifies
> error paths by avoiding the need for goto statements.
> 
> Signed-off-by: Rob Herring <robh@kernel.org>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>



^ permalink raw reply

* Re: [PATCH 1/3] of: Add a helper to free property struct
From: Jonathan Cameron @ 2024-04-06 16:47 UTC (permalink / raw)
  To: Rob Herring; +Cc: Saravana Kannan, devicetree, linux-kernel
In-Reply-To: <20240404-dt-cleanup-free-v1-1-c60e6cba8da9@kernel.org>

On Thu, 04 Apr 2024 09:15:10 -0500
Rob Herring <robh@kernel.org> wrote:

> Freeing a property struct is 3 kfree()'s which is duplicated in multiple
> spots. Add a helper, __of_prop_free(), and replace all the open coded
> cases in the DT code.
> 
> Signed-off-by: Rob Herring <robh@kernel.org>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>


^ 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