* Re: [PATCH v5 06/11] soc: mediatek: add new flow for mtcmos power.
From: Matthias Brugger @ 2018-07-18 14:50 UTC (permalink / raw)
To: Mars Cheng, Rob Herring, Marc Zyngier, Ryder Lee, Stephen Boyd,
Sean Wang
Cc: CC Hwang, Loda Chou, linux-kernel, linux-mediatek, devicetree,
wsd_upstream, linux-serial, linux-arm-kernel, linux-clk,
Owen Chen
In-Reply-To: <1531817552-17221-7-git-send-email-mars.cheng@mediatek.com>
On 17/07/18 10:52, Mars Cheng wrote:
> From: Owen Chen <owen.chen@mediatek.com>
>
> MT6765 need multiple register and actions to setup bus
> protect.
> 1. turn on subsys CG before release bus protect to receive
> ack.
> 2. turn off subsys CG after set bus protect and receive
> ack.
> 3. bus protect need not only infracfg but other domain
> register to setup. Therefore we add a set/clr APIs
> with more customize arguments.
>
> Signed-off-by: Owen Chen <owen.chen@mediatek.com>
> Signed-off-by: Mars Cheng <mars.cheng@mediatek.com>
> ---
> drivers/soc/mediatek/Makefile | 2 +-
> drivers/soc/mediatek/mtk-infracfg.c | 178 +++++++++++---
> drivers/soc/mediatek/mtk-scpsys-ext.c | 405 +++++++++++++++++++++++++++++++
> drivers/soc/mediatek/mtk-scpsys.c | 147 +++++++++--
> include/linux/soc/mediatek/infracfg.h | 9 +-
> include/linux/soc/mediatek/scpsys-ext.h | 66 +++++
> 6 files changed, 745 insertions(+), 62 deletions(-)
> create mode 100644 drivers/soc/mediatek/mtk-scpsys-ext.c
> create mode 100644 include/linux/soc/mediatek/scpsys-ext.h
>
> diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile
> index 12998b0..9dc6670 100644
> --- a/drivers/soc/mediatek/Makefile
> +++ b/drivers/soc/mediatek/Makefile
> @@ -1,3 +1,3 @@
> -obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o
> +obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o mtk-scpsys-ext.o
> obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o
> obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys.o
> diff --git a/drivers/soc/mediatek/mtk-infracfg.c b/drivers/soc/mediatek/mtk-infracfg.c
> index 958861c..11eadf8 100644
> --- a/drivers/soc/mediatek/mtk-infracfg.c
> +++ b/drivers/soc/mediatek/mtk-infracfg.c
> @@ -1,3 +1,4 @@
> +// SPDX-License-Identifier: GPL-2.0
> /*
> * Copyright (c) 2015 Pengutronix, Sascha Hauer <kernel@pengutronix.de>
> *
> @@ -15,6 +16,7 @@
> #include <linux/jiffies.h>
> #include <linux/regmap.h>
> #include <linux/soc/mediatek/infracfg.h>
> +#include <linux/soc/mediatek/scpsys-ext.h>
> #include <asm/processor.h>
>
> #define MTK_POLL_DELAY_US 10
> @@ -26,62 +28,176 @@
> #define INFRA_TOPAXI_PROTECTEN_CLR 0x0264
>
> /**
> - * mtk_infracfg_set_bus_protection - enable bus protection
> - * @regmap: The infracfg regmap
> - * @mask: The mask containing the protection bits to be enabled.
> - * @reg_update: The boolean flag determines to set the protection bits
> - * by regmap_update_bits with enable register(PROTECTEN) or
> - * by regmap_write with set register(PROTECTEN_SET).
> + * mtk_generic_set_cmd - enable bus protection with set register
> + * @regmap: The bus protect regmap
> + * @set_ofs: The set register offset to set corresponding bit to 1.
> + * @sta_ofs: The status register offset to show bus protect enable/disable.
> + * @mask: The mask containing the protection bits to be disabled.
> *
> * This function enables the bus protection bits for disabled power
> * domains so that the system does not hang when some unit accesses the
> * bus while in power down.
> */
> -int mtk_infracfg_set_bus_protection(struct regmap *infracfg, u32 mask,
> - bool reg_update)
> +int mtk_generic_set_cmd(struct regmap *regmap, u32 set_ofs,
> + u32 sta_ofs, u32 mask)
> {
> u32 val;
> int ret;
>
> - if (reg_update)
> - regmap_update_bits(infracfg, INFRA_TOPAXI_PROTECTEN, mask,
> - mask);
> - else
> - regmap_write(infracfg, INFRA_TOPAXI_PROTECTEN_SET, mask);
> + regmap_write(regmap, set_ofs, mask);
>
> - ret = regmap_read_poll_timeout(infracfg, INFRA_TOPAXI_PROTECTSTA1,
> - val, (val & mask) == mask,
> - MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
> + ret = regmap_read_poll_timeout(regmap, sta_ofs, val,
> + (val & mask) == mask,
> + MTK_POLL_DELAY_US,
> + MTK_POLL_TIMEOUT);
>
> return ret;
> }
>
> /**
> - * mtk_infracfg_clear_bus_protection - disable bus protection
> - * @regmap: The infracfg regmap
> + * mtk_generic_clr_cmd - disable bus protection with clr register
> + * @regmap: The bus protect regmap
> + * @clr_ofs: The clr register offset to clear corresponding bit to 0.
> + * @sta_ofs: The status register offset to show bus protect enable/disable.
> * @mask: The mask containing the protection bits to be disabled.
> - * @reg_update: The boolean flag determines to clear the protection bits
> - * by regmap_update_bits with enable register(PROTECTEN) or
> - * by regmap_write with clear register(PROTECTEN_CLR).
> *
> * This function disables the bus protection bits previously enabled with
> - * mtk_infracfg_set_bus_protection.
> + * mtk_set_bus_protection.
> */
>
> -int mtk_infracfg_clear_bus_protection(struct regmap *infracfg, u32 mask,
> - bool reg_update)
> +int mtk_generic_clr_cmd(struct regmap *regmap, u32 clr_ofs,
> + u32 sta_ofs, u32 mask)
> {
> int ret;
> u32 val;
>
> - if (reg_update)
> - regmap_update_bits(infracfg, INFRA_TOPAXI_PROTECTEN, mask, 0);
> - else
> - regmap_write(infracfg, INFRA_TOPAXI_PROTECTEN_CLR, mask);
> + regmap_write(regmap, clr_ofs, mask);
>
> - ret = regmap_read_poll_timeout(infracfg, INFRA_TOPAXI_PROTECTSTA1,
> - val, !(val & mask),
> - MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
> + ret = regmap_read_poll_timeout(regmap, sta_ofs, val,
> + !(val & mask),
> + MTK_POLL_DELAY_US,
> + MTK_POLL_TIMEOUT);
> + return ret;
> +}
> +
> +/**
> + * mtk_generic_enable_cmd - enable bus protection with upd register
> + * @regmap: The bus protect regmap
> + * @upd_ofs: The update register offset to directly rewrite value to
> + * corresponding bit.
> + * @sta_ofs: The status register offset to show bus protect enable/disable.
> + * @mask: The mask containing the protection bits to be disabled.
> + *
> + * This function enables the bus protection bits for disabled power
> + * domains so that the system does not hang when some unit accesses the
> + * bus while in power down.
> + */
> +int mtk_generic_enable_cmd(struct regmap *regmap, u32 upd_ofs,
> + u32 sta_ofs, u32 mask)
> +{
> + u32 val;
> + int ret;
> +
> + regmap_update_bits(regmap, upd_ofs, mask, mask);
>
> + ret = regmap_read_poll_timeout(regmap, sta_ofs, val,
> + (val & mask) == mask,
> + MTK_POLL_DELAY_US,
> + MTK_POLL_TIMEOUT);
> return ret;
> }
> +
> +/**
> + * mtk_generic_disable_cmd - disable bus protection with updd register
> + * @regmap: The bus protect regmap
> + * @upd_ofs: The update register offset to directly rewrite value to
> + * corresponding bit.
> + * @sta_ofs: The status register offset to show bus protect enable/disable.
> + * @mask: The mask containing the protection bits to be disabled.
> + *
> + * This function disables the bus protection bits previously enabled with
> + * mtk_set_bus_protection.
> + */
> +
> +int mtk_generic_disable_cmd(struct regmap *regmap, u32 upd_ofs,
> + u32 sta_ofs, u32 mask)
> +{
> + int ret;
> + u32 val;
> +
> + regmap_update_bits(regmap, upd_ofs, mask, 0);
> +
> + ret = regmap_read_poll_timeout(regmap, sta_ofs,
> + val, !(val & mask),
> + MTK_POLL_DELAY_US,
> + MTK_POLL_TIMEOUT);
> + return ret;
> +}
> +
> +/**
> + * mtk_set_bus_protection - enable bus protection
> + * @infracfg: The bus protect regmap, default use infracfg
> + * @mask: The mask containing the protection bits to be enabled.
> + *
> + * This function enables the bus protection bits for disabled power
> + * domains so that the system does not hang when some unit accesses the
> + * bus while in power down.
> + */
> +int mtk_infracfg_set_bus_protection(struct regmap *infracfg, u32 mask)
> +{
> + return mtk_generic_set_cmd(infracfg,
> + INFRA_TOPAXI_PROTECTEN_SET,
> + INFRA_TOPAXI_PROTECTSTA1,
> + mask);
> +}
> +
> +/**
> + * mtk_clear_bus_protection - disable bus protection
> + * @infracfg: The bus protect regmap, default use infracfg
> + * @mask: The mask containing the protection bits to be disabled.
> + *
> + * This function disables the bus protection bits previously enabled with
> + * mtk_set_bus_protection.
> + */
> +
> +int mtk_infracfg_clear_bus_protection(struct regmap *infracfg, u32 mask)
> +{
> + return mtk_generic_clr_cmd(infracfg,
> + INFRA_TOPAXI_PROTECTEN_CLR,
> + INFRA_TOPAXI_PROTECTSTA1,
> + mask);
> +}
> +
> +/**
> + * mtk_infracfg_enable_bus_protection - enable bus protection
> + * @infracfg: The bus protect regmap, default use infracfg
> + * @mask: The mask containing the protection bits to be disabled.
> + *
> + * This function enables the bus protection bits for disabled power
> + * domains so that the system does not hang when some unit accesses the
> + * bus while in power down.
> + */
> +int mtk_infracfg_enable_bus_protection(struct regmap *infracfg, u32 mask)
> +{
> + return mtk_generic_enable_cmd(infracfg,
> + INFRA_TOPAXI_PROTECTEN,
> + INFRA_TOPAXI_PROTECTSTA1,
> + mask);
> +}
> +
> +/**
> + * mtk_infracfg_disable_bus_protection - disable bus protection
> + * @infracfg: The bus protect regmap, default use infracfg
> + * @mask: The mask containing the protection bits to be disabled.
> + *
> + * This function disables the bus protection bits previously enabled with
> + * mtk_infracfg_set_bus_protection.
> + */
> +
> +int mtk_infracfg_disable_bus_protection(struct regmap *infracfg, u32 mask)
> +{
> + return mtk_generic_disable_cmd(infracfg,
> + INFRA_TOPAXI_PROTECTEN,
> + INFRA_TOPAXI_PROTECTSTA1,
> + mask);
> +}
> diff --git a/drivers/soc/mediatek/mtk-scpsys-ext.c b/drivers/soc/mediatek/mtk-scpsys-ext.c
> new file mode 100644
> index 0000000..965e64d
> --- /dev/null
> +++ b/drivers/soc/mediatek/mtk-scpsys-ext.c
> @@ -0,0 +1,405 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018 MediaTek Inc.
> + * Author: Owen Chen <owen.chen@mediatek.com>
> + */
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/slab.h>
> +#include <linux/export.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/soc/mediatek/infracfg.h>
> +#include <linux/soc/mediatek/scpsys-ext.h>
> +
> +
> +#define MAX_CLKS 10
> +#define INFRA "infracfg"
> +#define SMIC "smi_comm"
Don't use defines for this. While at it I suppose it should be "smi_common"
> +
> +static LIST_HEAD(ext_clk_map_list);
> +static LIST_HEAD(ext_attr_map_list);
> +
> +static struct regmap *infracfg;
> +static struct regmap *smi_comm;
> +
> +enum regmap_type {
> + IFR_TYPE,
> + SMI_TYPE,
> + MAX_REGMAP_TYPE,
> +};
> +
> +/**
> + * struct ext_reg_ctrl - set multiple register for bus protect
> + * @regmap: The bus protect regmap, 1: infracfg, 2: other master regmap
> + * such as SMI.
> + * @set_ofs: The set register offset to set corresponding bit to 1.
> + * @clr_ofs: The clr register offset to clear corresponding bit to 0.
> + * @sta_ofs: The status register offset to show bus protect enable/disable.
> + */
> +struct ext_reg_ctrl {
> + enum regmap_type type;
> + u32 set_ofs;
> + u32 clr_ofs;
> + u32 sta_ofs;
> +};
> +
> +/**
> + * struct ext_clk_ctrl - enable multiple clks for bus protect
> + * @clk: The clk need to enable before pwr on/bus protect.
> + * @scpd_n: The name present the scpsys domain where the clks belongs to.
> + * @clk_list: The list node linked to ext_clk_map_list.
> + */
> +struct ext_clk_ctrl {
> + struct clk *clk;
> + const char *scpd_n;
> + struct list_head clk_list;
> +};
> +
> +struct bus_mask_ops {
> + int (*set)(struct regmap *regmap, u32 set_ofs,
> + u32 sta_ofs, u32 mask);
> + int (*release)(struct regmap *regmap, u32 clr_ofs,
> + u32 sta_ofs, u32 mask);
> +};
> +
> +static struct scpsys_ext_attr *__get_attr_parent(const char *parent_n)
> +{
> + struct scpsys_ext_attr *attr;
> +
> + if (!parent_n)
> + return ERR_PTR(-EINVAL);
> +
> + list_for_each_entry(attr, &ext_attr_map_list, attr_list) {
> + if (attr->scpd_n && !strcmp(parent_n, attr->scpd_n))
> + return attr;
> + }
> +
> + return ERR_PTR(-EINVAL);
> +}
> +
> +int bus_ctrl_set_release(struct scpsys_ext_attr *attr, bool set)
> +{
> + int i;
> + int ret = 0;
> +
> + for (i = 0; i < MAX_STEP_NUM && attr->mask[i].mask; i++) {
> + struct ext_reg_ctrl *rc = attr->mask[i].regs;
> + struct regmap *regmap;
> +
> + if (rc->type == IFR_TYPE)
> + regmap = infracfg;
> + else if (rc->type == SMI_TYPE)
> + regmap = smi_comm;
> + else
> + return -EINVAL;
> +
> + if (set)
> + ret = attr->mask[i].ops->set(regmap,
> + rc->set_ofs,
> + rc->sta_ofs,
> + attr->mask[i].mask);
> + else
> + ret = attr->mask[i].ops->release(regmap,
> + rc->clr_ofs,
> + rc->sta_ofs,
> + attr->mask[i].mask);
> + }
> +
> + return ret;
> +}
> +
> +int bus_ctrl_set(struct scpsys_ext_attr *attr)
> +{
> + return bus_ctrl_set_release(attr, CMD_ENABLE);
> +}
> +
> +int bus_ctrl_release(struct scpsys_ext_attr *attr)
> +{
> + return bus_ctrl_set_release(attr, CMD_DISABLE);
> +}
> +
> +int bus_clk_enable_disable(struct scpsys_ext_attr *attr, bool enable)
> +{
> + int i = 0;
> + int ret = 0;
> + struct ext_clk_ctrl *cc;
> + struct clk *clk[MAX_CLKS];
> +
> + list_for_each_entry(cc, &ext_clk_map_list, clk_list) {
Why can't we handle this in the same way as we do in mtk-scpsys.c ?
> + if (!strcmp(cc->scpd_n, attr->scpd_n)) {
> + if (enable)
> + ret = clk_prepare_enable(cc->clk);
> + else
> + clk_disable_unprepare(cc->clk);
> +
> + if (ret) {
> + pr_err("Failed to %s %s\n",
> + enable ? "enable" : "disable",
> + __clk_get_name(cc->clk));
> + goto err;
> + } else {
> + clk[i] = cc->clk;
> + i++;
> + }
> + }
> + }
> +
> + return ret;
> +
> +err:
> + for (--i; i >= 0; i--)
> + if (enable)
> + clk_disable_unprepare(clk[i]);
> + else
> + clk_prepare_enable(clk[i]);
> + return ret;
> +}
> +
> +int bus_clk_enable(struct scpsys_ext_attr *attr)
> +{
> + struct scpsys_ext_attr *attr_p;
> + int ret = 0;
> +
> + attr_p = __get_attr_parent(attr->parent_n);
Why can't we implement this using the pg_genpd_add_subdomain approach?
> + if (!IS_ERR(attr_p)) {
> + ret = bus_clk_enable_disable(attr_p, CMD_ENABLE);
> + if (ret)
> + return ret;
> + }
> +
> + return bus_clk_enable_disable(attr, CMD_ENABLE);
> +}
> +
> +int bus_clk_disable(struct scpsys_ext_attr *attr)
> +{
> + struct scpsys_ext_attr *attr_p;
> + int ret = 0;
> +
> + ret = bus_clk_enable_disable(attr, CMD_DISABLE);
> + if (ret)
> + return ret;
> +
> + attr_p = __get_attr_parent(attr->parent_n);
> + if (!IS_ERR(attr_p))
> + ret = bus_clk_enable_disable(attr_p, CMD_DISABLE);
Same here.
> +
> + return ret;
> +}
> +
> +const struct bus_mask_ops bus_mask_set_clr_ctrl = {
> + .set = &mtk_generic_set_cmd,
> + .release = &mtk_generic_clr_cmd,
> +};
> +
> +const struct bus_ext_ops ext_bus_ctrl = {
> + .enable = &bus_ctrl_set,
> + .disable = &bus_ctrl_release,
> +};
> +
> +const struct bus_ext_ops ext_cg_ctrl = {
> + .enable = &bus_clk_enable,
> + .disable = &bus_clk_disable,
> +};
> +
> +/*
> + * scpsys bus driver init
> + */
> +struct regmap *syscon_regmap_lookup_by_phandle_idx(struct device_node *np,
> + const char *property,
> + int index)
> +{
> + struct device_node *syscon_np;
> + struct regmap *regmap;
> +
> + if (property)
> + syscon_np = of_parse_phandle(np, property, index);
> + else
> + syscon_np = np;
> +
> + if (!syscon_np)
> + return ERR_PTR(-ENODEV);
> +
> + regmap = syscon_node_to_regmap(syscon_np);
> + of_node_put(syscon_np);
> +
> + return regmap;
> +}
Why do we need this? It is never called...
> +
> +int scpsys_ext_regmap_init(struct platform_device *pdev)
> +{
> + infracfg = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
> + INFRA);
> + if (IS_ERR(infracfg)) {
> + dev_err(&pdev->dev,
> + "Cannot find bus infracfg controller: %ld\n",
> + PTR_ERR(infracfg));
> + return PTR_ERR(infracfg);
> + }
> +
> + smi_comm = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
> + SMIC);
> + if (IS_ERR(smi_comm)) {
> + dev_err(&pdev->dev,
> + "Cannot find bus smi_comm controller: %ld\n",
> + PTR_ERR(smi_comm));
> + return PTR_ERR(smi_comm);
> + }
> +
> + return 0;
> +}
> +
> +static int add_clk_to_list(struct platform_device *pdev,
> + const char *name,
> + const char *scpd_n)
> +{
> + struct clk *clk;
> + struct ext_clk_ctrl *cc;
> +
> + clk = devm_clk_get(&pdev->dev, name);
> + if (IS_ERR(clk)) {
> + dev_err(&pdev->dev, "Failed add clk %ld\n", PTR_ERR(clk));
> + return PTR_ERR(clk);
> + }
> +
> + cc = kzalloc(sizeof(*cc), GFP_KERNEL);
> + cc->clk = clk;
> + cc->scpd_n = kstrdup(scpd_n, GFP_KERNEL);
> +
> + list_add(&cc->clk_list, &ext_clk_map_list);
> +
> + return 0;
> +}
> +
> +static int add_cg_to_list(struct platform_device *pdev)
> +{
> + int i = 0;
> +
> + struct device_node *node = pdev->dev.of_node;
> +
> + if (!node) {
> + dev_err(&pdev->dev, "Cannot find topcksys node: %ld\n",
Why topcksys? Shouldn't that be the node of scpsys?
> + PTR_ERR(node));
> + return PTR_ERR(node);
> + }
> +
> + do {
> + const char *ck_name;
> + char *temp_str;
> + char *tok[2] = {NULL};
> + int cg_idx = 0;
> + int idx = 0;
> + int ret = 0;
> +
> + ret = of_property_read_string_index(node, "clock-names", i,
> + &ck_name);
> + if (ret < 0)
> + break;
> +
> + temp_str = kmalloc_array(strlen(ck_name), sizeof(char),
> + GFP_KERNEL | __GFP_ZERO);
> + memcpy(temp_str, ck_name, strlen(ck_name));
> + temp_str[strlen(ck_name)] = '\0';
why don't you use kstrdup or similar?
> + do {
> + tok[idx] = strsep(&temp_str, "-");
> + idx++;
> + } while (temp_str);
You want to split the clock name like "mm-2" in
char **tok = {"mm", "2"};
correct? That can be done easier AFAIK:
tok[0] = strsep(&temp_str, "-");
tok[1] = &temp_str;
> +
> + if (idx == 2) {
You don't add clocks like "mfg" and "mm". Why?
> + if (kstrtouint(tok[1], 10, &cg_idx))
And then? You don't do anything with cg_idx...
> + return -EINVAL;
> +
> + if (add_clk_to_list(pdev, ck_name, tok[0]))
add_clk_to_list third parameter is the name of the scp domain, but you pass the
clock name here. I'm puzzled.
> + return -EINVAL;
> + }
> + kfree(temp_str);
> + i++;
> + } while (1);
> +
> + return 0;
> +}
> +
> +int scpsys_ext_clk_init(struct platform_device *pdev)
> +{
> + int ret = 0;
> +
> + ret = add_cg_to_list(pdev);
> + if (ret)
> + goto err;
> +
> +err:
> + return ret;
> +}
Why do we need add_cg_to_list, it can be implemented directly here. Why is here
a goto to a return statement that will be executed anyway? Please go through the
code and check that it is clean before submitting.
> +
> +int scpsys_ext_attr_init(const struct scpsys_ext_data *data)
> +{
> + int i, count = 0;
> +
> + for (i = 0; i < data->num_attr; i++) {
> + struct scpsys_ext_attr *node = data->attr + i;
> +
> + if (!node)
> + continue;
> +
> + list_add(&node->attr_list, &ext_attr_map_list);
> + count++;
> + }
> +
> + if (!count)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +static const struct of_device_id of_scpsys_ext_match_tbl[] = {
> + {
> + /* sentinel */
> + }
> +};
> +
> +struct scpsys_ext_data *scpsys_ext_init(struct platform_device *pdev)
> +{
> + const struct of_device_id *match;
> + struct scpsys_ext_data *data;
> + int ret;
> +
> + match = of_match_device(of_scpsys_ext_match_tbl, &pdev->dev);
> +
> + if (!match) {
> + dev_err(&pdev->dev, "no match\n");
> + return ERR_CAST(match);
> + }
> +
> + data = (struct scpsys_ext_data *)match->data;
> + if (IS_ERR(data)) {
> + dev_err(&pdev->dev, "no match scpext data\n");
> + return ERR_CAST(data);
> + }
> +
> + ret = scpsys_ext_attr_init(data);
> + if (ret) {
> + dev_err(&pdev->dev,
> + "Failed to init bus attr: %d\n",
> + ret);
> + return ERR_PTR(ret);
> + }
> +
> + ret = scpsys_ext_regmap_init(pdev);
> + if (ret) {
> + dev_err(&pdev->dev,
> + "Failed to init bus register: %d\n",
> + ret);
> + return ERR_PTR(ret);
> + }
> +
> + ret = scpsys_ext_clk_init(pdev);
> + if (ret) {
> + dev_err(&pdev->dev, "Failed to init bus clks: %d\n",
> + ret);
> + return ERR_PTR(ret);
> + }
> +
> + return data;
> +}
> diff --git a/drivers/soc/mediatek/mtk-scpsys.c b/drivers/soc/mediatek/mtk-scpsys.c
> index 4bb6c7a..03df2d6 100644
> --- a/drivers/soc/mediatek/mtk-scpsys.c
> +++ b/drivers/soc/mediatek/mtk-scpsys.c
> @@ -1,3 +1,4 @@
> +// SPDX-License-Identifier: GPL-2.0
> /*
> * Copyright (c) 2015 Pengutronix, Sascha Hauer <kernel@pengutronix.de>
> *
> @@ -20,6 +21,7 @@
> #include <linux/pm_domain.h>
> #include <linux/regulator/consumer.h>
> #include <linux/soc/mediatek/infracfg.h>
> +#include <linux/soc/mediatek/scpsys-ext.h>
>
> #include <dt-bindings/power/mt2701-power.h>
> #include <dt-bindings/power/mt2712-power.h>
> @@ -117,6 +119,15 @@ enum clk_id {
>
> #define MAX_CLKS 3
>
> +/**
> + * struct scp_domain_data - scp domain data for power on/off flow
> + * @name: The domain name.
> + * @sta_mask: The mask for power on/off status bit.
> + * @ctl_offs: The offset for main power control register.
> + * @sram_pdn_bits: The mask for sram power control bits.
> + * @sram_pdn_ack_bits The mask for sram power control acked bits.
> + * @caps: The flag for active wake-up action.
> + */
> struct scp_domain_data {
> const char *name;
> u32 sta_mask;
> @@ -150,7 +161,7 @@ struct scp {
> void __iomem *base;
> struct regmap *infracfg;
> struct scp_ctrl_reg ctrl_reg;
> - bool bus_prot_reg_update;
> + struct scpsys_ext_data *ext_data;
> };
>
> struct scp_subdomain {
> @@ -164,7 +175,6 @@ struct scp_soc_data {
> const struct scp_subdomain *subdomains;
> int num_subdomains;
> const struct scp_ctrl_reg regs;
> - bool bus_prot_reg_update;
> };
>
> static int scpsys_domain_is_on(struct scp_domain *scpd)
> @@ -236,6 +246,31 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
> val |= PWR_RST_B_BIT;
> writel(val, ctl_addr);
>
> + if (!IS_ERR(scp->ext_data)) {
> + struct scpsys_ext_attr *attr;
> +
> + attr = scp->ext_data->get_attr(scpd->data->name);
> + if (!IS_ERR(attr) && attr->cg_ops) {
> + ret = attr->cg_ops->enable(attr);
> + if (ret)
> + goto err_ext_clk;
> + }
> + }
> +
> + val &= ~scpd->data->sram_pdn_bits;
> + writel(val, ctl_addr);
> +
> + if (!IS_ERR(scp->ext_data)) {
> + struct scpsys_ext_attr *attr;
> +
> + attr = scp->ext_data->get_attr(scpd->data->name);
> + if (!IS_ERR(attr) && attr->cg_ops) {
> + ret = attr->cg_ops->enable(attr);
> + if (ret)
> + goto err_ext_clk;
> + }
> + }
> +
> val &= ~scpd->data->sram_pdn_bits;
> writel(val, ctl_addr);
>
> @@ -247,25 +282,65 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
> * applied here.
> */
> usleep_range(12000, 12100);
> -
> } else {
> ret = readl_poll_timeout(ctl_addr, tmp, (tmp & pdn_ack) == 0,
> MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
> if (ret < 0)
> - goto err_pwr_ack;
> + goto err_sram;
> }
>
> if (scpd->data->bus_prot_mask) {
> ret = mtk_infracfg_clear_bus_protection(scp->infracfg,
> - scpd->data->bus_prot_mask,
> - scp->bus_prot_reg_update);
> + scpd->data->bus_prot_mask);
> if (ret)
> - goto err_pwr_ack;
> + goto err_sram;
> + }
> +
> + if (!IS_ERR(scp->ext_data)) {
> + struct scpsys_ext_attr *attr;
> +
> + attr = scp->ext_data->get_attr(scpd->data->name);
> + if (!IS_ERR(attr) && attr->bus_ops) {
> + ret = attr->bus_ops->disable(attr);
> + if (ret)
> + goto err_sram;
> + }
> + }
> +
> + if (!IS_ERR(scp->ext_data)) {
> + struct scpsys_ext_attr *attr;
> +
> + attr = scp->ext_data->get_attr(scpd->data->name);
> + if (!IS_ERR(attr) && attr->cg_ops) {
> + ret = attr->cg_ops->disable(attr);
> + if (ret)
> + goto err_sram;
> + }
> }
>
> return 0;
>
> +err_sram:
> + val = readl(ctl_addr);
> + val |= scpd->data->sram_pdn_bits;
> + writel(val, ctl_addr);
> +err_ext_clk:
> + val = readl(ctl_addr);
> + val |= PWR_ISO_BIT;
> + writel(val, ctl_addr);
> +
> + val &= ~PWR_RST_B_BIT;
> + writel(val, ctl_addr);
> +
> + val |= PWR_CLK_DIS_BIT;
> + writel(val, ctl_addr);
> err_pwr_ack:
> + val &= ~PWR_ON_BIT;
> + writel(val, ctl_addr);
> +
> + val &= ~PWR_ON_2ND_BIT;
> + writel(val, ctl_addr);
> +
> for (i = MAX_CLKS - 1; i >= 0; i--) {
> if (scpd->clk[i])
> clk_disable_unprepare(scpd->clk[i]);
> @@ -274,8 +349,6 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
> if (scpd->supply)
> regulator_disable(scpd->supply);
>
> - dev_err(scp->dev, "Failed to power on domain %s\n", genpd->name);
> -
> return ret;
> }
>
> @@ -289,14 +362,35 @@ static int scpsys_power_off(struct generic_pm_domain *genpd)
> int ret, tmp;
> int i;
>
> + if (!IS_ERR(scp->ext_data)) {
> + struct scpsys_ext_attr *attr;
> +
> + attr = scp->ext_data->get_attr(scpd->data->name);
> + if (!IS_ERR(attr) && attr->cg_ops) {
> + ret = attr->cg_ops->enable(attr);
> + if (ret)
> + goto out;
> + }
> + }
> +
> if (scpd->data->bus_prot_mask) {
> ret = mtk_infracfg_set_bus_protection(scp->infracfg,
> - scpd->data->bus_prot_mask,
> - scp->bus_prot_reg_update);
> + scpd->data->bus_prot_mask);
> if (ret)
> goto out;
> }
>
> + if (!IS_ERR(scp->ext_data)) {
> + struct scpsys_ext_attr *attr;
> +
> + attr = scp->ext_data->get_attr(scpd->data->name);
> + if (!IS_ERR(attr) && attr->bus_ops) {
> + ret = attr->bus_ops->enable(attr);
> + if (ret)
> + goto out;
> + }
> + }
> +
> val = readl(ctl_addr);
> val |= scpd->data->sram_pdn_bits;
> writel(val, ctl_addr);
> @@ -307,6 +401,17 @@ static int scpsys_power_off(struct generic_pm_domain *genpd)
> if (ret < 0)
> goto out;
>
> + if (!IS_ERR(scp->ext_data)) {
> + struct scpsys_ext_attr *attr;
> +
> + attr = scp->ext_data->get_attr(scpd->data->name);
> + if (!IS_ERR(attr) && attr->cg_ops) {
> + ret = attr->cg_ops->disable(attr);
> + if (ret)
> + goto out;
> + }
> + }
> +
> val |= PWR_ISO_BIT;
> writel(val, ctl_addr);
>
> @@ -337,8 +442,6 @@ static int scpsys_power_off(struct generic_pm_domain *genpd)
> return 0;
>
> out:
> - dev_err(scp->dev, "Failed to power off domain %s\n", genpd->name);
> -
> return ret;
> }
>
> @@ -352,8 +455,7 @@ static void init_clks(struct platform_device *pdev, struct clk **clk)
>
> static struct scp *init_scp(struct platform_device *pdev,
> const struct scp_domain_data *scp_domain_data, int num,
> - const struct scp_ctrl_reg *scp_ctrl_reg,
> - bool bus_prot_reg_update)
> + const struct scp_ctrl_reg *scp_ctrl_reg)
> {
> struct genpd_onecell_data *pd_data;
> struct resource *res;
> @@ -367,11 +469,10 @@ static struct scp *init_scp(struct platform_device *pdev,
>
> scp->ctrl_reg.pwr_sta_offs = scp_ctrl_reg->pwr_sta_offs;
> scp->ctrl_reg.pwr_sta2nd_offs = scp_ctrl_reg->pwr_sta2nd_offs;
> -
> - scp->bus_prot_reg_update = bus_prot_reg_update;
> -
> scp->dev = &pdev->dev;
>
> + scp->ext_data = scpsys_ext_init(pdev);
> +
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> scp->base = devm_ioremap_resource(&pdev->dev, res);
> if (IS_ERR(scp->base))
> @@ -1021,7 +1122,6 @@ static void mtk_register_power_domains(struct platform_device *pdev,
> .pwr_sta_offs = SPM_PWR_STATUS,
> .pwr_sta2nd_offs = SPM_PWR_STATUS_2ND
> },
> - .bus_prot_reg_update = true,
> };
>
> static const struct scp_soc_data mt2712_data = {
> @@ -1033,7 +1133,6 @@ static void mtk_register_power_domains(struct platform_device *pdev,
> .pwr_sta_offs = SPM_PWR_STATUS,
> .pwr_sta2nd_offs = SPM_PWR_STATUS_2ND
> },
> - .bus_prot_reg_update = false,
> };
>
> static const struct scp_soc_data mt6765_data = {
> @@ -1056,7 +1155,6 @@ static void mtk_register_power_domains(struct platform_device *pdev,
> .pwr_sta_offs = SPM_PWR_STATUS_MT6797,
> .pwr_sta2nd_offs = SPM_PWR_STATUS_2ND_MT6797
> },
> - .bus_prot_reg_update = true,
I don't understand why you can delete this flag if you don't change anything
else in the data structure. For me this looks like you will break other chips.
Please explain.
I have the gut feeling that this can be implemented in the existing mtk-scpsys
driver. Can you please explain what are the points that this is not possible.
I want to understand the design decisions you made here, because they seem
really odd to me.
Best regards,
Matthias
> };
>
> static const struct scp_soc_data mt7622_data = {
> @@ -1066,7 +1164,6 @@ static void mtk_register_power_domains(struct platform_device *pdev,
> .pwr_sta_offs = SPM_PWR_STATUS,
> .pwr_sta2nd_offs = SPM_PWR_STATUS_2ND
> },
> - .bus_prot_reg_update = true,
> };
>
> static const struct scp_soc_data mt7623a_data = {
> @@ -1076,7 +1173,6 @@ static void mtk_register_power_domains(struct platform_device *pdev,
> .pwr_sta_offs = SPM_PWR_STATUS,
> .pwr_sta2nd_offs = SPM_PWR_STATUS_2ND
> },
> - .bus_prot_reg_update = true,
> };
>
> static const struct scp_soc_data mt8173_data = {
> @@ -1088,7 +1184,6 @@ static void mtk_register_power_domains(struct platform_device *pdev,
> .pwr_sta_offs = SPM_PWR_STATUS,
> .pwr_sta2nd_offs = SPM_PWR_STATUS_2ND
> },
> - .bus_prot_reg_update = true,
> };
>
> /*
> @@ -1132,8 +1227,8 @@ static int scpsys_probe(struct platform_device *pdev)
>
> soc = of_device_get_match_data(&pdev->dev);
>
> - scp = init_scp(pdev, soc->domains, soc->num_domains, &soc->regs,
> - soc->bus_prot_reg_update);
> + scp = init_scp(pdev, soc->domains, soc->num_domains,
> + &soc->regs);
> if (IS_ERR(scp))
> return PTR_ERR(scp);
>
> diff --git a/include/linux/soc/mediatek/infracfg.h b/include/linux/soc/mediatek/infracfg.h
> index fd25f01..bfad082 100644
> --- a/include/linux/soc/mediatek/infracfg.h
> +++ b/include/linux/soc/mediatek/infracfg.h
> @@ -32,8 +32,9 @@
> #define MT7622_TOP_AXI_PROT_EN_WB (BIT(2) | BIT(6) | \
> BIT(7) | BIT(8))
>
> -int mtk_infracfg_set_bus_protection(struct regmap *infracfg, u32 mask,
> - bool reg_update);
> -int mtk_infracfg_clear_bus_protection(struct regmap *infracfg, u32 mask,
> - bool reg_update);
> +int mtk_infracfg_set_bus_protection(struct regmap *infracfg, u32 mask);
> +int mtk_infracfg_clear_bus_protection(struct regmap *infracfg, u32 mask);
> +int mtk_infracfg_enable_bus_protection(struct regmap *infracfg, u32 mask);
> +int mtk_infracfg_disable_bus_protection(struct regmap *infracfg, u32 mask);
> +
> #endif /* __SOC_MEDIATEK_INFRACFG_H */
> diff --git a/include/linux/soc/mediatek/scpsys-ext.h b/include/linux/soc/mediatek/scpsys-ext.h
> new file mode 100644
> index 0000000..99b5ff1
> --- /dev/null
> +++ b/include/linux/soc/mediatek/scpsys-ext.h
> @@ -0,0 +1,66 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __SOC_MEDIATEK_SCPSYS_EXT_H
> +#define __SOC_MEDIATEK_SCPSYS_EXT_H
> +
> +#include <linux/platform_device.h>
> +
> +#define CMD_ENABLE 1
> +#define CMD_DISABLE 0
> +
> +#define MAX_STEP_NUM 4
> +
> +/**
> + * struct bus_mask - set mask and corresponding operation for bus protect
> + * @regs: The register set of bus register control, including set/clr/sta.
> + * @mask: The mask set for bus protect.
> + * @flag: The flag to idetify which operation we take for bus protect.
> + */
> +struct bus_mask {
> + struct ext_reg_ctrl *regs;
> + u32 mask;
> + const struct bus_mask_ops *ops;
> +};
> +
> +/**
> + * struct scpsys_ext_attr - extended attribute for bus protect and further
> + * operand.
> + *
> + * @scpd_n: The name present the scpsys domain where the clks belongs to.
> + * @mask: The mask set for bus protect.
> + * @bus_ops: The operation we take for bus protect.
> + * @cg_ops: The operation we take for cg on/off.
> + * @attr_list: The list node linked to ext_attr_map_list.
> + */
> +struct scpsys_ext_attr {
> + const char *scpd_n;
> + struct bus_mask mask[MAX_STEP_NUM];
> + const char *parent_n;
> + const struct bus_ext_ops *bus_ops;
> + const struct bus_ext_ops *cg_ops;
> +
> + struct list_head attr_list;
> +};
> +
> +struct scpsys_ext_data {
> + struct scpsys_ext_attr *attr;
> + u8 num_attr;
> + struct scpsys_ext_attr * (*get_attr)(const char *scpd_n);
> +};
> +
> +struct bus_ext_ops {
> + int (*enable)(struct scpsys_ext_attr *attr);
> + int (*disable)(struct scpsys_ext_attr *attr);
> +};
> +
> +int mtk_generic_set_cmd(struct regmap *regmap, u32 set_ofs,
> + u32 sta_ofs, u32 mask);
> +int mtk_generic_clr_cmd(struct regmap *regmap, u32 clr_ofs,
> + u32 sta_ofs, u32 mask);
> +int mtk_generic_enable_cmd(struct regmap *regmap, u32 upd_ofs,
> + u32 sta_ofs, u32 mask);
> +int mtk_generic_disable_cmd(struct regmap *regmap, u32 upd_ofs,
> + u32 sta_ofs, u32 mask);
> +
> +struct scpsys_ext_data *scpsys_ext_init(struct platform_device *pdev);
> +
> +#endif /* __SOC_MEDIATEK_SCPSYS_EXT_H */
>
^ permalink raw reply
* Re: [PATCH v1 0/3] console, serial8250: Disable PM and DMA ops
From: Andy Shevchenko @ 2018-07-18 14:50 UTC (permalink / raw)
To: Tony Lindgren, Andy Shevchenko
Cc: Sebastian Andrzej Siewior, Petr Mladek, Sergey Senozhatsky,
Steven Rostedt, Linux Kernel Mailing List, Greg Kroah-Hartman,
Jiri Slaby, open list:SERIAL DRIVERS, Arnd Bergmann
In-Reply-To: <20180523180052.GP98604@atomide.com>
On Wed, 2018-05-23 at 11:00 -0700, Tony Lindgren wrote:
> * Andy Shevchenko <andy.shevchenko@gmail.com> [180522 21:54]:
> > On Thu, May 17, 2018 at 10:48 PM, Tony Lindgren <tony@atomide.com>
> > wrote:
> > > * Andy Shevchenko <andy.shevchenko@gmail.com> [180517 16:38]:
> > > > On Thu, May 17, 2018 at 4:48 PM, Tony Lindgren <tony@atomide.com
> > > > > wrote:
> > > > > * Sebastian Andrzej Siewior <bigeasy@linutronix.de> [180516
> > > > > 10:49]:
> > > > > The idea breaking PM seems silly to me considering that we've
> > > > > had
> > > > > it working for years already.
> > > >
> > > > Same question / note. World is much more complex than just
> > > > being OMAP.
> > >
> > > Sorry but you are making assumptions about hardware being powered
> > > on
> > > all the time.
> >
> > Nope, other way around. The so called "support" _prevents_ our
> > hardware to go to sleep.
>
> Hmm sorry now I'm all confused what issues you're having.
>
> I thought you said earlier the issue was that you wanted to keep
> the console enabled all the time and never idle?
Yes, for kernel console.
To be clear, if user supplies "console=ttySx" it keeps powered on
always. But if there is no such parameter, we are fine with RPM.
Letting kernel console do power management on the systems without
irq_safe hack is dangerous in terms of loosing important data (crash, or
some other stuff which needs atomic context: kgdb?).
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply
* Re: [PATCH v1 0/3] console, serial8250: Disable PM and DMA ops
From: Andy Shevchenko @ 2018-07-18 15:14 UTC (permalink / raw)
To: Tony Lindgren, Andy Shevchenko
Cc: Sebastian Andrzej Siewior, Petr Mladek, Sergey Senozhatsky,
Steven Rostedt, Linux Kernel Mailing List, Greg Kroah-Hartman,
Jiri Slaby, open list:SERIAL DRIVERS, Arnd Bergmann
In-Reply-To: <20180523175859.GO98604@atomide.com>
On Wed, 2018-05-23 at 10:58 -0700, Tony Lindgren wrote:
> * Andy Shevchenko <andy.shevchenko@gmail.com> [180522 21:42]:
> > On Thu, May 17, 2018 at 10:30 PM, Tony Lindgren <tony@atomide.com>
> > wrote:
> > > So how about add some "noidle" kernel command line parameter for
> > > console
> > > that calls
> > > pm_runtime_forbid() and then you have the UART permanently
> > > on.
> >
> > IIUC _forbid() can be overwritten via sysfs.
> > And I would prefer to do other way around, something like
> > console.idle
> > and put default for OMAP to yes and no for everything else.
>
> OK yeah console.idle sounds good to me. We should default to a
> safe option.
I'll see what we can do here.
> > > Hmm I guess you could make also serial8250_rpm_get() do nothing
> > > based on that.
> >
> > Have you seen entire series which I keep here:
> > https://bitbucket.org/andy-shev/linux/branch/topic/uart/rpm?
> > Among other things it gets rid of those specific callbacks entirely.
>
> Well I was not Cc:ed on it, I browsed it in some archive and it
> seemed unsafe to me. But if you figured out a way to do it
> conditionally
> based on console.idle
> without causing regressions.
I restored that branch with some updated patches. It's far from done and
doesn't have any new stuff (yet) regrading to this discussion.
> > > I do agree the serial runtime PM has an issue if it depends on
> > > pm_runtime_irq_safe() being set.
> >
> > It's more than an issue. The so called "support" of RPM for UART is
> > _based on the hack_.
> > I would love to NAK that in the first place if I would have known of
> > it in time.
>
> Hmm well it seems that you too have been patching the 8250_rpm
> functions for years and then now what after multiple years you
> hit this issue? :)
Nope, I hit it as soon as I tried.
I can't find easily the discussion (hmm... yes, I was so pissed off that
time, I put a bit of harsh in that) I had with Sebastian few years back,
but at least I reported about an issue.
> > > > So, I can, of course just remove callbacks from the console
> > > > ->write().
> > > > Though it will prevent to use kernel console anyway.
> > >
> > > Please et's not start breaking things, we already see a constant
> > > flow of regressions on weekly basis.
> >
> > Now we are stick with a hack and the case based on that is against
> > fixing things.
> > This is how it looks from my side.
>
> Sorry yeah I agree there are issues, but let's fix it properly
Agree.
> with
> no regressions.
...though I think a word "regression" is inappropriate here. Regression
is what the support did in the first place. Pity I didn't know about it
at that time.
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply
* Re: [PATCH v1 0/3] console, serial8250: Disable PM and DMA ops
From: Tony Lindgren @ 2018-07-19 6:47 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Andy Shevchenko, Sebastian Andrzej Siewior, Petr Mladek,
Sergey Senozhatsky, Steven Rostedt, Linux Kernel Mailing List,
Greg Kroah-Hartman, Jiri Slaby, open list:SERIAL DRIVERS,
Arnd Bergmann
In-Reply-To: <c10bb604a6bf416e2092ae64a2cb9bb4658ea0eb.camel@linux.intel.com>
* Andy Shevchenko <andriy.shevchenko@linux.intel.com> [180718 15:17]:
> On Wed, 2018-05-23 at 10:58 -0700, Tony Lindgren wrote:
> >
> > OK yeah console.idle sounds good to me. We should default to a
> > safe option.
>
> I'll see what we can do here.
Like we discussed offline I think if we allow detaching and attaching
kernel console from userspace we can get rid of irqsafe completely :)
And in that case no need for console.idle or anything like that.
Then if kernel console is attached, we can keep console uart enabled.
If detached, it can do runtime PM if hardware supports it.
Not sure if we need some separate new /sys entry for that, maybe
we already have that with the loglevel in /proc/sys/kernel/printk?
Regards,
Tony
^ permalink raw reply
* Re: [PATCH v1 0/3] console, serial8250: Disable PM and DMA ops
From: Tony Lindgren @ 2018-07-19 6:50 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Andy Shevchenko, Sebastian Andrzej Siewior, Petr Mladek,
Sergey Senozhatsky, Steven Rostedt, Linux Kernel Mailing List,
Greg Kroah-Hartman, Jiri Slaby, open list:SERIAL DRIVERS,
Arnd Bergmann
In-Reply-To: <9bc02408588716c4ddb5a2ffb7d915019433fedc.camel@linux.intel.com>
* Andy Shevchenko <andriy.shevchenko@linux.intel.com> [180718 14:54]:
> On Wed, 2018-05-23 at 11:00 -0700, Tony Lindgren wrote:
> > I thought you said earlier the issue was that you wanted to keep
> > the console enabled all the time and never idle?
>
> Yes, for kernel console.
> To be clear, if user supplies "console=ttySx" it keeps powered on
> always. But if there is no such parameter, we are fine with RPM.
I'm fine with that as long as we have a way to detach or disable
kernel console from userspace after booting so the console uart can
idle.
> Letting kernel console do power management on the systems without
> irq_safe hack is dangerous in terms of loosing important data (crash, or
> some other stuff which needs atomic context: kgdb?).
Yes in the long run we should get rid of all pm_runtime_irq_safe()
use. It increases the parent dev PM runtime usage count permanently
blocking the whole domain from idling.
Regards,
Tony
^ permalink raw reply
* Re: [PATCH v5 09/11] clk: mediatek: add new clkmux register API
From: Sean Wang @ 2018-07-19 6:57 UTC (permalink / raw)
To: Mars Cheng
Cc: Matthias Brugger, Rob Herring, Marc Zyngier, Ryder Lee,
Stephen Boyd, CC Hwang, Loda Chou, linux-kernel, linux-mediatek,
devicetree, wsd_upstream, linux-serial, linux-arm-kernel,
linux-clk, Owen Chen
In-Reply-To: <1531817552-17221-10-git-send-email-mars.cheng@mediatek.com>
On Tue, 2018-07-17 at 16:52 +0800, Mars Cheng wrote:
> From: Owen Chen <owen.chen@mediatek.com>
>
> MT6765 add "set/clr" register for each clkmux setting, and
> one update register to trigger value change. It is designed
> to prevent read-modify-write racing issue. The sw design
> need to add a new API to handle this hw change with a new
> mtk_clk_mux/mtk_clk_upd struct in new file "clk-mux"and
> clk-upd".
>
I don't see any word mtk_clk_upd or clk-upd in the patch
and the patch needs to be split into more patches
> Signed-off-by: Owen Chen <owen.chen@mediatek.com>
> ---
> drivers/clk/mediatek/Makefile | 2 +-
> drivers/clk/mediatek/clk-mtk.c | 41 +++++++
> drivers/clk/mediatek/clk-mtk.h | 85 ++++++++++++---
> drivers/clk/mediatek/clk-mux.c | 236 ++++++++++++++++++++++++++++++++++++++++
> drivers/clk/mediatek/clk-mux.h | 38 +++++++
> 5 files changed, 388 insertions(+), 14 deletions(-)
> create mode 100644 drivers/clk/mediatek/clk-mux.c
> create mode 100644 drivers/clk/mediatek/clk-mux.h
>
> diff --git a/drivers/clk/mediatek/Makefile b/drivers/clk/mediatek/Makefile
> index 844b55d..b97980d 100644
> --- a/drivers/clk/mediatek/Makefile
> +++ b/drivers/clk/mediatek/Makefile
> @@ -1,5 +1,5 @@
> # SPDX-License-Identifier: GPL-2.0
> -obj-$(CONFIG_COMMON_CLK_MEDIATEK) += clk-mtk.o clk-pll.o clk-gate.o clk-apmixed.o clk-cpumux.o reset.o
> +obj-$(CONFIG_COMMON_CLK_MEDIATEK) += clk-mtk.o clk-pll.o clk-gate.o clk-apmixed.o clk-cpumux.o reset.o clk-mux.o
> obj-$(CONFIG_COMMON_CLK_MT6797) += clk-mt6797.o
> obj-$(CONFIG_COMMON_CLK_MT6797_IMGSYS) += clk-mt6797-img.o
> obj-$(CONFIG_COMMON_CLK_MT6797_MMSYS) += clk-mt6797-mm.o
> diff --git a/drivers/clk/mediatek/clk-mtk.c b/drivers/clk/mediatek/clk-mtk.c
> index 9c0ae42..50becd0 100644
> --- a/drivers/clk/mediatek/clk-mtk.c
> +++ b/drivers/clk/mediatek/clk-mtk.c
> @@ -22,6 +22,7 @@
> #include <linux/mfd/syscon.h>
>
> #include "clk-mtk.h"
> +#include "clk-mux.h"
> #include "clk-gate.h"
>
> struct clk_onecell_data *mtk_alloc_clk_data(unsigned int clk_num)
> @@ -144,6 +145,46 @@ int mtk_clk_register_gates(struct device_node *node,
> return 0;
> }
>
> +int mtk_clk_register_muxes(const struct mtk_mux *muxes,
> + int num, struct device_node *node,
> + spinlock_t *lock,
> + struct clk_onecell_data *clk_data)
> +{
> + struct regmap *regmap;
> + struct clk *clk;
> + int i;
> +
> + if (!clk_data)
> + return -ENOMEM;
> +
general register function is able to handle that there is no clk_data.
It looks like a optional, not a mandatory
> + regmap = syscon_node_to_regmap(node);
> + if (IS_ERR(regmap)) {
> + pr_err("Cannot find regmap for %pOF: %ld\n", node,
> + PTR_ERR(regmap));
> + return PTR_ERR(regmap);
> + }
> +
> + for (i = 0; i < num; i++) {
> + const struct mtk_mux *mux = &muxes[i];
> +
> + if (clk_data && !IS_ERR_OR_NULL(clk_data->clks[mux->id]))
> + continue;
> +
it seems not necessary to check clk data every time
and always use positive check is good to read
> + clk = mtk_clk_register_mux(mux, regmap, lock);
> +
> + if (IS_ERR(clk)) {
> + pr_err("Failed to register clk %s: %ld\n",
> + mux->name, PTR_ERR(clk));
> + continue;
> + }
> +
> + if (clk_data)
> + clk_data->clks[mux->id] = clk;
don't alter any data from input, that is a surprise for users
> + }
> +
> + return 0;
> +}
> +
> struct clk *mtk_clk_register_composite(const struct mtk_composite *mc,
> void __iomem *base, spinlock_t *lock)
> {
> diff --git a/drivers/clk/mediatek/clk-mtk.h b/drivers/clk/mediatek/clk-mtk.h
> index 1882221..61693f6 100644
> --- a/drivers/clk/mediatek/clk-mtk.h
> +++ b/drivers/clk/mediatek/clk-mtk.h
> @@ -24,7 +24,9 @@
>
> #define MAX_MUX_GATE_BIT 31
> #define INVALID_MUX_GATE_BIT (MAX_MUX_GATE_BIT + 1)
> -
> +#define INVALID_OFS -1
> +#define INVALID_SHFT -1
> +#define INVALID_WIDTH -1
> #define MHZ (1000 * 1000)
>
> struct mtk_fixed_clk {
> @@ -84,10 +86,72 @@ struct mtk_composite {
> signed char num_parents;
> };
>
> +struct mtk_mux {
> + int id;
> + const char *name;
> + const char * const *parent_names;
> + unsigned int flags;
> +
> + u32 mux_ofs;
> + u32 set_ofs;
> + u32 clr_ofs;
> + u32 upd_ofs;
> +
> + signed char mux_shift;
> + signed char mux_width;
> + signed char gate_shift;
> + signed char upd_shift;
> +
> + const struct clk_ops *ops;
> +
> + signed char num_parents;
> +};
> +
you have created a mtk-mux.h, why is you don't move the newly create
struct in?
> /*
> * In case the rate change propagation to parent clocks is undesirable,
> * this macro allows to specify the clock flags manually.
> */
> +#define CLR_SET_UPD_FLAGS(_id, _name, _parents, _mux_ofs, _mux_set_ofs,\
> + _mux_clr_ofs, _shift, _width, _gate, \
> + _upd_ofs, _upd, _flags, _ops) { \
> + .id = _id, \
> + .name = _name, \
> + .mux_ofs = _mux_ofs, \
> + .set_ofs = _mux_set_ofs, \
> + .clr_ofs = _mux_clr_ofs, \
> + .upd_ofs = _upd_ofs, \
> + .mux_shift = _shift, \
> + .mux_width = _width, \
> + .gate_shift = _gate, \
> + .upd_shift = _upd, \
> + .parent_names = _parents, \
> + .num_parents = ARRAY_SIZE(_parents), \
> + .flags = _flags, \
> + .ops = &_ops, \
> + }
> +
> +#define MUX_CLR_SET_UPD_FLAGS(_id, _name, _parents, _mux_ofs, _mux_set_ofs,\
> + _mux_clr_ofs, _shift, _width, _gate, \
> + _upd_ofs, _upd, _flags) \
> + CLR_SET_UPD_FLAGS(_id, _name, _parents, _mux_ofs, \
> + _mux_set_ofs, _mux_clr_ofs, _shift, _width, \
> + _gate, _upd_ofs, _upd, _flags, \
> + mtk_mux_clr_set_upd_ops)
> +
> +#define MUX_CLR_SET_UPD(_id, _name, _parents, _mux_ofs, _mux_set_ofs, \
> + _mux_clr_ofs, _shift, _width, _gate, \
> + _upd_ofs, _upd) \
> + MUX_CLR_SET_UPD_FLAGS(_id, _name, _parents, _mux_ofs, \
> + _mux_set_ofs, _mux_clr_ofs, _shift, _width, \
> + _gate, _upd_ofs, _upd, CLK_SET_RATE_PARENT)
> +
> +#define MUX_UPD(_id, _name, _parents, _mux_ofs, _shift, _width, _gate, \
> + _upd_ofs, _upd) \
> + CLR_SET_UPD_FLAGS(_id, _name, _parents, _mux_ofs, \
> + INVALID_OFS, INVALID_OFS, _shift, _width, \
> + _gate, _upd_ofs, _upd, CLK_SET_RATE_PARENT, \
> + mtk_mux_upd_ops)
> +
> #define MUX_GATE_FLAGS(_id, _name, _parents, _reg, _shift, _width, \
> _gate, _flags) { \
> .id = _id, \
> @@ -111,18 +175,8 @@ struct mtk_composite {
> MUX_GATE_FLAGS(_id, _name, _parents, _reg, _shift, _width, \
> _gate, CLK_SET_RATE_PARENT)
>
> -#define MUX(_id, _name, _parents, _reg, _shift, _width) { \
> - .id = _id, \
> - .name = _name, \
> - .mux_reg = _reg, \
> - .mux_shift = _shift, \
> - .mux_width = _width, \
> - .gate_shift = -1, \
> - .divider_shift = -1, \
> - .parent_names = _parents, \
> - .num_parents = ARRAY_SIZE(_parents), \
> - .flags = CLK_SET_RATE_PARENT, \
> - }
As Matthias always said that, you alter the common thing that is already
used by a lot of SoC.
You should have a dedicate patch to state why you need it, provide the
way you propose. and use another patches for migrating old SoC, finally
then add the patch for your own SoC.
Don't mix everything in a single patch. The patch can't become reusable
hard to read it, even hard to backport to the other SoC picking up
patches they are really wanting.
> +#define MUX(_id, _name, _parents, _reg, _shift, _width) \
> + MUX_GATE(_id, _name, _parents, _reg, _shift, _width, INVALID_SHFT)
>
> #define DIV_GATE(_id, _name, _parent, _gate_reg, _gate_shift, _div_reg, \
> _div_width, _div_shift) { \
> @@ -138,6 +192,11 @@ struct mtk_composite {
> .flags = 0, \
> }
>
> +int mtk_clk_register_muxes(const struct mtk_mux *muxes,
> + int num, struct device_node *node,
> + spinlock_t *lock,
> + struct clk_onecell_data *clk_data);
> +
move to mtk-mux.h
> struct clk *mtk_clk_register_composite(const struct mtk_composite *mc,
> void __iomem *base, spinlock_t *lock);
>
> diff --git a/drivers/clk/mediatek/clk-mux.c b/drivers/clk/mediatek/clk-mux.c
> new file mode 100644
> index 0000000..219181b
> --- /dev/null
> +++ b/drivers/clk/mediatek/clk-mux.c
> @@ -0,0 +1,236 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018 MediaTek Inc.
> + * Author: Owen Chen <owen.chen@mediatek.com>
> + */
> +
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/slab.h>
> +
> +#include "clk-mtk.h"
> +#include "clk-mux.h"
> +
> +static inline struct mtk_clk_mux
> + *to_mtk_clk_mux(struct clk_hw *hw)
> +{
> + return container_of(hw, struct mtk_clk_mux, hw);
> +}
> +
> +static int mtk_mux_enable(struct clk_hw *hw)
> +{
> + struct mtk_clk_mux *mux = to_mtk_clk_mux(hw);
> + u32 mask = BIT(mux->gate_shift);
> + unsigned long flags = 0;
> +
> + if (mux->lock)
> + spin_lock_irqsave(mux->lock, flags);
> +
we can see many functions in kernel, similar to your need, they always
be defined as two versions. for example.
mtk_mux_enable refer to lock version
mtk_mux_enable_nolock for lock-free version
that makes less condition, more readable, and users don't care much
about what stuff is put inside
> + regmap_update_bits(mux->regmap, mux->mux_ofs, mask, 0);
> +
> + if (mux->lock)
> + spin_unlock_irqrestore(mux->lock, flags);
> + return 0;
> +}
> +
> +static void mtk_mux_disable(struct clk_hw *hw)
> +{
> + struct mtk_clk_mux *mux = to_mtk_clk_mux(hw);
> + u32 mask = BIT(mux->gate_shift);
> + unsigned long flags = 0;
> +
> + if (mux->lock)
> + spin_lock_irqsave(mux->lock, flags);
> +
ditto
> + regmap_update_bits(mux->regmap, mux->mux_ofs, mask, mask);
> +
> + if (mux->lock)
> + spin_unlock_irqrestore(mux->lock, flags);
> +}
> +
> +static int mtk_mux_enable_setclr(struct clk_hw *hw)
> +{
> + struct mtk_clk_mux *mux = to_mtk_clk_mux(hw);
> + u32 val;
> + unsigned long flags = 0;
> +
> + if (mux->lock)
> + spin_lock_irqsave(mux->lock, flags);
> +
ditto
> + val = BIT(mux->gate_shift);
> + regmap_write(mux->regmap, mux->mux_clr_ofs, val);
> +
> + if (mux->lock)
> + spin_unlock_irqrestore(mux->lock, flags);
> + return 0;
> +}
> +
> +static void mtk_mux_disable_setclr(struct clk_hw *hw)
> +{
> + struct mtk_clk_mux *mux = to_mtk_clk_mux(hw);
> + u32 val;
> + unsigned long flags = 0;
> +
> + if (mux->lock)
> + spin_lock_irqsave(mux->lock, flags);
> +
ditto
> + val = BIT(mux->gate_shift);
> + regmap_write(mux->regmap, mux->mux_set_ofs, val);
> +
> + if (mux->lock)
> + spin_unlock_irqrestore(mux->lock, flags);
> +}
> +
> +static int mtk_mux_is_enabled(struct clk_hw *hw)
> +{
> + struct mtk_clk_mux *mux = to_mtk_clk_mux(hw);
> + u32 val = 0;
> +
> + if (mux->gate_shift < 0)
> + return true;
> +
return value should be bool
> + regmap_read(mux->regmap, mux->mux_ofs, &val);
> +
> + return (val & BIT(mux->gate_shift)) == 0;
> +}
> +
> +static u8 mtk_mux_get_parent(struct clk_hw *hw)
> +{
return value should be int
> + struct mtk_clk_mux *mux = to_mtk_clk_mux(hw);
> + int num_parents = clk_hw_get_num_parents(hw);
> + u32 mask = GENMASK(mux->mux_width - 1, 0);
> + u32 val;
> +
> + regmap_read(mux->regmap, mux->mux_ofs, &val);
> + val = (val >> mux->mux_shift) & mask;
> +
> + if (val >= num_parents)
> + return -EINVAL;
> +
> + return val;
> +}
> +
> +static int mtk_mux_set_parent(struct clk_hw *hw, u8 index)
> +{
> + struct mtk_clk_mux *mux = to_mtk_clk_mux(hw);
> + u32 mask = GENMASK(mux->mux_width - 1, 0);
> + u32 val, orig;
> + unsigned long flags = 0;
> +
> + if (mux->lock)
> + spin_lock_irqsave(mux->lock, flags);
> +
use lock-free-or-not funciton
> + regmap_read(mux->regmap, mux->mux_ofs, &val);
> + orig = val;
> + val &= ~(mask << mux->mux_shift);
> + val |= index << mux->mux_shift;
> +
> + if (val != orig) {
> + regmap_write(mux->regmap, mux->mux_ofs, val);
> +
> + if (mux->upd_shift >= 0)
> + regmap_write(mux->regmap, mux->upd_ofs,
> + BIT(mux->upd_shift));
why not use regmap_update_bits like function ?
> + }
> +
> + if (mux->lock)
> + spin_unlock_irqrestore(mux->lock, flags);
> +
> + return 0;
> +}
> +
> +static int mtk_mux_set_parent_setclr(struct clk_hw *hw, u8 index)
> +{
> + struct mtk_clk_mux *mux = to_mtk_clk_mux(hw);
> + u32 mask = GENMASK(mux->mux_width - 1, 0);
> + u32 val, orig;
> + unsigned long flags = 0;
> +
> + if (mux->lock)
> + spin_lock_irqsave(mux->lock, flags);
> +
use lock-free-or-not funciton
> + regmap_read(mux->regmap, mux->mux_ofs, &val);
> + orig = val;
> + val &= ~(mask << mux->mux_shift);
> + val |= index << mux->mux_shift;
> +
> + if (val != orig) {
> + val = (mask << mux->mux_shift);
> + regmap_write(mux->regmap, mux->mux_clr_ofs, val);
> + val = (index << mux->mux_shift);
> + regmap_write(mux->regmap, mux->mux_set_ofs, val);
> +
why not use regmap_update_bits like function ?
> + if (mux->upd_shift >= 0)
> + regmap_write(mux->regmap, mux->upd_ofs,
> + BIT(mux->upd_shift));
> + }
> +
> + if (mux->lock)
> + spin_unlock_irqrestore(mux->lock, flags);
> +
> + return 0;
> +}
> +
> +const struct clk_ops mtk_mux_upd_ops = {
> + .enable = mtk_mux_enable,
> + .disable = mtk_mux_disable,
> + .is_enabled = mtk_mux_is_enabled,
> + .get_parent = mtk_mux_get_parent,
> + .set_parent = mtk_mux_set_parent,
> + .determine_rate = NULL,
explicitly set as NULL can be removed
> +};
> +
> +const struct clk_ops mtk_mux_clr_set_upd_ops = {
> + .enable = mtk_mux_enable_setclr,
> + .disable = mtk_mux_disable_setclr,
> + .is_enabled = mtk_mux_is_enabled,
> + .get_parent = mtk_mux_get_parent,
> + .set_parent = mtk_mux_set_parent_setclr,
> + .determine_rate = NULL,
explicitly set as NULL can be removed
> +};
> +
> +struct clk *mtk_clk_register_mux(const struct mtk_mux *mux,
> + struct regmap *regmap,
> + spinlock_t *lock)
> +{
> + struct clk *clk;
> + struct clk_init_data init;
> + struct mtk_clk_mux *mtk_mux = NULL;
> + int ret;
> +
make declaration as reverse xmas tree
> + mtk_mux = kzalloc(sizeof(*mtk_mux), GFP_KERNEL);
> + if (!mtk_mux)
> + return ERR_PTR(-ENOMEM);
> +
> + init.name = mux->name;
> + init.flags = (mux->flags) | CLK_SET_RATE_PARENT;
> + init.parent_names = mux->parent_names;
> + init.num_parents = mux->num_parents;
> + init.ops = mux->ops;
> +
> + mtk_mux->regmap = regmap;
> + mtk_mux->name = mux->name;
> + mtk_mux->mux_ofs = mux->mux_ofs;
> + mtk_mux->mux_set_ofs = mux->set_ofs;
> + mtk_mux->mux_clr_ofs = mux->clr_ofs;
> + mtk_mux->upd_ofs = mux->upd_ofs;
> + mtk_mux->mux_shift = mux->mux_shift;
> + mtk_mux->mux_width = mux->mux_width;
> + mtk_mux->gate_shift = mux->gate_shift;
> + mtk_mux->upd_shift = mux->upd_shift;
> +
> + mtk_mux->lock = lock;
> + mtk_mux->hw.init = &init;
> +
> + clk = clk_register(NULL, &mtk_mux->hw);
> + if (IS_ERR(clk)) {
> + ret = PTR_ERR(clk);
ret is superfluous
> + goto err_out;
> + }
> +
> + return clk;
> +err_out:
> + kfree(mtk_mux);
> +
I felt err path can be optimized
> + return ERR_PTR(ret);
> +}
> diff --git a/drivers/clk/mediatek/clk-mux.h b/drivers/clk/mediatek/clk-mux.h
> new file mode 100644
> index 0000000..64f8e7c
> --- /dev/null
> +++ b/drivers/clk/mediatek/clk-mux.h
> @@ -0,0 +1,38 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2018 MediaTek Inc.
> + * Author: Owen Chen <owen.chen@mediatek.com>
> + */
> +
> +#ifndef __DRV_CLK_MUX_H
> +#define __DRV_CLK_MUX_H
> +
> +#include <linux/clk-provider.h>
> +
> +struct mtk_clk_mux {
> + struct clk_hw hw;
> + struct regmap *regmap;
> +
> + const char *name;
> +
> + int mux_set_ofs;
> + int mux_clr_ofs;
> + int mux_ofs;
> + int upd_ofs;
> +
> + s8 mux_shift;
> + s8 mux_width;
> + s8 gate_shift;
> + s8 upd_shift;
> +
> + spinlock_t *lock;
> +};
> +
> +extern const struct clk_ops mtk_mux_upd_ops;
> +extern const struct clk_ops mtk_mux_clr_set_upd_ops;
> +
extern is superfluous
> +struct clk *mtk_clk_register_mux(const struct mtk_mux *mux,
> + struct regmap *regmap,
> + spinlock_t *lock);
> +
> +#endif /* __DRV_CLK_MUX_H */
^ permalink raw reply
* [PATCH v2 0/2] add ISO7816 support
From: Ludovic Desroches @ 2018-07-19 8:47 UTC (permalink / raw)
To: linux-serial, linux-arch, linux-arm-kernel
Cc: gregkh, jslaby, arnd, richard.genoud, nicolas.ferre,
alexandre.belloni, linux-kernel, Ludovic Desroches
Hi,
This patchset adds support for the ISO7816 standard. The USART devices in
Microchip SoCs have an ISO7816 mode. It allows to let the USART managing
the CLK and I/O signals of a smart card.
Changes:
- v2
- uart_get_iso7816_config: check there is an iso7816_config function
- use IOCTL macros to generate the IOCTL number
- check that reserved field is not used
- remove debug logs
- check that the iso7816_config is right before doing any action
- change the error from nack and max iteration status to a debug message
- remove patch 3 as it concerns both rs485 and iso7816 to think more
about the need of adding a lock or not
Nicolas Ferre (2):
tty/serial_core: add ISO7816 infrastructure
tty/serial: atmel: add ISO7816 support
arch/alpha/include/uapi/asm/ioctls.h | 2 +
arch/mips/include/uapi/asm/ioctls.h | 2 +
arch/powerpc/include/uapi/asm/ioctls.h | 2 +
arch/sh/include/uapi/asm/ioctls.h | 2 +
arch/sparc/include/uapi/asm/ioctls.h | 2 +
arch/xtensa/include/uapi/asm/ioctls.h | 2 +
drivers/tty/serial/atmel_serial.c | 170 +++++++++++++++++++++++++++++++--
drivers/tty/serial/atmel_serial.h | 3 +-
drivers/tty/serial/serial_core.c | 60 ++++++++++++
include/linux/serial_core.h | 3 +
include/uapi/asm-generic/ioctls.h | 2 +
include/uapi/linux/serial.h | 17 ++++
12 files changed, 256 insertions(+), 11 deletions(-)
--
2.12.2
^ permalink raw reply
* [PATCH v2 1/2] tty/serial_core: add ISO7816 infrastructure
From: Ludovic Desroches @ 2018-07-19 8:47 UTC (permalink / raw)
To: linux-serial, linux-arch, linux-arm-kernel
Cc: gregkh, jslaby, arnd, richard.genoud, nicolas.ferre,
alexandre.belloni, linux-kernel, Ludovic Desroches
In-Reply-To: <20180719084737.1490-1-ludovic.desroches@microchip.com>
From: Nicolas Ferre <nicolas.ferre@microchip.com>
Add the ISO7816 ioctl and associated accessors and data structure.
Drivers can then use this common implementation to handle ISO7816.
Signed-off-by: Nicolas Ferre <nicolas.ferre@microchip.com>
Signed-off-by: Ludovic Desroches <ludovic.desroches@microchip.com>
---
arch/alpha/include/uapi/asm/ioctls.h | 2 ++
arch/mips/include/uapi/asm/ioctls.h | 2 ++
arch/powerpc/include/uapi/asm/ioctls.h | 2 ++
arch/sh/include/uapi/asm/ioctls.h | 2 ++
arch/sparc/include/uapi/asm/ioctls.h | 2 ++
arch/xtensa/include/uapi/asm/ioctls.h | 2 ++
drivers/tty/serial/serial_core.c | 60 ++++++++++++++++++++++++++++++++++
include/linux/serial_core.h | 3 ++
include/uapi/asm-generic/ioctls.h | 2 ++
include/uapi/linux/serial.h | 17 ++++++++++
10 files changed, 94 insertions(+)
diff --git a/arch/alpha/include/uapi/asm/ioctls.h b/arch/alpha/include/uapi/asm/ioctls.h
index 3729d92d3fa8..1e9121c9b3c7 100644
--- a/arch/alpha/include/uapi/asm/ioctls.h
+++ b/arch/alpha/include/uapi/asm/ioctls.h
@@ -102,6 +102,8 @@
#define TIOCGPTLCK _IOR('T', 0x39, int) /* Get Pty lock state */
#define TIOCGEXCL _IOR('T', 0x40, int) /* Get exclusive mode state */
#define TIOCGPTPEER _IO('T', 0x41) /* Safely open the slave */
+#define TIOCGISO7816 _IOR('T', 0x42, struct serial_iso7816)
+#define TIOCSISO7816 _IOWR('T', 0x43, struct serial_iso7816)
#define TIOCSERCONFIG 0x5453
#define TIOCSERGWILD 0x5454
diff --git a/arch/mips/include/uapi/asm/ioctls.h b/arch/mips/include/uapi/asm/ioctls.h
index 890245a9f0c4..16aa8a766aec 100644
--- a/arch/mips/include/uapi/asm/ioctls.h
+++ b/arch/mips/include/uapi/asm/ioctls.h
@@ -93,6 +93,8 @@
#define TIOCGPTLCK _IOR('T', 0x39, int) /* Get Pty lock state */
#define TIOCGEXCL _IOR('T', 0x40, int) /* Get exclusive mode state */
#define TIOCGPTPEER _IO('T', 0x41) /* Safely open the slave */
+#define TIOCGISO7816 _IOR('T', 0x42, struct serial_iso7816)
+#define TIOCSISO7816 _IOWR('T', 0x43, struct serial_iso7816)
/* I hope the range from 0x5480 on is free ... */
#define TIOCSCTTY 0x5480 /* become controlling tty */
diff --git a/arch/powerpc/include/uapi/asm/ioctls.h b/arch/powerpc/include/uapi/asm/ioctls.h
index 41b1a5c15734..2c145da3b774 100644
--- a/arch/powerpc/include/uapi/asm/ioctls.h
+++ b/arch/powerpc/include/uapi/asm/ioctls.h
@@ -102,6 +102,8 @@
#define TIOCGPTLCK _IOR('T', 0x39, int) /* Get Pty lock state */
#define TIOCGEXCL _IOR('T', 0x40, int) /* Get exclusive mode state */
#define TIOCGPTPEER _IO('T', 0x41) /* Safely open the slave */
+#define TIOCGISO7816 _IOR('T', 0x42, struct serial_iso7816)
+#define TIOCSISO7816 _IOWR('T', 0x43, struct serial_iso7816)
#define TIOCSERCONFIG 0x5453
#define TIOCSERGWILD 0x5454
diff --git a/arch/sh/include/uapi/asm/ioctls.h b/arch/sh/include/uapi/asm/ioctls.h
index cc62f6f98103..11866d4f60e1 100644
--- a/arch/sh/include/uapi/asm/ioctls.h
+++ b/arch/sh/include/uapi/asm/ioctls.h
@@ -95,6 +95,8 @@
#define TIOCGPTLCK _IOR('T', 0x39, int) /* Get Pty lock state */
#define TIOCGEXCL _IOR('T', 0x40, int) /* Get exclusive mode state */
#define TIOCGPTPEER _IO('T', 0x41) /* Safely open the slave */
+#define TIOCGISO7816 _IOR('T', 0x42, struct serial_iso7816)
+#define TIOCSISO7816 _IOWR('T', 0x43, struct serial_iso7816)
#define TIOCSERCONFIG _IO('T', 83) /* 0x5453 */
#define TIOCSERGWILD _IOR('T', 84, int) /* 0x5454 */
diff --git a/arch/sparc/include/uapi/asm/ioctls.h b/arch/sparc/include/uapi/asm/ioctls.h
index 2df52711e170..7fd2f5873c9e 100644
--- a/arch/sparc/include/uapi/asm/ioctls.h
+++ b/arch/sparc/include/uapi/asm/ioctls.h
@@ -27,6 +27,8 @@
#define TIOCGEXCL _IOR('T', 0x40, int) /* Get exclusive mode state */
#define TIOCGRS485 _IOR('T', 0x41, struct serial_rs485)
#define TIOCSRS485 _IOWR('T', 0x42, struct serial_rs485)
+#define TIOCGISO7816 _IOR('T', 0x43, struct serial_iso7816)
+#define TIOCSISO7816 _IOWR('T', 0x44, struct serial_iso7816)
/* Note that all the ioctls that are not available in Linux have a
* double underscore on the front to: a) avoid some programs to
diff --git a/arch/xtensa/include/uapi/asm/ioctls.h b/arch/xtensa/include/uapi/asm/ioctls.h
index ec43609cbfc5..6d4a87296c95 100644
--- a/arch/xtensa/include/uapi/asm/ioctls.h
+++ b/arch/xtensa/include/uapi/asm/ioctls.h
@@ -107,6 +107,8 @@
#define TIOCGPTLCK _IOR('T', 0x39, int) /* Get Pty lock state */
#define TIOCGEXCL _IOR('T', 0x40, int) /* Get exclusive mode state */
#define TIOCGPTPEER _IO('T', 0x41) /* Safely open the slave */
+#define TIOCGISO7816 _IOR('T', 0x42, struct serial_iso7816)
+#define TIOCSISO7816 _IOWR('T', 0x43, struct serial_iso7816)
#define TIOCSERCONFIG _IO('T', 83)
#define TIOCSERGWILD _IOR('T', 84, int)
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 9c14a453f73c..af8d5b12fba9 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1301,6 +1301,58 @@ static int uart_set_rs485_config(struct uart_port *port,
return 0;
}
+static int uart_get_iso7816_config(struct uart_port *port,
+ struct serial_iso7816 __user *iso7816)
+{
+ unsigned long flags;
+ struct serial_iso7816 aux;
+
+ if (!port->iso7816_config)
+ return -ENOIOCTLCMD;
+
+ spin_lock_irqsave(&port->lock, flags);
+ aux = port->iso7816;
+ spin_unlock_irqrestore(&port->lock, flags);
+
+ if (copy_to_user(iso7816, &aux, sizeof(aux)))
+ return -EFAULT;
+
+ return 0;
+}
+
+static int uart_set_iso7816_config(struct uart_port *port,
+ struct serial_iso7816 __user *iso7816_user)
+{
+ struct serial_iso7816 iso7816;
+ int i, ret;
+ unsigned long flags;
+
+ if (!port->iso7816_config)
+ return -ENOIOCTLCMD;
+
+ if (copy_from_user(&iso7816, iso7816_user, sizeof(*iso7816_user)))
+ return -EFAULT;
+
+ /*
+ * There are 5 words reserved for future use. Check that userspace
+ * doesn't put stuff in there to prevent breakages in the future.
+ */
+ for (i = 0; i < 5; i++)
+ if (iso7816.reserved[i])
+ return -EINVAL;
+
+ spin_lock_irqsave(&port->lock, flags);
+ ret = port->iso7816_config(port, &iso7816);
+ spin_unlock_irqrestore(&port->lock, flags);
+ if (ret)
+ return ret;
+
+ if (copy_to_user(iso7816_user, &port->iso7816, sizeof(port->iso7816)))
+ return -EFAULT;
+
+ return 0;
+}
+
/*
* Called via sys_ioctl. We can use spin_lock_irq() here.
*/
@@ -1385,6 +1437,14 @@ uart_ioctl(struct tty_struct *tty, unsigned int cmd, unsigned long arg)
case TIOCSRS485:
ret = uart_set_rs485_config(uport, uarg);
break;
+
+ case TIOCSISO7816:
+ ret = uart_set_iso7816_config(state->uart_port, uarg);
+ break;
+
+ case TIOCGISO7816:
+ ret = uart_get_iso7816_config(state->uart_port, uarg);
+ break;
default:
if (uport->ops->ioctl)
ret = uport->ops->ioctl(uport, cmd, arg);
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 06ea4eeb09ab..d6e7747ffd46 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -137,6 +137,8 @@ struct uart_port {
void (*handle_break)(struct uart_port *);
int (*rs485_config)(struct uart_port *,
struct serial_rs485 *rs485);
+ int (*iso7816_config)(struct uart_port *,
+ struct serial_iso7816 *iso7816);
unsigned int irq; /* irq number */
unsigned long irqflags; /* irq flags */
unsigned int uartclk; /* base uart clock */
@@ -253,6 +255,7 @@ struct uart_port {
struct attribute_group *attr_group; /* port specific attributes */
const struct attribute_group **tty_groups; /* all attributes (serial core use only) */
struct serial_rs485 rs485;
+ struct serial_iso7816 iso7816;
void *private_data; /* generic platform data pointer */
};
diff --git a/include/uapi/asm-generic/ioctls.h b/include/uapi/asm-generic/ioctls.h
index 040651735662..cdc9f4ca8c27 100644
--- a/include/uapi/asm-generic/ioctls.h
+++ b/include/uapi/asm-generic/ioctls.h
@@ -79,6 +79,8 @@
#define TIOCGPTLCK _IOR('T', 0x39, int) /* Get Pty lock state */
#define TIOCGEXCL _IOR('T', 0x40, int) /* Get exclusive mode state */
#define TIOCGPTPEER _IO('T', 0x41) /* Safely open the slave */
+#define TIOCGISO7816 _IOR('T', 0x42, struct serial_iso7816)
+#define TIOCSISO7816 _IOWR('T', 0x43, struct serial_iso7816)
#define FIONCLEX 0x5450
#define FIOCLEX 0x5451
diff --git a/include/uapi/linux/serial.h b/include/uapi/linux/serial.h
index 3fdd0dee8b41..93eb3c496ff1 100644
--- a/include/uapi/linux/serial.h
+++ b/include/uapi/linux/serial.h
@@ -132,4 +132,21 @@ struct serial_rs485 {
are a royal PITA .. */
};
+/*
+ * Serial interface for controlling ISO7816 settings on chips with suitable
+ * support. Set with TIOCSISO7816 and get with TIOCGISO7816 if supported by
+ * your platform.
+ */
+struct serial_iso7816 {
+ __u32 flags; /* ISO7816 feature flags */
+#define SER_ISO7816_ENABLED (1 << 0)
+#define SER_ISO7816_T_PARAM (0x0f << 4)
+#define SER_ISO7816_T(t) (((t) & 0x0f) << 4)
+ __u32 tg;
+ __u32 sc_fi;
+ __u32 sc_di;
+ __u32 clk;
+ __u32 reserved[5];
+};
+
#endif /* _UAPI_LINUX_SERIAL_H */
--
2.12.2
^ permalink raw reply related
* [PATCH v2 2/2] tty/serial: atmel: add ISO7816 support
From: Ludovic Desroches @ 2018-07-19 8:47 UTC (permalink / raw)
To: linux-serial, linux-arch, linux-arm-kernel
Cc: gregkh, jslaby, arnd, richard.genoud, nicolas.ferre,
alexandre.belloni, linux-kernel, Ludovic Desroches
In-Reply-To: <20180719084737.1490-1-ludovic.desroches@microchip.com>
From: Nicolas Ferre <nicolas.ferre@microchip.com>
When mode is set in atmel_config_iso7816() we backup last RS232 mode
for coming back to this mode if requested.
Also allow setup of T=0 and T=1 parameter and basic support in set_termios
function as well.
Report NACK and ITER errors in irq handler.
Signed-off-by: Nicolas Ferre <nicolas.ferre@microchip.com>
Signed-off-by: Ludovic Desroches <ludovic.desroches@microchip.com>
---
drivers/tty/serial/atmel_serial.c | 170 +++++++++++++++++++++++++++++++++++---
drivers/tty/serial/atmel_serial.h | 3 +-
2 files changed, 162 insertions(+), 11 deletions(-)
diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index 8e4428725848..cec958f1e7d4 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -34,6 +34,7 @@
#include <linux/suspend.h>
#include <linux/mm.h>
+#include <asm/div64.h>
#include <asm/io.h>
#include <asm/ioctls.h>
@@ -147,6 +148,8 @@ struct atmel_uart_port {
struct circ_buf rx_ring;
struct mctrl_gpios *gpios;
+ u32 backup_mode; /* MR saved during iso7816 operations */
+ u32 backup_brgr; /* BRGR saved during iso7816 operations */
unsigned int tx_done_mask;
u32 fifo_size;
u32 rts_high;
@@ -362,6 +365,132 @@ static int atmel_config_rs485(struct uart_port *port,
return 0;
}
+static unsigned int atmel_calc_cd(struct uart_port *port,
+ struct serial_iso7816 *iso7816conf)
+{
+ struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
+ unsigned int cd;
+ u64 mck_rate;
+
+ mck_rate = (u64)clk_get_rate(atmel_port->clk);
+ do_div(mck_rate, iso7816conf->clk);
+ cd = mck_rate;
+ return cd;
+}
+
+static unsigned int atmel_calc_fidi(struct uart_port *port,
+ struct serial_iso7816 *iso7816conf)
+{
+ u64 fidi = 0;
+
+ if (iso7816conf->sc_fi && iso7816conf->sc_di) {
+ fidi = (u64)iso7816conf->sc_fi;
+ do_div(fidi, iso7816conf->sc_di);
+ }
+ return (u32)fidi;
+}
+
+/* Enable or disable the iso7816 support */
+/* Called with interrupts disabled */
+static int atmel_config_iso7816(struct uart_port *port,
+ struct serial_iso7816 *iso7816conf)
+{
+ struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
+ unsigned int mode, t;
+ unsigned int cd, fidi;
+ int ret = 0;
+
+ /* Disable RX and TX */
+ atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_RXDIS | ATMEL_US_TXDIS);
+ /* Disable interrupts */
+ atmel_uart_writel(port, ATMEL_US_IDR, atmel_port->tx_done_mask);
+
+ mode = atmel_uart_readl(port, ATMEL_US_MR);
+
+ if (iso7816conf->flags & SER_ISO7816_ENABLED) {
+ mode &= ~ATMEL_US_USMODE;
+
+ if ((iso7816conf->flags & SER_ISO7816_T_PARAM)
+ == SER_ISO7816_T(0)) {
+ mode |= ATMEL_US_USMODE_ISO7816_T0;
+ t = 0;
+ } else if ((iso7816conf->flags & SER_ISO7816_T_PARAM)
+ == SER_ISO7816_T(1)) {
+ mode |= ATMEL_US_USMODE_ISO7816_T1;
+ t = 1;
+ } else {
+ dev_warn(port->dev, "ISO7816 Type not supported. Resetting\n");
+ memset(iso7816conf, 0, sizeof(struct serial_iso7816));
+ goto err_out;
+ }
+
+ dev_dbg(port->dev, "Setting USART to ISO7816 mode T%d\n", t);
+
+ mode &= ~(ATMEL_US_USCLKS | ATMEL_US_CHRL
+ | ATMEL_US_NBSTOP | ATMEL_US_PAR);
+
+ /* NACK configuration */
+ if ((iso7816conf->flags & SER_ISO7816_T_PARAM)
+ == SER_ISO7816_T(0))
+ mode |= ATMEL_US_DSNACK;
+ else
+ mode |= ATMEL_US_INACK;
+ /* select mck clock, and output */
+ mode |= ATMEL_US_USCLKS_MCK | ATMEL_US_CLKO;
+ /* set parity for normal/inverse mode + max iterations */
+ mode |= ATMEL_US_PAR_EVEN | ATMEL_US_NBSTOP_1 | (3 << 24);
+
+ cd = atmel_calc_cd(port, iso7816conf);
+ fidi = atmel_calc_fidi(port, iso7816conf);
+ if (fidi < 0) {
+ dev_warn(port->dev, "ISO7816 fidi = 0, Generator generates no signal\n");
+ } else if (fidi == 1 || fidi == 2) {
+ dev_err(port->dev, "ISO7816 fidi = %u, value not supported\n", fidi);
+ ret = -EINVAL;
+ goto err_out;
+ }
+
+ if (!(port->iso7816.flags & SER_ISO7816_ENABLED)) {
+ /* port not yet in iso7816 mode: store configuration */
+ atmel_port->backup_mode = atmel_uart_readl(port, ATMEL_US_MR);
+ atmel_port->backup_brgr = atmel_uart_readl(port, ATMEL_US_BRGR);
+ }
+
+ /* Actually set ISO7816 mode */
+ atmel_uart_writel(port, ATMEL_US_TTGR, iso7816conf->tg);
+ atmel_uart_writel(port, ATMEL_US_BRGR, cd);
+ atmel_uart_writel(port, ATMEL_US_FIDIR, fidi);
+
+ atmel_port->tx_done_mask = ATMEL_US_TXEMPTY | ATMEL_US_NACK | ATMEL_US_ITERATION;
+ } else {
+ dev_dbg(port->dev, "Setting UART to RS232\n");
+ /* back to last RS232 settings */
+ mode = atmel_port->backup_mode;
+ memset(iso7816conf, 0, sizeof(struct serial_iso7816));
+ atmel_uart_writel(port, ATMEL_US_TTGR, 0);
+ atmel_uart_writel(port, ATMEL_US_BRGR, atmel_port->backup_brgr);
+ atmel_uart_writel(port, ATMEL_US_FIDIR, 0x174);
+
+ if (atmel_use_pdc_tx(port))
+ atmel_port->tx_done_mask = ATMEL_US_ENDTX |
+ ATMEL_US_TXBUFE;
+ else
+ atmel_port->tx_done_mask = ATMEL_US_TXRDY;
+ }
+
+ port->iso7816 = *iso7816conf;
+
+ atmel_uart_writel(port, ATMEL_US_MR, mode);
+
+err_out:
+ /* Enable interrupts */
+ atmel_uart_writel(port, ATMEL_US_IER, atmel_port->tx_done_mask);
+ /* Enable RX and TX */
+ atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_RXEN | ATMEL_US_TXEN);
+
+ return ret;
+}
+
/*
* Return TIOCSER_TEMT when transmitter FIFO and Shift register is empty.
*/
@@ -481,8 +610,9 @@ static void atmel_stop_tx(struct uart_port *port)
/* Disable interrupts */
atmel_uart_writel(port, ATMEL_US_IDR, atmel_port->tx_done_mask);
- if ((port->rs485.flags & SER_RS485_ENABLED) &&
- !(port->rs485.flags & SER_RS485_RX_DURING_TX))
+ if (((port->rs485.flags & SER_RS485_ENABLED) &&
+ !(port->rs485.flags & SER_RS485_RX_DURING_TX)) ||
+ port->iso7816.flags & SER_ISO7816_ENABLED)
atmel_start_rx(port);
}
@@ -500,8 +630,9 @@ static void atmel_start_tx(struct uart_port *port)
return;
if (atmel_use_pdc_tx(port) || atmel_use_dma_tx(port))
- if ((port->rs485.flags & SER_RS485_ENABLED) &&
- !(port->rs485.flags & SER_RS485_RX_DURING_TX))
+ if (((port->rs485.flags & SER_RS485_ENABLED) &&
+ !(port->rs485.flags & SER_RS485_RX_DURING_TX)) ||
+ port->iso7816.flags & SER_ISO7816_ENABLED)
atmel_stop_rx(port);
if (atmel_use_pdc_tx(port))
@@ -799,8 +930,9 @@ static void atmel_complete_tx_dma(void *arg)
*/
if (!uart_circ_empty(xmit))
atmel_tasklet_schedule(atmel_port, &atmel_port->tasklet_tx);
- else if ((port->rs485.flags & SER_RS485_ENABLED) &&
- !(port->rs485.flags & SER_RS485_RX_DURING_TX)) {
+ else if (((port->rs485.flags & SER_RS485_ENABLED) &&
+ !(port->rs485.flags & SER_RS485_RX_DURING_TX)) ||
+ port->iso7816.flags & SER_ISO7816_ENABLED) {
/* DMA done, stop TX, start RX for RS485 */
atmel_start_rx(port);
}
@@ -1281,6 +1413,9 @@ atmel_handle_status(struct uart_port *port, unsigned int pending,
wake_up_interruptible(&port->state->port.delta_msr_wait);
}
}
+
+ if (pending & (ATMEL_US_NACK | ATMEL_US_ITERATION))
+ dev_dbg(port->dev, "ISO7816 ERROR (0x%08x)\n", pending);
}
/*
@@ -1373,8 +1508,9 @@ static void atmel_tx_pdc(struct uart_port *port)
atmel_uart_writel(port, ATMEL_US_IER,
atmel_port->tx_done_mask);
} else {
- if ((port->rs485.flags & SER_RS485_ENABLED) &&
- !(port->rs485.flags & SER_RS485_RX_DURING_TX)) {
+ if (((port->rs485.flags & SER_RS485_ENABLED) &&
+ !(port->rs485.flags & SER_RS485_RX_DURING_TX)) ||
+ port->iso7816.flags & SER_ISO7816_ENABLED) {
/* DMA done, stop TX, start RX for RS485 */
atmel_start_rx(port);
}
@@ -2099,6 +2235,17 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
atmel_uart_writel(port, ATMEL_US_TTGR,
port->rs485.delay_rts_after_send);
mode |= ATMEL_US_USMODE_RS485;
+ } else if (port->iso7816.flags & SER_ISO7816_ENABLED) {
+ atmel_uart_writel(port, ATMEL_US_TTGR, port->iso7816.tg);
+ /* select mck clock, and output */
+ mode |= ATMEL_US_USCLKS_MCK | ATMEL_US_CLKO;
+ /* set max iterations */
+ mode |= (3 << 24);
+ if ((port->iso7816.flags & SER_ISO7816_T_PARAM)
+ == SER_ISO7816_T(0))
+ mode |= ATMEL_US_USMODE_ISO7816_T0;
+ else
+ mode |= ATMEL_US_USMODE_ISO7816_T1;
} else if (termios->c_cflag & CRTSCTS) {
/* RS232 with hardware handshake (RTS/CTS) */
if (atmel_use_fifo(port) &&
@@ -2175,7 +2322,8 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
}
quot = cd | fp << ATMEL_US_FP_OFFSET;
- atmel_uart_writel(port, ATMEL_US_BRGR, quot);
+ if (!(port->iso7816.flags & SER_ISO7816_ENABLED))
+ atmel_uart_writel(port, ATMEL_US_BRGR, quot);
atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_RSTSTA | ATMEL_US_RSTRX);
atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN | ATMEL_US_RXEN);
atmel_port->tx_stopped = false;
@@ -2355,6 +2503,7 @@ static int atmel_init_port(struct atmel_uart_port *atmel_port,
port->mapbase = pdev->resource[0].start;
port->irq = pdev->resource[1].start;
port->rs485_config = atmel_config_rs485;
+ port->iso7816_config = atmel_config_iso7816;
port->membase = NULL;
memset(&atmel_port->rx_ring, 0, sizeof(atmel_port->rx_ring));
@@ -2379,7 +2528,8 @@ static int atmel_init_port(struct atmel_uart_port *atmel_port,
}
/* Use TXEMPTY for interrupt when rs485 else TXRDY or ENDTX|TXBUFE */
- if (port->rs485.flags & SER_RS485_ENABLED)
+ if (port->rs485.flags & SER_RS485_ENABLED ||
+ port->iso7816.flags & SER_ISO7816_ENABLED)
atmel_port->tx_done_mask = ATMEL_US_TXEMPTY;
else if (atmel_use_pdc_tx(port)) {
port->fifosize = PDC_BUFFER_SIZE;
diff --git a/drivers/tty/serial/atmel_serial.h b/drivers/tty/serial/atmel_serial.h
index ba3a2437cde4..fff51f5fe8bc 100644
--- a/drivers/tty/serial/atmel_serial.h
+++ b/drivers/tty/serial/atmel_serial.h
@@ -124,7 +124,8 @@
#define ATMEL_US_TTGR 0x28 /* Transmitter Timeguard Register */
#define ATMEL_US_TG GENMASK(7, 0) /* Timeguard Value */
-#define ATMEL_US_FIDI 0x40 /* FI DI Ratio Register */
+#define ATMEL_US_FIDIR 0x40 /* FI DI Ratio Register */
+#define ATMEL_US_FIDI GENMASK(15, 0) /* FIDI ratio */
#define ATMEL_US_NER 0x44 /* Number of Errors Register */
#define ATMEL_US_IF 0x4c /* IrDA Filter Register */
--
2.12.2
^ permalink raw reply related
* Re: [PATCH v2 0/2] add ISO7816 support
From: Neil Armstrong @ 2018-07-19 8:59 UTC (permalink / raw)
To: Ludovic Desroches, linux-serial, linux-arch, linux-arm-kernel
Cc: alexandre.belloni, arnd, richard.genoud, gregkh, linux-kernel,
jslaby
In-Reply-To: <20180719084737.1490-1-ludovic.desroches@microchip.com>
Hi Ludovic,
On 19/07/2018 10:47, Ludovic Desroches wrote:
> Hi,
>
> This patchset adds support for the ISO7816 standard. The USART devices in
> Microchip SoCs have an ISO7816 mode. It allows to let the USART managing
> the CLK and I/O signals of a smart card.
Wow, I would have loved to have this at the time...
I'm curious, do you have an example of userspace code using this ?
The ATR rx needs a very weird handling, I'm curious how you managed it.
Thanks,
Neil
>
> Changes:
> - v2
> - uart_get_iso7816_config: check there is an iso7816_config function
> - use IOCTL macros to generate the IOCTL number
> - check that reserved field is not used
> - remove debug logs
> - check that the iso7816_config is right before doing any action
> - change the error from nack and max iteration status to a debug message
> - remove patch 3 as it concerns both rs485 and iso7816 to think more
> about the need of adding a lock or not
>
> Nicolas Ferre (2):
> tty/serial_core: add ISO7816 infrastructure
> tty/serial: atmel: add ISO7816 support
>
> arch/alpha/include/uapi/asm/ioctls.h | 2 +
> arch/mips/include/uapi/asm/ioctls.h | 2 +
> arch/powerpc/include/uapi/asm/ioctls.h | 2 +
> arch/sh/include/uapi/asm/ioctls.h | 2 +
> arch/sparc/include/uapi/asm/ioctls.h | 2 +
> arch/xtensa/include/uapi/asm/ioctls.h | 2 +
> drivers/tty/serial/atmel_serial.c | 170 +++++++++++++++++++++++++++++++--
> drivers/tty/serial/atmel_serial.h | 3 +-
> drivers/tty/serial/serial_core.c | 60 ++++++++++++
> include/linux/serial_core.h | 3 +
> include/uapi/asm-generic/ioctls.h | 2 +
> include/uapi/linux/serial.h | 17 ++++
> 12 files changed, 256 insertions(+), 11 deletions(-)
>
^ permalink raw reply
* Re: [PATCH v2 0/2] add ISO7816 support
From: Ludovic Desroches @ 2018-07-19 9:26 UTC (permalink / raw)
To: Neil Armstrong
Cc: linux-serial, linux-arch, linux-arm-kernel, alexandre.belloni,
arnd, richard.genoud, gregkh, linux-kernel, jslaby
In-Reply-To: <034fe4ed-979b-51ee-db7c-5dc7137b98e5@baylibre.com>
Hi Neil,
On Thu, Jul 19, 2018 at 10:59:47AM +0200, Neil Armstrong wrote:
> Hi Ludovic,
>
> On 19/07/2018 10:47, Ludovic Desroches wrote:
> > Hi,
> >
> > This patchset adds support for the ISO7816 standard. The USART devices in
> > Microchip SoCs have an ISO7816 mode. It allows to let the USART managing
> > the CLK and I/O signals of a smart card.
>
> Wow, I would have loved to have this at the time...
> I'm curious, do you have an example of userspace code using this ?
> The ATR rx needs a very weird handling, I'm curious how you managed it.
>
Unfortunately, I have nothing I can give you at the moment. I am doing
some experiments. Before going further, I need to have the interface
with the kernel accepted.
Of course, I can give more details about the experiments I am doing.
First of all, I am not used to ISO7816 so every feedback are
appreciated.
On the userspace part, there is the PCSC Lite library. It needs a
userspace driver. From what I've seen, most of the readers are managed
by the CCID driver. This driver handle different hardware capabilities.
Some hardware are very close to what we provide with our SoC ie. just
sending the data on the line and not manage the procedure bytes.
The CCID driver includes a lot of things related to the ISO7816 protocol
so I tried to plug my code into it to reuse this code. The issue is that I am
not a CCID device. At the moment, I am writing my own PCSC driver and I am
copying/pasting code I need. The USART only manages the CLK and I/O
signals. Others one are handled by the PCSC driver with the help of
GPIOs.
At a time, I was thinking about a CCID driver (which interprets the CCID
header and interacts with the device handling ISO7816) in the kernel to easily
interface with the PCSC CCID driver but I am not sure it's the right way
to go.
Regards
Ludovic
> Thanks,
> Neil
>
> >
> > Changes:
> > - v2
> > - uart_get_iso7816_config: check there is an iso7816_config function
> > - use IOCTL macros to generate the IOCTL number
> > - check that reserved field is not used
> > - remove debug logs
> > - check that the iso7816_config is right before doing any action
> > - change the error from nack and max iteration status to a debug message
> > - remove patch 3 as it concerns both rs485 and iso7816 to think more
> > about the need of adding a lock or not
> >
> > Nicolas Ferre (2):
> > tty/serial_core: add ISO7816 infrastructure
> > tty/serial: atmel: add ISO7816 support
> >
> > arch/alpha/include/uapi/asm/ioctls.h | 2 +
> > arch/mips/include/uapi/asm/ioctls.h | 2 +
> > arch/powerpc/include/uapi/asm/ioctls.h | 2 +
> > arch/sh/include/uapi/asm/ioctls.h | 2 +
> > arch/sparc/include/uapi/asm/ioctls.h | 2 +
> > arch/xtensa/include/uapi/asm/ioctls.h | 2 +
> > drivers/tty/serial/atmel_serial.c | 170 +++++++++++++++++++++++++++++++--
> > drivers/tty/serial/atmel_serial.h | 3 +-
> > drivers/tty/serial/serial_core.c | 60 ++++++++++++
> > include/linux/serial_core.h | 3 +
> > include/uapi/asm-generic/ioctls.h | 2 +
> > include/uapi/linux/serial.h | 17 ++++
> > 12 files changed, 256 insertions(+), 11 deletions(-)
> >
>
^ permalink raw reply
* Re: [PATCH 1/3] tty/serial_core: add ISO7816 infrastructure
From: Alan Cox @ 2018-07-19 11:07 UTC (permalink / raw)
To: Ludovic Desroches
Cc: linux-arch, alexandre.belloni, arnd, richard.genoud, Greg KH,
linux-kernel, linux-serial, jslaby, linux-arm-kernel
In-Reply-To: <20180713080129.2ifw7crjksvaxxpq@M43218.corp.atmel.com>
> >
> > > + if (!port->iso7816_config)
> > > + return -ENOIOCTLCMD;
> >
> > Why this error value?
> >
>
> It was a mimic of RS485.
Which is what you want - it means the upper tty layer knows to offer the
ioctl to other places and then return appropriately.
Alan
^ permalink raw reply
* Re: [PATCH v2 1/2] tty/serial_core: add ISO7816 infrastructure
From: kbuild test robot @ 2018-07-19 23:54 UTC (permalink / raw)
Cc: linux-arch, alexandre.belloni, arnd, richard.genoud, gregkh,
linux-kernel, Ludovic Desroches, kbuild-all, linux-serial, jslaby,
linux-arm-kernel
In-Reply-To: <20180719084737.1490-2-ludovic.desroches@microchip.com>
[-- Attachment #1: Type: text/plain, Size: 4392 bytes --]
Hi Nicolas,
I love your patch! Yet something to improve:
[auto build test ERROR on tty/tty-testing]
[also build test ERROR on v4.18-rc5 next-20180719]
[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/Ludovic-Desroches/tty-serial_core-add-ISO7816-infrastructure/20180719-183102
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
config: parisc-generic-64bit_defconfig (attached as .config)
compiler: hppa64-linux-gnu-gcc (GCC) 7.3.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
GCC_VERSION=7.3.0 make.cross ARCH=parisc
All errors (new ones prefixed by >>):
drivers/tty/serial/serial_core.c: In function 'uart_ioctl':
>> drivers/tty/serial/serial_core.c:1448:7: error: 'TIOCSISO7816' undeclared (first use in this function); did you mean 'TIOCSRS485'?
case TIOCSISO7816:
^~~~~~~~~~~~
TIOCSRS485
drivers/tty/serial/serial_core.c:1448:7: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/tty/serial/serial_core.c:1452:7: error: 'TIOCGISO7816' undeclared (first use in this function); did you mean 'TIOCSISO7816'?
case TIOCGISO7816:
^~~~~~~~~~~~
TIOCSISO7816
vim +1448 drivers/tty/serial/serial_core.c
1362
1363 /*
1364 * Called via sys_ioctl. We can use spin_lock_irq() here.
1365 */
1366 static int
1367 uart_ioctl(struct tty_struct *tty, unsigned int cmd, unsigned long arg)
1368 {
1369 struct uart_state *state = tty->driver_data;
1370 struct tty_port *port = &state->port;
1371 struct uart_port *uport;
1372 void __user *uarg = (void __user *)arg;
1373 int ret = -ENOIOCTLCMD;
1374
1375
1376 /*
1377 * These ioctls don't rely on the hardware to be present.
1378 */
1379 switch (cmd) {
1380 case TIOCGSERIAL:
1381 ret = uart_get_info_user(port, uarg);
1382 break;
1383
1384 case TIOCSSERIAL:
1385 down_write(&tty->termios_rwsem);
1386 ret = uart_set_info_user(tty, state, uarg);
1387 up_write(&tty->termios_rwsem);
1388 break;
1389
1390 case TIOCSERCONFIG:
1391 down_write(&tty->termios_rwsem);
1392 ret = uart_do_autoconfig(tty, state);
1393 up_write(&tty->termios_rwsem);
1394 break;
1395
1396 case TIOCSERGWILD: /* obsolete */
1397 case TIOCSERSWILD: /* obsolete */
1398 ret = 0;
1399 break;
1400 }
1401
1402 if (ret != -ENOIOCTLCMD)
1403 goto out;
1404
1405 if (tty_io_error(tty)) {
1406 ret = -EIO;
1407 goto out;
1408 }
1409
1410 /*
1411 * The following should only be used when hardware is present.
1412 */
1413 switch (cmd) {
1414 case TIOCMIWAIT:
1415 ret = uart_wait_modem_status(state, arg);
1416 break;
1417 }
1418
1419 if (ret != -ENOIOCTLCMD)
1420 goto out;
1421
1422 mutex_lock(&port->mutex);
1423 uport = uart_port_check(state);
1424
1425 if (!uport || tty_io_error(tty)) {
1426 ret = -EIO;
1427 goto out_up;
1428 }
1429
1430 /*
1431 * All these rely on hardware being present and need to be
1432 * protected against the tty being hung up.
1433 */
1434
1435 switch (cmd) {
1436 case TIOCSERGETLSR: /* Get line status register */
1437 ret = uart_get_lsr_info(tty, state, uarg);
1438 break;
1439
1440 case TIOCGRS485:
1441 ret = uart_get_rs485_config(uport, uarg);
1442 break;
1443
1444 case TIOCSRS485:
1445 ret = uart_set_rs485_config(uport, uarg);
1446 break;
1447
> 1448 case TIOCSISO7816:
1449 ret = uart_set_iso7816_config(state->uart_port, uarg);
1450 break;
1451
> 1452 case TIOCGISO7816:
1453 ret = uart_get_iso7816_config(state->uart_port, uarg);
1454 break;
1455 default:
1456 if (uport->ops->ioctl)
1457 ret = uport->ops->ioctl(uport, cmd, arg);
1458 break;
1459 }
1460 out_up:
1461 mutex_unlock(&port->mutex);
1462 out:
1463 return ret;
1464 }
1465
---
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: 19052 bytes --]
[-- Attachment #3: Type: text/plain, Size: 176 bytes --]
_______________________________________________
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 v3 0/4] Add basic support for Mediatek MT8183 SoC
From: Erin Lo @ 2018-07-20 8:19 UTC (permalink / raw)
To: Matthias Brugger
Cc: Rob Herring, Mark Rutland, Thomas Gleixner, Jason Cooper,
Marc Zyngier, Greg Kroah-Hartman, devicetree, srv_heupstream,
mars.cheng, linux-kernel, linux-mediatek, linux-serial,
yingjoe.chen, linux-arm-kernel
In-Reply-To: <9ba45353-a6e1-9a6e-f05b-4b1679ae9e52@gmail.com>
On Mon, 2018-07-16 at 11:28 +0200, Matthias Brugger wrote:
> Hi Erin,
>
> On 17/05/18 08:22, Erin Lo wrote:
> > MT8183 is a SoC based on 64bit ARMv8 architecture.
> > It contains 4 CA53 and 4 CA73 cores.
> > MT8183 share many HW IP with MT65xx series.
> > This patchset was tested on MT8183 evaluation board, and boot to shell ok.
> >
> > This series contains document bindings, device tree including interrupt, uart.
> >
> > Change in v3:
> > 1. Fill out GICC, GICH, GICV regions
> > 2. Update Copyright to 2018
> >
> > Change in v2:
> > 1. Split dt-bindings into different patches
> > 2. Correct bindings for supported SoCs (mtk-uart.txt)
> >
> > Ben Ho (1):
> > arm64: dts: Add Mediatek SoC MT8183 and evaluation board dts and
> > Makefile
> >
> > Erin Lo (3):
> > dt-bindings: arm: Add bindings for Mediatek MT8183 SoC Platform
> > dt-bindings: mtk-sysirq: Add compatible for Mediatek MT8183
> > dt-bindings: serial: Add compatible for Mediatek MT8183
> >
>
> I'm a bit reluctant to take this series, as it will only enable the EVB board to
> boot into a serial console. Are you planning to add support for other devices of
> this SoC?
>
> Apart please take into account that there is an issue with the dts file, as you
> were told by the kbuild test robot.
>
> Regards,
> Matthias
>
Hi, Matthias
Sorry for missing this letter...since mail proxy server.
We plan to add support all the devices of MT8183 in serious.
We have implemented the clock and pinctrl driver for upstream and they
are in internal review right now.
About the dts issue... do you suggest me to send new patch right now or
wait for clock and pinctrl driver ready then send them together?
Best Regards,
Erin
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply
* Re: [PATCH v3 0/4] Add basic support for Mediatek MT8183 SoC
From: Matthias Brugger @ 2018-07-20 10:44 UTC (permalink / raw)
To: Erin Lo
Cc: Rob Herring, Mark Rutland, Thomas Gleixner, Jason Cooper,
Marc Zyngier, Greg Kroah-Hartman, devicetree, srv_heupstream,
mars.cheng, linux-kernel, linux-mediatek, linux-serial,
yingjoe.chen, linux-arm-kernel
In-Reply-To: <1532074769.19148.2.camel@mtksdaap41>
On 20/07/18 10:19, Erin Lo wrote:
> On Mon, 2018-07-16 at 11:28 +0200, Matthias Brugger wrote:
>> Hi Erin,
>>
>> On 17/05/18 08:22, Erin Lo wrote:
>>> MT8183 is a SoC based on 64bit ARMv8 architecture.
>>> It contains 4 CA53 and 4 CA73 cores.
>>> MT8183 share many HW IP with MT65xx series.
>>> This patchset was tested on MT8183 evaluation board, and boot to shell ok.
>>>
>>> This series contains document bindings, device tree including interrupt, uart.
>>>
>>> Change in v3:
>>> 1. Fill out GICC, GICH, GICV regions
>>> 2. Update Copyright to 2018
>>>
>>> Change in v2:
>>> 1. Split dt-bindings into different patches
>>> 2. Correct bindings for supported SoCs (mtk-uart.txt)
>>>
>>> Ben Ho (1):
>>> arm64: dts: Add Mediatek SoC MT8183 and evaluation board dts and
>>> Makefile
>>>
>>> Erin Lo (3):
>>> dt-bindings: arm: Add bindings for Mediatek MT8183 SoC Platform
>>> dt-bindings: mtk-sysirq: Add compatible for Mediatek MT8183
>>> dt-bindings: serial: Add compatible for Mediatek MT8183
>>>
>>
>> I'm a bit reluctant to take this series, as it will only enable the EVB board to
>> boot into a serial console. Are you planning to add support for other devices of
>> this SoC?
>>
>> Apart please take into account that there is an issue with the dts file, as you
>> were told by the kbuild test robot.
>>
>> Regards,
>> Matthias
>>
>
> Hi, Matthias
> Sorry for missing this letter...since mail proxy server.
> We plan to add support all the devices of MT8183 in serious.
> We have implemented the clock and pinctrl driver for upstream and they
> are in internal review right now.
>
Nice to hear that :)
> About the dts issue... do you suggest me to send new patch right now or
> wait for clock and pinctrl driver ready then send them together?
>
I would prefer that you send at least the clock controller together, so that we
don't have any dummy clocks in the basic device tree.
Regards,
Matthias
^ permalink raw reply
* Re: [PATCH] serial: 8250_ingenic: Add support for the JZ4725B SoC
From: Rob Herring @ 2018-07-20 15:30 UTC (permalink / raw)
To: Paul Cercueil
Cc: Greg Kroah-Hartman, Mark Rutland, Jiri Slaby, linux-serial,
devicetree, linux-kernel
In-Reply-To: <20180713143840.9721-1-paul@crapouillou.net>
On Fri, Jul 13, 2018 at 04:38:40PM +0200, Paul Cercueil wrote:
> The UART in the jz4725b works just like in the other JZ SoCs, so this
> commit simply adds a new compatible string.
>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
> Documentation/devicetree/bindings/serial/ingenic,uart.txt | 1 +
> drivers/tty/serial/8250/8250_ingenic.c | 5 +++++
> 2 files changed, 6 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/serial/ingenic,uart.txt b/Documentation/devicetree/bindings/serial/ingenic,uart.txt
> index c3c6406d5cfe..a0b34fd3b602 100644
> --- a/Documentation/devicetree/bindings/serial/ingenic,uart.txt
> +++ b/Documentation/devicetree/bindings/serial/ingenic,uart.txt
> @@ -3,6 +3,7 @@
> Required properties:
> - compatible : One of:
> - "ingenic,jz4740-uart",
> + - "ingenic,jz4725b-uart",
> - "ingenic,jz4760-uart",
> - "ingenic,jz4770-uart",
> - "ingenic,jz4775-uart",
> diff --git a/drivers/tty/serial/8250/8250_ingenic.c b/drivers/tty/serial/8250/8250_ingenic.c
> index 15a8c8dfa92b..760266559a5a 100644
> --- a/drivers/tty/serial/8250/8250_ingenic.c
> +++ b/drivers/tty/serial/8250/8250_ingenic.c
> @@ -133,6 +133,10 @@ EARLYCON_DECLARE(jz4740_uart, ingenic_early_console_setup);
> OF_EARLYCON_DECLARE(jz4740_uart, "ingenic,jz4740-uart",
> ingenic_early_console_setup);
>
> +EARLYCON_DECLARE(jz4725b_uart, ingenic_early_console_setup);
> +OF_EARLYCON_DECLARE(jz4725b_uart, "ingenic,jz4725b-uart",
> + ingenic_early_console_setup);
> +
This shouldn't be necessary. Looks like it should be compatible with
ingenic,jz4740-uart.
> EARLYCON_DECLARE(jz4770_uart, ingenic_early_console_setup);
> OF_EARLYCON_DECLARE(jz4770_uart, "ingenic,jz4770-uart",
> ingenic_early_console_setup);
> @@ -330,6 +334,7 @@ static const struct ingenic_uart_config jz4780_uart_config = {
>
> static const struct of_device_id of_match[] = {
> { .compatible = "ingenic,jz4740-uart", .data = &jz4740_uart_config },
> + { .compatible = "ingenic,jz4725b-uart", .data = &jz4740_uart_config },
And this too.
> { .compatible = "ingenic,jz4760-uart", .data = &jz4760_uart_config },
> { .compatible = "ingenic,jz4770-uart", .data = &jz4760_uart_config },
> { .compatible = "ingenic,jz4775-uart", .data = &jz4760_uart_config },
> --
> 2.11.0
>
^ permalink raw reply
* Re: [PATCH v5 01/11] dt-bindings: clock: mediatek: document clk bindings for Mediatek MT6765 SoC
From: Rob Herring @ 2018-07-20 17:43 UTC (permalink / raw)
To: Mars Cheng
Cc: Matthias Brugger, Marc Zyngier, Ryder Lee, Stephen Boyd,
Sean Wang, CC Hwang, Loda Chou, linux-kernel, linux-mediatek,
devicetree, wsd_upstream, linux-serial, linux-arm-kernel,
linux-clk, Owen Chen
In-Reply-To: <1531817552-17221-2-git-send-email-mars.cheng@mediatek.com>
On Tue, Jul 17, 2018 at 04:52:22PM +0800, Mars Cheng wrote:
> This patch adds the binding documentation for apmixedsys, audsys, camsys,
> imgsys, infracfg, mipi0a, topckgen, vcodecsys
>
> Signed-off-by: Mars Cheng <mars.cheng@mediatek.com>
> Signed-off-by: Owen Chen <owen.chen@mediatek.com>
> ---
> .../bindings/arm/mediatek/mediatek,apmixedsys.txt | 1 +
> .../bindings/arm/mediatek/mediatek,audsys.txt | 1 +
> .../bindings/arm/mediatek/mediatek,camsys.txt | 27 ++++++++++++++++++++
> .../bindings/arm/mediatek/mediatek,imgsys.txt | 1 +
> .../bindings/arm/mediatek/mediatek,infracfg.txt | 1 +
> .../bindings/arm/mediatek/mediatek,mipi0a.txt | 23 +++++++++++++++++
> .../bindings/arm/mediatek/mediatek,mmsys.txt | 1 +
> .../bindings/arm/mediatek/mediatek,pericfg.txt | 1 +
> .../bindings/arm/mediatek/mediatek,topckgen.txt | 1 +
> .../bindings/arm/mediatek/mediatek,vcodecsys.txt | 22 ++++++++++++++++
> 10 files changed, 79 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/arm/mediatek/mediatek,camsys.txt
> create mode 100644 Documentation/devicetree/bindings/arm/mediatek/mediatek,mipi0a.txt
> create mode 100644 Documentation/devicetree/bindings/arm/mediatek/mediatek,vcodecsys.txt
>
> diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,apmixedsys.txt b/Documentation/devicetree/bindings/arm/mediatek/mediatek,apmixedsys.txt
> index b404d59..44eaeac 100644
> --- a/Documentation/devicetree/bindings/arm/mediatek/mediatek,apmixedsys.txt
> +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,apmixedsys.txt
> @@ -8,6 +8,7 @@ Required Properties:
> - compatible: Should be one of:
> - "mediatek,mt2701-apmixedsys"
> - "mediatek,mt2712-apmixedsys", "syscon"
> + - "mediatek,mt6765-apmixedsys", "syscon"
> - "mediatek,mt6797-apmixedsys"
> - "mediatek,mt7622-apmixedsys"
> - "mediatek,mt8135-apmixedsys"
> diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,audsys.txt b/Documentation/devicetree/bindings/arm/mediatek/mediatek,audsys.txt
> index 34a69ba..9a8672a 100644
> --- a/Documentation/devicetree/bindings/arm/mediatek/mediatek,audsys.txt
> +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,audsys.txt
> @@ -7,6 +7,7 @@ Required Properties:
>
> - compatible: Should be one of:
> - "mediatek,mt2701-audsys", "syscon"
> + - "mediatek,mt6765-audsys", "syscon"
> - "mediatek,mt7622-audsys", "syscon"
> - #clock-cells: Must be 1
>
> diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,camsys.txt b/Documentation/devicetree/bindings/arm/mediatek/mediatek,camsys.txt
> new file mode 100644
> index 0000000..dc75783
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,camsys.txt
> @@ -0,0 +1,27 @@
> +MediaTek CAMSYS controller
> +============================
> +
> +The MediaTek CAMSYS controller provides various clocks to the system.
Only clocks? If so, then this should be moved to bindings/clocks/.
> +
> +Required Properties:
> +
> +- compatible: Should be one of:
> + - "mediatek,mt6765-camsys", "syscon"
> +- #clock-cells: Must be 1
> +
> +The AUDSYS controller uses the common clk binding from
> +Documentation/devicetree/bindings/clock/clock-bindings.txt
> +The available clocks are defined in dt-bindings/clock/mt*-clk.h.
> +
> +Required sub-nodes:
> +-------
> +For common binding part and usage, refer to
> +../sonud/mt2701-afe-pcm.txt.
> +
> +Example:
> +
> +camsys: camsys@1a000000 {
clock-controller@...
if the above answer is yes.
Same comments on the other docs.
^ permalink raw reply
* Re: [PATCH v5 02/11] dt-bindings: mediatek: Add smi dts binding for Mediatek MT6765 SoC
From: Rob Herring @ 2018-07-20 17:44 UTC (permalink / raw)
To: Mars Cheng
Cc: Matthias Brugger, Marc Zyngier, Ryder Lee, Stephen Boyd,
Sean Wang, CC Hwang, Loda Chou, linux-kernel, linux-mediatek,
devicetree, wsd_upstream, linux-serial, linux-arm-kernel,
linux-clk, Owen Chen
In-Reply-To: <1531817552-17221-3-git-send-email-mars.cheng@mediatek.com>
On Tue, Jul 17, 2018 at 04:52:23PM +0800, Mars Cheng wrote:
> This patch adds MT6765 smi binding document
>
> Signed-off-by: Mars Cheng <mars.cheng@mediatek.com>
> Signed-off-by: Owen Chen <owen.chen@mediatek.com>
> ---
> .../memory-controllers/mediatek,smi-common.txt | 1 +
> 1 file changed, 1 insertion(+)
Acked-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* Re: [PATCH v5 03/11] dt-bindings: mediatek: add MT6765 power dt-bindings
From: Rob Herring @ 2018-07-20 17:45 UTC (permalink / raw)
To: Mars Cheng
Cc: Matthias Brugger, Marc Zyngier, Ryder Lee, Stephen Boyd,
Sean Wang, CC Hwang, Loda Chou, linux-kernel, linux-mediatek,
devicetree, wsd_upstream, linux-serial, linux-arm-kernel,
linux-clk, Owen Chen
In-Reply-To: <1531817552-17221-4-git-send-email-mars.cheng@mediatek.com>
On Tue, Jul 17, 2018 at 04:52:24PM +0800, Mars Cheng wrote:
> This adds power dt-bindings for MT6765
>
> Signed-off-by: Mars Cheng <mars.cheng@mediatek.com>
> Signed-off-by: Owen Chen <owen.chen@mediatek.com>
> ---
> .../devicetree/bindings/soc/mediatek/scpsys.txt | 6 ++++++
> 1 file changed, 6 insertions(+)
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* Re: [PATCH v5 04/11] soc: mediatek: add MT6765 scpsys support
From: Rob Herring @ 2018-07-20 17:46 UTC (permalink / raw)
To: Mars Cheng
Cc: Matthias Brugger, Marc Zyngier, Ryder Lee, Stephen Boyd,
Sean Wang, CC Hwang, Loda Chou, linux-kernel, linux-mediatek,
devicetree, wsd_upstream, linux-serial, linux-arm-kernel,
linux-clk, Owen Chen
In-Reply-To: <1531817552-17221-5-git-send-email-mars.cheng@mediatek.com>
On Tue, Jul 17, 2018 at 04:52:25PM +0800, Mars Cheng wrote:
> This adds scpsys support for MT6765
>
> Signed-off-by: Mars Cheng <mars.cheng@mediatek.com>
> Signed-off-by: Owen Chen <owen.chen@mediatek.com>
> ---
> drivers/soc/mediatek/mtk-scpsys.c | 88 ++++++++++++++++++++++++++++++
> include/dt-bindings/power/mt6765-power.h | 14 +++++
This file goes in the binding patch.
> 2 files changed, 102 insertions(+)
> create mode 100644 include/dt-bindings/power/mt6765-power.h
^ permalink raw reply
* Re: [PATCH] serial: 8250_ingenic: Add support for the JZ4725B SoC
From: Paul Cercueil @ 2018-07-20 18:58 UTC (permalink / raw)
To: Rob Herring
Cc: Greg Kroah-Hartman, Mark Rutland, Jiri Slaby, linux-serial,
devicetree, linux-kernel
In-Reply-To: <20180720153027.GA11978@rob-hp-laptop>
Hi Rob,
Le ven. 20 juil. 2018 à 17:30, Rob Herring <robh@kernel.org> a écrit :
> On Fri, Jul 13, 2018 at 04:38:40PM +0200, Paul Cercueil wrote:
>> The UART in the jz4725b works just like in the other JZ SoCs, so
>> this
>> commit simply adds a new compatible string.
>>
>> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>> ---
>> Documentation/devicetree/bindings/serial/ingenic,uart.txt | 1 +
>> drivers/tty/serial/8250/8250_ingenic.c | 5 +++++
>> 2 files changed, 6 insertions(+)
>>
>> diff --git
>> a/Documentation/devicetree/bindings/serial/ingenic,uart.txt
>> b/Documentation/devicetree/bindings/serial/ingenic,uart.txt
>> index c3c6406d5cfe..a0b34fd3b602 100644
>> --- a/Documentation/devicetree/bindings/serial/ingenic,uart.txt
>> +++ b/Documentation/devicetree/bindings/serial/ingenic,uart.txt
>> @@ -3,6 +3,7 @@
>> Required properties:
>> - compatible : One of:
>> - "ingenic,jz4740-uart",
>> + - "ingenic,jz4725b-uart",
>> - "ingenic,jz4760-uart",
>> - "ingenic,jz4770-uart",
>> - "ingenic,jz4775-uart",
>> diff --git a/drivers/tty/serial/8250/8250_ingenic.c
>> b/drivers/tty/serial/8250/8250_ingenic.c
>> index 15a8c8dfa92b..760266559a5a 100644
>> --- a/drivers/tty/serial/8250/8250_ingenic.c
>> +++ b/drivers/tty/serial/8250/8250_ingenic.c
>> @@ -133,6 +133,10 @@ EARLYCON_DECLARE(jz4740_uart,
>> ingenic_early_console_setup);
>> OF_EARLYCON_DECLARE(jz4740_uart, "ingenic,jz4740-uart",
>> ingenic_early_console_setup);
>>
>> +EARLYCON_DECLARE(jz4725b_uart, ingenic_early_console_setup);
>> +OF_EARLYCON_DECLARE(jz4725b_uart, "ingenic,jz4725b-uart",
>> + ingenic_early_console_setup);
>> +
>
> This shouldn't be necessary. Looks like it should be compatible with
> ingenic,jz4740-uart.
>
>> EARLYCON_DECLARE(jz4770_uart, ingenic_early_console_setup);
>> OF_EARLYCON_DECLARE(jz4770_uart, "ingenic,jz4770-uart",
>> ingenic_early_console_setup);
>> @@ -330,6 +334,7 @@ static const struct ingenic_uart_config
>> jz4780_uart_config = {
>>
>> static const struct of_device_id of_match[] = {
>> { .compatible = "ingenic,jz4740-uart", .data =
>> &jz4740_uart_config },
>> + { .compatible = "ingenic,jz4725b-uart", .data =
>> &jz4740_uart_config },
>
> And this too.
Well, I'm confused, the driver already uses multiple compatible strings
for SoCs that
work the exact same, so that was wrong?
>> { .compatible = "ingenic,jz4760-uart", .data =
>> &jz4760_uart_config },
>> { .compatible = "ingenic,jz4770-uart", .data =
>> &jz4760_uart_config },
>> { .compatible = "ingenic,jz4775-uart", .data =
>> &jz4760_uart_config },
>> --
>> 2.11.0
>>
^ permalink raw reply
* Re: [PATCH v3 0/4] Add basic support for Mediatek MT8183 SoC
From: Erin Lo @ 2018-07-21 7:28 UTC (permalink / raw)
To: Matthias Brugger
Cc: Mark Rutland, devicetree, Jason Cooper, srv_heupstream,
Marc Zyngier, Greg Kroah-Hartman, linux-mediatek, linux-kernel,
Rob Herring, mars.cheng, linux-serial, yingjoe.chen,
Thomas Gleixner, linux-arm-kernel
In-Reply-To: <2ca76468-198b-b764-c974-374e567e0c93@gmail.com>
On Fri, 2018-07-20 at 12:44 +0200, Matthias Brugger wrote:
>
> On 20/07/18 10:19, Erin Lo wrote:
> > On Mon, 2018-07-16 at 11:28 +0200, Matthias Brugger wrote:
> >> Hi Erin,
> >>
> >> On 17/05/18 08:22, Erin Lo wrote:
> >>> MT8183 is a SoC based on 64bit ARMv8 architecture.
> >>> It contains 4 CA53 and 4 CA73 cores.
> >>> MT8183 share many HW IP with MT65xx series.
> >>> This patchset was tested on MT8183 evaluation board, and boot to shell ok.
> >>>
> >>> This series contains document bindings, device tree including interrupt, uart.
> >>>
> >>> Change in v3:
> >>> 1. Fill out GICC, GICH, GICV regions
> >>> 2. Update Copyright to 2018
> >>>
> >>> Change in v2:
> >>> 1. Split dt-bindings into different patches
> >>> 2. Correct bindings for supported SoCs (mtk-uart.txt)
> >>>
> >>> Ben Ho (1):
> >>> arm64: dts: Add Mediatek SoC MT8183 and evaluation board dts and
> >>> Makefile
> >>>
> >>> Erin Lo (3):
> >>> dt-bindings: arm: Add bindings for Mediatek MT8183 SoC Platform
> >>> dt-bindings: mtk-sysirq: Add compatible for Mediatek MT8183
> >>> dt-bindings: serial: Add compatible for Mediatek MT8183
> >>>
> >>
> >> I'm a bit reluctant to take this series, as it will only enable the EVB board to
> >> boot into a serial console. Are you planning to add support for other devices of
> >> this SoC?
> >>
> >> Apart please take into account that there is an issue with the dts file, as you
> >> were told by the kbuild test robot.
> >>
> >> Regards,
> >> Matthias
> >>
> >
> > Hi, Matthias
> > Sorry for missing this letter...since mail proxy server.
> > We plan to add support all the devices of MT8183 in serious.
> > We have implemented the clock and pinctrl driver for upstream and they
> > are in internal review right now.
> >
>
> Nice to hear that :)
>
> > About the dts issue... do you suggest me to send new patch right now or
> > wait for clock and pinctrl driver ready then send them together?
> >
>
> I would prefer that you send at least the clock controller together, so that we
> don't have any dummy clocks in the basic device tree.
>
> Regards,
> Matthias
>
Got it! Next patch, we will send them with clock controller without
dummy clocks in the basic device tree.
By the way, the clock controller driver of MT8183 is a little bit
different from former ICs, so we need more time to prepare them.
We will send them to public as soon as possible.
Best Regards,
Erin
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply
* Re: [PATCH v3 0/4] Add basic support for Mediatek MT8183 SoC
From: Matthias Brugger @ 2018-07-22 21:11 UTC (permalink / raw)
To: Erin Lo
Cc: Mark Rutland, devicetree, Jason Cooper, srv_heupstream,
Marc Zyngier, Greg Kroah-Hartman, linux-kernel, Rob Herring,
mars.cheng, linux-serial, yingjoe.chen, linux-mediatek,
Thomas Gleixner, linux-arm-kernel
In-Reply-To: <1532158134.16009.10.camel@mtksdaap41>
On 21/07/18 09:28, Erin Lo wrote:
> On Fri, 2018-07-20 at 12:44 +0200, Matthias Brugger wrote:
>>
>> On 20/07/18 10:19, Erin Lo wrote:
>>> On Mon, 2018-07-16 at 11:28 +0200, Matthias Brugger wrote:
>>>> Hi Erin,
>>>>
>>>> On 17/05/18 08:22, Erin Lo wrote:
>>>>> MT8183 is a SoC based on 64bit ARMv8 architecture.
>>>>> It contains 4 CA53 and 4 CA73 cores.
>>>>> MT8183 share many HW IP with MT65xx series.
>>>>> This patchset was tested on MT8183 evaluation board, and boot to shell ok.
>>>>>
>>>>> This series contains document bindings, device tree including interrupt, uart.
>>>>>
>>>>> Change in v3:
>>>>> 1. Fill out GICC, GICH, GICV regions
>>>>> 2. Update Copyright to 2018
>>>>>
>>>>> Change in v2:
>>>>> 1. Split dt-bindings into different patches
>>>>> 2. Correct bindings for supported SoCs (mtk-uart.txt)
>>>>>
>>>>> Ben Ho (1):
>>>>> arm64: dts: Add Mediatek SoC MT8183 and evaluation board dts and
>>>>> Makefile
>>>>>
>>>>> Erin Lo (3):
>>>>> dt-bindings: arm: Add bindings for Mediatek MT8183 SoC Platform
>>>>> dt-bindings: mtk-sysirq: Add compatible for Mediatek MT8183
>>>>> dt-bindings: serial: Add compatible for Mediatek MT8183
>>>>>
>>>>
>>>> I'm a bit reluctant to take this series, as it will only enable the EVB board to
>>>> boot into a serial console. Are you planning to add support for other devices of
>>>> this SoC?
>>>>
>>>> Apart please take into account that there is an issue with the dts file, as you
>>>> were told by the kbuild test robot.
>>>>
>>>> Regards,
>>>> Matthias
>>>>
>>>
>>> Hi, Matthias
>>> Sorry for missing this letter...since mail proxy server.
>>> We plan to add support all the devices of MT8183 in serious.
>>> We have implemented the clock and pinctrl driver for upstream and they
>>> are in internal review right now.
>>>
>>
>> Nice to hear that :)
>>
>>> About the dts issue... do you suggest me to send new patch right now or
>>> wait for clock and pinctrl driver ready then send them together?
>>>
>>
>> I would prefer that you send at least the clock controller together, so that we
>> don't have any dummy clocks in the basic device tree.
>>
>> Regards,
>> Matthias
>>
>
> Got it! Next patch, we will send them with clock controller without
> dummy clocks in the basic device tree.
> By the way, the clock controller driver of MT8183 is a little bit
> different from former ICs, so we need more time to prepare them.
> We will send them to public as soon as possible.
>
Thanks for the info.
Best regards,
Matthias
^ permalink raw reply
* Re: [PATCH] serial: 8250_ingenic: Add support for the JZ4725B SoC
From: Rob Herring @ 2018-07-23 14:22 UTC (permalink / raw)
To: Paul Cercueil
Cc: Greg Kroah-Hartman, Mark Rutland, Jiri Slaby,
open list:SERIAL DRIVERS, devicetree,
linux-kernel@vger.kernel.org
In-Reply-To: <1532113088.2309.0@smtp.crapouillou.net>
On Fri, Jul 20, 2018 at 12:58 PM Paul Cercueil <paul@crapouillou.net> wrote:
>
> Hi Rob,
>
> Le ven. 20 juil. 2018 à 17:30, Rob Herring <robh@kernel.org> a écrit :
> > On Fri, Jul 13, 2018 at 04:38:40PM +0200, Paul Cercueil wrote:
> >> The UART in the jz4725b works just like in the other JZ SoCs, so
> >> this
> >> commit simply adds a new compatible string.
> >>
> >> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> >> ---
> >> Documentation/devicetree/bindings/serial/ingenic,uart.txt | 1 +
> >> drivers/tty/serial/8250/8250_ingenic.c | 5 +++++
> >> 2 files changed, 6 insertions(+)
> >>
> >> diff --git
> >> a/Documentation/devicetree/bindings/serial/ingenic,uart.txt
> >> b/Documentation/devicetree/bindings/serial/ingenic,uart.txt
> >> index c3c6406d5cfe..a0b34fd3b602 100644
> >> --- a/Documentation/devicetree/bindings/serial/ingenic,uart.txt
> >> +++ b/Documentation/devicetree/bindings/serial/ingenic,uart.txt
> >> @@ -3,6 +3,7 @@
> >> Required properties:
> >> - compatible : One of:
> >> - "ingenic,jz4740-uart",
> >> + - "ingenic,jz4725b-uart",
> >> - "ingenic,jz4760-uart",
> >> - "ingenic,jz4770-uart",
> >> - "ingenic,jz4775-uart",
> >> diff --git a/drivers/tty/serial/8250/8250_ingenic.c
> >> b/drivers/tty/serial/8250/8250_ingenic.c
> >> index 15a8c8dfa92b..760266559a5a 100644
> >> --- a/drivers/tty/serial/8250/8250_ingenic.c
> >> +++ b/drivers/tty/serial/8250/8250_ingenic.c
> >> @@ -133,6 +133,10 @@ EARLYCON_DECLARE(jz4740_uart,
> >> ingenic_early_console_setup);
> >> OF_EARLYCON_DECLARE(jz4740_uart, "ingenic,jz4740-uart",
> >> ingenic_early_console_setup);
> >>
> >> +EARLYCON_DECLARE(jz4725b_uart, ingenic_early_console_setup);
> >> +OF_EARLYCON_DECLARE(jz4725b_uart, "ingenic,jz4725b-uart",
> >> + ingenic_early_console_setup);
> >> +
> >
> > This shouldn't be necessary. Looks like it should be compatible with
> > ingenic,jz4740-uart.
> >
> >> EARLYCON_DECLARE(jz4770_uart, ingenic_early_console_setup);
> >> OF_EARLYCON_DECLARE(jz4770_uart, "ingenic,jz4770-uart",
> >> ingenic_early_console_setup);
> >> @@ -330,6 +334,7 @@ static const struct ingenic_uart_config
> >> jz4780_uart_config = {
> >>
> >> static const struct of_device_id of_match[] = {
> >> { .compatible = "ingenic,jz4740-uart", .data =
> >> &jz4740_uart_config },
> >> + { .compatible = "ingenic,jz4725b-uart", .data =
> >> &jz4740_uart_config },
> >
> > And this too.
>
> Well, I'm confused, the driver already uses multiple compatible strings
> for SoCs that
> work the exact same, so that was wrong?
Not wrong, but not necessary. Given that the data was the same was the
clue telling me that they should be compatible.
Rob
^ permalink raw reply
* Re: [PATCH v5 06/11] soc: mediatek: add new flow for mtcmos power.
From: Owen Chen @ 2018-07-25 9:42 UTC (permalink / raw)
To: Matthias Brugger
Cc: Mars Cheng, Rob Herring, Marc Zyngier, Ryder Lee, Stephen Boyd,
Sean Wang, CC Hwang, Loda Chou, linux-kernel, linux-mediatek,
devicetree, wsd_upstream, linux-serial, linux-arm-kernel,
linux-clk
In-Reply-To: <261612ef-cc32-3eaf-442c-5d61dc279a08@gmail.com>
Hi Matthias
On Wed, 2018-07-18 at 16:50 +0200, Matthias Brugger wrote:
>
> On 17/07/18 10:52, Mars Cheng wrote:
> > From: Owen Chen <owen.chen@mediatek.com>
> >
> > MT6765 need multiple register and actions to setup bus
> > protect.
> > 1. turn on subsys CG before release bus protect to receive
> > ack.
> > 2. turn off subsys CG after set bus protect and receive
> > ack.
> > 3. bus protect need not only infracfg but other domain
> > register to setup. Therefore we add a set/clr APIs
> > with more customize arguments.
> >
> > Signed-off-by: Owen Chen <owen.chen@mediatek.com>
> > Signed-off-by: Mars Cheng <mars.cheng@mediatek.com>
> > ---
> > drivers/soc/mediatek/Makefile | 2 +-
> > drivers/soc/mediatek/mtk-infracfg.c | 178 +++++++++++---
> > drivers/soc/mediatek/mtk-scpsys-ext.c | 405 +++++++++++++++++++++++++++++++
> > drivers/soc/mediatek/mtk-scpsys.c | 147 +++++++++--
> > include/linux/soc/mediatek/infracfg.h | 9 +-
> > include/linux/soc/mediatek/scpsys-ext.h | 66 +++++
> > 6 files changed, 745 insertions(+), 62 deletions(-)
> > create mode 100644 drivers/soc/mediatek/mtk-scpsys-ext.c
> > create mode 100644 include/linux/soc/mediatek/scpsys-ext.h
> >
> > diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile
> > index 12998b0..9dc6670 100644
> > --- a/drivers/soc/mediatek/Makefile
> > +++ b/drivers/soc/mediatek/Makefile
> > @@ -1,3 +1,3 @@
> > -obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o
> > +obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o mtk-scpsys-ext.o
> > obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o
> > obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys.o
> > diff --git a/drivers/soc/mediatek/mtk-infracfg.c b/drivers/soc/mediatek/mtk-infracfg.c
> > index 958861c..11eadf8 100644
> > --- a/drivers/soc/mediatek/mtk-infracfg.c
> > +++ b/drivers/soc/mediatek/mtk-infracfg.c
> > @@ -1,3 +1,4 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > /*
> > * Copyright (c) 2015 Pengutronix, Sascha Hauer <kernel@pengutronix.de>
> > *
> > @@ -15,6 +16,7 @@
> > #include <linux/jiffies.h>
> > #include <linux/regmap.h>
> > #include <linux/soc/mediatek/infracfg.h>
> > +#include <linux/soc/mediatek/scpsys-ext.h>
> > #include <asm/processor.h>
> >
> > #define MTK_POLL_DELAY_US 10
> > @@ -26,62 +28,176 @@
> > #define INFRA_TOPAXI_PROTECTEN_CLR 0x0264
> >
> > /**
> > - * mtk_infracfg_set_bus_protection - enable bus protection
> > - * @regmap: The infracfg regmap
> > - * @mask: The mask containing the protection bits to be enabled.
> > - * @reg_update: The boolean flag determines to set the protection bits
> > - * by regmap_update_bits with enable register(PROTECTEN) or
> > - * by regmap_write with set register(PROTECTEN_SET).
> > + * mtk_generic_set_cmd - enable bus protection with set register
> > + * @regmap: The bus protect regmap
> > + * @set_ofs: The set register offset to set corresponding bit to 1.
> > + * @sta_ofs: The status register offset to show bus protect enable/disable.
> > + * @mask: The mask containing the protection bits to be disabled.
> > *
> > * This function enables the bus protection bits for disabled power
> > * domains so that the system does not hang when some unit accesses the
> > * bus while in power down.
> > */
> > -int mtk_infracfg_set_bus_protection(struct regmap *infracfg, u32 mask,
> > - bool reg_update)
> > +int mtk_generic_set_cmd(struct regmap *regmap, u32 set_ofs,
> > + u32 sta_ofs, u32 mask)
> > {
> > u32 val;
> > int ret;
> >
> > - if (reg_update)
> > - regmap_update_bits(infracfg, INFRA_TOPAXI_PROTECTEN, mask,
> > - mask);
> > - else
> > - regmap_write(infracfg, INFRA_TOPAXI_PROTECTEN_SET, mask);
> > + regmap_write(regmap, set_ofs, mask);
> >
> > - ret = regmap_read_poll_timeout(infracfg, INFRA_TOPAXI_PROTECTSTA1,
> > - val, (val & mask) == mask,
> > - MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
> > + ret = regmap_read_poll_timeout(regmap, sta_ofs, val,
> > + (val & mask) == mask,
> > + MTK_POLL_DELAY_US,
> > + MTK_POLL_TIMEOUT);
> >
> > return ret;
> > }
> >
> > /**
> > - * mtk_infracfg_clear_bus_protection - disable bus protection
> > - * @regmap: The infracfg regmap
> > + * mtk_generic_clr_cmd - disable bus protection with clr register
> > + * @regmap: The bus protect regmap
> > + * @clr_ofs: The clr register offset to clear corresponding bit to 0.
> > + * @sta_ofs: The status register offset to show bus protect enable/disable.
> > * @mask: The mask containing the protection bits to be disabled.
> > - * @reg_update: The boolean flag determines to clear the protection bits
> > - * by regmap_update_bits with enable register(PROTECTEN) or
> > - * by regmap_write with clear register(PROTECTEN_CLR).
> > *
> > * This function disables the bus protection bits previously enabled with
> > - * mtk_infracfg_set_bus_protection.
> > + * mtk_set_bus_protection.
> > */
> >
> > -int mtk_infracfg_clear_bus_protection(struct regmap *infracfg, u32 mask,
> > - bool reg_update)
> > +int mtk_generic_clr_cmd(struct regmap *regmap, u32 clr_ofs,
> > + u32 sta_ofs, u32 mask)
> > {
> > int ret;
> > u32 val;
> >
> > - if (reg_update)
> > - regmap_update_bits(infracfg, INFRA_TOPAXI_PROTECTEN, mask, 0);
> > - else
> > - regmap_write(infracfg, INFRA_TOPAXI_PROTECTEN_CLR, mask);
> > + regmap_write(regmap, clr_ofs, mask);
> >
> > - ret = regmap_read_poll_timeout(infracfg, INFRA_TOPAXI_PROTECTSTA1,
> > - val, !(val & mask),
> > - MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
> > + ret = regmap_read_poll_timeout(regmap, sta_ofs, val,
> > + !(val & mask),
> > + MTK_POLL_DELAY_US,
> > + MTK_POLL_TIMEOUT);
> > + return ret;
> > +}
> > +
> > +/**
> > + * mtk_generic_enable_cmd - enable bus protection with upd register
> > + * @regmap: The bus protect regmap
> > + * @upd_ofs: The update register offset to directly rewrite value to
> > + * corresponding bit.
> > + * @sta_ofs: The status register offset to show bus protect enable/disable.
> > + * @mask: The mask containing the protection bits to be disabled.
> > + *
> > + * This function enables the bus protection bits for disabled power
> > + * domains so that the system does not hang when some unit accesses the
> > + * bus while in power down.
> > + */
> > +int mtk_generic_enable_cmd(struct regmap *regmap, u32 upd_ofs,
> > + u32 sta_ofs, u32 mask)
> > +{
> > + u32 val;
> > + int ret;
> > +
> > + regmap_update_bits(regmap, upd_ofs, mask, mask);
> >
> > + ret = regmap_read_poll_timeout(regmap, sta_ofs, val,
> > + (val & mask) == mask,
> > + MTK_POLL_DELAY_US,
> > + MTK_POLL_TIMEOUT);
> > return ret;
> > }
> > +
> > +/**
> > + * mtk_generic_disable_cmd - disable bus protection with updd register
> > + * @regmap: The bus protect regmap
> > + * @upd_ofs: The update register offset to directly rewrite value to
> > + * corresponding bit.
> > + * @sta_ofs: The status register offset to show bus protect enable/disable.
> > + * @mask: The mask containing the protection bits to be disabled.
> > + *
> > + * This function disables the bus protection bits previously enabled with
> > + * mtk_set_bus_protection.
> > + */
> > +
> > +int mtk_generic_disable_cmd(struct regmap *regmap, u32 upd_ofs,
> > + u32 sta_ofs, u32 mask)
> > +{
> > + int ret;
> > + u32 val;
> > +
> > + regmap_update_bits(regmap, upd_ofs, mask, 0);
> > +
> > + ret = regmap_read_poll_timeout(regmap, sta_ofs,
> > + val, !(val & mask),
> > + MTK_POLL_DELAY_US,
> > + MTK_POLL_TIMEOUT);
> > + return ret;
> > +}
> > +
> > +/**
> > + * mtk_set_bus_protection - enable bus protection
> > + * @infracfg: The bus protect regmap, default use infracfg
> > + * @mask: The mask containing the protection bits to be enabled.
> > + *
> > + * This function enables the bus protection bits for disabled power
> > + * domains so that the system does not hang when some unit accesses the
> > + * bus while in power down.
> > + */
> > +int mtk_infracfg_set_bus_protection(struct regmap *infracfg, u32 mask)
> > +{
> > + return mtk_generic_set_cmd(infracfg,
> > + INFRA_TOPAXI_PROTECTEN_SET,
> > + INFRA_TOPAXI_PROTECTSTA1,
> > + mask);
> > +}
> > +
> > +/**
> > + * mtk_clear_bus_protection - disable bus protection
> > + * @infracfg: The bus protect regmap, default use infracfg
> > + * @mask: The mask containing the protection bits to be disabled.
> > + *
> > + * This function disables the bus protection bits previously enabled with
> > + * mtk_set_bus_protection.
> > + */
> > +
> > +int mtk_infracfg_clear_bus_protection(struct regmap *infracfg, u32 mask)
> > +{
> > + return mtk_generic_clr_cmd(infracfg,
> > + INFRA_TOPAXI_PROTECTEN_CLR,
> > + INFRA_TOPAXI_PROTECTSTA1,
> > + mask);
> > +}
> > +
> > +/**
> > + * mtk_infracfg_enable_bus_protection - enable bus protection
> > + * @infracfg: The bus protect regmap, default use infracfg
> > + * @mask: The mask containing the protection bits to be disabled.
> > + *
> > + * This function enables the bus protection bits for disabled power
> > + * domains so that the system does not hang when some unit accesses the
> > + * bus while in power down.
> > + */
> > +int mtk_infracfg_enable_bus_protection(struct regmap *infracfg, u32 mask)
> > +{
> > + return mtk_generic_enable_cmd(infracfg,
> > + INFRA_TOPAXI_PROTECTEN,
> > + INFRA_TOPAXI_PROTECTSTA1,
> > + mask);
> > +}
> > +
> > +/**
> > + * mtk_infracfg_disable_bus_protection - disable bus protection
> > + * @infracfg: The bus protect regmap, default use infracfg
> > + * @mask: The mask containing the protection bits to be disabled.
> > + *
> > + * This function disables the bus protection bits previously enabled with
> > + * mtk_infracfg_set_bus_protection.
> > + */
> > +
> > +int mtk_infracfg_disable_bus_protection(struct regmap *infracfg, u32 mask)
> > +{
> > + return mtk_generic_disable_cmd(infracfg,
> > + INFRA_TOPAXI_PROTECTEN,
> > + INFRA_TOPAXI_PROTECTSTA1,
> > + mask);
> > +}
> > diff --git a/drivers/soc/mediatek/mtk-scpsys-ext.c b/drivers/soc/mediatek/mtk-scpsys-ext.c
> > new file mode 100644
> > index 0000000..965e64d
> > --- /dev/null
> > +++ b/drivers/soc/mediatek/mtk-scpsys-ext.c
> > @@ -0,0 +1,405 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2018 MediaTek Inc.
> > + * Author: Owen Chen <owen.chen@mediatek.com>
> > + */
> > +#include <linux/clk.h>
> > +#include <linux/clk-provider.h>
> > +#include <linux/slab.h>
> > +#include <linux/export.h>
> > +#include <linux/mfd/syscon.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +#include <linux/soc/mediatek/infracfg.h>
> > +#include <linux/soc/mediatek/scpsys-ext.h>
> > +
> > +
> > +#define MAX_CLKS 10
> > +#define INFRA "infracfg"
> > +#define SMIC "smi_comm"
>
> Don't use defines for this. While at it I suppose it should be "smi_common"
OK. I will fix it.
>
> > +
> > +static LIST_HEAD(ext_clk_map_list);
> > +static LIST_HEAD(ext_attr_map_list);
> > +
> > +static struct regmap *infracfg;
> > +static struct regmap *smi_comm;
> > +
> > +enum regmap_type {
> > + IFR_TYPE,
> > + SMI_TYPE,
> > + MAX_REGMAP_TYPE,
> > +};
> > +
> > +/**
> > + * struct ext_reg_ctrl - set multiple register for bus protect
> > + * @regmap: The bus protect regmap, 1: infracfg, 2: other master regmap
> > + * such as SMI.
> > + * @set_ofs: The set register offset to set corresponding bit to 1.
> > + * @clr_ofs: The clr register offset to clear corresponding bit to 0.
> > + * @sta_ofs: The status register offset to show bus protect enable/disable.
> > + */
> > +struct ext_reg_ctrl {
> > + enum regmap_type type;
> > + u32 set_ofs;
> > + u32 clr_ofs;
> > + u32 sta_ofs;
> > +};
> > +
> > +/**
> > + * struct ext_clk_ctrl - enable multiple clks for bus protect
> > + * @clk: The clk need to enable before pwr on/bus protect.
> > + * @scpd_n: The name present the scpsys domain where the clks belongs to.
> > + * @clk_list: The list node linked to ext_clk_map_list.
> > + */
> > +struct ext_clk_ctrl {
> > + struct clk *clk;
> > + const char *scpd_n;
> > + struct list_head clk_list;
> > +};
> > +
> > +struct bus_mask_ops {
> > + int (*set)(struct regmap *regmap, u32 set_ofs,
> > + u32 sta_ofs, u32 mask);
> > + int (*release)(struct regmap *regmap, u32 clr_ofs,
> > + u32 sta_ofs, u32 mask);
> > +};
> > +
> > +static struct scpsys_ext_attr *__get_attr_parent(const char *parent_n)
> > +{
> > + struct scpsys_ext_attr *attr;
> > +
> > + if (!parent_n)
> > + return ERR_PTR(-EINVAL);
> > +
> > + list_for_each_entry(attr, &ext_attr_map_list, attr_list) {
> > + if (attr->scpd_n && !strcmp(parent_n, attr->scpd_n))
> > + return attr;
> > + }
> > +
> > + return ERR_PTR(-EINVAL);
> > +}
> > +
> > +int bus_ctrl_set_release(struct scpsys_ext_attr *attr, bool set)
> > +{
> > + int i;
> > + int ret = 0;
> > +
> > + for (i = 0; i < MAX_STEP_NUM && attr->mask[i].mask; i++) {
> > + struct ext_reg_ctrl *rc = attr->mask[i].regs;
> > + struct regmap *regmap;
> > +
> > + if (rc->type == IFR_TYPE)
> > + regmap = infracfg;
> > + else if (rc->type == SMI_TYPE)
> > + regmap = smi_comm;
> > + else
> > + return -EINVAL;
> > +
> > + if (set)
> > + ret = attr->mask[i].ops->set(regmap,
> > + rc->set_ofs,
> > + rc->sta_ofs,
> > + attr->mask[i].mask);
> > + else
> > + ret = attr->mask[i].ops->release(regmap,
> > + rc->clr_ofs,
> > + rc->sta_ofs,
> > + attr->mask[i].mask);
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +int bus_ctrl_set(struct scpsys_ext_attr *attr)
> > +{
> > + return bus_ctrl_set_release(attr, CMD_ENABLE);
> > +}
> > +
> > +int bus_ctrl_release(struct scpsys_ext_attr *attr)
> > +{
> > + return bus_ctrl_set_release(attr, CMD_DISABLE);
> > +}
> > +
> > +int bus_clk_enable_disable(struct scpsys_ext_attr *attr, bool enable)
> > +{
> > + int i = 0;
> > + int ret = 0;
> > + struct ext_clk_ctrl *cc;
> > + struct clk *clk[MAX_CLKS];
> > +
> > + list_for_each_entry(cc, &ext_clk_map_list, clk_list) {
>
> Why can't we handle this in the same way as we do in mtk-scpsys.c ?
We originally thought it would be better to put all the additional flow
at new created file, but after consider the readability of code flow, we
would put this cg operation back to mtk_scpsys.c.
>
> > + if (!strcmp(cc->scpd_n, attr->scpd_n)) {
> > + if (enable)
> > + ret = clk_prepare_enable(cc->clk);
> > + else
> > + clk_disable_unprepare(cc->clk);
> > +
> > + if (ret) {
> > + pr_err("Failed to %s %s\n",
> > + enable ? "enable" : "disable",
> > + __clk_get_name(cc->clk));
> > + goto err;
> > + } else {
> > + clk[i] = cc->clk;
> > + i++;
> > + }
> > + }
> > + }
> > +
> > + return ret;
> > +
> > +err:
> > + for (--i; i >= 0; i--)
> > + if (enable)
> > + clk_disable_unprepare(clk[i]);
> > + else
> > + clk_prepare_enable(clk[i]);
> > + return ret;
> > +}
> > +
> > +int bus_clk_enable(struct scpsys_ext_attr *attr)
> > +{
> > + struct scpsys_ext_attr *attr_p;
> > + int ret = 0;
> > +
> > + attr_p = __get_attr_parent(attr->parent_n);
>
> Why can't we implement this using the pg_genpd_add_subdomain approach?
OK, we will follow it.
>
> > + if (!IS_ERR(attr_p)) {
> > + ret = bus_clk_enable_disable(attr_p, CMD_ENABLE);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + return bus_clk_enable_disable(attr, CMD_ENABLE);
> > +}
> > +
> > +int bus_clk_disable(struct scpsys_ext_attr *attr)
> > +{
> > + struct scpsys_ext_attr *attr_p;
> > + int ret = 0;
> > +
> > + ret = bus_clk_enable_disable(attr, CMD_DISABLE);
> > + if (ret)
> > + return ret;
> > +
> > + attr_p = __get_attr_parent(attr->parent_n);
> > + if (!IS_ERR(attr_p))
> > + ret = bus_clk_enable_disable(attr_p, CMD_DISABLE);
>
> Same here.
>
> > +
> > + return ret;
> > +}
> > +
> > +const struct bus_mask_ops bus_mask_set_clr_ctrl = {
> > + .set = &mtk_generic_set_cmd,
> > + .release = &mtk_generic_clr_cmd,
> > +};
> > +
> > +const struct bus_ext_ops ext_bus_ctrl = {
> > + .enable = &bus_ctrl_set,
> > + .disable = &bus_ctrl_release,
> > +};
> > +
> > +const struct bus_ext_ops ext_cg_ctrl = {
> > + .enable = &bus_clk_enable,
> > + .disable = &bus_clk_disable,
> > +};
> > +
> > +/*
> > + * scpsys bus driver init
> > + */
> > +struct regmap *syscon_regmap_lookup_by_phandle_idx(struct device_node *np,
> > + const char *property,
> > + int index)
> > +{
> > + struct device_node *syscon_np;
> > + struct regmap *regmap;
> > +
> > + if (property)
> > + syscon_np = of_parse_phandle(np, property, index);
> > + else
> > + syscon_np = np;
> > +
> > + if (!syscon_np)
> > + return ERR_PTR(-ENODEV);
> > +
> > + regmap = syscon_node_to_regmap(syscon_np);
> > + of_node_put(syscon_np);
> > +
> > + return regmap;
> > +}
>
> Why do we need this? It is never called...
Sorry, Forgot to delete it.
>
> > +
> > +int scpsys_ext_regmap_init(struct platform_device *pdev)
> > +{
> > + infracfg = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
> > + INFRA);
> > + if (IS_ERR(infracfg)) {
> > + dev_err(&pdev->dev,
> > + "Cannot find bus infracfg controller: %ld\n",
> > + PTR_ERR(infracfg));
> > + return PTR_ERR(infracfg);
> > + }
> > +
> > + smi_comm = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
> > + SMIC);
> > + if (IS_ERR(smi_comm)) {
> > + dev_err(&pdev->dev,
> > + "Cannot find bus smi_comm controller: %ld\n",
> > + PTR_ERR(smi_comm));
> > + return PTR_ERR(smi_comm);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int add_clk_to_list(struct platform_device *pdev,
> > + const char *name,
> > + const char *scpd_n)
> > +{
> > + struct clk *clk;
> > + struct ext_clk_ctrl *cc;
> > +
> > + clk = devm_clk_get(&pdev->dev, name);
> > + if (IS_ERR(clk)) {
> > + dev_err(&pdev->dev, "Failed add clk %ld\n", PTR_ERR(clk));
> > + return PTR_ERR(clk);
> > + }
> > +
> > + cc = kzalloc(sizeof(*cc), GFP_KERNEL);
> > + cc->clk = clk;
> > + cc->scpd_n = kstrdup(scpd_n, GFP_KERNEL);
> > +
> > + list_add(&cc->clk_list, &ext_clk_map_list);
> > +
> > + return 0;
> > +}
> > +
> > +static int add_cg_to_list(struct platform_device *pdev)
> > +{
> > + int i = 0;
> > +
> > + struct device_node *node = pdev->dev.of_node;
> > +
> > + if (!node) {
> > + dev_err(&pdev->dev, "Cannot find topcksys node: %ld\n",
>
> Why topcksys? Shouldn't that be the node of scpsys?
it's typo.we will fix it.
>
> > + PTR_ERR(node));
> > + return PTR_ERR(node);
> > + }
> > +
> > + do {
> > + const char *ck_name;
> > + char *temp_str;
> > + char *tok[2] = {NULL};
> > + int cg_idx = 0;
> > + int idx = 0;
> > + int ret = 0;
> > +
> > + ret = of_property_read_string_index(node, "clock-names", i,
> > + &ck_name);
> > + if (ret < 0)
> > + break;
> > +
> > + temp_str = kmalloc_array(strlen(ck_name), sizeof(char),
> > + GFP_KERNEL | __GFP_ZERO);
> > + memcpy(temp_str, ck_name, strlen(ck_name));
> > + temp_str[strlen(ck_name)] = '\0';
>
> why don't you use kstrdup or similar?
we will fix it.
>
> > + do {
> > + tok[idx] = strsep(&temp_str, "-");
> > + idx++;
> > + } while (temp_str);
>
> You want to split the clock name like "mm-2" in
> char **tok = {"mm", "2"};
> correct? That can be done easier AFAIK:
> tok[0] = strsep(&temp_str, "-");
> tok[1] = &temp_str;
we will fix it.
>
> > +
> > + if (idx == 2) {
>
> You don't add clocks like "mfg" and "mm". Why?
it's not the same, "mm" and "mfg" belong to mux, not a cg.and mux is
handle by mtk_scpsys.c
>
> > + if (kstrtouint(tok[1], 10, &cg_idx))
> i
> And then? You don't do anything with cg_idx..t
yes, we will delete this check.
>
> > + return -EINVAL;
> > +
> > + if (add_clk_to_list(pdev, ck_name, tok[0]))
>
> add_clk_to_list third parameter is the name of the scp domain, but you pass the
> clock name here. I'm puzzled.
yes, our idea is to set the prefix same as scp domain name, so we can
find the correct cg clks which belong to relative scp domain.
ex: "mm-0""mm-1" need to enable for "mm" scp domain power control flow.
>
> > + return -EINVAL;
> > + }
> > + kfree(temp_str);
> > + i++;
> > + } while (1);
> > +
> > + return 0;
> > +}
> > +
> > +int scpsys_ext_clk_init(struct platform_device *pdev)
> > +{
> > + int ret = 0;
> > +
> > + ret = add_cg_to_list(pdev);
> > + if (ret)
> > + goto err;
> > +
> > +err:
> > + return ret;
> > +}
>
> Why do we need add_cg_to_list, it can be implemented directly here. Why is here
> a goto to a return statement that will be executed anyway? Please go through the
> code and check that it is clean before submitting.
yes, we will fix it.
>
> > +
> > +int scpsys_ext_attr_init(const struct scpsys_ext_data *data)
> > +{
> > + int i, count = 0;
> > +
> > + for (i = 0; i < data->num_attr; i++) {
> > + struct scpsys_ext_attr *node = data->attr + i;
> > +
> > + if (!node)
> > + continue;
> > +
> > + list_add(&node->attr_list, &ext_attr_map_list);
> > + count++;
> > + }
> > +
> > + if (!count)
> > + return -EINVAL;
> > +
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id of_scpsys_ext_match_tbl[] = {
> > + {
> > + /* sentinel */
> > + }
> > +};
> > +
> > +struct scpsys_ext_data *scpsys_ext_init(struct platform_device *pdev)
> > +{
> > + const struct of_device_id *match;
> > + struct scpsys_ext_data *data;
> > + int ret;
> > +
> > + match = of_match_device(of_scpsys_ext_match_tbl, &pdev->dev);
> > +
> > + if (!match) {
> > + dev_err(&pdev->dev, "no match\n");
> > + return ERR_CAST(match);
> > + }
> > +
> > + data = (struct scpsys_ext_data *)match->data;
> > + if (IS_ERR(data)) {
> > + dev_err(&pdev->dev, "no match scpext data\n");
> > + return ERR_CAST(data);
> > + }
> > +
> > + ret = scpsys_ext_attr_init(data);
> > + if (ret) {
> > + dev_err(&pdev->dev,
> > + "Failed to init bus attr: %d\n",
> > + ret);
> > + return ERR_PTR(ret);
> > + }
> > +
> > + ret = scpsys_ext_regmap_init(pdev);
> > + if (ret) {
> > + dev_err(&pdev->dev,
> > + "Failed to init bus register: %d\n",
> > + ret);
> > + return ERR_PTR(ret);
> > + }
> > +
> > + ret = scpsys_ext_clk_init(pdev);
> > + if (ret) {
> > + dev_err(&pdev->dev, "Failed to init bus clks: %d\n",
> > + ret);
> > + return ERR_PTR(ret);
> > + }
> > +
> > + return data;
> > +}
> > diff --git a/drivers/soc/mediatek/mtk-scpsys.c b/drivers/soc/mediatek/mtk-scpsys.c
> > index 4bb6c7a..03df2d6 100644
> > --- a/drivers/soc/mediatek/mtk-scpsys.c
> > +++ b/drivers/soc/mediatek/mtk-scpsys.c
> > @@ -1,3 +1,4 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > /*
> > * Copyright (c) 2015 Pengutronix, Sascha Hauer <kernel@pengutronix.de>
> > *
> > @@ -20,6 +21,7 @@
> > #include <linux/pm_domain.h>
> > #include <linux/regulator/consumer.h>
> > #include <linux/soc/mediatek/infracfg.h>
> > +#include <linux/soc/mediatek/scpsys-ext.h>
> >
> > #include <dt-bindings/power/mt2701-power.h>
> > #include <dt-bindings/power/mt2712-power.h>
> > @@ -117,6 +119,15 @@ enum clk_id {
> >
> > #define MAX_CLKS 3
> >
> > +/**
> > + * struct scp_domain_data - scp domain data for power on/off flow
> > + * @name: The domain name.
> > + * @sta_mask: The mask for power on/off status bit.
> > + * @ctl_offs: The offset for main power control register.
> > + * @sram_pdn_bits: The mask for sram power control bits.
> > + * @sram_pdn_ack_bits The mask for sram power control acked bits.
> > + * @caps: The flag for active wake-up action.
> > + */
> > struct scp_domain_data {
> > const char *name;
> > u32 sta_mask;
> > @@ -150,7 +161,7 @@ struct scp {
> > void __iomem *base;
> > struct regmap *infracfg;
> > struct scp_ctrl_reg ctrl_reg;
> > - bool bus_prot_reg_update;
> > + struct scpsys_ext_data *ext_data;
> > };
> >
> > struct scp_subdomain {
> > @@ -164,7 +175,6 @@ struct scp_soc_data {
> > const struct scp_subdomain *subdomains;
> > int num_subdomains;
> > const struct scp_ctrl_reg regs;
> > - bool bus_prot_reg_update;
> > };
> >
> > static int scpsys_domain_is_on(struct scp_domain *scpd)
> > @@ -236,6 +246,31 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
> > val |= PWR_RST_B_BIT;
> > writel(val, ctl_addr);
> >
> > + if (!IS_ERR(scp->ext_data)) {
> > + struct scpsys_ext_attr *attr;
> > +
> > + attr = scp->ext_data->get_attr(scpd->data->name);
> > + if (!IS_ERR(attr) && attr->cg_ops) {
> > + ret = attr->cg_ops->enable(attr);
> > + if (ret)
> > + goto err_ext_clk;
> > + }
> > + }
> > +
> > + val &= ~scpd->data->sram_pdn_bits;
> > + writel(val, ctl_addr);
> > +
> > + if (!IS_ERR(scp->ext_data)) {
> > + struct scpsys_ext_attr *attr;
> > +
> > + attr = scp->ext_data->get_attr(scpd->data->name);
> > + if (!IS_ERR(attr) && attr->cg_ops) {
> > + ret = attr->cg_ops->enable(attr);
> > + if (ret)
> > + goto err_ext_clk;
> > + }
> > + }
> > +
> > val &= ~scpd->data->sram_pdn_bits;
> > writel(val, ctl_addr);
> >
> > @@ -247,25 +282,65 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
> > * applied here.
> > */
> > usleep_range(12000, 12100);
> > -
> > } else {
> > ret = readl_poll_timeout(ctl_addr, tmp, (tmp & pdn_ack) == 0,
> > MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
> > if (ret < 0)
> > - goto err_pwr_ack;
> > + goto err_sram;
> > }
> >
> > if (scpd->data->bus_prot_mask) {
> > ret = mtk_infracfg_clear_bus_protection(scp->infracfg,
> > - scpd->data->bus_prot_mask,
> > - scp->bus_prot_reg_update);
> > + scpd->data->bus_prot_mask);
> > if (ret)
> > - goto err_pwr_ack;
> > + goto err_sram;
> > + }
> > +
> > + if (!IS_ERR(scp->ext_data)) {
> > + struct scpsys_ext_attr *attr;
> > +
> > + attr = scp->ext_data->get_attr(scpd->data->name);
> > + if (!IS_ERR(attr) && attr->bus_ops) {
> > + ret = attr->bus_ops->disable(attr);
> > + if (ret)
> > + goto err_sram;
> > + }
> > + }
> > +
> > + if (!IS_ERR(scp->ext_data)) {
> > + struct scpsys_ext_attr *attr;
> > +
> > + attr = scp->ext_data->get_attr(scpd->data->name);
> > + if (!IS_ERR(attr) && attr->cg_ops) {
> > + ret = attr->cg_ops->disable(attr);
> > + if (ret)
> > + goto err_sram;
> > + }
> > }
> >
> > return 0;
> >
> > +err_sram:
> > + val = readl(ctl_addr);
> > + val |= scpd->data->sram_pdn_bits;
> > + writel(val, ctl_addr);
> > +err_ext_clk:
> > + val = readl(ctl_addr);
> > + val |= PWR_ISO_BIT;
> > + writel(val, ctl_addr);
> > +
> > + val &= ~PWR_RST_B_BIT;
> > + writel(val, ctl_addr);
> > +
> > + val |= PWR_CLK_DIS_BIT;
> > + writel(val, ctl_addr);
> > err_pwr_ack:
> > + val &= ~PWR_ON_BIT;
> > + writel(val, ctl_addr);
> > +
> > + val &= ~PWR_ON_2ND_BIT;
> > + writel(val, ctl_addr);
> > +
> > for (i = MAX_CLKS - 1; i >= 0; i--) {
> > if (scpd->clk[i])
> > clk_disable_unprepare(scpd->clk[i]);
> > @@ -274,8 +349,6 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
> > if (scpd->supply)
> > regulator_disable(scpd->supply);
> >
> > - dev_err(scp->dev, "Failed to power on domain %s\n", genpd->name);
> > -
> > return ret;
> > }
> >
> > @@ -289,14 +362,35 @@ static int scpsys_power_off(struct generic_pm_domain *genpd)
> > int ret, tmp;
> > int i;
> >
> > + if (!IS_ERR(scp->ext_data)) {
> > + struct scpsys_ext_attr *attr;
> > +
> > + attr = scp->ext_data->get_attr(scpd->data->name);
> > + if (!IS_ERR(attr) && attr->cg_ops) {
> > + ret = attr->cg_ops->enable(attr);
> > + if (ret)
> > + goto out;
> > + }
> > + }
> > +
> > if (scpd->data->bus_prot_mask) {
> > ret = mtk_infracfg_set_bus_protection(scp->infracfg,
> > - scpd->data->bus_prot_mask,
> > - scp->bus_prot_reg_update);
> > + scpd->data->bus_prot_mask);
> > if (ret)
> > goto out;
> > }
> >
> > + if (!IS_ERR(scp->ext_data)) {
> > + struct scpsys_ext_attr *attr;
> > +
> > + attr = scp->ext_data->get_attr(scpd->data->name);
> > + if (!IS_ERR(attr) && attr->bus_ops) {
> > + ret = attr->bus_ops->enable(attr);
> > + if (ret)
> > + goto out;
> > + }
> > + }
> > +
> > val = readl(ctl_addr);
> > val |= scpd->data->sram_pdn_bits;
> > writel(val, ctl_addr);
> > @@ -307,6 +401,17 @@ static int scpsys_power_off(struct generic_pm_domain *genpd)
> > if (ret < 0)
> > goto out;
> >
> > + if (!IS_ERR(scp->ext_data)) {
> > + struct scpsys_ext_attr *attr;
> > +
> > + attr = scp->ext_data->get_attr(scpd->data->name);
> > + if (!IS_ERR(attr) && attr->cg_ops) {
> > + ret = attr->cg_ops->disable(attr);
> > + if (ret)
> > + goto out;
> > + }
> > + }
> > +
> > val |= PWR_ISO_BIT;
> > writel(val, ctl_addr);
> >
> > @@ -337,8 +442,6 @@ static int scpsys_power_off(struct generic_pm_domain *genpd)
> > return 0;
> >
> > out:
> > - dev_err(scp->dev, "Failed to power off domain %s\n", genpd->name);
> > -
> > return ret;
> > }
> >
> > @@ -352,8 +455,7 @@ static void init_clks(struct platform_device *pdev, struct clk **clk)
> >
> > static struct scp *init_scp(struct platform_device *pdev,
> > const struct scp_domain_data *scp_domain_data, int num,
> > - const struct scp_ctrl_reg *scp_ctrl_reg,
> > - bool bus_prot_reg_update)
> > + const struct scp_ctrl_reg *scp_ctrl_reg)
> > {
> > struct genpd_onecell_data *pd_data;
> > struct resource *res;
> > @@ -367,11 +469,10 @@ static struct scp *init_scp(struct platform_device *pdev,
> >
> > scp->ctrl_reg.pwr_sta_offs = scp_ctrl_reg->pwr_sta_offs;
> > scp->ctrl_reg.pwr_sta2nd_offs = scp_ctrl_reg->pwr_sta2nd_offs;
> > -
> > - scp->bus_prot_reg_update = bus_prot_reg_update;
> > -
> > scp->dev = &pdev->dev;
> >
> > + scp->ext_data = scpsys_ext_init(pdev);
> > +
> > res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > scp->base = devm_ioremap_resource(&pdev->dev, res);
> > if (IS_ERR(scp->base))
> > @@ -1021,7 +1122,6 @@ static void mtk_register_power_domains(struct platform_device *pdev,
> > .pwr_sta_offs = SPM_PWR_STATUS,
> > .pwr_sta2nd_offs = SPM_PWR_STATUS_2ND
> > },
> > - .bus_prot_reg_update = true,
> > };
> >
> > static const struct scp_soc_data mt2712_data = {
> > @@ -1033,7 +1133,6 @@ static void mtk_register_power_domains(struct platform_device *pdev,
> > .pwr_sta_offs = SPM_PWR_STATUS,
> > .pwr_sta2nd_offs = SPM_PWR_STATUS_2ND
> > },
> > - .bus_prot_reg_update = false,
> > };
> >
> > static const struct scp_soc_data mt6765_data = {
> > @@ -1056,7 +1155,6 @@ static void mtk_register_power_domains(struct platform_device *pdev,
> > .pwr_sta_offs = SPM_PWR_STATUS_MT6797,
> > .pwr_sta2nd_offs = SPM_PWR_STATUS_2ND_MT6797
> > },
> > - .bus_prot_reg_update = true,
>
> I don't understand why you can delete this flag if you don't change anything
> else in the data structure. For me this looks like you will break other chips.
> Please explain.
>
> I have the gut feeling that this can be implemented in the existing mtk-scpsys
> driver. Can you please explain what are the points that this is not possible.
> I want to understand the design decisions you made here, because they seem
> really odd to me.
>
> Best regards,
> Matthias
>
sorry, it should not be deleted. we will fix it.
> > };
> >
> > static const struct scp_soc_data mt7622_data = {
> > @@ -1066,7 +1164,6 @@ static void mtk_register_power_domains(struct platform_device *pdev,
> > .pwr_sta_offs = SPM_PWR_STATUS,
> > .pwr_sta2nd_offs = SPM_PWR_STATUS_2ND
> > },
> > - .bus_prot_reg_update = true,
> > };
> >
> > static const struct scp_soc_data mt7623a_data = {
> > @@ -1076,7 +1173,6 @@ static void mtk_register_power_domains(struct platform_device *pdev,
> > .pwr_sta_offs = SPM_PWR_STATUS,
> > .pwr_sta2nd_offs = SPM_PWR_STATUS_2ND
> > },
> > - .bus_prot_reg_update = true,
> > };
> >
> > static const struct scp_soc_data mt8173_data = {
> > @@ -1088,7 +1184,6 @@ static void mtk_register_power_domains(struct platform_device *pdev,
> > .pwr_sta_offs = SPM_PWR_STATUS,
> > .pwr_sta2nd_offs = SPM_PWR_STATUS_2ND
> > },
> > - .bus_prot_reg_update = true,
> > };
> >
> > /*
> > @@ -1132,8 +1227,8 @@ static int scpsys_probe(struct platform_device *pdev)
> >
> > soc = of_device_get_match_data(&pdev->dev);
> >
> > - scp = init_scp(pdev, soc->domains, soc->num_domains, &soc->regs,
> > - soc->bus_prot_reg_update);
> > + scp = init_scp(pdev, soc->domains, soc->num_domains,
> > + &soc->regs);
> > if (IS_ERR(scp))
> > return PTR_ERR(scp);
> >
> > diff --git a/include/linux/soc/mediatek/infracfg.h b/include/linux/soc/mediatek/infracfg.h
> > index fd25f01..bfad082 100644
> > --- a/include/linux/soc/mediatek/infracfg.h
> > +++ b/include/linux/soc/mediatek/infracfg.h
> > @@ -32,8 +32,9 @@
> > #define MT7622_TOP_AXI_PROT_EN_WB (BIT(2) | BIT(6) | \
> > BIT(7) | BIT(8))
> >
> > -int mtk_infracfg_set_bus_protection(struct regmap *infracfg, u32 mask,
> > - bool reg_update);
> > -int mtk_infracfg_clear_bus_protection(struct regmap *infracfg, u32 mask,
> > - bool reg_update);
> > +int mtk_infracfg_set_bus_protection(struct regmap *infracfg, u32 mask);
> > +int mtk_infracfg_clear_bus_protection(struct regmap *infracfg, u32 mask);
> > +int mtk_infracfg_enable_bus_protection(struct regmap *infracfg, u32 mask);
> > +int mtk_infracfg_disable_bus_protection(struct regmap *infracfg, u32 mask);
> > +
> > #endif /* __SOC_MEDIATEK_INFRACFG_H */
> > diff --git a/include/linux/soc/mediatek/scpsys-ext.h b/include/linux/soc/mediatek/scpsys-ext.h
> > new file mode 100644
> > index 0000000..99b5ff1
> > --- /dev/null
> > +++ b/include/linux/soc/mediatek/scpsys-ext.h
> > @@ -0,0 +1,66 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef __SOC_MEDIATEK_SCPSYS_EXT_H
> > +#define __SOC_MEDIATEK_SCPSYS_EXT_H
> > +
> > +#include <linux/platform_device.h>
> > +
> > +#define CMD_ENABLE 1
> > +#define CMD_DISABLE 0
> > +
> > +#define MAX_STEP_NUM 4
> > +
> > +/**
> > + * struct bus_mask - set mask and corresponding operation for bus protect
> > + * @regs: The register set of bus register control, including set/clr/sta.
> > + * @mask: The mask set for bus protect.
> > + * @flag: The flag to idetify which operation we take for bus protect.
> > + */
> > +struct bus_mask {
> > + struct ext_reg_ctrl *regs;
> > + u32 mask;
> > + const struct bus_mask_ops *ops;
> > +};
> > +
> > +/**
> > + * struct scpsys_ext_attr - extended attribute for bus protect and further
> > + * operand.
> > + *
> > + * @scpd_n: The name present the scpsys domain where the clks belongs to.
> > + * @mask: The mask set for bus protect.
> > + * @bus_ops: The operation we take for bus protect.
> > + * @cg_ops: The operation we take for cg on/off.
> > + * @attr_list: The list node linked to ext_attr_map_list.
> > + */
> > +struct scpsys_ext_attr {
> > + const char *scpd_n;
> > + struct bus_mask mask[MAX_STEP_NUM];
> > + const char *parent_n;
> > + const struct bus_ext_ops *bus_ops;
> > + const struct bus_ext_ops *cg_ops;
> > +
> > + struct list_head attr_list;
> > +};
> > +
> > +struct scpsys_ext_data {
> > + struct scpsys_ext_attr *attr;
> > + u8 num_attr;
> > + struct scpsys_ext_attr * (*get_attr)(const char *scpd_n);
> > +};
> > +
> > +struct bus_ext_ops {
> > + int (*enable)(struct scpsys_ext_attr *attr);
> > + int (*disable)(struct scpsys_ext_attr *attr);
> > +};
> > +
> > +int mtk_generic_set_cmd(struct regmap *regmap, u32 set_ofs,
> > + u32 sta_ofs, u32 mask);
> > +int mtk_generic_clr_cmd(struct regmap *regmap, u32 clr_ofs,
> > + u32 sta_ofs, u32 mask);
> > +int mtk_generic_enable_cmd(struct regmap *regmap, u32 upd_ofs,
> > + u32 sta_ofs, u32 mask);
> > +int mtk_generic_disable_cmd(struct regmap *regmap, u32 upd_ofs,
> > + u32 sta_ofs, u32 mask);
> > +
> > +struct scpsys_ext_data *scpsys_ext_init(struct platform_device *pdev);
> > +
> > +#endif /* __SOC_MEDIATEK_SCPSYS_EXT_H */
> >
^ 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