Re: [PATCH v1 2/7] dt-bindings: usb: hpe,gxp-udc: Add binding for gxp gadget

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

On 09/11/2022 04:37, Yu, Richard wrote:
> Hi Mr. Kozlowski,
> 
> Thank you very much for inputs.
> 
>>>>> +
>>>>> +  vdevnum:
>>>>> +    description:
>>>>> +      virtual device number.
>>
>>>> That's unusual property... Why numbering devices is part of DT (hardware description)?
>>
>>>  In HPE GXP virtual EHCI controller chipset, it can support up to 8  
>>> virtual devices(gadgets). Each device/gadget will be represented  by 
>>> a bit in 8 bits register. For example, the interrupt register bit 0  
>>> indicates the interrupt from device 0, bit 1 for device 1 ... so on.
>>> When a user defines a device/gadget, he/she can define the device 
>>> number as between 0 and 7. Thus, the driver can look up to the bit 
>>> position. That is why we have numbering devices as part of DT.
> 
>> Wrap your lines properly, it's impossible to reply in-line to such messages.
> 
> Sorry for the improper wrapping. Hope the above fixed the problem.
> 
>> Then how do you specify two devices? You allow here only one, right?
> 
> In our current design, to specify two devices, we added the gadget 
> structure into the device tree, such as  gadget0:udc@80401000{}; gadget1:udc@80402000{};....
> 
> No, we can allow up to 8 devices by adding the gadget structure,
> such as gadget0:udc@80401000{}; gadget1:udc@80402000{};....gadget8:udc@80408000{};
> 
>> Which bit in which register? Your devices have separate address space, so why they cannot poke the same register, right? Then just always set it to 0...
> 
> In HPE GXP vEHCI controller, there are three register groups: standard USB EHCI registers, 
> virtual device global registers, and virtual device registers.
> 
> Standard USB EHCI registers ---- We defined as "hpe,gxp-vudc" in the device tree (vuhc0) 
> Virtual device global registers --- We defined as "hpe,gxp-udcg" 
> Virtual device registers -- We defined as "hpe,gxp-udc"
> 
> Each virtual device will have its own separate address space. 
> There is only single address space for the virtual device global registers. 
> 
> The virtual device global registers are including vDevice Global Interrupt Status register(EVGISTAT), 
> vDevice Global Interrupt Enable register(EVGIEN), vEHCI FlexEndpoint Mapping register (EVFEMAP) ....
> We need the vdevnum for the bit position in EVGISTAT and EVGIEN for each device.  
> We write vdevnum into the EVFEMAP register to assign an EP to a specific device. 
> 
>> I might miss here something but so far it looks to me like some hacky description matching the driver, not hardware, not existing bindings.
> 
> We create "vdevnum" as device configuration parameter due to our hardware need.

That's not an argument... everything can be a "hardware need".

> 
>>>>> +
>>>>> +  fepnum:
>>>>> +    description:
>>>>> +      number of the flexible end-points this device is needed.
>>>
>>>> Similar question.
>>>
>>> In HPE GXP virtual EHCI Controller chipset, there is a flexible End-Point(EP) pool. 
>>> Each flexible EP has its own mapping register. The mapping register 
>>> bit 0 to 3 is for device number (vdevnum) and bit 4 to 7 is for EP number inside the device.
>>> The device driver configures the mapping register to assign a flexible 
>>> EP to a specific device.  Here, "fepnum" is the input letting the 
>>> driver know how many EPs are needed for this device/gadget.
> 
>> Nope. So you create here some weird IDs to poke into syscon register.
>> First, syscon has offset if you need. You could treat it maybe as bits?
>> I don't know... but even then your design is poor - two devices 
>> changing the same register. Even though it is sunchronized by regmap, it is conflicting, obfuscated access.
> 
> The "fepnum" is the input parameter to define how many end-points (EPs) is needed
> for the device.
> 
> You are correct that all devices need to access the virtual 
> device global registers during the runtime. 
> Thus, we create " hpe,syscon-phandle = <&udc_system_controller>;'
> for the driver getting the vDevice Global registers address.

And how do you solve poking into the same register by two devices? Who
owns it? You don't...

> 
> In our current chip registers layout with the vDevice Global registers, I don’t see
> a way to avoid "two devices changing the same register".

I see at least an idea - create proper hierarchy, where parent device
instantiates its children (thus knows and increments the IDs) and is
responsible for proper handling of shared register (thus the parent owns
the register).

I understand why you created vdevnum/fepnum properties but the reason is
not matching DT bindings. These are not additional hardware properties
which deserve their own DT properties - they are already part of unit
address and/or just incremented ID based on device number managed by a
parent.

Best regards,
Krzysztof