Re: [PATCH 1/6] mfd: syscon: Add syscon_register() function

2018-01-22 Thread David Lechner 

On 01/22/2018 05:39 AM, Sekhar Nori wrote:

On Monday 22 January 2018 04:38 PM, Arnd Bergmann wrote:

On Mon, Jan 22, 2018 at 11:53 AM, Sekhar Nori  wrote:

On Saturday 20 January 2018 08:50 AM, David Lechner wrote:

This adds a new syscon_register() function that creates a new syscon
regmap and adds it to the lookup list.


/*
  * registers a "global" syscon "device", usually not backed by a real
  * device. To be used only in cases where the syscon is not
  * related to any actual device, like system clocks, for example.
  *
  * name passed here must be globally unique.
  */
struct regmap *syscon_register_by_name(resource_size_t start, size_t
size, const char *name);

and

struct regmap *syscon_regmap_lookup_by_name(const char *name);

I understand, with this the driver becomes little more complicated and
DT and non-DT cases still need to be handled differently. But I think
thats not the main issue you are trying to solve.


I think the easiest way to handle this in traditional board files is to use
a platform_data structure: when the platform registers the syscon,
it holds a pointer to the regmap and can simply add pass it to any
device using the syscon through platform_data. The driver using it
then does:

if (dev->of_node) {
   priv->regmap = syscon_regmap_lookup_by_phandle(...);
else
   priv->regmap = pdata->regmap;


I think this works as well and does not need any new API. For clocks,
there is no platform data AFAICT, so it will have to be whatever
structure is being used to pass clock info to CCF driver (like perhaps
an addition parameter to da850_pll_clk_init() in David's under-review
DaVinci CCF conversion series).


At some point, we had a syscon_regmap_lookup_by_pdevname()
that was introduced for a similar case, but we should just kill that
now, after the platform that needed it is DT-only.


Yeah, I just noticed that too. syscon_regmap_lookup_by_pdevname has only
two callers in kernel. The call from clps711x.c UART driver does not
seem to be needed anymore after clps711x was converted to DT-only in v4.8

The other user is phy-da8xx-usb.c which this series is trying to get rid
of. So, perhaps syscon_regmap_lookup_by_pdevname() should be dropped by
v2 of this series.



Sounds like a plan.

Sekhar, would you please have a look at [PATCH 4/6] "ARM: davinci: move
davinci_clk_init() to init_time" I've sent this 5 times already as part
of the common clock series. If it looks good to you, it would be nice if
you could pick it up so I don't have to keep resending it. :-)

Re: [PATCH 1/6] mfd: syscon: Add syscon_register() function

2018-01-22 Thread Sekhar Nori 
On Monday 22 January 2018 04:38 PM, Arnd Bergmann wrote:
> On Mon, Jan 22, 2018 at 11:53 AM, Sekhar Nori  wrote:
>> On Saturday 20 January 2018 08:50 AM, David Lechner wrote:
>>> This adds a new syscon_register() function that creates a new syscon
>>> regmap and adds it to the lookup list.
>>
>> /*
>>  * registers a "global" syscon "device", usually not backed by a real
>>  * device. To be used only in cases where the syscon is not
>>  * related to any actual device, like system clocks, for example.
>>  *
>>  * name passed here must be globally unique.
>>  */
>> struct regmap *syscon_register_by_name(resource_size_t start, size_t
>>size, const char *name);
>>
>> and
>>
>> struct regmap *syscon_regmap_lookup_by_name(const char *name);
>>
>> I understand, with this the driver becomes little more complicated and
>> DT and non-DT cases still need to be handled differently. But I think
>> thats not the main issue you are trying to solve.
> 
> I think the easiest way to handle this in traditional board files is to use
> a platform_data structure: when the platform registers the syscon,
> it holds a pointer to the regmap and can simply add pass it to any
> device using the syscon through platform_data. The driver using it
> then does:
> 
> if (dev->of_node) {
>   priv->regmap = syscon_regmap_lookup_by_phandle(...);
> else
>   priv->regmap = pdata->regmap;

I think this works as well and does not need any new API. For clocks,
there is no platform data AFAICT, so it will have to be whatever
structure is being used to pass clock info to CCF driver (like perhaps
an addition parameter to da850_pll_clk_init() in David's under-review
DaVinci CCF conversion series).

> At some point, we had a syscon_regmap_lookup_by_pdevname()
> that was introduced for a similar case, but we should just kill that
> now, after the platform that needed it is DT-only.

Yeah, I just noticed that too. syscon_regmap_lookup_by_pdevname has only
two callers in kernel. The call from clps711x.c UART driver does not
seem to be needed anymore after clps711x was converted to DT-only in v4.8

The other user is phy-da8xx-usb.c which this series is trying to get rid
of. So, perhaps syscon_regmap_lookup_by_pdevname() should be dropped by
v2 of this series.

Thanks,
Sekhar

Re: [PATCH 1/6] mfd: syscon: Add syscon_register() function

2018-01-22 Thread Arnd Bergmann 
On Mon, Jan 22, 2018 at 11:53 AM, Sekhar Nori  wrote:
> On Saturday 20 January 2018 08:50 AM, David Lechner wrote:
>> This adds a new syscon_register() function that creates a new syscon
>> regmap and adds it to the lookup list.
>
> /*
>  * registers a "global" syscon "device", usually not backed by a real
>  * device. To be used only in cases where the syscon is not
>  * related to any actual device, like system clocks, for example.
>  *
>  * name passed here must be globally unique.
>  */
> struct regmap *syscon_register_by_name(resource_size_t start, size_t
>size, const char *name);
>
> and
>
> struct regmap *syscon_regmap_lookup_by_name(const char *name);
>
> I understand, with this the driver becomes little more complicated and
> DT and non-DT cases still need to be handled differently. But I think
> thats not the main issue you are trying to solve.

I think the easiest way to handle this in traditional board files is to use
a platform_data structure: when the platform registers the syscon,
it holds a pointer to the regmap and can simply add pass it to any
device using the syscon through platform_data. The driver using it
then does:

if (dev->of_node) {
  priv->regmap = syscon_regmap_lookup_by_phandle(...);
else
  priv->regmap = pdata->regmap;

At some point, we had a syscon_regmap_lookup_by_pdevname()
that was introduced for a similar case, but we should just kill that
now, after the platform that needed it is DT-only.

   Arnd

Re: [PATCH 1/6] mfd: syscon: Add syscon_register() function

2018-01-22 Thread Sekhar Nori 
On Saturday 20 January 2018 08:50 AM, David Lechner wrote:
> This adds a new syscon_register() function that creates a new syscon
> regmap and adds it to the lookup list.
> 
> This function serves two purposes:
> 
> 1. This is needed for platforms without device tree support where the
>syscon regmap is needed in early boot (e.g. clocks), because using a
>platform driver at this point in boot is not an option.

Is this because platform_device_register() of the syscon device fails
when called early on? It will be nice to document the exact failure case
here (at least the return value) so its clear what the issue is.

> 2. It allows other drivers to use syscon_regmap_lookup_by_compatible()
>for both device tree and non-DT platforms instead of having to have
>a separate case that calls syscon_regmap_lookup_by_pdevname().

"compatible" is a very device-tree specific terminology so I am afraid
this is will actually turn out to be confusing.

So, if we are doing this, I would rather be more explicit about it:

/*
 * registers a "global" syscon "device", usually not backed by a real
 * device. To be used only in cases where the syscon is not
 * related to any actual device, like system clocks, for example.
 *
 * name passed here must be globally unique.
 */
struct regmap *syscon_register_by_name(resource_size_t start, size_t
   size, const char *name);

and

struct regmap *syscon_regmap_lookup_by_name(const char *name);

I understand, with this the driver becomes little more complicated and
DT and non-DT cases still need to be handled differently. But I think
thats not the main issue you are trying to solve.

> 
> Signed-off-by: David Lechner 

Thanks,
Sekhar

[PATCH 1/6] mfd: syscon: Add syscon_register() function

2018-01-19 Thread David Lechner 
This adds a new syscon_register() function that creates a new syscon
regmap and adds it to the lookup list.

This function serves two purposes:

1. This is needed for platforms without device tree support where the
   syscon regmap is needed in early boot (e.g. clocks), because using a
   platform driver at this point in boot is not an option.

2. It allows other drivers to use syscon_regmap_lookup_by_compatible()
   for both device tree and non-DT platforms instead of having to have
   a separate case that calls syscon_regmap_lookup_by_pdevname().

Signed-off-by: David Lechner 
---
 drivers/mfd/syscon.c   | 71 ++
 include/linux/mfd/syscon.h |  9 ++
 2 files changed, 80 insertions(+)

diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
index b93fe4c..ab086b1 100644
--- a/drivers/mfd/syscon.c
+++ b/drivers/mfd/syscon.c
@@ -25,6 +25,8 @@
 #include 
 #include 
 
+#define SYSCON_COMPATIBLE_SIZE 50
+
 static struct platform_driver syscon_driver;
 
 static DEFINE_SPINLOCK(syscon_list_slock);
@@ -34,6 +36,7 @@ struct syscon {
struct device_node *np;
struct regmap *regmap;
struct list_head list;
+   char compatible[SYSCON_COMPATIBLE_SIZE];
 };
 
 static const struct regmap_config syscon_regmap_config = {
@@ -140,9 +143,27 @@ EXPORT_SYMBOL_GPL(syscon_node_to_regmap);
 
 struct regmap *syscon_regmap_lookup_by_compatible(const char *s)
 {
+   struct syscon *entry, *syscon = NULL;
struct device_node *syscon_np;
struct regmap *regmap;
 
+   spin_lock(&syscon_list_slock);
+
+   /* Check for entries registered with syscon_register() */
+   list_for_each_entry(entry, &syscon_list, list) {
+   if (!entry->compatible)
+   continue;
+   if (!strncmp(entry->compatible, s, SYSCON_COMPATIBLE_SIZE)) {
+   syscon = entry;
+   break;
+   }
+   }
+
+   spin_unlock(&syscon_list_slock);
+
+   if (syscon)
+   return syscon->regmap;
+
syscon_np = of_find_compatible_node(NULL, NULL, s);
if (!syscon_np)
return ERR_PTR(-ENODEV);
@@ -196,6 +217,56 @@ struct regmap *syscon_regmap_lookup_by_phandle(struct 
device_node *np,
 }
 EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_phandle);
 
+/**
+ * syscon_register - Register a new syscon regmap
+ * @start: The starting memory address of the regmap
+ * @size: The size of the regmap in bytes
+ * @compatible: Compatible string used for lookup
+ *
+ * Returns: Pointer to a regmap or a negative error code.
+ */
+struct regmap *syscon_register(resource_size_t start, size_t size,
+  const char *compatible)
+{
+   struct regmap_config syscon_config = syscon_regmap_config;
+   struct syscon *syscon;
+   void __iomem *base;
+   int err;
+
+   syscon = kzalloc(sizeof(*syscon), GFP_KERNEL);
+   if (!syscon)
+   return ERR_PTR(-ENOMEM);
+
+   base = ioremap(start, size);
+   if (!base) {
+   err = -ENOMEM;
+   goto err_free_syscon;
+   }
+
+   strncpy(syscon->compatible, compatible, SYSCON_COMPATIBLE_SIZE);
+
+   syscon_config.max_register = size - 1;
+   syscon->regmap = regmap_init_mmio(NULL, base, &syscon_config);
+   if (IS_ERR(syscon->regmap)) {
+   err = PTR_ERR(syscon->regmap);
+   goto err_iounmap;
+   }
+
+   spin_lock(&syscon_list_slock);
+   list_add_tail(&syscon->list, &syscon_list);
+   spin_unlock(&syscon_list_slock);
+
+   return syscon->regmap;
+
+err_iounmap:
+   iounmap(base);
+err_free_syscon:
+   kfree(syscon);
+
+   return ERR_PTR(err);
+}
+EXPORT_SYMBOL_GPL(syscon_register);
+
 static int syscon_probe(struct platform_device *pdev)
 {
struct device *dev = &pdev->dev;
diff --git a/include/linux/mfd/syscon.h b/include/linux/mfd/syscon.h
index 40a76b9..ce531b4 100644
--- a/include/linux/mfd/syscon.h
+++ b/include/linux/mfd/syscon.h
@@ -27,6 +27,8 @@ extern struct regmap *syscon_regmap_lookup_by_pdevname(const 
char *s);
 extern struct regmap *syscon_regmap_lookup_by_phandle(
struct device_node *np,
const char *property);
+extern struct regmap *syscon_register(resource_size_t start, size_t size,
+ const char *compatible);
 #else
 static inline struct regmap *syscon_node_to_regmap(struct device_node *np)
 {
@@ -49,6 +51,13 @@ static inline struct regmap *syscon_regmap_lookup_by_phandle(
 {
return ERR_PTR(-ENOTSUPP);
 }
+
+static inline struct regmap *syscon_register(resource_size_t start,
+size_t size,
+const char *id)
+{
+   return ERR_PTR(-ENOTSUPP);
+}
 #endif
 
 #endif /* __LINUX_MFD_SYSCON_H__ */
-- 
2.7.4