Hi Linus, On Fri, 2023-06-16 at 14:48 +0200, Linus Walleij wrote: > Hi Sarah, > > thanks for your patch! > > On Tue, Jun 13, 2023 at 5:20 PM Sarah Walker <sarah.walker@xxxxxxxxxx> wrote: > > > Add the device tree binding documentation for the Series AXE GPU used in > > TI AM62 SoCs. > > > > Signed-off-by: Sarah Walker <sarah.walker@xxxxxxxxxx> > > +properties: > > + compatible: > > + oneOf: > > + - items: > > + - enum: > > + - ti,am62-gpu > > + - const: img,powervr-seriesaxe > > I don't see why you need to restrict the bindings to just the stuff > you happen to > be writing Linux drivers for right now. > > Add all of them! The main thinking here was to start off with a single simple case (TI AM62) to support the initial driver upstreaming rather than trying to cover too many things at once. This can then be built upon for other GPUs. So, for example, we can extend the bindings to add a second power domain for those that need it, such as the GPU in the Mediatek MT8173. The benefit of this approach is that the bindings, dts and code changes can all be reviewed together so that all the context is present. > > There is this out-of-tree binding: > https://github.com/openpvrsgx-devgroup/linux_openpvrsgx/commit/0ddd308a78926782b8a72f75c74b91417ceb9779 > > With these two on top: > https://github.com/openpvrsgx-devgroup/linux_openpvrsgx/commit/053346e1933932815066f16ebf6e6bda45d67548 > https://github.com/openpvrsgx-devgroup/linux_openpvrsgx/commit/1cb62c4cdcad096d438ee7d1d51f6001998acee3 > > They are indeed out-of-tree, but H. Nikolaus is a well-known and respected > contributor to the kernel, and I'm pretty sure he would be contributing > these upstream if he had the time and incentive. > > To me it seems much more like you should talk to Nikolaus about submitting > these patches initially, and then add Rogue support with patches on top of it. > It could be a nice series with just DT bindings. > > I see that your binding uses a power domain rather than a regulator > (sgx-supply) for power and that is probably a better approach but other > than that the openpvrsgx binding looks more complete and to the point? > > It will also help them to get these bindings settled so they can merge all > of the DTS patches adding the SGX block to misc platforms in the > kernel upstream so they can focus their work on the actual driver. > > When I look at the PowerVR wikipedia page: > https://en.wikipedia.org/wiki/PowerVR > there is no correspondence between the product names there and > "img,powervr-seriesaxe" as used here. > > I think you need to explain if these are internal product names or what > is going on, and what the relationship is to the marketing names, it could > be part of the description simply, so that people know what string to use > somewhat intuitively. Maybe all the strings in the out-of-tree documentation > are just wrong because internal code names need to be used? It's been quite some time since we originally wrote these bindings, so we'll need to go back and check why we settled on "powervr-seriesaxe", but I suspect it was just a case of us normalising the naming across different series. Thanks Frank > > Yours, > Linus Walleij
