| Message ID | 20200312121413.294384-1-joel@jms.id.au (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | soc: aspeed-lpc-ctrl: LPC to AHB mapping on ast2600 | expand |
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@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 :/ > +#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? Andrew
On Mon, 16 Mar 2020 at 01:58, Andrew Jeffery <andrew@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@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
On Fri, 25 Sep 2020, at 14:19, Joel Stanley wrote: > On Mon, 16 Mar 2020 at 01:58, Andrew Jeffery <andrew@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@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. Yep, all good. Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
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 +#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; + lpc_ctrl->miscdev.minor = MISC_DYNAMIC_MINOR; lpc_ctrl->miscdev.name = DEVICE_NAME; lpc_ctrl->miscdev.fops = &aspeed_lpc_ctrl_fops;
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(+)