Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36087 )
Change subject: soc/intel/tigerlake: Do initial SoC commit till ramstage ......................................................................
Patch Set 12:
(3 comments)
Patch Set 12:
> HI Arthur/Philipp, > > Will you please consider your -2 with this new sets of split CL in place and we have icl base clean up cl also merged ? This will help us to land TGL SOC copy code ?
This still has received no real review. It references many docs that I cannot find so how do you expect proper review to happen?
I have access to the documents and would be willing to review. However, I want to be fully honest: a review of copy-first patches will take weeks if not months. It pushes a lot of work that should be done by the authors to the reviewers and will create a bottle-neck. I estimate at least five times more work for reviewers this way. Doesn't seem fair, and due to the overhead of copy-first patch-later, it might even be less work for me if I write the patches by myself.
Again, document said to have TGL specific changes which are yet to land. this copy patches are placeholder from previous soc to help review only TGL specific changes as per logic.
Again, again, again, it eases the review of differences, yes; but it also increases the effort to confirm that everything else really stayed the same by one or two orders of magnitude.
Shouldn't the same we can confirm based on TGL specific CL's for an example: GPIO driver for TGL. If its an incremental change over original ICL gpio code then why should I write again a fresh code? thats the argument i have.
I don't say you should. But I don't see how this information, that it was an incremental change, is valuable in our Git history.
Given that we don't have trust on SOC vendors that they know what they are doing with new soc. in terms of IP. We are just asking help to make the code land and its matter to have 8-10 CL to make entire ICL to TGL transition.
That 8-10 sounds bad to me. Few, big commits are 10 times harder to review than many, small commits.
And i have shared the document number so u can verify whatever coming new makes sense or not.
I think we should also verify that everything that we assume is unchanged in hardware really didn't change. Otherwise, we'll create hard to track bugs for the future.
Let's stick to your GPIO example. In the current patch set, `gpio.c`. How is one supposed to review this file?
its ICL gpio.c is getting copied for TGL so we "assume" TGL is going to use "almost" similar GPIO structure hence this CL just copied the same. You should look for TGL GPIO changes. if you have issue with existing ICL GPIO structure that could have handle here already. Isn't the copy patch originally intended to save review time ? atleast since BSW i'm working in coreeboot, except APL (which was boot from star) we already had copied the previous generation soc.
- To see if this is the final state of the file, I'd have to look ahead on the patch branch. Time wasted.
- It is not the final state, so I'd have to figure out for every line of the file if that line is in its final state. Incredibly much time wasted.
- If I have to comment on one line but surrounding lines are patched later, on which change shall I comment? => messy review, more time wasted.
Another example `graphics.c`. This file is flawed, but not changed later it seems. If we don't go through the pain of reviewing it as part of the messy review, the errors will stick. Now, in a year or so, somebody might run into a bug there, they might check the Git history for clues about the code:
Please feel free to fix graphics.c, i believe its same code getting copied over soc and common piece already landed into soc common. if you think something is wrong why to wait for new soc copy ?
- How is one supposed to know if the code was already checked to be Tiger Lake compatible?
I believe that is the work we are doing to do the delta analysis and landing the CL
- If somebody assumes that it was checked but it wasn't, more time gets wasted.
Again there is part of SOC vendor work i believe to tell you what is coming new and what is getting copied.
i agreed that incremental CL would be good but right now the time we have might not suitable for complete incremental code development from scratch. And based on your feedback i have already splited CL's into smaller group to ease of ICL copy patch review. please hold for TGL specific code review after we could able to land the basic CLs
https://review.coreboot.org/c/coreboot/+/36087/12/src/soc/intel/tigerlake/Ma... File src/soc/intel/tigerlake/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/36087/12/src/soc/intel/tigerlake/Ma... PS12, Line 57: postcar-y += memmap.c
only needed in romstage anymore in master. Needs a rebase.
Done
https://review.coreboot.org/c/coreboot/+/36087/12/src/soc/intel/tigerlake/pm... File src/soc/intel/tigerlake/pmutil.c:
https://review.coreboot.org/c/coreboot/+/36087/12/src/soc/intel/tigerlake/pm... PS12, Line 140: void
cast to uint8_t *
Done
https://review.coreboot.org/c/coreboot/+/36087/12/src/soc/intel/tigerlake/sm... File src/soc/intel/tigerlake/smmrelocate.c:
https://review.coreboot.org/c/coreboot/+/36087/12/src/soc/intel/tigerlake/sm... PS12, Line 191: tseg_base
Check if aligned to tseg_size.
isn't we did that in line 182 ?