| Message ID | 20230331214549.756660-1-ahalaney@redhat.com |
|---|---|
| Headers | show |
| Series | Add EMAC3 support for sa8540p-ride | expand |
On Fri, Mar 31, 2023 at 04:45:37PM -0500, Andrew Halaney wrote: > This is a forward port / upstream refactor of code delivered > downstream by Qualcomm over at [0] to enable the DWMAC5 based > implementation called EMAC3 on the sa8540p-ride dev board. > > From what I can tell with the board schematic in hand, > as well as the code delivered, the main changes needed are: > > 1. A new address space layout for /dwmac5/EMAC3 MTL/DMA regs > 2. A new programming sequence required for the EMAC3 base platforms > > This series makes the change for 1 above as well as other housekeeping items > such as converting dt-bindings to yaml, etc. > > As requested[1], it has been split up by compile time / maintainer tree. > I will post a link to the associated devicetree changes that together > with this series get the hardware functioning. As promised: https://lore.kernel.org/netdev/20230331215804.783439-1-ahalaney@redhat.com/T/#t Thanks in advance for any reviews! - Andrew
On Fri, 31 Mar 2023 17:06:13 -0500 Andrew Halaney wrote:
> As promised: https://lore.kernel.org/netdev/20230331215804.783439-1-ahalaney@redhat.com/T/#t
Patch 12 never made it to netdev or lore :(
On Fri, Mar 31, 2023 at 04:45:44PM -0500, Andrew Halaney wrote: > There's a few spots in the hardware interface where a void pointer is > used, but what's passed in and later cast out is always the same type. > > Just use the proper type directly. > > Signed-off-by: Andrew Halaney <ahalaney@redhat.com> > --- > > Changes since v2: > * New patch. In later patches refactoring hwif.h I touched these in > their first form (which was later dropped) and changed this as > part of that. But I thought the change was still good overall, > so I left this improvement in the series. FWIIW, I am very pleased to see this kind of change. Reviewed-by: Simon Horman <simon.horman@corigine.com>
On Fri, Mar 31, 2023 at 04:45:46PM -0500, Andrew Halaney wrote: > Some platforms have dwmac4 implementations that have a different > address space layout than the default, resulting in the need to define > their own DMA/MTL offsets. > > Extend the functions to allow a platform driver to indicate what its > addresses are, overriding the defaults. > > Signed-off-by: Andrew Halaney <ahalaney@redhat.com> > --- > > This patch (and the prior patch) are replacements for > https://lore.kernel.org/netdev/20230320204153.21736840@kernel.org/ > as was requested. Hopefully I was understanding the intent correctly :) > > I'm pretty sure further refinement will be requested for this one, but > it is the best I could come up with myself! Specifically some of the > naming, dealing with spacing in some older spots of dwmac4, > where the addresses should live in the structure hierarchy, etc are > things I would not be surprised to have to rework if this is still > preferred over the wrapper approach. > > Changes since v2: > * New, replacing old wrapper approach > > drivers/net/ethernet/stmicro/stmmac/dwmac4.h | 91 ++++++++-- > .../net/ethernet/stmicro/stmmac/dwmac4_core.c | 36 ++-- > .../net/ethernet/stmicro/stmmac/dwmac4_dma.c | 157 ++++++++++-------- > .../net/ethernet/stmicro/stmmac/dwmac4_dma.h | 51 +++--- > .../net/ethernet/stmicro/stmmac/dwmac4_lib.c | 67 +++++--- > include/linux/stmmac.h | 19 +++ > 6 files changed, 279 insertions(+), 142 deletions(-) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4.h b/drivers/net/ethernet/stmicro/stmmac/dwmac4.h > index ccd49346d3b3..a0c0ee1dc13f 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4.h > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4.h > @@ -336,14 +336,23 @@ enum power_event { > > #define MTL_CHAN_BASE_ADDR 0x00000d00 > #define MTL_CHAN_BASE_OFFSET 0x40 > -#define MTL_CHANX_BASE_ADDR(x) (MTL_CHAN_BASE_ADDR + \ > - (x * MTL_CHAN_BASE_OFFSET)) > - > -#define MTL_CHAN_TX_OP_MODE(x) MTL_CHANX_BASE_ADDR(x) > -#define MTL_CHAN_TX_DEBUG(x) (MTL_CHANX_BASE_ADDR(x) + 0x8) > -#define MTL_CHAN_INT_CTRL(x) (MTL_CHANX_BASE_ADDR(x) + 0x2c) > -#define MTL_CHAN_RX_OP_MODE(x) (MTL_CHANX_BASE_ADDR(x) + 0x30) > -#define MTL_CHAN_RX_DEBUG(x) (MTL_CHANX_BASE_ADDR(x) + 0x38) > +#define MTL_CHANX_BASE_ADDR(addrs, x) \ > +({ \ > + const struct dwmac4_addrs *__addrs = addrs; \ > + const u32 __x = x; \ > + u32 __addr; \ > + if (__addrs) \ > + __addr = __addrs->mtl_chan + (__x * __addrs->mtl_chan_offset); \ > + else \ > + __addr = MTL_CHAN_BASE_ADDR + (__x * MTL_CHAN_BASE_OFFSET); \ > + __addr; \ > +}) Could this and similar macros added by this patch be functions? From my pov a benefit would be slightly more type safety. And as a bonus there wouldn't be any need to handle aliasing of input.
On Fri, Mar 31, 2023 at 04:45:45PM -0500, Andrew Halaney wrote: > Passing stmmac_priv to some of the callbacks allows hwif implementations > to grab some data that platforms can customize. Adjust the callbacks > accordingly in preparation of such a platform customization. > > Signed-off-by: Andrew Halaney <ahalaney@redhat.com> ... > #define stmmac_reset(__priv, __args...) \ > @@ -223,59 +240,59 @@ struct stmmac_dma_ops { > #define stmmac_dma_init(__priv, __args...) \ > stmmac_do_void_callback(__priv, dma, init, __args) > #define stmmac_init_chan(__priv, __args...) \ > - stmmac_do_void_callback(__priv, dma, init_chan, __args) > + stmmac_do_void_callback(__priv, dma, init_chan, __priv, __args) Hi Andrew, Rather than maintaining these macros can we just get rid of them? I'd be surprised if things aren't nicer with functions in their place [1]. f.e., we now have (__priv, ..., __priv, ...) due to a generalisation that seems to take a lot more than it gives. [1] https://lore.kernel.org/linux-arm-kernel/ZBst1SzcIS4j+t46@corigine.com/ > #define stmmac_init_rx_chan(__priv, __args...) \ > - stmmac_do_void_callback(__priv, dma, init_rx_chan, __args) > + stmmac_do_void_callback(__priv, dma, init_rx_chan, __priv, __args) > #define stmmac_init_tx_chan(__priv, __args...) \ > - stmmac_do_void_callback(__priv, dma, init_tx_chan, __args) > + stmmac_do_void_callback(__priv, dma, init_tx_chan, __priv, __args) > #define stmmac_axi(__priv, __args...) \ > stmmac_do_void_callback(__priv, dma, axi, __args) > #define stmmac_dump_dma_regs(__priv, __args...) \ > - stmmac_do_void_callback(__priv, dma, dump_regs, __args) > + stmmac_do_void_callback(__priv, dma, dump_regs, __priv, __args) > #define stmmac_dma_rx_mode(__priv, __args...) \ > - stmmac_do_void_callback(__priv, dma, dma_rx_mode, __args) > + stmmac_do_void_callback(__priv, dma, dma_rx_mode, __priv, __args) > #define stmmac_dma_tx_mode(__priv, __args...) \ > - stmmac_do_void_callback(__priv, dma, dma_tx_mode, __args) > + stmmac_do_void_callback(__priv, dma, dma_tx_mode, __priv, __args) > #define stmmac_dma_diagnostic_fr(__priv, __args...) \ > stmmac_do_void_callback(__priv, dma, dma_diagnostic_fr, __args) > #define stmmac_enable_dma_transmission(__priv, __args...) \ > stmmac_do_void_callback(__priv, dma, enable_dma_transmission, __args) > #define stmmac_enable_dma_irq(__priv, __args...) \ > - stmmac_do_void_callback(__priv, dma, enable_dma_irq, __args) > + stmmac_do_void_callback(__priv, dma, enable_dma_irq, __priv, __args) > #define stmmac_disable_dma_irq(__priv, __args...) \ > - stmmac_do_void_callback(__priv, dma, disable_dma_irq, __args) > + stmmac_do_void_callback(__priv, dma, disable_dma_irq, __priv, __args) > #define stmmac_start_tx(__priv, __args...) \ > - stmmac_do_void_callback(__priv, dma, start_tx, __args) > + stmmac_do_void_callback(__priv, dma, start_tx, __priv, __args) > #define stmmac_stop_tx(__priv, __args...) \ > - stmmac_do_void_callback(__priv, dma, stop_tx, __args) > + stmmac_do_void_callback(__priv, dma, stop_tx, __priv, __args) > #define stmmac_start_rx(__priv, __args...) \ > - stmmac_do_void_callback(__priv, dma, start_rx, __args) > + stmmac_do_void_callback(__priv, dma, start_rx, __priv, __args) > #define stmmac_stop_rx(__priv, __args...) \ > - stmmac_do_void_callback(__priv, dma, stop_rx, __args) > + stmmac_do_void_callback(__priv, dma, stop_rx, __priv, __args) > #define stmmac_dma_interrupt_status(__priv, __args...) \ > - stmmac_do_callback(__priv, dma, dma_interrupt, __args) > + stmmac_do_callback(__priv, dma, dma_interrupt, __priv, __args) > #define stmmac_get_hw_feature(__priv, __args...) \ > stmmac_do_callback(__priv, dma, get_hw_feature, __args) > #define stmmac_rx_watchdog(__priv, __args...) \ > - stmmac_do_void_callback(__priv, dma, rx_watchdog, __args) > + stmmac_do_void_callback(__priv, dma, rx_watchdog, __priv, __args) > #define stmmac_set_tx_ring_len(__priv, __args...) \ > - stmmac_do_void_callback(__priv, dma, set_tx_ring_len, __args) > + stmmac_do_void_callback(__priv, dma, set_tx_ring_len, __priv, __args) > #define stmmac_set_rx_ring_len(__priv, __args...) \ > - stmmac_do_void_callback(__priv, dma, set_rx_ring_len, __args) > + stmmac_do_void_callback(__priv, dma, set_rx_ring_len, __priv, __args) > #define stmmac_set_rx_tail_ptr(__priv, __args...) \ > - stmmac_do_void_callback(__priv, dma, set_rx_tail_ptr, __args) > + stmmac_do_void_callback(__priv, dma, set_rx_tail_ptr, __priv, __args) > #define stmmac_set_tx_tail_ptr(__priv, __args...) \ > - stmmac_do_void_callback(__priv, dma, set_tx_tail_ptr, __args) > + stmmac_do_void_callback(__priv, dma, set_tx_tail_ptr, __priv, __args) > #define stmmac_enable_tso(__priv, __args...) \ > - stmmac_do_void_callback(__priv, dma, enable_tso, __args) > + stmmac_do_void_callback(__priv, dma, enable_tso, __priv, __args) > #define stmmac_dma_qmode(__priv, __args...) \ > - stmmac_do_void_callback(__priv, dma, qmode, __args) > + stmmac_do_void_callback(__priv, dma, qmode, __priv, __args) > #define stmmac_set_dma_bfsize(__priv, __args...) \ > - stmmac_do_void_callback(__priv, dma, set_bfsize, __args) > + stmmac_do_void_callback(__priv, dma, set_bfsize, __priv, __args) > #define stmmac_enable_sph(__priv, __args...) \ > - stmmac_do_void_callback(__priv, dma, enable_sph, __args) > + stmmac_do_void_callback(__priv, dma, enable_sph, __priv, __args) > #define stmmac_enable_tbs(__priv, __args...) \ > - stmmac_do_callback(__priv, dma, enable_tbs, __args) > + stmmac_do_callback(__priv, dma, enable_tbs, __priv, __args) > > struct mac_device_info; > struct net_device; > @@ -307,21 +324,23 @@ struct stmmac_ops { > /* Program TX Algorithms */ > void (*prog_mtl_tx_algorithms)(struct mac_device_info *hw, u32 tx_alg); > /* Set MTL TX queues weight */ > - void (*set_mtl_tx_queue_weight)(struct mac_device_info *hw, > + void (*set_mtl_tx_queue_weight)(struct stmmac_priv *priv, > + struct mac_device_info *hw, > u32 weight, u32 queue); > /* RX MTL queue to RX dma mapping */ > void (*map_mtl_to_dma)(struct mac_device_info *hw, u32 queue, u32 chan); > /* Configure AV Algorithm */ > - void (*config_cbs)(struct mac_device_info *hw, u32 send_slope, > - u32 idle_slope, u32 high_credit, u32 low_credit, > - u32 queue); > + void (*config_cbs)(struct stmmac_priv *priv, struct mac_device_info *hw, > + u32 send_slope, u32 idle_slope, u32 high_credit, > + u32 low_credit, u32 queue); > /* Dump MAC registers */ > void (*dump_regs)(struct mac_device_info *hw, u32 *reg_space); > /* Handle extra events on specific interrupts hw dependent */ > int (*host_irq_status)(struct mac_device_info *hw, > struct stmmac_extra_stats *x); > /* Handle MTL interrupts */ > - int (*host_mtl_irq_status)(struct mac_device_info *hw, u32 chan); > + int (*host_mtl_irq_status)(struct stmmac_priv *priv, > + struct mac_device_info *hw, u32 chan); > /* Multicast filter setting */ > void (*set_filter)(struct mac_device_info *hw, struct net_device *dev); > /* Flow control setting */ > @@ -341,8 +360,9 @@ struct stmmac_ops { > void (*set_eee_lpi_entry_timer)(struct mac_device_info *hw, int et); > void (*set_eee_timer)(struct mac_device_info *hw, int ls, int tw); > void (*set_eee_pls)(struct mac_device_info *hw, int link); > - void (*debug)(void __iomem *ioaddr, struct stmmac_extra_stats *x, > - u32 rx_queues, u32 tx_queues); > + void (*debug)(struct stmmac_priv *priv, void __iomem *ioaddr, > + struct stmmac_extra_stats *x, u32 rx_queues, > + u32 tx_queues); > /* PCS calls */ > void (*pcs_ctrl_ane)(void __iomem *ioaddr, bool ane, bool srgmi_ral, > bool loopback); ... > @@ -422,17 +442,17 @@ struct stmmac_ops { > #define stmmac_prog_mtl_tx_algorithms(__priv, __args...) \ > stmmac_do_void_callback(__priv, mac, prog_mtl_tx_algorithms, __args) > #define stmmac_set_mtl_tx_queue_weight(__priv, __args...) \ > - stmmac_do_void_callback(__priv, mac, set_mtl_tx_queue_weight, __args) > + stmmac_do_void_callback(__priv, mac, set_mtl_tx_queue_weight, __priv, __args) > #define stmmac_map_mtl_to_dma(__priv, __args...) \ > stmmac_do_void_callback(__priv, mac, map_mtl_to_dma, __args) > #define stmmac_config_cbs(__priv, __args...) \ > - stmmac_do_void_callback(__priv, mac, config_cbs, __args) > + stmmac_do_void_callback(__priv, mac, config_cbs, __priv, __args) > #define stmmac_dump_mac_regs(__priv, __args...) \ > stmmac_do_void_callback(__priv, mac, dump_regs, __args) > #define stmmac_host_irq_status(__priv, __args...) \ > stmmac_do_callback(__priv, mac, host_irq_status, __args) > #define stmmac_host_mtl_irq_status(__priv, __args...) \ > - stmmac_do_callback(__priv, mac, host_mtl_irq_status, __args) > + stmmac_do_callback(__priv, mac, host_mtl_irq_status, __priv, __args) > #define stmmac_set_filter(__priv, __args...) \ > stmmac_do_void_callback(__priv, mac, set_filter, __args) > #define stmmac_flow_ctrl(__priv, __args...) \ > @@ -454,11 +474,11 @@ struct stmmac_ops { > #define stmmac_set_eee_pls(__priv, __args...) \ > stmmac_do_void_callback(__priv, mac, set_eee_pls, __args) > #define stmmac_mac_debug(__priv, __args...) \ > - stmmac_do_void_callback(__priv, mac, debug, __args) > + stmmac_do_void_callback(__priv, mac, debug, __priv, __args) > #define stmmac_pcs_ctrl_ane(__priv, __args...) \ > stmmac_do_void_callback(__priv, mac, pcs_ctrl_ane, __args) > #define stmmac_pcs_rane(__priv, __args...) \ > - stmmac_do_void_callback(__priv, mac, pcs_rane, __args) > + stmmac_do_void_callback(__priv, mac, pcs_rane, __priv, __args) > #define stmmac_pcs_get_adv_lp(__priv, __args...) \ > stmmac_do_void_callback(__priv, mac, pcs_get_adv_lp, __args) > #define stmmac_safety_feat_config(__priv, __args...) \ > @@ -506,8 +526,6 @@ struct stmmac_ops { > #define stmmac_fpe_irq_status(__priv, __args...) \ > stmmac_do_callback(__priv, mac, fpe_irq_status, __args)
On Fri, Mar 31, 2023 at 09:55:04PM -0700, Jakub Kicinski wrote: > On Fri, 31 Mar 2023 17:06:13 -0500 Andrew Halaney wrote: > > As promised: https://lore.kernel.org/netdev/20230331215804.783439-1-ahalaney@redhat.com/T/#t > > Patch 12 never made it to netdev or lore :( > Well, that's no good as I definitely want some eyes on that one :( I've already gotten _some_ reviews on the earlier patches in v3, I am going to absorb anything super quick into a v4 today, then send that out, maybe copy pasting larger questions I have yet to respond to? Seems like an ok approach.. not having the full solution in hand is crummy for review. Let me know if you think that's a bad call and just a resend is better. Sorry, not sure where I messed that up Friday evening when writing changelogs etc.
Hi, On Mon, 2023-04-03 at 11:52 -0500, Andrew Halaney wrote: > @@ -327,9 +370,17 @@ static int ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos) > RGMII_CONFIG2_RX_PROG_SWAP, > RGMII_IO_MACRO_CONFIG2); > > - /* Set PRG_RCLK_DLY to 57 for 1.8 ns delay */ > - rgmii_updatel(ethqos, SDCC_DDR_CONFIG_PRG_RCLK_DLY, > - 57, SDCC_HC_REG_DDR_CONFIG); > + /* PRG_RCLK_DLY = TCXO period * TCXO_CYCLES_CNT / 2 * RX delay ns, > + * in practice this becomes PRG_RCLK_DLY = 52 * 4 / 2 * RX delay ns > + */ > + if (ethqos->has_emac3) > + /* 0.9 ns */ > + rgmii_updatel(ethqos, SDCC_DDR_CONFIG_PRG_RCLK_DLY, > + 115, SDCC_HC_REG_DDR_CONFIG); > + else > + /* 1.8 ns */ > + rgmii_updatel(ethqos, SDCC_DDR_CONFIG_PRG_RCLK_DLY, > + 57, SDCC_HC_REG_DDR_CONFIG); The only (very minor) comment I have is that AFAIK the preferred style for the above block is: if (ethqos->has_emac3) { /* 0.9 ns */ rgmii_updatel(ethqos, SDCC_DDR_CONFIG_PRG_RCLK_DLY, 115, SDCC_HC_REG_DDR_CONFIG); } else { ... due to the comment presence this should be threaded as a multi-line statement. (even if checkpatch does not complain). Cheers, Paolo > SDCC_HC_REG_DDR_CONFIG); > @@ -355,8 +406,15 @@ static int ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos) > BIT(6), RGMII_IO_MACRO_CONFIG); > rgmii_updatel(ethqos, RGMII_CONFIG2_RSVD_CONFIG15, > 0, RGMII_IO_MACRO_CONFIG2); > - rgmii_updatel(ethqos, RGMII_CONFIG2_RX_PROG_SWAP, > - 0, RGMII_IO_MACRO_CONFIG2); > + > + if (ethqos->has_emac3) > + rgmii_updatel(ethqos, RGMII_CONFIG2_RX_PROG_SWAP, > + RGMII_CONFIG2_RX_PROG_SWAP, > + RGMII_IO_MACRO_CONFIG2); > + else > + rgmii_updatel(ethqos, RGMII_CONFIG2_RX_PROG_SWAP, > + 0, RGMII_IO_MACRO_CONFIG2); > + > /* Write 0x5 to PRG_RCLK_DLY_CODE */ > rgmii_updatel(ethqos, SDCC_DDR_CONFIG_EXT_PRG_RCLK_DLY_CODE, > (BIT(29) | BIT(27)), SDCC_HC_REG_DDR_CONFIG); > @@ -389,8 +447,13 @@ static int ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos) > RGMII_IO_MACRO_CONFIG); > rgmii_updatel(ethqos, RGMII_CONFIG2_RSVD_CONFIG15, > 0, RGMII_IO_MACRO_CONFIG2); > - rgmii_updatel(ethqos, RGMII_CONFIG2_RX_PROG_SWAP, > - 0, RGMII_IO_MACRO_CONFIG2); > + if (ethqos->has_emac3) > + rgmii_updatel(ethqos, RGMII_CONFIG2_RX_PROG_SWAP, > + RGMII_CONFIG2_RX_PROG_SWAP, > + RGMII_IO_MACRO_CONFIG2); > + else > + rgmii_updatel(ethqos, RGMII_CONFIG2_RX_PROG_SWAP, > + 0, RGMII_IO_MACRO_CONFIG2); > /* Write 0x5 to PRG_RCLK_DLY_CODE */ > rgmii_updatel(ethqos, SDCC_DDR_CONFIG_EXT_PRG_RCLK_DLY_CODE, > (BIT(29) | BIT(27)), SDCC_HC_REG_DDR_CONFIG); > @@ -433,6 +496,17 @@ static int ethqos_configure(struct qcom_ethqos *ethqos) > rgmii_updatel(ethqos, SDCC_DLL_CONFIG_PDN, > SDCC_DLL_CONFIG_PDN, SDCC_HC_REG_DLL_CONFIG); > > + if (ethqos->has_emac3) { > + if (ethqos->speed == SPEED_1000) { > + rgmii_writel(ethqos, 0x1800000, SDCC_TEST_CTL); > + rgmii_writel(ethqos, 0x2C010800, SDCC_USR_CTL); > + rgmii_writel(ethqos, 0xA001, SDCC_HC_REG_DLL_CONFIG2); > + } else { > + rgmii_writel(ethqos, 0x40010800, SDCC_USR_CTL); > + rgmii_writel(ethqos, 0xA001, SDCC_HC_REG_DLL_CONFIG2); > + } > + } > + > /* Clear DLL_RST */ > rgmii_updatel(ethqos, SDCC_DLL_CONFIG_DLL_RST, 0, > SDCC_HC_REG_DLL_CONFIG); > @@ -452,7 +526,9 @@ static int ethqos_configure(struct qcom_ethqos *ethqos) > SDCC_HC_REG_DLL_CONFIG); > > /* Set USR_CTL bit 26 with mask of 3 bits */ > - rgmii_updatel(ethqos, GENMASK(26, 24), BIT(26), SDCC_USR_CTL); > + if (!ethqos->has_emac3) > + rgmii_updatel(ethqos, GENMASK(26, 24), BIT(26), > + SDCC_USR_CTL); > > /* wait for DLL LOCK */ > do { > @@ -547,6 +623,7 @@ static int qcom_ethqos_probe(struct platform_device *pdev) > ethqos->por = data->por; > ethqos->num_por = data->num_por; > ethqos->rgmii_config_loopback_en = data->rgmii_config_loopback_en; > + ethqos->has_emac3 = data->has_emac3; > > ethqos->rgmii_clk = devm_clk_get(&pdev->dev, "rgmii"); > if (IS_ERR(ethqos->rgmii_clk)) { > @@ -566,6 +643,7 @@ static int qcom_ethqos_probe(struct platform_device *pdev) > plat_dat->fix_mac_speed = ethqos_fix_mac_speed; > plat_dat->dump_debug_regs = rgmii_dump; > plat_dat->has_gmac4 = 1; > + plat_dat->dwmac4_addrs = &data->dwmac4_addrs; > plat_dat->pmt = 1; > plat_dat->tso_en = of_property_read_bool(np, "snps,tso"); > if (of_device_is_compatible(np, "qcom,qcs404-ethqos")) > @@ -603,6 +681,7 @@ static int qcom_ethqos_remove(struct platform_device *pdev) > > static const struct of_device_id qcom_ethqos_match[] = { > { .compatible = "qcom,qcs404-ethqos", .data = &emac_v2_3_0_data}, > + { .compatible = "qcom,sc8280xp-ethqos", .data = &emac_v3_0_0_data}, > { .compatible = "qcom,sm8150-ethqos", .data = &emac_v2_1_0_data}, > { } > };
On Sat, Apr 01, 2023 at 05:06:21PM +0200, Simon Horman wrote: > On Fri, Mar 31, 2023 at 04:45:45PM -0500, Andrew Halaney wrote: > > Passing stmmac_priv to some of the callbacks allows hwif implementations > > to grab some data that platforms can customize. Adjust the callbacks > > accordingly in preparation of such a platform customization. > > > > Signed-off-by: Andrew Halaney <ahalaney@redhat.com> > > ... > > > #define stmmac_reset(__priv, __args...) \ > > @@ -223,59 +240,59 @@ struct stmmac_dma_ops { > > #define stmmac_dma_init(__priv, __args...) \ > > stmmac_do_void_callback(__priv, dma, init, __args) > > #define stmmac_init_chan(__priv, __args...) \ > > - stmmac_do_void_callback(__priv, dma, init_chan, __args) > > + stmmac_do_void_callback(__priv, dma, init_chan, __priv, __args) > > Hi Andrew, > > Rather than maintaining these macros can we just get rid of them? > I'd be surprised if things aren't nicer with functions in their place [1]. > > f.e., we now have (__priv, ..., __priv, ...) due to a generalisation > that seems to take a lot more than it gives. > > [1] https://lore.kernel.org/linux-arm-kernel/ZBst1SzcIS4j+t46@corigine.com/ > Thanks for the pointer. I think that makes sense, I'll take that approach for these functions (and maybe in a follow-up series I'll tackle all of them just because the lack of consistency will eat me up). Sorry for the delay, had some issues around the house that became urgent. Thanks, Andrew
On Sat, Apr 01, 2023 at 04:58:59PM +0200, Simon Horman wrote: > On Fri, Mar 31, 2023 at 04:45:46PM -0500, Andrew Halaney wrote: > > Some platforms have dwmac4 implementations that have a different > > address space layout than the default, resulting in the need to define > > their own DMA/MTL offsets. > > > > Extend the functions to allow a platform driver to indicate what its > > addresses are, overriding the defaults. > > > > Signed-off-by: Andrew Halaney <ahalaney@redhat.com> > > --- > > > > This patch (and the prior patch) are replacements for > > https://lore.kernel.org/netdev/20230320204153.21736840@kernel.org/ > > as was requested. Hopefully I was understanding the intent correctly :) > > > > I'm pretty sure further refinement will be requested for this one, but > > it is the best I could come up with myself! Specifically some of the > > naming, dealing with spacing in some older spots of dwmac4, > > where the addresses should live in the structure hierarchy, etc are > > things I would not be surprised to have to rework if this is still > > preferred over the wrapper approach. > > > > Changes since v2: > > * New, replacing old wrapper approach > > > > drivers/net/ethernet/stmicro/stmmac/dwmac4.h | 91 ++++++++-- > > .../net/ethernet/stmicro/stmmac/dwmac4_core.c | 36 ++-- > > .../net/ethernet/stmicro/stmmac/dwmac4_dma.c | 157 ++++++++++-------- > > .../net/ethernet/stmicro/stmmac/dwmac4_dma.h | 51 +++--- > > .../net/ethernet/stmicro/stmmac/dwmac4_lib.c | 67 +++++--- > > include/linux/stmmac.h | 19 +++ > > 6 files changed, 279 insertions(+), 142 deletions(-) > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4.h b/drivers/net/ethernet/stmicro/stmmac/dwmac4.h > > index ccd49346d3b3..a0c0ee1dc13f 100644 > > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4.h > > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4.h > > @@ -336,14 +336,23 @@ enum power_event { > > > > #define MTL_CHAN_BASE_ADDR 0x00000d00 > > #define MTL_CHAN_BASE_OFFSET 0x40 > > -#define MTL_CHANX_BASE_ADDR(x) (MTL_CHAN_BASE_ADDR + \ > > - (x * MTL_CHAN_BASE_OFFSET)) > > - > > -#define MTL_CHAN_TX_OP_MODE(x) MTL_CHANX_BASE_ADDR(x) > > -#define MTL_CHAN_TX_DEBUG(x) (MTL_CHANX_BASE_ADDR(x) + 0x8) > > -#define MTL_CHAN_INT_CTRL(x) (MTL_CHANX_BASE_ADDR(x) + 0x2c) > > -#define MTL_CHAN_RX_OP_MODE(x) (MTL_CHANX_BASE_ADDR(x) + 0x30) > > -#define MTL_CHAN_RX_DEBUG(x) (MTL_CHANX_BASE_ADDR(x) + 0x38) > > +#define MTL_CHANX_BASE_ADDR(addrs, x) \ > > +({ \ > > + const struct dwmac4_addrs *__addrs = addrs; \ > > + const u32 __x = x; \ > > + u32 __addr; \ > > + if (__addrs) \ > > + __addr = __addrs->mtl_chan + (__x * __addrs->mtl_chan_offset); \ > > + else \ > > + __addr = MTL_CHAN_BASE_ADDR + (__x * MTL_CHAN_BASE_OFFSET); \ > > + __addr; \ > > +}) > > Could this and similar macros added by this patch be functions? > From my pov a benefit would be slightly more type safety. > And as a bonus there wouldn't be any need to handle aliasing of input. > Sure, to be honest I'll be much more comfortable coding that up anyways. I don't do a ton of macro programming and had to refamiliarize myself of the pitfalls that comes with it when doing this. Thanks, Andrew
On Fri, Apr 07, 2023 at 12:34:53PM -0500, Andrew Halaney wrote: > On Sat, Apr 01, 2023 at 05:06:21PM +0200, Simon Horman wrote: > > On Fri, Mar 31, 2023 at 04:45:45PM -0500, Andrew Halaney wrote: > > > Passing stmmac_priv to some of the callbacks allows hwif implementations > > > to grab some data that platforms can customize. Adjust the callbacks > > > accordingly in preparation of such a platform customization. > > > > > > Signed-off-by: Andrew Halaney <ahalaney@redhat.com> > > > > ... > > > > > #define stmmac_reset(__priv, __args...) \ > > > @@ -223,59 +240,59 @@ struct stmmac_dma_ops { > > > #define stmmac_dma_init(__priv, __args...) \ > > > stmmac_do_void_callback(__priv, dma, init, __args) > > > #define stmmac_init_chan(__priv, __args...) \ > > > - stmmac_do_void_callback(__priv, dma, init_chan, __args) > > > + stmmac_do_void_callback(__priv, dma, init_chan, __priv, __args) > > > > Hi Andrew, > > > > Rather than maintaining these macros can we just get rid of them? > > I'd be surprised if things aren't nicer with functions in their place [1]. > > > > f.e., we now have (__priv, ..., __priv, ...) due to a generalisation > > that seems to take a lot more than it gives. > > > > [1] https://lore.kernel.org/linux-arm-kernel/ZBst1SzcIS4j+t46@corigine.com/ > > > > Thanks for the pointer. I think that makes sense, I'll take that > approach for these functions (and maybe in a follow-up series I'll > tackle all of them just because the lack of consistency will eat me up). > I tried taking this approach for a spin, and I'm not so sure about it now! 1. Implementing the functions as static inline requires us to know about stmmac_priv, but that's getting into circular dependency land 2. You could define them in hwif.c, but then they're not inlined 3. There's still a good bit of boilerplate that's repeated all over with the approach. Ignoring 1 above, you get something like this: static inline int stmmac_init_chan(struct stmmac_priv *priv, void __iomem *ioaddr, struct stmmac_dma_cfg *dma_cfg, u32 chan) { if (priv->hw->dma && priv->hw->dma->init_chan) { priv->hw->dma->init_chan(priv, ioaddr, dma_cfg, chan); return 0; } return -EINVAL; } that is then repeated for every function... which is making me actually appreciate the macros some for reducing boilerplate. Am I suffering from a case of holiday brain, and 1-3 above are silly points with obvious answers, or do they make you reconsider continuing with the current approach in hwif.h? Thanks, Andrew
On Mon, Apr 10, 2023 at 04:24:22PM -0500, Andrew Halaney wrote: > On Fri, Apr 07, 2023 at 12:34:53PM -0500, Andrew Halaney wrote: > > On Sat, Apr 01, 2023 at 05:06:21PM +0200, Simon Horman wrote: > > > On Fri, Mar 31, 2023 at 04:45:45PM -0500, Andrew Halaney wrote: > > > > Passing stmmac_priv to some of the callbacks allows hwif implementations > > > > to grab some data that platforms can customize. Adjust the callbacks > > > > accordingly in preparation of such a platform customization. > > > > > > > > Signed-off-by: Andrew Halaney <ahalaney@redhat.com> > > > > > > ... > > > > > > > #define stmmac_reset(__priv, __args...) \ > > > > @@ -223,59 +240,59 @@ struct stmmac_dma_ops { > > > > #define stmmac_dma_init(__priv, __args...) \ > > > > stmmac_do_void_callback(__priv, dma, init, __args) > > > > #define stmmac_init_chan(__priv, __args...) \ > > > > - stmmac_do_void_callback(__priv, dma, init_chan, __args) > > > > + stmmac_do_void_callback(__priv, dma, init_chan, __priv, __args) > > > > > > Hi Andrew, > > > > > > Rather than maintaining these macros can we just get rid of them? > > > I'd be surprised if things aren't nicer with functions in their place [1]. > > > > > > f.e., we now have (__priv, ..., __priv, ...) due to a generalisation > > > that seems to take a lot more than it gives. > > > > > > [1] https://lore.kernel.org/linux-arm-kernel/ZBst1SzcIS4j+t46@corigine.com/ > > > > > > > Thanks for the pointer. I think that makes sense, I'll take that > > approach for these functions (and maybe in a follow-up series I'll > > tackle all of them just because the lack of consistency will eat me up). > > > > I tried taking this approach for a spin, and I'm not so sure about it > now! > > 1. Implementing the functions as static inline requires us to know > about stmmac_priv, but that's getting into circular dependency land > 2. You could define them in hwif.c, but then they're not inlined > 3. There's still a good bit of boilerplate that's repeated all over > with the approach. Ignoring 1 above, you get something like this: > > static inline int stmmac_init_chan(struct stmmac_priv *priv, > void __iomem *ioaddr, > struct stmmac_dma_cfg *dma_cfg, u32 chan) > { > if (priv->hw->dma && priv->hw->dma->init_chan) { > priv->hw->dma->init_chan(priv, ioaddr, dma_cfg, chan); > return 0; > } > return -EINVAL; > } > > that is then repeated for every function... which is making me actually > appreciate the macros some for reducing boilerplate. > > Am I suffering from a case of holiday brain, and 1-3 above are silly > points with obvious answers, or do they make you reconsider continuing > with the current approach in hwif.h? I'm about to embark to the holiday brain zone. But before I do I wanted to acknowledge your concerns and that, yes, it may be easier said than done.
This is a forward port / upstream refactor of code delivered downstream by Qualcomm over at [0] to enable the DWMAC5 based implementation called EMAC3 on the sa8540p-ride dev board. From what I can tell with the board schematic in hand, as well as the code delivered, the main changes needed are: 1. A new address space layout for /dwmac5/EMAC3 MTL/DMA regs 2. A new programming sequence required for the EMAC3 base platforms This series makes the change for 1 above as well as other housekeeping items such as converting dt-bindings to yaml, etc. As requested[1], it has been split up by compile time / maintainer tree. I will post a link to the associated devicetree changes that together with this series get the hardware functioning. [0] https://git.codelinaro.org/clo/la/kernel/ark-5.14/-/commit/510235ad02d7f0df478146fb00d7a4ba74821b17 [1] https://lore.kernel.org/netdev/20230320202802.4e7dc54c@kernel.org/ v2: https://lore.kernel.org/netdev/20230320221617.236323-1-ahalaney@redhat.com/ v1: https://lore.kernel.org/netdev/20230313165620.128463-1-ahalaney@redhat.com/ Thanks, Andrew Andrew Halaney (9): dt-bindings: net: qcom,ethqos: Add Qualcomm sc8280xp compatibles net: stmmac: Remove unnecessary if statement brackets net: stmmac: Fix DMA typo net: stmmac: Remove some unnecessary void pointers net: stmmac: Pass stmmac_priv in some callbacks net: stmmac: dwmac4: Allow platforms to specify some DMA/MTL offsets net: stmmac: dwmac-qcom-ethqos: Respect phy-mode and TX delay net: stmmac: dwmac-qcom-ethqos: Use loopback_en for all speeds net: stmmac: dwmac-qcom-ethqos: Add EMAC3 support Bhupesh Sharma (3): dt-bindings: net: snps,dwmac: Update interrupt-names dt-bindings: net: snps,dwmac: Add Qualcomm Ethernet ETHQOS compatibles dt-bindings: net: qcom,ethqos: Convert bindings to yaml .../devicetree/bindings/net/qcom,ethqos.txt | 66 ------ .../devicetree/bindings/net/qcom,ethqos.yaml | 111 ++++++++++ .../devicetree/bindings/net/snps,dwmac.yaml | 9 +- MAINTAINERS | 2 +- .../net/ethernet/stmicro/stmmac/chain_mode.c | 10 +- drivers/net/ethernet/stmicro/stmmac/common.h | 2 +- .../stmicro/stmmac/dwmac-qcom-ethqos.c | 177 +++++++++++---- .../net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 36 ++-- .../ethernet/stmicro/stmmac/dwmac1000_core.c | 3 +- .../ethernet/stmicro/stmmac/dwmac1000_dma.c | 19 +- .../ethernet/stmicro/stmmac/dwmac100_dma.c | 14 +- drivers/net/ethernet/stmicro/stmmac/dwmac4.h | 91 ++++++-- .../net/ethernet/stmicro/stmmac/dwmac4_core.c | 50 +++-- .../ethernet/stmicro/stmmac/dwmac4_descs.c | 8 +- .../net/ethernet/stmicro/stmmac/dwmac4_dma.c | 201 +++++++++++------- .../net/ethernet/stmicro/stmmac/dwmac4_dma.h | 89 +++++--- .../net/ethernet/stmicro/stmmac/dwmac4_lib.c | 105 +++++---- .../net/ethernet/stmicro/stmmac/dwmac_dma.h | 22 +- .../net/ethernet/stmicro/stmmac/dwmac_lib.c | 18 +- .../ethernet/stmicro/stmmac/dwxgmac2_core.c | 9 +- .../ethernet/stmicro/stmmac/dwxgmac2_descs.c | 6 +- .../ethernet/stmicro/stmmac/dwxgmac2_dma.c | 71 ++++--- .../net/ethernet/stmicro/stmmac/enh_desc.c | 11 +- drivers/net/ethernet/stmicro/stmmac/hwif.h | 176 ++++++++------- .../net/ethernet/stmicro/stmmac/norm_desc.c | 8 +- .../net/ethernet/stmicro/stmmac/ring_mode.c | 10 +- .../net/ethernet/stmicro/stmmac/stmmac_mdio.c | 3 +- include/linux/stmmac.h | 19 ++ 28 files changed, 871 insertions(+), 475 deletions(-) delete mode 100644 Documentation/devicetree/bindings/net/qcom,ethqos.txt create mode 100644 Documentation/devicetree/bindings/net/qcom,ethqos.yaml