[U-Boot] [RFC] Merge all ns16550 dm serial drivers into one
Hi,
Currently there are 5 dm serial drivers, all of which are ns16550 compatible drivers. They are:
serial_omap.c serial_dw.c serial_tegra.c serial_x86.c serial_ppc.c
All these drivers are pretty much similar. I think we can justmerge these into one ns16550 driver.
If you think this is necessary, I will send a patch series to do this.
Regards, Bin
Hi Bin,
On 14 August 2015 at 03:18, Bin Meng bmeng.cn@gmail.com wrote:
Hi,
Currently there are 5 dm serial drivers, all of which are ns16550 compatible drivers. They are:
serial_omap.c serial_dw.c serial_tegra.c serial_x86.c serial_ppc.c
All these drivers are pretty much similar. I think we can justmerge these into one ns16550 driver.
If you think this is necessary, I will send a patch series to do this.
The tegra one is there because it needs an input clock and Stephen didn't want to add this to the device tree binding (the kernel has a clock framework which gets around this problem).
After that I followed the same pattern. I would support updating the binding to support an input clock. Even with the new clock framework in U-Boot it might be painful to fit it into SPL in some cases.
Regards, Simon
On 08/14/2015 10:50 AM, Simon Glass wrote:
Hi Bin,
On 14 August 2015 at 03:18, Bin Meng bmeng.cn@gmail.com wrote:
Hi,
Currently there are 5 dm serial drivers, all of which are ns16550 compatible drivers. They are:
serial_omap.c serial_dw.c serial_tegra.c serial_x86.c serial_ppc.c
All these drivers are pretty much similar. I think we can justmerge these into one ns16550 driver.
If you think this is necessary, I will send a patch series to do this.
The tegra one is there because it needs an input clock and Stephen didn't want to add this to the device tree binding (the kernel has a clock framework which gets around this problem).
After that I followed the same pattern. I would support updating the binding to support an input clock. Even with the new clock framework in U-Boot it might be painful to fit it into SPL in some cases.
The clock is already in the DT, in both Linux and U-Boot's copy, at least for Tegra DTs:
uarta: serial@0,70006000 { compatible = "nvidia,tegra124-uart", ... ... clocks = <&tegra_car TEGRA124_CLK_UARTA>;
Hi Stephen,
On 14 August 2015 at 10:58, Stephen Warren swarren@wwwdotorg.org wrote:
On 08/14/2015 10:50 AM, Simon Glass wrote:
Hi Bin,
On 14 August 2015 at 03:18, Bin Meng bmeng.cn@gmail.com wrote:
Hi,
Currently there are 5 dm serial drivers, all of which are ns16550 compatible drivers. They are:
serial_omap.c serial_dw.c serial_tegra.c serial_x86.c serial_ppc.c
All these drivers are pretty much similar. I think we can justmerge these into one ns16550 driver.
If you think this is necessary, I will send a patch series to do this.
The tegra one is there because it needs an input clock and Stephen didn't want to add this to the device tree binding (the kernel has a clock framework which gets around this problem).
After that I followed the same pattern. I would support updating the binding to support an input clock. Even with the new clock framework in U-Boot it might be painful to fit it into SPL in some cases.
The clock is already in the DT, in both Linux and U-Boot's copy, at least for Tegra DTs:
uarta: serial@0,70006000 { compatible = "nvidia,tegra124-uart", ...... clocks = <&tegra_car TEGRA124_CLK_UARTA>;
I mean the clock-frequency property. However if there is a plan to implement the clock framework in U-Boot that would be good too.
Regards, Simon
On Sat, Aug 15, 2015 at 12:59 AM, Simon Glass sjg@chromium.org wrote:
Hi Stephen,
On 14 August 2015 at 10:58, Stephen Warren swarren@wwwdotorg.org wrote:
On 08/14/2015 10:50 AM, Simon Glass wrote:
Hi Bin,
On 14 August 2015 at 03:18, Bin Meng bmeng.cn@gmail.com wrote:
Hi,
Currently there are 5 dm serial drivers, all of which are ns16550 compatible drivers. They are:
serial_omap.c serial_dw.c serial_tegra.c serial_x86.c serial_ppc.c
All these drivers are pretty much similar. I think we can justmerge these into one ns16550 driver.
If you think this is necessary, I will send a patch series to do this.
The tegra one is there because it needs an input clock and Stephen didn't want to add this to the device tree binding (the kernel has a clock framework which gets around this problem).
After that I followed the same pattern. I would support updating the binding to support an input clock. Even with the new clock framework in U-Boot it might be painful to fit it into SPL in some cases.
The clock is already in the DT, in both Linux and U-Boot's copy, at least for Tegra DTs:
uarta: serial@0,70006000 { compatible = "nvidia,tegra124-uart", ...... clocks = <&tegra_car TEGRA124_CLK_UARTA>;
I mean the clock-frequency property. However if there is a plan to implement the clock framework in U-Boot that would be good too.
The clock-frequency is a fixed value on x86 super i/o chipset, and fixed on the PCI bus too. But for ARM and PPC, it might get dynamically calculated due to different PLL settings. We can implement a _weak function like the one in serial_ppc.c get_serial_clock() to initialize plat->clock with its return value. The _weak function gets clock-frequency from device tree. If there is not, platform codes which uses the ns16550 driver should provide the implementation of get_serial_clock(). Thoughts?
Regards, Bin
On 08/14/2015 04:40 PM, Bin Meng wrote:
On Sat, Aug 15, 2015 at 12:59 AM, Simon Glass sjg@chromium.org wrote:
Hi Stephen,
On 14 August 2015 at 10:58, Stephen Warren swarren@wwwdotorg.org wrote:
On 08/14/2015 10:50 AM, Simon Glass wrote:
Hi Bin,
On 14 August 2015 at 03:18, Bin Meng bmeng.cn@gmail.com wrote:
Hi,
Currently there are 5 dm serial drivers, all of which are ns16550 compatible drivers. They are:
serial_omap.c serial_dw.c serial_tegra.c serial_x86.c serial_ppc.cAll these drivers are pretty much similar. I think we can justmerge these into one ns16550 driver.
If you think this is necessary, I will send a patch series to do this.
The tegra one is there because it needs an input clock and Stephen didn't want to add this to the device tree binding (the kernel has a clock framework which gets around this problem).
After that I followed the same pattern. I would support updating the binding to support an input clock. Even with the new clock framework in U-Boot it might be painful to fit it into SPL in some cases.
The clock is already in the DT, in both Linux and U-Boot's copy, at least for Tegra DTs:
uarta: serial@0,70006000 { compatible = "nvidia,tegra124-uart", ...... clocks = <&tegra_car TEGRA124_CLK_UARTA>;
I mean the clock-frequency property. However if there is a plan to implement the clock framework in U-Boot that would be good too.
The clock-frequency is a fixed value on x86 super i/o chipset, and fixed on the PCI bus too. But for ARM and PPC, it might get dynamically calculated due to different PLL settings. We can implement a _weak function like the one in serial_ppc.c get_serial_clock() to initialize plat->clock with its return value. The _weak function gets clock-frequency from device tree. If there is not, platform codes which uses the ns16550 driver should provide the implementation of get_serial_clock(). Thoughts?
There is no clock-frequency property in DT, at least for the Tegra DT binding. It looks like some other bindings have it. To obtain the clock frequency from DT for Tegra, you'd need to parse the clocks property, find the clock driver associated with the phandle in DT, and go and ask that clock driver what the clock frequency is.
I'd prefer not to have a weak function that parses clock-frequency, since it's too easy to accidentally use it on systems where parsing that property is incorrect.
Certainly, a generic UART driver can call out to a platform-supplied function to retrieve the clock, and we can provide driver-specific implementations for x86 super IO and PCI, and generic implementations that appropriate drivers can call to parse the clocks or clock-frequency property from DT, and finally for Tegra if we can't parse the clocks property right now, call the Tegra clock driver directly to look up the value.
Hi,
On 14 August 2015 at 16:51, Stephen Warren swarren@wwwdotorg.org wrote:
On 08/14/2015 04:40 PM, Bin Meng wrote:
On Sat, Aug 15, 2015 at 12:59 AM, Simon Glass sjg@chromium.org wrote:
Hi Stephen,
On 14 August 2015 at 10:58, Stephen Warren swarren@wwwdotorg.org wrote:
On 08/14/2015 10:50 AM, Simon Glass wrote:
Hi Bin,
On 14 August 2015 at 03:18, Bin Meng bmeng.cn@gmail.com wrote:
Hi,
Currently there are 5 dm serial drivers, all of which are ns16550 compatible drivers. They are:
serial_omap.c serial_dw.c serial_tegra.c serial_x86.c serial_ppc.cAll these drivers are pretty much similar. I think we can justmerge these into one ns16550 driver.
If you think this is necessary, I will send a patch series to do this.
The tegra one is there because it needs an input clock and Stephen didn't want to add this to the device tree binding (the kernel has a clock framework which gets around this problem).
After that I followed the same pattern. I would support updating the binding to support an input clock. Even with the new clock framework in U-Boot it might be painful to fit it into SPL in some cases.
The clock is already in the DT, in both Linux and U-Boot's copy, at least for Tegra DTs:
uarta: serial@0,70006000 { compatible = "nvidia,tegra124-uart", ...... clocks = <&tegra_car TEGRA124_CLK_UARTA>;
I mean the clock-frequency property. However if there is a plan to implement the clock framework in U-Boot that would be good too.
The clock-frequency is a fixed value on x86 super i/o chipset, and fixed on the PCI bus too. But for ARM and PPC, it might get dynamically calculated due to different PLL settings. We can implement a _weak function like the one in serial_ppc.c get_serial_clock() to initialize plat->clock with its return value. The _weak function gets clock-frequency from device tree. If there is not, platform codes which uses the ns16550 driver should provide the implementation of get_serial_clock(). Thoughts?
There is no clock-frequency property in DT, at least for the Tegra DT binding. It looks like some other bindings have it. To obtain the clock frequency from DT for Tegra, you'd need to parse the clocks property, find the clock driver associated with the phandle in DT, and go and ask that clock driver what the clock frequency is.
I'd prefer not to have a weak function that parses clock-frequency, since it's too easy to accidentally use it on systems where parsing that property is incorrect.
Certainly, a generic UART driver can call out to a platform-supplied function to retrieve the clock, and we can provide driver-specific implementations for x86 super IO and PCI, and generic implementations that appropriate drivers can call to parse the clocks or clock-frequency property from DT, and finally for Tegra if we can't parse the clocks property right now, call the Tegra clock driver directly to look up the value.
I'm not a big fan of weak functions either. In fact I think with driver model we should avoid them. If we can't call a uclass to get the info then perhaps we should wait until we can.
Pragmatically I wonder if a UART clock frequency would not be a useful compromise? Some bindings have it, some do not. Maybe we should just add it?
Regards, Simon
Hi,
On Sat, Aug 15, 2015 at 7:18 AM, Simon Glass sjg@chromium.org wrote:
Hi,
On 14 August 2015 at 16:51, Stephen Warren swarren@wwwdotorg.org wrote:
On 08/14/2015 04:40 PM, Bin Meng wrote:
On Sat, Aug 15, 2015 at 12:59 AM, Simon Glass sjg@chromium.org wrote:
Hi Stephen,
On 14 August 2015 at 10:58, Stephen Warren swarren@wwwdotorg.org wrote:
On 08/14/2015 10:50 AM, Simon Glass wrote:
Hi Bin,
On 14 August 2015 at 03:18, Bin Meng bmeng.cn@gmail.com wrote: > > > Hi, > > Currently there are 5 dm serial drivers, all of which are ns16550 > compatible drivers. They are: > > serial_omap.c > serial_dw.c > serial_tegra.c > serial_x86.c > serial_ppc.c > > All these drivers are pretty much similar. I think we can justmerge > these into one ns16550 driver. > > If you think this is necessary, I will send a patch series to do this.
The tegra one is there because it needs an input clock and Stephen didn't want to add this to the device tree binding (the kernel has a clock framework which gets around this problem).
After that I followed the same pattern. I would support updating the binding to support an input clock. Even with the new clock framework in U-Boot it might be painful to fit it into SPL in some cases.
The clock is already in the DT, in both Linux and U-Boot's copy, at least for Tegra DTs:
uarta: serial@0,70006000 { compatible = "nvidia,tegra124-uart", ...... clocks = <&tegra_car TEGRA124_CLK_UARTA>;
I mean the clock-frequency property. However if there is a plan to implement the clock framework in U-Boot that would be good too.
The clock-frequency is a fixed value on x86 super i/o chipset, and fixed on the PCI bus too. But for ARM and PPC, it might get dynamically calculated due to different PLL settings. We can implement a _weak function like the one in serial_ppc.c get_serial_clock() to initialize plat->clock with its return value. The _weak function gets clock-frequency from device tree. If there is not, platform codes which uses the ns16550 driver should provide the implementation of get_serial_clock(). Thoughts?
There is no clock-frequency property in DT, at least for the Tegra DT binding. It looks like some other bindings have it. To obtain the clock frequency from DT for Tegra, you'd need to parse the clocks property, find the clock driver associated with the phandle in DT, and go and ask that clock driver what the clock frequency is.
I'd prefer not to have a weak function that parses clock-frequency, since it's too easy to accidentally use it on systems where parsing that property is incorrect.
Certainly, a generic UART driver can call out to a platform-supplied function to retrieve the clock, and we can provide driver-specific implementations for x86 super IO and PCI, and generic implementations that appropriate drivers can call to parse the clocks or clock-frequency property from DT, and finally for Tegra if we can't parse the clocks property right now, call the Tegra clock driver directly to look up the value.
I'm not a big fan of weak functions either. In fact I think with driver model we should avoid them. If we can't call a uclass to get the info then perhaps we should wait until we can.
Pragmatically I wonder if a UART clock frequency would not be a useful compromise? Some bindings have it, some do not. Maybe we should just add it?
I am not sure which bindings you are looking at, but I checked the Power.org ePAPR spec v1.1, the clock-frequency is there specially listed for NS16550 compatible nodes.
clock-frequency R <u32> Specifies the frequency (in Hz) of the baud rate generator’s input clock
If you don't have such clock-frequency in your device tree, I would say you don't follow the spec.
Regards, Bin
On 08/14/2015 05:57 PM, Bin Meng wrote:
Hi,
On Sat, Aug 15, 2015 at 7:18 AM, Simon Glass sjg@chromium.org wrote:
Hi,
On 14 August 2015 at 16:51, Stephen Warren swarren@wwwdotorg.org wrote:
On 08/14/2015 04:40 PM, Bin Meng wrote:
On Sat, Aug 15, 2015 at 12:59 AM, Simon Glass sjg@chromium.org wrote:
Hi Stephen,
On 14 August 2015 at 10:58, Stephen Warren swarren@wwwdotorg.org wrote:
On 08/14/2015 10:50 AM, Simon Glass wrote: > > > Hi Bin, > > On 14 August 2015 at 03:18, Bin Meng bmeng.cn@gmail.com wrote: >> >> >> Hi, >> >> Currently there are 5 dm serial drivers, all of which are ns16550 >> compatible drivers. They are: >> >> serial_omap.c >> serial_dw.c >> serial_tegra.c >> serial_x86.c >> serial_ppc.c >> >> All these drivers are pretty much similar. I think we can justmerge >> these into one ns16550 driver. >> >> If you think this is necessary, I will send a patch series to do this. > > > > The tegra one is there because it needs an input clock and Stephen > didn't want to add this to the device tree binding (the kernel has a > clock framework which gets around this problem). > > After that I followed the same pattern. I would support updating the > binding to support an input clock. Even with the new clock framework > in U-Boot it might be painful to fit it into SPL in some cases.
The clock is already in the DT, in both Linux and U-Boot's copy, at least for Tegra DTs:
uarta: serial@0,70006000 { compatible = "nvidia,tegra124-uart", ...... clocks = <&tegra_car TEGRA124_CLK_UARTA>;
I mean the clock-frequency property. However if there is a plan to implement the clock framework in U-Boot that would be good too.
The clock-frequency is a fixed value on x86 super i/o chipset, and fixed on the PCI bus too. But for ARM and PPC, it might get dynamically calculated due to different PLL settings. We can implement a _weak function like the one in serial_ppc.c get_serial_clock() to initialize plat->clock with its return value. The _weak function gets clock-frequency from device tree. If there is not, platform codes which uses the ns16550 driver should provide the implementation of get_serial_clock(). Thoughts?
There is no clock-frequency property in DT, at least for the Tegra DT binding. It looks like some other bindings have it. To obtain the clock frequency from DT for Tegra, you'd need to parse the clocks property, find the clock driver associated with the phandle in DT, and go and ask that clock driver what the clock frequency is.
I'd prefer not to have a weak function that parses clock-frequency, since it's too easy to accidentally use it on systems where parsing that property is incorrect.
Certainly, a generic UART driver can call out to a platform-supplied function to retrieve the clock, and we can provide driver-specific implementations for x86 super IO and PCI, and generic implementations that appropriate drivers can call to parse the clocks or clock-frequency property from DT, and finally for Tegra if we can't parse the clocks property right now, call the Tegra clock driver directly to look up the value.
I'm not a big fan of weak functions either. In fact I think with driver model we should avoid them. If we can't call a uclass to get the info then perhaps we should wait until we can.
Pragmatically I wonder if a UART clock frequency would not be a useful compromise? Some bindings have it, some do not. Maybe we should just add it?
I am not sure which bindings you are looking at,
The binding for nvidia,tegra20-uart. In the kernel tree, that's at Documentation/devicetree/bindings/serial/nvidia,tegra20-hsuart.txt.
but I checked the Power.org ePAPR spec v1.1, the clock-frequency is there specially listed for NS16550 compatible nodes.
clock-frequency R <u32> Specifies the frequency (in Hz) of the baud rate generator’s input clock
If you don't have such clock-frequency in your device tree, I would say you don't follow the spec.
The binding for Tegra UARTs doesn't inherit from the generic NS16550 binding, so there's that binding isn't relevant.
On Sat, Aug 15, 2015 at 10:56 AM, Stephen Warren swarren@wwwdotorg.org wrote:
On 08/14/2015 05:57 PM, Bin Meng wrote:
Hi,
On Sat, Aug 15, 2015 at 7:18 AM, Simon Glass sjg@chromium.org wrote:
Hi,
On 14 August 2015 at 16:51, Stephen Warren swarren@wwwdotorg.org wrote:
On 08/14/2015 04:40 PM, Bin Meng wrote:
On Sat, Aug 15, 2015 at 12:59 AM, Simon Glass sjg@chromium.org wrote:
Hi Stephen,
On 14 August 2015 at 10:58, Stephen Warren swarren@wwwdotorg.org wrote: > > On 08/14/2015 10:50 AM, Simon Glass wrote: >> >> >> Hi Bin, >> >> On 14 August 2015 at 03:18, Bin Meng bmeng.cn@gmail.com wrote: >>> >>> >>> Hi, >>> >>> Currently there are 5 dm serial drivers, all of which are ns16550 >>> compatible drivers. They are: >>> >>> serial_omap.c >>> serial_dw.c >>> serial_tegra.c >>> serial_x86.c >>> serial_ppc.c >>> >>> All these drivers are pretty much similar. I think we can justmerge >>> these into one ns16550 driver. >>> >>> If you think this is necessary, I will send a patch series to do this. >> >> >> >> The tegra one is there because it needs an input clock and Stephen >> didn't want to add this to the device tree binding (the kernel has a >> clock framework which gets around this problem). >> >> After that I followed the same pattern. I would support updating the >> binding to support an input clock. Even with the new clock framework >> in U-Boot it might be painful to fit it into SPL in some cases. > > > > The clock is already in the DT, in both Linux and U-Boot's copy, at > least > for Tegra DTs: > > uarta: serial@0,70006000 { > compatible = "nvidia,tegra124-uart", ... > ... > clocks = <&tegra_car TEGRA124_CLK_UARTA>; >
I mean the clock-frequency property. However if there is a plan to implement the clock framework in U-Boot that would be good too.
The clock-frequency is a fixed value on x86 super i/o chipset, and fixed on the PCI bus too. But for ARM and PPC, it might get dynamically calculated due to different PLL settings. We can implement a _weak function like the one in serial_ppc.c get_serial_clock() to initialize plat->clock with its return value. The _weak function gets clock-frequency from device tree. If there is not, platform codes which uses the ns16550 driver should provide the implementation of get_serial_clock(). Thoughts?
There is no clock-frequency property in DT, at least for the Tegra DT binding. It looks like some other bindings have it. To obtain the clock frequency from DT for Tegra, you'd need to parse the clocks property, find the clock driver associated with the phandle in DT, and go and ask that clock driver what the clock frequency is.
I'd prefer not to have a weak function that parses clock-frequency, since it's too easy to accidentally use it on systems where parsing that property is incorrect.
Certainly, a generic UART driver can call out to a platform-supplied function to retrieve the clock, and we can provide driver-specific implementations for x86 super IO and PCI, and generic implementations that appropriate drivers can call to parse the clocks or clock-frequency property from DT, and finally for Tegra if we can't parse the clocks property right now, call the Tegra clock driver directly to look up the value.
I'm not a big fan of weak functions either. In fact I think with driver model we should avoid them. If we can't call a uclass to get the info then perhaps we should wait until we can.
Pragmatically I wonder if a UART clock frequency would not be a useful compromise? Some bindings have it, some do not. Maybe we should just add it?
I am not sure which bindings you are looking at,
The binding for nvidia,tegra20-uart. In the kernel tree, that's at Documentation/devicetree/bindings/serial/nvidia,tegra20-hsuart.txt.
but I checked the Power.org ePAPR spec v1.1, the clock-frequency is there specially listed for NS16550 compatible nodes.
clock-frequency R <u32> Specifies the frequency (in Hz) of the baud rate generator’s input clock
If you don't have such clock-frequency in your device tree, I would say you don't follow the spec.
The binding for Tegra UARTs doesn't inherit from the generic NS16550 binding, so there's that binding isn't relevant.
The kernel bindings has more like dma channels for Tegra hsuart. U-Boot does not need this as we just need it as a serial console. We are not going to transfer some big chunk of data via serial line in U-Boot. In essence if it is NS16550 compatible device, it should just use that device binding. But I agree, clock-frequency can be optional for platforms where dynamic clock is used. In such case we cannot hardcode one frequency in the device tree.
Regards, Bin
On 08/14/2015 05:18 PM, Simon Glass wrote:
Hi,
On 14 August 2015 at 16:51, Stephen Warren swarren@wwwdotorg.org wrote:
On 08/14/2015 04:40 PM, Bin Meng wrote:
On Sat, Aug 15, 2015 at 12:59 AM, Simon Glass sjg@chromium.org wrote:
Hi Stephen,
On 14 August 2015 at 10:58, Stephen Warren swarren@wwwdotorg.org wrote:
On 08/14/2015 10:50 AM, Simon Glass wrote:
Hi Bin,
On 14 August 2015 at 03:18, Bin Meng bmeng.cn@gmail.com wrote: > > > Hi, > > Currently there are 5 dm serial drivers, all of which are ns16550 > compatible drivers. They are: > > serial_omap.c > serial_dw.c > serial_tegra.c > serial_x86.c > serial_ppc.c > > All these drivers are pretty much similar. I think we can justmerge > these into one ns16550 driver. > > If you think this is necessary, I will send a patch series to do this.
The tegra one is there because it needs an input clock and Stephen didn't want to add this to the device tree binding (the kernel has a clock framework which gets around this problem).
After that I followed the same pattern. I would support updating the binding to support an input clock. Even with the new clock framework in U-Boot it might be painful to fit it into SPL in some cases.
The clock is already in the DT, in both Linux and U-Boot's copy, at least for Tegra DTs:
uarta: serial@0,70006000 { compatible = "nvidia,tegra124-uart", ...... clocks = <&tegra_car TEGRA124_CLK_UARTA>;
I mean the clock-frequency property. However if there is a plan to implement the clock framework in U-Boot that would be good too.
The clock-frequency is a fixed value on x86 super i/o chipset, and fixed on the PCI bus too. But for ARM and PPC, it might get dynamically calculated due to different PLL settings. We can implement a _weak function like the one in serial_ppc.c get_serial_clock() to initialize plat->clock with its return value. The _weak function gets clock-frequency from device tree. If there is not, platform codes which uses the ns16550 driver should provide the implementation of get_serial_clock(). Thoughts?
There is no clock-frequency property in DT, at least for the Tegra DT binding. It looks like some other bindings have it. To obtain the clock frequency from DT for Tegra, you'd need to parse the clocks property, find the clock driver associated with the phandle in DT, and go and ask that clock driver what the clock frequency is.
I'd prefer not to have a weak function that parses clock-frequency, since it's too easy to accidentally use it on systems where parsing that property is incorrect.
Certainly, a generic UART driver can call out to a platform-supplied function to retrieve the clock, and we can provide driver-specific implementations for x86 super IO and PCI, and generic implementations that appropriate drivers can call to parse the clocks or clock-frequency property from DT, and finally for Tegra if we can't parse the clocks property right now, call the Tegra clock driver directly to look up the value.
I'm not a big fan of weak functions either. In fact I think with driver model we should avoid them. If we can't call a uclass to get the info then perhaps we should wait until we can.
Pragmatically I wonder if a UART clock frequency would not be a useful compromise? Some bindings have it, some do not. Maybe we should just add it?
There's no need for it; the binding already has a clocks property, from which the data can be derived. Adding a clock-frequency property would just result in two sources of the same data. In all likelihood, all that'd happen is that the two would get out-of-sync, and code wouldn't know which to trust.
On Sat, Aug 15, 2015 at 10:57 AM, Stephen Warren swarren@wwwdotorg.org wrote:
On 08/14/2015 05:18 PM, Simon Glass wrote:
Hi,
On 14 August 2015 at 16:51, Stephen Warren swarren@wwwdotorg.org wrote:
On 08/14/2015 04:40 PM, Bin Meng wrote:
On Sat, Aug 15, 2015 at 12:59 AM, Simon Glass sjg@chromium.org wrote:
Hi Stephen,
On 14 August 2015 at 10:58, Stephen Warren swarren@wwwdotorg.org wrote:
On 08/14/2015 10:50 AM, Simon Glass wrote: > > > Hi Bin, > > On 14 August 2015 at 03:18, Bin Meng bmeng.cn@gmail.com wrote: >> >> >> Hi, >> >> Currently there are 5 dm serial drivers, all of which are ns16550 >> compatible drivers. They are: >> >> serial_omap.c >> serial_dw.c >> serial_tegra.c >> serial_x86.c >> serial_ppc.c >> >> All these drivers are pretty much similar. I think we can justmerge >> these into one ns16550 driver. >> >> If you think this is necessary, I will send a patch series to do this. > > > > The tegra one is there because it needs an input clock and Stephen > didn't want to add this to the device tree binding (the kernel has a > clock framework which gets around this problem). > > After that I followed the same pattern. I would support updating the > binding to support an input clock. Even with the new clock framework > in U-Boot it might be painful to fit it into SPL in some cases.
The clock is already in the DT, in both Linux and U-Boot's copy, at least for Tegra DTs:
uarta: serial@0,70006000 { compatible = "nvidia,tegra124-uart", ...... clocks = <&tegra_car TEGRA124_CLK_UARTA>;
I mean the clock-frequency property. However if there is a plan to implement the clock framework in U-Boot that would be good too.
The clock-frequency is a fixed value on x86 super i/o chipset, and fixed on the PCI bus too. But for ARM and PPC, it might get dynamically calculated due to different PLL settings. We can implement a _weak function like the one in serial_ppc.c get_serial_clock() to initialize plat->clock with its return value. The _weak function gets clock-frequency from device tree. If there is not, platform codes which uses the ns16550 driver should provide the implementation of get_serial_clock(). Thoughts?
There is no clock-frequency property in DT, at least for the Tegra DT binding. It looks like some other bindings have it. To obtain the clock frequency from DT for Tegra, you'd need to parse the clocks property, find the clock driver associated with the phandle in DT, and go and ask that clock driver what the clock frequency is.
I'd prefer not to have a weak function that parses clock-frequency, since it's too easy to accidentally use it on systems where parsing that property is incorrect.
Certainly, a generic UART driver can call out to a platform-supplied function to retrieve the clock, and we can provide driver-specific implementations for x86 super IO and PCI, and generic implementations that appropriate drivers can call to parse the clocks or clock-frequency property from DT, and finally for Tegra if we can't parse the clocks property right now, call the Tegra clock driver directly to look up the value.
I'm not a big fan of weak functions either. In fact I think with driver model we should avoid them. If we can't call a uclass to get the info then perhaps we should wait until we can.
Pragmatically I wonder if a UART clock frequency would not be a useful compromise? Some bindings have it, some do not. Maybe we should just add it?
There's no need for it; the binding already has a clocks property, from which the data can be derived. Adding a clock-frequency property would just result in two sources of the same data. In all likelihood, all that'd happen is that the two would get out-of-sync, and code wouldn't know which to trust.
I agree. So what's our next step? So far I still think using _weak is the easiest approach. Adding clock uclass for fixed clock-frequency chipset like x86 super i/o and PCI UART does not make sense. The clock uclass is only helpful when dealing with dynamic clock frequency platforms on PPC and ARM SoC.
Regards, Bin
On 08/14/2015 09:12 PM, Bin Meng wrote:
On Sat, Aug 15, 2015 at 10:57 AM, Stephen Warren swarren@wwwdotorg.org wrote:
On 08/14/2015 05:18 PM, Simon Glass wrote:
Hi,
On 14 August 2015 at 16:51, Stephen Warren swarren@wwwdotorg.org wrote:
On 08/14/2015 04:40 PM, Bin Meng wrote:
On Sat, Aug 15, 2015 at 12:59 AM, Simon Glass sjg@chromium.org wrote:
Hi Stephen,
On 14 August 2015 at 10:58, Stephen Warren swarren@wwwdotorg.org wrote: > > On 08/14/2015 10:50 AM, Simon Glass wrote: >> >> >> Hi Bin, >> >> On 14 August 2015 at 03:18, Bin Meng bmeng.cn@gmail.com wrote: >>> >>> >>> Hi, >>> >>> Currently there are 5 dm serial drivers, all of which are ns16550 >>> compatible drivers. They are: >>> >>> serial_omap.c >>> serial_dw.c >>> serial_tegra.c >>> serial_x86.c >>> serial_ppc.c >>> >>> All these drivers are pretty much similar. I think we can justmerge >>> these into one ns16550 driver. >>> >>> If you think this is necessary, I will send a patch series to do this. >> >> >> >> The tegra one is there because it needs an input clock and Stephen >> didn't want to add this to the device tree binding (the kernel has a >> clock framework which gets around this problem). >> >> After that I followed the same pattern. I would support updating the >> binding to support an input clock. Even with the new clock framework >> in U-Boot it might be painful to fit it into SPL in some cases. > > > > The clock is already in the DT, in both Linux and U-Boot's copy, at > least > for Tegra DTs: > > uarta: serial@0,70006000 { > compatible = "nvidia,tegra124-uart", ... > ... > clocks = <&tegra_car TEGRA124_CLK_UARTA>; >
I mean the clock-frequency property. However if there is a plan to implement the clock framework in U-Boot that would be good too.
The clock-frequency is a fixed value on x86 super i/o chipset, and fixed on the PCI bus too. But for ARM and PPC, it might get dynamically calculated due to different PLL settings. We can implement a _weak function like the one in serial_ppc.c get_serial_clock() to initialize plat->clock with its return value. The _weak function gets clock-frequency from device tree. If there is not, platform codes which uses the ns16550 driver should provide the implementation of get_serial_clock(). Thoughts?
There is no clock-frequency property in DT, at least for the Tegra DT binding. It looks like some other bindings have it. To obtain the clock frequency from DT for Tegra, you'd need to parse the clocks property, find the clock driver associated with the phandle in DT, and go and ask that clock driver what the clock frequency is.
I'd prefer not to have a weak function that parses clock-frequency, since it's too easy to accidentally use it on systems where parsing that property is incorrect.
Certainly, a generic UART driver can call out to a platform-supplied function to retrieve the clock, and we can provide driver-specific implementations for x86 super IO and PCI, and generic implementations that appropriate drivers can call to parse the clocks or clock-frequency property from DT, and finally for Tegra if we can't parse the clocks property right now, call the Tegra clock driver directly to look up the value.
I'm not a big fan of weak functions either. In fact I think with driver model we should avoid them. If we can't call a uclass to get the info then perhaps we should wait until we can.
Pragmatically I wonder if a UART clock frequency would not be a useful compromise? Some bindings have it, some do not. Maybe we should just add it?
There's no need for it; the binding already has a clocks property, from which the data can be derived. Adding a clock-frequency property would just result in two sources of the same data. In all likelihood, all that'd happen is that the two would get out-of-sync, and code wouldn't know which to trust.
I agree. So what's our next step? So far I still think using _weak is the easiest approach. Adding clock uclass for fixed clock-frequency chipset like x86 super i/o and PCI UART does not make sense. The clock uclass is only helpful when dealing with dynamic clock frequency platforms on PPC and ARM SoC.
I think having the core UART driver call a function to get the clock rate is perfectly fine. Just don't provide a _weak version of the function. This will require all users of the generic UART driver to make an explicit choice about how to implement this "callback" or "hook", which makes it most likely that it'll be correctly implemented on all platforms for all UARTs.
Platforms (or DT compatible value handlers, or ACPI whatever handlers, or ...) could either pass this function into the generic UART driver during instantiation/initialization, or put a function pointer into some configuration structure that's passed to the generic UART driver during instantiation/initialization, or something like that. The function shouldn't be a global symbol. There can be only one implementation of a global symbol. We need to support many implementations within the same U-Boot binary. Consider a board containing both an ARM SoC with some on-SoC UARTs where the clock rate is dynamically controlled by some on-chip clock module, and also containing a PCI port into which a PCI UART card is plugged, which requires a hard-coded clock rate (perhaps globally fixed for all PCI UARTs, or based on a lookup table from PCI vendor/device ID).
Hi Stephen,
On 14 August 2015 at 20:57, Stephen Warren swarren@wwwdotorg.org wrote:
On 08/14/2015 05:18 PM, Simon Glass wrote:
Hi,
On 14 August 2015 at 16:51, Stephen Warren swarren@wwwdotorg.org wrote:
On 08/14/2015 04:40 PM, Bin Meng wrote:
On Sat, Aug 15, 2015 at 12:59 AM, Simon Glass sjg@chromium.org wrote:
Hi Stephen,
On 14 August 2015 at 10:58, Stephen Warren swarren@wwwdotorg.org wrote:
On 08/14/2015 10:50 AM, Simon Glass wrote: > > > Hi Bin, > > On 14 August 2015 at 03:18, Bin Meng bmeng.cn@gmail.com wrote: >> >> >> Hi, >> >> Currently there are 5 dm serial drivers, all of which are ns16550 >> compatible drivers. They are: >> >> serial_omap.c >> serial_dw.c >> serial_tegra.c >> serial_x86.c >> serial_ppc.c >> >> All these drivers are pretty much similar. I think we can justmerge >> these into one ns16550 driver. >> >> If you think this is necessary, I will send a patch series to do this. > > > > The tegra one is there because it needs an input clock and Stephen > didn't want to add this to the device tree binding (the kernel has a > clock framework which gets around this problem). > > After that I followed the same pattern. I would support updating the > binding to support an input clock. Even with the new clock framework > in U-Boot it might be painful to fit it into SPL in some cases.
The clock is already in the DT, in both Linux and U-Boot's copy, at least for Tegra DTs:
uarta: serial@0,70006000 { compatible = "nvidia,tegra124-uart", ...... clocks = <&tegra_car TEGRA124_CLK_UARTA>;
I mean the clock-frequency property. However if there is a plan to implement the clock framework in U-Boot that would be good too.
The clock-frequency is a fixed value on x86 super i/o chipset, and fixed on the PCI bus too. But for ARM and PPC, it might get dynamically calculated due to different PLL settings. We can implement a _weak function like the one in serial_ppc.c get_serial_clock() to initialize plat->clock with its return value. The _weak function gets clock-frequency from device tree. If there is not, platform codes which uses the ns16550 driver should provide the implementation of get_serial_clock(). Thoughts?
There is no clock-frequency property in DT, at least for the Tegra DT binding. It looks like some other bindings have it. To obtain the clock frequency from DT for Tegra, you'd need to parse the clocks property, find the clock driver associated with the phandle in DT, and go and ask that clock driver what the clock frequency is.
I'd prefer not to have a weak function that parses clock-frequency, since it's too easy to accidentally use it on systems where parsing that property is incorrect.
Certainly, a generic UART driver can call out to a platform-supplied function to retrieve the clock, and we can provide driver-specific implementations for x86 super IO and PCI, and generic implementations that appropriate drivers can call to parse the clocks or clock-frequency property from DT, and finally for Tegra if we can't parse the clocks property right now, call the Tegra clock driver directly to look up the value.
I'm not a big fan of weak functions either. In fact I think with driver model we should avoid them. If we can't call a uclass to get the info then perhaps we should wait until we can.
Pragmatically I wonder if a UART clock frequency would not be a useful compromise? Some bindings have it, some do not. Maybe we should just add it?
There's no need for it; the binding already has a clocks property, from which the data can be derived. Adding a clock-frequency property would just result in two sources of the same data. In all likelihood, all that'd happen is that the two would get out-of-sync, and code wouldn't know which to trust.
There clearly is a need for it, since I wrote a driver which hard-codes it.
Even if/when someone puts generic clock support into Tegra it is unlikely to be worth spinning up all that clock tree stuff just to find out this value in SPL. It would be MUCH simpler if we just added an optional clock-frequency property to the binding. As Bin points out, it is commonly used in other bindings.
Regards, Simon
participants (3)
-
Bin Meng -
Simon Glass -
Stephen Warren