Re: [RESEND PATCH v2 3/5] net: dwc_eth_qos: Adapt probe() for PCI devices

Hi Marek,

Thank you for the review!

On Sun, Jun 30, 2024 at 07:33:33AM +0200, Marek Vasut wrote:

On 6/24/24 10:34 AM, Philip Oberfichtner wrote:

...

PCI devices do not necessarily use a device tree. In that case, the driver currently fails to find eqos->config and eqos->regs.

This commit factors out the respective functionality. Device tree usage remains default, but board specific implementations will be possible as well.

Signed-off-by: Philip Oberfichtner pro@denx.de

drivers/net/dwc_eth_qos.c | 28 +++++++++++++++++++++++++--- drivers/net/dwc_eth_qos.h | 3 +++ drivers/net/dwc_eth_qos_imx.c | 1 + drivers/net/dwc_eth_qos_qcom.c | 1 + drivers/net/dwc_eth_qos_rockchip.c | 1 + drivers/net/dwc_eth_qos_starfive.c | 1 + drivers/net/dwc_eth_qos_stm32.c | 1 + 7 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c index b87634f1f5..34e29301cc 100644 --- a/drivers/net/dwc_eth_qos.c +++ b/drivers/net/dwc_eth_qos.c @@ -1376,6 +1376,21 @@ static int eqos_remove_resources_tegra186(struct udevice *dev) return 0; } +/*

• • Get driver data based on the device tree. Boards not using a device tree can
• • overwrite this function.
• */

+__weak void *eqos_get_driver_data(struct udevice *dev) +{

• return (void *)dev_get_driver_data(dev);

+}

+/* Function to get base address, useable for all boards having a device tree. */ +fdt_addr_t eqos_get_base_addr_dt(struct udevice *dev) +{

• return dev_read_addr(dev);

+}

• static int eqos_probe(struct udevice *dev) { struct eqos_priv *eqos = dev_get_priv(dev);

@@ -1384,13 +1399,19 @@ static int eqos_probe(struct udevice *dev) debug("%s(dev=%p):\n", __func__, dev); eqos->dev = dev;

• eqos->config = (void *)dev_get_driver_data(dev);
• eqos->regs = dev_read_addr(dev);

• eqos->config = eqos_get_driver_data(dev);
• if (!eqos->config) {

• pr_err("Failed to get driver data.\n");
  

• return -ENODEV;
  

• }
• eqos->regs = eqos->config->ops->eqos_get_base_addr(dev); if (eqos->regs == FDT_ADDR_T_NONE) {

• pr_err("dev_read_addr() failed\n");
  

• pr_err("Failed to get device address.\n");
  

  return -ENODEV; }
• eqos->mac_regs = (void *)(eqos->regs + EQOS_MAC_REGS_BASE); eqos->mtl_regs = (void *)(eqos->regs + EQOS_MTL_REGS_BASE); eqos->dma_regs = (void *)(eqos->regs + EQOS_DMA_REGS_BASE);

Could you factor out the entirety of:

     eqos->regs = dev_read_addr(dev);
     if (eqos->regs == FDT_ADDR_T_NONE) {
             pr_err("dev_read_addr() failed\n");
             return -ENODEV;
     }
     eqos->mac_regs = (void *)(eqos->regs + EQOS_MAC_REGS_BASE);
     eqos->mtl_regs = (void *)(eqos->regs + EQOS_MTL_REGS_BASE);
     eqos->dma_regs = (void *)(eqos->regs + EQOS_DMA_REGS_BASE);
     eqos->tegra186_regs = (void *)(eqos->regs +

EQOS_TEGRA186_REGS_BASE);

into some common function, like eqos_get_base_addr_dt() , and call it from .eqos_probe_resources callback instead ? That way you wouldn't have to add new callback specifically for register address.

Good point. I'll include it in V3.

Best regards, Philip