[PATCH v2] gpio: Add Intel gpio controller support

2024-04-10 Thread Tomas Marek
Signed-off-by: Tomas Marek 
---
Changes in v2:
- Addressed comments from Ahmad - mainly fixing the return value of
  intel_gpio_get_direction, and implementing the add_intel_gpio_device
  helper.
- Link to v1: 
https://lore.barebox.org/barebox/20240409071422.5934-1-tomas.ma...@elrest.cz/
---
 drivers/gpio/Kconfig   |   6 +
 drivers/gpio/Makefile  |   1 +
 drivers/gpio/gpio-intel.c  | 198 +
 include/platform_data/gpio-intel.h |  23 
 4 files changed, 228 insertions(+)
 create mode 100644 drivers/gpio/gpio-intel.c
 create mode 100644 include/platform_data/gpio-intel.h

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 9f27addaa2..d9bb68e06b 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -219,6 +219,12 @@ config GPIO_LATCH
  Say yes here to enable a driver for GPIO multiplexers based on latches
  connected to other GPIOs.
 
+config GPIO_INTEL
+   bool "Intel GPIO driver"
+   depends on X86 || COMPILE_TEST
+   help
+ Say Y here to build support for the Intel GPIO driver.
+
 endmenu
 
 endif
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 628e975285..b0575ccf8c 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -30,3 +30,4 @@ obj-$(CONFIG_GPIO_SIFIVE) += gpio-sifive.o
 obj-$(CONFIG_GPIO_STARFIVE)+= gpio-starfive-vic.o
 obj-$(CONFIG_GPIO_ZYNQ)+= gpio-zynq.o
 obj-$(CONFIG_GPIO_LATCH)   += gpio-latch.o
