Re: [PATCH v3 00/42] ep93xx device tree conversion

Re: [PATCH v3 00/42] ep93xx device tree conversion

[Krzysztof Kozlowski]

On 20/07/2023 13:29, Nikita Shubin via B4 Relay wrote:
> This series aims to convert ep93xx from platform to full device tree support.
> 
> The main goal is to receive ACK's to take it via Arnd's arm-soc branch.
> 
> I've moved to b4, tricking it to consider v0 as v1, so it consider's
> this version to be v3, this exactly the third version.

Fix your clock/timezone, so your patches are not sent in the future.
Unfortunately this will stay at top of my queue, which is unfair, so I
will just ignore for now.

Best regards,
Krzysztof

Re: [PATCH v3 13/42] watchdog: ep93xx: add DT support for Cirrus EP93xx

[Alexander Sverdlin]

Hi Nikita,

On Thu, 2023-07-20 at 14:29 +0300, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
> 
> Add OF ID match table.
> 
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>

Tested-by: Alexander Sverdlin <alexander.sverdlin@gmail.com>

-- 
Alexander Sverdlin.

Re: [PATCH v3 38/42] ata: pata_ep93xx: remove legacy pinctrl use

[Sergey Shtylyov]

Hello!

On 7/20/23 2:29 PM, Nikita Shubin via B4 Relay wrote:

> From: Nikita Shubin <nikita.shubin@maquefel.me>
> 
> Drop legacy acquire/release since we are using pinctrl for this now.
> 
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>

   I think I've already given you my:

Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>

[...]

MBR, Sergey

Re: [PATCH v3 26/42] ata: pata_ep93xx: add device tree support

[Sergey Shtylyov]

On 7/20/23 2:29 PM, Nikita Shubin via B4 Relay wrote:

> From: Nikita Shubin <nikita.shubin@maquefel.me>
> 
> - Add OF ID match table
> - Drop ep93xx_chip_revision and use soc_device_match instead
> 
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
> ---
>  drivers/ata/pata_ep93xx.c | 26 ++++++++++++++++++++------
>  1 file changed, 20 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/ata/pata_ep93xx.c b/drivers/ata/pata_ep93xx.c
> index c6e043e05d43..a88824dfc5fa 100644
> --- a/drivers/ata/pata_ep93xx.c
> +++ b/drivers/ata/pata_ep93xx.c
[...]
> @@ -910,6 +912,12 @@ static struct ata_port_operations ep93xx_pata_port_ops = {
>  	.port_start		= ep93xx_pata_port_start,
>  };
>  
> +static const struct soc_device_attribute ep93xx_soc_table[] = {
> +	{ .revision = "E1", .data = (void *)ATA_UDMA3 },
> +	{ .revision = "E2", .data = (void *)ATA_UDMA4 },
> +	{ /* sentinel */ }
> +};
> +
>  static int ep93xx_pata_probe(struct platform_device *pdev)
>  {
>  	struct ep93xx_pata_data *drv_data;
> @@ -939,7 +947,7 @@ static int ep93xx_pata_probe(struct platform_device *pdev)
>  
>  	drv_data = devm_kzalloc(&pdev->dev, sizeof(*drv_data), GFP_KERNEL);
>  	if (!drv_data) {
> -		err = -ENXIO;
> +		err = -ENOMEM;
>  		goto err_rel_gpio;
>  	}
>  

   Hm, deserves its own patch. And even for this one, you should've documented it
in the patch secription...

> @@ -976,12 +984,11 @@ static int ep93xx_pata_probe(struct platform_device *pdev)
>  	 * so this driver supports only UDMA modes.
>  	 */
>  	if (drv_data->dma_rx_channel && drv_data->dma_tx_channel) {
> -		int chip_rev = ep93xx_chip_revision();
> +		const struct soc_device_attribute *match;
>  
> -		if (chip_rev == EP93XX_CHIP_REV_E1)
> -			ap->udma_mask = ATA_UDMA3;
> -		else if (chip_rev == EP93XX_CHIP_REV_E2)
> -			ap->udma_mask = ATA_UDMA4;
> +		match = soc_device_match(ep93xx_soc_table);
> +		if (match)
> +			ap->udma_mask = (unsigned int) match->data;
>  		else
>  			ap->udma_mask = ATA_UDMA2;
>  	}

   This one also looks as it could have been done separately -- before the DT
conversion?

[...]

MBR, Sergey

Re: [PATCH v3 12/42] dt-bindings: watchdog: Add Cirrus EP93x

[Guenter Roeck]

On 7/20/23 04:29, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
> 
> This adds device tree bindings for the Cirrus Logic EP93xx
> watchdog block used in these SoCs.
> 
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
> ---
>   .../bindings/watchdog/cirrus,ep9301-wdt.yaml       | 46 ++++++++++++++++++++++
>   1 file changed, 46 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/cirrus,ep9301-wdt.yaml b/Documentation/devicetree/bindings/watchdog/cirrus,ep9301-wdt.yaml
> new file mode 100644
> index 000000000000..d54595174a12
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/watchdog/cirrus,ep9301-wdt.yaml
> @@ -0,0 +1,46 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/watchdog/cirrus,ep9301-wdt.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Cirrus Logic EP93xx Watchdog Timer
> +
> +maintainers:
> +  - Nikita Shubin <nikita.shubin@maquefel.me>
> +  - Alexander Sverdlin <alexander.sverdlin@gmail.com>
> +
> +description:
> +  Cirrus Logic EP93xx SoC family has it's own watchdog implementation
> +

Odd description. Isn't that true for pretty much every devicetree
bindings file, and pretty much every hardware driver ?

> +allOf:
> +  - $ref: watchdog.yaml#
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - const: cirrus,ep9301-wdt
> +      - items:
> +          - enum:
> +              - cirrus,ep9302-wdt
> +              - cirrus,ep9307-wdt
> +              - cirrus,ep9312-wdt
> +              - cirrus,ep9315-wdt
> +          - const: cirrus,ep9301-wdt
> +
> +  reg:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    watchdog@80940000 {
> +        compatible = "cirrus,ep9301-wdt";
> +        reg = <0x80940000 0x08>;
> +    };
> +
> 

Re: [PATCH v3 13/42] watchdog: ep93xx: add DT support for Cirrus EP93xx

[Guenter Roeck]

On 7/20/23 04:29, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
> 
> Add OF ID match table.
> 
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>   drivers/watchdog/ep93xx_wdt.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/watchdog/ep93xx_wdt.c b/drivers/watchdog/ep93xx_wdt.c
> index 59dfd7f6bf0b..af89b7bb8f66 100644
> --- a/drivers/watchdog/ep93xx_wdt.c
> +++ b/drivers/watchdog/ep93xx_wdt.c
> @@ -19,6 +19,7 @@
>    */
>   
>   #include <linux/platform_device.h>
> +#include <linux/mod_devicetable.h>
>   #include <linux/module.h>
>   #include <linux/watchdog.h>
>   #include <linux/io.h>
> @@ -127,9 +128,16 @@ static int ep93xx_wdt_probe(struct platform_device *pdev)
>   	return 0;
>   }
>   
> +static const struct of_device_id ep93xx_wdt_of_ids[] = {
> +	{ .compatible = "cirrus,ep9301-wdt" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, ep93xx_wdt_of_ids);
> +
>   static struct platform_driver ep93xx_wdt_driver = {
>   	.driver		= {
>   		.name	= "ep93xx-wdt",
> +		.of_match_table = ep93xx_wdt_of_ids,
>   	},
>   	.probe		= ep93xx_wdt_probe,
>   };
> 

Re: [PATCH v3 32/42] wdt: ts72xx: add DT support for ts72xx

[Guenter Roeck]

On 7/20/23 04:29, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
> 
> Add OF ID match table.
> 
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>   drivers/watchdog/ts72xx_wdt.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/watchdog/ts72xx_wdt.c b/drivers/watchdog/ts72xx_wdt.c
> index 3d57670befe1..ac709dc31a65 100644
> --- a/drivers/watchdog/ts72xx_wdt.c
> +++ b/drivers/watchdog/ts72xx_wdt.c
> @@ -12,6 +12,7 @@
>    */
>   
>   #include <linux/platform_device.h>
> +#include <linux/mod_devicetable.h>
>   #include <linux/module.h>
>   #include <linux/watchdog.h>
>   #include <linux/io.h>
> @@ -160,10 +161,17 @@ static int ts72xx_wdt_probe(struct platform_device *pdev)
>   	return 0;
>   }
>   
> +static const struct of_device_id ts72xx_wdt_of_ids[] = {
> +	{ .compatible = "technologic,ts7200-wdt" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, ts72xx_wdt_of_ids);
> +
>   static struct platform_driver ts72xx_wdt_driver = {
>   	.probe		= ts72xx_wdt_probe,
>   	.driver		= {
>   		.name	= "ts72xx-wdt",
> +		.of_match_table = ts72xx_wdt_of_ids,
>   	},
>   };
>   
> 

Re: [PATCH v3 33/42] gpio: ep93xx: add DT support for gpio-ep93xx

[Bartosz Golaszewski]

On Thu, Jul 20, 2023 at 10:30 AM Nikita Shubin via B4 Relay
<devnull+nikita.shubin.maquefel.me@kernel.org> wrote:
>
> From: Nikita Shubin <nikita.shubin@maquefel.me>
>
> Add OF ID match table.
>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
> ---
>  drivers/gpio/gpio-ep93xx.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/gpio/gpio-ep93xx.c b/drivers/gpio/gpio-ep93xx.c
> index 9a25bb0caf17..c4e272a8773d 100644
> --- a/drivers/gpio/gpio-ep93xx.c
> +++ b/drivers/gpio/gpio-ep93xx.c
> @@ -360,9 +360,15 @@ static int ep93xx_gpio_probe(struct platform_device *pdev)
>         return devm_gpiochip_add_data(&pdev->dev, gc, egc);
>  }
>
> +static const struct of_device_id ep93xx_gpio_match[] = {
> +       { .compatible = "cirrus,ep9301-gpio" },
> +       { /* sentinel */ }
> +};
> +
>  static struct platform_driver ep93xx_gpio_driver = {
>         .driver         = {
>                 .name   = "gpio-ep93xx",
> +               .of_match_table = ep93xx_gpio_match,
>         },
>         .probe          = ep93xx_gpio_probe,
>  };
>
> --
> 2.39.2
>

Acked-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Re: [PATCH v3 01/42] gpio: ep93xx: split device in multiple

[Bartosz Golaszewski]

On Thu, Jul 20, 2023 at 10:29 AM Nikita Shubin via B4 Relay
<devnull+nikita.shubin.maquefel.me@kernel.org> wrote:
>
> From: Nikita Shubin <nikita.shubin@maquefel.me>
>
> This prepares ep93xx SOC gpio to convert into device tree driver:
> - dropped banks and legacy defines
> - split AB IRQ and make it shared
>
> We are relying on IRQ number information A, B ports have single shared
> IRQ, while F port have dedicated IRQ for each line.
>
> Also we had to split single ep93xx platform_device into multiple, one
> for each port, without this we can't do a full working transition from
> legacy platform code into device tree capable. All GPIO_LOOKUP were
> change to match new chip namings.
>
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> ---

Acked-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Re: [PATCH v3 28/42] input: keypad: ep93xx: add DT support for Cirrus EP93xx

[Dmitry Torokhov]

On Thu, Jul 20, 2023 at 02:29:28PM +0300, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
> 
> - drop flags, they were not used anyway
> - add OF ID match table
> - process "autorepeat", "debounce-delay-ms", prescale from device tree
> - drop platform data usage and it's header
> - keymap goes from device tree now on
> 
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>

This is awesome, thank you!

>  
>  #include <linux/bits.h>
>  #include <linux/module.h>
> +#include <linux/of.h>

Are you sure you need this? I think the only OF-specific structure that
is being used is of_device_id, which comes from mod_devicetable.h that
you include below.

Otherwise:

Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Please feel free to merge with the rest of the series.

Thanks.

-- 
Dmitry

Re: [PATCH v3 01/42] gpio: ep93xx: split device in multiple

[Andy Shevchenko]

On Thu, Jul 20, 2023 at 02:29:01PM +0300, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
> 
> This prepares ep93xx SOC gpio to convert into device tree driver:
> - dropped banks and legacy defines
> - split AB IRQ and make it shared
> 
> We are relying on IRQ number information A, B ports have single shared
> IRQ, while F port have dedicated IRQ for each line.
> 
> Also we had to split single ep93xx platform_device into multiple, one
> for each port, without this we can't do a full working transition from
> legacy platform code into device tree capable. All GPIO_LOOKUP were
> change to match new chip namings.

...

> -static void ep93xx_gpio_ab_irq_handler(struct irq_desc *desc)
> +static u32 ep93xx_gpio_ab_irq_handler(struct gpio_chip *gc)
>  {
> -	struct gpio_chip *gc = irq_desc_get_handler_data(desc);
> -	struct ep93xx_gpio *epg = gpiochip_get_data(gc);
> -	struct irq_chip *irqchip = irq_desc_get_chip(desc);
> +	struct ep93xx_gpio_irq_chip *eic = to_ep93xx_gpio_irq_chip(gc);
>  	unsigned long stat;
>  	int offset;
>  
> -	chained_irq_enter(irqchip, desc);
> -
> -	/*
> -	 * Dispatch the IRQs to the irqdomain of each A and B
> -	 * gpiochip irqdomains depending on what has fired.
> -	 * The tricky part is that the IRQ line is shared
> -	 * between bank A and B and each has their own gpiochip.
> -	 */
> -	stat = readb(epg->base + EP93XX_GPIO_A_INT_STATUS);
> +	stat = readb(eic->base + EP93XX_INT_STATUS_OFFSET);
>  	for_each_set_bit(offset, &stat, 8)
> -		generic_handle_domain_irq(epg->gc[0].gc.irq.domain,
> -					  offset);
> +		generic_handle_domain_irq(gc->irq.domain, offset);
>  
> -	stat = readb(epg->base + EP93XX_GPIO_B_INT_STATUS);
> -	for_each_set_bit(offset, &stat, 8)
> -		generic_handle_domain_irq(epg->gc[1].gc.irq.domain,
> -					  offset);
> +	return stat;
> +}
>  
> -	chained_irq_exit(irqchip, desc);
> +static irqreturn_t ep93xx_ab_irq_handler(int irq, void *dev_id)
> +{
> +	return IRQ_RETVAL(ep93xx_gpio_ab_irq_handler(dev_id));
>  }
>  
>  static void ep93xx_gpio_f_irq_handler(struct irq_desc *desc)
>  {
> -	/*
> -	 * map discontiguous hw irq range to continuous sw irq range:
> -	 *
> -	 *  IRQ_EP93XX_GPIO{0..7}MUX -> EP93XX_GPIO_LINE_F{0..7}
> -	 */
>  	struct irq_chip *irqchip = irq_desc_get_chip(desc);
> -	unsigned int irq = irq_desc_get_irq(desc);
> -	int port_f_idx = (irq & 7) ^ 4; /* {20..23,48..51} -> {0..7} */
> -	int gpio_irq = EP93XX_GPIO_F_IRQ_BASE + port_f_idx;
> +	struct gpio_chip *gc = irq_desc_get_handler_data(desc);
> +	struct gpio_irq_chip *gic = &gc->irq;
> +	unsigned int parent = irq_desc_get_irq(desc);
> +	unsigned int i;
>  
>  	chained_irq_enter(irqchip, desc);
> -	generic_handle_irq(gpio_irq);
> +	for (i = 0; i < gic->num_parents; i++)
> +		if (gic->parents[i] == parent)
> +			break;
> +
> +	if (i < gic->num_parents)
> +		generic_handle_irq(irq_find_mapping(gc->irq.domain, i));

Can we use

		generic_handle_domain_irq(gc->irq.domain, i);

here as well?

>  	chained_irq_exit(irqchip, desc);
>  }

...

> -	int offset = d->irq & 7;
> +	int offset = irqd_to_hwirq(d);

	irq_hw_number_t ?

>  	irq_flow_handler_t handler;

...

> +	int ret, irq, i = 0;

What do you need this assignment for?

...

> +		ret = devm_request_irq(dev, irq,
> +				ep93xx_ab_irq_handler,

It can be located on the previous line.

> +				IRQF_SHARED, gc->label, gc);
> +		if (ret)
> +			return dev_err_probe(dev, ret, "error requesting IRQ : %d\n", irq);

Drop duplicating word 'error' in the message.
Space is not needed before colon.

...

> +	/* TODO: replace with handle_bad_irq once we are fully hierarchical */

To be pedantic: handle_bad_irq()

> +	gc->label = dev_name(&pdev->dev);
> +	if (platform_irq_count(pdev) > 0) {
> +		dev_dbg(&pdev->dev, "setting up irqs for %s\n", dev_name(&pdev->dev));
> +		ret = ep93xx_setup_irqs(pdev, egc);
> +		if (ret)

> +			dev_err(&pdev->dev, "setup irqs failed for %s\n", dev_name(&pdev->dev));

What's the point to print dev name twice? Esp. taking into account
gc->label assignment above. Why not use dev_err_probe() to unify
the format of the messages from ->probe()?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 03/42] clk: ep93xx: add DT support for Cirrus EP93xx

[Andy Shevchenko]

On Thu, Jul 20, 2023 at 04:27:45PM -0700, Stephen Boyd wrote:
> Quoting Nikita Shubin via B4 Relay (2023-07-20 04:29:03)

...

> > +static bool is_best(unsigned long rate, unsigned long now,
> > +                    unsigned long best)
> > +{
> > +       return abs(rate - now) < abs(rate - best);
> 
> Another case where we need abs_diff() so that it doesn't get confused
> when trying to do signed comparison.

Here you are: Message-Id: <20230721134235.15517-1-andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 02/42] dt-bindings: clock: Add Cirrus EP93xx

[Krzysztof Kozlowski]

On 20/07/2023 13:29, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
> 
> This adds device tree bindings for the Cirrus Logic EP93xx
> clock block used in these SoCs.
> 
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
> ---
>  .../bindings/clock/cirrus,ep9301-clk.yaml          | 46 ++++++++++++++++++++++
>  include/dt-bindings/clock/cirrus,ep93xx-clock.h    | 41 +++++++++++++++++++
>  2 files changed, 87 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/clock/cirrus,ep9301-clk.yaml b/Documentation/devicetree/bindings/clock/cirrus,ep9301-clk.yaml
> new file mode 100644
> index 000000000000..111e016601fb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/cirrus,ep9301-clk.yaml
> @@ -0,0 +1,46 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/cirrus,ep9301-clk.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Cirrus Logic ep93xx SoC's clock controller
> +
> +maintainers:
> +  - Nikita Shubin <nikita.shubin@maquefel.me>
> +  - Alexander Sverdlin <alexander.sverdlin@gmail.com>
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - const: cirrus,ep9301-clk
> +      - items:
> +          - enum:
> +              - cirrus,ep9302-clk
> +              - cirrus,ep9307-clk
> +              - cirrus,ep9312-clk
> +              - cirrus,ep9315-clk
> +          - const: cirrus,ep9301-clk
> +
> +  "#clock-cells":
> +    const: 1
> +
> +  clocks:
> +    items:
> +      - description: reference clock
> +
> +required:
> +  - compatible
> +  - "#clock-cells"
> +  - clocks
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    clock-controller {
> +      compatible = "cirrus,ep9301-clk";
> +      #clock-cells = <1>;
> +      clocks = <&xtali>;
> +    };
> +...
> diff --git a/include/dt-bindings/clock/cirrus,ep93xx-clock.h b/include/dt-bindings/clock/cirrus,ep93xx-clock.h
> new file mode 100644
> index 000000000000..3cd053c0fdea
> --- /dev/null
> +++ b/include/dt-bindings/clock/cirrus,ep93xx-clock.h

Keep the same filename as bindings file.

Best regards,
Krzysztof

Re: [PATCH v3 04/42] dt-bindings: pinctrl: Add Cirrus EP93xx

[Krzysztof Kozlowski]

On 20/07/2023 13:29, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
> 
> Add YAML bindings for ep93xx SoC pinctrl.
> 
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  .../bindings/pinctrl/cirrus,ep9301-pinctrl.yaml    | 58 ++++++++++++++++++++++
>  1 file changed, 58 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/cirrus,ep9301-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/cirrus,ep9301-pinctrl.yaml
> new file mode 100644
> index 000000000000..d5682531b0da
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/cirrus,ep9301-pinctrl.yaml
> @@ -0,0 +1,58 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pinctrl/cirrus,ep9301-pinctrl.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Cirrus ep93xx pins mux controller
> +
> +maintainers:
> +  - Nikita Shubin <nikita.shubin@maquefel.me>
> +  - Alexander Sverdlin <alexander.sverdlin@gmail.com>
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - const: cirrus,ep9301-pinctrl
> +      - items:
> +          - enum:
> +              - cirrus,ep9302-pinctrl
> +              - cirrus,ep9307-pinctrl
> +              - cirrus,ep9312-pinctrl
> +              - cirrus,ep9315-pinctrl
> +          - const: cirrus,ep9301-pinctrl
> +
> +patternProperties:
> +  '^pins-':
> +    type: object
> +    description: pin node
> +    $ref: pinmux-node.yaml#

You need:
unevaluatedProperties: false



Best regards,
Krzysztof

Re: [PATCH v3 06/42] dt-bindings: soc: Add Cirrus EP93xx

[Krzysztof Kozlowski]

On 20/07/2023 13:29, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
> 
> This adds device tree bindings for the Cirrus Logic EP93xx.
> 
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
> ---
>  .../bindings/arm/cirrus/ep9301-syscon.yaml         | 59 ++++++++++++++++++++++

syscon goes to soc directory. Also, please add vendor prefix to the
filenames.

