[PATCH RFC] hw/sh4/sh7750: Add STBCR/STBCR2 register support

[PATCH RFC] hw/sh4/sh7750: Add STBCR/STBCR2 register support

[Geert Uytterhoeven]

The new Linux SH7750 clock driver uses the registers for power-down
mode control, causing a crash:

    byte read to SH7750_STBCR_A7 (0x000000001fc00004) not supported
    Aborted (core dumped)

Fix this by adding support for the Standby Control Registers STBCR and
STBCR2.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
[RFC PATCH v3 12/35] drivers/clk/renesas: clk-sh7750.c SH7750/7751 CPG driver.
https://lore.kernel.org/all/a772e1b6de89af22057d3af31cc03dcad7964fc7.1697199949.git.ysato@users.sourceforge.jp

Accesses to CLKSTP00 and CLKSTCLK00 (0xfe0a0000/0x1e0a0000 and
0xfe0a0008/0x1e0a0008) don't seem to cause any issues, although I can't
see immediately where they are handled?

---
 hw/sh4/sh7750.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/hw/sh4/sh7750.c b/hw/sh4/sh7750.c
index ebe0fd96d94ca17b..deeb83b4540bbf2b 100644
--- a/hw/sh4/sh7750.c
+++ b/hw/sh4/sh7750.c
@@ -59,6 +59,9 @@ typedef struct SH7750State {
     uint16_t bcr3;
     uint32_t bcr4;
     uint16_t rfcr;
+    /* Power-Down Modes */
+    uint8_t stbcr;
+    uint8_t stbcr2;
     /* PCMCIA controller */
     uint16_t pcr;
     /* IO ports */
@@ -219,7 +222,13 @@ static void ignore_access(const char *kind, hwaddr addr)
 
 static uint32_t sh7750_mem_readb(void *opaque, hwaddr addr)
 {
+    SH7750State *s = opaque;
+
     switch (addr) {
+    case SH7750_STBCR_A7:
+        return s->stbcr;
+    case SH7750_STBCR2_A7:
+        return s->stbcr2;
     default:
         error_access("byte read", addr);
         abort();
@@ -318,14 +327,24 @@ static uint32_t sh7750_mem_readl(void *opaque, hwaddr addr)
 static void sh7750_mem_writeb(void *opaque, hwaddr addr,
                               uint32_t mem_value)
 {
+    SH7750State *s = opaque;
 
     if (is_in_sdrmx(addr, 2) || is_in_sdrmx(addr, 3)) {
         ignore_access("byte write", addr);
         return;
     }
 
-    error_access("byte write", addr);
-    abort();
+    switch (addr) {
+    case SH7750_STBCR_A7:
+        s->stbcr = mem_value;
+        return;
+    case SH7750_STBCR2_A7:
+        s->stbcr2 = mem_value;
+        return;
+    default:
+        error_access("byte write", addr);
+        abort();
+    }
 }
 
 static void sh7750_mem_writew(void *opaque, hwaddr addr,
-- 
2.34.1

Re: [PATCH RFC] hw/sh4/sh7750: Add STBCR/STBCR2 register support

[John Paul Adrian Glaubitz]

Hi Geert!

On Wed, 2023-10-18 at 14:40 +0200, Geert Uytterhoeven wrote:
> The new Linux SH7750 clock driver uses the registers for power-down
> mode control, causing a crash:
> 
>     byte read to SH7750_STBCR_A7 (0x000000001fc00004) not supported
>     Aborted (core dumped)
> 
> Fix this by adding support for the Standby Control Registers STBCR and
> STBCR2.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Is this supposed to be applied on top of Yoshinori's DT conversion series?

Thanks,
Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer
`. `'   Physicist
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: [PATCH RFC] hw/sh4/sh7750: Add STBCR/STBCR2 register support

[Geert Uytterhoeven]

Hi Adrian,

On Wed, Oct 18, 2023 at 2:46 PM John Paul Adrian Glaubitz
<glaubitz@physik.fu-berlin.de> wrote:
> On Wed, 2023-10-18 at 14:40 +0200, Geert Uytterhoeven wrote:
> > The new Linux SH7750 clock driver uses the registers for power-down
> > mode control, causing a crash:
> >
> >     byte read to SH7750_STBCR_A7 (0x000000001fc00004) not supported
> >     Aborted (core dumped)
> >
> > Fix this by adding support for the Standby Control Registers STBCR and
> > STBCR2.
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> Is this supposed to be applied on top of Yoshinori's DT conversion series?

No, it's a patch for QEMU.  Sorry for the confusion.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH RFC] hw/sh4/sh7750: Add STBCR/STBCR2 register support

[Yoshinori Sato]

On Wed, 18 Oct 2023 21:40:23 +0900,
Geert Uytterhoeven wrote:
> 
> The new Linux SH7750 clock driver uses the registers for power-down
> mode control, causing a crash:
> 
>     byte read to SH7750_STBCR_A7 (0x000000001fc00004) not supported
>     Aborted (core dumped)
> 
> Fix this by adding support for the Standby Control Registers STBCR and
> STBCR2.

FRQCR is also not returning the correct value, so it needs to be fixed.
Here are my changes.
https://gitlab.com/yoshinori.sato/qemu.git

It include.
- Minimal CPG support.
- DT support
- Add target LANDISK.

> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> [RFC PATCH v3 12/35] drivers/clk/renesas: clk-sh7750.c SH7750/7751 CPG driver.
> https://lore.kernel.org/all/a772e1b6de89af22057d3af31cc03dcad7964fc7.1697199949.git.ysato@users.sourceforge.jp
> 
> Accesses to CLKSTP00 and CLKSTCLK00 (0xfe0a0000/0x1e0a0000 and
> 0xfe0a0008/0x1e0a0008) don't seem to cause any issues, although I can't
> see immediately where they are handled?
> 
> ---
>  hw/sh4/sh7750.c | 23 +++++++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/sh4/sh7750.c b/hw/sh4/sh7750.c
> index ebe0fd96d94ca17b..deeb83b4540bbf2b 100644
> --- a/hw/sh4/sh7750.c
> +++ b/hw/sh4/sh7750.c
> @@ -59,6 +59,9 @@ typedef struct SH7750State {
>      uint16_t bcr3;
>      uint32_t bcr4;
>      uint16_t rfcr;
> +    /* Power-Down Modes */
> +    uint8_t stbcr;
> +    uint8_t stbcr2;
>      /* PCMCIA controller */
>      uint16_t pcr;
>      /* IO ports */
> @@ -219,7 +222,13 @@ static void ignore_access(const char *kind, hwaddr addr)
>  
>  static uint32_t sh7750_mem_readb(void *opaque, hwaddr addr)
>  {
> +    SH7750State *s = opaque;
> +
>      switch (addr) {
> +    case SH7750_STBCR_A7:
> +        return s->stbcr;
> +    case SH7750_STBCR2_A7:
> +        return s->stbcr2;
>      default:
>          error_access("byte read", addr);
>          abort();
> @@ -318,14 +327,24 @@ static uint32_t sh7750_mem_readl(void *opaque, hwaddr addr)
>  static void sh7750_mem_writeb(void *opaque, hwaddr addr,
>                                uint32_t mem_value)
>  {
> +    SH7750State *s = opaque;
>  
>      if (is_in_sdrmx(addr, 2) || is_in_sdrmx(addr, 3)) {
>          ignore_access("byte write", addr);
>          return;
>      }
>  
> -    error_access("byte write", addr);
> -    abort();
> +    switch (addr) {
> +    case SH7750_STBCR_A7:
> +        s->stbcr = mem_value;
> +        return;
> +    case SH7750_STBCR2_A7:
> +        s->stbcr2 = mem_value;
> +        return;
> +    default:
> +        error_access("byte write", addr);
> +        abort();
> +    }
>  }
>  
>  static void sh7750_mem_writew(void *opaque, hwaddr addr,
> -- 
> 2.34.1
> 

-- 
Yosinori Sato

Re: [PATCH RFC] hw/sh4/sh7750: Add STBCR/STBCR2 register support

[Geert Uytterhoeven]

Hi Sato-san,

On Thu, Oct 19, 2023 at 4:03 AM Yoshinori Sato
<ysato@users.sourceforge.jp> wrote:
> On Wed, 18 Oct 2023 21:40:23 +0900,
> Geert Uytterhoeven wrote:
> > The new Linux SH7750 clock driver uses the registers for power-down
> > mode control, causing a crash:
> >
> >     byte read to SH7750_STBCR_A7 (0x000000001fc00004) not supported
> >     Aborted (core dumped)
> >
> > Fix this by adding support for the Standby Control Registers STBCR and
> > STBCR2.
>
> FRQCR is also not returning the correct value, so it needs to be fixed.

I knew there would be more, hence the RFC ;-)

> Here are my changes.
> https://gitlab.com/yoshinori.sato/qemu.git
>
> It include.
> - Minimal CPG support.
> - DT support
> - Add target LANDISK.

Thank you very much!
It would be a good idea to mention this is the cover letter of your
Linux patch series, so your test audience doesn't have to fix
already-solved problems...

BTW, your commit da64d6541226a516 ("hw/sh4: sh7750.c allow access
STBCR and STBCR2.") just ignores writes, and always returns zero
when reading.  This may cause issues with Linux code relying on
clock_ops.is_enabled() to return correct data.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds