Re: [PATCH 4/4] net: dsa: Add Lantiq / Intel DSA driver for vrx200
> >> +static const struct gswip_rmon_cnt_desc gswip_rmon_cnt[] = {
> >> + /** Receive Packet Count (only packets that are accepted and not
> >> discarded). */
> >> + MIB_DESC(1, 0x1F, "RxGoodPkts"),
> >> + /** Receive Size 1024-1522 (or more, if configured) Packet Count. */
> >> + MIB_DESC(1, 0x17, "RxMaxBytePkts"),
> >> + /** Transmit Size 1024-1522 (or more, if configured) Packet Count. */
> >> + MIB_DESC(1, 0x05, "TxMaxBytePkts"),
> >
> > Most of the comments here don't add anything useful. Maybe remove
> > them?
>
> Ok I removed them.
The comments i left above are useful, since they give additional
information which is not obvious from the name.
> Are the names ok, or should they follow any Linux definition?
There are no standard names. So each driver tends to be different.
> > Please return ETIMEOUT when needed. Maybe use one of the variants of
> > readx_poll_timeout().
>
> I am returning ETIMEOUT now.
>
> When I would use readx_poll_timeout() I can not use the gswip_mdio_r()
> function, because it takes two arguments and would have to use readl
> directly.
Yes, they don't always fit, which is why is said "maybe".
> > The names make this unclear. The callback is used to configure the MAC
> > layer when something happens at the PHY layer. phyaddr does not appear
> > to be an address, not should it be doing anything to a PHY.
>
> I renamed this to phyconf, as this contains multiple configuration
> values. This tells the mac what settings the phy wants to use.
macconf might be better, since this is configuring the MAC, not the
PHY.
> This is sort of a firmware, but it is also in the GPL driver.
> Currently the probe function is not marked __init so we can not make
> this easily __initdata.
> It has 64 entries of 8 bytes each so, 512 bytes, I think we can put this
> into the code.
512 bytes is fine.
Andrew
Re: [PATCH 4/4] net: dsa: Add Lantiq / Intel DSA driver for vrx200
On 07/25/2018 06:12 PM, Andrew Lunn wrote:
>> LANTIQ MIPS ARCHITECTURE
>> M: John Crispin
>> diff --git a/drivers/net/dsa/Kconfig b/drivers/net/dsa/Kconfig
>> index 2b81b97e994f..f1280aa3f9bd 100644
>> --- a/drivers/net/dsa/Kconfig
>> +++ b/drivers/net/dsa/Kconfig
>> @@ -23,6 +23,14 @@ config NET_DSA_LOOP
>>This enables support for a fake mock-up switch chip which
>>exercises the DSA APIs.
>>
>> +config NET_DSA_GSWIP
>> +tristate "Intel / Lantiq GSWIP"
>
> Minor nit pick. Could you make this NET_DSA_LANTIQ_GSWIP. We generally
> have some manufacture ID in the name. And change the text to Lantiq /
> Intel GSWIP.
done
>> +static const struct gswip_rmon_cnt_desc gswip_rmon_cnt[] = {
>> +/** Receive Packet Count (only packets that are accepted and not
>> discarded). */
>> +MIB_DESC(1, 0x1F, "RxGoodPkts"),
>> +/** Receive Unicast Packet Count. */
>> +MIB_DESC(1, 0x23, "RxUnicastPkts"),
>> +/** Receive Multicast Packet Count. */
>> +MIB_DESC(1, 0x22, "RxMulticastPkts"),
>> +/** Receive FCS Error Packet Count. */
>> +MIB_DESC(1, 0x21, "RxFCSErrorPkts"),
>> +/** Receive Undersize Good Packet Count. */
>> +MIB_DESC(1, 0x1D, "RxUnderSizeGoodPkts"),
>> +/** Receive Undersize Error Packet Count. */
>> +MIB_DESC(1, 0x1E, "RxUnderSizeErrorPkts"),
>> +/** Receive Oversize Good Packet Count. */
>> +MIB_DESC(1, 0x1B, "RxOversizeGoodPkts"),
>> +/** Receive Oversize Error Packet Count. */
>> +MIB_DESC(1, 0x1C, "RxOversizeErrorPkts"),
>> +/** Receive Good Pause Packet Count. */
>> +MIB_DESC(1, 0x20, "RxGoodPausePkts"),
>> +/** Receive Align Error Packet Count. */
>> +MIB_DESC(1, 0x1A, "RxAlignErrorPkts"),
>> +/** Receive Size 64 Packet Count. */
>> +MIB_DESC(1, 0x12, "Rx64BytePkts"),
>> +/** Receive Size 65-127 Packet Count. */
>> +MIB_DESC(1, 0x13, "Rx127BytePkts"),
>> +/** Receive Size 128-255 Packet Count. */
>> +MIB_DESC(1, 0x14, "Rx255BytePkts"),
>> +/** Receive Size 256-511 Packet Count. */
>> +MIB_DESC(1, 0x15, "Rx511BytePkts"),
>> +/** Receive Size 512-1023 Packet Count. */
>> +MIB_DESC(1, 0x16, "Rx1023BytePkts"),
>> +/** Receive Size 1024-1522 (or more, if configured) Packet Count. */
>> +MIB_DESC(1, 0x17, "RxMaxBytePkts"),
>> +/** Receive Dropped Packet Count. */
>> +MIB_DESC(1, 0x18, "RxDroppedPkts"),
>> +/** Filtered Packet Count. */
>> +MIB_DESC(1, 0x19, "RxFilteredPkts"),
>> +/** Receive Good Byte Count (64 bit). */
>> +MIB_DESC(2, 0x24, "RxGoodBytes"),
>> +/** Receive Bad Byte Count (64 bit). */
>> +MIB_DESC(2, 0x26, "RxBadBytes"),
>> +/** Transmit Dropped Packet Count, based on Congestion Management. */
>> +MIB_DESC(1, 0x11, "TxAcmDroppedPkts"),
>> +/** Transmit Packet Count. */
>> +MIB_DESC(1, 0x0C, "TxGoodPkts"),
>> +/** Transmit Unicast Packet Count. */
>> +MIB_DESC(1, 0x06, "TxUnicastPkts"),
>> +/** Transmit Multicast Packet Count. */
>> +MIB_DESC(1, 0x07, "TxMulticastPkts"),
>> +/** Transmit Size 64 Packet Count. */
>> +MIB_DESC(1, 0x00, "Tx64BytePkts"),
>> +/** Transmit Size 65-127 Packet Count. */
>> +MIB_DESC(1, 0x01, "Tx127BytePkts"),
>> +/** Transmit Size 128-255 Packet Count. */
>> +MIB_DESC(1, 0x02, "Tx255BytePkts"),
>> +/** Transmit Size 256-511 Packet Count. */
>> +MIB_DESC(1, 0x03, "Tx511BytePkts"),
>> +/** Transmit Size 512-1023 Packet Count. */
>> +MIB_DESC(1, 0x04, "Tx1023BytePkts"),
>> +/** Transmit Size 1024-1522 (or more, if configured) Packet Count. */
>> +MIB_DESC(1, 0x05, "TxMaxBytePkts"),
>> +/** Transmit Single Collision Count. */
>> +MIB_DESC(1, 0x08, "TxSingleCollCount"),
>> +/** Transmit Multiple Collision Count. */
>> +MIB_DESC(1, 0x09, "TxMultCollCount"),
>> +/** Transmit Late Collision Count. */
>> +MIB_DESC(1, 0x0A, "TxLateCollCount"),
>> +/** Transmit Excessive Collision Count. */
>> +MIB_DESC(1, 0x0B, "TxExcessCollCount"),
>> +/** Transmit Pause Packet Count. */
>> +MIB_DESC(1, 0x0D, "TxPauseCount"),
>> +/** Transmit Drop Packet Count. */
>> +MIB_DESC(1, 0x10, "TxDroppedPkts"),
>> +/** Transmit Good Byte Count (64 bit). */
>> +MIB_DESC(2, 0x0E, "TxGoodBytes"),
>
> Most of the comments here don't add anything useful. Maybe remove
> them?
Ok I removed them. Are the names ok, or should they follow any Linux
definition?
>> +};
>> +
>> +static u32 gswip_switch_r(struct gswip_priv *priv, u32 offset)
>> +{
>> +return __raw_readl(priv->gswip + (offset * 4));
>> +}
>> +
>> +static void gswip_switch_w(struct gswip_priv *priv, u32 val, u32 offset)
>> +{
>> +return __raw_writel(val, priv->gswip + (offset * 4));
>> +}
>
> Since this is MIPS, i assume re-ordering cannot happen, there are
> barriers, etc?
As far as I know this is not a problem on this bus and no barriers are
needed here.
>> +static int xrx200_mdio_poll(struct gswip_priv *priv)
>> +{
>> +
Re: [PATCH 4/4] net: dsa: Add Lantiq / Intel DSA driver for vrx200
> > i extracted this struct/blob/voodoo from UGW3/4 7 years ago and was puzzled > by it. for those of us that have worked with this table in the past, its > semi understandable, yet its almost like a blackbox FW blob. can we try to > make it a little more readable by adding a comment on each line explaining > why its there and what it does ? Hi John I was wondering if there was any reverse engineered documentation. Do you have any links? Thanks Andrew
Re: [PATCH 4/4] net: dsa: Add Lantiq / Intel DSA driver for vrx200
On 21/07/18 21:13, Hauke Mehrtens wrote:
+#define MC_ENTRY(val, msk, ns, out, len, type, flags, ipv4_len) \
+ { val, msk, (ns << 10 | out << 4 | len >> 1),\
+ (len & 1) << 15 | type << 13 | flags << 9 | ipv4_len << 8 }
+static const struct gswip_pce_microcode gswip_pce_microcode[] = {
+ /* valuemaskns fields L type flags
ipv4_len */
+ MC_ENTRY(0x88c3, 0x, 1, OUT_ITAG0, 4, INSTR, FLAG_ITAG, 0),
+ MC_ENTRY(0x8100, 0x, 2, OUT_VTAG0, 2, INSTR, FLAG_VLAN, 0),
+ MC_ENTRY(0x88A8, 0x, 1, OUT_VTAG0, 2, INSTR, FLAG_VLAN, 0),
+ MC_ENTRY(0x8100, 0x, 1, OUT_VTAG0, 2, INSTR, FLAG_VLAN, 0),
+ MC_ENTRY(0x8864, 0x, 17, OUT_ETHTYP, 1, INSTR, FLAG_NO,0),
+ MC_ENTRY(0x0800, 0x, 21, OUT_ETHTYP, 1, INSTR, FLAG_NO,0),
+ MC_ENTRY(0x86DD, 0x, 22, OUT_ETHTYP, 1, INSTR, FLAG_NO,0),
+ MC_ENTRY(0x8863, 0x, 16, OUT_ETHTYP, 1, INSTR, FLAG_NO,0),
+ MC_ENTRY(0x, 0xF800, 10, OUT_NONE, 0, INSTR, FLAG_NO,0),
+ MC_ENTRY(0x, 0x, 40, OUT_ETHTYP, 1, INSTR, FLAG_NO,0),
+ MC_ENTRY(0x0600, 0x0600, 40, OUT_ETHTYP, 1, INSTR, FLAG_NO,0),
+ MC_ENTRY(0x, 0x, 12, OUT_NONE, 1, INSTR, FLAG_NO,0),
+ MC_ENTRY(0x, 0x, 14, OUT_NONE, 1, INSTR, FLAG_NO,0),
+ MC_ENTRY(0x, 0x, 41, OUT_NONE, 0, INSTR, FLAG_NO,0),
+ MC_ENTRY(0x0300, 0xFF00, 41, OUT_NONE, 0, INSTR, FLAG_SNAP, 0),
+ MC_ENTRY(0x, 0x, 41, OUT_NONE, 0, INSTR, FLAG_NO,0),
+ MC_ENTRY(0x, 0x, 41, OUT_DIP7, 3, INSTR, FLAG_NO,0),
+ MC_ENTRY(0x, 0x, 18, OUT_DIP7, 3, INSTR, FLAG_PPPOE, 0),
+ MC_ENTRY(0x0021, 0x, 21, OUT_NONE, 1, INSTR, FLAG_NO,0),
+ MC_ENTRY(0x0057, 0x, 22, OUT_NONE, 1, INSTR, FLAG_NO,0),
+ MC_ENTRY(0x, 0x, 40, OUT_NONE, 0, INSTR, FLAG_NO,0),
+ MC_ENTRY(0x4000, 0xF000, 24, OUT_IP0,4, INSTR, FLAG_IPV4, 1),
+ MC_ENTRY(0x6000, 0xF000, 27, OUT_IP0,3, INSTR, FLAG_IPV6, 0),
+ MC_ENTRY(0x, 0x, 41, OUT_NONE, 0, INSTR, FLAG_NO,0),
+ MC_ENTRY(0x, 0x, 25, OUT_IP3,2, INSTR, FLAG_NO,0),
+ MC_ENTRY(0x, 0x, 26, OUT_SIP0, 4, INSTR, FLAG_NO,0),
+ MC_ENTRY(0x, 0x, 40, OUT_NONE, 0, LENACCU, FLAG_NO,0),
+ MC_ENTRY(0x1100, 0xFF00, 39, OUT_PROT, 1, INSTR, FLAG_NO,0),
+ MC_ENTRY(0x0600, 0xFF00, 39, OUT_PROT, 1, INSTR, FLAG_NO,0),
+ MC_ENTRY(0x, 0xFF00, 33, OUT_IP3, 17, INSTR, FLAG_HOP, 0),
+ MC_ENTRY(0x2B00, 0xFF00, 33, OUT_IP3, 17, INSTR, FLAG_NN1, 0),
+ MC_ENTRY(0x3C00, 0xFF00, 33, OUT_IP3, 17, INSTR, FLAG_NN2, 0),
+ MC_ENTRY(0x, 0x, 39, OUT_PROT, 1, INSTR, FLAG_NO,0),
+ MC_ENTRY(0x, 0x00E0, 35, OUT_NONE, 0, INSTR, FLAG_NO,0),
+ MC_ENTRY(0x, 0x, 40, OUT_NONE, 0, INSTR, FLAG_NO,0),
+ MC_ENTRY(0x, 0xFF00, 33, OUT_NONE, 0, IPV6,FLAG_HOP, 0),
+ MC_ENTRY(0x2B00, 0xFF00, 33, OUT_NONE, 0, IPV6,FLAG_NN1, 0),
+ MC_ENTRY(0x3C00, 0xFF00, 33, OUT_NONE, 0, IPV6,FLAG_NN2, 0),
+ MC_ENTRY(0x, 0x, 40, OUT_PROT, 1, IPV6,FLAG_NO,0),
+ MC_ENTRY(0x, 0x, 40, OUT_SIP0, 16, INSTR, FLAG_NO,0),
+ MC_ENTRY(0x, 0x, 41, OUT_APP0, 4, INSTR, FLAG_IGMP, 0),
+ MC_ENTRY(0x, 0x, 41, OUT_NONE, 0, INSTR, FLAG_END, 0),
+ MC_ENTRY(0x, 0x, 41, OUT_NONE, 0, INSTR, FLAG_END, 0),
+ MC_ENTRY(0x, 0x, 41, OUT_NONE, 0, INSTR, FLAG_END, 0),
+ MC_ENTRY(0x, 0x, 41, OUT_NONE, 0, INSTR, FLAG_END, 0),
+ MC_ENTRY(0x, 0x, 41, OUT_NONE, 0, INSTR, FLAG_END, 0),
+ MC_ENTRY(0x, 0x, 41, OUT_NONE, 0, INSTR, FLAG_END, 0),
+ MC_ENTRY(0x, 0x, 41, OUT_NONE, 0, INSTR, FLAG_END, 0),
+ MC_ENTRY(0x, 0x, 41, OUT_NONE, 0, INSTR, FLAG_END, 0),
+ MC_ENTRY(0x, 0x, 41, OUT_NONE, 0, INSTR, FLAG_END, 0),
+ MC_ENTRY(0x, 0x, 41, OUT_NONE, 0, INSTR, FLAG_END, 0),
+ MC_ENTRY(0x, 0x, 41, OUT_NONE, 0, INSTR, FLAG_END, 0),
+ MC_ENTRY(0x, 0x, 41, OUT_NONE, 0, INSTR, FLAG_END, 0),
+ MC_ENTRY(0x, 0x, 41, OUT_NONE, 0, INSTR, FLAG_END, 0),
+ MC_ENTRY(0x, 0x, 41, OUT_NONE, 0, INSTR, FLAG_END, 0),
+ MC_ENTRY(0x, 0x, 41, OUT_NONE, 0, INSTR, FLAG_END, 0),
+ MC_ENTRY(0x, 0x, 41, OUT_NONE, 0, INSTR, FLAG_END, 0),
+ MC_ENTRY(0x, 0x, 41, OUT_NONE, 0, INSTR, FLAG_END, 0),
+ MC_ENTRY(0x, 0x, 41, OUT_NONE, 0, INSTR, FLAG_END, 0),
+ MC_ENTRY(0x, 0x, 41, OUT_NONE, 0, INSTR,
Re: [PATCH 4/4] net: dsa: Add Lantiq / Intel DSA driver for vrx200
> LANTIQ MIPS ARCHITECTURE
> M: John Crispin
> diff --git a/drivers/net/dsa/Kconfig b/drivers/net/dsa/Kconfig
> index 2b81b97e994f..f1280aa3f9bd 100644
> --- a/drivers/net/dsa/Kconfig
> +++ b/drivers/net/dsa/Kconfig
> @@ -23,6 +23,14 @@ config NET_DSA_LOOP
> This enables support for a fake mock-up switch chip which
> exercises the DSA APIs.
>
> +config NET_DSA_GSWIP
> + tristate "Intel / Lantiq GSWIP"
Minor nit pick. Could you make this NET_DSA_LANTIQ_GSWIP. We generally
have some manufacture ID in the name. And change the text to Lantiq /
Intel GSWIP.
> +static const struct gswip_rmon_cnt_desc gswip_rmon_cnt[] = {
> + /** Receive Packet Count (only packets that are accepted and not
> discarded). */
> + MIB_DESC(1, 0x1F, "RxGoodPkts"),
> + /** Receive Unicast Packet Count. */
> + MIB_DESC(1, 0x23, "RxUnicastPkts"),
> + /** Receive Multicast Packet Count. */
> + MIB_DESC(1, 0x22, "RxMulticastPkts"),
> + /** Receive FCS Error Packet Count. */
> + MIB_DESC(1, 0x21, "RxFCSErrorPkts"),
> + /** Receive Undersize Good Packet Count. */
> + MIB_DESC(1, 0x1D, "RxUnderSizeGoodPkts"),
> + /** Receive Undersize Error Packet Count. */
> + MIB_DESC(1, 0x1E, "RxUnderSizeErrorPkts"),
> + /** Receive Oversize Good Packet Count. */
> + MIB_DESC(1, 0x1B, "RxOversizeGoodPkts"),
> + /** Receive Oversize Error Packet Count. */
> + MIB_DESC(1, 0x1C, "RxOversizeErrorPkts"),
> + /** Receive Good Pause Packet Count. */
> + MIB_DESC(1, 0x20, "RxGoodPausePkts"),
> + /** Receive Align Error Packet Count. */
> + MIB_DESC(1, 0x1A, "RxAlignErrorPkts"),
> + /** Receive Size 64 Packet Count. */
> + MIB_DESC(1, 0x12, "Rx64BytePkts"),
> + /** Receive Size 65-127 Packet Count. */
> + MIB_DESC(1, 0x13, "Rx127BytePkts"),
> + /** Receive Size 128-255 Packet Count. */
> + MIB_DESC(1, 0x14, "Rx255BytePkts"),
> + /** Receive Size 256-511 Packet Count. */
> + MIB_DESC(1, 0x15, "Rx511BytePkts"),
> + /** Receive Size 512-1023 Packet Count. */
> + MIB_DESC(1, 0x16, "Rx1023BytePkts"),
> + /** Receive Size 1024-1522 (or more, if configured) Packet Count. */
> + MIB_DESC(1, 0x17, "RxMaxBytePkts"),
> + /** Receive Dropped Packet Count. */
> + MIB_DESC(1, 0x18, "RxDroppedPkts"),
> + /** Filtered Packet Count. */
> + MIB_DESC(1, 0x19, "RxFilteredPkts"),
> + /** Receive Good Byte Count (64 bit). */
> + MIB_DESC(2, 0x24, "RxGoodBytes"),
> + /** Receive Bad Byte Count (64 bit). */
> + MIB_DESC(2, 0x26, "RxBadBytes"),
> + /** Transmit Dropped Packet Count, based on Congestion Management. */
> + MIB_DESC(1, 0x11, "TxAcmDroppedPkts"),
> + /** Transmit Packet Count. */
> + MIB_DESC(1, 0x0C, "TxGoodPkts"),
> + /** Transmit Unicast Packet Count. */
> + MIB_DESC(1, 0x06, "TxUnicastPkts"),
> + /** Transmit Multicast Packet Count. */
> + MIB_DESC(1, 0x07, "TxMulticastPkts"),
> + /** Transmit Size 64 Packet Count. */
> + MIB_DESC(1, 0x00, "Tx64BytePkts"),
> + /** Transmit Size 65-127 Packet Count. */
> + MIB_DESC(1, 0x01, "Tx127BytePkts"),
> + /** Transmit Size 128-255 Packet Count. */
> + MIB_DESC(1, 0x02, "Tx255BytePkts"),
> + /** Transmit Size 256-511 Packet Count. */
> + MIB_DESC(1, 0x03, "Tx511BytePkts"),
> + /** Transmit Size 512-1023 Packet Count. */
> + MIB_DESC(1, 0x04, "Tx1023BytePkts"),
> + /** Transmit Size 1024-1522 (or more, if configured) Packet Count. */
> + MIB_DESC(1, 0x05, "TxMaxBytePkts"),
> + /** Transmit Single Collision Count. */
> + MIB_DESC(1, 0x08, "TxSingleCollCount"),
> + /** Transmit Multiple Collision Count. */
> + MIB_DESC(1, 0x09, "TxMultCollCount"),
> + /** Transmit Late Collision Count. */
> + MIB_DESC(1, 0x0A, "TxLateCollCount"),
> + /** Transmit Excessive Collision Count. */
> + MIB_DESC(1, 0x0B, "TxExcessCollCount"),
> + /** Transmit Pause Packet Count. */
> + MIB_DESC(1, 0x0D, "TxPauseCount"),
> + /** Transmit Drop Packet Count. */
> + MIB_DESC(1, 0x10, "TxDroppedPkts"),
> + /** Transmit Good Byte Count (64 bit). */
> + MIB_DESC(2, 0x0E, "TxGoodBytes"),
Most of the comments here don't add anything useful. Maybe remove
them?
> +};
> +
> +static u32 gswip_switch_r(struct gswip_priv *priv, u32 offset)
> +{
> + return __raw_readl(priv->gswip + (offset * 4));
> +}
> +
> +static void gswip_switch_w(struct gswip_priv *priv, u32 val, u32 offset)
> +{
> + return __raw_writel(val, priv->gswip + (offset * 4));
> +}
Since this is MIPS, i assume re-ordering cannot happen, there are
barriers, etc?
> +static int xrx200_mdio_poll(struct gswip_priv *priv)
> +{
> + int cnt = 1;
> +
> + while (likely(cnt--)) {
> + u32 ctrl = gswip_mdio_r(priv, GSWIP_MDIO_CTRL);
> +
> + if ((ctrl & GSWIP_MDIO_CTRL_BUSY) == 0)
> + return 0;
> + cpu_relax();
> + }
> +
>
Re: [PATCH 4/4] net: dsa: Add Lantiq / Intel DSA driver for vrx200
On 07/22/2018 05:17 AM, David Miller wrote: > From: Hauke Mehrtens > Date: Sat, 21 Jul 2018 21:13:58 +0200 > >> +// start the table access: > > Please stick to C-style comments, except perhaps in the SPDX > identifiers. > > Thank you. > Sorry that I missed that, it is fixed now. Hauke
Re: [PATCH 4/4] net: dsa: Add Lantiq / Intel DSA driver for vrx200
From: Hauke Mehrtens Date: Sat, 21 Jul 2018 21:13:58 +0200 > + // start the table access: Please stick to C-style comments, except perhaps in the SPDX identifiers. Thank you.
