mbox series

[DO,NOT,MERGE,v5,00/37] Device Tree support for SH7751 based board

Message ID cover.1701768028.git.ysato@users.sourceforge.jp
Headers show
Series Device Tree support for SH7751 based board | expand

Message

Yoshinori Sato Dec. 5, 2023, 9:45 a.m. UTC
This is an updated version of something I wrote about 7 years ago.
Minimum support for R2D-plus and LANDISK.
I think R2D-1 will work if you add AX88796 to dts.
And board-specific functions and SCI's SPI functions are not supported.

You can get it working with qemu found here.
https://gitlab.com/yoshinori.sato/qemu/-/tree/landisk


v5 changes.
- pci-sh7751: revert header changes. and some fix in previuous driver.
- sh/kernel/iomap.c: Use SH io functions.
- sm501 and sm501fb: re-write DT support.

v4 changes.
- cpg-sh7750: use clk-divider and clk-gate.
- pci-sh7751: unified header files to old PCI driver.
- irq-renesas-sh7751: IPR registers direct mapping.
- irq-renesas-sh7751irl: useful register bit mapping.
- sm501 and sm501fb: re-write dt parser.
- j2_minus: fix build error.
- dt-binding schema: fix some errors.
- *.dts: cleanup.

v3 changes.
- Rewrite clk drivers.
- Added sh_tmu to OF support.
- Cleanup PCI stuff.
- Update sm501 and sm501fb OF support.
- Update devicetree and documents.

v2 changes.
- Rebasing v6,6-rc1
- re-write irqchip driver.
- Add binding documents.
- Cleanup review comment.

