Message ID | cover.1701768028.git.ysato@users.sourceforge.jp |
---|---|
Headers | show |
Series | Device Tree support for SH7751 based board | expand |
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
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
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
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
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
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
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
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
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 >
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