[PATCH v2 16/21] net: tsec: Support <reg> property from the subnode "queue-group"

[Vladimir Oltean]

On Fri, Mar 12, 2021 at 09:35:57PM +0800, Bin Meng wrote:
> At present the tsec driver uses a non-standard DT bindings to get
> its <reg> base / size. The upstream Linux kernel seems to require
> the <reg> base / size to be put under a subnode of the eTSEC node
> with a name prefix "queue-group". This is not documented in the
> kernel DT bindings, but it looks every dtsi file that contains the
> eTSEC node was written like this.
> 
> This commit updates the tsec driver to handle this case.
> 
> Signed-off-by: Bin Meng <bmeng.cn at gmail.com>
> Reviewed-by: Ramon Fried <rfried.dev at gmail.com>
> ---
> 
> (no changes since v1)
> 
>  drivers/net/tsec.c | 26 ++++++++++++++++++++++++--
>  1 file changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/tsec.c b/drivers/net/tsec.c
> index f801d020fb..49bed0c1dd 100644
> --- a/drivers/net/tsec.c
> +++ b/drivers/net/tsec.c
> @@ -827,13 +827,35 @@ int tsec_probe(struct udevice *dev)
>  	struct tsec_data *data;
>  	const char *phy_mode;
>  	fdt_addr_t reg;
> -	ofnode parent;
> +	ofnode parent, child;

Same comment about the "reverse Christmas tree" notation.

>  	int ret;
>  
>  	data = (struct tsec_data *)dev_get_driver_data(dev);
>  
>  	pdata->iobase = (phys_addr_t)dev_read_addr(dev);
> -	priv->regs = dev_remap_addr(dev);
> +	if (pdata->iobase != FDT_ADDR_T_NONE) {
> +		priv->regs = dev_remap_addr(dev);
> +	} else {
> +		ofnode_for_each_subnode(child, dev_ofnode(dev)) {
> +			if (!strncmp(ofnode_get_name(child), "queue-group",
> +				     strlen("queue-group"))) {

I would prefer the syntax:

			if (strncmp(ofnode_get_name(child), "queue-group",
				    strlen("queue-group")))
				continue;

which allows us to reduce the indentation level.

> +				reg = ofnode_get_addr(child);
> +				if (reg == FDT_ADDR_T_NONE) {
> +					printf("No 'reg' property of <queue-group>\n");
> +					return -ENOENT;
> +				}
> +				pdata->iobase = reg;
> +				priv->regs = map_physmem(pdata->iobase, 0,
> +							 MAP_NOCACHE);

Hmm, can't you just populate pdata->iobase, and call the same
dev_remap_addr in the common code path?

> +				break;

Could you please add a comment that if there are multiple queue groups,
we only use the first one?

> +			}
> +		}
> +
> +		if (!ofnode_valid(child)) {
> +			printf("No child node for <queue-group>?\n");
> +			return -ENOENT;
> +		}
> +	}