[OpenWrt-Devel] [PATCH v5 1/5] ar71xx: add eth rx delay for qca955x platforms

[John Crispin]

Hi,

sorry to jump in this late at a V5 but i have a few concerns. see inline

On 22/09/2015 18:49, Chris R Blake wrote:
> From: Chris R Blake <chrisrblake93 at gmail.com>
> 
> This patch is to add support for qca955x_eth_rx_delay
> to work with the qca955x SoC.
> 
> Signed-off-by: Chris R Blake <chrisrblake93 at gmail.com>
> ---
>  ...42-MIPS-ath79-add-qca955x-mac-tx-rx-delay.patch | 58 ++++++++++++++++++++++
>  1 file changed, 58 insertions(+)
>  create mode 100644 target/linux/ar71xx/patches-4.1/742-MIPS-ath79-add-qca955x-mac-tx-rx-delay.patch
> 
> diff --git a/target/linux/ar71xx/patches-4.1/742-MIPS-ath79-add-qca955x-mac-tx-rx-delay.patch b/target/linux/ar71xx/patches-4.1/742-MIPS-ath79-add-qca955x-mac-tx-rx-delay.patch
> new file mode 100644
> index 0000000..75e216e
> --- /dev/null
> +++ b/target/linux/ar71xx/patches-4.1/742-MIPS-ath79-add-qca955x-mac-tx-rx-delay.patch
> @@ -0,0 +1,58 @@
> +--- a/arch/mips/ath79/dev-eth.c
> ++++ b/arch/mips/ath79/dev-eth.c
> +@@ -823,6 +825,32 @@
> + 	iounmap(base);
> + }
> +
> ++void __init ath79_setup_qca955x_eth_rx_delay(unsigned int rxd,
> ++					      unsigned int rxdv)
> ++{
> ++	void __iomem *base;
> ++	u32 t;
> ++
> ++	rxd &= QCA955X_ETH_CFG_RXD_DELAY_MASK;
> ++	rxdv &= QCA955X_ETH_CFG_RDV_DELAY_MASK;
> ++
> ++	base = ioremap(QCA955X_GMAC_BASE, QCA955X_GMAC_SIZE);
> ++
> ++	t = __raw_readl(base + QCA955X_GMAC_REG_ETH_CFG);
> ++
> ++	t &= ~(QCA955X_ETH_CFG_RXD_DELAY_MASK << QCA955X_ETH_CFG_RXD_DELAY_SHIFT |
> ++	       QCA955X_ETH_CFG_RDV_DELAY_MASK << QCA955X_ETH_CFG_RDV_DELAY_SHIFT);
> ++
> ++	t |= (rxd << QCA955X_ETH_CFG_RXD_DELAY_SHIFT |
> ++	      rxdv << QCA955X_ETH_CFG_RDV_DELAY_SHIFT);
> ++
> ++	__raw_writel(t, base + QCA955X_GMAC_REG_ETH_CFG);
> ++	/* flush write */
> ++	__raw_readl(base + QCA955X_GMAC_REG_ETH_CFG);
> ++
> ++	iounmap(base);

this is a pretty ugly way of doing it. the register is also modified at
a different place which is also not optimal. the register is part of the
ethernet macs io range so this magic setting should be applied inside
the the actual driver. could you make such a change ?

	John

> ++}
> ++
> + static int ath79_eth_instance __initdata;
> + void __init ath79_register_eth(unsigned int id)
> + {
> +--- a/arch/mips/ath79/dev-eth.h
> ++++ b/arch/mips/ath79/dev-eth.h
> +@@ -49,5 +49,6 @@
> + void ath79_setup_ar934x_eth_cfg(u32 mask);
> + void ath79_setup_ar934x_eth_rx_delay(unsigned int rxd, unsigned int rxdv);
> + void ath79_setup_qca955x_eth_cfg(u32 mask);
> ++void ath79_setup_qca955x_eth_rx_delay(unsigned int rxd, unsigned int rxdv);
> +
> + #endif /* _ATH79_DEV_ETH_H */
> +--- a/arch/mips/include/asm/mach-ath79/ar71xx_regs.h
> ++++ b/arch/mips/include/asm/mach-ath79/ar71xx_regs.h
> +@@ -1098,5 +1098,11 @@
> +
> + #define QCA955X_ETH_CFG_RGMII_EN	BIT(0)
> + #define QCA955X_ETH_CFG_GE0_SGMII	BIT(6)
> ++#define QCA955X_ETH_CFG_RXD_DELAY	BIT(14)
> ++#define QCA955X_ETH_CFG_RXD_DELAY_MASK	0x3
> ++#define QCA955X_ETH_CFG_RXD_DELAY_SHIFT	14
> ++#define QCA955X_ETH_CFG_RDV_DELAY	BIT(16)
> ++#define QCA955X_ETH_CFG_RDV_DELAY_MASK	0x3
> ++#define QCA955X_ETH_CFG_RDV_DELAY_SHIFT	16
> +
> + #endif /* __ASM_MACH_AR71XX_REGS_H */
> 
_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel