> On Jan 15, 2016, at 11:14 AM, Ryan Harkin <ryan.har...@linaro.org> wrote: > > On 15 January 2016 at 18:32, Laszlo Ersek <ler...@redhat.com> wrote: >> On 01/15/16 18:59, Ryan Harkin wrote: >>> On 15 January 2016 at 17:39, Laszlo Ersek <ler...@redhat.com> wrote: >>>> On 01/15/16 18:33, Ryan Harkin wrote: >>>>> On 15 January 2016 at 17:05, Laszlo Ersek <ler...@redhat.com> wrote: >> >>>>>> Ryan, can you please test the following patch? >>>>>> >>>>>>> diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c >>>>>>> b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c >>>>>>> index 6fde3b2..e1a6527 100644 >>>>>>> --- a/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c >>>>>>> +++ b/MdeModulePkg/Universal/Console/TerminalDxe/Terminal.c >>>>>>> @@ -806,7 +806,7 @@ TerminalDriverBindingStart ( >>>>>>> Status = TerminalDevice->SerialIo->SetAttributes ( >>>>>>> TerminalDevice->SerialIo, >>>>>>> Mode->BaudRate, >>>>>>> - Mode->ReceiveFifoDepth, >>>>>>> + 0, // device's default FIFO >>>>>>> depth. >>>>>>> (UINT32) SerialInTimeOut, >>>>>>> (EFI_PARITY_TYPE) >>>>>>> (Mode->Parity), >>>>>>> (UINT8) Mode->DataBits, >>>>>>> diff --git a/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c >>>>>>> b/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c >>>>>>> index 3be877b..ffdc8f2 100644 >>>>>>> --- a/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c >>>>>>> +++ b/MdeModulePkg/Universal/Console/TerminalDxe/TerminalConIn.c >>>>>>> @@ -547,7 +547,7 @@ TerminalConInTimerHandler ( >>>>>>> Status = SerialIo->SetAttributes ( >>>>>>> SerialIo, >>>>>>> Mode->BaudRate, >>>>>>> - Mode->ReceiveFifoDepth, >>>>>>> + 0, // device's default FIFO depth. >>>>>>> (UINT32) SerialInTimeOut, >>>>>>> (EFI_PARITY_TYPE) (Mode->Parity), >>>>>>> (UINT8) Mode->DataBits, >>>>>> >>>>> >>>>> Yes, this works for me. >>>> >>>> Thanks for the testing (to Ard too)! >>>> >>>>> So what do you think the proper solution is? Is Ard's patch not what >>>>> you were thinking? >>>> >>>> I think the UEFI spec is clear enough on EFI_SERIAL_IO_PROTOCOL that we >>>> can derive what the right thing is: >>>> >>>> - SerialDxe needs a fix so that not only its ReceiveFifoDepth defaults >>>> are correct (beause they are correct now!), but also the Timeout defaults. >>>> >>>> - TerminalDxe needs a fix too: it should recognize the defaults set by >>>> SerialDxe (as per UEFI spec requirements), and set ReceiveFifoDepth >>>> explicitly, with a SetAttributes() call. A good value for that looks >>>> like zero (= device's default FIFO depth). I believe the value zero also >>>> happens to restore the previous behavior, in practice -- it's just not >>>> SerialDxe's responsibility. >>>> >>> >>> The problem I have is that is someone decides that TerminalDxe needs >>> to set the depth to 1 again, or some other value, the ARM platforms >>> will break, because they clearly can set that value, but it isn't >>> useful/usable. So perhaps PL011 needs to use the device's default >>> value, irrespective of what TerminalDxe asks for? >> >> "irrespective" would be a violation of the UEFI spec (end to end); > > What if the spec changes and the device cannot support the spec?
Generally the specification changes are additive and backward comparable. The higher level features of spec generally use verbiage like if the platform supports feature X it must implement it with protocol Y. > Surely it has to do something sane? > Return EFI_UNSUPPORTED Thanks, Andrew Fish >> the >> Serial IO Protocol implementation must not set a larger queue depth than >> what the client requests. If the client requests 1, the driver must not >> set 16 (for example). >> > > Even if it cannot support the value requested? > > >> In my reading, the Serial IO Protocol spec is reasonably flexible, and >> considers the case when a given serial port cannot provide the exact >> queue depth requested. I think if the SerialDxe driver sticks to the >> UEFI spec requirements, and the TerminalDxe driver also assumes nothing >> more and nothing less (about the Serial IO Protocol) than specified, >> then things should "just work". >> >> Your concern applies to all drivers and libraries under MdeModulePkg -- >> some of the central modules have the potential to break any and all >> platforms. I don't think we should try to work around that in >> platform-specific libraries that the central modules delegate some work >> to, especially if the UEFI spec goes into some detail on the relevant >> behavior. >> >> Anyway, I wouldn't like to force my opinion on others -- I just thought >> it was an interesting problem, with the solution coming more or less >> directly from the spec. Admittedly, there are places where edk2 deviates >> from the spec. At the moment I think it wouldn't be justified here. >> > > To be fair, I don't know why I'm getting involved. I'd simply like to > rebase to the tip of Tianocore and for it to actually work. > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
