| Message ID | 1436757876-29691-2-git-send-email-heyi.guo@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 13 July 2015 at 13:15, Laszlo Ersek <lersek@redhat.com> wrote: > On 07/13/15 05:24, Heyi Guo wrote: >> Change default terminal type to be consistent with default >> ConIn/ConOut device path, which is now determined by TTY_TERMINAL >> flag, TTYTERM or VT100. >> >> I can't say this is a bug, as we can pass the whole console device >> path to ConnectController, and TerminalDxe driver will pick up the >> terminal in the remaining device path. However, in rare circumstances, >> the console devices may be disconnected with the driver, and they will >> be ignored by ConPlatformDxe until we pass the device path explicitly >> just as BDS. >> >> Changing default terminal type to be the same with console device >> path could help serial terminal be reconnected with normal connect >> controller operation. >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Heyi Guo <heyi.guo@linaro.org> >> --- >> ArmVirtPkg/ArmVirtQemu.dsc | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc >> index fbc2b12..e62624f 100644 >> --- a/ArmVirtPkg/ArmVirtQemu.dsc >> +++ b/ArmVirtPkg/ArmVirtQemu.dsc >> @@ -141,9 +141,15 @@ >> !if $(TTY_TERMINAL) == TRUE >> gArmPlatformTokenSpaceGuid.PcdDefaultConOutPaths|L"VenHw(D3987D4B-971A-435F-8CAF-4967EB627241)/Uart(38400,8,N,1)/VenMsg(7D916D80-5BB1-458C-A48F-E25FDD51EF94)" >> gArmPlatformTokenSpaceGuid.PcdDefaultConInPaths|L"VenHw(D3987D4B-971A-435F-8CAF-4967EB627241)/Uart(38400,8,N,1)/VenMsg(7D916D80-5BB1-458C-A48F-E25FDD51EF94)" >> + ## Terminal Type - TTY, consistent with ConOut/ConIn Device Path. >> + gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType|4 >> !else >> gArmPlatformTokenSpaceGuid.PcdDefaultConOutPaths|L"VenHw(D3987D4B-971A-435F-8CAF-4967EB627241)/Uart(38400,8,N,1)/VenVt100()" >> gArmPlatformTokenSpaceGuid.PcdDefaultConInPaths|L"VenHw(D3987D4B-971A-435F-8CAF-4967EB627241)/Uart(38400,8,N,1)/VenVt100()" >> + ## Terminal Type - VT100, consistent with ConOut/ConIn Device Path. >> + ## When Intel BDS is enabled, the above ConOut/ConIn device path is useless, >> + ## but we still use VT100 terminal type when TTY_TERMINAL is not TRUE. >> + gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType|1 >> !endif >> gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|3 >> >> > > Reviewed-by: Laszlo Ersek <lersek@redhat.com> I am OK with the patch. However, the value '4' is not defined for PcdDefaultTerminalType is not defined in MdePkg. Is that a concern to anyone? I am aware that we suggested to Roy not to touch MdePkg with his TtyTerm changes, but it seems odd to just use '4' here ------------------------------------------------------------------------------ Don't Limit Your Business. Reach for the Cloud. GigeNET's Cloud Solutions provide you with the tools and support that you need to offload your IT needs and focus on growing your business. Configured For All Businesses. Start Your Cloud Today. https://www.gigenetcloud.com/
On 14 July 2015 at 12:15, Laszlo Ersek <lersek@redhat.com> wrote: > On 07/13/15 13:34, Laszlo Ersek wrote: >> On 07/13/15 13:22, Ard Biesheuvel wrote: >>> On 13 July 2015 at 13:15, Laszlo Ersek <lersek@redhat.com> wrote: >>>> On 07/13/15 05:24, Heyi Guo wrote: >>>>> Change default terminal type to be consistent with default >>>>> ConIn/ConOut device path, which is now determined by TTY_TERMINAL >>>>> flag, TTYTERM or VT100. >>>>> >>>>> I can't say this is a bug, as we can pass the whole console device >>>>> path to ConnectController, and TerminalDxe driver will pick up the >>>>> terminal in the remaining device path. However, in rare circumstances, >>>>> the console devices may be disconnected with the driver, and they will >>>>> be ignored by ConPlatformDxe until we pass the device path explicitly >>>>> just as BDS. >>>>> >>>>> Changing default terminal type to be the same with console device >>>>> path could help serial terminal be reconnected with normal connect >>>>> controller operation. >>>>> >>>>> Contributed-under: TianoCore Contribution Agreement 1.0 >>>>> Signed-off-by: Heyi Guo <heyi.guo@linaro.org> >>>>> --- >>>>> ArmVirtPkg/ArmVirtQemu.dsc | 6 ++++++ >>>>> 1 file changed, 6 insertions(+) >>>>> >>>>> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc >>>>> index fbc2b12..e62624f 100644 >>>>> --- a/ArmVirtPkg/ArmVirtQemu.dsc >>>>> +++ b/ArmVirtPkg/ArmVirtQemu.dsc >>>>> @@ -141,9 +141,15 @@ >>>>> !if $(TTY_TERMINAL) == TRUE >>>>> gArmPlatformTokenSpaceGuid.PcdDefaultConOutPaths|L"VenHw(D3987D4B-971A-435F-8CAF-4967EB627241)/Uart(38400,8,N,1)/VenMsg(7D916D80-5BB1-458C-A48F-E25FDD51EF94)" >>>>> gArmPlatformTokenSpaceGuid.PcdDefaultConInPaths|L"VenHw(D3987D4B-971A-435F-8CAF-4967EB627241)/Uart(38400,8,N,1)/VenMsg(7D916D80-5BB1-458C-A48F-E25FDD51EF94)" >>>>> + ## Terminal Type - TTY, consistent with ConOut/ConIn Device Path. >>>>> + gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType|4 >>>>> !else >>>>> gArmPlatformTokenSpaceGuid.PcdDefaultConOutPaths|L"VenHw(D3987D4B-971A-435F-8CAF-4967EB627241)/Uart(38400,8,N,1)/VenVt100()" >>>>> gArmPlatformTokenSpaceGuid.PcdDefaultConInPaths|L"VenHw(D3987D4B-971A-435F-8CAF-4967EB627241)/Uart(38400,8,N,1)/VenVt100()" >>>>> + ## Terminal Type - VT100, consistent with ConOut/ConIn Device Path. >>>>> + ## When Intel BDS is enabled, the above ConOut/ConIn device path is useless, >>>>> + ## but we still use VT100 terminal type when TTY_TERMINAL is not TRUE. >>>>> + gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType|1 >>>>> !endif >>>>> gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|3 >>>>> >>>>> >>>> >>>> Reviewed-by: Laszlo Ersek <lersek@redhat.com> >>> >>> I am OK with the patch. However, the value '4' is not defined for >>> PcdDefaultTerminalType is not defined in MdePkg. >>> Is that a concern to anyone? I am aware that we suggested to Roy not >>> to touch MdePkg with his TtyTerm changes, but it seems odd to just use >>> '4' here >>> >> >> See "MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h", commit >> 6e3227c8. (The TTYTERMTYPE macro.) >> >> What should belong to MdePkg (but can't, unless it gets standardized) is >> the textual representation for the new VenMsg(GUID). >> >> ... Hm, okay, you do have a point, I just checked "MdePkg/MdePkg.dec", >> and it does list the values 0-3. However, the PCD is not used inside >> MdePkg, it is only declared there. >> >> So, I think that this patch is okay. If another terminal type gets >> standardized *before* Roy's TTYTERM, then: >> - that type will get a new (different) GUID, >> - and it will get value 4 in "MdePkg/MdePkg.dec". (And, in Terminal.h.) >> >> The new GUID will obviously not conflict, and the current TTYTERMTYPE=4 >> constant will be possible to shift up to TTYTERMTYPE=5, at that point. >> Yes, referring DSCs will have to be updated as well. >> >> This is not 100% "clean", admittedly. I think the root issue is that the >> PCD should not be declared in MdePkg at all; at least (off the top off >> my head) I doubt the numeric value comes from the UEFI spec. So, maybe >> someone can submit a patch that moves the PCD declaration from MdePkg to >> MdeModulePkg. Otherwise, I'd be fine with the above "plan". So my R-b >> stands. > > So what do you think, Ard? With Feng's R-b for patches #2 and #3, we > could commit this series to SVN -- if you agree with #1. > Yes, I am fine with this series. Please go ahead and commit it, or I can do it instead if you're busy. In the mean time, I will follow up with a patch that moves PcdDefaultTerminalType to MdeModulePkg, and one that adds the missing #4 in the comments. Cheers, Ard. ------------------------------------------------------------------------------ Don't Limit Your Business. Reach for the Cloud. GigeNET's Cloud Solutions provide you with the tools and support that you need to offload your IT needs and focus on growing your business. Configured For All Businesses. Start Your Cloud Today. https://www.gigenetcloud.com/
On 14 July 2015 at 12:18, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 14 July 2015 at 12:15, Laszlo Ersek <lersek@redhat.com> wrote: >> On 07/13/15 13:34, Laszlo Ersek wrote: >>> On 07/13/15 13:22, Ard Biesheuvel wrote: >>>> On 13 July 2015 at 13:15, Laszlo Ersek <lersek@redhat.com> wrote: >>>>> On 07/13/15 05:24, Heyi Guo wrote: >>>>>> Change default terminal type to be consistent with default >>>>>> ConIn/ConOut device path, which is now determined by TTY_TERMINAL >>>>>> flag, TTYTERM or VT100. >>>>>> >>>>>> I can't say this is a bug, as we can pass the whole console device >>>>>> path to ConnectController, and TerminalDxe driver will pick up the >>>>>> terminal in the remaining device path. However, in rare circumstances, >>>>>> the console devices may be disconnected with the driver, and they will >>>>>> be ignored by ConPlatformDxe until we pass the device path explicitly >>>>>> just as BDS. >>>>>> >>>>>> Changing default terminal type to be the same with console device >>>>>> path could help serial terminal be reconnected with normal connect >>>>>> controller operation. >>>>>> >>>>>> Contributed-under: TianoCore Contribution Agreement 1.0 >>>>>> Signed-off-by: Heyi Guo <heyi.guo@linaro.org> >>>>>> --- >>>>>> ArmVirtPkg/ArmVirtQemu.dsc | 6 ++++++ >>>>>> 1 file changed, 6 insertions(+) >>>>>> >>>>>> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc >>>>>> index fbc2b12..e62624f 100644 >>>>>> --- a/ArmVirtPkg/ArmVirtQemu.dsc >>>>>> +++ b/ArmVirtPkg/ArmVirtQemu.dsc >>>>>> @@ -141,9 +141,15 @@ >>>>>> !if $(TTY_TERMINAL) == TRUE >>>>>> gArmPlatformTokenSpaceGuid.PcdDefaultConOutPaths|L"VenHw(D3987D4B-971A-435F-8CAF-4967EB627241)/Uart(38400,8,N,1)/VenMsg(7D916D80-5BB1-458C-A48F-E25FDD51EF94)" >>>>>> gArmPlatformTokenSpaceGuid.PcdDefaultConInPaths|L"VenHw(D3987D4B-971A-435F-8CAF-4967EB627241)/Uart(38400,8,N,1)/VenMsg(7D916D80-5BB1-458C-A48F-E25FDD51EF94)" >>>>>> + ## Terminal Type - TTY, consistent with ConOut/ConIn Device Path. >>>>>> + gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType|4 >>>>>> !else >>>>>> gArmPlatformTokenSpaceGuid.PcdDefaultConOutPaths|L"VenHw(D3987D4B-971A-435F-8CAF-4967EB627241)/Uart(38400,8,N,1)/VenVt100()" >>>>>> gArmPlatformTokenSpaceGuid.PcdDefaultConInPaths|L"VenHw(D3987D4B-971A-435F-8CAF-4967EB627241)/Uart(38400,8,N,1)/VenVt100()" >>>>>> + ## Terminal Type - VT100, consistent with ConOut/ConIn Device Path. >>>>>> + ## When Intel BDS is enabled, the above ConOut/ConIn device path is useless, >>>>>> + ## but we still use VT100 terminal type when TTY_TERMINAL is not TRUE. >>>>>> + gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType|1 >>>>>> !endif >>>>>> gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|3 >>>>>> >>>>>> >>>>> >>>>> Reviewed-by: Laszlo Ersek <lersek@redhat.com> >>>> >>>> I am OK with the patch. However, the value '4' is not defined for >>>> PcdDefaultTerminalType is not defined in MdePkg. >>>> Is that a concern to anyone? I am aware that we suggested to Roy not >>>> to touch MdePkg with his TtyTerm changes, but it seems odd to just use >>>> '4' here >>>> >>> >>> See "MdeModulePkg/Universal/Console/TerminalDxe/Terminal.h", commit >>> 6e3227c8. (The TTYTERMTYPE macro.) >>> >>> What should belong to MdePkg (but can't, unless it gets standardized) is >>> the textual representation for the new VenMsg(GUID). >>> >>> ... Hm, okay, you do have a point, I just checked "MdePkg/MdePkg.dec", >>> and it does list the values 0-3. However, the PCD is not used inside >>> MdePkg, it is only declared there. >>> >>> So, I think that this patch is okay. If another terminal type gets >>> standardized *before* Roy's TTYTERM, then: >>> - that type will get a new (different) GUID, >>> - and it will get value 4 in "MdePkg/MdePkg.dec". (And, in Terminal.h.) >>> >>> The new GUID will obviously not conflict, and the current TTYTERMTYPE=4 >>> constant will be possible to shift up to TTYTERMTYPE=5, at that point. >>> Yes, referring DSCs will have to be updated as well. >>> >>> This is not 100% "clean", admittedly. I think the root issue is that the >>> PCD should not be declared in MdePkg at all; at least (off the top off >>> my head) I doubt the numeric value comes from the UEFI spec. So, maybe >>> someone can submit a patch that moves the PCD declaration from MdePkg to >>> MdeModulePkg. Otherwise, I'd be fine with the above "plan". So my R-b >>> stands. >> >> So what do you think, Ard? With Feng's R-b for patches #2 and #3, we >> could commit this series to SVN -- if you agree with #1. >> > > Yes, I am fine with this series. Please go ahead and commit it, or I > can do it instead if you're busy. > > In the mean time, I will follow up with a patch that moves > PcdDefaultTerminalType to MdeModulePkg, and one that adds the missing > #4 in the comments. > On second thought, perhaps it might be slightly cleaner to introduce gEfiMdeModulePkgTokenSpaceGuid.PcdDefaultTerminalType first and move existing code in TerminalDxe to it? ------------------------------------------------------------------------------ Don't Limit Your Business. Reach for the Cloud. GigeNET's Cloud Solutions provide you with the tools and support that you need to offload your IT needs and focus on growing your business. Configured For All Businesses. Start Your Cloud Today. https://www.gigenetcloud.com/
No problem :) Heyi On 07/15/2015 07:20 PM, Laszlo Ersek wrote: > Heyi, > > On 07/13/15 13:22, Ard Biesheuvel wrote: >> On 13 July 2015 at 13:15, Laszlo Ersek <lersek@redhat.com> wrote: >>> On 07/13/15 05:24, Heyi Guo wrote: >>>> Change default terminal type to be consistent with default >>>> ConIn/ConOut device path, which is now determined by TTY_TERMINAL >>>> flag, TTYTERM or VT100. >>>> >>>> I can't say this is a bug, as we can pass the whole console device >>>> path to ConnectController, and TerminalDxe driver will pick up the >>>> terminal in the remaining device path. However, in rare circumstances, >>>> the console devices may be disconnected with the driver, and they will >>>> be ignored by ConPlatformDxe until we pass the device path explicitly >>>> just as BDS. >>>> >>>> Changing default terminal type to be the same with console device >>>> path could help serial terminal be reconnected with normal connect >>>> controller operation. >>>> >>>> Contributed-under: TianoCore Contribution Agreement 1.0 >>>> Signed-off-by: Heyi Guo <heyi.guo@linaro.org> >>>> --- >>>> ArmVirtPkg/ArmVirtQemu.dsc | 6 ++++++ >>>> 1 file changed, 6 insertions(+) >>>> >>>> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc >>>> index fbc2b12..e62624f 100644 >>>> --- a/ArmVirtPkg/ArmVirtQemu.dsc >>>> +++ b/ArmVirtPkg/ArmVirtQemu.dsc >>>> @@ -141,9 +141,15 @@ >>>> !if $(TTY_TERMINAL) == TRUE >>>> gArmPlatformTokenSpaceGuid.PcdDefaultConOutPaths|L"VenHw(D3987D4B-971A-435F-8CAF-4967EB627241)/Uart(38400,8,N,1)/VenMsg(7D916D80-5BB1-458C-A48F-E25FDD51EF94)" >>>> gArmPlatformTokenSpaceGuid.PcdDefaultConInPaths|L"VenHw(D3987D4B-971A-435F-8CAF-4967EB627241)/Uart(38400,8,N,1)/VenMsg(7D916D80-5BB1-458C-A48F-E25FDD51EF94)" >>>> + ## Terminal Type - TTY, consistent with ConOut/ConIn Device Path. >>>> + gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType|4 >>>> !else >>>> gArmPlatformTokenSpaceGuid.PcdDefaultConOutPaths|L"VenHw(D3987D4B-971A-435F-8CAF-4967EB627241)/Uart(38400,8,N,1)/VenVt100()" >>>> gArmPlatformTokenSpaceGuid.PcdDefaultConInPaths|L"VenHw(D3987D4B-971A-435F-8CAF-4967EB627241)/Uart(38400,8,N,1)/VenVt100()" >>>> + ## Terminal Type - VT100, consistent with ConOut/ConIn Device Path. >>>> + ## When Intel BDS is enabled, the above ConOut/ConIn device path is useless, >>>> + ## but we still use VT100 terminal type when TTY_TERMINAL is not TRUE. >>>> + gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType|1 >>>> !endif >>>> gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|3 >>>> >>>> >>> Reviewed-by: Laszlo Ersek <lersek@redhat.com> >> I am OK with the patch. However, the value '4' is not defined for >> PcdDefaultTerminalType is not defined in MdePkg. >> Is that a concern to anyone? I am aware that we suggested to Roy not >> to touch MdePkg with his TtyTerm changes, but it seems odd to just use >> '4' here >> > can you please prepend another patch to this series that updates the > PcdDefaultTerminalType declaration in MdePkg/MdePkg.dec, with the new > value 4, and a hint that TTYTERM, identified by value 4, is not defined > by the UEFI spec? > > Please see here (unless you've seen it already... hm, right, you were Cc'd): > > http://thread.gmane.org/gmane.comp.bios.tianocore.devel/17359/focus=17421 > > Thanks! > Laszlo ------------------------------------------------------------------------------ Don't Limit Your Business. Reach for the Cloud. GigeNET's Cloud Solutions provide you with the tools and support that you need to offload your IT needs and focus on growing your business. Configured For All Businesses. Start Your Cloud Today. https://www.gigenetcloud.com/
diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc index fbc2b12..e62624f 100644 --- a/ArmVirtPkg/ArmVirtQemu.dsc +++ b/ArmVirtPkg/ArmVirtQemu.dsc @@ -141,9 +141,15 @@ !if $(TTY_TERMINAL) == TRUE gArmPlatformTokenSpaceGuid.PcdDefaultConOutPaths|L"VenHw(D3987D4B-971A-435F-8CAF-4967EB627241)/Uart(38400,8,N,1)/VenMsg(7D916D80-5BB1-458C-A48F-E25FDD51EF94)" gArmPlatformTokenSpaceGuid.PcdDefaultConInPaths|L"VenHw(D3987D4B-971A-435F-8CAF-4967EB627241)/Uart(38400,8,N,1)/VenMsg(7D916D80-5BB1-458C-A48F-E25FDD51EF94)" + ## Terminal Type - TTY, consistent with ConOut/ConIn Device Path. + gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType|4 !else gArmPlatformTokenSpaceGuid.PcdDefaultConOutPaths|L"VenHw(D3987D4B-971A-435F-8CAF-4967EB627241)/Uart(38400,8,N,1)/VenVt100()" gArmPlatformTokenSpaceGuid.PcdDefaultConInPaths|L"VenHw(D3987D4B-971A-435F-8CAF-4967EB627241)/Uart(38400,8,N,1)/VenVt100()" + ## Terminal Type - VT100, consistent with ConOut/ConIn Device Path. + ## When Intel BDS is enabled, the above ConOut/ConIn device path is useless, + ## but we still use VT100 terminal type when TTY_TERMINAL is not TRUE. + gEfiMdePkgTokenSpaceGuid.PcdDefaultTerminalType|1 !endif gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|3
Change default terminal type to be consistent with default ConIn/ConOut device path, which is now determined by TTY_TERMINAL flag, TTYTERM or VT100. I can't say this is a bug, as we can pass the whole console device path to ConnectController, and TerminalDxe driver will pick up the terminal in the remaining device path. However, in rare circumstances, the console devices may be disconnected with the driver, and they will be ignored by ConPlatformDxe until we pass the device path explicitly just as BDS. Changing default terminal type to be the same with console device path could help serial terminal be reconnected with normal connect controller operation. Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Heyi Guo <heyi.guo@linaro.org> --- ArmVirtPkg/ArmVirtQemu.dsc | 6 ++++++ 1 file changed, 6 insertions(+)