+obj-$(CONFIG_GPIO_INTEL)   += gpio-intel.o
diff --git a/drivers/gpio/gpio-intel.c b/drivers/gpio/gpio-intel.c
new file mode 100644
index 00..ebec220f46
--- /dev/null
+++ b/drivers/gpio/gpio-intel.c
@@ -0,0 +1,198 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+// SPDX-FileCopyrightText: 2004 Tomas Marek, Elrest Solutions Company s.r.o.
+
+/*
+ * Based on the Linux kernel v6.8 drivers/pinctrl/intel/pinctrl-intel.c.
+ */
+
+#include 
+#include 
+#include 
+#include 
+
+#include 
+
+#define PADBAR 0x00c
+
+/* Offset from pad_regs */
+#define PADCFG00x000
+#define PADCFG0_RXEVCFG_MASK   GENMASK(26, 25)
+#define PADCFG0_RXEVCFG_LEVEL  (0 << 25)
+#define PADCFG0_RXEVCFG_EDGE   (1 << 25)
+#define PADCFG0_RXEVCFG_DISABLED   (2 << 25)
+#define PADCFG0_RXEVCFG_EDGE_BOTH  (3 << 25)
+#define PADCFG0_PREGFRXSEL BIT(24)
+#define PADCFG0_RXINV  BIT(23)
+#define PADCFG0_GPIROUTIOXAPIC BIT(20)
+#define PADCFG0_GPIROUTSCI BIT(19)
+#define PADCFG0_GPIROUTSMI BIT(18)
+#define PADCFG0_GPIROUTNMI BIT(17)
+#define PADCFG0_PMODE_SHIFT10
+#define PADCFG0_PMODE_MASK GENMASK(13, 10)
+#define PADCFG0_PMODE_GPIO 0
+#define PADCFG0_GPIORXDIS  BIT(9)
+#define PADCFG0_GPIOTXDIS  BIT(8)
+#define PADCFG0_GPIORXSTATEBIT(1)
+#define PADCFG0_GPIOTXSTATEBIT(0)
+
+struct intel_gpio_chip {
+   struct gpio_chip chip;
+   void __iomem *community_pad_base;
+};
+
+static inline struct intel_gpio_chip *to_intel_gpio(struct gpio_chip *gc)
+{
+   return container_of(gc, struct intel_gpio_chip, chip);
+}
+
+static void __iomem *intel_gpio_padcfg0_reg(const struct intel_gpio_chip *chip,
+   const unsigned int gpio)
+{
+   const u32 pad_cfg_offset = 16;
+
+   return chip->community_pad_base + pad_cfg_offset * gpio;
+}
+
+static u32 intel_gpio_padcfg0_value(const struct intel_gpio_chip *chip,
+   const unsigned int gpio)
+{
+   return readl(intel_gpio_padcfg0_reg(chip, gpio));
+}
+
+static void intel_gpio_padcfg0_write(const struct intel_gpio_chip *chip,
+const unsigned int gpio, u32 value)
+{
+   writel(value, intel_gpio_padcfg0_reg(chip, gpio));
+}
+
+static void intel_gpio_set_value(struct gpio_chip *gc, unsigned int gpio,
+int value)
+{
+   struct intel_gpio_chip *chip = to_intel_gpio(gc);
+   u32 padcfg0;
+
+   padcfg0 = intel_gpio_padcfg0_value(chip, gpio);
+   if (value)
+   padcfg0 |= PADCFG0_GPIOTXSTATE;
+   else
+   padcfg0 &= ~PADCFG0_GPIOTXSTATE;
+   intel_gpio_padcfg0_write(chip, gpio, padcfg0);
+}
+
+static int intel_gpio_get_value(struct gpio_chip *gc, unsigned int gpio)
+{
+   struct intel_gpio_chip *chip = to_intel_gpio(gc);
+   u32 padcfg0;
+
+   padcfg0 = intel_gpio_padcfg0_value(chip, gpio);
+   if (!(padcfg0 & PADCFG0_GPIOTXDIS))
+   return !!(padcfg0 & PADCFG0_GPIOTXSTATE);
+
+   return !!(padcfg0 & PADCFG0_GPIORXSTATE);
+}
+
+static int intel_gpio_get_direction(struct gpio_chip *gc, unsigned int gpio)
+{
+   struct intel_gpio_chip *chip = to_intel_gpio(gc);
+   u32 padcfg0;
+
+   padcfg0 = intel_gpio_padcfg0_value(chip, gpio);
+
+   if 

[PATCH] scripts: config: add script to manipulate .config files on the command line

2024-04-10 Thread Ahmad Fatoum
This ports over the Linux v6.9-rc3 state of the config script, which
allows easy enabling and disabling of options from the command line, e.g.:

  scripts/config --file build/.config -d CONFIG_WERROR

By having the script in the barebox scripts directory, it's available
for use by build systems instead of running sed over the .config file.

Signed-off-by: Ahmad Fatoum 
---
 scripts/config | 230 +
 1 file changed, 230 insertions(+)
 create mode 100755 scripts/config

diff --git a/scripts/config b/scripts/config
new file mode 100755
index ..ff88e2faefd3
--- /dev/null
+++ b/scripts/config
@@ -0,0 +1,230 @@
+#!/usr/bin/env bash
+# SPDX-License-Identifier: GPL-2.0
+# Manipulate options in a .config file from the command line
+
+myname=${0##*/}
+
+# If no prefix forced, use the default CONFIG_
+CONFIG_="${CONFIG_-CONFIG_}"
+
+# We use an uncommon delimiter for sed substitutions
+SED_DELIM=$(echo -en "\001")
+
+usage() {
+   cat >&2 <"$tmpfile"
+   # replace original file with the edited one
+   mv "$tmpfile" "$infile"
+}
+
+txt_subst() {
+   local before="$1"
+   local after="$2"
+   local infile="$3"
+   local tmpfile="$infile.swp"
+
+   sed -e "s$SED_DELIM$before$SED_DELIM$after$SED_DELIM" "$infile" 
>"$tmpfile"
+   # replace original file with the edited one
+   mv "$tmpfile" "$infile"
+}
+
+txt_delete() {
+   local text="$1"
+   local infile="$2"
+   local tmpfile="$infile.swp"
+
+   sed -e "/$text/d" "$infile" >"$tmpfile"
+   # replace original file with the edited one
+   mv "$tmpfile" "$infile"
+}
+
+set_var() {
+   local name=$1 new=$2 before=$3
+
+   name_re="^($name=|# $name is not set)"
+   before_re="^($before=|# $before is not set)"
+   if test -n "$before" && grep -Eq "$before_re" "$FN"; then
+   txt_append "^$before=" "$new" "$FN"
+   txt_append "^# $before is not set" "$new" "$FN"
+   elif grep -Eq "$name_re" "$FN"; then
+   txt_subst "^$name=.*" "$new" "$FN"
+   txt_subst "^# $name is not set" "$new" "$FN"
+   else
+   echo "$new" >>"$FN"
+   fi
+}
+
+undef_var() {
+   local name=$1
+
+   txt_delete "^$name=" "$FN"
+   txt_delete "^# $name is not set" "$FN"
+}
+
+if [ "$1" = "--file" ]; then
+   FN="$2"
+   if [ "$FN" = "" ] ; then
+   usage
+   fi
+   shift 2
+else
+   FN=.config
+fi
+
+if [ "$1" = "" ] ; then
+   usage
+fi
+
+MUNGE_CASE=yes
+while [ "$1" != "" ] ; do
+   CMD="$1"
+   shift
+   case "$CMD" in
+   --keep-case|-k)
+   MUNGE_CASE=no
+   continue
+   ;;
+   --refresh)
+   ;;
+   --*-after|-E|-D|-M)
+   checkarg "$1"
+   A=$ARG
+   checkarg "$2"
+   B=$ARG
+   shift 2
+   ;;
+   -*)
+   checkarg "$1"
+   shift
+   ;;
+   esac
+   case "$CMD" in
+   --enable|-e)
+   set_var "${CONFIG_}$ARG" "${CONFIG_}$ARG=y"
+   ;;
+
+   --disable|-d)
+   set_var "${CONFIG_}$ARG" "# ${CONFIG_}$ARG is not set"
+   ;;
+
+   --module|-m)
+   set_var "${CONFIG_}$ARG" "${CONFIG_}$ARG=m"
+   ;;
+
+   --set-str)
+   # sed swallows one level of escaping, so we need double-escaping
+   set_var "${CONFIG_}$ARG" "${CONFIG_}$ARG=\"${1//\"/\"}\""
+   shift
+   ;;
+
+   --set-val)
+   set_var "${CONFIG_}$ARG" "${CONFIG_}$ARG=$1"
+   shift
+   ;;
+   --undefine|-u)
+   undef_var "${CONFIG_}$ARG"
+   ;;
+
+   --state|-s)
+   if grep -q "# ${CONFIG_}$ARG is not set" $FN ; then
+   echo n
+   else
+   V="$(grep "^${CONFIG_}$ARG=" $FN)"
+   if [ $? != 0 ] ; then
+   echo undef
+   else
+   V="${V/#${CONFIG_}$ARG=/}"
+   V="${V/#\"/}"
+   V="${V/%\"/}"
+   V="${V//\\\"/\"}"
+   echo "${V}"
+   fi
+   fi
+   ;;
+
+   --enable-after|-E)
+   set_var "${CONFIG_}$B" "${CONFIG_}$B=y" "${CONFIG_}$A"
+   ;;
+
+   --disable-after|-D)
+   set_var "${CONFIG_}$B" "# ${CONFIG_}$B is not set" 
"${CONFIG_}$A"
+   ;;
+
+   --module-after|-M)
+   set_var "${CONFIG_}$B" "${CONFIG_}$B=m" "${CONFIG_}$A"
+   ;;
+
+   # undocumented because it ignores --file (fixme)
+   --refresh)
+   yes "" | make oldconfig
+   ;;
+
+   *)
+   echo "bad command: 

