linux-omap.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [PATCH 14/23] memory: ti-emif-pm: Fix cast to iomem pointer
       [not found] ` <20200723073744.13400-15-krzk@kernel.org>
@ 2020-07-23  8:48   ` Arnd Bergmann
  2020-07-23  9:02     ` Krzysztof Kozlowski
  0 siblings, 1 reply; 19+ messages in thread
From: Arnd Bergmann @ 2020-07-23  8:48 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Olof Johansson, arm-soc, SoC Team, Markus Mayer,
	bcm-kernel-feedback-list, Florian Fainelli, Santosh Shilimkar,
	Matthias Brugger, Roger Quadros, Tony Lindgren,
	Vladimir Zapolskiy, Thierry Reding, Jonathan Hunter,
	linux-kernel, Linux ARM, moderated list:ARM/Mediatek SoC...,
	linux-omap, open list:TEGRA ARCHITECTURE SUPPORT, Andrew Morton,
	Linus Torvalds, Greg Kroah-Hartman

On Thu, Jul 23, 2020 at 9:39 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> Cast pointer to iomem memory properly to fix sparse warning:
>
>     drivers/memory/ti-emif-pm.c:251:38: warning: incorrect type in argument 1 (different address spaces)
>     drivers/memory/ti-emif-pm.c:251:38:    expected void const volatile [noderef] __iomem *addr
>     drivers/memory/ti-emif-pm.c:251:38:    got void *
>
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> ---
>  drivers/memory/ti-emif-pm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/memory/ti-emif-pm.c b/drivers/memory/ti-emif-pm.c
> index 9c90f815ad3a..6c747c1e98cb 100644
> --- a/drivers/memory/ti-emif-pm.c
> +++ b/drivers/memory/ti-emif-pm.c
> @@ -248,7 +248,7 @@ MODULE_DEVICE_TABLE(of, ti_emif_of_match);
>  static int ti_emif_resume(struct device *dev)
>  {
>         unsigned long tmp =
> -                       __raw_readl((void *)emif_instance->ti_emif_sram_virt);
> +                       __raw_readl((void __iomem *)emif_instance->ti_emif_sram_virt);
>

Maybe this shouldn't even be __raw_readl(), but instead READ_ONCE()?

The other accesses in this file don't use MMIO wrappers either but just treat
it as a pointer. The effect would be the same though.

      Arnd

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 18/23] memory: mtk-smi: Add argument to function definition
       [not found] ` <20200723073744.13400-19-krzk@kernel.org>
@ 2020-07-23  8:50   ` Arnd Bergmann
  2020-07-23  8:55     ` Krzysztof Kozlowski
  0 siblings, 1 reply; 19+ messages in thread
From: Arnd Bergmann @ 2020-07-23  8:50 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Olof Johansson, arm-soc, SoC Team, Markus Mayer,
	bcm-kernel-feedback-list, Florian Fainelli, Santosh Shilimkar,
	Matthias Brugger, Roger Quadros, Tony Lindgren,
	Vladimir Zapolskiy, Thierry Reding, Jonathan Hunter,
	linux-kernel, Linux ARM, moderated list:ARM/Mediatek SoC...,
	linux-omap, open list:TEGRA ARCHITECTURE SUPPORT, Andrew Morton,
	Linus Torvalds, Greg Kroah-Hartman

On Thu, Jul 23, 2020 at 9:39 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> Fix checkpatch warning:
>     WARNING: function definition argument 'struct device *' should also have an identifier name
>
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>

Is this a bug in checkpatch? I don't see why it should warn about this,
as there is no function definition here.

Your change is clearly harmless, but I wonder if we should fix
checkpatch instead.

      Arnd

> ---
>  drivers/memory/mtk-smi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/memory/mtk-smi.c b/drivers/memory/mtk-smi.c
> index 109c7e51d551..c21262502581 100644
> --- a/drivers/memory/mtk-smi.c
> +++ b/drivers/memory/mtk-smi.c
> @@ -60,7 +60,7 @@ struct mtk_smi_common_plat {
>
>  struct mtk_smi_larb_gen {
>         int port_in_larb[MTK_LARB_NR_MAX + 1];
> -       void (*config_port)(struct device *);
> +       void (*config_port)(struct device *dev);
>         unsigned int                    larb_direct_to_common_mask;
>         bool                            has_gals;
>  };
> --
> 2.17.1
>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 18/23] memory: mtk-smi: Add argument to function definition
  2020-07-23  8:50   ` [PATCH 18/23] memory: mtk-smi: Add argument to function definition Arnd Bergmann
@ 2020-07-23  8:55     ` Krzysztof Kozlowski
  0 siblings, 0 replies; 19+ messages in thread
From: Krzysztof Kozlowski @ 2020-07-23  8:55 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Olof Johansson, arm-soc, SoC Team, Markus Mayer,
	bcm-kernel-feedback-list, Florian Fainelli, Santosh Shilimkar,
	Matthias Brugger, Roger Quadros, Tony Lindgren,
	Vladimir Zapolskiy, Thierry Reding, Jonathan Hunter,
	linux-kernel, Linux ARM, moderated list:ARM/Mediatek SoC...,
	linux-omap, open list:TEGRA ARCHITECTURE SUPPORT, Andrew Morton,
	Linus Torvalds, Greg Kroah-Hartman

On Thu, Jul 23, 2020 at 10:50:08AM +0200, Arnd Bergmann wrote:
> On Thu, Jul 23, 2020 at 9:39 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >
> > Fix checkpatch warning:
> >     WARNING: function definition argument 'struct device *' should also have an identifier name
> >
> > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> 
> Is this a bug in checkpatch? I don't see why it should warn about this,
> as there is no function definition here.
> 
> Your change is clearly harmless, but I wonder if we should fix
> checkpatch instead.

Good point. If this were not a warning, I would ignore it. However still
the common practice is to add arguments to such type definitions as it
helps to describe the type.

Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 14/23] memory: ti-emif-pm: Fix cast to iomem pointer
  2020-07-23  8:48   ` [PATCH 14/23] memory: ti-emif-pm: Fix cast to iomem pointer Arnd Bergmann
@ 2020-07-23  9:02     ` Krzysztof Kozlowski
  2020-07-23  9:14       ` Arnd Bergmann
  0 siblings, 1 reply; 19+ messages in thread
From: Krzysztof Kozlowski @ 2020-07-23  9:02 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Olof Johansson, arm-soc, SoC Team, Markus Mayer,
	bcm-kernel-feedback-list, Florian Fainelli, Santosh Shilimkar,
	Matthias Brugger, Roger Quadros, Tony Lindgren,
	Vladimir Zapolskiy, Thierry Reding, Jonathan Hunter,
	linux-kernel, Linux ARM, moderated list:ARM/Mediatek SoC...,
	linux-omap, open list:TEGRA ARCHITECTURE SUPPORT, Andrew Morton,
	Linus Torvalds, Greg Kroah-Hartman

On Thu, Jul 23, 2020 at 10:48:19AM +0200, Arnd Bergmann wrote:
> On Thu, Jul 23, 2020 at 9:39 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >
> > Cast pointer to iomem memory properly to fix sparse warning:
> >
> >     drivers/memory/ti-emif-pm.c:251:38: warning: incorrect type in argument 1 (different address spaces)
> >     drivers/memory/ti-emif-pm.c:251:38:    expected void const volatile [noderef] __iomem *addr
> >     drivers/memory/ti-emif-pm.c:251:38:    got void *
> >
> > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> > ---
> >  drivers/memory/ti-emif-pm.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/memory/ti-emif-pm.c b/drivers/memory/ti-emif-pm.c
> > index 9c90f815ad3a..6c747c1e98cb 100644
> > --- a/drivers/memory/ti-emif-pm.c
> > +++ b/drivers/memory/ti-emif-pm.c
> > @@ -248,7 +248,7 @@ MODULE_DEVICE_TABLE(of, ti_emif_of_match);
> >  static int ti_emif_resume(struct device *dev)
> >  {
> >         unsigned long tmp =
> > -                       __raw_readl((void *)emif_instance->ti_emif_sram_virt);
> > +                       __raw_readl((void __iomem *)emif_instance->ti_emif_sram_virt);
> >
> 
> Maybe this shouldn't even be __raw_readl(), but instead READ_ONCE()?

Won't readl() be enough? Indeed it looks problematic.

> 
> The other accesses in this file don't use MMIO wrappers either but just treat
> it as a pointer. The effect would be the same though.

I think all the reads and writes are with readl() and writel().

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 19/23] memory: omap-gpmc: Enclose macro statements in do-while
       [not found] ` <20200723073744.13400-20-krzk@kernel.org>
@ 2020-07-23  9:09   ` Arnd Bergmann
  2020-07-23 10:16     ` Krzysztof Kozlowski
  0 siblings, 1 reply; 19+ messages in thread
From: Arnd Bergmann @ 2020-07-23  9:09 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Olof Johansson, arm-soc, SoC Team, Markus Mayer,
	bcm-kernel-feedback-list, Florian Fainelli, Santosh Shilimkar,
	Matthias Brugger, Roger Quadros, Tony Lindgren,
	Vladimir Zapolskiy, Thierry Reding, Jonathan Hunter,
	linux-kernel, Linux ARM, moderated list:ARM/Mediatek SoC...,
	linux-omap, open list:TEGRA ARCHITECTURE SUPPORT, Andrew Morton,
	Linus Torvalds, Greg Kroah-Hartman

On Thu, Jul 23, 2020 at 9:39 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> do-while is a preferred way for complex macros because of safety
> reasons.  This fixes checkpatch error:
>
>     ERROR: Macros starting with if should be enclosed by a do - while
>         loop to avoid possible if/else logic defects
>
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>

This is an improvement, but the macro still has other issues that
are just as bad as the one you address:

- Using the # operator to avoid the "" in the invocation seems confusing
- it implicitly uses the 'cs' and 't' variables of the calling function instead
  of passing them as arguments.
- it calls 'return -1' in a function that otherwise uses errno-style
  return codes, so this gets interpreted as EPERM "Operation not
  permitted".

I would probably just open-code the entire thing and remove the
macro like:

