Re: [PATCH v1 3/6] ARM: dts: lan966x: add all flexcom usart nodes

[<Claudiu.Beznea@microchip.com>]

On 04.03.2022 13:01, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> Hi,
> 
> thanks for the quick review.
> 
> Am 2022-03-04 09:30, schrieb Claudiu.Beznea@microchip.com:
>> On 03.03.2022 18:03, Michael Walle wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>>> the content is safe
>>>
>>> Add all the usart nodes for the flexcom block. There was already
>>> an usart node for the flexcom3 block. But it missed the DMA
>>> channels.
>>
>> And it would be good to go though a different patch.
> 
> sure
> 
>>> Although the DMA channels are specified, DMA is not
>>> enabled by default because break detection doesn't work with DMA.
>>>
>>> Keep the nodes disabled by default.
>>>
>>> Signed-off-by: Michael Walle <michael@walle.cc>
>>> ---
>>>  arch/arm/boot/dts/lan966x.dtsi | 55
>>> ++++++++++++++++++++++++++++++++++
>>>  1 file changed, 55 insertions(+)
>>>
>>> diff --git a/arch/arm/boot/dts/lan966x.dtsi
>>> b/arch/arm/boot/dts/lan966x.dtsi
>>> index a7d46a2ca058..bea69b6d2749 100644
>>> --- a/arch/arm/boot/dts/lan966x.dtsi
>>> +++ b/arch/arm/boot/dts/lan966x.dtsi
>>> @@ -92,6 +92,19 @@ flx0: flexcom@e0040000 {
>>>                         #size-cells = <1>;
>>>                         ranges = <0x0 0xe0040000 0x800>;
>>>                         status = "disabled";
>>> +
>>> +                       usart0: serial@200 {
>>> +                               compatible =
>>> "atmel,at91sam9260-usart";
>>
>> Are the usart blocks in lan966x 1:1 compatible with what is is sam9260?
>> In
>> case not it may worth to have a new compatible here, for lan966x, such
>> that
>> when new features will be implemented in usart driver for lan966x the
>> old
>> DT (this one) will work with the new kernel implementation.
> 
> During my review of the inital dtsi patch, I've asked the same question
> [1]
> and I was told they are the same.
> 
> At least this exact usart compatible is already in this file. I was
> under
> the impression, that was the least controversial compatible :)

OK.

> 
> But you'll need to tell me if they are the same or not, I don't have
> any clue what microchip has reused. 

From software point of view comparing registers should be good, as far as I
can tell. All AT91 datasheet should be available. I though you have checked
one against LAN966. At the moment I don't have a DS for LAN966. I'll find
one and have a look.

> The only thing I can add is that
> there is a version register within the IP blocks which is already used
> in some drivers.
> 
>> Same for the rest of the nodes added in this series.
>>
>>> +                               reg = <0x200 0x200>;
>>> +                               interrupts = <GIC_SPI 48
>>> IRQ_TYPE_LEVEL_HIGH>;
>>> +                               dmas = <&dma0 AT91_XDMAC_DT_PERID(3)>,
>>> +                                          <&dma0
>>> AT91_XDMAC_DT_PERID(2)>;
>>
>> Keep dma entries aligned.
> 
> Oh shoot. How embarrassing they are alligned when the tab width is 4 *g*
> Thanks.
> 
> -michael
> 
> [1]
> https://lore.kernel.org/lkml/20220210123704.477826-1-michael@walle.cc/
> 
>>> +                               dma-names = "tx", "rx";
>>> +                               clocks = <&nic_clk>;
>>> +                               clock-names = "usart";
>>> +                               atmel,fifo-size = <32>;
>>> +                               status = "disabled";
>>> +                       };
>>>                 };
>>>
>>>                 flx1: flexcom@e0044000 {
>>> @@ -102,6 +115,19 @@ flx1: flexcom@e0044000 {
>>>                         #size-cells = <1>;
>>>                         ranges = <0x0 0xe0044000 0x800>;
>>>                         status = "disabled";
>>> +
>>> +                       usart1: serial@200 {
>>> +                               compatible =
>>> "atmel,at91sam9260-usart";
>>> +                               reg = <0x200 0x200>;
>>> +                               interrupts = <GIC_SPI 49
>>> IRQ_TYPE_LEVEL_HIGH>;
>>> +                               dmas = <&dma0 AT91_XDMAC_DT_PERID(5)>,
>>> +                                          <&dma0
>>> AT91_XDMAC_DT_PERID(4)>;
>>> +                               dma-names = "tx", "rx";
>>> +                               clocks = <&nic_clk>;
>>> +                               clock-names = "usart";
>>> +                               atmel,fifo-size = <32>;
>>> +                               status = "disabled";
>>> +                       };
>>>                 };
>>>
>>>                 trng: rng@e0048000 {
>>> @@ -129,6 +155,19 @@ flx2: flexcom@e0060000 {
>>>                         #size-cells = <1>;
>>>                         ranges = <0x0 0xe0060000 0x800>;
>>>                         status = "disabled";
>>> +
>>> +                       usart2: serial@200 {
>>> +                               compatible =
>>> "atmel,at91sam9260-usart";
>>> +                               reg = <0x200 0x200>;
>>> +                               interrupts = <GIC_SPI 50
>>> IRQ_TYPE_LEVEL_HIGH>;
>>> +                               dmas = <&dma0 AT91_XDMAC_DT_PERID(7)>,
>>> +                                          <&dma0
>>> AT91_XDMAC_DT_PERID(6)>;
>>> +                               dma-names = "tx", "rx";
>>> +                               clocks = <&nic_clk>;
>>> +                               clock-names = "usart";
>>> +                               atmel,fifo-size = <32>;
>>> +                               status = "disabled";
>>> +                       };
>>>                 };
>>>
>>>                 flx3: flexcom@e0064000 {
>>> @@ -144,6 +183,9 @@ usart3: serial@200 {
>>>                                 compatible =
>>> "atmel,at91sam9260-usart";
>>>                                 reg = <0x200 0x200>;
>>>                                 interrupts = <GIC_SPI 51
>>> IRQ_TYPE_LEVEL_HIGH>;
>>> +                               dmas = <&dma0 AT91_XDMAC_DT_PERID(9)>,
>>> +                                          <&dma0
>>> AT91_XDMAC_DT_PERID(8)>;
>>> +                               dma-names = "tx", "rx";
>>>                                 clocks = <&nic_clk>;
>>>                                 clock-names = "usart";
>>>                                 atmel,fifo-size = <32>;
>>> @@ -178,6 +220,19 @@ flx4: flexcom@e0070000 {
>>>                         #size-cells = <1>;
>>>                         ranges = <0x0 0xe0070000 0x800>;
>>>                         status = "disabled";
>>> +
>>> +                       usart4: serial@200 {
>>> +                               compatible =
>>> "atmel,at91sam9260-usart";
>>> +                               reg = <0x200 0x200>;
>>> +                               interrupts = <GIC_SPI 52
>>> IRQ_TYPE_LEVEL_HIGH>;
>>> +                               dmas = <&dma0
>>> AT91_XDMAC_DT_PERID(11)>,
>>> +                                          <&dma0
>>> AT91_XDMAC_DT_PERID(10)>;
>>> +                               dma-names = "tx", "rx";
>>> +                               clocks = <&nic_clk>;
>>> +                               clock-names = "usart";
>>> +                               atmel,fifo-size = <32>;
>>> +                               status = "disabled";
>>> +                       };
>>>                 };
>>>
>>>                 timer0: timer@e008c000 {
>>> -- 
>>> 2.30.2
>>>
> 
> -- 
> -michael