[PATCH -next] net: hisilicon: Never build on SPARC

[PATCH -next] net: hisilicon: Never build on SPARC

[Guenter Roeck]

The Hisilicon network driver does not build for Sparc. Enabling
COMPILE_TEST for it causes Sparc allmodconfig/allyesconfig builds
to fail with

drivers/net/ethernet/hisilicon/hns_mdio.c: In function 'hns_mdio_bus_name':
drivers/net/ethernet/hisilicon/hns_mdio.c:409:3: error:
		implicit declaration of function 'of_translate_address'

Fixes: 876133d3161d ("net: hisilicon: add OF dependency")
Cc: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/net/ethernet/hisilicon/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/hisilicon/Kconfig b/drivers/net/ethernet/hisilicon/Kconfig
index f250dec488fd..413935085591 100644
--- a/drivers/net/ethernet/hisilicon/Kconfig
+++ b/drivers/net/ethernet/hisilicon/Kconfig
@@ -5,7 +5,7 @@
 config NET_VENDOR_HISILICON
 	bool "Hisilicon devices"
 	default y
-	depends on OF && (ARM || ARM64 || COMPILE_TEST)
+	depends on OF && (ARM || ARM64 || COMPILE_TEST) && !SPARC
 	---help---
 	  If you have a network (Ethernet) card belonging to this class, say Y.
 
-- 
2.1.4

Re: [PATCH -next] net: hisilicon: Never build on SPARC

[Arnd Bergmann]

On Wednesday 21 October 2015 07:29:33 Guenter Roeck wrote:
> The Hisilicon network driver does not build for Sparc. Enabling
> COMPILE_TEST for it causes Sparc allmodconfig/allyesconfig builds
> to fail with
> 
> drivers/net/ethernet/hisilicon/hns_mdio.c: In function 'hns_mdio_bus_name':
> drivers/net/ethernet/hisilicon/hns_mdio.c:409:3: error:
>                 implicit declaration of function 'of_translate_address'

I see.

> Fixes: 876133d3161d ("net: hisilicon: add OF dependency")
> Cc: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/net/ethernet/hisilicon/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/hisilicon/Kconfig b/drivers/net/ethernet/hisilicon/Kconfig
> index f250dec488fd..413935085591 100644
> --- a/drivers/net/ethernet/hisilicon/Kconfig
> +++ b/drivers/net/ethernet/hisilicon/Kconfig
> @@ -5,7 +5,7 @@
>  config NET_VENDOR_HISILICON
>         bool "Hisilicon devices"
>         default y
> -       depends on OF && (ARM || ARM64 || COMPILE_TEST)
> +       depends on OF && (ARM || ARM64 || COMPILE_TEST) && !SPARC
>         ---help---
>           If you have a network (Ethernet) card belonging to this class, say Y.

This looks fragile to me. Checking the declaration of of_translate_address,
I see now that it actually depends on CONFIG_OF_ADDRESS, which is defined using
"depends on !SPARC && HAS_IOMEM". This means we would get the same problem on
SCORE, Tile, and UML.

How about this version?

diff --git a/include/linux/of_address.h b/include/linux/of_address.h
index d88e81be6368..f2f7986cac45 100644
--- a/include/linux/of_address.h
+++ b/include/linux/of_address.h
@@ -57,6 +57,11 @@ extern int of_dma_get_range(struct device_node *np, u64 *dma_addr,
 				u64 *paddr, u64 *size);
 extern bool of_dma_is_coherent(struct device_node *np);
 #else /* CONFIG_OF_ADDRESS */
+static inline u64 of_translate_address(struct device_node *np, const __be32 *addr)
+{
+	return 0;
+}
+
 static inline struct device_node *of_find_matching_node_by_address(
 					struct device_node *from,
 					const struct of_device_id *matches,


It looks like it's in line with the other wrappers here. Alternatively,
we could decide to use CONFIG_OF_ADDRESS instead of CONFIG_OF as the dependency.

	Arnd

Re: [PATCH -next] net: hisilicon: Never build on SPARC

[Guenter Roeck]

Hi Arnd,

On 10/21/2015 07:39 AM, Arnd Bergmann wrote:
> On Wednesday 21 October 2015 07:29:33 Guenter Roeck wrote:
>> The Hisilicon network driver does not build for Sparc. Enabling
>> COMPILE_TEST for it causes Sparc allmodconfig/allyesconfig builds
>> to fail with
>>
>> drivers/net/ethernet/hisilicon/hns_mdio.c: In function 'hns_mdio_bus_name':
>> drivers/net/ethernet/hisilicon/hns_mdio.c:409:3: error:
>>                  implicit declaration of function 'of_translate_address'
>
> I see.
>
>> Fixes: 876133d3161d ("net: hisilicon: add OF dependency")
>> Cc: Arnd Bergmann <arnd@arndb.de>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>>   drivers/net/ethernet/hisilicon/Kconfig | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/hisilicon/Kconfig b/drivers/net/ethernet/hisilicon/Kconfig
>> index f250dec488fd..413935085591 100644
>> --- a/drivers/net/ethernet/hisilicon/Kconfig
>> +++ b/drivers/net/ethernet/hisilicon/Kconfig
>> @@ -5,7 +5,7 @@
>>   config NET_VENDOR_HISILICON
>>          bool "Hisilicon devices"
>>          default y
>> -       depends on OF && (ARM || ARM64 || COMPILE_TEST)
>> +       depends on OF && (ARM || ARM64 || COMPILE_TEST) && !SPARC
>>          ---help---
>>            If you have a network (Ethernet) card belonging to this class, say Y.
>
> This looks fragile to me. Checking the declaration of of_translate_address,
> I see now that it actually depends on CONFIG_OF_ADDRESS, which is defined using
> "depends on !SPARC && HAS_IOMEM". This means we would get the same problem on
> SCORE, Tile, and UML.
>
> How about this version?
>
> diff --git a/include/linux/of_address.h b/include/linux/of_address.h
> index d88e81be6368..f2f7986cac45 100644
> --- a/include/linux/of_address.h
> +++ b/include/linux/of_address.h
> @@ -57,6 +57,11 @@ extern int of_dma_get_range(struct device_node *np, u64 *dma_addr,
>   				u64 *paddr, u64 *size);
>   extern bool of_dma_is_coherent(struct device_node *np);
>   #else /* CONFIG_OF_ADDRESS */
> +static inline u64 of_translate_address(struct device_node *np, const __be32 *addr)
> +{
> +	return 0;

Maybe return OF_BAD_ADDR ?

> +}
> +
>   static inline struct device_node *of_find_matching_node_by_address(
>   					struct device_node *from,
>   					const struct of_device_id *matches,
>
>
> It looks like it's in line with the other wrappers here. Alternatively,
> we could decide to use CONFIG_OF_ADDRESS instead of CONFIG_OF as the dependency.
>
You are right, both of those would be better than my patch.
My preference would be to introduce the dummy function. This would solve
the problem for good (it isn't the first time this happens).

Are you going to submit that patch ?

Thanks,
Guenter

Re: [PATCH -next] net: hisilicon: Never build on SPARC

[David Miller]

From: Guenter Roeck <linux@roeck-us.net>
Date: Wed, 21 Oct 2015 07:29:33 -0700

> The Hisilicon network driver does not build for Sparc. Enabling
> COMPILE_TEST for it causes Sparc allmodconfig/allyesconfig builds
> to fail with
> 
> drivers/net/ethernet/hisilicon/hns_mdio.c: In function 'hns_mdio_bus_name':
> drivers/net/ethernet/hisilicon/hns_mdio.c:409:3: error:
> 		implicit declaration of function 'of_translate_address'
> 
> Fixes: 876133d3161d ("net: hisilicon: add OF dependency")
> Cc: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

I wish we would really resolve this properly instead of hacking crap
like this all the time, it's stupid.

SPARC simply never needs to "translate" OF addresses, since all OF
resources are fully translated already at boot time during OF tree
import.

All IRQs are fully resolved as well.

So we could simply make of_translate_address() a NOP.

Re: [PATCH -next] net: hisilicon: Never build on SPARC

[David Miller]

From: Arnd Bergmann <arnd@arndb.de>
Date: Wed, 21 Oct 2015 16:39:02 +0200

> How about this version?

Yes, that is a million times better.

Re: [PATCH -next] net: hisilicon: Never build on SPARC

[David Miller]

From: Guenter Roeck <linux@roeck-us.net>
Date: Wed, 21 Oct 2015 07:56:18 -0700

>> @@ -57,6 +57,11 @@ extern int of_dma_get_range(struct device_node *np,
>> u64 *dma_addr,
>>   				u64 *paddr, u64 *size);
>>   extern bool of_dma_is_coherent(struct device_node *np);
>>   #else /* CONFIG_OF_ADDRESS */
>> +static inline u64 of_translate_address(struct device_node *np, const
>> __be32 *addr)
>> +{
>> +	return 0;
> 
> Maybe return OF_BAD_ADDR ?

The thing to really do on sparc, is just return the address raw untranslated
because that just works.

Re: [PATCH -next] net: hisilicon: Never build on SPARC

[Arnd Bergmann]

On Wednesday 21 October 2015 08:33:11 David Miller wrote:
> From: Guenter Roeck <linux@roeck-us.net>
> Date: Wed, 21 Oct 2015 07:56:18 -0700
> 
> >> @@ -57,6 +57,11 @@ extern int of_dma_get_range(struct device_node *np,
> >> u64 *dma_addr,
> >>                              u64 *paddr, u64 *size);
> >>   extern bool of_dma_is_coherent(struct device_node *np);
> >>   #else /* CONFIG_OF_ADDRESS */
> >> +static inline u64 of_translate_address(struct device_node *np, const
> >> __be32 *addr)
> >> +{
> >> +    return 0;
> > 
> > Maybe return OF_BAD_ADDR ?
> 
> The thing to really do on sparc, is just return the address raw untranslated
> because that just works.
> 

We still need to check #address-cells, right?

Something like this?

static inline u64 of_translate_address(struct device_node *np, const __be32 *addr)
{
#if defined(CONFIG_SPARC) || defined(CONFIG_M68K)
	int pna = of_n_addr_cells(np);
	u64 ret = be32_to_cpu(addr[pna - 1]);

	if (pna > 1)
		ret += (u64)be32_to_cpu(addr[pna - 2]) << 32;

	return ret;
#else
	return OF_BAD_ADDR;
#endif
}

	Arnd

Re: [PATCH -next] net: hisilicon: Never build on SPARC

[Guenter Roeck]

On 10/21/2015 08:57 AM, Arnd Bergmann wrote:
> On Wednesday 21 October 2015 08:33:11 David Miller wrote:
>> From: Guenter Roeck <linux@roeck-us.net>
>> Date: Wed, 21 Oct 2015 07:56:18 -0700
>>
>>>> @@ -57,6 +57,11 @@ extern int of_dma_get_range(struct device_node *np,
>>>> u64 *dma_addr,
>>>>                               u64 *paddr, u64 *size);
>>>>    extern bool of_dma_is_coherent(struct device_node *np);
>>>>    #else /* CONFIG_OF_ADDRESS */
>>>> +static inline u64 of_translate_address(struct device_node *np, const
>>>> __be32 *addr)
>>>> +{
>>>> +    return 0;
>>>
>>> Maybe return OF_BAD_ADDR ?
>>
>> The thing to really do on sparc, is just return the address raw untranslated
>> because that just works.
>>
>
> We still need to check #address-cells, right?
>
> Something like this?
>
> static inline u64 of_translate_address(struct device_node *np, const __be32 *addr)
> {
> #if defined(CONFIG_SPARC) || defined(CONFIG_M68K)
> 	int pna = of_n_addr_cells(np);
> 	u64 ret = be32_to_cpu(addr[pna - 1]);
>
> 	if (pna > 1)
> 		ret += (u64)be32_to_cpu(addr[pna - 2]) << 32;
>
> 	return ret;

That suggests that sparc would need a translation after all, which
seems to contradict what David said earlier.

Anyway, if it gets that complicated, I think we should stick with
just returning OF_BAD_ADDR. The above really suggests the need for
an architecture specific solution.

Guenter

> #else
> 	return OF_BAD_ADDR;
> #endif
> }
>
> 	Arnd
>

Re: [PATCH -next] net: hisilicon: Never build on SPARC

[Arnd Bergmann]

On Wednesday 21 October 2015 10:03:05 Guenter Roeck wrote:
> >
> > Something like this?
> >
> > static inline u64 of_translate_address(struct device_node *np, const __be32 *addr)
> > {
> > #if defined(CONFIG_SPARC) || defined(CONFIG_M68K)
> >       int pna = of_n_addr_cells(np);
> >       u64 ret = be32_to_cpu(addr[pna - 1]);
> >
> >       if (pna > 1)
> >               ret += (u64)be32_to_cpu(addr[pna - 2]) << 32;
> >
> >       return ret;
> 
> That suggests that sparc would need a translation after all, which
> seems to contradict what David said earlier.

No, not a translation: the value is used without any offset that
factors in the location of the bus, the above is just the shortest
possible way to read the 64-bit number from a big-endian property
of variable length.

> Anyway, if it gets that complicated, I think we should stick with
> just returning OF_BAD_ADDR. The above really suggests the need for
> an architecture specific solution.

Probably no harm in this really: the far more common
of_address_to_resource() and of_iomap() helpers are equally
broken on SPARC and we just return a runtime error for those
as well without CONFIG_OF_ADDRESS rather than breaking the build.

	Arnd

Re: [PATCH -next] net: hisilicon: Never build on SPARC

[Guenter Roeck]

On Wed, Oct 21, 2015 at 09:11:53PM +0200, Arnd Bergmann wrote:
> On Wednesday 21 October 2015 10:03:05 Guenter Roeck wrote:
> > >
> > > Something like this?
> > >
> > > static inline u64 of_translate_address(struct device_node *np, const __be32 *addr)
> > > {
> > > #if defined(CONFIG_SPARC) || defined(CONFIG_M68K)
> > >       int pna = of_n_addr_cells(np);
> > >       u64 ret = be32_to_cpu(addr[pna - 1]);
> > >
> > >       if (pna > 1)
> > >               ret += (u64)be32_to_cpu(addr[pna - 2]) << 32;
> > >
> > >       return ret;
> > 
> > That suggests that sparc would need a translation after all, which
> > seems to contradict what David said earlier.
> 
> No, not a translation: the value is used without any offset that
> factors in the location of the bus, the above is just the shortest
> possible way to read the 64-bit number from a big-endian property
> of variable length.
> 
Out of my realm .. David would have to comment on that.

> > Anyway, if it gets that complicated, I think we should stick with
> > just returning OF_BAD_ADDR. The above really suggests the need for
> > an architecture specific solution.
> 
> Probably no harm in this really: the far more common
> of_address_to_resource() and of_iomap() helpers are equally
> broken on SPARC and we just return a runtime error for those
> as well without CONFIG_OF_ADDRESS rather than breaking the build.
> 
Agreed. Given this, returning OF_BAD_ADDR sounds like a better choice.

Thanks,
Guenter

Re: [PATCH -next] net: hisilicon: Never build on SPARC

[David Miller]

From: Guenter Roeck <linux@roeck-us.net>
Date: Wed, 21 Oct 2015 10:03:05 -0700

> On 10/21/2015 08:57 AM, Arnd Bergmann wrote:
>> On Wednesday 21 October 2015 08:33:11 David Miller wrote:
>>> From: Guenter Roeck <linux@roeck-us.net>
>>> Date: Wed, 21 Oct 2015 07:56:18 -0700
>>>
>>>>> @@ -57,6 +57,11 @@ extern int of_dma_get_range(struct device_node *np,
>>>>> u64 *dma_addr,
>>>>>                               u64 *paddr, u64 *size);
>>>>>    extern bool of_dma_is_coherent(struct device_node *np);
>>>>>    #else /* CONFIG_OF_ADDRESS */
>>>>> +static inline u64 of_translate_address(struct device_node *np, const
>>>>> __be32 *addr)
>>>>> +{
>>>>> +    return 0;
>>>>
>>>> Maybe return OF_BAD_ADDR ?
>>>
>>> The thing to really do on sparc, is just return the address raw
>>> untranslated
>>> because that just works.
>>>
>>
>> We still need to check #address-cells, right?
>>
>> Something like this?
>>
>> static inline u64 of_translate_address(struct device_node *np, const
>> __be32 *addr)
>> {
>> #if defined(CONFIG_SPARC) || defined(CONFIG_M68K)
>> 	int pna = of_n_addr_cells(np);
>> 	u64 ret = be32_to_cpu(addr[pna - 1]);
>>
>> 	if (pna > 1)
>> 		ret += (u64)be32_to_cpu(addr[pna - 2]) << 32;
>>
>> 	return ret;
> 
> That suggests that sparc would need a translation after all, which
> seems to contradict what David said earlier.

It's not being translated, the code above is just figuring out what size
the object in the property is.

Re: [PATCH -next] net: hisilicon: Never build on SPARC

[Guenter Roeck]

Arnd,

On Wed, Oct 21, 2015 at 02:53:20PM -0700, Guenter Roeck wrote:
> On Wed, Oct 21, 2015 at 09:11:53PM +0200, Arnd Bergmann wrote:
> > On Wednesday 21 October 2015 10:03:05 Guenter Roeck wrote:
> > > >
> > > > Something like this?
> > > >
> > > > static inline u64 of_translate_address(struct device_node *np, const __be32 *addr)
> > > > {
> > > > #if defined(CONFIG_SPARC) || defined(CONFIG_M68K)
> > > >       int pna = of_n_addr_cells(np);
> > > >       u64 ret = be32_to_cpu(addr[pna - 1]);
> > > >
> > > >       if (pna > 1)
> > > >               ret += (u64)be32_to_cpu(addr[pna - 2]) << 32;
> > > >
> > > >       return ret;
> > > 
> > > That suggests that sparc would need a translation after all, which
> > > seems to contradict what David said earlier.
> > 
> > No, not a translation: the value is used without any offset that
> > factors in the location of the bus, the above is just the shortest
> > possible way to read the 64-bit number from a big-endian property
> > of variable length.
> > 
> Out of my realm .. David would have to comment on that.
> 
> > > Anyway, if it gets that complicated, I think we should stick with
> > > just returning OF_BAD_ADDR. The above really suggests the need for
> > > an architecture specific solution.
> > 
> > Probably no harm in this really: the far more common
> > of_address_to_resource() and of_iomap() helpers are equally
> > broken on SPARC and we just return a runtime error for those
> > as well without CONFIG_OF_ADDRESS rather than breaking the build.
> > 
> Agreed. Given this, returning OF_BAD_ADDR sounds like a better choice.
> 
Arnd,

do you know if a fix for this problem is pending in some branch ?
Mainline sparc builds are now affected.

Thanks,
Guenter

Re: [PATCH -next] net: hisilicon: Never build on SPARC

[Arnd Bergmann]

On Friday 06 November 2015 11:16:52 Guenter Roeck wrote:
> On Wed, Oct 21, 2015 at 02:53:20PM -0700, Guenter Roeck wrote:
> > On Wed, Oct 21, 2015 at 09:11:53PM +0200, Arnd Bergmann wrote:
> > > On Wednesday 21 October 2015 10:03:05 Guenter Roeck wrote:
> > > > Anyway, if it gets that complicated, I think we should stick with
> > > > just returning OF_BAD_ADDR. The above really suggests the need for
> > > > an architecture specific solution.
> > > 
> > > Probably no harm in this really: the far more common
> > > of_address_to_resource() and of_iomap() helpers are equally
> > > broken on SPARC and we just return a runtime error for those
> > > as well without CONFIG_OF_ADDRESS rather than breaking the build.
> > > 
> > Agreed. Given this, returning OF_BAD_ADDR sounds like a better choice.
> > 
> Arnd,
> 
> do you know if a fix for this problem is pending in some branch ?
> Mainline sparc builds are now affected.
> 

I don't think anyone wrote the patch to do this. Can you send one?

	Arnd

Re: [PATCH -next] net: hisilicon: Never build on SPARC

[Guenter Roeck]

On 11/06/2015 12:30 PM, Arnd Bergmann wrote:
> On Friday 06 November 2015 11:16:52 Guenter Roeck wrote:
>> On Wed, Oct 21, 2015 at 02:53:20PM -0700, Guenter Roeck wrote:
>>> On Wed, Oct 21, 2015 at 09:11:53PM +0200, Arnd Bergmann wrote:
>>>> On Wednesday 21 October 2015 10:03:05 Guenter Roeck wrote:
>>>>> Anyway, if it gets that complicated, I think we should stick with
>>>>> just returning OF_BAD_ADDR. The above really suggests the need for
>>>>> an architecture specific solution.
>>>>
>>>> Probably no harm in this really: the far more common
>>>> of_address_to_resource() and of_iomap() helpers are equally
>>>> broken on SPARC and we just return a runtime error for those
>>>> as well without CONFIG_OF_ADDRESS rather than breaking the build.
>>>>
>>> Agreed. Given this, returning OF_BAD_ADDR sounds like a better choice.
>>>
>> Arnd,
>>
>> do you know if a fix for this problem is pending in some branch ?
>> Mainline sparc builds are now affected.
>>
>
> I don't think anyone wrote the patch to do this. Can you send one?
>

I'll see what I can do.

Guenter