Re: raspi: should vc fixups update properties of existing nodes?

2024-04-10 Thread Ahmad Fatoum
Hello Jonas,

On 10.04.24 11:59, Jonas Richardsen wrote:
> Hi,
> 
> the function `rpi_vc_fdt_parse` in arch/arm/boards/raspberry-pi/rpi-common.c 
> registers multiple fixups that take over nodes from the video core device 
> tree. These fixups make use of the `of_copy_property` function to copy the 
> properties of the respective node. In the case of already existing nodes (and 
> properties), this function duplicates the properties instead of updating them.

Ouch.

> If the intention is to not override existing properties, one should probably 
> check for the existence of each property before copying to avoid kernel 
> warnings and misconfiguration due to duplicate properties.

I think that's the way to go. of_copy_property should maybe return
PTR_ERR(-EEXIST) if the property already exists. Users are free to
use of_delete_property_by_name beforehand if they want to remove
the content.

> If existing properties are supposed to be updated, this could be achieved by 
> switching to `of_set_property` (or something similar). Note that this notion 
> of overriding properties also exists in video core, see 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/broadcom/bcm2711.dtsi?h=v6.8#n412
>  for an example.

I think the default should be not to override and exceptions should
be done explicitly.

> I would be glad to submit a patch for either case.

That would be great!

Thanks,
Ahmad

> 
> Regards,
> 
> Jonas
> 
> 
> 

-- 
Pengutronix e.K.   | |
Steuerwalder Str. 21   | http://www.pengutronix.de/  |
31137 Hildesheim, Germany  | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |




raspi: should vc fixups update properties of existing nodes?

2024-04-10 Thread Jonas Richardsen

Hi,

the function `rpi_vc_fdt_parse` in 
arch/arm/boards/raspberry-pi/rpi-common.c registers multiple fixups that 
take over nodes from the video core device tree. These fixups make use 
of the `of_copy_property` function to copy the properties of the 
respective node. In the case of already existing nodes (and properties), 
this function duplicates the properties instead of updating them.


If the intention is to not override existing properties, one should 
probably check for the existence of each property before copying to 
avoid kernel warnings and misconfiguration due to duplicate properties.


If existing properties are supposed to be updated, this could be 
achieved by switching to `of_set_property` (or something similar). Note 
that this notion of overriding properties also exists in video core, see 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/broadcom/bcm2711.dtsi?h=v6.8#n412 
for an example.


I would be glad to submit a patch for either case.

Regards,

Jonas




[PATCH] jffs2: change pr_fmt prefix to jffs2

2024-04-10 Thread Sascha Hauer
KBUILD_MODNAME expands to the filename which is not a good prefix for
messages. Change it to jffs2 to give the messages a more meaningful
prefix.

Signed-off-by: Sascha Hauer 
---
 fs/jffs2/build.c   | 2 +-
 fs/jffs2/compr.c   | 2 +-
 fs/jffs2/compr_rubin.c | 2 +-
 fs/jffs2/compr_zlib.c  | 2 +-
 fs/jffs2/debug.c   | 2 +-
 fs/jffs2/dir.c | 2 +-
 fs/jffs2/fs.c  | 2 +-
 fs/jffs2/malloc.c  | 2 +-
 fs/jffs2/nodelist.c| 2 +-
 fs/jffs2/read.c| 2 +-
 fs/jffs2/readinode.c   | 2 +-
 fs/jffs2/scan.c| 2 +-
 fs/jffs2/super.c   | 2 +-
 13 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/fs/jffs2/build.c b/fs/jffs2/build.c
index d5757d100b..3871547b99 100644
--- a/fs/jffs2/build.c
+++ b/fs/jffs2/build.c
@@ -8,7 +8,7 @@
  * Created by David Woodhouse 
  */
 
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+#define pr_fmt(fmt) "jffs2: " fmt
 #include 
 #include 
 #include 
