RE: [PATCH net-next v3 7/8] r8152: support rtl_fc_pause_pkt_en()

[Chih Kai Hsu]

> -----Original Message-----
> From: netdev-bot+sashiko@xxxxxxxxxx <netdev-bot+sashiko@xxxxxxxxxx>
> Sent: Sunday, September 13, 2026 4:20 AM
> To: Chih Kai Hsu <hsu.chih.kai@xxxxxxxxxxx>
> Cc: davem@xxxxxxxxxxxxx; kuba@xxxxxxxxxx; netdev@xxxxxxxxxxxxxxx;
> nic_swsd <nic_swsd@xxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx;
> linux-usb@xxxxxxxxxxxxxxx; edumazet@xxxxxxxxxx; bjorn@xxxxxxx;
> pabeni@xxxxxxxxxx; andrew+netdev@xxxxxxx
> Subject: Re: [PATCH net-next v3 7/8] r8152: support rtl_fc_pause_pkt_en()
> 
> 
> External mail : This email originated from outside the organization. Do not
> reply, click links, or open attachments unless you recognize the sender and
> know the content is safe.
> 
> 
> 
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
> 
> Critical: 0 · High: 0 · Medium: 1 · Low: 0
> 
> - [Medium] Commit message mischaracterizes the change as an extraction
>   and omits the new register programming it introduces.
> 
> --
> 
> r8152: support rtl_fc_pause_pkt_en()
> 
> This moves the flow control programming out of r8156b_init() into a new
> helper rtl_fc_pause_pkt_en(), and calls it from r8156_init(), r8156b_init() and
> rtl8157_enable(). The helper switches on tp->version and, for RTL8157 and
> RTL8159, programs a new pause limit register based on the link speed.
> 
> > diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c index
> > 1fcb1cc5b4a18..2b0972b967385 100644
> > --- a/drivers/net/usb/r8152.c
> > +++ b/drivers/net/usb/r8152.c
> > @@ -89,6 +89,7 @@
> >  #define PLA_MTPS             0xe615
> >  #define PLA_TXFIFO_CTRL              0xe618
> >  #define PLA_TXFIFO_FULL              0xe61a
> > +#define PLA_PAUSE_LIMIT         0xe61e
> >  #define PLA_RSTTALLY         0xe800
> >  #define PLA_CR                       0xe813
> >  #define PLA_CRWECR           0xe81c
> > @@ -301,6 +302,10 @@
> >  #define MTPS_JUMBO           (12 * 1024 / 64)
> >  #define MTPS_DEFAULT         (6 * 1024 / 64)
> >
> > +/* PLA_PAUSE_LIMIT */
> > +#define PAUSE_LIMIT_EN               BIT(3)
> > +#define PAUSE_LIMIT_MASK     0xf0
> > +
> >  /* PLA_RSTTALLY */
> >  #define TALLY_RESET          0x0001
> >
> > @@ -6088,6 +6093,93 @@ static void r8152b_enter_oob(struct r8152 *tp)
> >                          RCR_APM | RCR_AM | RCR_AB);  }
> >
> > +static void rtl_fc_pause_pkt_en(struct r8152 *tp, u16 speed) {
> > +     int log2_ratio, ratio;
> > +     u16 num_pause_pkts;
> > +     u32 ocp_data;
> > +
> > +     switch (tp->version) {
> > +     case RTL_VER_10:
> > +     case RTL_VER_11:
> > +             ocp_write_word(tp, MCU_TYPE_USB, USB_FC_TIMER,
> > +                            CTRL_TIMER_EN | (1000 / 8));
> > +
> > +             ocp_word_set_bits(tp, MCU_TYPE_USB, USB_FW_CTRL,
> > +                               FLOW_CTRL_PATCH_OPT);
> > +
> > +             ocp_word_set_bits(tp, MCU_TYPE_USB, USB_FW_TASK,
> FC_PATCH_TASK);
> > +             break;
> > +     case RTL_VER_12:
> > +     case RTL_VER_13:
> > +     case RTL_VER_15:
> > +             ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_RCR, SLOT_EN);
> > +
> > +             ocp_word_set_bits(tp, MCU_TYPE_PLA, PLA_CPCR,
> > + FLOW_CTRL_EN);
> > +
> > +             /* enable fc timer and set timer to 600 ms. */
> > +             ocp_write_word(tp, MCU_TYPE_USB, USB_FC_TIMER,
> > +                            CTRL_TIMER_EN | (600 / 8));
> > +
> > +             ocp_data = ocp_read_word(tp, MCU_TYPE_PLA,
> PLA_POL_GPIO_CTRL);
> > +             if (!(ocp_data & DACK_DET_EN))
> > +                     ocp_word_set_bits(tp, MCU_TYPE_USB,
> USB_FW_CTRL,
> > +                                       FLOW_CTRL_PATCH_2);
> > +
> > +             ocp_word_set_bits(tp, MCU_TYPE_USB, USB_FW_TASK,
> FC_PATCH_TASK);
> > +             break;
> > +     case RTL_VER_16:
> > +     case RTL_VER_17_QFN68:
> > +     case RTL_VER_17_QFN100:
> > +             ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_RCR, SLOT_EN);
> > +
> > +             num_pause_pkts = 0xa;
> > +             ratio = 10000;
> > +
> > +             if (!(speed & LINK_STATUS)) {
> > +                     dev_dbg(&tp->intf->dev, "No link\n");
> > +                     goto no_link;
> > +             } else if (speed & _10bps) {
> > +                     ratio /= 10;
> > +             } else if (speed & _100bps) {
> > +                     ratio /= 100;
> > +             } else if (speed & _1000bps) {
> > +                     ratio /= 1000;
> > +             } else if (speed & _2500bps) {
> > +                     ratio /= 2500;
> > +             } else if (speed & _5000bps) {
> > +                     ratio /= 5000;
> > +             } else if (speed & _10000bps) {
> > +                     ratio /= 10000;
> > +             } else {
> > +                     dev_err(&tp->intf->dev, "Unknown link speed\n");
> > +                     goto no_link;
> > +             }
> > +
> > +             log2_ratio = ilog2(ratio);
> > +             num_pause_pkts -= log2_ratio;
> > +
> > +             /* Round up if ratio is more than halfway to the next power
> of 2.
> > +              * Floating-point is avoided by rewriting
> > +              * ratio > 1.5 * 2^log2_ratio as
> > +              * 2 * ratio > 3 * 2^log2_ratio
> > +              */
> > +             if (2 * ratio > 3 * (1 << log2_ratio))
> > +                     num_pause_pkts--;
> > +
> > +no_link:
> > +             ocp_word_w0w1(tp, MCU_TYPE_PLA, PLA_PAUSE_LIMIT,
> > +                           PAUSE_LIMIT_MASK | PAUSE_LIMIT_EN,
> > +                           num_pause_pkts << 4);
> > +
> > +             ocp_word_set_bits(tp, MCU_TYPE_PLA, PLA_PAUSE_LIMIT,
> > +                               PAUSE_LIMIT_EN);
> > +             break;
> > +     default:
> > +             break;
> > +     }
> > +}
> > +
> 
> [Severity: Medium]
> Is the changelog description of "Extract it as rtl_fc_pause_pkt_en()"
> accurate for the RTL_VER_10/RTL_VER_11 and RTL_VER_16/RTL_VER_17 arms
> of rtl_fc_pause_pkt_en()?  The three arms are not the same sequence that
> was inline in r8156b_init().

I will fix it.

> 
> For RTL_VER_10/RTL_VER_11 the helper programs:
> 
>         ocp_write_word(tp, MCU_TYPE_USB, USB_FC_TIMER,
>                        CTRL_TIMER_EN | (1000 / 8));
>         ocp_word_set_bits(tp, MCU_TYPE_USB, USB_FW_CTRL,
>                           FLOW_CTRL_PATCH_OPT);
>         ocp_word_set_bits(tp, MCU_TYPE_USB, USB_FW_TASK,
> FC_PATCH_TASK);
> 
> At the series baseline the flow control block in r8156_init() was gated as:
> 
>         if (tp->version >= RTL_VER_12 && tp->version <= RTL_VER_15) {
>                 ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_RCR,
> SLOT_EN);
>                 ocp_word_set_bits(tp, MCU_TYPE_PLA, PLA_CPCR,
> FLOW_CTRL_EN);
>                 ...
>         }
> 
> so RTL_VER_10/RTL_VER_11 received no flow control programming, and
> FLOW_CTRL_PATCH_OPT was only set from the RTL8153B firmware path.
> Could the changelog say why RTL8156A now gets a 1000 ms timer plus
> FLOW_CTRL_PATCH_OPT, while RTL8156B keeps the 600 ms timer,
> FLOW_CTRL_PATCH_2 and PLA_CPCR FLOW_CTRL_EN?

