Messages in this thread | | | From | Ryan Chen <> | Subject | RE: [PATCH 0/4] Remove LPC register partitioning | Date | Mon, 28 Sep 2020 07:43:25 +0000 |
| |
Hello Joel & Andrew, Those patches are more organize for ASPEED SOC LPC register layout. Does those patches have any feedback?
Ryan
> -----Original Message----- > From: ChiaWei Wang <chiawei_wang@aspeedtech.com> > Sent: Friday, September 11, 2020 4:21 PM > To: Andrew Jeffery <andrew@aj.id.au>; Joel Stanley <joel@jms.id.au> > Cc: Rob Herring <robh+dt@kernel.org>; Corey Minyard <minyard@acm.org>; > Linus Walleij <linus.walleij@linaro.org>; Haiyue Wang > <haiyue.wang@linux.intel.com>; Cyril Bur <cyrilbur@gmail.com>; Robert > Lippert <rlippert@google.com>; Linux ARM > <linux-arm-kernel@lists.infradead.org>; linux-aspeed > <linux-aspeed@lists.ozlabs.org>; Linux Kernel Mailing List > <linux-kernel@vger.kernel.org>; OpenBMC Maillist > <openbmc@lists.ozlabs.org>; Ryan Chen <ryan_chen@aspeedtech.com> > Subject: RE: [PATCH 0/4] Remove LPC register partitioning > > Hello, > > Thanks for your prompt feedback. > > > -----Original Message----- > > From: Andrew Jeffery <andrew@aj.id.au> > > Sent: Friday, September 11, 2020 12:46 PM > > To: Joel Stanley <joel@jms.id.au>; ChiaWei Wang > > <chiawei_wang@aspeedtech.com> > > Subject: Re: [PATCH 0/4] Remove LPC register partitioning > > > > > > On Fri, 11 Sep 2020, at 13:33, Joel Stanley wrote: > > > Hello, > > > > > > On Fri, 11 Sep 2020 at 03:46, Chia-Wei, Wang > > > <chiawei_wang@aspeedtech.com> wrote: > > > > > > > > The LPC controller has no concept of the BMC and the Host partitions. > > > > The incorrect partitioning can impose unnecessary range > > > > restrictions on register access through the syscon regmap interface. > > > > > > > > For instance, HICRB contains the I/O port address configuration of > > > > KCS channel 1/2. However, the KCS#1/#2 drivers cannot access HICRB > > > > as it is located at the other LPC partition. > > > > Thanks for addressing this, I've regretted that choice for a while now. > > > > The split was rooted in trying to support pinmux while not being > > across every detail of the LPC controller, and so I made some poor decisions. > > > > > > > > > > In addition, to be backward compatible, the newly added HW control > > > > bits could be added at any reserved bits over the LPC addressing space. > > > > > > > > Thereby, this patch series aims to remove the LPC partitioning for > > > > better driver development and maintenance. > > > > > > I support this cleanup. The only consideration is to be careful with > > > breaking the driver/device-tree relationship. We either need to > > > ensure the drivers remain compatible with both device trees. > > > > > > Another solution is to get agreement from all parties that for the > > > LPC device the device tree is always the one shipped with the > > > kernel, so it is okay to make incompatible changes. > If it is possible, I would prefer this solution to avoid adding additional if-logic > for the compatibility support in the driver implementation. > As the patch can be less change made to register offset definitions and leave > the core logic untouched. > > > > > > While we are doing a cleanup, Andrew suggested we remove the > > > detailed description of LPC out of the device tree. We would have > > > the one LPC node, and create a LPC driver that creates all of the > > > sub devices (snoop, FW cycles, kcs, bt, vuart). Andrew, can you > > > elaborate on this plan? > > > > I dug up the conversation I had with Rob over a year ago about being > > unhappy with what I'd cooked up. > > > > https://lore.kernel.org/linux-arm-kernel/CAL_JsqJ+sFDG8eKbV3gvmqVHx+ot > > W > > bki4dY213apzXgfhbXXEw@mail.gmail.com/ > > > > But I think you covered most of the idea there: We have the LPC driver > > create the subdevices and that moves the details out of the devicetree. > > However, I haven't thought about it more than that, and I think there > > are still problems with that idea. For instance, how we manage > > configuration of those devices, and how to enable only the devices a > > given platform actually cares about (i.e. the problems that devicetree solves > for us). > Another concern to make centralized LPC driver implementation more > complicated is the relationship with eSPI driver. > AST2500 binds the reset control of LPC and eSPI together. If eSPI is used for the > Host communication, the behavior in current "lpc-ctrl" should be skipped but > not for KCS, BT, Snoop, etc. > And this will be much easier to achieve by devicetree if LPC sub devices are > individually described. > > > > It may be that the only way to do that is with platform code, and > > that's not really a direction we should be going either. > >
| |