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

Groups > linux.kernel > #1475999 > unrolled thread

[PATCH 2/3] ARM: pxa: lubbock: add pcmcia clock

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

  [PATCH 2/3] ARM: pxa: lubbock: add pcmcia clock Robert Jarzmik <robert.jarzmik@free.fr> - 2016-09-04 21:10 +0200
    Re: [PATCH 2/3] ARM: pxa: lubbock: add pcmcia clock Russell King - ARM Linux <linux@armlinux.org.uk> - 2016-09-04 22:10 +0200
      Re: [PATCH 2/3] ARM: pxa: lubbock: add pcmcia clock Russell King - ARM Linux <linux@armlinux.org.uk> - 2016-09-04 22:40 +0200
      Re: [PATCH 2/3] ARM: pxa: lubbock: add pcmcia clock Robert Jarzmik <robert.jarzmik@free.fr> - 2016-09-05 16:40 +0200
        Re: [PATCH 2/3] ARM: pxa: lubbock: add pcmcia clock Russell King - ARM Linux <linux@armlinux.org.uk> - 2016-09-05 18:10 +0200
          Re: [PATCH 2/3] ARM: pxa: lubbock: add pcmcia clock Robert Jarzmik <robert.jarzmik@free.fr> - 2016-09-05 21:00 +0200
            Re: [PATCH 2/3] ARM: pxa: lubbock: add pcmcia clock Russell King - ARM Linux <linux@armlinux.org.uk> - 2016-09-06 13:30 +0200

#1475999 — [PATCH 2/3] ARM: pxa: lubbock: add pcmcia clock

Add the clock provided to the PCMCIA block so that sa1111_pcmcia_add()
doesn't end up on error while probing "1800" device.

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
 arch/arm/mach-pxa/lubbock.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-pxa/lubbock.c b/arch/arm/mach-pxa/lubbock.c
index cd401546cea8..0bcb107d69b4 100644
--- a/arch/arm/mach-pxa/lubbock.c
+++ b/arch/arm/mach-pxa/lubbock.c
@@ -491,6 +491,7 @@ static void __init lubbock_init(void)
 	gpiod_add_lookup_table(&sa1111_pcmcia_gpio_table);
 
 	clk_add_alias("SA1111_CLK", NULL, "GPIO11_CLK", NULL);
+	clk_add_alias(NULL, "1800", "SA1111_CLK", NULL);
 	pxa_set_udc_info(&udc_info);
 	pxa_set_fb_info(NULL, &sharp_lm8v31);
 	pxa_set_mci_info(&lubbock_mci_platform_data);
-- 
2.1.4

[toc] | [next] | [standalone]

#1476013

On Sun, Sep 04, 2016 at 08:59:46PM +0200, Robert Jarzmik wrote:
> Add the clock provided to the PCMCIA block so that sa1111_pcmcia_add()
> doesn't end up on error while probing "1800" device.

I don't think this is correct - SA1111 is not really the PCMCIA
controller - it's a load of logic which sits between the host
device and the PCMCIA sockets to manage buffers and the PCMCIA
socket control signals.

The timing is done by the SoC, and the PCMCIA driver needs a
clock to compute the timing information for the PCMCIA interface.
Originally, in 2.6.12-rc2, lubbock (and all of the PXA socket
drivers) were using pxa2xx_base.c, which got the clock frequency
via get_memclk_frequency_10khz().

In 2a125dd56b3a ("ARM: pxa: remove get_memclk_frequency_10khz()")
this was changed to use the clk API, and added the "pxa2xx-pcmcia"
clock.  However, it ignored Lubbock, because that doesn't use
_that_ device, but the SA1111 PCMCIA sub-device "1800".  This commit
should have introduced an identical entry for the SA1111 PCMCIA
sub-device.

This is actually one of the problems converting these platforms to
DT - seemingly simple stuff is actually quite complex because the
resources are spread around.  In the case of Lubbock, some of the
resources are on the PXA255 SoC, others are on the SA1111 which
requires a fair amount of initialisation to get to.  I think we're
really into the "combine multiple devices into one logical device"
game (iow, using the component helper), just like DRM and other
subsystems.

The quick fix here is to add a clock for the SA1111 PCMCIA sub-device,
but it still needs to be the SoC memory clock - iow, what
get_memclk_frequency_10khz() would have returned as that's what
pxa2xx_base.c wants.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

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

#1476021

