Re: Debian 12 al català

[Xavier Drudis Ferran]

El Wed, Apr 10, 2024 at 03:04:17PM +0200, Narcis Garcia deia:
> > 
> > He fet l'export $LANGUAGE=ca_ES i ara ho tinc així
> >

Sense $ però ja ho saps.

> > ➜ echo $LANGUAGE
> > es:ca:en_US
> > 
> > ➜ locale
> > LANG=ca_ES.UTF-8
> > LANGUAGE=es:ca:en_US
> > LC_CTYPE="ca_ES.UTF-8"
> > LC_NUMERIC="ca_ES.UTF-8"
> > LC_TIME="ca_ES.UTF-8"
> > LC_COLLATE="ca_ES.UTF-8"
> > LC_MONETARY="ca_ES.UTF-8"
> > LC_MESSAGES="ca_ES.UTF-8"
> > LC_PAPER="ca_ES.UTF-8"
> > LC_NAME="ca_ES.UTF-8"
> > LC_ADDRESS="ca_ES.UTF-8"
> > LC_TELEPHONE="ca_ES.UTF-8"
> > LC_MEASUREMENT="ca_ES.UTF-8"
> > LC_IDENTIFICATION="ca_ES.UTF-8"
> > LC_ALL=ca_ES.UTF-8
> > 
> > jordan@somtic ~
> > ❯ export LANGUAGE=ca:es:en_US
> > 
> > jordan@somtic ~
> > ➜ locale
> > LANG=ca_ES.UTF-8
> > LANGUAGE=ca:es:en_US
> > LC_CTYPE="ca_ES.UTF-8"
> > LC_NUMERIC="ca_ES.UTF-8"
> > LC_TIME="ca_ES.UTF-8"
> > LC_COLLATE="ca_ES.UTF-8"
> > LC_MONETARY="ca_ES.UTF-8"
> > LC_MESSAGES="ca_ES.UTF-8"
> > LC_PAPER="ca_ES.UTF-8"
> > LC_NAME="ca_ES.UTF-8"
> > LC_ADDRESS="ca_ES.UTF-8"
> > LC_TELEPHONE="ca_ES.UTF-8"
> > LC_MEASUREMENT="ca_ES.UTF-8"
> > LC_IDENTIFICATION="ca_ES.UTF-8"
> > LC_ALL=ca_ES.UTF-8
> > 
> > Però els missatges del terminal em continuen apareixent en castellà, pel
> > man, o per apt. Potser, no hi ha traducció al català per Debian 12?
> >

Jo tant amb LANGUAGE=ca:es:en_US
com amb LANGUAGE=

si faig
df
em surten les capçaleres en català.

si poso export LANGUAGE=es:ca:en_US
em surten en castellà.

Quan vas fer dpkg-reconfigure locales
ja vas triar ca_ES.UTF-8, oi ? O vas triar algun altre ca ?

> > No és greu, però ho volia per si feia captures de pantalla del terminal
> > per fer manuals que sortís en català.
> >

Igual dic una bestiesa.
Però tu fas servir coreutils o rust-coreutils ?
Jo faig servir coreutils.

Si miro strace df quan tinc el català configurat veig que llegeix

openat(AT_FDCWD, "/usr/share/locale/ca/LC_MESSAGES/coreutils.mo", O_RDONLY) = 3

i efectivament a
https://packages.debian.org/bookworm/arm64/coreutils/filelist
surt el fitxer de traduccions de coreutils
Però si miro
https://packages.debian.org/bookworm/arm64/rust-coreutils/filelist

no veig cap fitxer de traducció ni res a /usr/share/locale (per cap llengua)

Re: ConsultaInstal·lacióDEBIAN

[Xavier Drudis Ferran]

El Wed, Apr 03, 2024 at 11:53:06PM +0200, Daniel Abaurrea Ruiz deia:
> Els meus coneixements d’hardware i software son del nivell d’usuari. Sempre
> m’ha interessat molt saber com funciona tot el que m’envolta. Es per això
> que m’agradaria molt aprendre a instal·lar un sistema operatiu debian en un
> ordinador portàtil comprat sense que s’hi hagués instal·lat prèviament un
> sistema operatiu windows.
>

Però com és aquest ordinador ? tipus PC ? té un processador Intel o
AMD ?  O és un portàtil amb processador ARM ? o un altre cosa ?

Que no tingui Windows en principi hauria de fer-ho més
fàcil. t'estalvies barallar-t'hi si el vols mantenir i fer còpies de
seguretat o el que sigui. I si l'espifies no trenques gran cosa.

> He decidit posar aquest missatge en el vostre lloc web perquè he intentat
> aprendre a fer la instal·lació cercant cursos e informació per internet però
> m’he perdut en la quantitat d’informació que he trobat a la xarxa.
>

No sé, jo fa temps que no faig una instal·lació des de 0 en un PC portàtil,
però has provat començar per
https://www.debian.org/releases/stable/amd64/index.ca.html

Bàsicament primer has de baixar-te un instal·lador, enregistrar-lo en
algun suport (CD, DVD, memòria USB, etc). Això ho has de fer amb algun
altre ordinador que ja estigui instal·lat i funcionant (encara que no
sigui amb GNU/linux, pobre). Llavors has de posar el suport al
portàtil on vols instal·lar GNU/Linux i convèncer al programari
incrustat del portàtil (BIOS, UEFI, U-Boot... ) que arrenqui
l'instal·lador des d'aquest suport.

I després és navegar menus i contestar algunes preguntes, una mica
anar passant pantalles.

Llavors detectar coses que no funcionin o que no funcionen com tu
vols, i anar arreglant cosa per cosa...

> Hi ha algun col·lectiu que organitzi un curs en el que aprendre a fer aquest
> tipus d’instal·lació?
>

Abans es feien més install parties, ara estic més desconnectat, no sé
si encara se'n fan, s'ha perdut l'interés o s'ha tornat tan fàcil que
la gent ja s'espavila.
 
> Disposo d’un ordinador portàtil en el que podria assajar el procés
> d’instal·lació abans d’intentar-ho en un dispositiu nou comprat sense el
> sistema operatiu windows prèviament instal·lat.
>

No sé, caldria més informació de quins dispositius son. Jo començaria
pel que no té Windows, o més aviat pel que no tingui dades importants
a preservar.

> Algú em pot orientar en aquest propòsit?
>

És un dubte una mica massa generic. T'animo a que comencis i facis
preguntes més concretes. Per exemple:

"m'he baixat aquest fitxer d'aquí però no sé com posar-lo en una
memòria USB de manera que pugui arrencar l'ordinador d'aquí. Estic
treballant des d'aquest sistema operatiu."

o

"m'he llegit aquest document d'aquí, però quan parla de tal terme
no entenc què vol dir o com s'aplica al meu cas. El meu cas és..."

Llavors podem contestar alguna cosa més concreta (qui ho sàpiga).

> No dubteu a contactar amb mi:
> 
> danielabaurrear...@gmail.com
>

Sí, clar. Estàs escrivint a una llista de correu, no ? O ha anat a
parar el teu missatge a la llista de correu per una altra via ?
Saps com van, les llistes de correu ? T'has subscrit ja a aquesta ?

Re: Filtrar comodins/regex de les línies

[Xavier Drudis Ferran]

El primer cas que demanaves sembla fàcil

grep -f fitxer_regexps.txt fitxer_nums.txt

Pel segon no sé cap comanda directa, potser alguna cosa tipus

#!/bin/bash
text=${1:-93123111}
fitxer_regexps=${2:-fitxer_regexps.txt}

for r in $(< "$fitxer_regexps") ; do {
   grep -qE "$r" - <<< "$text" && echo "$r" ;
} ; done


Però això podria ser més eficient si t'ho programes el perl o python o
el que sigui...

Re: Us ha fallat l'actualització... Windows i GNU

[Xavier Drudis Ferran]

El Sun, Jan 07, 2024 at 01:05:15PM +0100, Narcis Garcia deia:
> > Odio Windows.
> > 

Amen

Re: Comprovar signatura de certificat x509

[Xavier Drudis Ferran]

El Wed, Dec 27, 2023 at 08:28:01PM +0100, Narcis Garcia deia:
> Bones i festives, per qui pugui.
> 
> Vaig crear un certificat auto-signat (clau privada i clau pública), com a
> «Autoritat de Certificació» (CA), per a poder generar els meus certificats
> signats amb openssl:
> 
> /etc/ssl/private/c...@ca.key
> /etc/ssl/certs/c...@ca.pem
> 
> Aleshores he creat certificats signats per la CA com:
> /etc/ssl/private/Un.key
> /etc/ssl/certs/Un.pem
> /etc/ssl/private/Dos.key
> /etc/ssl/certs/Dos.pem
> 
> El què passa és que tot això ho vaig fer de forma caòtica i ara em sembla
> que accidentalment vaig crear diferents CA i tot plegat ha perdut
> coherència.
> 
> Algú sap com verificar, per línia de comandes, que un certificat d'usuari ha
> estat signat amb determinada clau d'autoritat?
> 
> Un exemple imaginari:
> $ openssl --verifica-signatura /etc/ssl/certs/c...@ca.pem
> /etc/ssl/certs/Un.pem
> 
> He cercat per Intenret, i trobo explicacions de què es treuen uns bits del
> «Un.pem» i es comparen amb uns altres bits del c...@ca.pem i em perdo.
> 
> Gràcies.
> 

de memòria diria que era

openssl verify -CAfile /etc/ssl/certs/c...@ca.pem /etc/ssl/certs/Un.pem

on amb -CAfile  la concatenació de els PEMs dels certificats de les CAs
intermitges si n'hi ha i la CA arrel.

Alternativament li pots passar un CApath amb un directori on hi hagi
certificats de CAs,

si no, man openssl-verify

Re: [PATCH v6 08/25] spl: Refactor spl_load_info->read to use units of bytes

[Xavier Drudis Ferran]

El Wed, Nov 08, 2023 at 10:59:13AM -0500, Sean Anderson deia:
> 
> OK, what about
> 
> @offset: Offset to read from in bytes. This must be a multiple of 
> @load->bl_len.
> 
> --Sean

Great. Thank you.

Re: [PATCH v6 08/25] spl: Refactor spl_load_info->read to use units of bytes

[Xavier Drudis Ferran]

