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