On Sun, Sep 04, 2016 at 09:08:27PM +0100, Russell King - ARM Linux wrote:
> On Sun, Sep 04, 2016 at 08:59:46PM +0200, Robert Jarzmik wrote:
> > Add the clock provided to the PCMCIA block so that sa1111_pcmcia_add()
> > doesn't end up on error while probing "1800" device.
> 
> I don't think this is correct - SA1111 is not really the PCMCIA
> controller - it's a load of logic which sits between the host
> device and the PCMCIA sockets to manage buffers and the PCMCIA
> socket control signals.
> 
> The timing is done by the SoC, and the PCMCIA driver needs a
> clock to compute the timing information for the PCMCIA interface.
> Originally, in 2.6.12-rc2, lubbock (and all of the PXA socket
> drivers) were using pxa2xx_base.c, which got the clock frequency
> via get_memclk_frequency_10khz().
> 
> In 2a125dd56b3a ("ARM: pxa: remove get_memclk_frequency_10khz()")
> this was changed to use the clk API, and added the "pxa2xx-pcmcia"
> clock.  However, it ignored Lubbock, because that doesn't use
> _that_ device, but the SA1111 PCMCIA sub-device "1800".  This commit
> should have introduced an identical entry for the SA1111 PCMCIA
> sub-device.
> 
> This is actually one of the problems converting these platforms to
> DT - seemingly simple stuff is actually quite complex because the
> resources are spread around.  In the case of Lubbock, some of the
> resources are on the PXA255 SoC, others are on the SA1111 which
> requires a fair amount of initialisation to get to.  I think we're
> really into the "combine multiple devices into one logical device"
> game (iow, using the component helper), just like DRM and other
> subsystems.
> 
> The quick fix here is to add a clock for the SA1111 PCMCIA sub-device,
> but it still needs to be the SoC memory clock - iow, what
> get_memclk_frequency_10khz() would have returned as that's what
> pxa2xx_base.c wants.

IOW, something like:

 arch/arm/mach-pxa/lubbock.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-pxa/lubbock.c b/arch/arm/mach-pxa/lubbock.c
index cd401546cea8..d7c5fb00da7a 100644
--- a/arch/arm/mach-pxa/lubbock.c
+++ b/arch/arm/mach-pxa/lubbock.c
@@ -149,6 +149,20 @@ static struct gpiod_lookup_table sa1111_pcmcia_gpio_table = {
 	},
 };
 
