mach header files

mach header files

[Phil Edworthy]

Hi,

I am porting the kernel to a new device, for which I've created a new arch/arm/mach-... directory, and I also a clock driver that lives under driver/clk. Everything is all working fine, though I am now cleaning up the code and have a question about mach specific header files.

The clock driver is completely specific to this device, but needs to read from a system register (just for external boot mode pins) to determine some PLL settings. This register is in a block of system registers which are also used by the mach code in arch/arm/mach-...

Since the clock driver is specific to the mach, is there any point in specifying the address of this reg in the corresponding dtsi? The format and functionality described by this register would not be the same on any other device.

If I don't specify the address of the register in the dtsi, I think it would be best to have a common header file for all of the system registers. I've seen some drivers, e.g.  exynos-cpufreq.c, doing this by including files from mach-exynos/include/mach. Is that the right way to do this?

Thanks
Phil


This message is intended only for the use of the addressee(s) and may contain confidential and/or legally privileged information. If you are not the intended recipient, you are hereby notified that any dissemination of this email (including any attachments thereto) is strictly prohibited. If you have received this email in error, please notify the sender immediately by telephone or email and permanently destroy the original without making any copy. Please note that any material and advice from this mail is provided free of charge and shall be used as an example for demonstration purposes only.
RENESAS MAKES NO WARRANTIES THAT THE USAGE OF INFORMATION OR ADVICE FROM THIS E-MAIL WILL NOT INFRINGE ANY INTELLECTUAL PROPERTY RIGHTS (E.G. PATENTS, COPYRIGHTS). RENESAS CANNOT GUARANTEE BUG FREE OPERATION AND THE RECIPIENT WILL USE AND/OR DISTRIBUTE IT ONLY AT HIS OWN RISK. IN NO EVENT SHALL RENESAS BE LIABLE FOR ANY DAMAGE. The communication with Renesas Electronics Europe Ltd does not amend any written agreement in place.

Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.

mach header files

[Arnd Bergmann]

On Wednesday 02 April 2014 15:11:48 Phil Edworthy wrote:

> 
> This message is intended only for the use of the addressee(s) and may contain confidential and/or legally privileged information. If you are not the intended recipient, you are hereby notified that any dissemination of this email (including any attachments thereto) is strictly prohibited. If you have received this email in error, please notify the sender immediately by telephone or email and permanently destroy the original without making any copy. Please note that any material and advice from this mail is provided free of charge and shall be used as an example for demonstration purposes only.
> RENESAS MAKES NO WARRANTIES THAT THE USAGE OF INFORMATION OR ADVICE FROM THIS E-MAIL WILL NOT INFRINGE ANY INTELLECTUAL PROPERTY RIGHTS (E.G. PATENTS, COPYRIGHTS). RENESAS CANNOT GUARANTEE BUG FREE OPERATION AND THE RECIPIENT WILL USE AND/OR DISTRIBUTE IT ONLY AT HIS OWN RISK. IN NO EVENT SHALL RENESAS BE LIABLE FOR ANY DAMAGE. The communication with Renesas Electronics Europe Ltd does not amend any written agreement in place.
> 
> Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.
> 

I have an answer for you, but had to delete your mail since you asked me
to. Please send a new mail that I'm allowed to reply to on a public list.

	Arnd

mach header files

[Ben Dooks]

On 02/04/14 16:11, Phil Edworthy wrote:
> Hi,
>
> I am porting the kernel to a new device, for which I've created a new arch/arm/mach-... directory, and I also a clock driver that lives under driver/clk. Everything is all working fine, though I am now cleaning up the code and have a question about mach specific header files.
>
> The clock driver is completely specific to this device, but needs to read from a system register (just for external boot mode pins) to determine some PLL settings. This register is in a block of system registers which are also used by the mach code in arch/arm/mach-...
>
> Since the clock driver is specific to the mach, is there any point in specifying the address of this reg in the corresponding dtsi? The format and functionality described by this register would not be the same on any other device.
>
> If I don't specify the address of the register in the dtsi, I think it would be best to have a common header file for all of the system registers. I've seen some drivers, e.g.  exynos-cpufreq.c, doing this by including files from mach-exynos/include/mach. Is that the right way to do this?
>
> Thanks
> Phil
>
>
> This message is intended only for the use of the addressee(s) and may contain confidential and/or legally privileged information. If you are not the intended recipient, you are hereby notified that any dissemination of this email (including any attachments thereto) is strictly prohibited. If you have received this email in error, please notify the sender immediately by telephone or email and permanently destroy the original without making any copy. Please note that any material and advice from this mail is provided free of charge and shall be used as an example for demonstration purposes only.
> RENESAS MAKES NO WARRANTIES THAT THE USAGE OF INFORMATION OR ADVICE FROM THIS E-MAIL WILL NOT INFRINGE ANY INTELLECTUAL PROPERTY RIGHTS (E.G. PATENTS, COPYRIGHTS). RENESAS CANNOT GUARANTEE BUG FREE OPERATION AND THE RECIPIENT WILL USE AND/OR DISTRIBUTE IT ONLY AT HIS OWN RISK. IN NO EVENT SHALL RENESAS BE LIABLE FOR ANY DAMAGE. The communication with Renesas Electronics Europe Ltd does not amend any written agreement in place.

This is my least favourite method of law-breaking.


-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

mach header files

[Phil Edworthy]

