All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v4 2/3] ARM: dts: imx6: Support Savageboard dual
From: Milo Kim @ 2017-01-04  4:55 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20170104045553.26576-1-woogyom.kim@gmail.com>

Common savageboard DT file is used for board support.
Add the vendor name and specify the dtb file for i.MX6Q build.

Reviewed-by: Fabio Estevam <fabio.estevam@nxp.com>
Signed-off-by: Milo Kim <woogyom.kim@gmail.com>
---
 .../devicetree/bindings/vendor-prefixes.txt        |  1 +
 arch/arm/boot/dts/Makefile                         |  1 +
 arch/arm/boot/dts/imx6dl-savageboard.dts           | 51 ++++++++++++++++++++++
 3 files changed, 53 insertions(+)
 create mode 100644 arch/arm/boot/dts/imx6dl-savageboard.dts

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index 16d3b5e7f5d1..88c33d827e51 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -227,6 +227,7 @@ pine64	Pine64
 pixcir  PIXCIR MICROELECTRONICS Co., Ltd
 plathome	Plat'Home Co., Ltd.
 plda	PLDA
+poslab  Poslab Technology Co., Ltd.
 powervr	PowerVR (deprecated, use img)
 pulsedlight	PulsedLight, Inc
 qca	Qualcomm Atheros, Inc.
diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
index cccdbcb557b6..fb7f904a5235 100644
--- a/arch/arm/boot/dts/Makefile
+++ b/arch/arm/boot/dts/Makefile
@@ -357,6 +357,7 @@ dtb-$(CONFIG_SOC_IMX6Q) += \
 	imx6dl-sabreauto.dtb \
 	imx6dl-sabrelite.dtb \
 	imx6dl-sabresd.dtb \
+	imx6dl-savageboard.dtb \
 	imx6dl-ts4900.dtb \
 	imx6dl-tx6dl-comtft.dtb \
 	imx6dl-tx6s-8034.dtb \
diff --git a/arch/arm/boot/dts/imx6dl-savageboard.dts b/arch/arm/boot/dts/imx6dl-savageboard.dts
new file mode 100644
index 000000000000..b95469c520a4
--- /dev/null
+++ b/arch/arm/boot/dts/imx6dl-savageboard.dts
@@ -0,0 +1,51 @@
+/*
+ * Copyright (C) 2017 Milo Kim <woogyom.kim@gmail.com>
+ *
+ * This file is dual-licensed: you can use it either under the terms
+ * of the GPL or the X11 license, at your option. Note that this dual
+ * licensing only applies to this file, and not this project as a
+ * whole.
+ *
+ *  a) This file is free software; you can redistribute it and/or
+ *     modify it under the terms of the GNU General Public License as
+ *     published by the Free Software Foundation; either version 2 of the
+ *     License, or (at your option) any later version.
+ *
+ *     This file is distributed in the hope that it will be useful,
+ *     but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *     MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *     GNU General Public License for more details.
+ *
+ * Or, alternatively,
+ *
+ *  b) Permission is hereby granted, free of charge, to any person
+ *     obtaining a copy of this software and associated documentation
+ *     files (the "Software"), to deal in the Software without
+ *     restriction, including without limitation the rights to use,
+ *     copy, modify, merge, publish, distribute, sublicense, and/or
+ *     sell copies of the Software, and to permit persons to whom the
+ *     Software is furnished to do so, subject to the following
+ *     conditions:
+ *
+ *     The above copyright notice and this permission notice shall be
+ *     included in all copies or substantial portions of the Software.
+ *
+ *     THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ *     EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
+ *     OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ *     NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
+ *     HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
+ *     WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ *     FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ *     OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+/dts-v1/;
+
+#include "imx6dl.dtsi"
+#include "imx6qdl-savageboard.dtsi"
+
+/ {
+	model = "Poslab SavageBoard Dual";
+	compatible = "poslab,imx6dl-savageboard", "fsl,imx6dl";
+};
-- 
2.11.0

^ permalink raw reply related

* [PATCH v4 2/3] ARM: dts: imx6: Support Savageboard dual
From: Milo Kim @ 2017-01-04  4:55 UTC (permalink / raw)
  To: Shawn Guo, Sascha Hauer
  Cc: Fabio Estevam, devicetree, Milo Kim, linux-kernel,
	linux-arm-kernel
In-Reply-To: <20170104045553.26576-1-woogyom.kim@gmail.com>

Common savageboard DT file is used for board support.
Add the vendor name and specify the dtb file for i.MX6Q build.

Reviewed-by: Fabio Estevam <fabio.estevam@nxp.com>
Signed-off-by: Milo Kim <woogyom.kim@gmail.com>
---
 .../devicetree/bindings/vendor-prefixes.txt        |  1 +
 arch/arm/boot/dts/Makefile                         |  1 +
 arch/arm/boot/dts/imx6dl-savageboard.dts           | 51 ++++++++++++++++++++++
 3 files changed, 53 insertions(+)
 create mode 100644 arch/arm/boot/dts/imx6dl-savageboard.dts

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index 16d3b5e7f5d1..88c33d827e51 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -227,6 +227,7 @@ pine64	Pine64
 pixcir  PIXCIR MICROELECTRONICS Co., Ltd
 plathome	Plat'Home Co., Ltd.
 plda	PLDA
+poslab  Poslab Technology Co., Ltd.
 powervr	PowerVR (deprecated, use img)
 pulsedlight	PulsedLight, Inc
 qca	Qualcomm Atheros, Inc.
diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
index cccdbcb557b6..fb7f904a5235 100644
--- a/arch/arm/boot/dts/Makefile
+++ b/arch/arm/boot/dts/Makefile
@@ -357,6 +357,7 @@ dtb-$(CONFIG_SOC_IMX6Q) += \
 	imx6dl-sabreauto.dtb \
 	imx6dl-sabrelite.dtb \
 	imx6dl-sabresd.dtb \
+	imx6dl-savageboard.dtb \
 	imx6dl-ts4900.dtb \
 	imx6dl-tx6dl-comtft.dtb \
 	imx6dl-tx6s-8034.dtb \
diff --git a/arch/arm/boot/dts/imx6dl-savageboard.dts b/arch/arm/boot/dts/imx6dl-savageboard.dts
new file mode 100644
index 000000000000..b95469c520a4
--- /dev/null
+++ b/arch/arm/boot/dts/imx6dl-savageboard.dts
@@ -0,0 +1,51 @@
+/*
+ * Copyright (C) 2017 Milo Kim <woogyom.kim@gmail.com>
+ *
+ * This file is dual-licensed: you can use it either under the terms
+ * of the GPL or the X11 license, at your option. Note that this dual
+ * licensing only applies to this file, and not this project as a
+ * whole.
+ *
+ *  a) This file is free software; you can redistribute it and/or
+ *     modify it under the terms of the GNU General Public License as
+ *     published by the Free Software Foundation; either version 2 of the
+ *     License, or (at your option) any later version.
+ *
+ *     This file is distributed in the hope that it will be useful,
+ *     but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *     MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *     GNU General Public License for more details.
+ *
+ * Or, alternatively,
+ *
+ *  b) Permission is hereby granted, free of charge, to any person
+ *     obtaining a copy of this software and associated documentation
+ *     files (the "Software"), to deal in the Software without
+ *     restriction, including without limitation the rights to use,
+ *     copy, modify, merge, publish, distribute, sublicense, and/or
+ *     sell copies of the Software, and to permit persons to whom the
+ *     Software is furnished to do so, subject to the following
+ *     conditions:
+ *
+ *     The above copyright notice and this permission notice shall be
+ *     included in all copies or substantial portions of the Software.
+ *
+ *     THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ *     EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
+ *     OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ *     NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
+ *     HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
+ *     WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ *     FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ *     OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+/dts-v1/;
+
+#include "imx6dl.dtsi"
+#include "imx6qdl-savageboard.dtsi"
+
+/ {
+	model = "Poslab SavageBoard Dual";
+	compatible = "poslab,imx6dl-savageboard", "fsl,imx6dl";
+};
-- 
2.11.0

^ permalink raw reply related

* [PATCH v4 3/3] ARM: dts: imx6: Support Savageboard quad
From: Milo Kim @ 2017-01-04  4:55 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20170104045553.26576-1-woogyom.kim@gmail.com>

Use common board file and support SATA interface additionally.
Specify the dtb file for i.MX6 build.

Reviewed-by: Fabio Estevam <fabio.estevam@nxp.com>
Signed-off-by: Milo Kim <woogyom.kim@gmail.com>
---
 arch/arm/boot/dts/Makefile              |  1 +
 arch/arm/boot/dts/imx6q-savageboard.dts | 55 +++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+)
 create mode 100644 arch/arm/boot/dts/imx6q-savageboard.dts

diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
index fb7f904a5235..2f21e59adc1e 100644
--- a/arch/arm/boot/dts/Makefile
+++ b/arch/arm/boot/dts/Makefile
@@ -403,6 +403,7 @@ dtb-$(CONFIG_SOC_IMX6Q) += \
 	imx6q-sabreauto.dtb \
 	imx6q-sabrelite.dtb \
 	imx6q-sabresd.dtb \
+	imx6q-savageboard.dtb \
 	imx6q-sbc6x.dtb \
 	imx6q-tbs2910.dtb \
 	imx6q-ts4900.dtb \
diff --git a/arch/arm/boot/dts/imx6q-savageboard.dts b/arch/arm/boot/dts/imx6q-savageboard.dts
new file mode 100644
index 000000000000..717ac62fc2cf
--- /dev/null
+++ b/arch/arm/boot/dts/imx6q-savageboard.dts
@@ -0,0 +1,55 @@
+/*
+ * Copyright (C) 2017 Milo Kim <woogyom.kim@gmail.com>
+ *
+ * This file is dual-licensed: you can use it either under the terms
+ * of the GPL or the X11 license, at your option. Note that this dual
+ * licensing only applies to this file, and not this project as a
+ * whole.
+ *
+ *  a) This file is free software; you can redistribute it and/or
+ *     modify it under the terms of the GNU General Public License as
+ *     published by the Free Software Foundation; either version 2 of the
+ *     License, or (at your option) any later version.
+ *
+ *     This file is distributed in the hope that it will be useful,
+ *     but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *     MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *     GNU General Public License for more details.
+ *
+ * Or, alternatively,
+ *
+ *  b) Permission is hereby granted, free of charge, to any person
+ *     obtaining a copy of this software and associated documentation
+ *     files (the "Software"), to deal in the Software without
+ *     restriction, including without limitation the rights to use,
+ *     copy, modify, merge, publish, distribute, sublicense, and/or
+ *     sell copies of the Software, and to permit persons to whom the
+ *     Software is furnished to do so, subject to the following
+ *     conditions:
+ *
+ *     The above copyright notice and this permission notice shall be
+ *     included in all copies or substantial portions of the Software.
+ *
+ *     THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ *     EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
+ *     OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ *     NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
+ *     HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
+ *     WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ *     FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ *     OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+/dts-v1/;
+
+#include "imx6q.dtsi"
+#include "imx6qdl-savageboard.dtsi"
+
+/ {
+	model = "Poslab SavageBoard Quad";
+	compatible = "poslab,imx6q-savageboard", "fsl,imx6q";
+};
+
+&sata {
+	status = "okay";
+};
-- 
2.11.0

^ permalink raw reply related

* [PATCH v4 3/3] ARM: dts: imx6: Support Savageboard quad
From: Milo Kim @ 2017-01-04  4:55 UTC (permalink / raw)
  To: Shawn Guo, Sascha Hauer
  Cc: Fabio Estevam, devicetree, Milo Kim, linux-kernel,
	linux-arm-kernel
In-Reply-To: <20170104045553.26576-1-woogyom.kim@gmail.com>

Use common board file and support SATA interface additionally.
Specify the dtb file for i.MX6 build.

Reviewed-by: Fabio Estevam <fabio.estevam@nxp.com>
Signed-off-by: Milo Kim <woogyom.kim@gmail.com>
---
 arch/arm/boot/dts/Makefile              |  1 +
 arch/arm/boot/dts/imx6q-savageboard.dts | 55 +++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+)
 create mode 100644 arch/arm/boot/dts/imx6q-savageboard.dts

diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
index fb7f904a5235..2f21e59adc1e 100644
--- a/arch/arm/boot/dts/Makefile
+++ b/arch/arm/boot/dts/Makefile
@@ -403,6 +403,7 @@ dtb-$(CONFIG_SOC_IMX6Q) += \
 	imx6q-sabreauto.dtb \
 	imx6q-sabrelite.dtb \
 	imx6q-sabresd.dtb \
+	imx6q-savageboard.dtb \
 	imx6q-sbc6x.dtb \
 	imx6q-tbs2910.dtb \
 	imx6q-ts4900.dtb \
diff --git a/arch/arm/boot/dts/imx6q-savageboard.dts b/arch/arm/boot/dts/imx6q-savageboard.dts
new file mode 100644
index 000000000000..717ac62fc2cf
--- /dev/null
+++ b/arch/arm/boot/dts/imx6q-savageboard.dts
@@ -0,0 +1,55 @@
+/*
+ * Copyright (C) 2017 Milo Kim <woogyom.kim@gmail.com>
+ *
+ * This file is dual-licensed: you can use it either under the terms
+ * of the GPL or the X11 license, at your option. Note that this dual
+ * licensing only applies to this file, and not this project as a
+ * whole.
+ *
+ *  a) This file is free software; you can redistribute it and/or
+ *     modify it under the terms of the GNU General Public License as
+ *     published by the Free Software Foundation; either version 2 of the
+ *     License, or (at your option) any later version.
+ *
+ *     This file is distributed in the hope that it will be useful,
+ *     but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *     MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *     GNU General Public License for more details.
+ *
+ * Or, alternatively,
+ *
+ *  b) Permission is hereby granted, free of charge, to any person
+ *     obtaining a copy of this software and associated documentation
+ *     files (the "Software"), to deal in the Software without
+ *     restriction, including without limitation the rights to use,
+ *     copy, modify, merge, publish, distribute, sublicense, and/or
+ *     sell copies of the Software, and to permit persons to whom the
+ *     Software is furnished to do so, subject to the following
+ *     conditions:
+ *
+ *     The above copyright notice and this permission notice shall be
+ *     included in all copies or substantial portions of the Software.
+ *
+ *     THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ *     EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
+ *     OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ *     NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
+ *     HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
+ *     WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ *     FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ *     OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+/dts-v1/;
+
+#include "imx6q.dtsi"
+#include "imx6qdl-savageboard.dtsi"
+
+/ {
+	model = "Poslab SavageBoard Quad";
+	compatible = "poslab,imx6q-savageboard", "fsl,imx6q";
+};
+
+&sata {
+	status = "okay";
+};
-- 
2.11.0

^ permalink raw reply related

* [PATCH v4 3/3] ARM: dts: imx6: Support Savageboard quad
From: Milo Kim @ 2017-01-04  4:55 UTC (permalink / raw)
  To: Shawn Guo, Sascha Hauer
  Cc: Fabio Estevam, linux-arm-kernel, devicetree, linux-kernel,
	Milo Kim
In-Reply-To: <20170104045553.26576-1-woogyom.kim@gmail.com>

Use common board file and support SATA interface additionally.
Specify the dtb file for i.MX6 build.

Reviewed-by: Fabio Estevam <fabio.estevam@nxp.com>
Signed-off-by: Milo Kim <woogyom.kim@gmail.com>
---
 arch/arm/boot/dts/Makefile              |  1 +
 arch/arm/boot/dts/imx6q-savageboard.dts | 55 +++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+)
 create mode 100644 arch/arm/boot/dts/imx6q-savageboard.dts

diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
index fb7f904a5235..2f21e59adc1e 100644
--- a/arch/arm/boot/dts/Makefile
+++ b/arch/arm/boot/dts/Makefile
@@ -403,6 +403,7 @@ dtb-$(CONFIG_SOC_IMX6Q) += \
 	imx6q-sabreauto.dtb \
 	imx6q-sabrelite.dtb \
 	imx6q-sabresd.dtb \
+	imx6q-savageboard.dtb \
 	imx6q-sbc6x.dtb \
 	imx6q-tbs2910.dtb \
 	imx6q-ts4900.dtb \
diff --git a/arch/arm/boot/dts/imx6q-savageboard.dts b/arch/arm/boot/dts/imx6q-savageboard.dts
new file mode 100644
index 000000000000..717ac62fc2cf
--- /dev/null
+++ b/arch/arm/boot/dts/imx6q-savageboard.dts
@@ -0,0 +1,55 @@
+/*
+ * Copyright (C) 2017 Milo Kim <woogyom.kim@gmail.com>
+ *
+ * This file is dual-licensed: you can use it either under the terms
+ * of the GPL or the X11 license, at your option. Note that this dual
+ * licensing only applies to this file, and not this project as a
+ * whole.
+ *
+ *  a) This file is free software; you can redistribute it and/or
+ *     modify it under the terms of the GNU General Public License as
+ *     published by the Free Software Foundation; either version 2 of the
+ *     License, or (at your option) any later version.
+ *
+ *     This file is distributed in the hope that it will be useful,
+ *     but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *     MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *     GNU General Public License for more details.
+ *
+ * Or, alternatively,
+ *
+ *  b) Permission is hereby granted, free of charge, to any person
+ *     obtaining a copy of this software and associated documentation
+ *     files (the "Software"), to deal in the Software without
+ *     restriction, including without limitation the rights to use,
+ *     copy, modify, merge, publish, distribute, sublicense, and/or
+ *     sell copies of the Software, and to permit persons to whom the
+ *     Software is furnished to do so, subject to the following
+ *     conditions:
+ *
+ *     The above copyright notice and this permission notice shall be
+ *     included in all copies or substantial portions of the Software.
+ *
+ *     THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ *     EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
+ *     OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ *     NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
+ *     HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
+ *     WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ *     FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ *     OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+/dts-v1/;
+
+#include "imx6q.dtsi"
+#include "imx6qdl-savageboard.dtsi"
+
+/ {
+	model = "Poslab SavageBoard Quad";
+	compatible = "poslab,imx6q-savageboard", "fsl,imx6q";
+};
+
+&sata {
+	status = "okay";
+};
-- 
2.11.0

^ permalink raw reply related

* Re: [Qemu-arm] [PATCH v4 6/8] hw/timer: Add Epson RX8900 RTC support
From: Andrew Jeffery @ 2017-01-04  4:59 UTC (permalink / raw)
  To: Alastair D'Silva, qemu-arm
  Cc: Peter Maydell, qemu-devel, Chris Smart, Joel Stanley,
	Alastair D'Silva, Cédric Le Goater
In-Reply-To: <20161215054812.12602-7-alastair@au1.ibm.com>

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

Hi Alastair,

I have some mostly minor comments below.

On Thu, 2016-12-15 at 16:48 +1100, Alastair D'Silva wrote:
> > From: Alastair D'Silva <alastair@d-silva.org>
> 
> This patch adds support for the Epson RX8900 I2C RTC.
> 
> The following chip features are implemented:
>  - RTC (wallclock based, ptimer 10x oversampling to pick up
>         wallclock transitions)
>  - Time update interrupt (per second/minute, wallclock based)
>  - Alarms (wallclock based)
>  - Temperature (set via a property)
>  - Countdown timer (emulated clock via ptimer)
>  - FOUT via GPIO (emulated clock via ptimer)
> 
> The following chip features are unimplemented:
>  - Low voltage detection
>  - i2c timeout
> 
> The implementation exports the following named GPIOs:
> rx8900-interrupt-out
> rx8900-fout-enable
> rx8900-fout
> 
> > Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> > Signed-off-by: Chris Smart <chris@distroguy.com>
> ---
>  default-configs/arm-softmmu.mak |   1 +
>  hw/timer/Makefile.objs          |   2 +
>  hw/timer/rx8900.c               | 912 ++++++++++++++++++++++++++++++++++++++++
>  hw/timer/rx8900_regs.h          | 141 +++++++
>  hw/timer/trace-events           |  31 ++
>  5 files changed, 1087 insertions(+)
>  create mode 100644 hw/timer/rx8900.c
>  create mode 100644 hw/timer/rx8900_regs.h
> 
> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
> index 6de3e16..adb600e 100644
> --- a/default-configs/arm-softmmu.mak
> +++ b/default-configs/arm-softmmu.mak
> @@ -29,6 +29,7 @@ CONFIG_SMC91C111=y
>  CONFIG_ALLWINNER_EMAC=y
>  CONFIG_IMX_FEC=y
>  CONFIG_DS1338=y
> +CONFIG_RX8900=y
>  CONFIG_PFLASH_CFI01=y
>  CONFIG_PFLASH_CFI02=y
>  CONFIG_MICRODRIVE=y
> diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs
> index 7ba8c23..fa028ac 100644
> --- a/hw/timer/Makefile.objs
> +++ b/hw/timer/Makefile.objs
> @@ -3,6 +3,7 @@ common-obj-$(CONFIG_ARM_MPTIMER) += arm_mptimer.o
>  common-obj-$(CONFIG_A9_GTIMER) += a9gtimer.o
>  common-obj-$(CONFIG_CADENCE) += cadence_ttc.o
>  common-obj-$(CONFIG_DS1338) += ds1338.o
> +common-obj-$(CONFIG_RX8900) += rx8900.o
>  common-obj-$(CONFIG_HPET) += hpet.o
>  common-obj-$(CONFIG_I8254) += i8254_common.o i8254.o
>  common-obj-$(CONFIG_M48T59) += m48t59.o
> @@ -17,6 +18,7 @@ common-obj-$(CONFIG_IMX) += imx_epit.o
>  common-obj-$(CONFIG_IMX) += imx_gpt.o
>  common-obj-$(CONFIG_LM32) += lm32_timer.o
>  common-obj-$(CONFIG_MILKYMIST) += milkymist-sysctl.o
> +common-obj-$(CONFIG_RX8900) += rx8900.o
>  
>  obj-$(CONFIG_EXYNOS4) += exynos4210_mct.o
>  obj-$(CONFIG_EXYNOS4) += exynos4210_pwm.o
> diff --git a/hw/timer/rx8900.c b/hw/timer/rx8900.c
> new file mode 100644
> index 0000000..cb1a2c8
> --- /dev/null
> +++ b/hw/timer/rx8900.c
> @@ -0,0 +1,912 @@
> +/*
> + * Epson RX8900SA/CE Realtime Clock Module
> + *
> + * Copyright (c) 2016 IBM Corporation
> + * Authors:
> > + *  Alastair D'Silva <alastair@d-silva.org>
> > + *  Chris Smart <chris@distroguy.com>
> + *
> + * This code is licensed under the GPL version 2 or later.  See
> + * the COPYING file in the top-level directory.
> + *
> + * Datasheet available at:
> + *  https://support.epson.biz/td/api/doc_check.php?dl=app_RX8900CE&lang=en
> + *
> + * Not implemented:
> + *  Implement i2c timeout
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu-common.h"
> +#include "hw/i2c/i2c.h"
> +#include "hw/timer/rx8900_regs.h"
> +#include "hw/ptimer.h"
> +#include "qemu/main-loop.h"
> +#include "qemu/bcd.h"
> +#include "qemu/log.h"
> +#include "qapi/error.h"
> +#include "qapi/visitor.h"
> +#include "trace.h"
> +
> +#define TYPE_RX8900 "rx8900"
> +#define RX8900(obj) OBJECT_CHECK(RX8900State, (obj), TYPE_RX8900)
> +
> +typedef struct RX8900State {
> +    I2CSlave parent_obj;
> +
> +    ptimer_state *sec_timer; /* triggered once per second */
> +    ptimer_state *fout_timer;
> +    ptimer_state *countdown_timer;
> +    bool fout_state;
> +    int64_t offset;
> +    uint8_t weekday; /* Saved for deferred offset calculation, 0-6 */
> +    uint8_t wday_offset;
> +    uint8_t nvram[RX8900_NVRAM_SIZE];
> +    int32_t nvram_offset; /* Wrapped to stay within RX8900_NVRAM_SIZE */
> +    bool addr_byte;
> +    uint8_t last_interrupt_seconds; /* The last time the second timer ticked */
> +    /* the last minute the timer update interrupt was triggered (if enabled) */
> +    uint8_t last_update_interrupt_minutes;
> +    double supply_voltage;
> +    qemu_irq interrupt_pin;
> +    qemu_irq fout_pin;
> +} RX8900State;
> +
> +static const VMStateDescription vmstate_rx8900 = {
> +    .name = "rx8900",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_I2C_SLAVE(parent_obj, RX8900State),
> +        VMSTATE_PTIMER(sec_timer, RX8900State),
> +        VMSTATE_PTIMER(fout_timer, RX8900State),
> +        VMSTATE_PTIMER(countdown_timer, RX8900State),
> +        VMSTATE_BOOL(fout_state, RX8900State),
> +        VMSTATE_INT64(offset, RX8900State),
> +        VMSTATE_UINT8(weekday, RX8900State),
> +        VMSTATE_UINT8(wday_offset, RX8900State),
> +        VMSTATE_UINT8_ARRAY(nvram, RX8900State, RX8900_NVRAM_SIZE),
> +        VMSTATE_INT32(nvram_offset, RX8900State),
> +        VMSTATE_BOOL(addr_byte, RX8900State),
> +        VMSTATE_UINT8(last_interrupt_seconds, RX8900State),
> +        VMSTATE_UINT8(last_update_interrupt_minutes, RX8900State),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static void rx8900_reset(DeviceState *dev);
> +
> +static void capture_current_time(RX8900State *s)
> +{
> +    /* Capture the current time into the secondary registers
> +     * which will be actually read by the data transfer operation.
> +     */
> +    struct tm now;
> +    qemu_get_timedate(&now, s->offset);
> +    s->nvram[SECONDS] = to_bcd(now.tm_sec);
> +    s->nvram[MINUTES] = to_bcd(now.tm_min);
> +    s->nvram[HOURS] = to_bcd(now.tm_hour);
> +
> +    s->nvram[WEEKDAY] = 0x01 << ((now.tm_wday + s->wday_offset) % 7);
> +    s->nvram[DAY] = to_bcd(now.tm_mday);
> +    s->nvram[MONTH] = to_bcd(now.tm_mon + 1);
> +    s->nvram[YEAR] = to_bcd(now.tm_year % 100);
> +
> +    s->nvram[EXT_SECONDS] = s->nvram[SECONDS];
> +    s->nvram[EXT_MINUTES] = s->nvram[MINUTES];
> +    s->nvram[EXT_HOURS] = s->nvram[HOURS];
> +    s->nvram[EXT_WEEKDAY] = s->nvram[WEEKDAY];
> +    s->nvram[EXT_DAY] = s->nvram[DAY];
> +    s->nvram[EXT_MONTH] = s->nvram[MONTH];
> +    s->nvram[EXT_YEAR] = s->nvram[YEAR];
+
> +    trace_rx8900_capture_current_time(now.tm_hour, now.tm_min, now.tm_sec,
> +            (now.tm_wday + s->wday_offset) % 7,
> +            now.tm_mday, now.tm_mon, now.tm_year + 1900,
> +            s->nvram[HOURS], s->nvram[MINUTES], s->nvram[SECONDS],
> +            s->nvram[WEEKDAY], s->nvram[DAY], s->nvram[MONTH], s->nvram[YEAR]);
> +}
> +
> +/**
> + * Increment the internal register pointer, dealing with wrapping
> + * @param s the RTC to operate on
> + */
> +static void inc_regptr(RX8900State *s)
> +{
> +    /* The register pointer wraps around after 0x1F
> +     */
> +    s->nvram_offset = (s->nvram_offset + 1) & (RX8900_NVRAM_SIZE - 1);
> +    trace_rx8900_regptr_update(s->nvram_offset);
> +
> +    if (s->nvram_offset == START_ADDRESS) {
> +        trace_rx8900_regptr_overflow();
> +        capture_current_time(s);
> +    }
> +}
> +
> +#define INVALID_WEEKDAY 0xff
> +
> +/**
> + * Receive an I2C Event
> + * @param i2c the i2c device instance
> + * @param event the event to handle
> + */
> +static void rx8900_event(I2CSlave *i2c, enum i2c_event event)
> +{
> +    RX8900State *s = RX8900(i2c);
> +
> +    switch (event) {
> +    case I2C_START_RECV:
> +        /* In h/w, time capture happens on any START condition, not just a
> +         * START_RECV. For the emulation, it doesn't actually matter,
> +         * since a START_RECV has to occur before the data can be read.
> +         */
> +        capture_current_time(s);
> +        break;
> +    case I2C_START_SEND:
> +        s->addr_byte = true;
> +        break;
> +    case I2C_FINISH:
> +        if (s->weekday < 7) {
> +            /* We defer the weekday calculation as it is handed to us before
> +             * the date has been updated. If we calculate the weekday offset
> +             * when it is passed to us, we will incorrectly determine it
> +             * based on the current emulated date, rather than the date that
> +             * has been written.
> +             */
> +            struct tm now;
> +            qemu_get_timedate(&now, s->offset);
> +
> +            s->wday_offset = (s->weekday - now.tm_wday + 7) % 7;
> +
> +            trace_rx8900_event_weekday(s->weekday, BIT(s->weekday),
> +                    s->wday_offset);
> +
> +            s->weekday = INVALID_WEEKDAY;
> +        }
> +        break;
> +
> +    default:
> +        break;
> +    }
> +}
> +
> +/**
> + * Perform an i2c receive action
> + * @param i2c the i2c device instance
> + * @return the value of the current register
> + * @post the internal register pointer is incremented
> + */
> +static int rx8900_recv(I2CSlave *i2c)
> +{
> +    RX8900State *s = RX8900(i2c);
> +    uint8_t res = s->nvram[s->nvram_offset];
> +    trace_rx8900_read_register(s->nvram_offset, res);
> +    inc_regptr(s);
> +    return res;
> +}
> +
> +/**
> + * Disable the countdown timer
> + * @param s the RTC to operate on
> + */
> +static void disable_countdown_timer(RX8900State *s)
> +{
> +    trace_rx8900_disable_countdown_timer();
> +    ptimer_stop(s->countdown_timer);
> +}
> +
> +/**
> + * Enable the countdown timer
> + * @param s the RTC to operate on
> + */
> +static void enable_countdown_timer(RX8900State *s)
> +{
> +    trace_rx8900_enable_countdown_timer();
> +    ptimer_run(s->countdown_timer, 0);
> +}
> +
> +/**
> + * Tick the countdown timer
> + * @param opaque the device instance
> + */
> +static void rx8900_countdown_tick(void *opaque)
> +{
> +    RX8900State *s = (RX8900State *)opaque;
> +
> +    uint16_t count = s->nvram[TIMER_COUNTER_0] |
> +            ((s->nvram[TIMER_COUNTER_1] & 0x0F) << 8);
> +    trace_rx8900_countdown_tick(count);
> +    count--;
> +
> +    s->nvram[TIMER_COUNTER_0] = (uint8_t)(count & 0x00ff);
> +    s->nvram[TIMER_COUNTER_1] = (uint8_t)((count & 0x0f00) >> 8);
> +
> +    if (count == 0) {
> +        trace_rx8900_countdown_elapsed();
> +
> +        disable_countdown_timer(s);
> +
> +        s->nvram[FLAG_REGISTER] |= FLAG_MASK_TF;
> +
> +        if (s->nvram[CONTROL_REGISTER] & CTRL_MASK_TIE) {
> +            trace_rx8900_fire_interrupt();
> +            qemu_irq_pulse(s->interrupt_pin);
> +        }
> +    }
> +}
> +
> +/**
> + * Disable the per second timer
> + * @param s the RTC to operate on
> + */
> +static void disable_timer(RX8900State *s)
> +{
> +    trace_rx8900_disable_timer();
> +    ptimer_stop(s->sec_timer);
> +}
> +
> +/**
> + * Enable the per second timer
> + * @param s the RTC to operate on
> + */
> +static void enable_timer(RX8900State *s)
> +{
> +    trace_rx8900_enable_timer();
> +    ptimer_run(s->sec_timer, 0);
> +}
> +
> +/**
> + * Tick the per second timer (can be called more frequently as it early exits
> + * if the wall clock has not progressed)
> + * @param opaque the RTC to tick
> + */
> +static void rx8900_timer_tick(void *opaque)
> +{
> +    RX8900State *s = (RX8900State *)opaque;
> +    struct tm now;
> +    bool fire_interrupt = false;
> +    bool alarm_week_day_matches;
> +
> +    qemu_get_timedate(&now, s->offset);
> +
> +    if (now.tm_sec == s->last_interrupt_seconds) {
> +        return;
> +    }
> +
> +    s->last_interrupt_seconds = now.tm_sec;
> +
> +    trace_rx8900_tick();
> +
> +    /* Update timer interrupt */
> +    if (s->nvram[CONTROL_REGISTER] & CTRL_MASK_UIE) {
> +        if ((s->nvram[EXTENSION_REGISTER] & EXT_MASK_USEL) &&
> +                now.tm_min != s->last_update_interrupt_minutes) {
> +            s->last_update_interrupt_minutes = now.tm_min;
> +            s->nvram[FLAG_REGISTER] |= FLAG_MASK_UF;
> +            fire_interrupt = true;
> +        } else if (!(s->nvram[EXTENSION_REGISTER] & EXT_MASK_USEL)) {
> +            /* per second update interrupt */
> +            s->nvram[FLAG_REGISTER] |= FLAG_MASK_UF;
> +            fire_interrupt = true;
> +        }
> +    }
> +
> +    /* Alarm interrupt */
> +    if ((s->nvram[EXTENSION_REGISTER] & EXT_MASK_WADA)) {
> +        alarm_week_day_matches =
> +            s->nvram[ALARM_WEEK_DAY] == to_bcd(now.tm_mday);
> +    } else {
> +        alarm_week_day_matches =
> +            s->nvram[ALARM_WEEK_DAY] ==
> +                0x01 << ((now.tm_wday + s->wday_offset) % 7);
> +    }
> +
> +    if ((s->nvram[CONTROL_REGISTER] & CTRL_MASK_AIE) && now.tm_sec == 0 &&
> +            s->nvram[ALARM_MINUTE] == to_bcd(now.tm_min) &&
> +            s->nvram[ALARM_HOUR] == to_bcd(now.tm_hour) &&
> +            alarm_week_day_matches) {
> +        trace_rx8900_trigger_alarm();
> +        s->nvram[FLAG_REGISTER] |= FLAG_MASK_AF;
> +        fire_interrupt = true;
> +    }
> +
> +    if (fire_interrupt) {
> +        trace_rx8900_fire_interrupt();
> +        qemu_irq_pulse(s->interrupt_pin);
> +    }
> +}
> +
> +
> +#define COUNTDOWN_TIMER_FREQ 4096
> +
> +/**
> + * Validate the extension register and perform actions based on the bits
> + * @param s the RTC to operate on
> + * @param data the new data for the extension register
> + */
> +static void update_extension_register(RX8900State *s, uint8_t data)
> +{
> +    if (data & EXT_MASK_TEST) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                "Test bit is enabled but is forbidden by the manufacturer");
> +    }
> +
> +    if ((data ^ s->nvram[EXTENSION_REGISTER]) &
> +            (EXT_MASK_FSEL0 | EXT_MASK_FSEL1)) {
> +        /* FSELx has changed */
> +
> +        switch (data & (EXT_MASK_FSEL0 | EXT_MASK_FSEL1)) {
> +        case EXT_MASK_FSEL0:
> +            trace_rx8900_set_fout(1024);
> +            ptimer_set_limit(s->fout_timer, 32, 1);
> +            break;
> +        case EXT_MASK_FSEL1:
> +            trace_rx8900_set_fout(1);
> +            ptimer_set_limit(s->fout_timer, 32768, 1);
> +            break;
> +        case 0:
> +        case (EXT_MASK_FSEL0 | EXT_MASK_FSEL1):
> +            trace_rx8900_set_fout(32768);
> +            ptimer_set_limit(s->fout_timer, 1, 1);
> +            break;
> +        }
> +    }
> +
> +    if ((data ^ s->nvram[EXTENSION_REGISTER]) &
> +            (EXT_MASK_TSEL0 | EXT_MASK_TSEL1)) {
> +        /* TSELx has changed */
> +        switch (data & (EXT_MASK_TSEL0 | EXT_MASK_TSEL1)) {
> +        case 0:
> +            trace_rx8900_set_countdown_timer(64);
> +            ptimer_set_limit(s->countdown_timer, COUNTDOWN_TIMER_FREQ / 64, 1);
> +            break;
> +        case EXT_MASK_TSEL0:
> +            trace_rx8900_set_countdown_timer(1);
> +            ptimer_set_limit(s->countdown_timer, COUNTDOWN_TIMER_FREQ, 1);
> +            break;
> +        case EXT_MASK_TSEL1:
> +            trace_rx8900_set_countdown_timer_per_minute();
> +            ptimer_set_limit(s->countdown_timer, COUNTDOWN_TIMER_FREQ * 60, 1);
> +            break;
> +        case (EXT_MASK_TSEL0 | EXT_MASK_TSEL1):
> +            trace_rx8900_set_countdown_timer(COUNTDOWN_TIMER_FREQ);
> +            ptimer_set_limit(s->countdown_timer, 1, 1);
> +            break;
> +        }
> +    }
> +
> +    if (data & EXT_MASK_TE) {
> +        enable_countdown_timer(s);
> +    }
> +
> +    s->nvram[EXTENSION_REGISTER] = data;
> +    s->nvram[EXT_EXTENSION_REGISTER] = data;
> +
> +}
> +/**
> + * Validate the control register and perform actions based on the bits
> + * @param s the RTC to operate on
> + * @param data the new value for the control register
> + */
> +
> +static void update_control_register(RX8900State *s, uint8_t data)
> +{
> +    uint8_t diffmask = ~s->nvram[CONTROL_REGISTER] & data;
> +
> +    if (diffmask & CTRL_MASK_WP0) {
> +        data &= ~CTRL_MASK_WP0;
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +            "Attempt to write to write protected bit %d in control register",
> +            CTRL_REG_WP0);
> +    }
> +
> +    if (diffmask & CTRL_MASK_WP1) {
> +        data &= ~CTRL_MASK_WP1;
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +            "Attempt to write to write protected bit %d in control register",
> +            CTRL_REG_WP1);
> +    }
> +
> +    if (data & CTRL_MASK_RESET) {
> +        data &= ~CTRL_MASK_RESET;
> +        rx8900_reset(DEVICE(s));
> +    }
> +
> +    if (diffmask & CTRL_MASK_UIE) {
> +        /* Update interrupts were off and are now on */
> +        struct tm now;
> +
> +        trace_rx8900_enable_update_timer();
> +
> +        qemu_get_timedate(&now, s->offset);
> +
> +        s->last_update_interrupt_minutes = now.tm_min;
> +        s->last_interrupt_seconds = now.tm_sec;
> +        enable_timer(s);
> +    }
> +
> +    if (diffmask & CTRL_MASK_AIE) {
> +        /* Alarm interrupts were off and are now on */
> +        struct tm now;
> +
> +        trace_rx8900_enable_alarm();
> +
> +        qemu_get_timedate(&now, s->offset);
> +
> +        s->last_interrupt_seconds = now.tm_sec;
> +        enable_timer(s);
> +    }
> +
> +    if (!(data & (CTRL_MASK_UIE | CTRL_MASK_AIE))) {
> +        disable_timer(s);
> +    }
> +
> +    s->nvram[CONTROL_REGISTER] = data;
> +    s->nvram[EXT_CONTROL_REGISTER] = data;
> +}
> +
> +/**
> + * Validate the flag register
> + * @param s the RTC to operate on
> + * @param data the new value for the flag register
> + */
> +static void validate_flag_register(RX8900State *s, uint8_t *data)
> +{
> +    uint8_t diffmask = ~s->nvram[FLAG_REGISTER] & *data;
> +
> +    if (diffmask & FLAG_MASK_VDET) {
> +        *data &= ~FLAG_MASK_VDET;
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +            "Only 0 can be written to VDET bit %d in the flag register",
> +            FLAG_REG_VDET);
> +    }
> +
> +    if (diffmask & FLAG_MASK_VLF) {
> +        *data &= ~FLAG_MASK_VLF;
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +            "Only 0 can be written to VLF bit %d in the flag register",
> +            FLAG_REG_VLF);
> +    }
> +
> +    if (diffmask & FLAG_MASK_UNUSED_2) {
> +        *data &= ~FLAG_MASK_UNUSED_2;
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +            "Only 0 can be written to unused bit %d in the flag register",
> +            FLAG_REG_UNUSED_2);
> +    }
> +
> +    if (diffmask & FLAG_MASK_UNUSED_6) {
> +        *data &= ~FLAG_MASK_UNUSED_6;
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +            "Only 0 can be written to unused bit %d in the flag register",
> +            FLAG_REG_UNUSED_6);
> +    }
> +
> +    if (diffmask & FLAG_MASK_UNUSED_7) {
> +        *data &= ~FLAG_MASK_UNUSED_7;
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +            "Only 0 can be written to unused bit %d in the flag register",
> +            FLAG_REG_UNUSED_7);
> +    }
> +}
> +
> +/**
> + * Handle FOUT_ENABLE (FOE) line
> + * Enables/disables the FOUT line
> + * @param opaque the device instance
> + * @param n the IRQ number
> + * @param level true if the line has been raised
> + */
> +static void rx8900_fout_enable_handler(void *opaque, int n, int level)
> +{
> +    RX8900State *s = RX8900(opaque);
> +
> +    if (level) {
> +        trace_rx8900_enable_fout();
> +        ptimer_run(s->fout_timer, 0);
> +    } else {
> +        /* disable fout */
> +        trace_rx8900_disable_fout();
> +        ptimer_stop(s->fout_timer);
> +    }
> +}
> +
> +/**
> + * Tick the FOUT timer
> + * @param opaque the device instance
> + */
> +static void rx8900_fout_tick(void *opaque)
> +{
> +    RX8900State *s = (RX8900State *)opaque;
> +
> +    trace_rx8900_fout_toggle();
> +    s->fout_state = !s->fout_state;
> +
> +    if (s->fout_state) {
> +        qemu_irq_raise(s->fout_pin);
> +    } else {
> +        qemu_irq_lower(s->fout_pin);
> +    }

We could just use qemu_set_irq() here.

> +}
> +
> +/**
> + * Verify the current voltage and raise flags if it is low
> + * @param s the RTC to operate on
> + */
> +static void check_voltage(RX8900State *s)
> +{
> +    if (!(s->nvram[BACKUP_FUNCTION] & BACKUP_MASK_VDETOFF)) {
> +        if (s->supply_voltage < 2.0) {
> +            s->nvram[FLAG_REGISTER] |= FLAG_MASK_VDET;
> +        }
> +
> +        if (s->supply_voltage < 1.6) {
> +            s->nvram[FLAG_REGISTER] |= FLAG_MASK_VLF;
> +        }
> +    }
> +}
> +
> +/**
> + * Receive a byte of data from i2c
> + * @param i2c the i2c device that is receiving data
> + * @param data the data that was received
> + */
> +static int rx8900_send(I2CSlave *i2c, uint8_t data)
> +{
> +    RX8900State *s = RX8900(i2c);
> +    struct tm now;
> +
> +    trace_rx8900_i2c_data_receive(data);
> +
> +    if (s->addr_byte) {
> +        s->nvram_offset = data & (RX8900_NVRAM_SIZE - 1);
> +        trace_rx8900_regptr_update(s->nvram_offset);
> +        s->addr_byte = false;
> +        return 0;
> +    }
> +
> +    trace_rx8900_set_register(s->nvram_offset, data);
> +
> +    qemu_get_timedate(&now, s->offset);
> +    switch (s->nvram_offset) {
> +    case SECONDS:
> +    case EXT_SECONDS:
> +        now.tm_sec = from_bcd(data & 0x7f);
> +        if (now.tm_sec > 59) { /* RX8900 does not support leap seconds */
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                "RX8900 - second data '%x' is out of range, "
> +                    "undefined behavior will result", data);
> +        }
> +        s->offset = qemu_timedate_diff(&now);
> +        break;
> +
> +    case MINUTES:
> +    case EXT_MINUTES:
> +        now.tm_min = from_bcd(data & 0x7f);
> +        if (now.tm_min > 59) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                "RX8900 - minute data '%x' is out of range, "
> +                    "undefined behavior will result", data);
> +        }
> +        s->offset = qemu_timedate_diff(&now);
> +        break;
> +
> +    case HOURS:
> +    case EXT_HOURS:
> +        now.tm_hour = from_bcd(data & 0x3f);
> +        if (now.tm_hour > 24) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                "RX8900 - hour data '%x' is out of range, "
> +                    "undefined behavior will result", data);
> +        }
> +        s->offset = qemu_timedate_diff(&now);
> +        break;
> +
> +    case WEEKDAY:
> +    case EXT_WEEKDAY: {
> +        int user_wday = ctz32(data);
> +        /* The day field is supposed to contain a value in
> +         * with only 1 of bits 0-6 set. Otherwise behavior is undefined.
> +         */
> +        switch (data) {
> +        case 0x01:
> +        case 0x02:
> +        case 0x04:
> +        case 0x08:
> +        case 0x10:
> +        case 0x20:
> +        case 0x40:
> +            break;
> +        default:
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                "RX8900 - weekday data '%x' is out of range, "
> +                        "undefined behavior will result", data);
> +            break;
> +        }

