Quoting Pali Rohár (2022-01-19 16:06:51)

On Wednesday 19 January 2022 15:16:54 Stephen Boyd wrote:

quoted

Quoting Pali Rohár (2022-01-15 04:26:18)

quoted

On Saturday 15 January 2022 13:05:09 Marek Behún wrote:

quoted

On Sat, 15 Jan 2022 12:50:18 +0100
Pali Rohár [off-list ref] wrote:

quoted

On Saturday 15 January 2022 00:02:11 Stephen Boyd wrote:

quoted

Quoting Pali Rohár (2021-10-15 23:42:10)  

quoted

If I was designing this driver and DTS bindings I would have choose
something like this:

uart@0x12000 {  

Drop the 0x
  

quoted

    reg = <0x12000 0x18>, <0x12200 0x30>;
    clock-controller {
        ...
    };  

Drop this node and put whatever properties are inside into the parent
node.
  

quoted

    serial1 {
        ...
        status = "disabled";
    };
    serial2 {
        ...
        status = "disabled";
    };
};

Meaning that 0x12000 node would be 3 subnodes and all registers would be
defined in top level nodes and would be handled by one driver.

This is really how hardware block looks like. But it is not backward
compatible...  

Sounds good to me. I presume we need the serial child nodes so we can
reference them from the stdout-path?  

Yes, exactly, separate nodes for serial1 and serial2 are still required.

But dropping clock controller is not possible as for higher baudrates we
need to use and configure uart clock controller. Without it we just get
comparable feature support which is already present in driver.

What Stephen means is making clock controller out of the uart node
directly. No need to add separate subnode just for clock controller.

This is already implemented in v7 patch series. Clock controller is
already outside of uart nodes.

I mean to combine the uart node and the clock-controller node together

      uart-wrapper {
              reg = <0x12000 0x18>, <0x12200 0x30>;
              #clock-cells ...

              serial1 {
                      ...
              };

              serial2 {
                      ...
              };
      };

Ok, now I see what you mean.

But problem is that this is not backward compatible change. And would
not work per existing DT bindings definitions, which defines how
bootloader should set configured clocks.

As I wrote in emails 3 months ago, this new "proposed" DTS definition is
something which I would have chosen if I had designed this driver and
bindings in past. But that did not happen and different approach is
already widely in used.

To support existing DTS definitions and bootloaders, it is really
required to have current structure backward compatible like it is
defined in current DT bindings document. And my changes in this patch
series are backward compatible.

I'm lost. Is the bootloader the one that's expecting some particular
serial node format and updating something? What is the bootloader doing?

To change DTS structure, it would be needed to provide uart nodes in DTS
files two times: once in old style (the current one) and second time in
this new style.

That's not a good idea. Why do we need to support both at the same time?

But such thing would even more complicate updating driver and it needs
to be implemented.

Plus this would open a question how to define default stdout-path if
there would be 4 serial nodes, where one pair would describe old style
and second pair new style; meaning that 2 cross nodes would describe
same define.

Huh? We shouldn't have both bindings present in the DTB.

For me this looks like a more complications and I do not see any benefit
from it.

It is really important to break backward compatibility, just to try
having new cleaner API at the cost of having more complications and
requirement for more development and also important maintenance?

It's important to not make DT nodes have reg properties that overlap.
Maybe this is a DT purist viewpoint and I'm totally off base! I think
Rob did ack this binding already so I must be coming from the wrong
angle.

Nothing prevents register overlap from happening in practice, but it's
good to avoid such a situation as it clearly divides the I/O space by
assigning an address range to a particular device. In this case, we see
the two uarts are really one device, but we need two nodes in DT for
stdout-path, so we make some child nodes and have the driver figure out
which serial port to use for the console.

We shouldn't be adding more nodes to DT to get drivers to probe for
device I/O spaces that have already been described in DT. When this
happens, we learn that some I/O range is actually a combination of
functions, like uart and clks, and thus we should be able to add any
required properties to the existing DT node to support that new feature
that wasn't described before in the binding.