[PATCH linux dev-4.13 v2 0/4] LPC fixes

[PATCH linux dev-4.13 v2 0/4] LPC fixes

[Joel Stanley]

These patches were developed when bringing up dev-4.13 on Romulus. We
need to ensure the LPC clock is enabled, now that the clock driver turns
off any unused clocks. We also need to enable the LPC firmware cycles
bit as we no longer have the aspeed.c hack.

v2:
 - Set bit 10 and bit 8 to get the LPC mapping working
 - Add bindings documents

Joel Stanley (4):
  ARM: dts: aspeed: Add LPC clock phandles
  misc: aspeed-lpc: Request and enable LPC clock
  dt-bindings: aspeed-lpc: Document LPC Host Interface Controller
  misc: aspeed-lpc-ctrl: Enable FWH and A2H bridge cycles

 .../devicetree/bindings/mfd/aspeed-lpc.txt         | 39 ++++++++++++++++++++++
 arch/arm/boot/dts/aspeed-g4.dtsi                   |  1 +
 arch/arm/boot/dts/aspeed-g5.dtsi                   |  1 +
 drivers/misc/aspeed-lpc-ctrl.c                     | 32 ++++++++++++++++--
 4 files changed, 71 insertions(+), 2 deletions(-)

-- 
2.15.1

[PATCH linux dev-4.13 v2 1/4] ARM: dts: aspeed: Add LPC clock phandles

[Joel Stanley]

The LPC device uses LCLK.

Tested-by: Lei YU <mine260309@gmail.com>
Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 arch/arm/boot/dts/aspeed-g4.dtsi | 1 +
 arch/arm/boot/dts/aspeed-g5.dtsi | 1 +
 2 files changed, 2 insertions(+)

diff --git a/arch/arm/boot/dts/aspeed-g4.dtsi b/arch/arm/boot/dts/aspeed-g4.dtsi
index 1fb5b410917a..d697380f2f39 100644
--- a/arch/arm/boot/dts/aspeed-g4.dtsi
+++ b/arch/arm/boot/dts/aspeed-g4.dtsi
@@ -233,6 +233,7 @@
 					lpc_ctrl: lpc-ctrl@0 {
 						compatible = "aspeed,ast2400-lpc-ctrl";
 						reg = <0x0 0x80>;
+						clocks = <&syscon ASPEED_CLK_GATE_LCLK>;
 						status = "disabled";
 					};
 
diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi b/arch/arm/boot/dts/aspeed-g5.dtsi
index 77d6d98d24a0..89ef960c4ef3 100644
--- a/arch/arm/boot/dts/aspeed-g5.dtsi
+++ b/arch/arm/boot/dts/aspeed-g5.dtsi
@@ -283,6 +283,7 @@
 					lpc_ctrl: lpc-ctrl@0 {
 						compatible = "aspeed,ast2500-lpc-ctrl";
 						reg = <0x0 0x80>;
+						clocks = <&syscon ASPEED_CLK_GATE_LCLK>;
 						status = "disabled";
 					};
 
-- 
2.15.1

[PATCH linux dev-4.13 v2 2/4] misc: aspeed-lpc: Request and enable LPC clock

[Joel Stanley]

The LPC device needs to ensure it's clock is enabled before it can do
anything.

Tested-by: Lei YU <mine260309@gmail.com>
Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 drivers/misc/aspeed-lpc-ctrl.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/misc/aspeed-lpc-ctrl.c b/drivers/misc/aspeed-lpc-ctrl.c
index b5439643f54b..4a818ad3827e 100644
--- a/drivers/misc/aspeed-lpc-ctrl.c
+++ b/drivers/misc/aspeed-lpc-ctrl.c
@@ -7,6 +7,7 @@
  * 2 of the License, or (at your option) any later version.
  */
 
+#include <linux/clk.h>
 #include <linux/mfd/syscon.h>
 #include <linux/miscdevice.h>
 #include <linux/mm.h>
