Re: [PATCH v2] tegra2/tegra3 automotive clock init

Re: [PATCH v2] tegra2/tegra3 automotive clock init

[Dmitry Osipenko]

01.03.2019 18:35, Kejia Hu пишет:
> This is a re-implementation of adding clock initialization for
> tegra2 and tegra3 automotive devices accroding to Dmitry Osipenko's
> feedback on version 1[0].
> 
> We have moved the initialization from the clock driver to device
> tree, but instead of putting them into a dtsi file, we used
> dt overlay to patch the device tree at runtime, we believe the
> ability to dynamically detect the chipset and apply the right
> settings should be useful according to the discussion[1].
> 
> Changes against v1:
>   - use device tree overlay to introduce the automative clock configs
>   - enable dt overlay for tegra20/30
>   - move the condition "soc_is_tegra_auto" to be after applied
>     clock init table.
> 
> [0] https://www.spinics.net/lists/linux-tegra/msg38347.html
> [1] https://www.spinics.net/lists/arm-kernel/msg665373.html
> 

Hello Kejia,

Thank you very much for your work! My point was that the clock-assignments should be moved into the board's DT file out from the kernel's driver, in this series you're not moving out the board-specific properties from the kernel sources and essential doing the same thing that was in the previous versions in a different way now. Please take a look at tegra124.dtsi / tegra124-nyan.dtsi / tegra124-nyan-big.dts for the example, in your case it could be something similar.. somewhat like tegra20.dtsi / tegra20-automotive.dtsi / tegra20-automotive-board.dts. Hence you won't need to touch kernel's code at all and could specify everything in the device-tree solely.

If you're trying to workaround the case where device-tree isn't easily changeable, then I'm afraid this is not going to work well in regards to upstreaming because upstream maintainers have no interest in supporting downstream. If it's not that case, then please try to explain what you're trying to achieve in more detail.

In overall you'll need to:

1) Upstream device-tree of the board.
2) Get all other required bits of kernel drivers upstream'ed.
3) Issue firmware update for the device that will contain new upstream-conformant device-tree and upstream kernel at once. Also note that kernel supports appending DTB (compiled device-tree binary) to the kernel's image, hence you may workaround locked-down bootloader that doesn't allow to replace (or doesn't support) device-tree.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/6] soc/tegra: initial tegra-automotive detection

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2338 bytes --]

On Fri, Mar 01, 2019 at 03:35:35PM +0000, Kejia Hu wrote:
> From: Ben Dooks <ben.dooks@codethink.co.uk>
> 
> Add an initial soc_is_tegra_auto() with detection
> via a change in the device-tree.
> 
> Also print the path taken through soc_is_tegra_auto() to
> allow debugging. Only print when debug is enabled as this
> function may be be called from multiple places, resulting
> in duplicated messages in production.
> 
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> Signed-off-by: Thomas Preston <thomas.preston@codethink.co.uk>
> Signed-off-by: Kejia Hu <kejia.hu@codethink.co.uk>
> ---
>  drivers/soc/tegra/common.c             | 23 +++++++++++++++++++++++
>  drivers/soc/tegra/fuse/tegra-apbmisc.c |  2 ++
>  include/soc/tegra/common.h             |  1 +
>  include/soc/tegra/fuse.h               |  1 +
>  4 files changed, 27 insertions(+)
> 
> diff --git a/drivers/soc/tegra/common.c b/drivers/soc/tegra/common.c
> index 7bfb154d6fa5..a10bd26fb5df 100644
> --- a/drivers/soc/tegra/common.c
> +++ b/drivers/soc/tegra/common.c
> @@ -9,6 +9,7 @@
>  #include <linux/of.h>
>  
>  #include <soc/tegra/common.h>
> +#include <soc/tegra/fuse.h>
>  
>  static const struct of_device_id tegra_machine_match[] = {
>  	{ .compatible = "nvidia,tegra20", },
> @@ -34,3 +35,25 @@ bool soc_is_tegra(void)
>  
>  	return match != NULL;
>  }
> +
> +static const struct of_device_id tegra_machine_match_auto[] = {
> +	{ .compatible = "nvidia,tegra20auto", },
> +	{ .compatible = "nvidia,tegra30auto", },
> +	{ },
> +};
> +
> +bool soc_is_tegra_auto(void)
> +{
> +	struct device_node *root;
> +	bool id_match  = false;
> +
> +	root = of_find_node_by_path("/");
> +
> +	if (root && of_match_node(tegra_machine_match_auto, root))
> +		id_match = true;
> +
> +	pr_debug("%s of_device_id match %d, tegra_sku_info.is_automotive %d\n",
> +			__func__, id_match, tegra_sku_info.is_automotive);
> +
> +	return id_match || tegra_sku_info.is_automotive;
> +}

Why do we need two ways of specifying the same thing? If we can read
this information from fuses, I don't think we need the extra compatible
string matching.

Also, if you're going to use this exclusively for clock setup, then the
alternative is to do that as part of the board's device tree, as Dmitry
pointed out.

Thierry

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel