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

[Christian Lamparter]

On Saturday, November 21, 2015 08:45:23 PM John Crispin wrote:
> Hi,
> 
> sorry to jump in this late at a V5 but i have a few concerns. see inline
Well, the *beautiful thing* of this platform is that Cisco charges people
a yearly fee if they want to stick with the original firmware. So people
are definitely interested in this openwrt port. Just look at the positive
response to the support thread [0].
 
> 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

Wait, the code for this function was just adapted (qca955x uses
slightly different registers and bitmask offsets) from a similar
function called:

void __init ath79_setup_ar934x_eth_rx_delay(unsigned int rxd,
                                            unsigned int rxdv)

which is also in the dev-eth.c [1] (added Felix). If this is
"pretty ugly" then what should be done about ath79_setup_ar934x_eth_rx_delay?
If it's just the function that bothers you, I can also pass the
rx/tx delays via the ath79_setup_qca955x_eth_cfg call. but I 
would like to keep the ar71xx_regs changes in place. Ok?

> > +--- 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

Regards,
Christian

[0] <https://forum.openwrt.org/viewtopic.php?id=59248>
[1] <https://dev.openwrt.org/changeset/45523>
_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel