Introduce driver model serial uclass

Quoted 8 lines · Simon Glass

If the sandbox device tree is provided to U-Boot (with the -d flag) then it
will use the device tree version in preference to the built-in device. The only difference is the colour.

arch/sandbox/dts/sandbox.dts +0 -0

arch/sandbox/dts/sandbox.dts line 10

Quoted 3 lines

arch/sandbox/dts/sandbox.dts +3 -0

+       uart0: serial {
+               compatible = "sandbox,serial";
+               u-boot,dm-pre-reloc;

Shouldn't that be handled by the driver. It's certainly something that's only relevant to the internals of U-Boot, and hence inappropriate to put into DT.

Quoted 2 lines

+ text-colour = "cyan";

That's property should likely have a uboot, prefix, since it's non-standard.

Hi Stephen,

Quoted 16 lines · Stephen Warren

Quoted 13 lines · Simon Glass

If the sandbox device tree is provided to U-Boot (with the -d flag) then it
will use the device tree version in preference to the built-in device. The only difference is the colour.

arch/sandbox/dts/sandbox.dts +3 -0

 
 
+       uart0: serial {
+               compatible = "sandbox,serial";
+               u-boot,dm-pre-reloc;

Quoted 3 lines

Shouldn't that be handled by the driver. It's certainly something that's only relevant to the internals of U-Boot, and hence inappropriate to put into DT.

Hence the u-boot prefix. This is described in the driver model docs. I have found a work-around (which forces a driver to be inited pre-reloc if none is found) but I'm not 100% happy with it.

Quoted 4 lines

Quoted 1 line

+ text-colour = "cyan";

That's property should likely have a uboot, prefix, since it's non-standard.

Can I not just declare a binding for 'sandbox,serial'?

Quoted 11 lines

Quoted 7 lines

arch/sandbox/dts/sandbox.dts line 10

Quoted 3 lines

arch/sandbox/dts/sandbox.dts +3 -0

+       uart0: serial {
+               compatible = "sandbox,serial";
+               u-boot,dm-pre-reloc;

Shouldn't that be handled by the driver. It's certainly something that's only relevant to the internals of U-Boot, and hence inappropriate to put into DT.

Hence the u-boot prefix. This is described in the driver model docs. I have found a work-around (which forces a driver to be inited pre-reloc if none is found) but I'm not 100% happy with it.

I'm arguing that the property shouldn't exist in DT at all. DT is supposed to be a pure description of the HW, and not encode details that are specific to the implementation of particular SW. The fact that U-Boot performs relocation of its code during boot is completely irrelevant to a HW description.

As such, the issue isn't whether there is a u-boot, prefix on that property, but whether it's there at all.

Quoted 18 lines

Quoted 11 lines

Quoted 7 lines

arch/sandbox/dts/sandbox.dts line 10

Quoted 3 lines

arch/sandbox/dts/sandbox.dts +3 -0

+       uart0: serial {
+               compatible = "sandbox,serial";
+               u-boot,dm-pre-reloc;

Shouldn't that be handled by the driver. It's certainly something that's only relevant to the internals of U-Boot, and hence inappropriate to put into DT.

Hence the u-boot prefix. This is described in the driver model docs. I have found a work-around (which forces a driver to be inited pre-reloc if none is found) but I'm not 100% happy with it.

I'm arguing that the property shouldn't exist in DT at all. DT is supposed to be a pure description of the HW, and not encode details that are specific to the implementation of particular SW. The fact that U-Boot performs relocation of its code during boot is completely irrelevant to a HW description.

Quoted 2 lines

As such, the issue isn't whether there is a u-boot, prefix on that property, but whether it's there at all.

Right. And I've arguing that U-Boot should use exactly the same descriptions that are in the Kernel even. Those DTS descriptions should be common, applicable to both or neither, exactly because they do describe the HW and are agnostic WRT the SW that is using them.

Quoted 21 lines · Jon Loeliger

Quoted 20 lines

Quoted 11 lines

Quoted 7 lines

arch/sandbox/dts/sandbox.dts line 10

Quoted 3 lines

arch/sandbox/dts/sandbox.dts +3 -0

+       uart0: serial {
+               compatible = "sandbox,serial";
+               u-boot,dm-pre-reloc;

Shouldn't that be handled by the driver. It's certainly something that's only relevant to the internals of U-Boot, and hence inappropriate to put into DT.

Hence the u-boot prefix. This is described in the driver model docs. I have found a work-around (which forces a driver to be inited pre-reloc if none is found) but I'm not 100% happy with it.

I'm arguing that the property shouldn't exist in DT at all. DT is supposed to be a pure description of the HW, and not encode details that are specific to the implementation of particular SW. The fact that U-Boot performs relocation of its code during boot is completely irrelevant to a HW description.

As such, the issue isn't whether there is a u-boot, prefix on that property, but whether it's there at all.

Quoted 4 lines

Right. And I've arguing that U-Boot should use exactly the same descriptions that are in the Kernel even. Those DTS descriptions should be common, applicable to both or neither, exactly because they do describe the HW and are agnostic WRT the SW that is using them.

I also agree with this. We'll have to sort out the fall-out.

Quoted 26 lines · Simon Glass

Hi Stephen,

Quoted 22 lines · Stephen Warren

Quoted 17 lines · Simon Glass

If the sandbox device tree is provided to U-Boot (with the -d flag) then it
will use the device tree version in preference to the built-in device. The
only difference is the colour.

arch/sandbox/dts/sandbox.dts +3 -0

 
 
 
+       uart0: serial {
+               compatible = "sandbox,serial";
+               u-boot,dm-pre-reloc;

Quoted 8 lines

Quoted 3 lines

Shouldn't that be handled by the driver. It's certainly something that's only relevant to the internals of U-Boot, and hence inappropriate to put into DT.

Hence the u-boot prefix. This is described in the driver model docs. I have found a work-around (which forces a driver to be inited pre-reloc if none is found) but I'm not 100% happy with it.

I'm arguing that the property shouldn't exist in DT at all. DT is supposed to be a pure description of the HW, and not encode details that are specific to the implementation of particular SW. The fact that U-Boot performs relocation of its code during boot is completely irrelevant to a HW description.

As such, the issue isn't whether there is a u-boot, prefix on that property, but whether it's there at all.

Quoted 11 lines

Quoted 8 lines

Quoted 3 lines

+ text-colour = "cyan";

That's property should likely have a uboot, prefix, since it's non-standard.

Can I not just declare a binding for 'sandbox,serial'?

Properties that are relevant only to a particular binding, rather than being something quite generic an applicable to a whole class of devices, typically have a vendor prefix.

A binding should/must exist for every node or compatible value. So, whether you actually write the binding document or not makes no difference to the names or vendor prefixes of the properties the binding uses.

Hi Stephen,

Quoted 18 lines · Stephen Warren

Quoted 17 lines · Simon Glass

Hi Stephen,

Quoted 13 lines · Stephen Warren

Quoted 10 lines · Simon Glass

If the sandbox device tree is provided to U-Boot (with the -d flag) then it will use the device tree version in preference to the built-in device. The only difference is the colour.

arch/sandbox/dts/sandbox.dts +3 -0

+       uart0: serial {
+               compatible = "sandbox,serial";
+               u-boot,dm-pre-reloc;

Quoted 9 lines

Quoted 8 lines

Quoted 6 lines

Quoted 1 line

+ text-colour = "cyan";

That's property should likely have a uboot, prefix, since it's non-standard.

Can I not just declare a binding for 'sandbox,serial'?

Quoted 7 lines

Properties that are relevant only to a particular binding, rather than being something quite generic an applicable to a whole class of devices, typically have a vendor prefix.

A binding should/must exist for every node or compatible value. So, whether you actually write the binding document or not makes no difference to the names or vendor prefixes of the properties the binding uses.

So here you are saying it should be:

    sandbox,text-colour = "cyan";

Is that right?

Change the Exynos serial driver to work with driver model and switch over all Exynos5 boards to use it.

Signed-off-by Simon Glass <[email protected]>

---

Changes in v3:
- Avoid reordering functions

Quoted 8 lines · Simon Glass

Change the Exynos serial driver to work with driver model and switch over all Exynos5 boards to use it.

Signed-off-by Simon Glass <[email protected]>

---

Changes in v3:
- Avoid reordering functions

Yay.

Quoted 4 lines

drivers/serial/serial_s5p.c +0 -0

@@ -108,14 +91,11 @@ static int serial_init_dev(const int dev_index)
      /* No interrupts, no DMA, pure polling */
      writel(0x245, &uart->ucon);
 

drivers/serial/serial_s5p.c static int serial_init_dev(const int dev_index) · line 95

Quoted 4 lines

drivers/serial/serial_s5p.c +0 -2

-     serial_setbrg_dev(dev_index);
-
      return 0;
 }

So setbrg is called elsewhere, in a more common area? That's documented somewhere right?

This seems otherwise to be a straight-forward
conversion. Things we used to loop for in the driver are now handled a bit higher up and otherwise we find what to talk to in a slightly different manner.

Some Tegra device tree files do not include information about the serial ports. Add this and also add information about the input clock speed.

The console alias needs to be set up to indicate which port is used for the console.

Also add a binding file since this is missing.

arch/arm/dts/tegra114-dalmore.dts +0 -0

index 435c01e..e2426ef 100644

Quoted 18 lines · Simon Glass

Some Tegra device tree files do not include information about the serial ports. Add this and also add information about the input clock speed.

The console alias needs to be set up to indicate which port is used for the console.

Also add a binding file since this is missing.

arch/arm/dts/tegra114-dalmore.dts +0 -0

index 435c01e..e2426ef 100644
@@ -7,6 +7,7 @@
        compatible = "nvidia,dalmore", "nvidia,tegra114";
 

arch/arm/dts/tegra114-dalmore.dts line 10

Quoted 2 lines

arch/arm/dts/tegra114-dalmore.dts +1 -0

        aliases {
+               console = &uart_d;

I don't think that's a standard alias name. There was some recent discussion in the devicetree mailing list re: using some property in /chosen for this purpose instead. U-Boot and the kernel should use the same representation here.

arch/arm/dts/tegra114.dtsi line 120

Quoted 6 lines

arch/arm/dts/tegra114.dtsi +2 -0

 
 
+       uart_a: serial at 70006000 {
+               compatible = "nvidia,tegra20-uart";

This property needs to include both the specific HW (i.e. Tegra114) and any HW it's compatible with (i.e. Tegra20).

arch/arm/dts/tegra114.dtsi line 123

Quoted 5 lines

arch/arm/dts/tegra114.dtsi +3 -0

+               reg = <0x70006000 0x40>;
+               reg-shift = <2>;
+               clock-frequency = <408000000>;

This isn't a property that's defined by the Tegra serial binding. This information should be obtained by looking up the relevant clock, and querying its rate.

arch/arm/dts/tegra114.dtsi

Quoted 4 lines

arch/arm/dts/tegra114.dtsi +2 -0

+               interrupts = < 68 >;
+       };

For reference, here's the DT node for this UART in the kernel DT, which complies with the relevant binding document:

        uarta: serial at 70006000 {
                compatible = "nvidia,tegra114-uart", "nvidia,tegra20-uart";

                reg = <0x70006000 0x40>;
                reg-shift = <2>;
                interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
                clocks = <&tegra_car TEGRA114_CLK_UARTA>;
                resets = <&tegra_car 6>;
                reset-names = "serial";
                dmas = <&apbdma 8>, <&apbdma 8>;
                dma-names = "rx", "tx";
                status = "disabled";
        };

All the comment above apply to all the files in this patch.

Hi Stephen,

Quoted 19 lines · Stephen Warren

Quoted 18 lines · Simon Glass

Some Tegra device tree files do not include information about the serial ports. Add this and also add information about the input clock speed.

The console alias needs to be set up to indicate which port is used for the console.

Also add a binding file since this is missing.

arch/arm/dts/tegra114-dalmore.dts +0 -0

index 435c01e..e2426ef 100644
@@ -7,6 +7,7 @@
         compatible = "nvidia,dalmore", "nvidia,tegra114";
 

Quoted 8 lines

arch/arm/dts/tegra114-dalmore.dts line 10

Quoted 2 lines

arch/arm/dts/tegra114-dalmore.dts +1 -0

         aliases {
+               console = &uart_d;

I don't think that's a standard alias name. There was some recent discussion in the devicetree mailing list re: using some property in /chosen for this purpose instead. U-Boot and the kernel should use the same representation here.

This is U-Boot's approach at present,

if we change it then we should
change it everywhere. I worry that 'chosen' is for Linux rather than U-Boot and we might get very confused about what chosen is for?

Quoted 7 lines

Quoted 5 lines

arch/arm/dts/tegra114.dtsi +2 -0

 
 
+       uart_a: serial at 70006000 {
+               compatible = "nvidia,tegra20-uart";

Quoted 2 lines

This property needs to include both the specific HW (i.e. Tegra114) and any HW it's compatible with (i.e. Tegra20).

So something like this?

compatible = "nvidia,tegra114-uart", "nvidia,tegra20-uart";

Quoted 5 lines

Quoted 3 lines

arch/arm/dts/tegra114.dtsi +3 -0

+               reg = <0x70006000 0x40>;
+               reg-shift = <2>;
+               clock-frequency = <408000000>;

Quoted 3 lines

This isn't a property that's defined by the Tegra serial binding. This information should be obtained by looking up the relevant clock, and querying its rate.

We can't do that in the ns16550 driver as yet since there is no generic U-Boot clock infrastructure. I suspect that will come with time.

Quoted 17 lines

For reference, here's the DT node for this UART in the kernel DT, which complies with the relevant binding document:

         uarta: serial at 70006000 {
                 compatible = "nvidia,tegra114-uart", "nvidia,tegra20-uart";

                 reg = <0x70006000 0x40>;
                 reg-shift = <2>;
                 interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
                 clocks = <&tegra_car TEGRA114_CLK_UARTA>;
                 resets = <&tegra_car 6>;
                 reset-names = "serial";
                 dmas = <&apbdma 8>, <&apbdma 8>;
                 dma-names = "rx", "tx";
                 status = "disabled";
         };

Quoted 1 line

All the comment above apply to all the files in this patch.

My intent was to make this work with a more generic binding for now - ns16550 is a pretty standard thing and I thought I could avoid making the driver Tegra-specific. Then we could allow many SoCs to use it. Why does Tegra have its own binding in the kernel for this standard UART?

Quoted 39 lines · Simon Glass

Hi Stephen,

Quoted 22 lines · Stephen Warren

Quoted 20 lines · Simon Glass

Some Tegra device tree files do not include information about the serial ports. Add this and also add information about the input clock speed.

The console alias needs to be set up to indicate which port is used for the console.

Also add a binding file since this is missing.

arch/arm/dts/tegra114-dalmore.dts +0 -0

index 435c01e..e2426ef 100644
@@ -7,6 +7,7 @@
         compatible = "nvidia,dalmore", "nvidia,tegra114";
 

Quoted 11 lines

arch/arm/dts/tegra114-dalmore.dts line 10

Quoted 2 lines

arch/arm/dts/tegra114-dalmore.dts +1 -0

         aliases {
+               console = &uart_d;

I don't think that's a standard alias name. There was some recent discussion
in the devicetree mailing list re: using some property in /chosen for this
purpose instead. U-Boot and the kernel should use the same representation here.

Quoted 1 line

This is U-Boot's approach at present,

That's not the U-Boot approach on Tegra boards before this patch. I do not want Tegra U-Boot do adopt any more U-Boot-specific not-really-DT-but-pretending-to-be bindings.

Quoted 4 lines

if we change it then we should

change it everywhere. I worry that 'chosen' is for Linux rather than U-Boot and we might get very confused about what chosen is for?

That discussion should be had on the devicetree mailing list.

Quoted 11 lines

Quoted 11 lines

Quoted 8 lines

arch/arm/dts/tegra114.dtsi +2 -0

 
 
 
+       uart_a: serial at 70006000 {
+               compatible = "nvidia,tegra20-uart";

Quoted 8 lines

Quoted 3 lines

This property needs to include both the specific HW (i.e. Tegra114) and any
HW it's compatible with (i.e. Tegra20).

So something like this?

compatible = "nvidia,tegra114-uart", "nvidia,tegra20-uart";

Yes.

Quoted 6 lines

Quoted 6 lines

Quoted 3 lines

arch/arm/dts/tegra114.dtsi +3 -0

+               reg = <0x70006000 0x40>;
+               reg-shift = <2>;
+               clock-frequency = <408000000>;

Quoted 8 lines

Quoted 3 lines

This isn't a property that's defined by the Tegra serial binding. This information should be obtained by looking up the relevant clock, and querying its rate.

We can't do that in the ns16550 driver as yet since there is no generic U-Boot clock infrastructure. I suspect that will come with time.

The solution here is to put the clock infra-structure in place first. One thing I've learned from the kernel DT experience is that a lot of time would have been saved by putting the correct infra-structure in place first, then using it, rather than hacking around things the wrong way, then putting the infra-structure in place, then converting to it. That's a lot more work, and rather painful. Equally, if we don't just do the infra-structure right, there's really little guarantee that we'll ever convert to the correct approach. Just look at all the DT content in use in U-Boot that don't match the real DT bindings, even after it's been around years.

Quoted 20 lines

Quoted 20 lines

For reference, here's the DT node for this UART in the kernel DT, which complies with the relevant binding document:

uarta: serial at 70006000 {
compatible = "nvidia,tegra114-uart",
"nvidia,tegra20-uart";

                 reg = <0x70006000 0x40>;
                 reg-shift = <2>;
                 interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
                 clocks = <&tegra_car TEGRA114_CLK_UARTA>;
                 resets = <&tegra_car 6>;
                 reset-names = "serial";
                 dmas = <&apbdma 8>, <&apbdma 8>;
                 dma-names = "rx", "tx";
                 status = "disabled";
         };

Quoted 8 lines

Quoted 1 line

All the comment above apply to all the files in this patch.

My intent was to make this work with a more generic binding for now - ns16550 is a pretty standard thing and I thought I could avoid making the driver Tegra-specific. Then we could allow many SoCs to use it. Why does Tegra have its own binding in the kernel for this standard UART?

The HW is not a PC-style UART where all you care about is the 16550 registers, and clocks/resets/DMA/... can be ignored or deferred to firmware to set up before DT-driven SW runs.

As an aside, /almost/ all reviewed DT bindings use DT properties of the form:

clocks = <&provider parameters>;

rather than:

clock-rate = <number>;

So, that aspect of the Tegra UART binding isn't anything remotely unusual/non-standard.

Hi Stephen,

Quoted 50 lines · Stephen Warren

Quoted 39 lines · Simon Glass

Hi Stephen,

Quoted 22 lines · Stephen Warren

Quoted 20 lines · Simon Glass

Some Tegra device tree files do not include information about the serial ports. Add this and also add information about the input clock speed.

The console alias needs to be set up to indicate which port is used for the console.

Also add a binding file since this is missing.

arch/arm/dts/tegra114-dalmore.dts +0 -0

index 435c01e..e2426ef 100644
@@ -7,6 +7,7 @@
          compatible = "nvidia,dalmore", "nvidia,tegra114";
 

Quoted 11 lines

arch/arm/dts/tegra114-dalmore.dts line 10

Quoted 2 lines

arch/arm/dts/tegra114-dalmore.dts +1 -0

          aliases {
+               console = &uart_d;

I don't think that's a standard alias name. There was some recent discussion
in the devicetree mailing list re: using some property in /chosen for this
purpose instead. U-Boot and the kernel should use the same representation here.

Quoted 1 line

This is U-Boot's approach at present,

That's not the U-Boot approach on Tegra boards before this patch. I do not want Tegra U-Boot do adopt any more U-Boot-specific not-really-DT-but-pretending-to-be bindings.

Quoted 2 lines

if we change it then we should

Quoted 5 lines

Quoted 2 lines

change it everywhere. I worry that 'chosen' is for Linux rather than U-Boot and we might get very confused about what chosen is for?

That discussion should be had on the devicetree mailing list.

Please go ahead if you wish, but this is not a Linux issue. The aliases are used for U-Boot and have been for some time.

Quoted 15 lines

Quoted 11 lines

Quoted 10 lines

arch/arm/dts/tegra114.dtsi line 123

Quoted 6 lines

arch/arm/dts/tegra114.dtsi +5 -0

+       uart_a: serial at 70006000 {
+               compatible = "nvidia,tegra20-uart";
+               reg = <0x70006000 0x40>;
+               reg-shift = <2>;
+               clock-frequency = <408000000>;

This isn't a property that's defined by the Tegra serial binding. This information should be obtained by looking up the relevant clock, and querying its rate.

Quoted 3 lines

We can't do that in the ns16550 driver as yet since there is no generic U-Boot clock infrastructure. I suspect that will come with time.

Quoted 9 lines

The solution here is to put the clock infra-structure in place first. One thing I've learned from the kernel DT experience is that a lot of time would have been saved by putting the correct infra-structure in place first, then using it, rather than hacking around things the wrong way, then putting the infra-structure in place, then converting to it. That's a lot more work, and rather painful. Equally, if we don't just do the infra-structure right, there's really little guarantee that we'll ever convert to the correct approach. Just look at all the DT content in use in U-Boot that don't match the real DT bindings, even after it's been around years.

OK, is there a plan to put this in place? Who is working on it?

Quoted 27 lines

Quoted 26 lines

Quoted 18 lines

For reference, here's the DT node for this UART in the kernel DT, which complies with the relevant binding document:

uarta: serial at 70006000 {
compatible = "nvidia,tegra114-uart",
"nvidia,tegra20-uart";

                  reg = <0x70006000 0x40>;
                  reg-shift = <2>;
                  interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
                  clocks = <&tegra_car TEGRA114_CLK_UARTA>;
                  resets = <&tegra_car 6>;
                  reset-names = "serial";
                  dmas = <&apbdma 8>, <&apbdma 8>;
                  dma-names = "rx", "tx";
                  status = "disabled";
          };

Quoted 1 line

All the comment above apply to all the files in this patch.

My intent was to make this work with a more generic binding for now - ns16550 is a pretty standard thing and I thought I could avoid making the driver Tegra-specific. Then we could allow many SoCs to use it. Why does Tegra have its own binding in the kernel for this standard UART?

Quoted 3 lines

The HW is not a PC-style UART where all you care about is the 16550 registers, and clocks/resets/DMA/... can be ignored or deferred to firmware to set up before DT-driven SW runs.

I'm not sure what you are referring to here. Are you saying that U-Boot needs to support these things? I agree it would be great to add a generic clock/reset framework and have made considerable efforts in Tegra towards this myself. But we don't have it yet.

Quoted 7 lines

As an aside, /almost/ all reviewed DT bindings use DT properties of the form:

clocks = <&provider parameters>;

rather than:

Quoted 4 lines

clock-rate = <number>;

So, that aspect of the Tegra UART binding isn't anything remotely unusual/non-standard.

The great is the enemy of the good. In this case, I think I might just leave the clock as a #define.

Quoted 71 lines · Simon Glass

Hi Stephen,

Quoted 67 lines · Stephen Warren

Quoted 48 lines · Simon Glass

Hi Stephen,

Quoted 27 lines · Stephen Warren

Quoted 24 lines · Simon Glass

Some Tegra device tree files do not include information about the serial
ports. Add this and also add information about the input clock speed.

The console alias needs to be set up to indicate which port is used for
the console.

Also add a binding file since this is missing.

arch/arm/dts/tegra114-dalmore.dts +0 -0

index 435c01e..e2426ef 100644
@@ -7,6 +7,7 @@
          compatible = "nvidia,dalmore", "nvidia,tegra114";
 

Quoted 13 lines

arch/arm/dts/tegra114-dalmore.dts line 10

Quoted 2 lines

arch/arm/dts/tegra114-dalmore.dts +1 -0

          aliases {
+               console = &uart_d;

I don't think that's a standard alias name. There was some recent discussion
in the devicetree mailing list re: using some property in /chosen for this
purpose instead. U-Boot and the kernel should use the same representation
here.

Quoted 1 line

This is U-Boot's approach at present,

That's not the U-Boot approach on Tegra boards before this patch. I do not
want Tegra U-Boot do adopt any more U-Boot-specific not-really-DT-but-pretending-to-be bindings.

Quoted 4 lines

if we change it then we should

change it everywhere. I worry that 'chosen' is for Linux rather than U-Boot and we might get very confused about what chosen is for?

Quoted 5 lines

Quoted 1 line

That discussion should be had on the devicetree mailing list.

Please go ahead if you wish, but this is not a Linux issue. The aliases are used for U-Boot and have been for some time.

No, it's not a Linux issue, it's DT issue.

DT schemas/bindings MUST be identical between U-Boot, Linux, FreeBSD, Barebox, ... (all of which use DT). As such, all the DT bindings MUST be discussed on the devicetree mailing list.

Since you're the author of the patch, it's your responsibility to have that discussion.

Quoted 35 lines

Quoted 33 lines

Quoted 14 lines

Quoted 12 lines

arch/arm/dts/tegra114.dtsi line 123

Quoted 7 lines

arch/arm/dts/tegra114.dtsi +5 -0

+       uart_a: serial at 70006000 {
+               compatible = "nvidia,tegra20-uart";
+               reg = <0x70006000 0x40>;
+               reg-shift = <2>;
+               clock-frequency = <408000000>;

This isn't a property that's defined by the Tegra serial binding. This information should be obtained by looking up the relevant clock, and querying its rate.

Quoted 3 lines

We can't do that in the ns16550 driver as yet since there is no generic U-Boot clock infrastructure. I suspect that will come with time.

The solution here is to put the clock infra-structure in place first. One thing I've learned from the kernel DT experience is that a lot of time would
have been saved by putting the correct infra-structure in place first, then
using it, rather than hacking around things the wrong way, then putting the
infra-structure in place, then converting to it. That's a lot more work, and
rather painful. Equally, if we don't just do the infra-structure right, there's really little guarantee that we'll ever convert to the correct approach. Just look at all the DT content in use in U-Boot that don't match
the real DT bindings, even after it's been around years.

Quoted 1 line

OK, is there a plan to put this in place? Who is working on it?

I don't know whether anyone is or not. However, the fact that nobody is working on a clock driver is no excuse for using the incorrect DT bindings in order to hack around its lack of existence.

Quoted 37 lines

Quoted 31 lines

Quoted 24 lines

Quoted 20 lines

For reference, here's the DT node for this UART in the kernel DT, which complies with the relevant binding document:

uarta: serial at 70006000 {
compatible = "nvidia,tegra114-uart",
"nvidia,tegra20-uart";

                  reg = <0x70006000 0x40>;
                  reg-shift = <2>;
                  interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
                  clocks = <&tegra_car TEGRA114_CLK_UARTA>;
                  resets = <&tegra_car 6>;
                  reset-names = "serial";
                  dmas = <&apbdma 8>, <&apbdma 8>;
                  dma-names = "rx", "tx";
                  status = "disabled";
          };

Quoted 1 line

All the comment above apply to all the files in this patch.

Quoted 5 lines

My intent was to make this work with a more generic binding for now - ns16550 is a pretty standard thing and I thought I could avoid making the driver Tegra-specific. Then we could allow many SoCs to use it. Why does Tegra have its own binding in the kernel for this standard UART?

Quoted 4 lines

The HW is not a PC-style UART where all you care about is the 16550 registers, and clocks/resets/DMA/... can be ignored or deferred to firmware
to set up before DT-driven SW runs.

Quoted 4 lines

I'm not sure what you are referring to here. Are you saying that U-Boot needs to support these things? I agree it would be great to add a generic clock/reset framework and have made considerable efforts in Tegra towards this myself. But we don't have it yet.

Yes.

Part of the decision to use DT is to use *the* DT bindings, not something that looks like DT.

If you want to move the serial port information into DT, and part of doing so requires extracting clock-related information from DT, and that in turn requires a clock driver to do so, then yes, U-Boot needs to have that clock driver implemented before you can move the serial port information into DT.

Quoted 15 lines

Quoted 13 lines

As an aside, /almost/ all reviewed DT bindings use DT properties of the form:

clocks = <&provider parameters>;

rather than:

clock-rate = <number>;

So, that aspect of the Tegra UART binding isn't anything remotely unusual/non-standard.

Quoted 2 lines

The great is the enemy of the good. In this case, I think I might just leave the clock as a #define.

That probably makes sense.

To be honest, I'm not sure that using DT is going to buy U-Boot much, or even the kernel in retrospect.

On 1 August 2014 15:50, Stephen Warren <[email protected]> wrote:

Quoted 6 lines

DT schemas/bindings MUST be identical between U-Boot, Linux, FreeBSD, Barebox, ... (all of which use DT). As such, all the DT bindings MUST be discussed on the devicetree mailing list.

Since you're the author of the patch, it's your responsibility to have that discussion.

Are you referring to the linux,stdout-path discussion, or something more DT-generic?I suppose we could have a 'u-boot,console' for our part. But in any case you are talking about code and a convention that is already in mainline U-Boot.

While I accept that we might change to
something DT-generic if Linux points the way to something better, I don't want to stop using it just because Linux hasn't decided yet. The early console stuff and early debug UART stuff in Linux is not yet a shining example of perfection.

That said, it's a good time to adopt 'u-boot,console' if that's what we need?

Quoted 37 lines

Quoted 35 lines

Quoted 18 lines

Quoted 13 lines

Quoted 11 lines

arch/arm/dts/tegra114.dtsi

Quoted 6 lines

arch/arm/dts/tegra114.dtsi +5 -0

+       uart_a: serial at 70006000 {
+               compatible = "nvidia,tegra20-uart";
+               reg = <0x70006000 0x40>;
+               reg-shift = <2>;
+               clock-frequency = <408000000>;

This isn't a property that's defined by the Tegra serial binding. This information should be obtained by looking up the relevant clock, and querying its rate.

Quoted 3 lines

We can't do that in the ns16550 driver as yet since there is no generic U-Boot clock infrastructure. I suspect that will come with time.

Quoted 14 lines

The solution here is to put the clock infra-structure in place first. One thing I've learned from the kernel DT experience is that a lot of time would
have been saved by putting the correct infra-structure in place first, then
using it, rather than hacking around things the wrong way, then putting the
infra-structure in place, then converting to it. That's a lot more work, and
rather painful. Equally, if we don't just do the infra-structure right, there's really little guarantee that we'll ever convert to the correct approach. Just look at all the DT content in use in U-Boot that don't match
the real DT bindings, even after it's been around years.

OK, is there a plan to put this in place? Who is working on it?

Quoted 3 lines

I don't know whether anyone is or not. However, the fact that nobody is working on a clock driver is no excuse for using the incorrect DT bindings in order to hack around its lack of existence.

Actually the bindings are correct, you are referring to a single new addition which is the input clock speed of the UART.

Quoted 4 lines

If you want to move the serial port information into DT, and part of doing so requires extracting clock-related information from DT, and that in turn requires a clock driver to do so, then yes, U-Boot needs to have that clock driver implemented before you can move the serial port information into DT.

Again, your objection is purely about the clock-frequency property I think. The binding is otherwise correct, right?

Quoted 2 lines

To be honest, I'm not sure that using DT is going to buy U-Boot much, or even the kernel in retrospect.

It's not that bad :-)

Quoted 1 line · Simon Glass

On 1 August 2014 15:50, Stephen Warren <[email protected]> wrote:

...

Quoted 11 lines

Quoted 6 lines

DT schemas/bindings MUST be identical between U-Boot, Linux, FreeBSD, Barebox, ... (all of which use DT). As such, all the DT bindings MUST be discussed on the devicetree mailing list.

Since you're the author of the patch, it's your responsibility to have that discussion.

Are you referring to the linux,stdout-path discussion, or something more DT-generic?I suppose we could have a 'u-boot,console' for our part. But in any case you are talking about code and a convention that is already in mainline U-Boot.

I'm saying that any and all additions or changes to DT schemas/bindings must be discussed on the devicetree mailing list, not made/reviewed in isolation on only the U-Boot mailing list.

Quoted 5 lines

While I accept that we might change to
something DT-generic if Linux points the way to something better, I don't want to stop using it just because Linux hasn't decided yet. The early console stuff and early debug UART stuff in Linux is not yet a shining example of perfection.

I strongly believe that if U-Boot continues to use DT, the current DT usage in U-Boot needs to be actively moved in line with the bindings that the Linux kernel, Barebox, FreeBSD, ... use. I'd prefer this to happen even before U-Boot starts making additional use of DT, so the conversion doesn't get forgotten. However, I suppose it's a bit draconian to prevent further usage until the existing usage is cleaned up, except where new usage introduces additional dependencies on any current usage that's inconsistent with the standard bindings.

Quoted 22 lines · Stephen Warren

Quoted 1 line · Simon Glass

On 1 August 2014 15:50, Stephen Warren <[email protected]> wrote:

...

Quoted 10 lines

Quoted 6 lines

DT schemas/bindings MUST be identical between U-Boot, Linux, FreeBSD, Barebox, ... (all of which use DT). As such, all the DT bindings MUST be discussed on the devicetree mailing list.

Since you're the author of the patch, it's your responsibility to have that discussion.

Are you referring to the linux,stdout-path discussion, or something more DT-generic?I suppose we could have a 'u-boot,console' for our part. But in any case you are talking about code and a convention that is already in mainline U-Boot.

I'm saying that any and all additions or changes to DT schemas/bindings must be discussed on the devicetree mailing list, not made/reviewed in isolation on only the U-Boot mailing list.

Quoted 5 lines

While I accept that we might change to
something DT-generic if Linux points the way to something better, I don't want to stop using it just because Linux hasn't decided yet. The early console stuff and early debug UART stuff in Linux is not yet a shining example of perfection.

Quoted 9 lines

I strongly believe that if U-Boot continues to use DT, the current DT usage in U-Boot needs to be actively moved in line with the bindings that the Linux kernel, Barebox, FreeBSD, ... use. I'd prefer this to happen even before U-Boot starts making additional use of DT, so the conversion doesn't get forgotten. However, I suppose it's a bit draconian to prevent further usage until the existing usage is cleaned up, except where new usage introduces additional dependencies on any current usage that's inconsistent with the standard bindings.

I don't disagree (and at ELC I was trying to see who, if anyone, had been reaching out to the FreeBSD folks since their DT files were very far from what Linux uses afaict for am33xx), but I'm also not seeing anything from non-Linux kernel folks on the devicetrees ML, or at least my quick search failed.

Quoted 2 lines · Simon Glass

On 1 August 2014 15:50, Stephen Warren <[email protected]> wrote:

...

Quoted 4 lines

Quoted 4 lines

DT schemas/bindings MUST be identical between U-Boot, Linux, FreeBSD, Barebox, ... (all of which use DT). As such, all the DT bindings MUST be discussed on the devicetree mailing list.

Quoted 8 lines

Quoted 2 lines

Since you're the author of the patch, it's your responsibility to have that discussion.

Are you referring to the linux,stdout-path discussion, or something more DT-generic?I suppose we could have a 'u-boot,console' for our part. But in any case you are talking about code and a convention that is already in mainline U-Boot.

I'm saying that any and all additions or changes to DT schemas/bindings must be discussed on the devicetree mailing list, not made/reviewed in isolation on only the U-Boot mailing list.

Quoted 6 lines

While I accept that we might change to

something DT-generic if Linux points the way to something better, I don't want to stop using it just because Linux hasn't decided yet. The early console stuff and early debug UART stuff in Linux is not yet a shining example of perfection.

I strongly believe that if U-Boot continues to use DT, the current DT usage in U-Boot needs to be actively moved in line with the bindings that the Linux kernel, Barebox, FreeBSD, ... use. I'd prefer this to happen even before U-Boot starts making additional use of DT, so the conversion doesn't get forgotten. However, I suppose it's a bit draconian to prevent further usage until the existing usage is cleaned up, except where new usage introduces additional dependencies on any current usage that's inconsistent with the standard bindings.

Quoted 1 line

That said, it's a good time to adopt 'u-boot,console' if that's what we need?

It's certainly a good time to start that discussion on the devicetree mailing list, and get such a new property reviewed/ack'd there.

Hi Stephen,

Quoted 32 lines · Stephen Warren

Quoted 2 lines · Simon Glass

On 1 August 2014 15:50, Stephen Warren <[email protected]> wrote:

...

Quoted 4 lines

Quoted 4 lines

DT schemas/bindings MUST be identical between U-Boot, Linux, FreeBSD, Barebox, ... (all of which use DT). As such, all the DT bindings MUST be discussed on the devicetree mailing list.

Quoted 8 lines

Quoted 2 lines

Since you're the author of the patch, it's your responsibility to have that discussion.

Are you referring to the linux,stdout-path discussion, or something more DT-generic?I suppose we could have a 'u-boot,console' for our part. But in any case you are talking about code and a convention that is already in mainline U-Boot.

I'm saying that any and all additions or changes to DT schemas/bindings must be discussed on the devicetree mailing list, not made/reviewed in isolation on only the U-Boot mailing list.

Quoted 6 lines

While I accept that we might change to

something DT-generic if Linux points the way to something better, I don't want to stop using it just because Linux hasn't decided yet. The early console stuff and early debug UART stuff in Linux is not yet a shining example of perfection.

Quoted 7 lines

I strongly believe that if U-Boot continues to use DT, the current DT usage in U-Boot needs to be actively moved in line with the bindings that the Linux kernel, Barebox, FreeBSD, ... use. I'd prefer this to happen even before U-Boot starts making additional use of DT, so the conversion doesn't get forgotten. However, I suppose it's a bit draconian to prevent further usage until the existing usage is cleaned up, except where new usage introduces additional dependencies on any current usage that's inconsistent with the standard bindings.

Quoted 1 line

That said, it's a good time to adopt 'u-boot,console' if that's what we need?

It's certainly a good time to start that discussion on the devicetree mailing list, and get such a new property reviewed/ack'd there.

[side note: You will be aware that I have expended considerable effort getting agreement on bindings. I used to copy all DT patches to that mailing list, but I can't recall getting a reply that often. Also note that U-Boot's use of DT pre-dated the kernel with many subsystems (e.g. the request to retrofit clock bindings after the code was already written). Yes, DT bindings should be common across all platforms, but where subsystems don't exist in U-Boot I feel the approach of 'do nothing until someone writes a new subsystem' might just be a recipe for inaction/no progress. Better to iterate towards perfection than never move]

So to be clear, with the clock-frequency property moved back to being hard-coded in the CONFIG, your remaining objection is that the console alias, which is already used in U-Boot, should be agreed with the devicetree mailing list? Is that right?

Use driver model for serial ports (for test and comment only).

Quoted 2 lines · Simon Glass

Use driver model for serial ports (for test and comment only).

A couple of nits:

include/configs/tegra-common.h line 48

Quoted 6 lines

include/configs/tegra-common.h +1 -0

 
 
  #define CONFIG_SYS_MALLOC_LEN         (4 << 20)       /* 4MB  */
+#define CONFIG_SYS_MALLOC_F_LEN        (1 << 10)

README says:

Pre-relocation malloc() is only supported on sandbox at present but is fairly easy to enable for other archs.

I assume that should be deleted now.

include/configs/tegra-common.h line 58

Quoted 13 lines

/*
* NS16550 Configuration
*/

include/configs/tegra-common.h +3 -1

-#define CONFIG_SYS_NS16550
+#ifdef CONFIG_SPL_BUILD
  #define CONFIG_SYS_NS16550_SERIAL
  #define CONFIG_SYS_NS16550_REG_SIZE   (-4)
  #define CONFIG_SYS_NS16550_CLK                V_NS16550_CLK
+#endif
+#define CONFIG_SYS_NS16550

Leaving that define in the same place would simplify the diff.

Since Tegra now uses driver model for serial, adjust the definition of V_NS16550_CLK so that it is clear that this is only used for SPL.

Quoted 3 lines · Simon Glass

Since Tegra now uses driver model for serial, adjust the definition of V_NS16550_CLK so that it is clear that this is only used for SPL.

May as well squash this into the previous patch.