>  .../devicetree/bindings/arm/cirrus/ep9301.yaml     | 39 ++++++++++++++
>  2 files changed, 98 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/cirrus/ep9301-syscon.yaml b/Documentation/devicetree/bindings/arm/cirrus/ep9301-syscon.yaml
> new file mode 100644
> index 000000000000..77fbe1f006fd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/cirrus/ep9301-syscon.yaml
> @@ -0,0 +1,59 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/arm/cirrus/ep9301-syscon.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Cirrus Logic EP93xx Platforms System Controller
> +
> +maintainers:
> +  - Alexander Sverdlin <alexander.sverdlin@gmail.com>
> +  - Nikita Shubin <nikita.shubin@maquefel.me>
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - items:
> +          - enum:
> +              - cirrus,ep9302-syscon
> +              - cirrus,ep9307-syscon
> +              - cirrus,ep9312-syscon
> +              - cirrus,ep9315-syscon
> +          - const: cirrus,ep9301-syscon
> +          - const: syscon
> +          - const: simple-mfd
> +      - items:
> +          - const: cirrus,ep9301-syscon
> +          - const: syscon
> +          - const: simple-mfd
> +
> +  reg:
> +    maxItems: 1
> +
> +  reboot:
> +    type: object
> +    properties:
> +      compatible:
> +        const: cirrus,ep9301-reboot

Patch introducing it should be before this one. Also, do not use
different styles for your child nodes. Your other nodes use $ref.

> +
> +  clock-controller:
> +    type: object
> +    $ref: ../../clock/cirrus,ep9301-clk.yaml

Absolute path, so /schemas/clock/cirrus.....

> +
> +  pinctrl:
> +    type: object
> +    $ref: ../../pinctrl/cirrus,ep9301-pinctrl.yaml

Ditto

> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    syscon@80930000 {
> +      compatible = "cirrus,ep9301-syscon",
> +                   "syscon", "simple-mfd";
> +      reg = <0x80930000 0x1000>;

Incomplete example.

> +    };
> diff --git a/Documentation/devicetree/bindings/arm/cirrus/ep9301.yaml b/Documentation/devicetree/bindings/arm/cirrus/ep9301.yaml
> new file mode 100644
> index 000000000000..6087784e93fb
> --- /dev/null


Best regards,
Krzysztof

Re: [PATCH v3 10/42] dt-bindings: rtc: Add Cirrus EP93xx

[Krzysztof Kozlowski]

On 20/07/2023 13:29, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
> 
> This adds device tree bindings for the Cirrus Logic EP93xx
> RTC block used in these SoCs.
> 
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
> ---
>  .../devicetree/bindings/rtc/cirrus,ep9301-rtc.yaml | 40 ++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/rtc/cirrus,ep9301-rtc.yaml b/Documentation/devicetree/bindings/rtc/cirrus,ep9301-rtc.yaml
> new file mode 100644
> index 000000000000..63572c197e92
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/rtc/cirrus,ep9301-rtc.yaml
> @@ -0,0 +1,40 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/rtc/cirrus,ep9301-rtc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Cirrus EP93xx Real Time Clock controller
> +
> +maintainers:
> +  - Hartley Sweeten <hsweeten@visionengravers.com>
> +  - Alexander Sverdlin <alexander.sverdlin@gmail.com>
> +

allOf: with $ref to rtc.yaml

> +properties:
> +  compatible:
> +    oneOf:
> +      - const: cirrus,ep9301-rtc
> +      - items:
> +          - enum:
> +              - cirrus,ep9302-rtc
> +              - cirrus,ep9307-rtc
> +              - cirrus,ep9312-rtc
> +              - cirrus,ep9315-rtc
> +          - const: cirrus,ep9301-rtc
> +
> +  reg:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false

and then unevaluatedProperties instead.

> +
> +examples:
> +  - |
> +    rtc@80920000 {
> +      compatible = "cirrus,ep9301-rtc";
> +      reg = <0x80920000 0x100>;
> +    };
> +


Each of your file has a trailing blank line.

Best regards,
Krzysztof

Re: [PATCH v3 12/42] dt-bindings: watchdog: Add Cirrus EP93x

[Krzysztof Kozlowski]

On 20/07/2023 13:29, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
> 
> This adds device tree bindings for the Cirrus Logic EP93xx

Every patch:

Please do not use "This commit/patch", but imperative mood. See longer
explanation here:
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95


Best regards,
Krzysztof

Re: [PATCH v3 29/42] dt-bindings: rtc: Add ST M48T86

[Krzysztof Kozlowski]

On 20/07/2023 13:29, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
> 
> Add YAML bindings for ST M48T86 / Dallas DS12887 RTC.
> 

This shouldn't really be part of this patchset. It's not part of your SoC.

Best regards,
Krzysztof

Re: [PATCH v3 34/42] ARM: dts: add Cirrus EP93XX SoC .dtsi

[Krzysztof Kozlowski]