diff --git a/fs/jffs2/compr.c b/fs/jffs2/compr.c
index 04b014199f..a056be051c 100644
--- a/fs/jffs2/compr.c
+++ b/fs/jffs2/compr.c
@@ -9,7 +9,7 @@
  *
  * Created by Arjan van de Ven 
  */
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+#define pr_fmt(fmt) "jffs2: " fmt
 #include 
 #include "compr.h"
 
diff --git a/fs/jffs2/compr_rubin.c b/fs/jffs2/compr_rubin.c
index 91a500f4fb..913276d986 100644
--- a/fs/jffs2/compr_rubin.c
+++ b/fs/jffs2/compr_rubin.c
@@ -8,7 +8,7 @@
  * Created by Arjan van de Ven 
  */
 
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+#define pr_fmt(fmt) "jffs2: " fmt
 
 #include 
 #include 
diff --git a/fs/jffs2/compr_zlib.c b/fs/jffs2/compr_zlib.c
index 2b7914f1f5..0580bab0de 100644
--- a/fs/jffs2/compr_zlib.c
+++ b/fs/jffs2/compr_zlib.c
@@ -7,7 +7,7 @@
  *
  * Created by David Woodhouse 
  */
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+#define pr_fmt(fmt) "jffs2: " fmt
 #include 
 #include 
 #include 
diff --git a/fs/jffs2/debug.c b/fs/jffs2/debug.c
index edf8539762..4ac501e2de 100644
--- a/fs/jffs2/debug.c
+++ b/fs/jffs2/debug.c
@@ -8,7 +8,7 @@
  * Created by David Woodhouse 
  */
 
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+#define pr_fmt(fmt) "jffs2: " fmt
 
 #include 
 #include 
diff --git a/fs/jffs2/dir.c b/fs/jffs2/dir.c
index 34f8d141f2..94ef51f778 100644
--- a/fs/jffs2/dir.c
+++ b/fs/jffs2/dir.c
@@ -7,7 +7,7 @@
  *
  * Created by David Woodhouse 
  */
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+#define pr_fmt(fmt) "jffs2: " fmt
 #include 
 #include 
 #include 
diff --git a/fs/jffs2/fs.c b/fs/jffs2/fs.c
index 6f2cbff6c9..fcce56c15f 100644
--- a/fs/jffs2/fs.c
+++ b/fs/jffs2/fs.c
@@ -7,7 +7,7 @@
  *
  * Created by David Woodhouse 
  */
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+#define pr_fmt(fmt) "jffs2: " fmt
 #include 
 #include 
 #include 
diff --git a/fs/jffs2/malloc.c b/fs/jffs2/malloc.c
index 202191be94..e0e29fa648 100644
--- a/fs/jffs2/malloc.c
+++ b/fs/jffs2/malloc.c
@@ -7,7 +7,7 @@
  * Created by David Woodhouse 
  */
 
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+#define pr_fmt(fmt) "jffs2: " fmt
 
 #include 
 #include 
diff --git a/fs/jffs2/nodelist.c b/fs/jffs2/nodelist.c
index 94753e1995..debf10e751 100644
--- a/fs/jffs2/nodelist.c
+++ b/fs/jffs2/nodelist.c
@@ -7,7 +7,7 @@
  * Created by David Woodhouse 
  */
 
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+#define pr_fmt(fmt) "jffs2: " fmt
 
 #include 
 #include 
diff --git a/fs/jffs2/read.c b/fs/jffs2/read.c
index a1c3b9d47b..fffa5f60cb 100644
--- a/fs/jffs2/read.c
+++ b/fs/jffs2/read.c
@@ -7,7 +7,7 @@
  * Created by David Woodhouse 
  */
 
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+#define pr_fmt(fmt) "jffs2: " fmt
 #include 
 #include 
 #include 
diff --git a/fs/jffs2/readinode.c b/fs/jffs2/readinode.c
index aaf2619613..605130d60c 100644
--- a/fs/jffs2/readinode.c
+++ b/fs/jffs2/readinode.c
@@ -7,7 +7,7 @@
  * Created by David Woodhouse 
  */
 
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+#define pr_fmt(fmt) "jffs2: " fmt
 
 #include 
 #include 