+static void lubbock_init_pcmcia(void)
+{
+	struct clk *clk;
+
+	gpiod_add_lookup_table(&sa1111_pcmcia_gpio_table);
+
+	/* Add an alias for the SA1111 PCMCIA clock */
+	clk = clk_get_sys("pxa2xx-pcmcia", NULL);
+	if (!IS_ERR(clk)) {
+		clkdev_create(clk, NULL, "1800");
+		clk_put(clk);
+	}
+}
+
 static struct resource sa1111_resources[] = {
 	[0] = {
 		.start	= 0x10000000,
@@ -488,7 +502,7 @@ static void __init lubbock_init(void)
 	pxa_set_btuart_info(NULL);
 	pxa_set_stuart_info(NULL);
 
-	gpiod_add_lookup_table(&sa1111_pcmcia_gpio_table);
+	lubbock_init_pcmcia();
 
 	clk_add_alias("SA1111_CLK", NULL, "GPIO11_CLK", NULL);
 	pxa_set_udc_info(&udc_info);

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

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

#1476512

Russell King - ARM Linux <linux@armlinux.org.uk> writes:

> On Sun, Sep 04, 2016 at 08:59:46PM +0200, Robert Jarzmik wrote:
>> Add the clock provided to the PCMCIA block so that sa1111_pcmcia_add()
>> doesn't end up on error while probing "1800" device.
>
> I don't think this is correct - SA1111 is not really the PCMCIA
> controller - it's a load of logic which sits between the host
> device and the PCMCIA sockets to manage buffers and the PCMCIA
> socket control signals.

Gah I was naively thinking the SA1111 clock was used in the SA1111 to sample the
CF lines, and as a consequence, that the asynchronous nPIOWait, nPIORead,
nPIOWrite were derived from it.

I suppose the SA1111 takes 2 clocks, one for PS/2 etc ..., and SDCLK<1> for the
PCMCIA operations, while I was thinking SDCLK<1> input to SA1111 was only for
alternate bus-master operations.

> The quick fix here is to add a clock for the SA1111 PCMCIA sub-device,
> but it still needs to be the SoC memory clock - iow, what
> get_memclk_frequency_10khz() would have returned as that's what
> pxa2xx_base.c wants.
Right.

Would grant your sign-off to your patch in [1], if the commit message is good
enough for you (I can fix it up if the wording is too ... french) ?

Cheers.

-- 
Robert

[1]
---8>---
From f8ba8367164056d7a79052b2b10fcecabd8e854d Mon Sep 17 00:00:00 2001
From: Russell King <rmk+kernel@armlinux.org.uk>
Date: Sun, 4 Sep 2016 19:59:19 +0200
Subject: [PATCH] ARM: pxa: lubbock: add pcmcia clock

Add the clock provided to the PCMCIA block so that sa1111_pcmcia_add()
doesn't end up on error while probing "1800" device.

[To be confirmed] Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 arch/arm/mach-pxa/lubbock.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-pxa/lubbock.c b/arch/arm/mach-pxa/lubbock.c
index cd401546cea8..d7c5fb00da7a 100644
--- a/arch/arm/mach-pxa/lubbock.c
+++ b/arch/arm/mach-pxa/lubbock.c
@@ -149,6 +149,20 @@ static struct gpiod_lookup_table sa1111_pcmcia_gpio_table = {
 	},
 };
 
+static void lubbock_init_pcmcia(void)
+{
+	struct clk *clk;
+
+	gpiod_add_lookup_table(&sa1111_pcmcia_gpio_table);
+
+	/* Add an alias for the SA1111 PCMCIA clock */
+	clk = clk_get_sys("pxa2xx-pcmcia", NULL);
+	if (!IS_ERR(clk)) {
+		clkdev_create(clk, NULL, "1800");
+		clk_put(clk);
+	}
+}
+
 static struct resource sa1111_resources[] = {
 	[0] = {
 		.start	= 0x10000000,
@@ -488,7 +502,7 @@ static void __init lubbock_init(void)
 	pxa_set_btuart_info(NULL);
 	pxa_set_stuart_info(NULL);
 
-	gpiod_add_lookup_table(&sa1111_pcmcia_gpio_table);
+	lubbock_init_pcmcia();
 
 	clk_add_alias("SA1111_CLK", NULL, "GPIO11_CLK", NULL);
 	pxa_set_udc_info(&udc_info);
-- 
2.1.4

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

#1476580

On Mon, Sep 05, 2016 at 04:37:23PM +0200, Robert Jarzmik wrote:
> Russell King - ARM Linux <linux@armlinux.org.uk> writes:
> 
> > On Sun, Sep 04, 2016 at 08:59:46PM +0200, Robert Jarzmik wrote:
> >> Add the clock provided to the PCMCIA block so that sa1111_pcmcia_add()
> >> doesn't end up on error while probing "1800" device.
> >
> > I don't think this is correct - SA1111 is not really the PCMCIA
> > controller - it's a load of logic which sits between the host
> > device and the PCMCIA sockets to manage buffers and the PCMCIA
> > socket control signals.
> 
> Gah I was naively thinking the SA1111 clock was used in the SA1111 to
> sample the CF lines, and as a consequence, that the asynchronous
> nPIOWait, nPIORead, nPIOWrite were derived from it.
> 
> I suppose the SA1111 takes 2 clocks, one for PS/2 etc ..., and SDCLK<1>
> for the PCMCIA operations, while I was thinking SDCLK<1> input to SA1111
> was only for alternate bus-master operations.

From what I remember, the SA1111 takes the 3.6864MHz clock input as an
input to its own PLL to generate the clocks it needs internally.  All
the PCMCIA timing is handled by the SA1110 or PXA.

The reason that we need to tell the SA1111 PCMCIA device about the PXA
clock is not because the SA1111 uses it, but because the driver needs it
to work out the correct timing information to program the SA1110 or PXA
access times.

This means that the current driver structure does _not_ fit the DT model
at all - I hope that no one has plans to construct a DT model based on
the current code structure, because that would be totally wrong.

> > The quick fix here is to add a clock for the SA1111 PCMCIA sub-device,
> > but it still needs to be the SoC memory clock - iow, what
> > get_memclk_frequency_10khz() would have returned as that's what
> > pxa2xx_base.c wants.
> Right.
> 
> Would grant your sign-off to your patch in [1], if the commit message
> is good enough for you (I can fix it up if the wording is too ... french) ?

Thanks, the commit message is mostly fine, I think it ought to also
point out that it's used by the driver to derive the timing information,
and doesn't physically exist to the SA1111.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

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

#1476903

Russell King - ARM Linux <linux@armlinux.org.uk> writes:

> On Mon, Sep 05, 2016 at 04:37:23PM +0200, Robert Jarzmik wrote:
> From what I remember, the SA1111 takes the 3.6864MHz clock input as an
> input to its own PLL to generate the clocks it needs internally.  All
> the PCMCIA timing is handled by the SA1110 or PXA.
>
> The reason that we need to tell the SA1111 PCMCIA device about the PXA
> clock is not because the SA1111 uses it, but because the driver needs it
> to work out the correct timing information to program the SA1110 or PXA
> access times.

I've been pondering that last sentence for a bit and what appeared to me is that
between the PXA and the SA1111, as there is no clock from one to the other,
ie. that the SA1111 clock for PCMCIA signals is independent from any PXA clock
(well, excepting it's a PLL from a 3.6864MHz, but let's forget that), the
contract binding the PXA and the SA1111 is a _timings_ one rather that a clock
one.

Or said differently, the 2 IPs must agree on timing values in order to
inter-operate, and so should the device drivers. Their internal clock frequencies
are marginal, as on an asynchronous interface, what is important is that PXA
accepts nPIOR within [a .. b] and SA1111 accepts nPIOR within [c .. d], and the
intersection is used to setup them both.

I have not really looked into the PCMCIA structure, but I suppose it's clock
based today.

> This means that the current driver structure does _not_ fit the DT model
> at all - I hope that no one has plans to construct a DT model based on
> the current code structure, because that would be totally wrong.
Oh there are not that many candidates these days for PCMCIA nor PXA. I don't see
any other volunteer than me on the PXA front, and even if I find the time in 1
year or 2, I will remember this conversation and will discuss thoroughly before
trying to code.

> Thanks, the commit message is mostly fine, I think it ought to also
> point out that it's used by the driver to derive the timing information,
> and doesn't physically exist to the SA1111.

Something like ?
"The added clock doesn't actually exist, ie. there is no physical clock line
from the PXA to the SA1111 on lubbock used by the PCMCIA block on the
SA1111. The clocking information is only used to setup the memory bus timings."

Cheers.

-- 
Robert

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

#1477285

On Mon, Sep 05, 2016 at 08:50:53PM +0200, Robert Jarzmik wrote:
> Russell King - ARM Linux <linux@armlinux.org.uk> writes:
> 
> > On Mon, Sep 05, 2016 at 04:37:23PM +0200, Robert Jarzmik wrote:
> > From what I remember, the SA1111 takes the 3.6864MHz clock input as an
> > input to its own PLL to generate the clocks it needs internally.  All
> > the PCMCIA timing is handled by the SA1110 or PXA.
> >
> > The reason that we need to tell the SA1111 PCMCIA device about the PXA
> > clock is not because the SA1111 uses it, but because the driver needs it
> > to work out the correct timing information to program the SA1110 or PXA
> > access times.
> 
> I've been pondering that last sentence for a bit and what appeared to me
> is that between the PXA and the SA1111, as there is no clock from one to
> the other, ie. that the SA1111 clock for PCMCIA signals is independent
> from any PXA clock (well, excepting it's a PLL from a 3.6864MHz, but
> let's forget that), the contract binding the PXA and the SA1111 is a
> _timings_ one rather that a clock one.

It isn't.

The SA1110 and PXA already support PCMCIA/CF sockets, but need:
* external buffers to isolate the address/data buses from the sockets,
  and to provide level shifting for 5V setups.
* combinatorial logic to decode the socket control signals to route
  them to the appropriate socket and control the buffers.

The logic required for this is detailed in the SA1110 manual, and
probably the PXA manuals as well.

The SA1111 provides a single-chip solution to this, containing the
buffers and combinatorial logic.  The SA1111 provides a functional
description of this combinatorial logic.

The timing concerned here is the _access_ timing, and that is purely
controlled by the SA1110 or PXA.  The SA1111 plays no part in that,
apart from adding a little latency to the signals that flow through it,
in much the same way as the non-SA1111 solutions also add extra latency.

This is further evidenced by there being clock enable bits for everything
except the PCMCIA interface within the SA1111, and the fact that the
individual unit clocks must only be enabled once the internal clocks of
the SA1111 have been initialised by software.

> Or said differently, the 2 IPs must agree on timing values in order to
> inter-operate, and so should the device drivers.

There is no timing to program into the SA1111.

> I have not really looked into the PCMCIA structure, but I suppose it's clock
> based today.

PCMCIA doesn't have a clock.  It's an extension of buses like ISA, where
accesses are timed by the length of the memory/io read/write signals, and
an optional wait signal that is driven by the card.

If the card says "I want an access time of 300ns" then the SA1110 or PXA
must be programmed to achieve at least a 300ns access time to the card -
iow, the appropriate access enable signal must be asserted for at least
the specified time and data read after that period has elapsed.

As the SA1111 will be an implementation of the combinatorial logic shown
in the SA1110 manual, no clocks in the SA1111 come into play.

> Something like ?
> "The added clock doesn't actually exist, ie. there is no physical clock
> line from the PXA to the SA1111 on lubbock used by the PCMCIA block on
> the SA1111. The clocking information is only used to setup the memory
> bus timings."

Already committed with a modified commit message, thanks anyway. :)

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web