ret = 0;
ret |= set_gpmc_timing_reg(cs, GPMC_CS_CONFIG2,  0,  3, 0, t->cs_on,
GPMC_CD_FCLK, "cs_on");
ret |= set_gpmc_timing_reg(cs, GPMC_CS_CONFIG2,  8,  12, 0,
t->cs_rd_off, GPMC_CD_FCLK, "cs_rd_off");
ret |= set_gpmc_timing_reg(cs, GPMC_CS_CONFIG2,  16,  20, 0,
t->cs_wr_off, GPMC_CD_FCLK, "cs_wr_off);
...
if (ret)
     return -ENXIO;

Of maybe leave the macro, but remove the if/return part and use
the "ret |= GPMC_SET_ONE(...)" trick to avoid some of the problems.

      Arnd

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 20/23] memory: omap-gpmc: Fix whitespace issue
       [not found] ` <20200723073744.13400-21-krzk@kernel.org>
@ 2020-07-23  9:11   ` Arnd Bergmann
  2020-07-23 10:08     ` Krzysztof Kozlowski
  0 siblings, 1 reply; 19+ messages in thread
From: Arnd Bergmann @ 2020-07-23  9:11 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Olof Johansson, arm-soc, SoC Team, Markus Mayer,
	bcm-kernel-feedback-list, Florian Fainelli, Santosh Shilimkar,
	Matthias Brugger, Roger Quadros, Tony Lindgren,
	Vladimir Zapolskiy, Thierry Reding, Jonathan Hunter,
	linux-kernel, Linux ARM, moderated list:ARM/Mediatek SoC...,
	linux-omap, open list:TEGRA ARCHITECTURE SUPPORT, Andrew Morton,
	Linus Torvalds, Greg Kroah-Hartman

On Thu, Jul 23, 2020 at 9:39 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:

> @@ -1756,7 +1756,7 @@ static int gpmc_calc_common_timings(struct gpmc_timings *gpmc_t,
>  /* TODO: remove this function once all peripherals are confirmed to
>   * work with generic timing. Simultaneously gpmc_cs_set_timings()
>   * has to be modified to handle timings in ps instead of ns
> -*/
> + */

This still doesn't follow the normal coding style for multi-line comments.

I don't think it's worth fixing, but if you think it should be changed, then
make it

     /*
      * mult-line comment
      * ...
      */

     Arnd

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 14/23] memory: ti-emif-pm: Fix cast to iomem pointer
  2020-07-23  9:02     ` Krzysztof Kozlowski
@ 2020-07-23  9:14       ` Arnd Bergmann
  2020-07-23 10:01         ` Krzysztof Kozlowski
  0 siblings, 1 reply; 19+ messages in thread
From: Arnd Bergmann @ 2020-07-23  9:14 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Olof Johansson, arm-soc, SoC Team, Markus Mayer,
	bcm-kernel-feedback-list, Florian Fainelli, Santosh Shilimkar,
	Matthias Brugger, Roger Quadros, Tony Lindgren,
	Vladimir Zapolskiy, Thierry Reding, Jonathan Hunter,
	linux-kernel, Linux ARM, moderated list:ARM/Mediatek SoC...,
	linux-omap, open list:TEGRA ARCHITECTURE SUPPORT, Andrew Morton,
	Linus Torvalds, Greg Kroah-Hartman

On Thu, Jul 23, 2020 at 11:02 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On Thu, Jul 23, 2020 at 10:48:19AM +0200, Arnd Bergmann wrote:
> > On Thu, Jul 23, 2020 at 9:39 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > >
> > > Cast pointer to iomem memory properly to fix sparse warning:
> > >
> > >     drivers/memory/ti-emif-pm.c:251:38: warning: incorrect type in argument 1 (different address spaces)
> > >     drivers/memory/ti-emif-pm.c:251:38:    expected void const volatile [noderef] __iomem *addr
> > >     drivers/memory/ti-emif-pm.c:251:38:    got void *
> > >
> > > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> > > ---
> > >  drivers/memory/ti-emif-pm.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/memory/ti-emif-pm.c b/drivers/memory/ti-emif-pm.c
> > > index 9c90f815ad3a..6c747c1e98cb 100644
> > > --- a/drivers/memory/ti-emif-pm.c
> > > +++ b/drivers/memory/ti-emif-pm.c
> > > @@ -248,7 +248,7 @@ MODULE_DEVICE_TABLE(of, ti_emif_of_match);
> > >  static int ti_emif_resume(struct device *dev)
> > >  {
> > >         unsigned long tmp =
> > > -                       __raw_readl((void *)emif_instance->ti_emif_sram_virt);
> > > +                       __raw_readl((void __iomem *)emif_instance->ti_emif_sram_virt);
> > >
> >
> > Maybe this shouldn't even be __raw_readl(), but instead READ_ONCE()?
>
> Won't readl() be enough? Indeed it looks problematic.

readl() won't work on big-endian kernels, since this is a byte comparison.

> > The other accesses in this file don't use MMIO wrappers either but just treat
> > it as a pointer. The effect would be the same though.
>
> I think all the reads and writes are with readl() and writel().

I actually see only one other access:

        copy_addr = sram_exec_copy(emif_data->sram_pool_code,
                                   (void *)emif_data->ti_emif_sram_virt,
                                   &ti_emif_sram, ti_emif_sram_sz);

and this one ends up in a memcpy() that does not perform any byte
swapping or barriers.

     Arnd

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [RFC PATCH 00/23] memory: Cleanup, improve and compile test memory drivers
       [not found] <20200723073744.13400-1-krzk@kernel.org>
                   ` (3 preceding siblings ...)
       [not found] ` <20200723073744.13400-21-krzk@kernel.org>
@ 2020-07-23  9:31 ` Arnd Bergmann
  2020-07-23  9:52   ` Krzysztof Kozlowski
       [not found] ` <20200723073744.13400-17-krzk@kernel.org>
                   ` (2 subsequent siblings)
  7 siblings, 1 reply; 19+ messages in thread
From: Arnd Bergmann @ 2020-07-23  9:31 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Olof Johansson, arm-soc, SoC Team, Markus Mayer,
	bcm-kernel-feedback-list, Florian Fainelli, Santosh Shilimkar,
	Matthias Brugger, Roger Quadros, Tony Lindgren,
	Vladimir Zapolskiy, Thierry Reding, Jonathan Hunter,
	linux-kernel, Linux ARM, moderated list:ARM/Mediatek SoC...,
	linux-omap, open list:TEGRA ARCHITECTURE SUPPORT, Andrew Morton,
	Linus Torvalds, Greg Kroah-Hartman

On Thu, Jul 23, 2020 at 9:37 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> Dear All,
>
> The drivers/memory directory contains generic code (of_memory.c) and a
> bunch of drivers.  Changes to generic code were coming usually through
> different trees with the driver code.
>
> Over last days, memory drivers grew in numbers but not necessarily in
> quality.  They lacked compile testing and code cleanup.  Also lacked
> maintainer.
>
> I would be happy to take care about this part.
>
> If there are no objections, the patches could go either to Linus or to
> arm-soc (most of drivers are ARM specific).
>
> Driver-specific changes in the patchset were only compile-tested. Tests
> are welcome. The generic code was tested on ARMv7 Exynos based boards
> with a exynos5422-dmc memory controller driver.

Overall this looks great, I had a look through the patches and commented
on the few things that seemed slightly odd though harmless.

Thanks for picking up the subsystem. How do you want to proceed
in the long run? I suppose you can send a pull request to soc@kernel.org
to be picked up for the coming merge window as the normal way, since
you are not yet listed as the maintained until the end of the series.

Afterwards you could either send the pull requests to Linus directly,
or send them to the soc team (or to Greg) as well, the way we handle
a couple of other subsystems like drivers/reset and drivers/tee that
usually only have a handful of patches per release.

     Arnd

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [RFC PATCH 00/23] memory: Cleanup, improve and compile test memory drivers
  2020-07-23  9:31 ` [RFC PATCH 00/23] memory: Cleanup, improve and compile test memory drivers Arnd Bergmann
@ 2020-07-23  9:52   ` Krzysztof Kozlowski
  2020-07-23 10:14     ` Arnd Bergmann
  0 siblings, 1 reply; 19+ messages in thread
From: Krzysztof Kozlowski @ 2020-07-23  9:52 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Olof Johansson, arm-soc, SoC Team, Markus Mayer,
	bcm-kernel-feedback-list, Florian Fainelli, Santosh Shilimkar,
	Matthias Brugger, Roger Quadros, Tony Lindgren,
	Vladimir Zapolskiy, Thierry Reding, Jonathan Hunter,
	linux-kernel, Linux ARM, moderated list:ARM/Mediatek SoC...,
	linux-omap, open list:TEGRA ARCHITECTURE SUPPORT, Andrew Morton,
	Linus Torvalds, Greg Kroah-Hartman

On Thu, Jul 23, 2020 at 11:31:02AM +0200, Arnd Bergmann wrote:
> On Thu, Jul 23, 2020 at 9:37 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >
> > Dear All,
> >
> > The drivers/memory directory contains generic code (of_memory.c) and a
> > bunch of drivers.  Changes to generic code were coming usually through
> > different trees with the driver code.
> >
> > Over last days, memory drivers grew in numbers but not necessarily in
> > quality.  They lacked compile testing and code cleanup.  Also lacked
> > maintainer.
> >
> > I would be happy to take care about this part.
> >
> > If there are no objections, the patches could go either to Linus or to
> > arm-soc (most of drivers are ARM specific).
> >
> > Driver-specific changes in the patchset were only compile-tested. Tests
> > are welcome. The generic code was tested on ARMv7 Exynos based boards
> > with a exynos5422-dmc memory controller driver.
> 
> Overall this looks great, I had a look through the patches and commented
> on the few things that seemed slightly odd though harmless.
> 
> Thanks for picking up the subsystem. How do you want to proceed
> in the long run? I suppose you can send a pull request to soc@kernel.org
> to be picked up for the coming merge window as the normal way, since
> you are not yet listed as the maintained until the end of the series.
> 
> Afterwards you could either send the pull requests to Linus directly,
> or send them to the soc team (or to Greg) as well, the way we handle
> a couple of other subsystems like drivers/reset and drivers/tee that
> usually only have a handful of patches per release.

Most of the drivers are for ARM architecture so arm-soc seems like the
way to do it.  However BT1_L2_CTL and JZ4780_NEMC are MIPS specific and
maybe more would come in the future.  Are you fine taking them as well?

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 14/23] memory: ti-emif-pm: Fix cast to iomem pointer
  2020-07-23  9:14       ` Arnd Bergmann
@ 2020-07-23 10:01         ` Krzysztof Kozlowski
  0 siblings, 0 replies; 19+ messages in thread
From: Krzysztof Kozlowski @ 2020-07-23 10:01 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Olof Johansson, arm-soc, SoC Team, Markus Mayer,
	bcm-kernel-feedback-list, Florian Fainelli, Santosh Shilimkar,
	Matthias Brugger, Roger Quadros, Tony Lindgren,
	Vladimir Zapolskiy, Thierry Reding, Jonathan Hunter,
	linux-kernel, Linux ARM, moderated list:ARM/Mediatek SoC...,
	linux-omap, open list:TEGRA ARCHITECTURE SUPPORT, Andrew Morton,
	Linus Torvalds, Greg Kroah-Hartman

On Thu, Jul 23, 2020 at 11:14:02AM +0200, Arnd Bergmann wrote:
> On Thu, Jul 23, 2020 at 11:02 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > On Thu, Jul 23, 2020 at 10:48:19AM +0200, Arnd Bergmann wrote:
> > > On Thu, Jul 23, 2020 at 9:39 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > > >
> > > > Cast pointer to iomem memory properly to fix sparse warning:
> > > >
> > > >     drivers/memory/ti-emif-pm.c:251:38: warning: incorrect type in argument 1 (different address spaces)
> > > >     drivers/memory/ti-emif-pm.c:251:38:    expected void const volatile [noderef] __iomem *addr
> > > >     drivers/memory/ti-emif-pm.c:251:38:    got void *
> > > >
> > > > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> > > > ---
> > > >  drivers/memory/ti-emif-pm.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/memory/ti-emif-pm.c b/drivers/memory/ti-emif-pm.c
> > > > index 9c90f815ad3a..6c747c1e98cb 100644
> > > > --- a/drivers/memory/ti-emif-pm.c
> > > > +++ b/drivers/memory/ti-emif-pm.c
> > > > @@ -248,7 +248,7 @@ MODULE_DEVICE_TABLE(of, ti_emif_of_match);
> > > >  static int ti_emif_resume(struct device *dev)
> > > >  {
> > > >         unsigned long tmp =
> > > > -                       __raw_readl((void *)emif_instance->ti_emif_sram_virt);
> > > > +                       __raw_readl((void __iomem *)emif_instance->ti_emif_sram_virt);
> > > >
> > >
> > > Maybe this shouldn't even be __raw_readl(), but instead READ_ONCE()?
> >
> > Won't readl() be enough? Indeed it looks problematic.
> 
> readl() won't work on big-endian kernels, since this is a byte comparison.

Ah, right.

> 
> > > The other accesses in this file don't use MMIO wrappers either but just treat
> > > it as a pointer. The effect would be the same though.
> >
> > I think all the reads and writes are with readl() and writel().
> 
> I actually see only one other access:
> 
>         copy_addr = sram_exec_copy(emif_data->sram_pool_code,
>                                    (void *)emif_data->ti_emif_sram_virt,
>                                    &ti_emif_sram, ti_emif_sram_sz);
> 
> and this one ends up in a memcpy() that does not perform any byte
> swapping or barriers.

At least the barrier would come through mutex in sram_exec_copy() and
later spin locks for page table manipulation.

Anyway, I do not have the HW to test the changes or to confirm whether
this is real issue.  I guess the driver author/owner should follow up on
this report.

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 20/23] memory: omap-gpmc: Fix whitespace issue
  2020-07-23  9:11   ` [PATCH 20/23] memory: omap-gpmc: Fix whitespace issue Arnd Bergmann
@ 2020-07-23 10:08     ` Krzysztof Kozlowski
  0 siblings, 0 replies; 19+ messages in thread
From: Krzysztof Kozlowski @ 2020-07-23 10:08 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Olof Johansson, arm-soc, SoC Team, Markus Mayer,
	bcm-kernel-feedback-list, Florian Fainelli, Santosh Shilimkar,
	Matthias Brugger, Roger Quadros, Tony Lindgren,
	Vladimir Zapolskiy, Thierry Reding, Jonathan Hunter,
	linux-kernel, Linux ARM, moderated list:ARM/Mediatek SoC...,
	linux-omap, open list:TEGRA ARCHITECTURE SUPPORT, Andrew Morton,
	Linus Torvalds, Greg Kroah-Hartman

On Thu, Jul 23, 2020 at 11:11:08AM +0200, Arnd Bergmann wrote:
> On Thu, Jul 23, 2020 at 9:39 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> 
> > @@ -1756,7 +1756,7 @@ static int gpmc_calc_common_timings(struct gpmc_timings *gpmc_t,
> >  /* TODO: remove this function once all peripherals are confirmed to
> >   * work with generic timing. Simultaneously gpmc_cs_set_timings()
> >   * has to be modified to handle timings in ps instead of ns
> > -*/
> > + */
> 
> This still doesn't follow the normal coding style for multi-line comments.
> 
> I don't think it's worth fixing, but if you think it should be changed, then
> make it
> 
>      /*
>       * mult-line comment
>       * ...
>       */

Sure, thanks for feedback.

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [RFC PATCH 00/23] memory: Cleanup, improve and compile test memory drivers
  2020-07-23  9:52   ` Krzysztof Kozlowski
@ 2020-07-23 10:14     ` Arnd Bergmann
  0 siblings, 0 replies; 19+ messages in thread
From: Arnd Bergmann @ 2020-07-23 10:14 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Olof Johansson, arm-soc, SoC Team, Markus Mayer,
	bcm-kernel-feedback-list, Florian Fainelli, Santosh Shilimkar,
	Matthias Brugger, Roger Quadros, Tony Lindgren,
	Vladimir Zapolskiy, Thierry Reding, Jonathan Hunter,
	linux-kernel, Linux ARM, moderated list:ARM/Mediatek SoC...,
	linux-omap, open list:TEGRA ARCHITECTURE SUPPORT, Andrew Morton,
	Linus Torvalds, Greg Kroah-Hartman

On Thu, Jul 23, 2020 at 11:52 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On Thu, Jul 23, 2020 at 11:31:02AM +0200, Arnd Bergmann wrote:
> > On Thu, Jul 23, 2020 at 9:37 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >
> > Afterwards you could either send the pull requests to Linus directly,
> > or send them to the soc team (or to Greg) as well, the way we handle
> > a couple of other subsystems like drivers/reset and drivers/tee that
> > usually only have a handful of patches per release.
>
> Most of the drivers are for ARM architecture so arm-soc seems like the
> way to do it.  However BT1_L2_CTL and JZ4780_NEMC are MIPS specific and
> maybe more would come in the future.  Are you fine taking them as well?

Yes, that's not a problem at all. Most other architectures are ramping down
anyway, both on the maintainership side and on newly supported hardware,
so we are picking those up where necessary. I also merged a couple of
drivers for the MIPS based Baikal SoCs recently.

     Arnd

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 19/23] memory: omap-gpmc: Enclose macro statements in do-while
  2020-07-23  9:09   ` [PATCH 19/23] memory: omap-gpmc: Enclose macro statements in do-while Arnd Bergmann
@ 2020-07-23 10:16     ` Krzysztof Kozlowski
  0 siblings, 0 replies; 19+ messages in thread
From: Krzysztof Kozlowski @ 2020-07-23 10:16 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Olof Johansson, arm-soc, SoC Team, Markus Mayer,
	bcm-kernel-feedback-list, Florian Fainelli, Santosh Shilimkar,
	Matthias Brugger, Roger Quadros, Tony Lindgren,
	Vladimir Zapolskiy, Thierry Reding, Jonathan Hunter,
	linux-kernel, Linux ARM, moderated list:ARM/Mediatek SoC...,
	linux-omap, open list:TEGRA ARCHITECTURE SUPPORT, Andrew Morton,
	Linus Torvalds, Greg Kroah-Hartman

On Thu, Jul 23, 2020 at 11:09:40AM +0200, Arnd Bergmann wrote:
> On Thu, Jul 23, 2020 at 9:39 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >
> > do-while is a preferred way for complex macros because of safety
> > reasons.  This fixes checkpatch error:
> >
> >     ERROR: Macros starting with if should be enclosed by a do - while
> >         loop to avoid possible if/else logic defects
> >
> > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> 
> This is an improvement, but the macro still has other issues that
> are just as bad as the one you address:
> 
> - Using the # operator to avoid the "" in the invocation seems confusing

I guess it was useful for debugging.

> - it implicitly uses the 'cs' and 't' variables of the calling function instead
>   of passing them as arguments.

Actually another reason to convert it to just a function.

> - it calls 'return -1' in a function that otherwise uses errno-style
>   return codes, so this gets interpreted as EPERM "Operation not
>   permitted".

The users of this macro also do it (gpmc_cs_set_timings()) so this
wrong practice is consistent with the driver. :)

> 
> I would probably just open-code the entire thing and remove the
> macro like:
> 
> ret = 0;
> ret |= set_gpmc_timing_reg(cs, GPMC_CS_CONFIG2,  0,  3, 0, t->cs_on,
> GPMC_CD_FCLK, "cs_on");
> ret |= set_gpmc_timing_reg(cs, GPMC_CS_CONFIG2,  8,  12, 0,
> t->cs_rd_off, GPMC_CD_FCLK, "cs_rd_off");
> ret |= set_gpmc_timing_reg(cs, GPMC_CS_CONFIG2,  16,  20, 0,
> t->cs_wr_off, GPMC_CD_FCLK, "cs_wr_off);
> ...
> if (ret)
>      return -ENXIO;a

I like this approach because it also removes the 'return' from macro
which is not desired.

> 
> Of maybe leave the macro, but remove the if/return part and use
> the "ret |= GPMC_SET_ONE(...)" trick to avoid some of the problems.

I could probably then keep it as a function.  This would be the safest
and remove most of the problems here.

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 16/23] memory: brcmstb_dpfe: Constify the contents of string
       [not found] ` <20200723073744.13400-17-krzk@kernel.org>
@ 2020-07-23 17:11   ` Florian Fainelli
  2020-07-23 17:15     ` Markus Mayer
  0 siblings, 1 reply; 19+ messages in thread
From: Florian Fainelli @ 2020-07-23 17:11 UTC (permalink / raw)
  To: Krzysztof Kozlowski, Arnd Bergmann, Olof Johansson, arm, soc,
	Markus Mayer, bcm-kernel-feedback-list, Florian Fainelli,
	Santosh Shilimkar, Matthias Brugger, Roger Quadros,
	Tony Lindgren, Vladimir Zapolskiy, Thierry Reding,
	Jonathan Hunter, linux-kernel, linux-arm-kernel, linux-mediatek,
	linux-omap, linux-tegra
  Cc: Andrew Morton, Linus Torvalds, Greg Kroah-Hartman

On 7/23/20 12:37 AM, Krzysztof Kozlowski wrote:
> The string itself can be made const for safety.
> 
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>

Acked-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 17/23] memory: brcmstb_dpfe: Remove unneeded braces
       [not found] ` <20200723073744.13400-18-krzk@kernel.org>
@ 2020-07-23 17:11   ` Florian Fainelli
  2020-07-23 17:15     ` Markus Mayer
  0 siblings, 1 reply; 19+ messages in thread
From: Florian Fainelli @ 2020-07-23 17:11 UTC (permalink / raw)
  To: Krzysztof Kozlowski, Arnd Bergmann, Olof Johansson, arm, soc,
	Markus Mayer, bcm-kernel-feedback-list, Florian Fainelli,
	Santosh Shilimkar, Matthias Brugger, Roger Quadros,
	Tony Lindgren, Vladimir Zapolskiy, Thierry Reding,
	Jonathan Hunter, linux-kernel, linux-arm-kernel, linux-mediatek,
	linux-omap, linux-tegra
  Cc: Andrew Morton, Linus Torvalds, Greg Kroah-Hartman

On 7/23/20 12:37 AM, Krzysztof Kozlowski wrote:
> Single statement blocks don't need braces.  Fixes checkpatch warning:
> 
>     WARNING: braces {} are not necessary for single statement blocks
> 
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>

Acked-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 23/23] MAINTAINERS: Add Krzysztof Kozlowski as maintainer of memory controllers
       [not found] ` <20200723073744.13400-24-krzk@kernel.org>
