[U-Boot] [SoCFPGA] next steps
Hey,
given that we now have most of the u-boot socfpga stuff in mainline, I decided it would be a good idea to list what we're still missing and we should also decide how to move on now.
First thing I should probably clarify is the late acceptance of the socfpga patches. This is certainly not something we do regularly and is one of the worst possible practices to do, but this time it felt rather important to get the platform in shape, so this exception happened. Furthermore, all of the code in u-boot-socfpga should be based on u-boot-arm and should be submitted through the u-boot-arm repository, not directly to u-boot .
As you probably noticed, there is a lot of topic branches in the u-boot-socfpga [1] repository, each of them with a date tag. This decision came to be so that rebasing of a branch can be avoided. I will most likely garbage-collect the old and useless topic branches at some point, when they become irrelevant due to the code being merged into u-boot-arm/master (and then to u-boot/master).
I think that's about it for the organization part. Now for the remaining part, we are still missing a few things. Some of them are ready, some of them, not so much:
SPL: This is something we still miss from mainline. We will need to discuss this thoroughly, but one thing is obvious already -- we need to figure out how to interoperate with Quartus resp. the bsp-editor generated header files to assemble the SPL properly. USB: This is scheduled for the next merge window. The DWC2 driver is cleaned up and seems to be in rather good state. The USB driver currently resides in [2] EPCQ: This is something I prepared and tested real quick. The EPCS/EPCQ SPI NOR can be operated via the Altera SPI driver, which is currently unused at all and thus suffering bitrot. The current cleanup is here [3] NET: Does the SoCDK use EMAC0 or EMAC1 ? I believe we still have this issue open, I recall Chin was complaining about this.
You can find the latest combined version of all the patches at [4] to ease up your testing.
Please feel free to add into this list, it would be really good to keep track of what's still open and what's missing.
Thank you!
[1] http://git.denx.de/?p=u-boot/u-boot-socfpga.git;a=summary [2] http://git.denx.de/?p=u-boot/u-boot- usb.git;a=shortlog;h=refs/heads/topic/dwc2-20140930 [3] http://git.denx.de/?p=u-boot/u-boot- socfpga.git;a=shortlog;h=refs/heads/topic/drivers/spi-20141006 [4] http://git.denx.de/?p=u-boot/u-boot- socfpga.git;a=shortlog;h=refs/heads/topic/arm/socfpga-next-20141007
Hi,
On 10/07/2014 02:45 PM, Marek Vasut wrote:
Hey,
given that we now have most of the u-boot socfpga stuff in mainline, I decided it would be a good idea to list what we're still missing and we should also decide how to move on now.
First thing I should probably clarify is the late acceptance of the socfpga patches. This is certainly not something we do regularly and is one of the worst possible practices to do, but this time it felt rather important to get the platform in shape, so this exception happened. Furthermore, all of the code in u-boot-socfpga should be based on u-boot-arm and should be submitted through the u-boot-arm repository, not directly to u-boot .
Platform was in this shape for a while that's why I can't see the reason why this happen.
Tom: Does it mean that every platform which is not in good shape can go directly to the mainline in any time? It is definitely something which is good to know.
Thanks, Michal
On Wednesday, October 08, 2014 at 10:58:24 AM, Michal Simek wrote:
Hi,
On 10/07/2014 02:45 PM, Marek Vasut wrote:
Hey,
given that we now have most of the u-boot socfpga stuff in mainline, I decided it would be a good idea to list what we're still missing and we should also decide how to move on now.
First thing I should probably clarify is the late acceptance of the socfpga patches. This is certainly not something we do regularly and is one of the worst possible practices to do, but this time it felt rather important to get the platform in shape, so this exception happened. Furthermore, all of the code in u-boot-socfpga should be based on u-boot-arm and should be submitted through the u-boot-arm repository, not directly to u-boot .
Platform was in this shape for a while that's why I can't see the reason why this happen.
Tom: Does it mean that every platform which is not in good shape can go directly to the mainline in any time? It is definitely something which is good to know.
I believe this was an exception here. Also, the socfpga patches were posted on the list for a while (beginning of september) and they were tested on multiple devices by multiple parties, so there is clearly interest to get the platform in shape.
The initial patch was posted all the way back as "[RFC] mainline u-boot on socfpga" on Sept. 3rd 2014, which is in the middle of the release cycle and which still gave us enough time to clean the patches up still.
Best regards, Marek Vasut
On Wed 2014-10-08 10:58:24, Michal Simek wrote:
Hi,
On 10/07/2014 02:45 PM, Marek Vasut wrote:
Hey,
given that we now have most of the u-boot socfpga stuff in mainline, I decided it would be a good idea to list what we're still missing and we should also decide how to move on now.
First thing I should probably clarify is the late acceptance of the socfpga patches. This is certainly not something we do regularly and is one of the worst possible practices to do, but this time it felt rather important to get the platform in shape, so this exception happened. Furthermore, all of the code in u-boot-socfpga should be based on u-boot-arm and should be submitted through the u-boot-arm repository, not directly to u-boot .
Platform was in this shape for a while that's why I can't see the reason why this happen.
Tom: Does it mean that every platform which is not in good shape can go directly to the mainline in any time? It is definitely something which is good to know.
Well, the platform was in such a wrong shape that it would be hard to cause regressions, and code was self-contained enough that it would not break anything.
I'm not a Tom, but it makes sense to me. Pavel
On Wed, Oct 08, 2014 at 10:58:24AM +0200, Michal Simek wrote:
Hi,
On 10/07/2014 02:45 PM, Marek Vasut wrote:
Hey,
given that we now have most of the u-boot socfpga stuff in mainline, I decided it would be a good idea to list what we're still missing and we should also decide how to move on now.
First thing I should probably clarify is the late acceptance of the socfpga patches. This is certainly not something we do regularly and is one of the worst possible practices to do, but this time it felt rather important to get the platform in shape, so this exception happened. Furthermore, all of the code in u-boot-socfpga should be based on u-boot-arm and should be submitted through the u-boot-arm repository, not directly to u-boot .
Platform was in this shape for a while that's why I can't see the reason why this happen.
Tom: Does it mean that every platform which is not in good shape can go directly to the mainline in any time? It is definitely something which is good to know.
So, it's a long standing thing where for non-core changes, deferring to the relevant custodian about what's going to come in close to the release is what's done. So yes, I grilled Marek about what non-socfpga things would be impacted by the changes (RPi) and if he'd tested things there. It all had been through a few post/review cycles.
There's an argument to be made that we shouldn't have let socfpga in, back in 2012 or should have pushed harder, sooner, to get more progress made on "real" platform support.
Hi,
On 10/08/2014 10:09 PM, Tom Rini wrote:
On Wed, Oct 08, 2014 at 10:58:24AM +0200, Michal Simek wrote:
Hi,
On 10/07/2014 02:45 PM, Marek Vasut wrote:
Hey,
given that we now have most of the u-boot socfpga stuff in mainline, I decided it would be a good idea to list what we're still missing and we should also decide how to move on now.
First thing I should probably clarify is the late acceptance of the socfpga patches. This is certainly not something we do regularly and is one of the worst possible practices to do, but this time it felt rather important to get the platform in shape, so this exception happened. Furthermore, all of the code in u-boot-socfpga should be based on u-boot-arm and should be submitted through the u-boot-arm repository, not directly to u-boot .
Platform was in this shape for a while that's why I can't see the reason why this happen.
Tom: Does it mean that every platform which is not in good shape can go directly to the mainline in any time? It is definitely something which is good to know.
So, it's a long standing thing where for non-core changes, deferring to the relevant custodian about what's going to come in close to the release is what's done. So yes, I grilled Marek about what non-socfpga things would be impacted by the changes (RPi) and if he'd tested things there. It all had been through a few post/review cycles.
There's an argument to be made that we shouldn't have let socfpga in, back in 2012 or should have pushed harder, sooner, to get more progress made on "real" platform support.
AFAIK if platform is working in certain state and you are able to get for example console than there is no problem to be in. There is nowhere written what exactly should work that's why I can't see any problem that socfpga is in if it is not causing compilation issues and have at least minimum functionality.
The question was if the problem was that Altera just failed because didn't collect patches to any repo and sending it to Albert. Or there was just misunderstanding that Albert expected that Altera will do that and Altera expected that Albert will pick it up because he is ARM custodian and none was listed for socfpga. I have to defend Altera guys because if none is listed for SocFpga the nearest maintainer is collecting patches.
Then there was discussion that none did care about socfpga patches and problem was resolved by creating socfpga repository and Marek became custodian for it. Marek collected that patches to the new repo and also I believe add new one and rebased them on the top of current tree and send them out as one big 51 series which is not possible to even properly review. IMHO they should be sent separated to target exact audience which do care about spi/i2c/watchdog/fpga/soc etc. But maybe that's just matter of taste.
But I am still missing the point why that patches was that urgent that they were merged to rc3 when it was claimed that socfpga was in a wrong shape for a while. It means v2014.10 should be just another broken version for socfpga and all this mess should be solved properly in 2015.01 via socfpga repo.
And because patches went into rc3 and yesterday Jeroen is reporting problem on FreeBSD because of this merge.(http://patchwork.ozlabs.org/patch/397453/)
Regarding your point that all "It all had been through a few post/review cycles." I don't think all things have been fixed. Personally me I have reported here http://lists.denx.de/pipermail/u-boot/2014-September/189741.html (sha1: 0ae16cbb40a2881f6dfbe00fcb023ee7b548bc5c) issue with checkpatch.pl which hasn't been fixed.
Here is my ACK for one patch which is not present in mainline commit http://lists.denx.de/pipermail/u-boot/2014-September/189747.html (sha1: 2f210639c4f003b0d5310273979441f1bfc88eae)
Make no sense to go through all patches but this is just an example.
I think it is something what we should discuss at u-boot mini summit on Monday. I discussed this with Marek over IRC yesterday and I expect he will ping me today (because of this email :-) ).
If there is a problem because Albert is just too busy we should at least try to find out any reasonable way how to handle this. Like in Linux ARM-SOC custodian?
Thanks, Michal
On 9 October 2014 14:07, Michal Simek monstr@monstr.eu wrote:
Hi,
On 10/08/2014 10:09 PM, Tom Rini wrote:
On Wed, Oct 08, 2014 at 10:58:24AM +0200, Michal Simek wrote:
Hi,
On 10/07/2014 02:45 PM, Marek Vasut wrote:
Hey,
given that we now have most of the u-boot socfpga stuff in mainline, I decided it would be a good idea to list what we're still missing and we should also decide how to move on now.
First thing I should probably clarify is the late acceptance of the socfpga patches. This is certainly not something we do regularly and is one of the worst possible practices to do, but this time it felt rather important to get the platform in shape, so this exception happened. Furthermore, all of the code in u-boot-socfpga should be based on u-boot-arm and should be submitted through the u-boot-arm repository, not directly to u-boot .
Platform was in this shape for a while that's why I can't see the reason why this happen.
Tom: Does it mean that every platform which is not in good shape can go directly to the mainline in any time? It is definitely something which is good to know.
So, it's a long standing thing where for non-core changes, deferring to the relevant custodian about what's going to come in close to the release is what's done. So yes, I grilled Marek about what non-socfpga things would be impacted by the changes (RPi) and if he'd tested things there. It all had been through a few post/review cycles.
There's an argument to be made that we shouldn't have let socfpga in, back in 2012 or should have pushed harder, sooner, to get more progress made on "real" platform support.
AFAIK if platform is working in certain state and you are able to get for example console than there is no problem to be in. There is nowhere written what exactly should work that's why I can't see any problem that socfpga is in if it is not causing compilation issues and have at least minimum functionality.
The question was if the problem was that Altera just failed because didn't collect patches to any repo and sending it to Albert. Or there was just misunderstanding that Albert expected that Altera will do that and Altera expected that Albert will pick it up because he is ARM custodian and none was listed for socfpga. I have to defend Altera guys because if none is listed for SocFpga the nearest maintainer is collecting patches.
Then there was discussion that none did care about socfpga patches and problem was resolved by creating socfpga repository and Marek became custodian for it. Marek collected that patches to the new repo and also I believe add new one and rebased them on the top of current tree and send them out as one big 51 series which is not possible to even properly review. IMHO they should be sent separated to target exact audience which do care about spi/i2c/watchdog/fpga/soc etc. But maybe that's just matter of taste.
But I am still missing the point why that patches was that urgent that they were merged to rc3 when it was claimed that socfpga was in a wrong shape for a while. It means v2014.10 should be just another broken version for socfpga and all this mess should be solved properly in 2015.01 via socfpga repo.
And because patches went into rc3 and yesterday Jeroen is reporting problem on FreeBSD because of this merge.(http://patchwork.ozlabs.org/patch/397453/)
Regarding your point that all "It all had been through a few post/review cycles." I don't think all things have been fixed. Personally me I have reported here http://lists.denx.de/pipermail/u-boot/2014-September/189741.html (sha1: 0ae16cbb40a2881f6dfbe00fcb023ee7b548bc5c) issue with checkpatch.pl which hasn't been fixed.
Here is my ACK for one patch which is not present in mainline commit http://lists.denx.de/pipermail/u-boot/2014-September/189747.html (sha1: 2f210639c4f003b0d5310273979441f1bfc88eae)
Make no sense to go through all patches but this is just an example.
I think it is something what we should discuss at u-boot mini summit on Monday. I discussed this with Marek over IRC yesterday and I expect he will ping me today (because of this email :-) ).
If there is a problem because Albert is just too busy we should at least try to find out any reasonable way how to handle this. Like in Linux ARM-SOC custodian?
I think this traversing the actual development process in a different direction and it must be a valid point that need to discuss.
Apart from this, I'm experienced an another isuue where some of the subsystem patches (say for example spi stuff) are pushed in a different direction. http://patchwork.ozlabs.org/patch/346015/
These are the qspi stuff from freescale, and I didn't understand why these goes into u-boot-arm/master. And there is no intimation of mine as well.
Issue is that the driver itself is not in a proper shape, why does subsystem patches were pushed without the the review tag from a respective custodians.
Please try to discuss this point as well "Each subsystem patch(es) should be pushed if and only if the respective custodian should marked the review tag"
thanks!
On Thursday, October 09, 2014 at 01:20:23 PM, Jagan Teki wrote:
On 9 October 2014 14:07, Michal Simek monstr@monstr.eu wrote:
Hi,
On 10/08/2014 10:09 PM, Tom Rini wrote:
On Wed, Oct 08, 2014 at 10:58:24AM +0200, Michal Simek wrote:
Hi,
On 10/07/2014 02:45 PM, Marek Vasut wrote:
Hey,
given that we now have most of the u-boot socfpga stuff in mainline, I decided it would be a good idea to list what we're still missing and we should also decide how to move on now.
First thing I should probably clarify is the late acceptance of the socfpga patches. This is certainly not something we do regularly and is one of the worst possible practices to do, but this time it felt rather important to get the platform in shape, so this exception happened. Furthermore, all of the code in u-boot-socfpga should be based on u-boot-arm and should be submitted through the u-boot-arm repository, not directly to u-boot .
Platform was in this shape for a while that's why I can't see the reason why this happen.
Tom: Does it mean that every platform which is not in good shape can go directly to the mainline in any time? It is definitely something which is good to know.
So, it's a long standing thing where for non-core changes, deferring to the relevant custodian about what's going to come in close to the release is what's done. So yes, I grilled Marek about what non-socfpga things would be impacted by the changes (RPi) and if he'd tested things there. It all had been through a few post/review cycles.
There's an argument to be made that we shouldn't have let socfpga in, back in 2012 or should have pushed harder, sooner, to get more progress made on "real" platform support.
AFAIK if platform is working in certain state and you are able to get for example console than there is no problem to be in. There is nowhere written what exactly should work that's why I can't see any problem that socfpga is in if it is not causing compilation issues and have at least minimum functionality.
The question was if the problem was that Altera just failed because didn't collect patches to any repo and sending it to Albert. Or there was just misunderstanding that Albert expected that Altera will do that and Altera expected that Albert will pick it up because he is ARM custodian and none was listed for socfpga. I have to defend Altera guys because if none is listed for SocFpga the nearest maintainer is collecting patches.
Then there was discussion that none did care about socfpga patches and problem was resolved by creating socfpga repository and Marek became custodian for it. Marek collected that patches to the new repo and also I believe add new one and rebased them on the top of current tree and send them out as one big 51 series which is not possible to even properly review. IMHO they should be sent separated to target exact audience which do care about spi/i2c/watchdog/fpga/soc etc. But maybe that's just matter of taste.
But I am still missing the point why that patches was that urgent that they were merged to rc3 when it was claimed that socfpga was in a wrong shape for a while. It means v2014.10 should be just another broken version for socfpga and all this mess should be solved properly in 2015.01 via socfpga repo.
And because patches went into rc3 and yesterday Jeroen is reporting problem on FreeBSD because of this merge.(http://patchwork.ozlabs.org/patch/397453/)
Regarding your point that all "It all had been through a few post/review cycles." I don't think all things have been fixed. Personally me I have reported here http://lists.denx.de/pipermail/u-boot/2014-September/189741.html (sha1: 0ae16cbb40a2881f6dfbe00fcb023ee7b548bc5c) issue with checkpatch.pl which hasn't been fixed.
Here is my ACK for one patch which is not present in mainline commit http://lists.denx.de/pipermail/u-boot/2014-September/189747.html (sha1: 2f210639c4f003b0d5310273979441f1bfc88eae)
Make no sense to go through all patches but this is just an example.
I think it is something what we should discuss at u-boot mini summit on Monday. I discussed this with Marek over IRC yesterday and I expect he will ping me today (because of this email :-) ).
If there is a problem because Albert is just too busy we should at least try to find out any reasonable way how to handle this. Like in Linux ARM-SOC custodian?
I think this traversing the actual development process in a different direction and it must be a valid point that need to discuss.
Apart from this, I'm experienced an another isuue where some of the subsystem patches (say for example spi stuff) are pushed in a different direction. http://patchwork.ozlabs.org/patch/346015/
These are the qspi stuff from freescale, and I didn't understand why these goes into u-boot-arm/master. And there is no intimation of mine as well.
Did you comment on them at all please ? While I disagree with them bypassing your tree, I see they were rotting on the ML for a month and then Albert then picked those.
Issue is that the driver itself is not in a proper shape, why does subsystem patches were pushed without the the review tag from a respective custodians.
I produced a hypothesis above.
Can you retroactively comment on them and ask the author to fix the code?
Please try to discuss this point as well "Each subsystem patch(es) should be pushed if and only if the respective custodian should marked the review tag"
I agree we have an issue here, but I would suggest we move this discussion into a separate thread now. The subject of the email does not match the topic of the thread by far.
On 9 October 2014 19:12, Marek Vasut marex@denx.de wrote:
On Thursday, October 09, 2014 at 01:20:23 PM, Jagan Teki wrote:
On 9 October 2014 14:07, Michal Simek monstr@monstr.eu wrote:
Hi,
On 10/08/2014 10:09 PM, Tom Rini wrote:
On Wed, Oct 08, 2014 at 10:58:24AM +0200, Michal Simek wrote:
Hi,
On 10/07/2014 02:45 PM, Marek Vasut wrote:
Hey,
given that we now have most of the u-boot socfpga stuff in mainline, I decided it would be a good idea to list what we're still missing and we should also decide how to move on now.
First thing I should probably clarify is the late acceptance of the socfpga patches. This is certainly not something we do regularly and is one of the worst possible practices to do, but this time it felt rather important to get the platform in shape, so this exception happened. Furthermore, all of the code in u-boot-socfpga should be based on u-boot-arm and should be submitted through the u-boot-arm repository, not directly to u-boot .
Platform was in this shape for a while that's why I can't see the reason why this happen.
Tom: Does it mean that every platform which is not in good shape can go directly to the mainline in any time? It is definitely something which is good to know.
So, it's a long standing thing where for non-core changes, deferring to the relevant custodian about what's going to come in close to the release is what's done. So yes, I grilled Marek about what non-socfpga things would be impacted by the changes (RPi) and if he'd tested things there. It all had been through a few post/review cycles.
There's an argument to be made that we shouldn't have let socfpga in, back in 2012 or should have pushed harder, sooner, to get more progress made on "real" platform support.
AFAIK if platform is working in certain state and you are able to get for example console than there is no problem to be in. There is nowhere written what exactly should work that's why I can't see any problem that socfpga is in if it is not causing compilation issues and have at least minimum functionality.
The question was if the problem was that Altera just failed because didn't collect patches to any repo and sending it to Albert. Or there was just misunderstanding that Albert expected that Altera will do that and Altera expected that Albert will pick it up because he is ARM custodian and none was listed for socfpga. I have to defend Altera guys because if none is listed for SocFpga the nearest maintainer is collecting patches.
Then there was discussion that none did care about socfpga patches and problem was resolved by creating socfpga repository and Marek became custodian for it. Marek collected that patches to the new repo and also I believe add new one and rebased them on the top of current tree and send them out as one big 51 series which is not possible to even properly review. IMHO they should be sent separated to target exact audience which do care about spi/i2c/watchdog/fpga/soc etc. But maybe that's just matter of taste.
But I am still missing the point why that patches was that urgent that they were merged to rc3 when it was claimed that socfpga was in a wrong shape for a while. It means v2014.10 should be just another broken version for socfpga and all this mess should be solved properly in 2015.01 via socfpga repo.
And because patches went into rc3 and yesterday Jeroen is reporting problem on FreeBSD because of this merge.(http://patchwork.ozlabs.org/patch/397453/)
Regarding your point that all "It all had been through a few post/review cycles." I don't think all things have been fixed. Personally me I have reported here http://lists.denx.de/pipermail/u-boot/2014-September/189741.html (sha1: 0ae16cbb40a2881f6dfbe00fcb023ee7b548bc5c) issue with checkpatch.pl which hasn't been fixed.
Here is my ACK for one patch which is not present in mainline commit http://lists.denx.de/pipermail/u-boot/2014-September/189747.html (sha1: 2f210639c4f003b0d5310273979441f1bfc88eae)
Make no sense to go through all patches but this is just an example.
I think it is something what we should discuss at u-boot mini summit on Monday. I discussed this with Marek over IRC yesterday and I expect he will ping me today (because of this email :-) ).
If there is a problem because Albert is just too busy we should at least try to find out any reasonable way how to handle this. Like in Linux ARM-SOC custodian?
I think this traversing the actual development process in a different direction and it must be a valid point that need to discuss.
Apart from this, I'm experienced an another isuue where some of the subsystem patches (say for example spi stuff) are pushed in a different direction. http://patchwork.ozlabs.org/patch/346015/
These are the qspi stuff from freescale, and I didn't understand why these goes into u-boot-arm/master. And there is no intimation of mine as well.
Did you comment on them at all please ? While I disagree with them bypassing your tree, I see they were rotting on the ML for a month and then Albert then picked those.
This is not a question of commenting - but - about the process.
Yes, I asked the author to test the changes later for a while this got pushed. I never thought this could happen so suddenly without any ping or something.
I guess some times it happens few of the patches will rotted for a while on ML due to some delays, but taking them with/out any ping causes over head if the respective owner will look at the code for later modifications.
Issue is that the driver itself is not in a proper shape, why does subsystem patches were pushed without the the review tag from a respective custodians.
I produced a hypothesis above.
Can you retroactively comment on them and ask the author to fix the code?
Yes - I asked the author for fixing those for few of the patches against that change.
Please try to discuss this point as well "Each subsystem patch(es) should be pushed if and only if the respective custodian should marked the review tag"
I agree we have an issue here, but I would suggest we move this discussion into a separate thread now. The subject of the email does not match the topic of the thread by far.
Agreed - I mentioned this on this tread only for listing item on meeting, that it.
Thanks for your comments.
thanks!
On Thursday, October 09, 2014 at 06:11:37 PM, Jagan Teki wrote:
[...]
These are the qspi stuff from freescale, and I didn't understand why these goes into u-boot-arm/master. And there is no intimation of mine as well.
Did you comment on them at all please ? While I disagree with them bypassing your tree, I see they were rotting on the ML for a month and then Albert then picked those.
This is not a question of commenting - but - about the process.
Yes, I asked the author to test the changes later for a while this got pushed. I never thought this could happen so suddenly without any ping or something.
I guess some times it happens few of the patches will rotted for a while on ML due to some delays, but taking them with/out any ping causes over head if the respective owner will look at the code for later modifications.
I agree with you that there is a problem where custodians get bypassed and that such a thing happened to me as well. This is sporadic, but apparently annoying enough, so this should be added.
Issue is that the driver itself is not in a proper shape, why does subsystem patches were pushed without the the review tag from a respective custodians.
I produced a hypothesis above.
Can you retroactively comment on them and ask the author to fix the code?
Yes - I asked the author for fixing those for few of the patches against that change.
Thanks!
Please try to discuss this point as well "Each subsystem patch(es) should be pushed if and only if the respective custodian should marked the review tag"
I agree we have an issue here, but I would suggest we move this discussion into a separate thread now. The subject of the email does not match the topic of the thread by far.
Agreed - I mentioned this on this tread only for listing item on meeting, that it.
Will you join us as well? (sorry, I lost track of who will and who won't)
Best regards, Marek Vasut
On 9 October 2014 21:45, Marek Vasut marex@denx.de wrote:
On Thursday, October 09, 2014 at 06:11:37 PM, Jagan Teki wrote:
[...]
These are the qspi stuff from freescale, and I didn't understand why these goes into u-boot-arm/master. And there is no intimation of mine as well.
Did you comment on them at all please ? While I disagree with them bypassing your tree, I see they were rotting on the ML for a month and then Albert then picked those.
This is not a question of commenting - but - about the process.
Yes, I asked the author to test the changes later for a while this got pushed. I never thought this could happen so suddenly without any ping or something.
I guess some times it happens few of the patches will rotted for a while on ML due to some delays, but taking them with/out any ping causes over head if the respective owner will look at the code for later modifications.
I agree with you that there is a problem where custodians get bypassed and that such a thing happened to me as well. This is sporadic, but apparently annoying enough, so this should be added.
Ok - thanks.
Issue is that the driver itself is not in a proper shape, why does subsystem patches were pushed without the the review tag from a respective custodians.
I produced a hypothesis above.
Can you retroactively comment on them and ask the author to fix the code?
Yes - I asked the author for fixing those for few of the patches against that change.
Thanks!
Please try to discuss this point as well "Each subsystem patch(es) should be pushed if and only if the respective custodian should marked the review tag"
I agree we have an issue here, but I would suggest we move this discussion into a separate thread now. The subject of the email does not match the topic of the thread by far.
Agreed - I mentioned this on this tread only for listing item on meeting, that it.
Will you join us as well? (sorry, I lost track of who will and who won't)
I'm unable to join this, hope you guys have more fun than last year :)
thanks!
On Thursday, October 09, 2014 at 10:37:52 AM, Michal Simek wrote:
Hi,
Hi!
I changed the subject, since it long didn't match the topic.
On 10/08/2014 10:09 PM, Tom Rini wrote:
On Wed, Oct 08, 2014 at 10:58:24AM +0200, Michal Simek wrote:
Hi,
On 10/07/2014 02:45 PM, Marek Vasut wrote:
Hey,
given that we now have most of the u-boot socfpga stuff in mainline, I decided it would be a good idea to list what we're still missing and we should also decide how to move on now.
First thing I should probably clarify is the late acceptance of the socfpga patches. This is certainly not something we do regularly and is one of the worst possible practices to do, but this time it felt rather important to get the platform in shape, so this exception happened. Furthermore, all of the code in u-boot-socfpga should be based on u-boot-arm and should be submitted through the u-boot-arm repository, not directly to u-boot .
Platform was in this shape for a while that's why I can't see the reason why this happen.
Tom: Does it mean that every platform which is not in good shape can go directly to the mainline in any time? It is definitely something which is good to know.
So, it's a long standing thing where for non-core changes, deferring to the relevant custodian about what's going to come in close to the release is what's done. So yes, I grilled Marek about what non-socfpga things would be impacted by the changes (RPi) and if he'd tested things there. It all had been through a few post/review cycles.
There's an argument to be made that we shouldn't have let socfpga in, back in 2012 or should have pushed harder, sooner, to get more progress made on "real" platform support.
AFAIK if platform is working in certain state and you are able to get for example console than there is no problem to be in. There is nowhere written what exactly should work that's why I can't see any problem that socfpga is in if it is not causing compilation issues and have at least minimum functionality.
This was not the case in here. The platform was completely unusable.
The question was if the problem was that Altera just failed because didn't collect patches to any repo and sending it to Albert. Or there was just misunderstanding that Albert expected that Altera will do that and Altera expected that Albert will pick it up because he is ARM custodian and none was listed for socfpga. I have to defend Altera guys because if none is listed for SocFpga the nearest maintainer is collecting patches.
It was not Altera guys who started submitting patches to put this thing in shape. Altera only realized that things started to move after Pavel sent the initial blob-of-a-patch . Since then, we are moving forward.
Then there was discussion that none did care about socfpga patches and problem was resolved by creating socfpga repository and Marek became custodian for it. Marek collected that patches to the new repo and also I believe add new one and rebased them on the top of current tree and send them out as one big 51 series which is not possible to even properly review.
Patches which were contained to the architecture for the most part, mind you. The rest were fixes for issues which appeared during this development, thus fixing issues in U-Boot.
IMHO they should be sent separated to target exact audience which do care about spi/i2c/watchdog/fpga/soc etc. But maybe that's just matter of taste.
I fully agree on this part.
But I am still missing the point why that patches was that urgent that they were merged to rc3 when it was claimed that socfpga was in a wrong shape for a while. It means v2014.10 should be just another broken version for socfpga and all this mess should be solved properly in 2015.01 via socfpga repo.
Yes, this would mean that another broken version would be released even though the fixes were available. This doesn't sound right to me.
And because patches went into rc3 and yesterday Jeroen is reporting problem on FreeBSD because of this merge.(http://patchwork.ozlabs.org/patch/397453/)
This problem was discovered in a patch which was first posted to the list on Feb. 19 ( http://comments.gmane.org/gmane.comp.boot-loaders.u-boot/180797 ), which is before 2014.04 release and was not discovered through the review process.
Regarding your point that all "It all had been through a few post/review cycles." I don't think all things have been fixed. Personally me I have reported here http://lists.denx.de/pipermail/u-boot/2014-September/189741.html (sha1: 0ae16cbb40a2881f6dfbe00fcb023ee7b548bc5c) issue with checkpatch.pl which hasn't been fixed.
And I explicitly noted that the FPGA stuff still has a couple of checkpatch issues right in the subsequent email [1] . Processing all the checkpatch issues of that file would make the resulting patch a horrid mess instead of producing a well contained set of patches.
[1] http://lists.denx.de/pipermail/u-boot/2014-September/189745.html
Here is my ACK for one patch which is not present in mainline commit http://lists.denx.de/pipermail/u-boot/2014-September/189747.html (sha1: 2f210639c4f003b0d5310273979441f1bfc88eae)
Apologies, this one was indeed missed.
Make no sense to go through all patches but this is just an example.
I think it is something what we should discuss at u-boot mini summit on Monday. I discussed this with Marek over IRC yesterday and I expect he will ping me today (because of this email :-) ).
I agree we should discuss this on the minisummit. But what is "this" topic exactly? Do we have a list of topics we need to discuss somewhere, so this one can be added there ?
If there is a problem because Albert is just too busy we should at least try to find out any reasonable way how to handle this. Like in Linux ARM-SOC custodian?
I don't this Albert is the problem, I am starting to suspect we simply lack custodian manpower in general. And I also suspect we're not quite inviting and attractive crowd, which is something we should discuss too ...
On 10/09/2014 04:03 PM, Marek Vasut wrote:
On Thursday, October 09, 2014 at 10:37:52 AM, Michal Simek wrote:
Hi,
Hi!
I changed the subject, since it long didn't match the topic.
On 10/08/2014 10:09 PM, Tom Rini wrote:
On Wed, Oct 08, 2014 at 10:58:24AM +0200, Michal Simek wrote:
Hi,
On 10/07/2014 02:45 PM, Marek Vasut wrote:
Hey,
given that we now have most of the u-boot socfpga stuff in mainline, I decided it would be a good idea to list what we're still missing and we should also decide how to move on now.
First thing I should probably clarify is the late acceptance of the socfpga patches. This is certainly not something we do regularly and is one of the worst possible practices to do, but this time it felt rather important to get the platform in shape, so this exception happened. Furthermore, all of the code in u-boot-socfpga should be based on u-boot-arm and should be submitted through the u-boot-arm repository, not directly to u-boot .
Platform was in this shape for a while that's why I can't see the reason why this happen.
Tom: Does it mean that every platform which is not in good shape can go directly to the mainline in any time? It is definitely something which is good to know.
So, it's a long standing thing where for non-core changes, deferring to the relevant custodian about what's going to come in close to the release is what's done. So yes, I grilled Marek about what non-socfpga things would be impacted by the changes (RPi) and if he'd tested things there. It all had been through a few post/review cycles.
There's an argument to be made that we shouldn't have let socfpga in, back in 2012 or should have pushed harder, sooner, to get more progress made on "real" platform support.
AFAIK if platform is working in certain state and you are able to get for example console than there is no problem to be in. There is nowhere written what exactly should work that's why I can't see any problem that socfpga is in if it is not causing compilation issues and have at least minimum functionality.
This was not the case in here. The platform was completely unusable.
I think that doesn't matter too much now because it was just merged in.
Also I think that a lot of platforms are in u-boot and only one person has tested it. For completely new platform this is likely the case. Simply we have to trust to submitter than this is working.
The question was if the problem was that Altera just failed because didn't collect patches to any repo and sending it to Albert. Or there was just misunderstanding that Albert expected that Altera will do that and Altera expected that Albert will pick it up because he is ARM custodian and none was listed for socfpga. I have to defend Altera guys because if none is listed for SocFpga the nearest maintainer is collecting patches.
It was not Altera guys who started submitting patches to put this thing in shape. Altera only realized that things started to move after Pavel sent the initial blob-of-a-patch . Since then, we are moving forward.
Progress is important but should be done in a way which is standard.
Then there was discussion that none did care about socfpga patches and problem was resolved by creating socfpga repository and Marek became custodian for it. Marek collected that patches to the new repo and also I believe add new one and rebased them on the top of current tree and send them out as one big 51 series which is not possible to even properly review.
Patches which were contained to the architecture for the most part, mind you. The rest were fixes for issues which appeared during this development, thus fixing issues in U-Boot.
IMHO they should be sent separated to target exact audience which do care about spi/i2c/watchdog/fpga/soc etc. But maybe that's just matter of taste.
I fully agree on this part.
great.
But I am still missing the point why that patches was that urgent that they were merged to rc3 when it was claimed that socfpga was in a wrong shape for a while. It means v2014.10 should be just another broken version for socfpga and all this mess should be solved properly in 2015.01 via socfpga repo.
Yes, this would mean that another broken version would be released even though the fixes were available. This doesn't sound right to me.
Aug 2 merge windows was closed and Pavel sent RFC to mailing list Sep 3. And Sep 8 I have replied to him to move things to drivers/fpga.
http://lists.denx.de/pipermail/u-boot/2014-September/188311.html
That's why I don't think that at least fpga part was sent to the mailing list at proper time. Maybe I am just wrong and you will find out any really ancient commit with fpga code that I have no problem to admit that I was wrong with fpga part.
And because patches went into rc3 and yesterday Jeroen is reporting problem on FreeBSD because of this merge.(http://patchwork.ozlabs.org/patch/397453/)
This problem was discovered in a patch which was first posted to the list on Feb. 19 ( http://comments.gmane.org/gmane.comp.boot-loaders.u-boot/180797 ), which is before 2014.04 release and was not discovered through the review process.
No problem with timing but then why it is not the part of that series if you are aware about that. :-) Introduction new build error between rc2/rc3 is not the best thing to do.
Regarding your point that all "It all had been through a few post/review cycles." I don't think all things have been fixed. Personally me I have reported here http://lists.denx.de/pipermail/u-boot/2014-September/189741.html (sha1: 0ae16cbb40a2881f6dfbe00fcb023ee7b548bc5c) issue with checkpatch.pl which hasn't been fixed.
And I explicitly noted that the FPGA stuff still has a couple of checkpatch issues right in the subsequent email [1] . Processing all the checkpatch issues of that file would make the resulting patch a horrid mess instead of producing a well contained set of patches.
[1] http://lists.denx.de/pipermail/u-boot/2014-September/189745.html
If I look at that patch 27/51 it is just simple sed s/__FUNCTION__/__func__/g and s/PRINTF/debug_cond/g you do also a lot of changes like s/printf (/printf(/g and also for others functions that's why I tend to say that also that 5 warnings should be resolved.
Here is my ACK for one patch which is not present in mainline commit http://lists.denx.de/pipermail/u-boot/2014-September/189747.html (sha1: 2f210639c4f003b0d5310273979441f1bfc88eae)
Apologies, this one was indeed missed.
no problem at all. As you know I sent it just for fun. :-)
Make no sense to go through all patches but this is just an example.
I think it is something what we should discuss at u-boot mini summit on Monday. I discussed this with Marek over IRC yesterday and I expect he will ping me today (because of this email :-) ).
I agree we should discuss this on the minisummit. But what is "this" topic exactly? Do we have a list of topics we need to discuss somewhere, so this one can be added there ?
Last time IRC Detlev had some sort of list of topics which needs to be discussed.
If there is a problem because Albert is just too busy we should at least try to find out any reasonable way how to handle this. Like in Linux ARM-SOC custodian?
I don't this Albert is the problem, I am starting to suspect we simply lack custodian manpower in general. And I also suspect we're not quite inviting and attractive crowd, which is something we should discuss too ...
+1 on this.
Thanks, Michal
On Thu, Oct 09, 2014 at 04:45:07PM +0200, Michal Simek wrote:
On 10/09/2014 04:03 PM, Marek Vasut wrote:
On Thursday, October 09, 2014 at 10:37:52 AM, Michal Simek wrote:
Hi,
Hi!
I changed the subject, since it long didn't match the topic.
[snip]
If there is a problem because Albert is just too busy we should at least try to find out any reasonable way how to handle this. Like in Linux ARM-SOC custodian?
I don't this Albert is the problem, I am starting to suspect we simply lack custodian manpower in general. And I also suspect we're not quite inviting and attractive crowd, which is something we should discuss too ...
+1 on this.
Yes, lets talk more about this.
On Thursday, October 09, 2014 at 05:57:47 PM, Tom Rini wrote:
On Thu, Oct 09, 2014 at 04:45:07PM +0200, Michal Simek wrote:
On 10/09/2014 04:03 PM, Marek Vasut wrote:
On Thursday, October 09, 2014 at 10:37:52 AM, Michal Simek wrote:
Hi,
Hi!
I changed the subject, since it long didn't match the topic.
[snip]
If there is a problem because Albert is just too busy we should at least try to find out any reasonable way how to handle this. Like in Linux ARM-SOC custodian?
I don't this Albert is the problem, I am starting to suspect we simply lack custodian manpower in general. And I also suspect we're not quite inviting and attractive crowd, which is something we should discuss too ...
+1 on this.
Yes, lets talk more about this.
Also a CI would really help. Vadim was already proposing one last year too, we should certainly review the outcome on that.
Best regards, Marek Vasut
On Thu, Oct 09, 2014 at 06:10:12PM +0200, Marek Vasut wrote:
On Thursday, October 09, 2014 at 05:57:47 PM, Tom Rini wrote:
On Thu, Oct 09, 2014 at 04:45:07PM +0200, Michal Simek wrote:
On 10/09/2014 04:03 PM, Marek Vasut wrote:
On Thursday, October 09, 2014 at 10:37:52 AM, Michal Simek wrote:
Hi,
Hi!
I changed the subject, since it long didn't match the topic.
[snip]
If there is a problem because Albert is just too busy we should at least try to find out any reasonable way how to handle this. Like in Linux ARM-SOC custodian?
I don't this Albert is the problem, I am starting to suspect we simply lack custodian manpower in general. And I also suspect we're not quite inviting and attractive crowd, which is something we should discuss too ...
+1 on this.
Yes, lets talk more about this.
Also a CI would really help. Vadim was already proposing one last year too, we should certainly review the outcome on that.
I don't think anything went too far on that, but in a follow-up elsewhere I'm not having buildman do something I thought it did, but perhaps I imagined, which was only show me new problems since the last time it ran.
On Thursday, October 09, 2014 at 06:25:30 PM, Tom Rini wrote:
On Thu, Oct 09, 2014 at 06:10:12PM +0200, Marek Vasut wrote:
On Thursday, October 09, 2014 at 05:57:47 PM, Tom Rini wrote:
On Thu, Oct 09, 2014 at 04:45:07PM +0200, Michal Simek wrote:
On 10/09/2014 04:03 PM, Marek Vasut wrote:
On Thursday, October 09, 2014 at 10:37:52 AM, Michal Simek wrote:
Hi,
Hi!
I changed the subject, since it long didn't match the topic.
[snip]
If there is a problem because Albert is just too busy we should at least try to find out any reasonable way how to handle this. Like in Linux ARM-SOC custodian?
I don't this Albert is the problem, I am starting to suspect we simply lack custodian manpower in general. And I also suspect we're not quite inviting and attractive crowd, which is something we should discuss too ...
+1 on this.
Yes, lets talk more about this.
Also a CI would really help. Vadim was already proposing one last year too, we should certainly review the outcome on that.
I don't think anything went too far on that, but in a follow-up elsewhere I'm not having buildman do something I thought it did, but perhaps I imagined, which was only show me new problems since the last time it ran.
Tom, I think we should start building a list for the discussion. There seem to be an awful lot of ideas on what to discuss spurring here, but is anyone taking notes ?
Should I or shall we start logging this into some wiki ?
Best regards, Marek Vasut
Hi!
I don't this Albert is the problem, I am starting to suspect we simply lack custodian manpower in general. And I also suspect we're not quite inviting and attractive crowd, which is something we should discuss too ...
As I said privately, I believe we have way too many custodians...
Anyway, u-boot code looks similar to kernel code, but patch submission rules are really different.
Something like this could help..? Pavel
--- /dev/null 2014-10-09 01:15:57.354292026 +0200 +++ doc/SubmittingPatches 2014-10-09 23:58:53.058883776 +0200 @@ -0,0 +1,11 @@ +Differences from kernel: + +* SPDX license headers are required. + +* puts() is preffered over single-argument prinf() + +* later versions of patch should come with "diff changelog" below "---" + +* subject should begin with tags, such as "arm: socfpga:" + +* should pass checkpatch \ No newline at end of file
Dear Pavel,
In message 20141009221154.GA24774@amd you wrote:
Something like this could help..? Pavel
--- /dev/null 2014-10-09 01:15:57.354292026 +0200 +++ doc/SubmittingPatches 2014-10-09 23:58:53.058883776 +0200
Is there anything wrong with [1] ?
[1] http://www.denx.de/wiki/U-Boot/Patches
Best regards,
Wolfgang Denk
Hi!
Something like this could help..? Pavel
--- /dev/null 2014-10-09 01:15:57.354292026 +0200 +++ doc/SubmittingPatches 2014-10-09 23:58:53.058883776 +0200
Is there anything wrong with [1] ?
It should really go into tree.
It does not mention puts() vs. printf(), if it is indeed meant to be u-boot policy. Pavel
Dear Pavel,
In message 20141009230004.GA25685@amd you wrote:
It should really go into tree.
If you think so...
It does not mention puts() vs. printf(), if it is indeed meant to be u-boot policy.
This is not just U-Boot philosophy, but something that I would consider a matter of course when writing code - using the appropriate tools for the task at hand. If all you want to do is sendout a constant string to the utput device, there is no need to invoke a function that provides fancy formatting options.
Don't we always try to use the smallest, most efficient tool that is suited for a task?
Best regards,
Wolfgang Denk
Hello Wolfgang,
On 10-10-14 14:22, Wolfgang Denk wrote:
It does not mention puts() vs. printf(), if it is indeed meant to be u-boot policy.
This is not just U-Boot philosophy, but something that I would consider a matter of course when writing code - using the appropriate tools for the task at hand. If all you want to do is sendout a constant string to the utput device, there is no need to invoke a function that provides fancy formatting options.
Don't we always try to use the smallest, most efficient tool that is suited for a task?
calling printf("%s\n", "string") gets translated into puts by the compiler. There should be no difference in the binary.
Regards, Jeroen
On Friday, October 10, 2014 at 04:04:40 PM, Jeroen Hofstee wrote:
Hello Wolfgang,
On 10-10-14 14:22, Wolfgang Denk wrote:
It does not mention puts() vs. printf(), if it is indeed meant to be u-boot policy.
This is not just U-Boot philosophy, but something that I would consider a matter of course when writing code - using the appropriate tools for the task at hand. If all you want to do is sendout a constant string to the utput device, there is no need to invoke a function that provides fancy formatting options.
Don't we always try to use the smallest, most efficient tool that is suited for a task?
calling printf("%s\n", "string") gets translated into puts by the compiler. There should be no difference in the binary.
Is this LLVM specific or does GCC do that too ? This is interesting information.
Best regards, Marek Vasut
Hi Marek,
On Fri, Oct 10, 2014 at 11:26 AM, Marek Vasut marex@denx.de wrote:
calling printf("%s\n", "string") gets translated into puts by the compiler. There should be no difference in the binary.
Is this LLVM specific or does GCC do that too ? This is interesting information.
Just did a quick test here with gcc and the resulting u-boot binary size is the same when I use puts or printf.
Hello Marek,
On 10-10-14 16:26, Marek Vasut wrote:
On Friday, October 10, 2014 at 04:04:40 PM, Jeroen Hofstee wrote:
Hello Wolfgang,
On 10-10-14 14:22, Wolfgang Denk wrote:
It does not mention puts() vs. printf(), if it is indeed meant to be u-boot policy.
This is not just U-Boot philosophy, but something that I would consider a matter of course when writing code - using the appropriate tools for the task at hand. If all you want to do is sendout a constant string to the utput device, there is no need to invoke a function that provides fancy formatting options.
Don't we always try to use the smallest, most efficient tool that is suited for a task?
calling printf("%s\n", "string") gets translated into puts by the compiler. There should be no difference in the binary
Is this LLVM specific or does GCC do that too ? This is interesting information.
I was talking about gcc, it has been doing such since ages ago (unless you purposely disable it). clang does it as well.
Regards, Jeroen
Hi Jeroen,
On Fri, 10 Oct 2014 18:09:19 +0200, Jeroen Hofstee jeroen@myspectrum.nl wrote:
Hello Marek,
On 10-10-14 16:26, Marek Vasut wrote:
On Friday, October 10, 2014 at 04:04:40 PM, Jeroen Hofstee wrote:
Hello Wolfgang,
On 10-10-14 14:22, Wolfgang Denk wrote:
It does not mention puts() vs. printf(), if it is indeed meant to be u-boot policy.
This is not just U-Boot philosophy, but something that I would consider a matter of course when writing code - using the appropriate tools for the task at hand. If all you want to do is sendout a constant string to the utput device, there is no need to invoke a function that provides fancy formatting options.
Don't we always try to use the smallest, most efficient tool that is suited for a task?
calling printf("%s\n", "string") gets translated into puts by the compiler. There should be no difference in the binary
Is this LLVM specific or does GCC do that too ? This is interesting information.
I was talking about gcc, it has been doing such since ages ago (unless you purposely disable it). clang does it as well.
That's a good thing, but generally speaking, I think that just because the compiler is being clever doesn't mean we are allowed to rely on that, because if we do take a habit of relying on the compiler being clever, two things will happen:
1) we will keep thinking the compiler is being clever even when for some reason it will stop being clever -- for instance, because someone decided to disable the clever feature;
2) we will begin thinking the compiler is clever in situations where it never has and never will.
IMO, a quick cost/benefit comparison of choosing between manually turning printf() into puts whenever doable vs letting the compiler do the changes automatically, the manual option wins -- it's bit like Pascal's Wager: you don't lose much but you can only win.
Regards, Jeroen _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Amicalement,
Hello Albert,
On 10-10-14 21:51, Albert ARIBAUD wrote:
Hi Jeroen,
On Fri, 10 Oct 2014 18:09:19 +0200, Jeroen Hofstee jeroen@myspectrum.nl wrote:
Hello Marek,
On 10-10-14 16:26, Marek Vasut wrote:
On Friday, October 10, 2014 at 04:04:40 PM, Jeroen Hofstee wrote:
Hello Wolfgang,
On 10-10-14 14:22, Wolfgang Denk wrote:
It does not mention puts() vs. printf(), if it is indeed meant to be u-boot policy.
This is not just U-Boot philosophy, but something that I would consider a matter of course when writing code - using the appropriate tools for the task at hand. If all you want to do is sendout a constant string to the utput device, there is no need to invoke a function that provides fancy formatting options.
Don't we always try to use the smallest, most efficient tool that is suited for a task?
calling printf("%s\n", "string") gets translated into puts by the compiler. There should be no difference in the binary
Is this LLVM specific or does GCC do that too ? This is interesting information.
I was talking about gcc, it has been doing such since ages ago (unless you purposely disable it). clang does it as well.
That's a good thing, but generally speaking, I think that just because the compiler is being clever doesn't mean we are allowed to rely on that, because if we do take a habit of relying on the compiler being clever, two things will happen:
Why can't this be relied on, I gave up digging if this is a gcc 3 or 2 feature. It is old at least, museum stuff if it is not supported.
- we will keep thinking the compiler is being clever even when for
some reason it will stop being clever -- for instance, because someone decided to disable the clever feature;
If you ask to disable it, it is good if it does so, don't see a problem with that. Anyway, it is not an u-boot issue, anything below -O2 is not supported anyway.
- we will begin thinking the compiler is clever in situations where it
never has and never will.
I would almost take this as an insult, I hope u-boot folks know or at least check before they assume a compiler does XYZ. And yes compilers will replace simple printf call with their simpler equivalent and has been doing so for quite a while (and that is an understatement).
IMO, a quick cost/benefit comparison of choosing between manually turning printf() into puts whenever doable vs letting the compiler do the changes automatically, the manual option wins -- it's bit like Pascal's Wager: you don't lose much but you can only win.
No it is the other way around; why on earth do you want demand patch submitters to make changes which result in the exactly same binary; you waste time of reviewers / patch submitter and it doesn't serve a goal.
So to turn it around: just use printf: "you don't lose much but you can only win."
Regards, Jeroen
Hi Jeroen,
On Fri, 10 Oct 2014 22:40:48 +0200, Jeroen Hofstee dasuboot@myspectrum.nl wrote:
Hello Albert,
On 10-10-14 21:51, Albert ARIBAUD wrote:
Hi Jeroen,
On Fri, 10 Oct 2014 18:09:19 +0200, Jeroen Hofstee jeroen@myspectrum.nl wrote:
Hello Marek,
On 10-10-14 16:26, Marek Vasut wrote:
On Friday, October 10, 2014 at 04:04:40 PM, Jeroen Hofstee wrote:
Hello Wolfgang,
On 10-10-14 14:22, Wolfgang Denk wrote:
> It does not mention puts() vs. printf(), if it is indeed meant to be > u-boot policy. This is not just U-Boot philosophy, but something that I would consider a matter of course when writing code - using the appropriate tools for the task at hand. If all you want to do is sendout a constant string to the utput device, there is no need to invoke a function that provides fancy formatting options.
Don't we always try to use the smallest, most efficient tool that is suited for a task?
calling printf("%s\n", "string") gets translated into puts by the compiler. There should be no difference in the binary
Is this LLVM specific or does GCC do that too ? This is interesting information.
I was talking about gcc, it has been doing such since ages ago (unless you purposely disable it). clang does it as well.
That's a good thing, but generally speaking, I think that just because the compiler is being clever doesn't mean we are allowed to rely on that, because if we do take a habit of relying on the compiler being clever, two things will happen:
Why can't this be relied on, I gave up digging if this is a gcc 3 or 2 feature. It is old at least, museum stuff if it is not supported.
It's a general habit I have of never assuming anything is forever.
- we will keep thinking the compiler is being clever even when for
some reason it will stop being clever -- for instance, because someone decided to disable the clever feature;
If you ask to disable it, it is good if it does so, don't see a problem with that. Anyway, it is not an u-boot issue, anything below -O2 is not supported anyway.
There is no problem if the person disabling the feature is also the one doing assumptions on the feature state. Problems arise when these are two different persons, because the one relying on the feature might not know or notice that the other one disabled it.
- we will begin thinking the compiler is clever in situations where it
never has and never will.
I would almost take this as an insult, I hope u-boot folks know or at least check before they assume a compiler does XYZ. And yes compilers will replace simple printf call with their simpler equivalent and has been doing so for quite a while (and that is an understatement).
I certainly did not intend this as an insult. But no, if I personally had encountered a printf("%s\n", s) in the course of some development, I would not have gone and looked whether the compiler might turn it into a puts(s). I'd have assumed it does not, because assuming so was the fastest and safest approach.
Don't misunderstand me: I'm quite pleased that gcc and LLVM do that. And I'm also quite convinced that it's a good thing -- actually, I did start by saying so. The problem I see is not specifically with the printf()/puts() case, it is with the *general* assumption that a compiler is clever -- and again, I did start my opinion with "generally speaking".
The problem I see is that a developer will not necessarily go and test every single thing that the compiler *might* be clever enough to do (or dump enough not to); a developer will assume some of it, and whenever he assumes, he should assume a dumb compiler.
IMO, a quick cost/benefit comparison of choosing between manually turning printf() into puts whenever doable vs letting the compiler do the changes automatically, the manual option wins -- it's bit like Pascal's Wager: you don't lose much but you can only win.
No it is the other way around; why on earth do you want demand patch submitters to make changes which result in the exactly same binary; you waste time of reviewers / patch submitter and it doesn't serve a goal.
So to turn it around: just use printf: "you don't lose much but you can only win."
In the specific case of printf()/puts(), a wager is meaningless since the compiler capability is already known. Therefore, there is no reason to "wager on 'just us[ing] printf'"; there is reason to "just use printf because it is known to turn into puts() if possible".
But please remember that I considered the wager *only* in a *general* case where a developer could replace some code with some other, more efficient code, and does *not* know whether the compiler could be clever enough to do the replacement by itself. The developer /might/ check the compiler... or he might not check, and instead, wager on the compiler being clever or dumb; and in this *general* case, he should wager on a dumb compiler.
Of course, if the case is specific and known to him, i.e. he already knows whether the compiler will be clever or dumb in that specific case, then there is *no* wager to be done; the developer should just act on his knowledge (and check it again from time to time, but that's somewhat beside the point).
Regards, Jeroen
Amicalement,
Dear Jeroen,
In message 54384450.3000204@myspectrum.nl you wrote:
If you ask to disable it, it is good if it does so, don't see a problem with that. Anyway, it is not an u-boot issue, anything below -O2 is not supported anyway.
I'm not sure what you mean here. Gcc certainly does this replacement with -Os as used for U-Boot.
I would almost take this as an insult, I hope u-boot folks know or at least check before they assume a compiler does XYZ. And yes compilers will replace simple printf call with their simpler equivalent and has been doing so for quite a while (and that is an understatement).
I wonder how many people know about this - and where it is documented?
So to turn it around: just use printf: "you don't lose much but you can only win."
Sorry, but I disagree here.
First, we have a compatibility problem here. GCC assumes that puts() will add a newline character after the string; U-Boot puts() does NOT do this. So the GCC auto-converted printf()s will all be wrong, as they are missing the newline. [1]
Second, using puts() insteas of printf() is also a means of documenting your code. It shows your intention to print a constant string. Just one example: compare
A: void add_header(const char *s) { printf("MY HEADER: %s\n", s); }
versus
B: void add_header(const char *s) { puts("MY HEADER: "); puts(s); /* Assuming U-Boot puts() - no automatic \n added */ putc('\n'); }
Which is "better"? A is obviously much shorter and more elegant; but B is much more robust - A will happily crash your system when you try to print a string like "s%s%s%s%s%s%s%s%s%s%s" (not to mention that this may open a classic attack vector to break into a running system).
So yes, it does make sense to explicitly use puts().
[1] One might argue that this is a bug in U-Boot and should be fixed, but that is another topic.
Best regards,
Wolfgang Denk
Dear Jeroen,
In message 20141011150346.150C038352A@gemini.denx.de i wrote:
Which is "better"? A is obviously much shorter and more elegant; but B is much more robust - A will happily crash your system when you try to print a string like "s%s%s%s%s%s%s%s%s%s%s" (not to mention that this may open a classic attack vector to break into a running system).
Ignore me. This example was obviously crap. What I had in mind was something where you would use
char *s; ... printf(s);
Sorry...
Best regards,
Wolfgang Denk
Hi!
First, we have a compatibility problem here. GCC assumes that puts() will add a newline character after the string; U-Boot puts() does NOT do this. So the GCC auto-converted printf()s will all be wrong, as they are missing the newline. [1]
[1] One might argue that this is a bug in U-Boot and should be fixed, but that is another topic.
I believe we should fix that, yes.
I did quick grep,
pavel@duo:~/wagabuibui/u-boot$ grep -ri puts . | wc -l 4287
and that is probably too much to change in one go. So what about this?
Best regards, Pavel ---
Introduce __puts() that puts strings without trailing newline. Plan is to move the existing puts() users into __puts(), when no puts() users are left, fix the puts() to add the newline, and move users that want newline back to puts().
Signed-off-by: Pavel Machek pavel@denx.de
--- a/include/common.h +++ b/include/common.h @@ -836,6 +836,8 @@ int tstc(void); /* stdout */ void putc(const char c); void puts(const char *s); +static inline void __puts(const char *s) { puts(s); } + int printf(const char *fmt, ...) __attribute__ ((format (__printf__, 1, 2))); int vprintf(const char *fmt, va_list args);
On Wed 2014-10-15 10:40:12, Pavel Machek wrote:
Hi!
First, we have a compatibility problem here. GCC assumes that puts() will add a newline character after the string; U-Boot puts() does NOT do this. So the GCC auto-converted printf()s will all be wrong, as they are missing the newline. [1]
[1] One might argue that this is a bug in U-Boot and should be fixed, but that is another topic.
I believe we should fix that, yes.
I did quick grep,
pavel@duo:~/wagabuibui/u-boot$ grep -ri puts . | wc -l 4287
and that is probably too much to change in one go. So what about this?
Next step is probably
diff --git a/include/common.h b/include/common.h index d5020c8..95b2377 100644 --- a/include/common.h +++ b/include/common.h @@ -836,6 +836,9 @@ int tstc(void); /* stdout */ void putc(const char c); void puts(const char *s); +static inline void __puts(const char *s) { puts(s); } +static inline void putsnl(const char *s) { puts(s); putc('\n'); } + int printf(const char *fmt, ...) __attribute__ ((format (__printf__, 1, 2))); int vprintf(const char *fmt, va_list args);
Then run
sed 's/puts((.*)\n")/putsnl(\1")/' `find . -name "*.[ch]"`
... to get existing users converted. Then we can convert the rest to to either __puts or putsnl, and finally get rid of putsnl and convert it back to puts.
(Patch is >400KB, so I'll not post it here).
Best regards, Pavel
On Wed, Oct 15, 2014 at 10:40:12AM +0200, Pavel Machek wrote:
Hi!
First, we have a compatibility problem here. GCC assumes that puts() will add a newline character after the string; U-Boot puts() does NOT do this. So the GCC auto-converted printf()s will all be wrong, as they are missing the newline. [1]
[1] One might argue that this is a bug in U-Boot and should be fixed, but that is another topic.
I believe we should fix that, yes.
I did quick grep,
pavel@duo:~/wagabuibui/u-boot$ grep -ri puts . | wc -l 4287
and that is probably too much to change in one go. So what about this?
I'm thinking, now that we know that with $(CC) -ffreestanding printf is not converted to puts, we should (a) globally change puts("no newline") to printf (b) fix puts behavior to conform to standards and correct puts("newline\n") to puts("newline"). And then let people use whatever of the print functions they want, while we sort out making people use debug(...) / error(...) more and come up with a clever output scheme like we talked about in person.
Dear Jeroen,
In message 5437E778.3050306@myspectrum.nl you wrote:
calling printf("%s\n", "string") gets translated into puts by the compiler. There should be no difference in the binary.
Interesting, I didn't know that. Is this somewhere documented?
Is there any comprehensive list of such "magic" optimizations?
Best regards,
Wolfgang Denk
Hello Wolfgang / Albert / others,
On 10-10-14 16:04, Jeroen Hofstee wrote:
Hello Wolfgang,
On 10-10-14 14:22, Wolfgang Denk wrote:
It does not mention puts() vs. printf(), if it is indeed meant to be u-boot policy.
This is not just U-Boot philosophy, but something that I would consider a matter of course when writing code - using the appropriate tools for the task at hand. If all you want to do is sendout a constant string to the utput device, there is no need to invoke a function that provides fancy formatting options.
Don't we always try to use the smallest, most efficient tool that is suited for a task?
calling printf("%s\n", "string") gets translated into puts by the compiler. There should be no difference in the binary.
mumbles: while this is true in general it won't hold for u-boot since -ffreestanding disables such rewrites and u-boot is compiled with that flag. On the bright side, perhaps I educated some people a bit that they are wasting time rewriting such lines in normal, hosted applications.
Regards, Jeroen
On Fri 2014-10-10 00:24:46, Wolfgang Denk wrote:
Dear Pavel,
In message 20141009221154.GA24774@amd you wrote:
Something like this could help..? Pavel
--- /dev/null 2014-10-09 01:15:57.354292026 +0200 +++ doc/SubmittingPatches 2014-10-09 23:58:53.058883776 +0200
Is there anything wrong with [1] ?
..and actually... it makes submitting patches rather hard.
[PATCH] fix compilation on socfpga
Please add tags to the subject
[PATCHv2] arm: socfpga: fix compilation on socfpga
Please add diff from previous version
[PATCHv3] arm: socfpga: fix compilation on socfpga
---
v2: added tags to the subject
v3: added diffs to previous version
. (From memory, but IIRC something very similar to this happened before).
This scares of all but the most determined patch submitters, and does not really improve code quality.
I'd argue that if only changelog is updated, it is _not_ a new version of patch, and does not need changelog diff. Or maybe be less strict policy / less strict enforcement of the policy in trivial cases.
Best regards,
Pavel
Hi Pavel,
On Fri, 10 Oct 2014 01:05:59 +0200, Pavel Machek pavel@denx.de wrote:
On Fri 2014-10-10 00:24:46, Wolfgang Denk wrote:
Dear Pavel,
In message 20141009221154.GA24774@amd you wrote:
Something like this could help..? Pavel
--- /dev/null 2014-10-09 01:15:57.354292026 +0200 +++ doc/SubmittingPatches 2014-10-09 23:58:53.058883776 +0200
Is there anything wrong with [1] ?
..and actually... it makes submitting patches rather hard.
[PATCH] fix compilation on socfpga
Please add tags to the subject
[PATCHv2] arm: socfpga: fix compilation on socfpga
Please add diff from previous version
[PATCHv3] arm: socfpga: fix compilation on socfpga
v2: added tags to the subject
Tags can be useful in automating CC: lists from Patman through doc/git-mailrc, and as a filtering key in e.g. gitk, hence the suggestion to add them. Guessing which tags a patch could use is indeed a tedious and uncertain process, but I don't think it is requested of many patches, is it?
v3: added diffs to previous version
. (From memory, but IIRC something very similar to this happened before).
At least it happened that I requested the change logs when they were missing entirely in a v2-or-later series. The reason is that with these logs, reviewers can see what change requests were acknowledged by the submitter and what other changes were spontaneous additions.
This scares of all but the most determined patch submitters, and does not really improve code quality.
One can argue that it improves code /review/, by both making sure the submitter has involved the relevant custodians (tags) and provided a follow-up on their previous remarks (diffs).
Note that patman help a lot about maintaining the change log and tags.
I'd argue that if only changelog is updated, it is _not_ a new version of patch, and does not need changelog diff. Or maybe be less strict policy / less strict enforcement of the policy in trivial cases.
Well, if only a changelog is updated, then a [PATCH vN RESEND] should be as ok as a [PATCH vN+1], and anyway both will end up as "a new patch" for Patchwork, so the difference is not really major IMO -- meaning both should be accepted and, I believe, are accepted in practice.
Best regards,
Pavel
Amicalement,
Dear Pavel,
In message 20141009230559.GB25685@amd you wrote:
v2: added tags to the subject v3: added diffs to previous version . (From memory, but IIRC something very similar to this happened before).
Yes, this happens when people repeatedly ignore to read the patch posting rules.
I'd argue that if only changelog is updated, it is _not_ a new version of patch, and does not need changelog diff. Or maybe be less strict policy / less strict enforcement of the policy in trivial cases.
You ignore the fact that you are supposed to miimize the work load you create on reviewers. If I try to review a patch, I need to understand what has been changed compared to the previous version. Yes, of course I can apply both versions and run a diff (diffs over diffs are too difficult to read in general), but this costs time. And each reviewer has to spend this time.
On the other hand, you should know exactly what you've changes, so it is minimal work for you to add such an explanation. It you use the provided tools for the task (patman comes to mind), this is even well supported.
So what is better for the community: if you spend a little time once, or if every reviewer spends much more time trying to figure out what you might have changed? So it all depends whether you take the egotistic or the community point of view....
Best regards,
Wolfgang Denk
On Fri, Oct 10, 2014 at 12:11:54AM +0200, Pavel Machek wrote:
Hi!
I don't this Albert is the problem, I am starting to suspect we simply lack custodian manpower in general. And I also suspect we're not quite inviting and attractive crowd, which is something we should discuss too ...
As I said privately, I believe we have way too many custodians...
I think perhaps I need to better spell out custodian expectations. But as to having too many? No, I need some convincing in that direction.
Anyway, u-boot code looks similar to kernel code, but patch submission rules are really different.
Something like this could help..? Pavel
--- /dev/null 2014-10-09 01:15:57.354292026 +0200 +++ doc/SubmittingPatches 2014-10-09 23:58:53.058883776 +0200 @@ -0,0 +1,11 @@ +Differences from kernel:
Perhaps. If not in tree, a concice reminder to follow all of the rules spelled out on the wiki to avoid the ping-ponging you noted in a follow up later.
Hi Marek,
On 10/7/14, 7:45 AM, Marek Vasut wrote:
Hey,
given that we now have most of the u-boot socfpga stuff in mainline, I decided it would be a good idea to list what we're still missing and we should also decide how to move on now.
Thanks for all of your hard work on this!
First thing I should probably clarify is the late acceptance of the socfpga patches. This is certainly not something we do regularly and is one of the worst possible practices to do, but this time it felt rather important to get the platform in shape, so this exception happened. Furthermore, all of the code in u-boot-socfpga should be based on u-boot-arm and should be submitted through the u-boot-arm repository, not directly to u-boot .
As you probably noticed, there is a lot of topic branches in the u-boot-socfpga [1] repository, each of them with a date tag. This decision came to be so that rebasing of a branch can be avoided. I will most likely garbage-collect the old and useless topic branches at some point, when they become irrelevant due to the code being merged into u-boot-arm/master (and then to u-boot/master).
I think that's about it for the organization part. Now for the remaining part, we are still missing a few things. Some of them are ready, some of them, not so much:
SPL: This is something we still miss from mainline. We will need to discuss this thoroughly, but one thing is obvious already -- we need to figure out how to interoperate with Quartus resp. the bsp-editor generated header files to assemble the SPL properly.
Have you or anyone else already started on this work? If not, then this is an area that I can start to work on.
USB: This is scheduled for the next merge window. The DWC2 driver is cleaned up and seems to be in rather good state. The USB driver currently resides in [2] EPCQ: This is something I prepared and tested real quick. The EPCS/EPCQ SPI NOR can be operated via the Altera SPI driver, which is currently unused at all and thus suffering bitrot. The current cleanup is here [3] NET: Does the SoCDK use EMAC0 or EMAC1 ? I believe we still have this issue open, I recall Chin was complaining about this.
The SoCDK is using EMAC1.
BR, Dinh
On Wednesday, October 08, 2014 at 03:18:10 PM, Dinh Nguyen wrote:
Hi Marek,
Hi all!
On 10/7/14, 7:45 AM, Marek Vasut wrote:
Hey,
given that we now have most of the u-boot socfpga stuff in mainline, I decided it would be a good idea to list what we're still missing and we should also decide how to move on now.
Thanks for all of your hard work on this!
I only did what needed to be done and I'm looking forward to you taking over shortly ;-)
[...]
SPL: This is something we still miss from mainline. We will need to discuss
this thoroughly, but one thing is obvious already -- we need to figure out how to interoperate with Quartus resp. the bsp-editor generated header files to assemble the SPL properly.Have you or anyone else already started on this work? If not, then this is an area that I can start to work on.
No, noone did and I'd be really happy if you took this one.
USB: This is scheduled for the next merge window. The DWC2 driver is cleaned
up and seems to be in rather good state. The USB driver currently resides in [2]EPCQ: This is something I prepared and tested real quick. The EPCS/EPCQ SPI NOR
can be operated via the Altera SPI driver, which is currently unused at all and thus suffering bitrot. The current cleanup is here [3]NET: Does the SoCDK use EMAC0 or EMAC1 ? I believe we still have this issue
open, I recall Chin was complaining about this.The SoCDK is using EMAC1.
I guess we'd need one more quick patch for current mainline then, since this seems to be broken ever since. I will bake one shortly.
Also, let me add two more goals: - Migrate to driver model - Support probing of the whole board from Device Tree Blob
This would make our lives so much easier, so we should aim for this. I don't know if it is realistic to make this happen for 2015.01, but I would like to keep this in mind.
Best regards, Marek Vasut
Hi Marek,
2014-10-07 21:45 GMT+09:00 Marek Vasut marex@denx.de:
Hey,
given that we now have most of the u-boot socfpga stuff in mainline, I decided it would be a good idea to list what we're still missing and we should also decide how to move on now.
Can you refactor arch/arm/include/asm/spl.h and arch/arm/include/asm/arch-socfpga/spl.h ?
I mean, I want all the SoCs to use BOOT_DEVICE_* defined in arch/arm/include/asm/spl.h rather than duplicating it in arch/arm/include/asm/arch-*/spl.h
I am expecting something like what Michal did for Zynq in commit dbc31f6a and commit ae2ee77f98.
On Saturday, October 11, 2014 at 08:22:29 PM, Masahiro YAMADA wrote:
Hi Marek,
Hello!
2014-10-07 21:45 GMT+09:00 Marek Vasut marex@denx.de:
Hey,
given that we now have most of the u-boot socfpga stuff in mainline, I decided it would be a good idea to list what we're still missing and we should also decide how to move on now.
Can you refactor arch/arm/include/asm/spl.h and arch/arm/include/asm/arch-socfpga/spl.h ?
I mean, I want all the SoCs to use BOOT_DEVICE_* defined in arch/arm/include/asm/spl.h rather than duplicating it in arch/arm/include/asm/arch-*/spl.h
I am expecting something like what Michal did for Zynq in commit dbc31f6a and commit ae2ee77f98.
I just sent out a patch: Re: [PATCH] arm: socfpga: Zap spl.h and ad-hoc related syms This should resolve it.
Best regards, Marek Vasut
participants (12)
-
Albert ARIBAUD -
Dinh Nguyen -
Fabio Estevam -
Jagan Teki -
Jeroen Hofstee -
Jeroen Hofstee -
Marek Vasut -
Masahiro YAMADA -
Michal Simek -
Pavel Machek -
Tom Rini -
Wolfgang Denk