[PATCH v2] Device tree bindings for Xilinx devices

[Stephen Neuendorffer]

> -----Original Message-----
> From: glikely at secretlab.ca [mailto:glikely at secretlab.ca] On 
> Behalf Of Grant Likely
> Sent: Tuesday, October 16, 2007 12:36 PM
> To: Stephen Neuendorffer
> Cc: linuxppc-dev at ozlabs.org; Wolfgang Reissnegger; Leonid; 
> microblaze-uclinux at itee.uq.edu.au
> Subject: Re: [PATCH v2] Device tree bindings for Xilinx devices
> 
> On 10/16/07, Stephen Neuendorffer 
> <stephen.neuendorffer at xilinx.com> wrote:
> >
> > > +   n) Xilinx EMAC and Xilinx TEMAC
> > > +
> > > +   Xilinx Ethernet devices.  Uses common properties from
> > > other Ethernet
> > > +   devices with the following constraints:
> > > +
> > > +   Required properties:
> > > +    - compatible : Must include one of: "xilinx,plb-temac",
> > > +                   "xilinx,plb-emac", "xilinx-opb-emac"
> > > +    - dma-mode : Must be one of "none", "simple", "sg" (sg
> > > == scatter gather)
> >
> > I think it's going to be a significant headache to remap 
> things like the
> > dma-mode from the xilinx configurations to something else, and then
> > interpret them correctly in the drivers.
> >
> > Although it lacks a bit in style, perhaps, I'd greatly prefer having
> > something like:
> >
> >         Ethernet_MAC {
> >                 xilinx,C_DMA_PRESENT = <1>;
> >             ...
> >       }
> >
> > (which happens to correspond to "none" above)
> 
> Ugh.  Can't say I'm thrilled about this....
> 
> But in this case is might be a necessity.  The IP core is already
> parameterized and the best way to describe the device is to use the
> existing parameter names.

I don't really like it either, expecially after thinking about it more.
The problem is that a single IP core might correspond to multiple
logical devices.  The parameters actually need to be from the point of
view of the drivers. That is, they need to be names like in the
xparameters:

	xilinx,DMA_PRESENT = <1>;

In this case, the translation looks simple, but it is currently
implemented by a bunch of gnarly .tcl code.  (Don't ask... :)  In the
future, this *could* be simple code, but in the short term, it won't be
any simpler than having pretty names.  Furthermore, the best driver code
that I can come up with for using this essentially looks like:

static int __devinit xenet_of_probe(struct of_device *ofdev, const
struct of_device_id *match)
{
	struct xemac_platform_data pdata_struct;
	struct resource r_irq_struct;
	struct resource r_mem_struct;

	struct resource *r_irq = &r_irq_struct;	/* Interrupt resources
*/
	struct resource *r_mem = &r_mem_struct;	/* IO mem resources */
	struct xemac_platform_data *pdata = &pdata_struct;
	int rc = 0;

	printk(KERN_ERR "Device Tree Probing \'%s\'\n",
                        ofdev->node->name);

	/* Get iospace for the device */
	rc = of_address_to_resource(ofdev->node, 0, r_mem);
	if(rc) {
		dev_warn(&ofdev->dev, "invalid address\n");
		return rc;
	}

	/* Get IRQ for the device */
	rc = of_irq_to_resource(ofdev->node, 0, r_irq);
	if(rc == NO_IRQ) {
		dev_warn(&ofdev->dev, "no IRQ found.\n");
		return rc;
	}

	pdata_struct.dma_mode           = get_u32(ofdev,
"C_DMA_PRESENT");
	pdata_struct.has_mii		= get_u32(ofdev, "C_MII_EXIST");
	pdata_struct.has_cam		= get_u32(ofdev, "C_CAM_EXIST");
	pdata_struct.has_err_cnt	= get_u32(ofdev,
"C_ERR_COUNT_EXIST");
	pdata_struct.has_jumbo		= get_u32(ofdev,
"C_JUMBO_EXIST");
	pdata_struct.tx_dre		= get_u32(ofdev,
"C_TX_DRE_TYPE");
	pdata_struct.rx_dre		= get_u32(ofdev,
"C_RX_DRE_TYPE");
	pdata_struct.tx_hw_csum		= get_u32(ofdev,
"C_TX_INCLUDE_CSUM");
	pdata_struct.rx_hw_csum		= get_u32(ofdev,
"C_RX_INCLUDE_CSUM");
	memcpy(pdata_struct.mac_addr, of_get_mac_address(ofdev->node),
6);

        return probe_initialization(&ofdev->dev, r_mem, r_irq, pdata);
}

Where probe_initialization contains shares code with the platform_device
version of probe.  This means that the string is still translated into
another name anyway.  Anybody have any better ideas for doing this?

> 
> I want to be careful though.  Parameters can change from one version
> of an IP core to another.  The driver needs to be able to determine
> what version of the IP core is used so that the parameters make sense.
>  I'm also nervous about adding every possible C_ value to the device
> node for each xilinx IP-core; that could make the device tree quite
> large.  (on the other hand; maybe that doesn't matter... It is
> probably a bigger deal for microblaze than powerpc too.).

Using the xparameters set of values could solve this problem, since only
things that make sense to the driver are actually set.

Steve

> 
> Cheers,
> g.
> 
> -- 
> Grant Likely, B.Sc., P.Eng.
> Secret Lab Technologies Ltd.
> grant.likely at secretlab.ca
> (403) 399-0195
> 
>