| Message ID | 877faqjtuk.fsf@belgarion.home (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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.
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. :)
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);