On: 02 April 2014 16:16, Arnd wrote:
> On Wednesday 02 April 2014 15:11:48 Phil Edworthy wrote:
>
> >
> > This message is intended only for the use of the addressee(s) and may
> contain confidential and/or legally privileged information. If you are not the
> intended recipient, you are hereby notified that any dissemination of this
> email (including any attachments thereto) is strictly prohibited. If you have
> received this email in error, please notify the sender immediately by
> telephone or email and permanently destroy the original without making any
> copy. Please note that any material and advice from this mail is provided free
> of charge and shall be used as an example for demonstration purposes only.
> > RENESAS MAKES NO WARRANTIES THAT THE USAGE OF INFORMATION OR
> ADVICE FROM THIS E-MAIL WILL NOT INFRINGE ANY INTELLECTUAL
> PROPERTY RIGHTS (E.G. PATENTS, COPYRIGHTS). RENESAS CANNOT
> GUARANTEE BUG FREE OPERATION AND THE RECIPIENT WILL USE AND/OR
> DISTRIBUTE IT ONLY AT HIS OWN RISK. IN NO EVENT SHALL RENESAS BE
> LIABLE FOR ANY DAMAGE. The communication with Renesas Electronics
> Europe Ltd does not amend any written agreement in place.
> >
> > Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne
> End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under
> Registered No. 04586709.
> >
>
> I have an answer for you, but had to delete your mail since you asked me to.
> Please send a new mail that I'm allowed to reply to on a public list.
Oh great, a change in corporate mail client & this gets shoved on the end without my knowledge!

I'll sort it out...

Thanks
Phil


This message is intended only for the use of the addressee(s) and may contain confidential and/or legally privileged information. If you are not the intended recipient, you are hereby notified that any dissemination of this email (including any attachments thereto) is strictly prohibited. If you have received this email in error, please notify the sender immediately by telephone or email and permanently destroy the original without making any copy. Please note that any material and advice from this mail is provided free of charge and shall be used as an example for demonstration purposes only.
RENESAS MAKES NO WARRANTIES THAT THE USAGE OF INFORMATION OR ADVICE FROM THIS E-MAIL WILL NOT INFRINGE ANY INTELLECTUAL PROPERTY RIGHTS (E.G. PATENTS, COPYRIGHTS). RENESAS CANNOT GUARANTEE BUG FREE OPERATION AND THE RECIPIENT WILL USE AND/OR DISTRIBUTE IT ONLY AT HIS OWN RISK. IN NO EVENT SHALL RENESAS BE LIABLE FOR ANY DAMAGE. The communication with Renesas Electronics Europe Ltd does not amend any written agreement in place.

Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.

mach header files

[Phil Edworthy]

Resent, hopefully without the automatic corporate signature appended this time...

Hi,

I am porting the kernel to a new device, for which I've created a new arch/arm/mach-... directory, and I also a clock driver that lives under driver/clk. Everything is all working fine, though I am now cleaning up the code and have a question about mach specific header files.

The clock driver is completely specific to this device, but needs to read from a system register (just for external boot mode pins) to determine some PLL settings. This register is in a block of system registers which are also used by the mach code in arch/arm/mach-...

Since the clock driver is specific to the mach, is there any point in specifying the address of this reg in the corresponding dtsi? The format and functionality described by this register would not be the same on any other device.

If I don't specify the address of the register in the dtsi, I think it would be best to have a common header file for all of the system registers. I've seen some drivers, e.g.  exynos-cpufreq.c, doing this by including files from mach-exynos/include/mach. Is that the right way to do this?

Thanks
Phil

mach header files

[Grant Likely]

On Fri, Apr 4, 2014 at 1:19 AM, Phil Edworthy <phil.edworthy@renesas.com> wrote:
> Resent, hopefully without the automatic corporate signature appended this time...
>
> Hi,
>
> I am porting the kernel to a new device, for which I've created a new arch/arm/mach-... directory, and I also a clock driver that lives under driver/clk. Everything is all working fine, though I am now cleaning up the code and have a question about mach specific header files.
>
> The clock driver is completely specific to this device, but needs to read from a system register (just for external boot mode pins) to determine some PLL settings. This register is in a block of system registers which are also used by the mach code in arch/arm/mach-...
>
> Since the clock driver is specific to the mach, is there any point in specifying the address of this reg in the corresponding dtsi? The format and functionality described by this register would not be the same on any other device.

Why would you not? The dts is a central place where the entire memory
map layout of the device can be specified. If I were working on the
board support, I would create a node for the register block and have
the driver retrieve that node.

> If I don't specify the address of the register in the dtsi, I think it would be best to have a common header file for all of the system registers. I've seen some drivers, e.g.  exynos-cpufreq.c, doing this by including files from mach-exynos/include/mach. Is that the right way to do this?

You'd still want to have a header file for each of the register
offsets within the block. You wouldn't want to itemize register
offsets in the device tree node.

g.

mach header files

[Phil Edworthy]

Hi Grant,

On: 04 April 2014 11:30, Grant wrote:
> On Fri, Apr 4, 2014 at 1:19 AM, Phil Edworthy <phil.edworthy@renesas.com>
> wrote:
> > Resent, hopefully without the automatic corporate signature appended
> this time...
> >
> > Hi,
> >
> > I am porting the kernel to a new device, for which I've created a new
> arch/arm/mach-... directory, and I also a clock driver that lives under
> driver/clk. Everything is all working fine, though I am now cleaning up the
> code and have a question about mach specific header files.
> >
> > The clock driver is completely specific to this device, but needs to read from
> a system register (just for external boot mode pins) to determine some PLL
> settings. This register is in a block of system registers which are also used by
> the mach code in arch/arm/mach-...
> >
> > Since the clock driver is specific to the mach, is there any point in specifying
> the address of this reg in the corresponding dtsi? The format and
> functionality described by this register would not be the same on any other
> device.
> 
> Why would you not? The dts is a central place where the entire memory map
> layout of the device can be specified. If I were working on the board support,
> I would create a node for the register block and have the driver retrieve that
> node.

Ok, that's a slightly different view to the one I had. I had thought that the dts would only contain information that could be changed from one platform to another.

> > If I don't specify the address of the register in the dtsi, I think it would be
> best to have a common header file for all of the system registers. I've seen
> some drivers, e.g.  exynos-cpufreq.c, doing this by including files from mach-
> exynos/include/mach. Is that the right way to do this?
> 
> You'd still want to have a header file for each of the register offsets within
> the block. You wouldn't want to itemize register offsets in the device tree
> node.

You would put that include file in a mach-.../include/mach/ dir, right?

Thanks
Phil

mach header files

[Grant Likely]

