Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1411738 > unrolled thread

Re: [PATCH 4/4] Revert "powerpc/fsl: Move fsl_guts.h out of arch/powerpc"

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 4/4] Revert "powerpc/fsl: Move fsl_guts.h out of  arch/powerpc" Scott Wood <oss@buserror.net> - 2016-06-02 03:30 +0200
    Re: [PATCH 4/4] Revert "powerpc/fsl: Move fsl_guts.h out of arch/powerpc" Arnd Bergmann <arnd@arndb.de> - 2016-06-02 13:50 +0200
      Re: [PATCH 4/4] Revert "powerpc/fsl: Move fsl_guts.h out of  arch/powerpc" Scott Wood <oss@buserror.net> - 2016-06-11 04:20 +0200

#1411738 — Re: [PATCH 4/4] Revert "powerpc/fsl: Move fsl_guts.h out of arch/powerpc"

On Mon, 2016-05-30 at 15:18 +0200, Arnd Bergmann wrote:
> All users of this driver are PowerPC specific and the header file
> has no business in the global include/linux/ hierarchy, so move
> it back before anyone starts using it on ARM.
> 
> This reverts commit 948486544713492f00ac8a9572909101ea892cb0.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> This part of the series is not required for the eSDHC quirk,
> but it restores the asm/fsl_guts.h header so it doesn't accidentally
> get abused for this in the future. I found two drivers outside of
> arch/powerpc that already accessed the registers directly, but the
> functions look fairly contained, and can be easily hidden in an
> #ifdef CONFIG_PPC

NACK

Besides adding ifdef pollution for no good reason, this register block is used
on some ARM chips as well.  Why is it a problem if "anyone starts using it on
ARM"?

BTW, of all the mailing lists you included on this CC, you seem to have left
off the PPC list (I've added it).

-Scott

[toc] | [next] | [standalone]

#1412148 — Re: [PATCH 4/4] Revert "powerpc/fsl: Move fsl_guts.h out of arch/powerpc"

On Wednesday, June 1, 2016 8:24:20 PM CEST Scott Wood wrote:
> On Mon, 2016-05-30 at 15:18 +0200, Arnd Bergmann wrote:
> > All users of this driver are PowerPC specific and the header file
> > has no business in the global include/linux/ hierarchy, so move
> > it back before anyone starts using it on ARM.
> > 
> > This reverts commit 948486544713492f00ac8a9572909101ea892cb0.
> > 
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > ---
> > This part of the series is not required for the eSDHC quirk,
> > but it restores the asm/fsl_guts.h header so it doesn't accidentally
> > get abused for this in the future. I found two drivers outside of
> > arch/powerpc that already accessed the registers directly, but the
> > functions look fairly contained, and can be easily hidden in an
> > #ifdef CONFIG_PPC
> 
> NACK
> 
> Besides adding ifdef pollution for no good reason, this register block is used
> on some ARM chips as well.  Why is it a problem if "anyone starts using it on
> ARM"?

It's just not a good interface when it's defined as "this is the layout of
a register area that any driver can ioremap() if they can figure out the
device node". It's not uncommon to have register areas like that, but
normally you have at the minimum a 'syscon' device to handle locking
between drivers accessing the same registers and to avoid having to map
the same area multiple times.

If we need to use 'guts' registers on ARM, we can find a way to abstract
them properly for the given use cases, using a syscon or a driver with
exported functions, but just making a PowerPC platform specific header
global to all Linux drivers by putting it into include/linux doesn't seem
right.

Note that the header file uses a structure definition rather than the more
common macros with register offsets, which is fine for a driver that has
its own registers and abstracts them, but it doesn't really work with
the regmap interface, so if we want to use it with syscon, it also needs to
be rewritten.

> BTW, of all the mailing lists you included on this CC, you seem to have left
> off the PPC list (I've added it).

Sorry, my mistake.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1419892

On Thu, 2016-06-02 at 11:01 +0200, Arnd Bergmann wrote:
> On Wednesday, June 1, 2016 8:24:20 PM CEST Scott Wood wrote:
> > On Mon, 2016-05-30 at 15:18 +0200, Arnd Bergmann wrote:
> > > All users of this driver are PowerPC specific and the header file
> > > has no business in the global include/linux/ hierarchy, so move
> > > it back before anyone starts using it on ARM.
> > > 
> > > This reverts commit 948486544713492f00ac8a9572909101ea892cb0.
> > > 
> > > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > > ---
> > > This part of the series is not required for the eSDHC quirk,
> > > but it restores the asm/fsl_guts.h header so it doesn't accidentally
> > > get abused for this in the future. I found two drivers outside of
> > > arch/powerpc that already accessed the registers directly, but the
> > > functions look fairly contained, and can be easily hidden in an
> > > #ifdef CONFIG_PPC
> > 
> > NACK
> > 
> > Besides adding ifdef pollution for no good reason, this register block is
> > used
> > on some ARM chips as well.  Why is it a problem if "anyone starts using it
> > on
> > ARM"?
> 
> It's just not a good interface when it's defined as "this is the layout of
> a register area that any driver can ioremap() if they can figure out the
> device node".

That's why I want to move accesses into one guts driver.

>  It's not uncommon to have register areas like that, but
> normally you have at the minimum a 'syscon' device to handle locking
> between drivers accessing the same registers and to avoid having to map
> the same area multiple times.

syscon requires device tree changes.

I don't see read-modify-write operations in regmap -- how does locking around
an individual, inherently-atomic load or store help?

> If we need to use 'guts' registers on ARM, we can find a way to abstract
> them properly for the given use cases, using a syscon or a driver with
> exported functions, but just making a PowerPC platform specific header
> global to all Linux drivers by putting it into include/linux doesn't seem
> right.

Again, it's not PowerPC-specific!  It started that way but then the same
register block got put onto some ARM chips.

It's not global to "all Linux drivers", just the ones that choose to include
an fsl-specific header.

If and when all uses of guts are moved into the guts driver, the header can be
moved into drivers/soc/fsl.

> Note that the header file uses a structure definition rather than the more
> common macros with register offsets, which is fine for a driver that has
> its own registers and abstracts them, but it doesn't really work with
> the regmap interface, so if we want to use it with syscon, it also needs to
> be rewritten.

We don't want to use it with syscon.  If we did, the solution wouldn't be to
move the header back to arch/powerpc, but to convert the struct into offsets.

-Scott

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web