On 20/07/2023 13:29, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
> 
> Add support for Cirrus Logic EP93XX SoC's family.
> 
> Co-developed-by: Alexander Sverdlin <alexander.sverdlin@gmail.com>
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@gmail.com>
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
> ---
>  arch/arm/boot/dts/cirrus/ep93xx.dtsi | 449 +++++++++++++++++++++++++++++++++++
>  1 file changed, 449 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/cirrus/ep93xx.dtsi b/arch/arm/boot/dts/cirrus/ep93xx.dtsi
> new file mode 100644
> index 000000000000..1e04f39d7b80
> --- /dev/null
> +++ b/arch/arm/boot/dts/cirrus/ep93xx.dtsi
> @@ -0,0 +1,449 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Device Tree file for Cirrus Logic systems EP93XX SoC
> + */
> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/leds/common.h>
> +#include <dt-bindings/input/input.h>
> +#include <dt-bindings/clock/cirrus,ep93xx-clock.h>
> +/ {
> +	soc: soc {
> +		compatible = "simple-bus";
> +		ranges;
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +
> +		syscon: syscon@80930000 {
> +			compatible = "cirrus,ep9301-syscon",
> +						 "syscon", "simple-mfd";

Fix alignment.

> +			reg = <0x80930000 0x1000>;
> +
> +			eclk: clock-controller {
> +				compatible = "cirrus,ep9301-clk";
> +				#clock-cells = <1>;
> +				clocks = <&xtali>;
> +				status = "okay";

Drop statuses when not needed.

> +			};
> +
> +			pinctrl: pinctrl {
> +				compatible = "cirrus,ep9301-pinctrl";
> +
> +				spi_default_pins: pins-spi {
> +					function = "spi";
> +					groups = "ssp";
> +				};
> +

...

> +
> +		keypad: keypad@800f0000 {
> +			compatible = "cirrus,ep9307-keypad";
> +			reg = <0x800f0000 0x0c>;
> +			interrupt-parent = <&vic0>;
> +			interrupts = <29>;
> +			clocks = <&eclk EP93XX_CLK_KEYPAD>;
> +			pinctrl-names = "default";
> +			pinctrl-0 = <&keypad_default_pins>;
> +			linux,keymap =

No need for line break.

> +					<KEY_UP>,
> +					<KEY_DOWN>,
> +					<KEY_VOLUMEDOWN>,
> +					<KEY_HOME>,
> +					<KEY_RIGHT>,
> +					<KEY_LEFT>,
> +					<KEY_ENTER>,
> +					<KEY_VOLUMEUP>,
> +					<KEY_F6>,
> +					<KEY_F8>,
> +					<KEY_F9>,
> +					<KEY_F10>,
> +					<KEY_F1>,
> +					<KEY_F2>,
> +					<KEY_F3>,
> +					<KEY_POWER>;
> +		};
> +
> +		pwm0: pwm@80910000 {
> +			compatible = "cirrus,ep9301-pwm";
> +			reg = <0x80910000 0x10>;
> +			clocks = <&eclk EP93XX_CLK_PWM>;
> +			status = "disabled";
> +		};
> +
> +		pwm1: pwm@80910020 {
> +			compatible = "cirrus,ep9301-pwm";
> +			reg = <0x80910020 0x10>;
> +			clocks = <&eclk EP93XX_CLK_PWM>;
> +			pinctrl-names = "default";
> +			pinctrl-0 = <&pwm1_default_pins>;
> +			status = "disabled";
> +		};
> +
> +		rtc0: rtc@80920000 {
> +			compatible = "cirrus,ep9301-rtc";
> +			reg = <0x80920000 0x100>;
> +		};
> +
> +		spi0: spi@808a0000 {
> +			compatible = "cirrus,ep9301-spi";
> +			reg = <0x808a0000 0x18>;
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			interrupt-parent = <&vic1>;
> +			interrupts = <21>;
> +			clocks = <&eclk EP93XX_CLK_SPI>;
> +			pinctrl-names = "default";
> +			pinctrl-0 = <&spi_default_pins>;
> +			status = "disabled";
> +		};
> +
> +		timer: timer@80810000 {
> +			compatible = "cirrus,ep9301-timer";
> +			reg = <0x80810000 0x100>;
> +			interrupt-parent = <&vic1>;
> +			interrupts = <19>;
> +		};
> +
> +		uart0: uart@808c0000 {
> +			compatible = "arm,primecell";

This looks incomplete.

> +			reg = <0x808c0000 0x1000>;
> +			arm,primecell-periphid = <0x00041010>;
> +			clocks = <&eclk EP93XX_CLK_UART1>, <&eclk EP93XX_CLK_UART>;
> +			clock-names = "apb:uart1", "apb_pclk";

It does not look like you tested the DTS against bindings. Please run
`make dtbs_check` (see
Documentation/devicetree/bindings/writing-schema.rst or
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
for instructions).

> +			interrupt-parent = <&vic1>;
> +			interrupts = <20>;
> +			status = "disabled";
> +		};
> +
> +		uart1: uart@808d0000 {
> +			compatible = "arm,primecell";
> +			reg = <0x808d0000 0x1000>;
> +			arm,primecell-periphid = <0x00041010>;
> +			clocks = <&eclk EP93XX_CLK_UART2>, <&eclk EP93XX_CLK_UART>;
> +			clock-names = "apb:uart2", "apb_pclk";

It does not look like you tested the DTS against bindings. Please run
`make dtbs_check` (see
Documentation/devicetree/bindings/writing-schema.rst or
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
for instructions).

> +			interrupt-parent = <&vic1>;
> +			interrupts = <22>;
> +			status = "disabled";
> +		};
> +
> +		uart2: uart@808b0000 {
> +			compatible = "arm,primecell";
> +			reg = <0x808b0000 0x1000>;
> +			arm,primecell-periphid = <0x00041010>;
> +			clocks = <&eclk EP93XX_CLK_UART3>, <&eclk EP93XX_CLK_UART>;
> +			clock-names = "apb:uart3", "apb_pclk";
> +			interrupt-parent = <&vic1>;
> +			interrupts = <23>;
> +			status = "disabled";
> +		};
> +
> +		usb0: usb@80020000 {
> +			compatible = "generic-ohci";
> +			reg = <0x80020000 0x10000>;
> +			interrupt-parent = <&vic1>;
> +			interrupts = <24>;
> +			clocks = <&eclk EP93XX_CLK_USB>;
> +			status = "disabled";
> +		};
> +
> +		watchdog0: watchdog@80940000 {
> +			compatible = "cirrus,ep9301-wdt";
> +			reg = <0x80940000 0x08>;
> +		};
> +	};
> +
> +	xtali: oscillator {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <14745600>;
> +		clock-output-names = "xtali";
> +	};
> +
> +	i2c0: i2c {
> +		compatible = "i2c-gpio";
> +		sda-gpios = <&gpio6 1 (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>;
> +		scl-gpios = <&gpio6 0 (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>;

Are you sure this is part of SoC? It is rather unusual... I would say
this would be the first SoC, where GPIO pins must be an I2C.



Best regards,
Krzysztof

Re: [PATCH v3 07/42] soc: Add SoC driver for Cirrus ep93xx

[Andy Shevchenko]

On Thu, Jul 20, 2023 at 02:29:07PM +0300, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
> 
> This adds an SoC driver for the ep93xx. Currently there
> is only one thing not fitting into any other framework,
> and that is the swlock setting.
> 
> It's used for clock settings and restart.

> +#include <linux/clk-provider.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/of.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/sys_soc.h>
> +#include <linux/soc/cirrus/ep93xx.h>

...

> +#define EP93XX_SYSCON_SYSCFG_REV_MASK	0xf0000000

GENMASK() (you will need bits.h, and looking below you seem missed it anyway)

...

> +	spin_lock_irqsave(&ep93xx_swlock, flags);
> +
> +	regmap_read(map, EP93XX_SYSCON_DEVCFG, &val);
> +	val &= ~clear_bits;
> +	val |= set_bits;
> +	regmap_write(map, EP93XX_SYSCON_SWLOCK, EP93XX_SWLOCK_MAGICK);
> +	regmap_write(map, EP93XX_SYSCON_DEVCFG, val);

Is this sequence a must?
I.o.w. can you first supply magic and then update devcfg?

> +	spin_unlock_irqrestore(&ep93xx_swlock, flags);

...

> +void ep93xx_swlocked_update_bits(struct regmap *map, unsigned int reg,
> +				 unsigned int mask, unsigned int val)
> +{

Same Q as above.

> +}

...

> +	int rev = ep93xx_chip_revision(map);
> +
> +	switch (rev) {

	switch(ep93xx_chip_revision(map))

?

> +	case EP93XX_CHIP_REV_D0:
> +		return "D0";
> +	case EP93XX_CHIP_REV_D1:
> +		return "D1";
> +	case EP93XX_CHIP_REV_E0:
> +		return "E0";
> +	case EP93XX_CHIP_REV_E1:
> +		return "E1";
> +	case EP93XX_CHIP_REV_E2:
> +		return "E2";
> +	default:
> +		return "unknown";
> +	}

...

> +static unsigned long __init calc_pll_rate(u64 rate, u32 config_word)
> +{
> +	rate *= ((config_word >> 11) & GENMASK(4, 0)) + 1;	/* X1FBD */
> +	rate *= ((config_word >> 5) & GENMASK(5, 0)) + 1;	/* X2FBD */
> +	do_div(rate, (config_word & GENMASK(4, 0)) + 1);	/* X2IPD */
> +	rate >>= ((config_word >> 16) & 3);			/* PS */

GENMASK()

> +	return rate;
> +}

...

> +	/* Multiplatform guard, only proceed on ep93xx */
> +	if (!of_machine_is_compatible("cirrus,ep9301"))
> +		return 0;

Why?

> +	map = syscon_regmap_lookup_by_compatible("cirrus,ep9301-syscon");
> +	if (IS_ERR(map))
> +		return PTR_ERR(map);

Is this not enough?

...

> +	if (!(value & EP93XX_SYSCON_CLKSET1_NBYP1))
> +		clk_pll1_rate = EP93XX_EXT_CLK_RATE;
> +	else
> +		clk_pll1_rate = calc_pll_rate(EP93XX_EXT_CLK_RATE, value);

Positive conditionals in if-else are easier to be read.

...

> +	clk_f_div = fclk_divisors[(value >> 25) & 0x7];
> +	clk_h_div = hclk_divisors[(value >> 20) & 0x7];
> +	clk_p_div = pclk_divisors[(value >> 18) & 0x3];

GENMASK() in all three?

...

> +	np = of_find_node_by_path("/");
> +	of_property_read_string(np, "model", &machine);
> +	if (machine)
> +		attrs->machine = kstrdup(machine, GFP_KERNEL);

No error check?

> +	of_node_put(np);

Looks like NIH of_flat_dt_get_machine_name(). Can you rather make use of it as
it's done, e.g., here arch/riscv/kernel/setup.c:251?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 35/42] ARM: dts: ep93xx: add ts7250 board

[Krzysztof Kozlowski]

On 20/07/2023 13:29, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
> 
> Add device tree file for Technologic Systems ts7250 board and
> Liebherr bk3 board which have many in common, both are based on
> ep9302 SoC variant.
> 
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
> ---
>  arch/arm/boot/dts/cirrus/Makefile          |   3 +
>  arch/arm/boot/dts/cirrus/ep93xx-bk3.dts    | 126 +++++++++++++++++++++++++
>  arch/arm/boot/dts/cirrus/ep93xx-ts7250.dts | 145 +++++++++++++++++++++++++++++
>  3 files changed, 274 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/cirrus/Makefile b/arch/arm/boot/dts/cirrus/Makefile
> index e944d3e2129d..211a7e2f2115 100644
> --- a/arch/arm/boot/dts/cirrus/Makefile
> +++ b/arch/arm/boot/dts/cirrus/Makefile
> @@ -3,3 +3,6 @@ dtb-$(CONFIG_ARCH_CLPS711X) += \
>  	ep7211-edb7211.dtb
>  dtb-$(CONFIG_ARCH_CLPS711X) += \
>  	ep7211-edb7211.dtb
> +dtb-$(CONFIG_ARCH_EP93XX) += \
> +	ep93xx-bk3.dtb \
> +	ep93xx-ts7250.dtb
> diff --git a/arch/arm/boot/dts/cirrus/ep93xx-bk3.dts b/arch/arm/boot/dts/cirrus/ep93xx-bk3.dts
> new file mode 100644
> index 000000000000..91d76a1a8571
> --- /dev/null
> +++ b/arch/arm/boot/dts/cirrus/ep93xx-bk3.dts
> @@ -0,0 +1,126 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Device Tree file for Liebherr controller BK3.1 based on Cirrus EP9302 SoC
> + */
> +/dts-v1/;
> +#include "ep93xx.dtsi"
> +
> +/ {
> +	model = "Liebherr controller BK3.1";
> +	compatible = "liebherr,bk3", "cirrus,ep9301";
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +
> +	chosen {
> +	};
> +
> +	memory@0 {
> +		device_type = "memory";
> +		/* should be set from ATAGS */
> +		reg = <0x00000000 0x02000000>,
> +		      <0x000530c0 0x01fdd000>;
> +	};
> +
> +	nand-controller@60000000 {
> +		compatible = "technologic,ts7200-nand";
> +		reg = <0x60000000 0x8000000>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		nand@0 {
> +			reg = <0>;
> +			partitions {
> +				compatible = "fixed-partitions";
> +				#address-cells = <1>;
> +				#size-cells = <1>;
> +
> +				partition@0 {
> +					label = "System";
> +					reg = <0x00000000 0x01e00000>;
> +					read-only;
> +				};
> +
> +				partition@1e00000 {
> +					label = "Data";
> +					reg = <0x01e00000 0x05f20000>;
> +				};
> +
> +				partition@7d20000 {
> +					label = "RedBoot";
> +					reg = <0x07d20000 0x002e0000>;
> +					read-only;
> +				};
> +			};
> +		};
> +	};
> +
> +	leds {
> +		compatible = "gpio-leds";
> +		led-0 {
> +			label = "grled";
> +			gpios = <&gpio4 0 GPIO_ACTIVE_HIGH>;
> +			linux,default-trigger = "heartbeat";
> +			function = LED_FUNCTION_HEARTBEAT;
> +		};
> +
> +		led-1 {
> +			label = "rdled";
> +			gpios = <&gpio4 1 GPIO_ACTIVE_HIGH>;
> +			function = LED_FUNCTION_FAULT;
> +		};
> +	};
> +};
> +
> +&eth0 {
> +	phy-handle = <&phy0>;
> +};
> +
> +&i2c0 {
> +	status = "okay";
> +};
> +
> +&i2s {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&i2s_on_ac97_pins>;
> +	status = "okay";
> +};
> +
> +&gpio1 {
> +	/* PWM */
> +	gpio-ranges = <&pinctrl 6 163 1>;
> +};
> +
> +&gpio4 {
> +	gpio-ranges = <&pinctrl 0 97 2>;
> +	status = "okay";
> +};
> +
> +&gpio6 {
> +	gpio-ranges = <&pinctrl 0 87 2>;
> +	status = "okay";
> +};
> +
> +&gpio7 {
> +	gpio-ranges = <&pinctrl 2 199 4>;
> +	status = "okay";
> +};
> +
> +&mdio0 {
> +	phy0: ethernet-phy@1 {
> +		reg = <1>;
> +		device_type = "ethernet-phy";
> +	};
> +};
> +
> +&uart0 {
> +	status = "okay";
> +};
> +
> +&uart1 {
> +	status = "okay";
> +};
> +
> +&usb0 {
> +	status = "okay";
> +};
> +
> diff --git a/arch/arm/boot/dts/cirrus/ep93xx-ts7250.dts b/arch/arm/boot/dts/cirrus/ep93xx-ts7250.dts
> new file mode 100644
> index 000000000000..625202f8cd25
> --- /dev/null
> +++ b/arch/arm/boot/dts/cirrus/ep93xx-ts7250.dts
> @@ -0,0 +1,145 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Device Tree file for Technologic Systems ts7250 board based on Cirrus EP9302 SoC
> + */
> +/dts-v1/;
> +#include "ep93xx.dtsi"
> +#include <dt-bindings/dma/cirrus,ep93xx-dma.h>
> +
> +/ {
> +	compatible = "technologic,ts7250", "cirrus,ep9301";
> +	model = "TS-7250 SBC";
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +
> +	chosen {
> +	};
> +
> +	memory@0 {
> +		device_type = "memory";
> +		/* should be set from ATAGS */
> +		reg = <0x00000000 0x02000000>,
> +		      <0x000530c0 0x01fdd000>;
> +	};
> +
> +	nand-controller@60000000 {

Where is this address? It does not work like that. If this is part of
SoC, then should be in DTSI and part of soc node. If not, then it is
some other bus which needs some description. Top-level is not a bus.

You should see errors when testing dtbs with W=1.


> +		compatible = "technologic,ts7200-nand";
> +		reg = <0x60000000 0x8000000>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		nand@0 {
> +			reg = <0>;
> +			partitions {
> +				compatible = "fixed-partitions";
> +				#address-cells = <1>;
> +				#size-cells = <1>;
> +
> +				partition@0 {
> +					label = "TS-BOOTROM";
> +					reg = <0x00000000 0x00020000>;
> +					read-only;
> +				};
> +
> +				partition@20000 {
> +					label = "Linux";
> +					reg = <0x00020000 0x07d00000>;
> +				};
> +
> +				partition@7d20000 {
> +					label = "RedBoot";
> +					reg = <0x07d20000 0x002e0000>;
> +					read-only;
> +				};
> +			};
> +		};
> +	};
> +
> +	rtc1: rtc@10800000 {

Same problems

> +		compatible = "st,m48t86";
> +		reg = <0x10800000 0x1>,
> +		      <0x11700000 0x1>;
> +	};
> +
> +	watchdog1: watchdog@23800000 {

Same problems

> +		compatible = "technologic,ts7200-wdt";
> +		reg = <0x23800000 0x01>,
> +		      <0x23c00000 0x01>;
> +		timeout-sec = <30>;
> +	};
> +
> +	leds {
> +		compatible = "gpio-leds";
> +		led-0 {
> +			label = "grled";
> +			gpios = <&gpio4 0 GPIO_ACTIVE_HIGH>;
> +			linux,default-trigger = "heartbeat";
> +			function = LED_FUNCTION_HEARTBEAT;
> +		};
> +
> +		led-1 {
> +			label = "rdled";
> +			gpios = <&gpio4 1 GPIO_ACTIVE_HIGH>;
> +			function = LED_FUNCTION_FAULT;
> +		};
> +	};
> +};
> +
> +&eth0 {
> +	phy-handle = <&phy0>;
> +};
> +
> +&gpio1 {
> +	/* PWM */
> +	gpio-ranges = <&pinctrl 6 163 1>;
> +};
> +
> +&gpio4 {
> +	gpio-ranges = <&pinctrl 0 97 2>;
> +	status = "okay";
> +};
> +
> +&gpio6 {
> +	gpio-ranges = <&pinctrl 0 87 2>;
> +	status = "okay";
> +};
> +
> +&gpio7 {
> +	gpio-ranges = <&pinctrl 2 199 4>;
> +	status = "okay";
> +};
> +
> +&i2c0 {
> +	status = "okay";
> +};
> +
> +&spi0 {
> +	cs-gpios = <&gpio5 2 GPIO_ACTIVE_HIGH>;
> +	dmas = <&dma1 EP93XX_DMA_SSP>;
> +	status = "okay";
> +
> +	tmp122_spi: tmp122@0 {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation


> +		compatible = "ti,tmp122";
> +		reg = <0>;
> +		spi-max-frequency = <2000000>;
> +	};
> +};
> +


Best regards,
Krzysztof

Re: [PATCH v3 38/42] ata: pata_ep93xx: remove legacy pinctrl use

[Andy Shevchenko]

On Thu, Jul 20, 2023 at 02:29:38PM +0300, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
> 
> Drop legacy acquire/release since we are using pinctrl for this now.

...

>  	drv_data = devm_kzalloc(&pdev->dev, sizeof(*drv_data), GFP_KERNEL);
> -	if (!drv_data) {
> -		err = -ENOMEM;
> -		goto err_rel_gpio;
> -	}
> +	if (!drv_data)
> +		return -ENXIO;

Wrong error code. See above what you have deleted.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 41/42] ARM: dts: ep93xx: Add EDB9302 DT

[Krzysztof Kozlowski]

On 20/07/2023 13:29, Nikita Shubin via B4 Relay wrote:
> From: Alexander Sverdlin <alexander.sverdlin@gmail.com>
> 
> Add device tree for Cirrus EDB9302.
> 
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@gmail.com>
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
> ---
>  arch/arm/boot/dts/cirrus/Makefile           |   1 +
>  arch/arm/boot/dts/cirrus/ep93xx-edb9302.dts | 178 ++++++++++++++++++++++++++++
>  2 files changed, 179 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/cirrus/Makefile b/arch/arm/boot/dts/cirrus/Makefile
> index 211a7e2f2115..e6015983e464 100644
> --- a/arch/arm/boot/dts/cirrus/Makefile
> +++ b/arch/arm/boot/dts/cirrus/Makefile
> @@ -4,5 +4,6 @@ dtb-$(CONFIG_ARCH_CLPS711X) += \
>  dtb-$(CONFIG_ARCH_CLPS711X) += \
>  	ep7211-edb7211.dtb
>  dtb-$(CONFIG_ARCH_EP93XX) += \
> +	ep93xx-edb9302.dtb \
>  	ep93xx-bk3.dtb \
>  	ep93xx-ts7250.dtb
> diff --git a/arch/arm/boot/dts/cirrus/ep93xx-edb9302.dts b/arch/arm/boot/dts/cirrus/ep93xx-edb9302.dts
> new file mode 100644
> index 000000000000..b048fd131aa5
> --- /dev/null
> +++ b/arch/arm/boot/dts/cirrus/ep93xx-edb9302.dts
> @@ -0,0 +1,178 @@
> +// SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +/*
> + * Device Tree file for Cirrus Logic EDB9302 board based on EP9302 SoC
> + */
> +/dts-v1/;
> +#include "ep93xx.dtsi"
> +#include <dt-bindings/dma/cirrus,ep93xx-dma.h>
> +
> +/ {
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +	compatible = "cirrus,edb9302", "cirrus,ep9301";
> +	model = "cirrus,edb9302";
> +
> +	chosen {
> +	};
> +
> +	memory@0 {
> +		device_type = "memory";
> +		/* should be set from ATAGS */
> +		reg = <0x0000000 0x800000>,
> +		      <0x1000000 0x800000>,
> +		      <0x4000000 0x800000>,
> +		      <0x5000000 0x800000>;
> +	};
> +
> +	flash@60000000 {

Same problems - it's root, not soc bus.

> +		compatible = "cfi-flash";
> +		reg = <0x60000000 0x1000000>;
> +		bank-width = <2>;
> +	};
> +

Best regards,
Krzysztof

Re: [PATCH v3 07/42] soc: Add SoC driver for Cirrus ep93xx

[Krzysztof Kozlowski]

On 20/07/2023 13:29, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
> 
> This adds an SoC driver for the ep93xx. Currently there
> is only one thing not fitting into any other framework,
> and that is the swlock setting.
> 
> It's used for clock settings and restart.
> 
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
> Tested-by: Alexander Sverdlin <alexander.sverdlin@gmail.com>
> Acked-by: Alexander Sverdlin <alexander.sverdlin@gmail.com>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/soc/Kconfig               |   1 +
>  drivers/soc/Makefile              |   1 +
>  drivers/soc/cirrus/Kconfig        |  12 ++
>  drivers/soc/cirrus/Makefile       |   2 +
>  drivers/soc/cirrus/soc-ep93xx.c   | 231 ++++++++++++++++++++++++++++++++++++++
>  include/linux/soc/cirrus/ep93xx.h |  18 ++-
>  6 files changed, 264 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
> index 4e176280113a..16327b63b773 100644
> --- a/drivers/soc/Kconfig
> +++ b/drivers/soc/Kconfig
> @@ -8,6 +8,7 @@ source "drivers/soc/aspeed/Kconfig"
>  source "drivers/soc/atmel/Kconfig"
>  source "drivers/soc/bcm/Kconfig"
>  source "drivers/soc/canaan/Kconfig"
> +source "drivers/soc/cirrus/Kconfig"
>  source "drivers/soc/fsl/Kconfig"
>  source "drivers/soc/fujitsu/Kconfig"
>  source "drivers/soc/imx/Kconfig"
> diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
> index 3b0f9fb3b5c8..b76a03fe808e 100644
> --- a/drivers/soc/Makefile
> +++ b/drivers/soc/Makefile
> @@ -9,6 +9,7 @@ obj-y				+= aspeed/
>  obj-$(CONFIG_ARCH_AT91)		+= atmel/
>  obj-y				+= bcm/
>  obj-$(CONFIG_SOC_CANAAN)	+= canaan/
> +obj-$(CONFIG_EP93XX_SOC)        += cirrus/
>  obj-$(CONFIG_ARCH_DOVE)		+= dove/
>  obj-$(CONFIG_MACH_DOVE)		+= dove/
>  obj-y				+= fsl/
> diff --git a/drivers/soc/cirrus/Kconfig b/drivers/soc/cirrus/Kconfig
> new file mode 100644
> index 000000000000..408f3343a265
> --- /dev/null
> +++ b/drivers/soc/cirrus/Kconfig
> @@ -0,0 +1,12 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +if ARCH_EP93XX
> +
> +config EP93XX_SOC
> +	bool "Cirrus EP93xx chips SoC"
> +	select SOC_BUS
> +	default y if !EP93XX_SOC_COMMON
> +	help
> +	  Support SoC for Cirrus EP93xx chips.
> +
> +endif
> diff --git a/drivers/soc/cirrus/Makefile b/drivers/soc/cirrus/Makefile
> new file mode 100644
> index 000000000000..ed6752844c6f
> --- /dev/null
> +++ b/drivers/soc/cirrus/Makefile
> @@ -0,0 +1,2 @@
> +# SPDX-License-Identifier: GPL-2.0
> +obj-y	+= soc-ep93xx.o
> diff --git a/drivers/soc/cirrus/soc-ep93xx.c b/drivers/soc/cirrus/soc-ep93xx.c
> new file mode 100644
> index 000000000000..2fd48d900f24
> --- /dev/null
> +++ b/drivers/soc/cirrus/soc-ep93xx.c
> @@ -0,0 +1,231 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * SoC driver for Cirrus EP93xx chips.
> + * Copyright (C) 2022 Nikita Shubin <nikita.shubin@maquefel.me>
> + *
> + * Based on a rewrite of arch/arm/mach-ep93xx/core.c
> + * Copyright (C) 2006 Lennert Buytenhek <buytenh@wantstofly.org>
> + * Copyright (C) 2007 Herbert Valerio Riedel <hvr@gnu.org>
> + *
> + * Thanks go to Michael Burian and Ray Lehtiniemi for their key
> + * role in the ep93xx Linux community
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/of.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/sys_soc.h>
> +#include <linux/soc/cirrus/ep93xx.h>
> +
> +#define EP93XX_EXT_CLK_RATE		14745600
> +
> +#define EP93XX_SYSCON_DEVCFG		0x80
> +
> +#define EP93XX_SWLOCK_MAGICK		0xaa
> +#define EP93XX_SYSCON_SWLOCK		0xc0
> +#define EP93XX_SYSCON_SYSCFG		0x9c
> +#define EP93XX_SYSCON_SYSCFG_REV_MASK	0xf0000000
> +#define EP93XX_SYSCON_SYSCFG_REV_SHIFT	28
> +
> +#define EP93XX_SYSCON_CLKSET1		0x20
> +#define EP93XX_SYSCON_CLKSET1_NBYP1	BIT(23)
> +#define EP93XX_SYSCON_CLKSET2		0x24
> +#define EP93XX_SYSCON_CLKSET2_NBYP2	BIT(19)
> +#define EP93XX_SYSCON_CLKSET2_PLL2_EN	BIT(18)
> +
> +static DEFINE_SPINLOCK(ep93xx_swlock);
> +
> +/* EP93xx System Controller software locked register write */
> +void ep93xx_syscon_swlocked_write(struct regmap *map, unsigned int reg, unsigned int val)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&ep93xx_swlock, flags);
> +
> +	regmap_write(map, EP93XX_SYSCON_SWLOCK, EP93XX_SWLOCK_MAGICK);
> +	regmap_write(map, reg, val);
> +
> +	spin_unlock_irqrestore(&ep93xx_swlock, flags);
> +}
> +EXPORT_SYMBOL_NS_GPL(ep93xx_syscon_swlocked_write, EP93XX_SOC);

I doubt that your code compiles. Didn't you add a user of this in some
earlier patch?

Anyway, no, drop it, don't export some weird calls from core initcall to
drivers. You violate layering and driver encapsulation. There is no
dependency/probe ordering.

There is no even need for this, because this code does not use it!

> +
> +void ep93xx_devcfg_set_clear(struct regmap *map, unsigned int set_bits, unsigned int clear_bits)
> +{
> +	unsigned long flags;
> +	unsigned int val;
> +
> +	spin_lock_irqsave(&ep93xx_swlock, flags);
> +
> +	regmap_read(map, EP93XX_SYSCON_DEVCFG, &val);
> +	val &= ~clear_bits;
> +	val |= set_bits;
> +	regmap_write(map, EP93XX_SYSCON_SWLOCK, EP93XX_SWLOCK_MAGICK);
> +	regmap_write(map, EP93XX_SYSCON_DEVCFG, val);
> +
> +	spin_unlock_irqrestore(&ep93xx_swlock, flags);
> +}
> +EXPORT_SYMBOL_NS_GPL(ep93xx_devcfg_set_clear, EP93XX_SOC);

No.

> +
> +void ep93xx_swlocked_update_bits(struct regmap *map, unsigned int reg,
> +				 unsigned int mask, unsigned int val)
> +{
> +	unsigned long flags;
> +	unsigned int tmp, orig;
> +
> +	spin_lock_irqsave(&ep93xx_swlock, flags);
> +
> +	regmap_read(map, EP93XX_SYSCON_DEVCFG, &orig);
> +	tmp = orig & ~mask;
> +	tmp |= val & mask;
> +	if (tmp != orig) {
> +		regmap_write(map, EP93XX_SYSCON_SWLOCK, EP93XX_SWLOCK_MAGICK);
> +		regmap_write(map, reg, tmp);
> +	}
> +
> +	spin_unlock_irqrestore(&ep93xx_swlock, flags);
> +}
> +EXPORT_SYMBOL_NS_GPL(ep93xx_swlocked_update_bits, EP93XX_SOC);

No.


> +
> +unsigned int __init ep93xx_chip_revision(struct regmap *map)

Why this is visible outside? This should be static.


> +{
> +	unsigned int val;
> +
> +	regmap_read(map, EP93XX_SYSCON_SYSCFG, &val);
> +	val &= EP93XX_SYSCON_SYSCFG_REV_MASK;
> +	val >>= EP93XX_SYSCON_SYSCFG_REV_SHIFT;
> +	return val;
> +}


> +
> +static const char __init *ep93xx_get_soc_rev(struct regmap *map)
> +{
> +	int rev = ep93xx_chip_revision(map);
> +
> +	switch (rev) {
> +	case EP93XX_CHIP_REV_D0:
> +		return "D0";
> +	case EP93XX_CHIP_REV_D1:
> +		return "D1";
> +	case EP93XX_CHIP_REV_E0:
> +		return "E0";
> +	case EP93XX_CHIP_REV_E1:
> +		return "E1";
> +	case EP93XX_CHIP_REV_E2:
> +		return "E2";
> +	default:
> +		return "unknown";
> +	}
> +}
> +
> +/*
> + * PLL rate = 14.7456 MHz * (X1FBD + 1) * (X2FBD + 1) / (X2IPD + 1) / 2^PS
> + */
> +static unsigned long __init calc_pll_rate(u64 rate, u32 config_word)
> +{
> +	rate *= ((config_word >> 11) & GENMASK(4, 0)) + 1;	/* X1FBD */
> +	rate *= ((config_word >> 5) & GENMASK(5, 0)) + 1;	/* X2FBD */
> +	do_div(rate, (config_word & GENMASK(4, 0)) + 1);	/* X2IPD */
> +	rate >>= ((config_word >> 16) & 3);			/* PS */
> +
> +	return rate;
> +}
> +
> +static int __init ep93xx_soc_init(void)
> +{
> +	struct soc_device_attribute *attrs;
> +	struct soc_device *soc_dev;
> +	struct device_node *np;
> +	struct regmap *map;
> +	struct clk_hw *hw;
> +	unsigned long clk_pll1_rate, clk_pll2_rate;
> +	unsigned int clk_f_div, clk_h_div, clk_p_div, clk_usb_div;
> +	const char fclk_divisors[] = { 1, 2, 4, 8, 16, 1, 1, 1 };
> +	const char hclk_divisors[] = { 1, 2, 4, 5, 6, 8, 16, 32 };
> +	const char pclk_divisors[] = { 1, 2, 4, 8 };
> +	const char *machine = NULL;
> +	u32 value;
> +
> +	/* Multiplatform guard, only proceed on ep93xx */
> +	if (!of_machine_is_compatible("cirrus,ep9301"))
> +		return 0;

This should already be a warning sign for you...

> +
> +	map = syscon_regmap_lookup_by_compatible("cirrus,ep9301-syscon");
> +	if (IS_ERR(map))
> +		return PTR_ERR(map);

No, not-reusable. Use devices and device nodes.

> +
> +	/* Determine the bootloader configured pll1 rate */
> +	regmap_read(map, EP93XX_SYSCON_CLKSET1, &value);
> +	if (!(value & EP93XX_SYSCON_CLKSET1_NBYP1))
> +		clk_pll1_rate = EP93XX_EXT_CLK_RATE;
> +	else
> +		clk_pll1_rate = calc_pll_rate(EP93XX_EXT_CLK_RATE, value);
> +
> +	hw = clk_hw_register_fixed_rate(NULL, "pll1", "xtali", 0, clk_pll1_rate);
> +	if (IS_ERR(hw))
> +		return PTR_ERR(hw);
> +
> +	/* Initialize the pll1 derived clocks */
> +	clk_f_div = fclk_divisors[(value >> 25) & 0x7];
> +	clk_h_div = hclk_divisors[(value >> 20) & 0x7];
> +	clk_p_div = pclk_divisors[(value >> 18) & 0x3];
> +
> +	hw = clk_hw_register_fixed_factor(NULL, "fclk", "pll1", 0, 1, clk_f_div);
> +	if (IS_ERR(hw))
> +		return PTR_ERR(hw);
> +
> +	hw = clk_hw_register_fixed_factor(NULL, "hclk", "pll1", 0, 1, clk_h_div);
> +	if (IS_ERR(hw))
> +		return PTR_ERR(hw);
> +
> +	hw = clk_hw_register_fixed_factor(NULL, "pclk", "hclk", 0, 1, clk_p_div);
> +	if (IS_ERR(hw))
> +		return PTR_ERR(hw);
> +
> +	/* Determine the bootloader configured pll2 rate */
> +	regmap_read(map, EP93XX_SYSCON_CLKSET2, &value);
> +	if (!(value & EP93XX_SYSCON_CLKSET2_NBYP2))
> +		clk_pll2_rate = EP93XX_EXT_CLK_RATE;
> +	else if (value & EP93XX_SYSCON_CLKSET2_PLL2_EN)
> +		clk_pll2_rate = calc_pll_rate(EP93XX_EXT_CLK_RATE, value);
> +	else
> +		clk_pll2_rate = 0;
> +
> +	hw = clk_hw_register_fixed_rate(NULL, "pll2", "xtali", 0, clk_pll2_rate);
> +	if (IS_ERR(hw))
> +		return PTR_ERR(hw);
> +
> +	regmap_read(map, EP93XX_SYSCON_CLKSET2, &value);
> +	clk_usb_div = (((value >> 28) & GENMASK(3, 0)) + 1);
> +	hw = clk_hw_register_fixed_factor(NULL, "usb_clk", "pll2", 0, 1, clk_usb_div);
> +	if (IS_ERR(hw))
> +		return PTR_ERR(hw);
> +
> +	attrs = kzalloc(sizeof(*attrs), GFP_KERNEL);
> +	if (!attrs)
> +		return -ENOMEM;
> +
> +	np = of_find_node_by_path("/");
> +	of_property_read_string(np, "model", &machine);
> +	if (machine)
> +		attrs->machine = kstrdup(machine, GFP_KERNEL);
> +	of_node_put(np);
> +
> +	attrs->family = "Cirrus Logic EP93xx";
> +	attrs->revision = ep93xx_get_soc_rev(map);
> +
> +	soc_dev = soc_device_register(attrs);
> +	if (IS_ERR(soc_dev)) {
> +		kfree(attrs->soc_id);
> +		kfree(attrs->serial_number);
> +		kfree(attrs);
> +		return PTR_ERR(soc_dev);
> +	}
> +
> +	pr_info("EP93xx SoC revision %s\n", attrs->revision);
> +
> +	return 0;
> +}
> +core_initcall(ep93xx_soc_init);

That's not the way to add soc driver. You need a proper driver for it

Best regards,
Krzysztof

Re: [PATCH v3 00/42] ep93xx device tree conversion

[Krzysztof Kozlowski]

On 20/07/2023 13:29, Nikita Shubin via B4 Relay wrote:
> This series aims to convert ep93xx from platform to full device tree support.
> 
> The main goal is to receive ACK's to take it via Arnd's arm-soc branch.
> 

This approach makes patchset trickier to review with absolutely huge
Cc-list and inter-dependencies. I don't think this is correct approach.
This should be split per subsystem whenever possible.

Expect more grunts and complains from 50-other people you Cc-ed.

Best regards,
Krzysztof

Re: [PATCH v3 05/42] pinctrl: add a Cirrus ep93xx SoC pin controller

[Andy Shevchenko]

On Thu, Jul 20, 2023 at 02:29:05PM +0300, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
> 
> This adds a pin control (only multiplexing) driver for ep93xx
> SoC so we can fully convert ep93xx to device tree.
> 
> This driver is capable of muxing ep9301/ep9302/ep9307/ep9312/ep9315
> variants, this is chosen based on "compatible" in device tree.

...

> +config PINCTRL_EP93XX
> +	bool
> +	depends on OF && (ARCH_EP93XX || COMPILE_TEST)

The OF seems to be functional dependency and not compile time one.
Thus

	depends on (OF && ARCH_EP93XX) || COMPILE_TEST

> +	select PINMUX
> +	select GENERIC_PINCONF
> +	select MFD_SYSCON

...

> +#define EP93XX_SYSCON_DEVCFG_D1ONG	BIT(30) /* not used */
> +#define EP93XX_SYSCON_DEVCFG_D0ONG	BIT(29) /* not used */
> +#define EP93XX_SYSCON_DEVCFG_IONU2	BIT(28) /* not used */
> +#define EP93XX_SYSCON_DEVCFG_GONK	BIT(27) /* done */
> +#define EP93XX_SYSCON_DEVCFG_TONG	BIT(26) /* not used */
> +#define EP93XX_SYSCON_DEVCFG_MONG	BIT(25) /* not used */
> +#define EP93XX_SYSCON_DEVCFG_A2ONG	BIT(22) /* not used */
> +#define EP93XX_SYSCON_DEVCFG_A1ONG	BIT(21) /* not used */
> +#define EP93XX_SYSCON_DEVCFG_HONIDE	BIT(11) /* done */
> +#define EP93XX_SYSCON_DEVCFG_GONIDE	BIT(10) /* done */
> +#define EP93XX_SYSCON_DEVCFG_PONG	BIT(9) /* done */
> +#define EP93XX_SYSCON_DEVCFG_EONIDE	BIT(8) /* done */
> +#define EP93XX_SYSCON_DEVCFG_I2SONSSP	BIT(7) /* done */
> +#define EP93XX_SYSCON_DEVCFG_I2SONAC97	BIT(6) /* done */
> +#define EP93XX_SYSCON_DEVCFG_RASONP3	BIT(4) /* done */

> +#define PADS_MASK		(GENMASK(30, 25) | BIT(22) | BIT(21) | GENMASK(11, 6) | BIT(4))

Seems better to spell each bit as by definition given above.

...

> +/* Ordered by bit index */
> +static const char * const ep93xx_padgroups[] = {
> +	NULL, NULL, NULL, NULL,
> +	"RasOnP3",
> +	NULL,
> +	"I2SonAC97",
> +	"I2SonSSP",
> +	"EonIDE",
> +	"PonG",
> +	"GonIDE",
> +	"HonIDE",
> +	NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
> +	"A1onG",
> +	"A2onG",
> +	NULL, NULL,
> +	"MonG",
> +	"TonG",
> +	"GonK",
> +	"IonU2",
> +	"D0onG",
> +	"D1onG",

Instead of tons of NULLs, use

	[NN] = "...",

which is much less error prone.

> +};

...

> +/** ep9301, ep9302*/

Is it really kernel doc (besides missing space)?
Please, run

	scripts/kernel-doc -v -Wall -none $YOUR_FILE

for each file you contributed in the entire series and fix all warnings.

...

> +static const char *ep93xx_get_group_name(struct pinctrl_dev *pctldev,
> +					 unsigned int selector)
> +{
> +	struct ep93xx_pmx *pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> +	switch (pmx->model) {
> +	case EP93XX_9301_PINCTRL:
> +		return ep9301_pin_groups[selector].grp.name;
> +	case EP93XX_9307_PINCTRL:
> +		return ep9307_pin_groups[selector].grp.name;
> +	case EP93XX_9312_PINCTRL:
> +		return ep9312_pin_groups[selector].grp.name;
> +	}

> +	return NULL;

Use default case instead.

> +}

...

> +	/* Which bits changed */
> +	before &= PADS_MASK;
> +	after &= PADS_MASK;
> +	expected = before & ~grp->mask;
> +	expected |= grp->value;
> +	expected &= PADS_MASK;

Instead of above:

	expected = (before & ~grp->mask) | grp->value;

> +	/* Print changed states */
> +	tmp = expected ^ after;

	tmp = (expected ^ after) & PADS_MASK;

> +	for_each_set_bit(i, &tmp, PADS_MAXBIT) {
> +		bool enabled = expected & BIT(i);
> +
> +		dev_err(pmx->dev,
> +			    "pin group %s could not be %s: probably a hardware limitation\n",
> +			    ep93xx_padgroups[i], str_enabled_disabled(enabled));

Wrong indentation.

> +		dev_err(pmx->dev,
> +				"DeviceCfg before: %08x, after %08x, expected %08x\n",
> +				before, after, expected);

Wrong indentation.

I believe this one can go to debug level.

> +	}

...

> +	pmx->model = (int)of_device_get_match_data(dev);

(uintptr_t) is more appropriate here.

...

> +	pmx->pctl = devm_pinctrl_register(dev, &ep93xx_pmx_desc, pmx);
> +	if (IS_ERR(pmx->pctl)) {
> +		dev_err(dev, "could not register pinmux driver\n");
> +		return PTR_ERR(pmx->pctl);

Why not dev_err_probe() as elsewhere?

> +	}

...

> +static int __init ep93xx_pmx_init(void)
> +{
> +	return platform_driver_register(&ep93xx_pmx_driver);
> +}
> +arch_initcall(ep93xx_pmx_init);

+ blank line.

Also add everywhere MODULE_DESCRIPTION() as modpost recently started to
complain (probably with `make W=1` which you should execute anyway for
your new code).

> +MODULE_IMPORT_NS(EP93XX_SOC);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 09/42] clocksource: ep93xx: Add driver for Cirrus Logic EP93xx

[Andy Shevchenko]

On Thu, Jul 20, 2023 at 02:29:09PM +0300, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
> 
> This us a rewrite of EP93xx timer driver in
> arch/arm/mach-ep93xx/timer-ep93xx.c trying to do everything
> the device tree way:
> 
> - Make every IO-access relative to a base address and dynamic
>   so we can do a dynamic ioremap and get going.
> - Find register range and interrupt from the device tree.

...

+ bits.h

> +#include <linux/clockchips.h>
> +#include <linux/clocksource.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/io-64-nonatomic-lo-hi.h>
> +#include <linux/irq.h>
> +#include <linux/kernel.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/sched_clock.h>

...

> +/*************************************************************************

Won't you marc it as a DOC: section?

> + * Timer handling for EP93xx
> + *************************************************************************
> + * The ep93xx has four internal timers.  Timers 1, 2 (both 16 bit) and
> + * 3 (32 bit) count down at 508 kHz, are self-reloading, and can generate
> + * an interrupt on underflow.  Timer 4 (40 bit) counts down at 983.04 kHz,
> + * is free-running, and can't generate interrupts.
> + *
> + * The 508 kHz timers are ideal for use for the timer interrupt, as the
> + * most common values of HZ divide 508 kHz nicely.  We pick the 32 bit
> + * timer (timer 3) to get as long sleep intervals as possible when using
> + * CONFIG_NO_HZ.
> + *
> + * The higher clock rate of timer 4 makes it a better choice than the
> + * other timers for use as clock source and for sched_clock(), providing
> + * a stable 40 bit time base.
> + *************************************************************************
> + */

...

> +/*
> + * This read-only register contains the low word of the time stamp debug timer
> + * ( Timer4). When this register is read, the high byte of the Timer4 counter is

One too many spaces.

> + * saved in the Timer4ValueHigh register.
> + */

...

> +static irqreturn_t ep93xx_timer_interrupt(int irq, void *dev_id)
> +{
> +	struct ep93xx_tcu *tcu = ep93xx_tcu;
> +	struct clock_event_device *evt = dev_id;
> +
> +	/* Writing any value clears the timer interrupt */
> +	writel(1, tcu->base + EP93XX_TIMER3_CLEAR);

Would 0 suffice?

> +	evt->event_handler(evt);
> +
> +	return IRQ_HANDLED;
> +}

...

> +static int __init ep93xx_timer_of_init(struct device_node *np)
> +{
> +	int irq;
> +	unsigned long flags = IRQF_TIMER | IRQF_IRQPOLL;
> +	struct ep93xx_tcu *tcu;
> +	int ret;
> +
> +	tcu = kzalloc(sizeof(*tcu), GFP_KERNEL);
> +	if (!tcu)
> +		return -ENOMEM;
> +
> +	tcu->base = of_iomap(np, 0);

fwnode_iomap()?
See below why it might make sense.

> +	if (!tcu->base) {

> +		pr_err("Can't remap registers\n");

First of all, you may utilize pr_fmt().
Second, you may add %pOF for better user experience.

> +		ret = -ENXIO;
> +		goto out_free;
> +	}

> +	irq = irq_of_parse_and_map(np, 0);

fwnode_irq_get() which is better in terms of error handling.

> +	if (irq == 0)
> +		irq = -EINVAL;
> +	if (irq < 0) {

> +		pr_err("EP93XX Timer Can't parse IRQ %d", irq);

As per above.

> +		goto out_free;
> +	}

...

> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 28/42] input: keypad: ep93xx: add DT support for Cirrus EP93xx

[Andy Shevchenko]

On Thu, Jul 20, 2023 at 02:29:28PM +0300, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
> 
> - drop flags, they were not used anyway
> - add OF ID match table
> - process "autorepeat", "debounce-delay-ms", prescale from device tree
> - drop platform data usage and it's header
> - keymap goes from device tree now on

...

>  #include <linux/bits.h>
>  #include <linux/module.h>

> +#include <linux/of.h>

As Dmitry told you, but I think you also need property.h.

>  #include <linux/platform_device.h>
> +#include <linux/mod_devicetable.h>
>  #include <linux/interrupt.h>
>  #include <linux/clk.h>
>  #include <linux/io.h>

...

>  #include <linux/input/matrix_keypad.h>
>  #include <linux/slab.h>
>  #include <linux/soc/cirrus/ep93xx.h>
> -#include <linux/platform_data/keypad-ep93xx.h>
>  #include <linux/pm_wakeirq.h>

When adding new inclusions, try to squeeze them to most ordered part of
the block.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 22/42] dma: cirrus: add DT support for Cirrus EP93xx

[Andy Shevchenko]

On Thu, Jul 20, 2023 at 02:29:22PM +0300, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
> 
> - drop subsys_initcall code
> - add OF ID match table with data
> - add of_probe for device tree

...

> +#include <linux/of_device.h>

Why?

...

> +#ifdef CONFIG_OF

Why this ugly ifdeffery?

...

> +	data = of_device_get_match_data(&pdev->dev);

device_get_match_data()

> +	if (!data)
> +		return ERR_PTR(dev_err_probe(&pdev->dev, -ENODEV, "No device match found\n"));

...

> +	edma = devm_kzalloc(&pdev->dev,
> +					  struct_size(edma, channels, data->num_channels),
> +				      GFP_KERNEL);

Something wrong with indentation. Not the first time, please check all your
patches for this kind of issues.

> +		return ERR_PTR(-ENOMEM);

...

> +		edmac->regs = devm_platform_ioremap_resource(pdev, i);

No check?

> +		edmac->irq = platform_get_irq(pdev, i);

No check?

> +		edmac->edma = edma;
> +
> +		edmac->clk = of_clk_get(np, i);

> +

Redundant blank line.

Why one of devm_clk_get*() can't be called?

> +		if (IS_ERR(edmac->clk)) {
> +			dev_warn(&pdev->dev, "failed to get clock\n");
> +			continue;
> +		}

...

> +	if (platform_get_device_id(pdev))
> +		edma = ep93xx_init_from_pdata(pdev);
> +	else
> +		edma = ep93xx_dma_of_probe(pdev);

> +

Unneeded blank line.

> +	if (!edma)
> +		return PTR_ERR(edma);

...

> --- a/include/linux/platform_data/dma-ep93xx.h
> +++ b/include/linux/platform_data/dma-ep93xx.h

>  #include <linux/types.h>
>  #include <linux/dmaengine.h>
>  #include <linux/dma-mapping.h>

> +#include <linux/of.h>

property.h.

...

> +	if (of_device_is_compatible(dev_of_node(chan->device->dev), "cirrus,ep9301-dma-m2p"))
> +		return true;
> +

device_is_compatible()

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 24/42] mtd: nand: add support for ts72xx

[Andy Shevchenko]

On Thu, Jul 20, 2023 at 02:29:24PM +0300, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
> 
> Technologic Systems has it's own nand controller implementation in CPLD.

...

+ bits.h

> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>

...

> +static int ts72xx_nand_attach_chip(struct nand_chip *chip)
> +{
> +	switch (chip->ecc.engine_type) {
> +	case NAND_ECC_ENGINE_TYPE_SOFT:
> +		if (chip->ecc.algo == NAND_ECC_ALGO_UNKNOWN)
> +			chip->ecc.algo = NAND_ECC_ALGO_HAMMING;
> +		break;
> +	case NAND_ECC_ENGINE_TYPE_ON_HOST:
> +		return -EINVAL;
> +	default:

> +		break;

Here it will return 0, is it a problem?

> +	}
> +
> +	return 0;
> +}

...

> +static int ts72xx_nand_probe(struct platform_device *pdev)
> +{
> +	struct ts72xx_nand_data *data;
> +	struct device_node *child;
> +	struct mtd_info *mtd;
> +	int err;

> +	/* Allocate memory for the device structure (and zero it) */

Useless comment.

> +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->controller.ops = &ts72xx_nand_ops;
> +	nand_controller_init(&data->controller);
> +	data->chip.controller = &data->controller;
> +
> +	data->io_base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(data->io_base))
> +		return PTR_ERR(data->io_base);
> +
> +	child = of_get_next_child(pdev->dev.of_node, NULL);

Why not using device property API from day 1?

	fwnode_get_next_child_node()

> +	if (!child)
> +		return dev_err_probe(&pdev->dev, -ENXIO,
> +				"ts72xx controller node should have exactly one child\n");

From now on you leak the reference count in error path.

> +	nand_set_flash_node(&data->chip, child);
> +	mtd = nand_to_mtd(&data->chip);
> +	mtd->dev.parent = &pdev->dev;
> +
> +	data->chip.legacy.IO_ADDR_R = data->io_base;
> +	data->chip.legacy.IO_ADDR_W = data->io_base;
> +	data->chip.legacy.cmd_ctrl = ts72xx_nand_hwcontrol;
> +	data->chip.legacy.dev_ready = ts72xx_nand_device_ready;
> +
> +	platform_set_drvdata(pdev, data);
> +
> +	/*
> +	 * This driver assumes that the default ECC engine should be TYPE_SOFT.
> +	 * Set ->engine_type before registering the NAND devices in order to
> +	 * provide a driver specific default value.
> +	 */
> +	data->chip.ecc.engine_type = NAND_ECC_ENGINE_TYPE_SOFT;
> +
> +	/* Scan to find existence of the device */
> +	err = nand_scan(&data->chip, 1);
> +	if (err)
> +		return err;
> +
> +	err = mtd_device_parse_register(mtd, NULL, NULL, NULL, 0);
> +	if (err) {
> +		nand_cleanup(&data->chip);

> +		return err;
> +	}
> +
> +	return 0;


These 4 lines can be simply

	return err;

but see above.

> +}

