| Message ID | 9720627.53btSdPcQU@wuerfel |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, Nov 19, 2015 at 11:42:26AM +0100, Arnd Bergmann wrote: > The tulip driver causes annoying build-time warnings for allmodconfig > builds for all recent architectures: > > dec/tulip/winbond-840.c:910:2: warning: #warning Processor architecture undefined > dec/tulip/tulip_core.c:101:2: warning: #warning Processor architecture undefined! > > This is the last remaining warning for arm64, and I'd like to get rid of > it. We don't really know the cache line size, architecturally it would > be at least 16 bytes, but all implementations I found have 64 or 128 > bytes. Configuring tulip for 32-byte lines as we do on ARM32 seems to > be the safe but slow default, and nobody who cares about performance these > days would use a tulip chip anyway, so we can just use that. > > To save the next person the job of trying to find out what this is for > and picking a default for their architecture just to kill off the warning, > I'm now removing the preprocessor #warning and turning it into a pr_warn > or dev_warn that prints the equivalent information when the driver gets > loaded. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > diff --git a/drivers/net/ethernet/dec/tulip/tulip_core.c b/drivers/net/ethernet/dec/tulip/tulip_core.c > index ed41559bae77..b553409e04ad 100644 > --- a/drivers/net/ethernet/dec/tulip/tulip_core.c > +++ b/drivers/net/ethernet/dec/tulip/tulip_core.c > @@ -98,8 +98,7 @@ static int csr0 = 0x01A00000 | 0x4800; > #elif defined(__mips__) > static int csr0 = 0x00200000 | 0x4000; > #else > -#warning Processor architecture undefined! > -static int csr0 = 0x00A00000 | 0x4800; > +static int csr0; > #endif > > /* Operational parameters that usually are not changed. */ > @@ -1982,6 +1981,12 @@ static int __init tulip_init (void) > pr_info("%s", version); > #endif > > + if (!csr0) { > + pr_warn("tulip: unknown CPU architecture, using default csr0\n"); > + /* default to 8 longword cache line alignment */ > + csr0 = 0x00A00000 | 0x4800; Maybe print "defaulting to 8 longword cache line alignment" instead of "default csr0"? > diff --git a/drivers/net/ethernet/dec/tulip/winbond-840.c b/drivers/net/ethernet/dec/tulip/winbond-840.c > index 9beb3d34d4ba..3c0e4d5c5fef 100644 > --- a/drivers/net/ethernet/dec/tulip/winbond-840.c > +++ b/drivers/net/ethernet/dec/tulip/winbond-840.c > @@ -907,7 +907,7 @@ static void init_registers(struct net_device *dev) > #elif defined(CONFIG_SPARC) || defined (CONFIG_PARISC) || defined(CONFIG_ARM) > i |= 0x4800; > #else > -#warning Processor architecture undefined > + dev_warn(&dev->dev, "unknown CPU architecture, using default csr0 setting\n"); > i |= 0x4800; Then we could print the default csr0 value here. But, to be honest, this patch fixes a #warning on arm64 for a driver that I never expect to be used. So whatever you do to silence it: Acked-by: Will Deacon <will.deacon@arm.com> /me waits for on-soc tulip integration. Will -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 19/11/15 04:26, Will Deacon wrote: > On Thu, Nov 19, 2015 at 11:42:26AM +0100, Arnd Bergmann wrote: >> The tulip driver causes annoying build-time warnings for allmodconfig >> builds for all recent architectures: >> >> dec/tulip/winbond-840.c:910:2: warning: #warning Processor architecture undefined >> dec/tulip/tulip_core.c:101:2: warning: #warning Processor architecture undefined! >> >> This is the last remaining warning for arm64, and I'd like to get rid of >> it. We don't really know the cache line size, architecturally it would >> be at least 16 bytes, but all implementations I found have 64 or 128 >> bytes. Configuring tulip for 32-byte lines as we do on ARM32 seems to >> be the safe but slow default, and nobody who cares about performance these >> days would use a tulip chip anyway, so we can just use that. >> >> To save the next person the job of trying to find out what this is for >> and picking a default for their architecture just to kill off the warning, >> I'm now removing the preprocessor #warning and turning it into a pr_warn >> or dev_warn that prints the equivalent information when the driver gets >> loaded. >> >> Signed-off-by: Arnd Bergmann <arnd@arndb.de> >> >> diff --git a/drivers/net/ethernet/dec/tulip/tulip_core.c b/drivers/net/ethernet/dec/tulip/tulip_core.c >> index ed41559bae77..b553409e04ad 100644 >> --- a/drivers/net/ethernet/dec/tulip/tulip_core.c >> +++ b/drivers/net/ethernet/dec/tulip/tulip_core.c >> @@ -98,8 +98,7 @@ static int csr0 = 0x01A00000 | 0x4800; >> #elif defined(__mips__) >> static int csr0 = 0x00200000 | 0x4000; >> #else >> -#warning Processor architecture undefined! >> -static int csr0 = 0x00A00000 | 0x4800; >> +static int csr0; >> #endif >> >> /* Operational parameters that usually are not changed. */ >> @@ -1982,6 +1981,12 @@ static int __init tulip_init (void) >> pr_info("%s", version); >> #endif >> >> + if (!csr0) { >> + pr_warn("tulip: unknown CPU architecture, using default csr0\n"); >> + /* default to 8 longword cache line alignment */ >> + csr0 = 0x00A00000 | 0x4800; > > Maybe print "defaulting to 8 longword cache line alignment" instead of > "default csr0"? > >> diff --git a/drivers/net/ethernet/dec/tulip/winbond-840.c b/drivers/net/ethernet/dec/tulip/winbond-840.c >> index 9beb3d34d4ba..3c0e4d5c5fef 100644 >> --- a/drivers/net/ethernet/dec/tulip/winbond-840.c >> +++ b/drivers/net/ethernet/dec/tulip/winbond-840.c >> @@ -907,7 +907,7 @@ static void init_registers(struct net_device *dev) >> #elif defined(CONFIG_SPARC) || defined (CONFIG_PARISC) || defined(CONFIG_ARM) >> i |= 0x4800; >> #else >> -#warning Processor architecture undefined >> + dev_warn(&dev->dev, "unknown CPU architecture, using default csr0 setting\n"); >> i |= 0x4800; > > Then we could print the default csr0 value here. > > But, to be honest, this patch fixes a #warning on arm64 for a driver that > I never expect to be used. So whatever you do to silence it: > > Acked-by: Will Deacon <will.deacon@arm.com> > > /me waits for on-soc tulip integration. FWIW, this already happened, the ADMtek/Infineon ADM8668 actually integrated a Tulip chip. I have not submitted these patches below from the OpenWrt tree because the chip is barely used nowadays, and it was only mostly popular with the Linksys WRTU54G. The patches could be made less intrusive if we did convert the pci_dma* calls into regular DMA-API calls, which they are nowadays, oh well! https://dev.openwrt.org/browser/trunk/target/linux/adm8668/patches-3.18/004-tulip_pci_split.patch https://dev.openwrt.org/browser/trunk/target/linux/adm8668/patches-3.18/005-tulip_platform.patch
On Thu, Nov 19, 2015 at 4:26 AM, Will Deacon <will.deacon@arm.com> wrote: > On Thu, Nov 19, 2015 at 11:42:26AM +0100, Arnd Bergmann wrote: >> The tulip driver causes annoying build-time warnings for allmodconfig >> builds for all recent architectures: >> >> dec/tulip/winbond-840.c:910:2: warning: #warning Processor architecture undefined >> dec/tulip/tulip_core.c:101:2: warning: #warning Processor architecture undefined! >> >> This is the last remaining warning for arm64, and I'd like to get rid of >> it. We don't really know the cache line size, architecturally it would >> be at least 16 bytes, but all implementations I found have 64 or 128 >> bytes. Configuring tulip for 32-byte lines as we do on ARM32 seems to >> be the safe but slow default, and nobody who cares about performance these >> days would use a tulip chip anyway, so we can just use that. >> >> To save the next person the job of trying to find out what this is for >> and picking a default for their architecture just to kill off the warning, >> I'm now removing the preprocessor #warning and turning it into a pr_warn >> or dev_warn that prints the equivalent information when the driver gets >> loaded. >> >> Signed-off-by: Arnd Bergmann <arnd@arndb.de> >> >> diff --git a/drivers/net/ethernet/dec/tulip/tulip_core.c b/drivers/net/ethernet/dec/tulip/tulip_core.c >> index ed41559bae77..b553409e04ad 100644 >> --- a/drivers/net/ethernet/dec/tulip/tulip_core.c >> +++ b/drivers/net/ethernet/dec/tulip/tulip_core.c >> @@ -98,8 +98,7 @@ static int csr0 = 0x01A00000 | 0x4800; >> #elif defined(__mips__) >> static int csr0 = 0x00200000 | 0x4000; >> #else >> -#warning Processor architecture undefined! >> -static int csr0 = 0x00A00000 | 0x4800; >> +static int csr0; >> #endif >> >> /* Operational parameters that usually are not changed. */ >> @@ -1982,6 +1981,12 @@ static int __init tulip_init (void) >> pr_info("%s", version); >> #endif >> >> + if (!csr0) { >> + pr_warn("tulip: unknown CPU architecture, using default csr0\n"); >> + /* default to 8 longword cache line alignment */ >> + csr0 = 0x00A00000 | 0x4800; > > Maybe print "defaulting to 8 longword cache line alignment" instead of > "default csr0"? This is a good idea. >> diff --git a/drivers/net/ethernet/dec/tulip/winbond-840.c b/drivers/net/ethernet/dec/tulip/winbond-840.c >> index 9beb3d34d4ba..3c0e4d5c5fef 100644 >> --- a/drivers/net/ethernet/dec/tulip/winbond-840.c >> +++ b/drivers/net/ethernet/dec/tulip/winbond-840.c >> @@ -907,7 +907,7 @@ static void init_registers(struct net_device *dev) >> #elif defined(CONFIG_SPARC) || defined (CONFIG_PARISC) || defined(CONFIG_ARM) >> i |= 0x4800; >> #else >> -#warning Processor architecture undefined >> + dev_warn(&dev->dev, "unknown CPU architecture, using default csr0 setting\n"); >> i |= 0x4800; > > Then we could print the default csr0 value here. > > But, to be honest, this patch fixes a #warning on arm64 for a driver that > I never expect to be used. So whatever you do to silence it: > > Acked-by: Will Deacon <will.deacon@arm.com> yeah - same here: Acked-by: Grant Grundler <grundler@parisc-linux.org> cheers, grant > > /me waits for on-soc tulip integration. > > Will -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 19, 2015 at 12:29 PM, Florian Fainelli <f.fainelli@gmail.com> wrote: > On 19/11/15 04:26, Will Deacon wrote: ... >> /me waits for on-soc tulip integration. > > FWIW, this already happened, the ADMtek/Infineon ADM8668 actually > integrated a Tulip chip. I have not submitted these patches below from > the OpenWrt tree because the chip is barely used nowadays, and it was > only mostly popular with the Linksys WRTU54G. > > The patches could be made less intrusive if we did convert the pci_dma* > calls into regular DMA-API calls, which they are nowadays, oh well! I agree. IIRC, there was no DMA-API when this driver was written. James Bottomley added DMA API later and there was no need to convert since Tulip devices were _only_ PCI at the time. > https://dev.openwrt.org/browser/trunk/target/linux/adm8668/patches-3.18/004-tulip_pci_split.patch In general this would be a reasonable patch to submit here with some caveats: 1) convert to DMA API (first patch) 2) add CONFIG_PCI code (second patch) to handle the remaining discovery and PCI Config space bits. Some additional minor refactoring of the code could convert this into a "multi-bus driver" if there is any system that could incorporate both a platform device and a PCI device. I expect the conversion to DMA API to be straight forward as the next patch shows: > https://dev.openwrt.org/browser/trunk/target/linux/adm8668/patches-3.18/005-tulip_platform.patch Split this patch into two parts: convert to DMA-API (first patch) and platform device support (third patch). Should be a "no brainer" to accept. Lastly, net/ethernet/dec/tulip driver is up for adoption. I've just been extremely lazy about updating the MAINTAINERS entry but will submit that shortly (apologies to Arndt for the bounced email - I know it's a bit disconcerting to see that.) I'm happy to continue review tulip code changes anyway. cheers, grant > -- > Florian -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Arnd Bergmann <arnd@arndb.de> Date: Thu, 19 Nov 2015 11:42:26 +0100 > The tulip driver causes annoying build-time warnings for allmodconfig > builds for all recent architectures: > > dec/tulip/winbond-840.c:910:2: warning: #warning Processor architecture undefined > dec/tulip/tulip_core.c:101:2: warning: #warning Processor architecture undefined! > > This is the last remaining warning for arm64, and I'd like to get rid of > it. We don't really know the cache line size, architecturally it would > be at least 16 bytes, but all implementations I found have 64 or 128 > bytes. Configuring tulip for 32-byte lines as we do on ARM32 seems to > be the safe but slow default, and nobody who cares about performance these > days would use a tulip chip anyway, so we can just use that. > > To save the next person the job of trying to find out what this is for > and picking a default for their architecture just to kill off the warning, > I'm now removing the preprocessor #warning and turning it into a pr_warn > or dev_warn that prints the equivalent information when the driver gets > loaded. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> Seems reasonable, applied, thanks Arnd! -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/ethernet/dec/tulip/tulip_core.c b/drivers/net/ethernet/dec/tulip/tulip_core.c index ed41559bae77..b553409e04ad 100644 --- a/drivers/net/ethernet/dec/tulip/tulip_core.c +++ b/drivers/net/ethernet/dec/tulip/tulip_core.c @@ -98,8 +98,7 @@ static int csr0 = 0x01A00000 | 0x4800; #elif defined(__mips__) static int csr0 = 0x00200000 | 0x4000; #else -#warning Processor architecture undefined! -static int csr0 = 0x00A00000 | 0x4800; +static int csr0; #endif /* Operational parameters that usually are not changed. */ @@ -1982,6 +1981,12 @@ static int __init tulip_init (void) pr_info("%s", version); #endif + if (!csr0) { + pr_warn("tulip: unknown CPU architecture, using default csr0\n"); + /* default to 8 longword cache line alignment */ + csr0 = 0x00A00000 | 0x4800; + } + /* copy module parms into globals */ tulip_rx_copybreak = rx_copybreak; tulip_max_interrupt_work = max_interrupt_work; diff --git a/drivers/net/ethernet/dec/tulip/winbond-840.c b/drivers/net/ethernet/dec/tulip/winbond-840.c index 9beb3d34d4ba..3c0e4d5c5fef 100644 --- a/drivers/net/ethernet/dec/tulip/winbond-840.c +++ b/drivers/net/ethernet/dec/tulip/winbond-840.c @@ -907,7 +907,7 @@ static void init_registers(struct net_device *dev) #elif defined(CONFIG_SPARC) || defined (CONFIG_PARISC) || defined(CONFIG_ARM) i |= 0x4800; #else -#warning Processor architecture undefined + dev_warn(&dev->dev, "unknown CPU architecture, using default csr0 setting\n"); i |= 0x4800; #endif iowrite32(i, ioaddr + PCIBusCfg);
The tulip driver causes annoying build-time warnings for allmodconfig builds for all recent architectures: dec/tulip/winbond-840.c:910:2: warning: #warning Processor architecture undefined dec/tulip/tulip_core.c:101:2: warning: #warning Processor architecture undefined! This is the last remaining warning for arm64, and I'd like to get rid of it. We don't really know the cache line size, architecturally it would be at least 16 bytes, but all implementations I found have 64 or 128 bytes. Configuring tulip for 32-byte lines as we do on ARM32 seems to be the safe but slow default, and nobody who cares about performance these days would use a tulip chip anyway, so we can just use that. To save the next person the job of trying to find out what this is for and picking a default for their architecture just to kill off the warning, I'm now removing the preprocessor #warning and turning it into a pr_warn or dev_warn that prints the equivalent information when the driver gets loaded. Signed-off-by: Arnd Bergmann <arnd@arndb.de> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html