* [PATCH v5 1/4] mfd: bd71837: mfd driver for ROHM BD71837 PMIC
From: Matti Vaittinen @ 2018-06-04 13:18 UTC (permalink / raw)
To: mturquette, sboyd, robh+dt, mark.rutland, lee.jones, lgirdwood,
broonie, mazziesaccount
Cc: linux-clk, devicetree, linux-kernel, mikko.mutanen,
heikki.haikola
In-Reply-To: <cover.1528117485.git.matti.vaittinen@fi.rohmeurope.com>
ROHM BD71837 PMIC MFD driver providing interrupts and support
for two subsystems:
- clk
- Regulators
Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
---
drivers/mfd/Kconfig | 13 ++
drivers/mfd/Makefile | 1 +
drivers/mfd/bd71837.c | 223 ++++++++++++++++++++++++++++++++++
include/linux/mfd/bd71837.h | 288 ++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 525 insertions(+)
create mode 100644 drivers/mfd/bd71837.c
create mode 100644 include/linux/mfd/bd71837.h
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index b860eb5aa194..7aa05fc9ed8e 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1787,6 +1787,19 @@ config MFD_STW481X
in various ST Microelectronics and ST-Ericsson embedded
Nomadik series.
+config MFD_BD71837
+ bool "BD71837 Power Management chip"
+ depends on I2C=y
+ depends on OF
+ select REGMAP_I2C
+ select REGMAP_IRQ
+ select MFD_CORE
+ help
+ Select this option to get support for the ROHM BD71837
+ Power Management chips. BD71837 is designed to power processors like
+ NXP i.MX8. It contains 8 BUCK outputs and 7 LDOs, voltage monitoring
+ and emergency shut down as well as 32,768KHz clock output.
+
config MFD_STM32_LPTIMER
tristate "Support for STM32 Low-Power Timer"
depends on (ARCH_STM32 && OF) || COMPILE_TEST
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index e9fd20dba18d..09dc9eb3782c 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -227,4 +227,5 @@ obj-$(CONFIG_MFD_STM32_TIMERS) += stm32-timers.o
obj-$(CONFIG_MFD_MXS_LRADC) += mxs-lradc.o
obj-$(CONFIG_MFD_SC27XX_PMIC) += sprd-sc27xx-spi.o
obj-$(CONFIG_RAVE_SP_CORE) += rave-sp.o
+obj-$(CONFIG_MFD_BD71837) += bd71837.o
diff --git a/drivers/mfd/bd71837.c b/drivers/mfd/bd71837.c
new file mode 100644
index 000000000000..93930f1f2893
--- /dev/null
+++ b/drivers/mfd/bd71837.c
@@ -0,0 +1,223 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2018 ROHM Semiconductors
+// bd71837.c -- ROHM BD71837MWV mfd driver
+
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
+#include <linux/gpio.h>
+#include <linux/regmap.h>
+#include <linux/of_device.h>
+#include <linux/of_gpio.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/bd71837.h>
+
+/* bd71837 multi function cells */
+static struct mfd_cell bd71837_mfd_cells[] = {
+ {
+ .name = "bd71837-clk",
+ .of_compatible = "rohm,bd71837-clk",
+ }, {
+ .name = "bd71837-pmic",
+ },
+};
+
+static const struct regmap_irq bd71837_irqs[] = {
+ REGMAP_IRQ_REG(BD71837_INT_SWRST, 0, BD71837_INT_SWRST_MASK),
+ REGMAP_IRQ_REG(BD71837_INT_PWRBTN_S, 0, BD71837_INT_PWRBTN_S_MASK),
+ REGMAP_IRQ_REG(BD71837_INT_PWRBTN_L, 0, BD71837_INT_PWRBTN_L_MASK),
+ REGMAP_IRQ_REG(BD71837_INT_PWRBTN, 0, BD71837_INT_PWRBTN_MASK),
+ REGMAP_IRQ_REG(BD71837_INT_WDOG, 0, BD71837_INT_WDOG_MASK),
+ REGMAP_IRQ_REG(BD71837_INT_ON_REQ, 0, BD71837_INT_ON_REQ_MASK),
+ REGMAP_IRQ_REG(BD71837_INT_STBY_REQ, 0, BD71837_INT_STBY_REQ_MASK),
+};
+
+static struct regmap_irq_chip bd71837_irq_chip = {
+ .name = "bd71837-irq",
+ .irqs = bd71837_irqs,
+ .num_irqs = ARRAY_SIZE(bd71837_irqs),
+ .num_regs = 1,
+ .irq_reg_stride = 1,
+ .status_base = BD71837_REG_IRQ,
+ .mask_base = BD71837_REG_MIRQ,
+ .init_ack_masked = true,
+ .mask_invert = false,
+};
+
+static int bd71837_irq_exit(struct bd71837 *bd71837)
+{
+ if (bd71837->chip_irq > 0)
+ regmap_del_irq_chip(bd71837->chip_irq, bd71837->irq_data);
+ return 0;
+}
+
+static const struct regmap_range pmic_status_range = {
+ .range_min = BD71837_REG_IRQ,
+ .range_max = BD71837_REG_POW_STATE,
+};
+
+static const struct regmap_access_table volatile_regs = {
+ .yes_ranges = &pmic_status_range,
+ .n_yes_ranges = 1,
+};
+
+static const struct regmap_config bd71837_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+ .volatile_table = &volatile_regs,
+ .max_register = BD71837_MAX_REGISTER - 1,
+ .cache_type = REGCACHE_RBTREE,
+};
+
+#ifdef CONFIG_OF
+static const struct of_device_id bd71837_of_match[] = {
+ { .compatible = "rohm,bd71837", .data = (void *)0},
+ { },
+};
+MODULE_DEVICE_TABLE(of, bd71837_of_match);
+
+static int bd71837_parse_dt(struct i2c_client *client, struct bd71837_board **b)
+{
+ struct device_node *np = client->dev.of_node;
+ struct bd71837_board *board_info;
+ unsigned int prop;
+ int r;
+ int rv = -ENOMEM;
+
+ board_info = devm_kzalloc(&client->dev, sizeof(*board_info),
+ GFP_KERNEL);
+ if (!board_info)
+ goto err_out;
+
+ if (client->irq) {
+ dev_dbg(&client->dev, "Got irq %d\n", client->irq);
+ board_info->gpio_intr = client->irq;
+ } else {
+ dev_err(&client->dev, "no pmic intr pin available\n");
+ rv = -ENOENT;
+ goto err_intr;
+ }
+ r = of_property_read_u32(np, "irq_base", &prop);
+ if (!r)
+ board_info->irq_base = prop;
+ else
+ board_info->irq_base = 0;
+
+ rv = 0;
+ *b = board_info;
+ if (0) {
+err_intr:
+ devm_kfree(&client->dev, board_info);
+err_out:
+ dev_err(&client->dev, "failed to parse device-tree (%d)\n", rv);
+ }
+ return rv;
+}
+#endif //CONFIG_OF
+
+static int bd71837_i2c_probe(struct i2c_client *i2c,
+ const struct i2c_device_id *id)
+{
+ struct bd71837 *bd71837;
+ struct bd71837_board *pmic_plat_data;
+ int ret = -EINVAL;
+
+ pmic_plat_data = dev_get_platdata(&i2c->dev);
+
+ if (!pmic_plat_data && i2c->dev.of_node)
+ ret = bd71837_parse_dt(i2c, &pmic_plat_data);
+
+ if (!pmic_plat_data)
+ return ret;
+
+ bd71837 = devm_kzalloc(&i2c->dev, sizeof(struct bd71837), GFP_KERNEL);
+ if (bd71837 == NULL)
+ return -ENOMEM;
+
+ bd71837->of_plat_data = pmic_plat_data;
+ i2c_set_clientdata(i2c, bd71837);
+ bd71837->dev = &i2c->dev;
+ bd71837->i2c_client = i2c;
+ bd71837->chip_irq = pmic_plat_data->gpio_intr;
+
+ bd71837->regmap = devm_regmap_init_i2c(i2c, &bd71837_regmap_config);
+ if (IS_ERR(bd71837->regmap)) {
+ ret = PTR_ERR(bd71837->regmap);
+ dev_err(&i2c->dev, "regmap initialization failed: %d\n", ret);
+ goto err_out;
+ }
+
+ ret = bd71837_reg_read(bd71837, BD71837_REG_REV);
+ if (ret < 0) {
+ dev_err(bd71837->dev,
+ "%s(): Read BD71837_REG_DEVICE failed!\n", __func__);
+ goto err_out;
+ }
+
+ ret = regmap_add_irq_chip(bd71837->regmap, bd71837->chip_irq,
+ IRQF_ONESHOT, 0,
+ &bd71837_irq_chip, &bd71837->irq_data);
+ if (ret < 0) {
+ dev_err(bd71837->dev, "Failed to add irq_chip %d\n", ret);
+ goto err_out;
+ }
+
+ ret = mfd_add_devices(bd71837->dev, PLATFORM_DEVID_AUTO,
+ bd71837_mfd_cells, ARRAY_SIZE(bd71837_mfd_cells),
+ NULL, 0,
+ regmap_irq_get_domain(bd71837->irq_data));
+ if (ret)
+ regmap_del_irq_chip(bd71837->chip_irq, bd71837->irq_data);
+err_out:
+
+ return ret;
+}
+
+static int bd71837_i2c_remove(struct i2c_client *i2c)
+{
+ struct bd71837 *bd71837 = i2c_get_clientdata(i2c);
+
+ bd71837_irq_exit(bd71837);
+ mfd_remove_devices(bd71837->dev);
+
+ return 0;
+}
+
+static const struct i2c_device_id bd71837_i2c_id[] = {
+ { .name = "bd71837", },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, bd71837_i2c_id);
+
+static struct i2c_driver bd71837_i2c_driver = {
+ .driver = {
+ .name = "bd71837-mfd",
+ .owner = THIS_MODULE,
+ .of_match_table = of_match_ptr(bd71837_of_match),
+ },
+ .probe = bd71837_i2c_probe,
+ .remove = bd71837_i2c_remove,
+ .id_table = bd71837_i2c_id,
+};
+
+static int __init bd71837_i2c_init(void)
+{
+ return i2c_add_driver(&bd71837_i2c_driver);
+}
+/* init early so consumer devices can complete system boot */
+subsys_initcall(bd71837_i2c_init);
+
+static void __exit bd71837_i2c_exit(void)
+{
+ i2c_del_driver(&bd71837_i2c_driver);
+}
+module_exit(bd71837_i2c_exit);
+
+MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>");
+MODULE_DESCRIPTION("BD71837 chip multi-function driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/mfd/bd71837.h b/include/linux/mfd/bd71837.h
new file mode 100644
index 000000000000..641b7dba3076
--- /dev/null
+++ b/include/linux/mfd/bd71837.h
@@ -0,0 +1,288 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (C) 2018 ROHM Semiconductors */
+
+/*
+ * ROHM BD71837MWV header file
+ */
+
+#ifndef __LINUX_MFD_BD71837_H__
+#define __LINUX_MFD_BD71837_H__
+
+#include <linux/regmap.h>
+
+enum {
+ BD71837_BUCK1 = 0,
+ BD71837_BUCK2,
+ BD71837_BUCK3,
+ BD71837_BUCK4,
+ BD71837_BUCK5,
+ BD71837_BUCK6,
+ BD71837_BUCK7,
+ BD71837_BUCK8,
+ BD71837_LDO1,
+ BD71837_LDO2,
+ BD71837_LDO3,
+ BD71837_LDO4,
+ BD71837_LDO5,
+ BD71837_LDO6,
+ BD71837_LDO7,
+ BD71837_REGULATOR_CNT,
+};
+
+#define BD71837_BUCK1_VOLTAGE_NUM 0x40
+#define BD71837_BUCK2_VOLTAGE_NUM 0x40
+#define BD71837_BUCK3_VOLTAGE_NUM 0x40
+#define BD71837_BUCK4_VOLTAGE_NUM 0x40
+
+#define BD71837_BUCK5_VOLTAGE_NUM 0x08
+#define BD71837_BUCK6_VOLTAGE_NUM 0x04
+#define BD71837_BUCK7_VOLTAGE_NUM 0x08
+#define BD71837_BUCK8_VOLTAGE_NUM 0x40
+
+#define BD71837_LDO1_VOLTAGE_NUM 0x04
+#define BD71837_LDO2_VOLTAGE_NUM 0x02
+#define BD71837_LDO3_VOLTAGE_NUM 0x10
+#define BD71837_LDO4_VOLTAGE_NUM 0x10
+#define BD71837_LDO5_VOLTAGE_NUM 0x10
+#define BD71837_LDO6_VOLTAGE_NUM 0x10
+#define BD71837_LDO7_VOLTAGE_NUM 0x10
+
+enum {
+ BD71837_REG_REV = 0x00,
+ BD71837_REG_SWRESET = 0x01,
+ BD71837_REG_I2C_DEV = 0x02,
+ BD71837_REG_PWRCTRL0 = 0x03,
+ BD71837_REG_PWRCTRL1 = 0x04,
+ BD71837_REG_BUCK1_CTRL = 0x05,
+ BD71837_REG_BUCK2_CTRL = 0x06,
+ BD71837_REG_BUCK3_CTRL = 0x07,
+ BD71837_REG_BUCK4_CTRL = 0x08,
+ BD71837_REG_BUCK5_CTRL = 0x09,
+ BD71837_REG_BUCK6_CTRL = 0x0A,
+ BD71837_REG_BUCK7_CTRL = 0x0B,
+ BD71837_REG_BUCK8_CTRL = 0x0C,
+ BD71837_REG_BUCK1_VOLT_RUN = 0x0D,
+ BD71837_REG_BUCK1_VOLT_IDLE = 0x0E,
+ BD71837_REG_BUCK1_VOLT_SUSP = 0x0F,
+ BD71837_REG_BUCK2_VOLT_RUN = 0x10,
+ BD71837_REG_BUCK2_VOLT_IDLE = 0x11,
+ BD71837_REG_BUCK3_VOLT_RUN = 0x12,
+ BD71837_REG_BUCK4_VOLT_RUN = 0x13,
+ BD71837_REG_BUCK5_VOLT = 0x14,
+ BD71837_REG_BUCK6_VOLT = 0x15,
+ BD71837_REG_BUCK7_VOLT = 0x16,
+ BD71837_REG_BUCK8_VOLT = 0x17,
+ BD71837_REG_LDO1_VOLT = 0x18,
+ BD71837_REG_LDO2_VOLT = 0x19,
+ BD71837_REG_LDO3_VOLT = 0x1A,
+ BD71837_REG_LDO4_VOLT = 0x1B,
+ BD71837_REG_LDO5_VOLT = 0x1C,
+ BD71837_REG_LDO6_VOLT = 0x1D,
+ BD71837_REG_LDO7_VOLT = 0x1E,
+ BD71837_REG_TRANS_COND0 = 0x1F,
+ BD71837_REG_TRANS_COND1 = 0x20,
+ BD71837_REG_VRFAULTEN = 0x21,
+ BD71837_REG_MVRFLTMASK0 = 0x22,
+ BD71837_REG_MVRFLTMASK1 = 0x23,
+ BD71837_REG_MVRFLTMASK2 = 0x24,
+ BD71837_REG_RCVCFG = 0x25,
+ BD71837_REG_RCVNUM = 0x26,
+ BD71837_REG_PWRONCONFIG0 = 0x27,
+ BD71837_REG_PWRONCONFIG1 = 0x28,
+ BD71837_REG_RESETSRC = 0x29,
+ BD71837_REG_MIRQ = 0x2A,
+ BD71837_REG_IRQ = 0x2B,
+ BD71837_REG_IN_MON = 0x2C,
+ BD71837_REG_POW_STATE = 0x2D,
+ BD71837_REG_OUT32K = 0x2E,
+ BD71837_REG_REGLOCK = 0x2F,
+ BD71837_REG_OTPVER = 0xFF,
+ BD71837_MAX_REGISTER = 0x100,
+};
+
+#define REGLOCK_PWRSEQ 0x1
+#define REGLOCK_VREG 0x10
+
+/* Generic BUCK control masks */
+#define BD71837_BUCK_SEL 0x02
+#define BD71837_BUCK_EN 0x01
+#define BD71837_BUCK_RUN_ON 0x04
+
+/* Generic LDO masks */
+#define BD71837_LDO_SEL 0x80
+#define BD71837_LDO_EN 0x40
+
+/* BD71837 BUCK ramp rate CTRL reg bits */
+#define BUCK_RAMPRATE_MASK 0xC0
+#define BUCK_RAMPRATE_10P00MV 0x0
+#define BUCK_RAMPRATE_5P00MV 0x1
+#define BUCK_RAMPRATE_2P50MV 0x2
+#define BUCK_RAMPRATE_1P25MV 0x3
+
+/* BD71837_REG_BUCK1_VOLT_RUN bits */
+#define BUCK1_RUN_MASK 0x3F
+#define BUCK1_RUN_DEFAULT 0x14
+
+/* BD71837_REG_BUCK1_VOLT_SUSP bits */
+#define BUCK1_SUSP_MASK 0x3F
+#define BUCK1_SUSP_DEFAULT 0x14
+
+/* BD71837_REG_BUCK1_VOLT_IDLE bits */
+#define BUCK1_IDLE_MASK 0x3F
+#define BUCK1_IDLE_DEFAULT 0x14
+
+/* BD71837_REG_BUCK2_VOLT_RUN bits */
+#define BUCK2_RUN_MASK 0x3F
+#define BUCK2_RUN_DEFAULT 0x1E
+
+/* BD71837_REG_BUCK2_VOLT_IDLE bits */
+#define BUCK2_IDLE_MASK 0x3F
+#define BUCK2_IDLE_DEFAULT 0x14
+
+/* BD71837_REG_BUCK3_VOLT_RUN bits */
+#define BUCK3_RUN_MASK 0x3F
+#define BUCK3_RUN_DEFAULT 0x1E
+
+/* BD71837_REG_BUCK4_VOLT_RUN bits */
+#define BUCK4_RUN_MASK 0x3F
+#define BUCK4_RUN_DEFAULT 0x1E
+
+/* BD71837_REG_BUCK5_VOLT bits */
+#define BUCK5_MASK 0x07
+#define BUCK5_DEFAULT 0x02
+
+/* BD71837_REG_BUCK6_VOLT bits */
+#define BUCK6_MASK 0x03
+#define BUCK6_DEFAULT 0x03
+
+/* BD71837_REG_BUCK7_VOLT bits */
+#define BUCK7_MASK 0x07
+#define BUCK7_DEFAULT 0x03
+
+/* BD71837_REG_BUCK8_VOLT bits */
+#define BUCK8_MASK 0x3F
+#define BUCK8_DEFAULT 0x1E
+
+/* BD71837_REG_IRQ bits */
+#define IRQ_SWRST 0x40
+#define IRQ_PWRON_S 0x20
+#define IRQ_PWRON_L 0x10
+#define IRQ_PWRON 0x08
+#define IRQ_WDOG 0x04
+#define IRQ_ON_REQ 0x02
+#define IRQ_STBY_REQ 0x01
+
+/* BD71837_REG_OUT32K bits */
+#define BD71837_OUT32K_EN 0x01
+
+/* BD71837 gated clock rate */
+#define BD71837_CLK_RATE 32768
+
+/* BD71837 irqs */
+enum {
+ BD71837_INT_STBY_REQ,
+ BD71837_INT_ON_REQ,
+ BD71837_INT_WDOG,
+ BD71837_INT_PWRBTN,
+ BD71837_INT_PWRBTN_L,
+ BD71837_INT_PWRBTN_S,
+ BD71837_INT_SWRST
+};
+
+/* BD71837 interrupt masks */
+#define BD71837_INT_SWRST_MASK 0x40
+#define BD71837_INT_PWRBTN_S_MASK 0x20
+#define BD71837_INT_PWRBTN_L_MASK 0x10
+#define BD71837_INT_PWRBTN_MASK 0x8
+#define BD71837_INT_WDOG_MASK 0x4
+#define BD71837_INT_ON_REQ_MASK 0x2
+#define BD71837_INT_STBY_REQ_MASK 0x1
+
+/* BD71837_REG_LDO1_VOLT bits */
+#define LDO1_MASK 0x03
+
+/* BD71837_REG_LDO1_VOLT bits */
+#define LDO2_MASK 0x20
+
+/* BD71837_REG_LDO3_VOLT bits */
+#define LDO3_MASK 0x0F
+
+/* BD71837_REG_LDO4_VOLT bits */
+#define LDO4_MASK 0x0F
+
+/* BD71837_REG_LDO5_VOLT bits */
+#define LDO5_MASK 0x0F
+
+/* BD71837_REG_LDO6_VOLT bits */
+#define LDO6_MASK 0x0F
+
+/* BD71837_REG_LDO7_VOLT bits */
+#define LDO7_MASK 0x0F
+
+struct bd71837;
+struct bd71837_pmic;
+struct bd71837_clk;
+
+/*
+ * Board platform data may be used to initialize regulators.
+ */
+
+struct bd71837_board {
+ struct regulator_init_data *init_data[BD71837_REGULATOR_CNT];
+ int gpio_intr;
+ int irq_base;
+};
+
+struct bd71837 {
+ struct device *dev;
+ struct i2c_client *i2c_client;
+ struct regmap *regmap;
+ unsigned long int id;
+
+ int chip_irq;
+ struct regmap_irq_chip_data *irq_data;
+
+ struct bd71837_pmic *pmic;
+ struct bd71837_clk *clk;
+
+ struct bd71837_board *of_plat_data;
+};
+
+/*
+ * bd71837 sub-driver chip access routines
+ */
+
+static inline int bd71837_reg_read(struct bd71837 *bd71837, u8 reg)
+{
+ int r, val;
+
+ r = regmap_read(bd71837->regmap, reg, &val);
+ if (r < 0)
+ return r;
+ return val;
+}
+
+static inline int bd71837_reg_write(struct bd71837 *bd71837, u8 reg,
+ unsigned int val)
+{
+ return regmap_write(bd71837->regmap, reg, val);
+}
+
+static inline int bd71837_set_bits(struct bd71837 *bd71837, u8 reg, u8 mask)
+{
+ return regmap_update_bits(bd71837->regmap, reg, mask, mask);
+}
+
+static inline int bd71837_clear_bits(struct bd71837 *bd71837, u8 reg,
+ u8 mask)
+{
+ return regmap_update_bits(bd71837->regmap, reg, mask, 0);
+}
+
+static inline int bd71837_update_bits(struct bd71837 *bd71837, u8 reg,
+ u8 mask, u8 val)
+{
+ return regmap_update_bits(bd71837->regmap, reg, mask, val);
+}
+
+#endif /* __LINUX_MFD_BD71837_H__ */
--
2.14.3
^ permalink raw reply related
* [PATCH v5 2/4] mfd: bd71837: Devicetree bindings for ROHM BD71837 PMIC
From: Matti Vaittinen @ 2018-06-04 13:18 UTC (permalink / raw)
To: mturquette, sboyd, robh+dt, mark.rutland, lee.jones, lgirdwood,
broonie, mazziesaccount
Cc: linux-clk, devicetree, linux-kernel, mikko.mutanen,
heikki.haikola
In-Reply-To: <cover.1528117485.git.matti.vaittinen@fi.rohmeurope.com>
Document devicetree bindings for ROHM BD71837 PMIC MFD.
Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
---
.../devicetree/bindings/mfd/rohm,bd71837-pmic.txt | 76 ++++++++++++++++++++++
1 file changed, 76 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt
diff --git a/Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt b/Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt
new file mode 100644
index 000000000000..ac2b66181f17
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt
@@ -0,0 +1,76 @@
+* ROHM BD71837 Power Management Integrated Circuit bindings
+
+BD71837MWV is a programmable Power Management IC for powering single-core,
+dual-core, and quad-core SoC’s such as NXP-i.MX 8M. It is optimized for
+low BOM cost and compact solution footprint. It integrates 8 Buck
+egulators and 7 LDO’s to provide all the power rails required by the SoC and
+the commonly used peripherals.
+
+Required properties:
+ - compatible : Should be "rohm,bd71837".
+ - reg : I2C slave address.
+ - interrupt-parent : Phandle to the parent interrupt controller.
+ - interrupts : The interrupt line the device is connected to.
+ - regulators: : List of child nodes that specify the regulators
+ Please see ../regulator/rohm,bd71837-regulator.txt
+ - clock: : Please see ../clock/rohm,bd71837-clock.txt
+
+Optional properties:
+ - interrupt-controller : Marks the device node as an interrupt controller.
+ BD71837MWV can report different power state change
+ events to other drivers. Different events can be seen
+ as separate BD71837 domain interrupts.
+ The BD71837 driver only provides the infrastructure
+ for the IRQs. The users should write own driver to
+ convert the IRQ into the event they wish. The IRQ can
+ be used with the standard
+ request_irq/enable_irq/disable_irq API inside the
+ kernel.
+ - #interrupt-cells : The number of cells to describe an IRQ should be 1.
+ The value in cell is the IRQ number.
+ Meaningfull numbers are:
+ 0 => PMIC_STBY_REQ level change
+ 1 => PMIC_ON_REQ level change
+ 2 => WDOG_B level change
+ 3 => Power Button level change
+ 4 => Power Button Long Push
+ 5 => Power Button Short Push
+ 6 => SWRESET register is written 1
+
+Example:
+
+ pmic: pmic@4b {
+ compatible = "rohm,bd71837";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0x4b>;
+ interrupt-parent = <&gpio1>;
+ interrupts = <29 GPIO_ACTIVE_LOW>;
+ interrupt-names = "irq";
+ #interrupt-cells = <1>;
+ interrupt-controller;
+ #clock-cells = <0>;
+ clock-frequency = <32768>;
+
+ regulators {
+ buck1: BUCK1 {
+ regulator-name = "buck1";
+ regulator-min-microvolt = <700000>;
+ regulator-max-microvolt = <1300000>;
+ regulator-boot-on;
+ regulator-ramp-delay = <1250>;
+ };
+ /* ... */
+ };
+ };
+
+ /* driver consuming PMIC interrupts */
+
+ my-power-button: power-button {
+ compatible = "foo";
+ interrupt-parent = <&pmic>;
+ interrupts = <3>, <4>, <5>;
+ interrupt-names = "pwrb", "pwrb-l", "pwrb-s";
+ /* ... */
+ };
+
--
2.14.3
^ permalink raw reply related
* Re: [PATCH v2 5/5] venus: register separate driver for firmware device
From: Tomasz Figa @ 2018-06-04 13:18 UTC (permalink / raw)
To: vgarodia
Cc: Hans Verkuil, Mauro Carvalho Chehab, Rob Herring, Mark Rutland,
andy.gross, bjorn.andersson, Stanimir Varbanov,
Linux Media Mailing List, Linux Kernel Mailing List,
linux-arm-msm, linux-soc, devicetree, Alexandre Courbot
In-Reply-To: <1527884768-22392-6-git-send-email-vgarodia@codeaurora.org>
Hi Vikash,
On Sat, Jun 2, 2018 at 5:27 AM Vikash Garodia <vgarodia@codeaurora.org> wrote:
>
> A separate child device is added for video firmware.
> This is needed to
> [1] configure the firmware context bank with the desired SID.
> [2] ensure that the iova for firmware region is from 0x0.
>
> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
> ---
> .../devicetree/bindings/media/qcom,venus.txt | 8 +++-
> drivers/media/platform/qcom/venus/core.c | 48 +++++++++++++++++++---
> drivers/media/platform/qcom/venus/firmware.c | 20 ++++++++-
> drivers/media/platform/qcom/venus/firmware.h | 2 +
> 4 files changed, 71 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/media/qcom,venus.txt b/Documentation/devicetree/bindings/media/qcom,venus.txt
> index 00d0d1b..701cbe8 100644
> --- a/Documentation/devicetree/bindings/media/qcom,venus.txt
> +++ b/Documentation/devicetree/bindings/media/qcom,venus.txt
> @@ -53,7 +53,7 @@
>
> * Subnodes
> The Venus video-codec node must contain two subnodes representing
> -video-decoder and video-encoder.
> +video-decoder and video-encoder, one optional firmware subnode.
>
> Every of video-encoder or video-decoder subnode should have:
>
> @@ -79,6 +79,8 @@ Every of video-encoder or video-decoder subnode should have:
> power domain which is responsible for collapsing
> and restoring power to the subcore.
>
> +The firmware sub node must contain the iommus specifiers for ARM9.
Please document the compatible string here as well.
> +
> * An Example
> video-codec@1d00000 {
> compatible = "qcom,msm8916-venus";
> @@ -105,4 +107,8 @@ Every of video-encoder or video-decoder subnode should have:
> clock-names = "core";
> power-domains = <&mmcc VENUS_CORE1_GDSC>;
> };
> + venus-firmware {
> + compatible = "qcom,venus-firmware-no-tz";
I don't think "-no-tz" should be mentioned here in DT, since it's a
firmware/software detail.
> + iommus = <&apps_smmu 0x10b2 0x0>;
> + }
> };
> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
> index 101612b..5cfb3c2 100644
> --- a/drivers/media/platform/qcom/venus/core.c
> +++ b/drivers/media/platform/qcom/venus/core.c
> @@ -179,6 +179,19 @@ static u32 to_v4l2_codec_type(u32 codec)
> }
> }
>
> +static int store_firmware_dev(struct device *dev, void *data)
> +{
> + struct venus_core *core = data;
> +
> + if (!core)
> + return -EINVAL;
> +
No need to check this AFAICT.
> + if (of_device_is_compatible(dev->of_node, "qcom,venus-firmware-no-tz"))
> + core->fw.dev = dev;
> +
> + return 0;
> +}
> +
> static int venus_enumerate_codecs(struct venus_core *core, u32 type)
> {
> const struct hfi_inst_ops dummy_ops = {};
> @@ -279,6 +292,13 @@ static int venus_probe(struct platform_device *pdev)
> if (ret < 0)
> goto err_runtime_disable;
>
> + ret = of_platform_populate(dev->of_node, NULL, NULL, dev);
> + if (ret)
> + goto err_runtime_disable;
> +
> + /* Attempt to store firmware device */
> + device_for_each_child(dev, core, store_firmware_dev);
> +
> ret = venus_boot(core);
> if (ret)
> goto err_runtime_disable;
> @@ -303,10 +323,6 @@ static int venus_probe(struct platform_device *pdev)
> if (ret)
> goto err_core_deinit;
>
> - ret = of_platform_populate(dev->of_node, NULL, NULL, dev);
> - if (ret)
> - goto err_dev_unregister;
> -
> ret = pm_runtime_put_sync(dev);
> if (ret)
> goto err_dev_unregister;
> @@ -483,7 +499,29 @@ static __maybe_unused int venus_runtime_resume(struct device *dev)
> .pm = &venus_pm_ops,
> },
> };
> -module_platform_driver(qcom_venus_driver);
> +
> +static int __init venus_init(void)
> +{
> + int ret;
> +
> + ret = platform_driver_register(&qcom_video_firmware_driver);
> + if (ret)
> + return ret;
Do we really need this firmware driver? As far as I can see, the
approach used here should work even without any driver bound to the
firmware device.
Best regards,
Tomasz
^ permalink raw reply
* [PATCH v5 3/4] clk: bd71837: Devicetree bindings for ROHM BD71837 PMIC
From: Matti Vaittinen @ 2018-06-04 13:18 UTC (permalink / raw)
To: mturquette, sboyd, robh+dt, mark.rutland, lee.jones, lgirdwood,
broonie, mazziesaccount
Cc: linux-clk, devicetree, linux-kernel, mikko.mutanen,
heikki.haikola
In-Reply-To: <cover.1528117485.git.matti.vaittinen@fi.rohmeurope.com>
Document devicetree bindings for ROHM BD71837 PMIC clock output.
Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
---
.../bindings/clock/rohm,bd71837-clock.txt | 38 ++++++++++++++++++++++
1 file changed, 38 insertions(+)
create mode 100644 Documentation/devicetree/bindings/clock/rohm,bd71837-clock.txt
diff --git a/Documentation/devicetree/bindings/clock/rohm,bd71837-clock.txt b/Documentation/devicetree/bindings/clock/rohm,bd71837-clock.txt
new file mode 100644
index 000000000000..771acfe34114
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/rohm,bd71837-clock.txt
@@ -0,0 +1,38 @@
+ROHM BD71837 Power Management Integrated Circuit clock bindings
+
+This is a part of device tree bindings of ROHM BD71837 multi-function
+device. See generic BD71837 MFD bindings at:
+ Documentation/devicetree/bindings/mfd/rohm,bd71837-pmic.txt
+
+BD71837 contains one 32,768 KHz clock output which can be enabled and
+disabled via i2c.
+
+Following properties should be present in main device node of the MFD chip.
+
+Required properties:
+- clock-frequency : Should be 32768
+- #clock-cells : Should be 0
+
+Optional properties:
+- clock-output-names : Should contain name for output clock.
+
+Example:
+
+/* MFD node */
+
+pmic: pmic@4b {
+ compatible = "rohm,bd71837";
+ /* ... */
+ #clock-cells = <0>;
+ clock-frequency = <32768>;
+ /* ... */
+};
+
+/* Clock consumer node */
+
+foo@0 {
+ compatible = "bar,foo";
+ /* ... */
+ clock-names = "my-clock";
+ clocks = <&pmic>;
+};
--
2.14.3
^ permalink raw reply related
* [PATCH v5 4/4] clk: bd71837: Add driver for BD71837 PMIC clock
From: Matti Vaittinen @ 2018-06-04 13:19 UTC (permalink / raw)
To: mturquette, sboyd, robh+dt, mark.rutland, lee.jones, lgirdwood,
broonie, mazziesaccount
Cc: linux-clk, devicetree, linux-kernel, mikko.mutanen,
heikki.haikola
In-Reply-To: <cover.1528117485.git.matti.vaittinen@fi.rohmeurope.com>
Support BD71837 gateable 32768 Hz clock.
Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
---
drivers/clk/Kconfig | 7 +++
drivers/clk/Makefile | 1 +
drivers/clk/clk-bd71837.c | 146 ++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 154 insertions(+)
create mode 100644 drivers/clk/clk-bd71837.c
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 41492e980ef4..e693496f202a 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -279,6 +279,13 @@ config COMMON_CLK_STM32H7
---help---
Support for stm32h7 SoC family clocks
+config COMMON_CLK_BD71837
+ tristate "Clock driver for ROHM BD71837 PMIC MFD"
+ depends on MFD_BD71837
+ help
+ This driver supports ROHM BD71837 PMIC clock.
+
+
source "drivers/clk/bcm/Kconfig"
source "drivers/clk/hisilicon/Kconfig"
source "drivers/clk/imgtec/Kconfig"
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index de6d06ac790b..8393c4af7d5a 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -21,6 +21,7 @@ endif
obj-$(CONFIG_MACH_ASM9260) += clk-asm9260.o
obj-$(CONFIG_COMMON_CLK_AXI_CLKGEN) += clk-axi-clkgen.o
obj-$(CONFIG_ARCH_AXXIA) += clk-axm5516.o
+obj-$(CONFIG_COMMON_CLK_BD71837) += clk-bd71837.o
obj-$(CONFIG_COMMON_CLK_CDCE706) += clk-cdce706.o
obj-$(CONFIG_COMMON_CLK_CDCE925) += clk-cdce925.o
obj-$(CONFIG_ARCH_CLPS711X) += clk-clps711x.o
diff --git a/drivers/clk/clk-bd71837.c b/drivers/clk/clk-bd71837.c
new file mode 100644
index 000000000000..5ba6c05c5a98
--- /dev/null
+++ b/drivers/clk/clk-bd71837.c
@@ -0,0 +1,146 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2018 ROHM Semiconductors
+// bd71837.c -- ROHM BD71837MWV clock driver
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/err.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/mfd/bd71837.h>
+#include <linux/clk-provider.h>
+#include <linux/clkdev.h>
+
+
+struct bd71837_clk {
+ struct clk_hw hw;
+ uint8_t reg;
+ uint8_t mask;
+ unsigned long rate;
+ struct platform_device *pdev;
+ struct bd71837 *mfd;
+};
+
+static int bd71837_clk_set(struct clk_hw *hw, int status)
+{
+ struct bd71837_clk *c = container_of(hw, struct bd71837_clk, hw);
+
+ return bd71837_update_bits(c->mfd, c->reg, c->mask, status);
+}
+
+static void bd71837_clk_disable(struct clk_hw *hw)
+{
+ int rv;
+ struct bd71837_clk *c = container_of(hw, struct bd71837_clk, hw);
+
+ rv = bd71837_clk_set(hw, 0);
+ if (rv)
+ dev_dbg(&c->pdev->dev, "Failed to disable 32K clk (%d)\n", rv);
+}
+
+static int bd71837_clk_enable(struct clk_hw *hw)
+{
+ return bd71837_clk_set(hw, 1);
+}
+
+static int bd71837_clk_is_enabled(struct clk_hw *hw)
+{
+ struct bd71837_clk *c = container_of(hw, struct bd71837_clk, hw);
+
+ return c->mask & bd71837_reg_read(c->mfd, c->reg);
+}
+
+static unsigned long bd71837_clk_recalc_rate(struct clk_hw *hw,
+ unsigned long parent_rate)
+{
+ struct bd71837_clk *c = container_of(hw, struct bd71837_clk, hw);
+
+ return c->rate;
+}
+
+static const struct clk_ops bd71837_clk_ops = {
+ .recalc_rate = &bd71837_clk_recalc_rate,
+ .prepare = &bd71837_clk_enable,
+ .unprepare = &bd71837_clk_disable,
+ .is_prepared = &bd71837_clk_is_enabled,
+};
+
+static int bd71837_clk_probe(struct platform_device *pdev)
+{
+ struct bd71837_clk *c;
+ int rval = -ENOMEM;
+ struct bd71837 *mfd = dev_get_drvdata(pdev->dev.parent);
+ struct clk_init_data init = {
+ .name = "bd71837-32k-out",
+ .ops = &bd71837_clk_ops,
+ };
+
+ c = devm_kzalloc(&pdev->dev, sizeof(*c), GFP_KERNEL);
+ if (!c)
+ goto err_out;
+
+ c->reg = BD71837_REG_OUT32K;
+ c->mask = BD71837_OUT32K_EN;
+ c->rate = BD71837_CLK_RATE;
+ c->mfd = mfd;
+ c->pdev = pdev;
+
+ of_property_read_string_index(pdev->dev.parent->of_node,
+ "clock-output-names", 0,
+ &init.name);
+
+ c->hw.init = &init;
+
+ rval = devm_clk_hw_register(&pdev->dev, &c->hw);
+ if (rval) {
+ dev_err(&pdev->dev, "failed to register 32K clk");
+ goto err_out;
+ }
+
+ if (pdev->dev.parent->of_node) {
+ rval = of_clk_add_hw_provider(pdev->dev.parent->of_node,
+ of_clk_hw_simple_get,
+ &c->hw);
+ if (rval) {
+ dev_err(&pdev->dev, "adding clk provider failed\n");
+ goto err_out;
+ }
+ }
+
+ rval = clk_hw_register_clkdev(&c->hw, init.name, NULL);
+ if (rval) {
+ dev_err(&pdev->dev, "failed to register clkdev for bd71837");
+ goto err_clean_provider;
+ }
+
+ platform_set_drvdata(pdev, c);
+
+ return 0;
+
+err_clean_provider:
+ of_clk_del_provider(pdev->dev.parent->of_node);
+err_out:
+ return rval;
+}
+
+static int bd71837_clk_remove(struct platform_device *pdev)
+{
+ if (pdev->dev.parent->of_node)
+ of_clk_del_provider(pdev->dev.parent->of_node);
+ return 0;
+}
+
+static struct platform_driver bd71837_clk = {
+ .driver = {
+ .name = "bd71837-clk",
+ },
+ .probe = bd71837_clk_probe,
+ .remove = bd71837_clk_remove,
+};
+
+module_platform_driver(bd71837_clk);
+
+MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>");
+MODULE_DESCRIPTION("BD71837 chip clk driver");
+MODULE_LICENSE("GPL");
--
2.14.3
^ permalink raw reply related
* Re: [PATCH v2] of: platform: stop accessing invalid dev in of_platform_device_destroy
From: Rob Herring @ 2018-06-04 13:44 UTC (permalink / raw)
To: Srinivas Kandagatla
Cc: Frank Rowand, linux-arm-msm, Banajit Goswami, devicetree,
linux-kernel@vger.kernel.org,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
Rohit Kumar
In-Reply-To: <20180602000343.20045-1-srinivas.kandagatla@linaro.org>
On Fri, Jun 1, 2018 at 7:03 PM, Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
> Immediately after the platform_device_unregister() the device will be cleaned up.
> Accessing the freed pointer immediately after that will crash the system.
>
> Found this bug when kernel is built with CONFIG_PAGE_POISONING and testing
> loading/unloading audio drivers in a loop on Qcom platforms.
Curious, does the unittest not catch this too?
>
> Fix this by removing accessing the dev pointer.
> Below is the carsh trace:
s/carsh/crash/
[...]
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index c00d81dfac0b..84c5c899187b 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -529,10 +529,13 @@ arch_initcall_sync(of_platform_default_populate_init);
>
> int of_platform_device_destroy(struct device *dev, void *data)
> {
> + struct device_node *np;
> +
> /* Do not touch devices not populated from the device tree */
> if (!dev->of_node || !of_node_check_flag(dev->of_node, OF_POPULATED))
> return 0;
>
> + np = dev->of_node;
> /* Recurse for any nodes that were treated as busses */
> if (of_node_check_flag(dev->of_node, OF_POPULATED_BUS))
> device_for_each_child(dev, NULL, of_platform_device_destroy);
> @@ -544,8 +547,8 @@ int of_platform_device_destroy(struct device *dev, void *data)
> amba_device_unregister(to_amba_device(dev));
> #endif
>
> - of_node_clear_flag(dev->of_node, OF_POPULATED);
> - of_node_clear_flag(dev->of_node, OF_POPULATED_BUS);
Just move these 2 lines to before unregister calls.
> + of_node_clear_flag(np, OF_POPULATED);
> + of_node_clear_flag(np, OF_POPULATED_BUS);
> return 0;
> }
> EXPORT_SYMBOL_GPL(of_platform_device_destroy);
> --
> 2.16.2
>
^ permalink raw reply
* Re: [PATCH 09/10] dpaa_eth: add support for hardware timestamping
From: Richard Cochran @ 2018-06-04 13:49 UTC (permalink / raw)
To: Yangbo Lu
Cc: netdev, madalin.bucur, Rob Herring, Shawn Guo, David S . Miller,
devicetree, linuxppc-dev, linux-arm-kernel, linux-kernel
In-Reply-To: <20180604070837.19265-10-yangbo.lu@nxp.com>
On Mon, Jun 04, 2018 at 03:08:36PM +0800, Yangbo Lu wrote:
> +if FSL_DPAA_ETH
> +config FSL_DPAA_ETH_TS
> + bool "DPAA hardware timestamping support"
> + select PTP_1588_CLOCK_QORIQ
> + default n
> + help
> + Enable DPAA hardware timestamping support.
> + This option is useful for applications to get
> + hardware time stamps on the Ethernet packets
> + using the SO_TIMESTAMPING API.
> +endif
You should drop this #ifdef. In general, if a MAC supports time
stamping and PHC, then the driver support should simply be compiled
in.
[ When time stamping incurs a large run time performance penalty to
non-PTP users, then it might make sense to have a Kconfig option to
disable it, but that doesn't appear to be the case here. ]
> @@ -1615,6 +1635,24 @@ static int dpaa_eth_refill_bpools(struct dpaa_priv *priv)
> skbh = (struct sk_buff **)phys_to_virt(addr);
> skb = *skbh;
>
> +#ifdef CONFIG_FSL_DPAA_ETH_TS
> + if (priv->tx_tstamp &&
> + skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) {
This condition fits on one line easily.
> + struct skb_shared_hwtstamps shhwtstamps;
> + u64 ns;
Local variables belong at the top of the function.
> + memset(&shhwtstamps, 0, sizeof(shhwtstamps));
> +
> + if (!dpaa_get_tstamp_ns(priv->net_dev, &ns,
> + priv->mac_dev->port[TX],
> + (void *)skbh)) {
> + shhwtstamps.hwtstamp = ns_to_ktime(ns);
> + skb_tstamp_tx(skb, &shhwtstamps);
> + } else {
> + dev_warn(dev, "dpaa_get_tstamp_ns failed!\n");
> + }
> + }
> +#endif
> if (unlikely(qm_fd_get_format(fd) == qm_fd_sg)) {
> nr_frags = skb_shinfo(skb)->nr_frags;
> dma_unmap_single(dev, addr, qm_fd_get_offset(fd) +
> @@ -2086,6 +2124,14 @@ static int dpaa_start_xmit(struct sk_buff *skb, struct net_device *net_dev)
> if (unlikely(err < 0))
> goto skb_to_fd_failed;
>
> +#ifdef CONFIG_FSL_DPAA_ETH_TS
> + if (priv->tx_tstamp &&
> + skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) {
One line please.
> + fd.cmd |= FM_FD_CMD_UPD;
> + skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
> + }
> +#endif
> +
> if (likely(dpaa_xmit(priv, percpu_stats, queue_mapping, &fd) == 0))
> return NETDEV_TX_OK;
>
Thanks,
Richard
^ permalink raw reply
* Re: [PATCH v2 2/5] media: venus: add a routine to set venus state
From: Stanimir Varbanov @ 2018-06-04 13:50 UTC (permalink / raw)
To: Vikash Garodia, hverkuil, mchehab, robh, mark.rutland, andy.gross,
bjorn.andersson, stanimir.varbanov
Cc: linux-media, linux-kernel, linux-arm-msm, linux-soc, devicetree,
acourbot
In-Reply-To: <1527884768-22392-3-git-send-email-vgarodia@codeaurora.org>
Hi Vikash,
Thanks for the patch!
On 1.06.2018 23:26, Vikash Garodia wrote:
> Add a new routine which abstracts the TZ call to
Actually the new routine abstracts Venus CPU state, Isn't it?
> set the video hardware state.
>
> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
> ---
> drivers/media/platform/qcom/venus/core.h | 5 +++++
> drivers/media/platform/qcom/venus/firmware.c | 28 +++++++++++++++++++++++++++
> drivers/media/platform/qcom/venus/firmware.h | 1 +
> drivers/media/platform/qcom/venus/hfi_venus.c | 13 ++++---------
> 4 files changed, 38 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
> index 85e66e2..e7bfb63 100644
> --- a/drivers/media/platform/qcom/venus/core.h
> +++ b/drivers/media/platform/qcom/venus/core.h
> @@ -35,6 +35,11 @@ struct reg_val {
> u32 value;
> };
>
> +enum tzbsp_video_state {
> + TZBSP_VIDEO_SUSPEND = 0,
> + TZBSP_VIDEO_RESUME
> +};
please move this in firmware.c, for more see below.
> +
> struct venus_resources {
> u64 dma_mask;
> const struct freq_tbl *freq_tbl;
> diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c
> index 7d89b5a..b4664ed 100644
> --- a/drivers/media/platform/qcom/venus/firmware.c
> +++ b/drivers/media/platform/qcom/venus/firmware.c
> @@ -53,6 +53,34 @@ static void venus_reset_hw(struct venus_core *core)
> /* Bring Arm9 out of reset */
> writel_relaxed(0, reg_base + WRAPPER_A9SS_SW_RESET);
> }
> +
> +int venus_set_hw_state(enum tzbsp_video_state state, struct venus_core *core)
can we put this function this way:
venus_set_state(struct venus_core *core, bool on)
so we set the state to On when we are power-up the venus CPU and Off
when we power-down.
by this way we really abstract the state, IMO.
> +{
> + int ret;
> + struct device *dev = core->dev;
> + void __iomem *reg_base = core->base;
just 'base' should be enough.
> +
> + switch (state) {
> + case TZBSP_VIDEO_SUSPEND:
> + if (qcom_scm_is_available())
You really shouldn't rely on this function (see the comment from Bjorn
on first version of this patch series).
I think we have to replace qcom_scm_is_available() with some flag which
is reflecting does the firmware subnode is exist or not. In case it is
not exist the we have to go with TZ scm calls.
> + ret = qcom_scm_set_remote_state(TZBSP_VIDEO_SUSPEND, 0);
> + else
> + writel_relaxed(1, reg_base + WRAPPER_A9SS_SW_RESET);
> + break;
> + case TZBSP_VIDEO_RESUME:
> + if (qcom_scm_is_available())
> + ret = qcom_scm_set_remote_state(TZBSP_VIDEO_RESUME, 0);
> + else
> + venus_reset_hw(core);
> + break;
> + default:
> + dev_err(dev, "invalid state\n");
> + break;
> + }
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(venus_set_hw_state);
> +
regards,
Stan
^ permalink raw reply
* Re: [PATCH v2] of: platform: stop accessing invalid dev in of_platform_device_destroy
From: Srinivas Kandagatla @ 2018-06-04 13:54 UTC (permalink / raw)
To: Rob Herring
Cc: Frank Rowand, linux-arm-msm, Banajit Goswami, devicetree,
linux-kernel@vger.kernel.org,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
Rohit Kumar
In-Reply-To: <CAL_JsqJ9AMDiYCfgGgHZ=XL_1yJjTdSA4Jb_GqHAZML3jwB6+Q@mail.gmail.com>
On 04/06/18 14:44, Rob Herring wrote:
> On Fri, Jun 1, 2018 at 7:03 PM, Srinivas Kandagatla
> <srinivas.kandagatla@linaro.org> wrote:
>> Immediately after the platform_device_unregister() the device will be cleaned up.
>> Accessing the freed pointer immediately after that will crash the system.
>>
>> Found this bug when kernel is built with CONFIG_PAGE_POISONING and testing
>> loading/unloading audio drivers in a loop on Qcom platforms.
>
> Curious, does the unittest not catch this too?
>
Not sure!
>>
>> Fix this by removing accessing the dev pointer.
>> Below is the carsh trace:
>
> s/carsh/crash/
Yep.
>
> [...]
>
>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
>> index c00d81dfac0b..84c5c899187b 100644
>> --- a/drivers/of/platform.c
>> +++ b/drivers/of/platform.c
>> @@ -529,10 +529,13 @@ arch_initcall_sync(of_platform_default_populate_init);
>>
>> int of_platform_device_destroy(struct device *dev, void *data)
>> {
>> + struct device_node *np;
>> +
>> /* Do not touch devices not populated from the device tree */
>> if (!dev->of_node || !of_node_check_flag(dev->of_node, OF_POPULATED))
>> return 0;
>>
>> + np = dev->of_node;
>> /* Recurse for any nodes that were treated as busses */
>> if (of_node_check_flag(dev->of_node, OF_POPULATED_BUS))
>> device_for_each_child(dev, NULL, of_platform_device_destroy);
>> @@ -544,8 +547,8 @@ int of_platform_device_destroy(struct device *dev, void *data)
>> amba_device_unregister(to_amba_device(dev));
>> #endif
>>
>> - of_node_clear_flag(dev->of_node, OF_POPULATED);
>> - of_node_clear_flag(dev->of_node, OF_POPULATED_BUS);
>
> Just move these 2 lines to before unregister calls.
Make sense.. I will do that in v3.
thanks,
srini
>
>> + of_node_clear_flag(np, OF_POPULATED);
>> + of_node_clear_flag(np, OF_POPULATED_BUS);
>> return 0;
>> }
>> EXPORT_SYMBOL_GPL(of_platform_device_destroy);
>> --
>> 2.16.2
>>
^ permalink raw reply
* Re: [PATCH v2 5/5] venus: register separate driver for firmware device
From: Stanimir Varbanov @ 2018-06-04 13:56 UTC (permalink / raw)
To: Tomasz Figa, vgarodia
Cc: Hans Verkuil, Mauro Carvalho Chehab, Rob Herring, Mark Rutland,
andy.gross, bjorn.andersson, Linux Media Mailing List,
Linux Kernel Mailing List, linux-arm-msm, linux-soc, devicetree,
Alexandre Courbot
In-Reply-To: <CAAFQd5D39CkA=GucUs7YOHwsdj0gbk55BiY_gSvArY_RH4uDkg@mail.gmail.com>
Hi Tomasz,
On 06/04/2018 04:18 PM, Tomasz Figa wrote:
> Hi Vikash,
>
> On Sat, Jun 2, 2018 at 5:27 AM Vikash Garodia <vgarodia@codeaurora.org> wrote:
>>
>> A separate child device is added for video firmware.
>> This is needed to
>> [1] configure the firmware context bank with the desired SID.
>> [2] ensure that the iova for firmware region is from 0x0.
>>
>> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
>> ---
>> .../devicetree/bindings/media/qcom,venus.txt | 8 +++-
>> drivers/media/platform/qcom/venus/core.c | 48 +++++++++++++++++++---
>> drivers/media/platform/qcom/venus/firmware.c | 20 ++++++++-
>> drivers/media/platform/qcom/venus/firmware.h | 2 +
>> 4 files changed, 71 insertions(+), 7 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/media/qcom,venus.txt b/Documentation/devicetree/bindings/media/qcom,venus.txt
>> index 00d0d1b..701cbe8 100644
>> --- a/Documentation/devicetree/bindings/media/qcom,venus.txt
>> +++ b/Documentation/devicetree/bindings/media/qcom,venus.txt
>> @@ -53,7 +53,7 @@
>>
>> * Subnodes
>> The Venus video-codec node must contain two subnodes representing
>> -video-decoder and video-encoder.
>> +video-decoder and video-encoder, one optional firmware subnode.
>>
>> Every of video-encoder or video-decoder subnode should have:
>>
>> @@ -79,6 +79,8 @@ Every of video-encoder or video-decoder subnode should have:
>> power domain which is responsible for collapsing
>> and restoring power to the subcore.
>>
>> +The firmware sub node must contain the iommus specifiers for ARM9.
>
> Please document the compatible string here as well.
>
>> +
>> * An Example
>> video-codec@1d00000 {
>> compatible = "qcom,msm8916-venus";
>> @@ -105,4 +107,8 @@ Every of video-encoder or video-decoder subnode should have:
>> clock-names = "core";
>> power-domains = <&mmcc VENUS_CORE1_GDSC>;
>> };
>> + venus-firmware {
>> + compatible = "qcom,venus-firmware-no-tz";
>
> I don't think "-no-tz" should be mentioned here in DT, since it's a
> firmware/software detail.
I have to agree with Tomasz, non-tz or tz is a software detail and it
shouldn't be reflected in compatible string.
Also I'm not sure but what will happen if this video-firmware subnode is
not added, do you expect that backward compatibility is satisfied for
older venus versions?
>
>> + iommus = <&apps_smmu 0x10b2 0x0>;
>> + }
>> };
>> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
>> index 101612b..5cfb3c2 100644
>> --- a/drivers/media/platform/qcom/venus/core.c
>> +++ b/drivers/media/platform/qcom/venus/core.c
>> @@ -179,6 +179,19 @@ static u32 to_v4l2_codec_type(u32 codec)
>> }
>> }
>>
>> +static int store_firmware_dev(struct device *dev, void *data)
>> +{
>> + struct venus_core *core = data;
>> +
>> + if (!core)
>> + return -EINVAL;
>> +
>
> No need to check this AFAICT.>
>> + if (of_device_is_compatible(dev->of_node, "qcom,venus-firmware-no-tz"))
>> + core->fw.dev = dev;
>> +
>> + return 0;
>> +}
>> +
>> static int venus_enumerate_codecs(struct venus_core *core, u32 type)
>> {
>> const struct hfi_inst_ops dummy_ops = {};
>> @@ -279,6 +292,13 @@ static int venus_probe(struct platform_device *pdev)
>> if (ret < 0)
>> goto err_runtime_disable;
>>
>> + ret = of_platform_populate(dev->of_node, NULL, NULL, dev);
>> + if (ret)
>> + goto err_runtime_disable;
>> +
>> + /* Attempt to store firmware device */
>> + device_for_each_child(dev, core, store_firmware_dev);
>> +
>> ret = venus_boot(core);
>> if (ret)
>> goto err_runtime_disable;
>> @@ -303,10 +323,6 @@ static int venus_probe(struct platform_device *pdev)
>> if (ret)
>> goto err_core_deinit;
>>
>> - ret = of_platform_populate(dev->of_node, NULL, NULL, dev);
>> - if (ret)
>> - goto err_dev_unregister;
>> -
>> ret = pm_runtime_put_sync(dev);
>> if (ret)
>> goto err_dev_unregister;
>> @@ -483,7 +499,29 @@ static __maybe_unused int venus_runtime_resume(struct device *dev)
>> .pm = &venus_pm_ops,
>> },
>> };
>> -module_platform_driver(qcom_venus_driver);
>> +
>> +static int __init venus_init(void)
>> +{
>> + int ret;
>> +
>> + ret = platform_driver_register(&qcom_video_firmware_driver);
>> + if (ret)
>> + return ret;
>
> Do we really need this firmware driver? As far as I can see, the
> approach used here should work even without any driver bound to the
> firmware device.
We need device/driver bind because we need to call dma_configure() which
internally doing iommus sID parsing.
--
regards,
Stan
^ permalink raw reply
* Re: [PATCH v2 3/6] clk: ti: dra7: Add clkctrl clock data for the mcan clocks
From: Faiz Abbas @ 2018-06-04 13:58 UTC (permalink / raw)
To: Tony Lindgren
Cc: Rob Herring, paul, devicetree, linux-kernel, Tero Kristo,
bcousson, linux-omap, linux-clk, linux-arm-kernel
In-Reply-To: <20180601142613.GU5705@atomide.com>
Hi,
On Friday 01 June 2018 07:56 PM, Tony Lindgren wrote:
> * Faiz Abbas <faiz_abbas@ti.com> [180601 06:49]:
>> Hi,
>>
>> On Thursday 31 May 2018 06:59 PM, Tero Kristo wrote:
>>> On 31/05/18 13:14, Faiz Abbas wrote:
>>>> Hi,
>>>>
>>>> On Thursday 31 May 2018 09:33 AM, Rob Herring wrote:
>>>>> On Wed, May 30, 2018 at 07:41:30PM +0530, Faiz Abbas wrote:
>>>>>> Add clkctrl data for the m_can clocks and register it within the
>>>> ...
>>>>>> diff --git a/include/dt-bindings/clock/dra7.h
>>>>>> b/include/dt-bindings/clock/dra7.h
>>>>>> index 5e1061b15aed..d7549c57cac3 100644
>>>>>> --- a/include/dt-bindings/clock/dra7.h
>>>>>> +++ b/include/dt-bindings/clock/dra7.h
>>>>>> @@ -168,5 +168,6 @@
>>>>>> #define DRA7_COUNTER_32K_CLKCTRL DRA7_CLKCTRL_INDEX(0x50)
>>>>>> #define DRA7_UART10_CLKCTRL DRA7_CLKCTRL_INDEX(0x80)
>>>>>> #define DRA7_DCAN1_CLKCTRL DRA7_CLKCTRL_INDEX(0x88)
>>>>>> +#define DRA7_ADC_CLKCTRL DRA7_CLKCTRL_INDEX(0xa0)
>>>>>
>>>>> ADC and mcan are the same thing?
>>>>>
>>>>
>>>> The register to control MCAN clocks is called ADC_CLKCTRL, Yes.
>>>
>>> Is there any reason for this or is that just a documentation bug?
>>>
>>
>> Looks like they meant to have an ADC in dra74 or dra72 but decided
>> against it and then many years later used the same registers for MCAN
>> instead. You can see ADC_CLKCTRL exists in dra72 TRM but is explicitly
>> disabled.
>>
>> http://www.ti.com/lit/ug/spruic2b/spruic2b.pdf pg:1524
>
> How about make add also something like to dra7.h:
>
> #define DRA7_MCAN_CLKCTRL DRA7_ADC_CLKCTRL
>
> And you can add a comment to the dts file to avoid people
> getting confused with this constantly.
>
I would prefer to follow the TRM so that people don't look for registers
that don't exist at all.
Thanks,
Faiz
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 01/13] dt-bindings: net: bluetooth: add support for Realtek Bluetooth chips
From: Hans de Goede @ 2018-06-04 13:58 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Rob Herring, Johan Hedberg, Martin Blumenstingl, Jeremy Cline,
BlueZ development, linux-serial, linux-acpi, devicetree
In-Reply-To: <14D10B2C-319C-4676-AE84-1D76AA603C3B@holtmann.org>
Hi,
On 04-06-18 12:17, Marcel Holtmann wrote:
> Hi Hans,
>
>>>>>> This adds the documentation for Bluetooth functionality of the Realtek
>>>>>> RTL8723BS and RTL8723DS.
>>>>>> Both are SDIO wifi chips with an additional Bluetooth module which is
>>>>>> connected via UART to the host.
>>>>>>
>>>>>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>>>>>> Signed-off-by: Jeremy Cline <jeremy@jcline.org>
>>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>>> ---
>>>>>> .../bindings/net/realtek-bluetooth.txt | 41 +++++++++++++++++++
>>>>>> 1 file changed, 41 insertions(+)
>>>>>> create mode 100644 Documentation/devicetree/bindings/net/realtek-bluetooth.txt
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/net/realtek-bluetooth.txt b/Documentation/devicetree/bindings/net/realtek-bluetooth.txt
>>>>>> new file mode 100644
>>>>>> index 000000000000..1491329c4cd1
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/net/realtek-bluetooth.txt
>>>>>> @@ -0,0 +1,41 @@
>>>>>> +Realtek Bluetooth Chips
>>>>>> +-----------------------
>>>>>> +
>>>>>> +This documents the binding structure and common properties for serial
>>>>>> +attached Realtek devices.
>>>>>> +
>>>>>> +Serial attached Realtek devices shall be a child node of the host UART
>>>>>> +device the slave device is attached to. See ../serial/slave-device.txt
>>>>>> +for more information
>>>>>> +
>>>>>> +Required properties:
>>>>>> +- compatible: should contain one of the following:
>>>>>> + * "realtek,rtl8723bs-bluetooth"
>>>>>> + * "realtek,rtl8723ds-bluetooth"
>>>>>> +
>>>>>> +Optional properties:
>>>>>> +- realtek,config-data: Bluetooth chipset configuration data which is
>>>>>> + needed for communication (it typically contains
>>>>>> + board specific settings like the baudrate and
>>>>>> + whether flow-control is used).
>>>>>> + This is an array of u8 values.
>>>>>> +- enable-gpios: GPIO specifier, used to enable/disable the BT module
>>>>>> +- reset-gpios: GPIO specifier, used to reset the BT module
>>>>>> +
>>>>>> +
>>>>>> +Example:
>>>>>> +
>>>>>> +&uart {
>>>>>> + ...
>>>>>> +
>>>>>> + bluetooth {
>>>>>> + compatible = "realtek,rtl8723bs-bluetooth";
>>>>>> + enable-gpios = <&gpio 20 GPIO_ACTIVE_HIGH>;
>>>>>> + reset-gpios = <&gpio 11 GPIO_ACTIVE_HIGH>;
>>>>>> + realtek,config-data = /bits/ 8 <
>>>>>> + 0x55 0xab 0x23 0x87 0x29 0x00 0xf4 0x00 0x01 0x01 0xf6 0x00
>>>>>> + 0x02 0x81 0x00 0xfa 0x00 0x02 0x12 0x80 0x0c 0x00 0x10 0x02
>>>>>> + 0x80 0x92 0x04 0x50 0xc5 0xea 0x19 0xe1 0x1b 0xfd 0xaf 0x5f
>>>>>> + 0x01 0xa4 0x0b 0xd9 0x00 0x01 0x0f 0xe4 0x00 0x01 0x08>;
>>>>>> + };
>>>>>> +};
>>>>>
>>>>> we actually need to agree on this config-data. As I wrote in an earlier email some time ago, the actual config-data files are just memory portions that will overload the default config area. I wrote tools/rtlfw.c to parse these.
>>>>>
>>>>> So now I wonder if we can just read the current configuration and run with that when no extra blob is provided. That way it would always work and we might just give the config-data filename here. Mostly this is the for PCM and UART settings anyway.
>>>>
>>>> I've been thinking about this too and 2 different solutions come to mind.
>>>>
>>>> Note this is all x86 specific, I think it best to solve this for x86 first
>>>> and then we can apply the lessons learned there to ARM/devicetree when
>>>> someone comes along who wants to hook-up things on ARM.
>>>>
>>>> My first idea was to stick with a config blob using the firmware_load
>>>> mechanism, but to add a postfix to the filename based on the ACPI
>>>> HID (hardware-id) of the serdev device, so in practice this means
>>>> we would try to load:
>>>>
>>>> /lib/firmware/rtl_bt/rtl8723bs_config-OBDA8723.bin
>>>>
>>>> On most x86 devices, so far all my testing on a bunch of different
>>>> devices has shown that the same config works for all x86 devices
>>>> (I took the config from a Chuwi Vi8 tablet which uses 3000000bps
>>>> + hardware flowcontrol).
>>>>
>>>> This way we can put the config in linux-firmware, without it
>>>> getting loaded on ARM boards where it might very well be wrong.
>>>>
>>>>
>>>> My second idea is to include a default config inside the driver,
>>>> which can be optionally overridden by a file in
>>>> /lib/firmware/rtl_bt and/or dt. Combined with module-options to
>>>> override the baudrate and flowcontrol setting (and patch the
>>>> builtin config accordingly).
>>>>
>>>>
>>>> Your idea to read back the config from the device is also
>>>> interesting, but I don't think that will gain us anything because
>>>> AFAIK the whole purpose of the board specific config file
>>>> bits is that the chip lack eeprom space to store this info,
>>>> so we will just always get some generic defaults.
>>>>
>>>>
>>>> I've run your rtlfw tool on a bunch of different config files and
>>>> there are a lot of unknown fields, and even of the known fields
>>>> I don't think we understand all the bits. So all in all the
>>>> config file is a bit of a black box and as such I believe it
>>>> would be best to just treat it as such and I personally
>>>> prefer my first idea, which is to add a postfix to the
>>>> firmware filename request, so on x86 load:
>>>>
>>>> /lib/firmware/rtl_bt/rtl8723bs_config-OBDA8723.bin
>>>>
>>>> And on devicetree we could postfix things with the machine
>>>> model string (as returned by of_flat_dt_get_machine_name())
>>> If the firmware file is going to depend on the DT, then you might as
>>> well just put the data into the DT.
>>
>> Ok, note we don't need the entire firmware in DT, just a (binary
>> format) config file for the firmware. The question is if we are
>> going to try and break up the info from the config-file and then
>> regenerate it inside the kernel driver. Or if we are just going
>> to put the say 60 bytes in DT as an u8 array as the original
>> binding proposed by Martin Blumenstingl suggests.
>>
>> Since we only know the meaning for a couple of the bytes in
>> the file, which is typically 40 - 60 bytes big, I think it
>> is probably best to just treat it as a blob.
>
> wouldn’t it be still better to then just reference the firmware file or an identifier to build it? Putting the pure binary blob into the DT seems kinda counterproductive and hardware to update. After all it most likely depends on the firmware loaded since it is the config ram space that this changes.
Ah I see, so you suggest simply putting an identifier
in the DT which we then use to build the config-file
filename to request. That pretty much matches with what
I have in mind for the x86 code-paths as well as for
the somewhat related broadcom wifi nvram files.
So yes that sounds like a good idea.
With that said, given that I'm only testing this on x86
I think that for v2 of the patch-set it is probably best
to just drop the DT specific bits. Then when someone
comes along who wants to add the necessary glue for DT
based platforms they can brush the dust of the DT specific
patches and submit an actually tested version of them.
So for v2 I plan to just drop the DT bits and add a
patch to use the ACPI HID as an identifier in the
config-file filename requested on ACPI platforms.
> As a note, there are some hints that the config file is required for some firmware since it changes some memory around to make it successful boot.
>
>>> There's a similar issue on QCom Dragonboards. The WiFi (and BT?)
>>> firmware files are supposedly board specific, so folks don't want to put
>>> them into linux-firmware. But it also seems to be unknown as to which
>>> parts are board specific are not.
>>
>> In my experience with broadcom and now the rtl8723bs the
>> actual firmware and the needed config data are separate.
>>
>> Hmm that is actually no entirely true, for broadcom the
>> bluetooth patchram file depends on the clockcrystal freq
>> used on the board, so there are 2 versions of it for a
>> single chip, 1 for each of the 2 different freqs used.
>
> are you using .hex or .hcd files for Broadcom? The .hex files are pure patchram, while the .hcd can actually contain extra HCI commands. I remember that some .hcd files contain also HCI commands to do extra settings. Maybe we need a tool that shows the details of these files. We have this for nokfw and rtlfw now.
I'm using hcd files for broadcom.
Regards,
Hans
^ permalink raw reply
* [PATCH v3] of: platform: stop accessing invalid dev in of_platform_device_destroy
From: Srinivas Kandagatla @ 2018-06-04 14:14 UTC (permalink / raw)
To: robh+dt, frowand.list
Cc: linux-arm-msm, bgoswami, devicetree, linux-kernel,
linux-arm-kernel, rohkumar, Srinivas Kandagatla
Immediately after the platform_device_unregister() the device will be
cleaned up. Accessing the freed pointer immediately after that will
crash the system.
Found this bug when kernel is built with CONFIG_PAGE_POISONING and testing
loading/unloading audio drivers in a loop on Qcom platforms.
Fix this by moving of_node_clear_flag() just before the unregister calls.
Below is the crash trace:
Unable to handle kernel paging request at virtual address 6b6b6b6b6b6c03
Mem abort info:
ESR = 0x96000021
Exception class = DABT (current EL), IL = 32 bits
SET = 0, FnV = 0
EA = 0, S1PTW = 0
Data abort info:
ISV = 0, ISS = 0x00000021
CM = 0, WnR = 0
[006b6b6b6b6b6c03] address between user and kernel address ranges
Internal error: Oops: 96000021 [#1] PREEMPT SMP
Modules linked in:
CPU: 2 PID: 1784 Comm: sh Tainted: G W 4.17.0-rc7-02230-ge3a63a7ef641-dirty #204
Hardware name: Qualcomm Technologies, Inc. APQ 8016 SBC (DT)
pstate: 80000005 (Nzcv daif -PAN -UAO)
pc : clear_bit+0x18/0x2c
lr : of_platform_device_destroy+0x64/0xb8
sp : ffff00000c9c3930
x29: ffff00000c9c3930 x28: ffff80003d39b200
x27: ffff000008bb1000 x26: 0000000000000040
x25: 0000000000000124 x24: ffff80003a9a3080
x23: 0000000000000060 x22: ffff00000939f518
x21: ffff80003aa79e98 x20: ffff80003aa3dae0
x19: ffff80003aa3c890 x18: ffff800009feb794
x17: 0000000000000000 x16: 0000000000000000
x15: ffff800009feb790 x14: 0000000000000000
x13: ffff80003a058778 x12: ffff80003a058728
x11: ffff80003a058750 x10: 0000000000000000
x9 : 0000000000000006 x8 : ffff80003a825988
x7 : bbbbbbbbbbbbbbbb x6 : 0000000000000001
x5 : 0000000000000000 x4 : 0000000000000001
x3 : 0000000000000008 x2 : 0000000000000001
x1 : 6b6b6b6b6b6b6c03 x0 : 0000000000000000
Process sh (pid: 1784, stack limit = 0x (ptrval))
Call trace:
clear_bit+0x18/0x2c
q6afe_remove+0x20/0x38
apr_device_remove+0x30/0x70
device_release_driver_internal+0x170/0x208
device_release_driver+0x14/0x20
bus_remove_device+0xcc/0x150
device_del+0x10c/0x310
device_unregister+0x1c/0x70
apr_remove_device+0xc/0x18
device_for_each_child+0x50/0x80
apr_remove+0x18/0x20
rpmsg_dev_remove+0x38/0x68
device_release_driver_internal+0x170/0x208
device_release_driver+0x14/0x20
bus_remove_device+0xcc/0x150
device_del+0x10c/0x310
device_unregister+0x1c/0x70
qcom_smd_remove_device+0xc/0x18
device_for_each_child+0x50/0x80
qcom_smd_unregister_edge+0x3c/0x70
smd_subdev_remove+0x18/0x28
rproc_stop+0x48/0xd8
rproc_shutdown+0x60/0xe8
state_store+0xbc/0xf8
dev_attr_store+0x18/0x28
sysfs_kf_write+0x3c/0x50
kernfs_fop_write+0x118/0x1e0
__vfs_write+0x18/0x110
vfs_write+0xa4/0x1a8
ksys_write+0x48/0xb0
sys_write+0xc/0x18
el0_svc_naked+0x30/0x34
Code: d2800022 8b400c21 f9800031 9ac32043 (c85f7c22)
---[ end trace 32020935775616a2 ]---
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
Changes since v2:
Move the calls to of_node_clear_flag just before unregister,
suggested by Rob.
drivers/of/platform.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index c00d81dfac0b..9c91f97ffbe1 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -537,6 +537,9 @@ int of_platform_device_destroy(struct device *dev, void *data)
if (of_node_check_flag(dev->of_node, OF_POPULATED_BUS))
device_for_each_child(dev, NULL, of_platform_device_destroy);
+ of_node_clear_flag(dev->of_node, OF_POPULATED);
+ of_node_clear_flag(dev->of_node, OF_POPULATED_BUS);
+
if (dev->bus == &platform_bus_type)
platform_device_unregister(to_platform_device(dev));
#ifdef CONFIG_ARM_AMBA
@@ -544,8 +547,6 @@ int of_platform_device_destroy(struct device *dev, void *data)
amba_device_unregister(to_amba_device(dev));
#endif
- of_node_clear_flag(dev->of_node, OF_POPULATED);
- of_node_clear_flag(dev->of_node, OF_POPULATED_BUS);
return 0;
}
EXPORT_SYMBOL_GPL(of_platform_device_destroy);
--
2.16.2
^ permalink raw reply related
* Re: [PATCH 1/6] PCI: iproc: Update iProc PCI binding for INTx support
From: Rob Herring @ 2018-06-04 14:17 UTC (permalink / raw)
To: Ray Jui, Arnd Bergmann
Cc: Lorenzo Pieralisi, Bjorn Helgaas, Mark Rutland,
linux-kernel@vger.kernel.org,
maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE, linux-pci,
devicetree,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
In-Reply-To: <1527631130-20045-2-git-send-email-ray.jui@broadcom.com>
+Arnd
On Tue, May 29, 2018 at 4:58 PM, Ray Jui <ray.jui@broadcom.com> wrote:
> Update the iProc PCIe binding document for better modeling of the legacy
> interrupt (INTx) support
>
> Signed-off-by: Ray Jui <ray.jui@broadcom.com>
> ---
> .../devicetree/bindings/pci/brcm,iproc-pcie.txt | 31 +++++++++++++++++-----
> 1 file changed, 24 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
> index b8e48b4..7ea24dc 100644
> --- a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
> @@ -13,9 +13,6 @@ controller, used in Stingray
> PAXB-based root complex is used for external endpoint devices. PAXC-based
> root complex is connected to emulated endpoint devices internal to the ASIC
> - reg: base address and length of the PCIe controller I/O register space
> -- #interrupt-cells: set to <1>
> -- interrupt-map-mask and interrupt-map, standard PCI properties to define the
> - mapping of the PCIe interface to interrupt numbers
> - linux,pci-domain: PCI domain ID. Should be unique for each host controller
> - bus-range: PCI bus numbers covered
> - #address-cells: set to <3>
> @@ -41,6 +38,16 @@ Required:
> - brcm,pcie-ob-axi-offset: The offset from the AXI address to the internal
> address used by the iProc PCIe core (not the PCIe address)
>
> +Legacy interrupt (INTx) support (optional):
> +
> +Note INTx is for PAXB only.
> +
> +- interrupt-controller: claims itself as an interrupt controller for INTx
> +- #interrupt-cells: set to <1>
> +- interrupt-map-mask and interrupt-map, standard PCI properties to define
> +the mapping of the PCIe interface to interrupt numbers
> +- interrupts: interrupt line wired to the generic GIC for INTx support
> +
> MSI support (optional):
>
> For older platforms without MSI integrated in the GIC, iProc PCIe core provides
> @@ -77,9 +84,14 @@ Example:
> compatible = "brcm,iproc-pcie";
> reg = <0x18012000 0x1000>;
>
> + interrupt-controller;
> #interrupt-cells = <1>;
> - interrupt-map-mask = <0 0 0 0>;
> - interrupt-map = <0 0 0 0 &gic GIC_SPI 100 IRQ_TYPE_NONE>;
> + interrupt-map-mask = <0 0 0 7>;
> + interrupt-map = <0 0 0 1 &pcie0 1>,
Are you sure this works? The irq parsing code will ignore
interrupt-map if interrupt-controller is found. In other words, you
should have one or the other, but not both.
Maybe it happens to work because "pcie0" is this node and your irq
numbers are the same.
Arnd, any thoughts on this?
> + <0 0 0 2 &pcie0 2>,
> + <0 0 0 3 &pcie0 3>,
> + <0 0 0 4 &pcie0 4>;
> + interrupts = <GIC_SPI 100 IRQ_TYPE_NONE>;
>
> linux,pci-domain = <0>;
>
> @@ -115,9 +127,14 @@ Example:
> compatible = "brcm,iproc-pcie";
> reg = <0x18013000 0x1000>;
>
> + interrupt-controller;
> #interrupt-cells = <1>;
> - interrupt-map-mask = <0 0 0 0>;
> - interrupt-map = <0 0 0 0 &gic GIC_SPI 106 IRQ_TYPE_NONE>;
> + interrupt-map-mask = <0 0 0 7>;
> + interrupt-map = <0 0 0 1 &pcie1 1>,
> + <0 0 0 2 &pcie1 2>,
> + <0 0 0 3 &pcie1 3>,
> + <0 0 0 4 &pcie1 4>;
> + interrupts = <GIC_SPI 106 IRQ_TYPE_NONE>;
>
> linux,pci-domain = <1>;
>
> --
> 2.1.4
>
^ permalink raw reply
* Re: [RFC PATCH 7/8] dts: coresight: Define new bindings for direction of data flow
From: Suzuki K Poulose @ 2018-06-04 14:20 UTC (permalink / raw)
To: Mathieu Poirier
Cc: linux-arm-kernel, sudeep.holla, robh, mark.rutland, frowand.list,
matt.sealey, charles.garcia-tobin, john.horley, mike.leach,
coresight, linux-kernel, devicetree
In-Reply-To: <20180601203928.GD9838@xps15>
On 06/01/2018 09:39 PM, Mathieu Poirier wrote:
> On Fri, Jun 01, 2018 at 02:16:06PM +0100, Suzuki K Poulose wrote:
>> So far we have relied on an undocumented property "slave-mode",
>> to indicate if the given port is input or not. Since we are
>> redefining the coresight bindings, define new property for the
>> "direction" of data flow for a given connection endpoint in the
>> device.
>>
>> Each endpoint must define the following property.
>>
>> - "direction" : 0 => Port is input
>> 1 => Port is output
>>
>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>> ---
>> drivers/hwtracing/coresight/of_coresight.c | 20 ++++++++++++++++----
>
> You haven't documented the binding in bindings/arm/coresight.txt the same way
> you did with "coresight,hwid". I'm guessing you simply forgot to do a "git add"
> on the file when preparing the patchset.
>
>> 1 file changed, 16 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/of_coresight.c b/drivers/hwtracing/coresight/of_coresight.c
>> index 99d7a9c..63c1668 100644
>> --- a/drivers/hwtracing/coresight/of_coresight.c
>> +++ b/drivers/hwtracing/coresight/of_coresight.c
>> @@ -52,7 +52,19 @@ of_coresight_get_endpoint_device(struct device_node *endpoint)
>> endpoint, of_dev_node_match);
>> }
>>
>> -static void of_coresight_get_ports(const struct device_node *node,
>> +static bool of_coresight_ep_is_input(struct device *dev, struct device_node *ep_node)
>
> I suggested of_coresight_endpoint_get_port_id() in my review of 6/8. I'm good
> with either "ep" or "endpoint", as long as the names are consistent.
Yep, that's right. This is what I have for the documentation :
--- a/Documentation/devicetree/bindings/arm/coresight.txt
+++ b/Documentation/devicetree/bindings/arm/coresight.txt
@@ -103,9 +103,11 @@ with a specific direction of data flow, each
connection must define the
following properties to uniquely identify the connection details.
* Direction of the data flow w.r.t the component :
- Each input port must have the following property defined at the
"endpoint"
+ Each hardware port must have the following property defined at the
"endpoint"
for the port.
- "slave-mode"
+ "direction" - 32bit integer, whose values are defined as follows :
+ 0 => the endpoint is an Input port
+ 1 => the endpoint is an Output port.
and changes to the examples as well..
Cheers
Suzuki
^ permalink raw reply
* Re: [PATCH 2/3] arm64: dts: renesas: condor: specify EtherAVB PHY IRQ
From: Sergei Shtylyov @ 2018-06-04 14:22 UTC (permalink / raw)
To: Simon Horman
Cc: Mark Rutland, devicetree, Magnus Damm, Catalin Marinas,
Will Deacon, linux-renesas-soc, Rob Herring, linux-arm-kernel
In-Reply-To: <20180604103302.f5kx5fqhbl3ohfpi@verge.net.au>
On 06/04/2018 01:33 PM, Simon Horman wrote:
>> Specify EtherAVB PHY IRQ in the Condor board's device tree, now that
>> we have the GPIO support (previously phylib had to resort to polling).
>>
>> Based on the original (and large) patch by Vladimir Barinov.
>>
>> Signed-off-by: Vladimir Barinov <vladimir.barinov@cogentembedded.com>
>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>>
>> ---
>> arch/arm64/boot/dts/renesas/r8a77980-condor.dts | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> Index: renesas/arch/arm64/boot/dts/renesas/r8a77980-condor.dts
>> ===================================================================
>> --- renesas.orig/arch/arm64/boot/dts/renesas/r8a77980-condor.dts
>> +++ renesas/arch/arm64/boot/dts/renesas/r8a77980-condor.dts
>> @@ -59,6 +59,8 @@
>> phy0: ethernet-phy@0 {
>> rxc-skew-ps = <1500>;
>> reg = <0>;
>> + interrupt-parent = <&gpio1>;
>> + interrupts = <17 IRQ_TYPE_LEVEL_LOW>;
>
> I don't see this documented. Perhaps I'm missing something obvious.
Have you looked into the V3H PFC section for where in the GPSRs AVB_PHY_INT
is mapped?
The Condor schematics doesn't explicitly list the GPIO for AVB_PHY_INT
because that signal is meant to be routed thru the MAC. Unfortunately, the
sh_eth/ravb drivers don't support the PHY interrupt (the phylib function,
phy_mac_interrupt() reporting the PHY interrupts routed thru MAC is clearly
inadequate as it wants the link state as an argument), so we have to resort
to the GPIO interrupts...
> Or you have some extra information or newer documentation?
No.
> Also, given Olof Johansson's recent comments in ("Re: [GIT PULL] Renesas
> ARM64 Based SoC DT Updates for v4.18") please consider squashing this patch
> and the following one.
Hm... note that the different Ether cores are involved in these 2 PHY IRQ
patches. If that's OK, I can merge the patches...
[...]
MBR< Sergei
^ permalink raw reply
* Re: [PATCH v5 1/2] regulator: dt-bindings: add QCOM RPMh regulator bindings
From: Rob Herring @ 2018-06-04 14:56 UTC (permalink / raw)
To: David Collins
Cc: broonie, lgirdwood, mark.rutland, linux-arm-msm, linux-arm-kernel,
devicetree, linux-kernel, rnayak, sboyd, dianders
In-Reply-To: <af7152133afd7bbf9ad207905ec4e8d5cbe4e718.1527901471.git.collinsd@codeaurora.org>
On Fri, Jun 01, 2018 at 06:34:05PM -0700, David Collins wrote:
> Introduce bindings for RPMh regulator devices found on some
> Qualcomm Technlogies, Inc. SoCs. These devices allow a given
> processor within the SoC to make PMIC regulator requests which
> are aggregated within the RPMh hardware block along with requests
> from other processors in the SoC to determine the final PMIC
> regulator hardware state.
>
> Signed-off-by: David Collins <collinsd@codeaurora.org>
> ---
> .../bindings/regulator/qcom,rpmh-regulator.txt | 160 +++++++++++++++++++++
> .../dt-bindings/regulator/qcom,rpmh-regulator.h | 36 +++++
> 2 files changed, 196 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.txt
> create mode 100644 include/dt-bindings/regulator/qcom,rpmh-regulator.h
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* Re: [PATCH v5 4/4] ARM: dts: imx: add missing compatible and clock properties for EPIT
From: kbuild test robot @ 2018-06-04 14:57 UTC (permalink / raw)
To: Clément Péron
Cc: kbuild-all, Colin Didier, linux-arm-kernel, devicetree,
linux-kernel, Daniel Lezcano, Thomas Gleixner, Fabio Estevam,
Vladimir Zapolskiy, Sascha Hauer, Rob Herring, NXP Linux Team,
Pengutronix Kernel Team, Clément Peron
In-Reply-To: <20180604100035.19558-5-peron.clem@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1098 bytes --]
Hi Colin,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on shawnguo/for-next]
[also build test ERROR on v4.17 next-20180601]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Cl-ment-P-ron/Reintroduce-i-MX-EPIT-Timer/20180604-211036
base: https://git.kernel.org/pub/scm/linux/kernel/git/shawnguo/linux.git for-next
config: arm-realview_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm
All errors (new ones prefixed by >>):
>> Error: arch/arm/boot/dts/imx6sl.dtsi:661.19-20 syntax error
FATAL ERROR: Unable to parse input tree
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 18007 bytes --]
^ permalink raw reply
* Re: [PATCH 1/2] thermal: tsens: Add support for SDM845 platform
From: Bjorn Andersson @ 2018-06-04 15:03 UTC (permalink / raw)
To: Amit Kucheria
Cc: linux-arm-msm, rnayak, edubezval, Zhang Rui, Rob Herring,
Mark Rutland, open list:THERMAL,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list
In-Reply-To: <7a0f33cff94f41110bdbff14d28d36074fae2a7d.1527937396.git.amit.kucheria@linaro.org>
On Sat 02 Jun 04:11 PDT 2018, Amit Kucheria wrote:
> diff --git a/drivers/thermal/qcom/tsens-sdm845.c b/drivers/thermal/qcom/tsens-sdm845.c
[..]
> +#define TRDY_OFFSET 0xe4
> +#define TRDY_READY_BIT BIT(1)
This is bit 0.
> +
> +#define STATUS_OFFSET 0xa0
> +#define LAST_TEMP_MASK 0xfff
> +#define STATUS_VALID_BIT BIT(21)
> +#define CODE_SIGN_BIT BIT(11)
> +
> +static int get_temp_sdm845(struct tsens_device *tmdev, int id, int *temp)
> +{
> + struct tsens_sensor *s = &tmdev->sensor[id];
> + u32 code;
> + unsigned int sensor_addr;
> + int last_temp = 0, last_temp2 = 0, last_temp3 = 0, ret;
> +
> + ret = regmap_read(tmdev->map, TRDY_OFFSET, &code);
> + if (ret)
> + return ret;
> + if (code & TRDY_READY_BIT)
> + return -ENODATA;
This section is the only difference from 8996, but this register is
identical to 8996 and 8998. So I think you should add this to
tsens-8996.c and we can use that for 8996, 8998 and sdm845.
Perhaps we should name it tsens-v2, as that seems to be the common
denominator for these, according to the documentation.
Regards,
Bjorn
^ permalink raw reply
* Re: [PATCH 06/15] drm/sun4i: tcon: Add support for tcon-top
From: Jernej Škrabec @ 2018-06-04 15:09 UTC (permalink / raw)
To: Maxime Ripard
Cc: Chen-Yu Tsai, Rob Herring, Mark Rutland, dri-devel, devicetree,
linux-arm-kernel, linux-kernel, linux-clk, linux-sunxi
In-Reply-To: <20180604115034.kuy35s4ajewapk4m@flea>
Dne ponedeljek, 04. junij 2018 ob 13:50:34 CEST je Maxime Ripard napisal(a):
> On Fri, Jun 01, 2018 at 09:19:43AM -0700, Chen-Yu Tsai wrote:
> > On Fri, Jun 1, 2018 at 8:29 AM, Maxime Ripard <maxime.ripard-LDxbnhwyfcKIwRZHo2/mJg@public.gmane.orgm>
wrote:
> > > On Thu, May 31, 2018 at 07:54:08PM +0200, Jernej Škrabec wrote:
> > >> Dne četrtek, 31. maj 2018 ob 11:21:33 CEST je Maxime Ripard napisal(a):
> > >> > On Thu, May 24, 2018 at 03:01:09PM -0700, Chen-Yu Tsai wrote:
> > >> > > >> > > + if (tcon->quirks->needs_tcon_top) {
> > >> > > >> > > + struct device_node *np;
> > >> > > >> > > +
> > >> > > >> > > + np = of_parse_phandle(dev->of_node,
> > >> > > >> > > "allwinner,tcon-top",
> > >> > > >> > > 0);
> > >> > > >> > > + if (np) {
> > >> > > >> > > + struct platform_device *pdev;
> > >> > > >> > > +
> > >> > > >> > > + pdev = of_find_device_by_node(np);
> > >> > > >> > > + if (pdev)
> > >> > > >> > > + tcon->tcon_top =
> > >> > > >> > > platform_get_drvdata(pdev);
> > >> > > >> > > + of_node_put(np);
> > >> > > >> > > +
> > >> > > >> > > + if (!tcon->tcon_top)
> > >> > > >> > > + return -EPROBE_DEFER;
> > >> > > >> > > + }
> > >> > > >> > > + }
> > >> > > >> > > +
> > >> > > >> >
> > >> > > >> > I might have missed it, but I've not seen the bindings
> > >> > > >> > additions for
> > >> > > >> > that property. This shouldn't really be done that way anyway,
> > >> > > >> > instead
> > >> > > >> > of using a direct phandle, you should be using the of-graph,
> > >> > > >> > with the
> > >> > > >> > TCON-top sitting where it belongs in the flow of data.
> > >> > > >>
> > >> > > >> Just to answer to the first question, it did describe it in
> > >> > > >> "[PATCH
> > >> > > >> 07/15] dt- bindings: display: sun4i-drm: Add R40 HDMI pipeline".
> > >> > > >>
> > >> > > >> As why I designed it that way - HW representation could be
> > >> > > >> described
> > >> > > >> that way> >>
> > >> > > >>
> > >> > > >> (ASCII art makes sense when fixed width font is used to view
it):
> > >> > > >> / LCD0/LVDS0
> > >> > > >>
> > >> > > >> / TCON-LCD0
> > >> > > >>
> > >> > > >> | \ MIPI DSI
> > >> > > >>
> > >> > > >> mixer0 |
> > >> > > >>
> > >> > > >> \ / TCON-LCD1 - LCD1/LVDS1
> > >> > > >>
> > >> > > >> TCON-TOP
> > >> > > >>
> > >> > > >> / \ TCON-TV0 - TVE0/RGB
> > >> > > >>
> > >> > > >> mixer1 | \
> > >> > > >>
> > >> > > >> | TCON-TOP - HDMI
> > >> > > >> |
> > >> > > >> | /
> > >> > > >>
> > >> > > >> \ TCON-TV1 - TVE1/RGB
> > >> > > >>
> > >> > > >> This is a bit simplified, since there is also TVE-TOP, which is
> > >> > > >> responsible
> > >> > > >> for sharing 4 DACs between both TVE encoders. You can have two
> > >> > > >> TV outs
> > >> > > >> (PAL/ NTSC) or TVE0 as TV out and TVE1 as RGB or vice versa. It
> > >> > > >> even
> > >> > > >> seems that you can arbitrarly choose which DAC is responsible
> > >> > > >> for
> > >> > > >> which signal, so there is a ton of possible end combinations,
> > >> > > >> but I'm
> > >> > > >> not 100% sure.
> > >> > > >>
> > >> > > >> Even though I wrote TCON-TOP twice, this is same unit in HW. R40
> > >> > > >> manual
> > >> > > >> suggest more possibilities, although some of them seem wrong,
> > >> > > >> like RGB
> > >> > > >> feeding from LCD TCON. That is confirmed to be wrong when
> > >> > > >> checking BSP
> > >> > > >> code.
> > >> > > >>
> > >> > > >> Additionally, TCON-TOP comes in the middle of TVE0 and LCD0,
> > >> > > >> TVE1 and
> > >> > > >> LCD1 for pin muxing, although I'm not sure why is that needed at
> > >> > > >> all,
> > >> > > >> since according to R40 datasheet, TVE0 and TVE1 pins are
> > >> > > >> dedicated and
> > >> > > >> not on PORT D and PORT H, respectively, as TCON-TOP
> > >> > > >> documentation
> > >> > > >> suggest. However, HSYNC and PSYNC lines might be shared between
> > >> > > >> TVE
> > >> > > >> (when it works in RGB mode) and LCD. But that is just my guess
> > >> > > >> since
> > >> > > >> I'm not really familiar with RGB and LCD interfaces.
> > >> > > >>
> > >> > > >> I'm really not sure what would be the best representation in
> > >> > > >> OF-graph.
> > >> > > >> Can you suggest one?
> > >> > > >
> > >> > > > Rob might disagree on this one, but I don't see anything wrong
> > >> > > > with
> > >> > > > having loops in the graph. If the TCON-TOP can be both the input
> > >> > > > and
> > >> > > > output of the TCONs, then so be it, and have it described that
> > >> > > > way in
> > >> > > > the graph.
> > >> > > >
> > >> > > > The code is already able to filter out nodes that have already
> > >> > > > been
> > >> > > > added to the list of devices we need to wait for in the component
> > >> > > > framework, so that should work as well.
> > >> > > >
> > >> > > > And we'd need to describe TVE-TOP as well, even though we don't
> > >> > > > have a
> > >> > > > driver for it yet. That will simplify the backward compatibility
> > >> > > > later
> > >> > > > on.
> > >> > >
> > >> > > I'm getting the feeling that TCON-TOP / TVE-TOP is the glue layer
> > >> > > that
> > >> > > binds everything together, and provides signal routing, kind of
> > >> > > like
> > >> > > DE-TOP on A64. So the signal mux controls that were originally
> > >> > > found
> > >> > > in TCON0 and TVE0 were moved out.
> > >> > >
> > >> > > The driver needs to know about that, but the graph about doesn't
> > >> > > make
> > >> > > much sense directly. Without looking at the manual, I understand it
> > >> > > to
> > >> > > likely be one mux between the mixers and TCONs, and one between the
> > >> > > TCON-TVs and HDMI. Would it make more sense to just have the graph
> > >> > > connections between the muxed components, and remove TCON-TOP from
> > >> > > it, like we had in the past? A phandle could be used to reference
> > >> > > the TCON-TOP for mux controls, in addition to the clocks and
> > >> > > resets.
> > >> > >
> > >> > > For TVE, we would need something to represent each of the output
> > >> > > pins,
> > >> > > so the device tree can actually describe what kind of signal, be it
> > >> > > each component of RGB/YUV or composite video, is wanted on each
> > >> > > pin,
> > >> > > if any. This is also needed on the A20 for the Cubietruck, so we
> > >> > > can
> > >> > > describe which pins are tied to the VGA connector, and which one
> > >> > > does
> > >> > > R, G, or B.
> > >> >
> > >> > I guess we'll see how the DT maintainers feel about this, but my
> > >> > impression is that the OF graph should model the flow of data between
> > >> > the devices. If there's a mux somewhere, then the data is definitely
> > >> > going through it, and as such it should be part of the graph.
> > >>
> > >> I concur, but I spent few days thinking how to represent this sanely in
> > >> graph, but I didn't find any good solution. I'll represent here my
> > >> idea and please tell your opinion before I start implementing it.
> > >>
> > >> First, let me be clear that mixer0 and mixer1 don't have same
> > >> capabilities
> > >> (different number of planes, mixer0 supports writeback, mixer1 does
> > >> not,
> > >> etc.). Thus, it does matter which mixer is connected to which
> > >> TCON/encoder.
> > >> mixer0 is meant to be connected to main display and mixer1 to
> > >> auxiliary. That obviously depends on end system.
> > >>
> > >> So, TCON TOP has 3 muxes, which have to be represented in graph. Two of
> > >> them are for mixer/TCON relationship (each of them 1 input and 4
> > >> possible outputs) and one for TV TCON/HDMI pair selection (2 possible
> > >> inputs, 1 output).
> > >>
> > >> According to current practice in sun4i-drm driver, graph has to have
> > >> port 0, representing input and port 1, representing output. This would
> > >> mean that graph looks something like that:
> > >>
> > >> tcon_top: tcon-top@1c70000 {
> > >>
> > >> compatible = "allwinner,sun8i-r40-tcon-top";
> > >> ...
> > >> ports {
> > >>
> > >> #address-cells = <1>;
> > >> #size-cells = <0>;
> > >>
> > >> tcon_top_in: port@0 {
> > >>
> > >> #address-cells = <1>;
> > >> #size-cells = <0>;
> > >> reg = <0>;
> > >>
> > >> tcon_top_in_mixer0: endpoint@0 {
> > >>
> > >> reg = <0>;
> > >> remote-endpoint = <&mixer0_out_tcon_top>;
> > >>
> > >> };
> > >>
> > >> tcon_top_in_mixer1: endpoint@1 {
> > >>
> > >> reg = <1>;
> > >> remote-endpoint = <&mixer1_out_tcon_top>;
> > >>
> > >> };
> > >>
> > >> tcon_top_in_tcon_tv: endpoint@2 {
> > >>
> > >> reg = <2>;
> > >> // here is HDMI input connection, part of
> > >> board DTS
> > >> remote-endpoint = <board specific phandle
> > >> to TV TCON output>;
> > >>
> > >> };
> > >>
> > >> };
> > >>
> > >> tcon_top_out: port@1 {
> > >>
> > >> #address-cells = <1>;
> > >> #size-cells = <0>;
> > >> reg = <1>;
> > >>
> > >> tcon_top_out_tcon0: endpoint@0 {
> > >>
> > >> reg = <0>;
> > >> // here is mixer0 output connection, part
> > >> of board DTS
> > >> remote-endpoint = <board specific phandle
> > >> to TCON>;
> > >>
> > >> };
> > >>
> > >> tcon_top_out_tcon1: endpoint@1 {
> > >>
> > >> reg = <1>;
> > >> // here is mixer1 output connection, part
> > >> of board DTS
> > >> remote-endpoint = <board specific phandle
> > >> to TCON>;
> > >>
> > >> };
> > >>
> > >> tcon_top_out_hdmi: endpoint@2 {
> > >>
> > >> reg = <2>;
> > >> remote-endpoint = <&hdmi_in_tcon_top>;
> > >>
> > >> };
> > >>
> > >> };
> > >>
> > >> };
> > >>
> > >> };
> > >
> > > IIRC, each port is supposed to be one route for the data, so we would
> > > have multiple ports, one for the mixers in input and for the tcon in
> > > output, and one for the TCON in input and for the HDMI/TV in
> > > output. Rob might correct me here.
Ok, that seems more clean approach. I'll have to extend graph traversing
algorithm in sun4i_drv.c, but that's no problem.
Just to be clear, you have in mind 3 pairs of ports (0/1 for mixer0 mux, 2/3
for mixer1 and 4/5 for HDMI input), right? That way each mux is represented
with one pair of ports, even numbered for input and odd numbered for output.
> > >
> > >> tcon_tv0: lcd-controller@1c73000 {
> > >>
> > >> compatible = "allwinner,sun8i-r40-tcon-tv-0";
> > >> ...
> > >> ports {
> > >>
> > >> #address-cells = <1>;
> > >> #size-cells = <0>;
> > >>
> > >> tcon_tv0_in: port@0 {
> > >>
> > >> reg = <0>;
> > >>
> > >> tcon_tv0_in_tcon_top: endpoint {
> > >>
> > >> // endpoint depends on board, part of
> > >> board DTS
> > >> remote-endpoint = <phandle to one of
> > >> tcon_top_out_tcon>;
> > >
> > > Just curious, what would be there?
Either phandle to tcon_top_out_tcon0 or tcon_top_out_tcon1.
> > >
> > >> };
> > >>
> > >> };
> > >>
> > >> tcon_tv0_out: port@1 {
> > >>
> > >> #address-cells = <1>;
> > >> #size-cells = <0>;
> > >> reg = <1>;
> > >>
> > >> // endpoints to TV TOP and TCON TOP HDMI input
> > >> ...
> > >>
> > >> };
> > >>
> > >> };
> > >>
> > >> };
> > >>
> > >> tcon_tv1: lcd-controller@1c74000 {
> > >>
> > >> compatible = "allwinner,sun8i-r40-tcon-tv-1";
> > >> ...
> > >> ports {
> > >>
> > >> #address-cells = <1>;
> > >> #size-cells = <0>;
> > >>
> > >> tcon_tv1_in: port@0 {
> > >>
> > >> reg = <0>;
> > >>
> > >> tcon_tv1_in_tcon_top: endpoint {
> > >>
> > >> // endpoint depends on board, part of
> > >> board DTS
> > >> remote-endpoint = <phandle to one of
> > >> tcon_top_out_tcon>;
> > >>
> > >> };
> > >>
> > >> };
> > >>
> > >> tcon_tv1_out: port@1 {
> > >>
> > >> #address-cells = <1>;
> > >> #size-cells = <0>;
> > >> reg = <1>;
> > >>
> > >> // endpoints to TV TOP and TCON TOP HDMI input
> > >> ...
> > >>
> > >> };
> > >>
> > >> };
> > >>
> > >> };
> > >>
> > >> tcon_lcd0 and tcon_lcd1 would have similar connections, except that for
> > >> outputs would be LCD or LVDS panels or MIPI DSI encoder.
> > >>
> > >> Please note that each TCON (there are 4 of them) would need to have
> > >> unique
> > >> compatible and have HW index stored in quirks data. It would be used by
> > >> TCON TOP driver for configuring muxes.
> > >
> > > Can't we use the port/endpoint ID instead? If the mux is the only
> > > thing that changes, the compatible has no reason to. It's the same IP,
> > > and the only thing that changes is something that is not part of that
> > > IP.
> >
> > I agree. Endpoint IDs should provide that information. I'm still not
> > sure How to encode multiple in/out mux groups in a device node though.
>
> I guess we can do that through different ports?
Ok, I'll try to do something with "reg" property.
Best regards,
Jernej
--
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.
^ permalink raw reply
* Re: [PATCHv9 1/3] ARM:dt-bindings:display Intel FPGA Video and Image Processing Suite
From: Rob Herring @ 2018-06-04 15:13 UTC (permalink / raw)
To: Hean-Loong, Ong
Cc: Dinh Nguyen, Daniel Vetter, Laurent Pinchart, Randy Dunlap,
devicetree, linux-kernel, linux-arm-kernel, dri-devel, Ong
In-Reply-To: <1528094404-3542-2-git-send-email-hean.loong.ong@intel.com>
On Mon, Jun 04, 2018 at 02:40:02PM +0800, Hean-Loong, Ong wrote:
> From: Ong, Hean Loong <hean.loong.ong@intel.com>
>
> Device tree binding for Intel FPGA Video and Image Processing Suite. The binding involved would be generated from the Altera (Intel) Qsys system. The bindings would set the max width, max height, buts per pixel and memory port width. The device tree binding only supports the Intel
> Arria10 devkit and its variants. Vendor name retained as altr.
You need to wrap long lines.
>
> V8:
> *Add port to Display port decoder
>
> V7:
> *Fix OF graph for better description
> *Add description for encoder
>
> V6:
> *Description have not describe DT device in general
>
> V5:
> *remove bindings for bits per symbol as it has only one value which is 8
>
> V4:
> *fix properties that does not describe the values
>
> V3:
> *OF graph not in accordance to graph.txt
>
> V2:
> *Remove Linux driver description
>
> V1:
> *Missing vendor prefix
>
> Signed-off-by: Ong, Hean Loong <hean.loong.ong@intel.com>
> ---
> .../devicetree/bindings/display/altr,vip-fb2.txt | 99 ++++++++++++++++++++
> 1 files changed, 99 insertions(+), 0 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/display/altr,vip-fb2.txt
>
> diff --git a/Documentation/devicetree/bindings/display/altr,vip-fb2.txt b/Documentation/devicetree/bindings/display/altr,vip-fb2.txt
> new file mode 100644
> index 0000000..4092804
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/altr,vip-fb2.txt
> @@ -0,0 +1,99 @@
> +Intel Video and Image Processing(VIP) Frame Buffer II bindings
> +
> +Supported hardware: Intel FPGA SoC Arria10 and above with display port IP
> +
> +The Video Frame Buffer II in Video Image Processing (VIP) suite is an IP core
> +that interfaces between system memory and Avalon-ST video ports. The IP core
> +can be configured to support the memory reader (from memory to Avalon-ST)
> +and/or memory writer (from Avalon-ST to memory) interfaces.
> +
> +More information the FPGA video IP component can be acquired from
> +https://www.altera.com/content/dam/altera-www/global/en_US/pdfs\
> +/literature/ug/ug_vip.pdf
But URLs you don't need to wrap.
> +
> +DT-Bindings:
> +=============
> +Required properties:
> +----------------------------
> +- compatible: "altr,vip-frame-buffer-2.0"
> +- reg: Physical base address and length of the framebuffer controller's
> + registers.
> +- altr,max-width: The maximum width of the framebuffer in pixels.
> +- altr,max-height: The maximum height of the framebuffer in pixels.
> +- altr,mem-port-width = the bus width of the avalon master port
> + on the frame reader
> +
> +Optional sub-nodes:
> +- ports: The connection to the encoder
> +
> +Optional properties
These are not optional properties because this is a whole other node.
Group things by node (perhaps even make this 2 docments) and within each
node you describe required and optional properties.
> +----------------------------
> +- compatible: "altr, display-port"
spurious space ^
This needs to be more specific. Is there an IP version?
This is a DisplayPort encoder?
> +- reg: Physical base address and length of the display port controller's
> + registers
> +- clocks: required clock handles for specified pairs in clock name
> +- clock-names: required clock names. Contains:
> + - aux_clk: auxiliary clock,
> + - clk: 100 MHz output clock
But 'clocks' are input clocks.
Needs a better name than 'clk'.
> + - xcvr_mgmt_clk: transceiver management clock
'_clk' is redundant.
> +
> +Optional sub-nodes:
> +- ports: The connection to the controller
> +
> +Connections between the Frame Buffer II and other video IP cores in the system
> +are modelled using the OF graph DT bindings. The Frame Buffer II node has up
s/modelled/modeled/
> +to two OF graph ports. When the memory writer interface is enabled, port 0
> +maps to the Avalon-ST Input (din) port. When the memory reader interface is
> +enabled, port 1 maps to the Avalon-ST Output (dout) port.
> +
> +The encoder is built into the FPGA HW design and therefore would not
> +be accessible from the DDR.
> +
> + Port 0 Port1
> +---------------------------------------------------------
> +ARRIA10 AVALON_ST (DIN) AVALON_ST (DOUT)
> +
> +Required Properties Example:
> +----------------------------
> +
> +framebuffer@100000280 {
> + compatible = "altr,vip-frame-buffer-2.0";
> + reg = <0x00000001 0x00000280 0x00000040>;
> + altr,max-width = <1280>;
> + altr,max-height = <720>;
> + altr,mem-port-width = <128>;
Doesn't this block require some clocks too?
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@1 {
> + reg = <1>;
> + fb_output: endpoint {
> + remote-endpoint = <&dp_encoder_input>;
> + };
> + };
> + };
> +};
> +
> +Optional Properties Example:
> +This is not required unless there are needs to customize
> +Display Port controller settings.
> +
> +displayport@100002000 {
> + compatible = "altr, display-port";
> + reg = <0x00000001 0x00002000 0x00000800>;
> + clocks = <&dp_0_clk_16 &dp_0_clk_100 &dp_0_clk_100>;
> + clock-names = "aux_clk", "clk", "xcvr_mgmt_clk";
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <1>;
> + dp_input: endpoint {
> + remote-endpoint = <&dp_controller_input>;
> + };
> + };
> +};
> --
> 1.7.1
>
^ permalink raw reply
* Re: [PATCH] dt-bindings: net: ravb: Add support for r8a77990 SoC
From: Rob Herring @ 2018-06-04 15:32 UTC (permalink / raw)
To: Sergei Shtylyov, Shimoda, Yoshihiro
Cc: Simon Horman, David Miller, netdev,
open list:MEDIA DRIVERS FOR RENESAS - FCP, Mark Rutland,
devicetree
In-Reply-To: <19e43b97-9235-a752-7cf9-56a5e3bb281a@cogentembedded.com>
On Tue, May 15, 2018 at 3:43 AM, Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
> On 5/13/2018 10:58 AM, Simon Horman wrote:
>
>>>> Add documentation for r8a77990 compatible string to renesas ravb device
>>>> tree bindings documentation.
>>>>
>>>> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
>>>
>>>
>>> I'm assuming this isn't targetted at one of my trees. Just FYI.
>>
>>
>> Hi Dave,
>>
>> I think this is appropriate for net-next but if not I can take it.
>
>
> There's also Rob, and IIRC he has taken alike patch recently...
Applied.
Rob
^ permalink raw reply
* Re: [PATCH v3] of: platform: stop accessing invalid dev in of_platform_device_destroy
From: Rob Herring @ 2018-06-04 15:46 UTC (permalink / raw)
To: Srinivas Kandagatla
Cc: frowand.list, linux-arm-msm, bgoswami, devicetree, linux-kernel,
linux-arm-kernel, rohkumar
In-Reply-To: <20180604141408.3179-1-srinivas.kandagatla@linaro.org>
On Mon, Jun 04, 2018 at 03:14:08PM +0100, Srinivas Kandagatla wrote:
> Immediately after the platform_device_unregister() the device will be
> cleaned up. Accessing the freed pointer immediately after that will
> crash the system.
>
> Found this bug when kernel is built with CONFIG_PAGE_POISONING and testing
> loading/unloading audio drivers in a loop on Qcom platforms.
>
> Fix this by moving of_node_clear_flag() just before the unregister calls.
>
> Below is the crash trace:
>
> Unable to handle kernel paging request at virtual address 6b6b6b6b6b6c03
> Mem abort info:
> ESR = 0x96000021
> Exception class = DABT (current EL), IL = 32 bits
> SET = 0, FnV = 0
> EA = 0, S1PTW = 0
> Data abort info:
> ISV = 0, ISS = 0x00000021
> CM = 0, WnR = 0
> [006b6b6b6b6b6c03] address between user and kernel address ranges
> Internal error: Oops: 96000021 [#1] PREEMPT SMP
> Modules linked in:
> CPU: 2 PID: 1784 Comm: sh Tainted: G W 4.17.0-rc7-02230-ge3a63a7ef641-dirty #204
> Hardware name: Qualcomm Technologies, Inc. APQ 8016 SBC (DT)
> pstate: 80000005 (Nzcv daif -PAN -UAO)
> pc : clear_bit+0x18/0x2c
> lr : of_platform_device_destroy+0x64/0xb8
> sp : ffff00000c9c3930
> x29: ffff00000c9c3930 x28: ffff80003d39b200
> x27: ffff000008bb1000 x26: 0000000000000040
> x25: 0000000000000124 x24: ffff80003a9a3080
> x23: 0000000000000060 x22: ffff00000939f518
> x21: ffff80003aa79e98 x20: ffff80003aa3dae0
> x19: ffff80003aa3c890 x18: ffff800009feb794
> x17: 0000000000000000 x16: 0000000000000000
> x15: ffff800009feb790 x14: 0000000000000000
> x13: ffff80003a058778 x12: ffff80003a058728
> x11: ffff80003a058750 x10: 0000000000000000
> x9 : 0000000000000006 x8 : ffff80003a825988
> x7 : bbbbbbbbbbbbbbbb x6 : 0000000000000001
> x5 : 0000000000000000 x4 : 0000000000000001
> x3 : 0000000000000008 x2 : 0000000000000001
> x1 : 6b6b6b6b6b6b6c03 x0 : 0000000000000000
> Process sh (pid: 1784, stack limit = 0x (ptrval))
> Call trace:
> clear_bit+0x18/0x2c
> q6afe_remove+0x20/0x38
> apr_device_remove+0x30/0x70
> device_release_driver_internal+0x170/0x208
> device_release_driver+0x14/0x20
> bus_remove_device+0xcc/0x150
> device_del+0x10c/0x310
> device_unregister+0x1c/0x70
> apr_remove_device+0xc/0x18
> device_for_each_child+0x50/0x80
> apr_remove+0x18/0x20
> rpmsg_dev_remove+0x38/0x68
> device_release_driver_internal+0x170/0x208
> device_release_driver+0x14/0x20
> bus_remove_device+0xcc/0x150
> device_del+0x10c/0x310
> device_unregister+0x1c/0x70
> qcom_smd_remove_device+0xc/0x18
> device_for_each_child+0x50/0x80
> qcom_smd_unregister_edge+0x3c/0x70
> smd_subdev_remove+0x18/0x28
> rproc_stop+0x48/0xd8
> rproc_shutdown+0x60/0xe8
> state_store+0xbc/0xf8
> dev_attr_store+0x18/0x28
> sysfs_kf_write+0x3c/0x50
> kernfs_fop_write+0x118/0x1e0
> __vfs_write+0x18/0x110
> vfs_write+0xa4/0x1a8
> ksys_write+0x48/0xb0
> sys_write+0xc/0x18
> el0_svc_naked+0x30/0x34
> Code: d2800022 8b400c21 f9800031 9ac32043 (c85f7c22)
> ---[ end trace 32020935775616a2 ]---
>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
> Changes since v2:
> Move the calls to of_node_clear_flag just before unregister,
> suggested by Rob.
>
> drivers/of/platform.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
Applied, thanks.
Rob
^ permalink raw reply
* RE: [RESEND PATCH 2/4] PCI: cadence: Add generic PHY support to host and EP drivers
From: Alan Douglas @ 2018-06-04 16:13 UTC (permalink / raw)
To: Kishon Vijay Abraham I, bhelgaas@google.com,
lorenzo.pieralisi@arm.com, linux-pci@vger.kernel.org
Cc: devicetree@vger.kernel.org, cyrille.pitchen@free-electrons.com
In-Reply-To: <d701937d-f33b-d801-a803-56ac76a7119b@ti.com>
On 31 May 2018 14:26, Kishon Vijay Abraham I wrote:
> On Tuesday 22 May 2018 06:57 PM, Alan Douglas wrote:
> > If PHYs are present, they will be initialized and enabled in driver probe,
> > and disabled in driver shutdown.
> >
> > Signed-off-by: Alan Douglas <adouglas@cadence.com>
> > ---
> > drivers/pci/cadence/pcie-cadence-ep.c | 14 ++++-
> > drivers/pci/cadence/pcie-cadence-host.c | 31 +++++++++++
> > drivers/pci/cadence/pcie-cadence.c | 93 +++++++++++++++++++++++++++++++++
> > drivers/pci/cadence/pcie-cadence.h | 7 +++
> > 4 files changed, 144 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/cadence/pcie-cadence-ep.c b/drivers/pci/cadence/pcie-cadence-ep.c
> > index 3d8283e..2581caf 100644
> > --- a/drivers/pci/cadence/pcie-cadence-ep.c
> > +++ b/drivers/pci/cadence/pcie-cadence-ep.c
> > @@ -439,6 +439,7 @@ static int cdns_pcie_ep_probe(struct platform_device *pdev)
> > struct pci_epc *epc;
> > struct resource *res;
> > int ret;
> > + int phy_count;
> >
> > ep = devm_kzalloc(dev, sizeof(*ep), GFP_KERNEL);
> > if (!ep)
> > @@ -472,6 +473,12 @@ static int cdns_pcie_ep_probe(struct platform_device *pdev)
> > if (!ep->ob_addr)
> > return -ENOMEM;
> >
> > + ret = cdns_pcie_init_phy(dev, pcie);
> > + if (ret) {
> > + dev_err(dev, "failed to init phy\n");
> > + return ret;
> > + }
> > + platform_set_drvdata(pdev, pcie);
> > pm_runtime_enable(dev);
> > ret = pm_runtime_get_sync(dev);
> > if (ret < 0) {
> > @@ -520,6 +527,10 @@ static int cdns_pcie_ep_probe(struct platform_device *pdev)
> >
> > err_get_sync:
> > pm_runtime_disable(dev);
> > + cdns_pcie_disable_phy(pcie);
> > + phy_count = pcie->phy_count;
> > + while (phy_count--)
> > + device_link_del(pcie->link[phy_count]);
> >
> > return ret;
> > }
> > @@ -527,6 +538,7 @@ static int cdns_pcie_ep_probe(struct platform_device *pdev)
> > static void cdns_pcie_ep_shutdown(struct platform_device *pdev)
> > {
> > struct device *dev = &pdev->dev;
> > + struct cdns_pcie *pcie = dev_get_drvdata(dev);
> > int ret;
> >
> > ret = pm_runtime_put_sync(dev);
> > @@ -535,7 +547,7 @@ static void cdns_pcie_ep_shutdown(struct platform_device *pdev)
> >
> > pm_runtime_disable(dev);
> >
> > - /* The PCIe controller can't be disabled. */
> > + cdns_pcie_disable_phy(pcie);
> > }
> >
> > static struct platform_driver cdns_pcie_ep_driver = {
> > diff --git a/drivers/pci/cadence/pcie-cadence-host.c b/drivers/pci/cadence/pcie-cadence-host.c
> > index a4ebbd3..7536926a 100644
> > --- a/drivers/pci/cadence/pcie-cadence-host.c
> > +++ b/drivers/pci/cadence/pcie-cadence-host.c
> > @@ -58,6 +58,9 @@ static void __iomem *cdns_pci_map_bus(struct pci_bus *bus, unsigned int devfn,
> >
> > return pcie->reg_base + (where & 0xfff);
> > }
> > + /* Check that the link is up */
> > + if (!(cdns_pcie_readl(pcie, CDNS_PCIE_LM_BASE) & 0x1))
> > + return NULL;
> >
> > /* Update Output registers for AXI region 0. */
> > addr0 = CDNS_PCIE_AT_OB_REGION_PCI_ADDR0_NBITS(12) |
> > @@ -239,6 +242,7 @@ static int cdns_pcie_host_probe(struct platform_device *pdev)
> > struct cdns_pcie *pcie;
> > struct resource *res;
> > int ret;
> > + int phy_count;
> >
> > bridge = devm_pci_alloc_host_bridge(dev, sizeof(*rc));
> > if (!bridge)
> > @@ -290,6 +294,13 @@ static int cdns_pcie_host_probe(struct platform_device *pdev)
> > }
> > pcie->mem_res = res;
> >
> > + ret = cdns_pcie_init_phy(dev, pcie);
> > + if (ret) {
> > + dev_err(dev, "failed to init phy\n");
> > + return ret;
> > + }
> > + platform_set_drvdata(pdev, pcie);
> > +
> > pm_runtime_enable(dev);
> > ret = pm_runtime_get_sync(dev);
> > if (ret < 0) {
> > @@ -322,15 +333,35 @@ static int cdns_pcie_host_probe(struct platform_device *pdev)
> >
> > err_get_sync:
> > pm_runtime_disable(dev);
> > + cdns_pcie_disable_phy(pcie);
> > + phy_count = pcie->phy_count;
> > + while (phy_count--)
> > + device_link_del(pcie->link[phy_count]);
> >
> > return ret;
> > }
> >
> > +static void cdns_pcie_shutdown(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct cdns_pcie *pcie = dev_get_drvdata(dev);
> > + int ret;
> > +
> > + ret = pm_runtime_put_sync(dev);
> > + if (ret < 0)
> > + dev_dbg(dev, "pm_runtime_put_sync failed\n");
> > +
> > + pm_runtime_disable(dev);
> > + cdns_pcie_disable_phy(pcie);
> > +}
>
> shutdown callback can be added in a separate patch.
>
Will move to a separate patch in v3
> > +
> > +
> spurious blank line.
Will remove in v3
> > static struct platform_driver cdns_pcie_host_driver = {
> > .driver = {
> > .name = "cdns-pcie-host",
> > .of_match_table = cdns_pcie_host_of_match,
> > },
> > .probe = cdns_pcie_host_probe,
> > + .shutdown = cdns_pcie_shutdown,
> > };
> > builtin_platform_driver(cdns_pcie_host_driver);
> > diff --git a/drivers/pci/cadence/pcie-cadence.c b/drivers/pci/cadence/pcie-cadence.c
> > index 138d113..681609a 100644
> > --- a/drivers/pci/cadence/pcie-cadence.c
> > +++ b/drivers/pci/cadence/pcie-cadence.c
> > @@ -124,3 +124,96 @@ void cdns_pcie_reset_outbound_region(struct cdns_pcie *pcie, u32 r)
> > cdns_pcie_writel(pcie, CDNS_PCIE_AT_OB_REGION_CPU_ADDR0(r), 0);
> > cdns_pcie_writel(pcie, CDNS_PCIE_AT_OB_REGION_CPU_ADDR1(r), 0);
> > }
> > +
> > +void cdns_pcie_disable_phy(struct cdns_pcie *pcie)
> > +{
> > + int i = pcie->phy_count;
> > +
> > + while (i--) {
> > + phy_power_off(pcie->phy[i]);
> > + phy_exit(pcie->phy[i]);
> > + }
> > +}
> > +
> > +int cdns_pcie_enable_phy(struct cdns_pcie *pcie)
> > +{
> > + int ret;
> > + int i;
> > +
> > + for (i = 0; i < pcie->phy_count; i++) {
> > + ret = phy_init(pcie->phy[i]);
> > + if (ret < 0)
> > + goto err_phy;
> > +
> > + ret = phy_power_on(pcie->phy[i]);
> > + if (ret < 0) {
> > + phy_exit(pcie->phy[i]);
> > + goto err_phy;
> > + }
> > + }
> > +
> > + return 0;
> > +
> > +err_phy:
> > + while (--i >= 0) {
> > + phy_power_off(pcie->phy[i]);
> > + phy_exit(pcie->phy[i]);
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +int cdns_pcie_init_phy(struct device *dev, struct cdns_pcie *pcie)
> > +{
> > + struct device_node *np = dev->of_node;
> > + int phy_count;
> > + struct phy **phy;
> > + struct device_link **link;
> > + int i;
> > + int ret;
> > + const char *name;
> > +
> > + phy_count = of_property_count_strings(np, "phy-names");
>
> It's preferable to have num-lanes property, which can be used to configure the
> number of lanes supported in a platform even if it doesn't populate PHYs property.
There is no suitable register in the core IP to configure num-lanes, this is
configured externally to the IP, and so can be read from an IP register.
> > + if (phy_count < 1) {
> > + dev_err(dev, "no phy-names. PHY will not be initialized\n");
> > + pcie->phy_count = 0;
> > + return 0;
> > + }
> > +
> > + phy = devm_kzalloc(dev, sizeof(*phy) * phy_count, GFP_KERNEL);
> > + if (!phy)
> > + return -ENOMEM;
> > +
> > + link = devm_kzalloc(dev, sizeof(*link) * phy_count, GFP_KERNEL);
> > + if (!link)
> > + return -ENOMEM;
> > +
> > + for (i = 0; i < phy_count; i++) {
> > + of_property_read_string_index(np, "phy-names", i, &name);
> > + phy[i] = devm_phy_get(dev, name);
>
> For optional PHYs, devm_phy_optional_get() should be used.
>
I'll prepare a v3 using devm_phy_optional_get()
Thanks,
Alan
^ permalink raw reply
* Re: [PATCH v8 0/2] SDM845 System Cache Driver
From: rishabhb @ 2018-06-04 16:18 UTC (permalink / raw)
To: linux-arm-kernel, linux-arm-msm, devicetree
Cc: linux-kernel, linux-arm, tsoni, ckadabi, evgreen, robh,
andy.shevchenko
In-Reply-To: <1527122121-31452-1-git-send-email-rishabhb@codeaurora.org>
On 2018-05-23 17:35, Rishabh Bhatnagar wrote:
> This series implements system cache or LLCC(Last Level Cache
> Controller)
> driver for SDM845 SOC. The purpose of the driver is to partition the
> system cache and program the settings such as priortiy, lines to probe
> while doing a look up in the system cache, low power related settings
> etc.
> The partitions are called cache slices. Each cache slice is associated
> with size and SCID(System Cache ID). The driver also provides API for
> clients to query the cache slice details,activate and deactivate them.
>
> The driver can be broadly classified into:
> * SOC specific driver: llcc-sdm845.c: Cache partitioning and cache
> slice
> properties for usecases on sdm845 that need to use system cache.
>
> * API : llcc-slice.c: Exports APIs to clients to query cache slice
> details,
> activate and deactivate cache slices.
>
> Changes since v7:
> * Change the DT node name to cache-controller.
> * Use the module_platform_driver_macro
> * Use GENMASK and SZ_* macros
> * Correct indentation, and remove unnecessary assignemnts.
> * Addresed all comments by Andy Schevchenko except the comment to
> ignore some
> lines of code going over 80 characters.
>
> Changes since v6:
> * Remove the max-slices property from DT.
> * Make client's slice_ids as macros.
> * Unlock mutex while returning from function in case of error.
>
> Changes since v5:
> * Remove client information from DT.
> * Make the llcc driver data as global.
> * Check return value of llcc_update_act_ctrl function
> * Change error returned from -EFAULT to -EINVAL
>
> Changes since v4:
> * Remove null pointer checks as per comments.
> * Remove extra blank lines.
>
> Changes since v3:
> * Use the regmap_read_poll_timeout function
> * Check for regmap read/write errors.
> * Remove memory barrier after regmap write
> * Derive memory bank offsets using stride macro variable
> * Remove debug statements from code
> * Remove the qcom_llcc_remove function
> * Use if IS_ENABLED in place of ifdef for built-in module
> * Change EXPORT_SYMBOL to EXPORT_SYMBOL_GPL
> * Remove unnecessary free functions
> * Change the variable names as per review comments
>
> Changes since v2:
> * Corrected the Makefile to fix compilation.
>
> Changes since v1:
> * Added Makefile and Kconfig.
>
> Changes since v0:
> * Removed the syscon and simple-mfd approach
> * Updated the device tree nodes to mention LLCC as a single HW block
> * Moved llcc bank offsets from device tree and handled the offset
> in the driver.
>
> ckadabi@codeaurora.org (2):
> dt-bindings: Documentation for qcom, llcc
> drivers: soc: Add LLCC driver
>
> .../devicetree/bindings/arm/msm/qcom,llcc.txt | 26 ++
> drivers/soc/qcom/Kconfig | 17 ++
> drivers/soc/qcom/Makefile | 2 +
> drivers/soc/qcom/llcc-sdm845.c | 94 ++++++
> drivers/soc/qcom/llcc-slice.c | 335
> +++++++++++++++++++++
> include/linux/soc/qcom/llcc-qcom.h | 180 +++++++++++
> 6 files changed, 654 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt
> create mode 100644 drivers/soc/qcom/llcc-sdm845.c
> create mode 100644 drivers/soc/qcom/llcc-slice.c
> create mode 100644 include/linux/soc/qcom/llcc-qcom.h
Does this spin look fine to everyone? If yes can we go ahead and merge
this?
^ 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