* Re: [RFC] scmi: pinctrl: support i.MX9
2023-08-24 7:06 ` Peng Fan (OSS)
@ 2023-08-29 1:27 ` kernel test robot
-1 siblings, 0 replies; 30+ messages in thread
From: kernel test robot @ 2023-08-25 15:15 UTC (permalink / raw)
To: oe-kbuild; +Cc: lkp
::::::
:::::: Manual check reason: "git am base is a link in commit message"
::::::
BCC: lkp@intel.com
CC: llvm@lists.linux.dev
CC: oe-kbuild-all@lists.linux.dev
In-Reply-To: <20230824070611.3335107-1-peng.fan@oss.nxp.com>
References: <20230824070611.3335107-1-peng.fan@oss.nxp.com>
TO: "Peng Fan (OSS)" <peng.fan@oss.nxp.com>
Hi Peng,
[This is a private test report for your RFC patch.]
kernel test robot noticed the following build warnings:
[auto build test WARNING on linusw-pinctrl/devel]
[also build test WARNING on linusw-pinctrl/for-next]
[cannot apply to linus/master v6.5-rc7 next-20230825]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Peng-Fan-OSS/scmi-pinctrl-support-i-MX9/20230824-150323
base: https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git devel
patch link: https://lore.kernel.org/r/20230824070611.3335107-1-peng.fan%40oss.nxp.com
patch subject: [RFC] scmi: pinctrl: support i.MX9
:::::: branch date: 32 hours ago
:::::: commit date: 32 hours ago
config: i386-buildonly-randconfig-002-20230825 (https://download.01.org/0day-ci/archive/20230825/202308252347.LRqRzky5-lkp@intel.com/config)
compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
reproduce: (https://download.01.org/0day-ci/archive/20230825/202308252347.LRqRzky5-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/r/202308252347.LRqRzky5-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/firmware/arm_scmi/pinctrl.c:308:6: warning: unused variable 'attributes' [-Wunused-variable]
u32 attributes;
^
1 warning generated.
--
>> drivers/pinctrl/freescale/pinctrl-imx.c:83:5: warning: no previous prototype for function 'imx_scmi_dt_node_to_map' [-Wmissing-prototypes]
int imx_scmi_dt_node_to_map(struct pinctrl_dev *pctldev, struct device_node *np,
^
drivers/pinctrl/freescale/pinctrl-imx.c:83:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
int imx_scmi_dt_node_to_map(struct pinctrl_dev *pctldev, struct device_node *np,
^
static
1 warning generated.
vim +/attributes +308 drivers/firmware/arm_scmi/pinctrl.c
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 301
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 302 static int scmi_pinctrl_config_get(const struct scmi_protocol_handle *ph,
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 303 u32 selector,
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 304 enum scmi_pinctrl_selector_type type,
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 305 u8 config_type, unsigned long *config_value)
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 306 {
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 307 int ret;
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 @308 u32 attributes;
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 309 struct scmi_xfer *t;
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 310 struct scmi_msg_conf_get *tx;
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 311
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 312 if (!config_value || type == FUNCTION_TYPE)
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 313 return -EINVAL;
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 314
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 315 ret = scmi_pinctrl_validate_id(ph, selector, type);
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 316 if (ret)
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 317 return ret;
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 318
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 319 ret = ph->xops->xfer_get_init(ph, PINCTRL_CONFIG_GET, sizeof(*tx), sizeof(__le32), &t);
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 320 if (ret)
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 321 return ret;
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 322
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 323 tx = t->tx.buf;
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 324 tx->identifier = cpu_to_le32(selector);
1d613cbfa881b8 Peng Fan 2023-08-24 325 /*
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 326 attributes = FIELD_PREP(REG_TYPE_BITS, type) |
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 327 FIELD_PREP(REG_CONFIG, config_type);
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 328 tx->attributes = cpu_to_le32(attributes);
1d613cbfa881b8 Peng Fan 2023-08-24 329 */
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 330
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 331 ret = ph->xops->do_xfer(ph, t);
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 332 if (!ret)
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 333 *config_value = get_unaligned_le32(t->rx.buf);
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 334
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 335 ph->xops->xfer_put(ph, t);
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 336 return ret;
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 337 }
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 338
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [RFC] scmi: pinctrl: support i.MX9
@ 2023-08-29 1:27 ` kernel test robot
0 siblings, 0 replies; 30+ messages in thread
From: kernel test robot @ 2023-08-29 1:27 UTC (permalink / raw)
To: Peng Fan (OSS); +Cc: oe-kbuild-all
Hi Peng,
[This is a private test report for your RFC patch.]
kernel test robot noticed the following build warnings:
[auto build test WARNING on linusw-pinctrl/devel]
[also build test WARNING on linusw-pinctrl/for-next]
[cannot apply to linus/master v6.5-rc7 next-20230825]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Peng-Fan-OSS/scmi-pinctrl-support-i-MX9/20230824-150323
base: https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git devel
patch link: https://lore.kernel.org/r/20230824070611.3335107-1-peng.fan%40oss.nxp.com
patch subject: [RFC] scmi: pinctrl: support i.MX9
config: i386-buildonly-randconfig-002-20230825 (https://download.01.org/0day-ci/archive/20230825/202308252347.LRqRzky5-lkp@intel.com/config)
compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
reproduce: (https://download.01.org/0day-ci/archive/20230825/202308252347.LRqRzky5-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <yujie.liu@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202308252347.LRqRzky5-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/firmware/arm_scmi/pinctrl.c:308:6: warning: unused variable 'attributes' [-Wunused-variable]
u32 attributes;
^
1 warning generated.
--
>> drivers/pinctrl/freescale/pinctrl-imx.c:83:5: warning: no previous prototype for function 'imx_scmi_dt_node_to_map' [-Wmissing-prototypes]
int imx_scmi_dt_node_to_map(struct pinctrl_dev *pctldev, struct device_node *np,
^
drivers/pinctrl/freescale/pinctrl-imx.c:83:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
int imx_scmi_dt_node_to_map(struct pinctrl_dev *pctldev, struct device_node *np,
^
static
1 warning generated.
vim +/attributes +308 drivers/firmware/arm_scmi/pinctrl.c
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 301
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 302 static int scmi_pinctrl_config_get(const struct scmi_protocol_handle *ph,
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 303 u32 selector,
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 304 enum scmi_pinctrl_selector_type type,
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 305 u8 config_type, unsigned long *config_value)
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 306 {
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 307 int ret;
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 @308 u32 attributes;
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 309 struct scmi_xfer *t;
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 310 struct scmi_msg_conf_get *tx;
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 311
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 312 if (!config_value || type == FUNCTION_TYPE)
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 313 return -EINVAL;
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 314
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 315 ret = scmi_pinctrl_validate_id(ph, selector, type);
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 316 if (ret)
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 317 return ret;
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 318
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 319 ret = ph->xops->xfer_get_init(ph, PINCTRL_CONFIG_GET, sizeof(*tx), sizeof(__le32), &t);
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 320 if (ret)
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 321 return ret;
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 322
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 323 tx = t->tx.buf;
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 324 tx->identifier = cpu_to_le32(selector);
1d613cbfa881b8 Peng Fan 2023-08-24 325 /*
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 326 attributes = FIELD_PREP(REG_TYPE_BITS, type) |
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 327 FIELD_PREP(REG_CONFIG, config_type);
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 328 tx->attributes = cpu_to_le32(attributes);
1d613cbfa881b8 Peng Fan 2023-08-24 329 */
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 330
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 331 ret = ph->xops->do_xfer(ph, t);
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 332 if (!ret)
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 333 *config_value = get_unaligned_le32(t->rx.buf);
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 334
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 335 ph->xops->xfer_put(ph, t);
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 336 return ret;
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 337 }
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 338
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [RFC] scmi: pinctrl: support i.MX9
@ 2023-08-25 1:26 kernel test robot
0 siblings, 0 replies; 30+ messages in thread
From: kernel test robot @ 2023-08-25 1:26 UTC (permalink / raw)
To: oe-kbuild; +Cc: lkp
::::::
:::::: Manual check reason: "git am base is a link in commit message"
::::::
BCC: lkp@intel.com
CC: llvm@lists.linux.dev
CC: oe-kbuild-all@lists.linux.dev
In-Reply-To: <20230824070611.3335107-1-peng.fan@oss.nxp.com>
References: <20230824070611.3335107-1-peng.fan@oss.nxp.com>
TO: "Peng Fan (OSS)" <peng.fan@oss.nxp.com>
Hi Peng,
[This is a private test report for your RFC patch.]
kernel test robot noticed the following build warnings:
[auto build test WARNING on linusw-pinctrl/devel]
[also build test WARNING on linusw-pinctrl/for-next]
[cannot apply to linus/master v6.5-rc7 next-20230824]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Peng-Fan-OSS/scmi-pinctrl-support-i-MX9/20230824-150323
base: https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git devel
patch link: https://lore.kernel.org/r/20230824070611.3335107-1-peng.fan%40oss.nxp.com
patch subject: [RFC] scmi: pinctrl: support i.MX9
:::::: branch date: 18 hours ago
:::::: commit date: 18 hours ago
config: arm-randconfig-001-20230825 (https://download.01.org/0day-ci/archive/20230825/202308250912.eGNoOKED-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
reproduce: (https://download.01.org/0day-ci/archive/20230825/202308250912.eGNoOKED-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/r/202308250912.eGNoOKED-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/pinctrl/freescale/pinctrl-imx.c:83:5: warning: no previous prototype for function 'imx_scmi_dt_node_to_map' [-Wmissing-prototypes]
83 | int imx_scmi_dt_node_to_map(struct pinctrl_dev *pctldev, struct device_node *np,
| ^
drivers/pinctrl/freescale/pinctrl-imx.c:83:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
83 | int imx_scmi_dt_node_to_map(struct pinctrl_dev *pctldev, struct device_node *np,
| ^
| static
1 warning generated.
vim +/imx_scmi_dt_node_to_map +83 drivers/pinctrl/freescale/pinctrl-imx.c
ae75ff81453840 drivers/pinctrl/pinctrl-imx.c Dong Aisheng 2012-04-27 81
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 82 #define IMX_SCMI_NUM_CFG 4
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 @83 int imx_scmi_dt_node_to_map(struct pinctrl_dev *pctldev, struct device_node *np,
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 84 struct pinctrl_map **map, unsigned *num_maps)
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 85 {
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 86 struct pinctrl_map *new_map;
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 87 const __be32 *list;
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 88 unsigned long *configs = NULL;
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 89 unsigned long cfg[IMX_SCMI_NUM_CFG];
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 90 int map_num, size, pin_size, pin_id, num_pins;
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 91 int mux_reg, conf_reg, input_reg, mux_val, conf_val, input_val;
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 92 int i, j;
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 93 uint32_t ncfg;
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 94 static uint32_t daisy_off;
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 95
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 96 if (!daisy_off) {
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 97 if (of_machine_is_compatible("fsl,imx93"))
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 98 daisy_off = 0x360;
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 99 else if (of_machine_is_compatible("fsl,imx95"))
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 100 daisy_off = 0x408;
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 101 else
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 102 dev_err(pctldev->dev, "platform not support scmi pinctrl\n");
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 103 }
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 104
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 105 list = of_get_property(np, "fsl,pins", &size);
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 106 if (!list) {
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 107 dev_err(pctldev->dev, "no fsl,pins property in node %pOF\n", np);
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 108 return -EINVAL;
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 109 }
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 110
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 111 pin_size = FSL_PIN_SIZE;
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 112
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 113 if (!size || size % pin_size) {
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 114 dev_err(pctldev->dev, "Invalid fsl,pins or pins property in node %pOF\n", np);
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 115 return -EINVAL;
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 116 }
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 117
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 118 num_pins = size / pin_size;
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 119 map_num = num_pins;
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 120
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 121 new_map = kmalloc_array(map_num, sizeof(struct pinctrl_map),
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 122 GFP_KERNEL);
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 123 if (!new_map)
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 124 return -ENOMEM;
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 125
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 126 *map = new_map;
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 127 *num_maps = map_num;
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 128
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 129 /* create config map */
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 130 for (i = 0; i < num_pins; i++) {
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 131 j = 0;
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 132 ncfg = IMX_SCMI_NUM_CFG;
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 133 mux_reg = be32_to_cpu(*list++);
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 134 conf_reg = be32_to_cpu(*list++);
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 135 input_reg = be32_to_cpu(*list++);
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 136 mux_val = be32_to_cpu(*list++);
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 137 input_val = be32_to_cpu(*list++);
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 138 conf_val = be32_to_cpu(*list++);
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 139 if (conf_val & IMX_PAD_SION)
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 140 mux_val |= IOMUXC_CONFIG_SION;
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 141
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 142 pin_id = mux_reg / 4;
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 143
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 144 cfg[j++] = pinconf_to_config_packed(IMX_PIN_TYPE_MUX, mux_val);
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 145
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 146 if (!conf_reg || (conf_val & IMX_NO_PAD_CTL)) {
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 147 ncfg--;
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 148 } else {
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 149 cfg[j++] = pinconf_to_config_packed(IMX_PIN_TYPE_CONFIG, conf_val);
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 150 }
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 151
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 152 if (!input_reg) {
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 153 ncfg -= 2;
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 154 } else {
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 155 cfg[j++] = pinconf_to_config_packed(IMX_PIN_TYPE_DAISY_ID,
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 156 (input_reg - daisy_off) / 4);
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 157 cfg[j++] = pinconf_to_config_packed(IMX_PIN_TYPE_DAISY_CFG, input_val);
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 158 }
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 159
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 160 configs = kmemdup(cfg, ncfg * sizeof(unsigned long), GFP_KERNEL);
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 161
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 162 new_map[i].type = PIN_MAP_TYPE_CONFIGS_PIN;
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 163 new_map[i].data.configs.group_or_pin = pin_get_name(pctldev, pin_id);
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 164 new_map[i].data.configs.configs = configs;
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 165 new_map[i].data.configs.num_configs = ncfg;
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 166 }
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 167
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 168 return 0;
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 169 }
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 170 EXPORT_SYMBOL_GPL(imx_scmi_dt_node_to_map);
1d613cbfa881b8 drivers/pinctrl/freescale/pinctrl-imx.c Peng Fan 2023-08-24 171
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [RFC] scmi: pinctrl: support i.MX9
@ 2023-08-24 9:25 kernel test robot
0 siblings, 0 replies; 30+ messages in thread
From: kernel test robot @ 2023-08-24 9:25 UTC (permalink / raw)
To: oe-kbuild; +Cc: lkp
::::::
:::::: Manual check reason: "git am base is a link in commit message"
::::::
BCC: lkp@intel.com
CC: oe-kbuild-all@lists.linux.dev
In-Reply-To: <20230824070611.3335107-1-peng.fan@oss.nxp.com>
References: <20230824070611.3335107-1-peng.fan@oss.nxp.com>
TO: "Peng Fan (OSS)" <peng.fan@oss.nxp.com>
Hi Peng,
[This is a private test report for your RFC patch.]
kernel test robot noticed the following build warnings:
[auto build test WARNING on linusw-pinctrl/devel]
[also build test WARNING on linusw-pinctrl/for-next]
[cannot apply to linus/master v6.5-rc7 next-20230824]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Peng-Fan-OSS/scmi-pinctrl-support-i-MX9/20230824-150323
base: https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git devel
patch link: https://lore.kernel.org/r/20230824070611.3335107-1-peng.fan%40oss.nxp.com
patch subject: [RFC] scmi: pinctrl: support i.MX9
:::::: branch date: 2 hours ago
:::::: commit date: 2 hours ago
config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20230824/202308241754.4zFwbyxp-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20230824/202308241754.4zFwbyxp-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/r/202308241754.4zFwbyxp-lkp@intel.com/
All warnings (new ones prefixed by >>):
drivers/firmware/arm_scmi/pinctrl.c: In function 'scmi_pinctrl_config_get':
>> drivers/firmware/arm_scmi/pinctrl.c:308:13: warning: unused variable 'attributes' [-Wunused-variable]
308 | u32 attributes;
| ^~~~~~~~~~
--
>> drivers/pinctrl/freescale/pinctrl-imx.c:83:5: warning: no previous prototype for 'imx_scmi_dt_node_to_map' [-Wmissing-prototypes]
83 | int imx_scmi_dt_node_to_map(struct pinctrl_dev *pctldev, struct device_node *np,
| ^~~~~~~~~~~~~~~~~~~~~~~
vim +/attributes +308 drivers/firmware/arm_scmi/pinctrl.c
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 301
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 302 static int scmi_pinctrl_config_get(const struct scmi_protocol_handle *ph,
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 303 u32 selector,
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 304 enum scmi_pinctrl_selector_type type,
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 305 u8 config_type, unsigned long *config_value)
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 306 {
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 307 int ret;
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 @308 u32 attributes;
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 309 struct scmi_xfer *t;
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 310 struct scmi_msg_conf_get *tx;
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 311
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 312 if (!config_value || type == FUNCTION_TYPE)
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 313 return -EINVAL;
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 314
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 315 ret = scmi_pinctrl_validate_id(ph, selector, type);
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 316 if (ret)
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 317 return ret;
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 318
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 319 ret = ph->xops->xfer_get_init(ph, PINCTRL_CONFIG_GET, sizeof(*tx), sizeof(__le32), &t);
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 320 if (ret)
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 321 return ret;
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 322
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 323 tx = t->tx.buf;
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 324 tx->identifier = cpu_to_le32(selector);
1d613cbfa881b8 Peng Fan 2023-08-24 325 /*
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 326 attributes = FIELD_PREP(REG_TYPE_BITS, type) |
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 327 FIELD_PREP(REG_CONFIG, config_type);
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 328 tx->attributes = cpu_to_le32(attributes);
1d613cbfa881b8 Peng Fan 2023-08-24 329 */
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 330
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 331 ret = ph->xops->do_xfer(ph, t);
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 332 if (!ret)
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 333 *config_value = get_unaligned_le32(t->rx.buf);
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 334
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 335 ph->xops->xfer_put(ph, t);
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 336 return ret;
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 337 }
3a61c44f60d18a Oleksii Moisieiev 2023-08-08 338
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 30+ messages in thread* [RFC] scmi: pinctrl: support i.MX9
@ 2023-08-24 7:06 ` Peng Fan (OSS)
0 siblings, 0 replies; 30+ messages in thread
From: Peng Fan (OSS) @ 2023-08-24 7:06 UTC (permalink / raw)
To: oleksii_moisieiev, sudeep.holla, aisheng.dong, festevam, ping.bai,
linus.walleij, s.hauer
Cc: shawnguo, kernel, linux-imx, linux-arm-kernel, linux-kernel,
linux-gpio, Peng Fan
From: Peng Fan <peng.fan@nxp.com>
Based on : https://lore.kernel.org/all/cover.1691518313.git.oleksii_moisieiev@epam.com/
The upper patchset not support multiple configs in SCMI 3.2, this patch
add that in config_set, not update config_get, I suppose Oleksii will
fix.
This patch is just to introduce i.MX support to see whether people have
comments for the design.
The binding format:
<mux_reg conf_reg input_reg mux_mode input_val>
dts:
pinctrl_uart1: uart1grp {
fsl,pins = <
MX93_PAD_UART1_RXD__LPUART1_RX 0x31e
MX93_PAD_UART1_TXD__LPUART1_TX 0x31e
>;
};
i.MX pinctrl not use generic pinconf, this has been agreeed by
maintainers before. So after moving to SCMI, we will still
keep the same binding format, and i.MX SCMI firmware also use same
format when configure registers. So we need to use i.MX specific
dt_node_to_map function.
I not have good idea how to call imx dt_node_to_map, so just use
extern and of_machine_is_compatible to call imx dt_node_to_map.
Hope you could give some suggestions.
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
drivers/firmware/arm_scmi/pinctrl.c | 22 ++--
drivers/pinctrl/freescale/pinctrl-imx.c | 130 +++++++++++++++++++++---
drivers/pinctrl/pinctrl-scmi.c | 53 +++++-----
include/linux/scmi_protocol.h | 2 +-
4 files changed, 154 insertions(+), 53 deletions(-)
diff --git a/drivers/firmware/arm_scmi/pinctrl.c b/drivers/firmware/arm_scmi/pinctrl.c
index 868a2f9821be..93109997614d 100644
--- a/drivers/firmware/arm_scmi/pinctrl.c
+++ b/drivers/firmware/arm_scmi/pinctrl.c
@@ -6,13 +6,14 @@
*/
#include <linux/module.h>
+#include <linux/pinctrl/pinconf-generic.h>
#include <linux/scmi_protocol.h>
#include <linux/slab.h>
#include "protocols.h"
-#define REG_TYPE_BITS GENMASK(9, 8)
-#define REG_CONFIG GENMASK(7, 0)
+#define REG_TYPE_CONFIG GENMASK(1, 0)
+#define REG_NUM_CONFIG GENMASK(9, 2)
#define GET_GROUPS_NR(x) le32_get_bits((x), GENMASK(31, 16))
#define GET_PINS_NR(x) le32_get_bits((x), GENMASK(15, 0))
@@ -36,10 +37,11 @@ enum scmi_pinctrl_protocol_cmd {
PINCTRL_SET_PERMISSIONS = 0xb
};
+#define MAX_CONFIG_ENTRY 10
struct scmi_msg_conf_set {
__le32 identifier;
__le32 attributes;
- __le32 config_value;
+ __le32 config_value[MAX_CONFIG_ENTRY * 2];
};
struct scmi_msg_conf_get {
@@ -320,9 +322,11 @@ static int scmi_pinctrl_config_get(const struct scmi_protocol_handle *ph,
tx = t->tx.buf;
tx->identifier = cpu_to_le32(selector);
+ /*
attributes = FIELD_PREP(REG_TYPE_BITS, type) |
FIELD_PREP(REG_CONFIG, config_type);
tx->attributes = cpu_to_le32(attributes);
+ */
ret = ph->xops->do_xfer(ph, t);
if (!ret)
@@ -335,7 +339,7 @@ static int scmi_pinctrl_config_get(const struct scmi_protocol_handle *ph,
static int scmi_pinctrl_config_set(const struct scmi_protocol_handle *ph,
u32 selector,
enum scmi_pinctrl_selector_type type,
- u8 config_type, unsigned long config_value)
+ unsigned long *configs, unsigned int num_configs)
{
struct scmi_xfer *t;
struct scmi_msg_conf_set *tx;
@@ -355,10 +359,14 @@ static int scmi_pinctrl_config_set(const struct scmi_protocol_handle *ph,
tx = t->tx.buf;
tx->identifier = cpu_to_le32(selector);
- attributes = FIELD_PREP(REG_TYPE_BITS, type) |
- FIELD_PREP(REG_CONFIG, config_type);
+ for (int i = 0; i < num_configs; i++) {
+ tx->config_value[i * 2] = cpu_to_le32(pinconf_to_config_param(configs[i]));
+ tx->config_value[i * 2 + 1] = cpu_to_le32(pinconf_to_config_argument(configs[i]));
+ }
+
+ attributes = FIELD_PREP(REG_TYPE_CONFIG, type) |
+ FIELD_PREP(REG_NUM_CONFIG, num_configs);
tx->attributes = cpu_to_le32(attributes);
- tx->config_value = cpu_to_le32(config_value);
ret = ph->xops->do_xfer(ph, t);
diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c b/drivers/pinctrl/freescale/pinctrl-imx.c
index 9bc16943014f..dd0a66a17184 100644
--- a/drivers/pinctrl/freescale/pinctrl-imx.c
+++ b/drivers/pinctrl/freescale/pinctrl-imx.c
@@ -14,6 +14,7 @@
#include <linux/module.h>
#include <linux/of.h>
#include <linux/of_address.h>
+#include <linux/pinctrl/pinconf-generic.h>
#include <linux/platform_device.h>
#include <linux/regmap.h>
#include <linux/seq_file.h>
@@ -33,6 +34,29 @@
#define IMX_NO_PAD_CTL 0x80000000 /* no pin config need */
#define IMX_PAD_SION 0x40000000 /* set SION */
+/*
+ * Each pin represented in fsl,pins consists of a number of u32 PIN_FUNC_ID
+ * and 1 u32 CONFIG, the total size is PIN_FUNC_ID + CONFIG for each pin.
+ * For generic_pinconf case, there's no extra u32 CONFIG.
+ *
+ * PIN_FUNC_ID format:
+ * Default:
+ * <mux_reg conf_reg input_reg mux_mode input_val>
+ * SHARE_MUX_CONF_REG:
+ * <mux_conf_reg input_reg mux_mode input_val>
+ * IMX_USE_SCU:
+ * <pin_id mux_mode>
+ */
+#define FSL_PIN_SIZE 24
+#define FSL_PIN_SHARE_SIZE 20
+#define FSL_SCU_PIN_SIZE 12
+
+/* SCMI pin control types, aligned with SCMI firmware */
+#define IMX_PIN_TYPE_MUX 192
+#define IMX_PIN_TYPE_CONFIG 193
+#define IMX_PIN_TYPE_DAISY_ID 194
+#define IMX_PIN_TYPE_DAISY_CFG 195
+
static inline const struct group_desc *imx_pinctrl_find_group_by_name(
struct pinctrl_dev *pctldev,
const char *name)
@@ -55,6 +79,96 @@ static void imx_pin_dbg_show(struct pinctrl_dev *pctldev, struct seq_file *s,
seq_printf(s, "%s", dev_name(pctldev->dev));
}
+#define IMX_SCMI_NUM_CFG 4
+int imx_scmi_dt_node_to_map(struct pinctrl_dev *pctldev, struct device_node *np,
+ struct pinctrl_map **map, unsigned *num_maps)
+{
+ struct pinctrl_map *new_map;
+ const __be32 *list;
+ unsigned long *configs = NULL;
+ unsigned long cfg[IMX_SCMI_NUM_CFG];
+ int map_num, size, pin_size, pin_id, num_pins;
+ int mux_reg, conf_reg, input_reg, mux_val, conf_val, input_val;
+ int i, j;
+ uint32_t ncfg;
+ static uint32_t daisy_off;
+
+ if (!daisy_off) {
+ if (of_machine_is_compatible("fsl,imx93"))
+ daisy_off = 0x360;
+ else if (of_machine_is_compatible("fsl,imx95"))
+ daisy_off = 0x408;
+ else
+ dev_err(pctldev->dev, "platform not support scmi pinctrl\n");
+ }
+
+ list = of_get_property(np, "fsl,pins", &size);
+ if (!list) {
+ dev_err(pctldev->dev, "no fsl,pins property in node %pOF\n", np);
+ return -EINVAL;
+ }
+
+ pin_size = FSL_PIN_SIZE;
+
+ if (!size || size % pin_size) {
+ dev_err(pctldev->dev, "Invalid fsl,pins or pins property in node %pOF\n", np);
+ return -EINVAL;
+ }
+
+ num_pins = size / pin_size;
+ map_num = num_pins;
+
+ new_map = kmalloc_array(map_num, sizeof(struct pinctrl_map),
+ GFP_KERNEL);
+ if (!new_map)
+ return -ENOMEM;
+
+ *map = new_map;
+ *num_maps = map_num;
+
+ /* create config map */
+ for (i = 0; i < num_pins; i++) {
+ j = 0;
+ ncfg = IMX_SCMI_NUM_CFG;
+ mux_reg = be32_to_cpu(*list++);
+ conf_reg = be32_to_cpu(*list++);
+ input_reg = be32_to_cpu(*list++);
+ mux_val = be32_to_cpu(*list++);
+ input_val = be32_to_cpu(*list++);
+ conf_val = be32_to_cpu(*list++);
+ if (conf_val & IMX_PAD_SION)
+ mux_val |= IOMUXC_CONFIG_SION;
+
+ pin_id = mux_reg / 4;
+
+ cfg[j++] = pinconf_to_config_packed(IMX_PIN_TYPE_MUX, mux_val);
+
+ if (!conf_reg || (conf_val & IMX_NO_PAD_CTL)) {
+ ncfg--;
+ } else {
+ cfg[j++] = pinconf_to_config_packed(IMX_PIN_TYPE_CONFIG, conf_val);
+ }
+
+ if (!input_reg) {
+ ncfg -= 2;
+ } else {
+ cfg[j++] = pinconf_to_config_packed(IMX_PIN_TYPE_DAISY_ID,
+ (input_reg - daisy_off) / 4);
+ cfg[j++] = pinconf_to_config_packed(IMX_PIN_TYPE_DAISY_CFG, input_val);
+ }
+
+ configs = kmemdup(cfg, ncfg * sizeof(unsigned long), GFP_KERNEL);
+
+ new_map[i].type = PIN_MAP_TYPE_CONFIGS_PIN;
+ new_map[i].data.configs.group_or_pin = pin_get_name(pctldev, pin_id);
+ new_map[i].data.configs.configs = configs;
+ new_map[i].data.configs.num_configs = ncfg;
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(imx_scmi_dt_node_to_map);
+
static int imx_dt_node_to_map(struct pinctrl_dev *pctldev,
struct device_node *np,
struct pinctrl_map **map, unsigned *num_maps)
@@ -441,22 +555,6 @@ static const struct pinconf_ops imx_pinconf_ops = {
.pin_config_group_dbg_show = imx_pinconf_group_dbg_show,
};
-/*
- * Each pin represented in fsl,pins consists of a number of u32 PIN_FUNC_ID
- * and 1 u32 CONFIG, the total size is PIN_FUNC_ID + CONFIG for each pin.
- *
- * PIN_FUNC_ID format:
- * Default:
- * <mux_reg conf_reg input_reg mux_mode input_val>
- * SHARE_MUX_CONF_REG:
- * <mux_conf_reg input_reg mux_mode input_val>
- * IMX_USE_SCU:
- * <pin_id mux_mode>
- */
-#define FSL_PIN_SIZE 24
-#define FSL_PIN_SHARE_SIZE 20
-#define FSL_SCU_PIN_SIZE 12
-
static void imx_pinctrl_parse_pin_mmio(struct imx_pinctrl *ipctl,
unsigned int *pin_id, struct imx_pin *pin,
const __be32 **list_p,
diff --git a/drivers/pinctrl/pinctrl-scmi.c b/drivers/pinctrl/pinctrl-scmi.c
index a9304402ddf1..92cf10241aa8 100644
--- a/drivers/pinctrl/pinctrl-scmi.c
+++ b/drivers/pinctrl/pinctrl-scmi.c
@@ -8,6 +8,7 @@
#include <linux/device.h>
#include <linux/err.h>
#include <linux/module.h>
+#include <linux/of.h>
#include <linux/seq_file.h>
#include <linux/scmi_protocol.h>
#include <linux/slab.h>
@@ -77,13 +78,28 @@ static int pinctrl_scmi_get_group_pins(struct pinctrl_dev *pctldev,
}
#ifdef CONFIG_OF
+#ifdef CONFIG_PINCTRL_IMX
+extern int imx_scmi_dt_node_to_map(struct pinctrl_dev *pctldev, struct device_node *np,
+ struct pinctrl_map **map, unsigned *num_maps);
+#else
+static inline int imx_scmi_dt_node_to_map(struct pinctrl_dev *pctldev,
+ struct device_node *np,
+ struct pinctrl_map **map, unsigned *num_maps)
+{
+ return -ENOTSUPP;
+}
+#endif
static int pinctrl_scmi_dt_node_to_map(struct pinctrl_dev *pctldev,
struct device_node *np_config,
struct pinctrl_map **map,
u32 *num_maps)
{
+ if ((of_machine_is_compatible("fsl,imx93") || of_machine_is_compatible("fsl,imx95")))
+ return imx_scmi_dt_node_to_map(pctldev, np_config, map, num_maps);
+
return pinconf_generic_dt_node_to_map(pctldev, np_config, map, num_maps,
PIN_MAP_TYPE_INVALID);
+
}
static void pinctrl_scmi_dt_free_map(struct pinctrl_dev *pctldev, struct pinctrl_map *map,
@@ -242,25 +258,15 @@ static int pinctrl_scmi_pinconf_set(struct pinctrl_dev *pctldev,
unsigned long *configs,
unsigned int num_configs)
{
- int i, ret;
+ int ret;
struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
- enum pin_config_param config_type;
- unsigned long config_value;
if (!configs || num_configs == 0)
return -EINVAL;
- for (i = 0; i < num_configs; i++) {
- config_type = pinconf_to_config_param(configs[i]);
- config_value = pinconf_to_config_argument(configs[i]);
-
- ret = pinctrl_ops->config_set(pmx->ph, _pin, PIN_TYPE, config_type, config_value);
- if (ret) {
- dev_err(pmx->dev, "Error parsing config %ld\n",
- configs[i]);
- break;
- }
- }
+ ret = pinctrl_ops->config_set(pmx->ph, _pin, PIN_TYPE, configs, num_configs);
+ if (ret)
+ dev_err(pmx->dev, "Error parsing config %d\n", ret);
return ret;
}
@@ -270,26 +276,15 @@ static int pinctrl_scmi_pinconf_group_set(struct pinctrl_dev *pctldev,
unsigned long *configs,
unsigned int num_configs)
{
- int i, ret;
+ int ret;
struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
- enum pin_config_param config_type;
- unsigned long config_value;
if (!configs || num_configs == 0)
return -EINVAL;
- for (i = 0; i < num_configs; i++) {
- config_type = pinconf_to_config_param(configs[i]);
- config_value = pinconf_to_config_argument(configs[i]);
-
- ret = pinctrl_ops->config_set(pmx->ph, group, GROUP_TYPE, config_type,
- config_value);
- if (ret) {
- dev_err(pmx->dev, "Error parsing config = %ld",
- configs[i]);
- break;
- }
- }
+ ret = pinctrl_ops->config_set(pmx->ph, group, GROUP_TYPE, configs, num_configs);
+ if (ret)
+ dev_err(pmx->dev, "Error parsing config %d", ret);
return ret;
};
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index 99c1405decd7..e58d207b56b6 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -730,7 +730,7 @@ struct scmi_pinctrl_proto_ops {
u8 config_type, unsigned long *config_value);
int (*config_set)(const struct scmi_protocol_handle *ph, u32 selector,
enum scmi_pinctrl_selector_type type,
- u8 config_type, unsigned long config_value);
+ unsigned long *configs, unsigned int num_configs);
int (*pin_request)(const struct scmi_protocol_handle *ph, u32 pin);
int (*pin_free)(const struct scmi_protocol_handle *ph, u32 pin);
};
--
2.37.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* [RFC] scmi: pinctrl: support i.MX9
@ 2023-08-24 7:06 ` Peng Fan (OSS)
0 siblings, 0 replies; 30+ messages in thread
From: Peng Fan (OSS) @ 2023-08-24 7:06 UTC (permalink / raw)
To: oleksii_moisieiev, sudeep.holla, aisheng.dong, festevam, ping.bai,
linus.walleij, s.hauer
Cc: shawnguo, kernel, linux-imx, linux-arm-kernel, linux-kernel,
linux-gpio, Peng Fan
From: Peng Fan <peng.fan@nxp.com>
Based on : https://lore.kernel.org/all/cover.1691518313.git.oleksii_moisieiev@epam.com/
The upper patchset not support multiple configs in SCMI 3.2, this patch
add that in config_set, not update config_get, I suppose Oleksii will
fix.
This patch is just to introduce i.MX support to see whether people have
comments for the design.
The binding format:
<mux_reg conf_reg input_reg mux_mode input_val>
dts:
pinctrl_uart1: uart1grp {
fsl,pins = <
MX93_PAD_UART1_RXD__LPUART1_RX 0x31e
MX93_PAD_UART1_TXD__LPUART1_TX 0x31e
>;
};
i.MX pinctrl not use generic pinconf, this has been agreeed by
maintainers before. So after moving to SCMI, we will still
keep the same binding format, and i.MX SCMI firmware also use same
format when configure registers. So we need to use i.MX specific
dt_node_to_map function.
I not have good idea how to call imx dt_node_to_map, so just use
extern and of_machine_is_compatible to call imx dt_node_to_map.
Hope you could give some suggestions.
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
drivers/firmware/arm_scmi/pinctrl.c | 22 ++--
drivers/pinctrl/freescale/pinctrl-imx.c | 130 +++++++++++++++++++++---
drivers/pinctrl/pinctrl-scmi.c | 53 +++++-----
include/linux/scmi_protocol.h | 2 +-
4 files changed, 154 insertions(+), 53 deletions(-)
diff --git a/drivers/firmware/arm_scmi/pinctrl.c b/drivers/firmware/arm_scmi/pinctrl.c
index 868a2f9821be..93109997614d 100644
--- a/drivers/firmware/arm_scmi/pinctrl.c
+++ b/drivers/firmware/arm_scmi/pinctrl.c
@@ -6,13 +6,14 @@
*/
#include <linux/module.h>
+#include <linux/pinctrl/pinconf-generic.h>
#include <linux/scmi_protocol.h>
#include <linux/slab.h>
#include "protocols.h"
-#define REG_TYPE_BITS GENMASK(9, 8)
-#define REG_CONFIG GENMASK(7, 0)
+#define REG_TYPE_CONFIG GENMASK(1, 0)
+#define REG_NUM_CONFIG GENMASK(9, 2)
#define GET_GROUPS_NR(x) le32_get_bits((x), GENMASK(31, 16))
#define GET_PINS_NR(x) le32_get_bits((x), GENMASK(15, 0))
@@ -36,10 +37,11 @@ enum scmi_pinctrl_protocol_cmd {
PINCTRL_SET_PERMISSIONS = 0xb
};
+#define MAX_CONFIG_ENTRY 10
struct scmi_msg_conf_set {
__le32 identifier;
__le32 attributes;
- __le32 config_value;
+ __le32 config_value[MAX_CONFIG_ENTRY * 2];
};
struct scmi_msg_conf_get {
@@ -320,9 +322,11 @@ static int scmi_pinctrl_config_get(const struct scmi_protocol_handle *ph,
tx = t->tx.buf;
tx->identifier = cpu_to_le32(selector);
+ /*
attributes = FIELD_PREP(REG_TYPE_BITS, type) |
FIELD_PREP(REG_CONFIG, config_type);
tx->attributes = cpu_to_le32(attributes);
+ */
ret = ph->xops->do_xfer(ph, t);
if (!ret)
@@ -335,7 +339,7 @@ static int scmi_pinctrl_config_get(const struct scmi_protocol_handle *ph,
static int scmi_pinctrl_config_set(const struct scmi_protocol_handle *ph,
u32 selector,
enum scmi_pinctrl_selector_type type,
- u8 config_type, unsigned long config_value)
+ unsigned long *configs, unsigned int num_configs)
{
struct scmi_xfer *t;
struct scmi_msg_conf_set *tx;
@@ -355,10 +359,14 @@ static int scmi_pinctrl_config_set(const struct scmi_protocol_handle *ph,
tx = t->tx.buf;
tx->identifier = cpu_to_le32(selector);
- attributes = FIELD_PREP(REG_TYPE_BITS, type) |
- FIELD_PREP(REG_CONFIG, config_type);
+ for (int i = 0; i < num_configs; i++) {
+ tx->config_value[i * 2] = cpu_to_le32(pinconf_to_config_param(configs[i]));
+ tx->config_value[i * 2 + 1] = cpu_to_le32(pinconf_to_config_argument(configs[i]));
+ }
+
+ attributes = FIELD_PREP(REG_TYPE_CONFIG, type) |
+ FIELD_PREP(REG_NUM_CONFIG, num_configs);
tx->attributes = cpu_to_le32(attributes);
- tx->config_value = cpu_to_le32(config_value);
ret = ph->xops->do_xfer(ph, t);
diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c b/drivers/pinctrl/freescale/pinctrl-imx.c
index 9bc16943014f..dd0a66a17184 100644
--- a/drivers/pinctrl/freescale/pinctrl-imx.c
+++ b/drivers/pinctrl/freescale/pinctrl-imx.c
@@ -14,6 +14,7 @@
#include <linux/module.h>
#include <linux/of.h>
#include <linux/of_address.h>
+#include <linux/pinctrl/pinconf-generic.h>
#include <linux/platform_device.h>
#include <linux/regmap.h>
#include <linux/seq_file.h>
@@ -33,6 +34,29 @@
#define IMX_NO_PAD_CTL 0x80000000 /* no pin config need */
#define IMX_PAD_SION 0x40000000 /* set SION */
+/*
+ * Each pin represented in fsl,pins consists of a number of u32 PIN_FUNC_ID
+ * and 1 u32 CONFIG, the total size is PIN_FUNC_ID + CONFIG for each pin.
+ * For generic_pinconf case, there's no extra u32 CONFIG.
+ *
+ * PIN_FUNC_ID format:
+ * Default:
+ * <mux_reg conf_reg input_reg mux_mode input_val>
+ * SHARE_MUX_CONF_REG:
+ * <mux_conf_reg input_reg mux_mode input_val>
+ * IMX_USE_SCU:
+ * <pin_id mux_mode>
+ */
+#define FSL_PIN_SIZE 24
+#define FSL_PIN_SHARE_SIZE 20
+#define FSL_SCU_PIN_SIZE 12
+
+/* SCMI pin control types, aligned with SCMI firmware */
+#define IMX_PIN_TYPE_MUX 192
+#define IMX_PIN_TYPE_CONFIG 193
+#define IMX_PIN_TYPE_DAISY_ID 194
+#define IMX_PIN_TYPE_DAISY_CFG 195
+
static inline const struct group_desc *imx_pinctrl_find_group_by_name(
struct pinctrl_dev *pctldev,
const char *name)
@@ -55,6 +79,96 @@ static void imx_pin_dbg_show(struct pinctrl_dev *pctldev, struct seq_file *s,
seq_printf(s, "%s", dev_name(pctldev->dev));
}
+#define IMX_SCMI_NUM_CFG 4
+int imx_scmi_dt_node_to_map(struct pinctrl_dev *pctldev, struct device_node *np,
+ struct pinctrl_map **map, unsigned *num_maps)
+{
+ struct pinctrl_map *new_map;
+ const __be32 *list;
+ unsigned long *configs = NULL;
+ unsigned long cfg[IMX_SCMI_NUM_CFG];
+ int map_num, size, pin_size, pin_id, num_pins;
+ int mux_reg, conf_reg, input_reg, mux_val, conf_val, input_val;
+ int i, j;
+ uint32_t ncfg;
+ static uint32_t daisy_off;
+
+ if (!daisy_off) {
+ if (of_machine_is_compatible("fsl,imx93"))
+ daisy_off = 0x360;
+ else if (of_machine_is_compatible("fsl,imx95"))
+ daisy_off = 0x408;
+ else
+ dev_err(pctldev->dev, "platform not support scmi pinctrl\n");
+ }
+
+ list = of_get_property(np, "fsl,pins", &size);
+ if (!list) {
+ dev_err(pctldev->dev, "no fsl,pins property in node %pOF\n", np);
+ return -EINVAL;
+ }
+
+ pin_size = FSL_PIN_SIZE;
+
+ if (!size || size % pin_size) {
+ dev_err(pctldev->dev, "Invalid fsl,pins or pins property in node %pOF\n", np);
+ return -EINVAL;
+ }
+
+ num_pins = size / pin_size;
+ map_num = num_pins;
+
+ new_map = kmalloc_array(map_num, sizeof(struct pinctrl_map),
+ GFP_KERNEL);
+ if (!new_map)
+ return -ENOMEM;
+
+ *map = new_map;
+ *num_maps = map_num;
+
+ /* create config map */
+ for (i = 0; i < num_pins; i++) {
+ j = 0;
+ ncfg = IMX_SCMI_NUM_CFG;
+ mux_reg = be32_to_cpu(*list++);
+ conf_reg = be32_to_cpu(*list++);
+ input_reg = be32_to_cpu(*list++);
+ mux_val = be32_to_cpu(*list++);
+ input_val = be32_to_cpu(*list++);
+ conf_val = be32_to_cpu(*list++);
+ if (conf_val & IMX_PAD_SION)
+ mux_val |= IOMUXC_CONFIG_SION;
+
+ pin_id = mux_reg / 4;
+
+ cfg[j++] = pinconf_to_config_packed(IMX_PIN_TYPE_MUX, mux_val);
+
+ if (!conf_reg || (conf_val & IMX_NO_PAD_CTL)) {
+ ncfg--;
+ } else {
+ cfg[j++] = pinconf_to_config_packed(IMX_PIN_TYPE_CONFIG, conf_val);
+ }
+
+ if (!input_reg) {
+ ncfg -= 2;
+ } else {
+ cfg[j++] = pinconf_to_config_packed(IMX_PIN_TYPE_DAISY_ID,
+ (input_reg - daisy_off) / 4);
+ cfg[j++] = pinconf_to_config_packed(IMX_PIN_TYPE_DAISY_CFG, input_val);
+ }
+
+ configs = kmemdup(cfg, ncfg * sizeof(unsigned long), GFP_KERNEL);
+
+ new_map[i].type = PIN_MAP_TYPE_CONFIGS_PIN;
+ new_map[i].data.configs.group_or_pin = pin_get_name(pctldev, pin_id);
+ new_map[i].data.configs.configs = configs;
+ new_map[i].data.configs.num_configs = ncfg;
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(imx_scmi_dt_node_to_map);
+
static int imx_dt_node_to_map(struct pinctrl_dev *pctldev,
struct device_node *np,
struct pinctrl_map **map, unsigned *num_maps)
@@ -441,22 +555,6 @@ static const struct pinconf_ops imx_pinconf_ops = {
.pin_config_group_dbg_show = imx_pinconf_group_dbg_show,
};
-/*
- * Each pin represented in fsl,pins consists of a number of u32 PIN_FUNC_ID
- * and 1 u32 CONFIG, the total size is PIN_FUNC_ID + CONFIG for each pin.
- *
- * PIN_FUNC_ID format:
- * Default:
- * <mux_reg conf_reg input_reg mux_mode input_val>
- * SHARE_MUX_CONF_REG:
- * <mux_conf_reg input_reg mux_mode input_val>
- * IMX_USE_SCU:
- * <pin_id mux_mode>
- */
-#define FSL_PIN_SIZE 24
-#define FSL_PIN_SHARE_SIZE 20
-#define FSL_SCU_PIN_SIZE 12
-
static void imx_pinctrl_parse_pin_mmio(struct imx_pinctrl *ipctl,
unsigned int *pin_id, struct imx_pin *pin,
const __be32 **list_p,
diff --git a/drivers/pinctrl/pinctrl-scmi.c b/drivers/pinctrl/pinctrl-scmi.c
index a9304402ddf1..92cf10241aa8 100644
--- a/drivers/pinctrl/pinctrl-scmi.c
+++ b/drivers/pinctrl/pinctrl-scmi.c
@@ -8,6 +8,7 @@
#include <linux/device.h>
#include <linux/err.h>
#include <linux/module.h>
+#include <linux/of.h>
#include <linux/seq_file.h>
#include <linux/scmi_protocol.h>
#include <linux/slab.h>
@@ -77,13 +78,28 @@ static int pinctrl_scmi_get_group_pins(struct pinctrl_dev *pctldev,
}
#ifdef CONFIG_OF
+#ifdef CONFIG_PINCTRL_IMX
+extern int imx_scmi_dt_node_to_map(struct pinctrl_dev *pctldev, struct device_node *np,
+ struct pinctrl_map **map, unsigned *num_maps);
+#else
+static inline int imx_scmi_dt_node_to_map(struct pinctrl_dev *pctldev,
+ struct device_node *np,
+ struct pinctrl_map **map, unsigned *num_maps)
+{
+ return -ENOTSUPP;
+}
+#endif
static int pinctrl_scmi_dt_node_to_map(struct pinctrl_dev *pctldev,
struct device_node *np_config,
struct pinctrl_map **map,
u32 *num_maps)
{
+ if ((of_machine_is_compatible("fsl,imx93") || of_machine_is_compatible("fsl,imx95")))
+ return imx_scmi_dt_node_to_map(pctldev, np_config, map, num_maps);
+
return pinconf_generic_dt_node_to_map(pctldev, np_config, map, num_maps,
PIN_MAP_TYPE_INVALID);
+
}
static void pinctrl_scmi_dt_free_map(struct pinctrl_dev *pctldev, struct pinctrl_map *map,
@@ -242,25 +258,15 @@ static int pinctrl_scmi_pinconf_set(struct pinctrl_dev *pctldev,
unsigned long *configs,
unsigned int num_configs)
{
- int i, ret;
+ int ret;
struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
- enum pin_config_param config_type;
- unsigned long config_value;
if (!configs || num_configs == 0)
return -EINVAL;
- for (i = 0; i < num_configs; i++) {
- config_type = pinconf_to_config_param(configs[i]);
- config_value = pinconf_to_config_argument(configs[i]);
-
- ret = pinctrl_ops->config_set(pmx->ph, _pin, PIN_TYPE, config_type, config_value);
- if (ret) {
- dev_err(pmx->dev, "Error parsing config %ld\n",
- configs[i]);
- break;
- }
- }
+ ret = pinctrl_ops->config_set(pmx->ph, _pin, PIN_TYPE, configs, num_configs);
+ if (ret)
+ dev_err(pmx->dev, "Error parsing config %d\n", ret);
return ret;
}
@@ -270,26 +276,15 @@ static int pinctrl_scmi_pinconf_group_set(struct pinctrl_dev *pctldev,
unsigned long *configs,
unsigned int num_configs)
{
- int i, ret;
+ int ret;
struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
- enum pin_config_param config_type;
- unsigned long config_value;
if (!configs || num_configs == 0)
return -EINVAL;
- for (i = 0; i < num_configs; i++) {
- config_type = pinconf_to_config_param(configs[i]);
- config_value = pinconf_to_config_argument(configs[i]);
-
- ret = pinctrl_ops->config_set(pmx->ph, group, GROUP_TYPE, config_type,
- config_value);
- if (ret) {
- dev_err(pmx->dev, "Error parsing config = %ld",
- configs[i]);
- break;
- }
- }
+ ret = pinctrl_ops->config_set(pmx->ph, group, GROUP_TYPE, configs, num_configs);
+ if (ret)
+ dev_err(pmx->dev, "Error parsing config %d", ret);
return ret;
};
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index 99c1405decd7..e58d207b56b6 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -730,7 +730,7 @@ struct scmi_pinctrl_proto_ops {
u8 config_type, unsigned long *config_value);
int (*config_set)(const struct scmi_protocol_handle *ph, u32 selector,
enum scmi_pinctrl_selector_type type,
- u8 config_type, unsigned long config_value);
+ unsigned long *configs, unsigned int num_configs);
int (*pin_request)(const struct scmi_protocol_handle *ph, u32 pin);
int (*pin_free)(const struct scmi_protocol_handle *ph, u32 pin);
};
--
2.37.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [RFC] scmi: pinctrl: support i.MX9
2023-08-24 7:06 ` Peng Fan (OSS)
@ 2023-08-24 8:43 ` Linus Walleij
-1 siblings, 0 replies; 30+ messages in thread
From: Linus Walleij @ 2023-08-24 8:43 UTC (permalink / raw)
To: Peng Fan (OSS)
Cc: oleksii_moisieiev, sudeep.holla, aisheng.dong, festevam, ping.bai,
s.hauer, shawnguo, kernel, linux-imx, linux-arm-kernel,
linux-kernel, linux-gpio, Peng Fan
On Thu, Aug 24, 2023 at 9:01 AM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote:
> This patch is just to introduce i.MX support to see whether people have
> comments for the design.
Very interesting!
> The binding format:
> <mux_reg conf_reg input_reg mux_mode input_val>
> dts:
> pinctrl_uart1: uart1grp {
> fsl,pins = <
> MX93_PAD_UART1_RXD__LPUART1_RX 0x31e
> MX93_PAD_UART1_TXD__LPUART1_TX 0x31e
> >;
> };
>
> i.MX pinctrl not use generic pinconf, this has been agreeed by
> maintainers before.
Yes, it has historical reasons.
> So after moving to SCMI, we will still
> keep the same binding format, and i.MX SCMI firmware also use same
> format when configure registers. So we need to use i.MX specific
> dt_node_to_map function.
I thought the idea with SCMI was to abstract and hide the characteristics of
the underlying hardware. I.e. the firmware is to present groups and
functions and generic config options and then the driver will use these.
This patch, it seems, creates a hybrid between the old freescale driver
and the SCMI firmware communication link where the SCMI is just a
transport mechanism to something inside SCMI that poke the same
registers that userspace could poke, if it could only access these
registers.
I.e using SCMI on this platform isn't creating any abstraction of the
pin control hardware, it is merely making things more complex and
also slower bymaking the registers only accessible from this SCMI link.
But I could have misunderstood it, so please correct me!
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [RFC] scmi: pinctrl: support i.MX9
@ 2023-08-24 8:43 ` Linus Walleij
0 siblings, 0 replies; 30+ messages in thread
From: Linus Walleij @ 2023-08-24 8:43 UTC (permalink / raw)
To: Peng Fan (OSS)
Cc: oleksii_moisieiev, sudeep.holla, aisheng.dong, festevam, ping.bai,
s.hauer, shawnguo, kernel, linux-imx, linux-arm-kernel,
linux-kernel, linux-gpio, Peng Fan
On Thu, Aug 24, 2023 at 9:01 AM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote:
> This patch is just to introduce i.MX support to see whether people have
> comments for the design.
Very interesting!
> The binding format:
> <mux_reg conf_reg input_reg mux_mode input_val>
> dts:
> pinctrl_uart1: uart1grp {
> fsl,pins = <
> MX93_PAD_UART1_RXD__LPUART1_RX 0x31e
> MX93_PAD_UART1_TXD__LPUART1_TX 0x31e
> >;
> };
>
> i.MX pinctrl not use generic pinconf, this has been agreeed by
> maintainers before.
Yes, it has historical reasons.
> So after moving to SCMI, we will still
> keep the same binding format, and i.MX SCMI firmware also use same
> format when configure registers. So we need to use i.MX specific
> dt_node_to_map function.
I thought the idea with SCMI was to abstract and hide the characteristics of
the underlying hardware. I.e. the firmware is to present groups and
functions and generic config options and then the driver will use these.
This patch, it seems, creates a hybrid between the old freescale driver
and the SCMI firmware communication link where the SCMI is just a
transport mechanism to something inside SCMI that poke the same
registers that userspace could poke, if it could only access these
registers.
I.e using SCMI on this platform isn't creating any abstraction of the
pin control hardware, it is merely making things more complex and
also slower bymaking the registers only accessible from this SCMI link.
But I could have misunderstood it, so please correct me!
Yours,
Linus Walleij
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [RFC] scmi: pinctrl: support i.MX9
2023-08-24 8:43 ` Linus Walleij
@ 2023-08-24 10:51 ` Sudeep Holla
-1 siblings, 0 replies; 30+ messages in thread
From: Sudeep Holla @ 2023-08-24 10:51 UTC (permalink / raw)
To: Linus Walleij
Cc: Peng Fan (OSS), oleksii_moisieiev, Sudeep Holla, aisheng.dong,
festevam, ping.bai, s.hauer, shawnguo, kernel, linux-imx,
linux-arm-kernel, linux-kernel, linux-gpio, Peng Fan
Hi Linus,
Thanks a lot for the quick response.
On Thu, Aug 24, 2023 at 10:43:20AM +0200, Linus Walleij wrote:
> On Thu, Aug 24, 2023 at 9:01 AM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote:
>
> > This patch is just to introduce i.MX support to see whether people have
> > comments for the design.
>
> Very interesting!
>
> > The binding format:
> > <mux_reg conf_reg input_reg mux_mode input_val>
> > dts:
> > pinctrl_uart1: uart1grp {
> > fsl,pins = <
> > MX93_PAD_UART1_RXD__LPUART1_RX 0x31e
> > MX93_PAD_UART1_TXD__LPUART1_TX 0x31e
> > >;
> > };
> >
> > i.MX pinctrl not use generic pinconf, this has been agreeed by
> > maintainers before.
>
> Yes, it has historical reasons.
>
Good to know.
> > So after moving to SCMI, we will still
> > keep the same binding format, and i.MX SCMI firmware also use same
> > format when configure registers. So we need to use i.MX specific
> > dt_node_to_map function.
>
> I thought the idea with SCMI was to abstract and hide the characteristics of
> the underlying hardware. I.e. the firmware is to present groups and
> functions and generic config options and then the driver will use these.
>
Correct.
> This patch, it seems, creates a hybrid between the old freescale driver
> and the SCMI firmware communication link where the SCMI is just a
> transport mechanism to something inside SCMI that poke the same
> registers that userspace could poke, if it could only access these
> registers.
>
> I.e using SCMI on this platform isn't creating any abstraction of the
> pin control hardware, it is merely making things more complex and
> also slower bymaking the registers only accessible from this SCMI link.
>
Agreed.
I don't have much knowledge on generic pinmux conf and suggested Peng
to post the RFC to start the discussion instead of getting blocked by me
during some internal/private discussions as the main intention for him
was upstreaming the changes. I am against the idea of mixing platform
specific changes the way it is done here but since I didn't have much
knowledge on pinmux conf to suggest/provide any useful feedback I suggested
to trigger this discussion.
> But I could have misunderstood it, so please correct me!
+1
--
Regards,
Sudeep
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [RFC] scmi: pinctrl: support i.MX9
@ 2023-08-24 10:51 ` Sudeep Holla
0 siblings, 0 replies; 30+ messages in thread
From: Sudeep Holla @ 2023-08-24 10:51 UTC (permalink / raw)
To: Linus Walleij
Cc: Peng Fan (OSS), oleksii_moisieiev, Sudeep Holla, aisheng.dong,
festevam, ping.bai, s.hauer, shawnguo, kernel, linux-imx,
linux-arm-kernel, linux-kernel, linux-gpio, Peng Fan
Hi Linus,
Thanks a lot for the quick response.
On Thu, Aug 24, 2023 at 10:43:20AM +0200, Linus Walleij wrote:
> On Thu, Aug 24, 2023 at 9:01 AM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote:
>
> > This patch is just to introduce i.MX support to see whether people have
> > comments for the design.
>
> Very interesting!
>
> > The binding format:
> > <mux_reg conf_reg input_reg mux_mode input_val>
> > dts:
> > pinctrl_uart1: uart1grp {
> > fsl,pins = <
> > MX93_PAD_UART1_RXD__LPUART1_RX 0x31e
> > MX93_PAD_UART1_TXD__LPUART1_TX 0x31e
> > >;
> > };
> >
> > i.MX pinctrl not use generic pinconf, this has been agreeed by
> > maintainers before.
>
> Yes, it has historical reasons.
>
Good to know.
> > So after moving to SCMI, we will still
> > keep the same binding format, and i.MX SCMI firmware also use same
> > format when configure registers. So we need to use i.MX specific
> > dt_node_to_map function.
>
> I thought the idea with SCMI was to abstract and hide the characteristics of
> the underlying hardware. I.e. the firmware is to present groups and
> functions and generic config options and then the driver will use these.
>
Correct.
> This patch, it seems, creates a hybrid between the old freescale driver
> and the SCMI firmware communication link where the SCMI is just a
> transport mechanism to something inside SCMI that poke the same
> registers that userspace could poke, if it could only access these
> registers.
>
> I.e using SCMI on this platform isn't creating any abstraction of the
> pin control hardware, it is merely making things more complex and
> also slower bymaking the registers only accessible from this SCMI link.
>
Agreed.
I don't have much knowledge on generic pinmux conf and suggested Peng
to post the RFC to start the discussion instead of getting blocked by me
during some internal/private discussions as the main intention for him
was upstreaming the changes. I am against the idea of mixing platform
specific changes the way it is done here but since I didn't have much
knowledge on pinmux conf to suggest/provide any useful feedback I suggested
to trigger this discussion.
> But I could have misunderstood it, so please correct me!
+1
--
Regards,
Sudeep
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 30+ messages in thread* RE: [RFC] scmi: pinctrl: support i.MX9
2023-08-24 10:51 ` Sudeep Holla
@ 2023-08-24 12:52 ` Peng Fan
-1 siblings, 0 replies; 30+ messages in thread
From: Peng Fan @ 2023-08-24 12:52 UTC (permalink / raw)
To: Sudeep Holla, Linus Walleij
Cc: Peng Fan (OSS), oleksii_moisieiev@epam.com, Aisheng Dong,
festevam@gmail.com, Jacky Bai, s.hauer@pengutronix.de,
shawnguo@kernel.org, kernel@pengutronix.de, dl-linux-imx,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org
Hi Sudeep,
> Subject: Re: [RFC] scmi: pinctrl: support i.MX9
>
> Hi Linus,
>
> Thanks a lot for the quick response.
>
> On Thu, Aug 24, 2023 at 10:43:20AM +0200, Linus Walleij wrote:
> > On Thu, Aug 24, 2023 at 9:01 AM Peng Fan (OSS) <peng.fan@oss.nxp.com>
> wrote:
> >
> > > This patch is just to introduce i.MX support to see whether people
> > > have comments for the design.
> >
> > Very interesting!
> >
> > > The binding format:
> > > <mux_reg conf_reg input_reg mux_mode input_val>
> > > dts:
> > > pinctrl_uart1: uart1grp {
> > > fsl,pins = <
> > > MX93_PAD_UART1_RXD__LPUART1_RX 0x31e
> > > MX93_PAD_UART1_TXD__LPUART1_TX 0x31e
> > > >;
> > > };
> > >
> > > i.MX pinctrl not use generic pinconf, this has been agreeed by
> > > maintainers before.
> >
> > Yes, it has historical reasons.
> >
>
> Good to know.
>
> > > So after moving to SCMI, we will still keep the same binding format,
> > > and i.MX SCMI firmware also use same format when configure
> > > registers. So we need to use i.MX specific dt_node_to_map function.
> >
> > I thought the idea with SCMI was to abstract and hide the
> > characteristics of the underlying hardware. I.e. the firmware is to
> > present groups and functions and generic config options and then the
> driver will use these.
> >
>
> Correct.
>
> > This patch, it seems, creates a hybrid between the old freescale
> > driver and the SCMI firmware communication link where the SCMI is just
> > a transport mechanism to something inside SCMI that poke the same
> > registers that userspace could poke, if it could only access these
> > registers.
> >
> > I.e using SCMI on this platform isn't creating any abstraction of the
> > pin control hardware, it is merely making things more complex and also
> > slower bymaking the registers only accessible from this SCMI link.
> >
>
> Agreed.
>
> I don't have much knowledge on generic pinmux conf and suggested Peng to
> post the RFC to start the discussion instead of getting blocked by me during
> some internal/private discussions as the main intention for him was
> upstreaming the changes. I am against the idea of mixing platform specific
> changes the way it is done here but since I didn't have much knowledge on
> pinmux conf to suggest/provide any useful feedback I suggested to trigger
> this discussion.
To use generic pinconf, we still need to extend to use OEM type, our scmi firmware
will not support saying bias/pull-up and etc config type, so just as below, we add:
diff --git a/drivers/pinctrl/pinconf-generic.c b/drivers/pinctrl/pinconf-generic.c
index 365c4b0ca465..a71721cd321d 100644
--- a/drivers/pinctrl/pinconf-generic.c
+++ b/drivers/pinctrl/pinconf-generic.c
@@ -186,6 +186,16 @@ static const struct pinconf_generic_params dt_params[] = {
{ "sleep-hardware-state", PIN_CONFIG_SLEEP_HARDWARE_STATE, 0 },
{ "slew-rate", PIN_CONFIG_SLEW_RATE, 0 },
{ "skew-delay", PIN_CONFIG_SKEW_DELAY, 0 },
+ { "nxp-mux", PIN_CONFIG_NXP_MUX, 0 },
+ { "nxp-conf", PIN_CONFIG_NXP_CONF, 0 },
+ { "nxp-daisy-id", PIN_CONFIG_NXP_DAISY_ID, 0 },
+ { "nxp-daisy-val", PIN_CONFIG_NXP_DAISY_VAL, 0 },
};
And in dts:
+ pinctrl_uart1: uart1grp {
+ txd {
+ pins = "uart1txd";
+ nxp-mux = <0x0>;
+ nxp-conf = <0x31e>;
+ };
+
+ rxd {
+ pins = "uart1rxd";
+ nxp-mux = <0x0>;
+ nxp-conf = <0x31e>;
+ };
+ };
But the upper will make device tree diverge w/o scmi, because
we need support both for i.MX95.
I still prefer to use freescale current binding format which
would make dts reusable w/o scmi.
Thanks,
Peng.
>
> > But I could have misunderstood it, so please correct me!
>
> +1
>
> --
> Regards,
> Sudeep
^ permalink raw reply related [flat|nested] 30+ messages in thread* RE: [RFC] scmi: pinctrl: support i.MX9
@ 2023-08-24 12:52 ` Peng Fan
0 siblings, 0 replies; 30+ messages in thread
From: Peng Fan @ 2023-08-24 12:52 UTC (permalink / raw)
To: Sudeep Holla, Linus Walleij
Cc: Peng Fan (OSS), oleksii_moisieiev@epam.com, Aisheng Dong,
festevam@gmail.com, Jacky Bai, s.hauer@pengutronix.de,
shawnguo@kernel.org, kernel@pengutronix.de, dl-linux-imx,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org
Hi Sudeep,
> Subject: Re: [RFC] scmi: pinctrl: support i.MX9
>
> Hi Linus,
>
> Thanks a lot for the quick response.
>
> On Thu, Aug 24, 2023 at 10:43:20AM +0200, Linus Walleij wrote:
> > On Thu, Aug 24, 2023 at 9:01 AM Peng Fan (OSS) <peng.fan@oss.nxp.com>
> wrote:
> >
> > > This patch is just to introduce i.MX support to see whether people
> > > have comments for the design.
> >
> > Very interesting!
> >
> > > The binding format:
> > > <mux_reg conf_reg input_reg mux_mode input_val>
> > > dts:
> > > pinctrl_uart1: uart1grp {
> > > fsl,pins = <
> > > MX93_PAD_UART1_RXD__LPUART1_RX 0x31e
> > > MX93_PAD_UART1_TXD__LPUART1_TX 0x31e
> > > >;
> > > };
> > >
> > > i.MX pinctrl not use generic pinconf, this has been agreeed by
> > > maintainers before.
> >
> > Yes, it has historical reasons.
> >
>
> Good to know.
>
> > > So after moving to SCMI, we will still keep the same binding format,
> > > and i.MX SCMI firmware also use same format when configure
> > > registers. So we need to use i.MX specific dt_node_to_map function.
> >
> > I thought the idea with SCMI was to abstract and hide the
> > characteristics of the underlying hardware. I.e. the firmware is to
> > present groups and functions and generic config options and then the
> driver will use these.
> >
>
> Correct.
>
> > This patch, it seems, creates a hybrid between the old freescale
> > driver and the SCMI firmware communication link where the SCMI is just
> > a transport mechanism to something inside SCMI that poke the same
> > registers that userspace could poke, if it could only access these
> > registers.
> >
> > I.e using SCMI on this platform isn't creating any abstraction of the
> > pin control hardware, it is merely making things more complex and also
> > slower bymaking the registers only accessible from this SCMI link.
> >
>
> Agreed.
>
> I don't have much knowledge on generic pinmux conf and suggested Peng to
> post the RFC to start the discussion instead of getting blocked by me during
> some internal/private discussions as the main intention for him was
> upstreaming the changes. I am against the idea of mixing platform specific
> changes the way it is done here but since I didn't have much knowledge on
> pinmux conf to suggest/provide any useful feedback I suggested to trigger
> this discussion.
To use generic pinconf, we still need to extend to use OEM type, our scmi firmware
will not support saying bias/pull-up and etc config type, so just as below, we add:
diff --git a/drivers/pinctrl/pinconf-generic.c b/drivers/pinctrl/pinconf-generic.c
index 365c4b0ca465..a71721cd321d 100644
--- a/drivers/pinctrl/pinconf-generic.c
+++ b/drivers/pinctrl/pinconf-generic.c
@@ -186,6 +186,16 @@ static const struct pinconf_generic_params dt_params[] = {
{ "sleep-hardware-state", PIN_CONFIG_SLEEP_HARDWARE_STATE, 0 },
{ "slew-rate", PIN_CONFIG_SLEW_RATE, 0 },
{ "skew-delay", PIN_CONFIG_SKEW_DELAY, 0 },
+ { "nxp-mux", PIN_CONFIG_NXP_MUX, 0 },
+ { "nxp-conf", PIN_CONFIG_NXP_CONF, 0 },
+ { "nxp-daisy-id", PIN_CONFIG_NXP_DAISY_ID, 0 },
+ { "nxp-daisy-val", PIN_CONFIG_NXP_DAISY_VAL, 0 },
};
And in dts:
+ pinctrl_uart1: uart1grp {
+ txd {
+ pins = "uart1txd";
+ nxp-mux = <0x0>;
+ nxp-conf = <0x31e>;
+ };
+
+ rxd {
+ pins = "uart1rxd";
+ nxp-mux = <0x0>;
+ nxp-conf = <0x31e>;
+ };
+ };
But the upper will make device tree diverge w/o scmi, because
we need support both for i.MX95.
I still prefer to use freescale current binding format which
would make dts reusable w/o scmi.
Thanks,
Peng.
>
> > But I could have misunderstood it, so please correct me!
>
> +1
>
> --
> Regards,
> Sudeep
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 30+ messages in thread
* RE: [RFC] scmi: pinctrl: support i.MX9
2023-08-24 8:43 ` Linus Walleij
@ 2023-08-24 12:47 ` Peng Fan
-1 siblings, 0 replies; 30+ messages in thread
From: Peng Fan @ 2023-08-24 12:47 UTC (permalink / raw)
To: Linus Walleij, Peng Fan (OSS)
Cc: oleksii_moisieiev@epam.com, sudeep.holla@arm.com, Aisheng Dong,
festevam@gmail.com, Jacky Bai, s.hauer@pengutronix.de,
shawnguo@kernel.org, kernel@pengutronix.de, dl-linux-imx,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org
Hi Linus,
Thanks for quick reply.
> Subject: Re: [RFC] scmi: pinctrl: support i.MX9
>
> On Thu, Aug 24, 2023 at 9:01 AM Peng Fan (OSS) <peng.fan@oss.nxp.com>
> wrote:
>
> > This patch is just to introduce i.MX support to see whether people
> > have comments for the design.
>
> Very interesting!
>
> > The binding format:
> > <mux_reg conf_reg input_reg mux_mode input_val>
> > dts:
> > pinctrl_uart1: uart1grp {
> > fsl,pins = <
> > MX93_PAD_UART1_RXD__LPUART1_RX 0x31e
> > MX93_PAD_UART1_TXD__LPUART1_TX 0x31e
> > >;
> > };
> >
> > i.MX pinctrl not use generic pinconf, this has been agreeed by
> > maintainers before.
>
> Yes, it has historical reasons.
>
> > So after moving to SCMI, we will still keep the same binding format,
> > and i.MX SCMI firmware also use same format when configure registers.
> > So we need to use i.MX specific dt_node_to_map function.
>
> I thought the idea with SCMI was to abstract and hide the characteristics of
> the underlying hardware. I.e. the firmware is to present groups and
> functions and generic config options and then the driver will use these.
>
> This patch, it seems, creates a hybrid between the old freescale driver and
> the SCMI firmware communication link where the SCMI is just a transport
> mechanism to something inside SCMI that poke the same registers that
> userspace could poke, if it could only access these registers.
>
> I.e using SCMI on this platform isn't creating any abstraction of the pin
> control hardware,
Right.
it is merely making things more complex and also slower
> bymaking the registers only accessible from this SCMI link.
This is for safety reason, the pinctrl hardware must be handled
by a system manager entity. So mmio direct access not allowed
from Cortex-A side.
The SCMI firmware is very straightforward, there is no group or
function.
It just accepts the format as this:
MUX_TYPE, MUX VALUE, CONF_TYPE, CONF_VAL, DAISY_TYPE,
DAISY ID, DAISY_CFG, DAISY_VALUE.
Similar as linux MMIO format.
Our i.MX95 platform will support two settings, one with SCMI
firmware, one without SCMI. These two settings will share
the same pinctrl header file.
And to simplify the scmi firmware design(anyway I am not owner
of the firmware), to make pinctrl header shared w/o scmi,
we take the current in-upstream freescale imx binding format.
Thanks,
Peng.
>
> But I could have misunderstood it, so please correct me!
>
> Yours,
> Linus Walleij
^ permalink raw reply [flat|nested] 30+ messages in thread* RE: [RFC] scmi: pinctrl: support i.MX9
@ 2023-08-24 12:47 ` Peng Fan
0 siblings, 0 replies; 30+ messages in thread
From: Peng Fan @ 2023-08-24 12:47 UTC (permalink / raw)
To: Linus Walleij, Peng Fan (OSS)
Cc: oleksii_moisieiev@epam.com, sudeep.holla@arm.com, Aisheng Dong,
festevam@gmail.com, Jacky Bai, s.hauer@pengutronix.de,
shawnguo@kernel.org, kernel@pengutronix.de, dl-linux-imx,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org
Hi Linus,
Thanks for quick reply.
> Subject: Re: [RFC] scmi: pinctrl: support i.MX9
>
> On Thu, Aug 24, 2023 at 9:01 AM Peng Fan (OSS) <peng.fan@oss.nxp.com>
> wrote:
>
> > This patch is just to introduce i.MX support to see whether people
> > have comments for the design.
>
> Very interesting!
>
> > The binding format:
> > <mux_reg conf_reg input_reg mux_mode input_val>
> > dts:
> > pinctrl_uart1: uart1grp {
> > fsl,pins = <
> > MX93_PAD_UART1_RXD__LPUART1_RX 0x31e
> > MX93_PAD_UART1_TXD__LPUART1_TX 0x31e
> > >;
> > };
> >
> > i.MX pinctrl not use generic pinconf, this has been agreeed by
> > maintainers before.
>
> Yes, it has historical reasons.
>
> > So after moving to SCMI, we will still keep the same binding format,
> > and i.MX SCMI firmware also use same format when configure registers.
> > So we need to use i.MX specific dt_node_to_map function.
>
> I thought the idea with SCMI was to abstract and hide the characteristics of
> the underlying hardware. I.e. the firmware is to present groups and
> functions and generic config options and then the driver will use these.
>
> This patch, it seems, creates a hybrid between the old freescale driver and
> the SCMI firmware communication link where the SCMI is just a transport
> mechanism to something inside SCMI that poke the same registers that
> userspace could poke, if it could only access these registers.
>
> I.e using SCMI on this platform isn't creating any abstraction of the pin
> control hardware,
Right.
it is merely making things more complex and also slower
> bymaking the registers only accessible from this SCMI link.
This is for safety reason, the pinctrl hardware must be handled
by a system manager entity. So mmio direct access not allowed
from Cortex-A side.
The SCMI firmware is very straightforward, there is no group or
function.
It just accepts the format as this:
MUX_TYPE, MUX VALUE, CONF_TYPE, CONF_VAL, DAISY_TYPE,
DAISY ID, DAISY_CFG, DAISY_VALUE.
Similar as linux MMIO format.
Our i.MX95 platform will support two settings, one with SCMI
firmware, one without SCMI. These two settings will share
the same pinctrl header file.
And to simplify the scmi firmware design(anyway I am not owner
of the firmware), to make pinctrl header shared w/o scmi,
we take the current in-upstream freescale imx binding format.
Thanks,
Peng.
>
> But I could have misunderstood it, so please correct me!
>
> Yours,
> Linus Walleij
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [RFC] scmi: pinctrl: support i.MX9
2023-08-24 12:47 ` Peng Fan
@ 2023-08-25 8:28 ` Linus Walleij
-1 siblings, 0 replies; 30+ messages in thread
From: Linus Walleij @ 2023-08-25 8:28 UTC (permalink / raw)
To: Peng Fan
Cc: Peng Fan (OSS), oleksii_moisieiev@epam.com, sudeep.holla@arm.com,
Aisheng Dong, festevam@gmail.com, Jacky Bai,
s.hauer@pengutronix.de, shawnguo@kernel.org,
kernel@pengutronix.de, dl-linux-imx,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org
On Thu, Aug 24, 2023 at 2:47 PM Peng Fan <peng.fan@nxp.com> wrote:
> Me:
>> it is merely making things more complex and also slower
> > bymaking the registers only accessible from this SCMI link.
>
> This is for safety reason, the pinctrl hardware must be handled
> by a system manager entity. So mmio direct access not allowed
> from Cortex-A side.
Yeah I understood as much. But I don't think that the firmware is
really filtering any of the access, it will just poke into any pinctrl
register as instructed anyway so what's the point. Just looks like
a layer of indirection. But I'm not your system manager, so it's not
my decision.
> The SCMI firmware is very straightforward, there is no group or
> function.
>
> It just accepts the format as this:
> MUX_TYPE, MUX VALUE, CONF_TYPE, CONF_VAL, DAISY_TYPE,
> DAISY ID, DAISY_CFG, DAISY_VALUE.
>
> Similar as linux MMIO format.
>
> Our i.MX95 platform will support two settings, one with SCMI
> firmware, one without SCMI. These two settings will share
> the same pinctrl header file.
>
> And to simplify the scmi firmware design(anyway I am not owner
> of the firmware), to make pinctrl header shared w/o scmi,
> we take the current in-upstream freescale imx binding format.
The SCMI people will have to state their position on this.
Like what they consider conformance and what extensions are
allowed. This is more a standardization question than an
implementation question so it's not really my turf.
I was under the impression that the ambition with SCMI firmware
was to abstract away and hide aspects of the hardware behind
a consistent API. This approach drives a truck through that
idea.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [RFC] scmi: pinctrl: support i.MX9
@ 2023-08-25 8:28 ` Linus Walleij
0 siblings, 0 replies; 30+ messages in thread
From: Linus Walleij @ 2023-08-25 8:28 UTC (permalink / raw)
To: Peng Fan
Cc: Peng Fan (OSS), oleksii_moisieiev@epam.com, sudeep.holla@arm.com,
Aisheng Dong, festevam@gmail.com, Jacky Bai,
s.hauer@pengutronix.de, shawnguo@kernel.org,
kernel@pengutronix.de, dl-linux-imx,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org
On Thu, Aug 24, 2023 at 2:47 PM Peng Fan <peng.fan@nxp.com> wrote:
> Me:
>> it is merely making things more complex and also slower
> > bymaking the registers only accessible from this SCMI link.
>
> This is for safety reason, the pinctrl hardware must be handled
> by a system manager entity. So mmio direct access not allowed
> from Cortex-A side.
Yeah I understood as much. But I don't think that the firmware is
really filtering any of the access, it will just poke into any pinctrl
register as instructed anyway so what's the point. Just looks like
a layer of indirection. But I'm not your system manager, so it's not
my decision.
> The SCMI firmware is very straightforward, there is no group or
> function.
>
> It just accepts the format as this:
> MUX_TYPE, MUX VALUE, CONF_TYPE, CONF_VAL, DAISY_TYPE,
> DAISY ID, DAISY_CFG, DAISY_VALUE.
>
> Similar as linux MMIO format.
>
> Our i.MX95 platform will support two settings, one with SCMI
> firmware, one without SCMI. These two settings will share
> the same pinctrl header file.
>
> And to simplify the scmi firmware design(anyway I am not owner
> of the firmware), to make pinctrl header shared w/o scmi,
> we take the current in-upstream freescale imx binding format.
The SCMI people will have to state their position on this.
Like what they consider conformance and what extensions are
allowed. This is more a standardization question than an
implementation question so it's not really my turf.
I was under the impression that the ambition with SCMI firmware
was to abstract away and hide aspects of the hardware behind
a consistent API. This approach drives a truck through that
idea.
Yours,
Linus Walleij
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 30+ messages in thread
* RE: [RFC] scmi: pinctrl: support i.MX9
2023-08-25 8:28 ` Linus Walleij
@ 2023-08-25 8:43 ` Peng Fan
-1 siblings, 0 replies; 30+ messages in thread
From: Peng Fan @ 2023-08-25 8:43 UTC (permalink / raw)
To: Linus Walleij
Cc: Peng Fan (OSS), oleksii_moisieiev@epam.com, sudeep.holla@arm.com,
Aisheng Dong, festevam@gmail.com, Jacky Bai,
s.hauer@pengutronix.de, shawnguo@kernel.org,
kernel@pengutronix.de, dl-linux-imx,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org
> Subject: Re: [RFC] scmi: pinctrl: support i.MX9
>
> On Thu, Aug 24, 2023 at 2:47 PM Peng Fan <peng.fan@nxp.com> wrote:
> > Me:
>
> >> it is merely making things more complex and also slower
> > > bymaking the registers only accessible from this SCMI link.
> >
> > This is for safety reason, the pinctrl hardware must be handled by a
> > system manager entity. So mmio direct access not allowed from Cortex-A
> > side.
>
> Yeah I understood as much. But I don't think that the firmware is really
> filtering any of the access, it will just poke into any pinctrl register as
> instructed anyway so what's the point. Just looks like a layer of indirection.
No, the firmware has a check on whether a pin is allowed to be configured
by the agent that wanna to configure the pin.
> But I'm not your system manager, so it's not my decision.
>
> > The SCMI firmware is very straightforward, there is no group or
> > function.
> >
> > It just accepts the format as this:
> > MUX_TYPE, MUX VALUE, CONF_TYPE, CONF_VAL, DAISY_TYPE, DAISY ID,
> > DAISY_CFG, DAISY_VALUE.
> >
> > Similar as linux MMIO format.
> >
> > Our i.MX95 platform will support two settings, one with SCMI firmware,
> > one without SCMI. These two settings will share the same pinctrl
> > header file.
> >
> > And to simplify the scmi firmware design(anyway I am not owner of the
> > firmware), to make pinctrl header shared w/o scmi, we take the current
> > in-upstream freescale imx binding format.
>
> The SCMI people will have to state their position on this.
> Like what they consider conformance and what extensions are allowed. This
> is more a standardization question than an implementation question so it's
> not really my turf.
The i.MX95 SCMI firmware uses OEM extension type. So I just follow
what the firmware did and support it in linux. Anyway let's
wait Sudeep's reply.
Thanks,
Peng.
>
> I was under the impression that the ambition with SCMI firmware was to
> abstract away and hide aspects of the hardware behind a consistent API.
> This approach drives a truck through that idea.
>
> Yours,
> Linus Walleij
^ permalink raw reply [flat|nested] 30+ messages in thread
* RE: [RFC] scmi: pinctrl: support i.MX9
@ 2023-08-25 8:43 ` Peng Fan
0 siblings, 0 replies; 30+ messages in thread
From: Peng Fan @ 2023-08-25 8:43 UTC (permalink / raw)
To: Linus Walleij
Cc: Peng Fan (OSS), oleksii_moisieiev@epam.com, sudeep.holla@arm.com,
Aisheng Dong, festevam@gmail.com, Jacky Bai,
s.hauer@pengutronix.de, shawnguo@kernel.org,
kernel@pengutronix.de, dl-linux-imx,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org
> Subject: Re: [RFC] scmi: pinctrl: support i.MX9
>
> On Thu, Aug 24, 2023 at 2:47 PM Peng Fan <peng.fan@nxp.com> wrote:
> > Me:
>
> >> it is merely making things more complex and also slower
> > > bymaking the registers only accessible from this SCMI link.
> >
> > This is for safety reason, the pinctrl hardware must be handled by a
> > system manager entity. So mmio direct access not allowed from Cortex-A
> > side.
>
> Yeah I understood as much. But I don't think that the firmware is really
> filtering any of the access, it will just poke into any pinctrl register as
> instructed anyway so what's the point. Just looks like a layer of indirection.
No, the firmware has a check on whether a pin is allowed to be configured
by the agent that wanna to configure the pin.
> But I'm not your system manager, so it's not my decision.
>
> > The SCMI firmware is very straightforward, there is no group or
> > function.
> >
> > It just accepts the format as this:
> > MUX_TYPE, MUX VALUE, CONF_TYPE, CONF_VAL, DAISY_TYPE, DAISY ID,
> > DAISY_CFG, DAISY_VALUE.
> >
> > Similar as linux MMIO format.
> >
> > Our i.MX95 platform will support two settings, one with SCMI firmware,
> > one without SCMI. These two settings will share the same pinctrl
> > header file.
> >
> > And to simplify the scmi firmware design(anyway I am not owner of the
> > firmware), to make pinctrl header shared w/o scmi, we take the current
> > in-upstream freescale imx binding format.
>
> The SCMI people will have to state their position on this.
> Like what they consider conformance and what extensions are allowed. This
> is more a standardization question than an implementation question so it's
> not really my turf.
The i.MX95 SCMI firmware uses OEM extension type. So I just follow
what the firmware did and support it in linux. Anyway let's
wait Sudeep's reply.
Thanks,
Peng.
>
> I was under the impression that the ambition with SCMI firmware was to
> abstract away and hide aspects of the hardware behind a consistent API.
> This approach drives a truck through that idea.
>
> Yours,
> Linus Walleij
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [RFC] scmi: pinctrl: support i.MX9
2023-08-25 8:43 ` Peng Fan
@ 2023-08-30 11:47 ` Cristian Marussi
-1 siblings, 0 replies; 30+ messages in thread
From: Cristian Marussi @ 2023-08-30 11:47 UTC (permalink / raw)
To: Peng Fan
Cc: Linus Walleij, Peng Fan (OSS), oleksii_moisieiev@epam.com,
sudeep.holla@arm.com, cristian.marussi, Aisheng Dong,
festevam@gmail.com, Jacky Bai, s.hauer@pengutronix.de,
shawnguo@kernel.org, kernel@pengutronix.de, dl-linux-imx,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org
On Fri, Aug 25, 2023 at 08:43:38AM +0000, Peng Fan wrote:
> > Subject: Re: [RFC] scmi: pinctrl: support i.MX9
> >
> > On Thu, Aug 24, 2023 at 2:47 PM Peng Fan <peng.fan@nxp.com> wrote:
> > > Me:
Hi Peng,
> >
> > >> it is merely making things more complex and also slower
> > > > bymaking the registers only accessible from this SCMI link.
> > >
> > > This is for safety reason, the pinctrl hardware must be handled by a
> > > system manager entity. So mmio direct access not allowed from Cortex-A
> > > side.
> >
> > Yeah I understood as much. But I don't think that the firmware is really
> > filtering any of the access, it will just poke into any pinctrl register as
> > instructed anyway so what's the point. Just looks like a layer of indirection.
>
> No, the firmware has a check on whether a pin is allowed to be configured
> by the agent that wanna to configure the pin.
>
> > But I'm not your system manager, so it's not my decision.
> >
> > > The SCMI firmware is very straightforward, there is no group or
> > > function.
> > >
> > > It just accepts the format as this:
> > > MUX_TYPE, MUX VALUE, CONF_TYPE, CONF_VAL, DAISY_TYPE, DAISY ID,
> > > DAISY_CFG, DAISY_VALUE.
> > >
> > > Similar as linux MMIO format.
> > >
> > > Our i.MX95 platform will support two settings, one with SCMI firmware,
> > > one without SCMI. These two settings will share the same pinctrl
> > > header file.
> > >
> > > And to simplify the scmi firmware design(anyway I am not owner of the
> > > firmware), to make pinctrl header shared w/o scmi, we take the current
> > > in-upstream freescale imx binding format.
> >
> > The SCMI people will have to state their position on this.
> > Like what they consider conformance and what extensions are allowed. This
> > is more a standardization question than an implementation question so it's
> > not really my turf.
>
> The i.MX95 SCMI firmware uses OEM extension type. So I just follow
> what the firmware did and support it in linux. Anyway let's
> wait Sudeep's reply.
>
So my unsderstanding on this matter as of now is that:
1. the current SCMI Pinctrl specification can support your usecase by using
OEM Types and multiple pins/values CONFIG_GET/SET commands
2. the Kernel SCMI protocol layer (driver/firmware/arm_scmi/pinctrl.c)
is equally fine and can support your usecase, AFTER Oleksii fixes it to
align it to the latest v3.2-BETA2 specification changes.
IOW, this means that, using the SCMI Pinctrl protocol operations
exposed in scmi_protocol.h, from somewhere, you are able to properly
configure multiple pins/values with your specific OEM types.
3. The SCMI Pinctrl driver (by Oleksii) built on top of the pinctrl protocol
operations is instead NOT suitable for your usecase since it uses the Linux
Generic Pinconf and IMX does not make use of it, and instead IMX has
its own bindings and related parsing logic.
Am I right ?
If this is the case, I would NOT try to abuse the current SCMI Pinctrl
Generic driver (by Oleksii) by throwing into it a bunch of IMX specific DT
parsing, also because you'll end-up NOT using most of the generic SCMI
Pinctrl driver but just reusing a bit of the probe (customized with your
own DT maps parsing)
Instead, given that the spec[1.] and the protocol layer[2.] are fine for
your use case and you indeed have already a custom way to parse your DT
mappings, I would say that you could just write your own custom SCMI
driver ( ? pinctrl-imx-scmi), distinct and much more simple than the generic one,
that does its own IMX DT parsing and calls just the SCMI protocol operations
that it needs in the way that your platform expects: so basically another
Pinctrl SCMI driver that does not use the generic pinconf DT
configuration BUT DO USE the underlying SCMI Pinctrl protocol (via its
exposed protocol operations...)
Not sure what Sudeep thinks about supporting multiple SCMI driver for the
same protocol (we did it already for Sensors hwmon && iio), and if this
approach won't need some sort of mutual exclusion mechanism in Kconfig to
avoid loading both the generic and the custom IMX (even though they should
be able to co-exist from the SCMI kernel/fw stack pint of view, as long as
you dont mess-up the DTs and mixup generic pins with custom IMX pins...)
Instead, adding an IMX-custom extension to what it was supposed to be a generic
driver (as you propose) seems to me like a stretch of the generic Pinctrl driver
that is not really worth, since you'll end up polluting the generic driver with
some highly custom and specific IMX bits. while really NOT reusing so much of
the generic driver at all.
Thanks,
Cristian
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [RFC] scmi: pinctrl: support i.MX9
@ 2023-08-30 11:47 ` Cristian Marussi
0 siblings, 0 replies; 30+ messages in thread
From: Cristian Marussi @ 2023-08-30 11:47 UTC (permalink / raw)
To: Peng Fan
Cc: Linus Walleij, Peng Fan (OSS), oleksii_moisieiev@epam.com,
sudeep.holla@arm.com, cristian.marussi, Aisheng Dong,
festevam@gmail.com, Jacky Bai, s.hauer@pengutronix.de,
shawnguo@kernel.org, kernel@pengutronix.de, dl-linux-imx,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org
On Fri, Aug 25, 2023 at 08:43:38AM +0000, Peng Fan wrote:
> > Subject: Re: [RFC] scmi: pinctrl: support i.MX9
> >
> > On Thu, Aug 24, 2023 at 2:47 PM Peng Fan <peng.fan@nxp.com> wrote:
> > > Me:
Hi Peng,
> >
> > >> it is merely making things more complex and also slower
> > > > bymaking the registers only accessible from this SCMI link.
> > >
> > > This is for safety reason, the pinctrl hardware must be handled by a
> > > system manager entity. So mmio direct access not allowed from Cortex-A
> > > side.
> >
> > Yeah I understood as much. But I don't think that the firmware is really
> > filtering any of the access, it will just poke into any pinctrl register as
> > instructed anyway so what's the point. Just looks like a layer of indirection.
>
> No, the firmware has a check on whether a pin is allowed to be configured
> by the agent that wanna to configure the pin.
>
> > But I'm not your system manager, so it's not my decision.
> >
> > > The SCMI firmware is very straightforward, there is no group or
> > > function.
> > >
> > > It just accepts the format as this:
> > > MUX_TYPE, MUX VALUE, CONF_TYPE, CONF_VAL, DAISY_TYPE, DAISY ID,
> > > DAISY_CFG, DAISY_VALUE.
> > >
> > > Similar as linux MMIO format.
> > >
> > > Our i.MX95 platform will support two settings, one with SCMI firmware,
> > > one without SCMI. These two settings will share the same pinctrl
> > > header file.
> > >
> > > And to simplify the scmi firmware design(anyway I am not owner of the
> > > firmware), to make pinctrl header shared w/o scmi, we take the current
> > > in-upstream freescale imx binding format.
> >
> > The SCMI people will have to state their position on this.
> > Like what they consider conformance and what extensions are allowed. This
> > is more a standardization question than an implementation question so it's
> > not really my turf.
>
> The i.MX95 SCMI firmware uses OEM extension type. So I just follow
> what the firmware did and support it in linux. Anyway let's
> wait Sudeep's reply.
>
So my unsderstanding on this matter as of now is that:
1. the current SCMI Pinctrl specification can support your usecase by using
OEM Types and multiple pins/values CONFIG_GET/SET commands
2. the Kernel SCMI protocol layer (driver/firmware/arm_scmi/pinctrl.c)
is equally fine and can support your usecase, AFTER Oleksii fixes it to
align it to the latest v3.2-BETA2 specification changes.
IOW, this means that, using the SCMI Pinctrl protocol operations
exposed in scmi_protocol.h, from somewhere, you are able to properly
configure multiple pins/values with your specific OEM types.
3. The SCMI Pinctrl driver (by Oleksii) built on top of the pinctrl protocol
operations is instead NOT suitable for your usecase since it uses the Linux
Generic Pinconf and IMX does not make use of it, and instead IMX has
its own bindings and related parsing logic.
Am I right ?
If this is the case, I would NOT try to abuse the current SCMI Pinctrl
Generic driver (by Oleksii) by throwing into it a bunch of IMX specific DT
parsing, also because you'll end-up NOT using most of the generic SCMI
Pinctrl driver but just reusing a bit of the probe (customized with your
own DT maps parsing)
Instead, given that the spec[1.] and the protocol layer[2.] are fine for
your use case and you indeed have already a custom way to parse your DT
mappings, I would say that you could just write your own custom SCMI
driver ( ? pinctrl-imx-scmi), distinct and much more simple than the generic one,
that does its own IMX DT parsing and calls just the SCMI protocol operations
that it needs in the way that your platform expects: so basically another
Pinctrl SCMI driver that does not use the generic pinconf DT
configuration BUT DO USE the underlying SCMI Pinctrl protocol (via its
exposed protocol operations...)
Not sure what Sudeep thinks about supporting multiple SCMI driver for the
same protocol (we did it already for Sensors hwmon && iio), and if this
approach won't need some sort of mutual exclusion mechanism in Kconfig to
avoid loading both the generic and the custom IMX (even though they should
be able to co-exist from the SCMI kernel/fw stack pint of view, as long as
you dont mess-up the DTs and mixup generic pins with custom IMX pins...)
Instead, adding an IMX-custom extension to what it was supposed to be a generic
driver (as you propose) seems to me like a stretch of the generic Pinctrl driver
that is not really worth, since you'll end up polluting the generic driver with
some highly custom and specific IMX bits. while really NOT reusing so much of
the generic driver at all.
Thanks,
Cristian
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 30+ messages in thread
* RE: [RFC] scmi: pinctrl: support i.MX9
2023-08-30 11:47 ` Cristian Marussi
@ 2023-08-30 12:48 ` Peng Fan
-1 siblings, 0 replies; 30+ messages in thread
From: Peng Fan @ 2023-08-30 12:48 UTC (permalink / raw)
To: Cristian Marussi
Cc: Linus Walleij, Peng Fan (OSS), oleksii_moisieiev@epam.com,
sudeep.holla@arm.com, Aisheng Dong, festevam@gmail.com, Jacky Bai,
s.hauer@pengutronix.de, shawnguo@kernel.org,
kernel@pengutronix.de, dl-linux-imx,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org
Hi Cristian,
> Subject: Re: [RFC] scmi: pinctrl: support i.MX9
>
> On Fri, Aug 25, 2023 at 08:43:38AM +0000, Peng Fan wrote:
> > > Subject: Re: [RFC] scmi: pinctrl: support i.MX9
> > >
> > > On Thu, Aug 24, 2023 at 2:47 PM Peng Fan <peng.fan@nxp.com> wrote:
> > > > Me:
>
> Hi Peng,
>
> > >
> > > >> it is merely making things more complex and also slower
> > > > > bymaking the registers only accessible from this SCMI link.
> > > >
> > > > This is for safety reason, the pinctrl hardware must be handled by
> > > > a system manager entity. So mmio direct access not allowed from
> > > > Cortex-A side.
> > >
> > > Yeah I understood as much. But I don't think that the firmware is
> > > really filtering any of the access, it will just poke into any
> > > pinctrl register as instructed anyway so what's the point. Just looks like a
> layer of indirection.
> >
> > No, the firmware has a check on whether a pin is allowed to be
> > configured by the agent that wanna to configure the pin.
> >
> > > But I'm not your system manager, so it's not my decision.
> > >
> > > > The SCMI firmware is very straightforward, there is no group or
> > > > function.
> > > >
> > > > It just accepts the format as this:
> > > > MUX_TYPE, MUX VALUE, CONF_TYPE, CONF_VAL, DAISY_TYPE, DAISY
> ID,
> > > > DAISY_CFG, DAISY_VALUE.
> > > >
> > > > Similar as linux MMIO format.
> > > >
> > > > Our i.MX95 platform will support two settings, one with SCMI
> > > > firmware, one without SCMI. These two settings will share the same
> > > > pinctrl header file.
> > > >
> > > > And to simplify the scmi firmware design(anyway I am not owner of
> > > > the firmware), to make pinctrl header shared w/o scmi, we take the
> > > > current in-upstream freescale imx binding format.
> > >
> > > The SCMI people will have to state their position on this.
> > > Like what they consider conformance and what extensions are allowed.
> > > This is more a standardization question than an implementation
> > > question so it's not really my turf.
> >
> > The i.MX95 SCMI firmware uses OEM extension type. So I just follow
> > what the firmware did and support it in linux. Anyway let's wait
> > Sudeep's reply.
> >
>
> So my unsderstanding on this matter as of now is that:
>
> 1. the current SCMI Pinctrl specification can support your usecase by using
> OEM Types and multiple pins/values CONFIG_GET/SET commands
Yes, based on the Oleksii patchset with my local multiple configs support.
>
> 2. the Kernel SCMI protocol layer (driver/firmware/arm_scmi/pinctrl.c)
> is equally fine and can support your usecase, AFTER Oleksii fixes it to
> align it to the latest v3.2-BETA2 specification changes.
> IOW, this means that, using the SCMI Pinctrl protocol operations
> exposed in scmi_protocol.h, from somewhere, you are able to properly
> configure multiple pins/values with your specific OEM types.
Yes.
>
> 3. The SCMI Pinctrl driver (by Oleksii) built on top of the pinctrl protocol
> operations is instead NOT suitable for your usecase since it uses the Linux
> Generic Pinconf and IMX does not make use of it, and instead IMX has
> its own bindings and related parsing logic.
Yes.
>
> Am I right ?
You are right.
>
> If this is the case, I would NOT try to abuse the current SCMI Pinctrl Generic
> driver (by Oleksii) by throwing into it a bunch of IMX specific DT parsing,
> also because you'll end-up NOT using most of the generic SCMI Pinctrl driver
> but just reusing a bit of the probe (customized with your own DT maps
> parsing)
Only DT map to parse the dts and map to config array. Others are same,
so need to export some symbols for pinctrl-scmi-imx.c driver if build imx
scmi driver.
>
> Instead, given that the spec[1.] and the protocol layer[2.] are fine for your
> use case and you indeed have already a custom way to parse your DT
> mappings, I would say that you could just write your own custom SCMI
> driver ( ? pinctrl-imx-scmi), distinct and much more simple than the generic
> one, that does its own IMX DT parsing and calls just the SCMI protocol
> operations that it needs in the way that your platform expects: so basically
> another Pinctrl SCMI driver that does not use the generic pinconf DT
> configuration BUT DO USE the underlying SCMI Pinctrl protocol (via its
> exposed protocol operations...)
I am ok with this approach, but I need use the other ID, saying 0x99, not 0x19,
because 0x19 will bind with the pinctrl-scmi.c driver, I could not reuse
this ID for i.MX pinctrl-scmi-imx driver. Otherwise there will be issue if both
driver are built in kernel image.
>
> Not sure what Sudeep thinks about supporting multiple SCMI driver for the
> same protocol (we did it already for Sensors hwmon && iio), and if this
> approach won't need some sort of mutual exclusion mechanism in Kconfig
> to avoid loading both the generic and the custom IMX (even though they
> should be able to co-exist from the SCMI kernel/fw stack pint of view, as
> long as you dont mess-up the DTs and mixup generic pins with custom IMX
> pins...)
>
> Instead, adding an IMX-custom extension to what it was supposed to be a
> generic driver (as you propose) seems to me like a stretch of the generic
> Pinctrl driver that is not really worth, since you'll end up polluting the
> generic driver with some highly custom and specific IMX bits. while really
> NOT reusing so much of the generic driver at all.
If Sudeep agree, I will follow your suggestion and post a patch for review, later.
Thanks,
Peng.
>
> Thanks,
> Cristian
^ permalink raw reply [flat|nested] 30+ messages in thread
* RE: [RFC] scmi: pinctrl: support i.MX9
@ 2023-08-30 12:48 ` Peng Fan
0 siblings, 0 replies; 30+ messages in thread
From: Peng Fan @ 2023-08-30 12:48 UTC (permalink / raw)
To: Cristian Marussi
Cc: Linus Walleij, Peng Fan (OSS), oleksii_moisieiev@epam.com,
sudeep.holla@arm.com, Aisheng Dong, festevam@gmail.com, Jacky Bai,
s.hauer@pengutronix.de, shawnguo@kernel.org,
kernel@pengutronix.de, dl-linux-imx,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org
Hi Cristian,
> Subject: Re: [RFC] scmi: pinctrl: support i.MX9
>
> On Fri, Aug 25, 2023 at 08:43:38AM +0000, Peng Fan wrote:
> > > Subject: Re: [RFC] scmi: pinctrl: support i.MX9
> > >
> > > On Thu, Aug 24, 2023 at 2:47 PM Peng Fan <peng.fan@nxp.com> wrote:
> > > > Me:
>
> Hi Peng,
>
> > >
> > > >> it is merely making things more complex and also slower
> > > > > bymaking the registers only accessible from this SCMI link.
> > > >
> > > > This is for safety reason, the pinctrl hardware must be handled by
> > > > a system manager entity. So mmio direct access not allowed from
> > > > Cortex-A side.
> > >
> > > Yeah I understood as much. But I don't think that the firmware is
> > > really filtering any of the access, it will just poke into any
> > > pinctrl register as instructed anyway so what's the point. Just looks like a
> layer of indirection.
> >
> > No, the firmware has a check on whether a pin is allowed to be
> > configured by the agent that wanna to configure the pin.
> >
> > > But I'm not your system manager, so it's not my decision.
> > >
> > > > The SCMI firmware is very straightforward, there is no group or
> > > > function.
> > > >
> > > > It just accepts the format as this:
> > > > MUX_TYPE, MUX VALUE, CONF_TYPE, CONF_VAL, DAISY_TYPE, DAISY
> ID,
> > > > DAISY_CFG, DAISY_VALUE.
> > > >
> > > > Similar as linux MMIO format.
> > > >
> > > > Our i.MX95 platform will support two settings, one with SCMI
> > > > firmware, one without SCMI. These two settings will share the same
> > > > pinctrl header file.
> > > >
> > > > And to simplify the scmi firmware design(anyway I am not owner of
> > > > the firmware), to make pinctrl header shared w/o scmi, we take the
> > > > current in-upstream freescale imx binding format.
> > >
> > > The SCMI people will have to state their position on this.
> > > Like what they consider conformance and what extensions are allowed.
> > > This is more a standardization question than an implementation
> > > question so it's not really my turf.
> >
> > The i.MX95 SCMI firmware uses OEM extension type. So I just follow
> > what the firmware did and support it in linux. Anyway let's wait
> > Sudeep's reply.
> >
>
> So my unsderstanding on this matter as of now is that:
>
> 1. the current SCMI Pinctrl specification can support your usecase by using
> OEM Types and multiple pins/values CONFIG_GET/SET commands
Yes, based on the Oleksii patchset with my local multiple configs support.
>
> 2. the Kernel SCMI protocol layer (driver/firmware/arm_scmi/pinctrl.c)
> is equally fine and can support your usecase, AFTER Oleksii fixes it to
> align it to the latest v3.2-BETA2 specification changes.
> IOW, this means that, using the SCMI Pinctrl protocol operations
> exposed in scmi_protocol.h, from somewhere, you are able to properly
> configure multiple pins/values with your specific OEM types.
Yes.
>
> 3. The SCMI Pinctrl driver (by Oleksii) built on top of the pinctrl protocol
> operations is instead NOT suitable for your usecase since it uses the Linux
> Generic Pinconf and IMX does not make use of it, and instead IMX has
> its own bindings and related parsing logic.
Yes.
>
> Am I right ?
You are right.
>
> If this is the case, I would NOT try to abuse the current SCMI Pinctrl Generic
> driver (by Oleksii) by throwing into it a bunch of IMX specific DT parsing,
> also because you'll end-up NOT using most of the generic SCMI Pinctrl driver
> but just reusing a bit of the probe (customized with your own DT maps
> parsing)
Only DT map to parse the dts and map to config array. Others are same,
so need to export some symbols for pinctrl-scmi-imx.c driver if build imx
scmi driver.
>
> Instead, given that the spec[1.] and the protocol layer[2.] are fine for your
> use case and you indeed have already a custom way to parse your DT
> mappings, I would say that you could just write your own custom SCMI
> driver ( ? pinctrl-imx-scmi), distinct and much more simple than the generic
> one, that does its own IMX DT parsing and calls just the SCMI protocol
> operations that it needs in the way that your platform expects: so basically
> another Pinctrl SCMI driver that does not use the generic pinconf DT
> configuration BUT DO USE the underlying SCMI Pinctrl protocol (via its
> exposed protocol operations...)
I am ok with this approach, but I need use the other ID, saying 0x99, not 0x19,
because 0x19 will bind with the pinctrl-scmi.c driver, I could not reuse
this ID for i.MX pinctrl-scmi-imx driver. Otherwise there will be issue if both
driver are built in kernel image.
>
> Not sure what Sudeep thinks about supporting multiple SCMI driver for the
> same protocol (we did it already for Sensors hwmon && iio), and if this
> approach won't need some sort of mutual exclusion mechanism in Kconfig
> to avoid loading both the generic and the custom IMX (even though they
> should be able to co-exist from the SCMI kernel/fw stack pint of view, as
> long as you dont mess-up the DTs and mixup generic pins with custom IMX
> pins...)
>
> Instead, adding an IMX-custom extension to what it was supposed to be a
> generic driver (as you propose) seems to me like a stretch of the generic
> Pinctrl driver that is not really worth, since you'll end up polluting the
> generic driver with some highly custom and specific IMX bits. while really
> NOT reusing so much of the generic driver at all.
If Sudeep agree, I will follow your suggestion and post a patch for review, later.
Thanks,
Peng.
>
> Thanks,
> Cristian
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [RFC] scmi: pinctrl: support i.MX9
2023-08-30 12:48 ` Peng Fan
@ 2023-08-30 13:37 ` Cristian Marussi
-1 siblings, 0 replies; 30+ messages in thread
From: Cristian Marussi @ 2023-08-30 13:37 UTC (permalink / raw)
To: Peng Fan
Cc: Linus Walleij, Peng Fan (OSS), oleksii_moisieiev@epam.com,
sudeep.holla@arm.com, Aisheng Dong, festevam@gmail.com, Jacky Bai,
s.hauer@pengutronix.de, shawnguo@kernel.org,
kernel@pengutronix.de, dl-linux-imx,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org
On Wed, Aug 30, 2023 at 12:48:37PM +0000, Peng Fan wrote:
> Hi Cristian,
>
Hi,
> > Subject: Re: [RFC] scmi: pinctrl: support i.MX9
> >
> > On Fri, Aug 25, 2023 at 08:43:38AM +0000, Peng Fan wrote:
> > > > Subject: Re: [RFC] scmi: pinctrl: support i.MX9
> > > >
> > > > On Thu, Aug 24, 2023 at 2:47 PM Peng Fan <peng.fan@nxp.com> wrote:
> > > > > Me:
> >
> > Hi Peng,
> >
> > > >
> > > > >> it is merely making things more complex and also slower
> > > > > > bymaking the registers only accessible from this SCMI link.
> > > > >
> > > > > This is for safety reason, the pinctrl hardware must be handled by
> > > > > a system manager entity. So mmio direct access not allowed from
> > > > > Cortex-A side.
> > > >
> > > > Yeah I understood as much. But I don't think that the firmware is
> > > > really filtering any of the access, it will just poke into any
> > > > pinctrl register as instructed anyway so what's the point. Just looks like a
> > layer of indirection.
> > >
> > > No, the firmware has a check on whether a pin is allowed to be
> > > configured by the agent that wanna to configure the pin.
> > >
> > > > But I'm not your system manager, so it's not my decision.
> > > >
> > > > > The SCMI firmware is very straightforward, there is no group or
> > > > > function.
> > > > >
> > > > > It just accepts the format as this:
> > > > > MUX_TYPE, MUX VALUE, CONF_TYPE, CONF_VAL, DAISY_TYPE, DAISY
> > ID,
> > > > > DAISY_CFG, DAISY_VALUE.
> > > > >
> > > > > Similar as linux MMIO format.
> > > > >
> > > > > Our i.MX95 platform will support two settings, one with SCMI
> > > > > firmware, one without SCMI. These two settings will share the same
> > > > > pinctrl header file.
> > > > >
> > > > > And to simplify the scmi firmware design(anyway I am not owner of
> > > > > the firmware), to make pinctrl header shared w/o scmi, we take the
> > > > > current in-upstream freescale imx binding format.
> > > >
> > > > The SCMI people will have to state their position on this.
> > > > Like what they consider conformance and what extensions are allowed.
> > > > This is more a standardization question than an implementation
> > > > question so it's not really my turf.
> > >
> > > The i.MX95 SCMI firmware uses OEM extension type. So I just follow
> > > what the firmware did and support it in linux. Anyway let's wait
> > > Sudeep's reply.
> > >
> >
> > So my unsderstanding on this matter as of now is that:
> >
> > 1. the current SCMI Pinctrl specification can support your usecase by using
> > OEM Types and multiple pins/values CONFIG_GET/SET commands
>
> Yes, based on the Oleksii patchset with my local multiple configs support.
>
Yes, I know, I pointed out on his series that the protocol has still to
be fixed to be aligned with the latest BETA2 spec (we changed the spec
on the fly while he was already posting indeed..)
> >
> > 2. the Kernel SCMI protocol layer (driver/firmware/arm_scmi/pinctrl.c)
> > is equally fine and can support your usecase, AFTER Oleksii fixes it to
> > align it to the latest v3.2-BETA2 specification changes.
> > IOW, this means that, using the SCMI Pinctrl protocol operations
> > exposed in scmi_protocol.h, from somewhere, you are able to properly
> > configure multiple pins/values with your specific OEM types.
>
> Yes.
Good.
>
> >
> > 3. The SCMI Pinctrl driver (by Oleksii) built on top of the pinctrl protocol
> > operations is instead NOT suitable for your usecase since it uses the Linux
> > Generic Pinconf and IMX does not make use of it, and instead IMX has
> > its own bindings and related parsing logic.
>
> Yes.
>
> >
> > Am I right ?
>
> You are right.
>
> >
> > If this is the case, I would NOT try to abuse the current SCMI Pinctrl Generic
> > driver (by Oleksii) by throwing into it a bunch of IMX specific DT parsing,
> > also because you'll end-up NOT using most of the generic SCMI Pinctrl driver
> > but just reusing a bit of the probe (customized with your own DT maps
> > parsing)
>
> Only DT map to parse the dts and map to config array. Others are same,
> so need to export some symbols for pinctrl-scmi-imx.c driver if build imx
> scmi driver.
>
Yes, but you are basically using some exported symbol to parse the DT in
your way and then you do not use anything of the various
functions/groups stuff...you just leverage some of the probing stuff and
then issue you OEM Type configs....I mean most of the picntrl-scmi
driver would be unused anyway in this scenario.
> >
> > Instead, given that the spec[1.] and the protocol layer[2.] are fine for your
> > use case and you indeed have already a custom way to parse your DT
> > mappings, I would say that you could just write your own custom SCMI
> > driver ( ? pinctrl-imx-scmi), distinct and much more simple than the generic
> > one, that does its own IMX DT parsing and calls just the SCMI protocol
> > operations that it needs in the way that your platform expects: so basically
> > another Pinctrl SCMI driver that does not use the generic pinconf DT
> > configuration BUT DO USE the underlying SCMI Pinctrl protocol (via its
> > exposed protocol operations...)
>
> I am ok with this approach, but I need use the other ID, saying 0x99, not 0x19,
> because 0x19 will bind with the pinctrl-scmi.c driver, I could not reuse
> this ID for i.MX pinctrl-scmi-imx driver. Otherwise there will be issue if both
> driver are built in kernel image.
>
Ok here I lost you.
The protocol ID 0x19 is bound to the protocol layer and identifies the
standard Pinctrl protocol: usually you use a 0x99 to define and describe
you own specific NEW vendor protocol, BUT here you are saying you are fine to
use std Pinctrl spec AND the protocol operations as exposed in pinctrl.c, so
I dont see why you should use a new vendor protocol_id to basically
expose the same operations. (and I also dont see how you can do that
without hacks in the current codebase)
You CAN have multiple SCMI drivers using the same protocol at the same
time (even more than one protocol at the same time), even though we try
to avoid it if there are no good reason to have more than one driver, there
is nothing in the spec or in the current SCMI platform or agent stacks that
inhibits such scenario (and I use iot heavily for my offline testing
indeed.)
Look at:
- drivers/hwmon/scmi-hwmon
- drivers/iio/common/scmi_sensors/scmi_iio.c
and you'll see that these 2 drivers uses the same SENSOR protocol, just for
different sensor types so they do not interfere one with each other.
What happens is that the first driver using a protocol causes its
protocol_init to be called once for all.
This should work flawlessly like this, if this is not the case for some
reason, this will have to be fixed in the protocol implementation: you
are supposed to be able to grab the same protocol from different
drivers without any issue.
I agree that you have to be careful not to share the same pins across 2
different drivers using the same Pinctrl driver, but even if both driver
are compiled in, nothing is really happening until the related DT
binding are parsed, and so unless you mismatch your DT and assign same
pins to both the Generic SCMI Pinctrl and to the IMX SCMI Pinctrl I dont
see how they can interfere. You could indeed, have a set of pins managed
by your custom IMX driver and one distinct other set of pins handled by
the SCMI Generic driver by Oleksii, both magically handled by the same
SCMI Server backend :P !
BUT to be on the safe side you could anyway force a conflict in Kconfig
to mutually exclude one driver when the other is built and vice-versa.
Am I missing something ? Why would you need a new vendor ID to define a
new protocol without not really having any new protocol ?
Thanks,
Cristian
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [RFC] scmi: pinctrl: support i.MX9
@ 2023-08-30 13:37 ` Cristian Marussi
0 siblings, 0 replies; 30+ messages in thread
From: Cristian Marussi @ 2023-08-30 13:37 UTC (permalink / raw)
To: Peng Fan
Cc: Linus Walleij, Peng Fan (OSS), oleksii_moisieiev@epam.com,
sudeep.holla@arm.com, Aisheng Dong, festevam@gmail.com, Jacky Bai,
s.hauer@pengutronix.de, shawnguo@kernel.org,
kernel@pengutronix.de, dl-linux-imx,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org
On Wed, Aug 30, 2023 at 12:48:37PM +0000, Peng Fan wrote:
> Hi Cristian,
>
Hi,
> > Subject: Re: [RFC] scmi: pinctrl: support i.MX9
> >
> > On Fri, Aug 25, 2023 at 08:43:38AM +0000, Peng Fan wrote:
> > > > Subject: Re: [RFC] scmi: pinctrl: support i.MX9
> > > >
> > > > On Thu, Aug 24, 2023 at 2:47 PM Peng Fan <peng.fan@nxp.com> wrote:
> > > > > Me:
> >
> > Hi Peng,
> >
> > > >
> > > > >> it is merely making things more complex and also slower
> > > > > > bymaking the registers only accessible from this SCMI link.
> > > > >
> > > > > This is for safety reason, the pinctrl hardware must be handled by
> > > > > a system manager entity. So mmio direct access not allowed from
> > > > > Cortex-A side.
> > > >
> > > > Yeah I understood as much. But I don't think that the firmware is
> > > > really filtering any of the access, it will just poke into any
> > > > pinctrl register as instructed anyway so what's the point. Just looks like a
> > layer of indirection.
> > >
> > > No, the firmware has a check on whether a pin is allowed to be
> > > configured by the agent that wanna to configure the pin.
> > >
> > > > But I'm not your system manager, so it's not my decision.
> > > >
> > > > > The SCMI firmware is very straightforward, there is no group or
> > > > > function.
> > > > >
> > > > > It just accepts the format as this:
> > > > > MUX_TYPE, MUX VALUE, CONF_TYPE, CONF_VAL, DAISY_TYPE, DAISY
> > ID,
> > > > > DAISY_CFG, DAISY_VALUE.
> > > > >
> > > > > Similar as linux MMIO format.
> > > > >
> > > > > Our i.MX95 platform will support two settings, one with SCMI
> > > > > firmware, one without SCMI. These two settings will share the same
> > > > > pinctrl header file.
> > > > >
> > > > > And to simplify the scmi firmware design(anyway I am not owner of
> > > > > the firmware), to make pinctrl header shared w/o scmi, we take the
> > > > > current in-upstream freescale imx binding format.
> > > >
> > > > The SCMI people will have to state their position on this.
> > > > Like what they consider conformance and what extensions are allowed.
> > > > This is more a standardization question than an implementation
> > > > question so it's not really my turf.
> > >
> > > The i.MX95 SCMI firmware uses OEM extension type. So I just follow
> > > what the firmware did and support it in linux. Anyway let's wait
> > > Sudeep's reply.
> > >
> >
> > So my unsderstanding on this matter as of now is that:
> >
> > 1. the current SCMI Pinctrl specification can support your usecase by using
> > OEM Types and multiple pins/values CONFIG_GET/SET commands
>
> Yes, based on the Oleksii patchset with my local multiple configs support.
>
Yes, I know, I pointed out on his series that the protocol has still to
be fixed to be aligned with the latest BETA2 spec (we changed the spec
on the fly while he was already posting indeed..)
> >
> > 2. the Kernel SCMI protocol layer (driver/firmware/arm_scmi/pinctrl.c)
> > is equally fine and can support your usecase, AFTER Oleksii fixes it to
> > align it to the latest v3.2-BETA2 specification changes.
> > IOW, this means that, using the SCMI Pinctrl protocol operations
> > exposed in scmi_protocol.h, from somewhere, you are able to properly
> > configure multiple pins/values with your specific OEM types.
>
> Yes.
Good.
>
> >
> > 3. The SCMI Pinctrl driver (by Oleksii) built on top of the pinctrl protocol
> > operations is instead NOT suitable for your usecase since it uses the Linux
> > Generic Pinconf and IMX does not make use of it, and instead IMX has
> > its own bindings and related parsing logic.
>
> Yes.
>
> >
> > Am I right ?
>
> You are right.
>
> >
> > If this is the case, I would NOT try to abuse the current SCMI Pinctrl Generic
> > driver (by Oleksii) by throwing into it a bunch of IMX specific DT parsing,
> > also because you'll end-up NOT using most of the generic SCMI Pinctrl driver
> > but just reusing a bit of the probe (customized with your own DT maps
> > parsing)
>
> Only DT map to parse the dts and map to config array. Others are same,
> so need to export some symbols for pinctrl-scmi-imx.c driver if build imx
> scmi driver.
>
Yes, but you are basically using some exported symbol to parse the DT in
your way and then you do not use anything of the various
functions/groups stuff...you just leverage some of the probing stuff and
then issue you OEM Type configs....I mean most of the picntrl-scmi
driver would be unused anyway in this scenario.
> >
> > Instead, given that the spec[1.] and the protocol layer[2.] are fine for your
> > use case and you indeed have already a custom way to parse your DT
> > mappings, I would say that you could just write your own custom SCMI
> > driver ( ? pinctrl-imx-scmi), distinct and much more simple than the generic
> > one, that does its own IMX DT parsing and calls just the SCMI protocol
> > operations that it needs in the way that your platform expects: so basically
> > another Pinctrl SCMI driver that does not use the generic pinconf DT
> > configuration BUT DO USE the underlying SCMI Pinctrl protocol (via its
> > exposed protocol operations...)
>
> I am ok with this approach, but I need use the other ID, saying 0x99, not 0x19,
> because 0x19 will bind with the pinctrl-scmi.c driver, I could not reuse
> this ID for i.MX pinctrl-scmi-imx driver. Otherwise there will be issue if both
> driver are built in kernel image.
>
Ok here I lost you.
The protocol ID 0x19 is bound to the protocol layer and identifies the
standard Pinctrl protocol: usually you use a 0x99 to define and describe
you own specific NEW vendor protocol, BUT here you are saying you are fine to
use std Pinctrl spec AND the protocol operations as exposed in pinctrl.c, so
I dont see why you should use a new vendor protocol_id to basically
expose the same operations. (and I also dont see how you can do that
without hacks in the current codebase)
You CAN have multiple SCMI drivers using the same protocol at the same
time (even more than one protocol at the same time), even though we try
to avoid it if there are no good reason to have more than one driver, there
is nothing in the spec or in the current SCMI platform or agent stacks that
inhibits such scenario (and I use iot heavily for my offline testing
indeed.)
Look at:
- drivers/hwmon/scmi-hwmon
- drivers/iio/common/scmi_sensors/scmi_iio.c
and you'll see that these 2 drivers uses the same SENSOR protocol, just for
different sensor types so they do not interfere one with each other.
What happens is that the first driver using a protocol causes its
protocol_init to be called once for all.
This should work flawlessly like this, if this is not the case for some
reason, this will have to be fixed in the protocol implementation: you
are supposed to be able to grab the same protocol from different
drivers without any issue.
I agree that you have to be careful not to share the same pins across 2
different drivers using the same Pinctrl driver, but even if both driver
are compiled in, nothing is really happening until the related DT
binding are parsed, and so unless you mismatch your DT and assign same
pins to both the Generic SCMI Pinctrl and to the IMX SCMI Pinctrl I dont
see how they can interfere. You could indeed, have a set of pins managed
by your custom IMX driver and one distinct other set of pins handled by
the SCMI Generic driver by Oleksii, both magically handled by the same
SCMI Server backend :P !
BUT to be on the safe side you could anyway force a conflict in Kconfig
to mutually exclude one driver when the other is built and vice-versa.
Am I missing something ? Why would you need a new vendor ID to define a
new protocol without not really having any new protocol ?
Thanks,
Cristian
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [RFC] scmi: pinctrl: support i.MX9
2023-08-30 13:37 ` Cristian Marussi
@ 2023-09-06 5:43 ` AKASHI Takahiro
-1 siblings, 0 replies; 30+ messages in thread
From: AKASHI Takahiro @ 2023-09-06 5:43 UTC (permalink / raw)
To: Cristian Marussi
Cc: Peng Fan, Linus Walleij, Peng Fan (OSS),
oleksii_moisieiev@epam.com, sudeep.holla@arm.com, Aisheng Dong,
festevam@gmail.com, Jacky Bai, s.hauer@pengutronix.de,
shawnguo@kernel.org, kernel@pengutronix.de, dl-linux-imx,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org
On Wed, Aug 30, 2023 at 02:37:48PM +0100, Cristian Marussi wrote:
> On Wed, Aug 30, 2023 at 12:48:37PM +0000, Peng Fan wrote:
> > Hi Cristian,
> >
>
> Hi,
>
> > > Subject: Re: [RFC] scmi: pinctrl: support i.MX9
> > >
> > > On Fri, Aug 25, 2023 at 08:43:38AM +0000, Peng Fan wrote:
> > > > > Subject: Re: [RFC] scmi: pinctrl: support i.MX9
> > > > >
> > > > > On Thu, Aug 24, 2023 at 2:47 PM Peng Fan <peng.fan@nxp.com> wrote:
> > > > > > Me:
> > >
> > > Hi Peng,
> > >
> > > > >
> > > > > >> it is merely making things more complex and also slower
> > > > > > > bymaking the registers only accessible from this SCMI link.
> > > > > >
> > > > > > This is for safety reason, the pinctrl hardware must be handled by
> > > > > > a system manager entity. So mmio direct access not allowed from
> > > > > > Cortex-A side.
> > > > >
> > > > > Yeah I understood as much. But I don't think that the firmware is
> > > > > really filtering any of the access, it will just poke into any
> > > > > pinctrl register as instructed anyway so what's the point. Just looks like a
> > > layer of indirection.
> > > >
> > > > No, the firmware has a check on whether a pin is allowed to be
> > > > configured by the agent that wanna to configure the pin.
> > > >
> > > > > But I'm not your system manager, so it's not my decision.
> > > > >
> > > > > > The SCMI firmware is very straightforward, there is no group or
> > > > > > function.
> > > > > >
> > > > > > It just accepts the format as this:
> > > > > > MUX_TYPE, MUX VALUE, CONF_TYPE, CONF_VAL, DAISY_TYPE, DAISY
> > > ID,
> > > > > > DAISY_CFG, DAISY_VALUE.
> > > > > >
> > > > > > Similar as linux MMIO format.
> > > > > >
> > > > > > Our i.MX95 platform will support two settings, one with SCMI
> > > > > > firmware, one without SCMI. These two settings will share the same
> > > > > > pinctrl header file.
> > > > > >
> > > > > > And to simplify the scmi firmware design(anyway I am not owner of
> > > > > > the firmware), to make pinctrl header shared w/o scmi, we take the
> > > > > > current in-upstream freescale imx binding format.
> > > > >
> > > > > The SCMI people will have to state their position on this.
> > > > > Like what they consider conformance and what extensions are allowed.
> > > > > This is more a standardization question than an implementation
> > > > > question so it's not really my turf.
> > > >
> > > > The i.MX95 SCMI firmware uses OEM extension type. So I just follow
> > > > what the firmware did and support it in linux. Anyway let's wait
> > > > Sudeep's reply.
> > > >
> > >
> > > So my unsderstanding on this matter as of now is that:
> > >
> > > 1. the current SCMI Pinctrl specification can support your usecase by using
> > > OEM Types and multiple pins/values CONFIG_GET/SET commands
> >
> > Yes, based on the Oleksii patchset with my local multiple configs support.
> >
>
> Yes, I know, I pointed out on his series that the protocol has still to
> be fixed to be aligned with the latest BETA2 spec (we changed the spec
> on the fly while he was already posting indeed..)
>
> > >
> > > 2. the Kernel SCMI protocol layer (driver/firmware/arm_scmi/pinctrl.c)
> > > is equally fine and can support your usecase, AFTER Oleksii fixes it to
> > > align it to the latest v3.2-BETA2 specification changes.
> > > IOW, this means that, using the SCMI Pinctrl protocol operations
> > > exposed in scmi_protocol.h, from somewhere, you are able to properly
> > > configure multiple pins/values with your specific OEM types.
> >
> > Yes.
>
> Good.
>
> >
> > >
> > > 3. The SCMI Pinctrl driver (by Oleksii) built on top of the pinctrl protocol
> > > operations is instead NOT suitable for your usecase since it uses the Linux
> > > Generic Pinconf and IMX does not make use of it, and instead IMX has
> > > its own bindings and related parsing logic.
> >
> > Yes.
> >
> > >
> > > Am I right ?
> >
> > You are right.
> >
> > >
> > > If this is the case, I would NOT try to abuse the current SCMI Pinctrl Generic
> > > driver (by Oleksii) by throwing into it a bunch of IMX specific DT parsing,
> > > also because you'll end-up NOT using most of the generic SCMI Pinctrl driver
> > > but just reusing a bit of the probe (customized with your own DT maps
> > > parsing)
> >
> > Only DT map to parse the dts and map to config array. Others are same,
> > so need to export some symbols for pinctrl-scmi-imx.c driver if build imx
> > scmi driver.
> >
>
> Yes, but you are basically using some exported symbol to parse the DT in
> your way and then you do not use anything of the various
> functions/groups stuff...you just leverage some of the probing stuff and
> then issue you OEM Type configs....I mean most of the picntrl-scmi
> driver would be unused anyway in this scenario.
>
> > >
> > > Instead, given that the spec[1.] and the protocol layer[2.] are fine for your
> > > use case and you indeed have already a custom way to parse your DT
> > > mappings, I would say that you could just write your own custom SCMI
> > > driver ( ? pinctrl-imx-scmi), distinct and much more simple than the generic
> > > one, that does its own IMX DT parsing and calls just the SCMI protocol
> > > operations that it needs in the way that your platform expects: so basically
> > > another Pinctrl SCMI driver that does not use the generic pinconf DT
> > > configuration BUT DO USE the underlying SCMI Pinctrl protocol (via its
> > > exposed protocol operations...)
> >
> > I am ok with this approach, but I need use the other ID, saying 0x99, not 0x19,
> > because 0x19 will bind with the pinctrl-scmi.c driver, I could not reuse
> > this ID for i.MX pinctrl-scmi-imx driver. Otherwise there will be issue if both
> > driver are built in kernel image.
> >
>
> Ok here I lost you.
>
> The protocol ID 0x19 is bound to the protocol layer and identifies the
> standard Pinctrl protocol: usually you use a 0x99 to define and describe
> you own specific NEW vendor protocol, BUT here you are saying you are fine to
> use std Pinctrl spec AND the protocol operations as exposed in pinctrl.c, so
> I dont see why you should use a new vendor protocol_id to basically
> expose the same operations. (and I also dont see how you can do that
> without hacks in the current codebase)
>
> You CAN have multiple SCMI drivers using the same protocol at the same
> time (even more than one protocol at the same time), even though we try
> to avoid it if there are no good reason to have more than one driver, there
> is nothing in the spec or in the current SCMI platform or agent stacks that
> inhibits such scenario (and I use iot heavily for my offline testing
> indeed.)
>
> Look at:
>
> - drivers/hwmon/scmi-hwmon
> - drivers/iio/common/scmi_sensors/scmi_iio.c
>
> and you'll see that these 2 drivers uses the same SENSOR protocol, just for
> different sensor types so they do not interfere one with each other.
Then, how are those two devices identified in a device tree?
That is the point in Peng's case and why he wants to have a dedicated
protocol id (I don't agree to this, though.)
If we follow Cristian's idea, we may want to have two dt nodes, say
pinctrl-scmi-generic and pinctrl-scmi-imx, as phandles for other device
nodes to refer to pins, respectively.
I think there is currently no mechanism (or binding?) to allow this
except adding a protocol id.
-Takahiro Akashi
> What happens is that the first driver using a protocol causes its
> protocol_init to be called once for all.
>
> This should work flawlessly like this, if this is not the case for some
> reason, this will have to be fixed in the protocol implementation: you
> are supposed to be able to grab the same protocol from different
> drivers without any issue.
>
> I agree that you have to be careful not to share the same pins across 2
> different drivers using the same Pinctrl driver, but even if both driver
> are compiled in, nothing is really happening until the related DT
> binding are parsed, and so unless you mismatch your DT and assign same
> pins to both the Generic SCMI Pinctrl and to the IMX SCMI Pinctrl I dont
> see how they can interfere. You could indeed, have a set of pins managed
> by your custom IMX driver and one distinct other set of pins handled by
> the SCMI Generic driver by Oleksii, both magically handled by the same
> SCMI Server backend :P !
>
> BUT to be on the safe side you could anyway force a conflict in Kconfig
> to mutually exclude one driver when the other is built and vice-versa.
>
> Am I missing something ? Why would you need a new vendor ID to define a
> new protocol without not really having any new protocol ?
>
> Thanks,
> Cristian
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [RFC] scmi: pinctrl: support i.MX9
@ 2023-09-06 5:43 ` AKASHI Takahiro
0 siblings, 0 replies; 30+ messages in thread
From: AKASHI Takahiro @ 2023-09-06 5:43 UTC (permalink / raw)
To: Cristian Marussi
Cc: Peng Fan, Linus Walleij, Peng Fan (OSS),
oleksii_moisieiev@epam.com, sudeep.holla@arm.com, Aisheng Dong,
festevam@gmail.com, Jacky Bai, s.hauer@pengutronix.de,
shawnguo@kernel.org, kernel@pengutronix.de, dl-linux-imx,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org
On Wed, Aug 30, 2023 at 02:37:48PM +0100, Cristian Marussi wrote:
> On Wed, Aug 30, 2023 at 12:48:37PM +0000, Peng Fan wrote:
> > Hi Cristian,
> >
>
> Hi,
>
> > > Subject: Re: [RFC] scmi: pinctrl: support i.MX9
> > >
> > > On Fri, Aug 25, 2023 at 08:43:38AM +0000, Peng Fan wrote:
> > > > > Subject: Re: [RFC] scmi: pinctrl: support i.MX9
> > > > >
> > > > > On Thu, Aug 24, 2023 at 2:47 PM Peng Fan <peng.fan@nxp.com> wrote:
> > > > > > Me:
> > >
> > > Hi Peng,
> > >
> > > > >
> > > > > >> it is merely making things more complex and also slower
> > > > > > > bymaking the registers only accessible from this SCMI link.
> > > > > >
> > > > > > This is for safety reason, the pinctrl hardware must be handled by
> > > > > > a system manager entity. So mmio direct access not allowed from
> > > > > > Cortex-A side.
> > > > >
> > > > > Yeah I understood as much. But I don't think that the firmware is
> > > > > really filtering any of the access, it will just poke into any
> > > > > pinctrl register as instructed anyway so what's the point. Just looks like a
> > > layer of indirection.
> > > >
> > > > No, the firmware has a check on whether a pin is allowed to be
> > > > configured by the agent that wanna to configure the pin.
> > > >
> > > > > But I'm not your system manager, so it's not my decision.
> > > > >
> > > > > > The SCMI firmware is very straightforward, there is no group or
> > > > > > function.
> > > > > >
> > > > > > It just accepts the format as this:
> > > > > > MUX_TYPE, MUX VALUE, CONF_TYPE, CONF_VAL, DAISY_TYPE, DAISY
> > > ID,
> > > > > > DAISY_CFG, DAISY_VALUE.
> > > > > >
> > > > > > Similar as linux MMIO format.
> > > > > >
> > > > > > Our i.MX95 platform will support two settings, one with SCMI
> > > > > > firmware, one without SCMI. These two settings will share the same
> > > > > > pinctrl header file.
> > > > > >
> > > > > > And to simplify the scmi firmware design(anyway I am not owner of
> > > > > > the firmware), to make pinctrl header shared w/o scmi, we take the
> > > > > > current in-upstream freescale imx binding format.
> > > > >
> > > > > The SCMI people will have to state their position on this.
> > > > > Like what they consider conformance and what extensions are allowed.
> > > > > This is more a standardization question than an implementation
> > > > > question so it's not really my turf.
> > > >
> > > > The i.MX95 SCMI firmware uses OEM extension type. So I just follow
> > > > what the firmware did and support it in linux. Anyway let's wait
> > > > Sudeep's reply.
> > > >
> > >
> > > So my unsderstanding on this matter as of now is that:
> > >
> > > 1. the current SCMI Pinctrl specification can support your usecase by using
> > > OEM Types and multiple pins/values CONFIG_GET/SET commands
> >
> > Yes, based on the Oleksii patchset with my local multiple configs support.
> >
>
> Yes, I know, I pointed out on his series that the protocol has still to
> be fixed to be aligned with the latest BETA2 spec (we changed the spec
> on the fly while he was already posting indeed..)
>
> > >
> > > 2. the Kernel SCMI protocol layer (driver/firmware/arm_scmi/pinctrl.c)
> > > is equally fine and can support your usecase, AFTER Oleksii fixes it to
> > > align it to the latest v3.2-BETA2 specification changes.
> > > IOW, this means that, using the SCMI Pinctrl protocol operations
> > > exposed in scmi_protocol.h, from somewhere, you are able to properly
> > > configure multiple pins/values with your specific OEM types.
> >
> > Yes.
>
> Good.
>
> >
> > >
> > > 3. The SCMI Pinctrl driver (by Oleksii) built on top of the pinctrl protocol
> > > operations is instead NOT suitable for your usecase since it uses the Linux
> > > Generic Pinconf and IMX does not make use of it, and instead IMX has
> > > its own bindings and related parsing logic.
> >
> > Yes.
> >
> > >
> > > Am I right ?
> >
> > You are right.
> >
> > >
> > > If this is the case, I would NOT try to abuse the current SCMI Pinctrl Generic
> > > driver (by Oleksii) by throwing into it a bunch of IMX specific DT parsing,
> > > also because you'll end-up NOT using most of the generic SCMI Pinctrl driver
> > > but just reusing a bit of the probe (customized with your own DT maps
> > > parsing)
> >
> > Only DT map to parse the dts and map to config array. Others are same,
> > so need to export some symbols for pinctrl-scmi-imx.c driver if build imx
> > scmi driver.
> >
>
> Yes, but you are basically using some exported symbol to parse the DT in
> your way and then you do not use anything of the various
> functions/groups stuff...you just leverage some of the probing stuff and
> then issue you OEM Type configs....I mean most of the picntrl-scmi
> driver would be unused anyway in this scenario.
>
> > >
> > > Instead, given that the spec[1.] and the protocol layer[2.] are fine for your
> > > use case and you indeed have already a custom way to parse your DT
> > > mappings, I would say that you could just write your own custom SCMI
> > > driver ( ? pinctrl-imx-scmi), distinct and much more simple than the generic
> > > one, that does its own IMX DT parsing and calls just the SCMI protocol
> > > operations that it needs in the way that your platform expects: so basically
> > > another Pinctrl SCMI driver that does not use the generic pinconf DT
> > > configuration BUT DO USE the underlying SCMI Pinctrl protocol (via its
> > > exposed protocol operations...)
> >
> > I am ok with this approach, but I need use the other ID, saying 0x99, not 0x19,
> > because 0x19 will bind with the pinctrl-scmi.c driver, I could not reuse
> > this ID for i.MX pinctrl-scmi-imx driver. Otherwise there will be issue if both
> > driver are built in kernel image.
> >
>
> Ok here I lost you.
>
> The protocol ID 0x19 is bound to the protocol layer and identifies the
> standard Pinctrl protocol: usually you use a 0x99 to define and describe
> you own specific NEW vendor protocol, BUT here you are saying you are fine to
> use std Pinctrl spec AND the protocol operations as exposed in pinctrl.c, so
> I dont see why you should use a new vendor protocol_id to basically
> expose the same operations. (and I also dont see how you can do that
> without hacks in the current codebase)
>
> You CAN have multiple SCMI drivers using the same protocol at the same
> time (even more than one protocol at the same time), even though we try
> to avoid it if there are no good reason to have more than one driver, there
> is nothing in the spec or in the current SCMI platform or agent stacks that
> inhibits such scenario (and I use iot heavily for my offline testing
> indeed.)
>
> Look at:
>
> - drivers/hwmon/scmi-hwmon
> - drivers/iio/common/scmi_sensors/scmi_iio.c
>
> and you'll see that these 2 drivers uses the same SENSOR protocol, just for
> different sensor types so they do not interfere one with each other.
Then, how are those two devices identified in a device tree?
That is the point in Peng's case and why he wants to have a dedicated
protocol id (I don't agree to this, though.)
If we follow Cristian's idea, we may want to have two dt nodes, say
pinctrl-scmi-generic and pinctrl-scmi-imx, as phandles for other device
nodes to refer to pins, respectively.
I think there is currently no mechanism (or binding?) to allow this
except adding a protocol id.
-Takahiro Akashi
> What happens is that the first driver using a protocol causes its
> protocol_init to be called once for all.
>
> This should work flawlessly like this, if this is not the case for some
> reason, this will have to be fixed in the protocol implementation: you
> are supposed to be able to grab the same protocol from different
> drivers without any issue.
>
> I agree that you have to be careful not to share the same pins across 2
> different drivers using the same Pinctrl driver, but even if both driver
> are compiled in, nothing is really happening until the related DT
> binding are parsed, and so unless you mismatch your DT and assign same
> pins to both the Generic SCMI Pinctrl and to the IMX SCMI Pinctrl I dont
> see how they can interfere. You could indeed, have a set of pins managed
> by your custom IMX driver and one distinct other set of pins handled by
> the SCMI Generic driver by Oleksii, both magically handled by the same
> SCMI Server backend :P !
>
> BUT to be on the safe side you could anyway force a conflict in Kconfig
> to mutually exclude one driver when the other is built and vice-versa.
>
> Am I missing something ? Why would you need a new vendor ID to define a
> new protocol without not really having any new protocol ?
>
> Thanks,
> Cristian
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [RFC] scmi: pinctrl: support i.MX9
2023-09-06 5:43 ` AKASHI Takahiro
@ 2023-09-07 10:05 ` Cristian Marussi
-1 siblings, 0 replies; 30+ messages in thread
From: Cristian Marussi @ 2023-09-07 10:05 UTC (permalink / raw)
To: AKASHI Takahiro, Peng Fan, Linus Walleij, Peng Fan (OSS),
oleksii_moisieiev@epam.com, sudeep.holla@arm.com, Aisheng Dong,
festevam@gmail.com, Jacky Bai, s.hauer@pengutronix.de,
shawnguo@kernel.org, kernel@pengutronix.de, dl-linux-imx,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org
On Wed, Sep 06, 2023 at 02:43:46PM +0900, AKASHI Takahiro wrote:
> On Wed, Aug 30, 2023 at 02:37:48PM +0100, Cristian Marussi wrote:
> > On Wed, Aug 30, 2023 at 12:48:37PM +0000, Peng Fan wrote:
> > > Hi Cristian,
> > >
> >
> > Hi,
> >
Hi,
> > > > Subject: Re: [RFC] scmi: pinctrl: support i.MX9
> > > >
> > > > On Fri, Aug 25, 2023 at 08:43:38AM +0000, Peng Fan wrote:
> > > > > > Subject: Re: [RFC] scmi: pinctrl: support i.MX9
> > > > > >
> > > > > > On Thu, Aug 24, 2023 at 2:47 PM Peng Fan <peng.fan@nxp.com> wrote:
> > > > > > > Me:
> > > >
> > > > Hi Peng,
> > > >
> > > > > >
> > > > > > >> it is merely making things more complex and also slower
> > > > > > > > bymaking the registers only accessible from this SCMI link.
> > > > > > >
> > > > > > > This is for safety reason, the pinctrl hardware must be handled by
> > > > > > > a system manager entity. So mmio direct access not allowed from
> > > > > > > Cortex-A side.
> > > > > >
> > > > > > Yeah I understood as much. But I don't think that the firmware is
> > > > > > really filtering any of the access, it will just poke into any
> > > > > > pinctrl register as instructed anyway so what's the point. Just looks like a
> > > > layer of indirection.
> > > > >
> > > > > No, the firmware has a check on whether a pin is allowed to be
> > > > > configured by the agent that wanna to configure the pin.
> > > > >
> > > > > > But I'm not your system manager, so it's not my decision.
> > > > > >
> > > > > > > The SCMI firmware is very straightforward, there is no group or
> > > > > > > function.
> > > > > > >
> > > > > > > It just accepts the format as this:
> > > > > > > MUX_TYPE, MUX VALUE, CONF_TYPE, CONF_VAL, DAISY_TYPE, DAISY
> > > > ID,
> > > > > > > DAISY_CFG, DAISY_VALUE.
> > > > > > >
> > > > > > > Similar as linux MMIO format.
> > > > > > >
> > > > > > > Our i.MX95 platform will support two settings, one with SCMI
> > > > > > > firmware, one without SCMI. These two settings will share the same
> > > > > > > pinctrl header file.
> > > > > > >
> > > > > > > And to simplify the scmi firmware design(anyway I am not owner of
> > > > > > > the firmware), to make pinctrl header shared w/o scmi, we take the
> > > > > > > current in-upstream freescale imx binding format.
> > > > > >
> > > > > > The SCMI people will have to state their position on this.
> > > > > > Like what they consider conformance and what extensions are allowed.
> > > > > > This is more a standardization question than an implementation
> > > > > > question so it's not really my turf.
> > > > >
> > > > > The i.MX95 SCMI firmware uses OEM extension type. So I just follow
> > > > > what the firmware did and support it in linux. Anyway let's wait
> > > > > Sudeep's reply.
> > > > >
> > > >
> > > > So my unsderstanding on this matter as of now is that:
> > > >
> > > > 1. the current SCMI Pinctrl specification can support your usecase by using
> > > > OEM Types and multiple pins/values CONFIG_GET/SET commands
> > >
> > > Yes, based on the Oleksii patchset with my local multiple configs support.
> > >
> >
> > Yes, I know, I pointed out on his series that the protocol has still to
> > be fixed to be aligned with the latest BETA2 spec (we changed the spec
> > on the fly while he was already posting indeed..)
> >
> > > >
> > > > 2. the Kernel SCMI protocol layer (driver/firmware/arm_scmi/pinctrl.c)
> > > > is equally fine and can support your usecase, AFTER Oleksii fixes it to
> > > > align it to the latest v3.2-BETA2 specification changes.
> > > > IOW, this means that, using the SCMI Pinctrl protocol operations
> > > > exposed in scmi_protocol.h, from somewhere, you are able to properly
> > > > configure multiple pins/values with your specific OEM types.
> > >
> > > Yes.
> >
> > Good.
> >
> > >
> > > >
> > > > 3. The SCMI Pinctrl driver (by Oleksii) built on top of the pinctrl protocol
> > > > operations is instead NOT suitable for your usecase since it uses the Linux
> > > > Generic Pinconf and IMX does not make use of it, and instead IMX has
> > > > its own bindings and related parsing logic.
> > >
> > > Yes.
> > >
> > > >
> > > > Am I right ?
> > >
> > > You are right.
> > >
> > > >
> > > > If this is the case, I would NOT try to abuse the current SCMI Pinctrl Generic
> > > > driver (by Oleksii) by throwing into it a bunch of IMX specific DT parsing,
> > > > also because you'll end-up NOT using most of the generic SCMI Pinctrl driver
> > > > but just reusing a bit of the probe (customized with your own DT maps
> > > > parsing)
> > >
> > > Only DT map to parse the dts and map to config array. Others are same,
> > > so need to export some symbols for pinctrl-scmi-imx.c driver if build imx
> > > scmi driver.
> > >
> >
> > Yes, but you are basically using some exported symbol to parse the DT in
> > your way and then you do not use anything of the various
> > functions/groups stuff...you just leverage some of the probing stuff and
> > then issue you OEM Type configs....I mean most of the picntrl-scmi
> > driver would be unused anyway in this scenario.
> >
> > > >
> > > > Instead, given that the spec[1.] and the protocol layer[2.] are fine for your
> > > > use case and you indeed have already a custom way to parse your DT
> > > > mappings, I would say that you could just write your own custom SCMI
> > > > driver ( ? pinctrl-imx-scmi), distinct and much more simple than the generic
> > > > one, that does its own IMX DT parsing and calls just the SCMI protocol
> > > > operations that it needs in the way that your platform expects: so basically
> > > > another Pinctrl SCMI driver that does not use the generic pinconf DT
> > > > configuration BUT DO USE the underlying SCMI Pinctrl protocol (via its
> > > > exposed protocol operations...)
> > >
> > > I am ok with this approach, but I need use the other ID, saying 0x99, not 0x19,
> > > because 0x19 will bind with the pinctrl-scmi.c driver, I could not reuse
> > > this ID for i.MX pinctrl-scmi-imx driver. Otherwise there will be issue if both
> > > driver are built in kernel image.
> > >
> >
> > Ok here I lost you.
> >
> > The protocol ID 0x19 is bound to the protocol layer and identifies the
> > standard Pinctrl protocol: usually you use a 0x99 to define and describe
> > you own specific NEW vendor protocol, BUT here you are saying you are fine to
> > use std Pinctrl spec AND the protocol operations as exposed in pinctrl.c, so
> > I dont see why you should use a new vendor protocol_id to basically
> > expose the same operations. (and I also dont see how you can do that
> > without hacks in the current codebase)
> >
> > You CAN have multiple SCMI drivers using the same protocol at the same
> > time (even more than one protocol at the same time), even though we try
> > to avoid it if there are no good reason to have more than one driver, there
> > is nothing in the spec or in the current SCMI platform or agent stacks that
> > inhibits such scenario (and I use iot heavily for my offline testing
> > indeed.)
> >
> > Look at:
> >
> > - drivers/hwmon/scmi-hwmon
> > - drivers/iio/common/scmi_sensors/scmi_iio.c
> >
> > and you'll see that these 2 drivers uses the same SENSOR protocol, just for
> > different sensor types so they do not interfere one with each other.
>
> Then, how are those two devices identified in a device tree?
At SCMI probe time the SCMI core stack creates one device for each
'protocol_id/name' as provided by the registered SCMI drivers in
their respective scmi_device_id/id_tableS, as long as the requested
protocol is active in the DT (protocol@XX defined) and the 'name'
is not duplicated; each of these devices, sharing the same SCMI
protocol, are created sharing also the same DT node protocol@XX.
Their respective SCMI drivers are subsequently separately matched on
protocol_id/name and then probed as usual: it is up to the drivers not
to step on each other feet by competing for the same SCMI resources...
...like issuing comflicting request for the same reosurce domain
on the same protocol.
I suppose, though, a lot depends on how the respective drivers interact
with their subsystems, like IIO SCMI does not really need any info
from DT, and the hwmon uses just the phandle to pick the sensor_domain_id
to associate, as an example, to thermal sensors.
> That is the point in Peng's case and why he wants to have a dedicated
> protocol id (I don't agree to this, though.)
> If we follow Cristian's idea, we may want to have two dt nodes, say
> pinctrl-scmi-generic and pinctrl-scmi-imx, as phandles for other device
> nodes to refer to pins, respectively.
> I think there is currently no mechanism (or binding?) to allow this
> except adding a protocol id.
>
Beside the uglyness of having 2 different DT bindings schemas for 2
different drivers coexisting in the same parent protocol node, it is
still not clear to me why this cant be done technically without the need
of a new dummy 0x99 protocol_node, given that each driver can use its
own parsing logic in its probe, which is what Peng is doing in
dt_node_to_map indeed ...even though, maybe, the result would be so
ugly and/or not accepatble from bindings point of view that we will
not want to do it at the end anyway....I mean maybe it is just the
attempt itself to combine the Generic Pinctrl approach with the highly
specific IMX approach that inevitably leads to these clashes...
...I maybe missing something, though, so I'll made some experiments on
my side about this before I'll keep on blabbing :P
Thanks,
Cristian
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [RFC] scmi: pinctrl: support i.MX9
@ 2023-09-07 10:05 ` Cristian Marussi
0 siblings, 0 replies; 30+ messages in thread
From: Cristian Marussi @ 2023-09-07 10:05 UTC (permalink / raw)
To: AKASHI Takahiro, Peng Fan, Linus Walleij, Peng Fan (OSS),
oleksii_moisieiev@epam.com, sudeep.holla@arm.com, Aisheng Dong,
festevam@gmail.com, Jacky Bai, s.hauer@pengutronix.de,
shawnguo@kernel.org, kernel@pengutronix.de, dl-linux-imx,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org
On Wed, Sep 06, 2023 at 02:43:46PM +0900, AKASHI Takahiro wrote:
> On Wed, Aug 30, 2023 at 02:37:48PM +0100, Cristian Marussi wrote:
> > On Wed, Aug 30, 2023 at 12:48:37PM +0000, Peng Fan wrote:
> > > Hi Cristian,
> > >
> >
> > Hi,
> >
Hi,
> > > > Subject: Re: [RFC] scmi: pinctrl: support i.MX9
> > > >
> > > > On Fri, Aug 25, 2023 at 08:43:38AM +0000, Peng Fan wrote:
> > > > > > Subject: Re: [RFC] scmi: pinctrl: support i.MX9
> > > > > >
> > > > > > On Thu, Aug 24, 2023 at 2:47 PM Peng Fan <peng.fan@nxp.com> wrote:
> > > > > > > Me:
> > > >
> > > > Hi Peng,
> > > >
> > > > > >
> > > > > > >> it is merely making things more complex and also slower
> > > > > > > > bymaking the registers only accessible from this SCMI link.
> > > > > > >
> > > > > > > This is for safety reason, the pinctrl hardware must be handled by
> > > > > > > a system manager entity. So mmio direct access not allowed from
> > > > > > > Cortex-A side.
> > > > > >
> > > > > > Yeah I understood as much. But I don't think that the firmware is
> > > > > > really filtering any of the access, it will just poke into any
> > > > > > pinctrl register as instructed anyway so what's the point. Just looks like a
> > > > layer of indirection.
> > > > >
> > > > > No, the firmware has a check on whether a pin is allowed to be
> > > > > configured by the agent that wanna to configure the pin.
> > > > >
> > > > > > But I'm not your system manager, so it's not my decision.
> > > > > >
> > > > > > > The SCMI firmware is very straightforward, there is no group or
> > > > > > > function.
> > > > > > >
> > > > > > > It just accepts the format as this:
> > > > > > > MUX_TYPE, MUX VALUE, CONF_TYPE, CONF_VAL, DAISY_TYPE, DAISY
> > > > ID,
> > > > > > > DAISY_CFG, DAISY_VALUE.
> > > > > > >
> > > > > > > Similar as linux MMIO format.
> > > > > > >
> > > > > > > Our i.MX95 platform will support two settings, one with SCMI
> > > > > > > firmware, one without SCMI. These two settings will share the same
> > > > > > > pinctrl header file.
> > > > > > >
> > > > > > > And to simplify the scmi firmware design(anyway I am not owner of
> > > > > > > the firmware), to make pinctrl header shared w/o scmi, we take the
> > > > > > > current in-upstream freescale imx binding format.
> > > > > >
> > > > > > The SCMI people will have to state their position on this.
> > > > > > Like what they consider conformance and what extensions are allowed.
> > > > > > This is more a standardization question than an implementation
> > > > > > question so it's not really my turf.
> > > > >
> > > > > The i.MX95 SCMI firmware uses OEM extension type. So I just follow
> > > > > what the firmware did and support it in linux. Anyway let's wait
> > > > > Sudeep's reply.
> > > > >
> > > >
> > > > So my unsderstanding on this matter as of now is that:
> > > >
> > > > 1. the current SCMI Pinctrl specification can support your usecase by using
> > > > OEM Types and multiple pins/values CONFIG_GET/SET commands
> > >
> > > Yes, based on the Oleksii patchset with my local multiple configs support.
> > >
> >
> > Yes, I know, I pointed out on his series that the protocol has still to
> > be fixed to be aligned with the latest BETA2 spec (we changed the spec
> > on the fly while he was already posting indeed..)
> >
> > > >
> > > > 2. the Kernel SCMI protocol layer (driver/firmware/arm_scmi/pinctrl.c)
> > > > is equally fine and can support your usecase, AFTER Oleksii fixes it to
> > > > align it to the latest v3.2-BETA2 specification changes.
> > > > IOW, this means that, using the SCMI Pinctrl protocol operations
> > > > exposed in scmi_protocol.h, from somewhere, you are able to properly
> > > > configure multiple pins/values with your specific OEM types.
> > >
> > > Yes.
> >
> > Good.
> >
> > >
> > > >
> > > > 3. The SCMI Pinctrl driver (by Oleksii) built on top of the pinctrl protocol
> > > > operations is instead NOT suitable for your usecase since it uses the Linux
> > > > Generic Pinconf and IMX does not make use of it, and instead IMX has
> > > > its own bindings and related parsing logic.
> > >
> > > Yes.
> > >
> > > >
> > > > Am I right ?
> > >
> > > You are right.
> > >
> > > >
> > > > If this is the case, I would NOT try to abuse the current SCMI Pinctrl Generic
> > > > driver (by Oleksii) by throwing into it a bunch of IMX specific DT parsing,
> > > > also because you'll end-up NOT using most of the generic SCMI Pinctrl driver
> > > > but just reusing a bit of the probe (customized with your own DT maps
> > > > parsing)
> > >
> > > Only DT map to parse the dts and map to config array. Others are same,
> > > so need to export some symbols for pinctrl-scmi-imx.c driver if build imx
> > > scmi driver.
> > >
> >
> > Yes, but you are basically using some exported symbol to parse the DT in
> > your way and then you do not use anything of the various
> > functions/groups stuff...you just leverage some of the probing stuff and
> > then issue you OEM Type configs....I mean most of the picntrl-scmi
> > driver would be unused anyway in this scenario.
> >
> > > >
> > > > Instead, given that the spec[1.] and the protocol layer[2.] are fine for your
> > > > use case and you indeed have already a custom way to parse your DT
> > > > mappings, I would say that you could just write your own custom SCMI
> > > > driver ( ? pinctrl-imx-scmi), distinct and much more simple than the generic
> > > > one, that does its own IMX DT parsing and calls just the SCMI protocol
> > > > operations that it needs in the way that your platform expects: so basically
> > > > another Pinctrl SCMI driver that does not use the generic pinconf DT
> > > > configuration BUT DO USE the underlying SCMI Pinctrl protocol (via its
> > > > exposed protocol operations...)
> > >
> > > I am ok with this approach, but I need use the other ID, saying 0x99, not 0x19,
> > > because 0x19 will bind with the pinctrl-scmi.c driver, I could not reuse
> > > this ID for i.MX pinctrl-scmi-imx driver. Otherwise there will be issue if both
> > > driver are built in kernel image.
> > >
> >
> > Ok here I lost you.
> >
> > The protocol ID 0x19 is bound to the protocol layer and identifies the
> > standard Pinctrl protocol: usually you use a 0x99 to define and describe
> > you own specific NEW vendor protocol, BUT here you are saying you are fine to
> > use std Pinctrl spec AND the protocol operations as exposed in pinctrl.c, so
> > I dont see why you should use a new vendor protocol_id to basically
> > expose the same operations. (and I also dont see how you can do that
> > without hacks in the current codebase)
> >
> > You CAN have multiple SCMI drivers using the same protocol at the same
> > time (even more than one protocol at the same time), even though we try
> > to avoid it if there are no good reason to have more than one driver, there
> > is nothing in the spec or in the current SCMI platform or agent stacks that
> > inhibits such scenario (and I use iot heavily for my offline testing
> > indeed.)
> >
> > Look at:
> >
> > - drivers/hwmon/scmi-hwmon
> > - drivers/iio/common/scmi_sensors/scmi_iio.c
> >
> > and you'll see that these 2 drivers uses the same SENSOR protocol, just for
> > different sensor types so they do not interfere one with each other.
>
> Then, how are those two devices identified in a device tree?
At SCMI probe time the SCMI core stack creates one device for each
'protocol_id/name' as provided by the registered SCMI drivers in
their respective scmi_device_id/id_tableS, as long as the requested
protocol is active in the DT (protocol@XX defined) and the 'name'
is not duplicated; each of these devices, sharing the same SCMI
protocol, are created sharing also the same DT node protocol@XX.
Their respective SCMI drivers are subsequently separately matched on
protocol_id/name and then probed as usual: it is up to the drivers not
to step on each other feet by competing for the same SCMI resources...
...like issuing comflicting request for the same reosurce domain
on the same protocol.
I suppose, though, a lot depends on how the respective drivers interact
with their subsystems, like IIO SCMI does not really need any info
from DT, and the hwmon uses just the phandle to pick the sensor_domain_id
to associate, as an example, to thermal sensors.
> That is the point in Peng's case and why he wants to have a dedicated
> protocol id (I don't agree to this, though.)
> If we follow Cristian's idea, we may want to have two dt nodes, say
> pinctrl-scmi-generic and pinctrl-scmi-imx, as phandles for other device
> nodes to refer to pins, respectively.
> I think there is currently no mechanism (or binding?) to allow this
> except adding a protocol id.
>
Beside the uglyness of having 2 different DT bindings schemas for 2
different drivers coexisting in the same parent protocol node, it is
still not clear to me why this cant be done technically without the need
of a new dummy 0x99 protocol_node, given that each driver can use its
own parsing logic in its probe, which is what Peng is doing in
dt_node_to_map indeed ...even though, maybe, the result would be so
ugly and/or not accepatble from bindings point of view that we will
not want to do it at the end anyway....I mean maybe it is just the
attempt itself to combine the Generic Pinctrl approach with the highly
specific IMX approach that inevitably leads to these clashes...
...I maybe missing something, though, so I'll made some experiments on
my side about this before I'll keep on blabbing :P
Thanks,
Cristian
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [RFC] scmi: pinctrl: support i.MX9
2023-09-07 10:05 ` Cristian Marussi
@ 2023-09-13 9:52 ` AKASHI Takahiro
-1 siblings, 0 replies; 30+ messages in thread
From: AKASHI Takahiro @ 2023-09-13 9:52 UTC (permalink / raw)
To: Cristian Marussi
Cc: Peng Fan, Linus Walleij, Peng Fan (OSS),
oleksii_moisieiev@epam.com, sudeep.holla@arm.com, Aisheng Dong,
festevam@gmail.com, Jacky Bai, s.hauer@pengutronix.de,
shawnguo@kernel.org, kernel@pengutronix.de, dl-linux-imx,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org
On Thu, Sep 07, 2023 at 11:05:52AM +0100, Cristian Marussi wrote:
> On Wed, Sep 06, 2023 at 02:43:46PM +0900, AKASHI Takahiro wrote:
> > On Wed, Aug 30, 2023 at 02:37:48PM +0100, Cristian Marussi wrote:
> > > On Wed, Aug 30, 2023 at 12:48:37PM +0000, Peng Fan wrote:
> > > > Hi Cristian,
> > > >
> > >
> > > Hi,
> > >
>
> Hi,
>
> > > > > Subject: Re: [RFC] scmi: pinctrl: support i.MX9
> > > > >
> > > > > On Fri, Aug 25, 2023 at 08:43:38AM +0000, Peng Fan wrote:
> > > > > > > Subject: Re: [RFC] scmi: pinctrl: support i.MX9
> > > > > > >
> > > > > > > On Thu, Aug 24, 2023 at 2:47 PM Peng Fan <peng.fan@nxp.com> wrote:
> > > > > > > > Me:
> > > > >
> > > > > Hi Peng,
> > > > >
> > > > > > >
> > > > > > > >> it is merely making things more complex and also slower
> > > > > > > > > bymaking the registers only accessible from this SCMI link.
> > > > > > > >
> > > > > > > > This is for safety reason, the pinctrl hardware must be handled by
> > > > > > > > a system manager entity. So mmio direct access not allowed from
> > > > > > > > Cortex-A side.
> > > > > > >
> > > > > > > Yeah I understood as much. But I don't think that the firmware is
> > > > > > > really filtering any of the access, it will just poke into any
> > > > > > > pinctrl register as instructed anyway so what's the point. Just looks like a
> > > > > layer of indirection.
> > > > > >
> > > > > > No, the firmware has a check on whether a pin is allowed to be
> > > > > > configured by the agent that wanna to configure the pin.
> > > > > >
> > > > > > > But I'm not your system manager, so it's not my decision.
> > > > > > >
> > > > > > > > The SCMI firmware is very straightforward, there is no group or
> > > > > > > > function.
> > > > > > > >
> > > > > > > > It just accepts the format as this:
> > > > > > > > MUX_TYPE, MUX VALUE, CONF_TYPE, CONF_VAL, DAISY_TYPE, DAISY
> > > > > ID,
> > > > > > > > DAISY_CFG, DAISY_VALUE.
> > > > > > > >
> > > > > > > > Similar as linux MMIO format.
> > > > > > > >
> > > > > > > > Our i.MX95 platform will support two settings, one with SCMI
> > > > > > > > firmware, one without SCMI. These two settings will share the same
> > > > > > > > pinctrl header file.
> > > > > > > >
> > > > > > > > And to simplify the scmi firmware design(anyway I am not owner of
> > > > > > > > the firmware), to make pinctrl header shared w/o scmi, we take the
> > > > > > > > current in-upstream freescale imx binding format.
> > > > > > >
> > > > > > > The SCMI people will have to state their position on this.
> > > > > > > Like what they consider conformance and what extensions are allowed.
> > > > > > > This is more a standardization question than an implementation
> > > > > > > question so it's not really my turf.
> > > > > >
> > > > > > The i.MX95 SCMI firmware uses OEM extension type. So I just follow
> > > > > > what the firmware did and support it in linux. Anyway let's wait
> > > > > > Sudeep's reply.
> > > > > >
> > > > >
> > > > > So my unsderstanding on this matter as of now is that:
> > > > >
> > > > > 1. the current SCMI Pinctrl specification can support your usecase by using
> > > > > OEM Types and multiple pins/values CONFIG_GET/SET commands
> > > >
> > > > Yes, based on the Oleksii patchset with my local multiple configs support.
> > > >
> > >
> > > Yes, I know, I pointed out on his series that the protocol has still to
> > > be fixed to be aligned with the latest BETA2 spec (we changed the spec
> > > on the fly while he was already posting indeed..)
> > >
> > > > >
> > > > > 2. the Kernel SCMI protocol layer (driver/firmware/arm_scmi/pinctrl.c)
> > > > > is equally fine and can support your usecase, AFTER Oleksii fixes it to
> > > > > align it to the latest v3.2-BETA2 specification changes.
> > > > > IOW, this means that, using the SCMI Pinctrl protocol operations
> > > > > exposed in scmi_protocol.h, from somewhere, you are able to properly
> > > > > configure multiple pins/values with your specific OEM types.
> > > >
> > > > Yes.
> > >
> > > Good.
> > >
> > > >
> > > > >
> > > > > 3. The SCMI Pinctrl driver (by Oleksii) built on top of the pinctrl protocol
> > > > > operations is instead NOT suitable for your usecase since it uses the Linux
> > > > > Generic Pinconf and IMX does not make use of it, and instead IMX has
> > > > > its own bindings and related parsing logic.
> > > >
> > > > Yes.
> > > >
> > > > >
> > > > > Am I right ?
> > > >
> > > > You are right.
> > > >
> > > > >
> > > > > If this is the case, I would NOT try to abuse the current SCMI Pinctrl Generic
> > > > > driver (by Oleksii) by throwing into it a bunch of IMX specific DT parsing,
> > > > > also because you'll end-up NOT using most of the generic SCMI Pinctrl driver
> > > > > but just reusing a bit of the probe (customized with your own DT maps
> > > > > parsing)
> > > >
> > > > Only DT map to parse the dts and map to config array. Others are same,
> > > > so need to export some symbols for pinctrl-scmi-imx.c driver if build imx
> > > > scmi driver.
> > > >
> > >
> > > Yes, but you are basically using some exported symbol to parse the DT in
> > > your way and then you do not use anything of the various
> > > functions/groups stuff...you just leverage some of the probing stuff and
> > > then issue you OEM Type configs....I mean most of the picntrl-scmi
> > > driver would be unused anyway in this scenario.
> > >
> > > > >
> > > > > Instead, given that the spec[1.] and the protocol layer[2.] are fine for your
> > > > > use case and you indeed have already a custom way to parse your DT
> > > > > mappings, I would say that you could just write your own custom SCMI
> > > > > driver ( ? pinctrl-imx-scmi), distinct and much more simple than the generic
> > > > > one, that does its own IMX DT parsing and calls just the SCMI protocol
> > > > > operations that it needs in the way that your platform expects: so basically
> > > > > another Pinctrl SCMI driver that does not use the generic pinconf DT
> > > > > configuration BUT DO USE the underlying SCMI Pinctrl protocol (via its
> > > > > exposed protocol operations...)
> > > >
> > > > I am ok with this approach, but I need use the other ID, saying 0x99, not 0x19,
> > > > because 0x19 will bind with the pinctrl-scmi.c driver, I could not reuse
> > > > this ID for i.MX pinctrl-scmi-imx driver. Otherwise there will be issue if both
> > > > driver are built in kernel image.
> > > >
> > >
> > > Ok here I lost you.
> > >
> > > The protocol ID 0x19 is bound to the protocol layer and identifies the
> > > standard Pinctrl protocol: usually you use a 0x99 to define and describe
> > > you own specific NEW vendor protocol, BUT here you are saying you are fine to
> > > use std Pinctrl spec AND the protocol operations as exposed in pinctrl.c, so
> > > I dont see why you should use a new vendor protocol_id to basically
> > > expose the same operations. (and I also dont see how you can do that
> > > without hacks in the current codebase)
> > >
> > > You CAN have multiple SCMI drivers using the same protocol at the same
> > > time (even more than one protocol at the same time), even though we try
> > > to avoid it if there are no good reason to have more than one driver, there
> > > is nothing in the spec or in the current SCMI platform or agent stacks that
> > > inhibits such scenario (and I use iot heavily for my offline testing
> > > indeed.)
> > >
> > > Look at:
> > >
> > > - drivers/hwmon/scmi-hwmon
> > > - drivers/iio/common/scmi_sensors/scmi_iio.c
> > >
> > > and you'll see that these 2 drivers uses the same SENSOR protocol, just for
> > > different sensor types so they do not interfere one with each other.
> >
> > Then, how are those two devices identified in a device tree?
>
> At SCMI probe time the SCMI core stack creates one device for each
> 'protocol_id/name' as provided by the registered SCMI drivers in
> their respective scmi_device_id/id_tableS, as long as the requested
> protocol is active in the DT (protocol@XX defined) and the 'name'
> is not duplicated; each of these devices, sharing the same SCMI
> protocol, are created sharing also the same DT node protocol@XX.
That's fine.
> Their respective SCMI drivers are subsequently separately matched on
> protocol_id/name and then probed as usual: it is up to the drivers not
> to step on each other feet by competing for the same SCMI resources...
> ...like issuing comflicting request for the same reosurce domain
> on the same protocol.
That's fine.
My question/idea is simple; how two pinctrl drivers, if possible, be
represented in a single device tree. To allow the case of Peng Fan,
for example, I suppose that we need some DT binding like:
scmi {
scmi_pinctrl: protocol@19 {
compatible = "arm, scmi-pinctrl-generic"; // not sure if needed
pinmux ... // standard binding
...
}
}
scmi_pinctrl_fsl {
compatible = "fsl,scmi-pinctrl-imx9";
fsl,pinmux = <&scmi_pinctrl, ...>,
<&scmi_pinctrl, ...>;
...
}
"scmi_pinctrl_fsl" driver may reuse generic SCMI pinctrl helper functions,
except dt_node_to_map, in pictrl_ops.
-Takahiro Akashi
> I suppose, though, a lot depends on how the respective drivers interact
> with their subsystems, like IIO SCMI does not really need any info
> from DT, and the hwmon uses just the phandle to pick the sensor_domain_id
> to associate, as an example, to thermal sensors.
>
> > That is the point in Peng's case and why he wants to have a dedicated
> > protocol id (I don't agree to this, though.)
> > If we follow Cristian's idea, we may want to have two dt nodes, say
> > pinctrl-scmi-generic and pinctrl-scmi-imx, as phandles for other device
> > nodes to refer to pins, respectively.
> > I think there is currently no mechanism (or binding?) to allow this
> > except adding a protocol id.
> >
>
> Beside the uglyness of having 2 different DT bindings schemas for 2
> different drivers coexisting in the same parent protocol node, it is
> still not clear to me why this cant be done technically without the need
> of a new dummy 0x99 protocol_node, given that each driver can use its
> own parsing logic in its probe, which is what Peng is doing in
> dt_node_to_map indeed ...even though, maybe, the result would be so
> ugly and/or not accepatble from bindings point of view that we will
> not want to do it at the end anyway....I mean maybe it is just the
> attempt itself to combine the Generic Pinctrl approach with the highly
> specific IMX approach that inevitably leads to these clashes...
>
> ...I maybe missing something, though, so I'll made some experiments on
> my side about this before I'll keep on blabbing :P
>
> Thanks,
> Cristian
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [RFC] scmi: pinctrl: support i.MX9
@ 2023-09-13 9:52 ` AKASHI Takahiro
0 siblings, 0 replies; 30+ messages in thread
From: AKASHI Takahiro @ 2023-09-13 9:52 UTC (permalink / raw)
To: Cristian Marussi
Cc: Peng Fan, Linus Walleij, Peng Fan (OSS),
oleksii_moisieiev@epam.com, sudeep.holla@arm.com, Aisheng Dong,
festevam@gmail.com, Jacky Bai, s.hauer@pengutronix.de,
shawnguo@kernel.org, kernel@pengutronix.de, dl-linux-imx,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org
On Thu, Sep 07, 2023 at 11:05:52AM +0100, Cristian Marussi wrote:
> On Wed, Sep 06, 2023 at 02:43:46PM +0900, AKASHI Takahiro wrote:
> > On Wed, Aug 30, 2023 at 02:37:48PM +0100, Cristian Marussi wrote:
> > > On Wed, Aug 30, 2023 at 12:48:37PM +0000, Peng Fan wrote:
> > > > Hi Cristian,
> > > >
> > >
> > > Hi,
> > >
>
> Hi,
>
> > > > > Subject: Re: [RFC] scmi: pinctrl: support i.MX9
> > > > >
> > > > > On Fri, Aug 25, 2023 at 08:43:38AM +0000, Peng Fan wrote:
> > > > > > > Subject: Re: [RFC] scmi: pinctrl: support i.MX9
> > > > > > >
> > > > > > > On Thu, Aug 24, 2023 at 2:47 PM Peng Fan <peng.fan@nxp.com> wrote:
> > > > > > > > Me:
> > > > >
> > > > > Hi Peng,
> > > > >
> > > > > > >
> > > > > > > >> it is merely making things more complex and also slower
> > > > > > > > > bymaking the registers only accessible from this SCMI link.
> > > > > > > >
> > > > > > > > This is for safety reason, the pinctrl hardware must be handled by
> > > > > > > > a system manager entity. So mmio direct access not allowed from
> > > > > > > > Cortex-A side.
> > > > > > >
> > > > > > > Yeah I understood as much. But I don't think that the firmware is
> > > > > > > really filtering any of the access, it will just poke into any
> > > > > > > pinctrl register as instructed anyway so what's the point. Just looks like a
> > > > > layer of indirection.
> > > > > >
> > > > > > No, the firmware has a check on whether a pin is allowed to be
> > > > > > configured by the agent that wanna to configure the pin.
> > > > > >
> > > > > > > But I'm not your system manager, so it's not my decision.
> > > > > > >
> > > > > > > > The SCMI firmware is very straightforward, there is no group or
> > > > > > > > function.
> > > > > > > >
> > > > > > > > It just accepts the format as this:
> > > > > > > > MUX_TYPE, MUX VALUE, CONF_TYPE, CONF_VAL, DAISY_TYPE, DAISY
> > > > > ID,
> > > > > > > > DAISY_CFG, DAISY_VALUE.
> > > > > > > >
> > > > > > > > Similar as linux MMIO format.
> > > > > > > >
> > > > > > > > Our i.MX95 platform will support two settings, one with SCMI
> > > > > > > > firmware, one without SCMI. These two settings will share the same
> > > > > > > > pinctrl header file.
> > > > > > > >
> > > > > > > > And to simplify the scmi firmware design(anyway I am not owner of
> > > > > > > > the firmware), to make pinctrl header shared w/o scmi, we take the
> > > > > > > > current in-upstream freescale imx binding format.
> > > > > > >
> > > > > > > The SCMI people will have to state their position on this.
> > > > > > > Like what they consider conformance and what extensions are allowed.
> > > > > > > This is more a standardization question than an implementation
> > > > > > > question so it's not really my turf.
> > > > > >
> > > > > > The i.MX95 SCMI firmware uses OEM extension type. So I just follow
> > > > > > what the firmware did and support it in linux. Anyway let's wait
> > > > > > Sudeep's reply.
> > > > > >
> > > > >
> > > > > So my unsderstanding on this matter as of now is that:
> > > > >
> > > > > 1. the current SCMI Pinctrl specification can support your usecase by using
> > > > > OEM Types and multiple pins/values CONFIG_GET/SET commands
> > > >
> > > > Yes, based on the Oleksii patchset with my local multiple configs support.
> > > >
> > >
> > > Yes, I know, I pointed out on his series that the protocol has still to
> > > be fixed to be aligned with the latest BETA2 spec (we changed the spec
> > > on the fly while he was already posting indeed..)
> > >
> > > > >
> > > > > 2. the Kernel SCMI protocol layer (driver/firmware/arm_scmi/pinctrl.c)
> > > > > is equally fine and can support your usecase, AFTER Oleksii fixes it to
> > > > > align it to the latest v3.2-BETA2 specification changes.
> > > > > IOW, this means that, using the SCMI Pinctrl protocol operations
> > > > > exposed in scmi_protocol.h, from somewhere, you are able to properly
> > > > > configure multiple pins/values with your specific OEM types.
> > > >
> > > > Yes.
> > >
> > > Good.
> > >
> > > >
> > > > >
> > > > > 3. The SCMI Pinctrl driver (by Oleksii) built on top of the pinctrl protocol
> > > > > operations is instead NOT suitable for your usecase since it uses the Linux
> > > > > Generic Pinconf and IMX does not make use of it, and instead IMX has
> > > > > its own bindings and related parsing logic.
> > > >
> > > > Yes.
> > > >
> > > > >
> > > > > Am I right ?
> > > >
> > > > You are right.
> > > >
> > > > >
> > > > > If this is the case, I would NOT try to abuse the current SCMI Pinctrl Generic
> > > > > driver (by Oleksii) by throwing into it a bunch of IMX specific DT parsing,
> > > > > also because you'll end-up NOT using most of the generic SCMI Pinctrl driver
> > > > > but just reusing a bit of the probe (customized with your own DT maps
> > > > > parsing)
> > > >
> > > > Only DT map to parse the dts and map to config array. Others are same,
> > > > so need to export some symbols for pinctrl-scmi-imx.c driver if build imx
> > > > scmi driver.
> > > >
> > >
> > > Yes, but you are basically using some exported symbol to parse the DT in
> > > your way and then you do not use anything of the various
> > > functions/groups stuff...you just leverage some of the probing stuff and
> > > then issue you OEM Type configs....I mean most of the picntrl-scmi
> > > driver would be unused anyway in this scenario.
> > >
> > > > >
> > > > > Instead, given that the spec[1.] and the protocol layer[2.] are fine for your
> > > > > use case and you indeed have already a custom way to parse your DT
> > > > > mappings, I would say that you could just write your own custom SCMI
> > > > > driver ( ? pinctrl-imx-scmi), distinct and much more simple than the generic
> > > > > one, that does its own IMX DT parsing and calls just the SCMI protocol
> > > > > operations that it needs in the way that your platform expects: so basically
> > > > > another Pinctrl SCMI driver that does not use the generic pinconf DT
> > > > > configuration BUT DO USE the underlying SCMI Pinctrl protocol (via its
> > > > > exposed protocol operations...)
> > > >
> > > > I am ok with this approach, but I need use the other ID, saying 0x99, not 0x19,
> > > > because 0x19 will bind with the pinctrl-scmi.c driver, I could not reuse
> > > > this ID for i.MX pinctrl-scmi-imx driver. Otherwise there will be issue if both
> > > > driver are built in kernel image.
> > > >
> > >
> > > Ok here I lost you.
> > >
> > > The protocol ID 0x19 is bound to the protocol layer and identifies the
> > > standard Pinctrl protocol: usually you use a 0x99 to define and describe
> > > you own specific NEW vendor protocol, BUT here you are saying you are fine to
> > > use std Pinctrl spec AND the protocol operations as exposed in pinctrl.c, so
> > > I dont see why you should use a new vendor protocol_id to basically
> > > expose the same operations. (and I also dont see how you can do that
> > > without hacks in the current codebase)
> > >
> > > You CAN have multiple SCMI drivers using the same protocol at the same
> > > time (even more than one protocol at the same time), even though we try
> > > to avoid it if there are no good reason to have more than one driver, there
> > > is nothing in the spec or in the current SCMI platform or agent stacks that
> > > inhibits such scenario (and I use iot heavily for my offline testing
> > > indeed.)
> > >
> > > Look at:
> > >
> > > - drivers/hwmon/scmi-hwmon
> > > - drivers/iio/common/scmi_sensors/scmi_iio.c
> > >
> > > and you'll see that these 2 drivers uses the same SENSOR protocol, just for
> > > different sensor types so they do not interfere one with each other.
> >
> > Then, how are those two devices identified in a device tree?
>
> At SCMI probe time the SCMI core stack creates one device for each
> 'protocol_id/name' as provided by the registered SCMI drivers in
> their respective scmi_device_id/id_tableS, as long as the requested
> protocol is active in the DT (protocol@XX defined) and the 'name'
> is not duplicated; each of these devices, sharing the same SCMI
> protocol, are created sharing also the same DT node protocol@XX.
That's fine.
> Their respective SCMI drivers are subsequently separately matched on
> protocol_id/name and then probed as usual: it is up to the drivers not
> to step on each other feet by competing for the same SCMI resources...
> ...like issuing comflicting request for the same reosurce domain
> on the same protocol.
That's fine.
My question/idea is simple; how two pinctrl drivers, if possible, be
represented in a single device tree. To allow the case of Peng Fan,
for example, I suppose that we need some DT binding like:
scmi {
scmi_pinctrl: protocol@19 {
compatible = "arm, scmi-pinctrl-generic"; // not sure if needed
pinmux ... // standard binding
...
}
}
scmi_pinctrl_fsl {
compatible = "fsl,scmi-pinctrl-imx9";
fsl,pinmux = <&scmi_pinctrl, ...>,
<&scmi_pinctrl, ...>;
...
}
"scmi_pinctrl_fsl" driver may reuse generic SCMI pinctrl helper functions,
except dt_node_to_map, in pictrl_ops.
-Takahiro Akashi
> I suppose, though, a lot depends on how the respective drivers interact
> with their subsystems, like IIO SCMI does not really need any info
> from DT, and the hwmon uses just the phandle to pick the sensor_domain_id
> to associate, as an example, to thermal sensors.
>
> > That is the point in Peng's case and why he wants to have a dedicated
> > protocol id (I don't agree to this, though.)
> > If we follow Cristian's idea, we may want to have two dt nodes, say
> > pinctrl-scmi-generic and pinctrl-scmi-imx, as phandles for other device
> > nodes to refer to pins, respectively.
> > I think there is currently no mechanism (or binding?) to allow this
> > except adding a protocol id.
> >
>
> Beside the uglyness of having 2 different DT bindings schemas for 2
> different drivers coexisting in the same parent protocol node, it is
> still not clear to me why this cant be done technically without the need
> of a new dummy 0x99 protocol_node, given that each driver can use its
> own parsing logic in its probe, which is what Peng is doing in
> dt_node_to_map indeed ...even though, maybe, the result would be so
> ugly and/or not accepatble from bindings point of view that we will
> not want to do it at the end anyway....I mean maybe it is just the
> attempt itself to combine the Generic Pinctrl approach with the highly
> specific IMX approach that inevitably leads to these clashes...
>
> ...I maybe missing something, though, so I'll made some experiments on
> my side about this before I'll keep on blabbing :P
>
> Thanks,
> Cristian
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2023-09-13 9:53 UTC | newest]
Thread overview: 30+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-08-25 15:15 [RFC] scmi: pinctrl: support i.MX9 kernel test robot
2023-08-29 1:27 ` kernel test robot
-- strict thread matches above, loose matches on Subject: below --
2023-08-25 1:26 kernel test robot
2023-08-24 9:25 kernel test robot
2023-08-24 7:06 Peng Fan (OSS)
2023-08-24 7:06 ` Peng Fan (OSS)
2023-08-24 8:43 ` Linus Walleij
2023-08-24 8:43 ` Linus Walleij
2023-08-24 10:51 ` Sudeep Holla
2023-08-24 10:51 ` Sudeep Holla
2023-08-24 12:52 ` Peng Fan
2023-08-24 12:52 ` Peng Fan
2023-08-24 12:47 ` Peng Fan
2023-08-24 12:47 ` Peng Fan
2023-08-25 8:28 ` Linus Walleij
2023-08-25 8:28 ` Linus Walleij
2023-08-25 8:43 ` Peng Fan
2023-08-25 8:43 ` Peng Fan
2023-08-30 11:47 ` Cristian Marussi
2023-08-30 11:47 ` Cristian Marussi
2023-08-30 12:48 ` Peng Fan
2023-08-30 12:48 ` Peng Fan
2023-08-30 13:37 ` Cristian Marussi
2023-08-30 13:37 ` Cristian Marussi
2023-09-06 5:43 ` AKASHI Takahiro
2023-09-06 5:43 ` AKASHI Takahiro
2023-09-07 10:05 ` Cristian Marussi
2023-09-07 10:05 ` Cristian Marussi
2023-09-13 9:52 ` AKASHI Takahiro
2023-09-13 9:52 ` AKASHI Takahiro
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.