@ 2020-07-23 17:12   ` Florian Fainelli
  2020-07-24  7:15     ` Krzysztof Kozlowski
  0 siblings, 1 reply; 19+ messages in thread
From: Florian Fainelli @ 2020-07-23 17:12 UTC (permalink / raw)
  To: Krzysztof Kozlowski, Arnd Bergmann, Olof Johansson, arm, soc,
	Markus Mayer, bcm-kernel-feedback-list, Florian Fainelli,
	Santosh Shilimkar, Matthias Brugger, Roger Quadros,
	Tony Lindgren, Vladimir Zapolskiy, Thierry Reding,
	Jonathan Hunter, linux-kernel, linux-arm-kernel, linux-mediatek,
	linux-omap, linux-tegra
  Cc: Andrew Morton, Linus Torvalds, Greg Kroah-Hartman

On 7/23/20 12:37 AM, Krzysztof Kozlowski wrote:
> The specific drivers in drivers/memory usually go via architecture (e.g.
> ARM SoC) maintainers but the generic parts (of_memory.[ch]) lacked any
> care.
> 
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>

Acked-by: Florian Fainelli <f.fainelli@gmail.com>

What does this mean for specific drivers? For instance I tend to send
updates to brcmstb_dpfe.c through the Broadcom ARM SoC pull requests,
shall I continue to do this, or are you going to send memory controller
subsystem pull requests including that file in the future?
-- 
Florian

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 17/23] memory: brcmstb_dpfe: Remove unneeded braces
  2020-07-23 17:11   ` [PATCH 17/23] memory: brcmstb_dpfe: Remove unneeded braces Florian Fainelli
@ 2020-07-23 17:15     ` Markus Mayer
  0 siblings, 0 replies; 19+ messages in thread
From: Markus Mayer @ 2020-07-23 17:15 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: Krzysztof Kozlowski, Arnd Bergmann, Olof Johansson, arm, soc,
	BCM Kernel Feedback, Santosh Shilimkar, Matthias Brugger,
	Roger Quadros, Tony Lindgren, Vladimir Zapolskiy, Thierry Reding,
	Jonathan Hunter, Linux Kernel, Linux ARM Kernel, linux-mediatek,
	linux-omap, linux-tegra, Andrew Morton, Linus Torvalds,
	Greg Kroah-Hartman

On Thu, 23 Jul 2020 at 10:11, Florian Fainelli <f.fainelli@gmail.com> wrote:
>
> On 7/23/20 12:37 AM, Krzysztof Kozlowski wrote:
> > Single statement blocks don't need braces.  Fixes checkpatch warning:
> >
> >     WARNING: braces {} are not necessary for single statement blocks
> >
> > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
>
> Acked-by: Florian Fainelli <f.fainelli@gmail.com>

Acked-by: Markus Mayer <mmayer@broadcom.com>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 16/23] memory: brcmstb_dpfe: Constify the contents of string
  2020-07-23 17:11   ` [PATCH 16/23] memory: brcmstb_dpfe: Constify the contents of string Florian Fainelli
@ 2020-07-23 17:15     ` Markus Mayer
  0 siblings, 0 replies; 19+ messages in thread