...

> +static void ts72xx_nand_remove(struct platform_device *pdev)
> +{
> +	struct ts72xx_nand_data *data = platform_get_drvdata(pdev);
> +	struct nand_chip *chip = &data->chip;
> +	int ret;
> +
> +	ret = mtd_device_unregister(nand_to_mtd(chip));

> +	WARN_ON(ret);

Why?!  Is it like this in other MTD drivers?

> +	nand_cleanup(chip);
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 20/42] net: cirrus: add DT support for Cirrus EP93xx

[Andy Shevchenko]

On Thu, Jul 20, 2023 at 02:29:20PM +0300, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
> 
> - add OF ID match table
> - get phy_id from the device tree, as part of mdio
> - copy_addr is now always used, as there is no SoC/board that aren't
> - dropped platform header

...

> +	base_addr = ioremap(mem->start, resource_size(mem));
> +	if (!base_addr)
> +		return dev_err_probe(&pdev->dev, -EIO, "Failed to ioremap ethernet registers\n");
> +
> +	np = of_parse_phandle(pdev->dev.of_node, "phy-handle", 0);

Isn't it something which is done in PHY core should do for you?
Maybe Andrew Lunn can comment on this.

> +	if (!np)

Yeah, right, let's leak mapped IO address space...
And so on.

> +		return dev_err_probe(&pdev->dev, -ENODEV, "Please provide \"phy-handle\"\n");

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 14/42] power: reset: Add a driver for the ep93xx reset

[Andy Shevchenko]

On Thu, Jul 20, 2023 at 02:29:14PM +0300, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
> 
> Implement the reset behaviour of the various EP93xx SoCS in drivers/power/reset.
> 
> It used to be located in arch/arm/mach-ep93xx.

...

> +// SPDX-License-Identifier: (GPL-2.0)

Are you sure this is correct form? Have you checked your patches?

...

> +#include <linux/of_device.h>

Do you need this?
Or maybe you need another (of*.h) one?

...

> +	/* Issue the reboot */
> +	ep93xx_devcfg_set_clear(priv->map, EP93XX_SYSCON_DEVCFG_SWRST, 0x00);
> +	ep93xx_devcfg_set_clear(priv->map, 0x00, EP93XX_SYSCON_DEVCFG_SWRST);


> +	mdelay(1000);

Atomic?! Such a huge delay must be explained, esp. why it's atomic.

> +	pr_emerg("Unable to restart system\n");
> +	return NOTIFY_DONE;

...

> +	err = register_restart_handler(&priv->restart_handler);
> +	if (err)
> +		return dev_err_probe(dev, err, "can't register restart notifier\n");

> +	return err;

	return 0;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 18/42] spi: ep93xx: add DT support for Cirrus EP93xx

[Andy Shevchenko]

On Thu, Jul 20, 2023 at 02:29:18PM +0300, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
> 
> - add OF ID match table
> 
> Instead of platform use_dma flag - check if DT dmas property is present to
> decide to use dma ot not.

...

> +#ifdef CONFIG_OF

Why ifdeffery?

Can't it be first checked for firmware provided info and fell back to platdata?

> +static struct ep93xx_spi_info dt_spi_info;

...

> +#endif

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 02/42] dt-bindings: clock: Add Cirrus EP93xx

[Nikita Shubin]

Hello Stephen.

