diff mbox series

[1/2] dt-bindings: i3c-hub: Add Renesas RG3MxxB12A1 I3C HUB

Message ID 20240217131412.4145506-1-steven.niu.uj@renesas.com
State Changes Requested, archived
Headers show
Series [1/2] dt-bindings: i3c-hub: Add Renesas RG3MxxB12A1 I3C HUB | expand

Checks

Context Check Description
robh/patch-applied success
robh/checkpatch success
robh/dt-meta-schema fail build log

Commit Message

Steven Niu Feb. 17, 2024, 1:14 p.m. UTC
Document the Renesas RG3MxxB12A1 I3C HUB.

Signed-off-by: Steven Niu <steven.niu.uj@renesas.com>
---
 .../devicetree/bindings/i3c/i3c-hub.yaml      | 400 ++++++++++++++++++
 1 file changed, 400 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i3c/i3c-hub.yaml

Comments

Krzysztof Kozlowski Feb. 17, 2024, 1:53 p.m. UTC | #1
On 17/02/2024 14:14, Steven Niu wrote:
> Document the Renesas RG3MxxB12A1 I3C HUB.
> 
> Signed-off-by: Steven Niu <steven.niu.uj@renesas.com>
> ---
>  .../devicetree/bindings/i3c/i3c-hub.yaml      | 400 ++++++++++++++++++
>  1 file changed, 400 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i3c/i3c-hub.yaml
> 
> diff --git a/Documentation/devicetree/bindings/i3c/i3c-hub.yaml b/Documentation/devicetree/bindings/i3c/i3c-hub.yaml
> new file mode 100644
> index 000000000000..8ff6ca172975
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i3c/i3c-hub.yaml

This does not match at all your device name. Filenames should use
compatibles.

This also sounds like some generic class of schema, but your short
commit msg does no explain here anything. I have no clue what you wanted
to achieve.


> @@ -0,0 +1,400 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/i3c/i3c-hub.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +

> +title: I3C HUB

No, you said it is Renesas device...


> +
> +maintainers:
> +  - Zbigniew Lukwinski <zbigniew.lukwinski@linux.intel.com>
> +  - Steven Niu <steven.niu.uj@renesas.com>
> +
> +description: |
> +  I3C HUB is smart device which provides multiple functionality:
> +  * enabling voltage compatibility across I3C Controller and Target devices,
> +  * bus capacitance isolation
> +  * address conflict isolation
> +  * I3C port expansion
> +  * two controllers in a single I3C bus
> +  * I3C and SMBus device compatibility
> +  * GPIO expansion

All this only confuses me - do we talk about common schema or specific
device?

> +
> +  Having such big number of features, there is a need to have some DT knobs to tell the I3C HUB

No, there is no. Sorry, don't use such arguments and drop all unrelated
driver things like descriptions from binding.

Also, wrap it according to coding style.


> +  driver which features shall be enabled and how they shall be configured. I3C HUB driver read,
> +  validate DT knobs and set corresponding registers with the right way to satisfy user requests from
> +  DT.
> +
> +  All the DT properties for I3C HUB are located under dedicated (for I3C HUB) DT entry. I3C HUB DT
> +  entry structure is aligned with regular I3C device DT entry described in i3c.yaml.

Drop, instead describe hardware.

> +
> +allOf:
> +  - $ref: i3c.yaml#
> +
> +properties:
> +  $nodename:
> +    pattern: "^hub@0,0$"

No, why?

Where is compatible?

> +
> +  cp0-ldo-en:

No clue what's this... no vendor prefix, no ref, why is is a property of
a board.

> +    enum:
> +      - disabled
> +      - enabled
> +    description: |
> +      I3C HUB Controller Port 0 LDO disabling/enabling setting. If enabled, voltage produced by
> +      on-die LDO will be available externally on dedicated pin. This option could be used to supply
> +      external pull-up resistors or for any other purpose which does not cross LDO capabilities.

Wrap properly, as asked by Coding Style,

> +
> +      This property is optional. If not provided, LDO will be disabled.

Drop redundant sentence "This property is optional".

> +
> +  cp1-ldo-en:

Why is this a property of a board DTS?

> +    enum:
> +      - disabled
> +      - enabled
> +    description: |
> +      I3C HUB Controller Port 1 LDO disabling/enabling setting. If enabled, voltage produced by
> +      on-die LDO will be available externally on dedicated pin. This option could be used to supply
> +      external pull-up resistors or for any other purpose which does not cross LDO capabilities.
> +
> +      This property is optional. If not provided, LDO will be disabled.
> +
> +  tp0145-ldo-en:

Why is this a property of a board DTS?

> +    enum:
> +      - disabled
> +      - enabled
> +    description: |
> +      I3C HUB Target Ports 0/1/4/5 LDO disabling/enabling setting. If enabled, voltage produced by
> +      on-die LDO will be available externally on dedicated pin. This option could be used to supply
> +      external pull-up resistors or for any other purpose which does not cross LDO capabilities.
> +
> +      This property is optional. If not provided, LDO will be disabled.
> +
> +  tp2367-ldo-en:

None of these make sense to me, sorry.

> +    enum:
> +      - disabled
> +      - enabled
> +    description: |
> +      I3C HUB Target Ports 2/3/6/7 LDO disabling/enabling setting. If enabled, voltage produced by
> +      on-die LDO will be available externally on dedicated pin. This option could be used to supply
> +      external pull-up resistors or for any other purpose which does not cross LDO capabilities.
> +
> +      This property is optional. If not provided, LDO will be disabled.
> +
> +  cp0-ldo-volt:
> +    enum:
> +      - 1.0V
> +      - 1.1V
> +      - 1.2V
> +      - 1.8V
> +    description: |
> +      I3C HUB Controller Port 0 LDO setting to control the Controller Port 1 voltage level. This
> +      property is optional.
> +
> +      If not provided, LDO configuration is not modified in I3C HUB.
> +
> +  cp1-ldo-volt:
> +    enum:
> +      - 1.0V
> +      - 1.1V
> +      - 1.2V
> +      - 1.8V

OK, that's it. Sorry, stop reimplementing regulators as strings. Use
proper common property suffixes for properties.

> +    description: |
> +      I3C HUB Controller Port 1 LDO setting to control the Controller Port 1 voltage level. This
> +      property is optional.
> +
> +      If not provided, LDO configuration is not modified in I3C HUB.
> +
> +  tp0145-ldo-volt:
> +    enum:
> +      - disabled
> +      - 1.0V
> +      - 1.1V
> +      - 1.2V
> +      - 1.8V
> +    description: |
> +      I3C HUB Target Ports 0/1/4/5 LDO setting to control the Target Ports 0/1/4/5 voltage level.
> +
> +      If not provided, LDO configuration is not modified in I3C HUB.
> +
> +  tp2367-ldo-volt:
> +    enum:
> +      - disabled
> +      - 1.0V
> +      - 1.1V
> +      - 1.2V
> +      - 1.8V
> +    description: |
> +      I3C HUB Target Ports 2/3/6/7 LDO setting to control the Target Ports 2/3/6/7 voltage level.
> +
> +      If not provided, LDO configuration is not modified in I3C HUB.
> +
> +  tp0145-pullup:
> +    enum:
> +      - disabled
> +      - 250R
> +      - 500R
> +      - 1k
> +      - 2k
> +    description: |
> +      I3C HUB Target Ports 0/1/4/5 pull-up setting to control the Target Ports 0/1/4/5 pull-up
> +      resistance level.
> +
> +      This property is optional. If not provided, pull-up configuration is not modified in I3C HUB.
> +
> +  tp2367-pullup:
> +    enum:
> +      - disabled
> +      - 250R
> +      - 500R
> +      - 1k
> +      - 2k
> +    description: |
> +      I3C HUB Target Ports 2/3/6/7 pull-up setting to control the Target Ports 2/3/6/7 pull-up
> +      resistance level.
> +
> +      This property is optional. If not provided, pull-up configuration is not modified in I3C HUB.
> +
> +  cp0-io-strength:
> +    enum:
> +      - 20Ohms
> +      - 30Ohms
> +      - 40Ohms
> +      - 50Ohms
> +    description: |
> +      I3C HUB Controller Port 0 IO strength setting to control the Controller Port 0 output driver
> +      strength.
> +
> +      This property is optional. If not provided, IO strength configuration is not modified in I3C
> +      HUB.
> +
> +  cp1-io-strength:
> +    enum:
> +      - 20Ohms
> +      - 30Ohms
> +      - 40Ohms
> +      - 50Ohms
> +    description: |
> +      I3C HUB Controller Port 1 IO strength setting to control the Controller Port 1 output driver
> +      strength.
> +
> +      This property is optional. If not provided, IO strength configuration is not modified in I3C
> +      HUB.
> +
> +  tp0145-io-strength:
> +    enum:
> +      - 20Ohms
> +      - 30Ohms
> +      - 40Ohms
> +      - 50Ohms
> +    description: |
> +      I3C HUB Target Ports 0/1/4/5 IO strength setting to control the Target Ports 0/1/4/5 output
> +      driver strength.
> +
> +      This property is optional. If not provided, IO strength configuration is not modified in I3C
> +      HUB.
> +
> +  tp2367-io-strength:
> +    enum:
> +      - 20Ohms
> +      - 30Ohms
> +      - 40Ohms
> +      - 50Ohms
> +    description: |
> +      I3C HUB Target Ports 2/3/6/7 IO strength setting to control the Target Ports 2/3/6/7 output
> +      driver strength.
> +
> +      This property is optional. If not provided, IO strength configuration is not modified in I3C
> +      HUB.
> +
> +  id:
> +    enum:
> +      - 0
> +      - 1
> +      - 3

NAK, no ID properties.

You try to upstream some really non-upstream style of binding. One
looking like produced by vendor. Don't.


