From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> To: "Yu, Richard" <richard.yu@hpe.com>, "Verdun, Jean-Marie" <verdun@hpe.com>, "Hawkins, Nick" <nick.hawkins@hpe.com>, "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>, "robh+dt@kernel.org" <robh+dt@kernel.org>, "krzysztof.kozlowski+dt@linaro.org" <krzysztof.kozlowski+dt@linaro.org>, "linux@armlinux.org.uk" <linux@armlinux.org.uk>, "balbi@kernel.org" <balbi@kernel.org>, "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>, "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org> Subject: Re: [PATCH v1 2/7] dt-bindings: usb: hpe,gxp-udc: Add binding for gxp gadget Date: Tue, 8 Nov 2022 12:30:39 +0100 [thread overview] Message-ID: <c199600a-aad9-5639-ea57-a4d59d719ade@linaro.org> (raw) In-Reply-To: <SJ0PR84MB2085E6B922DAF1070DC802EF8D3C9@SJ0PR84MB2085.NAMPRD84.PROD.OUTLOOK.COM> On 07/11/2022 21:16, Yu, Richard wrote: > Hi Mr. Kozlowski, > >>> + >>> + 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. Then how do you specify two devices? You allow here only one, right? 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... I might miss here something but so far it looks to me like some hacky description matching the driver, not hardware, not existing bindings. > >>> + >>> + 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. Best regards, Krzysztof
WARNING: multiple messages have this Message-ID (diff)
From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> To: "Yu, Richard" <richard.yu@hpe.com>, "Verdun, Jean-Marie" <verdun@hpe.com>, "Hawkins, Nick" <nick.hawkins@hpe.com>, "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>, "robh+dt@kernel.org" <robh+dt@kernel.org>, "krzysztof.kozlowski+dt@linaro.org" <krzysztof.kozlowski+dt@linaro.org>, "linux@armlinux.org.uk" <linux@armlinux.org.uk>, "balbi@kernel.org" <balbi@kernel.org>, "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>, "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org> Subject: Re: [PATCH v1 2/7] dt-bindings: usb: hpe,gxp-udc: Add binding for gxp gadget Date: Tue, 8 Nov 2022 12:30:39 +0100 [thread overview] Message-ID: <c199600a-aad9-5639-ea57-a4d59d719ade@linaro.org> (raw) In-Reply-To: <SJ0PR84MB2085E6B922DAF1070DC802EF8D3C9@SJ0PR84MB2085.NAMPRD84.PROD.OUTLOOK.COM> On 07/11/2022 21:16, Yu, Richard wrote: > Hi Mr. Kozlowski, > >>> + >>> + 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. Then how do you specify two devices? You allow here only one, right? 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... I might miss here something but so far it looks to me like some hacky description matching the driver, not hardware, not existing bindings. > >>> + >>> + 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. Best regards, Krzysztof _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2022-11-08 11:30 UTC|newest] Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-11-03 16:06 [PATCH v1 0/7] Add USB Driver for HPE GXP Architecture richard.yu 2022-11-03 16:06 ` richard.yu 2022-11-03 16:06 ` [PATCH v1 1/7] usb: gadget: udc: gxp_udc: add gxp USB support richard.yu 2022-11-03 16:06 ` richard.yu 2022-11-03 16:06 ` [PATCH v1 2/7] dt-bindings: usb: hpe,gxp-udc: Add binding for gxp gadget richard.yu 2022-11-03 16:06 ` richard.yu 2022-11-03 16:29 ` Krzysztof Kozlowski 2022-11-03 16:29 ` Krzysztof Kozlowski 2022-11-04 20:03 ` Yu, Richard 2022-11-04 20:03 ` Yu, Richard 2022-11-04 20:09 ` Krzysztof Kozlowski 2022-11-04 20:09 ` Krzysztof Kozlowski 2022-11-07 20:16 ` Yu, Richard 2022-11-07 20:16 ` Yu, Richard 2022-11-08 11:30 ` Krzysztof Kozlowski [this message] 2022-11-08 11:30 ` Krzysztof Kozlowski 2022-11-09 3:37 ` Yu, Richard 2022-11-09 3:37 ` Yu, Richard 2022-11-11 8:20 ` Krzysztof Kozlowski 2022-11-11 8:20 ` Krzysztof Kozlowski 2022-11-03 16:06 ` [PATCH v1 3/7] dt-bindings: usb: hpe,gxp-udcg: Add binding for gxp gadget group richard.yu 2022-11-03 16:06 ` richard.yu 2022-11-03 16:30 ` Krzysztof Kozlowski 2022-11-03 16:30 ` Krzysztof Kozlowski 2022-11-03 16:06 ` [PATCH v1 4/7] dt-bindings: usb: hpe,gxp-vuhc: add binding for gxp vEHCI richard.yu 2022-11-03 16:06 ` richard.yu 2022-11-03 16:32 ` Krzysztof Kozlowski 2022-11-03 16:32 ` Krzysztof Kozlowski 2022-11-03 16:06 ` [PATCH v1 5/7] ARM: dts: hpe: Add UDC nodes richard.yu 2022-11-03 16:06 ` richard.yu 2022-11-08 11:28 ` Krzysztof Kozlowski 2022-11-08 11:28 ` Krzysztof Kozlowski 2022-11-03 16:06 ` [PATCH v1 6/7] ARM: configs: multi_v7_defconfig: Enable HPE GXP USB Driver richard.yu 2022-11-03 16:06 ` richard.yu 2022-11-03 16:32 ` Krzysztof Kozlowski 2022-11-03 16:32 ` Krzysztof Kozlowski 2022-11-03 16:06 ` [PATCH v1 7/7] MAINTAINERS: add USB support to GXP richard.yu 2022-11-03 16:06 ` richard.yu 2022-11-03 16:21 ` Krzysztof Kozlowski 2022-11-03 16:21 ` Krzysztof Kozlowski 2022-11-04 14:05 ` kernel test robot 2022-11-04 14:05 ` kernel test robot
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=c199600a-aad9-5639-ea57-a4d59d719ade@linaro.org \ --to=krzysztof.kozlowski@linaro.org \ --cc=balbi@kernel.org \ --cc=devicetree@vger.kernel.org \ --cc=gregkh@linuxfoundation.org \ --cc=krzysztof.kozlowski+dt@linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-usb@vger.kernel.org \ --cc=linux@armlinux.org.uk \ --cc=nick.hawkins@hpe.com \ --cc=richard.yu@hpe.com \ --cc=robh+dt@kernel.org \ --cc=verdun@hpe.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.