[PATCH] soc: aspeed-lpc-ctrl: LPC to AHB mapping on ast2600

[Joel Stanley]

On Mon, 16 Mar 2020 at 01:58, Andrew Jeffery <andrew at aj.id.au> wrote:
>
>
>
> On Thu, 12 Mar 2020, at 22:44, Joel Stanley wrote:
> > The ast2600 disables the mapping of AHB memory regions by default,
> > only allowing the LPC window to point to SPI NOR. In order to point the
> > window to any AHB address, an ast2600 specific bit must be toggled.
> >
> > Signed-off-by: Joel Stanley <joel at jms.id.au>
> > ---
> >  drivers/soc/aspeed/aspeed-lpc-ctrl.c | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> >
> > diff --git a/drivers/soc/aspeed/aspeed-lpc-ctrl.c
> > b/drivers/soc/aspeed/aspeed-lpc-ctrl.c
> > index f4ac14c40518..142cb4cc85e7 100644
> > --- a/drivers/soc/aspeed/aspeed-lpc-ctrl.c
> > +++ b/drivers/soc/aspeed/aspeed-lpc-ctrl.c
> > @@ -22,6 +22,9 @@
> >  #define HICR5_ENL2H  BIT(8)
> >  #define HICR5_ENFWH  BIT(10)
> >
> > +#define HICR6 0x4
>
> Given you introduced this I assume we don't have anything else touching
> the register, but if we ever do hopefully whoever it is that adds support is
> conscious that they need to be doing an read/modify/write to correctly
> clear the W1C registers without frobbing the bridge state.
>
> Looks like Aspeed should have introduced two bits to manage it :/

Yes, it would have been nice to have a separate register.

>
> > +#define SW_FWH2AHB   BIT(17)
> > +
> >  #define HICR7 0x8
> >  #define HICR8 0xc
> >
> > @@ -33,6 +36,7 @@ struct aspeed_lpc_ctrl {
> >       resource_size_t         mem_size;
> >       u32             pnor_size;
> >       u32             pnor_base;
> > +     bool                    fwh2ahb;
> >  };
> >
> >  static struct aspeed_lpc_ctrl *file_aspeed_lpc_ctrl(struct file *file)
> > @@ -177,6 +181,16 @@ static long aspeed_lpc_ctrl_ioctl(struct file
> > *file, unsigned int cmd,
> >               if (rc)
> >                       return rc;
> >
> > +             /*
> > +              * Switch to FWH2AHB mode, AST2600 only.
> > +              *
> > +              * The other bits in this register are interrupt status bits
> > +              * that are cleared by writing 1. As we don't want to clear
> > +              * them, set only the bit of interest.
> > +              */
> > +             if (lpc_ctrl->fwh2ahb)
> > +                     regmap_write(lpc_ctrl->regmap, HICR6, SW_FWH2AHB);
> > +
> >               /*
> >                * Enable LPC FHW cycles. This is required for the host to
> >                * access the regions specified.
> > @@ -274,6 +288,9 @@ static int aspeed_lpc_ctrl_probe(struct
> > platform_device *pdev)
> >               return rc;
> >       }
> >
> > +     if (of_device_is_compatible(dev->of_node, "aspeed,ast2600-lpc-ctrl"))
> > +             lpc_ctrl->fwh2ahb = true;
> > +
>
> This implies that we don't want the SPI-only behaviour by default, which is
> true for our platforms but doesn't really reflect the hardware. What are your
> thoughts on adding an explict devicetree property? use-fwh2ahb?

I chose to do it this way as userspace that is calling the ioctl to
set the mapping is probably expecting this behaviour. If someone in
the future wants to enhance the driver to separate out "lpc2spi" from
"lpc2ahb" then we could consider their patches.

The other upside of this is it maintains backwards compatibility with
the previous SoCs.

Cheers,

Joel