On Fri, 25 Sep 2020, at 14:19, Joel Stanley wrote:

On Mon, 16 Mar 2020 at 01:58, Andrew Jeffery [off-list ref] wrote:

quoted

On Thu, 12 Mar 2020, at 22:44, Joel Stanley wrote:

quoted

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@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.

quoted

quoted

+#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.

Yep, all good.

Reviewed-by: Andrew Jeffery <redacted>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel