Re: [PATCH 01/14] dmaengine: dma-jz4780: Avoid hardcoding number of channels

[PrasannaKumar Muralidharan]

On 5 July 2018 at 23:56, Paul Cercueil  wrote:
> Hi PrasannaKumar,
>
>
>> Hi Paul,
>>
>> On 3 July 2018 at 18:02, Paul Cercueil  wrote:
>>>
>>>  As part of the work to support various other Ingenic JZ47xx SoC
>>> versions,
>>>  which don't feature the same number of DMA channels per core, we now
>>>  deduce the number of DMA channels available from the devicetree
>>>  compatible string.
>>>
>>>  Signed-off-by: Paul Cercueil 
>>>  ---
>>>   drivers/dma/dma-jz4780.c | 53 +---
>>>   1 file changed, 39 insertions(+), 14 deletions(-)
>>>
>>>  diff --git a/drivers/dma/dma-jz4780.c b/drivers/dma/dma-jz4780.c
>>>  index 85820a2d69d4..b40f491f0367 100644
>>>  --- a/drivers/dma/dma-jz4780.c
>>>  +++ b/drivers/dma/dma-jz4780.c
>>>  @@ -16,6 +16,7 @@
>>>   #include 
>>>   #include 
>>>   #include 
>>>  +#include 
>>>   #include 
>>>   #include 
>>>   #include 
>>>  @@ -23,8 +24,6 @@
>>>   #include "dmaengine.h"
>>>   #include "virt-dma.h"
>>>
>>>  -#define JZ_DMA_NR_CHANNELS 32
>>>  -
>>>   /* Global registers. */
>>>   #define JZ_DMA_REG_DMAC0x1000
>>>   #define JZ_DMA_REG_DIRQP   0x1004
>>>  @@ -135,14 +134,20 @@ struct jz4780_dma_chan {
>>>  unsigned int curr_hwdesc;
>>>   };
>>>
>>>  +enum jz_version {
>>>  +   ID_JZ4780,
>>>  +};
>>>  +
>>>   struct jz4780_dma_dev {
>>>  struct dma_device dma_device;
>>>  void __iomem *base;
>>>  struct clk *clk;
>>>  unsigned int irq;
>>>  +   unsigned int nb_channels;
>>>  +   enum jz_version version;
>>>
>>>  uint32_t chan_reserved;
>>>  -   struct jz4780_dma_chan chan[JZ_DMA_NR_CHANNELS];
>>>  +   struct jz4780_dma_chan chan[];
>>
>>
>> Looks like a variable length array in struct. I think there is some
>> effort to remove the usage of VLA. Can you revisit this? I may be
>> wrong, please feel free to correct.
>
>
> Are you sure? It's the first time I hear about it.
> Could anybody confirm?

Please see [1] for info.

Variable Length Arrays in struct is expressly forbidden in C99, C11.
Clang does not support it. To make kernel compile with Clang few
people are trying to remove/reduce VLAIS usage.

1. 
https://blog.linuxplumbersconf.org/2013/ocw/system/presentations/1221/original/VLAIS.pdf

Re: [PATCH 01/14] dmaengine: dma-jz4780: Avoid hardcoding number of channels

[PrasannaKumar Muralidharan]

On 5 July 2018 at 23:56, Paul Cercueil  wrote:
> Hi PrasannaKumar,
>
>
>> Hi Paul,
>>
>> On 3 July 2018 at 18:02, Paul Cercueil  wrote:
>>>
>>>  As part of the work to support various other Ingenic JZ47xx SoC
>>> versions,
>>>  which don't feature the same number of DMA channels per core, we now
>>>  deduce the number of DMA channels available from the devicetree
>>>  compatible string.
>>>
>>>  Signed-off-by: Paul Cercueil 
>>>  ---
>>>   drivers/dma/dma-jz4780.c | 53 +---
>>>   1 file changed, 39 insertions(+), 14 deletions(-)
>>>
>>>  diff --git a/drivers/dma/dma-jz4780.c b/drivers/dma/dma-jz4780.c
>>>  index 85820a2d69d4..b40f491f0367 100644
>>>  --- a/drivers/dma/dma-jz4780.c
>>>  +++ b/drivers/dma/dma-jz4780.c
>>>  @@ -16,6 +16,7 @@
>>>   #include 
>>>   #include 
>>>   #include 
>>>  +#include 
>>>   #include 
>>>   #include 
>>>   #include 
>>>  @@ -23,8 +24,6 @@
>>>   #include "dmaengine.h"
>>>   #include "virt-dma.h"
>>>
>>>  -#define JZ_DMA_NR_CHANNELS 32
>>>  -
>>>   /* Global registers. */
>>>   #define JZ_DMA_REG_DMAC0x1000
>>>   #define JZ_DMA_REG_DIRQP   0x1004
>>>  @@ -135,14 +134,20 @@ struct jz4780_dma_chan {
>>>  unsigned int curr_hwdesc;
>>>   };
>>>
>>>  +enum jz_version {
>>>  +   ID_JZ4780,
>>>  +};
>>>  +
>>>   struct jz4780_dma_dev {
>>>  struct dma_device dma_device;
>>>  void __iomem *base;
>>>  struct clk *clk;
>>>  unsigned int irq;
>>>  +   unsigned int nb_channels;
>>>  +   enum jz_version version;
>>>
>>>  uint32_t chan_reserved;
>>>  -   struct jz4780_dma_chan chan[JZ_DMA_NR_CHANNELS];
>>>  +   struct jz4780_dma_chan chan[];
>>
>>
>> Looks like a variable length array in struct. I think there is some
>> effort to remove the usage of VLA. Can you revisit this? I may be
>> wrong, please feel free to correct.
>
>
> Are you sure? It's the first time I hear about it.
> Could anybody confirm?

Please see [1] for info.

Variable Length Arrays in struct is expressly forbidden in C99, C11.
Clang does not support it. To make kernel compile with Clang few
people are trying to remove/reduce VLAIS usage.

1. 
https://blog.linuxplumbersconf.org/2013/ocw/system/presentations/1221/original/VLAIS.pdf

Re: [PATCH 02/14] dmaengine: dma-jz4780: Separate chan/ctrl registers

[PrasannaKumar Muralidharan]

On 6 July 2018 at 03:15, Paul Cercueil  wrote:
>
>
>> Paul,
>>
>> On 3 July 2018 at 18:02, Paul Cercueil  wrote:
>>>
>>>  The register area of the JZ4780 DMA core can be split into different
>>>  sections for different purposes:
>>>
>>>  * one set of registers is used to perform actions at the DMA core level,
>>>  that will generally affect all channels;
>>>
>>>  * one set of registers per DMA channel, to perform actions at the DMA
>>>  channel level, that will only affect the channel in question.
>>>
>>>  The problem rises when trying to support new versions of the JZ47xx
>>>  Ingenic SoC. For instance, the JZ4770 has two DMA cores, each one
>>>  with six DMA channels, and the register sets are interleaved:
>>> 
>>>
>>>  By using one memory resource for the channel-specific registers and
>>>  one memory resource for the core-specific registers, we can support
>>>  the JZ4770, by initializing the driver once per DMA core with different
>>>  addresses.
>>
>>
>> As per my understanding device tree should be modified only when
>> hardware changes. This looks the other way around. It must be possible
>> to achieve what you are trying to do in this patch without changing
>> the device tree.
>
>
> I would agree that devicetree has an ABI that we shouldn't break if
> possible.
>
> However DTS support for all the Ingenic SoCs/boards is far from being
> complete, and more importantly, all Ingenic-based boards compile the DTS
> file within the kernel; so breaking the ABI is not (yet) a problem, and
> we should push the big changes right now while it's still possible.

Completely agree with you in this. Let's wait and see what DT maintainer's view.

Re: [PATCH 02/14] dmaengine: dma-jz4780: Separate chan/ctrl registers

[PrasannaKumar Muralidharan]

On 6 July 2018 at 03:15, Paul Cercueil  wrote:
>
>
>> Paul,
>>
>> On 3 July 2018 at 18:02, Paul Cercueil  wrote:
>>>
>>>  The register area of the JZ4780 DMA core can be split into different
>>>  sections for different purposes:
>>>
>>>  * one set of registers is used to perform actions at the DMA core level,
>>>  that will generally affect all channels;
>>>
>>>  * one set of registers per DMA channel, to perform actions at the DMA
>>>  channel level, that will only affect the channel in question.
>>>
>>>  The problem rises when trying to support new versions of the JZ47xx
>>>  Ingenic SoC. For instance, the JZ4770 has two DMA cores, each one
>>>  with six DMA channels, and the register sets are interleaved:
>>> 
>>>
>>>  By using one memory resource for the channel-specific registers and
>>>  one memory resource for the core-specific registers, we can support
>>>  the JZ4770, by initializing the driver once per DMA core with different
>>>  addresses.
>>
>>
>> As per my understanding device tree should be modified only when
>> hardware changes. This looks the other way around. It must be possible
>> to achieve what you are trying to do in this patch without changing
>> the device tree.
>
>
> I would agree that devicetree has an ABI that we shouldn't break if
> possible.
>
> However DTS support for all the Ingenic SoCs/boards is far from being
> complete, and more importantly, all Ingenic-based boards compile the DTS
> file within the kernel; so breaking the ABI is not (yet) a problem, and
> we should push the big changes right now while it's still possible.

Completely agree with you in this. Let's wait and see what DT maintainer's view.

Re: [PATCH 07/14] dmaengine: dma-jz4780: Enable Fast DMA to the AIC

[PrasannaKumar Muralidharan]

On 3 July 2018 at 18:02, Paul Cercueil  wrote:
> With the fast DMA bit set, the DMA will transfer twice as much data
> per clock period to the AIC, so there is little point not to set it.
>
> Signed-off-by: Paul Cercueil 
> ---
>  drivers/dma/dma-jz4780.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/dma/dma-jz4780.c b/drivers/dma/dma-jz4780.c
> index 922e4031e70e..7ee2c121948f 100644
> --- a/drivers/dma/dma-jz4780.c
> +++ b/drivers/dma/dma-jz4780.c
> @@ -52,6 +52,7 @@
>  #define JZ_DMA_DMAC_DMAE   BIT(0)
>  #define JZ_DMA_DMAC_AR BIT(2)
>  #define JZ_DMA_DMAC_HLTBIT(3)
> +#define JZ_DMA_DMAC_FAIC   BIT(27)
>  #define JZ_DMA_DMAC_FMSC   BIT(31)
>
>  #define JZ_DMA_DRT_AUTO0x8
> @@ -929,8 +930,8 @@ static int jz4780_dma_probe(struct platform_device *pdev)
>  * Also set the FMSC bit - it increases MSC performance, so it makes
>  * little sense not to enable it.
>  */
> -   jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DMAC,
> - JZ_DMA_DMAC_DMAE | JZ_DMA_DMAC_FMSC);
> +   jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DMAC, JZ_DMA_DMAC_DMAE |
> +  JZ_DMA_DMAC_FAIC | JZ_DMA_DMAC_FMSC);
>
> if (jzdma->version == ID_JZ4780)
> jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DMACP, 0);
> --
> 2.18.0
>
>

Reviewed-by: PrasannaKumar Muralidharan .

Re: [PATCH 07/14] dmaengine: dma-jz4780: Enable Fast DMA to the AIC

[PrasannaKumar Muralidharan]

On 3 July 2018 at 18:02, Paul Cercueil  wrote:
> With the fast DMA bit set, the DMA will transfer twice as much data
> per clock period to the AIC, so there is little point not to set it.
>
> Signed-off-by: Paul Cercueil 
> ---
>  drivers/dma/dma-jz4780.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/dma/dma-jz4780.c b/drivers/dma/dma-jz4780.c
> index 922e4031e70e..7ee2c121948f 100644
> --- a/drivers/dma/dma-jz4780.c
> +++ b/drivers/dma/dma-jz4780.c
> @@ -52,6 +52,7 @@
>  #define JZ_DMA_DMAC_DMAE   BIT(0)
>  #define JZ_DMA_DMAC_AR BIT(2)
>  #define JZ_DMA_DMAC_HLTBIT(3)
> +#define JZ_DMA_DMAC_FAIC   BIT(27)
>  #define JZ_DMA_DMAC_FMSC   BIT(31)
>
>  #define JZ_DMA_DRT_AUTO0x8
> @@ -929,8 +930,8 @@ static int jz4780_dma_probe(struct platform_device *pdev)
>  * Also set the FMSC bit - it increases MSC performance, so it makes
>  * little sense not to enable it.
>  */
> -   jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DMAC,
> - JZ_DMA_DMAC_DMAE | JZ_DMA_DMAC_FMSC);
> +   jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DMAC, JZ_DMA_DMAC_DMAE |
> +  JZ_DMA_DMAC_FAIC | JZ_DMA_DMAC_FMSC);
>
> if (jzdma->version == ID_JZ4780)
> jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DMACP, 0);
> --
> 2.18.0
>
>

Reviewed-by: PrasannaKumar Muralidharan .

Re: [PATCH 06/14] dmaengine: dma-jz4780: Add support for the JZ4725B SoC

[PrasannaKumar Muralidharan]

