Re: [coreboot] [poll] device_t
On Fri, May 8, 2015 at 12:16 PM, Patrick Georgi wrote: > 2015-05-08 17:31 GMT+02:00 Aaron Durbin : >> In romstage *both* struct device and device_t are present but are >> completely different types. It's the romstage, ramstage, and smm >> overlap that is large and extremely annoying to deal with. > History time: > Until not too long ago, bootblock and romstage used u32 device_t, > ramstage used struct device, smm didn't really matter and nothing else > existed. > This was further enforced by struct device being available in ramstage > _only_. We now have a read-only view of the device tree available in > romstage that makes things look more complicated. > > One of the ideas that led to the devicetree in romstage was to make > struct device the default interface where possible, together with the > aforementioned isolation of romcc based code. Essentially: for romcc, > use u32. For everything else, struct device. > > We may want to determine first what data structure we want to use in > each stage. This could be 'u32 for all hardware accesses, with some > struct device -> u32 conversion to move from devicetree to hardware', > which sounds like what you propose (and is completely contrary to that > other idea). No. I was suggesting doing a struct device lookup from a u32 one. Anyway, I think it's constructive to look at use cases. The struct device code assumes that one has a reference to the struct device in ramstage by way the state machine acting on each device. But then we have to add lookup routines because that's not always the valid use case. Even though struct device is there in romstage it's not used at all for IO (as you mentioned the plan was never realized). A lot of the juggling of types comes from the fact that there are statically known pci devices, and one wants to talk to them outside of the ramstage boot state machine. romstage has no concept nor a way to get at each device w/o doing the tree walk. That coupled with IO routines only operating on u32 devices makes one do the necessary back bends. Can at least a short term goal be to remove the crufty handling found in the examples I provided? Those are not the only 2, I'm sure. Does that mean we bring in static.c for all stages? What about the dynamic ops handling currently available in ramstage? Does that apply to all stages? I also wouldn't frame the discussion around the 'fate of device_t'. It's really more about defining the semantics for the IO routines on devices for each stage: how does one get a reference to said device? how does one do that IO? Can the same code be compiled in all stages w/o having to put per-compilation unit c preprocessor switches? -Aaron -- coreboot mailing list: coreboot@coreboot.org http://www.coreboot.org/mailman/listinfo/coreboot
Re: [coreboot] [poll] device_t
2015-05-08 17:31 GMT+02:00 Aaron Durbin : > In romstage *both* struct device and device_t are present but are > completely different types. It's the romstage, ramstage, and smm > overlap that is large and extremely annoying to deal with. History time: Until not too long ago, bootblock and romstage used u32 device_t, ramstage used struct device, smm didn't really matter and nothing else existed. This was further enforced by struct device being available in ramstage _only_. We now have a read-only view of the device tree available in romstage that makes things look more complicated. One of the ideas that led to the devicetree in romstage was to make struct device the default interface where possible, together with the aforementioned isolation of romcc based code. Essentially: for romcc, use u32. For everything else, struct device. We may want to determine first what data structure we want to use in each stage. This could be 'u32 for all hardware accesses, with some struct device -> u32 conversion to move from devicetree to hardware', which sounds like what you propose (and is completely contrary to that other idea). But until there's a consensus on that, I guess it's too early to discuss the fate of device_t. Patrick -- Google Germany GmbH, ABC-Str. 19, 20354 Hamburg Registergericht und -nummer: Hamburg, HRB 86891, Sitz der Gesellschaft: Hamburg Geschäftsführer: Graham Law, Christine Elizabeth Flores -- coreboot mailing list: coreboot@coreboot.org http://www.coreboot.org/mailman/listinfo/coreboot
Re: [coreboot] [poll] device_t
On Fri, May 8, 2015 at 8:31 AM Aaron Durbin wrote: > > I personally feel that changing device_t type based on stage makes the > code non-obvious and hard to follow. > > I'd rather we *always* provide simple u32 device_t functions in all > stages while allowing struct device IO functions for use in ramstage. > > that's pretty much the approach I tried to take in v3. While the device_t stuff is cool it's too hard for my brain. BUT, it's there, been in use for a long time, not sure this is the best time to change it. But The Will of the People will decide. ron -- coreboot mailing list: coreboot@coreboot.org http://www.coreboot.org/mailman/listinfo/coreboot
Re: [coreboot] [poll] device_t
On Thu, May 7, 2015 at 3:42 PM, Patrick Georgi via coreboot wrote: > 2015-05-07 22:00 GMT+02:00 Stefan Reinauer : >> With our current bootblock concept, it is never going away on x86 (for >> bootblock usage) > Which isn't that much of a problem once we provide separate headers > for x86 bootblock code. There's really very tiny overlap. > That could then be reused to deal with raminit on romcc-boards, too: > from coreboot's point of view, raminit is just an overly large > piece of cache-as-ram code, followed by a raminit noop. > This is simplified by the lack of the need for development tools (eg > printk) to develop new non-car x86 raminits. > > I think bootblock code overlap is tiny. However, device_t is more complicated than just bootblock and !bootblock. The io routines differ by stage: romstage and bootblock use the simple u32 device_t ramstage uses the sturct device device_t And lest we not forget SMM on x86 which only has simple u32 device_t. In romstage *both* struct device and device_t are present but are completely different types. It's the romstage, ramstage, and smm overlap that is large and extremely annoying to deal with. A few examples on boiler plate one needs to deal with for code sharing: src/soc/intel/baytrail/pmutil.c -- all the get_pcu_dev() src/soc/intel/baytrail/spi.c So basically while the function names are the same for the IO routines the parameter types change. So every caller of a compilation unit of these IO routines that wants to be shared needs to deal w/ the current type changes. I personally feel that changing device_t type based on stage makes the code non-obvious and hard to follow. I'd rather we *always* provide simple u32 device_t functions in all stages while allowing struct device IO functions for use in ramstage. The simple ones in ramstage can just be a struct device lookup then call appropriate IO function. -Aaron -- coreboot mailing list: coreboot@coreboot.org http://www.coreboot.org/mailman/listinfo/coreboot
Re: [coreboot] [poll] device_t
Oh, yeah, the history of device_t is as old as coreboot v2. It came in ca. 2002 and was part of what made romcc work. It's not a recent or ill-considered change, to say the least. ron -- coreboot mailing list: coreboot@coreboot.org http://www.coreboot.org/mailman/listinfo/coreboot
Re: [coreboot] [poll] device_t
On Thu, May 7, 2015 at 1:42 PM Patrick Georgi wrote: > > > > One of these days... > > well, that's kind of key ... one of these days. Once we came to that happy time it might make sense to make this change, but until then, I think we have to wait. Now I know how to vote. ron -- coreboot mailing list: coreboot@coreboot.org http://www.coreboot.org/mailman/listinfo/coreboot
Re: [coreboot] [poll] device_t
2015-05-07 22:00 GMT+02:00 Stefan Reinauer : > With our current bootblock concept, it is never going away on x86 (for > bootblock usage) Which isn't that much of a problem once we provide separate headers for x86 bootblock code. There's really very tiny overlap. That could then be reused to deal with raminit on romcc-boards, too: from coreboot's point of view, raminit is just an overly large piece of cache-as-ram code, followed by a raminit noop. This is simplified by the lack of the need for development tools (eg printk) to develop new non-car x86 raminits. One of these days... Patrick -- Google Germany GmbH, ABC-Str. 19, 20354 Hamburg Registergericht und -nummer: Hamburg, HRB 86891, Sitz der Gesellschaft: Hamburg Geschäftsführer: Graham Law, Christine Elizabeth Flores -- coreboot mailing list: coreboot@coreboot.org http://www.coreboot.org/mailman/listinfo/coreboot
Re: [coreboot] [poll] device_t
* ron minnich [150507 21:35]: > one counter-question: is romcc ever going away, or at least is the usage > gong to change such that no code would ever use the uint32 version of > device_t? > > > If it's *never* going away then I think the change makes no sense. If romcc is > going away, then I would favor the change. With our current bootblock concept, it is never going away on x86 (for bootblock usage) Stefan -- coreboot mailing list: coreboot@coreboot.org http://www.coreboot.org/mailman/listinfo/coreboot
Re: [coreboot] [poll] device_t
> > one counter-question: is romcc ever going away, or at least is the usage > gong to change such that no code would ever use the uint32 version of > device_t? If it's *never* going away then I think the change makes no sense. If romcc is going away, then I would favor the change. ron -- coreboot mailing list: coreboot@coreboot.org http://www.coreboot.org/mailman/listinfo/coreboot
[coreboot] [poll] device_t
Since Edward started hijacking patches on gerrit to push his agenda of getting rid of device_t without prior discussion, I would like to start a poll on how people think about this, and maybe find reasons for why it might be a good idea. Edward wrote: > The issue of 'device_t' has many heads. The primary issue I have here > is that these patches introduce many more 'device_t' instances that > make the job of sorting out which 'device_t' are 'uint32_t' and which > are 'struct device *' significantly harder as this sort of thing > progresses. > If the author would just use 'struct device *' and there is (for some > wrapped reasoning) a consensus to only use 'device_t' then it is trivial > to go do 's/struct device * /device_t /g'. > There is actually no reason needed here to use a opaque type which was > invented to solve a particular issue however has now spilled over to > becoming rampant though the tree. The reality here is that device_t was a concept used ever since coreboot v2 (LinuxBIOS v2 from 2003 actually) existed. It is indeed the use of struct device that has accidently spilled over. The reason this was done was to use the same driver code in romstage and ramstage. While this can be achieved by other means, no doubt, I don't think that this churn on the code base is particularly useful. Before we continue on this path I would like to see a reason for not using device_t or why the difference between the type has to be sorted out. The whole idea is that it does not matter, and the code does not have to know. Check out the poll at https://doodle.com/vvqtyhdxr9yhvqci Stefan -- coreboot mailing list: coreboot@coreboot.org http://www.coreboot.org/mailman/listinfo/coreboot
