Re: [PATCH v2 1/6] net: mvneta: Add support for AlleyCat5

["Pali Rohár" <pali@kernel.org>]

On Tuesday 20 September 2022 20:31:48 Chris Packham wrote:
> diff --git a/drivers/net/mvneta.c b/drivers/net/mvneta.c
> index d2c42c4396..07919d6d35 100644
> --- a/drivers/net/mvneta.c
> +++ b/drivers/net/mvneta.c
> @@ -91,6 +91,8 @@ DECLARE_GLOBAL_DATA_PTR;
>  #define MVNETA_WIN_SIZE_MASK			(0xffff0000)
>  #define MVNETA_BASE_ADDR_ENABLE                 0x2290
>  #define      MVNETA_BASE_ADDR_ENABLE_BIT	0x1
> +#define      MVNETA_AC5_CNM_DDR_TARGET		0x2
> +#define      MVNETA_AC5_CNM_DDR_ATTR		0xb
>  #define MVNETA_PORT_ACCESS_PROTECT              0x2294
>  #define      MVNETA_PORT_ACCESS_PROTECT_WIN0_RW	0x3
>  #define MVNETA_PORT_CONFIG                      0x2400
> @@ -282,6 +284,8 @@ struct mvneta_port {
>  	struct gpio_desc phy_reset_gpio;
>  	struct gpio_desc sfp_tx_disable_gpio;
>  #endif
> +
> +	uintptr_t dma_base;     /* base address for DMA address decoding */
>  };
>  
>  /* The mvneta_tx_desc and mvneta_rx_desc structures describe the
> @@ -513,10 +517,19 @@ static void mvneta_rxq_desc_num_update(struct mvneta_port *pp,
>  static struct mvneta_rx_desc *
>  mvneta_rxq_next_desc_get(struct mvneta_rx_queue *rxq)
>  {
> +	struct mvneta_rx_desc *curr;
>  	int rx_desc = rxq->next_desc_to_proc;
>  
> +	/* validate RX descriptor */
> +	curr = rxq->descs + rx_desc;
> +	if (curr->data_size == 0) {
> +		/* do it to read real descriptor next time */
> +		DSB;
> +		return NULL;
> +	}
> +
>  	rxq->next_desc_to_proc = MVNETA_QUEUE_NEXT_DESC(rxq, rx_desc);
> -	return rxq->descs + rx_desc;
> +	return curr;
>  }

Hello! This change looks like some fix, and not related to AC5 support.
So it should be in separate patch as it affects all mvneta platforms,
not only AC5.

> @@ -1508,11 +1544,15 @@ static int mvneta_recv(struct udevice *dev, int flags, uchar **packetp)
>  		 */
>  		rx_desc = mvneta_rxq_next_desc_get(rxq);
>  
> +		if (!rx_desc)
> +			return 0;
> +
>  		rx_status = rx_desc->status;
>  		if (!mvneta_rxq_desc_is_first_last(rx_status) ||
>  		    (rx_status & MVNETA_RXD_ERR_SUMMARY)) {
>  			mvneta_rx_error(pp, rx_desc);
> -			/* leave the descriptor untouched */
> +			/* invalidate the descriptor */
> +			rx_desc->data_size = 0;
>  			return -EIO;
>  		}

ditto.

> @@ -1525,13 +1565,15 @@ static int mvneta_recv(struct udevice *dev, int flags, uchar **packetp)
>  		 * No cache invalidation needed here, since the rx_buffer's are
>  		 * located in a uncached memory region
>  		 */
> -		*packetp = data;
> +		*packetp = data + pp->dma_base;
>  
>  		/*
>  		 * Only mark one descriptor as free
>  		 * since only one was processed
>  		 */
>  		mvneta_rxq_desc_num_update(pp, rxq, 1, 1);
> +		/* invalidate the descriptor */
> +		rx_desc->data_size = 0;

ditto.

>  	}
>  
>  	return rx_bytes;
> @@ -1544,6 +1586,11 @@ static int mvneta_probe(struct udevice *dev)
>  	struct ofnode_phandle_args sfp_args;
>  #endif
>  	void *bd_space;
> +	void *blob = (void *)gd->fdt_blob;
> +	int node = dev_of_offset(dev);
> +	const int *cell;
> +	int len;
> +
>  
>  	/*
>  	 * Allocate buffer area for descs and rx_buffers. This is only
> @@ -1577,9 +1624,21 @@ static int mvneta_probe(struct udevice *dev)
>  	/* Configure MBUS address windows */
>  	if (device_is_compatible(dev, "marvell,armada-3700-neta"))
>  		mvneta_bypass_mbus_windows(pp);
> +	else if (device_is_compatible(dev, "marvell,armada-ac5-neta"))
> +		mvneta_conf_ac5_cnm_xbar_windows(pp);
>  	else
>  		mvneta_conf_mbus_windows(pp);
>  
> +	/* fetch dma ranges property */
> +	cell = fdt_getprop(blob, node, "dma-ranges", &len);
> +	if (cell && len >= 4 * sizeof(int)) {
> +		/* RAZA - TODO do that in loop by address-cells size */
> +		pp->dma_base = fdt32_to_cpu(cell[1]);
> +		pp->dma_base = (pp->dma_base << 32) | fdt32_to_cpu(cell[2]);

Probably it should be better to use some function for parsing dma-ranges
instead of open-coded implementation. It is not really clear why
cell[0] and cell[3] are ignored there, if it is missing intentionally or
it is a bug.

And... This should be used only for AC5, right?

> +	} else {
> +		pp->dma_base = 0;
> +	}
> +
>  #if CONFIG_IS_ENABLED(DM_GPIO)
>  	if (!dev_read_phandle_with_args(dev, "sfp", NULL, 0, 0, &sfp_args) &&
>  	    ofnode_is_enabled(sfp_args.node))
> @@ -1620,6 +1679,7 @@ static const struct eth_ops mvneta_ops = {
>  
>  static const struct udevice_id mvneta_ids[] = {
>  	{ .compatible = "marvell,armada-370-neta" },
> +	{ .compatible = "marvell,armada-ac5-neta" },
>  	{ .compatible = "marvell,armada-xp-neta" },
>  	{ .compatible = "marvell,armada-3700-neta" },
>  	{ }
> -- 
> 2.37.3
>