Re: [PATCH 1/4] drivers/bus: Added Freescale Management Complex APIs

[Scott Wood <scottwood@freescale.com>]

On Mon, 2014-09-15 at 18:44 -0500, Kim Phillips wrote:
> On Thu, 11 Sep 2014 12:34:21 -0500
> "J. German Rivera" <German.Rivera@freescale.com> wrote:
> 
> > +int mc_get_version(struct fsl_mc_io *mc_io, struct mc_version *mc_ver_info)
> > +{
> > +	struct mc_command cmd = { 0 };
> 
> we can save some cycles if this initialization is not absolutely
> necessary: is it?  i.e., does the h/w actually look at the params
> section when doing a get_version?  not sure to what other commands
> this comment would apply to...at least get_container_id, but maybe
> more - all of them?

Do you really want to open that can of worms, much less to speed up
something that doesn't look performance critical?  Have fun debugging it
if it turns out the hardware does look at something you didn't
initialize.

> > diff --git a/drivers/bus/fsl-mc/fsl_mc_sys.c b/drivers/bus/fsl-mc/fsl_mc_sys.c
> 
> > +/**
> > + * Map an MC portal in the kernel virtual address space
> > + */
> > +static int map_mc_portal(phys_addr_t mc_portal_phys_addr,
> > +			 uint32_t mc_portal_size,
> > +			 void __iomem **new_mc_portal_virt_addr)
> > +{
> > +	void __iomem *mc_portal_virt_addr = NULL;
> > +	struct resource *res = NULL;
> > +	int error = -EINVAL;
> > +
> > +	res =
> > +	    request_mem_region(mc_portal_phys_addr, mc_portal_size,
> > +			       "mc_portal");
> > +	if (res == NULL) {
> > +		pr_err("request_mem_region() failed for MC portal %#llx\n",
> > +		       mc_portal_phys_addr);
> > +		error = -EBUSY;
> > +		goto error;
> > +	}
> > +
> > +	mc_portal_virt_addr = ioremap_nocache(mc_portal_phys_addr,
> > +					      mc_portal_size);
> > +	if (mc_portal_virt_addr == NULL) {
> > +		pr_err("ioremap_nocache() failed for MC portal %#llx\n",
> > +		       mc_portal_phys_addr);
> > +		error = -EFAULT;
> > +		goto error;
> > +	}
> > +
> > +	*new_mc_portal_virt_addr = mc_portal_virt_addr;
> > +	return 0;
> > +error:
> > +	if (mc_portal_virt_addr != NULL)
> > +		iounmap(mc_portal_virt_addr);
> > +
> > +	if (res != NULL)
> > +		release_mem_region(mc_portal_phys_addr, mc_portal_size);
> > +
> > +	return error;
> > +}
> 
> unnecessary initializations, bad error codes (both should be
> -ENOMEM),

Why should the first one be -ENOMEM?  It's not allocating memory, but
rather reserving I/O space.

>  unnecessarily complicated error path, plus a simpler
> implementation can be made if fn can return the mapped address, like
> so:
> 
> static void __iomem *map_mc_portal(phys_addr_t mc_portal_phys_addr,
>                                    uint32_t mc_portal_size)
> { 
>         struct resource *res;
>         void __iomem *mapped_addr;
> 
>         res = request_mem_region(mc_portal_phys_addr, mc_portal_size, 
>                                  "mc_portal");
>         if (!res) 
>                 return NULL;
> 
>         mapped_addr = ioremap_nocache(mc_portal_phys_addr,
>                                       mc_portal_size);
>         if (!mapped_addr)
>                 release_mem_region(mc_portal_phys_addr, mc_portal_size);
> 
>         return mapped_addr;
> }
> 
> the callsite can return -ENOMEM to its caller if returned NULL.

-ENOMEM would only be appropriate for one of these errors.

> > +#define ioread64(_p)	    readq(_p)
> > +#define iowrite64(_v, _p)   writeq(_v, _p)
> 
> these definitions have names that are too generic to belong in a FSL
> h/w header: conflicts will be introduced once the existing
> io{read,write}32 functions get promoted.  Either use readq/writeq
> directly, or, if you can justify it, patch a more generic io.h.
> 
> Also, is there a reason the 'relaxed' versions of the i/o accessors
> aren't being used?

Raw accessors should only be used in performance critical sections where
it's worth the effort to implement and verify manual synchronization.
My understanding is that the entire management complex is related to
setup, not on the I/O fast path.

-Scott