El Mon, Nov 06, 2023 at 08:54:03AM -0500, Sean Anderson deia:
> On 11/6/23 07:35, Xavier Drudis Ferran wrote:
> > Thanks for your work. I'm still reading... but...
> > 
> > 
> > El Sun, Nov 05, 2023 at 09:25:46PM -0500, Sean Anderson deia:
> > > diff --git a/include/spl.h b/include/spl.h
> > > index 951e136b9ea..ecfc50e0095 100644
> > > --- a/include/spl.h
> > > +++ b/include/spl.h
> > > @@ -297,10 +297,10 @@ struct spl_load_info {
> > >* read() - Read from device
> > >*
> > >* @load: Information about the load state
> > > -  * @sector: Sector number to read from (each @load->bl_len bytes)
> > > -  * @count: Number of sectors to read
> > > +  * @offset: Offset to read from in bytes, in multiples of @load->bl_len
> > > +  * @count: Number of bytes to read, in multiples of @load->bl_len
> > 
> > I'm no native English speaker, but would it be easier to understand? :
> > 
> > +* @offset: Offset to read from in bytes, a multiple of @load->bl_len
> > +* @count: Number of bytes to read, a multiple of @load->bl_len
> > 
> > 
> > 
> 
> I think it would have to be worded
> 
> @offset: Offset to read from in bytes, as a multiple of @load->bl_len
> 
> but to me these both mean the same thing.
> 
> --Sean

Ah, OK. I doubted on whether it should be a comma or "as". Apparently
it was both.

They may mean the same, but I got confused by having "in bytes", and
"in multiples" so close.

"in bytes" means the value should be the correct answer to the
question "how many bytes" (not a number of bits, not a number of
sectors)

but "in multiples of..." doesn't mean the value should be the correct
answer to "how many of multiples". Should the value be a number of
sectors (first multiple of bl_len) a number of dozens of sectors
(another multiple of bl_len) or ...?

In the first part "in" introduces a unit of measure, but in the second
the same word introduces a constraint on a value. That was confusing
to me.

Not saying it doesn't mean the same or even that you should change it,
just elaborating why I said it.

Re: [PATCH v6 08/25] spl: Refactor spl_load_info->read to use units of bytes

[Xavier Drudis Ferran]

Thanks for your work. I'm still reading... but...


El Sun, Nov 05, 2023 at 09:25:46PM -0500, Sean Anderson deia:
> diff --git a/include/spl.h b/include/spl.h
> index 951e136b9ea..ecfc50e0095 100644
> --- a/include/spl.h
> +++ b/include/spl.h
> @@ -297,10 +297,10 @@ struct spl_load_info {
>* read() - Read from device
>*
>* @load: Information about the load state
> -  * @sector: Sector number to read from (each @load->bl_len bytes)
> -  * @count: Number of sectors to read
> +  * @offset: Offset to read from in bytes, in multiples of @load->bl_len
> +  * @count: Number of bytes to read, in multiples of @load->bl_len

I'm no native English speaker, but would it be easier to understand? :

+* @offset: Offset to read from in bytes, a multiple of @load->bl_len
+* @count: Number of bytes to read, a multiple of @load->bl_len

Re: [SPAM] [PATCH 1/1] doc: short overlong title underlines

[Xavier Drudis Ferran]

El Sat, Oct 28, 2023 at 12:03:12PM +0200, Heinrich Schuchardt deia:
> Title underlines should match the length of the title. Unfortunately
> docutils only catches underlines that are too short.
> 
> Add some missing empty lines after titles.
> 
> Signed-off-by: Heinrich Schuchardt 

Reviewed-by: Xavier Drudis Ferran 

> ---
>  doc/arch/arm64.ffa.rst | 20 ++--
>  doc/board/AndesTech/ae350.rst  |  2 +-
>  doc/board/actions/cubieboard7.rst  |  4 ++--
>  doc/board/actions/index.rst|  2 +-
>  doc/board/armltd/index.rst |  2 +-
>  doc/board/mediatek/index.rst   |  2 +-
>  doc/board/nxp/imx8mm_evk.rst   |  9 +
>  doc/board/nxp/ls1046ardb.rst   |  2 +-
>  doc/board/nxp/mx6ul_14x14_evk.rst  |  2 +-
>  doc/board/openpiton/riscv64.rst|  4 ++--
>  doc/board/purism/librem5.rst   |  2 +-
>  doc/board/qualcomm/sdm845.rst  |  7 ++-
>  doc/board/samsung/index.rst|  2 +-
>  doc/board/st/st-dt.rst |  2 +-
>  doc/board/st/stm32_MCU.rst |  2 +-
>  doc/board/starfive/visionfive2.rst |  3 ++-
>  doc/board/thead/index.rst  |  2 +-
>  doc/board/ti/am62x_beagleplay.rst  |  4 ++--
>  doc/board/ti/am62x_sk.rst  |  5 +++--
>  doc/board/ti/am64x_evm.rst |  3 ++-
>  doc/board/ti/am65x_evm.rst |  3 ++-
>  doc/board/ti/j7200_evm.rst |  3 ++-
>  doc/board/ti/j721e_evm.rst |  3 ++-
>  doc/board/ti/j721s2_evm.rst|  6 +-
>  doc/board/ti/k3.rst|  4 ++--
>  doc/board/xilinx/xilinx.rst|  2 +-
>  doc/build/source.rst   |  2 +-
>  doc/develop/driver-model/ethernet.rst  | 12 ++--
>  doc/develop/driver-model/migration.rst |  2 +-
>  doc/develop/driver-model/nvmxip.rst|  8 
>  doc/develop/driver-model/spi-howto.rst |  2 +-
>  doc/develop/falcon.rst |  2 +-
>  doc/usage/cmd/askenv.rst   |  2 +-
>  doc/usage/cmd/bootdev.rst  |  4 ++--
>  doc/usage/cmd/cat.rst  |  2 +-
>  doc/usage/cmd/coninfo.rst  |  2 +-
>  doc/usage/cmd/mmc.rst  |  2 +-
>  doc/usage/cmd/part.rst |  2 +-
>  doc/usage/cmd/wdt.rst  |  2 +-
>  doc/usage/cmd/xxd.rst  |  2 +-
>  doc/usage/fit/beaglebone_vboot.rst |  2 +-
>  doc/usage/measured_boot.rst|  4 ++--
>  tools/binman/entries.rst   |  2 +-
>  43 files changed, 86 insertions(+), 70 deletions(-)
> 
> diff --git a/doc/arch/arm64.ffa.rst b/doc/arch/arm64.ffa.rst
> index 4ecdc31716..f966f8ba6a 100644
> --- a/doc/arch/arm64.ffa.rst
> +++ b/doc/arch/arm64.ffa.rst
> @@ -40,7 +40,7 @@ The U-Boot FF-A support provides the following parts:
>  - Sandbox FF-A test cases.
>  
>  FF-A and SMC specifications
> 
> +---
>  
>  The current implementation of the U-Boot FF-A support relies on
>  `FF-A v1.0 specification`_ and uses SMC32 calling convention which
> @@ -56,12 +56,12 @@ Hypervisors are supported if they are configured to trap 
> SMC calls.
>  The FF-A support uses 64-bit registers as per `SMC Calling Convention v1.2 
> specification`_.
>  
>  Supported hardware
> -
> +--
>  
>  Aarch64 plaforms
>  
>  Configuration
> ---
> +-
>  
>  CONFIG_ARM_FFA_TRANSPORT
>  Enables the FF-A support. Turn this on if you want to use FF-A
> @@ -70,7 +70,7 @@ CONFIG_ARM_FFA_TRANSPORT
>  When using sandbox, the sandbox FF-A emulator and FF-A sandbox driver 
> will be used.
>  
>  FF-A ABIs under the hood
> 
> +
>  
>  Invoking an FF-A ABI involves providing to the secure world/hypervisor the
>  expected arguments from the ABI.
> @@ -89,7 +89,7 @@ The driver reads the response and processes it accordingly.
>  This methodology applies to all the FF-A ABIs.
>  
>  FF-A bus discovery on Arm 64-bit platforms
> --
> +--
>  
>  When CONFIG_ARM_FFA_TRANSPORT is enabled, the FF-A bus is considered as
>  an architecture feature and discovered using ARM_SMCCC_FEATURES mechanism.
> @@ -136,7 +136,7 @@ When one of the above actions fails, probing fails and 
> the driver stays not acti
>  and can be probed again if needed.
>  
>  Requirements for clients
> --
> +
>  
>  When using the FF-A bus 

Re: [PATCH 1/1] general: fix GPL-2.0-or-later SPDX headers

[Xavier Drudis Ferran]

El Mon, Oct 23, 2023 at 10:21:52AM +0200, Heinrich Schuchardt deia:
> 
> We will need separate patches for LGPL-2.0+ and LGPL-2.1+, too.
>

That'd be for LGPL-2.1+ 
The only case of LGPL-2.0+ in code is included in the patch you sent.
Well, there's another appearance of LGPL-2.0+ in
Licenses/README

Re: [PATCH 1/1] general: fix GPL-2.0-or-later SPDX headers

[Xavier Drudis Ferran]

El Sun, Oct 22, 2023 at 07:47:07PM +0200, Heinrich Schuchardt deia:
> diff --git a/drivers/sound/tegra_ahub.c b/drivers/sound/tegra_ahub.c
> index 495a29c513..28f392504d 100644
> --- a/drivers/sound/tegra_ahub.c
> +++ b/drivers/sound/tegra_ahub.c
> @@ -1,4 +1,4 @@
> -// SPDX-License-Identifier: GPL-2.0+159
> +// SPDX-License-Identifier: GPL-2.0-or-later159
>  /*
>   * Take from dc tegra_ahub.c
>   *

What does this 159 mean (or meant) ?

> diff --git a/lib/errno_str.c b/lib/errno_str.c
> index 2e5f4a887d..9bae8ecb11 100644
> --- a/lib/errno_str.c
> +++ b/lib/errno_str.c
> @@ -1,8 +1,7 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
>  /*
>   * Copyright (C) 2014 Samsung Electronics
>   * Przemyslaw Marczak 
> - *
> - * SDPX-License-Identifier:  GPL-2.0+
>   */
>  #include 
>  #include 

Should be replaced, not supressed.
the tab might be replaced by one space too.

Re: [SPAM] [PATCH 20/26] test: spl: Add functions to create images

[Xavier Drudis Ferran]

El Wed, Oct 11, 2023 at 09:56:20PM -0400, Sean Anderson deia:
> This add some basic functions to create images, and a test for said
> functions. This is not intended to be a test of the image parsing
> functions, but rather a framework for creating minimal images for testing
> load methods. That said, it does do an OK job at finding bugs in the image
> parsing directly.
> 
> Since we have two methods for loading/parsing FIT images, add LOAD_FIT_FULL
> as a separate CI run.
> 
> Signed-off-by: Sean Anderson 
> ---
> 
>  .azure-pipelines.yml |   4 +
>  .gitlab-ci.yml   |   7 +
>  arch/sandbox/cpu/u-boot-spl.lds  |   2 +
>  configs/sandbox_noinst_defconfig |   6 +
>  configs/sandbox_spl_defconfig|   6 +
>  include/test/spl.h   | 117 ++
>  test/image/spl_load.c| 352 +++
>  test/image/spl_load_os.c |   5 +-
>  8 files changed, 495 insertions(+), 4 deletions(-)
>  create mode 100644 include/test/spl.h
>
[...]
> diff --git a/test/image/spl_load.c b/test/image/spl_load.c
> index 1a57bf846d2..ca3777cab37 100644
> --- a/test/image/spl_load.c
> +++ b/test/image/spl_load.c
> @@ -4,7 +4,15 @@
>   */
>  
>  #include 
> +#include 
> +#include 
>  #include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
>  
>  int board_fit_config_name_match(const char *name)
>  {
> @@ -15,3 +23,347 @@ struct legacy_img_hdr *spl_get_load_buffer(ssize_t 
> offset, size_t size)
>  {
>   return map_sysmem(0x10, 0);
>  }
> +
> +/* Try to reuse the load buffer to conserve memory */
> +void *board_spl_fit_buffer_addr(ulong fit_size, int sectors, int bl_len)
> +{
> + static void *buf;
> + static size_t size;
> + static bool invalid = true;
> +
> + if (invalid || size < sectors * bl_len) {
> + if (!invalid)
> + free(buf);
> + buf = malloc_cache_aligned(sectors * bl_len);
> + invalid = false;

Should size be updated here or am I being thick ?
size = ALIGN(sectors * bl_len, ARCH_DMA_MINALIGN);

> + }
> + return buf;
> +}
> +

Re: (deb-cat) LibreOffice i MariaDB/MySQL

[Xavier Drudis Ferran]

El Mon, Oct 02, 2023 at 12:45:26PM +0200, Narcis Garcia deia:
> 
> Però el què necessito és poder dissenyar un formulari: Amb etiquetes i
> explicacions per a cada camp que emplena l'usuari, limitació del format de
> les dades, algun avís o reacció programada, etc.
> També dissenyar un informe: Un llistat on aparegui el títol que jo vull,
> explicacions a l'efecte, numeració de pàgines, suma de la columna que jo
> vull, etc.
>

Ni idea.  La meva humil opinió és que això no és tan fàcil d'oferir
en una aplicació genèrica, i per això qui ho intenta es queda bastant
a mitges. Al final si intentes fer-ho amb una aplicació "per a
dummies" acabes tenint més feina que si et fas a mida una aplicació
web o una aplicació d'escriptori (però hauria de ser portable).

Els frameworks de programació són més complicats d'aprendre però
ofereixen més flexibilitat i llavors costen menys de fer servir.

Generadors d'informe potser encara en pots trobar de genèrics que et
solucionin alguna cosa (jasper reports o així). Però també si no es
complica molt el llistat, que si no arriba un moment que s'acaben o es
compliquen massa. Però formularis que no siguin estúpids del tot i
siguin còmodes i informatius, costen menys de fer amb HTML i un
programa de servidor que amb dissenyadors de formularis que tenen un
conjunt limitat de tipus, validacions i presentacions.

Vull dir que sí, que fer-te un programa a mida és feina. Però agafar
una eina genèrica i fer-te-la a mida a vegades és més feina i queda
pitjor. I si intentes estalviar temps acabaràs amb deute tecnològic,
però et passarà tant programant-ho tu com amb una eina genèrica. Si ho
programes tu no li voldràs deicar prou temps per deixar-ho super
perfecte i quan ho vulguis canviar en un futur serà més difícil. I si
agafes una eina, un cop tinguis alguna cosa passable, ho deixaràs així
i en un futur et trobaràs nous requeriments que l'eina no hi arribarà
i t'hauràs de passar a un programa a mida.

En qualsevol cas, bona sort.

Re: (deb-cat) LibreOffice i MariaDB/MySQL

[Xavier Drudis Ferran]

El Mon, Oct 02, 2023 at 12:27:04PM +0200, Xavier Drudis Ferran deia:
> El Mon, Oct 02, 2023 at 10:52:12AM +0200, Narcis Garcia deia:
> > 
> > Necessito reduir els requisits tècnics al mínim, i el meu objectiu és que
> > l'usuari només hagi de tenir LibreOffice i saber la seva credencial.
> > Si s'han d'instal·lar «extres» això complica les possibilitats d'èxit.
> >
> 
> No estic segur que el libreoffice sol inclogui totes les extensions
> necessàries per acedir a MySQL, o a tots els sgbd possibles.
> 
> MySQL té un client de base de dades gràfic (MySQL Workbench), però no
> tinc clar que un usuari final s'hi trobi molt còmode.
> 
> Si no aparentment hi ha aplicacions que no ho he provat com per dir
> si et serveixen, però em fa por que puguin espantar usuaris. Si vols mirar,
> sense masssa fe...
> 
> kexi, ferret, kettle (pentaho data integration)
> 
> Però sembla matar mosques a canonades, no sé, és per altres mena de feines...
>

O potser Tora, SQL Squirrel o DBeaver, però no estan a debian.

Re: (deb-cat) LibreOffice i MariaDB/MySQL

[Xavier Drudis Ferran]

El Mon, Oct 02, 2023 at 10:52:12AM +0200, Narcis Garcia deia:
> 
> Necessito reduir els requisits tècnics al mínim, i el meu objectiu és que
> l'usuari només hagi de tenir LibreOffice i saber la seva credencial.
> Si s'han d'instal·lar «extres» això complica les possibilitats d'èxit.
>

No estic segur que el libreoffice sol inclogui totes les extensions
necessàries per acedir a MySQL, o a tots els sgbd possibles.

MySQL té un client de base de dades gràfic (MySQL Workbench), però no
tinc clar que un usuari final s'hi trobi molt còmode.

Si no aparentment hi ha aplicacions que no ho he provat com per dir
si et serveixen, però em fa por que puguin espantar usuaris. Si vols mirar,
sense masssa fe...

kexi, ferret, kettle (pentaho data integration)

Però sembla matar mosques a canonades, no sé, és per altres mena de feines...


> > > i directament funciona
> > > amb algunes mancances.
> > > «Connect directly (using MariaDB C connector)»:
> > > 
> > > No desa claus primàries quan creo taules amb camps, i no reacciona quan
> > > demano crear un informe.
> > 
> > Ni idea, sembla alguna incompatibilitat.
> > Ho fa indiferentment del tipus de dades dels camps clau ?
> 
> Sembla que si, que és indiferent.

Vaja.

> I els camps d'autocomptador numèric, no compten.
>

Ara que ho dius això em sona com d'alguna prova fa mooolt de temps.
Crec que no ho vaig poder arreglar i vaig acabar havent de posar un
trigger al MySQL per una cosa que en teoria no calia.

> 
> Necessito que es pugui fer amb una aplicació d'escriptori, independentment
> de què algun dia es desenvolupi una interfície web, que no podrà dependre de
> CMS que condicionin tot.
>

> 
> El problema és requerir que l'usuari s'hagi d'instal·lar cosetes, i què això
> sigui viable amb GNU, Windows, etc.


No sé què dir-te. No em sona que la gent tingui als seus escriptoris
aplicacions genèriques d'actualitzar bases de dades que valguin gran
cosa.

Llavors si ha de ser d'escriptori, però no vols que hagin d'instal·lar
gran cosa...  difícil. Si te'n surts ja ens ho expliques.

Si, ja, el libreoffice base hauria de servir. Però jo les vegades que
el vaig provar (poc i fa temps) no em va semblar tan perfecte i
superpolit i infalible com per deixar gaire sols als usuaris,
dependent de qui fossin... Potser simplement vaig ser jo que no en
sabia o no m'hi vaig dedicar prou.

Re: [SPAM] [PATCH] clk: fix count parameter type for clk_release_all

[Xavier Drudis Ferran]

El Mon, Jun 19, 2023 at 01:47:52PM +0300, Eugen Hristev deia:
> The second parameter for clk_release_all is used as an unsigned
> (which makes sense) but the function prototype declares it as an int.
> This causes warnings/error like such below:
> 
> include/clk.h:422:48: error: conversion to ‘int’ from ‘unsigned int’ may 
> change the sign of the result [-Werror=sign-conversion]
>   422 | return clk_release_all(bulk->clks, bulk->count);
> 
> To fix this, changed the type of the count to `unsigned int`
> 
> Fixes: 82a8a669b4f7 ("clk: add clk_release_all()")
> Signed-off-by: Eugen Hristev 

Reviewed-by: Xavier Drudis Ferran 

> ---
>  drivers/clk/clk-uclass.c | 7 ---
>  include/clk.h| 4 ++--
>  2 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
> index dc3e9d6a2615..eada3a3a5b62 100644
> --- a/drivers/clk/clk-uclass.c
> +++ b/drivers/clk/clk-uclass.c
> @@ -416,12 +416,13 @@ int clk_get_by_name_nodev(ofnode node, const char 
> *name, struct clk *clk)
>   return clk_get_by_index_nodev(node, index, clk);
>  }
>  
> -int clk_release_all(struct clk *clk, int count)
> +int clk_release_all(struct clk *clk, unsigned int count)
>  {
> - int i, ret;
> + unsigned int i;
> + int ret;
>

This could also be changed in clk_enable_blk() and clk_disable_blk().
It's unlikely that we get so many clocks for it to matter, but it's
till wrong to compare int i to unsigned int bulk->count.  That'd be a
different patch, though.

>   for (i = 0; i < count; i++) {
> - debug("%s(clk[%d]=%p)\n", __func__, i, &clk[i]);
> + debug("%s(clk[%u]=%p)\n", __func__, i, &clk[i]);
>  
>   /* check if clock has been previously requested */
>   if (!clk[i].dev)
> diff --git a/include/clk.h b/include/clk.h
> index d91285235f79..a342297007b6 100644
> --- a/include/clk.h
> +++ b/include/clk.h
> @@ -243,7 +243,7 @@ static inline struct clk *devm_clk_get_optional(struct 
> udevice *dev,
>   *
>   * Return: zero on success, or -ve error code.
>   */
> -int clk_release_all(struct clk *clk, int count);
> +int clk_release_all(struct clk *clk, unsigned int count);
>  
>  /**
>   * devm_clk_put  - "free" a managed clock source
> @@ -307,7 +307,7 @@ clk_get_by_name_nodev(ofnode node, const char *name, 
> struct clk *clk)
>   return -ENOSYS;
>  }
>  
> -static inline int clk_release_all(struct clk *clk, int count)
> +static inline int clk_release_all(struct clk *clk, unsigned int count)
>  {
>   return -ENOSYS;
>  }
> -- 
> 2.34.1
> 

Re: (deb-cat) LibreOffice i MariaDB/MySQL

[Xavier Drudis Ferran]

Ho sento, no ho he provat ni gens ni gaire això que dius,
suposo que no et podré ajudar...

El Tue, Sep 12, 2023 at 09:05:56AM +0200, Narcis Garcia deia:
> Bon dia,
> 
> Estic intentant crear un document de base de dades amb LibreOffice per a
> compartir, connectat a una base de dades del servei remot MariaDB.
> Em trobo que fer-ho mitjançant ODBC requereix algun tipus de configuració a
> l'entorn operatiu (poc portable),

I amb les altres opciosn no necessites configuració local ?
Al menys credencials o això ?

> mitjançant JDBC no funciona* (Prova la
> classe: No s'ha pogut carregar el controlador JDBC),

tens libmariadb-java instal·lat ?

> i directament funciona
> amb algunes mancances.
> «Connect directly (using MariaDB C connector)»:
> 
> No desa claus primàries quan creo taules amb camps, i no reacciona quan
> demano crear un informe.

Ni idea, sembla alguna incompatibilitat.
Ho fa indiferentment del tipus de dades dels camps clau ?

> A més a més, si ho deixes obert perd la connexió i cap tancar tot el
> LibreOffice per a poder tornar a obrir i que connecti.
>

M'imagino que algun timeout es deu poder configurar per algun lloc,
wait_timeout, interactive_timeout...
però si no vols configuracions locals ja no sé...

De detalls així suposo que ja en saps buscar tu...
https://stackoverflow.com/questions/51506416/mariadb-server-times-out-client-connection-after-600-seconds

A mi em sonava que sense tancar el LibreOffice podies obrir i tancar
connexions, però potser no ho recordo bé o ha canviat en les versions
actuals.

> ¿Algú s'ha barallat amb aquest tema i pt fer recomanacions?
>

Poc i fa massa temps. Ho sento. Sort.

> El què busco és una forma senzilla de què, entre vàries persones cadascuna a
> casa seva, introduïm dades a una base de dades mitjançant un formulari, i
> podem treure un llistat.
> Que no requereixi cap coneixement tècnic per a utilitzar-ho per primera
> vegada.
>

Ni flors, Nextcloud forms ?
Framaforms ?
https://framaforms.org/abc/ca/
Framacalc ? 
https://framacalc.org/abc/ca/

Però la funcionalitat pot no quadrar i instal·lar o mantenir el teu
servidor pot ser massa feina.

En tot cas si vas per ODBC, JDBC, etc. tingues en compte si les dades
van xifrades o no per la xarxa.

Per a xifrar-les normalment caldria configurar claus i certificats al
servidor (i si vols al client per autenticar però això és més
configuració local i igual no vols). I si la CA del certificat de
servidor no és pública, o no et vols refiar de totes les CAs
públiques, o vols fer alguna mena de pinning, potser configurar
truststores i mandangues a cada client...

> 
> LibreOffice: 7.4
> Debian: 11
> (*) El paquet libreoffice-base-drivers està instal·lat, que inclou JDBC.

Sospito que inclou part de JDBC, el costat de l'aplicació, però no el
costat de la BD, i llavors falta un controlador JDBC (en java)
específic per a la BD, que seria libmariadb-java, però tampoc he
provat ara fa poc.

Re: [PATCH 1/1] spl: undefined return value in spl_blk_load_image

[Xavier Drudis Ferran]

El Wed, Sep 06, 2023 at 02:25:11PM +0200, Heinrich Schuchardt deia:
> spl_blk_load_image() should not return an uninitialized value if
> blk_get_devnum_by_uclass_id() fails.
> 
> Fixes: 8ce6a2e17577 ("spl: blk: Support loading images from fs")
> Reported-by: Xavier Drudis Ferran 

FWIW:

Reviewed-by:  Xavier Drudis Ferran 


> Signed-off-by: Heinrich Schuchardt 
> ---
>  common/spl/spl_blk_fs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/common/spl/spl_blk_fs.c b/common/spl/spl_blk_fs.c
> index eb6f526689..ea5d1a51d9 100644
> --- a/common/spl/spl_blk_fs.c
> +++ b/common/spl/spl_blk_fs.c
> @@ -53,7 +53,7 @@ int spl_blk_load_image(struct spl_image_info *spl_image,
>   blk_desc = blk_get_devnum_by_uclass_id(uclass_id, devnum);
>   if (!blk_desc) {
>   printf("blk desc for %d %d not found\n", uclass_id, devnum);
> - goto out;
> + return -ENODEV;
>   }
>  
>   blk_show_device(uclass_id, devnum);
> -- 
> 2.40.1
> 

Re: [PATCH v5 05/11] spl: Convert mmc to spl_load

[Xavier Drudis Ferran]

El Tue, Sep 05, 2023 at 11:02:44AM -0400, Sean Anderson deia:
> On 9/4/23 08:59, Xavier Drudis Ferran wrote:
> > El Sun, Sep 03, 2023 at 08:17:26AM +, Jonas Karlman deia:
> > 
> >> > Fundamentally, we can't really deal with unaligned images without a
> >> > bounce-buffer. The method used by SPL_LOAD_FIT_IMAGE_BUFFER_SIZE will
> >> > continue working, since we call into the FIT routines to load the image.
> > 
> > Yes
> > 
> >> > I would like to defer bounce buffering for other images until someone
> >> > actually needs it.
> >> >
> > 
> > Fine.
> > 
> >> > Note that in the FIT case, you can also do `mkimage -EB`, at least if
> >> > you aren't using FIT_LOAD_FULL.
> >> 
> >> With the following two commits introduced in v2023.04-rc1, the alignment
> >> issue for Rockchip has been fixed and all external data is now accessed
> >> block aligned.
> >> 
> >> 9b2fd2d22852 ("binman: Add support for align argument to mkimage tool")
> >> 5ad03fc77dfa ("rockchip: Align FIT image data to SD/MMC block length")
> >> 
> >> Regards,
> >> Jonas
> >>
> > 
> > Well, yes, thanks.
> > 
> > I was carrying Jerome's patch still thinking it was needed for me, but
> > I just tried without and it works too, in mmc. In spi I didn't try but
> > it should be even easier (bl_len=1).
> > 
> > For me it's still odd to write outside intended memory. Would a warning
> > in case legacy image loading writes before load_addr be acceptable ?
> > Just in case someone was using the memory there for something else.
> 
> We could add something, but it would have to be behind SHOW_ERRORS. This
> series is already having a very tough time reducing bloat without adding
> any (other) new features.
>

When I looked at it I thought the check and warning was only needed at
the end of spl_load, for legacy images. For fit it was already
aligned by calculating offset as a integer multiple of sectors.
I might misremember now.

In cases where the buffer is reserved it could just include the extra
bytes at the start before the load address, as I tried to suggest (v5
02/11), and then no warning is needed. Which is silly because the
spl_get_load_buffer doesn't do much at all. But someday it might do
more...

Your changes moves the extra bytes from the end to the start, and I
find it more unexpected than just having extra bytes at the end for an
image that might not have such a fixed length. I'd expect people to
leave room at the end when planning memory, thinking a future image
might be bigger, but not thinking to leave room at the start. But I'm
not all that sure...

If not a check and warning at least note something in documentation or
comments somewhere...

And I think you are really adding new features, in that even if the
formats and methods stay the same, there'll be more combinations
available after your change. So I'd be tolerant to a small code size
increase in exchange for the new flexibility. But I can understand
some cases can't afford the luxury, maybe.

Whatever you decide, looking forward to test a little your v6 if I can.

Re: [PATCH v2] usb: host: ohci-generic: Make usage of clock/reset bulk() API

[Xavier Drudis Ferran]

El Mon, Sep 04, 2023 at 02:20:21PM +0200, Fabrice Gasnier deia:
> Make usage of clock and reset bulk API in order to simplify the code
> 
> Reviewed-by: Marek Vasut 

Reviewed-by: Xavier Drudis Ferran 

Sorry. I don't know why I used the wrong address before.

> Signed-off-by: Fabrice Gasnier 
> ---
> 
> Changes in v2:
> - fix copy/paste on dev_err message since Marek's review comment
> - add Marek's review tag
> 
> ---
>  drivers/usb/host/ohci-generic.c | 92 +++--
>  1 file changed, 29 insertions(+), 63 deletions(-)
> 
> diff --git a/drivers/usb/host/ohci-generic.c b/drivers/usb/host/ohci-generic.c
> index 2d8d38ce9a40..ceed1911a95a 100644
> --- a/drivers/usb/host/ohci-generic.c
> +++ b/drivers/usb/host/ohci-generic.c
> @@ -16,75 +16,41 @@
>  
>  struct generic_ohci {
>   ohci_t ohci;
> - struct clk *clocks; /* clock list */
> - struct reset_ctl *resets; /* reset list */
> + struct clk_bulk clocks; /* clock list */
> + struct reset_ctl_bulk resets; /* reset list */
>   struct phy phy;
> - int clock_count;/* number of clock in clock list */
> - int reset_count;/* number of reset in reset list */
>  };
>  
>  static int ohci_usb_probe(struct udevice *dev)
>  {
>   struct ohci_regs *regs = dev_read_addr_ptr(dev);
>   struct generic_ohci *priv = dev_get_priv(dev);
> - int i, err, ret, clock_nb, reset_nb;
> -
> - err = 0;
> - priv->clock_count = 0;
> - clock_nb = dev_count_phandle_with_args(dev, "clocks", "#clock-cells",
> -0);
> - if (clock_nb > 0) {
> - priv->clocks = devm_kcalloc(dev, clock_nb, sizeof(struct clk),
> - GFP_KERNEL);
> - if (!priv->clocks)
> - return -ENOMEM;
> -
> - for (i = 0; i < clock_nb; i++) {
> - err = clk_get_by_index(dev, i, &priv->clocks[i]);
> - if (err < 0)
> - break;
> -
> - err = clk_enable(&priv->clocks[i]);
> - if (err && err != -ENOSYS) {
> - dev_err(dev, "failed to enable clock %d\n", i);
> - clk_free(&priv->clocks[i]);
> - goto clk_err;
> - }
> - priv->clock_count++;
> - }
> - } else if (clock_nb != -ENOENT) {
> - dev_err(dev, "failed to get clock phandle(%d)\n", clock_nb);
> - return clock_nb;
> + int err, ret;
> +
> + ret = clk_get_bulk(dev, &priv->clocks);
> + if (ret && ret != -ENOENT) {
> + dev_err(dev, "Failed to get clocks (ret=%d)\n", ret);
> + return ret;
> + }
> +
> + err = clk_enable_bulk(&priv->clocks);
> + if (err) {
> + dev_err(dev, "Failed to enable clocks (err=%d)\n", err);
> + goto clk_err;
>   }
>  
> - priv->reset_count = 0;
> - reset_nb = dev_count_phandle_with_args(dev, "resets", "#reset-cells",
> -0);
> - if (reset_nb > 0) {
> - priv->resets = devm_kcalloc(dev, reset_nb,
> - sizeof(struct reset_ctl),
> - GFP_KERNEL);
> - if (!priv->resets)
> - return -ENOMEM;
> -
> - for (i = 0; i < reset_nb; i++) {
> - err = reset_get_by_index(dev, i, &priv->resets[i]);
> - if (err < 0)
> - break;
> -
> - err = reset_deassert(&priv->resets[i]);
> - if (err) {
> - dev_err(dev, "failed to deassert reset %d\n", 
> i);
> - reset_free(&priv->resets[i]);
> - goto reset_err;
> - }
> - priv->reset_count++;
> - }
> - } else if (reset_nb != -ENOENT) {
> - dev_err(dev, "failed to get reset phandle(%d)\n", reset_nb);
> + err = reset_get_bulk(dev, &priv->resets);
> + if (err && err != -ENOENT) {
> + dev_err(dev, "failed to get resets (err=%d)\n", err);
>   goto clk_err;
>   }
>  
> + err = reset_deassert_bulk(&priv->resets);
> 

Re: [PATCH v5 05/11] spl: Convert mmc to spl_load

[Xavier Drudis Ferran]

El Sun, Sep 03, 2023 at 08:17:26AM +, Jonas Karlman deia:

> > Fundamentally, we can't really deal with unaligned images without a
> > bounce-buffer. The method used by SPL_LOAD_FIT_IMAGE_BUFFER_SIZE will
> > continue working, since we call into the FIT routines to load the image.

Yes

> > I would like to defer bounce buffering for other images until someone
> > actually needs it.
> >

Fine.

> > Note that in the FIT case, you can also do `mkimage -EB`, at least if
> > you aren't using FIT_LOAD_FULL.
> 
> With the following two commits introduced in v2023.04-rc1, the alignment
> issue for Rockchip has been fixed and all external data is now accessed
> block aligned.
> 
> 9b2fd2d22852 ("binman: Add support for align argument to mkimage tool")
> 5ad03fc77dfa ("rockchip: Align FIT image data to SD/MMC block length")
> 
> Regards,
> Jonas
>

Well, yes, thanks.

I was carrying Jerome's patch still thinking it was needed for me, but
I just tried without and it works too, in mmc. In spi I didn't try but
it should be even easier (bl_len=1).

For me it's still odd to write outside intended memory. Would a warning
in case legacy image loading writes before load_addr be acceptable ?
Just in case someone was using the memory there for something else.

Re: [PATCH] usb: host: ohci-generic: Make usage of clock/reset bulk() API

[Xavier Drudis Ferran]

El Mon, Sep 04, 2023 at 11:25:06AM +0200, Fabrice Gasnier deia:
> 
> IMHO, the OHCI should have failed too in this example, instead of
> silently ignoring the error. Hopefully it has probed.
>
> The clk_get_bulk() code does a similar job compared to ohci current
> code. It counts all clock entries. So, when trying to get them, these
> should be found.
> 
> Having a clock listed, but it can't be found or taken rather looks like
> a real error, that needs to be fixed.
> (e.g. missing config for a clk/reset controller ? Or could it be a bug
> in such a driver ? Or a miss-configured device-tree ? something else?)
> Ignoring such error may be miss-leading (as you pointed out, one was
> working).
> 
> I hope I don't miss your point. If this is the case, could you point
> more precise example, or how it used to fail ?
>

No, you don't miss my point. I'll give you pointers to the case I
meant, but I'm afraid it might mislead, because it's already solved,
and for current U-Boot it should pose no problem with or without your
patch.

The general problem might be that dts come from linux, and the drivers
come from U-Boot, so U-Boot might ignore some hardware described in
the linux dts that it doesn't need. Now this is more typical for, say,
a VPU than a clock or reset. But it once was a missing clock driver in
U-Boot that linux used for suspend/resume and happened to be at the
end of the clock list. So it worked when ohci probe ignored the missing
clock, because U-Boot doesn't need suspend, but it didn't work for ehci
that called clk_get_bulk().

There might be other cases like that example somewhere, but I'm not
saying it's likely. I guess we'll know if some board breaks.


If you really want the gory details...

https://patchwork.ozlabs.org/project/uboot/patch/20220701185959.GC1700@begut/
https://patchwork.ozlabs.org/project/uboot/patch/Y44+ayJfUlI08ptM@localhost/
https://patchwork.ozlabs.org/project/uboot/patch/Y5IWpjYLB4aXMy9o@localhost/
https://lists.denx.de/pipermail/u-boot/2022-December/501811.html
https://lists.denx.de/pipermail/u-boot/2023-February/510676.html
https://lists.denx.de/pipermail/u-boot/2023-February/510678.html
https://patchwork.ozlabs.org/project/uboot/patch/202013db5a47ecbac4a53c360ed1ca91ca663996.1685974993.git.xdru...@tinet.cat/
https://patchwork.ozlabs.org/project/uboot/patch/464111fca83008503022e8ada5305e69ffd1afbd.1685974993.git.xdru...@tinet.cat/


> > In that case, a fix by ignoring the missing clock
> > in ehci was rejected, so maybe that criteria applies here as well and
> > your patch is deemed correct. I don't know. That case won't break now,
> > I think, either with or without your patch, because after another fix,
> > that clock will be found.
> 
> If I understand correctly, this used to fixed elsewhere (e.g. there used
> to be a real bug fixed) ?
>

Yes. See above. Or don't, it's not that important.
A clock driver was missing, only needed for suspend/resume.
ohci ignored it and worked (U-Boot doesn't suspend)
ehci failed probing and dind't work.
Current situation is this particular clock driver is no longer missing.

> > But I don't know if there might be similar
> > cases.
> > 
> > I just wanted to point out the change in behaviour. If the change is
> > intended, then all is fine.
> 
> IMHO, this should be fine. I hope you agree with this change and the
> rationale.
>

I do.

I just wanted to point it out in case anyone knew why ohci wasn't
calling clk_get_bulk(). It might have been on purpose.

In fact Kever Yang once proposed to change ehci to be tolerant to a
missing clock like ohci was (but with an explicit warning). But Marek
Vasut proposed adding a clock driver and Kever didn't complain, so I
don't think this is his very strong opinion, he may just be happy when
things work and others are happy, I can't read minds.

https://lists.denx.de/pipermail/u-boot/2022-December/501811.html

FWIW

ohci_probe introduced:
fee331f66c9 (Alexey Brodkin   2015-12-14 17:18:50 +0300 

loop for clocks introduced in ohci_probe:
155d9f65d3b (Patrice Chotard  2017-07-18 11:57:12 +0200 

clk_get_bulk introduced:
a855be87da4 (Neil Armstrong  2018-04-03 11:44:18 +0200 156)

So ochi_probe() didn't call clk_get_bulk() most likely because it
din't exist back then.

So, unless someone else has a failing case, I agree to your change.

I'd welcome if the commit message would say that the new policy is any
missing clocks or resets cause the probe to fail. But since you
already sent v2, it doesn't matter.

Re: [PATCH] usb: host: ohci-generic: Make usage of clock/reset bulk() API

[Xavier Drudis Ferran]

Is the change of behaviour intended when a clock or reset is not found ?
(see below)

El Wed, Aug 30, 2023 at 10:01:49AM +0200, Fabrice Gasnier deia:
> Make usage of clock and reset bulk API in order to simplify the code
> 
> Signed-off-by: Fabrice Gasnier 
> ---
> 
>  drivers/usb/host/ohci-generic.c | 92 +++--
>  1 file changed, 29 insertions(+), 63 deletions(-)
> 
> diff --git a/drivers/usb/host/ohci-generic.c b/drivers/usb/host/ohci-generic.c
> index 2d8d38ce9a40..95aa608d8c19 100644
> --- a/drivers/usb/host/ohci-generic.c
> +++ b/drivers/usb/host/ohci-generic.c
> @@ -16,75 +16,41 @@
>  
>  struct generic_ohci {
>   ohci_t ohci;
> - struct clk *clocks; /* clock list */
> - struct reset_ctl *resets; /* reset list */
> + struct clk_bulk clocks; /* clock list */
> + struct reset_ctl_bulk resets; /* reset list */
>   struct phy phy;
> - int clock_count;/* number of clock in clock list */
> - int reset_count;/* number of reset in reset list */
>  };
>  
>  static int ohci_usb_probe(struct udevice *dev)
>  {
>   struct ohci_regs *regs = dev_read_addr_ptr(dev);
>   struct generic_ohci *priv = dev_get_priv(dev);
> - int i, err, ret, clock_nb, reset_nb;
> -
> - err = 0;
> - priv->clock_count = 0;
> - clock_nb = dev_count_phandle_with_args(dev, "clocks", "#clock-cells",
> -0);
> - if (clock_nb > 0) {
> - priv->clocks = devm_kcalloc(dev, clock_nb, sizeof(struct clk),
> - GFP_KERNEL);
> - if (!priv->clocks)
> - return -ENOMEM;
> -
> - for (i = 0; i < clock_nb; i++) {
> - err = clk_get_by_index(dev, i, &priv->clocks[i]);
> - if (err < 0)
> - break;
> -

Please note the old code was tolerant of not finding some clocks. It
ignored any clock not found and any other listed after it in the
"clocks" property and enabled the clocks before it.

The clk_get_bulk() function instead returns an error when any clock in
"clocks" is not found and releases (disables again and frees) those
before it.

I'm not aware of any case that breaks because of this, but I once saw
a case of ehci not working and ohci working because one of the listed
clocks not being found (ehci called clk_get_bulk(),
clk_enable_blk()). In that case, a fix by ignoring the missing clock
in ehci was rejected, so maybe that criteria applies here as well and
your patch is deemed correct. I don't know. That case won't break now,
I think, either with or without your patch, because after another fix,
that clock will be found. But I don't know if there might be similar
cases.

I just wanted to point out the change in behaviour. If the change is
intended, then all is fine.

> - err = clk_enable(&priv->clocks[i]);
> - if (err && err != -ENOSYS) {
> - dev_err(dev, "failed to enable clock %d\n", i);
> - clk_free(&priv->clocks[i]);
> - goto clk_err;
> - }
> - priv->clock_count++;
> - }
> - } else if (clock_nb != -ENOENT) {
> - dev_err(dev, "failed to get clock phandle(%d)\n", clock_nb);
> - return clock_nb;
> + int err, ret;
> +
> + ret = clk_get_bulk(dev, &priv->clocks);
> + if (ret && ret != -ENOENT) {
> + dev_err(dev, "Failed to get clocks (ret=%d)\n", ret);
> + return ret;
> + }
> +
> + err = clk_enable_bulk(&priv->clocks);
> + if (err) {
> + dev_err(dev, "Failed to enable clocks (err=%d)\n", err);
> + goto clk_err;
>   }
>  
> - priv->reset_count = 0;
> - reset_nb = dev_count_phandle_with_args(dev, "resets", "#reset-cells",
> -0);
> - if (reset_nb > 0) {
> - priv->resets = devm_kcalloc(dev, reset_nb,
> - sizeof(struct reset_ctl),
> - GFP_KERNEL);
> - if (!priv->resets)
> - return -ENOMEM;
> -
> - for (i = 0; i < reset_nb; i++) {
> - err = reset_get_by_index(dev, i, &priv->resets[i]);
> - if (err < 0)
> - break;
> -

Similar here.

> - err = reset_deassert(&priv->resets[i]);
> - if (err) {
> - dev_err(dev, "failed to deassert reset %d\n", 
> i);
> - reset_free(&priv->resets[i]);
> - goto reset_err;
> - }
> - priv->reset_count++;
> - }
> - } else if (reset_nb != -ENOENT) {
> - dev_err(dev, "failed to get reset phandle(%d)\n", reset_nb);
> +   

Re: [PATCH v5 10/11] spl: Convert spi to spl_load

[Xavier Drudis Ferran]

El Mon, Jul 31, 2023 at 06:43:02PM -0400, Sean Anderson deia:
> This converts the spi load method to use spl_load. As a consequence, it
> also adds support for LOAD_FIT_FULL.
> 
> Signed-off-by: Sean Anderson 
> ---
> 
> Changes in v5:
> - Rework to load header in spl_load
> 
>  common/spl/spl_spi.c | 72 +---
>  1 file changed, 8 insertions(+), 64 deletions(-)
> 
> diff --git a/common/spl/spl_spi.c b/common/spl/spl_spi.c
> index 2aff025f76..14391a1c96 100644
> --- a/common/spl/spl_spi.c
> +++ b/common/spl/spl_spi.c
> @@ -89,12 +89,14 @@ u32 __weak spl_spi_boot_cs(void)
>  static int spl_spi_load_image(struct spl_image_info *spl_image,
> struct spl_boot_device *bootdev)
>  {
> - int err = 0;
>   unsigned int payload_offs;
>   struct spi_flash *flash;
> - struct legacy_img_hdr *header;
>   unsigned int sf_bus = spl_spi_boot_bus();
>   unsigned int sf_cs = spl_spi_boot_cs();
> + struct spl_load_info load = {
> + .bl_len = 1,
> + .read = spl_spi_fit_read,
> + };
>  
>   /*
>* Load U-Boot image from SPI flash into RAM
> @@ -109,77 +111,19 @@ static int spl_spi_load_image(struct spl_image_info 
> *spl_image,
>   return -ENODEV;
>   }
>  
> + load.dev = flash;
>   payload_offs = spl_spi_get_uboot_offs(flash);
>  
> - header = spl_get_load_buffer(-sizeof(*header), sizeof(*header));
> -
>   if (CONFIG_IS_ENABLED(OF_REAL)) {
>   payload_offs = ofnode_conf_read_int("u-boot,spl-payload-offset",
>   payload_offs);
>   }
>  
>  #if CONFIG_IS_ENABLED(OS_BOOT)
> - if (spl_start_uboot() || spi_load_image_os(spl_image, bootdev, flash, 
> header))
> + if (!spl_start_uboot() && !spi_load_image_os(spl_image, bootdev, flash, 
> header))
> + return 0;
>  #endif

But with this return 0 we're skipping the soft reset at the end, aren't we ?
This is the same the old code did. Just not sure whether it was right or 
untested.
If some flash needs reset before running linux, then it might need it with 
U-Boot or in Falcon,
as long as SPL has probed the flash, mightn't it ?
If it needs fixing, then possibly better in a different commit, though ?
I mean yours is fine, you left things as they were.

> - {
> - /* Load u-boot, mkimage header is 64 bytes. */
> - err = spi_flash_read(flash, payload_offs, sizeof(*header),
> -  (void *)header);
> - if (err) {
> - debug("%s: Failed to read from SPI flash (err=%d)\n",
> -   __func__, err);
> - return err;
> - }
> -
> - if (IS_ENABLED(CONFIG_SPL_LOAD_FIT_FULL) &&
> - image_get_magic(header) == FDT_MAGIC) {
> - err = spi_flash_read(flash, payload_offs,
> -  roundup(fdt_totalsize(header), 4),
> -  (void *)CONFIG_SYS_LOAD_ADDR);
> - if (err)
> - return err;
> - err = spl_parse_image_header(spl_image, bootdev,
> - (struct legacy_img_hdr 
> *)CONFIG_SYS_LOAD_ADDR);


So this used to load to CONFIG_SYS_LOAD_ADDR and will now load to
CONFIG_TEXT_BASE, or whatever the applicable spl_get_load_buffer()
uses. Is this OK ? or do we need a new parameter for the buffer or
something ?
   
> - } else if (IS_ENABLED(CONFIG_SPL_LOAD_FIT) &&
> -image_get_magic(header) == FDT_MAGIC) {
> - struct spl_load_info load;
> -
> - debug("Found FIT\n");
> - load.dev = flash;
> - load.priv = NULL;
> - load.filename = NULL;
> - load.bl_len = 1;
> - load.read = spl_spi_fit_read;
> - err = spl_load_simple_fit(spl_image, &load,
> -   payload_offs,
> -   header);
> - } else if (IS_ENABLED(CONFIG_SPL_LOAD_IMX_CONTAINER)) {
> - struct spl_load_info load;
> -
> - load.dev = flash;
> - load.priv = NULL;
> - load.filename = NULL;
> - load.bl_len = 1;
> - load.read = spl_spi_fit_read;
> -
> - err = spl_load_imx_container(spl_image, &load,
> -  payload_offs);
> - } else {
> - err = spl_parse_image_header(spl_image, bootdev, 
> header);
> - if (err)
> - retu

Re: [PATCH v5 09/11] spl: Convert semihosting to spl_load

[Xavier Drudis Ferran]

El Mon, Jul 31, 2023 at 06:43:01PM -0400, Sean Anderson deia:
> This converts the semihosting load method to use spl_load. As a result, it
> also adds support for LOAD_FIT_FULL and IMX images.
> 
> Signed-off-by: Sean Anderson 
> ---
> 
> Changes in v5:
> - Rework to load header in spl_load
> 
> Changes in v2:
> - New
> 
>  common/spl/spl_semihosting.c | 43 
>  1 file changed, 14 insertions(+), 29 deletions(-)
> 
> diff --git a/common/spl/spl_semihosting.c b/common/spl/spl_semihosting.c
> index 5b5e842a11..e7cb9f4ce6 100644
> --- a/common/spl/spl_semihosting.c
> +++ b/common/spl/spl_semihosting.c
> @@ -9,16 +9,16 @@
>  #include 
>  #include 
>  
> -static int smh_read_full(long fd, void *memp, size_t len)
> +static ulong spl_smh_fit_read(struct spl_load_info *load, ulong sector,
> +   ulong count, void *buf)
>  {
> - long read;
> + int ret, fd = *(int *)load->priv;
>

should ret be long to hold smh_read() return value ?

> - read = smh_read(fd, memp, len);
> - if (read < 0)
> - return read;
> - if (read != len)
> - return -EIO;
> - return 0;
> + if (smh_seek(fd, sector))
> + return 0;
> +


 
I never used smh, but why would this not be :

+   ret = smh_seek(fd, sector);
+   if (ret)
+   return ret;

(why does smh_seek(), smh_write(), smh_open(), smh_close() return
long, by the way? the implementations return either 0 or smh_errno(),
so int would do ?)
   
> + ret = smh_read(fd, buf, count);
> + return ret < 0 ? 0 : ret;
>  }
>  
>  static int spl_smh_load_image(struct spl_image_info *spl_image,
> @@ -27,14 +27,17 @@ static int spl_smh_load_image(struct spl_image_info 
> *spl_image,
>   const char *filename = CONFIG_SPL_FS_LOAD_PAYLOAD_NAME;
>   int ret;
>   long fd, len;
> - struct legacy_img_hdr *header =
> - spl_get_load_buffer(-sizeof(*header), sizeof(*header));
> + struct spl_load_info load = {
> + .bl_len = 1,
> + .read = spl_smh_fit_read,
> + };
>  
>   fd = smh_open(filename, MODE_READ | MODE_BINARY);
>   if (fd < 0) {
>   log_debug("could not open %s: %ld\n", filename, fd);
>   return fd;
>   }
> + load.priv = &fd;
>  
>   ret = smh_flen(fd);
>   if (ret < 0) {
> @@ -43,25 +46,7 @@ static int spl_smh_load_image(struct spl_image_info 
> *spl_image,
>   }
>   len = ret;
>  
> - ret = smh_read_full(fd, header, sizeof(struct legacy_img_hdr));
> - if (ret) {
> - log_debug("could not read image header: %d\n", ret);
> - goto out;
> - }
> -
> - ret = spl_parse_image_header(spl_image, bootdev, header);
> - if (ret) {
> - log_debug("failed to parse image header: %d\n", ret);
> - goto out;
> - }
> -
> - ret = smh_seek(fd, 0);
> - if (ret) {
> - log_debug("could not seek to start of image: %d\n", ret);
> - goto out;
> - }
> -
> - ret = smh_read_full(fd, (void *)spl_image->load_addr, len);
> + ret = spl_load(spl_image, bootdev, &load, len, 0);
>   if (ret)
>   log_debug("could not read %s: %d\n", filename, ret);
>  out:
> -- 
> 2.40.1
> 

Re: [PATCH v5 08/11] spl: Convert nor to spl_load

[Xavier Drudis Ferran]

El Mon, Jul 31, 2023 at 06:43:00PM -0400, Sean Anderson deia:
> This converts the nor load method to use spl_load. As a result it also
> adds support for LOAD_FIT_FULL.
> 
> Signed-off-by: Sean Anderson 
> ---
> 
> Changes in v5:
> - Rework to load header in spl_load
> 
>  common/spl/spl_nor.c | 41 +++--
>  1 file changed, 7 insertions(+), 34 deletions(-)
> 
> diff --git a/common/spl/spl_nor.c b/common/spl/spl_nor.c
> index 5b65b96a77..7328a87024 100644
> --- a/common/spl/spl_nor.c
> +++ b/common/spl/spl_nor.c
> @@ -26,8 +26,10 @@ unsigned long __weak spl_nor_get_uboot_base(void)
>  static int spl_nor_load_image(struct spl_image_info *spl_image,
> struct spl_boot_device *bootdev)
>  {
> - __maybe_unused const struct legacy_img_hdr *header;
> - __maybe_unused struct spl_load_info load;
> + struct spl_load_info load = {
> + .bl_len = 1,
> + .read = spl_nor_load_read,
> + };
>  
>   /*
>* Loading of the payload to SDRAM is done with skipping of
> @@ -41,7 +43,8 @@ static int spl_nor_load_image(struct spl_image_info 
> *spl_image,
>* Load Linux from its location in NOR flash to its defined
>* location in SDRAM
>*/
> - header = (const struct legacy_img_hdr *)CONFIG_SYS_OS_BASE;
> + const struct legacy_img_hdr *header =
> + (const struct legacy_img_hdr *)CONFIG_SYS_OS_BASE;
>  #ifdef CONFIG_SPL_LOAD_FIT
>   if (image_get_magic(header) == FDT_MAGIC) {
>   int ret;
> @@ -91,36 +94,6 @@ static int spl_nor_load_image(struct spl_image_info 
> *spl_image,
>* Load real U-Boot from its location in NOR flash to its
>* defined location in SDRAM
>*/
> -#ifdef CONFIG_SPL_LOAD_FIT
> - header = (const struct legacy_img_hdr *)spl_nor_get_uboot_base();
> - if (image_get_magic(header) == FDT_MAGIC) {
> - debug("Found FIT format U-Boot\n");
> - load.bl_len = 1;
> - load.read = spl_nor_load_read;
> - return spl_load_simple_fit(spl_image, &load,
> -spl_nor_get_uboot_base(),
> -(void *)header);

this loaded the simple fit from sector=CFG_SYS_UBOOT_BASE and now we'll
call spl_load with sector=0 ?

But spl_nor_get_uboot_base() is overriden by calculations of
concatenated images in

arch/arm/mach-imx/image-container.c
arch/mips/mach-mtmips/mt7621/spl/spl.c
arch/mips/mach-mtmips/spl.c

> - }
> -#endif
> - if (IS_ENABLED(CONFIG_SPL_LOAD_IMX_CONTAINER)) {
> - load.bl_len = 1;
> - load.read = spl_nor_load_read;
> - return spl_load_imx_container(spl_image, &load,
> -   spl_nor_get_uboot_base());

this loaded the imx image from sector=CFG_SYS_UBOOT_BASE or whatever
spl_nor_get_uboot_base() gave and now we'll call spl_load with
sector=0 ?

> - }
> -
> - /* Legacy image handling */
> - if (IS_ENABLED(CONFIG_SPL_LEGACY_IMAGE_FORMAT)) {
> - struct legacy_img_hdr hdr;
> -
> - load.bl_len = 1;
> - load.read = spl_nor_load_read;
> - spl_nor_load_read(&load, spl_nor_get_uboot_base(), sizeof(hdr), 
> &hdr);
> - return spl_load_legacy_img(spl_image, bootdev, &load,
> -spl_nor_get_uboot_base(),
> -&hdr);

This loaded legacy image with potential lzma decompression and now
compressed images are no longer supported ?

> - }
> -
> - return -EINVAL;
> + return spl_load(spl_image, bootdev, &load, 0, 0);

maybe better

+   return spl_load(spl_image, bootdev, &load, 0, spl_nor_get_uboot_base());


and consider calling spl_load_legacy_img(spl_image, bootdev, &info, offset, 
header)
from spl_load()?

>  }
>  SPL_LOAD_IMAGE_METHOD("NOR", 0, BOOT_DEVICE_NOR, spl_nor_load_image);
> -- 
> 2.40.1
> 

Re: [PATCH v5 07/11] spl: Convert NVMe to spl_load

[Xavier Drudis Ferran]

El Mon, Jul 31, 2023 at 06:42:59PM -0400, Sean Anderson deia:
> This converts the blk load method (used exclusively by NVMe) to use
> spl_load. As a consequence, it also adds support for LOAD_FIT_FULL and
> IMX images.
> 
> Signed-off-by: Sean Anderson 
> ---
> 
> Changes in v5:
> - New
> 
>  common/spl/spl_blk_fs.c | 68 -
>  1 file changed, 12 insertions(+), 56 deletions(-)
> 
> diff --git a/common/spl/spl_blk_fs.c b/common/spl/spl_blk_fs.c
> index d97adc4d39..eb5775a470 100644
> --- a/common/spl/spl_blk_fs.c
> +++ b/common/spl/spl_blk_fs.c
> @@ -45,24 +45,28 @@ int spl_blk_load_image(struct spl_image_info *spl_image,
>  {
>   const char *filename = CONFIG_SPL_PAYLOAD;
>   struct disk_partition part_info = {};
> - struct legacy_img_hdr *header;
>   struct blk_desc *blk_desc;
> - loff_t actlen, filesize;
> + loff_t filesize;
>   struct blk_dev dev;
>   int ret;
> + struct spl_load_info load = {
> + .read = spl_fit_read,
> + .bl_len = 1,
> + .filename = filename,
> + .priv = &dev,
> + };
>  
>   blk_desc = blk_get_devnum_by_uclass_id(uclass_id, devnum);
>   if (!blk_desc) {
>   printf("blk desc for %d %d not found\n", uclass_id, devnum);
> - goto out;
> + return ret;

What do you mean return ret ?
ret is unused yet ?
maybe return -1 or return -ENODEV or something ?

I think the error was already there before your patch, but maybe worth
fixing ?

>   }
>  
>   blk_show_device(uclass_id, devnum);
> - header = spl_get_load_buffer(-sizeof(*header), sizeof(*header));
>   ret = part_get_info(blk_desc, 1, &part_info);
>   if (ret) {
>   printf("spl: no partition table found. Err - %d\n", ret);
> - goto out;
> + return ret;
>   }
>  
>   dev.ifname = blk_get_uclass_name(uclass_id);
> @@ -72,63 +76,15 @@ int spl_blk_load_image(struct spl_image_info *spl_image,
>   if (ret) {
>   printf("spl: unable to set blk_dev %s %s. Err - %d\n",
>  dev.ifname, dev.dev_part_str, ret);
> - goto out;
> - }
> -
> - ret = fs_read(filename, (ulong)header, 0,
> -   sizeof(struct legacy_img_hdr), &actlen);
> - if (ret) {
> - printf("spl: unable to read file %s. Err - %d\n", filename,
> -ret);
> - goto out;
> - }
> -
> - if (IS_ENABLED(CONFIG_SPL_LOAD_FIT) &&
> - image_get_magic(header) == FDT_MAGIC) {
> - struct spl_load_info load;
> -
> - debug("Found FIT\n");
> - load.read = spl_fit_read;
> - load.bl_len = 1;
> - load.filename = (void *)filename;
> - load.priv = &dev;
> -
> - return spl_load_simple_fit(spl_image, &load, 0, header);
> - }
> -
> - ret = spl_parse_image_header(spl_image, bootdev, header);
> - if (ret) {
> - printf("spl: unable to parse image header. Err - %d\n",
> -ret);
> - goto out;
> - }
> -
> - ret = fs_set_blk_dev(dev.ifname, dev.dev_part_str, FS_TYPE_ANY);
> - if (ret) {
> - printf("spl: unable to set blk_dev %s %s. Err - %d\n",
> -dev.ifname, dev.dev_part_str, ret);
> - goto out;
> + return ret;
>   }
>  
>   ret = fs_size(filename, &filesize);
>   if (ret) {
>   printf("spl: unable to get file size: %s. Err - %d\n",
>  filename, ret);
> - goto out;
> + return ret;
>   }
>  
> - ret = fs_set_blk_dev(dev.ifname, dev.dev_part_str, FS_TYPE_ANY);
> - if (ret) {
> - printf("spl: unable to set blk_dev %s %s. Err - %d\n",
> -dev.ifname, dev.dev_part_str, ret);
> - goto out;
> - }
> -
> - ret = fs_read(filename, (ulong)spl_image->load_addr, 0, filesize,
> -   &actlen);
> - if (ret)
> - printf("spl: unable to read file %s. Err - %d\n",
> -filename, ret);
> -out:
> - return ret;
> + return spl_load(spl_image, bootdev, &load, filesize, 0);
>  }
> -- 
> 2.40.1
> 

Re: [PATCH v5 05/11] spl: Convert mmc to spl_load

[Xavier Drudis Ferran]

El Mon, Jul 31, 2023 at 06:42:57PM -0400, Sean Anderson deia:
> This converts the mmc loader to spl_load. Legacy images are handled by
> spl_load (via spl_parse_image_header), so mmc_load_legacy can be
> omitted.
>

Yes. I haven't used the legacy case, but by looking at the code, it
seems to me that mmc_load_legacy left the image at some mapped memory
at [ spl_image->load_addr,   spl_image->load_addr + size )
and the new code does too, but when the image is not aligned, the
memory that gets written to
was at [ spl_image->load_addr,   spl_image->load_addr + size + 
spl_image->offset % mmc->read_bl_len )
and it will
be at [ spl_image->load_addr - spl_image->offset % mmc->read_bl_len,   
spl_image->load_addr + size )
after this patch.

Meaning both with or without this patch some memory outside the image
gets written when the image is not aligned on media, but before it was
some part of a block at the end and now is that part before the
beginning.

Whether that's better or worse I don't know. It depends on whether
it's a problem to write in non mapped memory, whether there's
something worth preserving there, and whether some SOC has some sort
of special behaving memory just there, like it happened with the issue
Jerome Forissier found (note in this case it wasn't legacy, it was
simple fit)

https://patchwork.ozlabs.org/project/uboot/patch/c941d835a85255ff81a21c72069c3a9fc9a8a255.1656489154.git.jerome.foriss...@linaro.org/
  
> Signed-off-by: Sean Anderson 
> ---
> 
> Changes in v5:
> - Rework to load header in spl_load
> 
>  common/spl/spl_mmc.c | 91 
>  1 file changed, 8 insertions(+), 83 deletions(-)
> 
> diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
> index a665091b00..c926a42c0f 100644
> --- a/common/spl/spl_mmc.c
> +++ b/common/spl/spl_mmc.c
> @@ -17,48 +17,6 @@
>  #include 
>  #include 
>  
> -static int mmc_load_legacy(struct spl_image_info *spl_image,
> -struct spl_boot_device *bootdev,
> -struct mmc *mmc,
> -ulong sector, struct legacy_img_hdr *header)
> -{
> - u32 image_offset_sectors;
> - u32 image_size_sectors;
> - unsigned long count;
> - u32 image_offset;
> - int ret;
> -
> - ret = spl_parse_image_header(spl_image, bootdev, header);
> - if (ret)
> - return ret;
> -
> - /* convert offset to sectors - round down */
> - image_offset_sectors = spl_image->offset / mmc->read_bl_len;
> - /* calculate remaining offset */
> - image_offset = spl_image->offset % mmc->read_bl_len;
> -
> - /* convert size to sectors - round up */
> - image_size_sectors = (spl_image->size + mmc->read_bl_len - 1) /
> -  mmc->read_bl_len;
> -
> - /* Read the header too to avoid extra memcpy */
> - count = blk_dread(mmc_get_blk_desc(mmc),
> -   sector + image_offset_sectors,
> -   image_size_sectors,
> -   (void *)(ulong)spl_image->load_addr);
> - debug("read %x sectors to %lx\n", image_size_sectors,
> -   spl_image->load_addr);
> - if (count != image_size_sectors)
> - return -EIO;
> -
> - if (image_offset)
> - memmove((void *)(ulong)spl_image->load_addr,
> - (void *)(ulong)spl_image->load_addr + image_offset,
> - spl_image->size);
> -
> - return 0;
> -}
> -
>  static ulong h_spl_load_read(struct spl_load_info *load, ulong sector,
>ulong count, void *buf)
>  {
> @@ -82,47 +40,14 @@ int mmc_load_image_raw_sector(struct spl_image_info 
> *spl_image,
> struct spl_boot_device *bootdev,
> struct mmc *mmc, unsigned long sector)
>  {
> - unsigned long count;
> - struct legacy_img_hdr *header;
> - struct blk_desc *bd = mmc_get_blk_desc(mmc);
> - int ret = 0;
> -
> - header = spl_get_load_buffer(-sizeof(*header), bd->blksz);
> -
> - /* read image header to find the image size & load address */
> - count = blk_dread(bd, sector, 1, header);
> - debug("hdr read sector %lx, count=%lu\n", sector, count);
> - if (count == 0) {
> - ret = -EIO;
> - goto end;
> - }
> -
> - if (IS_ENABLED(CONFIG_SPL_LOAD_FIT) &&
> - image_get_magic(header) == FDT_MAGIC) {
> - struct spl_load_info load;
> -
> - debug("Found FIT\n");
> - load.dev = mmc;
> - load.priv = NULL;
> - load.filename = NULL;
> - load.bl_len = mmc->read_bl_len;
> - load.read = h_spl_load_read;
> - ret = spl_load_simple_fit(spl_image, &load, sector, header);
> - } else if (IS_ENABLED(CONFIG_SPL_LOAD_IMX_CONTAINER)) {
> - struct spl_load_info load;
> -
> - load.dev = mmc;
> - 

Re: [PATCH v5 04/11] spl: Convert fat to spl_load

[Xavier Drudis Ferran]

El Mon, Jul 31, 2023 at 06:42:56PM -0400, Sean Anderson deia:
> This converts the fat loader to use spl_load. Some platforms are very
> tight on space, so we take care to only include the code we really need.
> 
> Signed-off-by: Sean Anderson 
> ---
> 
> Changes in v5:
> - Rework to load header in spl_load
> 
> Changes in v3:
> - Fix failing on success
> 
>  common/spl/spl_fat.c | 55 +++-
>  1 file changed, 19 insertions(+), 36 deletions(-)
> 
> diff --git a/common/spl/spl_fat.c b/common/spl/spl_fat.c
> index f8a5b80a3b..6530bcd5a7 100644
> --- a/common/spl/spl_fat.c
> +++ b/common/spl/spl_fat.c
> @@ -60,57 +60,40 @@ int spl_load_image_fat(struct spl_image_info *spl_image,
>  const char *filename)
>  {
>   int err;
> - struct legacy_img_hdr *header;
> + loff_t size;
> + struct spl_load_info load;
> +
> + /* This generates smaller code than using a compound literal */
> + load.read = spl_fit_read;
> + load.bl_len = 1;
> + load.filename = filename;
>  
>   err = spl_register_fat_device(block_dev, partition);
>   if (err)
>   goto end;
>  
> - header = spl_get_load_buffer(-sizeof(*header), sizeof(*header));
> -
> - err = file_fat_read(filename, header, sizeof(struct legacy_img_hdr));
> - if (err <= 0)
> - goto end;
> -
> - if (IS_ENABLED(CONFIG_SPL_LOAD_FIT_FULL) &&
> - image_get_magic(header) == FDT_MAGIC) {
> - err = file_fat_read(filename, (void *)CONFIG_SYS_LOAD_ADDR, 0);
> - if (err <= 0)
> - goto end;
> - err = spl_parse_image_header(spl_image, bootdev,
> - (struct legacy_img_hdr *)CONFIG_SYS_LOAD_ADDR);

So this used to load to CONFIG_SYS_LOAD_ADDR and will now load to
CONFIG_TEXT_BASE, or whatever the applicable spl_get_load_buffer()
uses. Is this OK ? or do we need a new parameter for the buffer or
something ?

> - if (err == -EAGAIN)
> - return err;
> - if (err == 0)
> - err = 1;
> - } else if (IS_ENABLED(CONFIG_SPL_LOAD_FIT) &&
> - image_get_magic(header) == FDT_MAGIC) {
> - struct spl_load_info load;
> -
> - debug("Found FIT\n");
> - load.read = spl_fit_read;
> - load.bl_len = 1;
> - load.filename = (void *)filename;
> - load.priv = NULL;
> -
> - return spl_load_simple_fit(spl_image, &load, 0, header);
> - } else {
> - err = spl_parse_image_header(spl_image, bootdev, header);
> + /*
> +  * Avoid pulling in this function for other image types since we are
> +  * very short on space on some boards.
> +  */
> + if (IS_ENABLED(CONFIG_SPL_LOAD_FIT_FULL)) {
> + err = fat_size(filename, &size);
>   if (err)
>   goto end;
> -
> - err = file_fat_read(filename,
> - (u8 *)(uintptr_t)spl_image->load_addr, 0);
> + } else {
> + size = 0;
>   }
>  
> + err = spl_load(spl_image, bootdev, &load, 0, size);
> +
>  end:
>  #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
> - if (err <= 0)
> + if (err < 0)
>   printf("%s: error reading image %s, err - %d\n",
>  __func__, filename, err);
>  #endif
>  
> - return (err <= 0);
> + return err;
>  }
>  
>  #if CONFIG_IS_ENABLED(OS_BOOT)
> -- 
> 2.40.1
> 

Re: mail --> mutt

[Xavier Drudis Ferran]

El Fri, Jul 07, 2023 at 01:39:30PM +, Ernest Adrogué deia:
> 2023-07- 7, 09:59 (+0200); Narcis Garcia escriu:
> > Em sembla que l'Alex es referia a aquest recorregut per a «smarthost»:
> > MUA (Mutt) -> MTA local -> MTA públic
> > 
> > Amb això, el servidor públic de correu-e és el què ha d'acreditar-se a
> > Internet com a fiable per als filtres anti-brossa.
> 
> En aquest cas ho trobo una complicació innecessària.
> 
> L'MUA es pot connectar directament a l'MTA públic, sense necessitat de
> passar per l'MTA local, que no aporta res, o pràcticament res.
> 

Si ho tens clar ja ho tens clar.

Però un MTA local pot aportar cues (esperar a poder enviar) i
lliurament a adreces locals que no existeixen al MTA públic.

Segur que li pòts buscar més coses, però aquestes són les principals
que em passen pel cap. Crec que l'Alex ja anava per aquí.

Si això no et convé, o no et compensa, no tens perquè tenir MTA local,
com menys coses millor, només és una idea que t'han donat.

Re: [PATCH 04/10] spl: fit: support for booting a GZIP-compressed U-boot raw binary

[Xavier Drudis Ferran]

El Fri, Jun 30, 2023 at 05:41:40PM +0530, Manoj Sai deia:
> If GZIP Compression support is enabled, GZIP compressed U-Boot raw binary will
> be at a specified RAM location which is defined at
> UBOOT_COMPRESSED_BINARY_FIT_USER_DEF_ADDR  and will be assign it as
> the source address.
> 
> gunzip function in spl_load_fit_image ,will decompress the GZIP compressed
> U-Boot raw binary which is placed at source address to the default
> CONFIG_SYS_TEXT_BASE location.
> 
> spl_load_fit_image function will load the decompressed U-Boot
> raw binary, which is placed at the CONFIG_SYS_TEXT_BASE location.
> 
> Signed-off-by: Manoj Sai 
> Signed-off-by: Suniel Mahesh 
> ---
>  common/spl/spl_fit.c | 20 ++--
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
> index 730639f756..e2101099ef 100644
> --- a/common/spl/spl_fit.c
> +++ b/common/spl/spl_fit.c
> @@ -281,7 +281,12 @@ static int spl_load_fit_image(struct spl_load_info 
> *info, ulong sector,
>   return 0;
>   }
>  
> - src_ptr = map_sysmem(ALIGN(load_addr, ARCH_DMA_MINALIGN), len);
> + if ((IS_ENABLED(CONFIG_SPL_GZIP) && image_comp == 
> IH_COMP_GZIP)) {
> + src_ptr = 
> map_sysmem(ALIGN(CONFIG_VAL(UBOOT_COMPRESSED_BINARY_FIT_USER_DEF_ADDR),
> + ARCH_DMA_MINALIGN), len);
> + } else {
> + src_ptr = map_sysmem(ALIGN(load_addr, 
> ARCH_DMA_MINALIGN), len);
> + }
>   length = len;
>  
>   overhead = get_aligned_image_overhead(info, offset);
> @@ -319,11 +324,14 @@ static int spl_load_fit_image(struct spl_load_info 
> *info, ulong sector,
>   board_fit_image_post_process(fit, node, &src, &length);
>  
>   load_ptr = map_sysmem(load_addr, length);
> - if (IS_ENABLED(CONFIG_SPL_GZIP) && image_comp == IH_COMP_GZIP) {
> - size = length;
> - if (gunzip(load_ptr, CONFIG_SYS_BOOTM_LEN, src, &size)) {
> - puts("Uncompressing error\n");
> - return -EIO;
> +
> + if ((IS_ENABLED(CONFIG_SPL_GZIP) && image_comp == IH_COMP_GZIP)) {
> + if ((IS_ENABLED(CONFIG_SPL_GZIP) && image_comp == 
> IH_COMP_GZIP)) {

You just repeated the same condition in a new if here ?

> + size = length;
> + if (gunzip(load_ptr, CONFIG_SYS_BOOTM_LEN, src, &size)) 
> {
> + puts("Uncompressing error\n");
> + return -EIO;
> + }
>   }
>   length = size;
>   } else {
> -- 
> 2.25.1
> 

[PATCH v2] cmd: usb: Prevent reset in usb tree/info command

[Xavier Drudis Ferran]

   Commands causing reset in some configs:

When bootflow scan is run, this will cause a UCLASS_BOOTDEV device to
be added as sibling of those UCLASS_BLK devices found in the search
chain defined in environment variable "boot_targets", until boot
succeeds from some device. This can happen automatically as part of
the default boot process on some boards (example: Rock Pi 4) depending
on the board configuration (DISTRO_DEFAULTS, BOOTSTD, BOOTCOMMAND,
etc.) because they have bootcmd=bootflow scan.

If boot doesn't succeed from any device, and usb is in boot_targets,
and an usb storage device is plugged to some usb port at boot time,
its UCLASS_MASS_STORAGE device will have a UCLASS_BOOTDEV device as
child, besides a UCLASS_BLK child.

If once the boot fails the user enters at the U-Boot shell prompt:

usb info

or

usb tree

The code in cmd/usb.c will eventually recurse into the UCLASS_BOOTDEV
device and pass a null usb_device pointer to usb_show_tree_graph() or
usb_show_info() (because it has no parent_priv_).

This causes a reset. The expected behaviour would be to ignore the
UCLASS_BOOTDEV device, continue listing the usb information and return
to the prompt.


   Minimal test:

Another way to trigger this reset as a minimal test or on boards with
a different bootcmd would be:

- make sure "usb" is in environment variable boot_targets (might need
  setenv boot_targets usb; and/or saveenv and reset), then, with a usb
  storage device plugged to a usb port, run:

=> usb reset ; bootflow scan ; usb info


   Solution:

Fix it (twice) by checking for null parent_priv_ and adding
UCLASS_BOOTDEV to the list of ignored class ids before the recursive
call.

This prevents the current particular problem with UCLASS_BOOTDEV, even
in case it ever gets some parent_priv_ struct which is not an
usb_device, despite being the child of a usb_device->dev. And it also
prevents possible future problems if other children are added to usb
devices that don't have parent_priv_ because they are not part of the
usb tree, just abstractions of functionality (like UCLASS_BLK and
UCLASS_BOOTDEV are now).


Signed-off-by: Xavier Drudis Ferran 
Reviewed-by: Simon Glass 
Reviewed-by: Marek Vasut 
Tested-by: Marek Vasut 

---

v2: added UCLASS_BOOTDEV check (discussion of v1 dried up without much
evident consensus, so hopefully Simon Glass likes it better now)
[ 
https://patchwork.ozlabs.org/project/uboot/patch/zh39sva1vbzr3...@xdrudis.tinet.cat/
 ]
Improved the commit message trying to follow Marek Vasut's advice.

---
 cmd/usb.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/cmd/usb.c b/cmd/usb.c
index 6193728384..23253f2223 100644
--- a/cmd/usb.c
+++ b/cmd/usb.c
@@ -421,7 +421,9 @@ static void usb_show_tree_graph(struct usb_device *dev, 
char *pre)
 * Ignore emulators and block child devices, we only want
 * real devices
 */
-   if ((device_get_uclass_id(child) != UCLASS_USB_EMUL) &&
+   if (udev &&
+   (device_get_uclass_id(child) != UCLASS_BOOTDEV) &&
+   (device_get_uclass_id(child) != UCLASS_USB_EMUL) &&
(device_get_uclass_id(child) != UCLASS_BLK)) {
usb_show_tree_graph(udev, pre);
pre[index] = 0;
@@ -604,10 +606,12 @@ static void usb_show_info(struct usb_device *udev)
 child;
 device_find_next_child(&child)) {
if (device_active(child) &&
+   (device_get_uclass_id(child) != UCLASS_BOOTDEV) &&
(device_get_uclass_id(child) != UCLASS_USB_EMUL) &&
(device_get_uclass_id(child) != UCLASS_BLK)) {
udev = dev_get_parent_priv(child);
-   usb_show_info(udev);
+   if (udev)
+   usb_show_info(udev);
}
}
 }
-- 
2.20.1

Re: [SPAM] Re: [PATCH] cmd: usb: Prevent reset in usb tree/info command

[Xavier Drudis Ferran]

El Tue, Jun 20, 2023 at 11:03:57AM +0100, Simon Glass deia:
> Hi Xavier,
>

Hi Simon,

> >
> > It is also possible that one day a device that is not UCLASS_BLK,
> > UCLASS_BOOTDEV or UCLASS_USB_EMUL is put as children of a usb storage
> > device (just imagine a future system similar to bootstd for firmware
> > updates, trust material, etc.). Is it likely to have a struct in
> > parent_priv_ that is not a usb_device ?
> >
> > So which is more likely to survive future changes ?
> >
> > - checking for parent_priv_ not null and not UCLASS_USB_EMUL
> >
> > - checking for parent_priv_ not null and not UCLASS_USB_EMUL and not 
> > UCLASS_BLK
> >   (my patch, overcautious ?)
> >
> > - checking for not (UCLASS_BLK, UCLASS_BOOTDEV or UCLASS_USB_EMUL)
> >   (Simon Glass' idea)
> >
> > - checking for not  UCLASS_BLK and not UCLASS_BOOTDEV and not 
> > UCLASS_USB_EMUL
> >and parent_priv_ not null
> 
> Really the parent_priv thing is a separate issue, a side effect of
> something having a UCLASS_USB parent.
>

I don't think it's a separate issue. If parent_priv is present it
could be a usb_device (most likely) or not, but if it's null there's
no way the recursive call can succeed.

> The key point here is that we cannot iterate down into a bootdev
> device looking for USB devices. So we should use that as the check,
> since it is the most direct check.
>

But things keep appearing that have a UCLASS_USB* parent and no
parent_priv.
in 2017 Suneel Garapati already fixed the issue of UCLASS_BLK
being a child of a device. Now it's UCLASS_BOOTDEV, and tomorrow
may be something else.

The most direct check will miss future cases as the devices tend to
become more abstract instead of mapping one to one to physical stuff.

> 
> >From my memory, I think you can check for a USB hub instead, but I'm
> not completely sure.
>

On second thoughts I didn't find it so easy. There's the root hub,
UCLASS_USB, I think and a UCLASS_USB_EMUL may also be a hub, so I
don't know anymore how more elegant that could be, so I left it be.

> I suggest adding a test for the command (see test/dm/acpi.c for an
> example) since a test is the best way to ensure this doesn't happen
> again.
>

Makes sense. But I don't have any more time for that, sorry.

I think we'll have to leave it at this unless someone else has the time.

Bye.

Re: [SPAM] Re: [PATCH] cmd: usb: Prevent reset in usb tree/info command

[Xavier Drudis Ferran]

El Tue, Jun 20, 2023 at 11:49:36AM +0200, Marek Vasut deia:
> 
> Default, see:
> $ git grep CONFIG_BOOTCOMMAND configs/
>

I'm lost.

I called default what Kconfig used as default.
You seem to call default what's in board specific config files.
Whatever. Fix the wording in the commit message if you like.


> 
> So, what is the minimal test case ?
> I have been asking for that for a while.
>

I sent a minimal test case last week.

https://lists.denx.de/pipermail/u-boot/2023-June/520109.html

You build for a Rock Pi 4 board, boot with usb stick and no boot media
and run usb info and you get a reset.

I won't send it again because I can't guess what you consider minimal.

> 
> I would really like a minimal test case. Empty your env and figure out the
> commands which need to be executed to trigger this. Without any interference
> from other commands/scripting/...
>

I'm sorry but if what I sent isn't enough I don't think I'll have time
to help you any further. Find your minimal test case yourself or
ignore my patch.

> > If it's just that you can't reproduce it, can you try to ?:
> > 
> > - set up a board with no boot media (I tested like this but it might
> >not be needed),
> > 
> > - put usb in boot_targets (if you put only usb there you may not need
> >the previous step):
> >setenv boot_targets usb
> 
> Here you assume distro bootcommand or some such . Can we remove that
> assumption ? (I think yes, and we should)
>

I don't think I'm assuming anything about bootcommand. That's
precisely why I wrote these steps instead of the "just boot a Rock Pi
4" scenario last week.

The commit message talks about bootcmd because it justifies
that the bootflow scan will be called automatically in some cases,
so the bug has more impact that it would otherwise have.

But the bug should appear whether or not you have bootcmd.
The bug should be an interaction between what bootflow scan does
and what usb info or usb tree do.

I'm assuming bootflow reads the boot_targets environment variable to
know where it searches for boot devices, and therefore to which
devices it will attach a UCLASS_BOOTDEV child to some devices, in
particular to usb mass storage devices if any is present, that will
later break usb info/usb tree. Whether bootflow is called from bootcmd
or not should be irrelevant.

If you follow the code from the bootflow command you may find
yourself that the boot_targets variable is involved. I did it
last week or sometime and won't do it again now for you, sorry.
I know I may have misunderstood something, and I'm sorry for the
noise if so.

> > - plug a non-booting usb mass storage device to an usb port,
> > 
> > - run usb reset in case you already had usb initialized at boot, or
> >skip this if usb is not initialized yet. If in doubt, run usb reset.
> > 
> > - run bootflow scan
> > 
> > - run usb info
> > 
> > It should list some devices, but give you a reset just after listing the
> > usb mass storage device without my patch, and it should just list all
> > usb devices and go back to the prompt with my patch.
> 
> Does it crash if you empty your env and run simply
> 
> => usb reset ; bootflow scan ; usb info
> 
> ?

I guess it won't crash if environment var boot_targets is absent or
empty.  Or even if it's full but has no "usb" in it. But I'm not sure
anymore at what time the variable is read, so it might be that
emptying it when bootflow structures are already set up wouldn't
change things, I don't know, I don't remember.

But I won't try it, sorry.

I found a bug, I sent a 4 line patch. Since I didn't justify it
enough I sent followup mails on how to reproduce.
This week I sent a second version, with redundant measures to seek
consensus. It's a 6 line patch or 8 or whatever.
I didn't write bootflow, and I didn't write cmd/usb.c

And I don't have time to keep writing long emails. I'm sorry.  Not
even to count how many lines of text I wrote compared to the 8 lines
of code or whatever. If someone has the bureaucratic skills and
patience to pursue this further, they can do what they want with my
patch (under GPL2 as implied by the sign off). If not, you all can
keep your bugs, I won't try anymore to steal them.

Bye and sorry for any disturbances.

Re: [PATCH] cmd: usb: Prevent reset in usb tree/info command

[Xavier Drudis Ferran]

El Mon, Jun 19, 2023 at 11:49:18PM +0200, Marek Vasut deia:
> On 6/19/23 12:12, Xavier Drudis Ferran wrote:
> 
> It seems the email addresses are being constantly corrupted in each email.
> This time the ML address is wrong and missing an e at the end. There is some
> e@ nonexistent address which I have to keep removing.
>

Yes, that's my fault. I'm sorry. I apologize to you and others.  I
resent my mail with the proper address. Please just look for my mail
with the wrong address and delete it from your mail archive to prevent
further such problems. You can reply to the other mail I sent (June
19th), because it has the same content, just with an added
apology. Sorry again.

> > When DISTRO_DEFAULTS is not set, the default environment has
> > bootcmd=bootflow
> 
> That is not right, on $randomboard I picked the bootcmd is something else.
>

And how default is your default environment for your $randomboard ?

Almost half the configs/* redefine CONFIG_BOOTCOMMAND (524/1268)

When DISTRO_DEFAULTS is not set, that makes BOOTSTD_BOOTCOMMAND default to y
and the default for BOOTCMD is not "run distro_boootcmd", but "bootflow scan"
or (for sandbox) "bootflow scan -lb". When there's bootcmd at all.

But this is only the default for the default environment. It can be
overriden and the Kconfig is not exactly simple. An extract:

next branch:

arch/Arm/Kconfig:

config ARCH_ROCKCHIP
[...]
imply BOOTSTD_DEFAULTS
[...]


cmd/Kconfig:

config CMD_BOOTFLOW
bool "bootflow"
depends on BOOTSTD
default y
[...]

config CMD_BOOTFLOW_FULL
bool "bootflow - extract subcommands"
depends on BOOTSTD_FULL
default y
[...]

boot/Kconfig:

config BOOT_DEFAULTS
bool  # Common defaults for standard boot and distroboot
imply USE_BOOTCOMMAND
[...]

config BOOTSTD
bool "Standard boot support"
default y
[...]

config BOOTSTD_FULL
bool "Enhanced features for standard boot"
default y if SANDBOX
[...]


if BOOTSTD
[...]

config BOOTSTD_DEFAULTS
bool "Select some common defaults for standard boot"
depends on BOOTSTD
imply USE_BOOTCOMMAND
select BOOT_DEFAULTS
select BOOTMETH_DISTRO
[...]

config BOOTSTD_BOOTCOMMAND
bool "Use bootstd to boot"
default y if !DISTRO_DEFAULTS
[...]
[...]
endif
[...]
config DISTRO_DEFAULTS
bool "Select defaults suitable for booting general purpose Linux 
distributions"
select BOOT_DEFAULTS
[...]

config BOOTCOMMAND
string "bootcmd value"
depends on USE_BOOTCOMMAND && !USE_DEFAULT_ENV_FILE
default "bootflow scan -lb" if BOOTSTD_DEFAULTS && CMD_BOOTFLOW_FULL
default "bootflow scan" if BOOTSTD_DEFAULTS && !CMD_BOOTFLOW_FULL
default "run distro_bootcmd" if !BOOTSTD_BOOTCOMMAND && DISTRO_DEFAULTS
> 
> Does this happen if you set empty bootcmd ('=> setenv bootcmd 'echo hello'
> for example), then 'saveenv' , then 'reset' , then drop into U-Boot shell
> and run 'usb reset ; usb info' too ?
>

I haven't tested it. If bootflow scan is not run it might not happen.
Someone has to hang the UCLASS_BOOTDEV on the usb mass storage device,
for it to fail. But as far as I know the idea is to make bootflow the
default in more and more cases. You'll always be able to avoid it
running in your board by setting your own environment at runtime or
changing the configuration, yes, but what's the point ?

I thought that failing one scenario was enough to fix things.  When
one finds a bug it tries to help others to reproduce it.  When others
help the bug finder to run other scenarios that don't have the bug,
what's that useful for ?

Note that it won't fail if the boot succeeds, because then you won't
have a shell to run usb info/tree. It won't fail if usb is not in
boot_targets. It won't fail if there's no mass storage device
connected to usb when bootflow scan is run...

But I still think the failing case is worth fixing. Someone may be
wondering why bootflow fails, run usb info and find a reset, when
setting up a new board, or trying to boot from the wrong usb stick
after the system partition has been corrupted, or whatever. It's not
something that breaks any board in production, but it's not something
to leave forever broken. In theory a null pointer dereference might be
used by some attacker, but in this case I don't really see any useful
attack, maybe it's my lack of imagination. So I'm not claiming it's a
severe bug. It's just a normal bug that needs fixing when possible.

Or ar

Re: [SPAM] Re: [PATCH] cmd: usb: Prevent reset in usb tree/info command

[Xavier Drudis Ferran]

El Tue, Jun 20, 2023 at 02:50:48AM +0200, Marek Vasut deia:
> On 6/13/23 08:52, Xavier Drudis Ferran wrote:
> > 
> > U-Boot TPL 2023.07-rc2-00144-g497967f1ee (Jun 12 2023 - 11:15:47)
> 
> Next is already at rc4 , what's this rc2 ?

It's a test I did last week (June 12th). What do you think has changed
in next between rc2 and rc4 so that this shouldn't happen now ?

My last patch (sent yesterday) was tested with the next branch as of
yesterday morning. But you can test it yourself with any version you like.
Do I have to send the logs of each test I do to the list every time ?

I thought Simon Glass reply last week to the mail you quoted already
showed agreement that the issue exists, and the adding of the
UCLASS_BOOTDEV device is per design, so cmd/usb.c needs fixing to deal
with this change.

Re: [PATCH] phy: rockchip-inno-usb2: cleanup comments and make ops static

[Xavier Drudis Ferran]

El Mon, Jun 19, 2023 at 02:58:50PM +0530, Jagan Teki deia:
> On Mon, Jun 19, 2023 at 2:39 PM Marek Vasut  wrote:
> >
> > On 6/19/23 09:24, Xavier Drudis Ferran wrote:
> > > Thanks to Jagan Teki for noting that three functions should be static
> > > and comments from uclass_clk are redundant.

[...]

> >
> > Why is the documentation dropped ?
> 
> As these comments seem redundant as in clk-uclass.h definitions, so I
> asked to drop the same from the driver.
> 
> Xavier, can you add the details in the commit message?
> 
> Jagan.

Which details ? The commit message says "comments from uclass_clk are
redundant" . What should it say ? Do you want me to copy the deleted
comments to the commit message ??

[PATCH] cmd: usb: Prevent reset in usb tree/info command

[Xavier Drudis Ferran]

When DISTRO_DEFAULTS is not set, the default environment has
bootcmd=bootflow and this will cause a UCLASS_BOOTDEV device to be
added as sibling of those UCLASS_BLK devices in boot_targets, until
boot succeeds from some device. If none succeeds, and usb is in
boot_targets, and an usb storage device is plugged to some usb port at
boot time, its UCLASS_MASS_STORAGE device will have a UCLASS_BOOTDEV
device as child, besides a UCLASS_BLK child.

If once the boot fails the user enters at the U-Boot shell prompt:

usb info

or

usb tree

The code in cmd/usb.c will eventually recurse into the UCLASS_BOOTDEV
and pass a null pointer to usb_device (because it has no parent_priv_).
This causes a reset.

Fix it (twice) by checking for null parent_priv_ and adding
UCLASS_BOOTDEV to the list of ignored class ids before the recursive
call.

This prevents the current particular problem with UCLASS_BOOTDEV, even
in case it ever gets some parent_priv_ struct which is not an
usb_device, despite being the child of a usb_device->dev. And it also
prevents possible future problems if other children are added to usb
devices that don't have parent_priv_ because they are not part of the
usb tree, just abstractions of functionality (like UCLASS_BLK and
UCLASS_BOOTDEV are now).

Signed-off-by: Xavier Drudis Ferran 
---

v2: added UCLASS_BOOTDEV check (discussion of v1 dried up without much
evident consensus, so hopefully Simon Glass likes it better now)
[ 
https://patchwork.ozlabs.org/project/uboot/patch/zh39sva1vbzr3...@xdrudis.tinet.cat/
 ]
---

Apologies to the people in Cc: for resending this. I had a typo in the
email list address.

---
 cmd/usb.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/cmd/usb.c b/cmd/usb.c
index 6193728384..23253f2223 100644
--- a/cmd/usb.c
+++ b/cmd/usb.c
@@ -421,7 +421,9 @@ static void usb_show_tree_graph(struct usb_device *dev, 
char *pre)
 * Ignore emulators and block child devices, we only want
 * real devices
 */
-   if ((device_get_uclass_id(child) != UCLASS_USB_EMUL) &&
+   if (udev &&
+   (device_get_uclass_id(child) != UCLASS_BOOTDEV) &&
+   (device_get_uclass_id(child) != UCLASS_USB_EMUL) &&
(device_get_uclass_id(child) != UCLASS_BLK)) {
usb_show_tree_graph(udev, pre);
pre[index] = 0;
@@ -604,10 +606,12 @@ static void usb_show_info(struct usb_device *udev)
 child;
 device_find_next_child(&child)) {
if (device_active(child) &&
+   (device_get_uclass_id(child) != UCLASS_BOOTDEV) &&
(device_get_uclass_id(child) != UCLASS_USB_EMUL) &&
(device_get_uclass_id(child) != UCLASS_BLK)) {
udev = dev_get_parent_priv(child);
-   usb_show_info(udev);
+   if (udev)
+   usb_show_info(udev);
}
}
 }
-- 
2.20.1

[PATCH] phy: rockchip-inno-usb2: cleanup comments and make ops static

[Xavier Drudis Ferran]

Thanks to Jagan Teki for noting that three functions should be static
and comments from uclass_clk are redundant.

Signed-off-by: Xavier Drudis Ferran 
---
 drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 25 +++
 1 file changed, 3 insertions(+), 22 deletions(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c 
b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
index be5f79490c..2b33b6f03c 100644
--- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
+++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
@@ -182,25 +182,12 @@ static struct phy_ops rockchip_usb2phy_ops = {
.of_xlate = rockchip_usb2phy_of_xlate,
 };
 
-/**
- * round_rate() - Adjust a rate to the exact rate a clock can provide.
- * @clk:   The clock to manipulate.
- * @rate:  Desidered clock rate in Hz.
- *
- * Return: rounded rate in Hz, or -ve error code.
- */
-ulong rockchip_usb2phy_clk_round_rate(struct clk *clk, ulong rate)
+static ulong rockchip_usb2phy_clk_round_rate(struct clk *clk, ulong rate)
 {
return 48000;
 }
 
-/**
- * enable() - Enable a clock.
- * @clk:   The clock to manipulate.
- *
- * Return: zero on success, or -ve error code.
- */
-int rockchip_usb2phy_clk_enable(struct clk *clk)
+static int rockchip_usb2phy_clk_enable(struct clk *clk)
 {
struct udevice *parent = dev_get_parent(clk->dev);
struct rockchip_usb2phy *priv = dev_get_priv(parent);
@@ -217,13 +204,7 @@ int rockchip_usb2phy_clk_enable(struct clk *clk)
return 0;
 }
 
-/**
- * disable() - Disable a clock.
- * @clk:   The clock to manipulate.
- *
- * Return: zero on success, or -ve error code.
- */
-int rockchip_usb2phy_clk_disable(struct clk *clk)
+static int rockchip_usb2phy_clk_disable(struct clk *clk)
 {
struct udevice *parent = dev_get_parent(clk->dev);
struct rockchip_usb2phy *priv = dev_get_priv(parent);
-- 
2.20.1

Re: [PATCH v7 2/2] phy: rockchip-inno-usb2: Implement clock operations for usb2phy clock

[Xavier Drudis Ferran]

El Mon, Jun 19, 2023 at 12:04:51PM +0530, Jagan Teki deia:
> 
> Please merge these two asap. Better would these two be part of the
> coming release?
>

How do you mean ?

They're both in master and next now.

see commits:

e81512ac30c154c320b54036919cd3a6f4cc1516
40359c94405b103d25233d8d727d671748b751b9

Or do you mean I should send fixes to comments and static qualifiers
for 3 functions as you pointed out ?

https://lists.denx.de/pipermail/u-boot/2023-June/519708.html

I wasn't sure if that was a notice for me to do it better next time or
it required a clean up patch.

Re: mail --> mutt

[Xavier Drudis Ferran]

El Wed, Jun 14, 2023 at 11:02:36AM +0200, Jordi deia:
> Bon dia, ja sé que em donareu altres opcions però voldria redirigir
> determinats correus locals a mutt de forma automàtica.
> 
> És a dir correu llegit amb mail que compleixi determinades condicions,
> redirigir-lo a mutt. Em podeu orientar ??
> 
> 
> Salutaciosn 
> 
> Jordi. 
> 

No t'entenc ? Vols procmail o alguna cosa així ?
Vols moure correus de carpeta ? O enviar-los per smtp ? O ... ?

Re: [SPAM] Re: [PATCH] cmd: usb: Prevent reset in usb tree/info command

[Xavier Drudis Ferran]

Thanks for explaining, Simon Glass.

Can someone please stop me if I'm splitting hairs or bikeshedding or
something ? I guess we agree both checking for null or UCLASS_BOOTDEV
would work right now but we are looking for what's more future-proof.

El Tue, Jun 13, 2023 at 09:12:30PM +0100, Simon Glass deia:
> Hi Xavier,
> 
> On Tue, 13 Jun 2023 at 17:04, Xavier Drudis Ferran  wrote:
> >
> > El Tue, Jun 13, 2023 at 03:58:22PM +0100, Simon Glass deia:
> > >
> > > Yes that's right. So 'usb info' should ignore UCLASS_BOOTDEV devices.
> >
> > That's a possibility, yes. It should work until someone adds another
> > device there for some other purpose.
> >
> > > That is better than checking for the NULL pointer.
> > >
> >
> > Why? What's wrong about checking for null ?
> > Or maybe checking for both not null and  not UCLASS_BOOTDEV ?
> >
> > Not that I care too much, just to understand your reasoning.
> 
> Well, devices may have something attached there, so it may be
> non-NULL, but point to a different struct from the expected one.
>

Yes. That is possible. How likely ?

That "there" is dev_get_parent_priv(). If I understand the driver
model, those devices shouldn't put things there themselves, it should
be their parent who puts stuff there for them. And the parent should
be an usb_device->dev (because of the recursive code). So what other
struct would such a parent put in a child parent_priv_ ? Yes, whatever
it wants, but I mean, isn't it likely to assume the child would be
"adopted" with null as parent_priv_ or a "natural child" with a usb_device
in parent_priv_ ? 

I did a very rough search

grep -rIE 'UCLASS_(BLK|BOOTDEV|USB_EMUL)' . |grep -F .id |cut -f1 -d:  |xargs 
-n 1 grep -l per_child_auto 
./drivers/usb/emul/usb-emul-uclass.c

Which seems to suggest only UCLASS_USB_EMUL would have parent_priv_,
and that would be an usb_device, which the current code does not want
to recurse anyway. (dev_set_parent_priv() is almost never called).

It is also possible that one day a device that is not UCLASS_BLK,
UCLASS_BOOTDEV or UCLASS_USB_EMUL is put as children of a usb storage
device (just imagine a future system similar to bootstd for firmware
updates, trust material, etc.). Is it likely to have a struct in
parent_priv_ that is not a usb_device ? 

So which is more likely to survive future changes ?

- checking for parent_priv_ not null and not UCLASS_USB_EMUL

- checking for parent_priv_ not null and not UCLASS_USB_EMUL and not UCLASS_BLK
  (my patch, overcautious ?)

- checking for not (UCLASS_BLK, UCLASS_BOOTDEV or UCLASS_USB_EMUL)
  (Simon Glass' idea)
  
- checking for not  UCLASS_BLK and not UCLASS_BOOTDEV and not UCLASS_USB_EMUL
   and parent_priv_ not null
   
> >
> > Or can we check for recursible devices somehow ?
> > Maybe flag them or something ?
> >
> > Why is better to state all devices are recursible except some UCLASSes
> > (meaning they can have stuff that needs listed in usb info - or
> > likewise usb tree -) instead of stating that some closed set are
> > recursible ?
> >
> 
> For USB we can have lots of different types of devices as children, so
> it would be a pain to enumerate them all. At least that's how I see
> it.
>

We can have lots of devices as children, but those do get listed
before the recursive call.  How many of those can have children that
we have to list too, i.e.  how many of those do we want to recurse
into ?

I can only think of usb hubs (maybe some node for multifunction
devices too ???).  Maybe I'm not understanding how U-Boot works with
USB devices... but it looks like it would be enough to look for
UCLASS_USB_HUB, then recursive call, else no recursion. I mean instead of

cmd/usb.c : usb_show_tree_graph() :

if ((device_get_uclass_id(  child  ) != UCLASS_USB_EMUL) &&
(device_get_uclass_id(  child  ) != UCLASS_BOOTDEV) &&
(device_get_uclass_id(  child  ) != UCLASS_BLK)) {
usb_show_tree_graph(udev, pre);
pre[index] = 0;
}

we could simply have

if (device_get_uclass_id(  dev->dev  ) == UCLASS_USB_HUB) {
usb_show_tree_graph(udev, pre);
pre[index] = 0;
}

(or put the condition directly before the for loop)

Or can we have an UCLASS_USB_EMUL, UCLASS_BLK or UCLASS_BOOTDEV as
direct child of an UCLASS_USB_HUB ???

Am I opening any cans of worms ?

Re: [SPAM] Re: [PATCH] cmd: usb: Prevent reset in usb tree/info command

[Xavier Drudis Ferran]

El Tue, Jun 13, 2023 at 03:58:22PM +0100, Simon Glass deia:
> 
> Yes that's right. So 'usb info' should ignore UCLASS_BOOTDEV devices.

That's a possibility, yes. It should work until someone adds another
device there for some other purpose.

> That is better than checking for the NULL pointer.
> 

Why? What's wrong about checking for null ?
Or maybe checking for both not null and  not UCLASS_BOOTDEV ?

Not that I care too much, just to understand your reasoning.

Or can we check for recursible devices somehow ?
Maybe flag them or something ?

Why is better to state all devices are recursible except some UCLASSes
(meaning they can have stuff that needs listed in usb info - or
likewise usb tree -) instead of stating that some closed set are
recursible ?

Re: [SPAM] Re: [PATCH] cmd: usb: Prevent reset in usb tree/info command

[Xavier Drudis Ferran]

Ok. New test.

This uses yesterday morning's   next   branch.
commit 5b589e139620214f
Merge: cc5a940923 32d2461e04
Merge branch 'next_net/phy_connect_dev'

USB2 does not work for rk3399 in next (fixes are in master, thanks),
but USB3 is enough.

I compiled for rock-pi-4-rk3399_defconfig

flashed to a new microSD card as per doc/board/rockchip/rockchip.rst :

   dd if=u-boot-rockchip.bin of=/dev/sda seek=64
   sync

Put this microSD card in a Rock Pi 4 B+
Put a new USB stick in the USB3 port (center blue port closer to board).

(the microSD card and USB stick come from factory, I guess they were
partitioned with a single FAT partition)

(make sure emmc and spi are blank)

Connected only serial console and power. 

Got this:

U-Boot TPL 2023.07-rc2-00144-g497967f1ee (Jun 12 2023 - 11:15:47)
lpddr4_set_rate: change freq to 400MHz 0, 1
Channel 0: LPDDR4, 400MHz
BW=32 Col=10 Bk=8 CS0 Row=15 CS1 Row=15 CS=2 Die BW=16 Size=2048MB
Channel 1: LPDDR4, 400MHz
BW=32 Col=10 Bk=8 CS0 Row=15 CS1 Row=15 CS=2 Die BW=16 Size=2048MB
256B stride
lpddr4_set_rate: change freq to 800MHz 1, 0
Trying to boot from BOOTROM
Returning to boot ROM...

U-Boot SPL 2023.07-rc2-00144-g497967f1ee (Jun 12 2023 - 11:15:47 +0200)
Trying to boot from MMC1
NOTICE:  BL31: v2.1(release):v2.1-728-ged01e0c4-dirty
NOTICE:  BL31: Built : 18:29:11, Mar 22 2022


U-Boot 2023.07-rc2-00144-g497967f1ee (Jun 12 2023 - 11:15:47 +0200)

SoC: Rockchip rk3399
Reset cause: POR
Model: Radxa ROCK Pi 4B
DRAM:  4 GiB (effective 3.9 GiB)
PMIC:  RK808 
Core:  283 devices, 29 uclasses, devicetree: separate
MMC:   mmc@fe31: 2, mmc@fe32: 1, mmc@fe33: 0
Loading Environment from MMC... *** Warning - bad CRC, using default environment

In:serial
Out:   serial
Err:   serial
Model: Radxa ROCK Pi 4B
Net:   eth0: ethernet@fe30
Hit any key to stop autoboot:  2  1  0 
rockchip_pcie pcie@f800: PCIe link training gen1 timeout!
Bus usb@fe38: ehci_generic usb@fe38: Failed to get clocks (ret=-19)
Port not available.
Bus usb@fe3c: ehci_generic usb@fe3c: Failed to get clocks (ret=-19)
Port not available.
Bus usb@fe80: Register 2000140 NbrPorts 2
Starting the controller
USB XHCI 1.10
Bus usb@fe90: Register 2000140 NbrPorts 2
Starting the controller
USB XHCI 1.10
scanning bus usb@fe80 for devices... 1 USB Device(s) found
scanning bus usb@fe90 for devices... cannot reset port 1!?
2 USB Device(s) found
rockchip_pcie pcie@f800: failed to find ep-gpios property
ethernet@fe30 Waiting for PHY auto negotiation to complete. TIMEOUT 
!
Could not initialize PHY ethernet@fe30
rockchip_pcie pcie@f800: failed to find ep-gpios property
ethernet@fe30 Waiting for PHY auto negotiation to complete. TIMEOUT 
!
Could not initialize PHY ethernet@fe30
=> printenv preboot
## Error: "preboot" not defined
=> printenv
arch=arm
baudrate=150
board=evb_rk3399
board_name=evb_rk3399
boot_targets=mmc1 mmc0 nvme scsi usb pxe dhcp spi

bootcmd=bootflow scan

bootdelay=2
cpu=armv8
cpuid#=[something]
eth1addr=[:so:me:th:in:g]
ethact=ethernet@fe30
ethaddr=[:so:me:th:in:g]
fdt_addr_r=0x01f0
fdtcontroladdr=f1ef9170
fdtfile=rockchip/rk3399-rock-pi-4b.dtb
fdtoverlay_addr_r=0x0200
kernel_addr_r=0x0208
kernel_comp_addr_r=0x0800
kernel_comp_size=0x200
loadaddr=0x800800
partitions=uuid_disk=${uuid_gpt_disk};name=loader1,start=32K,size=4000K,uuid=${uuid_gpt_loader1};name=loader2,start=8MB,size=4MB,uuid=${uuid_gpt_loader2};name=trust,size=4M,uuid=${uuid_gpt_atf};name=boot,size=112M,bootable,uuid=${uuid_gpt_boot};name=rootfs,size=-,uuid=[something];
pxefile_addr_r=0x0060
ramdisk_addr_r=0x0600
script_offset_f=0xffe000
script_size_f=0x2000
scriptaddr=0x0050
serial#=[something]
soc=rk3399
stderr=serial,vidconsole
stdin=serial,usbkbd
stdout=serial,vidconsole
vendor=rockchip

Environment size: 1041/32764 bytes
=> usb info
1: Hub,  USB Revision 3.0
 - U-Boot XHCI Host Controller 
 - Class: Hub
 - PacketSize: 512  Configurations: 1
 - Vendor: 0x  Product 0x Version 1.0
   Configuration: 1
   - Interfaces: 1 Self Powered 0mA
 Interface: 0
 - Alternate Setting 0, Endpoints: 1
 - Class Hub
 - Endpoint 1 In Interrupt MaxPacket 8 Interval 255ms

1: Hub,  USB Revision 3.0
 - U-Boot XHCI Host Controller 
 - Class: Hub
 - PacketSize: 512  Configurations: 1
 - Vendor: 0x  Product 0x Version 1.0
   Configuration: 1
   - Interfaces: 1 Self Powered 0mA
 Interface: 0
 - Alternate Setting 0, Endpoints: 1
 - Class Hub
 - Endpoint 1 In Interrupt MaxPacket 8 Interval 255ms

2: Mass Storage,  USB Revision 3.20
 -  USB  SanDisk 3.2Gen1 05017d2e4d7b4ea0c5822c90c51e0b7
 - Class: (from Interface) Mass Storage
 - PacketSize: 512  Configurations: 1
 - Vendor: 0x0781  Product 0x5591 Version 1.0
   Configuration: 1
   - Interfaces: 1 Bus Powered 224mA
 Interface: 0
 - Alternate Setting 0, Endpoints: 2
 - Class Mass Storage, Transp. SCSI, Bulk only
 - Endpoint 1 In B

Re: [SPAM] Re: [PATCH] cmd: usb: Prevent reset in usb tree/info command

[Xavier Drudis Ferran]

El Mon, Jun 12, 2023 at 10:17:38PM +0100, Simon Glass deia:
> 
> I'm not sure what is going on here. Which version are you testing? Do
> you have these two commits?
> 
> 8c29b73278d6 bootstd: usb: Avoid initing USB twice
> 9fea3a799dde usb: Tidy up the usb_start flag
> 
> Regards,
> Simon

Yes, I have both. I'm testing the next branch.

I'll send shortly a reply to Marek Vasut with a cleaner test I did
yesterday. Doesn't look hard to reproduce for me...

I'm still trying to understand when the UCLASS_BOOTDEV device should
be added under an usb mass storage device, and when (if ever) removed.

Re: [SPAM] Re: [PATCH] cmd: usb: Prevent reset in usb tree/info command

[Xavier Drudis Ferran]

Sorry, I replied to Marek only but meant to reply to all.

El Fri, Jun 09, 2023 at 03:20:33AM +0200, Marek Vasut deia:

> > No. Well, in some tests yes and some no, but I got the error in all cases.
> 
> This is doubtful. It is mandatory to run 'usb start' or 'usb reset' before
> you would get any meaningful result out of 'usb info'. Without either, you
> would get 'USB is stopped.' message. Could it be there are some extra
> scripts in your environment that manipulate the USB ?
>

I saw usb_boootdev_hunt() calls usb_init() in drivers/usb/host/usb_bootdev.c

But maybe I don't get that called and it's really something silly in
my setup as you say later... Maybe it doesn't get called unless it
finds nothing else useful to boot.

> 
> Can you test with stock U-Boot ?
>

I don't know. I'll see if I have time ...
I'd rather read the code to understand what's the condition for finding 
bootdevices...

> Can you test with another USB stick, i.e. is the issue specific to this USB
> stick ?
>

I could test this, yes.

> Is the issue specific to this partition layout of this USB stick, i.e. if
> you clone (dd if=... of=...) the content of the USB stick to another USB
> stick, does the error still occur.
>

I'll try to partition and flash a new USB.

> [...]
> 
> > Model: Radxa ROCK Pi 4B
> > Net:   eth0: ethernet@fe30
> > Hit any key to stop autoboot:  2  1  0
> > rockchip_pcie pcie@f800: PCIe link training gen1 timeout!
> > Bus usb@fe38: USB EHCI 1.00
> > Bus usb@fe3c: USB EHCI 1.00
> > Bus usb@fe80: Register 2000140 NbrPorts 2
> > Starting the controller
> > USB XHCI 1.10
> > Bus usb@fe90: Register 2000140 NbrPorts 2
> > Starting the controller
> > USB XHCI 1.10
> > scanning bus usb@fe38 for devices... 1 USB Device(s) found
> > scanning bus usb@fe3c for devices... 2 USB Device(s) found
> > scanning bus usb@fe80 for devices... 1 USB Device(s) found
> > scanning bus usb@fe90 for devices... 1 USB Device(s) found
> > rockchip_pcie pcie@f800: failed to find ep-gpios property
> > ethernet@fe30 Waiting for PHY auto negotiation to complete. 
> > TIMEOUT !
> > Could not initialize PHY ethernet@fe30
> > rockchip_pcie pcie@f800: failed to find ep-gpios property
> > ethernet@fe30 Waiting for PHY auto negotiation to complete. 
> > TIMEOUT !
> > Could not initialize PHY ethernet@fe30
> 
> Is this some $preboot script which initializes your hardware ?
>

Mmm... yes, I used to have it... I thought not in this test, but I'd better 
recheck

Anyway, one should be allowed to stop the boot, call usb start and usb tree
and don't get a reset, shouldn't one?

> => printenv preboot
>

I'll send this later when I repeat the test. I'd like to find a
minimal test case or something...

> > => usb tree
> > USB device tree:
> >1  Hub (480 Mb/s, 0mA)
> >   u-boot EHCI Host Controller
> >1  Hub (480 Mb/s, 0mA)
> >|  u-boot EHCI Host Controller
> >|
> > uclass_id=64
> >|+-2  Mass Storage (480 Mb/s, 200mA)
> > TDK LoR TF10 07032B6B1D0ACB96
> > uclass_id=22
> > uclass_id=25
> >   "Synchronous Abort" handler, esr 0x9610, far 0x948
> > elr: 002157d4 lr : 002157d4 (reloc)
> > elr: f3f4f7d4 lr : f3f4f7d4
> 
> Take the u-boot (unstripped elf) which matches this binary, and run
> aarch64-...objdump -lSD on it, then search for the $lr value, see
> doc/develop/crash_dumps.rst for details. That should tell you where exactly
> the crash occurred. Where did it occur ?
>

I didn't do it exactly so, but from u-boot.map I gathered that it was
in cmd/usb.c and the fact that my patch fixed it implies the problem
is the functions usb_show_tree_graph() or usb_show_info() get called
recursively with null as a first parameter.

Now I don't have that u-boot.map anymore and would have to repeat the
experiment, to find out exactly as you say, so I won't do it right
now. But thanks, understood.

The reason usb_show_tree_graph() gets called with a null usb_device *
is that the code in cmd/usb.c for usb info and usb tree assumes
everything a UCLASS_MASS_STORAGE device can have as children are
devices with some usb_device in their dev_get_parent_priv().  It
carves out exceptions to this general rule for UCLASS_USB_EMUL and
UCLASS_BLK, but not for UCLASS_BOOTDEV. When it finds a child that is
UCLASS_BOOTDEV it happily recurses on it passing its parent_priv as
usb_device, but the bootdev code did not put any usb_device there,
it's null. So the first access causes a null pointer dereference.

I would have to wrap my mind around more code to start understanding
if it's better to give that UCLASS_BOOTDEV some usb_device as parent
priv data, or it is better to give USB devices that can be enumerated
for listing (usb tree or usb info) some RECURSIBLE flag that indicates
their priv parent data is reliably a usb_device.

So checking that the alledged usb_de

Re: [PATCH] cmd: usb: Prevent reset in usb tree/info command

[Xavier Drudis Ferran]

El Thu, Jun 08, 2023 at 12:05:18AM +0200, Marek Vasut deia:
> On 6/5/23 17:20, Xavier Drudis Ferran wrote:
> > Add a check to avoid dommed (by null pointer dereference) recursive
> > call, not only for UCLASS_BLK.
> > 
> > When booting my Rock Pi 4B+ with a USB mass storage stick plugged
> > into one of the USB 2 ports (EHCI), when it is plugged before power
> > on, when issuing a
> > 
> > usb tree
> > 
> > or
> > 
> > usb info
> 
> I cannot reproduce the problem. Do you perform any other interaction with
> the USB stack, like e.g. 'usb start' or 'usb reset' before issuing the
> aforementioned commands ?
>

No. Well, in some tests yes and some no, but I got the error in all cases.

Btw, I was testing on the next branch. I had those two commits on top
https://patchwork.ozlabs.org/project/uboot/patch/202013db5a47ecbac4a53c360ed1ca91ca663996.1685974993.git.xdru...@tinet.cat/
https://patchwork.ozlabs.org/project/uboot/patch/464111fca83008503022e8ada5305e69ffd1afbd.1685974993.git.xdru...@tinet.cat/

And minor configuration changes (but I had bootstage active, might or might not 
be related)

U-Boot was in a microSD card. 


> What kind of USB stick is used here, please share model, VID, PID.
>

It may only happen with ext4 partitions or something...

Or it might have to do with other media not being bootable (they used
to be for some old and customized version of U-boot, but not the
current one)

This is the lsusb -v output of the same USB stick in another computer

Bus 007 Device 008: ID 0718:070a Imation Corp. TF10
Device Descriptor:
  bLength18
  bDescriptorType 1
  bcdUSB   2.00
  bDeviceClass0 
  bDeviceSubClass 0 
  bDeviceProtocol 0 
  bMaxPacketSize064
  idVendor   0x0718 Imation Corp.
  idProduct  0x070a 
  bcdDevice1.00
  iManufacturer   1 TDK LoR
  iProduct2 TF10
  iSerial 3 07032B6B1D0ACB96
  bNumConfigurations  1
  Configuration Descriptor:
bLength 9
bDescriptorType 2
wTotalLength   0x0020
bNumInterfaces  1
bConfigurationValue 1
iConfiguration  0 
bmAttributes 0x80
  (Bus Powered)
MaxPower  200mA
Interface Descriptor:
  bLength 9
  bDescriptorType 4
  bInterfaceNumber0
  bAlternateSetting   0
  bNumEndpoints   2
  bInterfaceClass 8 Mass Storage
  bInterfaceSubClass  6 SCSI
  bInterfaceProtocol 80 Bulk-Only
  iInterface  0 
  Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x81  EP 1 IN
bmAttributes2
  Transfer TypeBulk
  Synch Type   None
  Usage Type   Data
wMaxPacketSize 0x0200  1x 512 bytes
bInterval   0
  Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x02  EP 2 OUT
bmAttributes2
  Transfer TypeBulk
  Synch Type   None
  Usage Type   Data
wMaxPacketSize 0x0200  1x 512 bytes
bInterval   0
Device Qualifier (for other device speed):
  bLength10
  bDescriptorType 6
  bcdUSB   2.00
  bDeviceClass0 
  bDeviceSubClass 0 
  bDeviceProtocol 0 
  bMaxPacketSize064
  bNumConfigurations  1
can't get debug descriptor: Resource temporarily unavailable
Device Status: 0x
  (Bus Powered)

Partitions:

Welcome to GNU Parted! Type 'help' to view a list of commands.
(parted) p
Model: TDK LoR TF10 (scsi)
Disk /dev/sda: 32,0GB
Sector size (logical/physical): 512B/512B
Partition Table: loop
Disk Flags: 

Number  Start  End SizeFile system  Flags
 1  0,00B  32,0GB  32,0GB  ext4


The content in the ext4 partition is just data, 3 files,
lost+found and another directory

Let me get to my logs... (I added a printf here uclass_id=22 is UCLASS_BLK
uclass_id=25 is UCLASS_BOOTDEV )

U-Boot TPL 2023.07-rc2-00089-gab17b3d648-dirty (Jun 05 2023 - 10:42:52)
lpddr4_set_rate: change freq to 400MHz 0, 1
Channel 0: LPDDR4, 400MHz
BW=32 Col=10 Bk=8 CS0 Row=15 CS1 Row=15 CS=2 Die BW=16 Size=2048MB
Channel 1: LPDDR4, 400MHz
BW=32 Col=10 Bk=8 CS0 Row=15 CS1 Row=15 CS=2 Die BW=16 Size=2048MB
256B stride
lpddr4_set_rate: change freq to 800MHz 1, 0
Trying to boot from BOOTROM
Returning to boot ROM...

U-Boot SPL 2023.07-rc2-00089-gab17b3d648-dirty (Jun 05 2023 - 10:42:52 +0200)
Trying to boot from MMC1
NOTICE:  BL31: v2.1(release):v

Re: [PATCH v7 0/2] arm: dts: rockchip: rk3399: usb: ehci: Fix EHCI probe in rk3399 to access peripherals by USB 2

[Xavier Drudis Ferran]

El Wed, Jun 07, 2023 at 11:42:40PM +0200, Marek Vasut deia:
> On 6/5/23 17:04, Xavier Drudis Ferran wrote:
> > EHCI probing in Rock pi 4 currently fails.
> > 
> > Add a clock driver for usb2phy so that probing EHCI does not fail when
> > missing one of the clocks in the bundle for usb_host0_ehci, since
> > usb2phy is UCLASS_PHY but not UCLASS_CLK.
> > 
> > Xavier Drudis Ferran (2):
> >phy: rockchip-inno-usb2: Add usb2phy clock provider of 480MHz clock
> >phy: rockchip-inno-usb2: Implement clock operations for usb2phy clock
> > 
> >   drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 109 +-
> >   1 file changed, 106 insertions(+), 3 deletions(-)
> 
> Applied both to usb/master .
> 
> btw the cover letter subject should not have 'arm: dts:' tags, but rather
> 'phy:' tag , since this does not touch any DTs .


Yes, sorry. v1 did touch *-u-boot.dts. I hesitated about what was more
confusing, changing subject on different versions of a patch with the
same intent or keeping the old subject.

Thanks for merging.

Re: [PATCH v2 5/7] phy: rockchip-inno-usb2: Add USB2 PHY for RK3328

[Xavier Drudis Ferran]

El Tue, Jun 06, 2023 at 10:39:16PM +0530, Jagan Teki deia:
> USB2.0 Host and OTG controllers in RK3328 are using USB2PHY.
> 
> Add support for it.
> 
> Signed-off-by: Jagan Teki 

Reviewed-by: Xavier Drudis Ferran 

(fwiw, I just compared it with linux)

I just wanted to note this patch as written most likely depends on
https://patchwork.ozlabs.org/project/uboot/patch/464111fca83008503022e8ada5305e69ffd1afbd.1685974993.git.xdru...@tinet.cat/

which introduces the struct member clkout_ctl used here.


> ---
> Changes for v2:
> - add clkout_ctl
> 
>  drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 34 +++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c 
> b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> index e43a5ba9b5..d8738f891d 100644
> --- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> +++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> @@ -345,6 +345,36 @@ bind_fail:
>   return ret;
>  }
>  
> +static const struct rockchip_usb2phy_cfg rk3328_usb2phy_cfgs[] = {
> + {
> + .reg = 0x100,
> + .clkout_ctl = { 0x108, 4, 4, 1, 0 },
[...]

Re: [PATCH v6 0/2] arm: dts: rockchip: rk3399: usb: ehci: Fix EHCI probe in rk3399 to access peripherals by USB 2.

[Xavier Drudis Ferran]

El Mon, Jun 05, 2023 at 08:11:07AM +0530, Jagan Teki deia:
> On Sun, Jun 4, 2023 at 1:42 PM Xavier Drudis Ferran  wrote:
> >
> > EHCI probing in Rock pi 4 currently fails.
> >
> > Add a clock driver for usb2phy so that probing EHCI does not fail when
> > missing one of the clocks in the bundle for usb_host0_ehci, since
> > usb2phy is UCLASS_PHY but not UCLASS_CLK.
> >
> > Xavier Drudis Ferran (2):
> >   arm: dts: rockchip: rk3399: usb: ehci: Fix EHCI probe in rk3399 to
> > access peripherals by USB 2.
> >   arm: rk3399: usb2phy: phy-rockchip-inno-usb2.c: Implement operations
> > for the 480MHz usb2phy clock in rk3399.
> 
> Please note that commit head on both the patches seems improper to me.
> 
> Commit body looks fine, but the head should start
> 
> phy: rockchip-inno-usb2: Add usb2phy clock provider of 480MHz clock
> phy: rockchip-inno-usb2: Implement clock operations for usb2phy clock
> 
> Thanks,
> Jagan.


Done, thank you. I hope it's right now.

[PATCH] cmd: usb: Prevent reset in usb tree/info command

[Xavier Drudis Ferran]

Add a check to avoid dommed (by null pointer dereference) recursive
call, not only for UCLASS_BLK.

When booting my Rock Pi 4B+ with a USB mass storage stick plugged
into one of the USB 2 ports (EHCI), when it is plugged before power
on, when issuing a

usb tree

or

usb info

command I get a "Synchronous Error" and a reset just after printing the
mass storage device in the usb tree or usb info. It might depend on the
contents of the USB stick too, I'm not sure.

It seems like I have two devices as children of the mass storage
device.  When there's only a UCLASS_BLK it works fine, but when there's
a UCLASS_BLK and a UCLASS_BOOTDEV, it recurses with a null udev as
first parameter and fails.

Likewise for usb_show_info().

Not sure if this should be a patch, an RFC or a bug report.  There may
be a better way to solve this, I haven't researched commit
201417d700a2ab09 ("bootstd: Add the bootdev uclass") and bootdev flow
properly, or thought about cases where udev is not null but the
recursive call might need preventing too. Feel free to think it over
before merging (or after). But this at least fixes a reset at an
innocent looking usb tree or usb info command. Maybe we can improve it
later?

Cc: Simon Glass 
Cc: Lukasz Majewski 
Cc: Marek Vasut 


Signed-off-by: Xavier Drudis Ferran 
---
 cmd/usb.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/cmd/usb.c b/cmd/usb.c
index 73addb04c4..7e6065aa51 100644
--- a/cmd/usb.c
+++ b/cmd/usb.c
@@ -421,7 +421,8 @@ static void usb_show_tree_graph(struct usb_device *dev, 
char *pre)
 * Ignore emulators and block child devices, we only want
 * real devices
 */
-   if ((device_get_uclass_id(child) != UCLASS_USB_EMUL) &&
+   if (udev &&
+   (device_get_uclass_id(child) != UCLASS_USB_EMUL) &&
(device_get_uclass_id(child) != UCLASS_BLK)) {
usb_show_tree_graph(udev, pre);
pre[index] = 0;
@@ -607,7 +608,8 @@ static void usb_show_info(struct usb_device *udev)
(device_get_uclass_id(child) != UCLASS_USB_EMUL) &&
(device_get_uclass_id(child) != UCLASS_BLK)) {
udev = dev_get_parent_priv(child);
-   usb_show_info(udev);
+   if (udev)
+   usb_show_info(udev);
}
}
 }
-- 
2.20.1

[PATCH v7 1/2] phy: rockchip-inno-usb2: Add usb2phy clock provider of 480MHz clock

[Xavier Drudis Ferran]

arch/arm/dts/rk3399.dtsi has a node

  usb_host0_ehci: usb@fe38 {
   compatible = "generic-ehci";

with clocks:

   clocks = <&cru HCLK_HOST0>, <&cru HCLK_HOST0_ARB>,
<&u2phy0>;

The first 2 refer to nodes with class UCLASS_CLK, but &u2phy0
has class UCLASS_PHY.

  u2phy0: usb2phy@e450 {
   compatible = "rockchip,rk3399-usb2phy";

Since clk_get_bulk() only looks for devices with UCLASS_CLK,
it fails with -ENODEV and then ehci_usb_probe() aborts.

The consequence is peripherals connected to a USB 2 port (e.g. in a
Rock Pi 4 the white port, nearer the edge) not being detected.
They're detected if CONFIG_USB_OHCI_GENERIC is selected in Kconfig,
because ohci_usb_probe() does not abort when one clk_get_by_index()
fails, but then they work in USB 1 mode.

rk3399.dtsi comes from linux and the  u2phy0 was added[1] to the clock
list in:

commit b5d1c57299734f5b54035ef2e61706b83041f20c
Author: William wu 
Date:   Wed Dec 21 18:41:05 2016 +0800

arm64: dts: rockchip: add u2phy clock for ehci and ohci of rk3399

We found that the suspend process was blocked when it run into
ehci/ohci module due to clk-480m of usb2-phy was disabled.
[...]

Suspend concerns don't apply to U-Boot, and the problem with U-Boot
failing to probe EHCI doesn't apply to linux, because in linux
rockchip_usb2phy_clk480m_register makes u2phy0 a proper clock provider
when called by rockchip_usb2phy_probe().

So I can think of a few alternative solutions:

1- Change ehci_usb_probe() to make it more similar to
   ohci_usb_probe(), and survive failure to get one clock. Looks a
   little harder, and I don't know whether it could break something if
   it ignored a clock that was important for something else than
   suspend.

2- Change rk3399.dtsi effectively reverting the linux commit
   b5d1c57299734f5b54035ef2e61706b83041f20c. This dealigns the .dtsi
   from linux and seems fragile at the next synchronisation.

3- Change the clock list in rk3399-u-boot.dtsi or somewhere else.
   This survives .dts* sync but may survive "too much" and miss some
   change from linux that we might want.

4- Enable CONFIG_USB_OHCI_GENERIC and use the ports in USB 1 mode.
   This would need to be made for all boards using rk3399.  In a
   simple test reading one file from USB storage it gave 769.5 KiB/s
   instead of 20.5 MiB/s with solution 2.

5- Trying to replicate linux and have usb2phy somehow provide a clk,
   or have a separate clock device for usb2phy in addition to the phy
   device.

This patch tries to implement option 5 as Marek Vasut requested in
December 5th.  Options 1 and 3 didn't get through [2][3].

It just registers usb2phy as a clock driver (device_bind_driver()
didn't work but device_bind_driver_to_node() did), without any
specific operations, so that ehci-generic.c finds it and is happy. It
worked in my tests on a Rock Pi 4 B+ (rk3399).

Link: [1] https://lkml.kernel.org/lkml/1731551.Q6cHK6n5ZM@phil/T/
  [2] 
https://patchwork.ozlabs.org/project/uboot/patch/20220701185959.GC1700@begut/
  [3] 
https://patchwork.ozlabs.org/project/uboot/patch/Y44+ayJfUlI08ptM@localhost/

Cc: Simon Glass 
Cc: Philipp Tomsich 
Cc: Kever Yang 
Cc: Lukasz Majewski 
Cc: Sean Anderson 
Cc: Marek Vasut 
Cc: Christoph Fritz 
Cc: Jagan Teki 

Signed-off-by: Xavier Drudis Ferran 
---

 V7: improve error handling. Call device_chld_unbind() on error.
 Remove unnecessary if.

 v6: just retested over current next branch and some corrections
 to message and headers
 (no changes to code).
---
 drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 33 +--
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c 
b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
index 55e1dbcfef..732d37201d 100644
--- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
+++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
@@ -7,10 +7,11 @@
  */
 
 #include 
-#include 
+#include 
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -168,6 +169,9 @@ static struct phy_ops rockchip_usb2phy_ops = {
.of_xlate = rockchip_usb2phy_of_xlate,
 };
 
+static struct clk_ops rockchip_usb2phy_clk_ops = {
+};
+
 static int rockchip_usb2phy_probe(struct udevice *dev)
 {
struct rockchip_usb2phy *priv = dev_get_priv(dev);
@@ -234,7 +238,8 @@ static int rockchip_usb2phy_bind(struct udevice *dev)
dev_for_each_subnode(node, dev) {
if (!ofnode_valid(node)) {
dev_info(dev, "subnode %s not found\n", dev->name);
-   return -ENXIO;
+   ret = -ENXIO;
+   goto bind_fail;
}
 
name = ofnode_get_name(node);
@@ -245,10 +250,26 @@ static int rockchip_usb2phy_bind(struct udevice *dev)
if (ret) {
 

[PATCH v7 2/2] phy: rockchip-inno-usb2: Implement clock operations for usb2phy clock

[Xavier Drudis Ferran]

This clock doesn't seem needed but appears in a phandle list used by
ehci-generic.c to bulk enable it. The phandle list comes from linux,
where it is needed for suspend/resume to work [1].

My tests give the same results with or without this patch, but Marek
Vasut found it weird to declare an empty clk_ops [2].

So I adapted the code from linux 6.1-rc8 so that it hopefully works
if it ever has some user. For now, without real use, it seems to
at least not give any errors when called.

Link: [1] https://lkml.kernel.org/lkml/1731551.Q6cHK6n5ZM@phil/T/
  [2] 
https://patchwork.ozlabs.org/project/uboot/patch/Y5IWpjYLB4aXMy9o@localhost/

Cc: Simon Glass 
Cc: Philipp Tomsich 
Cc: Kever Yang 
Cc: Lukasz Majewski 
Cc: Sean Anderson 
Cc: Marek Vasut 
Cc: Christoph Fritz 
Cc: Jagan Teki 

Signed-off-by: Xavier Drudis Ferran 
---

 v7: add clkout_ctl values for rk3568 (from linux).
 UNTESTED (I don't have the hardware).

 v6: just retested over current next branch and some corrections
 to message and headers
 (no changes to code).

 v5: ignores the return value from property_enable() which is not
 an error code in U-Boot (unlike in linux). This avoid a false
 failure of rockchip_usb2phy_clk_disable() that interfered with
 clock disable and appeared to cause hang or reset.
---
 drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 80 ++-
 1 file changed, 78 insertions(+), 2 deletions(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c 
b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
index 732d37201d..be5f79490c 100644
--- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
+++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
@@ -56,6 +56,7 @@ struct rockchip_usb2phy_port_cfg {
 
 struct rockchip_usb2phy_cfg {
unsigned int reg;
+   struct usb2phy_reg  clkout_ctl;
const struct rockchip_usb2phy_port_cfg port_cfgs[USB2PHY_NUM_PORTS];
 };
 
@@ -77,6 +78,18 @@ static inline int property_enable(void *reg_base,
return writel(val, reg_base + reg->offset);
 }
 
+static inline bool property_enabled(void *reg_base,
+   const struct usb2phy_reg *reg)
+{
+   unsigned int tmp, orig;
+   unsigned int mask = GENMASK(reg->bitend, reg->bitstart);
+
+   orig = readl(reg_base + reg->offset);
+
+   tmp = (orig & mask) >> reg->bitstart;
+   return tmp != reg->disable;
+}
+
 static const
 struct rockchip_usb2phy_port_cfg *us2phy_get_port(struct phy *phy)
 {
@@ -169,7 +182,63 @@ static struct phy_ops rockchip_usb2phy_ops = {
.of_xlate = rockchip_usb2phy_of_xlate,
 };
 
+/**
+ * round_rate() - Adjust a rate to the exact rate a clock can provide.
+ * @clk:   The clock to manipulate.
+ * @rate:  Desidered clock rate in Hz.
+ *
+ * Return: rounded rate in Hz, or -ve error code.
+ */
+ulong rockchip_usb2phy_clk_round_rate(struct clk *clk, ulong rate)
+{
+   return 48000;
+}
+
+/**
+ * enable() - Enable a clock.
+ * @clk:   The clock to manipulate.
+ *
+ * Return: zero on success, or -ve error code.
+ */
+int rockchip_usb2phy_clk_enable(struct clk *clk)
+{
+   struct udevice *parent = dev_get_parent(clk->dev);
+   struct rockchip_usb2phy *priv = dev_get_priv(parent);
+   const struct rockchip_usb2phy_cfg *phy_cfg = priv->phy_cfg;
+
+   /* turn on 480m clk output if it is off */
+   if (!property_enabled(priv->reg_base, &phy_cfg->clkout_ctl)) {
+   property_enable(priv->reg_base, &phy_cfg->clkout_ctl, true);
+
+   /* waiting for the clk become stable */
+   usleep_range(1200, 1300);
+   }
+
+   return 0;
+}
+
+/**
+ * disable() - Disable a clock.
+ * @clk:   The clock to manipulate.
+ *
+ * Return: zero on success, or -ve error code.
+ */
+int rockchip_usb2phy_clk_disable(struct clk *clk)
+{
+   struct udevice *parent = dev_get_parent(clk->dev);
+   struct rockchip_usb2phy *priv = dev_get_priv(parent);
+   const struct rockchip_usb2phy_cfg *phy_cfg = priv->phy_cfg;
+
+   /* turn off 480m clk output */
+   property_enable(priv->reg_base, &phy_cfg->clkout_ctl, false);
+
+   return 0;
+}
+
 static struct clk_ops rockchip_usb2phy_clk_ops = {
+   .enable = rockchip_usb2phy_clk_enable,
+   .disable = rockchip_usb2phy_clk_disable,
+   .round_rate = rockchip_usb2phy_clk_round_rate
 };
 
 static int rockchip_usb2phy_probe(struct udevice *dev)
@@ -255,8 +324,11 @@ static int rockchip_usb2phy_bind(struct udevice *dev)
}
 
node = dev_ofnode(dev);
-   name = ofnode_get_name(node);
-   dev_dbg(dev, "clk for node %s\n", name);
+   name = "clk_usbphy_480m";
+   dev_read_string_index(dev, "clock-output-names", 0, &name);
+
+   dev_dbg(dev, "clk %s for node %s\n", name, ofnode_get_name(node));
+
ret = device_bind_driver_to_node(dev, "

[PATCH v7 0/2] arm: dts: rockchip: rk3399: usb: ehci: Fix EHCI probe in rk3399 to access peripherals by USB 2

[Xavier Drudis Ferran]

EHCI probing in Rock pi 4 currently fails. 

Add a clock driver for usb2phy so that probing EHCI does not fail when
missing one of the clocks in the bundle for usb_host0_ehci, since
usb2phy is UCLASS_PHY but not UCLASS_CLK.

Xavier Drudis Ferran (2):
  phy: rockchip-inno-usb2: Add usb2phy clock provider of 480MHz clock
  phy: rockchip-inno-usb2: Implement clock operations for usb2phy clock

 drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 109 +-
 1 file changed, 106 insertions(+), 3 deletions(-)

Cc: Simon Glass 
Cc: Philipp Tomsich 
Cc: Kever Yang 
Cc: Lukasz Majewski 
Cc: Sean Anderson 
Cc: Marek Vasut 
Cc: Christoph Fritz 
Cc: Jagan Teki 
Signed-off-by: Xavier Drudis Ferran 

---

   Changes:

   v7: Error handling. Remove unnecessary if.
   Adding config data for rk3568 (untested).

   v6: just retested over current next branch and some corrections
   to message and headers
   (no changes to code).

   v5: fixes a bug that Christoph Fritz discovered, consisting in the
   wrong eror code returned when enabling or disabling the clock
   because property_enable() returns an error code in linux but
   the modified register value in U-Boot. This caused the clk
   disable to abort before freeing the clock.

   v4: move v3 to one patch in the series and add a second patch
   to add operations to enable disable the usb2phy 480Mhz clock.
   Also, honour clock-output-names for what is worth.

   v3: implement option 5 (bind usb2phy as a clk driver too) instead
   of option 1 (ehci-generic.c tolerates missing clocks).

   v2: implement option 1 (ehci-generic.c tolerates missing clocks)
  instead of option 3 (change dts node to remove the missing
  clock).

-- 
2.20.1

Re: [PATCH v6 2/2] arm: rk3399: usb2phy: phy-rockchip-inno-usb2.c: Implement operations for the 480MHz usb2phy clock in rk3399.

[Xavier Drudis Ferran]

Thanks for looking at this.

El Sun, Jun 04, 2023 at 11:33:21AM +0200, Marek Vasut deia:
> > diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c 
> > b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> > index 2f31350134..451841b025 100644
> > --- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> > +++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> > @@ -55,6 +55,7 @@ struct rockchip_usb2phy_port_cfg {
> >   struct rockchip_usb2phy_cfg {
> > unsigned int reg;
> > +   struct usb2phy_reg  clkout_ctl;
> > const struct rockchip_usb2phy_port_cfg port_cfgs[USB2PHY_NUM_PORTS];
> >   };
> > @@ -76,6 +77,18 @@ static inline int property_enable(void *reg_base,
> > return writel(val, reg_base + reg->offset);
> >   }
> > +static inline bool property_enabled(void *reg_base,
> > +   const struct usb2phy_reg *reg)
> > +{
> > +   unsigned int tmp, orig;
> > +   unsigned int mask = GENMASK(reg->bitend, reg->bitstart);
> > +
> > +   orig = readl(reg_base + reg->offset);
> > +
> > +   tmp = (orig & mask) >> reg->bitstart;
> 
> Use FIELD_GET() macro if possible.
>

It would be possible, but it seems to require a constant mask.  Now
the mask is read from a cfg struct, so it's not constant.  Currently
the mask bitend and bitstart are really always the same value (4 and
4) for all (2) SOCs, so I could change the code to make it a
constant and use FIELD_GET(), but I'd see two drawbacks:

- It makes code more different than needed from linux
  drivers/phy/rockchip/phy-rockchip-inno-usb2.c

- It will stop working if we ever support rk3366 here, the mask there
  is bit 15.

So I'd rather leave it as it is.

But you made me realise I was missing the clkout_ctl struct for
rk3568, so I'll copy from linux in v7. I can't test it, though.

Thank you.

Re: [PATCH v6 1/2] arm: dts: rockchip: rk3399: usb: ehci: Fix EHCI probe in rk3399 to access peripherals by USB 2.

[Xavier Drudis Ferran]

Thanks for your feedback.

El Sun, Jun 04, 2023 at 11:31:28AM +0200, Marek Vasut deia:
> > diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c 
> > b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> > index 55e1dbcfef..2f31350134 100644
> > --- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> > +++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> > @@ -7,7 +7,7 @@
> >*/
> >   #include 
> > -#include 
> > +#include 
> >   #include 
> >   #include 
> >   #include 
> > @@ -168,6 +168,9 @@ static struct phy_ops rockchip_usb2phy_ops = {
> > .of_xlate = rockchip_usb2phy_of_xlate,
> >   };
> > +static struct clk_ops rockchip_usb2phy_clk_ops = {
> > +};
> > +
> >   static int rockchip_usb2phy_probe(struct udevice *dev)
> >   {
> > struct rockchip_usb2phy *priv = dev_get_priv(dev);
> > @@ -249,6 +252,18 @@ static int rockchip_usb2phy_bind(struct udevice *dev)
> > }
> > }
> > +   if (!ret) {
> 
> Can $ret ever be != 0 here ?
>

No, you're right. I can get rid of the if in v7.

> btw. the dev_for_each_subnode() above is missing error handling, in case
> device_bind_driver_to_node() there returns non-zero, there should be some
> 'goto err' and 'err: dev_for_each_subnode() device_unbind()' fail path.
>
> > +   node = dev_ofnode(dev);
> > +   name = ofnode_get_name(node);
> > +   dev_dbg(dev, "clk for node %s\n", name);
> > +   ret = device_bind_driver_to_node(dev, "rockchip_usb2phy_clock",
> > +name, node, &usb2phy_dev);
> > +   if (ret) {
> > +   dev_err(dev,
> > +   "'%s' cannot bind 'rockchip_usb2phy_clock'\n", 
> > name);
> 
> Use device_unbind() in fail path here too.
>

Well, dev_for_each_subnode wouldn't give me the dev to pass to
device_unbind, but I can simply call device_chld_unbind(dev) on the
error path (on the parent device) and that should clean up any bound
children.

I'll fix it in v7, thanks.

Re: [SPAM] Re: [PATCH v5 0/2] arm: dts: rockchip: rk3399: usb: ehci: Fix EHCI probe in rk3399 to access peripherals by USB 2.

[Xavier Drudis Ferran]

El Sat, Jun 03, 2023 at 09:23:36AM +0200, Xavier Drudis Ferran deia:
> El Fri, Jun 02, 2023 at 03:34:37PM +0530, Jagan Teki deia:
> > On Fri, Jun 2, 2023 at 2:54 PM Xavier Drudis Ferran  
> > wrote:
> > >
> > > Should I try again with the current next branch and send v6 ?
> > 
> > Please send.
> > 
> > Thanks,
> > Jagan.
> 
> I will try asap, thanks.

Done

https://patchwork.ozlabs.org/project/uboot/list/?series=358005

Tests, etc. welcome.

Christoph Fritz issue with his USB-ethernet adapter on XHCI may still
be there, it looked unrelated and I can't test it.

[PATCH v6 2/2] arm: rk3399: usb2phy: phy-rockchip-inno-usb2.c: Implement operations for the 480MHz usb2phy clock in rk3399.

[Xavier Drudis Ferran]

This clock doesn't seem needed but appears in a phandle list used by
ehci-generic.c to bulk enable it. The phandle list comes from linux,
where it is needed for suspend/resume to work [1].

My tests give the same results with or without this patch, but Marek
Vasut found it weird to declare an empty clk_ops [2].

So I adapted the code from linux 6.1-rc8 so that it hopefully works
if it ever has some user. For now, without real use, it seems to
at least not give any errors when called.

Link: [1] https://lkml.kernel.org/lkml/1731551.Q6cHK6n5ZM@phil/T/
  [2] 
https://patchwork.ozlabs.org/project/uboot/patch/Y5IWpjYLB4aXMy9o@localhost/

Cc: Simon Glass 
Cc: Philipp Tomsich 
Cc: Kever Yang 
Cc: Lukasz Majewski 
Cc: Sean Anderson 
Cc: Marek Vasut 
Cc: Christoph Fritz 
Cc: Jagan Teki 

Signed-off-by: Xavier Drudis Ferran 
---

 v6: just retested over current next branch and some corrections
 to message and headers
 (no changes to code).


 v5: ignores the return value from property_enable() which is not
 an error code in U-Boot (unlike in linux). This avoid a false
 failure of rockchip_usb2phy_clk_disable() that interfered with
 clock disable and appeared to cause hang or reset.
 
---
 drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 78 ++-
 1 file changed, 76 insertions(+), 2 deletions(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c 
b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
index 2f31350134..451841b025 100644
--- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
+++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
@@ -55,6 +55,7 @@ struct rockchip_usb2phy_port_cfg {
 
 struct rockchip_usb2phy_cfg {
unsigned int reg;
+   struct usb2phy_reg  clkout_ctl;
const struct rockchip_usb2phy_port_cfg port_cfgs[USB2PHY_NUM_PORTS];
 };
 
@@ -76,6 +77,18 @@ static inline int property_enable(void *reg_base,
return writel(val, reg_base + reg->offset);
 }
 
+static inline bool property_enabled(void *reg_base,
+   const struct usb2phy_reg *reg)
+{
+   unsigned int tmp, orig;
+   unsigned int mask = GENMASK(reg->bitend, reg->bitstart);
+
+   orig = readl(reg_base + reg->offset);
+
+   tmp = (orig & mask) >> reg->bitstart;
+   return tmp != reg->disable;
+}
+
 static const
 struct rockchip_usb2phy_port_cfg *us2phy_get_port(struct phy *phy)
 {
@@ -168,7 +181,63 @@ static struct phy_ops rockchip_usb2phy_ops = {
.of_xlate = rockchip_usb2phy_of_xlate,
 };
 
+/**
+ * round_rate() - Adjust a rate to the exact rate a clock can provide.
+ * @clk:   The clock to manipulate.
+ * @rate:  Desidered clock rate in Hz.
+ *
+ * Return: rounded rate in Hz, or -ve error code.
+ */
+ulong rockchip_usb2phy_clk_round_rate(struct clk *clk, ulong rate)
+{
+   return 48000;
+}
+
+/**
+ * enable() - Enable a clock.
+ * @clk:   The clock to manipulate.
+ *
+ * Return: zero on success, or -ve error code.
+ */
+int rockchip_usb2phy_clk_enable(struct clk *clk)
+{
+   struct udevice *parent = dev_get_parent(clk->dev);
+   struct rockchip_usb2phy *priv = dev_get_priv(parent);
+   const struct rockchip_usb2phy_cfg *phy_cfg = priv->phy_cfg;
+
+   /* turn on 480m clk output if it is off */
+   if (!property_enabled(priv->reg_base, &phy_cfg->clkout_ctl)) {
+   property_enable(priv->reg_base, &phy_cfg->clkout_ctl, true);
+
+   /* waiting for the clk become stable */
+   usleep_range(1200, 1300);
+   }
+
+   return 0;
+}
+
+/**
+ * disable() - Disable a clock.
+ * @clk:   The clock to manipulate.
+ *
+ * Return: zero on success, or -ve error code.
+ */
+int rockchip_usb2phy_clk_disable(struct clk *clk)
+{
+   struct udevice *parent = dev_get_parent(clk->dev);
+   struct rockchip_usb2phy *priv = dev_get_priv(parent);
+   const struct rockchip_usb2phy_cfg *phy_cfg = priv->phy_cfg;
+
+   /* turn off 480m clk output */
+   property_enable(priv->reg_base, &phy_cfg->clkout_ctl, false);
+
+   return 0;
+}
+
 static struct clk_ops rockchip_usb2phy_clk_ops = {
+   .enable = rockchip_usb2phy_clk_enable,
+   .disable = rockchip_usb2phy_clk_disable,
+   .round_rate = rockchip_usb2phy_clk_round_rate
 };
 
 static int rockchip_usb2phy_probe(struct udevice *dev)
@@ -254,8 +323,11 @@ static int rockchip_usb2phy_bind(struct udevice *dev)
 
if (!ret) {
node = dev_ofnode(dev);
-   name = ofnode_get_name(node);
-   dev_dbg(dev, "clk for node %s\n", name);
+   name = "clk_usbphy_480m";
+   dev_read_string_index(dev, "clock-output-names", 0, &name);
+
+   dev_dbg(dev, "clk %s for node %s\n", name, 
ofnode_get_name(node));
+
ret = device_bind_driver_to_node(dev, "rockchip_usb2phy_clock",

[PATCH v6 1/2] arm: dts: rockchip: rk3399: usb: ehci: Fix EHCI probe in rk3399 to access peripherals by USB 2.

[Xavier Drudis Ferran]

arch/arm/dts/rk3399.dtsi has a node

  usb_host0_ehci: usb@fe38 {
   compatible = "generic-ehci";

with clocks:

   clocks = <&cru HCLK_HOST0>, <&cru HCLK_HOST0_ARB>,
<&u2phy0>;

The first 2 refer to nodes with class UCLASS_CLK, but &u2phy0
has class UCLASS_PHY.

  u2phy0: usb2phy@e450 {
   compatible = "rockchip,rk3399-usb2phy";

Since clk_get_bulk() only looks for devices with UCLASS_CLK,
it fails with -ENODEV and then ehci_usb_probe() aborts.

The consequence is peripherals connected to a USB 2 port (e.g. in a
Rock Pi 4 the white port, nearer the edge) not being detected.
They're detected if CONFIG_USB_OHCI_GENERIC is selected in Kconfig,
because ohci_usb_probe() does not abort when one clk_get_by_index()
fails, but then they work in USB 1 mode.

rk3399.dtsi comes from linux and the  u2phy0 was added[1] to the clock
list in:

commit b5d1c57299734f5b54035ef2e61706b83041f20c
Author: William wu 
Date:   Wed Dec 21 18:41:05 2016 +0800

arm64: dts: rockchip: add u2phy clock for ehci and ohci of rk3399

We found that the suspend process was blocked when it run into
ehci/ohci module due to clk-480m of usb2-phy was disabled.
[...]

Suspend concerns don't apply to U-Boot, and the problem with U-Boot
failing to probe EHCI doesn't apply to linux, because in linux
rockchip_usb2phy_clk480m_register makes u2phy0 a proper clock provider
when called by rockchip_usb2phy_probe().

So I can think of a few alternative solutions:

1- Change ehci_usb_probe() to make it more similar to
   ohci_usb_probe(), and survive failure to get one clock. Looks a
   little harder, and I don't know whether it could break something if
   it ignored a clock that was important for something else than
   suspend.

2- Change rk3399.dtsi effectively reverting the linux commit
   b5d1c57299734f5b54035ef2e61706b83041f20c. This dealigns the .dtsi
   from linux and seems fragile at the next synchronisation.

3- Change the clock list in rk3399-u-boot.dtsi or somewhere else.
   This survives .dts* sync but may survive "too much" and miss some
   change from linux that we might want.

4- Enable CONFIG_USB_OHCI_GENERIC and use the ports in USB 1 mode.
   This would need to be made for all boards using rk3399.  In a
   simple test reading one file from USB storage it gave 769.5 KiB/s
   instead of 20.5 MiB/s with solution 2.

5- Trying to replicate linux and have usb2phy somehow provide a clk,
   or have a separate clock device for usb2phy in addition to the phy
   device.

This patch tries to implement option 5 as Marek Vasut requested in
December 5th.  Options 1 and 3 didn't get through [2][3].

It just registers usb2phy as a clock driver (device_bind_driver()
didn't work but device_bind_driver_to_node() did), without any
specific operations, so that ehci-generic.c finds it and is happy. It
worked in my tests on a Rock Pi 4 B+ (rk3399).

Link: [1] https://lkml.kernel.org/lkml/1731551.Q6cHK6n5ZM@phil/T/
  [2] 
https://patchwork.ozlabs.org/project/uboot/patch/20220701185959.GC1700@begut/
  [3] 
https://patchwork.ozlabs.org/project/uboot/patch/Y44+ayJfUlI08ptM@localhost/
  
Cc: Simon Glass 
Cc: Philipp Tomsich 
Cc: Kever Yang 
Cc: Lukasz Majewski 
Cc: Sean Anderson 
Cc: Marek Vasut 
Cc: Christoph Fritz 
Cc: Jagan Teki 

Signed-off-by: Xavier Drudis Ferran 
---
 drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 23 ++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c 
b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
index 55e1dbcfef..2f31350134 100644
--- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
+++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
@@ -7,7 +7,7 @@
  */
 
 #include 
-#include 
+#include 
 #include 
 #include 
 #include 
@@ -168,6 +168,9 @@ static struct phy_ops rockchip_usb2phy_ops = {
.of_xlate = rockchip_usb2phy_of_xlate,
 };
 
+static struct clk_ops rockchip_usb2phy_clk_ops = {
+};
+
 static int rockchip_usb2phy_probe(struct udevice *dev)
 {
struct rockchip_usb2phy *priv = dev_get_priv(dev);
@@ -249,6 +252,18 @@ static int rockchip_usb2phy_bind(struct udevice *dev)
}
}
 
+   if (!ret) {
+   node = dev_ofnode(dev);
+   name = ofnode_get_name(node);
+   dev_dbg(dev, "clk for node %s\n", name);
+   ret = device_bind_driver_to_node(dev, "rockchip_usb2phy_clock",
+name, node, &usb2phy_dev);
+   if (ret) {
+   dev_err(dev,
+   "'%s' cannot bind 'rockchip_usb2phy_clock'\n", 
name);
+   }
+   }
+
return ret;
 }
 
@@ -366,6 +381,12 @@ U_BOOT_DRIVER(rockchip_usb2phy_port) = {
.ops= &rockchip_usb2phy_ops,

[PATCH v6 0/2] arm: dts: rockchip: rk3399: usb: ehci: Fix EHCI probe in rk3399 to access peripherals by USB 2.

[Xavier Drudis Ferran]

EHCI probing in Rock pi 4 currently fails. 

Add a clock driver for usb2phy so that probing EHCI does not fail when
missing one of the clocks in the bundle for usb_host0_ehci, since
usb2phy is UCLASS_PHY but not UCLASS_CLK.

Xavier Drudis Ferran (2):
  arm: dts: rockchip: rk3399: usb: ehci: Fix EHCI probe in rk3399 to
access peripherals by USB 2.
  arm: rk3399: usb2phy: phy-rockchip-inno-usb2.c: Implement operations
for the 480MHz usb2phy clock in rk3399.

 drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 97 ++-
 1 file changed, 96 insertions(+), 1 deletion(-)

  
Cc: Simon Glass 
Cc: Philipp Tomsich 
Cc: Kever Yang 
Cc: Lukasz Majewski 
Cc: Sean Anderson 
Cc: Marek Vasut 
Cc: Christoph Fritz 
Cc: Jagan Teki 
Signed-off-by: Xavier Drudis Ferran 

---

   Changes:

   v6: just retested over current next branch and some corrections
   to message and headers
   (no changes to code).

   v5: fixes a bug that Christoph Fritz discovered, consisting in the
   wrong eror code returned when enabling or disabling the clock
   because property_enable() returns an error code in linux but
   the modified register value in U-Boot. This caused the clk
   disable to abort before freeing the clock.

   v4: move v3 to one patch in the series and add a second patch
   to add operations to enable disable the usb2phy 480Mhz clock.
   Also, honour clock-output-names for what is worth.

   v3: implement option 5 (bind usb2phy as a clk driver too) instead
   of option 1 (ehci-generic.c tolerates missing clocks).

   v2: implement option 1 (ehci-generic.c tolerates missing clocks)
  instead of option 3 (change dts node to remove the missing
  clock).




-- 
2.20.1

Re: [SPAM] Re: [PATCH v5 0/2] arm: dts: rockchip: rk3399: usb: ehci: Fix EHCI probe in rk3399 to access peripherals by USB 2.

[Xavier Drudis Ferran]

El Fri, Jun 02, 2023 at 03:34:37PM +0530, Jagan Teki deia:
> On Fri, Jun 2, 2023 at 2:54 PM Xavier Drudis Ferran  wrote:
> >
> > Should I try again with the current next branch and send v6 ?
> 
> Please send.
> 
> Thanks,
> Jagan.

I will try asap, thanks.

Re: [SPAM] Re: [PATCH v5 0/2] arm: dts: rockchip: rk3399: usb: ehci: Fix EHCI probe in rk3399 to access peripherals by USB 2.

[Xavier Drudis Ferran]

El Fri, Jun 02, 2023 at 12:11:13PM +0530, Jagan Teki deia:
> 
> Any news about the next revision of this patch? RK3399 has broken
> since release due to this issue. This fix might make upcoming release
> workable.
> 
> Please let us know.
>

Sorry, I'm not sure you meant to send this to me ?

For my side I stopped working on it after I got no feedback for my last version:

https://lists.denx.de/pipermail/u-boot/2023-February/510672.html
https://lists.denx.de/pipermail/u-boot/2023-February/510676.html
https://lists.denx.de/pipermail/u-boot/2023-February/510678.html

I got no replies.

I asked whether I should resend.

https://lists.denx.de/pipermail/u-boot/2023-March/511621.html

> Btw, my v5 seems not to be correctly in patchwork, should I resend ?
> I guess I messed up subject lines ?

And I got no answer, so I thought there were still no interest and
gave up.

Now I see I answered to a question to Eugen only, not the list, so maybe
the list kept waiting for my answer.

Might be some misunderstanding.

Should I try again with the current next branch and send v6 ? 

Re: [PATCH 2/3] phy: rockchip-inno-usb2: add initial support for rk3588 PHY

[Xavier Drudis Ferran]

El Wed, Mar 08, 2023 at 01:59:54PM +0200, Eugen Hristev deia:
> On 3/8/23 13:30, Xavier Drudis Ferran wrote:
> > El Fri, Mar 03, 2023 at 09:31:33AM +0200, Eugen Hristev deia:
> > > @@ -105,6 +130,17 @@ static int rockchip_usb2phy_power_off(struct phy 
> > > *phy)
> > >   struct udevice *parent = dev_get_parent(phy->dev);
> > >   struct rockchip_usb2phy *priv = dev_get_priv(parent);
> > >   const struct rockchip_usb2phy_port_cfg *port_cfg = 
> > > us2phy_get_port(phy);
> > > + struct udevice *vbus = NULL;
> > > + int ret;
> > > +
> > > + vbus = rockchip_usb2phy_check_vbus(phy);
> > > + if (vbus) {
> > > + ret = regulator_set_enable(vbus, false);
> > > + if (ret) {
> > 
> > Could we have
> > if (ret && ret != -EACCES ) {
> > instead here ?
> > (reason below)
> Hi,
> 
> I have nothing against it, the regulator should be all the way optional IMO
> 

Well, at least if it is always-on for whatever reason, then it is not an
error that it cannot be turned off.

> > The apparent reason is that arch/arm/dts/rk3399-rock-pi-4.dtsi
> > says
> > 
> > vcc5v0_host: vcc5v0-host-regulator {
> > compatible = "regulator-fixed";
> > enable-active-high;
> > gpio = <&gpio4 RK_PD1 GPIO_ACTIVE_HIGH>;
> > pinctrl-names = "default";
> > pinctrl-0 = <&vcc5v0_host_en>;
> > regulator-name = "vcc5v0_host";
> > 
> > /*/ regulator-always-on; /*/
> 
> Pretty weird that a regulator that can be turned on/off via a GPIO is
> 'regulator-always-on'. I find this odd and i think it's not correctly
> described at DT level.
>

I don't know enough to tell.  I've just looked a little and it seems
to be used for USB only (on rock pi 4, firefly, eaidk-610,
khadas-edge, leez-p710, nanopc-t4, orangepi, puma, rock960, rockpro64)

Curiously rk3399-evb does NOT have regulator-always-on in vcc5v0_host

and roc-pc seems to add it in u-boot.dtsi only, since it was preserved
at some u-boot - linux sync.

pinebook-pro has regulator-always-on, but then has
regulator-state-mem, regulator-off-in-suspend...

> 
> Anyway, maybe we should move on even if we can't disable the regulator in
> any case ? We should just dev_err and continue ?
>

dev_err or not dev_err depends on whether always-on is always a bug
there or may be a feature, I don't know. But moving on would be nice, yes.

> Kever, do you have any preference ?
> 
> Eugen

Thanks

Re: [PATCH 2/3] phy: rockchip-inno-usb2: add initial support for rk3588 PHY

[Xavier Drudis Ferran]

ld I resend ?
I guess I messed up subject lines ?

I think there are other rk3399 boards that have similar .dtsi files,
but I haven't investigated thoroughly. Do they all have broken EHCI ?
If someone has a rk3399 board with working EHCI maybe they should test
your patch and try usb start ; usb stop and usb start ; usb reset ?

[1] https://lists.denx.de/pipermail/u-boot/2023-February/510672.html

Thanks,

--
Xavier Drudis Ferran

can't reproduce XHCI hang in Rock Pi 4

[Xavier Drudis Ferran]

I'm sorry but I haven't been able to reproduce your issue.


El Fri, Mar 03, 2023 at 11:26:46AM +0100, Xavier Drudis Ferran deia:
> El Fri, Mar 03, 2023 at 10:42:20AM +0100, Christoph Fritz deia:
> 
> Unfortunately I don't think I have any usb-ethernet dongle here to test...
> 
[...]
> > => usb stop
> > stopping USB..
> > => usb start
> > starting USB...
> > Bus usb@fe38: USB EHCI 1.00
> > Bus usb@fe3c: USB EHCI 1.00
> > Bus usb@fe80: Register 2000140 NbrPorts 2
> > Starting the controller
> > USB XHCI 1.10
> > Bus usb@fe90: Register 2000140 NbrPorts 2
> > Starting the controller
> > USB XHCI 1.10
> > scanning bus usb@fe38 for devices... 4 USB Device(s) found
> > scanning bus usb@fe3c for devices... 1 USB Device(s) found
> > scanning bus usb@fe80 for devices... 3 USB Device(s) found
> > scanning bus usb@fe90 for devices... 1 USB Device(s) found
> >scanning usb for storage devices... 0 Storage Device(s) found
> > => usb tree
> > USB device tree:
> >   1  Hub (480 Mb/s, 0mA)
> >   |  u-boot EHCI Host Controller
> >   |
> >   +-2  Hub (480 Mb/s, 100mA)
> > |   USB2.0 Hub
> > |
> > +-3  Hub (480 Mb/s, 0mA)
> > |VIA Labs, Inc.  USB2.0 Hub
> > |
> > +-4  Human Interface (12 Mb/s, 400mA)
> >  ILITEK ILITEK-TP
> > 
> >   1  Hub (480 Mb/s, 0mA)
> >  u-boot EHCI Host Controller
> > 
> >   1  Hub (5 Gb/s, 0mA)
> >   |  U-Boot XHCI Host Controller
> >   |
> >   +-2  Hub (5 Gb/s, 0mA)
> > |  VIA Labs, Inc.  USB3.0 Hub
> > |
> > +-3  Vendor specific (5 Gb/s, 36mA)
> >  Realtek USB 10/100/1000 LAN 00E04C68034E
> > 
> >   1  Hub (5 Gb/s, 0mA)
> >  U-Boot XHCI Host Controller
> >
> 

I tried with a USB3 hub connected to a usb3 port, a sata
adapter connected to the USB3 hub, and a SATA disk connected to the
SATA adapter, and I could read the disk.

The difference is that in my tree both the USB2 and the USB 3 hub
hang from the XHCI controller, not one from EHCI and one from XHCI.

This with my v5 patches on top of 4eb7c5030d3f3c70 (2023-02-19) and some
minor config changes that don't seem to matter.


=> usb start
starting USB...
Bus usb@fe38: USB EHCI 1.00
Bus usb@fe3c: USB EHCI 1.00
Bus usb@fe80: Register 2000140 NbrPorts 2
Starting the controller
USB XHCI 1.10
Bus usb@fe90: Register 2000140 NbrPorts 2
Starting the controller
USB XHCI 1.10
scanning bus usb@fe38 for devices... 1 USB Device(s) found
scanning bus usb@fe3c for devices... 1 USB Device(s) found
scanning bus usb@fe80 for devices... 4 USB Device(s) found
scanning bus usb@fe90 for devices... 1 USB Device(s) found
   scanning usb for storage devices... 1 Storage Device(s) found
=> usb tree
USB device tree:
  1  Hub (480 Mb/s, 0mA)
 u-boot EHCI Host Controller 
   
  1  Hub (480 Mb/s, 0mA)
 u-boot EHCI Host Controller 
   
  1  Hub (5 Gb/s, 0mA)
  |  U-Boot XHCI Host Controller 
  |
  |+-2  Hub (5 Gb/s, 0mA)
  | |  GenesysLogic USB3.0 Hub 
  | |
  | |+-4  Mass Storage (5 Gb/s, 24mA)
  |  Prolific Technology Inc. ATAPI-6 Bridge Controller 01234567895
  |
  |+-3  Hub (480 Mb/s, 100mA)
   GenesysLogic USB2.0 Hub 
 
  1  Hub (5 Gb/s, 0mA)
 U-Boot XHCI Host Controller 
   
=> ls usb 0
   4096 .
   4096 ..
  16384 lost+found
   4096 var
  12288 etc
[...]
=> usb stop
stopping USB..
=> usb start
starting USB...
Bus usb@fe38: USB EHCI 1.00
Bus usb@fe3c: USB EHCI 1.00
Bus usb@fe80: Register 2000140 NbrPorts 2
Starting the controller
USB XHCI 1.10
Bus usb@fe90: Register 2000140 NbrPorts 2
Starting the controller
USB XHCI 1.10
scanning bus usb@fe38 for devices... 1 USB Device(s) found
scanning bus usb@fe3c for devices... 1 USB Device(s) found
scanning bus usb@fe80 for devices... 4 USB Device(s) found
scanning bus usb@fe90 for devices... 1 USB Device(s) found
   scanning usb for storage devices... 1 Storage Device(s) found
=> usb tree
USB device tree:
  1  Hub (480 Mb/s, 0mA)
 u-boot EHCI Host Controller 
   
  1  Hub (480 Mb/s, 0mA)
 u-boot EHCI Host Controller 
   
  1  Hub (5 Gb/s, 0mA)
  |  U-Boot XHCI Host Controller 
  |
  |+-2  Hub (480 Mb/s, 100mA)
  |GenesysLogic USB2.0 Hub 
  |  
  |+-3  Hub (5 Gb/s, 0mA)
|  GenesysLogic USB3.0 Hub 
|
|+-4  Mass Storage (5 Gb/s, 24mA)
 Prolific Technology Inc. ATAPI-6 Bridge Controller 01234567895
   
  1  Hub (5 Gb/s, 0mA)
 U-Boot XHCI Host Controller 
   
=>

[no hang]

I don't know how to help, maybe just try to increase log verbosity or 
something...

Sorry

--
Xavier Drudis Ferran

Re: [PATCH v5 0/2] arm: dts: rockchip: rk3399: usb: ehci: Fix EHCI probe in rk3399 to access peripherals by USB 2.

[Xavier Drudis Ferran]

El Fri, Mar 03, 2023 at 10:42:20AM +0100, Christoph Fritz deia:
> >    Changes:
> > 
> >    v5: fixes a bug that Christoph Fritz discovered, consisting in the
> >    wrong eror code returned when enabling or disabling the clock
> >    because property_enable() returns an error code in linux but
> >    the modified register value in U-Boot. This caused the clk
> >    disable to abort before freeing the clock and it apparently
> >    left things bad enough to cause a hang or a reset.
> > 
> 
> With your patches ontop of v2023.04-rc2, xhci works now fine on a
> rk3399 board:
>

Fine, thanks a lot for testing. I thought the problem was with EHCI.

> Totally unrelated to your patches, stopping usb still crashes (with or
> without your patch) but only when a USB-Ethernet-Dongle (+HUB) is
> connected:
>

Well, but with v4 you got a couple more messages about devices not
removed but children gone. With v5 those messages don't show any
more. So v5 fixes one of the bugs you were seeing, more bugs remain.

Unfortunately I don't think I have any usb-ethernet dongle here to test...

> => usb start
> starting USB...
> do_usb_start, 581
> Bus usb@fe38: USB EHCI 1.00
> Bus usb@fe3c: USB EHCI 1.00
> Bus usb@fe80: Register 2000140 NbrPorts 2
> Starting the controller
> USB XHCI 1.10
> Bus usb@fe90: Register 2000140 NbrPorts 2
> Starting the controller
> USB XHCI 1.10
> scanning bus usb@fe38 for devices... 3 USB Device(s) found
> scanning bus usb@fe3c for devices... 1 USB Device(s) found
> scanning bus usb@fe80 for devices... 1 USB Device(s) found
> scanning bus usb@fe90 for devices... 1 USB Device(s) found
>scanning usb for storage devices... 0 Storage Device(s) found
> => usb tree
> USB device tree:
>   1  Hub (480 Mb/s, 0mA)
>   |  u-boot EHCI Host Controller
>   |
>   +-2  Hub (480 Mb/s, 100mA)
> |   USB2.0 Hub
> |
> +-3  Human Interface (12 Mb/s, 400mA)
>  ILITEK ILITEK-TP
> 
>   1  Hub (480 Mb/s, 0mA)
>  u-boot EHCI Host Controller
> 
>   1  Hub (5 Gb/s, 0mA)
>  U-Boot XHCI Host Controller
> 
>   1  Hub (5 Gb/s, 0mA)
>  U-Boot XHCI Host Controller
>

Ok, so this was with a keyboard or something connected to a USB 2 port
and nothing in the blue USB 3 port, right?

> => usb stop
> stopping USB..
> => usb start
> starting USB...
> Bus usb@fe38: USB EHCI 1.00
> Bus usb@fe3c: USB EHCI 1.00
> Bus usb@fe80: Register 2000140 NbrPorts 2
> Starting the controller
> USB XHCI 1.10
> Bus usb@fe90: Register 2000140 NbrPorts 2
> Starting the controller
> USB XHCI 1.10
> scanning bus usb@fe38 for devices... 4 USB Device(s) found
> scanning bus usb@fe3c for devices... 1 USB Device(s) found
> scanning bus usb@fe80 for devices... 3 USB Device(s) found
> scanning bus usb@fe90 for devices... 1 USB Device(s) found
>scanning usb for storage devices... 0 Storage Device(s) found
> => usb tree
> USB device tree:
>   1  Hub (480 Mb/s, 0mA)
>   |  u-boot EHCI Host Controller
>   |
>   +-2  Hub (480 Mb/s, 100mA)
> |   USB2.0 Hub
> |
> +-3  Hub (480 Mb/s, 0mA)
> |VIA Labs, Inc.  USB2.0 Hub
> |
> +-4  Human Interface (12 Mb/s, 400mA)
>  ILITEK ILITEK-TP
> 
>   1  Hub (480 Mb/s, 0mA)
>  u-boot EHCI Host Controller
> 
>   1  Hub (5 Gb/s, 0mA)
>   |  U-Boot XHCI Host Controller
>   |
>   +-2  Hub (5 Gb/s, 0mA)
> |  VIA Labs, Inc.  USB3.0 Hub
> |
> +-3  Vendor specific (5 Gb/s, 36mA)
>  Realtek USB 10/100/1000 LAN 00E04C68034E
> 
>   1  Hub (5 Gb/s, 0mA)
>  U-Boot XHCI Host Controller
>

And this was with the keyboard in USB2 and a USB3 VIA Labs hub
connected to the USB3 port and a Realtek ethernet-USB dongle attached
to that USB3 hub, right?


> => usb stop
> stopping USB..
> 
> 
> I just quirked/masked the underlying issue by not doing usb_stop() at
> all in drivers/usb/host/usb-uclass.c.
> 

Does usb reset also hang ? If so the problem then must be that you are
left with no way to rescan for devices that were not connected at usb
start but get connected later ?

Did you say before it all worked if you unconfigured EHCI and worked
with XHCI only? (or OHCI + XHCI but no EHCI). Might it have to do with
the same hub hanging from the EHCI and XHCI controller? (but it doesn't
sound strange to me, the hub doesn't know what devices will be
connected to it...)

If you leave drivers/usb/host/usb-uclass.c as it was but drop some
#define DEBUG in device-remove.c does it give any hint ? 

I'll try to do some more tests with a USB3 hub and usb storage...

Thanks again for testing.

[PATCH 2/2] arm: rk3399: usb2phy: phy-rockchip-inno-usb2.c: Implement operations for the 480MHz usb2phy clock in rk3399.

[Xavier Drudis Ferran]

This clock has no users but appears in a phandle list used by
ehci-generic.c to bulk enable it. The phandle list comes from linux,
where it is needed for suspend/resume to work [1].

My tests give the same results with or without this patch, but Marek
Vasut found it weird to declare an empty clk_ops.

So I adapted the code from linux 6.1-rc8 so that it hopefully works
if it ever has some user. For now, without real users, it seems to
at least not give any errors.

Link: [1] https://lkml.kernel.org/lkml/1731551.Q6cHK6n5ZM@phil/T/

Cc: Simon Glass 
Cc: Philipp Tomsich 
Cc: Kever Yang 
Cc: Lukasz Majewski 
Cc: Sean Anderson 
Cc: Marek Vasut 

Signed-off-by: Xavier Drudis Ferran 
---

 v5: ignores the return value from property_enable() which is not
 an error code in U-Boot (unlike in linux). This avoid a false
 failure of rockchip_usb2phy_clk_disable() that interfered with
 clock disable and appeared to cause hang or reset.
 
---
 drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 78 ++-
 1 file changed, 76 insertions(+), 2 deletions(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c 
b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
index 9769ada376..66e0e5aed5 100644
--- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
+++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
@@ -55,6 +55,7 @@ struct rockchip_usb2phy_port_cfg {
 
 struct rockchip_usb2phy_cfg {
unsigned int reg;
+   struct usb2phy_reg  clkout_ctl;
const struct rockchip_usb2phy_port_cfg port_cfgs[USB2PHY_NUM_PORTS];
 };
 
@@ -76,6 +77,18 @@ static inline int property_enable(void *reg_base,
return writel(val, reg_base + reg->offset);
 }
 
+static inline bool property_enabled(void *reg_base,
+   const struct usb2phy_reg *reg)
+{
+   unsigned int tmp, orig;
+   unsigned int mask = GENMASK(reg->bitend, reg->bitstart);
+
+   orig = readl(reg_base + reg->offset);
+
+   tmp = (orig & mask) >> reg->bitstart;
+   return tmp != reg->disable;
+}
+
 static const
 struct rockchip_usb2phy_port_cfg *us2phy_get_port(struct phy *phy)
 {
@@ -168,7 +181,63 @@ static struct phy_ops rockchip_usb2phy_ops = {
.of_xlate = rockchip_usb2phy_of_xlate,
 };
 
+/**
+ * round_rate() - Adjust a rate to the exact rate a clock can provide.
+ * @clk:   The clock to manipulate.
+ * @rate:  Desidered clock rate in Hz.
+ *
+ * Return: rounded rate in Hz, or -ve error code.
+ */
+ulong rockchip_usb2phy_clk_round_rate(struct clk *clk, ulong rate)
+{
+   return 48000;
+}
+
+/**
+ * enable() - Enable a clock.
+ * @clk:   The clock to manipulate.
+ *
+ * Return: zero on success, or -ve error code.
+ */
+int rockchip_usb2phy_clk_enable(struct clk *clk)
+{
+   struct udevice *parent = dev_get_parent(clk->dev);
+   struct rockchip_usb2phy *priv = dev_get_priv(parent);
+   const struct rockchip_usb2phy_cfg *phy_cfg = priv->phy_cfg;
+
+   /* turn on 480m clk output if it is off */
+   if (!property_enabled(priv->reg_base, &phy_cfg->clkout_ctl)) {
+   property_enable(priv->reg_base, &phy_cfg->clkout_ctl, true);
+
+   /* waiting for the clk become stable */
+   usleep_range(1200, 1300);
+   }
+
+   return 0;
+}
+
+/**
+ * disable() - Disable a clock.
+ * @clk:   The clock to manipulate.
+ *
+ * Return: zero on success, or -ve error code.
+ */
+int rockchip_usb2phy_clk_disable(struct clk *clk)
+{
+   struct udevice *parent = dev_get_parent(clk->dev);
+   struct rockchip_usb2phy *priv = dev_get_priv(parent);
+   const struct rockchip_usb2phy_cfg *phy_cfg = priv->phy_cfg;
+
+   /* turn off 480m clk output */
+   property_enable(priv->reg_base, &phy_cfg->clkout_ctl, false);
+
+   return 0;
+}
+
 static struct clk_ops rockchip_usb2phy_clk_ops = {
+   .enable = rockchip_usb2phy_clk_enable,
+   .disable = rockchip_usb2phy_clk_disable,
+   .round_rate = rockchip_usb2phy_clk_round_rate
 };
 
 static int rockchip_usb2phy_probe(struct udevice *dev)
@@ -245,8 +314,11 @@ static int rockchip_usb2phy_bind(struct udevice *dev)
 
if (!ret) {
node = dev_ofnode(dev);
-   name = ofnode_get_name(node);
-   dev_dbg(dev, "clk for node %s\n", name);
+   name = "clk_usbphy_480m";
+   dev_read_string_index(dev, "clock-output-names", 0, &name);
+
+   dev_dbg(dev, "clk %s for node %s\n", name, 
ofnode_get_name(node));
+
ret = device_bind_driver_to_node(dev, "rockchip_usb2phy_clock",
 name, node, &usb2phy_dev);
if (ret) {
@@ -261,6 +333,7 @@ static int rockchip_usb2phy_bind(struct udevice *dev)
 static const struct rockchip_usb2phy_cfg rk3399_usb2phy_cfgs[] = {

Re: [PATCH v5 1/2] arm: dts: rockchip: rk3399: usb: ehci: Fix EHCI probe in rk3399 to access peripherals by USB 2.

[Xavier Drudis Ferran]

arch/arm/dts/rk3399.dtsi has a node

  usb_host0_ehci: usb@fe38 {
   compatible = "generic-ehci";

with clocks:

   clocks = <&cru HCLK_HOST0>, <&cru HCLK_HOST0_ARB>,
<&u2phy0>;

The first 2 refer to nodes with class UCLASS_CLK, but &u2phy0
has class UCLASS_PHY.

  u2phy0: usb2phy@e450 {
   compatible = "rockchip,rk3399-usb2phy";

Since clk_get_bulk() only looks for devices with UCLASS_CLK,
it fails with -ENODEV and then ehci_usb_probe() aborts.

The consequence is peripherals connected to a USB 2 port (e.g. in a
Rock Pi 4 one of the two ports nearer the edge) not being detected.
They're detected if CONFIG_USB_OHCI_GENERIC is selected in Kconfig,
because ohci_usb_probe() does not abort when one clk_get_by_index()
fails, but then they work in USB 1 mode,.

rk3399.dtsi comes from linux and the  u2phy0 was added[1] to the clock
list in:


commit b5d1c57299734f5b54035ef2e61706b83041f20c
Author: William wu 
Date:   Wed Dec 21 18:41:05 2016 +0800

arm64: dts: rockchip: add u2phy clock for ehci and ohci of rk3399

We found that the suspend process was blocked when it run into
ehci/ohci module due to clk-480m of usb2-phy was disabled.
[...]

Suspend concerns don't apply to U-Boot, and the problem with U-Boot
failing to probe EHCI doesn't apply to linux, because in linux
rockchip_usb2phy_clk480m_register makes u2phy0 a proper clock provider
when called by rockchip_usb2phy_probe().

So I can think of a few alternative solutions:

1- Change ehci_usb_probe() to make it more similar to
   ohci_usb_probe(), and survive failure to get one clock. Looks a
   little harder, and I don't know whether it could break something if
   it ignored a clock that was important for something else than
   suspend. (tried in v2)

2- Change rk3399.dtsi effecttively reverting the linux commit
   b5d1c57299734f5b54035ef2e61706b83041f20c. This dealigns the .dtsi
   from linux and seems fragile at the next synchronisation.

3- Change the clock list in rk3399-u-boot.dtsi or somewhere else.
   This survives .dts* sync but may survive "too much" and miss some
   change from linux that we might want. (tried in v1)

4- Enable CONFIG_USB_OHCI_GENERIC and use the ports in USB 1 mode.
   This would need to be made for all boards using rk3399.  In a
   simple test reading one file from USB storage it gave 769.5 KiB/s
   instead of 20.5 MiB/s with solution 2.

5- Trying to replicate linux and have usb2phy somehow provide a clk,
   or have a separate clock device for usb2phy in addition to the phy
   device. (tried in v3 and this v5, v4 did it wrong)

This series is a third attempt to implement option 5 as Marek Vasut
requested in December 5th.  Options 1 and 3 didn't get through[2,3].

The first patch in the series (identical to v3) just registers usb2phy
as a clock driver, without any specific operations, so that
ehci-generic.c finds it and is happy. It worked in my tests on a Rock
Pi 4 B+ (rk3399).

Since Marek Vasut objected to an operationless driver[4], the second
patch adds enable and disable operations adapted from linux prepare
and unprepare operations (and round_rate(), which doesn't seem very
useful anyway since it's a fixed clock). Since there're no users of
this clock in u-boot, I can't see any difference in my tests with only
the first patch or both, so I can't be sure it really works if it's
ever needed, but it's hopefully more complete.

Link: [1] https://lkml.kernel.org/lkml/1731551.Q6cHK6n5ZM@phil/T/

Cc: Simon Glass 
Cc: Philipp Tomsich 
Cc: Kever Yang 
Cc: Lukasz Majewski 
Cc: Sean Anderson 
Cc: Marek Vasut 
Cc: Christoph Fritz 

Signed-off-by: Xavier Drudis Ferran 
---
 drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 23 ++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c 
b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
index b32a498ea7..9769ada376 100644
--- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
+++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
@@ -7,7 +7,7 @@
  */
 
 #include 
-#include 
+#include 
 #include 
 #include 
 #include 
@@ -168,6 +168,9 @@ static struct phy_ops rockchip_usb2phy_ops = {
.of_xlate = rockchip_usb2phy_of_xlate,
 };
 
+static struct clk_ops rockchip_usb2phy_clk_ops = {
+};
+
 static int rockchip_usb2phy_probe(struct udevice *dev)
 {
struct rockchip_usb2phy *priv = dev_get_priv(dev);
@@ -240,6 +243,18 @@ static int rockchip_usb2phy_bind(struct udevice *dev)
}
}
 
+   if (!ret) {
+   node = dev_ofnode(dev);
+   name = ofnode_get_name(node);
+   dev_dbg(dev, "clk for node %s\n", name);
+   ret = device_bind_driver_to_node(dev, "rockchip_usb2phy_clock",
+name, node, &usb2phy_dev);

[PATCH v5 0/2] arm: dts: rockchip: rk3399: usb: ehci: Fix EHCI probe in rk3399 to access peripherals by USB 2.

[Xavier Drudis Ferran]

arch/arm/dts/rk3399.dtsi has a node

  usb_host0_ehci: usb@fe38 {
   compatible = "generic-ehci";

with clocks:

   clocks = <&cru HCLK_HOST0>, <&cru HCLK_HOST0_ARB>,
<&u2phy0>;

The first 2 refer to nodes with class UCLASS_CLK, but &u2phy0
has class UCLASS_PHY.

  u2phy0: usb2phy@e450 {
   compatible = "rockchip,rk3399-usb2phy";

Since clk_get_bulk() only looks for devices with UCLASS_CLK,
it fails with -ENODEV and then ehci_usb_probe() aborts.

The consequence is peripherals connected to a USB 2 port (e.g. in a
Rock Pi 4 one of the two ports nearer the edge) not being detected.
They're detected if CONFIG_USB_OHCI_GENERIC is selected in Kconfig,
because ohci_usb_probe() does not abort when one clk_get_by_index()
fails, but then they work in USB 1 mode,.

rk3399.dtsi comes from linux and the  u2phy0 was added[1] to the clock
list in:

commit b5d1c57299734f5b54035ef2e61706b83041f20c
Author: William wu 
Date:   Wed Dec 21 18:41:05 2016 +0800

arm64: dts: rockchip: add u2phy clock for ehci and ohci of rk3399

We found that the suspend process was blocked when it run into
ehci/ohci module due to clk-480m of usb2-phy was disabled.
[...]

Suspend concerns don't apply to U-Boot, and the problem with U-Boot
failing to probe EHCI doesn't apply to linux, because in linux
rockchip_usb2phy_clk480m_register makes u2phy0 a proper clock provider
when called by rockchip_usb2phy_probe().

So I can think of a few alternative solutions:

1- Change ehci_usb_probe() to make it more similar to
   ohci_usb_probe(), and survive failure to get one clock. Looks a
   little harder, and I don't know whether it could break something if
   it ignored a clock that was important for something else than
   suspend. (tried in v2)

2- Change rk3399.dtsi effecttively reverting the linux commit
   b5d1c57299734f5b54035ef2e61706b83041f20c. This dealigns the .dtsi
   from linux and seems fragile at the next synchronisation.

3- Change the clock list in rk3399-u-boot.dtsi or somewhere else.
   This survives .dts* sync but may survive "too much" and miss some
   change from linux that we might want. (tried in v1)

4- Enable CONFIG_USB_OHCI_GENERIC and use the ports in USB 1 mode.
   This would need to be made for all boards using rk3399.  In a
   simple test reading one file from USB storage it gave 769.5 KiB/s
   instead of 20.5 MiB/s with solution 2.

5- Trying to replicate linux and have usb2phy somehow provide a clk,
   or have a separate clock device for usb2phy in addition to the phy
   device. (tried in v3 and this v5, v4 did it wrong)

This series is a third attempt to implement option 5 as Marek Vasut
requested in December 5th.  Options 1 and 3 didn't get through[2,3].

The first patch in the series (identical to v3) just registers usb2phy
as a clock driver, without any specific operations, so that
ehci-generic.c finds it and is happy. It worked in my tests on a Rock
Pi 4 B+ (rk3399).

Since Marek Vasut objected to an operationless driver[4], the second
patch adds enable and disable operations adapted from linux prepare
and unprepare operations (and round_rate(), which doesn't seem very
useful anyway since it's a fixed clock). Since there're no users of
this clock in u-boot, I can't see any difference in my tests with only
the first patch or both, so I can't be sure it really works if it's
ever needed, but it's hopefully more complete.

Links: [1] https://lkml.kernel.org/lkml/1731551.Q6cHK6n5ZM@phil/T/
   [2] 
https://patchwork.ozlabs.org/project/uboot/patch/20220701185959.GC1700@begut/#2954536
   [3] 
https://patchwork.ozlabs.org/project/uboot/patch/Y44+ayJfUlI08ptM@localhost/#3016099
   [4] 
https://patchwork.ozlabs.org/project/uboot/patch/Y5IWpjYLB4aXMy9o@localhost/#3018135
  
Cc: Simon Glass 
Cc: Philipp Tomsich 
Cc: Kever Yang 
Cc: Lukasz Majewski 
Cc: Sean Anderson 
Cc: Marek Vasut 
Cc: Christoph Fritz 
Signed-off-by: Xavier Drudis Ferran 
---

   Changes:

   v5: fixes a bug that Christoph Fritz discovered, consisting in the
   wrong eror code returned when enabling or disabling the clock
   because property_enable() returns an error code in linux but
   the modified register value in U-Boot. This caused the clk
   disable to abort before freeing the clock and it apparently
   left things bad enough to cause a hang or a reset.

   v4: move v3 to one patch in the series and add a second patch
   to add operations to enable disable the usb2phy 480Mhz clock.
   Also, honour clock-output-names for what is worth.

   v3: implement option 5 (bind usb2phy as a clk driver too) instead
   of option 1 (ehci-generic.c tolerates missing clocks).

   v2: implement option 1 (ehci-generic.c tolerates missing clocks)
  instead of option 3 (change dts node to remove the missing
  clock).

Re: [PATCH v4 0/2] arm: dts: rockchip: rk3399: usb: ehci: Fix EHCI probe in rk3399 to access peripherals by USB 2.

[Xavier Drudis Ferran]

El Sun, Feb 19, 2023 at 08:48:57PM +0100, Christoph Fritz deia:
> 
> I have tested both of your patches on an rk3399:
> 
> without patches applied:
> 
>   | starting USB...
>   | Bus usb@fe38: ehci_generic usb@fe38: Failed to get clocks 
> (ret=-19)
>   | Port not available.
> 
> with patches applied:
> 
>   | starting USB...
>   | Bus usb@fe38: USB EHCI 1.00
>   | Bus usb@fe3c: USB EHCI 1.00
>   | Bus usb@fe80: Register 2000140 NbrPorts 2
> 
> 
> 'usb stop' still makes u-boot hang, but with your patches applied
> following output gets printed before:
> 
>   | => usb stop
>   | stopping USB..
>   | device_remove: Device 'usb@fe38' failed to remove, but children are 
> gone
>   | device_remove: Device 'usb@fe3c' failed to remove, but children are 
> gone
>   
> 
> Without CONFIG_USB_EHCI_HCD 'usb stop' works just fine.
> 
> Thanks
>   -- Christoph
> 

Thank you a lot for testing and reporting.

I think I found the cause. property_enable() returns an error code in
linux but just the value written to the register in U-Boot. So I'll be
sending an v5 with a change to ignore the return from
property_enable() when enabling and disabling the clock, just as other
calls in the file do.

Alternatively one could use v1, v2 or v3 of the patch. But since they
weren't accepted I'll try an v5.

I wellcome help testing or ideas on how to test whether an output
clock is correctly enabled or disabled when everything seems to work
fine already without enabling it, but it needs to be enabled simply
because of a dts coming from linux where it seems to have effects on
suspend/resume.

Thanks,

--
Xavier Drudis Ferran

Re: [PATCH] usb: host: ehci-generic: Handle DM_RESET=n case

[Xavier Drudis Ferran]

El Tue, Jan 24, 2023 at 09:07:58AM +0100, Patrice CHOTARD deia:
> Hi Marek
> 
> On 1/23/23 23:32, Marek Vasut wrote:
> > In case CONFIG_DM_RESET=n, reset_get_bulk() returns -ENOTSUPP.
> > Do not fail in that case either. This is a valid use case, e.g.
> > in case the reset driver is a no-op and would only waste space
> > in the build.
> > 
> > Fixes: 81755b8c20f ("usb: host: ehci-generic: Make resets and clocks 
> > optional")
> > Signed-off-by: Marek Vasut 
> > ---
> > Cc: Andre Przywara 
> > Cc: Patrice Chotard 
> > ---
> >  drivers/usb/host/ehci-generic.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/usb/host/ehci-generic.c 
> > b/drivers/usb/host/ehci-generic.c
> > index a765a307a32..83fe701ff67 100644
> > --- a/drivers/usb/host/ehci-generic.c
> > +++ b/drivers/usb/host/ehci-generic.c
> > @@ -81,7 +81,7 @@ static int ehci_usb_probe(struct udevice *dev)
> > }
> >  
> > err = reset_get_bulk(dev, &priv->resets);
> > -   if (err && err != -ENOENT) {
> > +   if (err && err != -ENOENT && err != -ENOTSUPP) {
> > dev_err(dev, "Failed to get resets (err=%d)\n", err);
> > goto clk_err;
> > }
> 
> A similar patch can be applied for testing return value of clk_get_bulk() ?
> In case CONFIG_CLK is not set, clk_get_bulk() can return -ENOSYS.
>

I thought without CONFIG_CLK the USB port wouldn't work in general ?
Is there a case where it works ? (a board with the clocks enabled
before U-Boot or something?)  I once sent a patch[1] allowing some of
the clocks to fail to be got, but it still returned error if they all
failed.

[1] https://patchwork.ozlabs.org/project/uboot/patch/Y44+ayJfUlI08ptM@localhost/

Re: Problema amb el mail: Helo command rejected: Host not found

[Xavier Drudis Ferran]

El Mon, Dec 12, 2022 at 02:04:56PM +0100, Joan deia:
> En una debian 11 a la que he posat el postfix perquè m'envii el mail
> del sistema (el que arriba a root), tinc aquest error si me l'envio
> al meu mail, que és del mateix domini, però està en una altre màquina
> / subdomini.
> 
> Dec 12 13:37:32 berta postfix/smtpd[16972]: NOQUEUE: reject:
> RCPT from safareig.calbasi.net[75.119.147.123]: 450 4.7.1
> : Helo command rejected: Host not found;

safareig acaba amb g no x.

Vols dir que no has posat l'adreça malament ?

o ho fas expressament, tens un subdomini safareig i un subdomini safareix ?

> from= to= proto=ESMTP
> helo=
>
de *g* a *x* 

Re: [PATCH v4 2/2] arm: rk3399: usb2phy: phy-rockchip-inno-usb2.c: Implement operations for the 480MHz usb2phy clock in rk3399.

[Xavier Drudis Ferran]

El Sun, Dec 11, 2022 at 06:20:41AM +0100, Marek Vasut deia:
> On 12/9/22 16:47, Xavier Drudis Ferran wrote:
> > This clock has no users but appears in a phandle list used by
> > ehci-generic.c to bulk enable it. The phandle list comes from linux,
> > where it is needed for suspend/resume to work [1].
> > 
> > My tests give the same results with or without this patch, but Marek
> > Vasut found it weird to declare an empty clk_ops[2].
> > 
> > So I adapted the code from linux 6.1-rc8 so that it hopefully works
> > if it ever has some user. For now, without real users, it seems to
> > at least not give any errors.
> 
> You might want to squash 1/2 and 2/2 together, since it's one change (add
> clock operations to a phy driver).
>

I prefer to send them separate, but I don't mind if they are squashed
when applied. In my mind it is better if the second patch can be
reverted easily if it ever is found to give any trouble (or just
increase code size). But I'm not the most knowledgeable person in
this list by a huge margin, so if mantainers apply it squashed I'll be
happy with my 2 USB2 ports working.

In fact I don't mind if u-boot takes v4, v3, v2 or v1 of this, or any
other solution, I just want 2 USB2 ports in Rock Pi 4B to be usable in
u-boot.  As it is now only the USB3 ports work in u-boot, unless one
enables CONFIG_OCHI_GENERIC, and then the USB2 ports would work slowly.

> Since 1/2 works without any clock operations, who does enable these usb
> clock on this SoC ? Is there any driver or platform code for that ? If so,
> you can drop that platform code with this driver-side implementation in
> place (which is nice).
>

I'm sorry if I'm not being clear.

I can't drop that code, it needs to enable 2 clocks, but it is
enabling 3.  I can remove one clock from the list of enabled clocks (I
did so in v1) or I can leave it and ignore the error (I did so in
v2). But since that didn't seem to please the list, I tried v3 and v4.
Maybe I just added noise and should have waited further opinions.

ehci-generic.c bulk enables all clocks in the phandle list assigned to the
usb host (usb_host_0ehci).
The arch/arm/dts/rk3399.dtsi provides such a list.

But the last clock is the usb phy &u2phy0. The phy is otherwise
enabled, and the output clock associated with it seems not to serve
any purpose, since u-boot works when I remove &u2phy0 from the clock
phandle list (v1), and also when I leave it there but ignore the error
for the missing clock (v2). In master, v1 and v2 &u2phy0 is
UCLASS_PHY, but not UCLASS_CLK.

With version v3 the &u2phy0 is bound as UCLASS_CLK too, so the bulk
enable from ehci-generic succeeds, although it does nothing to the
hardware (the clock driver having no operations). This does not have
any bad effect as far as I can see. The simple fact that ehci-generic
doesn't fail makes the 2 USB2 ports usable in u-boot.

With v4 the &u2phy0 is bound as UCLASS_CLK, it is bulk enabled, and
that ends up calling an operation that does touch the hardware. This
still doesn't have any bad effect, but I can't tell that it works, because
it does the same as in v3 or v2 or v1.

So when I say there are no users, I don't mean the code isn't called.
It is, but the behaviour I can see is the same when it is and when it
isn't, so whatever the enabled clock does, it does not seem that
u-boot is using it.

Why is it in the phandle list then ? Because it apparently helps linux
resume or suspend. When the output clock is not enabled, something
seems to block linux resume. But I'm not testing linux resume, and it
doesn't matter, because in linux the output clock is enabled anyway,
because &u2phy0 provides a clk, and is in the clock phandle list of
usb_host0_ehci).

Since u-boot and linux share rk3399.dtsi the &u2phy0 is in the phandle list
in u-boot too, but since in master it isn't UCLASS_CLK, ehci-generic fails
and USB2 is not available in that port. 

Maybe there is some use for the clock in u-boot that I'm not seeing because
I don't test good enough or I don't know enough about USB, or lack USB testing
equipment, or something. 

> btw I am still hoping some of the rockchip people will have a look at this
> series.

Yes, I'm looking forward to it too. So far Kever Yang reacted in late August
to v1 by proposing v2 (as far as I understand).

Thanks for listening to my walls of text.

--
Xavi

Re: [PATCH v4 2/2] arm: rk3399: usb2phy: phy-rockchip-inno-usb2.c: Implement operations for the 480MHz usb2phy clock in rk3399.

[Xavier Drudis Ferran]

This clock has no users but appears in a phandle list used by
ehci-generic.c to bulk enable it. The phandle list comes from linux,
where it is needed for suspend/resume to work [1].

My tests give the same results with or without this patch, but Marek
Vasut found it weird to declare an empty clk_ops[2].

So I adapted the code from linux 6.1-rc8 so that it hopefully works
if it ever has some user. For now, without real users, it seems to
at least not give any errors.

Links: [1] https://lkml.kernel.org/lkml/1731551.Q6cHK6n5ZM@phil/T/
   [2] 
https://patchwork.ozlabs.org/project/uboot/patch/Y5IWpjYLB4aXMy9o@localhost/#3018135
   
Cc: Simon Glass 
Cc: Philipp Tomsich 
Cc: Kever Yang 
Cc: Lukasz Majewski 
Cc: Sean Anderson 
Cc: Marek Vasut 

Signed-off-by: Xavier Drudis Ferran 
---
 drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 79 ++-
 1 file changed, 77 insertions(+), 2 deletions(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c 
b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
index cfbdc7d87e..766dde11a6 100644
--- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
+++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
@@ -55,6 +55,7 @@ struct rockchip_usb2phy_port_cfg {
 
 struct rockchip_usb2phy_cfg {
unsigned int reg;
+   struct usb2phy_reg  clkout_ctl;
const struct rockchip_usb2phy_port_cfg port_cfgs[USB2PHY_NUM_PORTS];
 };
 
@@ -76,6 +77,18 @@ static inline int property_enable(void *reg_base,
return writel(val, reg_base + reg->offset);
 }
 
+static inline bool property_enabled(void *reg_base,
+   const struct usb2phy_reg *reg)
+{
+   unsigned int tmp, orig;
+   unsigned int mask = GENMASK(reg->bitend, reg->bitstart);
+
+   orig = readl(reg_base + reg->offset);
+
+   tmp = (orig & mask) >> reg->bitstart;
+   return tmp != reg->disable;
+}
+
 static const
 struct rockchip_usb2phy_port_cfg *us2phy_get_port(struct phy *phy)
 {
@@ -168,7 +181,64 @@ static struct phy_ops rockchip_usb2phy_ops = {
.of_xlate = rockchip_usb2phy_of_xlate,
 };
 
+/**
+ * round_rate() - Adjust a rate to the exact rate a clock can provide.
+ * @clk:   The clock to manipulate.
+ * @rate:  Desidered clock rate in Hz.
+ *
+ * Return: rounded rate in Hz, or -ve error code.
+ */
+ulong rockchip_usb2phy_clk_round_rate(struct clk *clk, ulong rate)
+{
+   return 48000;
+}
+
+/**
+ * enable() - Enable a clock.
+ * @clk:   The clock to manipulate.
+ *
+ * Return: zero on success, or -ve error code.
+ */
+int rockchip_usb2phy_clk_enable(struct clk *clk)
+{
+   struct udevice *parent = dev_get_parent(clk->dev);
+   struct rockchip_usb2phy *priv = dev_get_priv(parent);
+   const struct rockchip_usb2phy_cfg *phy_cfg = priv->phy_cfg;
+   int ret;
+
+   /* turn on 480m clk output if it is off */
+   if (!property_enabled(priv->reg_base, &phy_cfg->clkout_ctl)) {
+   ret = property_enable(priv->reg_base, &phy_cfg->clkout_ctl, 
true);
+   if (ret)
+   return ret;
+
+   /* waiting for the clk become stable */
+   usleep_range(1200, 1300);
+   }
+
+   return 0;
+}
+
+/**
+ * disable() - Disable a clock.
+ * @clk:   The clock to manipulate.
+ *
+ * Return: zero on success, or -ve error code.
+ */
+int rockchip_usb2phy_clk_disable(struct clk *clk)
+{
+   struct udevice *parent = dev_get_parent(clk->dev);
+   struct rockchip_usb2phy *priv = dev_get_priv(parent);
+   const struct rockchip_usb2phy_cfg *phy_cfg = priv->phy_cfg;
+
+   /* turn off 480m clk output */
+   return property_enable(priv->reg_base, &phy_cfg->clkout_ctl, false);
+}
+
 static struct clk_ops rockchip_usb2phy_clk_ops = {
+   .enable = rockchip_usb2phy_clk_enable,
+   .disable = rockchip_usb2phy_clk_disable,
+   .round_rate = rockchip_usb2phy_clk_round_rate
 };
 
 static int rockchip_usb2phy_probe(struct udevice *dev)
@@ -245,8 +315,11 @@ static int rockchip_usb2phy_bind(struct udevice *dev)
 
if (!ret) {
node = dev_ofnode(dev);
-   name = ofnode_get_name(node);
-   dev_dbg(dev, "clk for node %s\n", name);
+   name = "clk_usbphy_480m";
+   dev_read_string_index(dev, "clock-output-names", 0, &name);
+
+   dev_dbg(dev, "clk %s for node %s\n", name, 
ofnode_get_name(node));
+
ret = device_bind_driver_to_node(dev, "rockchip_usb2phy_clock",
 name, node, &usb2phy_dev);
if (ret) {
@@ -261,6 +334,7 @@ static int rockchip_usb2phy_bind(struct udevice *dev)
 static const struct rockchip_usb2phy_cfg rk3399_usb2phy_cfgs[] = {
{
.reg= 0xe450,
+   .clkout_ctl = { 0xe450, 4, 4, 1, 

Re: [PATCH v4 1/2] arm: rockchip: rk3399: usb: ehci: Fix EHCI probe in rk3399 to access peripherals by USB 2.

[Xavier Drudis Ferran]

arch/arm/dts/rk3399.dtsi has a node

  usb_host0_ehci: usb@fe38 {
   compatible = "generic-ehci";

with clocks:

   clocks = <&cru HCLK_HOST0>, <&cru HCLK_HOST0_ARB>,
<&u2phy0>;

The first 2 refer to nodes with class UCLASS_CLK, but &u2phy0
has class UCLASS_PHY.

  u2phy0: usb2phy@e450 {
   compatible = "rockchip,rk3399-usb2phy";

Since clk_get_bulk() only looks for devices with UCLASS_CLK,
it fails with -ENODEV and then ehci_usb_probe() aborts.

The consequence is peripherals connected to a USB 2 port (e.g. in a
Rock Pi 4 the white port, nearer the edge) not being detected.
They're detected if CONFIG_USB_OHCI_GENERIC is selected in Kconfig,
because ohci_usb_probe() does not abort when one clk_get_by_index()
fails, but then they work in USB 1 mode,.

rk3399.dtsi comes from linux and the  u2phy0 was added[1] to the clock
list in:

commit b5d1c57299734f5b54035ef2e61706b83041f20c
Author: William wu 
Date:   Wed Dec 21 18:41:05 2016 +0800

arm64: dts: rockchip: add u2phy clock for ehci and ohci of rk3399

We found that the suspend process was blocked when it run into
ehci/ohci module due to clk-480m of usb2-phy was disabled.
[...]

Suspend concerns don't apply to U-Boot, and the problem with U-Boot
failing to probe EHCI doesn't apply to linux, because in linux
rockchip_usb2phy_clk480m_register makes u2phy0 a proper clock provider
when called by rockchip_usb2phy_probe().


This patch registers usb2phy as a clock driver (device_bind_driver()
didn't work but device_bind_driver_to_node() did), without any
specific operations, so that ehci-generic.c finds it and is happy. It
worked in my tests on a Rock Pi 4 B+ (rk3399).

Link: [1] https://lkml.kernel.org/lkml/1731551.Q6cHK6n5ZM@phil/T/

Cc: Simon Glass 
Cc: Philipp Tomsich 
Cc: Kever Yang 
Cc: Lukasz Majewski 
Cc: Sean Anderson 
Cc: Marek Vasut 

Signed-off-by: Xavier Drudis Ferran 

---
 drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 23 ++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c 
b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
index 62b8ba3a4a..97a1e11239 100644
--- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
+++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
@@ -7,7 +7,7 @@
  */
 
 #include 
-#include 
+#include 
 #include 
 #include 
 #include 
@@ -168,6 +168,9 @@ static struct phy_ops rockchip_usb2phy_ops = {
.of_xlate = rockchip_usb2phy_of_xlate,
 };
 
+static struct clk_ops rockchip_usb2phy_clk_ops = {
+};
+
 static int rockchip_usb2phy_probe(struct udevice *dev)
 {
struct rockchip_usb2phy *priv = dev_get_priv(dev);
@@ -240,6 +243,18 @@ static int rockchip_usb2phy_bind(struct udevice *dev)
}
}
 
+   if (!ret) {
+   node = dev_ofnode(dev);
+   name = ofnode_get_name(node);
+   dev_dbg(dev, "clk for node %s\n", name);
+   ret = device_bind_driver_to_node(dev, "rockchip_usb2phy_clock",
+name, node, &usb2phy_dev);
+   if (ret) {
+   dev_err(dev,
+   "'%s' cannot bind 'rockchip_usb2phy_clock'\n", 
name);
+   }
+   }
+
return ret;
 }
 
@@ -303,6 +318,12 @@ U_BOOT_DRIVER(rockchip_usb2phy_port) = {
.ops= &rockchip_usb2phy_ops,
 };
 
+U_BOOT_DRIVER(rockchip_usb2phy_clock) = {
+   .name   = "rockchip_usb2phy_clock",
+   .id = UCLASS_CLK,
+   .ops= &rockchip_usb2phy_clk_ops,
+};
+
 U_BOOT_DRIVER(rockchip_usb2phy) = {
.name   = "rockchip_usb2phy",
.id = UCLASS_PHY,
-- 
2.20.1

Re: [PATCH v4 0/2] arm: dts: rockchip: rk3399: usb: ehci: Fix EHCI probe in rk3399 to access peripherals by USB 2.

[Xavier Drudis Ferran]

arch/arm/dts/rk3399.dtsi has a node

  usb_host0_ehci: usb@fe38 {
   compatible = "generic-ehci";

with clocks:

   clocks = <&cru HCLK_HOST0>, <&cru HCLK_HOST0_ARB>,
<&u2phy0>;

The first 2 refer to nodes with class UCLASS_CLK, but &u2phy0
has class UCLASS_PHY.

  u2phy0: usb2phy@e450 {
   compatible = "rockchip,rk3399-usb2phy";

Since clk_get_bulk() only looks for devices with UCLASS_CLK,
it fails with -ENODEV and then ehci_usb_probe() aborts.

The consequence is peripherals connected to a USB 2 port (e.g. in a
Rock Pi 4 the white port, nearer the edge) not being detected.
They're detected if CONFIG_USB_OHCI_GENERIC is selected in Kconfig,
because ohci_usb_probe() does not abort when one clk_get_by_index()
fails, but then they work in USB 1 mode,.

rk3399.dtsi comes from linux and the  u2phy0 was added[1] to the clock
list in:

commit b5d1c57299734f5b54035ef2e61706b83041f20c
Author: William wu 
Date:   Wed Dec 21 18:41:05 2016 +0800

arm64: dts: rockchip: add u2phy clock for ehci and ohci of rk3399

We found that the suspend process was blocked when it run into
ehci/ohci module due to clk-480m of usb2-phy was disabled.
[...]

Suspend concerns don't apply to U-Boot, and the problem with U-Boot
failing to probe EHCI doesn't apply to linux, because in linux
rockchip_usb2phy_clk480m_register makes u2phy0 a proper clock provider
when called by rockchip_usb2phy_probe().

So I can think of a few alternative solutions:

1- Change ehci_usb_probe() to make it more similar to
   ohci_usb_probe(), and survive failure to get one clock. Looks a
   little harder, and I don't know whether it could break something if
   it ignored a clock that was important for something else than
   suspend.

2- Change rk3399.dtsi effecttively reverting the linux commit
   b5d1c57299734f5b54035ef2e61706b83041f20c. This dealigns the .dtsi
   from linux and seems fragile at the next synchronisation.

3- Change the clock list in rk3399-u-boot.dtsi or somewhere else.
   This survives .dts* sync but may survive "too much" and miss some
   change from linux that we might want.

4- Enable CONFIG_USB_OHCI_GENERIC and use the ports in USB 1 mode.
   This would need to be made for all boards using rk3399.  In a
   simple test reading one file from USB storage it gave 769.5 KiB/s
   instead of 20.5 MiB/s with solution 2.

5- Trying to replicate linux and have usb2phy somehow provide a clk,
   or have a separate clock device for usb2phy in addition to the phy
   device.

This series is a second attempt to implement option 5 as Marek Vasut
requested in December 5th.  Options 1 and 3 didn't get through[2,3].

The first patch in the series (identical to v3) just registers usb2phy
as a clock driver (device_bind_driver() didn't work but
device_bind_driver_to_node() did), without any specific operations, so
that ehci-generic.c finds it and is happy. It worked in my tests on a
Rock Pi 4 B+ (rk3399).

Since Marek Vasut objected to an operationless driver[4], the second
patch adds enable and disable operations adapted from linux prepare
and unprepare operations (and round_rate(), which doesn't seem very
useful anyway since it's a fixed clock). Since there're no users of
this clock in u-boot, I can't see any difference in my tests with only
the first patch or both, so I can't be sure it really works if it's
ever needed, but it's hopefully more complete.

Links: [1] https://lkml.kernel.org/lkml/1731551.Q6cHK6n5ZM@phil/T/
   [2] 
https://patchwork.ozlabs.org/project/uboot/patch/20220701185959.GC1700@begut/#2954536
   [3] 
https://patchwork.ozlabs.org/project/uboot/patch/Y44+ayJfUlI08ptM@localhost/#3016099
   [4] 
https://patchwork.ozlabs.org/project/uboot/patch/Y5IWpjYLB4aXMy9o@localhost/#3018135
  
Cc: Simon Glass 
Cc: Philipp Tomsich 
Cc: Kever Yang 
Cc: Lukasz Majewski 
Cc: Sean Anderson 
Cc: Marek Vasut 

Signed-off-by: Xavier Drudis Ferran 
---

   Changes:

   v4: move v3 to one patch in the series and add a second patch
   to add operations to enable disable the usb2phy 480Mhz clock.
   Also, honour clock-output-names for what is worth.

   v3: implement option 5 (bind usb2phy as a clk driver too) instead
   of option 1 (ehci-generic.c tolerates missing clocks).

   v2: implement option 1 (ehci-generic.c tolerates missing clocks)
  instead of option 3 (change dts node to remove the missing
  clock).

Re: [PATCH v3] arm: dts: rockchip: rk3399: usb: ehci: Fix EHCI probe in rk3399 to access peripherals by USB 2.

[Xavier Drudis Ferran]

El Thu, Dec 08, 2022 at 09:12:08PM +0100, Marek Vasut deia:
> On 12/8/22 17:53, Xavier Drudis Ferran wrote:
> 
> [...]
> 
> > +++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> > @@ -7,7 +7,7 @@
> >*/
> >   #include 
> > -#include 
> > +#include 
> >   #include 
> >   #include 
> >   #include 
> > @@ -168,6 +168,9 @@ static struct phy_ops rockchip_usb2phy_ops = {
> > .of_xlate = rockchip_usb2phy_of_xlate,
> >   };
> > +static struct clk_ops rockchip_usb2phy_clk_ops = {
> > +};
> 
> Is this empty structure needed ? Why ?
> 
> Either it shouldn't be here, or it should implement some callbacks, like
> clock enable/disable ?
>

I tried without it but it gave me a runtime error.  I think I have the
log somewhere if you want to see it.  It looked like a null pointer
dereference at first sight.  I just added it and it got fixed. I
didn't research what the failing functions were trying to do.

Or is this a common case and there's some null_clk_ops() function or
macro magic or something somewhere ?

The thing is nobody is using this clk in u-boot. It's just its phandle
in a clock phandle list that ehci-generic.c happens to use in bulk. So
the default implementation seems to be enough to allocate, enable and
release it in bulk with its clk set. Or at least to call the functions
without error.

As I have left it, it might not work if ever someone wants to use it.
But should I try to implement it so that it is usable ? How should I
test it ?  Shouldn't we wait until someone has some real use for it ?
Linux spent sometime without it in the phandle list until William wu
discovered it was needed for suspend/resume, so u-boot may never need
it.

Or should I add a comment in the code ?

> Otherwise looks much better, thanks !

Thanks.

[PATCH v3] arm: dts: rockchip: rk3399: usb: ehci: Fix EHCI probe in rk3399 to access peripherals by USB 2.

[Xavier Drudis Ferran]

arch/arm/dts/rk3399.dtsi has a node

  usb_host0_ehci: usb@fe38 {
   compatible = "generic-ehci";

with clocks:

   clocks = <&cru HCLK_HOST0>, <&cru HCLK_HOST0_ARB>,
<&u2phy0>;

The first 2 refer to nodes with class UCLASS_CLK, but &u2phy0
has class UCLASS_PHY.

  u2phy0: usb2phy@e450 {
   compatible = "rockchip,rk3399-usb2phy";

Since clk_get_bulk() only looks for devices with UCLASS_CLK,
it fails with -ENODEV and then ehci_usb_probe() aborts.

The consequence is peripherals connected to a USB 2 port (e.g. in a
Rock Pi 4 the white port, nearer the edge) not being detected.
They're detected if CONFIG_USB_OHCI_GENERIC is selected in Kconfig,
because ohci_usb_probe() does not abort when one clk_get_by_index()
fails, but then they work in USB 1 mode,.

rk3399.dtsi comes from linux and the  u2phy0 was added[1] to the clock
list in:

commit b5d1c57299734f5b54035ef2e61706b83041f20c
Author: William wu 
Date:   Wed Dec 21 18:41:05 2016 +0800

arm64: dts: rockchip: add u2phy clock for ehci and ohci of rk3399

We found that the suspend process was blocked when it run into
ehci/ohci module due to clk-480m of usb2-phy was disabled.
[...]

Suspend concerns don't apply to U-Boot, and the problem with U-Boot
failing to probe EHCI doesn't apply to linux, because in linux
rockchip_usb2phy_clk480m_register makes u2phy0 a proper clock provider
when called by rockchip_usb2phy_probe().

So I can think of a few alternative solutions:

1- Change ehci_usb_probe() to make it more similar to
   ohci_usb_probe(), and survive failure to get one clock. Looks a
   little harder, and I don't know whether it could break something if
   it ignored a clock that was important for something else than
   suspend.

2- Change rk3399.dtsi effecttively reverting the linux commit
   b5d1c57299734f5b54035ef2e61706b83041f20c. This dealigns the .dtsi
   from linux and seems fragile at the next synchronisation.

3- Change the clock list in rk3399-u-boot.dtsi or somewhere else.
   This survives .dts* sync but may survive "too much" and miss some
   change from linux that we might want.

4- Enable CONFIG_USB_OHCI_GENERIC and use the ports in USB 1 mode.
   This would need to be made for all boards using rk3399.  In a
   simple test reading one file from USB storage it gave 769.5 KiB/s
   instead of 20.5 MiB/s with solution 2.

5- Trying to replicate linux and have usb2phy somehow provide a clk,
   or have a separate clock device for usb2phy in addition to the phy
   device.

This patch tries to implement option 5 as Marek Vasut requested in
December 5th.  Options 1 and 3 didn't get through[2,3].

It just registers usb2phy as a clock driver (device_bind_driver()
didn't work but device_bind_driver_to_node() did), without any
specific operations, so that ehci-generic.c finds it and is happy. It
worked in my tests on a Rock Pi 4 B+ (rk3399).

Link: [1] https://lkml.kernel.org/lkml/1731551.Q6cHK6n5ZM@phil/T/
  [2] 
https://patchwork.ozlabs.org/project/uboot/patch/20220701185959.GC1700@begut/#2954536
  [3] 
https://patchwork.ozlabs.org/project/uboot/patch/Y44+ayJfUlI08ptM@localhost/#3016099
  
Cc: Simon Glass 
Cc: Philipp Tomsich 
Cc: Kever Yang 
Cc: Lukasz Majewski 
Cc: Sean Anderson 
Cc: Marek Vasut 

Signed-off-by: Xavier Drudis Ferran 
---

   Changes:

   v3: implement option 5 (bind usb2phy as a clk driver too) instead
   of option 1 (ehci-generic.c tolerates missing clocks).

   v2: implement option 1 (ehci-generic.c tolerates missing clocks)
  instead of option 3 (change dts node to remove the missing
  clock).

---
 drivers/usb/host/ehci-generic.c | 59 -
 1 file changed, 58 insertions(+), 1 deletion(-)

---
 drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 23 ++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c 
b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
index 62b8ba3a4a..97a1e11239 100644
--- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
+++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
@@ -7,7 +7,7 @@
  */
 
 #include 
-#include 
+#include 
 #include 
 #include 
 #include 
@@ -168,6 +168,9 @@ static struct phy_ops rockchip_usb2phy_ops = {
.of_xlate = rockchip_usb2phy_of_xlate,
 };
 
+static struct clk_ops rockchip_usb2phy_clk_ops = {
+};
+
 static int rockchip_usb2phy_probe(struct udevice *dev)
 {
struct rockchip_usb2phy *priv = dev_get_priv(dev);
@@ -240,6 +243,18 @@ static int rockchip_usb2phy_bind(struct udevice *dev)
}
}
 
+   if (!ret) {
+   node = dev_ofnode(dev);
+   name = ofnode_get_name(node);
+   dev_dbg(dev, "clk for node %s\n", name);
+   ret = device_bind_driver_to_node(dev, "rockchip_usb2phy_clock",
+   

Re: [SPAM] [PATCH v5 4/7] rockchip: Support building the all output files in binman

[Xavier Drudis Ferran]

El Thu, Dec 08, 2022 at 08:56:57AM +1300, Simon Glass deia:
> + @tee-SEQ {
> + fit,operation = "split-elf";
> + description = "TEE";
> + type = "tee";
> + arch = "arm64";
> + os = "tee";
> + compression = "none";
> + fit,load;
> + fit,entry;
> + fit,data;
> +
> + tee-os {
> + };
> + };

I don't know, I may likely have missed something here, but are you sure you're
taking Jerome into account ?

https://lists.denx.de/pipermail/u-boot/2022-July/490069.html
https://lists.denx.de/pipermail/u-boot/2022-November/499306.html

Re: [PATCH v2] arm: dts: rockchip: rk3399: usb: ehci: Fix EHCI probe in rk3399 to access peripherals by USB 2.

[Xavier Drudis Ferran]

El Mon, Dec 05, 2022 at 08:08:40PM +0100, Marek Vasut deia:
> On 12/5/22 19:54, Xavier Drudis Ferran wrote:
> > 5- Trying to replicate linux and have usb2phy somehow provide a clk,
> > or have a separate clock device for usb2phy in addition to the phy
> > device. I just can't seem to imagine how to achieve this with the
> > U-Boot driver model, maybe because of my limited familiarity with
> > it.
> 
> Yes please
> 
> Have a look at the end of drivers/led/led_gpio.c and how gpio_led_wrap binds
> a gpio_led driver to each LED. You can bind an UCLASS_PHY and UCLASS_CLK
> driver to the u2phy0 the same way, the u2phy0 would behave the same way as
> gpio_led_wrap wrapper . You would likely be using device_bind_driver()
> instead of device_bind_driver_to_node() in the bind callback.
> 


Ok, I will take a look and send a v3 if I understand it.

Thank you.

[PATCH v2] arm: dts: rockchip: rk3399: usb: ehci: Fix EHCI probe in rk3399 to access peripherals by USB 2.

[Xavier Drudis Ferran]

arch/arm/dts/rk3399.dtsi has a node

  usb_host0_ehci: usb@fe38 {
   compatible = "generic-ehci";

with clocks:

   clocks = <&cru HCLK_HOST0>, <&cru HCLK_HOST0_ARB>,
<&u2phy0>;

The first 2 refer to nodes with class UCLASS_CLK, but &u2phy0
has class UCLASS_PHY.

  u2phy0: usb2phy@e450 {
   compatible = "rockchip,rk3399-usb2phy";

Since clk_get_bulk() only looks for devices with UCLASS_CLK,
it fails with -ENODEV and then ehci_usb_probe() aborts.

The consequence is peripherals connected to a USB 2 port (e.g. in a
Rock Pi 4 the white port, nearer the edge) not being detected.
They're detected if CONFIG_USB_OHCI_GENERIC is selected in Kconfig,
because ohci_usb_probe() does not abort when one clk_get_by_index()
fails, but then they work in USB 1 mode,.

rk3399.dtsi comes from linux and the  u2phy0 was added[1] to the clock
list in:

commit b5d1c57299734f5b54035ef2e61706b83041f20c
Author: William wu 
Date:   Wed Dec 21 18:41:05 2016 +0800

arm64: dts: rockchip: add u2phy clock for ehci and ohci of rk3399

We found that the suspend process was blocked when it run into
ehci/ohci module due to clk-480m of usb2-phy was disabled.
[...]

Suspend concerns don't apply to U-Boot, and the problem with U-Boot
failing to probe EHCI doesn't apply to linux, because in linux
rockchip_usb2phy_clk480m_register makes u2phy0 a proper clock provider
when called by rockchip_usb2phy_probe().

So I can think of a few alternative solutions:

1- Change ehci_usb_probe() to make it more similar to
   ohci_usb_probe(), and survive failure to get one clock. Looks a
   little harder, and I don't know whether it could break something if
   it ignored a clock that was important for something else than
   suspend.

2- Change rk3399.dtsi effecttively reverting the linux commit
   b5d1c57299734f5b54035ef2e61706b83041f20c. This dealigns the .dtsi
   from linux and seems fragile at the next synchronisation.

3- Change the clock list in rk3399-u-boot.dtsi or somewhere else.
   This survives .dts* sync but may survive "too much" and miss some
   change from linux that we might want.

4- Enable CONFIG_USB_OHCI_GENERIC and use the ports in USB 1 mode.
   This would need to be made for all boards using rk3399.  In a
   simple test reading one file from USB storage it gave 769.5 KiB/s
   instead of 20.5 MiB/s with solution 2.

5- Trying to replicate linux and have usb2phy somehow provide a clk,
   or have a separate clock device for usb2phy in addition to the phy
   device. I just can't seem to imagine how to achieve this with the
   U-Boot driver model, maybe because of my limited familiarity with
   it.

This patch is option 1 as Kever Yang requested on August 27th[2] when
option 3 didn't get through. Sorry for the delay.

Yet maybe the get_some_clks() function should be added to clk-uclass.c
if it may be useful elsewhere. Or alternatively, clk_get_bulk() could
be changed to the lenient behaviour of get_some_clks(), but that seems
too invasive to me. Either of these changes can always be done in a
later patch if needed.

If so, one possibility would be to call it from ohci-generic.c. As it
is now it looks like if it ever misses a clock but finds a subsequent
clock, assigned to a higher index in the clock table, it may leave
clock_count too low to release all found clocks. I don't know of a
case where this happens, for rk3399 usb, even with non-default
CONFIG_OHCI_GENERIC, the missing clock is the last one, and so the
release iteration happens to find all found clocks.

Link: [1] https://lkml.kernel.org/lkml/1731551.Q6cHK6n5ZM@phil/T/
  [2] 
https://patchwork.ozlabs.org/project/uboot/patch/20220701185959.GC1700@begut/#2954536
  
Cc: Simon Glass 
Cc: Philipp Tomsich 
Cc: Kever Yang 
Cc: Lukasz Majewski 
Cc: Sean Anderson 
Cc: Marek Vasut 

Signed-off-by: Xavier Drudis Ferran 
---

  Changes:
  
  v2: implement option 1 (ehci-generic.c tolerates missing clocks)
  instead of option 3 (change dts node to remove the missing
  clock).
  
---
 drivers/usb/host/ehci-generic.c | 59 -
 1 file changed, 58 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/ehci-generic.c b/drivers/usb/host/ehci-generic.c
index a765a307a3..aa86dcc435 100644
--- a/drivers/usb/host/ehci-generic.c
+++ b/drivers/usb/host/ehci-generic.c
@@ -60,6 +60,63 @@ static int ehci_disable_vbus_supply(struct generic_ehci 
*priv)
return 0;
 }
 
+/**
+ * get_some_clks() - Get/request all available clocks of a
+ *device. Leave other as null.
+ *
+ * @dev:   The client device.
+ * @bulk:  A pointer to a clock bulk struct to initialize.
+ *
+ * This looks up and requests all clocks of the client device; each
+ * device is assumed to have n clocks associated with it somehow, and
+ * this function tries to find and request all of them in a s

Re: configuració /dev/tty1

[Xavier Drudis Ferran]

El Fri, Nov 25, 2022 at 12:03:17PM +0100, Jordi deia:
> De fet em passa sempre i  inutilitza força el terminal. Imagina que
> qualsevol programa interactiu que et demani dades,
> en arribar abaix de tot ja no saps com continuar.
> 
> Salutacions
>

Perdoneu si pixo fora de test perquè jo no m'hi he trobat mai, però
has provat de posar
stty cols 80 rows 20
o els números que et vagin bé per les línies visibles que tens ?

(stty està al paquet coreutils, ja el deus tenir)

Re: (deb-cat) Firefox ESR: out of memory

[Xavier Drudis Ferran]

A mi el firefox-esr 102.5 de bullseye (del repositiori security, suposo) em va 
bé.
Però fa poques hores que el tinc i tampoc he navegat tantíssim, no he fet res 
especial.
Ah, i parlo en arm64.

Igual tens alguna extensió que no acaba d'anar fina amb la nova versió o alguna 
cosa així ?

Re: (deb-cat) Actualitzar Debootstrap

[Xavier Drudis Ferran]

M'alegro que ho hagis arreglat. 

El Fri, Sep 09, 2022 at 01:56:06PM +0200, Narcis Garcia deia:
> 
> El què esperava és una manera més ben prevista d'afrontar aquestes
> situacions, en comptes d'haver de collir Debootstrap de repositoris no
> previstos.
> Per exemple, un web amb la última versió i binaris descarregables per a les
> distribucions. 

Per a mi un repositori ja és una web amb la última versió i binaris 
descarregables. 
Hi ha massa distribucions per fer un paquet per a cadascuna i anar-los 
mantenint. 
I aparentment no cal. 

> O que el mateix Debootstrap tingués alguna opció de
> descàrrega dels últims «scripts» (deboostrap update-scripts).
> 
No sé, no hi haurà conflictes quan després actualizessis el debootstrap pel 
repositori de la distro d'on el vas instal·lar inicialment ?

Re: (deb-cat) Actualitzar Debootstrap

[Xavier Drudis Ferran]

El Thu, Sep 08, 2022 at 11:58:01AM +0200, Narcis Garcia deia:
> Bon dia,
> 
> Tinc un ordinador amb Ubuntu GNU/Linux 14.04 (trusty) i386, i hi vull fer un
> «debootstrap» per a instal·lar Debian 9 (stretch) amd64 en un subdirectori.
> 
> El cas és que l'últim perfil de Debian que té el Debootstrap en aquest
> entorn tant antic (que no haig d'actualitzar) és per a «jessie» (Debian 8).
> Hi ha alguna font d'actualització de «scripts» de Debian per a casos així?
> 
> Gràcies.
>

No sé si t'entenc, però debootstrap no és un paquet normal, és més portable. 
https://wiki.debian.org/Debootstrap
Potser pots instal·lar un debootstrap de Debian >= 9 a l'Ubuntu directament.
I si no vol, agafes un deboostrap instal·lat a un altre ordinador i copies
els fitxers del paquet a l'Ubuntu. hauria de ser només 
/usr/share/debootstrap/* i /usr/sbin/debootstrap
si passes de la documentació. I si no dpkg -L debootstrap et dirà quins 
fitxers té la versió que tinguis instal·lada. 

No ho he provat mai, però igual cola. Potser li has de passar més 
parametres sobre repositoris o alguna cosa, però vaja...

Una altra cosa es veure com arrenques del directori un cop instal·lat si 
no és l'arrel d'alguna partició. Ho tens pensat ?

Re: [PATCH] arm: dts: rockchip: rk3399: usb: ehci: Fix EHCI probe in rk3399 to access peripherals by USB 2.

[Xavier Drudis Ferran]

El Sat, Aug 27, 2022 at 11:20:17AM +0800, Kever Yang deia:
> The idea for dtsi now is to sync the kernel and U-Boot dts, and Linux
> distribution will
> use the dts from U-Boot. If we change the dts property in -u-boot.dts, it
> will also pass
> to kernel, which make kernel function not available. So we would like to
> sync the driver
> for kernel and u-boot to use the same dts nodes.
>

That's news to me. I was thinking all the time that the reason for having 
some dtsi? files with -u-boot suffix was that these were exclusive for u-boot
and the others were shared with linux. If now the content of .*-u-boot.dtsi? 
gets passed to linux too, I no longer know why we separate the files.
Is this for Rockchip or ARM64 or a general rule for all of U-Boot? 
Should I (or someone) try to change documentation or is that already done?
I thought I read something different few months ago ? Maybe I misunderstood...

> So we can try option 1, output warning instead of error return for clock get
> fail, since the clock
> driver for kernel and u-boot are totally different.
> 

Ok. I hope I find time to try that some day. 

Thanks for looking at the patch.

Re: [SPAM] rk3399 TPL memory setup code triggers clock frequency limit assertion

[Xavier Drudis Ferran]

El Mon, Aug 08, 2022 at 11:22:49PM +0530, Jagan Teki deia:
> 
> If I remember correctly when I work with YouMin on LPDDR4 the initial
> code to start to check with was 50MHz (It was not working at that time
> with 48MHz). Not sure what to make other changes to fix that to try on
> 48MHz.
> 

Not sure I understand.

Do you mean when you and YouMin worked in this (thanks for your work) 
you had mesured that the code gave 50MHz ? Maybe. It seems out of spec, so it
doesn't have to give 48MHz, I guess it can give whatever. 48MHz is the concluion
of a theory for which we haven't satisfied the hypothesis.

Or do you mean the code for this clock was different when you worked initially,
and that code gave 50MHz theoretically ? I haven't looked at the git log.

> Better resend the patch again and add YouMin and others to see for comments.
> 

I don't have much time right now to pull, see if it applies still and
test again.  Michal just tried, not sure how clean it might have been
for him, or what base he used, so anyone feel free to resend if you
think it's useful or know better who to put in cc. Who would "others" be ?
If I got my Cc: wrong the first time I fear I'll fail again. Michal
just sent a tested-by to my orignal patch[1]. Should a resend fare
better ? Or how many resends?  I may resend this one line patch when I
have time if nobody has resent yet or merged the original.

YouMin helped me confirm and said something unconclusive in private,
not opposing to change it.

[1] 
https://patchwork.ozlabs.org/project/uboot/patch/20220716103144.GA2167@begut/

Re: [SPAM] [PATCH 2/2] binman: Add more documentation about binman usage

[Xavier Drudis Ferran]

El Sun, Aug 07, 2022 at 04:33:26PM -0600, Simon Glass deia:
> This is an attempt to answer the comments provided by Xavier [1].
> 

Thank you. Sorry if I point out silly things too. No agony intended.

>  
> +Note that binman can itself can create a FIT. This helps to move mkimage

one "can" too much

> +invocations out of the Makefile and into binman image descriptions. It also
> +helps by removing the need for ad-hoc tools like `make_fit_atf.py`.
> +
>

Maybe in future tense?
We're not there yet?, because of tee.bin, see Jerome's message.
Or is tee.bin not officialy supported yet in U-Boot?


> +How do you know how to incorporate ATF? It is handled by the atf-bl31 entry 
> type
> +(etype). An etype is an implementation of reading a binary into binman, in 
> this
> +case the `bl31.bin` file. When you build U-Boot but do not set the BL31
> +environment variable, binman provides a help message, which comes from
> +`missing-blob-help`::
> +
> +See the documentation for your board. You may need to build ARM Trusted
> +Firmware and build with BL31=/path/to/bl31.bin
> +
> +The mechanism by which binman is advised of this is also in the Makefile. See
> +the `-a atf-bl31-path=${BL31}` piece in `cmd_binman`. This tells binman to
> +set the EntryArg `atf-bl31-path` to the value of the `BL31` environment
> +variable. Within binman, this EntryArg is picked up by the `Entry_atf_bl31`
> +etype. An EntryArg is simply an argument to the entry. The `atf-bl31-path`
> +name is documented in :ref:`etype_atf_bl31`.
> +

Still confused. Shouldn't you mention split-elf ?
The way I use it bl31.elf is not simply copied into an image (atf_bl31.py
is just a Entry_blob_named_by_arg). It is sliced in pieces and put into 
u-boot.itb. Each piece carries a little metadada like the load address,
extracted from the elf by make_fit_atf.py or binman with split-elf. 

The problem is binman cannot incorporate an image it produced as a binary into
another, so we must still use make_fit_atf.py. But then binman does not use
atf-bl31, it simply includes a blob with filename="u-boot.itb".

Maybe it's not the best way to do it and people should just add a atf-bl31 {} 
section in their binman image and it somehow works ? Because that's what
I would assume from your text.

Or maybe you're talking of other boards, but then try not to write it as this
is the method one uses always when one has atf (TF-A)?

Anyway, thank you for writing it. The docs help understand many of the issues
and are a good improvement.

Re: [SPAM] rk3399 TPL memory setup code triggers clock frequency limit assertion

[Xavier Drudis Ferran]

El Sun, Aug 07, 2022 at 04:44:04PM +0200, Michal Suchánek deia:
> Hello,
> 
> when compiled with clock debug rk3399 cannot be booted because memory
> setup code triggers clock assertion:
> 
> U-Boot TPL 2022.07-00038-g61e11a8e9f-dirty (Aug 07 2022 - 16:13:17)
> TPL PLL at ff76: fbdiv=50, refdiv=1, postdiv1=2, postdiv2=1, vco=120 
> khz, output=60 khz
> TPL PLL at ff760020: fbdiv=50, refdiv=1, postdiv1=2, postdiv2=1, vco=120 
> khz, output=60 khz
> TPL PLL at ff760080: fbdiv=99, refdiv=2, postdiv1=2, postdiv2=1, vco=1188000 
> khz, output=594000 khz
> TPL PLL at ff760060: fbdiv=64, refdiv=1, postdiv1=2, postdiv2=2, vco=1536000 
> khz, output=384000 khz
> TPL PLL at ff760040: fbdiv=12, refdiv=1, postdiv1=3, postdiv2=2, vco=288000 
> khz, output=48000 khz
> drivers/clk/rockchip/clk_rk3399.c:347: rkclk_set_pll: Assertion `vco_khz >= 
> VCO_MIN_KHZ && vco_khz <= VCO_MAX_KHZ && output_khz >= OUTPUT_MIN_KHZ && 
> output_khz <= OUTPUT_MAX_KHZ && div->fbdiv >= PLL_DIV_MIN && div->fbdiv <= 
> PLL_DIV_MAX' failed.Channel 0: LPDDR4, 50MHz

Sorry, I don't have time now. But It might be related to 

https://patchwork.ozlabs.org/project/uboot/patch/20220716103144.GA2167@begut/

Apparently this clock is wrong but nobody finds any consequence of it being 
wrong. 
If one asks for a 50MHz clock and gets a 48MHz clockthings might work anyway, 
but 
it's nice that at least when one asks to be told of problems one is told.


> 
> What would be a resonable way to make rk3399 bootable with clock debug
> enabled?
>

Try my patch, I don't think it can hurt ?

Re: [SPAM] Re: Replace make-fit-atf.py with binman. Was: migrate u-boot-rockchip.bin to binman and generate an image for SPI

[Xavier Drudis Ferran]

El Tue, Aug 02, 2022 at 06:41:40AM -0600, Simon Glass deia:
> 
> It seems we need a lot more guidance here. I can help write more into
> the packaging docs, perhaps:
> 
> https://u-boot.readthedocs.io/en/latest/develop/package/binman.html#motivation
> 
> Can you please suggest a few questions to answer?
>

I can try. But let me start by saying thank you for the guidance that was 
already there.
I had read it and I still don't understand it together with some of what you 
said on the list.
So how about...:

- Exact criteria for what is a binary you take from external sources
   and and what is a packaged image that's binman responsability. So,
   TF-A is compiled externally, and the U-Boot build system just gets
   a BL31 env variable pointing at an bl31.elf or so. But u-boot.itb,
   despite being a FIT that includes parts of TF-A, maybe parts of
   tee.bin, some dtbs and binaries built by U-Boot itself and who
   knows if public keys or extra dtb overlays provided directly by the
   user, or something I can't think of now, it's not a binary from
   external sources but a U-Boot image that needs to be packaged by
   binman? For me there's some grey zone. Since make_fit_atf.py is in
   U-Boot then u-boot.itb is a binary, but since part of the contents
   are external, and mixed with U-Boot's own code then it's a packaged
   image. But then it's just part of the final packaged image that
   one can store in SPI, SD or wherever.

-  Scope ? how customizable needs the description of the image to be ?

   If a board meeds different images for booting from sd, spi and nand, 
   should the *-u-boot.dts* provide the 3 images or one or ...? 

   Must the image description recognize all possible kconfig options ?
   So should it look for splash source = spi and put a copy of some
   bmp or bmp.gz at some offset if the user opted for splash image in spi?

   Or that's a stretch and it's basically it should work for the
   default config for each board and users who change things should
   change the dts or package manually following the various READMEs ?
   (taking into account that any manual packaging risks the image
   description in dts no longer matching the image in case some soft is
   reading that)

- Use ? Visibility ?

  From the motivation text one would think that users run 
 make boardX_defconfig
 make
 BL31=bl31.elf binman 

  But in fact currently it is make who calls binman. Is it just a temporary 
  situation and the intent is for make just to build binaries and then 
  binman will be called afterwards to package images ? Because binman not 
  only packages what make built, it changes some properties of the dtb that
  was built by make, if I've understood it. In that sense it seems to be 
  part of the build system not just a packager that comes afterwards.

  And the different parameters of binman are supposed to be set by make, 
  and then more technical or is binman supposed to be used by the user
  to build some custom image too?

- What about reuse ?

  the binman recipes go into u-boot.dts* files . But into which ?  The
  SOC .dtsi ? the mach .dtsi ? the board .dtsi ? For example many
  boards include rk3399-u-boot.dtsi. Rk3399 can boot from spi, sd, or
  emmc (the same image can be used for emmc and sd). That's the
  bootrom. In theory SPL (a fatter SPL) could load U-Boot from nvme, I
  think, or USB, or serial, or network but I don't think that's
  implemented?

  So if you put the binman node in rk3399-u-boot.dtsi you could have
  it generate two images, one for sd/emmc and one for spi (plus
  intermediate images that people asked for to customize things). If
  you put it in rockchip-u-boot.dtsi then it can be more reused, but
  maybe then you need a format for nand too? And what if a board
  doesn't have emmc nor SD ? or doesn't have spi NOR? Should it modify
  the inherited binman node to disable one of the images, or just generate
  some unused image and call it a day ? Or you should put your binman
  subnodes in independent .dtsi files and carefully include the needed
  ones in each board -u-boot.dts ? Or you put it all in the most
  generic u-boot.dtsi you can and #ifdef away the parts that apply to
  each board/configuration ?  So far we seem to prefer this last
  approach, but I don't know whether it was a plan or what.


> 
> Because we have lots of scripts with no tests and it will proliferate
> even more. Binman is data-driven, has tests and allows us to build
> common functionality used by all SoCs.
>

Makes sense, tests are necessary, but some functionality may be common
and some may be very specific for some arch or SOC or vendor. If all
specific kirks also go into binman it can get a little complex. It can
still be worth it, it's just that when some external entity decides
things work some way (because they design a SOC or write a bootrom, or
whatever) it'll be easier for them to give a shell script that
implements what's needed than to understand and adapt binman. S

Re: [SPAM] Re: Replace make-fit-atf.py with binman. Was: migrate u-boot-rockchip.bin to binman and generate an image for SPI

[Xavier Drudis Ferran]

El Mon, Aug 01, 2022 at 01:13:27PM -0600, Simon Glass deia:
> >
> > Am I completely lost or does what I want to do make some kind of sense?
> 
> Well I still feel that we should handle this properly in binman.
>

I respect your feelings but would you care explaining the goal of binman?
I first thought it was about handling binaries (things like copy binary content,
aligning it, padding, extracting and selecting parts from one binary to form
the final image...). A little like a linker but for other kinds of binary files,
with maybe headers but no symbol resolution and all. Maybe more like GNU ar but
for images instead of executables.
 
But then you seem to say it should handle dependencies between build files and 
be called only once ? So should it eventually become a sort of make itself ?
 
> I don't even think we need to allow images to be incorporated in other
> images. We can probably just allow a section to have a filename, a bit
> like this patch [1].
> 

There's a image r-sd and an image r-spi. Both need to have inside the
same binary itb, packed differently. If r-sd has a section with itb
and writes that section to a file, r-spi still needs to incorporate
that section as a binary, and to do that it needs to know the other
image has already written its section to a file or built into
somewhere in memory. So there're dependencies, and synchronisation if
you want to built images in parallel. make can handle that if you call
binman multiple times. But if you don't want that, then you need to
either include dependency management into binman (turning it into a
little make working from dts subtrees instead of makefiles) or forbid
that binaries used by binman could be images produced by binman, and
then you need other binary manipulation tools (like make_fit_atf.py,
which is basically what you have now, but that's apparently not
desired either).

If it was just me who feels lost, then it'd be just that I don't get it, 
but Quentin is saying something similar.

In any case, don't forget what Jerome said about tee.bin needing different
parsing than split-elf does in binman. The sections of tee.bin can also
end up in u-boot.itb. Adding that to binman would maybe make more sense
that what I was trying to add, in the sense that binman be a swiss army knive 
of parsing and building binaries. But then you'd still have the problem 
that if images built by binman cannot be incorporated in images built by 
binman, 
then something else has to build u-boot.itb. And that something else is 
currently make_fit_atf.py and works fine, so why the trouble ?

We're kind of running in circles, and for me it would be helpful to
understand the goal and scope of binman to understand what would be
desirable, because the status quo seems preferable to some kind of
feature creep in binman that I can't seem to reconcile with the 
philosophy of one tool doing one task and well. 

Maybe most people here already understand all this, but sometimes 
explaining the basics to a foreign bystander who knows little of
the subject (hello, that's me) can help people sort out thoughts?

Thanks, and sorry for the wall of text.

Re: Replace make-fit-atf.py with binman. Was: migrate u-boot-rockchip.bin to binman and generate an image for SPI

[Xavier Drudis Ferran]

El Tue, Jul 26, 2022 at 09:08:25PM +0200, Xavier Drudis Ferran deia:
> want u-boot.itb it runs binman -a of-list="rk3399-rock-pi-4b.dts 
> rockchip-itb-u-boot.dts"

Sorry. I mistook the parameter. That would only generate two config entries and 
2 fdts in the .itb image. 
We could use some includes, but then we'd need too many .dts files. 
Never mind. 
Maybe we could just stay with make_fit_atf.py

Re: [SPAM] Re: Replace make-fit-atf.py with binman. Was: migrate u-boot-rockchip.bin to binman and generate an image for SPI

[Xavier Drudis Ferran]

El Tue, Jul 26, 2022 at 09:15:10PM +0200, Jerome Forissier deia:
>  
> > I'm sending a patch below that adds a couple of configuration properties to 
> > binman so that split-elf can fill the properties. How many segments are 
> > in bl31.elf or optee is not something that we have in CONFIGs, I think, 
> > so it may be difficult to catch all cases with ifdefs.
> 
> Careful with OP-TEE: tee.elf must NOT be used, please see commit
> 348310233dac ("mach-rockchip: make_fit_atf.py: support OP-TEE tee.bin v1
> format").
>

Uh. Maybe it isn't worth to migrate from make_fit_atf.py to split-elf ?  

Re: Replace make-fit-atf.py with binman. Was: migrate u-boot-rockchip.bin to binman and generate an image for SPI

[Xavier Drudis Ferran]

&binman {
#ifndef CONFIG_USE_SPL_FIT_GENERATOR
itb: itb {
filename = "u-boot.itb";
fit {
filename = "u-boot.itb";
description = "U-Boot FIT";
fit,fdt-list = "of-list";
fit,external-offset=<0>;

images {
uboot {
description = "U-Boot (64-bit)";
type = "standalone";
os = "U-Boot";
arch = "arm64";
compression = "none";
load = ;
u-boot-nodtb {
};
};
#ifdef CONFIG_SPL_ATF
@atf_SEQ {
fit,operation = "split-elf";
description = "ARM Trusted Firmware";
type = "firmware";
arch = "arm64";
os = "arm-trusted-firmware";
compression = "none";
fit,load;
fit,entry;
fit,data;

atf-bl31 {
};
};
#endif
#ifdef CONFIG_TEE
@tee_SEQ {
fit,operation = "split-elf";
description = "TEE";
type = "tee";
arch = "arm64";
os = "tee";
compression = "none";
fit,load;
fit,entry;
fit,data;

tee-os {
};
};
#endif
@fdt_SEQ {
description = "NAME.dtb";
type = "flat_dt";
compression = "none";
};
};
configurations {
default = "@config_DEFAULT-SEQ";

@config_SEQ {
description = "NAME.dtb";
fdt = "fdt_SEQ";
fit,firmware = "atf_1";
fit,prepend-to-loadables = "uboot";
fit,loadables;
};
};
};
};
#endif
simple-bin {
filename = "u-boot-rockchip.bin";
pad-byte = <0xff>;

mkimage {
args = "-n", CONFIG_SYS_SOC, "-T", "rksd";
#ifndef CONFIG_TPL
u-boot-spl {
};
};
#else
u-boot-tpl {
};
};

u-boot-spl {
};
#endif

#ifdef CONFIG_ARM64
#ifdef CONFIG_USE_SPL_FIT_GENERATOR
blob {
filename = "u-boot.itb";
#else
collection {
content = <&itb>;
#endif
#else
u-boot-img {
#endif
offset = <((CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR - 
64) * 512)>;
};
};

#ifdef CONFIG_ROCKCHIP_SPI_IMAGE
simple-bin-spi {
filename = "u-boot-rockchip-spi.bin";
        pad-byte = <0xff>;

mkimage {
args = "-n", CONFIG_SYS_SOC, "-T", "rkspi";
#ifdef CONFIG_TPL
multiple-data-files;

u-boot-tpl {
};
#endif
u-boot-spl {
};
};

#ifdef CONFIG_ARM64
#ifdef CONFIG_USE_SPL_FIT_GENERATOR
blob {
   

Re: [SPAM] Re: [PATCH v3 3/4] spi-nor: Adapt soft reset to XTX25F32B in Rock Pi 4 rev 1.4

[Xavier Drudis Ferran]

El Tue, Jul 26, 2022 at 02:39:09PM +0530, Pratyush Yadav deia:
> 
> Yes, this is indeed a chicken and egg problem of sorts. The octal mode 
> soft reset I added is a hack that gets _some_ flashes working but not 
> all. I see 3 possible ways to solve this:
> 

But it was a hack that solved a problem. We still have hunger, war and
disease, but we have some more flashes working. That's some good,
isn't it?

>   1. Encode soft reset type in device tree. Read DT to issue the correct 
>  type of reset. But if a flash supports multiple stateful modes like 
>  4D-4D-4D and 8D-8D-8D, this might not work so well.

So must flash chips always be listed in dts ?
Can some boards have a socket and users plug different flashes to it ? 
Might it then not work so well also ?
My idea was try what you can with the controller you have because you 
know more what controller you have than what flash you have (until you
can ask your flash who it is and what it looks like). This is only active when 
a nondefault kconfig is enabled, anyway.

>   2. Do a hardware reset. This needs the RESET# line to be connected on 
>  the board and some way to toggle it. Does not work for boards that 
>  do not have this.

ACK.
Unfortunately not in my case[1]. 

>   3. Discover the mode the flash is in. The SFDP spec (JESD216D-01) does 
>  recommend a way to do this in section "4.5 Command Modes". This can 
>  let us find out which type of soft reset command the flash takes by 
>  reading SFDP. Of course, this needs the flash to implement the read 
>  SFDP command in all modes, which is not mandatory as per spec (at 
>  least as per xSPI spec (JESD251), which documents 8D-8D-8D 
>  flashes).
>

I think my XTX supports the soft reset according to the manual
(commands 66 and 99), but the SFDP info does not include info about
soft reset. I might misremember though.
 
> There is no one size fits all solution that I can see, so I think we 
> need to do all 3 to get full coverage of all cases. But start with the 
> one that fixes your use case and is easiest for you. I would guess that 
> is point 2.
>

My XT25F32B-S has no hardware reset pin. The manual[2] says so in a sentence, 
in page 49 (out of context) and the pinout is : 
   
   - Chip Select Input (CS#)
   - Data Output(SO)  or I/O 1 (IO1)
   - Write Protect Input (WP#), or I/O2 (IO2)
   - Ground (VSS)
   - Data Input (SI), or I/O 0 (IO0)
   - Serial Clock Input (SCLK)
   - Hold Input (HOLD#)
   - Power Supply (VCC)

And of course the board does not have a line for the nonexistent pin,
so no, the closest to hardware reset would be board poweroff.  SI, SO,
CS and SCLK are connected to a 40 pin connector though, so I can
ground them or pull them up or something. But no use.

> And all this does not even fix the problem of flashes that do not 
> support 1S-1S-1S mode at all. There are some flashes out there that 
> simply only support 8D-8D-8D mode. For those flashes, a reset would 
> still put them in 8D-8D-8D mode. To support those you need to discover 
> the flash mode and then initialize the flash in that mode itself, 
> without resetting.
> 

Well, maybe we can limit ourselves to solve solvable problems... That
should keep us busy for a while...

> I would recommending working with the Linux SPI NOR subsystem first. 
> That has more eyes looking at it so it would be easier to come up with a 
> good solution. Then you can backport it to U-Boot.
>

Thanks for the advice. Makes sense I guess. I'll see if I have the time and
motivation. 

[1] 
https://dl.radxa.com/rockpi4/docs/hw/rockpi4/rockpi4b_plus_v16_sch_20200628.pdf
[2] https://www.xtxtech.com/download/?AId=157

Re: [PATCH v3 3/4] spi-nor: Adapt soft reset to XTX25F32B in Rock Pi 4 rev 1.4

[Xavier Drudis Ferran]

Thank you for your time looking at the patch. 

El Tue, Jul 26, 2022 at 12:43:06PM +0530, Pratyush Yadav deia:
> 
> Please don't put the changelog in the commit message. Put it below the 3 
> dashed lines below.
>

Sorry. Will try to remember it next time. Not sure there's a next version of
this patch, though. 
 
> 
> This only tells if the _controller_ supports the op. There is no way to 
> find out if the flash supports the op without reading SFDP, which would 
> need you to either reset the flash or know which mode it is in.

Correct. But you can't always read the SFDP if you get the flash in 
some wrong state and try to reset it through a mode that the controller
doesn't support. 

>  So this patch is all wrong.

Then I won't send a 4th version unless someone else thinks it's worth
it and I have more time for it.
 
> The only way to find out what the flash supports is to read its ID or 
> SFDP.
> 

Chicken and egg ? The soft_reset is done before reading SFDP. 
It was done always in octal mode before my patch ("always" means
whenever CONFIG_SPI_FLASH_SOFT_RESET_ON_BOOT was enabled). My
patch tries to do it in a mode that the controller supports. 

In my experiments this helps being able to program /dev/mtd0 from
linux with flashrom. If that is working in mainstream U-Boot on a Rock
Pi 4B version 1.4 or later (with XTX flash) for the rest of people,
then I must be testing wrong, what do I know?

Thanks. Have a nice day.

Re: Replace make-fit-atf.py with binman. Was: migrate u-boot-rockchip.bin to binman and generate an image for SPI

[Xavier Drudis Ferran]

El Mon, Jul 25, 2022 at 07:29:53PM +0200, Xavier Drudis Ferran deia:
> 
> I copy here the rockchip-u-boot.dtsi file and then 2 patches on top of yours.
>

Sorry I copied a dirty version that din't work. The patches were correct, the 
dtsi wasn't. 
 
> #else
>   collection {
>   content = <&/binman/itb>;

It doesn't like this phandles nor just &itb. 

These patches were the real thing. Sorry for the noise.


> 
> >From 0f9cf7452a62268ec5978c80f46bf9323a269630 Mon Sep 17 00:00:00 2001
> From: Xavier Drudis Ferran 
> Date: Mon, 25 Jul 2022 17:35:27 +0200
> Subject: [PATCH] Align the fit images that binman produces to 8 bytes.
> 
> In commit 570c4636808 ("Makefile: Align fit-dtb.blob and u-boot.itb by
> 64bits") Michal Simek claims that according to the device tree spec,
> some things should be aligned to 4 bytes and some to 8, so passes -B 8
> to mkimage. Do the same when using binman so that we can try to
> replace make_fit_atf.py .
> 
> Should this be optional? I haven't found any uses of split-elf that I
> might be breaking, and Marek said it's from dt spec, so it should be
> always required. So I'm hard coding it until the need for being
> optional arises.
> ---
>  tools/binman/btool/mkimage.py | 4 +++-
>  tools/binman/etype/fit.py | 1 +
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/binman/btool/mkimage.py b/tools/binman/btool/mkimage.py
> index c85bfe053c..d614d72d62 100644
> --- a/tools/binman/btool/mkimage.py
> +++ b/tools/binman/btool/mkimage.py
> @@ -22,7 +22,7 @@ class Bintoolmkimage(bintool.Bintool):
>  
>  # pylint: disable=R0913
>  def run(self, reset_timestamp=False, output_fname=None, external=False,
> -pad=None, version=False):
> +pad=None, version=False, align=None):
>  """Run mkimage
>  
>  Args:
> @@ -36,6 +36,8 @@ class Bintoolmkimage(bintool.Bintool):
>  version: True to get the mkimage version
>  """
>  args = []
> +if align:
> +args += ['-B', f'{align:x}']
>  if external:
>  args.append('-E')
>  if pad:
> diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
> index 12306623af..7b99b83fa3 100644
> --- a/tools/binman/etype/fit.py
> +++ b/tools/binman/etype/fit.py
> @@ -420,6 +420,7 @@ class Entry_fit(Entry_section):
>  ext_offset = self._fit_props.get('fit,external-offset')
>  if ext_offset is not None:
>  args = {
> +'align': 8,
>  'external': True,
>  'pad': fdt_util.fdt32_to_cpu(ext_offset.value)
>  }
> -- 
> 2.20.1
> 
> >From 9fc65a2eb55f742dd805ed96e3d2028b20078a03 Mon Sep 17 00:00:00 2001
> From: Xavier Drudis Ferran 
> Date: Mon, 25 Jul 2022 18:01:24 +0200
> Subject: [PATCH] Replace make_fit_atf.py with binman.
> 
> This boots my Rock Pi 4B, but likely needs more generalisation to
> cover more boards and configurations.
> ---
>  Makefile   |  3 --
>  arch/arm/dts/rockchip-u-boot.dtsi  | 70 ++
>  configs/rock-pi-4-rk3399_defconfig |  1 +
>  3 files changed, 71 insertions(+), 3 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 279aeacee3..ad739ef357 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -997,9 +997,6 @@ endif
>  
>  ifeq ($(CONFIG_ARCH_ROCKCHIP)$(CONFIG_SPL),yy)
>  # Binman image dependencies
> -ifeq ($(CONFIG_ARM64),y)
> -INPUTS-y += u-boot.itb
> -endif
>  else
>  INPUTS-y += u-boot.img
>  endif
> diff --git a/arch/arm/dts/rockchip-u-boot.dtsi 
> b/arch/arm/dts/rockchip-u-boot.dtsi
> index 4c26caa92a..5a613650f5 100644
> --- a/arch/arm/dts/rockchip-u-boot.dtsi
> +++ b/arch/arm/dts/rockchip-u-boot.dtsi
> @@ -13,6 +13,75 @@
>  
>  #ifdef CONFIG_SPL
>  &binman {
> + itb {
> + filename = "u-boot.itb";
> + fit {
> + filename = "u-boot.itb";
> + description = "U-Boot FIT";
> + fit,fdt-list = "of-list";
> + fit,external-offset=<0>;
> +
> + images {
> + uboot {
> + description = "U-Boot (64-bit)";
> + type = "standalone";
> + os = "U-Boot";
> + arch 

Replace make-fit-atf.py with binman. Was: migrate u-boot-rockchip.bin to binman and generate an image for SPI

[Xavier Drudis Ferran]

SEQ";
firmware = "atf_1";
loadables = "uboot","atf_2","atf_3";
};
};
};
};
#endif
simple-bin {
filename = "u-boot-rockchip.bin";
pad-byte = <0xff>;

mkimage {
args = "-n", CONFIG_SYS_SOC, "-T", "rksd";
#ifndef CONFIG_TPL
u-boot-spl {
};
};
#else
u-boot-tpl {
};
};

u-boot-spl {
};
#endif

#ifdef CONFIG_ARM64
#ifdef CONFIG_USE_SPL_FIT_GENERATOR
blob {
filename = "u-boot.itb";
#else
collection {
content = <&/binman/itb>;
#endif
#else
u-boot-img {
#endif
offset = <((CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR - 
64) * 512)>;
};
};

#ifdef CONFIG_ROCKCHIP_SPI_IMAGE
simple-bin-spi {
filename = "u-boot-rockchip-spi.bin";
pad-byte = <0xff>;

mkimage {
args = "-n", CONFIG_SYS_SOC, "-T", "rkspi";
#ifdef CONFIG_TPL
multiple-data-files;

u-boot-tpl {
};
#endif
u-boot-spl {
    };
    };

#ifdef CONFIG_ARM64
#ifdef CONFIG_USE_SPL_FIT_GENERATOR
blob {
filename = "u-boot.itb";
#else
collection {
content = <&/binman/itb>;
#endif
#else
u-boot-img {
#endif
/* Sync with u-boot,spl-payload-offset if present */
offset = ;
};
};
#endif
};
#endif

>From 0f9cf7452a62268ec5978c80f46bf9323a269630 Mon Sep 17 00:00:00 2001
From: Xavier Drudis Ferran 
Date: Mon, 25 Jul 2022 17:35:27 +0200
Subject: [PATCH] Align the fit images that binman produces to 8 bytes.

In commit 570c4636808 ("Makefile: Align fit-dtb.blob and u-boot.itb by
64bits") Michal Simek claims that according to the device tree spec,
some things should be aligned to 4 bytes and some to 8, so passes -B 8
to mkimage. Do the same when using binman so that we can try to
replace make_fit_atf.py .

Should this be optional? I haven't found any uses of split-elf that I
might be breaking, and Marek said it's from dt spec, so it should be
always required. So I'm hard coding it until the need for being
optional arises.
---
 tools/binman/btool/mkimage.py | 4 +++-
 tools/binman/etype/fit.py | 1 +
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/binman/btool/mkimage.py b/tools/binman/btool/mkimage.py
index c85bfe053c..d614d72d62 100644
--- a/tools/binman/btool/mkimage.py
+++ b/tools/binman/btool/mkimage.py
@@ -22,7 +22,7 @@ class Bintoolmkimage(bintool.Bintool):
 
 # pylint: disable=R0913
 def run(self, reset_timestamp=False, output_fname=None, external=False,
-pad=None, version=False):
+pad=None, version=False, align=None):
 """Run mkimage
 
 Args:
@@ -36,6 +36,8 @@ class Bintoolmkimage(bintool.Bintool):
 version: True to get the mkimage version
 """
 args = []
+if align:
+args += ['-B', f'{align:x}']
 if external:
     args.append('-E')
 if pad:
diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
index 12306623af..7b99b83fa3 100644
--- a/tools/binman/etype/fit.py
+++ b/tools/binman/etype/fit.py
@@ -420,6 +420,7 @@ class Entry_fit(Entry_section):
 ext_offset = self._fit_props.get('fit,external-offset')
 if ext_offset is not None:
 args = {
+'align': 8,
 'external': True,
 'pad': fdt_util.fdt32_to_cpu(ext_offset.value)
 }
-- 
2.20.1

>From 9fc65a2eb55f742dd805ed96e3d2028b20078a03 Mon Sep 17 00:00:00 2001
From: Xavier Drudis Ferran 
Date: Mon, 25 Jul 2022 18:01:24 +0200
Subject: [PATCH] Replace make_fit_atf.py with binman.

This boots my Rock Pi 4B, but likely needs more generalisation to
cover more boards and configurations.
---
 Makefile   |  3 --
 arch/arm/dts/rockchip-u-boot.dtsi  | 70 ++
 configs/rock-pi-4-rk3399_defconfig |  1 +
 3 files changed, 71 insertions(+), 3 deletions(-)

diff --git a/Makefile b/Makefile
index 279aeacee3..ad739ef357 100644
--- a/Makefile
+++ b/Makefile
@@ -997,9 +997,6 @@

Re: [SPAM] [PATCH v2 0/7] migrate u-boot-rockchip.bin to binman and generate an image for SPI

[Xavier Drudis Ferran]

El Mon, Jul 25, 2022 at 12:54:04PM +0200, Quentin Schulz deia:
 
> You'd need a new binman entry I assume for calling mkenvimage.
>

 
> It's not a super safe assumption that CONFIG_ENV_OFFSET will be used for
> declaring where the environment is stored. E.g., CONFIG_ENV_OFFSET for Puma
> declares where the env is located on SPI-NOR, however for MMC devices, it is
> specified with u-boot,mmc-env-offset DT property (though, if it is not
> defined, it defaults to CONFIG_ENV_OFFSET, c.f. env/mmc.c). But maybe the
> same comment as I did for CONFIG_SYS_SPI_U_BOOT_OFFS would be enough? e.g.
> states that this should be in sync with the DT property if there's one for
> the board in question. See: 
> https://lore.kernel.org/u-boot/20220722160655.3904213-13-foss+ub...@0leil.net/
> for what I needed to do for Puma to get u-boot-rockchip-spi.bin support.
> 
> But that overall seems like a reasonable feature to add.
> 

But it is interesting enough ? 
I mean once you could call mkenvimage you'd still need to give it
some environment (variables values). And what would you give it
that would be different from the default environment ?
Maybe what's wanted is just to fail to read the environment
the first time. And the user can save it from hush (or run mkenvimage
themselves). 

> > Or maybe we should just disable CONFIG_ENV_IS_IN_MMC in the board ? (I
> > think it conflicts with trust settings anyway).
> > 
> 
> Yes, anything but ENV_IS_NOWHERE depends on !CHAIN_OF_TRUST. So I guess a
> check on #ifndef CONFIG_CHAIN_OF_TRUST for that section in binman would do
> the trick?
>

If we need to add the environment to binman we could just check ENV_IS_IN_...
because those will already be false if CHAIN_OF_TRUST is false, I think.
 
But mayeb we don't need to do that, or we just want to leave some 
empty space somewhere (yeah, difficult to know where if the offset can be in dt 
or Kconfig).

> 
> If there's no environment where U-Boot is looking for it, it'll not fatally
> fail right now. The next time you save the environment (saveenv), it'll be
> written in the correct location and read during next boot. Again, this patch
> series does not modify u-boot-rockchip.bin current behavior :)
>

You're right, so maybe we don't really need to run mkenvimage to build the 
image,
we leave the environment empty, and let the user do their thing. We may want 
to make sure we don't put something else there, though.
 
> > Now that we have Quentin's patches maybe we can remove the Makefile
> > warning about CONFIG_USE_SPL_FIT_GENERATOR and move the task that
> > arch/arm/mach-rockchip/make_fit_atf.py is doing to binman.
> 
> I wasn't aware of this entry, maybe it is actually possible. That'd be nice
> to have :)
> 

I'm playing with this right now. I don't have anything that boots.
First attempt complains that SPL can't allocate so much RAM to fit the
internal image (not really feeling like increasing space a lot for
many boards) and 2nd attempt at having an external image to make it closer
to what make_fit_atf.py used to do and have less breakage, then it doesn't even
complain, just stops short of running ATF.

I'll play some more...

> 
> My understanding is that there's a will to replace make_fit_atf.py output
> generated by binman instead, if binman supports everything needed to do this
> migration, I don't know.
>

Thanks for your confirmation.

Re: [PATCH v2 1/7] rockchip: generate idbloader.img content for u-boot-rockchip.bin with binman for ARM

[Xavier Drudis Ferran]

Note: I removed a few recipients from Cc: with a quite random criteria, 
just to avoid error messages from the list software that there were 
too many addresses in Cc:. I hope those will get it from the list anyway
and sorry if this is a problem.

 
> I'm sure I'm just missing out on something obvious but cannot find it right
> now.
> I have an issue with the assumption of what u-boot-rockchip.bin is supposed
> to be. What does it actually represent?
> 
> I have three possible boot media on my board: SPI-NOR, eMMC and SD Card.
> Both MMC devices can use the same offsets and images, so that's fine for me
> to have something like u-boot-rockchip-mmc.bin.
> However, SPI-NOR has a different offset for U-Boot proper than MMC devices.
> It would be ridiculous to have two defconfigs with the only difference being
> the value of SPL_PAD_TO option. Hence why there's a u-boot-rockchip-spi.bin
> image now and also why I argue in this patch series that using SPL_PAD_TO is
> incorrect. I replaced SPL_PAD_TO with the formula that was originally used
> to define the padding, see 
> https://source.denx.de/u-boot/u-boot/-/commit/79030a486128bdb6888059113711a6ae66ff89c5.
> I understand this change is not ok, but I cannot use SPL_PAD_TO either. I
> would like to have some opinion/answer on what I asked in the paragraph
> above.

The difference between u-boot-rockchip-mmc.bin and
u-boot-rockchip-spi.bin is not only the offset. The image for SPI has
the parts loaded by bootrom (tpl and spl) written in 8Kb chunks
separated by 8Kb empty space (or something like this, I don't
remember). That's why there are different -T rksd and -T rkspi
options to mkimage. This is so at least for RK3399.

I have no idea whether nand images need this special format or not, 
or whether it depends on the SoC model or what. If it's only the 
offset, then maybe we can avoid a 3rd image and reuse the MMC one. 
I don't know.