| Message ID | 20240624083450.90297-4-pro@denx.de |
|---|---|
| State | Superseded |
| Delegated to: | Ramon Fried |
| Headers | show |
| Series | net: dwc_eth_qos: Add glue driver for Intel MAC | expand |
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.
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
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); @@ -1497,6 +1518,7 @@ static struct eqos_ops eqos_tegra186_ops = { .eqos_flush_buffer = eqos_flush_buffer_tegra186, .eqos_probe_resources = eqos_probe_resources_tegra186, .eqos_remove_resources = eqos_remove_resources_tegra186, + .eqos_get_base_addr = eqos_get_base_addr_dt, .eqos_stop_resets = eqos_stop_resets_tegra186, .eqos_start_resets = eqos_start_resets_tegra186, .eqos_stop_clks = eqos_stop_clks_tegra186, diff --git a/drivers/net/dwc_eth_qos.h b/drivers/net/dwc_eth_qos.h index c7d3e23ab7..829eb0a565 100644 --- a/drivers/net/dwc_eth_qos.h +++ b/drivers/net/dwc_eth_qos.h @@ -239,6 +239,7 @@ struct eqos_ops { void (*eqos_flush_buffer)(void *buf, size_t size); int (*eqos_probe_resources)(struct udevice *dev); int (*eqos_remove_resources)(struct udevice *dev); + fdt_addr_t (*eqos_get_base_addr)(struct udevice *dev); int (*eqos_stop_resets)(struct udevice *dev); int (*eqos_start_resets)(struct udevice *dev); int (*eqos_stop_clks)(struct udevice *dev); @@ -290,6 +291,8 @@ void eqos_flush_desc_generic(void *desc); void eqos_inval_buffer_generic(void *buf, size_t size); void eqos_flush_buffer_generic(void *buf, size_t size); int eqos_null_ops(struct udevice *dev); +void *eqos_get_driver_data(struct udevice *dev); +fdt_addr_t eqos_get_base_addr_dt(struct udevice *dev); extern struct eqos_config eqos_imx_config; extern struct eqos_config eqos_rockchip_config; diff --git a/drivers/net/dwc_eth_qos_imx.c b/drivers/net/dwc_eth_qos_imx.c index 9c4e390441..21dbf45a22 100644 --- a/drivers/net/dwc_eth_qos_imx.c +++ b/drivers/net/dwc_eth_qos_imx.c @@ -218,6 +218,7 @@ static struct eqos_ops eqos_imx_ops = { .eqos_flush_buffer = eqos_flush_buffer_generic, .eqos_probe_resources = eqos_probe_resources_imx, .eqos_remove_resources = eqos_remove_resources_imx, + .eqos_get_base_addr = eqos_get_base_addr_dt, .eqos_stop_resets = eqos_null_ops, .eqos_start_resets = eqos_null_ops, .eqos_stop_clks = eqos_stop_clks_imx, diff --git a/drivers/net/dwc_eth_qos_qcom.c b/drivers/net/dwc_eth_qos_qcom.c index 8178138fc6..78213e5ca0 100644 --- a/drivers/net/dwc_eth_qos_qcom.c +++ b/drivers/net/dwc_eth_qos_qcom.c @@ -589,6 +589,7 @@ static struct eqos_ops eqos_qcom_ops = { .eqos_flush_buffer = eqos_flush_buffer_generic, .eqos_probe_resources = eqos_probe_resources_qcom, .eqos_remove_resources = eqos_remove_resources_qcom, + .eqos_get_base_addr = eqos_get_base_addr_dt, .eqos_stop_resets = eqos_null_ops, .eqos_start_resets = eqos_start_resets_qcom, .eqos_stop_clks = eqos_stop_clks_qcom, diff --git a/drivers/net/dwc_eth_qos_rockchip.c b/drivers/net/dwc_eth_qos_rockchip.c index fa9e513fae..2c67753ec9 100644 --- a/drivers/net/dwc_eth_qos_rockchip.c +++ b/drivers/net/dwc_eth_qos_rockchip.c @@ -504,6 +504,7 @@ static struct eqos_ops eqos_rockchip_ops = { .eqos_flush_buffer = eqos_flush_buffer_generic, .eqos_probe_resources = eqos_probe_resources_rk, .eqos_remove_resources = eqos_remove_resources_rk, + .eqos_get_base_addr = eqos_get_base_addr_dt, .eqos_stop_resets = eqos_stop_resets_rk, .eqos_start_resets = eqos_start_resets_rk, .eqos_stop_clks = eqos_stop_clks_rk, diff --git a/drivers/net/dwc_eth_qos_starfive.c b/drivers/net/dwc_eth_qos_starfive.c index 5be8ac0f1a..341fd2b946 100644 --- a/drivers/net/dwc_eth_qos_starfive.c +++ b/drivers/net/dwc_eth_qos_starfive.c @@ -219,6 +219,7 @@ static struct eqos_ops eqos_jh7110_ops = { .eqos_flush_buffer = eqos_flush_buffer_generic, .eqos_probe_resources = eqos_probe_resources_jh7110, .eqos_remove_resources = eqos_remove_resources_jh7110, + .eqos_get_base_addr = eqos_get_base_addr_dt, .eqos_stop_resets = eqos_stop_resets_jh7110, .eqos_start_resets = eqos_start_resets_jh7110, .eqos_stop_clks = eqos_stop_clks_jh7110, diff --git a/drivers/net/dwc_eth_qos_stm32.c b/drivers/net/dwc_eth_qos_stm32.c index fbc08bba1d..2c18896b59 100644 --- a/drivers/net/dwc_eth_qos_stm32.c +++ b/drivers/net/dwc_eth_qos_stm32.c @@ -291,6 +291,7 @@ static struct eqos_ops eqos_stm32_ops = { .eqos_flush_buffer = eqos_flush_buffer_generic, .eqos_probe_resources = eqos_probe_resources_stm32, .eqos_remove_resources = eqos_remove_resources_stm32, + .eqos_get_base_addr = eqos_get_base_addr_dt, .eqos_stop_resets = eqos_null_ops, .eqos_start_resets = eqos_null_ops, .eqos_stop_clks = eqos_stop_clks_stm32,
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(-)