Re: [PATCH] net: designware: Support high memory nodes

[Andre Przywara <andre.przywara@arm.com>]

On Sat, 11 Nov 2023 16:26:14 +0100
Nils Le Roux <gilbsgilbert@gmail.com> wrote:

Hi Nils,

> Some platforms (such as the Lichee Pi 4A) have their dwmac device
> addressable only in high memory space. Storing the node's base address
> on 32 bits is not possible in such case.

That's indeed true, so thanks for the patch.
Some comments below.

> Use platform's physical address type to store the base address.
> 
> Signed-off-by: Nils Le Roux <gilbsgilbert@gmail.com>
> ---
> 
>  drivers/net/designware.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/designware.c b/drivers/net/designware.c
> index a174344b3e..327732fbf7 100644
> --- a/drivers/net/designware.c
> +++ b/drivers/net/designware.c
> @@ -678,8 +678,8 @@ int designware_eth_probe(struct udevice *dev)
>  {
>  	struct eth_pdata *pdata = dev_get_plat(dev);
>  	struct dw_eth_dev *priv = dev_get_priv(dev);
> -	u32 iobase = pdata->iobase;
> -	ulong ioaddr;
> +	phys_addr_t iobase = pdata->iobase;
> +	phys_addr_t ioaddr;

So I agree that iobase should be a phys_addr_t, since pdata->iobase is
of this type, but ioaddr seems to be a virtual address, not a physical
one. So either we make this a void*, or keep it at ulong.

>  	int ret, err;
>  	struct reset_ctl_bulk reset_bulk;
>  #ifdef CONFIG_CLK
> @@ -740,7 +740,7 @@ int designware_eth_probe(struct udevice *dev)
>  	 * or via a PCI bridge, fill in plat before we probe the hardware.
>  	 */
>  	if (IS_ENABLED(CONFIG_PCI) && device_is_on_pci_bus(dev)) {
> -		dm_pci_read_config32(dev, PCI_BASE_ADDRESS_0, &iobase);
> +		dm_pci_read_config32(dev, PCI_BASE_ADDRESS_0, (u32 *)&iobase);

This looks sketchy, relies on little endian, and assumes that iobase
has been initialised to 0 before. Since the read is explicitly 32 bits
wide, I'd declare a new local u32 variable (inside this if-statement)
and read it into there, then assign this to iobase.

>  		iobase &= PCI_BASE_ADDRESS_MEM_MASK;
>  		iobase = dm_pci_mem_to_phys(dev, iobase);
>  
> @@ -748,7 +748,7 @@ int designware_eth_probe(struct udevice *dev)
>  		pdata->phy_interface = PHY_INTERFACE_MODE_RMII;
>  	}
>  
> -	debug("%s, iobase=%x, priv=%p\n", __func__, iobase, priv);
> +	debug("%s, iobase=%llx, priv=%p\n", __func__, (u64)iobase, priv);

This assumes that u64 is always %llx, which is not universally true.
Fortunately we have %pa for that (see doc/develop/printf.rst), so you
can just use this and drop the cast.

Cheers,
Andre

>  	ioaddr = iobase;
>  	priv->mac_regs_p = (struct eth_mac_regs *)ioaddr;
>  	priv->dma_regs_p = (struct eth_dma_regs *)(ioaddr + DW_DMA_BASE_OFFSET);