* Re: [PATCH 04/10] sata: hardreset: retry if phys link is down
From: Tejun Heo @ 2017-01-15 23:10 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Kevin Hilman, Sekhar Nori, Patrick Titiano, Michael Turquette,
Rob Herring, Mark Rutland, Russell King, David Lechner, linux-ide,
devicetree, linux-kernel, linux-arm-kernel
In-Reply-To: <1484311084-31547-5-git-send-email-bgolaszewski@baylibre.com>
Hello,
On Fri, Jan 13, 2017 at 01:37:58PM +0100, Bartosz Golaszewski wrote:
> The sata core driver already retries to resume the link because some
> controllers ignore writes to the SControl register.
>
> We have a use case with the da850 SATA controller where at PLL0
> frequency of 456MHz (needed to properly service the LCD controller)
> the chip becomes unstable and the hardreset operation is ignored the
> first time 50% of times.
>
> Retrying just the resume operation doesn't work - we need to issue
> the phy/wake reset again to make it work.
>
> If ata_phys_link_offline() returns true in sata_link_hardreset(),
> retry a couple times before really giving up.
I think it'd be better to implement the driver specific implementation
rather than changing the behavior for everybody.
Thanks.
--
tejun
^ permalink raw reply
* Re: command emulation fix
From: Tejun Heo @ 2017-01-15 23:07 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-ide
In-Reply-To: <1484412602-11801-1-git-send-email-hch@lst.de>
Hello, Christoph.
On Sat, Jan 14, 2017 at 05:50:01PM +0100, Christoph Hellwig wrote:
> Of course we can't just do a blind GFP_NOIO from ->queuecommand,
> mea culpa. For most of the commands GFP_ATOMIC should be absolutely
> fine as they are only called from the probe path. If you're
> paranoid TRIM might want a sector-sized mempool, but other common
> drivers like NVMe rely on plain GFP_ATOMIC allocations for those
> as well. Let me know, and I'll send a mempool patch on top of it.
Ugh... I don't know. What we had previously is always guaranteed to
work. I'm not really liking the fact that we're adding a possibility
of failure here. Even if we do mempool, we would still have to
protect it with a spinlock as mempool only guarantees one allocation
at a time. Until we have a better solution, can we please revert back
to where we were at least for the buffers needed from atomic context?
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH 0/3] ata: add m68k/Atari Falcon PATA support
From: Finn Thain @ 2017-01-15 4:42 UTC (permalink / raw)
To: Michael Schmitz
Cc: Bartlomiej Zolnierkiewicz, Tejun Heo, Geert Uytterhoeven,
linux-ide, Linux/m68k, Linux Kernel Development, Andreas Schwab
In-Reply-To: <a637f52e-d70a-4b10-3998-9db5c2534d83@gmail.com>
On Sun, 15 Jan 2017, Michael Schmitz wrote:
> Am 15.01.2017 um 12:47 schrieb Finn Thain:
>
> > For the sake of discussion, I'll assume that the FDC driver will not
> > be using DMA. (Perhaps FDC and SCSI can share the ST-DMA chip, using
> > the present locking mechanism, but it would not simplify things much:
> > when IDE no longer participates in that locking mechanism then both
> > FDC and SCSI drivers have to solve the same issues.)
>
> Correct - IIRC the FDC registers are also accessible 'through' the
> ST-DMA registers only so the same problem WRT DMA status arises.
>
I had not considered that limitation.
> > What compiler are you using, BTW? Are you still using the gcc-4.6.3
> > m68k cross-compiler from kernel.org? I had to abandon it in order to
> > get my SCSI driver patches to work.
>
> 4.6.3 is the version I still use. You had trouble with that one? I
> recall some discussion on gcc versions on the m68k list a while back,
> just never seemed to see any problems...
>
... none that could be easily blamed on the compiler, anyway.
The gcc 4.6.3 issue affecting my builds was discussed back in
November. There are alternative compilers available:
http://marc.info/?l=linux-m68k&m=147859596303294&w=2
http://marc.info/?l=linux-m68k&m=147859567903210&w=2
> >> I need to think about that some more - if no DMA is in progress we
> >> can safely peek at the SCSI registers. So the logic could be changed
> >> to test for DMA operation first, and just try and service the
> >> interrupt if DMA wasn't active.
> >>
> >
> > OK, so based on the above, we handle the possible IDE interrupt
> > (without checking DMA registers), handle the possible FDC interrupt
> > (again without checking DMA registers) and finally handle the possible
> > SCSI interrupt.
>
> No, we can't check either FDC or SCSI interrupts (or indeed any chip
> registers) without touching the ST-DMA. The moment we select a FDC or
> SCSI register for read, DMA is terminated no questions asked.
>
Perhaps we can convert DMA operations to PDMA (by polling with local irqs
disabled) and avoid the whole problem of interrupt handlers executing
during DMA transfers. The docs suggest that it is doable.
"Poll or service the Disk Driver Controller interrupt on the MK68901 MFP
General Purpose I/O Register to detect the completion of a WD1772 FDC
command. Do not poll the FDC Busy or DMA Sector Count Zero status bits."
-- ST HW Spec, p. 36.
http://dev-docs.atariforge.org/files/ST_HW_Spec_1-7-1986.pdf
On page 18 there is an algorithm for floppy writes which is interesting.
I suspect that we will need to keep the FDC idle during SCSI transfers
(and vice versa) much as the present stdma.c lock does.
"The interrupt outputs of the internal floppy disk controller and the
external ACSI DMA port are logically OR'ed. The pin of the MFP GPIP will
read as a '0' if either the FDC or a selected ACSI device controller is
asserting its interrupt request."
-- ACSI/DMA Integration Guide, p.16.
http://dev-docs.atariforge.org/files/ACSI_DMA_Guide_6-28-1991.pdf
Polling the logically OR'ed interrupt sources to detect end-of-DMA will
not be reliable unless we disable those sources that aren't relevant.
Otherwise we access the DMA registers too early (which IIUC would kill the
transfer). I'm afraid we shall have to expect that a few transfers will be
interrupted by other devices in this way, and carefully check for this.
For example, the 5380 SCSI bus reset interrupt is not maskable, which
could affect FDC transfers. If this terminated the polling for DMA
completion, the FDC driver then has to access the FDC registers and
confirm that the transfer was not terminated early.
--
^ permalink raw reply
* Re: [PATCH 0/3] ata: add m68k/Atari Falcon PATA support
From: Michael Schmitz @ 2017-01-15 1:48 UTC (permalink / raw)
To: Finn Thain
Cc: Bartlomiej Zolnierkiewicz, Tejun Heo, Geert Uytterhoeven,
linux-ide, Linux/m68k, Linux Kernel Development, Andreas Schwab
In-Reply-To: <alpine.LNX.2.00.1701151046500.352@nippy.intranet>
Hi Finn,
Am 15.01.2017 um 12:47 schrieb Finn Thain:
> For the sake of discussion, I'll assume that the FDC driver will not
> be using DMA. (Perhaps FDC and SCSI can share the ST-DMA chip, using
> the present locking mechanism, but it would not simplify things much:
> when IDE no longer participates in that locking mechanism then both
> FDC and SCSI drivers have to solve the same issues.)
Correct - IIRC the FDC registers are also accessible 'through' the
ST-DMA registers only so the same problem WRT DMA status arises.
> What compiler are you using, BTW? Are you still using the gcc-4.6.3
> m68k cross-compiler from kernel.org? I had to abandon it in order to
> get my SCSI driver patches to work.
4.6.3 is the version I still use. You had trouble with that one? I
recall some discussion on gcc versions on the m68k list a while back,
just never seemed to see any problems...
>> I need to think about that some more - if no DMA is in progress we
>> can safely peek at the SCSI registers. So the logic could be
>> changed to test for DMA operation first, and just try and service
>> the interrupt if DMA wasn't active.
>>
>
> OK, so based on the above, we handle the possible IDE interrupt
> (without checking DMA registers), handle the possible FDC interrupt
> (again without checking DMA registers) and finally handle the
> possible SCSI interrupt.
No, we can't check either FDC or SCSI interrupts (or indeed any chip
registers) without touching the ST-DMA. The moment we select a FDC or
SCSI register for read, DMA is terminated no questions asked.
> The core 5380 driver knows whether or not it has started a DMA. The
> atari_scsi driver also knows that no other Falcon driver uses DMA. So
> the atari_scsi handler only has to figure out whether the interrupt
> was asserted by the ST-DMA chip or the 5380 chip, or neither. (The
> "neither" possility arises when IDE ditches the the stdma.c lock
> mechanism.)
>
> Without the stdma.c lock, any or all of these interrupts could be
> asserted simultaneously, so the IDE and FDC drivers need to be able
> to do the right thing in the presence of the other interrupts and do
> so without accessing the ST-DMA chip. And the SCSI interrupt handler
> needs to do the right thing when there is no DMA interrupt, and yet a
> DMA is running.
Again, whenever DMA was running (and it might still be), we have to stop
it in order to look at FDC or SCSI registers. Utter braindamage, but
> Perhaps we could reverse the algorithm in scsi_falcon_intr(): if
> NCR5380_intr() completes with IRQ_HANDLED and the core 5380 driver is
> no longer in DMA, then check the ST-DMA registers for errors etc.
>
> Alternatively, if NCR5380_intr() returns IRQ_NONE, then do nothing at
> all, on the basis that the interrupt was handled by FDC or IDE.
We may only call NCR5380_intr() if DMA hadn't been active (or we are
sure it's completed, i.e. transfer address == end address. If that's
even possible). If the DMA is still ongoing, we have the choice of
punting (hoping for a command timeout to happen and clean up the mess),
or terminate DMA (by selecting the SCSI chip registers instead of the
DMA ones) and deal with the fallout.
> In this situation, I gather that atari_scsi could miss out on a DMA
> completion interrupt from the ST-DMA chip, which could lead to
> command timeout?
Doing nothing if DMA is enabled (and IDE had successfully handled an
interrupt!) would cause us to miss a stacked interrupt, yes. Timeout
would likely ensue. Not sure it's wise to kludge around that using a
watchdog timer activated in case we're not sure of the DMA completion
state...
>
>> If DMA has been in progress, I'm not sure that we can figure out if
>> it's still active from looking at the status register (that is,
>> whether bits 0 or 1 are set while DMA is ongoing). We'd have to
>> peek at the DMA status register (or DMA address registers) without
>> first stopping DMA, which the current driver does. The docs seem to
>> advise against that. If DMA was in progress, stopping it would
>> likely leave us with residual bytes to be transferred -
>
> I can't comment on the Profibuch doc or the ST-DMA chip details
> (Andreas?)
>
> I suspect it has to be tried.
Yes, I fear I'll have to just try what happens if SCSI and IDE raise an
interrupt at the same time.
For now, polled IDE might be working well enough (haven't seen a huge
impact in IDE-only test workloads, I'll have to check impact on lots of
seeks across the whole disk a lot harder though). I need to recheck the
old IDE driver with my current combined test workload though (my second
4 GB Seagate disk has finally kicked the bucket, after the latest power
brownout).
>
>> we'd have to handle that transfer as we would any other DMA error
>> (from memory, probably best to retry the entire command, or
>> transfer the remaining bytes using PIO if we're sure no bytes have
>> been lost).
>>
>
> Allowing the command to fail should be fine so long as the 5380
> driver sends the correct result code to the mid-layer. To attempt to
> complete the command after a failed DMA is needless complexity, and
> it's a trick that probably can't be pulled off reliably anyway.
Yep, we've been there before (when my CT60 caused SCSI DMA errors and
lost bytes). Not sure anymore what the correct result code was...
Cheers,
Michael
^ permalink raw reply
* Re: [PATCH 0/3] ata: add m68k/Atari Falcon PATA support
From: Finn Thain @ 2017-01-14 23:47 UTC (permalink / raw)
To: Michael Schmitz
Cc: Bartlomiej Zolnierkiewicz, Tejun Heo, Geert Uytterhoeven,
linux-ide, Linux/m68k, Linux Kernel Development, Andreas Schwab
In-Reply-To: <c55cb48c-7200-9577-974b-3c080e141907@gmail.com>
On Sat, 14 Jan 2017, Michael Schmitz wrote:
> Am 13.01.2017 um 15:33 schrieb Finn Thain:
>
> >> The case I'm worried about is both IDE and SCSI raising an interrupt.
> >> We don't currently mask the IDE/ST-DMA interrupt so a stacked
> >> interrupt must be processed in the same pass as the initial interrupt
> >> or it will get dropped. We'd have to peek at the DMA registers to
> >> check the SCSI or floppy interrupt status, and we just can't safely
> >> do that. So races of this kind are currently prevented by including
> >> IDE in the IRQ locking process.
> >>
> >> Whether it's possible to mask the interrupt, do one pass, unmask and
> >> process the second interrupt I don't know.
> >
> > Would that require handling the SCSI DMA interrupt in the first pass?
> > Or handling IDE first, and ensuring that the IDE handler does not
> > access ST-DMA registers? What about FDC?
>
> Handling the IDE interrupt first I think, then looking at the DMA (for
> SCSI or FDC).
>
For the sake of discussion, I'll assume that the FDC driver will not be
using DMA. (Perhaps FDC and SCSI can share the ST-DMA chip, using the
present locking mechanism, but it would not simplify things much: when IDE
no longer participates in that locking mechanism then both FDC and SCSI
drivers have to solve the same issues.)
> > The atari_scsi handler accesses the ST-DMA registers; it can do so
> > because it knows that any DMA must have completed -- it can infer this
> > because a simultaneous pending interrupt from FDC or IDE is impossible
> > due to stdma_lock().
>
> libata dropped the locking (and does not use IDE interrupts at present
> so it seems to be safe. Still testing - I've seen IO errors, and that's
> a bit of a worry).
>
What compiler are you using, BTW? Are you still using the gcc-4.6.3 m68k
cross-compiler from kernel.org? I had to abandon it in order to get my
SCSI driver patches to work.
> > Your suggestion would seem to allow other pending interrupts, hence
> > the atari_scsi interrupt handler logic has to be tossed out. What
> > logic would replace it?
>
> I need to think about that some more - if no DMA is in progress we can
> safely peek at the SCSI registers. So the logic could be changed to test
> for DMA operation first, and just try and service the interrupt if DMA
> wasn't active.
>
OK, so based on the above, we handle the possible IDE interrupt (without
checking DMA registers), handle the possible FDC interrupt (again without
checking DMA registers) and finally handle the possible SCSI interrupt.
The core 5380 driver knows whether or not it has started a DMA. The
atari_scsi driver also knows that no other Falcon driver uses DMA. So the
atari_scsi handler only has to figure out whether the interrupt was
asserted by the ST-DMA chip or the 5380 chip, or neither. (The "neither"
possility arises when IDE ditches the the stdma.c lock mechanism.)
Without the stdma.c lock, any or all of these interrupts could be asserted
simultaneously, so the IDE and FDC drivers need to be able to do the right
thing in the presence of the other interrupts and do so without accessing
the ST-DMA chip. And the SCSI interrupt handler needs to do the right
thing when there is no DMA interrupt, and yet a DMA is running.
Perhaps we could reverse the algorithm in scsi_falcon_intr(): if
NCR5380_intr() completes with IRQ_HANDLED and the core 5380 driver is no
longer in DMA, then check the ST-DMA registers for errors etc.
Alternatively, if NCR5380_intr() returns IRQ_NONE, then do nothing at all,
on the basis that the interrupt was handled by FDC or IDE. In this
situation, I gather that atari_scsi could miss out on a DMA completion
interrupt from the ST-DMA chip, which could lead to command timeout?
> If DMA has been in progress, I'm not sure that we can figure out if it's
> still active from looking at the status register (that is, whether bits
> 0 or 1 are set while DMA is ongoing). We'd have to peek at the DMA
> status register (or DMA address registers) without first stopping DMA,
> which the current driver does. The docs seem to advise against that. If
> DMA was in progress, stopping it would likely leave us with residual
> bytes to be transferred -
I can't comment on the Profibuch doc or the ST-DMA chip details (Andreas?)
I suspect it has to be tried.
> we'd have to handle that transfer as we would any other DMA error (from
> memory, probably best to retry the entire command, or transfer the
> remaining bytes using PIO if we're sure no bytes have been lost).
>
Allowing the command to fail should be fine so long as the 5380 driver
sends the correct result code to the mid-layer. To attempt to complete the
command after a failed DMA is needless complexity, and it's a trick that
probably can't be pulled off reliably anyway.
--
> > If all else fails, perhaps we could inhibit DMA entirely when the new
> > ATA driver is loaded. Then we can just dispatch the ST-DMA irq like a
> > shared irq. I'm sure that atari_scsi can work without DMA. No idea
> > about the FDC driver though (ataflop.c).
>
> Yes, SCSI can work using PIO but's it a real dog. Been there, done that
> (about 20 years ago). I know nothing about the FDC chip though.
>
> > Another solution would be to dedicate the DMA function to atari_scsi,
> > and then mask the FDC and IDE interrupts during each DMA transfer. But
> > once again, this would mean changing the FDC driver to eliminate DMA,
> > if that is possible. From the schematic it looks the the FDC chip,
> > "AJAX", is another custom ...
> > http://dev-docs.atariforge.org/files/Falcon030_Schematic.pdf
> >
> > Unfortunately my grasp of the ST hardware reflects my inability to
> > read German; those who can may want to take a look at "ATARI Profibuch
> > ST-STE-TT.pdf".
>
> I'll reread the ST-DMA description (and the FDC one).
>
> Let me know if you think this could work ...
>
> Cheers,
>
> Michael
>
^ permalink raw reply
* command emulation fix
From: Christoph Hellwig @ 2017-01-14 16:50 UTC (permalink / raw)
To: tj; +Cc: linux-ide
Of course we can't just do a blind GFP_NOIO from ->queuecommand,
mea culpa. For most of the commands GFP_ATOMIC should be absolutely
fine as they are only called from the probe path. If you're
paranoid TRIM might want a sector-sized mempool, but other common
drivers like NVMe rely on plain GFP_ATOMIC allocations for those
as well. Let me know, and I'll send a mempool patch on top of it.
^ permalink raw reply
* [PATCH] libata: switch allocations for command emulation to GFP_ATOMIC
From: Christoph Hellwig @ 2017-01-14 16:50 UTC (permalink / raw)
To: tj; +Cc: linux-ide
In-Reply-To: <1484412602-11801-1-git-send-email-hch@lst.de>
->queuecommand may be called under a spinlock or inside a critical section,
so we need to use GFP_ATOMIC for them.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Fixes a234f739 ("libata: switch to dynamic allocation instead of ata_scsi_rbuf")
---
drivers/ata/libata-scsi.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 4de273b..465fad1 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -2074,7 +2074,7 @@ static void ata_scsi_rbuf_fill(struct ata_scsi_args *args,
struct scsi_cmnd *cmd = args->cmd;
u8 *buf;
- buf = kzalloc(ATA_SCSI_RBUF_SIZE, GFP_NOIO);
+ buf = kzalloc(ATA_SCSI_RBUF_SIZE, GFP_ATOMIC);
if (!buf) {
ata_scsi_set_sense(args->dev, cmd, NOT_READY, 0x08, 0);
return;
@@ -3325,7 +3325,7 @@ static ssize_t ata_format_dsm_trim_descr(struct scsi_cmnd *cmd, u32 trmax,
__le64 *buf;
u32 i = 0;
- buf = kzalloc(cmd->device->sector_size, GFP_NOFS);
+ buf = kzalloc(cmd->device->sector_size, GFP_ATOMIC);
if (!buf)
return -ENOMEM;
@@ -3362,7 +3362,7 @@ static ssize_t ata_format_sct_write_same(struct scsi_cmnd *cmd, u64 lba,
size_t r;
u16 *buf;
- buf = kzalloc(cmd->device->sector_size, GFP_NOIO);
+ buf = kzalloc(cmd->device->sector_size, GFP_ATOMIC);
if (!buf)
return -ENOMEM;
--
2.1.4
^ permalink raw reply related
* Re: [PATCH 0/3] ata: add m68k/Atari Falcon PATA support
From: Michael Schmitz @ 2017-01-14 8:55 UTC (permalink / raw)
To: Finn Thain
Cc: Bartlomiej Zolnierkiewicz, Tejun Heo, Geert Uytterhoeven,
linux-ide, Linux/m68k, Linux Kernel Development, Andreas Schwab
In-Reply-To: <alpine.LNX.2.00.1701111053340.22260@nippy.intranet>
Hi Finn,
Am 13.01.2017 um 15:33 schrieb Finn Thain:
>> The case I'm worried about is both IDE and SCSI raising an interrupt. We
>> don't currently mask the IDE/ST-DMA interrupt so a stacked interrupt
>> must be processed in the same pass as the initial interrupt or it will
>> get dropped. We'd have to peek at the DMA registers to check the SCSI or
>> floppy interrupt status, and we just can't safely do that. So races of
>> this kind are currently prevented by including IDE in the IRQ locking
>> process.
>>
>> Whether it's possible to mask the interrupt, do one pass, unmask and
>> process the second interrupt I don't know.
>
> Would that require handling the SCSI DMA interrupt in the first pass? Or
> handling IDE first, and ensuring that the IDE handler does not access
> ST-DMA registers? What about FDC?
Handling the IDE interrupt first I think, then looking at the DMA (for
SCSI or FDC).
> The atari_scsi handler accesses the ST-DMA registers; it can do so because
> it knows that any DMA must have completed -- it can infer this because a
> simultaneous pending interrupt from FDC or IDE is impossible due to
> stdma_lock().
libata dropped the locking (and does not use IDE interrupts at present
so it seems to be safe. Still testing - I've seen IO errors, and that's
a bit of a worry).
> Your suggestion would seem to allow other pending interrupts, hence the
> atari_scsi interrupt handler logic has to be tossed out. What logic would
> replace it?
I need to think about that some more - if no DMA is in progress we can
safely peek at the SCSI registers. So the logic could be changed to test
for DMA operation first, and just try and service the interrupt if DMA
wasn't active.
If DMA has been in progress, I'm not sure that we can figure out if it's
still active from looking at the status register (that is, whether bits
0 or 1 are set while DMA is ongoing). We'd have to peek at the DMA
status register (or DMA address registers) without first stopping DMA,
which the current driver does. The docs seem to advise against that.
If DMA was in progress, stopping it would likely leave us with residual
bytes to be transferred - we'd have to handle that transfer as we would
any other DMA error (from memory, probably best to retry the entire
command, or transfer the remaining bytes using PIO if we're sure no
bytes have been lost).
> If all else fails, perhaps we could inhibit DMA entirely when the new ATA
> driver is loaded. Then we can just dispatch the ST-DMA irq like a shared
> irq. I'm sure that atari_scsi can work without DMA. No idea about the FDC
> driver though (ataflop.c).
Yes, SCSI can work using PIO but's it a real dog. Been there, done that
(about 20 years ago). I know nothing about the FDC chip though.
> Another solution would be to dedicate the DMA function to atari_scsi, and
> then mask the FDC and IDE interrupts during each DMA transfer. But once
> again, this would mean changing the FDC driver to eliminate DMA, if that
> is possible. From the schematic it looks the the FDC chip, "AJAX", is
> another custom ...
> http://dev-docs.atariforge.org/files/Falcon030_Schematic.pdf
>
> Unfortunately my grasp of the ST hardware reflects my inability to read
> German; those who can may want to take a look at "ATARI Profibuch
> ST-STE-TT.pdf".
I'll reread the ST-DMA description (and the FDC one).
Let me know if you think this could work ...
Cheers,
Michael
^ permalink raw reply
* Re: [libata] a234f7395c: BUG:sleeping_function_called_from_invalid_context_at_mm/slab.h
From: Christoph Hellwig @ 2017-01-14 7:32 UTC (permalink / raw)
To: kernel test robot
Cc: Christoph Hellwig, Tejun Heo, linux-ide, Stephen Rothwell, lkp
In-Reply-To: <20170113222659.GA105338@inn.lkp.intel.com>
Ok, plain GFP_NOFS allocations obviously don't do it inside
->queuecommand context. I'll cook up a fix later today.
^ permalink raw reply
* Re: [PATCH 09/10] ARM: dts: da850: add the SATA node
From: David Lechner @ 2017-01-13 19:36 UTC (permalink / raw)
To: Bartosz Golaszewski, Kevin Hilman, Sekhar Nori, Patrick Titiano,
Michael Turquette, Tejun Heo, Rob Herring, Mark Rutland,
Russell King
Cc: linux-ide, devicetree, linux-kernel, linux-arm-kernel
In-Reply-To: <1484311084-31547-10-git-send-email-bgolaszewski@baylibre.com>
On 01/13/2017 06:38 AM, Bartosz Golaszewski wrote:
> Add the SATA node to the da850 device tree.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
> arch/arm/boot/dts/da850.dtsi | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
> index 1f6a47d..f5086b1 100644
> --- a/arch/arm/boot/dts/da850.dtsi
> +++ b/arch/arm/boot/dts/da850.dtsi
> @@ -427,6 +427,12 @@
> phy-names = "usb-phy";
> status = "disabled";
> };
> + sata: ahci@0x218000 {
0x needs to be omitted.
sata: ahci@218000 {
> + compatible = "ti,da850-ahci";
> + reg = <0x218000 0x2000>, <0x22c018 0x4>;
> + interrupts = <67>;
> + status = "disabled";
> + };
> mdio: mdio@224000 {
> compatible = "ti,davinci_mdio";
> #address-cells = <1>;
>
^ permalink raw reply
* Re: [PATCH 07/10] sata: ahci_da850: add support for the da850,clk_multiplier DT property
From: David Lechner @ 2017-01-13 19:29 UTC (permalink / raw)
To: Bartosz Golaszewski, Kevin Hilman, Sekhar Nori, Patrick Titiano,
Michael Turquette, Tejun Heo, Rob Herring, Mark Rutland,
Russell King
Cc: linux-ide, linux-kernel, linux-arm-kernel, devicetree
In-Reply-To: <1484311084-31547-8-git-send-email-bgolaszewski@baylibre.com>
On 01/13/2017 06:38 AM, Bartosz Golaszewski wrote:
> Currently the clock multiplier is hardcoded in the driver for
> the da850-evm board. Make it configurable over DT, but keep the
> previous value as default in case the property is missing.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
> drivers/ata/ahci_da850.c | 83 +++++++++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 75 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/ata/ahci_da850.c b/drivers/ata/ahci_da850.c
> index bb9eb4c..cd04caf 100644
> --- a/drivers/ata/ahci_da850.c
> +++ b/drivers/ata/ahci_da850.c
> @@ -28,17 +28,70 @@
> #define SATA_PHY_TXSWING(x) ((x) << 19)
> #define SATA_PHY_ENPLL(x) ((x) << 31)
>
> +struct da850_sata_mpy_mapping {
> + unsigned int multiplier;
> + unsigned int regval;
> +};
> +
> +static const struct da850_sata_mpy_mapping da850_sata_mpy_table[] = {
> + {
> + .multiplier = 5,
> + .regval = 0x01,
> + },
> + {
> + .multiplier = 6,
> + .regval = 0x02,
> + },
> + {
> + .multiplier = 8,
> + .regval = 0x04,
> + },
> + {
> + .multiplier = 10,
> + .regval = 0x05,
> + },
> + {
> + .multiplier = 12,
> + .regval = 0x06,
> + },
> + /* TODO Add 12.5 multiplier. */
Looks like you should be using an enum here for the multiplier field.
> + {
> + .multiplier = 15,
> + .regval = 0x08,
> + },
> + {
> + .multiplier = 20,
> + .regval = 0x09,
> + },
> + {
> + .multiplier = 25,
> + .regval = 0x0a,
> + }
> +};
> +
> +static const struct da850_sata_mpy_mapping *
> +da850_sata_get_mpy(unsigned int multiplier)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(da850_sata_mpy_table); i++)
> + if (da850_sata_mpy_table[i].multiplier == multiplier)
> + return &da850_sata_mpy_table[i];
> +
> + return NULL;
> +}
> +
> /*
> * The multiplier needed for 1.5GHz PLL output.
> *
> - * NOTE: This is currently hardcoded to be suitable for 100MHz crystal
> - * frequency (which is used by DA850 EVM board) and may need to be changed
> - * if you would like to use this driver on some other board.
> + * This is the default value suitable for the 100MHz crystal frequency
> + * used by DA850 EVM board, which doesn't use DT.
As I said in a reply on another patch, it seems like it would be better
to use a clock that represents the crystal and use clk_get_rate() and
calculate the multiplier from that.
For example, we have done something like this in
usb20_phy_clk_set_parent() in arch/arm/mach-davinci/usb-da8xx.c.
> */
> -#define DA850_SATA_CLK_MULTIPLIER 7
> +#define DA850_SATA_CLK_MULTIPLIER_DEFAULT 15
>
> static void da850_sata_init(struct device *dev, void __iomem *pwrdn_reg,
> - void __iomem *ahci_base)
> + void __iomem *ahci_base,
> + const struct da850_sata_mpy_mapping *mpy)
> {
> unsigned int val;
>
> @@ -47,7 +100,7 @@ static void da850_sata_init(struct device *dev, void __iomem *pwrdn_reg,
> val &= ~BIT(0);
> writel(val, pwrdn_reg);
>
> - val = SATA_PHY_MPY(DA850_SATA_CLK_MULTIPLIER + 1) | SATA_PHY_LOS(1) |
> + val = SATA_PHY_MPY(mpy->regval) | SATA_PHY_LOS(1) |
> SATA_PHY_RXCDR(4) | SATA_PHY_RXEQ(1) | SATA_PHY_TXSWING(3) |
> SATA_PHY_ENPLL(1);
>
> @@ -87,10 +140,12 @@ static struct scsi_host_template ahci_platform_sht = {
>
> static int ahci_da850_probe(struct platform_device *pdev)
> {
> + const struct da850_sata_mpy_mapping *mpy;
> struct device *dev = &pdev->dev;
> struct ahci_host_priv *hpriv;
> - struct resource *res;
> + unsigned int multiplier;
> void __iomem *pwrdn_reg;
> + struct resource *res;
> int rc;
>
> hpriv = ahci_platform_get_resources(pdev);
> @@ -109,7 +164,19 @@ static int ahci_da850_probe(struct platform_device *pdev)
> if (!pwrdn_reg)
> goto disable_resources;
>
> - da850_sata_init(dev, pwrdn_reg, hpriv->mmio);
> + rc = of_property_read_u32(dev->of_node,
> + "da850,clk_multiplier", &multiplier);
> + if (rc)
> + multiplier = DA850_SATA_CLK_MULTIPLIER_DEFAULT;
> +
> + mpy = da850_sata_get_mpy(multiplier);
> + if (!mpy) {
> + dev_err(dev, "invalid multiplier value: %u\n", multiplier);
> + rc = -EINVAL;
> + goto disable_resources;
> + }
> +
> + da850_sata_init(dev, pwrdn_reg, hpriv->mmio, mpy);
>
> rc = ahci_platform_init_host(pdev, hpriv, &ahci_da850_port_info,
> &ahci_platform_sht);
>
^ permalink raw reply
* Re: [PATCH 03/10] devicetree: bindings: add bindings for ahci-da850
From: David Lechner @ 2017-01-13 19:25 UTC (permalink / raw)
To: Bartosz Golaszewski, Kevin Hilman, Sekhar Nori, Patrick Titiano,
Michael Turquette, Tejun Heo, Rob Herring, Mark Rutland,
Russell King
Cc: linux-ide-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <1484311084-31547-4-git-send-email-bgolaszewski-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
On 01/13/2017 06:37 AM, Bartosz Golaszewski wrote:
> Add DT bindings for the TI DA850 AHCI SATA controller.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
> ---
> .../devicetree/bindings/ata/ahci-da850.txt | 21 +++++++++++++++++++++
> 1 file changed, 21 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/ata/ahci-da850.txt
>
> diff --git a/Documentation/devicetree/bindings/ata/ahci-da850.txt b/Documentation/devicetree/bindings/ata/ahci-da850.txt
> new file mode 100644
> index 0000000..d07c241
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/ata/ahci-da850.txt
> @@ -0,0 +1,21 @@
> +Device tree binding for the TI DA850 AHCI SATA Controller
> +---------------------------------------------------------
> +
> +Required properties:
> + - compatible: must be "ti,da850-ahci"
> + - reg: physical base addresses and sizes of the controller's register areas
> + - interrupts: interrupt specifier (refer to the interrupt binding)
> +
> +Optional properties:
> + - clocks: clock specifier (refer to the common clock binding)
> + - da850,clk_multiplier: the multiplier for the reference clock needed
> + for 1.5GHz PLL output
A clock multiplier property seems redundant if you are specifying a
clock. It should be possible to get the rate from the clock to determine
which multiplier is needed.
> +
> +Example:
> +
> + sata: ahci@0x218000 {
> + compatible = "ti,da850-ahci";
> + reg = <0x218000 0x2000>, <0x22c018 0x4>;
> + interrupts = <67>;
> + da850,clk_multiplier = <7>;
> + };
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 00/10] ARM: da850-lcdk: add SATA support
From: Sekhar Nori @ 2017-01-13 14:32 UTC (permalink / raw)
To: Bartosz Golaszewski, Kevin Hilman, Patrick Titiano,
Michael Turquette, Tejun Heo, Rob Herring, Mark Rutland,
Russell King, David Lechner
Cc: linux-ide, linux-kernel, linux-arm-kernel, devicetree
In-Reply-To: <1484311084-31547-1-git-send-email-bgolaszewski@baylibre.com>
On Friday 13 January 2017 06:07 PM, Bartosz Golaszewski wrote:
> This series contains all the changes necessary to make SATA work on
> the da850-lcdk board.
>
> The first patch adds a clock lookup entry required for the ahci core
> to retrieve a functional clock.
>
> The second enables relevant config options for all davinci boards.
>
> The third adds device tree bindings for the ahci_da850 driver.
>
> The fourth adds a workaround for a SATA controller instability we
> detected after increasing the PLL0 frequency for proper LCD
> controller support.
>
> Patches 5 through 7 extend the ahci_da850 driver - add DT support,
> un-hardcode the clock multiplier value and add a workaround for
> a quirk present on the da850 SATA controller.
>
> Patches 8-10 add the device tree changes required to probe the driver.
>
> I'm posting the series as a whole to give all reviewers the full
> picture and visibility of the changes required, if needed I can resend
> the patches separately.
I just tested this series on my LCDK board using a Western Digital SATA
HDD and it works great with some basic read / write tests. Thanks!
For the non-platform patches which I wont be queuing:
Tested-by: Sekhar Nori <nsekhar@ti.com>
I will take a look at the series closely next week.
Thanks,
Sekhar
^ permalink raw reply
* [PATCH 08/10] ARM: dts: da850: add pinmux settings for the SATA controller
From: Bartosz Golaszewski @ 2017-01-13 12:38 UTC (permalink / raw)
To: Kevin Hilman, Sekhar Nori, Patrick Titiano, Michael Turquette,
Tejun Heo, Rob Herring, Mark Rutland, Russell King, David Lechner
Cc: linux-ide, devicetree, linux-kernel, linux-arm-kernel,
Bartosz Golaszewski
In-Reply-To: <1484311084-31547-1-git-send-email-bgolaszewski@baylibre.com>
Add pinmux sub-nodes for all muxed SATA pins.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
arch/arm/boot/dts/da850.dtsi | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)
diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
index 104155d..1f6a47d 100644
--- a/arch/arm/boot/dts/da850.dtsi
+++ b/arch/arm/boot/dts/da850.dtsi
@@ -78,6 +78,30 @@
0x10 0x00220000 0x00ff0000
>;
};
+ sata_cp_det_pin: pinmux_sata_cp_det_pin {
+ pinctrl-single,bits = <
+ /* SATA_CP_DET */
+ 0x0c 0x00000000 0xf0000000
+ >;
+ };
+ sata_mp_switch_pin: pinmux_sata_mp_switch_pin {
+ pinctrl-single,bits = <
+ /* SATA_MP_SWITCH */
+ 0x0c 0x00000000 0x0f000000
+ >;
+ };
+ sata_cp_pod_pin: pinmux_sata_cp_pod_pin {
+ pinctrl-single,bits = <
+ /* SATA_CP_POD */
+ 0x10 0x40000000 0xf0000000
+ >;
+ };
+ sata_led_pin: pinmux_sata_led_pin {
+ pinctrl-single,bits = <
+ /* SATA_LED */
+ 0x10 0x04000000 0x0f000000
+ >;
+ };
i2c0_pins: pinmux_i2c0_pins {
pinctrl-single,bits = <
/* I2C0_SDA,I2C0_SCL */
--
2.9.3
^ permalink raw reply related
* [PATCH 10/10] ARM: dts: da850-lcdk: enable the SATA node
From: Bartosz Golaszewski @ 2017-01-13 12:38 UTC (permalink / raw)
To: Kevin Hilman, Sekhar Nori, Patrick Titiano, Michael Turquette,
Tejun Heo, Rob Herring, Mark Rutland, Russell King, David Lechner
Cc: linux-ide-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
Bartosz Golaszewski
In-Reply-To: <1484311084-31547-1-git-send-email-bgolaszewski-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
Enable the SATA node for da850-lcdk. We omit the pinctrl property on
purpose - the muxed SATA pins are not hooked up to anything
SATA-related on the lcdk.
The REFCLKN/P rate on the board is 100MHz, so we need a multiplier of
15 for 1.5GHz PLL rate.
Signed-off-by: Bartosz Golaszewski <bgolaszewski-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
---
arch/arm/boot/dts/da850-lcdk.dts | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/arch/arm/boot/dts/da850-lcdk.dts b/arch/arm/boot/dts/da850-lcdk.dts
index afcb482..1e638da 100644
--- a/arch/arm/boot/dts/da850-lcdk.dts
+++ b/arch/arm/boot/dts/da850-lcdk.dts
@@ -105,6 +105,11 @@
status = "okay";
};
+&sata {
+ status = "okay";
+ da850,clk_multiplier = <15>;
+};
+
&mdio {
pinctrl-names = "default";
pinctrl-0 = <&mdio_pins>;
--
2.9.3
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH 09/10] ARM: dts: da850: add the SATA node
From: Bartosz Golaszewski @ 2017-01-13 12:38 UTC (permalink / raw)
To: Kevin Hilman, Sekhar Nori, Patrick Titiano, Michael Turquette,
Tejun Heo, Rob Herring, Mark Rutland, Russell King, David Lechner
Cc: linux-ide, devicetree, linux-kernel, linux-arm-kernel,
Bartosz Golaszewski
In-Reply-To: <1484311084-31547-1-git-send-email-bgolaszewski@baylibre.com>
Add the SATA node to the da850 device tree.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
arch/arm/boot/dts/da850.dtsi | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
index 1f6a47d..f5086b1 100644
--- a/arch/arm/boot/dts/da850.dtsi
+++ b/arch/arm/boot/dts/da850.dtsi
@@ -427,6 +427,12 @@
phy-names = "usb-phy";
status = "disabled";
};
+ sata: ahci@0x218000 {
+ compatible = "ti,da850-ahci";
+ reg = <0x218000 0x2000>, <0x22c018 0x4>;
+ interrupts = <67>;
+ status = "disabled";
+ };
mdio: mdio@224000 {
compatible = "ti,davinci_mdio";
#address-cells = <1>;
--
2.9.3
^ permalink raw reply related
* [PATCH 07/10] sata: ahci_da850: add support for the da850, clk_multiplier DT property
From: Bartosz Golaszewski @ 2017-01-13 12:38 UTC (permalink / raw)
To: Kevin Hilman, Sekhar Nori, Patrick Titiano, Michael Turquette,
Tejun Heo, Rob Herring, Mark Rutland, Russell King, David Lechner
Cc: linux-ide, Bartosz Golaszewski, linux-kernel, linux-arm-kernel,
devicetree
In-Reply-To: <1484311084-31547-1-git-send-email-bgolaszewski@baylibre.com>
Currently the clock multiplier is hardcoded in the driver for
the da850-evm board. Make it configurable over DT, but keep the
previous value as default in case the property is missing.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
drivers/ata/ahci_da850.c | 83 +++++++++++++++++++++++++++++++++++++++++++-----
1 file changed, 75 insertions(+), 8 deletions(-)
diff --git a/drivers/ata/ahci_da850.c b/drivers/ata/ahci_da850.c
index bb9eb4c..cd04caf 100644
--- a/drivers/ata/ahci_da850.c
+++ b/drivers/ata/ahci_da850.c
@@ -28,17 +28,70 @@
#define SATA_PHY_TXSWING(x) ((x) << 19)
#define SATA_PHY_ENPLL(x) ((x) << 31)
+struct da850_sata_mpy_mapping {
+ unsigned int multiplier;
+ unsigned int regval;
+};
+
+static const struct da850_sata_mpy_mapping da850_sata_mpy_table[] = {
+ {
+ .multiplier = 5,
+ .regval = 0x01,
+ },
+ {
+ .multiplier = 6,
+ .regval = 0x02,
+ },
+ {
+ .multiplier = 8,
+ .regval = 0x04,
+ },
+ {
+ .multiplier = 10,
+ .regval = 0x05,
+ },
+ {
+ .multiplier = 12,
+ .regval = 0x06,
+ },
+ /* TODO Add 12.5 multiplier. */
+ {
+ .multiplier = 15,
+ .regval = 0x08,
+ },
+ {
+ .multiplier = 20,
+ .regval = 0x09,
+ },
+ {
+ .multiplier = 25,
+ .regval = 0x0a,
+ }
+};
+
+static const struct da850_sata_mpy_mapping *
+da850_sata_get_mpy(unsigned int multiplier)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(da850_sata_mpy_table); i++)
+ if (da850_sata_mpy_table[i].multiplier == multiplier)
+ return &da850_sata_mpy_table[i];
+
+ return NULL;
+}
+
/*
* The multiplier needed for 1.5GHz PLL output.
*
- * NOTE: This is currently hardcoded to be suitable for 100MHz crystal
- * frequency (which is used by DA850 EVM board) and may need to be changed
- * if you would like to use this driver on some other board.
+ * This is the default value suitable for the 100MHz crystal frequency
+ * used by DA850 EVM board, which doesn't use DT.
*/
-#define DA850_SATA_CLK_MULTIPLIER 7
+#define DA850_SATA_CLK_MULTIPLIER_DEFAULT 15
static void da850_sata_init(struct device *dev, void __iomem *pwrdn_reg,
- void __iomem *ahci_base)
+ void __iomem *ahci_base,
+ const struct da850_sata_mpy_mapping *mpy)
{
unsigned int val;
@@ -47,7 +100,7 @@ static void da850_sata_init(struct device *dev, void __iomem *pwrdn_reg,
val &= ~BIT(0);
writel(val, pwrdn_reg);
- val = SATA_PHY_MPY(DA850_SATA_CLK_MULTIPLIER + 1) | SATA_PHY_LOS(1) |
+ val = SATA_PHY_MPY(mpy->regval) | SATA_PHY_LOS(1) |
SATA_PHY_RXCDR(4) | SATA_PHY_RXEQ(1) | SATA_PHY_TXSWING(3) |
SATA_PHY_ENPLL(1);
@@ -87,10 +140,12 @@ static struct scsi_host_template ahci_platform_sht = {
static int ahci_da850_probe(struct platform_device *pdev)
{
+ const struct da850_sata_mpy_mapping *mpy;
struct device *dev = &pdev->dev;
struct ahci_host_priv *hpriv;
- struct resource *res;
+ unsigned int multiplier;
void __iomem *pwrdn_reg;
+ struct resource *res;
int rc;
hpriv = ahci_platform_get_resources(pdev);
@@ -109,7 +164,19 @@ static int ahci_da850_probe(struct platform_device *pdev)
if (!pwrdn_reg)
goto disable_resources;
- da850_sata_init(dev, pwrdn_reg, hpriv->mmio);
+ rc = of_property_read_u32(dev->of_node,
+ "da850,clk_multiplier", &multiplier);
+ if (rc)
+ multiplier = DA850_SATA_CLK_MULTIPLIER_DEFAULT;
+
+ mpy = da850_sata_get_mpy(multiplier);
+ if (!mpy) {
+ dev_err(dev, "invalid multiplier value: %u\n", multiplier);
+ rc = -EINVAL;
+ goto disable_resources;
+ }
+
+ da850_sata_init(dev, pwrdn_reg, hpriv->mmio, mpy);
rc = ahci_platform_init_host(pdev, hpriv, &ahci_da850_port_info,
&ahci_platform_sht);
--
2.9.3
^ permalink raw reply related
* [PATCH 06/10] sata: ahci_da850: implement a softreset quirk
From: Bartosz Golaszewski @ 2017-01-13 12:38 UTC (permalink / raw)
To: Kevin Hilman, Sekhar Nori, Patrick Titiano, Michael Turquette,
Tejun Heo, Rob Herring, Mark Rutland, Russell King, David Lechner
Cc: linux-ide, Bartosz Golaszewski, linux-kernel, linux-arm-kernel,
devicetree
In-Reply-To: <1484311084-31547-1-git-send-email-bgolaszewski@baylibre.com>
There's an issue with the da850 SATA controller: if port multiplier
support is compiled in, but we're connecting the drive directly to
the SATA port on the board, the drive can't be detected.
To make SATA work on the da850-lcdk board: first try to softreset
with pmp - if the operation fails with -EBUSY, retry without pmp.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
drivers/ata/ahci_da850.c | 22 +++++++++++++++++++++-
1 file changed, 21 insertions(+), 1 deletion(-)
diff --git a/drivers/ata/ahci_da850.c b/drivers/ata/ahci_da850.c
index 5930af81..bb9eb4c 100644
--- a/drivers/ata/ahci_da850.c
+++ b/drivers/ata/ahci_da850.c
@@ -54,11 +54,31 @@ static void da850_sata_init(struct device *dev, void __iomem *pwrdn_reg,
writel(val, ahci_base + SATA_P0PHYCR_REG);
}
+static int ahci_da850_softreset(struct ata_link *link,
+ unsigned int *class, unsigned long deadline)
+{
+ int pmp, ret;
+
+ pmp = sata_srst_pmp(link);
+
+ ret = ahci_do_softreset(link, class, pmp, deadline, ahci_check_ready);
+ if (pmp && ret == -EBUSY)
+ return ahci_do_softreset(link, class, 0,
+ deadline, ahci_check_ready);
+
+ return ret;
+}
+
+static struct ata_port_operations ahci_da850_port_ops = {
+ .inherits = &ahci_platform_ops,
+ .softreset = ahci_da850_softreset,
+};
+
static const struct ata_port_info ahci_da850_port_info = {
.flags = AHCI_FLAG_COMMON,
.pio_mask = ATA_PIO4,
.udma_mask = ATA_UDMA6,
- .port_ops = &ahci_platform_ops,
+ .port_ops = &ahci_da850_port_ops,
};
static struct scsi_host_template ahci_platform_sht = {
--
2.9.3
^ permalink raw reply related
* [PATCH 05/10] sata: ahci_da850: add device tree match table
From: Bartosz Golaszewski @ 2017-01-13 12:37 UTC (permalink / raw)
To: Kevin Hilman, Sekhar Nori, Patrick Titiano, Michael Turquette,
Tejun Heo, Rob Herring, Mark Rutland, Russell King, David Lechner
Cc: linux-ide, Bartosz Golaszewski, linux-kernel, linux-arm-kernel,
devicetree
In-Reply-To: <1484311084-31547-1-git-send-email-bgolaszewski@baylibre.com>
We're using device tree for da850-lcdk. Add the match table to allow
to probe the driver.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
drivers/ata/ahci_da850.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/ata/ahci_da850.c b/drivers/ata/ahci_da850.c
index 267a3d3..5930af81 100644
--- a/drivers/ata/ahci_da850.c
+++ b/drivers/ata/ahci_da850.c
@@ -105,11 +105,18 @@ static int ahci_da850_probe(struct platform_device *pdev)
static SIMPLE_DEV_PM_OPS(ahci_da850_pm_ops, ahci_platform_suspend,
ahci_platform_resume);
+static const struct of_device_id ahci_da850_of_match[] = {
+ { .compatible = "ti,da850-ahci", },
+ { },
+};
+MODULE_DEVICE_TABLE(of, ahci_da850_of_match);
+
static struct platform_driver ahci_da850_driver = {
.probe = ahci_da850_probe,
.remove = ata_platform_remove_one,
.driver = {
.name = DRV_NAME,
+ .of_match_table = ahci_da850_of_match,
.pm = &ahci_da850_pm_ops,
},
};
--
2.9.3
^ permalink raw reply related
* [PATCH 04/10] sata: hardreset: retry if phys link is down
From: Bartosz Golaszewski @ 2017-01-13 12:37 UTC (permalink / raw)
To: Kevin Hilman, Sekhar Nori, Patrick Titiano, Michael Turquette,
Tejun Heo, Rob Herring, Mark Rutland, Russell King, David Lechner
Cc: linux-ide, Bartosz Golaszewski, linux-kernel, linux-arm-kernel,
devicetree
In-Reply-To: <1484311084-31547-1-git-send-email-bgolaszewski@baylibre.com>
The sata core driver already retries to resume the link because some
controllers ignore writes to the SControl register.
We have a use case with the da850 SATA controller where at PLL0
frequency of 456MHz (needed to properly service the LCD controller)
the chip becomes unstable and the hardreset operation is ignored the
first time 50% of times.
Retrying just the resume operation doesn't work - we need to issue
the phy/wake reset again to make it work.
If ata_phys_link_offline() returns true in sata_link_hardreset(),
retry a couple times before really giving up.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
drivers/ata/libata-core.c | 16 ++++++++++++----
include/linux/libata.h | 4 +++-
2 files changed, 15 insertions(+), 5 deletions(-)
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 9cd0a2d..3b848a3 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -3985,8 +3985,8 @@ int sata_link_hardreset(struct ata_link *link, const unsigned long *timing,
unsigned long deadline,
bool *online, int (*check_ready)(struct ata_link *))
{
+ int rc, retry = ATA_LINK_RESET_TRIES;
u32 scontrol;
- int rc;
DPRINTK("ENTER\n");
@@ -4009,7 +4009,7 @@ int sata_link_hardreset(struct ata_link *link, const unsigned long *timing,
sata_set_spd(link);
}
-
+retry:
/* issue phy wake/reset */
if ((rc = sata_scr_read(link, SCR_CONTROL, &scontrol)))
goto out;
@@ -4028,9 +4028,17 @@ int sata_link_hardreset(struct ata_link *link, const unsigned long *timing,
rc = sata_link_resume(link, timing, deadline);
if (rc)
goto out;
- /* if link is offline nothing more to do */
- if (ata_phys_link_offline(link))
+
+ if (ata_phys_link_offline(link)) {
+ if (retry--) {
+ ata_link_warn(link,
+ "link still offline after hardreset - retrying\n");
+ goto retry;
+ }
+
+ /* if link is still offline nothing more to do */
goto out;
+ }
/* Link is online. From this point, -ENODEV too is an error. */
if (online)
diff --git a/include/linux/libata.h b/include/linux/libata.h
index c170be5..2c840c0 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -392,8 +392,10 @@ enum {
/* max tries if error condition is still set after ->error_handler */
ATA_EH_MAX_TRIES = 5,
- /* sometimes resuming a link requires several retries */
+ /* sometimes resuming a link requires several retries... */
ATA_LINK_RESUME_TRIES = 5,
+ /* ... and sometimes we need to retry the whole reset procedure */
+ ATA_LINK_RESET_TRIES = 5,
/* how hard are we gonna try to probe/recover devices */
ATA_PROBE_MAX_TRIES = 3,
--
2.9.3
^ permalink raw reply related
* [PATCH 03/10] devicetree: bindings: add bindings for ahci-da850
From: Bartosz Golaszewski @ 2017-01-13 12:37 UTC (permalink / raw)
To: Kevin Hilman, Sekhar Nori, Patrick Titiano, Michael Turquette,
Tejun Heo, Rob Herring, Mark Rutland, Russell King, David Lechner
Cc: linux-ide, Bartosz Golaszewski, linux-kernel, linux-arm-kernel,
devicetree
In-Reply-To: <1484311084-31547-1-git-send-email-bgolaszewski@baylibre.com>
Add DT bindings for the TI DA850 AHCI SATA controller.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
.../devicetree/bindings/ata/ahci-da850.txt | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
create mode 100644 Documentation/devicetree/bindings/ata/ahci-da850.txt
diff --git a/Documentation/devicetree/bindings/ata/ahci-da850.txt b/Documentation/devicetree/bindings/ata/ahci-da850.txt
new file mode 100644
index 0000000..d07c241
--- /dev/null
+++ b/Documentation/devicetree/bindings/ata/ahci-da850.txt
@@ -0,0 +1,21 @@
+Device tree binding for the TI DA850 AHCI SATA Controller
+---------------------------------------------------------
+
+Required properties:
+ - compatible: must be "ti,da850-ahci"
+ - reg: physical base addresses and sizes of the controller's register areas
+ - interrupts: interrupt specifier (refer to the interrupt binding)
+
+Optional properties:
+ - clocks: clock specifier (refer to the common clock binding)
+ - da850,clk_multiplier: the multiplier for the reference clock needed
+ for 1.5GHz PLL output
+
+Example:
+
+ sata: ahci@0x218000 {
+ compatible = "ti,da850-ahci";
+ reg = <0x218000 0x2000>, <0x22c018 0x4>;
+ interrupts = <67>;
+ da850,clk_multiplier = <7>;
+ };
--
2.9.3
^ permalink raw reply related
* [PATCH 02/10] ARM: davinci_all_defconfig: enable SATA modules
From: Bartosz Golaszewski @ 2017-01-13 12:37 UTC (permalink / raw)
To: Kevin Hilman, Sekhar Nori, Patrick Titiano, Michael Turquette,
Tejun Heo, Rob Herring, Mark Rutland, Russell King, David Lechner
Cc: linux-ide, Bartosz Golaszewski, linux-kernel, linux-arm-kernel,
devicetree
In-Reply-To: <1484311084-31547-1-git-send-email-bgolaszewski@baylibre.com>
Add the da850-ahci driver to davinci defconfig.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
arch/arm/configs/davinci_all_defconfig | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/arm/configs/davinci_all_defconfig b/arch/arm/configs/davinci_all_defconfig
index 8806754..a1b9c58 100644
--- a/arch/arm/configs/davinci_all_defconfig
+++ b/arch/arm/configs/davinci_all_defconfig
@@ -78,6 +78,8 @@ CONFIG_IDE=m
CONFIG_BLK_DEV_PALMCHIP_BK3710=m
CONFIG_SCSI=m
CONFIG_BLK_DEV_SD=m
+CONFIG_ATA=m
+CONFIG_AHCI_DA850=m
CONFIG_NETDEVICES=y
CONFIG_NETCONSOLE=y
CONFIG_TUN=m
--
2.9.3
^ permalink raw reply related
* [PATCH 01/10] ARM: davinci: add a clock lookup entry for the SATA clock
From: Bartosz Golaszewski @ 2017-01-13 12:37 UTC (permalink / raw)
To: Kevin Hilman, Sekhar Nori, Patrick Titiano, Michael Turquette,
Tejun Heo, Rob Herring, Mark Rutland, Russell King, David Lechner
Cc: linux-ide, Bartosz Golaszewski, linux-kernel, linux-arm-kernel,
devicetree
In-Reply-To: <1484311084-31547-1-git-send-email-bgolaszewski@baylibre.com>
This entry is needed for the ahci driver to get a functional clock.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
arch/arm/mach-davinci/da8xx-dt.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm/mach-davinci/da8xx-dt.c b/arch/arm/mach-davinci/da8xx-dt.c
index 9ee44da..b83e5d1 100644
--- a/arch/arm/mach-davinci/da8xx-dt.c
+++ b/arch/arm/mach-davinci/da8xx-dt.c
@@ -42,6 +42,7 @@ static struct of_dev_auxdata da850_auxdata_lookup[] __initdata = {
OF_DEV_AUXDATA("ti,da830-ohci", 0x01e25000, "ohci-da8xx", NULL),
OF_DEV_AUXDATA("ti,da830-musb", 0x01e00000, "musb-da8xx", NULL),
OF_DEV_AUXDATA("ti,da830-usb-phy", 0x01c1417c, "da8xx-usb-phy", NULL),
+ OF_DEV_AUXDATA("ti,da850-ahci", 0x01e18000, "ahci_da850", NULL),
{}
};
--
2.9.3
^ permalink raw reply related
* [PATCH 00/10] ARM: da850-lcdk: add SATA support
From: Bartosz Golaszewski @ 2017-01-13 12:37 UTC (permalink / raw)
To: Kevin Hilman, Sekhar Nori, Patrick Titiano, Michael Turquette,
Tejun Heo, Rob Herring, Mark Rutland, Russell King, David Lechner
Cc: linux-ide, Bartosz Golaszewski, linux-kernel, linux-arm-kernel,
devicetree
This series contains all the changes necessary to make SATA work on
the da850-lcdk board.
The first patch adds a clock lookup entry required for the ahci core
to retrieve a functional clock.
The second enables relevant config options for all davinci boards.
The third adds device tree bindings for the ahci_da850 driver.
The fourth adds a workaround for a SATA controller instability we
detected after increasing the PLL0 frequency for proper LCD
controller support.
Patches 5 through 7 extend the ahci_da850 driver - add DT support,
un-hardcode the clock multiplier value and add a workaround for
a quirk present on the da850 SATA controller.
Patches 8-10 add the device tree changes required to probe the driver.
I'm posting the series as a whole to give all reviewers the full
picture and visibility of the changes required, if needed I can resend
the patches separately.
Bartosz Golaszewski (10):
ARM: davinci: add a clock lookup entry for the SATA clock
ARM: davinci_all_defconfig: enable SATA modules
devicetree: bindings: add bindings for ahci-da850
sata: hardreset: retry if phys link is down
sata: ahci_da850: add device tree match table
sata: ahci_da850: implement a softreset quirk
sata: ahci_da850: add support for the da850,clk_multiplier DT property
ARM: dts: da850: add pinmux settings for the SATA controller
ARM: dts: da850: add the SATA node
ARM: dts: da850-lcdk: enable the SATA node
.../devicetree/bindings/ata/ahci-da850.txt | 21 ++++
arch/arm/boot/dts/da850-lcdk.dts | 5 +
arch/arm/boot/dts/da850.dtsi | 30 ++++++
arch/arm/configs/davinci_all_defconfig | 2 +
arch/arm/mach-davinci/da8xx-dt.c | 1 +
drivers/ata/ahci_da850.c | 112 +++++++++++++++++++--
drivers/ata/libata-core.c | 16 ++-
include/linux/libata.h | 4 +-
8 files changed, 177 insertions(+), 14 deletions(-)
create mode 100644 Documentation/devicetree/bindings/ata/ahci-da850.txt
--
2.9.3
^ permalink raw reply
* Re: [PATCH 0/3] ata: add m68k/Atari Falcon PATA support
From: Finn Thain @ 2017-01-13 2:33 UTC (permalink / raw)
To: Michael Schmitz
Cc: Bartlomiej Zolnierkiewicz, Tejun Heo, Geert Uytterhoeven,
linux-ide, Linux/m68k, Linux Kernel Development, Andreas Schwab
In-Reply-To: <CAOmrzkJfQu6PuCpyyRJL8PNvSYxYO42F-k8jEYf2d1-5C28A_A@mail.gmail.com>
On Wed, 11 Jan 2017, Michael Schmitz wrote:
> What is still correct is that the IDE driver does use the interrupt
> only, not the ST-DMA chip. And a single IDE interrupt can be correctly
> assigned to IDE by looking at the status register.
>
> With the SCSI (and IIRC also floppy) interrupts, we don't have direct
> access to the status registers without disturbing the state of the DMA
> though. Unless we know for definite that either chips have raised the
> interrupt (and DMA ops are in flight), we must not touch the DMA chip at
> all.
>
> The case I'm worried about is both IDE and SCSI raising an interrupt. We
> don't currently mask the IDE/ST-DMA interrupt so a stacked interrupt
> must be processed in the same pass as the initial interrupt or it will
> get dropped. We'd have to peek at the DMA registers to check the SCSI or
> floppy interrupt status, and we just can't safely do that. So races of
> this kind are currently prevented by including IDE in the IRQ locking
> process.
>
> Whether it's possible to mask the interrupt, do one pass, unmask and
> process the second interrupt I don't know.
Would that require handling the SCSI DMA interrupt in the first pass? Or
handling IDE first, and ensuring that the IDE handler does not access
ST-DMA registers? What about FDC?
The atari_scsi handler accesses the ST-DMA registers; it can do so because
it knows that any DMA must have completed -- it can infer this because a
simultaneous pending interrupt from FDC or IDE is impossible due to
stdma_lock().
Your suggestion would seem to allow other pending interrupts, hence the
atari_scsi interrupt handler logic has to be tossed out. What logic would
replace it?
If all else fails, perhaps we could inhibit DMA entirely when the new ATA
driver is loaded. Then we can just dispatch the ST-DMA irq like a shared
irq. I'm sure that atari_scsi can work without DMA. No idea about the FDC
driver though (ataflop.c).
Another solution would be to dedicate the DMA function to atari_scsi, and
then mask the FDC and IDE interrupts during each DMA transfer. But once
again, this would mean changing the FDC driver to eliminate DMA, if that
is possible. From the schematic it looks the the FDC chip, "AJAX", is
another custom ...
http://dev-docs.atariforge.org/files/Falcon030_Schematic.pdf
Unfortunately my grasp of the ST hardware reflects my inability to read
German; those who can may want to take a look at "ATARI Profibuch
ST-STE-TT.pdf".
--
> Maybe Andreas does?
>
> Cheers,
>
> Michael
>
>
> > it should be okay to use IDE at the same time as SCSI/Floppy which is
> > what the new driver does (the old one is serialized operations by
> > ST-DMA related IRQ handling magic).
> >
> > Also the comment itself may need some fixups as on Falcon it is SCSI
> > not ACSI (according to the earlier comment in same file) and the old
> > IDE host driver name is not falhd.c but falconide.c.
> >
> > Best regards,
> > --
> > Bartlomiej Zolnierkiewicz
> > Samsung R&D Institute Poland
> > Samsung Electronics
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-m68k" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox