RE: [PATCH v2] drivers/ata: add support to Freescale 3.0Gbps SATA Controller
> -Original Message- > From: Alan Cox [mailto:[EMAIL PROTECTED] > Sent: Friday, October 12, 2007 9:55 PM > To: Li Yang-r58472 > Cc: [EMAIL PROTECTED]; [EMAIL PROTECTED]; > linuxppc-dev@ozlabs.org; Kalra Ashish-B00888; Li Yang-r58472 > Subject: Re: [PATCH v2] drivers/ata: add support to Freescale > 3.0Gbps SATA Controller > > > + cd = pp->cmdentry + tag; > > + > > + memcpy(fis, &cd->sfis, 6 * 4); /* should we use > memcpy_from_io() */ > > If cd->sfis points at memory over the PCI bus (eg mmio or > memory on the controller card) then you need to use > ioread/_io type functions. If > cd->sfis points into host memory where the FIS is delivered > by DMA from > the card you will be fine if it was allocated with an > _coherent allocator Thanks for the clarification. So the code here is ok, that we use command descriptor from dma_alloc_coherent(). :) - Leo ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH v2] drivers/ata: add support to Freescale 3.0Gbps SATA Controller
On Friday 12 October 2007, Li Yang wrote:
> This patch adds support for Freescale 3.0Gbps SATA Controller supporting
> Native Command Queueing(NCQ), device hotplug, and ATAPI. This controller
> can be found on MPC8315 and MPC8378.
Most of the driver looks really good, but here are a few things that
stick out:
> +static int sata_fsl_probe(struct of_device *ofdev,
> + const struct of_device_id *match)
> +{
> + int retval = 0;
> + void __iomem *hcr_base = NULL;
> + void __iomem *ssr_base = NULL;
> + void __iomem *csr_base = NULL;
> + struct sata_fsl_host_priv *host_priv = NULL;
> + struct resource *r;
> + int irq;
> + struct ata_host *host;
> +
> + struct ata_port_info pi = sata_fsl_port_info[0];
> + const struct ata_port_info *ppi[] = { &pi, NULL };
> +
> + dev_printk(KERN_INFO, &ofdev->dev,
> +"Sata FSL Platform/CSB Driver init\n");
> +
> + r = kmalloc(sizeof(struct resource), GFP_KERNEL);
> + retval = of_address_to_resource(ofdev->node, 0, r);
> + if (retval)
> + return -EINVAL;
> +
> + DPRINTK("start i/o @0x%x\n", r->start);
> +
> + hcr_base = ioremap(r->start, r->end - r->start + 1);
> + if (!hcr_base)
> + goto error_exit_with_cleanup;
Hmm, I think we should redefine of_iomap to do the right thing
for you. currently, it is the combination of of_address_to_resource
and ioremap, which you do as well, so your code can be simplified
to do that. However, ioremap is meant to be used with readl/writel
or in_le32/out_le32 and similar functions, not with ioread32/iowrite32
which you are using.
I had planned to do a patch to get that right for some time so
you can use of_iomap with ioread in all cases, but I guess you
should start using of_iomap even now.
> +
> +error_exit_with_cleanup:
> +
> + if (hcr_base)
> + iounmap(hcr_base);
> + if (host_priv)
> + kfree(host_priv);
> +
> + return retval;
> +}
Once of_iomap start using devres, we would no longer need to iounmap here.
> +static int sata_fsl_remove(struct of_device *ofdev)
> +{
> + struct ata_host *host = dev_get_drvdata(&ofdev->dev);
> + struct sata_fsl_host_priv *host_priv = host->private_data;
> +
> + dev_set_drvdata(&ofdev->dev, NULL);
> +
> + iounmap(host_priv->hcr_base);
> + kfree(host_priv);
> +
> + return 0;
> +}
Should you also free the irq mapping here?
> --- /dev/null
> +++ b/drivers/ata/sata_fsl.h
> @@ -0,0 +1,92 @@
> +/*
> + * drivers/ata/sata_fsl.h
> + *
> + * Freescale 3.0Gbps SATA device driver
The header file is entirely pointless, since all definitions in here
are only used in a single file. Please merge the header into the
implementation.
> +static int sata_fsl_probe(struct of_device *ofdev,
> + const struct of_device_id *match);
> +static int sata_fsl_remove(struct of_device *ofdev);
> +static int sata_fsl_scr_read(struct ata_port *, unsigned int, u32 *);
> +static int sata_fsl_scr_write(struct ata_port *, unsigned int, u32);
> +static unsigned int sata_fsl_qc_issue(struct ata_queued_cmd *);
> +static irqreturn_t sata_fsl_interrupt(int, void *);
> +static void sata_fsl_irq_clear(struct ata_port *);
> +static int sata_fsl_port_start(struct ata_port *);
> +static void sata_fsl_port_stop(struct ata_port *);
> +static void sata_fsl_tf_read(struct ata_port *, struct ata_taskfile *);
> +static void sata_fsl_qc_prep(struct ata_queued_cmd *);
> +static u8 sata_fsl_check_status(struct ata_port *);
> +static void sata_fsl_freeze(struct ata_port *);
> +static void sata_fsl_thaw(struct ata_port *);
> +static void sata_fsl_error_handler(struct ata_port *);
> +static void sata_fsl_post_internal_cmd(struct ata_queued_cmd *);
> +
> +static inline unsigned int sata_fsl_tag(unsigned int, void __iomem *);
In particular, get rid of the forward declarations for static functions.
All functions in a simple driver should be ordered in a way that you
always reference only code that was previously defined. This helps
avoid accidental recursions and makes it easier to review.
Arnd <><
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH v2] drivers/ata: add support to Freescale 3.0Gbps SATA Controller
> + cd = pp->cmdentry + tag; > + > + memcpy(fis, &cd->sfis, 6 * 4); /* should we use memcpy_from_io() */ If cd->sfis points at memory over the PCI bus (eg mmio or memory on the controller card) then you need to use ioread/_io type functions. If cd->sfis points into host memory where the FIS is delivered by DMA from the card you will be fine if it was allocated with an _coherent allocator Alan ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
RE: [PATCH v2] drivers/ata: add support to Freescale 3.0Gbps SATA Controller
This should be fine, as cd->sfis is allocated in host memory using a _coherent allocator and the SATA-2 controller is DMA'ing the D2H FIS into it. Ashish -Original Message- From: Alan Cox [mailto:[EMAIL PROTECTED] Sent: Friday, October 12, 2007 7:25 PM To: Li Yang-r58472 Cc: [EMAIL PROTECTED]; [EMAIL PROTECTED]; linuxppc-dev@ozlabs.org; Kalra Ashish-B00888; Li Yang-r58472 Subject: Re: [PATCH v2] drivers/ata: add support to Freescale 3.0Gbps SATA Controller > + cd = pp->cmdentry + tag; > + > + memcpy(fis, &cd->sfis, 6 * 4); /* should we use memcpy_from_io() */ If cd->sfis points at memory over the PCI bus (eg mmio or memory on the controller card) then you need to use ioread/_io type functions. If cd->sfis points into host memory where the FIS is delivered by DMA from the card you will be fine if it was allocated with an _coherent allocator Alan ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
RE: [PATCH v2] drivers/ata: add support to Freescale 3.0Gbps SATA Controller
Hello Arnd,
Thanks for your comments and feedback.
Actually, for PowerPC platforms iowrite32/ioread32 internally call
writel/readl, which are again mapped to out_le32/in_le32,
therefore we will modify our code to use of_iomap() to combine
functionalities of both of_address_to_resource() & ioremap().
We will also incorporate your other suggestions and then resubmit the
patch.
Ashish
-Original Message-
From: Arnd Bergmann [mailto:[EMAIL PROTECTED]
Sent: Friday, October 12, 2007 7:52 PM
To: linuxppc-dev@ozlabs.org
Cc: Li Yang-r58472; [EMAIL PROTECTED]; [EMAIL PROTECTED]; Kalra
Ashish-B00888
Subject: Re: [PATCH v2] drivers/ata: add support to Freescale 3.0Gbps
SATA Controller
On Friday 12 October 2007, Li Yang wrote:
> This patch adds support for Freescale 3.0Gbps SATA Controller
> supporting Native Command Queueing(NCQ), device hotplug, and ATAPI.
> This controller can be found on MPC8315 and MPC8378.
Most of the driver looks really good, but here are a few things that
stick out:
> +static int sata_fsl_probe(struct of_device *ofdev,
> + const struct of_device_id *match)
> +{
> + int retval = 0;
> + void __iomem *hcr_base = NULL;
> + void __iomem *ssr_base = NULL;
> + void __iomem *csr_base = NULL;
> + struct sata_fsl_host_priv *host_priv = NULL;
> + struct resource *r;
> + int irq;
> + struct ata_host *host;
> +
> + struct ata_port_info pi = sata_fsl_port_info[0];
> + const struct ata_port_info *ppi[] = { &pi, NULL };
> +
> + dev_printk(KERN_INFO, &ofdev->dev,
> +"Sata FSL Platform/CSB Driver init\n");
> +
> + r = kmalloc(sizeof(struct resource), GFP_KERNEL);
> + retval = of_address_to_resource(ofdev->node, 0, r);
> + if (retval)
> + return -EINVAL;
> +
> + DPRINTK("start i/o @0x%x\n", r->start);
> +
> + hcr_base = ioremap(r->start, r->end - r->start + 1);
> + if (!hcr_base)
> + goto error_exit_with_cleanup;
Hmm, I think we should redefine of_iomap to do the right thing for you.
currently, it is the combination of of_address_to_resource and ioremap,
which you do as well, so your code can be simplified to do that.
However, ioremap is meant to be used with readl/writel or
in_le32/out_le32 and similar functions, not with ioread32/iowrite32
which you are using.
I had planned to do a patch to get that right for some time so you can
use of_iomap with ioread in all cases, but I guess you should start
using of_iomap even now.
> +
> +error_exit_with_cleanup:
> +
> + if (hcr_base)
> + iounmap(hcr_base);
> + if (host_priv)
> + kfree(host_priv);
> +
> + return retval;
> +}
Once of_iomap start using devres, we would no longer need to iounmap
here.
> +static int sata_fsl_remove(struct of_device *ofdev) {
> + struct ata_host *host = dev_get_drvdata(&ofdev->dev);
> + struct sata_fsl_host_priv *host_priv = host->private_data;
> +
> + dev_set_drvdata(&ofdev->dev, NULL);
> +
> + iounmap(host_priv->hcr_base);
> + kfree(host_priv);
> +
> + return 0;
> +}
Should you also free the irq mapping here?
> --- /dev/null
> +++ b/drivers/ata/sata_fsl.h
> @@ -0,0 +1,92 @@
> +/*
> + * drivers/ata/sata_fsl.h
> + *
> + * Freescale 3.0Gbps SATA device driver
The header file is entirely pointless, since all definitions in here are
only used in a single file. Please merge the header into the
implementation.
> +static int sata_fsl_probe(struct of_device *ofdev,
> + const struct of_device_id *match); static int
> +sata_fsl_remove(struct of_device *ofdev); static int
> +sata_fsl_scr_read(struct ata_port *, unsigned int, u32 *); static int
> +sata_fsl_scr_write(struct ata_port *, unsigned int, u32); static
> +unsigned int sata_fsl_qc_issue(struct ata_queued_cmd *); static
> +irqreturn_t sata_fsl_interrupt(int, void *); static void
> +sata_fsl_irq_clear(struct ata_port *); static int
> +sata_fsl_port_start(struct ata_port *); static void
> +sata_fsl_port_stop(struct ata_port *); static void
> +sata_fsl_tf_read(struct ata_port *, struct ata_taskfile *); static
> +void sata_fsl_qc_prep(struct ata_queued_cmd *); static u8
> +sata_fsl_check_status(struct ata_port *); static void
> +sata_fsl_freeze(struct ata_port *); static void sata_fsl_thaw(struct
> +ata_port *); static void sata_fsl_error_handler(struct ata_port *);
> +static void sata_fsl_post_internal_cmd(struct ata_queued_cmd *);
> +
> +static inline unsigned int sata_fsl_tag(unsigned int, void __iomem
> +*);
In particular, get rid of the forward declarations for static functions.
All functions in a simple driver should be ordered in a way that you
always reference only code that was previously defined. This helps avoid
accidental recursions and makes it easier to review.
Arnd <><
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH v2] drivers/ata: add support to Freescale 3.0Gbps SATA Controller
On Monday 15 October 2007, Kalra Ashish-B00888 wrote: > Thanks for your comments and feedback. > > Actually, for PowerPC platforms iowrite32/ioread32 internally call > writel/readl, which are again mapped to out_le32/in_le32, This is correct on 6xx and e500 for now, but it's a little more complicated than that in general: iowriteXX/ioreadXX are either the simple readX/writeX wrappers from arch/powerpc/kernel/iomap.c, or the more complex functions from lib/iomap.c, depending on whether any of the active platforms has set CONFIG_PPC_INDIRECT_IO. writeX/readX can either map directly to out_leXX/in_leXX, or do something more complicated, again depending on the platform. In general, writeX/readX may need to consider PCI specific error handling or synchronization requirements, while out_leXX/in_leXX are low-level accessors that should only be used by device drivers when they are used on directly connected (non-PCI) devices. Since iowriteXX/ioreadXX is supposed to be the most generic variant covering both PCI MMIO (writeX/readX) and PCI PIO (outX/inX) transfers, it would make sense to extend them to also do SOC MMIO (out_leXX/in_leXX/out_beXX/in_beXX) and DCR (dcr_write/dcr_read) accesses in the future. Arnd <>< ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
