[Cbe-oss-dev] [PATCH 2/2] edac: Add Cell memory controller

[Arnd Bergmann]

On Wednesday 03 October 2007, Benjamin Herrenschmidt wrote:

> Adds driver for the Cell memory controller when used without a
> Hypervisor such as on the IBM Cell blades. There might still
> be some improvements to do to this such as finding if it's
> possible to properly obtain more details about the address
> of the error but it's good enough already to report CE counts
> which is our main priority at the moment.

Thanks for this driver, we should definitely get that into 2.6.24.

> @@ -83,12 +83,22 @@ static void cell_progress(char *s, unsig
>  
>  static int __init cell_publish_devices(void)
>  {
> +	int node;
> +
>  	if (!machine_is(cell))
>  		return 0;
>  
>  	/* Publish OF platform devices for southbridge IOs */
>  	of_platform_bus_probe(NULL, NULL, NULL);
>  
> +	/* There is no device for the MIC memory controller, thus we create
> +	 * a platform device for it to attach the EDAC driver to.
> +	 */
> +	for_each_online_node(node) {
> +		if (cbe_get_cpu_mic_tm_regs(cbe_node_to_cpu(node)) == NULL)
> +			continue;
> +		platform_device_register_simple("cbe-mic", node, NULL, 0);
> +	}
>  	return 0;
>  }

In cbe_fill_regs_map(), we look for a device of type "mic-tm", which makes
it look like there is indeed a device we could use. Should the platform_device
perhaps be a child of that node?

> +#include <../arch/powerpc/platforms/cell/cbe_regs.h>

I see a mixture of this style and '#include <platforms/cell/cbe_regs.h>' in
powerpc code, with the ../../.. stuff looking really ugly to me. Can 
we get a consensus of what we want to use that consistently?

> +struct cell_edac_priv
> +{

Sorry to be picky today, but shouldn't that be

struct cell_edac_priv {

> +static void __devinit cell_edac_init_csrows(struct mem_ctl_info *mci)
> +{
> +	struct csrow_info		*csrow = &mci->csrows[0];
> +	struct cell_edac_priv		*priv = mci->pvt_info;
> +	struct device_node		*np;
> +
> +	for (np = NULL;
> +	     (np = of_find_node_by_name(np, "memory")) != NULL;) {
> +		struct resource r;
> +
> +		/* We "know" that the Cell firmware only creates one entry
> +		 * in the "memory" nodes. If that changes, this code will
> +		 * need to be adapted.
> +		 */
> +		if (of_address_to_resource(np, 0, &r))
> +			continue;
> +		if (of_node_to_nid(np) != priv->node)
> +			continue;
> +		csrow->first_page = r.start >> PAGE_SHIFT;
> +		csrow->nr_pages = (r.end - r.start + 1) >> PAGE_SHIFT;
> +		csrow->last_page = csrow->first_page + csrow->nr_pages - 1;
> +		csrow->mtype = MEM_XDR;
> +		csrow->edac_mode = EDAC_FLAG_EC | EDAC_FLAG_SECDED;

Should we have a way to detect the memory technology here? Sure, we can only
connect XDR memory to the chip, but it feels like a bogus assumption anyway
because the registers you are touching do not seem to be XDR specific.

	Arnd <><