diff --git a/fs/jffs2/scan.c b/fs/jffs2/scan.c
index 0d74a8f51f..e1e5120a28 100644
--- a/fs/jffs2/scan.c
+++ b/fs/jffs2/scan.c
@@ -7,7 +7,7 @@
  * Created by David Woodhouse 
  */
 
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+#define pr_fmt(fmt) "jffs2: " fmt
 #include 
 #include 
 #include 
diff --git a/fs/jffs2/super.c b/fs/jffs2/super.c
index 37b3f328c6..d56cdfe132 100644
--- a/fs/jffs2/super.c
+++ b/fs/jffs2/super.c
@@ -7,7 +7,7 @@
  *
  * Created by David Woodhouse 
  */
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+#define pr_fmt(fmt) "jffs2: " fmt
 #include 
 #include 
 #include 
-- 
2.39.2




Re: [PATCH] gpio: Add Intel gpio controller support

2024-04-10 Thread Ahmad Fatoum
Hello Tomas,

On 10.04.24 09:39, Tomas Marek wrote:
> On Tue, Apr 09, 2024 at 09:41:28AM +0200, Ahmad Fatoum wrote:
>>> +static int intel_gpio_get_direction(struct gpio_chip *gc, unsigned int 
>>> gpio)
>>> +   kfree(intel_gpio);
>>> +   return ret;
>>> +   }
>>> +
>>> +   return 0;
>>> +}
>>> +
>>> +static struct driver_d intel_gpio_driver = {
>>> +   .name = "intel-gpio",
>>> +   .probe = intel_gpio_probe,
>>> +};
>>> +
>>> +coredevice_platform_driver(intel_gpio_driver);
>>
>> Who will register this device? Is it possible to add an ACPI table match
>> (like itco_wdt does for example) for your SoC and then register the device

Typo: s/itco_wdt/WDAT/

>> there like Linux does?
>>
>> This would make extension for more SoCs easier in future.
> 
> I have registered the device in the board code now. In theory, it is
> definitely possible to register the device using ACPI match, and I agree
> with you that it's useful.
> 
> Unfortunately, the GPIO community resource definition is inside the DSDT
> ACPI table for my device. I might be wrong here, but I think that Barebox
> parses root tables but doesn’t delve into the nested DSDT at the moment.

We indeed do not do DSDT parsing yet.

> So it's getting a bit complicated here. I can take a closer look at it
> later (without any guarantee of success, of course :-)), but I would
> consider it a different patch if you don't mind.

I was thinking of matching against the SoC type (e.g. Alderlake) and just
register hardcoded values for the addresses/ngpios. I am not sure what's the
easiest way to get this information though (maybe SMBIOS?).

This shouldn't delay applying this driver though.

> Needless to say, I am coming from the ARM world and haven't found my way
> the ACPI just yet :-).

Same here ^^

>>> +#ifndef __GPIO_INTEL_H
>>> +#define __GPIO_INTEL_H
>>> +
>>> +struct gpio_intel_platform_data {
>>> +   unsigned intngpios;
>>> +};
>>
>> I'd suggest you add a add_intel_gpio_device helper here that would create a 
>> suitable
>> device. This could be then called from the ACPI driver probe or from board 
>> code if
>> discoverability is not possible.
> 
> Is following code what you have in mind?
> 
> include/platform_data/gpio-intel.h:
> 
>   struct gpio_intel_platform_data {
>   resource_size_t community_base;
>   resource_size_t community_size;
>   unsigned intngpios;
>   };
> 
>   static inline struct device *add_intel_gpio_device(
>   struct gpio_intel_platform_data *pdata
>   )
>   {
>   return add_generic_device("intel-gpio", DEVICE_ID_DYNAMIC, NULL,
>   pdata->community_base, pdata->community_size,
>   IORESOURCE_MEM, pdata);
>   }

