| Message ID | 1468935397-11926-7-git-send-email-mirza.krak@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 19/07/16 14:36, Mirza Krak wrote: > From: Mirza Krak <mirza.krak@gmail.com> > > The NOR bus can be used to connect high-speed devices such as NOR flash, > FPGAs, DSPs, CAN chips, Wi-Fi chips etc. > > Signed-off-by: Mirza Krak <mirza.krak@gmail.com> > --- > drivers/bus/Kconfig | 7 +++ > drivers/bus/Makefile | 1 + > drivers/bus/tegra-nor.c | 118 ++++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 126 insertions(+) > create mode 100644 drivers/bus/tegra-nor.c > > diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig > index 3b205e2..b74be7d8 100644 > --- a/drivers/bus/Kconfig > +++ b/drivers/bus/Kconfig > @@ -145,6 +145,13 @@ config TEGRA_ACONNECT > Driver for the Tegra ACONNECT bus which is used to interface with > the devices inside the Audio Processing Engine (APE) for Tegra210. > > +config TEGRA_NOR > + tristate "Nvidia Tegra NOR flash bus driver a.k.a GMI/SNOR" > + depends on ARCH_TEGRA_2x_SOC || ARCH_TEGRA_3x_SOC > + depends on OF > + help > + Driver for NOR flash bus found on Tegra30/20 SOC`s. It is actually present on all Tegra's and so I would drop the 30/20. > + > config UNIPHIER_SYSTEM_BUS > tristate "UniPhier System Bus driver" > depends on ARCH_UNIPHIER && OF > diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile > index ac84cc4..46d0129 100644 > --- a/drivers/bus/Makefile > +++ b/drivers/bus/Makefile > @@ -18,5 +18,6 @@ obj-$(CONFIG_OMAP_OCP2SCP) += omap-ocp2scp.o > obj-$(CONFIG_SUNXI_RSB) += sunxi-rsb.o > obj-$(CONFIG_SIMPLE_PM_BUS) += simple-pm-bus.o > obj-$(CONFIG_TEGRA_ACONNECT) += tegra-aconnect.o > +obj-$(CONFIG_TEGRA_NOR) += tegra-nor.o > obj-$(CONFIG_UNIPHIER_SYSTEM_BUS) += uniphier-system-bus.o > obj-$(CONFIG_VEXPRESS_CONFIG) += vexpress-config.o > diff --git a/drivers/bus/tegra-nor.c b/drivers/bus/tegra-nor.c > new file mode 100644 > index 0000000..1e48113 > --- /dev/null > +++ b/drivers/bus/tegra-nor.c > @@ -0,0 +1,118 @@ > +/* > + * Driver for Nvidia NOR Flash bus a.k.a SNOR/GMI. > + * > + * Copyright (C) 2016 Host Mobility AB. All rights reserved. > + * > + * This file is licensed under the terms of the GNU General Public > + * License version 2. This program is licensed "as is" without any > + * warranty of any kind, whether express or implied. > + */ > + > +#include <linux/clk.h> > +#include <linux/io.h> > +#include <linux/mfd/syscon.h> Is this needed? > +#include <linux/module.h> > +#include <linux/of_device.h> > +#include <linux/regmap.h> Or this? > + > +#define TEGRA_NOR_TIMING_REG_COUNT 2 > + > +#define TEGRA_NOR_CONFIG 0x00 > +#define TEGRA_NOR_STATUS 0x04 > +#define TEGRA_NOR_ADDR_PTR 0x08 > +#define TEGRA_NOR_AHB_ADDR_PTR 0x0c > +#define TEGRA_NOR_TIMING0 0x10 > +#define TEGRA_NOR_TIMING1 0x14 > +#define TEGRA_NOR_MIO_CONFIG 0x18 > +#define TEGRA_NOR_MIO_TIMING 0x1c > +#define TEGRA_NOR_DMA_CONFIG 0x20 > +#define TEGRA_NOR_CS_MUX_CONFIG 0x24 Not all of these are used. It is good to define them and I wonder if we should add support for MIO while are at it :-) > + > +#define TEGRA_NOR_CONFIG_GO BIT(31) > + > +static const struct of_device_id nor_id_table[] = { > + /* Tegra30 */ I don't think this comment is needed. > + { .compatible = "nvidia,tegra30-nor", .data = NULL, }, You don't need to set data to NULL. > + /* Tegra20 */ > + { .compatible = "nvidia,tegra20-nor", .data = NULL, }, Same here. > + > + { } > +}; > +MODULE_DEVICE_TABLE(of, nor_id_table); > + > + > +static int __init nor_parse_dt(struct platform_device *pdev, > + void __iomem *base) > +{ > + struct device_node *of_node = pdev->dev.of_node; > + u32 config, timing[TEGRA_NOR_TIMING_REG_COUNT]; > + int ret; > + > + ret = of_property_read_u32_array(of_node, "nvidia,cs-timing", > + timing, TEGRA_NOR_TIMING_REG_COUNT); > + if (!ret) { > + writel(timing[0], base + TEGRA_NOR_TIMING0); > + writel(timing[1], base + TEGRA_NOR_TIMING1); > + } > + > + ret = of_property_read_u32(of_node, "nvidia,config", &config); > + if (ret) > + return ret; > + > + config |= TEGRA_NOR_CONFIG_GO; > + writel(config, base + TEGRA_NOR_CONFIG); > + > + if (of_get_child_count(of_node)) > + ret = of_platform_populate(of_node, > + of_default_bus_match_table, > + NULL, &pdev->dev); > + > + return ret; > +} > + > +static int __init nor_probe(struct platform_device *pdev) > +{ I would drop the __init. > + struct resource *res; > + struct clk *clk; > + void __iomem *base; > + int ret; > + > + /* get the resource */ > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + base = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(base)) > + return PTR_ERR(base); > + > + /* get the clock */ > + clk = devm_clk_get(&pdev->dev, NULL); > + if (IS_ERR(clk)) > + return PTR_ERR(clk); > + > + ret = clk_prepare_enable(clk); > + if (ret) > + return ret; > + > + /* parse the device node */ > + ret = nor_parse_dt(pdev, base); > + if (ret) { > + dev_err(&pdev->dev, "%s fail to create devices.\n", > + pdev->dev.of_node->full_name); > + clk_disable_unprepare(clk); > + return ret; > + } > + > + dev_info(&pdev->dev, "Driver registered.\n"); > + return 0; > +} > + > +static struct platform_driver nor_driver = { > + .driver = { > + .name = "tegra-nor", > + .of_match_table = nor_id_table, > + }, > +}; The driver should have a remove function given that we can build as a module. > +module_platform_driver_probe(nor_driver, nor_probe); I would use "tegra_nor" namespace for all the structs, functions, etc. However, we may prefer to go with GMI and in which case tegra_gmi_probe, etc. Cheers Jon
On 19/07/16 14:36, Mirza Krak wrote: > From: Mirza Krak <mirza.krak@gmail.com> > > The NOR bus can be used to connect high-speed devices such as NOR flash, > FPGAs, DSPs, CAN chips, Wi-Fi chips etc. > > Signed-off-by: Mirza Krak <mirza.krak@gmail.com> > --- > drivers/bus/Kconfig | 7 +++ > drivers/bus/Makefile | 1 + > drivers/bus/tegra-nor.c | 118 ++++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 126 insertions(+) > create mode 100644 drivers/bus/tegra-nor.c ... > +static int __init nor_probe(struct platform_device *pdev) > +{ > + struct resource *res; > + struct clk *clk; > + void __iomem *base; > + int ret; > + > + /* get the resource */ > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + base = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(base)) > + return PTR_ERR(base); > + > + /* get the clock */ > + clk = devm_clk_get(&pdev->dev, NULL); > + if (IS_ERR(clk)) > + return PTR_ERR(clk); > + > + ret = clk_prepare_enable(clk); > + if (ret) > + return ret; > + > + /* parse the device node */ > + ret = nor_parse_dt(pdev, base); > + if (ret) { > + dev_err(&pdev->dev, "%s fail to create devices.\n", > + pdev->dev.of_node->full_name); > + clk_disable_unprepare(clk); > + return ret; > + } > + > + dev_info(&pdev->dev, "Driver registered.\n"); > + return 0; > +} The reset is defined in the binding, but never used. Should we make sure the reset is de-asserted in the probe? Jon
2016-07-21 12:15 GMT+02:00 Jon Hunter <jonathanh@nvidia.com>: > > On 19/07/16 14:36, Mirza Krak wrote: >> From: Mirza Krak <mirza.krak@gmail.com> >> +config TEGRA_NOR >> + tristate "Nvidia Tegra NOR flash bus driver a.k.a GMI/SNOR" >> + depends on ARCH_TEGRA_2x_SOC || ARCH_TEGRA_3x_SOC >> + depends on OF >> + help >> + Driver for NOR flash bus found on Tegra30/20 SOC`s. > > It is actually present on all Tegra's and so I would drop the 30/20. ACK. >> +++ b/drivers/bus/tegra-nor.c >> @@ -0,0 +1,118 @@ >> +/* >> + * Driver for Nvidia NOR Flash bus a.k.a SNOR/GMI. >> + * >> + * Copyright (C) 2016 Host Mobility AB. All rights reserved. >> + * >> + * This file is licensed under the terms of the GNU General Public >> + * License version 2. This program is licensed "as is" without any >> + * warranty of any kind, whether express or implied. >> + */ >> + >> +#include <linux/clk.h> >> +#include <linux/io.h> >> +#include <linux/mfd/syscon.h> > > Is this needed? It is not. ACK. > >> +#include <linux/module.h> >> +#include <linux/of_device.h> >> +#include <linux/regmap.h> > > Or this? It is not. ACK. > >> + >> +#define TEGRA_NOR_TIMING_REG_COUNT 2 >> + >> +#define TEGRA_NOR_CONFIG 0x00 >> +#define TEGRA_NOR_STATUS 0x04 >> +#define TEGRA_NOR_ADDR_PTR 0x08 >> +#define TEGRA_NOR_AHB_ADDR_PTR 0x0c >> +#define TEGRA_NOR_TIMING0 0x10 >> +#define TEGRA_NOR_TIMING1 0x14 >> +#define TEGRA_NOR_MIO_CONFIG 0x18 >> +#define TEGRA_NOR_MIO_TIMING 0x1c >> +#define TEGRA_NOR_DMA_CONFIG 0x20 >> +#define TEGRA_NOR_CS_MUX_CONFIG 0x24 > > Not all of these are used. It is good to define them and I wonder if we > should add support for MIO while are at it :-) We can come back to this once I have all SNOR stuff in order. > >> + >> +#define TEGRA_NOR_CONFIG_GO BIT(31) >> + >> +static const struct of_device_id nor_id_table[] = { >> + /* Tegra30 */ > > I don't think this comment is needed. ACK. > >> + { .compatible = "nvidia,tegra30-nor", .data = NULL, }, > > You don't need to set data to NULL. ACK. > >> + /* Tegra20 */ >> + { .compatible = "nvidia,tegra20-nor", .data = NULL, }, > > Same here. ACK >> +static int __init nor_probe(struct platform_device *pdev) >> +{ > > I would drop the __init. ACK. >> +static struct platform_driver nor_driver = { >> + .driver = { >> + .name = "tegra-nor", >> + .of_match_table = nor_id_table, >> + }, >> +}; > > The driver should have a remove function given that we can build as a > module. At the moment I do not know what we would do in a remove function and hence me not adding one. > >> +module_platform_driver_probe(nor_driver, nor_probe); > > I would use "tegra_nor" namespace for all the structs, functions, etc. > However, we may prefer to go with GMI and in which case tegra_gmi_probe, > etc. ACK. Who gets the last call on what we should call the driver? It seems that we both think GMI is a better name, do we need a third? :). Thank you Jonathan for your feedback. Best Regards, Mirza -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2016-07-21 17:12 GMT+02:00 Jon Hunter <jonathanh@nvidia.com>: > > The reset is defined in the binding, but never used. Should we make sure > the reset is de-asserted in the probe? Yes, will do that. -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 21/07/16 21:42, Mirza Krak wrote: > 2016-07-21 12:15 GMT+02:00 Jon Hunter <jonathanh@nvidia.com>: >> >> On 19/07/16 14:36, Mirza Krak wrote: >>> From: Mirza Krak <mirza.krak@gmail.com> >>> +config TEGRA_NOR >>> + tristate "Nvidia Tegra NOR flash bus driver a.k.a GMI/SNOR" >>> + depends on ARCH_TEGRA_2x_SOC || ARCH_TEGRA_3x_SOC >>> + depends on OF >>> + help >>> + Driver for NOR flash bus found on Tegra30/20 SOC`s. >> >> It is actually present on all Tegra's and so I would drop the 30/20. > > ACK. > >>> +++ b/drivers/bus/tegra-nor.c >>> @@ -0,0 +1,118 @@ >>> +/* >>> + * Driver for Nvidia NOR Flash bus a.k.a SNOR/GMI. >>> + * >>> + * Copyright (C) 2016 Host Mobility AB. All rights reserved. >>> + * >>> + * This file is licensed under the terms of the GNU General Public >>> + * License version 2. This program is licensed "as is" without any >>> + * warranty of any kind, whether express or implied. >>> + */ >>> + >>> +#include <linux/clk.h> >>> +#include <linux/io.h> >>> +#include <linux/mfd/syscon.h> >> >> Is this needed? > > It is not. ACK. > >> >>> +#include <linux/module.h> >>> +#include <linux/of_device.h> >>> +#include <linux/regmap.h> >> >> Or this? > > It is not. ACK. > >> >>> + >>> +#define TEGRA_NOR_TIMING_REG_COUNT 2 >>> + >>> +#define TEGRA_NOR_CONFIG 0x00 >>> +#define TEGRA_NOR_STATUS 0x04 >>> +#define TEGRA_NOR_ADDR_PTR 0x08 >>> +#define TEGRA_NOR_AHB_ADDR_PTR 0x0c >>> +#define TEGRA_NOR_TIMING0 0x10 >>> +#define TEGRA_NOR_TIMING1 0x14 >>> +#define TEGRA_NOR_MIO_CONFIG 0x18 >>> +#define TEGRA_NOR_MIO_TIMING 0x1c >>> +#define TEGRA_NOR_DMA_CONFIG 0x20 >>> +#define TEGRA_NOR_CS_MUX_CONFIG 0x24 >> >> Not all of these are used. It is good to define them and I wonder if we >> should add support for MIO while are at it :-) > > We can come back to this once I have all SNOR stuff in order. > >> >>> + >>> +#define TEGRA_NOR_CONFIG_GO BIT(31) >>> + >>> +static const struct of_device_id nor_id_table[] = { >>> + /* Tegra30 */ >> >> I don't think this comment is needed. > > ACK. > >> >>> + { .compatible = "nvidia,tegra30-nor", .data = NULL, }, >> >> You don't need to set data to NULL. > > ACK. > >> >>> + /* Tegra20 */ >>> + { .compatible = "nvidia,tegra20-nor", .data = NULL, }, >> >> Same here. > > ACK > >>> +static int __init nor_probe(struct platform_device *pdev) >>> +{ >> >> I would drop the __init. > > ACK. > >>> +static struct platform_driver nor_driver = { >>> + .driver = { >>> + .name = "tegra-nor", >>> + .of_match_table = nor_id_table, >>> + }, >>> +}; >> >> The driver should have a remove function given that we can build as a >> module. > > At the moment I do not know what we would do in a remove function and > hence me not adding one. Should just be the inverse of the probe (although there is no inverse for the parsing DT bit). If you don't wish to add a remove, that is fine, but make the driver a 'bool' and not 'tristate' in the Kconfig so it cannot be configured as a module. >> >>> +module_platform_driver_probe(nor_driver, nor_probe); >> >> I would use "tegra_nor" namespace for all the structs, functions, etc. >> However, we may prefer to go with GMI and in which case tegra_gmi_probe, >> etc. > > ACK. Who gets the last call on what we should call the driver? It > seems that we both think GMI is a better name, do we need a third? :). The patches would have to go via Thierry and so ultimately, Thierry. However, I can't imagine he would object to GMI ;-) Jon
2016-07-22 11:38 GMT+02:00 Jon Hunter <jonathanh@nvidia.com>: >>> The driver should have a remove function given that we can build as a >>> module. >> >> At the moment I do not know what we would do in a remove function and >> hence me not adding one. > > Should just be the inverse of the probe (although there is no inverse > for the parsing DT bit). If you don't wish to add a remove, that is > fine, but make the driver a 'bool' and not 'tristate' in the Kconfig so > it cannot be configured as a module. I understand the concept of a remove function, but I use devm_ calls for all resources. These should be handled by the device core on a driver detach? One thing came to mind now that could be done in a remove method and that is clearing the CONFIG_GO bit, or I could just do that first on probe instead to make sure the controller is stopped. Ok, one more thing came to mind, and that is depopulating the child devices. Got it remove function it is then. >>>> +module_platform_driver_probe(nor_driver, nor_probe); >>> >>> I would use "tegra_nor" namespace for all the structs, functions, etc. >>> However, we may prefer to go with GMI and in which case tegra_gmi_probe, >>> etc. >> >> ACK. Who gets the last call on what we should call the driver? It >> seems that we both think GMI is a better name, do we need a third? :). > > The patches would have to go via Thierry and so ultimately, Thierry. > However, I can't imagine he would object to GMI ;-) Eagerly awaiting Thierry`s comments :). Best Regards, Mirza -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 22/07/16 20:18, Mirza Krak wrote: > 2016-07-22 11:38 GMT+02:00 Jon Hunter <jonathanh@nvidia.com>: >>>> The driver should have a remove function given that we can build as a >>>> module. >>> >>> At the moment I do not know what we would do in a remove function and >>> hence me not adding one. >> >> Should just be the inverse of the probe (although there is no inverse >> for the parsing DT bit). If you don't wish to add a remove, that is >> fine, but make the driver a 'bool' and not 'tristate' in the Kconfig so >> it cannot be configured as a module. > > I understand the concept of a remove function, but I use devm_ calls > for all resources. These should be handled by the device core on a > driver detach? There is one clock that needs to be disabled in the remove. Cheers Jon
On Fri, Jul 22, 2016 at 09:18:37PM +0200, Mirza Krak wrote: > 2016-07-22 11:38 GMT+02:00 Jon Hunter <jonathanh@nvidia.com>: > >>> The driver should have a remove function given that we can build as a > >>> module. > >> > >> At the moment I do not know what we would do in a remove function and > >> hence me not adding one. > > > > Should just be the inverse of the probe (although there is no inverse > > for the parsing DT bit). If you don't wish to add a remove, that is > > fine, but make the driver a 'bool' and not 'tristate' in the Kconfig so > > it cannot be configured as a module. > > I understand the concept of a remove function, but I use devm_ calls > for all resources. These should be handled by the device core on a > driver detach? > > One thing came to mind now that could be done in a remove method and > that is clearing the CONFIG_GO bit, or I could just do that first on > probe instead to make sure the controller is stopped. > > Ok, one more thing came to mind, and that is depopulating the child > devices. Got it remove function it is then. > > >>>> +module_platform_driver_probe(nor_driver, nor_probe); > >>> > >>> I would use "tegra_nor" namespace for all the structs, functions, etc. > >>> However, we may prefer to go with GMI and in which case tegra_gmi_probe, > >>> etc. > >> > >> ACK. Who gets the last call on what we should call the driver? It > >> seems that we both think GMI is a better name, do we need a third? :). > > > > The patches would have to go via Thierry and so ultimately, Thierry. > > However, I can't imagine he would object to GMI ;-) > > Eagerly awaiting Thierry`s comments :). Let's go with GMI. The TRM has a mix of GMI vs. SNOR, but as Jon points out the pinmux doesn't mention SNOR, so in order to hopefully reduce the confusion, let's stick with GMI. Thierry
On Tue, Jul 19, 2016 at 03:36:37PM +0200, Mirza Krak wrote: > From: Mirza Krak <mirza.krak@gmail.com> > > The NOR bus can be used to connect high-speed devices such as NOR flash, > FPGAs, DSPs, CAN chips, Wi-Fi chips etc. > > Signed-off-by: Mirza Krak <mirza.krak@gmail.com> > --- > drivers/bus/Kconfig | 7 +++ > drivers/bus/Makefile | 1 + > drivers/bus/tegra-nor.c | 118 ++++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 126 insertions(+) > create mode 100644 drivers/bus/tegra-nor.c > > diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig > index 3b205e2..b74be7d8 100644 > --- a/drivers/bus/Kconfig > +++ b/drivers/bus/Kconfig > @@ -145,6 +145,13 @@ config TEGRA_ACONNECT > Driver for the Tegra ACONNECT bus which is used to interface with > the devices inside the Audio Processing Engine (APE) for Tegra210. > > +config TEGRA_NOR > + tristate "Nvidia Tegra NOR flash bus driver a.k.a GMI/SNOR" > + depends on ARCH_TEGRA_2x_SOC || ARCH_TEGRA_3x_SOC I think an ARCH_TEGRA dependency is enough here. > + depends on OF You can drop this because Tegra has been OF-only for a long time now. > + help > + Driver for NOR flash bus found on Tegra30/20 SOC`s. Maybe make this "Driver for GMI controller found on NVIDIA Tegra SoCs." or similar in light of the name change. > config UNIPHIER_SYSTEM_BUS > tristate "UniPhier System Bus driver" > depends on ARCH_UNIPHIER && OF > diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile > index ac84cc4..46d0129 100644 > --- a/drivers/bus/Makefile > +++ b/drivers/bus/Makefile > @@ -18,5 +18,6 @@ obj-$(CONFIG_OMAP_OCP2SCP) += omap-ocp2scp.o > obj-$(CONFIG_SUNXI_RSB) += sunxi-rsb.o > obj-$(CONFIG_SIMPLE_PM_BUS) += simple-pm-bus.o > obj-$(CONFIG_TEGRA_ACONNECT) += tegra-aconnect.o > +obj-$(CONFIG_TEGRA_NOR) += tegra-nor.o > obj-$(CONFIG_UNIPHIER_SYSTEM_BUS) += uniphier-system-bus.o > obj-$(CONFIG_VEXPRESS_CONFIG) += vexpress-config.o > diff --git a/drivers/bus/tegra-nor.c b/drivers/bus/tegra-nor.c > new file mode 100644 > index 0000000..1e48113 > --- /dev/null > +++ b/drivers/bus/tegra-nor.c > @@ -0,0 +1,118 @@ > +/* > + * Driver for Nvidia NOR Flash bus a.k.a SNOR/GMI. Nit: I encourage the use of "NVIDIA" as spelling for consistency. > + * > + * Copyright (C) 2016 Host Mobility AB. All rights reserved. > + * > + * This file is licensed under the terms of the GNU General Public > + * License version 2. This program is licensed "as is" without any > + * warranty of any kind, whether express or implied. > + */ > + > +#include <linux/clk.h> > +#include <linux/io.h> > +#include <linux/mfd/syscon.h> > +#include <linux/module.h> > +#include <linux/of_device.h> > +#include <linux/regmap.h> > + > +#define TEGRA_NOR_TIMING_REG_COUNT 2 > + > +#define TEGRA_NOR_CONFIG 0x00 > +#define TEGRA_NOR_STATUS 0x04 > +#define TEGRA_NOR_ADDR_PTR 0x08 > +#define TEGRA_NOR_AHB_ADDR_PTR 0x0c > +#define TEGRA_NOR_TIMING0 0x10 > +#define TEGRA_NOR_TIMING1 0x14 > +#define TEGRA_NOR_MIO_CONFIG 0x18 > +#define TEGRA_NOR_MIO_TIMING 0x1c > +#define TEGRA_NOR_DMA_CONFIG 0x20 > +#define TEGRA_NOR_CS_MUX_CONFIG 0x24 > + > +#define TEGRA_NOR_CONFIG_GO BIT(31) > + > +static const struct of_device_id nor_id_table[] = { > + /* Tegra30 */ > + { .compatible = "nvidia,tegra30-nor", .data = NULL, }, > + /* Tegra20 */ > + { .compatible = "nvidia,tegra20-nor", .data = NULL, }, > + > + { } > +}; > +MODULE_DEVICE_TABLE(of, nor_id_table); > + > + > +static int __init nor_parse_dt(struct platform_device *pdev, > + void __iomem *base) > +{ > + struct device_node *of_node = pdev->dev.of_node; > + u32 config, timing[TEGRA_NOR_TIMING_REG_COUNT]; > + int ret; > + > + ret = of_property_read_u32_array(of_node, "nvidia,cs-timing", > + timing, TEGRA_NOR_TIMING_REG_COUNT); > + if (!ret) { > + writel(timing[0], base + TEGRA_NOR_TIMING0); > + writel(timing[1], base + TEGRA_NOR_TIMING1); > + } > + > + ret = of_property_read_u32(of_node, "nvidia,config", &config); > + if (ret) > + return ret; > + > + config |= TEGRA_NOR_CONFIG_GO; > + writel(config, base + TEGRA_NOR_CONFIG); My preference would be for the tegra_gmi_parse_dt() function not to do any register programming. Instead I think it would be better to store any of the parameters inside a struct tegra_gmi and do the programming from within tegra_gmi_probe() (or via an other function such as tegra_gmi_initialize(), called from tegra_gmi_probe()). The reason is that you'll most likely want to do this programming when you resume from suspend, and you could reuse tegra_gmi_initialize() to do that. > + > + if (of_get_child_count(of_node)) > + ret = of_platform_populate(of_node, > + of_default_bus_match_table, > + NULL, &pdev->dev); Why the extra check? of_platform_populate() is almost a no-op if there aren't any children anyway. What it will do is set the OF_POPULATED_BUS flag, but I think we want that irrespective of whether or not there are any children. Was there any problem with calling it unconditionally that made you opt for the extra check? Also, I think you can call of_platform_default_populate(), which is a little shorter than the above because you can omit the match table. > + > + return ret; > +} > + > +static int __init nor_probe(struct platform_device *pdev) > +{ > + struct resource *res; > + struct clk *clk; > + void __iomem *base; > + int ret; > + > + /* get the resource */ > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + base = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(base)) > + return PTR_ERR(base); > + > + /* get the clock */ > + clk = devm_clk_get(&pdev->dev, NULL); Let's use a consumer name here. The problem with a NULL consumer name is that it effectively restricts the DT binding. It means that whatever the clock is its name needs to be the first in a clock-names property. We'll likely get away with this because there's only one clock, but should we ever need to add another we'd need to add wording to the device tree bindings that the "gmi" entry needs to be first. I'm not sure if that's clear, so I'll try to explain in other words: If you pass a NULL consumer name to clk_get() it will simply use the first clock in the clocks property. If you want to later extend the DT binding by adding a clock in a backwards-compatible way, you'll need to add the restriction that the "gmi" clock (the one that was previously unnamed) must be the first in the clock-names and clocks properties. That's all a little confusing, so let's avoid this by giving it a proper consumer name to begin with. > + if (IS_ERR(clk)) > + return PTR_ERR(clk); > + > + ret = clk_prepare_enable(clk); > + if (ret) > + return ret; > + > + /* parse the device node */ > + ret = nor_parse_dt(pdev, base); > + if (ret) { > + dev_err(&pdev->dev, "%s fail to create devices.\n", > + pdev->dev.of_node->full_name); > + clk_disable_unprepare(clk); > + return ret; > + } The good thing if you don't do any programming in tegra_gmi_parse_dt(), is that you can easily move the clk_prepare_enable() call to here where things can't fail anymore, so you don't have to add cleanup code. > + > + dev_info(&pdev->dev, "Driver registered.\n"); Please avoid this kind of output. Users expect that everything will work so you want to let them know when something goes wrong, and be quiet when all goes as expected. > + return 0; > +} > + > +static struct platform_driver nor_driver = { > + .driver = { > + .name = "tegra-nor", > + .of_match_table = nor_id_table, > + }, > +}; > +module_platform_driver_probe(nor_driver, nor_probe); > + > +MODULE_AUTHOR("Mirza Krak <mirza.krak@gmail.com"); > +MODULE_DESCRIPTION("NVIDIA Tegra NOR Bus Driver"); > +MODULE_LICENSE("GPL"); You're header comment says GPL version 2, which means that the MODULE_LICENSE needs to be "GPL v2". "GPL" means "version 2 or later". Thierry
2016-07-25 13:14 GMT+02:00 Thierry Reding <thierry.reding@gmail.com>: > On Tue, Jul 19, 2016 at 03:36:37PM +0200, Mirza Krak wrote: >> +config TEGRA_NOR >> + tristate "Nvidia Tegra NOR flash bus driver a.k.a GMI/SNOR" >> + depends on ARCH_TEGRA_2x_SOC || ARCH_TEGRA_3x_SOC > > I think an ARCH_TEGRA dependency is enough here. ACK. > >> + depends on OF > > You can drop this because Tegra has been OF-only for a long time now. ACK. > >> + help >> + Driver for NOR flash bus found on Tegra30/20 SOC`s. > > Maybe make this "Driver for GMI controller found on NVIDIA Tegra SoCs." > or similar in light of the name change. ACK. >> --- /dev/null >> +++ b/drivers/bus/tegra-nor.c >> @@ -0,0 +1,118 @@ >> +/* >> + * Driver for Nvidia NOR Flash bus a.k.a SNOR/GMI. > > Nit: I encourage the use of "NVIDIA" as spelling for consistency. ACK. >> +static int __init nor_parse_dt(struct platform_device *pdev, >> + void __iomem *base) >> +{ >> + struct device_node *of_node = pdev->dev.of_node; >> + u32 config, timing[TEGRA_NOR_TIMING_REG_COUNT]; >> + int ret; >> + >> + ret = of_property_read_u32_array(of_node, "nvidia,cs-timing", >> + timing, TEGRA_NOR_TIMING_REG_COUNT); >> + if (!ret) { >> + writel(timing[0], base + TEGRA_NOR_TIMING0); >> + writel(timing[1], base + TEGRA_NOR_TIMING1); >> + } >> + >> + ret = of_property_read_u32(of_node, "nvidia,config", &config); >> + if (ret) >> + return ret; >> + >> + config |= TEGRA_NOR_CONFIG_GO; >> + writel(config, base + TEGRA_NOR_CONFIG); > > My preference would be for the tegra_gmi_parse_dt() function not to do > any register programming. Instead I think it would be better to store > any of the parameters inside a struct tegra_gmi and do the programming > from within tegra_gmi_probe() (or via an other function such as > tegra_gmi_initialize(), called from tegra_gmi_probe()). > > The reason is that you'll most likely want to do this programming when > you resume from suspend, and you could reuse tegra_gmi_initialize() to > do that. ACK. > >> + >> + if (of_get_child_count(of_node)) >> + ret = of_platform_populate(of_node, >> + of_default_bus_match_table, >> + NULL, &pdev->dev); > > Why the extra check? of_platform_populate() is almost a no-op if there > aren't any children anyway. What it will do is set the OF_POPULATED_BUS > flag, but I think we want that irrespective of whether or not there are > any children. > > Was there any problem with calling it unconditionally that made you opt > for the extra check? I have not tested calling it unconditionally. Used another driver as reference and they had that condition (drivers/bus/imx-weim.c), that driver does not mention why the condition is there. But will test removing the condition and see what happens. > > Also, I think you can call of_platform_default_populate(), which is a > little shorter than the above because you can omit the match table. Will look in to this. > >> + >> + return ret; >> +} >> + >> +static int __init nor_probe(struct platform_device *pdev) >> +{ >> + struct resource *res; >> + struct clk *clk; >> + void __iomem *base; >> + int ret; >> + >> + /* get the resource */ >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + base = devm_ioremap_resource(&pdev->dev, res); >> + if (IS_ERR(base)) >> + return PTR_ERR(base); >> + >> + /* get the clock */ >> + clk = devm_clk_get(&pdev->dev, NULL); > > Let's use a consumer name here. The problem with a NULL consumer name is > that it effectively restricts the DT binding. It means that whatever the > clock is its name needs to be the first in a clock-names property. We'll > likely get away with this because there's only one clock, but should we > ever need to add another we'd need to add wording to the device tree > bindings that the "gmi" entry needs to be first. > > I'm not sure if that's clear, so I'll try to explain in other words: If > you pass a NULL consumer name to clk_get() it will simply use the first > clock in the clocks property. If you want to later extend the DT binding > by adding a clock in a backwards-compatible way, you'll need to add the > restriction that the "gmi" clock (the one that was previously unnamed) > must be the first in the clock-names and clocks properties. > > That's all a little confusing, so let's avoid this by giving it a proper > consumer name to begin with. Got it. Will add a proper consumer name. > >> + if (IS_ERR(clk)) >> + return PTR_ERR(clk); >> + >> + ret = clk_prepare_enable(clk); >> + if (ret) >> + return ret; >> + >> + /* parse the device node */ >> + ret = nor_parse_dt(pdev, base); >> + if (ret) { >> + dev_err(&pdev->dev, "%s fail to create devices.\n", >> + pdev->dev.of_node->full_name); >> + clk_disable_unprepare(clk); >> + return ret; >> + } > > The good thing if you don't do any programming in tegra_gmi_parse_dt(), > is that you can easily move the clk_prepare_enable() call to here where > things can't fail anymore, so you don't have to add cleanup code. ACK. > >> + >> + dev_info(&pdev->dev, "Driver registered.\n"); > > Please avoid this kind of output. Users expect that everything will work > so you want to let them know when something goes wrong, and be quiet > when all goes as expected. Will remove that. >> +static struct platform_driver nor_driver = { >> + .driver = { >> + .name = "tegra-nor", >> + .of_match_table = nor_id_table, >> + }, >> +}; >> +module_platform_driver_probe(nor_driver, nor_probe); >> + >> +MODULE_AUTHOR("Mirza Krak <mirza.krak@gmail.com"); >> +MODULE_DESCRIPTION("NVIDIA Tegra NOR Bus Driver"); >> +MODULE_LICENSE("GPL"); > > You're header comment says GPL version 2, which means that the > MODULE_LICENSE needs to be "GPL v2". "GPL" means "version 2 or later". Ok, I do not really mind it being GPL version 2 or later so will change the header comment instead if that is ok. Best regards, Mirza -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 25, 2016 at 02:17:11PM +0200, Mirza Krak wrote: > 2016-07-25 13:14 GMT+02:00 Thierry Reding <thierry.reding@gmail.com>: > > On Tue, Jul 19, 2016 at 03:36:37PM +0200, Mirza Krak wrote: [...] > >> + > >> + if (of_get_child_count(of_node)) > >> + ret = of_platform_populate(of_node, > >> + of_default_bus_match_table, > >> + NULL, &pdev->dev); > > > > Why the extra check? of_platform_populate() is almost a no-op if there > > aren't any children anyway. What it will do is set the OF_POPULATED_BUS > > flag, but I think we want that irrespective of whether or not there are > > any children. > > > > Was there any problem with calling it unconditionally that made you opt > > for the extra check? > > I have not tested calling it unconditionally. Used another driver as > reference and they had that condition (drivers/bus/imx-weim.c), that > driver does not mention why the condition is there. But will test > removing the condition and see what happens. If that works fine for Tegra, it might be a good idea to get rid of the call in the imx-weim.c driver, too. > >> +static struct platform_driver nor_driver = { > >> + .driver = { > >> + .name = "tegra-nor", > >> + .of_match_table = nor_id_table, > >> + }, > >> +}; > >> +module_platform_driver_probe(nor_driver, nor_probe); > >> + > >> +MODULE_AUTHOR("Mirza Krak <mirza.krak@gmail.com"); > >> +MODULE_DESCRIPTION("NVIDIA Tegra NOR Bus Driver"); > >> +MODULE_LICENSE("GPL"); > > > > You're header comment says GPL version 2, which means that the > > MODULE_LICENSE needs to be "GPL v2". "GPL" means "version 2 or later". > > Ok, I do not really mind it being GPL version 2 or later so will > change the header comment instead if that is ok. I think "GPL v2" is traditionally more common in kernel code, but as the author the decision is obviously yours. Thierry
diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig index 3b205e2..b74be7d8 100644 --- a/drivers/bus/Kconfig +++ b/drivers/bus/Kconfig @@ -145,6 +145,13 @@ config TEGRA_ACONNECT Driver for the Tegra ACONNECT bus which is used to interface with the devices inside the Audio Processing Engine (APE) for Tegra210. +config TEGRA_NOR + tristate "Nvidia Tegra NOR flash bus driver a.k.a GMI/SNOR" + depends on ARCH_TEGRA_2x_SOC || ARCH_TEGRA_3x_SOC + depends on OF + help + Driver for NOR flash bus found on Tegra30/20 SOC`s. + config UNIPHIER_SYSTEM_BUS tristate "UniPhier System Bus driver" depends on ARCH_UNIPHIER && OF diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile index ac84cc4..46d0129 100644 --- a/drivers/bus/Makefile +++ b/drivers/bus/Makefile @@ -18,5 +18,6 @@ obj-$(CONFIG_OMAP_OCP2SCP) += omap-ocp2scp.o obj-$(CONFIG_SUNXI_RSB) += sunxi-rsb.o obj-$(CONFIG_SIMPLE_PM_BUS) += simple-pm-bus.o obj-$(CONFIG_TEGRA_ACONNECT) += tegra-aconnect.o +obj-$(CONFIG_TEGRA_NOR) += tegra-nor.o obj-$(CONFIG_UNIPHIER_SYSTEM_BUS) += uniphier-system-bus.o obj-$(CONFIG_VEXPRESS_CONFIG) += vexpress-config.o diff --git a/drivers/bus/tegra-nor.c b/drivers/bus/tegra-nor.c new file mode 100644 index 0000000..1e48113 --- /dev/null +++ b/drivers/bus/tegra-nor.c @@ -0,0 +1,118 @@ +/* + * Driver for Nvidia NOR Flash bus a.k.a SNOR/GMI. + * + * Copyright (C) 2016 Host Mobility AB. All rights reserved. + * + * This file is licensed under the terms of the GNU General Public + * License version 2. This program is licensed "as is" without any + * warranty of any kind, whether express or implied. + */ + +#include <linux/clk.h> +#include <linux/io.h> +#include <linux/mfd/syscon.h> +#include <linux/module.h> +#include <linux/of_device.h> +#include <linux/regmap.h> + +#define TEGRA_NOR_TIMING_REG_COUNT 2 + +#define TEGRA_NOR_CONFIG 0x00 +#define TEGRA_NOR_STATUS 0x04 +#define TEGRA_NOR_ADDR_PTR 0x08 +#define TEGRA_NOR_AHB_ADDR_PTR 0x0c +#define TEGRA_NOR_TIMING0 0x10 +#define TEGRA_NOR_TIMING1 0x14 +#define TEGRA_NOR_MIO_CONFIG 0x18 +#define TEGRA_NOR_MIO_TIMING 0x1c +#define TEGRA_NOR_DMA_CONFIG 0x20 +#define TEGRA_NOR_CS_MUX_CONFIG 0x24 + +#define TEGRA_NOR_CONFIG_GO BIT(31) + +static const struct of_device_id nor_id_table[] = { + /* Tegra30 */ + { .compatible = "nvidia,tegra30-nor", .data = NULL, }, + /* Tegra20 */ + { .compatible = "nvidia,tegra20-nor", .data = NULL, }, + + { } +}; +MODULE_DEVICE_TABLE(of, nor_id_table); + + +static int __init nor_parse_dt(struct platform_device *pdev, + void __iomem *base) +{ + struct device_node *of_node = pdev->dev.of_node; + u32 config, timing[TEGRA_NOR_TIMING_REG_COUNT]; + int ret; + + ret = of_property_read_u32_array(of_node, "nvidia,cs-timing", + timing, TEGRA_NOR_TIMING_REG_COUNT); + if (!ret) { + writel(timing[0], base + TEGRA_NOR_TIMING0); + writel(timing[1], base + TEGRA_NOR_TIMING1); + } + + ret = of_property_read_u32(of_node, "nvidia,config", &config); + if (ret) + return ret; + + config |= TEGRA_NOR_CONFIG_GO; + writel(config, base + TEGRA_NOR_CONFIG); + + if (of_get_child_count(of_node)) + ret = of_platform_populate(of_node, + of_default_bus_match_table, + NULL, &pdev->dev); + + return ret; +} + +static int __init nor_probe(struct platform_device *pdev) +{ + struct resource *res; + struct clk *clk; + void __iomem *base; + int ret; + + /* get the resource */ + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + base = devm_ioremap_resource(&pdev->dev, res); + if (IS_ERR(base)) + return PTR_ERR(base); + + /* get the clock */ + clk = devm_clk_get(&pdev->dev, NULL); + if (IS_ERR(clk)) + return PTR_ERR(clk); + + ret = clk_prepare_enable(clk); + if (ret) + return ret; + + /* parse the device node */ + ret = nor_parse_dt(pdev, base); + if (ret) { + dev_err(&pdev->dev, "%s fail to create devices.\n", + pdev->dev.of_node->full_name); + clk_disable_unprepare(clk); + return ret; + } + + dev_info(&pdev->dev, "Driver registered.\n"); + return 0; +} + +static struct platform_driver nor_driver = { + .driver = { + .name = "tegra-nor", + .of_match_table = nor_id_table, + }, +}; +module_platform_driver_probe(nor_driver, nor_probe); + +MODULE_AUTHOR("Mirza Krak <mirza.krak@gmail.com"); +MODULE_DESCRIPTION("NVIDIA Tegra NOR Bus Driver"); +MODULE_LICENSE("GPL");