Yoshinori Sato (37):
  sh: passing FDT address to kernel startup.
  sh: Kconfig unified OF supported targets.
  sh: Enable OF support for build and configuration.
  dt-bindings: interrupt-controller: Add header for Renesas SH3/4 INTC.
  sh: GENERIC_IRQ_CHIP support for CONFIG_OF=y
  sh: kernel/setup Update DT support.
  sh: Fix COMMON_CLK support in CONFIG_OF=y.
  clocksource: sh_tmu: CLOCKSOURCE support.
  dt-bindings: timer: renesas,tmu: add renesas,tmu-sh7750
  sh: Common PCI Framework driver support.
  pci: pci-sh7751: Add SH7751 PCI driver
  dt-bindings: pci: pci-sh7751: Add SH7751 PCI
  dt-bindings: clock: sh7750-cpg: Add renesas,sh7750-cpg header.
  clk: Compatible with narrow registers
  clk: renesas: Add SH7750/7751 CPG Driver
  irqchip: Add SH7751 INTC driver
  dt-bindings: interrupt-controller: renesas,sh7751-intc: Add
    json-schema
  irqchip: SH7751 IRL external encoder with enable gate.
  dt-bindings: interrupt-controller: renesas,sh7751-irl-ext: Add
    json-schema
  serial: sh-sci: fix SH4 OF support.
  dt-bindings: serial: renesas,scif: Add scif-sh7751.
  dt-bindings: display: smi,sm501: SMI SM501 binding json-schema
  mfd: sm501: Convert platform_data to OF property
  dt-binding: sh: cpus: Add SH CPUs json-schema
  dt-bindings: vendor-prefixes: Add iodata
  dt-bindings: vendor-prefixes:  Add smi
  dt-bindings: ata: ata-generic: Add new targets
  dt-bindings: soc: renesas: sh: Add SH7751 based target
  sh: SH7751R SoC Internal peripheral definition dtsi.
  sh: add RTS7751R2D Plus DTS
  sh: Add IO DATA LANDISK dts
  sh: Add IO DATA USL-5P dts
  sh: j2_mimas_v2.dts update
  sh: Add dtbs target support.
  sh: RTS7751R2D Plus OF defconfig
  sh: LANDISK OF defconfig
  sh: j2_defconfig: update

 .../devicetree/bindings/ata/ata-generic.yaml  |   2 +
 .../bindings/clock/renesas,sh7750-cpg.yaml    | 103 ++++
 .../bindings/display/smi,sm501.yaml           | 134 +++++
 .../renesas,sh7751-intc.yaml                  | 105 ++++
 .../renesas,sh7751-irl-ext.yaml               |  83 +++
 .../bindings/pci/renesas,sh7751-pci.yaml      | 128 +++++
 .../bindings/serial/renesas,scif.yaml         |   1 +
 .../devicetree/bindings/sh/cpus.yaml          |  73 +++
 .../devicetree/bindings/soc/renesas/sh.yaml   |  32 ++
 .../bindings/timer/renesas,tmu.yaml           |  11 +-
 .../devicetree/bindings/vendor-prefixes.yaml  |   4 +
 arch/sh/Kconfig                               |  11 +-
 arch/sh/boards/Kconfig                        |  24 +-
 arch/sh/boards/of-generic.c                   |  28 +-
 arch/sh/boot/compressed/head_32.S             |   5 +-
 arch/sh/boot/dts/Makefile                     |   5 +
 arch/sh/boot/dts/j2_mimas_v2.dts              |   2 +-
 arch/sh/boot/dts/landisk.dts                  |  74 +++
 arch/sh/boot/dts/rts7751r2dplus.dts           | 157 ++++++
 arch/sh/boot/dts/sh7751r.dtsi                 | 150 ++++++
 arch/sh/boot/dts/usl-5p.dts                   |  84 +++
 arch/sh/configs/j2_defconfig                  |  11 +-
 arch/sh/configs/landisk-of_defconfig          | 111 ++++
 arch/sh/configs/rts7751r2dplus-of_defconfig   |  93 ++++
 arch/sh/drivers/Makefile                      |   2 +
 arch/sh/drivers/pci/pci.c                     |   6 -
 arch/sh/include/asm/io.h                      |   6 +
 arch/sh/include/asm/irq.h                     |  10 +-
 arch/sh/include/asm/pci.h                     |   4 +
 arch/sh/kernel/cpu/Makefile                   |   6 +-
 arch/sh/kernel/cpu/irq/imask.c                |  17 +
 arch/sh/kernel/cpu/sh4/Makefile               |   3 +
 arch/sh/kernel/iomap.c                        |  24 +
 arch/sh/kernel/setup.c                        |  51 +-
 arch/sh/kernel/time.c                         |  12 +
 drivers/clk/clk-divider.c                     |  56 +-
 drivers/clk/clk-gate.c                        |  56 +-
 drivers/clk/renesas/Kconfig                   |  16 +-
 drivers/clk/renesas/Makefile                  |   1 +
 drivers/clk/renesas/clk-sh7750.c              | 498 ++++++++++++++++++
 drivers/clocksource/sh_tmu.c                  | 161 ++++--
 drivers/irqchip/Kconfig                       |  15 +
 drivers/irqchip/Makefile                      |   3 +
 drivers/irqchip/irq-renesas-sh7751.c          | 290 ++++++++++
 drivers/irqchip/irq-renesas-sh7751irl.c       | 227 ++++++++
 drivers/mfd/sm501.c                           |  99 ++++
 drivers/pci/controller/Kconfig                |   9 +
 drivers/pci/controller/Makefile               |   1 +
 drivers/pci/controller/pci-sh7751.c           | 302 +++++++++++
 drivers/pci/controller/pci-sh7751.h           |  76 +++
 drivers/tty/serial/Kconfig                    |   2 +-
 drivers/tty/serial/sh-sci.c                   |   6 +-
 drivers/video/fbdev/sm501fb.c                 |  82 +++
 include/dt-bindings/clock/sh7750-cpg.h        |  26 +
 include/dt-bindings/display/sm501.h           |  25 +
 .../interrupt-controller/sh7751-intc.h        |  21 +
 include/linux/clk-provider.h                  |  22 +-
 include/linux/sh_intc.h                       |   7 +-
 58 files changed, 3396 insertions(+), 177 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/clock/renesas,sh7750-cpg.yaml
 create mode 100644 Documentation/devicetree/bindings/display/smi,sm501.yaml
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/renesas,sh7751-intc.yaml
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/renesas,sh7751-irl-ext.yaml
 create mode 100644 Documentation/devicetree/bindings/pci/renesas,sh7751-pci.yaml
 create mode 100644 Documentation/devicetree/bindings/sh/cpus.yaml
 create mode 100644 Documentation/devicetree/bindings/soc/renesas/sh.yaml
 create mode 100644 arch/sh/boot/dts/landisk.dts
 create mode 100644 arch/sh/boot/dts/rts7751r2dplus.dts
 create mode 100644 arch/sh/boot/dts/sh7751r.dtsi
 create mode 100644 arch/sh/boot/dts/usl-5p.dts
 create mode 100644 arch/sh/configs/landisk-of_defconfig
 create mode 100644 arch/sh/configs/rts7751r2dplus-of_defconfig
 create mode 100644 drivers/clk/renesas/clk-sh7750.c
 create mode 100644 drivers/irqchip/irq-renesas-sh7751.c
 create mode 100644 drivers/irqchip/irq-renesas-sh7751irl.c
 create mode 100644 drivers/pci/controller/pci-sh7751.c
 create mode 100644 drivers/pci/controller/pci-sh7751.h
 create mode 100644 include/dt-bindings/clock/sh7750-cpg.h
 create mode 100644 include/dt-bindings/display/sm501.h
 create mode 100644 include/dt-bindings/interrupt-controller/sh7751-intc.h

Comments

Uwe Kleine-König Dec. 5, 2023, 10:10 a.m. UTC | #1
Hello,

On Tue, Dec 05, 2023 at 06:45:33PM +0900, Yoshinori Sato wrote:
> @@ -675,13 +681,17 @@ struct clk_div_table {
>   * CLK_DIVIDER_BIG_ENDIAN - By default little endian register accesses are used
>   *	for the divider register.  Setting this flag makes the register accesses
>   *	big endian.
> + * CLK_DIVIDER_REG_8BIT - by default 32bit register accesses are used for
> + *	the gate register.  Setting this flag makes the register accesses 8bit.
> + * CLK_DIVIDER_REG_16BIT - by default 32bit register accesses are used for
> + *	the gate register.  Setting this flag makes the register accesses 16bit.
>   */
>  struct clk_divider {
>  	struct clk_hw	hw;
>  	void __iomem	*reg;
>  	u8		shift;
>  	u8		width;
> -	u8		flags;
> +	u32		flags;
>  	const struct clk_div_table	*table;
>  	spinlock_t	*lock;
>  };

I wonder why .flags was made bigger here. The two new flag values would
still fit into the u8, right?

Best regards
Uwe
Arnd Bergmann Dec. 5, 2023, 1:01 p.m. UTC | #2
On Tue, Dec 5, 2023, at 10:45, Yoshinori Sato wrote:
> Signed-off-by: Yoshinori Sato <ysato@users.sourceforge.jp>
> ---
>  arch/sh/configs/rts7751r2dplus-of_defconfig | 93 +++++++++++++++++++++

This is very similar to the landisk config, so it may be
easier to just have one of them that works for both, as well
as future ones.

> +CONFIG_LOG_BUF_SHIFT=14
> +CONFIG_NAMESPACES=y
> +CONFIG_EXPERT=y

You should not normally need to enable CONFIG_EXPERT in a
defconfig. Is there any particular reason for this?

    Arnd
Arnd Bergmann Dec. 5, 2023, 1:05 p.m. UTC | #3
On Tue, Dec 5, 2023, at 10:45, Yoshinori Sato wrote:
> +
> +#if defined(CONFIG_PCI) && !defined(CONFIG_GENERIC_IOMAP)
> +void pci_iounmap(struct pci_dev *dev, void __iomem *addr)
> +{
> +	iounmap(addr);
> +}
> +EXPORT_SYMBOL(pci_iounmap);

This definition does not work for addresses that are
returned by ioport_map(), include pci_iomap() on
IORESOURCE_IO.  However, the definition in lib/pci_iomap.c
should work fine, you just need to #define ARCH_WANTS_GENERIC_PCI_IOUNMAP
to get that.

      Arnd
Arnd Bergmann Dec. 5, 2023, 1:14 p.m. UTC | #4
On Tue, Dec 5, 2023, at 10:45, Yoshinori Sato wrote:
> Fix extrnal fdt initialize and bootargs.
>
> Signed-off-by: Yoshinori Sato <ysato@users.sourceforge.jp>
> ---
>  arch/sh/kernel/setup.c | 51 ++++++++++++++++++++++++++++--------------
>  1 file changed, 34 insertions(+), 17 deletions(-)
>
> diff --git a/arch/sh/kernel/setup.c b/arch/sh/kernel/setup.c
> index 3d80515298d2..b299abff68e0 100644
> --- a/arch/sh/kernel/setup.c
> +++ b/arch/sh/kernel/setup.c
> @@ -30,6 +30,7 @@
>  #include <linux/memblock.h>
>  #include <linux/of.h>
>  #include <linux/of_fdt.h>
> +#include <linux/libfdt.h>
>  #include <linux/uaccess.h>
>  #include <uapi/linux/mount.h>
>  #include <asm/io.h>
> @@ -74,7 +75,13 @@ extern int root_mountflags;
>  #define RAMDISK_PROMPT_FLAG		0x8000
>  #define RAMDISK_LOAD_FLAG		0x4000
> 
> +#if defined(CONFIG_OF) && !defined(CONFIG_USE_BUILTIN_DTB)
> +#define CHOSEN_BOOTARGS
> +#endif
> +
> +#ifndef CHOSEN_BOOTARGS
>  static char __initdata command_line[COMMAND_LINE_SIZE] = { 0, };
> +#endif

I think an appended DTB is generally better than a built-in
one, as that allows you to still have a single kernel
image across machines and just pick the dtb when installing it.

With everything else being equal, I would suggest not
actually making this an option for new platforms.

    Arnd
Arnd Bergmann Dec. 5, 2023, 1:26 p.m. UTC | #5
On Tue, Dec 5, 2023, at 10:45, Yoshinori Sato wrote:

> +#include <asm/addrspace.h>
> +#include "pci-sh7751.h"
> +
> +#define pcic_writel(val, base, reg) __raw_writel(val, base + (reg))
> +#define pcic_readl(base, reg) __raw_readl(base + (reg))

__raw_writel()/__raw_readl() has a number of problems with
atomicity (the compiler may split or merge pointer
dereferences), barriers and endianess. You should normally
always use readl()/writel() instead.

> +	memset(pci_config, 0, sizeof(pci_config));
> +	if (of_property_read_u32_array(np, "renesas,config",
> +				       pci_config, SH7751_NUM_CONFIG) == 0) {
> +		for (i = 0; i < SH7751_NUM_CONFIG; i++) {
> +			r = pci_config[i * 2];
> +			/* CONFIG0 is read-only, so make it a sentinel. */
> +			if (r == 0)
> +				break;
> +			pcic_writel(pci_config[i * 2 + 1], pcic, r * 4);
> +		}
> +	}

the config property seems a little too specific to this
implementation of the driver. Instead of encoding register
values in DT, I think these should either be described
in named properties where needed, or hardcoded in the driver
if there is only one sensible value.

> +/*
> + * We need to avoid collisions with `mirrored' VGA ports
> + * and other strange ISA hardware, so we always want the
> + * addresses to be allocated in the 0x000-0x0ff region
> + * modulo 0x400.
> + */
> +#define IO_REGION_BASE 0x1000
> +resource_size_t pcibios_align_resource(void *data, const struct 
> resource *res,
> +				resource_size_t size, resource_size_t align)
> +{

You can't have these generic functions in a driver, as that
prevents you from building more than one such driver.

The logic you have here is neither architecture nor
driver specific.

> +static int sh7751_pci_probe(struct platform_device *pdev)
> +{
> +	struct resource *res, *bscres;
> +	void __iomem *pcic;
> +	void __iomem *bsc;
> +	u32 memory[2];
> +	u16 vid, did;
> +	u32 word;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (IS_ERR(res))
> +		return PTR_ERR(res);
> +	pcic = (void __iomem *)res->start;

This cast is invalid, as res->start is a physical address
that you would need to ioremap() to turn into an __iomem
pointer.

> +	bscres = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +	bsc = devm_ioremap_resource(&pdev->dev, bscres);
> +	if (IS_ERR(bsc))
> +		return PTR_ERR(bsc);
> +
> +	if (of_property_read_u32_array(pdev->dev.of_node,
> +				       "renesas,memory", memory, 2) < 0) {
> +		/*
> +		 * If no memory range is specified,
> +		 *  the entire main memory will be targeted for DMA.
> +		 */
> +		memory[0] = memory_start;
> +		memory[1] = memory_end - memory_start;
> +	}

There is a generic "dma-ranges" proerty for describing
which memory is visible by a bus.

> diff --git a/drivers/pci/controller/pci-sh7751.h 
> b/drivers/pci/controller/pci-sh7751.h
> new file mode 100644
> index 000000000000..540cee7095c6
> --- /dev/null
> +++ b/drivers/pci/controller/pci-sh7751.h
> @@ -0,0 +1,76 @@

If the header is only included from one file, just removed
it and add the register definitions to the driver directly.

     Arnd
Geert Uytterhoeven Dec. 5, 2023, 1:34 p.m. UTC | #6
Hi Arnd,

On Tue, Dec 5, 2023 at 2:26 PM Arnd Bergmann <arnd@arndb.de> wrote:
> On Tue, Dec 5, 2023, at 10:45, Yoshinori Sato wrote:
> > +     bscres = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > +     bsc = devm_ioremap_resource(&pdev->dev, bscres);
> > +     if (IS_ERR(bsc))
> > +             return PTR_ERR(bsc);
> > +
> > +     if (of_property_read_u32_array(pdev->dev.of_node,
> > +                                    "renesas,memory", memory, 2) < 0) {
> > +             /*
> > +              * If no memory range is specified,
> > +              *  the entire main memory will be targeted for DMA.
> > +              */
> > +             memory[0] = memory_start;
> > +             memory[1] = memory_end - memory_start;
> > +     }
>
> There is a generic "dma-ranges" proerty for describing
> which memory is visible by a bus.

I was just going to give that comment on the bindings patch ;-)

> > --- /dev/null
> > +++ b/drivers/pci/controller/pci-sh7751.h
> > @@ -0,0 +1,76 @@
>
> If the header is only included from one file, just removed
> it and add the register definitions to the driver directly.

$ git grep pci-sh7751.h
arch/sh/drivers/pci/pci-sh4.h:#include "pci-sh7751.h"
drivers/pci/controller/pci-sh7751.c:#include "pci-sh7751.h"

Gr{oetje,eeting}s,

                        Geert
Geert Uytterhoeven Dec. 5, 2023, 3:27 p.m. UTC | #7
Hi Sato-san,

Thanks for your patch!

On Tue, Dec 5, 2023 at 10:46 AM Yoshinori Sato
<ysato@users.sourceforge.jp> wrote:
> - fix earlycon name.
> - fix earlyprintk hung (NULL pointer reference).

- fix SERIAL_SH_SCI_EARLYCON enablement

> Signed-off-by: Yoshinori Sato <ysato@users.sourceforge.jp>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

> --- a/drivers/tty/serial/Kconfig
> +++ b/drivers/tty/serial/Kconfig
> @@ -658,7 +658,7 @@ config SERIAL_SH_SCI_EARLYCON
>         depends on SERIAL_SH_SCI=y
>         select SERIAL_CORE_CONSOLE
>         select SERIAL_EARLYCON
> -       default ARCH_RENESAS
> +       default ARCH_RENESAS || SUPERH
>
>  config SERIAL_SH_SCI_DMA
>         bool "DMA support" if EXPERT

Gr{oetje,eeting}s,

                        Geert
Linus Walleij Dec. 5, 2023, 9:48 p.m. UTC | #8
On Tue, Dec 5, 2023 at 2:26 PM Arnd Bergmann <arnd@arndb.de> wrote:
> On Tue, Dec 5, 2023, at 10:45, Yoshinori Sato wrote:

> > +     if (of_property_read_u32_array(pdev->dev.of_node,
> > +                                    "renesas,memory", memory, 2) < 0) {
> > +             /*
> > +              * If no memory range is specified,
> > +              *  the entire main memory will be targeted for DMA.
> > +              */
> > +             memory[0] = memory_start;
> > +             memory[1] = memory_end - memory_start;
> > +     }
>
> There is a generic "dma-ranges" proerty for describing
> which memory is visible by a bus.

It's really a headache to use, so I put a bit of documentation here:
https://elinux.org/Device_Tree_Usage#PCI_DMA_Address_Translation

Yoshinoro, you can look at these bindings and drivers that use
dma-ranges for help:
Documentation/devicetree/bindings/pci/intel,ixp4xx-pci.yaml
drivers/pci/controller/pci-ixp4xx.c
Documentation/devicetree/bindings/pci/faraday,ftpci100.yaml
drivers/pci/controller/pci-ftpci100.c

Yours,
Linus Walleij
Lee Jones Dec. 7, 2023, 4:36 p.m. UTC | #9
On Tue, 05 Dec 2023, Yoshinori Sato wrote:

> Various parameters of SM501 can be set using platform_data,
> so parameters cannot be passed in the DeviceTree target.
> Expands the parameters set in platform_data so that they can be
> specified using DeviceTree properties.
> 
> Signed-off-by: Yoshinori Sato <ysato@users.sourceforge.jp>
> ---
>  drivers/mfd/sm501.c           | 99 +++++++++++++++++++++++++++++++++++
>  drivers/video/fbdev/sm501fb.c | 82 +++++++++++++++++++++++++++++
>  2 files changed, 181 insertions(+)
> 
> diff --git a/drivers/mfd/sm501.c b/drivers/mfd/sm501.c
> index 28027982cf69..f0104fdf0f34 100644
> --- a/drivers/mfd/sm501.c
> +++ b/drivers/mfd/sm501.c
> @@ -1370,6 +1370,99 @@ static int sm501_init_dev(struct sm501_devdata *sm)
>  	return 0;
>  }
>  
> +static void sm501_of_read_reg_init(struct device_node *np,
> +				   const char *propname, struct sm501_reg_init *val)
> +{
> +	u32 u32_val[2];

Encoding the size of the variable in the variable name is odd.

> +
> +	if (!of_property_read_u32_array(np, propname, u32_val, sizeof(u32_val))) {
> +		val->set = u32_val[0];
> +		val->mask = u32_val[1];

Masks and register values stored as DT properties?  This is generally
not permitted.  Please seek counsel from the DT maintainers.

> +	}
> +}
> +
> +/* Read GPIO I2C configuration */
> +static int sm501_parse_dt_gpio_i2c(struct device *dev, struct sm501_platdata *plat,
> +				   struct device_node *np)
> +{
> +	struct sm501_platdata_gpio_i2c *gpio_i2c_p;
> +	struct property *prop;
> +	u32 gpio_i2c[5];

Why 5?  Please define all magic numbers.

> +	const __be32 *p;
> +	unsigned int i;
> +	u32 i2c_nr;
> +
> +	prop = of_find_property(np, "smi,gpio-i2c", NULL);
> +	if (!prop)
> +		return 0;
> +
> +	i2c_nr = of_property_count_u32_elems(np, "smi,gpio-i2c");

Why do you need both of these probing functions?  What does
of_property_count_u32_elems() return if smi,gpio-i2c doesn't exist?

> +	/* GPIO I2C define 5 words per channel. */
> +	if (i2c_nr % 5)
> +		return -EINVAL;
> +	i2c_nr /= 5;

'\n'

> +	plat->gpio_i2c = devm_kzalloc(dev, sizeof(*plat->gpio_i2c) * i2c_nr,
> +				      GFP_KERNEL);
> +	if (!plat->gpio_i2c)
> +		return -ENOMEM;
> +
> +	plat->gpio_i2c_nr = i2c_nr;
> +	gpio_i2c_p = plat->gpio_i2c;

What's the purpose of this intermediary variable?

> +
> +	for (; i2c_nr > 0; i2c_nr--) {

You can define 'p' in here, right?

> +		for (i = 0; i < ARRAY_SIZE(gpio_i2c); i++) {
> +			p = of_prop_next_u32(prop, p, &gpio_i2c[i]);
> +			if (!p)
> +				return -EINVAL;
> +		}
> +		gpio_i2c_p->bus_num = gpio_i2c[0];
> +		gpio_i2c_p->pin_sda = gpio_i2c[1];
> +		gpio_i2c_p->pin_scl = gpio_i2c[2];
> +		gpio_i2c_p->udelay  = gpio_i2c[3];
> +		gpio_i2c_p->timeout = gpio_i2c[4];

I'm not even going to ask.  I'll leave this to the DT maintainers.

> +		gpio_i2c_p++;
> +	}
> +	return 0;
> +}
> +
> +/* Build platform_data from OF property */
> +static int sm501_parse_dt(struct sm501_devdata *sm, struct device_node *np)
> +{
> +	struct sm501_platdata *plat;
> +	u32 u32_val;
> +	int ret;
> +
> +	plat = devm_kzalloc(sm->dev, sizeof(*plat), GFP_KERNEL);
> +	if (!plat)
> +		return -ENOMEM;
> +
> +	plat->init = devm_kzalloc(sm->dev, sizeof(*plat->init), GFP_KERNEL);

Why not grab all of the memory at once?

Maybe make this 'init' thing a non-pointer.

> +	if (!plat->init)
> +		return -ENOMEM;
> +
> +	if (!of_property_read_u32(np, "smi,devices", &u32_val))
> +		plat->init->devices = u32_val;

What happens if you do:

	of_property_read_u32(np, "smi,devices", &plat->init->devices);

> +	if (!of_property_read_u32(np, "smi,mclk", &u32_val))
> +		plat->init->mclk = u32_val;
> +	if (!of_property_read_u32(np, "smi,m1xclk", &u32_val))
> +		plat->init->m1xclk = u32_val;
> +
> +	sm501_of_read_reg_init(np, "smi,misc-timing", &plat->init->misc_timing);
> +	sm501_of_read_reg_init(np, "smi,misc-control", &plat->init->misc_control);
> +	sm501_of_read_reg_init(np, "smi,gpio-low", &plat->init->gpio_low);
> +	sm501_of_read_reg_init(np, "smi,gpio-high", &plat->init->gpio_high);
> +
> +	if (IS_ENABLED(CONFIG_MFD_SM501_GPIO) &&
> +	    (plat->init->devices & SM501_USE_GPIO)) {

That's over-bracketed, right?

plat->init->devices is a bit mask of enable devices stored in DT?

Okay, I think this is going to need a lot of work on the DT side.

Leaving the review here for now.

> +		ret = sm501_parse_dt_gpio_i2c(sm->dev, plat, np);
> +		if (ret)
> +			return ret;
> +	}
> +	sm->platdata = plat;
> +	return 0;
> +}
> +
>  static int sm501_plat_probe(struct platform_device *dev)
>  {
>  	struct sm501_devdata *sm;
> @@ -1406,6 +1499,12 @@ static int sm501_plat_probe(struct platform_device *dev)
>  		goto err_res;
>  	}
>  
> +	if (IS_ENABLED(CONFIG_OF) && dev->dev.of_node) {
> +		ret = sm501_parse_dt(sm, dev->dev.of_node);
> +		if (ret)
> +			goto err_res;
> +	}
> +
>  	platform_set_drvdata(dev, sm);
>  
>  	sm->regs = ioremap(sm->io_res->start, resource_size(sm->io_res));
> diff --git a/drivers/video/fbdev/sm501fb.c b/drivers/video/fbdev/sm501fb.c
> index d6fdc1737cd2..d35285819d28 100644
> --- a/drivers/video/fbdev/sm501fb.c
> +++ b/drivers/video/fbdev/sm501fb.c
> @@ -1932,6 +1932,82 @@ static int sm501fb_start_one(struct sm501fb_info *info,
>  	return 0;
>  }
>  
> +#if defined(CONFIG_OF)
> +/* parse CRT / panel configuration */
> +static struct sm501_platdata_fbsub *dt_fbsub(struct device *dev,
> +					     struct device_node *np,
> +					     const char *name)
> +{
> +	struct sm501_platdata_fbsub *fbsub = NULL;
> +	struct fb_videomode *def_mode = NULL;
> +	struct device_node *child;
> +	const void *p_edid;
> +	u32 flags = 0;
> +	u32 bpp = 0;
> +	int len;
> +
> +	child = of_get_child_by_name(np, name);
> +	if (child == NULL)
> +		return NULL;
> +
> +	p_edid = of_get_property(child, "edid", &len);
> +	if (p_edid && len == EDID_LENGTH) {
> +		struct fb_monspecs *specs;
> +		u8 *edid;
> +
> +		edid = kmemdup(p_edid, EDID_LENGTH, GFP_KERNEL);
> +		if (edid) {
> +			specs = kzalloc(sizeof(*specs), GFP_KERNEL);
> +			if (specs) {
> +				fb_edid_to_monspecs(edid, specs);
> +				def_mode = specs->modedb;
> +			}
> +		}
> +		kfree(edid);
> +	}
> +
> +	of_property_read_u32(child, "bpp", &bpp);
> +
> +	/* If flags property is obtained, fbsub is returned. */
> +	if (!of_property_read_u32(child, "smi,flags", &flags)) {
> +		fbsub = devm_kzalloc(dev, sizeof(*fbsub), GFP_KERNEL);
> +		if (fbsub) {
> +			fbsub->def_mode = def_mode;
> +			fbsub->def_bpp = bpp;
> +			fbsub->flags = flags;
> +		}
> +	}
> +	return fbsub;
> +}
> +
> +/* Build platform_data from OF property */
> +static struct sm501_platdata_fb *pdata_from_dt(struct device *dev, struct device_node *np)
> +{
> +	enum sm501_fb_routing fb_route = SM501_FB_OWN;
> +	struct sm501_platdata_fb *pdata = NULL;
> +	struct sm501_platdata_fbsub *fb_crt;
> +	struct sm501_platdata_fbsub *fb_pnl;
> +	unsigned int flags = 0;
> +
> +	if (of_property_read_bool(np, "route-crt-panel"))
> +		fb_route = SM501_FB_CRT_PANEL;
> +	if (of_property_read_bool(np, "swap-fb-endian"))
> +		flags = SM501_FBPD_SWAP_FB_ENDIAN;
> +	fb_crt = dt_fbsub(dev, np, "crt");
> +	fb_pnl = dt_fbsub(dev, np, "panel");
> +	if (fb_crt || fb_pnl) {
> +		pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> +		if (pdata) {
> +			pdata->fb_route = fb_route;
> +			pdata->flags = flags;
> +			pdata->fb_crt = fb_crt;
> +			pdata->fb_pnl = fb_pnl;
> +		}
> +	}
> +	return pdata;
> +}
> +#endif
> +
>  static int sm501fb_probe(struct platform_device *pdev)
>  {
>  	struct sm501fb_info *info;
> @@ -1974,6 +2050,12 @@ static int sm501fb_probe(struct platform_device *pdev)
>  				if (info->edid_data)
>  					found = 1;
>  			}
> +			/* Get platform data compatible configuration */
> +			if (!found) {
> +				info->pdata = pdata_from_dt(dev, np);
> +				if (info->pdata)
> +					found = 1;
> +			}
>  		}
>  #endif
>  		if (!found) {
> -- 
> 2.39.2
>
Thomas Gleixner Dec. 8, 2023, 3:49 p.m. UTC | #10
On Tue, Dec 05 2023 at 18:45, Yoshinori Sato wrote:
> +static void endisable_irq(struct irq_data *data, int enable)

bool enable?

> +{
> +	struct sh7751_intc_priv *priv;
> +	unsigned int irq;
> +
> +	priv = irq_data_to_priv(data);
> +
> +	irq = irqd_to_hwirq(data);
> +	if (!is_valid_irq(irq)) {
> +		/* IRQ out of range */
> +		pr_warn_once("%s: IRQ %u is out of range\n", __FILE__, irq);
> +		return;
> +	}
> +
> +	if (irq <= MAX_IRL && !priv->irlm)
> +		/* IRL encoded external interrupt */
> +		/* disable for SR.IMASK */
> +		update_sr_imask(irq - IRQ_START, enable);
> +	else
> +		/* Internal peripheral interrupt */
> +		/* mask for IPR priority 0 */
> +		update_ipr(priv, irq, enable);

Lacks curly brackets on the if/else

> +static int irq_sh7751_map(struct irq_domain *h, unsigned int virq,
> +			  irq_hw_number_t hw_irq_num)
> +{
> +	irq_set_chip_and_handler(virq, &sh7751_irq_chip, handle_level_irq);
> +	irq_get_irq_data(virq)->chip_data = h->host_data;
> +	irq_modify_status(virq, IRQ_NOREQUEST, IRQ_NOPROBE);
> +	return 0;
> +}
> +static const struct irq_domain_ops irq_ops = {

Newline before 'static ...'

> +	.map    = irq_sh7751_map,
> +	.xlate  = irq_domain_xlate_onecell,
> +};
> +
> +static int __init load_ipr_map(struct device_node *intc,
> +			       struct sh7751_intc_priv *priv)
> +{
> +	struct property *ipr_map;
> +	unsigned int num_ipr, i;
> +	struct ipr *ipr;
> +	const __be32 *p;
> +	u32 irq;
> +
> +	ipr_map = of_find_property(intc, "renesas,ipr-map", &num_ipr);
> +	if (IS_ERR(ipr_map))
> +		return PTR_ERR(ipr_map);
> +	num_ipr /= sizeof(u32);
> +	/* 3words per entry. */
> +	if (num_ipr % 3)

Three words per ... But you can spare the comment by doing:

        if (num_ipr % WORDS_PER_ENTRY)

> +		goto error1;
> +	num_ipr /= 3;
> +static int __init sh7751_intc_of_init(struct device_node *intc,
> +				      struct device_node *parent)
> +{
> +	struct sh7751_intc_priv *priv;
> +	void __iomem *base, *base2;
> +	struct irq_domain *domain;
> +	u16 icr;
> +	int ret;
> +
> +	base = of_iomap(intc, 0);
> +	base2 = of_iomap(intc, 1);
> +	if (!base || !base2) {
> +		pr_err("%pOFP: Invalid register definition\n", intc);

What unmaps 'base' if 'base' is valid and base2 == NULL?

> +		return -EINVAL;
> +	}
> +
> +	priv = kzalloc(sizeof(struct sh7751_intc_priv), GFP_KERNEL);
> +	if (priv == NULL)
> +		return -ENOMEM;

Leaks base[2] maps, no?

> +	ret = load_ipr_map(intc, priv);
> +	if (ret < 0) {
> +		kfree(priv);
> +		return ret;
> +	}
> +
> +	priv->base = base;
> +	priv->intpri00 = base2;
> +
> +	if (of_property_read_bool(intc, "renesas,irlm")) {
> +		priv->irlm = true;
> +		icr = __raw_readw(priv->base + R_ICR);
> +		icr |= ICR_IRLM;
> +		__raw_writew(icr, priv->base + R_ICR);
> +	}
> +
> +	domain = irq_domain_add_linear(intc, NR_IRQS, &irq_ops, priv);
> +	if (domain == NULL) {
> +		pr_err("%pOFP: cannot initialize irq domain\n", intc);
> +		kfree(priv);
> +		return -ENOMEM;
> +	}
> +
> +	irq_set_default_host(domain);
> +	pr_info("%pOFP: SH7751 Interrupt controller (%s external IRQ)",
> +		intc, priv->irlm ? "4 lines" : "15 level");
> +	return 0;
> +}
> +
> +IRQCHIP_DECLARE(sh_7751_intc,
> +		"renesas,sh7751-intc", sh7751_intc_of_init);

One line please.

Thanks,

        tglx