I think it might be clearer to do:

    if (data == 0x80 || ctpop8(data) != 1) {
        qemu_log_mask(...);
        break;
    }

> +        s->weekday = user_wday;
> +        break;
> +    }
> +
> +    case DAY:
> +    case EXT_DAY:
> +        now.tm_mday = from_bcd(data & 0x3f);
> +        s->offset = qemu_timedate_diff(&now);
> +        break;
> +
> +    case MONTH:
> +    case EXT_MONTH:
> +        now.tm_mon = from_bcd(data & 0x1f) - 1;
> +        s->offset = qemu_timedate_diff(&now);
> +        break;
> +
> +    case YEAR:
> +    case EXT_YEAR:
> +        now.tm_year = from_bcd(data) + 100;
> +        s->offset = qemu_timedate_diff(&now);
> +        break;
> +
> +    case EXTENSION_REGISTER:
> +    case EXT_EXTENSION_REGISTER:
> +        update_extension_register(s, data);
> +        break;
> +
> +    case FLAG_REGISTER:
> +    case EXT_FLAG_REGISTER:
> +        validate_flag_register(s, &data);
> +
> +        s->nvram[FLAG_REGISTER] = data;
> +        s->nvram[EXT_FLAG_REGISTER] = data;
> +
> +        check_voltage(s);
> +        break;
> +
> +    case CONTROL_REGISTER:
> +    case EXT_CONTROL_REGISTER:
> +        update_control_register(s, data);
> +        break;
> +
> +    default:
> +        s->nvram[s->nvram_offset] = data;
> +    }
> +
> +    inc_regptr(s);
> +    return 0;
> +}
> +
> +/**
> + * Get the device temperature in Celsius as a property
> + * @param obj the device
> + * @param v
> + * @param name the property name
> + * @param opaque
> + * @param errp an error object to populate on failure
> + */
> +static void rx8900_get_temperature(Object *obj, Visitor *v, const char *name,
> +                                   void *opaque, Error **errp)
> +{
> +    RX8900State *s = RX8900(obj);
> +    double value = (s->nvram[TEMPERATURE] * 2.0 - 187.1) / 3.218;

You have encode_temperature() below, should we not have
decode_temperature()?

> +
> +    trace_rx8900_get_temperature(s->nvram[TEMPERATURE], value);
> +
> +    visit_type_number(v, name, &value, errp);
> +}
> +
> +/**
> + * Encode a temperature in Celsius
> + * @param celsius the temperature
> + * @return the encoded temperature
> + */
> +static inline uint8_t encode_temperature(double celsius)
> +{
> +    return (uint8_t) ((celsius * 3.218 + 187.19) / 2);
> +}
> +
> +/**
> + * Set the device temperature in Celsius as a property
> + * @param obj the device
> + * @param v
> + * @param name the property name
> + * @param opaque
> + * @param errp an error object to populate on failure
> + */
> +static void rx8900_set_temperature(Object *obj, Visitor *v, const char *name,
> +                                   void *opaque, Error **errp)
> +{
> +    RX8900State *s = RX8900(obj);
> +    Error *local_err = NULL;
> +    double temp; /* degrees Celsius */
> +    visit_type_number(v, name, &temp, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +    if (temp >= 100 || temp < -58) {
> +        error_setg(errp, "value %fC is out of range", temp);
> +        return;
> +    }
> +
> +    s->nvram[TEMPERATURE] = encode_temperature(temp);
> +
> +    trace_rx8900_set_temperature(s->nvram[TEMPERATURE], temp);
> +}
> +
> +/**
> + * Get the device supply voltage as a property
> + * @param obj the device
> + * @param v
> + * @param name the property name
> + * @param opaque
> + * @param errp an error object to populate on failure
> + */
> +static void rx8900_get_voltage(Object *obj, Visitor *v, const char *name,
> +                                   void *opaque, Error **errp)
> +{
> +    RX8900State *s = RX8900(obj);
> +
> +    visit_type_number(v, name, &s->supply_voltage, errp);
> +}
> +
> +/**
> + * Set the device supply voltage as a property
> + * @param obj the device
> + * @param v
> + * @param name the property name
> + * @param opaque
> + * @param errp an error object to populate on failure
> + */
> +static void rx8900_set_voltage(Object *obj, Visitor *v, const char *name,
> +                                   void *opaque, Error **errp)
> +{
> +    RX8900State *s = RX8900(obj);
> +    Error *local_err = NULL;
> +    double temp;

Maybe voltage? It's ambiguous as to whether it's a copy/paste error
from 'temperature' or if you mean 'temporary'...

> +    visit_type_number(v, name, &temp, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    s->supply_voltage = temp;

... as this almost looks like a bug.

> +    trace_rx8900_set_voltage(s->supply_voltage);
> +
> +    check_voltage(s);
> +}
> +
> +
> +/**
> + * Configure device properties
> + * @param obj the device
> + */
> +static void rx8900_initfn(Object *obj)
> +{
> +    object_property_add(obj, "temperature", "number",
> +                        rx8900_get_temperature,
> +                        rx8900_set_temperature, NULL, NULL, NULL);
> +
> +    object_property_add(obj, "voltage", "number",
> +                        rx8900_get_voltage,
> +                        rx8900_set_voltage, NULL, NULL, NULL);
> +}
> +
> +/**
> + * Reset the device
> + * @param dev the RX8900 device to reset
> + */
> +static void rx8900_reset(DeviceState *dev)

Is there a reason we can't define rx8900_reset() further up the file to
avoid forward-declaring it?

> +{
> +    RX8900State *s = RX8900(dev);
> +
> +    trace_rx8900_reset();
> +
> +    /* The clock is running and synchronized with the host */
> +    s->offset = 0;
> +    s->weekday = 7; /* Set to an invalid value */

Can you explain why we do that rather than just stating that 7 is
invalid?

> +
> +    s->nvram[EXTENSION_REGISTER] = EXT_MASK_TSEL1;
> +    s->nvram[CONTROL_REGISTER] = CTRL_MASK_CSEL0;
> +    s->nvram[FLAG_REGISTER] &= FLAG_MASK_VDET | FLAG_MASK_VLF;
> +
> +    s->nvram_offset = 0;
> +
> +    trace_rx8900_regptr_update(s->nvram_offset);
> +
> +    s->addr_byte = false;
> +}
> +
> +/**
> + * Realize an RX8900 device instance
> + * Set up timers
> + * Configure GPIO lines
> + * @param dev the device instance to realize
> + * @param errp an error object to populate on error
> + */
> +static void rx8900_realize(DeviceState *dev, Error **errp)
> +{
> +    RX8900State *s = RX8900(dev);
> +    I2CSlave *i2c = I2C_SLAVE(dev);
> +    QEMUBH *bh;
> +    char name[64];
> +
> +    s->fout_state = false;
> +
> +    memset(s->nvram, 0, RX8900_NVRAM_SIZE);
> +    /* Set the initial state to 25 degrees Celsius */
> +    s->nvram[TEMPERATURE] = encode_temperature(25.0);
> +
> +    /* Set up timers */
> +    bh = qemu_bh_new(rx8900_timer_tick, s);
> +    s->sec_timer = ptimer_init(bh, PTIMER_POLICY_DEFAULT);
> +    /* we trigger the timer at 10Hz and check for rollover, as the qemu
> +     * clock does not advance in realtime in the test environment,
> +     * leading to unstable test results
> +     */

It seems unfortunate that some of the code seems dedicated to working
around test suite issues (e.g. early return in some functions). Ah
well.

> +    ptimer_set_freq(s->sec_timer, 10);
> +    ptimer_set_limit(s->sec_timer, 1, 1);
> +
> +    bh = qemu_bh_new(rx8900_fout_tick, s);
> +    s->fout_timer = ptimer_init(bh, PTIMER_POLICY_DEFAULT);
> +    /* frequency doubled to generate 50% duty cycle square wave */
> +    ptimer_set_freq(s->fout_timer, 32768 * 2);
> +    ptimer_set_limit(s->fout_timer, 1, 1);
> +
> +    bh = qemu_bh_new(rx8900_countdown_tick, s);
> +    s->countdown_timer = ptimer_init(bh, PTIMER_POLICY_DEFAULT);
> +    ptimer_set_freq(s->countdown_timer, COUNTDOWN_TIMER_FREQ);
> +    ptimer_set_limit(s->countdown_timer, COUNTDOWN_TIMER_FREQ, 1);
> +
> +
> +    /* set up GPIO */
> +    snprintf(name, sizeof(name), "rx8900-interrupt-out");

Why the snprintf()s?

> +    qdev_init_gpio_out_named(&i2c->qdev, &s->interrupt_pin, name, 1);
> +    trace_rx8900_pin_name("Interrupt", name);
> +
> +    snprintf(name, sizeof(name), "rx8900-fout-enable");
> +    qdev_init_gpio_in_named(&i2c->qdev, rx8900_fout_enable_handler, name, 1);
> +    trace_rx8900_pin_name("Fout-enable", name);
> +
> +    snprintf(name, sizeof(name), "rx8900-fout");
> +    qdev_init_gpio_out_named(&i2c->qdev, &s->fout_pin, name, 1);
> +    trace_rx8900_pin_name("Fout", name);
> +
> +    /* Set up default voltage */
> +    s->supply_voltage = 3.3f;
> +    trace_rx8900_set_voltage(s->supply_voltage);
> +}
> +
> +/**
> + * Set up the device callbacks
> + * @param klass the device class
> + * @param data
> + */
> +static void rx8900_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    I2CSlaveClass *k = I2C_SLAVE_CLASS(klass);
> +
> +    k->event = rx8900_event;
> +    k->recv = rx8900_recv;
> +    k->send = rx8900_send;
> +    dc->realize = rx8900_realize;
> +    dc->reset = rx8900_reset;
> +    dc->vmsd = &vmstate_rx8900;
> +}
> +
> +static const TypeInfo rx8900_info = {
> +    .name = TYPE_RX8900,
> +    .parent = TYPE_I2C_SLAVE,
> +    .instance_size = sizeof(RX8900State),
> +    .instance_init = rx8900_initfn,
> +    .class_init = rx8900_class_init,
> +};
> +
> +/**
> + * Register the device with QEMU
> + */
> +static void rx8900_register_types(void)
> +{
> +    type_register_static(&rx8900_info);
> +}
> +
> +type_init(rx8900_register_types)
> diff --git a/hw/timer/rx8900_regs.h b/hw/timer/rx8900_regs.h
> new file mode 100644
> index 0000000..0aaa9a3
> --- /dev/null
> +++ b/hw/timer/rx8900_regs.h
> @@ -0,0 +1,141 @@
> +/*
> + * Epson RX8900SA/CE Realtime Clock Module
> + *
> + * Copyright (c) 2016 IBM Corporation
> + * Authors:
> > + *  Alastair D'Silva <alastair@d-silva.org>
> + *
> + * This code is licensed under the GPL version 2 or later.  See
> + * the COPYING file in the top-level directory.
> + *
> + * Datasheet available at:
> + *  https://support.epson.biz/td/api/doc_check.php?dl=app_RX8900CE&lang=en
> + *
> + */
> +
> +#ifndef RX8900_REGS_H
> +#define RX8900_REGS_H
> +
> +#include "qemu/bitops.h"
> +
> +#define RX8900_NVRAM_SIZE 0x20

Do all the enums below need to be defined in the header or are they
only relevant to the implementation? If it's the latter, then they
should go in the .c file.

Also I would've reached for #define for all these values, not enums,
but that might be a personal thing.

> +
> +typedef enum RX8900Addresses {
> +    START_ADDRESS = 0x00,
> +    SECONDS = 0x00,
> +    MINUTES = 0x01,
> +    HOURS = 0x02,
> +    WEEKDAY = 0x03, /* a walking bit with bit 0 set for Sunday, 1 for Monday...
> +                       6 for Saturday, see the datasheet for details */
> +    DAY = 0x04,
> +    MONTH = 0x05,
> +    YEAR = 0x06,
> +    RAM = 0x07,
> +    ALARM_MINUTE = 0x08,
> +    ALARM_HOUR = 0x09,
> +    ALARM_WEEK_DAY = 0x0A,
> +    TIMER_COUNTER_0 = 0x0B,
> +    TIMER_COUNTER_1 = 0x0C,
> +    EXTENSION_REGISTER = 0x0D,
> +    FLAG_REGISTER = 0X0E,
> +    CONTROL_REGISTER = 0X0F,
> +    EXT_SECONDS = 0x010, /* Alias of SECONDS */
> +    EXT_MINUTES = 0x11, /* Alias of MINUTES */
> +    EXT_HOURS = 0x12, /* Alias of HOURS */
> +    EXT_WEEKDAY = 0x13, /* Alias of WEEKDAY */
> +    EXT_DAY = 0x14, /* Alias of DAY */
> +    EXT_MONTH = 0x15, /* Alias of MONTH */
> +    EXT_YEAR = 0x16, /* Alias of YEAR */
> +    TEMPERATURE = 0x17,
> +    BACKUP_FUNCTION = 0x18,
> +    NO_USE_1 = 0x19,
> +    NO_USE_2 = 0x1A,
> +    EXT_TIMER_COUNTER_0 = 0x1B, /* Alias of TIMER_COUNTER_0 */
> +    EXT_TIMER_COUNTER_1 = 0x1C, /* Alias of TIMER_COUNTER_1 */
> +    EXT_EXTENSION_REGISTER = 0x1D, /* Alias of EXTENSION_REGISTER */
> +    EXT_FLAG_REGISTER = 0X1E, /* Alias of FLAG_REGISTER */
> +    EXT_CONTROL_REGISTER = 0X1F /* Alias of CONTROL_REGISTER */
> +} RX8900Addresses;
> +
> +typedef enum ExtRegBits {
> +    EXT_REG_TSEL0 = 0,
> +    EXT_REG_TSEL1 = 1,
> +    EXT_REG_FSEL0 = 2,
> +    EXT_REG_FSEL1 = 3,
> +    EXT_REG_TE = 4,
> +    EXT_REG_USEL = 5,
> +    EXT_REG_WADA = 6,
> +    EXT_REG_TEST = 7
> +} ExtRegBits;
> +
> +typedef enum ExtRegMasks {
> +    EXT_MASK_TSEL0 = BIT(0),
> +    EXT_MASK_TSEL1 = BIT(1),
> +    EXT_MASK_FSEL0 = BIT(2),
> +    EXT_MASK_FSEL1 = BIT(3),
> +    EXT_MASK_TE = BIT(4),
> +    EXT_MASK_USEL = BIT(5),
> +    EXT_MASK_WADA = BIT(6),
> +    EXT_MASK_TEST = BIT(7)
> +} ExtRegMasks;
> +
> +typedef enum CtrlRegBits {
> +    CTRL_REG_RESET = 0,
> +    CTRL_REG_WP0 = 1,
> +    CTRL_REG_WP1 = 2,
> +    CTRL_REG_AIE = 3,
> +    CTRL_REG_TIE = 4,
> +    CTRL_REG_UIE = 5,
> +    CTRL_REG_CSEL0 = 6,
> +    CTRL_REG_CSEL1 = 7
> +} CtrlRegBits;
> +
> +typedef enum CtrlRegMask {
> +    CTRL_MASK_RESET = BIT(0),
> +    CTRL_MASK_WP0 = BIT(1),
> +    CTRL_MASK_WP1 = BIT(2),
> +    CTRL_MASK_AIE = BIT(3),
> +    CTRL_MASK_TIE = BIT(4),
> +    CTRL_MASK_UIE = BIT(5),
> +    CTRL_MASK_CSEL0 = BIT(6),
> +    CTRL_MASK_CSEL1 = BIT(7)
> +} CtrlRegMask;
> +
> +typedef enum FlagRegBits {
> +    FLAG_REG_VDET = 0,
> +    FLAG_REG_VLF = 1,
> +    FLAG_REG_UNUSED_2 = 2,
> +    FLAG_REG_AF = 3,
> +    FLAG_REG_TF = 4,
> +    FLAG_REG_UF = 5,
> +    FLAG_REG_UNUSED_6 = 6,
> +    FLAG_REG_UNUSED_7 = 7
> +} FlagRegBits;
> +
> +#define RX8900_INTERRUPT_SOURCES 6
> +typedef enum FlagRegMask {
> +    FLAG_MASK_VDET = BIT(0),
> +    FLAG_MASK_VLF = BIT(1),
> +    FLAG_MASK_UNUSED_2 = BIT(2),
> +    FLAG_MASK_AF = BIT(3),
> +    FLAG_MASK_TF = BIT(4),
> +    FLAG_MASK_UF = BIT(5),
> +    FLAG_MASK_UNUSED_6 = BIT(6),
> +    FLAG_MASK_UNUSED_7 = BIT(7)
> +} FlagRegMask;
> +
> +typedef enum BackupRegBits {
> +    BACKUP_REG_BKSMP0 = 0,
> +    BACKUP_REG_BKSMP1 = 1,
> +    BACKUP_REG_SWOFF = 2,
> +    BACKUP_REG_VDETOFF = 3
> +} BackupRegBits;
> +
> +typedef enum BackupRegMask {
> +    BACKUP_MASK_BKSMP0 = BIT(0),
> +    BACKUP_MASK_BKSMP1 = BIT(1),
> +    BACKUP_MASK_SWOFF = BIT(2),
> +    BACKUP_MASK_VDETOFF = BIT(3)
> +} BackupRegMask;
> +
> +#endif
> diff --git a/hw/timer/trace-events b/hw/timer/trace-events
> index 3495c41..8f4969b 100644
> --- a/hw/timer/trace-events
> +++ b/hw/timer/trace-events
> @@ -49,3 +49,34 @@ aspeed_timer_ctrl_pulse_enable(uint8_t i, bool enable) "Timer %" PRIu8 ": %d"
>  aspeed_timer_set_ctrl2(uint32_t value) "Value: 0x%" PRIx32
>  aspeed_timer_set_value(int timer, int reg, uint32_t value) "Timer %d register %d: 0x%" PRIx32
>  aspeed_timer_read(uint64_t offset, unsigned size, uint64_t value) "From 0x%" PRIx64 ": of size %u: 0x%" PRIx64
> +
> +# hw/timer/rx8900.c
> +rx8900_capture_current_time(int hour, int minute, int second, int weekday, int mday, int month, int year, int raw_hours, int raw_minutes, int raw_seconds, int raw_weekday, int raw_day, int raw_month, int raw_year) "Update current time to %02d:%02d:%02d %d %d/%d/%d (0x%02x%02x%02x%02x%02x%02x%02x)"
> +rx8900_regptr_update(uint32_t ptr) "Operating on register 0x%02x"
> +rx8900_regptr_overflow(void) "Register pointer has overflowed, wrapping to 0"
> +rx8900_event_weekday(int weekday, int weekmask, int weekday_offset) "Set weekday to %d (0x%02x), wday_offset=%d"
> +rx8900_read_register(int address, int val) "Read register 0x%x = 0x%x"
> +rx8900_set_fout(int hz) "Setting fout to %dHz"
> +rx8900_set_countdown_timer(int hz) "Setting countdown timer to %d Hz"
> +rx8900_set_countdown_timer_per_minute(void) "Setting countdown timer to per minute updates"
> +rx8900_enable_update_timer(void) ""
> +rx8900_enable_alarm(void) ""
> +rx8900_trigger_alarm(void) ""
> +rx8900_tick(void) ""
> +rx8900_fire_interrupt(void) ""
> +rx8900_disable_timer(void) ""
> +rx8900_enable_timer(void) ""
> +rx8900_disable_fout(void) ""
> +rx8900_enable_fout(void) ""
> +rx8900_fout_toggle(void) ""
> +rx8900_disable_countdown_timer(void) ""
> +rx8900_enable_countdown_timer(void) ""
> +rx8900_countdown_tick(int count) "Countdown tick, count=%d"
> +rx8900_countdown_elapsed(void) ""
> +rx8900_i2c_data_receive(uint8_t data) "Received I2C data 0x%02x"
> +rx8900_set_register(uint32_t addr, uint8_t data) "Set data 0x%02x=0x%02x"
> +rx8900_get_temperature(uint8_t raw, double val) "0x%x = %fC"
> +rx8900_set_temperature(uint8_t raw, double val) "0x%x = %fC"
> +rx8900_reset(void) ""
> +rx8900_pin_name(const char *type, const char *name) "'%s' pin is '%s'"
> +rx8900_set_voltage(double voltage) "Device voltage set to %f"

Is there a justification for all the empty traces?

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

^ permalink raw reply

* Re: [Qemu-devel] [PATCH v4 6/8] hw/timer: Add Epson RX8900 RTC support
From: Andrew Jeffery @ 2017-01-04  4:59 UTC (permalink / raw)
  To: Alastair D'Silva, qemu-arm
  Cc: qemu-devel, Peter Maydell, Cédric Le Goater, Joel Stanley,
	Alastair D'Silva, Chris Smart
In-Reply-To: <20161215054812.12602-7-alastair@au1.ibm.com>

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

Hi Alastair,

I have some mostly minor comments below.

On Thu, 2016-12-15 at 16:48 +1100, Alastair D'Silva wrote:
> > From: Alastair D'Silva <alastair@d-silva.org>
> 
> This patch adds support for the Epson RX8900 I2C RTC.
> 
> The following chip features are implemented:
>  - RTC (wallclock based, ptimer 10x oversampling to pick up
>         wallclock transitions)
>  - Time update interrupt (per second/minute, wallclock based)
>  - Alarms (wallclock based)
>  - Temperature (set via a property)
>  - Countdown timer (emulated clock via ptimer)
>  - FOUT via GPIO (emulated clock via ptimer)
> 
> The following chip features are unimplemented:
>  - Low voltage detection
>  - i2c timeout
> 
> The implementation exports the following named GPIOs:
> rx8900-interrupt-out
> rx8900-fout-enable
> rx8900-fout
> 
> > Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> > Signed-off-by: Chris Smart <chris@distroguy.com>
> ---
>  default-configs/arm-softmmu.mak |   1 +
>  hw/timer/Makefile.objs          |   2 +
>  hw/timer/rx8900.c               | 912 ++++++++++++++++++++++++++++++++++++++++
>  hw/timer/rx8900_regs.h          | 141 +++++++
>  hw/timer/trace-events           |  31 ++
>  5 files changed, 1087 insertions(+)
>  create mode 100644 hw/timer/rx8900.c
>  create mode 100644 hw/timer/rx8900_regs.h
> 
> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
> index 6de3e16..adb600e 100644
> --- a/default-configs/arm-softmmu.mak
> +++ b/default-configs/arm-softmmu.mak
> @@ -29,6 +29,7 @@ CONFIG_SMC91C111=y
>  CONFIG_ALLWINNER_EMAC=y
>  CONFIG_IMX_FEC=y
>  CONFIG_DS1338=y
> +CONFIG_RX8900=y
>  CONFIG_PFLASH_CFI01=y
>  CONFIG_PFLASH_CFI02=y
>  CONFIG_MICRODRIVE=y
> diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs
> index 7ba8c23..fa028ac 100644
> --- a/hw/timer/Makefile.objs
> +++ b/hw/timer/Makefile.objs
> @@ -3,6 +3,7 @@ common-obj-$(CONFIG_ARM_MPTIMER) += arm_mptimer.o
>  common-obj-$(CONFIG_A9_GTIMER) += a9gtimer.o
>  common-obj-$(CONFIG_CADENCE) += cadence_ttc.o
>  common-obj-$(CONFIG_DS1338) += ds1338.o
> +common-obj-$(CONFIG_RX8900) += rx8900.o
>  common-obj-$(CONFIG_HPET) += hpet.o
>  common-obj-$(CONFIG_I8254) += i8254_common.o i8254.o
>  common-obj-$(CONFIG_M48T59) += m48t59.o
> @@ -17,6 +18,7 @@ common-obj-$(CONFIG_IMX) += imx_epit.o
>  common-obj-$(CONFIG_IMX) += imx_gpt.o
>  common-obj-$(CONFIG_LM32) += lm32_timer.o
>  common-obj-$(CONFIG_MILKYMIST) += milkymist-sysctl.o
> +common-obj-$(CONFIG_RX8900) += rx8900.o
>  
>  obj-$(CONFIG_EXYNOS4) += exynos4210_mct.o
>  obj-$(CONFIG_EXYNOS4) += exynos4210_pwm.o
> diff --git a/hw/timer/rx8900.c b/hw/timer/rx8900.c
> new file mode 100644
> index 0000000..cb1a2c8
> --- /dev/null
> +++ b/hw/timer/rx8900.c
> @@ -0,0 +1,912 @@
> +/*
> + * Epson RX8900SA/CE Realtime Clock Module
> + *
> + * Copyright (c) 2016 IBM Corporation
> + * Authors:
> > + *  Alastair D'Silva <alastair@d-silva.org>
> > + *  Chris Smart <chris@distroguy.com>
> + *
> + * This code is licensed under the GPL version 2 or later.  See
> + * the COPYING file in the top-level directory.
> + *
> + * Datasheet available at:
> + *  https://support.epson.biz/td/api/doc_check.php?dl=app_RX8900CE&lang=en
> + *
> + * Not implemented:
> + *  Implement i2c timeout
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu-common.h"
> +#include "hw/i2c/i2c.h"
> +#include "hw/timer/rx8900_regs.h"
> +#include "hw/ptimer.h"
> +#include "qemu/main-loop.h"
> +#include "qemu/bcd.h"
> +#include "qemu/log.h"
> +#include "qapi/error.h"
> +#include "qapi/visitor.h"
> +#include "trace.h"
> +
> +#define TYPE_RX8900 "rx8900"
> +#define RX8900(obj) OBJECT_CHECK(RX8900State, (obj), TYPE_RX8900)
> +
> +typedef struct RX8900State {
> +    I2CSlave parent_obj;
> +
> +    ptimer_state *sec_timer; /* triggered once per second */
> +    ptimer_state *fout_timer;
> +    ptimer_state *countdown_timer;
> +    bool fout_state;
> +    int64_t offset;
> +    uint8_t weekday; /* Saved for deferred offset calculation, 0-6 */
> +    uint8_t wday_offset;
> +    uint8_t nvram[RX8900_NVRAM_SIZE];
> +    int32_t nvram_offset; /* Wrapped to stay within RX8900_NVRAM_SIZE */
> +    bool addr_byte;
> +    uint8_t last_interrupt_seconds; /* The last time the second timer ticked */
> +    /* the last minute the timer update interrupt was triggered (if enabled) */
> +    uint8_t last_update_interrupt_minutes;
> +    double supply_voltage;
> +    qemu_irq interrupt_pin;
> +    qemu_irq fout_pin;
> +} RX8900State;
> +
> +static const VMStateDescription vmstate_rx8900 = {
> +    .name = "rx8900",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_I2C_SLAVE(parent_obj, RX8900State),
> +        VMSTATE_PTIMER(sec_timer, RX8900State),
> +        VMSTATE_PTIMER(fout_timer, RX8900State),
> +        VMSTATE_PTIMER(countdown_timer, RX8900State),
> +        VMSTATE_BOOL(fout_state, RX8900State),
> +        VMSTATE_INT64(offset, RX8900State),
> +        VMSTATE_UINT8(weekday, RX8900State),
> +        VMSTATE_UINT8(wday_offset, RX8900State),
> +        VMSTATE_UINT8_ARRAY(nvram, RX8900State, RX8900_NVRAM_SIZE),
> +        VMSTATE_INT32(nvram_offset, RX8900State),
> +        VMSTATE_BOOL(addr_byte, RX8900State),
> +        VMSTATE_UINT8(last_interrupt_seconds, RX8900State),
> +        VMSTATE_UINT8(last_update_interrupt_minutes, RX8900State),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static void rx8900_reset(DeviceState *dev);
> +
> +static void capture_current_time(RX8900State *s)
> +{
> +    /* Capture the current time into the secondary registers
> +     * which will be actually read by the data transfer operation.
> +     */
> +    struct tm now;
> +    qemu_get_timedate(&now, s->offset);
> +    s->nvram[SECONDS] = to_bcd(now.tm_sec);
> +    s->nvram[MINUTES] = to_bcd(now.tm_min);
> +    s->nvram[HOURS] = to_bcd(now.tm_hour);
> +
> +    s->nvram[WEEKDAY] = 0x01 << ((now.tm_wday + s->wday_offset) % 7);
> +    s->nvram[DAY] = to_bcd(now.tm_mday);
> +    s->nvram[MONTH] = to_bcd(now.tm_mon + 1);
> +    s->nvram[YEAR] = to_bcd(now.tm_year % 100);
> +
> +    s->nvram[EXT_SECONDS] = s->nvram[SECONDS];
> +    s->nvram[EXT_MINUTES] = s->nvram[MINUTES];
> +    s->nvram[EXT_HOURS] = s->nvram[HOURS];
> +    s->nvram[EXT_WEEKDAY] = s->nvram[WEEKDAY];
> +    s->nvram[EXT_DAY] = s->nvram[DAY];
> +    s->nvram[EXT_MONTH] = s->nvram[MONTH];
> +    s->nvram[EXT_YEAR] = s->nvram[YEAR];
+
> +    trace_rx8900_capture_current_time(now.tm_hour, now.tm_min, now.tm_sec,
> +            (now.tm_wday + s->wday_offset) % 7,
> +            now.tm_mday, now.tm_mon, now.tm_year + 1900,
> +            s->nvram[HOURS], s->nvram[MINUTES], s->nvram[SECONDS],
> +            s->nvram[WEEKDAY], s->nvram[DAY], s->nvram[MONTH], s->nvram[YEAR]);
> +}
> +
> +/**
> + * Increment the internal register pointer, dealing with wrapping
> + * @param s the RTC to operate on
> + */
> +static void inc_regptr(RX8900State *s)
> +{
> +    /* The register pointer wraps around after 0x1F
> +     */
> +    s->nvram_offset = (s->nvram_offset + 1) & (RX8900_NVRAM_SIZE - 1);
> +    trace_rx8900_regptr_update(s->nvram_offset);
> +
> +    if (s->nvram_offset == START_ADDRESS) {
> +        trace_rx8900_regptr_overflow();
> +        capture_current_time(s);
> +    }
> +}
> +
> +#define INVALID_WEEKDAY 0xff
> +
> +/**
> + * Receive an I2C Event
> + * @param i2c the i2c device instance
> + * @param event the event to handle
> + */
> +static void rx8900_event(I2CSlave *i2c, enum i2c_event event)
> +{
> +    RX8900State *s = RX8900(i2c);
> +
> +    switch (event) {
> +    case I2C_START_RECV:
> +        /* In h/w, time capture happens on any START condition, not just a
> +         * START_RECV. For the emulation, it doesn't actually matter,
> +         * since a START_RECV has to occur before the data can be read.
> +         */
> +        capture_current_time(s);
> +        break;
> +    case I2C_START_SEND:
> +        s->addr_byte = true;
> +        break;
> +    case I2C_FINISH:
> +        if (s->weekday < 7) {
> +            /* We defer the weekday calculation as it is handed to us before
> +             * the date has been updated. If we calculate the weekday offset
> +             * when it is passed to us, we will incorrectly determine it
> +             * based on the current emulated date, rather than the date that
> +             * has been written.
> +             */
> +            struct tm now;
> +            qemu_get_timedate(&now, s->offset);
> +
> +            s->wday_offset = (s->weekday - now.tm_wday + 7) % 7;
> +
> +            trace_rx8900_event_weekday(s->weekday, BIT(s->weekday),
> +                    s->wday_offset);
> +
> +            s->weekday = INVALID_WEEKDAY;
> +        }
> +        break;
> +
> +    default:
> +        break;
> +    }
> +}
> +
> +/**
> + * Perform an i2c receive action
> + * @param i2c the i2c device instance
> + * @return the value of the current register
> + * @post the internal register pointer is incremented
> + */
> +static int rx8900_recv(I2CSlave *i2c)
> +{
> +    RX8900State *s = RX8900(i2c);
> +    uint8_t res = s->nvram[s->nvram_offset];
> +    trace_rx8900_read_register(s->nvram_offset, res);
> +    inc_regptr(s);
> +    return res;
> +}
> +
> +/**
> + * Disable the countdown timer
> + * @param s the RTC to operate on
> + */
> +static void disable_countdown_timer(RX8900State *s)
> +{
> +    trace_rx8900_disable_countdown_timer();
> +    ptimer_stop(s->countdown_timer);
> +}
> +
> +/**
> + * Enable the countdown timer
> + * @param s the RTC to operate on
> + */
> +static void enable_countdown_timer(RX8900State *s)
> +{
> +    trace_rx8900_enable_countdown_timer();
> +    ptimer_run(s->countdown_timer, 0);
> +}
> +
> +/**
> + * Tick the countdown timer
> + * @param opaque the device instance
> + */
> +static void rx8900_countdown_tick(void *opaque)
> +{
> +    RX8900State *s = (RX8900State *)opaque;
> +
> +    uint16_t count = s->nvram[TIMER_COUNTER_0] |
> +            ((s->nvram[TIMER_COUNTER_1] & 0x0F) << 8);
> +    trace_rx8900_countdown_tick(count);
> +    count--;
> +
> +    s->nvram[TIMER_COUNTER_0] = (uint8_t)(count & 0x00ff);
> +    s->nvram[TIMER_COUNTER_1] = (uint8_t)((count & 0x0f00) >> 8);
> +
> +    if (count == 0) {
> +        trace_rx8900_countdown_elapsed();
> +
> +        disable_countdown_timer(s);
> +
> +        s->nvram[FLAG_REGISTER] |= FLAG_MASK_TF;
> +
> +        if (s->nvram[CONTROL_REGISTER] & CTRL_MASK_TIE) {
> +            trace_rx8900_fire_interrupt();
> +            qemu_irq_pulse(s->interrupt_pin);
> +        }
> +    }
> +}
> +
> +/**
> + * Disable the per second timer
> + * @param s the RTC to operate on
> + */
> +static void disable_timer(RX8900State *s)
> +{
> +    trace_rx8900_disable_timer();
> +    ptimer_stop(s->sec_timer);
> +}
> +
> +/**
> + * Enable the per second timer
> + * @param s the RTC to operate on
> + */
> +static void enable_timer(RX8900State *s)
> +{
> +    trace_rx8900_enable_timer();
> +    ptimer_run(s->sec_timer, 0);
> +}
> +
> +/**
> + * Tick the per second timer (can be called more frequently as it early exits
> + * if the wall clock has not progressed)
> + * @param opaque the RTC to tick
> + */
> +static void rx8900_timer_tick(void *opaque)
> +{
> +    RX8900State *s = (RX8900State *)opaque;
> +    struct tm now;
> +    bool fire_interrupt = false;
> +    bool alarm_week_day_matches;
> +
> +    qemu_get_timedate(&now, s->offset);
> +
> +    if (now.tm_sec == s->last_interrupt_seconds) {
> +        return;
> +    }
> +
> +    s->last_interrupt_seconds = now.tm_sec;
> +
> +    trace_rx8900_tick();
> +
> +    /* Update timer interrupt */
> +    if (s->nvram[CONTROL_REGISTER] & CTRL_MASK_UIE) {
> +        if ((s->nvram[EXTENSION_REGISTER] & EXT_MASK_USEL) &&
> +                now.tm_min != s->last_update_interrupt_minutes) {
> +            s->last_update_interrupt_minutes = now.tm_min;
> +            s->nvram[FLAG_REGISTER] |= FLAG_MASK_UF;
> +            fire_interrupt = true;
> +        } else if (!(s->nvram[EXTENSION_REGISTER] & EXT_MASK_USEL)) {
> +            /* per second update interrupt */
> +            s->nvram[FLAG_REGISTER] |= FLAG_MASK_UF;
> +            fire_interrupt = true;
> +        }
> +    }
> +
> +    /* Alarm interrupt */
> +    if ((s->nvram[EXTENSION_REGISTER] & EXT_MASK_WADA)) {
> +        alarm_week_day_matches =
> +            s->nvram[ALARM_WEEK_DAY] == to_bcd(now.tm_mday);
> +    } else {
> +        alarm_week_day_matches =
> +            s->nvram[ALARM_WEEK_DAY] ==
> +                0x01 << ((now.tm_wday + s->wday_offset) % 7);
> +    }
> +
> +    if ((s->nvram[CONTROL_REGISTER] & CTRL_MASK_AIE) && now.tm_sec == 0 &&
> +            s->nvram[ALARM_MINUTE] == to_bcd(now.tm_min) &&
> +            s->nvram[ALARM_HOUR] == to_bcd(now.tm_hour) &&
> +            alarm_week_day_matches) {
> +        trace_rx8900_trigger_alarm();
> +        s->nvram[FLAG_REGISTER] |= FLAG_MASK_AF;
> +        fire_interrupt = true;
> +    }
> +
> +    if (fire_interrupt) {
> +        trace_rx8900_fire_interrupt();
> +        qemu_irq_pulse(s->interrupt_pin);
> +    }
> +}
> +
> +
> +#define COUNTDOWN_TIMER_FREQ 4096
> +
> +/**
> + * Validate the extension register and perform actions based on the bits
> + * @param s the RTC to operate on
> + * @param data the new data for the extension register
> + */
> +static void update_extension_register(RX8900State *s, uint8_t data)
> +{
> +    if (data & EXT_MASK_TEST) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                "Test bit is enabled but is forbidden by the manufacturer");
> +    }
> +
> +    if ((data ^ s->nvram[EXTENSION_REGISTER]) &
> +            (EXT_MASK_FSEL0 | EXT_MASK_FSEL1)) {
> +        /* FSELx has changed */
> +
> +        switch (data & (EXT_MASK_FSEL0 | EXT_MASK_FSEL1)) {
> +        case EXT_MASK_FSEL0:
> +            trace_rx8900_set_fout(1024);
> +            ptimer_set_limit(s->fout_timer, 32, 1);
> +            break;
> +        case EXT_MASK_FSEL1:
> +            trace_rx8900_set_fout(1);
> +            ptimer_set_limit(s->fout_timer, 32768, 1);
> +            break;
> +        case 0:
> +        case (EXT_MASK_FSEL0 | EXT_MASK_FSEL1):
> +            trace_rx8900_set_fout(32768);
> +            ptimer_set_limit(s->fout_timer, 1, 1);
> +            break;
> +        }
> +    }
> +
> +    if ((data ^ s->nvram[EXTENSION_REGISTER]) &
> +            (EXT_MASK_TSEL0 | EXT_MASK_TSEL1)) {
> +        /* TSELx has changed */
> +        switch (data & (EXT_MASK_TSEL0 | EXT_MASK_TSEL1)) {
> +        case 0:
> +            trace_rx8900_set_countdown_timer(64);
> +            ptimer_set_limit(s->countdown_timer, COUNTDOWN_TIMER_FREQ / 64, 1);
> +            break;
> +        case EXT_MASK_TSEL0:
> +            trace_rx8900_set_countdown_timer(1);
> +            ptimer_set_limit(s->countdown_timer, COUNTDOWN_TIMER_FREQ, 1);
> +            break;
> +        case EXT_MASK_TSEL1:
> +            trace_rx8900_set_countdown_timer_per_minute();
> +            ptimer_set_limit(s->countdown_timer, COUNTDOWN_TIMER_FREQ * 60, 1);
> +            break;
> +        case (EXT_MASK_TSEL0 | EXT_MASK_TSEL1):
> +            trace_rx8900_set_countdown_timer(COUNTDOWN_TIMER_FREQ);
> +            ptimer_set_limit(s->countdown_timer, 1, 1);
> +            break;
> +        }
> +    }
> +
> +    if (data & EXT_MASK_TE) {
> +        enable_countdown_timer(s);
> +    }
> +
> +    s->nvram[EXTENSION_REGISTER] = data;
> +    s->nvram[EXT_EXTENSION_REGISTER] = data;
> +
> +}
> +/**
> + * Validate the control register and perform actions based on the bits
> + * @param s the RTC to operate on
> + * @param data the new value for the control register
> + */
> +
> +static void update_control_register(RX8900State *s, uint8_t data)
> +{
> +    uint8_t diffmask = ~s->nvram[CONTROL_REGISTER] & data;
> +
> +    if (diffmask & CTRL_MASK_WP0) {
> +        data &= ~CTRL_MASK_WP0;
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +            "Attempt to write to write protected bit %d in control register",
> +            CTRL_REG_WP0);
> +    }
> +
> +    if (diffmask & CTRL_MASK_WP1) {
> +        data &= ~CTRL_MASK_WP1;
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +            "Attempt to write to write protected bit %d in control register",
> +            CTRL_REG_WP1);
> +    }
> +
> +    if (data & CTRL_MASK_RESET) {
> +        data &= ~CTRL_MASK_RESET;
> +        rx8900_reset(DEVICE(s));
> +    }
> +
> +    if (diffmask & CTRL_MASK_UIE) {
> +        /* Update interrupts were off and are now on */
> +        struct tm now;
> +
> +        trace_rx8900_enable_update_timer();
> +
> +        qemu_get_timedate(&now, s->offset);
> +
> +        s->last_update_interrupt_minutes = now.tm_min;
> +        s->last_interrupt_seconds = now.tm_sec;
> +        enable_timer(s);
> +    }
> +
> +    if (diffmask & CTRL_MASK_AIE) {
> +        /* Alarm interrupts were off and are now on */
> +        struct tm now;
> +
> +        trace_rx8900_enable_alarm();
> +
> +        qemu_get_timedate(&now, s->offset);
> +
> +        s->last_interrupt_seconds = now.tm_sec;
> +        enable_timer(s);
> +    }
> +
> +    if (!(data & (CTRL_MASK_UIE | CTRL_MASK_AIE))) {
> +        disable_timer(s);
> +    }
> +
> +    s->nvram[CONTROL_REGISTER] = data;
> +    s->nvram[EXT_CONTROL_REGISTER] = data;
> +}
> +
> +/**
> + * Validate the flag register
> + * @param s the RTC to operate on
> + * @param data the new value for the flag register
> + */
> +static void validate_flag_register(RX8900State *s, uint8_t *data)
> +{
> +    uint8_t diffmask = ~s->nvram[FLAG_REGISTER] & *data;
> +
> +    if (diffmask & FLAG_MASK_VDET) {
> +        *data &= ~FLAG_MASK_VDET;
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +            "Only 0 can be written to VDET bit %d in the flag register",
> +            FLAG_REG_VDET);
> +    }
> +
> +    if (diffmask & FLAG_MASK_VLF) {
> +        *data &= ~FLAG_MASK_VLF;
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +            "Only 0 can be written to VLF bit %d in the flag register",
> +            FLAG_REG_VLF);
> +    }
> +
> +    if (diffmask & FLAG_MASK_UNUSED_2) {
> +        *data &= ~FLAG_MASK_UNUSED_2;
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +            "Only 0 can be written to unused bit %d in the flag register",
> +            FLAG_REG_UNUSED_2);
> +    }
> +
> +    if (diffmask & FLAG_MASK_UNUSED_6) {
> +        *data &= ~FLAG_MASK_UNUSED_6;
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +            "Only 0 can be written to unused bit %d in the flag register",
> +            FLAG_REG_UNUSED_6);
> +    }
> +
> +    if (diffmask & FLAG_MASK_UNUSED_7) {
> +        *data &= ~FLAG_MASK_UNUSED_7;
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +            "Only 0 can be written to unused bit %d in the flag register",
> +            FLAG_REG_UNUSED_7);
> +    }
> +}
> +
> +/**
> + * Handle FOUT_ENABLE (FOE) line
> + * Enables/disables the FOUT line
> + * @param opaque the device instance
> + * @param n the IRQ number
> + * @param level true if the line has been raised
> + */
> +static void rx8900_fout_enable_handler(void *opaque, int n, int level)
> +{
> +    RX8900State *s = RX8900(opaque);
> +
> +    if (level) {
> +        trace_rx8900_enable_fout();
> +        ptimer_run(s->fout_timer, 0);
> +    } else {
> +        /* disable fout */
> +        trace_rx8900_disable_fout();
> +        ptimer_stop(s->fout_timer);
> +    }
> +}
> +
> +/**
> + * Tick the FOUT timer
> + * @param opaque the device instance
> + */
> +static void rx8900_fout_tick(void *opaque)
> +{
> +    RX8900State *s = (RX8900State *)opaque;
> +
> +    trace_rx8900_fout_toggle();
> +    s->fout_state = !s->fout_state;
> +
> +    if (s->fout_state) {
> +        qemu_irq_raise(s->fout_pin);
> +    } else {
> +        qemu_irq_lower(s->fout_pin);
> +    }

We could just use qemu_set_irq() here.

> +}
> +
> +/**
> + * Verify the current voltage and raise flags if it is low
> + * @param s the RTC to operate on
> + */
> +static void check_voltage(RX8900State *s)
> +{
> +    if (!(s->nvram[BACKUP_FUNCTION] & BACKUP_MASK_VDETOFF)) {
> +        if (s->supply_voltage < 2.0) {
> +            s->nvram[FLAG_REGISTER] |= FLAG_MASK_VDET;
> +        }
> +
> +        if (s->supply_voltage < 1.6) {
> +            s->nvram[FLAG_REGISTER] |= FLAG_MASK_VLF;
> +        }
> +    }
> +}
> +
> +/**
> + * Receive a byte of data from i2c
> + * @param i2c the i2c device that is receiving data
> + * @param data the data that was received
> + */
> +static int rx8900_send(I2CSlave *i2c, uint8_t data)
> +{
> +    RX8900State *s = RX8900(i2c);
> +    struct tm now;
> +
> +    trace_rx8900_i2c_data_receive(data);
> +
> +    if (s->addr_byte) {
> +        s->nvram_offset = data & (RX8900_NVRAM_SIZE - 1);
> +        trace_rx8900_regptr_update(s->nvram_offset);
> +        s->addr_byte = false;
> +        return 0;
> +    }
> +
> +    trace_rx8900_set_register(s->nvram_offset, data);
> +
> +    qemu_get_timedate(&now, s->offset);
> +    switch (s->nvram_offset) {
> +    case SECONDS:
> +    case EXT_SECONDS:
> +        now.tm_sec = from_bcd(data & 0x7f);
> +        if (now.tm_sec > 59) { /* RX8900 does not support leap seconds */
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                "RX8900 - second data '%x' is out of range, "
> +                    "undefined behavior will result", data);
> +        }
> +        s->offset = qemu_timedate_diff(&now);
> +        break;
> +
> +    case MINUTES:
> +    case EXT_MINUTES:
> +        now.tm_min = from_bcd(data & 0x7f);
> +        if (now.tm_min > 59) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                "RX8900 - minute data '%x' is out of range, "
> +                    "undefined behavior will result", data);
> +        }
> +        s->offset = qemu_timedate_diff(&now);
> +        break;
> +
> +    case HOURS:
> +    case EXT_HOURS:
> +        now.tm_hour = from_bcd(data & 0x3f);
> +        if (now.tm_hour > 24) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                "RX8900 - hour data '%x' is out of range, "
> +                    "undefined behavior will result", data);
> +        }
> +        s->offset = qemu_timedate_diff(&now);
> +        break;
> +
> +    case WEEKDAY:
> +    case EXT_WEEKDAY: {
> +        int user_wday = ctz32(data);
> +        /* The day field is supposed to contain a value in
> +         * with only 1 of bits 0-6 set. Otherwise behavior is undefined.
> +         */
> +        switch (data) {
> +        case 0x01:
> +        case 0x02:
> +        case 0x04:
> +        case 0x08:
> +        case 0x10:
> +        case 0x20:
> +        case 0x40:
> +            break;
> +        default:
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                "RX8900 - weekday data '%x' is out of range, "
> +                        "undefined behavior will result", data);
> +            break;
> +        }