On Fri, Apr 4, 2014 at 5:16 AM, Phil Edworthy <phil.edworthy@renesas.com> wrote:
> Hi Grant,
>
> On: 04 April 2014 11:30, Grant wrote:
>> On Fri, Apr 4, 2014 at 1:19 AM, Phil Edworthy <phil.edworthy@renesas.com>
>> wrote:
>> > Resent, hopefully without the automatic corporate signature appended
>> this time...
>> >
>> > Hi,
>> >
>> > I am porting the kernel to a new device, for which I've created a new
>> arch/arm/mach-... directory, and I also a clock driver that lives under
>> driver/clk. Everything is all working fine, though I am now cleaning up the
>> code and have a question about mach specific header files.
>> >
>> > The clock driver is completely specific to this device, but needs to read from
>> a system register (just for external boot mode pins) to determine some PLL
>> settings. This register is in a block of system registers which are also used by
>> the mach code in arch/arm/mach-...
>> >
>> > Since the clock driver is specific to the mach, is there any point in specifying
>> the address of this reg in the corresponding dtsi? The format and
>> functionality described by this register would not be the same on any other
>> device.
>>
>> Why would you not? The dts is a central place where the entire memory map
>> layout of the device can be specified. If I were working on the board support,
>> I would create a node for the register block and have the driver retrieve that
>> node.
>
> Ok, that's a slightly different view to the one I had. I had thought that the dts would only contain information that could be changed from one platform to another.
>
>> > If I don't specify the address of the register in the dtsi, I think it would be
>> best to have a common header file for all of the system registers. I've seen
>> some drivers, e.g.  exynos-cpufreq.c, doing this by including files from mach-
>> exynos/include/mach. Is that the right way to do this?
>>
>> You'd still want to have a header file for each of the register offsets within
>> the block. You wouldn't want to itemize register offsets in the device tree
>> node.
>
> You would put that include file in a mach-.../include/mach/ dir, right?

If it is used only by one driver, then it should go directly into that
driver without a #include. For SoC defines, you'll need to check with
the arm-soc maintainers. I *think* we're trying to get away from
separate arch/arm/mach-* directories, but I'm slightly out of touch.
You certainly should not do anything that breaks multiplatform builds.

g.

mach header files

[Phil Edworthy]

Hi Grant,

On: 04 April 2014 13:23, Grant wrote:
> On Fri, Apr 4, 2014 at 5:16 AM, Phil Edworthy <phil.edworthy@renesas.com>
> wrote:
> > Hi Grant,
> >
> > On: 04 April 2014 11:30, Grant wrote:
> >> On Fri, Apr 4, 2014 at 1:19 AM, Phil Edworthy
> <phil.edworthy@renesas.com>
> >> wrote:
> >> > Resent, hopefully without the automatic corporate signature appended
> >> this time...
> >> >
> >> > Hi,
> >> >
> >> > I am porting the kernel to a new device, for which I've created a new
> >> arch/arm/mach-... directory, and I also a clock driver that lives under
> >> driver/clk. Everything is all working fine, though I am now cleaning up the
> >> code and have a question about mach specific header files.
> >> >
> >> > The clock driver is completely specific to this device, but needs to read
> from
> >> a system register (just for external boot mode pins) to determine some
> PLL
> >> settings. This register is in a block of system registers which are also used
> by
> >> the mach code in arch/arm/mach-...
> >> >
> >> > Since the clock driver is specific to the mach, is there any point in
> specifying
> >> the address of this reg in the corresponding dtsi? The format and
> >> functionality described by this register would not be the same on any
> other
> >> device.
> >>
> >> Why would you not? The dts is a central place where the entire memory
> map
> >> layout of the device can be specified. If I were working on the board
> support,
> >> I would create a node for the register block and have the driver retrieve
> that
> >> node.
> >
> > Ok, that's a slightly different view to the one I had. I had thought that the
> dts would only contain information that could be changed from one platform
> to another.
> >
> >> > If I don't specify the address of the register in the dtsi, I think it would
> be
> >> best to have a common header file for all of the system registers. I've
> seen
> >> some drivers, e.g.  exynos-cpufreq.c, doing this by including files from
> mach-
> >> exynos/include/mach. Is that the right way to do this?
> >>
> >> You'd still want to have a header file for each of the register offsets within
> >> the block. You wouldn't want to itemize register offsets in the device tree
> >> node.
> >
> > You would put that include file in a mach-.../include/mach/ dir, right?
> 
> If it is used only by one driver, then it should go directly into that
> driver without a #include. For SoC defines, you'll need to check with
> the arm-soc maintainers. I *think* we're trying to get away from
> separate arch/arm/mach-* directories, but I'm slightly out of touch.
> You certainly should not do anything that breaks multiplatform builds.

This is where it gets tricky. The clock driver only wants to know about one reg in a block of general system control registers. There is very little input to the clock driver... the SoC code needs to access other registers in the system control block.

I think you are right about trying to avoid separate arch/arm/mach-* directories, which is what prompted my question. So, how best to handle this?

Arnd, I would appreciate your comments on this!

Thanks
Phil

mach header files

[Thomas Petazzoni]

Dear Phil Edworthy,

On Fri, 4 Apr 2014 13:14:44 +0000, Phil Edworthy wrote:

> > If it is used only by one driver, then it should go directly into
> > that driver without a #include. For SoC defines, you'll need to
> > check with the arm-soc maintainers. I *think* we're trying to get
> > away from separate arch/arm/mach-* directories, but I'm slightly
> > out of touch. You certainly should not do anything that breaks
> > multiplatform builds.
> 
> This is where it gets tricky. The clock driver only wants to know
> about one reg in a block of general system control registers. There
> is very little input to the clock driver... the SoC code needs to
> access other registers in the system control block.
> 
> I think you are right about trying to avoid separate arch/arm/mach-*
> directories, which is what prompted my question. So, how best to
> handle this?

I believe there are two solutions here:

 * Have a driver in arch/arm/mach-foo/ for your "system control block"
   that exposes a small API used by your clock driver in drivers/clk/.
   The issue is of course always: where to put the header files for
   this small API.

 * Have both your clock driver and your system control block driver map
   the registers they need, even if some of those registers are the
   same. Then you can use atomic_io_modify() from both drivers to
   atomically make changes to these registers. This is typically OK if
   you don't have any performance sensitive usage of those registers
   (atomic_io_modify accesses are protected by a global spinlock). This
   is the solution we've used between the Armada watchdog and Armada
   clocksource drivers, which require accessing a shared register, even
   though those two drivers are completely unrelated.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

mach header files

[Arnd Bergmann]

On Friday 04 April 2014 03:29:54 Grant Likely wrote:
> On Fri, Apr 4, 2014 at 1:19 AM, Phil Edworthy <phil.edworthy@renesas.com> wrote:
> > Resent, hopefully without the automatic corporate signature appended this time...
> >
> > I am porting the kernel to a new device, for which I've created a new
> > arch/arm/mach-... directory, and I also a clock driver that lives under
> > driver/clk. Everything is all working fine, though I am now cleaning up
> > the code and have a question about mach specific header files.

I'm glad you are asking while you are still in the process of cleaning
up your code, because you need to know the rules for new platforms.
Basically at this point we expect zero code in arch/arm/mach-* for a new
platform. You are absolutely required to have DT based probing and
multiplatform support, and the latter also means that there are no
mach/*.h header files that are visible to device drivers.

We still make the occasional exception for adding code in the mach-*
directory, but we are getting pretty close to the state where this
is not needed for new platforms, and all the existing uses are for
things that can eventually get cleaned up.
If you think you need an exception here, please explain what you
are doing, and we can see if there is a better way to do that already.

> > The clock driver is completely specific to this device, but needs to read
> > from a system register (just for external boot mode pins) to determine some
> > PLL settings. This register is in a block of system registers which are
> > also used by the mach code in arch/arm/mach-...
> >
> > Since the clock driver is specific to the mach, is there any point in
> > specifying the address of this reg in the corresponding dtsi? The format
> > and functionality described by this register would not be the same on any other device.
> 
> Why would you not? The dts is a central place where the entire memory
> map layout of the device can be specified. If I were working on the
> board support, I would create a node for the register block and have
> the driver retrieve that node.

Right. More specifically, this case is often handled using the drivers/mfd/syscon.c
driver, which exports a "regmap" pointer that can be used by drivers to access
those registers. The driver will need to look up the regmap through a phandle
in DT, and it can either use a hardcoded offset into the regmap to get
to the specific register it needs, or it can look up the offset from DT
as well. Which of the two you pick depends on the particular needs of the
device.

> > If I don't specify the address of the register in the dtsi, I think it
> > would be best to have a common header file for all of the system registers.
> > I've seen some drivers, e.g.  exynos-cpufreq.c, doing this by including files
> > from mach-exynos/include/mach. Is that the right way to do this?

You have managed to pick the perfect example: The exynos cpufreq driver is the
one that is currently blocking support for multiplatform exynos builds because
it uses header files from the platform code. Because of this, we are actually
planning to throw out that driver entirely in the next merge window.

In short, definitely the wrong way.

	Arnd

mach header files

[Barry Song]

2014-04-04 21:24 GMT+08:00 Arnd Bergmann <arnd@arndb.de>:
> On Friday 04 April 2014 03:29:54 Grant Likely wrote:
>> On Fri, Apr 4, 2014 at 1:19 AM, Phil Edworthy <phil.edworthy@renesas.com> wrote:
>> > Resent, hopefully without the automatic corporate signature appended this time...
>> >
>> > I am porting the kernel to a new device, for which I've created a new
>> > arch/arm/mach-... directory, and I also a clock driver that lives under
>> > driver/clk. Everything is all working fine, though I am now cleaning up
>> > the code and have a question about mach specific header files.
>
> I'm glad you are asking while you are still in the process of cleaning
> up your code, because you need to know the rules for new platforms.
> Basically at this point we expect zero code in arch/arm/mach-* for a new
> platform. You are absolutely required to have DT based probing and
> multiplatform support, and the latter also means that there are no
> mach/*.h header files that are visible to device drivers.
>
> We still make the occasional exception for adding code in the mach-*
> directory, but we are getting pretty close to the state where this
> is not needed for new platforms, and all the existing uses are for
> things that can eventually get cleaned up.
> If you think you need an exception here, please explain what you
> are doing, and we can see if there is a better way to do that already.

Arnd, my question is that mach-prima2/common.c has supported prima2,
atlas6, marco, what if i add an atlas7 support in it?

-barry

mach header files

[Grant Likely]

On Fri, Apr 4, 2014 at 6:14 AM, Phil Edworthy <phil.edworthy@renesas.com> wrote:
> Hi Grant,
>
> On: 04 April 2014 13:23, Grant wrote:
>> On Fri, Apr 4, 2014 at 5:16 AM, Phil Edworthy <phil.edworthy@renesas.com>
>> wrote:
>> > Hi Grant,
>> >
>> > On: 04 April 2014 11:30, Grant wrote:
>> >> On Fri, Apr 4, 2014 at 1:19 AM, Phil Edworthy
>> <phil.edworthy@renesas.com>
>> >> wrote:
>> >> > Resent, hopefully without the automatic corporate signature appended
>> >> this time...
>> >> >
>> >> > Hi,
>> >> >
>> >> > I am porting the kernel to a new device, for which I've created a new
>> >> arch/arm/mach-... directory, and I also a clock driver that lives under
>> >> driver/clk. Everything is all working fine, though I am now cleaning up the
>> >> code and have a question about mach specific header files.
>> >> >
>> >> > The clock driver is completely specific to this device, but needs to read
>> from
>> >> a system register (just for external boot mode pins) to determine some
>> PLL
>> >> settings. This register is in a block of system registers which are also used
>> by
>> >> the mach code in arch/arm/mach-...
>> >> >
>> >> > Since the clock driver is specific to the mach, is there any point in
>> specifying
>> >> the address of this reg in the corresponding dtsi? The format and
>> >> functionality described by this register would not be the same on any
>> other
>> >> device.
>> >>
>> >> Why would you not? The dts is a central place where the entire memory
>> map
>> >> layout of the device can be specified. If I were working on the board
>> support,
>> >> I would create a node for the register block and have the driver retrieve
>> that
>> >> node.
>> >
>> > Ok, that's a slightly different view to the one I had. I had thought that the
>> dts would only contain information that could be changed from one platform
>> to another.
>> >
>> >> > If I don't specify the address of the register in the dtsi, I think it would
>> be
>> >> best to have a common header file for all of the system registers. I've
>> seen
>> >> some drivers, e.g.  exynos-cpufreq.c, doing this by including files from
>> mach-
>> >> exynos/include/mach. Is that the right way to do this?
>> >>
>> >> You'd still want to have a header file for each of the register offsets within
>> >> the block. You wouldn't want to itemize register offsets in the device tree
>> >> node.
>> >
>> > You would put that include file in a mach-.../include/mach/ dir, right?
>>
>> If it is used only by one driver, then it should go directly into that
>> driver without a #include. For SoC defines, you'll need to check with
>> the arm-soc maintainers. I *think* we're trying to get away from
>> separate arch/arm/mach-* directories, but I'm slightly out of touch.
>> You certainly should not do anything that breaks multiplatform builds.
>
> This is where it gets tricky. The clock driver only wants to know about one reg in a block of general system control registers. There is very little input to the clock driver... the SoC code needs to access other registers in the system control block.

As long as it can find the registers and if the clock driver is only
reading the registers then there is no problem. You don't need to
worry about locking and the driver can access the registers directly.

If the driver needs to coordinate with other drivers, then you should
have a little block of code to manage the system register block.

g.

mach header files

[Phil Edworthy]

Hi Arnd,

On 04 April 2014 14:24, Arnd wrote:
> On Friday 04 April 2014 03:29:54 Grant Likely wrote:
> > On Fri, Apr 4, 2014 at 1:19 AM, Phil Edworthy
> <phil.edworthy@renesas.com> wrote:
> > > Resent, hopefully without the automatic corporate signature appended
> this time...
> > >
> > > I am porting the kernel to a new device, for which I've created a new
> > > arch/arm/mach-... directory, and I also a clock driver that lives under
> > > driver/clk. Everything is all working fine, though I am now cleaning up
> > > the code and have a question about mach specific header files.
> 
> I'm glad you are asking while you are still in the process of cleaning
> up your code, because you need to know the rules for new platforms.
> Basically at this point we expect zero code in arch/arm/mach-* for a new
> platform. You are absolutely required to have DT based probing and
> multiplatform support, and the latter also means that there are no
> mach/*.h header files that are visible to device drivers.
> 
> We still make the occasional exception for adding code in the mach-*
> directory, but we are getting pretty close to the state where this
> is not needed for new platforms, and all the existing uses are for
> things that can eventually get cleaned up.
> If you think you need an exception here, please explain what you
> are doing, and we can see if there is a better way to do that already.

Ok, that's what I suspected about the mach header files.

I didn't realise about the expectation that there is no arch/arm/mach-* for new platforms. At the moment I have only a small amount of platform code:
1. SMP holding pen code (as there is no power control for the second core), though I have recently seen patches to provide a generic solution for this.
2. Firmware calls (via struct firmware_ops) for set_cpu_boot_addr and cpu_boot. This falls back to directly accessing registers in the system control block, if the firmware fails. This is useful when developing the system when no firmware exists yet.
3. A write to a system control register to switch the pl011 uart from DMA single requests, to DMA burst requests.
 
> > > The clock driver is completely specific to this device, but needs to read
> > > from a system register (just for external boot mode pins) to determine
> some
> > > PLL settings. This register is in a block of system registers which are
> > > also used by the mach code in arch/arm/mach-...
> > >
> > > Since the clock driver is specific to the mach, is there any point in
> > > specifying the address of this reg in the corresponding dtsi? The format
> > > and functionality described by this register would not be the same on any
> other device.
> >
> > Why would you not? The dts is a central place where the entire memory
> > map layout of the device can be specified. If I were working on the
> > board support, I would create a node for the register block and have
> > the driver retrieve that node.
> 
> Right. More specifically, this case is often handled using the
> drivers/mfd/syscon.c
> driver, which exports a "regmap" pointer that can be used by drivers to
> access
> those registers. The driver will need to look up the regmap through a
> phandle
> in DT, and it can either use a hardcoded offset into the regmap to get
> to the specific register it needs, or it can look up the offset from DT
> as well. Which of the two you pick depends on the particular needs of the
> device.
> 
> > > If I don't specify the address of the register in the dtsi, I think it
> > > would be best to have a common header file for all of the system
> registers.
> > > I've seen some drivers, e.g.  exynos-cpufreq.c, doing this by including
> files
> > > from mach-exynos/include/mach. Is that the right way to do this?
> 
> You have managed to pick the perfect example: The exynos cpufreq driver is
> the
> one that is currently blocking support for multiplatform exynos builds
> because
> it uses header files from the platform code. Because of this, we are actually
> planning to throw out that driver entirely in the next merge window.
> 
> In short, definitely the wrong way.
I had pretty much realised that, but saw the exynos code and wanted to check. I'm glad I asked!

Thanks
Phil

mach header files

[Arnd Bergmann]

On Friday 04 April 2014 21:32:33 Barry Song wrote:
> 2014-04-04 21:24 GMT+08:00 Arnd Bergmann <arnd@arndb.de>:
> > On Friday 04 April 2014 03:29:54 Grant Likely wrote:
> >> On Fri, Apr 4, 2014 at 1:19 AM, Phil Edworthy <phil.edworthy@renesas.com> wrote:
> >> > Resent, hopefully without the automatic corporate signature appended this time...
> >> >
> >> > I am porting the kernel to a new device, for which I've created a new
> >> > arch/arm/mach-... directory, and I also a clock driver that lives under
> >> > driver/clk. Everything is all working fine, though I am now cleaning up
> >> > the code and have a question about mach specific header files.
> >
> > I'm glad you are asking while you are still in the process of cleaning
> > up your code, because you need to know the rules for new platforms.
> > Basically at this point we expect zero code in arch/arm/mach-* for a new
> > platform. You are absolutely required to have DT based probing and
> > multiplatform support, and the latter also means that there are no
> > mach/*.h header files that are visible to device drivers.
> >
> > We still make the occasional exception for adding code in the mach-*
> > directory, but we are getting pretty close to the state where this
> > is not needed for new platforms, and all the existing uses are for
> > things that can eventually get cleaned up.
> > If you think you need an exception here, please explain what you
> > are doing, and we can see if there is a better way to do that already.
> 
> Arnd, my question is that mach-prima2/common.c has supported prima2,
> atlas6, marco, what if i add an atlas7 support in it?

It's not a new platform, so it is ok as long as you don't add more
code there that can be avoided and you keep cleaning up the code
that is there, as you have been doing so far.

	Arnd

mach header files

[Arnd Bergmann]

On Friday 04 April 2014 13:52:53 Phil Edworthy wrote:
> On 04 April 2014 14:24, Arnd wrote:
> > We still make the occasional exception for adding code in the mach-*
> > directory, but we are getting pretty close to the state where this
> > is not needed for new platforms, and all the existing uses are for
> > things that can eventually get cleaned up.
> > If you think you need an exception here, please explain what you
> > are doing, and we can see if there is a better way to do that already.
> 
> Ok, that's what I suspected about the mach header files.
> 
> I didn't realise about the expectation that there is no 
> arch/arm/mach-* for new platforms. At the moment I have only a
> small amount of platform code:
> 1. SMP holding pen code (as there is no power control for the second
> core), though I have recently seen patches to provide a generic solution
> for this.

Proper platforms should not be using the holding pen, that is only
done on systems that don't have real hardware support for CPU
power management. Ideally you should be implementing PSCI in the
firmware, and then you won't need any code for SMP.

> 2. Firmware calls (via struct firmware_ops) for set_cpu_boot_addr
>  and cpu_boot. This falls back to directly accessing registers in
> the system control block, if the firmware fails. This is useful when
> developing the system when no firmware exists yet.

Same thing.

> 3. A write to a system control register to switch the pl011 uart
> from DMA single requests, to DMA burst requests.

Do you need to do this at run-time, or is it something that could
be done in the bootloader before Linux starts up?

	Arnd

mach header files

[Phil Edworthy]

Hi Arnd,

On 04 April 2014 16:31, Arnd wrote:
> On Friday 04 April 2014 13:52:53 Phil Edworthy wrote:
> > On 04 April 2014 14:24, Arnd wrote:
> > > We still make the occasional exception for adding code in the mach-*
> > > directory, but we are getting pretty close to the state where this
> > > is not needed for new platforms, and all the existing uses are for
> > > things that can eventually get cleaned up.
> > > If you think you need an exception here, please explain what you
> > > are doing, and we can see if there is a better way to do that already.
> >
> > Ok, that's what I suspected about the mach header files.
> >
> > I didn't realise about the expectation that there is no
> > arch/arm/mach-* for new platforms. At the moment I have only a
> > small amount of platform code:
> > 1. SMP holding pen code (as there is no power control for the second
> > core), though I have recently seen patches to provide a generic solution
> > for this.
> 
> Proper platforms should not be using the holding pen, that is only
> done on systems that don't have real hardware support for CPU
> power management. Ideally you should be implementing PSCI in the
> firmware, and then you won't need any code for SMP.
> 
> > 2. Firmware calls (via struct firmware_ops) for set_cpu_boot_addr
> >  and cpu_boot. This falls back to directly accessing registers in
> > the system control block, if the firmware fails. This is useful when
> > developing the system when no firmware exists yet.
> 
> Same thing.
Ok, something to look into. How the hardware will work is complicated by the fact that we don't have silicon for this device yet, just a simulator.  As for the firmware, I am not sure I will see that. Our customer is implementing it and responsible for creating the firmware calls from Linux, hence the fall backs.


> > 3. A write to a system control register to switch the pl011 uart
> > from DMA single requests, to DMA burst requests.
> 
> Do you need to do this at run-time, or is it something that could
> be done in the bootloader before Linux starts up?
I might be able to get it into the boot loader, but this is implemented by the customer. One question is whether it should be in the boot loader. The behaviour is specify to the Linux port, whereas the boot loader can be used for a number of different OS.

Thanks
Phil

mach header files

[Arnd Bergmann]

On Friday 04 April 2014 15:42:37 Phil Edworthy wrote:
> 
> On 04 April 2014 16:31, Arnd wrote:
> > On Friday 04 April 2014 13:52:53 Phil Edworthy wrote:
> > > On 04 April 2014 14:24, Arnd wrote:
> > > > We still make the occasional exception for adding code in the mach-*
> > > > directory, but we are getting pretty close to the state where this
> > > > is not needed for new platforms, and all the existing uses are for
> > > > things that can eventually get cleaned up.
> > > > If you think you need an exception here, please explain what you
> > > > are doing, and we can see if there is a better way to do that already.
> > >
> > > Ok, that's what I suspected about the mach header files.
> > >
> > > I didn't realise about the expectation that there is no
> > > arch/arm/mach-* for new platforms. At the moment I have only a
> > > small amount of platform code:
> > > 1. SMP holding pen code (as there is no power control for the second
> > > core), though I have recently seen patches to provide a generic solution
> > > for this.
> > 
> > Proper platforms should not be using the holding pen, that is only
> > done on systems that don't have real hardware support for CPU
> > power management. Ideally you should be implementing PSCI in the
> > firmware, and then you won't need any code for SMP.
> > 
> > > 2. Firmware calls (via struct firmware_ops) for set_cpu_boot_addr
> > >  and cpu_boot. This falls back to directly accessing registers in
> > > the system control block, if the firmware fails. This is useful when
> > > developing the system when no firmware exists yet.
> > 
> > Same thing.
> Ok, something to look into. How the hardware will work is complicated
> by the fact that we don't have silicon for this device yet, just a
> simulator.  As for the firmware, I am not sure I will see that. Our
> customer is implementing it and responsible for creating the firmware 
> calls from Linux, hence the fall backs.

In that case, I would recommend that you just rely on PSCI in the
kernel and leave it up to your customer to either implement that
in their firmware, or provide kernel support for the interfaces
they want. If they have a good reason to need a different interface,
they can also send patches for that and argue for inclusion, but
that is not part of the base platform support you do.
 
> > > 3. A write to a system control register to switch the pl011 uart
> > > from DMA single requests, to DMA burst requests.
> > 
> > Do you need to do this at run-time, or is it something that could
> > be done in the bootloader before Linux starts up?
>
> I might be able to get it into the boot loader, but this is
> implemented by the customer. One question is whether it should
> be in the boot loader. The behaviour is specify to the Linux port,
> whereas the boot loader can be used for a number of different OS.

What is the impact for single-request vs burst request? Is it just
performance? You can add a DT property in the pl011 node to let
the device driver know which of the two modes was set up by the
boot loader, and implement both in the driver. Then anybody who
wants burst mode can set it in the boot loader and every OS will
be able to find out the mode from DT.

	Arnd 

mach header files

[Phil Edworthy]

On 04 April 2014 16:51, Arnd wrote:
> On Friday 04 April 2014 15:42:37 Phil Edworthy wrote:
> >
> > On 04 April 2014 16:31, Arnd wrote:
> > > On Friday 04 April 2014 13:52:53 Phil Edworthy wrote:
> > > > On 04 April 2014 14:24, Arnd wrote:
> > > > > We still make the occasional exception for adding code in the mach-*
> > > > > directory, but we are getting pretty close to the state where this
> > > > > is not needed for new platforms, and all the existing uses are for
> > > > > things that can eventually get cleaned up.
> > > > > If you think you need an exception here, please explain what you
> > > > > are doing, and we can see if there is a better way to do that already.
> > > >
> > > > Ok, that's what I suspected about the mach header files.
> > > >
> > > > I didn't realise about the expectation that there is no
> > > > arch/arm/mach-* for new platforms. At the moment I have only a
> > > > small amount of platform code:
> > > > 1. SMP holding pen code (as there is no power control for the second
> > > > core), though I have recently seen patches to provide a generic solution
> > > > for this.
> > >
> > > Proper platforms should not be using the holding pen, that is only
> > > done on systems that don't have real hardware support for CPU
> > > power management. Ideally you should be implementing PSCI in the
> > > firmware, and then you won't need any code for SMP.
> > >
> > > > 2. Firmware calls (via struct firmware_ops) for set_cpu_boot_addr
> > > >  and cpu_boot. This falls back to directly accessing registers in
> > > > the system control block, if the firmware fails. This is useful when
> > > > developing the system when no firmware exists yet.
> > >
> > > Same thing.
> > Ok, something to look into. How the hardware will work is complicated
> > by the fact that we don't have silicon for this device yet, just a
> > simulator.  As for the firmware, I am not sure I will see that. Our
> > customer is implementing it and responsible for creating the firmware
> > calls from Linux, hence the fall backs.
> 
> In that case, I would recommend that you just rely on PSCI in the
> kernel and leave it up to your customer to either implement that
> in their firmware, or provide kernel support for the interfaces
> they want. If they have a good reason to need a different interface,
> they can also send patches for that and argue for inclusion, but
> that is not part of the base platform support you do.

Makes sense, I'll see if I can push it that way!

 
> > > > 3. A write to a system control register to switch the pl011 uart
> > > > from DMA single requests, to DMA burst requests.
> > >
> > > Do you need to do this at run-time, or is it something that could
> > > be done in the bootloader before Linux starts up?
> >
> > I might be able to get it into the boot loader, but this is
> > implemented by the customer. One question is whether it should
> > be in the boot loader. The behaviour is specify to the Linux port,
> > whereas the boot loader can be used for a number of different OS.
> 
> What is the impact for single-request vs burst request? Is it just
> performance? You can add a DT property in the pl011 node to let
> the device driver know which of the two modes was set up by the
> boot loader, and implement both in the driver. Then anybody who
> wants burst mode can set it in the boot loader and every OS will
> be able to find out the mode from DT.
Unfortunately, it's not a performance toggle. Functionally, the pl011 will only work with a DMAC if we use the DMA burst request. That's not completely true though as whether it will work depends on the pl011 driver. With the upstream pl011 linux driver, it's true.

BTW, the register to control this has nothing to do with the pl011 hardware. The standard pl011 hardware, afaik, outputs both the DMA single request and DMA burst request signals, and which one is wired up to the DMAC depends on who designed the SoC. For this hardware, the hardware people played safe and provided a register that can be used to switch between the single and burst request signals.

Thanks
Phil

mach header files

[Russell King - ARM Linux]

On Fri, Apr 04, 2014 at 04:02:58PM +0000, Phil Edworthy wrote:
> Unfortunately, it's not a performance toggle. Functionally, the pl011
> will only work with a DMAC if we use the DMA burst request. That's not
> completely true though as whether it will work depends on the pl011
> driver. With the upstream pl011 linux driver, it's true.
> 
> BTW, the register to control this has nothing to do with the pl011
> hardware. The standard pl011 hardware, afaik, outputs both the DMA
> single request and DMA burst request signals, and which one is wired
> up to the DMAC depends on who designed the SoC. For this hardware,
> the hardware people played safe and provided a register that can be
> used to switch between the single and burst request signals.

That's not how it's supposed to be used.

The idea behind the two requests is that the peripheral signals to the
DMA controller whether it has enough entries in the FIFO for the DMA
controller to do a burst transfer, or whether it should do a series of
single transfers.

It sounds to me like someone hasn't taken the time to read the PL011
documentation and thought about this.  As such, it's probably not worth
bothering trying to get DMA working.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

mach header files

[Phil Edworthy]

Hi Russell,

On 04 April 2014 17:22, Russell wrote:
> On Fri, Apr 04, 2014 at 04:02:58PM +0000, Phil Edworthy wrote:
> > Unfortunately, it's not a performance toggle. Functionally, the pl011
> > will only work with a DMAC if we use the DMA burst request. That's not
> > completely true though as whether it will work depends on the pl011
> > driver. With the upstream pl011 linux driver, it's true.
> >
> > BTW, the register to control this has nothing to do with the pl011
> > hardware. The standard pl011 hardware, afaik, outputs both the DMA
> > single request and DMA burst request signals, and which one is wired
> > up to the DMAC depends on who designed the SoC. For this hardware,
> > the hardware people played safe and provided a register that can be
> > used to switch between the single and burst request signals.
> 
> That's not how it's supposed to be used.
> 
> The idea behind the two requests is that the peripheral signals to the
> DMA controller whether it has enough entries in the FIFO for the DMA
> controller to do a burst transfer, or whether it should do a series of
> single transfers.
> 
> It sounds to me like someone hasn't taken the time to read the PL011
> documentation and thought about this.  As such, it's probably not worth
> bothering trying to get DMA working.

DMA with the pl011 works just fine, thanks - we've integrated our DMAC & pl011 hardware onto a Xilinx Zynq device.
The only software change required is to modify the burst threshold in the pl011 driver to 3/4 full. The DMA burst size is left to 1/2 the FIFO size to ensure there is data left in the FIFO after the DMAC has performed it's burst. This allows the pl011 to fire of the rx timeout irq when no further data has been received.

Thanks
Phil

mach header files

[Kent Borg]

On 04/04/2014 09:24 AM, Arnd Bergmann wrote:
> Basically at this point we expect zero code in arch/arm/mach-* for a 
> new platform. 

I am in a position similar to Phil's, bringing up Linux on emulated 
hardware. And obviously I look at existing code to see how things are done.

What is the cleanest look-here-first, least mach-code, SMP example for 
me to try to slavishly copy?


Thanks,

-kb, the Kent who was so glad to see the recent anti-pen ranting, as in 
his looking he couldn't figure out what the pen was for.

mach header files

[Arnd Bergmann]

On Friday 04 April 2014 14:01:43 Kent Borg wrote:
> On 04/04/2014 09:24 AM, Arnd Bergmann wrote:
> > Basically at this point we expect zero code in arch/arm/mach-* for a 
> > new platform. 
> 
> I am in a position similar to Phil's, bringing up Linux on emulated 
> hardware. And obviously I look at existing code to see how things are done.
> 
> What is the cleanest look-here-first, least mach-code, SMP example for 
> me to try to slavishly copy?

The trouble with examples here is that the good ones are those
without any code, which is harder to copy than the bad ones ;-)

mach-sunxi is a rather good example, since it was added only
recently. Note that the platsmp.c code in there is only used
for the sun6i variant, which doesn't have a boot loader
implementing the PSCI SMP code yet. IIRC sun7i does this right,
so the .smp pointer is empty.

Most of the code in mach-sunxi is actually just the system-restart
implementation, and my feeling is that this should just be moved
into the watchdog timer driver (there is still some discussion
about that).

Once these things are both done, you can boot a sunxi machine
without any code in mach-sunxi.

An ideal example is mach-moxart, which has zero code.

Can you say what platform you are working on? If I can see
your code, that would make it easier to give you advice.

	Arnd

mach header files

[Kent Borg]

On 04/04/2014 02:49 PM, Arnd Bergmann wrote:
> The trouble with examples here is that the good ones are those without 
> any code, which is harder to copy than the bad ones ;-) 

Indeed.

> mach-sunxi is a rather good example, [...] An ideal example is 
> mach-moxart, which has zero code. 

Cool.

> Can you say what platform you are working on?

Doesn't exist yet, as I said I am running an emulation, trying to get 
Linux up, but also part of verifying design. Pretty sure no marketing 
announcements have been made; I don't think I can blab before the suits do.

-kb

mach header files

[Arnd Bergmann]

On Friday 04 April 2014 15:01:01 Kent Borg wrote:
> On 04/04/2014 02:49 PM, Arnd Bergmann wrote:
> > Can you say what platform you are working on?
> 
> Doesn't exist yet, as I said I am running an emulation, trying to get 
> Linux up, but also part of verifying design. Pretty sure no marketing 
> announcements have been made; I don't think I can blab before the suits do.

Ok, fair enough. At least that means you are at a stage where you
can ensure that the platform is able to use PSCI to do SMP.

	Arnd

mach header files

[Laurent Pinchart]

Hi,

On Friday 04 April 2014 15:20:41 Thomas Petazzoni wrote:
> On Fri, 4 Apr 2014 13:14:44 +0000, Phil Edworthy wrote:
> > > If it is used only by one driver, then it should go directly into
> > > that driver without a #include. For SoC defines, you'll need to
> > > check with the arm-soc maintainers. I *think* we're trying to get
> > > away from separate arch/arm/mach-* directories, but I'm slightly
> > > out of touch. You certainly should not do anything that breaks
> > > multiplatform builds.
> > 
> > This is where it gets tricky. The clock driver only wants to know
> > about one reg in a block of general system control registers. There
> > is very little input to the clock driver... the SoC code needs to
> > access other registers in the system control block.
> > 
> > I think you are right about trying to avoid separate arch/arm/mach-*
> > directories, which is what prompted my question. So, how best to
> > handle this?
> 
> I believe there are two solutions here:
> 
>  * Have a driver in arch/arm/mach-foo/ for your "system control block"
>    that exposes a small API used by your clock driver in drivers/clk/.
>    The issue is of course always: where to put the header files for
>    this small API.
> 
>  * Have both your clock driver and your system control block driver map
>    the registers they need, even if some of those registers are the
>    same. Then you can use atomic_io_modify() from both drivers to
>    atomically make changes to these registers. This is typically OK if
>    you don't have any performance sensitive usage of those registers
>    (atomic_io_modify accesses are protected by a global spinlock). This
>    is the solution we've used between the Armada watchdog and Armada
>    clocksource drivers, which require accessing a shared register, even
>    though those two drivers are completely unrelated.

Just for what it's worth, we've solved this problem on R-Car Gen2 by passing 
the boot mode using a direct explicit call from platform code to the clock 
driver. See rcar_gen2_clocks_init() in drivers/clk/shmobile/clk-rcar-gen2.c 
and arch/arm/mach-shmobile/setup-rcar-gen2.c.

If we want to get rid of that I'd go for the first option, as the boot mode 
register is really not part of the clock IP core and should not be listed as a 
memory resource in its DT node.

-- 
Regards,

Laurent Pinchart