On Wed, Mar 03, 2021 at 05:26:23PM +0100, Helge Deller wrote:
On 2/24/21 2:23 AM, Kevin O'Connor wrote:
Similarly, there are many cases where parisc has different implementations of similar functionality that isn't architecture
I assume you meant "...that IS architecture specific" ?
No, I mean that the patch series introduces some code that is different from the X86 seabios code, but does not need to be different. That is, it introduces different versions of some functions that are not architecture specific. I understand introducing different versions of functions that are architectures specific - that's a requirement for supporting a different architecture - but it should not be necessary to introduce a parisc version of a malloc function, for example.
[...]
Overall, thanks a lot for your feedback!!!
But, I'm not clear yet on how to continue. Currently SeaBIOS-parisc is a fork, and I think it's easy to still keep the fork for the time beeing. That way there will not be additional work for the developers.
What I would prefer is if we maybe could work through at least some of the patches and see if we could integrate them (where it makes sense), so that my diff to upstream-seabios can get reduced. Would that be an acceptable way forward?
Sure - if there are ways to improve the SeaBIOS code that also make it easier to support parisc then that's fine.
If yes, I think I need clear guidance, e.g. first of all, is adding a CONFIG_X86 and CONFIG_PARISC config option (patch #1) in the Kconfig OK ?
In general, no. It wouldn't make sense for us to maintain code or config options for external code. That increases maintenance costs and is unlikely to succeed in general. (No one will test the config options not used locally.)
What about simplifying the bda accessors (patch #2 and #3, but drop the parisc part there before applying).
We can add accessor functions like get_bda_ptr(). However, patch 2 adds a bunch of ifdefs that don't look right to me - in particular, farvar.h already substitutes the simpler assignments when not in "segment mode", so it should not be necessary to add in additional ifdefs in biosvar.h .
Same for the patches regarding endianess (e.g. patch #4).
We can improve the endianness code, but I'd prefer an approach with less ifdefs. Gcc should have a macro for endianness already, and we should be able to use runtime C code to make the checks (which gcc can optimize at compile time).
Trivial ones like patch #7 which adds some parisc constants?
That has the issue of introducing ifdefs, and I don't think that is a good plan for external code (as mentioned above).
And patch #10 which adds the portaddr_t typedef?
That's a case where I'd just change the io functions to use a u32 universally. I don't think a typedef is needed.
Cheers, -Kevin