This looks ok.

Cheers,
Ahmad

> 
> 
> Best regards
> 
> Tomas
> 
>>
>>
>> Cheers,
>> Ahmad
>>
>>> +
>>> +#endif /* __GPIO_INTEL_H */
>>
>> -- 
>> Pengutronix e.K.   | |
>> Steuerwalder Str. 21   | http://www.pengutronix.de/  |
>> 31137 Hildesheim, Germany  | Phone: +49-5121-206917-0|
>> Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |
>>
> 

-- 
Pengutronix e.K.   | |
Steuerwalder Str. 21   | http://www.pengutronix.de/  |
31137 Hildesheim, Germany  | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |




Re: [PATCH] gpio: Add Intel gpio controller support

2024-04-10 Thread Tomas Marek
Hello Ahmad,

many thanks for the review.

On Tue, Apr 09, 2024 at 09:41:28AM +0200, Ahmad Fatoum wrote:
> Hello Tomas,
> 
> On 09.04.24 09:14, Tomas Marek wrote:
> > Signed-off-by: Tomas Marek 
> 
> Thanks for your patch. I have a soft spot for barebox-as-efi-payload,
> so it's cool to see you contributing new features.
> 
> It also makes me curious what more drivers are you intending to
> contribute. :-)

Nice to hear someone is interested in :-).

> 
> Some review below. 
> 
> > ---
> >  drivers/gpio/Kconfig   |   5 +
> >  drivers/gpio/Makefile  |   1 +
> >  drivers/gpio/gpio-intel.c  | 198 +
> >  include/platform_data/gpio-intel.h |  10 ++
> >  4 files changed, 214 insertions(+)
> >  create mode 100644 drivers/gpio/gpio-intel.c
> >  create mode 100644 include/platform_data/gpio-intel.h
> > 
> > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> > index 9f27addaa2..094c9b7fd4 100644
> > --- a/drivers/gpio/Kconfig
> > +++ b/drivers/gpio/Kconfig
> > @@ -219,6 +219,11 @@ config GPIO_LATCH
> >   Say yes here to enable a driver for GPIO multiplexers based on latches
> >   connected to other GPIOs.
> >  
> > +config GPIO_INTEL
> > +   tristate "Intel GPIO driver"
> 
> Please add a depends on X86 || COMPILE_TEST here, so other architectures
> aren't prompted for this driver by default.

Make sense, I’ll do so.

> 
> > +   help
> > + Say Y or M here to build support for the Intel GPIO driver.
> 
> Nitpick: We only have [M]odule support for ARM, so tristate == bool in your
> case and one couldn't set M here, despite what the help text suggests.

I understand. I will change 'tristate' to 'bool' and update the help
to avoid any confusion.

> 
> > +static int intel_gpio_get_direction(struct gpio_chip *gc, unsigned int 
> > gpio)
> > +{
> > +   struct intel_gpio_chip *chip = to_intel_gpio(gc);
> > +   u32 padcfg0;
> > +
> > +   padcfg0 = intel_gpio_padcfg0_value(chip, gpio);
> > +
> > +   if (padcfg0 & PADCFG0_PMODE_MASK)
> > +   return -EINVAL;
> > +
> > +   if (padcfg0 & PADCFG0_GPIOTXDIS)
> > +   return GPIOF_DIR_IN;
> > +
> > +   return GPIOF_DIR_IN;
> 
> Your never return GPIOF_DIR_OUT. Is this intended?

Silly me. No, it was not intentional; it's a mistake. The last statement
should return GPIOF_DIR_OUT. Thank you for pointing that out. I'll fix it.

> 
> > +   ret = gpiochip_add(_gpio->chip);
> > +
> > +   if (ret) {
> > +   dev_err(dev, "Couldn't add gpiochip: %d\n", ret);
> 
> Nitpick: %pe\n", ERR_PTR(ret)

Thanks for hit.

> 
> > +   kfree(intel_gpio);
> > +   return ret;
> > +   }
> > +
> > +   return 0;
> > +}
> > +
> > +static struct driver_d intel_gpio_driver = {
> > +   .name = "intel-gpio",
> > +   .probe = intel_gpio_probe,
> > +};
> > +
> > +coredevice_platform_driver(intel_gpio_driver);
> 
> Who will register this device? Is it possible to add an ACPI table match
> (like itco_wdt does for example) for your SoC and then register the device
> there like Linux does?
> 
> This would make extension for more SoCs easier in future.