On Thu, 2023-07-20 at 16:20 -0700, Stephen Boyd wrote:
> Quoting Nikita Shubin via B4 Relay (2023-07-20 04:29:02)
> > diff --git a/Documentation/devicetree/bindings/clock/cirrus,ep9301-
> > clk.yaml b/Documentation/devicetree/bindings/clock/cirrus,ep9301-
> > clk.yaml
> > new file mode 100644
> > index 000000000000..111e016601fb
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/cirrus,ep9301-
> > clk.yaml
> > @@ -0,0 +1,46 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/clock/cirrus,ep9301-clk.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Cirrus Logic ep93xx SoC's clock controller
> > +
> > +maintainers:
> > +  - Nikita Shubin <nikita.shubin@maquefel.me>
> > +  - Alexander Sverdlin <alexander.sverdlin@gmail.com>
> > +
> > +properties:
> > +  compatible:
> > +    oneOf:
> > +      - const: cirrus,ep9301-clk
> > +      - items:
> > +          - enum:
> > +              - cirrus,ep9302-clk
> > +              - cirrus,ep9307-clk
> > +              - cirrus,ep9312-clk
> > +              - cirrus,ep9315-clk
> > +          - const: cirrus,ep9301-clk
> > +
> > +  "#clock-cells":
> > +    const: 1
> > +
> > +  clocks:
> > +    items:
> > +      - description: reference clock
> > +
> > +required:
> > +  - compatible
> > +  - "#clock-cells"
> > +  - clocks
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    clock-controller {
> > +      compatible = "cirrus,ep9301-clk";
> 
> Is there a reg property? The driver grabs some syscon and then iomaps
> it, so presumably there is a register range. Is it part of some other
> hardware block? If so, can you make that device register sub-devices
> with the auxiliary bus instead of using a syscon?

Is reg property missing the only thing that doesn't fit ? 

`devm_of_iomap` was done only for reusing `devm_clk_hw_register_gate`
for DMA's and USB clock gates, i can give clk node it's own registers,
like:

reg = <0x80930004 0x04>;

Or drop devm_clk_hw_register_gate reusage entirely and just implement
non swlocked version of clk enable/disable that will go through syscon
regmap.

The ep93xx really looks like an syscon device in docs it refers itself
as a "Syscon block", also converting into "Auxiliary Bus" won't help
with `ep93xx_syscon_swlocked_write` either.

Re: [PATCH v3 03/42] clk: ep93xx: add DT support for Cirrus EP93xx

[Nikita Shubin]

Hello Stephen.

On Thu, 2023-07-20 at 16:27 -0700, Stephen Boyd wrote:
> Quoting Nikita Shubin via B4 Relay (2023-07-20 04:29:03)
> > diff --git a/drivers/clk/clk-ep93xx.c b/drivers/clk/clk-ep93xx.c
> > new file mode 100644
> > index 000000000000..7b9b3a0894ab
> > --- /dev/null
> > +++ b/drivers/clk/clk-ep93xx.c
> > @@ -0,0 +1,764 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Clock control for Cirrus EP93xx chips.
> > + * Copyright (C) 2021 Nikita Shubin <nikita.shubin@maquefel.me>
> > + *
> > + * Based on a rewrite of arch/arm/mach-ep93xx/clock.c:
> > + * Copyright (C) 2006 Lennert Buytenhek <buytenh@wantstofly.org>
> > + */
> > +#define pr_fmt(fmt) "ep93xx " KBUILD_MODNAME ": " fmt
> > +
> > +#include <linux/bits.h>
> > +#include <linux/clk-provider.h>
> > +#include <linux/clk.h>
> > +#include <linux/clkdev.h>
> 
> Drop the unused includes.
> 
> > +#include <linux/err.h>
> > +#include <linux/io.h>
> > +#include <linux/kernel.h>
> > +#include <linux/mfd/syscon.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/string.h>
> > +#include <linux/sys_soc.h>
> > +
> > +#include <linux/soc/cirrus/ep93xx.h>
> > +#include <dt-bindings/clock/cirrus,ep93xx-clock.h>
> > +
> > +#include <asm/div64.h>
> > +
> > +#define EP93XX_EXT_RTC_RATE            32768
> > +
> > +#define EP93XX_SYSCON_POWER_STATE      0x00
> > +#define EP93XX_SYSCON_PWRCNT           0x04
> > +#define EP93XX_SYSCON_PWRCNT_UARTBAUD  BIT(29)
> > +#define EP93XX_SYSCON_PWRCNT_USH_EN    28
> > +#define EP93XX_SYSCON_PWRCNT_DMA_M2M1  27
> > +#define EP93XX_SYSCON_PWRCNT_DMA_M2M0  26
> > +#define EP93XX_SYSCON_PWRCNT_DMA_M2P8  25
> > +#define EP93XX_SYSCON_PWRCNT_DMA_M2P9  24
> > +#define EP93XX_SYSCON_PWRCNT_DMA_M2P6  23
> > +#define EP93XX_SYSCON_PWRCNT_DMA_M2P7  22
> > +#define EP93XX_SYSCON_PWRCNT_DMA_M2P4  21
> > +#define EP93XX_SYSCON_PWRCNT_DMA_M2P5  20
> > +#define EP93XX_SYSCON_PWRCNT_DMA_M2P2  19
> > +#define EP93XX_SYSCON_PWRCNT_DMA_M2P3  18
> > +#define EP93XX_SYSCON_PWRCNT_DMA_M2P0  17
> > +#define EP93XX_SYSCON_PWRCNT_DMA_M2P1  16
> > +#define EP93XX_SYSCON_DEVCFG           0x80
> > +#define EP93XX_SYSCON_DEVCFG_U3EN      24
> > +#define EP93XX_SYSCON_DEVCFG_U2EN      20
> > +#define EP93XX_SYSCON_DEVCFG_U1EN      18
> > +#define EP93XX_SYSCON_VIDCLKDIV                0x84
> > +#define EP93XX_SYSCON_CLKDIV_ENABLE    15
> > +#define EP93XX_SYSCON_CLKDIV_ESEL      BIT(14)
> > +#define EP93XX_SYSCON_CLKDIV_PSEL      BIT(13)
> > +#define EP93XX_SYSCON_CLKDIV_MASK      GENMASK(14, 13)
> > +#define EP93XX_SYSCON_CLKDIV_PDIV_SHIFT        8
> > +#define EP93XX_SYSCON_I2SCLKDIV                0x8c
> > +#define EP93XX_SYSCON_I2SCLKDIV_SENA   31
> > +#define EP93XX_SYSCON_I2SCLKDIV_ORIDE  BIT(29)
> > +#define EP93XX_SYSCON_I2SCLKDIV_SPOL   BIT(19)
> > +#define EP93XX_I2SCLKDIV_SDIV          (1 << 16)
> > +#define EP93XX_I2SCLKDIV_LRDIV32       (0 << 17)
> > +#define EP93XX_I2SCLKDIV_LRDIV64       (1 << 17)
> > +#define EP93XX_I2SCLKDIV_LRDIV128      (2 << 17)
> > +#define EP93XX_I2SCLKDIV_LRDIV_MASK    (3 << 17)
> > +#define EP93XX_SYSCON_KEYTCHCLKDIV     0x90
> > +#define EP93XX_SYSCON_KEYTCHCLKDIV_TSEN        31
> > +#define EP93XX_SYSCON_KEYTCHCLKDIV_ADIV        16
> > +#define EP93XX_SYSCON_KEYTCHCLKDIV_KEN 15
> > +#define EP93XX_SYSCON_KEYTCHCLKDIV_KDIV        0
> > +#define EP93XX_SYSCON_CHIPID           0x94
> > +#define EP93XX_SYSCON_CHIPID_ID                0x9213
> > +
> > +/* Keeps track of all clocks */
> > +static const char adc_divisors[] = { 16, 4 };
> > +static const char sclk_divisors[] = { 2, 4 };
> > +static const char lrclk_divisors[] = { 32, 64, 128 };
> > +
> > +#define EP_PARENT(NAME) { .name = NAME, .fw_name = NAME }
> > +
> > +static const struct clk_parent_data ep93xx_clk_parents[] = {
> > +       EP_PARENT("xtali"),
> > +       EP_PARENT("pll1"),
> > +       EP_PARENT("pll2"),
> 
> pll2 and pll1 aren't in the DT binding, so they shouldn't be set for
> .fw_name here.
> 
> > +};
> > +
> > +struct ep93xx_clk {
> > +       struct clk_hw hw;
> > +       u16 idx;
> > +       u16 reg;
> > +       u32 mask;
> > +       u8 bit_idx;
> > +       u8 shift;
> > +       u8 width;
> > +       u8 num_div;
> > +       const char *div;
> > +};
> > +
> > +struct ep93xx_clk_priv {
> > +       spinlock_t lock;
> > +       struct device *dev;
> > +       void __iomem *base;
> > +       struct regmap *map;
> > +       struct clk_hw *fixed[16];
> > +       struct ep93xx_clk reg[];
> > +};
> > +
> > +static struct ep93xx_clk *ep93xx_clk_from(struct clk_hw *hw)
> > +{
> > +       return container_of(hw, struct ep93xx_clk, hw);
> > +}
> > +
> > +static struct ep93xx_clk_priv *ep93xx_priv_from(struct ep93xx_clk
> > *clk)
> > +{
> > +       return container_of(clk, struct ep93xx_clk_priv, reg[clk-
> > >idx]);
> > +}
> > +
> > +static int ep93xx_clk_is_enabled(struct clk_hw *hw)
> > +{
> > +       struct ep93xx_clk *clk = ep93xx_clk_from(hw);
> > +       struct ep93xx_clk_priv *priv = ep93xx_priv_from(clk);
> > +       u32 val;
> > +
> > +       regmap_read(priv->map, clk->reg, &val);
> > +
> > +       return !!(val & BIT(clk->bit_idx));
> > +}
> > +
> > +static int ep93xx_clk_enable(struct clk_hw *hw)
> > +{
> > +       struct ep93xx_clk *clk = ep93xx_clk_from(hw);
> > +       struct ep93xx_clk_priv *priv = ep93xx_priv_from(clk);
> > +       unsigned long flags;
> > +       u32 val;
> > +
> > +       spin_lock_irqsave(&priv->lock, flags);
> > +
> > +       regmap_read(priv->map, clk->reg, &val);
> > +       val |= BIT(clk->bit_idx);
> > +
> > +       ep93xx_syscon_swlocked_write(priv->map, clk->reg, val);
> > +
> > +       spin_unlock_irqrestore(&priv->lock, flags);
> > +
> > +       return 0;
> > +}
> > +
> > +static void ep93xx_clk_disable(struct clk_hw *hw)
> > +{
> > +       struct ep93xx_clk *clk = ep93xx_clk_from(hw);
> > +       struct ep93xx_clk_priv *priv = ep93xx_priv_from(clk);
> > +       unsigned long flags;
> > +       u32 val;
> > +
> > +       spin_lock_irqsave(&priv->lock, flags);
> > +
> > +       regmap_read(priv->map, clk->reg, &val);
> > +       val &= ~BIT(clk->bit_idx);
> > +
> > +       ep93xx_syscon_swlocked_write(priv->map, clk->reg, val);
> > +
> > +       spin_unlock_irqrestore(&priv->lock, flags);
> > +}
> > +
> > +static const struct clk_ops clk_ep93xx_gate_ops = {
> > +       .enable = ep93xx_clk_enable,
> > +       .disable = ep93xx_clk_disable,
> > +       .is_enabled = ep93xx_clk_is_enabled,
> > +};
> > +
> > +static int ep93xx_clk_register_gate(struct ep93xx_clk *clk,
> > +                                       const char *name,
> > +                                       const char *parent_name,
> > +                                       unsigned long flags,
> > +                                       unsigned int reg,
> > +                                       u8 bit_idx)
> > +{
> > +       struct ep93xx_clk_priv *priv = ep93xx_priv_from(clk);
> > +       struct clk_parent_data parent_data = { };
> > +       struct clk_init_data init = { };
> > +
> > +       init.name = name;
> > +       init.ops = &clk_ep93xx_gate_ops;
> > +       init.flags = flags;
> > +
> > +       parent_data.fw_name = parent_name;
> > +       parent_data.name = parent_name;
> > +       init.parent_data = &parent_data;
> > +       init.num_parents = 1;
> > +
> > +       clk->reg = reg;
> > +       clk->bit_idx = bit_idx;
> > +       clk->hw.init = &init;
> > +
> > +       return devm_clk_hw_register(priv->dev, &clk->hw);
> > +}
> > +
> > +static u8 ep93xx_mux_get_parent(struct clk_hw *hw)
> > +{
> > +       struct ep93xx_clk *clk = ep93xx_clk_from(hw);
> > +       struct ep93xx_clk_priv *priv = ep93xx_priv_from(clk);
> > +       u32 val;
> > +
> > +       regmap_read(priv->map, clk->reg, &val);
> > +
> > +       val &= EP93XX_SYSCON_CLKDIV_MASK;
> > +
> > +       switch (val) {
> > +       case EP93XX_SYSCON_CLKDIV_ESEL:
> > +               return 1; // PLL1
> > +       case EP93XX_SYSCON_CLKDIV_MASK:
> > +               return 2; // PLL2
> > +       default:
> > +               break;
> > +       };
> > +
> > +       return 0; // XTALI
> 
> Please use /* comments like this */
> 
> > +}
> > +
> > +static int ep93xx_mux_set_parent_lock(struct clk_hw *hw, u8 index)
> > +{
> > +       struct ep93xx_clk *clk = ep93xx_clk_from(hw);
> > +       struct ep93xx_clk_priv *priv = ep93xx_priv_from(clk);
> > +       unsigned long flags;
> > +       u32 val;
> > +
> > +       if (index >= ARRAY_SIZE(ep93xx_clk_parents))
> > +               return -EINVAL;
> > +
> > +       spin_lock_irqsave(&priv->lock, flags);
> > +
> > +       regmap_read(priv->map, clk->reg, &val);
> > +       val &= ~(EP93XX_SYSCON_CLKDIV_MASK);
> > +       if (index) {
> > +               val |= EP93XX_SYSCON_CLKDIV_ESEL;
> > +               val |= (index - 1) ? EP93XX_SYSCON_CLKDIV_PSEL : 0;
> > +       }
> > +
> > +       ep93xx_syscon_swlocked_write(priv->map, clk->reg, val);
> > +
> > +       spin_unlock_irqrestore(&priv->lock, flags);
> > +
> > +       return 0;
> > +}
> > +
> > +static bool is_best(unsigned long rate, unsigned long now,
> > +                    unsigned long best)
> > +{
> > +       return abs(rate - now) < abs(rate - best);
> 
> Another case where we need abs_diff() so that it doesn't get confused
> when trying to do signed comparison.
> 
> > +}
> > +
> > +static int ep93xx_mux_determine_rate(struct clk_hw *hw,
> > +                               struct clk_rate_request *req)
> > +{
> > +       unsigned long best_rate = 0, actual_rate, mclk_rate;
> > +       unsigned long rate = req->rate;
> > +       struct clk_hw *parent_best = NULL;
> > +       unsigned long parent_rate_best;
> > +       unsigned long parent_rate;
> > +       int div, pdiv;
> > +       unsigned int i;
> > +
> > +       /*
> > +        * Try the two pll's and the external clock
> > +        * Because the valid predividers are 2, 2.5 and 3, we
> > multiply
> > +        * all the clocks by 2 to avoid floating point math.
> > +        *
> > +        * This is based on the algorithm in the ep93xx raster
> > guide:
> > +        * http://be-a-maverick.com/en/pubs/appNote/AN269REV1.pdf
> > +        *
> > +        */
> > +       for (i = 0; i < clk_hw_get_num_parents(hw); i++) {
> > +               struct clk_hw *parent =
> > clk_hw_get_parent_by_index(hw, i);
> > +
> > +               parent_rate = clk_hw_get_rate(parent);
> > +               mclk_rate = parent_rate * 2;
> > +
> > +               /* Try each predivider value */
> > +               for (pdiv = 4; pdiv <= 6; pdiv++) {
> > +                       div = DIV_ROUND_CLOSEST(mclk_rate, rate *
> > pdiv);
> > +                       if (div < 1 || div > 127)
> > +                               continue;
> > +
> > +                       actual_rate = DIV_ROUND_CLOSEST(mclk_rate,
> > pdiv * div);
> > +                       if (is_best(rate, actual_rate, best_rate))
> > {
> > +                               best_rate = actual_rate;
> > +                               parent_rate_best = parent_rate;
> > +                               parent_best = parent;
> > +                       }
> > +               }
> > +       }
> > +
> > +       if (!parent_best)
> > +               return -EINVAL;
> > +
> > +       req->best_parent_rate = parent_rate_best;
> > +       req->best_parent_hw = parent_best;
> > +       req->rate = best_rate;
> > +
> > +       return 0;
> > +}
> > +
> > +static unsigned long ep93xx_ddiv_recalc_rate(struct clk_hw *hw,
> > +                                               unsigned long
> > parent_rate)
> > +{
> > +       struct ep93xx_clk *clk = ep93xx_clk_from(hw);
> > +       struct ep93xx_clk_priv *priv = ep93xx_priv_from(clk);
> > +       unsigned int pdiv, div;
> > +       u32 val;
> > +
> > +       regmap_read(priv->map, clk->reg, &val);
> > +       pdiv = ((val >> EP93XX_SYSCON_CLKDIV_PDIV_SHIFT) & 0x03);
> > +       div = val & GENMASK(6, 0);
> > +       if (!div)
> > +               return 0;
> > +
> > +       return DIV_ROUND_CLOSEST(parent_rate * 2, (pdiv + 3) *
> > div);
> > +}
> > +
> > +static int ep93xx_ddiv_set_rate(struct clk_hw *hw, unsigned long
> > rate,
> > +                               unsigned long parent_rate)
> > +{
> > +       struct ep93xx_clk *clk = ep93xx_clk_from(hw);
> > +       struct ep93xx_clk_priv *priv = ep93xx_priv_from(clk);
> > +       int pdiv, div, npdiv, ndiv;
> > +       unsigned long actual_rate, mclk_rate, rate_err = ULONG_MAX;
> > +       u32 val;
> > +
> > +       regmap_read(priv->map, clk->reg, &val);
> > +       mclk_rate = parent_rate * 2;
> > +
> > +       for (pdiv = 4; pdiv <= 6; pdiv++) {
> > +               div = DIV_ROUND_CLOSEST(mclk_rate, rate * pdiv);
> > +               if (div < 1 || div > 127)
> > +                       continue;
> > +
> > +               actual_rate = DIV_ROUND_CLOSEST(mclk_rate, pdiv *
> > div);
> > +               if (abs(actual_rate - rate) < rate_err) {
> > +                       npdiv = pdiv - 3;
> > +                       ndiv = div;
> > +                       rate_err = abs(actual_rate - rate);
> > +               }
> > +       }
> > +
> > +       if (rate_err == ULONG_MAX)
> > +               return -EINVAL;
> > +
> > +       /* Clear old dividers */
> > +       val &= ~(GENMASK(9, 0) & ~BIT(7));
> > +
> > +       /* Set the new pdiv and div bits for the new clock rate */
> > +       val |= (npdiv << EP93XX_SYSCON_CLKDIV_PDIV_SHIFT) | ndiv;
> > +
> > +       ep93xx_syscon_swlocked_write(priv->map, clk->reg, val);
> > +
> > +       return 0;
> > +}
> > +
> > +static const struct clk_ops clk_ddiv_ops = {
> > +       .enable = ep93xx_clk_enable,
> > +       .disable = ep93xx_clk_disable,
> > +       .is_enabled = ep93xx_clk_is_enabled,
> > +       .get_parent = ep93xx_mux_get_parent,
> > +       .set_parent = ep93xx_mux_set_parent_lock,
> > +       .determine_rate = ep93xx_mux_determine_rate,
> > +       .recalc_rate = ep93xx_ddiv_recalc_rate,
> > +       .set_rate = ep93xx_ddiv_set_rate,
> > +};
> > +
> > +static int clk_hw_register_ddiv(struct ep93xx_clk *clk,
> > +                                               const char *name,
> > +                                               unsigned int reg,
> > +                                               u8 bit_idx)
> > +{
> > +       struct ep93xx_clk_priv *priv = ep93xx_priv_from(clk);
> > +       struct clk_init_data init = { };
> > +
> > +       init.name = name;
> > +       init.ops = &clk_ddiv_ops;
> > +       init.flags = 0;
> > +       init.parent_data = ep93xx_clk_parents;
> > +       init.num_parents = ARRAY_SIZE(ep93xx_clk_parents);
> > +
> > +       clk->reg = reg;
> > +       clk->bit_idx = bit_idx;
> > +       clk->hw.init = &init;
> > +
> > +       return devm_clk_hw_register(priv->dev, &clk->hw);
> > +}
> > +
> > +static unsigned long ep93xx_div_recalc_rate(struct clk_hw *hw,
> > +                                           unsigned long
> > parent_rate)
> > +{
> > +       struct ep93xx_clk *clk = ep93xx_clk_from(hw);
> > +       struct ep93xx_clk_priv *priv = ep93xx_priv_from(clk);
> > +       u32 val;
> > +       u8 index;
> > +
> > +       regmap_read(priv->map, clk->reg, &val);
> > +       index = (val & clk->mask) >> clk->shift;
> > +       if (index > clk->num_div)
> > +               return 0;
> > +
> > +       return DIV_ROUND_CLOSEST(parent_rate, clk->div[index]);
> > +}
> > +
> > +static long ep93xx_div_round_rate(struct clk_hw *hw, unsigned long
> > rate,
> > +                                  unsigned long *parent_rate)
> > +{
> > +       struct ep93xx_clk *clk = ep93xx_clk_from(hw);
> > +       unsigned long best = 0, now;
> > +       unsigned int i;
> > +
> > +       for (i = 0; i < clk->num_div; i++) {
> > +               if ((rate * clk->div[i]) == *parent_rate)
> > +                       return rate;
> > +
> > +               now = DIV_ROUND_CLOSEST(*parent_rate, clk->div[i]);
> > +               if (!best || is_best(rate, now, best))
> > +                       best = now;
> > +       }
> > +
> > +       return best;
> > +}
> > +
> > +static int ep93xx_div_set_rate(struct clk_hw *hw, unsigned long
> > rate,
> > +                              unsigned long parent_rate)
> > +{
> > +       struct ep93xx_clk *clk = ep93xx_clk_from(hw);
> > +       struct ep93xx_clk_priv *priv = ep93xx_priv_from(clk);
> > +       unsigned int i;
> > +       u32 val;
> > +
> > +       regmap_read(priv->map, clk->reg, &val);
> > +       val &= ~clk->mask;
> > +       for (i = 0; i < clk->num_div; i++)
> > +               if (rate == DIV_ROUND_CLOSEST(parent_rate, clk-
> > >div[i])) {
> > +                       val |= i << clk->shift;
> > +                       break;
> > +               }
> > +
> > +       if (i == clk->num_div)
> > +               return -EINVAL;
> > +
> > +       ep93xx_syscon_swlocked_write(priv->map, clk->reg, val);
> > +
> > +       return 0;
> > +}
> > +
> > +static const struct clk_ops ep93xx_div_ops = {
> > +       .enable = ep93xx_clk_enable,
> > +       .disable = ep93xx_clk_disable,
> > +       .is_enabled = ep93xx_clk_is_enabled,
> > +       .recalc_rate = ep93xx_div_recalc_rate,
> > +       .round_rate = ep93xx_div_round_rate,
> > +       .set_rate = ep93xx_div_set_rate,
> > +};
> > +
> > +static int clk_hw_register_div(struct ep93xx_clk *clk,
> > +                                         const char *name,
> > +                                         const char *parent_name,
> 
> Please try to pass a direct pointer to the parent clk_hw instead or
> if
> the clk exists outside the controller pass a DT index.
> 
> > +                                         unsigned int reg,
> > +                                         u8 enable_bit,
> > +                                         u8 shift,
> > +                                         u8 width,
> > +                                         const char *clk_divisors,
> > +                                         u8 num_div)
> > +{
> > +       struct ep93xx_clk_priv *priv = ep93xx_priv_from(clk);
> > +       struct clk_parent_data parent_data = { };
> > +       struct clk_init_data init = { };
> > +
> > +       init.name = name;
> > +       init.ops = &ep93xx_div_ops;
> > +       init.flags = 0;
> > +       parent_data.fw_name = parent_name;
> > +       parent_data.name = parent_name;
> > +       init.parent_data = &parent_data;
> > +       init.num_parents = 1;
> > +
> > +       clk->reg = reg;
> > +       clk->bit_idx = enable_bit;
> > +       clk->mask = GENMASK(shift + width - 1, shift);
> > +       clk->shift = shift;
> > +       clk->div = clk_divisors;
> [...]
> > +
> > +static int ep93xx_clk_probe(struct platform_device *pdev)
> > +{
> > +       struct device *dev = &pdev->dev;
> > +       struct device_node *np = dev->of_node;
> > +       const struct soc_device_attribute *match;
> > +       struct ep93xx_clk_priv *priv;
> > +       struct ep93xx_clk *clk;
> > +       struct device_node *parent;
> > +       unsigned long clk_spi_div;
> > +       int ret;
> > +       u32 value;
> > +
> > +       priv = devm_kzalloc(dev, struct_size(priv, reg,
> > EP93XX_CLK_UART), GFP_KERNEL);
> > +       if (!priv)
> > +               return -ENOMEM;
> > +
> > +       spin_lock_init(&priv->lock);
> > +       priv->dev = dev;
> > +       parent = of_get_parent(np);
> > +       if (!parent)
> > +               return dev_err_probe(dev, -EINVAL, "no syscon
> > parent for clk node\n");
> > +
> > +       priv->map = syscon_node_to_regmap(parent);
> > +       if (IS_ERR(priv->map)) {
> > +               of_node_put(parent);
> > +               return dev_err_probe(dev, -EINVAL, "no syscon
> > regmap\n");
> > +       }
> > +
> > +       priv->base = devm_of_iomap(dev, parent, 0, NULL);
> > +       of_node_put(parent);
> > +       if (IS_ERR(priv->base))
> > +               return PTR_ERR(priv->base);
> > +
> > +       ret = ep93xx_uart_clock_init(priv);
> > +       if (ret)
> > +               return ret;
> > +
> > +       ret = ep93xx_dma_clock_init(priv);
> > +       if (ret)
> > +               return ret;
> > +
> > +       /*
> > +        * EP93xx SSP clock rate was doubled in version E2. For
> > more information
> > +        * see:
> > +        *     http://www.cirrus.com/en/pubs/appNote/AN273REV4.pdf
> > +        */
> > +       clk_spi_div = 2;
> > +       match = soc_device_match(ep93xx_soc_table);
> 
> Why isn't this part of the compatible string for the device? We're
> able
> to introduce new bindings, correct?

It's not a SoC version related, so we don't know such things in
advance.

It's currently determined by querying "ep93xx_chip_revision" which
determines chip revision:

https://elixir.bootlin.com/linux/v6.5-rc2/source/arch/arm/mach-ep93xx/clock.c#L631

I decided it's a good idea to drop another header exposed function.

"soc_device_match" only uses revision:

```
static const struct soc_device_attribute ep93xx_soc_table[] = {
	{ .revision = "E2", .data = (void *)1 },
	{ /* sentinel */ }
};
```

But as there are no revisions greater than E2 - it is single match
only.

> 
> > +       if (match)
> > +               clk_spi_div = (unsigned long)match->data;

Re: [PATCH v3 24/42] mtd: nand: add support for ts72xx

[Miquel Raynal]

Hi Andy,

> > +static int ts72xx_nand_attach_chip(struct nand_chip *chip)
> > +{
> > +	switch (chip->ecc.engine_type) {
> > +	case NAND_ECC_ENGINE_TYPE_SOFT:
> > +		if (chip->ecc.algo == NAND_ECC_ALGO_UNKNOWN)
> > +			chip->ecc.algo = NAND_ECC_ALGO_HAMMING;
> > +		break;
> > +	case NAND_ECC_ENGINE_TYPE_ON_HOST:
> > +		return -EINVAL;
> > +	default:  
> 
> > +		break;  
> 
> Here it will return 0, is it a problem?

Seems ok, there are two other situations: on-die ECC engine and no ECC
engine, both do not require any specific handling on the controller
side.

> 
> > +	}
> > +
> > +	return 0;
> > +}  
> 
> ...
> 
> > +static void ts72xx_nand_remove(struct platform_device *pdev)
> > +{
> > +	struct ts72xx_nand_data *data = platform_get_drvdata(pdev);
> > +	struct nand_chip *chip = &data->chip;
> > +	int ret;
> > +
> > +	ret = mtd_device_unregister(nand_to_mtd(chip));  
> 
> > +	WARN_ON(ret);  
> 
> Why?!  Is it like this in other MTD drivers?

Yes, we did not yet change the internal machinery to return void, and
we don't want people to think getting errors there is normal.

> > +	nand_cleanup(chip);
> > +}  
> 

Thanks,
Miquèl

Re: [PATCH v3 35/42] ARM: dts: ep93xx: add ts7250 board

[Nikita Shubin]

Hello Krzysztof!

Thank you for checking so quickly!

On Fri, 2023-07-21 at 16:15 +0200, Krzysztof Kozlowski wrote:
> On 20/07/2023 13:29, Nikita Shubin via B4 Relay wrote:
> > From: Nikita Shubin <nikita.shubin@maquefel.me>
> > 
> > Add device tree file for Technologic Systems ts7250 board and
> > Liebherr bk3 board which have many in common, both are based on
> > ep9302 SoC variant.
> > 
> > Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
> > ---
> >  arch/arm/boot/dts/cirrus/Makefile          |   3 +
> >  arch/arm/boot/dts/cirrus/ep93xx-bk3.dts    | 126
> > +++++++++++++++++++++++++
> >  arch/arm/boot/dts/cirrus/ep93xx-ts7250.dts | 145
> > +++++++++++++++++++++++++++++
> >  3 files changed, 274 insertions(+)
> > 
> > diff --git a/arch/arm/boot/dts/cirrus/Makefile
> > b/arch/arm/boot/dts/cirrus/Makefile
> > index e944d3e2129d..211a7e2f2115 100644
> > --- a/arch/arm/boot/dts/cirrus/Makefile
> > +++ b/arch/arm/boot/dts/cirrus/Makefile
> > @@ -3,3 +3,6 @@ dtb-$(CONFIG_ARCH_CLPS711X) += \
> >         ep7211-edb7211.dtb
> >  dtb-$(CONFIG_ARCH_CLPS711X) += \
> >         ep7211-edb7211.dtb
> > +dtb-$(CONFIG_ARCH_EP93XX) += \
> > +       ep93xx-bk3.dtb \
> > +       ep93xx-ts7250.dtb
> > diff --git a/arch/arm/boot/dts/cirrus/ep93xx-bk3.dts
> > b/arch/arm/boot/dts/cirrus/ep93xx-bk3.dts
> > new file mode 100644
> > index 000000000000..91d76a1a8571
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/cirrus/ep93xx-bk3.dts
> > @@ -0,0 +1,126 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Device Tree file for Liebherr controller BK3.1 based on Cirrus
> > EP9302 SoC
> > + */
> > +/dts-v1/;
> > +#include "ep93xx.dtsi"
> > +
> > +/ {
> > +       model = "Liebherr controller BK3.1";
> > +       compatible = "liebherr,bk3", "cirrus,ep9301";
> > +       #address-cells = <1>;
> > +       #size-cells = <1>;
> > +
> > +       chosen {
> > +       };
> > +
> > +       memory@0 {
> > +               device_type = "memory";
> > +               /* should be set from ATAGS */
> > +               reg = <0x00000000 0x02000000>,
> > +                     <0x000530c0 0x01fdd000>;
> > +       };
> > +
> > +       nand-controller@60000000 {
> > +               compatible = "technologic,ts7200-nand";
> > +               reg = <0x60000000 0x8000000>;
> > +               #address-cells = <1>;
> > +               #size-cells = <0>;
> > +
> > +               nand@0 {
> > +                       reg = <0>;
> > +                       partitions {
> > +                               compatible = "fixed-partitions";
> > +                               #address-cells = <1>;
> > +                               #size-cells = <1>;
> > +
> > +                               partition@0 {
> > +                                       label = "System";
> > +                                       reg = <0x00000000
> > 0x01e00000>;
> > +                                       read-only;
> > +                               };
> > +
> > +                               partition@1e00000 {
> > +                                       label = "Data";
> > +                                       reg = <0x01e00000
> > 0x05f20000>;
> > +                               };
> > +
> > +                               partition@7d20000 {
> > +                                       label = "RedBoot";
> > +                                       reg = <0x07d20000
> > 0x002e0000>;
> > +                                       read-only;
> > +                               };
> > +                       };
> > +               };
> > +       };
> > +
> > +       leds {
> > +               compatible = "gpio-leds";
> > +               led-0 {
> > +                       label = "grled";
> > +                       gpios = <&gpio4 0 GPIO_ACTIVE_HIGH>;
> > +                       linux,default-trigger = "heartbeat";
> > +                       function = LED_FUNCTION_HEARTBEAT;
> > +               };
> > +
> > +               led-1 {
> > +                       label = "rdled";
> > +                       gpios = <&gpio4 1 GPIO_ACTIVE_HIGH>;
> > +                       function = LED_FUNCTION_FAULT;
> > +               };
> > +       };
> > +};
> > +
> > +&eth0 {
> > +       phy-handle = <&phy0>;
> > +};
> > +
> > +&i2c0 {
> > +       status = "okay";
> > +};
> > +
> > +&i2s {
> > +       pinctrl-names = "default";
> > +       pinctrl-0 = <&i2s_on_ac97_pins>;
> > +       status = "okay";
> > +};
> > +
> > +&gpio1 {
> > +       /* PWM */
> > +       gpio-ranges = <&pinctrl 6 163 1>;
> > +};
> > +
> > +&gpio4 {
> > +       gpio-ranges = <&pinctrl 0 97 2>;
> > +       status = "okay";
> > +};
> > +
> > +&gpio6 {
> > +       gpio-ranges = <&pinctrl 0 87 2>;
> > +       status = "okay";
> > +};
> > +
> > +&gpio7 {
> > +       gpio-ranges = <&pinctrl 2 199 4>;
> > +       status = "okay";
> > +};
> > +
> > +&mdio0 {
> > +       phy0: ethernet-phy@1 {
> > +               reg = <1>;
> > +               device_type = "ethernet-phy";
> > +       };
> > +};
> > +
> > +&uart0 {
> > +       status = "okay";
> > +};
> > +
> > +&uart1 {
> > +       status = "okay";
> > +};
> > +
> > +&usb0 {
> > +       status = "okay";
> > +};
> > +
> > diff --git a/arch/arm/boot/dts/cirrus/ep93xx-ts7250.dts
> > b/arch/arm/boot/dts/cirrus/ep93xx-ts7250.dts
> > new file mode 100644
> > index 000000000000..625202f8cd25
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/cirrus/ep93xx-ts7250.dts
> > @@ -0,0 +1,145 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Device Tree file for Technologic Systems ts7250 board based on
> > Cirrus EP9302 SoC
> > + */
> > +/dts-v1/;
> > +#include "ep93xx.dtsi"
> > +#include <dt-bindings/dma/cirrus,ep93xx-dma.h>
> > +
> > +/ {
> > +       compatible = "technologic,ts7250", "cirrus,ep9301";
> > +       model = "TS-7250 SBC";
> > +       #address-cells = <1>;
> > +       #size-cells = <1>;
> > +
> > +       chosen {
> > +       };
> > +
> > +       memory@0 {
> > +               device_type = "memory";
> > +               /* should be set from ATAGS */
> > +               reg = <0x00000000 0x02000000>,
> > +                     <0x000530c0 0x01fdd000>;
> > +       };
> > +
> > +       nand-controller@60000000 {
> 
> Where is this address? It does not work like that. If this is part of
> SoC, then should be in DTSI and part of soc node. If not, then it is
> some other bus which needs some description. Top-level is not a bus.
> 

It's some kind of EBI, but it doesn't need a driver it is transparent 
on ts7250, the logic is controlled through installed CPLD.

The EBI it self is a part of the SoC through:

https://elixir.bootlin.com/linux/v6.5-rc3/source/arch/arm/mach-ep93xx/soc.h#L35

EP93XX_CS0_PHYS_BASE_ASYNC to EP93XX_CS0_PHYS_BASE_SYNC.

So for ts7250 this includes:

- NAND
- m48t86
- watchdog

I don't even know how to represent it correctly, would "simple-bus"
with "ranges" defined suit here, so it will represent hierarchy but
won't do anything ?

> You should see errors when testing dtbs with W=1.

Strangely - i don't see any, but anyway the above will change.

> 
> 
> > +               compatible = "technologic,ts7200-nand";
> > +               reg = <0x60000000 0x8000000>;
> > +               #address-cells = <1>;
> > +               #size-cells = <0>;
> > +
> > +               nand@0 {
> > +                       reg = <0>;
> > +                       partitions {
> > +                               compatible = "fixed-partitions";
> > +                               #address-cells = <1>;
> > +                               #size-cells = <1>;
> > +
> > +                               partition@0 {
> > +                                       label = "TS-BOOTROM";
> > +                                       reg = <0x00000000
> > 0x00020000>;
> > +                                       read-only;
> > +                               };
> > +
> > +                               partition@20000 {
> > +                                       label = "Linux";
> > +                                       reg = <0x00020000
> > 0x07d00000>;
> > +                               };
> > +
> > +                               partition@7d20000 {
> > +                                       label = "RedBoot";
> > +                                       reg = <0x07d20000
> > 0x002e0000>;
> > +                                       read-only;
> > +                               };
> > +                       };
> > +               };
> > +       };
> > +
> > +       rtc1: rtc@10800000 {
> 
> Same problems
> 
> > +               compatible = "st,m48t86";
> > +               reg = <0x10800000 0x1>,
> > +                     <0x11700000 0x1>;
> > +       };
> > +
> > +       watchdog1: watchdog@23800000 {
> 
> Same problems
> 
> > +               compatible = "technologic,ts7200-wdt";
> > +               reg = <0x23800000 0x01>,
> > +                     <0x23c00000 0x01>;
> > +               timeout-sec = <30>;
> > +       };
> > +
> > +       leds {
> > +               compatible = "gpio-leds";
> > +               led-0 {
> > +                       label = "grled";
> > +                       gpios = <&gpio4 0 GPIO_ACTIVE_HIGH>;
> > +                       linux,default-trigger = "heartbeat";
> > +                       function = LED_FUNCTION_HEARTBEAT;
> > +               };
> > +
> > +               led-1 {
> > +                       label = "rdled";
> > +                       gpios = <&gpio4 1 GPIO_ACTIVE_HIGH>;
> > +                       function = LED_FUNCTION_FAULT;
> > +               };
> > +       };
> > +};
> > +
> > +&eth0 {
> > +       phy-handle = <&phy0>;
> > +};
> > +
> > +&gpio1 {
> > +       /* PWM */
> > +       gpio-ranges = <&pinctrl 6 163 1>;
> > +};
> > +
> > +&gpio4 {
> > +       gpio-ranges = <&pinctrl 0 97 2>;
> > +       status = "okay";
> > +};
> > +
> > +&gpio6 {
> > +       gpio-ranges = <&pinctrl 0 87 2>;
> > +       status = "okay";
> > +};
> > +
> > +&gpio7 {
> > +       gpio-ranges = <&pinctrl 2 199 4>;
> > +       status = "okay";
> > +};
> > +
> > +&i2c0 {
> > +       status = "okay";
> > +};
> > +
> > +&spi0 {
> > +       cs-gpios = <&gpio5 2 GPIO_ACTIVE_HIGH>;
> > +       dmas = <&dma1 EP93XX_DMA_SSP>;
> > +       status = "okay";
> > +
> > +       tmp122_spi: tmp122@0 {
> 
> Node names should be generic. See also an explanation and list of
> examples (not exhaustive) in DT specification:
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> 
> 
> > +               compatible = "ti,tmp122";
> > +               reg = <0>;
> > +               spi-max-frequency = <2000000>;
> > +       };
> > +};
> > +
> 
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v3 35/42] ARM: dts: ep93xx: add ts7250 board

[Krzysztof Kozlowski]

On 24/07/2023 15:41, Nikita Shubin wrote:
>>> diff --git a/arch/arm/boot/dts/cirrus/ep93xx-ts7250.dts
>>> b/arch/arm/boot/dts/cirrus/ep93xx-ts7250.dts
>>> new file mode 100644
>>> index 000000000000..625202f8cd25
>>> --- /dev/null
>>> +++ b/arch/arm/boot/dts/cirrus/ep93xx-ts7250.dts
>>> @@ -0,0 +1,145 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Device Tree file for Technologic Systems ts7250 board based on
>>> Cirrus EP9302 SoC
>>> + */
>>> +/dts-v1/;
>>> +#include "ep93xx.dtsi"
>>> +#include <dt-bindings/dma/cirrus,ep93xx-dma.h>
>>> +
>>> +/ {
>>> +       compatible = "technologic,ts7250", "cirrus,ep9301";
>>> +       model = "TS-7250 SBC";
>>> +       #address-cells = <1>;
>>> +       #size-cells = <1>;
>>> +
>>> +       chosen {
>>> +       };
>>> +
>>> +       memory@0 {
>>> +               device_type = "memory";
>>> +               /* should be set from ATAGS */
>>> +               reg = <0x00000000 0x02000000>,
>>> +                     <0x000530c0 0x01fdd000>;
>>> +       };
>>> +
>>> +       nand-controller@60000000 {
>>
>> Where is this address? It does not work like that. If this is part of
>> SoC, then should be in DTSI and part of soc node. If not, then it is
>> some other bus which needs some description. Top-level is not a bus.
>>
> 
> It's some kind of EBI, but it doesn't need a driver it is transparent 

I did not mention any drivers. It's not really important here.


> on ts7250, the logic is controlled through installed CPLD.
> 
> The EBI it self is a part of the SoC through:

So should be in soc.

> 
> https://elixir.bootlin.com/linux/v6.5-rc3/source/arch/arm/mach-ep93xx/soc.h#L35
> 
> EP93XX_CS0_PHYS_BASE_ASYNC to EP93XX_CS0_PHYS_BASE_SYNC.
> 
> So for ts7250 this includes:
> 
> - NAND
> - m48t86
> - watchdog
> 
> I don't even know how to represent it correctly, would "simple-bus"
> with "ranges" defined suit here, so it will represent hierarchy but
> won't do anything ?

You said it is part of soc, so why shouldn't it be in the soc?

> 
>> You should see errors when testing dtbs with W=1.
> 
> Strangely - i don't see any, but anyway the above will change.



Best regards,
Krzysztof

Re: [PATCH v3 07/42] soc: Add SoC driver for Cirrus ep93xx

[Nikita Shubin]

On Fri, 2023-07-21 at 16:22 +0200, Krzysztof Kozlowski wrote:
> On 20/07/2023 13:29, Nikita Shubin via B4 Relay wrote:
> > From: Nikita Shubin <nikita.shubin@maquefel.me>
> > 
> > This adds an SoC driver for the ep93xx. Currently there
> > is only one thing not fitting into any other framework,
> > and that is the swlock setting.
> > 
> > It's used for clock settings and restart.
> > 
> > Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
> > Tested-by: Alexander Sverdlin <alexander.sverdlin@gmail.com>
> > Acked-by: Alexander Sverdlin <alexander.sverdlin@gmail.com>
> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > ---
> >  drivers/soc/Kconfig               |   1 +
> >  drivers/soc/Makefile              |   1 +
> >  drivers/soc/cirrus/Kconfig        |  12 ++
> >  drivers/soc/cirrus/Makefile       |   2 +
> >  drivers/soc/cirrus/soc-ep93xx.c   | 231
> > ++++++++++++++++++++++++++++++++++++++
> >  include/linux/soc/cirrus/ep93xx.h |  18 ++-
> >  6 files changed, 264 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
> > index 4e176280113a..16327b63b773 100644
> > --- a/drivers/soc/Kconfig
> > +++ b/drivers/soc/Kconfig
> > @@ -8,6 +8,7 @@ source "drivers/soc/aspeed/Kconfig"
> >  source "drivers/soc/atmel/Kconfig"
> >  source "drivers/soc/bcm/Kconfig"
> >  source "drivers/soc/canaan/Kconfig"
> > +source "drivers/soc/cirrus/Kconfig"
> >  source "drivers/soc/fsl/Kconfig"
> >  source "drivers/soc/fujitsu/Kconfig"
> >  source "drivers/soc/imx/Kconfig"
> > diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
> > index 3b0f9fb3b5c8..b76a03fe808e 100644
> > --- a/drivers/soc/Makefile
> > +++ b/drivers/soc/Makefile
> > @@ -9,6 +9,7 @@ obj-y                           += aspeed/
> >  obj-$(CONFIG_ARCH_AT91)                += atmel/
> >  obj-y                          += bcm/
> >  obj-$(CONFIG_SOC_CANAAN)       += canaan/
> > +obj-$(CONFIG_EP93XX_SOC)        += cirrus/
> >  obj-$(CONFIG_ARCH_DOVE)                += dove/
> >  obj-$(CONFIG_MACH_DOVE)                += dove/
> >  obj-y                          += fsl/
> > diff --git a/drivers/soc/cirrus/Kconfig
> > b/drivers/soc/cirrus/Kconfig
> > new file mode 100644
> > index 000000000000..408f3343a265
> > --- /dev/null
> > +++ b/drivers/soc/cirrus/Kconfig
> > @@ -0,0 +1,12 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +
> > +if ARCH_EP93XX
> > +
> > +config EP93XX_SOC
> > +       bool "Cirrus EP93xx chips SoC"
> > +       select SOC_BUS
> > +       default y if !EP93XX_SOC_COMMON
> > +       help
> > +         Support SoC for Cirrus EP93xx chips.
> > +
> > +endif
> > diff --git a/drivers/soc/cirrus/Makefile
> > b/drivers/soc/cirrus/Makefile
> > new file mode 100644
> > index 000000000000..ed6752844c6f
> > --- /dev/null
> > +++ b/drivers/soc/cirrus/Makefile
> > @@ -0,0 +1,2 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +obj-y  += soc-ep93xx.o
> > diff --git a/drivers/soc/cirrus/soc-ep93xx.c
> > b/drivers/soc/cirrus/soc-ep93xx.c
> > new file mode 100644
> > index 000000000000..2fd48d900f24
> > --- /dev/null
> > +++ b/drivers/soc/cirrus/soc-ep93xx.c
> > @@ -0,0 +1,231 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * SoC driver for Cirrus EP93xx chips.
> > + * Copyright (C) 2022 Nikita Shubin <nikita.shubin@maquefel.me>
> > + *
> > + * Based on a rewrite of arch/arm/mach-ep93xx/core.c
> > + * Copyright (C) 2006 Lennert Buytenhek <buytenh@wantstofly.org>
> > + * Copyright (C) 2007 Herbert Valerio Riedel <hvr@gnu.org>
> > + *
> > + * Thanks go to Michael Burian and Ray Lehtiniemi for their key
> > + * role in the ep93xx Linux community
> > + */
> > +
> > +#include <linux/clk-provider.h>
> > +#include <linux/init.h>
> > +#include <linux/kernel.h>
> > +#include <linux/mfd/syscon.h>
> > +#include <linux/of.h>
> > +#include <linux/regmap.h>
> > +#include <linux/slab.h>
> > +#include <linux/sys_soc.h>
> > +#include <linux/soc/cirrus/ep93xx.h>
> > +
> > +#define EP93XX_EXT_CLK_RATE            14745600
> > +
> > +#define EP93XX_SYSCON_DEVCFG           0x80
> > +
> > +#define EP93XX_SWLOCK_MAGICK           0xaa
> > +#define EP93XX_SYSCON_SWLOCK           0xc0
> > +#define EP93XX_SYSCON_SYSCFG           0x9c
> > +#define EP93XX_SYSCON_SYSCFG_REV_MASK  0xf0000000
> > +#define EP93XX_SYSCON_SYSCFG_REV_SHIFT 28
> > +
> > +#define EP93XX_SYSCON_CLKSET1          0x20
> > +#define EP93XX_SYSCON_CLKSET1_NBYP1    BIT(23)
> > +#define EP93XX_SYSCON_CLKSET2          0x24
> > +#define EP93XX_SYSCON_CLKSET2_NBYP2    BIT(19)
> > +#define EP93XX_SYSCON_CLKSET2_PLL2_EN  BIT(18)
> > +
> > +static DEFINE_SPINLOCK(ep93xx_swlock);
> > +
> > +/* EP93xx System Controller software locked register write */
> > +void ep93xx_syscon_swlocked_write(struct regmap *map, unsigned int
> > reg, unsigned int val)
> > +{
> > +       unsigned long flags;
> > +
> > +       spin_lock_irqsave(&ep93xx_swlock, flags);
> > +
> > +       regmap_write(map, EP93XX_SYSCON_SWLOCK,
> > EP93XX_SWLOCK_MAGICK);
> > +       regmap_write(map, reg, val);
> > +
> > +       spin_unlock_irqrestore(&ep93xx_swlock, flags);
> > +}
> > +EXPORT_SYMBOL_NS_GPL(ep93xx_syscon_swlocked_write, EP93XX_SOC);
> 
> I doubt that your code compiles. Didn't you add a user of this in
> some
> earlier patch?
> 
> Anyway, no, drop it, don't export some weird calls from core initcall
> to
> drivers. You violate layering and driver encapsulation. There is no
> dependency/probe ordering.
> 
> There is no even need for this, because this code does not use it!

It's a little emotional, so i can hardly understand the exact reason of
dissatisfaction.

Are you against usage of EXPORT_SYMBOL_NS_GPL ? - then indeed my fault
it's not needed as both PINCTRL and CLK (the only users of this code)
can't be built as modules.

> 
> > +
> > +void ep93xx_devcfg_set_clear(struct regmap *map, unsigned int
> > set_bits, unsigned int clear_bits)
> > +{
> > +       unsigned long flags;
> > +       unsigned int val;
> > +
> > +       spin_lock_irqsave(&ep93xx_swlock, flags);
> > +
> > +       regmap_read(map, EP93XX_SYSCON_DEVCFG, &val);
> > +       val &= ~clear_bits;
> > +       val |= set_bits;
> > +       regmap_write(map, EP93XX_SYSCON_SWLOCK,
> > EP93XX_SWLOCK_MAGICK);
> > +       regmap_write(map, EP93XX_SYSCON_DEVCFG, val);
> > +
> > +       spin_unlock_irqrestore(&ep93xx_swlock, flags);
> > +}
> > +EXPORT_SYMBOL_NS_GPL(ep93xx_devcfg_set_clear, EP93XX_SOC);
> 
> No.
> 
> > +
> > +void ep93xx_swlocked_update_bits(struct regmap *map, unsigned int
> > reg,
> > +                                unsigned int mask, unsigned int
> > val)
> > +{
> > +       unsigned long flags;
> > +       unsigned int tmp, orig;
> > +
> > +       spin_lock_irqsave(&ep93xx_swlock, flags);
> > +
> > +       regmap_read(map, EP93XX_SYSCON_DEVCFG, &orig);
> > +       tmp = orig & ~mask;
> > +       tmp |= val & mask;
> > +       if (tmp != orig) {
> > +               regmap_write(map, EP93XX_SYSCON_SWLOCK,
> > EP93XX_SWLOCK_MAGICK);
> > +               regmap_write(map, reg, tmp);
> > +       }
> > +
> > +       spin_unlock_irqrestore(&ep93xx_swlock, flags);
> > +}
> > +EXPORT_SYMBOL_NS_GPL(ep93xx_swlocked_update_bits, EP93XX_SOC);
> 
> No.
> 
> 
> > +
> > +unsigned int __init ep93xx_chip_revision(struct regmap *map)
> 
> Why this is visible outside? This should be static.

Indeed.

> 
> > +{
> > +       unsigned int val;
> > +
> > +       regmap_read(map, EP93XX_SYSCON_SYSCFG, &val);
> > +       val &= EP93XX_SYSCON_SYSCFG_REV_MASK;
> > +       val >>= EP93XX_SYSCON_SYSCFG_REV_SHIFT;
> > +       return val;
> > +}
> 
> 
> > +
> > +static const char __init *ep93xx_get_soc_rev(struct regmap *map)
> > +{
> > +       int rev = ep93xx_chip_revision(map);
> > +
> > +       switch (rev) {
> > +       case EP93XX_CHIP_REV_D0:
> > +               return "D0";
> > +       case EP93XX_CHIP_REV_D1:
> > +               return "D1";
> > +       case EP93XX_CHIP_REV_E0:
> > +               return "E0";
> > +       case EP93XX_CHIP_REV_E1:
> > +               return "E1";
> > +       case EP93XX_CHIP_REV_E2:
> > +               return "E2";
> > +       default:
> > +               return "unknown";
> > +       }
> > +}
> > +
> > +/*
> > + * PLL rate = 14.7456 MHz * (X1FBD + 1) * (X2FBD + 1) / (X2IPD +
> > 1) / 2^PS
> > + */
> > +static unsigned long __init calc_pll_rate(u64 rate, u32
> > config_word)
> > +{
> > +       rate *= ((config_word >> 11) & GENMASK(4, 0)) + 1;      /*
> > X1FBD */
> > +       rate *= ((config_word >> 5) & GENMASK(5, 0)) + 1;       /*
> > X2FBD */
> > +       do_div(rate, (config_word & GENMASK(4, 0)) + 1);        /*
> > X2IPD */
> > +       rate >>= ((config_word >> 16) & 3);                     /*
> > PS */
> > +
> > +       return rate;
> > +}
> > +
> > +static int __init ep93xx_soc_init(void)
> > +{
> > +       struct soc_device_attribute *attrs;
> > +       struct soc_device *soc_dev;
> > +       struct device_node *np;
> > +       struct regmap *map;
> > +       struct clk_hw *hw;
> > +       unsigned long clk_pll1_rate, clk_pll2_rate;
> > +       unsigned int clk_f_div, clk_h_div, clk_p_div, clk_usb_div;
> > +       const char fclk_divisors[] = { 1, 2, 4, 8, 16, 1, 1, 1 };
> > +       const char hclk_divisors[] = { 1, 2, 4, 5, 6, 8, 16, 32 };
> > +       const char pclk_divisors[] = { 1, 2, 4, 8 };
> > +       const char *machine = NULL;
> > +       u32 value;
> > +
> > +       /* Multiplatform guard, only proceed on ep93xx */
> > +       if (!of_machine_is_compatible("cirrus,ep9301"))
> > +               return 0;
> 
> This should already be a warning sign for you...
> 
> > +
> > +       map = syscon_regmap_lookup_by_compatible("cirrus,ep9301-
> > syscon");
> > +       if (IS_ERR(map))
> > +               return PTR_ERR(map);
> 
> No, not-reusable. Use devices and device nodes.
> 
> > +
> > +       /* Determine the bootloader configured pll1 rate */
> > +       regmap_read(map, EP93XX_SYSCON_CLKSET1, &value);
> > +       if (!(value & EP93XX_SYSCON_CLKSET1_NBYP1))
> > +               clk_pll1_rate = EP93XX_EXT_CLK_RATE;
> > +       else
> > +               clk_pll1_rate = calc_pll_rate(EP93XX_EXT_CLK_RATE,
> > value);
> > +
> > +       hw = clk_hw_register_fixed_rate(NULL, "pll1", "xtali", 0,
> > clk_pll1_rate);
> > +       if (IS_ERR(hw))
> > +               return PTR_ERR(hw);
> > +
> > +       /* Initialize the pll1 derived clocks */
> > +       clk_f_div = fclk_divisors[(value >> 25) & 0x7];
> > +       clk_h_div = hclk_divisors[(value >> 20) & 0x7];
> > +       clk_p_div = pclk_divisors[(value >> 18) & 0x3];
> > +
> > +       hw = clk_hw_register_fixed_factor(NULL, "fclk", "pll1", 0,
> > 1, clk_f_div);
> > +       if (IS_ERR(hw))
> > +               return PTR_ERR(hw);
> > +
> > +       hw = clk_hw_register_fixed_factor(NULL, "hclk", "pll1", 0,
> > 1, clk_h_div);
> > +       if (IS_ERR(hw))
> > +               return PTR_ERR(hw);
> > +
> > +       hw = clk_hw_register_fixed_factor(NULL, "pclk", "hclk", 0,
> > 1, clk_p_div);
> > +       if (IS_ERR(hw))
> > +               return PTR_ERR(hw);
> > +
> > +       /* Determine the bootloader configured pll2 rate */
> > +       regmap_read(map, EP93XX_SYSCON_CLKSET2, &value);
> > +       if (!(value & EP93XX_SYSCON_CLKSET2_NBYP2))
> > +               clk_pll2_rate = EP93XX_EXT_CLK_RATE;
> > +       else if (value & EP93XX_SYSCON_CLKSET2_PLL2_EN)
> > +               clk_pll2_rate = calc_pll_rate(EP93XX_EXT_CLK_RATE,
> > value);
> > +       else
> > +               clk_pll2_rate = 0;
> > +
> > +       hw = clk_hw_register_fixed_rate(NULL, "pll2", "xtali", 0,
> > clk_pll2_rate);
> > +       if (IS_ERR(hw))
> > +               return PTR_ERR(hw);
> > +
> > +       regmap_read(map, EP93XX_SYSCON_CLKSET2, &value);
> > +       clk_usb_div = (((value >> 28) & GENMASK(3, 0)) + 1);
> > +       hw = clk_hw_register_fixed_factor(NULL, "usb_clk", "pll2",
> > 0, 1, clk_usb_div);
> > +       if (IS_ERR(hw))
> > +               return PTR_ERR(hw);
> > +
> > +       attrs = kzalloc(sizeof(*attrs), GFP_KERNEL);
> > +       if (!attrs)
> > +               return -ENOMEM;
> > +
> > +       np = of_find_node_by_path("/");
> > +       of_property_read_string(np, "model", &machine);
> > +       if (machine)
> > +               attrs->machine = kstrdup(machine, GFP_KERNEL);
> > +       of_node_put(np);
> > +
> > +       attrs->family = "Cirrus Logic EP93xx";
> > +       attrs->revision = ep93xx_get_soc_rev(map);
> > +
> > +       soc_dev = soc_device_register(attrs);
> > +       if (IS_ERR(soc_dev)) {
> > +               kfree(attrs->soc_id);
> > +               kfree(attrs->serial_number);
> > +               kfree(attrs);
> > +               return PTR_ERR(soc_dev);
> > +       }
> > +
> > +       pr_info("EP93xx SoC revision %s\n", attrs->revision);
> > +
> > +       return 0;
> > +}
> > +core_initcall(ep93xx_soc_init);
> 
> That's not the way to add soc driver. You need a proper driver for it

What is a proper driver for these ? 

Even the latest additions in drivers/soc/* go with *_initcall.

The only i can see is:
 ./versatile/soc-
realview.c:132:builtin_platform_driver(realview_soc_driver);

By Linus =).

> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v3 07/42] soc: Add SoC driver for Cirrus ep93xx

[Krzysztof Kozlowski]

On 24/07/2023 17:02, Nikita Shubin wrote:
>>> +static DEFINE_SPINLOCK(ep93xx_swlock);
>>> +
>>> +/* EP93xx System Controller software locked register write */
>>> +void ep93xx_syscon_swlocked_write(struct regmap *map, unsigned int
>>> reg, unsigned int val)
>>> +{
>>> +       unsigned long flags;
>>> +
>>> +       spin_lock_irqsave(&ep93xx_swlock, flags);
>>> +
>>> +       regmap_write(map, EP93XX_SYSCON_SWLOCK,
>>> EP93XX_SWLOCK_MAGICK);
>>> +       regmap_write(map, reg, val);
>>> +
>>> +       spin_unlock_irqrestore(&ep93xx_swlock, flags);
>>> +}
>>> +EXPORT_SYMBOL_NS_GPL(ep93xx_syscon_swlocked_write, EP93XX_SOC);
>>
>> I doubt that your code compiles. Didn't you add a user of this in
>> some
>> earlier patch?
>>
>> Anyway, no, drop it, don't export some weird calls from core initcall
>> to
>> drivers. You violate layering and driver encapsulation. There is no
>> dependency/probe ordering.
>>
>> There is no even need for this, because this code does not use it!
> 
> It's a little emotional, so i can hardly understand the exact reason of
> dissatisfaction.
> 
> Are you against usage of EXPORT_SYMBOL_NS_GPL ? - then indeed my fault
> it's not needed as both PINCTRL and CLK (the only users of this code)
> can't be built as modules.

I am against any exported symbols and most of functions visible outside
of this driver.

...

>>> +
>>> +       pr_info("EP93xx SoC revision %s\n", attrs->revision);
>>> +
>>> +       return 0;
>>> +}
>>> +core_initcall(ep93xx_soc_init);
>>
>> That's not the way to add soc driver. You need a proper driver for it
> 
> What is a proper driver for these ? 

Usually platform_driver, e.g. exynos-chipid.

> 
> Even the latest additions in drivers/soc/* go with *_initcall.

Which one? Ones which call platform_driver_register()? That's quite
different story, isn't it? I don't see other recent cases, except
regulator couplers. They might be, some people send and accept poor
code, so what do you expect from us? Crappy code got in once, so more of
it can go?

> 
> The only i can see is:
>  ./versatile/soc-
> realview.c:132:builtin_platform_driver(realview_soc_driver);
> 
> By Linus =).

20 years ago module parameters were quite popular. Try to add one now, I
know what comment you will get. Just because something was added by
someone some time ago, is not a reason to do the same. Things change,
Linux changed.

Best regards,
Krzysztof

Re: [PATCH v3 07/42] soc: Add SoC driver for Cirrus ep93xx

[Arnd Bergmann]

On Mon, Jul 24, 2023, at 18:31, Krzysztof Kozlowski wrote:
> On 24/07/2023 17:02, Nikita Shubin wrote:
>>>
>>> There is no even need for this, because this code does not use it!
>> 
>> It's a little emotional, so i can hardly understand the exact reason of
>> dissatisfaction.
>> 
>> Are you against usage of EXPORT_SYMBOL_NS_GPL ? - then indeed my fault
>> it's not needed as both PINCTRL and CLK (the only users of this code)
>> can't be built as modules.
>
> I am against any exported symbols and most of functions visible outside
> of this driver.

I think it's a reasonable compromise here, this all ancient code
and we don't need to rewrite all of it as part of the DT conversion,
even if we would do it differently for a new platform.

I really just want to merge the series before Nikita runs out
of motivation to finish the work, after having done almost all
of it.

>>>> +
>>>> +       pr_info("EP93xx SoC revision %s\n", attrs->revision);
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +core_initcall(ep93xx_soc_init);
>>>
>>> That's not the way to add soc driver. You need a proper driver for it
>> 
>> What is a proper driver for these ? 
>
> Usually platform_driver, e.g. exynos-chipid.

Using a platform driver sounds like the right thing to do here,
it's cleaner and should not be hard to do if the driver just matches
the cirrus,ep9301-syscon node. I would have just merged
the v3 version, but this is something that makes sense changing
in v4.

>> Even the latest additions in drivers/soc/* go with *_initcall.
>
> Which one? Ones which call platform_driver_register()? That's quite
> different story, isn't it? I don't see other recent cases, except
> regulator couplers. They might be, some people send and accept poor
> code, so what do you expect from us? Crappy code got in once, so more of
> it can go?

That's not what is happening here, this is all part of an incremental
improvement of the existing code. If the DT bindings make sense, I'm
happy to take some shortcuts with the implementation that keep it
closer to the existing implementation and either clean them up over
time or just throw out the platform in five or ten years when the last
machines are dead.

      Arnd

Re: [PATCH v3 07/42] soc: Add SoC driver for Cirrus ep93xx

[Krzysztof Kozlowski]

On 24/07/2023 20:04, Arnd Bergmann wrote:
> 
>>>>> +
>>>>> +       pr_info("EP93xx SoC revision %s\n", attrs->revision);
>>>>> +
>>>>> +       return 0;
>>>>> +}
>>>>> +core_initcall(ep93xx_soc_init);
>>>>
>>>> That's not the way to add soc driver. You need a proper driver for it
>>>
>>> What is a proper driver for these ? 
>>
>> Usually platform_driver, e.g. exynos-chipid.
> 
> Using a platform driver sounds like the right thing to do here,
> it's cleaner and should not be hard to do if the driver just matches
> the cirrus,ep9301-syscon node. I would have just merged
> the v3 version, but this is something that makes sense changing
> in v4.
> 
>>> Even the latest additions in drivers/soc/* go with *_initcall.
>>
>> Which one? Ones which call platform_driver_register()? That's quite
>> different story, isn't it? I don't see other recent cases, except
>> regulator couplers. They might be, some people send and accept poor
>> code, so what do you expect from us? Crappy code got in once, so more of
>> it can go?
> 
> That's not what is happening here, this is all part of an incremental
> improvement of the existing code. If the DT bindings make sense, I'm
> happy to take some shortcuts with the implementation that keep it
> closer to the existing implementation and either clean them up over
> time or just throw out the platform in five or ten years when the last
> machines are dead.

I am not saying that this is happening here, but the argument that once
we took some patch is not the reason to keep going. For sure we could
cut here some slack, unlike for big SoC vendors...

Best regards,
Krzysztof

Re: [PATCH v3 07/42] soc: Add SoC driver for Cirrus ep93xx

[Nikita Shubin]

Hello Arnd, Krzysztof !

On Mon, 2023-07-24 at 20:04 +0200, Arnd Bergmann wrote:
> On Mon, Jul 24, 2023, at 18:31, Krzysztof Kozlowski wrote:
> > On 24/07/2023 17:02, Nikita Shubin wrote:
> > > > 
> > > > There is no even need for this, because this code does not use
> > > > it!
> > > 
> > > It's a little emotional, so i can hardly understand the exact
> > > reason of
> > > dissatisfaction.
> > > 
> > > Are you against usage of EXPORT_SYMBOL_NS_GPL ? - then indeed my
> > > fault
> > > it's not needed as both PINCTRL and CLK (the only users of this
> > > code)
> > > can't be built as modules.
> > 
> > I am against any exported symbols and most of functions visible
> > outside
> > of this driver.
> 
> I think it's a reasonable compromise here, this all ancient code
> and we don't need to rewrite all of it as part of the DT conversion,
> even if we would do it differently for a new platform.
> 
> I really just want to merge the series before Nikita runs out
> of motivation to finish the work, after having done almost all
> of it.

Thank you for your kind words, i think i have steam for at least for a
couple more versions.

I agree with Krzysztof, through i don't see any good options to dial
with "swlocked" registers (these means we have to write a magic value
to some register just before writing to "swlocked" register) without
exposing some functions - the only variants i can think of:

- introduce a "fake" hwlock, so the drivers will provide magic
themselves while holding the lock
- convert SYSCON, CLK, PINCTRL into "Auxiliary" (suggested by Stephen)
but this introduces more problems:
  - as AFAIK CLK can use SYSCON node - but i am not sure about PINCTRL
  - it still will have a common header for CLK and PINCTRL (no
functions - just structs however)

> 
> > > > > +
> > > > > +       pr_info("EP93xx SoC revision %s\n", attrs->revision);
> > > > > +
> > > > > +       return 0;
> > > > > +}
> > > > > +core_initcall(ep93xx_soc_init);
> > > > 
> > > > That's not the way to add soc driver. You need a proper driver
> > > > for it
> > > 
> > > What is a proper driver for these ? 
> > 
> > Usually platform_driver, e.g. exynos-chipid.
> 
> Using a platform driver sounds like the right thing to do here,
> it's cleaner and should not be hard to do if the driver just matches
> the cirrus,ep9301-syscon node. I would have just merged
> the v3 version, but this is something that makes sense changing
> in v4.

Indeed.

> 
> > > Even the latest additions in drivers/soc/* go with *_initcall.
> > 
> > Which one? Ones which call platform_driver_register()? That's quite
> > different story, isn't it? I don't see other recent cases, except
> > regulator couplers. They might be, some people send and accept poor
> > code, so what do you expect from us? Crappy code got in once, so
> > more of
> > it can go?
> 
> That's not what is happening here, this is all part of an incremental
> improvement of the existing code. If the DT bindings make sense, I'm
> happy to take some shortcuts with the implementation that keep it
> closer to the existing implementation and either clean them up over
> time or just throw out the platform in five or ten years when the
> last
> machines are dead.
> 
>       Arnd

Re: [PATCH v3 37/42] pwm: ep93xx: drop legacy pinctrl

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 628 bytes --]

On Thu, Jul 20, 2023 at 02:29:37PM +0300, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
> 
> Drop legacy gpio request/free since we are using
> pinctrl for this now.
> 
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
> Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  arch/arm/mach-ep93xx/core.c       | 42 ---------------------------------------
>  drivers/pwm/pwm-ep93xx.c          | 18 -----------------
>  include/linux/soc/cirrus/ep93xx.h |  4 ----
>  3 files changed, 64 deletions(-)

Acked-by: Thierry Reding <thierry.reding@gmail.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 03/42] clk: ep93xx: add DT support for Cirrus EP93xx

[Nikita Shubin]

Hello Stephen!

On Thu, 2023-07-20 at 16:27 -0700, Stephen Boyd wrote:
> Quoting Nikita Shubin via B4 Relay (2023-07-20 04:29:03)
> 
> > +static int clk_hw_register_div(struct ep93xx_clk *clk,
> > +                                         const char *name,
> > +                                         const char *parent_name,
> 
> Please try to pass a direct pointer to the parent clk_hw instead or
> if
> the clk exists outside the controller pass a DT index.

I'll use parent_data everywhere, however i currently can't pass "xtali"
as index, as: 

- moved all clk's that are not used via device tree to SoC driver 
https://lore.kernel.org/b4-sent/20230605-ep93xx-v3-7-3d63a5f1103e@maquefel.me/

and SoC driver isn't a OF clock provider, should i make it clock
provider as well ?

Re: [PATCH v3 07/42] soc: Add SoC driver for Cirrus ep93xx

[Nikita Shubin]

Hello Andy!

On Fri, 2023-07-21 at 17:13 +0300, Andy Shevchenko wrote:
> On Thu, Jul 20, 2023 at 02:29:07PM +0300, Nikita Shubin via B4 Relay
> wrote:
> > From: Nikita Shubin <nikita.shubin@maquefel.me>
> > +       spin_lock_irqsave(&ep93xx_swlock, flags);
> > +
> > +       regmap_read(map, EP93XX_SYSCON_DEVCFG, &val);
> > +       val &= ~clear_bits;
> > +       val |= set_bits;
> > +       regmap_write(map, EP93XX_SYSCON_SWLOCK,
> > EP93XX_SWLOCK_MAGICK);
> > +       regmap_write(map, EP93XX_SYSCON_DEVCFG, val);
> 
> Is this sequence a must?
> I.o.w. can you first supply magic and then update devcfg?
> 

Unfortunately it is a must to write EP93XX_SYSCON_SWLOCK and only then
the next write to swlocked registers will succeed.

Re: [PATCH v3 07/42] soc: Add SoC driver for Cirrus ep93xx

[Andy Shevchenko]

On Fri, Jul 28, 2023 at 12:28 PM Nikita Shubin
<nikita.shubin@maquefel.me> wrote:
>
> Hello Andy!
>
> On Fri, 2023-07-21 at 17:13 +0300, Andy Shevchenko wrote:
> > On Thu, Jul 20, 2023 at 02:29:07PM +0300, Nikita Shubin via B4 Relay
> > wrote:
> > > From: Nikita Shubin <nikita.shubin@maquefel.me>
> > > +       spin_lock_irqsave(&ep93xx_swlock, flags);
> > > +
> > > +       regmap_read(map, EP93XX_SYSCON_DEVCFG, &val);
> > > +       val &= ~clear_bits;
> > > +       val |= set_bits;
> > > +       regmap_write(map, EP93XX_SYSCON_SWLOCK,
> > > EP93XX_SWLOCK_MAGICK);
> > > +       regmap_write(map, EP93XX_SYSCON_DEVCFG, val);
> >
> > Is this sequence a must?
> > I.o.w. can you first supply magic and then update devcfg?
> >
>
> Unfortunately it is a must to write EP93XX_SYSCON_SWLOCK and only then
> the next write to swlocked registers will succeed.

This doesn't answer my question. Can you first write a magic and then
_update_ the other register (update means RMW op)?


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 07/42] soc: Add SoC driver for Cirrus ep93xx

[Nikita Shubin]

On Fri, 2023-07-28 at 12:46 +0300, Andy Shevchenko wrote:
> On Fri, Jul 28, 2023 at 12:28 PM Nikita Shubin
> <nikita.shubin@maquefel.me> wrote:
> > 
> > Hello Andy!
> > 
> > On Fri, 2023-07-21 at 17:13 +0300, Andy Shevchenko wrote:
> > > On Thu, Jul 20, 2023 at 02:29:07PM +0300, Nikita Shubin via B4
> > > Relay
> > > wrote:
> > > > From: Nikita Shubin <nikita.shubin@maquefel.me>
> > > > +       spin_lock_irqsave(&ep93xx_swlock, flags);
> > > > +
> > > > +       regmap_read(map, EP93XX_SYSCON_DEVCFG, &val);
> > > > +       val &= ~clear_bits;
> > > > +       val |= set_bits;
> > > > +       regmap_write(map, EP93XX_SYSCON_SWLOCK,
> > > > EP93XX_SWLOCK_MAGICK);
> > > > +       regmap_write(map, EP93XX_SYSCON_DEVCFG, val);
> > > 
> > > Is this sequence a must?
> > > I.o.w. can you first supply magic and then update devcfg?
> > > 
> > 
> > Unfortunately it is a must to write EP93XX_SYSCON_SWLOCK and only
> > then
> > the next write to swlocked registers will succeed.
> 
> This doesn't answer my question. Can you first write a magic and then
> _update_ the other register (update means RMW op)?
> 

I see your point now - citing docs:

"Logic safeguards are included to condition the control signals for
power connection to the matrix to prevent part damage. In addition, a
software lock register is included that must be written with 0xAA
before each register write to change the values of the four switch
matrix control registers."

So reading SHOULDN'T affect the lock.

But as we checked reading also breaks the lock, that's why this looks
so odd, it was done for purpose - i'll check it once again anyway.

Re: [PATCH v3 14/42] power: reset: Add a driver for the ep93xx reset

[Nikita Shubin]

On Fri, 2023-07-21 at 19:37 +0300, Andy Shevchenko wrote:
> On Thu, Jul 20, 2023 at 02:29:14PM +0300, Nikita Shubin via B4 Relay
> wrote:
> > From: Nikita Shubin <nikita.shubin@maquefel.me>
> > 
> > Implement the reset behaviour of the various EP93xx SoCS in
> > drivers/power/reset.
> > 
> > It used to be located in arch/arm/mach-ep93xx.
> 
> ...
> 
> > +// SPDX-License-Identifier: (GPL-2.0)
> 
> Are you sure this is correct form? 

Should it be // SPDX-License-Identifier: GPL-2.0+ ?

> Have you checked your patches?

Could you please be more specific:
$ scripts/checkpatch.pl -f drivers/power/reset/ep93xx-restart.c
total: 0 errors, 0 warnings, 86 lines checked

$ git format-patch -1 51f03c64b8fde79fb16b146d87769b7508b6d114 --stdout
| scripts/checkpatch.pl -
WARNING: please write a help paragraph that fully describes the config
symbol
...
WARNING: added, moved or deleted file(s), does MAINTAINERS need
updating?

I don't see any license complains...

checkpatch.pl is working as intented as:

$ scripts/checkpatch.pl -f drivers/power/reset/ep93xx-restart.c
WARNING: 'SPDX-License-Identifier: (FOOOO)' is not supported in
LICENSES/...
#1: FILE: drivers/power/reset/ep93xx-restart.c:1:
+// SPDX-License-Identifier: (FOOOO)

> 
> ...
> 
> > +#include <linux/of_device.h>
> 
> Do you need this?
> Or maybe you need another (of*.h) one?
> 
> ...
> 
> > +       /* Issue the reboot */
> > +       ep93xx_devcfg_set_clear(priv->map,
> > EP93XX_SYSCON_DEVCFG_SWRST, 0x00);
> > +       ep93xx_devcfg_set_clear(priv->map, 0x00,
> > EP93XX_SYSCON_DEVCFG_SWRST);
> 
> 
> > +       mdelay(1000);
> 
> Atomic?! Such a huge delay must be explained, esp. why it's atomic.

Indeed let's drop it entirely.

> 
> > +       pr_emerg("Unable to restart system\n");
> > +       return NOTIFY_DONE;
> 
> ...
> 
> > +       err = register_restart_handler(&priv->restart_handler);
> > +       if (err)
> > +               return dev_err_probe(dev, err, "can't register
> > restart notifier\n");
> 
> > +       return err;
> 
>         return 0;
> 

Re: [PATCH v3 35/42] ARM: dts: ep93xx: add ts7250 board

[Linus Walleij]

On Mon, Jul 24, 2023 at 3:45 PM Nikita Shubin <nikita.shubin@maquefel.me> wrote:

> > > +       nand-controller@60000000 {
> >
> > Where is this address? It does not work like that. If this is part of
> > SoC, then should be in DTSI and part of soc node. If not, then it is
> > some other bus which needs some description. Top-level is not a bus.
> >
>
> It's some kind of EBI, but it doesn't need a driver it is transparent
> on ts7250, the logic is controlled through installed CPLD.
>
> The EBI it self is a part of the SoC through:
>
> https://elixir.bootlin.com/linux/v6.5-rc3/source/arch/arm/mach-ep93xx/soc.h#L35
>
> EP93XX_CS0_PHYS_BASE_ASYNC to EP93XX_CS0_PHYS_BASE_SYNC.
>
> So for ts7250 this includes:
>
> - NAND
> - m48t86
> - watchdog
>
> I don't even know how to represent it correctly, would "simple-bus"
> with "ranges" defined suit here, so it will represent hierarchy but
> won't do anything ?

Check how I solved this on the IXP4xx EBI for an example:
Documentation/devicetree/bindings/memory-controllers/intel,ixp4xx-expansion-bus-controller.yaml

Top level bus inside soc:
arch/arm/boot/dts/intel/ixp/intel-ixp4xx.dtsi
Example platform:
arch/arm/boot/dts/intel/ixp/intel-ixp42x-linksys-nslu2.dts

Notice chip select number in first cell.

I think you want to do something similar here?

Yours,
Linus Walleij

Re: [PATCH v3 35/42] ARM: dts: ep93xx: add ts7250 board

[Nikita Shubin]

Hello Linus!

On Sat, 2023-07-29 at 22:59 +0200, Linus Walleij wrote:
> On Mon, Jul 24, 2023 at 3:45 PM Nikita Shubin
> <nikita.shubin@maquefel.me> wrote:
> 
> > > > +       nand-controller@60000000 {
> > > 
> > > Where is this address? It does not work like that. If this is
> > > part of
> > > SoC, then should be in DTSI and part of soc node. If not, then it
> > > is
> > > some other bus which needs some description. Top-level is not a
> > > bus.
> > > 
> > 
> > It's some kind of EBI, but it doesn't need a driver it is
> > transparent
> > on ts7250, the logic is controlled through installed CPLD.
> > 
> > The EBI it self is a part of the SoC through:
> > 
> > https://elixir.bootlin.com/linux/v6.5-rc3/source/arch/arm/mach-ep93xx/soc.h#L35
> > 
> > EP93XX_CS0_PHYS_BASE_ASYNC to EP93XX_CS0_PHYS_BASE_SYNC.
> > 
> > So for ts7250 this includes:
> > 
> > - NAND
> > - m48t86
> > - watchdog
> > 
> > I don't even know how to represent it correctly, would "simple-bus"
> > with "ranges" defined suit here, so it will represent hierarchy but
> > won't do anything ?
> 
> Check how I solved this on the IXP4xx EBI for an example:
> Documentation/devicetree/bindings/memory-controllers/intel,ixp4xx-
> expansion-bus-controller.yaml
> 
> Top level bus inside soc:
> arch/arm/boot/dts/intel/ixp/intel-ixp4xx.dtsi
> Example platform:
> arch/arm/boot/dts/intel/ixp/intel-ixp42x-linksys-nslu2.dts
> 
> Notice chip select number in first cell.
> 
> I think you want to do something similar here?

Thank you - it looks like what i need !

> 
> Yours,
> Linus Walleij

Re: [PATCH v3 07/42] soc: Add SoC driver for Cirrus ep93xx

[Andy Shevchenko]

On Fri, Jul 28, 2023 at 01:01:11PM +0300, Nikita Shubin wrote:
> On Fri, 2023-07-28 at 12:46 +0300, Andy Shevchenko wrote:

...

> I see your point now - citing docs:
> 
> "Logic safeguards are included to condition the control signals for
> power connection to the matrix to prevent part damage. In addition, a
> software lock register is included that must be written with 0xAA
> before each register write to change the values of the four switch
> matrix control registers."
> 
> So reading SHOULDN'T affect the lock.
> 
> But as we checked reading also breaks the lock, that's why this looks
> so odd, it was done for purpose - i'll check it once again anyway.

This is very interesting information! Please, document this somewhere in
the code.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 14/42] power: reset: Add a driver for the ep93xx reset

[Andy Shevchenko]

On Fri, Jul 28, 2023 at 04:53:19PM +0300, Nikita Shubin wrote:
> On Fri, 2023-07-21 at 19:37 +0300, Andy Shevchenko wrote:
> > On Thu, Jul 20, 2023 at 02:29:14PM +0300, Nikita Shubin via B4 Relay
> > wrote:

...

> > > +// SPDX-License-Identifier: (GPL-2.0)
> > 
> > Are you sure this is correct form? 
> 
> Should it be // SPDX-License-Identifier: GPL-2.0+ ?

I don't know, ask your Legal department or your lawyer.

(note that the 2.0-only and 2.0-or-later are different)

I'm talking only about the form, not about the licence itself.

> > Have you checked your patches?
> 
> Could you please be more specific:
> $ scripts/checkpatch.pl -f drivers/power/reset/ep93xx-restart.c
> total: 0 errors, 0 warnings, 86 lines checked
> 
> $ git format-patch -1 51f03c64b8fde79fb16b146d87769b7508b6d114 --stdout
> | scripts/checkpatch.pl -
> WARNING: please write a help paragraph that fully describes the config
> symbol
> ...
> WARNING: added, moved or deleted file(s), does MAINTAINERS need
> updating?
> 
> I don't see any license complains...

checkpatch is not the ideal tool...

> checkpatch.pl is working as intented as:
> 
> $ scripts/checkpatch.pl -f drivers/power/reset/ep93xx-restart.c
> WARNING: 'SPDX-License-Identifier: (FOOOO)' is not supported in
> LICENSES/...
> #1: FILE: drivers/power/reset/ep93xx-restart.c:1:
> +// SPDX-License-Identifier: (FOOOO)

https://kernel.org/doc/html/latest/process/license-rules.html
doesn't mention your format.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 07/42] soc: Add SoC driver for Cirrus ep93xx

[Linus Walleij]

On Mon, Jul 24, 2023 at 8:04 PM Arnd Bergmann <arnd@arndb.de> wrote:

> I really just want to merge the series before Nikita runs out
> of motivation to finish the work, after having done almost all
> of it.

I feel the same. At some point I think the SoC maintainers
need to ask the question "does the kernel look better after the
series than before" and if yes, merge it and propose a list
of improvements to come later. Given how persistent Nikita
has been already, I pretty much trust him to fix rough edges
later if need be.

I tend to agree that DT bindings must be "just right" though,
since once merged they cannot really change, that is a fair
point.

Yours,
Linus Walleij

Re: [PATCH v3 29/42] dt-bindings: rtc: Add ST M48T86

[Krzysztof Kozlowski]

On 20/07/2023 13:29, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
> 
> Add YAML bindings for ST M48T86 / Dallas DS12887 RTC.
> 
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v3 14/42] power: reset: Add a driver for the ep93xx reset

[Alexander Sverdlin]

Hi Andy,

On Fri, 2023-07-21 at 19:37 +0300, Andy Shevchenko wrote:
> > +       /* Issue the reboot */
> > +       ep93xx_devcfg_set_clear(priv->map, EP93XX_SYSCON_DEVCFG_SWRST, 0x00);
> > +       ep93xx_devcfg_set_clear(priv->map, 0x00, EP93XX_SYSCON_DEVCFG_SWRST);
> 
> 
> > +       mdelay(1000);
> 
> Atomic?! Such a huge delay must be explained, esp. why it's atomic.

atomic or not, SoC is supposed to reset itself here.
However there is an errata [1] and the SoC can lockup instead.
So even pr_emerg() makes sense to me.

> > +       pr_emerg("Unable to restart system\n");
> > +       return NOTIFY_DONE;

[1] http://web.archive.org/web/20161130230727/http://www.cirrus.com/en/pubs/appNote/AN258REV2.pdf

-- 
Alexander Sverdlin.

Re: [PATCH v3 07/42] soc: Add SoC driver for Cirrus ep93xx

[Alexander Sverdlin]

Hello Andy,

On Fri, 2023-07-21 at 17:13 +0300, Andy Shevchenko wrote:
> > +       spin_lock_irqsave(&ep93xx_swlock, flags);
> > +
> > +       regmap_read(map, EP93XX_SYSCON_DEVCFG, &val);
> > +       val &= ~clear_bits;
> > +       val |= set_bits;
> > +       regmap_write(map, EP93XX_SYSCON_SWLOCK, EP93XX_SWLOCK_MAGICK);
> > +       regmap_write(map, EP93XX_SYSCON_DEVCFG, val);
> 
> Is this sequence a must?
> I.o.w. can you first supply magic and then update devcfg?
> 
> > +       spin_unlock_irqrestore(&ep93xx_swlock, flags);
> 
> ...
> 
> > +void ep93xx_swlocked_update_bits(struct regmap *map, unsigned int reg,
> > +                                unsigned int mask, unsigned int val)
> > +{
> 
> Same Q as above.

EP93xx User Manual [1] has most verbose description of SWLock for ADC
block:
"Writing 0xAA to this register will unlock all locked registers until the
next block access. The ARM lock instruction prefix should be used for the
two consequtive write cycles when writing to locked chip registers."

One may conclude that RmW (two accesses to the particular block) sequence
is not appropriate.

[1] https://cdn.embeddedts.com/resource-attachments/ts-7000_ep9301-ug.pdf 

-- 
Alexander Sverdlin.

Re: [PATCH v3 14/42] power: reset: Add a driver for the ep93xx reset

[Andy Shevchenko]

On Sat, Nov 11, 2023 at 8:18 PM Alexander Sverdlin
<alexander.sverdlin@gmail.com> wrote:
> On Fri, 2023-07-21 at 19:37 +0300, Andy Shevchenko wrote:

...

> > > +       mdelay(1000);
> >
> > Atomic?! Such a huge delay must be explained, esp. why it's atomic.
>
> atomic or not, SoC is supposed to reset itself here.
> However there is an errata [1] and the SoC can lockup instead.

Good, and what I'm saying is that this piece of code must have a
comment explaining this.

> So even pr_emerg() makes sense to me.

This is irrelevant to the comment.

> > > +       pr_emerg("Unable to restart system\n");
> > > +       return NOTIFY_DONE;
>
> [1] http://web.archive.org/web/20161130230727/http://www.cirrus.com/en/pubs/appNote/AN258REV2.pdf

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 14/42] power: reset: Add a driver for the ep93xx reset

[Alexander Sverdlin]

Hi Andy!

On Mon, 2023-11-13 at 11:59 +0200, Andy Shevchenko wrote:
> On Sat, Nov 11, 2023 at 8:18 PM Alexander Sverdlin
> <alexander.sverdlin@gmail.com> wrote:
> > On Fri, 2023-07-21 at 19:37 +0300, Andy Shevchenko wrote:
> 
> ...

[1]

> 
> > > > +       mdelay(1000);
> > > 
> > > Atomic?! Such a huge delay must be explained, esp. why it's atomic.
> > 
> > atomic or not, SoC is supposed to reset itself here.
> > However there is an errata [1] and the SoC can lockup instead.
> 
> Good, and what I'm saying is that this piece of code must have a
> comment explaining this.

And it has, but for some reason you've trimmed it in your reply...

-- 
Alexander Sverdlin.

Re: [PATCH v3 14/42] power: reset: Add a driver for the ep93xx reset

[Alexander Sverdlin]

Hi Andy!

On Fri, 2023-07-21 at 19:37 +0300, Andy Shevchenko wrote:
> > +       /* Issue the reboot */
            ^^^^^^^^^^^^^^^^^^^^^^
This is the relevant comment, one can extend it, but looks already quite
informative considering EP93XX_SYSCON_DEVCFG_SWRST register name.

But Nikita would be able to include more verbose comment if
you'd have a suggestion.

> > +       ep93xx_devcfg_set_clear(priv->map, EP93XX_SYSCON_DEVCFG_SWRST, 0x00);
> > +       ep93xx_devcfg_set_clear(priv->map, 0x00, EP93XX_SYSCON_DEVCFG_SWRST);
> 
> 
> > +       mdelay(1000);
> 
> Atomic?! Such a huge delay must be explained, esp. why it's atomic.

-- 
Alexander Sverdlin.

Re: [PATCH v3 07/42] soc: Add SoC driver for Cirrus ep93xx

[Andy Shevchenko]

On Sat, Nov 11, 2023 at 11:33 PM Alexander Sverdlin
<alexander.sverdlin@gmail.com> wrote:
> On Fri, 2023-07-21 at 17:13 +0300, Andy Shevchenko wrote:

...

> > > +       spin_lock_irqsave(&ep93xx_swlock, flags);
> > > +
> > > +       regmap_read(map, EP93XX_SYSCON_DEVCFG, &val);
> > > +       val &= ~clear_bits;
> > > +       val |= set_bits;
> > > +       regmap_write(map, EP93XX_SYSCON_SWLOCK, EP93XX_SWLOCK_MAGICK);
> > > +       regmap_write(map, EP93XX_SYSCON_DEVCFG, val);
> >
> > Is this sequence a must?
> > I.o.w. can you first supply magic and then update devcfg?
> >
> > > +       spin_unlock_irqrestore(&ep93xx_swlock, flags);

...

> > > +void ep93xx_swlocked_update_bits(struct regmap *map, unsigned int reg,
> > > +                                unsigned int mask, unsigned int val)

> > Same Q as above.
>
> EP93xx User Manual [1] has most verbose description of SWLock for ADC
> block:
> "Writing 0xAA to this register will unlock all locked registers until the
> next block access. The ARM lock instruction prefix should be used for the
> two consequtive write cycles when writing to locked chip registers."
>
> One may conclude that RmW (two accesses to the particular block) sequence
> is not appropriate.

It's not obvious to me. The terms "block access" and "block cycle"
occur only once in the very same section that describes ADCSWLock. The
meaning of these terms is not fully understandable. So, assuming that
you have tried it differently and it indeed doesn't work, let's go
with this one.

> [1] https://cdn.embeddedts.com/resource-attachments/ts-7000_ep9301-ug.pdf



--
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 14/42] power: reset: Add a driver for the ep93xx reset

[Andy Shevchenko]

On Mon, Nov 13, 2023 at 12:07 PM Alexander Sverdlin
<alexander.sverdlin@gmail.com> wrote:
> On Fri, 2023-07-21 at 19:37 +0300, Andy Shevchenko wrote:
> > > +       /* Issue the reboot */
>             ^^^^^^^^^^^^^^^^^^^^^^
> This is the relevant comment, one can extend it, but looks already quite
> informative considering EP93XX_SYSCON_DEVCFG_SWRST register name.

This does not explain the necessity of the mdelay() below.

> But Nikita would be able to include more verbose comment if
> you'd have a suggestion.

Please,add one.

> > > +       ep93xx_devcfg_set_clear(priv->map, EP93XX_SYSCON_DEVCFG_SWRST, 0x00);
> > > +       ep93xx_devcfg_set_clear(priv->map, 0x00, EP93XX_SYSCON_DEVCFG_SWRST);
> >
> >
> > > +       mdelay(1000);
> >
> > Atomic?! Such a huge delay must be explained, esp. why it's atomic.

-- 
With Best Regards,
Andy Shevchenko