@@ -26,6 +27,7 @@
 struct aspeed_lpc_ctrl {
 	struct miscdevice	miscdev;
 	struct regmap		*regmap;
+	struct clk		*clk;
 	phys_addr_t		mem_base;
 	resource_size_t		mem_size;
 	u32		pnor_size;
@@ -180,6 +182,17 @@ static int aspeed_lpc_ctrl_probe(struct platform_device *pdev)
 	if (!lpc_ctrl)
 		return -ENOMEM;
 
+	lpc_ctrl->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(lpc_ctrl->clk)) {
+		dev_err(dev, "couldn't get clock\n");
+		return PTR_ERR(lpc_ctrl->clk);
+	}
+	rc = clk_prepare_enable(lpc_ctrl->clk);
+	if (rc) {
+		dev_err(dev, "couldn't enable clock\n");
+		return rc;
+	}
+
 	node = of_parse_phandle(dev->of_node, "flash", 0);
 	if (!node) {
 		dev_err(dev, "Didn't find host pnor flash node\n");
@@ -239,6 +252,7 @@ static int aspeed_lpc_ctrl_remove(struct platform_device *pdev)
 	struct aspeed_lpc_ctrl *lpc_ctrl = dev_get_drvdata(&pdev->dev);
 
 	misc_deregister(&lpc_ctrl->miscdev);
+	clk_disable_unprepare(lpc_ctrl->clk);
 
 	return 0;
 }
-- 
2.15.1

Re: [PATCH linux dev-4.13 v2 2/4] misc: aspeed-lpc: Request and enable LPC clock

[Cyril Bur]

On Fri, 2018-02-09 at 15:41 +1030, Joel Stanley wrote:
> The LPC device needs to ensure it's clock is enabled before it can do
> anything.
> 
> Tested-by: Lei YU <mine260309@gmail.com>
> Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
>  drivers/misc/aspeed-lpc-ctrl.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/misc/aspeed-lpc-ctrl.c b/drivers/misc/aspeed-lpc-ctrl.c
> index b5439643f54b..4a818ad3827e 100644
> --- a/drivers/misc/aspeed-lpc-ctrl.c
> +++ b/drivers/misc/aspeed-lpc-ctrl.c
> @@ -7,6 +7,7 @@
>   * 2 of the License, or (at your option) any later version.
>   */
>  
> +#include <linux/clk.h>
>  #include <linux/mfd/syscon.h>
>  #include <linux/miscdevice.h>
>  #include <linux/mm.h>
> @@ -26,6 +27,7 @@
>  struct aspeed_lpc_ctrl {
>  	struct miscdevice	miscdev;
>  	struct regmap		*regmap;
> +	struct clk		*clk;
>  	phys_addr_t		mem_base;
>  	resource_size_t		mem_size;
>  	u32		pnor_size;
> @@ -180,6 +182,17 @@ static int aspeed_lpc_ctrl_probe(struct platform_device *pdev)
>  	if (!lpc_ctrl)
>  		return -ENOMEM;
>  
> +	lpc_ctrl->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(lpc_ctrl->clk)) {
> +		dev_err(dev, "couldn't get clock\n");
> +		return PTR_ERR(lpc_ctrl->clk);
> +	}
> +	rc = clk_prepare_enable(lpc_ctrl->clk);
> +	if (rc) {
> +		dev_err(dev, "couldn't enable clock\n");
> +		return rc;
> +	}
> +

Hi Joel,

Shouldn't you be calling clk_disable_unprepare() on error cases in the
rest of aspeed_lpc_ctrl_probe()?

Cyril
>  	node = of_parse_phandle(dev->of_node, "flash", 0);
>  	if (!node) {
>  		dev_err(dev, "Didn't find host pnor flash node\n");
> @@ -239,6 +252,7 @@ static int aspeed_lpc_ctrl_remove(struct platform_device *pdev)
>  	struct aspeed_lpc_ctrl *lpc_ctrl = dev_get_drvdata(&pdev->dev);
>  
>  	misc_deregister(&lpc_ctrl->miscdev);
> +	clk_disable_unprepare(lpc_ctrl->clk);
>  
>  	return 0;
>  }

[PATCH linux dev-4.13 v2 3/4] dt-bindings: aspeed-lpc: Document LPC Host Interface Controller

[Joel Stanley]

The LPC Host Interface Controller is part of a BMC SoC that is used for
communication with the host.

Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 .../devicetree/bindings/mfd/aspeed-lpc.txt         | 39 ++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/aspeed-lpc.txt b/Documentation/devicetree/bindings/mfd/aspeed-lpc.txt
index 514d82ced95b..c40b707df907 100644
--- a/Documentation/devicetree/bindings/mfd/aspeed-lpc.txt
+++ b/Documentation/devicetree/bindings/mfd/aspeed-lpc.txt
@@ -109,6 +109,45 @@ lpc: lpc@1e789000 {
 	};
 };
 
