diff mbox

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

Message ID 2288287.BPFWLnqO46@debian64
State Changes Requested
Headers show

Commit Message

Christian Lamparter Sept. 22, 2015, 4:49 p.m. UTC 
From: Chris R Blake <chrisrblake93@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@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

Comments

John Crispin Nov. 21, 2015, 7:45 p.m. UTC | #1 
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@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@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 */
>
Christian Lamparter Nov. 27, 2015, 7:34 p.m. UTC | #2 
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@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@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>
Christian Lamparter Dec. 4, 2015, 10:44 p.m. UTC | #3 
Ping.

Any comments? what to do about ath79_setup_ar934x_eth_rx_delay.
Leave it as is, or get rid of it?

Regards,

Christian

On Friday, November 27, 2015 08:34:40 PM Christian Lamparter wrote:
> 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@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@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>
diff mbox

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);
++}
++
+ 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 */