Skip site navigation (1)Skip section navigation (2)
Date:      Thu, 17 Dec 2015 09:11:54 -0800
From:      Nathan Whitehorn <nwhitehorn@freebsd.org>
To:        Andrew Turner <andrew@freebsd.org>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org
Subject:   Re: svn commit: r292405 - in head/sys: arm64/include dev/ofw
Message-ID:  <5672ECDA.80101@freebsd.org>
In-Reply-To: <201512171700.tBHH04QC021609@repo.freebsd.org>
References:  <201512171700.tBHH04QC021609@repo.freebsd.org>

next in thread | previous in thread | raw e-mail | index | archive | help
I don't really like the #ifdef here. If it contradicts ePAPR, switching 
on the behavior through a #define OFW_EPAPR is at best misleading. And, 
aside from the default value of 2, this *is* compliant with the ePAPR 
spec, as well as with CHRP and PAPR (which is where ePAPR got it from). 
Do we have any in-the-wild CHRP-ish device trees for which 
#address-cells is unspecified to even worry about the default anyway?

In any event, I'd much prefer that it be explicitly disabled on SPARC, 
which is the odd man out on our platforms, rather than opt-in on all 
other platforms, which use CHRP-heritage bindings, and that the #define 
be something more descriptive/correct like #define 
OFW_IMAP_NO_INTERRUPT_PARENT.
-Nathan

On 12/17/15 09:00, Andrew Turner wrote:
> Author: andrew
> Date: Thu Dec 17 17:00:04 2015
> New Revision: 292405
> URL: https://svnweb.freebsd.org/changeset/base/292405
>
> Log:
>    Support the variant of the interrupt-map property where the parent bus has
>    the #address-cells property set. For this we need to read more data before
>    the parent interrupt description.
>    
>    this is only enabled on arm64 for now as it's not quite compliant with the
>    ePAPR spec. We should use a default of 2 where the #address-cells property
>    is missing, however this will need further testing across architectures.
>    
>    Obtained from:	ABT Systems Ltd
>    Sponsored by:	SoftIron Inc
>    Differential Revision:	https://reviews.freebsd.org/D4518
>
> Modified:
>    head/sys/arm64/include/ofw_machdep.h
>    head/sys/dev/ofw/ofw_bus_subr.c
>
> Modified: head/sys/arm64/include/ofw_machdep.h
> ==============================================================================
> --- head/sys/arm64/include/ofw_machdep.h	Thu Dec 17 16:09:15 2015	(r292404)
> +++ head/sys/arm64/include/ofw_machdep.h	Thu Dec 17 17:00:04 2015	(r292405)
> @@ -41,4 +41,7 @@ struct mem_region {
>   	vm_size_t	mr_size;
>   };
>   
> +/* FDT follows ePAPR */
> +#define	OFW_EPAPR
> +
>   #endif /* _MACHINE_OFW_MACHDEP_H_ */
>
> Modified: head/sys/dev/ofw/ofw_bus_subr.c
> ==============================================================================
> --- head/sys/dev/ofw/ofw_bus_subr.c	Thu Dec 17 16:09:15 2015	(r292404)
> +++ head/sys/dev/ofw/ofw_bus_subr.c	Thu Dec 17 17:00:04 2015	(r292405)
> @@ -341,6 +341,7 @@ ofw_bus_search_intrmap(void *intr, int i
>   	uint8_t *uiregs = regs;
>   	uint8_t *uiimapmsk = imapmsk;
>   	uint8_t *mptr;
> +	pcell_t paddrsz;
>   	pcell_t pintrsz;
>   	int i, rsz, tsz;
>   
> @@ -357,19 +358,31 @@ ofw_bus_search_intrmap(void *intr, int i
>   
>   	mptr = imap;
>   	i = imapsz;
> +	paddrsz = 0;
>   	while (i > 0) {
>   		bcopy(mptr + physsz + intrsz, &parent, sizeof(parent));
> +#ifdef OFW_EPAPR
> +		/*
> +		 * Find if we need to read the parent address data. Sparc64
> +		 * uses a different encoding that doesn't include this data.
> +		 */
> +		if (OF_getencprop(OF_node_from_xref(parent),
> +		    "#address-cells", &paddrsz, sizeof(paddrsz)) == -1)
> +			paddrsz = 0;	/* default */
> +		paddrsz *= sizeof(pcell_t);
> +#endif
> +
>   		if (OF_searchencprop(OF_node_from_xref(parent),
>   		    "#interrupt-cells", &pintrsz, sizeof(pintrsz)) == -1)
>   			pintrsz = 1;	/* default */
>   		pintrsz *= sizeof(pcell_t);
>   
>   		/* Compute the map stride size. */
> -		tsz = physsz + intrsz + sizeof(phandle_t) + pintrsz;
> +		tsz = physsz + intrsz + sizeof(phandle_t) + paddrsz + pintrsz;
>   		KASSERT(i >= tsz, ("ofw_bus_search_intrmap: truncated map"));
>   
>   		if (bcmp(ref, mptr, physsz + intrsz) == 0) {
> -			bcopy(mptr + physsz + intrsz + sizeof(parent),
> +			bcopy(mptr + physsz + intrsz + sizeof(parent) + paddrsz,
>   			    result, MIN(rintrsz, pintrsz));
>   
>   			if (iparent != NULL)
>




Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?5672ECDA.80101>