I think it might be clearer to do:

    if (data == 0x80 || ctpop8(data) != 1) {
        qemu_log_mask(...);
        break;
    }

> +        s->weekday = user_wday;
> +        break;
> +    }
> +
> +    case DAY:
> +    case EXT_DAY:
> +        now.tm_mday = from_bcd(data & 0x3f);
> +        s->offset = qemu_timedate_diff(&now);
> +        break;
> +
> +    case MONTH:
> +    case EXT_MONTH:
> +        now.tm_mon = from_bcd(data & 0x1f) - 1;
> +        s->offset = qemu_timedate_diff(&now);
> +        break;
> +
> +    case YEAR:
> +    case EXT_YEAR:
> +        now.tm_year = from_bcd(data) + 100;
> +        s->offset = qemu_timedate_diff(&now);
> +        break;
> +
> +    case EXTENSION_REGISTER:
> +    case EXT_EXTENSION_REGISTER:
> +        update_extension_register(s, data);
> +        break;
> +
> +    case FLAG_REGISTER:
> +    case EXT_FLAG_REGISTER:
> +        validate_flag_register(s, &data);
> +
> +        s->nvram[FLAG_REGISTER] = data;
> +        s->nvram[EXT_FLAG_REGISTER] = data;
> +
> +        check_voltage(s);
> +        break;
> +
> +    case CONTROL_REGISTER:
> +    case EXT_CONTROL_REGISTER:
> +        update_control_register(s, data);
> +        break;
> +
> +    default:
> +        s->nvram[s->nvram_offset] = data;
> +    }
> +
> +    inc_regptr(s);
> +    return 0;
> +}
> +
> +/**
> + * Get the device temperature in Celsius as a property
> + * @param obj the device
> + * @param v
> + * @param name the property name
> + * @param opaque
> + * @param errp an error object to populate on failure
> + */
> +static void rx8900_get_temperature(Object *obj, Visitor *v, const char *name,
> +                                   void *opaque, Error **errp)
> +{
> +    RX8900State *s = RX8900(obj);
> +    double value = (s->nvram[TEMPERATURE] * 2.0 - 187.1) / 3.218;

You have encode_temperature() below, should we not have
decode_temperature()?

> +
> +    trace_rx8900_get_temperature(s->nvram[TEMPERATURE], value);
> +
> +    visit_type_number(v, name, &value, errp);
> +}
> +
> +/**
> + * Encode a temperature in Celsius
> + * @param celsius the temperature
> + * @return the encoded temperature
> + */
> +static inline uint8_t encode_temperature(double celsius)
> +{
> +    return (uint8_t) ((celsius * 3.218 + 187.19) / 2);
> +}
> +
> +/**
> + * Set the device temperature in Celsius as a property
> + * @param obj the device
> + * @param v
> + * @param name the property name
> + * @param opaque
> + * @param errp an error object to populate on failure
> + */
> +static void rx8900_set_temperature(Object *obj, Visitor *v, const char *name,
> +                                   void *opaque, Error **errp)
> +{
> +    RX8900State *s = RX8900(obj);
> +    Error *local_err = NULL;
> +    double temp; /* degrees Celsius */
> +    visit_type_number(v, name, &temp, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +    if (temp >= 100 || temp < -58) {
> +        error_setg(errp, "value %fC is out of range", temp);
> +        return;
> +    }
> +
> +    s->nvram[TEMPERATURE] = encode_temperature(temp);
> +
> +    trace_rx8900_set_temperature(s->nvram[TEMPERATURE], temp);
> +}
> +
> +/**
> + * Get the device supply voltage as a property
> + * @param obj the device
> + * @param v
> + * @param name the property name
> + * @param opaque
> + * @param errp an error object to populate on failure
> + */
> +static void rx8900_get_voltage(Object *obj, Visitor *v, const char *name,
> +                                   void *opaque, Error **errp)
> +{
> +    RX8900State *s = RX8900(obj);
> +
> +    visit_type_number(v, name, &s->supply_voltage, errp);
> +}
> +
> +/**
> + * Set the device supply voltage as a property
> + * @param obj the device
> + * @param v
> + * @param name the property name
> + * @param opaque
> + * @param errp an error object to populate on failure
> + */
> +static void rx8900_set_voltage(Object *obj, Visitor *v, const char *name,
> +                                   void *opaque, Error **errp)
> +{
> +    RX8900State *s = RX8900(obj);
> +    Error *local_err = NULL;
> +    double temp;

Maybe voltage? It's ambiguous as to whether it's a copy/paste error
from 'temperature' or if you mean 'temporary'...

> +    visit_type_number(v, name, &temp, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    s->supply_voltage = temp;

... as this almost looks like a bug.

> +    trace_rx8900_set_voltage(s->supply_voltage);
> +
> +    check_voltage(s);
> +}
> +
> +
> +/**
> + * Configure device properties
> + * @param obj the device
> + */
> +static void rx8900_initfn(Object *obj)
> +{
> +    object_property_add(obj, "temperature", "number",
> +                        rx8900_get_temperature,
> +                        rx8900_set_temperature, NULL, NULL, NULL);
> +
> +    object_property_add(obj, "voltage", "number",
> +                        rx8900_get_voltage,
> +                        rx8900_set_voltage, NULL, NULL, NULL);
> +}
> +
> +/**
> + * Reset the device
> + * @param dev the RX8900 device to reset
> + */
> +static void rx8900_reset(DeviceState *dev)

Is there a reason we can't define rx8900_reset() further up the file to
avoid forward-declaring it?

> +{
> +    RX8900State *s = RX8900(dev);
> +
> +    trace_rx8900_reset();
> +
> +    /* The clock is running and synchronized with the host */
> +    s->offset = 0;
> +    s->weekday = 7; /* Set to an invalid value */

Can you explain why we do that rather than just stating that 7 is
invalid?

> +
> +    s->nvram[EXTENSION_REGISTER] = EXT_MASK_TSEL1;
> +    s->nvram[CONTROL_REGISTER] = CTRL_MASK_CSEL0;
> +    s->nvram[FLAG_REGISTER] &= FLAG_MASK_VDET | FLAG_MASK_VLF;
> +
> +    s->nvram_offset = 0;
> +
> +    trace_rx8900_regptr_update(s->nvram_offset);
> +
> +    s->addr_byte = false;
> +}
> +
> +/**
> + * Realize an RX8900 device instance
> + * Set up timers
> + * Configure GPIO lines
> + * @param dev the device instance to realize
> + * @param errp an error object to populate on error
> + */
> +static void rx8900_realize(DeviceState *dev, Error **errp)
> +{
> +    RX8900State *s = RX8900(dev);
> +    I2CSlave *i2c = I2C_SLAVE(dev);
> +    QEMUBH *bh;
> +    char name[64];
> +
> +    s->fout_state = false;
> +
> +    memset(s->nvram, 0, RX8900_NVRAM_SIZE);
> +    /* Set the initial state to 25 degrees Celsius */
> +    s->nvram[TEMPERATURE] = encode_temperature(25.0);
> +
> +    /* Set up timers */
> +    bh = qemu_bh_new(rx8900_timer_tick, s);
> +    s->sec_timer = ptimer_init(bh, PTIMER_POLICY_DEFAULT);
> +    /* we trigger the timer at 10Hz and check for rollover, as the qemu
> +     * clock does not advance in realtime in the test environment,
> +     * leading to unstable test results
> +     */

It seems unfortunate that some of the code seems dedicated to working
around test suite issues (e.g. early return in some functions). Ah
well.

> +    ptimer_set_freq(s->sec_timer, 10);
> +    ptimer_set_limit(s->sec_timer, 1, 1);
> +
> +    bh = qemu_bh_new(rx8900_fout_tick, s);
> +    s->fout_timer = ptimer_init(bh, PTIMER_POLICY_DEFAULT);
> +    /* frequency doubled to generate 50% duty cycle square wave */
> +    ptimer_set_freq(s->fout_timer, 32768 * 2);
> +    ptimer_set_limit(s->fout_timer, 1, 1);
> +
> +    bh = qemu_bh_new(rx8900_countdown_tick, s);
> +    s->countdown_timer = ptimer_init(bh, PTIMER_POLICY_DEFAULT);
> +    ptimer_set_freq(s->countdown_timer, COUNTDOWN_TIMER_FREQ);
> +    ptimer_set_limit(s->countdown_timer, COUNTDOWN_TIMER_FREQ, 1);
> +
> +
> +    /* set up GPIO */
> +    snprintf(name, sizeof(name), "rx8900-interrupt-out");

Why the snprintf()s?

> +    qdev_init_gpio_out_named(&i2c->qdev, &s->interrupt_pin, name, 1);
> +    trace_rx8900_pin_name("Interrupt", name);
> +
> +    snprintf(name, sizeof(name), "rx8900-fout-enable");
> +    qdev_init_gpio_in_named(&i2c->qdev, rx8900_fout_enable_handler, name, 1);
> +    trace_rx8900_pin_name("Fout-enable", name);
> +
> +    snprintf(name, sizeof(name), "rx8900-fout");
> +    qdev_init_gpio_out_named(&i2c->qdev, &s->fout_pin, name, 1);
> +    trace_rx8900_pin_name("Fout", name);
> +
> +    /* Set up default voltage */
> +    s->supply_voltage = 3.3f;
> +    trace_rx8900_set_voltage(s->supply_voltage);
> +}
> +
> +/**
> + * Set up the device callbacks
> + * @param klass the device class
> + * @param data
> + */
> +static void rx8900_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    I2CSlaveClass *k = I2C_SLAVE_CLASS(klass);
> +
> +    k->event = rx8900_event;
> +    k->recv = rx8900_recv;
> +    k->send = rx8900_send;
> +    dc->realize = rx8900_realize;
> +    dc->reset = rx8900_reset;
> +    dc->vmsd = &vmstate_rx8900;
> +}
> +
> +static const TypeInfo rx8900_info = {
> +    .name = TYPE_RX8900,
> +    .parent = TYPE_I2C_SLAVE,
> +    .instance_size = sizeof(RX8900State),
> +    .instance_init = rx8900_initfn,
> +    .class_init = rx8900_class_init,
> +};
> +
> +/**
> + * Register the device with QEMU
> + */
> +static void rx8900_register_types(void)
> +{
> +    type_register_static(&rx8900_info);
> +}
> +
> +type_init(rx8900_register_types)
> diff --git a/hw/timer/rx8900_regs.h b/hw/timer/rx8900_regs.h
> new file mode 100644
> index 0000000..0aaa9a3
> --- /dev/null
> +++ b/hw/timer/rx8900_regs.h
> @@ -0,0 +1,141 @@
> +/*
> + * Epson RX8900SA/CE Realtime Clock Module
> + *
> + * Copyright (c) 2016 IBM Corporation
> + * Authors:
> > + *  Alastair D'Silva <alastair@d-silva.org>
> + *
> + * This code is licensed under the GPL version 2 or later.  See
> + * the COPYING file in the top-level directory.
> + *
> + * Datasheet available at:
> + *  https://support.epson.biz/td/api/doc_check.php?dl=app_RX8900CE&lang=en
> + *
> + */
> +
> +#ifndef RX8900_REGS_H
> +#define RX8900_REGS_H
> +
> +#include "qemu/bitops.h"
> +
> +#define RX8900_NVRAM_SIZE 0x20

Do all the enums below need to be defined in the header or are they
only relevant to the implementation? If it's the latter, then they
should go in the .c file.

Also I would've reached for #define for all these values, not enums,
but that might be a personal thing.

> +
> +typedef enum RX8900Addresses {
> +    START_ADDRESS = 0x00,
> +    SECONDS = 0x00,
> +    MINUTES = 0x01,
> +    HOURS = 0x02,
> +    WEEKDAY = 0x03, /* a walking bit with bit 0 set for Sunday, 1 for Monday...
> +                       6 for Saturday, see the datasheet for details */
> +    DAY = 0x04,
> +    MONTH = 0x05,
> +    YEAR = 0x06,
> +    RAM = 0x07,
> +    ALARM_MINUTE = 0x08,
> +    ALARM_HOUR = 0x09,
> +    ALARM_WEEK_DAY = 0x0A,
> +    TIMER_COUNTER_0 = 0x0B,
> +    TIMER_COUNTER_1 = 0x0C,
> +    EXTENSION_REGISTER = 0x0D,
> +    FLAG_REGISTER = 0X0E,
> +    CONTROL_REGISTER = 0X0F,
> +    EXT_SECONDS = 0x010, /* Alias of SECONDS */
> +    EXT_MINUTES = 0x11, /* Alias of MINUTES */
> +    EXT_HOURS = 0x12, /* Alias of HOURS */
> +    EXT_WEEKDAY = 0x13, /* Alias of WEEKDAY */
> +    EXT_DAY = 0x14, /* Alias of DAY */
> +    EXT_MONTH = 0x15, /* Alias of MONTH */
> +    EXT_YEAR = 0x16, /* Alias of YEAR */
> +    TEMPERATURE = 0x17,
> +    BACKUP_FUNCTION = 0x18,
> +    NO_USE_1 = 0x19,
> +    NO_USE_2 = 0x1A,
> +    EXT_TIMER_COUNTER_0 = 0x1B, /* Alias of TIMER_COUNTER_0 */
> +    EXT_TIMER_COUNTER_1 = 0x1C, /* Alias of TIMER_COUNTER_1 */
> +    EXT_EXTENSION_REGISTER = 0x1D, /* Alias of EXTENSION_REGISTER */
> +    EXT_FLAG_REGISTER = 0X1E, /* Alias of FLAG_REGISTER */
> +    EXT_CONTROL_REGISTER = 0X1F /* Alias of CONTROL_REGISTER */
> +} RX8900Addresses;
> +
> +typedef enum ExtRegBits {
> +    EXT_REG_TSEL0 = 0,
> +    EXT_REG_TSEL1 = 1,
> +    EXT_REG_FSEL0 = 2,
> +    EXT_REG_FSEL1 = 3,
> +    EXT_REG_TE = 4,
> +    EXT_REG_USEL = 5,
> +    EXT_REG_WADA = 6,
> +    EXT_REG_TEST = 7
> +} ExtRegBits;
> +
> +typedef enum ExtRegMasks {
> +    EXT_MASK_TSEL0 = BIT(0),
> +    EXT_MASK_TSEL1 = BIT(1),
> +    EXT_MASK_FSEL0 = BIT(2),
> +    EXT_MASK_FSEL1 = BIT(3),
> +    EXT_MASK_TE = BIT(4),
> +    EXT_MASK_USEL = BIT(5),
> +    EXT_MASK_WADA = BIT(6),
> +    EXT_MASK_TEST = BIT(7)
> +} ExtRegMasks;
> +
> +typedef enum CtrlRegBits {
> +    CTRL_REG_RESET = 0,
> +    CTRL_REG_WP0 = 1,
> +    CTRL_REG_WP1 = 2,
> +    CTRL_REG_AIE = 3,
> +    CTRL_REG_TIE = 4,
> +    CTRL_REG_UIE = 5,
> +    CTRL_REG_CSEL0 = 6,
> +    CTRL_REG_CSEL1 = 7
> +} CtrlRegBits;
> +
> +typedef enum CtrlRegMask {
> +    CTRL_MASK_RESET = BIT(0),
> +    CTRL_MASK_WP0 = BIT(1),
> +    CTRL_MASK_WP1 = BIT(2),
> +    CTRL_MASK_AIE = BIT(3),
> +    CTRL_MASK_TIE = BIT(4),
> +    CTRL_MASK_UIE = BIT(5),
> +    CTRL_MASK_CSEL0 = BIT(6),
> +    CTRL_MASK_CSEL1 = BIT(7)
> +} CtrlRegMask;
> +
> +typedef enum FlagRegBits {
> +    FLAG_REG_VDET = 0,
> +    FLAG_REG_VLF = 1,
> +    FLAG_REG_UNUSED_2 = 2,
> +    FLAG_REG_AF = 3,
> +    FLAG_REG_TF = 4,
> +    FLAG_REG_UF = 5,
> +    FLAG_REG_UNUSED_6 = 6,
> +    FLAG_REG_UNUSED_7 = 7
> +} FlagRegBits;
> +
> +#define RX8900_INTERRUPT_SOURCES 6
> +typedef enum FlagRegMask {
> +    FLAG_MASK_VDET = BIT(0),
> +    FLAG_MASK_VLF = BIT(1),
> +    FLAG_MASK_UNUSED_2 = BIT(2),
> +    FLAG_MASK_AF = BIT(3),
> +    FLAG_MASK_TF = BIT(4),
> +    FLAG_MASK_UF = BIT(5),
> +    FLAG_MASK_UNUSED_6 = BIT(6),
> +    FLAG_MASK_UNUSED_7 = BIT(7)
> +} FlagRegMask;
> +
> +typedef enum BackupRegBits {
> +    BACKUP_REG_BKSMP0 = 0,
> +    BACKUP_REG_BKSMP1 = 1,
> +    BACKUP_REG_SWOFF = 2,
> +    BACKUP_REG_VDETOFF = 3
> +} BackupRegBits;
> +
> +typedef enum BackupRegMask {
> +    BACKUP_MASK_BKSMP0 = BIT(0),
> +    BACKUP_MASK_BKSMP1 = BIT(1),
> +    BACKUP_MASK_SWOFF = BIT(2),
> +    BACKUP_MASK_VDETOFF = BIT(3)
> +} BackupRegMask;
> +
> +#endif
> diff --git a/hw/timer/trace-events b/hw/timer/trace-events
> index 3495c41..8f4969b 100644
> --- a/hw/timer/trace-events
> +++ b/hw/timer/trace-events
> @@ -49,3 +49,34 @@ aspeed_timer_ctrl_pulse_enable(uint8_t i, bool enable) "Timer %" PRIu8 ": %d"
>  aspeed_timer_set_ctrl2(uint32_t value) "Value: 0x%" PRIx32
>  aspeed_timer_set_value(int timer, int reg, uint32_t value) "Timer %d register %d: 0x%" PRIx32
>  aspeed_timer_read(uint64_t offset, unsigned size, uint64_t value) "From 0x%" PRIx64 ": of size %u: 0x%" PRIx64
> +
> +# hw/timer/rx8900.c
> +rx8900_capture_current_time(int hour, int minute, int second, int weekday, int mday, int month, int year, int raw_hours, int raw_minutes, int raw_seconds, int raw_weekday, int raw_day, int raw_month, int raw_year) "Update current time to %02d:%02d:%02d %d %d/%d/%d (0x%02x%02x%02x%02x%02x%02x%02x)"
> +rx8900_regptr_update(uint32_t ptr) "Operating on register 0x%02x"
> +rx8900_regptr_overflow(void) "Register pointer has overflowed, wrapping to 0"
> +rx8900_event_weekday(int weekday, int weekmask, int weekday_offset) "Set weekday to %d (0x%02x), wday_offset=%d"
> +rx8900_read_register(int address, int val) "Read register 0x%x = 0x%x"
> +rx8900_set_fout(int hz) "Setting fout to %dHz"
> +rx8900_set_countdown_timer(int hz) "Setting countdown timer to %d Hz"
> +rx8900_set_countdown_timer_per_minute(void) "Setting countdown timer to per minute updates"
> +rx8900_enable_update_timer(void) ""
> +rx8900_enable_alarm(void) ""
> +rx8900_trigger_alarm(void) ""
> +rx8900_tick(void) ""
> +rx8900_fire_interrupt(void) ""
> +rx8900_disable_timer(void) ""
> +rx8900_enable_timer(void) ""
> +rx8900_disable_fout(void) ""
> +rx8900_enable_fout(void) ""
> +rx8900_fout_toggle(void) ""
> +rx8900_disable_countdown_timer(void) ""
> +rx8900_enable_countdown_timer(void) ""
> +rx8900_countdown_tick(int count) "Countdown tick, count=%d"
> +rx8900_countdown_elapsed(void) ""
> +rx8900_i2c_data_receive(uint8_t data) "Received I2C data 0x%02x"
> +rx8900_set_register(uint32_t addr, uint8_t data) "Set data 0x%02x=0x%02x"
> +rx8900_get_temperature(uint8_t raw, double val) "0x%x = %fC"
> +rx8900_set_temperature(uint8_t raw, double val) "0x%x = %fC"
> +rx8900_reset(void) ""
> +rx8900_pin_name(const char *type, const char *name) "'%s' pin is '%s'"
> +rx8900_set_voltage(double voltage) "Device voltage set to %f"

Is there a justification for all the empty traces?

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

^ permalink raw reply

* Re: [PATCH v4 1/4] powerpc/mm: refactor radix physical page mapping
From: Aneesh Kumar K.V @ 2017-01-04  5:04 UTC (permalink / raw)
  To: Reza Arbab, Michael Ellerman, Benjamin Herrenschmidt,
	Paul Mackerras
  Cc: linuxppc-dev, Balbir Singh, Alistair Popple
In-Reply-To: <1483476218-17271-2-git-send-email-arbab@linux.vnet.ibm.com>

Reza Arbab <arbab@linux.vnet.ibm.com> writes:

> Move the page mapping code in radix_init_pgtable() into a separate
> function that will also be used for memory hotplug.
>
> The current goto loop progressively decreases its mapping size as it
> covers the tail of a range whose end is unaligned. Change this to a for
> loop which can do the same for both ends of the range.
>

We lost the below in the change.

		pr_info("Mapping range 0x%lx - 0x%lx with 0x%lx\n",
			(unsigned long)base, (unsigned long)end,
			linear_page_size);


Is there a way to dump the range and the size with which we mapped that
range ?


> Signed-off-by: Reza Arbab <arbab@linux.vnet.ibm.com>
> ---
>  arch/powerpc/mm/pgtable-radix.c | 69 ++++++++++++++++++-----------------------
>  1 file changed, 31 insertions(+), 38 deletions(-)
>
> diff --git a/arch/powerpc/mm/pgtable-radix.c b/arch/powerpc/mm/pgtable-radix.c
> index 623a0dc..5cee6d1 100644
> --- a/arch/powerpc/mm/pgtable-radix.c
> +++ b/arch/powerpc/mm/pgtable-radix.c
> @@ -107,54 +107,47 @@ int radix__map_kernel_page(unsigned long ea, unsigned long pa,
>  	return 0;
>  }
>
> +static int __meminit create_physical_mapping(unsigned long start,
> +					     unsigned long end)
> +{
> +	unsigned long mapping_size;
> +
> +	start = _ALIGN_UP(start, PAGE_SIZE);
> +	for (; start < end; start += mapping_size) {
> +		unsigned long gap = end - start;
> +		int rc;
> +
> +		if (IS_ALIGNED(start, PUD_SIZE) && gap >= PUD_SIZE &&
> +		    mmu_psize_defs[MMU_PAGE_1G].shift)
> +			mapping_size = PUD_SIZE;
> +		else if (IS_ALIGNED(start, PMD_SIZE) && gap >= PMD_SIZE &&
> +			 mmu_psize_defs[MMU_PAGE_2M].shift)
> +			mapping_size = PMD_SIZE;
> +		else
> +			mapping_size = PAGE_SIZE;
> +
> +		rc = radix__map_kernel_page((unsigned long)__va(start), start,
> +					    PAGE_KERNEL_X, mapping_size);
> +		if (rc)
> +			return rc;
> +	}
> +
> +	return 0;
> +}
> +
>  static void __init radix_init_pgtable(void)
>  {
> -	int loop_count;
> -	u64 base, end, start_addr;
>  	unsigned long rts_field;
>  	struct memblock_region *reg;
> -	unsigned long linear_page_size;
>
>  	/* We don't support slb for radix */
>  	mmu_slb_size = 0;
>  	/*
>  	 * Create the linear mapping, using standard page size for now
>  	 */
> -	loop_count = 0;
> -	for_each_memblock(memory, reg) {
> -
> -		start_addr = reg->base;
> -
> -redo:
> -		if (loop_count < 1 && mmu_psize_defs[MMU_PAGE_1G].shift)
> -			linear_page_size = PUD_SIZE;
> -		else if (loop_count < 2 && mmu_psize_defs[MMU_PAGE_2M].shift)
> -			linear_page_size = PMD_SIZE;
> -		else
> -			linear_page_size = PAGE_SIZE;
> -
> -		base = _ALIGN_UP(start_addr, linear_page_size);
> -		end = _ALIGN_DOWN(reg->base + reg->size, linear_page_size);
> -
> -		pr_info("Mapping range 0x%lx - 0x%lx with 0x%lx\n",
> -			(unsigned long)base, (unsigned long)end,
> -			linear_page_size);
> -
> -		while (base < end) {
> -			radix__map_kernel_page((unsigned long)__va(base),
> -					      base, PAGE_KERNEL_X,
> -					      linear_page_size);
> -			base += linear_page_size;
> -		}
> -		/*
> -		 * map the rest using lower page size
> -		 */
> -		if (end < reg->base + reg->size) {
> -			start_addr = end;
> -			loop_count++;
> -			goto redo;
> -		}
> -	}
> +	for_each_memblock(memory, reg)
> +		WARN_ON(create_physical_mapping(reg->base,
> +						reg->base + reg->size));
>  	/*
>  	 * Allocate Partition table and process table for the
>  	 * host.
> -- 
> 1.8.3.1

^ permalink raw reply

* [PATCH v4 2/3] ARM: dts: imx6: Support Savageboard dual
From: Milo Kim @ 2017-01-04  4:55 UTC (permalink / raw)
  To: Shawn Guo, Sascha Hauer
  Cc: Fabio Estevam, linux-arm-kernel, devicetree, linux-kernel,
	Milo Kim
In-Reply-To: <20170104045553.26576-1-woogyom.kim@gmail.com>

Common savageboard DT file is used for board support.
Add the vendor name and specify the dtb file for i.MX6Q build.

Reviewed-by: Fabio Estevam <fabio.estevam@nxp.com>
Signed-off-by: Milo Kim <woogyom.kim@gmail.com>
---
 .../devicetree/bindings/vendor-prefixes.txt        |  1 +
 arch/arm/boot/dts/Makefile                         |  1 +
 arch/arm/boot/dts/imx6dl-savageboard.dts           | 51 ++++++++++++++++++++++
 3 files changed, 53 insertions(+)
 create mode 100644 arch/arm/boot/dts/imx6dl-savageboard.dts

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index 16d3b5e7f5d1..88c33d827e51 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -227,6 +227,7 @@ pine64	Pine64
 pixcir  PIXCIR MICROELECTRONICS Co., Ltd
 plathome	Plat'Home Co., Ltd.
 plda	PLDA
+poslab  Poslab Technology Co., Ltd.
 powervr	PowerVR (deprecated, use img)
 pulsedlight	PulsedLight, Inc
 qca	Qualcomm Atheros, Inc.
diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
index cccdbcb557b6..fb7f904a5235 100644
--- a/arch/arm/boot/dts/Makefile
+++ b/arch/arm/boot/dts/Makefile
@@ -357,6 +357,7 @@ dtb-$(CONFIG_SOC_IMX6Q) += \
 	imx6dl-sabreauto.dtb \
 	imx6dl-sabrelite.dtb \
 	imx6dl-sabresd.dtb \
+	imx6dl-savageboard.dtb \
 	imx6dl-ts4900.dtb \
 	imx6dl-tx6dl-comtft.dtb \
 	imx6dl-tx6s-8034.dtb \
diff --git a/arch/arm/boot/dts/imx6dl-savageboard.dts b/arch/arm/boot/dts/imx6dl-savageboard.dts
new file mode 100644
index 000000000000..b95469c520a4
--- /dev/null
+++ b/arch/arm/boot/dts/imx6dl-savageboard.dts
@@ -0,0 +1,51 @@
+/*
+ * Copyright (C) 2017 Milo Kim <woogyom.kim@gmail.com>
+ *
+ * This file is dual-licensed: you can use it either under the terms
+ * of the GPL or the X11 license, at your option. Note that this dual
+ * licensing only applies to this file, and not this project as a
+ * whole.
+ *
+ *  a) This file is free software; you can redistribute it and/or
+ *     modify it under the terms of the GNU General Public License as
+ *     published by the Free Software Foundation; either version 2 of the
+ *     License, or (at your option) any later version.
+ *
+ *     This file is distributed in the hope that it will be useful,
+ *     but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *     MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *     GNU General Public License for more details.
+ *
+ * Or, alternatively,
+ *
+ *  b) Permission is hereby granted, free of charge, to any person
+ *     obtaining a copy of this software and associated documentation
+ *     files (the "Software"), to deal in the Software without
+ *     restriction, including without limitation the rights to use,
+ *     copy, modify, merge, publish, distribute, sublicense, and/or
+ *     sell copies of the Software, and to permit persons to whom the
+ *     Software is furnished to do so, subject to the following
+ *     conditions:
+ *
+ *     The above copyright notice and this permission notice shall be
+ *     included in all copies or substantial portions of the Software.
+ *
+ *     THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ *     EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
+ *     OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ *     NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
+ *     HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
+ *     WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ *     FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ *     OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+/dts-v1/;
+
+#include "imx6dl.dtsi"
+#include "imx6qdl-savageboard.dtsi"
+
+/ {
+	model = "Poslab SavageBoard Dual";
+	compatible = "poslab,imx6dl-savageboard", "fsl,imx6dl";
+};
-- 
2.11.0

^ permalink raw reply related

* [PATCH net-next] net: phy: add extension of phy-mode for XLGMII
From: Jie Deng @ 2017-01-04  5:04 UTC (permalink / raw)
  To: f.fainelli, davem; +Cc: netdev, linux-kernel, Jie Deng

The Synopsys DWC_xlgmac core provides a multiplexed 40-Gigabit
Media-Independent Interface (XLGMII, an IEEE 802.3 Clause 81
compliant reconciliation sub-layer) for communication with
the 100/50/40/25-Gigabit PHY and 10-Gigabit Media-Independent
Interface (XGMII, an IEEE 802.3 Clause 46 compliant reconciliation
sub-layer) for communication with the 10-Gigabit PHY.

Currently, There are only interface mode definitions for "xgmii".
This patch adds the definitions for the PHY layer to recognize
"xlgmii" as a valid PHY interface.

Signed-off-by: Jie Deng <jiedeng@synopsys.com>
---
 include/linux/phy.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/phy.h b/include/linux/phy.h
index f7d95f6..7b6bfb3 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -82,6 +82,7 @@
 	PHY_INTERFACE_MODE_MOCA,
 	PHY_INTERFACE_MODE_QSGMII,
 	PHY_INTERFACE_MODE_TRGMII,
+	PHY_INTERFACE_MODE_XLGMII,
 	PHY_INTERFACE_MODE_MAX,
 } phy_interface_t;
 
@@ -142,6 +143,8 @@ static inline const char *phy_modes(phy_interface_t interface)
 		return "qsgmii";
 	case PHY_INTERFACE_MODE_TRGMII:
 		return "trgmii";
+	case PHY_INTERFACE_MODE_XLGMII:
+		return "xlgmii";
 	default:
 		return "unknown";
 	}
-- 
1.9.1

^ permalink raw reply related

* [PATCH v4 1/3] ARM: dts: imx6: Add Savageboard common file
From: Milo Kim @ 2017-01-04  4:55 UTC (permalink / raw)
  To: Shawn Guo, Sascha Hauer
  Cc: Fabio Estevam, linux-arm-kernel, devicetree, linux-kernel,
	Milo Kim
In-Reply-To: <20170104045553.26576-1-woogyom.kim@gmail.com>

* Memory
  memblock for DDR3 1GB

* Regulator
  3.3V for panel and backlight.

* Display
  Enable HDMI and LVDS panel. Savageboard supports AVIC TM097TDH02 panel
  which is compatible with Hannstar HSD100PXN1, so reuse it.

* Clock
  The commit d28be499c45e6 ("ARM: dts: imx6qdl-sabresd: Allow HDMI and
  LVDS to work simultaneously") is applied to support LVDS and HDMI output
  at the same time.

* Pinmux
  Support eMMC, ethernet, gpio key for power button, I2C, PWM, SD card 
  and UART.

* Others
  Enable ethernet, UART1 debug, USB host, USDHC3 for microSD card and
  USDHC4 for built-in eMMC storage.

Reviewed-by: Fabio Estevam <fabio.estevam@nxp.com>
Signed-off-by: Milo Kim <woogyom.kim@gmail.com>
---
 arch/arm/boot/dts/imx6qdl-savageboard.dtsi | 255 +++++++++++++++++++++++++++++
 1 file changed, 255 insertions(+)
 create mode 100644 arch/arm/boot/dts/imx6qdl-savageboard.dtsi

diff --git a/arch/arm/boot/dts/imx6qdl-savageboard.dtsi b/arch/arm/boot/dts/imx6qdl-savageboard.dtsi
new file mode 100644
index 000000000000..a616e3c400d3
--- /dev/null
+++ b/arch/arm/boot/dts/imx6qdl-savageboard.dtsi
@@ -0,0 +1,255 @@
+/*
+ * Copyright (C) 2017 Milo Kim <woogyom.kim@gmail.com>
+ *
+ * This file is dual-licensed: you can use it either under the terms
+ * of the GPL or the X11 license, at your option. Note that this dual
+ * licensing only applies to this file, and not this project as a
+ * whole.
+ *
+ *  a) This file is free software; you can redistribute it and/or
+ *     modify it under the terms of the GNU General Public License as
+ *     published by the Free Software Foundation; either version 2 of the
+ *     License, or (at your option) any later version.
+ *
+ *     This file is distributed in the hope that it will be useful,
+ *     but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *     MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *     GNU General Public License for more details.
+ *
+ * Or, alternatively,
+ *
+ *  b) Permission is hereby granted, free of charge, to any person
+ *     obtaining a copy of this software and associated documentation
+ *     files (the "Software"), to deal in the Software without
+ *     restriction, including without limitation the rights to use,
+ *     copy, modify, merge, publish, distribute, sublicense, and/or
+ *     sell copies of the Software, and to permit persons to whom the
+ *     Software is furnished to do so, subject to the following
+ *     conditions:
+ *
+ *     The above copyright notice and this permission notice shall be
+ *     included in all copies or substantial portions of the Software.
+ *
+ *     THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ *     EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
+ *     OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ *     NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
+ *     HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
+ *     WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ *     FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ *     OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+#include <dt-bindings/gpio/gpio.h>
+#include <dt-bindings/input/input.h>
+
+/ {
+	chosen {
+		stdout-path = &uart1;
+	};
+
+	memory@10000000 {
+		device_type = "memory";
+		reg = <0x10000000 0x40000000>;
+	};
+
+	gpio-keys {
+		compatible = "gpio-keys";
+		pinctrl-names = "default";
+		pinctrl-0 = <&pinctrl_gpio_keys>;
+
+		power {
+			gpios = <&gpio3 7 GPIO_ACTIVE_LOW>;
+			label = "Power Button";
+			linux,code = <KEY_POWER>;
+			wakeup-source;
+		};
+	};
+
+	panel {
+		compatible = "avic,tm097tdh02", "hannstar,hsd100pxn1";
+		backlight = <&panel_bl>;
+		power-supply = <&reg_3p3v>;
+
+		port {
+			panel_in: endpoint {
+				remote-endpoint = <&lvds0_out>;
+			};
+		};
+	};
+
+	panel_bl: backlight {
+		compatible = "pwm-backlight";
+		brightness-levels = <0 4 8 16 32 64 128 255>;
+		default-brightness-level = <4>;
+		power-supply = <&reg_3p3v>;
+		pwms = <&pwm1 0 10000>;
+	};
+
+	reg_3p3v: regulator-3p3v {
+		compatible = "regulator-fixed";
+		regulator-name = "3P3V";
+		regulator-min-microvolt = <3300000>;
+		regulator-max-microvolt = <3300000>;
+		regulator-always-on;
+	};
+};
+
+&clks {
+	assigned-clocks = <&clks IMX6QDL_CLK_LDB_DI0_SEL>,
+			  <&clks IMX6QDL_CLK_LDB_DI1_SEL>;
+	assigned-clock-parents = <&clks IMX6QDL_CLK_PLL3_USB_OTG>,
+				 <&clks IMX6QDL_CLK_PLL3_USB_OTG>;
+};
+
+&fec {
+	phy-mode = "rgmii";
+	phy-reset-gpios = <&gpio1 25 GPIO_ACTIVE_LOW>;
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_enet>;
+	status = "okay";
+};
+
+&hdmi {
+	ddc-i2c-bus = <&i2c2>;
+	status = "okay";
+};
+
+&i2c2 {
+	clock-frequency = <100000>;
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_i2c2>;
+	status = "okay";
+};
+
+&ldb {
+	status = "okay";
+
+	lvds-channel@0 {
+		reg = <0>;
+		status = "okay";
+
+		port@4 {
+			reg = <4>;
+
+			lvds0_out: endpoint {
+				remote-endpoint = <&panel_in>;
+			};
+		};
+	};
+};
+
+&pwm1 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_pwm1>;
+	status = "okay";
+};
+
+&uart1 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_uart1>;
+	status = "okay";
+};
+
+&usbh1 {
+	status = "okay";
+};
+
+/* SD card */
+&usdhc3 {
+	bus-width = <4>;
+	cd-gpios = <&gpio2 0 GPIO_ACTIVE_LOW>;
+	no-1-8-v;
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_sd>;
+	status = "okay";
+};
+
+/* eMMC */
+&usdhc4 {
+	bus-width = <8>;
+	keep-power-in-suspend;
+	no-1-8-v;
+	non-removable;
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_emmc>;
+	status = "okay";
+};
+
+&iomuxc {
+	pinctrl_emmc: emmcgrp {
+		fsl,pins = <
+			MX6QDL_PAD_SD4_CMD__SD4_CMD		0x17059
+			MX6QDL_PAD_SD4_CLK__SD4_CLK		0x10059
+			MX6QDL_PAD_SD4_DAT0__SD4_DATA0		0x17059
+			MX6QDL_PAD_SD4_DAT1__SD4_DATA1		0x17059
+			MX6QDL_PAD_SD4_DAT2__SD4_DATA2		0x17059
+			MX6QDL_PAD_SD4_DAT3__SD4_DATA3		0x17059
+			MX6QDL_PAD_SD4_DAT4__SD4_DATA4		0x17059
+			MX6QDL_PAD_SD4_DAT5__SD4_DATA5		0x17059
+			MX6QDL_PAD_SD4_DAT6__SD4_DATA6		0x17059
+			MX6QDL_PAD_SD4_DAT7__SD4_DATA7		0x17059
+		>;
+	};
+
+	pinctrl_enet: enetgrp {
+		fsl,pins = <
+			MX6QDL_PAD_ENET_MDIO__ENET_MDIO		0x1b0b0
+			MX6QDL_PAD_ENET_MDC__ENET_MDC		0x1b0b0
+			MX6QDL_PAD_RGMII_TXC__RGMII_TXC		0x1b030
+			MX6QDL_PAD_RGMII_TD0__RGMII_TD0		0x1b030
+			MX6QDL_PAD_RGMII_TD1__RGMII_TD1		0x1b030
+			MX6QDL_PAD_RGMII_TD2__RGMII_TD2		0x1b030
+			MX6QDL_PAD_RGMII_TD3__RGMII_TD3		0x1b030
+			MX6QDL_PAD_RGMII_TX_CTL__RGMII_TX_CTL	0x1b030
+			MX6QDL_PAD_ENET_REF_CLK__ENET_TX_CLK	0x1b0b0
+			MX6QDL_PAD_RGMII_RXC__RGMII_RXC		0x1b030
+			MX6QDL_PAD_RGMII_RD0__RGMII_RD0		0x1b030
+			MX6QDL_PAD_RGMII_RD1__RGMII_RD1		0x1b030
+			MX6QDL_PAD_RGMII_RD2__RGMII_RD2		0x1b030
+			MX6QDL_PAD_RGMII_RD3__RGMII_RD3		0x1b030
+			MX6QDL_PAD_RGMII_RX_CTL__RGMII_RX_CTL	0x1b030
+			/* PHY reset */
+			MX6QDL_PAD_ENET_CRS_DV__GPIO1_IO25	0x1b0b0
+		>;
+	};
+
+	pinctrl_gpio_keys: gpiokeysgrp {
+		fsl,pins = <
+			MX6QDL_PAD_EIM_DA7__GPIO3_IO07		0x1b0b1
+		>;
+	};
+
+	pinctrl_i2c2: i2c2grp {
+		fsl,pins = <
+			MX6QDL_PAD_KEY_COL3__I2C2_SCL		0x4001b8b1
+			MX6QDL_PAD_KEY_ROW3__I2C2_SDA		0x4001b8b1
+		>;
+	};
+
+	pinctrl_pwm1: pwm1grp {
+		fsl,pins = <
+			MX6QDL_PAD_SD1_DAT3__PWM1_OUT		0x1b0b1
+		>;
+	};
+
+	pinctrl_sd: sdgrp {
+		fsl,pins = <
+			MX6QDL_PAD_SD3_CMD__SD3_CMD		0x17059
+			MX6QDL_PAD_SD3_CLK__SD3_CLK		0x10059
+			MX6QDL_PAD_SD3_DAT0__SD3_DATA0		0x17059
+			MX6QDL_PAD_SD3_DAT1__SD3_DATA1		0x17059
+			MX6QDL_PAD_SD3_DAT2__SD3_DATA2		0x17059
+			MX6QDL_PAD_SD3_DAT3__SD3_DATA3		0x17059
+			/* CD pin */
+			MX6QDL_PAD_NANDF_D0__GPIO2_IO00		0x1b0b1
+		>;
+	};
+
+	pinctrl_uart1: uart1grp {
+		fsl,pins = <
+			MX6QDL_PAD_CSI0_DAT10__UART1_TX_DATA	0x1b0b1
+			MX6QDL_PAD_CSI0_DAT11__UART1_RX_DATA	0x1b0b1
+		>;
+	};
+};
-- 
2.11.0

^ permalink raw reply related

* Re: [RFC 1/3] abi_spec: basic definitions of constraints, args and syscalls
From: alexander.levin @ 2017-01-04  4:52 UTC (permalink / raw)
  To: Dmitry Vyukov
  Cc: tglx@linutronix.de, scientist@fb.com, glider@google.com,
	andreyknvl@google.com, rostedt@goodmis.org, arnd@arndb.de,
	mathieu.desnoyers@efficios.com, daniel.vetter@ffwll.ch,
	linux-kernel@vger.kernel.org
In-Reply-To: <CACT4Y+b_M3J2xB-CpOafFizRGWeK=dWAkQQ-6WFyFSda-VdDQA@mail.gmail.com>

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

On Wed, Dec 14, 2016 at 08:46:25PM +0100, Dmitry Vyukov wrote:
> Here is my current prototype:
> https://github.com/dvyukov/linux/commit/6200a9333e78bef393f8ead41205813b94d340f3
> 
> For now it can trace arguments of 4 system calls:
> 
> [    4.055483] [pid 1258] open(&00007ffdefc023a0=[], 0x0, 0x1b6)
> [    4.055991] [pid 1258] open(&00007ffdefc023a0=[], 0x0, 0x1b6) = 3
> [    4.056486] [pid 1258] read(0x3, &00007ffdefc01320=[], 0x1000)
> [    4.056977] [pid 1258] read(0x3, &00007ffdefc01320=[], 0x1000) = 1780
> [    4.057485] [pid 1258] read(0x3, &00007f316a732000=[], 0x1000)
> [    4.057991] [pid 1258] read(0x3, &00007f316a732000=[], 0x1000) = 0
> [    4.058488] [pid 1258] close(0x0) = 0
> [    4.058848] [pid 1258] write(0x1, &00007f316a732000=[], 0x5)
> [    4.059304] [pid 1258] write(0x1, &00007f316a732000=[], 0x5) = 5
> [    4.059905] [pid 1234] close(0x0) = 0
> [    4.060239] [pid 1234] close(0x0) = 0
> 
> 
> Main outstanding problems:
>  - understanding length of arrays and buffers
>  - understanding discriminators of unions and syscall variations

Happy new year! I've been away for a bit myself, but now back working on this.

Attached a patch on top of your commit.

There are two things (very messy, I just want to go through the concept):

 - Reading the values into a generic fields struct, based on your suggestion.
There's no actual struct there, just the values - we can figure out how it'll
look like exactly, but something along this path makes sense?

tglx also raised a point that we want to read from userspace only once for
performance; it's a bit early to address performance at this stage, but it's
another advantage to pursuing this approach.

 - Array/string length. Since we read all values, we can point to the array's
length by using an offset from the currect arg. So for example, in read(), the
length of the buffer is at +1 offset from the buffer. This seems to be the case
for most syscalls.

The exception here is strings which we can just define as offset == 0.


With the patch, the trace is now able to work with strings:

[    1.234156] [pid 891] open(&00007fa7b35d4035=[ /etc/ld.so.cache ], 0x80000, 0x1)
[    1.235244] [pid 891] open(&00007fa7b35d4035=[ /etc/ld.so.cache ], 0x80000, 0x1) = -2
[    1.236101] [pid 891] open(&00007ffe57ca2a70=[ /lib/x86_64-linux-gnu/tls/x86_64/libc.so.6 ], 0x80000, 0xb37db168)
[    1.237361] [pid 891] open(&00007ffe57ca2a70=[ /lib/x86_64-linux-gnu/tls/x86_64/libc.so.6 ], 0x80000, 0xb37db168) = -2
[    1.238545] [pid 891] open(&00007ffe57ca2a70=[ /lib/x86_64-linux-gnu/tls/libc.so.6 ], 0x80000, 0xb37db168)
[    1.239600] [pid 891] open(&00007ffe57ca2a70=[ /lib/x86_64-linux-gnu/tls/libc.so.6 ], 0x80000, 0xb37db168) = -2
[    1.241033] [pid 891] open(&00007ffe57ca2a70=[ /lib/x86_64-linux-gnu/x86_64/libc.so.6 ], 0x80000, 0xb37db168)
[    1.242163] [pid 891] open(&00007ffe57ca2a70=[ /lib/x86_64-linux-gnu/x86_64/libc.so.6 ], 0x80000, 0xb37db168) = -2
[    1.243329] [pid 891] open(&00007ffe57ca2a70=[ /lib/x86_64-linux-gnu/libc.so.6 ], 0x80000, 0xb37db168)
[    1.244712] [pid 891] open(&00007ffe57ca2a70=[ /lib/x86_64-linux-gnu/libc.so.6 ], 0x80000, 0xb37db168) = 3
[    1.245633] [pid 891] read(0x3, &00007ffe57ca2c98=[ (null) ], 0x340)
[    1.246334] [pid 891] read(0x3, &00007ffe57ca2c98=[ (null) ], 0x340) = 832

Does the idea makes sense?

-- 

Thanks,
Sasha

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: diff.patch --]
[-- Type: text/x-diff; name="diff.patch", Size: 6635 bytes --]

diff --git a/include/uapi/linux/abi_spec.h b/include/uapi/linux/abi_spec.h
index dd8ddc3..21b31f9 100644
--- a/include/uapi/linux/abi_spec.h
+++ b/include/uapi/linux/abi_spec.h
@@ -80,6 +80,7 @@ struct type {
 		// KIND_ARRAY
 		struct {
 			struct type	*type;
+			u8		length_off;
 		} array;
 
 		// KIND_STRUCT
@@ -110,4 +111,15 @@ struct syscall_spec {
 	struct argument	args[ABI_MAX_ARGS];
 };
 
+struct syscall_values {
+	union {
+		u8 u8;
+		u16 u16;
+		u32 u32;
+		u64 u64;
+		void *ptr;
+		char *str;
+	};
+};
+
 #endif
diff --git a/kernel/abi_spec.c b/kernel/abi_spec.c
index fa249bd..dbafb98 100644
--- a/kernel/abi_spec.c
+++ b/kernel/abi_spec.c
@@ -7,10 +7,11 @@
 #include <linux/errno.h>
 #include <linux/fcntl.h>
 #include <linux/stat.h>
+#include <linux/vmalloc.h>
 
-typedef void (*cb_t)(void *ctx, struct type *t, int flags, const void __user *p, int post);
+typedef void (*cb_t)(void *ctx, struct type *t, int flags, const void __user *p, struct syscall_values *val, int post);
 
-static size_t handle_type(cb_t cb, void *ctx, struct type *t, int flags, const void __user *p);
+static size_t handle_type(cb_t cb, void *ctx, struct type *t, int flags, const void __user *p, struct syscall_values *val);
 
 static u64 read_val(struct type *t, int flags, const void __user *p)
 {
@@ -60,27 +61,34 @@ static u64 read_val(struct type *t, int flags, const void __user *p)
 	}
 }
 
-static size_t handle_type(cb_t cb, void *ctx, struct type *t, int flags, const void __user *p)
+static size_t handle_type(cb_t cb, void *ctx, struct type *t, int flags, const void __user *p, struct syscall_values *val)
 {
 	size_t off;
 
-	cb(ctx, t, flags, p, 0);
+	cb(ctx, t, flags, p, val, 0);
 	switch (t->kind) {
 	case KIND_SCALAR: {
 		off = t->scalar.size;
+		val->u64 = read_val(t, flags, p);
 		break;
 	}
 	case KIND_PTR: {
 		const void __user* p1;
 
 		p1 = (const void __user*)read_val(t, flags, p);
-		handle_type(cb, ctx, t->ptr.type, 0, p1);
+		handle_type(cb, ctx, t->ptr.type, 0, p1, val);
 		off = sizeof(void *);
 		break;
 	}
 	case KIND_ARRAY: {
-		// TODO: don't know the size...
-		// off = handle_array(cb, ctx, t, p);
+		// length_off == 0 => string
+		if (t->array.length_off == 0) {
+			u8 len = strnlen_user(p, 0x10000);
+			val->ptr = vmalloc(len);
+			if (WARN_ON(!val->ptr))
+				break;
+			strncpy_from_user(val->ptr, p, len);
+		} else {} // todo
 		break;
 	}
 	case KIND_STRUCT: {
@@ -95,8 +103,8 @@ static size_t handle_type(cb_t cb, void *ctx, struct type *t, int flags, const v
 			if (!f)
 				break;
 			if (i)
-				cb(ctx, NULL, 0, NULL, 0);
-			off += handle_type(cb, ctx, f, arg->flags, p + off);
+				cb(ctx, NULL, 0, NULL, val, 0);
+			off += handle_type(cb, ctx, f, arg->flags, p + off, val);
 		}
 		break;
 	}
@@ -109,13 +117,13 @@ static size_t handle_type(cb_t cb, void *ctx, struct type *t, int flags, const v
 		struct type *t1;
 
 		for (t1 = t; t1->kind == KIND_RESOURCE; t1 = t1->res.type) {}
-		off = handle_type(cb, ctx, t1, flags, p);
+		off = handle_type(cb, ctx, t1, flags, p, val);
 		break;
 	}
 	default:
 		BUG();
 	}
-	cb(ctx, t, flags, p, 1);
+	cb(ctx, t, flags, p, val, 1);
 	return off;
 }
 
@@ -125,6 +133,7 @@ static void handle_syscall(cb_t cb, void *ctx, struct syscall_spec *s, va_list *
 	struct argument *arg;
 	struct type *f;
 	long v;
+	struct syscall_values vals[ABI_MAX_ARGS] = { 0 };
 
 	for (i = 0; i < ABI_MAX_ARGS; i++) {
 		arg = &s->args[i];
@@ -132,9 +141,9 @@ static void handle_syscall(cb_t cb, void *ctx, struct syscall_spec *s, va_list *
 		if (!f)
 			break;
 		if (i)
-			cb(ctx, NULL, 0, NULL, 0);
+			cb(ctx, NULL, 0, NULL, &vals[i], 0);
 		v = va_arg(*ap, long);
-		handle_type(cb, ctx, f, arg->flags, &v);
+		handle_type(cb, ctx, f, arg->flags, &v, &vals[i]);
 	}
 }
 
@@ -148,7 +157,7 @@ static void check_retval(struct syscall_spec *s, long retval)
 		if (s->errno[i] == -retval)
 			return;
 	}
-	__WARN_printf("syscall %s returned unexpected error %ld",
+	WARN("syscall %s returned unexpected error %ld",
 		s->name, retval);
 }
 
@@ -169,7 +178,7 @@ void check_pre_printf(struct check_pre_ctx *ctx, const char *fmt, ...)
 	va_end(args);
 }
 