+BMC Node Children
+==================
+
+LPC Host Interface Controller
+-------------------
+
+The LPC Host Interface Controller manages functions exposed to the host such as
+LPC firmware hub cycles, configuration of the LPC-to-AHB mapping, UART
+management and bus snoop configuration.
+
+Required properties:
+
+- compatible:	One of:
+		"aspeed,ast2400-lpc-ctrl";
+		"aspeed,ast2500-lpc-ctrl";
+
+- reg:		contains offset/length values of the LHC memory regions
+
+- clocks:	contains a phandle to the syscon node describing the clocks.
+		There should then be one cell representing the clock to use
+
+- memory-region: A phandle to a reserved_memory region to be used for the LPC
+		to AHB mapping
+
+- flash:	A phandle to the SPI flash controller containing the flash to
+		be exposed over the LPC to AHB mapping
+
+Example:
+
+lpc-host@80 {
+	lpc_ctrl: lpc-ctrl@0 {
+		compatible = "aspeed,ast2500-lpc-ctrl";
+		reg = <0x0 0x80>;
+		clocks = <&syscon ASPEED_CLK_GATE_LCLK>;
+		memory-region = <&flash_memory>;
+		flash = <&spi>;
+	};
+};
+
 Host Node Children
 ==================
 
-- 
2.15.1

Re: [PATCH linux dev-4.13 v2 3/4] dt-bindings: aspeed-lpc: Document LPC Host Interface Controller

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 2305 bytes --]

On Fri, 2018-02-09 at 15:41 +1030, Joel Stanley wrote:
> The LPC Host Interface Controller is part of a BMC SoC that is used for
> communication with the host.
> 
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
>  .../devicetree/bindings/mfd/aspeed-lpc.txt         | 39 ++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/aspeed-lpc.txt b/Documentation/devicetree/bindings/mfd/aspeed-lpc.txt
> index 514d82ced95b..c40b707df907 100644
> --- a/Documentation/devicetree/bindings/mfd/aspeed-lpc.txt
> +++ b/Documentation/devicetree/bindings/mfd/aspeed-lpc.txt
> @@ -109,6 +109,45 @@ lpc: lpc@1e789000 {
>  	};
>  };
>  
> +BMC Node Children
> +==================
> +
> +LPC Host Interface Controller
> +-------------------
> +
> +The LPC Host Interface Controller manages functions exposed to the host such as
> +LPC firmware hub cycles, configuration of the LPC-to-AHB mapping, UART
> +management and bus snoop configuration.
> +
> +Required properties:
> +
> +- compatible:	One of:
> +		"aspeed,ast2400-lpc-ctrl";
> +		"aspeed,ast2500-lpc-ctrl";
> +
> +- reg:		contains offset/length values of the LHC memory regions

It's annoying to reconcile this with the datasheet. I think LHC should
be HIC here: The LHC registers concern themselves with configuration of
the LPC bus itself, whilst the HIC registers control the behaviour of
the BMC with respect to requests over the LPC bus. This driver is for
the HIC part of the IP, so we should try to keep the documentation
consistent.

Cheers,

Andrew

> +
> +- clocks:	contains a phandle to the syscon node describing the clocks.
> +		There should then be one cell representing the clock to use
> +
> +- memory-region: A phandle to a reserved_memory region to be used for the LPC
> +		to AHB mapping
> +
> +- flash:	A phandle to the SPI flash controller containing the flash to
> +		be exposed over the LPC to AHB mapping
> +
> +Example:
> +
> +lpc-host@80 {
> +	lpc_ctrl: lpc-ctrl@0 {
> +		compatible = "aspeed,ast2500-lpc-ctrl";
> +		reg = <0x0 0x80>;
> +		clocks = <&syscon ASPEED_CLK_GATE_LCLK>;
> +		memory-region = <&flash_memory>;
> +		flash = <&spi>;
> +	};
> +};
> +
>  Host Node Children
>  ==================
>  

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[PATCH linux dev-4.13 v2 4/4] misc: aspeed-lpc-ctrl: Enable FWH and A2H bridge cycles

[Joel Stanley]

To date this driver has relied on prevous state from out of tree hacks
and vendor u-boot trees in order to have the host be able to access
data over the LPC bus.

Now we explicitly enable the AHB to LPC bridge and FWH cycles from when
the user first configures the address to map. We chose to do this then
as before that time there is no way for the kernel to know where it is
safe to point the LPC window.

Tested-by: Lei YU <mine260309@gmail.com>
Signed-off-by: Joel Stanley <joel@jms.id.au>
--
v2: Enable both FWH and A2H
---
 drivers/misc/aspeed-lpc-ctrl.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/misc/aspeed-lpc-ctrl.c b/drivers/misc/aspeed-lpc-ctrl.c
index 4a818ad3827e..1d7d3a6c570f 100644
--- a/drivers/misc/aspeed-lpc-ctrl.c
+++ b/drivers/misc/aspeed-lpc-ctrl.c
@@ -21,6 +21,10 @@
 
 #define DEVICE_NAME	"aspeed-lpc-ctrl"
 
+#define HICR5 0x0
+#define HICR5_ENL2H	BIT(8)
+#define HICR5_ENFWH	BIT(10)
+
 #define HICR7 0x8
 #define HICR8 0xc
 
@@ -155,8 +159,18 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file, unsigned int cmd,
 		if (rc)
 			return rc;
 
-		return regmap_write(lpc_ctrl->regmap, HICR8,
-			(~(map.size - 1)) | ((map.size >> 16) - 1));
+		rc = regmap_write(lpc_ctrl->regmap, HICR8,
+				(~(map.size - 1)) | ((map.size >> 16) - 1));
+		if (rc)
+			return rc;
+
+		/*
+		 * Enable LPC FHW cycles. This is required for the host to
+		 * access the regions specified.
+		 */
+		return regmap_update_bits(lpc_ctrl->regmap, HICR5,
+				HICR5_ENFWH | HICR5_ENL2H,
+				HICR5_ENFWH | HICR5_ENL2H);
 	}
 
 	return -EINVAL;
-- 
2.15.1

Re: [PATCH linux dev-4.13 v2 4/4] misc: aspeed-lpc-ctrl: Enable FWH and A2H bridge cycles

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 1866 bytes --]

On Fri, 2018-02-09 at 15:41 +1030, Joel Stanley wrote:
> To date this driver has relied on prevous state from out of tree hacks
> and vendor u-boot trees in order to have the host be able to access
> data over the LPC bus.
> 
> Now we explicitly enable the AHB to LPC bridge and FWH cycles from when
> the user first configures the address to map. We chose to do this then
> as before that time there is no way for the kernel to know where it is
> safe to point the LPC window.
> 
> Tested-by: Lei YU <mine260309@gmail.com>
> Signed-off-by: Joel Stanley <joel@jms.id.au>

Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

> --
> v2: Enable both FWH and A2H
> ---
>  drivers/misc/aspeed-lpc-ctrl.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/misc/aspeed-lpc-ctrl.c b/drivers/misc/aspeed-lpc-ctrl.c
> index 4a818ad3827e..1d7d3a6c570f 100644
> --- a/drivers/misc/aspeed-lpc-ctrl.c
> +++ b/drivers/misc/aspeed-lpc-ctrl.c
> @@ -21,6 +21,10 @@
>  
>  #define DEVICE_NAME	"aspeed-lpc-ctrl"
>  
> +#define HICR5 0x0
> +#define HICR5_ENL2H	BIT(8)
> +#define HICR5_ENFWH	BIT(10)
> +
>  #define HICR7 0x8
>  #define HICR8 0xc
>  
> @@ -155,8 +159,18 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file, unsigned int cmd,
>  		if (rc)
>  			return rc;
>  
> -		return regmap_write(lpc_ctrl->regmap, HICR8,
> -			(~(map.size - 1)) | ((map.size >> 16) - 1));
> +		rc = regmap_write(lpc_ctrl->regmap, HICR8,
> +				(~(map.size - 1)) | ((map.size >> 16) - 1));
> +		if (rc)
> +			return rc;
> +
> +		/*
> +		 * Enable LPC FHW cycles. This is required for the host to
> +		 * access the regions specified.
> +		 */
> +		return regmap_update_bits(lpc_ctrl->regmap, HICR5,
> +				HICR5_ENFWH | HICR5_ENL2H,
> +				HICR5_ENFWH | HICR5_ENL2H);
>  	}
>  
>  	return -EINVAL;

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]