Re: [PATCH net-next 4/6] r8152: support new chips

2021-04-20 Thread Jakub Kicinski 
On Tue, 20 Apr 2021 07:00:39 + Hayes Wang wrote:
> > > @@ -6878,7 +8942,11 @@ static int rtl8152_probe(struct usb_interface 
> > > *intf,  
> > >   set_ethernet_addr(tp);
> > >
> > >   usb_set_intfdata(intf, tp);
> > > - netif_napi_add(netdev, &tp->napi, r8152_poll, RTL8152_NAPI_WEIGHT);
> > > +
> > > + if (tp->support_2500full)
> > > + netif_napi_add(netdev, &tp->napi, r8152_poll, 256);  
> > 
> > why 256? We have 100G+ drivers all using 64 what's special here?
> >   
> > > + else
> > > + netif_napi_add(netdev, &tp->napi, r8152_poll, 64);  
> 
> We test 2.5G Ethernet on some embedded platform.
> And we find 64 is not large enough, and the performance
> couldn't reach 2.5 G bits/s.

Did you manage to identify what the cause is?

NAPI will keep calling your driver if the budget was exhausted, the
only difference between 64 and 256 should be the setup cost of the
driver's internal loop. And perhaps more frequent GRO flush - what's
the CONFIG_HZ set to?

RE: [PATCH net-next 4/6] r8152: support new chips

2021-04-20 Thread Hayes Wang 
Jakub Kicinski 
> Sent: Saturday, April 17, 2021 5:50 AM
> > +   switch (tp->version) {
> > +   case RTL_VER_10:
> > +   data = ocp_reg_read(tp, 0xad40);
> > +   data &= ~0x3ff;
> > +   data |= BIT(7) | BIT(2);
> > +   ocp_reg_write(tp, 0xad40, data);
> > +
> > +   data = ocp_reg_read(tp, 0xad4e);
> > +   data |= BIT(4);
> > +   ocp_reg_write(tp, 0xad4e, data);
> > +   data = ocp_reg_read(tp, 0xad16);
> > +   data &= ~0x3ff;
> > +   data |= 0x6;
> > +   ocp_reg_write(tp, 0xad16, data);
> > +   data = ocp_reg_read(tp, 0xad32);
> > +   data &= ~0x3f;
> > +   data |= 6;
> > +   ocp_reg_write(tp, 0xad32, data);
> > +   data = ocp_reg_read(tp, 0xac08);
> > +   data &= ~(BIT(12) | BIT(8));
> > +   ocp_reg_write(tp, 0xac08, data);
> > +   data = ocp_reg_read(tp, 0xac8a);
> > +   data |= BIT(12) | BIT(13) | BIT(14);
> > +   data &= ~BIT(15);
> > +   ocp_reg_write(tp, 0xac8a, data);
> > +   data = ocp_reg_read(tp, 0xad18);
> > +   data |= BIT(10);
> > +   ocp_reg_write(tp, 0xad18, data);
> > +   data = ocp_reg_read(tp, 0xad1a);
> > +   data |= 0x3ff;
> > +   ocp_reg_write(tp, 0xad1a, data);
> > +   data = ocp_reg_read(tp, 0xad1c);
> > +   data |= 0x3ff;
> > +   ocp_reg_write(tp, 0xad1c, data);
> > +
> > +   data = sram_read(tp, 0x80ea);
> > +   data &= ~0xff00;
> > +   data |= 0xc400;
> > +   sram_write(tp, 0x80ea, data);
> > +   data = sram_read(tp, 0x80eb);
> > +   data &= ~0x0700;
> > +   data |= 0x0300;
> > +   sram_write(tp, 0x80eb, data);
> > +   data = sram_read(tp, 0x80f8);
> > +   data &= ~0xff00;
> > +   data |= 0x1c00;
> > +   sram_write(tp, 0x80f8, data);
> > +   data = sram_read(tp, 0x80f1);
> > +   data &= ~0xff00;
> > +   data |= 0x3000;
> > +   sram_write(tp, 0x80f1, data);

These are the parameters of PHY.
Some are used for speed down about power saving.
And some are used for performance.

> > +   switch (tp->version) {
> > +   case RTL_VER_12:
> > +   ocp_reg_write(tp, 0xbf86, 0x9000);
> > +   data = ocp_reg_read(tp, 0xc402);
> > +   data |= BIT(10);
> > +   ocp_reg_write(tp, 0xc402, data);
> > +   data &= ~BIT(10);
> > +   ocp_reg_write(tp, 0xc402, data);
> > +   ocp_reg_write(tp, 0xbd86, 0x1010);
> > +   ocp_reg_write(tp, 0xbd88, 0x1010);
> > +   data = ocp_reg_read(tp, 0xbd4e);
> > +   data &= ~(BIT(10) | BIT(11));
> > +   data |= BIT(11);
> > +   ocp_reg_write(tp, 0xbd4e, data);
> > +   data = ocp_reg_read(tp, 0xbf46);
> > +   data &= ~0xf00;
> > +   data |= 0x700;
> > +   ocp_reg_write(tp, 0xbf46, data);

These are used to adjust the clock of GPHY.
It influences the linking.

> > +   data = r8153_phy_status(tp, 0);
> > +   switch (data) {
> > +   case PHY_STAT_EXT_INIT:
> > +   rtl8152_apply_firmware(tp, true);
> > +
> > +   data = ocp_reg_read(tp, 0xa466);
> > +   data &= ~BIT(0);
> > +   ocp_reg_write(tp, 0xa466, data);

These let the PHY exit PHY_STAT_EXT_INIT state.

> What are all these magic constants? :(

I think it is difficult for me to make all magic values meaningful.
The PHY setting is very complex. Only PHY engineers know
what are the settings mean.

> > @@ -6878,7 +8942,11 @@ static int rtl8152_probe(struct usb_interface
> *intf,
> > set_ethernet_addr(tp);
> >
> > usb_set_intfdata(intf, tp);
> > -   netif_napi_add(netdev, &tp->napi, r8152_poll, RTL8152_NAPI_WEIGHT);
> > +
> > +   if (tp->support_2500full)
> > +   netif_napi_add(netdev, &tp->napi, r8152_poll, 256);
> 
> why 256? We have 100G+ drivers all using 64 what's special here?
> 
> > +   else
> > +   netif_napi_add(netdev, &tp->napi, r8152_poll, 64);

We test 2.5G Ethernet on some embedded platform.
And we find 64 is not large enough, and the performance
couldn't reach 2.5 G bits/s.

Best Regards,
Hayes

Re: [PATCH net-next 4/6] r8152: support new chips

2021-04-16 Thread Jakub Kicinski 
On Fri, 16 Apr 2021 16:04:35 +0800 Hayes Wang wrote:
> Support RTL8153C, RTL8153D, RTL8156A, and RTL8156B. The RTL8156A
> and RTL8156B are the 2.5G ethernet.
> 
> Signed-off-by: Hayes Wang 

> + switch (tp->version) {
> + case RTL_VER_10:
> + data = ocp_reg_read(tp, 0xad40);
> + data &= ~0x3ff;
> + data |= BIT(7) | BIT(2);
> + ocp_reg_write(tp, 0xad40, data);
> +
> + data = ocp_reg_read(tp, 0xad4e);
> + data |= BIT(4);
> + ocp_reg_write(tp, 0xad4e, data);
> + data = ocp_reg_read(tp, 0xad16);
> + data &= ~0x3ff;
> + data |= 0x6;
> + ocp_reg_write(tp, 0xad16, data);
> + data = ocp_reg_read(tp, 0xad32);
> + data &= ~0x3f;
> + data |= 6;
> + ocp_reg_write(tp, 0xad32, data);
> + data = ocp_reg_read(tp, 0xac08);
> + data &= ~(BIT(12) | BIT(8));
> + ocp_reg_write(tp, 0xac08, data);
> + data = ocp_reg_read(tp, 0xac8a);
> + data |= BIT(12) | BIT(13) | BIT(14);
> + data &= ~BIT(15);
> + ocp_reg_write(tp, 0xac8a, data);
> + data = ocp_reg_read(tp, 0xad18);
> + data |= BIT(10);
> + ocp_reg_write(tp, 0xad18, data);
> + data = ocp_reg_read(tp, 0xad1a);
> + data |= 0x3ff;
> + ocp_reg_write(tp, 0xad1a, data);
> + data = ocp_reg_read(tp, 0xad1c);
> + data |= 0x3ff;
> + ocp_reg_write(tp, 0xad1c, data);
> +
> + data = sram_read(tp, 0x80ea);
> + data &= ~0xff00;
> + data |= 0xc400;
> + sram_write(tp, 0x80ea, data);
> + data = sram_read(tp, 0x80eb);
> + data &= ~0x0700;
> + data |= 0x0300;
> + sram_write(tp, 0x80eb, data);
> + data = sram_read(tp, 0x80f8);
> + data &= ~0xff00;
> + data |= 0x1c00;
> + sram_write(tp, 0x80f8, data);
> + data = sram_read(tp, 0x80f1);
> + data &= ~0xff00;
> + data |= 0x3000;
> + sram_write(tp, 0x80f1, data);

> + switch (tp->version) {
> + case RTL_VER_12:
> + ocp_reg_write(tp, 0xbf86, 0x9000);
> + data = ocp_reg_read(tp, 0xc402);
> + data |= BIT(10);
> + ocp_reg_write(tp, 0xc402, data);
> + data &= ~BIT(10);
> + ocp_reg_write(tp, 0xc402, data);
> + ocp_reg_write(tp, 0xbd86, 0x1010);
> + ocp_reg_write(tp, 0xbd88, 0x1010);
> + data = ocp_reg_read(tp, 0xbd4e);
> + data &= ~(BIT(10) | BIT(11));
> + data |= BIT(11);
> + ocp_reg_write(tp, 0xbd4e, data);
> + data = ocp_reg_read(tp, 0xbf46);
> + data &= ~0xf00;
> + data |= 0x700;
> + ocp_reg_write(tp, 0xbf46, data);

> + data = r8153_phy_status(tp, 0);
> + switch (data) {
> + case PHY_STAT_EXT_INIT:
> + rtl8152_apply_firmware(tp, true);
> +
> + data = ocp_reg_read(tp, 0xa466);
> + data &= ~BIT(0);
> + ocp_reg_write(tp, 0xa466, data);

What are all these magic constants? :(

> @@ -6878,7 +8942,11 @@ static int rtl8152_probe(struct usb_interface *intf,
>   set_ethernet_addr(tp);
>  
>   usb_set_intfdata(intf, tp);
> - netif_napi_add(netdev, &tp->napi, r8152_poll, RTL8152_NAPI_WEIGHT);
> +
> + if (tp->support_2500full)
> + netif_napi_add(netdev, &tp->napi, r8152_poll, 256);

why 256? We have 100G+ drivers all using 64 what's special here?

> + else
> + netif_napi_add(netdev, &tp->napi, r8152_poll, 64);