| Message ID | 20240522140817.409936-1-Parthiban.Veerasooran@microchip.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | net: usb: smsc95xx: configure external LEDs function for EVB-LAN8670-USB | expand |
On Wed, May 22, 2024 at 07:38:17PM +0530, Parthiban Veerasooran wrote: > By default, LAN9500A configures the external LEDs to the below function. > nSPD_LED -> Speed Indicator > nLNKA_LED -> Link and Activity Indicator > nFDX_LED -> Full Duplex Link Indicator > > But, EVB-LAN8670-USB uses the below external LEDs function which can be > enabled by writing 1 to the LED Select (LED_SEL) bit in the LAN9500A. > nSPD_LED -> Speed Indicator > nLNKA_LED -> Link Indicator > nFDX_LED -> Activity Indicator What else can the LEDs indicate? > + /* Set LED Select (LED_SEL) bit for the external LED pins functionality > + * in the Microchip's EVB-LAN8670-USB 10BASE-T1S Ethernet device which Is this a function of the USB dongle? Or a function of the PHY? Andrew
On Wed, May 22, 2024 at 07:38:17PM +0530, Parthiban Veerasooran wrote: > By default, LAN9500A configures the external LEDs to the below function. > nSPD_LED -> Speed Indicator > nLNKA_LED -> Link and Activity Indicator > nFDX_LED -> Full Duplex Link Indicator > > But, EVB-LAN8670-USB uses the below external LEDs function which can be > enabled by writing 1 to the LED Select (LED_SEL) bit in the LAN9500A. > nSPD_LED -> Speed Indicator > nLNKA_LED -> Link Indicator > nFDX_LED -> Activity Indicator > > Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com> > --- > drivers/net/usb/smsc95xx.c | 12 ++++++++++++ > drivers/net/usb/smsc95xx.h | 1 + > 2 files changed, 13 insertions(+) > > diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c > index cbea24666479..05975461bf10 100644 > --- a/drivers/net/usb/smsc95xx.c > +++ b/drivers/net/usb/smsc95xx.c > @@ -1006,6 +1006,18 @@ static int smsc95xx_reset(struct usbnet *dev) > /* Configure GPIO pins as LED outputs */ > write_buf = LED_GPIO_CFG_SPD_LED | LED_GPIO_CFG_LNK_LED | > LED_GPIO_CFG_FDX_LED; > + > + /* Set LED Select (LED_SEL) bit for the external LED pins functionality > + * in the Microchip's EVB-LAN8670-USB 10BASE-T1S Ethernet device which > + * uses the below LED function. > + * nSPD_LED -> Speed Indicator > + * nLNKA_LED -> Link Indicator > + * nFDX_LED -> Activity Indicator > + */ > + if (dev->udev->descriptor.idVendor == 0x184F && > + dev->udev->descriptor.idProduct == 0x0051) Hi Parthiban, There seems to be an endian missmatch here. The type of .idVendor and .idProduct is __le16, but here they are compared against host byte-order integers. Flagged by Sparse. > + write_buf |= LED_GPIO_CFG_LED_SEL; > + > ret = smsc95xx_write_reg(dev, LED_GPIO_CFG, write_buf); > if (ret < 0) > return ret; ...
Hi Parthiban, LED_SEL is configurable option by EEPROM which should be populated on EVB-LAN8670-USB. I would suggest changing EEPROM configuration than hard-coded in driver code. Thanks. Woojung > -----Original Message----- > From: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com> > Sent: Wednesday, May 22, 2024 10:08 AM > To: steve.glendinning@shawell.net; UNGLinuxDriver > <UNGLinuxDriver@microchip.com>; davem@davemloft.net; edumazet@google.com; > kuba@kernel.org; pabeni@redhat.com > Cc: netdev@vger.kernel.org; linux-usb@vger.kernel.org; linux- > kernel@vger.kernel.org; Parthiban Veerasooran - I17164 > <Parthiban.Veerasooran@microchip.com> > Subject: [PATCH] net: usb: smsc95xx: configure external LEDs function for > EVB-LAN8670-USB > > By default, LAN9500A configures the external LEDs to the below function. > nSPD_LED -> Speed Indicator > nLNKA_LED -> Link and Activity Indicator > nFDX_LED -> Full Duplex Link Indicator > > But, EVB-LAN8670-USB uses the below external LEDs function which can be > enabled by writing 1 to the LED Select (LED_SEL) bit in the LAN9500A. > nSPD_LED -> Speed Indicator > nLNKA_LED -> Link Indicator > nFDX_LED -> Activity Indicator > > Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com> > --- > drivers/net/usb/smsc95xx.c | 12 ++++++++++++ > drivers/net/usb/smsc95xx.h | 1 + > 2 files changed, 13 insertions(+) > > diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c > index cbea24666479..05975461bf10 100644 > --- a/drivers/net/usb/smsc95xx.c > +++ b/drivers/net/usb/smsc95xx.c > @@ -1006,6 +1006,18 @@ static int smsc95xx_reset(struct usbnet *dev) > /* Configure GPIO pins as LED outputs */ > write_buf = LED_GPIO_CFG_SPD_LED | LED_GPIO_CFG_LNK_LED | > LED_GPIO_CFG_FDX_LED; > + > + /* Set LED Select (LED_SEL) bit for the external LED pins > functionality > + * in the Microchip's EVB-LAN8670-USB 10BASE-T1S Ethernet device which > + * uses the below LED function. > + * nSPD_LED -> Speed Indicator > + * nLNKA_LED -> Link Indicator > + * nFDX_LED -> Activity Indicator > + */ > + if (dev->udev->descriptor.idVendor == 0x184F && > + dev->udev->descriptor.idProduct == 0x0051) > + write_buf |= LED_GPIO_CFG_LED_SEL; > + > ret = smsc95xx_write_reg(dev, LED_GPIO_CFG, write_buf); > if (ret < 0) > return ret; > diff --git a/drivers/net/usb/smsc95xx.h b/drivers/net/usb/smsc95xx.h > index 013bf42e27f2..134f3c2fddd9 100644 > --- a/drivers/net/usb/smsc95xx.h > +++ b/drivers/net/usb/smsc95xx.h > @@ -114,6 +114,7 @@ > > /* LED General Purpose IO Configuration Register */ > #define LED_GPIO_CFG (0x24) > +#define LED_GPIO_CFG_LED_SEL BIT(31) /* Separate Link/Act LEDs */ > #define LED_GPIO_CFG_SPD_LED (0x01000000) /* GPIOz as Speed LED */ > #define LED_GPIO_CFG_LNK_LED (0x00100000) /* GPIOy as Link LED */ > #define LED_GPIO_CFG_FDX_LED (0x00010000) /* GPIOx as Full Duplex LED > */ > > base-commit: 4b377b4868ef17b040065bd468668c707d2477a5 > -- > 2.34.1
Hi Parthiban, kernel test robot noticed the following build warnings: [auto build test WARNING on 4b377b4868ef17b040065bd468668c707d2477a5] url: https://github.com/intel-lab-lkp/linux/commits/Parthiban-Veerasooran/net-usb-smsc95xx-configure-external-LEDs-function-for-EVB-LAN8670-USB/20240522-221645 base: 4b377b4868ef17b040065bd468668c707d2477a5 patch link: https://lore.kernel.org/r/20240522140817.409936-1-Parthiban.Veerasooran%40microchip.com patch subject: [PATCH] net: usb: smsc95xx: configure external LEDs function for EVB-LAN8670-USB config: i386-randconfig-062-20240523 (https://download.01.org/0day-ci/archive/20240523/202405231332.bBXpW9Bj-lkp@intel.com/config) compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240523/202405231332.bBXpW9Bj-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202405231332.bBXpW9Bj-lkp@intel.com/ sparse warnings: (new ones prefixed by >>) >> drivers/net/usb/smsc95xx.c:1017:34: sparse: sparse: restricted __le16 degrades to integer drivers/net/usb/smsc95xx.c:1018:34: sparse: sparse: restricted __le16 degrades to integer vim +1017 drivers/net/usb/smsc95xx.c 878 879 static int smsc95xx_reset(struct usbnet *dev) 880 { 881 struct smsc95xx_priv *pdata = dev->driver_priv; 882 u32 read_buf, write_buf, burst_cap; 883 int ret = 0, timeout; 884 885 netif_dbg(dev, ifup, dev->net, "entering smsc95xx_reset\n"); 886 887 ret = smsc95xx_write_reg(dev, HW_CFG, HW_CFG_LRST_); 888 if (ret < 0) 889 return ret; 890 891 timeout = 0; 892 do { 893 msleep(10); 894 ret = smsc95xx_read_reg(dev, HW_CFG, &read_buf); 895 if (ret < 0) 896 return ret; 897 timeout++; 898 } while ((read_buf & HW_CFG_LRST_) && (timeout < 100)); 899 900 if (timeout >= 100) { 901 netdev_warn(dev->net, "timeout waiting for completion of Lite Reset\n"); 902 return -ETIMEDOUT; 903 } 904 905 ret = smsc95xx_set_mac_address(dev); 906 if (ret < 0) 907 return ret; 908 909 netif_dbg(dev, ifup, dev->net, "MAC Address: %pM\n", 910 dev->net->dev_addr); 911 912 ret = smsc95xx_read_reg(dev, HW_CFG, &read_buf); 913 if (ret < 0) 914 return ret; 915 916 netif_dbg(dev, ifup, dev->net, "Read Value from HW_CFG : 0x%08x\n", 917 read_buf); 918 919 read_buf |= HW_CFG_BIR_; 920 921 ret = smsc95xx_write_reg(dev, HW_CFG, read_buf); 922 if (ret < 0) 923 return ret; 924 925 ret = smsc95xx_read_reg(dev, HW_CFG, &read_buf); 926 if (ret < 0) 927 return ret; 928 929 netif_dbg(dev, ifup, dev->net, 930 "Read Value from HW_CFG after writing HW_CFG_BIR_: 0x%08x\n", 931 read_buf); 932 933 if (!turbo_mode) { 934 burst_cap = 0; 935 dev->rx_urb_size = MAX_SINGLE_PACKET_SIZE; 936 } else if (dev->udev->speed == USB_SPEED_HIGH) { 937 burst_cap = DEFAULT_HS_BURST_CAP_SIZE / HS_USB_PKT_SIZE; 938 dev->rx_urb_size = DEFAULT_HS_BURST_CAP_SIZE; 939 } else { 940 burst_cap = DEFAULT_FS_BURST_CAP_SIZE / FS_USB_PKT_SIZE; 941 dev->rx_urb_size = DEFAULT_FS_BURST_CAP_SIZE; 942 } 943 944 netif_dbg(dev, ifup, dev->net, "rx_urb_size=%ld\n", 945 (ulong)dev->rx_urb_size); 946 947 ret = smsc95xx_write_reg(dev, BURST_CAP, burst_cap); 948 if (ret < 0) 949 return ret; 950 951 ret = smsc95xx_read_reg(dev, BURST_CAP, &read_buf); 952 if (ret < 0) 953 return ret; 954 955 netif_dbg(dev, ifup, dev->net, 956 "Read Value from BURST_CAP after writing: 0x%08x\n", 957 read_buf); 958 959 ret = smsc95xx_write_reg(dev, BULK_IN_DLY, DEFAULT_BULK_IN_DELAY); 960 if (ret < 0) 961 return ret; 962 963 ret = smsc95xx_read_reg(dev, BULK_IN_DLY, &read_buf); 964 if (ret < 0) 965 return ret; 966 967 netif_dbg(dev, ifup, dev->net, 968 "Read Value from BULK_IN_DLY after writing: 0x%08x\n", 969 read_buf); 970 971 ret = smsc95xx_read_reg(dev, HW_CFG, &read_buf); 972 if (ret < 0) 973 return ret; 974 975 netif_dbg(dev, ifup, dev->net, "Read Value from HW_CFG: 0x%08x\n", 976 read_buf); 977 978 if (turbo_mode) 979 read_buf |= (HW_CFG_MEF_ | HW_CFG_BCE_); 980 981 read_buf &= ~HW_CFG_RXDOFF_; 982 983 /* set Rx data offset=2, Make IP header aligns on word boundary. */ 984 read_buf |= NET_IP_ALIGN << 9; 985 986 ret = smsc95xx_write_reg(dev, HW_CFG, read_buf); 987 if (ret < 0) 988 return ret; 989 990 ret = smsc95xx_read_reg(dev, HW_CFG, &read_buf); 991 if (ret < 0) 992 return ret; 993 994 netif_dbg(dev, ifup, dev->net, 995 "Read Value from HW_CFG after writing: 0x%08x\n", read_buf); 996 997 ret = smsc95xx_write_reg(dev, INT_STS, INT_STS_CLEAR_ALL_); 998 if (ret < 0) 999 return ret; 1000 1001 ret = smsc95xx_read_reg(dev, ID_REV, &read_buf); 1002 if (ret < 0) 1003 return ret; 1004 netif_dbg(dev, ifup, dev->net, "ID_REV = 0x%08x\n", read_buf); 1005 1006 /* Configure GPIO pins as LED outputs */ 1007 write_buf = LED_GPIO_CFG_SPD_LED | LED_GPIO_CFG_LNK_LED | 1008 LED_GPIO_CFG_FDX_LED; 1009 1010 /* Set LED Select (LED_SEL) bit for the external LED pins functionality 1011 * in the Microchip's EVB-LAN8670-USB 10BASE-T1S Ethernet device which 1012 * uses the below LED function. 1013 * nSPD_LED -> Speed Indicator 1014 * nLNKA_LED -> Link Indicator 1015 * nFDX_LED -> Activity Indicator 1016 */ > 1017 if (dev->udev->descriptor.idVendor == 0x184F && 1018 dev->udev->descriptor.idProduct == 0x0051) 1019 write_buf |= LED_GPIO_CFG_LED_SEL; 1020 1021 ret = smsc95xx_write_reg(dev, LED_GPIO_CFG, write_buf); 1022 if (ret < 0) 1023 return ret; 1024 1025 /* Init Tx */ 1026 ret = smsc95xx_write_reg(dev, FLOW, 0); 1027 if (ret < 0) 1028 return ret; 1029 1030 ret = smsc95xx_write_reg(dev, AFC_CFG, AFC_CFG_DEFAULT); 1031 if (ret < 0) 1032 return ret; 1033 1034 /* Don't need mac_cr_lock during initialisation */ 1035 ret = smsc95xx_read_reg(dev, MAC_CR, &pdata->mac_cr); 1036 if (ret < 0) 1037 return ret; 1038 1039 /* Init Rx */ 1040 /* Set Vlan */ 1041 ret = smsc95xx_write_reg(dev, VLAN1, (u32)ETH_P_8021Q); 1042 if (ret < 0) 1043 return ret; 1044 1045 /* Enable or disable checksum offload engines */ 1046 ret = smsc95xx_set_features(dev->net, dev->net->features); 1047 if (ret < 0) { 1048 netdev_warn(dev->net, "Failed to set checksum offload features\n"); 1049 return ret; 1050 } 1051 1052 smsc95xx_set_multicast(dev->net); 1053 1054 ret = smsc95xx_read_reg(dev, INT_EP_CTL, &read_buf); 1055 if (ret < 0) 1056 return ret; 1057 1058 /* enable PHY interrupts */ 1059 read_buf |= INT_EP_CTL_PHY_INT_; 1060 1061 ret = smsc95xx_write_reg(dev, INT_EP_CTL, read_buf); 1062 if (ret < 0) 1063 return ret; 1064 1065 ret = smsc95xx_start_tx_path(dev); 1066 if (ret < 0) { 1067 netdev_warn(dev->net, "Failed to start TX path\n"); 1068 return ret; 1069 } 1070 1071 ret = smsc95xx_start_rx_path(dev); 1072 if (ret < 0) { 1073 netdev_warn(dev->net, "Failed to start RX path\n"); 1074 return ret; 1075 } 1076 1077 netif_dbg(dev, ifup, dev->net, "smsc95xx_reset, return 0\n"); 1078 return 0; 1079 } 1080
Hi Woojung, On 23/05/24 1:22 am, Woojung Huh - C21699 wrote: > Hi Parthiban, > > LED_SEL is configurable option by EEPROM which should be populated on > EVB-LAN8670-USB. I would suggest changing EEPROM configuration than > hard-coded in driver code. Ah OK. Thanks for letting me know. I tried that EEPROM approach but that is needed a fix to work properly. I will send out another fix patch separately. Please review it. Please discard this patch as it is going to be invalid. Thanks for your understanding. Best regards, Parthiban V > > Thanks. > Woojung > >> -----Original Message----- >> From: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com> >> Sent: Wednesday, May 22, 2024 10:08 AM >> To: steve.glendinning@shawell.net; UNGLinuxDriver >> <UNGLinuxDriver@microchip.com>; davem@davemloft.net; edumazet@google.com; >> kuba@kernel.org; pabeni@redhat.com >> Cc: netdev@vger.kernel.org; linux-usb@vger.kernel.org; linux- >> kernel@vger.kernel.org; Parthiban Veerasooran - I17164 >> <Parthiban.Veerasooran@microchip.com> >> Subject: [PATCH] net: usb: smsc95xx: configure external LEDs function for >> EVB-LAN8670-USB >> >> By default, LAN9500A configures the external LEDs to the below function. >> nSPD_LED -> Speed Indicator >> nLNKA_LED -> Link and Activity Indicator >> nFDX_LED -> Full Duplex Link Indicator >> >> But, EVB-LAN8670-USB uses the below external LEDs function which can be >> enabled by writing 1 to the LED Select (LED_SEL) bit in the LAN9500A. >> nSPD_LED -> Speed Indicator >> nLNKA_LED -> Link Indicator >> nFDX_LED -> Activity Indicator >> >> Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com> >> --- >> drivers/net/usb/smsc95xx.c | 12 ++++++++++++ >> drivers/net/usb/smsc95xx.h | 1 + >> 2 files changed, 13 insertions(+) >> >> diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c >> index cbea24666479..05975461bf10 100644 >> --- a/drivers/net/usb/smsc95xx.c >> +++ b/drivers/net/usb/smsc95xx.c >> @@ -1006,6 +1006,18 @@ static int smsc95xx_reset(struct usbnet *dev) >> /* Configure GPIO pins as LED outputs */ >> write_buf = LED_GPIO_CFG_SPD_LED | LED_GPIO_CFG_LNK_LED | >> LED_GPIO_CFG_FDX_LED; >> + >> + /* Set LED Select (LED_SEL) bit for the external LED pins >> functionality >> + * in the Microchip's EVB-LAN8670-USB 10BASE-T1S Ethernet device which >> + * uses the below LED function. >> + * nSPD_LED -> Speed Indicator >> + * nLNKA_LED -> Link Indicator >> + * nFDX_LED -> Activity Indicator >> + */ >> + if (dev->udev->descriptor.idVendor == 0x184F && >> + dev->udev->descriptor.idProduct == 0x0051) >> + write_buf |= LED_GPIO_CFG_LED_SEL; >> + >> ret = smsc95xx_write_reg(dev, LED_GPIO_CFG, write_buf); >> if (ret < 0) >> return ret; >> diff --git a/drivers/net/usb/smsc95xx.h b/drivers/net/usb/smsc95xx.h >> index 013bf42e27f2..134f3c2fddd9 100644 >> --- a/drivers/net/usb/smsc95xx.h >> +++ b/drivers/net/usb/smsc95xx.h >> @@ -114,6 +114,7 @@ >> >> /* LED General Purpose IO Configuration Register */ >> #define LED_GPIO_CFG (0x24) >> +#define LED_GPIO_CFG_LED_SEL BIT(31) /* Separate Link/Act LEDs */ >> #define LED_GPIO_CFG_SPD_LED (0x01000000) /* GPIOz as Speed LED */ >> #define LED_GPIO_CFG_LNK_LED (0x00100000) /* GPIOy as Link LED */ >> #define LED_GPIO_CFG_FDX_LED (0x00010000) /* GPIOx as Full Duplex LED >> */ >> >> base-commit: 4b377b4868ef17b040065bd468668c707d2477a5 >> -- >> 2.34.1 >
Hi Simon, On 23/05/24 12:24 am, Simon Horman wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Wed, May 22, 2024 at 07:38:17PM +0530, Parthiban Veerasooran wrote: >> By default, LAN9500A configures the external LEDs to the below function. >> nSPD_LED -> Speed Indicator >> nLNKA_LED -> Link and Activity Indicator >> nFDX_LED -> Full Duplex Link Indicator >> >> But, EVB-LAN8670-USB uses the below external LEDs function which can be >> enabled by writing 1 to the LED Select (LED_SEL) bit in the LAN9500A. >> nSPD_LED -> Speed Indicator >> nLNKA_LED -> Link Indicator >> nFDX_LED -> Activity Indicator >> >> Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com> >> --- >> drivers/net/usb/smsc95xx.c | 12 ++++++++++++ >> drivers/net/usb/smsc95xx.h | 1 + >> 2 files changed, 13 insertions(+) >> >> diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c >> index cbea24666479..05975461bf10 100644 >> --- a/drivers/net/usb/smsc95xx.c >> +++ b/drivers/net/usb/smsc95xx.c >> @@ -1006,6 +1006,18 @@ static int smsc95xx_reset(struct usbnet *dev) >> /* Configure GPIO pins as LED outputs */ >> write_buf = LED_GPIO_CFG_SPD_LED | LED_GPIO_CFG_LNK_LED | >> LED_GPIO_CFG_FDX_LED; >> + >> + /* Set LED Select (LED_SEL) bit for the external LED pins functionality >> + * in the Microchip's EVB-LAN8670-USB 10BASE-T1S Ethernet device which >> + * uses the below LED function. >> + * nSPD_LED -> Speed Indicator >> + * nLNKA_LED -> Link Indicator >> + * nFDX_LED -> Activity Indicator >> + */ >> + if (dev->udev->descriptor.idVendor == 0x184F && >> + dev->udev->descriptor.idProduct == 0x0051) > > Hi Parthiban, > > There seems to be an endian missmatch here. > The type of .idVendor and .idProduct is __le16, > but here they are compared against host byte-order integers. Sorry, somehow I missed to check this. Will try to avoid this in the future. Thanks for letting me know. But I am not going to proceed further with this patch as Woojung pointed out EEPROM approach can be used instead of updating the driver. So please discard this patch. But the EEPROM approach mentioned by Woojung is needed a fix in the driver to work properly. I will send out that fix patch separately. Please review it. Thanks for your understanding. Best regards, Parthiban V > > Flagged by Sparse. > >> + write_buf |= LED_GPIO_CFG_LED_SEL; >> + >> ret = smsc95xx_write_reg(dev, LED_GPIO_CFG, write_buf); >> if (ret < 0) >> return ret; > > ... > > -- > pw-bot: changes-requested >
Hi Andrew, On 22/05/24 10:14 pm, Andrew Lunn wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Wed, May 22, 2024 at 07:38:17PM +0530, Parthiban Veerasooran wrote: >> By default, LAN9500A configures the external LEDs to the below function. >> nSPD_LED -> Speed Indicator >> nLNKA_LED -> Link and Activity Indicator >> nFDX_LED -> Full Duplex Link Indicator >> >> But, EVB-LAN8670-USB uses the below external LEDs function which can be >> enabled by writing 1 to the LED Select (LED_SEL) bit in the LAN9500A. >> nSPD_LED -> Speed Indicator >> nLNKA_LED -> Link Indicator >> nFDX_LED -> Activity Indicator > > What else can the LEDs indicate? There is no other indications. > >> + /* Set LED Select (LED_SEL) bit for the external LED pins functionality >> + * in the Microchip's EVB-LAN8670-USB 10BASE-T1S Ethernet device which > > Is this a function of the USB dongle? Or a function of the PHY? It is the function of USB dongle. Sorry Andrew, I am not going to proceed further with this patch as Woojung pointed out EEPROM approach can be used instead of updating the driver. I tried that as well. So please discard this patch. But the EEPROM approach mentioned by Woojung is needed a fix in the driver to work properly. I will send out that fix patch separately. Please review it. Thanks for your understanding. Best regards, Parthiban V > > Andrew
On Thu, May 23, 2024 at 08:51:54AM +0000, Parthiban.Veerasooran@microchip.com wrote: > Hi Andrew, > > On 22/05/24 10:14 pm, Andrew Lunn wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > > > On Wed, May 22, 2024 at 07:38:17PM +0530, Parthiban Veerasooran wrote: > >> By default, LAN9500A configures the external LEDs to the below function. > >> nSPD_LED -> Speed Indicator > >> nLNKA_LED -> Link and Activity Indicator > >> nFDX_LED -> Full Duplex Link Indicator > >> > >> But, EVB-LAN8670-USB uses the below external LEDs function which can be > >> enabled by writing 1 to the LED Select (LED_SEL) bit in the LAN9500A. > >> nSPD_LED -> Speed Indicator > >> nLNKA_LED -> Link Indicator > >> nFDX_LED -> Activity Indicator > > > > What else can the LEDs indicate? > There is no other indications. O.K. So it is probably not worth going the direction of using the netde LED infrastructure to allow the use to configure the LED. > >> + /* Set LED Select (LED_SEL) bit for the external LED pins functionality > >> + * in the Microchip's EVB-LAN8670-USB 10BASE-T1S Ethernet device which > > > > Is this a function of the USB dongle? Or a function of the PHY? > It is the function of USB dongle. So an OEM designing a dongle could make the LEDs do different things? You are solving the problem only for your reference design, and OEMs are going to have to solve the same problem for their own design? This is why i'm asking is it a function of the PHY or the board. If it is the PHY, we could have one generic solution for everybody using that PHY. Andrew
Hi Andrew, On 23/05/24 6:13 pm, Andrew Lunn wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Thu, May 23, 2024 at 08:51:54AM +0000, Parthiban.Veerasooran@microchip.com wrote: >> Hi Andrew, >> >> On 22/05/24 10:14 pm, Andrew Lunn wrote: >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >>> >>> On Wed, May 22, 2024 at 07:38:17PM +0530, Parthiban Veerasooran wrote: >>>> By default, LAN9500A configures the external LEDs to the below function. >>>> nSPD_LED -> Speed Indicator >>>> nLNKA_LED -> Link and Activity Indicator >>>> nFDX_LED -> Full Duplex Link Indicator >>>> >>>> But, EVB-LAN8670-USB uses the below external LEDs function which can be >>>> enabled by writing 1 to the LED Select (LED_SEL) bit in the LAN9500A. >>>> nSPD_LED -> Speed Indicator >>>> nLNKA_LED -> Link Indicator >>>> nFDX_LED -> Activity Indicator >>> >>> What else can the LEDs indicate? >> There is no other indications. > > O.K. So it is probably not worth going the direction of using the > netde LED infrastructure to allow the use to configure the LED. > >>>> + /* Set LED Select (LED_SEL) bit for the external LED pins functionality >>>> + * in the Microchip's EVB-LAN8670-USB 10BASE-T1S Ethernet device which >>> >>> Is this a function of the USB dongle? Or a function of the PHY? >> It is the function of USB dongle. > > So an OEM designing a dongle could make the LEDs do different things? Yes. > > You are solving the problem only for your reference design, and OEMs > are going to have to solve the same problem for their own design? > > This is why i'm asking is it a function of the PHY or the board. If it > is the PHY, we could have one generic solution for everybody using > that PHY. OK, it is a function of the board not PHY and also that depends on the board design based on the requirement I guess. Best regards, Parthiban V > > Andrew
On 22.05.24 16:08, Parthiban Veerasooran wrote: Hi, however you solve this, the descriptors are stored in wire order. > + if (dev->udev->descriptor.idVendor == 0x184F && > + dev->udev->descriptor.idProduct == 0x0051) > + write_buf |= LED_GPIO_CFG_LED_SEL; This needs to be if (dev->udev->descriptor.idVendor == cpu_to_le16(0x184F) && dev->udev->descriptor.idProduct == cpu_to_le16(0x0051)) HTH Oliver
Hi Oliver, On 27/05/24 1:58 pm, Oliver Neukum wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know > the content is safe > > On 22.05.24 16:08, Parthiban Veerasooran wrote: > > Hi, > > however you solve this, the descriptors are stored in wire order. > >> + if (dev->udev->descriptor.idVendor == 0x184F && >> + dev->udev->descriptor.idProduct == 0x0051) >> + write_buf |= LED_GPIO_CFG_LED_SEL; > > This needs to be > > if (dev->udev->descriptor.idVendor == cpu_to_le16(0x184F) && > dev->udev->descriptor.idProduct == cpu_to_le16(0x0051)) > > HTH > Oliver Thanks for reviewing the patch. This one was already pointed out by Simon Horman and kernel test robot. I agreed that but unfortunately there was an another proposal by Woojung with EEPROM. So I asked to discard this patch proceeding further and sent out another fix patch for supporting with Woojung's EEPROM proposal. My request to discard this patch: --------------------------------- https://lore.kernel.org/netdev/3e2b2b18-4dd9-4b22-9690-01e1bdd44828@microchip.com/ My request to review the other/new patch: ----------------------------------------- https://lore.kernel.org/netdev/20240523085314.167650-1-Parthiban.Veerasooran@microchip.com/ The above patch has been already reviewed by both Woojung and Simon: -------------------------------------------------------------------- https://lore.kernel.org/netdev/20240523144056.GO883722@kernel.org/ https://lore.kernel.org/netdev/BL0PR11MB2913CE81AFC08F2D619C3B6CE7F42@BL0PR11MB2913.namprd11.prod.outlook.com/ Hope this clarifies. Best regards, Parthiban V
diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c index cbea24666479..05975461bf10 100644 --- a/drivers/net/usb/smsc95xx.c +++ b/drivers/net/usb/smsc95xx.c @@ -1006,6 +1006,18 @@ static int smsc95xx_reset(struct usbnet *dev) /* Configure GPIO pins as LED outputs */ write_buf = LED_GPIO_CFG_SPD_LED | LED_GPIO_CFG_LNK_LED | LED_GPIO_CFG_FDX_LED; + + /* Set LED Select (LED_SEL) bit for the external LED pins functionality + * in the Microchip's EVB-LAN8670-USB 10BASE-T1S Ethernet device which + * uses the below LED function. + * nSPD_LED -> Speed Indicator + * nLNKA_LED -> Link Indicator + * nFDX_LED -> Activity Indicator + */ + if (dev->udev->descriptor.idVendor == 0x184F && + dev->udev->descriptor.idProduct == 0x0051) + write_buf |= LED_GPIO_CFG_LED_SEL; + ret = smsc95xx_write_reg(dev, LED_GPIO_CFG, write_buf); if (ret < 0) return ret; diff --git a/drivers/net/usb/smsc95xx.h b/drivers/net/usb/smsc95xx.h index 013bf42e27f2..134f3c2fddd9 100644 --- a/drivers/net/usb/smsc95xx.h +++ b/drivers/net/usb/smsc95xx.h @@ -114,6 +114,7 @@ /* LED General Purpose IO Configuration Register */ #define LED_GPIO_CFG (0x24) +#define LED_GPIO_CFG_LED_SEL BIT(31) /* Separate Link/Act LEDs */ #define LED_GPIO_CFG_SPD_LED (0x01000000) /* GPIOz as Speed LED */ #define LED_GPIO_CFG_LNK_LED (0x00100000) /* GPIOy as Link LED */ #define LED_GPIO_CFG_FDX_LED (0x00010000) /* GPIOx as Full Duplex LED */
By default, LAN9500A configures the external LEDs to the below function. nSPD_LED -> Speed Indicator nLNKA_LED -> Link and Activity Indicator nFDX_LED -> Full Duplex Link Indicator But, EVB-LAN8670-USB uses the below external LEDs function which can be enabled by writing 1 to the LED Select (LED_SEL) bit in the LAN9500A. nSPD_LED -> Speed Indicator nLNKA_LED -> Link Indicator nFDX_LED -> Activity Indicator Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com> --- drivers/net/usb/smsc95xx.c | 12 ++++++++++++ drivers/net/usb/smsc95xx.h | 1 + 2 files changed, 13 insertions(+) base-commit: 4b377b4868ef17b040065bd468668c707d2477a5