> +    description: |
> +      I3C HUB ID based on CSEL pin. There are three possible values:
> +      0 - CP0 is selected as primary Controller Port
> +      1 - Primary Controller Port is selected by software by writing the REG#56
> +      3 - CP1 is selected as primary Controller Port
> +
> +      I3C HUB driver reads CSEL pin status (REG#121[5:4]) and tries to find DT node with matching
> +      value in 'id' property.
> +
> +      This property is optional. If not provided, DT node can only be used by the I3C HUB driver if
> +      there is no others with matching 'id' or 'id-cp1'. If there is a multiple DT nodes with no
> +      'id' property - the first one will be chosen by I3C HUB driver. If there is a multiple DT
> +      nodes with matching 'id' property - the first one will be chosen by I3C HUB driver.
> +
> +      If both 'id' and 'id-cp1' are available, DT node will chosen only when both values match those
> +      read from I3C HUB.

All these redundant texts are no helping... Encode proper reliationships
in schema.


> +
> +  id-cp1:
> +    enum:
> +      - 0
> +      - 1
> +      - 2
> +      - 3
> +    description: |
> +      I3C HUB ID based on CP1 SDA and SCL pins state probed during power on.
> +
> +      I3C HUB driver reads CP1 SDA and SCL pin status and tries to find DT node with matching value
> +      in 'id-cp1' property.
> +
> +      This property is optional. If not provided, DT node can only be used by the I3C HUB
> +      driver if there is no others with matching 'id' or 'id-cp1'. If there is a multiple DT nodes
> +      with no 'id-cp1' property - the first one will be chosen by I3C HUB driver. If there is a
> +      multiple DT nodes with matching 'id-cp1' property - the first one will be chosen by I3C HUB
> +      driver.
> +
> +      If both 'id' and 'id-cp1' are available, DT node will chosen only when both values match those
> +      read from I3C HUB.
> +
> +patternProperties:
> +  "@[0-9]$":
> +    type: object
> +    description: |

Do not need '|' unless you need to preserve formatting.

> +      I3C HUB Target Port child, should be named: target-port@<target-port-id>
> +
> +    properties:
> +      mode:
> +        enum:
> +          - disabled
> +          - i3c
> +          - smbus
> +          - gpio
> +        description: |
> +          I3C HUB Target Port mode setting to control Target Port functionality.
> +
> +          This property is optional. If not provided, Target Port mode configuration is not modified
> +          in I3C HUB.

Not modified? What does it even mean? Not modified from what?

Missing default:

Anyway, now you implement pinctrl here. Sorry, this needs proper
justification but still half of these properties are a clear NAK.

> +
> +      pullup:
> +        enum:
> +          - disabled
> +          - enabled
> +        description: |
> +          I3C HUB Target Port pull-up setting to disable/enable Target Port pull-up.
> +
> +          This property is optional. If not provided, Target Port pull-up configuration is not
> +          modified in I3C HUB.
> +
> +      always-enable:
> +        type: boolean
> +        description: |
> +          I3C HUB Target Port settings to control the port enable/disable policy.
> +
> +          This property is optional. If not provided, Target Port is enabled only on accessing to
> +          the devices connected to it and the port is disabled automatically after the accessing
> +          is done. If provided, the Target Port is always enabled.
> +
> +    patternProperties:
> +      "@0,0$":
> +        type: object
> +        description: |
> +          Backend for handling SMBus mode, should be named: backend@0,0
> +          Adding this node installs the backed for handling SMBus Target mode communication.
> +
> +        properties:
> +          compatible:
> +            description:
> +              Compatible of the I2C/SMBus backend.
> +
> +          target-reg:
> +            description:
> +              Target address used for Target Port in the I3C HUB configured as SMBus Target mode.
> +
> +additionalProperties: true

NAK, please start from basics from example schema.


Best regards,
Krzysztof
Krzysztof Kozlowski Feb. 17, 2024, 1:57 p.m. UTC | #2
On 17/02/2024 14:14, Steven Niu wrote:
> +
> +    patternProperties:
> +      "@0,0$":
> +        type: object
> +        description: |
> +          Backend for handling SMBus mode, should be named: backend@0,0
> +          Adding this node installs the backed for handling SMBus Target mode communication.
> +
> +        properties:
> +          compatible:
> +            description:
> +              Compatible of the I2C/SMBus backend.
> +
> +          target-reg:
> +            description:

Heh, you did not even bother with testing this...

Best regards,
Krzysztof
Krzysztof Kozlowski Feb. 17, 2024, 1:58 p.m. UTC | #3
On 17/02/2024 14:14, Steven Niu wrote:
>  endif # I3C
> diff --git a/drivers/i3c/Makefile b/drivers/i3c/Makefile
> index 11982efbc6d9..ca298faaee9a 100644
> --- a/drivers/i3c/Makefile
> +++ b/drivers/i3c/Makefile
> @@ -2,3 +2,4 @@
>  i3c-y				:= device.o master.o
>  obj-$(CONFIG_I3C)		+= i3c.o
>  obj-$(CONFIG_I3C)		+= master/
> +obj-$(CONFIG_I3C_HUB)	+= i3c-hub.o
> diff --git a/drivers/i3c/i3c-hub.c b/drivers/i3c/i3c-hub.c
> new file mode 100644
> index 000000000000..73a9b96e6635
> --- /dev/null
> +++ b/drivers/i3c/i3c-hub.c
> @@ -0,0 +1,1982 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (C) 2021 - 2023 Intel Corporation.*/


So this explains the code... You push to us some vendor stuff with
bindings not up to any upstream style. Please clean the bindings first
so they look like other bindings.


> +
> +static int read_backend_from_i3c_hub_dts(struct device_node *i3c_node_target,
> +					 struct i3c_hub *priv)
> +{
> +	struct device_node *i3c_node_tp;

Your coding style is terrible. device_node are called np or node.

> +	const char *compatible;
> +	int tp_port, ret;
> +	u32 addr_dts;
> +	struct smbus_backend *backend;
> +
> +	if (sscanf(i3c_node_target->full_name, "target-port@%d", &tp_port) == 0)
> +		return -EINVAL;
> +
> +	if (tp_port > I3C_HUB_TP_MAX_COUNT)
> +		return -ERANGE;
> +
> +	if (tp_port < 0)
> +		return -EINVAL;
> +
> +	INIT_LIST_HEAD(&priv->logical_bus[tp_port].smbus_port_adapter.backend_entry);
> +	for_each_available_child_of_node(i3c_node_target, i3c_node_tp) {
> +		if (strcmp(i3c_node_tp->name, "backend"))

No, don't compare names. What for?

> +			continue;
> +
> +		ret = of_property_read_u32(i3c_node_tp, "target-reg", &addr_dts);
> +		if (ret)
> +			return ret;
> +
> +		if (backend_node_is_exist(tp_port, priv, addr_dts))
> +			continue;
> +
> +		ret = of_property_read_string(i3c_node_tp, "compatible", &compatible);
> +		if (ret)
> +			return ret;
> +
> +		/* Currently only the slave-mqueue backend is supported */
> +		if (strcmp("slave-mqueue", compatible))

NAK, undocumented compatible.

> +			return -EINVAL;
> +
> +		backend = kzalloc(sizeof(*backend), GFP_KERNEL);
> +		if (!backend)
> +			return -ENOMEM;
> +
> +		backend->addr = addr_dts;
> +		backend->compatible = compatible;
> +		list_add(&backend->list,
> +			 &priv->logical_bus[tp_port].smbus_port_adapter.backend_entry);
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * This function saves information about the i3c_hub's ports
> + * working in slave mode. It takes its data from the DTs
> + * (aspeed-bmc-intel-avc.dts) and saves the parameters
> + * into the coresponding target port i2c_adapter_group structure
> + * in the i3c_hub
> + *
> + * @dev: device used by i3c_hub
> + * @i3c_node_hub: device node pointing to the hub
> + * @priv: pointer to the i3c_hub structure
> + */
> +static void i3c_hub_parse_dt_tp(struct device *dev,
> +				const struct device_node *i3c_node_hub,
> +				struct i3c_hub *priv)
> +{
> +	struct device_node *i3c_node_target;
> +	int ret;
> +
> +	for_each_available_child_of_node(i3c_node_hub, i3c_node_target) {
> +		if (!strcmp(i3c_node_target->name, "target-port")) {
> +			ret = read_backend_from_i3c_hub_dts(i3c_node_target, priv);
> +			if (ret)
> +				dev_err(dev, "DTS entry invalid - error %d", ret);
> +		}
> +	}
> +}
> +
> +static int i3c_hub_probe(struct i3c_device *i3cdev)
> +{
> +	struct regmap_config i3c_hub_regmap_config = {
> +		.reg_bits = 8,
> +		.val_bits = 8,
> +	};
> +	struct device *dev = &i3cdev->dev;
> +	struct device_node *node = NULL;
> +	struct regmap *regmap;
> +	struct i3c_hub *priv;
> +	char hub_id[32];
> +	int ret;
> +	int i;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->i3cdev = i3cdev;
> +	priv->driving_master = i3c_dev_get_master(i3cdev->desc);
> +	i3cdev_set_drvdata(i3cdev, priv);
> +	INIT_DELAYED_WORK(&priv->delayed_work, i3c_hub_delayed_work);
> +	sprintf(hub_id, "i3c-hub-%d-%llx", i3c_dev_get_master(i3cdev->desc)->bus.id,
> +		i3cdev->desc->info.pid);
> +	ret = i3c_hub_debugfs_init(priv, hub_id);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to initialized DebugFS.\n");

Drop, you cannot fail probe on debugfs.

> +
> +	i3c_hub_of_default_configuration(dev);
> +
> +	regmap = devm_regmap_init_i3c(i3cdev, &i3c_hub_regmap_config);
> +	if (IS_ERR(regmap)) {
> +		ret = PTR_ERR(regmap);
> +		dev_err(dev, "Failed to register I3C HUB regmap\n");
> +		goto error;
> +	}
> +	priv->regmap = regmap;
> +
> +	ret = i3c_hub_read_id(dev);
> +	if (ret)
> +		goto error;
> +
> +	priv->hub_dt_sel_id = -1;
> +	priv->hub_dt_cp1_id = -1;
> +	if (priv->hub_pin_cp1_id >= 0 && priv->hub_pin_sel_id >= 0)
> +		/* Find hub node in DT matching HW ID or just first without ID provided in DT */
> +		node = i3c_hub_get_dt_hub_node(dev->parent->of_node, priv);
> +
> +	if (!node) {
> +		dev_info(dev, "No DT entry - running with hardware defaults.\n");
> +	} else {
> +		of_node_get(node);
> +		i3c_hub_of_get_conf_static(dev, node);
> +		i3c_hub_of_get_conf_runtime(dev, node);
> +		of_node_put(node);
> +
> +		/* Parse DTS to find data on the SMBus target mode */
> +		i3c_hub_parse_dt_tp(dev, node, priv);
> +	}
> +
> +	/* Unlock access to protected registers */
> +	ret = regmap_write(priv->regmap, I3C_HUB_PROTECTION_CODE, REGISTERS_UNLOCK_CODE);
> +	if (ret) {
> +		dev_err(dev, "Failed to unlock HUB's protected registers\n");
> +		goto error;
> +	}
> +
> +	/* Register logic for native smbus ports */
> +	for (i = 0; i < I3C_HUB_TP_MAX_COUNT; i++) {
> +		priv->logical_bus[i].smbus_port_adapter.used = 0;
> +		if (priv->settings.tp[i].mode == I3C_HUB_DT_TP_MODE_SMBUS)
> +			ret = i3c_hub_smbus_tp_algo(priv, i);
> +	}
> +
> +	ret = i3c_hub_configure_hw(dev);
> +	if (ret) {
> +		dev_err(dev, "Failed to configure the HUB\n");
> +		goto error;
> +	}
> +
> +	/* Lock access to protected registers */
> +	ret = regmap_write(priv->regmap, I3C_HUB_PROTECTION_CODE, REGISTERS_LOCK_CODE);
> +	if (ret) {
> +		dev_err(dev, "Failed to lock HUB's protected registers\n");
> +		goto error;
> +	}
> +
> +	/* TBD: Apply special/security lock here using DEV_CMD register */
> +
> +	schedule_delayed_work(&priv->delayed_work, msecs_to_jiffies(100));
> +
> +	return 0;
> +
> +error:
> +	debugfs_remove_recursive(priv->debug_dir);
> +	return ret;
> +}
> +
> +static void i3c_hub_remove(struct i3c_device *i3cdev)
> +{
> +	struct i3c_hub *priv = i3cdev_get_drvdata(i3cdev);
> +	struct i2c_adapter_group *g_adap;
> +	struct smbus_backend *backend = NULL;
> +	int i;
> +
> +	for (i = 0; i < I3C_HUB_TP_MAX_COUNT; i++) {
> +		if (priv->logical_bus[i].smbus_port_adapter.used) {
> +			g_adap = &priv->logical_bus[i].smbus_port_adapter;
> +			cancel_delayed_work_sync(&g_adap->delayed_work_polling);
> +			list_for_each_entry(backend,  &g_adap->backend_entry, list) {
> +				i2c_unregister_device(backend->client);
> +				kfree(backend);
> +			}
> +		}
> +
> +		if (priv->logical_bus[i].smbus_port_adapter.used ||
> +		    priv->logical_bus[i].registered)
> +			i3c_master_unregister(&priv->logical_bus[i].controller);
> +	}
> +
> +	cancel_delayed_work_sync(&priv->delayed_work);
> +	debugfs_remove_recursive(priv->debug_dir);
> +}
> +
> +static struct i3c_driver i3c_hub = {
> +	.driver.name = "i3c-hub",
> +	.id_table = i3c_hub_ids,
> +	.probe = i3c_hub_probe,
> +	.remove = i3c_hub_remove,
> +};
> +
> +module_i3c_driver(i3c_hub);
> +
> +MODULE_AUTHOR("Zbigniew Lukwinski <zbigniew.lukwinski@linux.intel.com>");
> +MODULE_AUTHOR("Steven Niu <steven.niu.uj@renesas.com>");
> +MODULE_DESCRIPTION("I3C HUB driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 3afa530c5e32..b7cf15ba4e67 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -2244,15 +2244,26 @@ static int of_populate_i3c_bus(struct i3c_master_controller *master)
>  	struct device_node *node;
>  	int ret;
>  	u32 val;
> +	bool ignore_hub_node = false;
> +	char *addr_pid;
>  
>  	if (!i3cbus_np)
>  		return 0;
>  
>  	for_each_available_child_of_node(i3cbus_np, node) {
> -		ret = of_i3c_master_add_dev(master, node);
> -		if (ret) {
> -			of_node_put(node);
> -			return ret;
> +		ignore_hub_node = false;
> +		if (node->full_name && strstr(node->full_name, "hub")) {

NAK, you cannot rely on node name. Node name can be whatever, not "hub".

Best regards,
Krzysztof
Krzysztof Kozlowski Feb. 17, 2024, 2:07 p.m. UTC | #4
On 17/02/2024 14:14, Steven Niu wrote:
> Document the Renesas RG3MxxB12A1 I3C HUB.
> 
> Signed-off-by: Steven Niu <steven.niu.uj@renesas.com>
> ---
>  .../devicetree/bindings/i3c/i3c-hub.yaml      | 400 ++++++++++++++++++
>  1 file changed, 400 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i3c/i3c-hub.yaml
> 
> diff --git a/Documentation/devicetree/bindings/i3c/i3c-hub.yaml b/Documentation/devicetree/bindings/i3c/i3c-hub.yaml
> new file mode 100644
> index 000000000000..8ff6ca172975
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i3c/i3c-hub.yaml
> @@ -0,0 +1,400 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/i3c/i3c-hub.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: I3C HUB
> +
> +maintainers:
> +  - Zbigniew Lukwinski <zbigniew.lukwinski@linux.intel.com>
> +  - Steven Niu <steven.niu.uj@renesas.com>
> +
> +description: |
> +  I3C HUB is smart device which provides multiple functionality:
> +  * enabling voltage compatibility across I3C Controller and Target devices,
> +  * bus capacitance isolation
> +  * address conflict isolation
> +  * I3C port expansion
> +  * two controllers in a single I3C bus
> +  * I3C and SMBus device compatibility
> +  * GPIO expansion
> +
> +  Having such big number of features, there is a need to have some DT knobs to tell the I3C HUB
> +  driver which features shall be enabled and how they shall be configured. I3C HUB driver read,
> +  validate DT knobs and set corresponding registers with the right way to satisfy user requests from
> +  DT.

Ah, now I understand. Instead of doing proper schema for specific
device, you want binding for generic driver.

Sorry, that's a clear NAK and clear reason why Intel did not upstream this.

Bindings are not for drivers. Bindings are for hardware. Describe
specific device and that's it. Any non-device, but driver-related,
property is NAKed.

Best regards,
Krzysztof
Rob Herring (Arm) Feb. 17, 2024, 2:55 p.m. UTC | #5
On Sat, 17 Feb 2024 21:14:11 +0800, Steven Niu wrote:
> Document the Renesas RG3MxxB12A1 I3C HUB.
> 
> Signed-off-by: Steven Niu <steven.niu.uj@renesas.com>
> ---
>  .../devicetree/bindings/i3c/i3c-hub.yaml      | 400 ++++++++++++++++++
>  1 file changed, 400 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i3c/i3c-hub.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/i3c/i3c-hub.yaml: id: missing type definition
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/i3c/i3c-hub.yaml: id-cp1: missing type definition
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/i3c/i3c-hub.yaml: target-reg: missing type definition
Documentation/devicetree/bindings/i3c/i3c-hub.example.dts:18.28-60.11: Warning (unit_address_vs_reg): /example-0/i3c-master@d040000: node has a unit name, but no reg or ranges property
Documentation/devicetree/bindings/i3c/i3c-hub.example.dts:22.19-59.13: Warning (unit_address_vs_reg): /example-0/i3c-master@d040000/hub@0,0: node has a unit name, but no reg or ranges property
Documentation/devicetree/bindings/i3c/i3c-hub.example.dts:38.27-42.15: Warning (unit_address_vs_reg): /example-0/i3c-master@d040000/hub@0,0/target-port@0: node has a unit name, but no reg or ranges property
Documentation/devicetree/bindings/i3c/i3c-hub.example.dts:43.27-50.15: Warning (unit_address_vs_reg): /example-0/i3c-master@d040000/hub@0,0/target-port@1: node has a unit name, but no reg or ranges property
Documentation/devicetree/bindings/i3c/i3c-hub.example.dts:46.26-49.17: Warning (unit_address_vs_reg): /example-0/i3c-master@d040000/hub@0,0/target-port@1/backend@0,0: node has a unit name, but no reg or ranges property
Documentation/devicetree/bindings/i3c/i3c-hub.example.dts:51.27-54.15: Warning (unit_address_vs_reg): /example-0/i3c-master@d040000/hub@0,0/target-port@2: node has a unit name, but no reg or ranges property
Documentation/devicetree/bindings/i3c/i3c-hub.example.dts:55.27-58.15: Warning (unit_address_vs_reg): /example-0/i3c-master@d040000/hub@0,0/target-port@3: node has a unit name, but no reg or ranges property
Documentation/devicetree/bindings/i3c/i3c-hub.example.dts:78.28-124.11: Warning (unit_address_vs_reg): /example-1/i3c-master@d040000: node has a unit name, but no reg or ranges property
Documentation/devicetree/bindings/i3c/i3c-hub.example.dts:102.27-106.15: Warning (unit_address_vs_reg): /example-1/i3c-master@d040000/hub@70,3C000000100/target-port@0: node has a unit name, but no reg or ranges property
Documentation/devicetree/bindings/i3c/i3c-hub.example.dts:107.27-114.15: Warning (unit_address_vs_reg): /example-1/i3c-master@d040000/hub@70,3C000000100/target-port@1: node has a unit name, but no reg or ranges property
Documentation/devicetree/bindings/i3c/i3c-hub.example.dts:110.26-113.17: Warning (unit_address_vs_reg): /example-1/i3c-master@d040000/hub@70,3C000000100/target-port@1/backend@0,0: node has a unit name, but no reg or ranges property
Documentation/devicetree/bindings/i3c/i3c-hub.example.dts:115.27-118.15: Warning (unit_address_vs_reg): /example-1/i3c-master@d040000/hub@70,3C000000100/target-port@2: node has a unit name, but no reg or ranges property
Documentation/devicetree/bindings/i3c/i3c-hub.example.dts:119.27-122.15: Warning (unit_address_vs_reg): /example-1/i3c-master@d040000/hub@70,3C000000100/target-port@3: node has a unit name, but no reg or ranges property
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/i3c/i3c-hub.example.dtb: i3c-master@d040000: #address-cells:0:0: 3 was expected
	from schema $id: http://devicetree.org/schemas/i3c/i3c.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/i3c/i3c-hub.example.dtb: i3c-master@d040000: hub@0,0: 'reg' is a required property
	from schema $id: http://devicetree.org/schemas/i3c/i3c.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/i3c/i3c-hub.example.dtb: hub@0,0: $nodename:0: 'hub@0,0' does not match '^i3c-master@[0-9a-f]+$'
	from schema $id: http://devicetree.org/schemas/i3c/i3c-hub.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/i3c/i3c-hub.example.dtb: hub@0,0: '#address-cells' is a required property
	from schema $id: http://devicetree.org/schemas/i3c/i3c-hub.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/i3c/i3c-hub.example.dtb: hub@0,0: '#size-cells' is a required property
	from schema $id: http://devicetree.org/schemas/i3c/i3c-hub.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/i3c/i3c-hub.example.dtb: hub@0,0: target-port@0: 'compatible' is a required property
	from schema $id: http://devicetree.org/schemas/i3c/i3c-hub.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/i3c/i3c-hub.example.dtb: hub@0,0: target-port@0: 'reg' is a required property
	from schema $id: http://devicetree.org/schemas/i3c/i3c-hub.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/i3c/i3c-hub.example.dtb: hub@0,0: target-port@1: 'compatible' is a required property
	from schema $id: http://devicetree.org/schemas/i3c/i3c-hub.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/i3c/i3c-hub.example.dtb: hub@0,0: target-port@1: 'reg' is a required property
	from schema $id: http://devicetree.org/schemas/i3c/i3c-hub.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/i3c/i3c-hub.example.dtb: hub@0,0: target-port@2: 'compatible' is a required property
	from schema $id: http://devicetree.org/schemas/i3c/i3c-hub.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/i3c/i3c-hub.example.dtb: hub@0,0: target-port@2: 'reg' is a required property
	from schema $id: http://devicetree.org/schemas/i3c/i3c-hub.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/i3c/i3c-hub.example.dtb: hub@0,0: target-port@3: 'compatible' is a required property
	from schema $id: http://devicetree.org/schemas/i3c/i3c-hub.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/i3c/i3c-hub.example.dtb: hub@0,0: target-port@3: 'reg' is a required property
	from schema $id: http://devicetree.org/schemas/i3c/i3c-hub.yaml#
Documentation/devicetree/bindings/i3c/i3c-hub.example.dtb: /example-0/i3c-master@d040000/hub@0,0/target-port@1/backend@0,0: failed to match any schema with compatible: ['slave-mqueue']
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/i3c/i3c-hub.example.dtb: i3c-master@d040000: #address-cells:0:0: 3 was expected
	from schema $id: http://devicetree.org/schemas/i3c/i3c.yaml#
Documentation/devicetree/bindings/i3c/i3c-hub.example.dtb: /example-1/i3c-master@d040000/hub@70,3C000000100/target-port@1/backend@0,0: failed to match any schema with compatible: ['slave-mqueue']

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240217131412.4145506-1-steven.niu.uj@renesas.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
Jeremy Kerr June 19, 2024, 6:55 a.m. UTC | #6
Hi Steven,

> RG3MxxB12A1 I3C HUB is smart device which provide multiple
> functionality:
> * Enabling voltage compatibility across I3C Controller and Target
> Device.
> * Enabling voltage compatibility across I3C Controller and Target
> devices.
> * Bus capacitance isolation.
> * Address conflict isolation.
> * I3C port expansion.
> * Two controllers in a single I3C bus.
> * I3C and SMBus device compatibility.
> * GPIO expansion.

Do you plan any more work on this driver? It would be great to have
this included upstream, but looks like there's a fairly large amount of
work required to get to that point, particularly in regards to using
existing kernel infrastructure to support the device functions.

Anything I can do to help there? Would a (further) review on this
series be useful?

Cheers,


Jeremy
Steven Niu July 24, 2024, 11:47 a.m. UTC | #7
Hi, Jeremy,

Apologize for the late reply. I am out of work for a long time because of my personal emergency. I am back at work now.

Thank you for your interest in the development of this driver.  

We are working on the following items after the first submit of the patch:
1. Modify the binding file to make it focused on the device description instead of driver description.
2. Fix the codes which doesn't following the kernel development rules.
3. Re-implement the SMBus Agent function with IBI mechanism instead of polling. 
   SMBus Agent function requires the driver to check the status register of the I3C hub. Polling the status register over I3C bus exhausts too much bus resources. I3C Hub supports reporting status changes with IBI. This shall be supported in the driver.
4. Remove the i2c slave-mqueue module. 
   The slave-mqueue module is used to provide a user interface for SMBus Agent slave function. But it has not been included in upstream.

The first two items have been finished. And we are working on the #3 and #4. 

I would appreciate your input on this driver. I think a review with you is very helpful. 

Regards,
Steven



-----Original Message-----
From: Jeremy Kerr <jk@codeconstruct.com.au> 
Sent: Wednesday, June 19, 2024 2:56 PM
To: Steven Niu <steven.niu.uj@renesas.com>; alexandre.belloni@bootlin.com; linux-i3c@lists.infradead.org; linux-kernel@vger.kernel.org; robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org; devicetree@vger.kernel.org
Cc: zbigniew.lukwinski@linux.intel.com
Subject: Re: [PATCH 2/2] i3c: i3c-hub: Add Renesas RG3MxxB12A1 I3C HUB driver

Hi Steven,

> RG3MxxB12A1 I3C HUB is smart device which provide multiple
> functionality:
> * Enabling voltage compatibility across I3C Controller and Target 
> Device.
> * Enabling voltage compatibility across I3C Controller and Target 
> devices.
> * Bus capacitance isolation.
> * Address conflict isolation.
> * I3C port expansion.
> * Two controllers in a single I3C bus.
> * I3C and SMBus device compatibility.
> * GPIO expansion.

Do you plan any more work on this driver? It would be great to have this included upstream, but looks like there's a fairly large amount of work required to get to that point, particularly in regards to using existing kernel infrastructure to support the device functions.

Anything I can do to help there? Would a (further) review on this series be useful?

Cheers,


Jeremy
Jeremy Kerr July 25, 2024, 1:52 a.m. UTC | #8
Hi Steven,

> Apologize for the late reply. I am out of work for a long time
> because of my personal emergency. I am back at work now.

No problem at all; Sorry to hear it, I hope things are okay now. Thanks
for getting back to me though!

> We are working on the following items after the first submit of the
> patch:
> 1. Modify the binding file to make it focused on the device
> description instead of driver description.
> 2. Fix the codes which doesn't following the kernel development
> rules.
> 3. Re-implement the SMBus Agent function with IBI mechanism instead
> of polling. 
>    SMBus Agent function requires the driver to check the status
> register of the I3C hub. Polling the status register over I3C bus
> exhausts too much bus resources. I3C Hub supports reporting status
> changes with IBI. This shall be supported in the driver.
> 4. Remove the i2c slave-mqueue module. 
>    The slave-mqueue module is used to provide a user interface for
> SMBus Agent slave function. But it has not been included in upstream.
> 
> The first two items have been finished. And we are working on the #3
> and #4. 

OK, sounds good. I have a few structural suggestions; some of these have
been covered by others' reviews too, but to summarise:

Specificity: it looks like you're proposing this as a generic i3c hub
driver, but I don't think that's the case; as far as I can tell, it's a
Renesas-specific design (and IO interface), so you want to reflect that
in the naming & config options.

which leads to:

Binding: the driver cannot match on the I3C hub class BCR, as that
doesn't specify any sort of register/behavioural interface. If another
hardware hub was implemented, using the same class BCR, this driver
would conflict. You'll need to match on vendor/device IDs instead.

Existing infrastructure: you're re-implementing a few common bits of
kernel infrastructure with this change; for example the LDO settings can
use the regulator devices instead. Is this included in your (2) point
above?

Configuration: as you've noted, the run-time configuration of the device
doesn't belong in the device tree binding in most cases. In fixing
those, there are a couple of approaches:

 * when moving to existing infrastructure (for example, the regulator 
   and/or pinctrl devices), the consumers of those devices can request
   the correct configuration anyway, so you don't need to worry about
   the specific configuration chosen

 * for other runtime things, such as the downstream port configuration
   (in i3c mode vs. SMBus Agent mode) may be better exposed via configfs

 * for things that do belong as new properties in the device tree, you
   want numeric values (with an appropriately named property) rather
   than strings.

 * there may be other configuration settings that don't sit well in
   above categories, best to discuss those here.

SMBus Agent i2c interface: the controller side looks fine, but the
device side should be implemented as i2c_slave_event() events on the
same controllers rather than the i2c-mqueue device (which isn't
upstream, and doesn't look likely to be).

Happy to provide specific feedback as you go, but it's the maintainers
(Alexandre and Krzysztof) that will have the final say in design and
style decisions.

Cheers,


Jeremy
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/i3c/i3c-hub.yaml b/Documentation/devicetree/bindings/i3c/i3c-hub.yaml
new file mode 100644
index 000000000000..8ff6ca172975
--- /dev/null
+++ b/Documentation/devicetree/bindings/i3c/i3c-hub.yaml
@@ -0,0 +1,400 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/i3c/i3c-hub.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: I3C HUB
+
+maintainers:
+  - Zbigniew Lukwinski <zbigniew.lukwinski@linux.intel.com>
+  - Steven Niu <steven.niu.uj@renesas.com>
+
+description: |
+  I3C HUB is smart device which provides multiple functionality:
+  * enabling voltage compatibility across I3C Controller and Target devices,
+  * bus capacitance isolation
+  * address conflict isolation
+  * I3C port expansion
+  * two controllers in a single I3C bus
+  * I3C and SMBus device compatibility
+  * GPIO expansion
+
+  Having such big number of features, there is a need to have some DT knobs to tell the I3C HUB
+  driver which features shall be enabled and how they shall be configured. I3C HUB driver read,
+  validate DT knobs and set corresponding registers with the right way to satisfy user requests from
+  DT.
+
+  All the DT properties for I3C HUB are located under dedicated (for I3C HUB) DT entry. I3C HUB DT
+  entry structure is aligned with regular I3C device DT entry described in i3c.yaml.
+
+allOf:
+  - $ref: i3c.yaml#
+
+properties:
+  $nodename:
+    pattern: "^hub@0,0$"
+
+  cp0-ldo-en:
+    enum:
+      - disabled
+      - enabled
+    description: |
+      I3C HUB Controller Port 0 LDO disabling/enabling setting. If enabled, voltage produced by
+      on-die LDO will be available externally on dedicated pin. This option could be used to supply
+      external pull-up resistors or for any other purpose which does not cross LDO capabilities.
+
+      This property is optional. If not provided, LDO will be disabled.
+
+  cp1-ldo-en:
+    enum:
+      - disabled
+      - enabled
+    description: |
+      I3C HUB Controller Port 1 LDO disabling/enabling setting. If enabled, voltage produced by
+      on-die LDO will be available externally on dedicated pin. This option could be used to supply
+      external pull-up resistors or for any other purpose which does not cross LDO capabilities.
+
+      This property is optional. If not provided, LDO will be disabled.
+
+  tp0145-ldo-en:
+    enum:
+      - disabled
+      - enabled
+    description: |
+      I3C HUB Target Ports 0/1/4/5 LDO disabling/enabling setting. If enabled, voltage produced by
+      on-die LDO will be available externally on dedicated pin. This option could be used to supply
+      external pull-up resistors or for any other purpose which does not cross LDO capabilities.
+
+      This property is optional. If not provided, LDO will be disabled.
+
+  tp2367-ldo-en:
+    enum:
+      - disabled
+      - enabled
+    description: |
+      I3C HUB Target Ports 2/3/6/7 LDO disabling/enabling setting. If enabled, voltage produced by
+      on-die LDO will be available externally on dedicated pin. This option could be used to supply
+      external pull-up resistors or for any other purpose which does not cross LDO capabilities.
+
+      This property is optional. If not provided, LDO will be disabled.
+
+  cp0-ldo-volt:
+    enum:
+      - 1.0V
+      - 1.1V
+      - 1.2V
+      - 1.8V
+    description: |
+      I3C HUB Controller Port 0 LDO setting to control the Controller Port 1 voltage level. This
+      property is optional.
+
+      If not provided, LDO configuration is not modified in I3C HUB.
+
+  cp1-ldo-volt:
+    enum:
+      - 1.0V
+      - 1.1V
+      - 1.2V
+      - 1.8V
+    description: |
+      I3C HUB Controller Port 1 LDO setting to control the Controller Port 1 voltage level. This
+      property is optional.
+
+      If not provided, LDO configuration is not modified in I3C HUB.
+
+  tp0145-ldo-volt:
+    enum:
+      - disabled
+      - 1.0V
+      - 1.1V
+      - 1.2V
+      - 1.8V
+    description: |
+      I3C HUB Target Ports 0/1/4/5 LDO setting to control the Target Ports 0/1/4/5 voltage level.
+
+      If not provided, LDO configuration is not modified in I3C HUB.
+
+  tp2367-ldo-volt:
+    enum:
+      - disabled
+      - 1.0V
+      - 1.1V
+      - 1.2V
+      - 1.8V
+    description: |
+      I3C HUB Target Ports 2/3/6/7 LDO setting to control the Target Ports 2/3/6/7 voltage level.
+
+      If not provided, LDO configuration is not modified in I3C HUB.
+
+  tp0145-pullup:
+    enum:
+      - disabled
+      - 250R
+      - 500R
+      - 1k
+      - 2k
+    description: |
+      I3C HUB Target Ports 0/1/4/5 pull-up setting to control the Target Ports 0/1/4/5 pull-up
+      resistance level.
+
+      This property is optional. If not provided, pull-up configuration is not modified in I3C HUB.
+
+  tp2367-pullup:
+    enum:
+      - disabled
+      - 250R
+      - 500R
+      - 1k
+      - 2k
+    description: |
+      I3C HUB Target Ports 2/3/6/7 pull-up setting to control the Target Ports 2/3/6/7 pull-up
+      resistance level.
+
+      This property is optional. If not provided, pull-up configuration is not modified in I3C HUB.
+
+  cp0-io-strength:
+    enum:
+      - 20Ohms
+      - 30Ohms
+      - 40Ohms
+      - 50Ohms
+    description: |
+      I3C HUB Controller Port 0 IO strength setting to control the Controller Port 0 output driver
+      strength.
+
+      This property is optional. If not provided, IO strength configuration is not modified in I3C
+      HUB.
+
+  cp1-io-strength:
+    enum:
+      - 20Ohms
+      - 30Ohms
+      - 40Ohms
+      - 50Ohms
+    description: |
+      I3C HUB Controller Port 1 IO strength setting to control the Controller Port 1 output driver
+      strength.
+
+      This property is optional. If not provided, IO strength configuration is not modified in I3C
+      HUB.
+
+  tp0145-io-strength:
+    enum:
+      - 20Ohms
+      - 30Ohms
+      - 40Ohms
+      - 50Ohms
+    description: |
+      I3C HUB Target Ports 0/1/4/5 IO strength setting to control the Target Ports 0/1/4/5 output
+      driver strength.
+
+      This property is optional. If not provided, IO strength configuration is not modified in I3C
+      HUB.
+
+  tp2367-io-strength:
+    enum:
+      - 20Ohms
+      - 30Ohms
+      - 40Ohms
+      - 50Ohms
+    description: |
+      I3C HUB Target Ports 2/3/6/7 IO strength setting to control the Target Ports 2/3/6/7 output
+      driver strength.
+
+      This property is optional. If not provided, IO strength configuration is not modified in I3C
+      HUB.
+
+  id:
+    enum:
+      - 0
+      - 1
+      - 3
+    description: |
+      I3C HUB ID based on CSEL pin. There are three possible values:
+      0 - CP0 is selected as primary Controller Port
+      1 - Primary Controller Port is selected by software by writing the REG#56
+      3 - CP1 is selected as primary Controller Port
+
+      I3C HUB driver reads CSEL pin status (REG#121[5:4]) and tries to find DT node with matching
+      value in 'id' property.
+
+      This property is optional. If not provided, DT node can only be used by the I3C HUB driver if
+      there is no others with matching 'id' or 'id-cp1'. If there is a multiple DT nodes with no
+      'id' property - the first one will be chosen by I3C HUB driver. If there is a multiple DT
+      nodes with matching 'id' property - the first one will be chosen by I3C HUB driver.
+
+      If both 'id' and 'id-cp1' are available, DT node will chosen only when both values match those
+      read from I3C HUB.
+
+  id-cp1:
+    enum:
+      - 0
+      - 1
+      - 2
+      - 3
+    description: |
+      I3C HUB ID based on CP1 SDA and SCL pins state probed during power on.
+
+      I3C HUB driver reads CP1 SDA and SCL pin status and tries to find DT node with matching value
+      in 'id-cp1' property.
+
+      This property is optional. If not provided, DT node can only be used by the I3C HUB
+      driver if there is no others with matching 'id' or 'id-cp1'. If there is a multiple DT nodes
+      with no 'id-cp1' property - the first one will be chosen by I3C HUB driver. If there is a
+      multiple DT nodes with matching 'id-cp1' property - the first one will be chosen by I3C HUB
+      driver.
+
+      If both 'id' and 'id-cp1' are available, DT node will chosen only when both values match those
+      read from I3C HUB.
+
+patternProperties:
+  "@[0-9]$":
+    type: object
+    description: |
+      I3C HUB Target Port child, should be named: target-port@<target-port-id>
+
+    properties:
+      mode:
+        enum:
+          - disabled
+          - i3c
+          - smbus
+          - gpio
+        description: |
+          I3C HUB Target Port mode setting to control Target Port functionality.
+
+          This property is optional. If not provided, Target Port mode configuration is not modified
+          in I3C HUB.
+
+      pullup:
+        enum:
+          - disabled
+          - enabled
+        description: |
+          I3C HUB Target Port pull-up setting to disable/enable Target Port pull-up.
+
+          This property is optional. If not provided, Target Port pull-up configuration is not
+          modified in I3C HUB.
+
+      always-enable:
+        type: boolean
+        description: |
+          I3C HUB Target Port settings to control the port enable/disable policy.
+
+          This property is optional. If not provided, Target Port is enabled only on accessing to
+          the devices connected to it and the port is disabled automatically after the accessing
+          is done. If provided, the Target Port is always enabled.
+
+    patternProperties:
+      "@0,0$":
+        type: object
+        description: |
+          Backend for handling SMBus mode, should be named: backend@0,0
+          Adding this node installs the backed for handling SMBus Target mode communication.
+
+        properties:
+          compatible:
+            description:
+              Compatible of the I2C/SMBus backend.
+
+          target-reg:
+            description:
+              Target address used for Target Port in the I3C HUB configured as SMBus Target mode.
+
+additionalProperties: true
+
+examples:
+  - |
+    i3c-master@d040000 {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      hub@0,0 {
+        cp0-ldo-en = "disabled";
+        cp1-ldo-en = "enabled";
+        cp0-ldo-volt = "1.0V";
+        cp1-ldo-volt = "1.1V";
+        tp0145-ldo-en = "enabled";
+        tp2367-ldo-en = "disabled";
+        tp0145-ldo-volt = "1.2V";
+        tp2367-ldo-volt = "1.8V";
+        tp0145-pullup = "2k";
+        tp2367-pullup = "500R";
+        tp0145-io-strength = "50Ohms";
+        tp2367-io-strength = "30Ohms";
+        cp0-io-strength = "20Ohms";
+        cp1-io-strength = "40Ohms";
+
+        target-port@0 {
+          mode = "i3c";
+          pullup = "enabled";
+          always_enable;
+        };
+        target-port@1 {
+          mode = "smbus";
+          pullup = "enabled";
+          backend@0,0{
+              compatible = "slave-mqueue";
+              target-reg = <0x10>;
+          };
+        };
+        target-port@2 {
+          mode = "gpio";
+          pullup = "disabled";
+        };
+        target-port@3 {
+          mode = "disabled";
+          pullup = "disabled";
+        };
+      };
+    };
+
+  - |
+    i3c-master@d040000 {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      hub@70,3C000000100 {
+        reg = <0x70 0x3C0 0x00000100>;
+        assigned-address = <0x70>;
+        dcr = <0xC2>;
+
+        cp0-ldo-en = "disabled";
+        cp1-ldo-en = "enabled";
+        cp0-ldo-volt = "1.0V";
+        cp1-ldo-volt = "1.1V";
+        tp0145-ldo-en = "enabled";
+        tp2367-ldo-en = "disabled";
+        tp0145-ldo-volt = "1.2V";
+        tp2367-ldo-volt = "1.8V";
+        tp0145-pullup = "2k";
+        tp2367-pullup = "500R";
+        tp0145-io-strength = "50Ohms";
+        tp2367-io-strength = "30Ohms";
+        cp0-io-strength = "20Ohms";
+        cp1-io-strength = "40Ohms";
+
+        target-port@0 {
+          mode = "i3c";
+          pullup = "enabled";
+          always-enable;
+        };
+        target-port@1 {
+          mode = "smbus";
+          pullup = "enabled";
+          backend@0,0{
+              compatible = "slave-mqueue";
+              target-reg = <0x12>;
+          };
+        };
+        target-port@2 {
+          mode = "gpio";
+          pullup = "disabled";
+        };
+        target-port@3 {
+          mode = "disabled";
+          pullup = "disabled";
+        };
+      };
+    };