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

[Benjamin Herrenschmidt]

> >  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?

I'll double check, I think we don't have it on all blade types tho.

> > +#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?

Paulus ? I haven't followed the latest discussions about that. It makes
sense to have arch/powerpc in the default include path though, I agree
so if #include <platform/*> works and will still work tomorrow, then I'm
happy to update the driver.

> > +struct cell_edac_priv
> > +{
> 
> Sorry to be picky today, but shouldn't that be
>
> struct cell_edac_priv {

Heh, allright :-)

> 
> > +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.

At this stage, I'm not sure how... at this stage, "public" Cell only
supports XDR, so I'm happy to keep it that way and maybe push a fixup if
we come to agree on a way (device-tree ?) to ignore possible future
implementations that would use something else...

Ben.