From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751704AbcBZJA5 (ORCPT ); Fri, 26 Feb 2016 04:00:57 -0500 Received: from mailgw02.mediatek.com ([210.61.82.184]:51214 "EHLO mailgw02.mediatek.com" rhost-flags-OK-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S1751540AbcBZJAu (ORCPT ); Fri, 26 Feb 2016 04:00:50 -0500 Message-ID: <1456477230.17841.2.camel@mtksdaap41> Subject: Re: [RFC 3/3] CMDQ: Mediatek CMDQ driver From: Horng-Shyang Liao To: Daniel Kurtz CC: Sascha Hauer , "open list:OPEN FIRMWARE AND..." , "linux-kernel@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "moderated list:ARM/Mediatek SoC support" , srv_heupstream , Sascha Hauer , Philipp Zabel , Nicolas Boichat , CK HU , cawa cheng , Bibby Hsieh , YT Shen , Daoyuan Huang , "Damon Chu" , Josh-YC Liu , "Glory Hung" , Yong Wu , "Matthias Brugger" , Rob Herring Date: Fri, 26 Feb 2016 17:00:30 +0800 In-Reply-To: References: <8A0FDEEF9DBBD140A3857422D81DA20F867B19FD@mtkmbs01n1> <1454479333.11967.12.camel@mtksdaap41> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.2.3-0ubuntu6 Content-Transfer-Encoding: 7bit MIME-Version: 1.0 X-MTK: N Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 2016-02-03 at 14:40 +0800, Daniel Kurtz wrote: > > Hi Dan, > > > > Thanks for your comment. > > This solution looks good to me. > > I will change it as your suggestion. > > > > But, I have a question about 'mask out the provided *device virtual* > > address'. > > Are lower 16-bits (or 24-bits for JUMP op) of device virtual address the > > same as device physical address? > > I'm not sure. But I doubt it we can rely on this. > My guess would be that the ioremap only preserves the lower 12 bits > (4k page size). > > > If not, we still need to pass in physical address into CMDQ driver. > > Or, instead of the iommu/slot approach, we can just provide a > registration function for the gce driver. > Each gce consumer could then have a simple gce node, with no slot/address: > > mediatek,gce = <&gce>; > > Then on probe, the gce consumer could pass in its (struct device *) to > gce_register_device(). gce_register_device() could then access the > device's of_node to extract its physical address range, and look up > its physical address in its table of per-soc of > "device_address:gce_subsys_address" entries. If the physical address > is in a valid subsys ranges, the gce_register_device would cache the > subsys address, and an offset in a (struct gce_consumer). > gce_register_device() could then add this struct to a struct list_head > of gce_consumers, and finally return a pointer to it back to the > caller. > > Later, the gce consumer could pass in ths (struct gce_consumer *) when > make gce calls, along with the *offset* (not the physical address or > virtual address) for the register that it wishes to access. Then the > gce driver can simply use the gce_consumer->subsys entry to create a > gce address from the passed in offset. > > This will keep the binding very simple, and would remove the need to > convert from device virtual to physical addresses by the gce consumer, > but require a little more per-gce-consumer setup. > > -Dan Hi Dan, When I try to implement this comment, I realize the only benefit from this comment is to wrap physical address. Recall from my previous reply: gce address = subsys + valid low bits. So, CMDQ driver still need to do "(Base + offset) & valid mask" to get gce valid low bits. Current implementation let display driver do "base + offset". This comment just transfers this calculation from display driver to CMDQ driver. However, this comment will let CMDQ interface (behavior) become more complicated, e.g. gce_register_device(), struct gce_consumer, and int cmdq_rec_write(struct cmdq_rec *handle, u32 value, struct gce_consumer *consumer, u32 offset) Do you think it is worth to do this effort to wrap physical address? Thanks, HS Liao