Re: [PATCH 03/17] x86: Allow coreboot serial driver to guess the UART
Hi Simon, On Sat, May 8, 2021 at 10:12 AM Simon Glass wrote: > > Hi Bin, > > On Fri, 7 May 2021 at 18:48, Bin Meng wrote: > > > > Hi Simon, > > > > On Sat, May 8, 2021 at 9:42 AM Simon Glass wrote: > > > > > > Hi Bin, > > > > > > On Tue, 4 May 2021 at 08:26, Simon Glass wrote: > > > > > > > > Hi Bin and Andy, > > > > > > > > On Fri, 30 Apr 2021 at 12:41, Andy Shevchenko > > > > wrote: > > > > > > > > > > On Fri, Apr 30, 2021 at 9:14 PM Simon Glass wrote: > > > > > > On Thu, 29 Apr 2021 at 17:01, Bin Meng wrote: > > > > > > > On Fri, Apr 30, 2021 at 12:10 AM Simon Glass > > > > > > > wrote: > > > > > > > > On Sun, 25 Apr 2021 at 18:21, Bin Meng > > > > > > > > wrote: > > > > > > > > > On Sun, Apr 25, 2021 at 10:10 AM Simon Glass > > > > > > > > > wrote: > > > > > > > > > > On Sun, 25 Apr 2021 at 13:49, Bin Meng > > > > > > > > > > wrote: > > > > > > > > > > > On Sat, Apr 24, 2021 at 12:56 PM Simon Glass > > > > > > > > > > > wrote: > > > > > > > > > > > > On Thu, 8 Apr 2021 at 14:23, Bin Meng > > > > > > > > > > > > wrote: > > > > > > > > > > > > > On Wed, Apr 7, 2021 at 12:32 PM Simon Glass > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > At present this driver relies on coreboot to > > > > > > > > > > > > > > provide information about > > > > > > > > > > > > > > the console UART. However if coreboot is not > > > > > > > > > > > > > > compiled with the UART > > > > > > > > > > > > > > enabled, the information is left out. This > > > > > > > > > > > > > > configuration is quite > > > > > > > > > > > > > > common, e.g. with shipping x86-based Chrome OS > > > > > > > > > > > > > > Chromebooks. > > > > > > > > > > > > > > > > > > > > > > > > > > > > Add a way to determine the UART settings in this > > > > > > > > > > > > > > case, using a > > > > > > > > > > > > > > hard-coded list of PCI IDs. > > > > > > > > > > I don't like this either, so -1 from me. > > > > > > > > > > What coreboot should do is either provide serial information or SPCR > > > > > ACPI table. > > > > > Otherwise if it does not provide it, I think it's on purpose, and we > > > > > have to respect this. > > > > > > > > > > The patch is ugly hack in my opinion. Sorry. > > > > > > > > > > You may make it less ugly by checking PCI class rather than IDs. > > > > > > > > I have not been able to distinguish a pattern on Intel SoCs yet. > > > > Perhaps you can help with that as there must be a way... > > > > > > > > > > > > > > > > > > > > > > > > > > Why not just simply put a serial node in the device > > > > > > > > > > > > > tree and we are all done? > > > > > > > > > > > > > > > > > > > > > > > > See my other email...I am trying to make this boot on > > > > > > > > > > > > any board that > > > > > > > > > > > > coreboot supports. > > > > > > > > > > > > > > > > > > > > > > But this solution does not scale. One has to put all > > > > > > > > > > > known UARTs into > > > > > > > > > > > serial_coreboot.c. > > > > > > > > > > > > > > > > > > > > Yes that's right...but this is only for when coreboot does > > > > > > > > > > not enable > > > > > > > > > > serial. Also the number of new platforms is not that great. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Why not patch coreboot instead? Why coreboot does not > > > > > > > > > > > provide a cbinfo > > > > > > > > > > > with serial? > > > > > > > > > > > > > > > > > > > > Because it does not even set up the serial device in that > > > > > > > > > > case, so > > > > > > > > > > doesn't know the details. The driver is completely missing. > > > > > > > > > > > > > > > > > > Sigh. Is it possible to upstream a patch to coreboot to > > > > > > > > > enable that? > > > > > > > > > > > > > > > > Well I'm not even sure upstream coreboot boots on the various > > > > > > > > Chromebooks I am targetting. If it does, then serial is probablt > > > > > > > > enabled. But certainly for chromebooks, it is not. My goal is > > > > > > > > to have > > > > > > > > U-Boot boot on a chromebook in altfw mode with serial enabled. > > > > > > > > > > > > > > > > > > > > > > > > > > I don't like the current approach because it ends up > > > > > > > > > duplicating all > > > > > > > > > UART IDs/info in C. > > > > > > > > > > > > > > > > Yes. Do you think it would be better to put it in the > > > > > > > > devicetree? I > > > > > > > > suppose we could add some more stuff to the compatible string, > > > > > > > > although U-Boot does not support the PCI compatible strings at > > > > > > > > present. > > > > > > > > > > > > > > Putting it in the device tree also looks odd, because it only > > > > > > > matters > > > > > > > on a dedicated board. > > > > > > > > > > > > Right, but that is the nature of trying to run the same image on > > > > > > different hardware. > > > > > > > > > > > > > > > > > > > > > What do you suggest? > > > > > > > > > > > > > > Or parse the ACPI table coreboot has set up? But that might be > > > > > > >
Re: [PATCH 03/17] x86: Allow coreboot serial driver to guess the UART
Hi Bin, On Fri, 7 May 2021 at 18:48, Bin Meng wrote: > > Hi Simon, > > On Sat, May 8, 2021 at 9:42 AM Simon Glass wrote: > > > > Hi Bin, > > > > On Tue, 4 May 2021 at 08:26, Simon Glass wrote: > > > > > > Hi Bin and Andy, > > > > > > On Fri, 30 Apr 2021 at 12:41, Andy Shevchenko > > > wrote: > > > > > > > > On Fri, Apr 30, 2021 at 9:14 PM Simon Glass wrote: > > > > > On Thu, 29 Apr 2021 at 17:01, Bin Meng wrote: > > > > > > On Fri, Apr 30, 2021 at 12:10 AM Simon Glass > > > > > > wrote: > > > > > > > On Sun, 25 Apr 2021 at 18:21, Bin Meng wrote: > > > > > > > > On Sun, Apr 25, 2021 at 10:10 AM Simon Glass > > > > > > > > wrote: > > > > > > > > > On Sun, 25 Apr 2021 at 13:49, Bin Meng > > > > > > > > > wrote: > > > > > > > > > > On Sat, Apr 24, 2021 at 12:56 PM Simon Glass > > > > > > > > > > wrote: > > > > > > > > > > > On Thu, 8 Apr 2021 at 14:23, Bin Meng > > > > > > > > > > > wrote: > > > > > > > > > > > > On Wed, Apr 7, 2021 at 12:32 PM Simon Glass > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > At present this driver relies on coreboot to provide > > > > > > > > > > > > > information about > > > > > > > > > > > > > the console UART. However if coreboot is not compiled > > > > > > > > > > > > > with the UART > > > > > > > > > > > > > enabled, the information is left out. This > > > > > > > > > > > > > configuration is quite > > > > > > > > > > > > > common, e.g. with shipping x86-based Chrome OS > > > > > > > > > > > > > Chromebooks. > > > > > > > > > > > > > > > > > > > > > > > > > > Add a way to determine the UART settings in this > > > > > > > > > > > > > case, using a > > > > > > > > > > > > > hard-coded list of PCI IDs. > > > > > > > > I don't like this either, so -1 from me. > > > > > > > > What coreboot should do is either provide serial information or SPCR > > > > ACPI table. > > > > Otherwise if it does not provide it, I think it's on purpose, and we > > > > have to respect this. > > > > > > > > The patch is ugly hack in my opinion. Sorry. > > > > > > > > You may make it less ugly by checking PCI class rather than IDs. > > > > > > I have not been able to distinguish a pattern on Intel SoCs yet. > > > Perhaps you can help with that as there must be a way... > > > > > > > > > > > > > > > > > > > > > > Why not just simply put a serial node in the device > > > > > > > > > > > > tree and we are all done? > > > > > > > > > > > > > > > > > > > > > > See my other email...I am trying to make this boot on any > > > > > > > > > > > board that > > > > > > > > > > > coreboot supports. > > > > > > > > > > > > > > > > > > > > But this solution does not scale. One has to put all known > > > > > > > > > > UARTs into > > > > > > > > > > serial_coreboot.c. > > > > > > > > > > > > > > > > > > Yes that's right...but this is only for when coreboot does > > > > > > > > > not enable > > > > > > > > > serial. Also the number of new platforms is not that great. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Why not patch coreboot instead? Why coreboot does not > > > > > > > > > > provide a cbinfo > > > > > > > > > > with serial? > > > > > > > > > > > > > > > > > > Because it does not even set up the serial device in that > > > > > > > > > case, so > > > > > > > > > doesn't know the details. The driver is completely missing. > > > > > > > > > > > > > > > > Sigh. Is it possible to upstream a patch to coreboot to enable > > > > > > > > that? > > > > > > > > > > > > > > Well I'm not even sure upstream coreboot boots on the various > > > > > > > Chromebooks I am targetting. If it does, then serial is probablt > > > > > > > enabled. But certainly for chromebooks, it is not. My goal is to > > > > > > > have > > > > > > > U-Boot boot on a chromebook in altfw mode with serial enabled. > > > > > > > > > > > > > > > > > > > > > > > I don't like the current approach because it ends up > > > > > > > > duplicating all > > > > > > > > UART IDs/info in C. > > > > > > > > > > > > > > Yes. Do you think it would be better to put it in the devicetree? > > > > > > > I > > > > > > > suppose we could add some more stuff to the compatible string, > > > > > > > although U-Boot does not support the PCI compatible strings at > > > > > > > present. > > > > > > > > > > > > Putting it in the device tree also looks odd, because it only > > > > > > matters > > > > > > on a dedicated board. > > > > > > > > > > Right, but that is the nature of trying to run the same image on > > > > > different hardware. > > > > > > > > > > > > > > > > > > What do you suggest? > > > > > > > > > > > > Or parse the ACPI table coreboot has set up? But that might be > > > > > > another > > > > > > huge monster :( > > > > > > > > > > Yes, even worse... > > > > > > All of the suggestions so far do not solve the problem, so we are left > > > without serial output, or something else...? > > > > In the interested of getting the patches applied, shall we just drop > > this one? I will revisit
Re: [PATCH 03/17] x86: Allow coreboot serial driver to guess the UART
Hi Simon, On Sat, May 8, 2021 at 9:42 AM Simon Glass wrote: > > Hi Bin, > > On Tue, 4 May 2021 at 08:26, Simon Glass wrote: > > > > Hi Bin and Andy, > > > > On Fri, 30 Apr 2021 at 12:41, Andy Shevchenko > > wrote: > > > > > > On Fri, Apr 30, 2021 at 9:14 PM Simon Glass wrote: > > > > On Thu, 29 Apr 2021 at 17:01, Bin Meng wrote: > > > > > On Fri, Apr 30, 2021 at 12:10 AM Simon Glass > > > > > wrote: > > > > > > On Sun, 25 Apr 2021 at 18:21, Bin Meng wrote: > > > > > > > On Sun, Apr 25, 2021 at 10:10 AM Simon Glass > > > > > > > wrote: > > > > > > > > On Sun, 25 Apr 2021 at 13:49, Bin Meng > > > > > > > > wrote: > > > > > > > > > On Sat, Apr 24, 2021 at 12:56 PM Simon Glass > > > > > > > > > wrote: > > > > > > > > > > On Thu, 8 Apr 2021 at 14:23, Bin Meng > > > > > > > > > > wrote: > > > > > > > > > > > On Wed, Apr 7, 2021 at 12:32 PM Simon Glass > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > At present this driver relies on coreboot to provide > > > > > > > > > > > > information about > > > > > > > > > > > > the console UART. However if coreboot is not compiled > > > > > > > > > > > > with the UART > > > > > > > > > > > > enabled, the information is left out. This > > > > > > > > > > > > configuration is quite > > > > > > > > > > > > common, e.g. with shipping x86-based Chrome OS > > > > > > > > > > > > Chromebooks. > > > > > > > > > > > > > > > > > > > > > > > > Add a way to determine the UART settings in this case, > > > > > > > > > > > > using a > > > > > > > > > > > > hard-coded list of PCI IDs. > > > > > > I don't like this either, so -1 from me. > > > > > > What coreboot should do is either provide serial information or SPCR ACPI > > > table. > > > Otherwise if it does not provide it, I think it's on purpose, and we > > > have to respect this. > > > > > > The patch is ugly hack in my opinion. Sorry. > > > > > > You may make it less ugly by checking PCI class rather than IDs. > > > > I have not been able to distinguish a pattern on Intel SoCs yet. > > Perhaps you can help with that as there must be a way... > > > > > > > > > > > > > > > > > > Why not just simply put a serial node in the device tree > > > > > > > > > > > and we are all done? > > > > > > > > > > > > > > > > > > > > See my other email...I am trying to make this boot on any > > > > > > > > > > board that > > > > > > > > > > coreboot supports. > > > > > > > > > > > > > > > > > > But this solution does not scale. One has to put all known > > > > > > > > > UARTs into > > > > > > > > > serial_coreboot.c. > > > > > > > > > > > > > > > > Yes that's right...but this is only for when coreboot does not > > > > > > > > enable > > > > > > > > serial. Also the number of new platforms is not that great. > > > > > > > > > > > > > > > > > > > > > > > > > > Why not patch coreboot instead? Why coreboot does not provide > > > > > > > > > a cbinfo > > > > > > > > > with serial? > > > > > > > > > > > > > > > > Because it does not even set up the serial device in that case, > > > > > > > > so > > > > > > > > doesn't know the details. The driver is completely missing. > > > > > > > > > > > > > > Sigh. Is it possible to upstream a patch to coreboot to enable > > > > > > > that? > > > > > > > > > > > > Well I'm not even sure upstream coreboot boots on the various > > > > > > Chromebooks I am targetting. If it does, then serial is probablt > > > > > > enabled. But certainly for chromebooks, it is not. My goal is to > > > > > > have > > > > > > U-Boot boot on a chromebook in altfw mode with serial enabled. > > > > > > > > > > > > > > > > > > > > I don't like the current approach because it ends up duplicating > > > > > > > all > > > > > > > UART IDs/info in C. > > > > > > > > > > > > Yes. Do you think it would be better to put it in the devicetree? I > > > > > > suppose we could add some more stuff to the compatible string, > > > > > > although U-Boot does not support the PCI compatible strings at > > > > > > present. > > > > > > > > > > Putting it in the device tree also looks odd, because it only matters > > > > > on a dedicated board. > > > > > > > > Right, but that is the nature of trying to run the same image on > > > > different hardware. > > > > > > > > > > > > > > > What do you suggest? > > > > > > > > > > Or parse the ACPI table coreboot has set up? But that might be another > > > > > huge monster :( > > > > > > > > Yes, even worse... > > > > All of the suggestions so far do not solve the problem, so we are left > > without serial output, or something else...? > > In the interested of getting the patches applied, shall we just drop > this one? I will revisit it when I try the next platform and see if I > can find a different pattern. I think we should drop this patch for now, and apply other patches in this series. But I suspect the rest patches cannot be applied cleanly if dropping this patch. Regards, Bin
Re: [PATCH 03/17] x86: Allow coreboot serial driver to guess the UART
Hi Bin, On Tue, 4 May 2021 at 08:26, Simon Glass wrote: > > Hi Bin and Andy, > > On Fri, 30 Apr 2021 at 12:41, Andy Shevchenko > wrote: > > > > On Fri, Apr 30, 2021 at 9:14 PM Simon Glass wrote: > > > On Thu, 29 Apr 2021 at 17:01, Bin Meng wrote: > > > > On Fri, Apr 30, 2021 at 12:10 AM Simon Glass wrote: > > > > > On Sun, 25 Apr 2021 at 18:21, Bin Meng wrote: > > > > > > On Sun, Apr 25, 2021 at 10:10 AM Simon Glass > > > > > > wrote: > > > > > > > On Sun, 25 Apr 2021 at 13:49, Bin Meng wrote: > > > > > > > > On Sat, Apr 24, 2021 at 12:56 PM Simon Glass > > > > > > > > wrote: > > > > > > > > > On Thu, 8 Apr 2021 at 14:23, Bin Meng > > > > > > > > > wrote: > > > > > > > > > > On Wed, Apr 7, 2021 at 12:32 PM Simon Glass > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > At present this driver relies on coreboot to provide > > > > > > > > > > > information about > > > > > > > > > > > the console UART. However if coreboot is not compiled > > > > > > > > > > > with the UART > > > > > > > > > > > enabled, the information is left out. This configuration > > > > > > > > > > > is quite > > > > > > > > > > > common, e.g. with shipping x86-based Chrome OS > > > > > > > > > > > Chromebooks. > > > > > > > > > > > > > > > > > > > > > > Add a way to determine the UART settings in this case, > > > > > > > > > > > using a > > > > > > > > > > > hard-coded list of PCI IDs. > > > > I don't like this either, so -1 from me. > > > > What coreboot should do is either provide serial information or SPCR ACPI > > table. > > Otherwise if it does not provide it, I think it's on purpose, and we > > have to respect this. > > > > The patch is ugly hack in my opinion. Sorry. > > > > You may make it less ugly by checking PCI class rather than IDs. > > I have not been able to distinguish a pattern on Intel SoCs yet. > Perhaps you can help with that as there must be a way... > > > > > > > > > > > > > > Why not just simply put a serial node in the device tree > > > > > > > > > > and we are all done? > > > > > > > > > > > > > > > > > > See my other email...I am trying to make this boot on any > > > > > > > > > board that > > > > > > > > > coreboot supports. > > > > > > > > > > > > > > > > But this solution does not scale. One has to put all known > > > > > > > > UARTs into > > > > > > > > serial_coreboot.c. > > > > > > > > > > > > > > Yes that's right...but this is only for when coreboot does not > > > > > > > enable > > > > > > > serial. Also the number of new platforms is not that great. > > > > > > > > > > > > > > > > > > > > > > > Why not patch coreboot instead? Why coreboot does not provide a > > > > > > > > cbinfo > > > > > > > > with serial? > > > > > > > > > > > > > > Because it does not even set up the serial device in that case, so > > > > > > > doesn't know the details. The driver is completely missing. > > > > > > > > > > > > Sigh. Is it possible to upstream a patch to coreboot to enable that? > > > > > > > > > > Well I'm not even sure upstream coreboot boots on the various > > > > > Chromebooks I am targetting. If it does, then serial is probablt > > > > > enabled. But certainly for chromebooks, it is not. My goal is to have > > > > > U-Boot boot on a chromebook in altfw mode with serial enabled. > > > > > > > > > > > > > > > > > I don't like the current approach because it ends up duplicating all > > > > > > UART IDs/info in C. > > > > > > > > > > Yes. Do you think it would be better to put it in the devicetree? I > > > > > suppose we could add some more stuff to the compatible string, > > > > > although U-Boot does not support the PCI compatible strings at > > > > > present. > > > > > > > > Putting it in the device tree also looks odd, because it only matters > > > > on a dedicated board. > > > > > > Right, but that is the nature of trying to run the same image on > > > different hardware. > > > > > > > > > > > > What do you suggest? > > > > > > > > Or parse the ACPI table coreboot has set up? But that might be another > > > > huge monster :( > > > > > > Yes, even worse... > > All of the suggestions so far do not solve the problem, so we are left > without serial output, or something else...? In the interested of getting the patches applied, shall we just drop this one? I will revisit it when I try the next platform and see if I can find a different pattern. Regards, Simon
Re: [PATCH 03/17] x86: Allow coreboot serial driver to guess the UART
On Tue, May 04, 2021 at 09:26:19AM -0600, Simon Glass wrote: > On Fri, 30 Apr 2021 at 12:41, Andy Shevchenko > wrote: > > On Fri, Apr 30, 2021 at 9:14 PM Simon Glass wrote: > > > On Thu, 29 Apr 2021 at 17:01, Bin Meng wrote: > > > > On Fri, Apr 30, 2021 at 12:10 AM Simon Glass wrote: > > > > > On Sun, 25 Apr 2021 at 18:21, Bin Meng wrote: > > > > > > On Sun, Apr 25, 2021 at 10:10 AM Simon Glass > > > > > > wrote: > > > > > > > On Sun, 25 Apr 2021 at 13:49, Bin Meng wrote: > > > > > > > > On Sat, Apr 24, 2021 at 12:56 PM Simon Glass > > > > > > > > wrote: > > > > > > > > > On Thu, 8 Apr 2021 at 14:23, Bin Meng > > > > > > > > > wrote: > > > > > > > > > > On Wed, Apr 7, 2021 at 12:32 PM Simon Glass > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > At present this driver relies on coreboot to provide > > > > > > > > > > > information about > > > > > > > > > > > the console UART. However if coreboot is not compiled > > > > > > > > > > > with the UART > > > > > > > > > > > enabled, the information is left out. This configuration > > > > > > > > > > > is quite > > > > > > > > > > > common, e.g. with shipping x86-based Chrome OS > > > > > > > > > > > Chromebooks. > > > > > > > > > > > > > > > > > > > > > > Add a way to determine the UART settings in this case, > > > > > > > > > > > using a > > > > > > > > > > > hard-coded list of PCI IDs. > > > > I don't like this either, so -1 from me. > > > > What coreboot should do is either provide serial information or SPCR ACPI > > table. > > Otherwise if it does not provide it, I think it's on purpose, and we > > have to respect this. > > > > The patch is ugly hack in my opinion. Sorry. > > > > You may make it less ugly by checking PCI class rather than IDs. > > I have not been able to distinguish a pattern on Intel SoCs yet. > Perhaps you can help with that as there must be a way... > > > > > > > > > > > > > > Why not just simply put a serial node in the device tree > > > > > > > > > > and we are all done? > > > > > > > > > > > > > > > > > > See my other email...I am trying to make this boot on any > > > > > > > > > board that > > > > > > > > > coreboot supports. > > > > > > > > > > > > > > > > But this solution does not scale. One has to put all known > > > > > > > > UARTs into > > > > > > > > serial_coreboot.c. > > > > > > > > > > > > > > Yes that's right...but this is only for when coreboot does not > > > > > > > enable > > > > > > > serial. Also the number of new platforms is not that great. > > > > > > > > > > > > > > > > > > > > > > > Why not patch coreboot instead? Why coreboot does not provide a > > > > > > > > cbinfo > > > > > > > > with serial? > > > > > > > > > > > > > > Because it does not even set up the serial device in that case, so > > > > > > > doesn't know the details. The driver is completely missing. > > > > > > > > > > > > Sigh. Is it possible to upstream a patch to coreboot to enable that? > > > > > > > > > > Well I'm not even sure upstream coreboot boots on the various > > > > > Chromebooks I am targetting. If it does, then serial is probablt > > > > > enabled. But certainly for chromebooks, it is not. My goal is to have > > > > > U-Boot boot on a chromebook in altfw mode with serial enabled. > > > > > > > > > > > > > > > > > I don't like the current approach because it ends up duplicating all > > > > > > UART IDs/info in C. > > > > > > > > > > Yes. Do you think it would be better to put it in the devicetree? I > > > > > suppose we could add some more stuff to the compatible string, > > > > > although U-Boot does not support the PCI compatible strings at > > > > > present. > > > > > > > > Putting it in the device tree also looks odd, because it only matters > > > > on a dedicated board. > > > > > > Right, but that is the nature of trying to run the same image on > > > different hardware. > > > > > > > > > > > > What do you suggest? > > > > > > > > Or parse the ACPI table coreboot has set up? But that might be another > > > > huge monster :( > > > > > > Yes, even worse... > > All of the suggestions so far do not solve the problem, so we are left > without serial output, or something else...? You are truing to solve Coreboot issue in U-Boot, doesn't seem to be quite right. Yes, I prefer no serial than some "smart" hack. It will encourage people who want to have serial out of Coreboot to whine to coreboot project and see what they will do about it. -- With Best Regards, Andy Shevchenko
Re: [PATCH 03/17] x86: Allow coreboot serial driver to guess the UART
Hi Bin and Andy, On Fri, 30 Apr 2021 at 12:41, Andy Shevchenko wrote: > > On Fri, Apr 30, 2021 at 9:14 PM Simon Glass wrote: > > On Thu, 29 Apr 2021 at 17:01, Bin Meng wrote: > > > On Fri, Apr 30, 2021 at 12:10 AM Simon Glass wrote: > > > > On Sun, 25 Apr 2021 at 18:21, Bin Meng wrote: > > > > > On Sun, Apr 25, 2021 at 10:10 AM Simon Glass > > > > > wrote: > > > > > > On Sun, 25 Apr 2021 at 13:49, Bin Meng wrote: > > > > > > > On Sat, Apr 24, 2021 at 12:56 PM Simon Glass > > > > > > > wrote: > > > > > > > > On Thu, 8 Apr 2021 at 14:23, Bin Meng > > > > > > > > wrote: > > > > > > > > > On Wed, Apr 7, 2021 at 12:32 PM Simon Glass > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > At present this driver relies on coreboot to provide > > > > > > > > > > information about > > > > > > > > > > the console UART. However if coreboot is not compiled with > > > > > > > > > > the UART > > > > > > > > > > enabled, the information is left out. This configuration is > > > > > > > > > > quite > > > > > > > > > > common, e.g. with shipping x86-based Chrome OS Chromebooks. > > > > > > > > > > > > > > > > > > > > Add a way to determine the UART settings in this case, > > > > > > > > > > using a > > > > > > > > > > hard-coded list of PCI IDs. > > I don't like this either, so -1 from me. > > What coreboot should do is either provide serial information or SPCR ACPI > table. > Otherwise if it does not provide it, I think it's on purpose, and we > have to respect this. > > The patch is ugly hack in my opinion. Sorry. > > You may make it less ugly by checking PCI class rather than IDs. I have not been able to distinguish a pattern on Intel SoCs yet. Perhaps you can help with that as there must be a way... > > > > > > > > > > Why not just simply put a serial node in the device tree and > > > > > > > > > we are all done? > > > > > > > > > > > > > > > > See my other email...I am trying to make this boot on any board > > > > > > > > that > > > > > > > > coreboot supports. > > > > > > > > > > > > > > But this solution does not scale. One has to put all known UARTs > > > > > > > into > > > > > > > serial_coreboot.c. > > > > > > > > > > > > Yes that's right...but this is only for when coreboot does not > > > > > > enable > > > > > > serial. Also the number of new platforms is not that great. > > > > > > > > > > > > > > > > > > > > Why not patch coreboot instead? Why coreboot does not provide a > > > > > > > cbinfo > > > > > > > with serial? > > > > > > > > > > > > Because it does not even set up the serial device in that case, so > > > > > > doesn't know the details. The driver is completely missing. > > > > > > > > > > Sigh. Is it possible to upstream a patch to coreboot to enable that? > > > > > > > > Well I'm not even sure upstream coreboot boots on the various > > > > Chromebooks I am targetting. If it does, then serial is probablt > > > > enabled. But certainly for chromebooks, it is not. My goal is to have > > > > U-Boot boot on a chromebook in altfw mode with serial enabled. > > > > > > > > > > > > > > I don't like the current approach because it ends up duplicating all > > > > > UART IDs/info in C. > > > > > > > > Yes. Do you think it would be better to put it in the devicetree? I > > > > suppose we could add some more stuff to the compatible string, > > > > although U-Boot does not support the PCI compatible strings at > > > > present. > > > > > > Putting it in the device tree also looks odd, because it only matters > > > on a dedicated board. > > > > Right, but that is the nature of trying to run the same image on > > different hardware. > > > > > > > > > What do you suggest? > > > > > > Or parse the ACPI table coreboot has set up? But that might be another > > > huge monster :( > > > > Yes, even worse... All of the suggestions so far do not solve the problem, so we are left without serial output, or something else...? Regards, Simon
Re: [PATCH 03/17] x86: Allow coreboot serial driver to guess the UART
On Fri, Apr 30, 2021 at 9:14 PM Simon Glass wrote: > On Thu, 29 Apr 2021 at 17:01, Bin Meng wrote: > > On Fri, Apr 30, 2021 at 12:10 AM Simon Glass wrote: > > > On Sun, 25 Apr 2021 at 18:21, Bin Meng wrote: > > > > On Sun, Apr 25, 2021 at 10:10 AM Simon Glass wrote: > > > > > On Sun, 25 Apr 2021 at 13:49, Bin Meng wrote: > > > > > > On Sat, Apr 24, 2021 at 12:56 PM Simon Glass > > > > > > wrote: > > > > > > > On Thu, 8 Apr 2021 at 14:23, Bin Meng wrote: > > > > > > > > On Wed, Apr 7, 2021 at 12:32 PM Simon Glass > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > At present this driver relies on coreboot to provide > > > > > > > > > information about > > > > > > > > > the console UART. However if coreboot is not compiled with > > > > > > > > > the UART > > > > > > > > > enabled, the information is left out. This configuration is > > > > > > > > > quite > > > > > > > > > common, e.g. with shipping x86-based Chrome OS Chromebooks. > > > > > > > > > > > > > > > > > > Add a way to determine the UART settings in this case, using a > > > > > > > > > hard-coded list of PCI IDs. I don't like this either, so -1 from me. What coreboot should do is either provide serial information or SPCR ACPI table. Otherwise if it does not provide it, I think it's on purpose, and we have to respect this. The patch is ugly hack in my opinion. Sorry. You may make it less ugly by checking PCI class rather than IDs. > > > > > > > > Why not just simply put a serial node in the device tree and we > > > > > > > > are all done? > > > > > > > > > > > > > > See my other email...I am trying to make this boot on any board > > > > > > > that > > > > > > > coreboot supports. > > > > > > > > > > > > But this solution does not scale. One has to put all known UARTs > > > > > > into > > > > > > serial_coreboot.c. > > > > > > > > > > Yes that's right...but this is only for when coreboot does not enable > > > > > serial. Also the number of new platforms is not that great. > > > > > > > > > > > > > > > > > Why not patch coreboot instead? Why coreboot does not provide a > > > > > > cbinfo > > > > > > with serial? > > > > > > > > > > Because it does not even set up the serial device in that case, so > > > > > doesn't know the details. The driver is completely missing. > > > > > > > > Sigh. Is it possible to upstream a patch to coreboot to enable that? > > > > > > Well I'm not even sure upstream coreboot boots on the various > > > Chromebooks I am targetting. If it does, then serial is probablt > > > enabled. But certainly for chromebooks, it is not. My goal is to have > > > U-Boot boot on a chromebook in altfw mode with serial enabled. > > > > > > > > > > > I don't like the current approach because it ends up duplicating all > > > > UART IDs/info in C. > > > > > > Yes. Do you think it would be better to put it in the devicetree? I > > > suppose we could add some more stuff to the compatible string, > > > although U-Boot does not support the PCI compatible strings at > > > present. > > > > Putting it in the device tree also looks odd, because it only matters > > on a dedicated board. > > Right, but that is the nature of trying to run the same image on > different hardware. > > > > > > What do you suggest? > > > > Or parse the ACPI table coreboot has set up? But that might be another > > huge monster :( > > Yes, even worse... -- With Best Regards, Andy Shevchenko
Re: [PATCH 03/17] x86: Allow coreboot serial driver to guess the UART
Hi Bin,
On Thu, 29 Apr 2021 at 17:01, Bin Meng wrote:
>
> Hi Simon,
>
> On Fri, Apr 30, 2021 at 12:10 AM Simon Glass wrote:
> >
> > Hi Bin,
> >
> > On Sun, 25 Apr 2021 at 18:21, Bin Meng wrote:
> > >
> > > Hi Simon,
> > >
> > > On Sun, Apr 25, 2021 at 10:10 AM Simon Glass wrote:
> > > >
> > > > Hi Bin,
> > > >
> > > > On Sun, 25 Apr 2021 at 13:49, Bin Meng wrote:
> > > > >
> > > > > Hi Simon,
> > > > >
> > > > > On Sat, Apr 24, 2021 at 12:56 PM Simon Glass
> > > > > wrote:
> > > > > >
> > > > > > Hi Bin,
> > > > > >
> > > > > > On Thu, 8 Apr 2021 at 14:23, Bin Meng wrote:
> > > > > > >
> > > > > > > Hi Simon,
> > > > > > >
> > > > > > > On Wed, Apr 7, 2021 at 12:32 PM Simon Glass
> > > > > > > wrote:
> > > > > > > >
> > > > > > > > At present this driver relies on coreboot to provide
> > > > > > > > information about
> > > > > > > > the console UART. However if coreboot is not compiled with the
> > > > > > > > UART
> > > > > > > > enabled, the information is left out. This configuration is
> > > > > > > > quite
> > > > > > > > common, e.g. with shipping x86-based Chrome OS Chromebooks.
> > > > > > > >
> > > > > > > > Add a way to determine the UART settings in this case, using a
> > > > > > > > hard-coded list of PCI IDs.
> > > > > > > >
> > > > > > > > Signed-off-by: Simon Glass
> > > > > > > > ---
> > > > > > > >
> > > > > > > > drivers/serial/serial_coreboot.c | 68
> > > > > > > >
> > > > > > > > include/pci_ids.h| 1 +
> > > > > > > > 2 files changed, 61 insertions(+), 8 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/serial/serial_coreboot.c
> > > > > > > > b/drivers/serial/serial_coreboot.c
> > > > > > > > index de09c8681f5..4b4619432d8 100644
> > > > > > > > --- a/drivers/serial/serial_coreboot.c
> > > > > > > > +++ b/drivers/serial/serial_coreboot.c
> > > > > > > > @@ -11,19 +11,71 @@
> > > > > > > > #include
> > > > > > > > #include
> > > > > > > >
> > > > > > > > +static const struct pci_device_id ids[] = {
> > > > > > > > + { PCI_DEVICE(PCI_VENDOR_ID_INTEL,
> > > > > > > > PCI_DEVICE_ID_INTEL_APL_UART2) },
> > > > > > > > + {},
> > > > > > > > +};
> > > > > > > > +
> > > > > > > > +/*
> > > > > > > > + * Coreboot only sets up the UART if it uses it and doesn't
> > > > > > > > bother to put the
> > > > > > > > + * details in sysinfo if it doesn't. Try to guess in that
> > > > > > > > case, using devices
> > > > > > > > + * we know about
> > > > > > > > + *
> > > > > > > > + * @plat: Platform data to fill in
> > > > > > > > + * @return 0 if found, -ve if no UART was found
> > > > > > > > + */
> > > > > > > > +static int guess_uart(struct ns16550_plat *plat)
> > > > > > >
> > > > > > > This is really not a guess, but use a pre-configured platform
> > > > > > > data.
> > > > > > > Also this only work for Apollo Lake board, and will break other
> > > > > > > boards
> > > > > > > if they don't have cbinfo available.
> > > > > >
> > > > > > Which bit of it breaks other boards?
> > > > >
> > > > > I see it does not return the error code to the caller, that's okay ...
> > > > >
> > > > > >
> > > > > > >
> > > > > > > Why not just simply put a serial node in the device tree and we
> > > > > > > are all done?
> > > > > >
> > > > > > See my other email...I am trying to make this boot on any board that
> > > > > > coreboot supports.
> > > > >
> > > > > But this solution does not scale. One has to put all known UARTs into
> > > > > serial_coreboot.c.
> > > >
> > > > Yes that's right...but this is only for when coreboot does not enable
> > > > serial. Also the number of new platforms is not that great.
> > > >
> > > > >
> > > > > Why not patch coreboot instead? Why coreboot does not provide a cbinfo
> > > > > with serial?
> > > >
> > > > Because it does not even set up the serial device in that case, so
> > > > doesn't know the details. The driver is completely missing.
> > >
> > > Sigh. Is it possible to upstream a patch to coreboot to enable that?
> >
> > Well I'm not even sure upstream coreboot boots on the various
> > Chromebooks I am targetting. If it does, then serial is probablt
> > enabled. But certainly for chromebooks, it is not. My goal is to have
> > U-Boot boot on a chromebook in altfw mode with serial enabled.
> >
> > >
> > > I don't like the current approach because it ends up duplicating all
> > > UART IDs/info in C.
> >
> > Yes. Do you think it would be better to put it in the devicetree? I
> > suppose we could add some more stuff to the compatible string,
> > although U-Boot does not support the PCI compatible strings at
> > present.
>
> Putting it in the device tree also looks odd, because it only matters
> on a dedicated board.
Right, but that is the nature of trying to run the same image on
different hardware.
>
> > What do you suggest?
>
> Or parse the ACPI table coreboot has set up? But that might be another
> huge monster :(
Yes, even worse...
Regards,
Simon
Re: [PATCH 03/17] x86: Allow coreboot serial driver to guess the UART
Hi Simon,
On Fri, Apr 30, 2021 at 12:10 AM Simon Glass wrote:
>
> Hi Bin,
>
> On Sun, 25 Apr 2021 at 18:21, Bin Meng wrote:
> >
> > Hi Simon,
> >
> > On Sun, Apr 25, 2021 at 10:10 AM Simon Glass wrote:
> > >
> > > Hi Bin,
> > >
> > > On Sun, 25 Apr 2021 at 13:49, Bin Meng wrote:
> > > >
> > > > Hi Simon,
> > > >
> > > > On Sat, Apr 24, 2021 at 12:56 PM Simon Glass wrote:
> > > > >
> > > > > Hi Bin,
> > > > >
> > > > > On Thu, 8 Apr 2021 at 14:23, Bin Meng wrote:
> > > > > >
> > > > > > Hi Simon,
> > > > > >
> > > > > > On Wed, Apr 7, 2021 at 12:32 PM Simon Glass
> > > > > > wrote:
> > > > > > >
> > > > > > > At present this driver relies on coreboot to provide information
> > > > > > > about
> > > > > > > the console UART. However if coreboot is not compiled with the
> > > > > > > UART
> > > > > > > enabled, the information is left out. This configuration is quite
> > > > > > > common, e.g. with shipping x86-based Chrome OS Chromebooks.
> > > > > > >
> > > > > > > Add a way to determine the UART settings in this case, using a
> > > > > > > hard-coded list of PCI IDs.
> > > > > > >
> > > > > > > Signed-off-by: Simon Glass
> > > > > > > ---
> > > > > > >
> > > > > > > drivers/serial/serial_coreboot.c | 68
> > > > > > >
> > > > > > > include/pci_ids.h| 1 +
> > > > > > > 2 files changed, 61 insertions(+), 8 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/serial/serial_coreboot.c
> > > > > > > b/drivers/serial/serial_coreboot.c
> > > > > > > index de09c8681f5..4b4619432d8 100644
> > > > > > > --- a/drivers/serial/serial_coreboot.c
> > > > > > > +++ b/drivers/serial/serial_coreboot.c
> > > > > > > @@ -11,19 +11,71 @@
> > > > > > > #include
> > > > > > > #include
> > > > > > >
> > > > > > > +static const struct pci_device_id ids[] = {
> > > > > > > + { PCI_DEVICE(PCI_VENDOR_ID_INTEL,
> > > > > > > PCI_DEVICE_ID_INTEL_APL_UART2) },
> > > > > > > + {},
> > > > > > > +};
> > > > > > > +
> > > > > > > +/*
> > > > > > > + * Coreboot only sets up the UART if it uses it and doesn't
> > > > > > > bother to put the
> > > > > > > + * details in sysinfo if it doesn't. Try to guess in that case,
> > > > > > > using devices
> > > > > > > + * we know about
> > > > > > > + *
> > > > > > > + * @plat: Platform data to fill in
> > > > > > > + * @return 0 if found, -ve if no UART was found
> > > > > > > + */
> > > > > > > +static int guess_uart(struct ns16550_plat *plat)
> > > > > >
> > > > > > This is really not a guess, but use a pre-configured platform data.
> > > > > > Also this only work for Apollo Lake board, and will break other
> > > > > > boards
> > > > > > if they don't have cbinfo available.
> > > > >
> > > > > Which bit of it breaks other boards?
> > > >
> > > > I see it does not return the error code to the caller, that's okay ...
> > > >
> > > > >
> > > > > >
> > > > > > Why not just simply put a serial node in the device tree and we are
> > > > > > all done?
> > > > >
> > > > > See my other email...I am trying to make this boot on any board that
> > > > > coreboot supports.
> > > >
> > > > But this solution does not scale. One has to put all known UARTs into
> > > > serial_coreboot.c.
> > >
> > > Yes that's right...but this is only for when coreboot does not enable
> > > serial. Also the number of new platforms is not that great.
> > >
> > > >
> > > > Why not patch coreboot instead? Why coreboot does not provide a cbinfo
> > > > with serial?
> > >
> > > Because it does not even set up the serial device in that case, so
> > > doesn't know the details. The driver is completely missing.
> >
> > Sigh. Is it possible to upstream a patch to coreboot to enable that?
>
> Well I'm not even sure upstream coreboot boots on the various
> Chromebooks I am targetting. If it does, then serial is probablt
> enabled. But certainly for chromebooks, it is not. My goal is to have
> U-Boot boot on a chromebook in altfw mode with serial enabled.
>
> >
> > I don't like the current approach because it ends up duplicating all
> > UART IDs/info in C.
>
> Yes. Do you think it would be better to put it in the devicetree? I
> suppose we could add some more stuff to the compatible string,
> although U-Boot does not support the PCI compatible strings at
> present.
Putting it in the device tree also looks odd, because it only matters
on a dedicated board.
> What do you suggest?
Or parse the ACPI table coreboot has set up? But that might be another
huge monster :(
Regards,
Bin
Re: [PATCH 03/17] x86: Allow coreboot serial driver to guess the UART
Hi Bin,
On Sun, 25 Apr 2021 at 18:21, Bin Meng wrote:
>
> Hi Simon,
>
> On Sun, Apr 25, 2021 at 10:10 AM Simon Glass wrote:
> >
> > Hi Bin,
> >
> > On Sun, 25 Apr 2021 at 13:49, Bin Meng wrote:
> > >
> > > Hi Simon,
> > >
> > > On Sat, Apr 24, 2021 at 12:56 PM Simon Glass wrote:
> > > >
> > > > Hi Bin,
> > > >
> > > > On Thu, 8 Apr 2021 at 14:23, Bin Meng wrote:
> > > > >
> > > > > Hi Simon,
> > > > >
> > > > > On Wed, Apr 7, 2021 at 12:32 PM Simon Glass wrote:
> > > > > >
> > > > > > At present this driver relies on coreboot to provide information
> > > > > > about
> > > > > > the console UART. However if coreboot is not compiled with the UART
> > > > > > enabled, the information is left out. This configuration is quite
> > > > > > common, e.g. with shipping x86-based Chrome OS Chromebooks.
> > > > > >
> > > > > > Add a way to determine the UART settings in this case, using a
> > > > > > hard-coded list of PCI IDs.
> > > > > >
> > > > > > Signed-off-by: Simon Glass
> > > > > > ---
> > > > > >
> > > > > > drivers/serial/serial_coreboot.c | 68
> > > > > >
> > > > > > include/pci_ids.h| 1 +
> > > > > > 2 files changed, 61 insertions(+), 8 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/serial/serial_coreboot.c
> > > > > > b/drivers/serial/serial_coreboot.c
> > > > > > index de09c8681f5..4b4619432d8 100644
> > > > > > --- a/drivers/serial/serial_coreboot.c
> > > > > > +++ b/drivers/serial/serial_coreboot.c
> > > > > > @@ -11,19 +11,71 @@
> > > > > > #include
> > > > > > #include
> > > > > >
> > > > > > +static const struct pci_device_id ids[] = {
> > > > > > + { PCI_DEVICE(PCI_VENDOR_ID_INTEL,
> > > > > > PCI_DEVICE_ID_INTEL_APL_UART2) },
> > > > > > + {},
> > > > > > +};
> > > > > > +
> > > > > > +/*
> > > > > > + * Coreboot only sets up the UART if it uses it and doesn't bother
> > > > > > to put the
> > > > > > + * details in sysinfo if it doesn't. Try to guess in that case,
> > > > > > using devices
> > > > > > + * we know about
> > > > > > + *
> > > > > > + * @plat: Platform data to fill in
> > > > > > + * @return 0 if found, -ve if no UART was found
> > > > > > + */
> > > > > > +static int guess_uart(struct ns16550_plat *plat)
> > > > >
> > > > > This is really not a guess, but use a pre-configured platform data.
> > > > > Also this only work for Apollo Lake board, and will break other boards
> > > > > if they don't have cbinfo available.
> > > >
> > > > Which bit of it breaks other boards?
> > >
> > > I see it does not return the error code to the caller, that's okay ...
> > >
> > > >
> > > > >
> > > > > Why not just simply put a serial node in the device tree and we are
> > > > > all done?
> > > >
> > > > See my other email...I am trying to make this boot on any board that
> > > > coreboot supports.
> > >
> > > But this solution does not scale. One has to put all known UARTs into
> > > serial_coreboot.c.
> >
> > Yes that's right...but this is only for when coreboot does not enable
> > serial. Also the number of new platforms is not that great.
> >
> > >
> > > Why not patch coreboot instead? Why coreboot does not provide a cbinfo
> > > with serial?
> >
> > Because it does not even set up the serial device in that case, so
> > doesn't know the details. The driver is completely missing.
>
> Sigh. Is it possible to upstream a patch to coreboot to enable that?
Well I'm not even sure upstream coreboot boots on the various
Chromebooks I am targetting. If it does, then serial is probablt
enabled. But certainly for chromebooks, it is not. My goal is to have
U-Boot boot on a chromebook in altfw mode with serial enabled.
>
> I don't like the current approach because it ends up duplicating all
> UART IDs/info in C.
Yes. Do you think it would be better to put it in the devicetree? I
suppose we could add some more stuff to the compatible string,
although U-Boot does not support the PCI compatible strings at
present.
What do you suggest?
Regards,
Simon
Re: [PATCH 03/17] x86: Allow coreboot serial driver to guess the UART
Hi Simon,
On Sun, Apr 25, 2021 at 10:10 AM Simon Glass wrote:
>
> Hi Bin,
>
> On Sun, 25 Apr 2021 at 13:49, Bin Meng wrote:
> >
> > Hi Simon,
> >
> > On Sat, Apr 24, 2021 at 12:56 PM Simon Glass wrote:
> > >
> > > Hi Bin,
> > >
> > > On Thu, 8 Apr 2021 at 14:23, Bin Meng wrote:
> > > >
> > > > Hi Simon,
> > > >
> > > > On Wed, Apr 7, 2021 at 12:32 PM Simon Glass wrote:
> > > > >
> > > > > At present this driver relies on coreboot to provide information about
> > > > > the console UART. However if coreboot is not compiled with the UART
> > > > > enabled, the information is left out. This configuration is quite
> > > > > common, e.g. with shipping x86-based Chrome OS Chromebooks.
> > > > >
> > > > > Add a way to determine the UART settings in this case, using a
> > > > > hard-coded list of PCI IDs.
> > > > >
> > > > > Signed-off-by: Simon Glass
> > > > > ---
> > > > >
> > > > > drivers/serial/serial_coreboot.c | 68
> > > > >
> > > > > include/pci_ids.h| 1 +
> > > > > 2 files changed, 61 insertions(+), 8 deletions(-)
> > > > >
> > > > > diff --git a/drivers/serial/serial_coreboot.c
> > > > > b/drivers/serial/serial_coreboot.c
> > > > > index de09c8681f5..4b4619432d8 100644
> > > > > --- a/drivers/serial/serial_coreboot.c
> > > > > +++ b/drivers/serial/serial_coreboot.c
> > > > > @@ -11,19 +11,71 @@
> > > > > #include
> > > > > #include
> > > > >
> > > > > +static const struct pci_device_id ids[] = {
> > > > > + { PCI_DEVICE(PCI_VENDOR_ID_INTEL,
> > > > > PCI_DEVICE_ID_INTEL_APL_UART2) },
> > > > > + {},
> > > > > +};
> > > > > +
> > > > > +/*
> > > > > + * Coreboot only sets up the UART if it uses it and doesn't bother
> > > > > to put the
> > > > > + * details in sysinfo if it doesn't. Try to guess in that case,
> > > > > using devices
> > > > > + * we know about
> > > > > + *
> > > > > + * @plat: Platform data to fill in
> > > > > + * @return 0 if found, -ve if no UART was found
> > > > > + */
> > > > > +static int guess_uart(struct ns16550_plat *plat)
> > > >
> > > > This is really not a guess, but use a pre-configured platform data.
> > > > Also this only work for Apollo Lake board, and will break other boards
> > > > if they don't have cbinfo available.
> > >
> > > Which bit of it breaks other boards?
> >
> > I see it does not return the error code to the caller, that's okay ...
> >
> > >
> > > >
> > > > Why not just simply put a serial node in the device tree and we are all
> > > > done?
> > >
> > > See my other email...I am trying to make this boot on any board that
> > > coreboot supports.
> >
> > But this solution does not scale. One has to put all known UARTs into
> > serial_coreboot.c.
>
> Yes that's right...but this is only for when coreboot does not enable
> serial. Also the number of new platforms is not that great.
>
> >
> > Why not patch coreboot instead? Why coreboot does not provide a cbinfo
> > with serial?
>
> Because it does not even set up the serial device in that case, so
> doesn't know the details. The driver is completely missing.
Sigh. Is it possible to upstream a patch to coreboot to enable that?
I don't like the current approach because it ends up duplicating all
UART IDs/info in C.
Regards,
Bin
Re: [PATCH 03/17] x86: Allow coreboot serial driver to guess the UART
Hi Bin,
On Sun, 25 Apr 2021 at 13:49, Bin Meng wrote:
>
> Hi Simon,
>
> On Sat, Apr 24, 2021 at 12:56 PM Simon Glass wrote:
> >
> > Hi Bin,
> >
> > On Thu, 8 Apr 2021 at 14:23, Bin Meng wrote:
> > >
> > > Hi Simon,
> > >
> > > On Wed, Apr 7, 2021 at 12:32 PM Simon Glass wrote:
> > > >
> > > > At present this driver relies on coreboot to provide information about
> > > > the console UART. However if coreboot is not compiled with the UART
> > > > enabled, the information is left out. This configuration is quite
> > > > common, e.g. with shipping x86-based Chrome OS Chromebooks.
> > > >
> > > > Add a way to determine the UART settings in this case, using a
> > > > hard-coded list of PCI IDs.
> > > >
> > > > Signed-off-by: Simon Glass
> > > > ---
> > > >
> > > > drivers/serial/serial_coreboot.c | 68
> > > > include/pci_ids.h| 1 +
> > > > 2 files changed, 61 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/drivers/serial/serial_coreboot.c
> > > > b/drivers/serial/serial_coreboot.c
> > > > index de09c8681f5..4b4619432d8 100644
> > > > --- a/drivers/serial/serial_coreboot.c
> > > > +++ b/drivers/serial/serial_coreboot.c
> > > > @@ -11,19 +11,71 @@
> > > > #include
> > > > #include
> > > >
> > > > +static const struct pci_device_id ids[] = {
> > > > + { PCI_DEVICE(PCI_VENDOR_ID_INTEL,
> > > > PCI_DEVICE_ID_INTEL_APL_UART2) },
> > > > + {},
> > > > +};
> > > > +
> > > > +/*
> > > > + * Coreboot only sets up the UART if it uses it and doesn't bother to
> > > > put the
> > > > + * details in sysinfo if it doesn't. Try to guess in that case, using
> > > > devices
> > > > + * we know about
> > > > + *
> > > > + * @plat: Platform data to fill in
> > > > + * @return 0 if found, -ve if no UART was found
> > > > + */
> > > > +static int guess_uart(struct ns16550_plat *plat)
> > >
> > > This is really not a guess, but use a pre-configured platform data.
> > > Also this only work for Apollo Lake board, and will break other boards
> > > if they don't have cbinfo available.
> >
> > Which bit of it breaks other boards?
>
> I see it does not return the error code to the caller, that's okay ...
>
> >
> > >
> > > Why not just simply put a serial node in the device tree and we are all
> > > done?
> >
> > See my other email...I am trying to make this boot on any board that
> > coreboot supports.
>
> But this solution does not scale. One has to put all known UARTs into
> serial_coreboot.c.
Yes that's right...but this is only for when coreboot does not enable
serial. Also the number of new platforms is not that great.
>
> Why not patch coreboot instead? Why coreboot does not provide a cbinfo
> with serial?
Because it does not even set up the serial device in that case, so
doesn't know the details. The driver is completely missing.
Regards,
Simon
Re: [PATCH 03/17] x86: Allow coreboot serial driver to guess the UART
Hi Simon,
On Sat, Apr 24, 2021 at 12:56 PM Simon Glass wrote:
>
> Hi Bin,
>
> On Thu, 8 Apr 2021 at 14:23, Bin Meng wrote:
> >
> > Hi Simon,
> >
> > On Wed, Apr 7, 2021 at 12:32 PM Simon Glass wrote:
> > >
> > > At present this driver relies on coreboot to provide information about
> > > the console UART. However if coreboot is not compiled with the UART
> > > enabled, the information is left out. This configuration is quite
> > > common, e.g. with shipping x86-based Chrome OS Chromebooks.
> > >
> > > Add a way to determine the UART settings in this case, using a
> > > hard-coded list of PCI IDs.
> > >
> > > Signed-off-by: Simon Glass
> > > ---
> > >
> > > drivers/serial/serial_coreboot.c | 68
> > > include/pci_ids.h| 1 +
> > > 2 files changed, 61 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/serial/serial_coreboot.c
> > > b/drivers/serial/serial_coreboot.c
> > > index de09c8681f5..4b4619432d8 100644
> > > --- a/drivers/serial/serial_coreboot.c
> > > +++ b/drivers/serial/serial_coreboot.c
> > > @@ -11,19 +11,71 @@
> > > #include
> > > #include
> > >
> > > +static const struct pci_device_id ids[] = {
> > > + { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_APL_UART2)
> > > },
> > > + {},
> > > +};
> > > +
> > > +/*
> > > + * Coreboot only sets up the UART if it uses it and doesn't bother to
> > > put the
> > > + * details in sysinfo if it doesn't. Try to guess in that case, using
> > > devices
> > > + * we know about
> > > + *
> > > + * @plat: Platform data to fill in
> > > + * @return 0 if found, -ve if no UART was found
> > > + */
> > > +static int guess_uart(struct ns16550_plat *plat)
> >
> > This is really not a guess, but use a pre-configured platform data.
> > Also this only work for Apollo Lake board, and will break other boards
> > if they don't have cbinfo available.
>
> Which bit of it breaks other boards?
I see it does not return the error code to the caller, that's okay ...
>
> >
> > Why not just simply put a serial node in the device tree and we are all
> > done?
>
> See my other email...I am trying to make this boot on any board that
> coreboot supports.
But this solution does not scale. One has to put all known UARTs into
serial_coreboot.c.
Why not patch coreboot instead? Why coreboot does not provide a cbinfo
with serial?
Regards,
Bin
Re: [PATCH 03/17] x86: Allow coreboot serial driver to guess the UART
Hi Bin,
On Thu, 8 Apr 2021 at 14:23, Bin Meng wrote:
>
> Hi Simon,
>
> On Wed, Apr 7, 2021 at 12:32 PM Simon Glass wrote:
> >
> > At present this driver relies on coreboot to provide information about
> > the console UART. However if coreboot is not compiled with the UART
> > enabled, the information is left out. This configuration is quite
> > common, e.g. with shipping x86-based Chrome OS Chromebooks.
> >
> > Add a way to determine the UART settings in this case, using a
> > hard-coded list of PCI IDs.
> >
> > Signed-off-by: Simon Glass
> > ---
> >
> > drivers/serial/serial_coreboot.c | 68
> > include/pci_ids.h| 1 +
> > 2 files changed, 61 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/serial/serial_coreboot.c
> > b/drivers/serial/serial_coreboot.c
> > index de09c8681f5..4b4619432d8 100644
> > --- a/drivers/serial/serial_coreboot.c
> > +++ b/drivers/serial/serial_coreboot.c
> > @@ -11,19 +11,71 @@
> > #include
> > #include
> >
> > +static const struct pci_device_id ids[] = {
> > + { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_APL_UART2) },
> > + {},
> > +};
> > +
> > +/*
> > + * Coreboot only sets up the UART if it uses it and doesn't bother to put
> > the
> > + * details in sysinfo if it doesn't. Try to guess in that case, using
> > devices
> > + * we know about
> > + *
> > + * @plat: Platform data to fill in
> > + * @return 0 if found, -ve if no UART was found
> > + */
> > +static int guess_uart(struct ns16550_plat *plat)
>
> This is really not a guess, but use a pre-configured platform data.
> Also this only work for Apollo Lake board, and will break other boards
> if they don't have cbinfo available.
Which bit of it breaks other boards?
>
> Why not just simply put a serial node in the device tree and we are all done?
See my other email...I am trying to make this boot on any board that
coreboot supports.
[..]
Regards,
Simon
Re: [PATCH 03/17] x86: Allow coreboot serial driver to guess the UART
Hi Simon,
On Wed, Apr 7, 2021 at 12:32 PM Simon Glass wrote:
>
> At present this driver relies on coreboot to provide information about
> the console UART. However if coreboot is not compiled with the UART
> enabled, the information is left out. This configuration is quite
> common, e.g. with shipping x86-based Chrome OS Chromebooks.
>
> Add a way to determine the UART settings in this case, using a
> hard-coded list of PCI IDs.
>
> Signed-off-by: Simon Glass
> ---
>
> drivers/serial/serial_coreboot.c | 68
> include/pci_ids.h| 1 +
> 2 files changed, 61 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/serial/serial_coreboot.c
> b/drivers/serial/serial_coreboot.c
> index de09c8681f5..4b4619432d8 100644
> --- a/drivers/serial/serial_coreboot.c
> +++ b/drivers/serial/serial_coreboot.c
> @@ -11,19 +11,71 @@
> #include
> #include
>
> +static const struct pci_device_id ids[] = {
> + { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_APL_UART2) },
> + {},
> +};
> +
> +/*
> + * Coreboot only sets up the UART if it uses it and doesn't bother to put the
> + * details in sysinfo if it doesn't. Try to guess in that case, using devices
> + * we know about
> + *
> + * @plat: Platform data to fill in
> + * @return 0 if found, -ve if no UART was found
> + */
> +static int guess_uart(struct ns16550_plat *plat)
This is really not a guess, but use a pre-configured platform data.
Also this only work for Apollo Lake board, and will break other boards
if they don't have cbinfo available.
Why not just simply put a serial node in the device tree and we are all done?
> +{
> + struct udevice *bus, *dev;
> + ulong addr;
> + int index;
> + int ret;
> +
> + ret = uclass_first_device_err(UCLASS_PCI, );
> + if (ret)
> + return ret;
> + index = 0;
> + ret = pci_bus_find_devices(bus, ids, , );
> + if (ret)
> + return ret;
> + addr = dm_pci_read_bar32(dev, 0);
> + plat->base = addr;
> + plat->reg_shift = 2;
> + plat->reg_width = 4;
> + plat->clock = 1843200;
> + plat->fcr = UART_FCR_DEFVAL;
> + plat->flags = 0;
> +
> + return 0;
> +}
> +
> static int coreboot_of_to_plat(struct udevice *dev)
> {
> struct ns16550_plat *plat = dev_get_plat(dev);
> struct cb_serial *cb_info = lib_sysinfo.serial;
>
> - plat->base = cb_info->baseaddr;
> - plat->reg_shift = cb_info->regwidth == 4 ? 2 : 0;
> - plat->reg_width = cb_info->regwidth;
> - plat->clock = cb_info->input_hertz;
> - plat->fcr = UART_FCR_DEFVAL;
> - plat->flags = 0;
> - if (cb_info->type == CB_SERIAL_TYPE_IO_MAPPED)
> - plat->flags |= NS16550_FLAG_IO;
> + if (cb_info) {
> + plat->base = cb_info->baseaddr;
> + plat->reg_shift = cb_info->regwidth == 4 ? 2 : 0;
> + plat->reg_width = cb_info->regwidth;
> + plat->clock = cb_info->input_hertz;
> + plat->fcr = UART_FCR_DEFVAL;
> + plat->flags = 0;
> + if (cb_info->type == CB_SERIAL_TYPE_IO_MAPPED)
> + plat->flags |= NS16550_FLAG_IO;
> + } else if (CONFIG_IS_ENABLED(PCI)) {
> + int ret;
> +
> + ret = guess_uart(plat);
> + if (ret) {
> + /*
> +* Returning an error will cause U-Boot to complain
> that
> +* there is no UART, which may panic. So stay silent
> and
> +* pray that the video console will work.
> +*/
> + log_debug("Cannot detect UART\n");
> + }
> + }
>
> return 0;
> }
> diff --git a/include/pci_ids.h b/include/pci_ids.h
> index 7ecedc7f04c..d91c1d08f1a 100644
> --- a/include/pci_ids.h
> +++ b/include/pci_ids.h
> @@ -2987,6 +2987,7 @@
> #define PCI_DEVICE_ID_INTEL_UNC_R3QPI1 0x3c45
> #define PCI_DEVICE_ID_INTEL_JAKETOWN_UBOX 0x3ce0
> #define PCI_DEVICE_ID_INTEL_IOAT_SNB 0x402f
> +#define PCI_DEVICE_ID_INTEL_APL_UART2 0x5ac0
> #define PCI_DEVICE_ID_INTEL_5100_160x65f0
> #define PCI_DEVICE_ID_INTEL_5100_190x65f3
> #define PCI_DEVICE_ID_INTEL_5100_210x65f5
> --
Regards,
Bin