-void check_pre_cb(void *ctx, struct type *t, int flags, const void __user *p, int post)
+void check_pre_cb(void *ctx, struct type *t, int flags, const void __user *p, struct syscall_values *val, int post)
 {
 	if (!t) {
 		check_pre_printf(ctx, ", ");
@@ -188,7 +197,7 @@ void check_pre_cb(void *ctx, struct type *t, int flags, const void __user *p, in
 		check_pre_printf(ctx, "&%p=", (void*)read_val(t, flags, p));
 		break;
 	case KIND_ARRAY:
-		check_pre_printf(ctx, post ? "]" : "[");
+		check_pre_printf(ctx, post ? " %s ]" : "[", val->ptr);
 		break;
 	case KIND_STRUCT:
 		check_pre_printf(ctx, post ? "}" : "{");
@@ -287,9 +296,16 @@ static struct type type_iptr = {
 	.scalar.size = sizeof(void *),
 };
 
+static struct type type_string_i8 = {
+        .kind = KIND_ARRAY,
+        .array.type = &type_i8,
+        .array.length_off = 0,
+};
+
 static struct type type_array_i8 = {
 	.kind = KIND_ARRAY,
 	.array.type = &type_i8,
+	.array.length_off = 1,
 };
 
 static struct type type_ptr_array_i8 = {
@@ -300,7 +316,7 @@ static struct type type_ptr_array_i8 = {
 static struct type type_pathname = {
 	.kind = KIND_RESOURCE,
 	.res.res = RES_PATHNAME,
-	.res.type = &type_array_i8,
+	.res.type = &type_string_i8,
 };
 
 static struct type type_ptr_pathname = {
@@ -752,5 +768,15 @@ struct syscall_spec syscall_spec_sync_file_range2 = { .name = "sync_file_range2"
 struct syscall_spec syscall_spec_statfs64 = { .name = "statfs64" };
 struct syscall_spec syscall_spec_fstatfs64 = { .name = "fstatfs64" };
 struct syscall_spec syscall_spec_bdflush = { .name = "bdflush" };
+struct syscall_spec syscall_spec_sigaction = { .name = "sigaction" };
+struct syscall_spec syscall_spec_old_mmap = { .name = "old_mmap" };
+struct syscall_spec syscall_spec_truncate64 = { .name = "truncate64" };
+struct syscall_spec syscall_spec_ftruncate64 = { .name = "ftruncate64" };
+struct syscall_spec syscall_spec_stat64 = { .name = "stat64" };
+struct syscall_spec syscall_spec_lstat64 = { .name = "lstat64" };
+struct syscall_spec syscall_spec_fstat64 = { .name = "fstat64" };
+struct syscall_spec syscall_spec_fstatat64 = { .name = "fstatat64" };
+struct syscall_spec syscall_spec_fcntl64 = { .name = "fcntl64" };
+struct syscall_spec syscall_spec_old_select = { .name = "old_select" };
 
 #undef $

^ permalink raw reply related

* Re: [PATCH tip/master v3] kprobes: extable: Identify kprobes' insn-slots as kernel text area
From: Masami Hiramatsu @ 2017-01-04  5:06 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Ingo Molnar, Josh Poimboeuf, linux-kernel,
	Ananth N Mavinakayanahalli, Thomas Gleixner, H . Peter Anvin,
	Andrey Konovalov, Steven Rostedt
In-Reply-To: <20170103105402.GB25813@worktop.programming.kicks-ass.net>

On Tue, 3 Jan 2017 11:54:02 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> On Tue, Dec 27, 2016 at 03:14:10PM +0900, Masami Hiramatsu wrote:
> 
> > diff --git a/kernel/extable.c b/kernel/extable.c
> > index e820cce..81c9633 100644
> > --- a/kernel/extable.c
> > +++ b/kernel/extable.c
> 
> > @@ -123,7 +126,11 @@ int kernel_text_address(unsigned long addr)
> >  		return 1;
> >  	if (is_module_text_address(addr))
> >  		return 1;
> > -	return is_ftrace_trampoline(addr);
> > +	if (is_ftrace_trampoline(addr))
> > +		return 1;
> > +	if (is_kprobe_optinsn_slot(addr) || is_kprobe_insn_slot(addr))
> > +		return 1;
> > +	return 0;
> >  }
> 
> > diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> > index d630954..be41f6d 100644
> > --- a/kernel/kprobes.c
> > +++ b/kernel/kprobes.c
> 
> > +/*
> > + * Check given address is on the page of kprobe instruction slots.
> > + * This will be used for checking whether the address on a stack
> > + * is on a text area or not.
> > + */
> > +bool __is_insn_slot_addr(struct kprobe_insn_cache *c, unsigned long addr)
> > +{
> > +	struct kprobe_insn_page *kip;
> > +	bool ret = false;
> > +
> > +	rcu_read_lock();
> > +	list_for_each_entry_rcu(kip, &c->pages, list) {
> > +		if (addr >= (unsigned long)kip->insns &&
> > +		    addr < (unsigned long)kip->insns + PAGE_SIZE) {
> > +			ret = true;
> > +			break;
> > +		}
> > +	}
> > +	rcu_read_unlock();
> > +
> > +	return ret;
> > +}
> 
> How many entries should one expect on that list? I spend quite a bit of
> time reducing the cost of is_module_text_address() a while back and see
> that both ftrace (which actually needs this to be fast) and now
> kprobes have linear list walks in here.

It depends on how many probes are used and optimized. However, in most
cases, there should be one entry (unless user defines optimized probes
over 32 on x86, from my experience, it is very rare case. :) )

> I'm assuming the ftrace thing to be mostly empty, since I never saw it
> on my benchmarks back then, but it is something Steve should look at I
> suppose.
> 
> Similarly, the changelog here should include some talk about worst case
> costs.

Would you have any good benchmark to measure it?

Thanks,

> 
> FWIW, see commit: 93c2e105f6bc ("module: Optimize __module_address() using a latched RB-tree")


-- 
Masami Hiramatsu <mhiramat@kernel.org>

^ permalink raw reply

* Re: [PATCH 2/7] mm, vmscan: add active list aging tracepoint
From: Minchan Kim @ 2017-01-04  5:07 UTC (permalink / raw)
  To: Michal Hocko
  Cc: Hillf Danton, linux-mm, Andrew Morton, Mel Gorman,
	Johannes Weiner, Vlastimil Babka, Rik van Riel, LKML
In-Reply-To: <20170103082122.GA30111@dhcp22.suse.cz>

On Tue, Jan 03, 2017 at 09:21:22AM +0100, Michal Hocko wrote:
> On Tue 03-01-17 14:03:28, Minchan Kim wrote:
> > Hi Michal,
> > 
> > On Fri, Dec 30, 2016 at 05:37:42PM +0100, Michal Hocko wrote:
> > > On Sat 31-12-16 01:04:56, Minchan Kim wrote:
> > > [...]
> > > > > From 5f1bc22ad1e54050b4da3228d68945e70342ebb6 Mon Sep 17 00:00:00 2001
> > > > > From: Michal Hocko <mhocko@suse.com>
> > > > > Date: Tue, 27 Dec 2016 13:18:20 +0100
> > > > > Subject: [PATCH] mm, vmscan: add active list aging tracepoint
> > > > > 
> > > > > Our reclaim process has several tracepoints to tell us more about how
> > > > > things are progressing. We are, however, missing a tracepoint to track
> > > > > active list aging. Introduce mm_vmscan_lru_shrink_active which reports
> > > > 
> > > > I agree this part.
> > > > 
> > > > > the number of
> > > > > 	- nr_scanned, nr_taken pages to tell us the LRU isolation
> > > > > 	  effectiveness.
> > > > 
> > > > I agree nr_taken for knowing shrinking effectiveness but don't
> > > > agree nr_scanned. If we want to know LRU isolation effectiveness
> > > > with nr_scanned and nr_taken, isolate_lru_pages will do.
> > > 
> > > Yes it will. On the other hand the number is there and there is no
> > > additional overhead, maintenance or otherwise, to provide that number.
> > 
> > You are adding some instructions, how can you imagine it's no overhead?
> 
> There should be close to zero overhead when the tracepoint is disabled
> (we pay only one more argument when the function is called). Is this
> really worth discussing in this cold path? We are talking about the
> reclaim here.

I am talking about that why we should add pointless code in there.
No matter it's overhead. We are looping infinite. Blindly, it adds
overhead although you might think so trivial.

> 
> > Let's say whether it's measurable. Although it's not big in particular case,
> > it would be measurable if everyone start to say like that "it's trivial so
> > what's the problem adding a few instructions although it was duplicated?"
> > 
> > You already said "LRU isolate effectiveness". It should be done in there,
> > isolate_lru_pages and we have been. You need another reasons if you want to
> > add the duplicated work, strongly.
> 
> isolate_lru_pages is certainly there but you have to enable a trace
> point for that. Sometimes it is quite useful to get a reasonably good
> picture even without all the vmscan tracepoints enabled because they
> can generate quite a lot of output. So if the counter is available I

If someone want to see "isolate effectivenss", he should enable
mm_vmscan_lru_isolate which was born in that and has more helpful
information.

Think it in an opposit way. If some users want to see just active
list aging problem and no interested in "LRU isolate effectivness",
you are adding meaningless output for him and he has no choice to
turn it off with your patch.

> see no reason to exclude it, especially when it can provide a useful
> information. One of the most frustrating debugging experience is when

I said several times. Please think over if everyone begins adding extra
parameters in every tracepoints which we could already get it via other
tracepoint with "just, it might be useful in a specific context".
Could you be happy with that, really?

> you are missing some part of the information and have to guess which
> part is that and patch, rebuild the kernel and hope to reproduce it
> again in the same/similar way.

No need to rebuild. Just enable mm_vmscan_lru_isolate.

> 
> There are two things about this and other tracepoint patches in general
> I believe. 1) Is the tracepoint useful? and 2) Do we have to go over
> extra hops to show tracepoint data?
> 
> I guess we are in an agreement that the answer for 1 is yes. And

yeb.

> regarding 2, all the data we are showing are there or trivially
> retrieved without touching _any_ hot path. Som of it might be duplicated


Currently, you rely on just unfortunate modulization to just add
unncessary information to the tracepoint.

I just removed nr_scanned in your patch and look below.

./scripts/bloat-o-meter vmlinux.old vmlinux.new
add/remove: 0/0 grow/shrink: 0/6 up/down: 0/-147 (-147)
function                                     old     new   delta
perf_trace_mm_vmscan_lru_shrink_active       264     256      -8
trace_raw_output_mm_vmscan_lru_shrink_active     203     193     -10
trace_event_raw_event_mm_vmscan_lru_shrink_active     241     225     -16
print_fmt_mm_vmscan_lru_shrink_active        458     426     -32
shrink_active_list                          1265    1232     -33
trace_event_define_fields_mm_vmscan_lru_shrink_active     384     336     -48
Total: Before=26268743, After=26268596, chg -0.00%

Let's furhter it more.

We can factor out logics to account isolation of LRU from shrink_[in]active_list
which is more clean, I think.

^ permalink raw reply

* Re: [PATCH 2/7] mm, vmscan: add active list aging tracepoint
From: Minchan Kim @ 2017-01-04  5:07 UTC (permalink / raw)
  To: Michal Hocko
  Cc: Hillf Danton, linux-mm, Andrew Morton, Mel Gorman,
	Johannes Weiner, Vlastimil Babka, Rik van Riel, LKML
In-Reply-To: <20170103082122.GA30111@dhcp22.suse.cz>

On Tue, Jan 03, 2017 at 09:21:22AM +0100, Michal Hocko wrote:
> On Tue 03-01-17 14:03:28, Minchan Kim wrote:
> > Hi Michal,
> > 
> > On Fri, Dec 30, 2016 at 05:37:42PM +0100, Michal Hocko wrote:
> > > On Sat 31-12-16 01:04:56, Minchan Kim wrote:
> > > [...]
> > > > > From 5f1bc22ad1e54050b4da3228d68945e70342ebb6 Mon Sep 17 00:00:00 2001
> > > > > From: Michal Hocko <mhocko@suse.com>
> > > > > Date: Tue, 27 Dec 2016 13:18:20 +0100
> > > > > Subject: [PATCH] mm, vmscan: add active list aging tracepoint
> > > > > 
> > > > > Our reclaim process has several tracepoints to tell us more about how
> > > > > things are progressing. We are, however, missing a tracepoint to track
> > > > > active list aging. Introduce mm_vmscan_lru_shrink_active which reports
> > > > 
> > > > I agree this part.
> > > > 
> > > > > the number of
> > > > > 	- nr_scanned, nr_taken pages to tell us the LRU isolation
> > > > > 	  effectiveness.
> > > > 
> > > > I agree nr_taken for knowing shrinking effectiveness but don't
> > > > agree nr_scanned. If we want to know LRU isolation effectiveness
> > > > with nr_scanned and nr_taken, isolate_lru_pages will do.
> > > 
> > > Yes it will. On the other hand the number is there and there is no
> > > additional overhead, maintenance or otherwise, to provide that number.
> > 
> > You are adding some instructions, how can you imagine it's no overhead?
> 
> There should be close to zero overhead when the tracepoint is disabled
> (we pay only one more argument when the function is called). Is this
> really worth discussing in this cold path? We are talking about the
> reclaim here.

I am talking about that why we should add pointless code in there.
No matter it's overhead. We are looping infinite. Blindly, it adds
overhead although you might think so trivial.

> 
> > Let's say whether it's measurable. Although it's not big in particular case,
> > it would be measurable if everyone start to say like that "it's trivial so
> > what's the problem adding a few instructions although it was duplicated?"
> > 
> > You already said "LRU isolate effectiveness". It should be done in there,
> > isolate_lru_pages and we have been. You need another reasons if you want to
> > add the duplicated work, strongly.
> 
> isolate_lru_pages is certainly there but you have to enable a trace
> point for that. Sometimes it is quite useful to get a reasonably good
> picture even without all the vmscan tracepoints enabled because they
> can generate quite a lot of output. So if the counter is available I

If someone want to see "isolate effectivenss", he should enable
mm_vmscan_lru_isolate which was born in that and has more helpful
information.

Think it in an opposit way. If some users want to see just active
list aging problem and no interested in "LRU isolate effectivness",
you are adding meaningless output for him and he has no choice to
turn it off with your patch.

> see no reason to exclude it, especially when it can provide a useful
> information. One of the most frustrating debugging experience is when

I said several times. Please think over if everyone begins adding extra
parameters in every tracepoints which we could already get it via other
tracepoint with "just, it might be useful in a specific context".
Could you be happy with that, really?

> you are missing some part of the information and have to guess which
> part is that and patch, rebuild the kernel and hope to reproduce it
> again in the same/similar way.

No need to rebuild. Just enable mm_vmscan_lru_isolate.

> 
> There are two things about this and other tracepoint patches in general
> I believe. 1) Is the tracepoint useful? and 2) Do we have to go over
> extra hops to show tracepoint data?
> 
> I guess we are in an agreement that the answer for 1 is yes. And

yeb.

> regarding 2, all the data we are showing are there or trivially
> retrieved without touching _any_ hot path. Som of it might be duplicated


Currently, you rely on just unfortunate modulization to just add
unncessary information to the tracepoint.

I just removed nr_scanned in your patch and look below.

./scripts/bloat-o-meter vmlinux.old vmlinux.new
add/remove: 0/0 grow/shrink: 0/6 up/down: 0/-147 (-147)
function                                     old     new   delta
perf_trace_mm_vmscan_lru_shrink_active       264     256      -8
trace_raw_output_mm_vmscan_lru_shrink_active     203     193     -10
trace_event_raw_event_mm_vmscan_lru_shrink_active     241     225     -16
print_fmt_mm_vmscan_lru_shrink_active        458     426     -32
shrink_active_list                          1265    1232     -33
trace_event_define_fields_mm_vmscan_lru_shrink_active     384     336     -48
Total: Before=26268743, After=26268596, chg -0.00%

Let's furhter it more.

We can factor out logics to account isolation of LRU from shrink_[in]active_list
which is more clean, I think.

>From 1053968d526427ecad96b682aa586701c4ecfc84 Mon Sep 17 00:00:00 2001
From: Minchan Kim <minchan@kernel.org>
Date: Wed, 4 Jan 2017 10:04:36 +0900
Subject: [PATCH] factor out LRU isolation accounting.

Not-yet-signed-off-by: Minchan Kim <minchan@kernel.org>
---
 include/trace/events/vmscan.h | 14 +++++----
 mm/vmscan.c                   | 68 ++++++++++++++++++-------------------------
 2 files changed, 37 insertions(+), 45 deletions(-)

diff --git a/include/trace/events/vmscan.h b/include/trace/events/vmscan.h
index 79b3cd9c7048..5fc3a94a14cd 100644
--- a/include/trace/events/vmscan.h
+++ b/include/trace/events/vmscan.h
@@ -364,14 +364,15 @@ TRACE_EVENT(mm_vmscan_writepage,
 TRACE_EVENT(mm_vmscan_lru_shrink_inactive,
 
 	TP_PROTO(int nid,
-		unsigned long nr_scanned, unsigned long nr_reclaimed,
+		unsigned long nr_taken,
+		unsigned long nr_reclaimed,
 		int priority, int file),
 
-	TP_ARGS(nid, nr_scanned, nr_reclaimed, priority, file),
+	TP_ARGS(nid, nr_taken, nr_reclaimed, priority, file),
 
 	TP_STRUCT__entry(
 		__field(int, nid)
-		__field(unsigned long, nr_scanned)
+		__field(unsigned long, nr_taken)
 		__field(unsigned long, nr_reclaimed)
 		__field(int, priority)
 		__field(int, reclaim_flags)
@@ -379,15 +380,16 @@ TRACE_EVENT(mm_vmscan_lru_shrink_inactive,
 
 	TP_fast_assign(
 		__entry->nid = nid;
-		__entry->nr_scanned = nr_scanned;
+		__entry->nr_taken = nr_taken;
 		__entry->nr_reclaimed = nr_reclaimed;
 		__entry->priority = priority;
 		__entry->reclaim_flags = trace_shrink_flags(file);
 	),
 
-	TP_printk("nid=%d nr_scanned=%ld nr_reclaimed=%ld priority=%d flags=%s",
+	TP_printk("nid=%d nr_taken=%ld nr_reclaimed=%ld priority=%d flags=%s",
 		__entry->nid,
-		__entry->nr_scanned, __entry->nr_reclaimed,
+		__entry->nr_taken,
+		__entry->nr_reclaimed,
 		__entry->priority,
 		show_reclaim_flags(__entry->reclaim_flags))
 );
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 37ccd4e0b349..74f55f39f963 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -1454,16 +1454,16 @@ static __always_inline void update_lru_sizes(struct lruvec *lruvec,
  * @nr_to_scan:	The number of pages to look through on the list.
  * @lruvec:	The LRU vector to pull pages from.
  * @dst:	The temp list to put pages on to.
- * @nr_scanned:	The number of pages that were scanned.
  * @sc:		The scan_control struct for this reclaim session
  * @mode:	One of the LRU isolation modes
  * @lru:	LRU list id for isolating
  *
  * returns how many pages were moved onto *@dst.
  */
-static unsigned long isolate_lru_pages(unsigned long nr_to_scan,
+static unsigned long isolate_lru_pages(struct pglist_data *pgdat,
+		unsigned long nr_to_scan,
 		struct lruvec *lruvec, struct list_head *dst,
-		unsigned long *nr_scanned, struct scan_control *sc,
+		struct scan_control *sc,
 		isolate_mode_t mode, enum lru_list lru)
 {
 	struct list_head *src = &lruvec->lists[lru];
@@ -1471,8 +1471,11 @@ static unsigned long isolate_lru_pages(unsigned long nr_to_scan,
 	unsigned long nr_zone_taken[MAX_NR_ZONES] = { 0 };
 	unsigned long nr_skipped[MAX_NR_ZONES] = { 0, };
 	unsigned long scan, nr_pages;
+	struct zone_reclaim_stat *reclaim_stat = &lruvec->reclaim_stat;
 	LIST_HEAD(pages_skipped);
+	int file = is_file_lru(lru);
 
+	spin_lock_irq(&pgdat->lru_lock);
 	for (scan = 0; scan < nr_to_scan && nr_taken < nr_to_scan &&
 					!list_empty(src);) {
 		struct page *page;
@@ -1540,10 +1543,25 @@ static unsigned long isolate_lru_pages(unsigned long nr_to_scan,
 
 		list_splice(&pages_skipped, src);
 	}
-	*nr_scanned = scan;
 	trace_mm_vmscan_lru_isolate(sc->reclaim_idx, sc->order, nr_to_scan, scan,
 				    nr_taken, mode, is_file_lru(lru));
 	update_lru_sizes(lruvec, lru, nr_zone_taken, nr_taken);
+
+	__mod_node_page_state(pgdat, NR_ISOLATED_ANON + file, nr_taken);
+	reclaim_stat->recent_scanned[file] += nr_taken;
+
+	if (global_reclaim(sc))
+		__mod_node_page_state(pgdat, NR_PAGES_SCANNED, scan);
+	if (is_active_lru(lru)) {
+		__count_vm_events(PGREFILL, scan);
+	} else {
+		if (current_is_kswapd())
+			__count_vm_events(PGSCAN_KSWAPD, scan);
+		else
+			__count_vm_events(PGSCAN_DIRECT, scan);
+	}
+	spin_unlock_irq(&pgdat->lru_lock);
+
 	return nr_taken;
 }
 
@@ -1735,7 +1753,6 @@ shrink_inactive_list(unsigned long nr_to_scan, struct lruvec *lruvec,
 		     struct scan_control *sc, enum lru_list lru)
 {
 	LIST_HEAD(page_list);
-	unsigned long nr_scanned;
 	unsigned long nr_reclaimed = 0;
 	unsigned long nr_taken;
 	unsigned long nr_dirty = 0;
@@ -1746,7 +1763,6 @@ shrink_inactive_list(unsigned long nr_to_scan, struct lruvec *lruvec,
 	isolate_mode_t isolate_mode = 0;
 	int file = is_file_lru(lru);
 	struct pglist_data *pgdat = lruvec_pgdat(lruvec);
-	struct zone_reclaim_stat *reclaim_stat = &lruvec->reclaim_stat;
 
 	if (!inactive_reclaimable_pages(lruvec, sc, lru))
 		return 0;
@@ -1766,23 +1782,8 @@ shrink_inactive_list(unsigned long nr_to_scan, struct lruvec *lruvec,
 	if (!sc->may_writepage)
 		isolate_mode |= ISOLATE_CLEAN;
 
-	spin_lock_irq(&pgdat->lru_lock);
-
-	nr_taken = isolate_lru_pages(nr_to_scan, lruvec, &page_list,
-				     &nr_scanned, sc, isolate_mode, lru);
-
-	__mod_node_page_state(pgdat, NR_ISOLATED_ANON + file, nr_taken);
-	reclaim_stat->recent_scanned[file] += nr_taken;
-
-	if (global_reclaim(sc)) {
-		__mod_node_page_state(pgdat, NR_PAGES_SCANNED, nr_scanned);
-		if (current_is_kswapd())
-			__count_vm_events(PGSCAN_KSWAPD, nr_scanned);
-		else
-			__count_vm_events(PGSCAN_DIRECT, nr_scanned);
-	}
-	spin_unlock_irq(&pgdat->lru_lock);
-
+	nr_taken = isolate_lru_pages(pgdat, nr_to_scan, lruvec, &page_list,
+					sc, isolate_mode, lru);
 	if (nr_taken == 0)
 		return 0;
 
@@ -1866,7 +1867,8 @@ shrink_inactive_list(unsigned long nr_to_scan, struct lruvec *lruvec,
 		wait_iff_congested(pgdat, BLK_RW_ASYNC, HZ/10);
 
 	trace_mm_vmscan_lru_shrink_inactive(pgdat->node_id,
-			nr_scanned, nr_reclaimed,
+			nr_taken,
+			nr_reclaimed,
 			sc->priority, file);
 	return nr_reclaimed;
 }
@@ -1943,18 +1945,17 @@ static void shrink_active_list(unsigned long nr_to_scan,
 			       enum lru_list lru)
 {
 	unsigned long nr_taken;
-	unsigned long nr_scanned;
 	unsigned long vm_flags;
 	LIST_HEAD(l_hold);	/* The pages which were snipped off */
 	LIST_HEAD(l_active);
 	LIST_HEAD(l_inactive);
 	struct page *page;
-	struct zone_reclaim_stat *reclaim_stat = &lruvec->reclaim_stat;
 	unsigned nr_deactivate, nr_activate;
 	unsigned nr_rotated = 0;
 	isolate_mode_t isolate_mode = 0;
 	int file = is_file_lru(lru);
 	struct pglist_data *pgdat = lruvec_pgdat(lruvec);
+	struct zone_reclaim_stat *reclaim_stat = &lruvec->reclaim_stat;
 
 	lru_add_drain();
 
@@ -1963,19 +1964,8 @@ static void shrink_active_list(unsigned long nr_to_scan,
 	if (!sc->may_writepage)
 		isolate_mode |= ISOLATE_CLEAN;
 
-	spin_lock_irq(&pgdat->lru_lock);
-
-	nr_taken = isolate_lru_pages(nr_to_scan, lruvec, &l_hold,
-				     &nr_scanned, sc, isolate_mode, lru);
-
-	__mod_node_page_state(pgdat, NR_ISOLATED_ANON + file, nr_taken);
-	reclaim_stat->recent_scanned[file] += nr_taken;
-
-	if (global_reclaim(sc))
-		__mod_node_page_state(pgdat, NR_PAGES_SCANNED, nr_scanned);
-	__count_vm_events(PGREFILL, nr_scanned);
-
-	spin_unlock_irq(&pgdat->lru_lock);
+	nr_taken = isolate_lru_pages(pgdat, nr_to_scan, lruvec, &l_hold,
+				     sc, isolate_mode, lru);
 
 	while (!list_empty(&l_hold)) {
 		cond_resched();
-- 
2.7.4

With this,
./scripts/bloat-o-meter vmlinux.old vmlinux.new.new
add/remove: 1/1 grow/shrink: 0/9 up/down: 1394/-1636 (-242)
function                                     old     new   delta
isolate_lru_pages                              -    1394   +1394
print_fmt_mm_vmscan_lru_shrink_inactive      359     355      -4
vermagic                                      64      58      -6
perf_trace_mm_vmscan_lru_shrink_active       264     256      -8
trace_raw_output_mm_vmscan_lru_shrink_active     203     193     -10
trace_event_raw_event_mm_vmscan_lru_shrink_active     241     225     -16
print_fmt_mm_vmscan_lru_shrink_active        458     426     -32
trace_event_define_fields_mm_vmscan_lru_shrink_active     384     336     -48
shrink_inactive_list                        1430    1271    -159
shrink_active_list                          1265    1082    -183
isolate_lru_pages.isra                      1170       -   -1170
Total: Before=26268743, After=26268501, chg -0.00%

We can save 242 bytes.

If we consider binary size, 424 bytes save.

#> ls -l vmlinux.old vmlinux.new.new
194092840  vmlinux.old
194092416  vmlinux.new.new

> with other tracepoints but that can be helpful because you do not have
> all the tracepoints enabled all the time. So unless you see this
> particular thing as a road block I would rather keep it.

I didn't know how long this thread becomes lenghy. To me, it was no worth
to discuss. I did best effot to explain my stand with valid points, I think
and don't want to go infinite loop. If you don't agree still, separate
the patch. One includes only necessary things with removing nr_scanned, which
I am happy to ack it. Based upon it, add one more patch you want adding
nr_scanned with your claim. I will reply that thread with my claim and
let's keep an eye on it that whether maintainer will take it or not.
If maintainer will take it, it's good indication which will represent
we can add more extra tracepoint easily with "might be helpful with someone
although it's redunant" so do not prevent others who want to do
in the future.

>  
> > > The inactive counterpart does that for quite some time already. So why
> > 
> > It couldn't be a reason. If it was duplicated in there, it would be
> > better to fix it rather than adding more duplciated work to match both
> > sides.
> 
> I really do not see this as a bad thing.
> 
> > > exactly does that matter? Don't take me wrong but isn't this more on a
> > > nit picking side than necessary? Or do I just misunderstand your
> > > concenrs? It is not like we are providing a stable user API as the
> > 
> > My concern is that I don't see what we can get benefit from those
> > duplicated work. If it doesn't give benefit to us, I don't want to add.
> > I hope you think another reasonable reasons.
> > 
> > > tracepoint is clearly implementation specific and not something to be
> > > used for anything other than debugging.
> > 
> > My point is we already had things "LRU isolation effectivness". Namely,
> > isolate_lru_pages.
> > 
> > > 
> > > > > 	- nr_rotated pages which tells us that we are hitting referenced
> > > > > 	  pages which are deactivated. If this is a large part of the
> > > > > 	  reported nr_deactivated pages then the active list is too small
> > > > 
> > > > It might be but not exactly. If your goal is to know LRU size, it can be
> > > > done in get_scan_count. I tend to agree LRU size is helpful for
> > > > performance analysis because decreased LRU size signals memory shortage
> > > > then performance drop.
> > > 
> > > No, I am not really interested in the exact size but rather to allow to
> > > find whether we are aging the active list too early...
> > 
> > Could you elaborate it more that how we can get active list early aging
> > with nr_rotated?
> 
> If you see too many referenced pages on the active list then they have
> been used since promoted and that is an indication that they might be
> reclaimed too early. If you are debugging a performance issue and see
> this happening then it might be a good indication to look at.

This is better than "active list is too small". I hope you change
description with this.

^ permalink raw reply related

* Re: [PATCH v4 3/4] powerpc/mm: add radix__remove_section_mapping()
From: Aneesh Kumar K.V @ 2017-01-04  5:07 UTC (permalink / raw)
  To: Reza Arbab, Michael Ellerman, Benjamin Herrenschmidt,
	Paul Mackerras
  Cc: linuxppc-dev, Balbir Singh, Alistair Popple
In-Reply-To: <1483476218-17271-4-git-send-email-arbab@linux.vnet.ibm.com>

Reza Arbab <arbab@linux.vnet.ibm.com> writes:

> Tear down and free the four-level page tables of physical mappings
> during memory hotremove.
>
> Borrow the basic structure of remove_pagetable() and friends from the
> identically-named x86 functions. Simplify things a bit so locking and
> tlb flushing are only done in the outermost function.
>
> Memory must be offline to be removed, thus not in use. So there
> shouldn't be the sort of concurrent page walking activity here that
> might prompt us to use RCU.
>
> Signed-off-by: Reza Arbab <arbab@linux.vnet.ibm.com>
> ---
>  arch/powerpc/include/asm/book3s/64/radix.h |   1 +
>  arch/powerpc/mm/pgtable-book3s64.c         |   2 +-
>  arch/powerpc/mm/pgtable-radix.c            | 149 +++++++++++++++++++++++++++++
>  3 files changed, 151 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/include/asm/book3s/64/radix.h b/arch/powerpc/include/asm/book3s/64/radix.h
> index 43c2571..0032b66 100644
> --- a/arch/powerpc/include/asm/book3s/64/radix.h
> +++ b/arch/powerpc/include/asm/book3s/64/radix.h
> @@ -294,6 +294,7 @@ static inline unsigned long radix__get_tree_size(void)
>
>  #ifdef CONFIG_MEMORY_HOTPLUG
>  int radix__create_section_mapping(unsigned long start, unsigned long end);
> +int radix__remove_section_mapping(unsigned long start, unsigned long end);
>  #endif /* CONFIG_MEMORY_HOTPLUG */
>  #endif /* __ASSEMBLY__ */
>  #endif
> diff --git a/arch/powerpc/mm/pgtable-book3s64.c b/arch/powerpc/mm/pgtable-book3s64.c
> index 2b13f6b..b798ff6 100644
> --- a/arch/powerpc/mm/pgtable-book3s64.c
> +++ b/arch/powerpc/mm/pgtable-book3s64.c
> @@ -139,7 +139,7 @@ int create_section_mapping(unsigned long start, unsigned long end)
>  int remove_section_mapping(unsigned long start, unsigned long end)
>  {
>  	if (radix_enabled())
> -		return -ENODEV;
> +		return radix__remove_section_mapping(start, end);
>
>  	return hash__remove_section_mapping(start, end);
>  }
> diff --git a/arch/powerpc/mm/pgtable-radix.c b/arch/powerpc/mm/pgtable-radix.c
> index 3588895..f7a8e625 100644
> --- a/arch/powerpc/mm/pgtable-radix.c
> +++ b/arch/powerpc/mm/pgtable-radix.c
> @@ -457,10 +457,159 @@ void radix__setup_initial_memory_limit(phys_addr_t first_memblock_base,
>  }
>
>  #ifdef CONFIG_MEMORY_HOTPLUG
> +static void free_pte_table(pte_t *pte_start, pmd_t *pmd)
> +{
> +	pte_t *pte;
> +	int i;
> +
> +	for (i = 0; i < PTRS_PER_PTE; i++) {
> +		pte = pte_start + i;
> +		if (!pte_none(*pte))
> +			return;
> +	}
> +
> +	pte_free_kernel(&init_mm, pte_start);
> +	pmd_clear(pmd);
> +}
> +
> +static void free_pmd_table(pmd_t *pmd_start, pud_t *pud)
> +{
> +	pmd_t *pmd;
> +	int i;
> +
> +	for (i = 0; i < PTRS_PER_PMD; i++) {
> +		pmd = pmd_start + i;
> +		if (!pmd_none(*pmd))
> +			return;
> +	}
> +
> +	pmd_free(&init_mm, pmd_start);
> +	pud_clear(pud);
> +}
> +
> +static void free_pud_table(pud_t *pud_start, pgd_t *pgd)
> +{
> +	pud_t *pud;
> +	int i;
> +
> +	for (i = 0; i < PTRS_PER_PUD; i++) {
> +		pud = pud_start + i;
> +		if (!pud_none(*pud))
> +			return;
> +	}
> +
> +	pud_free(&init_mm, pud_start);
> +	pgd_clear(pgd);
> +}
> +
> +static void remove_pte_table(pte_t *pte_start, unsigned long addr,
> +			     unsigned long end)
> +{
> +	unsigned long next;
> +	pte_t *pte;
> +
> +	pte = pte_start + pte_index(addr);
> +	for (; addr < end; addr = next, pte++) {
> +		next = (addr + PAGE_SIZE) & PAGE_MASK;
> +		if (next > end)
> +			next = end;
> +
> +		if (!pte_present(*pte))
> +			continue;
> +
> +		pte_clear(&init_mm, addr, pte);
> +	}
> +}
> +
> +static void remove_pmd_table(pmd_t *pmd_start, unsigned long addr,
> +			     unsigned long end)
> +{
> +	unsigned long next;
> +	pte_t *pte_base;
> +	pmd_t *pmd;
> +
> +	pmd = pmd_start + pmd_index(addr);
> +	for (; addr < end; addr = next, pmd++) {
> +		next = pmd_addr_end(addr, end);
> +
> +		if (!pmd_present(*pmd))
> +			continue;
> +
> +		if (pmd_huge(*pmd)) {
> +			pte_clear(&init_mm, addr, (pte_t *)pmd);
> +			continue;
> +		}
> +
> +		pte_base = (pte_t *)pmd_page_vaddr(*pmd);
> +		remove_pte_table(pte_base, addr, next);
> +		free_pte_table(pte_base, pmd);
> +	}
> +}
> +
> +static void remove_pud_table(pud_t *pud_start, unsigned long addr,
> +			     unsigned long end)
> +{
> +	unsigned long next;
> +	pmd_t *pmd_base;
> +	pud_t *pud;
> +
> +	pud = pud_start + pud_index(addr);
> +	for (; addr < end; addr = next, pud++) {
> +		next = pud_addr_end(addr, end);
> +
> +		if (!pud_present(*pud))
> +			continue;
> +
> +		if (pud_huge(*pud)) {
> +			pte_clear(&init_mm, addr, (pte_t *)pud);
> +			continue;
> +		}
> +
> +		pmd_base = (pmd_t *)pud_page_vaddr(*pud);
> +		remove_pmd_table(pmd_base, addr, next);
> +		free_pmd_table(pmd_base, pud);
> +	}
> +}
> +
> +static void remove_pagetable(unsigned long start, unsigned long end)
> +{
> +	unsigned long addr, next;
> +	pud_t *pud_base;
> +	pgd_t *pgd;
> +
> +	spin_lock(&init_mm.page_table_lock);
> +
> +	for (addr = start; addr < end; addr = next) {
> +		next = pgd_addr_end(addr, end);
> +
> +		pgd = pgd_offset_k(addr);
> +		if (!pgd_present(*pgd))
> +			continue;
> +
> +		if (pgd_huge(*pgd)) {
> +			pte_clear(&init_mm, addr, (pte_t *)pgd);
> +			continue;
> +		}
> +
> +		pud_base = (pud_t *)pgd_page_vaddr(*pgd);
> +		remove_pud_table(pud_base, addr, next);
> +		free_pud_table(pud_base, pgd);
> +	}
> +
> +	spin_unlock(&init_mm.page_table_lock);

What is this lock protecting ?


> +	flush_tlb_kernel_range(start, end);

We can use radix__flush_tlb_kernel_range avoiding an if
(radix_enabled()) conditional ? Also if needed we could make all the
above take a radix__ prefix ?


> +}
> +
>  int __ref radix__create_section_mapping(unsigned long start, unsigned long end)
>  {
>  	return create_physical_mapping(start, end);
>  }
> +
> +int radix__remove_section_mapping(unsigned long start, unsigned long end)
> +{
> +	remove_pagetable(start, end);
> +	return 0;
> +}
>  #endif /* CONFIG_MEMORY_HOTPLUG */
>
>  #ifdef CONFIG_SPARSEMEM_VMEMMAP
> -- 
> 1.8.3.1

^ permalink raw reply

* [PATCH] ARM: dts: am335x-icev2: Remove the duplicated pinmux setting
From: Lokesh Vutla @ 2017-01-04  5:09 UTC (permalink / raw)
  To: Linux OMAP Mailing List, Tony Lindgren
  Cc: robh+dt-DgEjT+Ai2ygdnm+yROfE0A, devicetree-u79uwXL29TY76Z2rM5mHXA,
	Tero Kristo, Sekhar Nori, Linux ARM Mailing List, Lokesh Vutla

There is no mmc sd card detect on am335x-ice board. But the spi0_cs1
pin being configured as mmcsd_cd. Removing it fixes the below warning
during boot.

[1.401303] pinctrl-single 44e10800.pinmux: pin 44e10960.0 already requested by 48030000.spi; cannot claim for 48060000.mmc

Signed-off-by: Lokesh Vutla <lokeshvutla-l0cyMroinI0@public.gmane.org>
---
Logs:
Before this patch: http://pastebin.ubuntu.com/23737509/
After this patch: http://pastebin.ubuntu.com/23737467/

 arch/arm/boot/dts/am335x-icev2.dts | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm/boot/dts/am335x-icev2.dts b/arch/arm/boot/dts/am335x-icev2.dts
index 1463df3b5b19..8ed46f9d79b7 100644
--- a/arch/arm/boot/dts/am335x-icev2.dts
+++ b/arch/arm/boot/dts/am335x-icev2.dts
@@ -170,7 +170,6 @@
 			AM33XX_IOPAD(0x8fc, PIN_INPUT_PULLUP | MUX_MODE0) /* (G16) mmc0_dat0.mmc0_dat0 */
 			AM33XX_IOPAD(0x900, PIN_INPUT_PULLUP | MUX_MODE0) /* (G17) mmc0_clk.mmc0_clk */
 			AM33XX_IOPAD(0x904, PIN_INPUT_PULLUP | MUX_MODE0) /* (G18) mmc0_cmd.mmc0_cmd */
-			AM33XX_IOPAD(0x960, PIN_INPUT_PULLUP | MUX_MODE5) /* (C15) spi0_cs1.mmc0_sdcd */
 		>;
 	};
 
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related

* [PATCH] ARM: dts: am335x-icev2: Remove the duplicated pinmux setting
From: Lokesh Vutla @ 2017-01-04  5:09 UTC (permalink / raw)
  To: linux-arm-kernel

There is no mmc sd card detect on am335x-ice board. But the spi0_cs1
pin being configured as mmcsd_cd. Removing it fixes the below warning
during boot.

[1.401303] pinctrl-single 44e10800.pinmux: pin 44e10960.0 already requested by 48030000.spi; cannot claim for 48060000.mmc

Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
---
Logs:
Before this patch: http://pastebin.ubuntu.com/23737509/
After this patch: http://pastebin.ubuntu.com/23737467/

 arch/arm/boot/dts/am335x-icev2.dts | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm/boot/dts/am335x-icev2.dts b/arch/arm/boot/dts/am335x-icev2.dts
index 1463df3b5b19..8ed46f9d79b7 100644
--- a/arch/arm/boot/dts/am335x-icev2.dts
+++ b/arch/arm/boot/dts/am335x-icev2.dts
@@ -170,7 +170,6 @@
 			AM33XX_IOPAD(0x8fc, PIN_INPUT_PULLUP | MUX_MODE0) /* (G16) mmc0_dat0.mmc0_dat0 */
 			AM33XX_IOPAD(0x900, PIN_INPUT_PULLUP | MUX_MODE0) /* (G17) mmc0_clk.mmc0_clk */
 			AM33XX_IOPAD(0x904, PIN_INPUT_PULLUP | MUX_MODE0) /* (G18) mmc0_cmd.mmc0_cmd */
-			AM33XX_IOPAD(0x960, PIN_INPUT_PULLUP | MUX_MODE5) /* (C15) spi0_cs1.mmc0_sdcd */
 		>;
 	};
 
-- 
2.11.0

^ permalink raw reply related

* Re: [PATCH v2 1/1] block: fix blk_queue_split() resource exhaustion
From: NeilBrown @ 2017-01-04  5:12 UTC (permalink / raw)
  To: Jack Wang, Lars Ellenberg
  Cc: Jens Axboe, linux-raid, Michael Wang, Mike Snitzer,
	Peter Zijlstra, Jiri Kosina, Ming Lei, linux-kernel, Zheng Liu,
	linux-block, Takashi Iwai, linux-bcache@vger.kernel.org,
	Ingo Molnar, Alasdair Kergon, Martin K. Petersen, Keith Busch,
	device-mapper development, Shaohua Li, Kent Overstreet,
	Kirill A. Shutemov, R
In-Reply-To: <CA+res+SjL6FrYWMroB-xjBO5gyz9pca6FQCy-arr7+4RQHeJGw@mail.gmail.com>


[-- Attachment #1.1: Type: text/plain, Size: 4832 bytes --]

On Tue, Jan 03 2017, Jack Wang wrote:

> 2016-12-23 12:45 GMT+01:00 Lars Ellenberg <lars.ellenberg@linbit.com>:
>> On Fri, Dec 23, 2016 at 09:49:53AM +0100, Michael Wang wrote:
>>> Dear Maintainers
>>>
>>> I'd like to ask for the status of this patch since we hit the
>>> issue too during our testing on md raid1.
>>>
>>> Split remainder bio_A was queued ahead, following by bio_B for
>>> lower device, at this moment raid start freezing, the loop take
>>> out bio_A firstly and deliver it, which will hung since raid is
>>> freezing, while the freezing never end since it waiting for
>>> bio_B to finish, and bio_B is still on the queue, waiting for
>>> bio_A to finish...
>>>
>>> We're looking for a good solution and we found this patch
>>> already progressed a lot, but we can't find it on linux-next,
>>> so we'd like to ask are we still planning to have this fix
>>> in upstream?
>>
>> I don't see why not, I'd even like to have it in older kernels,
>> but did not have the time and energy to push it.
>>
>> Thanks for the bump.
>>
>>         Lars
>>
> Hi folks,
>
> As Michael mentioned, we hit a bug this patch is trying to fix.
> Neil suggested another way to fix it.  I attached below.
> I personal prefer Neil's version as it's less code change, and straight forward.
>
> Could you share your comments, we can get one fix into mainline.
>
> Thanks,
> Jinpu
> From 69a4829a55503e496ce9c730d2c8e3dd8a08874a Mon Sep 17 00:00:00 2001
> From: NeilBrown <neilb@suse.com>
> Date: Wed, 14 Dec 2016 16:55:52 +0100
> Subject: [PATCH] block: fix deadlock between freeze_array() and wait_barrier()
>
> When we call wait_barrier, we might have some bios waiting
> in current->bio_list, which prevents the array_freeze call to
> complete. Those can only be internal READs, which have already
> passed the wait_barrier call (thus incrementing nr_pending), but
> still were not submitted to the lower level, due to generic_make_request
> logic to avoid recursive calls. In such case, we have a deadlock:
> - array_frozen is already set to 1, so wait_barrier unconditionally waits, so
> - internal READ bios will not be submitted, thus freeze_array will
> never completes.
>
> To fix this, modify generic_make_request to always sort bio_list_on_stack
> first with lowest level, then higher, until same level.
>
> Sent to linux-raid mail list:
> https://marc.info/?l=linux-raid&m=148232453107685&w=2
>

This should probably also have

  Inspired-by: Lars Ellenberg <lars.ellenberg@linbit.com>

or something that, as I was building on Lars' ideas when I wrote this.

It would also be worth noting in the description that this addresses
issues with dm and drbd as well as md.

In fact, I think that with this patch in place, much of the need for the
rescue_workqueue won't exist any more.  I cannot promise it can be
removed completely, but it should be to hard to make it optional and
only enabled for those few block devices that will still need it.
The rescuer should only be needed for a bioset which can be allocated
From twice in the one call the ->make_request_fn.  This would include
raid0 for example, though raid0_make_reqest could be re-written to not
use a loop and to just call generic_make_request(bio) if bio != split.

Thanks,
NeilBrown


> Suggested-by: NeilBrown <neilb@suse.com>
> Signed-off-by: Jack Wang <jinpu.wang@profitbricks.com>
> ---
>  block/blk-core.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 9e3ac56..47ef373 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -2138,10 +2138,30 @@ blk_qc_t generic_make_request(struct bio *bio)
>  		struct request_queue *q = bdev_get_queue(bio->bi_bdev);
>  
>  		if (likely(blk_queue_enter(q, __GFP_DIRECT_RECLAIM) == 0)) {
> +			struct bio_list lower, same, hold;
> +
> +			/* Create a fresh bio_list for all subordinate requests */
> +			bio_list_init(&hold);
> +			bio_list_merge(&hold, &bio_list_on_stack);
> +			bio_list_init(&bio_list_on_stack);
>  
>  			ret = q->make_request_fn(q, bio);
>  
>  			blk_queue_exit(q);
> +			/* sort new bios into those for a lower level
> +			 * and those for the same level
> +			 */
> +			bio_list_init(&lower);
> +			bio_list_init(&same);
> +			while ((bio = bio_list_pop(&bio_list_on_stack)) != NULL)
> +				if (q == bdev_get_queue(bio->bi_bdev))
> +					bio_list_add(&same, bio);
> +				else
> +					bio_list_add(&lower, bio);
> +			/* now assemble so we handle the lowest level first */
> +			bio_list_merge(&bio_list_on_stack, &lower);
> +			bio_list_merge(&bio_list_on_stack, &same);
> +			bio_list_merge(&bio_list_on_stack, &hold);
>  
>  			bio = bio_list_pop(current->bio_list);
>  		} else {
> -- 
> 2.7.4

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



^ permalink raw reply

* Re: [dm-devel] [PATCH v2 1/1] block: fix blk_queue_split() resource exhaustion
From: NeilBrown @ 2017-01-04  5:12 UTC (permalink / raw)
  To: Jack Wang, Lars Ellenberg
  Cc: Michael Wang, Jens Axboe, linux-block, Martin K. Petersen,
	Mike Snitzer, Peter Zijlstra, Jiri Kosina, Ming Lei,
	Kirill A. Shutemov, linux-kernel, linux-raid, Takashi Iwai,
	linux-bcache@vger.kernel.org, Zheng Liu, Kent Overstreet,
	Keith Busch, device-mapper development, Shaohua Li, Ingo Molnar,
	Alasdair Kergon, Roland Kammerer
In-Reply-To: <CA+res+SjL6FrYWMroB-xjBO5gyz9pca6FQCy-arr7+4RQHeJGw@mail.gmail.com>

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

On Tue, Jan 03 2017, Jack Wang wrote:

> 2016-12-23 12:45 GMT+01:00 Lars Ellenberg <lars.ellenberg@linbit.com>:
>> On Fri, Dec 23, 2016 at 09:49:53AM +0100, Michael Wang wrote:
>>> Dear Maintainers
>>>
>>> I'd like to ask for the status of this patch since we hit the
>>> issue too during our testing on md raid1.
>>>
>>> Split remainder bio_A was queued ahead, following by bio_B for
>>> lower device, at this moment raid start freezing, the loop take
>>> out bio_A firstly and deliver it, which will hung since raid is
>>> freezing, while the freezing never end since it waiting for
>>> bio_B to finish, and bio_B is still on the queue, waiting for
>>> bio_A to finish...
>>>
>>> We're looking for a good solution and we found this patch
>>> already progressed a lot, but we can't find it on linux-next,
>>> so we'd like to ask are we still planning to have this fix
>>> in upstream?
>>
>> I don't see why not, I'd even like to have it in older kernels,
>> but did not have the time and energy to push it.
>>
>> Thanks for the bump.
>>
>>         Lars
>>
> Hi folks,
>
> As Michael mentioned, we hit a bug this patch is trying to fix.
> Neil suggested another way to fix it.  I attached below.
> I personal prefer Neil's version as it's less code change, and straight forward.
>
> Could you share your comments, we can get one fix into mainline.
>
> Thanks,
> Jinpu
> From 69a4829a55503e496ce9c730d2c8e3dd8a08874a Mon Sep 17 00:00:00 2001
> From: NeilBrown <neilb@suse.com>
> Date: Wed, 14 Dec 2016 16:55:52 +0100
> Subject: [PATCH] block: fix deadlock between freeze_array() and wait_barrier()
>
> When we call wait_barrier, we might have some bios waiting
> in current->bio_list, which prevents the array_freeze call to
> complete. Those can only be internal READs, which have already
> passed the wait_barrier call (thus incrementing nr_pending), but
> still were not submitted to the lower level, due to generic_make_request
> logic to avoid recursive calls. In such case, we have a deadlock:
> - array_frozen is already set to 1, so wait_barrier unconditionally waits, so
> - internal READ bios will not be submitted, thus freeze_array will
> never completes.
>
> To fix this, modify generic_make_request to always sort bio_list_on_stack
> first with lowest level, then higher, until same level.
>
> Sent to linux-raid mail list:
> https://marc.info/?l=linux-raid&m=148232453107685&w=2
>

This should probably also have

  Inspired-by: Lars Ellenberg <lars.ellenberg@linbit.com>

or something that, as I was building on Lars' ideas when I wrote this.

It would also be worth noting in the description that this addresses
issues with dm and drbd as well as md.

In fact, I think that with this patch in place, much of the need for the
rescue_workqueue won't exist any more.  I cannot promise it can be
removed completely, but it should be to hard to make it optional and
only enabled for those few block devices that will still need it.
The rescuer should only be needed for a bioset which can be allocated
From twice in the one call the ->make_request_fn.  This would include
raid0 for example, though raid0_make_reqest could be re-written to not
use a loop and to just call generic_make_request(bio) if bio != split.

Thanks,
NeilBrown


> Suggested-by: NeilBrown <neilb@suse.com>
> Signed-off-by: Jack Wang <jinpu.wang@profitbricks.com>
> ---
>  block/blk-core.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 9e3ac56..47ef373 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -2138,10 +2138,30 @@ blk_qc_t generic_make_request(struct bio *bio)
>  		struct request_queue *q = bdev_get_queue(bio->bi_bdev);
>  
>  		if (likely(blk_queue_enter(q, __GFP_DIRECT_RECLAIM) == 0)) {
> +			struct bio_list lower, same, hold;
> +
> +			/* Create a fresh bio_list for all subordinate requests */
> +			bio_list_init(&hold);
> +			bio_list_merge(&hold, &bio_list_on_stack);
> +			bio_list_init(&bio_list_on_stack);
>  
>  			ret = q->make_request_fn(q, bio);
>  
>  			blk_queue_exit(q);
> +			/* sort new bios into those for a lower level
> +			 * and those for the same level
> +			 */
> +			bio_list_init(&lower);
> +			bio_list_init(&same);
> +			while ((bio = bio_list_pop(&bio_list_on_stack)) != NULL)
> +				if (q == bdev_get_queue(bio->bi_bdev))
> +					bio_list_add(&same, bio);
> +				else
> +					bio_list_add(&lower, bio);
> +			/* now assemble so we handle the lowest level first */
> +			bio_list_merge(&bio_list_on_stack, &lower);
> +			bio_list_merge(&bio_list_on_stack, &same);
> +			bio_list_merge(&bio_list_on_stack, &hold);
>  
>  			bio = bio_list_pop(current->bio_list);
>  		} else {
> -- 
> 2.7.4

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

^ permalink raw reply

* [PATCH] ASoC: rt5640: move DAC2 Power to rt5640_dapm_widgets
From: Bard Liao @ 2017-01-04  5:14 UTC (permalink / raw)
  To: broonie, lgirdwood
  Cc: oder_chiou, jack.yu, alsa-devel, lars, pierre-louis.bossart,
	shumingf, Bard Liao, flove

"DAC L2 Power" and "DAC R2 Power" are used by both rt5639 and rt5640.
But it was defined in rt5640_specific_dapm_widgets[]. Move them to
rt5640_dapm_widgets will let both rt5639 and rt5640 can use it.

Signed-off-by: Bard Liao <bardliao@realtek.com>
---
 sound/soc/codecs/rt5640.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/sound/soc/codecs/rt5640.c b/sound/soc/codecs/rt5640.c
index e29a6de..0f1b216 100644
--- a/sound/soc/codecs/rt5640.c
+++ b/sound/soc/codecs/rt5640.c
@@ -1227,6 +1227,10 @@ static int rt5640_hp_post_event(struct snd_soc_dapm_widget *w,
 		RT5640_PWR_DAC_L1_BIT, 0, NULL, 0),
 	SND_SOC_DAPM_SUPPLY("DAC R1 Power", RT5640_PWR_DIG1,
 		RT5640_PWR_DAC_R1_BIT, 0, NULL, 0),
+	SND_SOC_DAPM_SUPPLY("DAC L2 Power", RT5640_PWR_DIG1,
+		RT5640_PWR_DAC_L2_BIT, 0, NULL, 0),
+	SND_SOC_DAPM_SUPPLY("DAC R2 Power", RT5640_PWR_DIG1,
+		RT5640_PWR_DAC_R2_BIT, 0, NULL, 0),
 	/* SPK/OUT Mixer */
 	SND_SOC_DAPM_MIXER("SPK MIXL", RT5640_PWR_MIXER, RT5640_PWR_SM_L_BIT,
 		0, rt5640_spk_l_mix, ARRAY_SIZE(rt5640_spk_l_mix)),
@@ -1322,10 +1326,6 @@ static int rt5640_hp_post_event(struct snd_soc_dapm_widget *w,
 		rt5640_mono_mix, ARRAY_SIZE(rt5640_mono_mix)),
 	SND_SOC_DAPM_SUPPLY("Improve MONO Amp Drv", RT5640_PWR_ANLG1,
 		RT5640_PWR_MA_BIT, 0, NULL, 0),
-	SND_SOC_DAPM_SUPPLY("DAC L2 Power", RT5640_PWR_DIG1,
-		RT5640_PWR_DAC_L2_BIT, 0, NULL, 0),
-	SND_SOC_DAPM_SUPPLY("DAC R2 Power", RT5640_PWR_DIG1,
-		RT5640_PWR_DAC_R2_BIT, 0, NULL, 0),
 
 	SND_SOC_DAPM_OUTPUT("MONOP"),
 	SND_SOC_DAPM_OUTPUT("MONON"),
-- 
1.8.1.1.439.g50a6b54

^ permalink raw reply related

* [PATCH 1/9] powerpc: Move ARCH_DLINFO out of uapi
From: Benjamin Herrenschmidt @ 2017-01-04  5:15 UTC (permalink / raw)
  To: linuxppc-dev; +Cc: Steven Munroe, Benjamin Herrenschmidt

It's an kernel private macro, it doesn't belong there

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 arch/powerpc/include/asm/elf.h      | 22 ++++++++++++++++++++++
 arch/powerpc/include/uapi/asm/elf.h | 23 -----------------------
 2 files changed, 22 insertions(+), 23 deletions(-)

diff --git a/arch/powerpc/include/asm/elf.h b/arch/powerpc/include/asm/elf.h
index ee46ffe..730c27e 100644
--- a/arch/powerpc/include/asm/elf.h
+++ b/arch/powerpc/include/asm/elf.h
@@ -136,4 +136,26 @@ extern int arch_setup_additional_pages(struct linux_binprm *bprm,
 
 #endif /* CONFIG_SPU_BASE */
 
+/*
+ * The requirements here are:
+ * - keep the final alignment of sp (sp & 0xf)
+ * - make sure the 32-bit value at the first 16 byte aligned position of
+ *   AUXV is greater than 16 for glibc compatibility.
+ *   AT_IGNOREPPC is used for that.
+ * - for compatibility with glibc ARCH_DLINFO must always be defined on PPC,
+ *   even if DLINFO_ARCH_ITEMS goes to zero or is undefined.
+ * update AT_VECTOR_SIZE_ARCH if the number of NEW_AUX_ENT entries changes
+ */
+#define ARCH_DLINFO							\
+do {									\
+	/* Handle glibc compatibility. */				\
+	NEW_AUX_ENT(AT_IGNOREPPC, AT_IGNOREPPC);			\
+	NEW_AUX_ENT(AT_IGNOREPPC, AT_IGNOREPPC);			\
+	/* Cache size items */						\
+	NEW_AUX_ENT(AT_DCACHEBSIZE, dcache_bsize);			\
+	NEW_AUX_ENT(AT_ICACHEBSIZE, icache_bsize);			\
+	NEW_AUX_ENT(AT_UCACHEBSIZE, ucache_bsize);			\
+	VDSO_AUX_ENT(AT_SYSINFO_EHDR, current->mm->context.vdso_base);	\
+} while (0)
+
 #endif /* _ASM_POWERPC_ELF_H */
diff --git a/arch/powerpc/include/uapi/asm/elf.h b/arch/powerpc/include/uapi/asm/elf.h
index 3a9e44c..b2c6fdd 100644
--- a/arch/powerpc/include/uapi/asm/elf.h
+++ b/arch/powerpc/include/uapi/asm/elf.h
@@ -162,29 +162,6 @@ typedef elf_vrreg_t elf_vrregset_t32[ELF_NVRREG32];
 typedef elf_fpreg_t elf_vsrreghalf_t32[ELF_NVSRHALFREG];
 #endif
 
-
-/*
- * The requirements here are:
- * - keep the final alignment of sp (sp & 0xf)
- * - make sure the 32-bit value at the first 16 byte aligned position of
- *   AUXV is greater than 16 for glibc compatibility.
- *   AT_IGNOREPPC is used for that.
- * - for compatibility with glibc ARCH_DLINFO must always be defined on PPC,
- *   even if DLINFO_ARCH_ITEMS goes to zero or is undefined.
- * update AT_VECTOR_SIZE_ARCH if the number of NEW_AUX_ENT entries changes
- */
-#define ARCH_DLINFO							\
-do {									\
-	/* Handle glibc compatibility. */				\
-	NEW_AUX_ENT(AT_IGNOREPPC, AT_IGNOREPPC);			\
-	NEW_AUX_ENT(AT_IGNOREPPC, AT_IGNOREPPC);			\
-	/* Cache size items */						\
-	NEW_AUX_ENT(AT_DCACHEBSIZE, dcache_bsize);			\
-	NEW_AUX_ENT(AT_ICACHEBSIZE, icache_bsize);			\
-	NEW_AUX_ENT(AT_UCACHEBSIZE, ucache_bsize);			\
-	VDSO_AUX_ENT(AT_SYSINFO_EHDR, current->mm->context.vdso_base);	\
-} while (0)
-
 /* PowerPC64 relocations defined by the ABIs */
 #define R_PPC64_NONE    R_PPC_NONE
 #define R_PPC64_ADDR32  R_PPC_ADDR32  /* 32bit absolute address.  */
-- 
2.9.3

^ permalink raw reply related

* [PATCH 2/9] powerpc: Move {d, i, u}cache_bsize definitions to a common place
From: Benjamin Herrenschmidt @ 2017-01-04  5:15 UTC (permalink / raw)
  To: linuxppc-dev; +Cc: Steven Munroe, Benjamin Herrenschmidt
In-Reply-To: <20170104051535.9454-1-benh@kernel.crashing.org>

The variables are defined twice in setup_32.c and setup_64.c, do it
once in setup-common.c instead

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 arch/powerpc/kernel/setup-common.c | 9 +++++++++
 arch/powerpc/kernel/setup_32.c     | 8 --------
 arch/powerpc/kernel/setup_64.c     | 8 --------
 3 files changed, 9 insertions(+), 16 deletions(-)

diff --git a/arch/powerpc/kernel/setup-common.c b/arch/powerpc/kernel/setup-common.c
index dba265c..e0eeed4 100644
--- a/arch/powerpc/kernel/setup-common.c
+++ b/arch/powerpc/kernel/setup-common.c
@@ -87,6 +87,15 @@ EXPORT_SYMBOL(machine_id);
 int boot_cpuid = -1;
 EXPORT_SYMBOL_GPL(boot_cpuid);
 
+/*
+ * These are used in binfmt_elf.c to put aux entries on the stack
+ * for each elf executable being started.
+ */
+int dcache_bsize;
+int icache_bsize;
+int ucache_bsize;
+
+
 unsigned long klimit = (unsigned long) _end;
 
 /*
diff --git a/arch/powerpc/kernel/setup_32.c b/arch/powerpc/kernel/setup_32.c
index 24ec3ea..3b81742 100644
--- a/arch/powerpc/kernel/setup_32.c
+++ b/arch/powerpc/kernel/setup_32.c
@@ -53,14 +53,6 @@ unsigned int DMA_MODE_READ;
 unsigned int DMA_MODE_WRITE;
 
 /*
- * These are used in binfmt_elf.c to put aux entries on the stack
- * for each elf executable being started.
- */
-int dcache_bsize;
-int icache_bsize;
-int ucache_bsize;
-
-/*
  * We're called here very early in the boot.
  *
  * Note that the kernel may be running at an address which is different
diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
index 7ac8e6e..6263e0d 100644
--- a/arch/powerpc/kernel/setup_64.c
+++ b/arch/powerpc/kernel/setup_64.c
@@ -88,14 +88,6 @@ struct ppc64_caches ppc64_caches = {
 };
 EXPORT_SYMBOL_GPL(ppc64_caches);
 
-/*
- * These are used in binfmt_elf.c to put aux entries on the stack
- * for each elf executable being started.
- */
-int dcache_bsize;
-int icache_bsize;
-int ucache_bsize;
-
 #if defined(CONFIG_PPC_BOOK3E) && defined(CONFIG_SMP)
 void __init setup_tlb_core_data(void)
 {
-- 
2.9.3

^ permalink raw reply related

* [PATCH 3/9] powerpc: Remove obsolete comment about patching instructions
From: Benjamin Herrenschmidt @ 2017-01-04  5:15 UTC (permalink / raw)
  To: linuxppc-dev; +Cc: Steven Munroe, Benjamin Herrenschmidt
In-Reply-To: <20170104051535.9454-1-benh@kernel.crashing.org>

We don't patch instructions based on the cache lines or block
sizes these days.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 arch/powerpc/kernel/setup_64.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
index 6263e0d..222bbb0 100644
--- a/arch/powerpc/kernel/setup_64.c
+++ b/arch/powerpc/kernel/setup_64.c
@@ -77,9 +77,6 @@
 int spinning_secondaries;
 u64 ppc64_pft_size;
 
-/* Pick defaults since we might want to patch instructions
- * before we've read this from the device tree.
- */
 struct ppc64_caches ppc64_caches = {
 	.dline_size = 0x40,
 	.log_dline_size = 6,
-- 
2.9.3

^ permalink raw reply related

* [PATCH 9/9] powerpc: A new cache shape aux vectors
From: Benjamin Herrenschmidt @ 2017-01-04  5:15 UTC (permalink / raw)
  To: linuxppc-dev; +Cc: Steven Munroe, Benjamin Herrenschmidt
In-Reply-To: <20170104051535.9454-1-benh@kernel.crashing.org>

The definition is loosely based on sh and alpha, modified to
accomodate larger associativity and cache size for future-proofing.

We currently set all the values to -1 which indicates that the
information isn't available.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 arch/powerpc/include/asm/cache.h       |  1 +
 arch/powerpc/include/asm/elf.h         | 17 ++++++++++++++++-
 arch/powerpc/include/uapi/asm/auxvec.h | 33 ++++++++++++++++++++++++++++++++-
 arch/powerpc/kernel/setup-common.c     |  5 ++++-
 arch/powerpc/kernel/setup_64.c         |  4 ++++
 5 files changed, 57 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/include/asm/cache.h b/arch/powerpc/include/asm/cache.h
index 3987bd9..1557d26 100644
--- a/arch/powerpc/include/asm/cache.h
+++ b/arch/powerpc/include/asm/cache.h
@@ -35,6 +35,7 @@ struct ppc_cache_info {
 	u32 log_block_size;
 	u32 blocks_per_page;
 	u32 sets;
+	u32 assoc;
 };
 
 struct ppc64_caches {
diff --git a/arch/powerpc/include/asm/elf.h b/arch/powerpc/include/asm/elf.h
index 730c27e..27dece6 100644
--- a/arch/powerpc/include/asm/elf.h
+++ b/arch/powerpc/include/asm/elf.h
@@ -108,13 +108,17 @@ do {								\
  */
 # define elf_read_implies_exec(ex, exec_stk) (is_32bit_task() ? \
 		(exec_stk == EXSTACK_DEFAULT) : 0)
-#else 
+#else
 # define elf_read_implies_exec(ex, exec_stk) (exec_stk == EXSTACK_DEFAULT)
 #endif /* __powerpc64__ */
 
 extern int dcache_bsize;
 extern int icache_bsize;
 extern int ucache_bsize;
+extern long il1cache_shape;
+extern long dl1cache_shape;
+extern long l2cache_shape;
+extern long l3cache_shape;
 
 /* vDSO has arch_setup_additional_pages */
 #define ARCH_HAS_SETUP_ADDITIONAL_PAGES
@@ -136,6 +140,9 @@ extern int arch_setup_additional_pages(struct linux_binprm *bprm,
 
 #endif /* CONFIG_SPU_BASE */
 
+#define get_cache_shape(level) \
+	(ppc64_caches.level.assoc << 16 | ppc64_caches.level.line_size)
+
 /*
  * The requirements here are:
  * - keep the final alignment of sp (sp & 0xf)
@@ -156,6 +163,14 @@ do {									\
 	NEW_AUX_ENT(AT_ICACHEBSIZE, icache_bsize);			\
 	NEW_AUX_ENT(AT_UCACHEBSIZE, ucache_bsize);			\
 	VDSO_AUX_ENT(AT_SYSINFO_EHDR, current->mm->context.vdso_base);	\
+	NEW_AUX_ENT(AT_L1I_CACHESIZE, ppc64_caches.l1i.size);		\
+	NEW_AUX_ENT(AT_L1I_CACHESHAPE, get_cache_shape(l1i));		\
+	NEW_AUX_ENT(AT_L1D_CACHESIZE, ppc64_caches.l1i.size);		\
+	NEW_AUX_ENT(AT_L1D_CACHESHAPE, get_cache_shape(l1i));		\
+	NEW_AUX_ENT(AT_L2_CACHESIZE, ppc64_caches.l2.size);		\
+	NEW_AUX_ENT(AT_L2_CACHESHAPE, get_cache_shape(l2));		\
+	NEW_AUX_ENT(AT_L3_CACHESIZE, ppc64_caches.l3.size);		\
+	NEW_AUX_ENT(AT_L3_CACHESHAPE, get_cache_shape(l3));		\
 } while (0)
 
 #endif /* _ASM_POWERPC_ELF_H */
diff --git a/arch/powerpc/include/uapi/asm/auxvec.h b/arch/powerpc/include/uapi/asm/auxvec.h
index ce17d2c..79183d2 100644
--- a/arch/powerpc/include/uapi/asm/auxvec.h
+++ b/arch/powerpc/include/uapi/asm/auxvec.h
@@ -16,6 +16,37 @@
  */
 #define AT_SYSINFO_EHDR		33
 
-#define AT_VECTOR_SIZE_ARCH 6 /* entries in ARCH_DLINFO */
+/*
+ * AT_*CACHEBSIZE above represent the cache *block* size which is
+ * the size that is affected by the cache management instructions.
+ *
+ * It doesn't nececssarily matches the cache *line* size which is
+ * more of a performance tuning hint. Additionally the latter can
+ * be different for the different cache levels.
+ *
+ * The set of entries below represent more extensive information
+ * about the caches, in the form of two entry per cache type,
+ * one entry containing the cache size in bytes, and the other
+ * containing the cache line size in bytes in the bottom 16 bits
+ * and the cache associativity in the next 16 bits.
+ *
+ * The associativity is such that if N is the 16-bit value, the
+ * cache is N way set associative. A value if 0xffff means fully
+ * associative, a value of 1 means directly mapped.
+ *
+ * For all these fields, a value of 0 means that the information
+ * is not known.
+ */
+
+#define AT_L1I_CACHESIZE	40
+#define AT_L1I_CACHESHAPE	41
+#define AT_L1D_CACHESIZE	42
+#define AT_L1D_CACHESHAPE	43
+#define AT_L2_CACHESIZE		44
+#define AT_L2_CACHESHAPE	45
+#define AT_L3_CACHESIZE		46
+#define AT_L3_CACHESHAPE	47
+
+#define AT_VECTOR_SIZE_ARCH	14 /* entries in ARCH_DLINFO */
 
 #endif
diff --git a/arch/powerpc/kernel/setup-common.c b/arch/powerpc/kernel/setup-common.c
index e0eeed4..cfa2a06 100644
--- a/arch/powerpc/kernel/setup-common.c
+++ b/arch/powerpc/kernel/setup-common.c
@@ -94,7 +94,10 @@ EXPORT_SYMBOL_GPL(boot_cpuid);
 int dcache_bsize;
 int icache_bsize;
 int ucache_bsize;
-
+long il1cache_shape = -1;
+long dl1cache_shape = -1;
+long l2cache_shape = -1;
+long l3cache_shape = -1;
 
 unsigned long klimit = (unsigned long) _end;
 
diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
index b3c93d8..16cb0b7 100644
--- a/arch/powerpc/kernel/setup_64.c
+++ b/arch/powerpc/kernel/setup_64.c
@@ -451,6 +451,10 @@ static bool __init parse_cache_info(struct device_node *np,
 	info->block_size = bsize;
 	info->log_block_size = __ilog2(bsize);
 	info->blocks_per_page = PAGE_SIZE / bsize;
+	if (sets == 0)
+		info->assoc = 0xffff;
+	else
+		info->assoc = size / (sets * lsize);
 
 	return success;
 }
-- 
2.9.3

^ permalink raw reply related


This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.