Re: [PATCH net-next] r8169:add support for RTL8168EP

From: Francois Romieu
Date: Sat Sep 27 2014 - 16:39:49 EST

Next message: Nicholas Krause: "[PATCH] wireless: Remove unnessary write over of register 11 in rt2800lib.c"
Previous message: Bjorn Helgaas: "Re: [PATCH v12 08/12] PCI: OF: Introduce helper function for retrieving PCI domain numbers"
In reply to: Chun-Hao Lin: "[PATCH net-next] r8169:add support for RTL8168EP"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Chun-Hao Lin <hau@xxxxxxxxxxx> :
> RTL8168EP is Realtek PCIe Gigabit Ethernet controller.
> It is a successor chip of RTL8168DP.
>
> This patch add support for this chip.

It does more than that.

Did Hayes review it ?

> Signed-off-by: Chun-Hao Lin <hau@xxxxxxxxxxx>
> ---
> drivers/net/ethernet/realtek/r8169.c | 611 +++++++++++++++++++++++++++++------
> 1 file changed, 514 insertions(+), 97 deletions(-)
>
> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
> index 1d81238..0ead9a7 100644
> --- a/drivers/net/ethernet/realtek/r8169.c
> +++ b/drivers/net/ethernet/realtek/r8169.c
> @@ -155,6 +155,9 @@ enum mac_version {
> RTL_GIGA_MAC_VER_46,
> RTL_GIGA_MAC_VER_47,
> RTL_GIGA_MAC_VER_48,
> + RTL_GIGA_MAC_VER_49,
> + RTL_GIGA_MAC_VER_50,
> + RTL_GIGA_MAC_VER_51,
> RTL_GIGA_MAC_NONE = 0xff,
> };
>
> @@ -302,6 +305,15 @@ static const struct {
> [RTL_GIGA_MAC_VER_48] =
> _R("RTL8107e", RTL_TD_1, FIRMWARE_8107E_2,
> JUMBO_1K, false),
> + [RTL_GIGA_MAC_VER_49] =
> + _R("RTL8168ep/8111ep", RTL_TD_1, NULL,
> + JUMBO_9K, false),
> + [RTL_GIGA_MAC_VER_50] =
> + _R("RTL8168ep/8111ep", RTL_TD_1, NULL,
> + JUMBO_9K, false),
> + [RTL_GIGA_MAC_VER_51] =
> + _R("RTL8168ep/8111ep", RTL_TD_1, NULL,
> + JUMBO_9K, false),
> };
> #undef _R
>
> @@ -400,6 +412,10 @@ enum rtl_registers {
> FuncEvent = 0xf0,
> FuncEventMask = 0xf4,
> FuncPresetState = 0xf8,
> + IBCR0 = 0xf8,
> + IBCR2 = 0xf9,
> + IBIMR0 = 0xfa,
> + IBISR0 = 0xfb,
> FuncForceEvent = 0xfc,
> };
>
> @@ -467,6 +483,7 @@ enum rtl8168_registers {
> #define ERIAR_EXGMAC (0x00 << ERIAR_TYPE_SHIFT)
> #define ERIAR_MSIX (0x01 << ERIAR_TYPE_SHIFT)
> #define ERIAR_ASF (0x02 << ERIAR_TYPE_SHIFT)
> +#define ERIAR_OOB (0x02 << ERIAR_TYPE_SHIFT)
> #define ERIAR_MASK_SHIFT 12
> #define ERIAR_MASK_0001 (0x1 << ERIAR_MASK_SHIFT)
> #define ERIAR_MASK_0011 (0x3 << ERIAR_MASK_SHIFT)
> @@ -935,93 +952,10 @@ static const struct rtl_cond name = { \
> \
> static bool name ## _check(struct rtl8169_private *tp)
>
> -DECLARE_RTL_COND(rtl_ocpar_cond)
> -{
> - void __iomem *ioaddr = tp->mmio_addr;
> -
> - return RTL_R32(OCPAR) & OCPAR_FLAG;
> -}

Feel free to move this function around but please do it in a separate
patch. You are adding extra noise and thus making this stuff harder
to review than it should be.

> -
> -static u32 ocp_read(struct rtl8169_private *tp, u8 mask, u16 reg)
> -{
> - void __iomem *ioaddr = tp->mmio_addr;
> -
> - RTL_W32(OCPAR, ((u32)mask & 0x0f) << 12 | (reg & 0x0fff));
> -
> - return rtl_udelay_loop_wait_high(tp, &rtl_ocpar_cond, 100, 20) ?
> - RTL_R32(OCPDR) : ~0;
> -}

(this one is modified)

> -
> -static void ocp_write(struct rtl8169_private *tp, u8 mask, u16 reg, u32 data)
> -{
> - void __iomem *ioaddr = tp->mmio_addr;
> -
> - RTL_W32(OCPDR, data);
> - RTL_W32(OCPAR, OCPAR_FLAG | ((u32)mask & 0x0f) << 12 | (reg & 0x0fff));
> -
> - rtl_udelay_loop_wait_low(tp, &rtl_ocpar_cond, 100, 20);
> -}

(this one is modified)

> -
> -DECLARE_RTL_COND(rtl_eriar_cond)
> -{
> - void __iomem *ioaddr = tp->mmio_addr;
> -
> - return RTL_R32(ERIAR) & ERIAR_FLAG;
> -}

Feel free to move this function around but please do it in a separate
patch.

> -
> -static void rtl8168_oob_notify(struct rtl8169_private *tp, u8 cmd)
> -{
> - void __iomem *ioaddr = tp->mmio_addr;
> -
> - RTL_W8(ERIDR, cmd);
> - RTL_W32(ERIAR, 0x800010e8);
> - msleep(2);
> -
> - if (!rtl_udelay_loop_wait_low(tp, &rtl_eriar_cond, 100, 5))
> - return;
> -
> - ocp_write(tp, 0x1, 0x30, 0x00000001);
> -}

This one is modified but it is also modified for the existing 8168DP.

Mantra: please do it in a separate patch.

> -
> #define OOB_CMD_RESET 0x00
> #define OOB_CMD_DRIVER_START 0x05
> #define OOB_CMD_DRIVER_STOP 0x06
>
> -static u16 rtl8168_get_ocp_reg(struct rtl8169_private *tp)
> -{
> - return (tp->mac_version == RTL_GIGA_MAC_VER_31) ? 0xb8 : 0x10;
> -}
> -
> -DECLARE_RTL_COND(rtl_ocp_read_cond)
> -{
> - u16 reg;
> -
> - reg = rtl8168_get_ocp_reg(tp);
> -
> - return ocp_read(tp, 0x0f, reg) & 0x00000800;
> -}

(this one is simply moved around)

> -
> -static void rtl8168_driver_start(struct rtl8169_private *tp)
> -{
> - rtl8168_oob_notify(tp, OOB_CMD_DRIVER_START);
> -
> - rtl_msleep_loop_wait_high(tp, &rtl_ocp_read_cond, 10, 10);
> -}

(this one is modified)

> -
> -static void rtl8168_driver_stop(struct rtl8169_private *tp)
> -{
> - rtl8168_oob_notify(tp, OOB_CMD_DRIVER_STOP);
> -
> - rtl_msleep_loop_wait_low(tp, &rtl_ocp_read_cond, 10, 10);
> -}

(this one is modified)

> -
> -static int r8168dp_check_dash(struct rtl8169_private *tp)
> -{
> - u16 reg = rtl8168_get_ocp_reg(tp);
> -
> - return (ocp_read(tp, 0x0f, reg) & 0x00008000) ? 1 : 0;
> -}

(this one is modified)

[...]
> @@ -1329,6 +1277,45 @@ static void rtl_w1w0_eri(struct rtl8169_private *tp, int addr, u32 mask, u32 p,
> rtl_eri_write(tp, addr, mask, (val & ~m) | p, type);
> }
>
> +static u32 ocp_read(struct rtl8169_private *tp, u8 mask, u16 reg)
> +{
> + void __iomem *ioaddr = tp->mmio_addr;
> +
> + if (tp->mac_version == RTL_GIGA_MAC_VER_49 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_50 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_51) {
> + return rtl_eri_read(tp, reg, ERIAR_OOB);
> + } else if (tp->mac_version == RTL_GIGA_MAC_VER_27 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_28 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_31) {
> + RTL_W32(OCPAR, ((u32)mask & 0x0f) << 12 | (reg & 0x0fff));
> + return rtl_udelay_loop_wait_high(tp, &rtl_ocpar_cond, 100, 20)
> + ? RTL_R32(OCPDR) : ~0;

Please:
- use a local "bool rc" variable for rtl_udelay_loop_wait_high status
so that you can line things correctly.
- use a switch case
- croak in the default case

> + }
> +
> + return ~0;
> +}
> +
> +static void ocp_write(struct rtl8169_private *tp, u8 mask, u16 reg, u32 data)
> +{
> + void __iomem *ioaddr = tp->mmio_addr;
> +
> + if (tp->mac_version == RTL_GIGA_MAC_VER_49 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_50 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_51) {
> + rtl_eri_write(tp, reg, ((u32)mask & 0x0f) << ERIAR_MASK_SHIFT,
> + data, ERIAR_OOB);

It should line up after the opening parenthesis in "rtl_eri_write("

> + } else if (tp->mac_version == RTL_GIGA_MAC_VER_27 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_28 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_31) {
> + RTL_W32(OCPDR, data);
> + RTL_W32(OCPAR, OCPAR_FLAG | ((u32)mask & 0x0f) << 12 |
> + (reg & 0x0fff));

It should line up after the opening parenthesis in "RTL_W32("

Please:
- use a switch case
- croak in the default case

You may state in a comment that this function is only designed for 8168DP
and derivatives (whence the reduced switch case).

[...]
> @@ -1361,6 +1348,103 @@ static u8 rtl8168d_efuse_read(struct rtl8169_private *tp, int reg_addr)
[...]
> +static void rtl8168_driver_start(struct rtl8169_private *tp)
> +{
> + if (tp->mac_version == RTL_GIGA_MAC_VER_49 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_50 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_51) {
> + u32 tmp;
> +
> + ocp_write(tp, 0x01, 0x180, OOB_CMD_DRIVER_START);
> + tmp = ocp_read(tp, 0x01, 0x30);
> + tmp |= 0x01;
> + ocp_write(tp, 0x01, 0x30, tmp);
> + rtl_msleep_loop_wait_high(tp, &rtl_ep_ocp_read_cond, 10, 10);
> + } else if (tp->mac_version == RTL_GIGA_MAC_VER_27 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_28 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_31) {
> + rtl8168_oob_notify(tp, OOB_CMD_DRIVER_START);
> + rtl_msleep_loop_wait_high(tp, &rtl_ocp_read_cond, 10, 10);

switch case + croak on default please.

[snip]
> @@ -3724,6 +3819,143 @@ static void rtl8168h_2_hw_phy_config(struct rtl8169_private *tp)
> rtl_writephy(tp, 0x1f, 0x0000);
> }
>
> +static void rtl8168ep_1_hw_phy_config(struct rtl8169_private *tp)
> +{
> + rtl_apply_firmware(tp);

?

You did not specify any firmware information in rtl_chip_infos.

It some firmware is supposed to help, please be sure to add the relevant
#define FIRMWARE_8xyz as well.

[...]
> +static void rtl8168ep_2_hw_phy_config(struct rtl8169_private *tp)
> +{
> + rtl_apply_firmware(tp);

See above.

[...]
> @@ -4265,7 +4511,7 @@ static void r810x_pll_power_up(struct rtl8169_private *tp)
> break;
> case RTL_GIGA_MAC_VER_47:
> case RTL_GIGA_MAC_VER_48:
> - RTL_W8(PMCH, RTL_R8(PMCH) | 0xC0);
> + RTL_W8(PMCH, RTL_R8(PMCH) | 0xc0);
> break;

It's welcome but it should be done in a different patch.

[...]
> @@ -4339,8 +4585,11 @@ static void r8168_pll_power_down(struct rtl8169_private *tp)
>
> if ((tp->mac_version == RTL_GIGA_MAC_VER_27 ||
> tp->mac_version == RTL_GIGA_MAC_VER_28 ||
> - tp->mac_version == RTL_GIGA_MAC_VER_31) &&
> - r8168dp_check_dash(tp)) {
> + tp->mac_version == RTL_GIGA_MAC_VER_31 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_49 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_50 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_51) &&
> + r8168_check_dash(tp)) {
> return;

Unrelated behavior change for RTL_GIGA_MAC_VER_2[78]. Different patch.

> }
>
> @@ -4369,12 +4618,16 @@ static void r8168_pll_power_down(struct rtl8169_private *tp)
> case RTL_GIGA_MAC_VER_33:
> case RTL_GIGA_MAC_VER_45:
> case RTL_GIGA_MAC_VER_46:
> + case RTL_GIGA_MAC_VER_50:
> + case RTL_GIGA_MAC_VER_51:
> RTL_W8(PMCH, RTL_R8(PMCH) & ~0x80);
> break;
> case RTL_GIGA_MAC_VER_40:
> case RTL_GIGA_MAC_VER_41:
> + case RTL_GIGA_MAC_VER_49:
> rtl_w1w0_eri(tp, 0x1a8, ERIAR_MASK_1111, 0x00000000,
> 0xfc000000, ERIAR_EXGMAC);
> + RTL_W8(PMCH, RTL_R8(PMCH) & ~0x80);

Unrelated behavior change for RTL_GIGA_MAC_VER_4[01] (8168g, huh ?).
-> Different patch.

[...]
> @@ -4395,10 +4648,14 @@ static void r8168_pll_power_up(struct rtl8169_private *tp)
> break;
> case RTL_GIGA_MAC_VER_45:
> case RTL_GIGA_MAC_VER_46:
> - RTL_W8(PMCH, RTL_R8(PMCH) | 0xC0);
> + case RTL_GIGA_MAC_VER_50:
> + case RTL_GIGA_MAC_VER_51:
> + RTL_W8(PMCH, RTL_R8(PMCH) | 0xc0);

See above.

> break;
> case RTL_GIGA_MAC_VER_40:
> case RTL_GIGA_MAC_VER_41:
> + case RTL_GIGA_MAC_VER_49:
> + RTL_W8(PMCH, RTL_R8(PMCH) | 0xc0);
> rtl_w1w0_eri(tp, 0x1a8, ERIAR_MASK_1111, 0xfc000000,
> 0x00000000, ERIAR_EXGMAC);

See above.

[...]
> @@ -7366,9 +7766,13 @@ static void rtl_remove_one(struct pci_dev *pdev)
> struct net_device *dev = pci_get_drvdata(pdev);
> struct rtl8169_private *tp = netdev_priv(dev);
>
> - if (tp->mac_version == RTL_GIGA_MAC_VER_27 ||
> - tp->mac_version == RTL_GIGA_MAC_VER_28 ||
> - tp->mac_version == RTL_GIGA_MAC_VER_31) {
> + if ((tp->mac_version == RTL_GIGA_MAC_VER_27 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_28 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_31 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_49 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_50 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_51) &&
> + r8168_check_dash(tp)) {

It does not line up correctly (one space excess).

Unrelated behavior change for RTL_GIGA_MAC_VER_{27, 28, 31}. Different patch.

> rtl8168_driver_stop(tp);
> }
>
[...]
> @@ -7703,11 +8113,14 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
> if (tp->mac_version == RTL_GIGA_MAC_VER_45 ||
> tp->mac_version == RTL_GIGA_MAC_VER_46 ||
> tp->mac_version == RTL_GIGA_MAC_VER_47 ||
> - tp->mac_version == RTL_GIGA_MAC_VER_48) {
> + tp->mac_version == RTL_GIGA_MAC_VER_48 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_49 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_50 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_51) {
> u16 mac_addr[3];
>
> - *(u32 *)&mac_addr[0] = rtl_eri_read(tp, 0xE0, ERIAR_EXGMAC);
> - *(u16 *)&mac_addr[2] = rtl_eri_read(tp, 0xE4, ERIAR_EXGMAC);
> + *(u32 *)&mac_addr[0] = rtl_eri_read(tp, 0xe0, ERIAR_EXGMAC);
> + *(u16 *)&mac_addr[2] = rtl_eri_read(tp, 0xe4, ERIAR_EXGMAC);

It's welcome but it brings noise in the current patch. Different patch.

>
> if (is_valid_ether_addr((u8 *)mac_addr))
> rtl_rar_set(tp, (u8 *)mac_addr);
> @@ -7780,9 +8193,13 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
> rtl_chip_infos[chipset].jumbo_tx_csum ? "ok" : "ko");
> }
>
> - if (tp->mac_version == RTL_GIGA_MAC_VER_27 ||
> - tp->mac_version == RTL_GIGA_MAC_VER_28 ||
> - tp->mac_version == RTL_GIGA_MAC_VER_31) {
> + if ((tp->mac_version == RTL_GIGA_MAC_VER_27 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_28 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_31 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_49 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_50 ||
> + tp->mac_version == RTL_GIGA_MAC_VER_51) &&
> + r8168_check_dash(tp)) {

See above. Unrelated change, etc.

> rtl8168_driver_start(tp);
> }

--
Ueimor
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Nicholas Krause: "[PATCH] wireless: Remove unnessary write over of register 11 in rt2800lib.c"
Previous message: Bjorn Helgaas: "Re: [PATCH v12 08/12] PCI: OF: Introduce helper function for retrieving PCI domain numbers"
In reply to: Chun-Hao Lin: "[PATCH net-next] r8169:add support for RTL8168EP"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]