On 3 July 2018 at 18:02, Paul Cercueil  wrote:
> The JZ4725B has one DMA core starring six DMA channels.
> As for the JZ4770, each DMA channel's clock can be enabled with
> a register write, the difference here being that once started, it
> is not possible to turn it off.
>
> Signed-off-by: Paul Cercueil 
> ---
>  Documentation/devicetree/bindings/dma/jz4780-dma.txt | 1 +
>  drivers/dma/dma-jz4780.c | 6 ++
>  2 files changed, 7 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/dma/jz4780-dma.txt 
> b/Documentation/devicetree/bindings/dma/jz4780-dma.txt
> index d7ca3f925fdf..5d302b488e88 100644
> --- a/Documentation/devicetree/bindings/dma/jz4780-dma.txt
> +++ b/Documentation/devicetree/bindings/dma/jz4780-dma.txt
> @@ -5,6 +5,7 @@ Required properties:
>  - compatible: Should be one of:
>* ingenic,jz4780-dma
>* ingenic,jz4770-dma
> +  * ingenic,jz4725b-dma
>* ingenic,jz4740-dma
>  - reg: Should contain the DMA channel registers location and length, followed
>by the DMA controller registers location and length.
> diff --git a/drivers/dma/dma-jz4780.c b/drivers/dma/dma-jz4780.c
> index ccadbe61dde7..922e4031e70e 100644
> --- a/drivers/dma/dma-jz4780.c
> +++ b/drivers/dma/dma-jz4780.c
> @@ -134,6 +134,7 @@ struct jz4780_dma_chan {
>
>  enum jz_version {
> ID_JZ4740,
> +   ID_JZ4725B,
> ID_JZ4770,
> ID_JZ4780,
>  };
> @@ -204,6 +205,8 @@ static inline void jz4780_dma_chan_enable(struct 
> jz4780_dma_dev *jzdma,
>  {
> if (jzdma->version == ID_JZ4770)
> jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DCKES, BIT(chn));
> +   else if (jzdma->version == ID_JZ4725B)
> +   jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DCKE, BIT(chn));
>  }
>
>  static inline void jz4780_dma_chan_disable(struct jz4780_dma_dev *jzdma,
> @@ -249,6 +252,7 @@ static void jz4780_dma_desc_free(struct virt_dma_desc 
> *vdesc)
>
>  static const unsigned int jz4780_dma_ord_max[] = {
> [ID_JZ4740] = 5,
> +   [ID_JZ4725B] = 5,
> [ID_JZ4770] = 6,
> [ID_JZ4780] = 7,
>  };
> @@ -804,12 +808,14 @@ static struct dma_chan *jz4780_of_dma_xlate(struct 
> of_phandle_args *dma_spec,
>
>  static const unsigned int jz4780_dma_nb_channels[] = {
> [ID_JZ4740] = 6,
> +   [ID_JZ4725B] = 6,
> [ID_JZ4770] = 6,
> [ID_JZ4780] = 32,
>  };
>
>  static const struct of_device_id jz4780_dma_dt_match[] = {
> { .compatible = "ingenic,jz4740-dma", .data = (void *)ID_JZ4740 },
> +   { .compatible = "ingenic,jz4725b-dma", .data = (void *)ID_JZ4725B },
> { .compatible = "ingenic,jz4770-dma", .data = (void *)ID_JZ4770 },
> { .compatible = "ingenic,jz4780-dma", .data = (void *)ID_JZ4780 },
> {},
> --
> 2.18.0
>
>

Reviewed-by: PrasannaKumar Muralidharan 

Re: [PATCH 06/14] dmaengine: dma-jz4780: Add support for the JZ4725B SoC

[PrasannaKumar Muralidharan]

On 3 July 2018 at 18:02, Paul Cercueil  wrote:
> The JZ4725B has one DMA core starring six DMA channels.
> As for the JZ4770, each DMA channel's clock can be enabled with
> a register write, the difference here being that once started, it
> is not possible to turn it off.
>
> Signed-off-by: Paul Cercueil 
> ---
>  Documentation/devicetree/bindings/dma/jz4780-dma.txt | 1 +
>  drivers/dma/dma-jz4780.c | 6 ++
>  2 files changed, 7 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/dma/jz4780-dma.txt 
> b/Documentation/devicetree/bindings/dma/jz4780-dma.txt
> index d7ca3f925fdf..5d302b488e88 100644
> --- a/Documentation/devicetree/bindings/dma/jz4780-dma.txt
> +++ b/Documentation/devicetree/bindings/dma/jz4780-dma.txt
> @@ -5,6 +5,7 @@ Required properties:
>  - compatible: Should be one of:
>* ingenic,jz4780-dma
>* ingenic,jz4770-dma
> +  * ingenic,jz4725b-dma
>* ingenic,jz4740-dma
>  - reg: Should contain the DMA channel registers location and length, followed
>by the DMA controller registers location and length.
> diff --git a/drivers/dma/dma-jz4780.c b/drivers/dma/dma-jz4780.c
> index ccadbe61dde7..922e4031e70e 100644
> --- a/drivers/dma/dma-jz4780.c
> +++ b/drivers/dma/dma-jz4780.c
> @@ -134,6 +134,7 @@ struct jz4780_dma_chan {
>
>  enum jz_version {
> ID_JZ4740,
> +   ID_JZ4725B,
> ID_JZ4770,
> ID_JZ4780,
>  };
> @@ -204,6 +205,8 @@ static inline void jz4780_dma_chan_enable(struct 
> jz4780_dma_dev *jzdma,
>  {
> if (jzdma->version == ID_JZ4770)
> jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DCKES, BIT(chn));
> +   else if (jzdma->version == ID_JZ4725B)
> +   jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DCKE, BIT(chn));
>  }
>
>  static inline void jz4780_dma_chan_disable(struct jz4780_dma_dev *jzdma,
> @@ -249,6 +252,7 @@ static void jz4780_dma_desc_free(struct virt_dma_desc 
> *vdesc)
>
>  static const unsigned int jz4780_dma_ord_max[] = {
> [ID_JZ4740] = 5,
> +   [ID_JZ4725B] = 5,
> [ID_JZ4770] = 6,
> [ID_JZ4780] = 7,
>  };
> @@ -804,12 +808,14 @@ static struct dma_chan *jz4780_of_dma_xlate(struct 
> of_phandle_args *dma_spec,
>
>  static const unsigned int jz4780_dma_nb_channels[] = {
> [ID_JZ4740] = 6,
> +   [ID_JZ4725B] = 6,
> [ID_JZ4770] = 6,
> [ID_JZ4780] = 32,
>  };
>
>  static const struct of_device_id jz4780_dma_dt_match[] = {
> { .compatible = "ingenic,jz4740-dma", .data = (void *)ID_JZ4740 },
> +   { .compatible = "ingenic,jz4725b-dma", .data = (void *)ID_JZ4725B },
> { .compatible = "ingenic,jz4770-dma", .data = (void *)ID_JZ4770 },
> { .compatible = "ingenic,jz4780-dma", .data = (void *)ID_JZ4780 },
> {},
> --
> 2.18.0
>
>

Reviewed-by: PrasannaKumar Muralidharan 

Re: [PATCH 05/14] dmaengine: dma-jz4780: Add support for the JZ4740 SoC

[PrasannaKumar Muralidharan]

On 3 July 2018 at 18:02, Paul Cercueil  wrote:
> The JZ4740 SoC has a single DMA core starring six DMA channels.
>
> Signed-off-by: Paul Cercueil 
> ---
>  Documentation/devicetree/bindings/dma/jz4780-dma.txt | 1 +
>  drivers/dma/Kconfig  | 2 +-
>  drivers/dma/dma-jz4780.c | 4 
>  3 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/dma/jz4780-dma.txt 
> b/Documentation/devicetree/bindings/dma/jz4780-dma.txt
> index 0fd0759053be..d7ca3f925fdf 100644
> --- a/Documentation/devicetree/bindings/dma/jz4780-dma.txt
> +++ b/Documentation/devicetree/bindings/dma/jz4780-dma.txt
> @@ -5,6 +5,7 @@ Required properties:
>  - compatible: Should be one of:
>* ingenic,jz4780-dma
>* ingenic,jz4770-dma
> +  * ingenic,jz4740-dma
>  - reg: Should contain the DMA channel registers location and length, followed
>by the DMA controller registers location and length.
>  - interrupts: Should contain the interrupt specifier of the DMA controller.
> diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
> index 48d25dccedb7..a935d15ec581 100644
> --- a/drivers/dma/Kconfig
> +++ b/drivers/dma/Kconfig
> @@ -143,7 +143,7 @@ config DMA_JZ4740
>
>  config DMA_JZ4780
> tristate "JZ4780 DMA support"
> -   depends on MACH_JZ4780 || MACH_JZ4770 || COMPILE_TEST
> +   depends on MACH_INGENIC || COMPILE_TEST
> select DMA_ENGINE
> select DMA_VIRTUAL_CHANNELS
> help
> diff --git a/drivers/dma/dma-jz4780.c b/drivers/dma/dma-jz4780.c
> index 7b8b2dcd119e..ccadbe61dde7 100644
> --- a/drivers/dma/dma-jz4780.c
> +++ b/drivers/dma/dma-jz4780.c
> @@ -133,6 +133,7 @@ struct jz4780_dma_chan {
>  };
>
>  enum jz_version {
> +   ID_JZ4740,
> ID_JZ4770,
> ID_JZ4780,
>  };
> @@ -247,6 +248,7 @@ static void jz4780_dma_desc_free(struct virt_dma_desc 
> *vdesc)
>  }
>
>  static const unsigned int jz4780_dma_ord_max[] = {
> +   [ID_JZ4740] = 5,
> [ID_JZ4770] = 6,
> [ID_JZ4780] = 7,
>  };
> @@ -801,11 +803,13 @@ static struct dma_chan *jz4780_of_dma_xlate(struct 
> of_phandle_args *dma_spec,
>  }
>
>  static const unsigned int jz4780_dma_nb_channels[] = {
> +   [ID_JZ4740] = 6,
> [ID_JZ4770] = 6,
> [ID_JZ4780] = 32,
>  };
>
>  static const struct of_device_id jz4780_dma_dt_match[] = {
> +   { .compatible = "ingenic,jz4740-dma", .data = (void *)ID_JZ4740 },
> { .compatible = "ingenic,jz4770-dma", .data = (void *)ID_JZ4770 },
> { .compatible = "ingenic,jz4780-dma", .data = (void *)ID_JZ4780 },
> {},
> --
> 2.18.0
>
>

Patch looks good to me.
Reviewed-by: PrasannaKumar Muralidharan /

Re: [PATCH 05/14] dmaengine: dma-jz4780: Add support for the JZ4740 SoC

[PrasannaKumar Muralidharan]

On 3 July 2018 at 18:02, Paul Cercueil  wrote:
> The JZ4740 SoC has a single DMA core starring six DMA channels.
>
> Signed-off-by: Paul Cercueil 
> ---
>  Documentation/devicetree/bindings/dma/jz4780-dma.txt | 1 +
>  drivers/dma/Kconfig  | 2 +-
>  drivers/dma/dma-jz4780.c | 4 
>  3 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/dma/jz4780-dma.txt 
> b/Documentation/devicetree/bindings/dma/jz4780-dma.txt
> index 0fd0759053be..d7ca3f925fdf 100644
> --- a/Documentation/devicetree/bindings/dma/jz4780-dma.txt
> +++ b/Documentation/devicetree/bindings/dma/jz4780-dma.txt
> @@ -5,6 +5,7 @@ Required properties:
>  - compatible: Should be one of:
>* ingenic,jz4780-dma
>* ingenic,jz4770-dma
> +  * ingenic,jz4740-dma
>  - reg: Should contain the DMA channel registers location and length, followed
>by the DMA controller registers location and length.
>  - interrupts: Should contain the interrupt specifier of the DMA controller.
> diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
> index 48d25dccedb7..a935d15ec581 100644
> --- a/drivers/dma/Kconfig
> +++ b/drivers/dma/Kconfig
> @@ -143,7 +143,7 @@ config DMA_JZ4740
>
>  config DMA_JZ4780
> tristate "JZ4780 DMA support"
> -   depends on MACH_JZ4780 || MACH_JZ4770 || COMPILE_TEST
> +   depends on MACH_INGENIC || COMPILE_TEST
> select DMA_ENGINE
> select DMA_VIRTUAL_CHANNELS
> help
> diff --git a/drivers/dma/dma-jz4780.c b/drivers/dma/dma-jz4780.c
> index 7b8b2dcd119e..ccadbe61dde7 100644
> --- a/drivers/dma/dma-jz4780.c
> +++ b/drivers/dma/dma-jz4780.c
> @@ -133,6 +133,7 @@ struct jz4780_dma_chan {
>  };
>
>  enum jz_version {
> +   ID_JZ4740,
> ID_JZ4770,
> ID_JZ4780,
>  };
> @@ -247,6 +248,7 @@ static void jz4780_dma_desc_free(struct virt_dma_desc 
> *vdesc)
>  }
>
>  static const unsigned int jz4780_dma_ord_max[] = {
> +   [ID_JZ4740] = 5,
> [ID_JZ4770] = 6,
> [ID_JZ4780] = 7,
>  };
> @@ -801,11 +803,13 @@ static struct dma_chan *jz4780_of_dma_xlate(struct 
> of_phandle_args *dma_spec,
>  }
>
>  static const unsigned int jz4780_dma_nb_channels[] = {
> +   [ID_JZ4740] = 6,
> [ID_JZ4770] = 6,
> [ID_JZ4780] = 32,
>  };
>
>  static const struct of_device_id jz4780_dma_dt_match[] = {
> +   { .compatible = "ingenic,jz4740-dma", .data = (void *)ID_JZ4740 },
> { .compatible = "ingenic,jz4770-dma", .data = (void *)ID_JZ4770 },
> { .compatible = "ingenic,jz4780-dma", .data = (void *)ID_JZ4780 },
> {},
> --
> 2.18.0
>
>

Patch looks good to me.
Reviewed-by: PrasannaKumar Muralidharan /

Re: [PATCH 03/14] dmaengine: dma-jz4780: Use 4-word descriptors

[PrasannaKumar Muralidharan]

Hi Paul,

On 3 July 2018 at 18:02, Paul Cercueil  wrote:
> The only information we use in the 8-word version of the hardware DMA
> descriptor that is not present in the 4-word version is the transfer
> type, aka. the ID of the source or recipient device.
>
> Since the transfer type will never change for a DMA channel in use,
> we can just set it once for all in the corresponding DMA register
> before starting any transfer.
>
> This has several benefits:
>
> * the driver will handle twice as many hardware DMA descriptors;
>
> * the driver is closer to support the JZ4740, which only supports 4-word
>   hardware DMA descriptors;
>
> * the JZ4770 SoC needs the transfer type to be set in the corresponding
>   DMA register anyway, even if 8-word descriptors are in use.
>
> Signed-off-by: Paul Cercueil 
> ---
>  drivers/dma/dma-jz4780.c | 21 +
>  1 file changed, 9 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/dma/dma-jz4780.c b/drivers/dma/dma-jz4780.c
> index 4d234caf5d62..cd2cd70fd843 100644
> --- a/drivers/dma/dma-jz4780.c
> +++ b/drivers/dma/dma-jz4780.c
> @@ -93,17 +93,12 @@
>   * @dtc: transfer count (number of blocks of the transfer size specified in 
> DCM
>   * to transfer) in the low 24 bits, offset of the next descriptor from the
>   * descriptor base address in the upper 8 bits.
> - * @sd: target/source stride difference (in stride transfer mode).
> - * @drt: request type
>   */
>  struct jz4780_dma_hwdesc {
> uint32_t dcm;
> uint32_t dsa;
> uint32_t dta;
> uint32_t dtc;
> -   uint32_t sd;
> -   uint32_t drt;
> -   uint32_t reserved[2];
>  };
>
>  /* Size of allocations for hardware descriptor blocks. */
> @@ -280,7 +275,6 @@ static int jz4780_dma_setup_hwdesc(struct jz4780_dma_chan 
> *jzchan,
> desc->dcm = JZ_DMA_DCM_SAI;
> desc->dsa = addr;
> desc->dta = config->dst_addr;
> -   desc->drt = jzchan->transfer_type;
>
> width = config->dst_addr_width;
> maxburst = config->dst_maxburst;
> @@ -288,7 +282,6 @@ static int jz4780_dma_setup_hwdesc(struct jz4780_dma_chan 
> *jzchan,
> desc->dcm = JZ_DMA_DCM_DAI;
> desc->dsa = config->src_addr;
> desc->dta = addr;
> -   desc->drt = jzchan->transfer_type;
>
> width = config->src_addr_width;
> maxburst = config->src_maxburst;
> @@ -433,9 +426,10 @@ static struct dma_async_tx_descriptor 
> *jz4780_dma_prep_dma_memcpy(
> tsz = jz4780_dma_transfer_size(dest | src | len,
>>transfer_shift);
>
> +   jzchan->transfer_type = JZ_DMA_DRT_AUTO;
> +
> desc->desc[0].dsa = src;
> desc->desc[0].dta = dest;
> -   desc->desc[0].drt = JZ_DMA_DRT_AUTO;
> desc->desc[0].dcm = JZ_DMA_DCM_TIE | JZ_DMA_DCM_SAI | JZ_DMA_DCM_DAI |
> tsz << JZ_DMA_DCM_TSZ_SHIFT |
> JZ_DMA_WIDTH_32_BIT << JZ_DMA_DCM_SP_SHIFT |
> @@ -490,9 +484,12 @@ static void jz4780_dma_begin(struct jz4780_dma_chan 
> *jzchan)
> (jzchan->curr_hwdesc + 1) % jzchan->desc->count;
> }
>
> -   /* Use 8-word descriptors. */
> -   jz4780_dma_chn_writel(jzdma, jzchan->id,
> - JZ_DMA_REG_DCS, JZ_DMA_DCS_DES8);
> +   /* Use 4-word descriptors. */
> +   jz4780_dma_chn_writel(jzdma, jzchan->id, JZ_DMA_REG_DCS, 0);
> +
> +   /* Set transfer type. */
> +   jz4780_dma_chn_writel(jzdma, jzchan->id, JZ_DMA_REG_DRT,
> + jzchan->transfer_type);
>
> /* Write descriptor address and initiate descriptor fetch. */
> desc_phys = jzchan->desc->desc_phys +
> @@ -502,7 +499,7 @@ static void jz4780_dma_begin(struct jz4780_dma_chan 
> *jzchan)
>
> /* Enable the channel. */
> jz4780_dma_chn_writel(jzdma, jzchan->id, JZ_DMA_REG_DCS,
> - JZ_DMA_DCS_DES8 | JZ_DMA_DCS_CTE);
> + JZ_DMA_DCS_CTE);
>  }
>
>  static void jz4780_dma_issue_pending(struct dma_chan *chan)
> --
> 2.18.0
>
>

Patch looks good to me.
Reviewed-by: PrasannaKumar Muralidharan 

Regards,
PrasannaKumar

Re: [PATCH 03/14] dmaengine: dma-jz4780: Use 4-word descriptors

[PrasannaKumar Muralidharan]

Hi Paul,

On 3 July 2018 at 18:02, Paul Cercueil  wrote:
> The only information we use in the 8-word version of the hardware DMA
> descriptor that is not present in the 4-word version is the transfer
> type, aka. the ID of the source or recipient device.
>
> Since the transfer type will never change for a DMA channel in use,
> we can just set it once for all in the corresponding DMA register
> before starting any transfer.
>
> This has several benefits:
>
> * the driver will handle twice as many hardware DMA descriptors;
>
> * the driver is closer to support the JZ4740, which only supports 4-word
>   hardware DMA descriptors;
>
> * the JZ4770 SoC needs the transfer type to be set in the corresponding
>   DMA register anyway, even if 8-word descriptors are in use.
>
> Signed-off-by: Paul Cercueil 
> ---
>  drivers/dma/dma-jz4780.c | 21 +
>  1 file changed, 9 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/dma/dma-jz4780.c b/drivers/dma/dma-jz4780.c
> index 4d234caf5d62..cd2cd70fd843 100644
> --- a/drivers/dma/dma-jz4780.c
> +++ b/drivers/dma/dma-jz4780.c
> @@ -93,17 +93,12 @@
>   * @dtc: transfer count (number of blocks of the transfer size specified in 
> DCM
>   * to transfer) in the low 24 bits, offset of the next descriptor from the
>   * descriptor base address in the upper 8 bits.
> - * @sd: target/source stride difference (in stride transfer mode).
> - * @drt: request type
>   */
>  struct jz4780_dma_hwdesc {
> uint32_t dcm;
> uint32_t dsa;
> uint32_t dta;
> uint32_t dtc;
> -   uint32_t sd;
> -   uint32_t drt;
> -   uint32_t reserved[2];
>  };
>
>  /* Size of allocations for hardware descriptor blocks. */
> @@ -280,7 +275,6 @@ static int jz4780_dma_setup_hwdesc(struct jz4780_dma_chan 
> *jzchan,
> desc->dcm = JZ_DMA_DCM_SAI;
> desc->dsa = addr;
> desc->dta = config->dst_addr;
> -   desc->drt = jzchan->transfer_type;
>
> width = config->dst_addr_width;
> maxburst = config->dst_maxburst;
> @@ -288,7 +282,6 @@ static int jz4780_dma_setup_hwdesc(struct jz4780_dma_chan 
> *jzchan,
> desc->dcm = JZ_DMA_DCM_DAI;
> desc->dsa = config->src_addr;
> desc->dta = addr;
> -   desc->drt = jzchan->transfer_type;
>
> width = config->src_addr_width;
> maxburst = config->src_maxburst;
> @@ -433,9 +426,10 @@ static struct dma_async_tx_descriptor 
> *jz4780_dma_prep_dma_memcpy(
> tsz = jz4780_dma_transfer_size(dest | src | len,
>>transfer_shift);
>
> +   jzchan->transfer_type = JZ_DMA_DRT_AUTO;
> +
> desc->desc[0].dsa = src;
> desc->desc[0].dta = dest;
> -   desc->desc[0].drt = JZ_DMA_DRT_AUTO;
> desc->desc[0].dcm = JZ_DMA_DCM_TIE | JZ_DMA_DCM_SAI | JZ_DMA_DCM_DAI |
> tsz << JZ_DMA_DCM_TSZ_SHIFT |
> JZ_DMA_WIDTH_32_BIT << JZ_DMA_DCM_SP_SHIFT |
> @@ -490,9 +484,12 @@ static void jz4780_dma_begin(struct jz4780_dma_chan 
> *jzchan)
> (jzchan->curr_hwdesc + 1) % jzchan->desc->count;
> }
>
> -   /* Use 8-word descriptors. */
> -   jz4780_dma_chn_writel(jzdma, jzchan->id,
> - JZ_DMA_REG_DCS, JZ_DMA_DCS_DES8);
> +   /* Use 4-word descriptors. */
> +   jz4780_dma_chn_writel(jzdma, jzchan->id, JZ_DMA_REG_DCS, 0);
> +
> +   /* Set transfer type. */
> +   jz4780_dma_chn_writel(jzdma, jzchan->id, JZ_DMA_REG_DRT,
> + jzchan->transfer_type);
>
> /* Write descriptor address and initiate descriptor fetch. */
> desc_phys = jzchan->desc->desc_phys +
> @@ -502,7 +499,7 @@ static void jz4780_dma_begin(struct jz4780_dma_chan 
> *jzchan)
>
> /* Enable the channel. */
> jz4780_dma_chn_writel(jzdma, jzchan->id, JZ_DMA_REG_DCS,
> - JZ_DMA_DCS_DES8 | JZ_DMA_DCS_CTE);
> + JZ_DMA_DCS_CTE);
>  }
>
>  static void jz4780_dma_issue_pending(struct dma_chan *chan)
> --
> 2.18.0
>
>

Patch looks good to me.
Reviewed-by: PrasannaKumar Muralidharan 

Regards,
PrasannaKumar

Re: [PATCH 02/14] dmaengine: dma-jz4780: Separate chan/ctrl registers

[PrasannaKumar Muralidharan]

Paul,

On 3 July 2018 at 18:02, Paul Cercueil  wrote:
> The register area of the JZ4780 DMA core can be split into different
> sections for different purposes:
>
> * one set of registers is used to perform actions at the DMA core level,
> that will generally affect all channels;
>
> * one set of registers per DMA channel, to perform actions at the DMA
> channel level, that will only affect the channel in question.
>
> The problem rises when trying to support new versions of the JZ47xx
> Ingenic SoC. For instance, the JZ4770 has two DMA cores, each one
> with six DMA channels, and the register sets are interleaved:
>
>
> By using one memory resource for the channel-specific registers and
> one memory resource for the core-specific registers, we can support
> the JZ4770, by initializing the driver once per DMA core with different
> addresses.

As per my understanding device tree should be modified only when
hardware changes. This looks the other way around. It must be possible
to achieve what you are trying to do in this patch without changing
the device tree.

> Signed-off-by: Paul Cercueil 
> ---
>  .../devicetree/bindings/dma/jz4780-dma.txt|   6 +-
>  drivers/dma/dma-jz4780.c  | 106 +++---
>  2 files changed, 69 insertions(+), 43 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/dma/jz4780-dma.txt 
> b/Documentation/devicetree/bindings/dma/jz4780-dma.txt
> index f25feee62b15..f9b1864f5b77 100644
> --- a/Documentation/devicetree/bindings/dma/jz4780-dma.txt
> +++ b/Documentation/devicetree/bindings/dma/jz4780-dma.txt
> @@ -3,7 +3,8 @@
>  Required properties:
>
>  - compatible: Should be "ingenic,jz4780-dma"
> -- reg: Should contain the DMA controller registers location and length.
> +- reg: Should contain the DMA channel registers location and length, followed
> +  by the DMA controller registers location and length.
>  - interrupts: Should contain the interrupt specifier of the DMA controller.
>  - interrupt-parent: Should be the phandle of the interrupt controller that
>  - clocks: Should contain a clock specifier for the JZ4780 PDMA clock.
> @@ -22,7 +23,8 @@ Example:
>
>  dma: dma@1342 {
> compatible = "ingenic,jz4780-dma";
> -   reg = <0x1342 0x1>;
> +   reg = <0x1342 0x400
> +  0x13421000 0x40>;
>
> interrupt-parent = <>;
> interrupts = <10>;
> diff --git a/drivers/dma/dma-jz4780.c b/drivers/dma/dma-jz4780.c
> index b40f491f0367..4d234caf5d62 100644
> --- a/drivers/dma/dma-jz4780.c
> +++ b/drivers/dma/dma-jz4780.c
> @@ -25,26 +25,26 @@
>  #include "virt-dma.h"
>
>  /* Global registers. */
> -#define JZ_DMA_REG_DMAC0x1000
> -#define JZ_DMA_REG_DIRQP   0x1004
> -#define JZ_DMA_REG_DDR 0x1008
> -#define JZ_DMA_REG_DDRS0x100c
> -#define JZ_DMA_REG_DMACP   0x101c
> -#define JZ_DMA_REG_DSIRQP  0x1020
> -#define JZ_DMA_REG_DSIRQM  0x1024
> -#define JZ_DMA_REG_DCIRQP  0x1028
> -#define JZ_DMA_REG_DCIRQM  0x102c
> +#define JZ_DMA_REG_DMAC0x00
> +#define JZ_DMA_REG_DIRQP   0x04
> +#define JZ_DMA_REG_DDR 0x08
> +#define JZ_DMA_REG_DDRS0x0c
> +#define JZ_DMA_REG_DMACP   0x1c
> +#define JZ_DMA_REG_DSIRQP  0x20
> +#define JZ_DMA_REG_DSIRQM  0x24
> +#define JZ_DMA_REG_DCIRQP  0x28
> +#define JZ_DMA_REG_DCIRQM  0x2c
>
>  /* Per-channel registers. */
>  #define JZ_DMA_REG_CHAN(n) (n * 0x20)
> -#define JZ_DMA_REG_DSA(n)  (0x00 + JZ_DMA_REG_CHAN(n))
> -#define JZ_DMA_REG_DTA(n)  (0x04 + JZ_DMA_REG_CHAN(n))
> -#define JZ_DMA_REG_DTC(n)  (0x08 + JZ_DMA_REG_CHAN(n))
> -#define JZ_DMA_REG_DRT(n)  (0x0c + JZ_DMA_REG_CHAN(n))
> -#define JZ_DMA_REG_DCS(n)  (0x10 + JZ_DMA_REG_CHAN(n))
> -#define JZ_DMA_REG_DCM(n)  (0x14 + JZ_DMA_REG_CHAN(n))
> -#define JZ_DMA_REG_DDA(n)  (0x18 + JZ_DMA_REG_CHAN(n))
> -#define JZ_DMA_REG_DSD(n)  (0x1c + JZ_DMA_REG_CHAN(n))
> +#define JZ_DMA_REG_DSA 0x00
> +#define JZ_DMA_REG_DTA 0x04
> +#define JZ_DMA_REG_DTC 0x08
> +#define JZ_DMA_REG_DRT 0x0c
> +#define JZ_DMA_REG_DCS 0x10
> +#define JZ_DMA_REG_DCM 0x14
> +#define JZ_DMA_REG_DDA 0x18
> +#define JZ_DMA_REG_DSD 0x1c
>
>  #define JZ_DMA_DMAC_DMAE   BIT(0)
>  #define JZ_DMA_DMAC_AR BIT(2)
> @@ -140,7 +140,8 @@ enum jz_version {
>
>  struct jz4780_dma_dev {
> struct dma_device dma_device;
> -   void __iomem *base;
> +   void __iomem *chn_base;
> +   void __iomem *ctrl_base;
> struct clk *clk;
> unsigned int irq;
> unsigned int nb_channels;
> @@ -174,16 +175,28 @@ static inline struct jz4780_dma_dev 
> *jz4780_dma_chan_parent(
> dma_device);
>  }
>
> -static inline uint32_t jz4780_dma_readl(struct jz4780_dma_dev *jzdma,
> +static inline uint32_t jz4780_dma_chn_readl(struct jz4780_dma_dev *jzdma,
> +   unsigned int chn, 

Re: [PATCH 02/14] dmaengine: dma-jz4780: Separate chan/ctrl registers

[PrasannaKumar Muralidharan]

Paul,

On 3 July 2018 at 18:02, Paul Cercueil  wrote:
> The register area of the JZ4780 DMA core can be split into different
> sections for different purposes:
>
> * one set of registers is used to perform actions at the DMA core level,
> that will generally affect all channels;
>
> * one set of registers per DMA channel, to perform actions at the DMA
> channel level, that will only affect the channel in question.
>
> The problem rises when trying to support new versions of the JZ47xx
> Ingenic SoC. For instance, the JZ4770 has two DMA cores, each one
> with six DMA channels, and the register sets are interleaved:
>
>
> By using one memory resource for the channel-specific registers and
> one memory resource for the core-specific registers, we can support
> the JZ4770, by initializing the driver once per DMA core with different
> addresses.

As per my understanding device tree should be modified only when
hardware changes. This looks the other way around. It must be possible
to achieve what you are trying to do in this patch without changing
the device tree.

> Signed-off-by: Paul Cercueil 
> ---
>  .../devicetree/bindings/dma/jz4780-dma.txt|   6 +-
>  drivers/dma/dma-jz4780.c  | 106 +++---
>  2 files changed, 69 insertions(+), 43 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/dma/jz4780-dma.txt 
> b/Documentation/devicetree/bindings/dma/jz4780-dma.txt
> index f25feee62b15..f9b1864f5b77 100644
> --- a/Documentation/devicetree/bindings/dma/jz4780-dma.txt
> +++ b/Documentation/devicetree/bindings/dma/jz4780-dma.txt
> @@ -3,7 +3,8 @@
>  Required properties:
>
>  - compatible: Should be "ingenic,jz4780-dma"
> -- reg: Should contain the DMA controller registers location and length.
> +- reg: Should contain the DMA channel registers location and length, followed
> +  by the DMA controller registers location and length.
>  - interrupts: Should contain the interrupt specifier of the DMA controller.
>  - interrupt-parent: Should be the phandle of the interrupt controller that
>  - clocks: Should contain a clock specifier for the JZ4780 PDMA clock.
> @@ -22,7 +23,8 @@ Example:
>
>  dma: dma@1342 {
> compatible = "ingenic,jz4780-dma";
> -   reg = <0x1342 0x1>;
> +   reg = <0x1342 0x400
> +  0x13421000 0x40>;
>
> interrupt-parent = <>;
> interrupts = <10>;
> diff --git a/drivers/dma/dma-jz4780.c b/drivers/dma/dma-jz4780.c
> index b40f491f0367..4d234caf5d62 100644
> --- a/drivers/dma/dma-jz4780.c
> +++ b/drivers/dma/dma-jz4780.c
> @@ -25,26 +25,26 @@
>  #include "virt-dma.h"
>
>  /* Global registers. */
> -#define JZ_DMA_REG_DMAC0x1000
> -#define JZ_DMA_REG_DIRQP   0x1004
> -#define JZ_DMA_REG_DDR 0x1008
> -#define JZ_DMA_REG_DDRS0x100c
> -#define JZ_DMA_REG_DMACP   0x101c
> -#define JZ_DMA_REG_DSIRQP  0x1020
> -#define JZ_DMA_REG_DSIRQM  0x1024
> -#define JZ_DMA_REG_DCIRQP  0x1028
> -#define JZ_DMA_REG_DCIRQM  0x102c
> +#define JZ_DMA_REG_DMAC0x00
> +#define JZ_DMA_REG_DIRQP   0x04
> +#define JZ_DMA_REG_DDR 0x08
> +#define JZ_DMA_REG_DDRS0x0c
> +#define JZ_DMA_REG_DMACP   0x1c
> +#define JZ_DMA_REG_DSIRQP  0x20
> +#define JZ_DMA_REG_DSIRQM  0x24
> +#define JZ_DMA_REG_DCIRQP  0x28
> +#define JZ_DMA_REG_DCIRQM  0x2c
>
>  /* Per-channel registers. */
>  #define JZ_DMA_REG_CHAN(n) (n * 0x20)
> -#define JZ_DMA_REG_DSA(n)  (0x00 + JZ_DMA_REG_CHAN(n))
> -#define JZ_DMA_REG_DTA(n)  (0x04 + JZ_DMA_REG_CHAN(n))
> -#define JZ_DMA_REG_DTC(n)  (0x08 + JZ_DMA_REG_CHAN(n))
> -#define JZ_DMA_REG_DRT(n)  (0x0c + JZ_DMA_REG_CHAN(n))
> -#define JZ_DMA_REG_DCS(n)  (0x10 + JZ_DMA_REG_CHAN(n))
> -#define JZ_DMA_REG_DCM(n)  (0x14 + JZ_DMA_REG_CHAN(n))
> -#define JZ_DMA_REG_DDA(n)  (0x18 + JZ_DMA_REG_CHAN(n))
> -#define JZ_DMA_REG_DSD(n)  (0x1c + JZ_DMA_REG_CHAN(n))
> +#define JZ_DMA_REG_DSA 0x00
> +#define JZ_DMA_REG_DTA 0x04
> +#define JZ_DMA_REG_DTC 0x08
> +#define JZ_DMA_REG_DRT 0x0c
> +#define JZ_DMA_REG_DCS 0x10
> +#define JZ_DMA_REG_DCM 0x14
> +#define JZ_DMA_REG_DDA 0x18
> +#define JZ_DMA_REG_DSD 0x1c
>
>  #define JZ_DMA_DMAC_DMAE   BIT(0)
>  #define JZ_DMA_DMAC_AR BIT(2)
> @@ -140,7 +140,8 @@ enum jz_version {
>
>  struct jz4780_dma_dev {
> struct dma_device dma_device;
> -   void __iomem *base;
> +   void __iomem *chn_base;
> +   void __iomem *ctrl_base;
> struct clk *clk;
> unsigned int irq;
> unsigned int nb_channels;
> @@ -174,16 +175,28 @@ static inline struct jz4780_dma_dev 
> *jz4780_dma_chan_parent(
> dma_device);
>  }
>
> -static inline uint32_t jz4780_dma_readl(struct jz4780_dma_dev *jzdma,
> +static inline uint32_t jz4780_dma_chn_readl(struct jz4780_dma_dev *jzdma,
> +   unsigned int chn, 

Re: [PATCH 01/14] dmaengine: dma-jz4780: Avoid hardcoding number of channels

[PrasannaKumar Muralidharan]

Hi Paul,

On 3 July 2018 at 18:02, Paul Cercueil  wrote:
> As part of the work to support various other Ingenic JZ47xx SoC versions,
> which don't feature the same number of DMA channels per core, we now
> deduce the number of DMA channels available from the devicetree
> compatible string.
>
> Signed-off-by: Paul Cercueil 
> ---
>  drivers/dma/dma-jz4780.c | 53 +---
>  1 file changed, 39 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/dma/dma-jz4780.c b/drivers/dma/dma-jz4780.c
> index 85820a2d69d4..b40f491f0367 100644
> --- a/drivers/dma/dma-jz4780.c
> +++ b/drivers/dma/dma-jz4780.c
> @@ -16,6 +16,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -23,8 +24,6 @@
>  #include "dmaengine.h"
>  #include "virt-dma.h"
>
> -#define JZ_DMA_NR_CHANNELS 32
> -
>  /* Global registers. */
>  #define JZ_DMA_REG_DMAC0x1000
>  #define JZ_DMA_REG_DIRQP   0x1004
> @@ -135,14 +134,20 @@ struct jz4780_dma_chan {
> unsigned int curr_hwdesc;
>  };
>
> +enum jz_version {
> +   ID_JZ4780,
> +};
> +
>  struct jz4780_dma_dev {
> struct dma_device dma_device;
> void __iomem *base;
> struct clk *clk;
> unsigned int irq;
> +   unsigned int nb_channels;
> +   enum jz_version version;
>
> uint32_t chan_reserved;
> -   struct jz4780_dma_chan chan[JZ_DMA_NR_CHANNELS];
> +   struct jz4780_dma_chan chan[];

Looks like a variable length array in struct. I think there is some
effort to remove the usage of VLA. Can you revisit this? I may be
wrong, please feel free to correct.

>  };
>
>  struct jz4780_dma_filter_data {
> @@ -648,7 +653,7 @@ static irqreturn_t jz4780_dma_irq_handler(int irq, void 
> *data)
>
> pending = jz4780_dma_readl(jzdma, JZ_DMA_REG_DIRQP);
>
> -   for (i = 0; i < JZ_DMA_NR_CHANNELS; i++) {
> +   for (i = 0; i < jzdma->nb_channels; i++) {
> if (!(pending & (1< continue;
>
> @@ -728,7 +733,7 @@ static struct dma_chan *jz4780_of_dma_xlate(struct 
> of_phandle_args *dma_spec,
> data.channel = dma_spec->args[1];
>
> if (data.channel > -1) {
> -   if (data.channel >= JZ_DMA_NR_CHANNELS) {
> +   if (data.channel >= jzdma->nb_channels) {
> dev_err(jzdma->dma_device.dev,
> "device requested non-existent channel %u\n",
> data.channel);
> @@ -752,19 +757,45 @@ static struct dma_chan *jz4780_of_dma_xlate(struct 
> of_phandle_args *dma_spec,
> }
>  }
>
> +static const unsigned int jz4780_dma_nb_channels[] = {
> +   [ID_JZ4780] = 32,
> +};
> +
> +static const struct of_device_id jz4780_dma_dt_match[] = {
> +   { .compatible = "ingenic,jz4780-dma", .data = (void *)ID_JZ4780 },
> +   {},
> +};
> +MODULE_DEVICE_TABLE(of, jz4780_dma_dt_match);
> +
>  static int jz4780_dma_probe(struct platform_device *pdev)
>  {
> struct device *dev = >dev;
> +   const struct of_device_id *of_id = of_match_device(
> +   jz4780_dma_dt_match, dev);
> struct jz4780_dma_dev *jzdma;
> struct jz4780_dma_chan *jzchan;
> struct dma_device *dd;
> struct resource *res;
> +   enum jz_version version;
> +   unsigned int nb_channels;
> int i, ret;
>
> -   jzdma = devm_kzalloc(dev, sizeof(*jzdma), GFP_KERNEL);
> +   if (of_id)
> +   version = (enum jz_version)of_id->data;
> +   else
> +   version = ID_JZ4780; /* Default when not probed from DT */
> +
> +   nb_channels = jz4780_dma_nb_channels[version];
> +
> +   jzdma = devm_kzalloc(dev, sizeof(*jzdma)
> +   + sizeof(*jzdma->chan) * nb_channels,
> +   GFP_KERNEL);
> if (!jzdma)
> return -ENOMEM;
>
> +   jzdma->nb_channels = nb_channels;
> +   jzdma->version = version;
> +
> platform_set_drvdata(pdev, jzdma);
>
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> @@ -839,7 +870,7 @@ static int jz4780_dma_probe(struct platform_device *pdev)
>
> INIT_LIST_HEAD(>channels);
>
> -   for (i = 0; i < JZ_DMA_NR_CHANNELS; i++) {
> +   for (i = 0; i < jzdma->nb_channels; i++) {
> jzchan = >chan[i];
> jzchan->id = i;
>
> @@ -884,19 +915,13 @@ static int jz4780_dma_remove(struct platform_device 
> *pdev)
>
> free_irq(jzdma->irq, jzdma);
>
> -   for (i = 0; i < JZ_DMA_NR_CHANNELS; i++)
> +   for (i = 0; i < jzdma->nb_channels; i++)
> tasklet_kill(>chan[i].vchan.task);
>
> dma_async_device_unregister(>dma_device);
> return 0;
>  }
>
> -static const struct of_device_id jz4780_dma_dt_match[] = {
> -   { .compatible = "ingenic,jz4780-dma", .data = NULL },
> -   {},
> -};
> -MODULE_DEVICE_TABLE(of, jz4780_dma_dt_match);
> 

Re: [PATCH 01/14] dmaengine: dma-jz4780: Avoid hardcoding number of channels

[PrasannaKumar Muralidharan]

Hi Paul,

On 3 July 2018 at 18:02, Paul Cercueil  wrote:
> As part of the work to support various other Ingenic JZ47xx SoC versions,
> which don't feature the same number of DMA channels per core, we now
> deduce the number of DMA channels available from the devicetree
> compatible string.
>
> Signed-off-by: Paul Cercueil 
> ---
>  drivers/dma/dma-jz4780.c | 53 +---
>  1 file changed, 39 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/dma/dma-jz4780.c b/drivers/dma/dma-jz4780.c
> index 85820a2d69d4..b40f491f0367 100644
> --- a/drivers/dma/dma-jz4780.c
> +++ b/drivers/dma/dma-jz4780.c
> @@ -16,6 +16,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -23,8 +24,6 @@
>  #include "dmaengine.h"
>  #include "virt-dma.h"
>
> -#define JZ_DMA_NR_CHANNELS 32
> -
>  /* Global registers. */
>  #define JZ_DMA_REG_DMAC0x1000
>  #define JZ_DMA_REG_DIRQP   0x1004
> @@ -135,14 +134,20 @@ struct jz4780_dma_chan {
> unsigned int curr_hwdesc;
>  };
>
> +enum jz_version {
> +   ID_JZ4780,
> +};
> +
>  struct jz4780_dma_dev {
> struct dma_device dma_device;
> void __iomem *base;
> struct clk *clk;
> unsigned int irq;
> +   unsigned int nb_channels;
> +   enum jz_version version;
>
> uint32_t chan_reserved;
> -   struct jz4780_dma_chan chan[JZ_DMA_NR_CHANNELS];
> +   struct jz4780_dma_chan chan[];

Looks like a variable length array in struct. I think there is some
effort to remove the usage of VLA. Can you revisit this? I may be
wrong, please feel free to correct.

>  };
>
>  struct jz4780_dma_filter_data {
> @@ -648,7 +653,7 @@ static irqreturn_t jz4780_dma_irq_handler(int irq, void 
> *data)
>
> pending = jz4780_dma_readl(jzdma, JZ_DMA_REG_DIRQP);
>
> -   for (i = 0; i < JZ_DMA_NR_CHANNELS; i++) {
> +   for (i = 0; i < jzdma->nb_channels; i++) {
> if (!(pending & (1< continue;
>
> @@ -728,7 +733,7 @@ static struct dma_chan *jz4780_of_dma_xlate(struct 
> of_phandle_args *dma_spec,
> data.channel = dma_spec->args[1];
>
> if (data.channel > -1) {
> -   if (data.channel >= JZ_DMA_NR_CHANNELS) {
> +   if (data.channel >= jzdma->nb_channels) {
> dev_err(jzdma->dma_device.dev,
> "device requested non-existent channel %u\n",
> data.channel);
> @@ -752,19 +757,45 @@ static struct dma_chan *jz4780_of_dma_xlate(struct 
> of_phandle_args *dma_spec,
> }
>  }
>
> +static const unsigned int jz4780_dma_nb_channels[] = {
> +   [ID_JZ4780] = 32,
> +};
> +
> +static const struct of_device_id jz4780_dma_dt_match[] = {
> +   { .compatible = "ingenic,jz4780-dma", .data = (void *)ID_JZ4780 },
> +   {},
> +};
> +MODULE_DEVICE_TABLE(of, jz4780_dma_dt_match);
> +
>  static int jz4780_dma_probe(struct platform_device *pdev)
>  {
> struct device *dev = >dev;
> +   const struct of_device_id *of_id = of_match_device(
> +   jz4780_dma_dt_match, dev);
> struct jz4780_dma_dev *jzdma;
> struct jz4780_dma_chan *jzchan;
> struct dma_device *dd;
> struct resource *res;
> +   enum jz_version version;
> +   unsigned int nb_channels;
> int i, ret;
>
> -   jzdma = devm_kzalloc(dev, sizeof(*jzdma), GFP_KERNEL);
> +   if (of_id)
> +   version = (enum jz_version)of_id->data;
> +   else
> +   version = ID_JZ4780; /* Default when not probed from DT */
> +
> +   nb_channels = jz4780_dma_nb_channels[version];
> +
> +   jzdma = devm_kzalloc(dev, sizeof(*jzdma)
> +   + sizeof(*jzdma->chan) * nb_channels,
> +   GFP_KERNEL);
> if (!jzdma)
> return -ENOMEM;
>
> +   jzdma->nb_channels = nb_channels;
> +   jzdma->version = version;
> +
> platform_set_drvdata(pdev, jzdma);
>
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> @@ -839,7 +870,7 @@ static int jz4780_dma_probe(struct platform_device *pdev)
>
> INIT_LIST_HEAD(>channels);
>
> -   for (i = 0; i < JZ_DMA_NR_CHANNELS; i++) {
> +   for (i = 0; i < jzdma->nb_channels; i++) {
> jzchan = >chan[i];
> jzchan->id = i;
>
> @@ -884,19 +915,13 @@ static int jz4780_dma_remove(struct platform_device 
> *pdev)
>
> free_irq(jzdma->irq, jzdma);
>
> -   for (i = 0; i < JZ_DMA_NR_CHANNELS; i++)
> +   for (i = 0; i < jzdma->nb_channels; i++)
> tasklet_kill(>chan[i].vchan.task);
>
> dma_async_device_unregister(>dma_device);
> return 0;
>  }
>
> -static const struct of_device_id jz4780_dma_dt_match[] = {
> -   { .compatible = "ingenic,jz4780-dma", .data = NULL },
> -   {},
> -};
> -MODULE_DEVICE_TABLE(of, jz4780_dma_dt_match);
> 

Re: [RFC 3/4] MIPS: Ingenic: Initial X1000 SoC support

[PrasannaKumar Muralidharan]

On 7 March 2018 at 21:22, Jiaxun Yang <jiaxun.y...@flygoat.com> wrote:
> 在 2018-03-07三的 20:35 +0530，PrasannaKumar Muralidharan写道：
>> Hi James,
>>
>> Seems Jiaxun is interested in the board and is willing to help.
>>
>> I have been told that Ingenic is focusing on IoT market and X1000 is
>> intended for IoT segment. I think that they would be selling several
>> 100Ks of chip over the coming years. But I feel Ingenic spends time
>> only on maintaining their Linux port which is usually based on very
>> old kernel version.
>
> Ingenic is going to release their XBrust2 core with it's products such
> as X2000 a few days later. Witch is a pure MIPS64r5 with MXU2(A
> superset of MIPS's MSA SIMD instruction set). The newest kernel port of
> X1000 maintain by Ingenic is based on Linux-4.4 [1]. After communicated
> with Ingenic, they said they are forcusing on China domestic market.
> But they're looking for partners to enter foriegn market.
>
> [1] https://pan.baidu.com/s/1o8MeYts (Well you can download from this
> Chinese website, ingenic have a gerrit but I don't have access to it.
> As my experience, it's hard to develop on Chinese-made chips wihout
> reading Chinese documents.)

Can you take a diff and upload it somewhere else? Download speed is a
couple of bytes per second. Can you put the diff in github or similar
other service?
I would like to see if the port uses DT.

Re: [RFC 3/4] MIPS: Ingenic: Initial X1000 SoC support

[PrasannaKumar Muralidharan]

On 7 March 2018 at 21:22, Jiaxun Yang  wrote:
> 在 2018-03-07三的 20:35 +0530，PrasannaKumar Muralidharan写道：
>> Hi James,
>>
>> Seems Jiaxun is interested in the board and is willing to help.
>>
>> I have been told that Ingenic is focusing on IoT market and X1000 is
>> intended for IoT segment. I think that they would be selling several
>> 100Ks of chip over the coming years. But I feel Ingenic spends time
>> only on maintaining their Linux port which is usually based on very
>> old kernel version.
>
> Ingenic is going to release their XBrust2 core with it's products such
> as X2000 a few days later. Witch is a pure MIPS64r5 with MXU2(A
> superset of MIPS's MSA SIMD instruction set). The newest kernel port of
> X1000 maintain by Ingenic is based on Linux-4.4 [1]. After communicated
> with Ingenic, they said they are forcusing on China domestic market.
> But they're looking for partners to enter foriegn market.
>
> [1] https://pan.baidu.com/s/1o8MeYts (Well you can download from this
> Chinese website, ingenic have a gerrit but I don't have access to it.
> As my experience, it's hard to develop on Chinese-made chips wihout
> reading Chinese documents.)

Can you take a diff and upload it somewhere else? Download speed is a
couple of bytes per second. Can you put the diff in github or similar
other service?
I would like to see if the port uses DT.

Re: [RFC 3/4] MIPS: Ingenic: Initial X1000 SoC support

[PrasannaKumar Muralidharan]

On 7 March 2018 at 20:40, James Hogan <jho...@kernel.org> wrote:
> On Wed, Mar 07, 2018 at 08:35:00PM +0530, PrasannaKumar Muralidharan wrote:
>> On 7 March 2018 at 20:05, James Hogan <jho...@kernel.org> wrote:
>> > On Wed, Mar 07, 2018 at 07:14:49PM +0530, PrasannaKumar Muralidharan wrote:
>> >> > Does X1000 use a different PRID, or is it basically just a JZ4780 core
>> >> > with different SoC peripherals?
>> >>
>> >> Yes X1000 does have a different PRID (PRID = 0x2ed1024f). X1000 has
>> >
>> > Right, so thats 0x2e00 | PRID_COMP_INGENIC_D1 | PRID_IMP_JZRISC |
>> > 0x4f, which cpu-probe.c already handles (apparently the D1 company code
>> > is used for JZ4770 & JZ4775 too).
>>
>> Okay. Does this mean I need not modify get_board_mach_type() and
>> get_system_type()?
>
> You still need to modify them, otherwise it won't understand
> "ingenic,x1000" compatible string, and will call it a JZ4740 in
> /proc/cpuinfo.

Oh okay. Will make the changes.

>> >> I used to get my code tested from Domink but I could not reach him for
>> >> quite some time. Before buying the development board myself I would
>> >> like to see if anyone can help me in testing. Do you have any contact
>> >> with Ingenic who can help in testing this?
>> >
>> > Not personally, but I'll ask around. Of course if nobody much cares
>> > about it in practice and nobody has the hardware, there may be little
>> > value in supporting it upstream.
>>
>> Seems Jiaxun is interested in the board and is willing to help.
>
> Okay, cool.
>
> Cheers
> James

Re: [RFC 3/4] MIPS: Ingenic: Initial X1000 SoC support

[PrasannaKumar Muralidharan]

On 7 March 2018 at 20:40, James Hogan  wrote:
> On Wed, Mar 07, 2018 at 08:35:00PM +0530, PrasannaKumar Muralidharan wrote:
>> On 7 March 2018 at 20:05, James Hogan  wrote:
>> > On Wed, Mar 07, 2018 at 07:14:49PM +0530, PrasannaKumar Muralidharan wrote:
>> >> > Does X1000 use a different PRID, or is it basically just a JZ4780 core
>> >> > with different SoC peripherals?
>> >>
>> >> Yes X1000 does have a different PRID (PRID = 0x2ed1024f). X1000 has
>> >
>> > Right, so thats 0x2e00 | PRID_COMP_INGENIC_D1 | PRID_IMP_JZRISC |
>> > 0x4f, which cpu-probe.c already handles (apparently the D1 company code
>> > is used for JZ4770 & JZ4775 too).
>>
>> Okay. Does this mean I need not modify get_board_mach_type() and
>> get_system_type()?
>
> You still need to modify them, otherwise it won't understand
> "ingenic,x1000" compatible string, and will call it a JZ4740 in
> /proc/cpuinfo.

Oh okay. Will make the changes.

>> >> I used to get my code tested from Domink but I could not reach him for
>> >> quite some time. Before buying the development board myself I would
>> >> like to see if anyone can help me in testing. Do you have any contact
>> >> with Ingenic who can help in testing this?
>> >
>> > Not personally, but I'll ask around. Of course if nobody much cares
>> > about it in practice and nobody has the hardware, there may be little
>> > value in supporting it upstream.
>>
>> Seems Jiaxun is interested in the board and is willing to help.
>
> Okay, cool.
>
> Cheers
> James

Re: [RFC 3/4] MIPS: Ingenic: Initial X1000 SoC support

[PrasannaKumar Muralidharan]

Hi Jiaxun,

On 7 March 2018 at 19:49, Jiaxun Yang <jiaxun.y...@flygoat.com> wrote:
> 在 2018-03-07三的 19:14 +0530，PrasannaKumar Muralidharan写道：
>>
>> I used to get my code tested from Domink but I could not reach him
>> for
>> quite some time. Before buying the development board myself I would
>> like to see if anyone can help me in testing. Do you have any contact
>> with Ingenic who can help in testing this?
>>
>
> Hi PrasannaKumar
>
> I'm resently working on Ingenic chips too. Ingentic guys have sent me a
> X1000 development broad and it will arrive in about two weeks. I have a
> ejtag debugger also (but not very suit with X1000 because X1000 have
> different ejtag interface with standard MIPS cores, maybe we need some
> modification on openocd). So maybe I can help in testing this after I
> get my broad. Just ask if you need any help.
> Thanks
> --
> Jiaxun Yang <jiaxun.y...@flygoat.com>

Really nice to hear. I will ask for help once I make changes.

Thanks,
PrasannaKumar

Re: [RFC 3/4] MIPS: Ingenic: Initial X1000 SoC support

[PrasannaKumar Muralidharan]

Hi Jiaxun,

On 7 March 2018 at 19:49, Jiaxun Yang  wrote:
> 在 2018-03-07三的 19:14 +0530，PrasannaKumar Muralidharan写道：
>>
>> I used to get my code tested from Domink but I could not reach him
>> for
>> quite some time. Before buying the development board myself I would
>> like to see if anyone can help me in testing. Do you have any contact
>> with Ingenic who can help in testing this?
>>
>
> Hi PrasannaKumar
>
> I'm resently working on Ingenic chips too. Ingentic guys have sent me a
> X1000 development broad and it will arrive in about two weeks. I have a
> ejtag debugger also (but not very suit with X1000 because X1000 have
> different ejtag interface with standard MIPS cores, maybe we need some
> modification on openocd). So maybe I can help in testing this after I
> get my broad. Just ask if you need any help.
> Thanks
> --
> Jiaxun Yang 

Really nice to hear. I will ask for help once I make changes.

Thanks,
PrasannaKumar

Re: [RFC 3/4] MIPS: Ingenic: Initial X1000 SoC support

[PrasannaKumar Muralidharan]

Hi James,

On 7 March 2018 at 20:05, James Hogan <jho...@kernel.org> wrote:
> On Wed, Mar 07, 2018 at 07:14:49PM +0530, PrasannaKumar Muralidharan wrote:
>> > Does X1000 use a different PRID, or is it basically just a JZ4780 core
>> > with different SoC peripherals?
>>
>> Yes X1000 does have a different PRID (PRID = 0x2ed1024f). X1000 has
>
> Right, so thats 0x2e00 | PRID_COMP_INGENIC_D1 | PRID_IMP_JZRISC |
> 0x4f, which cpu-probe.c already handles (apparently the D1 company code
> is used for JZ4770 & JZ4775 too).

Okay. Does this mean I need not modify get_board_mach_type() and
get_system_type()?

>> I used to get my code tested from Domink but I could not reach him for
>> quite some time. Before buying the development board myself I would
>> like to see if anyone can help me in testing. Do you have any contact
>> with Ingenic who can help in testing this?
>
> Not personally, but I'll ask around. Of course if nobody much cares
> about it in practice and nobody has the hardware, there may be little
> value in supporting it upstream.

Seems Jiaxun is interested in the board and is willing to help.

I have been told that Ingenic is focusing on IoT market and X1000 is
intended for IoT segment. I think that they would be selling several
100Ks of chip over the coming years. But I feel Ingenic spends time
only on maintaining their Linux port which is usually based on very
old kernel version.

> Cheers
> James

Thanks and regards,
PrasannaKumar

Re: [RFC 3/4] MIPS: Ingenic: Initial X1000 SoC support

[PrasannaKumar Muralidharan]

Hi James,

On 7 March 2018 at 20:05, James Hogan  wrote:
> On Wed, Mar 07, 2018 at 07:14:49PM +0530, PrasannaKumar Muralidharan wrote:
>> > Does X1000 use a different PRID, or is it basically just a JZ4780 core
>> > with different SoC peripherals?
>>
>> Yes X1000 does have a different PRID (PRID = 0x2ed1024f). X1000 has
>
> Right, so thats 0x2e00 | PRID_COMP_INGENIC_D1 | PRID_IMP_JZRISC |
> 0x4f, which cpu-probe.c already handles (apparently the D1 company code
> is used for JZ4770 & JZ4775 too).

Okay. Does this mean I need not modify get_board_mach_type() and
get_system_type()?

>> I used to get my code tested from Domink but I could not reach him for
>> quite some time. Before buying the development board myself I would
>> like to see if anyone can help me in testing. Do you have any contact
>> with Ingenic who can help in testing this?
>
> Not personally, but I'll ask around. Of course if nobody much cares
> about it in practice and nobody has the hardware, there may be little
> value in supporting it upstream.

Seems Jiaxun is interested in the board and is willing to help.

I have been told that Ingenic is focusing on IoT market and X1000 is
intended for IoT segment. I think that they would be selling several
100Ks of chip over the coming years. But I feel Ingenic spends time
only on maintaining their Linux port which is usually based on very
old kernel version.

> Cheers
> James

Thanks and regards,
PrasannaKumar

Re: [RFC 3/4] MIPS: Ingenic: Initial X1000 SoC support

[PrasannaKumar Muralidharan]

Hi James,

Thanks for reviewing this.

On 6 March 2018 at 05:38, James Hogan <jho...@kernel.org> wrote:
> On Wed, Sep 27, 2017 at 08:45:26PM +0530, PrasannaKumar Muralidharan wrote:
>> Add initial Ingenic X1000 SoC support. Provide minimum necessary
>> information to boot kernel to an initramfs userspace.
>>
>> Signed-off-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
>> ---
>>  arch/mips/boot/dts/ingenic/x1000.dtsi | 93 
>> +++
>>  arch/mips/jz4740/Kconfig  |  6 +++
>>  arch/mips/jz4740/time.c   |  2 +-
>>  3 files changed, 100 insertions(+), 1 deletion(-)
>>  create mode 100644 arch/mips/boot/dts/ingenic/x1000.dtsi
>
> arch/mips/jz4780/setup.c, specifically get_board_mach_type() and
> get_system_type() will need updating too.

Missed it. Will make necessary changes.

> Does X1000 use a different PRID, or is it basically just a JZ4780 core
> with different SoC peripherals?

Yes X1000 does have a different PRID (PRID = 0x2ed1024f). X1000 has
single CPU core so it is definitely not JZ4780.

>> diff --git a/arch/mips/boot/dts/ingenic/x1000.dtsi 
>> b/arch/mips/boot/dts/ingenic/x1000.dtsi
>> new file mode 100644
>> index 000..abbb9ec
>> --- /dev/null
>> +++ b/arch/mips/boot/dts/ingenic/x1000.dtsi
>> @@ -0,0 +1,93 @@
>> +/*
>> + * Copyright (C) 2016 PrasannaKumar Muralidharan 
>> <prasannatsmku...@gmail.com>
>> + *
>> + * 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.
>
> (these will need updating to use SPDX identifiers if you respin)

Sure. Will take care while submitting next version.

>> + cgu: jz4780-cgu@1000 {
>
> not sure jz4780 is appropriate here.

No, it is not. Copy pasted from jz4780.dtsi but missed updating this.

>> + compatible = "ingenic,x1000-cgu";
>> + reg = <0x1000 0x100>;
>> +
>> + clocks = <>, <>;
>> + clock-names = "ext", "rtc";
>> +
>> + #clock-cells = <1>;
>> + };
>
> Cheers
> James

I used to get my code tested from Domink but I could not reach him for
quite some time. Before buying the development board myself I would
like to see if anyone can help me in testing. Do you have any contact
with Ingenic who can help in testing this?

Thanks,
PrasannaKumar

Re: [RFC 3/4] MIPS: Ingenic: Initial X1000 SoC support

[PrasannaKumar Muralidharan]

Hi James,

Thanks for reviewing this.

On 6 March 2018 at 05:38, James Hogan  wrote:
> On Wed, Sep 27, 2017 at 08:45:26PM +0530, PrasannaKumar Muralidharan wrote:
>> Add initial Ingenic X1000 SoC support. Provide minimum necessary
>> information to boot kernel to an initramfs userspace.
>>
>> Signed-off-by: PrasannaKumar Muralidharan 
>> ---
>>  arch/mips/boot/dts/ingenic/x1000.dtsi | 93 
>> +++
>>  arch/mips/jz4740/Kconfig  |  6 +++
>>  arch/mips/jz4740/time.c   |  2 +-
>>  3 files changed, 100 insertions(+), 1 deletion(-)
>>  create mode 100644 arch/mips/boot/dts/ingenic/x1000.dtsi
>
> arch/mips/jz4780/setup.c, specifically get_board_mach_type() and
> get_system_type() will need updating too.

Missed it. Will make necessary changes.

> Does X1000 use a different PRID, or is it basically just a JZ4780 core
> with different SoC peripherals?

Yes X1000 does have a different PRID (PRID = 0x2ed1024f). X1000 has
single CPU core so it is definitely not JZ4780.

>> diff --git a/arch/mips/boot/dts/ingenic/x1000.dtsi 
>> b/arch/mips/boot/dts/ingenic/x1000.dtsi
>> new file mode 100644
>> index 000..abbb9ec
>> --- /dev/null
>> +++ b/arch/mips/boot/dts/ingenic/x1000.dtsi
>> @@ -0,0 +1,93 @@
>> +/*
>> + * Copyright (C) 2016 PrasannaKumar Muralidharan 
>> 
>> + *
>> + * 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.
>
> (these will need updating to use SPDX identifiers if you respin)

Sure. Will take care while submitting next version.

>> + cgu: jz4780-cgu@1000 {
>
> not sure jz4780 is appropriate here.

No, it is not. Copy pasted from jz4780.dtsi but missed updating this.

>> + compatible = "ingenic,x1000-cgu";
>> + reg = <0x1000 0x100>;
>> +
>> + clocks = <>, <>;
>> + clock-names = "ext", "rtc";
>> +
>> + #clock-cells = <1>;
>> + };
>
> Cheers
> James

I used to get my code tested from Domink but I could not reach him for
quite some time. Before buying the development board myself I would
like to see if anyone can help me in testing. Do you have any contact
with Ingenic who can help in testing this?

Thanks,
PrasannaKumar

Re: [RFC] NAND: Optimize NAND_ECC_HW_OOB_FIRST read

[PrasannaKumar Muralidharan]

Hi Boris,

On 30 October 2017 at 14:04, Boris Brezillon
<boris.brezil...@free-electrons.com> wrote:
> Hi PrasannaKumar,
>
> On Sat, 28 Oct 2017 13:13:51 +0530
> PrasannaKumar Muralidharan <prasannatsmku...@gmail.com> wrote:
>
>> From: Lars-Peter Clausen <l...@metafoo.de>
>>
>> Avoid sending unnecessary READ commands to the chip.
>>
>> Signed-off-by: Lars-Peter Clausen <l...@metafoo.de>
>> Signed-off-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
>> ---
>> This patch is taken from git://projects.qi-hardware.com/qi-kernel.git
>> branch jz-3.16. From [1] and [2] it can be seen that the patch author
>> thinks this can be sent upstream but it never happened so far. This
>> patch is used in OpenWRT as seen from [3].
>
> Sounds reasonable, but it's likely to conflict with something I'd like
> to queue for 4.16 [1]. I'll rebase this patch on nand/next once the
> ->exec_op() series is merged. Don't hesitate to ping me if I forget.
>
> Regards,
>
> Boris
>
> [1]http://patchwork.ozlabs.org/project/linux-mtd/list/?series=8923
>
>>
>> I have only compile tested the patch.
>>
>> 1. 
>> https://www.mail-archive.com/discussion@lists.en.qi-hardware.com/msg04635.html
>> 2. 
>> https://www.mail-archive.com/discussion@lists.en.qi-hardware.com/msg04639.html
>> 3. 
>> https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/xburst/patches-3.18/002-NAND-Optimize-NAND_ECC_HW_OOB_FIRST-read.patch;h=046da51912de3cd779df5de13d2c1999719a;hb=c03d4317a6bc891cb4a5e89cbdd77f37c23aff86
>>
>>  drivers/mtd/nand/nand_base.c | 16 
>>  1 file changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
>> index 12edaae..4bf3bdb 100644
>> --- a/drivers/mtd/nand/nand_base.c
>> +++ b/drivers/mtd/nand/nand_base.c
>> @@ -1680,9 +1680,15 @@ static int nand_read_page_hwecc_oob_first(struct 
>> mtd_info *mtd,
>>   unsigned int max_bitflips = 0;
>>
>>   /* Read the OOB area first */
>> - chip->cmdfunc(mtd, NAND_CMD_READOOB, 0, page);
>> - chip->read_buf(mtd, chip->oob_poi, mtd->oobsize);
>> - chip->cmdfunc(mtd, NAND_CMD_READ0, 0, page);
>> + if (mtd->writesize > 512) {
>> + chip->cmdfunc(mtd, NAND_CMD_READ0, mtd->writesize, page);
>> + chip->read_buf(mtd, chip->oob_poi, mtd->oobsize);
>> + chip->cmdfunc(mtd, NAND_CMD_RNDOUT, 0, -1);
>> + } else {
>> + chip->cmdfunc(mtd, NAND_CMD_READOOB, 0, page);
>> + chip->read_buf(mtd, chip->oob_poi, mtd->oobsize);
>> + chip->cmdfunc(mtd, NAND_CMD_READ0, 0, page);
>> + }
>>
>>   ret = mtd_ooblayout_get_eccbytes(mtd, ecc_code, chip->oob_poi, 0,
>>chip->ecc.total);
>> @@ -1902,8 +1908,10 @@ static int nand_do_read_ops(struct mtd_info *mtd, 
>> loff_t from,
>>__func__, buf);
>>
>>  read_retry:
>> - if (nand_standard_page_accessors(>ecc))
>> + if (nand_standard_page_accessors(>ecc) &&
>> + chip->ecc.mode != NAND_ECC_HW_OOB_FIRST) {
>>   chip->cmdfunc(mtd, NAND_CMD_READ0, 0x00, page);
>> + }
>>
>>   /*
>>* Now read the page into the buffer.  Absent an error,
>

Can you please revisit this?

Thanks,
PrasannaKumar

Re: [RFC] NAND: Optimize NAND_ECC_HW_OOB_FIRST read

[PrasannaKumar Muralidharan]

Hi Boris,

On 30 October 2017 at 14:04, Boris Brezillon
 wrote:
> Hi PrasannaKumar,
>
> On Sat, 28 Oct 2017 13:13:51 +0530
> PrasannaKumar Muralidharan  wrote:
>
>> From: Lars-Peter Clausen 
>>
>> Avoid sending unnecessary READ commands to the chip.
>>
>> Signed-off-by: Lars-Peter Clausen 
>> Signed-off-by: PrasannaKumar Muralidharan 
>> ---
>> This patch is taken from git://projects.qi-hardware.com/qi-kernel.git
>> branch jz-3.16. From [1] and [2] it can be seen that the patch author
>> thinks this can be sent upstream but it never happened so far. This
>> patch is used in OpenWRT as seen from [3].
>
> Sounds reasonable, but it's likely to conflict with something I'd like
> to queue for 4.16 [1]. I'll rebase this patch on nand/next once the
> ->exec_op() series is merged. Don't hesitate to ping me if I forget.
>
> Regards,
>
> Boris
>
> [1]http://patchwork.ozlabs.org/project/linux-mtd/list/?series=8923
>
>>
>> I have only compile tested the patch.
>>
>> 1. 
>> https://www.mail-archive.com/discussion@lists.en.qi-hardware.com/msg04635.html
>> 2. 
>> https://www.mail-archive.com/discussion@lists.en.qi-hardware.com/msg04639.html
>> 3. 
>> https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/xburst/patches-3.18/002-NAND-Optimize-NAND_ECC_HW_OOB_FIRST-read.patch;h=046da51912de3cd779df5de13d2c1999719a;hb=c03d4317a6bc891cb4a5e89cbdd77f37c23aff86
>>
>>  drivers/mtd/nand/nand_base.c | 16 
>>  1 file changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
>> index 12edaae..4bf3bdb 100644
>> --- a/drivers/mtd/nand/nand_base.c
>> +++ b/drivers/mtd/nand/nand_base.c
>> @@ -1680,9 +1680,15 @@ static int nand_read_page_hwecc_oob_first(struct 
>> mtd_info *mtd,
>>   unsigned int max_bitflips = 0;
>>
>>   /* Read the OOB area first */
>> - chip->cmdfunc(mtd, NAND_CMD_READOOB, 0, page);
>> - chip->read_buf(mtd, chip->oob_poi, mtd->oobsize);
>> - chip->cmdfunc(mtd, NAND_CMD_READ0, 0, page);
>> + if (mtd->writesize > 512) {
>> + chip->cmdfunc(mtd, NAND_CMD_READ0, mtd->writesize, page);
>> + chip->read_buf(mtd, chip->oob_poi, mtd->oobsize);
>> + chip->cmdfunc(mtd, NAND_CMD_RNDOUT, 0, -1);
>> + } else {
>> + chip->cmdfunc(mtd, NAND_CMD_READOOB, 0, page);
>> + chip->read_buf(mtd, chip->oob_poi, mtd->oobsize);
>> + chip->cmdfunc(mtd, NAND_CMD_READ0, 0, page);
>> + }
>>
>>   ret = mtd_ooblayout_get_eccbytes(mtd, ecc_code, chip->oob_poi, 0,
>>chip->ecc.total);
>> @@ -1902,8 +1908,10 @@ static int nand_do_read_ops(struct mtd_info *mtd, 
>> loff_t from,
>>__func__, buf);
>>
>>  read_retry:
>> - if (nand_standard_page_accessors(>ecc))
>> + if (nand_standard_page_accessors(>ecc) &&
>> + chip->ecc.mode != NAND_ECC_HW_OOB_FIRST) {
>>   chip->cmdfunc(mtd, NAND_CMD_READ0, 0x00, page);
>> + }
>>
>>   /*
>>* Now read the page into the buffer.  Absent an error,
>

Can you please revisit this?

Thanks,
PrasannaKumar

Re: [PATCH v2] tpm: Move Linux RNG connection to hwrng

[PrasannaKumar Muralidharan]

Hi Jarkko,

On 17 November 2017 at 19:27, Jarkko Sakkinen
 wrote:
> On Fri, Nov 17, 2017 at 03:28:53PM +0200, Jarkko Sakkinen wrote:
>
> At least signed-off-by from PrassanaKumar is missing from the 2nd
> commit. I'll add it.

I had the impression that my signed-off-by will be present in this
change. But it is missing in [1]. Is it supposed to be that way?

1. 
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=6e592a065d51d26f9d62b8b7501a5114076af8b4

Thanks,
PrasannaKumar

Re: [PATCH v2] tpm: Move Linux RNG connection to hwrng

[PrasannaKumar Muralidharan]

Hi Jarkko,

On 17 November 2017 at 19:27, Jarkko Sakkinen
 wrote:
> On Fri, Nov 17, 2017 at 03:28:53PM +0200, Jarkko Sakkinen wrote:
>
> At least signed-off-by from PrassanaKumar is missing from the 2nd
> commit. I'll add it.

I had the impression that my signed-off-by will be present in this
change. But it is missing in [1]. Is it supposed to be that way?

1. 
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=6e592a065d51d26f9d62b8b7501a5114076af8b4

Thanks,
PrasannaKumar

Re: [PATCH v2 3/8] watchdog: JZ4740: Register a restart handler

[PrasannaKumar Muralidharan]

Hi Guenter,

On 20 January 2018 at 21:15, Guenter Roeck <li...@roeck-us.net> wrote:
> On 01/19/2018 11:31 PM, PrasannaKumar Muralidharan wrote:
>>
>> Hi Paul,
>>
>> On 30 December 2017 at 19:21, Paul Cercueil <p...@crapouillou.net> wrote:
>>>
>>> The watchdog driver can restart the system by simply configuring the
>>> hardware for a timeout of 0 seconds.
>>>
>>> Signed-off-by: Paul Cercueil <p...@crapouillou.net>
>>> Reviewed-by: Guenter Roeck <li...@roeck-us.net>
>>> ---
>>>   drivers/watchdog/jz4740_wdt.c | 9 +
>>>   1 file changed, 9 insertions(+)
>>>
>>>   v2: No change
>>>
>>> diff --git a/drivers/watchdog/jz4740_wdt.c
>>> b/drivers/watchdog/jz4740_wdt.c
>>> index 92d6ca8ceb49..fa7f49a3212c 100644
>>> --- a/drivers/watchdog/jz4740_wdt.c
>>> +++ b/drivers/watchdog/jz4740_wdt.c
>>> @@ -130,6 +130,14 @@ static int jz4740_wdt_stop(struct watchdog_device
>>> *wdt_dev)
>>>  return 0;
>>>   }
>>>
>>> +static int jz4740_wdt_restart(struct watchdog_device *wdt_dev,
>>> + unsigned long action, void *data)
>>> +{
>>> +   wdt_dev->timeout = 0;
>>> +   jz4740_wdt_start(wdt_dev);
>>> +   return 0;
>>> +}
>>> +
>>>   static const struct watchdog_info jz4740_wdt_info = {
>>>  .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING |
>>> WDIOF_MAGICCLOSE,
>>>  .identity = "jz4740 Watchdog",
>>> @@ -141,6 +149,7 @@ static const struct watchdog_ops jz4740_wdt_ops = {
>>>  .stop = jz4740_wdt_stop,
>>>  .ping = jz4740_wdt_ping,
>>>  .set_timeout = jz4740_wdt_set_timeout,
>>> +   .restart = jz4740_wdt_restart,
>>>   };
>>>
>>>   #ifdef CONFIG_OF
>>> --
>>> 2.11.0
>>>
>>>
>>
>> Noticed that min_timeout of the watchdog device is set to 1 but this
>> function calls start with timeout set to 0. Even though this works I
>> feel it is better to set min_timeout to 0.
>>
>
> No. That would be wrong. If you want to be pedantic, write a new function
> __jz4740_wdt_set_timeout(u16 clock_div, u16 timeout_value) and call it
> instead, but don't mess with min_timeout.
>
> Guenter

What is the effect of changing min_timeout? I could see only
validation checks with it.

Thanks,
PrasannaKumar

Re: [PATCH v2 3/8] watchdog: JZ4740: Register a restart handler

[PrasannaKumar Muralidharan]

Hi Guenter,

On 20 January 2018 at 21:15, Guenter Roeck  wrote:
> On 01/19/2018 11:31 PM, PrasannaKumar Muralidharan wrote:
>>
>> Hi Paul,
>>
>> On 30 December 2017 at 19:21, Paul Cercueil  wrote:
>>>
>>> The watchdog driver can restart the system by simply configuring the
>>> hardware for a timeout of 0 seconds.
>>>
>>> Signed-off-by: Paul Cercueil 
>>> Reviewed-by: Guenter Roeck 
>>> ---
>>>   drivers/watchdog/jz4740_wdt.c | 9 +
>>>   1 file changed, 9 insertions(+)
>>>
>>>   v2: No change
>>>
>>> diff --git a/drivers/watchdog/jz4740_wdt.c
>>> b/drivers/watchdog/jz4740_wdt.c
>>> index 92d6ca8ceb49..fa7f49a3212c 100644
>>> --- a/drivers/watchdog/jz4740_wdt.c
>>> +++ b/drivers/watchdog/jz4740_wdt.c
>>> @@ -130,6 +130,14 @@ static int jz4740_wdt_stop(struct watchdog_device
>>> *wdt_dev)
>>>  return 0;
>>>   }
>>>
>>> +static int jz4740_wdt_restart(struct watchdog_device *wdt_dev,
>>> + unsigned long action, void *data)
>>> +{
>>> +   wdt_dev->timeout = 0;
>>> +   jz4740_wdt_start(wdt_dev);
>>> +   return 0;
>>> +}
>>> +
>>>   static const struct watchdog_info jz4740_wdt_info = {
>>>  .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING |
>>> WDIOF_MAGICCLOSE,
>>>  .identity = "jz4740 Watchdog",
>>> @@ -141,6 +149,7 @@ static const struct watchdog_ops jz4740_wdt_ops = {
>>>  .stop = jz4740_wdt_stop,
>>>  .ping = jz4740_wdt_ping,
>>>  .set_timeout = jz4740_wdt_set_timeout,
>>> +   .restart = jz4740_wdt_restart,
>>>   };
>>>
>>>   #ifdef CONFIG_OF
>>> --
>>> 2.11.0
>>>
>>>
>>
>> Noticed that min_timeout of the watchdog device is set to 1 but this
>> function calls start with timeout set to 0. Even though this works I
>> feel it is better to set min_timeout to 0.
>>
>
> No. That would be wrong. If you want to be pedantic, write a new function
> __jz4740_wdt_set_timeout(u16 clock_div, u16 timeout_value) and call it
> instead, but don't mess with min_timeout.
>
> Guenter

What is the effect of changing min_timeout? I could see only
validation checks with it.

Thanks,
PrasannaKumar

Re: [PATCH v2 4/8] watchdog: JZ4740: Drop module remove function

[PrasannaKumar Muralidharan]

Hi Guenter,

On 20 January 2018 at 21:20, Guenter Roeck <li...@roeck-us.net> wrote:
> On 01/19/2018 11:41 PM, PrasannaKumar Muralidharan wrote:
>>
>> Hi Paul,
>>
>> On 30 December 2017 at 19:21, Paul Cercueil <p...@crapouillou.net> wrote:
>>>
>>> When the watchdog was configured for nowayout, and after the
>>> userspace watchdog daemon closed the dev node without sending the
>>> magic character, unloading this module stopped the watchdog
>>> hardware, which was clearly a problem.
>>>
>>> Besides, unloading the module is not possible when the userspace
>>> watchdog daemon is running, so it's safe to assume that we don't
>>> need to stop the watchdog hardware in the jz4740_wdt_remove()
>>> function.
>>>
>>> For this reason, the jz4740_wdt_remove() function can then be
>>> dropped alltogether.
>>>
>>> Signed-off-by: Paul Cercueil <p...@crapouillou.net>
>>> ---
>>>   drivers/watchdog/jz4740_wdt.c | 8 
>>>   1 file changed, 8 deletions(-)
>>>
>>>   v2: New patch in this series
>>>
>>> diff --git a/drivers/watchdog/jz4740_wdt.c
>>> b/drivers/watchdog/jz4740_wdt.c
>>> index fa7f49a3212c..02b9b8e946a2 100644
>>> --- a/drivers/watchdog/jz4740_wdt.c
>>> +++ b/drivers/watchdog/jz4740_wdt.c
>>> @@ -205,16 +205,8 @@ static int jz4740_wdt_probe(struct platform_device
>>> *pdev)
>>>  return 0;
>>>   }
>>>
>>> -static int jz4740_wdt_remove(struct platform_device *pdev)
>>> -{
>>> -   struct jz4740_wdt_drvdata *drvdata = platform_get_drvdata(pdev);
>>> -
>>> -   return jz4740_wdt_stop(>wdt);
>>> -}
>>> -
>>>   static struct platform_driver jz4740_wdt_driver = {
>>>  .probe = jz4740_wdt_probe,
>>> -   .remove = jz4740_wdt_remove,
>>>  .driver = {
>>>  .name = "jz4740-wdt",
>>>  .of_match_table = of_match_ptr(jz4740_wdt_of_matches),
>>> --
>>> 2.11.0
>>>
>>>
>>
>> As ".remove" is removed and wdt is required for restarting the system
>> I am thinking that stop callback is also not required. If so does it
>> makes sense to remove the stop callback? I can submit a patch for the
>> same once this patch series goes in.
>>
> The remove function was removed because it would otherwise be an empty
> function. Since it is optional, it can and should be removed if it does not
> do anything. If the stop function is removed, it is no longer possible
> to stop the watchdog. Why would this make sense, and why would it make sense
> to drop the stop function if there is no remove function ?
>
> Guenter
>

I missed the fact that stopping is watchdog is possible. Sorry for the noise.

Thanks,
PrasannaKumar

Re: [PATCH v2 4/8] watchdog: JZ4740: Drop module remove function

[PrasannaKumar Muralidharan]

Hi Guenter,

On 20 January 2018 at 21:20, Guenter Roeck  wrote:
> On 01/19/2018 11:41 PM, PrasannaKumar Muralidharan wrote:
>>
>> Hi Paul,
>>
>> On 30 December 2017 at 19:21, Paul Cercueil  wrote:
>>>
>>> When the watchdog was configured for nowayout, and after the
>>> userspace watchdog daemon closed the dev node without sending the
>>> magic character, unloading this module stopped the watchdog
>>> hardware, which was clearly a problem.
>>>
>>> Besides, unloading the module is not possible when the userspace
>>> watchdog daemon is running, so it's safe to assume that we don't
>>> need to stop the watchdog hardware in the jz4740_wdt_remove()
>>> function.
>>>
>>> For this reason, the jz4740_wdt_remove() function can then be
>>> dropped alltogether.
>>>
>>> Signed-off-by: Paul Cercueil 
>>> ---
>>>   drivers/watchdog/jz4740_wdt.c | 8 
>>>   1 file changed, 8 deletions(-)
>>>
>>>   v2: New patch in this series
>>>
>>> diff --git a/drivers/watchdog/jz4740_wdt.c
>>> b/drivers/watchdog/jz4740_wdt.c
>>> index fa7f49a3212c..02b9b8e946a2 100644
>>> --- a/drivers/watchdog/jz4740_wdt.c
>>> +++ b/drivers/watchdog/jz4740_wdt.c
>>> @@ -205,16 +205,8 @@ static int jz4740_wdt_probe(struct platform_device
>>> *pdev)
>>>  return 0;
>>>   }
>>>
>>> -static int jz4740_wdt_remove(struct platform_device *pdev)
>>> -{
>>> -   struct jz4740_wdt_drvdata *drvdata = platform_get_drvdata(pdev);
>>> -
>>> -   return jz4740_wdt_stop(>wdt);
>>> -}
>>> -
>>>   static struct platform_driver jz4740_wdt_driver = {
>>>  .probe = jz4740_wdt_probe,
>>> -   .remove = jz4740_wdt_remove,
>>>  .driver = {
>>>  .name = "jz4740-wdt",
>>>  .of_match_table = of_match_ptr(jz4740_wdt_of_matches),
>>> --
>>> 2.11.0
>>>
>>>
>>
>> As ".remove" is removed and wdt is required for restarting the system
>> I am thinking that stop callback is also not required. If so does it
>> makes sense to remove the stop callback? I can submit a patch for the
>> same once this patch series goes in.
>>
> The remove function was removed because it would otherwise be an empty
> function. Since it is optional, it can and should be removed if it does not
> do anything. If the stop function is removed, it is no longer possible
> to stop the watchdog. Why would this make sense, and why would it make sense
> to drop the stop function if there is no remove function ?
>
> Guenter
>

I missed the fact that stopping is watchdog is possible. Sorry for the noise.

Thanks,
PrasannaKumar

Re: [PATCH v2 1/2] nvmem: add driver for JZ4780 efuse

[PrasannaKumar Muralidharan]

On 11 January 2018 at 20:38, Rob Herring <r...@kernel.org> wrote:
> On Sat, Jan 6, 2018 at 6:43 AM, PrasannaKumar Muralidharan
> <prasannatsmku...@gmail.com> wrote:
>> Hi Rob,
>>
>> On 4 January 2018 at 01:32, Rob Herring <r...@kernel.org> wrote:
>>> On Thu, Dec 28, 2017 at 10:29:52PM +0100, Mathieu Malaterre wrote:
>>>> From: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
>>>>
>>>> This patch brings support for the JZ4780 efuse. Currently it only expose
>>>> a read only access to the entire 8K bits efuse memory.
>>>>
>>>> Tested-by: Mathieu Malaterre <ma...@debian.org>
>>>> Signed-off-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
>>>> Signed-off-by: Mathieu Malaterre <ma...@debian.org>
>>>> ---
>>>>  .../ABI/testing/sysfs-driver-jz4780-efuse  |  16 ++
>>>>  .../bindings/nvmem/ingenic,jz4780-efuse.txt|  17 ++
>>>
>>> Please split bindings to separate patch.
>>>
>>>>  MAINTAINERS|   5 +
>>>>  arch/mips/boot/dts/ingenic/jz4780.dtsi |  40 ++-
>>>
>>> dts files should also be separate.
>>>
>>>>  drivers/nvmem/Kconfig  |  10 +
>>>>  drivers/nvmem/Makefile |   2 +
>>>>  drivers/nvmem/jz4780-efuse.c   | 305 
>>>> +
>>>>  7 files changed, 383 insertions(+), 12 deletions(-)
>>>>  create mode 100644 Documentation/ABI/testing/sysfs-driver-jz4780-efuse
>>>>  create mode 100644 
>>>> Documentation/devicetree/bindings/nvmem/ingenic,jz4780-efuse.txt
>>>>  create mode 100644 drivers/nvmem/jz4780-efuse.c
>>>>
>>>> diff --git a/Documentation/ABI/testing/sysfs-driver-jz4780-efuse 
>>>> b/Documentation/ABI/testing/sysfs-driver-jz4780-efuse
>>>> new file mode 100644
>>>> index ..bb6f5d6ceea0
>>>> --- /dev/null
>>>> +++ b/Documentation/ABI/testing/sysfs-driver-jz4780-efuse
>>>> @@ -0,0 +1,16 @@
>>>> +What:/sys/devices/*//nvmem
>>>> +Date:December 2017
>>>> +Contact: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
>>>> +Description: read-only access to the efuse on the Ingenic JZ4780 SoC
>>>> + The SoC has a one time programmable 8K efuse that is
>>>> + split into segments. The driver supports read only.
>>>> + The segments are
>>>> + 0x000   64 bit Random Number
>>>> + 0x008  128 bit Ingenic Chip ID
>>>> + 0x018  128 bit Customer ID
>>>> + 0x028 3520 bit Reserved
>>>> + 0x1E08 bit Protect Segment
>>>> + 0x1E1 2296 bit HDMI Key
>>>> + 0x300 2048 bit Security boot key
>>>
>>> Why do these need to be exposed to userspace?
>>>
>>> sysfs is 1 value per file and this is lots of different things.
>>>
>>> We already have ways to feed random data (entropy) to the system. And we
>>> have a way to expose SoC ID info to userspace (socdev).
>>
>> Currently ingenic chip id is not used anywhere. The vendor BSP exposed
>> only chip id and customer id. Should we do the same? Please provide
>> your suggestion.
>
> No. Don't create an ABI if you don't really need it.

Rob,
MAC address of the ethernet device is stored in customer id segment of
efuse. So only customer id is needed. Do you think exposing customer
id would suffice?

Srini,
Only user would be dm900 ethernet driver (need to make changes to it
once the efuse driver goes in). There is no need to expose it to user
space. I am planning to modify nvmem core to not expose efuse if the
efuse driver chooses so. Do you think it makes sense? The need to
maintain ABI for user space disappears if such a change is introduced.

Thanks,
PrasannaKumar

Re: [PATCH v2 1/2] nvmem: add driver for JZ4780 efuse

[PrasannaKumar Muralidharan]

On 11 January 2018 at 20:38, Rob Herring  wrote:
> On Sat, Jan 6, 2018 at 6:43 AM, PrasannaKumar Muralidharan
>  wrote:
>> Hi Rob,
>>
>> On 4 January 2018 at 01:32, Rob Herring  wrote:
>>> On Thu, Dec 28, 2017 at 10:29:52PM +0100, Mathieu Malaterre wrote:
>>>> From: PrasannaKumar Muralidharan 
>>>>
>>>> This patch brings support for the JZ4780 efuse. Currently it only expose
>>>> a read only access to the entire 8K bits efuse memory.
>>>>
>>>> Tested-by: Mathieu Malaterre 
>>>> Signed-off-by: PrasannaKumar Muralidharan 
>>>> Signed-off-by: Mathieu Malaterre 
>>>> ---
>>>>  .../ABI/testing/sysfs-driver-jz4780-efuse  |  16 ++
>>>>  .../bindings/nvmem/ingenic,jz4780-efuse.txt|  17 ++
>>>
>>> Please split bindings to separate patch.
>>>
>>>>  MAINTAINERS|   5 +
>>>>  arch/mips/boot/dts/ingenic/jz4780.dtsi |  40 ++-
>>>
>>> dts files should also be separate.
>>>
>>>>  drivers/nvmem/Kconfig  |  10 +
>>>>  drivers/nvmem/Makefile |   2 +
>>>>  drivers/nvmem/jz4780-efuse.c   | 305 
>>>> +
>>>>  7 files changed, 383 insertions(+), 12 deletions(-)
>>>>  create mode 100644 Documentation/ABI/testing/sysfs-driver-jz4780-efuse
>>>>  create mode 100644 
>>>> Documentation/devicetree/bindings/nvmem/ingenic,jz4780-efuse.txt
>>>>  create mode 100644 drivers/nvmem/jz4780-efuse.c
>>>>
>>>> diff --git a/Documentation/ABI/testing/sysfs-driver-jz4780-efuse 
>>>> b/Documentation/ABI/testing/sysfs-driver-jz4780-efuse
>>>> new file mode 100644
>>>> index ..bb6f5d6ceea0
>>>> --- /dev/null
>>>> +++ b/Documentation/ABI/testing/sysfs-driver-jz4780-efuse
>>>> @@ -0,0 +1,16 @@
>>>> +What:/sys/devices/*//nvmem
>>>> +Date:December 2017
>>>> +Contact: PrasannaKumar Muralidharan 
>>>> +Description: read-only access to the efuse on the Ingenic JZ4780 SoC
>>>> + The SoC has a one time programmable 8K efuse that is
>>>> + split into segments. The driver supports read only.
>>>> + The segments are
>>>> + 0x000   64 bit Random Number
>>>> + 0x008  128 bit Ingenic Chip ID
>>>> + 0x018  128 bit Customer ID
>>>> + 0x028 3520 bit Reserved
>>>> + 0x1E08 bit Protect Segment
>>>> + 0x1E1 2296 bit HDMI Key
>>>> + 0x300 2048 bit Security boot key
>>>
>>> Why do these need to be exposed to userspace?
>>>
>>> sysfs is 1 value per file and this is lots of different things.
>>>
>>> We already have ways to feed random data (entropy) to the system. And we
>>> have a way to expose SoC ID info to userspace (socdev).
>>
>> Currently ingenic chip id is not used anywhere. The vendor BSP exposed
>> only chip id and customer id. Should we do the same? Please provide
>> your suggestion.
>
> No. Don't create an ABI if you don't really need it.

Rob,
MAC address of the ethernet device is stored in customer id segment of
efuse. So only customer id is needed. Do you think exposing customer
id would suffice?

Srini,
Only user would be dm900 ethernet driver (need to make changes to it
once the efuse driver goes in). There is no need to expose it to user
space. I am planning to modify nvmem core to not expose efuse if the
efuse driver chooses so. Do you think it makes sense? The need to
maintain ABI for user space disappears if such a change is introduced.

Thanks,
PrasannaKumar

Re: [PATCH v3] tpm: use struct tpm_chip for tpm_chip_find_get()

[PrasannaKumar Muralidharan]

Hi Jarkko,

On 18 January 2018 at 21:50, Jarkko Sakkinen
<jarkko.sakki...@linux.intel.com> wrote:
> On Wed, Jan 17, 2018 at 07:43:51PM +0530, PrasannaKumar Muralidharan wrote:
>> Hi Jarkko,
>>
>> On 14 November 2017 at 20:02, Jarkko Sakkinen
>> <jarkko.sakki...@linux.intel.com> wrote:
>> > On Sun, Nov 12, 2017 at 10:53:35AM +0530, PrasannaKumar Muralidharan wrote:
>> >> Did basic check on tpm rng patch, it works fine. As it depends on this
>> >> patch this should be working fine too.
>> >>
>> >> Tested-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
>> >>
>> >> Regards,
>> >> PrasannaKumar
>> >
>> > Thank you.
>> >
>> > /Jarkko
>>
>> Wondering what happened to this and tpm rng patch. Is there something
>> more to do for this to get merged?
>>
>> Thanks,
>> PrasannaKumar
>
> Was part of 4.6 PR.
>
> /Jarkko

Ah, I see.

PS: I think you meant 4.16.

Thanks,
PrasannaKumar

Re: [PATCH v3] tpm: use struct tpm_chip for tpm_chip_find_get()

[PrasannaKumar Muralidharan]

Hi Jarkko,

On 18 January 2018 at 21:50, Jarkko Sakkinen
 wrote:
> On Wed, Jan 17, 2018 at 07:43:51PM +0530, PrasannaKumar Muralidharan wrote:
>> Hi Jarkko,
>>
>> On 14 November 2017 at 20:02, Jarkko Sakkinen
>>  wrote:
>> > On Sun, Nov 12, 2017 at 10:53:35AM +0530, PrasannaKumar Muralidharan wrote:
>> >> Did basic check on tpm rng patch, it works fine. As it depends on this
>> >> patch this should be working fine too.
>> >>
>> >> Tested-by: PrasannaKumar Muralidharan 
>> >>
>> >> Regards,
>> >> PrasannaKumar
>> >
>> > Thank you.
>> >
>> > /Jarkko
>>
>> Wondering what happened to this and tpm rng patch. Is there something
>> more to do for this to get merged?
>>
>> Thanks,
>> PrasannaKumar
>
> Was part of 4.6 PR.
>
> /Jarkko

Ah, I see.

PS: I think you meant 4.16.

Thanks,
PrasannaKumar

Re: [PATCH v2 4/8] watchdog: JZ4740: Drop module remove function

[PrasannaKumar Muralidharan]

Hi Paul,

On 30 December 2017 at 19:21, Paul Cercueil  wrote:
> When the watchdog was configured for nowayout, and after the
> userspace watchdog daemon closed the dev node without sending the
> magic character, unloading this module stopped the watchdog
> hardware, which was clearly a problem.
>
> Besides, unloading the module is not possible when the userspace
> watchdog daemon is running, so it's safe to assume that we don't
> need to stop the watchdog hardware in the jz4740_wdt_remove()
> function.
>
> For this reason, the jz4740_wdt_remove() function can then be
> dropped alltogether.
>
> Signed-off-by: Paul Cercueil 
> ---
>  drivers/watchdog/jz4740_wdt.c | 8 
>  1 file changed, 8 deletions(-)
>
>  v2: New patch in this series
>
> diff --git a/drivers/watchdog/jz4740_wdt.c b/drivers/watchdog/jz4740_wdt.c
> index fa7f49a3212c..02b9b8e946a2 100644
> --- a/drivers/watchdog/jz4740_wdt.c
> +++ b/drivers/watchdog/jz4740_wdt.c
> @@ -205,16 +205,8 @@ static int jz4740_wdt_probe(struct platform_device *pdev)
> return 0;
>  }
>
> -static int jz4740_wdt_remove(struct platform_device *pdev)
> -{
> -   struct jz4740_wdt_drvdata *drvdata = platform_get_drvdata(pdev);
> -
> -   return jz4740_wdt_stop(>wdt);
> -}
> -
>  static struct platform_driver jz4740_wdt_driver = {
> .probe = jz4740_wdt_probe,
> -   .remove = jz4740_wdt_remove,
> .driver = {
> .name = "jz4740-wdt",
> .of_match_table = of_match_ptr(jz4740_wdt_of_matches),
> --
> 2.11.0
>
>

As ".remove" is removed and wdt is required for restarting the system
I am thinking that stop callback is also not required. If so does it
makes sense to remove the stop callback? I can submit a patch for the
same once this patch series goes in.

Regards,
PrasannaKumar

Re: [PATCH v2 4/8] watchdog: JZ4740: Drop module remove function

[PrasannaKumar Muralidharan]

Hi Paul,

On 30 December 2017 at 19:21, Paul Cercueil  wrote:
> When the watchdog was configured for nowayout, and after the
> userspace watchdog daemon closed the dev node without sending the
> magic character, unloading this module stopped the watchdog
> hardware, which was clearly a problem.
>
> Besides, unloading the module is not possible when the userspace
> watchdog daemon is running, so it's safe to assume that we don't
> need to stop the watchdog hardware in the jz4740_wdt_remove()
> function.
>
> For this reason, the jz4740_wdt_remove() function can then be
> dropped alltogether.
>
> Signed-off-by: Paul Cercueil 
> ---
>  drivers/watchdog/jz4740_wdt.c | 8 
>  1 file changed, 8 deletions(-)
>
>  v2: New patch in this series
>
> diff --git a/drivers/watchdog/jz4740_wdt.c b/drivers/watchdog/jz4740_wdt.c
> index fa7f49a3212c..02b9b8e946a2 100644
> --- a/drivers/watchdog/jz4740_wdt.c
> +++ b/drivers/watchdog/jz4740_wdt.c
> @@ -205,16 +205,8 @@ static int jz4740_wdt_probe(struct platform_device *pdev)
> return 0;
>  }
>
> -static int jz4740_wdt_remove(struct platform_device *pdev)
> -{
> -   struct jz4740_wdt_drvdata *drvdata = platform_get_drvdata(pdev);
> -
> -   return jz4740_wdt_stop(>wdt);
> -}
> -
>  static struct platform_driver jz4740_wdt_driver = {
> .probe = jz4740_wdt_probe,
> -   .remove = jz4740_wdt_remove,
> .driver = {
> .name = "jz4740-wdt",
> .of_match_table = of_match_ptr(jz4740_wdt_of_matches),
> --
> 2.11.0
>
>

As ".remove" is removed and wdt is required for restarting the system
I am thinking that stop callback is also not required. If so does it
makes sense to remove the stop callback? I can submit a patch for the
same once this patch series goes in.

Regards,
PrasannaKumar

Re: [PATCH v2 3/8] watchdog: JZ4740: Register a restart handler

[PrasannaKumar Muralidharan]

Hi Paul,

On 30 December 2017 at 19:21, Paul Cercueil  wrote:
> The watchdog driver can restart the system by simply configuring the
> hardware for a timeout of 0 seconds.
>
> Signed-off-by: Paul Cercueil 
> Reviewed-by: Guenter Roeck 
> ---
>  drivers/watchdog/jz4740_wdt.c | 9 +
>  1 file changed, 9 insertions(+)
>
>  v2: No change
>
> diff --git a/drivers/watchdog/jz4740_wdt.c b/drivers/watchdog/jz4740_wdt.c
> index 92d6ca8ceb49..fa7f49a3212c 100644
> --- a/drivers/watchdog/jz4740_wdt.c
> +++ b/drivers/watchdog/jz4740_wdt.c
> @@ -130,6 +130,14 @@ static int jz4740_wdt_stop(struct watchdog_device 
> *wdt_dev)
> return 0;
>  }
>
> +static int jz4740_wdt_restart(struct watchdog_device *wdt_dev,
> + unsigned long action, void *data)
> +{
> +   wdt_dev->timeout = 0;
> +   jz4740_wdt_start(wdt_dev);
> +   return 0;
> +}
> +
>  static const struct watchdog_info jz4740_wdt_info = {
> .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
> .identity = "jz4740 Watchdog",
> @@ -141,6 +149,7 @@ static const struct watchdog_ops jz4740_wdt_ops = {
> .stop = jz4740_wdt_stop,
> .ping = jz4740_wdt_ping,
> .set_timeout = jz4740_wdt_set_timeout,
> +   .restart = jz4740_wdt_restart,
>  };
>
>  #ifdef CONFIG_OF
> --
> 2.11.0
>
>

Noticed that min_timeout of the watchdog device is set to 1 but this
function calls start with timeout set to 0. Even though this works I
feel it is better to set min_timeout to 0.

Regards,
PrasannaKumar

Re: [PATCH v2 3/8] watchdog: JZ4740: Register a restart handler

[PrasannaKumar Muralidharan]

Hi Paul,

On 30 December 2017 at 19:21, Paul Cercueil  wrote:
> The watchdog driver can restart the system by simply configuring the
> hardware for a timeout of 0 seconds.
>
> Signed-off-by: Paul Cercueil 
> Reviewed-by: Guenter Roeck 
> ---
>  drivers/watchdog/jz4740_wdt.c | 9 +
>  1 file changed, 9 insertions(+)
>
>  v2: No change
>
> diff --git a/drivers/watchdog/jz4740_wdt.c b/drivers/watchdog/jz4740_wdt.c
> index 92d6ca8ceb49..fa7f49a3212c 100644
> --- a/drivers/watchdog/jz4740_wdt.c
> +++ b/drivers/watchdog/jz4740_wdt.c
> @@ -130,6 +130,14 @@ static int jz4740_wdt_stop(struct watchdog_device 
> *wdt_dev)
> return 0;
>  }
>
> +static int jz4740_wdt_restart(struct watchdog_device *wdt_dev,
> + unsigned long action, void *data)
> +{
> +   wdt_dev->timeout = 0;
> +   jz4740_wdt_start(wdt_dev);
> +   return 0;
> +}
> +
>  static const struct watchdog_info jz4740_wdt_info = {
> .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
> .identity = "jz4740 Watchdog",
> @@ -141,6 +149,7 @@ static const struct watchdog_ops jz4740_wdt_ops = {
> .stop = jz4740_wdt_stop,
> .ping = jz4740_wdt_ping,
> .set_timeout = jz4740_wdt_set_timeout,
> +   .restart = jz4740_wdt_restart,
>  };
>
>  #ifdef CONFIG_OF
> --
> 2.11.0
>
>

Noticed that min_timeout of the watchdog device is set to 1 but this
function calls start with timeout set to 0. Even though this works I
feel it is better to set min_timeout to 0.

Regards,
PrasannaKumar

Re: [PATCH v2 1/2] nvmem: add driver for JZ4780 efuse

[PrasannaKumar Muralidharan]

Hi Rob,

On 11 January 2018 at 20:38, Rob Herring <r...@kernel.org> wrote:
> On Sat, Jan 6, 2018 at 6:43 AM, PrasannaKumar Muralidharan
> <prasannatsmku...@gmail.com> wrote:
>> Hi Rob,
>>
>> On 4 January 2018 at 01:32, Rob Herring <r...@kernel.org> wrote:
>>> On Thu, Dec 28, 2017 at 10:29:52PM +0100, Mathieu Malaterre wrote:
>>>> From: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
>>>>
>>>> This patch brings support for the JZ4780 efuse. Currently it only expose
>>>> a read only access to the entire 8K bits efuse memory.
>>>>
>>>> Tested-by: Mathieu Malaterre <ma...@debian.org>
>>>> Signed-off-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
>>>> Signed-off-by: Mathieu Malaterre <ma...@debian.org>
>>>> ---
>>>>  .../ABI/testing/sysfs-driver-jz4780-efuse  |  16 ++
>>>>  .../bindings/nvmem/ingenic,jz4780-efuse.txt|  17 ++
>>>
>>> Please split bindings to separate patch.
>>>
>>>>  MAINTAINERS|   5 +
>>>>  arch/mips/boot/dts/ingenic/jz4780.dtsi |  40 ++-
>>>
>>> dts files should also be separate.
>>>
>>>>  drivers/nvmem/Kconfig  |  10 +
>>>>  drivers/nvmem/Makefile |   2 +
>>>>  drivers/nvmem/jz4780-efuse.c   | 305 
>>>> +
>>>>  7 files changed, 383 insertions(+), 12 deletions(-)
>>>>  create mode 100644 Documentation/ABI/testing/sysfs-driver-jz4780-efuse
>>>>  create mode 100644 
>>>> Documentation/devicetree/bindings/nvmem/ingenic,jz4780-efuse.txt
>>>>  create mode 100644 drivers/nvmem/jz4780-efuse.c
>>>>
>>>> diff --git a/Documentation/ABI/testing/sysfs-driver-jz4780-efuse 
>>>> b/Documentation/ABI/testing/sysfs-driver-jz4780-efuse
>>>> new file mode 100644
>>>> index ..bb6f5d6ceea0
>>>> --- /dev/null
>>>> +++ b/Documentation/ABI/testing/sysfs-driver-jz4780-efuse
>>>> @@ -0,0 +1,16 @@
>>>> +What:/sys/devices/*//nvmem
>>>> +Date:December 2017
>>>> +Contact: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
>>>> +Description: read-only access to the efuse on the Ingenic JZ4780 SoC
>>>> + The SoC has a one time programmable 8K efuse that is
>>>> + split into segments. The driver supports read only.
>>>> + The segments are
>>>> + 0x000   64 bit Random Number
>>>> + 0x008  128 bit Ingenic Chip ID
>>>> + 0x018  128 bit Customer ID
>>>> + 0x028 3520 bit Reserved
>>>> + 0x1E08 bit Protect Segment
>>>> + 0x1E1 2296 bit HDMI Key
>>>> + 0x300 2048 bit Security boot key
>>>
>>> Why do these need to be exposed to userspace?
>>>
>>> sysfs is 1 value per file and this is lots of different things.
>>>
>>> We already have ways to feed random data (entropy) to the system. And we
>>> have a way to expose SoC ID info to userspace (socdev).
>>
>> Currently ingenic chip id is not used anywhere. The vendor BSP exposed
>> only chip id and customer id. Should we do the same? Please provide
>> your suggestion.
>
> No. Don't create an ABI if you don't really need it.
>
>>
>>>> +Users:   any user space application which wants to read the 
>>>> Chip
>>>> + and Customer ID
>>>> diff --git 
>>>> a/Documentation/devicetree/bindings/nvmem/ingenic,jz4780-efuse.txt 
>>>> b/Documentation/devicetree/bindings/nvmem/ingenic,jz4780-efuse.txt
>>>> new file mode 100644
>>>> index ..cd6d67ec22fc
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/nvmem/ingenic,jz4780-efuse.txt
>>>> @@ -0,0 +1,17 @@
>>>> +Ingenic JZ EFUSE driver bindings
>>>> +
>>>> +Required properties:
>>>> +- "compatible"   Must be set to "ingenic,jz4780-efuse"
>>>> +- "reg"  Register location and length
>>>> +- "clocks"   Handle for the ahb clock for the efuse.
>>>> +- "clock-names"  Must be "bus_clk

Re: [PATCH v2 1/2] nvmem: add driver for JZ4780 efuse

[PrasannaKumar Muralidharan]

Hi Rob,

On 11 January 2018 at 20:38, Rob Herring  wrote:
> On Sat, Jan 6, 2018 at 6:43 AM, PrasannaKumar Muralidharan
>  wrote:
>> Hi Rob,
>>
>> On 4 January 2018 at 01:32, Rob Herring  wrote:
>>> On Thu, Dec 28, 2017 at 10:29:52PM +0100, Mathieu Malaterre wrote:
>>>> From: PrasannaKumar Muralidharan 
>>>>
>>>> This patch brings support for the JZ4780 efuse. Currently it only expose
>>>> a read only access to the entire 8K bits efuse memory.
>>>>
>>>> Tested-by: Mathieu Malaterre 
>>>> Signed-off-by: PrasannaKumar Muralidharan 
>>>> Signed-off-by: Mathieu Malaterre 
>>>> ---
>>>>  .../ABI/testing/sysfs-driver-jz4780-efuse  |  16 ++
>>>>  .../bindings/nvmem/ingenic,jz4780-efuse.txt|  17 ++
>>>
>>> Please split bindings to separate patch.
>>>
>>>>  MAINTAINERS|   5 +
>>>>  arch/mips/boot/dts/ingenic/jz4780.dtsi |  40 ++-
>>>
>>> dts files should also be separate.
>>>
>>>>  drivers/nvmem/Kconfig  |  10 +
>>>>  drivers/nvmem/Makefile |   2 +
>>>>  drivers/nvmem/jz4780-efuse.c   | 305 
>>>> +
>>>>  7 files changed, 383 insertions(+), 12 deletions(-)
>>>>  create mode 100644 Documentation/ABI/testing/sysfs-driver-jz4780-efuse
>>>>  create mode 100644 
>>>> Documentation/devicetree/bindings/nvmem/ingenic,jz4780-efuse.txt
>>>>  create mode 100644 drivers/nvmem/jz4780-efuse.c
>>>>
>>>> diff --git a/Documentation/ABI/testing/sysfs-driver-jz4780-efuse 
>>>> b/Documentation/ABI/testing/sysfs-driver-jz4780-efuse
>>>> new file mode 100644
>>>> index ..bb6f5d6ceea0
>>>> --- /dev/null
>>>> +++ b/Documentation/ABI/testing/sysfs-driver-jz4780-efuse
>>>> @@ -0,0 +1,16 @@
>>>> +What:/sys/devices/*//nvmem
>>>> +Date:December 2017
>>>> +Contact: PrasannaKumar Muralidharan 
>>>> +Description: read-only access to the efuse on the Ingenic JZ4780 SoC
>>>> + The SoC has a one time programmable 8K efuse that is
>>>> + split into segments. The driver supports read only.
>>>> + The segments are
>>>> + 0x000   64 bit Random Number
>>>> + 0x008  128 bit Ingenic Chip ID
>>>> + 0x018  128 bit Customer ID
>>>> + 0x028 3520 bit Reserved
>>>> + 0x1E08 bit Protect Segment
>>>> + 0x1E1 2296 bit HDMI Key
>>>> + 0x300 2048 bit Security boot key
>>>
>>> Why do these need to be exposed to userspace?
>>>
>>> sysfs is 1 value per file and this is lots of different things.
>>>
>>> We already have ways to feed random data (entropy) to the system. And we
>>> have a way to expose SoC ID info to userspace (socdev).
>>
>> Currently ingenic chip id is not used anywhere. The vendor BSP exposed
>> only chip id and customer id. Should we do the same? Please provide
>> your suggestion.
>
> No. Don't create an ABI if you don't really need it.
>
>>
>>>> +Users:   any user space application which wants to read the 
>>>> Chip
>>>> + and Customer ID
>>>> diff --git 
>>>> a/Documentation/devicetree/bindings/nvmem/ingenic,jz4780-efuse.txt 
>>>> b/Documentation/devicetree/bindings/nvmem/ingenic,jz4780-efuse.txt
>>>> new file mode 100644
>>>> index ..cd6d67ec22fc
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/nvmem/ingenic,jz4780-efuse.txt
>>>> @@ -0,0 +1,17 @@
>>>> +Ingenic JZ EFUSE driver bindings
>>>> +
>>>> +Required properties:
>>>> +- "compatible"   Must be set to "ingenic,jz4780-efuse"
>>>> +- "reg"  Register location and length
>>>> +- "clocks"   Handle for the ahb clock for the efuse.
>>>> +- "clock-names"  Must be "bus_clk"
>>>> +
>>>> +Example:
>>>> +
>>>> +efuse: efuse@134100d0 {
>>>> + compatible = "ingenic,jz4780-efuse";
>>>> + reg = <0x134100D0 0xFF>;

Re: [PATCH v3] tpm: use struct tpm_chip for tpm_chip_find_get()

[PrasannaKumar Muralidharan]

Hi Jarkko,

On 14 November 2017 at 20:02, Jarkko Sakkinen
<jarkko.sakki...@linux.intel.com> wrote:
> On Sun, Nov 12, 2017 at 10:53:35AM +0530, PrasannaKumar Muralidharan wrote:
>> Did basic check on tpm rng patch, it works fine. As it depends on this
>> patch this should be working fine too.
>>
>> Tested-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
>>
>> Regards,
>> PrasannaKumar
>
> Thank you.
>
> /Jarkko

Wondering what happened to this and tpm rng patch. Is there something
more to do for this to get merged?

Thanks,
PrasannaKumar

Re: [PATCH v3] tpm: use struct tpm_chip for tpm_chip_find_get()

[PrasannaKumar Muralidharan]

Hi Jarkko,

On 14 November 2017 at 20:02, Jarkko Sakkinen
 wrote:
> On Sun, Nov 12, 2017 at 10:53:35AM +0530, PrasannaKumar Muralidharan wrote:
>> Did basic check on tpm rng patch, it works fine. As it depends on this
>> patch this should be working fine too.
>>
>> Tested-by: PrasannaKumar Muralidharan 
>>
>> Regards,
>> PrasannaKumar
>
> Thank you.
>
> /Jarkko

Wondering what happened to this and tpm rng patch. Is there something
more to do for this to get merged?

Thanks,
PrasannaKumar

Re: [PATCH v2 1/2] nvmem: add driver for JZ4780 efuse

[PrasannaKumar Muralidharan]

Hi Rob,

On 4 January 2018 at 01:32, Rob Herring <r...@kernel.org> wrote:
> On Thu, Dec 28, 2017 at 10:29:52PM +0100, Mathieu Malaterre wrote:
>> From: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
>>
>> This patch brings support for the JZ4780 efuse. Currently it only expose
>> a read only access to the entire 8K bits efuse memory.
>>
>> Tested-by: Mathieu Malaterre <ma...@debian.org>
>> Signed-off-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
>> Signed-off-by: Mathieu Malaterre <ma...@debian.org>
>> ---
>>  .../ABI/testing/sysfs-driver-jz4780-efuse  |  16 ++
>>  .../bindings/nvmem/ingenic,jz4780-efuse.txt|  17 ++
>
> Please split bindings to separate patch.
>
>>  MAINTAINERS|   5 +
>>  arch/mips/boot/dts/ingenic/jz4780.dtsi |  40 ++-
>
> dts files should also be separate.
>
>>  drivers/nvmem/Kconfig  |  10 +
>>  drivers/nvmem/Makefile |   2 +
>>  drivers/nvmem/jz4780-efuse.c   | 305 
>> +
>>  7 files changed, 383 insertions(+), 12 deletions(-)
>>  create mode 100644 Documentation/ABI/testing/sysfs-driver-jz4780-efuse
>>  create mode 100644 
>> Documentation/devicetree/bindings/nvmem/ingenic,jz4780-efuse.txt
>>  create mode 100644 drivers/nvmem/jz4780-efuse.c
>>
>> diff --git a/Documentation/ABI/testing/sysfs-driver-jz4780-efuse 
>> b/Documentation/ABI/testing/sysfs-driver-jz4780-efuse
>> new file mode 100644
>> index ..bb6f5d6ceea0
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-driver-jz4780-efuse
>> @@ -0,0 +1,16 @@
>> +What:/sys/devices/*//nvmem
>> +Date:December 2017
>> +Contact: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
>> +Description: read-only access to the efuse on the Ingenic JZ4780 SoC
>> + The SoC has a one time programmable 8K efuse that is
>> + split into segments. The driver supports read only.
>> + The segments are
>> + 0x000   64 bit Random Number
>> + 0x008  128 bit Ingenic Chip ID
>> + 0x018  128 bit Customer ID
>> + 0x028 3520 bit Reserved
>> + 0x1E08 bit Protect Segment
>> + 0x1E1 2296 bit HDMI Key
>> + 0x300 2048 bit Security boot key
>
> Why do these need to be exposed to userspace?
>
> sysfs is 1 value per file and this is lots of different things.
>
> We already have ways to feed random data (entropy) to the system. And we
> have a way to expose SoC ID info to userspace (socdev).

Currently ingenic chip id is not used anywhere. The vendor BSP exposed
only chip id and customer id. Should we do the same? Please provide
your suggestion.

>> +Users:   any user space application which wants to read the Chip
>> + and Customer ID
>> diff --git 
>> a/Documentation/devicetree/bindings/nvmem/ingenic,jz4780-efuse.txt 
>> b/Documentation/devicetree/bindings/nvmem/ingenic,jz4780-efuse.txt
>> new file mode 100644
>> index ..cd6d67ec22fc
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/nvmem/ingenic,jz4780-efuse.txt
>> @@ -0,0 +1,17 @@
>> +Ingenic JZ EFUSE driver bindings
>> +
>> +Required properties:
>> +- "compatible"   Must be set to "ingenic,jz4780-efuse"
>> +- "reg"  Register location and length
>> +- "clocks"   Handle for the ahb clock for the efuse.
>> +- "clock-names"  Must be "bus_clk"
>> +
>> +Example:
>> +
>> +efuse: efuse@134100d0 {
>> + compatible = "ingenic,jz4780-efuse";
>> + reg = <0x134100D0 0xFF>;
>> +
>> + clocks = < JZ4780_CLK_AHB2>;
>> + clock-names = "bus_clk";
>> +};
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index a6e86e20761e..7a050c20c533 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -6902,6 +6902,11 @@ M: Zubair Lutfullah Kakakhel 
>> <zubair.kakak...@imgtec.com>
>>  S:   Maintained
>>  F:   drivers/dma/dma-jz4780.c
>>
>> +INGENIC JZ4780 EFUSE Driver
>> +M:   PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
>> +S:   Maintained
>> +F:   drivers/nvmem/jz4780-efuse.c
>
> Binding file?

Sorry, missed it. Will add it.

>> +
>>  INGENIC JZ4780 NAND DRIVER
>>  M:   H

Re: [PATCH v2 1/2] nvmem: add driver for JZ4780 efuse

[PrasannaKumar Muralidharan]

Hi Rob,

On 4 January 2018 at 01:32, Rob Herring  wrote:
> On Thu, Dec 28, 2017 at 10:29:52PM +0100, Mathieu Malaterre wrote:
>> From: PrasannaKumar Muralidharan 
>>
>> This patch brings support for the JZ4780 efuse. Currently it only expose
>> a read only access to the entire 8K bits efuse memory.
>>
>> Tested-by: Mathieu Malaterre 
>> Signed-off-by: PrasannaKumar Muralidharan 
>> Signed-off-by: Mathieu Malaterre 
>> ---
>>  .../ABI/testing/sysfs-driver-jz4780-efuse  |  16 ++
>>  .../bindings/nvmem/ingenic,jz4780-efuse.txt|  17 ++
>
> Please split bindings to separate patch.
>
>>  MAINTAINERS|   5 +
>>  arch/mips/boot/dts/ingenic/jz4780.dtsi |  40 ++-
>
> dts files should also be separate.
>
>>  drivers/nvmem/Kconfig  |  10 +
>>  drivers/nvmem/Makefile |   2 +
>>  drivers/nvmem/jz4780-efuse.c   | 305 
>> +
>>  7 files changed, 383 insertions(+), 12 deletions(-)
>>  create mode 100644 Documentation/ABI/testing/sysfs-driver-jz4780-efuse
>>  create mode 100644 
>> Documentation/devicetree/bindings/nvmem/ingenic,jz4780-efuse.txt
>>  create mode 100644 drivers/nvmem/jz4780-efuse.c
>>
>> diff --git a/Documentation/ABI/testing/sysfs-driver-jz4780-efuse 
>> b/Documentation/ABI/testing/sysfs-driver-jz4780-efuse
>> new file mode 100644
>> index ..bb6f5d6ceea0
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-driver-jz4780-efuse
>> @@ -0,0 +1,16 @@
>> +What:/sys/devices/*//nvmem
>> +Date:December 2017
>> +Contact: PrasannaKumar Muralidharan 
>> +Description: read-only access to the efuse on the Ingenic JZ4780 SoC
>> + The SoC has a one time programmable 8K efuse that is
>> + split into segments. The driver supports read only.
>> + The segments are
>> + 0x000   64 bit Random Number
>> + 0x008  128 bit Ingenic Chip ID
>> + 0x018  128 bit Customer ID
>> + 0x028 3520 bit Reserved
>> + 0x1E08 bit Protect Segment
>> + 0x1E1 2296 bit HDMI Key
>> + 0x300 2048 bit Security boot key
>
> Why do these need to be exposed to userspace?
>
> sysfs is 1 value per file and this is lots of different things.
>
> We already have ways to feed random data (entropy) to the system. And we
> have a way to expose SoC ID info to userspace (socdev).

Currently ingenic chip id is not used anywhere. The vendor BSP exposed
only chip id and customer id. Should we do the same? Please provide
your suggestion.

>> +Users:   any user space application which wants to read the Chip
>> + and Customer ID
>> diff --git 
>> a/Documentation/devicetree/bindings/nvmem/ingenic,jz4780-efuse.txt 
>> b/Documentation/devicetree/bindings/nvmem/ingenic,jz4780-efuse.txt
>> new file mode 100644
>> index ..cd6d67ec22fc
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/nvmem/ingenic,jz4780-efuse.txt
>> @@ -0,0 +1,17 @@
>> +Ingenic JZ EFUSE driver bindings
>> +
>> +Required properties:
>> +- "compatible"   Must be set to "ingenic,jz4780-efuse"
>> +- "reg"  Register location and length
>> +- "clocks"   Handle for the ahb clock for the efuse.
>> +- "clock-names"  Must be "bus_clk"
>> +
>> +Example:
>> +
>> +efuse: efuse@134100d0 {
>> + compatible = "ingenic,jz4780-efuse";
>> + reg = <0x134100D0 0xFF>;
>> +
>> + clocks = < JZ4780_CLK_AHB2>;
>> + clock-names = "bus_clk";
>> +};
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index a6e86e20761e..7a050c20c533 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -6902,6 +6902,11 @@ M: Zubair Lutfullah Kakakhel 
>> 
>>  S:   Maintained
>>  F:   drivers/dma/dma-jz4780.c
>>
>> +INGENIC JZ4780 EFUSE Driver
>> +M:   PrasannaKumar Muralidharan 
>> +S:   Maintained
>> +F:   drivers/nvmem/jz4780-efuse.c
>
> Binding file?

Sorry, missed it. Will add it.

>> +
>>  INGENIC JZ4780 NAND DRIVER
>>  M:   Harvey Hunt 
>>  L:   linux-...@lists.infradead.org
>> diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi 
>> b/arch/mips/boot/dts/ingenic/jz4780.dtsi
>> index 9b5794667aee..3fb9d916a2ea 100644
>> --- a/

Re: [PATCH 1/2] nvmem: add driver for JZ4780 efuse

[PrasannaKumar Muralidharan]

Hi Marcin,

On 28 December 2017 at 13:35, Marcin Nowakowski
<marcin.nowakow...@mips.com> wrote:
> Hi Mathieu,
>
> On 28.12.2017 08:26, Mathieu Malaterre wrote:
>>
>> Hi Marcin,
>>
>> On Thu, Dec 28, 2017 at 8:13 AM, Marcin Nowakowski
>> <marcin.nowakow...@mips.com <mailto:marcin.nowakow...@mips.com>> wrote:
>>  > Hi Mathieu, PrasannaKumar,
>>  >
>>  > On 27.12.2017 13:27, Mathieu Malaterre wrote:
>>  >>
>>  >> From: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com
>> <mailto:prasannatsmku...@gmail.com>>
>>  >>
>>  >> This patch brings support for the JZ4780 efuse. Currently it only
>> expose
>>  >> a read only access to the entire 8K bits efuse memory.
>>  >>
>>  >> Tested-by: Mathieu Malaterre <ma...@debian.org
>> <mailto:ma...@debian.org>>
>>  >> Signed-off-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com
>> <mailto:prasannatsmku...@gmail.com>>
>>  >> ---
>>  >
>>  >
>>  >> +
>>  >> +/* main entry point */
>>  >> +static int jz4780_efuse_read(void *context, unsigned int offset,
>>  >> +   void *val, size_t bytes)
>>  >> +{
>>  >> +   static const int nsegments = sizeof(segments) /
>> sizeof(*segments);
>>  >> +   struct jz4780_efuse *efuse = context;
>>  >> +   char buf[32];
>>  >> +   char *cur = val;
>>  >> +   int i;
>>  >> +   /* PM recommends read/write each segment separately */
>>  >> +   for (i = 0; i < nsegments; ++i) {
>>  >> +   unsigned int *segment = segments[i];
>>  >> +   unsigned int lpos = segment[0];
>>  >> +   unsigned int buflen = segment[1] / 8;
>>  >> +   unsigned int ncount = buflen / 32;
>>  >> +   unsigned int remain = buflen % 32;
>>  >> +   int j;
>>  >
>>  >
>>  > This doesn't look right, as offset & bytes are completely ignored. This
>>  > means it will return data from an offset other than requested and may
>> also
>>  > overrun the provided output buffer?
>>
>>
>> Thanks for the review ! That was the part of nvmem framework I was not
>> totally clear. Let say I want to expose only a portion of efuse space, eg:
>
>
> Do you need to expose this to the userspace or to other drivers only?
> For the second case have a look at the description of nvmem cell interface.
>
>
>> diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi
>> b/arch/mips/boot/dts/ingenic/jz4780.dtsi
>> index 2f26922718559..44d97c06a6d15 100644
>> --- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
>> +++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
>> @@ -299,6 +299,15 @@
>> clocks = < JZ4780_CLK_AHB2>;
>> clock-names = "bus_clk";
>> +
>> +#address-cells = <1>;
>> +#size-cells = <1>;
>> +
>> +eth_mac: eth_mac@12 {
>> +/* six byte/48bit MAC address stored as 8-bit integers */
>> +reg = <0x12 0x6>;
>> +};
>> +
>> };
>> };
>> What should I do to expose that chunk only in the user space ?
>
>
> The nvmem interface's userspace interface (via /sys/.../nvmem) provides
> access to the complete device raw memory so the only way to achieve that
> would be to parse the devicetree description in your driver and only
> register part of the memory with the nvmem driver - but that would be a
> slight abuse of the interface.
> The nvmem devicetree binding document shows clearly how to define the cell
> interface that can later be used by any consumer - that way you could have
> the ethernet driver access the cell directly. However, as the dm9000 driver
> isn't designed to do that and this is a SoC-specific extention, I don't know
> how it fits with the general eth driver design ...
>
> Potentially a good and useful compromise would be to have all of the cell
> regs exposed via /sys/.../nvmem-cellname file (or something similar), but
> this is not currently supported and I don't know what the view of nvmem
> maintainers on adding such extension would be.

Currently exposing MAC address is necessary. No need to worry about
user space stuff for now.

>>  >
>>  >> +   /* EFUSE can read or write maximum 256bit in each time
>> */
>>  >> +   for (j = 0; j < ncount ; ++j) {
>>  >> +   jz4780_efuse_read_32bytes(efuse, buf, lpos);
>>  >> +   memcpy(cur, buf, sizeof(buf));
>>  >> +   cur += sizeof(buf);
>>  >> +   lpos += sizeof(buf);
>>  >> +   }
>>  >> +   if (remain) {
>>  >> +   jz4780_efuse_read_32bytes(efuse, buf, lpos);
>>  >> +   memcpy(cur, buf, remain);
>>  >> +   cur += remain;
>>  >> +   }
>>  >> +   }
>>  >> +
>>  >> +   return 0;
>>  >> +}
>
>
> Regardless of the choices above, you still always have to make sure in your
> reg_read method that you only read from the offset specified in method
> arguments and never return more than 'bytes' of data requested.

Sure, will do that.

Regards,
PrasannaKumar

Re: [PATCH 1/2] nvmem: add driver for JZ4780 efuse

[PrasannaKumar Muralidharan]

Hi Marcin,

On 28 December 2017 at 13:35, Marcin Nowakowski
 wrote:
> Hi Mathieu,
>
> On 28.12.2017 08:26, Mathieu Malaterre wrote:
>>
>> Hi Marcin,
>>
>> On Thu, Dec 28, 2017 at 8:13 AM, Marcin Nowakowski
>> mailto:marcin.nowakow...@mips.com>> wrote:
>>  > Hi Mathieu, PrasannaKumar,
>>  >
>>  > On 27.12.2017 13:27, Mathieu Malaterre wrote:
>>  >>
>>  >> From: PrasannaKumar Muralidharan > <mailto:prasannatsmku...@gmail.com>>
>>  >>
>>  >> This patch brings support for the JZ4780 efuse. Currently it only
>> expose
>>  >> a read only access to the entire 8K bits efuse memory.
>>  >>
>>  >> Tested-by: Mathieu Malaterre > <mailto:ma...@debian.org>>
>>  >> Signed-off-by: PrasannaKumar Muralidharan > <mailto:prasannatsmku...@gmail.com>>
>>  >> ---
>>  >
>>  >
>>  >> +
>>  >> +/* main entry point */
>>  >> +static int jz4780_efuse_read(void *context, unsigned int offset,
>>  >> +   void *val, size_t bytes)
>>  >> +{
>>  >> +   static const int nsegments = sizeof(segments) /
>> sizeof(*segments);
>>  >> +   struct jz4780_efuse *efuse = context;
>>  >> +   char buf[32];
>>  >> +   char *cur = val;
>>  >> +   int i;
>>  >> +   /* PM recommends read/write each segment separately */
>>  >> +   for (i = 0; i < nsegments; ++i) {
>>  >> +   unsigned int *segment = segments[i];
>>  >> +   unsigned int lpos = segment[0];
>>  >> +   unsigned int buflen = segment[1] / 8;
>>  >> +   unsigned int ncount = buflen / 32;
>>  >> +   unsigned int remain = buflen % 32;
>>  >> +   int j;
>>  >
>>  >
>>  > This doesn't look right, as offset & bytes are completely ignored. This
>>  > means it will return data from an offset other than requested and may
>> also
>>  > overrun the provided output buffer?
>>
>>
>> Thanks for the review ! That was the part of nvmem framework I was not
>> totally clear. Let say I want to expose only a portion of efuse space, eg:
>
>
> Do you need to expose this to the userspace or to other drivers only?
> For the second case have a look at the description of nvmem cell interface.
>
>
>> diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi
>> b/arch/mips/boot/dts/ingenic/jz4780.dtsi
>> index 2f26922718559..44d97c06a6d15 100644
>> --- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
>> +++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
>> @@ -299,6 +299,15 @@
>> clocks = < JZ4780_CLK_AHB2>;
>> clock-names = "bus_clk";
>> +
>> +#address-cells = <1>;
>> +#size-cells = <1>;
>> +
>> +eth_mac: eth_mac@12 {
>> +/* six byte/48bit MAC address stored as 8-bit integers */
>> +reg = <0x12 0x6>;
>> +};
>> +
>> };
>> };
>> What should I do to expose that chunk only in the user space ?
>
>
> The nvmem interface's userspace interface (via /sys/.../nvmem) provides
> access to the complete device raw memory so the only way to achieve that
> would be to parse the devicetree description in your driver and only
> register part of the memory with the nvmem driver - but that would be a
> slight abuse of the interface.
> The nvmem devicetree binding document shows clearly how to define the cell
> interface that can later be used by any consumer - that way you could have
> the ethernet driver access the cell directly. However, as the dm9000 driver
> isn't designed to do that and this is a SoC-specific extention, I don't know
> how it fits with the general eth driver design ...
>
> Potentially a good and useful compromise would be to have all of the cell
> regs exposed via /sys/.../nvmem-cellname file (or something similar), but
> this is not currently supported and I don't know what the view of nvmem
> maintainers on adding such extension would be.

Currently exposing MAC address is necessary. No need to worry about
user space stuff for now.

>>  >
>>  >> +   /* EFUSE can read or write maximum 256bit in each time
>> */
>>  >> +   for (j = 0; j < ncount ; ++j) {
>>  >> +   jz4780_efuse_read_32bytes(efuse, buf, lpos);
>>  >> +   memcpy(cur, buf, sizeof(buf));
>>  >> +   cur += sizeof(buf);
>>  >> +   lpos += sizeof(buf);
>>  >> +   }
>>  >> +   if (remain) {
>>  >> +   jz4780_efuse_read_32bytes(efuse, buf, lpos);
>>  >> +   memcpy(cur, buf, remain);
>>  >> +   cur += remain;
>>  >> +   }
>>  >> +   }
>>  >> +
>>  >> +   return 0;
>>  >> +}
>
>
> Regardless of the choices above, you still always have to make sure in your
> reg_read method that you only read from the offset specified in method
> arguments and never return more than 'bytes' of data requested.

Sure, will do that.

Regards,
PrasannaKumar

Re: [PATCH v2 5/8] MIPS: jz4740: dts: Add bindings for the jz4740-wdt driver

[PrasannaKumar Muralidharan]

Hi Guenter,

On 3 January 2018 at 10:16, Guenter Roeck <li...@roeck-us.net> wrote:
> On 01/02/2018 08:48 AM, Paul Cercueil wrote:
>>
>> Hi PrasannaKumar,
>>
>> Le mar. 2 janv. 2018 à 17:37, PrasannaKumar Muralidharan
>> <prasannatsmku...@gmail.com> a écrit :
>>>
>>> Hi Paul,
>>>
>>> On 30 December 2017 at 19:21, Paul Cercueil <p...@crapouillou.net> wrote:
>>>>
>>>>  Also remove the watchdog platform_device from platform.c, since it
>>>>  wasn't used anywhere anyway.
>>>>
>>>>  Signed-off-by: Paul Cercueil <p...@crapouillou.net>
>>>>  ---
>>>>   arch/mips/boot/dts/ingenic/jz4740.dtsi |  8 
>>>>   arch/mips/jz4740/platform.c| 16 
>>>>   2 files changed, 8 insertions(+), 16 deletions(-)
>>>>
>>>>   v2: No change
>>>>
>>>>  diff --git a/arch/mips/boot/dts/ingenic/jz4740.dtsi
>>>> b/arch/mips/boot/dts/ingenic/jz4740.dtsi
>>>>  index cd5185bb90ae..26c6b561d6f7 100644
>>>>  --- a/arch/mips/boot/dts/ingenic/jz4740.dtsi
>>>>  +++ b/arch/mips/boot/dts/ingenic/jz4740.dtsi
>>>>  @@ -45,6 +45,14 @@
>>>>  #clock-cells = <1>;
>>>>  };
>>>>
>>>>  +   watchdog: watchdog@10002000 {
>>>>  +   compatible = "ingenic,jz4740-watchdog";
>>>>  +   reg = <0x10002000 0x10>;
>>>>  +
>>>>  +   clocks = < JZ4740_CLK_RTC>;
>>>>  +   clock-names = "rtc";
>>>>  +   };
>>>>  +
>>>
>>>
>>> The watchdog driver calls jz4740_timer_enable_watchdog and
>>> jz4740_timer_disable_watchdog which defined in
>>> arch/mips/jz4740/timer.c. It accesses registers iomapped by timer
>>> code. Declaring register size as 0x10 does not show the real picture.
>>> Better use register size as 0x100 and let timer, wdt, pwm drivers to
>>> share them.
>>
>>
>> As you said, it accesses registers iomapped by timer code. So the watchdog
>> driver doesn't need to iomap them.
>>
>>> Code from one of your branches
>>>
>>> (https://github.com/OpenDingux/linux/blob/for-upstream-clocksource/arch/mips/boot/dts/ingenic/jz4740.dtsi)
>>> does it. Can you prepare a patch series and send it?
>>> I have a patch set that moves timer code out of arch/mips/jz4740/ and
>>> does a similar thing for watchdog and pwm. As your new timer driver is
>>> better than the existing one I have not sent my patches yet. I would
>>> like to see it getting mainlined as it paves way for removing most of
>>> code in arch/mips/jz4740.
>>
>>
>> The whole 'for-upstream-clocksource' branch is supposed to go upstream,
>> but I can't do it in one big patchset without having lots of breakages
>> with
>> my other patchsets (jz4770 SoC support, and jz4740 watchdog updates)
>> currently under review. That also makes it simpler to upstream than having
>> one single patchset that touches 6 different frameworks (MIPS, irq,
>> clocks,
>> clocksource, watchdog, PWM).
>>
>> So I will submit it in two steps, first the irq/clocks/clocksource drivers
>> (this patchset) hopefully for 4.16, and then the platform/watchdog/PWM
>> fixes
>> for 4.17.
>>
>
> I kind of lost it in this exchange, sorry. At this point I don't know if
> something
> is wrong with the watchdog patches, and I have no clue what the upstream
> path

There is nothing wrong in this watchdog patches.

> is supposed to be. My working assumption is that 1) something may be wrong
> with
> the current version of the patches, and, 2), even if not, none of the
> patches
> is expected to find its way upstream through the watchdog subsystem. Plus,
> 3),
> even if some of the patches are supposed to go upstream through the watchdog
> subsystem, that won't happen in 4.16, and the patches will be resubmitted
> later
> when they are ready [and will hopefully marked clearly for submission
> through
> the watchdog subsystem].
>
> With that in mind, I'll mark the series for my reference as "not
> applicable".
> If this is wrong please let me know.

Paul has patches related to timer code. While sending that he would
send changes to watchdog dts entry also. I was suggesting him to send
timer patches together with watchdog patches as a single patch series
but he prefers to send them as separate patch series.

I would like to reiterate that there is nothing wrong with this
watchdog patches. I should have set the correct context in my previous
email itself. I sincerely apologize for creating this confusion.

Regards,
PrasannaKumar

Re: [PATCH v2 5/8] MIPS: jz4740: dts: Add bindings for the jz4740-wdt driver

[PrasannaKumar Muralidharan]

Hi Guenter,

On 3 January 2018 at 10:16, Guenter Roeck  wrote:
> On 01/02/2018 08:48 AM, Paul Cercueil wrote:
>>
>> Hi PrasannaKumar,
>>
>> Le mar. 2 janv. 2018 à 17:37, PrasannaKumar Muralidharan
>>  a écrit :
>>>
>>> Hi Paul,
>>>
>>> On 30 December 2017 at 19:21, Paul Cercueil  wrote:
>>>>
>>>>  Also remove the watchdog platform_device from platform.c, since it
>>>>  wasn't used anywhere anyway.
>>>>
>>>>  Signed-off-by: Paul Cercueil 
>>>>  ---
>>>>   arch/mips/boot/dts/ingenic/jz4740.dtsi |  8 
>>>>   arch/mips/jz4740/platform.c| 16 
>>>>   2 files changed, 8 insertions(+), 16 deletions(-)
>>>>
>>>>   v2: No change
>>>>
>>>>  diff --git a/arch/mips/boot/dts/ingenic/jz4740.dtsi
>>>> b/arch/mips/boot/dts/ingenic/jz4740.dtsi
>>>>  index cd5185bb90ae..26c6b561d6f7 100644
>>>>  --- a/arch/mips/boot/dts/ingenic/jz4740.dtsi
>>>>  +++ b/arch/mips/boot/dts/ingenic/jz4740.dtsi
>>>>  @@ -45,6 +45,14 @@
>>>>  #clock-cells = <1>;
>>>>  };
>>>>
>>>>  +   watchdog: watchdog@10002000 {
>>>>  +   compatible = "ingenic,jz4740-watchdog";
>>>>  +   reg = <0x10002000 0x10>;
>>>>  +
>>>>  +   clocks = < JZ4740_CLK_RTC>;
>>>>  +   clock-names = "rtc";
>>>>  +   };
>>>>  +
>>>
>>>
>>> The watchdog driver calls jz4740_timer_enable_watchdog and
>>> jz4740_timer_disable_watchdog which defined in
>>> arch/mips/jz4740/timer.c. It accesses registers iomapped by timer
>>> code. Declaring register size as 0x10 does not show the real picture.
>>> Better use register size as 0x100 and let timer, wdt, pwm drivers to
>>> share them.
>>
>>
>> As you said, it accesses registers iomapped by timer code. So the watchdog
>> driver doesn't need to iomap them.
>>
>>> Code from one of your branches
>>>
>>> (https://github.com/OpenDingux/linux/blob/for-upstream-clocksource/arch/mips/boot/dts/ingenic/jz4740.dtsi)
>>> does it. Can you prepare a patch series and send it?
>>> I have a patch set that moves timer code out of arch/mips/jz4740/ and
>>> does a similar thing for watchdog and pwm. As your new timer driver is
>>> better than the existing one I have not sent my patches yet. I would
>>> like to see it getting mainlined as it paves way for removing most of
>>> code in arch/mips/jz4740.
>>
>>
>> The whole 'for-upstream-clocksource' branch is supposed to go upstream,
>> but I can't do it in one big patchset without having lots of breakages
>> with
>> my other patchsets (jz4770 SoC support, and jz4740 watchdog updates)
>> currently under review. That also makes it simpler to upstream than having
>> one single patchset that touches 6 different frameworks (MIPS, irq,
>> clocks,
>> clocksource, watchdog, PWM).
>>
>> So I will submit it in two steps, first the irq/clocks/clocksource drivers
>> (this patchset) hopefully for 4.16, and then the platform/watchdog/PWM
>> fixes
>> for 4.17.
>>
>
> I kind of lost it in this exchange, sorry. At this point I don't know if
> something
> is wrong with the watchdog patches, and I have no clue what the upstream
> path

There is nothing wrong in this watchdog patches.

> is supposed to be. My working assumption is that 1) something may be wrong
> with
> the current version of the patches, and, 2), even if not, none of the
> patches
> is expected to find its way upstream through the watchdog subsystem. Plus,
> 3),
> even if some of the patches are supposed to go upstream through the watchdog
> subsystem, that won't happen in 4.16, and the patches will be resubmitted
> later
> when they are ready [and will hopefully marked clearly for submission
> through
> the watchdog subsystem].
>
> With that in mind, I'll mark the series for my reference as "not
> applicable".
> If this is wrong please let me know.

Paul has patches related to timer code. While sending that he would
send changes to watchdog dts entry also. I was suggesting him to send
timer patches together with watchdog patches as a single patch series
but he prefers to send them as separate patch series.

I would like to reiterate that there is nothing wrong with this
watchdog patches. I should have set the correct context in my previous
email itself. I sincerely apologize for creating this confusion.

Regards,
PrasannaKumar

Re: [PATCH v5 13/15] MIPS: JZ4770: Workaround for corrupted DMA transfers

[PrasannaKumar Muralidharan]

Hi Paul,

On 2 January 2018 at 20:38, Paul Cercueil  wrote:
> From: Maarten ter Huurne 
>
> We have seen MMC DMA transfers read corrupted data from SDRAM when
> a burst interval ends at physical address 0x1000. To avoid this
> problem, we remove the final page of low memory from the memory map.
>
> Signed-off-by: Maarten ter Huurne 
> ---
>  arch/mips/jz4740/setup.c | 24 
>  arch/mips/kernel/setup.c |  8 
>  2 files changed, 32 insertions(+)
>
>  v2: No change
>  v3: No change
>  v4: No change
>  v5: No change
>
> diff --git a/arch/mips/jz4740/setup.c b/arch/mips/jz4740/setup.c
> index afd84ee966e8..6948b133a15d 100644
> --- a/arch/mips/jz4740/setup.c
> +++ b/arch/mips/jz4740/setup.c
> @@ -23,6 +23,7 @@
>
>  #include 
>  #include 
> +#include 
>  #include 
>
>  #include 
> @@ -102,6 +103,29 @@ void __init arch_init_irq(void)
> irqchip_init();
>  }
>
> +/*
> + * We have seen MMC DMA transfers read corrupted data from SDRAM when a burst
> + * interval ends at physical address 0x1000. To avoid this problem, we
> + * remove the final page of low memory from the memory map.
> + */
> +void __init jz4770_reserve_unsafe_for_dma(void)
> +{
> +   int i;
> +
> +   for (i = 0; i < boot_mem_map.nr_map; i++) {
> +   struct boot_mem_map_entry *entry = boot_mem_map.map + i;
> +
> +   if (entry->type != BOOT_MEM_RAM)
> +   continue;
> +
> +   if (entry->addr + entry->size != 0x1000)
> +   continue;
> +
> +   entry->size -= PAGE_SIZE;
> +   break;
> +   }
> +}
> +

Just a wild idea (probably bad too). Changing the memory node in the
device tree to skip this physical address would work I think. What is
your opinion about that?

>  static int __init jz4740_machine_setup(void)
>  {
> mips_machine_setup();
> diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
> index 85bc601e9a0d..5a2c20145aee 100644
> --- a/arch/mips/kernel/setup.c
> +++ b/arch/mips/kernel/setup.c
> @@ -879,6 +879,14 @@ static void __init arch_mem_init(char **cmdline_p)
>
> parse_early_param();
>
> +#ifdef CONFIG_MACH_JZ4770
> +   if (current_cpu_type() == CPU_JZRISC &&
> +   mips_machtype == MACH_INGENIC_JZ4770) {
> +   extern void __init jz4770_reserve_unsafe_for_dma(void);
> +   jz4770_reserve_unsafe_for_dma();
> +   }
> +#endif
> +
> if (usermem) {
> pr_info("User-defined physical RAM map:\n");
> print_memory_map();
> --
> 2.11.0
>
>

Thanks,
PrasannaKumar

Re: [PATCH v5 13/15] MIPS: JZ4770: Workaround for corrupted DMA transfers

[PrasannaKumar Muralidharan]

Hi Paul,

On 2 January 2018 at 20:38, Paul Cercueil  wrote:
> From: Maarten ter Huurne 
>
> We have seen MMC DMA transfers read corrupted data from SDRAM when
> a burst interval ends at physical address 0x1000. To avoid this
> problem, we remove the final page of low memory from the memory map.
>
> Signed-off-by: Maarten ter Huurne 
> ---
>  arch/mips/jz4740/setup.c | 24 
>  arch/mips/kernel/setup.c |  8 
>  2 files changed, 32 insertions(+)
>
>  v2: No change
>  v3: No change
>  v4: No change
>  v5: No change
>
> diff --git a/arch/mips/jz4740/setup.c b/arch/mips/jz4740/setup.c
> index afd84ee966e8..6948b133a15d 100644
> --- a/arch/mips/jz4740/setup.c
> +++ b/arch/mips/jz4740/setup.c
> @@ -23,6 +23,7 @@
>
>  #include 
>  #include 
> +#include 
>  #include 
>
>  #include 
> @@ -102,6 +103,29 @@ void __init arch_init_irq(void)
> irqchip_init();
>  }
>
> +/*
> + * We have seen MMC DMA transfers read corrupted data from SDRAM when a burst
> + * interval ends at physical address 0x1000. To avoid this problem, we
> + * remove the final page of low memory from the memory map.
> + */
> +void __init jz4770_reserve_unsafe_for_dma(void)
> +{
> +   int i;
> +
> +   for (i = 0; i < boot_mem_map.nr_map; i++) {
> +   struct boot_mem_map_entry *entry = boot_mem_map.map + i;
> +
> +   if (entry->type != BOOT_MEM_RAM)
> +   continue;
> +
> +   if (entry->addr + entry->size != 0x1000)
> +   continue;
> +
> +   entry->size -= PAGE_SIZE;
> +   break;
> +   }
> +}
> +

Just a wild idea (probably bad too). Changing the memory node in the
device tree to skip this physical address would work I think. What is
your opinion about that?

>  static int __init jz4740_machine_setup(void)
>  {
> mips_machine_setup();
> diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
> index 85bc601e9a0d..5a2c20145aee 100644
> --- a/arch/mips/kernel/setup.c
> +++ b/arch/mips/kernel/setup.c
> @@ -879,6 +879,14 @@ static void __init arch_mem_init(char **cmdline_p)
>
> parse_early_param();
>
> +#ifdef CONFIG_MACH_JZ4770
> +   if (current_cpu_type() == CPU_JZRISC &&
> +   mips_machtype == MACH_INGENIC_JZ4770) {
> +   extern void __init jz4770_reserve_unsafe_for_dma(void);
> +   jz4770_reserve_unsafe_for_dma();
> +   }
> +#endif
> +
> if (usermem) {
> pr_info("User-defined physical RAM map:\n");
> print_memory_map();
> --
> 2.11.0
>
>

Thanks,
PrasannaKumar

Re: [PATCH v2 5/8] MIPS: jz4740: dts: Add bindings for the jz4740-wdt driver

[PrasannaKumar Muralidharan]

Hi Paul,

On 30 December 2017 at 19:21, Paul Cercueil  wrote:
> Also remove the watchdog platform_device from platform.c, since it
> wasn't used anywhere anyway.
>
> Signed-off-by: Paul Cercueil 
> ---
>  arch/mips/boot/dts/ingenic/jz4740.dtsi |  8 
>  arch/mips/jz4740/platform.c| 16 
>  2 files changed, 8 insertions(+), 16 deletions(-)
>
>  v2: No change
>
> diff --git a/arch/mips/boot/dts/ingenic/jz4740.dtsi 
> b/arch/mips/boot/dts/ingenic/jz4740.dtsi
> index cd5185bb90ae..26c6b561d6f7 100644
> --- a/arch/mips/boot/dts/ingenic/jz4740.dtsi
> +++ b/arch/mips/boot/dts/ingenic/jz4740.dtsi
> @@ -45,6 +45,14 @@
> #clock-cells = <1>;
> };
>
> +   watchdog: watchdog@10002000 {
> +   compatible = "ingenic,jz4740-watchdog";
> +   reg = <0x10002000 0x10>;
> +
> +   clocks = < JZ4740_CLK_RTC>;
> +   clock-names = "rtc";
> +   };
> +

The watchdog driver calls jz4740_timer_enable_watchdog and
jz4740_timer_disable_watchdog which defined in
arch/mips/jz4740/timer.c. It accesses registers iomapped by timer
code. Declaring register size as 0x10 does not show the real picture.
Better use register size as 0x100 and let timer, wdt, pwm drivers to
share them.

Code from one of your branches
(https://github.com/OpenDingux/linux/blob/for-upstream-clocksource/arch/mips/boot/dts/ingenic/jz4740.dtsi)
does it. Can you prepare a patch series and send it?
I have a patch set that moves timer code out of arch/mips/jz4740/ and
does a similar thing for watchdog and pwm. As your new timer driver is
better than the existing one I have not sent my patches yet. I would
like to see it getting mainlined as it paves way for removing most of
code in arch/mips/jz4740.

> rtc_dev: rtc@10003000 {
> compatible = "ingenic,jz4740-rtc";
> reg = <0x10003000 0x40>;
> diff --git a/arch/mips/jz4740/platform.c b/arch/mips/jz4740/platform.c
> index 5b7cdd67a9d9..cbc5f8e87230 100644
> --- a/arch/mips/jz4740/platform.c
> +++ b/arch/mips/jz4740/platform.c
> @@ -233,22 +233,6 @@ struct platform_device jz4740_adc_device = {
> .resource   = jz4740_adc_resources,
>  };
>
> -/* Watchdog */
> -static struct resource jz4740_wdt_resources[] = {
> -   {
> -   .start = JZ4740_WDT_BASE_ADDR,
> -   .end   = JZ4740_WDT_BASE_ADDR + 0x10 - 1,
> -   .flags = IORESOURCE_MEM,
> -   },
> -};
> -
> -struct platform_device jz4740_wdt_device = {
> -   .name  = "jz4740-wdt",
> -   .id= -1,
> -   .num_resources = ARRAY_SIZE(jz4740_wdt_resources),
> -   .resource  = jz4740_wdt_resources,
> -};
> -
>  /* PWM */
>  struct platform_device jz4740_pwm_device = {
> .name = "jz4740-pwm",
> --
> 2.11.0
>
>

Regards,
PrasannaKumar

Re: [PATCH v2 5/8] MIPS: jz4740: dts: Add bindings for the jz4740-wdt driver

[PrasannaKumar Muralidharan]

Hi Paul,

On 30 December 2017 at 19:21, Paul Cercueil  wrote:
> Also remove the watchdog platform_device from platform.c, since it
> wasn't used anywhere anyway.
>
> Signed-off-by: Paul Cercueil 
> ---
>  arch/mips/boot/dts/ingenic/jz4740.dtsi |  8 
>  arch/mips/jz4740/platform.c| 16 
>  2 files changed, 8 insertions(+), 16 deletions(-)
>
>  v2: No change
>
> diff --git a/arch/mips/boot/dts/ingenic/jz4740.dtsi 
> b/arch/mips/boot/dts/ingenic/jz4740.dtsi
> index cd5185bb90ae..26c6b561d6f7 100644
> --- a/arch/mips/boot/dts/ingenic/jz4740.dtsi
> +++ b/arch/mips/boot/dts/ingenic/jz4740.dtsi
> @@ -45,6 +45,14 @@
> #clock-cells = <1>;
> };
>
> +   watchdog: watchdog@10002000 {
> +   compatible = "ingenic,jz4740-watchdog";
> +   reg = <0x10002000 0x10>;
> +
> +   clocks = < JZ4740_CLK_RTC>;
> +   clock-names = "rtc";
> +   };
> +

The watchdog driver calls jz4740_timer_enable_watchdog and
jz4740_timer_disable_watchdog which defined in
arch/mips/jz4740/timer.c. It accesses registers iomapped by timer
code. Declaring register size as 0x10 does not show the real picture.
Better use register size as 0x100 and let timer, wdt, pwm drivers to
share them.

Code from one of your branches
(https://github.com/OpenDingux/linux/blob/for-upstream-clocksource/arch/mips/boot/dts/ingenic/jz4740.dtsi)
does it. Can you prepare a patch series and send it?
I have a patch set that moves timer code out of arch/mips/jz4740/ and
does a similar thing for watchdog and pwm. As your new timer driver is
better than the existing one I have not sent my patches yet. I would
like to see it getting mainlined as it paves way for removing most of
code in arch/mips/jz4740.

> rtc_dev: rtc@10003000 {
> compatible = "ingenic,jz4740-rtc";
> reg = <0x10003000 0x40>;
> diff --git a/arch/mips/jz4740/platform.c b/arch/mips/jz4740/platform.c
> index 5b7cdd67a9d9..cbc5f8e87230 100644
> --- a/arch/mips/jz4740/platform.c
> +++ b/arch/mips/jz4740/platform.c
> @@ -233,22 +233,6 @@ struct platform_device jz4740_adc_device = {
> .resource   = jz4740_adc_resources,
>  };
>
> -/* Watchdog */
> -static struct resource jz4740_wdt_resources[] = {
> -   {
> -   .start = JZ4740_WDT_BASE_ADDR,
> -   .end   = JZ4740_WDT_BASE_ADDR + 0x10 - 1,
> -   .flags = IORESOURCE_MEM,
> -   },
> -};
> -
> -struct platform_device jz4740_wdt_device = {
> -   .name  = "jz4740-wdt",
> -   .id= -1,
> -   .num_resources = ARRAY_SIZE(jz4740_wdt_resources),
> -   .resource  = jz4740_wdt_resources,
> -};
> -
>  /* PWM */
>  struct platform_device jz4740_pwm_device = {
> .name = "jz4740-pwm",
> --
> 2.11.0
>
>

Regards,
PrasannaKumar

Re: [PATCH v2 1/2] nvmem: add driver for JZ4780 efuse

[PrasannaKumar Muralidharan]

Hi Srini,

On 2 January 2018 at 17:32, Srinivas Kandagatla
<srinivas.kandaga...@linaro.org> wrote:
>
>
> On 28/12/17 21:29, Mathieu Malaterre wrote:
>>
>> From: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
>>
>> This patch brings support for the JZ4780 efuse. Currently it only expose
>> a read only access to the entire 8K bits efuse memory.
>>
>> Tested-by: Mathieu Malaterre <ma...@debian.org>
>> Signed-off-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
>> Signed-off-by: Mathieu Malaterre <ma...@debian.org>
>> ---
>
> Please split this patch, as you are mixing code, documentation, dts and
> MAINTAINER changes here.
>
> Without which patch can not be reviewed!!

Sure, will do it soon.

>
> Thanks,
> Srini
>
>>   .../ABI/testing/sysfs-driver-jz4780-efuse  |  16 ++
>
>
>>   .../bindings/nvmem/ingenic,jz4780-efuse.txt|  17 ++
>>   MAINTAINERS|   5 +
>>   arch/mips/boot/dts/ingenic/jz4780.dtsi |  40 ++-
>>   drivers/nvmem/Kconfig  |  10 +
>>   drivers/nvmem/Makefile |   2 +
>>   drivers/nvmem/jz4780-efuse.c   | 305
>> +
>>   7 files changed, 383 insertions(+), 12 deletions(-)
>
> ...

Regards,
PrasannaKumar

Re: [PATCH v2 1/2] nvmem: add driver for JZ4780 efuse

[PrasannaKumar Muralidharan]

Hi Srini,

On 2 January 2018 at 17:32, Srinivas Kandagatla
 wrote:
>
>
> On 28/12/17 21:29, Mathieu Malaterre wrote:
>>
>> From: PrasannaKumar Muralidharan 
>>
>> This patch brings support for the JZ4780 efuse. Currently it only expose
>> a read only access to the entire 8K bits efuse memory.
>>
>> Tested-by: Mathieu Malaterre 
>> Signed-off-by: PrasannaKumar Muralidharan 
>> Signed-off-by: Mathieu Malaterre 
>> ---
>
> Please split this patch, as you are mixing code, documentation, dts and
> MAINTAINER changes here.
>
> Without which patch can not be reviewed!!

Sure, will do it soon.

>
> Thanks,
> Srini
>
>>   .../ABI/testing/sysfs-driver-jz4780-efuse  |  16 ++
>
>
>>   .../bindings/nvmem/ingenic,jz4780-efuse.txt|  17 ++
>>   MAINTAINERS|   5 +
>>   arch/mips/boot/dts/ingenic/jz4780.dtsi |  40 ++-
>>   drivers/nvmem/Kconfig  |  10 +
>>   drivers/nvmem/Makefile |   2 +
>>   drivers/nvmem/jz4780-efuse.c   | 305
>> +
>>   7 files changed, 383 insertions(+), 12 deletions(-)
>
> ...

Regards,
PrasannaKumar

Re: [PATCH v2 0/2] Add efuse driver for Ingenic JZ4780 SoC

[PrasannaKumar Muralidharan]

Hi Srinivas,

On 2 January 2018 at 17:31, Srinivas Kandagatla
 wrote:
>
>
> On 28/12/17 21:29, Mathieu Malaterre wrote:
>>
>> This patchset bring support for read-only access to the JZ4780 efuse as
>> found
>> on MIPS Creator CI20.
>>
>> To keep the driver as simple as possible, it was not possible to re-use
>> most of
>> the nvmem core functionalities. This driver is not compatible with the
>> original
>
> Can you explain a bit more on not able to re-use nvmem core?
>
> If you are referring to adding nvmem cell entires in sysfs, This should
> probably go in to nvmem core, rather that in individual providers.
> This is one of the feature my todo list, will try to come up with some thing
> soon.

We could not find a way to expose different sized segments using nvmem
framework. Do you have any pointers for this?
We were not aware of the fact that nvmem does not expose individual
cell entries in sysfs.

Regards,
PrasannaKumar

Re: [PATCH v2 0/2] Add efuse driver for Ingenic JZ4780 SoC

[PrasannaKumar Muralidharan]

Hi Srinivas,

On 2 January 2018 at 17:31, Srinivas Kandagatla
 wrote:
>
>
> On 28/12/17 21:29, Mathieu Malaterre wrote:
>>
>> This patchset bring support for read-only access to the JZ4780 efuse as
>> found
>> on MIPS Creator CI20.
>>
>> To keep the driver as simple as possible, it was not possible to re-use
>> most of
>> the nvmem core functionalities. This driver is not compatible with the
>> original
>
> Can you explain a bit more on not able to re-use nvmem core?
>
> If you are referring to adding nvmem cell entires in sysfs, This should
> probably go in to nvmem core, rather that in individual providers.
> This is one of the feature my todo list, will try to come up with some thing
> soon.

We could not find a way to expose different sized segments using nvmem
framework. Do you have any pointers for this?
We were not aware of the fact that nvmem does not expose individual
cell entries in sysfs.

Regards,
PrasannaKumar

Re: [PATCH v5 11/15] MIPS: ingenic: Initial JZ4770 support

[PrasannaKumar Muralidharan]

 = <2>;
> +
> +   interrupt-controller;
> +   #interrupt-cells = <2>;
> +
> +   interrupt-parent = <>;
> +   interrupts = <14>;
> +   };
> +
> +   gpe: gpio@4 {
> +   compatible = "ingenic,jz4770-gpio";
> +   reg = <4>;
> +
> +   gpio-controller;
> +   gpio-ranges = < 0 128 32>;
> +   #gpio-cells = <2>;
> +
> +   interrupt-controller;
> +   #interrupt-cells = <2>;
> +
> +   interrupt-parent = <>;
> +   interrupts = <13>;
> +   };
> +
> +   gpf: gpio@5 {
> +   compatible = "ingenic,jz4770-gpio";
> +   reg = <5>;
> +
> +   gpio-controller;
> +   gpio-ranges = < 0 160 32>;
> +   #gpio-cells = <2>;
> +
> +   interrupt-controller;
> +   #interrupt-cells = <2>;
> +
> +   interrupt-parent = <>;
> +   interrupts = <12>;
> +   };
> +   };
> +
> +   uart0: serial@1003 {
> +   compatible = "ingenic,jz4770-uart";
> +   reg = <0x1003 0x100>;
> +
> +   clocks = <>, < JZ4770_CLK_UART0>;
> +   clock-names = "baud", "module";
> +
> +   interrupt-parent = <>;
> +   interrupts = <5>;
> +
> +   status = "disabled";
> +   };
> +
> +   uart1: serial@10031000 {
> +   compatible = "ingenic,jz4770-uart";
> +   reg = <0x10031000 0x100>;
> +
> +   clocks = <>, < JZ4770_CLK_UART1>;
> +   clock-names = "baud", "module";
> +
> +   interrupt-parent = <>;
> +   interrupts = <4>;
> +
> +   status = "disabled";
> +   };
> +
> +   uart2: serial@10032000 {
> +   compatible = "ingenic,jz4770-uart";
> +   reg = <0x10032000 0x100>;
> +
> +   clocks = <>, < JZ4770_CLK_UART2>;
> +   clock-names = "baud", "module";
> +
> +   interrupt-parent = <>;
> +   interrupts = <3>;
> +
> +   status = "disabled";
> +   };
> +
> +   uart3: serial@10033000 {
> +   compatible = "ingenic,jz4770-uart";
> +   reg = <0x10033000 0x100>;
> +
> +   clocks = <>, < JZ4770_CLK_UART3>;
> +   clock-names = "baud", "module";
> +
> +   interrupt-parent = <>;
> +   interrupts = <2>;
> +
> +   status = "disabled";
> +   };
> +
> +   uhc: uhc@1343 {
> +   compatible = "generic-ohci";
> +   reg = <0x1343 0x1000>;
> +
> +   clocks = < JZ4770_CLK_UHC>, < JZ4770_CLK_UHC_PHY>;
> +   assigned-clocks = < JZ4770_CLK_UHC>;
> +   assigned-clock-rates = <4800>;
> +
> +   interrupt-parent = <>;
> +   interrupts = <20>;
> +
> +   status = "disabled";
> +   };
> +};
> diff --git a/arch/mips/jz4740/Kconfig b/arch/mips/jz4740/Kconfig
> index 643af2012e14..29a9361a2b77 100644
> --- a/arch/mips/jz4740/Kconfig
> +++ b/arch/mips/jz4740/Kconfig
> @@ -18,6 +18,12 @@ config MACH_JZ4740
> bool
> select SYS_HAS_CPU_MIPS32_R1
>
> +config MACH_JZ4770
> +   bool
> +   select MIPS_CPU_SCACHE
> +   select SYS_HAS_CPU_MIPS32_R2
> +   select SYS_SUPPORTS_HIGHMEM
> +
>  config MACH_JZ4780
> bool
> select MIPS_CPU_SCACHE
> diff --git a/arch/mips/jz4740/time.c b/arch/mips/jz4740/time.c
> index bb1ad5119da4..2ca9160f642a 100644
> --- a/arch/mips/jz4740/time.c
> +++ b/arch/mips/jz4740/time.c
> @@ -113,7 +113,7 @@ static struct clock_event_device jz4740_clockevent = {
>  #ifdef CONFIG_MACH_JZ4740
> .irq = JZ4740_IRQ_TCU0,
>  #endif
> -#ifdef CONFIG_MACH_JZ4780
> +#if defined(CONFIG_MACH_JZ4770) || defined(CONFIG_MACH_JZ4780)
> .irq = JZ4780_IRQ_TCU2,
>  #endif
>  };
> --
> 2.11.0
>
>

Looks good to me.
Reviewed-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>

Re: [PATCH v5 11/15] MIPS: ingenic: Initial JZ4770 support

[PrasannaKumar Muralidharan]

r;
> +   #interrupt-cells = <2>;
> +
> +   interrupt-parent = <>;
> +   interrupts = <14>;
> +   };
> +
> +   gpe: gpio@4 {
> +   compatible = "ingenic,jz4770-gpio";
> +   reg = <4>;
> +
> +   gpio-controller;
> +   gpio-ranges = < 0 128 32>;
> +   #gpio-cells = <2>;
> +
> +   interrupt-controller;
> +   #interrupt-cells = <2>;
> +
> +   interrupt-parent = <>;
> +   interrupts = <13>;
> +   };
> +
> +   gpf: gpio@5 {
> +   compatible = "ingenic,jz4770-gpio";
> +   reg = <5>;
> +
> +   gpio-controller;
> +   gpio-ranges = < 0 160 32>;
> +   #gpio-cells = <2>;
> +
> +   interrupt-controller;
> +   #interrupt-cells = <2>;
> +
> +   interrupt-parent = <>;
> +   interrupts = <12>;
> +   };
> +   };
> +
> +   uart0: serial@1003 {
> +   compatible = "ingenic,jz4770-uart";
> +   reg = <0x1003 0x100>;
> +
> +   clocks = <>, < JZ4770_CLK_UART0>;
> +   clock-names = "baud", "module";
> +
> +   interrupt-parent = <>;
> +   interrupts = <5>;
> +
> +   status = "disabled";
> +   };
> +
> +   uart1: serial@10031000 {
> +   compatible = "ingenic,jz4770-uart";
> +   reg = <0x10031000 0x100>;
> +
> +   clocks = <>, < JZ4770_CLK_UART1>;
> +   clock-names = "baud", "module";
> +
> +   interrupt-parent = <>;
> +   interrupts = <4>;
> +
> +   status = "disabled";
> +   };
> +
> +   uart2: serial@10032000 {
> +   compatible = "ingenic,jz4770-uart";
> +   reg = <0x10032000 0x100>;
> +
> +   clocks = <>, < JZ4770_CLK_UART2>;
> +   clock-names = "baud", "module";
> +
> +   interrupt-parent = <>;
> +   interrupts = <3>;
> +
> +   status = "disabled";
> +   };
> +
> +   uart3: serial@10033000 {
> +   compatible = "ingenic,jz4770-uart";
> +   reg = <0x10033000 0x100>;
> +
> +   clocks = <>, < JZ4770_CLK_UART3>;
> +   clock-names = "baud", "module";
> +
> +   interrupt-parent = <>;
> +   interrupts = <2>;
> +
> +   status = "disabled";
> +   };
> +
> +   uhc: uhc@1343 {
> +   compatible = "generic-ohci";
> +   reg = <0x1343 0x1000>;
> +
> +   clocks = < JZ4770_CLK_UHC>, < JZ4770_CLK_UHC_PHY>;
> +   assigned-clocks = < JZ4770_CLK_UHC>;
> +   assigned-clock-rates = <4800>;
> +
> +   interrupt-parent = <>;
> +   interrupts = <20>;
> +
> +   status = "disabled";
> +   };
> +};
> diff --git a/arch/mips/jz4740/Kconfig b/arch/mips/jz4740/Kconfig
> index 643af2012e14..29a9361a2b77 100644
> --- a/arch/mips/jz4740/Kconfig
> +++ b/arch/mips/jz4740/Kconfig
> @@ -18,6 +18,12 @@ config MACH_JZ4740
> bool
> select SYS_HAS_CPU_MIPS32_R1
>
> +config MACH_JZ4770
> +   bool
> +   select MIPS_CPU_SCACHE
> +   select SYS_HAS_CPU_MIPS32_R2
> +   select SYS_SUPPORTS_HIGHMEM
> +
>  config MACH_JZ4780
> bool
> select MIPS_CPU_SCACHE
> diff --git a/arch/mips/jz4740/time.c b/arch/mips/jz4740/time.c
> index bb1ad5119da4..2ca9160f642a 100644
> --- a/arch/mips/jz4740/time.c
> +++ b/arch/mips/jz4740/time.c
> @@ -113,7 +113,7 @@ static struct clock_event_device jz4740_clockevent = {
>  #ifdef CONFIG_MACH_JZ4740
> .irq = JZ4740_IRQ_TCU0,
>  #endif
> -#ifdef CONFIG_MACH_JZ4780
> +#if defined(CONFIG_MACH_JZ4770) || defined(CONFIG_MACH_JZ4780)
> .irq = JZ4780_IRQ_TCU2,
>  #endif
>  };
> --
> 2.11.0
>
>

Looks good to me.
Reviewed-by: PrasannaKumar Muralidharan 

Re: [PATCH v5 10/15] MIPS: ingenic: Add machine info for supported boards

[PrasannaKumar Muralidharan]

Hi Paul,

On 2 January 2018 at 20:38, Paul Cercueil  wrote:
> This makes sure that 'mips_machtype' will be initialized to the SoC
> version used on the board.
>
> Signed-off-by: Paul Cercueil 
> ---
>  arch/mips/Kconfig |  1 +
>  arch/mips/jz4740/Makefile |  2 +-
>  arch/mips/jz4740/boards.c | 12 
>  arch/mips/jz4740/setup.c  | 34 +-
>  4 files changed, 43 insertions(+), 6 deletions(-)
>  create mode 100644 arch/mips/jz4740/boards.c
>
>  v2: No change
>  v3: No change
>  v4: No change
>  v5: Use SPDX license identifier
>
> diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
> index 350a990fc719..83243e427e36 100644
> --- a/arch/mips/Kconfig
> +++ b/arch/mips/Kconfig
> @@ -376,6 +376,7 @@ config MACH_INGENIC
> select BUILTIN_DTB
> select USE_OF
> select LIBFDT
> +   select MIPS_MACHINE
>
>  config LANTIQ
> bool "Lantiq based platforms"
> diff --git a/arch/mips/jz4740/Makefile b/arch/mips/jz4740/Makefile
> index 88d6aa7d000b..fc2d3b3c4a80 100644
> --- a/arch/mips/jz4740/Makefile
> +++ b/arch/mips/jz4740/Makefile
> @@ -6,7 +6,7 @@
>  # Object file lists.
>
>  obj-y += prom.o time.o reset.o setup.o \
> -   platform.o timer.o
> +   platform.o timer.o boards.o
>
>  CFLAGS_setup.o = -I$(src)/../../../scripts/dtc/libfdt
>
> diff --git a/arch/mips/jz4740/boards.c b/arch/mips/jz4740/boards.c
> new file mode 100644
> index ..13b0bddd8cb7
> --- /dev/null
> +++ b/arch/mips/jz4740/boards.c
> @@ -0,0 +1,12 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Ingenic boards support
> + * Copyright 2017, Paul Cercueil 
> + */
> +
> +#include 
> +#include 
> +
> +MIPS_MACHINE(MACH_INGENIC_JZ4740, "qi,lb60", "Qi Hardware Ben Nanonote", 
> NULL);
> +MIPS_MACHINE(MACH_INGENIC_JZ4780, "img,ci20",
> +   "Imagination Technologies CI20", NULL);
> diff --git a/arch/mips/jz4740/setup.c b/arch/mips/jz4740/setup.c
> index 6d0152321819..afd84ee966e8 100644
> --- a/arch/mips/jz4740/setup.c
> +++ b/arch/mips/jz4740/setup.c
> @@ -22,6 +22,7 @@
>  #include 
>
>  #include 
> +#include 
>  #include 
>
>  #include 
> @@ -53,16 +54,34 @@ static void __init jz4740_detect_mem(void)
> add_memory_region(0, size, BOOT_MEM_RAM);
>  }
>
> +static unsigned long __init get_board_mach_type(const void *fdt)
> +{
> +   const struct mips_machine *mach;
> +
> +   for (mach = (struct mips_machine *)&__mips_machines_start;
> +   mach < (struct mips_machine *)&__mips_machines_end;
> +   mach++) {
> +   if (!fdt_node_check_compatible(fdt, 0, mach->mach_id))
> +   return mach->mach_type;
> +   }
> +
> +   return MACH_INGENIC_JZ4740;
> +}
> +
>  void __init plat_mem_setup(void)
>  {
> int offset;
>
> +   if (!early_init_dt_scan(__dtb_start))
> +   return;
> +
> jz4740_reset_init();
> -   __dt_setup_arch(__dtb_start);
>
> offset = fdt_path_offset(__dtb_start, "/memory");
> if (offset < 0)
> jz4740_detect_mem();
> +
> +   mips_machtype = get_board_mach_type(__dtb_start);
>  }
>
>  void __init device_tree_init(void)
> @@ -75,13 +94,18 @@ void __init device_tree_init(void)
>
>  const char *get_system_type(void)
>  {
> -   if (IS_ENABLED(CONFIG_MACH_JZ4780))
> -   return "JZ4780";
> -
> -   return "JZ4740";
> +   return mips_get_machine_name();
>  }
>
>  void __init arch_init_irq(void)
>  {
> irqchip_init();
>  }
> +
> +static int __init jz4740_machine_setup(void)
> +{
> +   mips_machine_setup();
> +
> +   return 0;
> +}
> +arch_initcall(jz4740_machine_setup);
> --
> 2.11.0
>
>

Why add another file in arch/mips/jz4740/? I think declaring a machine
and compatible string in dts would suffice. Please feel free to
correct me if I am wrong.

Regards,
PrasannaKumar

Re: [PATCH v5 10/15] MIPS: ingenic: Add machine info for supported boards

[PrasannaKumar Muralidharan]

Hi Paul,

On 2 January 2018 at 20:38, Paul Cercueil  wrote:
> This makes sure that 'mips_machtype' will be initialized to the SoC
> version used on the board.
>
> Signed-off-by: Paul Cercueil 
> ---
>  arch/mips/Kconfig |  1 +
>  arch/mips/jz4740/Makefile |  2 +-
>  arch/mips/jz4740/boards.c | 12 
>  arch/mips/jz4740/setup.c  | 34 +-
>  4 files changed, 43 insertions(+), 6 deletions(-)
>  create mode 100644 arch/mips/jz4740/boards.c
>
>  v2: No change
>  v3: No change
>  v4: No change
>  v5: Use SPDX license identifier
>
> diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
> index 350a990fc719..83243e427e36 100644
> --- a/arch/mips/Kconfig
> +++ b/arch/mips/Kconfig
> @@ -376,6 +376,7 @@ config MACH_INGENIC
> select BUILTIN_DTB
> select USE_OF
> select LIBFDT
> +   select MIPS_MACHINE
>
>  config LANTIQ
> bool "Lantiq based platforms"
> diff --git a/arch/mips/jz4740/Makefile b/arch/mips/jz4740/Makefile
> index 88d6aa7d000b..fc2d3b3c4a80 100644
> --- a/arch/mips/jz4740/Makefile
> +++ b/arch/mips/jz4740/Makefile
> @@ -6,7 +6,7 @@
>  # Object file lists.
>
>  obj-y += prom.o time.o reset.o setup.o \
> -   platform.o timer.o
> +   platform.o timer.o boards.o
>
>  CFLAGS_setup.o = -I$(src)/../../../scripts/dtc/libfdt
>
> diff --git a/arch/mips/jz4740/boards.c b/arch/mips/jz4740/boards.c
> new file mode 100644
> index ..13b0bddd8cb7
> --- /dev/null
> +++ b/arch/mips/jz4740/boards.c
> @@ -0,0 +1,12 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Ingenic boards support
> + * Copyright 2017, Paul Cercueil 
> + */
> +
> +#include 
> +#include 
> +
> +MIPS_MACHINE(MACH_INGENIC_JZ4740, "qi,lb60", "Qi Hardware Ben Nanonote", 
> NULL);
> +MIPS_MACHINE(MACH_INGENIC_JZ4780, "img,ci20",
> +   "Imagination Technologies CI20", NULL);
> diff --git a/arch/mips/jz4740/setup.c b/arch/mips/jz4740/setup.c
> index 6d0152321819..afd84ee966e8 100644
> --- a/arch/mips/jz4740/setup.c
> +++ b/arch/mips/jz4740/setup.c
> @@ -22,6 +22,7 @@
>  #include 
>
>  #include 
> +#include 
>  #include 
>
>  #include 
> @@ -53,16 +54,34 @@ static void __init jz4740_detect_mem(void)
> add_memory_region(0, size, BOOT_MEM_RAM);
>  }
>
> +static unsigned long __init get_board_mach_type(const void *fdt)
> +{
> +   const struct mips_machine *mach;
> +
> +   for (mach = (struct mips_machine *)&__mips_machines_start;
> +   mach < (struct mips_machine *)&__mips_machines_end;
> +   mach++) {
> +   if (!fdt_node_check_compatible(fdt, 0, mach->mach_id))
> +   return mach->mach_type;
> +   }
> +
> +   return MACH_INGENIC_JZ4740;
> +}
> +
>  void __init plat_mem_setup(void)
>  {
> int offset;
>
> +   if (!early_init_dt_scan(__dtb_start))
> +   return;
> +
> jz4740_reset_init();
> -   __dt_setup_arch(__dtb_start);
>
> offset = fdt_path_offset(__dtb_start, "/memory");
> if (offset < 0)
> jz4740_detect_mem();
> +
> +   mips_machtype = get_board_mach_type(__dtb_start);
>  }
>
>  void __init device_tree_init(void)
> @@ -75,13 +94,18 @@ void __init device_tree_init(void)
>
>  const char *get_system_type(void)
>  {
> -   if (IS_ENABLED(CONFIG_MACH_JZ4780))
> -   return "JZ4780";
> -
> -   return "JZ4740";
> +   return mips_get_machine_name();
>  }
>
>  void __init arch_init_irq(void)
>  {
> irqchip_init();
>  }
> +
> +static int __init jz4740_machine_setup(void)
> +{
> +   mips_machine_setup();
> +
> +   return 0;
> +}
> +arch_initcall(jz4740_machine_setup);
> --
> 2.11.0
>
>

Why add another file in arch/mips/jz4740/? I think declaring a machine
and compatible string in dts would suffice. Please feel free to
correct me if I am wrong.

Regards,
PrasannaKumar

Re: [PATCH v5 09/15] MIPS: platform: add machtype IDs for more Ingenic SoCs

[PrasannaKumar Muralidharan]

On 2 January 2018 at 20:38, Paul Cercueil <p...@crapouillou.net> wrote:
> Add a machtype ID for the JZ4780 SoC, which was missing, and one for the
> newly supported JZ4770 SoC.
>
> Signed-off-by: Paul Cercueil <p...@crapouillou.net>
> ---
>  arch/mips/include/asm/bootinfo.h | 2 ++
>  1 file changed, 2 insertions(+)
>
>  v2: No change
>  v3: No change
>  v5: No change
>
> diff --git a/arch/mips/include/asm/bootinfo.h 
> b/arch/mips/include/asm/bootinfo.h
> index e26a093bb17a..a301a8f4bc66 100644
> --- a/arch/mips/include/asm/bootinfo.h
> +++ b/arch/mips/include/asm/bootinfo.h
> @@ -79,6 +79,8 @@ enum loongson_machine_type {
>   */
>  #define  MACH_INGENIC_JZ4730   0   /* JZ4730 SOC   */
>  #define  MACH_INGENIC_JZ4740   1   /* JZ4740 SOC   */
> +#define  MACH_INGENIC_JZ4770   2   /* JZ4770 SOC   */
> +#define  MACH_INGENIC_JZ4780   3   /* JZ4780 SOC   */
>
>  extern char *system_type;
>  const char *get_system_type(void);
> --
> 2.11.0
>
>

Reviewed-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>

Re: [PATCH v5 09/15] MIPS: platform: add machtype IDs for more Ingenic SoCs

[PrasannaKumar Muralidharan]

On 2 January 2018 at 20:38, Paul Cercueil  wrote:
> Add a machtype ID for the JZ4780 SoC, which was missing, and one for the
> newly supported JZ4770 SoC.
>
> Signed-off-by: Paul Cercueil 
> ---
>  arch/mips/include/asm/bootinfo.h | 2 ++
>  1 file changed, 2 insertions(+)
>
>  v2: No change
>  v3: No change
>  v5: No change
>
> diff --git a/arch/mips/include/asm/bootinfo.h 
> b/arch/mips/include/asm/bootinfo.h
> index e26a093bb17a..a301a8f4bc66 100644
> --- a/arch/mips/include/asm/bootinfo.h
> +++ b/arch/mips/include/asm/bootinfo.h
> @@ -79,6 +79,8 @@ enum loongson_machine_type {
>   */
>  #define  MACH_INGENIC_JZ4730   0   /* JZ4730 SOC   */
>  #define  MACH_INGENIC_JZ4740   1   /* JZ4740 SOC   */
> +#define  MACH_INGENIC_JZ4770   2   /* JZ4770 SOC   */
> +#define  MACH_INGENIC_JZ4780   3   /* JZ4780 SOC   */
>
>  extern char *system_type;
>  const char *get_system_type(void);
> --
> 2.11.0
>
>

Reviewed-by: PrasannaKumar Muralidharan 

Re: [PATCH v5 08/15] MIPS: ingenic: Use common cmdline handling code

[PrasannaKumar Muralidharan]

Hi Paul,

On 2 January 2018 at 20:38, Paul Cercueil <p...@crapouillou.net> wrote:
> From: Paul Burton <paul.bur...@imgtec.com>
>
> jz4740_init_cmdline appends all arguments from argv (in fw_arg1) to
> arcs_cmdline, up to argc (in fw_arg0). The common code in
> fw_init_cmdline will do the exact same thing when run on a system where
> fw_arg0 isn't a pointer to kseg0 (it'll also set _fw_envp but we don't
> use it). Remove the custom implementation & use the generic code.
>
> Signed-off-by: Paul Burton <paul.bur...@imgtec.com>
> ---
>  arch/mips/jz4740/prom.c | 24 ++--
>  1 file changed, 2 insertions(+), 22 deletions(-)
>
>  v2: No change
>  v3: No change
>  v4: No change
>  v5: No change
>
> diff --git a/arch/mips/jz4740/prom.c b/arch/mips/jz4740/prom.c
> index 47e857194ce6..a62dd8e6ecf9 100644
> --- a/arch/mips/jz4740/prom.c
> +++ b/arch/mips/jz4740/prom.c
> @@ -20,33 +20,13 @@
>  #include 
>
>  #include 
> +#include 
>  #include 
>
> -static __init void jz4740_init_cmdline(int argc, char *argv[])
> -{
> -   unsigned int count = COMMAND_LINE_SIZE - 1;
> -   int i;
> -   char *dst = &(arcs_cmdline[0]);
> -   char *src;
> -
> -   for (i = 1; i < argc && count; ++i) {
> -   src = argv[i];
> -   while (*src && count) {
> -   *dst++ = *src++;
> -   --count;
> -   }
> -   *dst++ = ' ';
> -   }
> -   if (i > 1)
> -   --dst;
> -
> -   *dst = 0;
> -}
> -
>  void __init prom_init(void)
>  {
> -   jz4740_init_cmdline((int)fw_arg0, (char **)fw_arg1);
> mips_machtype = MACH_INGENIC_JZ4740;
> +   fw_init_cmdline();
>  }
>
>  void __init prom_free_prom_memory(void)
> --
> 2.11.0
>
>

Looks good to me.
Reviewed-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>

Re: [PATCH v5 08/15] MIPS: ingenic: Use common cmdline handling code

[PrasannaKumar Muralidharan]

Hi Paul,

On 2 January 2018 at 20:38, Paul Cercueil  wrote:
> From: Paul Burton 
>
> jz4740_init_cmdline appends all arguments from argv (in fw_arg1) to
> arcs_cmdline, up to argc (in fw_arg0). The common code in
> fw_init_cmdline will do the exact same thing when run on a system where
> fw_arg0 isn't a pointer to kseg0 (it'll also set _fw_envp but we don't
> use it). Remove the custom implementation & use the generic code.
>
> Signed-off-by: Paul Burton 
> ---
>  arch/mips/jz4740/prom.c | 24 ++--
>  1 file changed, 2 insertions(+), 22 deletions(-)
>
>  v2: No change
>  v3: No change
>  v4: No change
>  v5: No change
>
> diff --git a/arch/mips/jz4740/prom.c b/arch/mips/jz4740/prom.c
> index 47e857194ce6..a62dd8e6ecf9 100644
> --- a/arch/mips/jz4740/prom.c
> +++ b/arch/mips/jz4740/prom.c
> @@ -20,33 +20,13 @@
>  #include 
>
>  #include 
> +#include 
>  #include 
>
> -static __init void jz4740_init_cmdline(int argc, char *argv[])
> -{
> -   unsigned int count = COMMAND_LINE_SIZE - 1;
> -   int i;
> -   char *dst = &(arcs_cmdline[0]);
> -   char *src;
> -
> -   for (i = 1; i < argc && count; ++i) {
> -   src = argv[i];
> -   while (*src && count) {
> -   *dst++ = *src++;
> -   --count;
> -   }
> -   *dst++ = ' ';
> -   }
> -   if (i > 1)
> -   --dst;
> -
> -   *dst = 0;
> -}
> -
>  void __init prom_init(void)
>  {
> -   jz4740_init_cmdline((int)fw_arg0, (char **)fw_arg1);
> mips_machtype = MACH_INGENIC_JZ4740;
> +   fw_init_cmdline();
>  }
>
>  void __init prom_free_prom_memory(void)
> --
> 2.11.0
>
>

Looks good to me.
Reviewed-by: PrasannaKumar Muralidharan 

Re: [PATCH] [RFT] crypto: aes-generic - turn off -ftree-pre and -ftree-sra

[PrasannaKumar Muralidharan]

Hi Ard,

On 21 December 2017 at 17:52, Ard Biesheuvel  wrote:
> On 21 December 2017 at 10:20, Arnd Bergmann  wrote:
>> On Wed, Dec 20, 2017 at 10:46 PM, Jakub Jelinek  wrote:
>>> On Wed, Dec 20, 2017 at 09:52:05PM +0100, Arnd Bergmann wrote:
 diff --git a/crypto/aes_generic.c b/crypto/aes_generic.c
 index ca554d57d01e..35f973ba9878 100644
 --- a/crypto/aes_generic.c
 +++ b/crypto/aes_generic.c
 @@ -1331,6 +1331,20 @@ EXPORT_SYMBOL_GPL(crypto_aes_set_key);
   f_rl(bo, bi, 3, k); \
  } while (0)

 +#if __GNUC__ >= 7
 +/*
 + * Newer compilers try to optimize integer arithmetic more aggressively,
 + * which generally improves code quality a lot, but in this specific case
 + * ends up hurting more than it helps, in some configurations drastically
 + * so. This turns off two optimization steps that have been shown to
 + * lead to rather badly optimized code with gcc-7.
 + *
 + * See also https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83356
 + */
 +#pragma GCC optimize("-fno-tree-pre")
 +#pragma GCC optimize("-fno-tree-sra")
>>>
>>> So do it only when UBSAN is enabled?  GCC doesn't have a particular
>>> predefined macro for those (only for asan and tsan), but either the kernel
>>> does have something already, or could have something added in the
>>> corresponding Makefile.
>>
>> My original interpretation of the resulting object code suggested that 
>> disabling
>> those two optimizations produced better results for this particular
>> file even without
>> UBSAN, on both gcc-7 and gcc-8 (but not gcc-6), so my patch might have
>> been better, but I did some measurements now as Ard suggested, showing
>> cycles/byte for AES256/CBC with 8KB blocks:
>>
>>
>>default  ubsan patchedpatched+ubsan
>> gcc-4.3.614.9   14.9 
>> gcc-4.6.415.0   15.8 
>> gcc-4.9.415.520.7   15.9 20.9
>> gcc-5.5.015.647.3   86.4 48.8
>> gcc-6.3.114.649.4   94.3 50.9
>> gcc-7.1.113.554.6   15.2 52.0
>> gcc-7.2.116.8   124.7   92.0 52.2
>> gcc-8.0.015.0  no boot  15.3no boot
>>
>> I checked that there are actually three significant digits on the 
>> measurements,
>> detailed output is available at https://pastebin.com/eFsWYjQp
>>
>> It seems that I was wrong about the interpretation that disabling
>> the optimization would be a win on gcc-7 and higher, it almost
>> always makes things worse even with UBSAN. Making that
>> check "#if __GNUC__ == 7 && IS_ENABLED(CONFIG_UBSAN_SANITIZE_ALL)"
>> would help here, or we could list the file as an exception for
>> UBSAN and never sanitize it.
>>
>> Looking at the 'default' column, I wonder if anyone would be interested
>> in looking at why the throughput regressed with gcc-7.2 and gcc-8.
>>
>
> Thanks for the elaborate benchmarks. Looking at the bugzilla entry, it
> appears the UBSAN code inserts runtime checks to ensure that certain
> u8 variables don't assume values <0 or >255, which seems like a rather
> pointless exercise to me. But even if it didn't, I think it is
> justified to disable UBSAN on all of the low-level cipher
> implementations, given that they are hardly ever modified, and
> typically don't suffer from the issues UBSAN tries to identify.
>
> So my vote is to disable UBSAN for all such cipher implementations:
> aes_generic, but also aes_ti, which has a similar 256 byte lookup
> table [although it does not seem to be affected by the same issue as
> aes_generic], and possibly others as well.
>
> Perhaps it makes sense to move core cipher code into a separate
> sub-directory, and disable UBSAN at the directory level?
>
> It would involve the following files
>
> crypto/aes_generic.c
> crypto/aes_ti.c
> crypto/anubis.c
> crypto/arc4.c
> crypto/blowfish_generic.c
> crypto/camellia_generic.c
> crypto/cast5_generic.c
> crypto/cast6_generic.c
> crypto/des_generic.c
> crypto/fcrypt.c
> crypto/khazad.c
> crypto/seed.c
> crypto/serpent_generic.c
> crypto/tea.c
> crypto/twofish_generic.c

As *SAN is enabled only on developer setup, is such a change required?
Looks like I am missing something here. Can you explain what value it
provides?

Regards,
PrasannaKumar

Re: [PATCH] [RFT] crypto: aes-generic - turn off -ftree-pre and -ftree-sra

[PrasannaKumar Muralidharan]

Hi Ard,

On 21 December 2017 at 17:52, Ard Biesheuvel  wrote:
> On 21 December 2017 at 10:20, Arnd Bergmann  wrote:
>> On Wed, Dec 20, 2017 at 10:46 PM, Jakub Jelinek  wrote:
>>> On Wed, Dec 20, 2017 at 09:52:05PM +0100, Arnd Bergmann wrote:
 diff --git a/crypto/aes_generic.c b/crypto/aes_generic.c
 index ca554d57d01e..35f973ba9878 100644
 --- a/crypto/aes_generic.c
 +++ b/crypto/aes_generic.c
 @@ -1331,6 +1331,20 @@ EXPORT_SYMBOL_GPL(crypto_aes_set_key);
   f_rl(bo, bi, 3, k); \
  } while (0)

 +#if __GNUC__ >= 7
 +/*
 + * Newer compilers try to optimize integer arithmetic more aggressively,
 + * which generally improves code quality a lot, but in this specific case
 + * ends up hurting more than it helps, in some configurations drastically
 + * so. This turns off two optimization steps that have been shown to
 + * lead to rather badly optimized code with gcc-7.
 + *
 + * See also https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83356
 + */
 +#pragma GCC optimize("-fno-tree-pre")
 +#pragma GCC optimize("-fno-tree-sra")
>>>
>>> So do it only when UBSAN is enabled?  GCC doesn't have a particular
>>> predefined macro for those (only for asan and tsan), but either the kernel
>>> does have something already, or could have something added in the
>>> corresponding Makefile.
>>
>> My original interpretation of the resulting object code suggested that 
>> disabling
>> those two optimizations produced better results for this particular
>> file even without
>> UBSAN, on both gcc-7 and gcc-8 (but not gcc-6), so my patch might have
>> been better, but I did some measurements now as Ard suggested, showing
>> cycles/byte for AES256/CBC with 8KB blocks:
>>
>>
>>default  ubsan patchedpatched+ubsan
>> gcc-4.3.614.9   14.9 
>> gcc-4.6.415.0   15.8 
>> gcc-4.9.415.520.7   15.9 20.9
>> gcc-5.5.015.647.3   86.4 48.8
>> gcc-6.3.114.649.4   94.3 50.9
>> gcc-7.1.113.554.6   15.2 52.0
>> gcc-7.2.116.8   124.7   92.0 52.2
>> gcc-8.0.015.0  no boot  15.3no boot
>>
>> I checked that there are actually three significant digits on the 
>> measurements,
>> detailed output is available at https://pastebin.com/eFsWYjQp
>>
>> It seems that I was wrong about the interpretation that disabling
>> the optimization would be a win on gcc-7 and higher, it almost
>> always makes things worse even with UBSAN. Making that
>> check "#if __GNUC__ == 7 && IS_ENABLED(CONFIG_UBSAN_SANITIZE_ALL)"
>> would help here, or we could list the file as an exception for
>> UBSAN and never sanitize it.
>>
>> Looking at the 'default' column, I wonder if anyone would be interested
>> in looking at why the throughput regressed with gcc-7.2 and gcc-8.
>>
>
> Thanks for the elaborate benchmarks. Looking at the bugzilla entry, it
> appears the UBSAN code inserts runtime checks to ensure that certain
> u8 variables don't assume values <0 or >255, which seems like a rather
> pointless exercise to me. But even if it didn't, I think it is
> justified to disable UBSAN on all of the low-level cipher
> implementations, given that they are hardly ever modified, and
> typically don't suffer from the issues UBSAN tries to identify.
>
> So my vote is to disable UBSAN for all such cipher implementations:
> aes_generic, but also aes_ti, which has a similar 256 byte lookup
> table [although it does not seem to be affected by the same issue as
> aes_generic], and possibly others as well.
>
> Perhaps it makes sense to move core cipher code into a separate
> sub-directory, and disable UBSAN at the directory level?
>
> It would involve the following files
>
> crypto/aes_generic.c
> crypto/aes_ti.c
> crypto/anubis.c
> crypto/arc4.c
> crypto/blowfish_generic.c
> crypto/camellia_generic.c
> crypto/cast5_generic.c
> crypto/cast6_generic.c
> crypto/des_generic.c
> crypto/fcrypt.c
> crypto/khazad.c
> crypto/seed.c
> crypto/serpent_generic.c
> crypto/tea.c
> crypto/twofish_generic.c

As *SAN is enabled only on developer setup, is such a change required?
Looks like I am missing something here. Can you explain what value it
provides?

Regards,
PrasannaKumar

Re: [PATCH v2 09/13] MIPS: mscc: Add initial support for Microsemi MIPS SoCs

[PrasannaKumar Muralidharan]

Hi Alexandre,

On 20 December 2017 at 01:39, Alexandre Belloni
<alexandre.bell...@free-electrons.com> wrote:
> Hi,
>
> On 19/12/2017 at 20:27:02 +0530, PrasannaKumar Muralidharan wrote:
>> Given the fact that setup code is very small and most of it is generic
>> code I strongly believe that it is plausible to make use of generic
>> code completely. Please have a look at [1] and [2].
>>
>> 1. https://patchwork.kernel.org/patch/9655699/
>> 2. https://patchwork.kernel.org/patch/9655697/
>>
>> PS: My rb tag stays if this could not be done immediately.
>>
>
> I think we had that discussion on the previous version:
> https://www.linux-mips.org/archives/linux-mips/2017-11/msg00532.html
>
> I can't test on the sead3 so I'd prefer not changing its code right now.
>
> --
> Alexandre Belloni, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

Sorry I missed it. Your v1 did not show up in my mailbox somehow even
though I am subscribed to linux-mips mailing list. Hope generic code
can be used in future.

Regards,
PrasannaKumar

Re: [PATCH v2 09/13] MIPS: mscc: Add initial support for Microsemi MIPS SoCs

[PrasannaKumar Muralidharan]

Hi Alexandre,

On 20 December 2017 at 01:39, Alexandre Belloni
 wrote:
> Hi,
>
> On 19/12/2017 at 20:27:02 +0530, PrasannaKumar Muralidharan wrote:
>> Given the fact that setup code is very small and most of it is generic
>> code I strongly believe that it is plausible to make use of generic
>> code completely. Please have a look at [1] and [2].
>>
>> 1. https://patchwork.kernel.org/patch/9655699/
>> 2. https://patchwork.kernel.org/patch/9655697/
>>
>> PS: My rb tag stays if this could not be done immediately.
>>
>
> I think we had that discussion on the previous version:
> https://www.linux-mips.org/archives/linux-mips/2017-11/msg00532.html
>
> I can't test on the sead3 so I'd prefer not changing its code right now.
>
> --
> Alexandre Belloni, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

Sorry I missed it. Your v1 did not show up in my mailbox somehow even
though I am subscribed to linux-mips mailing list. Hope generic code
can be used in future.

Regards,
PrasannaKumar

Re: [PATCH v2 09/13] MIPS: mscc: Add initial support for Microsemi MIPS SoCs

[PrasannaKumar Muralidharan]

strcpy(arcs_cmdline, prom_argv[1]);
> +   }
> +}
> +
> +void __init prom_free_prom_memory(void)
> +{
> +}
> +
> +unsigned int get_c0_compare_int(void)
> +{
> +   return CP0_LEGACY_COMPARE_IRQ;
> +}
> +
> +void __init plat_time_init(void)
> +{
> +   struct device_node *np;
> +   u32 freq;
> +
> +   np = of_find_node_by_name(NULL, "cpus");
> +   if (!np)
> +   panic("missing 'cpus' DT node");
> +   if (of_property_read_u32(np, "mips-hpt-frequency", ) < 0)
> +   panic("missing 'mips-hpt-frequency' property");
> +   of_node_put(np);
> +
> +   mips_hpt_frequency = freq;
> +}
> +
> +void __init arch_init_irq(void)
> +{
> +   irqchip_init();
> +}
> +
> +const char *get_system_type(void)
> +{
> +   return "Microsemi Ocelot";
> +}
> +
> +static void __init ocelot_late_init(void)
> +{
> +   ocelot_earlyprintk_init();
> +}
> +
> +extern void (*late_time_init)(void);
> +
> +void __init plat_mem_setup(void)
> +{
> +   /* This has to be done so late because ioremap needs to work */
> +   late_time_init = ocelot_late_init;
> +
> +   __dt_setup_arch(__dtb_start);
> +}
> +
> +void __init device_tree_init(void)
> +{
> +   if (!initial_boot_params)
> +   return;
> +
> +   unflatten_and_copy_device_tree();
> +}
> +
> +static int __init populate_machine(void)
> +{
> +   of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
> +   return 0;
> +}
> +arch_initcall(populate_machine);
> --
> 2.15.1
>
>

Given the fact that setup code is very small and most of it is generic
code I strongly believe that it is plausible to make use of generic
code completely. Please have a look at [1] and [2].

1. https://patchwork.kernel.org/patch/9655699/
2. https://patchwork.kernel.org/patch/9655697/

PS: My rb tag stays if this could not be done immediately.

Regards,
PrasannaKumar Muralidharan

Re: [PATCH v2 09/13] MIPS: mscc: Add initial support for Microsemi MIPS SoCs

[PrasannaKumar Muralidharan]

 __init prom_free_prom_memory(void)
> +{
> +}
> +
> +unsigned int get_c0_compare_int(void)
> +{
> +   return CP0_LEGACY_COMPARE_IRQ;
> +}
> +
> +void __init plat_time_init(void)
> +{
> +   struct device_node *np;
> +   u32 freq;
> +
> +   np = of_find_node_by_name(NULL, "cpus");
> +   if (!np)
> +   panic("missing 'cpus' DT node");
> +   if (of_property_read_u32(np, "mips-hpt-frequency", ) < 0)
> +   panic("missing 'mips-hpt-frequency' property");
> +   of_node_put(np);
> +
> +   mips_hpt_frequency = freq;
> +}
> +
> +void __init arch_init_irq(void)
> +{
> +   irqchip_init();
> +}
> +
> +const char *get_system_type(void)
> +{
> +   return "Microsemi Ocelot";
> +}
> +
> +static void __init ocelot_late_init(void)
> +{
> +   ocelot_earlyprintk_init();
> +}
> +
> +extern void (*late_time_init)(void);
> +
> +void __init plat_mem_setup(void)
> +{
> +   /* This has to be done so late because ioremap needs to work */
> +   late_time_init = ocelot_late_init;
> +
> +   __dt_setup_arch(__dtb_start);
> +}
> +
> +void __init device_tree_init(void)
> +{
> +   if (!initial_boot_params)
> +   return;
> +
> +   unflatten_and_copy_device_tree();
> +}
> +
> +static int __init populate_machine(void)
> +{
> +   of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
> +   return 0;
> +}
> +arch_initcall(populate_machine);
> --
> 2.15.1
>
>

Given the fact that setup code is very small and most of it is generic
code I strongly believe that it is plausible to make use of generic
code completely. Please have a look at [1] and [2].

1. https://patchwork.kernel.org/patch/9655699/
2. https://patchwork.kernel.org/patch/9655697/

PS: My rb tag stays if this could not be done immediately.

Regards,
PrasannaKumar Muralidharan

Re: [PATCH v2 11/13] MIPS: mscc: add ocelot PCB123 device tree

[PrasannaKumar Muralidharan]

On 8 December 2017 at 21:16, Alexandre Belloni
<alexandre.bell...@free-electrons.com> wrote:
> Add a device tree for the Microsemi Ocelot PCB123 evaluation board.
>
> Signed-off-by: Alexandre Belloni <alexandre.bell...@free-electrons.com>
> ---
>  arch/mips/boot/dts/mscc/Makefile  |  2 ++
>  arch/mips/boot/dts/mscc/ocelot_pcb123.dts | 27 +++
>  2 files changed, 29 insertions(+)
>  create mode 100644 arch/mips/boot/dts/mscc/ocelot_pcb123.dts
>
> diff --git a/arch/mips/boot/dts/mscc/Makefile 
> b/arch/mips/boot/dts/mscc/Makefile
> index f0a155a74e02..09a1c4b97de2 100644
> --- a/arch/mips/boot/dts/mscc/Makefile
> +++ b/arch/mips/boot/dts/mscc/Makefile
> @@ -1,3 +1,5 @@
> +dtb-$(CONFIG_MSCC_OCELOT)  += ocelot_pcb123.dtb
> +
>  obj-y  += $(patsubst %.dtb, %.dtb.o, $(dtb-y))
>
>  # Force kbuild to make empty built-in.o if necessary
> diff --git a/arch/mips/boot/dts/mscc/ocelot_pcb123.dts 
> b/arch/mips/boot/dts/mscc/ocelot_pcb123.dts
> new file mode 100644
> index ..42bd404471f6
> --- /dev/null
> +++ b/arch/mips/boot/dts/mscc/ocelot_pcb123.dts
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */
> +/* Copyright (c) 2017 Microsemi Corporation */
> +
> +/dts-v1/;
> +
> +#include "ocelot.dtsi"
> +
> +/ {
> +   compatible = "mscc,ocelot-pcb123", "mscc,ocelot";
> +
> +   chosen {
> +   stdout-path = "serial0:115200n8";
> +   };
> +
> +   memory {
> +   device_type = "memory";
> +   reg = <0x0 0x0e00>;
> +   };
> +};
> +
> + {
> +   status = "okay";
> +};
> +
> + {
> +   status = "okay";
> +};
> --
> 2.15.1
>
>

Looks good to me.
Reviewed-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>

Regards,
PrasannaKumar

Re: [PATCH v2 11/13] MIPS: mscc: add ocelot PCB123 device tree

[PrasannaKumar Muralidharan]

On 8 December 2017 at 21:16, Alexandre Belloni
 wrote:
> Add a device tree for the Microsemi Ocelot PCB123 evaluation board.
>
> Signed-off-by: Alexandre Belloni 
> ---
>  arch/mips/boot/dts/mscc/Makefile  |  2 ++
>  arch/mips/boot/dts/mscc/ocelot_pcb123.dts | 27 +++
>  2 files changed, 29 insertions(+)
>  create mode 100644 arch/mips/boot/dts/mscc/ocelot_pcb123.dts
>
> diff --git a/arch/mips/boot/dts/mscc/Makefile 
> b/arch/mips/boot/dts/mscc/Makefile
> index f0a155a74e02..09a1c4b97de2 100644
> --- a/arch/mips/boot/dts/mscc/Makefile
> +++ b/arch/mips/boot/dts/mscc/Makefile
> @@ -1,3 +1,5 @@
> +dtb-$(CONFIG_MSCC_OCELOT)  += ocelot_pcb123.dtb
> +
>  obj-y  += $(patsubst %.dtb, %.dtb.o, $(dtb-y))
>
>  # Force kbuild to make empty built-in.o if necessary
> diff --git a/arch/mips/boot/dts/mscc/ocelot_pcb123.dts 
> b/arch/mips/boot/dts/mscc/ocelot_pcb123.dts
> new file mode 100644
> index ..42bd404471f6
> --- /dev/null
> +++ b/arch/mips/boot/dts/mscc/ocelot_pcb123.dts
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */
> +/* Copyright (c) 2017 Microsemi Corporation */
> +
> +/dts-v1/;
> +
> +#include "ocelot.dtsi"
> +
> +/ {
> +   compatible = "mscc,ocelot-pcb123", "mscc,ocelot";
> +
> +   chosen {
> +   stdout-path = "serial0:115200n8";
> +   };
> +
> +   memory {
> +   device_type = "memory";
> +   reg = <0x0 0x0e00>;
> +   };
> +};
> +
> + {
> +   status = "okay";
> +};
> +
> + {
> +   status = "okay";
> +};
> --
> 2.15.1
>
>

Looks good to me.
Reviewed-by: PrasannaKumar Muralidharan 

Regards,
PrasannaKumar

Re: [PATCH v2 08/13] power: reset: Add a driver for the Microsemi Ocelot reset

[PrasannaKumar Muralidharan]

dev_err(dev, "can't register restart notifier (err=%d)\n", 
> err);
> +
> +   return err;
> +}
> +
> +static const struct of_device_id ocelot_reset_of_match[] = {
> +   { .compatible = "mscc,ocelot-chip-reset" },
> +   {}
> +};
> +
> +static struct platform_driver ocelot_reset_driver = {
> +   .probe = ocelot_reset_probe,
> +   .driver = {
> +   .name = "ocelot-chip-reset",
> +   .of_match_table = ocelot_reset_of_match,
> +   },
> +};
> +builtin_platform_driver(ocelot_reset_driver);
> --
> 2.15.1
>
>

Looks good to me.
Reviewed-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>

Regards,
PrasannaKumar

Re: [PATCH v2 08/13] power: reset: Add a driver for the Microsemi Ocelot reset

[PrasannaKumar Muralidharan]

t; +
> +static const struct of_device_id ocelot_reset_of_match[] = {
> +   { .compatible = "mscc,ocelot-chip-reset" },
> +   {}
> +};
> +
> +static struct platform_driver ocelot_reset_driver = {
> +   .probe = ocelot_reset_probe,
> +   .driver = {
> +   .name = "ocelot-chip-reset",
> +   .of_match_table = ocelot_reset_of_match,
> +   },
> +};
> +builtin_platform_driver(ocelot_reset_driver);
> --
> 2.15.1
>
>

Looks good to me.
Reviewed-by: PrasannaKumar Muralidharan 

Regards,
PrasannaKumar

Re: [PATCH v2 09/13] MIPS: mscc: Add initial support for Microsemi MIPS SoCs

[PrasannaKumar Muralidharan]

strcpy(arcs_cmdline, prom_argv[1]);
> +   }
> +}
> +
> +void __init prom_free_prom_memory(void)
> +{
> +}
> +
> +unsigned int get_c0_compare_int(void)
> +{
> +   return CP0_LEGACY_COMPARE_IRQ;
> +}
> +
> +void __init plat_time_init(void)
> +{
> +   struct device_node *np;
> +   u32 freq;
> +
> +   np = of_find_node_by_name(NULL, "cpus");
> +   if (!np)
> +   panic("missing 'cpus' DT node");
> +   if (of_property_read_u32(np, "mips-hpt-frequency", ) < 0)
> +   panic("missing 'mips-hpt-frequency' property");
> +   of_node_put(np);
> +
> +   mips_hpt_frequency = freq;
> +}
> +
> +void __init arch_init_irq(void)
> +{
> +   irqchip_init();
> +}
> +
> +const char *get_system_type(void)
> +{
> +   return "Microsemi Ocelot";
> +}
> +
> +static void __init ocelot_late_init(void)
> +{
> +   ocelot_earlyprintk_init();
> +}
> +
> +extern void (*late_time_init)(void);
> +
> +void __init plat_mem_setup(void)
> +{
> +   /* This has to be done so late because ioremap needs to work */
> +   late_time_init = ocelot_late_init;
> +
> +   __dt_setup_arch(__dtb_start);
> +}
> +
> +void __init device_tree_init(void)
> +{
> +   if (!initial_boot_params)
> +   return;
> +
> +   unflatten_and_copy_device_tree();
> +}
> +
> +static int __init populate_machine(void)
> +{
> +   of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
> +   return 0;
> +}
> +arch_initcall(populate_machine);
> --
> 2.15.1
>
>

Looks good to me.
Reviewed-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>

Regards,
PrasannaKumar

Re: [PATCH v2 10/13] MIPS: mscc: add ocelot dtsi

[PrasannaKumar Muralidharan]

ocks = <_clk>;
> +   reg-io-width = <4>;
> +   reg-shift = <2>;
> +
> +   status = "disabled";
> +   };
> +
> +   chip_regs: syscon@107 {
> +   compatible = "mscc,ocelot-chip-regs", "simple-mfd", 
> "syscon";
> +   reg = <0x107 0x1c>;
> +
> +   reset {
> +   compatible = "mscc,ocelot-chip-reset";
> +   mscc,cpucontrol = <_ctrl>;
> +   };
> +   };
> +
> +   gpio: pinctrl@1070034 {
> +   compatible = "mscc,ocelot-pinctrl";
> +   reg = <0x1070034 0x28>;
> +   gpio-controller;
> +   #gpio-cells = <2>;
> +   gpio-ranges = < 0 0 22>;
> +
> +   uart_pins: uart-pins {
> +   pins = "GPIO_6", "GPIO_7";
> +   function = "uart";
> +   };
> +
> +   uart2_pins: uart2-pins {
> +   pins = "GPIO_12", "GPIO_13";
> +   function = "uart2";
> +   };
> +   };
> +   };
> +};
> --
> 2.15.1
>
>

Looks good to me.
Reviewed-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>

Regards,
PrasannaKumar

Re: [PATCH v2 10/13] MIPS: mscc: add ocelot dtsi

[PrasannaKumar Muralidharan]

>;
> +   reg-shift = <2>;
> +
> +   status = "disabled";
> +   };
> +
> +   chip_regs: syscon@107 {
> +   compatible = "mscc,ocelot-chip-regs", "simple-mfd", 
> "syscon";
> +   reg = <0x107 0x1c>;
> +
> +   reset {
> +   compatible = "mscc,ocelot-chip-reset";
> +   mscc,cpucontrol = <_ctrl>;
> +   };
> +   };
> +
> +   gpio: pinctrl@1070034 {
> +   compatible = "mscc,ocelot-pinctrl";
> +   reg = <0x1070034 0x28>;
> +   gpio-controller;
> +   #gpio-cells = <2>;
> +   gpio-ranges = < 0 0 22>;
> +
> +   uart_pins: uart-pins {
> +   pins = "GPIO_6", "GPIO_7";
> +   function = "uart";
> +   };
> +
> +   uart2_pins: uart2-pins {
> +   pins = "GPIO_12", "GPIO_13";
> +   function = "uart2";
> +   };
> +   };
> +   };
> +};
> --
> 2.15.1
>
>

Looks good to me.
Reviewed-by: PrasannaKumar Muralidharan 

Regards,
PrasannaKumar

Re: [PATCH v2 09/13] MIPS: mscc: Add initial support for Microsemi MIPS SoCs

[PrasannaKumar Muralidharan]

 __init prom_free_prom_memory(void)
> +{
> +}
> +
> +unsigned int get_c0_compare_int(void)
> +{
> +   return CP0_LEGACY_COMPARE_IRQ;
> +}
> +
> +void __init plat_time_init(void)
> +{
> +   struct device_node *np;
> +   u32 freq;
> +
> +   np = of_find_node_by_name(NULL, "cpus");
> +   if (!np)
> +   panic("missing 'cpus' DT node");
> +   if (of_property_read_u32(np, "mips-hpt-frequency", ) < 0)
> +   panic("missing 'mips-hpt-frequency' property");
> +   of_node_put(np);
> +
> +   mips_hpt_frequency = freq;
> +}
> +
> +void __init arch_init_irq(void)
> +{
> +   irqchip_init();
> +}
> +
> +const char *get_system_type(void)
> +{
> +   return "Microsemi Ocelot";
> +}
> +
> +static void __init ocelot_late_init(void)
> +{
> +   ocelot_earlyprintk_init();
> +}
> +
> +extern void (*late_time_init)(void);
> +
> +void __init plat_mem_setup(void)
> +{
> +   /* This has to be done so late because ioremap needs to work */
> +   late_time_init = ocelot_late_init;
> +
> +   __dt_setup_arch(__dtb_start);
> +}
> +
> +void __init device_tree_init(void)
> +{
> +   if (!initial_boot_params)
> +   return;
> +
> +   unflatten_and_copy_device_tree();
> +}
> +
> +static int __init populate_machine(void)
> +{
> +   of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
> +   return 0;
> +}
> +arch_initcall(populate_machine);
> --
> 2.15.1
>
>

Looks good to me.
Reviewed-by: PrasannaKumar Muralidharan 

Regards,
PrasannaKumar

Re: [PATCH v2 00/13] MIPS: add support for the Microsemi MIPS SoCs

[PrasannaKumar Muralidharan]

Hi Alexandre,

With very small amount of code in arch/mips this series looks really nice.

On 8 December 2017 at 21:16, Alexandre Belloni
<alexandre.bell...@free-electrons.com> wrote:
> Hi,
>
> This patch series adds initial support for the Microsemi MIPS SoCs. It
> is currently focusing on the Microsemi Ocelot (VSC7513, VSC7514).
>
> It adds support for the IRQ controller, pinmux and gpio controller and
> reset control.
>
> This produces a kernel that can boot to the console.
>
> This is a single series for reference but it can also be taken
> separately by each maintainer as each drivers are independant.
>
> Changes in v2:
>  - removed the wildcard in MAINAINERS
>  - corrected the Cc list
>  - added proper documentation for both syscons
>  - removed the mscc,cpucontrol property
>  - updated the ranges property in the ocelot dtsi
>
> Cc: Rob Herring <robh...@kernel.org>
> Cc: devicet...@vger.kernel.org
> Cc: Thomas Gleixner <t...@linutronix.de>
> Cc: Jason Cooper <ja...@lakedaemon.net>
> Cc: Linus Walleij <linus.wall...@linaro.org>
> Cc: linux-g...@vger.kernel.org
> Cc: Sebastian Reichel <s...@kernel.org>
> Cc: linux...@vger.kernel.org
>
>
> Alexandre Belloni (13):
>   dt-bindings: Add vendor prefix for Microsemi Corporation
>   dt-bindings: interrupt-controller: Add binding for the Microsemi
> Ocelot interrupt controller
>   irqchip: Add a driver for the Microsemi Ocelot controller
>   dt-bindings: pinctrl: Add bindings for Microsemi Ocelot
>   pinctrl: Add Microsemi Ocelot SoC driver
>   dt-bindings: mips: Add bindings for Microsemi SoCs
>   dt-bindings: power: reset: Document ocelot-reset binding
>   power: reset: Add a driver for the Microsemi Ocelot reset
>   MIPS: mscc: Add initial support for Microsemi MIPS SoCs
>   MIPS: mscc: add ocelot dtsi
>   MIPS: mscc: add ocelot PCB123 device tree
>   MIPS: defconfigs: add a defconfig for Microsemi SoCs
>   MAINTAINERS: Add entry for Microsemi MIPS SoCs
>
>  .../interrupt-controller/mscc,ocelot-icpu-intr.txt |  22 +
>  Documentation/devicetree/bindings/mips/mscc.txt|  46 ++
>  .../bindings/pinctrl/mscc,ocelot-pinctrl.txt   |  39 ++
>  .../bindings/power/reset/ocelot-reset.txt  |  17 +
>  .../devicetree/bindings/vendor-prefixes.txt|   1 +
>  MAINTAINERS|   7 +
>  arch/mips/Kbuild.platforms |   1 +
>  arch/mips/Kconfig  |  24 +
>  arch/mips/boot/dts/Makefile|   1 +
>  arch/mips/boot/dts/mscc/Makefile   |   6 +
>  arch/mips/boot/dts/mscc/ocelot.dtsi| 115 +
>  arch/mips/boot/dts/mscc/ocelot_pcb123.dts  |  27 ++
>  arch/mips/configs/mscc_defconfig   |  84 
>  arch/mips/mscc/Makefile|  11 +
>  arch/mips/mscc/Platform|  12 +
>  arch/mips/mscc/setup.c | 106 +
>  drivers/irqchip/Kconfig|   5 +
>  drivers/irqchip/Makefile   |   1 +
>  drivers/irqchip/irq-mscc-ocelot.c  | 109 +
>  drivers/pinctrl/Kconfig|  10 +
>  drivers/pinctrl/Makefile   |   1 +
>  drivers/pinctrl/pinctrl-ocelot.c   | 505 
> +
>  drivers/power/reset/Kconfig|   7 +
>  drivers/power/reset/Makefile   |   1 +
>  drivers/power/reset/ocelot-reset.c |  86 
>  25 files changed, 1244 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/interrupt-controller/mscc,ocelot-icpu-intr.txt
>  create mode 100644 Documentation/devicetree/bindings/mips/mscc.txt
>  create mode 100644 
> Documentation/devicetree/bindings/pinctrl/mscc,ocelot-pinctrl.txt
>  create mode 100644 
> Documentation/devicetree/bindings/power/reset/ocelot-reset.txt
>  create mode 100644 arch/mips/boot/dts/mscc/Makefile
>  create mode 100644 arch/mips/boot/dts/mscc/ocelot.dtsi
>  create mode 100644 arch/mips/boot/dts/mscc/ocelot_pcb123.dts
>  create mode 100644 arch/mips/configs/mscc_defconfig
>  create mode 100644 arch/mips/mscc/Makefile
>  create mode 100644 arch/mips/mscc/Platform
>  create mode 100644 arch/mips/mscc/setup.c
>  create mode 100644 drivers/irqchip/irq-mscc-ocelot.c
>  create mode 100644 drivers/pinctrl/pinctrl-ocelot.c
>  create mode 100644 drivers/power/reset/ocelot-reset.c
>
> --
> 2.15.1
>
>

Except for irqchip driver and pinctrl driver other parts of the series is
Reviewed-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>

Regards,
PrasannaKumar

Re: [PATCH v2 00/13] MIPS: add support for the Microsemi MIPS SoCs

[PrasannaKumar Muralidharan]

Hi Alexandre,

With very small amount of code in arch/mips this series looks really nice.

On 8 December 2017 at 21:16, Alexandre Belloni
 wrote:
> Hi,
>
> This patch series adds initial support for the Microsemi MIPS SoCs. It
> is currently focusing on the Microsemi Ocelot (VSC7513, VSC7514).
>
> It adds support for the IRQ controller, pinmux and gpio controller and
> reset control.
>
> This produces a kernel that can boot to the console.
>
> This is a single series for reference but it can also be taken
> separately by each maintainer as each drivers are independant.
>
> Changes in v2:
>  - removed the wildcard in MAINAINERS
>  - corrected the Cc list
>  - added proper documentation for both syscons
>  - removed the mscc,cpucontrol property
>  - updated the ranges property in the ocelot dtsi
>
> Cc: Rob Herring 
> Cc: devicet...@vger.kernel.org
> Cc: Thomas Gleixner 
> Cc: Jason Cooper 
> Cc: Linus Walleij 
> Cc: linux-g...@vger.kernel.org
> Cc: Sebastian Reichel 
> Cc: linux...@vger.kernel.org
>
>
> Alexandre Belloni (13):
>   dt-bindings: Add vendor prefix for Microsemi Corporation
>   dt-bindings: interrupt-controller: Add binding for the Microsemi
> Ocelot interrupt controller
>   irqchip: Add a driver for the Microsemi Ocelot controller
>   dt-bindings: pinctrl: Add bindings for Microsemi Ocelot
>   pinctrl: Add Microsemi Ocelot SoC driver
>   dt-bindings: mips: Add bindings for Microsemi SoCs
>   dt-bindings: power: reset: Document ocelot-reset binding
>   power: reset: Add a driver for the Microsemi Ocelot reset
>   MIPS: mscc: Add initial support for Microsemi MIPS SoCs
>   MIPS: mscc: add ocelot dtsi
>   MIPS: mscc: add ocelot PCB123 device tree
>   MIPS: defconfigs: add a defconfig for Microsemi SoCs
>   MAINTAINERS: Add entry for Microsemi MIPS SoCs
>
>  .../interrupt-controller/mscc,ocelot-icpu-intr.txt |  22 +
>  Documentation/devicetree/bindings/mips/mscc.txt|  46 ++
>  .../bindings/pinctrl/mscc,ocelot-pinctrl.txt   |  39 ++
>  .../bindings/power/reset/ocelot-reset.txt  |  17 +
>  .../devicetree/bindings/vendor-prefixes.txt|   1 +
>  MAINTAINERS|   7 +
>  arch/mips/Kbuild.platforms |   1 +
>  arch/mips/Kconfig  |  24 +
>  arch/mips/boot/dts/Makefile|   1 +
>  arch/mips/boot/dts/mscc/Makefile   |   6 +
>  arch/mips/boot/dts/mscc/ocelot.dtsi| 115 +
>  arch/mips/boot/dts/mscc/ocelot_pcb123.dts  |  27 ++
>  arch/mips/configs/mscc_defconfig   |  84 
>  arch/mips/mscc/Makefile|  11 +
>  arch/mips/mscc/Platform|  12 +
>  arch/mips/mscc/setup.c | 106 +
>  drivers/irqchip/Kconfig|   5 +
>  drivers/irqchip/Makefile   |   1 +
>  drivers/irqchip/irq-mscc-ocelot.c  | 109 +
>  drivers/pinctrl/Kconfig|  10 +
>  drivers/pinctrl/Makefile   |   1 +
>  drivers/pinctrl/pinctrl-ocelot.c   | 505 
> +
>  drivers/power/reset/Kconfig|   7 +
>  drivers/power/reset/Makefile   |   1 +
>  drivers/power/reset/ocelot-reset.c |  86 
>  25 files changed, 1244 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/interrupt-controller/mscc,ocelot-icpu-intr.txt
>  create mode 100644 Documentation/devicetree/bindings/mips/mscc.txt
>  create mode 100644 
> Documentation/devicetree/bindings/pinctrl/mscc,ocelot-pinctrl.txt
>  create mode 100644 
> Documentation/devicetree/bindings/power/reset/ocelot-reset.txt
>  create mode 100644 arch/mips/boot/dts/mscc/Makefile
>  create mode 100644 arch/mips/boot/dts/mscc/ocelot.dtsi
>  create mode 100644 arch/mips/boot/dts/mscc/ocelot_pcb123.dts
>  create mode 100644 arch/mips/configs/mscc_defconfig
>  create mode 100644 arch/mips/mscc/Makefile
>  create mode 100644 arch/mips/mscc/Platform
>  create mode 100644 arch/mips/mscc/setup.c
>  create mode 100644 drivers/irqchip/irq-mscc-ocelot.c
>  create mode 100644 drivers/pinctrl/pinctrl-ocelot.c
>  create mode 100644 drivers/power/reset/ocelot-reset.c
>
> --
> 2.15.1
>
>

Except for irqchip driver and pinctrl driver other parts of the series is
Reviewed-by: PrasannaKumar Muralidharan 

Regards,
PrasannaKumar

Re: [PATCH v2 2/3] hwrng: exynos - add Samsung Exynos True RNG driver

[PrasannaKumar Muralidharan]

;
> +
> +   return 0;
> +}
> +
> +static int __maybe_unused exynos_trng_suspend(struct device *dev)
> +{
> +   pm_runtime_put_sync(dev);
> +
> +   return 0;
> +}
> +
> +static int __maybe_unused exynos_trng_resume(struct device *dev)
> +{
> +   int ret;
> +
> +   ret = pm_runtime_get_sync(dev);
> +   if (ret < 0) {
> +   dev_err(dev, "Could not get runtime PM.\n");
> +   pm_runtime_put_noidle(dev);
> +   return ret;
> +   }
> +
> +   return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(exynos_trng_pm_ops, exynos_trng_suspend,
> +exynos_trng_resume);
> +
> +static const struct of_device_id exynos_trng_dt_match[] = {
> +   {
> +   .compatible = "samsung,exynos5250-trng",
> +   },
> +   { },
> +};
> +MODULE_DEVICE_TABLE(of, exynos_rng_dt_match);
> +
> +static struct platform_driver exynos_trng_driver = {
> +   .driver = {
> +   .name = "exynos-trng",
> +   .pm = _trng_pm_ops,
> +   .of_match_table = exynos_trng_dt_match,
> +   },
> +   .probe = exynos_trng_probe,
> +   .remove = exynos_trng_remove,
> +};
> +
> +module_platform_driver(exynos_trng_driver);
> +MODULE_AUTHOR("Łukasz Stelmach");
> +MODULE_DESCRIPTION("H/W TRNG driver for Exynos chips");
> +MODULE_LICENSE("GPL");
> --
> 2.11.0
>

Reviewed-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>

Regards,
PrasannaKumar

Re: [PATCH v2 2/3] hwrng: exynos - add Samsung Exynos True RNG driver

[PrasannaKumar Muralidharan]

   return 0;
> +}
> +
> +static int __maybe_unused exynos_trng_resume(struct device *dev)
> +{
> +   int ret;
> +
> +   ret = pm_runtime_get_sync(dev);
> +   if (ret < 0) {
> +   dev_err(dev, "Could not get runtime PM.\n");
> +   pm_runtime_put_noidle(dev);
> +   return ret;
> +   }
> +
> +   return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(exynos_trng_pm_ops, exynos_trng_suspend,
> +exynos_trng_resume);
> +
> +static const struct of_device_id exynos_trng_dt_match[] = {
> +   {
> +   .compatible = "samsung,exynos5250-trng",
> +   },
> +   { },
> +};
> +MODULE_DEVICE_TABLE(of, exynos_rng_dt_match);
> +
> +static struct platform_driver exynos_trng_driver = {
> +   .driver = {
> +   .name = "exynos-trng",
> +   .pm = _trng_pm_ops,
> +   .of_match_table = exynos_trng_dt_match,
> +   },
> +   .probe = exynos_trng_probe,
> +   .remove = exynos_trng_remove,
> +};
> +
> +module_platform_driver(exynos_trng_driver);
> +MODULE_AUTHOR("Łukasz Stelmach");
> +MODULE_DESCRIPTION("H/W TRNG driver for Exynos chips");
> +MODULE_LICENSE("GPL");
> --
> 2.11.0
>

Reviewed-by: PrasannaKumar Muralidharan 

Regards,
PrasannaKumar

Re: [PATCH] lib: memmove: Use optimised memcpy if possible

[PrasannaKumar Muralidharan]

Hi Dan,

On 26 November 2017 at 02:10, Dan Carpenter <dan.carpen...@oracle.com> wrote:
> Paul's original patch should have been separated into two patches to
> begin with.  The patch does two different things and one part goes
> through the MIPS tree and one part goes through Andrew, probably.

Okay. I will split his patch into two and send with my modifications.

> On Sat, Nov 25, 2017 at 10:52:04PM +0530, PrasannaKumar Muralidharan wrote:
>> Hi,
>>
>> On 4 October 2017 at 22:26, PrasannaKumar Muralidharan
>> <prasannatsmku...@gmail.com> wrote:
>> > When there is no overlap between src and dst use optimised memcpy if it
>> > is available.
>> >
>> > Signed-off-by: Paul Burton <paul.bur...@imgtec.com>
>> > Signed-off-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
>> > ---
>> > This change is a small part of a patch [1] from Paul Burton. I have
>> > added his Signed-off by. I do not know whether it is correct. Please let
>> > me know if it has to be changed, I will send a v2.
>
>
> Sign-off is like signing a legal document.  Read
> Documentation/process/submitting-patches.rst the section about
> "11) Sign your work - the Developer's Certificate of Origin" for an
> explanation.
>
> So, yeah, Paul provided his s-o-b and it needs to be here as well.  But
> also he maybe should get authorship credit.  Just put the first line in
> the email as:
>
> From: Paul Burton <paul.bur...@imgtec.com>
>
> But that's sort of a trickier thing, so maybe put some explanation that
> you chopped out a bit from Pauls patch in the changelog:
>
> This is part of a patch that Paul Burton wrote
> https://patchwork.linux-mips.org/patch/14517/
>
> I know you put that here, but since it's under the --- cut off it won't
> be saved in the final git log.

Sure. Will do.

>> >
>> > This patch is boot tested with qemu for MIPS architecture by removing
>> > mips's memmove routine. This patch does not contain MIPS changes. I
>> > will try to find out why [1] was not taken already and figure out what
>> > to do.
>> >
>
> Instead of boot testing, it would be better if we had a benchmark to
> show it helped speed things up.

I will try to come up with some reasonable benchmark and post its results.

>> > 1. https://patchwork.linux-mips.org/patch/14517/
>> >
>> >  lib/string.c | 11 +++
>> >  1 file changed, 11 insertions(+)
>> >
>> > diff --git a/lib/string.c b/lib/string.c
>> > index 9921dc2..462ab7b 100644
>> > --- a/lib/string.c
>> > +++ b/lib/string.c
>> > @@ -825,6 +825,17 @@ void *memmove(void *dest, const void *src, size_t 
>> > count)
>> > char *tmp;
>> > const char *s;
>> >
>> > +#ifdef __HAVE_ARCH_MEMCPY
>> > +   /* Use optimised memcpy when there is no overlap */
>> > +   const char *s_end = src + count;
>> > +   const char *d = dest;
>> > +   char *d_end = dest + count;
>> > +
>> > +   s = src;
>> > +   if ((d_end <= s) || (s_end <= d))
>> > +   return memcpy(dest, src, count);
>> > +#endif /* __HAVE_ARCH_MEMCPY */
>> > +
>> > if (dest <= src) {
>> > tmp = dest;
>> > s = src;
>> > --
>> > 2.10.0
>> >
>>
>> Is there anything more that I have to do for this patch?
>>
>
> Probably a patch like this needs to go through Andrew.  Send it again
> and CC Andrew Morton <a...@linux-foundation.org>.  It would be nice if
> we could CC a better list than LKML but I don't know which one...  Few
> people read LKML.

I will add Andrew. Get maintainer script gave me a small list of email
id for this. I don't know if there is a better way than using
get_maintainer.pl.

> regards,
> dan carpenter

Thanks a lot for your time Dan.

Thanks,
PrasannaKumar

Re: [PATCH] lib: memmove: Use optimised memcpy if possible

[PrasannaKumar Muralidharan]

Hi Dan,

On 26 November 2017 at 02:10, Dan Carpenter  wrote:
> Paul's original patch should have been separated into two patches to
> begin with.  The patch does two different things and one part goes
> through the MIPS tree and one part goes through Andrew, probably.

Okay. I will split his patch into two and send with my modifications.

> On Sat, Nov 25, 2017 at 10:52:04PM +0530, PrasannaKumar Muralidharan wrote:
>> Hi,
>>
>> On 4 October 2017 at 22:26, PrasannaKumar Muralidharan
>>  wrote:
>> > When there is no overlap between src and dst use optimised memcpy if it
>> > is available.
>> >
>> > Signed-off-by: Paul Burton 
>> > Signed-off-by: PrasannaKumar Muralidharan 
>> > ---
>> > This change is a small part of a patch [1] from Paul Burton. I have
>> > added his Signed-off by. I do not know whether it is correct. Please let
>> > me know if it has to be changed, I will send a v2.
>
>
> Sign-off is like signing a legal document.  Read
> Documentation/process/submitting-patches.rst the section about
> "11) Sign your work - the Developer's Certificate of Origin" for an
> explanation.
>
> So, yeah, Paul provided his s-o-b and it needs to be here as well.  But
> also he maybe should get authorship credit.  Just put the first line in
> the email as:
>
> From: Paul Burton 
>
> But that's sort of a trickier thing, so maybe put some explanation that
> you chopped out a bit from Pauls patch in the changelog:
>
> This is part of a patch that Paul Burton wrote
> https://patchwork.linux-mips.org/patch/14517/
>
> I know you put that here, but since it's under the --- cut off it won't
> be saved in the final git log.

Sure. Will do.

>> >
>> > This patch is boot tested with qemu for MIPS architecture by removing
>> > mips's memmove routine. This patch does not contain MIPS changes. I
>> > will try to find out why [1] was not taken already and figure out what
>> > to do.
>> >
>
> Instead of boot testing, it would be better if we had a benchmark to
> show it helped speed things up.

I will try to come up with some reasonable benchmark and post its results.

>> > 1. https://patchwork.linux-mips.org/patch/14517/
>> >
>> >  lib/string.c | 11 +++
>> >  1 file changed, 11 insertions(+)
>> >
>> > diff --git a/lib/string.c b/lib/string.c
>> > index 9921dc2..462ab7b 100644
>> > --- a/lib/string.c
>> > +++ b/lib/string.c
>> > @@ -825,6 +825,17 @@ void *memmove(void *dest, const void *src, size_t 
>> > count)
>> > char *tmp;
>> > const char *s;
>> >
>> > +#ifdef __HAVE_ARCH_MEMCPY
>> > +   /* Use optimised memcpy when there is no overlap */
>> > +   const char *s_end = src + count;
>> > +   const char *d = dest;
>> > +   char *d_end = dest + count;
>> > +
>> > +   s = src;
>> > +   if ((d_end <= s) || (s_end <= d))
>> > +   return memcpy(dest, src, count);
>> > +#endif /* __HAVE_ARCH_MEMCPY */
>> > +
>> > if (dest <= src) {
>> > tmp = dest;
>> > s = src;
>> > --
>> > 2.10.0
>> >
>>
>> Is there anything more that I have to do for this patch?
>>
>
> Probably a patch like this needs to go through Andrew.  Send it again
> and CC Andrew Morton .  It would be nice if
> we could CC a better list than LKML but I don't know which one...  Few
> people read LKML.

I will add Andrew. Get maintainer script gave me a small list of email
id for this. I don't know if there is a better way than using
get_maintainer.pl.

> regards,
> dan carpenter

Thanks a lot for your time Dan.

Thanks,
PrasannaKumar

Re: [PATCH] lib: memmove: Use optimised memcpy if possible

[PrasannaKumar Muralidharan]

Hi,

On 4 October 2017 at 22:26, PrasannaKumar Muralidharan
<prasannatsmku...@gmail.com> wrote:
> When there is no overlap between src and dst use optimised memcpy if it
> is available.
>
> Signed-off-by: Paul Burton <paul.bur...@imgtec.com>
> Signed-off-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
> ---
> This change is a small part of a patch [1] from Paul Burton. I have
> added his Signed-off by. I do not know whether it is correct. Please let
> me know if it has to be changed, I will send a v2.
>
> This patch is boot tested with qemu for MIPS architecture by removing
> mips's memmove routine. This patch does not contain MIPS changes. I
> will try to find out why [1] was not taken already and figure out what
> to do.
>
> 1. https://patchwork.linux-mips.org/patch/14517/
>
>  lib/string.c | 11 +++
>  1 file changed, 11 insertions(+)
>
> diff --git a/lib/string.c b/lib/string.c
> index 9921dc2..462ab7b 100644
> --- a/lib/string.c
> +++ b/lib/string.c
> @@ -825,6 +825,17 @@ void *memmove(void *dest, const void *src, size_t count)
> char *tmp;
> const char *s;
>
> +#ifdef __HAVE_ARCH_MEMCPY
> +   /* Use optimised memcpy when there is no overlap */
> +   const char *s_end = src + count;
> +   const char *d = dest;
> +   char *d_end = dest + count;
> +
> +   s = src;
> +   if ((d_end <= s) || (s_end <= d))
> +   return memcpy(dest, src, count);
> +#endif /* __HAVE_ARCH_MEMCPY */
> +
> if (dest <= src) {
> tmp = dest;
> s = src;
> --
> 2.10.0
>

Is there anything more that I have to do for this patch?

Regards,
PrasannaKumar

Re: [PATCH] lib: memmove: Use optimised memcpy if possible

[PrasannaKumar Muralidharan]

Hi,

On 4 October 2017 at 22:26, PrasannaKumar Muralidharan
 wrote:
> When there is no overlap between src and dst use optimised memcpy if it
> is available.
>
> Signed-off-by: Paul Burton 
> Signed-off-by: PrasannaKumar Muralidharan 
> ---
> This change is a small part of a patch [1] from Paul Burton. I have
> added his Signed-off by. I do not know whether it is correct. Please let
> me know if it has to be changed, I will send a v2.
>
> This patch is boot tested with qemu for MIPS architecture by removing
> mips's memmove routine. This patch does not contain MIPS changes. I
> will try to find out why [1] was not taken already and figure out what
> to do.
>
> 1. https://patchwork.linux-mips.org/patch/14517/
>
>  lib/string.c | 11 +++
>  1 file changed, 11 insertions(+)
>
> diff --git a/lib/string.c b/lib/string.c
> index 9921dc2..462ab7b 100644
> --- a/lib/string.c
> +++ b/lib/string.c
> @@ -825,6 +825,17 @@ void *memmove(void *dest, const void *src, size_t count)
> char *tmp;
> const char *s;
>
> +#ifdef __HAVE_ARCH_MEMCPY
> +   /* Use optimised memcpy when there is no overlap */
> +   const char *s_end = src + count;
> +   const char *d = dest;
> +   char *d_end = dest + count;
> +
> +   s = src;
> +   if ((d_end <= s) || (s_end <= d))
> +   return memcpy(dest, src, count);
> +#endif /* __HAVE_ARCH_MEMCPY */
> +
> if (dest <= src) {
> tmp = dest;
> s = src;
> --
> 2.10.0
>

Is there anything more that I have to do for this patch?

Regards,
PrasannaKumar

Re: [PATCH 2/3] hwrng: exynos - add Samsung Exynos True RNG driver

[PrasannaKumar Muralidharan]

On 24 November 2017 at 20:55, PrasannaKumar Muralidharan
<prasannatsmku...@gmail.com> wrote:
> Hi Lukasz,
>
> Some minor comments below.
>
> On 23 November 2017 at 20:39, Łukasz Stelmach <l.stelm...@samsung.com> wrote:
>> Add support for True Random Number Generator found in Samsung Exynos
>> 5250+ SoCs.
>>
>> Signed-off-by: Łukasz Stelmach <l.stelm...@samsung.com>
>> ---
>>  MAINTAINERS  |   7 +
>>  drivers/char/hw_random/Kconfig   |  12 ++
>>  drivers/char/hw_random/Makefile  |   1 +
>>  drivers/char/hw_random/exynos-trng.c | 256 
>> +++
>>  4 files changed, 276 insertions(+)
>>  create mode 100644 drivers/char/hw_random/exynos-trng.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 2811a211632c..992074cca612 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -11780,6 +11780,13 @@ S: Maintained
>>  F: drivers/crypto/exynos-rng.c
>>  F: Documentation/devicetree/bindings/rng/samsung,exynos-rng4.txt
>>
>> +SAMSUNG EXYNOS TRUE RANDOM NUMBER GENERATOR (TRNG) DRIVER
>> +M: Łukasz Stelmach <l.stelm...@samsung.com>
>> +L: linux-samsung-...@vger.kernel.org
>> +S: Maintained
>> +F: drivers/char/hw_random/exynos-trng.c
>> +F: Documentation/devicetree/bindings/rng/samsung,exynos5250-trng.txt
>> +
>>  SAMSUNG FRAMEBUFFER DRIVER
>>  M: Jingoo Han <jingooh...@gmail.com>
>>  L: linux-fb...@vger.kernel.org
>> diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
>> index 95a031e9eced..a788ac07081b 100644
>> --- a/drivers/char/hw_random/Kconfig
>> +++ b/drivers/char/hw_random/Kconfig
>> @@ -449,6 +449,18 @@ config HW_RANDOM_S390
>>
>>   If unsure, say Y.
>>
>> +config HW_RANDOM_EXYNOS
>> +   tristate "Samsung Exynos True Random Number Generator support"
>> +   depends on ARCH_EXYNOS || COMPILE_TEST
>> +   default HW_RANDOM
>> +   ---help---
>> + This driver provides support for the True Random Number
>> + Generator available in Exynos SoCs.
>> +
>> +To compile this driver as a module, choose M here: the module
>> +will be called exynos-trng.
>> +
>> +If unsure, say Y.
>>  endif # HW_RANDOM
>>
>>  config UML_RANDOM
>> diff --git a/drivers/char/hw_random/Makefile 
>> b/drivers/char/hw_random/Makefile
>> index f3728d008fff..5595df97da7a 100644
>> --- a/drivers/char/hw_random/Makefile
>> +++ b/drivers/char/hw_random/Makefile
>> @@ -14,6 +14,7 @@ obj-$(CONFIG_HW_RANDOM_GEODE) += geode-rng.o
>>  obj-$(CONFIG_HW_RANDOM_N2RNG) += n2-rng.o
>>  n2-rng-y := n2-drv.o n2-asm.o
>>  obj-$(CONFIG_HW_RANDOM_VIA) += via-rng.o
>> +obj-$(CONFIG_HW_RANDOM_EXYNOS) += exynos-trng.o
>>  obj-$(CONFIG_HW_RANDOM_IXP4XX) += ixp4xx-rng.o
>>  obj-$(CONFIG_HW_RANDOM_OMAP) += omap-rng.o
>>  obj-$(CONFIG_HW_RANDOM_OMAP3_ROM) += omap3-rom-rng.o
>> diff --git a/drivers/char/hw_random/exynos-trng.c 
>> b/drivers/char/hw_random/exynos-trng.c
>> new file mode 100644
>> index ..340e106cae24
>> --- /dev/null
>> +++ b/drivers/char/hw_random/exynos-trng.c
>> @@ -0,0 +1,256 @@
>> +/*
>> + * RNG driver for Exynos TRNGs
>> + *
>> + * Author: Łukasz Stelmach <l.stelm...@samsung.com>
>> + *
>> + * Copyright 2017 (c) Samsung Electronics Software, Inc.
>> + *
>> + * Based on the Exynos PRNG driver drivers/crypto/exynos-rng by
>> + * Krzysztof Kozłowski <k...@kernel.org>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation;
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#define EXYNOS_TRNG_CLKDIV (0x0)
>> +#define EXYNOS_TRNG_CTRL   (0x20)
>> +#define EXYNOS_TRNG_POST_CTRL  (0x30)
>> +#define EXYNOS_TRNG_ONLINE_CTRL(0x40)
>> +#define EXYNOS_TRNG_ONLINE_STAT(0x44)
>> +#define EXY

Re: [PATCH 2/3] hwrng: exynos - add Samsung Exynos True RNG driver

[PrasannaKumar Muralidharan]

On 24 November 2017 at 20:55, PrasannaKumar Muralidharan
 wrote:
> Hi Lukasz,
>
> Some minor comments below.
>
> On 23 November 2017 at 20:39, Łukasz Stelmach  wrote:
>> Add support for True Random Number Generator found in Samsung Exynos
>> 5250+ SoCs.
>>
>> Signed-off-by: Łukasz Stelmach 
>> ---
>>  MAINTAINERS  |   7 +
>>  drivers/char/hw_random/Kconfig   |  12 ++
>>  drivers/char/hw_random/Makefile  |   1 +
>>  drivers/char/hw_random/exynos-trng.c | 256 
>> +++
>>  4 files changed, 276 insertions(+)
>>  create mode 100644 drivers/char/hw_random/exynos-trng.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 2811a211632c..992074cca612 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -11780,6 +11780,13 @@ S: Maintained
>>  F: drivers/crypto/exynos-rng.c
>>  F: Documentation/devicetree/bindings/rng/samsung,exynos-rng4.txt
>>
>> +SAMSUNG EXYNOS TRUE RANDOM NUMBER GENERATOR (TRNG) DRIVER
>> +M: Łukasz Stelmach 
>> +L: linux-samsung-...@vger.kernel.org
>> +S: Maintained
>> +F: drivers/char/hw_random/exynos-trng.c
>> +F: Documentation/devicetree/bindings/rng/samsung,exynos5250-trng.txt
>> +
>>  SAMSUNG FRAMEBUFFER DRIVER
>>  M: Jingoo Han 
>>  L: linux-fb...@vger.kernel.org
>> diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
>> index 95a031e9eced..a788ac07081b 100644
>> --- a/drivers/char/hw_random/Kconfig
>> +++ b/drivers/char/hw_random/Kconfig
>> @@ -449,6 +449,18 @@ config HW_RANDOM_S390
>>
>>   If unsure, say Y.
>>
>> +config HW_RANDOM_EXYNOS
>> +   tristate "Samsung Exynos True Random Number Generator support"
>> +   depends on ARCH_EXYNOS || COMPILE_TEST
>> +   default HW_RANDOM
>> +   ---help---
>> + This driver provides support for the True Random Number
>> + Generator available in Exynos SoCs.
>> +
>> +To compile this driver as a module, choose M here: the module
>> +will be called exynos-trng.
>> +
>> +If unsure, say Y.
>>  endif # HW_RANDOM
>>
>>  config UML_RANDOM
>> diff --git a/drivers/char/hw_random/Makefile 
>> b/drivers/char/hw_random/Makefile
>> index f3728d008fff..5595df97da7a 100644
>> --- a/drivers/char/hw_random/Makefile
>> +++ b/drivers/char/hw_random/Makefile
>> @@ -14,6 +14,7 @@ obj-$(CONFIG_HW_RANDOM_GEODE) += geode-rng.o
>>  obj-$(CONFIG_HW_RANDOM_N2RNG) += n2-rng.o
>>  n2-rng-y := n2-drv.o n2-asm.o
>>  obj-$(CONFIG_HW_RANDOM_VIA) += via-rng.o
>> +obj-$(CONFIG_HW_RANDOM_EXYNOS) += exynos-trng.o
>>  obj-$(CONFIG_HW_RANDOM_IXP4XX) += ixp4xx-rng.o
>>  obj-$(CONFIG_HW_RANDOM_OMAP) += omap-rng.o
>>  obj-$(CONFIG_HW_RANDOM_OMAP3_ROM) += omap3-rom-rng.o
>> diff --git a/drivers/char/hw_random/exynos-trng.c 
>> b/drivers/char/hw_random/exynos-trng.c
>> new file mode 100644
>> index ..340e106cae24
>> --- /dev/null
>> +++ b/drivers/char/hw_random/exynos-trng.c
>> @@ -0,0 +1,256 @@
>> +/*
>> + * RNG driver for Exynos TRNGs
>> + *
>> + * Author: Łukasz Stelmach 
>> + *
>> + * Copyright 2017 (c) Samsung Electronics Software, Inc.
>> + *
>> + * Based on the Exynos PRNG driver drivers/crypto/exynos-rng by
>> + * Krzysztof Kozłowski 
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation;
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#define EXYNOS_TRNG_CLKDIV (0x0)
>> +#define EXYNOS_TRNG_CTRL   (0x20)
>> +#define EXYNOS_TRNG_POST_CTRL  (0x30)
>> +#define EXYNOS_TRNG_ONLINE_CTRL(0x40)
>> +#define EXYNOS_TRNG_ONLINE_STAT(0x44)
>> +#define EXYNOS_TRNG_ONLINE_MAXCHI2 (0x48)
>> +#define EXYNOS_TRNG_FIFO_CTRL  (0x50)
>> +#define EXYNOS_TRNG_FIFO_0 (0x80)
>> +#define EXYNOS_TRNG_FIFO_1 (0x84)
>> +#def

Re: [PATCH 2/3] hwrng: exynos - add Samsung Exynos True RNG driver

[PrasannaKumar Muralidharan]

Hi Lukasz,

Some minor comments below.

On 23 November 2017 at 20:39, Łukasz Stelmach  wrote:
> Add support for True Random Number Generator found in Samsung Exynos
> 5250+ SoCs.
>
> Signed-off-by: Łukasz Stelmach 
> ---
>  MAINTAINERS  |   7 +
>  drivers/char/hw_random/Kconfig   |  12 ++
>  drivers/char/hw_random/Makefile  |   1 +
>  drivers/char/hw_random/exynos-trng.c | 256 
> +++
>  4 files changed, 276 insertions(+)
>  create mode 100644 drivers/char/hw_random/exynos-trng.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 2811a211632c..992074cca612 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11780,6 +11780,13 @@ S: Maintained
>  F: drivers/crypto/exynos-rng.c
>  F: Documentation/devicetree/bindings/rng/samsung,exynos-rng4.txt
>
> +SAMSUNG EXYNOS TRUE RANDOM NUMBER GENERATOR (TRNG) DRIVER
> +M: Łukasz Stelmach 
> +L: linux-samsung-...@vger.kernel.org
> +S: Maintained
> +F: drivers/char/hw_random/exynos-trng.c
> +F: Documentation/devicetree/bindings/rng/samsung,exynos5250-trng.txt
> +
>  SAMSUNG FRAMEBUFFER DRIVER
>  M: Jingoo Han 
>  L: linux-fb...@vger.kernel.org
> diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
> index 95a031e9eced..a788ac07081b 100644
> --- a/drivers/char/hw_random/Kconfig
> +++ b/drivers/char/hw_random/Kconfig
> @@ -449,6 +449,18 @@ config HW_RANDOM_S390
>
>   If unsure, say Y.
>
> +config HW_RANDOM_EXYNOS
> +   tristate "Samsung Exynos True Random Number Generator support"
> +   depends on ARCH_EXYNOS || COMPILE_TEST
> +   default HW_RANDOM
> +   ---help---
> + This driver provides support for the True Random Number
> + Generator available in Exynos SoCs.
> +
> +To compile this driver as a module, choose M here: the module
> +will be called exynos-trng.
> +
> +If unsure, say Y.
>  endif # HW_RANDOM
>
>  config UML_RANDOM
> diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
> index f3728d008fff..5595df97da7a 100644
> --- a/drivers/char/hw_random/Makefile
> +++ b/drivers/char/hw_random/Makefile
> @@ -14,6 +14,7 @@ obj-$(CONFIG_HW_RANDOM_GEODE) += geode-rng.o
>  obj-$(CONFIG_HW_RANDOM_N2RNG) += n2-rng.o
>  n2-rng-y := n2-drv.o n2-asm.o
>  obj-$(CONFIG_HW_RANDOM_VIA) += via-rng.o
> +obj-$(CONFIG_HW_RANDOM_EXYNOS) += exynos-trng.o
>  obj-$(CONFIG_HW_RANDOM_IXP4XX) += ixp4xx-rng.o
>  obj-$(CONFIG_HW_RANDOM_OMAP) += omap-rng.o
>  obj-$(CONFIG_HW_RANDOM_OMAP3_ROM) += omap3-rom-rng.o
> diff --git a/drivers/char/hw_random/exynos-trng.c 
> b/drivers/char/hw_random/exynos-trng.c
> new file mode 100644
> index ..340e106cae24
> --- /dev/null
> +++ b/drivers/char/hw_random/exynos-trng.c
> @@ -0,0 +1,256 @@
> +/*
> + * RNG driver for Exynos TRNGs
> + *
> + * Author: Łukasz Stelmach 
> + *
> + * Copyright 2017 (c) Samsung Electronics Software, Inc.
> + *
> + * Based on the Exynos PRNG driver drivers/crypto/exynos-rng by
> + * Krzysztof Kozłowski 
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation;
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define EXYNOS_TRNG_CLKDIV (0x0)
> +#define EXYNOS_TRNG_CTRL   (0x20)
> +#define EXYNOS_TRNG_POST_CTRL  (0x30)
> +#define EXYNOS_TRNG_ONLINE_CTRL(0x40)
> +#define EXYNOS_TRNG_ONLINE_STAT(0x44)
> +#define EXYNOS_TRNG_ONLINE_MAXCHI2 (0x48)
> +#define EXYNOS_TRNG_FIFO_CTRL  (0x50)
> +#define EXYNOS_TRNG_FIFO_0 (0x80)
> +#define EXYNOS_TRNG_FIFO_1 (0x84)
> +#define EXYNOS_TRNG_FIFO_2 (0x88)
> +#define EXYNOS_TRNG_FIFO_3 (0x8c)
> +#define EXYNOS_TRNG_FIFO_4 (0x90)
> +#define EXYNOS_TRNG_FIFO_5 (0x94)
> +#define EXYNOS_TRNG_FIFO_6 (0x98)
> +#define EXYNOS_TRNG_FIFO_7 (0x9c)
> +#define EXYNOS_TRNG_FIFO_LEN   (8)
> +#define EXYNOS_TRNG_CLOCK_RATE (50)
> +
> +struct exynos_trng_dev {
> +   struct device*dev;
> +   void __iomem *mem;
> +   struct clk   *clk;
> +   struct hwrng rng;
> +};
> +
> +struct exynos_trng_dev *exynos_trng_dev;
> +
> +static inline void exynos_trng_set_reg(struct exynos_trng_dev *trng, u16 reg,
> +  u32 val)
> +{
> +   /* Check range of reg? */
> +   __raw_writel(val, trng->mem + 

Re: [PATCH 2/3] hwrng: exynos - add Samsung Exynos True RNG driver

[PrasannaKumar Muralidharan]

Hi Lukasz,

Some minor comments below.

On 23 November 2017 at 20:39, Łukasz Stelmach  wrote:
> Add support for True Random Number Generator found in Samsung Exynos
> 5250+ SoCs.
>
> Signed-off-by: Łukasz Stelmach 
> ---
>  MAINTAINERS  |   7 +
>  drivers/char/hw_random/Kconfig   |  12 ++
>  drivers/char/hw_random/Makefile  |   1 +
>  drivers/char/hw_random/exynos-trng.c | 256 
> +++
>  4 files changed, 276 insertions(+)
>  create mode 100644 drivers/char/hw_random/exynos-trng.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 2811a211632c..992074cca612 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11780,6 +11780,13 @@ S: Maintained
>  F: drivers/crypto/exynos-rng.c
>  F: Documentation/devicetree/bindings/rng/samsung,exynos-rng4.txt
>
> +SAMSUNG EXYNOS TRUE RANDOM NUMBER GENERATOR (TRNG) DRIVER
> +M: Łukasz Stelmach 
> +L: linux-samsung-...@vger.kernel.org
> +S: Maintained
> +F: drivers/char/hw_random/exynos-trng.c
> +F: Documentation/devicetree/bindings/rng/samsung,exynos5250-trng.txt
> +
>  SAMSUNG FRAMEBUFFER DRIVER
>  M: Jingoo Han 
>  L: linux-fb...@vger.kernel.org
> diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
> index 95a031e9eced..a788ac07081b 100644
> --- a/drivers/char/hw_random/Kconfig
> +++ b/drivers/char/hw_random/Kconfig
> @@ -449,6 +449,18 @@ config HW_RANDOM_S390
>
>   If unsure, say Y.
>
> +config HW_RANDOM_EXYNOS
> +   tristate "Samsung Exynos True Random Number Generator support"
> +   depends on ARCH_EXYNOS || COMPILE_TEST
> +   default HW_RANDOM
> +   ---help---
> + This driver provides support for the True Random Number
> + Generator available in Exynos SoCs.
> +
> +To compile this driver as a module, choose M here: the module
> +will be called exynos-trng.
> +
> +If unsure, say Y.
>  endif # HW_RANDOM
>
>  config UML_RANDOM
> diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
> index f3728d008fff..5595df97da7a 100644
> --- a/drivers/char/hw_random/Makefile
> +++ b/drivers/char/hw_random/Makefile
> @@ -14,6 +14,7 @@ obj-$(CONFIG_HW_RANDOM_GEODE) += geode-rng.o
>  obj-$(CONFIG_HW_RANDOM_N2RNG) += n2-rng.o
>  n2-rng-y := n2-drv.o n2-asm.o
>  obj-$(CONFIG_HW_RANDOM_VIA) += via-rng.o
> +obj-$(CONFIG_HW_RANDOM_EXYNOS) += exynos-trng.o
>  obj-$(CONFIG_HW_RANDOM_IXP4XX) += ixp4xx-rng.o
>  obj-$(CONFIG_HW_RANDOM_OMAP) += omap-rng.o
>  obj-$(CONFIG_HW_RANDOM_OMAP3_ROM) += omap3-rom-rng.o
> diff --git a/drivers/char/hw_random/exynos-trng.c 
> b/drivers/char/hw_random/exynos-trng.c
> new file mode 100644
> index ..340e106cae24
> --- /dev/null
> +++ b/drivers/char/hw_random/exynos-trng.c
> @@ -0,0 +1,256 @@
> +/*
> + * RNG driver for Exynos TRNGs
> + *
> + * Author: Łukasz Stelmach 
> + *
> + * Copyright 2017 (c) Samsung Electronics Software, Inc.
> + *
> + * Based on the Exynos PRNG driver drivers/crypto/exynos-rng by
> + * Krzysztof Kozłowski 
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation;
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define EXYNOS_TRNG_CLKDIV (0x0)
> +#define EXYNOS_TRNG_CTRL   (0x20)
> +#define EXYNOS_TRNG_POST_CTRL  (0x30)
> +#define EXYNOS_TRNG_ONLINE_CTRL(0x40)
> +#define EXYNOS_TRNG_ONLINE_STAT(0x44)
> +#define EXYNOS_TRNG_ONLINE_MAXCHI2 (0x48)
> +#define EXYNOS_TRNG_FIFO_CTRL  (0x50)
> +#define EXYNOS_TRNG_FIFO_0 (0x80)
> +#define EXYNOS_TRNG_FIFO_1 (0x84)
> +#define EXYNOS_TRNG_FIFO_2 (0x88)
> +#define EXYNOS_TRNG_FIFO_3 (0x8c)
> +#define EXYNOS_TRNG_FIFO_4 (0x90)
> +#define EXYNOS_TRNG_FIFO_5 (0x94)
> +#define EXYNOS_TRNG_FIFO_6 (0x98)
> +#define EXYNOS_TRNG_FIFO_7 (0x9c)
> +#define EXYNOS_TRNG_FIFO_LEN   (8)
> +#define EXYNOS_TRNG_CLOCK_RATE (50)
> +
> +struct exynos_trng_dev {
> +   struct device*dev;
> +   void __iomem *mem;
> +   struct clk   *clk;
> +   struct hwrng rng;
> +};
> +
> +struct exynos_trng_dev *exynos_trng_dev;
> +
> +static inline void exynos_trng_set_reg(struct exynos_trng_dev *trng, u16 reg,
> +  u32 val)
> +{
> +   /* Check range of reg? */
> +   __raw_writel(val, trng->mem + reg);

Any specific reason to use __raw_writel? Why not just writel?

> +}
> +
> +static inline u32 exynos_trng_get_reg(struct 

Re: [PATCH] crypto: hifn_795x - Fix a memory leak in the error handling path of 'hifn_probe()'

[PrasannaKumar Muralidharan]

Hi Christophe,

On 18 November 2017 at 19:15, Christophe JAILLET
<christophe.jail...@wanadoo.fr> wrote:
> 'dev' is leaking in the error handling path of 'hifn_probe()'.
>
> Add a 'kfree(dev)' to match the code in 'hifn_remove()'
>
> Signed-off-by: Christophe JAILLET <christophe.jail...@wanadoo.fr>
> ---
>  drivers/crypto/hifn_795x.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/crypto/hifn_795x.c b/drivers/crypto/hifn_795x.c
> index e09d4055b19e..a5a36fe7bf2c 100644
> --- a/drivers/crypto/hifn_795x.c
> +++ b/drivers/crypto/hifn_795x.c
> @@ -2579,6 +2579,7 @@ static int hifn_probe(struct pci_dev *pdev, const 
> struct pci_device_id *id)
> for (i = 0; i < 3; ++i)
> if (dev->bar[i])
> iounmap(dev->bar[i]);
> +   kfree(dev);
>
>  err_out_free_regions:
>     pci_release_regions(pdev);
> --
> 2.14.1
>

Nice catch.

Reviewed-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>

Regards,
PrasannaKumar

Re: [PATCH] crypto: hifn_795x - Fix a memory leak in the error handling path of 'hifn_probe()'

[PrasannaKumar Muralidharan]

Hi Christophe,

On 18 November 2017 at 19:15, Christophe JAILLET
 wrote:
> 'dev' is leaking in the error handling path of 'hifn_probe()'.
>
> Add a 'kfree(dev)' to match the code in 'hifn_remove()'
>
> Signed-off-by: Christophe JAILLET 
> ---
>  drivers/crypto/hifn_795x.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/crypto/hifn_795x.c b/drivers/crypto/hifn_795x.c
> index e09d4055b19e..a5a36fe7bf2c 100644
> --- a/drivers/crypto/hifn_795x.c
> +++ b/drivers/crypto/hifn_795x.c
> @@ -2579,6 +2579,7 @@ static int hifn_probe(struct pci_dev *pdev, const 
> struct pci_device_id *id)
> for (i = 0; i < 3; ++i)
> if (dev->bar[i])
> iounmap(dev->bar[i]);
> +   kfree(dev);
>
>  err_out_free_regions:
> pci_release_regions(pdev);
> --
> 2.14.1
>

Nice catch.

Reviewed-by: PrasannaKumar Muralidharan 

Regards,
PrasannaKumar

Re: [PATCH v2] tpm: Move Linux RNG connection to hwrng

[PrasannaKumar Muralidharan]

Hi Jason,

On 9 November 2017 at 21:59, Jason Gunthorpe <j...@ziepe.ca> wrote:
> On Thu, Nov 09, 2017 at 09:49:33PM +0530, PrasannaKumar Muralidharan wrote:
>> Hi Jason,
>>
>> On 7 November 2017 at 21:34, Jason Gunthorpe <j...@ziepe.ca> wrote:
>> > On Tue, Nov 07, 2017 at 08:50:44AM +0530, PrasannaKumar Muralidharan wrote:
>> >
>> >> I am assuming you are talking about the following patches - using
>> >> struct tpm_chip instead of chip number and this patch.
>> >
>> > yes
>> >
>> >> I won't be able to test if struct tpm_chip usage as I don't have
>> >> multiple tpm hw in one machine. In case of tpm rng changes I can test
>> >> only the lifecycle of tpm rng device. Is that enough? I feel my test
>> >> will be limited. Please provide your thoughts on this.
>> >
>> > That is certainly better than no testing.
>>
>> The struct tpm_chip patch partially applied on linux next. I had to
>> manually change the code. In qemu tpm rng device did not show up on
>> loading tpm module. My laptop has tpm hw but Linux next did not work
>> properly in that. All my console were getting spammed with some USB
>> log message and I could not do anything. X did not start either. I
>> could not debug the issue as the logs were printing infinitely. Will
>> get little more time this weekend. Will do a proper test and provide
>> you the result.
>
> Test against 4.15-rc, here are the two patches
>
> https://github.com/jgunthorpe/linux/tree/tpm
>
> Pull from here and merge the latest rc and you will probably have a
> bootable system.
>
> Jason

Applied this patch on v4.14-rc4. Able to get data from tpm rng
(/dev/hwrng with tpm as the chosen rng). This patch works fine. Its
just a basic test though.

Tested-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>

Regards,
PrasannaKumar

Re: [PATCH v2] tpm: Move Linux RNG connection to hwrng

[PrasannaKumar Muralidharan]

Hi Jason,

On 9 November 2017 at 21:59, Jason Gunthorpe  wrote:
> On Thu, Nov 09, 2017 at 09:49:33PM +0530, PrasannaKumar Muralidharan wrote:
>> Hi Jason,
>>
>> On 7 November 2017 at 21:34, Jason Gunthorpe  wrote:
>> > On Tue, Nov 07, 2017 at 08:50:44AM +0530, PrasannaKumar Muralidharan wrote:
>> >
>> >> I am assuming you are talking about the following patches - using
>> >> struct tpm_chip instead of chip number and this patch.
>> >
>> > yes
>> >
>> >> I won't be able to test if struct tpm_chip usage as I don't have
>> >> multiple tpm hw in one machine. In case of tpm rng changes I can test
>> >> only the lifecycle of tpm rng device. Is that enough? I feel my test
>> >> will be limited. Please provide your thoughts on this.
>> >
>> > That is certainly better than no testing.
>>
>> The struct tpm_chip patch partially applied on linux next. I had to
>> manually change the code. In qemu tpm rng device did not show up on
>> loading tpm module. My laptop has tpm hw but Linux next did not work
>> properly in that. All my console were getting spammed with some USB
>> log message and I could not do anything. X did not start either. I
>> could not debug the issue as the logs were printing infinitely. Will
>> get little more time this weekend. Will do a proper test and provide
>> you the result.
>
> Test against 4.15-rc, here are the two patches
>
> https://github.com/jgunthorpe/linux/tree/tpm
>
> Pull from here and merge the latest rc and you will probably have a
> bootable system.
>
> Jason

Applied this patch on v4.14-rc4. Able to get data from tpm rng
(/dev/hwrng with tpm as the chosen rng). This patch works fine. Its
just a basic test though.

Tested-by: PrasannaKumar Muralidharan 

Regards,
PrasannaKumar

Re: [PATCH v3] tpm: use struct tpm_chip for tpm_chip_find_get()

[PrasannaKumar Muralidharan]

Did basic check on tpm rng patch, it works fine. As it depends on this
patch this should be working fine too.

Tested-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>

Regards,
PrasannaKumar

Re: [PATCH v3] tpm: use struct tpm_chip for tpm_chip_find_get()

[PrasannaKumar Muralidharan]

Did basic check on tpm rng patch, it works fine. As it depends on this
patch this should be working fine too.

Tested-by: PrasannaKumar Muralidharan 

Regards,
PrasannaKumar