RE: [PATCH 0/4] Remove LPC register partitioning

[Ryan Chen]

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@xxxxxxxxxxxxxx>
> Sent: Friday, September 11, 2020 4:21 PM
> To: Andrew Jeffery <andrew@xxxxxxxx>; Joel Stanley <joel@xxxxxxxxx>
> Cc: Rob Herring <robh+dt@xxxxxxxxxx>; Corey Minyard <minyard@xxxxxxx>;
> Linus Walleij <linus.walleij@xxxxxxxxxx>; Haiyue Wang
> <haiyue.wang@xxxxxxxxxxxxxxx>; Cyril Bur <cyrilbur@xxxxxxxxx>; Robert
> Lippert <rlippert@xxxxxxxxxx>; Linux ARM
> <linux-arm-kernel@xxxxxxxxxxxxxxxxxxx>; linux-aspeed
> <linux-aspeed@xxxxxxxxxxxxxxxx>; Linux Kernel Mailing List
> <linux-kernel@xxxxxxxxxxxxxxx>; OpenBMC Maillist
> <openbmc@xxxxxxxxxxxxxxxx>; Ryan Chen <ryan_chen@xxxxxxxxxxxxxx>
> Subject: RE: [PATCH 0/4] Remove LPC register partitioning
> 
> Hello,
> 
> Thanks for your prompt feedback.
> 
> > -----Original Message-----
> > From: Andrew Jeffery <andrew@xxxxxxxx>
> > Sent: Friday, September 11, 2020 12:46 PM
> > To: Joel Stanley <joel@xxxxxxxxx>; ChiaWei Wang
> > <chiawei_wang@xxxxxxxxxxxxxx>
> > 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@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx/
> >
> > 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.
> >