VER_10 and VER_11 have the same way to enable flow control patch as RTL8153B, so
follow the settings that RTL8153B applies.

> 
> For RTL_VER_16/RTL_VER_17_QFN68/RTL_VER_17_QFN100 the helper adds a
> new register, PLA_PAUSE_LIMIT (0xe61e), the new
> PAUSE_LIMIT_EN/PAUSE_LIMIT_MASK bits, and a link-speed dependent pause
> packet count, but it sets neither PLA_CPCR FLOW_CTRL_EN nor
> USB_FC_TIMER even though the changelog says the flow control patch was
> "missing for ... RTL8157/8159".  None of the new register or the pause budget
> calculation is described in the changelog.
> 
> Could the changelog describe the observed malfunction, the link speeds and
> traffic conditions it was seen under, and what was tested, so the behaviour
> change on already supported RTL8156A hardware and the new pause limit
> programming can be judged?

The formula computes the number of pause packets to be sent. The datasheet gives
a fixed count per link speed via the new PLA_PAUSE_LIMIT reg. The formula reproduces
the table.  

> 
> >  static int r8153_pre_firmware_1(struct r8152 *tp)  {
> >       int i;
> > @@ -6619,6 +6711,8 @@ static int rtl8157_enable(struct r8152 *tp)
> >       r8153_set_rx_early_size(tp);
> >
> >       speed = rtl8152_get_speed(tp);
> > +     rtl_fc_pause_pkt_en(tp, speed);
> > +
> >       rtl_set_ifg(tp, speed);
> >
> >       return rtl_enable(tp);
> > @@ -8751,6 +8845,8 @@ static void r8156_init(struct r8152 *tp)
> >
> >       usb_enable_lpm(tp->udev);
> >
> > +     rtl_fc_pause_pkt_en(tp, 0);
> > +
> >       r8156_mac_clk_spd(tp, true);
> >
> 
> [ ... ]
> 
> > @@ -8852,20 +8947,7 @@ static void r8156b_init(struct r8152 *tp)
> >
> >       usb_enable_lpm(tp->udev);
> >
> > -     ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_RCR, SLOT_EN);
> > -
> > -     ocp_word_set_bits(tp, MCU_TYPE_PLA, PLA_CPCR, FLOW_CTRL_EN);
> > -
> > -     /* enable fc timer and set timer to 600 ms. */
> > -     ocp_write_word(tp, MCU_TYPE_USB, USB_FC_TIMER,
> > -                    CTRL_TIMER_EN | (600 / 8));
> > -
> > -     ocp_data = ocp_read_word(tp, MCU_TYPE_PLA,
> PLA_POL_GPIO_CTRL);
> > -     if (!(ocp_data & DACK_DET_EN))
> > -             ocp_word_set_bits(tp, MCU_TYPE_USB, USB_FW_CTRL,
> > -                               FLOW_CTRL_PATCH_2);
> > -
> > -     ocp_word_set_bits(tp, MCU_TYPE_USB, USB_FW_TASK,
> FC_PATCH_TASK);
> > +     rtl_fc_pause_pkt_en(tp, 0);
> >
> >       r8156_mac_clk_spd(tp, true);
> >
> 
> --
> Sashiko AI review ·
> https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260908075634.23740-1
> -nic_swsd%40realtek.com

Best,
Chih-Kai