* RE: [PATCH V5 1/2] powerpc/85xx: Add QE common init function
From: Xiaobo Xie @ 2013-11-06 2:27 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc-dev@lists.ozlabs.org
In-Reply-To: <1380301237.24959.397.camel@snotra.buserror.net>
SGkgU2NvdHQsDQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogV29vZCBT
Y290dC1CMDc0MjENCj4gU2VudDogU2F0dXJkYXksIFNlcHRlbWJlciAyOCwgMjAxMyAxOjAxIEFN
DQo+IFRvOiBYaWUgWGlhb2JvLVI2MzA2MQ0KPiBDYzogbGludXhwcGMtZGV2QGxpc3RzLm96bGFi
cy5vcmcNCj4gU3ViamVjdDogUmU6IFtQQVRDSCBWNSAxLzJdIHBvd2VycGMvODV4eDogQWRkIFFF
IGNvbW1vbiBpbml0IGZ1bmN0aW9uDQo+IA0KPiBPbiBUaHUsIDIwMTMtMDktMjYgYXQgMTc6Mzcg
KzA4MDAsIFhpZSBYaWFvYm8gd3JvdGU6DQo+ID4gKyNpZmRlZiBDT05GSUdfUVVJQ0NfRU5HSU5F
DQo+ID4gK3ZvaWQgX19pbml0IG1wYzg1eHhfcWVfaW5pdCh2b2lkKQ0KPiA+ICt7DQo+ID4gKwlz
dHJ1Y3QgZGV2aWNlX25vZGUgKm5wOw0KPiA+ICsNCj4gPiArCW5wID0gb2ZfZmluZF9jb21wYXRp
YmxlX25vZGUoTlVMTCwgTlVMTCwgImZzbCxxZSIpOw0KPiA+ICsJaWYgKCFucCkgew0KPiA+ICsJ
CW5wID0gb2ZfZmluZF9ub2RlX2J5X25hbWUoTlVMTCwgInFlIik7DQo+ID4gKwkJaWYgKCFucCkg
ew0KPiA+ICsJCQlwcl9lcnIoIiVzOiBDb3VsZCBub3QgZmluZCBRdWljYyBFbmdpbmUgbm9kZVxu
IiwNCj4gPiArCQkJCQlfX2Z1bmNfXyk7DQo+ID4gKwkJCXJldHVybjsNCj4gPiArCQl9DQo+ID4g
Kwl9DQo+ID4gKw0KPiA+ICsJcWVfcmVzZXQoKTsNCj4gPiArCW9mX25vZGVfcHV0KG5wKTsNCj4g
DQo+IFlvdSdyZSBtaXNzaW5nIHRoZSBvZl9kZXZpY2VfaXNfYXZhaWxhYmxlKCkgY2hlY2s6DQoN
Ckkgd2lsbCBhZGQgdGhpcyBjaGVjaywgdGhhbmtzLg0KDQo+IA0KPiA+IC0JbnAgPSBvZl9maW5k
X2NvbXBhdGlibGVfbm9kZShOVUxMLCBOVUxMLCAiZnNsLHFlIik7DQo+ID4gLQlpZiAoIW5wKSB7
DQo+ID4gLQkJbnAgPSBvZl9maW5kX25vZGVfYnlfbmFtZShOVUxMLCAicWUiKTsNCj4gDQo+IC1T
Y290dA0KPiANCg0K
^ permalink raw reply
* Re: [PATCH 3/7] IBM Akebono: Add support for a new PHY to the IBM emac driver
From: Florian Fainelli @ 2013-11-06 2:23 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Alistair Popple, linuxppc-dev, David S. Miller, netdev
In-Reply-To: <1383704240.4776.115.camel@pasglop>
2013/11/5 Benjamin Herrenschmidt <benh@kernel.crashing.org>:
> On Wed, 2013-11-06 at 12:38 +1100, Alistair Popple wrote:
>> > Right, rgmii_mode_name() just has informative purposes and should be
>> > removed, I would suggest using standard device tree bindings
>> property
>> > (phy-mode) anyway such that you could use of_get_phy_mode() and use
>> > phy_interface_t types.
>>
>> Ok, that's what is currently done in the core IBM EMAC driver. As this
>> is used
>> just for informative purposes I will remove it. Thanks.
>
> Why ? Information is useful...
Not the way they are currently handled:
+ /* Check if we need to attach to a RGMII */
+ if (!rgmii_valid_mode(mode)) {
+ dev_err(&ofdev->dev, "unsupported settings !\n");
Considering that nothing useful is being printed, that or nothing at
all really looks identical to me.
--
Florian
^ permalink raw reply
* Re: [PATCH 3/7] IBM Akebono: Add support for a new PHY to the IBM emac driver
From: Benjamin Herrenschmidt @ 2013-11-06 2:17 UTC (permalink / raw)
To: Alistair Popple; +Cc: netdev, Florian Fainelli, linuxppc-dev, David S. Miller
In-Reply-To: <9250529.5UedqhaNce@mexican>
On Wed, 2013-11-06 at 12:38 +1100, Alistair Popple wrote:
> > Right, rgmii_mode_name() just has informative purposes and should be
> > removed, I would suggest using standard device tree bindings
> property
> > (phy-mode) anyway such that you could use of_get_phy_mode() and use
> > phy_interface_t types.
>
> Ok, that's what is currently done in the core IBM EMAC driver. As this
> is used
> just for informative purposes I will remove it. Thanks.
Why ? Information is useful...
Ben.
^ permalink raw reply
* Re: [PATCH 3/7] IBM Akebono: Add support for a new PHY to the IBM emac driver
From: Alistair Popple @ 2013-11-06 1:38 UTC (permalink / raw)
To: Florian Fainelli; +Cc: netdev, linuxppc-dev, David S. Miller
In-Reply-To: <CAGVrzcYzBtDcxYXDopOJn_=vW_58SFs9y0uKeGNB9TDPh6J4wg@mail.gmail.com>
On Tue, 5 Nov 2013 16:16:08 Florian Fainelli wrote:
[snip]
> 2013/11/5 Alistair Popple <alistair@popple.id.au>:
> >> Any reasons why you are duplicating what is available in
> >> drivers/of/of_net.c ::of_get_phy_mode()?
> >
> > Unless I'm missing something of_get_phy_mode() is going the other way.
> > rgmii_mode_name() is converting PHY_MODE_* into a human-readable string. I
> > couldn't find any obvious kernel method to do this but maybe I missed it?
>
> Right, rgmii_mode_name() just has informative purposes and should be
> removed, I would suggest using standard device tree bindings property
> (phy-mode) anyway such that you could use of_get_phy_mode() and use
> phy_interface_t types.
Ok, that's what is currently done in the core IBM EMAC driver. As this is used
just for informative purposes I will remove it. Thanks.
Regards,
Alistair
^ permalink raw reply
* Re: [PATCH 3/7] IBM Akebono: Add support for a new PHY to the IBM emac driver
From: Alistair Popple @ 2013-11-06 1:34 UTC (permalink / raw)
To: Ben Hutchings; +Cc: linuxppc-dev, David S. Miller, netdev
In-Reply-To: <1383693110.2868.17.camel@bwh-desktop.uk.level5networks.com>
On Tue, 5 Nov 2013 23:11:50 Ben Hutchings wrote:
> On Wed, 2013-11-06 at 06:54 +1100, Benjamin Herrenschmidt wrote:
[snip]
> > It's an SoC bit so there's little point making it generally
> > selectable by the user.
>
> I think a better way to do this is:
>
> config IBM_EMAC_RGMII_WOL
> bool "IBM EMAC RGMII wake-on-LAN support"
> depends on MY_WONDERFUL_NEW_SOC || COMPILE_TEST
> default y if MY_WONDERFUL_NEW_SOC
>
> Then anyone making an API change that affects this driver can check that
> it still complies.
The method used in this patch is the same as what is currently used by the
other IBM EMAC PHY interfaces (eg. config IBM_EMAC_ZMII etc). I'm happy to
send a patch to update all of those as well for consistency but that would
mean adding what each platform requires into EMACS Kconfig as well.
Personally I think it is nicer to keep the definitions of what each platform
requires in one place (ie. arch/powerpc/platforms/44x/Kconfig) as it is
consistent with what we do for other 44x drivers, however I am happy to use
the above method if people think it's better.
Alternatively we could do something like this:
config IBM_EMAC_RGMII_WOL
bool
default y if COMPILE_TEST
default n
This would leave the platform dependencies as they are currently but still
allow compile testing.
Regards,
Alistair
> Ben.
>
> > Alistair: The commit name should be different, it's not a PHY you are
> > adding, it's a PHY interface (the PHY itself is off chip).
^ permalink raw reply
* Re: [PATCH 3/7] IBM Akebono: Add support for a new PHY to the IBM emac driver
From: Florian Fainelli @ 2013-11-06 0:16 UTC (permalink / raw)
To: Alistair Popple; +Cc: netdev, linuxppc-dev, David S. Miller
In-Reply-To: <81419762.0SyAdYsUqu@mexican>
2013/11/5 Alistair Popple <alistair@popple.id.au>:
> On Tue, 5 Nov 2013 10:47:22 Florian Fainelli wrote:
>> [snip]
>>
>> > +/* RGMII bridge supports only GMII/TBI and RGMII/RTBI PHYs */
>> > +static inline int rgmii_valid_mode(int phy_mode)
>> > +{
>> > + return phy_mode == PHY_MODE_GMII ||
>> > + phy_mode == PHY_MODE_MII ||
>> > + phy_mode == PHY_MODE_RGMII ||
>> > + phy_mode == PHY_MODE_TBI ||
>> > + phy_mode == PHY_MODE_RTBI;
>> > +}
>> > +
>> > +static inline const char *rgmii_mode_name(int mode)
>> > +{
>> > + switch (mode) {
>> > + case PHY_MODE_RGMII:
>> > + return "RGMII";
>> > + case PHY_MODE_TBI:
>> > + return "TBI";
>> > + case PHY_MODE_GMII:
>> > + return "GMII";
>> > + case PHY_MODE_MII:
>> > + return "MII";
>> > + case PHY_MODE_RTBI:
>> > + return "RTBI";
>> > + default:
>> > + BUG();
>> > + }
>>
>> Any reasons why you are duplicating what is available in
>> drivers/of/of_net.c ::of_get_phy_mode()?
>
> Unless I'm missing something of_get_phy_mode() is going the other way.
> rgmii_mode_name() is converting PHY_MODE_* into a human-readable string. I
> couldn't find any obvious kernel method to do this but maybe I missed it?
Right, rgmii_mode_name() just has informative purposes and should be
removed, I would suggest using standard device tree bindings property
(phy-mode) anyway such that you could use of_get_phy_mode() and use
phy_interface_t types.
--
Florian
^ permalink raw reply
* Re: [PATCH 3/7] IBM Akebono: Add support for a new PHY to the IBM emac driver
From: Alistair Popple @ 2013-11-06 0:08 UTC (permalink / raw)
To: Florian Fainelli; +Cc: netdev, linuxppc-dev, David S. Miller
In-Reply-To: <CAGVrzcZQ26kbim2piOwrvpyOcSaTET5da7Yh4GWv+erfuOBDhQ@mail.gmail.com>
On Tue, 5 Nov 2013 10:47:22 Florian Fainelli wrote:
> [snip]
>
> > +/* RGMII bridge supports only GMII/TBI and RGMII/RTBI PHYs */
> > +static inline int rgmii_valid_mode(int phy_mode)
> > +{
> > + return phy_mode == PHY_MODE_GMII ||
> > + phy_mode == PHY_MODE_MII ||
> > + phy_mode == PHY_MODE_RGMII ||
> > + phy_mode == PHY_MODE_TBI ||
> > + phy_mode == PHY_MODE_RTBI;
> > +}
> > +
> > +static inline const char *rgmii_mode_name(int mode)
> > +{
> > + switch (mode) {
> > + case PHY_MODE_RGMII:
> > + return "RGMII";
> > + case PHY_MODE_TBI:
> > + return "TBI";
> > + case PHY_MODE_GMII:
> > + return "GMII";
> > + case PHY_MODE_MII:
> > + return "MII";
> > + case PHY_MODE_RTBI:
> > + return "RTBI";
> > + default:
> > + BUG();
> > + }
>
> Any reasons why you are duplicating what is available in
> drivers/of/of_net.c ::of_get_phy_mode()?
Unless I'm missing something of_get_phy_mode() is going the other way.
rgmii_mode_name() is converting PHY_MODE_* into a human-readable string. I
couldn't find any obvious kernel method to do this but maybe I missed it?
Regards,
Alistair
^ permalink raw reply
* Re: [PATCH] net: mv643xx_eth: Add missing phy_addr_set in DT mode
From: Arnaud Ebalard @ 2013-11-05 23:14 UTC (permalink / raw)
To: Sebastian Hesselbarth
Cc: Thomas Petazzoni, Andrew Lunn, Jason Cooper, netdev, linux-kernel,
Jason Gunthorpe, Lennert Buytenhek, linuxppc-dev, David Miller,
linux-arm-kernel
In-Reply-To: <52796E82.5010800@gmail.com>
Hi,
Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> writes:
> On 11/05/2013 11:12 PM, Arnaud Ebalard wrote:
>> Hi Jason,
>>
>> Jason Gunthorpe <jgunthorpe@obsidianresearch.com> writes:
>>
>>> Commit cc9d4598 'net: mv643xx_eth: use of_phy_connect if phy_node
>>> present' made the call to phy_scan optional, if the DT has a link to
>>> the phy node.
>>>
>>> However phy_scan has the side effect of calling phy_addr_set, which
>>> writes the phy MDIO address to the ethernet controller. If phy_addr_set
>>> is not called, and the bootloader has not set the correct address then
>>> the driver will fail to function.
>>
>> Thanks *a lot* for fixing this one! I had the issue on my ReadyNAS 102
>> (Armada 370 based) which I had put on a todo list and temporarily
>
> Erm, just to make sure: Armada 370 isn't using mv643xx_eth but mvneta,
> are you sure it is (was) related to Jason's fix?
Thanks for pointing this, Sebastian and my apologies for the noise.
Jason's fix is indeed for a file which is not compiled for my RN102.
As the problem perfectly matched the issue I had and current kernel w/
the patch applied does indeed fix it, I did not try and do the test w/o
the patch applied. It would have showed the problem was fixed by
something else in 3.12. Well, I spent some time digging the changes on
mvneta.c and:
commit 714086029116b6b0a34e67ba1dd2f0d1cf26770c
Author: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Date: Wed Sep 4 16:21:18 2013 +0200
net: mvneta: properly disable HW PHY polling and ensure adjust_link() works
This commit fixes a long-standing bug that has been reported by many
users: on some Armada 370 platforms, only the network interface that
has been used in U-Boot to tftp the kernel works properly in
Linux. The other network interfaces can see a 'link up', but are
unable to transmit data. The reports were generally made on the Armada
370-based Mirabox, but have also been given on the Armada 370-RD
board.
[SNIP]
$ git tag --contains 714086029116
v3.12
v3.12-rc1
v3.12-rc2
v3.12-rc3
v3.12-rc4
v3.12-rc5
v3.12-rc6
v3.12-rc7
So the problem was indeed fixed at the beginning of 3.12 series by Thomas.
Anyway, my bad and thanks again for pointing it out.
Cheers,
a+
^ permalink raw reply
* Re: [PATCH 3/7] IBM Akebono: Add support for a new PHY to the IBM emac driver
From: Ben Hutchings @ 2013-11-05 23:11 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Alistair Popple, linuxppc-dev, David S. Miller, netdev
In-Reply-To: <1383681240.4776.97.camel@pasglop>
On Wed, 2013-11-06 at 06:54 +1100, Benjamin Herrenschmidt wrote:
> On Tue, 2013-11-05 at 18:16 +0000, Ben Hutchings wrote:
> > On Tue, 2013-11-05 at 16:31 +1100, Alistair Popple wrote:
> > [...]
> > > --- a/drivers/net/ethernet/ibm/emac/Kconfig
> > > +++ b/drivers/net/ethernet/ibm/emac/Kconfig
> > > @@ -55,6 +55,10 @@ config IBM_EMAC_RGMII
> > > bool
> > > default n
> > >
> > > +config IBM_EMAC_RGMII_WOL
> > > + bool
> > > + default n
> > > +
> > [...]
> >
> > So no-one can even build-test this at present!
>
> Patch 7/7 adds the select to the platform, isn't that sufficient ?
I suppose so, but I don't think that went to netdev.
> It's an SoC bit so there's little point making it generally
> selectable by the user.
I think a better way to do this is:
config IBM_EMAC_RGMII_WOL
bool "IBM EMAC RGMII wake-on-LAN support"
depends on MY_WONDERFUL_NEW_SOC || COMPILE_TEST
default y if MY_WONDERFUL_NEW_SOC
Then anyone making an API change that affects this driver can check that
it still complies.
Ben.
> Alistair: The commit name should be different, it's not a PHY you are
> adding, it's a PHY interface (the PHY itself is off chip).
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [PATCH] net: mv643xx_eth: Add missing phy_addr_set in DT mode
From: Jason Cooper @ 2013-11-05 23:00 UTC (permalink / raw)
To: Arnaud Ebalard
Cc: Andrew Lunn, netdev, linux-kernel, Jason Gunthorpe,
Lennert Buytenhek, linuxppc-dev, David Miller, linux-arm-kernel,
Sebastian Hesselbarth
In-Reply-To: <87sivacxcf.fsf@natisbad.org>
On Tue, Nov 05, 2013 at 11:12:00PM +0100, Arnaud Ebalard wrote:
> Hi Jason,
>
> Jason Gunthorpe <jgunthorpe@obsidianresearch.com> writes:
>
> > Commit cc9d4598 'net: mv643xx_eth: use of_phy_connect if phy_node
> > present' made the call to phy_scan optional, if the DT has a link to
> > the phy node.
> >
> > However phy_scan has the side effect of calling phy_addr_set, which
> > writes the phy MDIO address to the ethernet controller. If phy_addr_set
> > is not called, and the bootloader has not set the correct address then
> > the driver will fail to function.
>
> Thanks *a lot* for fixing this one! I had the issue on my ReadyNAS 102
> (Armada 370 based) which I had put on a todo list and temporarily
readynas duo v2?
thx,
Jason.
> workarounded by including a 'ping whatever' call in my u-boot env in
> order to force it to do the init. Without it, I was unable to properly
> use the interface. With your fix, after multiple reboots to test it,
> everything works as expected. So, FWIW:
>
> Tested-by: Arnaud Ebalard <arno@natisbad.org>
>
> Cheers,
>
> a+
^ permalink raw reply
* Re: [PATCH] net: mv643xx_eth: Add missing phy_addr_set in DT mode
From: Sebastian Hesselbarth @ 2013-11-05 22:17 UTC (permalink / raw)
To: Arnaud Ebalard, Jason Gunthorpe
Cc: Andrew Lunn, Jason Cooper, linux-kernel, Lennert Buytenhek,
netdev, linuxppc-dev, David Miller, linux-arm-kernel
In-Reply-To: <87sivacxcf.fsf@natisbad.org>
On 11/05/2013 11:12 PM, Arnaud Ebalard wrote:
> Hi Jason,
>
> Jason Gunthorpe <jgunthorpe@obsidianresearch.com> writes:
>
>> Commit cc9d4598 'net: mv643xx_eth: use of_phy_connect if phy_node
>> present' made the call to phy_scan optional, if the DT has a link to
>> the phy node.
>>
>> However phy_scan has the side effect of calling phy_addr_set, which
>> writes the phy MDIO address to the ethernet controller. If phy_addr_set
>> is not called, and the bootloader has not set the correct address then
>> the driver will fail to function.
>
> Thanks *a lot* for fixing this one! I had the issue on my ReadyNAS 102
> (Armada 370 based) which I had put on a todo list and temporarily
Erm, just to make sure: Armada 370 isn't using mv643xx_eth but mvneta,
are you sure it is (was) related to Jason's fix?
Sebastian
> workarounded by including a 'ping whatever' call in my u-boot env in
> order to force it to do the init. Without it, I was unable to properly
> use the interface. With your fix, after multiple reboots to test it,
> everything works as expected. So, FWIW:
>
> Tested-by: Arnaud Ebalard <arno@natisbad.org>
^ permalink raw reply
* Re: [PATCH] net: mv643xx_eth: Add missing phy_addr_set in DT mode
From: Arnaud Ebalard @ 2013-11-05 22:12 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Andrew Lunn, Jason Cooper, netdev, linux-kernel,
Lennert Buytenhek, linuxppc-dev, David Miller, linux-arm-kernel,
Sebastian Hesselbarth
In-Reply-To: <1383611239-14556-1-git-send-email-jgunthorpe@obsidianresearch.com>
Hi Jason,
Jason Gunthorpe <jgunthorpe@obsidianresearch.com> writes:
> Commit cc9d4598 'net: mv643xx_eth: use of_phy_connect if phy_node
> present' made the call to phy_scan optional, if the DT has a link to
> the phy node.
>
> However phy_scan has the side effect of calling phy_addr_set, which
> writes the phy MDIO address to the ethernet controller. If phy_addr_set
> is not called, and the bootloader has not set the correct address then
> the driver will fail to function.
Thanks *a lot* for fixing this one! I had the issue on my ReadyNAS 102
(Armada 370 based) which I had put on a todo list and temporarily
workarounded by including a 'ping whatever' call in my u-boot env in
order to force it to do the init. Without it, I was unable to properly
use the interface. With your fix, after multiple reboots to test it,
everything works as expected. So, FWIW:
Tested-by: Arnaud Ebalard <arno@natisbad.org>
Cheers,
a+
^ permalink raw reply
* Re: [RFC PATCH v2] KVM: PPC: vfio kvm device: support spapr tce
From: Alex Williamson @ 2013-11-05 21:32 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: kvm, Gleb Natapov, Alexander Graf, kvm-ppc, linux-kernel,
Paul Mackerras, Paolo Bonzini, linuxppc-dev
In-Reply-To: <1383638731-13467-1-git-send-email-aik@ozlabs.ru>
On Tue, 2013-11-05 at 19:05 +1100, Alexey Kardashevskiy wrote:
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>
> Changes:
> v2:
> * it does not try to introduce a realmode search function.
> Instead, liobn-to-iommu-group lookup is done by VFIO KVM device
> in virtual mode and the result (iommu_group pointer) is cached
> in kvm_arch so the realmode handlers do not use VFIO KVM device for that.
> And the iommu groups get released on KVM termination.
>
> I tried this, seems viable.
>
> Did not I miss anything? Thanks.
A commit message ;)
> ---
> arch/powerpc/include/asm/kvm_host.h | 3 ++
> arch/powerpc/kvm/Kconfig | 1 +
> arch/powerpc/kvm/Makefile | 3 ++
> include/linux/vfio.h | 3 ++
> include/uapi/linux/kvm.h | 1 +
> virt/kvm/vfio.c | 74 +++++++++++++++++++++++++++++++++++++
> 6 files changed, 85 insertions(+)
>
> diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h
> index 48dbe8b..e1163d7 100644
> --- a/arch/powerpc/include/asm/kvm_host.h
> +++ b/arch/powerpc/include/asm/kvm_host.h
> @@ -293,6 +293,9 @@ struct kvm_arch {
> #ifdef CONFIG_KVM_XICS
> struct kvmppc_xics *xics;
> #endif
> +#ifdef CONFIG_KVM_VFIO
> + struct kvm_vfio *vfio;
> +#endif
> };
>
> /*
> diff --git a/arch/powerpc/kvm/Kconfig b/arch/powerpc/kvm/Kconfig
> index 61b3535..d1b7f64 100644
> --- a/arch/powerpc/kvm/Kconfig
> +++ b/arch/powerpc/kvm/Kconfig
> @@ -60,6 +60,7 @@ config KVM_BOOK3S_64
> select KVM_BOOK3S_64_HANDLER
> select KVM
> select SPAPR_TCE_IOMMU
> + select KVM_VFIO
> ---help---
> Support running unmodified book3s_64 and book3s_32 guest kernels
> in virtual machines on book3s_64 host processors.
> diff --git a/arch/powerpc/kvm/Makefile b/arch/powerpc/kvm/Makefile
> index 6646c95..2438d2e 100644
> --- a/arch/powerpc/kvm/Makefile
> +++ b/arch/powerpc/kvm/Makefile
> @@ -87,6 +87,9 @@ kvm-book3s_64-builtin-objs-$(CONFIG_KVM_BOOK3S_64_HV) := \
> kvm-book3s_64-objs-$(CONFIG_KVM_XICS) += \
> book3s_xics.o
>
> +kvm-book3s_64-objs-$(CONFIG_KVM_VFIO) += \
> + $(KVM)/vfio.o \
> +
> kvm-book3s_64-module-objs := \
> $(KVM)/kvm_main.o \
> $(KVM)/eventfd.o \
> diff --git a/include/linux/vfio.h b/include/linux/vfio.h
> index 24579a0..681e19b 100644
> --- a/include/linux/vfio.h
> +++ b/include/linux/vfio.h
> @@ -97,4 +97,7 @@ extern struct vfio_group *vfio_group_get_external_user(struct file *filep);
> extern void vfio_group_put_external_user(struct vfio_group *group);
> extern int vfio_external_user_iommu_id(struct vfio_group *group);
>
> +extern struct iommu_group *vfio_find_group_by_liobn(struct kvm *kvm,
> + unsigned long liobn);
> +
Nope, this doesn't go in vfio.h, it's a function provided by kvm. It
should be named as such too, kvm_vfio_... It also depends on both
CONFIG_KVM_VFIO and CONFIG_SPAPR_TCE_IOMMU and needs stub version
otherwise. Is just _liobn specific enough or does it need a spapr_tce
thrown in to avoid confusion with embedded ppc folks?
> #endif /* VFIO_H */
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 7c1a349..a74ad16 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -847,6 +847,7 @@ struct kvm_device_attr {
> #define KVM_DEV_VFIO_GROUP 1
> #define KVM_DEV_VFIO_GROUP_ADD 1
> #define KVM_DEV_VFIO_GROUP_DEL 2
> +#define KVM_DEV_VFIO_SPAPR_TCE_LIOBN 2
I wonder if it would be better architecturally if this was an attribute
rather than a new group, ex:
#define KVM_DEV_VFIO_GROUP_SET_SPAPR_TCE_LIOBN 3
It's a mouthful, but we are setting an attribute of a VFIO group, so it
makes sense. kvm_device_attr.addr would then need to point to a struct
containing both the fd and liobn.
Whatever we come up with need a documentation addition in
Documentation/virtual/kvm/devices/vfio.txt.
>
> /*
> * ioctls for VM fds
> diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c
> index ca4260e..f9271d5 100644
> --- a/virt/kvm/vfio.c
> +++ b/virt/kvm/vfio.c
> @@ -22,6 +22,9 @@
> struct kvm_vfio_group {
> struct list_head node;
> struct vfio_group *vfio_group;
> +#ifdef CONFIG_SPAPR_TCE_IOMMU
> + uint64_t liobn;
Why is liobn an unsigned long in the exported function but a uint64_t
here?
> +#endif
> };
>
> struct kvm_vfio {
> @@ -188,12 +191,76 @@ static int kvm_vfio_set_group(struct kvm_device *dev, long attr, u64 arg)
> return -ENXIO;
> }
>
> +#ifdef CONFIG_SPAPR_TCE_IOMMU
> +static int kvm_vfio_set_spapr_tce_liobn(struct kvm_device *dev,
> + long attr, u64 arg)
> +{
> + struct kvm_vfio *kv = dev->private;
> + struct vfio_group *vfio_group;
> + struct kvm_vfio_group *kvg;
> + void __user *argp = (void __user *)arg;
> + struct fd f;
> + int32_t fd;
> + uint64_t liobn = attr;
> +
> + if (get_user(fd, (int32_t __user *)argp))
> + return -EFAULT;
> +
> + f = fdget(fd);
> + if (!f.file)
> + return -EBADF;
> +
> + vfio_group = kvm_vfio_group_get_external_user(f.file);
> + fdput(f);
Not sure why you dropped this from the example of kvm_vfio_set_group:
if (IS_ERR(vfio_group))
return PTR_ERR(vfio_group);
You're also ignoring kv->lock.
> +
> + list_for_each_entry(kvg, &kv->group_list, node) {
> + if (kvg->vfio_group == vfio_group) {
> + WARN_ON(kvg->liobn);
Userspace should not be able to trigger a WARN this easily. Return
EBUSY if it's an error, otherwise let it go.
> + kvg->liobn = liobn;
> + kvm_vfio_group_put_external_user(vfio_group);
> + return 0;
> + }
> + }
> +
> + kvm_vfio_group_put_external_user(vfio_group);
> +
> + return -ENXIO;
> +}
> +
> +struct iommu_group *vfio_find_group_by_liobn(struct kvm *kvm,
> + unsigned long liobn)
As mentioned, kvm_vfio_...
> +{
> + struct kvm_vfio_group *kvg;
> +
> + if (!kvm->arch.vfio)
> + return NULL;
If this is already a slow path you can avoid stashing this pointer and
search kvm->devices for the matching ops struct.
kv->lock...
> +
> + list_for_each_entry(kvg, &kvm->arch.vfio->group_list, node) {
> + if (kvg->liobn == liobn) {
> + int group_id = vfio_external_user_iommu_id(
> + kvg->vfio_group);
> + struct iommu_group *grp =
> + iommu_group_get_by_id(group_id);
nit, ugly line wrapping. Where's the iommu_group_put() done?
> + return grp;
So you've now got an liobn to iommu_group mapping cached away somewhere,
what happens when a group is removed? Would it be invalid for userspace
to re-use the liobn? Do we need a way to invalidate your cached entry
and perhaps do an iommu_group_put()?
This version is actually plausible, so big improvement from v1! Thanks,
Alex
> + }
> + }
> +
> + return NULL;
> +}
> +EXPORT_SYMBOL_GPL(vfio_find_group_by_liobn);
> +#endif
> +
> static int kvm_vfio_set_attr(struct kvm_device *dev,
> struct kvm_device_attr *attr)
> {
> switch (attr->group) {
> case KVM_DEV_VFIO_GROUP:
> return kvm_vfio_set_group(dev, attr->attr, attr->addr);
> +#ifdef CONFIG_SPAPR_TCE_IOMMU
> + case KVM_DEV_VFIO_SPAPR_TCE_LIOBN:
> + return kvm_vfio_set_spapr_tce_liobn(dev, attr->attr,
> + attr->addr);
> +#endif
> }
>
> return -ENXIO;
> @@ -211,6 +278,10 @@ static int kvm_vfio_has_attr(struct kvm_device *dev,
> }
>
> break;
> +#ifdef CONFIG_SPAPR_TCE_IOMMU
> + case KVM_DEV_VFIO_SPAPR_TCE_LIOBN:
> + return 0;
> +#endif
> }
>
> return -ENXIO;
> @@ -251,6 +322,9 @@ static int kvm_vfio_create(struct kvm_device *dev, u32 type)
> mutex_init(&kv->lock);
>
> dev->private = kv;
> +#ifdef CONFIG_SPAPR_TCE_IOMMU
> + dev->kvm->arch.vfio = kv;
> +#endif
>
> return 0;
> }
^ permalink raw reply
* Re: [PATCH 3/7] IBM Akebono: Add support for a new PHY to the IBM emac driver
From: Benjamin Herrenschmidt @ 2013-11-05 19:54 UTC (permalink / raw)
To: Ben Hutchings; +Cc: Alistair Popple, linuxppc-dev, David S. Miller, netdev
In-Reply-To: <1383675404.2868.8.camel@bwh-desktop.uk.level5networks.com>
On Tue, 2013-11-05 at 18:16 +0000, Ben Hutchings wrote:
> On Tue, 2013-11-05 at 16:31 +1100, Alistair Popple wrote:
> [...]
> > --- a/drivers/net/ethernet/ibm/emac/Kconfig
> > +++ b/drivers/net/ethernet/ibm/emac/Kconfig
> > @@ -55,6 +55,10 @@ config IBM_EMAC_RGMII
> > bool
> > default n
> >
> > +config IBM_EMAC_RGMII_WOL
> > + bool
> > + default n
> > +
> [...]
>
> So no-one can even build-test this at present!
Patch 7/7 adds the select to the platform, isn't that sufficient ?
It's an SoC bit so there's little point making it generally
selectable by the user.
Alistair: The commit name should be different, it's not a PHY you are
adding, it's a PHY interface (the PHY itself is off chip).
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH 5/7] IBM Akebono: Add support to the EHCI platform driver for Akebono
From: Benjamin Herrenschmidt @ 2013-11-05 19:52 UTC (permalink / raw)
To: Alan Stern; +Cc: Alistair Popple, linux-usb, linuxppc-dev
In-Reply-To: <Pine.LNX.4.44L0.1311051004080.1360-100000@iolanthe.rowland.org>
On Tue, 2013-11-05 at 10:04 -0500, Alan Stern wrote:
> On Tue, 5 Nov 2013, Alistair Popple wrote:
>
> > The IBM Akebono board has an EHCI compliant USB host interface. This
> > patch adds support for it to the EHCI platform driver.
> >
> > Signed-off-by: Alistair Popple <alistair@popple.id.au>
> > Cc: Alan Stern <stern@rowland.harvard.edu>
> > Cc: linux-usb@vger.kernel.org
> > ---
> > drivers/usb/host/ehci-platform.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
> > index f6b790c..0a67616 100644
> > --- a/drivers/usb/host/ehci-platform.c
> > +++ b/drivers/usb/host/ehci-platform.c
> > @@ -203,9 +203,10 @@ static int ehci_platform_resume(struct device *dev)
> > #define ehci_platform_resume NULL
> > #endif /* CONFIG_PM */
> >
> > -static const struct of_device_id vt8500_ehci_ids[] = {
> > +static const struct of_device_id ehci_platform_ids[] = {
> > { .compatible = "via,vt8500-ehci", },
> > { .compatible = "wm,prizm-ehci", },
> > + { .compatible = "ibm,akebono-ehci", },
> > {}
> > };
Why ? Do we need to add an entry for every platform in there ? Besides,
it probably should be the SoC name not the platform here....
Why not simply a generic compatible "usb-ehci" ? It's a standard
programming interface, there are no specific quirks, we shouldn't
need to have to add new entries to the driver like that for every
new SoC/platform.
> > @@ -229,7 +230,7 @@ static struct platform_driver ehci_platform_driver = {
> > .owner = THIS_MODULE,
> > .name = "ehci-platform",
> > .pm = &ehci_platform_pm_ops,
> > - .of_match_table = vt8500_ehci_ids,
> > + .of_match_table = ehci_platform_ids,
> > }
> > };
>
> Acked-by: Alan Stern <stern@rowland.harvard.edu>
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
^ permalink raw reply
* Re: [RFC] arch: Introduce new TSO memory barrier smp_tmb()
From: Peter Zijlstra @ 2013-11-05 18:49 UTC (permalink / raw)
To: Will Deacon
Cc: Michael Neuling, Mathieu Desnoyers, heiko.carstens@de.ibm.com,
Oleg Nesterov, LKML, Linux PPC dev, Anton Blanchard,
Frederic Weisbecker, Victor Kaplansky, linux@arm.linux.org.uk,
Paul E. McKenney, Linus Torvalds, schwidefsky@de.ibm.com
In-Reply-To: <20131105140548.GD26895@mudshark.cambridge.arm.com>
On Tue, Nov 05, 2013 at 02:05:48PM +0000, Will Deacon wrote:
> > > +
> > > +#define smp_store_release(p, v) \
> > > +do { \
> > > + smp_mb(); \
> > > + ACCESS_ONCE(p) = (v); \
> > > +} while (0)
> > > +
> > > +#define smp_load_acquire(p, v) \
> > > +do { \
> > > + typeof(p) ___p1 = ACCESS_ONCE(p); \
> > > + smp_mb(); \
> > > + return ___p1; \
> > > +} while (0)
>
> What data sizes do these accessors operate on? Assuming that we want
> single-copy atomicity (with respect to interrupts in the UP case), we
> probably want a check to stop people passing in things like structs.
Fair enough; I think we should restrict to native word sizes same as we
do for atomics.
Something like so perhaps:
#ifdef CONFIG_64BIT
#define __check_native_word(t) (sizeof(t) == 4 || sizeof(t) == 8)
#else
#define __check_native_word(t) (sizeof(t) == 4)
#endif
#define smp_store_release(p, v) \
do { \
BUILD_BUG_ON(!__check_native_word(p)); \
smp_mb(); \
ACCESS_ONCE(p) = (v); \
} while (0)
> > > +#define smp_store_release(p, v) \
> > > +do { \
> > > + asm volatile ("stlr %w0 [%1]" : : "r" (v), "r" (&p) : "memory");\
>
> Missing comma between the operands. Also, that 'w' output modifier enforces
> a 32-bit store (same early question about sizes). Finally, it might be more
> efficient to use "=Q" for the addressing mode, rather than take the address
> of p manually.
so something like:
asm volatile ("stlr %0, [%1]" : : "r" (v), "=Q" (p) : "memory");
?
My inline asm foo is horrid and I mostly get by with copy paste from a
semi similar existing form :/
> Random other question: have you considered how these accessors should behave
> when presented with __iomem pointers?
A what? ;-)
^ permalink raw reply
* Re: [PATCH 3/7] IBM Akebono: Add support for a new PHY to the IBM emac driver
From: Florian Fainelli @ 2013-11-05 18:47 UTC (permalink / raw)
To: Alistair Popple; +Cc: netdev, linuxppc-dev, David S. Miller
In-Reply-To: <1383629471-16979-3-git-send-email-alistair@popple.id.au>
[snip]
> +/* RGMII bridge supports only GMII/TBI and RGMII/RTBI PHYs */
> +static inline int rgmii_valid_mode(int phy_mode)
> +{
> + return phy_mode == PHY_MODE_GMII ||
> + phy_mode == PHY_MODE_MII ||
> + phy_mode == PHY_MODE_RGMII ||
> + phy_mode == PHY_MODE_TBI ||
> + phy_mode == PHY_MODE_RTBI;
> +}
> +
> +static inline const char *rgmii_mode_name(int mode)
> +{
> + switch (mode) {
> + case PHY_MODE_RGMII:
> + return "RGMII";
> + case PHY_MODE_TBI:
> + return "TBI";
> + case PHY_MODE_GMII:
> + return "GMII";
> + case PHY_MODE_MII:
> + return "MII";
> + case PHY_MODE_RTBI:
> + return "RTBI";
> + default:
> + BUG();
> + }
Any reasons why you are duplicating what is available in
drivers/of/of_net.c ::of_get_phy_mode()?
--
Florian
^ permalink raw reply
* Re: [PATCH 3/7] IBM Akebono: Add support for a new PHY to the IBM emac driver
From: Ben Hutchings @ 2013-11-05 18:16 UTC (permalink / raw)
To: Alistair Popple; +Cc: netdev, linuxppc-dev, David S. Miller
In-Reply-To: <1383629471-16979-3-git-send-email-alistair@popple.id.au>
On Tue, 2013-11-05 at 16:31 +1100, Alistair Popple wrote:
[...]
> --- a/drivers/net/ethernet/ibm/emac/Kconfig
> +++ b/drivers/net/ethernet/ibm/emac/Kconfig
> @@ -55,6 +55,10 @@ config IBM_EMAC_RGMII
> bool
> default n
>
> +config IBM_EMAC_RGMII_WOL
> + bool
> + default n
> +
[...]
So no-one can even build-test this at present!
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [PATCH 5/7] IBM Akebono: Add support to the EHCI platform driver for Akebono
From: Alan Stern @ 2013-11-05 15:04 UTC (permalink / raw)
To: Alistair Popple; +Cc: linux-usb, linuxppc-dev
In-Reply-To: <1383629471-16979-5-git-send-email-alistair@popple.id.au>
On Tue, 5 Nov 2013, Alistair Popple wrote:
> The IBM Akebono board has an EHCI compliant USB host interface. This
> patch adds support for it to the EHCI platform driver.
>
> Signed-off-by: Alistair Popple <alistair@popple.id.au>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Cc: linux-usb@vger.kernel.org
> ---
> drivers/usb/host/ehci-platform.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
> index f6b790c..0a67616 100644
> --- a/drivers/usb/host/ehci-platform.c
> +++ b/drivers/usb/host/ehci-platform.c
> @@ -203,9 +203,10 @@ static int ehci_platform_resume(struct device *dev)
> #define ehci_platform_resume NULL
> #endif /* CONFIG_PM */
>
> -static const struct of_device_id vt8500_ehci_ids[] = {
> +static const struct of_device_id ehci_platform_ids[] = {
> { .compatible = "via,vt8500-ehci", },
> { .compatible = "wm,prizm-ehci", },
> + { .compatible = "ibm,akebono-ehci", },
> {}
> };
>
> @@ -229,7 +230,7 @@ static struct platform_driver ehci_platform_driver = {
> .owner = THIS_MODULE,
> .name = "ehci-platform",
> .pm = &ehci_platform_pm_ops,
> - .of_match_table = vt8500_ehci_ids,
> + .of_match_table = ehci_platform_ids,
> }
> };
Acked-by: Alan Stern <stern@rowland.harvard.edu>
^ permalink raw reply
* Re: [PATCH 4/7] IBM Akebono: Add support to the OHCI platform driver for Akebono
From: Alan Stern @ 2013-11-05 15:04 UTC (permalink / raw)
To: Alistair Popple; +Cc: linux-usb, linuxppc-dev
In-Reply-To: <1383629471-16979-4-git-send-email-alistair@popple.id.au>
On Tue, 5 Nov 2013, Alistair Popple wrote:
> The IBM Akebono board has a OHCI compliant USB host interface. This
> patch adds support for it to the OHCI platform driver.
>
> As we use device tree to pass platform specific data instead of
> platform data we remove the check for platform data and instead
> provide reasonable defaults if no platform data is present. This is
> similar to what is currently done in ehci-platform.c.
>
> Signed-off-by: Alistair Popple <alistair@popple.id.au>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Cc: linux-usb@vger.kernel.org
> ---
> drivers/usb/host/ohci-platform.c | 20 +++++++++++++++-----
> 1 file changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/host/ohci-platform.c b/drivers/usb/host/ohci-platform.c
> index a4c6410..4331454 100644
> --- a/drivers/usb/host/ohci-platform.c
> +++ b/drivers/usb/host/ohci-platform.c
> @@ -23,6 +23,8 @@
> #include <linux/usb/ohci_pdriver.h>
> #include <linux/usb.h>
> #include <linux/usb/hcd.h>
> +#include <linux/slab.h>
> +#include <linux/of.h>
>
> #include "ohci.h"
>
> @@ -55,6 +57,8 @@ static const struct ohci_driver_overrides platform_overrides __initconst = {
> .reset = ohci_platform_reset,
> };
>
> +static struct usb_ohci_pdata ohci_platform_defaults;
> +
> static int ohci_platform_probe(struct platform_device *dev)
> {
> struct usb_hcd *hcd;
> @@ -63,14 +67,14 @@ static int ohci_platform_probe(struct platform_device *dev)
> int irq;
> int err = -ENOMEM;
>
> - if (!pdata) {
> - WARN_ON(1);
> - return -ENODEV;
> - }
> -
> if (usb_disabled())
> return -ENODEV;
>
> + /* Platforms using DT don't always provide platform data.
> + * This should provide reasonable defaults. */
/*
* The accepted format for multi-line
* comments is like this.
*/
> + if (!pdata)
> + dev->dev.platform_data = pdata = &ohci_platform_defaults;
> +
> irq = platform_get_irq(dev, 0);
> if (irq < 0) {
> dev_err(&dev->dev, "no irq provided");
> @@ -171,6 +175,11 @@ static int ohci_platform_resume(struct device *dev)
> #define ohci_platform_resume NULL
> #endif /* CONFIG_PM */
>
> +static const struct of_device_id ohci_of_match[] = {
> + { .compatible = "ibm,akebono-ohci", },
> + {},
> +};
> +
> static const struct platform_device_id ohci_platform_table[] = {
> { "ohci-platform", 0 },
> { }
> @@ -191,6 +200,7 @@ static struct platform_driver ohci_platform_driver = {
> .owner = THIS_MODULE,
> .name = "ohci-platform",
> .pm = &ohci_platform_pm_ops,
> + .of_match_table = ohci_of_match,
> }
> };
Update the comment formatting, and then you can resubmit with
Acked-by: Alan Stern <stern@rowland.harvard.edu>
^ permalink raw reply
* Re: [RFC] arch: Introduce new TSO memory barrier smp_tmb()
From: Paul E. McKenney @ 2013-11-05 14:49 UTC (permalink / raw)
To: Will Deacon
Cc: Michael Neuling, Mathieu Desnoyers, Peter Zijlstra,
heiko.carstens@de.ibm.com, Oleg Nesterov, LKML, Linux PPC dev,
Anton Blanchard, Frederic Weisbecker, Victor Kaplansky,
linux@arm.linux.org.uk, Linus Torvalds, schwidefsky@de.ibm.com
In-Reply-To: <20131105140548.GD26895@mudshark.cambridge.arm.com>
On Tue, Nov 05, 2013 at 02:05:48PM +0000, Will Deacon wrote:
> On Mon, Nov 04, 2013 at 08:53:44PM +0000, Paul E. McKenney wrote:
> > On Mon, Nov 04, 2013 at 08:11:27PM +0100, Peter Zijlstra wrote:
> > Some comments below. I believe that opcodes need to be fixed for IA64.
> > I am unsure of the ifdefs and opcodes for arm64, but the ARM folks should
> > be able to tell us.
[ . . . ]
> > > +} while (0)
> > > +
> > > +#define smp_load_acquire(p) \
> > > +do { \
> > > + typeof(p) ___p1; \
> > > + asm volatile ("ldar %w0, [%1]" \
> > > + : "=r" (___p1) : "r" (&p) : "memory"); \
> > > + return ___p1; \
>
> Similar comments here wrt Q constraint.
>
> Random other question: have you considered how these accessors should behave
> when presented with __iomem pointers?
Should we have something to make sparse yell if not __kernel or some such?
Thanx, Paul
^ permalink raw reply
* Re: [RFC] arch: Introduce new TSO memory barrier smp_tmb()
From: Will Deacon @ 2013-11-05 14:05 UTC (permalink / raw)
To: Paul E. McKenney
Cc: Michael Neuling, Mathieu Desnoyers, Peter Zijlstra,
heiko.carstens@de.ibm.com, Oleg Nesterov, LKML, Linux PPC dev,
Anton Blanchard, Frederic Weisbecker, Victor Kaplansky,
linux@arm.linux.org.uk, Linus Torvalds, schwidefsky@de.ibm.com
In-Reply-To: <20131104205344.GW3947@linux.vnet.ibm.com>
On Mon, Nov 04, 2013 at 08:53:44PM +0000, Paul E. McKenney wrote:
> On Mon, Nov 04, 2013 at 08:11:27PM +0100, Peter Zijlstra wrote:
> Some comments below. I believe that opcodes need to be fixed for IA64.
> I am unsure of the ifdefs and opcodes for arm64, but the ARM folks should
> be able to tell us.
[...]
> > diff --git a/arch/arm/include/asm/barrier.h b/arch/arm/include/asm/barrier.h
> > index 60f15e274e6d..a804093d6891 100644
> > --- a/arch/arm/include/asm/barrier.h
> > +++ b/arch/arm/include/asm/barrier.h
> > @@ -53,10 +53,36 @@
> > #define smp_mb() barrier()
> > #define smp_rmb() barrier()
> > #define smp_wmb() barrier()
> > +
> > +#define smp_store_release(p, v) \
> > +do { \
> > + smp_mb(); \
> > + ACCESS_ONCE(p) = (v); \
> > +} while (0)
> > +
> > +#define smp_load_acquire(p, v) \
> > +do { \
> > + typeof(p) ___p1 = ACCESS_ONCE(p); \
> > + smp_mb(); \
> > + return ___p1; \
> > +} while (0)
What data sizes do these accessors operate on? Assuming that we want
single-copy atomicity (with respect to interrupts in the UP case), we
probably want a check to stop people passing in things like structs.
> > #else
> > #define smp_mb() dmb(ish)
> > #define smp_rmb() smp_mb()
> > #define smp_wmb() dmb(ishst)
> > +
>
> Seems like there should be some sort of #ifdef condition to distinguish
> between these. My guess is something like:
>
> #if __LINUX_ARM_ARCH__ > 7
>
> But I must defer to the ARM guys. For all I know, they might prefer
> arch/arm to stick with smp_mb() and have arch/arm64 do the ldar and stlr.
Yes. For arch/arm/, I'd rather we stick with the smp_mb() for the time
being. We don't (yet) have any 32-bit ARMv8 support, and the efforts towards
a single zImage could do without minor variations like this, not to mention
the usual backlash I get whenever introducing something that needs a
relatively recent binutils.
> > +#define smp_store_release(p, v) \
> > +do { \
> > + asm volatile ("stlr %w0 [%1]" : : "r" (v), "r" (&p) : "memory");\
> > +} while (0)
> > +
> > +#define smp_load_acquire(p) \
> > +do { \
> > + typeof(p) ___p1; \
> > + asm volatile ("ldar %w0, [%1]" \
> > + : "=r" (___p1) : "r" (&p) : "memory"); \
> > + return ___p1; \
> > +} while (0)
> > #endif
> >
> > #define read_barrier_depends() do { } while(0)
> > diff --git a/arch/arm64/include/asm/barrier.h b/arch/arm64/include/asm/barrier.h
> > index d4a63338a53c..0da2d4ebb9a8 100644
> > --- a/arch/arm64/include/asm/barrier.h
> > +++ b/arch/arm64/include/asm/barrier.h
> > @@ -35,10 +35,38 @@
> > #define smp_mb() barrier()
> > #define smp_rmb() barrier()
> > #define smp_wmb() barrier()
> > +
> > +#define smp_store_release(p, v) \
> > +do { \
> > + smp_mb(); \
> > + ACCESS_ONCE(p) = (v); \
> > +} while (0)
> > +
> > +#define smp_load_acquire(p, v) \
> > +do { \
> > + typeof(p) ___p1 = ACCESS_ONCE(p); \
> > + smp_mb(); \
> > + return ___p1; \
> > +} while (0)
> > +
> > #else
> > +
> > #define smp_mb() asm volatile("dmb ish" : : : "memory")
> > #define smp_rmb() asm volatile("dmb ishld" : : : "memory")
> > #define smp_wmb() asm volatile("dmb ishst" : : : "memory")
> > +
> > +#define smp_store_release(p, v) \
> > +do { \
> > + asm volatile ("stlr %w0 [%1]" : : "r" (v), "r" (&p) : "memory");\
Missing comma between the operands. Also, that 'w' output modifier enforces
a 32-bit store (same early question about sizes). Finally, it might be more
efficient to use "=Q" for the addressing mode, rather than take the address
of p manually.
> > +} while (0)
> > +
> > +#define smp_load_acquire(p) \
> > +do { \
> > + typeof(p) ___p1; \
> > + asm volatile ("ldar %w0, [%1]" \
> > + : "=r" (___p1) : "r" (&p) : "memory"); \
> > + return ___p1; \
Similar comments here wrt Q constraint.
Random other question: have you considered how these accessors should behave
when presented with __iomem pointers?
Will
^ permalink raw reply
* Re: [PATCHv2 1/8] ALSA: Add SAI SoC Digital Audio Interface driver.
From: Timur Tabi @ 2013-11-05 13:26 UTC (permalink / raw)
To: Li Xiubo, Guangyu Chen
Cc: mark.rutland@arm.com, alsa-devel@alsa-project.org,
linux-doc@vger.kernel.org, tiwai@suse.de, Huan Wang,
perex@perex.cz, Shawn Guo, LW@KARO-electronics.de,
linux@arm.linux.org.uk, linux-arm-kernel@lists.infradead.org,
grant.likely@linaro.org, devicetree@vger.kernel.org,
ian.campbell@citrix.com, pawel.moll@arm.com,
swarren@wwwdotorg.org, rob.herring@calxeda.com,
broonie@kernel.org, oskar@scara.com, Fabio Estevam,
lgirdwood@gmail.com, linux-kernel@vger.kernel.org,
rob@landley.net, Zhengxiong Jin, shawn.guo@linaro.org,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <1DD289F6464F0949A2FCA5AA6DC23F82874201@039-SN2MPN1-013.039d.mgd.msft.net>
Li Xiubo wrote:
> But fsl-ssi.o and fsl-spdif.o is based PowrePC platform? Which we can see from the comments.
fsl_ssi was originally PPC-only, but it now supports PPC and ARM. You
can see that from the git history.
If there are any comments that say PPC but are not PPC-specific, that
should be fixed.
^ permalink raw reply
* Re: [PATCH] net: mv643xx_eth: Add missing phy_addr_set in DT mode
From: Sebastian Hesselbarth @ 2013-11-05 9:04 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Andrew Lunn, Jason Cooper, netdev, linux-kernel, linux-arm-kernel,
linuxppc-dev, David Miller, Lennert Buytenhek
In-Reply-To: <1383611239-14556-1-git-send-email-jgunthorpe@obsidianresearch.com>
On 11/05/2013 01:27 AM, Jason Gunthorpe wrote:
> Commit cc9d4598 'net: mv643xx_eth: use of_phy_connect if phy_node
> present' made the call to phy_scan optional, if the DT has a link to
> the phy node.
>
> However phy_scan has the side effect of calling phy_addr_set, which
> writes the phy MDIO address to the ethernet controller. If phy_addr_set
> is not called, and the bootloader has not set the correct address then
> the driver will fail to function.
>
> Tested on Kirkwood.
>
> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> ---
Jason,
thanks for catching this! I do my kirkwood testing on Dockstar,
which has PHY addr 0x0 - also the reset default, which may be
why it slipped through.
Acked-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> ---
> drivers/net/ethernet/marvell/mv643xx_eth.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/net/ethernet/marvell/mv643xx_eth.c b/drivers/net/ethernet/marvell/mv643xx_eth.c
> index 2c210ec..00e43b5 100644
> --- a/drivers/net/ethernet/marvell/mv643xx_eth.c
> +++ b/drivers/net/ethernet/marvell/mv643xx_eth.c
> @@ -2890,6 +2890,7 @@ static int mv643xx_eth_probe(struct platform_device *pdev)
> PHY_INTERFACE_MODE_GMII);
> if (!mp->phy)
> err = -ENODEV;
> + phy_addr_set(mp, mp->phy->addr);
> } else if (pd->phy_addr != MV643XX_ETH_PHY_NONE) {
> mp->phy = phy_scan(mp, pd->phy_addr);
>
>
^ permalink raw reply
* [PATCH] powerpc: memcpy optimization for 64bit LE
From: Philippe Bergheaud @ 2013-11-05 8:38 UTC (permalink / raw)
To: Linuxppc-dev; +Cc: Philippe Bergheaud
Unaligned stores take alignment exceptions on POWER7 running in little-endian.
This is a dumb little-endian base memcpy that prevents unaligned stores.
It is replaced by the VMX memcpy at boot.
Signed-off-by: Philippe Bergheaud <felix@linux.vnet.ibm.com>
---
arch/powerpc/include/asm/string.h | 4 ----
arch/powerpc/kernel/ppc_ksyms.c | 2 --
arch/powerpc/lib/Makefile | 2 --
arch/powerpc/lib/memcpy_64.S | 19 +++++++++++++++++++
4 files changed, 19 insertions(+), 8 deletions(-)
diff --git a/arch/powerpc/include/asm/string.h b/arch/powerpc/include/asm/string.h
index 0dffad6..e40010a 100644
--- a/arch/powerpc/include/asm/string.h
+++ b/arch/powerpc/include/asm/string.h
@@ -10,9 +10,7 @@
#define __HAVE_ARCH_STRNCMP
#define __HAVE_ARCH_STRCAT
#define __HAVE_ARCH_MEMSET
-#ifdef __BIG_ENDIAN__
#define __HAVE_ARCH_MEMCPY
-#endif
#define __HAVE_ARCH_MEMMOVE
#define __HAVE_ARCH_MEMCMP
#define __HAVE_ARCH_MEMCHR
@@ -24,9 +22,7 @@ extern int strcmp(const char *,const char *);
extern int strncmp(const char *, const char *, __kernel_size_t);
extern char * strcat(char *, const char *);
extern void * memset(void *,int,__kernel_size_t);
-#ifdef __BIG_ENDIAN__
extern void * memcpy(void *,const void *,__kernel_size_t);
-#endif
extern void * memmove(void *,const void *,__kernel_size_t);
extern int memcmp(const void *,const void *,__kernel_size_t);
extern void * memchr(const void *,int,__kernel_size_t);
diff --git a/arch/powerpc/kernel/ppc_ksyms.c b/arch/powerpc/kernel/ppc_ksyms.c
index 526ad5c..0c2dd60 100644
--- a/arch/powerpc/kernel/ppc_ksyms.c
+++ b/arch/powerpc/kernel/ppc_ksyms.c
@@ -147,9 +147,7 @@ EXPORT_SYMBOL(__ucmpdi2);
#endif
long long __bswapdi2(long long);
EXPORT_SYMBOL(__bswapdi2);
-#ifdef __BIG_ENDIAN__
EXPORT_SYMBOL(memcpy);
-#endif
EXPORT_SYMBOL(memset);
EXPORT_SYMBOL(memmove);
EXPORT_SYMBOL(memcmp);
diff --git a/arch/powerpc/lib/Makefile b/arch/powerpc/lib/Makefile
index 5310132..6670361 100644
--- a/arch/powerpc/lib/Makefile
+++ b/arch/powerpc/lib/Makefile
@@ -23,9 +23,7 @@ obj-y += checksum_$(CONFIG_WORD_SIZE).o
obj-$(CONFIG_PPC64) += checksum_wrappers_64.o
endif
-ifeq ($(CONFIG_CPU_LITTLE_ENDIAN),)
obj-$(CONFIG_PPC64) += memcpy_power7.o memcpy_64.o
-endif
obj-$(CONFIG_PPC_EMULATE_SSTEP) += sstep.o ldstfp.o
diff --git a/arch/powerpc/lib/memcpy_64.S b/arch/powerpc/lib/memcpy_64.S
index d2bbbc8..358cf74 100644
--- a/arch/powerpc/lib/memcpy_64.S
+++ b/arch/powerpc/lib/memcpy_64.S
@@ -12,10 +12,28 @@
.align 7
_GLOBAL(memcpy)
BEGIN_FTR_SECTION
+#ifdef __LITTLE_ENDIAN__
+ cmpdi cr7,r5,0 /* dumb little-endian memcpy */
+#else
std r3,48(r1) /* save destination pointer for return value */
+#endif
FTR_SECTION_ELSE
b memcpy_power7
ALT_FTR_SECTION_END_IFCLR(CPU_FTR_VMX_COPY)
+#ifdef __LITTLE_ENDIAN__
+ addi r5,r5,-1
+ addi r9,r3,-1
+ add r5,r3,r5
+ subf r5,r9,r5
+ addi r4,r4,-1
+ mtctr r5
+ beqlr cr7
+1:
+ lbzu r10,1(r4)
+ stbu r10,1(r9)
+ bdnz 1b
+ blr
+#else
PPC_MTOCRF(0x01,r5)
cmpldi cr1,r5,16
neg r6,r3 # LS 3 bits = # bytes to 8-byte dest bdry
@@ -201,3 +219,4 @@ END_FTR_SECTION_IFCLR(CPU_FTR_UNALIGNED_LD_STD)
stb r0,0(r3)
4: ld r3,48(r1) /* return dest pointer */
blr
+#endif
--
1.7.10.4
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox