Re: [PATCH V2 1/2] dt-bindings: serial: add Broadcom's BCMBCA family High Speed UART

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 22/11/2023 16:32, Rafał Miłecki wrote:
> On 22.11.2023 16:00, Krzysztof Kozlowski wrote:
>> On 22/11/2023 15:52, Rafał Miłecki wrote:
>>>>> +maintainers:
>>>>> +  - Rafał Miłecki <rafal@milecki.pl>
>>>>> +
>>>>> +allOf:
>>>>> +  - $ref: serial.yaml#
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    items:
>>>>> +      - enum:
>>>>> +          - brcm,bcm4908-hs-uart
>>>>> +          - brcm,bcm4912-hs-uart
>>>>> +          - brcm,bcm6756-hs-uart
>>>>> +          - brcm,bcm6813-hs-uart
>>>>> +          - brcm,bcm6846-hs-uart
>>>>> +          - brcm,bcm6855-hs-uart
>>>>> +          - brcm,bcm6856-hs-uart
>>>>> +          - brcm,bcm6858-hs-uart
>>>>> +          - brcm,bcm6878-hs-uart
>>>>> +          - brcm,bcm47622-hs-uart
>>>>> +          - brcm,bcm63138-hs-uart
>>>>> +          - brcm,bcm63146-hs-uart
>>>>> +          - brcm,bcm63158-hs-uart
>>>>> +          - brcm,bcm63178-hs-uart
>>>>> +      - const: brcm,bcmbca-hs-uart
>>>>
>>>> git grep did not find driver for this compatible. Is it in separate
>>>> patchset?
>>>
>>> No. My project based on BCMBCA has been canceled and I don't work on it
>>> full time anymore. I just wanted to fill empty bits I can afford
>>> handling in my free time and complete hardware description in DTS.
>>>
>>> I may still work on some BCMBCA drivers from time to time but as a side
>>> project.
>>
>> This means we cannot use driver to verify whether the fallback is
>> actually suitable. Considering that existing UART bindings do not
>> fallback (brcm,bcm6345-uart, brcm,bcm7271-uart), I don't understand what
>> is the benefit here.
> 
> I believed the rule for maintaining bindings and DTS files was to
> describe hardware no matter what/if system needs it.
> 
> For example a year ago I added binding for BCMBCA SoC timer without
> actual driver, see e112f2de151b ("dt-bindings: timer: Add Broadcom's
> BCMBCA timers").
> 
> I'm not sure if we're going to agree on this, but personally I like
> describing hardware as much as I can. So it's well documented /
> understood and people may eventually write drivers for it. Maybe it's
> partially because I come from Broadcom's world that isn't well known
> for upstream efforts in general.

The problem is that "brcm,bcmbca-hs-uart" is not describing hardware. It
is saying that all these devices have similar (compatible) programming
model, so the OS can use just one compatible. This goes away from pure
hardware description into interpretation.

Rob already commented on such non-SoC compatibles multiple times. I do
not see any reason here to not use specific compatible as fallback.

> 
> As for verifying this binding against actual driver I can definitely
> understand your concerns. Hoping it may help I uploaded Broadcom's HS
> UART driver extracted from the RAXE500-V1.0.8.70_2.0.36_gpl SDK/GPL
> package: http://files.zajec.net/hs_uart/
> 
> Please note it's not much of a clean code and its design would not be
> accepted upstream but hopefully you can glance at it to verify this
> binding's compatibility.
> 
> Let me know if there is anything else (other than rewriting Broadcom's
> downstream driver) I could do to get this binding accepted.



Best regards,
Krzysztof