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);