Re: [U-Boot] [PATCH v4 2/2] NS16550: buffer reads
On 10/17/2011 07:53 AM, Wolfgang Denk wrote: Dear Simon, I wrote: - However, your implementation does not result in reliable multi-line input. It works only in a few specific use cases, and will fail silently for others. I don't see a way ho you would announce this new feature. The numbers you mention in the commit message (it handles around 70 lines before lossage) apply for this specific board only, and for your use case only (pasting of short lines that produce no output). Actually I believe that the restriction is even worse - I think that any commands that consume any significant amount of time will casue problems, too. Can you please test what happens when you have, for example, a sleep 10 or a erase all in the first few lines of the pasted input ? The reference to 70 lines was meant as an anecdote about how it made pasting things significantly more usable for me. I never advertised it as a complete fix for serial loss (indeed, it even says that I still experienced loss after 70 lines). It's just a fairly simple change that lessens the problem for me (and apparently others) to the point where it's no longer a significant nuisance. -Scott ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v4 2/2] NS16550: buffer reads
Dear Graeme Russ, In message 4ea34086.4030...@gmail.com you wrote: One problem I see with XON/XOFF is that if we don't send XOFF at the right time, we run the risk of entering a busy loop (any reasonable timeout delay for example) and loosing input. So in theory, we would need to send XOFF after every getc() ... That's not true. I am not aware of any significant delays that take place while receiving characters that belong to a single line. If we had any of these, we would lose characters all the time - but we don't. It should be sufficient to send XOFF after receiving a newline character. Maybe we need disable/enable flow control functions for when we know we will be entering a busy loop the consumes serial input (ymodem and kermit transfers and readline for example) This should not be necessary. Actually the implementation should not need to know about such special cases. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Compassion -- that's the one things no machine ever had. Maybe it's the one thing that keeps men ahead of them. -- McCoy, The Ultimate Computer, stardate 4731.3 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v4 2/2] NS16550: buffer reads
Hi Wolgang, On Oct 23, 2011 7:20 PM, Wolfgang Denk w...@denx.de wrote: Dear Graeme Russ, In message 4ea34086.4030...@gmail.com you wrote: One problem I see with XON/XOFF is that if we don't send XOFF at the right time, we run the risk of entering a busy loop (any reasonable timeout delay for example) and loosing input. So in theory, we would need to send XOFF after every getc() ... That's not true. I am not aware of any significant delays that take place while receiving characters that belong to a single line. If we had any of these, we would lose characters all the time - but we don't. It should be sufficient to send XOFF after receiving a newline character. And, ergo, we send an XON when entering the readline function Hmm, should we move readline() into console.c Maybe we need disable/enable flow control functions for when we know we will be entering a busy loop the consumes serial input (ymodem and kermit transfers and readline for example) This should not be necessary. Actually the implementation should not need to know about such special cases. So how does kermit/ymodem send the XON after the user has entered the receive command and we have sent the XOFF after the newline? Regards, Graeme ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v4 2/2] NS16550: buffer reads
Dear Graeme Russ, In message calbutcleg6c30en3n4ljpv1wojjfxwkekhvqymojy8+mgsz...@mail.gmail.com you wrote: It should be sufficient to send XOFF after receiving a newline character. And, ergo, we send an XON when entering the readline function This is probably not sufficient, as some commands take direct input. I think both getc() and tstc() should check the XON/XOFF state and send a XON if XOFF was sent before. Hmm, should we move readline() into console.c Makes sense. This should not be necessary. Actually the implementation should not need to know about such special cases. So how does kermit/ymodem send the XON after the user has entered the receive command and we have sent the XOFF after the newline? Upon the first getc() that follows? Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Marriage is the sole cause of divorce. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v4 2/2] NS16550: buffer reads
Dear Simon Glass, In message 1318742050-2201-2-git-send-email-...@chromium.org you wrote: From: Scott Wood scottw...@freescale.com From: Scott Wood scottw...@freescale.com This improves the performance of U-Boot when accepting rapid input, such as pasting a sequence of commands. Without this patch, on P4080DS I see a maximum of around 5 lines can be pasted. With this patch, it handles around 70 lines before lossage, long enough for most things you'd paste. With it enabled, on ppc it's an extra 396 bytes of image size, and 1056 bytes of BSS. ARM note from Simon Glass s...@chromium.org - ARM code size goes from 212 to 484 bytes (extra 272 bytes), BSS to 1056 bytes. Signed-off-by: Scott Wood scottw...@freescale.com I've made up my mind. The reasons why you want to add such code are well understood, but the unsolved and unsolvable issues with this approach are so severe that I don't want to raise false hopes in innocent users. Sorry, but I reject this patch. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Nature is very un-American. Nature never hurries. - William George Jordan ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v4 2/2] NS16550: buffer reads
Hi Wolfgang, On Oct 24, 2011 4:15 AM, Wolfgang Denk w...@denx.de wrote: Dear Graeme Russ, In message calbutcleg6c30en3n4ljpv1wojjfxwkekhvqymojy8+mgsz...@mail.gmail.com you wrote: [snip] This should not be necessary. Actually the implementation should not need to know about such special cases. So how does kermit/ymodem send the XON after the user has entered the receive command and we have sent the XOFF after the newline? Upon the first getc() that follows? And as there will be no corresponding newline, when do we send XOFF? Regards, Graeme ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v4 2/2] NS16550: buffer reads
Dear Graeme Russ, In message CALButCK0oVLTbYxbTF=y-vXL+3+=09vejpzunx1+-0syjqe...@mail.gmail.com you wrote: So how does kermit/ymodem send the XON after the user has entered the receive command and we have sent the XOFF after the newline? Upon the first getc() that follows? And as there will be no corresponding newline, when do we send XOFF? Never? Note that kermit / ymodem / S-record download etc. all don't have any issues with sendign characters back-to-back at line speed. Problems happen only with multi-line input, so it is perfectly fine to handle just that - at the root cause, i. e. when input turns into multi-line input. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Include the success of others in your dreams for your own success. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v4 2/2] NS16550: buffer reads
Le 17/10/2011 22:58, Simon Glass a écrit : Hi Wolfgang, On Mon, Oct 17, 2011 at 1:33 PM, Wolfgang Denkw...@denx.de wrote: Dear Simon Glass, In messagecapnjgz1412ezjh3f4ww16k4z55kjdggwj7+vmyk_ot-mzvd...@mail.gmail.com you wrote: Can you please tell me which ELDK version this is using? I see that the 405 board seems to need ppc_4xx which suggests 4.2 rather than 5, since in 5 the compiler is called powerpc- Right, I was testing this with ELDK 4.2. I am struggling to repeat this and don't even get the same numbers... Could be due to two things: 1) both of you do not work from the same exact tree commit, or 2) both of you work in a different base directory For your AR405 board, you saw: - 246058 12972 14636 273666 42d02 /work/wd/tmp-ppc/u-boot + 246062 12972 14636 273670 42d06 /work/wd/tmp-ppc/u-boot For me: 24594212964 14632 273538 42c82 ppc/u-boot I suggest Wolfgang and you compare your trees to eliminate risk 1, and make sure you compare .bin files to reduce risk 2. Amicalement, -- Albert. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v4 2/2] NS16550: buffer reads
Le 17/10/2011 18:40, Simon Glass a écrit : So essentially you are saying: hey, we now have multi-line input, but it works only a bit. It will fail sooner or later, without error messages, and I cannot even tell you what the limits for your system are. And it depends on which input you send. This does not exactly sound promising. That all sounds reasonable. I think it is good enough, but not perfect. Due to the fundamental design decision you mention at the top, we cannot squirrel away serial input in the background. The best we can do is squirrel it away in the foreground, as we output new serial data (I suppose we could squirrel it away in net loops and other places but I don't want to go there!). This turns out to be mostly good enough, because it is rare for people to want to paste 'sleep 10' and the like into their terminal (your other message). I think you're not seeing the point that while your patch is good enough for 'making multiline input less impractical in some cases', it is not good enough for 'making multiline input reliable': it adds marginal benefits and we have clear evidence that it will never guarantee a correct result however you tweak it, because the only way it could would be by multi-tasking in some way; otherwise, there can always be some U-Boot activity in between input readings that will be long enough to cause character loss. I would like to spit out a warning when serial input is lost - as I mentioned that could be combined with a serial overhaul at some point. I don't think the warning would make the functionality better. On the other hand, we also have the rule that things that are useful to some and that don't hurt others should be allowed to go in. What makes me hesitate are two things: - The patch promises a feature (multi-line input), but fails to provide a reliably working implementation. I *think* it does the best possible within the fundamental design decision constraint. If there is more it can do, please let me have your ideas. I don't believe buffering conflicts with the constraint - they are separate things. But yes in systems with interrupts normally the input buffer is filled in the background and drained in the foreground. There *is* a much better solution within the fundamental single tasking design constraint, and it is the one that was invented precisely for this: flow control. Granted, hardware flow control may be impractical, and that's why software flow control was conceived as early as 1963 and is still widely used today and supported by any serial software or driver worth its salt. - As it turns out, the patch increases code size even for boards that do not activate this feature. Yes, I will take a look at this problem. I have to admit that I'm at a loss with a decision here. Well it's not easy being a maintainer :-) Well, FWIW, were I the serial code maintainer, I'd NAK this and ask for a XON/XOFF implementation. In any case this patch is not the end of the story as serial needs some work - another objection you didn't mention above is that this function lives in only one driver. Is that a good thing (hide it away) or a bad thing (all drivers should support it and the implementation should move up a level)? Software flow control, whatever way it is implemented, is pure SW and hardware independent, so it should be a generic thing. If XON/XOFF input flow control gets implemented (and I believe it should), then it should be done above hardware serial drivers. Regards, Simon Amicalement, -- Albert. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v4 2/2] NS16550: buffer reads
Hi Simon, Albert, On 22/10/11 19:44, Albert ARIBAUD wrote: Le 17/10/2011 18:40, Simon Glass a écrit : [snip] On the other hand, we also have the rule that things that are useful to some and that don't hurt others should be allowed to go in. What makes me hesitate are two things: - The patch promises a feature (multi-line input), but fails to provide a reliably working implementation. I fully agree with this - We should strive to eliminate bugs, not cover them with more dirt :) I *think* it does the best possible within the fundamental design decision constraint. If there is more it can do, please let me have your ideas. I don't believe buffering conflicts with the constraint - they are separate things. But yes in systems with interrupts normally the input buffer is filled in the background and drained in the foreground. There *is* a much better solution within the fundamental single tasking design constraint, and it is the one that was invented precisely for this: flow control. Granted, hardware flow control may be impractical, and that's why software flow control was conceived as early as 1963 and is still widely used today and supported by any serial software or driver worth its salt. Agree - flow control is the way to go Well, FWIW, were I the serial code maintainer, I'd NAK this and ask for a XON/XOFF implementation. In any case this patch is not the end of the story as serial needs some work - another objection you didn't mention above is that this function lives in only one driver. Is that a good thing (hide it away) or a bad thing (all drivers should support it and the implementation should move up a level)? Software flow control, whatever way it is implemented, is pure SW and hardware independent, so it should be a generic thing. If XON/XOFF input flow control gets implemented (and I believe it should), then it should be done above hardware serial drivers. One problem I see with XON/XOFF is that if we don't send XOFF at the right time, we run the risk of entering a busy loop (any reasonable timeout delay for example) and loosing input. So in theory, we would need to send XOFF after every getc() and XON before each getc() and then need to delay a bit to wait for the character (unless tstc returns true in which case we can just grab the char in the buffer) which seems a bit excessive. Maybe we need disable/enable flow control functions for when we know we will be entering a busy loop the consumes serial input (ymodem and kermit transfers and readline for example) Regards, Graeme ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v4 2/2] NS16550: buffer reads
Dear Simon Glass, In message 1318742050-2201-2-git-send-email-...@chromium.org you wrote: ... With it enabled, on ppc it's an extra 396 bytes of image size, and 1056 bytes of BSS. ARM note from Simon Glass s...@chromium.org - ARM code size goes from 212 to 484 bytes (extra 272 bytes), BSS to 1056 bytes. One problem I see is that the code size on some boards grows even if the new option is not enabled. Here a compare before and after applying your patches (without any changes to the respective board configurations): Configuring for AR405 board... textdata bss dec hex filename - 246058 12972 14636 273666 42d02 /work/wd/tmp-ppc/u-boot + 246062 12972 14636 273670 42d06 /work/wd/tmp-ppc/u-boot Configuring for CANBT board... textdata bss dec hex filename - 12071086043876 133190 20846 /work/wd/tmp-ppc/u-boot + 12071486043876 133194 2084a /work/wd/tmp-ppc/u-boot Configuring for pcs440ep board... textdata bss dec hex filename - 296191 19636 345088 660915 a15b3 /work/wd/tmp-ppc/u-boot + 296195 19636 345088 660919 a15b7 /work/wd/tmp-ppc/u-boot ... Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Out of register space (ugh) - vi ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v4 2/2] NS16550: buffer reads
Dear Simon, In message 1318742050-2201-2-git-send-email-...@chromium.org you wrote: This improves the performance of U-Boot when accepting rapid input, such as pasting a sequence of commands. Without this patch, on P4080DS I see a maximum of around 5 lines can be pasted. With this patch, it handles around 70 lines before lossage, long enough for most things you'd paste. Let me try to summarize my thinking: - It is a fundamental design decision that U-Boot is single tasking. This implies that while a command is running, no other things get done in parallel. This includes communication over the serial line, USB, Ethernet, ... - That means that by design U-Boot does not support multi-line input: as soon as you hit enter U-Boot will start executing your command, and will only become ready for new input when it has completed the execution of the command(s), i. e. after printing the next prompt. - For this suported mode of operation your patch is not needed. It just adds code size and complexity. - Your description rapid input is actually wrong. The speed of input over the serial line is limited by the baud rate settings, and even if you transfer at maximum speed all supported systems are fast enough to receive the data without loss. - The use case you are trying to support is actually multi-line input, so you should describe it as such. I agree that this would be an interesting feature for some use cases, but if we go on and implement it, we should (1) document it properly and (2) implement it in a way that works reliably. - However, your implementation does not result in reliable multi-line input. It works only in a few specific use cases, and will fail silently for others. I don't see a way ho you would announce this new feature. The numbers you mention in the commit message (it handles around 70 lines before lossage) apply for this specific board only, and for your use case only (pasting of short lines that produce no output). So essentially you are saying: hey, we now have multi-line input, but it works only a bit. It will fail sooner or later, without error messages, and I cannot even tell you what the limits for your system are. And it depends on which input you send. This does not exactly sound promising. On the other hand, we also have the rule that things that are useful to some and that don't hurt others should be allowed to go in. What makes me hesitate are two things: - The patch promises a feature (multi-line input), but fails to provide a reliably working implementation. - As it turns out, the patch increases code size even for boards that do not activate this feature. I have to admit that I'm at a loss with a decision here. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de We don't have to protect the environment -- the Second Coming is at hand. - James Watt ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v4 2/2] NS16550: buffer reads
Dear Simon, I wrote: - However, your implementation does not result in reliable multi-line input. It works only in a few specific use cases, and will fail silently for others. I don't see a way ho you would announce this new feature. The numbers you mention in the commit message (it handles around 70 lines before lossage) apply for this specific board only, and for your use case only (pasting of short lines that produce no output). Actually I believe that the restriction is even worse - I think that any commands that consume any significant amount of time will casue problems, too. Can you please test what happens when you have, for example, a sleep 10 or a erase all in the first few lines of the pasted input ? Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de What can it profit a man to gain the whole world and to come to his property with a gastric ulcer, a blown prostate, and bifocals? -- John Steinbeck, _Cannery Row_ ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v4 2/2] NS16550: buffer reads
Hi Wolfgang, On Mon, Oct 17, 2011 at 4:08 AM, Wolfgang Denk w...@denx.de wrote: Dear Simon Glass, In message 1318742050-2201-2-git-send-email-...@chromium.org you wrote: ... With it enabled, on ppc it's an extra 396 bytes of image size, and 1056 bytes of BSS. ARM note from Simon Glass s...@chromium.org - ARM code size goes from 212 to 484 bytes (extra 272 bytes), BSS to 1056 bytes. One problem I see is that the code size on some boards grows even if the new option is not enabled. Here a compare before and after applying your patches (without any changes to the respective board configurations): Configuring for AR405 board... text data bss dec hex filename - 246058 12972 14636 273666 42d02 /work/wd/tmp-ppc/u-boot + 246062 12972 14636 273670 42d06 /work/wd/tmp-ppc/u-boot Configuring for CANBT board... text data bss dec hex filename - 120710 8604 3876 133190 20846 /work/wd/tmp-ppc/u-boot + 120714 8604 3876 133194 2084a /work/wd/tmp-ppc/u-boot Configuring for pcs440ep board... text data bss dec hex filename - 296191 19636 345088 660915 a15b3 /work/wd/tmp-ppc/u-boot + 296195 19636 345088 660919 a15b7 /work/wd/tmp-ppc/u-boot ... This doesn't happen for me on ARM - the compiler just inlines the new static raw functions into the public functions. I will see if I can figure out what is happening on PPC. It is 4 bytes, or one instruction, right? The fix might be to duplicate more code, or perhaps add an inline keyword to the two functions. Regards, Simon Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Out of register space (ugh) - vi ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v4 2/2] NS16550: buffer reads
Hi Wolfgang, On Mon, Oct 17, 2011 at 5:18 AM, Wolfgang Denk w...@denx.de wrote: Dear Simon, In message 1318742050-2201-2-git-send-email-...@chromium.org you wrote: This improves the performance of U-Boot when accepting rapid input, such as pasting a sequence of commands. Without this patch, on P4080DS I see a maximum of around 5 lines can be pasted. With this patch, it handles around 70 lines before lossage, long enough for most things you'd paste. Let me try to summarize my thinking: - It is a fundamental design decision that U-Boot is single tasking. This implies that while a command is running, no other things get done in parallel. This includes communication over the serial line, USB, Ethernet, ... - That means that by design U-Boot does not support multi-line input: as soon as you hit enter U-Boot will start executing your command, and will only become ready for new input when it has completed the execution of the command(s), i. e. after printing the next prompt. - For this suported mode of operation your patch is not needed. It just adds code size and complexity. - Your description rapid input is actually wrong. The speed of input over the serial line is limited by the baud rate settings, and even if you transfer at maximum speed all supported systems are fast enough to receive the data without loss. - The use case you are trying to support is actually multi-line input, so you should describe it as such. I agree that this would be an interesting feature for some use cases, but if we go on and implement it, we should (1) document it properly and (2) implement it in a way that works reliably. I can certainly improve the documentation and commit message. - However, your implementation does not result in reliable multi-line input. It works only in a few specific use cases, and will fail silently for others. I don't see a way ho you would announce this new feature. The numbers you mention in the commit message (it handles around 70 lines before lossage) apply for this specific board only, and for your use case only (pasting of short lines that produce no output). So essentially you are saying: hey, we now have multi-line input, but it works only a bit. It will fail sooner or later, without error messages, and I cannot even tell you what the limits for your system are. And it depends on which input you send. This does not exactly sound promising. That all sounds reasonable. I think it is good enough, but not perfect. Due to the fundamental design decision you mention at the top, we cannot squirrel away serial input in the background. The best we can do is squirrel it away in the foreground, as we output new serial data (I suppose we could squirrel it away in net loops and other places but I don't want to go there!). This turns out to be mostly good enough, because it is rare for people to want to paste 'sleep 10' and the like into their terminal (your other message). I would like to spit out a warning when serial input is lost - as I mentioned that could be combined with a serial overhaul at some point. On the other hand, we also have the rule that things that are useful to some and that don't hurt others should be allowed to go in. What makes me hesitate are two things: - The patch promises a feature (multi-line input), but fails to provide a reliably working implementation. I *think* it does the best possible within the fundamental design decision constraint. If there is more it can do, please let me have your ideas. I don't believe buffering conflicts with the constraint - they are separate things. But yes in systems with interrupts normally the input buffer is filled in the background and drained in the foreground. - As it turns out, the patch increases code size even for boards that do not activate this feature. Yes, I will take a look at this problem. I have to admit that I'm at a loss with a decision here. Well it's not easy being a maintainer :-) In any case this patch is not the end of the story as serial needs some work - another objection you didn't mention above is that this function lives in only one driver. Is that a good thing (hide it away) or a bad thing (all drivers should support it and the implementation should move up a level)? Regards, Simon Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de We don't have to protect the environment -- the Second Coming is at hand. - James Watt ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v4 2/2] NS16550: buffer reads
On Mon, Oct 17, 2011 at 4:08 AM, Wolfgang Denk w...@denx.de wrote: Dear Simon Glass, In message 1318742050-2201-2-git-send-email-...@chromium.org you wrote: ... With it enabled, on ppc it's an extra 396 bytes of image size, and 1056 bytes of BSS. ARM note from Simon Glass s...@chromium.org - ARM code size goes from 212 to 484 bytes (extra 272 bytes), BSS to 1056 bytes. One problem I see is that the code size on some boards grows even if the new option is not enabled. Here a compare before and after applying your patches (without any changes to the respective board configurations): Configuring for AR405 board... text data bss dec hex filename - 246058 12972 14636 273666 42d02 /work/wd/tmp-ppc/u-boot + 246062 12972 14636 273670 42d06 /work/wd/tmp-ppc/u-boot Configuring for CANBT board... text data bss dec hex filename - 120710 8604 3876 133190 20846 /work/wd/tmp-ppc/u-boot + 120714 8604 3876 133194 2084a /work/wd/tmp-ppc/u-boot Configuring for pcs440ep board... text data bss dec hex filename - 296191 19636 345088 660915 a15b3 /work/wd/tmp-ppc/u-boot + 296195 19636 345088 660919 a15b7 /work/wd/tmp-ppc/u-boot ... Hi Wolfgang, Can you please tell me which ELDK version this is using? I see that the 405 board seems to need ppc_4xx which suggests 4.2 rather than 5, since in 5 the compiler is called powerpc- But I might be confused. Regards, Simon Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Out of register space (ugh) - vi ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v4 2/2] NS16550: buffer reads
Dear Simon Glass, In message capnjgz1412ezjh3f4ww16k4z55kjdggwj7+vmyk_ot-mzvd...@mail.gmail.com you wrote: Can you please tell me which ELDK version this is using? I see that the 405 board seems to need ppc_4xx which suggests 4.2 rather than 5, since in 5 the compiler is called powerpc- Right, I was testing this with ELDK 4.2. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de You can observe a lot just by watchin'. - Yogi Berra ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v4 2/2] NS16550: buffer reads
Hi Wolfgang, On Mon, Oct 17, 2011 at 1:33 PM, Wolfgang Denk w...@denx.de wrote: Dear Simon Glass, In message capnjgz1412ezjh3f4ww16k4z55kjdggwj7+vmyk_ot-mzvd...@mail.gmail.com you wrote: Can you please tell me which ELDK version this is using? I see that the 405 board seems to need ppc_4xx which suggests 4.2 rather than 5, since in 5 the compiler is called powerpc- Right, I was testing this with ELDK 4.2. I am struggling to repeat this and don't even get the same numbers... For your AR405 board, you saw: - 246058 12972 14636 273666 42d02 /work/wd/tmp-ppc/u-boot + 246062 12972 14636 273670 42d06 /work/wd/tmp-ppc/u-boot For me: ppc_4xx-gcc -v Reading specs from /opt/eldk-4.2.4xx/usr/bin/../lib/gcc/powerpc-linux/4.2.2/specs Target: powerpc-linux Configured with: /opt/eldk/build/ppc-2008-04-01/work/usr/src/denx/BUILD/crosstool-0.43/build/gcc-4.2.2-glibc-20070515T2025-eldk/powerpc-linux/gcc-4.2.2/configure --target=powerpc-linux --host=i686-host_pc-linux-gnu --prefix=/var/tmp/eldk.UZpAG7/usr/crosstool/gcc-4.2.2-glibc-20070515T2025-eldk/powerpc-linux --disable-hosted-libstdcxx --with-headers=/var/tmp/eldk.UZpAG7/usr/crosstool/gcc-4.2.2-glibc-20070515T2025-eldk/powerpc-linux/powerpc-linux/include --with-local-prefix=/var/tmp/eldk.UZpAG7/usr/crosstool/gcc-4.2.2-glibc-20070515T2025-eldk/powerpc-linux/powerpc-linux --disable-nls --enable-threads=posix --enable-symvers=gnu --enable-__cxa_atexit --enable-languages=c,c++,java --enable-shared --enable-c99 --enable-long-long --without-x Thread model: posix gcc version 4.2.2 ARCH=powerpc make O=ppc AR405_config ARCH=powerpc make O=ppc without feature (checkout of upstream/master d8fffa05): $ ppc_4xx-objdump -h ppc/drivers/serial/ns16550.o ... 7 .text.NS16550_tstc 0020 0bf4 2**2 CONTENTS, ALLOC, LOAD, READONLY, CODE 8 .text.NS16550_putc 002c 0c14 2**2 CONTENTS, ALLOC, LOAD, READONLY, CODE 9 .text.NS16550_getc 0038 0c40 2**2 CONTENTS, ALLOC, LOAD, READONLY, CODE 10 .text.NS16550_init 007c 0c78 2**2 CONTENTS, ALLOC, LOAD, READONLY, CODE 11 .text.NS16550_reinit 0080 0cf4 2**2 CONTENTS, ALLOC, LOAD, READONLY, CODE textdata bss dec hex filename 245942 12964 14632 273538 42c82 ppc/u-boot with feature but not enabled (with Scott's two patches): 7 .text.NS16550_putc 002c 0fec 2**2 CONTENTS, ALLOC, LOAD, READONLY, CODE 8 .text.NS16550_getc 0038 1018 2**2 CONTENTS, ALLOC, LOAD, READONLY, CODE 9 .text.NS16550_tstc 0020 1050 2**2 CONTENTS, ALLOC, LOAD, READONLY, CODE 10 .text.NS16550_init 007c 1070 2**2 CONTENTS, ALLOC, LOAD, READONLY, CODE 11 .text.NS16550_reinit 0080 10ec 2**2 CONTENTS, ALLOC, LOAD, READONLY, CODE size ppc/u-boot textdata bss dec hex filename 245942 12964 14632 273538 42c82 ppc/u-boot Do you have any suggestions please? Regards, Simon Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de You can observe a lot just by watchin'. - Yogi Berra ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v4 2/2] NS16550: buffer reads
Hi Simon, Le 16/10/2011 07:14, Simon Glass a écrit : From: Scott Woodscottw...@freescale.com From: Scott Woodscottw...@freescale.com This improves the performance of U-Boot when accepting rapid input, such as pasting a sequence of commands. Without this patch, on P4080DS I see a maximum of around 5 lines can be pasted. With this patch, it handles around 70 lines before lossage, long enough for most things you'd paste. With it enabled, on ppc it's an extra 396 bytes of image size, and 1056 bytes of BSS. ARM note from Simon Glasss...@chromium.org - ARM code size goes from 212 to 484 bytes (extra 272 bytes), BSS to 1056 bytes. Signed-off-by: Scott Woodscottw...@freescale.com --- Changes in v2: - Fix checkpatch warnings, the other one was already there Changes in v3: - Use CONFIG_NS16550_BUFFER_READS to set the buffer size also Changes in v4: - Change config option to CONFIG_NS16550_RBUF_SIZE - Add additional checkpatch-cleanup patch Hmm... What about the conversation around V2 of the patch re: using XOFF/XON to control input flow? IIUC, even this V4 patch would not help much if there is a command within the pasted code that sends a lot of output, right? Amicalement, -- Albert. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v4 2/2] NS16550: buffer reads
Hi Albert, On Sun, Oct 16, 2011 at 5:57 AM, Albert ARIBAUD albert.u.b...@aribaud.net wrote: Hi Simon, Le 16/10/2011 07:14, Simon Glass a écrit : From: Scott Woodscottw...@freescale.com From: Scott Woodscottw...@freescale.com This improves the performance of U-Boot when accepting rapid input, such as pasting a sequence of commands. Without this patch, on P4080DS I see a maximum of around 5 lines can be pasted. With this patch, it handles around 70 lines before lossage, long enough for most things you'd paste. With it enabled, on ppc it's an extra 396 bytes of image size, and 1056 bytes of BSS. ARM note from Simon Glasss...@chromium.org - ARM code size goes from 212 to 484 bytes (extra 272 bytes), BSS to 1056 bytes. Signed-off-by: Scott Woodscottw...@freescale.com --- Changes in v2: - Fix checkpatch warnings, the other one was already there Changes in v3: - Use CONFIG_NS16550_BUFFER_READS to set the buffer size also Changes in v4: - Change config option to CONFIG_NS16550_RBUF_SIZE - Add additional checkpatch-cleanup patch Hmm... What about the conversation around V2 of the patch re: using XOFF/XON to control input flow? IIUC, even this V4 patch would not help much if there is a command within the pasted code that sends a lot of output, right? Well it will help so long as there isn't much more input. Perhaps the way to think of it is that every character you output that isn't just an echo, puts you one more character behind the input. This adds one character to the buffer in this patch. When you have fallen (say) 256 characters behind the input, then you will start losing characters even with this patch. However it is rare to paste in commands which send lots of output - and 3 lines of full output is pretty uncommon in my experience. I feel that relying on the first level UART FIFO as the only buffering is brave, and cannot be justified by lack of interrupts alone. There seems to me to be no fundamental reason why U-Boot should not buffer its input for when it is latest ready to process it. Granted this should perhaps be higher up the software stack, but until someone tidies up serial a little I suspect that would be a pain to implement. With a more generic serial layer (a device pointer and a -priv pointer across all drivers for example) we could attach this buffering as a separate file to be enabled / disabled by a Makefile. To address your question, I think we did discuss it. Wolfgang felt that it should be as simple as sending XOFF when receiving the end of a line and XON when waiting for characters on the new line. It certainly sounds plausible. I have other reasons for favouring this patch. On the main hardware platform I currently use, SPI and the console UART are multiplexed, so before using SPI (e.g. saveenv, reading the environment) we must read in any pending characters to avoid corruption. After re-enabling the UART later we must clear out any junk that has arrived. This patch provides a way around that which XON/XOFF does not (since when you send XOFF you may already have input characters outstanding - junk will later be added and you can't tell the difference). I am unsure whether to bring this up or not - clearly this platform is not ideal, but it is in very common use and we need to support it with U-Boot. Regards, Simon Amicalement, -- Albert. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v4 2/2] NS16550: buffer reads
Dear Simon Glass, In message CAPnjgZ2Q3XtGK=5hhf0khqo4xqda6cah3qfkfmgcwznv_y0...@mail.gmail.com you wrote: Hmm... What about the conversation around V2 of the patch re: using XOFF/XON to control input flow? IIUC, even this V4 patch would not help much if there is a command within the pasted code that sends a lot of output, right? Well it will help so long as there isn't much more input. Perhaps the Don't you see that this as long as there isn't much more input is the fatal flaw of your patch? You are not fixing the problem, you are just extending the point wher eit hits to some extend - it may work for a few more situations, but it's still guaranteed to fail for others. To address your question, I think we did discuss it. Wolfgang felt that it should be as simple as sending XOFF when receiving the end of a line and XON when waiting for characters on the new line. It certainly sounds plausible. Then please let's take this approach instead, if you really need to support such a mode of operation. I have other reasons for favouring this patch. On the main hardware platform I currently use, SPI and the console UART are multiplexed, so before using SPI (e.g. saveenv, reading the environment) we must read in any pending characters to avoid corruption. After re-enabling the UART later we must clear out any junk that has arrived. This patch provides a way around that which XON/XOFF does not (since when you send XOFF you may already have input characters outstanding - junk will later be added and you can't tell the difference). That's an unrelated hardware issue that needs to be addressed independently. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Most legends have their basis in facts. -- Kirk, And The Children Shall Lead, stardate 5029.5 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v4 2/2] NS16550: buffer reads
Hi Wolfgang, On Sun, Oct 16, 2011 at 1:13 PM, Wolfgang Denk w...@denx.de wrote: Dear Simon Glass, In message CAPnjgZ2Q3XtGK=5hhf0khqo4xqda6cah3qfkfmgcwznv_y0...@mail.gmail.com you wrote: Hmm... What about the conversation around V2 of the patch re: using XOFF/XON to control input flow? IIUC, even this V4 patch would not help much if there is a command within the pasted code that sends a lot of output, right? Well it will help so long as there isn't much more input. Perhaps the Don't you see that this as long as there isn't much more input is the fatal flaw of your patch? You are not fixing the problem, you are just extending the point wher eit hits to some extend - it may work for a few more situations, but it's still guaranteed to fail for others. In a similar way, the Linux kernel has a fatal flaw. Serial data coming into the flip buffers under extreme interrupt load can be lost, or the secondary buffers can become exhausted, or user space cannot keep up with input under heavy load, etc., etc. This is the reality of the world. As each problem presents itself we direct our attention to it. To address your question, I think we did discuss it. Wolfgang felt that it should be as simple as sending XOFF when receiving the end of a line and XON when waiting for characters on the new line. It certainly sounds plausible. Then please let's take this approach instead, if you really need to support such a mode of operation. As I said I will look at this. I have other reasons for favouring this patch. On the main hardware platform I currently use, SPI and the console UART are multiplexed, so before using SPI (e.g. saveenv, reading the environment) we must read in any pending characters to avoid corruption. After re-enabling the UART later we must clear out any junk that has arrived. This patch provides a way around that which XON/XOFF does not (since when you send XOFF you may already have input characters outstanding - junk will later be added and you can't tell the difference). That's an unrelated hardware issue that needs to be addressed independently. Yes it is. I can't help wondering what the solution might be though, if we are not allowed to buffer things. Regards, Simon Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Most legends have their basis in facts. -- Kirk, And The Children Shall Lead, stardate 5029.5 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v4 2/2] NS16550: buffer reads
Dear Simon Glass, In message capnjgz1vuf-hfbdx9byuv32sjfee3skrit124gfgqom0f+q...@mail.gmail.com you wrote: In a similar way, the Linux kernel has a fatal flaw. Serial data coming into the flip buffers under extreme interrupt load can be lost, or the secondary buffers can become exhausted, or user space cannot keep up with input under heavy load, etc., etc. This is the reality of the world. As each problem presents itself we direct our attention to it. This is not a fatal flaw. There is nothing in the definition of RS232 or the serialinterface that guarantees you an error free data stream - not on that protocol level. You are facing loss of Ethernet packages on the network as well. Problems start only when you make wrong assumptions. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de A girl with a future avoids the man with a past. -- Evan Esar, The Humor of Humor ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot