Re: [PATCH v2 01/23] clk: rockchip: rk356x: Add CLK_USB3OTGx_REF support

[Sean Anderson]

On 4/13/24 14:13, Jonas Karlman wrote:

The CLK_USB3OTGx_REF clocks is used as reference clock for USB3 block.

Add simple support to get rate of CLK_USB3OTGx_REF clocks to fix
reference clock period configuration.

Signed-off-by: Jonas Karlman 
---
v2: No change
---
  drivers/clk/rockchip/clk_rk3568.c | 4 
  1 file changed, 4 insertions(+)

diff --git a/drivers/clk/rockchip/clk_rk3568.c 
b/drivers/clk/rockchip/clk_rk3568.c
index 57ef27dda893..999f48ea4b4e 100644
--- a/drivers/clk/rockchip/clk_rk3568.c
+++ b/drivers/clk/rockchip/clk_rk3568.c
@@ -2417,6 +2417,8 @@ static ulong rk3568_clk_get_rate(struct clk *clk)
case BCLK_EMMC:
rate = rk3568_emmc_get_bclk(priv);
break;
+   case CLK_USB3OTG0_REF:
+   case CLK_USB3OTG1_REF:
case TCLK_EMMC:
rate = OSC_HZ;
break;
@@ -2596,6 +2598,8 @@ static ulong rk3568_clk_set_rate(struct clk *clk, ulong 
rate)
case BCLK_EMMC:
ret = rk3568_emmc_set_bclk(priv, rate);
break;
+   case CLK_USB3OTG0_REF:
+   case CLK_USB3OTG1_REF:
case TCLK_EMMC:
ret = OSC_HZ;
break;


Acked-by: Sean Anderson 

Re: [PATCH v2 02/23] clk: rockchip: rk3588: Add REF_CLK_USB3OTGx support

[Sean Anderson]

On 4/13/24 14:13, Jonas Karlman wrote:

The REF_CLK_USB3OTGx clocks is used as reference clock for USB3 block.

Add simple support to get rate of REF_CLK_USB3OTGx clocks to fix
reference clock period configuration.

Signed-off-by: Jonas Karlman 
Reviewed-by: Quentin Schulz 
---
v2: Collect r-b tag
---
  drivers/clk/rockchip/clk_rk3588.c | 6 ++
  1 file changed, 6 insertions(+)

diff --git a/drivers/clk/rockchip/clk_rk3588.c 
b/drivers/clk/rockchip/clk_rk3588.c
index 8f33843179b0..4c611a390499 100644
--- a/drivers/clk/rockchip/clk_rk3588.c
+++ b/drivers/clk/rockchip/clk_rk3588.c
@@ -1569,6 +1569,9 @@ static ulong rk3588_clk_get_rate(struct clk *clk)
case DCLK_DECOM:
rate = rk3588_mmc_get_clk(priv, clk->id);
break;
+   case REF_CLK_USB3OTG0:
+   case REF_CLK_USB3OTG1:
+   case REF_CLK_USB3OTG2:
case TMCLK_EMMC:
case TCLK_WDT0:
rate = OSC_HZ;
@@ -1734,6 +1737,9 @@ static ulong rk3588_clk_set_rate(struct clk *clk, ulong 
rate)
case DCLK_DECOM:
ret = rk3588_mmc_set_clk(priv, clk->id, rate);
break;
+   case REF_CLK_USB3OTG0:
+   case REF_CLK_USB3OTG1:
+   case REF_CLK_USB3OTG2:
case TMCLK_EMMC:
case TCLK_WDT0:
ret = OSC_HZ;


Acked-by: Sean Anderson 

[PATCH 3/3] patman: Add a tag for when a patch gets added to a series

[Sean Anderson]

When a patch is added to a series after the initial version, there are no
changes to note except that it is new. This is typically done to suppress
the "(no changes in vN)" message. It's also nice to add a change to the
cover letter so reviewers know there is an additional patch. Add a tag to
automate this process a bit.

There are two nits with the current approach:

- It favors '-' as a bullet point, but some people may prefer '*' (or
  something else)
- Tags (e.g. 'patman: ' in 'patman: foo bar') are not stripped. They are
  probably just noise in most series, but they may be useful for treewide
  series to distinguish 'gpio: frobnicate' from 'reset: frobnicate', so
  I've left them in.

Suggestions for the above appreciated.

Suggested-by: Douglas Anderson 
Signed-off-by: Sean Anderson 
---

 tools/patman/func_test.py   |  2 ++
 tools/patman/patchstream.py |  5 +
 tools/patman/patman.rst | 13 +
 ...t-cast-for-sandbox-in-fdtdec_setup_mem_siz.patch |  1 +
 tools/patman/test/test01.txt|  1 +
 5 files changed, 22 insertions(+)

diff --git a/tools/patman/func_test.py b/tools/patman/func_test.py
index 3b4c9448882..af6c025a441 100644
--- a/tools/patman/func_test.py
+++ b/tools/patman/func_test.py
@@ -293,6 +293,7 @@ Changes in v4:
   change
 - Some changes
 - Some notes for the cover letter
+- fdt: Correct cast for sandbox in fdtdec_setup_mem_size_base()
 
 Simon Glass (2):
   pci: Correct cast for sandbox
@@ -342,6 +343,7 @@ Changes in v4:
 - Multi
   line
   change
+- New
 - Some changes
 
 Changes in v2:
diff --git a/tools/patman/patchstream.py b/tools/patman/patchstream.py
index ec1ca874fb2..a09ae9c7371 100644
--- a/tools/patman/patchstream.py
+++ b/tools/patman/patchstream.py
@@ -477,6 +477,11 @@ class PatchStream:
 self.change_version = self._parse_version(value, line)
 elif name == 'cc':
 self.commit.add_cc(value.split(','))
+elif name == 'added-in':
+version = self._parse_version(value, line)
+self.commit.add_change(version, '- New')
+self.series.AddChange(version, None, '- %s' %
+  self.commit.subject)
 else:
 self._add_warn('Line %d: Ignoring Commit-%s' %
(self.linenum, name))
diff --git a/tools/patman/patman.rst b/tools/patman/patman.rst
index 9971fa8c0fd..63b95a6b161 100644
--- a/tools/patman/patman.rst
+++ b/tools/patman/patman.rst
@@ -350,6 +350,19 @@ Cover-changes: n
 - This line will only appear in the cover letter
 
 
+Commit-added-in: n
+Add a change noting the version this commit was added in. This is
+equivalent to::
+
+Commit-changes: n
+- New
+
+Cover-changes: n
+- 
+
+It is a convenient shorthand for suppressing the '(no changes in vN)'
+message.
+
 Patch-cc / Commit-cc: Their Name 
 This copies a single patch to another email address. Note that the
 Cc: used by git send-email is ignored by patman, but will be
diff --git 
a/tools/patman/test/0002-fdt-Correct-cast-for-sandbox-in-fdtdec_setup_mem_siz.patch
 
b/tools/patman/test/0002-fdt-Correct-cast-for-sandbox-in-fdtdec_setup_mem_siz.patch
index 55a0d6756aa..48ea1793b47 100644
--- 
a/tools/patman/test/0002-fdt-Correct-cast-for-sandbox-in-fdtdec_setup_mem_siz.patch
+++ 
b/tools/patman/test/0002-fdt-Correct-cast-for-sandbox-in-fdtdec_setup_mem_siz.patch
@@ -23,6 +23,7 @@ Series-version: 3
 Patch-cc: fred
 Commit-cc: joe
 Series-process-log: sort, uniq
+Commit-added-in: 4
 Series-changes: 4
 - Some changes
 - Multi
diff --git a/tools/patman/test/test01.txt b/tools/patman/test/test01.txt
index 271d9bf043f..b2d73c5972c 100644
--- a/tools/patman/test/test01.txt
+++ b/tools/patman/test/test01.txt
@@ -51,6 +51,7 @@ Date:   Sat Apr 15 15:39:08 2017 -0600
 Patch-cc: fred
 Commit-cc: joe
 Series-process-log: sort, uniq
+Commit-added-in: 4
 Series-changes: 4
 - Some changes
 - Multi
-- 
2.37.1

[PATCH 2/3] patman: Add Commit-cc as an alias for Patch-cc

[Sean Anderson]

Most tags referring to commits (or patches) are named Commit-something. The
exception is Patch-cc. Add a Commit-cc alias so we can use whichever one is
convenient.

Signed-off-by: Sean Anderson 
---

 tools/patman/func_test.py| 5 -
 tools/patman/patchstream.py  | 2 ++
 tools/patman/patman.rst  | 2 +-
 ...dt-Correct-cast-for-sandbox-in-fdtdec_setup_mem_siz.patch | 1 +
 tools/patman/test/test01.txt | 1 +
 5 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/tools/patman/func_test.py b/tools/patman/func_test.py
index 9c016fb5e9a..3b4c9448882 100644
--- a/tools/patman/func_test.py
+++ b/tools/patman/func_test.py
@@ -211,6 +211,7 @@ class TestFunctional(unittest.TestCase):
 'u-boot': ['u-boot@lists.denx.de'],
 'simon': [self.leb],
 'fred': [self.fred],
+'joe': [self.joe],
 }
 
 text = self._get_text('test01.txt')
@@ -259,6 +260,7 @@ class TestFunctional(unittest.TestCase):
 self.assertEqual('Postfix:\t  some-branch', next(lines))
 self.assertEqual('Cover: 4 lines', next(lines))
 self.assertEqual('  Cc:  %s' % self.fred, next(lines))
+self.assertEqual('  Cc:  %s' % self.joe, next(lines))
 self.assertEqual('  Cc:  %s' % self.leb,
  next(lines))
 self.assertEqual('  Cc:  %s' % mel, next(lines))
@@ -272,7 +274,8 @@ class TestFunctional(unittest.TestCase):
 
 self.assertEqual(('%s %s\0%s' % (args[0], rick, stefan)), cc_lines[0])
 self.assertEqual(
-'%s %s\0%s\0%s\0%s' % (args[1], self.fred, self.leb, rick, stefan),
+'%s %s\0%s\0%s\0%s\0%s' % (args[1], self.fred, self.joe, self.leb,
+   rick, stefan),
 cc_lines[1])
 
 expected = '''
diff --git a/tools/patman/patchstream.py b/tools/patman/patchstream.py
index e2e2a83e677..ec1ca874fb2 100644
--- a/tools/patman/patchstream.py
+++ b/tools/patman/patchstream.py
@@ -475,6 +475,8 @@ class PatchStream:
 elif name == 'changes':
 self.in_change = 'Commit'
 self.change_version = self._parse_version(value, line)
+elif name == 'cc':
+self.commit.add_cc(value.split(','))
 else:
 self._add_warn('Line %d: Ignoring Commit-%s' %
(self.linenum, name))
diff --git a/tools/patman/patman.rst b/tools/patman/patman.rst
index f4588c00fc1..9971fa8c0fd 100644
--- a/tools/patman/patman.rst
+++ b/tools/patman/patman.rst
@@ -350,7 +350,7 @@ Cover-changes: n
 - This line will only appear in the cover letter
 
 
-Patch-cc: Their Name 
+Patch-cc / Commit-cc: Their Name 
 This copies a single patch to another email address. Note that the
 Cc: used by git send-email is ignored by patman, but will be
 interpreted by git send-email if you use it.
diff --git 
a/tools/patman/test/0002-fdt-Correct-cast-for-sandbox-in-fdtdec_setup_mem_siz.patch
 
b/tools/patman/test/0002-fdt-Correct-cast-for-sandbox-in-fdtdec_setup_mem_siz.patch
index 56278a6ce9b..55a0d6756aa 100644
--- 
a/tools/patman/test/0002-fdt-Correct-cast-for-sandbox-in-fdtdec_setup_mem_siz.patch
+++ 
b/tools/patman/test/0002-fdt-Correct-cast-for-sandbox-in-fdtdec_setup_mem_siz.patch
@@ -21,6 +21,7 @@ Series-cc: Stefan Brüns 
 Cover-letter-cc: Lord Mëlchett 
 Series-version: 3
 Patch-cc: fred
+Commit-cc: joe
 Series-process-log: sort, uniq
 Series-changes: 4
 - Some changes
diff --git a/tools/patman/test/test01.txt b/tools/patman/test/test01.txt
index fc3066e50b4..271d9bf043f 100644
--- a/tools/patman/test/test01.txt
+++ b/tools/patman/test/test01.txt
@@ -49,6 +49,7 @@ Date:   Sat Apr 15 15:39:08 2017 -0600
 Cover-letter-cc: Lord Mëlchett 
 Series-version: 3
 Patch-cc: fred
+Commit-cc: joe
 Series-process-log: sort, uniq
 Series-changes: 4
 - Some changes
-- 
2.37.1

[PATCH 1/3] patman: Fix tests if add_maintainers is set to False

[Sean Anderson]

If add_maintainers is set to False in the user's ~/.patman config, it will
cause the custom_get_maintainer_script to fail since that test expects
maintainers to be added. Set add_maintainer to True in the .patman config
to prevent this.

Fixes: 8c042fb7f9f ("patman: add '--get-maintainer-script' argument")
Signed-off-by: Sean Anderson 
---

 tools/patman/func_test.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/patman/func_test.py b/tools/patman/func_test.py
index e3918497cf4..9c016fb5e9a 100644
--- a/tools/patman/func_test.py
+++ b/tools/patman/func_test.py
@@ -540,7 +540,8 @@ complicated as possible''')
 with open('.patman', 'w', buffering=1) as f:
 f.write('[settings]\n'
 'get_maintainer_script: dummy-script.sh\n'
-'check_patch: False\n')
+'check_patch: False\n'
+'add_maintainers: True\n')
 with open('dummy-script.sh', 'w', buffering=1) as f:
 f.write('#!/usr/bin/env python\n'
 'print("he...@there.com")\n')
-- 
2.37.1

[PATCH 0/3] patman: A fix and some new tags

[Sean Anderson]

This series has a fix along with a couple of convenient tags.


Sean Anderson (3):
  patman: Fix tests if add_maintainers is set to False
  patman: Add Commit-cc as an alias for Patch-cc
  patman: Add a tag for when a patch gets added to a series

 tools/patman/func_test.py | 10 --
 tools/patman/patchstream.py   |  7 +++
 tools/patman/patman.rst   | 15 ++-
 ...cast-for-sandbox-in-fdtdec_setup_mem_siz.patch |  2 ++
 tools/patman/test/test01.txt  |  2 ++
 5 files changed, 33 insertions(+), 3 deletions(-)

-- 
2.37.1

Re: [PATCH] mmc: sdhci: programmable clock calculation needs multiplier +1

[Sean Anderson]

On 4/12/24 15:26, curtis.mach...@intel.com wrote:
> [You don't often get email from curtis.mach...@intel.com. Learn why this is 
> important at 
> https://cas5-0-urlprotect.trendmicro.com:443/wis/clicktime/v1/query?url=https%3a%2f%2faka.ms%2fLearnAboutSenderIdentification=4f25273f-b33b-4eb3-8643-c1f6eff8842b=d807158c60b7d2502abde8a2fc01f40662980862-c95c2e418b830130ad99c91e42b2732e3f7e44ee
>  ]
> 
> From: cmachida 
> 
> According to the SD Host Controller Simplified Specification v4.20,
> the multiplier value M is one more than the Clock Multiplier field.
> 
> Copied code from Linux project.  drivers/mmc/host/sdhci.c line 4405
> 
> Signed-off-by: cmachida 
> ---
> 
>  drivers/mmc/sdhci.c | 9 +
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c
> index 0178ed8a11..a8476ec4e9 100644
> --- a/drivers/mmc/sdhci.c
> +++ b/drivers/mmc/sdhci.c
> @@ -929,6 +929,15 @@ int sdhci_setup_cfg(struct mmc_config *cfg, struct 
> sdhci_host *host,
> debug("%s, caps_1: 0x%x\n", __func__, caps_1);
> host->clk_mul = (caps_1 & SDHCI_CLOCK_MUL_MASK) >>
> SDHCI_CLOCK_MUL_SHIFT;
> +
> +   /*
> +* In case the value in Clock Multiplier is 0, then 
> programmable
> +* clock mode is not supported, otherwise the actual clock
> +* multiplier is one more than the value of Clock Multiplier
> +* in the Capabilities Register.
> +*/
> +   if (host->clk_mul)
> +   host->clk_mul += 1;
> }
> 
> if (host->max_clk == 0) {
> --
> 2.43.2
> 

Reviewed-by: Sean Anderson 

Re: [PATCH 1/1] clk: sifive: append missing \n to messages

[Sean Anderson]

On 4/11/24 04:37, Heinrich Schuchardt wrote:

On 11.04.24 05:13, Sean Anderson wrote:

On 2/16/24 11:35, Heinrich Schuchardt wrote:

If multiple messages are written, line-feeds improve the readability.

Fixes: c40b6df87fc0 ("clk: Add SiFive FU540 PRCI clock driver")
Signed-off-by: Heinrich Schuchardt 
---
  drivers/clk/analogbits/wrpll-cln28hpc.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/analogbits/wrpll-cln28hpc.c 
b/drivers/clk/analogbits/wrpll-cln28hpc.c
index a3cb109d357..537c696b727 100644
--- a/drivers/clk/analogbits/wrpll-cln28hpc.c
+++ b/drivers/clk/analogbits/wrpll-cln28hpc.c
@@ -81,7 +81,7 @@ static int __wrpll_calc_filter_range(unsigned long 
post_divr_freq)
  {
  if (post_divr_freq < MIN_POST_DIVR_FREQ ||
  post_divr_freq > MAX_POST_DIVR_FREQ) {
-    WARN(1, "%s: post-divider reference freq out of range: %lu",
+    WARN(1, "%s: post-divider reference freq out of range: %lu\n",
   __func__, post_divr_freq);
  return -ERANGE;
  }
@@ -229,7 +229,7 @@ int wrpll_configure_for_rate(struct wrpll_cfg *c, u32 
target_rate,
  int range;
  if (c->flags == 0) {
-    WARN(1, "%s called with uninitialized PLL config", __func__);
+    WARN(1, "%s called with uninitialized PLL config\n", __func__);
  return -EINVAL;
  }
@@ -335,7 +335,7 @@ unsigned long wrpll_calc_output_rate(const struct wrpll_cfg 
*c,
  u64 n;
  if (c->flags & WRPLL_FLAGS_EXT_FEEDBACK_MASK) {
-    WARN(1, "external feedback mode not yet supported");
+    WARN(1, "external feedback mode not yet supported\n");
  return ULONG_MAX;
  }


Reviewed-by: Sean Anderson 

But maybe these should be dev_dbg?


The messages look like indicating misconfiguration. So the user should see 
these.


Yeah, but we already return an error. So the user will notice.


The kernel code also uses WARN() (and also lacks the line-feeds).


In the kernel there are no size restrictions, so it is fine to include messages.

--Sean

Re: [PATCH] clk: imx8mp: add pwm clocks support

[Sean Anderson]

On 4/10/24 23:18, Sean Anderson wrote:

On 3/10/23 10:15, Tommaso Merciai wrote:

Add clocks support for the PWM controllers. This is ported from
Linux v6.3.0-rc1

Signed-off-by: Tommaso Merciai 
---
  drivers/clk/imx/clk-imx8mp.c | 24 
  1 file changed, 24 insertions(+)

diff --git a/drivers/clk/imx/clk-imx8mp.c b/drivers/clk/imx/clk-imx8mp.c
index ffbc1d1ba9..fac87ff505 100644
--- a/drivers/clk/imx/clk-imx8mp.c
+++ b/drivers/clk/imx/clk-imx8mp.c
@@ -122,6 +122,22 @@ static const char *imx8mp_gic_sels[] = {"clock-osc-24m", 
"sys_pll2_200m", "sys_p
  "sys_pll2_100m", "sys_pll1_800m",
  "sys_pll2_500m", "clk_ext4", "audio_pll2_out" };
+static const char * const imx8mp_pwm1_sels[] = {"clock-osc-24m", "sys_pll2_100m", 
"sys_pll1_160m",
+    "sys_pll1_40m", "sys_pll3_out", "clk_ext1",
+    "sys_pll1_80m", "video_pll1_out", };
+
+static const char * const imx8mp_pwm2_sels[] = {"clock-osc-24m", "sys_pll2_100m", 
"sys_pll1_160m",
+    "sys_pll1_40m", "sys_pll3_out", "clk_ext1",
+    "sys_pll1_80m", "video_pll1_out", };
+
+static const char * const imx8mp_pwm3_sels[] = {"clock-osc-24m", "sys_pll2_100m", 
"sys_pll1_160m",
+    "sys_pll1_40m", "sys_pll3_out", "clk_ext2",
+    "sys_pll1_80m", "video_pll1_out", };
+
+static const char * const imx8mp_pwm4_sels[] = {"clock-osc-24m", "sys_pll2_100m", 
"sys_pll1_160m",
+    "sys_pll1_40m", "sys_pll3_out", "clk_ext2",
+    "sys_pll1_80m", "video_pll1_out", };
+
  static const char *imx8mp_ecspi1_sels[] = {"clock-osc-24m", "sys_pll2_200m", 
"sys_pll1_40m",
    "sys_pll1_160m", "sys_pll1_800m", "sys_pll3_out",
    "sys_pll2_250m", "audio_pll2_out", };
@@ -270,6 +286,10 @@ static int imx8mp_clk_probe(struct udevice *dev)
  clk_dm(IMX8MP_CLK_GIC, imx8m_clk_composite_critical("gic", 
imx8mp_gic_sels, base + 0xb200));
  clk_dm(IMX8MP_CLK_ECSPI1, imx8m_clk_composite("ecspi1", 
imx8mp_ecspi1_sels, base + 0xb280));
  clk_dm(IMX8MP_CLK_ECSPI2, imx8m_clk_composite("ecspi2", 
imx8mp_ecspi2_sels, base + 0xb300));
+    clk_dm(IMX8MP_CLK_PWM1, imx8m_clk_composite_critical("pwm1", 
imx8mp_pwm1_sels, base + 0xb380));
+    clk_dm(IMX8MP_CLK_PWM2, imx8m_clk_composite_critical("pwm2", 
imx8mp_pwm2_sels, base + 0xb400));
+    clk_dm(IMX8MP_CLK_PWM3, imx8m_clk_composite_critical("pwm3", 
imx8mp_pwm3_sels, base + 0xb480));
+    clk_dm(IMX8MP_CLK_PWM4, imx8m_clk_composite_critical("pwm4", 
imx8mp_pwm4_sels, base + 0xb500));
  clk_dm(IMX8MP_CLK_ECSPI3, imx8m_clk_composite("ecspi3", 
imx8mp_ecspi3_sels, base + 0xc180));
  clk_dm(IMX8MP_CLK_WDOG, imx8m_clk_composite("wdog", imx8mp_wdog_sels, 
base + 0xb900));
@@ -292,6 +312,10 @@ static int imx8mp_clk_probe(struct udevice *dev)
  clk_dm(IMX8MP_CLK_I2C2_ROOT, imx_clk_gate4("i2c2_root_clk", "i2c2", base 
+ 0x4180, 0));
  clk_dm(IMX8MP_CLK_I2C3_ROOT, imx_clk_gate4("i2c3_root_clk", "i2c3", base 
+ 0x4190, 0));
  clk_dm(IMX8MP_CLK_I2C4_ROOT, imx_clk_gate4("i2c4_root_clk", "i2c4", base 
+ 0x41a0, 0));
+    clk_dm(IMX8MP_CLK_PWM1_ROOT, imx_clk_gate4("pwm1_root_clk", "pwm1", base + 
0x4280, 0));
+    clk_dm(IMX8MP_CLK_PWM2_ROOT, imx_clk_gate4("pwm2_root_clk", "pwm2", base + 
0x4290, 0));
+    clk_dm(IMX8MP_CLK_PWM3_ROOT, imx_clk_gate4("pwm3_root_clk", "pwm3", base + 
0x42a0, 0));
+    clk_dm(IMX8MP_CLK_PWM4_ROOT, imx_clk_gate4("pwm4_root_clk", "pwm4", base + 
0x42b0, 0));
  clk_dm(IMX8MP_CLK_QSPI_ROOT, imx_clk_gate4("qspi_root_clk", "qspi", base 
+ 0x42f0, 0));
  clk_dm(IMX8MP_CLK_I2C5_ROOT, imx_clk_gate2("i2c5_root_clk", "i2c5", base 
+ 0x4330, 0));
  clk_dm(IMX8MP_CLK_I2C6_ROOT, imx_clk_gate2("i2c6_root_clk", "i2c6", base 
+ 0x4340, 0));


Acked-by: Sean Anderson 

But I would like to see a RB from one of the i.MX maintainers (CC'd).


Whoops, didn't notice the date. Seems like this has been added in

https://lore.kernel.org/u-boot/20230330152333.53b1185...@phobos.denx.de/

Re: [PATCH] clk: imx8mp: add pwm clocks support

[Sean Anderson]

On 3/10/23 10:15, Tommaso Merciai wrote:

Add clocks support for the PWM controllers. This is ported from
Linux v6.3.0-rc1

Signed-off-by: Tommaso Merciai 
---
  drivers/clk/imx/clk-imx8mp.c | 24 
  1 file changed, 24 insertions(+)

diff --git a/drivers/clk/imx/clk-imx8mp.c b/drivers/clk/imx/clk-imx8mp.c
index ffbc1d1ba9..fac87ff505 100644
--- a/drivers/clk/imx/clk-imx8mp.c
+++ b/drivers/clk/imx/clk-imx8mp.c
@@ -122,6 +122,22 @@ static const char *imx8mp_gic_sels[] = {"clock-osc-24m", 
"sys_pll2_200m", "sys_p
"sys_pll2_100m", "sys_pll1_800m",
"sys_pll2_500m", "clk_ext4", 
"audio_pll2_out" };
  
+static const char * const imx8mp_pwm1_sels[] = {"clock-osc-24m", "sys_pll2_100m", "sys_pll1_160m",

+   "sys_pll1_40m", "sys_pll3_out", 
"clk_ext1",
+   "sys_pll1_80m", "video_pll1_out", };
+
+static const char * const imx8mp_pwm2_sels[] = {"clock-osc-24m", "sys_pll2_100m", 
"sys_pll1_160m",
+   "sys_pll1_40m", "sys_pll3_out", 
"clk_ext1",
+   "sys_pll1_80m", "video_pll1_out", };
+
+static const char * const imx8mp_pwm3_sels[] = {"clock-osc-24m", "sys_pll2_100m", 
"sys_pll1_160m",
+   "sys_pll1_40m", "sys_pll3_out", 
"clk_ext2",
+   "sys_pll1_80m", "video_pll1_out", };
+
+static const char * const imx8mp_pwm4_sels[] = {"clock-osc-24m", "sys_pll2_100m", 
"sys_pll1_160m",
+   "sys_pll1_40m", "sys_pll3_out", 
"clk_ext2",
+   "sys_pll1_80m", "video_pll1_out", };
+
  static const char *imx8mp_ecspi1_sels[] = {"clock-osc-24m", "sys_pll2_200m", 
"sys_pll1_40m",
  "sys_pll1_160m", "sys_pll1_800m", 
"sys_pll3_out",
  "sys_pll2_250m", 
"audio_pll2_out", };
@@ -270,6 +286,10 @@ static int imx8mp_clk_probe(struct udevice *dev)
clk_dm(IMX8MP_CLK_GIC, imx8m_clk_composite_critical("gic", 
imx8mp_gic_sels, base + 0xb200));
clk_dm(IMX8MP_CLK_ECSPI1, imx8m_clk_composite("ecspi1", 
imx8mp_ecspi1_sels, base + 0xb280));
clk_dm(IMX8MP_CLK_ECSPI2, imx8m_clk_composite("ecspi2", 
imx8mp_ecspi2_sels, base + 0xb300));
+   clk_dm(IMX8MP_CLK_PWM1, imx8m_clk_composite_critical("pwm1", 
imx8mp_pwm1_sels, base + 0xb380));
+   clk_dm(IMX8MP_CLK_PWM2, imx8m_clk_composite_critical("pwm2", 
imx8mp_pwm2_sels, base + 0xb400));
+   clk_dm(IMX8MP_CLK_PWM3, imx8m_clk_composite_critical("pwm3", 
imx8mp_pwm3_sels, base + 0xb480));
+   clk_dm(IMX8MP_CLK_PWM4, imx8m_clk_composite_critical("pwm4", 
imx8mp_pwm4_sels, base + 0xb500));
clk_dm(IMX8MP_CLK_ECSPI3, imx8m_clk_composite("ecspi3", 
imx8mp_ecspi3_sels, base + 0xc180));
  
  	clk_dm(IMX8MP_CLK_WDOG, imx8m_clk_composite("wdog", imx8mp_wdog_sels, base + 0xb900));

@@ -292,6 +312,10 @@ static int imx8mp_clk_probe(struct udevice *dev)
clk_dm(IMX8MP_CLK_I2C2_ROOT, imx_clk_gate4("i2c2_root_clk", "i2c2", 
base + 0x4180, 0));
clk_dm(IMX8MP_CLK_I2C3_ROOT, imx_clk_gate4("i2c3_root_clk", "i2c3", 
base + 0x4190, 0));
clk_dm(IMX8MP_CLK_I2C4_ROOT, imx_clk_gate4("i2c4_root_clk", "i2c4", 
base + 0x41a0, 0));
+   clk_dm(IMX8MP_CLK_PWM1_ROOT, imx_clk_gate4("pwm1_root_clk", "pwm1", 
base + 0x4280, 0));
+   clk_dm(IMX8MP_CLK_PWM2_ROOT, imx_clk_gate4("pwm2_root_clk", "pwm2", 
base + 0x4290, 0));
+   clk_dm(IMX8MP_CLK_PWM3_ROOT, imx_clk_gate4("pwm3_root_clk", "pwm3", 
base + 0x42a0, 0));
+   clk_dm(IMX8MP_CLK_PWM4_ROOT, imx_clk_gate4("pwm4_root_clk", "pwm4", 
base + 0x42b0, 0));
clk_dm(IMX8MP_CLK_QSPI_ROOT, imx_clk_gate4("qspi_root_clk", "qspi", 
base + 0x42f0, 0));
clk_dm(IMX8MP_CLK_I2C5_ROOT, imx_clk_gate2("i2c5_root_clk", "i2c5", 
base + 0x4330, 0));
clk_dm(IMX8MP_CLK_I2C6_ROOT, imx_clk_gate2("i2c6_root_clk", "i2c6", 
base + 0x4340, 0));


Acked-by: Sean Anderson 

But I would like to see a RB from one of the i.MX maintainers (CC'd).

Re: [PATCH 1/1] clk: sifive: avoid declaring static variables in includes

[Sean Anderson]

On 2/16/24 18:18, Heinrich Schuchardt wrote:

The existing code is unnecessarily convoluted:

Arrays __prci_init_clocks_fu[5|7]40  are initialized with data.
In separate includes fu[5|7]40-prci.h the size of the arrays is provided as
constants.

By moving the structures prci_clk_fu[5|7]40 to the respective code modules
we can directly use ARRAY_SIZE() to access the size of the data used for
initialization.

Signed-off-by: Heinrich Schuchardt 
---
  drivers/clk/sifive/fu540-prci.c  |  7 ++-
  drivers/clk/sifive/fu540-prci.h  | 22 --
  drivers/clk/sifive/fu740-prci.c  |  7 ++-
  drivers/clk/sifive/fu740-prci.h  | 22 --
  drivers/clk/sifive/sifive-prci.c |  3 +--
  drivers/clk/sifive/sifive-prci.h |  4 
  6 files changed, 17 insertions(+), 48 deletions(-)
  delete mode 100644 drivers/clk/sifive/fu540-prci.h
  delete mode 100644 drivers/clk/sifive/fu740-prci.h

diff --git a/drivers/clk/sifive/fu540-prci.c b/drivers/clk/sifive/fu540-prci.c
index ceb2c6fab0d..e55a26ab8fd 100644
--- a/drivers/clk/sifive/fu540-prci.c
+++ b/drivers/clk/sifive/fu540-prci.c
@@ -58,7 +58,7 @@ static const struct __prci_clock_ops 
sifive_fu540_prci_tlclksel_clk_ops = {
  };
  
  /* List of clock controls provided by the PRCI */

-struct __prci_clock __prci_init_clocks_fu540[] = {
+static struct __prci_clock __prci_init_clocks_fu540[] = {
[PRCI_CLK_COREPLL] = {
.name = "corepll",
.parent_name = "hfclk",
@@ -83,3 +83,8 @@ struct __prci_clock __prci_init_clocks_fu540[] = {
.ops = _fu540_prci_tlclksel_clk_ops,
},
  };
+
+const struct prci_clk_desc prci_clk_fu540 = {
+   .clks = __prci_init_clocks_fu540,
+   .num_clks = ARRAY_SIZE(__prci_init_clocks_fu540),
+};
diff --git a/drivers/clk/sifive/fu540-prci.h b/drivers/clk/sifive/fu540-prci.h
deleted file mode 100644
index 113301107da..000
--- a/drivers/clk/sifive/fu540-prci.h
+++ /dev/null
@@ -1,22 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-/*
- * Copyright (C) 2020-2021 SiFive, Inc.
- * Zong Li
- * Pragnesh Patel
- */
-
-#ifndef __SIFIVE_CLK_FU540_PRCI_H
-#define __SIFIVE_CLK_FU540_PRCI_H
-
-#include "sifive-prci.h"
-
-#define NUM_CLOCK_FU5404
-
-extern struct __prci_clock __prci_init_clocks_fu540[NUM_CLOCK_FU540];
-
-static const struct prci_clk_desc prci_clk_fu540 = {
-   .clks = __prci_init_clocks_fu540,
-   .num_clks = ARRAY_SIZE(__prci_init_clocks_fu540),
-};
-
-#endif /* __SIFIVE_CLK_FU540_PRCI_H */
diff --git a/drivers/clk/sifive/fu740-prci.c b/drivers/clk/sifive/fu740-prci.c
index 5edc864e4bd..4274b215d2f 100644
--- a/drivers/clk/sifive/fu740-prci.c
+++ b/drivers/clk/sifive/fu740-prci.c
@@ -102,7 +102,7 @@ static const struct __prci_clock_ops 
sifive_fu740_prci_pcieaux_clk_ops = {
  };
  
  /* List of clock controls provided by the PRCI */

-struct __prci_clock __prci_init_clocks_fu740[] = {
+static struct __prci_clock __prci_init_clocks_fu740[] = {
[FU740_PRCI_CLK_COREPLL] = {
.name = "corepll",
.parent_name = "hfclk",
@@ -156,3 +156,8 @@ struct __prci_clock __prci_init_clocks_fu740[] = {
.pwd = &__prci_pcieaux_data,
}
  };
+
+const struct prci_clk_desc prci_clk_fu740 = {
+   .clks = __prci_init_clocks_fu740,
+   .num_clks = ARRAY_SIZE(__prci_init_clocks_fu740),
+};
diff --git a/drivers/clk/sifive/fu740-prci.h b/drivers/clk/sifive/fu740-prci.h
deleted file mode 100644
index b74f0789061..000
--- a/drivers/clk/sifive/fu740-prci.h
+++ /dev/null
@@ -1,22 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-/*
- * Copyright (C) 2020-2021 SiFive, Inc.
- * Zong Li
- * Pragnesh Patel
- */
-
-#ifndef __SIFIVE_CLK_FU740_PRCI_H
-#define __SIFIVE_CLK_FU740_PRCI_H
-
-#include "sifive-prci.h"
-
-#define NUM_CLOCK_FU7409
-
-extern struct __prci_clock __prci_init_clocks_fu740[NUM_CLOCK_FU740];
-
-static const struct prci_clk_desc prci_clk_fu740 = {
-   .clks = __prci_init_clocks_fu740,
-   .num_clks = ARRAY_SIZE(__prci_init_clocks_fu740),
-};
-
-#endif /* __SIFIVE_CLK_FU740_PRCI_H */
diff --git a/drivers/clk/sifive/sifive-prci.c b/drivers/clk/sifive/sifive-prci.c
index c8fb6002907..603a176d06a 100644
--- a/drivers/clk/sifive/sifive-prci.c
+++ b/drivers/clk/sifive/sifive-prci.c
@@ -34,8 +34,7 @@
  #include 
  #include 
  
-#include "fu540-prci.h"

-#include "fu740-prci.h"
+#include "sifive-prci.h"
  
  /*

   * Private functions
diff --git a/drivers/clk/sifive/sifive-prci.h b/drivers/clk/sifive/sifive-prci.h
index 5ce33d61846..b391698081d 100644
--- a/drivers/clk/sifive/sifive-prci.h
+++ b/drivers/clk/sifive/sifive-prci.h
@@ -320,4 +320,8 @@ unsigned long sifive_prci_hfpclkplldiv_recalc_rate(struct 
__prci_clock *pc,
  
  int sifive_prci_clock_enable(struct __prci_clock *pc, bool enable);
  
+/* Clock driver data */

+extern const struct prci_clk_desc prci_clk_fu540;
+extern const struct prci_clk_desc prci_clk_fu740;
+
  #endif /* 

Re: [PATCH] clk: sifive: check wrpll_configure_for_rate() return value

[Sean Anderson]

On 2/16/24 12:06, Heinrich Schuchardt wrote:

wrpll_configure_for_rate() might fail. We should check the return value.

Fixes: d56d79ed27c6 ("drivers: clk: add fu740 support")
Signed-off-by: Heinrich Schuchardt 
---
  drivers/clk/sifive/sifive-prci.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/sifive/sifive-prci.c b/drivers/clk/sifive/sifive-prci.c
index c8fb6002907..a950736f11b 100644
--- a/drivers/clk/sifive/sifive-prci.c
+++ b/drivers/clk/sifive/sifive-prci.c
@@ -209,7 +209,9 @@ unsigned long sifive_prci_wrpll_round_rate(struct 
__prci_clock *pc,
  
  	memcpy(, >c, sizeof(c));
  
-	wrpll_configure_for_rate(, rate, *parent_rate);

+   r = wrpll_configure_for_rate(, rate, *parent_rate);
+   if (r)
+   return r;
  
  	return wrpll_calc_output_rate(, *parent_rate);

  }


Reviewed-by: Sean Anderson 

Re: [PATCH 1/1] clk: sifive: append missing \n to messages

[Sean Anderson]

On 2/16/24 11:35, Heinrich Schuchardt wrote:

If multiple messages are written, line-feeds improve the readability.

Fixes: c40b6df87fc0 ("clk: Add SiFive FU540 PRCI clock driver")
Signed-off-by: Heinrich Schuchardt 
---
  drivers/clk/analogbits/wrpll-cln28hpc.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/analogbits/wrpll-cln28hpc.c 
b/drivers/clk/analogbits/wrpll-cln28hpc.c
index a3cb109d357..537c696b727 100644
--- a/drivers/clk/analogbits/wrpll-cln28hpc.c
+++ b/drivers/clk/analogbits/wrpll-cln28hpc.c
@@ -81,7 +81,7 @@ static int __wrpll_calc_filter_range(unsigned long 
post_divr_freq)
  {
if (post_divr_freq < MIN_POST_DIVR_FREQ ||
post_divr_freq > MAX_POST_DIVR_FREQ) {
-   WARN(1, "%s: post-divider reference freq out of range: %lu",
+   WARN(1, "%s: post-divider reference freq out of range: %lu\n",
 __func__, post_divr_freq);
return -ERANGE;
}
@@ -229,7 +229,7 @@ int wrpll_configure_for_rate(struct wrpll_cfg *c, u32 
target_rate,
int range;
  
  	if (c->flags == 0) {

-   WARN(1, "%s called with uninitialized PLL config", __func__);
+   WARN(1, "%s called with uninitialized PLL config\n", __func__);
return -EINVAL;
}
  
@@ -335,7 +335,7 @@ unsigned long wrpll_calc_output_rate(const struct wrpll_cfg *c,

u64 n;
  
  	if (c->flags & WRPLL_FLAGS_EXT_FEEDBACK_MASK) {

-   WARN(1, "external feedback mode not yet supported");
+   WARN(1, "external feedback mode not yet supported\n");
return ULONG_MAX;
}
  


Reviewed-by: Sean Anderson 

But maybe these should be dev_dbg?

Re: [PATCH] clk: Fix error message in clk_get_bulk

[Sean Anderson]

On 3/9/24 07:27, Jan Kiszka wrote:

From: Jan Kiszka 

Fix a logical inversion of the printed text.

Signed-off-by: Jan Kiszka 
---
  drivers/clk/clk-uclass.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
index ed6e60bc484..78d8ea94c65 100644
--- a/drivers/clk/clk-uclass.c
+++ b/drivers/clk/clk-uclass.c
@@ -180,7 +180,7 @@ int clk_get_bulk(struct udevice *dev, struct clk_bulk *bulk)
  bulk_get_err:
err = clk_release_all(bulk->clks, bulk->count);
if (err)
-   debug("%s: could release all clocks for %p\n",
+   debug("%s: could not release all clocks for %p\n",
  __func__, dev);
  
  	return ret;


Reviewed-by: Sean Anderson 

Re: [PATCH v3] clk: set initial best mux parent to current parent with CLK_MUX_ROUND_CLOSEST

[Sean Anderson]

On 3/12/24 04:52, Yang Xiwen wrote:

On 3/11/2024 5:34 PM, Maxime Ripard wrote:

On Thu, Mar 07, 2024 at 07:18:05PM +0800, Yang Xiwen wrote:

On 3/7/2024 4:48 PM, Maxime Ripard wrote:

Hi,

On Thu, Mar 07, 2024 at 10:03:50AM +0800, Yang Xiwen via B4 Relay wrote:

From: Yang Xiwen 

Originally, the initial clock rate is hardcoded to 0, this can lead to
some problem when setting a very small rate with CLK_MUX_ROUND_CLOSEST.

For example, if the lowest possible rate provided by the mux is 1000Hz,
setting a rate below 500Hz will fail, because no clock can provide a
better rate than the non-existant 0Hz. But it should succeed with 1000Hz
being set.

Setting the initial best parent to current parent could solve this bug.

Signed-off-by: Yang Xiwen 

I don't think it would be the way to go. The biggest issue to me is that
it's inconsistent, and only changing the behaviour for a given flag
doesn't solve that.


I think the current behavior is odd but conforms to the document if
CLK_MUX_ROUND_CLOSEST is not specified.

clk_mux_determine_rate_flags isn't documented, and the determine_rate
clk_ops documentation doesn't mention it can return an error.


If i understand correctly, the default behavior of mux clocks is to
select the closest rate lower than requested rate, and
CLK_MUX_ROUND_CLOSEST removes the "lower than" limitation, which is
what this version tries to accomplish.

The situation is not as clear-cut as you make it to be, unfortunately.
The determine_rate clk_ops implementation states:

   Given a target rate as input, returns the closest rate actually
   supported by the clock, and optionally the parent clock that should be
   used to provide the clock rate.

So CLK_MUX_ROUND_CLOSEST shouldn't exist, because that's what
determine_rate expects so it should always be there.

Now, the "actually supported by the clock" can be interpreted in
multiple ways, and most importantly, doesn't state what the behaviour is
if we can't find a rate actually supported by the clock.

But now, this situation has been ambiguous for a while and thus drivers
kind of relied on that ambiguity.

So the way to fix it up is:

   - Assess what drivers are relying on
   - Document the current behaviour in clk_ops determine_rate



 From my investigation, it's totally a mess, especially for platform clk 
drivers (PLL). Some drivers always round down, the others round to nearest, 
with or without a specific flag to switch between them, depend on the division 
functions they choose. Fixing all of them seems needs quite a lot of time and 
would probably introduce some regressions.

We'd probably only have to say both rounding to nearest and rounding down are 
acceptable, though either one is preferred.




   - Make clk_mux_determine_rate_flags consistent with that



I think we must keep existing flags and document the current behavior correctly 
because of the massive existing users of clk_mux.


That's why i'm going to only fix CLK_MUX_ROUND_CLOSEST users. Hopefully it 
won't cause too many regressions.



   - Run that through kernelci to make sure we don't have any regression



We don't. I run 'tools/testing/kunit/kunit.py run --kunitconfig 
drivers/clk/.kunitconfig' each time before i send patches.


Over all, it seems quite a lot of work here.




Maxime



The situation here becomes even more complex when it comes to U-Boot clk framework. They chose slightly different prototypes and stated clk_set_rate() can fail with -ve. 


Maybe you mean clk_get_rate? Setting a rate can always fail due to the
nature of clocks...

Personally, I am not terribly attached to the API (as not many callers
handle errors correctly), but I have not had the time recently to do any
cleanup.

It's a great burden for clk driver authors and maintainers when they try to 
port their drivers to U-Boot. Let's Cc U-Boot clk maintainers as well, and see 
how we can resolve the mess here.




Regarding rounding mode, IMO it is better to let driver authors pick
whatever is convenient. Round closest is best, but there may be code size
savings for round lowest or some other scheme. [1] has the current
recommendation, which is to punt and let the caller use round_rate if
they actually care.

--Sean

[1] 
https://source.denx.de/u-boot/custodians/u-boot-clk/-/blob/a8dc4965f09d28a59c156437673ddb66860c847e/include/clk-uclass.h#L143

Re: [PATCH] clk: Propagate clk_set_rate() if CLK_SET_PARENT_RATE present

[Sean Anderson]

On 3/7/24 19:04, Sam Protsenko wrote:

Sometimes clocks provided to a consumer might not have .set_rate
operation (like gate or mux clocks), but have CLK_SET_PARENT_RATE flag
set. In that case it's usually possible to find a parent up the tree
which is capable of setting the rate (div, pll, etc). Implement a simple
lookup procedure for such cases, to traverse the clock tree until
.set_rate capable parent is found, and use that parent to actually
change the rate. The search will stop once the first .set_rate capable
clock is found, which is usually enough to handle most cases.

Signed-off-by: Sam Protsenko 
---
  drivers/clk/clk-uclass.c | 16 ++--
  1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
index ed6e60bc4841..755f05f34669 100644
--- a/drivers/clk/clk-uclass.c
+++ b/drivers/clk/clk-uclass.c
@@ -571,8 +571,20 @@ ulong clk_set_rate(struct clk *clk, ulong rate)
return 0;
ops = clk_dev_ops(clk->dev);
  
-	if (!ops->set_rate)

-   return -ENOSYS;
+   /* Try to find parents which can set rate */
+   while (!ops->set_rate) {
+   struct clk *parent;
+
+   if (!(clk->flags & CLK_SET_RATE_PARENT))
+   return -ENOSYS;
+
+   parent = clk_get_parent(clk);
+   if (IS_ERR_OR_NULL(parent) || !clk_valid(parent))
+   return -ENODEV;
+
+   clk = parent;
+   ops = clk_dev_ops(clk->dev);
+   }
  
  	/* get private clock struct used for cache */

clk_get_priv(clk, );


Can you give an example of where this is needed?

--Sean

Re: [PATCH 1/1] sandbox: use sane access rights for files

[Sean Anderson]

On 4/10/24 04:38, Heinrich Schuchardt wrote:

When writing an executable, allowing other users to modify it introduces
a security issue.

Generally we should avoid giving other users write access to our files by
default.

Replace chmod(777) by chmod(755) and chmod(644).

Fixes: 47f5fcfb4169 ("sandbox: Add os_jump_to_image() to run another 
executable")
Fixes: d9165153caea ("sandbox: add flags for open() call")
Fixes: 5c2859cdc302 ("sandbox: Allow reading/writing of RAM buffer")
Signed-off-by: Heinrich Schuchardt 
---
  arch/sandbox/cpu/os.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/sandbox/cpu/os.c b/arch/sandbox/cpu/os.c
index cbae5109e85..1cf41578010 100644
--- a/arch/sandbox/cpu/os.c
+++ b/arch/sandbox/cpu/os.c
@@ -109,7 +109,7 @@ int os_open(const char *pathname, int os_flags)
 */
flags |= O_CLOEXEC;
  
-	return open(pathname, flags, 0777);

+   return open(pathname, flags, 0644);
  }
  
  int os_close(int fd)

@@ -746,7 +746,7 @@ int os_write_ram_buf(const char *fname)
struct sandbox_state *state = state_get_current();
int fd, ret;
  
-	fd = open(fname, O_CREAT | O_WRONLY, 0777);

+   fd = open(fname, O_CREAT | O_WRONLY, 0644);
if (fd < 0)
return -ENOENT;
ret = write(fd, state->ram_buf, state->ram_size);
@@ -791,7 +791,7 @@ static int make_exec(char *fname, const void *data, int 
size)
if (write(fd, data, size) < 0)
return -EIO;
close(fd);
-   if (chmod(fname, 0777))
+   if (chmod(fname, 0755))
return -ENOEXEC;
  
  	return 0;


Reviewed-by: Sean Anderson 

Re: [PATCH 1/1] sandbox: don't call os_close with invalid file descriptor

[Sean Anderson]

On 4/10/24 17:50, Heinrich Schuchardt wrote:

If open() fails it returns -1. Calling close() with this value
makes no sense. Return -EIO instead.

Addresses-Coverity-ID: 185828 Improper use of negative value
Signed-off-by: Heinrich Schuchardt 
---
  arch/sandbox/cpu/os.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/sandbox/cpu/os.c b/arch/sandbox/cpu/os.c
index cbae5109e85..154a5d77490 100644
--- a/arch/sandbox/cpu/os.c
+++ b/arch/sandbox/cpu/os.c
@@ -188,7 +188,7 @@ int os_read_file(const char *fname, void **bufp, int *sizep)
fd = os_open(fname, OS_O_RDONLY);
if (fd < 0) {
printf("Cannot open file '%s'\n", fname);
-   goto err;
+   return -EIO;
}
size = os_filesize(fd);
if (size < 0) {


Fixes: 566bf3a8698 ("sandbox: Add a function to read a host file")
Reviewed-by: Sean Anderson 

Re: [PATCH 1/2] dm: core: add support for fallback drivers

[Sean Anderson]

On 4/10/24 15:44, Tom Rini wrote:

On Wed, Apr 10, 2024 at 07:27:17PM +0200, Heinrich Schuchardt wrote:



Am 10. April 2024 19:06:57 MESZ schrieb Caleb Connolly 
:

Introduce support for a uclass to provide a fallback/stub driver which
can be used when no device is found for a given node. This might be
useful for handling non-essential clock controllers like the RPMh on
Qualcomm platforms, or during early bringup to get UART output before a
real clock driver has been created.

Signed-off-by: Caleb Connolly 
---
drivers/core/Kconfig  | 10 ++
drivers/core/uclass.c | 24 +++-
include/dm/uclass.h   |  3 +++
3 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/drivers/core/Kconfig b/drivers/core/Kconfig
index 1081d61fcf01..09075b9b7a15 100644
--- a/drivers/core/Kconfig
+++ b/drivers/core/Kconfig
@@ -466,5 +466,15 @@ config BOUNCE_BUFFER

  A second possible use of bounce buffers is their ability to
  provide aligned buffers for DMA operations.

+menuconfig FALLBACK_DRIVERS


Wouldn't it be preferable to mark individual drivers as fallback drivers in 
their declaration?

This would allow alternative fallback drivers and would not require any 
definitions at uclass level.

Just by building a fallback driver you would enable the fallback behavior for 
its uclass.


I think some of this is addressed in the cover letter. My concern /
questions come down to I think just a matter of naming. Both "fallback"
and "stub" are used at times. As a concept, we have some areas where we
need a no-op driver because whereas the DT describes a relationship in
the hardware, here in U-Boot we can just accept things as configured. To
me "fallback" implies more functionality of the driver when it's really
just a generic no-op driver to fill in the dependencies within the tree.
Can we rename things a bit along those lines?



I would rather just have a stub driver with compatibles for all clocks we want
it to match. I think having a generic fallback driver could cause issues in the
future if we need to switch over to using a real driver.

--Sean

Re: [RFC PATCH v1 1/1] mmc: snps_sdhci: Add sdhci driver support for TH1520 SoC

[Sean Anderson]

On 3/30/24 13:59, Maksim Kiselev wrote:
> Add support for DesignWare SDHCI host controller on Alibaba TH1520 SoC
>
> Signed-off-by: Maksim Kiselev 
> ---
>  drivers/mmc/Kconfig  |  12 +
>  drivers/mmc/Makefile |   1 +
>  drivers/mmc/snps_sdhci.c | 464 +++
>  3 files changed, 477 insertions(+)
>  create mode 100644 drivers/mmc/snps_sdhci.c
>
> diff --git a/drivers/mmc/Kconfig b/drivers/mmc/Kconfig
> index cef05790dd..6936fa0928 100644
> --- a/drivers/mmc/Kconfig
> +++ b/drivers/mmc/Kconfig
> @@ -671,6 +671,18 @@ config MMC_SDHCI_S5P
>
> If unsure, say N.
>
> +config MMC_SDHCI_SNPS
> + bool "Synopsys DesignWare SDHCI controller"
> + depends on MMC_SDHCI
> + depends on DM_MMC
> + help
> +   Support for DesignWare SDHCI host controller on Alibaba TH1520 SoC.
> +   This is a highly configurable and programmable, high performance
> +   Mobile Storage Host Controller (MSHC) with AXI as the bus interface
> +   for data transfer.
> +
> +   If unsure, say N.
> +

Why not use MMC_DW?

--Sean


[Embedded World 2024, SECO 
SpA]

Re: [PATCH] arm64: zynqmp: Also support JTAG as alternative boot mode

[Sean Anderson]

On 3/22/24 07:53, Michal Simek wrote:
> 
> 
> On 3/21/24 17:20, Sean Anderson wrote:
>> On 3/20/24 07:18, Michal Simek wrote:
>>> if (reg >> BOOT_MODE_ALT_SHIFT) condition rules out alternative jtag boot
>>> mode which is 0. When 0 was used origin(HW) boot mode was used instead.
>>> That's why directly fill reg variable with requested boot mode and don't
>>> let code to read value back. "else" part of code remain unchanged.
>>>
>>> Signed-off-by: Michal Simek 
>>> ---
>>>
>>>   arch/arm/mach-zynqmp/spl.c | 5 +++--
>>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-zynqmp/spl.c b/arch/arm/mach-zynqmp/spl.c
>>> index 5af735aa5cef..979ff3aef6c2 100644
>>> --- a/arch/arm/mach-zynqmp/spl.c
>>> +++ b/arch/arm/mach-zynqmp/spl.c
>>> @@ -91,13 +91,14 @@ u32 spl_boot_device(void)
>>>     #if defined(CONFIG_SPL_ZYNQMP_ALT_BOOTMODE_ENABLED)
>>>   /* Change default boot mode at run-time */
>>> +    reg = CONFIG_SPL_ZYNQMP_ALT_BOOTMODE;
>>>   writel(CONFIG_SPL_ZYNQMP_ALT_BOOTMODE << BOOT_MODE_ALT_SHIFT,
>>>  _base->boot_mode);
>>> -#endif
>>> -
>>> +#else
>>>   reg = readl(_base->boot_mode);
>>>   if (reg >> BOOT_MODE_ALT_SHIFT)
>>>   reg >>= BOOT_MODE_ALT_SHIFT;
>>> +#endif
>>>     bootmode = reg & BOOT_MODES_MASK;
>>>   
>>
>> Looks fine; can we change this to
>>
>> if (IS_ENABLED(CONFIG_SPL_ZYNQMP_ALT_BOOTMODE_ENABLED)) {
>> ...
>> } else {
>> ...
>> }
> 
> Issue is that CONFIG_SPL_ZYNQMP_ALT_BOOTMODE symbol is not defined and 
> depends on CONFIG_SPL_ZYNQMP_ALT_BOOTMODE_ENABLED. It means you get 
> compilation error.
> That symbol can be setup and then what you have above can work.
> Is it worth? TBH I don't have preference. Please take a look at patch below.
> (And if v1 is fine then at least there should be added depends on 
> SPL_ZYNQMP_ALT_BOOTMODE_ENABLED to SPL_ZYNQMP_ALT_BOOTMODE which is missing 
> there now).
> 
> Thanks,
> Michal
> 
> diff --git a/arch/arm/mach-zynqmp/Kconfig b/arch/arm/mach-zynqmp/Kconfig
> index eee34380f0a0..75d3ec916a66 100644
> --- a/arch/arm/mach-zynqmp/Kconfig
> +++ b/arch/arm/mach-zynqmp/Kconfig
> @@ -145,7 +145,7 @@ config ZYNQ_SDHCI_MAX_FREQ
> 
>  config SPL_ZYNQMP_ALT_BOOTMODE
>     hex
> -   default 0x0 if JTAG_MODE
> +   default 0x0
>     default 0x1 if QSPI_MODE_24BIT
>     default 0x2 if QSPI_MODE_32BIT
>     default 0x3 if SD_MODE
> diff --git a/arch/arm/mach-zynqmp/spl.c b/arch/arm/mach-zynqmp/spl.c
> index 979ff3aef6c2..bbbf684ae496 100644
> --- a/arch/arm/mach-zynqmp/spl.c
> +++ b/arch/arm/mach-zynqmp/spl.c
> @@ -89,16 +89,16 @@ u32 spl_boot_device(void)
>     u32 reg = 0;
>     u8 bootmode;
> 
> -#if defined(CONFIG_SPL_ZYNQMP_ALT_BOOTMODE_ENABLED)
> -   /* Change default boot mode at run-time */
> -   reg = CONFIG_SPL_ZYNQMP_ALT_BOOTMODE;
> -   writel(CONFIG_SPL_ZYNQMP_ALT_BOOTMODE << BOOT_MODE_ALT_SHIFT,
> -  _base->boot_mode);
> -#else
> -   reg = readl(_base->boot_mode);
> -   if (reg >> BOOT_MODE_ALT_SHIFT)
> -   reg >>= BOOT_MODE_ALT_SHIFT;
> -#endif
> +   if (IS_ENABLED(CONFIG_SPL_ZYNQMP_ALT_BOOTMODE_ENABLED)) {
> +   /* Change default boot mode at run-time */
> +   reg = CONFIG_SPL_ZYNQMP_ALT_BOOTMODE;
> +   writel(CONFIG_SPL_ZYNQMP_ALT_BOOTMODE << BOOT_MODE_ALT_SHIFT,
> +  _base->boot_mode);
> +   } else {
> +   reg = readl(_base->boot_mode);
> +   if (reg >> BOOT_MODE_ALT_SHIFT)
> +   reg >>= BOOT_MODE_ALT_SHIFT;
> +   }
> 
>     bootmode = reg & BOOT_MODES_MASK;
> 
> 
> 

Reviewed-by: Sean Anderson 

Re: [PATCH] arm64: zynqmp: Also support JTAG as alternative boot mode

[Sean Anderson]

On 3/20/24 07:18, Michal Simek wrote:
> if (reg >> BOOT_MODE_ALT_SHIFT) condition rules out alternative jtag boot
> mode which is 0. When 0 was used origin(HW) boot mode was used instead.
> That's why directly fill reg variable with requested boot mode and don't
> let code to read value back. "else" part of code remain unchanged.
> 
> Signed-off-by: Michal Simek 
> ---
> 
>  arch/arm/mach-zynqmp/spl.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/mach-zynqmp/spl.c b/arch/arm/mach-zynqmp/spl.c
> index 5af735aa5cef..979ff3aef6c2 100644
> --- a/arch/arm/mach-zynqmp/spl.c
> +++ b/arch/arm/mach-zynqmp/spl.c
> @@ -91,13 +91,14 @@ u32 spl_boot_device(void)
>  
>  #if defined(CONFIG_SPL_ZYNQMP_ALT_BOOTMODE_ENABLED)
>   /* Change default boot mode at run-time */
> + reg = CONFIG_SPL_ZYNQMP_ALT_BOOTMODE;
>   writel(CONFIG_SPL_ZYNQMP_ALT_BOOTMODE << BOOT_MODE_ALT_SHIFT,
>  _base->boot_mode);
> -#endif
> -
> +#else
>   reg = readl(_base->boot_mode);
>   if (reg >> BOOT_MODE_ALT_SHIFT)
>   reg >>= BOOT_MODE_ALT_SHIFT;
> +#endif
>  
>   bootmode = reg & BOOT_MODES_MASK;
>  

Looks fine; can we change this to

if (IS_ENABLED(CONFIG_SPL_ZYNQMP_ALT_BOOTMODE_ENABLED)) {
...
} else {
...
}

?

--Sean

Re: [PATCH 1/2] clk: clk-imx8qxp: Add LPUART IPG entries

[Sean Anderson]

On 3/19/24 15:04, Tom Rini wrote:

On Mon, Mar 18, 2024 at 09:14:53PM -0300, Fabio Estevam wrote:

Hi Tom and Sean,

On Fri, Mar 8, 2024 at 5:13 PM Fabio Estevam  wrote:


Since commit cc7df0b9e8bc ("serial: lpuart: Enable IPG clock")
the colibri-imx8qxp board no longer boots.

The reason is that the imx8qxp clock driver does not handle the
LPUART IPG clocks inside get_rate(), set_rate() and enable() functions.

Fix the boot regression by adding the LPUART IPG entries.

Fixes: cc7df0b9e8bc ("serial: lpuart: Enable IPG clock")
Reported-by: Marcel Ziswiler 
Signed-off-by: Fabio Estevam 


Please consider applying this series to 2024.04.

It fixes a boot regression on imx8qxp and imx8qm.


Fine with me, Sean?



Acked-by: Sean Anderson 

Can you pick this up directly, Tom? Thanks.

Re: [PATCH] clk: Propagate clk_set_rate() if CLK_SET_PARENT_RATE present

[Sean Anderson]

On 3/20/24 10:02, Yang Xiwen wrote:

On 3/20/2024 2:42 AM, Sam Protsenko wrote:

On Thu, Mar 7, 2024 at 6:04 PM Sam Protsenko  wrote:

Sometimes clocks provided to a consumer might not have .set_rate
operation (like gate or mux clocks), but have CLK_SET_PARENT_RATE flag
set. In that case it's usually possible to find a parent up the tree
which is capable of setting the rate (div, pll, etc). Implement a simple
lookup procedure for such cases, to traverse the clock tree until
.set_rate capable parent is found, and use that parent to actually
change the rate. The search will stop once the first .set_rate capable
clock is found, which is usually enough to handle most cases.

Signed-off-by: Sam Protsenko 
---

Hi Lukasz, Sean, Tom,

If this patch looks good to you and there are no outstanding comments,
can you please apply it? It's needed as a part of eMMC enablement for
E850-96 board, as eMMC gate (leaf) clock is specified as ciu clock in
dts, which requires further clock rate change propagation up to
divider clock.

Thanks!



Please be patient. The release cycle is quite long. My patch set for HiSilicon 
clk framework has been ignored for ~2months already.


Sorry, I've been busy IRL recently.

I will try to review patches in the backlog, but it might be a few more weeks.

--Sean

Re: [PATCH] cmd: bootm: add ELF file support

[Sean Anderson]

Hi Maxim,

On 3/19/24 12:10, Maxim Moskalets wrote:
> [You don't often get email from maximmo...@gmail.com. Learn why this is 
> important at 
> https://cas5-0-urlprotect.trendmicro.com:443/wis/clicktime/v1/query?url=https%3a%2f%2faka.ms%2fLearnAboutSenderIdentification=d3728a4f-0e7b-4ded-94bc-5bd133b6deef=d807158c60b7d2502abde8a2fc01f40662980862-7a67d937c0016d0d3292f79af55f0ce3d78f480b
>  ]
>
> Some operating systems (e.g. seL4) and embedded applications are ELF
> images. It is convenient to use FIT-images to implement trusted boot.
> Added "elf" image type for booting using bootm command.

Why not use objcopy to create a binary?

--Sean

> Signed-off-by: Maxim Moskalets 
> ---
>  boot/bootm_os.c  | 21 +
>  boot/image-fit.c |  3 ++-
>  boot/image.c |  3 +++
>  include/image.h  |  1 +
>  4 files changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/boot/bootm_os.c b/boot/bootm_os.c
> index ccde72d22c..71bfb34926 100644
> --- a/boot/bootm_os.c
> +++ b/boot/bootm_os.c
> @@ -395,6 +395,24 @@ static int do_bootm_qnxelf(int flag, struct bootm_info 
> *bmi)
>  }
>  #endif
>
> +#if defined(CONFIG_CMD_ELF)
> +static int do_bootm_elf(int flag, struct bootm_info *bmi)
> +{
> +   struct bootm_headers *images = bmi->images;
> +   char *local_args[2] = {NULL};
> +   char str[19] = ""; /* "0x" + 16 digits + "\0" */
> +
> +   sprintf(str, "0x%lx", images->ep); /* write entry-point into string */
> +   str[18] = '\0';
> +   local_args[0] = bmi->argv[0];
> +   local_args[1] = str;/* and provide it via the arguments */
> +
> +   do_bootelf(NULL, 0, 2, local_args);
> +
> +   return 1;
> +}
> +#endif
> +
>  #ifdef CONFIG_INTEGRITY
>  static int do_bootm_integrity(int flag, struct bootm_info *bmi)
>  {
> @@ -536,6 +554,9 @@ static boot_os_fn *boot_os[] = {
>  #ifdef CONFIG_BOOTM_EFI
> [IH_OS_EFI] = do_bootm_efi,
>  #endif
> +#if defined(CONFIG_CMD_ELF)
> +   [IH_OS_ELF] = do_bootm_elf,
> +#endif
>  };
>
>  /* Allow for arch specific config before we boot */
> diff --git a/boot/image-fit.c b/boot/image-fit.c
> index 89e377563c..0419bef6d2 100644
> --- a/boot/image-fit.c
> +++ b/boot/image-fit.c
> @@ -2180,7 +2180,8 @@ int fit_image_load(struct bootm_headers *images, ulong 
> addr,
> fit_image_check_os(fit, noffset, IH_OS_TEE) ||
> fit_image_check_os(fit, noffset, IH_OS_OPENRTOS) ||
> fit_image_check_os(fit, noffset, IH_OS_EFI) ||
> -   fit_image_check_os(fit, noffset, IH_OS_VXWORKS);
> +   fit_image_check_os(fit, noffset, IH_OS_VXWORKS) ||
> +   fit_image_check_os(fit, noffset, IH_OS_ELF);
>
> /*
>  * If either of the checks fail, we should report an error, but
> diff --git a/boot/image.c b/boot/image.c
> index 073931cd7a..5b88d6808c 100644
> --- a/boot/image.c
> +++ b/boot/image.c
> @@ -134,6 +134,9 @@ static const table_entry_t uimage_os[] = {
>  #endif
> {   IH_OS_OPENSBI,  "opensbi",  "RISC-V OpenSBI",   },
> {   IH_OS_EFI,  "efi",  "EFI Firmware" },
> +#ifdef CONFIG_CMD_ELF
> +   {   IH_OS_ELF,  "elf",  "ELF Image" },
> +#endif
>
> {   -1, "", "", },
>  };
> diff --git a/include/image.h b/include/image.h
> index 21de70f0c9..9a40bca22c 100644
> --- a/include/image.h
> +++ b/include/image.h
> @@ -100,6 +100,7 @@ enum {
> IH_OS_TEE,  /* Trusted Execution Environment */
> IH_OS_OPENSBI,  /* RISC-V OpenSBI */
> IH_OS_EFI,  /* EFI Firmware (e.g. GRUB2) */
> +   IH_OS_ELF,  /* ELF Image (e.g. seL4) */
>
> IH_OS_COUNT,
>  };
> --
> 2.39.2
>


[Embedded World 2024, SECO 
SpA]

Re: [PATCH v2] cmd: nand: Add support to print the manufacturer, model and size

[Sean Anderson]

On 3/18/24 09:42, Mihai Sain wrote:

Add support to nand info for printing the manufacturer,model and size.
The manufacturer and model are printed only for ONFI flashes.

U-Boot> nand info

Device 0: nand0, sector size 256 KiB
   Manufacturer  MACRONIX
   Model MX30LF4G28AD
   Device size512 MiB
   Page size 4096 b
   OOB size   256 b
   Erase size  262144 b
   ecc strength 8 bits
   ecc step size  512 b
   subpagesize   4096 b
   options   0x4200
   bbt options   0x00028000

Signed-off-by: Mihai Sain 

Changes in v2:
--
* use #ifdef directive for ONFI flashes.
* use unsigned int for chipsize.
---
  cmd/nand.c | 5 +
  1 file changed, 5 insertions(+)

diff --git a/cmd/nand.c b/cmd/nand.c
index fe834c4ac5..5773246d64 100644
--- a/cmd/nand.c
+++ b/cmd/nand.c
@@ -418,6 +418,11 @@ static void nand_print_and_set_info(int idx)
printf("%dx ", chip->numchips);
printf("%s, sector size %u KiB\n",
   mtd->name, mtd->erasesize >> 10);
+#ifdef CONFIG_SYS_NAND_ONFI_DETECTION
+   printf("  Manufacturer  %s \n", chip->onfi_params.manufacturer);
+   printf("  Model %s \n", chip->onfi_params.model);
+#endif
+   printf("  Device size   %8u MiB\n", (unsigned int)(chip->chipsize >> 
20));
printf("  Page size %8d b\n", mtd->writesize);
printf("  OOB size  %8d b\n", mtd->oobsize);
printf("  Erase size%8d b\n", mtd->erasesize);


Can you refactor the logic out of the end of nand_detect as a separate
function and use that instead? That will cover more cases (e.g. JEDEC).

--Sean

Re: [PATCH v5] misc: fs-loader: Use fw_storage_interface instead of storage_interface

[Sean Anderson]

Hi Danish,

On 2/27/24 05:26, MD Danish Anwar wrote:

On 09/02/24 3:38 pm, MD Danish Anwar wrote:

The fs-loader driver reads env storage_interface and uses it to load
firmware file into memory using the medium set by env. Update the driver
to use env fw_storage_interface as this variable is only used to load
firmwares. This is to keep all variables used by fs-loader driver with
'fw_' prefix. All other variables have 'fw_' prefix except for
storage_interface.

The env storage_interface will act as fallback so that the
existing implementations do not break.

Also update the FS Loader documentation accordingly.

Signed-off-by: MD Danish Anwar 
---


Hi Tom / Sean, can you please pick this patch if there is no pending
comments to address.



Sorry, I forgot to respond to this earlier.

To be honest, I'm not really convinced. We have plenty of environmental
variables which are inconsistent (e.g. ethaddr, eth2addr, eth3addr) and it
doesn't cause any issues. While fixing code has no cost, the environment
is an ABI which we can't break. So we'd have to support both of these
variables forever. I'm not really a fan of doing that without good reason,
and I think aesthetics of the variable name isn't really compelling.

--Sean

Re: [PATCH] clk: Revise help text for clk_get_parent_rate()

[Sean Anderson]

On 2/23/24 07:01, Alexander Dahl wrote:

The function returns the rate of the parent clock, the previous text
made no sense at all.

Fixes: 4aa78300a025 ("dm: clk: Define clk_get_parent_rate() for clk operations")
Signed-off-by: Alexander Dahl 
---
  include/clk.h | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/clk.h b/include/clk.h
index af23e4f3475..045e923a529 100644
--- a/include/clk.h
+++ b/include/clk.h
@@ -444,7 +444,7 @@ ulong clk_get_rate(struct clk *clk);
  struct clk *clk_get_parent(struct clk *clk);
  
  /**

- * clk_get_parent_rate() - Get parent of current clock rate.
+ * clk_get_parent_rate() - Get rate of current clock's parent.
   * @clk:  A clock struct that was previously successfully requested by
   *clk_request/get_by_*().
   *

base-commit: 7bb761c42d75b2ebacc7190a76cc5385cbba1045


Reviewed-by: Sean Anderson 

Re: [PATCH] Check curve_name for null to avoid crash

[Sean Anderson]

On 2/22/24 17:18, Bob Wolff wrote:

If mixed rsa and ecdsa keys are specified in dtsi, an rsa key can be sent
into the ecdsa verify. Without the ecdsa,curve property, this function will
crash due to lack of checking the null pointer return.


nit: there should be a blank line here


Signed-off-by: Bob Wolff 
---

  lib/ecdsa/ecdsa-verify.c | 5 +
  1 file changed, 5 insertions(+)

diff --git a/lib/ecdsa/ecdsa-verify.c b/lib/ecdsa/ecdsa-verify.c
index 0601700c4f..4d1835b598 100644
--- a/lib/ecdsa/ecdsa-verify.c
+++ b/lib/ecdsa/ecdsa-verify.c
@@ -31,6 +31,11 @@ static int fdt_get_key(struct ecdsa_public_key *key, const 
void *fdt, int node)
int x_len, y_len;
  
  	key->curve_name = fdt_getprop(fdt, node, "ecdsa,curve", NULL);

+   if (!key->curve_name) {
+   debug("Error: ecdsa cannot get 'ecdsa,curve' property from key. 
Likely not an ecdsa key.\n");
+   return -ENOMSG;
+   }
+
key->size_bits = ecdsa_key_size(key->curve_name);
if (key->size_bits == 0) {
debug("Unknown ECDSA curve '%s'", key->curve_name);


Reviewed-by: Sean Anderson 

[RESEND PATCH v2] arm64: zynqmp: Support semihosting boot method

[Sean Anderson]

Currently, when we boot from JTAG we try to boot U-Boot from RAM.
However, this is a bit tricky to time, since the debugger has to wait
for SPL to initialize RAM before it can load U-Boot. This can result in
long waits, since occasionally initializing RAM (and other things in
psu_init) takes a long time to complete and the debugger must wait for
this worst case.

Support semihosting if it is enabled, as it lets U-Boot tell the
debugger when we are ready for the image. This means we don't have to
wait any more than necessary. We don't change the default config to
ensure we don't break compatibility with existing debuggers that don't
expect us to hit semihosting breakpoints.

Signed-off-by: Sean Anderson 
---
I'm resending this from my linux.dev email as my regular email is
mangling my patches.

Changes in v2:
- Fix typo in subject

 arch/arm/mach-zynqmp/spl.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-zynqmp/spl.c b/arch/arm/mach-zynqmp/spl.c
index a0f35f36faa..5af735aa5ce 100644
--- a/arch/arm/mach-zynqmp/spl.c
+++ b/arch/arm/mach-zynqmp/spl.c
@@ -9,6 +9,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -66,6 +67,11 @@ void spl_board_init(void)
 }
 #endif
 
+static u32 jtag_boot_device(void)
+{
+   return semihosting_enabled() ? BOOT_DEVICE_SMH : BOOT_DEVICE_RAM;
+}
+
 void board_boot_order(u32 *spl_boot_list)
 {
spl_boot_list[0] = spl_boot_device();
@@ -75,7 +81,7 @@ void board_boot_order(u32 *spl_boot_list)
if (spl_boot_list[0] == BOOT_DEVICE_MMC2)
spl_boot_list[1] = BOOT_DEVICE_MMC1;
 
-   spl_boot_list[2] = BOOT_DEVICE_RAM;
+   spl_boot_list[2] = jtag_boot_device();
 }
 
 u32 spl_boot_device(void)
@@ -97,7 +103,7 @@ u32 spl_boot_device(void)
 
switch (bootmode) {
case JTAG_MODE:
-   return BOOT_DEVICE_RAM;
+   return jtag_boot_device();
 #ifdef CONFIG_SPL_MMC
case SD_MODE1:
case SD1_LSHFT_MODE: /* not working on silicon v1 */
-- 
2.35.1.1320.gc452695387.dirty

Re: [PATCH v2] arm64: zynqmp: Support semihosting boot method

[Sean Anderson]

On 2/22/24 14:36, Fabio Estevam wrote:
> On Thu, Feb 22, 2024 at 1:53 PM Sean Anderson  wrote:
>>
>> From: Sean Anderson 
> ...
>> Signed-off-by: Sean Anderson 
>> ---
>> I'm resending this from my linux.dev email as my regular email is
>> mangling my patches.
> 
> That's OK, but you need to make sure that the From and Signed-off-by
> fields match.
> 
> Otherwise, checkpatch complains.

Yeah, I thought I had fixed the authors, but I guess not.

--Sean

Re: [PATCH] arm64: zynqmp: Support semhosting boot method

[Sean Anderson]

On 2/22/24 05:34, Michal Simek wrote:
>
>
> On 2/20/24 20:30, Sean Anderson wrote:
>> On 2/20/24 14:18, Michal Simek wrote:
>>>
>>>
>>> On 2/20/24 19:43, Sean Anderson wrote:
>>>> On 2/20/24 13:24, Michal Simek wrote:
>>>>>
>>>>>
>>>>> On 2/16/24 17:09, Sean Anderson wrote:
>>>>>> On 2/16/24 11:03, Sean Anderson wrote:
>>>>>>> On 2/16/24 10:06, Michal Simek wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 2/16/24 14:48, Michal Simek wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 2/15/24 20:31, Sean Anderson wrote:
>>>>>>>>>> On 2/15/24 14:08, Michal Simek wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 2/15/24 18:19, Sean Anderson wrote:
>>>>>>>>>>>> Currently, when we boot from JTAG we try to boot U-Boot from RAM.
>>>>>>>>>>>> However, this is a bit tricky to time, since the debugger has to 
>>>>>>>>>>>> wait
>>>>>>>>>>>> for SPL to initialize RAM before it can load U-Boot. This can 
>>>>>>>>>>>> result in
>>>>>>>>>>>> long waits, since occasionally initializing RAM (and other things 
>>>>>>>>>>>> in
>>>>>>>>>>>> psu_init) takes a long time to complete and the debugger must wait 
>>>>>>>>>>>> for
>>>>>>>>>>>> this worst case.
>>>>>>>>>>>>
>>>>>>>>>>>> Support semihosting if it is enabled, as it lets U-Boot tell the
>>>>>>>>>>>> debugger when we are ready for the image. This means we don't have 
>>>>>>>>>>>> to
>>>>>>>>>>>> wait any more than necessary. We don't change the default config to
>>>>>>>>>>>> ensure we don't break compatibility with existing debuggers that 
>>>>>>>>>>>> don't
>>>>>>>>>>>> expect us to hit semihosting breakpoints.
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Sean Anderson 
>>>>>>>>>>>> ---
>>>>>>>>>>>>
>>>>>>>>>>>>   arch/arm/mach-zynqmp/spl.c | 10 --
>>>>>>>>>>>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>>>>>>>>>>>
>>>>>>>>>>>> diff --git a/arch/arm/mach-zynqmp/spl.c 
>>>>>>>>>>>> b/arch/arm/mach-zynqmp/spl.c
>>>>>>>>>>>> index a0f35f36faa..5af735aa5ce 100644
>>>>>>>>>>>> --- a/arch/arm/mach-zynqmp/spl.c
>>>>>>>>>>>> +++ b/arch/arm/mach-zynqmp/spl.c
>>>>>>>>>>>> @@ -9,6 +9,7 @@
>>>>>>>>>>>>   #include 
>>>>>>>>>>>>   #include 
>>>>>>>>>>>>   #include 
>>>>>>>>>>>> +#include 
>>>>>>>>>>>>   #include 
>>>>>>>>>>>>   #include 
>>>>>>>>>>>>
>>>>>>>>>>>> @@ -66,6 +67,11 @@ void spl_board_init(void)
>>>>>>>>>>>>   }
>>>>>>>>>>>>   #endif
>>>>>>>>>>>>
>>>>>>>>>>>> +static u32 jtag_boot_device(void)
>>>>>>>>>>>> +{
>>>>>>>>>>>> +   return semihosting_enabled() ? BOOT_DEVICE_SMH : 
>>>>>>>>>>>> BOOT_DEVICE_RAM;
>>>>>>>>>>>> +}
>>>>>>>>>>>> +
>>>>>>>>>>>>   void board_boot_order(u32 *spl_boot_list)
>>>>>>>>>>>>   {
>>>>>>>>>>>>  spl_boot_list[0] = spl_boot_device();
>>>>>>>>>>>> @@ -7

[PATCH v2] arm64: zynqmp: Support semihosting boot method

[Sean Anderson]

From: Sean Anderson 

Currently, when we boot from JTAG we try to boot U-Boot from RAM.
However, this is a bit tricky to time, since the debugger has to wait
for SPL to initialize RAM before it can load U-Boot. This can result in
long waits, since occasionally initializing RAM (and other things in
psu_init) takes a long time to complete and the debugger must wait for
this worst case.

Support semihosting if it is enabled, as it lets U-Boot tell the
debugger when we are ready for the image. This means we don't have to
wait any more than necessary. We don't change the default config to
ensure we don't break compatibility with existing debuggers that don't
expect us to hit semihosting breakpoints.

Signed-off-by: Sean Anderson 
---
I'm resending this from my linux.dev email as my regular email is
mangling my patches.

Changes in v2:
- Fix typo in subject

 arch/arm/mach-zynqmp/spl.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-zynqmp/spl.c b/arch/arm/mach-zynqmp/spl.c
index a0f35f36faa..5af735aa5ce 100644
--- a/arch/arm/mach-zynqmp/spl.c
+++ b/arch/arm/mach-zynqmp/spl.c
@@ -9,6 +9,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -66,6 +67,11 @@ void spl_board_init(void)
 }
 #endif
 
+static u32 jtag_boot_device(void)
+{
+   return semihosting_enabled() ? BOOT_DEVICE_SMH : BOOT_DEVICE_RAM;
+}
+
 void board_boot_order(u32 *spl_boot_list)
 {
spl_boot_list[0] = spl_boot_device();
@@ -75,7 +81,7 @@ void board_boot_order(u32 *spl_boot_list)
if (spl_boot_list[0] == BOOT_DEVICE_MMC2)
spl_boot_list[1] = BOOT_DEVICE_MMC1;
 
-   spl_boot_list[2] = BOOT_DEVICE_RAM;
+   spl_boot_list[2] = jtag_boot_device();
 }
 
 u32 spl_boot_device(void)
@@ -97,7 +103,7 @@ u32 spl_boot_device(void)
 
switch (bootmode) {
case JTAG_MODE:
-   return BOOT_DEVICE_RAM;
+   return jtag_boot_device();
 #ifdef CONFIG_SPL_MMC
case SD_MODE1:
case SD1_LSHFT_MODE: /* not working on silicon v1 */
-- 
2.35.1.1320.gc452695387.dirty

Re: [PATCH] ecdsa: Avoid null pointer crash in ecdsa-verify due to absent property

[Sean Anderson]

Hi Bob,

On 2/21/24 19:27, Bob Wolff wrote:

If mixed rsa and ecdsa keys are specified in
dtsi, an rsa key can be sent into the ecdsa
verify. Without the ecdsa,curve property, this
function will crash due to lack of checking
the null pointer return.


You can wrap commit messages at 75 characters


Signed-off-by: Bob Wolff 
---

  lib/ecdsa/ecdsa-verify.c | 5 +
  1 file changed, 5 insertions(+)

diff --git a/lib/ecdsa/ecdsa-verify.c b/lib/ecdsa/ecdsa-verify.c
index 0601700c4f..01ffc3477c 100644
--- a/lib/ecdsa/ecdsa-verify.c
+++ b/lib/ecdsa/ecdsa-verify.c
@@ -31,6 +31,11 @@ static int fdt_get_key(struct ecdsa_public_key *key,
const void *fdt, int node)
   int x_len, y_len;

   key->curve_name = fdt_getprop(fdt, node, "ecdsa,curve", NULL);
+ if (!key->curve_name) {
+ printf("Error: ecdsa cannot get 'ecdsa,curve' property from key. Likely
not an ecdsa key.\n");


this should probably be a debug (like the below message)


+ return -ENOMSG;
+ }
+


and it looks like something ate your indentation

--Sean


   key->size_bits = ecdsa_key_size(key->curve_name);
   if (key->size_bits == 0) {
   debug("Unknown ECDSA curve '%s'", key->curve_name);
--
2.39.3 (Apple Git-145)

Re: [PATCH] arm64: zynqmp: Support semhosting boot method

[Sean Anderson]

On 2/20/24 14:18, Michal Simek wrote:
>
>
> On 2/20/24 19:43, Sean Anderson wrote:
>> On 2/20/24 13:24, Michal Simek wrote:
>>>
>>>
>>> On 2/16/24 17:09, Sean Anderson wrote:
>>>> On 2/16/24 11:03, Sean Anderson wrote:
>>>>> On 2/16/24 10:06, Michal Simek wrote:
>>>>>>
>>>>>>
>>>>>> On 2/16/24 14:48, Michal Simek wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 2/15/24 20:31, Sean Anderson wrote:
>>>>>>>> On 2/15/24 14:08, Michal Simek wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 2/15/24 18:19, Sean Anderson wrote:
>>>>>>>>>> Currently, when we boot from JTAG we try to boot U-Boot from RAM.
>>>>>>>>>> However, this is a bit tricky to time, since the debugger has to wait
>>>>>>>>>> for SPL to initialize RAM before it can load U-Boot. This can result 
>>>>>>>>>> in
>>>>>>>>>> long waits, since occasionally initializing RAM (and other things in
>>>>>>>>>> psu_init) takes a long time to complete and the debugger must wait 
>>>>>>>>>> for
>>>>>>>>>> this worst case.
>>>>>>>>>>
>>>>>>>>>> Support semihosting if it is enabled, as it lets U-Boot tell the
>>>>>>>>>> debugger when we are ready for the image. This means we don't have to
>>>>>>>>>> wait any more than necessary. We don't change the default config to
>>>>>>>>>> ensure we don't break compatibility with existing debuggers that 
>>>>>>>>>> don't
>>>>>>>>>> expect us to hit semihosting breakpoints.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Sean Anderson 
>>>>>>>>>> ---
>>>>>>>>>>
>>>>>>>>>>  arch/arm/mach-zynqmp/spl.c | 10 --
>>>>>>>>>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/arch/arm/mach-zynqmp/spl.c b/arch/arm/mach-zynqmp/spl.c
>>>>>>>>>> index a0f35f36faa..5af735aa5ce 100644
>>>>>>>>>> --- a/arch/arm/mach-zynqmp/spl.c
>>>>>>>>>> +++ b/arch/arm/mach-zynqmp/spl.c
>>>>>>>>>> @@ -9,6 +9,7 @@
>>>>>>>>>>  #include 
>>>>>>>>>>  #include 
>>>>>>>>>>  #include 
>>>>>>>>>> +#include 
>>>>>>>>>>  #include 
>>>>>>>>>>  #include 
>>>>>>>>>>
>>>>>>>>>> @@ -66,6 +67,11 @@ void spl_board_init(void)
>>>>>>>>>>  }
>>>>>>>>>>  #endif
>>>>>>>>>>
>>>>>>>>>> +static u32 jtag_boot_device(void)
>>>>>>>>>> +{
>>>>>>>>>> +   return semihosting_enabled() ? BOOT_DEVICE_SMH : 
>>>>>>>>>> BOOT_DEVICE_RAM;
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>>  void board_boot_order(u32 *spl_boot_list)
>>>>>>>>>>  {
>>>>>>>>>> spl_boot_list[0] = spl_boot_device();
>>>>>>>>>> @@ -75,7 +81,7 @@ void board_boot_order(u32 *spl_boot_list)
>>>>>>>>>> if (spl_boot_list[0] == BOOT_DEVICE_MMC2)
>>>>>>>>>> spl_boot_list[1] = BOOT_DEVICE_MMC1;
>>>>>>>>>>
>>>>>>>>>> -   spl_boot_list[2] = BOOT_DEVICE_RAM;
>>>>>>>>>> +   spl_boot_list[2] = jtag_boot_device();
>>>>>>>>>>  }
>>>>>>>>>>
>>>>>>>>>>  u32 spl_boot_device(void)
>>>>>>>>>> @@ -97,7 +103,7 @@ u32 spl_boot_device(void)
>>>>>>>>>>
>>>>>>>>>> switch (bootmode) {
>>>>>>>&

Re: [PATCH] arm64: zynqmp: Support semhosting boot method

[Sean Anderson]

On 2/20/24 13:24, Michal Simek wrote:
>
>
> On 2/16/24 17:09, Sean Anderson wrote:
>> On 2/16/24 11:03, Sean Anderson wrote:
>>> On 2/16/24 10:06, Michal Simek wrote:
>>>>
>>>>
>>>> On 2/16/24 14:48, Michal Simek wrote:
>>>>>
>>>>>
>>>>> On 2/15/24 20:31, Sean Anderson wrote:
>>>>>> On 2/15/24 14:08, Michal Simek wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 2/15/24 18:19, Sean Anderson wrote:
>>>>>>>> Currently, when we boot from JTAG we try to boot U-Boot from RAM.
>>>>>>>> However, this is a bit tricky to time, since the debugger has to wait
>>>>>>>> for SPL to initialize RAM before it can load U-Boot. This can result in
>>>>>>>> long waits, since occasionally initializing RAM (and other things in
>>>>>>>> psu_init) takes a long time to complete and the debugger must wait for
>>>>>>>> this worst case.
>>>>>>>>
>>>>>>>> Support semihosting if it is enabled, as it lets U-Boot tell the
>>>>>>>> debugger when we are ready for the image. This means we don't have to
>>>>>>>> wait any more than necessary. We don't change the default config to
>>>>>>>> ensure we don't break compatibility with existing debuggers that don't
>>>>>>>> expect us to hit semihosting breakpoints.
>>>>>>>>
>>>>>>>> Signed-off-by: Sean Anderson 
>>>>>>>> ---
>>>>>>>>
>>>>>>>> arch/arm/mach-zynqmp/spl.c | 10 --
>>>>>>>> 1 file changed, 8 insertions(+), 2 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/arch/arm/mach-zynqmp/spl.c b/arch/arm/mach-zynqmp/spl.c
>>>>>>>> index a0f35f36faa..5af735aa5ce 100644
>>>>>>>> --- a/arch/arm/mach-zynqmp/spl.c
>>>>>>>> +++ b/arch/arm/mach-zynqmp/spl.c
>>>>>>>> @@ -9,6 +9,7 @@
>>>>>>>> #include 
>>>>>>>> #include 
>>>>>>>> #include 
>>>>>>>> +#include 
>>>>>>>> #include 
>>>>>>>> #include 
>>>>>>>>
>>>>>>>> @@ -66,6 +67,11 @@ void spl_board_init(void)
>>>>>>>> }
>>>>>>>> #endif
>>>>>>>>
>>>>>>>> +static u32 jtag_boot_device(void)
>>>>>>>> +{
>>>>>>>> +   return semihosting_enabled() ? BOOT_DEVICE_SMH : 
>>>>>>>> BOOT_DEVICE_RAM;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> void board_boot_order(u32 *spl_boot_list)
>>>>>>>> {
>>>>>>>>spl_boot_list[0] = spl_boot_device();
>>>>>>>> @@ -75,7 +81,7 @@ void board_boot_order(u32 *spl_boot_list)
>>>>>>>>if (spl_boot_list[0] == BOOT_DEVICE_MMC2)
>>>>>>>>spl_boot_list[1] = BOOT_DEVICE_MMC1;
>>>>>>>>
>>>>>>>> -   spl_boot_list[2] = BOOT_DEVICE_RAM;
>>>>>>>> +   spl_boot_list[2] = jtag_boot_device();
>>>>>>>> }
>>>>>>>>
>>>>>>>> u32 spl_boot_device(void)
>>>>>>>> @@ -97,7 +103,7 @@ u32 spl_boot_device(void)
>>>>>>>>
>>>>>>>>switch (bootmode) {
>>>>>>>>case JTAG_MODE:
>>>>>>>> -   return BOOT_DEVICE_RAM;
>>>>>>>> +   return jtag_boot_device();
>>>>>>>> #ifdef CONFIG_SPL_MMC
>>>>>>>>case SD_MODE1:
>>>>>>>>case SD1_LSHFT_MODE: /* not working on silicon v1 */
>>>>>>>
>>>>>>> Good timing. Can you please tell me how to test this? What's the setup?
>>>>>>> Which debugger are you using?
>>>>>>
>>>>>> I am using OpenOCD with the patches at 
>>>>>> https://cas5-0-urlprotect.trendmicro.com:443/wis/clicktime/v1/query?url=https%3a%2f%2freview.openocd.org%2fc%

Re: HABv4 with SPL and u-boot-dtb.img on i.MX6

[Sean Anderson]

On 2/20/24 04:50, Benjamin Lemouzy wrote:
> Hello,
>
> I'm trying to make secure boot work on i.MX6 SABRE with SPL and 
> u-boot-dtb.img files and I'm not sure how to do it.
>
> I'm using the U-Boot vanilla master branch (2024.04-rc2) with the following 
> configuration:
>
> # Remove some stuff to not exceed file size limit
> $ cat <> configs/mx6sabresd_defconfig
> CONFIG_BOOTMETH_EFILOADER=n
> CONFIG_CMD_NET=n
> CONFIG_NET=n
> EOF
>
> # Enable secure boot
> $ cat <> configs/mx6sabresd_defconfig
> CONFIG_IMX_HAB=y
> CONFIG_SPL_LOAD_FIT_ADDRESS=0x1800
> EOF
>
> $ make ARCH=arm O=build mx6sabresd_defconfig
>
> $ make ARCH=arm O=build
>
> I have no issue to generate a working SPL-signed file following 
> doc/imx/habv4/guides/mx6_mx7_spl_secure_boot.txt instructions.
>
> doc/imx/habv4/guides/mx6_mx7_spl_secure_boot.txt only gives instructions to 
> sign u-boot-ivt.img but this file doesn't contain device trees listed in 
> CONFIG_OF_LIST as u-boot-dtb.img does and I need them.
>
>
>
> NXP AN4581 lists 2 possible formats to sign additional images:
>
> - Image format:
>
> --- +-+ <-- *load_address
> ^   | |
> |   | |
> |   |  Image data |
>  Signed |   | |
>   Data  |   | |
> |   +-+
> |   |Padding Next Boundary|
> |   +-+ <-- *ivt
> v   | Image Vector Table  |
> --- +-+ <-- *csf
> | |
> | Command Sequence File (CSF) |
> | |
> +-+
> | Padding (optional)  |
> +-+
>
> - FIT image format:
>
> --- +-+ ---
> ^   | |^
> |   | ||
> |   |   FDT FIT   ||
> |   | ||
> Signed data |   | ||
> |   +-+|
> |   |Padding Next Boundary||
> |   +-+|
> v   | Image Vector Table  ||
> --- +-+| FIT image
> | ||
> | Command Sequence File (CSF) ||
> | ||
> +-+|
> | Padding (optional)  ||
> --- +-+|
> ^   | ||
> Signed data |   |   U-Boot||
> v   | |v
> --- +-+ ---
>
> And as u-boot-dtb.img is a FIT image, I probably have to use the FIT image 
> format, right?
>
>
>
> I manually craft the signed FIT image using 
> doc/imx/habv4/csf_examples/mx8m/csf.sh as reference and everything looks fine:
>
> U-Boot SPL 2024.04-rc2-00025-g9e00b6993f-dirty (Feb 19 2024 - 13:17:31 
> +0100)
> >>SPL: board_init_r()
> spl_init
> Trying to boot from MMC1
> fit read offset 11400, size=12800, dst=1800, count=12800
> spl_load_simple_fit_fix_load: ivt: 18001000 offset: 1000 size: 3060
> spl_load_simple_fit_fix_load: ivt self: 18001000
> hab fuse not enabled
>
> Authenticate image from DDR location 0x1800...
>
> ivt_offset = 0x1000, ivt addr = 0x18001000
> ivt entry = 0x1800, dcd = 0x, csf = 0x18001020
> Dumping IVT
> .. @
>  ...
> Dumping CSF Header
> ..PC...P
> 
> ...<
> ...8
>
> Calling authenticate_image in ROM
> ivt_offset = 0x1000
> start = 0x1800
> bytes = 0x3060
> firmware: 'firmware-1'
> External data: dst=1780, offset=3060, size=86138
> Image OS is U-Boot
> fdt: 'fdt-1'
> Can't get 'load' property from FIT 0x1800, node: offset 464, name 
> fdt-1 (FDT_ERR_NOTFOUND)
> External data: dst=17886140, offset=89198, size=ac00
> Can't get 'entry' property from FIT 0x1800, node: offset 464, name 
> fdt-1 (FDT_ERR_NOTFOUND)
> loadables: 'firmware-1'
> no string for index 1
> Jumping to U-Boot...
> SPL malloc() used 0x0 

Re: [PATCH] arm64: zynqmp: Support semhosting boot method

[Sean Anderson]

On 2/16/24 11:03, Sean Anderson wrote:
> On 2/16/24 10:06, Michal Simek wrote:
>>
>>
>> On 2/16/24 14:48, Michal Simek wrote:
>>>
>>>
>>> On 2/15/24 20:31, Sean Anderson wrote:
>>>> On 2/15/24 14:08, Michal Simek wrote:
>>>>>
>>>>>
>>>>> On 2/15/24 18:19, Sean Anderson wrote:
>>>>>> Currently, when we boot from JTAG we try to boot U-Boot from RAM.
>>>>>> However, this is a bit tricky to time, since the debugger has to wait
>>>>>> for SPL to initialize RAM before it can load U-Boot. This can result in
>>>>>> long waits, since occasionally initializing RAM (and other things in
>>>>>> psu_init) takes a long time to complete and the debugger must wait for
>>>>>> this worst case.
>>>>>>
>>>>>> Support semihosting if it is enabled, as it lets U-Boot tell the
>>>>>> debugger when we are ready for the image. This means we don't have to
>>>>>> wait any more than necessary. We don't change the default config to
>>>>>> ensure we don't break compatibility with existing debuggers that don't
>>>>>> expect us to hit semihosting breakpoints.
>>>>>>
>>>>>> Signed-off-by: Sean Anderson 
>>>>>> ---
>>>>>>
>>>>>>arch/arm/mach-zynqmp/spl.c | 10 --
>>>>>>1 file changed, 8 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/arm/mach-zynqmp/spl.c b/arch/arm/mach-zynqmp/spl.c
>>>>>> index a0f35f36faa..5af735aa5ce 100644
>>>>>> --- a/arch/arm/mach-zynqmp/spl.c
>>>>>> +++ b/arch/arm/mach-zynqmp/spl.c
>>>>>> @@ -9,6 +9,7 @@
>>>>>>#include 
>>>>>>#include 
>>>>>>#include 
>>>>>> +#include 
>>>>>>#include 
>>>>>>#include 
>>>>>>
>>>>>> @@ -66,6 +67,11 @@ void spl_board_init(void)
>>>>>>}
>>>>>>#endif
>>>>>>
>>>>>> +static u32 jtag_boot_device(void)
>>>>>> +{
>>>>>> +   return semihosting_enabled() ? BOOT_DEVICE_SMH : BOOT_DEVICE_RAM;
>>>>>> +}
>>>>>> +
>>>>>>void board_boot_order(u32 *spl_boot_list)
>>>>>>{
>>>>>>   spl_boot_list[0] = spl_boot_device();
>>>>>> @@ -75,7 +81,7 @@ void board_boot_order(u32 *spl_boot_list)
>>>>>>   if (spl_boot_list[0] == BOOT_DEVICE_MMC2)
>>>>>>   spl_boot_list[1] = BOOT_DEVICE_MMC1;
>>>>>>
>>>>>> -   spl_boot_list[2] = BOOT_DEVICE_RAM;
>>>>>> +   spl_boot_list[2] = jtag_boot_device();
>>>>>>}
>>>>>>
>>>>>>u32 spl_boot_device(void)
>>>>>> @@ -97,7 +103,7 @@ u32 spl_boot_device(void)
>>>>>>
>>>>>>   switch (bootmode) {
>>>>>>   case JTAG_MODE:
>>>>>> -   return BOOT_DEVICE_RAM;
>>>>>> +   return jtag_boot_device();
>>>>>>#ifdef CONFIG_SPL_MMC
>>>>>>   case SD_MODE1:
>>>>>>   case SD1_LSHFT_MODE: /* not working on silicon v1 */
>>>>>
>>>>> Good timing. Can you please tell me how to test this? What's the setup?
>>>>> Which debugger are you using?
>>>>
>>>> I am using OpenOCD with the patches at 
>>>> https://cas5-0-urlprotect.trendmicro.com:443/wis/clicktime/v1/query?url=https%3a%2f%2freview.openocd.org%2fc%2fopenocd%2f%2b%2f8133=6e1be473-0b3f-4bc4-a4f0-403592e74baf=d807158c60b7d2502abde8a2fc01f40662980862-afc15b07b0f91c910f832185958363d84f990a08
>>>>
>>>
>>> I am trying it on the top of the latest git but getting issue with event 
>>> block and no idea how to fix it.
>>>
>>> # sudo openocd -f 
>>> /usr/local/share/openocd/scripts/interface/ftdi/digilent_jtag_hs3.cfg -f 
>>> /usr/local/share/openocd/scripts/target/xilinx_zynqmp.cfg
>>> Open On-Chip Debugger 0.12.0+dev-01509-g6d288937cb2d (2024-02-16-12:22)
>>> Licensed under GNU GPL v2
>>> For bug reports, read
>>>  
>>> https://cas5-0-urlprotect.trendmicro.com:443/wis/clicktime/v1/query?url=http%3a%2f%

Re: [PATCH] arm64: zynqmp: Support semhosting boot method

[Sean Anderson]

On 2/16/24 10:06, Michal Simek wrote:
>
>
> On 2/16/24 14:48, Michal Simek wrote:
>>
>>
>> On 2/15/24 20:31, Sean Anderson wrote:
>>> On 2/15/24 14:08, Michal Simek wrote:
>>>>
>>>>
>>>> On 2/15/24 18:19, Sean Anderson wrote:
>>>>> Currently, when we boot from JTAG we try to boot U-Boot from RAM.
>>>>> However, this is a bit tricky to time, since the debugger has to wait
>>>>> for SPL to initialize RAM before it can load U-Boot. This can result in
>>>>> long waits, since occasionally initializing RAM (and other things in
>>>>> psu_init) takes a long time to complete and the debugger must wait for
>>>>> this worst case.
>>>>>
>>>>> Support semihosting if it is enabled, as it lets U-Boot tell the
>>>>> debugger when we are ready for the image. This means we don't have to
>>>>> wait any more than necessary. We don't change the default config to
>>>>> ensure we don't break compatibility with existing debuggers that don't
>>>>> expect us to hit semihosting breakpoints.
>>>>>
>>>>> Signed-off-by: Sean Anderson 
>>>>> ---
>>>>>
>>>>>arch/arm/mach-zynqmp/spl.c | 10 --
>>>>>1 file changed, 8 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm/mach-zynqmp/spl.c b/arch/arm/mach-zynqmp/spl.c
>>>>> index a0f35f36faa..5af735aa5ce 100644
>>>>> --- a/arch/arm/mach-zynqmp/spl.c
>>>>> +++ b/arch/arm/mach-zynqmp/spl.c
>>>>> @@ -9,6 +9,7 @@
>>>>>#include 
>>>>>#include 
>>>>>#include 
>>>>> +#include 
>>>>>#include 
>>>>>#include 
>>>>>
>>>>> @@ -66,6 +67,11 @@ void spl_board_init(void)
>>>>>}
>>>>>#endif
>>>>>
>>>>> +static u32 jtag_boot_device(void)
>>>>> +{
>>>>> +   return semihosting_enabled() ? BOOT_DEVICE_SMH : BOOT_DEVICE_RAM;
>>>>> +}
>>>>> +
>>>>>void board_boot_order(u32 *spl_boot_list)
>>>>>{
>>>>>   spl_boot_list[0] = spl_boot_device();
>>>>> @@ -75,7 +81,7 @@ void board_boot_order(u32 *spl_boot_list)
>>>>>   if (spl_boot_list[0] == BOOT_DEVICE_MMC2)
>>>>>   spl_boot_list[1] = BOOT_DEVICE_MMC1;
>>>>>
>>>>> -   spl_boot_list[2] = BOOT_DEVICE_RAM;
>>>>> +   spl_boot_list[2] = jtag_boot_device();
>>>>>}
>>>>>
>>>>>u32 spl_boot_device(void)
>>>>> @@ -97,7 +103,7 @@ u32 spl_boot_device(void)
>>>>>
>>>>>   switch (bootmode) {
>>>>>   case JTAG_MODE:
>>>>> -   return BOOT_DEVICE_RAM;
>>>>> +   return jtag_boot_device();
>>>>>#ifdef CONFIG_SPL_MMC
>>>>>   case SD_MODE1:
>>>>>   case SD1_LSHFT_MODE: /* not working on silicon v1 */
>>>>
>>>> Good timing. Can you please tell me how to test this? What's the setup?
>>>> Which debugger are you using?
>>>
>>> I am using OpenOCD with the patches at 
>>> https://cas5-0-urlprotect.trendmicro.com:443/wis/clicktime/v1/query?url=https%3a%2f%2freview.openocd.org%2fc%2fopenocd%2f%2b%2f8133=6e1be473-0b3f-4bc4-a4f0-403592e74baf=d807158c60b7d2502abde8a2fc01f40662980862-afc15b07b0f91c910f832185958363d84f990a08
>>>
>>
>> I am trying it on the top of the latest git but getting issue with event 
>> block and no idea how to fix it.
>>
>> # sudo openocd -f 
>> /usr/local/share/openocd/scripts/interface/ftdi/digilent_jtag_hs3.cfg -f 
>> /usr/local/share/openocd/scripts/target/xilinx_zynqmp.cfg
>> Open On-Chip Debugger 0.12.0+dev-01509-g6d288937cb2d (2024-02-16-12:22)
>> Licensed under GNU GPL v2
>> For bug reports, read
>>  
>> https://cas5-0-urlprotect.trendmicro.com:443/wis/clicktime/v1/query?url=http%3a%2f%2fopenocd.org%2fdoc%2fdoxygen%2fbugs.html=6e1be473-0b3f-4bc4-a4f0-403592e74baf=d807158c60b7d2502abde8a2fc01f40662980862-f501ab9aa5516ff666e387e53598fd624398f1bc
>> Info : auto-selecting first available session transport "jtag". To override 
>> use 'transport select '.
>> wrong # args: should be "-event  "
>>
>>
>> Do you know how to fix it?
>
&

Re: [PATCH] arm64: zynqmp: Support semhosting boot method

[Sean Anderson]

On 2/15/24 14:31, Sean Anderson wrote:
> On 2/15/24 14:08, Michal Simek wrote:
>>
>>
>> On 2/15/24 18:19, Sean Anderson wrote:
>>> Currently, when we boot from JTAG we try to boot U-Boot from RAM.
>>> However, this is a bit tricky to time, since the debugger has to wait
>>> for SPL to initialize RAM before it can load U-Boot. This can result in
>>> long waits, since occasionally initializing RAM (and other things in
>>> psu_init) takes a long time to complete and the debugger must wait for
>>> this worst case.
>>>
>>> Support semihosting if it is enabled, as it lets U-Boot tell the
>>> debugger when we are ready for the image. This means we don't have to
>>> wait any more than necessary. We don't change the default config to
>>> ensure we don't break compatibility with existing debuggers that don't
>>> expect us to hit semihosting breakpoints.
>>>
>>> Signed-off-by: Sean Anderson 
>>> ---
>>>
>>>   arch/arm/mach-zynqmp/spl.c | 10 --
>>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-zynqmp/spl.c b/arch/arm/mach-zynqmp/spl.c
>>> index a0f35f36faa..5af735aa5ce 100644
>>> --- a/arch/arm/mach-zynqmp/spl.c
>>> +++ b/arch/arm/mach-zynqmp/spl.c
>>> @@ -9,6 +9,7 @@
>>>   #include 
>>>   #include 
>>>   #include 
>>> +#include 
>>>   #include 
>>>   #include 
>>>
>>> @@ -66,6 +67,11 @@ void spl_board_init(void)
>>>   }
>>>   #endif
>>>
>>> +static u32 jtag_boot_device(void)
>>> +{
>>> +   return semihosting_enabled() ? BOOT_DEVICE_SMH : BOOT_DEVICE_RAM;
>>> +}
>>> +
>>>   void board_boot_order(u32 *spl_boot_list)
>>>   {
>>>  spl_boot_list[0] = spl_boot_device();
>>> @@ -75,7 +81,7 @@ void board_boot_order(u32 *spl_boot_list)
>>>  if (spl_boot_list[0] == BOOT_DEVICE_MMC2)
>>>  spl_boot_list[1] = BOOT_DEVICE_MMC1;
>>>
>>> -   spl_boot_list[2] = BOOT_DEVICE_RAM;
>>> +   spl_boot_list[2] = jtag_boot_device();
>>>   }
>>>
>>>   u32 spl_boot_device(void)
>>> @@ -97,7 +103,7 @@ u32 spl_boot_device(void)
>>>
>>>  switch (bootmode) {
>>>  case JTAG_MODE:
>>> -   return BOOT_DEVICE_RAM;
>>> +   return jtag_boot_device();
>>>   #ifdef CONFIG_SPL_MMC
>>>  case SD_MODE1:
>>>  case SD1_LSHFT_MODE: /* not working on silicon v1 */
>>
>> Good timing. Can you please tell me how to test this? What's the setup?
>> Which debugger are you using?
> 
> I am using OpenOCD with the patches at 
> https://cas5-0-urlprotect.trendmicro.com:443/wis/clicktime/v1/query?url=https%3a%2f%2freview.openocd.org%2fc%2fopenocd%2f%2b%2f8133=819f1021-60b3-42c6-ac85-f327c489da7c=d807158c60b7d2502abde8a2fc01f40662980862-7485d72bf875c6ddb963725debf981fe8dd33731
> 
> My boot flow is
> 
> SPL -> ATF -> U-Boot (no FSBL)
> 
> This allows me to use a FIT which saves some time since I can
> compress the bitstream. Right now I am using
> xilinx_zynqmp_virt_defconfig with the following modifications:
> 
> CONFIG_SYS_LOAD_ADDR=0x3000
> # CONFIG_SPL_OS_BOOT is not set
> CONFIG_SPL_SEMIHOSTING=y
> 
> I also have CONFIG_XILINX_PS_INIT_FILE,
> CONFIG_ZYNQMP_SPL_PM_CFG_OBJ_FILE, and CONFIG_PMUFW_INIT_FILE defined as
> appropriate. I use the following FIT for U-Boot:
> 
> /dts-v1/;
> 
> / {
> description = "U-Boot and ATF";
> #address-cells = <1>;
> 
> images {
> atf {
> description = "Arm Trusted Firmware";
> data = /incbin/("arm-trusted-firmware.bin");
> type = "firmware";
> os = "arm-trusted-firmware";
> arch = "arm64";
> compression = "none";
> load = <0xfffea000>;
> entry = <0xfffea000>;
> };
> 
> fpga {
> description = "PL Bitstream";
> data = /incbin/("system.bit.gz");
> type = "fpga";
> arch = "arm64";
> compression = "gzip";
> load = <0x7c00>;
> compatible = "u-boot,fpga-legacy";
> };
> 
> u-boot {
> description = "U-Boot";
> data = /incbin/("u-boot-nodtb.bin

Re: [PATCH] arm64: zynqmp: Support semhosting boot method

[Sean Anderson]

On 2/15/24 14:08, Michal Simek wrote:
>
>
> On 2/15/24 18:19, Sean Anderson wrote:
>> Currently, when we boot from JTAG we try to boot U-Boot from RAM.
>> However, this is a bit tricky to time, since the debugger has to wait
>> for SPL to initialize RAM before it can load U-Boot. This can result in
>> long waits, since occasionally initializing RAM (and other things in
>> psu_init) takes a long time to complete and the debugger must wait for
>> this worst case.
>>
>> Support semihosting if it is enabled, as it lets U-Boot tell the
>> debugger when we are ready for the image. This means we don't have to
>> wait any more than necessary. We don't change the default config to
>> ensure we don't break compatibility with existing debuggers that don't
>> expect us to hit semihosting breakpoints.
>>
>> Signed-off-by: Sean Anderson 
>> ---
>>
>>   arch/arm/mach-zynqmp/spl.c | 10 --
>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm/mach-zynqmp/spl.c b/arch/arm/mach-zynqmp/spl.c
>> index a0f35f36faa..5af735aa5ce 100644
>> --- a/arch/arm/mach-zynqmp/spl.c
>> +++ b/arch/arm/mach-zynqmp/spl.c
>> @@ -9,6 +9,7 @@
>>   #include 
>>   #include 
>>   #include 
>> +#include 
>>   #include 
>>   #include 
>>
>> @@ -66,6 +67,11 @@ void spl_board_init(void)
>>   }
>>   #endif
>>
>> +static u32 jtag_boot_device(void)
>> +{
>> +   return semihosting_enabled() ? BOOT_DEVICE_SMH : BOOT_DEVICE_RAM;
>> +}
>> +
>>   void board_boot_order(u32 *spl_boot_list)
>>   {
>>  spl_boot_list[0] = spl_boot_device();
>> @@ -75,7 +81,7 @@ void board_boot_order(u32 *spl_boot_list)
>>  if (spl_boot_list[0] == BOOT_DEVICE_MMC2)
>>  spl_boot_list[1] = BOOT_DEVICE_MMC1;
>>
>> -   spl_boot_list[2] = BOOT_DEVICE_RAM;
>> +   spl_boot_list[2] = jtag_boot_device();
>>   }
>>
>>   u32 spl_boot_device(void)
>> @@ -97,7 +103,7 @@ u32 spl_boot_device(void)
>>
>>  switch (bootmode) {
>>  case JTAG_MODE:
>> -   return BOOT_DEVICE_RAM;
>> +   return jtag_boot_device();
>>   #ifdef CONFIG_SPL_MMC
>>  case SD_MODE1:
>>  case SD1_LSHFT_MODE: /* not working on silicon v1 */
>
> Good timing. Can you please tell me how to test this? What's the setup?
> Which debugger are you using?

I am using OpenOCD with the patches at 
https://review.openocd.org/c/openocd/+/8133

My boot flow is

SPL -> ATF -> U-Boot (no FSBL)

This allows me to use a FIT which saves some time since I can
compress the bitstream. Right now I am using
xilinx_zynqmp_virt_defconfig with the following modifications:

CONFIG_SYS_LOAD_ADDR=0x3000
# CONFIG_SPL_OS_BOOT is not set
CONFIG_SPL_SEMIHOSTING=y

I also have CONFIG_XILINX_PS_INIT_FILE,
CONFIG_ZYNQMP_SPL_PM_CFG_OBJ_FILE, and CONFIG_PMUFW_INIT_FILE defined as
appropriate. I use the following FIT for U-Boot:

/dts-v1/;

/ {
description = "U-Boot and ATF";
#address-cells = <1>;

images {
atf {
description = "Arm Trusted Firmware";
data = /incbin/("arm-trusted-firmware.bin");
type = "firmware";
os = "arm-trusted-firmware";
arch = "arm64";
compression = "none";
load = <0xfffea000>;
entry = <0xfffea000>;
};

fpga {
description = "PL Bitstream";
data = /incbin/("system.bit.gz");
type = "fpga";
arch = "arm64";
compression = "gzip";
load = <0x7c00>;
compatible = "u-boot,fpga-legacy";
};

u-boot {
description = "U-Boot";
data = /incbin/("u-boot-nodtb.bin");
type = "firmware";
os = "u-boot";
arch = "arm64";
compression = "none";
load = <0x0008>;
entry = <0x0008>;
};

fdt {
description = "U-Boot FDT";
data = /incbin/("u-boot.dtb");
type = "flat_dt";
arch = "arm64";
compression = "none";
hash-1 {
algo = "crc32";
};
};
};

configurations {
default = "conf";
conf {
description = "Boot ATF and U-Boot";
firmware = "atf";

[PATCH] arm64: zynqmp: Support semhosting boot method

[Sean Anderson]

Currently, when we boot from JTAG we try to boot U-Boot from RAM.
However, this is a bit tricky to time, since the debugger has to wait
for SPL to initialize RAM before it can load U-Boot. This can result in
long waits, since occasionally initializing RAM (and other things in
psu_init) takes a long time to complete and the debugger must wait for
this worst case.

Support semihosting if it is enabled, as it lets U-Boot tell the
debugger when we are ready for the image. This means we don't have to
wait any more than necessary. We don't change the default config to
ensure we don't break compatibility with existing debuggers that don't
expect us to hit semihosting breakpoints.

Signed-off-by: Sean Anderson 
---

 arch/arm/mach-zynqmp/spl.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-zynqmp/spl.c b/arch/arm/mach-zynqmp/spl.c
index a0f35f36faa..5af735aa5ce 100644
--- a/arch/arm/mach-zynqmp/spl.c
+++ b/arch/arm/mach-zynqmp/spl.c
@@ -9,6 +9,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 

@@ -66,6 +67,11 @@ void spl_board_init(void)
 }
 #endif

+static u32 jtag_boot_device(void)
+{
+   return semihosting_enabled() ? BOOT_DEVICE_SMH : BOOT_DEVICE_RAM;
+}
+
 void board_boot_order(u32 *spl_boot_list)
 {
spl_boot_list[0] = spl_boot_device();
@@ -75,7 +81,7 @@ void board_boot_order(u32 *spl_boot_list)
if (spl_boot_list[0] == BOOT_DEVICE_MMC2)
spl_boot_list[1] = BOOT_DEVICE_MMC1;

-   spl_boot_list[2] = BOOT_DEVICE_RAM;
+   spl_boot_list[2] = jtag_boot_device();
 }

 u32 spl_boot_device(void)
@@ -97,7 +103,7 @@ u32 spl_boot_device(void)

switch (bootmode) {
case JTAG_MODE:
-   return BOOT_DEVICE_RAM;
+   return jtag_boot_device();
 #ifdef CONFIG_SPL_MMC
case SD_MODE1:
case SD1_LSHFT_MODE: /* not working on silicon v1 */
--
2.35.1.1320.gc452695387.dirty


[Embedded World 2024, SECO 
SpA]<https://www.messe-ticket.de/Nuernberg/embeddedworld2024/Register/ew24517689>

[PATCH] boot: Only define checksum algos when the hashes are enabled

[Sean Anderson]

Don't define checksum algos when the underlying hashes are not enabled.
This allows disabling these hashes in SPL (or U-Boot).

Fixes: d16b38f4270 ("Add support for SHA384 and SHA512")
Fixes: 646257d1f40 ("rsa: add sha256-rsa2048 algorithm")
Signed-off-by: Sean Anderson 
---

 boot/image-sig.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/boot/image-sig.c b/boot/image-sig.c
index b5692d58b24..0421a61b040 100644
--- a/boot/image-sig.c
+++ b/boot/image-sig.c
@@ -17,6 +17,7 @@ DECLARE_GLOBAL_DATA_PTR;
 #define IMAGE_MAX_HASHED_NODES 100

 struct checksum_algo checksum_algos[] = {
+#if CONFIG_IS_ENABLED(SHA1)
{
.name = "sha1",
.checksum_len = SHA1_SUM_LEN,
@@ -24,6 +25,8 @@ struct checksum_algo checksum_algos[] = {
.der_prefix = sha1_der_prefix,
.calculate = hash_calculate,
},
+#endif
+#if CONFIG_IS_ENABLED(SHA256)
{
.name = "sha256",
.checksum_len = SHA256_SUM_LEN,
@@ -31,7 +34,8 @@ struct checksum_algo checksum_algos[] = {
.der_prefix = sha256_der_prefix,
.calculate = hash_calculate,
},
-#ifdef CONFIG_SHA384
+#endif
+#if CONFIG_IS_ENABLED(SHA384)
{
.name = "sha384",
.checksum_len = SHA384_SUM_LEN,
@@ -40,7 +44,7 @@ struct checksum_algo checksum_algos[] = {
.calculate = hash_calculate,
},
 #endif
-#ifdef CONFIG_SHA512
+#if CONFIG_IS_ENABLED(SHA512)
{
.name = "sha512",
.checksum_len = SHA512_SUM_LEN,
--
2.35.1.1320.gc452695387.dirty


[Embedded World 2024, SECO 
SpA]<https://www.messe-ticket.de/Nuernberg/embeddedworld2024/Register/ew24517689>

Re: [PATCH v4] misc: fs-loader: Use fw_storage_interface instead of storage_interface

[Sean Anderson]

On 1/30/24 01:26, MD Danish Anwar wrote:

The fs-loader driver reads env storage_interface and uses it to load
firmware file into memory using the medium set by env. Update the driver
to use env fw_storage_interface as this variable is only used to load
firmwares. The env storage_interface will act as fallback so that the
existing implementations do not break.

Also update the FS Loader documentation accordingly.


So why do you want to do this? I don't see what the point of renaming the
variable is, since you are not e.g. adding any new functionality, and we
have to pay for the rename in code size.

--Sean

Re: [PATCH v3 3/3] board: Add support for Conclusive WHLE-LS1088A

[Sean Anderson]

On 11/28/23 05:34, Artur Rojek wrote:
> Introduce support for Conclusive WHLE-LS1088A Single Board Computer.
> 
> Co-developed-by: Jakub Klama 
> Signed-off-by: Jakub Klama 
> Signed-off-by: Artur Rojek 
> ---
> 
> v3: new patch
> 
>  arch/arm/Kconfig  |  19 ++
>  arch/arm/dts/Makefile |   1 +
>  arch/arm/dts/fsl-ls1088a-whle-u-boot.dtsi |   8 +
>  arch/arm/dts/fsl-ls1088a-whle.dts | 235 ++
>  board/conclusive/whle-ls1088a/Kconfig |  29 ++
>  board/conclusive/whle-ls1088a/MAINTAINERS |  11 +
>  board/conclusive/whle-ls1088a/Makefile|   7 +
>  board/conclusive/whle-ls1088a/ddr.c   | 134 
>  board/conclusive/whle-ls1088a/ddr.h   |  47 +++
>  board/conclusive/whle-ls1088a/eth.c   |  13 +
>  board/conclusive/whle-ls1088a/whle-ls1088a.c  | 301 ++
>  .../conclusive/whle-ls1088a/whle-ls1088a.env  |  13 +
>  configs/whle_ls1088a_emmc_defconfig   |  84 +
>  configs/whle_ls1088a_qspi_defconfig   |  84 +
>  include/configs/whle_ls1088a.h|  92 ++
>  15 files changed, 1078 insertions(+)
>  create mode 100644 arch/arm/dts/fsl-ls1088a-whle-u-boot.dtsi
>  create mode 100644 arch/arm/dts/fsl-ls1088a-whle.dts
>  create mode 100644 board/conclusive/whle-ls1088a/Kconfig
>  create mode 100644 board/conclusive/whle-ls1088a/MAINTAINERS
>  create mode 100644 board/conclusive/whle-ls1088a/Makefile
>  create mode 100644 board/conclusive/whle-ls1088a/ddr.c
>  create mode 100644 board/conclusive/whle-ls1088a/ddr.h
>  create mode 100644 board/conclusive/whle-ls1088a/eth.c
>  create mode 100644 board/conclusive/whle-ls1088a/whle-ls1088a.c
>  create mode 100644 board/conclusive/whle-ls1088a/whle-ls1088a.env
>  create mode 100644 configs/whle_ls1088a_emmc_defconfig
>  create mode 100644 configs/whle_ls1088a_qspi_defconfig
>  create mode 100644 include/configs/whle_ls1088a.h
> 
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 609571e6e421..cd53fcaac883 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -1869,6 +1869,24 @@ config TARGET_WHLE_LS1046A
> Layerscape Architecture processor:
> 
> https://cas5-0-urlprotect.trendmicro.com:443/wis/clicktime/v1/query?url=https%3a%2f%2fconclusive.tech%2fproducts%2fwhle%2dls1%2dsbc%2f=54a8c79f-0cf2-4f48-aefc-d35c76864d4f=d807158c60b7d2502abde8a2fc01f40662980862-cf53e7a0650a67cfac22b122ae0b3fcf9018c729
>  
> +config TARGET_WHLE_LS1088A
> + bool "Support Conclusive WHLE-LS1088A"
> + select ARCH_LS1088A
> + select ARM64
> + select ARMV8_MULTIENTRY
> + select ARCH_SUPPORT_TFABOOT
> + select BOARD_EARLY_INIT_F
> + select BOARD_LATE_INIT
> + select GPIO_EXTRA_HEADER
> + select DM_SPI_FLASH if DM_SPI
> + imply SCSI
> + help
> +   Support for Conclusive WHLE-LS1088A platform.
> +   The WHLE-LS1088A is a high-performance Single Board Computer with
> +   extensive connectivity features that supports the QorIQ LS1088A
> +   Layerscape Architecture processor:
> +   
> https://cas5-0-urlprotect.trendmicro.com:443/wis/clicktime/v1/query?url=https%3a%2f%2fconclusive.tech%2fproducts%2fwhle%2dls1%2dsbc%2f=54a8c79f-0cf2-4f48-aefc-d35c76864d4f=d807158c60b7d2502abde8a2fc01f40662980862-cf53e7a0650a67cfac22b122ae0b3fcf9018c729
> +
>  config TARGET_TEN64
>   bool "Support ten64"
>   select ARCH_LS1088A
> @@ -2318,6 +2336,7 @@ source "board/broadcom/bcmns/Kconfig"
>  source "board/broadcom/bcmns3/Kconfig"
>  source "board/cavium/thunderx/Kconfig"
>  source "board/conclusive/whle-ls1046a/Kconfig"
> +source "board/conclusive/whle-ls1088a/Kconfig"
>  source "board/eets/pdu001/Kconfig"
>  source "board/emulation/qemu-arm/Kconfig"
>  source "board/freescale/ls2080aqds/Kconfig"
> diff --git a/arch/arm/dts/Makefile b/arch/arm/dts/Makefile
> index 8dcbf29df363..b0782b4f29bc 100644
> --- a/arch/arm/dts/Makefile
> +++ b/arch/arm/dts/Makefile
> @@ -591,6 +591,7 @@ dtb-$(CONFIG_FSL_LSCH3) += fsl-ls2080a-qds.dtb \
>   fsl-ls1088a-qds.dtb \
>   fsl-ls1088a-qds-21-x.dtb \
>   fsl-ls1088a-qds-29-x.dtb \
> + fsl-ls1088a-whle.dtb \
>   fsl-ls1028a-rdb.dtb \
>   fsl-ls1028a-qds-duart.dtb \
>   fsl-ls1028a-qds-lpuart.dtb \
> diff --git a/arch/arm/dts/fsl-ls1088a-whle-u-boot.dtsi 
> b/arch/arm/dts/fsl-ls1088a-whle-u-boot.dtsi
> new file mode 100644
> index ..bbe93a1d6e4f
> --- /dev/null
> +++ b/arch/arm/dts/fsl-ls1088a-whle-u-boot.dtsi
> @@ -0,0 +1,8 @@
> +// SPDX-License-Identifier: GPL-2.0+ OR X11
> +/*
> + * Copyright 2020-2023 Conclusive Engineering Sp. z o. o.
> + */
> +
> +#include 
> +
> +#include "fsl-ls1088a-u-boot.dtsi"
> diff --git a/arch/arm/dts/fsl-ls1088a-whle.dts 
> b/arch/arm/dts/fsl-ls1088a-whle.dts
> new file mode 100644
> index ..76ef1c748059
> --- /dev/null
> +++ b/arch/arm/dts/fsl-ls1088a-whle.dts
> @@ -0,0 +1,235 @@
> +// SPDX-License-Identifier: GPL-2.0+ OR X11
> +/*
> + * 

Re: [PATCH v3 2/3] board: Add support for Conclusive WHLE-LS1046A

[Sean Anderson]

On 11/28/23 05:34, Artur Rojek wrote:
> Introduce support for Conclusive WHLE-LS1046A Single Board Computer.
> 
> Co-developed-by: Jakub Klama 
> Signed-off-by: Jakub Klama 
> Signed-off-by: Artur Rojek 
> ---
> 
> v3: no change
> 
> v2: - drop non-DM_ETH case
> - clean-up defines in configs/whle_ls1046a.h: remove unneeded ones,
>   move others to appropriate files in board directory
> - move environment variables to whle-ls1046a.env
> - move away from distro_bootcmd and use BOOTSTD 
> - fix i2c-mux node parent and ext_i2c address in Device Tree
> - style changes to eth.c
> - fix CONFIG_MTDPARTS_DEFAULT value in defconfigs
> 
>  arch/arm/Kconfig  |  19 ++
>  arch/arm/dts/Makefile |   1 +
>  arch/arm/dts/fsl-ls1046a-whle.dts | 208 +
>  board/conclusive/whle-ls1046a/Kconfig |  15 ++
>  board/conclusive/whle-ls1046a/MAINTAINERS |   9 +
>  board/conclusive/whle-ls1046a/Makefile|   7 +
>  board/conclusive/whle-ls1046a/ddr.c   |  21 ++
>  board/conclusive/whle-ls1046a/eth.c   |  65 ++
>  board/conclusive/whle-ls1046a/whle-ls1046a.c  | 215 ++
>  .../conclusive/whle-ls1046a/whle-ls1046a.env  |  13 ++
>  configs/whle_ls1046a_emmc_defconfig   |  83 +++
>  configs/whle_ls1046a_qspi_defconfig   |  84 +++
>  include/configs/whle_ls1046a.h|  47 
>  13 files changed, 787 insertions(+)
>  create mode 100644 arch/arm/dts/fsl-ls1046a-whle.dts
>  create mode 100644 board/conclusive/whle-ls1046a/Kconfig
>  create mode 100644 board/conclusive/whle-ls1046a/MAINTAINERS
>  create mode 100644 board/conclusive/whle-ls1046a/Makefile
>  create mode 100644 board/conclusive/whle-ls1046a/ddr.c
>  create mode 100644 board/conclusive/whle-ls1046a/eth.c
>  create mode 100644 board/conclusive/whle-ls1046a/whle-ls1046a.c
>  create mode 100644 board/conclusive/whle-ls1046a/whle-ls1046a.env
>  create mode 100644 configs/whle_ls1046a_emmc_defconfig
>  create mode 100644 configs/whle_ls1046a_qspi_defconfig
>  create mode 100644 include/configs/whle_ls1046a.h
> 
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index d812685c9842..609571e6e421 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -1851,6 +1851,24 @@ config TARGET_SL28
>   help
> Support for Kontron SMARC-sAL28 board.
>  
> +config TARGET_WHLE_LS1046A
> + bool "Support Conclusive WHLE-LS1046A"
> + select ARCH_LS1046A
> + select ARM64
> + select ARMV8_MULTIENTRY
> + select ARCH_SUPPORT_TFABOOT
> + select BOARD_EARLY_INIT_F
> + select BOARD_LATE_INIT
> + select GPIO_EXTRA_HEADER
> + select DM_SPI_FLASH if DM_SPI
> + imply SCSI
> + help
> +   Support for Conclusive WHLE-LS1046A platform.
> +   The WHLE-LS1046A is a high-performance Single Board Computer with
> +   extensive connectivity features that supports the QorIQ LS1046A
> +   Layerscape Architecture processor:
> +   
> https://cas5-0-urlprotect.trendmicro.com:443/wis/clicktime/v1/query?url=https%3a%2f%2fconclusive.tech%2fproducts%2fwhle%2dls1%2dsbc%2f=bfe70e04-e3cb-4832-a510-cd7686941d3c=d807158c60b7d2502abde8a2fc01f40662980862-65691d364ff53ae48e02cdbc52323838b4503b62
> +
>  config TARGET_TEN64
>   bool "Support ten64"
>   select ARCH_LS1088A
> @@ -2299,6 +2317,7 @@ source "board/cortina/presidio-asic/Kconfig"
>  source "board/broadcom/bcmns/Kconfig"
>  source "board/broadcom/bcmns3/Kconfig"
>  source "board/cavium/thunderx/Kconfig"
> +source "board/conclusive/whle-ls1046a/Kconfig"
>  source "board/eets/pdu001/Kconfig"
>  source "board/emulation/qemu-arm/Kconfig"
>  source "board/freescale/ls2080aqds/Kconfig"
> diff --git a/arch/arm/dts/Makefile b/arch/arm/dts/Makefile
> index 1be08c5fdc2e..8dcbf29df363 100644
> --- a/arch/arm/dts/Makefile
> +++ b/arch/arm/dts/Makefile
> @@ -615,6 +615,7 @@ dtb-$(CONFIG_FSL_LSCH2) += fsl-ls1043a-qds-duart.dtb \
>   fsl-ls1046a-qds-lpuart.dtb \
>   fsl-ls1046a-rdb.dtb \
>   fsl-ls1046a-frwy.dtb \
> + fsl-ls1046a-whle.dtb \
>   fsl-ls1012a-qds.dtb \
>   fsl-ls1012a-rdb.dtb \
>   fsl-ls1012a-2g5rdb.dtb \
> diff --git a/arch/arm/dts/fsl-ls1046a-whle.dts 
> b/arch/arm/dts/fsl-ls1046a-whle.dts
> new file mode 100644
> index ..1aed3e8c4701
> --- /dev/null
> +++ b/arch/arm/dts/fsl-ls1046a-whle.dts
> @@ -0,0 +1,208 @@
> +// SPDX-License-Identifier: GPL-2.0+ OR X11
> +/*
> + * Copyright 2020-2023 Conclusive Engineering Sp. z o. o.
> + */
> +
> +#include 
> +
> +/dts-v1/;
> +#include "fsl-ls1046a.dtsi"
> +
> +/ {
> + model = "Conclusive WHLE-LS1046A";
> + compatible = "conclusive,whle-ls1046a", "fsl,ls1046a";
> +
> + chosen {
> + stdout-path = 
> + };
> +
> + aliases {
> + spi0 = 
> + };
> +};
> +
> + {
> + pcie@340 {
> + status = "okay";
> + };
> +};
> +
> + {
> + status = "okay";
> +
> +

[PATCH] lib: sparse: Fix error checking for write_sparse_chunk_raw

[Sean Anderson]

The return value of write_sparse_chunk_raw is unsigned, so the existing
check has no effect. Use IS_ERR_VALUE to detect error instead, which is
what write_sparse_chunk_raw does itself.

Fixes: 62649165cb0 ("lib: sparse: Make CHUNK_TYPE_RAW buffer aligned")
Reported-by: Dan Carpenter 
Link: 
https://lore.kernel.org/u-boot/1b323ec3-59b0-490b-a2f0-fd961dafcf49@moroto.mountain/
Signed-off-by: Sean Anderson 
---

 lib/image-sparse.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/image-sparse.c b/lib/image-sparse.c
index f8289064692..09225692e9b 100644
--- a/lib/image-sparse.c
+++ b/lib/image-sparse.c
@@ -211,7 +211,7 @@ int write_sparse_image(struct sparse_storage *info,
 
blks = write_sparse_chunk_raw(info, blk, blkcnt,
  data, response);
-   if (blks < 0)
+   if (IS_ERR_VALUE(blks))
return -1;
 
blk += blks;
-- 
2.35.1.1320.gc452695387.dirty

[GIT PULL] Clock changes for v2024.04

[Sean Anderson]

The following changes since commit 6faba41927bdc8973b59678649ef83c564cc421e:

  Prepare v2024.04-rc1 (2024-01-29 20:53:19 -0500)

are available in the Git repository at:

  https://source.denx.de/u-boot/custodians/u-boot-clk.git tags/clk-2024.04-rc2

for you to fetch changes up to a8dc4965f09d28a59c156437673ddb66860c847e:

  clk: clk-gpio: add actual gated clock (2024-01-29 22:35:34 -0500)


Clock changes for v2024.04

This pull has the usual fixes and new (clock-adjacent) drivers. It also has some
cleanups for the clock API; in particular removing the unused rfree callback.

CI: https://source.denx.de/u-boot/custodians/u-boot-clk/-/pipelines/19486


Julien Masson (1):
  clk: fix clk_get_rate() always return ulong

Neil Armstrong (1):
  clk: meson: add Hardware Clock measure driver

Sean Anderson (3):
  clk: Remove rfree
  treewide: Remove clk_free
  clk: Document clk_ops return codes and behavior

Svyatoslav Ryhel (1):
  clk: clk-gpio: add actual gated clock

 arch/arm/mach-rockchip/rk3288/rk3288.c|   2 -
 arch/arm/mach-socfpga/clock_manager_agilex.c  |   2 -
 arch/arm/mach-socfpga/clock_manager_arria10.c |   7 +-
 arch/arm/mach-socfpga/clock_manager_n5x.c |   2 -
 arch/arm/mach-zynq/clk.c  |   2 -
 arch/mips/mach-pic32/cpu.c|   7 +-
 arch/sandbox/include/asm/clk.h|   8 --
 board/microchip/pic32mzda/pic32mzda.c |   2 -
 board/sipeed/maix/maix.c  |   1 -
 board/synopsys/hsdk/clk-lib.c |   2 -
 drivers/clk/aspeed/clk_ast2600.c  |   2 -
 drivers/clk/at91/compat.c |  14 +-
 drivers/clk/clk-gpio.c|  38 +-
 drivers/clk/clk-uclass.c  |  47 +--
 drivers/clk/clk-xlnx-clock-wizard.c   |   1 -
 drivers/clk/clk_sandbox.c |  12 --
 drivers/clk/clk_sandbox_test.c|  12 --
 drivers/clk/clk_versaclock.c  |  12 +-
 drivers/clk/clk_zynq.c|   2 -
 drivers/clk/clk_zynqmp.c  |   2 -
 drivers/clk/imx/clk-imx8.c|   2 -
 drivers/clk/meson/Kconfig |  10 ++
 drivers/clk/meson/Makefile|   1 +
 drivers/clk/meson/clk-measure.c   | 634 
++
 drivers/clk/mvebu/armada-37xx-periph.c|   2 -
 drivers/cpu/riscv_cpu.c   |   2 -
 drivers/dma/bcm6348-iudma.c   |   2 -
 drivers/gpio/at91_gpio.c  |   2 -
 drivers/gpio/atmel_pio4.c |   2 -
 drivers/gpio/gpio-rcar.c  |   1 -
 drivers/hwspinlock/stm32_hwspinlock.c |   6 +-
 drivers/i2c/at91_i2c.c|   2 -
 drivers/i2c/designware_i2c.c  |   2 -
 drivers/i2c/i2c-microchip.c   |   2 -
 drivers/i2c/npcm_i2c.c|   1 -
 drivers/i2c/ocores_i2c.c  |   2 -
 drivers/i2c/stm32f7_i2c.c |   4 +-
 drivers/mailbox/stm32-ipcc.c  |   7 +-
 drivers/misc/ls2_sfp.c|   1 -
 drivers/mmc/arm_pl180_mmci.c  |   1 -
 drivers/mmc/aspeed_sdhci.c|   4 +-
 drivers/mmc/atmel_sdhci.c |   2 -
 drivers/mmc/gen_atmel_mci.c   |  19 +--
 drivers/mmc/msm_sdhci.c   |   1 -
 drivers/mmc/pic32_sdhci.c |   1 -
 drivers/mmc/renesas-sdhi.c|  21 +--
 drivers/mmc/snps_dw_mmc.c |   8 +-
 drivers/mmc/socfpga_dw_mmc.c  |   1 -
 drivers/mmc/stm32_sdmmc2.c|   4 +-
 drivers/mmc/uniphier-sd.c |   1 -
 drivers/mtd/nand/raw/atmel/nand-controller.c  |   4 +-
 drivers/mtd/renesas_rpc_hf.c  |   1 -
 drivers/net/bcm6348-eth.c |   2 -
 drivers/net/bcm6368-eth.c |   2 -
 drivers/net/designware.c  |   1 -
 drivers/net/dwc_eth_qos.c |  43 +-
 drivers/net/dwc_eth_qos_imx.c |  21 +--
 drivers/net/dwc_eth_qos_qcom.c|   1 -
 drivers/net/dwc_eth_qos_rockchip.c|   6 +-
 drivers/net/sni_ave.c |   5 +-
 drivers/net/ti/am65-cpsw-nuss.c   |   1 -
 drivers/phy/bcm6318-usbh-phy.c|   2 -
 drivers/phy/bcm6348-usbh-phy.c|   2 -
 drivers/phy/bcm6368-usbh-phy.c|   4 -
 drivers/phy/meson-axg-mipi-dphy.c |   1 -
 drivers/phy/meson-g12a-usb3-pcie.c|   1 -
 drivers/phy/meson-gxl-usb2.c  |   1 -
 drivers/phy/phy-rcar-gen2.c   |   1

Re: [PATCH 0/3] clk: Remove clk_free and better document clk_ops

[Sean Anderson]

On Sat, 16 Dec 2023 14:38:40 -0500, Sean Anderson wrote:
> This series contains two unrelated changes. They are included together because
> they touch the same files and would otherwise conflict. The first change is to
> completely remove clk_free. This is because the op it calls (rfree) is
> completely unused. I believe the original intent of this op was to allow clock
> drivers to free resources allocated in request. However, no clock drivers do
> this, so we can remove the function and all its callers.
> 
> [...]

Applied, thanks!

[1/3] clk: Remove rfree
  https://source.denx.de/u-boot/custodians/u-boot-clk/-/commit/82719d3f409f
[2/3] treewide: Remove clk_free
  https://source.denx.de/u-boot/custodians/u-boot-clk/-/commit/c9309f40a683
[3/3] clk: Document clk_ops return codes and behavior
  https://source.denx.de/u-boot/custodians/u-boot-clk/-/commit/0bfbb830554f

Best regards,
-- 
Sean Anderson 

Re: [PATCH v2] clk: fix clk_get_rate() always return ulong

[Sean Anderson]

On Fri, 15 Dec 2023 15:09:43 +0100, Julien Masson wrote:
> When we call clk_get_rate(), we expect to get clock rate value as
> ulong.
> In that case we should not use log_ret() macro since it use internally
> an int.
> Otherwise we may return an invalid/truncated clock rate value.
> 
> 
> [...]

Applied, thanks!

[1/1] clk: fix clk_get_rate() always return ulong
  https://source.denx.de/u-boot/custodians/u-boot-clk/-/commit/b500447ad6ae

Best regards,
-- 
Sean Anderson 

Re: [PATCH v3] clk: meson: add Hardware Clock measure driver

[Sean Anderson]

On Mon, 18 Dec 2023 10:47:55 +0100, Neil Armstrong wrote:
> Amlogic SoCs embeds an hardware clock measure block, port it
> from Linux and implement it as a UCLK_CLK with only the dump
> op and fail-only xlate.
> 
> Based on the Linux driver introduced in [1].
> 
> [1] commit 2b45ebef39a2 ("soc: amlogic: Add Meson Clock Measure driver").
> 
> [...]

Applied, thanks!

[1/1] clk: meson: add Hardware Clock measure driver
  https://source.denx.de/u-boot/custodians/u-boot-clk/-/commit/2da1331d20b3

Best regards,
-- 
Sean Anderson 

Re: [PATCH v3 0/1] Fix clk-gpio driver

[Sean Anderson]

On Wed, 10 Jan 2024 18:09:55 +0200, Svyatoslav Ryhel wrote:
> Existing gpio-gate-clock driver acts like a simple GPIO switch without any
> effect on gated clock. Add actual clock actions into enable/disable ops and
> implement get_rate op by passing gated clock if it is enabled.
> 
> Testing current driver implementation shows that it is not fully capable
> and lacks gated clock manipulations.
> 
> [...]

Applied, thanks!

[1/1] clk: clk-gpio: add actual gated clock
  https://source.denx.de/u-boot/custodians/u-boot-clk/-/commit/a8dc4965f09d

Best regards,
-- 
Sean Anderson 

Re: [PATCH v3 1/1] clk: clk-gpio: add actual gated clock

[Sean Anderson]

On 1/10/24 11:09, Svyatoslav Ryhel wrote:

Existing gpio-gate-clock driver acts like a simple GPIO switch without any
effect on gated clock. Add actual clock actions into enable/disable ops and
implement get_rate op by passing gated clock if it is enabled.

Signed-off-by: Svyatoslav Ryhel 
---
  drivers/clk/clk-gpio.c | 40 ++--
  1 file changed, 34 insertions(+), 6 deletions(-)

diff --git a/drivers/clk/clk-gpio.c b/drivers/clk/clk-gpio.c
index 26d795b978..d2dd8070fa 100644
--- a/drivers/clk/clk-gpio.c
+++ b/drivers/clk/clk-gpio.c
@@ -3,19 +3,23 @@
   * Copyright (C) 2023 Marek Vasut 
   */
  
-#include 

-#include 
-#include 
+#include 
  #include 
+#include 


this is out of order, but I will fix it when applying


+#include 
+
+#include 
  
  struct clk_gpio_priv {

-   struct gpio_descenable;
+   struct gpio_descenable; /* GPIO, controlling the gate */
+   struct clk  *clk;   /* Gated clock */
  };
  
  static int clk_gpio_enable(struct clk *clk)

  {
struct clk_gpio_priv *priv = dev_get_priv(clk->dev);
  
+	clk_enable(priv->clk);

dm_gpio_set_value(>enable, 1);
  
  	return 0;

@@ -26,21 +30,45 @@ static int clk_gpio_disable(struct clk *clk)
struct clk_gpio_priv *priv = dev_get_priv(clk->dev);
  
  	dm_gpio_set_value(>enable, 0);

+   clk_disable(priv->clk);
  
  	return 0;

  }
  
+static ulong clk_gpio_get_rate(struct clk *clk)

+{
+   struct clk_gpio_priv *priv = dev_get_priv(clk->dev);
+
+   return clk_get_rate(priv->clk);
+}
+
  const struct clk_ops clk_gpio_ops = {
.enable = clk_gpio_enable,
.disable= clk_gpio_disable,
+   .get_rate   = clk_gpio_get_rate,
  };
  
  static int clk_gpio_probe(struct udevice *dev)

  {
struct clk_gpio_priv *priv = dev_get_priv(dev);
+   int ret;
  
-	return gpio_request_by_name(dev, "enable-gpios", 0,

-   >enable, GPIOD_IS_OUT);
+   priv->clk = devm_clk_get(dev, NULL);
+   if (IS_ERR(priv->clk)) {
+   log_debug("%s: Could not get gated clock: %ld\n",
+ __func__, PTR_ERR(priv->clk));
+   return PTR_ERR(priv->clk);
+   }
+
+   ret = gpio_request_by_name(dev, "enable-gpios", 0,
+  >enable, GPIOD_IS_OUT);
+   if (ret) {
+   log_debug("%s: Could not decode enable-gpios (%d)\n",
+ __func__, ret);
+   return ret;
+   }
+
+   return 0;
  }
  
  /*


Reviewed-by: Sean Anderson 

Re: [PATCH] net: phy: realtek: Add support for LED configuration

[Sean Anderson]

Hi Artur/Jakub,

On 11/30/23 15:39, Artur Rojek wrote:
> From: Jakub Klama 
> 
> Introduce an ability to configure LED and Fiber LEDs found in RTL8211F
> PHYs. This is achieved through two optional Device Tree properties:
> * rtl,lcr  for LED control
> * rtl,flcr for Fiber LED control

The Linux netdev maintainers do not like this sort of LED binding (see
e.g. [1] for reasoning). They would prefer that PHY drivers use an
LED-subsystem-style binding for LEDS [2]. So I can't see this sort of
binding being accepted in Linux.

Unfortunately, U-Boot's LED support is a much more primitive than
Linux's. In particular, there is no comprehensive trigger system like in
Linux. So this would be a lot more work than your initial patch.

In Linux, LEDs are configured by userspace, but in U-Boot there really
isn't a userspace to do this. So I think maybe we need a u-boot,trigger
(like linux,default-trigger) to determine the blink mode. This would
need to be more expressive than Linux's.

Ideally there would be a way for LED triggers to hook into the net
send/receive functions. But I think it is probably fine for any initial
attempt to just parse a Linux-style binding and configure hardware
control.

Alternatively, you can just write these registers in board code, which
is not ideal but which is what everyone does anyway...

--Sean

[1] https://lpc.events/event/17/contributions/1604/
[2] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/net/ethernet-phy.yaml#n206

> Signed-off-by: Jakub Klama 
> Signed-off-by: Artur Rojek 
> ---
>  drivers/net/phy/realtek.c | 18 --
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> index 396cac76d6..d078f41bee 100644
> --- a/drivers/net/phy/realtek.c
> +++ b/drivers/net/phy/realtek.c
> @@ -59,6 +59,7 @@
>  #define MIIM_RTL8211F_TX_DELAY   0x100
>  #define MIIM_RTL8211F_RX_DELAY   0x8
>  #define MIIM_RTL8211F_LCR0x10
> +#define MIIM_RTL8211F_FLCR   0x12
>  
>  #define RTL8201F_RMSR0x10
>  
> @@ -220,6 +221,8 @@ default_delay:
>  
>  static int rtl8211f_config(struct phy_device *phydev)
>  {
> + ofnode node = phy_get_ofnode(phydev);
> + u32 lcr, flcr;
>   u16 reg;
>  
>   if (phydev->flags & PHY_RTL8211F_FORCE_EEE_RXC_ON) {
> @@ -254,14 +257,17 @@ static int rtl8211f_config(struct phy_device *phydev)
>   reg &= ~MIIM_RTL8211F_RX_DELAY;
>   phy_write(phydev, MDIO_DEVAD_NONE, 0x15, reg);
>  
> - /* restore to default page 0 */
> - phy_write(phydev, MDIO_DEVAD_NONE,
> -   MIIM_RTL8211F_PAGE_SELECT, 0x0);
> -
> - /* Set green LED for Link, yellow LED for Active */
>   phy_write(phydev, MDIO_DEVAD_NONE,
> MIIM_RTL8211F_PAGE_SELECT, 0xd04);
> - phy_write(phydev, MDIO_DEVAD_NONE, 0x10, 0x617f);
> +
> + if (ofnode_valid(node) && !ofnode_read_u32(node, "rtl,lcr", ))
> + phy_write(phydev, MDIO_DEVAD_NONE, MIIM_RTL8211F_LCR, lcr);
> + else /* Set green LED for Link, yellow LED for Active */
> + phy_write(phydev, MDIO_DEVAD_NONE, MIIM_RTL8211F_LCR, 0x617f);
> +
> + if (ofnode_valid(node) && !ofnode_read_u32(node, "rtl,flcr", ))
> + phy_write(phydev, MDIO_DEVAD_NONE, MIIM_RTL8211F_FLCR, flcr);
> +
>   phy_write(phydev, MDIO_DEVAD_NONE,
> MIIM_RTL8211F_PAGE_SELECT, 0x0);
>  

Re: [PATCH] mtd: nand: Print warning for unsupported ecc modes

[Sean Anderson]

On 1/16/24 23:08, Venkatesh Yadav Abbarapu wrote:

Currently only hw ecc is supported in U-Boot. If any other ecc mode is
given in DT, it simply ignores and switches to hw ecc. So better print
what is being done.


This is not true? Sandbox for example uses soft ECC (hamming and BCH).

--Sean


Revert this patch once soft ecc support is fixed in future.

Signed-off-by: Ashok Reddy Soma 
Signed-off-by: Venkatesh Yadav Abbarapu 
---
  drivers/mtd/nand/raw/nand_base.c | 3 +++
  1 file changed, 3 insertions(+)

diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index c40a0f23d7..200f7f8ba0 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -4463,6 +4463,9 @@ static int nand_dt_init(struct mtd_info *mtd, struct 
nand_chip *chip, ofnode nod
  
  	str = ofnode_read_string(node, "nand-ecc-mode");

if (str) {
+   if (strcmp(str, "hw"))
+   printf("%s ecc is not supported, switching to hw 
ecc\n", str);
+
if (!strcmp(str, "none"))
ecc_mode = NAND_ECC_NONE;
else if (!strcmp(str, "soft"))

Re: [PATCH v2 1/2] part: Add a function to find ESP partition

[Sean Anderson]

On 1/16/24 07:36, Mayuresh Chitale wrote:

If a disk has an EFI system partition (ESP) then it can be used to
locate the boot files. Add a function to find the ESP.

Signed-off-by: Mayuresh Chitale 
Reviewed-by: Heinrich Schuchardt 
---
  disk/part.c| 16 
  include/part.h | 11 +++
  2 files changed, 27 insertions(+)

diff --git a/disk/part.c b/disk/part.c
index 36b88205eca..6b1fbc18637 100644
--- a/disk/part.c
+++ b/disk/part.c
@@ -848,3 +848,19 @@ int part_get_bootable(struct blk_desc *desc)
  
  	return 0;

  }
+
+int part_get_esp(struct blk_desc *desc)
+{
+   struct disk_partition info;
+   int p;
+
+   for (p = 1; p <= MAX_SEARCH_PARTITIONS; p++) {
+   int ret;
+
+   ret = part_get_info(desc, p, );
+   if (!ret && (info.bootable & PART_EFI_SYSTEM_PARTITION))
+   return p;
+   }
+
+   return 0;
+}
diff --git a/include/part.h b/include/part.h
index db34bc6bb7d..30e049c8f19 100644
--- a/include/part.h
+++ b/include/part.h
@@ -690,6 +690,14 @@ int part_get_type_by_name(const char *name);
   */
  int part_get_bootable(struct blk_desc *desc);
  
+/**

+ * part_get_esp() - Find the EFI system partition
+ *
+ * @desc: Block-device descriptor
+ * @Return the EFI system partition, or 0 if there is none
+ */
+int part_get_esp(struct blk_desc *desc);
+
  #else
  static inline int part_driver_get_count(void)
  { return 0; }
@@ -700,6 +708,9 @@ static inline struct part_driver 
*part_driver_get_first(void)
  static inline bool part_get_bootable(struct blk_desc *desc)
  { return false; }
  
+static inline bool part_get_esp(struct blk_desc *desc)


Please use the same signature as above.


+{ return false; }


The statement here should be on its own line.

--Sean


+
  #endif /* CONFIG_PARTITIONS */
  
  #endif /* _PART_H */

Re: [PATCH v1] fastboot: introduce 'oem board' subcommand

[Sean Anderson]

On 1/10/24 03:03, Alexey Romanov wrote:
> Hi,
> 
> On Tue, Jan 09, 2024 at 10:45:46AM -0500, Sean Anderson wrote:
>> On 1/9/24 05:27, Alexey Romanov wrote:
>> > Hello Sean!
>> > 
>> > Thanks for you reply.
>> > 
>> > On Thu, Dec 28, 2023 at 11:45:04AM -0500, Sean Anderson wrote:
>> >> On 12/28/23 10:25, Alexey Romanov wrote:
>> >> > Currently, fastboot protocol in U-Boot has no opportunity
>> >> > to execute vendor custom code with verifed boot.
>> >> 
>> >> Well, I would say the most conventional way to do this would be something 
>> >> like
>> >> 
>> >> => fastboot 0
>> >> => source \# CONFIG_FASTBOOT_BUF_ADDR
>> >> 
>> >> and on your host machine,
>> >> 
>> >> $ fastboot stage my_script.itb
>> >> 
>> >> where my_script.its looks like
>> >> 
>> >> /dts-v1/;
>> >> 
>> >> / {
>> >> description = "my script";
>> >> #address-cells = <1>;
>> >> 
>> >> images {
>> >> my-script {
>> >> data = /incbin/("my_script.scr");
>> >> type = "script";
>> >> arch = "arm64";
>> >> compression = "none";
>> >> hash-1 {
>> >> algo = "sha256";
>> >> };
>> >> };
>> >> };
>> >> 
>> >> configurations {
>> >> default = "conf";
>> >> conf {
>> >> description = "Load my script";
>> >> script = "my-script";
>> >> signature {
>> >> algo = "sha256,rsa2048";
>> >> key-name-hint = "vboot";
>> >> sign-images = "script";
>> >> };
>> >> };
>> >> };
>> >> };
>> >> 
>> >> This method is especially useful to pass complex parameters to your 
>> >> command.
>> >> This method of course requires commit bcc85b96b5f ("cmd: source: Support
>> >> specifying config name").
>> >> 
>> >> Would it be possible to use the above method for your use case?
>> > 
>> > This method sounds good for some cases, but we have encountered some
>> > problems that prevent us from applying it:
>> > 
>> > 1. Looks like this method requires access to UART (for typing 'source'
>> > command). Open the UART is unsafe at devices with verified boot.
>> 
>> Generally the idea is that you run source after you run fastboot. E.g. you 
>> set
>> your altbootcmd (or whatever) to something like
>> 
>> while true; fastboot 0; source \# || boot; done
>> 
>> so you try to source whatever gets staged, and boot it otherwise.
> 
> If I understand right, U-Boot always will try fastboot mode?

Well, you do this however you would enable fastboot normally. So usually
you run fastboot if you main boot fails or if the user holds down a
button or something.  Same thing here.

> Yes, it seems like it will work. But the code for complex
> fastboot scripts will be complex and difficult to read.

If you write a (regular) command to call meson_bootloader_write_bl2 you
can do it in a script with imxtract. Although the latter command does
not check signatures. I have a patch for that, but I still need to write
tests for it.

> I think my version looks more elegant and simple.
> 
>> 
>> > 2. We use automation scripts to flash our devices using fastboot
>> > protocol. A typical example of flashing scripts (device is already in
>> > fastboot mode):
>> > 
>> >   $ fastboot erase bootloader
>> >   $ fastboot stage bootloader.img
>> >   $ fastboot oem board:write_bootloader
>> > 
>> >   $ fastboot reboot-bootloader
>> > 
>> >   $ fastboot erase super
>> >   $ fastboot flash super super.bin
>> > 
>> >   $ fastboot reboot
>> > 
>> > This method doesn't assume what something typing additional command in
>> > U-Boot shell, even if we have access to UART.
>> 
>> I'm curious what you actual usage for this is? That is, what do you need
>> a custom command for.
> 
> Our SoC vendor use custom scheme for flashing bootloader partitio

Re: [PATCH v2 1/1] clk: clk-gpio: add actual gated clock

[Sean Anderson]

On 1/10/24 10:53, Svyatoslav wrote:



10 січня 2024 р. 17:45:57 GMT+02:00, Sean Anderson  
написав(-ла):

On 12/16/23 10:37, Sean Anderson wrote:

On 12/16/23 03:48, Svyatoslav Ryhel wrote:

Existing gpio-gate-clock driver acts like a simple GPIO switch without any
effect on gated clock. Add actual clock actions into enable/disable ops and
implement get_rate op by passing gated clock if it is enabled.

Signed-off-by: Svyatoslav Ryhel 
---
   drivers/clk/clk-gpio.c | 44 ++
   1 file changed, 36 insertions(+), 8 deletions(-)

diff --git a/drivers/clk/clk-gpio.c b/drivers/clk/clk-gpio.c
index 26d795b978..72d9747a47 100644
--- a/drivers/clk/clk-gpio.c
+++ b/drivers/clk/clk-gpio.c
@@ -3,19 +3,23 @@
    * Copyright (C) 2023 Marek Vasut 
    */
-#include 
-#include 
-#include 
+#include 
   #include 
+#include 
+#include 
+
+#include 
   struct clk_gpio_priv {
-    struct gpio_desc    enable;
+    struct gpio_desc    enable;    /* GPIO, controlling the gate */
+    struct clk    *clk;    /* Gated clock */
   };
   static int clk_gpio_enable(struct clk *clk)
   {
   struct clk_gpio_priv *priv = dev_get_priv(clk->dev);
+    clk_enable(priv->clk);
   dm_gpio_set_value(>enable, 1);
   return 0;
@@ -26,21 +30,45 @@ static int clk_gpio_disable(struct clk *clk)
   struct clk_gpio_priv *priv = dev_get_priv(clk->dev);
   dm_gpio_set_value(>enable, 0);
+    clk_disable(priv->clk);
   return 0;
   }
+static ulong clk_gpio_get_rate(struct clk *clk)
+{
+    struct clk_gpio_priv *priv = dev_get_priv(clk->dev);
+
+    return clk_get_rate(priv->clk);
+}
+
   const struct clk_ops clk_gpio_ops = {
   .enable    = clk_gpio_enable,
   .disable    = clk_gpio_disable,
+    .get_rate    = clk_gpio_get_rate,
   };
-static int clk_gpio_probe(struct udevice *dev)
+static int clk_gpio_of_to_plat(struct udevice *dev)


Same comment as the first time:

Why the conversion from probe to of_to_plat?



Same answer as the first time.


Last time you said



-static int clk_gpio_probe(struct udevice *dev)
+static int clk_gpio_of_to_plat(struct udevice *dev)


This change should be a separate commit.


This actually should not be accepted first place

.of_to_plat - convert device tree data to plat
.probe - make a device ready for use

https://github.com/u-boot/u-boot/blob/master/doc/develop/driver-model/design.rst



and I thought "This actually should not be accepted first place" was referring 
to
the conversion (i.e. that you would be removing it next time).

This sort of conversion should be mentioned in the commit message.

And the weird thing to me is that I would expect of_to_plat to fill in a 
platdata
struct. If you are not doing that, why use this function?


You propose to spam commits for this small adjustment?


Yes. One change per commit.

--Sean

Re: [PATCH v2 1/1] clk: clk-gpio: add actual gated clock

[Sean Anderson]

On 12/16/23 10:37, Sean Anderson wrote:

On 12/16/23 03:48, Svyatoslav Ryhel wrote:

Existing gpio-gate-clock driver acts like a simple GPIO switch without any
effect on gated clock. Add actual clock actions into enable/disable ops and
implement get_rate op by passing gated clock if it is enabled.

Signed-off-by: Svyatoslav Ryhel 
---
  drivers/clk/clk-gpio.c | 44 ++
  1 file changed, 36 insertions(+), 8 deletions(-)

diff --git a/drivers/clk/clk-gpio.c b/drivers/clk/clk-gpio.c
index 26d795b978..72d9747a47 100644
--- a/drivers/clk/clk-gpio.c
+++ b/drivers/clk/clk-gpio.c
@@ -3,19 +3,23 @@
   * Copyright (C) 2023 Marek Vasut 
   */
-#include 
-#include 
-#include 
+#include 
  #include 
+#include 
+#include 
+
+#include 
  struct clk_gpio_priv {
-    struct gpio_desc    enable;
+    struct gpio_desc    enable;    /* GPIO, controlling the gate */
+    struct clk    *clk;    /* Gated clock */
  };
  static int clk_gpio_enable(struct clk *clk)
  {
  struct clk_gpio_priv *priv = dev_get_priv(clk->dev);
+    clk_enable(priv->clk);
  dm_gpio_set_value(>enable, 1);
  return 0;
@@ -26,21 +30,45 @@ static int clk_gpio_disable(struct clk *clk)
  struct clk_gpio_priv *priv = dev_get_priv(clk->dev);
  dm_gpio_set_value(>enable, 0);
+    clk_disable(priv->clk);
  return 0;
  }
+static ulong clk_gpio_get_rate(struct clk *clk)
+{
+    struct clk_gpio_priv *priv = dev_get_priv(clk->dev);
+
+    return clk_get_rate(priv->clk);
+}
+
  const struct clk_ops clk_gpio_ops = {
  .enable    = clk_gpio_enable,
  .disable    = clk_gpio_disable,
+    .get_rate    = clk_gpio_get_rate,
  };
-static int clk_gpio_probe(struct udevice *dev)
+static int clk_gpio_of_to_plat(struct udevice *dev)


Same comment as the first time:

Why the conversion from probe to of_to_plat?

--Sean


  {
  struct clk_gpio_priv *priv = dev_get_priv(dev);
+    int ret;
-    return gpio_request_by_name(dev, "enable-gpios", 0,
-    >enable, GPIOD_IS_OUT);
+    priv->clk = devm_clk_get(dev, NULL);
+    if (IS_ERR(priv->clk)) {
+    log_debug("%s: Could not get gated clock: %ld\n",
+  __func__, PTR_ERR(priv->clk));
+    return PTR_ERR(priv->clk);
+    }
+
+    ret = gpio_request_by_name(dev, "enable-gpios", 0,
+   >enable, GPIOD_IS_OUT);
+    if (ret) {
+    log_debug("%s: Could not decode enable-gpios (%d)\n",
+  __func__, ret);
+    return ret;
+    }
+
+    return 0;
  }
  /*
@@ -59,7 +87,7 @@ U_BOOT_DRIVER(gpio_gate_clock) = {
  .name    = "gpio_clock",
  .id    = UCLASS_CLK,
  .of_match    = clk_gpio_match,
-    .probe    = clk_gpio_probe,
+    .of_to_plat    = clk_gpio_of_to_plat,
  .priv_auto    = sizeof(struct clk_gpio_priv),
  .ops    = _gpio_ops,
  .flags    = DM_FLAG_PRE_RELOC,


+CC Marek

Re: [PATCH v3] clk: meson: add Hardware Clock measure driver

[Sean Anderson]

On 1/10/24 04:23, Neil Armstrong wrote:

Hi Sean,

On 18/12/2023 10:47, Neil Armstrong via groups.io wrote:

Amlogic SoCs embeds an hardware clock measure block, port it
from Linux and implement it as a UCLK_CLK with only the dump
op and fail-only xlate.

Based on the Linux driver introduced in [1].

[1] commit 2b45ebef39a2 ("soc: amlogic: Add Meson Clock Measure driver").

Reviewed-by: Sean Anderson 
Signed-off-by: Neil Armstrong 


Should I take this via the amlogic tree ?


If you like.

I will be doing a PR sometime before the merge window ends.

--Sean

Re: [PATCH v1] fastboot: introduce 'oem board' subcommand

[Sean Anderson]

On 1/9/24 05:27, Alexey Romanov wrote:
> Hello Sean!
> 
> Thanks for you reply.
> 
> On Thu, Dec 28, 2023 at 11:45:04AM -0500, Sean Anderson wrote:
>> On 12/28/23 10:25, Alexey Romanov wrote:
>> > Currently, fastboot protocol in U-Boot has no opportunity
>> > to execute vendor custom code with verifed boot.
>> 
>> Well, I would say the most conventional way to do this would be something 
>> like
>> 
>> => fastboot 0
>> => source \# CONFIG_FASTBOOT_BUF_ADDR
>> 
>> and on your host machine,
>> 
>> $ fastboot stage my_script.itb
>> 
>> where my_script.its looks like
>> 
>> /dts-v1/;
>> 
>> / {
>> description = "my script";
>> #address-cells = <1>;
>> 
>> images {
>> my-script {
>> data = /incbin/("my_script.scr");
>> type = "script";
>> arch = "arm64";
>> compression = "none";
>> hash-1 {
>> algo = "sha256";
>> };
>> };
>> };
>> 
>> configurations {
>> default = "conf";
>> conf {
>> description = "Load my script";
>> script = "my-script";
>> signature {
>> algo = "sha256,rsa2048";
>> key-name-hint = "vboot";
>> sign-images = "script";
>> };
>> };
>> };
>> };
>> 
>> This method is especially useful to pass complex parameters to your command.
>> This method of course requires commit bcc85b96b5f ("cmd: source: Support
>> specifying config name").
>> 
>> Would it be possible to use the above method for your use case?
> 
> This method sounds good for some cases, but we have encountered some
> problems that prevent us from applying it:
> 
> 1. Looks like this method requires access to UART (for typing 'source'
> command). Open the UART is unsafe at devices with verified boot.

Generally the idea is that you run source after you run fastboot. E.g. you set
your altbootcmd (or whatever) to something like

while true; fastboot 0; source \# || boot; done

so you try to source whatever gets staged, and boot it otherwise.

> 2. We use automation scripts to flash our devices using fastboot
> protocol. A typical example of flashing scripts (device is already in
> fastboot mode):
> 
>   $ fastboot erase bootloader
>   $ fastboot stage bootloader.img
>   $ fastboot oem board:write_bootloader
> 
>   $ fastboot reboot-bootloader
> 
>   $ fastboot erase super
>   $ fastboot flash super super.bin
> 
>   $ fastboot reboot
> 
> This method doesn't assume what something typing additional command in
> U-Boot shell, even if we have access to UART.

I'm curious what you actual usage for this is? That is, what do you need
a custom command for.

--Sean

>> 
>> --Sean
>> 
>> > This patch
>> > introduce new fastboot subcommand fastboot oem board:,
>> > which allow to run custom oem_board function.
>> > =
>> > Default implementation is __weak. Vendor must redefine it in
>> > board/ folder with his own logic.
>> > 
>> > For example, some vendors have their custom nand/emmc partition
>> > flashing or erasing. Here some typical command for such use cases:
>> > 
>> > - flashing:
>> > 
>> >   $ fastboot stage bootloader.img
>> >   $ fastboot oem board:write_bootloader
>> > 
>> > - erasing:
>> > 
>> >   $ fastboot oem board:erase_env
>> > 
>> > Signed-off-by: Alexey Romanov 
>> > ---
>> >  drivers/fastboot/Kconfig  |  7 +++
>> >  drivers/fastboot/fb_command.c | 15 +++
>> >  include/fastboot.h|  1 +
>> >  3 files changed, 23 insertions(+)
>> > 
>> > diff --git a/drivers/fastboot/Kconfig b/drivers/fastboot/Kconfig
>> > index 3cfeea4837..4c955cabab 100644
>> > --- a/drivers/fastboot/Kconfig
>> > +++ b/drivers/fastboot/Kconfig
>> > @@ -241,6 +241,13 @@ config FASTBOOT_OEM_RUN
>> >  this feature if you are using verified boot, as it will allow an
>> >  attacker to bypass any restrictions you have in place.
>> >  
>> > +config FASTBOOT_OEM_BOARD
>> > +  bool "Enable the 'oem board' command"
>> > +  help
>> > +This extends the fastboot protocol with an "oem

Re: Fwd: New Defects reported by Coverity Scan for Das U-Boot

[Sean Anderson]

Comments on NAND stuff only.

On 1/8/24 12:45, Tom Rini wrote:


*** CID 477216:(BAD_SHIFT)
/drivers/mtd/nand/raw/nand_base.c: 3921 in nand_flash_detect_onfi()
3915
3916/*
3917 * pages_per_block and blocks_per_lun may not be a
power-of-2 size
3918 * (don't ask me who thought of this...). MTD assumes that these
3919 * dimensions will be power-of-2, so just truncate the
remaining area.
3920 */

 CID 477216:(BAD_SHIFT)
 In expression "1 << generic_fls((__u32)(__le32)p->pages_per_block) - 1", shifting by a 
negative amount has undefined behavior.  The shift amount, 
"generic_fls((__u32)(__le32)p->pages_per_block) - 1", is -1.

3921mtd->erasesize = 1 <<
(fls(le32_to_cpu(p->pages_per_block)) - 1);
3922mtd->erasesize *= mtd->writesize;
3923
3924mtd->oobsize = le16_to_cpu(p->spare_bytes_per_page);
3925
3926/* See erasesize comment */
/drivers/mtd/nand/raw/nand_base.c: 3927 in nand_flash_detect_onfi()
3921mtd->erasesize = 1 <<
(fls(le32_to_cpu(p->pages_per_block)) - 1);
3922mtd->erasesize *= mtd->writesize;
3923
3924mtd->oobsize = le16_to_cpu(p->spare_bytes_per_page);
3925
3926/* See erasesize comment */

 CID 477216:(BAD_SHIFT)
 In expression "1 << generic_fls((__u32)(__le32)p->blocks_per_lun) - 1", shifting by a 
negative amount has undefined behavior.  The shift amount, 
"generic_fls((__u32)(__le32)p->blocks_per_lun) - 1", is -1.

3927chip->chipsize = 1 << (fls(le32_to_cpu(p->blocks_per_lun)) - 1);
3928chip->chipsize *= (uint64_t)mtd->erasesize * p->lun_count;
3929chip->bits_per_cell = p->bits_per_cell;
3930
3931if (onfi_feature(chip) & ONFI_FEATURE_16_BIT_BUS)
3932chip->options |= NAND_BUSWIDTH_16;


Yeah, this looks like a bug.


** CID 477215:  Control flow issues  (MISSING_BREAK)
/drivers/mtd/nand/raw/nand_base.c: 4978 in nand_scan_tail()



*** CID 477215:  Control flow issues  (MISSING_BREAK)
/drivers/mtd/nand/raw/nand_base.c: 4978 in nand_scan_tail()
4972pr_warn("No ECC functions supplied;
hardware ECC not possible\n");
4973BUG();
4974}
4975if (!ecc->read_page)
4976ecc->read_page = nand_read_page_hwecc_oob_first;
4977

 CID 477215:  Control flow issues  (MISSING_BREAK)
 The case for value "NAND_ECC_HW" is not terminated by a "break" statement.

4978case NAND_ECC_HW:
4979/* Use standard hwecc read page function? */
4980if (!ecc->read_page)
4981ecc->read_page = nand_read_page_hwecc;
4982if (!ecc->write_page)
4983ecc->write_page = nand_write_page_hwecc;


I think we just need a fallthrough comment here.


** CID 477214:  Integer handling issues  (BAD_SHIFT)
/drivers/mtd/nand/raw/nand_base.c: 4397 in nand_detect()



*** CID 477214:  Integer handling issues  (BAD_SHIFT)
/drivers/mtd/nand/raw/nand_base.c: 4397 in nand_detect()
4391
4392nand_decode_bbm_options(mtd, chip);
4393
4394/* Calculate the address shift from the page size */
4395chip->page_shift = ffs(mtd->writesize) - 1;
4396/* Convert chipsize to number of pages per chip -1 */

 CID 477214:  Integer handling issues  (BAD_SHIFT)
 In expression "chip->chipsize >> chip->page_shift", shifting by a negative amount has 
undefined behavior.  The shift amount, "chip->page_shift", is -1.

4397chip->pagemask = (chip->chipsize >> chip->page_shift) - 1;
4398
4399chip->bbt_erase_shift = chip->phys_erase_shift =
4400ffs(mtd->erasesize) - 1;
4401if (chip->chipsize & 0x)
4402chip->chip_shift = ffs((unsigned)chip->chipsize) - 1;


Buggy, but only when writesize is 0 (which is a bigger bug in the nand chip).


** CID 477213:  Security best practices violations  (DC.WEAK_CRYPTO)
/test/dm/nand.c: 67 in dm_test_nand()



*** CID 477213:  Security best practices violations  (DC.WEAK_CRYPTO)
/test/dm/nand.c: 67 in dm_test_nand()
61  ops.ooblen = mtd->oobsize;
62  ut_assertok(mtd_read_oob(mtd, mtd->erasesize, ));
63  ut_asserteq(0, oob[mtd_to_nand(mtd)->badblockpos]);
64
65  /* Generate some data and write it */
66  for (i = 0; i < size / sizeof(int); i++)

 CID 477213:  Security best practices 

Re: [PATCH v1 09/12] cmd: allow to enable CMD_NAND for SPI NAND devices

[Sean Anderson]

On 12/28/23 10:39, Alexey Romanov wrote:
> Boards with SPI NAND also can use this command.
> 
> Signed-off-by: Alexey Romanov 
> ---
>  cmd/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index c47523a03b..c8484576e2 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -1367,7 +1367,7 @@ config CMD_MUX
>  config CMD_NAND
>   bool "nand"
>   default y if NAND_SUNXI
> - depends on MTD_RAW_NAND
> + depends on MTD_RAW_NAND || MTD_SPI_NAND
>   help
> NAND support.
>  

Reviewed-by: Sean Anderson 

Re: [PATCH v1 11/12] fastboot: enable FASTBOOT_FLASH option for SPI NAND devices

[Sean Anderson]

On 12/28/23 10:39, Alexey Romanov wrote:
> From now we can use FASTBOOT_FLASH_NAND option for SPI NAND.
> 
> Signed-off-by: Alexey Romanov 
> ---
>  drivers/fastboot/Kconfig | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/fastboot/Kconfig b/drivers/fastboot/Kconfig
> index a3df9aa3d0..3cfeea4837 100644
> --- a/drivers/fastboot/Kconfig
> +++ b/drivers/fastboot/Kconfig
> @@ -86,7 +86,7 @@ config FASTBOOT_USB_DEV
>  config FASTBOOT_FLASH
>   bool "Enable FASTBOOT FLASH command"
>   default y if ARCH_SUNXI || ARCH_ROCKCHIP
> - depends on MMC || (MTD_RAW_NAND && CMD_MTDPARTS)
> + depends on MMC || ((MTD_RAW_NAND || MTD_SPI_NAND) && CMD_MTDPARTS)
>   select IMAGE_SPARSE
>   help
> The fastboot protocol includes a "flash" command for writing
> @@ -112,7 +112,7 @@ config FASTBOOT_FLASH_MMC
>  
>  config FASTBOOT_FLASH_NAND
>   bool "FASTBOOT on NAND"
> - depends on MTD_RAW_NAND && CMD_MTDPARTS
> + depends on (MTD_RAW_NAND || MTD_SPI_NAND) && CMD_MTDPARTS
>  
>  endchoice
>  

Reviewed-by: Sean Anderson 

Re: [PATCH v1 10/12] fastboot: check device type for SPI NAND too

[Sean Anderson]

On 12/28/23 10:39, Alexey Romanov wrote:
> SPI NAND devices also supports 'fastboot erase / flash' commands.
> 
> Signed-off-by: Alexey Romanov 
> ---
>  drivers/fastboot/fb_nand.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/fastboot/fb_nand.c b/drivers/fastboot/fb_nand.c
> index 6d3a900c77..39d888301f 100644
> --- a/drivers/fastboot/fb_nand.c
> +++ b/drivers/fastboot/fb_nand.c
> @@ -53,7 +53,7 @@ static int fb_nand_lookup(const char *partname,
>   return ret;
>   }
>  
> - if (dev->id->type != MTD_DEV_TYPE_NAND) {
> + if (dev->id->type != MTD_DEV_TYPE_NAND && dev->id->type != 
> MTD_DEV_TYPE_SPINAND) {
>   pr_err("partition '%s' is not stored on a NAND device",
> partname);
>   fastboot_fail("not a NAND device", response);

Reviewed-by: Sean Anderson 

Re: [PATCH v1 12/12] fastboot: fb_nand: add missing newlines in pr_err() macro

[Sean Anderson]

On 12/28/23 10:39, Alexey Romanov wrote:
> pr_err() doesn't add an newline symbol when printing.
> 
> Signed-off-by: Alexey Romanov 
> ---
>  drivers/fastboot/fb_nand.c | 10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/fastboot/fb_nand.c b/drivers/fastboot/fb_nand.c
> index 39d888301f..9db1903e41 100644
> --- a/drivers/fastboot/fb_nand.c
> +++ b/drivers/fastboot/fb_nand.c
> @@ -48,13 +48,13 @@ static int fb_nand_lookup(const char *partname,
>  
>   ret = find_dev_and_part(partname, , , part);
>   if (ret) {
> - pr_err("cannot find partition: '%s'", partname);
> + pr_err("cannot find partition: '%s'\n", partname);
>   fastboot_fail("cannot find partition", response);
>   return ret;
>   }
>  
>   if (dev->id->type != MTD_DEV_TYPE_NAND && dev->id->type != 
> MTD_DEV_TYPE_SPINAND) {
> - pr_err("partition '%s' is not stored on a NAND device",
> + pr_err("partition '%s' is not stored on a NAND device\n",
> partname);
>   fastboot_fail("not a NAND device", response);
>   return -EINVAL;
> @@ -178,7 +178,7 @@ void fastboot_nand_flash_write(const char *cmd, void 
> *download_buffer,
>  
>   ret = fb_nand_lookup(cmd, , , response);
>   if (ret) {
> - pr_err("invalid NAND device");
> + pr_err("invalid NAND device\n");
>   fastboot_fail("invalid NAND device", response);
>   return;
>   }
> @@ -242,7 +242,7 @@ void fastboot_nand_erase(const char *cmd, char *response)
>  
>   ret = fb_nand_lookup(cmd, , , response);
>   if (ret) {
> - pr_err("invalid NAND device");
> + pr_err("invalid NAND device\n");
>   fastboot_fail("invalid NAND device", response);
>   return;
>   }
> @@ -253,7 +253,7 @@ void fastboot_nand_erase(const char *cmd, char *response)
>  
>   ret = _fb_nand_erase(mtd, part);
>   if (ret) {
> - pr_err("failed erasing from device %s", mtd->name);
> + pr_err("failed erasing from device %s\n", mtd->name);
>   fastboot_fail("failed erasing from device", response);
>   return;
>   }

Reviewed-by: Sean Anderson 

Re: [PATCH v1 01/12] nand: move NAND initialization API to nand/core.c

[Sean Anderson]

On 12/28/23 10:39, Alexey Romanov wrote:
> nand_register() and nand_init() is generic API for both
> RAW and SPI NAND's. We have to move this functions
> from drivers/mtd/nand/raw/nand.c to drivers/mtd/nand/core.c.
> 
> Functions designed to work with RAW NAND should remain
> in drivers/mtd/nand/raw/nand.c.
> 
> Signed-off-by: Alexey Romanov 
> ---
>  drivers/mtd/Kconfig  |   2 +-
>  drivers/mtd/nand/Kconfig |  10 +++
>  drivers/mtd/nand/core.c  | 136 +++
>  drivers/mtd/nand/raw/Kconfig |  10 ---
>  drivers/mtd/nand/raw/nand.c  | 134 --
>  include/nand.h   |   2 +
>  6 files changed, 149 insertions(+), 145 deletions(-)
> 
> diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
> index c56840c849..1902351719 100644
> --- a/drivers/mtd/Kconfig
> +++ b/drivers/mtd/Kconfig
> @@ -260,7 +260,7 @@ config SYS_NAND_MAX_ECCPOS
>  
>  config SYS_NAND_MAX_CHIPS
>   int "NAND max chips"
> - depends on MTD_RAW_NAND || CMD_ONENAND || TARGET_S5PC210_UNIVERSAL || \
> + depends on MTD_RAW_NAND || MTD_SPI_NAND || CMD_ONENAND || 
> TARGET_S5PC210_UNIVERSAL || \
>   SPL_OMAP3_ID_NAND
>   default 1
>   help
> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
> index 78ae04bdcb..9a1d4ac0dc 100644
> --- a/drivers/mtd/nand/Kconfig
> +++ b/drivers/mtd/nand/Kconfig
> @@ -1,6 +1,16 @@
>  config MTD_NAND_CORE
>   tristate
>  
> +config SYS_MAX_NAND_DEVICE
> + int "Maximum number of NAND devices to support"
> + default 1
> +
> +config SYS_NAND_SELF_INIT
> + bool
> + help
> +   This option, if enabled, provides more flexible and linux-like
> +   NAND initialization process.
> +
>  source "drivers/mtd/nand/raw/Kconfig"
>  
>  source "drivers/mtd/nand/spi/Kconfig"
> diff --git a/drivers/mtd/nand/core.c b/drivers/mtd/nand/core.c
> index 4b9dd6a926..ff298e3a0f 100644
> --- a/drivers/mtd/nand/core.c
> +++ b/drivers/mtd/nand/core.c
> @@ -10,6 +10,7 @@
>  #define pr_fmt(fmt)  "nand: " fmt
>  
>  #include 
> +#include 
>  #include 
>  #ifndef __UBOOT__
>  #include 
> @@ -18,6 +19,12 @@
>  #include 
>  #include 
>  
> +int nand_curr_device = -1;
> +
> +static struct mtd_info *nand_info[CONFIG_SYS_MAX_NAND_DEVICE];
> +static char dev_name[CONFIG_SYS_MAX_NAND_DEVICE][8];
> +static unsigned long total_nand_size; /* in kiB */
> +
>  /**
>   * nanddev_isbad() - Check if a block is bad
>   * @nand: NAND device
> @@ -250,6 +257,135 @@ void nanddev_cleanup(struct nand_device *nand)
>  }
>  EXPORT_SYMBOL_GPL(nanddev_cleanup);
>  
> +struct mtd_info *get_nand_dev_by_index(int dev)
> +{
> + if (dev < 0 || dev >= CONFIG_SYS_MAX_NAND_DEVICE || !nand_info[dev] ||
> + !nand_info[dev]->name)
> + return NULL;
> +
> + return nand_info[dev];
> +}
> +EXPORT_SYMBOL_GPL(get_nand_dev_by_index);
> +
> +int nand_mtd_to_devnum(struct mtd_info *mtd)
> +{
> + int i;
> +
> + for (i = 0; i < CONFIG_SYS_MAX_NAND_DEVICE; i++) {
> + if (mtd && get_nand_dev_by_index(i) == mtd)
> + return i;
> + }
> +
> + return -ENODEV;
> +}
> +EXPORT_SYMBOL_GPL(nand_mtd_to_devnum);
> +
> +/* Register an initialized NAND mtd device with the U-Boot NAND command. */
> +int nand_register(int devnum, struct mtd_info *mtd)
> +{
> + if (devnum >= CONFIG_SYS_MAX_NAND_DEVICE)
> + return -EINVAL;
> +
> + nand_info[devnum] = mtd;
> +
> + sprintf(dev_name[devnum], "nand%d", devnum);
> + mtd->name = dev_name[devnum];
> +
> +#ifdef CONFIG_MTD
> + /*
> +  * Add MTD device so that we can reference it later
> +  * via the mtdcore infrastructure (e.g. ubi).
> +  */
> + add_mtd_device(mtd);
> +#endif
> +
> + total_nand_size += mtd->size / 1024;
> +
> + if (nand_curr_device == -1)
> + nand_curr_device = devnum;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(nand_register);
> +
> +#ifdef CONFIG_MTD_CONCAT
> +static void create_mtd_concat(void)
> +{
> + struct mtd_info *nand_info_list[CONFIG_SYS_MAX_NAND_DEVICE];
> + int nand_devices_found = 0;
> + int i;
> +
> + for (i = 0; i < CONFIG_SYS_MAX_NAND_DEVICE; i++) {
> + struct mtd_info *mtd = get_nand_dev_by_index(i);
> + if (mtd != NULL) {
> + nand_info_list[nand_devices_found] = mtd;
> + nand_devices_found++;
> + }
> + }
> + if (nand_devices_found > 1) {
> + struct mtd_info *mtd;
> + char c_mtd_name[16];
> +
> + /*
> +  * We detected multiple devices. Concatenate them together.
> +  */
> + sprintf(c_mtd_name, "nand%d", nand_devices_found);
> + mtd = mtd_concat_create(nand_info_list, nand_devices_found,
> + c_mtd_name);
> +
> + if (mtd == NULL)
> + return;
> +
> + nand_register(nand_devices_found, 

Re: [PATCH v1] fastboot: introduce 'oem board' subcommand

[Sean Anderson]

On 12/28/23 10:25, Alexey Romanov wrote:
> Currently, fastboot protocol in U-Boot has no opportunity
> to execute vendor custom code with verifed boot.

Well, I would say the most conventional way to do this would be something like

=> fastboot 0
=> source \# CONFIG_FASTBOOT_BUF_ADDR

and on your host machine,

$ fastboot stage my_script.itb

where my_script.its looks like

/dts-v1/;

/ {
description = "my script";
#address-cells = <1>;

images {
my-script {
data = /incbin/("my_script.scr");
type = "script";
arch = "arm64";
compression = "none";
hash-1 {
algo = "sha256";
};
};
};

configurations {
default = "conf";
conf {
description = "Load my script";
script = "my-script";
signature {
algo = "sha256,rsa2048";
key-name-hint = "vboot";
sign-images = "script";
};
};
};
};

This method is especially useful to pass complex parameters to your command.
This method of course requires commit bcc85b96b5f ("cmd: source: Support
specifying config name").

Would it be possible to use the above method for your use case?

--Sean

> This patch
> introduce new fastboot subcommand fastboot oem board:,
> which allow to run custom oem_board function.
> =
> Default implementation is __weak. Vendor must redefine it in
> board/ folder with his own logic.
> 
> For example, some vendors have their custom nand/emmc partition
> flashing or erasing. Here some typical command for such use cases:
> 
> - flashing:
> 
>   $ fastboot stage bootloader.img
>   $ fastboot oem board:write_bootloader
> 
> - erasing:
> 
>   $ fastboot oem board:erase_env
> 
> Signed-off-by: Alexey Romanov 
> ---
>  drivers/fastboot/Kconfig  |  7 +++
>  drivers/fastboot/fb_command.c | 15 +++
>  include/fastboot.h|  1 +
>  3 files changed, 23 insertions(+)
> 
> diff --git a/drivers/fastboot/Kconfig b/drivers/fastboot/Kconfig
> index 3cfeea4837..4c955cabab 100644
> --- a/drivers/fastboot/Kconfig
> +++ b/drivers/fastboot/Kconfig
> @@ -241,6 +241,13 @@ config FASTBOOT_OEM_RUN
> this feature if you are using verified boot, as it will allow an
> attacker to bypass any restrictions you have in place.
>  
> +config FASTBOOT_OEM_BOARD
> + bool "Enable the 'oem board' command"
> + help
> +   This extends the fastboot protocol with an "oem board" command. This
> +   command allows running vendor custom code defined in board/ files.
> +   Otherwise, it will do nothing and send fastboot fail.
> +
>  endif # FASTBOOT
>  
>  endmenu
> diff --git a/drivers/fastboot/fb_command.c b/drivers/fastboot/fb_command.c
> index 71cfaec6e9..4d2b451f46 100644
> --- a/drivers/fastboot/fb_command.c
> +++ b/drivers/fastboot/fb_command.c
> @@ -39,6 +39,7 @@ static void reboot_recovery(char *, char *);
>  static void oem_format(char *, char *);
>  static void oem_partconf(char *, char *);
>  static void oem_bootbus(char *, char *);
> +static void oem_board(char *, char *);
>  static void run_ucmd(char *, char *);
>  static void run_acmd(char *, char *);
>  
> @@ -106,6 +107,10 @@ static const struct {
>   .command = "oem run",
>   .dispatch = CONFIG_IS_ENABLED(FASTBOOT_OEM_RUN, (run_ucmd), 
> (NULL))
>   },
> + [FASTBOOT_COMMAND_OEM_BOARD] = {
> + .command = "oem board",
> + .dispatch = CONFIG_IS_ENABLED(FASTBOOT_OEM_BOARD, (oem_board), 
> (NULL))
> + },
>   [FASTBOOT_COMMAND_UCMD] = {
>   .command = "UCmd",
>   .dispatch = CONFIG_IS_ENABLED(FASTBOOT_UUU_SUPPORT, (run_ucmd), 
> (NULL))
> @@ -489,3 +494,13 @@ static void __maybe_unused oem_bootbus(char 
> *cmd_parameter, char *response)
>   else
>   fastboot_okay(NULL, response);
>  }
> +
> +void __weak fastboot_oem_board(char *cmd_parameter, void *data, u32 size, 
> char *response)
> +{
> + fastboot_fail("oem board function not defined", response);
> +}
> +
> +static void __maybe_unused oem_board(char *cmd_parameter, char *response)
> +{
> + fastboot_oem_board(cmd_parameter, fastboot_buf_addr, image_size, 
> response);
> +}
> diff --git a/include/fastboot.h b/include/fastboot.h
> index 296451f89d..06c1f26b6c 100644
> --- a/include/fastboot.h
> +++ b/include/fastboot.h
> @@ -37,6 +37,7 @@ enum {
>   FASTBOOT_COMMAND_OEM_PARTCONF,
>   FASTBOOT_COMMAND_OEM_BOOTBUS,
>   FASTBOOT_COMMAND_OEM_RUN,
> + FASTBOOT_COMMAND_OEM_BOARD,
>   FASTBOOT_COMMAND_ACMD,
>   FASTBOOT_COMMAND_UCMD,
>   FASTBOOT_COMMAND_COUNT

Re: [PATHv2 2/9] net: sandbox: fix NULL pointer derefences

[Sean Anderson]

On 12/26/23 01:18, Maxim Uvarov wrote:



On Tue, 26 Dec 2023 at 04:43, Sean Anderson mailto:sean...@gmail.com>> wrote:

On 12/25/23 10:39, Maxim Uvarov wrote:
 > Add additional checks for NULL pointers.
 >
 > Signed-off-by: Maxim Uvarov mailto:maxim.uva...@linaro.org>>
 > ---
 >   drivers/net/sandbox.c | 3 +++
 >   1 file changed, 3 insertions(+)
 >
 > diff --git a/drivers/net/sandbox.c b/drivers/net/sandbox.c
 > index 13022addb6..d91935e032 100644
 > --- a/drivers/net/sandbox.c
 > +++ b/drivers/net/sandbox.c
 > @@ -65,6 +65,9 @@ int sandbox_eth_arp_req_to_reply(struct udevice *dev, 
void *packet,
 >       struct ethernet_hdr *eth_recv;
 >       struct arp_hdr *arp_recv;
 >
 > +     if (!priv)
 > +             return -EAGAIN;
 > +

When can priv be NULL?

--Sean


Function
struct eth_sandbox_priv *priv = dev_get_priv(dev)
can return NULL. If you ask why it doesn't return NULL without lwip patches and 
can return NULL with lwip patch while there is no clear code dependency..
Then I can not say right now and need additional investigation. But anyway the 
return code of dev_dev_priv() has to be checked I think.


If you set priv_auto to a nonzero value, dev_get_priv will always return 
non-null
and does not need to be checked. So this is a NACK from me until you can 
justify this.

--Sean

Re: [PATHv2 8/9] omap3: use device specific naming for mem_init

[Sean Anderson]

On 12/25/23 10:39, Maxim Uvarov wrote:

Use device specific naming for functions so as to not overlap
with common function names.

Signed-off-by: Maxim Uvarov 
Reviewed-by: Tom Rini 
---
  arch/arm/include/asm/arch-omap3/mem.h | 2 +-
  arch/arm/mach-omap2/omap3/board.c | 2 +-
  arch/arm/mach-omap2/omap3/emif4.c | 4 ++--
  arch/arm/mach-omap2/omap3/sdrc.c  | 6 +++---
  4 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/arch/arm/include/asm/arch-omap3/mem.h 
b/arch/arm/include/asm/arch-omap3/mem.h
index 569779c55e..fce3568eca 100644
--- a/arch/arm/include/asm/arch-omap3/mem.h
+++ b/arch/arm/include/asm/arch-omap3/mem.h
@@ -475,7 +475,7 @@ enum {
  #ifndef __ASSEMBLY__
  
  /* Function prototypes */

-void mem_init(void);
+void omap3_mem_init(void);
  
  u32 is_mem_sdr(void);

  u32 mem_ok(u32 cs);
diff --git a/arch/arm/mach-omap2/omap3/board.c 
b/arch/arm/mach-omap2/omap3/board.c
index 8b70251457..c76a95dd5d 100644
--- a/arch/arm/mach-omap2/omap3/board.c
+++ b/arch/arm/mach-omap2/omap3/board.c
@@ -216,7 +216,7 @@ void s_init(void)
  void board_init_f(ulong dummy)
  {
early_system_init();
-   mem_init();
+   omap3_mem_init();
/*
* Save the boot parameters passed from romcode.
* We cannot delay the saving further than this,
diff --git a/arch/arm/mach-omap2/omap3/emif4.c 
b/arch/arm/mach-omap2/omap3/emif4.c
index 7e5a281922..4fbfb387ab 100644
--- a/arch/arm/mach-omap2/omap3/emif4.c
+++ b/arch/arm/mach-omap2/omap3/emif4.c
@@ -159,10 +159,10 @@ int dram_init_banksize(void)
  }
  
  /*

- * mem_init() -
+ * omap3_mem_init() -
   *  - Initialize memory subsystem
   */
-void mem_init(void)
+void omap3_mem_init(void)
  {
do_emif4_init();
  }
diff --git a/arch/arm/mach-omap2/omap3/sdrc.c b/arch/arm/mach-omap2/omap3/sdrc.c
index 5d43e7c9cf..f2a0769b9d 100644
--- a/arch/arm/mach-omap2/omap3/sdrc.c
+++ b/arch/arm/mach-omap2/omap3/sdrc.c
@@ -4,7 +4,7 @@
   *
   * This file has been created after exctracting and consolidating
   * the SDRC related content from mem.c and board.c, also created
- * generic init function (mem_init).
+ * generic init function (omap3_mem_init).
   *
   * Copyright (C) 2004-2010
   * Texas Instruments Incorporated - https://www.ti.com/
@@ -232,11 +232,11 @@ int dram_init_banksize(void)
  }
  
  /*

- * mem_init -
+ * map3_mem_init -


nit: omap3


   *  - Init the sdrc chip,
   *  - Selects CS0 and CS1,
   */
-void mem_init(void)
+void omap3_mem_init(void)
  {
/* only init up first bank here */
do_sdrc_init(CS0, EARLY_INIT);

Re: [PATHv2 2/9] net: sandbox: fix NULL pointer derefences

[Sean Anderson]

On 12/25/23 10:39, Maxim Uvarov wrote:

Add additional checks for NULL pointers.

Signed-off-by: Maxim Uvarov 
---
  drivers/net/sandbox.c | 3 +++
  1 file changed, 3 insertions(+)

diff --git a/drivers/net/sandbox.c b/drivers/net/sandbox.c
index 13022addb6..d91935e032 100644
--- a/drivers/net/sandbox.c
+++ b/drivers/net/sandbox.c
@@ -65,6 +65,9 @@ int sandbox_eth_arp_req_to_reply(struct udevice *dev, void 
*packet,
struct ethernet_hdr *eth_recv;
struct arp_hdr *arp_recv;
  
+	if (!priv)

+   return -EAGAIN;
+


When can priv be NULL?

--Sean


if (ntohs(eth->et_protlen) != PROT_ARP)
return -EAGAIN;
  

Re: Passing boot logs to Linux?

[Sean Anderson]

On 12/20/23 14:24, Csókás Bence wrote:

On 2023. 12. 20. 9:29, Dragan Simic wrote:

On 2023-12-20 08:52, Csókás Bence wrote:

On 2023. 12. 20. 8:09, Dragan Simic wrote:

On 2023-12-20 07:49, Csókás Bence wrote:

I don't think that would be a huge problem, Linux userspace can filter
ANSI control codes if it wants to. For now, I'd like a byte-for-byte
copy of the console, as-is, presented to Linux.


But wouldn't recording the control sequences as-is actually be pretty
much useless?  For example, some user can spend a few minutes
scrolling through the boot menu, which would produce a fair amount of
nearly useless recorded data.

Moreover, if I'm not wrong, viewing or parsing such as-is data would
actually require replaying the control sequences, to reproduce the
actual console contents as it was recorded, which would be quite
cumbersome.


It *is* useless, but most of the times the user won't be scrolling at
all anyways, that's why I don't think these few extra bytes would be a
problem.


Quite frankly, when it comes to computing in general, taking the "ah, it
probably won't happen" approach isn't the best thing to do.  We should
aim toward covering properly even the edge cases, if you agree.


You're absolutely right; however, I did not say "it won't happen", I
said that Linux userspace can go deal with it if it happens, and if it
wants to. And that in my opinion, the best course of action for U-Boot
is to record *everything* (as it does now), and then userspace can
decide to filter it for eg. ANSI codes. But I think it's best to keep
the U-Boot code dead simple, and also supply the most data to userspace.



That's what I read as well. Is there support for U-boot to write and
Linux to read PStores?


No and yes, but U-Boot can already read pstore.  Please see
doc/usage/cmd/pstore.rst for the U-Boot part, and
Documentation/admin-guide/pstore-blk.rst for the Linux kernel part.


Irrelevant, as we only want to write out the console log to U-Boot, and
not the other way around (that's for collecting panic logs, which is
already implemented).


Another benefit of using pstore would be no permanently wasted RAM for
the recorded console contents.  Also, having the data recorded to a
storage device also goes along with providing permanent records.


I'm positively *not gonna save boot logs to disk*, as most embedded
systems have Flash-based storage media; writing to them on every boot
would be devastating. Plus, I don't want the console subsystem to depend
on any file/disk operations/drivers.


pstore is agnostic to the backend used. In U-Boot we only support RAM
backends at the moment.

--Sean

Re: Passing boot logs to Linux?

[Sean Anderson]

On 12/19/23 23:11, Simon Glass wrote:

Hi Csókás,

On Tue, 19 Dec 2023 at 13:15, Csókás Bence  wrote:


Hi!

Is passing the U-Boot boot log to Linux supported yet? We are working
with a third-party solution, which works, but is a bit hacky, so I was
wondering if an official solution has been merged yet.

I saw that there was an option CONFIG_CONSOLE_RECORD that saves
everything to a membuff, but I don't know if that can be exported to
Linux yet. And if not in the tree yet, would such a patch be welcome?


Not yet, but yes I would like to see this.


I think most of the infrastructure is already here. We could use either
console recording (as mentioned above), which is more complete, or a new
LOG_DRIVER (which would have the advantage of omitting things like countdowns).

In terms of sending things to Linux, I think the natural choice would be pstore,
which we already have a parser for.

--Sean

Re: [PATCH v2 1/1] clk: clk-gpio: add actual gated clock

[Sean Anderson]

On 12/17/23 11:45, Marek Vasut wrote:

On 12/17/23 16:37, Sean Anderson wrote:

On 12/17/23 10:19, Marek Vasut wrote:

On 12/17/23 02:20, Sean Anderson wrote:

On 12/16/23 19:19, Marek Vasut wrote:

On 12/16/23 17:56, Sean Anderson wrote:

On 12/16/23 11:52, Marek Vasut wrote:

On 12/16/23 16:37, Sean Anderson wrote:

On 12/16/23 03:48, Svyatoslav Ryhel wrote:

Existing gpio-gate-clock driver acts like a simple GPIO switch without any
effect on gated clock. Add actual clock actions into enable/disable ops and
implement get_rate op by passing gated clock if it is enabled.

Signed-off-by: Svyatoslav Ryhel 
---
  drivers/clk/clk-gpio.c | 44 ++
  1 file changed, 36 insertions(+), 8 deletions(-)

diff --git a/drivers/clk/clk-gpio.c b/drivers/clk/clk-gpio.c
index 26d795b978..72d9747a47 100644
--- a/drivers/clk/clk-gpio.c
+++ b/drivers/clk/clk-gpio.c
@@ -3,19 +3,23 @@
   * Copyright (C) 2023 Marek Vasut 
   */
-#include 
-#include 
-#include 
+#include 
  #include 
+#include 
+#include 
+
+#include 
  struct clk_gpio_priv {
-    struct gpio_desc    enable;
+    struct gpio_desc    enable;    /* GPIO, controlling the gate */
+    struct clk    *clk;    /* Gated clock */
  };
  static int clk_gpio_enable(struct clk *clk)
  {
  struct clk_gpio_priv *priv = dev_get_priv(clk->dev);
+    clk_enable(priv->clk);
  dm_gpio_set_value(>enable, 1);
  return 0;
@@ -26,21 +30,45 @@ static int clk_gpio_disable(struct clk *clk)
  struct clk_gpio_priv *priv = dev_get_priv(clk->dev);
  dm_gpio_set_value(>enable, 0);
+    clk_disable(priv->clk);
  return 0;
  }
+static ulong clk_gpio_get_rate(struct clk *clk)
+{
+    struct clk_gpio_priv *priv = dev_get_priv(clk->dev);
+
+    return clk_get_rate(priv->clk);
+}
+
  const struct clk_ops clk_gpio_ops = {
  .enable    = clk_gpio_enable,
  .disable    = clk_gpio_disable,
+    .get_rate    = clk_gpio_get_rate,
  };
-static int clk_gpio_probe(struct udevice *dev)
+static int clk_gpio_of_to_plat(struct udevice *dev)
  {
  struct clk_gpio_priv *priv = dev_get_priv(dev);
+    int ret;
-    return gpio_request_by_name(dev, "enable-gpios", 0,
-    >enable, GPIOD_IS_OUT);
+    priv->clk = devm_clk_get(dev, NULL);
+    if (IS_ERR(priv->clk)) {
+    log_debug("%s: Could not get gated clock: %ld\n",
+  __func__, PTR_ERR(priv->clk));
+    return PTR_ERR(priv->clk);
+    }
+
+    ret = gpio_request_by_name(dev, "enable-gpios", 0,
+   >enable, GPIOD_IS_OUT);
+    if (ret) {
+    log_debug("%s: Could not decode enable-gpios (%d)\n",
+  __func__, ret);
+    return ret;
+    }
+
+    return 0;


All this forwarding of clock enable/disable/get_rate for parent clock should 
already be done in clock core. If it is not, then it should be fixed in clock 
core.


Only CCF forwards things. It's up to the driver to do it in the non-CCF case.


Shall we add dependency on CCF instead of duplicating code in drivers ?


I'd rather not. This will be useful on non-CCF systems.


Then the forwarding functionality should be in the clock core, basically if a driver does 
not implement callback , call parent clock callback , and 
repeat until you reach the top of the clock tree. If you do reach the top, fail with some 
error code. Isn't that how clock enable already works anyway ?


No. In the non-CCF case, we have no idea what the parent of any given clock is. 
So
there's no framework we can provide.


Huh, but this driver is attempting to grab some sort of parent clock and enable 
them, so how does that work ?


The driver knows what its parent is, so it can enable it just fine.


Now, you might say "why not add a get_parent op?" Well, that will work fine for
enabling things, but it is a bit hairy on the disable side. You really need to 
have
refcounting to protect parent clocks (which can have many children). But we 
don't
really have anywhere to store refcounts. struct clk has a member for it, but 
that's
only because it's abused as private data by CCF. Non-CCF clocks have no clock
subsystem data allocated at all. Another alternative is to just only disable 
the leaf
node, which is what we currently do. That's OK I think.

But that solution requires some more-intensive effort which I don't want to 
impose on
random contributors. So for the time being, all non-CCF clock drivers must 
handle
enabling their own parent clocks.


So, in light of this, why not push for use of CCF as much as possible and 
deprecate the current non-CCF clock support ? It seems like the right thing to 
do, and avoids growing ad-hoc workarounds for missing core functionality in 
drivers, which I really want to avoid .


Because the CCF framework abuses struct clk for private data, uses strings 
everywhere, and
dynamically allocates all its clocks (which are known at compile-time). You can 
easil

Re: [PATCH v2 1/1] clk: clk-gpio: add actual gated clock

[Sean Anderson]

On 12/17/23 10:19, Marek Vasut wrote:

On 12/17/23 02:20, Sean Anderson wrote:

On 12/16/23 19:19, Marek Vasut wrote:

On 12/16/23 17:56, Sean Anderson wrote:

On 12/16/23 11:52, Marek Vasut wrote:

On 12/16/23 16:37, Sean Anderson wrote:

On 12/16/23 03:48, Svyatoslav Ryhel wrote:

Existing gpio-gate-clock driver acts like a simple GPIO switch without any
effect on gated clock. Add actual clock actions into enable/disable ops and
implement get_rate op by passing gated clock if it is enabled.

Signed-off-by: Svyatoslav Ryhel 
---
  drivers/clk/clk-gpio.c | 44 ++
  1 file changed, 36 insertions(+), 8 deletions(-)

diff --git a/drivers/clk/clk-gpio.c b/drivers/clk/clk-gpio.c
index 26d795b978..72d9747a47 100644
--- a/drivers/clk/clk-gpio.c
+++ b/drivers/clk/clk-gpio.c
@@ -3,19 +3,23 @@
   * Copyright (C) 2023 Marek Vasut 
   */
-#include 
-#include 
-#include 
+#include 
  #include 
+#include 
+#include 
+
+#include 
  struct clk_gpio_priv {
-    struct gpio_desc    enable;
+    struct gpio_desc    enable;    /* GPIO, controlling the gate */
+    struct clk    *clk;    /* Gated clock */
  };
  static int clk_gpio_enable(struct clk *clk)
  {
  struct clk_gpio_priv *priv = dev_get_priv(clk->dev);
+    clk_enable(priv->clk);
  dm_gpio_set_value(>enable, 1);
  return 0;
@@ -26,21 +30,45 @@ static int clk_gpio_disable(struct clk *clk)
  struct clk_gpio_priv *priv = dev_get_priv(clk->dev);
  dm_gpio_set_value(>enable, 0);
+    clk_disable(priv->clk);
  return 0;
  }
+static ulong clk_gpio_get_rate(struct clk *clk)
+{
+    struct clk_gpio_priv *priv = dev_get_priv(clk->dev);
+
+    return clk_get_rate(priv->clk);
+}
+
  const struct clk_ops clk_gpio_ops = {
  .enable    = clk_gpio_enable,
  .disable    = clk_gpio_disable,
+    .get_rate    = clk_gpio_get_rate,
  };
-static int clk_gpio_probe(struct udevice *dev)
+static int clk_gpio_of_to_plat(struct udevice *dev)
  {
  struct clk_gpio_priv *priv = dev_get_priv(dev);
+    int ret;
-    return gpio_request_by_name(dev, "enable-gpios", 0,
-    >enable, GPIOD_IS_OUT);
+    priv->clk = devm_clk_get(dev, NULL);
+    if (IS_ERR(priv->clk)) {
+    log_debug("%s: Could not get gated clock: %ld\n",
+  __func__, PTR_ERR(priv->clk));
+    return PTR_ERR(priv->clk);
+    }
+
+    ret = gpio_request_by_name(dev, "enable-gpios", 0,
+   >enable, GPIOD_IS_OUT);
+    if (ret) {
+    log_debug("%s: Could not decode enable-gpios (%d)\n",
+  __func__, ret);
+    return ret;
+    }
+
+    return 0;


All this forwarding of clock enable/disable/get_rate for parent clock should 
already be done in clock core. If it is not, then it should be fixed in clock 
core.


Only CCF forwards things. It's up to the driver to do it in the non-CCF case.


Shall we add dependency on CCF instead of duplicating code in drivers ?


I'd rather not. This will be useful on non-CCF systems.


Then the forwarding functionality should be in the clock core, basically if a driver does 
not implement callback , call parent clock callback , and 
repeat until you reach the top of the clock tree. If you do reach the top, fail with some 
error code. Isn't that how clock enable already works anyway ?


No. In the non-CCF case, we have no idea what the parent of any given clock is. 
So
there's no framework we can provide.

Now, you might say "why not add a get_parent op?" Well, that will work fine for
enabling things, but it is a bit hairy on the disable side. You really need to 
have
refcounting to protect parent clocks (which can have many children). But we 
don't
really have anywhere to store refcounts. struct clk has a member for it, but 
that's
only because it's abused as private data by CCF. Non-CCF clocks have no clock
subsystem data allocated at all. Another alternative is to just only disable 
the leaf
node, which is what we currently do. That's OK I think.

But that solution requires some more-intensive effort which I don't want to 
impose on
random contributors. So for the time being, all non-CCF clock drivers must 
handle
enabling their own parent clocks.

--Sean

Re: [NEXT] Pull request for efi-next-20231217

[Sean Anderson]

On 12/17/23 08:47, Heinrich Schuchardt wrote:

Dear Tom,

This pull request if for the next branch.

The following changes since commit cd948210332783c2b1c6d10a982a80c0da4f69b9:

   Revert "board: ti: am62x/am62ax: Update virtual interrupt allocations
in board config" (2023-12-15 20:23:59 -0500)

are available in the Git repository at:

   https://source.denx.de/u-boot/custodians/u-boot-efi.git
tags/efi-next-20231217

for you to fetch changes up to 291ab919355df5ee28183546049d5fbdb2777d2d:

   doc: Replace examples of MD5 and SHA1 with SHA256 (2023-12-17
13:06:48 +0100)

Gitlab CI showed no issues:

https://source.denx.de/u-boot/custodians/u-boot-efi/-/pipelines/18990


Pull request for efi-next-20231217

Documentation:

* replace MD5 and SHA1 by SHA256 in examples

UEFI:

* Refactor boot manager and bootefi command to let the EFI boot method
work without shell.


AKASHI Takahiro (8):
   cmd: bootefi: unfold do_bootefi_image()
   cmd: bootefi: re-organize do_bootefi()
   cmd: bootefi: carve out EFI boot manager interface
   cmd: bootefi: carve out binary execution interface
   cmd: bootefi: localize global device paths for efi_selftest
   cmd: bootefi: move library interfaces under lib/efi_loader
   cmd: efidebug: ease efi configuration dependency
   bootmeth: use efi_loader interfaces instead of bootefi command

Sean Anderson (1):
   doc: Replace examples of MD5 and SHA1 with SHA256


FYI there is a v2 of this, but is mostly just expands the commit message. I am
fine sending a follow-up patch.

--Sean


  boot/Kconfig |   4 +-
  boot/Makefile    |   2 +-
  boot/bootm_os.c  |  32 +-
  boot/bootmeth_efi.c  |   8 +-
  boot/bootmeth_efi_mgr.c  |   2 +-
  cmd/Kconfig  |  16 +-
  cmd/bootefi.c    | 670
+--
  cmd/efidebug.c   |   4 +-
  doc/chromium/files/chromebook_jerry.its  |   4 +-
  doc/chromium/files/nyan-big.its  |   4 +-
  doc/usage/cmd/imxtract.rst   |   6 +-
  doc/usage/fit/beaglebone_vboot.rst   |  74 ++--
  doc/usage/fit/howto.rst  |  40 +-
  doc/usage/fit/kernel.rst |   6 +-
  doc/usage/fit/kernel_fdt.rst |   4 +-
  doc/usage/fit/kernel_fdts_compressed.rst |   6 +-
  doc/usage/fit/multi-with-fpga.rst    |   6 +-
  doc/usage/fit/multi-with-loadables.rst   |   8 +-
  doc/usage/fit/multi.rst  |  12 +-
  doc/usage/fit/sign-configs.rst   |   6 +-
  doc/usage/fit/sign-images.rst    |   4 +-
  doc/usage/fit/signature.rst  |  22 +-
  doc/usage/fit/update3.rst    |   6 +-
  doc/usage/fit/update_uboot.rst   |   2 +-
  doc/usage/fit/x86-fit-boot.rst   |   8 +-
  include/efi_loader.h |  12 +-
  lib/efi_loader/efi_bootmgr.c | 530 
  test/boot/bootflow.c |   2 +-
  28 files changed, 771 insertions(+), 729 deletions(-)

Re: [PATCH v2 1/1] clk: clk-gpio: add actual gated clock

[Sean Anderson]

On 12/16/23 19:19, Marek Vasut wrote:

On 12/16/23 17:56, Sean Anderson wrote:

On 12/16/23 11:52, Marek Vasut wrote:

On 12/16/23 16:37, Sean Anderson wrote:

On 12/16/23 03:48, Svyatoslav Ryhel wrote:

Existing gpio-gate-clock driver acts like a simple GPIO switch without any
effect on gated clock. Add actual clock actions into enable/disable ops and
implement get_rate op by passing gated clock if it is enabled.

Signed-off-by: Svyatoslav Ryhel 
---
  drivers/clk/clk-gpio.c | 44 ++
  1 file changed, 36 insertions(+), 8 deletions(-)

diff --git a/drivers/clk/clk-gpio.c b/drivers/clk/clk-gpio.c
index 26d795b978..72d9747a47 100644
--- a/drivers/clk/clk-gpio.c
+++ b/drivers/clk/clk-gpio.c
@@ -3,19 +3,23 @@
   * Copyright (C) 2023 Marek Vasut 
   */
-#include 
-#include 
-#include 
+#include 
  #include 
+#include 
+#include 
+
+#include 
  struct clk_gpio_priv {
-    struct gpio_desc    enable;
+    struct gpio_desc    enable;    /* GPIO, controlling the gate */
+    struct clk    *clk;    /* Gated clock */
  };
  static int clk_gpio_enable(struct clk *clk)
  {
  struct clk_gpio_priv *priv = dev_get_priv(clk->dev);
+    clk_enable(priv->clk);
  dm_gpio_set_value(>enable, 1);
  return 0;
@@ -26,21 +30,45 @@ static int clk_gpio_disable(struct clk *clk)
  struct clk_gpio_priv *priv = dev_get_priv(clk->dev);
  dm_gpio_set_value(>enable, 0);
+    clk_disable(priv->clk);
  return 0;
  }
+static ulong clk_gpio_get_rate(struct clk *clk)
+{
+    struct clk_gpio_priv *priv = dev_get_priv(clk->dev);
+
+    return clk_get_rate(priv->clk);
+}
+
  const struct clk_ops clk_gpio_ops = {
  .enable    = clk_gpio_enable,
  .disable    = clk_gpio_disable,
+    .get_rate    = clk_gpio_get_rate,
  };
-static int clk_gpio_probe(struct udevice *dev)
+static int clk_gpio_of_to_plat(struct udevice *dev)
  {
  struct clk_gpio_priv *priv = dev_get_priv(dev);
+    int ret;
-    return gpio_request_by_name(dev, "enable-gpios", 0,
-    >enable, GPIOD_IS_OUT);
+    priv->clk = devm_clk_get(dev, NULL);
+    if (IS_ERR(priv->clk)) {
+    log_debug("%s: Could not get gated clock: %ld\n",
+  __func__, PTR_ERR(priv->clk));
+    return PTR_ERR(priv->clk);
+    }
+
+    ret = gpio_request_by_name(dev, "enable-gpios", 0,
+   >enable, GPIOD_IS_OUT);
+    if (ret) {
+    log_debug("%s: Could not decode enable-gpios (%d)\n",
+  __func__, ret);
+    return ret;
+    }
+
+    return 0;


All this forwarding of clock enable/disable/get_rate for parent clock should 
already be done in clock core. If it is not, then it should be fixed in clock 
core.


Only CCF forwards things. It's up to the driver to do it in the non-CCF case.


Shall we add dependency on CCF instead of duplicating code in drivers ?


I'd rather not. This will be useful on non-CCF systems.

--Sean

[PATCH 1/3] clk: Remove rfree

[Sean Anderson]

Nothing uses this function. Remove it. Since clk_free no longer does
anything, just stub it out.

Signed-off-by: Sean Anderson 
---

 arch/sandbox/include/asm/clk.h |  8 
 drivers/clk/clk-uclass.c   | 14 --
 drivers/clk/clk_sandbox.c  | 12 
 drivers/clk/clk_sandbox_test.c | 12 
 include/clk-uclass.h   | 10 --
 include/clk.h  | 18 --
 test/dm/clk.c  |  9 -
 7 files changed, 4 insertions(+), 79 deletions(-)

diff --git a/arch/sandbox/include/asm/clk.h b/arch/sandbox/include/asm/clk.h
index 2b7dbca8f75..b8204227023 100644
--- a/arch/sandbox/include/asm/clk.h
+++ b/arch/sandbox/include/asm/clk.h
@@ -180,14 +180,6 @@ int sandbox_clk_test_disable(struct udevice *dev, int id);
  * @return:0 if OK, or a negative error code.
  */
 int sandbox_clk_test_disable_bulk(struct udevice *dev);
-/**
- * sandbox_clk_test_free - Ask the sandbox clock test device to free its
- * clocks.
- *
- * @dev:   The sandbox clock test (client) device.
- * @return:0 if OK, or a negative error code.
- */
-int sandbox_clk_test_free(struct udevice *dev);
 /**
  * sandbox_clk_test_release_bulk - Ask the sandbox clock test device to release
  * all clocks in it's clock bulk struct.
diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
index 3b5e3f9c86b..9260e434988 100644
--- a/drivers/clk/clk-uclass.c
+++ b/drivers/clk/clk-uclass.c
@@ -461,20 +461,6 @@ int clk_request(struct udevice *dev, struct clk *clk)
return ops->request(clk);
 }
 
-void clk_free(struct clk *clk)
-{
-   const struct clk_ops *ops;
-
-   debug("%s(clk=%p)\n", __func__, clk);
-   if (!clk_valid(clk))
-   return;
-   ops = clk_dev_ops(clk->dev);
-
-   if (ops->rfree)
-   ops->rfree(clk);
-   return;
-}
-
 ulong clk_get_rate(struct clk *clk)
 {
const struct clk_ops *ops;
diff --git a/drivers/clk/clk_sandbox.c b/drivers/clk/clk_sandbox.c
index 636914db8ca..73d943f9e09 100644
--- a/drivers/clk/clk_sandbox.c
+++ b/drivers/clk/clk_sandbox.c
@@ -101,17 +101,6 @@ static int sandbox_clk_request(struct clk *clk)
return 0;
 }
 
-static void sandbox_clk_free(struct clk *clk)
-{
-   struct sandbox_clk_priv *priv = dev_get_priv(clk->dev);
-
-   if (clk->id >= SANDBOX_CLK_ID_COUNT)
-   return;
-
-   priv->requested[clk->id] = false;
-   return;
-}
-
 static struct clk_ops sandbox_clk_ops = {
.round_rate = sandbox_clk_round_rate,
.get_rate   = sandbox_clk_get_rate,
@@ -119,7 +108,6 @@ static struct clk_ops sandbox_clk_ops = {
.enable = sandbox_clk_enable,
.disable= sandbox_clk_disable,
.request= sandbox_clk_request,
-   .rfree  = sandbox_clk_free,
 };
 
 static int sandbox_clk_probe(struct udevice *dev)
diff --git a/drivers/clk/clk_sandbox_test.c b/drivers/clk/clk_sandbox_test.c
index 5807a454f3b..d9ec070110e 100644
--- a/drivers/clk/clk_sandbox_test.c
+++ b/drivers/clk/clk_sandbox_test.c
@@ -134,18 +134,6 @@ int sandbox_clk_test_disable_bulk(struct udevice *dev)
return clk_disable_bulk(>bulk);
 }
 
-int sandbox_clk_test_free(struct udevice *dev)
-{
-   struct sandbox_clk_test *sbct = dev_get_priv(dev);
-   int i;
-
-   devm_clk_put(dev, sbct->clkps[SANDBOX_CLK_TEST_ID_DEVM1]);
-   for (i = 0; i < SANDBOX_CLK_TEST_NON_DEVM_COUNT; i++)
-   clk_free(>clks[i]);
-
-   return 0;
-}
-
 int sandbox_clk_test_release_bulk(struct udevice *dev)
 {
struct sandbox_clk_test *sbct = dev_get_priv(dev);
diff --git a/include/clk-uclass.h b/include/clk-uclass.h
index cd62848bece..4353b664800 100644
--- a/include/clk-uclass.h
+++ b/include/clk-uclass.h
@@ -18,7 +18,6 @@ struct ofnode_phandle_args;
  * struct clk_ops - The functions that a clock driver must implement.
  * @of_xlate: Translate a client's device-tree (OF) clock specifier.
  * @request: Request a translated clock.
- * @rfree: Free a previously requested clock.
  * @round_rate: Adjust a rate to the exact rate a clock can provide.
  * @get_rate: Get current clock rate.
  * @set_rate: Set current clock rate.
@@ -33,7 +32,6 @@ struct clk_ops {
int (*of_xlate)(struct clk *clock,
struct ofnode_phandle_args *args);
int (*request)(struct clk *clock);
-   void (*rfree)(struct clk *clock);
ulong (*round_rate)(struct clk *clk, ulong rate);
ulong (*get_rate)(struct clk *clk);
ulong (*set_rate)(struct clk *clk, ulong rate);
@@ -81,14 +79,6 @@ int of_xlate(struct clk *clock, struct ofnode_phandle_args 
*args);
  */
 int request(struct clk *clock);
 
-/**
- * rfree() - Free a previously requested clock.
- * @clock: The clock to free.
- *
- * Free any resources allocated in request().
- */
-void rfree(struct clk *clock);
-
 /**
  * round_rate() - Adjust a rat

[PATCH 2/3] treewide: Remove clk_free

[Sean Anderson]

This function is a no-op. Remove it.

Signed-off-by: Sean Anderson 
---

 arch/arm/mach-rockchip/rk3288/rk3288.c|  2 -
 arch/arm/mach-socfpga/clock_manager_agilex.c  |  2 -
 arch/arm/mach-socfpga/clock_manager_arria10.c |  7 +--
 arch/arm/mach-socfpga/clock_manager_n5x.c |  2 -
 arch/arm/mach-zynq/clk.c  |  2 -
 arch/mips/mach-pic32/cpu.c|  7 +--
 board/microchip/pic32mzda/pic32mzda.c |  2 -
 board/sipeed/maix/maix.c  |  1 -
 board/synopsys/hsdk/clk-lib.c |  2 -
 drivers/clk/aspeed/clk_ast2600.c  |  2 -
 drivers/clk/at91/compat.c | 14 +-
 drivers/clk/clk-uclass.c  | 26 +--
 drivers/clk/clk-xlnx-clock-wizard.c   |  1 -
 drivers/clk/clk_versaclock.c  | 12 +-
 drivers/clk/clk_zynq.c|  2 -
 drivers/clk/clk_zynqmp.c  |  2 -
 drivers/clk/imx/clk-imx8.c|  2 -
 drivers/clk/mvebu/armada-37xx-periph.c|  2 -
 drivers/cpu/riscv_cpu.c   |  2 -
 drivers/dma/bcm6348-iudma.c   |  2 -
 drivers/gpio/at91_gpio.c  |  2 -
 drivers/gpio/atmel_pio4.c |  2 -
 drivers/gpio/gpio-rcar.c  |  1 -
 drivers/hwspinlock/stm32_hwspinlock.c |  6 +--
 drivers/i2c/at91_i2c.c|  2 -
 drivers/i2c/designware_i2c.c  |  2 -
 drivers/i2c/i2c-microchip.c   |  2 -
 drivers/i2c/npcm_i2c.c|  1 -
 drivers/i2c/ocores_i2c.c  |  2 -
 drivers/i2c/stm32f7_i2c.c |  4 +-
 drivers/mailbox/stm32-ipcc.c  |  7 +--
 drivers/misc/ls2_sfp.c|  1 -
 drivers/mmc/arm_pl180_mmci.c  |  1 -
 drivers/mmc/aspeed_sdhci.c|  4 +-
 drivers/mmc/atmel_sdhci.c |  2 -
 drivers/mmc/gen_atmel_mci.c   | 19 +++-
 drivers/mmc/msm_sdhci.c   |  1 -
 drivers/mmc/pic32_sdhci.c |  1 -
 drivers/mmc/renesas-sdhi.c| 21 +++--
 drivers/mmc/snps_dw_mmc.c |  8 +---
 drivers/mmc/socfpga_dw_mmc.c  |  1 -
 drivers/mmc/stm32_sdmmc2.c|  4 +-
 drivers/mmc/uniphier-sd.c |  1 -
 drivers/mtd/nand/raw/atmel/nand-controller.c  |  4 +-
 drivers/mtd/renesas_rpc_hf.c  |  1 -
 drivers/net/bcm6348-eth.c |  2 -
 drivers/net/bcm6368-eth.c |  2 -
 drivers/net/designware.c  |  1 -
 drivers/net/dwc_eth_qos.c | 43 +++
 drivers/net/dwc_eth_qos_imx.c | 21 ++---
 drivers/net/dwc_eth_qos_qcom.c|  1 -
 drivers/net/dwc_eth_qos_rockchip.c|  6 +--
 drivers/net/sni_ave.c |  5 +--
 drivers/net/ti/am65-cpsw-nuss.c   |  1 -
 drivers/phy/bcm6318-usbh-phy.c|  2 -
 drivers/phy/bcm6348-usbh-phy.c|  2 -
 drivers/phy/bcm6368-usbh-phy.c|  4 --
 drivers/phy/meson-axg-mipi-dphy.c |  1 -
 drivers/phy/meson-g12a-usb3-pcie.c|  1 -
 drivers/phy/meson-gxl-usb2.c  |  1 -
 drivers/phy/phy-rcar-gen2.c   |  1 -
 drivers/phy/phy-rcar-gen3.c   |  1 -
 drivers/pinctrl/pinctrl-k210.c| 20 +++--
 drivers/power/domain/imx8mp-hsiomix.c |  4 +-
 drivers/rtc/stm32_rtc.c   | 16 ++-
 drivers/serial/atmel_usart.c  |  2 -
 drivers/serial/serial_bcm6345.c   |  1 -
 drivers/serial/serial_msm.c   |  1 -
 drivers/serial/serial_pic32.c |  1 -
 drivers/spi/atcspi200_spi.c   |  1 -
 drivers/spi/atmel-quadspi.c   | 14 ++
 drivers/spi/atmel_spi.c   |  2 -
 drivers/spi/bcm63xx_hsspi.c   |  4 --
 drivers/spi/bcm63xx_spi.c |  2 -
 drivers/spi/bcmbca_hsspi.c|  4 --
 drivers/spi/cadence_qspi.c|  1 -
 drivers/spi/designware_spi.c  |  5 ---
 drivers/spi/meson_spifc_a1.c  | 10 -
 drivers/spi/mvebu_a3700_spi.c | 10 -
 drivers/spi/spi-aspeed-smc.c  |  1 -
 drivers/spi/stm32_spi.c   | 19 ++--
 drivers/timer/dw-apb-timer.c  |  2 -
 drivers/timer/ostm_timer.c|  2 -
 drivers/usb/dwc3/dwc3-meson-g12a.c|  4 +-
 drivers/usb/dwc3/dwc3-meson-gxl.c |  4 +-
 drivers/usb/host/ehci-atmel.c |  8 +---
 drivers/usb/host/ohci-da8xx.c |  1 -
 drivers/usb/host/xhci-rcar.c  |  5 +--
 drivers/video/atmel_hlcdfb.c

[PATCH 3/3] clk: Document clk_ops return codes and behavior

[Sean Anderson]

Currently, clock consumers cannot take any programmatic action based on the
return code of a clock function. This is because there is no
standardization, and generally no way of separating e.g. "there was a major
problem setting the rate for this clock" which usually should not be
recovered from, from "this clock doesn't support setting its rate" or "this
clock doesn't support *this* rate" which could be absolutely fine depending
on the driver.

This commit aims to standardize the acceptable codes which may be returned
from clock operations. In general,

- ENOSYS should be returned when an operation is not supported for a
  particular clock.
- ENOENT may be returned if the clock ID is invalid. However, it is
  encouraged to move any checks to request() to reduce code duplication.
- EINVAL should be returned for logical errors only (such as requesting an
  invalid rate).

Each function has had specific guidance added for when to return each error
code. This is just guidance for now; most of the clock subsystem does not
yet conform to this standard. However, it is expected that new clock
drivers return these error codes.

Additionally, this commit adds expected behavior for each of the clock
operations. I believe these should be mostly straightforward and correspond
to existing behavior. I remember not understanding what the expected
invariants were for several clock functions, so hopefully this should help
out new driver authors. In the future, some of these invariants could be
checked via an optional config option.

Signed-off-by: Sean Anderson 
---

 include/clk-uclass.h | 113 +++
 1 file changed, 103 insertions(+), 10 deletions(-)

diff --git a/include/clk-uclass.h b/include/clk-uclass.h
index 4353b664800..8c07e723cff 100644
--- a/include/clk-uclass.h
+++ b/include/clk-uclass.h
@@ -56,11 +56,20 @@ struct clk_ops {
  * default implementation, which assumes #clock-cells = <1>, and that
  * the DT cell contains a simple integer clock ID.
  *
+ * This function should be a simple translation of @args into @clock->id and
+ * (optionally) @clock->data. All other processing, allocation, or error
+ * checking should take place in request().
+ *
  * At present, the clock API solely supports device-tree. If this
  * changes, other xxx_xlate() functions may be added to support those
  * other mechanisms.
  *
- * Return: 0 if OK, or a negative error code.
+ * Return:
+ * * 0 on success
+ * * -%EINVAL if @args does not have the correct format. For example, it could
+ *   have too many/few arguments.
+ * * -%ENOENT if @args has the correct format but cannot be translated. This 
can
+ *   happen if translation involves a table lookup and @args is not present.
  */
 int of_xlate(struct clk *clock, struct ofnode_phandle_args *args);
 
@@ -75,16 +84,36 @@ int of_xlate(struct clk *clock, struct ofnode_phandle_args 
*args);
  * xxx_xlate() call, or as the only step in implementing a client's
  * clk_request() call.
  *
- * Return: 0 if OK, or a negative error code.
+ * This is the right place to do bounds checking (rejecting invalid or
+ * unimplemented clocks), allocate resources, or perform other setup not done
+ * during driver probe(). Most clock drivers should allocate resources in their
+ * probe() function, but it is possible to lazily initialize something here.
+ *
+ * Return:
+ * * 0 on success
+ * * -%ENOENT, if there is no clock corresponding to @clock->id and
+ *   @clock->data.
  */
 int request(struct clk *clock);
 
 /**
  * round_rate() - Adjust a rate to the exact rate a clock can provide.
- * @clk:   The clock to manipulate.
- * @rate:  Desidered clock rate in Hz.
+ * @clk:   The clock to query.
+ * @rate:  Desired clock rate in Hz.
  *
- * Return: rounded rate in Hz, or -ve error code.
+ * This function returns a new rate which can be provided to set_rate(). This
+ * new rate should be the closest rate to @rate which can be set without
+ * rounding. The following pseudo-code should hold::
+ *
+ *   for all rate in range(ULONG_MAX):
+ * rounded = round_rate(clk, rate)
+ * new_rate = set_rate(clk, rate)
+ * assert(IS_ERR_VALUE(new_rate) || new_rate == rounded)
+ *
+ * Return:
+ * * The rounded rate in Hz on success
+ * * A negative error value from another API (such as clk_get_rate()). This
+ *   function must not return an error for any other reason.
  */
 ulong round_rate(struct clk *clk, ulong rate);
 
@@ -92,7 +121,22 @@ ulong round_rate(struct clk *clk, ulong rate);
  * get_rate() - Get current clock rate.
  * @clk:   The clock to query.
  *
- * Return: clock rate in Hz, or -ve error code
+ * This returns the current rate of a clock. If the clock is disabled, it
+ * returns the rate at which the clock would run if it was enabled. The
+ * following pseudo-code should hold::
+ *
+ *   disable(clk)
+ *   rate = get_rate(clk)
+ *   enable(clk)
+ *   assert(get_rate(clk) == rate)
+ *
+

[PATCH 0/3] clk: Remove clk_free and better document clk_ops

[Sean Anderson]

This series contains two unrelated changes. They are included together because
they touch the same files and would otherwise conflict. The first change is to
completely remove clk_free. This is because the op it calls (rfree) is
completely unused. I believe the original intent of this op was to allow clock
drivers to free resources allocated in request. However, no clock drivers do
this, so we can remove the function and all its callers.

The second change is to document the expected behavior and return codes of all
clock functions. At the moment, drivers are very inconsistent. I would like to
have some easily-verified expected behavior we can use when reviewing clock
drivers. Existing code is not fixed, but I hope to make some conversions in the
future.


Sean Anderson (3):
  clk: Remove rfree
  treewide: Remove clk_free
  clk: Document clk_ops return codes and behavior

 arch/arm/mach-rockchip/rk3288/rk3288.c|   2 -
 arch/arm/mach-socfpga/clock_manager_agilex.c  |   2 -
 arch/arm/mach-socfpga/clock_manager_arria10.c |   7 +-
 arch/arm/mach-socfpga/clock_manager_n5x.c |   2 -
 arch/arm/mach-zynq/clk.c  |   2 -
 arch/mips/mach-pic32/cpu.c|   7 +-
 arch/sandbox/include/asm/clk.h|   8 --
 board/microchip/pic32mzda/pic32mzda.c |   2 -
 board/sipeed/maix/maix.c  |   1 -
 board/synopsys/hsdk/clk-lib.c |   2 -
 drivers/clk/aspeed/clk_ast2600.c  |   2 -
 drivers/clk/at91/compat.c |  14 +-
 drivers/clk/clk-uclass.c  |  40 +-
 drivers/clk/clk-xlnx-clock-wizard.c   |   1 -
 drivers/clk/clk_sandbox.c |  12 --
 drivers/clk/clk_sandbox_test.c|  12 --
 drivers/clk/clk_versaclock.c  |  12 +-
 drivers/clk/clk_zynq.c|   2 -
 drivers/clk/clk_zynqmp.c  |   2 -
 drivers/clk/imx/clk-imx8.c|   2 -
 drivers/clk/mvebu/armada-37xx-periph.c|   2 -
 drivers/cpu/riscv_cpu.c   |   2 -
 drivers/dma/bcm6348-iudma.c   |   2 -
 drivers/gpio/at91_gpio.c  |   2 -
 drivers/gpio/atmel_pio4.c |   2 -
 drivers/gpio/gpio-rcar.c  |   1 -
 drivers/hwspinlock/stm32_hwspinlock.c |   6 +-
 drivers/i2c/at91_i2c.c|   2 -
 drivers/i2c/designware_i2c.c  |   2 -
 drivers/i2c/i2c-microchip.c   |   2 -
 drivers/i2c/npcm_i2c.c|   1 -
 drivers/i2c/ocores_i2c.c  |   2 -
 drivers/i2c/stm32f7_i2c.c |   4 +-
 drivers/mailbox/stm32-ipcc.c  |   7 +-
 drivers/misc/ls2_sfp.c|   1 -
 drivers/mmc/arm_pl180_mmci.c  |   1 -
 drivers/mmc/aspeed_sdhci.c|   4 +-
 drivers/mmc/atmel_sdhci.c |   2 -
 drivers/mmc/gen_atmel_mci.c   |  19 +--
 drivers/mmc/msm_sdhci.c   |   1 -
 drivers/mmc/pic32_sdhci.c |   1 -
 drivers/mmc/renesas-sdhi.c|  21 +--
 drivers/mmc/snps_dw_mmc.c |   8 +-
 drivers/mmc/socfpga_dw_mmc.c  |   1 -
 drivers/mmc/stm32_sdmmc2.c|   4 +-
 drivers/mmc/uniphier-sd.c |   1 -
 drivers/mtd/nand/raw/atmel/nand-controller.c  |   4 +-
 drivers/mtd/renesas_rpc_hf.c  |   1 -
 drivers/net/bcm6348-eth.c |   2 -
 drivers/net/bcm6368-eth.c |   2 -
 drivers/net/designware.c  |   1 -
 drivers/net/dwc_eth_qos.c |  43 +-
 drivers/net/dwc_eth_qos_imx.c |  21 +--
 drivers/net/dwc_eth_qos_qcom.c|   1 -
 drivers/net/dwc_eth_qos_rockchip.c|   6 +-
 drivers/net/sni_ave.c |   5 +-
 drivers/net/ti/am65-cpsw-nuss.c   |   1 -
 drivers/phy/bcm6318-usbh-phy.c|   2 -
 drivers/phy/bcm6348-usbh-phy.c|   2 -
 drivers/phy/bcm6368-usbh-phy.c|   4 -
 drivers/phy/meson-axg-mipi-dphy.c |   1 -
 drivers/phy/meson-g12a-usb3-pcie.c|   1 -
 drivers/phy/meson-gxl-usb2.c  |   1 -
 drivers/phy/phy-rcar-gen2.c   |   1 -
 drivers/phy/phy-rcar-gen3.c   |   1 -
 drivers/pinctrl/pinctrl-k210.c|  20 +--
 drivers/power/domain/imx8mp-hsiomix.c |   4 +-
 drivers/rtc/stm32_rtc.c   |  16 +--
 drivers/serial/atmel_usart.c  |   2 -
 drivers/serial/serial_bcm6345.c   |   1 -
 drivers/serial/serial_msm.c   |   1 -
 drivers/serial/serial_pic32.c |   1 -
 drivers/spi/atcspi200_spi.c   |   1 -
 drivers/spi/atmel-quadspi.c   |  14

Re: [PATCH v2 1/1] clk: clk-gpio: add actual gated clock

[Sean Anderson]

On 12/16/23 11:52, Marek Vasut wrote:

On 12/16/23 16:37, Sean Anderson wrote:

On 12/16/23 03:48, Svyatoslav Ryhel wrote:

Existing gpio-gate-clock driver acts like a simple GPIO switch without any
effect on gated clock. Add actual clock actions into enable/disable ops and
implement get_rate op by passing gated clock if it is enabled.

Signed-off-by: Svyatoslav Ryhel 
---
  drivers/clk/clk-gpio.c | 44 ++
  1 file changed, 36 insertions(+), 8 deletions(-)

diff --git a/drivers/clk/clk-gpio.c b/drivers/clk/clk-gpio.c
index 26d795b978..72d9747a47 100644
--- a/drivers/clk/clk-gpio.c
+++ b/drivers/clk/clk-gpio.c
@@ -3,19 +3,23 @@
   * Copyright (C) 2023 Marek Vasut 
   */
-#include 
-#include 
-#include 
+#include 
  #include 
+#include 
+#include 
+
+#include 
  struct clk_gpio_priv {
-    struct gpio_desc    enable;
+    struct gpio_desc    enable;    /* GPIO, controlling the gate */
+    struct clk    *clk;    /* Gated clock */
  };
  static int clk_gpio_enable(struct clk *clk)
  {
  struct clk_gpio_priv *priv = dev_get_priv(clk->dev);
+    clk_enable(priv->clk);
  dm_gpio_set_value(>enable, 1);
  return 0;
@@ -26,21 +30,45 @@ static int clk_gpio_disable(struct clk *clk)
  struct clk_gpio_priv *priv = dev_get_priv(clk->dev);
  dm_gpio_set_value(>enable, 0);
+    clk_disable(priv->clk);
  return 0;
  }
+static ulong clk_gpio_get_rate(struct clk *clk)
+{
+    struct clk_gpio_priv *priv = dev_get_priv(clk->dev);
+
+    return clk_get_rate(priv->clk);
+}
+
  const struct clk_ops clk_gpio_ops = {
  .enable    = clk_gpio_enable,
  .disable    = clk_gpio_disable,
+    .get_rate    = clk_gpio_get_rate,
  };
-static int clk_gpio_probe(struct udevice *dev)
+static int clk_gpio_of_to_plat(struct udevice *dev)
  {
  struct clk_gpio_priv *priv = dev_get_priv(dev);
+    int ret;
-    return gpio_request_by_name(dev, "enable-gpios", 0,
-    >enable, GPIOD_IS_OUT);
+    priv->clk = devm_clk_get(dev, NULL);
+    if (IS_ERR(priv->clk)) {
+    log_debug("%s: Could not get gated clock: %ld\n",
+  __func__, PTR_ERR(priv->clk));
+    return PTR_ERR(priv->clk);
+    }
+
+    ret = gpio_request_by_name(dev, "enable-gpios", 0,
+   >enable, GPIOD_IS_OUT);
+    if (ret) {
+    log_debug("%s: Could not decode enable-gpios (%d)\n",
+  __func__, ret);
+    return ret;
+    }
+
+    return 0;


All this forwarding of clock enable/disable/get_rate for parent clock should 
already be done in clock core. If it is not, then it should be fixed in clock 
core.


Only CCF forwards things. It's up to the driver to do it in the non-CCF case.

--Sean

Re: [PATCH v2 1/1] clk: clk-gpio: add actual gated clock

[Sean Anderson]

On 12/16/23 03:48, Svyatoslav Ryhel wrote:

Existing gpio-gate-clock driver acts like a simple GPIO switch without any
effect on gated clock. Add actual clock actions into enable/disable ops and
implement get_rate op by passing gated clock if it is enabled.

Signed-off-by: Svyatoslav Ryhel 
---
  drivers/clk/clk-gpio.c | 44 ++
  1 file changed, 36 insertions(+), 8 deletions(-)

diff --git a/drivers/clk/clk-gpio.c b/drivers/clk/clk-gpio.c
index 26d795b978..72d9747a47 100644
--- a/drivers/clk/clk-gpio.c
+++ b/drivers/clk/clk-gpio.c
@@ -3,19 +3,23 @@
   * Copyright (C) 2023 Marek Vasut 
   */
  
-#include 

-#include 
-#include 
+#include 
  #include 
+#include 
+#include 
+
+#include 
  
  struct clk_gpio_priv {

-   struct gpio_descenable;
+   struct gpio_descenable; /* GPIO, controlling the gate */
+   struct clk  *clk;   /* Gated clock */
  };
  
  static int clk_gpio_enable(struct clk *clk)

  {
struct clk_gpio_priv *priv = dev_get_priv(clk->dev);
  
+	clk_enable(priv->clk);

dm_gpio_set_value(>enable, 1);
  
  	return 0;

@@ -26,21 +30,45 @@ static int clk_gpio_disable(struct clk *clk)
struct clk_gpio_priv *priv = dev_get_priv(clk->dev);
  
  	dm_gpio_set_value(>enable, 0);

+   clk_disable(priv->clk);
  
  	return 0;

  }
  
+static ulong clk_gpio_get_rate(struct clk *clk)

+{
+   struct clk_gpio_priv *priv = dev_get_priv(clk->dev);
+
+   return clk_get_rate(priv->clk);
+}
+
  const struct clk_ops clk_gpio_ops = {
.enable = clk_gpio_enable,
.disable= clk_gpio_disable,
+   .get_rate   = clk_gpio_get_rate,
  };
  
-static int clk_gpio_probe(struct udevice *dev)

+static int clk_gpio_of_to_plat(struct udevice *dev)
  {
struct clk_gpio_priv *priv = dev_get_priv(dev);
+   int ret;
  
-	return gpio_request_by_name(dev, "enable-gpios", 0,

-   >enable, GPIOD_IS_OUT);
+   priv->clk = devm_clk_get(dev, NULL);
+   if (IS_ERR(priv->clk)) {
+   log_debug("%s: Could not get gated clock: %ld\n",
+ __func__, PTR_ERR(priv->clk));
+   return PTR_ERR(priv->clk);
+   }
+
+   ret = gpio_request_by_name(dev, "enable-gpios", 0,
+  >enable, GPIOD_IS_OUT);
+   if (ret) {
+   log_debug("%s: Could not decode enable-gpios (%d)\n",
+ __func__, ret);
+   return ret;
+   }
+
+   return 0;
  }
  
  /*

@@ -59,7 +87,7 @@ U_BOOT_DRIVER(gpio_gate_clock) = {
.name   = "gpio_clock",
.id = UCLASS_CLK,
.of_match   = clk_gpio_match,
-   .probe  = clk_gpio_probe,
+   .of_to_plat = clk_gpio_of_to_plat,
.priv_auto  = sizeof(struct clk_gpio_priv),
.ops= _gpio_ops,
.flags  = DM_FLAG_PRE_RELOC,


+CC Marek

[GIT PULL] clock changes for u-boot/master

[Sean Anderson]

The following changes since commit 3ac22891cfc0dc6d8eec25d2b0fbdd2eb8f3d3ed:

  Merge tag 'u-boot-imx-20231214' of 
https://gitlab.denx.de/u-boot/custodians/u-boot-imx (2023-12-15 08:22:31 -0500)

are available in the Git repository at:

  https://source.denx.de/u-boot/custodians/u-boot-clk.git tags/clk-2024.01-rc5

for you to fetch changes up to 97d65b32d76cb3b8297cd8aa2c1f0caab5ab6c57:

  test: dm: clk_ccf: fix building error (2023-12-15 15:30:12 -0500)


clock changes for u-boot/master

This has some clock fixes which should go in before the release. It's a bit
late in the cycle, but most of these have tests to go along with them.

Signed-off-by: Sean Anderson 


Igor Prusov (2):
  clk: Check that composite clock's div has set_rate()
  dm: test: clk: Add test for ccf clk_set_rate()

Yang Xiwen (4):
  clk: check parent_name in clk_register to avoid confusing log_error() 
output
  clk: get correct ops for clk_enable() and clk_disable()
  test: dm: clk_ccf: test ccf_clk_ops
  test: dm: clk_ccf: fix building error

 arch/sandbox/dts/test.dts  |  4 +++-
 arch/sandbox/include/asm/clk.h |  1 +
 drivers/clk/clk-composite.c|  2 +-
 drivers/clk/clk-uclass.c   |  2 ++
 drivers/clk/clk.c  | 18 ++
 drivers/clk/clk_sandbox_ccf.c  |  1 +
 drivers/clk/clk_sandbox_test.c |  1 +
 test/dm/clk_ccf.c  | 22 --
 8 files changed, 39 insertions(+), 12 deletions(-)

Re: (subset) [PATCH v3 2/2] test: dm: clk_ccf: test ccf_clk_ops

[Sean Anderson]

On Sat, 16 Dec 2023 02:28:52 +0800, Yang Xiwen wrote:
> Assign ccf_clk_ops to .ops of clk_ccf driver so that it can act as an
> clk provider. Also add "#clock-cells=<1>" to its device tree node.
> 
> Add "i2c_root" to clk_test in the device tree and driver for testing.
> 
> Get "i2c_root" clock in CCF unit tests and add tests for it.
> 
> [...]

Applied, thanks!

[2/2] test: dm: clk_ccf: test ccf_clk_ops
  https://source.denx.de/u-boot/custodians/u-boot-clk/-/commit/d30618243990

Best regards,
-- 
Sean Anderson 

Re: [PATCH] test: dm: clk_ccf: fix building error

[Sean Anderson]

On Sat, 16 Dec 2023 04:21:11 +0800, Yang Xiwen wrote:
> Fix unused variable error produced by building tests
> 
> 

Applied, thanks!

[1/1] test: dm: clk_ccf: fix building error
  https://source.denx.de/u-boot/custodians/u-boot-clk/-/commit/97d65b32d76c

Best regards,
-- 
Sean Anderson 

[GIT PULL] clock changes for u-boot/next

[Sean Anderson]

The following changes since commit fa3f19aa56c519d6345cc774187b7a8fdc053d71:

  Merge tag 'xilinx-for-v2024.04-rc1' of 
https://source.denx.de/u-boot/custodians/u-boot-microblaze into next 
(2023-12-14 13:27:11 -0500)

are available in the Git repository at:

  https://source.denx.de/u-boot/custodians/u-boot-clk.git tags/clk-2024.01-next

for you to fetch changes up to 652d8d4561a34fc71d9bcb6ad2b0035d2c4189c1:

  clk: nuvoton: add read only feature for clk driver (2023-12-15 13:05:55 -0500)


clock patches for u-boot/next

The main thing in here is Igor's conversion of soc_clk_dump to a clk_ops
member. There's also a write-protect feature for nuvoton clocks.

Signed-off-by: Sean Anderson 


Igor Prusov (8):
  clk: zynq: Move soc_clk_dump to Zynq clock driver
  clk: ast2600: Move soc_clk_dump function
  clk: k210: Move soc_clk_dump function
  clk: amlogic: Move driver and ops structs
  clk: Add dump operation to clk_ops
  cmd: clk: Use dump function from clk_ops
  clk: treewide: switch to clock dump from clk_ops
  cmd: clk: Make soc_clk_dump static

Jim Liu (1):
  clk: nuvoton: add read only feature for clk driver

 arch/arm/mach-zynq/clk.c   |  57 
---
 arch/mips/mach-pic32/cpu.c |  23 -
 cmd/clk.c  |  13 ++--
 drivers/clk/aspeed/clk_ast2600.c   |  83 
+-
 drivers/clk/clk_k210.c | 104 
+
 drivers/clk/clk_pic32.c|  37 +
 drivers/clk/clk_versal.c   |   9 +---
 drivers/clk/clk_zynq.c |  52 
+++
 drivers/clk/clk_zynqmp.c   |  22 ++--
 drivers/clk/imx/clk-imx8.c |  13 
 drivers/clk/meson/a1.c |  58 
+---
 drivers/clk/mvebu/armada-37xx-periph.c |  20 +++---
 drivers/clk/nuvoton/clk_npcm.c |  15 +++---
 drivers/clk/nuvoton/clk_npcm.h |   1 +
 drivers/clk/nuvoton/clk_npcm8xx.c  |  12 +--
 drivers/clk/stm32/clk-stm32mp1.c   |  31 
 include/clk-uclass.h   |  13 
 include/clk.h  |   2 --
 18 files changed, 288 insertions(+), 277 deletions(-)

Re: [PATCH v7 5/8] clk: Add dump operation to clk_ops

[Sean Anderson]

On 11/9/23 05:55, Igor Prusov wrote:

This adds dump function to struct clk_ops which should replace
soc_clk_dump. It allows clock drivers to provide custom dump
implementation without overriding generic CCF dump function.

Reviewed-by: Patrice Chotard 
Tested-by: Patrice Chotard 
Reviewed-by: Sean Anderson 
Signed-off-by: Igor Prusov 
---
  include/clk-uclass.h | 13 +
  1 file changed, 13 insertions(+)

diff --git a/include/clk-uclass.h b/include/clk-uclass.h
index a22f1a5d84..f10dd213ff 100644
--- a/include/clk-uclass.h
+++ b/include/clk-uclass.h
@@ -25,6 +25,7 @@ struct ofnode_phandle_args;
   * @set_parent: Set current clock parent
   * @enable: Enable a clock.
   * @disable: Disable a clock.
+ * @dump: Print clock information.
   *
   * The individual methods are described more fully below.
   */
@@ -39,6 +40,9 @@ struct clk_ops {
int (*set_parent)(struct clk *clk, struct clk *parent);
int (*enable)(struct clk *clk);
int (*disable)(struct clk *clk);
+#if IS_ENABLED(CONFIG_CMD_CLK)
+   void (*dump)(struct udevice *dev);
+#endif
  };
  
  #if 0 /* For documentation only */

@@ -135,6 +139,15 @@ int enable(struct clk *clk);
   * Return: zero on success, or -ve error code.
   */
  int disable(struct clk *clk);
+
+/**
+ * dump() - Print clock information.
+ * @clk:   The clock device to dump.


The correct member here is @dev. I fixed this when applying.

--Sean


+ * If present, this function is called by "clk dump" command for each
+ * bound device.
+ */
+void dump(struct udevice *dev);
  #endif
  
  #endif

Re: [PATCH v3] clk: nuvoton: add read only feature for clk driver

[Sean Anderson]

On Tue, 14 Nov 2023 17:00:04 +0800, Jim Liu wrote:
> Add a flag to set ahb/apb/fiu/spi clock divider as read-only
> The spi clock setting is related to booting flash, it is setup by early
> bootloader.
> It just protects the clock source and can't modify it in uboot.
> 
> 

Applied, thanks!

[1/1] clk: nuvoton: add read only feature for clk driver
  https://source.denx.de/u-boot/custodians/u-boot-clk/-/commit/652d8d4561a3

Best regards,
-- 
Sean Anderson 

Re: [PATCH v7 0/8] clk: Switch from soc_clk_dump to clk_ops function

[Sean Anderson]

On Thu, 9 Nov 2023 13:55:08 +0300, Igor Prusov wrote:
> Currently clock providers may override default implementation of
> soc_clk_dump function to replace clk dump command output. This causes
> confusing behaviour when u-boot is built with one of such drivers
> enabled but still has clocks defined using CCF. For example, enabling
> CMD_CLK and using clk dump on sandbox target will not show CCF clocks
> because k210 driver overrides common soc_clk_dump.
> 
> [...]

Applied, thanks!

[1/8] clk: zynq: Move soc_clk_dump to Zynq clock driver
  https://source.denx.de/u-boot/custodians/u-boot-clk/-/commit/bdac75511411
[2/8] clk: ast2600: Move soc_clk_dump function
  https://source.denx.de/u-boot/custodians/u-boot-clk/-/commit/3f192541df79
[3/8] clk: k210: Move soc_clk_dump function
  https://source.denx.de/u-boot/custodians/u-boot-clk/-/commit/77beaad3d1d2
[4/8] clk: amlogic: Move driver and ops structs
  https://source.denx.de/u-boot/custodians/u-boot-clk/-/commit/4f48202ba82e
[5/8] clk: Add dump operation to clk_ops
  https://source.denx.de/u-boot/custodians/u-boot-clk/-/commit/505ef5f627d8
[6/8] cmd: clk: Use dump function from clk_ops
  https://source.denx.de/u-boot/custodians/u-boot-clk/-/commit/258c1002383e
[7/8] clk: treewide: switch to clock dump from clk_ops
  https://source.denx.de/u-boot/custodians/u-boot-clk/-/commit/bc3e313ff6af
[8/8] cmd: clk: Make soc_clk_dump static
  https://source.denx.de/u-boot/custodians/u-boot-clk/-/commit/5666558a6cb0

Best regards,
-- 
Sean Anderson 

Re: [PATCH] test: dm: clk_ccf: fix building error

[Sean Anderson]

On 12/15/23 15:21, Yang Xiwen via B4 Relay wrote:

From: Yang Xiwen 

Fix unused variable error produced by building tests

Fixes: d3061824 (test: dm: clk_ccf: test ccf_clk_ops)
Signed-off-by: Yang Xiwen 
---
it's detected by u-boot gitlab CI.
---
  test/dm/clk_ccf.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/test/dm/clk_ccf.c b/test/dm/clk_ccf.c
index b8be6d6572..5a3c58c6b5 100644
--- a/test/dm/clk_ccf.c
+++ b/test/dm/clk_ccf.c
@@ -18,11 +18,12 @@
  /* Tests for Common Clock Framework driver */
  static int dm_test_clk_ccf(struct unit_test_state *uts)
  {
-   struct clk *clk, *pclk, clk_ccf;
+   struct clk *clk, *pclk;
struct udevice *dev, *test_dev;
long long rate;
int ret;
  #if CONFIG_IS_ENABLED(CLK_CCF)
+   struct clk clk_ccf;
const char *clkname;
int clkid, i;
  #endif

---
base-commit: c9945276f77921feb8df7be75cced3338d08e8d4
change-id: 20231216-b4-fix_build-d05f2b26c8a7

Best regards,


Reviewed-by: Sean Anderson 

Re: [PATCH v1 1/1] clk: clk-gpio: add actual gated clock

[Sean Anderson]

On 12/15/23 15:00, Svyatoslav Ryhel wrote:

пт, 15 груд. 2023 р. о 20:45 Sean Anderson  пише:


On 12/15/23 13:26, Svyatoslav Ryhel wrote:

пт, 15 груд. 2023 р. о 19:25 Sean Anderson  пише:


On 11/17/23 02:52, Svyatoslav Ryhel wrote:

Existing gpio-gate-clock driver acts like a simple GPIO switch without any
effect on gated clock. Add actual clock actions into enable/disable ops and
implement get_rate op by passing gated clock if it is enabled.

Signed-off-by: Svyatoslav Ryhel 
---
drivers/clk/clk-gpio.c | 47 +++---
1 file changed, 40 insertions(+), 7 deletions(-)

diff --git a/drivers/clk/clk-gpio.c b/drivers/clk/clk-gpio.c
index 26d795b978..a55470cfbf 100644
--- a/drivers/clk/clk-gpio.c
+++ b/drivers/clk/clk-gpio.c
@@ -3,19 +3,23 @@
 * Copyright (C) 2023 Marek Vasut 
 */

-#include 
-#include 
-#include 
+#include 
#include 
+#include 
+#include 
+
+#include 

struct clk_gpio_priv {
struct gpio_descenable;
+ struct clk  *clk;


Please name this "parent".


I see no need in this since clk is generic name and this driver can
accept only one clock with any name.


It's good documentation. It makes it clear what the purpose of the member is.


This driver uses a single (1) clock with a single purpose.

Why do you think you need a clock in a clock-gpio driver? Hm, I don't
know. This will remain a mystery for future archeo-coders yeah.


If you don't want to change the name, then add a doc comment.


};

static int clk_gpio_enable(struct clk *clk)
{
struct clk_gpio_priv *priv = dev_get_priv(clk->dev);

+ clk_prepare_enable(priv->clk);


Just clk_enable is fine. We don't have a separate prepare stage line Linux.


U-Boot supports clk_prepare_enable and since this support is present I
see no contradiction in using it.


These functions are only there for to make it easier to port code from Linux.
Since this code was written for U-Boot, just use the actual function.


Then document that its use is discouraged. Until then it is not restricted.


There are many functions like this in U-Boot (e.g. kmalloc which takes an unused
flags argument, or all the pr_* functions which are almost-but-not-quite the
same as log_*). Not all of them are explicitly document as such, but generally
you should use the native functions where it is not inconvenient.

--Sean

Re: [PATCH v3 2/2] test: dm: clk_ccf: test ccf_clk_ops

[Sean Anderson]

On 12/15/23 13:28, Yang Xiwen via B4 Relay wrote:

From: Yang Xiwen 

Assign ccf_clk_ops to .ops of clk_ccf driver so that it can act as an
clk provider. Also add "#clock-cells=<1>" to its device tree node.

Add "i2c_root" to clk_test in the device tree and driver for testing.

Get "i2c_root" clock in CCF unit tests and add tests for it.

Signed-off-by: Yang Xiwen 
---
  arch/sandbox/dts/test.dts  |  4 +++-
  arch/sandbox/include/asm/clk.h |  1 +
  drivers/clk/clk_sandbox_ccf.c  |  1 +
  drivers/clk/clk_sandbox_test.c |  1 +
  test/dm/clk_ccf.c  | 14 +++---
  5 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
index c7197795ef..a3a865d65c 100644
--- a/arch/sandbox/dts/test.dts
+++ b/arch/sandbox/dts/test.dts
@@ -631,9 +631,10 @@
clocks = <_fixed>,
 <_sandbox 1>,
 <_sandbox 0>,
+< 11>,
 <_sandbox 3>,
 <_sandbox 2>;
-   clock-names = "fixed", "i2c", "spi", "uart2", "uart1";
+   clock-names = "fixed", "i2c", "spi", "i2c_root", "uart2", 
"uart1";
};
  
  	clk-test2 {

@@ -654,6 +655,7 @@
  
  	ccf: clk-ccf {

compatible = "sandbox,clk-ccf";
+   #clock-cells = <1>;
};
  
  	efi-media {

diff --git a/arch/sandbox/include/asm/clk.h b/arch/sandbox/include/asm/clk.h
index df7156fe31..1daf2e7ac7 100644
--- a/arch/sandbox/include/asm/clk.h
+++ b/arch/sandbox/include/asm/clk.h
@@ -39,6 +39,7 @@ enum sandbox_clk_test_id {
SANDBOX_CLK_TEST_ID_FIXED,
SANDBOX_CLK_TEST_ID_SPI,
SANDBOX_CLK_TEST_ID_I2C,
+   SANDBOX_CLK_TEST_ID_I2C_ROOT,
SANDBOX_CLK_TEST_ID_DEVM1,
SANDBOX_CLK_TEST_ID_DEVM2,
SANDBOX_CLK_TEST_ID_DEVM_NULL,
diff --git a/drivers/clk/clk_sandbox_ccf.c b/drivers/clk/clk_sandbox_ccf.c
index fedcdd4044..38184e27aa 100644
--- a/drivers/clk/clk_sandbox_ccf.c
+++ b/drivers/clk/clk_sandbox_ccf.c
@@ -284,6 +284,7 @@ static int sandbox_clk_ccf_probe(struct udevice *dev)
  U_BOOT_DRIVER(sandbox_clk_ccf) = {
.name = "sandbox_clk_ccf",
.id = UCLASS_CLK,
+   .ops = _clk_ops,
.probe = sandbox_clk_ccf_probe,
.of_match = sandbox_clk_ccf_test_ids,
  };
diff --git a/drivers/clk/clk_sandbox_test.c b/drivers/clk/clk_sandbox_test.c
index 5807a454f3..c695b69321 100644
--- a/drivers/clk/clk_sandbox_test.c
+++ b/drivers/clk/clk_sandbox_test.c
@@ -15,6 +15,7 @@ static const char * const sandbox_clk_test_names[] = {
[SANDBOX_CLK_TEST_ID_FIXED] = "fixed",
[SANDBOX_CLK_TEST_ID_SPI] = "spi",
[SANDBOX_CLK_TEST_ID_I2C] = "i2c",
+   [SANDBOX_CLK_TEST_ID_I2C_ROOT] = "i2c_root",
  };
  
  int sandbox_clk_test_get(struct udevice *dev)

diff --git a/test/dm/clk_ccf.c b/test/dm/clk_ccf.c
index e4ebb93cda..b8be6d6572 100644
--- a/test/dm/clk_ccf.c
+++ b/test/dm/clk_ccf.c
@@ -18,8 +18,8 @@
  /* Tests for Common Clock Framework driver */
  static int dm_test_clk_ccf(struct unit_test_state *uts)
  {
-   struct clk *clk, *pclk;
-   struct udevice *dev;
+   struct clk *clk, *pclk, clk_ccf;
+   struct udevice *dev, *test_dev;
long long rate;
int ret;
  #if CONFIG_IS_ENABLED(CLK_CCF)
@@ -29,6 +29,7 @@ static int dm_test_clk_ccf(struct unit_test_state *uts)
  
  	/* Get the device using the clk device */

ut_assertok(uclass_get_device_by_name(UCLASS_CLK, "clk-ccf", ));
+   ut_assertok(uclass_get_device_by_name(UCLASS_MISC, "clk-test", 
_dev));
  
  	/* Test for clk_get_by_id() */

ret = clk_get_by_id(SANDBOX_CLK_ECSPI_ROOT, );
@@ -110,11 +111,18 @@ static int dm_test_clk_ccf(struct unit_test_state *uts)
  
  #if CONFIG_IS_ENABLED(CLK_CCF)

/* Test clk tree enable/disable */
+
+   ret = clk_get_by_index(test_dev, SANDBOX_CLK_TEST_ID_I2C_ROOT, 
_ccf);
+   ut_assertok(ret);
+   ut_asserteq_str("clk-ccf", clk_ccf.dev->name);
+   ut_asserteq(clk_ccf.id, SANDBOX_CLK_I2C_ROOT);
+
ret = clk_get_by_id(SANDBOX_CLK_I2C_ROOT, );
    ut_assertok(ret);
ut_asserteq_str("i2c_root", clk->dev->name);
+   ut_asserteq(clk->id, SANDBOX_CLK_I2C_ROOT);
  
-	ret = clk_enable(clk);

+   ret = clk_enable(_ccf);
ut_assertok(ret);
  
  	ret = sandbox_clk_enable_count(clk);




Reviewed-by: Sean Anderson 

Re: [PATCH v1 1/1] clk: clk-gpio: add actual gated clock

[Sean Anderson]

On 12/15/23 13:26, Svyatoslav Ryhel wrote:

пт, 15 груд. 2023 р. о 19:25 Sean Anderson  пише:


On 11/17/23 02:52, Svyatoslav Ryhel wrote:

Existing gpio-gate-clock driver acts like a simple GPIO switch without any
effect on gated clock. Add actual clock actions into enable/disable ops and
implement get_rate op by passing gated clock if it is enabled.

Signed-off-by: Svyatoslav Ryhel 
---
   drivers/clk/clk-gpio.c | 47 +++---
   1 file changed, 40 insertions(+), 7 deletions(-)

diff --git a/drivers/clk/clk-gpio.c b/drivers/clk/clk-gpio.c
index 26d795b978..a55470cfbf 100644
--- a/drivers/clk/clk-gpio.c
+++ b/drivers/clk/clk-gpio.c
@@ -3,19 +3,23 @@
* Copyright (C) 2023 Marek Vasut 
*/

-#include 
-#include 
-#include 
+#include 
   #include 
+#include 
+#include 
+
+#include 

   struct clk_gpio_priv {
   struct gpio_descenable;
+ struct clk  *clk;


Please name this "parent".


I see no need in this since clk is generic name and this driver can
accept only one clock with any name.


It's good documentation. It makes it clear what the purpose of the member is.


   };

   static int clk_gpio_enable(struct clk *clk)
   {
   struct clk_gpio_priv *priv = dev_get_priv(clk->dev);

+ clk_prepare_enable(priv->clk);


Just clk_enable is fine. We don't have a separate prepare stage line Linux.


U-Boot supports clk_prepare_enable and since this support is present I
see no contradiction in using it.


These functions are only there for to make it easier to port code from Linux.
Since this code was written for U-Boot, just use the actual function.


   dm_gpio_set_value(>enable, 1);

   return 0;
@@ -26,21 +30,50 @@ static int clk_gpio_disable(struct clk *clk)
   struct clk_gpio_priv *priv = dev_get_priv(clk->dev);

   dm_gpio_set_value(>enable, 0);
+ clk_disable_unprepare(priv->clk);

   return 0;
   }

+static ulong clk_gpio_get_rate(struct clk *clk)
+{
+ struct clk_gpio_priv *priv = dev_get_priv(clk->dev);
+ int ret;
+
+ ret = dm_gpio_get_value(>enable);
+ if (ret)
+ return clk_get_rate(priv->clk);
+ else
+ return -EINVAL;


This should always return the parent rate.


How can you return clock rate if the gate is disabled?


This allows consumers to know what the rate will be before they
enable the clock.


+}
+
   const struct clk_ops clk_gpio_ops = {
   .enable = clk_gpio_enable,
   .disable= clk_gpio_disable,
+ .get_rate   = clk_gpio_get_rate,
   };

-static int clk_gpio_probe(struct udevice *dev)
+static int clk_gpio_of_to_plat(struct udevice *dev)


This change should be a separate commit.


This actually should not be accepted first place

.of_to_plat - convert device tree data to plat
.probe - make a device ready for use

https://github.com/u-boot/u-boot/blob/master/doc/develop/driver-model/design.rst


Yes, this is why I was a bit confused :)


   {
   struct clk_gpio_priv *priv = dev_get_priv(dev);
+ int ret;

- return gpio_request_by_name(dev, "enable-gpios", 0,
- >enable, GPIOD_IS_OUT);
+ priv->clk = devm_clk_get(dev, NULL);
+ if (IS_ERR(priv->clk)) {
+ log_err("%s: Could not get gated clock: %ld\n",
+ __func__, PTR_ERR(priv->clk));
+ return PTR_ERR(priv->clk);
+ }
+
+ ret = gpio_request_by_name(dev, "enable-gpios", 0,
+>enable, GPIOD_IS_OUT);
+ if (ret) {
+ log_err("%s: Could not decode enable-gpios (%d)\n",
+ __func__, ret);


Use dev_dbg for probe errors to reduce binary size.


These errors provide intel about probing 2 key parts of the driver.
How would you know it fails in case you have no devices which face
this error? Iis there any convention or restriction which prohibits
this? If yes, then provide a document please.


https://docs.u-boot.org/en/latest/develop/driver-model/design.html#error-codes

| Ideally, messages in drivers should only be displayed when debugging,
| e.g. by using log_debug() although in extreme cases log_warning() or
| log_error() may be used.


All other general clock drivers use log_err/dev_err versions btw.


Yeah, it's something we have not been so consistent with enforcing.


+ return ret;
+ }
+
+ return 0;
   }

   /*
@@ -59,7 +92,7 @@ U_BOOT_DRIVER(gpio_gate_clock) = {
   .name   = "gpio_clock",
   .id = UCLASS_CLK,
   .of_match   = clk_gpio_match,
- .probe  = clk_gpio_probe,
+ .of_to_plat = clk_gpio_of_to_plat,
   .priv_auto  = sizeof(struct clk_gpio_priv),
   .ops= _gpio_ops,
   .flags  = DM_FLAG_PRE_RELOC,


--Sean

Re: [PATCH 0/2] Fix NULL dereference in clk_composite_set_rate()

[Sean Anderson]

On Wed, 6 Dec 2023 02:23:32 +0300, Igor Prusov wrote:
> On sandbox it's possible to trigger NULL dereference when setting rate
> of a composite clock. It happens because sandbox composite divider does
> not implement set_rate() operation. This series adds NULL check and a
> test cases for clk_set_rate().
> 
> 
> Igor Prusov (2):
>   clk: Check that composite clock's div has set_rate()
>   dm: test: clk: Add test for ccf clk_set_rate()
> 
> [...]

Applied, thanks!

[1/2] clk: Check that composite clock's div has set_rate()
  https://source.denx.de/u-boot/custodians/u-boot-clk/-/commit/54d7da773062
[2/2] dm: test: clk: Add test for ccf clk_set_rate()
  https://source.denx.de/u-boot/custodians/u-boot-clk/-/commit/9e0250321a0d

Best regards,
-- 
Sean Anderson 

Re: [PATCH v2] clk: check parent_name in clk_register to avoid confusing log_error() output

[Sean Anderson]

On Sat, 11 Nov 2023 03:19:52 +0800, Yang Xiwen wrote:
> For some gate clocks and fixed clocks without a parent, calling
> clk_register will print an useless error message indicating that parent
> is missing. Fix that by gaurding log_xxx() with an if-statement.
> 
> 

Applied, thanks!

[1/1] clk: check parent_name in clk_register to avoid confusing log_error() 
output
  https://source.denx.de/u-boot/custodians/u-boot-clk/-/commit/09844d0de555

Best regards,
-- 
Sean Anderson 

Re: (subset) [PATCH v2 2/4] clk: get correct ops for clk_enable() and clk_disable()

[Sean Anderson]

On Sun, 19 Nov 2023 06:10:06 +0800, Yang Xiwen wrote:
> assign clk_dev_ops(clkp->dev) to ops to ensure correct clk operations
> are called on clocks.
> 
> This fixes the incorrect enable_count issue as described in [1].
> 
> [1]: 
> https://lore.kernel.org/all/sezpr06mb695927a6deeef8489a06897396...@sezpr06mb6959.apcprd06.prod.outlook.com/
> 
> [...]

Applied, thanks!

[2/4] clk: get correct ops for clk_enable() and clk_disable()
  https://source.denx.de/u-boot/custodians/u-boot-clk/-/commit/3fb2d3d6acba

Best regards,
-- 
Sean Anderson 

Re: [PATCH v2] clk: meson: add Hardware Clock measure driver

[Sean Anderson]

 CLK_MSR_ID(43, "fclk_div5"),
+   CLK_MSR_ID(44, "pwm_b"),
+   CLK_MSR_ID(45, "pwm_a"),
+   CLK_MSR_ID(46, "vpu"),
+   CLK_MSR_ID(47, "ddr_dpll_pt"),
+   CLK_MSR_ID(48, "mp1_out"),
+   CLK_MSR_ID(49, "mp2_out"),
+   CLK_MSR_ID(50, "mp3_out"),
+   CLK_MSR_ID(51, "sd_emmc_c"),
+   CLK_MSR_ID(52, "sd_emmc_b"),
+   CLK_MSR_ID(53, "sd_emmc_a"),
+   CLK_MSR_ID(54, "vpu_clkc"),
+   CLK_MSR_ID(55, "vid_pll_div_out"),
+   CLK_MSR_ID(56, "wave420l_a"),
+   CLK_MSR_ID(57, "wave420l_c"),
+   CLK_MSR_ID(58, "wave420l_b"),
+   CLK_MSR_ID(59, "hcodec"),
+   CLK_MSR_ID(60, "arm_ring_osc_out_5"),
+   CLK_MSR_ID(61, "gpio_msr"),
+   CLK_MSR_ID(62, "hevcb"),
+   CLK_MSR_ID(63, "dsi_meas"),
+   CLK_MSR_ID(64, "spicc_1"),
+   CLK_MSR_ID(65, "spicc_0"),
+   CLK_MSR_ID(66, "vid_lock"),
+   CLK_MSR_ID(67, "dsi_phy"),
+   CLK_MSR_ID(68, "hdcp22_esm"),
+   CLK_MSR_ID(69, "hdcp22_skp"),
+   CLK_MSR_ID(70, "pwm_f"),
+   CLK_MSR_ID(71, "pwm_e"),
+   CLK_MSR_ID(72, "pwm_d"),
+   CLK_MSR_ID(73, "pwm_c"),
+   CLK_MSR_ID(74, "arm_ring_osc_out_6"),
+   CLK_MSR_ID(75, "hevcf"),
+   CLK_MSR_ID(76, "arm_ring_osc_out_7"),
+   CLK_MSR_ID(77, "rng_ring_osc_0"),
+   CLK_MSR_ID(78, "rng_ring_osc_1"),
+   CLK_MSR_ID(79, "rng_ring_osc_2"),
+   CLK_MSR_ID(80, "rng_ring_osc_3"),
+   CLK_MSR_ID(81, "vapb"),
+   CLK_MSR_ID(82, "ge2d"),
+   CLK_MSR_ID(83, "co_rx"),
+   CLK_MSR_ID(84, "co_tx"),
+   CLK_MSR_ID(85, "arm_ring_osc_out_8"),
+   CLK_MSR_ID(86, "arm_ring_osc_out_9"),
+   CLK_MSR_ID(87, "mipi_dsi_phy"),
+   CLK_MSR_ID(88, "cis2_adapt"),
+   CLK_MSR_ID(89, "hdmi_todig"),
+   CLK_MSR_ID(90, "hdmitx_sys"),
+   CLK_MSR_ID(91, "nna_core"),
+   CLK_MSR_ID(92, "nna_axi"),
+   CLK_MSR_ID(93, "vad"),
+   CLK_MSR_ID(94, "eth_phy_rx"),
+   CLK_MSR_ID(95, "eth_phy_pll"),
+   CLK_MSR_ID(96, "vpu_b"),
+   CLK_MSR_ID(97, "cpu_b_tmp"),
+   CLK_MSR_ID(98, "ts"),
+   CLK_MSR_ID(99, "arm_ring_osc_out_10"),
+   CLK_MSR_ID(100, "arm_ring_osc_out_11"),
+   CLK_MSR_ID(101, "arm_ring_osc_out_12"),
+   CLK_MSR_ID(102, "arm_ring_osc_out_13"),
+   CLK_MSR_ID(103, "arm_ring_osc_out_14"),
+   CLK_MSR_ID(104, "arm_ring_osc_out_15"),
+   CLK_MSR_ID(105, "arm_ring_osc_out_16"),
+   CLK_MSR_ID(106, "ephy_test"),
+   CLK_MSR_ID(107, "au_dac_g128x"),
+   CLK_MSR_ID(108, "audio_locker_out"),
+   CLK_MSR_ID(109, "audio_locker_in"),
+   CLK_MSR_ID(110, "audio_tdmout_c_sclk"),
+   CLK_MSR_ID(111, "audio_tdmout_b_sclk"),
+   CLK_MSR_ID(112, "audio_tdmout_a_sclk"),
+   CLK_MSR_ID(113, "audio_tdmin_lb_sclk"),
+   CLK_MSR_ID(114, "audio_tdmin_c_sclk"),
+   CLK_MSR_ID(115, "audio_tdmin_b_sclk"),
+   CLK_MSR_ID(116, "audio_tdmin_a_sclk"),
+   CLK_MSR_ID(117, "audio_resample"),
+   CLK_MSR_ID(118, "audio_pdm_sys"),
+   CLK_MSR_ID(119, "audio_spdifout_b"),
+   CLK_MSR_ID(120, "audio_spdifout"),
+   CLK_MSR_ID(121, "audio_spdifin"),
+   CLK_MSR_ID(122, "audio_pdm_dclk"),
+   CLK_MSR_ID(123, "audio_resampled"),
+   CLK_MSR_ID(124, "earcrx_pll"),
+   CLK_MSR_ID(125, "earcrx_pll_test"),
+   CLK_MSR_ID(126, "csi_phy0"),
+   CLK_MSR_ID(127, "csi2_data"),
+};
+
+static int meson_clk_msr_measure_id(struct meson_msr *priv, unsigned int id,
+   unsigned int duration)
+{
+   unsigned int val;
+   int ret;
+
+   regmap_write(priv->regmap, MSR_CLK_REG0, 0);
+
+   /* Set measurement duration */
+   regmap_update_bits(priv->regmap, MSR_CLK_REG0, MSR_DURATION,
+  FIELD_PREP(MSR_DURATION, duration - 1));
+
+   /* Set ID */
+   regmap_update_bits(priv->regmap, MSR_CLK_REG0, MSR_CLK_SRC,
+  FIELD_PREP(MSR_CLK_SRC, id));
+
+   /* Enable & Start */
+   regmap_update_bits(priv->regmap, MSR_CLK_REG0,
+  MSR_RUN | MSR_ENABLE,
+  MSR_RUN | MSR_ENABLE);
+
+   ret = regmap_read_poll_timeout(priv->regmap, MSR_CLK_REG0,
+  val, !(val & MSR_BUSY), 10, 1);
+   if (ret)
+   return ret;
+
+   /* Disable */
+   regmap_update_bits(priv->regmap, MSR_CLK_REG0, MSR_ENABLE, 0);
+
+   /* Get the value in multiple of gate time counts */
+   regmap_read(priv->regmap, MSR_CLK_REG2, );
+
+   if (val >= MSR_VAL_MASK)
+   return -EINVAL;
+
+   return DIV_ROUND_CLOSEST_ULL((val & MSR_VAL_MASK) * 100ULL,
+duration);
+}
+
+static int meson_clk_msr_best_id(struct meson_msr *priv, unsigned int id,
+unsigned int *precision)
+{
+   unsigned int duration = DIV_MAX;
+   int ret;
+
+   /* Start from max duration and down to min duration */
+   do {
+   ret = meson_clk_msr_measure_id(priv, id, duration);
+   if (ret >= 0)
+   *precision = (2 * 100) / duration;
+   else
+   duration -= DIV_STEP;
+   } while (duration >= DIV_MIN && ret == -EINVAL);
+
+   return ret;
+}
+
+static void meson_clk_msr_dump(struct udevice *dev)
+{
+   struct meson_msr *priv = dev_get_priv(dev);
+   unsigned int precision = 0;
+   int val, i;
+
+   printf("  clock rateprecision\n");
+   printf("-\n");
+
+   for (i = 0 ; i < CLK_MSR_MAX ; ++i) {
+   if (!priv->msr_table[i].name)
+   continue;
+
+   val = meson_clk_msr_best_id(priv, i, );
+   if (val < 0)
+   return;
+
+   printf(" %-20s %10d+/-%dHz\n",
+  priv->msr_table[i].name, val, precision);
+   }
+}


To confirm, this is based on [1], right?

[1] 
https://patchwork.ozlabs.org/project/uboot/cover/20231109105516.24892-1-ivpru...@sberdevices.ru/


+static int meson_clk_msr_xlate(struct clk *clk, struct ofnode_phandle_args 
*args)
+{
+   /* This driver doesn't expose any clocks */
+   return -EINVAL;
+}
+
+static int meson_clk_msr_probe(struct udevice *dev)
+{
+   struct meson_msr *priv = dev_get_priv(dev);
+   int ret;
+
+   priv->msr_table = (struct meson_msr_id *)dev_get_driver_data(dev);
+
+   ret = regmap_init_mem(dev_ofnode(dev), >regmap);
+   if (ret)
+   return ret;
+
+   return 0;
+}
+
+static struct clk_ops meson_clk_msr_ops = {
+   .of_xlate = meson_clk_msr_xlate,
+   .dump = meson_clk_msr_dump,
+};
+
+static const struct udevice_id meson_clk_msr_ids[] = {
+   {
+   .compatible = "amlogic,meson-gx-clk-measure",
+   .data = (ulong)clk_msr_gx,
+   },
+   {
+   .compatible = "amlogic,meson8-clk-measure",
+   .data = (ulong)clk_msr_m8,
+   },
+   {
+   .compatible = "amlogic,meson8b-clk-measure",
+   .data = (ulong)clk_msr_m8,
+   },
+   {
+   .compatible = "amlogic,meson-axg-clk-measure",
+   .data = (ulong)clk_msr_axg,
+   },
+   {
+   .compatible = "amlogic,meson-g12a-clk-measure",
+   .data = (ulong)clk_msr_g12a,
+   },
+   {
+   .compatible = "amlogic,meson-sm1-clk-measure",
+   .data = (ulong)clk_msr_sm1,
+   },
+   { /* sentinel */ }
+};
+
+U_BOOT_DRIVER(meson_clk_msr) = {
+   .name   = "meson_clk_msr",
+   .id = UCLASS_CLK,
+   .of_match   = meson_clk_msr_ids,
+   .priv_auto  = sizeof(struct meson_msr),
+   .ops= _clk_msr_ops,
+   .probe  = meson_clk_msr_probe,
+};

---
base-commit: e1b7717928eff69178604a400fe40dbf3287d9a1
change-id: 20231113-uboot-meson-clk-msr-21cf9101278b

Best regards,


With the commit/kconfig message expanded a bit,

Reviewed-by: Sean Anderson 

Re: [PATCH v6 06/17] arm: mach-k3: j784s4: Add clk and power support

[Sean Anderson]

ttps://www.ti.com/
+ */
+
+#include "k3-dev.h"
+
+static struct ti_psc soc_psc_list[] = {
+   [0] = PSC(0, 0x4200),
+   [1] = PSC(1, 0x0042),
+   [2] = PSC(2, 0x0040),
+};
+
+static struct ti_pd soc_pd_list[] = {
+   [0] = PSC_PD(0, _psc_list[0], NULL),
+   [1] = PSC_PD(3, _psc_list[1], NULL),
+   [2] = PSC_PD(0, _psc_list[2], NULL),
+   [3] = PSC_PD(1, _psc_list[2], _pd_list[2]),
+   [4] = PSC_PD(14, _psc_list[2], NULL),
+   [5] = PSC_PD(15, _psc_list[2], _pd_list[4]),
+   [6] = PSC_PD(16, _psc_list[2], _pd_list[4]),
+   [7] = PSC_PD(38, _psc_list[2], NULL),
+};
+
+static struct ti_lpsc soc_lpsc_list[] = {
+   [0] = PSC_LPSC(0, _psc_list[0], _pd_list[0], NULL),
+   [1] = PSC_LPSC(3, _psc_list[0], _pd_list[0], NULL),
+   [2] = PSC_LPSC(10, _psc_list[0], _pd_list[0], NULL),
+   [3] = PSC_LPSC(11, _psc_list[0], _pd_list[0], NULL),
+   [4] = PSC_LPSC(12, _psc_list[0], _pd_list[0], NULL),
+   [5] = PSC_LPSC(12, _psc_list[1], _pd_list[1], 
_lpsc_list[6]),
+   [6] = PSC_LPSC(13, _psc_list[1], _pd_list[1], NULL),
+   [7] = PSC_LPSC(0, _psc_list[2], _pd_list[2], NULL),
+   [8] = PSC_LPSC(9, _psc_list[2], _pd_list[2], _lpsc_list[2]),
+   [9] = PSC_LPSC(14, _psc_list[2], _pd_list[2], 
_lpsc_list[10]),
+   [10] = PSC_LPSC(15, _psc_list[2], _pd_list[2], NULL),
+   [11] = PSC_LPSC(16, _psc_list[2], _pd_list[2], 
_lpsc_list[12]),
+   [12] = PSC_LPSC(17, _psc_list[2], _pd_list[2], NULL),
+   [13] = PSC_LPSC(20, _psc_list[2], _pd_list[2], 
_lpsc_list[5]),
+   [14] = PSC_LPSC(23, _psc_list[2], _pd_list[2], 
_lpsc_list[5]),
+   [15] = PSC_LPSC(25, _psc_list[2], _pd_list[2], 
_lpsc_list[5]),
+   [16] = PSC_LPSC(43, _psc_list[2], _pd_list[3], NULL),
+   [17] = PSC_LPSC(45, _psc_list[2], _pd_list[3], NULL),
+   [18] = PSC_LPSC(78, _psc_list[2], _pd_list[4], NULL),
+   [19] = PSC_LPSC(80, _psc_list[2], _pd_list[5], 
_lpsc_list[18]),
+   [20] = PSC_LPSC(81, _psc_list[2], _pd_list[6], 
_lpsc_list[18]),
+   [21] = PSC_LPSC(120, _psc_list[2], _pd_list[7], 
_lpsc_list[22]),
+   [22] = PSC_LPSC(121, _psc_list[2], _pd_list[7], NULL),
+};
+
+static struct ti_dev soc_dev_list[] = {
+   PSC_DEV(35, _lpsc_list[0]),
+   PSC_DEV(160, _lpsc_list[0]),
+   PSC_DEV(161, _lpsc_list[0]),
+   PSC_DEV(162, _lpsc_list[0]),
+   PSC_DEV(243, _lpsc_list[0]),
+   PSC_DEV(149, _lpsc_list[0]),
+   PSC_DEV(167, _lpsc_list[1]),
+   PSC_DEV(279, _lpsc_list[1]),
+   PSC_DEV(161, _lpsc_list[2]),
+   PSC_DEV(162, _lpsc_list[3]),
+   PSC_DEV(160, _lpsc_list[4]),
+   PSC_DEV(139, _lpsc_list[5]),
+   PSC_DEV(194, _lpsc_list[6]),
+   PSC_DEV(78, _lpsc_list[7]),
+   PSC_DEV(61, _lpsc_list[8]),
+   PSC_DEV(131, _lpsc_list[9]),
+   PSC_DEV(191, _lpsc_list[10]),
+   PSC_DEV(132, _lpsc_list[11]),
+   PSC_DEV(192, _lpsc_list[12]),
+   PSC_DEV(398, _lpsc_list[13]),
+   PSC_DEV(141, _lpsc_list[14]),
+   PSC_DEV(140, _lpsc_list[15]),
+   PSC_DEV(146, _lpsc_list[16]),
+   PSC_DEV(392, _lpsc_list[17]),
+   PSC_DEV(395, _lpsc_list[17]),
+   PSC_DEV(198, _lpsc_list[18]),
+   PSC_DEV(202, _lpsc_list[19]),
+   PSC_DEV(203, _lpsc_list[20]),
+   PSC_DEV(133, _lpsc_list[21]),
+   PSC_DEV(193, _lpsc_list[22]),
+};
+
+const struct ti_k3_pd_platdata j784s4_pd_platdata = {
+   .psc = soc_psc_list,
+   .pd = soc_pd_list,
+   .lpsc = soc_lpsc_list,
+   .devs = soc_dev_list,
+   .num_psc = 3,
+   .num_pd = 8,
+   .num_lpsc = 23,
+   .num_devs = 30,
+};
diff --git a/drivers/clk/ti/clk-k3.c b/drivers/clk/ti/clk-k3.c
index eb76195bd7..10f6049797 100644
--- a/drivers/clk/ti/clk-k3.c
+++ b/drivers/clk/ti/clk-k3.c
@@ -86,6 +86,12 @@ static const struct soc_attr ti_k3_soc_clk_data[] = {
.family = "AM62AX",
.data = _clk_platdata,
},
+#endif
+#ifdef CONFIG_SOC_K3_J784S4
+   {
+   .family = "J784S4",
+   .data = _clk_platdata,
+   },
  #endif
{ /* sentinel */ }
  };
diff --git a/drivers/power/domain/ti-power-domain.c 
b/drivers/power/domain/ti-power-domain.c
index b34c982f4f..fb4ca2dd6b 100644
--- a/drivers/power/domain/ti-power-domain.c
+++ b/drivers/power/domain/ti-power-domain.c
@@ -98,6 +98,12 @@ static const struct soc_attr ti_k3_soc_pd_data[] = {
.family = "AM62AX",
.data = _pd_platdata,
},
+#endif
+#ifdef CONFIG_SOC_K3_J784S4
+   {
+   .family = "J784S4",
+   .data = _pd_platdata,
+   },
  #endif
{ /* sentinel */ }
  };
diff --git a/include/k3-clk.h b/include/k3-clk.h
index 1b6ab8fe65..e161f09c0f 100644
--- a/include/k3-clk.h
+++ b/include/k3-clk.h
@@ -176,6 +176,7 @@ extern const struct ti_k3_clk_platdata j7200_clk_platdata;
  extern const struct ti_k3_clk_platdata j721s2_clk_platdata;
  extern const struct ti_k3_clk_platdata am62x_clk_platdata;
  extern const struct ti_k3_clk_platdata am62ax_clk_platdata;
+extern const struct ti_k3_clk_platdata j784s4_clk_platdata;
  
  struct clk *clk_register_ti_pll(const char *name, const char *parent_name,

void __iomem *reg);
diff --git a/include/k3-dev.h b/include/k3-dev.h
index 072e10ba63..a9d1ada804 100644
--- a/include/k3-dev.h
+++ b/include/k3-dev.h
@@ -80,6 +80,7 @@ extern const struct ti_k3_pd_platdata j7200_pd_platdata;
  extern const struct ti_k3_pd_platdata j721s2_pd_platdata;
  extern const struct ti_k3_pd_platdata am62x_pd_platdata;
  extern const struct ti_k3_pd_platdata am62ax_pd_platdata;
+extern const struct ti_k3_pd_platdata j784s4_pd_platdata;
  
  u8 ti_pd_state(struct ti_pd *pd);

  u8 lpsc_get_state(struct ti_lpsc *lpsc);


Reviewed-by: Sean Anderson 

Re: [PATCH v1 1/1] clk: clk-gpio: add actual gated clock

[Sean Anderson]

On 11/17/23 02:52, Svyatoslav Ryhel wrote:

Existing gpio-gate-clock driver acts like a simple GPIO switch without any
effect on gated clock. Add actual clock actions into enable/disable ops and
implement get_rate op by passing gated clock if it is enabled.

Signed-off-by: Svyatoslav Ryhel 
---
  drivers/clk/clk-gpio.c | 47 +++---
  1 file changed, 40 insertions(+), 7 deletions(-)

diff --git a/drivers/clk/clk-gpio.c b/drivers/clk/clk-gpio.c
index 26d795b978..a55470cfbf 100644
--- a/drivers/clk/clk-gpio.c
+++ b/drivers/clk/clk-gpio.c
@@ -3,19 +3,23 @@
   * Copyright (C) 2023 Marek Vasut 
   */
  
-#include 

-#include 
-#include 
+#include 
  #include 
+#include 
+#include 
+
+#include 
  
  struct clk_gpio_priv {

struct gpio_descenable;
+   struct clk  *clk;


Please name this "parent".


  };
  
  static int clk_gpio_enable(struct clk *clk)

  {
struct clk_gpio_priv *priv = dev_get_priv(clk->dev);
  
+	clk_prepare_enable(priv->clk);


Just clk_enable is fine. We don't have a separate prepare stage line Linux.


dm_gpio_set_value(>enable, 1);
  
  	return 0;

@@ -26,21 +30,50 @@ static int clk_gpio_disable(struct clk *clk)
struct clk_gpio_priv *priv = dev_get_priv(clk->dev);
  
  	dm_gpio_set_value(>enable, 0);

+   clk_disable_unprepare(priv->clk);
  
  	return 0;

  }
  
+static ulong clk_gpio_get_rate(struct clk *clk)

+{
+   struct clk_gpio_priv *priv = dev_get_priv(clk->dev);
+   int ret;
+
+   ret = dm_gpio_get_value(>enable);
+   if (ret)
+   return clk_get_rate(priv->clk);
+   else
+   return -EINVAL;


This should always return the parent rate.


+}
+
  const struct clk_ops clk_gpio_ops = {
.enable = clk_gpio_enable,
.disable= clk_gpio_disable,
+   .get_rate   = clk_gpio_get_rate,
  };
  
-static int clk_gpio_probe(struct udevice *dev)

+static int clk_gpio_of_to_plat(struct udevice *dev)


This change should be a separate commit.


  {
struct clk_gpio_priv *priv = dev_get_priv(dev);
+   int ret;
  
-	return gpio_request_by_name(dev, "enable-gpios", 0,

-   >enable, GPIOD_IS_OUT);
+   priv->clk = devm_clk_get(dev, NULL);
+   if (IS_ERR(priv->clk)) {
+   log_err("%s: Could not get gated clock: %ld\n",
+   __func__, PTR_ERR(priv->clk));
+   return PTR_ERR(priv->clk);
+   }
+
+   ret = gpio_request_by_name(dev, "enable-gpios", 0,
+  >enable, GPIOD_IS_OUT);
+   if (ret) {
+   log_err("%s: Could not decode enable-gpios (%d)\n",
+   __func__, ret);


Use dev_dbg for probe errors to reduce binary size.


+   return ret;
+   }
+
+   return 0;
  }
  
  /*

@@ -59,7 +92,7 @@ U_BOOT_DRIVER(gpio_gate_clock) = {
.name   = "gpio_clock",
.id = UCLASS_CLK,
.of_match   = clk_gpio_match,
-   .probe  = clk_gpio_probe,
+   .of_to_plat = clk_gpio_of_to_plat,
.priv_auto  = sizeof(struct clk_gpio_priv),
.ops= _gpio_ops,
.flags  = DM_FLAG_PRE_RELOC,


--Sean