On Jan 12, 2005, at 2:36 AM, Eugene Surovegin wrote:
> On Wed, Jan 12, 2005 at 01:43:09AM -0600, Kumar Gala wrote:
> >
> > Please take a look at the following patch.? It adds driver model
> support
> > via platform devices to 85xx.? This is originally based on patches
> from
> > Jason M.
> >
> > The idea behind the code is that for a give family: 4xx, 8xx, 82xx,
> 83xx,
> > 85xx, 86xx we will have structure defns for the following:
> >
> > enum ppc_soc_devices
> > in asm-ppc/<family.h>:
> >?? list of all unique devices in the family
> >
> > struct platform_device soc_platform_devices[]
> > in arch/ppc/platforms/<family>/<family>_devices.c:
> >?? describes all platform devices that exist in the family
> >
> > struct soc_spec soc_specs[]
> > in arch/ppc/platforms/<family>/<family>_soc.c:
> >?? describes each unique chip in the family and what devices it has
>
> Well, there is a problem right here at least for 4xx.
> Current OCP implementation is much more flexible IMHO.
>
> For 4xx is not uncommon when you have the same "logical" device at the
> different places with different "properties" (e.h. different channel,
> etc).
>
> Your case (85xx) looks simpler - all you need is a list of devices
> which particular SoC supports, without significant differences in
> "properties". This will not work that easy for 4xx.
>
> In fact, I don't see any gain (at least for 4xx) in all these changes.
> It makes 4xx much more tangled IMHO, because we'll still need all
> those ibm405gp.c, ibm405ep.c, ibm440gp.c etc.
>
> Please note, using platform_device is orthogonal to the way we
> describe each SoC (this is what I don't quite like in your patch), and
> I don't have any strong objections to using platform_device instead of
> OCP or feature_call or whatever for communication with device drivers.
I need to understand a bit more about how 4xx does things. When I
started the SOC stuff, it was freescale specific. I agree that its
orthogonal to the use of platform device. Does this some like a bad
idea for the fsl case?
> > Plus the following functions:
> >
> > identify_soc_by_id() -- determine soc by an int id
> > identify_soc_by_name() -- determin soc by name (useful in some 82xx
> cases)
> > ppc_soc_get_pdata() -- get platform_data pointer so board code can
> modify
> > ppc_soc_update_paddr() -- update iomem resources with a given paddr
>
> IMHO, ppc_soc_update_paddr - is a very confusing name, in fact, from
> first read I though it _changes_ paddr to the new value, not _adds_ it
> :)
>
> Probably this function should be made 85xx specific as I cannot come
> up with any use for it in 4xx (we don't have anything similar to
> CCSRBAR ;).
Not an issue, any suggestions on renaming to make it clear what it
does? Also, I can make this fsl_increment_paddr() since its useful to
more than on family of fsl processors.
> [snip]
>
> > +
> > +struct platform_device soc_platform_devices[] = {
> > +???? [MPC85xx_TSEC1] = {
> > +???? ??????? .name = "fsl-gianfar",
> > +???? ??????? .id???? = 1,
> > +???? ??????? .dev.platform_data = &mpc85xx_tsec1_pdata,
> > +???? ??????? .num_resources?? = 4,
> > +???? ??????? .resource = (struct resource[]) {
> > +???? ??????? ??????? {
> > +???? ??????? ??????? ??????? .start? = MPC85xx_ENET1_OFFSET,
> > +???? ??????? ??????? ??????? .end??? = MPC85xx_ENET1_OFFSET +
> > +???? ??????? ??????? ??????? ??????? ??????? MPC85xx_ENET1_SIZE -
> 1,
> > +???? ??????? ??????? ??????? .flags? = IORESOURCE_MEM,
> > +???? ??????? ??????? },
> > +???? ??????? ??????? {
> > +???? ??????? ??????? ??????? .name?? = "tx",
> > +???? ??????? ??????? ??????? .start? = MPC85xx_IRQ_TSEC1_TX,
> > +???? ??????? ??????? ??????? .end??? = MPC85xx_IRQ_TSEC1_TX,
> > +???? ??????? ??????? ??????? .flags? = IORESOURCE_IRQ,
> > +???? ??????? ??????? },
> > +???? ??????? ??????? {
> > +???? ??????? ??????? ??????? .name?? = "rx",
> > +???? ??????? ??????? ??????? .start? = MPC85xx_IRQ_TSEC1_RX,
> > +???? ??????? ??????? ??????? .end??? = MPC85xx_IRQ_TSEC1_RX,
> > +???? ??????? ??????? ??????? .flags? = IORESOURCE_IRQ,
> > +???? ??????? ??????? },
> > +???? ??????? ??????? {
> > +???? ??????? ??????? ??????? .name?? = "error",
> > +???? ??????? ??????? ??????? .start? = MPC85xx_IRQ_TSEC1_ERROR,
> > +???? ??????? ??????? ??????? .end??? = MPC85xx_IRQ_TSEC1_ERROR,
> > +???? ??????? ??????? ??????? .flags? = IORESOURCE_IRQ,
> > +???? ??????? ??????? },
> [snip]
>
>
>
> I already wrote about this but repeat again :(.
>
> Why put all these defines (e.g. for memory regions) into header when
> the only user is this particular file. It doesn't help readability and
> only obfuscates sources (and 4xx is a very good example of such mess
> :)
Understood, I forgot about this. I've got now issue with changing it,
just trying to minimize changes.
thanks
- kumar
