* [PATCH 2/2] media: i2c: Add GT97xx VCM driver
From: Zhi Mao @ 2024-04-10 10:40 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski,
Conor Dooley
Cc: Matthias Brugger, AngeloGioacchino Del Regno, Zhi Mao,
Philipp Zabel, Laurent Pinchart, Heiko Stuebner, Sakari Ailus,
Hans Verkuil, Hans de Goede, Tomi Valkeinen, Alain Volmat,
Paul Elder, Mehdi Djait, Andy Shevchenko, Bingbu Cao, linux-media,
devicetree, linux-kernel, linux-arm-kernel, linux-mediatek,
shengnan.wang, yaya.chang, yunkec, 10572168
In-Reply-To: <20240410104002.1197-1-zhi.mao@mediatek.com>
Add a V4L2 sub-device driver for Giantec GT97xx VCM.
Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
---
drivers/media/i2c/Kconfig | 13 +
drivers/media/i2c/Makefile | 1 +
drivers/media/i2c/gt97xx.c | 640 +++++++++++++++++++++++++++++++++++++
3 files changed, 654 insertions(+)
create mode 100644 drivers/media/i2c/gt97xx.c
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 56f276b920ab..fcb330cebfe0 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -759,6 +759,19 @@ config VIDEO_DW9807_VCM
capability. This is designed for linear control of
voice coil motors, controlled via I2C serial interface.
+config VIDEO_GT97XX
+ tristate "GT97xx lens voice coil support"
+ depends on I2C && VIDEO_DEV
+ select MEDIA_CONTROLLER
+ select VIDEO_V4L2_SUBDEV_API
+ select V4L2_FWNODE
+ select V4L2_CCI_I2C
+ help
+ This is a driver for the GT97xx camera lens voice coil.
+ GT97xx is a 10 bit DAC with 100mA output current sink
+ capability. It is designed for linear control of
+ voice coil motors, controlled via I2C serial interface.
+
endmenu
menu "Flash devices"
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index dfbe6448b549..af36a7aa3d12 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -39,6 +39,7 @@ obj-$(CONFIG_VIDEO_DW9807_VCM) += dw9807-vcm.o
obj-$(CONFIG_VIDEO_ET8EK8) += et8ek8/
obj-$(CONFIG_VIDEO_GC0308) += gc0308.o
obj-$(CONFIG_VIDEO_GC2145) += gc2145.o
+obj-$(CONFIG_VIDEO_GT97XX) += gt97xx.o
obj-$(CONFIG_VIDEO_HI556) += hi556.o
obj-$(CONFIG_VIDEO_HI846) += hi846.o
obj-$(CONFIG_VIDEO_HI847) += hi847.o
diff --git a/drivers/media/i2c/gt97xx.c b/drivers/media/i2c/gt97xx.c
new file mode 100644
index 000000000000..d91314b872fa
--- /dev/null
+++ b/drivers/media/i2c/gt97xx.c
@@ -0,0 +1,640 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for Giantec gt97xx VCM lens device
+ *
+ * Copyright 2024 MediaTek
+ *
+ * Zhi Mao <zhi.mao@mediatek.com>
+ */
+#include <linux/array_size.h>
+#include <linux/bits.h>
+#include <linux/clk.h>
+#include <linux/container_of.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/gpio/consumer.h>
+#include <linux/mod_devicetable.h>
+#include <linux/pm_runtime.h>
+#include <linux/property.h>
+#include <linux/regulator/consumer.h>
+#include <linux/types.h>
+#include <linux/units.h>
+
+#include <media/v4l2-cci.h>
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-event.h>
+#include <media/v4l2-fwnode.h>
+#include <media/v4l2-subdev.h>
+
+/* gt97xx chip info register and name */
+#define GT97XX_IC_INFO_REG CCI_REG8(0x00)
+#define GT9768_ID 0xE9
+#define GT9769_ID 0xE1
+#define GT97XX_NAME "gt97xx"
+
+/*
+ * Ring control and Power control register
+ * Bit[1] RING_EN
+ * 0: Direct mode
+ * 1: AAC mode (ringing control mode)
+ * Bit[0] PD
+ * 0: Normal operation mode
+ * 1: Power down mode
+ * gt97xx requires waiting time of Topr after PD reset takes place.
+ */
+#define GT97XX_RING_PD_CONTROL_REG CCI_REG8(0x02)
+#define GT97XX_PD_MODE_OFF 0x00
+#define GT97XX_PD_MODE_EN BIT(0)
+#define GT97XX_AAC_MODE_EN BIT(1)
+
+/*
+ * gt97xx separates two registers to control the VCM position.
+ * One for MSB value, another is LSB value.
+ * DAC_MSB: D[9:8] (ADD: 0x03)
+ * DAC_LSB: D[7:0] (ADD: 0x04)
+ * D[9:0] DAC data input: positive output current = D[9:0] / 1023 * 100[mA]
+ */
+#define GT97XX_MSB_ADDR_REG CCI_REG16(0x03)
+
+/*
+ * AAC mode control & prescale register
+ * Bit[7:5] Namely AC[2:0], decide the VCM mode and operation time.
+ * 001 AAC2 0.48 x Tvib
+ * 010 AAC3 0.70 x Tvib
+ * 011 AAC4 0.75 x Tvib
+ * 101 AAC8 1.13 x Tvib
+ * Bit[2:0] Namely PRESC[2:0], set the internal clock dividing rate as follow.
+ * 000 2
+ * 001 1
+ * 010 1/2
+ * 011 1/4
+ * 100 8
+ * 101 4
+ */
+#define GT97XX_AAC_PRESC_REG CCI_REG8(0x06)
+#define GT97XX_AAC_MODE_SEL_MASK GENMASK(7, 5)
+#define GT97XX_CLOCK_PRE_SCALE_SEL_MASK GENMASK(2, 0)
+
+/*
+ * VCM period of vibration register
+ * Bit[5:0] Defined as VCM rising periodic time (Tvib) together with PRESC[2:0]
+ * Tvib = (6.3ms + AACT[5:0] * 0.1ms) * Dividing Rate
+ * Dividing Rate is the internal clock dividing rate that is defined at
+ * PRESCALE register (ADD: 0x06)
+ */
+#define GT97XX_AAC_TIME_REG CCI_REG8(0x07)
+
+/*
+ * gt97xx requires waiting time (delay time) of t_OPR after power-up,
+ * or in the case of PD reset taking place.
+ */
+#define GT97XX_T_OPR_US (1 * USEC_PER_MSEC)
+#define GT97XX_TVIB_MS_BASE10 (64 - 1)
+#define GT97XX_AAC_MODE_DEFAULT 2
+#define GT97XX_AAC_TIME_DEFAULT 0x20
+#define GT97XX_CLOCK_PRE_SCALE_DEFAULT 1
+
+/*
+ * This acts as the minimum granularity of lens movement.
+ * Keep this value power of 2, so the control steps can be
+ * uniformly adjusted for gradual lens movement, with desired
+ * number of control steps.
+ */
+#define GT97XX_MOVE_STEPS 16
+#define GT97XX_MAX_FOCUS_POS (1024 - 1)
+
+/*
+ * This sets the minimum granularity for the focus positions.
+ * A value of 1 gives maximum accuracy for a desired focus position
+ */
+#define GT97XX_FOCUS_STEPS 1
+
+enum vcm_giantec_reg_desc {
+ GT_IC_INFO_REG,
+ GT_RING_PD_CONTROL_REG,
+ GT_MSB_ADDR_REG,
+ GT_AAC_PRESC_REG,
+ GT_AAC_TIME_REG,
+ GT_MAX_REG,
+};
+
+struct vcm_giantec_of_data {
+ unsigned int id;
+ unsigned int regs[GT_MAX_REG];
+};
+
+static const char *const gt97xx_supply_names[] = {
+ "vin", /* Digital I/O power */
+ "vdd", /* Digital core power */
+};
+
+/* gt97xx device structure */
+struct gt97xx {
+ struct v4l2_subdev sd;
+
+ struct regulator_bulk_data supplies[ARRAY_SIZE(gt97xx_supply_names)];
+
+ struct v4l2_ctrl_handler ctrls;
+ struct v4l2_ctrl *focus;
+
+ u32 aac_mode;
+ u32 aac_timing;
+ u32 clock_presc;
+ u32 move_delay_us;
+
+ struct regmap *regmap;
+
+ const struct vcm_giantec_of_data *chip;
+};
+
+static inline struct gt97xx *sd_to_gt97xx(struct v4l2_subdev *subdev)
+{
+ return container_of(subdev, struct gt97xx, sd);
+}
+
+struct regval_list {
+ u8 reg_num;
+ u8 value;
+};
+
+struct gt97xx_aac_mode_ot_multi {
+ u32 aac_mode_enum;
+ u32 ot_multi_base100;
+};
+
+struct gt97xx_clk_presc_dividing_rate {
+ u32 clk_presc_enum;
+ u32 dividing_rate_base100;
+};
+
+static const struct gt97xx_aac_mode_ot_multi aac_mode_ot_multi[] = {
+ { 1, 48 },
+ { 2, 70 },
+ { 3, 75 },
+ { 5, 113 },
+};
+
+static const struct gt97xx_clk_presc_dividing_rate presc_dividing_rate[] = {
+ { 0, 200 }, { 1, 100 }, { 2, 50 }, { 3, 25 }, { 4, 800 }, { 5, 400 },
+};
+
+static u32 gt97xx_find_ot_multi(u32 aac_mode_param)
+{
+ u32 cur_ot_multi_base100 = 70;
+ unsigned int i;
+
+ for (i = 0; i < ARRAY_SIZE(aac_mode_ot_multi); i++) {
+ if (aac_mode_ot_multi[i].aac_mode_enum == aac_mode_param) {
+ cur_ot_multi_base100 =
+ aac_mode_ot_multi[i].ot_multi_base100;
+ }
+ }
+
+ return cur_ot_multi_base100;
+}
+
+static u32 gt97xx_find_dividing_rate(u32 presc_param)
+{
+ u32 cur_clk_dividing_rate_base100 = 100;
+ unsigned int i;
+
+ for (i = 0; i < ARRAY_SIZE(presc_dividing_rate); i++) {
+ if (presc_dividing_rate[i].clk_presc_enum == presc_param) {
+ cur_clk_dividing_rate_base100 =
+ presc_dividing_rate[i].dividing_rate_base100;
+ }
+ }
+
+ return cur_clk_dividing_rate_base100;
+}
+
+/*
+ * GT97xx_AAC_PRESC_REG & GT97xx_AAC_TIME_REG determine VCM operation time.
+ * For current VCM mode: AAC3, Operation Time would be 0.70 x Tvib.
+ * Tvib = (6.3ms + AACT[5:0] * 0.1MS) * Dividing Rate.
+ * Below is calculation of the operation delay for each step.
+ */
+static inline u32 gt97xx_cal_move_delay(u32 aac_mode_param, u32 presc_param,
+ u32 aac_timing_param)
+{
+ u32 tvib_us;
+ u32 ot_multi_base100;
+ u32 clk_dividing_rate_base100;
+
+ ot_multi_base100 = gt97xx_find_ot_multi(aac_mode_param);
+
+ clk_dividing_rate_base100 = gt97xx_find_dividing_rate(presc_param);
+
+ tvib_us = (GT97XX_TVIB_MS_BASE10 + aac_timing_param) *
+ clk_dividing_rate_base100;
+
+ return tvib_us * ot_multi_base100 / 100;
+}
+
+static int gt97xx_mod_reg(struct gt97xx *gt97xx, u32 reg, u8 mask, u8 val)
+{
+ u64 read_val;
+ int ret;
+
+ ret = cci_read(gt97xx->regmap, reg, &read_val, NULL);
+ if (ret < 0)
+ return ret;
+
+ val = ((unsigned char)read_val & ~mask) | (val & mask);
+
+ return cci_write(gt97xx->regmap, reg, val, NULL);
+}
+
+static int gt97xx_set_dac(struct gt97xx *gt97xx, u16 val)
+{
+ /* Write VCM position to registers */
+ return cci_write(gt97xx->regmap,
+ gt97xx->chip->regs[GT_MSB_ADDR_REG], val, NULL);
+}
+
+static int gt97xx_identify_module(struct gt97xx *gt97xx)
+{
+ int ret;
+ u64 ic_id;
+ struct i2c_client *client = v4l2_get_subdevdata(>97xx->sd);
+
+ ret = cci_read(gt97xx->regmap, gt97xx->chip->regs[GT_IC_INFO_REG],
+ &ic_id, NULL);
+ if (ret < 0)
+ return ret;
+
+ if (ic_id != gt97xx->chip->id) {
+ dev_err(&client->dev, "chip id mismatch: 0x%x!=0x%llx",
+ gt97xx->chip->id, ic_id);
+ return -1;
+ }
+
+ return 0;
+}
+
+static int gt97xx_init(struct gt97xx *gt97xx)
+{
+ int ret, val;
+
+ ret = gt97xx_identify_module(gt97xx);
+ if (ret < 0)
+ return ret;
+
+ /* Reset GT97xx_RING_PD_CONTROL_REG to default status 0x00 */
+ ret = cci_write(gt97xx->regmap,
+ gt97xx->chip->regs[GT_RING_PD_CONTROL_REG],
+ GT97XX_PD_MODE_OFF, NULL);
+ if (ret < 0)
+ return ret;
+
+ /*
+ * GT97xx requires waiting delay time of t_OPR
+ * after PD reset takes place.
+ */
+ fsleep(GT97XX_T_OPR_US);
+
+ /* Set GT97xx_RING_PD_CONTROL_REG to GT97xx_AAC_MODE_EN(0x01) */
+ ret = cci_write(gt97xx->regmap,
+ gt97xx->chip->regs[GT_RING_PD_CONTROL_REG],
+ GT97XX_AAC_MODE_EN, NULL);
+ if (ret < 0)
+ return ret;
+
+ /* Set AAC mode */
+ ret = gt97xx_mod_reg(gt97xx, gt97xx->chip->regs[GT_AAC_PRESC_REG],
+ GT97XX_AAC_MODE_SEL_MASK, gt97xx->aac_mode << 5);
+ if (ret < 0)
+ return ret;
+
+ /* Set clock presc */
+ if (gt97xx->clock_presc != GT97XX_CLOCK_PRE_SCALE_DEFAULT) {
+ ret = gt97xx_mod_reg(gt97xx,
+ gt97xx->chip->regs[GT_AAC_PRESC_REG],
+ GT97XX_CLOCK_PRE_SCALE_SEL_MASK,
+ gt97xx->clock_presc);
+ if (ret < 0)
+ return ret;
+ }
+
+ /* Set AAC Timing */
+ if (gt97xx->aac_timing != GT97XX_AAC_TIME_DEFAULT) {
+ ret = cci_write(gt97xx->regmap,
+ gt97xx->chip->regs[GT_AAC_TIME_REG],
+ gt97xx->aac_timing, NULL);
+ if (ret < 0)
+ return ret;
+ }
+
+ for (val = gt97xx->focus->val % GT97XX_MOVE_STEPS;
+ val <= gt97xx->focus->val; val += GT97XX_MOVE_STEPS) {
+ ret = gt97xx_set_dac(gt97xx, val);
+ if (ret)
+ return ret;
+
+ fsleep(gt97xx->move_delay_us);
+ }
+
+ return 0;
+}
+
+static int gt97xx_release(struct gt97xx *gt97xx)
+{
+ int ret, val;
+
+ val = round_down(gt97xx->focus->val, GT97XX_MOVE_STEPS);
+ for (; val >= 0; val -= GT97XX_MOVE_STEPS) {
+ ret = gt97xx_set_dac(gt97xx, val);
+ if (ret)
+ return ret;
+
+ fsleep(gt97xx->move_delay_us);
+ }
+
+ return 0;
+}
+
+static int gt97xx_power_on(struct device *dev)
+{
+ struct v4l2_subdev *sd = dev_get_drvdata(dev);
+ struct gt97xx *gt97xx = sd_to_gt97xx(sd);
+ int ret;
+
+ ret = regulator_bulk_enable(ARRAY_SIZE(gt97xx_supply_names),
+ gt97xx->supplies);
+ if (ret < 0) {
+ dev_err(dev, "failed to enable regulators\n");
+ return ret;
+ }
+
+ return ret;
+}
+
+static int gt97xx_power_off(struct device *dev)
+{
+ struct v4l2_subdev *sd = dev_get_drvdata(dev);
+ struct gt97xx *gt97xx = sd_to_gt97xx(sd);
+ int ret;
+
+ ret = regulator_bulk_disable(ARRAY_SIZE(gt97xx_supply_names),
+ gt97xx->supplies);
+ if (ret < 0) {
+ dev_err(dev, "failed to disable regulators\n");
+ return ret;
+ }
+
+ return ret;
+}
+
+static int gt97xx_runtime_suspend(struct device *dev)
+{
+ struct v4l2_subdev *sd = dev_get_drvdata(dev);
+ struct gt97xx *gt97xx = sd_to_gt97xx(sd);
+
+ gt97xx_release(gt97xx);
+ gt97xx_power_off(dev);
+
+ return 0;
+}
+
+static int gt97xx_runtime_resume(struct device *dev)
+{
+ struct v4l2_subdev *sd = dev_get_drvdata(dev);
+ struct gt97xx *gt97xx = sd_to_gt97xx(sd);
+ int ret;
+
+ ret = gt97xx_power_on(dev);
+ if (ret < 0) {
+ dev_err(dev, "failed to power_on\n");
+ return ret;
+ }
+
+ /*
+ * The datasheet refers to t_OPR that needs to be waited before sending
+ * I2C commands after power-up.
+ */
+ fsleep(GT97XX_T_OPR_US);
+
+ ret = gt97xx_init(gt97xx);
+ if (ret < 0)
+ goto disable_power;
+
+ return 0;
+
+disable_power:
+ gt97xx_power_off(dev);
+
+ return ret;
+}
+
+static int gt97xx_set_ctrl(struct v4l2_ctrl *ctrl)
+{
+ struct gt97xx *gt97xx =
+ container_of(ctrl->handler, struct gt97xx, ctrls);
+
+ if (ctrl->id == V4L2_CID_FOCUS_ABSOLUTE)
+ return gt97xx_set_dac(gt97xx, ctrl->val);
+
+ return 0;
+}
+
+static const struct v4l2_ctrl_ops gt97xx_ctrl_ops = {
+ .s_ctrl = gt97xx_set_ctrl,
+};
+
+static int gt97xx_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
+{
+ return pm_runtime_resume_and_get(sd->dev);
+}
+
+static int gt97xx_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
+{
+ return pm_runtime_put(sd->dev);
+}
+
+static const struct v4l2_subdev_internal_ops gt97xx_int_ops = {
+ .open = gt97xx_open,
+ .close = gt97xx_close,
+};
+
+static const struct v4l2_subdev_core_ops gt97xx_core_ops = {
+ .subscribe_event = v4l2_ctrl_subdev_subscribe_event,
+ .unsubscribe_event = v4l2_event_subdev_unsubscribe,
+};
+
+static const struct v4l2_subdev_ops gt97xx_ops = {
+ .core = >97xx_core_ops,
+};
+
+static int gt97xx_init_controls(struct gt97xx *gt97xx)
+{
+ struct v4l2_ctrl_handler *hdl = >97xx->ctrls;
+ const struct v4l2_ctrl_ops *ops = >97xx_ctrl_ops;
+
+ v4l2_ctrl_handler_init(hdl, 1);
+
+ gt97xx->focus = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_FOCUS_ABSOLUTE, 0,
+ GT97XX_MAX_FOCUS_POS,
+ GT97XX_FOCUS_STEPS, 0);
+
+ if (hdl->error)
+ return hdl->error;
+
+ gt97xx->sd.ctrl_handler = hdl;
+
+ return 0;
+}
+
+static int gt97xx_probe(struct i2c_client *client)
+{
+ struct device *dev = &client->dev;
+ struct gt97xx *gt97xx;
+ unsigned int i;
+ int ret;
+
+ gt97xx = devm_kzalloc(dev, sizeof(*gt97xx), GFP_KERNEL);
+ if (!gt97xx)
+ return -ENOMEM;
+
+ gt97xx->regmap = devm_cci_regmap_init_i2c(client, 8);
+ if (IS_ERR(gt97xx->regmap))
+ return dev_err_probe(dev, PTR_ERR(gt97xx->regmap),
+ "failed to init CCI\n");
+
+ /* Initialize subdev */
+ v4l2_i2c_subdev_init(>97xx->sd, client, >97xx_ops);
+
+ gt97xx->chip = of_device_get_match_data(dev);
+
+ gt97xx->aac_mode = GT97XX_AAC_MODE_DEFAULT;
+ gt97xx->aac_timing = GT97XX_AAC_TIME_DEFAULT;
+ gt97xx->clock_presc = GT97XX_CLOCK_PRE_SCALE_DEFAULT;
+
+ /* Optional indication of AAC mode select */
+ fwnode_property_read_u32(dev_fwnode(dev), "giantec,aac-mode",
+ >97xx->aac_mode);
+
+ /* Optional indication of clock pre-scale select */
+ fwnode_property_read_u32(dev_fwnode(dev), "giantec,clock-presc",
+ >97xx->clock_presc);
+
+ /* Optional indication of AAC Timing */
+ fwnode_property_read_u32(dev_fwnode(dev), "giantec,aac-timing",
+ >97xx->aac_timing);
+
+ gt97xx->move_delay_us = gt97xx_cal_move_delay(gt97xx->aac_mode,
+ gt97xx->clock_presc,
+ gt97xx->aac_timing);
+
+ for (i = 0; i < ARRAY_SIZE(gt97xx_supply_names); i++)
+ gt97xx->supplies[i].supply = gt97xx_supply_names[i];
+
+ ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(gt97xx_supply_names),
+ gt97xx->supplies);
+ if (ret < 0)
+ return dev_err_probe(dev, ret,
+ "failed to get regulators\n");
+
+ /* Initialize controls */
+ ret = gt97xx_init_controls(gt97xx);
+ if (ret)
+ goto err_free_handler;
+
+ /* Initialize subdev */
+ gt97xx->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+ gt97xx->sd.internal_ops = >97xx_int_ops;
+ gt97xx->sd.entity.function = MEDIA_ENT_F_LENS;
+
+ ret = media_entity_pads_init(>97xx->sd.entity, 0, NULL);
+ if (ret < 0)
+ goto err_free_handler;
+
+ /*power on and Initialize hw*/
+ ret = gt97xx_runtime_resume(dev);
+ if (ret < 0) {
+ dev_err(dev, "failed to power on: %d\n", ret);
+ goto err_clean_entity;
+ }
+
+ pm_runtime_set_active(dev);
+ pm_runtime_enable(dev);
+ pm_runtime_set_autosuspend_delay(dev, 1000);
+ pm_runtime_use_autosuspend(dev);
+ pm_runtime_idle(dev);
+
+ ret = v4l2_async_register_subdev(>97xx->sd);
+ if (ret < 0) {
+ dev_err(dev, "failed to register V4L2 subdev: %d", ret);
+ goto err_power_off;
+ }
+
+ return 0;
+
+err_power_off:
+ pm_runtime_disable(dev);
+err_clean_entity:
+ media_entity_cleanup(>97xx->sd.entity);
+err_free_handler:
+ v4l2_ctrl_handler_free(>97xx->ctrls);
+
+ return ret;
+}
+
+static void gt97xx_remove(struct i2c_client *client)
+{
+ struct v4l2_subdev *sd = i2c_get_clientdata(client);
+ struct gt97xx *gt97xx = sd_to_gt97xx(sd);
+
+ v4l2_async_unregister_subdev(>97xx->sd);
+ v4l2_ctrl_handler_free(>97xx->ctrls);
+ media_entity_cleanup(>97xx->sd.entity);
+ pm_runtime_disable(&client->dev);
+ if (!pm_runtime_status_suspended(&client->dev))
+ gt97xx_runtime_suspend(&client->dev);
+ pm_runtime_set_suspended(&client->dev);
+}
+
+static DEFINE_RUNTIME_DEV_PM_OPS(gt97xx_pm_ops,
+ gt97xx_runtime_suspend,
+ gt97xx_runtime_resume,
+ NULL);
+
+static const struct vcm_giantec_of_data gt9768_data = {
+ .id = GT9768_ID,
+ .regs[GT_IC_INFO_REG] = GT97XX_IC_INFO_REG,
+ .regs[GT_RING_PD_CONTROL_REG] = GT97XX_RING_PD_CONTROL_REG,
+ .regs[GT_MSB_ADDR_REG] = GT97XX_MSB_ADDR_REG,
+ .regs[GT_AAC_PRESC_REG] = GT97XX_AAC_PRESC_REG,
+ .regs[GT_AAC_TIME_REG] = GT97XX_AAC_TIME_REG,
+};
+
+static const struct vcm_giantec_of_data gt9769_data = {
+ .id = GT9769_ID,
+ .regs[GT_IC_INFO_REG] = GT97XX_IC_INFO_REG,
+ .regs[GT_RING_PD_CONTROL_REG] = GT97XX_RING_PD_CONTROL_REG,
+ .regs[GT_MSB_ADDR_REG] = GT97XX_MSB_ADDR_REG,
+ .regs[GT_AAC_PRESC_REG] = GT97XX_AAC_PRESC_REG,
+ .regs[GT_AAC_TIME_REG] = GT97XX_AAC_TIME_REG,
+};
+
+static const struct of_device_id gt97xx_of_table[] = {
+ { .compatible = "giantec,gt9768", .data = >9768_data },
+ { .compatible = "giantec,gt9769", .data = >9769_data },
+ {}
+};
+MODULE_DEVICE_TABLE(of, gt97xx_of_table);
+
+static struct i2c_driver gt97xx_i2c_driver = {
+ .driver = {
+ .name = GT97XX_NAME,
+ .pm = pm_ptr(>97xx_pm_ops),
+ .of_match_table = gt97xx_of_table,
+ },
+ .probe = gt97xx_probe,
+ .remove = gt97xx_remove,
+};
+module_i2c_driver(gt97xx_i2c_driver);
+
+MODULE_AUTHOR("Zhi Mao <zhi.mao@mediatek.com>");
+MODULE_DESCRIPTION("GT97xx VCM driver");
+MODULE_LICENSE("GPL");
--
2.25.1
^ permalink raw reply related
* Re: (subset) [PATCH RESEND v8 0/6] riscv: add initial support for Canaan Kendryte K230
From: Conor Dooley @ 2024-04-10 10:44 UTC (permalink / raw)
To: linux-riscv, Conor Dooley, Yangyu Chen
Cc: Conor Dooley, Paul Walmsley, Palmer Dabbelt, Albert Ou,
linux-kernel, Damien Le Moal, Rob Herring, Krzysztof Kozlowski,
Dan Carpenter, Guo Ren, devicetree
In-Reply-To: <tencent_22BA0425B4DF1CA1713B62E4423C1BFBF809@qq.com>
From: Conor Dooley <conor.dooley@microchip.com>
On Mon, 08 Apr 2024 00:26:58 +0800, Yangyu Chen wrote:
> K230 is an ideal chip for RISC-V Vector 1.0 evaluation now. Add initial
> support for it to allow more people to participate in building drivers
> to mainline for it.
>
> This kernel has been tested upon factory SDK [1] with
> k230_evb_only_linux_defconfig and patched mainline opensbi [2] to skip
> locked pmp and successfully booted to busybox on initrd with this log [3].
>
> [...]
Applied to riscv-soc-for-next, thanks!
[6/6] riscv: config: enable ARCH_CANAAN in defconfig
https://git.kernel.org/conor/c/cd899f85b1e4
Thanks,
Conor.
^ permalink raw reply
* [PATCH V3 2/2] arm64: dts: imx8mp-msc-sm2s: Add i2c{1,6} sda-/scl-gpios
From: Ian Ray @ 2024-04-10 10:56 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Shawn Guo,
Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam
Cc: Ian Ray, devicetree, imx, linux-arm-kernel, linux-kernel
In-Reply-To: <20240410105612.982-1-ian.ray@gehealthcare.com>
Add i2c{1,6} sda-/scl-gpios with the corresponding pinmux entries.
Signed-off-by: Ian Ray <ian.ray@gehealthcare.com>
---
.../boot/dts/freescale/imx8mp-msc-sm2s.dtsi | 22 +++++++++++++++++--
1 file changed, 20 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/boot/dts/freescale/imx8mp-msc-sm2s.dtsi b/arch/arm64/boot/dts/freescale/imx8mp-msc-sm2s.dtsi
index e794f05cf5aa..0fd5c3abcdb7 100644
--- a/arch/arm64/boot/dts/freescale/imx8mp-msc-sm2s.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mp-msc-sm2s.dtsi
@@ -200,8 +200,11 @@ ethphy1: ethernet-phy@1 {
};
&i2c1 {
- pinctrl-names = "default";
+ pinctrl-names = "default", "gpio";
pinctrl-0 = <&pinctrl_i2c1>;
+ pinctrl-1 = <&pinctrl_i2c1_gpio>;
+ scl-gpios = <&gpio5 14 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
+ sda-gpios = <&gpio5 15 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
clock-frequency = <400000>;
status = "okay";
@@ -241,8 +244,11 @@ &i2c5 {
};
&i2c6 {
- pinctrl-names = "default";
+ pinctrl-names = "default", "gpio";
pinctrl-0 = <&pinctrl_i2c6>;
+ pinctrl-1 = <&pinctrl_i2c6_gpio>;
+ scl-gpios = <&gpio3 19 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
+ sda-gpios = <&gpio3 20 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
clock-frequency = <400000>;
status = "okay";
@@ -606,6 +612,12 @@ pinctrl_i2c1: i2c1grp {
<MX8MP_IOMUXC_I2C1_SDA__I2C1_SDA 0x400001e0>;
};
+ pinctrl_i2c1_gpio: i2c1gpiogrp {
+ fsl,pins =
+ <MX8MP_IOMUXC_I2C1_SCL__GPIO5_IO14 0x1e0>,
+ <MX8MP_IOMUXC_I2C1_SDA__GPIO5_IO15 0x1e0>;
+ };
+
pinctrl_i2c2: i2c2grp {
fsl,pins =
<MX8MP_IOMUXC_I2C2_SCL__I2C2_SCL 0x400001e0>,
@@ -636,6 +648,12 @@ pinctrl_i2c6: i2c6grp {
<MX8MP_IOMUXC_SAI5_RXC__I2C6_SDA 0x400001e0>;
};
+ pinctrl_i2c6_gpio: i2c6gpiogrp {
+ fsl,pins =
+ <MX8MP_IOMUXC_SAI5_RXFS__GPIO3_IO19 0x1e0>,
+ <MX8MP_IOMUXC_SAI5_RXC__GPIO3_IO20 0x1e0>;
+ };
+
pinctrl_lcd0_backlight: lcd0-backlightgrp {
fsl,pins =
<MX8MP_IOMUXC_GPIO1_IO05__GPIO1_IO05 0x41>;
--
2.39.2
^ permalink raw reply related
* [PATCH V3 1/2] arm64: dts: imx8mp-msc-sm2s: correct i2c{1..6} pad drive strength
From: Ian Ray @ 2024-04-10 10:56 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Shawn Guo,
Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam
Cc: Ian Ray, devicetree, imx, linux-arm-kernel, linux-kernel
Adjust i2c drive strength based on latest Avnet BSP.
Signed-off-by: Ian Ray <ian.ray@gehealthcare.com>
---
.../boot/dts/freescale/imx8mp-msc-sm2s.dtsi | 24 +++++++++----------
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/arch/arm64/boot/dts/freescale/imx8mp-msc-sm2s.dtsi b/arch/arm64/boot/dts/freescale/imx8mp-msc-sm2s.dtsi
index 61c2a63efc6d..e794f05cf5aa 100644
--- a/arch/arm64/boot/dts/freescale/imx8mp-msc-sm2s.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mp-msc-sm2s.dtsi
@@ -602,38 +602,38 @@ pinctrl_flexspi0: flexspi0grp {
pinctrl_i2c1: i2c1grp {
fsl,pins =
- <MX8MP_IOMUXC_I2C1_SCL__I2C1_SCL 0x400001c3>,
- <MX8MP_IOMUXC_I2C1_SDA__I2C1_SDA 0x400001c3>;
+ <MX8MP_IOMUXC_I2C1_SCL__I2C1_SCL 0x400001e0>,
+ <MX8MP_IOMUXC_I2C1_SDA__I2C1_SDA 0x400001e0>;
};
pinctrl_i2c2: i2c2grp {
fsl,pins =
- <MX8MP_IOMUXC_I2C2_SCL__I2C2_SCL 0x400001c3>,
- <MX8MP_IOMUXC_I2C2_SDA__I2C2_SDA 0x400001c3>;
+ <MX8MP_IOMUXC_I2C2_SCL__I2C2_SCL 0x400001e0>,
+ <MX8MP_IOMUXC_I2C2_SDA__I2C2_SDA 0x400001e0>;
};
pinctrl_i2c3: i2c3grp {
fsl,pins =
- <MX8MP_IOMUXC_I2C3_SCL__I2C3_SCL 0x400001c3>,
- <MX8MP_IOMUXC_I2C3_SDA__I2C3_SDA 0x400001c3>;
+ <MX8MP_IOMUXC_I2C3_SCL__I2C3_SCL 0x400001e0>,
+ <MX8MP_IOMUXC_I2C3_SDA__I2C3_SDA 0x400001e0>;
};
pinctrl_i2c4: i2c4grp {
fsl,pins =
- <MX8MP_IOMUXC_I2C4_SCL__I2C4_SCL 0x400001c3>,
- <MX8MP_IOMUXC_I2C4_SDA__I2C4_SDA 0x400001c3>;
+ <MX8MP_IOMUXC_I2C4_SCL__I2C4_SCL 0x400001e0>,
+ <MX8MP_IOMUXC_I2C4_SDA__I2C4_SDA 0x400001e0>;
};
pinctrl_i2c5: i2c5grp {
fsl,pins =
- <MX8MP_IOMUXC_SPDIF_TX__I2C5_SCL 0x400001c3>,
- <MX8MP_IOMUXC_SPDIF_RX__I2C5_SDA 0x400001c3>;
+ <MX8MP_IOMUXC_SPDIF_TX__I2C5_SCL 0x400001e0>,
+ <MX8MP_IOMUXC_SPDIF_RX__I2C5_SDA 0x400001e0>;
};
pinctrl_i2c6: i2c6grp {
fsl,pins =
- <MX8MP_IOMUXC_SAI5_RXFS__I2C6_SCL 0x400001c3>,
- <MX8MP_IOMUXC_SAI5_RXC__I2C6_SDA 0x400001c3>;
+ <MX8MP_IOMUXC_SAI5_RXFS__I2C6_SCL 0x400001e0>,
+ <MX8MP_IOMUXC_SAI5_RXC__I2C6_SDA 0x400001e0>;
};
pinctrl_lcd0_backlight: lcd0-backlightgrp {
--
2.39.2
^ permalink raw reply related
* Re: [PATCH v2 1/2] dt-bindings: mfd: Add ROHM BD71879
From: Matti Vaittinen @ 2024-04-10 10:57 UTC (permalink / raw)
To: Conor Dooley
Cc: Andreas Kemnade, lee, robh+dt, krzysztof.kozlowski+dt, conor+dt,
devicetree, linux-kernel
In-Reply-To: <20240410-possum-lecturer-716185901aa5@spud>
On 4/10/24 13:03, Conor Dooley wrote:
> On Wed, Apr 10, 2024 at 10:57:08AM +0300, Matti Vaittinen wrote:
>> On 4/4/24 22:54, Andreas Kemnade wrote:
>>> As this chip was seen in several devices in the wild, add it.
>>>
>>> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
>>> Suggested-by: Matti Vaittinen <mazziesaccount@gmail.com>
>>> ---
>>> .../devicetree/bindings/mfd/rohm,bd71828-pmic.yaml | 7 ++++++-
>>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/mfd/rohm,bd71828-pmic.yaml b/Documentation/devicetree/bindings/mfd/rohm,bd71828-pmic.yaml
>>> index 0b62f854bf6b..07f99738fcf6 100644
>>> --- a/Documentation/devicetree/bindings/mfd/rohm,bd71828-pmic.yaml
>>> +++ b/Documentation/devicetree/bindings/mfd/rohm,bd71828-pmic.yaml
>>> @@ -17,7 +17,12 @@ description: |
>>> properties:
>>> compatible:
>>> - const: rohm,bd71828
>>> + oneOf:
>>> + - const: rohm,bd71828
>>> +
>>> + - items:
>>> + - const: rohm,bd71879
>>> + - const: rohm,bd71828
>>> reg:
>>> description:
>>
>> Am I correct, this reads as:
>>
>> Either
>> compatible = rohm,bd71828
>> or
>> compatible = rohm,bd71879, rohm,bd71828
>>
>> but not compatible = rohm,bd71879?
>
> Correct.
Thanks Conor!
I guess that if we later find out that there is a 'gizmo' in BD71828
which is not present on BD71879 - and if we write driver supporting
this, then we need to have handling for both the BD71879 and BD71828 in
this driver to ensure the fall-back wont happen. It's a bit ugly as I
think the fall-back compatible should be used only in case where the
"fall-back" HW can be guaranteed to have exactly same, or a subset of,
features of the "full board".
Well, I've been told these are similar by HW colleagues. I have my
doubts if 'similar' really is '_similar_', or if it's 'similar, except
of course...' - but let's trust the HW guys on this. We have the above
mentioned workaround if we should not have trusted...
Acked-By: Matti Vaittinen <mazziesaccount@gmail.com>
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
^ permalink raw reply
* Re: [PATCH v1] arm64: dts: imx8mm: fix missing pgc_vpu_* power domain parent
From: Vitor Soares @ 2024-04-10 11:01 UTC (permalink / raw)
To: Lucas Stach, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam
Cc: Vitor Soares, devicetree, imx, linux-arm-kernel, linux-kernel,
stable
In-Reply-To: <bd4d7198e58bd89b46a4c721546f6975b287a5fc.camel@gmail.com>
Hi Lucas,
On Tue, 2024-04-09 at 17:44 +0100, Vitor Soares wrote:
> On Tue, 2024-04-09 at 16:36 +0200, Lucas Stach wrote:
> > Am Dienstag, dem 09.04.2024 um 14:22 +0100 schrieb Vitor Soares:
> > > Hi Lucas,
> > >
> > > Thanks for your feedback.
> > >
> > > On Tue, 2024-04-09 at 11:13 +0200, Lucas Stach wrote:
> > > > Hi Vitor,
> > > >
> > > > Am Dienstag, dem 09.04.2024 um 09:58 +0100 schrieb Vitor
> > > > Soares:
> > > > > From: Vitor Soares <vitor.soares@toradex.com>
> > > > >
> > > > > The pgc_vpu_* nodes miss the reference to the power domain
> > > > > parent,
> > > > > leading the system to hang during the resume.
> > > > >
> > > > This change is not correct. The vpumix domain is controlled
> > > > through
> > > > the
> > > > imx8mm-vpu-blk-ctrl and must not be directly triggered by the
> > > > child
> > > > domains in order to guarantee proper power sequencing.
> > > >
> > > > If the sequencing is incorrect for resume, it needs to be fixed
> > > > in
> > > > the
> > > > blk-ctrl driver. I'll happily assist if you have any questions
> > > > about
> > > > this intricate mix between GPC and blk-ctrl hardware/drivers.
> > >
> > > I'm new into the topic, so I tried to follow same approach as in
> > > imx8mp
> > > DT.
> > >
> > That's a good hint, the 8MP VPU GPC node additions missed my radar.
> > The
> > direct dependency there between the GPC domains is equally wrong.
> >
> > > I also checked the imx8mq DT and it only have one domain for the
> > > VPU in the GPC. It seem blk-ctrl also dependes on pgc_vpu_* to
> > > work
> > > properly.
> > >
> > > The blk-ctrl driver hangs on imx8m_blk_ctrl_power_on() when
> > > access
> > > the
> > > ip registers for the soft reset. I tried to power-up the before
> > > the
> > > soft reset, but it didn't work.
> > >
> > The runtime_pm_get_sync() at the start of that function should
> > ensure
> > that bus GPC domain aka vpumix is powered up. Can you check if that
> > is
> > happening?
>
> I checked bc->bus_power_dev->power.runtime_status and it is
> RPM_ACTIVE.
>
> Am I looking to on the right thing? It is RPM_ACTIVE event before
> runtime_pm_get_sync().
During the probe I can see that
bus_power_dev->power.runtime_status = RPM_SUSPENDED and then vpumix is
powered up on GPC driver.
On resume routine I can't see this flow. bus_power_dev-
>power.runtime_status = RPM_ACTIVE and vpumix end up not being powered-
up.
I checked the suspend flow and the GPC tries to poweroff vpumix.
Best regards,
Vitor Soares
>
>
> >
> > Regards,
> > Lucas
> >
> > > Do you have an idea how we can address this within blk-ctrl?
> > >
> > > Best regards,
> > > Vitor
>
^ permalink raw reply
* Re: [PATCH v7 1/5] dt-bindings: interconnect: Add Qualcomm IPQ9574 support
From: Krzysztof Kozlowski @ 2024-04-10 11:15 UTC (permalink / raw)
To: Varadarajan Narayanan
Cc: andersson, konrad.dybcio, mturquette, sboyd, robh, krzk+dt,
conor+dt, djakov, dmitry.baryshkov, quic_anusha, linux-arm-msm,
linux-clk, devicetree, linux-kernel, linux-pm
In-Reply-To: <ZhZjuCkJrtPbwtS/@hu-varada-blr.qualcomm.com>
On 10/04/2024 12:02, Varadarajan Narayanan wrote:
>> Okay, so what happens if icc-clk way of generating them changes a bit?
>> It can change, why not, driver implementation is not an ABI.
>>
>>>
>>> 2. These auto-generated id-numbers have to be correctly
>>> tied to the DT nodes. Else, the relevant clocks may
>>> not get enabled.
>>
>> Sorry, I don't get, how auto generated ID number is tied to DT node.
>> What DT node?
>
> I meant the following usage for the 'interconnects' entry of the
> consumer peripheral's node.
>
> interconnects = <&gcc MASTER_ANOC_PCIE0 &gcc SLAVE_ANOC_PCIE0>,
> ^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^
> <&gcc MASTER_SNOC_PCIE0 &gcc SLAVE_SNOC_PCIE0>;
> ^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^
>
>>> Since ICC-CLK creates two ids per clock entry (one MASTER_xxx and
>>> one SLAVE_xxx), using those MASTER/SLAVE_xxx macros as indices in
>>> the below array would create holes.
>>>
>>> static int icc_ipq9574_hws[] = {
>>> [MASTER_ANOC_PCIE0] = GCC_ANOC_PCIE0_1LANE_M_CLK,
>>> [MASTER_SNOC_PCIE0] = GCC_SNOC_PCIE0_1LANE_S_CLK,
>>> [MASTER_ANOC_PCIE1] = GCC_ANOC_PCIE1_1LANE_M_CLK,
>>> [MASTER_SNOC_PCIE1] = GCC_SNOC_PCIE1_1LANE_S_CLK,
>>> . . .
>>> };
>>>
>>> Other Qualcomm drivers don't have this issue and they can
>>> directly use the MASTER/SLAVE_xxx macros.
>>
>> I understand, thanks, yet your last patch keeps adding fake IDs, means
>> IDs which are not part of ABI.
>>
>>>
>>> As the MASTER_xxx macros cannot be used, have to define a new set
>>> of macros that can be used for indices in the above array. This
>>> is the reason for the ICC_BINDING_NAME macros.
>>
>> Then maybe fix the driver, instead of adding something which is not an
>> ABI to bindings and completely skipping the actual ABI.
>
> Will remove the ICC_xxx defines from the header. And in the
> driver will change the declaration as follows. Will that be
> acceptable?
>
> static int icc_ipq9574_hws[] = {
> [MASTER_ANOC_PCIE0 / 2] = GCC_ANOC_PCIE0_1LANE_M_CLK,
What is the binding in such case? What exactly do you bind between
driver and DTS?
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH 1/2] media: dt-bindings: i2c: add Giantec GT97xx VCM driver
From: Conor Dooley @ 2024-04-10 11:27 UTC (permalink / raw)
To: Zhi Mao
Cc: Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Matthias Brugger, AngeloGioacchino Del Regno,
Philipp Zabel, Laurent Pinchart, Heiko Stuebner, Sakari Ailus,
Hans Verkuil, Hans de Goede, Tomi Valkeinen, Alain Volmat,
Paul Elder, Mehdi Djait, Andy Shevchenko, Bingbu Cao, linux-media,
devicetree, linux-kernel, linux-arm-kernel, linux-mediatek,
shengnan.wang, yaya.chang, yunkec, 10572168
In-Reply-To: <20240410104002.1197-2-zhi.mao@mediatek.com>
[-- Attachment #1: Type: text/plain, Size: 4177 bytes --]
Hey,
On Wed, Apr 10, 2024 at 06:40:01PM +0800, Zhi Mao wrote:
> Add YAML device tree binding for GT97xx VCM driver,
Please don't mention drivers here, bindings are for hardware.
> and the relevant MAINTAINERS entries.
>
> Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
> ---
> .../bindings/media/i2c/giantec,gt97xx.yaml | 91 +++++++++++++++++++
> 1 file changed, 91 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/i2c/giantec,gt97xx.yaml
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/giantec,gt97xx.yaml b/Documentation/devicetree/bindings/media/i2c/giantec,gt97xx.yaml
> new file mode 100644
> index 000000000000..8c9f1eb4dac8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/giantec,gt97xx.yaml
> @@ -0,0 +1,91 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright (c) 2020 MediaTek Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/i2c/giantec,gt97xx.yaml#
Filename patching compatible please.
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Giantec Semiconductor, Crop. GT97xx Voice Coil Motor (VCM)
> +
> +maintainers:
> + - Zhi Mao <zhi.mao@mediatek.com>
> +
> +description: |-
> + The Giantec GT97xx is a 10-bit DAC with current sink capability.
> + The DAC is controlled via I2C bus that operates at clock rates up to 1MHz.
> + This chip integrates Advanced Actuator Control (AAC) technology
> + and is intended for driving voice coil lens in camera modules.
> +
> +properties:
> + compatible:
> + enum:
> + - giantec,gt9768 # for GT9768 VCM
> + - giantec,gt9769 # for GT9769 VCM
I don't think these comments are needed, they should be clear from the
compatibles, no?
> +
> + reg:
> + maxItems: 1
> +
> + vin-supply: true
> +
> + vdd-supply: true
> +
> + giantec,aac-mode:
> + description:
> + Indication of AAC mode select.
> + $ref: /schemas/types.yaml#/definitions/uint32
> + enum:
> + - 1 # AAC2 mode(operation time# 0.48 x Tvib)
> + - 2 # AAC3 mode(operation time# 0.70 x Tvib)
> + - 3 # AAC4 mode(operation time# 0.75 x Tvib)
> + - 5 # AAC8 mode(operation time# 1.13 x Tvib)
I dislike these enum based properties and I would rather this either be
the values themselves (0.48, 0.70 etc).
> + default: 2
> +
> + giantec,aac-timing:
> + description:
> + Number of AAC Timing count that controlled by one 6-bit period of
> + vibration register AACT[5:0], the unit of which is 100 us.
Then the property should be in a standard unit of time, not "random" hex
numbers that correspond to register values.
> + $ref: /schemas/types.yaml#/definitions/uint32
> + default: 0x20
> + minimum: 0x00
> + maximum: 0x3f
> +
> + giantec,clock-presc:
> + description:
> + Indication of VCM internal clock dividing rate select, as one multiple
> + factor to calculate VCM ring periodic time Tvib.
> + $ref: /schemas/types.yaml#/definitions/uint32
> + enum:
> + - 0 # Dividing Rate - 2
> + - 1 # Dividing Rate - 1
> + - 2 # Dividing Rate - 1/2
> + - 3 # Dividing Rate - 1/4
> + - 4 # Dividing Rate - 8
> + - 5 # Dividing Rate - 4
Same here, you should not need these comments explaining the values, use
an enum with meaningful values please.
> + default: 1
> +
> +required:
> + - compatible
> + - reg
> + - vin-supply
> + - vdd-supply
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> +
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + vcm@c {
"vcm" is not a generic node-name, can you use one please?
Look around similar bindings or at the dt spec for generic node-names.
Thanks,
Conor.
> + compatible = "giantec,gt9768";
> + reg = <0x0c>;
> +
> + vin-supply = <>97xx_vin>;
> + vdd-supply = <>97xx_vdd>;
> + giantec,aac-timing = <0x20>;
> + };
> + };
> +
> +...
> --
> 2.25.1
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH 1/2] media: dt-bindings: i2c: add Giantec GT97xx VCM driver
From: Sakari Ailus @ 2024-04-10 11:41 UTC (permalink / raw)
To: Conor Dooley
Cc: Zhi Mao, Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Matthias Brugger, AngeloGioacchino Del Regno,
Philipp Zabel, Laurent Pinchart, Heiko Stuebner, Hans Verkuil,
Hans de Goede, Tomi Valkeinen, Alain Volmat, Paul Elder,
Mehdi Djait, Andy Shevchenko, Bingbu Cao, linux-media, devicetree,
linux-kernel, linux-arm-kernel, linux-mediatek, shengnan.wang,
yaya.chang, yunkec, 10572168
In-Reply-To: <20240410-rice-fringe-4ae992217a2f@spud>
Hi Conor, Zhi,
On Wed, Apr 10, 2024 at 12:27:07PM +0100, Conor Dooley wrote:
> Hey,
>
> On Wed, Apr 10, 2024 at 06:40:01PM +0800, Zhi Mao wrote:
> > Add YAML device tree binding for GT97xx VCM driver,
>
> Please don't mention drivers here, bindings are for hardware.
>
> > and the relevant MAINTAINERS entries.
> >
> > Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
> > ---
> > .../bindings/media/i2c/giantec,gt97xx.yaml | 91 +++++++++++++++++++
> > 1 file changed, 91 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/media/i2c/giantec,gt97xx.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/media/i2c/giantec,gt97xx.yaml b/Documentation/devicetree/bindings/media/i2c/giantec,gt97xx.yaml
> > new file mode 100644
> > index 000000000000..8c9f1eb4dac8
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/i2c/giantec,gt97xx.yaml
> > @@ -0,0 +1,91 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +# Copyright (c) 2020 MediaTek Inc.
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/media/i2c/giantec,gt97xx.yaml#
>
> Filename patching compatible please.
>
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Giantec Semiconductor, Crop. GT97xx Voice Coil Motor (VCM)
> > +
> > +maintainers:
> > + - Zhi Mao <zhi.mao@mediatek.com>
> > +
> > +description: |-
> > + The Giantec GT97xx is a 10-bit DAC with current sink capability.
> > + The DAC is controlled via I2C bus that operates at clock rates up to 1MHz.
> > + This chip integrates Advanced Actuator Control (AAC) technology
> > + and is intended for driving voice coil lens in camera modules.
> > +
> > +properties:
> > + compatible:
> > + enum:
> > + - giantec,gt9768 # for GT9768 VCM
> > + - giantec,gt9769 # for GT9769 VCM
>
> I don't think these comments are needed, they should be clear from the
> compatibles, no?
>
> > +
> > + reg:
> > + maxItems: 1
> > +
> > + vin-supply: true
> > +
> > + vdd-supply: true
> > +
> > + giantec,aac-mode:
> > + description:
> > + Indication of AAC mode select.
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + enum:
> > + - 1 # AAC2 mode(operation time# 0.48 x Tvib)
> > + - 2 # AAC3 mode(operation time# 0.70 x Tvib)
> > + - 3 # AAC4 mode(operation time# 0.75 x Tvib)
> > + - 5 # AAC8 mode(operation time# 1.13 x Tvib)
>
> I dislike these enum based properties and I would rather this either be
> the values themselves (0.48, 0.70 etc).
>
> > + default: 2
> > +
> > + giantec,aac-timing:
> > + description:
> > + Number of AAC Timing count that controlled by one 6-bit period of
> > + vibration register AACT[5:0], the unit of which is 100 us.
>
> Then the property should be in a standard unit of time, not "random" hex
> numbers that correspond to register values.
>
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + default: 0x20
> > + minimum: 0x00
> > + maximum: 0x3f
> > +
> > + giantec,clock-presc:
> > + description:
> > + Indication of VCM internal clock dividing rate select, as one multiple
> > + factor to calculate VCM ring periodic time Tvib.
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + enum:
> > + - 0 # Dividing Rate - 2
> > + - 1 # Dividing Rate - 1
> > + - 2 # Dividing Rate - 1/2
> > + - 3 # Dividing Rate - 1/4
> > + - 4 # Dividing Rate - 8
> > + - 5 # Dividing Rate - 4
>
> Same here, you should not need these comments explaining the values, use
> an enum with meaningful values please.
>
> > + default: 1
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - vin-supply
> > + - vdd-supply
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > +
> > + i2c {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + vcm@c {
>
> "vcm" is not a generic node-name, can you use one please?
> Look around similar bindings or at the dt spec for generic node-names.
"camera-lens" would seem to be widely used. I can post patches to change
some of the rest out there that aren't aligned.
>
> Thanks,
> Conor.
>
> > + compatible = "giantec,gt9768";
> > + reg = <0x0c>;
> > +
> > + vin-supply = <>97xx_vin>;
> > + vdd-supply = <>97xx_vdd>;
> > + giantec,aac-timing = <0x20>;
> > + };
> > + };
> > +
> > +...
--
Kind regards,
Sakari Ailus
^ permalink raw reply
* Re: [PATCH 4/6] drm/msm/adreno: Implement SMEM-based speed bin
From: Konrad Dybcio @ 2024-04-10 11:42 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Bjorn Andersson, Rob Clark, Abhinav Kumar, Sean Paul,
Marijn Suijten, David Airlie, Daniel Vetter, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-arm-msm, linux-kernel,
dri-devel, freedreno, devicetree, Neil Armstrong
In-Reply-To: <scvwfj44z3wpp7phvesfwjuv5awtlkwby2vvrpaq4i5fircrt3@i3ebya4iymf3>
On 4/6/24 05:23, Dmitry Baryshkov wrote:
> On Fri, Apr 05, 2024 at 10:41:32AM +0200, Konrad Dybcio wrote:
>> On recent (SM8550+) Snapdragon platforms, the GPU speed bin data is
>> abstracted through SMEM, instead of being directly available in a fuse.
>>
>> Add support for SMEM-based speed binning, which includes getting
>> "feature code" and "product code" from said source and parsing them
>> to form something that lets us match OPPs against.
>>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
[...]
>> - return nvmem_cell_read_variable_le_u32(dev, "speed_bin", speedbin);
>> + u32 fcode, pcode;
>> + int ret;
>> +
>> + /* Try reading the speedbin via a nvmem cell first */
>> + ret = nvmem_cell_read_variable_le_u32(dev, "speed_bin", speedbin);
>> + if (!ret && ret != -EINVAL)
>
> This is always false.
Right, a better condition would be (!ret || ret != EINVAL)..
>
>> + return ret;
>> +
>> + ret = qcom_smem_get_feature_code(&fcode);
>> + if (ret) {
>> + dev_err(dev, "Couldn't get feature code from SMEM!\n");
>> + return ret;
>
> This brings in QCOM_SMEM dependency (which is not mentioned in the
> Kconfig). Please keep iMX5 hardware in mind, so the dependency should be
> optional. Respective functions should be stubbed in the header.
OK, I had this in mind early on, but forgot to actually impl it.
>
>> + }
>> +
>> + ret = qcom_smem_get_product_code(&pcode);
>> + if (ret) {
>> + dev_err(dev, "Couldn't get product code from SMEM!\n");
>> + return ret;
>> + }
>> +
>> + /* Don't consider fcode for external feature codes */
>> + if (fcode <= SOCINFO_FC_EXT_RESERVE)
>> + fcode = SOCINFO_FC_UNKNOWN;
>> +
>> + *speedbin = FIELD_PREP(ADRENO_SKU_ID_PCODE, pcode) |
>> + FIELD_PREP(ADRENO_SKU_ID_FCODE, fcode);
>
> What about just asking the qcom_smem for the 'gpu_bin' and hiding gory
> details there? It almost feels that handling raw PCODE / FCODE here is
> too low-level and a subject to change depending on the socinfo format.
No, the FCODE & PCODE can be interpreted differently across consumers.
>
>> +
>> + return ret;
>> }
>>
>> int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
>> @@ -1098,9 +1129,9 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
>> devm_pm_opp_set_clkname(dev, "core");
>> }
>>
>> - if (adreno_read_speedbin(dev, &speedbin) || !speedbin)
>> + if (adreno_read_speedbin(adreno_gpu, dev, &speedbin) || !speedbin)
>> speedbin = 0xffff;
>> - adreno_gpu->speedbin = (uint16_t) (0xffff & speedbin);
>
> the &= 0xffff should probably go to the adreno_read_speedbin / nvmem
> case. WDYT?
Ok, I can keep it, though realistically if this ever does anything
useful, it likely means the dt is wrong
Konrad
^ permalink raw reply
* Re: [PATCH 3/6] drm/msm/adreno: Allow specifying default speedbin value
From: Konrad Dybcio @ 2024-04-10 11:47 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Rob Clark, Bjorn Andersson, Abhinav Kumar, Sean Paul,
Marijn Suijten, David Airlie, Daniel Vetter, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-arm-msm, linux-kernel,
dri-devel, freedreno, devicetree, Neil Armstrong
In-Reply-To: <CAA8EJprAcBDEwYeGJZ=JX2qkS4wQEraXsgCiCyZU3z1ksuxivw@mail.gmail.com>
On 4/9/24 20:31, Dmitry Baryshkov wrote:
> On Tue, 9 Apr 2024 at 21:27, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>>
>>
>>
>> On 4/9/24 20:15, Dmitry Baryshkov wrote:
>>> On Tue, Apr 09, 2024 at 08:07:56PM +0200, Konrad Dybcio wrote:
>>>>
>>>>
>>>> On 4/9/24 20:04, Dmitry Baryshkov wrote:
>>>>> On Tue, Apr 09, 2024 at 10:12:00AM -0700, Rob Clark wrote:
>>>>>> On Tue, Apr 9, 2024 at 8:23 AM Dmitry Baryshkov
>>>>>> <dmitry.baryshkov@linaro.org> wrote:
>>>>>>>
>>>>>>> On Tue, Apr 09, 2024 at 05:12:46PM +0200, Konrad Dybcio wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 4/6/24 04:56, Dmitry Baryshkov wrote:
>>>>>>>>> On Fri, Apr 05, 2024 at 10:41:31AM +0200, Konrad Dybcio wrote:
>>>>>>>>>> From: Neil Armstrong <neil.armstrong@linaro.org>
>>>>>>>>>>
>>>>>>>>>> Usually, speedbin 0 is the "super SKU", a.k.a the one which can clock
>>>>>>>>>> the highest. Falling back to it when things go wrong is largely
>>>>>>>>>> suboptimal, as more often than not, the top frequencies are not
>>>>>>>>>> supposed to work on other bins.
>>>>>>>>>
>>>>>>>>> Isn't it better to just return an error here instead of trying to guess
>>>>>>>>> which speedbin to use?
>>>>>>>>
>>>>>>>> Not sure. I'd rather better compatibility for e.g. booting up a new
>>>>>>>> laptop with just dt.
>>>>>>>
>>>>>>> New speedbin can have lower max speed, so by attempting to run it at
>>>>>>> higher freq you might be breaking it.
>>>>>>
>>>>>> Usually there are some OPPs in common to all speedbins, so picking a
>>>>>> freq from that set would seem like the safe thing to do
>>>>>
>>>>> Well, the issue is about an uknown speed bin. So in theory we know
>>>>> nothing about the set of speeds itsupports. My point is that we should
>>>>> simplfy fail in such case.
>>>>
>>>> Or we could allow e.g. the lowest frequency (or 2) which if often shared
>>>> across the board to work, giving a compromise between OOBE and sanity
>>>
>>> That's also an option. But we should not be using existing speed table for
>>> the unknown bin.
>>
>> I derived this logic from msm-5.15 where it's "intended behavior".. I
>> suppose we can do better as you said though
>>
>> There have been cases in the past where the default speed bin ended up
>> having a higher max freq than subsequent ones, and I don't think I can
>> trust this product/feature code approach to guarantee this never
>> happening again.
>>
>> So. I think sticking to a single lowest freq and printing a big red line
>> in dmesg makes sense here
>
> Make 0x80 the default supported-hw, make sure that the lowest freq has
> 0xff. Plus big-red-line.
> And hope that we'll never see 16 speed bins for the hardware.
opp-supported-hw is a u32 bitmask fwiw
I was thinking, either 0xffffffff or some driver-side hackery
(dev_pm_opp_enable).
Perhaps I'd be more in favor of the latter, as putting meaningless gobblygoo
in dt is not good
Konrad
^ permalink raw reply
* Re: [PATCH v7 1/5] dt-bindings: interconnect: Add Qualcomm IPQ9574 support
From: Konrad Dybcio @ 2024-04-10 11:48 UTC (permalink / raw)
To: Krzysztof Kozlowski, Varadarajan Narayanan
Cc: andersson, mturquette, sboyd, robh, krzk+dt, conor+dt, djakov,
dmitry.baryshkov, quic_anusha, linux-arm-msm, linux-clk,
devicetree, linux-kernel, linux-pm
In-Reply-To: <70d0afa7-4990-4180-8dfa-cdf267e4c7a2@kernel.org>
On 4/10/24 13:15, Krzysztof Kozlowski wrote:
> On 10/04/2024 12:02, Varadarajan Narayanan wrote:
>>> Okay, so what happens if icc-clk way of generating them changes a bit?
>>> It can change, why not, driver implementation is not an ABI.
>>>
>>>>
>>>> 2. These auto-generated id-numbers have to be correctly
>>>> tied to the DT nodes. Else, the relevant clocks may
>>>> not get enabled.
>>>
>>> Sorry, I don't get, how auto generated ID number is tied to DT node.
>>> What DT node?
>>
>> I meant the following usage for the 'interconnects' entry of the
>> consumer peripheral's node.
>>
>> interconnects = <&gcc MASTER_ANOC_PCIE0 &gcc SLAVE_ANOC_PCIE0>,
>> ^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^
>> <&gcc MASTER_SNOC_PCIE0 &gcc SLAVE_SNOC_PCIE0>;
>> ^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^
>>
>>>> Since ICC-CLK creates two ids per clock entry (one MASTER_xxx and
>>>> one SLAVE_xxx), using those MASTER/SLAVE_xxx macros as indices in
>>>> the below array would create holes.
>>>>
>>>> static int icc_ipq9574_hws[] = {
>>>> [MASTER_ANOC_PCIE0] = GCC_ANOC_PCIE0_1LANE_M_CLK,
>>>> [MASTER_SNOC_PCIE0] = GCC_SNOC_PCIE0_1LANE_S_CLK,
>>>> [MASTER_ANOC_PCIE1] = GCC_ANOC_PCIE1_1LANE_M_CLK,
>>>> [MASTER_SNOC_PCIE1] = GCC_SNOC_PCIE1_1LANE_S_CLK,
>>>> . . .
>>>> };
>>>>
>>>> Other Qualcomm drivers don't have this issue and they can
>>>> directly use the MASTER/SLAVE_xxx macros.
>>>
>>> I understand, thanks, yet your last patch keeps adding fake IDs, means
>>> IDs which are not part of ABI.
>>>
>>>>
>>>> As the MASTER_xxx macros cannot be used, have to define a new set
>>>> of macros that can be used for indices in the above array. This
>>>> is the reason for the ICC_BINDING_NAME macros.
>>>
>>> Then maybe fix the driver, instead of adding something which is not an
>>> ABI to bindings and completely skipping the actual ABI.
>>
>> Will remove the ICC_xxx defines from the header. And in the
>> driver will change the declaration as follows. Will that be
>> acceptable?
>>
>> static int icc_ipq9574_hws[] = {
>> [MASTER_ANOC_PCIE0 / 2] = GCC_ANOC_PCIE0_1LANE_M_CLK,
>
> What is the binding in such case? What exactly do you bind between
> driver and DTS?
I think what Krzysztof is trying to say here is "the icc-clk API is tragic"
and the best solution would be to make it such that the interconnect indices
are set explicitly, instead of (master, slave), (master, slave) etc.
Does that sound good, Krzysztof?
Konrad
^ permalink raw reply
* Re: [PATCH 3/4] dt-bindings: thermal: loongson,ls2k-thermal: Fix incorrect compatible definition
From: Binbin Zhou @ 2024-04-10 11:50 UTC (permalink / raw)
To: Conor Dooley
Cc: Binbin Zhou, Huacai Chen, Rafael J . Wysocki, Daniel Lezcano,
Amit Kucheria, Zhang Rui, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Huacai Chen, loongson-kernel, linux-pm, devicetree,
Yinbo Zhu, WANG Xuerui, loongarch
In-Reply-To: <20240410-wrecker-procreate-11e0915f7bbd@spud>
On Wed, Apr 10, 2024 at 6:00 PM Conor Dooley <conor@kernel.org> wrote:
>
> On Wed, Apr 10, 2024 at 05:49:02PM +0800, Binbin Zhou wrote:
> > The temperature output register of the Loongson-2K2000 is defined in the
> > chip configuration domain, which is different from the Loongson-2K1000,
> > so it can't be fallbacked.
> >
> > We need to use two groups of registers to describe it: the first group
> > is the high and low temperature threshold setting register; the second
> > group is the temperature output register.
> >
> > It is true that this fix will cause ABI corruption, but it is necessary
> > otherwise the Loongson-2K2000 temperature sensor will not work properly.
> >
> > Fixes: 72684d99a854 ("thermal: dt-bindings: add loongson-2 thermal")
> > Cc: Yinbo Zhu <zhuyinbo@loongson.cn>
> > Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
> > ---
> > .../thermal/loongson,ls2k-thermal.yaml | 22 +++++++++++++++++--
> > 1 file changed, 20 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/thermal/loongson,ls2k-thermal.yaml b/Documentation/devicetree/bindings/thermal/loongson,ls2k-thermal.yaml
> > index 9748a479dcd4..a25b42702788 100644
> > --- a/Documentation/devicetree/bindings/thermal/loongson,ls2k-thermal.yaml
> > +++ b/Documentation/devicetree/bindings/thermal/loongson,ls2k-thermal.yaml
> > @@ -18,14 +18,15 @@ properties:
> > oneOf:
> > - enum:
> > - loongson,ls2k1000-thermal
> > + - loongson,ls2k2000-thermal
> > - items:
> > - enum:
> > - loongson,ls2k0500-thermal
> > - - loongson,ls2k2000-thermal
> > - const: loongson,ls2k1000-thermal
> >
> > reg:
> > - maxItems: 1
> > + minItems: 1
> > + maxItems: 2
> >
> > interrupts:
> > maxItems: 1
> > @@ -41,6 +42,23 @@ required:
> >
> > unevaluatedProperties: false
> >
> > +if:
> > + properties:
> > + compatible:
> > + contains:
> > + enum:
> > + - loongson,ls2k2000-thermal
> > +
> > +then:
> > + properties:
> > + reg:
> > + maxItems: 2
>
> You need a minItems: 2 here also, so that providing only the original
> reg is not allowed anymore.
Hi Conor:
Indeed, it is necessary that the whole description is more rigorous.
I'll fix it in the next version.
Thanks.
Binbin
>
> Cheers,
> Conor.
>
> > +
> > +else:
> > + properties:
> > + reg:
> > + maxItems: 1
> > +
> > examples:
> > - |
> > #include <dt-bindings/interrupt-controller/irq.h>
> > --
> > 2.43.0
> >
^ permalink raw reply
* Re: [PATCH 1/3] arm64: dts: ti: k3-am62-main: Fix the reg-range for dma-controller
From: Jayesh Choudhary @ 2024-04-10 11:53 UTC (permalink / raw)
To: Kumar, Udit, nm, vigneshr, bb, devicetree
Cc: kristo, robh, krzk+dt, conor+dt, linux-kernel, linux-arm-kernel
In-Reply-To: <b77128a1-8c61-49c2-81a4-c0811c5c8e3c@ti.com>
Hello Udit,
On 10/04/24 15:36, Kumar, Udit wrote:
> Thanks for this fix Jayesh
>
> On 4/5/2024 2:22 PM, Jayesh Choudhary wrote:
>> The TX Channel Realtime Registers region 'tchanrt' is 128KB and Ring
>> Realtime Registers region 'ringrt' is 2MB as shown in memory map in
>> the TRM[0]. So fix ranges for those reg-regions.
>
> Minor one, but it will be better to say that you are fixing these
> register ranges for PKTDMA .
>
> Something like that
The cover-letter[0] for the series does mention that the changes are for
DMASS0_PKTDMA_RINGRT and DMASS0_PKTDMA_TCHANRT with the current and
correct region size.
It also mentions the section and the Table from where we can check them.
{*** section for Main Memory Map (Table 2-1)}
[0]: https://lore.kernel.org/all/20240405085208.32227-1-j-choudhary@ti.com/
It was same check across all platform so added in cover letter.
But I will add that in each patch.
Thanks,
Jayesh
>
> For PKTDMA, The TX Channel Realtime Registers region 'tchanrt' ....
>
> Also, it will help reviewer if you can mention which section in TRM[0],
> holding memory map .
>
> If you are doing v2 then please take care of above in commit message.
>
> rest for whole series
>
> LGTM
>
>
>
>> [0]: <https://www.ti.com/lit/pdf/spruiv7>
>>
>> Fixes: c37c58fdeb8a ("arm64: dts: ti: k3-am62: Add more peripheral
>> nodes")
>> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
>> ---
>> arch/arm64/boot/dts/ti/k3-am62-main.dtsi | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm64/boot/dts/ti/k3-am62-main.dtsi
>> b/arch/arm64/boot/dts/ti/k3-am62-main.dtsi
>> index e9cffca073ef..e10cc9fc0b10 100644
>> --- a/arch/arm64/boot/dts/ti/k3-am62-main.dtsi
>> +++ b/arch/arm64/boot/dts/ti/k3-am62-main.dtsi
>> @@ -141,8 +141,8 @@ main_pktdma: dma-controller@485c0000 {
>> compatible = "ti,am64-dmss-pktdma";
>> reg = <0x00 0x485c0000 0x00 0x100>,
>> <0x00 0x4a800000 0x00 0x20000>,
>> - <0x00 0x4aa00000 0x00 0x40000>,
>> - <0x00 0x4b800000 0x00 0x400000>,
>> + <0x00 0x4aa00000 0x00 0x20000>,
>> + <0x00 0x4b800000 0x00 0x200000>,
>> <0x00 0x485e0000 0x00 0x10000>,
>> <0x00 0x484a0000 0x00 0x2000>,
>> <0x00 0x484c0000 0x00 0x2000>,
^ permalink raw reply
* Re: [PATCH] dt-bindings: firmware: arm,scmi: Update examples for protocol@13
From: Rob Herring @ 2024-04-10 11:56 UTC (permalink / raw)
To: Ulf Hansson
Cc: Sudeep Holla, Cristian Marussi, Krzysztof Kozlowski, devicetree,
linux-arm-kernel
In-Reply-To: <CAPDyKFpthS4eZKTTYJ=aoE=FbExD6kd_bcc_WRNLh=2BzNVHPw@mail.gmail.com>
On Thu, Apr 04, 2024 at 12:52:08PM +0200, Ulf Hansson wrote:
> On Wed, 3 Apr 2024 at 15:53, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Wed, Apr 03, 2024 at 01:11:06PM +0200, Ulf Hansson wrote:
> > > Recently we extended the binding for protocol@13 to allow it to be modelled
> > > as a generic performance domain. In a way to promote using the new binding,
> > > let's update the examples.
> > >
> >
> > Does it make sense to keep one DVFS example with #clock-cells until we
> > mark it as deprecated ? Otherwise it may be confusing as the binding still
> > lists. Or leave some comment in the example or something, I am open for
> > suggestions.
>
> I am certainly fine with either way!
>
> However, if we intend to make #clock-cells deprecated down the road,
> maybe it's better to start avoiding the use of it already now. That
> said, what do you think of following up $subject patch with an update
> to Juno's dts(i) to move to #power-domains-cells too? That would mean
> we get a nice reference for how to use this too.
>
> >
> > Other than that,
> >
> > Acked-by: Sudeep Holla <sudeep.holla@arm.com>
>
> Are you picking this via your scmi tree, or which route is this going?
Please take via SCMI tree.
Acked-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* Re: [PATCH v2 02/18] PCI: endpoint: Introduce pci_epc_map_align()
From: Kishon Vijay Abraham I @ 2024-04-10 11:57 UTC (permalink / raw)
To: Niklas Cassel, Damien Le Moal
Cc: Manivannan Sadhasivam, Lorenzo Pieralisi, Kishon Vijay Abraham I,
Shawn Lin, Krzysztof Wilczyński, Bjorn Helgaas,
Heiko Stuebner, linux-pci, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, devicetree, linux-rockchip, linux-arm-kernel,
Rick Wertenbroek, Wilfred Mallawa
In-Reply-To: <Zg_skLWnl04-pxkn@ryzen>
On 4/5/2024 5:50 PM, Niklas Cassel wrote:
> On Thu, Apr 04, 2024 at 11:43:47AM +0900, Damien Le Moal wrote:
>> On 4/3/24 21:33, Kishon Vijay Abraham I wrote:
>>> Hi Damien,
>>>
>>> On 3/30/2024 9:49 AM, Damien Le Moal wrote:
>>>> Some endpoint controllers have requirements on the alignment of the
>>>> controller physical memory address that must be used to map a RC PCI
>>>> address region. For instance, the rockchip endpoint controller uses
>>>> at most the lower 20 bits of a physical memory address region as the
>>>> lower bits of an RC PCI address. For mapping a PCI address region of
>>>> size bytes starting from pci_addr, the exact number of address bits
>>>> used is the number of address bits changing in the address range
>>>> [pci_addr..pci_addr + size - 1].
>>>>
>>>> For this example, this creates the following constraints:
>>>> 1) The offset into the controller physical memory allocated for a
>>>> mapping depends on the mapping size *and* the starting PCI address
>>>> for the mapping.
>>>> 2) A mapping size cannot exceed the controller windows size (1MB) minus
>>>> the offset needed into the allocated physical memory, which can end
>>>> up being a smaller size than the desired mapping size.
>>>>
>>>> Handling these constraints independently of the controller being used in
>>>> a PCI EP function driver is not possible with the current EPC API as
>>>> it only provides the ->align field in struct pci_epc_features.
>>>> Furthermore, this alignment is static and does not depend on a mapping
>>>> pci address and size.
>>>>
>>>> Solve this by introducing the function pci_epc_map_align() and the
>>>> endpoint controller operation ->map_align to allow endpoint function
>>>> drivers to obtain the size and the offset into a controller address
>>>> region that must be used to map an RC PCI address region. The size
>>>> of the physical address region provided by pci_epc_map_align() can then
>>>> be used as the size argument for the function pci_epc_mem_alloc_addr().
>>>> The offset into the allocated controller memory can be used to
>>>> correctly handle data transfers. Of note is that pci_epc_map_align() may
>>>> indicate upon return a mapping size that is smaller (but not 0) than the
>>>> requested PCI address region size. For such case, an endpoint function
>>>> driver must handle data transfers in fragments.
>>>>
>>>> The controller operation ->map_align is optional: controllers that do
>>>> not have any address alignment constraints for mapping a RC PCI address
>>>> region do not need to implement this operation. For such controllers,
>>>> pci_epc_map_align() always returns the mapping size as equal
>>>> to the requested size and an offset equal to 0.
>>>>
>>>> The structure pci_epc_map is introduced to represent a mapping start PCI
>>>> address, size and the size and offset into the controller memory needed
>>>> for mapping the PCI address region.
>>>>
>>>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
>>>> ---
>>>> drivers/pci/endpoint/pci-epc-core.c | 66 +++++++++++++++++++++++++++++
>>>> include/linux/pci-epc.h | 33 +++++++++++++++
>>>> 2 files changed, 99 insertions(+)
>>>>
>>>> diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
>>>> index 754afd115bbd..37758ca91d7f 100644
>>>> --- a/drivers/pci/endpoint/pci-epc-core.c
>>>> +++ b/drivers/pci/endpoint/pci-epc-core.c
>>>> @@ -433,6 +433,72 @@ void pci_epc_unmap_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>>>> }
>>>> EXPORT_SYMBOL_GPL(pci_epc_unmap_addr);
>>>>
>>>> +/**
>>>> + * pci_epc_map_align() - Get the offset into and the size of a controller memory
>>>> + * address region needed to map a RC PCI address region
>>>> + * @epc: the EPC device on which address is allocated
>>>> + * @func_no: the physical endpoint function number in the EPC device
>>>> + * @vfunc_no: the virtual endpoint function number in the physical function
>>>> + * @pci_addr: PCI address to which the physical address should be mapped
>>>> + * @size: the size of the mapping starting from @pci_addr
>>>> + * @map: populate here the actual size and offset into the controller memory
>>>> + * that must be allocated for the mapping
>>>> + *
>>>> + * Invoke the controller map_align operation to obtain the size and the offset
>>>> + * into a controller address region that must be allocated to map @size
>>>> + * bytes of the RC PCI address space starting from @pci_addr.
>>>> + *
>>>> + * The size of the mapping that can be handled by the controller is indicated
>>>> + * using the pci_size field of @map. This size may be smaller than the requested
>>>> + * @size. In such case, the function driver must handle the mapping using
>>>> + * several fragments. The offset into the controller memory for the effective
>>>> + * mapping of the @pci_addr..@pci_addr+@map->pci_size address range is indicated
>>>> + * using the map_ofst field of @map.
>>>> + */
>>>> +int pci_epc_map_align(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>>>> + u64 pci_addr, size_t size, struct pci_epc_map *map)
>>>> +{
>>>> + const struct pci_epc_features *features;
>>>> + size_t mask;
>>>> + int ret;
>>>> +
>>>> + if (!pci_epc_function_is_valid(epc, func_no, vfunc_no))
>>>> + return -EINVAL;
>>>> +
>>>> + if (!size || !map)
>>>> + return -EINVAL;
>>>> +
>>>> + memset(map, 0, sizeof(*map));
>>>> + map->pci_addr = pci_addr;
>>>> + map->pci_size = size;
>>>> +
>>>> + if (epc->ops->map_align) {
>>>> + mutex_lock(&epc->lock);
>>>> + ret = epc->ops->map_align(epc, func_no, vfunc_no, map);
>>>> + mutex_unlock(&epc->lock);
>>>> + return ret;
>>>> + }
>>>> +
>>>> + /*
>>>> + * Assume a fixed alignment constraint as specified by the controller
>>>> + * features.
>>>> + */
>>>> + features = pci_epc_get_features(epc, func_no, vfunc_no);
>>>> + if (!features || !features->align) {
>>>> + map->map_pci_addr = pci_addr;
>>>> + map->map_size = size;
>>>> + map->map_ofst = 0;
>>>> + }
>>>
>>> The 'align' of pci_epc_features was initially added only to address the
>>> inbound ATU constraints. This is also added as comment in [1]. The PCI
>>> address restrictions (only fixed alignment constraint) were handled by
>>> the host side driver and depends on the connected endpoint device
>>> (atleast it was like that for pci_endpoint_test.c [2]).
>>> So pci-epf-test.c used the 'align' in pci_epc_features only as part of
>>> pci_epf_alloc_space().
>>>
>>> Though I have abused 'align' of pci_epc_features in pci-epf-ntb.c using
>>> it out of pci_epf_alloc_space(), I think we should keep the 'align' of
>>> pci_epc_features only within pci_epf_alloc_space() and controllers with
>>> any PCI address restrictions to implement ->map_align(). This could as
>>> well be done in a phased manner to let controllers implement
>>> ->map_align() and then remove using pci_epc_features in
>>> pci_epc_map_align(). Let me know what you think?
>
> First you say that you want to avoid using epc_features->align inside
> pci_epc_map_align(), and then you say that we could do it in phases,
> and eventually stop using epc_features->align in pci_epc_map_align().
>
> I'm confused... :)
>
> Do you really want pci_epc_map_align() to make use of epc_features->align ?
Would like pci_epc_map_align() to not use epc_features->align as
pci_epc_map_align() is for PCIe address programmed in outbound ATU
(destination) and epc_features->align is for physical address programmed
in inbound ATU (target for BAR accesses).
I mentioned "in phases" for if some platforms require
pci_epc_map_align() to use epc_features->align, we could keep
epc_features->align in pci_epc_map_align() till all of them implement
the map_align() callback (and need not be part of this series itself).
But eventually stop using epc_features->align in pci_epc_map_align()
once all the platforms that require alignment implement ->map_align().
> > Don't you mean ep->page_size ?
> (Please read the whole email to see my reasoning.)
page_size is for two purposes:
1) Dividing into fixed size blocks the outbound address space for
simpler memory management
2) Alignment restrictions for source address (outbound address space) of
the outbound ATU (so page_size is for source alignment restriction in
outbound ATU and pci_epc_map_align() is for destination alignment
restriction in outbound ATU. Source address would be an address in the
outbound address space and destination address would be PCI address
usually provided by the host).
Even if some platform doesn't require source alignment restriction, it
still has to divide into fixed size blocks.
>
>
>>
>> Yep, good idea. I will remove the use of "align" as a default alignment
>> constraint. For controllers that have a fixed alignment constraint (not
>> necessarilly epc->features->align), it is trivial to provide a generic helper
>> function that implements the ->map_align method.
>
> We can see that commit:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2a9a801620efac92885fc9cd53594c0b9aba87a4
>
> Introduced epc_features->align and modified pci_epf_alloc_space() to use it.
>
> From reading the commit, it appears that epc_features->align was intended to
> represent inbound iATU alignment requirement.
>
> For DWC based controllers, the inbound iATU address must be aligned to:
> CX_ATU_MIN_REGION_SIZE.
>
> AFAICT, epc_features->align currently has nothing to do with traffic outbound
> from the EP.
It was initially added that way but I abused that in pci-epf-ntb.c
>
>
> For aligning the reads/writes to buffers allocated on the host side,
> we currently have .alignment in the host side driver:
> https://github.com/torvalds/linux/blob/v6.9-rc2/drivers/misc/pci_endpoint_test.c#L966-L1021
>
> Which should be set to the outbound iATU alignment requirement.
>
> For DWC based controllers, the outbound iATU address must be aligned to:
> CX_ATU_MIN_REGION_SIZE.
>
>
> Additionally, we have ep->page_size, which defines the smallest outbound unit
> that can be mapped.
> (On DWC based controllers, tis is CX_ATU_MIN_REGION_SIZE.)
>
> ep->page_size is used to specify the outbound alignment for e.g.
> dw_pcie_ep_raise_msi_irq() and dw_pcie_ep_raise_msix_irq():
> https://github.com/torvalds/linux/blob/v6.9-rc2/drivers/pci/controller/dwc/pcie-designware-ep.c#L488
> https://github.com/torvalds/linux/blob/v6.9-rc2/drivers/pci/controller/dwc/pcie-designware-ep.c#L555
>
> which makes sure that we can write to the RC side MSI/MSI-X address
> while satisfying the outbound iATU alignment requirement.
>
> See also:
> https://lore.kernel.org/linux-pci/20240402-pci2_upstream-v3-2-803414bdb430@nxp.com/
>
>
>
> Now I understand that rockchip is the first one that does not have a fixed
> alignment.
> So for that platform, map_align() will be different from ep->page_size.
> (For all DWC based drivers the outbound iATU alignment requirement is
> the same as the page size.)
>
> However, it would be nice if:
> 1) We could have a default implementation of map_align() that by default uses
> ep->page_size. Platforms that have non-fixed alignment requirements could
> define their own map_align().
IMHO generic/core functions should not overload ep->page_size or
epc_features->align as each of them has their own specific purpose.
>
> 2) We fix dw_pcie_ep_raise_msi_irq() and dw_pcie_ep_raise_msix_irq() to use
> the new pci_epc_map_align().
>
> 3) It is getting too complicated with all these...
> epc_features->align, ep->page_size, map_align(), and .alignment in host driver.
> I think that we need to document each of these in Documentation/PCI/endpoint/
right, .alignment should be deprecated and each of the others should be
documented to indicate the specific purpose it's added for.
>
> 4) It would be nice if we could set page_size correctly for all the PCI device
> and vendor IDs that have defined an .alignment in drivers/misc/pci_endpoint_test.c
> in the correct EPC driver. That way, we should be able to completely remove all
> .alignment specified in drivers/misc/pci_endpoint_test.c.
We should remove .alignment specified in
drivers/misc/pci_endpoint_test.c and each EPC driver should populate
->map_align() callback to provide the correct alignment. Don't think
this should change the page_size.
>
> 5) Unfortunately drivers/misc/pci_endpoint_test.c defines a default alignment
> of 4K:
> https://github.com/torvalds/linux/blob/v6.9-rc2/drivers/misc/pci_endpoint_test.c#L968
> https://github.com/torvalds/linux/blob/v6.9-rc2/drivers/misc/pci_endpoint_test.c#L820
>
> It would be nice if we could get rid of this as well. Or perhaps add an option
> to pci_test so that it does not use this 4k alignment, such that we can verify
> that pci_epc_map_align() is actually working.
+1
Thanks,
Kishon
>
>
>
> In my opinion 4) is the biggest win with this series, because it means that
> we define the alignment in the EPC driver, instead of needing to define it in
> each and every host side driver. But right now, this great improvement is not
> really visible for someone looking quickly at the current series.
>
>
> Kind regards,
> Niklas
^ permalink raw reply
* Re: [PATCH v3 04/29] riscv: zicfilp / zicfiss in dt-bindings (extensions.yaml)
From: Rob Herring @ 2024-04-10 11:58 UTC (permalink / raw)
To: Deepak Gupta
Cc: paul.walmsley, rick.p.edgecombe, broonie, Szabolcs.Nagy,
kito.cheng, keescook, ajones, conor.dooley, cleger, atishp, alex,
bjorn, alexghiti, samuel.holland, conor, linux-doc, linux-riscv,
linux-kernel, devicetree, linux-mm, linux-arch, linux-kselftest,
corbet, palmer, aou, krzysztof.kozlowski+dt, oleg, akpm, arnd,
ebiederm, Liam.Howlett, vbabka, lstoakes, shuah, brauner,
andy.chiu, jerry.shih, hankuan.chen, greentime.hu, evan,
xiao.w.wang, charlie, apatel, mchitale, dbarboza, sameo,
shikemeng, willy, vincent.chen, guoren, samitolvanen,
songshuaishuai, gerg, heiko, bhe, jeeheng.sia, cyy, maskray,
ancientmodern4, mathis.salmen, cuiyunhui, bgray, mpe, baruch, alx,
david, catalin.marinas, revest, josh, shr, deller, omosnace,
ojeda, jhubbard
In-Reply-To: <20240403234054.2020347-5-debug@rivosinc.com>
On Wed, Apr 03, 2024 at 04:34:52PM -0700, Deepak Gupta wrote:
> Make an entry for cfi extensions in extensions.yaml.
>
> Signed-off-by: Deepak Gupta <debug@rivosinc.com>
> ---
> .../devicetree/bindings/riscv/extensions.yaml | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/riscv/extensions.yaml b/Documentation/devicetree/bindings/riscv/extensions.yaml
> index 63d81dc895e5..45b87ad6cc1c 100644
> --- a/Documentation/devicetree/bindings/riscv/extensions.yaml
> +++ b/Documentation/devicetree/bindings/riscv/extensions.yaml
> @@ -317,6 +317,16 @@ properties:
> The standard Zicboz extension for cache-block zeroing as ratified
> in commit 3dd606f ("Create cmobase-v1.0.pdf") of riscv-CMOs.
>
> + - const: zicfilp
> + description:
> + The standard Zicfilp extension for enforcing forward edge control-flow
> + integrity in commit 3a20dc9 of riscv-cfi and is in public review.
Does in public review mean the commit sha is going to change?
> +
> + - const: zicfiss
> + description:
> + The standard Zicfiss extension for enforcing backward edge control-flow
> + integrity in commit 3a20dc9 of riscv-cfi and is in publc review.
> +
> - const: zicntr
> description:
> The standard Zicntr extension for base counters and timers, as
> --
> 2.43.2
>
^ permalink raw reply
* Re: [PATCH v7 1/5] dt-bindings: interconnect: Add Qualcomm IPQ9574 support
From: Krzysztof Kozlowski @ 2024-04-10 12:01 UTC (permalink / raw)
To: Konrad Dybcio, Varadarajan Narayanan
Cc: andersson, mturquette, sboyd, robh, krzk+dt, conor+dt, djakov,
dmitry.baryshkov, quic_anusha, linux-arm-msm, linux-clk,
devicetree, linux-kernel, linux-pm
In-Reply-To: <f1b0d280-6986-4055-a611-2caceb15867d@linaro.org>
On 10/04/2024 13:48, Konrad Dybcio wrote:
>
>
> On 4/10/24 13:15, Krzysztof Kozlowski wrote:
>> On 10/04/2024 12:02, Varadarajan Narayanan wrote:
>>>> Okay, so what happens if icc-clk way of generating them changes a bit?
>>>> It can change, why not, driver implementation is not an ABI.
>>>>
>>>>>
>>>>> 2. These auto-generated id-numbers have to be correctly
>>>>> tied to the DT nodes. Else, the relevant clocks may
>>>>> not get enabled.
>>>>
>>>> Sorry, I don't get, how auto generated ID number is tied to DT node.
>>>> What DT node?
>>>
>>> I meant the following usage for the 'interconnects' entry of the
>>> consumer peripheral's node.
>>>
>>> interconnects = <&gcc MASTER_ANOC_PCIE0 &gcc SLAVE_ANOC_PCIE0>,
>>> ^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^
>>> <&gcc MASTER_SNOC_PCIE0 &gcc SLAVE_SNOC_PCIE0>;
>>> ^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^
>>>
>>>>> Since ICC-CLK creates two ids per clock entry (one MASTER_xxx and
>>>>> one SLAVE_xxx), using those MASTER/SLAVE_xxx macros as indices in
>>>>> the below array would create holes.
>>>>>
>>>>> static int icc_ipq9574_hws[] = {
>>>>> [MASTER_ANOC_PCIE0] = GCC_ANOC_PCIE0_1LANE_M_CLK,
>>>>> [MASTER_SNOC_PCIE0] = GCC_SNOC_PCIE0_1LANE_S_CLK,
>>>>> [MASTER_ANOC_PCIE1] = GCC_ANOC_PCIE1_1LANE_M_CLK,
>>>>> [MASTER_SNOC_PCIE1] = GCC_SNOC_PCIE1_1LANE_S_CLK,
>>>>> . . .
>>>>> };
>>>>>
>>>>> Other Qualcomm drivers don't have this issue and they can
>>>>> directly use the MASTER/SLAVE_xxx macros.
>>>>
>>>> I understand, thanks, yet your last patch keeps adding fake IDs, means
>>>> IDs which are not part of ABI.
>>>>
>>>>>
>>>>> As the MASTER_xxx macros cannot be used, have to define a new set
>>>>> of macros that can be used for indices in the above array. This
>>>>> is the reason for the ICC_BINDING_NAME macros.
>>>>
>>>> Then maybe fix the driver, instead of adding something which is not an
>>>> ABI to bindings and completely skipping the actual ABI.
>>>
>>> Will remove the ICC_xxx defines from the header. And in the
>>> driver will change the declaration as follows. Will that be
>>> acceptable?
>>>
>>> static int icc_ipq9574_hws[] = {
>>> [MASTER_ANOC_PCIE0 / 2] = GCC_ANOC_PCIE0_1LANE_M_CLK,
>>
>> What is the binding in such case? What exactly do you bind between
>> driver and DTS?
>
> I think what Krzysztof is trying to say here is "the icc-clk API is tragic"
> and the best solution would be to make it such that the interconnect indices
> are set explicitly, instead of (master, slave), (master, slave) etc.
>
> Does that sound good, Krzysztof?
Yes, I think earlier I expressed that icc-clk might needs fixes. The
indices you define in the binding must be used by DTS and by the driver.
Directly, otherwise it is error-prone and not really an ABI...
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v7 2/7] iommu/arm-smmu-qcom-tbu: Add Qualcomm TBU driver
From: Konrad Dybcio @ 2024-04-10 12:02 UTC (permalink / raw)
To: Georgi Djakov, will, robin.murphy, joro, iommu
Cc: robh+dt, krzysztof.kozlowski+dt, conor+dt, devicetree, andersson,
robdclark, linux-arm-kernel, linux-kernel, linux-arm-msm,
quic_cgoldswo, quic_sukadev, quic_pdaly, quic_sudaraja, djakov
In-Reply-To: <20240329210638.3647523-3-quic_c_gdjako@quicinc.com>
On 3/29/24 22:06, Georgi Djakov wrote:
> Operating the TBUs (Translation Buffer Units) from Linux on Qualcomm
> platforms can help with debugging context faults. To help with that,
> the TBUs can run ATOS (Address Translation Operations) to manually
> trigger address translation of IOVA to physical address in hardware
> and provide more details when a context fault happens.
>
> The driver will control the resources needed by the TBU to allow
> running the debug operations such as ATOS, check for outstanding
> transactions, do snapshot capture etc.
>
> Signed-off-by: Georgi Djakov <quic_c_gdjako@quicinc.com>
> ---
> drivers/iommu/Kconfig | 9 +
> drivers/iommu/arm/arm-smmu/Makefile | 1 +
> .../iommu/arm/arm-smmu/arm-smmu-qcom-tbu.c | 372 ++++++++++++++++++
> drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h | 2 +
> drivers/iommu/arm/arm-smmu/arm-smmu.h | 2 +
> 5 files changed, 386 insertions(+)
> create mode 100644 drivers/iommu/arm/arm-smmu/arm-smmu-qcom-tbu.c
>
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index 0af39bbbe3a3..b699e88f42c5 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -374,6 +374,15 @@ config ARM_SMMU_QCOM
> When running on a Qualcomm platform that has the custom variant
> of the ARM SMMU, this needs to be built into the SMMU driver.
>
> +config ARM_SMMU_QCOM_TBU
> + bool "Qualcomm TBU driver"
> + depends on ARM_SMMU_QCOM
> + help
> + The SMMUs on Qualcomm platforms may include Translation Buffer
> + Units (TBUs) for each master. Enabling support for these units
> + allows to operate the TBUs and obtain additional information
> + when debugging memory management issues like context faults.
> +
> config ARM_SMMU_QCOM_DEBUG
> bool "ARM SMMU QCOM implementation defined debug support"
> depends on ARM_SMMU_QCOM
> diff --git a/drivers/iommu/arm/arm-smmu/Makefile b/drivers/iommu/arm/arm-smmu/Makefile
> index 2a5a95e8e3f9..c35ff78fcfd5 100644
> --- a/drivers/iommu/arm/arm-smmu/Makefile
> +++ b/drivers/iommu/arm/arm-smmu/Makefile
> @@ -3,4 +3,5 @@ obj-$(CONFIG_QCOM_IOMMU) += qcom_iommu.o
> obj-$(CONFIG_ARM_SMMU) += arm_smmu.o
> arm_smmu-objs += arm-smmu.o arm-smmu-impl.o arm-smmu-nvidia.o
> arm_smmu-$(CONFIG_ARM_SMMU_QCOM) += arm-smmu-qcom.o
> +arm_smmu-$(CONFIG_ARM_SMMU_QCOM_TBU) += arm-smmu-qcom-tbu.o
> arm_smmu-$(CONFIG_ARM_SMMU_QCOM_DEBUG) += arm-smmu-qcom-debug.o
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom-tbu.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom-tbu.c
> new file mode 100644
> index 000000000000..e3202ed89566
> --- /dev/null
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom-tbu.c
> @@ -0,0 +1,372 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#include <linux/interconnect.h>
> +#include <linux/iopoll.h>
> +#include <linux/list.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +#include <linux/spinlock.h>
> +
> +#include "arm-smmu.h"
> +#include "arm-smmu-qcom.h"
> +
> +#define TBU_DBG_TIMEOUT_US 100
> +#define DEBUG_AXUSER_REG 0x30
> +#define DEBUG_AXUSER_CDMID GENMASK_ULL(43, 36)
> +#define DEBUG_AXUSER_CDMID_VAL 0xff
> +#define DEBUG_PAR_REG 0x28
> +#define DEBUG_PAR_FAULT_VAL BIT(0)
> +#define DEBUG_PAR_PA GENMASK_ULL(47, 12)
> +#define DEBUG_SID_HALT_REG 0x0
> +#define DEBUG_SID_HALT_VAL BIT(16)
> +#define DEBUG_SID_HALT_SID GENMASK(9, 0)
> +#define DEBUG_SR_HALT_ACK_REG 0x20
> +#define DEBUG_SR_HALT_ACK_VAL BIT(1)
> +#define DEBUG_SR_ECATS_RUNNING_VAL BIT(0)
> +#define DEBUG_TXN_AXCACHE GENMASK(5, 2)
> +#define DEBUG_TXN_AXPROT GENMASK(8, 6)
> +#define DEBUG_TXN_AXPROT_PRIV 0x1
> +#define DEBUG_TXN_AXPROT_NSEC 0x2
> +#define DEBUG_TXN_TRIGG_REG 0x18
> +#define DEBUG_TXN_TRIGGER BIT(0)
> +#define DEBUG_VA_ADDR_REG 0x8
> +
> +static LIST_HEAD(tbu_list);
> +static DEFINE_MUTEX(tbu_list_lock);
> +static DEFINE_SPINLOCK(atos_lock);
> +
> +struct qcom_tbu {
> + struct device *dev;
> + struct device_node *smmu_np;
> + u32 sid_range[2];
> + struct list_head list;
> + struct clk *clk;
> + struct icc_path *path;
> + void __iomem *base;
> + spinlock_t halt_lock; /* multiple halt or resume can't execute concurrently */
> + int halt_count;
> +};
> +
> +static struct qcom_smmu *to_qcom_smmu(struct arm_smmu_device *smmu)
> +{
> + return container_of(smmu, struct qcom_smmu, smmu);
> +}
> +
> +static struct qcom_tbu *qcom_find_tbu(struct qcom_smmu *qsmmu, u32 sid)
> +{
> + struct qcom_tbu *tbu;
> + u32 start, end;
> +
> + mutex_lock(&tbu_list_lock);
#include <linux/cleanup.h>
guard(mutex)(&tbu_list_lock);
and remove the unlocks
similarly for the spinlocks below
Konrad
^ permalink raw reply
* Re: [PATCH v2] arm64: dts: mediatek: mt8192: Add missing trip point in thermal zone
From: Hsin-Te Yuan @ 2024-04-10 12:06 UTC (permalink / raw)
To: AngeloGioacchino Del Regno
Cc: Hsin-Te Yuan, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Matthias Brugger, Nícolas F. R. A. Prado,
Bernhard Rosenkränzer, Balsam CHIHI, Alexandre Mergnat,
devicetree, linux-kernel, linux-arm-kernel, linux-mediatek
In-Reply-To: <d1dafc46-9b14-4752-822c-86cbabdcfb8b@collabora.com>
Hi Angelo,
1. 68°C is provided by MediaTek previously in downstream
2. I think sustainable power will be estimated by
estimate_sustainable_power in drivers/thermal/gov_power_allocator.c,
but I'll ask MediaTek if they can provide the precise value.
3. Sure. I'll do it in v3 after I check 2. with MediaTek
Regards,
Hsin-Te
On Wed, Apr 10, 2024 at 5:50 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Il 10/04/24 10:40, Hsin-Te Yuan ha scritto:
> > According to Documentation/driver-api/thermal/power_allocator.rst, there
> > should be two passive trip points. Adding the missing trip point to
> > ensure that the governor works optimally.
> >
> > Fixes: c7a728051f4e ("arm64: dts: mediatek: mt8192: Add thermal nodes and thermal zones")
> > Signed-off-by: Hsin-Te Yuan <yuanhsinte@chromium.org>
>
> That's better - but while I can agree about setting a general temperature
> for that, I still have a few questions and requests:
>
> 1. Why is this 68°C? Was that value provided by MediaTek, or was it calculated?
> 1b. If this was calculated, please, can you explain how?
>
> 2. The power allocator documentation also says that the governor works good
> when a "sustainable dissipatable power" parameter is fed to it through the
> `sustainable-power` devicetree property (with that being strictly board
> specific and never SoC-global, as that depends on the form factor and on
> the cooling method of the machine), can you please also add the right
> sustainable power indication to the Chromebook devicetrees?
> In the MT8192 specific case, that's mt8192-asurada.dtsi.
>
> 3. I just noticed that MT8192 is not the only one that would be affected by
> the issue that you're describing in this commit; can you please perform a
> similar change on the others, if parameters are known?
>
> Thanks,
> Angelo
>
> > ---
> > Changes in v2:
> > - Clearify the reason of adding another passive trip point
> > - Link to v1: https://lore.kernel.org/r/20240410-upstream-torvalds-master-v1-1-852e903f0cec@chromium.org
> > ---
> > arch/arm64/boot/dts/mediatek/mt8192.dtsi | 40 ++++++++++++++++++++++++++++++++
> > 1 file changed, 40 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/mediatek/mt8192.dtsi b/arch/arm64/boot/dts/mediatek/mt8192.dtsi
> > index 05e401670bced..08d8bccc84669 100644
> > --- a/arch/arm64/boot/dts/mediatek/mt8192.dtsi
> > +++ b/arch/arm64/boot/dts/mediatek/mt8192.dtsi
> > @@ -1959,6 +1959,11 @@ cpu0-thermal {
> > thermal-sensors = <&lvts_mcu MT8192_MCU_LITTLE_CPU0>;
> >
> > trips {
> > + cpu0_thres: trip-point {
> > + temperature = <68000>;
> > + hysteresis = <2000>;
> > + type = "passive";
> > + };
> > cpu0_alert: trip-alert {
> > temperature = <85000>;
> > hysteresis = <2000>;
> > @@ -1989,6 +1994,11 @@ cpu1-thermal {
> > thermal-sensors = <&lvts_mcu MT8192_MCU_LITTLE_CPU1>;
> >
> > trips {
> > + cpu1_thres: trip-point {
> > + temperature = <68000>;
> > + hysteresis = <2000>;
> > + type = "passive";
> > + };
> > cpu1_alert: trip-alert {
> > temperature = <85000>;
> > hysteresis = <2000>;
> > @@ -2019,6 +2029,11 @@ cpu2-thermal {
> > thermal-sensors = <&lvts_mcu MT8192_MCU_LITTLE_CPU2>;
> >
> > trips {
> > + cpu2_thres: trip-point {
> > + temperature = <68000>;
> > + hysteresis = <2000>;
> > + type = "passive";
> > + };
> > cpu2_alert: trip-alert {
> > temperature = <85000>;
> > hysteresis = <2000>;
> > @@ -2049,6 +2064,11 @@ cpu3-thermal {
> > thermal-sensors = <&lvts_mcu MT8192_MCU_LITTLE_CPU3>;
> >
> > trips {
> > + cpu3_thres: trip-point {
> > + temperature = <68000>;
> > + hysteresis = <2000>;
> > + type = "passive";
> > + };
> > cpu3_alert: trip-alert {
> > temperature = <85000>;
> > hysteresis = <2000>;
> > @@ -2079,6 +2099,11 @@ cpu4-thermal {
> > thermal-sensors = <&lvts_mcu MT8192_MCU_BIG_CPU0>;
> >
> > trips {
> > + cpu4_thres: trip-point {
> > + temperature = <68000>;
> > + hysteresis = <2000>;
> > + type = "passive";
> > + };
> > cpu4_alert: trip-alert {
> > temperature = <85000>;
> > hysteresis = <2000>;
> > @@ -2109,6 +2134,11 @@ cpu5-thermal {
> > thermal-sensors = <&lvts_mcu MT8192_MCU_BIG_CPU1>;
> >
> > trips {
> > + cpu5_thres: trip-point {
> > + temperature = <68000>;
> > + hysteresis = <2000>;
> > + type = "passive";
> > + };
> > cpu5_alert: trip-alert {
> > temperature = <85000>;
> > hysteresis = <2000>;
> > @@ -2139,6 +2169,11 @@ cpu6-thermal {
> > thermal-sensors = <&lvts_mcu MT8192_MCU_BIG_CPU2>;
> >
> > trips {
> > + cpu6_thres: trip-point {
> > + temperature = <68000>;
> > + hysteresis = <2000>;
> > + type = "passive";
> > + };
> > cpu6_alert: trip-alert {
> > temperature = <85000>;
> > hysteresis = <2000>;
> > @@ -2169,6 +2204,11 @@ cpu7-thermal {
> > thermal-sensors = <&lvts_mcu MT8192_MCU_BIG_CPU3>;
> >
> > trips {
> > + cpu7_thres: trip-point {
> > + temperature = <68000>;
> > + hysteresis = <2000>;
> > + type = "passive";
> > + };
> > cpu7_alert: trip-alert {
> > temperature = <85000>;
> > hysteresis = <2000>;
> >
> > ---
> > base-commit: 20cb38a7af88dc40095da7c2c9094da3873fea23
> > change-id: 20240410-upstream-torvalds-master-40aeff5416c7
> >
> > Best regards,
>
>
^ permalink raw reply
* Re: [PATCH v7 5/7] arm64: dts: qcom: sdm845: Add DT nodes for the TBUs
From: Konrad Dybcio @ 2024-04-10 12:08 UTC (permalink / raw)
To: Georgi Djakov, will, robin.murphy, joro, iommu
Cc: robh+dt, krzysztof.kozlowski+dt, conor+dt, devicetree, andersson,
robdclark, linux-arm-kernel, linux-kernel, linux-arm-msm,
quic_cgoldswo, quic_sukadev, quic_pdaly, quic_sudaraja, djakov
In-Reply-To: <20240329210638.3647523-6-quic_c_gdjako@quicinc.com>
On 3/29/24 22:06, Georgi Djakov wrote:
> Add the device-tree nodes for the TBUs (translation buffer units) that
> are present on the sdm845 platforms. The TBUs can be used debug the
> kernel and provide additional information when a context faults occur.
>
> Describe the all registers, clocks, interconnects and power-domain
> resources that are needed for each of the TBUs.
>
> Signed-off-by: Georgi Djakov <quic_c_gdjako@quicinc.com>
> ---
[...]
> + mnoc_hf_0_tbu: tbu@150cd000 {
> + compatible = "qcom,sdm845-tbu";
> + reg = <0x0 0x150cd000 0x0 0x1000>;
> + interconnects = <&mmss_noc MASTER_MDP0 QCOM_ICC_TAG_ACTIVE_ONLY
> + &mmss_noc SLAVE_MNOC_HF_MEM_NOC QCOM_ICC_TAG_ACTIVE_ONLY>;
> + qcom,stream-id-range = <&apps_smmu 0x800 0x400>;
> + };
> +
> + mnoc_hf_1_tbu: tbu@150d1000 {
> + compatible = "qcom,sdm845-tbu";
> + reg = <0x0 0x150d1000 0x0 0x1000>;
> + interconnects = <&mmss_noc MASTER_MDP0 QCOM_ICC_TAG_ACTIVE_ONLY
> + &mmss_noc SLAVE_MNOC_HF_MEM_NOC QCOM_ICC_TAG_ACTIVE_ONLY>;
> + qcom,stream-id-range = <&apps_smmu 0xc00 0x400>;
> + };
> +
> + mnoc_sf_0_tbu: tbu@150d5000 {
> + compatible = "qcom,sdm845-tbu";
> + reg = <0x0 0x150d5000 0x0 0x1000>;
> + interconnects = <&mmss_noc MASTER_CAMNOC_SF QCOM_ICC_TAG_ACTIVE_ONLY
> + &mmss_noc SLAVE_MNOC_SF_MEM_NOC QCOM_ICC_TAG_ACTIVE_ONLY>;
> + qcom,stream-id-range = <&apps_smmu 0x1000 0x400>;
> + };
These three are missing their GDSCs, the rest looks in line with msm-4.4
Konrad
^ permalink raw reply
* Re: [RESEND v7 09/37] dt-binding: Add compatible SH7750 SoC
From: Rob Herring @ 2024-04-10 12:12 UTC (permalink / raw)
To: Yoshinori Sato
Cc: Stephen Rothwell, Javier Martinez Canillas, linux-ide,
Chris Morgan, Laurent Pinchart, Thomas Gleixner, linux-pci,
Jiri Slaby, Sam Ravnborg, John Paul Adrian Glaubitz, David Airlie,
Maxime Ripard, Niklas Cassel, Herve Codina, Anup Patel,
linux-serial, Max Filippov, Maarten Lankhorst, Lee Jones,
Shawn Guo, devicetree, Lorenzo Pieralisi, Daniel Lezcano,
linux-renesas-soc, linux-clk, Azeem Shaikh, Greg Kroah-Hartman,
Bjorn Helgaas, Jonathan Corbet, Thomas Zimmermann, Hyeonggon Yoo,
Geert Uytterhoeven, Stephen Boyd, Jacky Huang,
Manikanta Guntupalli, Damien Le Moal, Rich Felker,
Sebastian Reichel, Heiko Stuebner, Arnd Bergmann, David Rientjes,
Uwe Kleine-König, dri-devel, Michael Turquette, Conor Dooley,
Vlastimil Babka, Baoquan He, linux-kernel, Krzysztof Kozlowski,
Helge Deller, Guo Ren, Magnus Damm, Sergey Shtylyov, Biju Das,
Andrew Morton, Daniel Vetter, Kefeng Wang, Guenter Roeck,
linux-sh, Krzysztof Wilczyński, Linus Walleij, linux-fbdev
In-Reply-To: <4ac65d0f311e890c1ca92bf057c70954ec7ac351.1712207606.git.ysato@users.sourceforge.jp>
On Thu, 04 Apr 2024 14:14:20 +0900, Yoshinori Sato wrote:
> Signed-off-by: Yoshinori Sato <ysato@users.sourceforge.jp>
> ---
> Documentation/devicetree/bindings/timer/renesas,tmu.yaml | 2 ++
> 1 file changed, 2 insertions(+)
>
Acked-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* Re: [PATCH v7 7/7] arm64: dts: qcom: sc7280: Add DT nodes for the TBUs
From: Konrad Dybcio @ 2024-04-10 12:13 UTC (permalink / raw)
To: Georgi Djakov, will, robin.murphy, joro, iommu
Cc: robh+dt, krzysztof.kozlowski+dt, conor+dt, devicetree, andersson,
robdclark, linux-arm-kernel, linux-kernel, linux-arm-msm,
quic_cgoldswo, quic_sukadev, quic_pdaly, quic_sudaraja, djakov
In-Reply-To: <20240329210638.3647523-8-quic_c_gdjako@quicinc.com>
On 3/29/24 22:06, Georgi Djakov wrote:
> Add the device-tree nodes for the TBUs (translation buffer units) that
> are present on the sc7280 platforms. The TBUs can be used debug the
> kernel and provide additional information when a context faults occur.
>
> Describe all the registers, clocks, interconnects and power-domain
> resources that are needed for each of the TBUs.
>
> Signed-off-by: Georgi Djakov <quic_c_gdjako@quicinc.com>
> ---
[...]
> + compute_dsp_0_tbu: tbu@151ed000 {
The labels are swapped between this node and the one below (the rest is ok)
> + compatible = "qcom,sc7280-tbu";
> + reg = <0x0 0x151ed000 0x0 0x1000>;
> + interconnects = <&nsp_noc MASTER_CDSP_PROC QCOM_ICC_TAG_ACTIVE_ONLY
> + &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ACTIVE_ONLY>;
> + power-domains = <&gcc HLOS1_VOTE_TURING_MMU_TBU1_GDSC>;
> + qcom,stream-id-range = <&apps_smmu 0x1000 0x400>;
> + };
> +
> + compute_dsp_1_tbu: tbu@151f1000 {
> + compatible = "qcom,sc7280-tbu";
> + reg = <0x0 0x151f1000 0x0 0x1000>;
> + interconnects = <&nsp_noc MASTER_CDSP_PROC QCOM_ICC_TAG_ACTIVE_ONLY
> + &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ACTIVE_ONLY>;
> + power-domains = <&gcc HLOS1_VOTE_TURING_MMU_TBU0_GDSC>;
> + qcom,stream-id-range = <&apps_smmu 0x1400 0x400>;
> + };
Konrad
^ permalink raw reply
* Re: [RESEND v7 12/37] dt-bindings: pci: pci-sh7751: Add SH7751 PCI
From: Rob Herring @ 2024-04-10 12:16 UTC (permalink / raw)
To: Yoshinori Sato
Cc: linux-sh, Damien Le Moal, Niklas Cassel, Krzysztof Kozlowski,
Conor Dooley, Geert Uytterhoeven, Michael Turquette, Stephen Boyd,
David Airlie, Daniel Vetter, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, Thomas Gleixner, Bjorn Helgaas,
Lorenzo Pieralisi, Krzysztof Wilczyński, Greg Kroah-Hartman,
Jiri Slaby, Magnus Damm, Daniel Lezcano, Rich Felker,
John Paul Adrian Glaubitz, Lee Jones, Helge Deller,
Heiko Stuebner, Shawn Guo, Sebastian Reichel, Chris Morgan,
Linus Walleij, Arnd Bergmann, David Rientjes, Hyeonggon Yoo,
Vlastimil Babka, Baoquan He, Andrew Morton, Guenter Roeck,
Kefeng Wang, Stephen Rothwell, Javier Martinez Canillas, Guo Ren,
Azeem Shaikh, Max Filippov, Jonathan Corbet, Jacky Huang,
Herve Codina, Manikanta Guntupalli, Anup Patel, Biju Das,
Uwe Kleine-König, Sam Ravnborg, Sergey Shtylyov,
Laurent Pinchart, linux-ide, devicetree, linux-kernel,
linux-renesas-soc, linux-clk, dri-devel, linux-pci, linux-serial,
linux-fbdev
In-Reply-To: <5ab3c5952b49d7998734855e2ec1ee980795a724.1712207606.git.ysato@users.sourceforge.jp>
On Thu, Apr 04, 2024 at 02:14:23PM +0900, Yoshinori Sato wrote:
> Renesas SH7751 PCI Controller json-schema.
>
> Signed-off-by: Yoshinori Sato <ysato@users.sourceforge.jp>
> ---
> .../bindings/pci/renesas,sh7751-pci.yaml | 89 +++++++++++++++++++
> 1 file changed, 89 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/pci/renesas,sh7751-pci.yaml
>
> diff --git a/Documentation/devicetree/bindings/pci/renesas,sh7751-pci.yaml b/Documentation/devicetree/bindings/pci/renesas,sh7751-pci.yaml
> new file mode 100644
> index 000000000000..115c2bb67339
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/renesas,sh7751-pci.yaml
> @@ -0,0 +1,89 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pci/renesas,sh7751-pci.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Renesas SH7751 PCI Host controller
> +
> +maintainers:
> + - Yoshinori Sato <ysato@users.sourceforge.jp>
> +
> +allOf:
> + - $ref: /schemas/pci/pci-bus.yaml#
> +
> +properties:
> + compatible:
> + const: renesas,sh7751-pci
> +
> + reg:
> + minItems: 2
> + maxItems: 2
> +
> + reg-names:
> + items:
> + - const: PCI Controller
> + - const: Bus State Controller
> +
> + "#interrupt-cells":
> + const: 1
> +
> + "#address-cells":
> + const: 3
> +
> + "#size-cells":
> + const: 2
> +
> + ranges: true
> +
> + dma-ranges: true
All 5 of these are defined in pci-bus-common.yaml, so you can drop them.
> +
> + interrupt-controller: true
> +
> + renesas,bus-arbit-round-robin:
> + $ref: /schemas/types.yaml#/definitions/flag
> + description: |
Don't need '|'.
> + Set DMA bus arbitration to round robin.
> +
> +required:
> + - compatible
> + - reg
> + - "#interrupt-cells"
> + - "#address-cells"
> + - "#size-cells"
> + - ranges
These 3 are already required, so drop.
> + - interrupt-map
> + - interrupt-map-mask
> +
> +unevaluatedProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/interrupt-controller/irq.h>
> + pci@fe200000 {
> + compatible = "renesas,sh7751-pci";
> + #address-cells = <3>;
> + #size-cells = <2>;
> + #interrupt-cells = <1>;
> + interrupt-controller;
> + device_type = "pci";
> + bus-range = <0 0>;
> + ranges = <0x02000000 0 0xfd000000 0xfd000000 0 0x01000000>,
> + <0x01000000 0 0x00000000 0xfe240000 0 0x00040000>;
> + dma-ranges = <0x02000000 0 0xc000000 0x0c000000 0 0x04000000>;
> + reg = <0xfe200000 0x0400>,
> + <0xff800000 0x0100>;
> + interrupt-map = <0x0000 0 0 1 &julianintc 5 IRQ_TYPE_LEVEL_LOW>,
> + <0x0000 0 0 2 &julianintc 6 IRQ_TYPE_LEVEL_LOW>,
> + <0x0000 0 0 3 &julianintc 7 IRQ_TYPE_LEVEL_LOW>,
> + <0x0000 0 0 4 &julianintc 8 IRQ_TYPE_LEVEL_LOW>,
> + <0x0800 0 0 1 &julianintc 6 IRQ_TYPE_LEVEL_LOW>,
> + <0x0800 0 0 2 &julianintc 7 IRQ_TYPE_LEVEL_LOW>,
> + <0x0800 0 0 3 &julianintc 8 IRQ_TYPE_LEVEL_LOW>,
> + <0x0800 0 0 4 &julianintc 5 IRQ_TYPE_LEVEL_LOW>,
> + <0x1000 0 0 1 &julianintc 7 IRQ_TYPE_LEVEL_LOW>,
> + <0x1000 0 0 2 &julianintc 8 IRQ_TYPE_LEVEL_LOW>,
> + <0x1000 0 0 3 &julianintc 5 IRQ_TYPE_LEVEL_LOW>,
> + <0x1000 0 0 4 &julianintc 6 IRQ_TYPE_LEVEL_LOW>;
> + interrupt-map-mask = <0x1800 0 0 7>;
> + };
> --
> 2.39.2
>
^ permalink raw reply
* Re: [PATCH v2] arm64: dts: mediatek: mt8192: Add missing trip point in thermal zone
From: AngeloGioacchino Del Regno @ 2024-04-10 12:17 UTC (permalink / raw)
To: Hsin-Te Yuan
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Matthias Brugger,
Nícolas F. R. A. Prado, Bernhard Rosenkränzer,
Balsam CHIHI, Alexandre Mergnat, devicetree, linux-kernel,
linux-arm-kernel, linux-mediatek
In-Reply-To: <CAHc4DNLrU=QVWODOzHXW16Ko1V=W31zkSHOi7rztiJMOMDcKqg@mail.gmail.com>
Il 10/04/24 14:06, Hsin-Te Yuan ha scritto:
> Hi Angelo,
>
> 1. 68°C is provided by MediaTek previously in downstream
> 2. I think sustainable power will be estimated by
> estimate_sustainable_power in drivers/thermal/gov_power_allocator.c,
> but I'll ask MediaTek if they can provide the precise value.
> 3. Sure. I'll do it in v3 after I check 2. with MediaTek
>
Perfect, thanks for the clarification.
Will wait for v3.
Cheers,
Angelo
> Regards,
> Hsin-Te
>
> On Wed, Apr 10, 2024 at 5:50 PM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com> wrote:
>>
>> Il 10/04/24 10:40, Hsin-Te Yuan ha scritto:
>>> According to Documentation/driver-api/thermal/power_allocator.rst, there
>>> should be two passive trip points. Adding the missing trip point to
>>> ensure that the governor works optimally.
>>>
>>> Fixes: c7a728051f4e ("arm64: dts: mediatek: mt8192: Add thermal nodes and thermal zones")
>>> Signed-off-by: Hsin-Te Yuan <yuanhsinte@chromium.org>
>>
>> That's better - but while I can agree about setting a general temperature
>> for that, I still have a few questions and requests:
>>
>> 1. Why is this 68°C? Was that value provided by MediaTek, or was it calculated?
>> 1b. If this was calculated, please, can you explain how?
>>
>> 2. The power allocator documentation also says that the governor works good
>> when a "sustainable dissipatable power" parameter is fed to it through the
>> `sustainable-power` devicetree property (with that being strictly board
>> specific and never SoC-global, as that depends on the form factor and on
>> the cooling method of the machine), can you please also add the right
>> sustainable power indication to the Chromebook devicetrees?
>> In the MT8192 specific case, that's mt8192-asurada.dtsi.
>>
>> 3. I just noticed that MT8192 is not the only one that would be affected by
>> the issue that you're describing in this commit; can you please perform a
>> similar change on the others, if parameters are known?
>>
>> Thanks,
>> Angelo
>>
>>> ---
>>> Changes in v2:
>>> - Clearify the reason of adding another passive trip point
>>> - Link to v1: https://lore.kernel.org/r/20240410-upstream-torvalds-master-v1-1-852e903f0cec@chromium.org
>>> ---
>>> arch/arm64/boot/dts/mediatek/mt8192.dtsi | 40 ++++++++++++++++++++++++++++++++
>>> 1 file changed, 40 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/mediatek/mt8192.dtsi b/arch/arm64/boot/dts/mediatek/mt8192.dtsi
>>> index 05e401670bced..08d8bccc84669 100644
>>> --- a/arch/arm64/boot/dts/mediatek/mt8192.dtsi
>>> +++ b/arch/arm64/boot/dts/mediatek/mt8192.dtsi
>>> @@ -1959,6 +1959,11 @@ cpu0-thermal {
>>> thermal-sensors = <&lvts_mcu MT8192_MCU_LITTLE_CPU0>;
>>>
>>> trips {
>>> + cpu0_thres: trip-point {
>>> + temperature = <68000>;
>>> + hysteresis = <2000>;
>>> + type = "passive";
>>> + };
>>> cpu0_alert: trip-alert {
>>> temperature = <85000>;
>>> hysteresis = <2000>;
>>> @@ -1989,6 +1994,11 @@ cpu1-thermal {
>>> thermal-sensors = <&lvts_mcu MT8192_MCU_LITTLE_CPU1>;
>>>
>>> trips {
>>> + cpu1_thres: trip-point {
>>> + temperature = <68000>;
>>> + hysteresis = <2000>;
>>> + type = "passive";
>>> + };
>>> cpu1_alert: trip-alert {
>>> temperature = <85000>;
>>> hysteresis = <2000>;
>>> @@ -2019,6 +2029,11 @@ cpu2-thermal {
>>> thermal-sensors = <&lvts_mcu MT8192_MCU_LITTLE_CPU2>;
>>>
>>> trips {
>>> + cpu2_thres: trip-point {
>>> + temperature = <68000>;
>>> + hysteresis = <2000>;
>>> + type = "passive";
>>> + };
>>> cpu2_alert: trip-alert {
>>> temperature = <85000>;
>>> hysteresis = <2000>;
>>> @@ -2049,6 +2064,11 @@ cpu3-thermal {
>>> thermal-sensors = <&lvts_mcu MT8192_MCU_LITTLE_CPU3>;
>>>
>>> trips {
>>> + cpu3_thres: trip-point {
>>> + temperature = <68000>;
>>> + hysteresis = <2000>;
>>> + type = "passive";
>>> + };
>>> cpu3_alert: trip-alert {
>>> temperature = <85000>;
>>> hysteresis = <2000>;
>>> @@ -2079,6 +2099,11 @@ cpu4-thermal {
>>> thermal-sensors = <&lvts_mcu MT8192_MCU_BIG_CPU0>;
>>>
>>> trips {
>>> + cpu4_thres: trip-point {
>>> + temperature = <68000>;
>>> + hysteresis = <2000>;
>>> + type = "passive";
>>> + };
>>> cpu4_alert: trip-alert {
>>> temperature = <85000>;
>>> hysteresis = <2000>;
>>> @@ -2109,6 +2134,11 @@ cpu5-thermal {
>>> thermal-sensors = <&lvts_mcu MT8192_MCU_BIG_CPU1>;
>>>
>>> trips {
>>> + cpu5_thres: trip-point {
>>> + temperature = <68000>;
>>> + hysteresis = <2000>;
>>> + type = "passive";
>>> + };
>>> cpu5_alert: trip-alert {
>>> temperature = <85000>;
>>> hysteresis = <2000>;
>>> @@ -2139,6 +2169,11 @@ cpu6-thermal {
>>> thermal-sensors = <&lvts_mcu MT8192_MCU_BIG_CPU2>;
>>>
>>> trips {
>>> + cpu6_thres: trip-point {
>>> + temperature = <68000>;
>>> + hysteresis = <2000>;
>>> + type = "passive";
>>> + };
>>> cpu6_alert: trip-alert {
>>> temperature = <85000>;
>>> hysteresis = <2000>;
>>> @@ -2169,6 +2204,11 @@ cpu7-thermal {
>>> thermal-sensors = <&lvts_mcu MT8192_MCU_BIG_CPU3>;
>>>
>>> trips {
>>> + cpu7_thres: trip-point {
>>> + temperature = <68000>;
>>> + hysteresis = <2000>;
>>> + type = "passive";
>>> + };
>>> cpu7_alert: trip-alert {
>>> temperature = <85000>;
>>> hysteresis = <2000>;
>>>
>>> ---
>>> base-commit: 20cb38a7af88dc40095da7c2c9094da3873fea23
>>> change-id: 20240410-upstream-torvalds-master-40aeff5416c7
>>>
>>> Best regards,
>>
>>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox