Re: [RFC PATCH 1/7] mmc: sdhci: add quirk for broken 3.0V support
On Fri, Jun 20, 2014 at 05:35:22PM +0800, Vincent Yang wrote:
> This patch defines a quirk for platforms unable
> to enable 3.0V support.
> It is a preparation and will be used by Fujitsu
> SDHCI controller f_sdh30 driver.
>
> Signed-off-by: Vincent Yang
I don't think you need this patch. Instead, you can exclude 3V using the
voltage-ranges = <> in the device tree.
Thanks,
Anton
> drivers/mmc/host/sdhci.c | 3 +++
> include/linux/mmc/sdhci.h | 2 ++
> 2 files changed, 5 insertions(+)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 47055f3..523075f 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -3069,6 +3069,9 @@ int sdhci_add_host(struct sdhci_host *host)
> }
> #endif /* CONFIG_REGULATOR */
>
> + if (host->quirks2 & SDHCI_QUIRK2_NO_3_0_V)
> + caps[0] &= ~SDHCI_CAN_VDD_300;
> +
> /*
>* According to SD Host Controller spec v3.00, if the Host System
>* can afford more than 150mA, Host Driver should set XPC to 1. Also
> diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h
> index 08abe99..cac0958 100644
> --- a/include/linux/mmc/sdhci.h
> +++ b/include/linux/mmc/sdhci.h
> @@ -98,6 +98,8 @@ struct sdhci_host {
> #define SDHCI_QUIRK2_BROKEN_HS200(1<<6)
> /* Controller does not support DDR50 */
> #define SDHCI_QUIRK2_BROKEN_DDR50(1<<7)
> +/* The system physically doesn't support 3.0v, even if the host does */
> +#define SDHCI_QUIRK2_NO_3_0_V(1<<8)
>
> int irq;/* Device IRQ */
> void __iomem *ioaddr; /* Mapped address */
> --
> 1.9.0
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/3 V3] mmc:sdhc: get voltage from sdhc host
On Mon, Aug 12, 2013 at 09:39:06AM +0800, Haijun Zhang wrote: > We use host->ocr_mask to hold the voltage get from device-tree > node, In case host->ocr_mask was available, we use host->ocr_mask > as the final available voltage can be used by MMC/SD/SDIO card. > > Signed-off-by: Haijun Zhang > --- Reviewed-by: Anton Vorontsov ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 3/3 V3] mmc:esdhc: add support to get voltage from device-tree
On Mon, Aug 12, 2013 at 09:39:04AM +0800, Haijun Zhang wrote: > Add suppport to get voltage from device-tree node for esdhc host, > if voltage-ranges was specified in device-tree node we can get > ocr_mask instead of read from host capacity register. If not voltages > still can be get from host capacity register. > > Signed-off-by: Haijun Zhang Acked-by: Anton Vorontsov ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH V3] mmc:of_spi: Update the code of getting voltage-ranges
On Mon, Aug 12, 2013 at 09:39:05AM +0800, Haijun Zhang wrote: > Using function mmc_of_parse_voltage() to get voltage-ranges. > > Signed-off-by: Haijun Zhang > --- Acked-by: Anton Vorontsov ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 1/3 V5] mmc:core: parse voltage from device-tree
On Wed, Aug 14, 2013 at 01:46:11PM +0800, Haijun Zhang wrote: > Add function to support get voltage from device-tree. > If there are voltage-range specified in device-tree node, this function > will parse it and return the available voltage mask. > > Signed-off-by: Haijun Zhang Acked-by: Anton Vorontsov Thanks a lot! Anton ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 1/3 V2] mmc:core: parse voltage from device-tree
On Wed, Jul 31, 2013 at 02:25:25PM +0800, Haijun Zhang wrote:
> Add function to support get voltage from device-tree.
> If there are voltage-range specified in device-tree node, this function
> will parse it and return the avail voltage mask.
>
> Signed-off-by: Haijun Zhang
> ---
> changes for v2:
> - Update the parameters of function
>
> drivers/mmc/core/core.c | 46 ++
> include/linux/mmc/core.h | 1 +
> 2 files changed, 47 insertions(+)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 49a5bca..ce9c957 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -27,6 +27,7 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -1196,6 +1197,51 @@ u32 mmc_vddrange_to_ocrmask(int vdd_min, int vdd_max)
> }
> EXPORT_SYMBOL(mmc_vddrange_to_ocrmask);
>
> +#ifdef CONFIG_OF
> +
> +/*
This is not kernel-doc formatted comment for the function.. it should
start with /**...
> + * mmc_of_parse_voltage - return mask of supported voltages
> + * @np: The device node need to be parsed.
> + *
> + * 1. Return zero: voltage-ranges unspecified in device-tree.
> + * 2. Return negative errno: voltage-range is invalid.
This doesn't seem right... the function returns the unsigned mask... You
can change the prototype of this func to something like this:
int mmc_of_parse_voltage(struct device_node *np, u32 *mask);
So the function will fill the mask and return 0 on success, and will
return negtive errno on errors.
> + * 3. Return ocr_mask: a mask of voltages that parse from device-tree
> + * node can be provided to MMC/SD/SDIO devices.
> + */
> +
No need for this empty line...
> +u32 mmc_of_parse_voltage(struct device_node *np)
> +{
> + const u32 *voltage_ranges;
> + int num_ranges, i;
> + u32 ocr_mask = 0;
> +
> + voltage_ranges = of_get_property(np, "voltage-ranges", &num_ranges);
> + num_ranges = num_ranges / sizeof(*voltage_ranges) / 2;
> + if (!voltage_ranges || !num_ranges) {
> + pr_info("%s: voltage-ranges unspecified\n", np->full_name);
> + return 0;
> + }
> +
> + for (i = 0; i < num_ranges; i++) {
> + const int j = i * 2;
> + u32 mask;
> +
> + mask = mmc_vddrange_to_ocrmask(be32_to_cpu(voltage_ranges[j]),
> + be32_to_cpu(voltage_ranges[j + 1]));
You lost some [pretty] formatting to line up the two arguments. :)
> + if (!mask) {
> + pr_err("%s: voltage-range #%d is invalid\n",
> + np->full_name, i);
> + return -EINVAL;
> + }
> + ocr_mask |= mask;
> + }
> +
> + return ocr_mask;
> +}
> +EXPORT_SYMBOL(mmc_of_parse_voltage);
> +
> +#endif /* CONFIG_OF */
> +
> #ifdef CONFIG_REGULATOR
>
> /**
> diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
> index 443243b..e3f8fe3 100644
> --- a/include/linux/mmc/core.h
> +++ b/include/linux/mmc/core.h
> @@ -209,5 +209,6 @@ static inline void mmc_claim_host(struct mmc_host *host)
> }
>
> extern u32 mmc_vddrange_to_ocrmask(int vdd_min, int vdd_max);
> +extern u32 mmc_of_parse_voltage(struct device_node *np);
You need to add a 'struct device_node;' forward-declaration, otherwise
non-OF code might fail to compile...
Thanks,
Anton
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] mmc:of_spi: Update the code of getting voltage-ranges
On Wed, Jul 31, 2013 at 02:25:27PM +0800, Haijun Zhang wrote:
> int num_ranges;
> + u32 ocr_mask;
> int i;
> int ret = -EINVAL;
>
> @@ -102,26 +103,11 @@ struct mmc_spi_platform_data *mmc_spi_get_pdata(struct
> spi_device *spi)
> if (!oms)
> return NULL;
>
> - voltage_ranges = of_get_property(np, "voltage-ranges", &num_ranges);
> - num_ranges = num_ranges / sizeof(*voltage_ranges) / 2;
> - if (!voltage_ranges || !num_ranges) {
> - dev_err(dev, "OF: voltage-ranges unspecified\n");
> + ocr_mask = mmc_of_parse_voltage(np);
> + if (ocr_mask <= 0)
'< 0' check for an unsigned type? :) I'd write just !ocr_mask...
But other than that the patch looks good to me...
Reviewed-by: Anton Vorontsov
Thanks!
> goto err_ocr;
> - }
> -
> - for (i = 0; i < num_ranges; i++) {
> - const int j = i * 2;
> - u32 mask;
>
> - mask = mmc_vddrange_to_ocrmask(be32_to_cpu(voltage_ranges[j]),
> -be32_to_cpu(voltage_ranges[j +
> 1]));
> - if (!mask) {
> - ret = -EINVAL;
> - dev_err(dev, "OF: voltage-range #%d is invalid\n", i);
> - goto err_ocr;
> - }
> - oms->pdata.ocr_mask |= mask;
> - }
> + oms->pdata.ocr_mask |= ocr_mask;
>
> for (i = 0; i < ARRAY_SIZE(oms->gpios); i++) {
> enum of_gpio_flags gpio_flags;
> --
> 1.8.0
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/2 V2] mmc: esdhc: get voltage from dts file
On Thu, Jul 25, 2013 at 08:38:11AM +0800, Haijun Zhang wrote: > Add voltage-range support in esdhc of T4, So we can choose > to read voltages from dts file as one optional. > If we can get a valid voltage-range from device node, we use > this voltage as the final voltage support. Else we still read > from capacity or from other provider. > > Signed-off-by: Haijun Zhang > Signed-off-by: Anton Vorontsov Development process nitpick... The code originated from me, but I did not sign off this patch... Per Documentation/SubmittingPatches: The Signed-off-by: tag indicates that the signer was involved in the development of the patch, or that he/she was in the patch's delivery path. The order of the sign-off lines also has a meaning. Putting my sign off below yours means that I was not only involved in the development of the patch but also somehow approved the patch (but I did not :). [..] > +void sdhci_get_voltage(struct platform_device *pdev) You still duplicate the code... Per my previous email, this should probably go into mmc/core (with the function renamed to something more generic, of course.) Thanks, Anton ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/2] mmc: esdhc: get voltage from dts file
On Mon, Jul 22, 2013 at 09:41:34PM -0500, Scott Wood wrote:
[...]
> >> > +static void esdhc_get_voltage(struct sdhci_host *host,
> >> > +struct platform_device *pdev)
> >> > +{
> >> > +}
> >>
> >> Don't duplicate this code. Move it somewhere common and share it.
> >[Haijun Wrote:] So, move it drivers/mmc/host/sdhci-pltfm.c and
> >share it as
> >Sdhc_get_voltage()?
>
> I'll let the MMC maintainer say what the appropriate place would
> be... Don't capitalize the function name, though. :-)
Somewhere in drivers/mmc/core/core.c, near mmc_vddrange_to_ocrmask() would
be most appropriate, IMO. #ifdef CONFIG_OF would be needed, though.
Thanks,
Anton
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 3/4 V2] mmc: esdhc: Add quirks to support T4240QDS board
On Tue, Jul 16, 2013 at 03:11:47AM +, Zhang Haijun-B42677 wrote: > Hi, Anton > > You mean get voltage support from DTS? > Or get voltage both from DTS and host capacity register? The logic might be that you first check device-tree, if it specifies voltage ranges, assume that DTS knows better, otherwise read capabilities from the register. Anton > Thanks. > > Regards > Haijun. > > > > -Original Message- > > From: linux-mmc-ow...@vger.kernel.org [mailto:linux-mmc- > > ow...@vger.kernel.org] On Behalf Of Anton Vorontsov > > Sent: Saturday, July 13, 2013 2:35 AM > > To: Wood Scott-B07421 > > Cc: Zhang Haijun-B42677; linux-...@vger.kernel.org; linuxppc- > > d...@lists.ozlabs.org; c...@laptop.org; Fleming Andy-AFLEMING; Wrobel > > Heinz-R39252 > > Subject: Re: [PATCH 3/4 V2] mmc: esdhc: Add quirks to support T4240QDS > > board > > > > On Mon, Jul 08, 2013 at 12:18:39PM -0500, Scott Wood wrote: > > > On 07/08/2013 02:16:04 AM, Haijun Zhang wrote: > > > >On T4240QDS board controllers has an unusable ADMA engine, so use > > > >SDMA instead. > > > >Also 3.0v is support on T4240QDS board even if the capacity detailed > > > >only 1.8v support. Without this quirk SD card will declare voltage > > > >not support and > > > > > > > >Signed-off-by: Haijun Zhang > > > > > > ...and what? > > > > > > Why is this board-specific? Isn't the controller part of the SoC, not > > > part of the board? If it really is board-specific, could you be more > > > detailed about why (ideally with an erratum number), and check the > > > toplevel board compatible (or get the information from platform > > > code) rather than modify the device tree? > > > > Yup, and if anything, I would recommend to reuse voltage-ranges property, > > it is already implemented for mmc spi driver, > > > > drivers/mmc/host/of_mmc_spi.c > > Documentation/devicetree/bindings/mmc/mmc-spi-slot.txt > > > > Thanks, > > > > Anton > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-mmc" in > > the body of a message to majord...@vger.kernel.org More majordomo info at > > http://vger.kernel.org/majordomo-info.html > ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 3/4 V2] mmc: esdhc: Add quirks to support T4240QDS board
On Mon, Jul 08, 2013 at 12:18:39PM -0500, Scott Wood wrote: > On 07/08/2013 02:16:04 AM, Haijun Zhang wrote: > >On T4240QDS board controllers has an unusable ADMA engine, so use > >SDMA instead. > >Also 3.0v is support on T4240QDS board even if the capacity > >detailed only 1.8v > >support. Without this quirk SD card will declare voltage not > >support and > > > >Signed-off-by: Haijun Zhang > > ...and what? > > Why is this board-specific? Isn't the controller part of the SoC, > not part of the board? If it really is board-specific, could you be > more detailed about why (ideally with an erratum number), and check > the toplevel board compatible (or get the information from platform > code) rather than modify the device tree? Yup, and if anything, I would recommend to reuse voltage-ranges property, it is already implemented for mmc spi driver, drivers/mmc/host/of_mmc_spi.c Documentation/devicetree/bindings/mmc/mmc-spi-slot.txt Thanks, Anton ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] powerpc/mpc85xx: fix non-bootcpu cannot up after hibernation resume
Hi! On Tue, May 14, 2013 at 08:59:13AM +, Wang Dongsheng-B40534 wrote: > I send to a wrong email address "Anton Vorontsov " > > Add Anton Vorontsov to this email. I don't have any means to test it, but the patch itself looks good and the description makes sense. So, Reviewed-by: Anton Vorontsov Thanks! > > Thanks all. > > > -Original Message- > > From: Wang Dongsheng-B40534 > > Sent: Tuesday, May 14, 2013 4:06 PM > > To: avoront...@ru.mvista.com > > Cc: pau...@samba.org; r...@sisk.pl; b...@kernel.crashing.org; > > johan...@sipsolutions.net; Wood Scott-B07421; Li Yang-R58472; Zhao > > Chenhui-B35336; linuxppc-dev@lists.ozlabs.org; Wang Dongsheng-B40534 > > Subject: [PATCH] powerpc/mpc85xx: fix non-bootcpu cannot up after > > hibernation resume > > > > This problem belongs to the core synchronization issues. > > The cpu1 already updated spin_table values, but bootcore cannot get > > this value in time. > > > > After bootcpu hibiernation restore the pages. we are now running > > with the kernel data of the old kernel fully restored. if we reset > > the non-bootcpus that will be reset cache(tlb), the non-bootcpus > > will get new address(map virtual and physical address spaces). > > but bootcpu tlb cache still use boot kernel data, so we need to > > invalidate the bootcpu tlb cache make it to get new main memory data. > > > > log: > > Enabling non-boot CPUs ... > > smp_85xx_kick_cpu: timeout waiting for core 1 to reset > > smp: failed starting cpu 1 (rc -2) > > Error taking CPU1 up: -2 > > > > Signed-off-by: Wang Dongsheng > > > > diff --git a/arch/powerpc/kernel/swsusp_booke.S > > b/arch/powerpc/kernel/swsusp_booke.S > > index 11a3930..9503249 100644 > > --- a/arch/powerpc/kernel/swsusp_booke.S > > +++ b/arch/powerpc/kernel/swsusp_booke.S > > @@ -141,6 +141,19 @@ _GLOBAL(swsusp_arch_resume) > > lis r11,swsusp_save_area@h > > ori r11,r11,swsusp_save_area@l > > > > + /* > > +* The boot core get a virtual address, when the boot process, > > +* the virtual address corresponds to a physical address. After > > +* hibernation resume memory snapshots, The corresponding > > +* relationship between the virtual memory and physical memory > > +* might change again. We need to get a new page table. So we > > +* need to invalidate TLB after resume pages. > > +* > > +* Invalidations TLB Using tlbilx/tlbivax/MMUCSR0. > > +* tlbilx used here. > > +*/ > > + bl _tlbil_all > > + > > lwz r4,SL_SPRG0(r11) > > mtsprg 0,r4 > > lwz r4,SL_SPRG1(r11) > > -- > > 1.8.0 > > ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] powerpc/esdhc: enable the card insert/remove interrupt
Hello Huang, On Fri, Oct 26, 2012 at 02:42:36AM +, Huang Changming-R66093 wrote: > For the current polling mode, driver will send CMD13 to poll the card status > periodically , which will cause too many interrupts. > Once I sent patches to detect the card when using polling mode last year: > read the state register, instead of send CMD13. But, these patches were not > accepted. Now I attach them for you. Was there any specific reason why the patches didn't get accepted? I very briefly looked at them, and they seem to be OK (there are a few cosmetic details I'd comment on, tho -- but please send them in a normal way (i.e. not as attachments). Thanks, Anton. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] powerpc/esdhc: enable the card insert/remove interrupt
On Thu, Oct 25, 2012 at 10:05:44AM +, Huang Changming-R66093 wrote:
> Hi, Anton.
> Could you have any comment about it?
> If not, I will resend this patch with v2.
Technically, the patch looks fine.
But again, as far as I recall, the card detection logic was broken on the
SOC level (it's actually very hard to break it on the board level -- it
would either work or not, but in the eSDHC case it was just unreliable,
again, if I recall everything correctly -- I might be wrong).
Of course you know the hardware much better, so your words weight more, so
you don't need my ack on the patch. :)
Although there's a second issue: for P4080DS and mpc837x boards you still
have the same problem with polling method, right? It is causing
performance drops/freezes every like 100 ms, and that's why you want to
avoid the polling.
So, you've "fixed" some boards, but left others to suffer. Ideally, the
best fix would be to also make the card polling cheap.
Anyways, using (d) clause of the "Reviewer's statement of oversight", I
can easily give this:
Reviewed-by: Anton Vorontsov
:)
Thanks!
[...]
> > > IIRC, the card detection is broken SOC-revision-wise, not board-wise.
> > > So the change seems wrong.
> > >
> > > Also, take a look at this:
> > >
> > > http://lkml.org/lkml/2010/7/14/127
> > >
> > > I started the work but never finished, unfortunately it caused some
> > > regressions for non-FSL hardware...
> > >
> > > > drivers/mmc/host/sdhci-of-esdhc.c |7 ++-
> > > > 1 file changed, 6 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/mmc/host/sdhci-of-esdhc.c
> > > > b/drivers/mmc/host/sdhci-of-esdhc.c
> > > > index ffc1226..5dc362f 100644
> > > > --- a/drivers/mmc/host/sdhci-of-esdhc.c
> > > > +++ b/drivers/mmc/host/sdhci-of-esdhc.c
> > > > @@ -196,6 +196,11 @@ static void esdhc_of_detect_limitation(struct
> > > platform_device *pdev,
> > > > if (vvn == VENDOR_V_22)
> > > > pdata->quirks2 |= SDHCI_QUIRK2_HOST_NO_CMD23;
> > > >
> > > > + /* P4080DS and MPC837XMDS board don't support interrupt mode */
> > > > + if (of_machine_is_compatible("fsl,mpc837xmds") ||
> > > > + of_machine_is_compatible("fsl,P4080DS"))
> > > > + pdata->quirks |= SDHCI_QUIRK_BROKEN_CARD_DETECTION;
> > > > +
> > > > iounmap(ioaddr);
> > > > end:
> > > > return;
> > > > @@ -223,7 +228,7 @@ static struct sdhci_pltfm_data sdhci_esdhc_pdata
> > > > =
> > > {
> > > > * card detection could be handled via GPIO
> > > > * eSDHC cannot support End Attribute in NOP ADMA descriptor
> > > > */
> > > > - .quirks = ESDHC_DEFAULT_QUIRKS |
> > > > SDHCI_QUIRK_BROKEN_CARD_DETECTION
> > > > + .quirks = ESDHC_DEFAULT_QUIRKS
> > > > | SDHCI_QUIRK_NO_CARD_NO_RESET
> > > > | SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC,
> > > > .ops = &sdhci_esdhc_ops,
> > > > --
> > > > 1.7.9.5
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] powerpc/esdhc: enable the card insert/remove interrupt
On Tue, Oct 23, 2012 at 03:01:17PM +0800, r66...@freescale.com wrote:
> From: Jerry Huang
>
> The current eSDHC driver use the poll mode to detect
> if the SD/MMC card is inserted or removed, which will generate
> many interrupts and impact the performance.
> Therefore, change the default card detect to interrupt mode,
> if the board can't support this mode, we still use the poll mode.
>
> Signed-off-by: Jerry Huang
> CC: Anton Vorontsov
> CC: Chris Ball
> ---
IIRC, the card detection is broken SOC-revision-wise, not board-wise. So
the change seems wrong.
Also, take a look at this:
http://lkml.org/lkml/2010/7/14/127
I started the work but never finished, unfortunately it caused some
regressions for non-FSL hardware...
> drivers/mmc/host/sdhci-of-esdhc.c |7 ++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci-of-esdhc.c
> b/drivers/mmc/host/sdhci-of-esdhc.c
> index ffc1226..5dc362f 100644
> --- a/drivers/mmc/host/sdhci-of-esdhc.c
> +++ b/drivers/mmc/host/sdhci-of-esdhc.c
> @@ -196,6 +196,11 @@ static void esdhc_of_detect_limitation(struct
> platform_device *pdev,
> if (vvn == VENDOR_V_22)
> pdata->quirks2 |= SDHCI_QUIRK2_HOST_NO_CMD23;
>
> + /* P4080DS and MPC837XMDS board don't support interrupt mode */
> + if (of_machine_is_compatible("fsl,mpc837xmds") ||
> + of_machine_is_compatible("fsl,P4080DS"))
> + pdata->quirks |= SDHCI_QUIRK_BROKEN_CARD_DETECTION;
> +
> iounmap(ioaddr);
> end:
> return;
> @@ -223,7 +228,7 @@ static struct sdhci_pltfm_data sdhci_esdhc_pdata = {
>* card detection could be handled via GPIO
>* eSDHC cannot support End Attribute in NOP ADMA descriptor
>*/
> - .quirks = ESDHC_DEFAULT_QUIRKS | SDHCI_QUIRK_BROKEN_CARD_DETECTION
> + .quirks = ESDHC_DEFAULT_QUIRKS
> | SDHCI_QUIRK_NO_CARD_NO_RESET
> | SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC,
> .ops = &sdhci_esdhc_ops,
> --
> 1.7.9.5
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/3] powerpc/esdhc: add property to disable the CMD23
On Wed, Sep 12, 2012 at 03:19:18AM +, Huang Changming-R66093 wrote: [...] > I don't think it is the best way to do it. For the VVN2.2 or older, > some silicon support this feature (mpc8536 and p2020), but other > silicones don't support it (e.g. p4080, p102x). Though, the current > p5/p4/p3 has supported this feature, can we sure the future silicon > support it? So I think the best way is to specify it in device tree > as 'sdhci,auto-cmd12' In addition to your current patches, you could just add another patch that blacklists affected SOC revisions based on the info from PVR/SVR. For example, see gfar_detect_errata() in drivers/net/ethernet/freescale/gianfar.c. That way you could help users that don't have the newest device trees. Thanks, Anton. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/3] powerpc/esdhc: add property to disable the CMD23
On Tue, Sep 11, 2012 at 12:54:29AM -0700, Anton Vorontsov wrote:
> On Tue, Sep 11, 2012 at 03:12:44PM +0800, chang-ming.hu...@freescale.com
> wrote:
> > From: Jerry Huang
> >
> > Below SOCs don't support the cmd23 command for MMC card,
> > therefore, disable it in device tree:
> > P1020, P1021, P1022, P1024, P1025 and P4080
> >
> > Signed-off-by: Jerry Huang
>
> Acked-by: Anton Vorontsov
Btw, although the patch is trivial, I guess you still want to let know
PowerPC folks about it. Adding Cc and copying the patch:
- - - -
From: Jerry Huang
Below SOCs don't support the cmd23 command for MMC card,
therefore, disable it in device tree:
P1020, P1021, P1022, P1024, P1025 and P4080
Signed-off-by: Jerry Huang
CC: Anton Vorontsov
---
arch/powerpc/boot/dts/fsl/p1020si-post.dtsi |1 +
arch/powerpc/boot/dts/fsl/p1021si-post.dtsi |1 +
arch/powerpc/boot/dts/fsl/p1022si-post.dtsi |1 +
arch/powerpc/boot/dts/fsl/p4080si-post.dtsi |1 +
4 files changed, 4 insertions(+)
diff --git a/arch/powerpc/boot/dts/fsl/p1020si-post.dtsi
b/arch/powerpc/boot/dts/fsl/p1020si-post.dtsi
index 68cc5e7..793a30b 100644
--- a/arch/powerpc/boot/dts/fsl/p1020si-post.dtsi
+++ b/arch/powerpc/boot/dts/fsl/p1020si-post.dtsi
@@ -154,6 +154,7 @@
sdhc@2e000 {
compatible = "fsl,p1020-esdhc", "fsl,esdhc";
sdhci,auto-cmd12;
+ sdhci,no-cmd23;
};
/include/ "pq3-sec3.3-0.dtsi"
diff --git a/arch/powerpc/boot/dts/fsl/p1021si-post.dtsi
b/arch/powerpc/boot/dts/fsl/p1021si-post.dtsi
index adb82fd..2b7fd2a 100644
--- a/arch/powerpc/boot/dts/fsl/p1021si-post.dtsi
+++ b/arch/powerpc/boot/dts/fsl/p1021si-post.dtsi
@@ -149,6 +149,7 @@
/include/ "pq3-esdhc-0.dtsi"
sdhc@2e000 {
sdhci,auto-cmd12;
+ sdhci,no-cmd23;
};
/include/ "pq3-sec3.3-0.dtsi"
diff --git a/arch/powerpc/boot/dts/fsl/p1022si-post.dtsi
b/arch/powerpc/boot/dts/fsl/p1022si-post.dtsi
index 06216b8..2334a52 100644
--- a/arch/powerpc/boot/dts/fsl/p1022si-post.dtsi
+++ b/arch/powerpc/boot/dts/fsl/p1022si-post.dtsi
@@ -215,6 +215,7 @@
sdhc@2e000 {
compatible = "fsl,p1022-esdhc", "fsl,esdhc";
sdhci,auto-cmd12;
+ sdhci,no-cmd23;
};
/include/ "pq3-sec3.3-0.dtsi"
diff --git a/arch/powerpc/boot/dts/fsl/p4080si-post.dtsi
b/arch/powerpc/boot/dts/fsl/p4080si-post.dtsi
index 8d35d2c..5b39952 100644
--- a/arch/powerpc/boot/dts/fsl/p4080si-post.dtsi
+++ b/arch/powerpc/boot/dts/fsl/p4080si-post.dtsi
@@ -337,6 +337,7 @@
sdhc@114000 {
voltage-ranges = <3300 3300>;
sdhci,auto-cmd12;
+ sdhci,no-cmd23;
};
/include/ "qoriq-i2c-0.dtsi"
--
1.7.9.5
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH] cpu: Document clear_tasks_mm_cpumask()
This patch adds more comments on clear_tasks_mm_cpumask, plus adds
a runtime check: the function is only suitable for offlined CPUs,
and if called inappropriately, the kernel should scream aloud.
Suggested-by: Andrew Morton
Suggested-by: Peter Zijlstra
Signed-off-by: Anton Vorontsov
---
On Tue, May 01, 2012 at 12:45:33PM +0200, Peter Zijlstra wrote:
> On Thu, 2012-04-26 at 16:59 -0700, Andrew Morton wrote:
> > > +void clear_tasks_mm_cpumask(int cpu)
> >
> > The operation of this function was presumably obvious to you at the
> > time you wrote it, but that isn't true of other people at later times.
> >
> > Please document it?
[...]
> > If someone tries to use this function for a different purpose, or
> > copies-and-modifies it for a different purpose, we just shot them in
> > the foot.
> >
> > They'd be pretty dumb to do that without reading the local comment,
> > but still...
>
> Methinks something simple like:
>
> WARN_ON(cpu_online(cpu));
>
> Ought to cure that worry, no? :-)
Yeah, this is all good ideas, thanks.
How about the following patch?
kernel/cpu.c | 18 ++
1 file changed, 18 insertions(+)
diff --git a/kernel/cpu.c b/kernel/cpu.c
index ecdf499..1bfa26f 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -13,6 +13,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -173,6 +174,18 @@ void __ref unregister_cpu_notifier(struct notifier_block
*nb)
}
EXPORT_SYMBOL(unregister_cpu_notifier);
+/**
+ * clear_tasks_mm_cpumask - Safely clear tasks' mm_cpumask for a CPU
+ * @cpu: a CPU id
+ *
+ * This function walks up all processes, finds a valid mm struct for
+ * each one and then clears a corresponding bit in mm's cpumask. While
+ * this all sounds trivial, there are various non-obvious corner cases,
+ * which this function tries to solve in a safe manner.
+ *
+ * Also note that the function uses a somewhat relaxed locking scheme,
+ * so it may be called only for an already offlined CPU.
+ */
void clear_tasks_mm_cpumask(int cpu)
{
struct task_struct *p;
@@ -184,10 +197,15 @@ void clear_tasks_mm_cpumask(int cpu)
* Thus, we may use rcu_read_lock() here, instead of grabbing
* full-fledged tasklist_lock.
*/
+ WARN_ON(cpu_online(cpu));
rcu_read_lock();
for_each_process(p) {
struct task_struct *t;
+ /*
+* Main thread might exit, but other threads may still have
+* a valid mm. Find one.
+*/
t = find_lock_task_mm(p);
if (!t)
continue;
--
1.7.9.2
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 1/9] cpu: Introduce clear_tasks_mm_cpumask() helper
On Thu, Apr 26, 2012 at 04:59:11PM -0700, Andrew Morton wrote:
[...]
> > so its not like new tasks will ever get this cpu set in
> > +* their mm mask. -- Peter Zijlstra
> > +* Thus, we may use rcu_read_lock() here, instead of grabbing
> > +* full-fledged tasklist_lock.
> > +*/
> > + rcu_read_lock();
> > + for_each_process(p) {
> > + struct task_struct *t;
> > +
> > + t = find_lock_task_mm(p);
> > + if (!t)
> > + continue;
> > + cpumask_clear_cpu(cpu, mm_cpumask(t->mm));
> > + task_unlock(t);
> > + }
> > + rcu_read_unlock();
> > +}
>
> It is good that this code exists under CONFIG_HOTPLUG_CPU. Did you
> check that everything works correctly with CONFIG_HOTPLUG_CPU=n?
Yeah, only the code under CONFIG_HOTPLUG_CPU calls the function, so
it should be all fine.
Thanks!
--
Anton Vorontsov
Email: cbouatmai...@gmail.com
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 7/9] um: Should hold tasklist_lock while traversing processes
On Mon, Apr 23, 2012 at 04:57:54PM +0200, Richard Weinberger wrote: > On 23.04.2012 09:09, Anton Vorontsov wrote: > >Traversing the tasks requires holding tasklist_lock, otherwise it > >is unsafe. > > > >p.s. However, I'm not sure that calling os_kill_ptraced_process() > >in the atomic context is correct. It seem to work, but please > >take a closer look. > > > >Signed-off-by: Anton Vorontsov > >--- > > You forgot my Ack and I've already explained why > os_kill_ptraced_process() is fine. Ouch, sorry! -- Anton Vorontsov Email: cbouatmai...@gmail.com ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH 9/9] um: Properly check all process' threads for a live mm
kill_off_processes() might miss a valid process, this is because
checking for process->mm is not enough. Process' main thread may
exit or detach its mm via use_mm(), but other threads may still
have a valid mm.
To catch this we use find_lock_task_mm(), which walks up all
threads and returns an appropriate task (with task lock held).
Suggested-by: Oleg Nesterov
Signed-off-by: Anton Vorontsov
---
arch/um/kernel/reboot.c | 13 +++--
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/arch/um/kernel/reboot.c b/arch/um/kernel/reboot.c
index 1411f4e..3d15243 100644
--- a/arch/um/kernel/reboot.c
+++ b/arch/um/kernel/reboot.c
@@ -6,6 +6,7 @@
#include "linux/sched.h"
#include "linux/spinlock.h"
#include "linux/slab.h"
+#include "linux/oom.h"
#include "kern_util.h"
#include "os.h"
#include "skas.h"
@@ -25,13 +26,13 @@ static void kill_off_processes(void)
read_lock(&tasklist_lock);
for_each_process(p) {
- task_lock(p);
- if (!p->mm) {
- task_unlock(p);
+ struct task_struct *t;
+
+ t = find_lock_task_mm(p);
+ if (!t)
continue;
- }
- pid = p->mm->context.id.u.pid;
- task_unlock(p);
+ pid = t->mm->context.id.u.pid;
+ task_unlock(t);
os_kill_ptraced_process(pid, 1);
}
read_unlock(&tasklist_lock);
--
1.7.9.2
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH 8/9] um: Fix possible race on task->mm
Checking for task->mm is dangerous as ->mm might disappear (exit_mm()
assigns NULL under task_lock(), so tasklist lock is not enough).
We can't use get_task_mm()/mmput() pair as mmput() might sleep,
so let's take the task lock while we care about its mm.
Note that we should also use find_lock_task_mm() to check all process'
threads for a valid mm, but for uml we'll do it in a separate patch.
Signed-off-by: Anton Vorontsov
---
arch/um/kernel/reboot.c |7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/arch/um/kernel/reboot.c b/arch/um/kernel/reboot.c
index 66d754c..1411f4e 100644
--- a/arch/um/kernel/reboot.c
+++ b/arch/um/kernel/reboot.c
@@ -25,10 +25,13 @@ static void kill_off_processes(void)
read_lock(&tasklist_lock);
for_each_process(p) {
- if (p->mm == NULL)
+ task_lock(p);
+ if (!p->mm) {
+ task_unlock(p);
continue;
-
+ }
pid = p->mm->context.id.u.pid;
+ task_unlock(p);
os_kill_ptraced_process(pid, 1);
}
read_unlock(&tasklist_lock);
--
1.7.9.2
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH 7/9] um: Should hold tasklist_lock while traversing processes
Traversing the tasks requires holding tasklist_lock, otherwise it
is unsafe.
p.s. However, I'm not sure that calling os_kill_ptraced_process()
in the atomic context is correct. It seem to work, but please
take a closer look.
Signed-off-by: Anton Vorontsov
---
arch/um/kernel/reboot.c |3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/um/kernel/reboot.c b/arch/um/kernel/reboot.c
index 4d93dff..66d754c 100644
--- a/arch/um/kernel/reboot.c
+++ b/arch/um/kernel/reboot.c
@@ -4,6 +4,7 @@
*/
#include "linux/sched.h"
+#include "linux/spinlock.h"
#include "linux/slab.h"
#include "kern_util.h"
#include "os.h"
@@ -22,6 +23,7 @@ static void kill_off_processes(void)
struct task_struct *p;
int pid;
+ read_lock(&tasklist_lock);
for_each_process(p) {
if (p->mm == NULL)
continue;
@@ -29,6 +31,7 @@ static void kill_off_processes(void)
pid = p->mm->context.id.u.pid;
os_kill_ptraced_process(pid, 1);
}
+ read_unlock(&tasklist_lock);
}
}
--
1.7.9.2
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH 6/9] blackfin: Fix possible deadlock in decode_address()
Oleg Nesterov found an interesting deadlock possibility:
> sysrq_showregs_othercpus() does smp_call_function(showacpu)
> and showacpu() show_stack()->decode_address(). Now suppose that IPI
> interrupts the task holding read_lock(tasklist).
To fix this, blackfin should not grab the write_ variant of the
tasklist lock, read_ one is enough.
Suggested-by: Oleg Nesterov
Signed-off-by: Anton Vorontsov
---
arch/blackfin/kernel/trace.c |6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/blackfin/kernel/trace.c b/arch/blackfin/kernel/trace.c
index d08f0e3..f7f7a18 100644
--- a/arch/blackfin/kernel/trace.c
+++ b/arch/blackfin/kernel/trace.c
@@ -29,7 +29,7 @@ void decode_address(char *buf, unsigned long address)
{
struct task_struct *p;
struct mm_struct *mm;
- unsigned long flags, offset;
+ unsigned long offset;
struct rb_node *n;
#ifdef CONFIG_KALLSYMS
@@ -113,7 +113,7 @@ void decode_address(char *buf, unsigned long address)
* mappings of all our processes and see if we can't be a whee
* bit more specific
*/
- write_lock_irqsave(&tasklist_lock, flags);
+ read_lock(&tasklist_lock);
for_each_process(p) {
struct task_struct *t;
@@ -186,7 +186,7 @@ __continue:
sprintf(buf, "/* kernel dynamic memory */");
done:
- write_unlock_irqrestore(&tasklist_lock, flags);
+ read_unlock(&tasklist_lock);
}
#define EXPAND_LEN ((1 << CONFIG_DEBUG_BFIN_HWTRACE_EXPAND_LEN) * 256 - 1)
--
1.7.9.2
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH 5/9] blackfin: A couple of task->mm handling fixes
The patch fixes two problems:
1. Working with task->mm w/o getting mm or grabing the task lock is
dangerous as ->mm might disappear (exit_mm() assigns NULL under
task_lock(), so tasklist lock is not enough).
We can't use get_task_mm()/mmput() pair as mmput() might sleep,
so we have to take the task lock while handle its mm.
2. Checking for process->mm is not enough because process' main
thread may exit or detach its mm via use_mm(), but other threads
may still have a valid mm.
To catch this we use find_lock_task_mm(), which walks up all
threads and returns an appropriate task (with task lock held).
Suggested-by: Oleg Nesterov
Signed-off-by: Anton Vorontsov
---
arch/blackfin/kernel/trace.c | 26 +-
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/arch/blackfin/kernel/trace.c b/arch/blackfin/kernel/trace.c
index 44bbf2f..d08f0e3 100644
--- a/arch/blackfin/kernel/trace.c
+++ b/arch/blackfin/kernel/trace.c
@@ -10,6 +10,8 @@
#include
#include
#include
+#include
+#include
#include
#include
#include
@@ -28,7 +30,6 @@ void decode_address(char *buf, unsigned long address)
struct task_struct *p;
struct mm_struct *mm;
unsigned long flags, offset;
- unsigned char in_atomic = (bfin_read_IPEND() & 0x10) || in_atomic();
struct rb_node *n;
#ifdef CONFIG_KALLSYMS
@@ -114,15 +115,15 @@ void decode_address(char *buf, unsigned long address)
*/
write_lock_irqsave(&tasklist_lock, flags);
for_each_process(p) {
- mm = (in_atomic ? p->mm : get_task_mm(p));
- if (!mm)
- continue;
+ struct task_struct *t;
- if (!down_read_trylock(&mm->mmap_sem)) {
- if (!in_atomic)
- mmput(mm);
+ t = find_lock_task_mm(p);
+ if (!t)
continue;
- }
+
+ mm = t->mm;
+ if (!down_read_trylock(&mm->mmap_sem))
+ goto __continue;
for (n = rb_first(&mm->mm_rb); n; n = rb_next(n)) {
struct vm_area_struct *vma;
@@ -131,7 +132,7 @@ void decode_address(char *buf, unsigned long address)
if (address >= vma->vm_start && address < vma->vm_end) {
char _tmpbuf[256];
- char *name = p->comm;
+ char *name = t->comm;
struct file *file = vma->vm_file;
if (file) {
@@ -164,8 +165,7 @@ void decode_address(char *buf, unsigned long address)
name, vma->vm_start,
vma->vm_end);
up_read(&mm->mmap_sem);
- if (!in_atomic)
- mmput(mm);
+ task_unlock(t);
if (buf[0] == '\0')
sprintf(buf, "[ %s ] dynamic memory",
name);
@@ -175,8 +175,8 @@ void decode_address(char *buf, unsigned long address)
}
up_read(&mm->mmap_sem);
- if (!in_atomic)
- mmput(mm);
+__continue:
+ task_unlock(t);
}
/*
--
1.7.9.2
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH 4/9] sh: Use clear_tasks_mm_cpumask()
Checking for process->mm is not enough because process' main
thread may exit or detach its mm via use_mm(), but other threads
may still have a valid mm.
To fix this we would need to use find_lock_task_mm(), which would
walk up all threads and returns an appropriate task (with task
lock held).
clear_tasks_mm_cpumask() has the issue fixed, so let's use it.
Suggested-by: Oleg Nesterov
Signed-off-by: Anton Vorontsov
---
arch/sh/kernel/smp.c |7 +--
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/arch/sh/kernel/smp.c b/arch/sh/kernel/smp.c
index eaebdf6..4664f76 100644
--- a/arch/sh/kernel/smp.c
+++ b/arch/sh/kernel/smp.c
@@ -123,7 +123,6 @@ void native_play_dead(void)
int __cpu_disable(void)
{
unsigned int cpu = smp_processor_id();
- struct task_struct *p;
int ret;
ret = mp_ops->cpu_disable(cpu);
@@ -153,11 +152,7 @@ int __cpu_disable(void)
flush_cache_all();
local_flush_tlb_all();
- read_lock(&tasklist_lock);
- for_each_process(p)
- if (p->mm)
- cpumask_clear_cpu(cpu, mm_cpumask(p->mm));
- read_unlock(&tasklist_lock);
+ clear_tasks_mm_cpumask(cpu);
return 0;
}
--
1.7.9.2
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH 3/9] powerpc: Use clear_tasks_mm_cpumask()
Current CPU hotplug code has some task->mm handling issues:
1. Working with task->mm w/o getting mm or grabing the task lock is
dangerous as ->mm might disappear (exit_mm() assigns NULL under
task_lock(), so tasklist lock is not enough).
We can't use get_task_mm()/mmput() pair as mmput() might sleep,
so we must take the task lock while handle its mm.
2. Checking for process->mm is not enough because process' main
thread may exit or detach its mm via use_mm(), but other threads
may still have a valid mm.
To fix this we would need to use find_lock_task_mm(), which would
walk up all threads and returns an appropriate task (with task
lock held).
clear_tasks_mm_cpumask() has all the issues fixed, so let's use it.
Suggested-by: Oleg Nesterov
Signed-off-by: Anton Vorontsov
---
arch/powerpc/mm/mmu_context_nohash.c | 11 ++-
1 file changed, 2 insertions(+), 9 deletions(-)
diff --git a/arch/powerpc/mm/mmu_context_nohash.c
b/arch/powerpc/mm/mmu_context_nohash.c
index 5b63bd3..e779642 100644
--- a/arch/powerpc/mm/mmu_context_nohash.c
+++ b/arch/powerpc/mm/mmu_context_nohash.c
@@ -333,9 +333,7 @@ static int __cpuinit mmu_context_cpu_notify(struct
notifier_block *self,
unsigned long action, void *hcpu)
{
unsigned int cpu = (unsigned int)(long)hcpu;
-#ifdef CONFIG_HOTPLUG_CPU
- struct task_struct *p;
-#endif
+
/* We don't touch CPU 0 map, it's allocated at aboot and kept
* around forever
*/
@@ -358,12 +356,7 @@ static int __cpuinit mmu_context_cpu_notify(struct
notifier_block *self,
stale_map[cpu] = NULL;
/* We also clear the cpu_vm_mask bits of CPUs going away */
- read_lock(&tasklist_lock);
- for_each_process(p) {
- if (p->mm)
- cpumask_clear_cpu(cpu, mm_cpumask(p->mm));
- }
- read_unlock(&tasklist_lock);
+ clear_tasks_mm_cpumask(cpu);
break;
#endif /* CONFIG_HOTPLUG_CPU */
}
--
1.7.9.2
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH 2/9] arm: Use clear_tasks_mm_cpumask()
Checking for process->mm is not enough because process' main
thread may exit or detach its mm via use_mm(), but other threads
may still have a valid mm.
To fix this we would need to use find_lock_task_mm(), which would
walk up all threads and returns an appropriate task (with task
lock held).
clear_tasks_mm_cpumask() has this issue fixed, so let's use it.
Suggested-by: Oleg Nesterov
Signed-off-by: Anton Vorontsov
---
arch/arm/kernel/smp.c |8 +---
1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index addbbe8..e824ee4 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -130,7 +130,6 @@ static void percpu_timer_stop(void);
int __cpu_disable(void)
{
unsigned int cpu = smp_processor_id();
- struct task_struct *p;
int ret;
ret = platform_cpu_disable(cpu);
@@ -160,12 +159,7 @@ int __cpu_disable(void)
flush_cache_all();
local_flush_tlb_all();
- read_lock(&tasklist_lock);
- for_each_process(p) {
- if (p->mm)
- cpumask_clear_cpu(cpu, mm_cpumask(p->mm));
- }
- read_unlock(&tasklist_lock);
+ clear_tasks_mm_cpumask(cpu);
return 0;
}
--
1.7.9.2
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH 1/9] cpu: Introduce clear_tasks_mm_cpumask() helper
Many architectures clear tasks' mm_cpumask like this:
read_lock(&tasklist_lock);
for_each_process(p) {
if (p->mm)
cpumask_clear_cpu(cpu, mm_cpumask(p->mm));
}
read_unlock(&tasklist_lock);
Depending on the context, the code above may have several problems,
such as:
1. Working with task->mm w/o getting mm or grabing the task lock is
dangerous as ->mm might disappear (exit_mm() assigns NULL under
task_lock(), so tasklist lock is not enough).
2. Checking for process->mm is not enough because process' main
thread may exit or detach its mm via use_mm(), but other threads
may still have a valid mm.
This patch implements a small helper function that does things
correctly, i.e.:
1. We take the task's lock while whe handle its mm (we can't use
get_task_mm()/mmput() pair as mmput() might sleep);
2. To catch exited main thread case, we use find_lock_task_mm(),
which walks up all threads and returns an appropriate task
(with task lock held).
Also, Per Peter Zijlstra's idea, now we don't grab tasklist_lock in
the new helper, instead we take the rcu read lock. We can do this
because the function is called after the cpu is taken down and marked
offline, so no new tasks will get this cpu set in their mm mask.
Signed-off-by: Anton Vorontsov
---
include/linux/cpu.h |1 +
kernel/cpu.c| 26 ++
2 files changed, 27 insertions(+)
diff --git a/include/linux/cpu.h b/include/linux/cpu.h
index ee28844..d2ca49f 100644
--- a/include/linux/cpu.h
+++ b/include/linux/cpu.h
@@ -179,6 +179,7 @@ extern void put_online_cpus(void);
#define hotcpu_notifier(fn, pri) cpu_notifier(fn, pri)
#define register_hotcpu_notifier(nb) register_cpu_notifier(nb)
#define unregister_hotcpu_notifier(nb) unregister_cpu_notifier(nb)
+void clear_tasks_mm_cpumask(int cpu);
int cpu_down(unsigned int cpu);
#ifdef CONFIG_ARCH_CPU_PROBE_RELEASE
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 2060c6e..ecdf499 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -10,6 +10,8 @@
#include
#include
#include
+#include
+#include
#include
#include
#include
@@ -171,6 +173,30 @@ void __ref unregister_cpu_notifier(struct notifier_block
*nb)
}
EXPORT_SYMBOL(unregister_cpu_notifier);
+void clear_tasks_mm_cpumask(int cpu)
+{
+ struct task_struct *p;
+
+ /*
+* This function is called after the cpu is taken down and marked
+* offline, so its not like new tasks will ever get this cpu set in
+* their mm mask. -- Peter Zijlstra
+* Thus, we may use rcu_read_lock() here, instead of grabbing
+* full-fledged tasklist_lock.
+*/
+ rcu_read_lock();
+ for_each_process(p) {
+ struct task_struct *t;
+
+ t = find_lock_task_mm(p);
+ if (!t)
+ continue;
+ cpumask_clear_cpu(cpu, mm_cpumask(t->mm));
+ task_unlock(t);
+ }
+ rcu_read_unlock();
+}
+
static inline void check_for_tasks(int cpu)
{
struct task_struct *p;
--
1.7.9.2
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH v3 0/9] Fixes for common mistakes w/ for_each_process and task->mm
Hi all, This is another resend of several task->mm fixes, the bugs I found during LMK code audit. Architectures were traverse the tasklist in an unsafe manner, plus there are a few cases of unsafe access to task->mm in general. There were no objections on the previous resend, and the final words were somewhere along "the patches are fine" line. In v3: - Dropped a controversal 'Make find_lock_task_mm() sparse-aware' patch; - Reword arm and sh commit messages, per Oleg Nesterov's suggestions; - Added an optimization trick in clear_tasks_mm_cpumask(): take only the rcu read lock, no need for the whole tasklist_lock. Suggested by Peter Zijlstra. In v2: - introduced a small helper in cpu.c: most arches duplicate the same [buggy] code snippet, so it's better to fix it and move the logic into a common function. Thanks, -- Anton Vorontsov Email: cbouatmai...@gmail.com ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 07/10] um: Should hold tasklist_lock while traversing processes
On Sat, Mar 24, 2012 at 01:48:23PM +0100, Peter Zijlstra wrote: > On Sat, 2012-03-24 at 14:30 +0400, Anton Vorontsov wrote: > > Traversing the tasks requires holding tasklist_lock, otherwise it > > is unsafe. > > No it doesn't, it either requires tasklist_lock or rcu_read_lock(). Well, currently the code does neither. :-) -- Anton Vorontsov Email: cbouatmai...@gmail.com ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH v2.1 01/10] cpu: Introduce clear_tasks_mm_cpumask() helper
Many architctures clear tasks' mm_cpumask like this:
read_lock(&tasklist_lock);
for_each_process(p) {
if (p->mm)
cpumask_clear_cpu(cpu, mm_cpumask(p->mm));
}
read_unlock(&tasklist_lock);
The code above has several problems, such as:
1. Working with task->mm w/o getting mm or grabing the task lock is
dangerous as ->mm might disappear (exit_mm() assigns NULL under
task_lock(), so tasklist lock is not enough).
2. Checking for process->mm is not enough because process' main
thread may exit or detach its mm via use_mm(), but other threads
may still have a valid mm.
This patch implements a small helper function that does things
correctly, i.e.:
1. We take the task's lock while whe handle its mm (we can't use
get_task_mm()/mmput() pair as mmput() might sleep);
2. To catch exited main thread case, we use find_lock_task_mm(),
which walks up all threads and returns an appropriate task
(with task lock held).
Also, Per Peter Zijlstra's idea, now we don't grab tasklist_lock in
the new helper, instead we take the rcu read lock. We can do this
because the function is called after the cpu is taken down and marked
offline, so no new tasks will get this cpu set in their mm mask.
Signed-off-by: Anton Vorontsov
---
On Sat, Mar 24, 2012 at 01:43:41PM +0100, Peter Zijlstra wrote:
> On Sat, 2012-03-24 at 14:27 +0400, Anton Vorontsov wrote:
> > + read_unlock(&tasklist_lock);
> > +}
>
> Why bother with the tasklist_lock at all anymore, afaict you could use
> rcu_read_lock() here. This all is called after the cpu is taken down and
> marked offline, so its not like new tasks will ever get this cpu set in
> their mm mask.
I guess you're right. Well, this would make the code a bit fragile,
but a comment might help.
How about this version?
include/linux/cpu.h |1 +
kernel/cpu.c| 26 ++
2 files changed, 27 insertions(+)
diff --git a/include/linux/cpu.h b/include/linux/cpu.h
index 1f65875..941e865 100644
--- a/include/linux/cpu.h
+++ b/include/linux/cpu.h
@@ -171,6 +171,7 @@ extern void put_online_cpus(void);
#define hotcpu_notifier(fn, pri) cpu_notifier(fn, pri)
#define register_hotcpu_notifier(nb) register_cpu_notifier(nb)
#define unregister_hotcpu_notifier(nb) unregister_cpu_notifier(nb)
+void clear_tasks_mm_cpumask(int cpu);
int cpu_down(unsigned int cpu);
#ifdef CONFIG_ARCH_CPU_PROBE_RELEASE
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 2060c6e..ecdf499 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -10,6 +10,8 @@
#include
#include
#include
+#include
+#include
#include
#include
#include
@@ -171,6 +173,30 @@ void __ref unregister_cpu_notifier(struct notifier_block
*nb)
}
EXPORT_SYMBOL(unregister_cpu_notifier);
+void clear_tasks_mm_cpumask(int cpu)
+{
+ struct task_struct *p;
+
+ /*
+* This function is called after the cpu is taken down and marked
+* offline, so its not like new tasks will ever get this cpu set in
+* their mm mask. -- Peter Zijlstra
+* Thus, we may use rcu_read_lock() here, instead of grabbing
+* full-fledged tasklist_lock.
+*/
+ rcu_read_lock();
+ for_each_process(p) {
+ struct task_struct *t;
+
+ t = find_lock_task_mm(p);
+ if (!t)
+ continue;
+ cpumask_clear_cpu(cpu, mm_cpumask(t->mm));
+ task_unlock(t);
+ }
+ rcu_read_unlock();
+}
+
static inline void check_for_tasks(int cpu)
{
struct task_struct *p;
--
1.7.9.2
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 10/10] oom: Make find_lock_task_mm() sparse-aware
On Sat, Mar 24, 2012 at 01:52:54PM +0100, Peter Zijlstra wrote: [...] > > p.s. I know Peter Zijlstra detest the __cond_lock() stuff, but untill > > we have anything better in sparse, let's use it. This particular > > patch helped me to detect one bug that I myself made during > > task->mm fixup series. So, it is useful. > > Yeah, so Nacked-by: Peter Zijlstra > > Also, why didn't lockdep catch it? Because patch authors test their patches on architectures they own (well, sometimes I do check patches on exotic architectures w/ qemu, but it is less convenient than just build/sparse-test the patch w/ a cross compiler). And since lockdep is a runtime checker, it is not very useful. Sparse is a build-time checker, so it is even better in the sense that it is able to catch bugs even in code that is executed rarely. > Fix sparse already instead of smearing ugly all over. Just wonder how do you see the feature implemented? Something like this? #define __ret_cond_locked(l, c) __attribute__((ret_cond_locked(l, c))) #define __ret_value __attribute__((ret_value)) #define __ret_locked_nonnull(l) __ret_cond_locked(l, __ret_value); extern struct task_struct *find_lock_task_mm(struct task_struct *p) __ret_locked_nonnull(&__ret_value->alloc_lock); Thanks, -- Anton Vorontsov Email: cbouatmai...@gmail.com ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH 10/10] oom: Make find_lock_task_mm() sparse-aware
This is needed so that callers would not get 'context imbalance'
warnings from the sparse tool.
As a side effect, this patch fixes the following sparse warnings:
CHECK mm/oom_kill.c
mm/oom_kill.c:201:28: warning: context imbalance in 'oom_badness' -
unexpected unlock
include/linux/rcupdate.h:249:30: warning: context imbalance in
'dump_tasks' - unexpected unlock
mm/oom_kill.c:453:9: warning: context imbalance in 'oom_kill_task' -
unexpected unlock
CHECK mm/memcontrol.c
...
mm/memcontrol.c:1130:17: warning: context imbalance in
'task_in_mem_cgroup' - unexpected unlock
p.s. I know Peter Zijlstra detest the __cond_lock() stuff, but untill
we have anything better in sparse, let's use it. This particular
patch helped me to detect one bug that I myself made during
task->mm fixup series. So, it is useful.
Signed-off-by: Anton Vorontsov
Acked-by: KOSAKI Motohiro
---
include/linux/oom.h | 12 +++-
mm/oom_kill.c |2 +-
2 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/include/linux/oom.h b/include/linux/oom.h
index 552fba9..26cf628 100644
--- a/include/linux/oom.h
+++ b/include/linux/oom.h
@@ -21,6 +21,7 @@
#ifdef __KERNEL__
+#include
#include
#include
#include
@@ -65,7 +66,16 @@ static inline void oom_killer_enable(void)
oom_killer_disabled = false;
}
-extern struct task_struct *find_lock_task_mm(struct task_struct *p);
+extern struct task_struct *__find_lock_task_mm(struct task_struct *p);
+
+static inline struct task_struct *find_lock_task_mm(struct task_struct *p)
+{
+ struct task_struct *ret;
+
+ ret = __find_lock_task_mm(p);
+ (void)__cond_lock(&ret->alloc_lock, ret);
+ return ret;
+}
/* sysctls */
extern int sysctl_oom_dump_tasks;
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 2958fd8..0ebb383 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -136,7 +136,7 @@ static bool has_intersects_mems_allowed(struct task_struct
*tsk,
* pointer. Return p, or any of its subthreads with a valid ->mm, with
* task_lock() held.
*/
-struct task_struct *find_lock_task_mm(struct task_struct *p)
+struct task_struct *__find_lock_task_mm(struct task_struct *p)
{
struct task_struct *t = p;
--
1.7.9.2
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH 09/10] um: Properly check all process' threads for a live mm
kill_off_processes() might miss a valid process, this is because
checking for process->mm is not enough. Process' main thread may
exit or detach its mm via use_mm(), but other threads may still
have a valid mm.
To catch this we use find_lock_task_mm(), which walks up all
threads and returns an appropriate task (with task lock held).
Suggested-by: Oleg Nesterov
Signed-off-by: Anton Vorontsov
---
arch/um/kernel/reboot.c | 13 +++--
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/arch/um/kernel/reboot.c b/arch/um/kernel/reboot.c
index 1411f4e..3d15243 100644
--- a/arch/um/kernel/reboot.c
+++ b/arch/um/kernel/reboot.c
@@ -6,6 +6,7 @@
#include "linux/sched.h"
#include "linux/spinlock.h"
#include "linux/slab.h"
+#include "linux/oom.h"
#include "kern_util.h"
#include "os.h"
#include "skas.h"
@@ -25,13 +26,13 @@ static void kill_off_processes(void)
read_lock(&tasklist_lock);
for_each_process(p) {
- task_lock(p);
- if (!p->mm) {
- task_unlock(p);
+ struct task_struct *t;
+
+ t = find_lock_task_mm(p);
+ if (!t)
continue;
- }
- pid = p->mm->context.id.u.pid;
- task_unlock(p);
+ pid = t->mm->context.id.u.pid;
+ task_unlock(t);
os_kill_ptraced_process(pid, 1);
}
read_unlock(&tasklist_lock);
--
1.7.9.2
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH 08/10] um: Fix possible race on task->mm
Checking for task->mm is dangerous as ->mm might disappear (exit_mm()
assigns NULL under task_lock(), so tasklist lock is not enough).
We can't use get_task_mm()/mmput() pair as mmput() might sleep,
so let's take the task lock while we care about its mm.
Note that we should also use find_lock_task_mm() to check all process'
threads for a valid mm, but for uml we'll do it in a separate patch.
Signed-off-by: Anton Vorontsov
---
arch/um/kernel/reboot.c |7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/arch/um/kernel/reboot.c b/arch/um/kernel/reboot.c
index 66d754c..1411f4e 100644
--- a/arch/um/kernel/reboot.c
+++ b/arch/um/kernel/reboot.c
@@ -25,10 +25,13 @@ static void kill_off_processes(void)
read_lock(&tasklist_lock);
for_each_process(p) {
- if (p->mm == NULL)
+ task_lock(p);
+ if (!p->mm) {
+ task_unlock(p);
continue;
-
+ }
pid = p->mm->context.id.u.pid;
+ task_unlock(p);
os_kill_ptraced_process(pid, 1);
}
read_unlock(&tasklist_lock);
--
1.7.9.2
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH 07/10] um: Should hold tasklist_lock while traversing processes
Traversing the tasks requires holding tasklist_lock, otherwise it
is unsafe.
p.s. However, I'm not sure that calling os_kill_ptraced_process()
in the atomic context is correct. It seem to work, but please
take a closer look.
Signed-off-by: Anton Vorontsov
---
arch/um/kernel/reboot.c |3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/um/kernel/reboot.c b/arch/um/kernel/reboot.c
index 4d93dff..66d754c 100644
--- a/arch/um/kernel/reboot.c
+++ b/arch/um/kernel/reboot.c
@@ -4,6 +4,7 @@
*/
#include "linux/sched.h"
+#include "linux/spinlock.h"
#include "linux/slab.h"
#include "kern_util.h"
#include "os.h"
@@ -22,6 +23,7 @@ static void kill_off_processes(void)
struct task_struct *p;
int pid;
+ read_lock(&tasklist_lock);
for_each_process(p) {
if (p->mm == NULL)
continue;
@@ -29,6 +31,7 @@ static void kill_off_processes(void)
pid = p->mm->context.id.u.pid;
os_kill_ptraced_process(pid, 1);
}
+ read_unlock(&tasklist_lock);
}
}
--
1.7.9.2
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH 06/10] blackfin: Fix possible deadlock in decode_address()
Oleg Nesterov found an interesting deadlock possibility:
> sysrq_showregs_othercpus() does smp_call_function(showacpu)
> and showacpu() show_stack()->decode_address(). Now suppose that IPI
> interrupts the task holding read_lock(tasklist).
To fix this, blackfin should not grab the write_ variant of the
tasklist lock, read_ one is enough.
Suggested-by: Oleg Nesterov
Signed-off-by: Anton Vorontsov
---
arch/blackfin/kernel/trace.c |6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/blackfin/kernel/trace.c b/arch/blackfin/kernel/trace.c
index 5102cdf..9cc9302 100644
--- a/arch/blackfin/kernel/trace.c
+++ b/arch/blackfin/kernel/trace.c
@@ -28,7 +28,7 @@ void decode_address(char *buf, unsigned long address)
{
struct task_struct *p;
struct mm_struct *mm;
- unsigned long flags, offset;
+ unsigned long offset;
struct rb_node *n;
#ifdef CONFIG_KALLSYMS
@@ -112,7 +112,7 @@ void decode_address(char *buf, unsigned long address)
* mappings of all our processes and see if we can't be a whee
* bit more specific
*/
- write_lock_irqsave(&tasklist_lock, flags);
+ read_lock(&tasklist_lock);
for_each_process(p) {
struct task_struct *t;
@@ -185,7 +185,7 @@ __continue:
sprintf(buf, "/* kernel dynamic memory */");
done:
- write_unlock_irqrestore(&tasklist_lock, flags);
+ read_unlock(&tasklist_lock);
}
#define EXPAND_LEN ((1 << CONFIG_DEBUG_BFIN_HWTRACE_EXPAND_LEN) * 256 - 1)
--
1.7.9.2
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH 05/10] blackfin: A couple of task->mm handling fixes
The patch fixes two problems:
1. Working with task->mm w/o getting mm or grabing the task lock is
dangerous as ->mm might disappear (exit_mm() assigns NULL under
task_lock(), so tasklist lock is not enough).
We can't use get_task_mm()/mmput() pair as mmput() might sleep,
so we have to take the task lock while handle its mm.
2. Checking for process->mm is not enough because process' main
thread may exit or detach its mm via use_mm(), but other threads
may still have a valid mm.
To catch this we use find_lock_task_mm(), which walks up all
threads and returns an appropriate task (with task lock held).
Suggested-by: Oleg Nesterov
Signed-off-by: Anton Vorontsov
---
arch/blackfin/kernel/trace.c | 26 +-
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/arch/blackfin/kernel/trace.c b/arch/blackfin/kernel/trace.c
index 050db44..5102cdf 100644
--- a/arch/blackfin/kernel/trace.c
+++ b/arch/blackfin/kernel/trace.c
@@ -10,6 +10,8 @@
#include
#include
#include
+#include
+#include
#include
#include
#include
@@ -27,7 +29,6 @@ void decode_address(char *buf, unsigned long address)
struct task_struct *p;
struct mm_struct *mm;
unsigned long flags, offset;
- unsigned char in_atomic = (bfin_read_IPEND() & 0x10) || in_atomic();
struct rb_node *n;
#ifdef CONFIG_KALLSYMS
@@ -113,15 +114,15 @@ void decode_address(char *buf, unsigned long address)
*/
write_lock_irqsave(&tasklist_lock, flags);
for_each_process(p) {
- mm = (in_atomic ? p->mm : get_task_mm(p));
- if (!mm)
- continue;
+ struct task_struct *t;
- if (!down_read_trylock(&mm->mmap_sem)) {
- if (!in_atomic)
- mmput(mm);
+ t = find_lock_task_mm(p);
+ if (!t)
continue;
- }
+
+ mm = t->mm;
+ if (!down_read_trylock(&mm->mmap_sem))
+ goto __continue;
for (n = rb_first(&mm->mm_rb); n; n = rb_next(n)) {
struct vm_area_struct *vma;
@@ -130,7 +131,7 @@ void decode_address(char *buf, unsigned long address)
if (address >= vma->vm_start && address < vma->vm_end) {
char _tmpbuf[256];
- char *name = p->comm;
+ char *name = t->comm;
struct file *file = vma->vm_file;
if (file) {
@@ -163,8 +164,7 @@ void decode_address(char *buf, unsigned long address)
name, vma->vm_start,
vma->vm_end);
up_read(&mm->mmap_sem);
- if (!in_atomic)
- mmput(mm);
+ task_unlock(t);
if (buf[0] == '\0')
sprintf(buf, "[ %s ] dynamic memory",
name);
@@ -174,8 +174,8 @@ void decode_address(char *buf, unsigned long address)
}
up_read(&mm->mmap_sem);
- if (!in_atomic)
- mmput(mm);
+__continue:
+ task_unlock(t);
}
/*
--
1.7.9.2
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH 04/10] sh: Use clear_tasks_mm_cpumask()
Current CPU hotplug code has some task->mm handling issues:
1. Working with task->mm w/o getting mm or grabing the task lock is
dangerous as ->mm might disappear (exit_mm() assigns NULL under
task_lock(), so tasklist lock is not enough).
We can't use get_task_mm()/mmput() pair as mmput() might sleep,
so we must take the task lock while handle its mm.
2. Checking for process->mm is not enough because process' main
thread may exit or detach its mm via use_mm(), but other threads
may still have a valid mm.
To fix this we would need to use find_lock_task_mm(), which would
walk up all threads and returns an appropriate task (with task
lock held).
clear_tasks_mm_cpumask() has all the issues fixed, so let's use it.
Suggested-by: Oleg Nesterov
Signed-off-by: Anton Vorontsov
---
arch/sh/kernel/smp.c |7 +--
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/arch/sh/kernel/smp.c b/arch/sh/kernel/smp.c
index f624174..2470c83 100644
--- a/arch/sh/kernel/smp.c
+++ b/arch/sh/kernel/smp.c
@@ -123,7 +123,6 @@ void native_play_dead(void)
int __cpu_disable(void)
{
unsigned int cpu = smp_processor_id();
- struct task_struct *p;
int ret;
ret = mp_ops->cpu_disable(cpu);
@@ -153,11 +152,7 @@ int __cpu_disable(void)
flush_cache_all();
local_flush_tlb_all();
- read_lock(&tasklist_lock);
- for_each_process(p)
- if (p->mm)
- cpumask_clear_cpu(cpu, mm_cpumask(p->mm));
- read_unlock(&tasklist_lock);
+ clear_tasks_mm_cpumask(cpu);
return 0;
}
--
1.7.9.2
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH 03/10] powerpc: Use clear_tasks_mm_cpumask()
Current CPU hotplug code has some task->mm handling issues:
1. Working with task->mm w/o getting mm or grabing the task lock is
dangerous as ->mm might disappear (exit_mm() assigns NULL under
task_lock(), so tasklist lock is not enough).
We can't use get_task_mm()/mmput() pair as mmput() might sleep,
so we must take the task lock while handle its mm.
2. Checking for process->mm is not enough because process' main
thread may exit or detach its mm via use_mm(), but other threads
may still have a valid mm.
To fix this we would need to use find_lock_task_mm(), which would
walk up all threads and returns an appropriate task (with task
lock held).
clear_tasks_mm_cpumask() has all the issues fixed, so let's use it.
Suggested-by: Oleg Nesterov
Signed-off-by: Anton Vorontsov
---
arch/powerpc/mm/mmu_context_nohash.c | 11 ++-
1 file changed, 2 insertions(+), 9 deletions(-)
diff --git a/arch/powerpc/mm/mmu_context_nohash.c
b/arch/powerpc/mm/mmu_context_nohash.c
index 5b63bd3..e779642 100644
--- a/arch/powerpc/mm/mmu_context_nohash.c
+++ b/arch/powerpc/mm/mmu_context_nohash.c
@@ -333,9 +333,7 @@ static int __cpuinit mmu_context_cpu_notify(struct
notifier_block *self,
unsigned long action, void *hcpu)
{
unsigned int cpu = (unsigned int)(long)hcpu;
-#ifdef CONFIG_HOTPLUG_CPU
- struct task_struct *p;
-#endif
+
/* We don't touch CPU 0 map, it's allocated at aboot and kept
* around forever
*/
@@ -358,12 +356,7 @@ static int __cpuinit mmu_context_cpu_notify(struct
notifier_block *self,
stale_map[cpu] = NULL;
/* We also clear the cpu_vm_mask bits of CPUs going away */
- read_lock(&tasklist_lock);
- for_each_process(p) {
- if (p->mm)
- cpumask_clear_cpu(cpu, mm_cpumask(p->mm));
- }
- read_unlock(&tasklist_lock);
+ clear_tasks_mm_cpumask(cpu);
break;
#endif /* CONFIG_HOTPLUG_CPU */
}
--
1.7.9.2
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH 02/10] arm: Use clear_tasks_mm_cpumask()
Current CPU hotplug code has some task->mm handling issues:
1. Working with task->mm w/o getting mm or grabing the task lock is
dangerous as ->mm might disappear (exit_mm() assigns NULL under
task_lock(), so tasklist lock is not enough).
We can't use get_task_mm()/mmput() pair as mmput() might sleep,
so we must take the task lock while handle its mm.
2. Checking for process->mm is not enough because process' main
thread may exit or detach its mm via use_mm(), but other threads
may still have a valid mm.
To fix this we would need to use find_lock_task_mm(), which would
walk up all threads and returns an appropriate task (with task
lock held).
clear_tasks_mm_cpumask() has all the issues fixed, so let's use it.
Suggested-by: Oleg Nesterov
Signed-off-by: Anton Vorontsov
---
arch/arm/kernel/smp.c |8 +---
1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index cdeb727..880b93a 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -136,7 +136,6 @@ static void percpu_timer_stop(void);
int __cpu_disable(void)
{
unsigned int cpu = smp_processor_id();
- struct task_struct *p;
int ret;
ret = platform_cpu_disable(cpu);
@@ -166,12 +165,7 @@ int __cpu_disable(void)
flush_cache_all();
local_flush_tlb_all();
- read_lock(&tasklist_lock);
- for_each_process(p) {
- if (p->mm)
- cpumask_clear_cpu(cpu, mm_cpumask(p->mm));
- }
- read_unlock(&tasklist_lock);
+ clear_tasks_mm_cpumask(cpu);
return 0;
}
--
1.7.9.2
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH 01/10] cpu: Introduce clear_tasks_mm_cpumask() helper
Many architctures clear tasks' mm_cpumask like this:
read_lock(&tasklist_lock);
for_each_process(p) {
if (p->mm)
cpumask_clear_cpu(cpu, mm_cpumask(p->mm));
}
read_unlock(&tasklist_lock);
The code above has several problems, such as:
1. Working with task->mm w/o getting mm or grabing the task lock is
dangerous as ->mm might disappear (exit_mm() assigns NULL under
task_lock(), so tasklist lock is not enough).
2. Checking for process->mm is not enough because process' main
thread may exit or detach its mm via use_mm(), but other threads
may still have a valid mm.
This patch implements a small helper function that does things
correctly, i.e.:
1. We take the task's lock while whe handle its mm (we can't use
get_task_mm()/mmput() pair as mmput() might sleep);
2. To catch exited main thread case, we use find_lock_task_mm(),
which walks up all threads and returns an appropriate task
(with task lock held).
Signed-off-by: Anton Vorontsov
---
include/linux/cpu.h |1 +
kernel/cpu.c| 18 ++
2 files changed, 19 insertions(+)
diff --git a/include/linux/cpu.h b/include/linux/cpu.h
index 1f65875..941e865 100644
--- a/include/linux/cpu.h
+++ b/include/linux/cpu.h
@@ -171,6 +171,7 @@ extern void put_online_cpus(void);
#define hotcpu_notifier(fn, pri) cpu_notifier(fn, pri)
#define register_hotcpu_notifier(nb) register_cpu_notifier(nb)
#define unregister_hotcpu_notifier(nb) unregister_cpu_notifier(nb)
+void clear_tasks_mm_cpumask(int cpu);
int cpu_down(unsigned int cpu);
#ifdef CONFIG_ARCH_CPU_PROBE_RELEASE
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 2060c6e..5255936 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -10,6 +10,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -171,6 +172,23 @@ void __ref unregister_cpu_notifier(struct notifier_block
*nb)
}
EXPORT_SYMBOL(unregister_cpu_notifier);
+void clear_tasks_mm_cpumask(int cpu)
+{
+ struct task_struct *p;
+
+ read_lock(&tasklist_lock);
+ for_each_process(p) {
+ struct task_struct *t;
+
+ t = find_lock_task_mm(p);
+ if (!t)
+ continue;
+ cpumask_clear_cpu(cpu, mm_cpumask(t->mm));
+ task_unlock(t);
+ }
+ read_unlock(&tasklist_lock);
+}
+
static inline void check_for_tasks(int cpu)
{
struct task_struct *p;
--
1.7.9.2
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH v2 0/10] Fixes for common mistakes w/ for_each_process and task->mm
Hi all, This is a reincarnation of the task->mm fixes. Several architectures were traverse the tasklist in an unsafe manner, plus there are a few cases of unsafe access to task->mm. In v2 I decided to introduce a small helper in cpu.c: most arches duplicate the same [buggy] code snippet, so it's better to fix it and move the logic into a common function. Thanks! -- Anton Vorontsov Email: cbouatmai...@gmail.com ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] eSDHC: Access Freescale eSDHC registers by 32-bit
On Fri, Sep 09, 2011 at 08:05:46PM +0800, Roy Zang wrote:
> From: Xu lei
>
> Freescale eSDHC registers only support 32-bit accesses,
> this patch ensures that all Freescale eSDHC register accesses
> are 32-bit.
>
> Signed-off-by: Xu lei
> Signed-off-by: Roy Zang
> Signed-off-by: Kumar Gala
> ---
The patch looks OK.
Acked-by: Anton Vorontsov
[...]
> +static u8 esdhc_readb(struct sdhci_host *host, int reg)
> +{
> + int base = reg & ~0x3;
> + int shift = (reg & 0x3) * 8;
> + u8 ret = (in_be32(host->ioaddr + base) >> shift) & 0xff;
> return ret;
> }
Though, I wonder if we could change sdhci_be32bs_read{b,w}, instead
of making this local to eSDHC.
The thing is: sdhci_be32bs_writeb() is using clrsetbits_be32,
so the write variant already uses 32-bit accessors, so nothing should
break if we switch sdhci_be32bs_readb() to in_be32().
But maybe it's safer if we do this in a separate patch, so that it
could be easily reverted without impacting eSDHC if something actually
breaks.
You decide. :-)
Thanks!
--
Anton Vorontsov
Email: cbouatmai...@gmail.com
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] usb: Allocate pram dynamically.
On Tue, Aug 23, 2011 at 02:38:41PM +0200, Joakim Tjernlund wrote: > MPC832x does not have enough MURAM to do fixed MURAM allocation. > Change to dynamic allocation. > > Signed-off-by: Joakim Tjernlund Acked-by: Anton Vorontsov Thanks! p.s. You probably want to send this to Greg KH, + Cc linux-usb mailing list. -- Anton Vorontsov Email: cbouatmai...@gmail.com ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/2 v2] eSDHC: Fix errors when booting kernel with fsl esdhc
Hello, On Fri, Aug 12, 2011 at 09:44:26AM +, Zang Roy-R61911 wrote: [...] > > We try to not pollute generic sdhci.c driver with chip-specific > > quirks. > > > > Maybe you can do the fixups via IO accessors? Or by introducing > > some additional sdhci op? > Anton, > thanks for the comment, as we discussed, the original code use 8 bit byte > operation, > while in fact, on some powerpc platform, 32 bit operation is needed. > should it be possible fixed by adding some wrapper in IO accessors or > introduce additional sdhci op? I would do it in the IO accessors. Thanks, -- Anton Vorontsov Email: cbouatmai...@gmail.com ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/2 v2] eSDHC: Fix errors when booting kernel with fsl esdhc
On Fri, Jul 22, 2011 at 06:15:17PM +0800, Roy Zang wrote:
[...]
> if (host->version >= SDHCI_SPEC_200) {
> - ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
> - ctrl &= ~SDHCI_CTRL_DMA_MASK;
> - if ((host->flags & SDHCI_REQ_USE_DMA) &&
> - (host->flags & SDHCI_USE_ADMA))
> - ctrl |= SDHCI_CTRL_ADMA32;
> - else
> - ctrl |= SDHCI_CTRL_SDMA;
> - sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
> + if (host->quirks & SDHCI_QUIRK_QORIQ_PROCTL_WEIRD) {
> +#define ESDHCI_PROCTL_DMAS_MASK 0x0300
> +#define ESDHCI_PROCTL_ADMA32 0x0200
> +#define ESDHCI_PROCTL_SDMA 0x
> + ctrl = sdhci_readl(host, SDHCI_HOST_CONTROL);
> + ctrl &= ~ESDHCI_PROCTL_DMAS_MASK;
> + if ((host->flags & SDHCI_REQ_USE_DMA) &&
> + (host->flags & SDHCI_USE_ADMA))
> + ctrl |= ESDHCI_PROCTL_ADMA32;
> + else
> + ctrl |= ESDHCI_PROCTL_SDMA;
> + sdhci_writel(host, ctrl, SDHCI_HOST_CONTROL);
> + } else {
> + ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
> + ctrl &= ~SDHCI_CTRL_DMA_MASK;
> + if ((host->flags & SDHCI_REQ_USE_DMA) &&
> + (host->flags & SDHCI_USE_ADMA))
> + ctrl |= SDHCI_CTRL_ADMA32;
> + else
> + ctrl |= SDHCI_CTRL_SDMA;
> + sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
We try to not pollute generic sdhci.c driver with chip-specific
quirks.
Maybe you can do the fixups via IO accessors? Or by introducing
some additional sdhci op?
[...]
> if (power != (unsigned short)-1) {
> switch (1 << power) {
> +#define ESDHCI_FSL_POWER_MASK 0x40
> +#define ESDHCI_FSL_POWER_1800x00
> +#define ESDHCI_FSL_POWER_3000x40
Same here. The driver will rot quickly if everyone would start
putting chip-specific quirks into sdhci.c. Please don't.
Thanks,
--
Anton Vorontsov
Email: cbouatmai...@gmail.com
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 3/3] eSDHC: fix incorrect default value of the capabilities register on P4080
On Tue, Jul 05, 2011 at 12:19:03PM +0800, Roy Zang wrote: > P4080 eSDHC errata 12 describes incorrect default value of the > the host controller capabilities register. > > The default value of the VS18 and VS30 fields in the host controller > capabilities register (HOSTCAPBLT) are incorrect. The default of these bits > should be zero instead of one in the eSDHC logic. > > This patch adds the workaround for these errata. > > Signed-off-by: Roy Zang > --- > drivers/mmc/host/sdhci-of-core.c |3 +++ > drivers/mmc/host/sdhci.c |6 ++ > include/linux/mmc/sdhci.h|4 > 3 files changed, 13 insertions(+), 0 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-of-core.c > b/drivers/mmc/host/sdhci-of-core.c > index fede43d..9bdd30d 100644 > --- a/drivers/mmc/host/sdhci-of-core.c > +++ b/drivers/mmc/host/sdhci-of-core.c > @@ -182,6 +182,9 @@ static int __devinit sdhci_of_probe(struct > platform_device *ofdev) > if (of_device_is_compatible(np, "fsl,esdhc")) > host->quirks |= SDHCI_QUIRK_QORIQ_PROCTL_WEIRD; > > + if (of_device_is_compatible(np, "fsl,p4080-esdhc")) > + host->quirks |= SDHCI_QUIRK_QORIQ_HOSTCAPBLT_ONLY_VS33; Should really use voltage-ranges, not quirks. http://www.spinics.net/lists/linux-mmc/msg02785.html Thanks, -- Anton Vorontsov Email: cbouatmai...@gmail.com ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 1/2] ppc/85xx: create a platform node for PCI EDAC device
On Thu, Jun 02, 2011 at 04:25:02PM +0400, Dmitry Eremin-Solenikov wrote:
> As a device for pci node isn't created, create a special platform_device
> for PCI EDAC device on MPC85xx.
>
> Signed-off-by: Dmitry Eremin-Solenikov
> ---
> arch/powerpc/sysdev/fsl_pci.c | 33 +
> 1 files changed, 33 insertions(+), 0 deletions(-)
>
> diff --git a/arch/powerpc/sysdev/fsl_pci.c b/arch/powerpc/sysdev/fsl_pci.c
> index 68ca929..0e37259 100644
> --- a/arch/powerpc/sysdev/fsl_pci.c
> +++ b/arch/powerpc/sysdev/fsl_pci.c
> @@ -381,6 +381,39 @@ int __init fsl_add_bridge(struct device_node *dev, int
> is_primary)
> return 0;
> }
>
> +int __init fsl_add_pci_err(void)
static :-)
> +{
> + struct device_node *np;
> +
> + for_each_node_by_type(np, "pci") {
> + /* Only PCI, not PCI Express! */
> + if (of_device_is_compatible(np, "fsl,mpc8540-pci")) {
> + struct resource r[2];
How about '= {};' initializer instead of the '= NULL's down below?
> +
> + r[0].parent = NULL;
> + r[1].parent = NULL;
Thanks,
--
Anton Vorontsov
Email: cbouatmai...@gmail.com
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 1/2] ppc/85xx: create a platform node for PCI EDAC device
On Wed, Jun 01, 2011 at 06:55:35PM +0400, Dmitry Eremin-Solenikov wrote:
> On 6/1/11, Anton Vorontsov wrote:
> > On Wed, Jun 01, 2011 at 04:28:11PM +0400, Dmitry Eremin-Solenikov wrote:
> > [...]
> >> --- a/arch/powerpc/sysdev/fsl_pci.h
> >> +++ b/arch/powerpc/sysdev/fsl_pci.h
> >> @@ -92,6 +92,7 @@ extern int fsl_add_bridge(struct device_node *dev, int
> >> is_primary);
> >> extern void fsl_pcibios_fixup_bus(struct pci_bus *bus);
> >> extern int mpc83xx_add_bridge(struct device_node *dev);
> >> u64 fsl_pci_immrbar_base(struct pci_controller *hose);
> >> +int fsl_add_pci_err(void);
> >
> > With
> >
> > #ifdef CONFIG_PCI
> > int fsl_add_pci_err(void);
> > #else
> > static inline int fsl_add_pci_err(void) { return -ENODEV; }
> > #endif
> >
> > You won't need endless ifdefs in the board files:
>
> OK, will redo this patch.
Btw, if you don't check return value of fsl_add_pci_err(), then
it would probably make sense to return void. And if you do
check it somewhere, be sure to include linux/errno.h for
-ENODEV. :-)
Thanks,
--
Anton Vorontsov
Email: cbouatmai...@gmail.com
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 1/2] ppc/85xx: create a platform node for PCI EDAC device
On Wed, Jun 01, 2011 at 04:28:11PM +0400, Dmitry Eremin-Solenikov wrote:
[...]
> --- a/arch/powerpc/sysdev/fsl_pci.h
> +++ b/arch/powerpc/sysdev/fsl_pci.h
> @@ -92,6 +92,7 @@ extern int fsl_add_bridge(struct device_node *dev, int
> is_primary);
> extern void fsl_pcibios_fixup_bus(struct pci_bus *bus);
> extern int mpc83xx_add_bridge(struct device_node *dev);
> u64 fsl_pci_immrbar_base(struct pci_controller *hose);
> +int fsl_add_pci_err(void);
With
#ifdef CONFIG_PCI
int fsl_add_pci_err(void);
#else
static inline int fsl_add_pci_err(void) { return -ENODEV; }
#endif
You won't need endless ifdefs in the board files:
#ifdef CONFIG_PCI
fsl_add_pci_err();
#endif
Also, why not add this call to the fsl_add_bridge(), so you
won't need to touch board files at all.
Thanks,
--
Anton Vorontsov
Email: cbouatmai...@gmail.com
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [rtc-linux] Re: [PATCH] Add support for pt7c4338 (rtc device) in rtc-ds1307 driver
On Mon, May 30, 2011 at 02:29:58PM +, Tabi Timur-B04825 wrote: > On Mon, May 30, 2011 at 3:24 AM, Wolfram Sang wrote: > > > The first place where this should be mentioned is the datasheet of the > > pt-chip, so you might ask the producer to add this information (don't > > expect much to happen, though). > > It's true that the data sheet does not mention that it's identical to > the DS1307, but that's still no excuse for not noticing it and writing > a whole driver for it. :-( > > > IIRC I asked you explicitly for the differences between the chips. If > > there are none, you can use the driver directly, right? :) > > Yes. The device tree node for the PT7C4338 device should just say > >/* The board has a PT7C4338, which is compatible with the DS1307 */ >compatible = "dallas,ds1307"; While it seems to be 100% compatible, there could be chip-specific bugs or some interesting features that are hidden behind "reserved" bits and registers. So I think device tree should not lie about the chip model. Doing 'compatible = "pericom,pt7c4338", "dallas,ds1307"' is perfectly fine. Note that today the several compatible entries approach gives you almost nothing, as you will need to add pt7c4338 entry into the driver anyway. I tried to improve this, i.e. make linux do OF-matching on the most generic compatible entry (the last one): http://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg21196.html It was received coldly though: http://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg22041.html http://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg21273.html -- Anton Vorontsov Email: cbouatmai...@gmail.com ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v2 1/7] mmc: sdhci: make sdhci-pltfm device drivers self registered
On Thu, May 05, 2011 at 09:22:52PM +0800, Shawn Guo wrote:
[...]
> +static int __devinit sdhci_cns3xxx_probe(struct platform_device *pdev)
> +{
> + return sdhci_pltfm_register(pdev, &sdhci_cns3xxx_pdata);
> +}
> +
> +static int __devexit sdhci_cns3xxx_remove(struct platform_device *pdev)
> +{
> + return sdhci_pltfm_unregister(pdev);
> +}
> +
> +static struct platform_driver sdhci_cns3xxx_driver = {
> + .driver = {
> + .name = "sdhci-cns3xxx",
> + .owner = THIS_MODULE,
> + },
> + .probe = sdhci_cns3xxx_probe,
> + .remove = __devexit_p(sdhci_cns3xxx_remove),
> +#ifdef CONFIG_PM
> + .suspend= sdhci_pltfm_suspend,
> + .resume = sdhci_pltfm_resume,
> +#endif
> +};
> +
> +static int __init sdhci_cns3xxx_init(void)
> +{
> + return platform_driver_register(&sdhci_cns3xxx_driver);
> +}
> +module_init(sdhci_cns3xxx_init);
> +
> +static void __exit sdhci_cns3xxx_exit(void)
> +{
> + platform_driver_unregister(&sdhci_cns3xxx_driver);
> +}
> +module_exit(sdhci_cns3xxx_exit);
I don't think I like this duplicate code for each platform sub-
driver. It's repetitive and annoying. :-/
But considering that ARM will be multiplatform soon, we don't
want to have every mach-* stuff in the single sdhci-pltfm.
So... OK. If that compiles, I'm fine with it. :-D
Acked-by: Anton Vorontsov
Thanks!
--
Anton Vorontsov
Email: cbouatmai...@gmail.com
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v2 2/7] mmc: sdhci: eliminate sdhci_of_host and sdhci_of_data
On Thu, May 05, 2011 at 09:22:53PM +0800, Shawn Guo wrote: > The patch migrates the use of sdhci_of_host and sdhci_of_data to > sdhci_pltfm_host and sdhci_pltfm_data, so that the former pair can > be eliminated. > > Signed-off-by: Shawn Guo > Reviewed-by: Grant Likely Acked-by: Anton Vorontsov Thanks! -- Anton Vorontsov Email: cbouatmai...@gmail.com ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v2 3/7] mmc: sdhci: make sdhci-of device drivers self registered
On Thu, May 05, 2011 at 09:22:54PM +0800, Shawn Guo wrote:
[...]
> - * Copyright (c) 2007 Freescale Semiconductor, Inc.
> - * Copyright (c) 2009 MontaVista Software, Inc.
> - *
> - * Authors: Xiaobo Xie
> - * Anton Vorontsov
[...]
> -#ifdef CONFIG_MMC_SDHCI_BIG_ENDIAN_32BIT_BYTE_SWAPPER
> -
> -/*
> - * These accessors are designed for big endian hosts doing I/O to
> - * little endian controllers incorporating a 32-bit hardware byte swapper.
> - */
> -
> -u32 sdhci_be32bs_readl(struct sdhci_host *host, int reg)
> -{
> - return in_be32(host->ioaddr + reg);
> -}
> -
> -u16 sdhci_be32bs_readw(struct sdhci_host *host, int reg)
> -{
> - return in_be16(host->ioaddr + (reg ^ 0x2));
> -}
> -
> -u8 sdhci_be32bs_readb(struct sdhci_host *host, int reg)
> -{
> - return in_8(host->ioaddr + (reg ^ 0x3));
> -}
> -
> -void sdhci_be32bs_writel(struct sdhci_host *host, u32 val, int reg)
> -{
> - out_be32(host->ioaddr + reg, val);
> -}
> -
> -void sdhci_be32bs_writew(struct sdhci_host *host, u16 val, int reg)
> -{
> - struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> - int base = reg & ~0x3;
> - int shift = (reg & 0x2) * 8;
> -
> - switch (reg) {
> - case SDHCI_TRANSFER_MODE:
> - /*
> - * Postpone this write, we must do it together with a
> - * command write that is down below.
> - */
> - pltfm_host->xfer_mode_shadow = val;
> - return;
> - case SDHCI_COMMAND:
> - sdhci_be32bs_writel(host, val << 16 |
> pltfm_host->xfer_mode_shadow,
> - SDHCI_TRANSFER_MODE);
> - return;
> - }
> - clrsetbits_be32(host->ioaddr + base, 0x << shift, val << shift);
> -}
> -
> -void sdhci_be32bs_writeb(struct sdhci_host *host, u8 val, int reg)
> -{
> - int base = reg & ~0x3;
> - int shift = (reg & 0x3) * 8;
> -
> - clrsetbits_be32(host->ioaddr + base , 0xff << shift, val << shift);
> -}
> -#endif /* CONFIG_MMC_SDHCI_BIG_ENDIAN_32BIT_BYTE_SWAPPER */
[...]
> +#ifdef CONFIG_MMC_SDHCI_BIG_ENDIAN_32BIT_BYTE_SWAPPER
> +/*
> + * These accessors are designed for big endian hosts doing I/O to
> + * little endian controllers incorporating a 32-bit hardware byte swapper.
> + */
> +u32 sdhci_be32bs_readl(struct sdhci_host *host, int reg)
> +{
> + return in_be32(host->ioaddr + reg);
> +}
> +
> +u16 sdhci_be32bs_readw(struct sdhci_host *host, int reg)
> +{
> + return in_be16(host->ioaddr + (reg ^ 0x2));
> +}
> +
> +u8 sdhci_be32bs_readb(struct sdhci_host *host, int reg)
> +{
> + return in_8(host->ioaddr + (reg ^ 0x3));
> +}
> +
> +void sdhci_be32bs_writel(struct sdhci_host *host, u32 val, int reg)
> +{
> + out_be32(host->ioaddr + reg, val);
> +}
> +
> +void sdhci_be32bs_writew(struct sdhci_host *host, u16 val, int reg)
> +{
> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> + int base = reg & ~0x3;
> + int shift = (reg & 0x2) * 8;
> +
> + switch (reg) {
> + case SDHCI_TRANSFER_MODE:
> + /*
> + * Postpone this write, we must do it together with a
> + * command write that is down below.
> + */
> + pltfm_host->xfer_mode_shadow = val;
> + return;
> + case SDHCI_COMMAND:
> + sdhci_be32bs_writel(host, val << 16 |
> pltfm_host->xfer_mode_shadow,
> + SDHCI_TRANSFER_MODE);
> + return;
> + }
> + clrsetbits_be32(host->ioaddr + base, 0x << shift, val << shift);
> +}
> +
> +void sdhci_be32bs_writeb(struct sdhci_host *host, u8 val, int reg)
> +{
> + int base = reg & ~0x3;
> + int shift = (reg & 0x3) * 8;
> +
> + clrsetbits_be32(host->ioaddr + base , 0xff << shift, val << shift);
> +}
> +#endif /* CONFIG_MMC_SDHCI_BIG_ENDIAN_32BIT_BYTE_SWAPPER */
I noticed you're very careful wrt copyright/authorship notices,
except for this case. How about retaining copyright stuff when
you merge these two files?
The patch itself looks great though.
Acked-by: Anton Vorontsov
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v2 5/7] mmc: sdhci: consolidate sdhci-of-esdhc and sdhci-esdhc-imx
On Thu, May 05, 2011 at 09:22:56PM +0800, Shawn Guo wrote: > This patch is to consolidate SDHCI driver for Freescale eSDHC > controller found on both MPCxxx and i.MX platforms. It merges > sdhci-of-esdhc.c into sdhci-esdhc.c, so that the same pair of > .probe/.remove hook works with eSDHC for two platforms. > > As the results, sdhci-of-esdhc.c and sdhci-esdhc.h are removed, and > header esdhc.h containing the definition of esdhc_platform_data is > put into the public folder. > > Signed-off-by: Shawn Guo I'm not sure if that makes sense to merge these two. I'd rather vote for renaming sdhci-of-esdhc to sdhci-esdhc-mpc. :-) -- Anton Vorontsov Email: cbouatmai...@gmail.com ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v2 6/7] mmc: sdhci: merge two sdhci-pltfm.h into one
On Thu, May 05, 2011 at 09:22:57PM +0800, Shawn Guo wrote: > The structure sdhci_pltfm_data is not necessarily to be in a public > header like include/linux/mmc/sdhci-pltfm.h, so the patch moves it > into drivers/mmc/host/sdhci-pltfm.h and eliminates the former one. > > Signed-off-by: Shawn Guo > Reviewed-by: Grant Likely > Reviewed-by: Wolfram Sang Acked-by: Anton Vorontsov ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] gianfar: Fall back to software tcp/udp checksum on older controllers
Hello Alex,
On Thu, Jan 27, 2011 at 12:14:21AM -0800, Alex Dubov wrote:
> As specified by errata eTSEC49 of MPC8548 and errata eTSEC12 of MPC83xx,
> older revisions of gianfar controllers will be unable to calculate a TCP/UDP
> packet checksum for some aligments of the appropriate FCB. This patch checks
> for FCB alignment on such controllers and falls back to software checksumming
> if the aligment is known to be bad.
>
> Signed-off-by: Alex Dubov
> ---
> This is my, somewhat different approach to Matthew Creech proposed solution.
>
> drivers/net/gianfar.c | 21 +++--
> drivers/net/gianfar.h |1 +
> 2 files changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c
> index 5ed8f9f..b4f0e99 100644
> --- a/drivers/net/gianfar.c
> +++ b/drivers/net/gianfar.c
> @@ -950,6 +950,11 @@ static void gfar_detect_errata(struct gfar_private *priv)
> (pvr == 0x80861010 && (mod & 0xfff9) == 0x80c0))
> priv->errata |= GFAR_ERRATA_A002;
>
> + /* MPC8313 Rev < 2.0, MPC8548 rev 2.0 */
> + if ((pvr == 0x80850010 && mod == 0x80b0 && rev < 0x0020)
> + || (pvr == 0x80210020 && mod == 0x8030 && rev == 0x0020))
Please indent it like the above: with two tabs. This is
to keep things consistent.
> + priv->errata |= GFAR_ERRATA_12;
> +
> if (priv->errata)
> dev_info(dev, "enabled errata workarounds, flags: 0x%x\n",
>priv->errata);
> @@ -2156,8 +2161,20 @@ static int gfar_start_xmit(struct sk_buff *skb, struct
> net_device *dev)
> /* Set up checksumming */
> if (CHECKSUM_PARTIAL == skb->ip_summed) {
> fcb = gfar_add_fcb(skb);
> - lstatus |= BD_LFLAG(TXBD_TOE);
> - gfar_tx_checksum(skb, fcb);
> + switch (unlikely(gfar_has_errata(priv, GFAR_ERRATA_12))
> + ? 1 : 0) {
The switch construction is quite bizarre. ;-) Why not
if (gfar_has_errata() && (ulong)fcb % 0x20 > 18) {
csum_help();
} else {
lstatus |=...
tx_csum();
}
?
Thanks,
> + case 1:
> + /* as specified by errata */
> + if (((unsigned long)fcb % 0x20) > 0x18) {
> + __skb_pull(skb, GMAC_FCB_LEN);
> + skb_checksum_help(skb);
> + break;
> + }
> + /* otherwise, fall through */
> + default:
> + lstatus |= BD_LFLAG(TXBD_TOE);
> + gfar_tx_checksum(skb, fcb);
> + }
> }
--
Anton Vorontsov
Email: cbouatmai...@gmail.com
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] of_mmc_spi: add card detect irq support
On Mon, Aug 30, 2010 at 11:49:08AM -0600, Grant Likely wrote: > On Mon, Aug 30, 2010 at 10:38 AM, David Brownell wrote: > > Since I don't do OpenFirmware, let's hear from > > Grant on this one. > > Looks good to me. > > Acked-by: Grant Likely I wonder what happened with this patch? Thanks, -- Anton Vorontsov Email: cbouatmai...@gmail.com ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/2] powerpc: pcm030/032: add pagesize to dts
On Tue, Nov 16, 2010 at 10:45:37PM +0100, Wolfram Sang wrote: > > > Even worse, I seem to recall that I had once seen a manufacturer increasing > > the > > page-size from one charge to the next without changing the part-number, so I > > got this feeling "you can't map pagesize to manufacturer/type" which I still > > have. Sadly, this was long ago, so I can't proof it right now. Will try to > > dig > > up some datasheets when in the office tomorrow. > > Had a look, couldn't find anything :( And now? Well, it seems that there are enough people in "pagesize" camp anyway, I'd say just go ahead with the current approach. :-) It's an additional information, so won't do any harm anyway... Thanks, -- Anton Vorontsov email: cbouatmai...@gmail.com irc://irc.freenode.net/bd2 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/2] powerpc: pcm030/032: add pagesize to dts
On Mon, Nov 15, 2010 at 11:06:44AM -1000, Mitch Bradley wrote:
[...]
> >>eep...@52 {
> >>compatible = "catalyst,24c32";
> >>reg =<0x52>;
> >>+ pagesize =<32>;
> >
> >I think you'd better drop the pagesize property altogether, and
> >instead make the compatible string more specific (if needed at
> >all. are there any 'catalyst,24c32' chips with pagesize != 32?)
>
> Microchip makes a 24c32 part that looks pretty similar to the
> catalyst part, but Microchip's has a 64-byte page size compared to
> Catalyst's 32.
Well, when using microchip part, the compatible string would be
"microchip,24c32", correct? Then we have all the information
already, no need for the pagesize.
> It would probably be feasible to have a generic I2C EEPROM driver
> that could handle many different parts, parameterized by total size,
> block size, and page size.
I guess it can do this already via I2C ID table. The problem
is that I2C core is only using part ID (no vendor ID matching).
So, the current driver may just implement quirks like this:
if (of_device_is_compatible(np, "catalyst,24c32"))
pagesize = 32;
Or, if it's some generic pattern, something like
if (of_device_is_compatible_vendor(np, "catalyst"))
pagesize /= 2;
Thanks,
--
Anton Vorontsov
email: cbouatmai...@gmail.com
irc://irc.freenode.net/bd2
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/2] powerpc: pcm030/032: add pagesize to dts
On Mon, Nov 15, 2010 at 06:25:16PM +0100, Wolfram Sang wrote:
> Signed-off-by: Wolfram Sang
> ---
> arch/powerpc/boot/dts/pcm030.dts |1 +
> arch/powerpc/boot/dts/pcm032.dts |3 ++-
> 2 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/arch/powerpc/boot/dts/pcm030.dts
> b/arch/powerpc/boot/dts/pcm030.dts
> index 8a4ec30..e7c36bc 100644
> --- a/arch/powerpc/boot/dts/pcm030.dts
> +++ b/arch/powerpc/boot/dts/pcm030.dts
> @@ -259,6 +259,7 @@
> eep...@52 {
> compatible = "catalyst,24c32";
> reg = <0x52>;
> + pagesize = <32>;
I think you'd better drop the pagesize property altogether, and
instead make the compatible string more specific (if needed at
all. are there any 'catalyst,24c32' chips with pagesize != 32?)
Thanks,
--
Anton Vorontsov
email: cbouatmai...@gmail.com
irc://irc.freenode.net/bd2
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/2] ucc_geth: Fix deadlock
On Fri, Nov 12, 2010 at 02:55:09PM +0100, Joakim Tjernlund wrote:
> This script:
> while [ 1==1 ] ; do ifconfig eth0 up; usleep 195 ;ifconfig eth0 down;
> dmesg -c ;done
> causes in just a second or two:
> INFO: task ifconfig:572 blocked for more than 120 seconds.
[...]
> The reason appears to be ucc_geth_stop meets adjust_link as the
> PHY reports PHY changes. I belive adjust_link hangs somewhere,
> holding the PHY lock, because ucc_geth_stop disabled the
> controller HW.
> Fix is to stop the PHY before disabling the controller.
>
> Signed-off-by: Joakim Tjernlund
It's unclear where exactly adjust_link() hangs, but the patch
looks as the right thing overall.
Thanks!
Reviewed-by: Anton Vorontsov
> ---
> drivers/net/ucc_geth.c | 10 +++---
> 1 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ucc_geth.c b/drivers/net/ucc_geth.c
> index 6c254ed..06a5db3 100644
> --- a/drivers/net/ucc_geth.c
> +++ b/drivers/net/ucc_geth.c
> @@ -2050,12 +2050,16 @@ static void ucc_geth_stop(struct ucc_geth_private
> *ugeth)
>
> ugeth_vdbg("%s: IN", __func__);
>
> + /*
> + * Tell the kernel the link is down.
> + * Must be done before disabling the controller
> + * or deadlock may happen.
> + */
> + phy_stop(phydev);
> +
> /* Disable the controller */
> ugeth_disable(ugeth, COMM_DIR_RX_AND_TX);
>
> - /* Tell the kernel the link is down */
> - phy_stop(phydev);
> -
> /* Mask all interrupts */
> out_be32(ugeth->uccf->p_uccm, 0x);
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 1/2] ucc_geth: Do not bring the whole IF down when TX failure.
On Fri, Nov 12, 2010 at 02:55:08PM +0100, Joakim Tjernlund wrote: > ucc_geth_close lacks a cancel_work_sync(&ugeth->timeout_work) > to stop any outstanding processing of TX fail. However, one > can not call cancel_work_sync without fixing the timeout function > otherwise it will deadlock. This patch brings ucc_geth in line with > gianfar: > > Don't bring the interface down and up, just reinit controller HW > and PHY. > > Signed-off-by: Joakim Tjernlund Looks sane, thanks! Reviewed-by: Anton Vorontsov ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] edac: mpc85xx: Add support for new MPCxxx/Pxxxx EDAC controllers (fix)
On Thu, Oct 07, 2010 at 02:00:50AM -0500, Kumar Gala wrote:
>
> On Oct 7, 2010, at 1:37 AM, Kumar Gala wrote:
>
> >> @ -1125,7 +1128,10 @@ static struct of_device_id mpc85xx_mc_err_of_match[]
> >> = {
> >>{ .compatible = "fsl,mpc8569-memory-controller", },
> >>{ .compatible = "fsl,mpc8572-memory-controller", },
> >>{ .compatible = "fsl,mpc8349-memory-controller", },
> >> + { .compatible = "fsl,p1020-memory-controller", },
> >> + { .compatible = "fsl,p1021-memory-controller", },
> >>{ .compatible = "fsl,p2020-memory-controller", },
> >> + { .compatible = "fsl,p4080-memory-controller", },
> >
> > This line should be here ;)
>
> should NOT be here.
Hm. Are you sure? I thought that only L2 cache controller is
not applicable (and based on Scott's comment I removed
the l2 cache compatible entry for p4080). But I guess
memory-controller is somewhat similar to all other 85xx?
If it's not, I can surely prepare a patch that removes
p4080 entry.
Thanks,
--
Anton Vorontsov
email: cbouatmai...@gmail.com
irc://irc.freenode.net/bd2
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] edac: mpc85xx: Add support for new MPCxxx/Pxxxx EDAC controllers (fix)
On Thu, Oct 07, 2010 at 01:18:19AM -0500, Kumar Gala wrote:
[...]
> > diff --git a/drivers/edac/mpc85xx_edac.c b/drivers/edac/mpc85xx_edac.c
> > index cfa86f7..b178cfa 100644
> > --- a/drivers/edac/mpc85xx_edac.c
> > +++ b/drivers/edac/mpc85xx_edac.c
> > @@ -652,7 +652,6 @@ static struct of_device_id mpc85xx_l2_err_of_match[] = {
> > { .compatible = "fsl,p1020-l2-cache-controller", },
> > { .compatible = "fsl,p1021-l2-cache-controller", },
> > { .compatible = "fsl,p2020-l2-cache-controller", },
> > - { .compatible = "fsl,p4080-l2-cache-controller", },
> > {},
> > };
> > MODULE_DEVICE_TABLE(of, mpc85xx_l2_err_of_match);
> > --
> > 1.7.0.5
>
> Can you post a new patch as it doesn't look like this got merged by Andrew so
> we need to clean up after ourselves.
It's already in Linus' tree.
Thanks,
- - - -
commit cd1542c8197fc3c2eb3a8301505d5d9738fab1e4
Author: Anton Vorontsov
Date: Tue Aug 10 18:03:21 2010 -0700
edac: mpc85xx: add support for new MPCxxx/P EDAC controllers
Simply add proper IDs into the device table.
Signed-off-by: Anton Vorontsov
Cc: Scott Wood
Cc: Peter Tyser
Cc: Dave Jiang
Cc: Doug Thompson
Signed-off-by: Andrew Morton
Signed-off-by: Linus Torvalds
diff --git a/drivers/edac/mpc85xx_edac.c b/drivers/edac/mpc85xx_edac.c
index fdbad55..af75e27 100644
--- a/drivers/edac/mpc85xx_edac.c
+++ b/drivers/edac/mpc85xx_edac.c
@@ -647,7 +647,10 @@ static struct of_device_id mpc85xx_l2_err_of_match[] = {
{ .compatible = "fsl,mpc8555-l2-cache-controller", },
{ .compatible = "fsl,mpc8560-l2-cache-controller", },
{ .compatible = "fsl,mpc8568-l2-cache-controller", },
+ { .compatible = "fsl,mpc8569-l2-cache-controller", },
{ .compatible = "fsl,mpc8572-l2-cache-controller", },
+ { .compatible = "fsl,p1020-l2-cache-controller", },
+ { .compatible = "fsl,p1021-l2-cache-controller", },
{ .compatible = "fsl,p2020-l2-cache-controller", },
{},
};
@@ -1125,7 +1128,10 @@ static struct of_device_id mpc85xx_mc_err_of_match[] = {
{ .compatible = "fsl,mpc8569-memory-controller", },
{ .compatible = "fsl,mpc8572-memory-controller", },
{ .compatible = "fsl,mpc8349-memory-controller", },
+ { .compatible = "fsl,p1020-memory-controller", },
+ { .compatible = "fsl,p1021-memory-controller", },
{ .compatible = "fsl,p2020-memory-controller", },
+ { .compatible = "fsl,p4080-memory-controller", },
{},
};
MODULE_DEVICE_TABLE(of, mpc85xx_mc_err_of_match);
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 2/7] spi/mpc8xxx: refactor the common code for SPI/eSPI controller
On Thu, Sep 30, 2010 at 04:00:41PM +0800, Mingkai Hu wrote:
[...]
> -static void mpc8xxx_spi_change_mode(struct spi_device *spi)
> +static void fsl_spi_change_mode(struct spi_device *spi)
> {
> struct mpc8xxx_spi *mspi = spi_master_get_devdata(spi->master);
> struct spi_mpc8xxx_cs *cs = spi->controller_state;
> - __be32 __iomem *mode = &mspi->base->mode;
> + struct fsl_spi_reg *reg_base = (struct fsl_spi_reg *)mspi->reg_base;
No need for these type casts (the same is for the whole patch).
--
Anton Vorontsov
email: cbouatmai...@gmail.com
irc://irc.freenode.net/bd2
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 3/7] eSPI: add eSPI controller support
(dev, master);
> +
> + ret = mpc8xxx_spi_probe(dev, mem, irq);
> + if (ret)
> + goto err_probe;
> +
> + master->setup = fsl_espi_setup;
> +
> + mpc8xxx_spi = spi_master_get_devdata(master);
> + mpc8xxx_spi->spi_do_one_msg = fsl_espi_do_one_msg;
> + mpc8xxx_spi->spi_remove = fsl_espi_remove;
> +
> + mpc8xxx_spi->reg_base = ioremap(mem->start, resource_size(mem));
> + if (mpc8xxx_spi->reg_base == NULL) {
Ditto.
> + ret = -ENOMEM;
> + goto err_probe;
> + }
> +
> + reg_base = (struct fsl_espi_reg *)mpc8xxx_spi->reg_base;
> +
> + /* Register for SPI Interrupt */
> + ret = request_irq(mpc8xxx_spi->irq, fsl_espi_irq,
> + 0, "fsl_espi", mpc8xxx_spi);
> +
> + if (ret != 0)
Every time someone writes 'if (rc != 0)', a kitty dies.
Simple 'if (rc)' saves kittens.
> + goto free_irq;
> +
> + if (mpc8xxx_spi->flags & SPI_QE_CPU_MODE) {
> + mpc8xxx_spi->rx_shift = 16;
> + mpc8xxx_spi->tx_shift = 24;
> + }
[...]
> +
> +static int __devexit of_fsl_espi_remove(struct platform_device *dev)
> +{
> + int ret;
> +
> + ret = mpc8xxx_spi_remove(&dev->dev);
> + if (ret)
> + return ret;
> +
> + return 0;
Just 'return mpc8xxx_spi_remove(&dev->dev);' is sufficient.
Also, I think there's no need for this wrapper nowadays (but
splitting OF and real probe() stuff is still appropriate).
> +}
> +
> +static const struct of_device_id of_fsl_espi_match[] = {
> + { .compatible = "fsl,mpc8536-espi" },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, of_fsl_espi_match);
> +
> +static struct of_platform_driver fsl_espi_driver = {
> + .driver = {
> + .name = "fsl_espi",
> + .owner = THIS_MODULE,
> + .of_match_table = of_fsl_espi_match,
> + },
> + .probe = of_fsl_espi_probe,
> + .remove = __devexit_p(of_fsl_espi_remove),
> +};
> +
> +static int __init fsl_espi_init(void)
> +{
> + return of_register_platform_driver(&fsl_espi_driver);
> +}
> +module_init(fsl_espi_init);
> +
> +static void __exit fsl_espi_exit(void)
> +{
> + of_unregister_platform_driver(&fsl_espi_driver);
> +}
> +module_exit(fsl_espi_exit);
> +
> +MODULE_AUTHOR("Mingkai Hu");
> +MODULE_DESCRIPTION("Enhanced Freescale SPI Driver");
This sounds like that this is an enhanced version of the
Freescale SPI driver, which it is not. ;-)
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/spi/spi_fsl_lib.h b/drivers/spi/spi_fsl_lib.h
> index 6ae8949..9c81498 100644
> --- a/drivers/spi/spi_fsl_lib.h
> +++ b/drivers/spi/spi_fsl_lib.h
> @@ -26,6 +26,7 @@ struct mpc8xxx_spi {
> /* rx & tx bufs from the spi_transfer */
> const void *tx;
> void *rx;
> + int len;
I'd place the #ifdef CONFIG_SPI_ESPI, for documentation purposes.
Thanks,
--
Anton Vorontsov
email: cbouatmai...@gmail.com
irc://irc.freenode.net/bd2
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 6/7] mtd: m25p80: add a read function to read page by page
On Thu, Sep 30, 2010 at 11:41:40PM +0900, Grant Likely wrote: > On Thu, Sep 30, 2010 at 11:16 PM, Grant Likely > wrote: > > On Thu, Sep 30, 2010 at 7:46 PM, David Brownell wrote: > >> > >> --- On Thu, 9/30/10, Mingkai Hu wrote: > >> > >>> From: Mingkai Hu > >>> Subject: [PATCH v3 6/7] mtd: m25p80: add a read function to read page by > >>> page > >> > >> NAK. > >> > >> We went over this before. > > > > Yes, I agree with David on this. If large transfers don't work, then > > it is the SPI master driver that is buggy. > > By the way, does this fix your problem? > > https://patchwork.kernel.org/patch/184752/ It shouldn't. AFAIK, eSPI is PIO-only controller, and the overrun fix is for the DMA mode. Thanks, p.s. Btw, in patch 3/7, is_dma_mapped argument of fsl_espi_bufs() is unneeded. -- Anton Vorontsov email: cbouatmai...@gmail.com irc://irc.freenode.net/bd2 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v2 1/3] powerpc/fsl_soc.c: remove FSL USB platform code
On Tue, Sep 28, 2010 at 11:36:32AM +0200, Anatolij Gustschin wrote: > This removed code will be replaced by simple of_platform > driver for creation of FSL USB platform devices which is > added by next patch of the series. > > Signed-off-by: Anatolij Gustschin > Cc: Kumar Gala > Cc: Grant Likely > --- This is not bisectable. You have to merge 1/3 and 2/3. Thanks, -- Anton Vorontsov email: cbouatmai...@gmail.com irc://irc.freenode.net/bd2 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] spi_mpc8xxx: issue with using definition of pram in Device Tree
Hello,
On Fri, Sep 24, 2010 at 09:20:27AM +0200, LEROY Christophe wrote:
> The issue is that cpm_muram_alloc_fixed() allocates memory from the
> general purpose muram area (from 0x0 to 0x1bff).
> Here we need to return a pointer to the parameter RAM, which is
> located somewhere starting at 0x1c00. It is not a dynamic allocation
> that is required here but only to point on the correct location in
> the parameter RAM.
>
> For the CPM2, I don't know. I'm working with a MPC866.
>
> Attached is a previous discussion on the subject where I explain a
> bit more in details the issue.
The patch looks OK, I think.
Doesn't explain why that worked on MPC8272 (CPM2) and MPC8560
(also CPM2) machines though. But here's my guess (I no longer
have these boards to test it):
On 8272 I used this node:
+ s...@4c0 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ compatible = "fsl,cpm2-spi", "fsl,spi";
+ reg = <0x11a80 0x40 0x89fc 0x2>;
On that SOC there are two muram data regions 0x0..0x2000 and
0x9000..0x9100. Note that we actually don't want "data" regions,
and the only reason why that worked is that sysdev/cpm_common.c
maps muram(0)..muram(max).
Thanks,
--
Anton Vorontsov
email: cbouatmai...@gmail.com
irc://irc.freenode.net/bd2
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 1/3 v4] P4080/eLBC: Make Freescale elbc interrupt common to elbc devices
On Fri, Sep 17, 2010 at 03:01:07PM +0800, Roy Zang wrote:
[...]
> +struct fsl_lbc_ctrl {
> + /* device info */
> + struct device *dev;
Forward declaration for struct device is needed (and enough).
> + struct fsl_lbc_regs __iomem *regs;
> + int irq;
> + wait_queue_head_t irq_wait;
#include is needed for this.
> + spinlock_t lock;
#include
[...]
> diff --git a/arch/powerpc/sysdev/fsl_lbc.c b/arch/powerpc/sysdev/fsl_lbc.c
> index dceb8d1..4920cd3 100644
> --- a/arch/powerpc/sysdev/fsl_lbc.c
> +++ b/arch/powerpc/sysdev/fsl_lbc.c
[...]
> +#include
> +#include
I guess of_platform.h is not needed any longer.
> +#include
> +#include
>
[...]
> default:
> return -EINVAL;
> @@ -143,14 +130,18 @@ EXPORT_SYMBOL(fsl_upm_find);
> * thus UPM pattern actually executed. Note that mar usage depends on the
> * pre-programmed AMX bits in the UPM RAM.
> */
> +
No need for this empty line.
> int fsl_upm_run_pattern(struct fsl_upm *upm, void __iomem *io_base, u32 mar)
> {
> int ret = 0;
> unsigned long flags;
[...]
> +static int __devinit fsl_lbc_ctrl_probe(struct platform_device *dev)
> +{
> + int ret;
> +
> + if (!dev->dev.of_node) {
> + dev_err(&dev->dev, "Device OF-Node is NULL");
> + return -EFAULT;
> + }
> + fsl_lbc_ctrl_dev = kzalloc(sizeof(*fsl_lbc_ctrl_dev), GFP_KERNEL);
Btw, the code in the NAND driver checks for !fsl_lbc_ctrl_dev.
So it might make sense to assign the global variable after the
struct is fully initialized.
> + if (!fsl_lbc_ctrl_dev)
> + return -ENOMEM;
> +
> + dev_set_drvdata(&dev->dev, fsl_lbc_ctrl_dev);
> +
> + spin_lock_init(&fsl_lbc_ctrl_dev->lock);
> + init_waitqueue_head(&fsl_lbc_ctrl_dev->irq_wait);
> +
> + fsl_lbc_ctrl_dev->regs = of_iomap(dev->dev.of_node, 0);
> + if (!fsl_lbc_ctrl_dev->regs) {
> + dev_err(&dev->dev, "failed to get memory region\n");
> + ret = -ENODEV;
> + goto err;
> + }
> +
> + fsl_lbc_ctrl_dev->irq = irq_of_parse_and_map(dev->dev.of_node, 0);
> + if (fsl_lbc_ctrl_dev->irq == NO_IRQ) {
> + dev_err(&dev->dev, "failed to get irq resource\n");
> + ret = -ENODEV;
> + goto err;
> + }
> +
> + fsl_lbc_ctrl_dev->dev = &dev->dev;
> +
> + ret = fsl_lbc_ctrl_init(fsl_lbc_ctrl_dev);
> + if (ret < 0)
> + goto err;
> +
> + ret = request_irq(fsl_lbc_ctrl_dev->irq, fsl_lbc_ctrl_irq, 0,
> + "fsl-lbc", fsl_lbc_ctrl_dev);
> + if (ret != 0) {
> + dev_err(&dev->dev, "failed to install irq (%d)\n",
> + fsl_lbc_ctrl_dev->irq);
> + ret = fsl_lbc_ctrl_dev->irq;
> + goto err;
> + }
> +
> + return 0;
> +
> +err:
> + iounmap(fsl_lbc_ctrl_dev->regs);
> + kfree(fsl_lbc_ctrl_dev);
> + return ret;
> +}
> +
> +static const struct of_device_id fsl_lbc_match[] = {
#include is needed for this.
Plus, I think the patch is not runtime bisectable (i.e. you
now do request_irq() here, but not removing it from the nand
driver, so nand will fail to probe).
Thanks,
--
Anton Vorontsov
email: cbouatmai...@gmail.com
irc://irc.freenode.net/bd2
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/3 v4] P4080/mtd: Only make elbc nand driver detect nand flash partitions
On Fri, Sep 17, 2010 at 03:01:08PM +0800, Roy Zang wrote:
[...]
> +static struct mutex fsl_elbc_nand_mutex;
> +
> +static int __devinit fsl_elbc_nand_probe(struct platform_device *dev)
> {
> - struct fsl_lbc_regs __iomem *lbc = ctrl->regs;
> + struct fsl_lbc_regs __iomem *lbc;
> struct fsl_elbc_mtd *priv;
> struct resource res;
> + struct fsl_elbc_fcm_ctrl *elbc_fcm_ctrl = NULL;
No need for = NULL.
[...]
> - ctrl->chips[bank] = priv;
> + mutex_init(&fsl_elbc_nand_mutex);
This may cause all sorts of misbehaviours, e.g.
A: mutex_init(foo)
A: mutex_lock(foo)
B: mutex_init(foo) <- destroyed "A"-context mutex.
A: mutex_unlock(foo) <- oops
Instead of dynamically initializing the mutex, just define it
with DEFINE_MUTEX() above.
(Btw, #include is needed.)
> +
> + mutex_lock(&fsl_elbc_nand_mutex);
[...]
> -static int __devinit fsl_elbc_ctrl_init(struct fsl_elbc_ctrl *ctrl)
> +static int fsl_elbc_nand_remove(struct platform_device *dev)
[...]
> + struct fsl_elbc_fcm_ctrl *elbc_fcm_ctrl = fsl_lbc_ctrl_dev->nand;
[...]
> + if (elbc_fcm_ctrl->chips[i])
> + fsl_elbc_chip_remove(elbc_fcm_ctrl->chips[i]);
[...]
> + fsl_lbc_ctrl_dev->nand = NULL;
> + kfree(elbc_fcm_ctrl);
Will cause NULL dereference and/or use-after-free for other
elbc nand instances. To avoid that, reference counting for
elbc_fcm_ctrl is required.
Thanks,
--
Anton Vorontsov
email: cbouatmai...@gmail.com
irc://irc.freenode.net/bd2
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/3 v3] P4080/mtd: Only make elbc nand driver detect nand flash partitions
On Thu, Sep 16, 2010 at 11:14:48AM -0500, Scott Wood wrote: > > > DEFINE_MUTEX(fsl_elbc_mutex); > > > > I'd place the mutex inside the fsl_lbc_ctrl_dev, > > i.e. fsl_lbc_ctrl_dev->nand_lock. This is to avoid more > > global variables. > > I wouldn't. If the lock is only meaningful to the NAND driver, it > should be declared in the NAND driver. > > Besides, it's not any less of a global just because it's sitting inside > a singleton struct. > > Perhaps it should be declared as a static local inside the probe > function, if it's just to guard against this one race. OK, in that case better be persistent and not introduce fsl_lbc_ctrl_dev->nand at all, as it isn't used outside of the driver. Having fsl_lbc_ctrl_dev->nand and its lock elsewhere in the code makes no sense. > > Btw, even before this patch, it seems that the driver had > > all these bugs/races, i.e. ctrl->controller.lock was not > > used at all. Ugh. > > It is used, search nand_base.c for controller->lock. OK, now I see, the driver implements its own chip->controller (which is exactly what ctrl->controller is). Then we're fine. Thanks, -- Anton Vorontsov email: cbouatmai...@gmail.com irc://irc.freenode.net/bd2 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/3 v3] P4080/mtd: Only make elbc nand driver detect nand flash partitions
On Thu, Sep 16, 2010 at 06:39:40PM +0800, Zang Roy-R61911 wrote:
[...]
> But my code has some assignment for "foo" instead of a simple
> allocation, how about this way for my code:
This will surely work, and all the rest is just a matter of
taste. So, I'm fine with it. But see below, I think I found
some new, quite serious issues.
> DEFINE_MUTEX(fsl_elbc_mutex);
I'd place the mutex inside the fsl_lbc_ctrl_dev,
i.e. fsl_lbc_ctrl_dev->nand_lock. This is to avoid more
global variables.
> ...
> static int __devinit fsl_elbc_nand_probe(struct platform_device *dev)
> {
> ...
> mutex_lock(&fsl_lbc_mutex);
> if (!fsl_lbc_ctrl_dev->nand) {
> elbc_fcm_ctrl = kzalloc(sizeof(*elbc_fcm_ctrl), GFP_KERNEL);
> if (!elbc_fcm_ctrl) {
> dev_err(fsl_lbc_ctrl_dev->dev, "failed to allocate "
> "memory\n");
> ret = -ENOMEM;
> goto err;
> }
>
> elbc_fcm_ctrl->read_bytes = 0;
> elbc_fcm_ctrl->index = 0;
> elbc_fcm_ctrl->addr = NULL;
I guess these variables should be per chip select, as
otherwise there will be tons of races when somebody try
to access two or more NAND chips simultaneously.
(Plus, you don't need these = 0 and = NULL as you used
kzalloc() for allocation.)
>
> spin_lock_init(&elbc_fcm_ctrl->controller.lock);
> init_waitqueue_head(&elbc_fcm_ctrl->controller.wq);
Some of these may need to be per chip select too.
So, I'd suggest to redo the whole thing this way: don't allocate
elbc_fcm_ctrl in this driver, but make an array inside the
fsl_lbc_ctrl_dev. I.e.
fsl_lbc_ctrl_dev->nand_ctrl[MAX_CHIP_SELECTS]
or something like that.
Btw, even before this patch, it seems that the driver had
all these bugs/races, i.e. ctrl->controller.lock was not
used at all. Ugh.
> fsl_lbc_ctrl_dev->nand = elbc_fcm_ctrl;
> } else
> elbc_fcm_ctrl = fsl_lbc_ctrl_dev->nand;
Per coding style this should be
} else {
elbc_fcm_ctrl = fsl_lbc_ctrl_dev->nand;
}
> mutex_unlock(&fsl_lbc_mutex);
>
> elbc_fcm_ctrl->chips[bank] = priv;
> ...
> }
Thanks,
--
Anton Vorontsov
email: cbouatmai...@gmail.com
irc://irc.freenode.net/bd2
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/3 v3] P4080/mtd: Only make elbc nand driver detect nand flash partitions
On Thu, Sep 16, 2010 at 06:08:14PM +0800, Zang Roy-R61911 wrote:
[...]
> Interesting.
> How about this?
> #include
> #include
>
> char *foo;
>
> void probe(void)
> {
> char *bar = NULL;
>
> if (!foo) {
> bar = malloc(sizeof(*bar));
> if (!bar)
> return;
> foo = bar;
> } else
> bar = foo;
This willl work of course; but I'd write it as
foo_lock();
if (!foo)
foo = alloc();
foo_unlock();
bar = foo;
bar->baz;
--
Anton Vorontsov
email: cbouatmai...@gmail.com
irc://irc.freenode.net/bd2
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/3 v3] P4080/mtd: Only make elbc nand driver detect nand flash partitions
On Thu, Sep 16, 2010 at 04:50:05PM +0800, Zang Roy-R61911 wrote:
> > On Thu, Sep 16, 2010 at 02:41:23PM +0800, Roy Zang wrote:
> > [...]
> > > -static int __devinit fsl_elbc_chip_probe(struct fsl_elbc_ctrl *ctrl,
> > > - struct device_node *node)
> > > +/*
> > > + * Currently only one elbc probe is supported.
> > > + */
> > > +static int __devinit fsl_elbc_nand_probe(struct platform_device *dev)
> > > {
> > > - struct fsl_lbc_regs __iomem *lbc = ctrl->regs;
> > > + struct fsl_lbc_regs __iomem *lbc;
> > > struct fsl_elbc_mtd *priv;
> > > struct resource res;
> > > + struct fsl_elbc_fcm_ctrl *elbc_fcm_ctrl = NULL;
> > [...]
> > > - ctrl->chips[bank] = priv;
> > > + if (fsl_lbc_ctrl_dev->nand == NULL) {
> > > + elbc_fcm_ctrl = kzalloc(sizeof(*elbc_fcm_ctrl), GFP_KERNEL);
> > > + if (!elbc_fcm_ctrl) {
> > [...]
> > > + goto err;
> > > + }
> > > + fsl_lbc_ctrl_dev->nand = elbc_fcm_ctrl;
> > > + }
> > > +
> > > + elbc_fcm_ctrl->chips[bank] = priv;
> >
> > Again, this will oops on the second probe.
> Why?
Because of a NULL dereference ("elbc_fcm_ctrl->").
I understand that you don't have to believe me, but will you believe
a compiler?
oksana:~$ cat a.c
#include
#include
char *foo;
void probe(void)
{
char *bar = NULL;
if (!foo) {
bar = malloc(sizeof(*bar));
if (!bar)
return;
foo = bar;
}
*bar = 'a';
}
int main(void)
{
probe();
probe();
return 0;
}
oksana:~$ gcc a.c && ./a.out
Segmentation fault
--
Anton Vorontsov
email: cbouatmai...@gmail.com
irc://irc.freenode.net/bd2
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/3 v3] P4080/mtd: Only make elbc nand driver detect nand flash partitions
On Thu, Sep 16, 2010 at 02:41:23PM +0800, Roy Zang wrote:
[...]
> -static int __devinit fsl_elbc_chip_probe(struct fsl_elbc_ctrl *ctrl,
> - struct device_node *node)
> +/*
> + * Currently only one elbc probe is supported.
> + */
> +static int __devinit fsl_elbc_nand_probe(struct platform_device *dev)
> {
> - struct fsl_lbc_regs __iomem *lbc = ctrl->regs;
> + struct fsl_lbc_regs __iomem *lbc;
> struct fsl_elbc_mtd *priv;
> struct resource res;
> + struct fsl_elbc_fcm_ctrl *elbc_fcm_ctrl = NULL;
[...]
> - ctrl->chips[bank] = priv;
> + if (fsl_lbc_ctrl_dev->nand == NULL) {
> + elbc_fcm_ctrl = kzalloc(sizeof(*elbc_fcm_ctrl), GFP_KERNEL);
> + if (!elbc_fcm_ctrl) {
[...]
> + goto err;
> + }
> + fsl_lbc_ctrl_dev->nand = elbc_fcm_ctrl;
> + }
> +
> + elbc_fcm_ctrl->chips[bank] = priv;
Again, this will oops on the second probe. And you still don't
lock fsl_lbc_ctrl_dev->nand during check-then-assignment, so
with parallel probing there will be a race. :-(
We do have boards with several NAND chips (e.g.
arch/powerpc/boot/dts/mpc8610_hpcd.dts), so these issues
are all legitimate.
Thanks,
--
Anton Vorontsov
email: cbouatmai...@gmail.com
irc://irc.freenode.net/bd2
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 3/3 v3] P4080/mtd: Fix the freescale lbc issue with 36bit mode
On Thu, Sep 16, 2010 at 02:41:24PM +0800, Roy Zang wrote:
> From: Lan Chunhe-B25806
>
> When system uses 36bit physical address, res.start is 36bit
> physical address. But the function of in_be32 returns 32bit
> physical address. Then both of them compared each other is
> wrong. So by converting the address of res.start into
> the right format fixes this issue.
>
> Signed-off-by: Lan Chunhe-B25806
> Signed-off-by: Roy Zang
> ---
> arch/powerpc/include/asm/fsl_lbc.h |1 +
> arch/powerpc/sysdev/fsl_lbc.c | 23 ++-
> drivers/mtd/nand/fsl_elbc_nand.c |2 +-
> 3 files changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/fsl_lbc.h
> b/arch/powerpc/include/asm/fsl_lbc.h
> index db94698..5638b1e 100644
> --- a/arch/powerpc/include/asm/fsl_lbc.h
> +++ b/arch/powerpc/include/asm/fsl_lbc.h
> @@ -246,6 +246,7 @@ struct fsl_upm {
> int width;
> };
>
> +extern unsigned int fsl_lbc_addr(phys_addr_t addr_base);
u32 here.
Other than that, the patch looks good.
Reviewed-by: Anton Vorontsov
Thanks!
--
Anton Vorontsov
email: cbouatmai...@gmail.com
irc://irc.freenode.net/bd2
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 1/3 v3] P4080/eLBC: Make Freescale elbc interrupt common to elbc devices
On Thu, Sep 16, 2010 at 02:41:22PM +0800, Roy Zang wrote:
[...]
> +static const struct platform_device_id fsl_lbc_match[] = {
> + { "fsl,elbc", },
> + { "fsl,pq3-localbus", },
> + { "fsl,pq2-localbus", },
> + { "fsl,pq2pro-localbus", },
> + {},
> +};
> +
> +static struct platform_driver fsl_lbc_ctrl_driver = {
> + .driver = {
> + .name = "fsl-lbc",
> + },
> + .probe = fsl_lbc_ctrl_probe,
> + .id_table = fsl_lbc_match,
> +};
No, it won't work that way (at least not w/o a device constructor
somewhere in fsl_soc.c). Instead, you should write something like
static const struct of_device_id fsl_lbc_match[] = {
...
};
static struct platform_driver fsl_lbc_ctrl_driver = {
.driver = {
.name = "fsl-lbc",
.of_match_table = fsl_lbc_match;
}
...
};
(Note that platform_driver.driver has of_match_table nowadays --
that's what makes it possible to seamlessly transit from
of_platform_driver to platform_driver.)
The same applies for the second patch as well.
Thanks,
--
Anton Vorontsov
email: cbouatmai...@gmail.com
irc://irc.freenode.net/bd2
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: How to define an I2C-to-SPI bridge device ?
On Fri, Sep 10, 2010 at 08:14:44PM +0200, André Schwarz wrote: [...] > > Does the device actually generate edge interrupts? Or is it a level > > irq device? If it is a level irq device, then the correct way to > > handle this is to disable the irq line so that the event can be > > handled at non-irq context, and then reenable it when finished. > > The irq is level-low active. > Will do it via disable/re-enable then. FYI, In newer kernels you don't have to do it manually, there's request_threaded_irq() for this. -- Anton Vorontsov email: cbouatmai...@gmail.com irc://irc.freenode.net/bd2 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 1/3 v2][MTD] P4080/eLBC: Make Freescale elbc interrupt common to elbc devices
On Fri, Sep 10, 2010 at 02:58:15PM +0800, Zang Roy-R61911 wrote:
[...]
> > > +static struct of_platform_driver fsl_lbc_ctrl_driver = {
> >
> > Need linux/of_platform.h for this.
> It has been include by
> fsl_lbc.h->linux/of_platform.h-> linux/platform_device.h
> Before submitting the patch, I have built and tested it.
In Linux we try to include all the headers explicitly (except
for asm/* if the same header name exists in linux/).
That's to avoid problems if for some reason fsl_lbc.h will stop
including of_plaform.h some day.
Oh, and by the way, there is absolutely no reason to add
linux/of_platform.h and interrupts.h into fsl_lbc.h, they're
is simply not needed in fsl_lbc.h.
> Do you think I do not build the tree before I send out the patch?
Nope, that's not what I think. I didn't say that the file
won't build w/o these fixes, but they're still needed.
> > > +
> > > +static struct of_platform_driver fsl_lbc_ctrl_driver = {
> >
> > Need linux/of_platform.h for this.
> >
> > But you actually don't need of_platform_driver, as for the
> > new code you can use platform_driver (and thus
> > linux/platform_device.h).
> I'd prefer using of_platform_driver here for simplified code.
> Any special reason to use platform_device here?
In the new kernels, of_platform_driver is almost a synonym of
platform_driver, and 'of_platform_driver' stuff is soon to be
deleted. You can use platform_driver just like
of_platform_driver nowadays.
Thanks,
--
Anton Vorontsov
email: cbouatmai...@gmail.com
irc://irc.freenode.net/bd2
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 1/3 v2][MTD] P4080/eLBC: Make Freescale elbc interrupt common to elbc devices
SR 0x%08X\n", status);
> + if (status & LTESR_CS)
> + dev_err(ctrl->dev, "Chip select error: "
> + "LTESR 0x%08X\n", status);
> + if (status & LTESR_UPM)
> + ;
> + if (status & LTESR_FCT) {
> + dev_err(ctrl->dev, "FCM command time-out: "
> + "LTESR 0x%08X\n", status);
> + smp_wmb();
> + wake_up(&ctrl->irq_wait);
> + }
> + if (status & LTESR_PAR) {
> + dev_err(ctrl->dev, "Parity or Uncorrectable ECC error: "
> + "LTESR 0x%08X\n", status);
> + smp_wmb();
> + wake_up(&ctrl->irq_wait);
> + }
> + if (status & LTESR_CC) {
> + smp_wmb();
> + wake_up(&ctrl->irq_wait);
> + }
> + if (status & ~LTESR_MASK)
> + dev_err(ctrl->dev, "Unknown error: "
> + "LTESR 0x%08X\n", status);
> +
> + return IRQ_HANDLED;
> + }
> +
> + return IRQ_NONE;
> +}
> +
> +/*
> + * fsl_lbc_ctrl_probe
> + *
> + * called by device layer when it finds a device matching
> + * one our driver can handled. This code allocates all of
> + * the resources needed for the controller only. The
> + * resources for the NAND banks themselves are allocated
> + * in the chip probe function.
> +*/
> +
> +static int __devinit fsl_lbc_ctrl_probe(struct platform_device *ofdev,
> + const struct of_device_id *match)
> +{
> + int ret;
> +
> + if (!ofdev->dev.of_node) {
> + dev_err(&ofdev->dev, "Device OF-Node is NULL");
> + return -EFAULT;
> + }
> + fsl_lbc_ctrl_dev = kzalloc(sizeof(*fsl_lbc_ctrl_dev), GFP_KERNEL);
> + if (!fsl_lbc_ctrl_dev)
> + return -ENOMEM;
> +
> + dev_set_drvdata(&ofdev->dev, fsl_lbc_ctrl_dev);
> +
> + spin_lock_init(&fsl_lbc_ctrl_dev->lock);
> + init_waitqueue_head(&fsl_lbc_ctrl_dev->irq_wait);
> +
> + fsl_lbc_ctrl_dev->regs = of_iomap(ofdev->dev.of_node, 0);
> + if (!fsl_lbc_ctrl_dev->regs) {
> + dev_err(&ofdev->dev, "failed to get memory region\n");
> + ret = -ENODEV;
> + goto err;
> + }
> +
> + fsl_lbc_ctrl_dev->irq = irq_of_parse_and_map(ofdev->dev.of_node, 0);
> + if (fsl_lbc_ctrl_dev->irq == NO_IRQ) {
> + dev_err(&ofdev->dev, "failed to get irq resource\n");
> + ret = -ENODEV;
> + goto err;
> + }
> +
> + fsl_lbc_ctrl_dev->dev = &ofdev->dev;
> +
> + ret = fsl_lbc_ctrl_init(fsl_lbc_ctrl_dev);
> + if (ret < 0)
> + goto err;
> +
> + ret = request_irq(fsl_lbc_ctrl_dev->irq, fsl_lbc_ctrl_irq, 0,
> + "fsl-lbc", fsl_lbc_ctrl_dev);
> + if (ret != 0) {
> + dev_err(&ofdev->dev, "failed to install irq (%d)\n",
> + fsl_lbc_ctrl_dev->irq);
> + ret = fsl_lbc_ctrl_dev->irq;
> + goto err;
> + }
> +
> + return 0;
> +
> +err:
> + iounmap(fsl_lbc_ctrl_dev->regs);
> + kfree(fsl_lbc_ctrl_dev);
> + return ret;
> +}
> +
> +static const struct of_device_id fsl_lbc_match[] = {
> + { .compatible = "fsl,elbc", },
> + { .compatible = "fsl,pq3-localbus", },
> + { .compatible = "fsl,pq2-localbus", },
> + { .compatible = "fsl,pq2pro-localbus", },
> + {},
> +};
You need linux/mod_devicetable.h for this.
> +
> +static struct of_platform_driver fsl_lbc_ctrl_driver = {
Need linux/of_platform.h for this.
But you actually don't need of_platform_driver, as for the
new code you can use platform_driver (and thus
linux/platform_device.h).
> + .driver = {
> + .name = "fsl-lbc",
> + .of_match_table = fsl_lbc_match,
> + },
> + .probe = fsl_lbc_ctrl_probe,
> +};
> +
> +static int __init fsl_lbc_init(void)
> +{
> + return of_register_platform_driver(&fsl_lbc_ctrl_driver);
> +}
> +
No need for this empty line.
> +module_init(fsl_lbc_init);
Thanks,
--
Anton Vorontsov
email: cbouatmai...@gmail.com
irc://irc.freenode.net/bd2
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v2 3/3][MTD] P4080/mtd: Fix the freescale lbc issue with 36bit mode
On Thu, Sep 09, 2010 at 06:20:32PM +0800, Roy Zang wrote:
[...]
> /**
> + * fsl_lbc_addr - convert the base address
> + * @addr_base: base address of the memory bank
> + *
> + * This function converts a base address of lbc into the right format for
> the BR
> + * registers. If the SOC has eLBC then it returns 32bit physical address else
> + * it returns 34bit physical address for local bus(Example: MPC8641).
> + */
It returns 34bit physical address encoded in a 32 bit word,
right? Because, IIRC, 'unsigned int' is always 32 bit.
Worth mentioning this fact.
> +unsigned int fsl_lbc_addr(phys_addr_t addr_base)
> +{
> + void *dev;
struct device_node *np;
> + int compatible;
> +
> + dev = fsl_lbc_ctrl_dev->dev->of_node;
> + compatible = of_device_is_compatible(dev, "fsl,elbc");
> +
> + if (compatible)
> + return addr_base & 0x8000;
> + else
> + return (addr_base & 0x08000ull)
> + | ((addr_base & 0x3ull) >> 19);
> +}
> +EXPORT_SYMBOL(fsl_lbc_addr);
Almost perfect. I'm not sure if 'unsigned int' is technically
correct return type for this function though. I guess it should
be u32.
Also, the function may be a bit more understandable and shorter:
u32 fsl_lbc_addr(phys_addr_t addr)
{
struct device_node *np = fsl_lbc_ctrl_dev->dev->of_node;
u32 addrl = addr & 0x8000;
if (of_device_is_compatible(np, "fsl,elbc"))
return addrl;
return addrl | ((addr & 0x3ull) >> 19);
}
EXPORT_SYMBOL(fsl_lbc_addr);
Thanks,
--
Anton Vorontsov
email: cbouatmai...@gmail.com
irc://irc.freenode.net/bd2
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/3 v2][MTD] P4080/mtd: Only make elbc nand driver detect nand flash partitions
ank] = priv;
The driver will oops on the second probe.
You probably meant
struct fsl_elbc_fcm_ctrl *elbc_fcm_ctrl = fsl_lbc_ctrl_dev->nand;
at (***1).
Also, nowadays the kernel may probe devices in parallel, which
means that you probably need a mutex for fsl_lbc_ctrl_dev->nand.
[...]
> -static const struct of_device_id fsl_elbc_match[] = {
> +static const struct of_device_id fsl_elbc_nand_match[] = {
linux/mod_devicetable.h is needed for this.
> {
> - .compatible = "fsl,elbc",
> + .compatible = "fsl,elbc-fcm-nand",
> },
> {}
> };
>
> -static struct of_platform_driver fsl_elbc_ctrl_driver = {
> +static struct of_platform_driver fsl_elbc_nand_driver = {
If you write of_platform_driver, you need linux/of_platform.h (which
you removed in this patch).
But I think that you need just 'struct platform_driver' here, and
include linux/platform_device.h.
> .driver = {
> - .name = "fsl-elbc",
> + .name = "fsl,elbc-fcm-nand",
> .owner = THIS_MODULE,
> - .of_match_table = fsl_elbc_match,
> + .of_match_table = fsl_elbc_nand_match,
> },
> - .probe = fsl_elbc_ctrl_probe,
> - .remove = fsl_elbc_ctrl_remove,
> + .probe = fsl_elbc_nand_probe,
> + .remove = fsl_elbc_nand_remove,
> };
Thanks,
--
Anton Vorontsov
email: cbouatmai...@gmail.com
irc://irc.freenode.net/bd2
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 0/8] sdhci: Move real work out of an atomic context
On Thu, Sep 09, 2010 at 03:28:34AM +0100, Chris Ball wrote: [...] > [7.372843] [] __might_sleep+0xd9/0xe0 > [7.387864] [] mutex_lock+0x1c/0x2a > [7.402576] [] sdhci_led_control+0x1a/0x41 > [7.417727] [] led_trigger_event+0x42/0x5c led_trigger_even grabs a readlock. :-( > [7.432807] [] mmc_request_done+0x56/0x6f > [7.447597] [] sdhci_finish_work+0xc8/0xcd > [7.462643] [] ? sdhci_finish_work+0x0/0xcd > [7.477941] [] worker_thread+0x165/0x1ed > [7.492856] [] ? sdhci_finish_work+0x0/0xcd > [7.508204] [] ? autoremove_wake_function+0x0/0x34 > [7.524178] [] ? worker_thread+0x0/0x1ed > [7.538953] [] kthread+0x63/0x68 > [7.552659] [] ? kthread+0x0/0x68 > [7.566349] [] kernel_thread_helper+0x6/0x10 > [7.709931] udev: starting version 141 > [7.940374] mmc2: new high speed SDHC card at address e4da > [8.058165] mmcblk0: mmc2:e4da SU04G 3.69 GiB > [8.135730] mmcblk0: p1 p2 > > Full dmesg is at http://chris.printf.net/anton-mutex-dmesg.txt. > Anton, the kernel is 2.6.35.4-olpc plus your patchset from -mm. > I can think about how to test on an upstream kernel instead, but > perhaps your own tests simply didn't hit sdhci_led_control(). Yep, LEDS support was turned off. > Andrew, if you want to drop this while the BUG() and potential > performance regressions are worked out, I'd be happy to keep > testing patches from Anton until it's without regressions here. Thanks Chris. I also think that it's better to drop these series now, and meanwhile I'll try to prepare another patchset. -- Anton Vorontsov email: cbouatmai...@gmail.com irc://irc.freenode.net/bd2 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 0/8] sdhci: Move real work out of an atomic context
On Wed, Sep 08, 2010 at 11:05:48PM +0100, Chris Ball wrote: > Hi Anton, > > On Thu, Sep 09, 2010 at 01:57:50AM +0400, Anton Vorontsov wrote: > > Thanks! > > > > Would be also great if you could point out which patch causes > > most of the performance drop (if any)? > > > > Albert, if you could find time, can you also "bisect" the > > patchset? I wouldn't want to buy Nintendo WII just to debug the > > perf regression. ;-) FWIW, I tried to disable multiblock > > read/writes and test with SD cards, and still didn't notice > > any performance drops. > > > > Maybe it's SDIO IRQs that cause the performance drop for the > > WII case, as we delay them a little bit? Or it could be the > > patch that introduces threaded IRQ handler in whole causes > > it. If so, I guess we'd need to move some of the processing to > > the real IRQ context, keeping the handler lockless (if > > possible) or introducing a very fine grained locking. > > I didn't know anything about a reported performance drop, and I don't > think Andrew did either -- Albert's test results don't seem to have > made it to this list, or anywhere else that I can see. Could you > link to/repost his comments? > > (I'll be testing with libertas, so that will stress-test SDIO IRQs.) Sure thing, here are Albert's results. - Forwarded message from Albert Herranz - Date: Mon, 02 Aug 2010 21:23:51 +0200 From: Albert Herranz To: Anton Vorontsov CC: a...@linux-foundation.org, mm-comm...@vger.kernel.org, ben-li...@fluff.org, m...@console-pimps.org, pie...@ossman.eu, w.s...@pengutronix.de, m...@bu3sch.de Subject: Re: + sdhci-use-work-structs-instead-of-tasklets.patch added to -mm tree Hi, Some initial numbers regarding performance. The patchset seems to cause a noticeable performance drop. I've run two iperf client tests (see the two invocations of iperf -c) and two iperf server tests (see iperf -s invocation). == 2.6.33 == $ iperf -c 192.168.1.130 Client connecting to 192.168.1.130, TCP port 5001 TCP window size: 16.0 KByte (default) [ 3] local 192.168.1.127 port 40119 connected with 192.168.1.130 port 5001 [ ID] Interval Transfer Bandwidth [ 3] 0.0-10.1 sec 1.05 MBytes872 Kbits/sec $ iperf -c 192.168.1.130 Client connecting to 192.168.1.130, TCP port 5001 TCP window size: 16.0 KByte (default) [ 3] local 192.168.1.127 port 40120 connected with 192.168.1.130 port 5001 [ ID] Interval Transfer Bandwidth [ 3] 0.0-10.0 sec 1.04 MBytes870 Kbits/sec $ iperf -s Server listening on TCP port 5001 TCP window size: 85.3 KByte (default) [ 4] local 192.168.1.127 port 5001 connected with 192.168.1.130 port 36691 [ ID] Interval Transfer Bandwidth [ 4] 0.0-10.2 sec 3.61 MBytes 2.98 Mbits/sec [ 5] local 192.168.1.127 port 5001 connected with 192.168.1.130 port 36692 [ 5] 0.0-10.1 sec 4.94 MBytes 4.09 Mbits/sec == 2.6.33 + "sdhci: Move real work out of an atomic context" patchset == $ iperf -c 192.168.1.130 Client connecting to 192.168.1.130, TCP port 5001 TCP window size: 16.0 KByte (default) [ 3] local 192.168.1.127 port 39210 connected with 192.168.1.130 port 5001 [ ID] Interval Transfer Bandwidth [ 3] 0.0-10.0 sec368 KBytes301 Kbits/sec $ iperf -c 192.168.1.130 Client connecting to 192.168.1.130, TCP port 5001 TCP window size: 16.0 KByte (default) [ 3] local 192.168.1.127 port 39211 connected with 192.168.1.130 port 5001 [ ID] Interval Transfer Bandwidth [ 3] 0.0-10.2 sec440 KBytes354 Kbits/sec $ iperf -s Server listening on TCP port 5001 TCP window size: 85.3 KByte (default) [ 4] local 192.168.1.127 port 5001 connected with 192.168.1.130 port 57833 [ ID] Interval Transfer Bandwidth [ 4] 0.0-10.2 sec 2.37 MBytes 1.95 Mbits/sec [ 5] local 192.168.1.127 port 5001 connected with 192.168.1.130 port 57834 [ 5] 0.0-10.2 sec 2.30 MBytes 1.90 Mbits/sec The subjective feeling is too that the system is slower. Cheers, Albert - End forwarded message - ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 0/8] sdhci: Move real work out of an atomic context
On Wed, Sep 08, 2010 at 10:37:41PM +0100, Chris Ball wrote: > Hi Andrew, > > On Tue, Sep 07, 2010 at 03:38:13PM -0700, Andrew Morton wrote: > > > I noticed no throughput drop neither with PIO transfers nor > > > with DMA (tested on MPC8569E CPU), while latencies should be > > > greatly improved. > > > > This patchset isn't causing any problems yet, but may do so in the > > future and will impact the validity of any testing. It seems to be > > kind of stuck. Should I drop it all? > > I suggest keeping it -- I'll find time to test it out here soon, and > will keep it in mind as a possible regression cause. Thanks! Would be also great if you could point out which patch causes most of the performance drop (if any)? Albert, if you could find time, can you also "bisect" the patchset? I wouldn't want to buy Nintendo WII just to debug the perf regression. ;-) FWIW, I tried to disable multiblock read/writes and test with SD cards, and still didn't notice any performance drops. Maybe it's SDIO IRQs that cause the performance drop for the WII case, as we delay them a little bit? Or it could be the patch that introduces threaded IRQ handler in whole causes it. If so, I guess we'd need to move some of the processing to the real IRQ context, keeping the handler lockless (if possible) or introducing a very fine grained locking. Thanks, -- Anton Vorontsov email: cbouatmai...@gmail.com irc://irc.freenode.net/bd2 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 1/3][MTD] P4080/eLBC: Make Freescale elbc interrupt common to elbc devices
On Mon, Sep 06, 2010 at 05:24:35PM +0800, Zang Roy-R61911 wrote:
[..]
> > mxmr = &fsl_lbc_ctrl_dev->regs->mcmr;
> That makes sense. A global or local variable for fsl_lbc_ctrl_dev->regs?
> Which one is better?
The less global variables, the better. So, I'd vote for
a local one.
> > [...]
> > > > > +static int __devinit fsl_lbc_ctrl_probe(struct of_device *ofdev,
> > > > > + const struct of_device_id
> > > > > *match)
> > > > > +{
> > > > > + int ret = 0;
> > > >
> > > > no need for the initial value here.
> > > Any harm?
> >
> > Probably not as gcc will likely optimize it away,
> > but it's not needed, so why keep it there?
> habit.
;-)
Thanks,
--
Anton Vorontsov
email: cbouatmai...@gmail.com
irc://irc.freenode.net/bd2
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 1/3][MTD] P4080/eLBC: Make Freescale elbc interrupt common to elbc devices
On Mon, Sep 06, 2010 at 11:38:09AM +0800, Zang Roy-R61911 wrote:
[...]
> > > switch (br & BR_MSEL) {
> > > case BR_MS_UPMA:
> > > - upm->mxmr = &fsl_lbc_regs->mamr;
> > > + upm->mxmr = &fsl_lbc_ctrl_dev->regs->mamr;
> >
> > Ditto, a very repetitive stuff, desires a variable for regs?
> But the fact is that the variable represents different reg
> address according to the condition. It will be ugly to use
> the reg address directoly.
I meant a dedicated var for 'fsl_lbc_ctrl_dev->regs'.
I.e.
regs = fsl_lbc_ctrl_dev->regs;
...
mxmr = ®s->mamr;
...
mxmr = ®s->mbmr;
..
mxmr = ®s->mcmr;
Instead of
mxmr = &fsl_lbc_ctrl_dev->regs->mamr;
...
mxmr = &fsl_lbc_ctrl_dev->regs->mbmr;
..
mxmr = &fsl_lbc_ctrl_dev->regs->mcmr;
[...]
> > > +static int __devinit fsl_lbc_ctrl_probe(struct of_device *ofdev,
> > > + const struct of_device_id *match)
> > > +{
> > > + int ret = 0;
> >
> > no need for the initial value here.
> Any harm?
Probably not as gcc will likely optimize it away,
but it's not needed, so why keep it there?
Thanks,
--
Anton Vorontsov
email: cbouatmai...@gmail.com
irc://irc.freenode.net/bd2
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/3][MTD] P4080/nand: Only make elbc nand driver detect nand flash partitions
On Mon, Sep 06, 2010 at 12:49:17PM +0800, Zang Roy-R61911 wrote: > > On Fri, Aug 06, 2010 at 10:51:35AM +0800, Roy Zang wrote: > > [...] > > > > > > +static struct fsl_elbc_fcm_ctrl *elbc_fcm_ctrl; > > > + > > > > Are you sure that you want it as a global var? A bit scary change. > > > > Oh, you probably don't need it, as you can get it from > > fsl_lbc_ctrl_dev->nand? > Get it form fsl_lbc_ctrl_dev->nand or assign it to > fsl_lbc_ctrl_dev->nand in probe? I meant to get it from fsl_lbc_ctrl_dev->nand. I.e. in probe() you do: fsl_lbc_ctrl_dev->nand = elbc_fcm_ctrl, so you probably don't need the global var. (fsl_lbc_ctrl_dev seems to be global as well, duh. Well, one variable less in the global name space. But I'd probably use lbc_np->data to store the LBC private struct). Scott seem to be fine with it as there are probably no plans to to add several localbus controllers into the SoCs. But, I saw a custom board with two MPC82xx SoCs connected together, one as a master (core + peripheral devs), and other as a slave (its core was halted, and only slave's CPM peripheral devices were used by the master CPU). I think it is possible to connect two (or more) SoCs in a such way so that two or more LBC controllers would be visible for the Linux. Thanks, -- Anton Vorontsov email: cbouatmai...@gmail.com irc://irc.freenode.net/bd2 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: How to define an I2C-to-SPI bridge device ?
On Fri, Sep 03, 2010 at 10:36:19AM +0200, André Schwarz wrote:
> Hi,
>
> we're about to get new MPC8377 based hardware with various peripherals.
> There are two I2C-to-SPI bridge devices (NXP SC18IS602) and I'm not sure
> how to define a proper dts...
>
> Of course it's an easy thing creating 2 child nodes on the CPU's I2C
> device - but how can I represent the created SPI bus ?
Um.. the same as the other SPI buses? I.e.
i2c-controller { /* SOC I2C controller */
spi-controller { /* The I2C-to-SPI bridge */
spi-dev...@0 {
};
spi-dev...@1 {
};
};
};
> Is the (possibly) required driver (of_sc18is60x_spi ?) supposed to be an
> I2C slave or an SPI host driver ?
It should be an I2C driver that registers an SPI master (i.e.
calls spi_alloc_master() and spi_register_master()).
Thanks,
--
Anton Vorontsov
email: cbouatmai...@gmail.com
irc://irc.freenode.net/bd2
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/3][MTD] P4080/nand: Only make elbc nand driver detect nand flash partitions
On Fri, Aug 06, 2010 at 10:51:35AM +0800, Roy Zang wrote: [...] > > +static struct fsl_elbc_fcm_ctrl *elbc_fcm_ctrl; > + Are you sure that you want it as a global var? A bit scary change. Oh, you probably don't need it, as you can get it from fsl_lbc_ctrl_dev->nand? I wonder if Scott saw these patches? Cc'ed. -- Anton Vorontsov email: cbouatmai...@gmail.com irc://irc.freenode.net/bd2 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 3/3][MTD] P4080/nand: Fix the freescale lbc issue with 36bit mode
On Fri, Aug 06, 2010 at 10:51:36AM +0800, Roy Zang wrote:
[...]
> /**
> + * convert_lbc_address - convert the base address
> + * @addr_base: base address of the memory bank
> + *
> + * This function converts a base address of lbc into the right format for
> the BR
> + * registers. If the SOC has eLBC then it returns 32bit physical address else
> + * it returns 34bit physical address for local bus(Example: MPC8641).
> + */
> +unsigned int convert_lbc_address(phys_addr_t addr_base)
> +{
> + void *dev;
> + int compatible;
> +
> + dev = of_find_node_by_name(NULL, "localbus");
Nope, you shouldn't do this. Never search by name.
Also, aren't there already a global dev, which was found by the
_probe() stuff?
> + if (!dev) {
> + printk(KERN_INFO "fsl-lbc: can't find localbus node\n");
> + of_node_put(dev);
> + return 0;
> + }
> +
> + compatible = of_device_is_compatible(dev, "fsl,elbc");
> + of_node_put(dev);
> + if (compatible)
> + return addr_base & 0x8000;
> + else
> + return (addr_base & 0x08000ull) \
> + | ((addr_base & 0x3ull) >> 19);
> +}
> +EXPORT_SYMBOL(convert_lbc_address);
> +
> +/**
> * fsl_lbc_find - find Localbus bank
> * @addr_base: base address of the memory bank
> *
> @@ -50,7 +80,8 @@ int fsl_lbc_find(phys_addr_t addr_base)
> __be32 br = in_be32(&fsl_lbc_ctrl_dev->regs->bank[i].br);
> __be32 or = in_be32(&fsl_lbc_ctrl_dev->regs->bank[i].or);
>
> - if (br & BR_V && (br & or & BR_BA) == addr_base)
> + if (br & BR_V && (br & or & BR_BA) \
No need for "\" at the end of the line, keep == on the same line.
> + == convert_lbc_address(addr_base))
Would be prettier if you name it fsl_lbc_addr(). Keeps prefix
the same for the rest of the file, plus makes it shorter (so
there probably won't be any need for breaking the line).
> return i;
> }
>
> diff --git a/drivers/mtd/nand/fsl_elbc_nand.c
> b/drivers/mtd/nand/fsl_elbc_nand.c
> index 7bbcb3f..0e8dc40 100644
> --- a/drivers/mtd/nand/fsl_elbc_nand.c
> +++ b/drivers/mtd/nand/fsl_elbc_nand.c
> @@ -838,7 +838,7 @@ static int __devinit fsl_elbc_nand_probe(struct of_device
> *dev,
> (in_be32(&lbc->bank[bank].br) & BR_MSEL) == BR_MS_FCM &&
> (in_be32(&lbc->bank[bank].br) &
>in_be32(&lbc->bank[bank].or) & BR_BA)
> - == res.start)
> + == convert_lbc_address(res.start))
> break;
>
> if (bank >= MAX_BANKS) {
> --
> 1.5.6.5
Thanks,
--
Anton Vorontsov
email: cbouatmai...@gmail.com
irc://irc.freenode.net/bd2
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 1/3][MTD] P4080/eLBC: Make Freescale elbc interrupt common to elbc devices
On Fri, Aug 06, 2010 at 10:51:34AM +0800, Roy Zang wrote:
> From: Lan Chunhe-B25806
>
> Move Freescale elbc interrupt from nand dirver to elbc driver.
> Then all elbc devices can use the interrupt instead of ONLY nand.
>
> Signed-off-by: Lan Chunhe-B25806
> Signed-off-by: Roy Zang
> ---
> send the patch to linux-...@lists.infradead.org
> it has been posted to linuxppc-...@ozlabs.org and do not get any comment.
>
> arch/powerpc/Kconfig |7 +-
> arch/powerpc/include/asm/fsl_lbc.h | 34 +-
> arch/powerpc/sysdev/fsl_lbc.c | 254
> ++--
> 3 files changed, 253 insertions(+), 42 deletions(-)
>
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 2031a28..5155b53 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -703,9 +703,12 @@ config 4xx_SOC
> bool
>
> config FSL_LBC
> - bool
> + bool "Freescale Local Bus support"
> + depends on FSL_SOC
> help
> - Freescale Localbus support
> + Enables reporting of errors from the Freescale local bus
> + controller. Also contains some common code used by
> + drivers for specific local bus peripherals.
>
> config FSL_GTM
> bool
[...]
> diff --git a/arch/powerpc/sysdev/fsl_lbc.c b/arch/powerpc/sysdev/fsl_lbc.c
> index dceb8d1..9c9e44f 100644
> --- a/arch/powerpc/sysdev/fsl_lbc.c
> +++ b/arch/powerpc/sysdev/fsl_lbc.c
> @@ -5,6 +5,10 @@
> *
> * Author: Anton Vorontsov
> *
> + * Copyright (c) 2010 Freescale Semiconductor
> + *
> + * Authors: Jack Lan
Would be prettier if you'd group copyright and authorship notices.
I.e.
Copyright 2008 MV
Copyright 2010 FSL
Authors: Anton
Jack
> + *
> * This program is free software; you can redistribute it and/or modify
> * it under the terms of the GNU General Public License as published by
> * the Free Software Foundation; either version 2 of the License, or
> @@ -23,35 +27,8 @@
> #include
>
> static spinlock_t fsl_lbc_lock = __SPIN_LOCK_UNLOCKED(fsl_lbc_lock);
> -static struct fsl_lbc_regs __iomem *fsl_lbc_regs;
> -
> -static char __initdata *compat_lbc[] = {
> - "fsl,pq2-localbus",
> - "fsl,pq2pro-localbus",
> - "fsl,pq3-localbus",
> - "fsl,elbc",
> -};
> -
> -static int __init fsl_lbc_init(void)
> -{
> - struct device_node *lbus;
> - int i;
> -
> - for (i = 0; i < ARRAY_SIZE(compat_lbc); i++) {
> - lbus = of_find_compatible_node(NULL, NULL, compat_lbc[i]);
> - if (lbus)
> - goto found;
> - }
> - return -ENODEV;
> -
> -found:
> - fsl_lbc_regs = of_iomap(lbus, 0);
> - of_node_put(lbus);
> - if (!fsl_lbc_regs)
> - return -ENOMEM;
> - return 0;
> -}
> -arch_initcall(fsl_lbc_init);
> +struct fsl_lbc_ctrl *fsl_lbc_ctrl_dev;
> +EXPORT_SYMBOL(fsl_lbc_ctrl_dev);
>
> /**
> * fsl_lbc_find - find Localbus bank
> @@ -66,12 +43,12 @@ int fsl_lbc_find(phys_addr_t addr_base)
> {
> int i;
>
> - if (!fsl_lbc_regs)
> + if (!fsl_lbc_ctrl_dev || !fsl_lbc_ctrl_dev->regs)
> return -ENODEV;
>
> - for (i = 0; i < ARRAY_SIZE(fsl_lbc_regs->bank); i++) {
> - __be32 br = in_be32(&fsl_lbc_regs->bank[i].br);
> - __be32 or = in_be32(&fsl_lbc_regs->bank[i].or);
> + for (i = 0; i < ARRAY_SIZE(fsl_lbc_ctrl_dev->regs->bank); i++) {
> + __be32 br = in_be32(&fsl_lbc_ctrl_dev->regs->bank[i].br);
> + __be32 or = in_be32(&fsl_lbc_ctrl_dev->regs->bank[i].or);
A dedicated variable for regs would make this much more readable?
>
> if (br & BR_V && (br & or & BR_BA) == addr_base)
> return i;
> @@ -99,17 +76,20 @@ int fsl_upm_find(phys_addr_t addr_base, struct fsl_upm
> *upm)
> if (bank < 0)
> return bank;
>
> - br = in_be32(&fsl_lbc_regs->bank[bank].br);
> + if (!fsl_lbc_ctrl_dev || !fsl_lbc_ctrl_dev->regs)
> + return -ENODEV;
> +
> + br = in_be32(&fsl_lbc_ctrl_dev->regs->bank[bank].br);
>
> switch (br & BR_MSEL) {
> case BR_MS_UPMA:
> - upm->mxmr = &fsl_lbc_regs->mamr;
> + upm->mxmr = &fsl_lbc_ctrl_dev->regs->mamr;
Ditto, a very repetitive stuff, desires a variable for regs?
> break;
> case BR_MS_UPMB:
> - upm->mxmr = &fsl_lbc_regs->mbmr;
> + upm->mxmr = &fsl_lb
Re: [PATCH 1/2] [PPC] Motion-PRO: Added LED support for the Promess Motion-Pro board. The driver is based on the original version(http://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg06694.htm
On Thu, Sep 02, 2010 at 05:34:56PM +0400, Sergei Poselenov wrote: [...] > > > + tristate "Motionpro LEDs Support" > > > + depends on LEDS_CLASS > > > + help > > > + This option enables support for status and ready LEDs > > > connected > > > + to GPIO lines on Motionpro board. > > > > Why not expose these GPIOs via GPIOLIB[1] and use generic GPIO > > LEDs[2] driver, along with the timer LED trigger[3] for blinking? > > > > Thanks, > > > > [1] Documentation/gpio.txt > > Documentation/powerpc/dts-bindings/gpio/gpio.txt > > [2] drivers/leds/leds-gpio.c > > Documentation/powerpc/dts-bindings/gpio/led.txt > > [3] drivers/leds/ledtrig-timer.c > > > > Yes, this seem possible to implement (and thanks for pointing into > this), however, the driver is already exists (actually, since 2007), > so why to not add it to save efforts? - Faking PWM in the LEDs driver is just wrong thing to do. I don't see any other drivers doing this, and even if they were, they would need to be fixed; - This duplicates timer trigger functionality; - By writing (if there isn't any already) a generic GPIOLIB driver for the GPIO controller that you have, you could use these GPIOs not only for LEDs, but also for SPI, MDIO, I2C, MMC, and even raw NAND chips. I.e., by choosing the right methodology you save much more efforts in the long run. Thanks, -- Anton Vorontsov email: cbouatmai...@gmail.com irc://irc.freenode.net/bd2 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 1/2] [PPC] Motion-PRO: Added LED support for the Promess Motion-Pro board. The driver is based on the original version(http://www.mail-archive.com/linuxppc-dev@lists.ozlabs.org/msg06694.htm
On Thu, Sep 02, 2010 at 12:20:31PM +0200, sposele...@emcraft.com wrote: [...] > +config LEDS_MOTIONPRO > + tristate "Motionpro LEDs Support" > + depends on LEDS_CLASS > + help > + This option enables support for status and ready LEDs connected > + to GPIO lines on Motionpro board. Why not expose these GPIOs via GPIOLIB[1] and use generic GPIO LEDs[2] driver, along with the timer LED trigger[3] for blinking? Thanks, [1] Documentation/gpio.txt Documentation/powerpc/dts-bindings/gpio/gpio.txt [2] drivers/leds/leds-gpio.c Documentation/powerpc/dts-bindings/gpio/led.txt [3] drivers/leds/ledtrig-timer.c -- Anton Vorontsov email: cbouatmai...@gmail.com irc://irc.freenode.net/bd2 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] gpiolib: Add 'struct gpio_chip' forward declaration for !GPIOLIB case
On Tue, Aug 31, 2010 at 10:44:14AM -0600, Grant Likely wrote:
> On Tue, Aug 31, 2010 at 2:37 AM, Anton Vorontsov
> wrote:
> > On Tue, Aug 31, 2010 at 02:03:44AM -0600, Grant Likely wrote:
> >> On Tue, Aug 24, 2010 at 01:26:23PM +0400, Anton Vorontsov wrote:
> >> > With CONFIG_GPIOLIB=n, the 'struct gpio_chip' is not declared,
> >> > so the following pops up on PowerPC:
> >> >
> >> > cc1: warnings being treated as errors
> >> > In file included from arch/powerpc/platforms/52xx/mpc52xx_common.c:19:
> >> > include/linux/of_gpio.h:74: warning: 'struct gpio_chip' declared
> >> > inside parameter list
> >> > include/linux/of_gpio.h:74: warning: its scope is only this definition
> >> > or declaration, which is probably not what
> >> > you want
> >> > include/linux/of_gpio.h:75: warning: 'struct gpio_chip' declared
> >> > inside parameter list
> >> > make[2]: *** [arch/powerpc/platforms/52xx/mpc52xx_common.o] Error 1
> >> >
> >> > This patch fixes the issue by providing the proper forward declaration.
> >> >
> >> > Signed-off-by: Anton Vorontsov
> >>
> >> This doesn't actually solve the problem, and gpiochip should
> >> remain undefined when CONFIG_GPIOLIB=n to catch exactly these
> >> build failures. The real problem is that I merged a change
> >> into the mpc5200 code that required CONFIG_GPIOLIB be enabled
> >> without reflecting that requirement in Kconfig.
> >
> > No, look closer. The error is in of_gpio.h, and it's perfectly
> > fine to include it w/o GPIOLIB=y.
>
> Looking even closer, we're both wrong. You're right I didn't look
> carefully enough, and the error is in of_gpio.h, not the .c file.
>
> However, it is not okay to get the definitions from of_gpio.h when
> CONFIG_GPIOLIB=n. If GPIOLIB, or more specifically OF_GPIO isn't set,
> then the of_gpio.h definitions should either not be defined, or should
> be defined as empty stubs (where appropriate).
Grrr. Grant, look again, even closer than you did.
They are stubs!
#else /* CONFIG_OF_GPIO */ <<<<<< !OF_GPIO (or !GPIOLIB) case.
/* Drivers may not strictly depend on the GPIO support, so let them link. */
static inline int of_get_gpio_flags(struct device_node *np, int index,
enum of_gpio_flags *flags)
{
return -ENOSYS;
}
static inline unsigned int of_gpio_count(struct device_node *np)
{
return 0;
}
static inline void of_gpiochip_add(struct gpio_chip *gc) { }
static inline void of_gpiochip_remove(struct gpio_chip *gc) { }
#endif /* CONFIG_OF_GPIO */
The errors are triggered by the of_gpiochip_*() stubs, which
are needed by the drivers/gpio/gpiolib.c.
Do you want to add another #ifdef CONFIG_GPIOLIB around
of_gpiochip_*()? That would be ugly.
There's nothing wrong in providing the forward decls, you
can't dereference it anyway (so you would still catch the
bogus users). And at the same time it's will work great
for these stubs.
--
Anton Vorontsov
email: cbouatmai...@gmail.com
irc://irc.freenode.net/bd2
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] gpiolib: Add 'struct gpio_chip' forward declaration for !GPIOLIB case
On Tue, Aug 31, 2010 at 02:03:44AM -0600, Grant Likely wrote: > On Tue, Aug 24, 2010 at 01:26:23PM +0400, Anton Vorontsov wrote: > > With CONFIG_GPIOLIB=n, the 'struct gpio_chip' is not declared, > > so the following pops up on PowerPC: > > > > cc1: warnings being treated as errors > > In file included from arch/powerpc/platforms/52xx/mpc52xx_common.c:19: > > include/linux/of_gpio.h:74: warning: 'struct gpio_chip' declared > > inside parameter list > > include/linux/of_gpio.h:74: warning: its scope is only this definition > > or declaration, which is probably not what > > you want > > include/linux/of_gpio.h:75: warning: 'struct gpio_chip' declared > > inside parameter list > > make[2]: *** [arch/powerpc/platforms/52xx/mpc52xx_common.o] Error 1 > > > > This patch fixes the issue by providing the proper forward declaration. > > > > Signed-off-by: Anton Vorontsov > > This doesn't actually solve the problem, and gpiochip should > remain undefined when CONFIG_GPIOLIB=n to catch exactly these > build failures. The real problem is that I merged a change > into the mpc5200 code that required CONFIG_GPIOLIB be enabled > without reflecting that requirement in Kconfig. No, look closer. The error is in of_gpio.h, and it's perfectly fine to include it w/o GPIOLIB=y. > > --- > > > > On Tue, Aug 24, 2010 at 04:26:08PM +1000, Benjamin Herrenschmidt wrote: > > > I get that with my current stuff: > > > > > > cc1: warnings being treated as errors > > > In file included from [..]/mpc52xx_common.c:19: > > > of_gpio.h:74: error: ‘struct gpio_chip’ declared inside parameter list > > [...] > > > make[3]: *** Waiting for unfinished jobs > > > > That's because with GPIOCHIP=n no one declares struct gpio_chip. > > > > It should be either of_gpio.h or gpio.h. Let's make it gpio.h, as > > this feels more generic, and we already have some !GPIOLIB handling > > in there. > > > > include/linux/gpio.h |6 ++ > > 1 files changed, 6 insertions(+), 0 deletions(-) > > > > diff --git a/include/linux/gpio.h b/include/linux/gpio.h > > index 03f616b..85207d2 100644 > > --- a/include/linux/gpio.h > > +++ b/include/linux/gpio.h > > @@ -15,6 +15,12 @@ > > struct device; > > > > /* > > + * Some code might rely on the declaration. Still, it is illegal > > + * to dereference it for !GPIOLIB case. > > + */ > > +struct gpio_chip; > > + > > +/* > > * Some platforms don't support the GPIO programming interface. > > * > > * In case some driver uses it anyway (it should normally have > > -- > > 1.7.0.5 > > -- Anton Vorontsov email: cbouatmai...@gmail.com irc://irc.freenode.net/bd2 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
