Re: [PATCH v2] Device tree bindings for Xilinx devices

["Grant Likely" <grant.likely@secretlab.ca>]

On 10/16/07, Grant Likely <grant.likely@secretlab.ca> wrote:
> On 10/16/07, Stephen Neuendorffer <stephen.neuendorffer@xilinx.com> wrote:
> >
> >
> > > -----Original Message-----
> > > From: glikely@secretlab.ca [mailto:glikely@secretlab.ca] On
> > > Behalf Of Grant Likely
> > > Sent: Tuesday, October 16, 2007 12:36 PM
> > > To: Stephen Neuendorffer
> > > Cc: linuxppc-dev@ozlabs.org; Wolfgang Reissnegger; Leonid;
> > > microblaze-uclinux@itee.uq.edu.au
> > > Subject: Re: [PATCH v2] Device tree bindings for Xilinx devices
> > >
> > > On 10/16/07, Stephen Neuendorffer
> > > <stephen.neuendorffer@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>;
>
> Hmm, not bad.  It does still get a little harry when one IP core
> implements multiple logical devices, but maybe the IP core should
> create a node and the logical devices can be sub nodes with the
> appropriate properties needed to describe which logical device it is.
>
> >
> > 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?
>
> That's pretty close to what I've done on the sysace, uartlite and
> xilinxfb drivers.
>
> > > 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.
>
> I've thought about this more since I wrote the above paragraph.  The
> compatible property was designed to solve that exact problem.  We'll
> just embed the version in the compatible list.

And, yes, you're right.  Using the xparams value set is probably the
place to start for choosing which properties to put in the device
tree.  I'll document this stuff up and respin my patch.

Cheers,
g.

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