I have registered the device in the board code now. In theory, it is
definitely possible to register the device using ACPI match, and I agree
with you that it's useful.

Unfortunately, the GPIO community resource definition is inside the DSDT
ACPI table for my device. I might be wrong here, but I think that Barebox
parses root tables but doesn’t delve into the nested DSDT at the moment.
So it's getting a bit complicated here. I can take a closer look at it
later (without any guarantee of success, of course :-)), but I would
consider it a different patch if you don't mind.

Needless to say, I am coming from the ARM world and haven't found my way
the ACPI just yet :-).

> 
> > diff --git a/include/platform_data/gpio-intel.h 
> > b/include/platform_data/gpio-intel.h
> > new file mode 100644
> > index 00..f04baadd4d
> > --- /dev/null
> > +++ b/include/platform_data/gpio-intel.h
> > @@ -0,0 +1,10 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +
> > +#ifndef __GPIO_INTEL_H
> > +#define __GPIO_INTEL_H
> > +
> > +struct gpio_intel_platform_data {
> > +   unsigned intngpios;
> > +};
> 
> I'd suggest you add a add_intel_gpio_device helper here that would create a 
> suitable
> device. This could be then called from the ACPI driver probe or from board 
> code if
> discoverability is not possible.

Is following code what you have in mind?

include/platform_data/gpio-intel.h:

struct gpio_intel_platform_data {
resource_size_t community_base;
resource_size_t community_size;
unsigned intngpios;
};

static inline struct device *add_intel_gpio_device(
struct gpio_intel_platform_data *pdata
)
{
return add_generic_device("intel-gpio", DEVICE_ID_DYNAMIC, NULL,
pdata->community_base, pdata->community_size,
  

Re: [PATCH v2 1/2] bootm: replace CONFIG_BOOTM_FORCE_SIGNED_IMAGES with helper

2024-04-10 Thread Sascha Hauer


On Mon, 08 Apr 2024 16:31:30 +0200, Marco Felsch wrote:
> In preparation for allowing even CONFIG_BOOTM_FORCE_SIGNED_IMAGES=n
> configurations to force boot of only signed images, replace direct
> use of IS_ENABLED(CONFIG_BOOTM_FORCE_SIGNED_IMAGES) with a helper that
> queries a static variable that can be forced at runtime in a follow-up
> commit.
> 
> No functional change.
> 
> [...]

Applied, thanks!

[1/2] bootm: replace CONFIG_BOOTM_FORCE_SIGNED_IMAGES with helper
  https://git.pengutronix.de/cgit/barebox/commit/?id=7c80ebdcecd9 (link may 
not be stable)
[2/2] bootm: add support for dynamically forcing signature verification
  https://git.pengutronix.de/cgit/barebox/commit/?id=933db056bbdf (link may 
not be stable)

Best regards,
-- 
Sascha Hauer 




Re: [PATCH] mci: sdhci: fix dma mapping

2024-04-10 Thread Sascha Hauer


On Mon, 08 Apr 2024 18:31:01 +0200, Marco Felsch wrote:
> In case of MMC_DATA_READ the dest address should be used and in case of
> MMC_DATA_WRITE the src address should be used. We had no issues for now
> since both dest and src point to same address due to the union.
> 
> 

Applied, thanks!

[1/1] mci: sdhci: fix dma mapping
  https://git.pengutronix.de/cgit/barebox/commit/?id=743130a7c75f (link may 
not be stable)

Best regards,
-- 
Sascha Hauer