From: Markus Mayer @ 2020-07-23 17:15 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: Krzysztof Kozlowski, Arnd Bergmann, Olof Johansson, arm, soc,
	BCM Kernel Feedback, Santosh Shilimkar, Matthias Brugger,
	Roger Quadros, Tony Lindgren, Vladimir Zapolskiy, Thierry Reding,
	Jonathan Hunter, Linux Kernel, Linux ARM Kernel, linux-mediatek,
	linux-omap, linux-tegra, Andrew Morton, Linus Torvalds,
	Greg Kroah-Hartman

On Thu, 23 Jul 2020 at 10:11, Florian Fainelli <f.fainelli@gmail.com> wrote:
>
> On 7/23/20 12:37 AM, Krzysztof Kozlowski wrote:
> > The string itself can be made const for safety.
> >
> > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
>
> Acked-by: Florian Fainelli <f.fainelli@gmail.com>

Acked-by: Markus Mayer <mmayer@broadcom.com>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 23/23] MAINTAINERS: Add Krzysztof Kozlowski as maintainer of memory controllers
  2020-07-23 17:12   ` [PATCH 23/23] MAINTAINERS: Add Krzysztof Kozlowski as maintainer of memory controllers Florian Fainelli
@ 2020-07-24  7:15     ` Krzysztof Kozlowski
  0 siblings, 0 replies; 19+ messages in thread
From: Krzysztof Kozlowski @ 2020-07-24  7:15 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: Arnd Bergmann, Olof Johansson, arm, soc, Markus Mayer,
	bcm-kernel-feedback-list, Santosh Shilimkar, Matthias Brugger,
	Roger Quadros, Tony Lindgren, Vladimir Zapolskiy, Thierry Reding,
	Jonathan Hunter, linux-kernel, linux-arm-kernel, linux-mediatek,
	linux-omap, linux-tegra, Andrew Morton, Linus Torvalds,
	Greg Kroah-Hartman

On Thu, Jul 23, 2020 at 10:12:36AM -0700, Florian Fainelli wrote:
> On 7/23/20 12:37 AM, Krzysztof Kozlowski wrote:
> > The specific drivers in drivers/memory usually go via architecture (e.g.
> > ARM SoC) maintainers but the generic parts (of_memory.[ch]) lacked any
> > care.
> > 
> > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> 
> Acked-by: Florian Fainelli <f.fainelli@gmail.com>
> 
> What does this mean for specific drivers? For instance I tend to send
> updates to brcmstb_dpfe.c through the Broadcom ARM SoC pull requests,
> shall I continue to do this, or are you going to send memory controller
> subsystem pull requests including that file in the future?

If that's okay, I intend to take the drivers patches as well. I will
rephrase the commit message.

Thanks for the feddback.

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2020-07-24  7:15 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20200723073744.13400-1-krzk@kernel.org>
     [not found] ` <20200723073744.13400-15-krzk@kernel.org>
2020-07-23  8:48   ` [PATCH 14/23] memory: ti-emif-pm: Fix cast to iomem pointer Arnd Bergmann
2020-07-23  9:02     ` Krzysztof Kozlowski
2020-07-23  9:14       ` Arnd Bergmann
2020-07-23 10:01         ` Krzysztof Kozlowski
     [not found] ` <20200723073744.13400-19-krzk@kernel.org>
2020-07-23  8:50   ` [PATCH 18/23] memory: mtk-smi: Add argument to function definition Arnd Bergmann
2020-07-23  8:55     ` Krzysztof Kozlowski
     [not found] ` <20200723073744.13400-20-krzk@kernel.org>
2020-07-23  9:09   ` [PATCH 19/23] memory: omap-gpmc: Enclose macro statements in do-while Arnd Bergmann
2020-07-23 10:16     ` Krzysztof Kozlowski
     [not found] ` <20200723073744.13400-21-krzk@kernel.org>
2020-07-23  9:11   ` [PATCH 20/23] memory: omap-gpmc: Fix whitespace issue Arnd Bergmann
2020-07-23 10:08     ` Krzysztof Kozlowski
2020-07-23  9:31 ` [RFC PATCH 00/23] memory: Cleanup, improve and compile test memory drivers Arnd Bergmann
2020-07-23  9:52   ` Krzysztof Kozlowski
2020-07-23 10:14     ` Arnd Bergmann
     [not found] ` <20200723073744.13400-17-krzk@kernel.org>
2020-07-23 17:11   ` [PATCH 16/23] memory: brcmstb_dpfe: Constify the contents of string Florian Fainelli
2020-07-23 17:15     ` Markus Mayer
     [not found] ` <20200723073744.13400-18-krzk@kernel.org>
2020-07-23 17:11   ` [PATCH 17/23] memory: brcmstb_dpfe: Remove unneeded braces Florian Fainelli
2020-07-23 17:15     ` Markus Mayer
     [not found] ` <20200723073744.13400-24-krzk@kernel.org>
2020-07-23 17:12   ` [PATCH 23/23] MAINTAINERS: Add Krzysztof Kozlowski as maintainer of memory controllers Florian Fainelli
2020-07-24  7:15     ` Krzysztof Kozlowski

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).