* Could you please let me know if you receive my last email, introducing myself to you, on a ( Business Opportunity)? I urgently wish to know if we can work together.
From: United Bank For Africa @ 2017-03-17 11:39 UTC (permalink / raw)
^ permalink raw reply
* Re: [PATCH] pata_pcmcia: add EBSA110's PCMCIA slot support
From: Tejun Heo @ 2017-03-17 13:39 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz
Cc: Sergei Shtylyov, Arnd Bergmann, Dmitry Eremin-Solenikov,
Kevin Hilman, Sekhar Nori, linux-kernel, linux-ide, Russell King,
Olof Johansson, linux-arm-kernel
In-Reply-To: <1533391.BtkrkgvqUW@amdc3058>
Hello,
On Fri, Mar 17, 2017 at 12:59:56PM +0100, Bartlomiej Zolnierkiewicz wrote:
> There is no support for this device in the upstream ide driver but
> Russell has a hacky patch to make it work by redefining inb()/outb()
> operations globally for the whole ide subsystem, please see:
>
> https://www.spinics.net/lists/arm-kernel/msg567454.html
Well, that mail says that the hardware is a museum piece. What's the
point here? This can't even be tested, can it?
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH] pata_pcmcia: add EBSA110's PCMCIA slot support
From: Bartlomiej Zolnierkiewicz @ 2017-03-17 14:08 UTC (permalink / raw)
To: Tejun Heo
Cc: Sergei Shtylyov, Arnd Bergmann, Dmitry Eremin-Solenikov,
Kevin Hilman, Sekhar Nori, linux-kernel, linux-ide, Russell King,
Olof Johansson, linux-arm-kernel
In-Reply-To: <20170317133900.GA5078@htj.duckdns.org>
On Friday, March 17, 2017 09:39:00 AM Tejun Heo wrote:
> Hello,
>
> On Fri, Mar 17, 2017 at 12:59:56PM +0100, Bartlomiej Zolnierkiewicz wrote:
> > There is no support for this device in the upstream ide driver but
> > Russell has a hacky patch to make it work by redefining inb()/outb()
> > operations globally for the whole ide subsystem, please see:
> >
> > https://www.spinics.net/lists/arm-kernel/msg567454.html
>
> Well, that mail says that the hardware is a museum piece. What's the
> point here? This can't even be tested, can it?
To be honest, the main point here is to make Russell happy. ;-)
This is the second time he brought EBSA110 issue while talking about
conversion of ARM platforms to use libata PATA so I've finally decided
to do the patch just to have a reference example of how to deal with
this and similar devices in a proper way.
Even though the patch can't be tested EBSA110 is still a supported ARM
platform and the patch should work fine in theory (+ it cannot break
anything). OTOH it adds an extra code which has some cost from the
long-term maintenance POV. Anyway, it is up to you to decide what to
do with it. :-)
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
^ permalink raw reply
* Re: [PATCH] pata_pcmcia: add EBSA110's PCMCIA slot support
From: Tejun Heo @ 2017-03-17 14:13 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz
Cc: Sergei Shtylyov, Arnd Bergmann, Dmitry Eremin-Solenikov,
Kevin Hilman, Sekhar Nori, linux-kernel, linux-ide, Russell King,
Olof Johansson, linux-arm-kernel
In-Reply-To: <11380780.Ccg1NEX8q5@amdc3058>
Hello,
On Fri, Mar 17, 2017 at 03:08:56PM +0100, Bartlomiej Zolnierkiewicz wrote:
> To be honest, the main point here is to make Russell happy. ;-)
>
> This is the second time he brought EBSA110 issue while talking about
> conversion of ARM platforms to use libata PATA so I've finally decided
> to do the patch just to have a reference example of how to deal with
> this and similar devices in a proper way.
>
> Even though the patch can't be tested EBSA110 is still a supported ARM
> platform and the patch should work fine in theory (+ it cannot break
> anything). OTOH it adds an extra code which has some cost from the
> long-term maintenance POV. Anyway, it is up to you to decide what to
> do with it. :-)
It's fairly low impact and I don't think it's gonna add noticeable
maintenance overhead at all, so that part is fine but it's super weird
to add code which we know not to have any users. It looks like we
can't even test it.
What's the end game here? Is it part of the effort to remove ide? If
so, I have no objection to that but think it isn't a pressing issue
either and the ultimate decision has to come from Dave.
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH] pata_pcmcia: add EBSA110's PCMCIA slot support
From: Bartlomiej Zolnierkiewicz @ 2017-03-17 15:51 UTC (permalink / raw)
To: Tejun Heo
Cc: Sergei Shtylyov, Arnd Bergmann, Dmitry Eremin-Solenikov,
Kevin Hilman, Sekhar Nori, linux-kernel, linux-ide, Russell King,
Olof Johansson, linux-arm-kernel
In-Reply-To: <20170317141313.GC5078@htj.duckdns.org>
On Friday, March 17, 2017 10:13:13 AM Tejun Heo wrote:
> Hello,
>
> On Fri, Mar 17, 2017 at 03:08:56PM +0100, Bartlomiej Zolnierkiewicz wrote:
> > To be honest, the main point here is to make Russell happy. ;-)
> >
> > This is the second time he brought EBSA110 issue while talking about
> > conversion of ARM platforms to use libata PATA so I've finally decided
> > to do the patch just to have a reference example of how to deal with
> > this and similar devices in a proper way.
> >
> > Even though the patch can't be tested EBSA110 is still a supported ARM
> > platform and the patch should work fine in theory (+ it cannot break
> > anything). OTOH it adds an extra code which has some cost from the
> > long-term maintenance POV. Anyway, it is up to you to decide what to
> > do with it. :-)
>
> It's fairly low impact and I don't think it's gonna add noticeable
> maintenance overhead at all, so that part is fine but it's super weird
> to add code which we know not to have any users. It looks like we
> can't even test it.
Well, this is more general problem. We keep things like ARM EBSA110
platform support in the kernel for some reason.. maybe we shouldn't?
> What's the end game here? Is it part of the effort to remove ide? If
> so, I have no objection to that but think it isn't a pressing issue
> either and the ultimate decision has to come from Dave.
I'm just helping in migrating users that are still using ide to libata,
enhancing libata's hardware support in the process. I would prefer to
have ide removed in the long-term (once there are no known users of
it) but Dave has a valid point (see below). This particular patch is
more a by-product of the above work than a part of it itself.
Dave's current opinion is that we can't prove that all configurations
currently supported by ide are supported by libata. Since it can never
be really proved (as there are ide drivers for hardware that cannot be
tested because of having no active users) he is fine with ide staying
in the kernel forever. Which may be not as bad for me personally as
I have a lot of contributions there. ;-)
Theoretically another option is to finish ide's evolution into "ide
compat" layer for SCSI/libata. This would give us full git history from
ide to libata support + thin compatibility layer. However with libata
PATA availability and hardware being obsolete there is no real need
for it and the cost to finish such transition is much higher than cost
of ide staying in the kernel forever. Moreover it would need to happen
outside of the upstream kernel and there are no guarantees that it
would be ever merged (ide is in deep maintenance policy and treated
as a "safe fallback" for libata; SCSI & libata changes would need to
be agreed on).
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
^ permalink raw reply
* Re: [PATCH v2 1/3] ata: add Palmchip BK3710 PATA controller driver
From: Sergei Shtylyov @ 2017-03-18 13:52 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz, Tejun Heo
Cc: Russell King, Arnd Bergmann, Dmitry Eremin-Solenikov,
Kevin Hilman, Sekhar Nori, linux-kernel, linux-ide,
linux-arm-kernel
In-Reply-To: <1489509414-11491-2-git-send-email-b.zolnierkie@samsung.com>
Hello!
On 3/14/2017 7:36 PM, Bartlomiej Zolnierkiewicz wrote:
> Add Palmchip BK3710 PATA controller driver.
>
> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
[...]
> diff --git a/drivers/ata/pata_bk3710.c b/drivers/ata/pata_bk3710.c
> new file mode 100644
> index 0000000..6d77217
> --- /dev/null
> +++ b/drivers/ata/pata_bk3710.c
> @@ -0,0 +1,386 @@
[...]
> +static void pata_bk3710_chipinit(void __iomem *base)
> +{
[...]
> + /*
> + * IORDYTMP IORDY Timer for Primary Register
> + * (ATA_IORDYTMP_IORDYTMP , 0xffff )
> + */
> + iowrite32(0xFFFF, base + BK3710_IORDYTMP);
As I've already said, this is useless as we don't handle the IORDY timeout
interrupt anyway; writing 0 would be fine.
> +
> + /*
> + * Configure BMISP Register
> + * (ATA_BMISP_DMAEN1 , DISABLE ) |
> + * (ATA_BMISP_DMAEN0 , DISABLE ) |
> + * (ATA_BMISP_IORDYINT , CLEAR) |
> + * (ATA_BMISP_INTRSTAT , CLEAR) |
> + * (ATA_BMISP_DMAERROR , CLEAR)
> + */
> + iowrite16(0, base + BK3710_BMISP);
Bits 0-3 cane only be cleared by writing 1, so this write can't clear
them, contrary to what the comment says. Might be a material for a follow-up
patch tho...
[...]
> +static int __init pata_bk3710_probe(struct platform_device *pdev)
> +{
> + struct clk *clk;
> + struct resource *mem;
> + struct ata_host *host;
> + struct ata_port *ap;
> + void __iomem *base;
> + unsigned long rate;
> + int irq;
> +
> + clk = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(clk))
> + return -ENODEV;
> +
> + clk_enable(clk);
> + rate = clk_get_rate(clk);
> + if (!rate)
> + return -EINVAL;
> +
> + /* NOTE: round *down* to meet minimum timings; we count in clocks */
> + ideclk_period = 1000000000UL / rate;
> +
> + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (mem == NULL) {
> + pr_err(DRV_NAME ": failed to get memory region resource\n");
> + return -ENODEV;
> + }
NULL check not needed here, devm_ioremap_resource() checks this anyway.
> +
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0) {
> + pr_err(DRV_NAME ": failed to get IRQ resource\n");
> + return irq;
> + }
> +
> + base = devm_ioremap_resource(&pdev->dev, mem);
> + if (IS_ERR(base))
> + return PTR_ERR(base);
> +
[...]
> +/* work with hotplug and coldplug */
> +MODULE_ALIAS("platform:palm_bk3710");
> +
> +static struct platform_driver pata_bk3710_driver = {
> + .driver = {
> + .name = "palm_bk3710",
Not DRV_NAME?
[...]
MBR, Sergei
^ permalink raw reply
* Re: [PATCH] pata_pcmcia: add EBSA110's PCMCIA slot support
From: Bartlomiej Zolnierkiewicz @ 2017-03-20 13:00 UTC (permalink / raw)
To: Tejun Heo
Cc: Sergei Shtylyov, Arnd Bergmann, Dmitry Eremin-Solenikov,
Kevin Hilman, Sekhar Nori, linux-kernel, linux-ide, Russell King,
Olof Johansson, David S. Miller, linux-arm-kernel
In-Reply-To: <5827966.LxfQpi93qC@amdc3058>
On Friday, March 17, 2017 04:51:13 PM Bartlomiej Zolnierkiewicz wrote:
> > What's the end game here? Is it part of the effort to remove ide? If
> > so, I have no objection to that but think it isn't a pressing issue
> > either and the ultimate decision has to come from Dave.
>
> I'm just helping in migrating users that are still using ide to libata,
> enhancing libata's hardware support in the process. I would prefer to
> have ide removed in the long-term (once there are no known users of
> it) but Dave has a valid point (see below). This particular patch is
> more a by-product of the above work than a part of it itself.
>
> Dave's current opinion is that we can't prove that all configurations
> currently supported by ide are supported by libata. Since it can never
> be really proved (as there are ide drivers for hardware that cannot be
> tested because of having no active users) he is fine with ide staying
> in the kernel forever. Which may be not as bad for me personally as
> I have a lot of contributions there. ;-)
I've given some more thought on this over the weekend.
I completely agree with Dave's opinion now. He has a good understanding
of a long-term kernel development process (his opinions on ide were right
back in 2009 and 2005).
We keep support for hardware that has no active users "just in case" until
the maintenance cost becomes too high or it blocks (in major way) some new
developments. This is not the case with ide (even with all active users
ported to libata we will still be left with some support for hardware which
can no longer be tested) and it should stay unless some of above conditions
change. My personal wish to have coherent PATA support in the kernel doesn't
really matter here.
I would apologize for being wrong on this and I'm sorry for waisting
people's time.
Coming back to the pata_pcmcia patch, Tejun, please just ignore it. It is
now in mailing list's archives and this is sufficient to reference it if
ever needed.
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
^ permalink raw reply
* support ranges TRIM for libata
From: Christoph Hellwig @ 2017-03-20 20:43 UTC (permalink / raw)
To: tj, martin.petersen, axboe; +Cc: linux-ide, linux-scsi, linux-block
This series implements rangeѕ discard for ATA SSDs. Compared to the
initial NVMe support there are two things that complicate the ATA
support:
- ATA only suports 16-bit long ranges
- the whole mess of generating a SCSI command first and then
translating it to an ATA one.
This series adds support for limited range size to the block layer,
and stops translating discard commands - instead we add a new
Vendor Specific SCSI command that contains the TRIM payload when
the device asks for it.
^ permalink raw reply
* [PATCH 1/7] ѕd: split sd_setup_discard_cmnd
From: Christoph Hellwig @ 2017-03-20 20:43 UTC (permalink / raw)
To: tj, martin.petersen, axboe; +Cc: linux-ide, linux-scsi, linux-block
In-Reply-To: <20170320204319.12628-1-hch@lst.de>
Split sd_setup_discard_cmnd into one function per provisioning type. While
this creates some very slight duplication of boilerplate code it keeps the
code modular for additions of new provisioning types, and for reusing the
write same functions for the upcoming scsi implementation of the Write Zeroes
operation.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/scsi/sd.c | 153 ++++++++++++++++++++++++++++++------------------------
1 file changed, 84 insertions(+), 69 deletions(-)
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index fcfeddc79331..b853f91fb3da 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -701,93 +701,97 @@ static void sd_config_discard(struct scsi_disk *sdkp, unsigned int mode)
queue_flag_set_unlocked(QUEUE_FLAG_DISCARD, q);
}
-/**
- * sd_setup_discard_cmnd - unmap blocks on thinly provisioned device
- * @sdp: scsi device to operate on
- * @rq: Request to prepare
- *
- * Will issue either UNMAP or WRITE SAME(16) depending on preference
- * indicated by target device.
- **/
-static int sd_setup_discard_cmnd(struct scsi_cmnd *cmd)
+static int sd_setup_unmap_cmnd(struct scsi_cmnd *cmd)
{
- struct request *rq = cmd->request;
struct scsi_device *sdp = cmd->device;
- struct scsi_disk *sdkp = scsi_disk(rq->rq_disk);
- sector_t sector = blk_rq_pos(rq);
- unsigned int nr_sectors = blk_rq_sectors(rq);
- unsigned int len;
- int ret;
+ struct request *rq = cmd->request;
+ u64 sector = blk_rq_pos(rq) >> (ilog2(sdp->sector_size) - 9);
+ u32 nr_sectors = blk_rq_sectors(rq) >> (ilog2(sdp->sector_size) - 9);
+ unsigned int data_len = 24;
char *buf;
- struct page *page;
- sector >>= ilog2(sdp->sector_size) - 9;
- nr_sectors >>= ilog2(sdp->sector_size) - 9;
-
- page = alloc_page(GFP_ATOMIC | __GFP_ZERO);
- if (!page)
+ rq->special_vec.bv_page = alloc_page(GFP_ATOMIC | __GFP_ZERO);
+ if (!rq->special_vec.bv_page)
return BLKPREP_DEFER;
+ rq->special_vec.bv_offset = 0;
+ rq->special_vec.bv_len = data_len;
+ rq->rq_flags |= RQF_SPECIAL_PAYLOAD;
- switch (sdkp->provisioning_mode) {
- case SD_LBP_UNMAP:
- buf = page_address(page);
-
- cmd->cmd_len = 10;
- cmd->cmnd[0] = UNMAP;
- cmd->cmnd[8] = 24;
-
- put_unaligned_be16(6 + 16, &buf[0]);
- put_unaligned_be16(16, &buf[2]);
- put_unaligned_be64(sector, &buf[8]);
- put_unaligned_be32(nr_sectors, &buf[16]);
+ cmd->cmd_len = 10;
+ cmd->cmnd[0] = UNMAP;
+ cmd->cmnd[8] = 24;
- len = 24;
- break;
+ buf = page_address(rq->special_vec.bv_page);
+ put_unaligned_be16(6 + 16, &buf[0]);
+ put_unaligned_be16(16, &buf[2]);
+ put_unaligned_be64(sector, &buf[8]);
+ put_unaligned_be32(nr_sectors, &buf[16]);
- case SD_LBP_WS16:
- cmd->cmd_len = 16;
- cmd->cmnd[0] = WRITE_SAME_16;
- cmd->cmnd[1] = 0x8; /* UNMAP */
- put_unaligned_be64(sector, &cmd->cmnd[2]);
- put_unaligned_be32(nr_sectors, &cmd->cmnd[10]);
+ cmd->allowed = SD_MAX_RETRIES;
+ cmd->transfersize = data_len;
+ rq->timeout = SD_TIMEOUT;
+ scsi_req(rq)->resid_len = data_len;
- len = sdkp->device->sector_size;
- break;
+ return scsi_init_io(cmd);
+}
- case SD_LBP_WS10:
- case SD_LBP_ZERO:
- cmd->cmd_len = 10;
- cmd->cmnd[0] = WRITE_SAME;
- if (sdkp->provisioning_mode == SD_LBP_WS10)
- cmd->cmnd[1] = 0x8; /* UNMAP */
- put_unaligned_be32(sector, &cmd->cmnd[2]);
- put_unaligned_be16(nr_sectors, &cmd->cmnd[7]);
+static int sd_setup_write_same16_cmnd(struct scsi_cmnd *cmd)
+{
+ struct scsi_device *sdp = cmd->device;
+ struct request *rq = cmd->request;
+ u64 sector = blk_rq_pos(rq) >> (ilog2(sdp->sector_size) - 9);
+ u32 nr_sectors = blk_rq_sectors(rq) >> (ilog2(sdp->sector_size) - 9);
+ u32 data_len = sdp->sector_size;
- len = sdkp->device->sector_size;
- break;
+ rq->special_vec.bv_page = alloc_page(GFP_ATOMIC | __GFP_ZERO);
+ if (!rq->special_vec.bv_page)
+ return BLKPREP_DEFER;
+ rq->special_vec.bv_offset = 0;
+ rq->special_vec.bv_len = data_len;
+ rq->rq_flags |= RQF_SPECIAL_PAYLOAD;
- default:
- ret = BLKPREP_INVALID;
- goto out;
- }
+ cmd->cmd_len = 16;
+ cmd->cmnd[0] = WRITE_SAME_16;
+ cmd->cmnd[1] = 0x8; /* UNMAP */
+ put_unaligned_be64(sector, &cmd->cmnd[2]);
+ put_unaligned_be32(nr_sectors, &cmd->cmnd[10]);
+ cmd->allowed = SD_MAX_RETRIES;
+ cmd->transfersize = data_len;
rq->timeout = SD_TIMEOUT;
+ scsi_req(rq)->resid_len = data_len;
- cmd->transfersize = len;
- cmd->allowed = SD_MAX_RETRIES;
+ return scsi_init_io(cmd);
+}
- rq->special_vec.bv_page = page;
- rq->special_vec.bv_offset = 0;
- rq->special_vec.bv_len = len;
+static int sd_setup_write_same10_cmnd(struct scsi_cmnd *cmd, bool unmap)
+{
+ struct scsi_device *sdp = cmd->device;
+ struct request *rq = cmd->request;
+ u64 sector = blk_rq_pos(rq) >> (ilog2(sdp->sector_size) - 9);
+ u32 nr_sectors = blk_rq_sectors(rq) >> (ilog2(sdp->sector_size) - 9);
+ u32 data_len = sdp->sector_size;
+ rq->special_vec.bv_page = alloc_page(GFP_ATOMIC | __GFP_ZERO);
+ if (!rq->special_vec.bv_page)
+ return BLKPREP_DEFER;
+ rq->special_vec.bv_offset = 0;
+ rq->special_vec.bv_len = data_len;
rq->rq_flags |= RQF_SPECIAL_PAYLOAD;
- scsi_req(rq)->resid_len = len;
- ret = scsi_init_io(cmd);
-out:
- if (ret != BLKPREP_OK)
- __free_page(page);
- return ret;
+ cmd->cmd_len = 10;
+ cmd->cmnd[0] = WRITE_SAME;
+ if (unmap)
+ cmd->cmnd[1] = 0x8; /* UNMAP */
+ put_unaligned_be32(sector, &cmd->cmnd[2]);
+ put_unaligned_be16(nr_sectors, &cmd->cmnd[7]);
+
+ cmd->allowed = SD_MAX_RETRIES;
+ cmd->transfersize = data_len;
+ rq->timeout = SD_TIMEOUT;
+ scsi_req(rq)->resid_len = data_len;
+
+ return scsi_init_io(cmd);
}
static void sd_config_write_same(struct scsi_disk *sdkp)
@@ -1155,7 +1159,18 @@ static int sd_init_command(struct scsi_cmnd *cmd)
switch (req_op(rq)) {
case REQ_OP_DISCARD:
- return sd_setup_discard_cmnd(cmd);
+ switch (scsi_disk(rq->rq_disk)->provisioning_mode) {
+ case SD_LBP_UNMAP:
+ return sd_setup_unmap_cmnd(cmd);
+ case SD_LBP_WS16:
+ return sd_setup_write_same16_cmnd(cmd);
+ case SD_LBP_WS10:
+ return sd_setup_write_same10_cmnd(cmd, true);
+ case SD_LBP_ZERO:
+ return sd_setup_write_same10_cmnd(cmd, false);
+ default:
+ return BLKPREP_INVALID;
+ }
case REQ_OP_WRITE_SAME:
return sd_setup_write_same_cmnd(cmd);
case REQ_OP_FLUSH:
--
2.11.0
^ permalink raw reply related
* [PATCH 2/7] sd: provide a new ata trim provisioning mode
From: Christoph Hellwig @ 2017-03-20 20:43 UTC (permalink / raw)
To: tj, martin.petersen, axboe; +Cc: linux-ide, linux-scsi, linux-block
In-Reply-To: <20170320204319.12628-1-hch@lst.de>
This uses a vendor specific command to pass the ATA TRIM payload to
libata without having to rewrite it in place. Support for it is
indicated by a new flag in struct scsi_device that libata will set
in it's slave_configure routine. A second flag indicates if TRIM
will reliably zero data.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/scsi/sd.c | 60 +++++++++++++++++++++++++++++++++++++++++++---
drivers/scsi/sd.h | 1 +
include/scsi/scsi_device.h | 2 ++
include/scsi/scsi_proto.h | 3 +++
4 files changed, 63 insertions(+), 3 deletions(-)
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index b853f91fb3da..cc8684818305 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -371,6 +371,7 @@ static const char *lbp_mode[] = {
[SD_LBP_WS16] = "writesame_16",
[SD_LBP_WS10] = "writesame_10",
[SD_LBP_ZERO] = "writesame_zero",
+ [SD_LBP_ATA_TRIM] = "ata_trim",
[SD_LBP_DISABLE] = "disabled",
};
@@ -411,7 +412,7 @@ provisioning_mode_store(struct device *dev, struct device_attribute *attr,
sd_config_discard(sdkp, SD_LBP_ZERO);
else if (!strncmp(buf, lbp_mode[SD_LBP_DISABLE], 20))
sd_config_discard(sdkp, SD_LBP_DISABLE);
- else
+ else /* we don't allow manual setting of SD_LBP_ATA_TRIM */
return -EINVAL;
return count;
@@ -653,7 +654,7 @@ static void sd_config_discard(struct scsi_disk *sdkp, unsigned int mode)
* lead to data corruption. If LBPRZ is not set, we honor the
* device preference.
*/
- if (sdkp->lbprz) {
+ if (sdkp->lbprz || sdkp->device->ata_trim) {
q->limits.discard_alignment = 0;
q->limits.discard_granularity = logical_block_size;
} else {
@@ -695,6 +696,12 @@ static void sd_config_discard(struct scsi_disk *sdkp, unsigned int mode)
(u32)SD_MAX_WS10_BLOCKS);
q->limits.discard_zeroes_data = 1;
break;
+
+ case SD_LBP_ATA_TRIM:
+ max_blocks = 65535 * (512 / sizeof(__le64));
+ if (sdkp->device->ata_trim_zeroes_data)
+ q->limits.discard_zeroes_data = 1;
+ break;
}
blk_queue_max_discard_sectors(q, max_blocks * (logical_block_size >> 9));
@@ -794,6 +801,49 @@ static int sd_setup_write_same10_cmnd(struct scsi_cmnd *cmd, bool unmap)
return scsi_init_io(cmd);
}
+static int sd_setup_ata_trim_cmnd(struct scsi_cmnd *cmd)
+{
+ struct scsi_device *sdp = cmd->device;
+ struct request *rq = cmd->request;
+ u64 sector = blk_rq_pos(rq) >> (ilog2(sdp->sector_size) - 9);
+ u32 nr_sectors = blk_rq_sectors(rq) >> (ilog2(sdp->sector_size) - 9);
+ u32 data_len = sdp->sector_size, i;
+ __le64 *buf;
+
+ rq->special_vec.bv_page = alloc_page(GFP_ATOMIC | __GFP_ZERO);
+ if (!rq->special_vec.bv_page)
+ return BLKPREP_DEFER;
+ rq->special_vec.bv_offset = 0;
+ rq->special_vec.bv_len = data_len;
+ rq->rq_flags |= RQF_SPECIAL_PAYLOAD;
+
+ /*
+ * Use the Linux Vendor Specific TRIM command to pass the TRIM payload
+ * to libata.
+ */
+ cmd->cmd_len = 10;
+ cmd->cmnd[0] = LINUX_VS_TRIM;
+ cmd->cmnd[8] = data_len;
+
+ buf = page_address(rq->special_vec.bv_page);
+ for (i = 0; i < (data_len >> 3); i++) {
+ u64 n = min(nr_sectors, 0xffffu);
+
+ buf[i] = cpu_to_le64(sector | (n << 48));
+ if (nr_sectors <= 0xffff)
+ break;
+ sector += 0xffff;
+ nr_sectors -= 0xffff;
+ }
+
+ cmd->allowed = SD_MAX_RETRIES;
+ cmd->transfersize = data_len;
+ rq->timeout = SD_TIMEOUT;
+ scsi_req(rq)->resid_len = data_len;
+
+ return scsi_init_io(cmd);
+}
+
static void sd_config_write_same(struct scsi_disk *sdkp)
{
struct request_queue *q = sdkp->disk->queue;
@@ -1168,6 +1218,8 @@ static int sd_init_command(struct scsi_cmnd *cmd)
return sd_setup_write_same10_cmnd(cmd, true);
case SD_LBP_ZERO:
return sd_setup_write_same10_cmnd(cmd, false);
+ case SD_LBP_ATA_TRIM:
+ return sd_setup_ata_trim_cmnd(cmd);
default:
return BLKPREP_INVALID;
}
@@ -2739,7 +2791,9 @@ static void sd_read_block_limits(struct scsi_disk *sdkp)
sdkp->max_xfer_blocks = get_unaligned_be32(&buffer[8]);
sdkp->opt_xfer_blocks = get_unaligned_be32(&buffer[12]);
- if (buffer[3] == 0x3c) {
+ if (sdkp->device->ata_trim) {
+ sd_config_discard(sdkp, SD_LBP_ATA_TRIM);
+ } else if (buffer[3] == 0x3c) {
unsigned int lba_count, desc_count;
sdkp->max_ws_blocks = (u32)get_unaligned_be64(&buffer[36]);
diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
index 4dac35e96a75..711d48cea5d7 100644
--- a/drivers/scsi/sd.h
+++ b/drivers/scsi/sd.h
@@ -56,6 +56,7 @@ enum {
SD_LBP_WS16, /* Use WRITE SAME(16) with UNMAP bit */
SD_LBP_WS10, /* Use WRITE SAME(10) with UNMAP bit */
SD_LBP_ZERO, /* Use WRITE SAME(10) with zero payload */
+ SD_LBP_ATA_TRIM, /* generate a ATA TRIM payload for libata */
SD_LBP_DISABLE, /* Discard disabled due to failed cmd */
};
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 080c7ce9bae8..7b1450b1b130 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -182,6 +182,8 @@ struct scsi_device {
unsigned broken_fua:1; /* Don't set FUA bit */
unsigned lun_in_cdb:1; /* Store LUN bits in CDB[1] */
unsigned synchronous_alua:1; /* Synchronous ALUA commands */
+ unsigned ata_trim:1; /* use ATA TRIM payload for discard */
+ unsigned ata_trim_zeroes_data:1;/* ATA TRIM zeroes discard blocks */
atomic_t disk_events_disable_depth; /* disable depth for disk events */
diff --git a/include/scsi/scsi_proto.h b/include/scsi/scsi_proto.h
index 6ba66e01f6df..033b5662a5f5 100644
--- a/include/scsi/scsi_proto.h
+++ b/include/scsi/scsi_proto.h
@@ -169,6 +169,9 @@
/* Vendor specific CDBs start here */
#define VENDOR_SPECIFIC_CDB 0xc0
+/* used to pass the TRIM payload to libata with rewriting it: */
+#define LINUX_VS_TRIM VENDOR_SPECIFIC_CDB
+
/*
* SCSI command lengths
*/
--
2.11.0
^ permalink raw reply related
* [PATCH 3/7] libata: remove SCT WRITE SAME support
From: Christoph Hellwig @ 2017-03-20 20:43 UTC (permalink / raw)
To: tj, martin.petersen, axboe; +Cc: linux-ide, linux-scsi, linux-block
In-Reply-To: <20170320204319.12628-1-hch@lst.de>
This was already disabled a while ago because it caused I/O errors,
and it's severly getting into the way of the discard / write zeroes
rework.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/ata/libata-scsi.c | 128 +++++++++++-----------------------------------
1 file changed, 29 insertions(+), 99 deletions(-)
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 1ac70744ae7b..b93d7e33789a 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3393,46 +3393,6 @@ static size_t ata_format_dsm_trim_descr(struct scsi_cmnd *cmd, u32 trmax,
}
/**
- * ata_format_dsm_trim_descr() - SATL Write Same to ATA SCT Write Same
- * @cmd: SCSI command being translated
- * @lba: Starting sector
- * @num: Number of sectors to be zero'd.
- *
- * Rewrite the WRITE SAME payload to be an SCT Write Same formatted
- * descriptor.
- * NOTE: Writes a pattern (0's) in the foreground.
- *
- * Return: Number of bytes copied into sglist.
- */
-static size_t ata_format_sct_write_same(struct scsi_cmnd *cmd, u64 lba, u64 num)
-{
- struct scsi_device *sdp = cmd->device;
- size_t len = sdp->sector_size;
- size_t r;
- u16 *buf;
- unsigned long flags;
-
- spin_lock_irqsave(&ata_scsi_rbuf_lock, flags);
- buf = ((void *)ata_scsi_rbuf);
-
- put_unaligned_le16(0x0002, &buf[0]); /* SCT_ACT_WRITE_SAME */
- put_unaligned_le16(0x0101, &buf[1]); /* WRITE PTRN FG */
- put_unaligned_le64(lba, &buf[2]);
- put_unaligned_le64(num, &buf[6]);
- put_unaligned_le32(0u, &buf[10]); /* pattern */
-
- WARN_ON(len > ATA_SCSI_RBUF_SIZE);
-
- if (len > ATA_SCSI_RBUF_SIZE)
- len = ATA_SCSI_RBUF_SIZE;
-
- r = sg_copy_from_buffer(scsi_sglist(cmd), scsi_sg_count(cmd), buf, len);
- spin_unlock_irqrestore(&ata_scsi_rbuf_lock, flags);
-
- return r;
-}
-
-/**
* ata_scsi_write_same_xlat() - SATL Write Same to ATA SCT Write Same
* @qc: Command to be translated
*
@@ -3468,26 +3428,17 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc)
}
scsi_16_lba_len(cdb, &block, &n_block);
- if (unmap) {
- /* If trim is not enabled the cmd is invalid. */
- if ((dev->horkage & ATA_HORKAGE_NOTRIM) ||
- !ata_id_has_trim(dev->id)) {
- fp = 1;
- bp = 3;
- goto invalid_fld;
- }
- /* If the request is too large the cmd is invalid */
- if (n_block > 0xffff * trmax) {
- fp = 2;
- goto invalid_fld;
- }
- } else {
- /* If write same is not available the cmd is invalid */
- if (!ata_id_sct_write_same(dev->id)) {
- fp = 1;
- bp = 3;
- goto invalid_fld;
- }
+ if (!unmap ||
+ (dev->horkage & ATA_HORKAGE_NOTRIM) ||
+ !ata_id_has_trim(dev->id)) {
+ fp = 1;
+ bp = 3;
+ goto invalid_fld;
+ }
+ /* If the request is too large the cmd is invalid */
+ if (n_block > 0xffff * trmax) {
+ fp = 2;
+ goto invalid_fld;
}
/*
@@ -3502,49 +3453,28 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc)
* For DATA SET MANAGEMENT TRIM in ACS-2 nsect (aka count)
* is defined as number of 512 byte blocks to be transferred.
*/
- if (unmap) {
- size = ata_format_dsm_trim_descr(scmd, trmax, block, n_block);
- if (size != len)
- goto invalid_param_len;
- if (ata_ncq_enabled(dev) && ata_fpdma_dsm_supported(dev)) {
- /* Newer devices support queued TRIM commands */
- tf->protocol = ATA_PROT_NCQ;
- tf->command = ATA_CMD_FPDMA_SEND;
- tf->hob_nsect = ATA_SUBCMD_FPDMA_SEND_DSM & 0x1f;
- tf->nsect = qc->tag << 3;
- tf->hob_feature = (size / 512) >> 8;
- tf->feature = size / 512;
+ size = ata_format_dsm_trim_descr(scmd, trmax, block, n_block);
+ if (size != len)
+ goto invalid_param_len;
- tf->auxiliary = 1;
- } else {
- tf->protocol = ATA_PROT_DMA;
- tf->hob_feature = 0;
- tf->feature = ATA_DSM_TRIM;
- tf->hob_nsect = (size / 512) >> 8;
- tf->nsect = size / 512;
- tf->command = ATA_CMD_DSM;
- }
- } else {
- size = ata_format_sct_write_same(scmd, block, n_block);
- if (size != len)
- goto invalid_param_len;
+ if (ata_ncq_enabled(dev) && ata_fpdma_dsm_supported(dev)) {
+ /* Newer devices support queued TRIM commands */
+ tf->protocol = ATA_PROT_NCQ;
+ tf->command = ATA_CMD_FPDMA_SEND;
+ tf->hob_nsect = ATA_SUBCMD_FPDMA_SEND_DSM & 0x1f;
+ tf->nsect = qc->tag << 3;
+ tf->hob_feature = (size / 512) >> 8;
+ tf->feature = size / 512;
- tf->hob_feature = 0;
- tf->feature = 0;
- tf->hob_nsect = 0;
- tf->nsect = 1;
- tf->lbah = 0;
- tf->lbam = 0;
- tf->lbal = ATA_CMD_STANDBYNOW1;
- tf->hob_lbah = 0;
- tf->hob_lbam = 0;
- tf->hob_lbal = 0;
- tf->device = ATA_CMD_STANDBYNOW1;
+ tf->auxiliary = 1;
+ } else {
tf->protocol = ATA_PROT_DMA;
- tf->command = ATA_CMD_WRITE_LOG_DMA_EXT;
- if (unlikely(dev->flags & ATA_DFLAG_PIO))
- tf->command = ATA_CMD_WRITE_LOG_EXT;
+ tf->hob_feature = 0;
+ tf->feature = ATA_DSM_TRIM;
+ tf->hob_nsect = (size / 512) >> 8;
+ tf->nsect = size / 512;
+ tf->command = ATA_CMD_DSM;
}
tf->flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE | ATA_TFLAG_LBA48 |
--
2.11.0
^ permalink raw reply related
* [PATCH 4/7] libata: simplify the trim implementation
From: Christoph Hellwig @ 2017-03-20 20:43 UTC (permalink / raw)
To: tj, martin.petersen, axboe; +Cc: linux-ide, linux-scsi, linux-block
In-Reply-To: <20170320204319.12628-1-hch@lst.de>
Don't try to fake up basic SCSI logical block provisioning and WRITE SAME
support, but offer support for the Linux Vendor Specific TRIM command
instead. This simplifies the implementation a lot, and avoids rewriting
the data out buffer in the I/O path. Note that this new command is only
offered to the block layer and will fail for pass through commands.
While this is theoretically a regression in the functionality offered
through SG_IO the previous support was buggy and corrupted user memory
by rewriting the data out buffer in place.
Last but not least this removes the global ata_scsi_rbuf_lock from
the trim path.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/ata/libata-scsi.c | 179 ++++++++--------------------------------------
1 file changed, 28 insertions(+), 151 deletions(-)
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index b93d7e33789a..965b9e7dbb7d 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1322,6 +1322,16 @@ static int ata_scsi_dev_config(struct scsi_device *sdev,
blk_queue_flush_queueable(q, false);
+ if (ata_id_has_trim(dev->id) &&
+ !(dev->horkage & ATA_HORKAGE_NOTRIM)) {
+ sdev->ata_trim = 1;
+ if (ata_id_has_zero_after_trim(dev->id) &&
+ (dev->horkage & ATA_HORKAGE_ZERO_AFTER_TRIM)) {
+ ata_dev_info(dev, "Enabling discard_zeroes_data\n");
+ sdev->ata_trim_zeroes_data = 1;
+ }
+ }
+
dev->sdev = sdev;
return 0;
}
@@ -2383,21 +2393,6 @@ static unsigned int ata_scsiop_inq_b0(struct ata_scsi_args *args, u8 *rbuf)
*/
min_io_sectors = 1 << ata_id_log2_per_physical_sector(args->id);
put_unaligned_be16(min_io_sectors, &rbuf[6]);
-
- /*
- * Optimal unmap granularity.
- *
- * The ATA spec doesn't even know about a granularity or alignment
- * for the TRIM command. We can leave away most of the unmap related
- * VPD page entries, but we have specifify a granularity to signal
- * that we support some form of unmap - in thise case via WRITE SAME
- * with the unmap bit set.
- */
- if (ata_id_has_trim(args->id)) {
- put_unaligned_be64(65535 * ATA_MAX_TRIM_RNUM, &rbuf[36]);
- put_unaligned_be32(1, &rbuf[28]);
- }
-
return 0;
}
@@ -2746,16 +2741,6 @@ static unsigned int ata_scsiop_read_cap(struct ata_scsi_args *args, u8 *rbuf)
rbuf[14] = (lowest_aligned >> 8) & 0x3f;
rbuf[15] = lowest_aligned;
- if (ata_id_has_trim(args->id) &&
- !(dev->horkage & ATA_HORKAGE_NOTRIM)) {
- rbuf[14] |= 0x80; /* LBPME */
-
- if (ata_id_has_zero_after_trim(args->id) &&
- dev->horkage & ATA_HORKAGE_ZERO_AFTER_TRIM) {
- ata_dev_info(dev, "Enabling discard_zeroes_data\n");
- rbuf[14] |= 0x40; /* LBPRZ */
- }
- }
if (ata_id_zoned_cap(args->id) ||
args->dev->class == ATA_DEV_ZAC)
rbuf[12] = (1 << 4); /* RC_BASIS */
@@ -3339,141 +3324,45 @@ static unsigned int ata_scsi_pass_thru(struct ata_queued_cmd *qc)
}
/**
- * ata_format_dsm_trim_descr() - SATL Write Same to DSM Trim
- * @cmd: SCSI command being translated
- * @trmax: Maximum number of entries that will fit in sector_size bytes.
- * @sector: Starting sector
- * @count: Total Range of request in logical sectors
- *
- * Rewrite the WRITE SAME descriptor to be a DSM TRIM little-endian formatted
- * descriptor.
- *
- * Upto 64 entries of the format:
- * 63:48 Range Length
- * 47:0 LBA
- *
- * Range Length of 0 is ignored.
- * LBA's should be sorted order and not overlap.
- *
- * NOTE: this is the same format as ADD LBA(S) TO NV CACHE PINNED SET
- *
- * Return: Number of bytes copied into sglist.
- */
-static size_t ata_format_dsm_trim_descr(struct scsi_cmnd *cmd, u32 trmax,
- u64 sector, u32 count)
-{
- struct scsi_device *sdp = cmd->device;
- size_t len = sdp->sector_size;
- size_t r;
- __le64 *buf;
- u32 i = 0;
- unsigned long flags;
-
- WARN_ON(len > ATA_SCSI_RBUF_SIZE);
-
- if (len > ATA_SCSI_RBUF_SIZE)
- len = ATA_SCSI_RBUF_SIZE;
-
- spin_lock_irqsave(&ata_scsi_rbuf_lock, flags);
- buf = ((void *)ata_scsi_rbuf);
- memset(buf, 0, len);
- while (i < trmax) {
- u64 entry = sector |
- ((u64)(count > 0xffff ? 0xffff : count) << 48);
- buf[i++] = __cpu_to_le64(entry);
- if (count <= 0xffff)
- break;
- count -= 0xffff;
- sector += 0xffff;
- }
- r = sg_copy_from_buffer(scsi_sglist(cmd), scsi_sg_count(cmd), buf, len);
- spin_unlock_irqrestore(&ata_scsi_rbuf_lock, flags);
-
- return r;
-}
-
-/**
- * ata_scsi_write_same_xlat() - SATL Write Same to ATA SCT Write Same
+ * ata_scsi_trim_xlat() - Handle the vendor specific TRIM command.
* @qc: Command to be translated
*
- * Translate a SCSI WRITE SAME command to be either a DSM TRIM command or
- * an SCT Write Same command.
- * Based on WRITE SAME has the UNMAP flag
- * When set translate to DSM TRIM
- * When clear translate to SCT Write Same
+ * Setup a DSM TRIM command (or it's queued variant) after sd already
+ * prepared the payload for us.
*/
-static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc)
+static unsigned int ata_scsi_trim_xlat(struct ata_queued_cmd *qc)
{
struct ata_taskfile *tf = &qc->tf;
struct scsi_cmnd *scmd = qc->scsicmd;
- struct scsi_device *sdp = scmd->device;
- size_t len = sdp->sector_size;
struct ata_device *dev = qc->dev;
- const u8 *cdb = scmd->cmnd;
- u64 block;
- u32 n_block;
- const u32 trmax = len >> 3;
- u32 size;
- u16 fp;
- u8 bp = 0xff;
- u8 unmap = cdb[1] & 0x8;
-
- /* we may not issue DMA commands if no DMA mode is set */
- if (unlikely(!dev->dma_mode))
- goto invalid_opcode;
- if (unlikely(scmd->cmd_len < 16)) {
- fp = 15;
- goto invalid_fld;
+ if (unlikely(!dev->dma_mode)) {
+ ata_scsi_set_sense(dev, scmd, ILLEGAL_REQUEST, 0x20, 0x0);
+ return 1;
}
- scsi_16_lba_len(cdb, &block, &n_block);
- if (!unmap ||
- (dev->horkage & ATA_HORKAGE_NOTRIM) ||
- !ata_id_has_trim(dev->id)) {
- fp = 1;
- bp = 3;
- goto invalid_fld;
- }
- /* If the request is too large the cmd is invalid */
- if (n_block > 0xffff * trmax) {
- fp = 2;
- goto invalid_fld;
+ /* We only allow sending this command through the block layer */
+ if (unlikely(req_op(scmd->request) != REQ_OP_DISCARD)) {
+ ata_scsi_set_sense(dev, scmd, ILLEGAL_REQUEST, 0x20, 0x0);
+ return 1;
}
- /*
- * WRITE SAME always has a sector sized buffer as payload, this
- * should never be a multiple entry S/G list.
- */
- if (!scsi_sg_count(scmd))
- goto invalid_param_len;
-
- /*
- * size must match sector size in bytes
- * For DATA SET MANAGEMENT TRIM in ACS-2 nsect (aka count)
- * is defined as number of 512 byte blocks to be transferred.
- */
-
- size = ata_format_dsm_trim_descr(scmd, trmax, block, n_block);
- if (size != len)
- goto invalid_param_len;
-
if (ata_ncq_enabled(dev) && ata_fpdma_dsm_supported(dev)) {
/* Newer devices support queued TRIM commands */
tf->protocol = ATA_PROT_NCQ;
tf->command = ATA_CMD_FPDMA_SEND;
tf->hob_nsect = ATA_SUBCMD_FPDMA_SEND_DSM & 0x1f;
tf->nsect = qc->tag << 3;
- tf->hob_feature = (size / 512) >> 8;
- tf->feature = size / 512;
+ tf->hob_feature = (scmd->device->sector_size / 512) >> 8;
+ tf->feature = scmd->device->sector_size / 512;
tf->auxiliary = 1;
} else {
tf->protocol = ATA_PROT_DMA;
tf->hob_feature = 0;
tf->feature = ATA_DSM_TRIM;
- tf->hob_nsect = (size / 512) >> 8;
- tf->nsect = size / 512;
+ tf->hob_nsect = (scmd->device->sector_size / 512) >> 8;
+ tf->nsect = scmd->device->sector_size / 512;
tf->command = ATA_CMD_DSM;
}
@@ -3483,18 +3372,6 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc)
ata_qc_set_pc_nbytes(qc);
return 0;
-
-invalid_fld:
- ata_scsi_set_invalid_field(dev, scmd, fp, bp);
- return 1;
-invalid_param_len:
- /* "Parameter list length error" */
- ata_scsi_set_sense(dev, scmd, ILLEGAL_REQUEST, 0x1a, 0x0);
- return 1;
-invalid_opcode:
- /* "Invalid command operation code" */
- ata_scsi_set_sense(dev, scmd, ILLEGAL_REQUEST, 0x20, 0x0);
- return 1;
}
/**
@@ -4087,9 +3964,6 @@ static inline ata_xlat_func_t ata_get_xlat_func(struct ata_device *dev, u8 cmd)
case WRITE_16:
return ata_scsi_rw_xlat;
- case WRITE_SAME_16:
- return ata_scsi_write_same_xlat;
-
case SYNCHRONIZE_CACHE:
if (ata_try_flush_cache(dev))
return ata_scsi_flush_xlat;
@@ -4116,6 +3990,9 @@ static inline ata_xlat_func_t ata_get_xlat_func(struct ata_device *dev, u8 cmd)
case START_STOP:
return ata_scsi_start_stop_xlat;
+
+ case LINUX_VS_TRIM:
+ return ata_scsi_trim_xlat;
}
return NULL;
--
2.11.0
^ permalink raw reply related
* [PATCH 5/7] block: add a max_discard_segment_size queue limit
From: Christoph Hellwig @ 2017-03-20 20:43 UTC (permalink / raw)
To: tj, martin.petersen, axboe; +Cc: linux-ide, linux-scsi, linux-block
In-Reply-To: <20170320204319.12628-1-hch@lst.de>
ATA only allows 16 bits, so we need a limit.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
block/blk-core.c | 6 ++++++
block/blk-merge.c | 9 +++++++++
block/blk-settings.c | 14 ++++++++++++++
include/linux/blkdev.h | 8 ++++++++
4 files changed, 37 insertions(+)
diff --git a/block/blk-core.c b/block/blk-core.c
index d772c221cc17..3eb3bd89b47a 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1486,9 +1486,15 @@ bool bio_attempt_discard_merge(struct request_queue *q, struct request *req,
struct bio *bio)
{
unsigned short segments = blk_rq_nr_discard_segments(req);
+ unsigned max_segment_sectors = queue_max_discard_segment_size(q) >> 9;
if (segments >= queue_max_discard_segments(q))
goto no_merge;
+ if (blk_rq_sectors(req) > max_segment_sectors)
+ goto no_merge;
+ if (bio_sectors(bio) > max_segment_sectors)
+ goto no_merge;
+
if (blk_rq_sectors(req) + bio_sectors(bio) >
blk_rq_get_max_sectors(req, blk_rq_pos(req)))
goto no_merge;
diff --git a/block/blk-merge.c b/block/blk-merge.c
index 2afa262425d1..c62a6f0325e0 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -11,6 +11,15 @@
#include "blk.h"
+/*
+ * Split a discard bio if it doesn't fit into the overall discard request size
+ * of the device. Note that we don't split it here if it's over the maximum
+ * discard segment size to avoid creating way too many bios in that case.
+ * We will simply take care of never merging such a larger than segment size
+ * bio into a request that has other bios, and let the low-level driver take
+ * care of splitting the request into multiple ranges in the on the wire
+ * format.
+ */
static struct bio *blk_bio_discard_split(struct request_queue *q,
struct bio *bio,
struct bio_set *bs,
diff --git a/block/blk-settings.c b/block/blk-settings.c
index 1e7174ffc9d4..9d515ae3a405 100644
--- a/block/blk-settings.c
+++ b/block/blk-settings.c
@@ -93,6 +93,7 @@ void blk_set_default_limits(struct queue_limits *lim)
lim->seg_boundary_mask = BLK_SEG_BOUNDARY_MASK;
lim->virt_boundary_mask = 0;
lim->max_segment_size = BLK_MAX_SEGMENT_SIZE;
+ lim->max_discard_segment_size = UINT_MAX;
lim->max_sectors = lim->max_hw_sectors = BLK_SAFE_MAX_SECTORS;
lim->max_dev_sectors = 0;
lim->chunk_sectors = 0;
@@ -132,6 +133,7 @@ void blk_set_stacking_limits(struct queue_limits *lim)
lim->max_discard_segments = 1;
lim->max_hw_sectors = UINT_MAX;
lim->max_segment_size = UINT_MAX;
+ lim->max_discard_segment_size = UINT_MAX;
lim->max_sectors = UINT_MAX;
lim->max_dev_sectors = UINT_MAX;
lim->max_write_same_sectors = UINT_MAX;
@@ -376,6 +378,18 @@ void blk_queue_max_segment_size(struct request_queue *q, unsigned int max_size)
EXPORT_SYMBOL(blk_queue_max_segment_size);
/**
+ * blk_queue_max_discard_segment_size - set max segment size for discards
+ * @q: the request queue for the device
+ * @max_size: max size of a discard segment in bytes
+ **/
+void blk_queue_max_discard_segment_size(struct request_queue *q,
+ unsigned int max_size)
+{
+ q->limits.max_discard_segment_size = max_size;
+}
+EXPORT_SYMBOL_GPL(blk_queue_max_discard_segment_size);
+
+/**
* blk_queue_logical_block_size - set logical block size for the queue
* @q: the request queue for the device
* @size: the logical block size, in bytes
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 5a7da607ca04..3b3bd646f580 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -333,6 +333,7 @@ struct queue_limits {
unsigned short max_segments;
unsigned short max_integrity_segments;
unsigned short max_discard_segments;
+ unsigned int max_discard_segment_size;
unsigned char misaligned;
unsigned char discard_misaligned;
@@ -1150,6 +1151,8 @@ extern void blk_queue_max_segments(struct request_queue *, unsigned short);
extern void blk_queue_max_discard_segments(struct request_queue *,
unsigned short);
extern void blk_queue_max_segment_size(struct request_queue *, unsigned int);
+extern void blk_queue_max_discard_segment_size(struct request_queue *,
+ unsigned int);
extern void blk_queue_max_discard_sectors(struct request_queue *q,
unsigned int max_discard_sectors);
extern void blk_queue_max_write_same_sectors(struct request_queue *q,
@@ -1415,6 +1418,11 @@ static inline unsigned int queue_max_segment_size(struct request_queue *q)
return q->limits.max_segment_size;
}
+static inline unsigned int queue_max_discard_segment_size(struct request_queue *q)
+{
+ return q->limits.max_discard_segment_size;
+}
+
static inline unsigned short queue_logical_block_size(struct request_queue *q)
{
int retval = 512;
--
2.11.0
^ permalink raw reply related
* [PATCH 6/7] sd: support multi-range TRIM for ATA disks
From: Christoph Hellwig @ 2017-03-20 20:43 UTC (permalink / raw)
To: tj, martin.petersen, axboe; +Cc: linux-ide, linux-scsi, linux-block
In-Reply-To: <20170320204319.12628-1-hch@lst.de>
Almost the same scheme as the older multi-range support for NVMe.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/scsi/sd.c | 37 +++++++++++++++++++++++++------------
1 file changed, 25 insertions(+), 12 deletions(-)
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index cc8684818305..c18fe9ff1f8f 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -643,7 +643,7 @@ static void sd_config_discard(struct scsi_disk *sdkp, unsigned int mode)
{
struct request_queue *q = sdkp->disk->queue;
unsigned int logical_block_size = sdkp->device->sector_size;
- unsigned int max_blocks = 0;
+ unsigned int max_blocks = 0, max_ranges = 0, max_range_size = 0;
q->limits.discard_zeroes_data = 0;
@@ -698,13 +698,19 @@ static void sd_config_discard(struct scsi_disk *sdkp, unsigned int mode)
break;
case SD_LBP_ATA_TRIM:
- max_blocks = 65535 * (512 / sizeof(__le64));
+ max_ranges = 512 / sizeof(__le64);
+ max_range_size = USHRT_MAX;
+ max_blocks = max_ranges * max_range_size;
if (sdkp->device->ata_trim_zeroes_data)
q->limits.discard_zeroes_data = 1;
break;
}
blk_queue_max_discard_sectors(q, max_blocks * (logical_block_size >> 9));
+ if (max_ranges)
+ blk_queue_max_discard_segments(q, max_ranges);
+ if (max_range_size)
+ blk_queue_max_discard_segment_size(q, max_range_size);
queue_flag_set_unlocked(QUEUE_FLAG_DISCARD, q);
}
@@ -805,9 +811,9 @@ static int sd_setup_ata_trim_cmnd(struct scsi_cmnd *cmd)
{
struct scsi_device *sdp = cmd->device;
struct request *rq = cmd->request;
- u64 sector = blk_rq_pos(rq) >> (ilog2(sdp->sector_size) - 9);
- u32 nr_sectors = blk_rq_sectors(rq) >> (ilog2(sdp->sector_size) - 9);
- u32 data_len = sdp->sector_size, i;
+ u32 sector_shift = ilog2(sdp->sector_size);
+ u32 data_len = sdp->sector_size, i = 0;
+ struct bio *bio;
__le64 *buf;
rq->special_vec.bv_page = alloc_page(GFP_ATOMIC | __GFP_ZERO);
@@ -826,14 +832,21 @@ static int sd_setup_ata_trim_cmnd(struct scsi_cmnd *cmd)
cmd->cmnd[8] = data_len;
buf = page_address(rq->special_vec.bv_page);
- for (i = 0; i < (data_len >> 3); i++) {
- u64 n = min(nr_sectors, 0xffffu);
+ __rq_for_each_bio(bio, rq) {
+ u64 sector = bio->bi_iter.bi_sector >> (sector_shift - 9);
+ u32 nr_sectors = bio->bi_iter.bi_size >> sector_shift;
- buf[i] = cpu_to_le64(sector | (n << 48));
- if (nr_sectors <= 0xffff)
- break;
- sector += 0xffff;
- nr_sectors -= 0xffff;
+ do {
+ u64 n = min(nr_sectors, 0xffffu);
+
+ buf[i] = cpu_to_le64(sector | (n << 48));
+ if (nr_sectors <= 0xffff)
+ break;
+ sector += 0xffff;
+ nr_sectors -= 0xffff;
+ i++;
+
+ } while (!WARN_ON_ONCE(i >= data_len >> 3));
}
cmd->allowed = SD_MAX_RETRIES;
--
2.11.0
^ permalink raw reply related
* [PATCH 7/7] sd: use ZERO_PAGE for WRITE_SAME payloads
From: Christoph Hellwig @ 2017-03-20 20:43 UTC (permalink / raw)
To: tj, martin.petersen, axboe; +Cc: linux-ide, linux-scsi, linux-block
In-Reply-To: <20170320204319.12628-1-hch@lst.de>
We're never touching the contents of the page, so save a memory
allocation for these cases.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/scsi/sd.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index c18fe9ff1f8f..af632e350ab4 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -756,7 +756,7 @@ static int sd_setup_write_same16_cmnd(struct scsi_cmnd *cmd)
u32 nr_sectors = blk_rq_sectors(rq) >> (ilog2(sdp->sector_size) - 9);
u32 data_len = sdp->sector_size;
- rq->special_vec.bv_page = alloc_page(GFP_ATOMIC | __GFP_ZERO);
+ rq->special_vec.bv_page = ZERO_PAGE(0);
if (!rq->special_vec.bv_page)
return BLKPREP_DEFER;
rq->special_vec.bv_offset = 0;
@@ -785,7 +785,7 @@ static int sd_setup_write_same10_cmnd(struct scsi_cmnd *cmd, bool unmap)
u32 nr_sectors = blk_rq_sectors(rq) >> (ilog2(sdp->sector_size) - 9);
u32 data_len = sdp->sector_size;
- rq->special_vec.bv_page = alloc_page(GFP_ATOMIC | __GFP_ZERO);
+ rq->special_vec.bv_page = ZERO_PAGE(0);
if (!rq->special_vec.bv_page)
return BLKPREP_DEFER;
rq->special_vec.bv_offset = 0;
@@ -1256,7 +1256,8 @@ static void sd_uninit_command(struct scsi_cmnd *SCpnt)
{
struct request *rq = SCpnt->request;
- if (rq->rq_flags & RQF_SPECIAL_PAYLOAD)
+ if ((rq->rq_flags & RQF_SPECIAL_PAYLOAD) &&
+ rq->special_vec.bv_page != ZERO_PAGE(0))
__free_page(rq->special_vec.bv_page);
if (SCpnt->cmnd != scsi_req(rq)->cmd) {
--
2.11.0
^ permalink raw reply related
* Re: [PATCH v2 1/2] dt-bindings: ata: add DT bindings for ahci-dm816 SATA controller
From: Rob Herring @ 2017-03-20 22:13 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Tejun Heo, Mark Rutland, Neil Armstrong, Michael Turquette,
Kevin Hilman, Patrick Titiano, Tony Lindgren, Sergei Shtylyov,
linux-ide, devicetree, linux-kernel
In-Reply-To: <1489489491-14195-2-git-send-email-bgolaszewski@baylibre.com>
On Tue, Mar 14, 2017 at 12:04:50PM +0100, Bartosz Golaszewski wrote:
> Add DT bindings for the onboard SATA controller present on the DM816x
> SoCs.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
> .../devicetree/bindings/ata/ahci-dm816.txt | 21 +++++++++++++++++++++
> 1 file changed, 21 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/ata/ahci-dm816.txt
Acked-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* Re: [PATCH v2 1/3] ata: add Palmchip BK3710 PATA controller driver
From: Sekhar Nori @ 2017-03-21 8:34 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz, Tejun Heo
Cc: Russell King, Sergei Shtylyov, Arnd Bergmann,
Dmitry Eremin-Solenikov, Kevin Hilman, linux-kernel, linux-ide,
linux-arm-kernel
In-Reply-To: <1489509414-11491-2-git-send-email-b.zolnierkie@samsung.com>
Hi,
On Tuesday 14 March 2017 10:06 PM, Bartlomiej Zolnierkiewicz wrote:
> Add Palmchip BK3710 PATA controller driver.
>
> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
There are some checkpatch suggestions that are worth fixing, I think.
[PATCH v2 1_3] ata: add Palmchip BK3710 PATA controller driver - Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> - 2017-03-14 2206.eml:277: CHECK: Alignment should match open parenthesis
[PATCH v2 1_3] ata: add Palmchip BK3710 PATA controller driver - Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> - 2017-03-14 2206.eml:320: CHECK: Alignment should match open parenthesis
[PATCH v2 1_3] ata: add Palmchip BK3710 PATA controller driver - Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> - 2017-03-14 2206.eml:489: CHECK: Comparison to NULL could be written "!mem"
Thanks,
Sekhar
^ permalink raw reply
* Re: [PATCH v2 0/3] ATA/ARM: convert ARM/DaVinci to use libata PATA drivers
From: Sekhar Nori @ 2017-03-21 8:38 UTC (permalink / raw)
To: Tejun Heo, Bartlomiej Zolnierkiewicz
Cc: Russell King, Sergei Shtylyov, Arnd Bergmann,
Dmitry Eremin-Solenikov, Kevin Hilman, linux-kernel, linux-ide,
linux-arm-kernel
In-Reply-To: <20170316213255.GF1480@htj.duckdns.org>
On Friday 17 March 2017 03:02 AM, Tejun Heo wrote:
> On Tue, Mar 14, 2017 at 05:36:51PM +0100, Bartlomiej Zolnierkiewicz wrote:
>> Hi,
>>
>> This patchset adds Palmchip BK3710 IDE controller driver to
>> libata and switches ARM/DaVinci to use it (instead of the old
>> IDE driver).
>>
>> Sekhar, please check that it still works after changes, thanks.
>>
>> Changes since v1
>> (https://www.spinics.net/lists/arm-kernel/msg567442.html):
>> - addressed review comments from Sergei Shtylyov
>> - fixed cycle_time unitialized variable issue
>>
>> Changes since v0.1 draft patch version
>> (https://www.spinics.net/lists/arm-kernel/msg566932.html):
>> - fixed cycle_time build warning
>> - added platform support fixes from Sekhar
>> - added defconfig changes from Sekhar
>> - preserved platform support for the old IDE driver
>> - split it on 3 patches
>
> Looks fine to me from libata side. Once it gets tested, how should
> the patches be routed? I don't think it'd make sense to route them
> separately.
Hi Tejun, I can take the series through ARM-SoC tree with your ack.
Thanks,
Sekhar
^ permalink raw reply
* Re: [PATCH v2 0/3] ATA/ARM: convert ARM/DaVinci to use libata PATA drivers
From: Sekhar Nori @ 2017-03-21 8:39 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz, Tejun Heo
Cc: Sergei Shtylyov, Kevin Hilman, Arnd Bergmann, Russell King,
Dmitry Eremin-Solenikov, linux-ide, linux-arm-kernel,
linux-kernel
In-Reply-To: <1489509414-11491-1-git-send-email-b.zolnierkie@samsung.com>
On Tuesday 14 March 2017 10:06 PM, Bartlomiej Zolnierkiewicz wrote:
> Hi,
>
> This patchset adds Palmchip BK3710 IDE controller driver to
> libata and switches ARM/DaVinci to use it (instead of the old
> IDE driver).
>
> Sekhar, please check that it still works after changes, thanks.
It looks like there are some comments pending. I would like to test
again after those are dispositioned. Hope thats fine.
Thanks,
Sekhar
^ permalink raw reply
* 51519 linux-ide
From: robert.berry @ 2017-03-21 13:42 UTC (permalink / raw)
To: linux-ide
[-- Attachment #1: 76130338818.zip --]
[-- Type: application/zip, Size: 2949 bytes --]
^ permalink raw reply
* Re: [PATCH v2 0/3] ATA/ARM: convert ARM/DaVinci to use libata PATA drivers
From: Tejun Heo @ 2017-03-21 16:56 UTC (permalink / raw)
To: Sekhar Nori
Cc: Russell King, Sergei Shtylyov, Arnd Bergmann,
Bartlomiej Zolnierkiewicz, Dmitry Eremin-Solenikov, Kevin Hilman,
linux-kernel, linux-ide, linux-arm-kernel
In-Reply-To: <3c115ec1-2d11-695e-eca7-25a75f71b96f@ti.com>
Hello,
On Tue, Mar 21, 2017 at 02:08:26PM +0530, Sekhar Nori wrote:
> > Looks fine to me from libata side. Once it gets tested, how should
> > the patches be routed? I don't think it'd make sense to route them
> > separately.
>
> Hi Tejun, I can take the series through ARM-SoC tree with your ack.
Once the review points are addressed, please feel free to add
Acked-by: Tejun Heo <tj@kernel.org>
and route through the arm tree.
Thanks.
--
tejun
^ permalink raw reply
* Re: support ranges TRIM for libata
From: Tejun Heo @ 2017-03-21 18:59 UTC (permalink / raw)
To: Christoph Hellwig
Cc: martin.petersen, axboe, linux-ide, linux-scsi, linux-block
In-Reply-To: <20170320204319.12628-1-hch@lst.de>
Hello,
On Mon, Mar 20, 2017 at 04:43:12PM -0400, Christoph Hellwig wrote:
> This series implements rangeѕ discard for ATA SSDs. Compared to the
> initial NVMe support there are two things that complicate the ATA
> support:
>
> - ATA only suports 16-bit long ranges
> - the whole mess of generating a SCSI command first and then
> translating it to an ATA one.
>
> This series adds support for limited range size to the block layer,
> and stops translating discard commands - instead we add a new
> Vendor Specific SCSI command that contains the TRIM payload when
> the device asks for it.
I do like the fact that this is a lot simpler than the previous
implementation but am not quite sure we want to deviate significantly
from what we do for other commands (command translation). Is it
because fixing the existing implementation would involve invaisve
changes including memory allocations?
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH v2 1/3] ata: add Palmchip BK3710 PATA controller driver
From: Bartlomiej Zolnierkiewicz @ 2017-03-22 17:49 UTC (permalink / raw)
To: Sekhar Nori
Cc: Tejun Heo, Sergei Shtylyov, Kevin Hilman, Arnd Bergmann,
Russell King, Dmitry Eremin-Solenikov, linux-ide,
linux-arm-kernel, linux-kernel
In-Reply-To: <8fc9171f-15b7-2c6a-d0b5-3af21cc88405@ti.com>
Hi,
On Tuesday, March 21, 2017 02:04:38 PM Sekhar Nori wrote:
> Hi,
>
> On Tuesday 14 March 2017 10:06 PM, Bartlomiej Zolnierkiewicz wrote:
> > Add Palmchip BK3710 PATA controller driver.
> >
> > Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
>
> There are some checkpatch suggestions that are worth fixing, I think.
>
> [PATCH v2 1_3] ata: add Palmchip BK3710 PATA controller driver - Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> - 2017-03-14 2206.eml:277: CHECK: Alignment should match open parenthesis
> [PATCH v2 1_3] ata: add Palmchip BK3710 PATA controller driver - Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> - 2017-03-14 2206.eml:320: CHECK: Alignment should match open parenthesis
> [PATCH v2 1_3] ata: add Palmchip BK3710 PATA controller driver - Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> - 2017-03-14 2206.eml:489: CHECK: Comparison to NULL could be written "!mem"
Will be fixed in v3, thanks.
[ My scripts didn't run checkpatch with --strict so I haven't noticed them. ]
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
^ permalink raw reply
* Re: [PATCH v2 1/3] ata: add Palmchip BK3710 PATA controller driver
From: Sergei Shtylyov @ 2017-03-22 17:56 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz, Sekhar Nori
Cc: Tejun Heo, Kevin Hilman, Arnd Bergmann, Russell King,
Dmitry Eremin-Solenikov, linux-ide, linux-arm-kernel,
linux-kernel
In-Reply-To: <1819267.Nm9hpTOhuY@amdc3058>
Hello!
On 03/22/2017 08:49 PM, Bartlomiej Zolnierkiewicz wrote:
>>> Add Palmchip BK3710 PATA controller driver.
>>>
>>> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
>>
>> There are some checkpatch suggestions that are worth fixing, I think.
>>
>> [PATCH v2 1_3] ata: add Palmchip BK3710 PATA controller driver - Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> - 2017-03-14 2206.eml:277: CHECK: Alignment should match open parenthesis
>> [PATCH v2 1_3] ata: add Palmchip BK3710 PATA controller driver - Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> - 2017-03-14 2206.eml:320: CHECK: Alignment should match open parenthesis
>> [PATCH v2 1_3] ata: add Palmchip BK3710 PATA controller driver - Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> - 2017-03-14 2206.eml:489: CHECK: Comparison to NULL could be written "!mem"
>
> Will be fixed in v3, thanks.
>
> [ My scripts didn't run checkpatch with --strict so I haven't noticed them. ]
--strict'ly speaking, this switch is only forced on in the networking
trees. :-)
> Best regards,
> --
> Bartlomiej Zolnierkiewicz
> Samsung R&D Institute Poland
> Samsung Electronics
MBR, Sergei
^ permalink raw reply
* Re: [PATCH v2 1/3] ata: add Palmchip BK3710 PATA controller driver
From: Bartlomiej Zolnierkiewicz @ 2017-03-22 17:59 UTC (permalink / raw)
To: Sergei Shtylyov
Cc: Tejun Heo, Sekhar Nori, Kevin Hilman, Arnd Bergmann, Russell King,
Dmitry Eremin-Solenikov, linux-ide, linux-arm-kernel,
linux-kernel
In-Reply-To: <bc9a2d9b-1f16-4b59-0033-235686c76dcb@cogentembedded.com>
Hi,
On Saturday, March 18, 2017 04:52:18 PM Sergei Shtylyov wrote:
> Hello!
>
> On 3/14/2017 7:36 PM, Bartlomiej Zolnierkiewicz wrote:
>
> > Add Palmchip BK3710 PATA controller driver.
> >
> > Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> [...]
> > diff --git a/drivers/ata/pata_bk3710.c b/drivers/ata/pata_bk3710.c
> > new file mode 100644
> > index 0000000..6d77217
> > --- /dev/null
> > +++ b/drivers/ata/pata_bk3710.c
> > @@ -0,0 +1,386 @@
> [...]
> > +static void pata_bk3710_chipinit(void __iomem *base)
> > +{
> [...]
> > + /*
> > + * IORDYTMP IORDY Timer for Primary Register
> > + * (ATA_IORDYTMP_IORDYTMP , 0xffff )
> > + */
> > + iowrite32(0xFFFF, base + BK3710_IORDYTMP);
>
> As I've already said, this is useless as we don't handle the IORDY timeout
> interrupt anyway; writing 0 would be fine.
Will fix in v3, in the incremental patch (so it is easier to revert
if it turns out to cause problems later or port to palm_bk3710).
> > +
> > + /*
> > + * Configure BMISP Register
> > + * (ATA_BMISP_DMAEN1 , DISABLE ) |
> > + * (ATA_BMISP_DMAEN0 , DISABLE ) |
> > + * (ATA_BMISP_IORDYINT , CLEAR) |
> > + * (ATA_BMISP_INTRSTAT , CLEAR) |
> > + * (ATA_BMISP_DMAERROR , CLEAR)
> > + */
> > + iowrite16(0, base + BK3710_BMISP);
>
> Bits 0-3 cane only be cleared by writing 1, so this write can't clear
The documentation does say this about bits 1-3, bit 0 is handled in
a different way.
> them, contrary to what the comment says. Might be a material for a follow-up
> patch tho...
Will fix in the incremental patch in v3.
> [...]
> > +static int __init pata_bk3710_probe(struct platform_device *pdev)
> > +{
> > + struct clk *clk;
> > + struct resource *mem;
> > + struct ata_host *host;
> > + struct ata_port *ap;
> > + void __iomem *base;
> > + unsigned long rate;
> > + int irq;
> > +
> > + clk = devm_clk_get(&pdev->dev, NULL);
> > + if (IS_ERR(clk))
> > + return -ENODEV;
> > +
> > + clk_enable(clk);
> > + rate = clk_get_rate(clk);
> > + if (!rate)
> > + return -EINVAL;
> > +
> > + /* NOTE: round *down* to meet minimum timings; we count in clocks */
> > + ideclk_period = 1000000000UL / rate;
> > +
> > + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + if (mem == NULL) {
> > + pr_err(DRV_NAME ": failed to get memory region resource\n");
> > + return -ENODEV;
> > + }
>
> NULL check not needed here, devm_ioremap_resource() checks this anyway.
Will be fixed in v3.
> > +
> > + irq = platform_get_irq(pdev, 0);
> > + if (irq < 0) {
> > + pr_err(DRV_NAME ": failed to get IRQ resource\n");
> > + return irq;
> > + }
> > +
> > + base = devm_ioremap_resource(&pdev->dev, mem);
> > + if (IS_ERR(base))
> > + return PTR_ERR(base);
> > +
> [...]
> > +/* work with hotplug and coldplug */
> > +MODULE_ALIAS("platform:palm_bk3710");
> > +
> > +static struct platform_driver pata_bk3710_driver = {
> > + .driver = {
> > + .name = "palm_bk3710",
>
> Not DRV_NAME?
DRV_NAME is "pata_bk3710" and the platform driver name needs to
match the old driver name for compatibility reasons (supporting
both drivers by the arch specific code).
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
^ 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