[PATCH] ARM: dts: aspeed: tiogapass: Remove vuart
Removed vuart for facebook tiogapass platform as it uses uart2 and
uart3 pin with aspeed uart routing feature.
Signed-off-by: Vijay Khemka
---
arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts | 5 -
1 file changed, 5 deletions(-)
diff --git a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
index 2d44d9ad4e40..e6ad821a8635 100644
--- a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
@@ -82,11 +82,6 @@ _ctrl {
status = "okay";
};
- {
- // VUART Host Console
- status = "okay";
-};
-
{
// Host Console
status = "okay";
--
2.24.1
Re: [PATCH v2] hwmon:(adm1275) Enable adm1278 ADM1278_TEMP1_EN
On 5/29/20, 11:56 AM, "Guenter Roeck" wrote:
On 5/29/20 10:57 AM, Vijay Khemka wrote:
>
>
> On 5/29/20, 5:47 AM, "Manikandan Elumalai"
wrote:
>
> The adm1278 temperature sysfs attribute need it for one of the
openbmc platform .
> This functionality is not enabled by default, so PMON_CONFIG needs to
be modified in order to enable it.
>
> Signed-off-by : Manikandan Elumalai
>
> v2:
>- Add Signed-off-by.
>- Removed ADM1278_TEMP1_EN check.
> ---
> drivers/hwmon/pmbus/adm1275.c | 21 +
> 1 file changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hwmon/pmbus/adm1275.c
b/drivers/hwmon/pmbus/adm1275.c
> index 5caa37fb..ab5fceb 100644
> --- a/drivers/hwmon/pmbus/adm1275.c
> +++ b/drivers/hwmon/pmbus/adm1275.c
> @@ -666,7 +666,23 @@ static int adm1275_probe(struct i2c_client
*client,
> tindex = 3;
>
> info->func[0] |= PMBUS_HAVE_PIN |
PMBUS_HAVE_STATUS_INPUT |
> - PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT;
> + PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT |
> + PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP;
> +
> + config = i2c_smbus_read_byte_data(client,
ADM1275_PMON_CONFIG);
> + if (config < 0)
> + return config;
> +
> + /* Enable TEMP1 by default */
> + config |= ADM1278_TEMP1_EN;
> + ret = i2c_smbus_write_byte_data(client,
> + ADM1275_PMON_CONFIG,
> + config);
> + if (ret < 0) {
> + dev_err(>dev,
> + "Failed to enable temperature config\n");
> + return -ENODEV;
> + }
> You don't need this above code removing check as below should be enough
to
> populate sysfs entry you need.
>
You mean you are only interested in having the attribute, even if it doesn't
report anything useful because monitoring is disabled in the chip ?
Sorry, I don't understand. Can you elaborate ?
Sorry for misinterpretation, No I don't mean that. This should be fine.
Thanks,
Guenter
> /* Enable VOUT if not enabled (it is disabled by
default) */
> if (!(config & ADM1278_VOUT_EN)) {
> @@ -681,9 +697,6 @@ static int adm1275_probe(struct i2c_client
*client,
> }
> }
>
> - if (config & ADM1278_TEMP1_EN)
> - info->func[0] |=
> - PMBUS_HAVE_TEMP |
PMBUS_HAVE_STATUS_TEMP;
> if (config & ADM1278_VIN_EN)
> info->func[0] |= PMBUS_HAVE_VIN;
> break;
> --
> 2.7.4
>
>
>
Re: [PATCH v2] hwmon:(adm1275) Enable adm1278 ADM1278_TEMP1_EN
On 5/29/20, 5:47 AM, "Manikandan Elumalai"
wrote:
The adm1278 temperature sysfs attribute need it for one of the openbmc
platform .
This functionality is not enabled by default, so PMON_CONFIG needs to be
modified in order to enable it.
Signed-off-by : Manikandan Elumalai
v2:
- Add Signed-off-by.
- Removed ADM1278_TEMP1_EN check.
---
drivers/hwmon/pmbus/adm1275.c | 21 +
1 file changed, 17 insertions(+), 4 deletions(-)
diff --git a/drivers/hwmon/pmbus/adm1275.c b/drivers/hwmon/pmbus/adm1275.c
index 5caa37fb..ab5fceb 100644
--- a/drivers/hwmon/pmbus/adm1275.c
+++ b/drivers/hwmon/pmbus/adm1275.c
@@ -666,7 +666,23 @@ static int adm1275_probe(struct i2c_client *client,
tindex = 3;
info->func[0] |= PMBUS_HAVE_PIN | PMBUS_HAVE_STATUS_INPUT |
- PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT;
+ PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT |
+ PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP;
+
+ config = i2c_smbus_read_byte_data(client, ADM1275_PMON_CONFIG);
+ if (config < 0)
+ return config;
+
+ /* Enable TEMP1 by default */
+ config |= ADM1278_TEMP1_EN;
+ ret = i2c_smbus_write_byte_data(client,
+ ADM1275_PMON_CONFIG,
+ config);
+ if (ret < 0) {
+ dev_err(>dev,
+ "Failed to enable temperature config\n");
+ return -ENODEV;
+ }
You don't need this above code removing check as below should be enough to
populate sysfs entry you need.
/* Enable VOUT if not enabled (it is disabled by default) */
if (!(config & ADM1278_VOUT_EN)) {
@@ -681,9 +697,6 @@ static int adm1275_probe(struct i2c_client *client,
}
}
- if (config & ADM1278_TEMP1_EN)
- info->func[0] |=
- PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP;
if (config & ADM1278_VIN_EN)
info->func[0] |= PMBUS_HAVE_VIN;
break;
--
2.7.4
Re: [PATCH] hwmon:(adm1275) Enable adm1278 ADM1278_TEMP1_EN
On 5/28/20, 7:15 AM, "Manikandan Elumalai"
wrote:
The adm1278 temperature sysfs attribute need it for one of the our openbmc
platform .
This functionality is not enabled by default, so PMON_CONFIG needs to be
modified in order to enable it.
There is no Signed-off-by.
---
drivers/hwmon/pmbus/adm1275.c | 15 +++
1 file changed, 15 insertions(+)
diff --git a/drivers/hwmon/pmbus/adm1275.c b/drivers/hwmon/pmbus/adm1275.c
index 5caa37fb..47b293d 100644
--- a/drivers/hwmon/pmbus/adm1275.c
+++ b/drivers/hwmon/pmbus/adm1275.c
@@ -681,6 +681,21 @@ static int adm1275_probe(struct i2c_client *client,
}
}
+ config = i2c_smbus_read_byte_data(client, ADM1275_PMON_CONFIG);
+ if (config < 0)
+ return config;
+
+ /* Enable TEMP1 by defult */
+ config |= ADM1278_TEMP1_EN;
+ ret = i2c_smbus_write_byte_data(client,
+ ADM1275_PMON_CONFIG,
+ config);
+ if (ret < 0) {
+ dev_err(>dev,
+ "Failed to enable temperature config\n");
+ return -ENODEV;
+ }
+
if (config & ADM1278_TEMP1_EN)
This check becomes irrelevant as you are enabling it above then rather than
enabling it just remove this check.
info->func[0] |=
PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP;
--
2.7.4
Re: [PATCH v2] ftgmac100: Disable HW checksum generation on AST2500
On 10/18/19, 5:03 PM, "Benjamin Herrenschmidt" wrote: On Fri, 2019-10-18 at 22:50 +0000, Vijay Khemka wrote: > I don't have much understanding of IP Stack but I went through code details and > you are right and found that it should fallback to SW calculation for IPV6 but it doesn't > happen because ftgmac100_hard_start_xmit checks for CHECKSUM_PARTIAL before > setting HW checksum and calling ftgmac100_prep_tx_csum function. And in my > understanding, this value is set CHECKSUM_PARTIAL in IP stack. I looked up IP stack for > IPV6, file net/ipv6/ip6_output.c, function __ip6_append_data: here it sets > CHECKSUM_PARTIAL only for UDP packets not for TCP packets. Please look at line > number 1880. This could be an issue we are seeing here as why > ftgmac100_prep_tx_csum is not getting triggered for IPV6 with TCP. Please correct > me if my understanding is wrong. > Not entirely sure. tcp_v6_send_response() in tcp_ipv6.c does set CHECKSUM_PARTIAL as well. I don't really know how things are being handled in that part of the network stack though. From a driver perspective, if the value of ip_summed is not CHECKSUM_PARTIAL it means we should not have to calculate any checksum. At least that's my understanding here. You may need to add some traces to the driver to see what you get in there, what protocol indication etc... and analyze the corresponding packets with something like tcpdump or wireshark on the other end. Thanks Ben, I will try to add some trace and test whatever possible and test it. As we don't have tcpdump into our image and I have limited understanding of networking stack so if you get some time to verify ipv6, it will be really helpful. Cheers, Ben.
Re: [PATCH v2] ftgmac100: Disable HW checksum generation on AST2500
On 10/17/19, 5:33 PM, "Benjamin Herrenschmidt"
wrote:
On Fri, 2019-10-18 at 00:06 +0000, Vijay Khemka wrote:
>
> > This is not a matter of unsupported csum, it is broken hw csum.
> > That's why we disable hw checksum. My guess is once we disable
> > Hw checksum, it will use sw checksum. So I am just disabling hw
> > Checksum.
>
> I don't understand what you are saying. You reported a problem with
> IPV6 checksums generation. The HW doesn't support it. What's "not a
> matter of unsupported csum" ?
>
> Your patch uses a *deprecated* bit to tell the network stack to only
do
> HW checksum generation on IPV4.
>
> This bit is deprecated for a reason, again, see skbuff.h. The right
> approach, *which the driver already does*, is to tell the stack that
we
> support HW checksuming using NETIF_F_HW_CSUM, and then, in the
transmit
> handler, to call skb_checksum_help() to have the SW calculate the
> checksum if it's not a supported type.
>
> My understanding was when we enable NETIF_F_HW_CSUM means network
> stack enables HW checksum and doesn't calculate SW checksum. But as per
> this supported types HW checksum are used only for IPV4 and not for IPV6
even
> though driver enabled NETIF_F_HW_CSUM. For IPV6 it is always a SW
generated
> checksum, please correct me here.
Have you actually read the comments in skbuff.h that I pointed you to ?
And the rest of my email for that matter ?
Yes, I went through comments in skbuff.h and your comments as well. I knew about
this deprecated bit that's why I have disabled NETIF_F_HW_CSUM completely in my
V1 patch. Then Florian gave a comment and asked me to disable only IPV6 not IPV4
as it is working for IPV4 and issue with only IPV6. That's why I sent patch V2
accommodating his feedback.
I don't have much understanding of IP Stack but I went through code details and
you are right and found that it should fallback to SW calculation for IPV6 but
it doesn't
happen because ftgmac100_hard_start_xmit checks for CHECKSUM_PARTIAL before
setting HW checksum and calling ftgmac100_prep_tx_csum function. And in my
understanding, this value is set CHECKSUM_PARTIAL in IP stack. I looked up IP
stack for
IPV6, file net/ipv6/ip6_output.c, function __ip6_append_data: here it sets
CHECKSUM_PARTIAL only for UDP packets not for TCP packets. Please look at line
number 1880. This could be an issue we are seeing here as why
ftgmac100_prep_tx_csum is not getting triggered for IPV6 with TCP. Please
correct
me if my understanding is wrong.
> This is exactly what ftgmac100_prep_tx_csum() does. It only enables HW
> checksum generation on supported types and uses skb_checksum_help()
> otherwise, supported types being protocol ETH_P_IP and IP protocol
> being raw IP, TCP and UDP.
>
>
> So this *should* have fallen back to SW for IPV6. So either something
> in my code there is making an incorrect assumption, or something is
> broken in skb_checksum_help() for IPV6 (which I somewhat doubt) or
> something else I can't think of, but setting a *deprecated* flag is
> definitely not the right answer, neither is completely disabling HW
> checksumming.
>
> So can you investigate what's going on a bit more closely please ? I
> can try myself, though I have very little experience with IPV6 and
> probably won't have time before next week.
>
> Cheers,
> Ben.
>
> > The driver should have handled unsupported csum via SW fallback
> > already in ftgmac100_prep_tx_csum()
> >
> > Can you check why this didn't work for you ?
> >
> > Cheers,
> > Ben.
> >
> > > Signed-off-by: Vijay Khemka
> > > ---
> > > Changes since v1:
> > > Enabled IPV4 hw checksum generation as it works for IPV4.
> > >
> > > drivers/net/ethernet/faraday/ftgmac100.c | 13 -
> > > 1 file changed, 12 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/net/ethernet/faraday/ftgmac100.c
> > > b/drivers/net/ethernet/faraday/ftgmac100.c
> > > index 030fed65393e..0255a28d2958 100644
> > > --- a/drivers/net/ethernet/faraday/ftgmac100.c
> > > +++ b/drivers/net/ethernet/faraday/ftgmac100
Re: [PATCH v2] ftgmac100: Disable HW checksum generation on AST2500
On 10/17/19, 4:15 PM, "Benjamin Herrenschmidt"
wrote:
On Thu, 2019-10-17 at 22:01 +0000, Vijay Khemka wrote:
>
> On 10/16/19, 6:29 PM, "Benjamin Herrenschmidt"
wrote:
>
> On Fri, 2019-10-11 at 14:30 -0700, Vijay Khemka wrote:
> > HW checksum generation is not working for AST2500, specially with
> > IPV6
> > over NCSI. All TCP packets with IPv6 get dropped. By disabling this
> > it works perfectly fine with IPV6. As it works for IPV4 so enabled
> > hw checksum back for IPV4.
> >
> > Verified with IPV6 enabled and can do ssh.
>
> So while this probably works, I don't think this is the right
> approach, at least according to the comments in skbuff.h
>
> This is not a matter of unsupported csum, it is broken hw csum.
> That's why we disable hw checksum. My guess is once we disable
> Hw checksum, it will use sw checksum. So I am just disabling hw
> Checksum.
I don't understand what you are saying. You reported a problem with
IPV6 checksums generation. The HW doesn't support it. What's "not a
matter of unsupported csum" ?
Your patch uses a *deprecated* bit to tell the network stack to only do
HW checksum generation on IPV4.
This bit is deprecated for a reason, again, see skbuff.h. The right
approach, *which the driver already does*, is to tell the stack that we
support HW checksuming using NETIF_F_HW_CSUM, and then, in the transmit
handler, to call skb_checksum_help() to have the SW calculate the
checksum if it's not a supported type.
My understanding was when we enable NETIF_F_HW_CSUM means network
stack enables HW checksum and doesn't calculate SW checksum. But as per
this supported types HW checksum are used only for IPV4 and not for IPV6 even
though driver enabled NETIF_F_HW_CSUM. For IPV6 it is always a SW generated
checksum, please correct me here.
This is exactly what ftgmac100_prep_tx_csum() does. It only enables HW
checksum generation on supported types and uses skb_checksum_help()
otherwise, supported types being protocol ETH_P_IP and IP protocol
being raw IP, TCP and UDP.
So this *should* have fallen back to SW for IPV6. So either something
in my code there is making an incorrect assumption, or something is
broken in skb_checksum_help() for IPV6 (which I somewhat doubt) or
something else I can't think of, but setting a *deprecated* flag is
definitely not the right answer, neither is completely disabling HW
checksumming.
So can you investigate what's going on a bit more closely please ? I
can try myself, though I have very little experience with IPV6 and
probably won't have time before next week.
Cheers,
Ben.
> The driver should have handled unsupported csum via SW fallback
> already in ftgmac100_prep_tx_csum()
>
> Can you check why this didn't work for you ?
>
> Cheers,
> Ben.
>
> > Signed-off-by: Vijay Khemka
> > ---
> > Changes since v1:
> > Enabled IPV4 hw checksum generation as it works for IPV4.
> >
> > drivers/net/ethernet/faraday/ftgmac100.c | 13 -
> > 1 file changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/faraday/ftgmac100.c
> > b/drivers/net/ethernet/faraday/ftgmac100.c
> > index 030fed65393e..0255a28d2958 100644
> > --- a/drivers/net/ethernet/faraday/ftgmac100.c
> > +++ b/drivers/net/ethernet/faraday/ftgmac100.c
> > @@ -1842,8 +1842,19 @@ static int ftgmac100_probe(struct
> > platform_device *pdev)
> > /* AST2400 doesn't have working HW checksum generation */
> > if (np && (of_device_is_compatible(np, "aspeed,ast2400-mac")))
> > netdev->hw_features &= ~NETIF_F_HW_CSUM;
> > +
> > + /* AST2500 doesn't have working HW checksum generation for IPV6
> > +* but it works for IPV4, so disabling hw checksum and enabling
> > +* it for only IPV4.
> > +*/
> > + if (np && (of_device_is_compatible(np, "aspeed,ast2500-mac")))
> > {
> > + netdev->hw_features &= ~NETIF_F_HW_CSUM;
> > + netdev->hw_features |= NETIF_F_IP_CSUM;
> > + }
> > +
> > if (np && of_get_property(np, "no-hw-checksum", NULL))
> > - netdev->hw_features &= ~(NETIF_F_HW_CSUM |
> > NETIF_F_RXCSUM);
> > + netdev->hw_features &= ~(NETIF_F_HW_CSUM |
> > NETIF_F_RXCSUM
> > +| NETIF_F_IP_CSUM);
> > netdev->features |= netdev->hw_features;
> >
> > /* register network device */
>
>
>
Re: [PATCH v2] ftgmac100: Disable HW checksum generation on AST2500
On 10/16/19, 6:29 PM, "Benjamin Herrenschmidt"
wrote:
On Fri, 2019-10-11 at 14:30 -0700, Vijay Khemka wrote:
> HW checksum generation is not working for AST2500, specially with
> IPV6
> over NCSI. All TCP packets with IPv6 get dropped. By disabling this
> it works perfectly fine with IPV6. As it works for IPV4 so enabled
> hw checksum back for IPV4.
>
> Verified with IPV6 enabled and can do ssh.
So while this probably works, I don't think this is the right
approach, at least according to the comments in skbuff.h
This is not a matter of unsupported csum, it is broken hw csum.
That's why we disable hw checksum. My guess is once we disable
Hw checksum, it will use sw checksum. So I am just disabling hw
Checksum.
The driver should have handled unsupported csum via SW fallback
already in ftgmac100_prep_tx_csum()
Can you check why this didn't work for you ?
Cheers,
Ben.
> Signed-off-by: Vijay Khemka
> ---
> Changes since v1:
> Enabled IPV4 hw checksum generation as it works for IPV4.
>
> drivers/net/ethernet/faraday/ftgmac100.c | 13 -
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/faraday/ftgmac100.c
> b/drivers/net/ethernet/faraday/ftgmac100.c
> index 030fed65393e..0255a28d2958 100644
> --- a/drivers/net/ethernet/faraday/ftgmac100.c
> +++ b/drivers/net/ethernet/faraday/ftgmac100.c
> @@ -1842,8 +1842,19 @@ static int ftgmac100_probe(struct
> platform_device *pdev)
> /* AST2400 doesn't have working HW checksum generation */
> if (np && (of_device_is_compatible(np, "aspeed,ast2400-mac")))
> netdev->hw_features &= ~NETIF_F_HW_CSUM;
> +
> + /* AST2500 doesn't have working HW checksum generation for IPV6
> + * but it works for IPV4, so disabling hw checksum and enabling
> + * it for only IPV4.
> + */
> + if (np && (of_device_is_compatible(np, "aspeed,ast2500-mac")))
> {
> + netdev->hw_features &= ~NETIF_F_HW_CSUM;
> + netdev->hw_features |= NETIF_F_IP_CSUM;
> + }
> +
> if (np && of_get_property(np, "no-hw-checksum", NULL))
> - netdev->hw_features &= ~(NETIF_F_HW_CSUM |
> NETIF_F_RXCSUM);
> + netdev->hw_features &= ~(NETIF_F_HW_CSUM |
> NETIF_F_RXCSUM
> + | NETIF_F_IP_CSUM);
> netdev->features |= netdev->hw_features;
>
> /* register network device */
Re: [PATCH] ftgmac100: Disable HW checksum generation on AST2500
On 10/10/19, 8:11 PM, "Benjamin Herrenschmidt" wrote: On Thu, 2019-10-10 at 19:15 +0000, Vijay Khemka wrote: > Any news on this ? AST2400 has no HW checksum logic in HW, AST2500 > should work for IPV4 fine, we should only selectively disable it for > IPV6. > > Ben, I have already sent v2 for this with requested change which only disable > for IPV6 in AST2500. I can send it again. I didn't see it, did you CC me ? I maintain that driver... Let me send again via git send-email. Cheers, Ben.
Re: [PATCH v2] ftgmac100: Disable HW checksum generation on AST2500
Resending this patch again.
On 9/11/19, 1:05 PM, "Vijay Khemka" wrote:
HW checksum generation is not working for AST2500, specially with IPV6
over NCSI. All TCP packets with IPv6 get dropped. By disabling this
it works perfectly fine with IPV6. As it works for IPV4 so enabled
hw checksum back for IPV4.
Verified with IPV6 enabled and can do ssh.
Signed-off-by: Vijay Khemka
---
Changes since v1:
Enabled IPV4 hw checksum generation as it works for IPV4.
drivers/net/ethernet/faraday/ftgmac100.c | 13 -
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/faraday/ftgmac100.c
b/drivers/net/ethernet/faraday/ftgmac100.c
index 030fed65393e..0255a28d2958 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -1842,8 +1842,19 @@ static int ftgmac100_probe(struct platform_device
*pdev)
/* AST2400 doesn't have working HW checksum generation */
if (np && (of_device_is_compatible(np, "aspeed,ast2400-mac")))
netdev->hw_features &= ~NETIF_F_HW_CSUM;
+
+ /* AST2500 doesn't have working HW checksum generation for IPV6
+* but it works for IPV4, so disabling hw checksum and enabling
+* it for only IPV4.
+*/
+ if (np && (of_device_is_compatible(np, "aspeed,ast2500-mac"))) {
+ netdev->hw_features &= ~NETIF_F_HW_CSUM;
+ netdev->hw_features |= NETIF_F_IP_CSUM;
+ }
+
if (np && of_get_property(np, "no-hw-checksum", NULL))
- netdev->hw_features &= ~(NETIF_F_HW_CSUM | NETIF_F_RXCSUM);
+ netdev->hw_features &= ~(NETIF_F_HW_CSUM | NETIF_F_RXCSUM
+| NETIF_F_IP_CSUM);
netdev->features |= netdev->hw_features;
/* register network device */
--
2.17.1
Re: [PATCH] ftgmac100: Disable HW checksum generation on AST2500
On 10/8/19, 9:37 PM, "Benjamin Herrenschmidt" wrote:
On Wed, 2019-09-11 at 14:48 +, Joel Stanley wrote:
> Hi Ben,
>
> On Tue, 10 Sep 2019 at 22:05, Florian Fainelli
> wrote:
> >
> > On 9/10/19 2:37 PM, Vijay Khemka wrote:
> > > HW checksum generation is not working for AST2500, specially with
> > > IPV6
> > > over NCSI. All TCP packets with IPv6 get dropped. By disabling
> > > this
> > > it works perfectly fine with IPV6.
> > >
> > > Verified with IPV6 enabled and can do ssh.
> >
> > How about IPv4, do these packets have problem? If not, can you
> > continue
> > advertising NETIF_F_IP_CSUM but take out NETIF_F_IPV6_CSUM?
> >
> > >
> > > Signed-off-by: Vijay Khemka
> > > ---
> > > drivers/net/ethernet/faraday/ftgmac100.c | 5 +++--
> > > 1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/faraday/ftgmac100.c
> > > b/drivers/net/ethernet/faraday/ftgmac100.c
> > > index 030fed65393e..591c9725002b 100644
> > > --- a/drivers/net/ethernet/faraday/ftgmac100.c
> > > +++ b/drivers/net/ethernet/faraday/ftgmac100.c
> > > @@ -1839,8 +1839,9 @@ static int ftgmac100_probe(struct
> > > platform_device *pdev)
> > > if (priv->use_ncsi)
> > > netdev->hw_features |= NETIF_F_HW_VLAN_CTAG_FILTER;
> > >
> > > - /* AST2400 doesn't have working HW checksum generation */
> > > - if (np && (of_device_is_compatible(np, "aspeed,ast2400-
> > > mac")))
> > > + /* AST2400 and AST2500 doesn't have working HW checksum
> > > generation */
> > > + if (np && (of_device_is_compatible(np, "aspeed,ast2400-
> > > mac") ||
> > > +of_device_is_compatible(np, "aspeed,ast2500-
> > > mac")))
>
> Do you recall under what circumstances we need to disable hardware
> checksumming?
Any news on this ? AST2400 has no HW checksum logic in HW, AST2500
should work for IPV4 fine, we should only selectively disable it for
IPV6.
Ben, I have already sent v2 for this with requested change which only disable
for IPV6 in AST2500. I can send it again.
Can you do an updated patch ?
Cheers,
Ben.
Re: [PATCH v2] ftgmac100: Disable HW checksum generation on AST2500
Florian/Joel,
Can you please look into below patch and let me know who can apply this.
Regards
-Vijay
On 9/17/19, 12:34 PM, "Vijay Khemka" wrote:
Please review below patch and provide your valuable feedback.
Regards
-Vijay
On 9/11/19, 1:05 PM, "Vijay Khemka" wrote:
HW checksum generation is not working for AST2500, specially with IPV6
over NCSI. All TCP packets with IPv6 get dropped. By disabling this
it works perfectly fine with IPV6. As it works for IPV4 so enabled
hw checksum back for IPV4.
Verified with IPV6 enabled and can do ssh.
Signed-off-by: Vijay Khemka
---
Changes since v1:
Enabled IPV4 hw checksum generation as it works for IPV4.
drivers/net/ethernet/faraday/ftgmac100.c | 13 -
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/faraday/ftgmac100.c
b/drivers/net/ethernet/faraday/ftgmac100.c
index 030fed65393e..0255a28d2958 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -1842,8 +1842,19 @@ static int ftgmac100_probe(struct
platform_device *pdev)
/* AST2400 doesn't have working HW checksum generation */
if (np && (of_device_is_compatible(np, "aspeed,ast2400-mac")))
netdev->hw_features &= ~NETIF_F_HW_CSUM;
+
+ /* AST2500 doesn't have working HW checksum generation for IPV6
+* but it works for IPV4, so disabling hw checksum and enabling
+* it for only IPV4.
+*/
+ if (np && (of_device_is_compatible(np, "aspeed,ast2500-mac"))) {
+ netdev->hw_features &= ~NETIF_F_HW_CSUM;
+ netdev->hw_features |= NETIF_F_IP_CSUM;
+ }
+
if (np && of_get_property(np, "no-hw-checksum", NULL))
- netdev->hw_features &= ~(NETIF_F_HW_CSUM |
NETIF_F_RXCSUM);
+ netdev->hw_features &= ~(NETIF_F_HW_CSUM |
NETIF_F_RXCSUM
+| NETIF_F_IP_CSUM);
netdev->features |= netdev->hw_features;
/* register network device */
--
2.17.1
Re: [PATCH v2] ftgmac100: Disable HW checksum generation on AST2500
Please review below patch and provide your valuable feedback.
Regards
-Vijay
On 9/11/19, 1:05 PM, "Vijay Khemka" wrote:
HW checksum generation is not working for AST2500, specially with IPV6
over NCSI. All TCP packets with IPv6 get dropped. By disabling this
it works perfectly fine with IPV6. As it works for IPV4 so enabled
hw checksum back for IPV4.
Verified with IPV6 enabled and can do ssh.
Signed-off-by: Vijay Khemka
---
Changes since v1:
Enabled IPV4 hw checksum generation as it works for IPV4.
drivers/net/ethernet/faraday/ftgmac100.c | 13 -
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/faraday/ftgmac100.c
b/drivers/net/ethernet/faraday/ftgmac100.c
index 030fed65393e..0255a28d2958 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -1842,8 +1842,19 @@ static int ftgmac100_probe(struct platform_device
*pdev)
/* AST2400 doesn't have working HW checksum generation */
if (np && (of_device_is_compatible(np, "aspeed,ast2400-mac")))
netdev->hw_features &= ~NETIF_F_HW_CSUM;
+
+ /* AST2500 doesn't have working HW checksum generation for IPV6
+* but it works for IPV4, so disabling hw checksum and enabling
+* it for only IPV4.
+*/
+ if (np && (of_device_is_compatible(np, "aspeed,ast2500-mac"))) {
+ netdev->hw_features &= ~NETIF_F_HW_CSUM;
+ netdev->hw_features |= NETIF_F_IP_CSUM;
+ }
+
if (np && of_get_property(np, "no-hw-checksum", NULL))
- netdev->hw_features &= ~(NETIF_F_HW_CSUM | NETIF_F_RXCSUM);
+ netdev->hw_features &= ~(NETIF_F_HW_CSUM | NETIF_F_RXCSUM
+| NETIF_F_IP_CSUM);
netdev->features |= netdev->hw_features;
/* register network device */
--
2.17.1
Re: [PATCH] net/ncsi: Disable global multicast filter
On 9/15/19, 7:39 PM, "Samuel Mendoza-Jonas" wrote:
On Thu, 2019-09-12 at 12:04 -0700, Vijay Khemka wrote:
> Disabling multicast filtering from NCSI if it is supported. As it
> should not filter any multicast packets. In current code, multicast
> filter is enabled and with an exception of optional field supported
> by device are disabled filtering.
>
> Mainly I see if goal is to disable filtering for IPV6 packets then
> let
> it disabled for every other types as well. As we are seeing issues
> with
> LLDP not working with this enabled filtering. And there are other
> issues
> with IPV6.
>
> By Disabling this multicast completely, it is working for both IPV6
> as
> well as LLDP.
>
> Signed-off-by: Vijay Khemka
Hi Vijay,
There are definitely some current issues with multicast filtering and
IPv6 when behind NC-SI at the moment. It would be nice to make this
configurable instead of disabling the component wholesale but I don't
believe this actually *breaks* anyone's configuration. It would be nice
to see some Tested-By's from the OpenBMC people though.
I'll have a look at the multicast issues, CC'ing in Chris too who IIRC
was looking at similar issues for u-bmc in case he got further.
Sam,
The current multicast issues for IPV6 are vendor's issues. And if some is
not supporting IPV6 forwarding to management controller bit fields in
EGMF then we are filtering it and by disabling filtering it works for Mellanox
And Broadcom cards. I have tested it. That's why I have disabled it to start
with
And it can be enabled from netlink utility if required.
Acked-by: Samuel Mendoza-Jonas
> ---
> net/ncsi/internal.h| 7 +--
> net/ncsi/ncsi-manage.c | 98 +---
> --
> 2 files changed, 12 insertions(+), 93 deletions(-)
>
> diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h
> index 0b3f0673e1a2..ad3fd7f1da75 100644
> --- a/net/ncsi/internal.h
> +++ b/net/ncsi/internal.h
> @@ -264,9 +264,7 @@ enum {
> ncsi_dev_state_config_ev,
> ncsi_dev_state_config_sma,
> ncsi_dev_state_config_ebf,
> -#if IS_ENABLED(CONFIG_IPV6)
> - ncsi_dev_state_config_egmf,
> -#endif
> + ncsi_dev_state_config_dgmf,
> ncsi_dev_state_config_ecnt,
> ncsi_dev_state_config_ec,
> ncsi_dev_state_config_ae,
> @@ -295,9 +293,6 @@ struct ncsi_dev_priv {
> #define NCSI_DEV_RESET 8/* Reset state of
> NC */
> unsigned intgma_flag;/* OEM GMA
> flag */
> spinlock_t lock;/* Protect the NCSI
> device*/
> -#if IS_ENABLED(CONFIG_IPV6)
> - unsigned intinet6_addr_num; /* Number of IPv6
> addresses */
> -#endif
> unsigned intpackage_probe_id;/* Current ID during
> probe*/
> unsigned intpackage_num; /* Number of
> packages */
> struct list_headpackages;/* List of
> packages */
> diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
> index 755aab66dcab..bce8b443289d 100644
> --- a/net/ncsi/ncsi-manage.c
> +++ b/net/ncsi/ncsi-manage.c
> @@ -14,7 +14,6 @@
> #include
> #include
> #include
> -#include
> #include
>
> #include "internal.h"
> @@ -978,9 +977,7 @@ static void ncsi_configure_channel(struct
> ncsi_dev_priv *ndp)
> case ncsi_dev_state_config_ev:
> case ncsi_dev_state_config_sma:
> case ncsi_dev_state_config_ebf:
> -#if IS_ENABLED(CONFIG_IPV6)
> - case ncsi_dev_state_config_egmf:
> -#endif
> + case ncsi_dev_state_config_dgmf:
> case ncsi_dev_state_config_ecnt:
> case ncsi_dev_state_config_ec:
> case ncsi_dev_state_config_ae:
> @@ -1033,23 +1030,23 @@ static void ncsi_configure_channel(struct
> ncsi_dev_priv *ndp)
> } else if (nd->state == ncsi_dev_state_config_ebf) {
> nca.type = NCSI_PKT_CMD_EBF;
> nca.dwords[0] = nc->caps[NCSI_CAP_BC].cap;
> - if (ncsi_channel_is_tx(ndp, nc))
> + /* if multicast global filtering is supported
> then
> + * disable it so that all multicast packet will
> be
> + * forwarded to management controller
> + */
> + if (nc->caps[NCSI_CAP_GENERIC].cap &
> + NCSI_CAP_GENERIC_MC)
> +
[PATCH] net/ncsi: Disable global multicast filter
Disabling multicast filtering from NCSI if it is supported. As it
should not filter any multicast packets. In current code, multicast
filter is enabled and with an exception of optional field supported
by device are disabled filtering.
Mainly I see if goal is to disable filtering for IPV6 packets then let
it disabled for every other types as well. As we are seeing issues with
LLDP not working with this enabled filtering. And there are other issues
with IPV6.
By Disabling this multicast completely, it is working for both IPV6 as
well as LLDP.
Signed-off-by: Vijay Khemka
---
net/ncsi/internal.h| 7 +--
net/ncsi/ncsi-manage.c | 98 +-
2 files changed, 12 insertions(+), 93 deletions(-)
diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h
index 0b3f0673e1a2..ad3fd7f1da75 100644
--- a/net/ncsi/internal.h
+++ b/net/ncsi/internal.h
@@ -264,9 +264,7 @@ enum {
ncsi_dev_state_config_ev,
ncsi_dev_state_config_sma,
ncsi_dev_state_config_ebf,
-#if IS_ENABLED(CONFIG_IPV6)
- ncsi_dev_state_config_egmf,
-#endif
+ ncsi_dev_state_config_dgmf,
ncsi_dev_state_config_ecnt,
ncsi_dev_state_config_ec,
ncsi_dev_state_config_ae,
@@ -295,9 +293,6 @@ struct ncsi_dev_priv {
#define NCSI_DEV_RESET 8/* Reset state of NC */
unsigned intgma_flag;/* OEM GMA flag */
spinlock_t lock;/* Protect the NCSI device*/
-#if IS_ENABLED(CONFIG_IPV6)
- unsigned intinet6_addr_num; /* Number of IPv6 addresses */
-#endif
unsigned intpackage_probe_id;/* Current ID during probe*/
unsigned intpackage_num; /* Number of packages */
struct list_headpackages;/* List of packages */
diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
index 755aab66dcab..bce8b443289d 100644
--- a/net/ncsi/ncsi-manage.c
+++ b/net/ncsi/ncsi-manage.c
@@ -14,7 +14,6 @@
#include
#include
#include
-#include
#include
#include "internal.h"
@@ -978,9 +977,7 @@ static void ncsi_configure_channel(struct ncsi_dev_priv
*ndp)
case ncsi_dev_state_config_ev:
case ncsi_dev_state_config_sma:
case ncsi_dev_state_config_ebf:
-#if IS_ENABLED(CONFIG_IPV6)
- case ncsi_dev_state_config_egmf:
-#endif
+ case ncsi_dev_state_config_dgmf:
case ncsi_dev_state_config_ecnt:
case ncsi_dev_state_config_ec:
case ncsi_dev_state_config_ae:
@@ -1033,23 +1030,23 @@ static void ncsi_configure_channel(struct ncsi_dev_priv
*ndp)
} else if (nd->state == ncsi_dev_state_config_ebf) {
nca.type = NCSI_PKT_CMD_EBF;
nca.dwords[0] = nc->caps[NCSI_CAP_BC].cap;
- if (ncsi_channel_is_tx(ndp, nc))
+ /* if multicast global filtering is supported then
+* disable it so that all multicast packet will be
+* forwarded to management controller
+*/
+ if (nc->caps[NCSI_CAP_GENERIC].cap &
+NCSI_CAP_GENERIC_MC)
+ nd->state = ncsi_dev_state_config_dgmf;
+ else if (ncsi_channel_is_tx(ndp, nc))
nd->state = ncsi_dev_state_config_ecnt;
else
nd->state = ncsi_dev_state_config_ec;
-#if IS_ENABLED(CONFIG_IPV6)
- if (ndp->inet6_addr_num > 0 &&
- (nc->caps[NCSI_CAP_GENERIC].cap &
-NCSI_CAP_GENERIC_MC))
- nd->state = ncsi_dev_state_config_egmf;
- } else if (nd->state == ncsi_dev_state_config_egmf) {
- nca.type = NCSI_PKT_CMD_EGMF;
- nca.dwords[0] = nc->caps[NCSI_CAP_MC].cap;
+ } else if (nd->state == ncsi_dev_state_config_dgmf) {
+ nca.type = NCSI_PKT_CMD_DGMF;
if (ncsi_channel_is_tx(ndp, nc))
nd->state = ncsi_dev_state_config_ecnt;
else
nd->state = ncsi_dev_state_config_ec;
-#endif /* CONFIG_IPV6 */
} else if (nd->state == ncsi_dev_state_config_ecnt) {
if (np->preferred_channel &&
nc != np->preferred_channel)
@@ -1483,70 +1480,6 @@ int ncsi_process_next_channel(struct ncsi_dev_priv *ndp)
return -ENODEV;
}
-#if IS_ENABLED(CONFIG_IPV6)
-static int ncsi_inet6addr_event(struct notifier_block *this,
- unsigned long event, void *data)
-{
- struct inet6_ifaddr *ifa = data;
- struct net_device
Re: [PATCH] ftgmac100: Disable HW checksum generation on AST2500
On 9/11/19, 11:34 AM, "Florian Fainelli" wrote: On 9/11/19 11:30 AM, Vijay Khemka wrote: > > > On 9/10/19, 4:08 PM, "Linux-aspeed on behalf of Vijay Khemka" wrote: > > > > On 9/10/19, 3:50 PM, "Linux-aspeed on behalf of Vijay Khemka" wrote: > > > > On 9/10/19, 3:05 PM, "Florian Fainelli" wrote: > > On 9/10/19 2:37 PM, Vijay Khemka wrote: > > HW checksum generation is not working for AST2500, specially with IPV6 > > over NCSI. All TCP packets with IPv6 get dropped. By disabling this > > it works perfectly fine with IPV6. > > > > Verified with IPV6 enabled and can do ssh. > > How about IPv4, do these packets have problem? If not, can you continue > advertising NETIF_F_IP_CSUM but take out NETIF_F_IPV6_CSUM? > > I changed code from (netdev->hw_features &= ~NETIF_F_HW_CSUM) to > (netdev->hw_features &= ~NETIF_F_ IPV6_CSUM). And it is not working. > Don't know why. IPV4 works without any change but IPv6 needs HW_CSUM > Disabled. > > Now I changed to > netdev->hw_features &= (~NETIF_F_HW_CSUM) | NETIF_F_IP_CSUM; > And it works. > > I investigated more on these features and found that we cannot set NETIF_F_IP_CSUM > While NETIF_F_HW_CSUM is set. So I disabled NETIF_F_HW_CSUM first and enabled > NETIF_F_IP_CSUM in next statement. And it works fine. > > But as per line 166 in include/linux/skbuff.h, > * NETIF_F_IP_CSUM and NETIF_F_IPV6_CSUM are being deprecated in favor of > * NETIF_F_HW_CSUM. New devices should use NETIF_F_HW_CSUM to indicate > * checksum offload capability. > > Please suggest which of below 2 I should do. As both works for me. > 1. Disable completely NETIF_F_HW_CSUM and do nothing. This is original patch. > 2. Enable NETIF_F_IP_CSUM in addition to 1. I can have v2 if this is accepted. Sounds like 2 would leave the option of offloading IPv4 checksum offload, so that would be a better middle group than flat out disable checksum offload for both IPv4 and IPv6, no? Sounds good, I will have v2 after lot more testing. -- Florian
Re: [PATCH] ftgmac100: Disable HW checksum generation on AST2500
On 9/10/19, 4:08 PM, "Linux-aspeed on behalf of Vijay Khemka"
wrote:
On 9/10/19, 3:50 PM, "Linux-aspeed on behalf of Vijay Khemka"
wrote:
On 9/10/19, 3:05 PM, "Florian Fainelli" wrote:
On 9/10/19 2:37 PM, Vijay Khemka wrote:
> HW checksum generation is not working for AST2500, specially with
IPV6
> over NCSI. All TCP packets with IPv6 get dropped. By disabling
this
> it works perfectly fine with IPV6.
>
> Verified with IPV6 enabled and can do ssh.
How about IPv4, do these packets have problem? If not, can you
continue
advertising NETIF_F_IP_CSUM but take out NETIF_F_IPV6_CSUM?
I changed code from (netdev->hw_features &= ~NETIF_F_HW_CSUM) to
(netdev->hw_features &= ~NETIF_F_ IPV6_CSUM). And it is not working.
Don't know why. IPV4 works without any change but IPv6 needs HW_CSUM
Disabled.
Now I changed to
netdev->hw_features &= (~NETIF_F_HW_CSUM) | NETIF_F_IP_CSUM;
And it works.
I investigated more on these features and found that we cannot set
NETIF_F_IP_CSUM
While NETIF_F_HW_CSUM is set. So I disabled NETIF_F_HW_CSUM first and enabled
NETIF_F_IP_CSUM in next statement. And it works fine.
But as per line 166 in include/linux/skbuff.h,
* NETIF_F_IP_CSUM and NETIF_F_IPV6_CSUM are being deprecated in favor of
* NETIF_F_HW_CSUM. New devices should use NETIF_F_HW_CSUM to indicate
* checksum offload capability.
Please suggest which of below 2 I should do. As both works for me.
1. Disable completely NETIF_F_HW_CSUM and do nothing. This is original patch.
2. Enable NETIF_F_IP_CSUM in addition to 1. I can have v2 if this is accepted.
>
> Signed-off-by: Vijay Khemka
> ---
> drivers/net/ethernet/faraday/ftgmac100.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/faraday/ftgmac100.c
b/drivers/net/ethernet/faraday/ftgmac100.c
> index 030fed65393e..591c9725002b 100644
> --- a/drivers/net/ethernet/faraday/ftgmac100.c
> +++ b/drivers/net/ethernet/faraday/ftgmac100.c
> @@ -1839,8 +1839,9 @@ static int ftgmac100_probe(struct
platform_device *pdev)
> if (priv->use_ncsi)
> netdev->hw_features |= NETIF_F_HW_VLAN_CTAG_FILTER;
>
> - /* AST2400 doesn't have working HW checksum generation */
> - if (np && (of_device_is_compatible(np, "aspeed,ast2400-mac")))
> + /* AST2400 and AST2500 doesn't have working HW checksum
generation */
> + if (np && (of_device_is_compatible(np, "aspeed,ast2400-mac") ||
> +of_device_is_compatible(np, "aspeed,ast2500-mac")))
> netdev->hw_features &= ~NETIF_F_HW_CSUM;
> if (np && of_get_property(np, "no-hw-checksum", NULL))
> netdev->hw_features &= ~(NETIF_F_HW_CSUM |
NETIF_F_RXCSUM);
>
--
Florian
Re: [PATCH 1/4] gpio/aspeed: Fix incorrect number of banks
On 9/11/19, 5:16 AM, "Linux-aspeed on behalf of Rashmica Gupta"
wrote:
Fixes: 361b79119a4b7 ('gpio: Add Aspeed driver')
Signed-off-by: Rashmica Gupta
---
drivers/gpio/gpio-aspeed.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpio/gpio-aspeed.c b/drivers/gpio/gpio-aspeed.c
index 9defe25d4721..77752b2624e8 100644
--- a/drivers/gpio/gpio-aspeed.c
+++ b/drivers/gpio/gpio-aspeed.c
@@ -1165,7 +1165,7 @@ static int __init aspeed_gpio_probe(struct
platform_device *pdev)
gpio->chip.base = -1;
/* Allocate a cache of the output registers */
- banks = gpio->config->nr_gpios >> 5;
+ banks = (gpio->config->nr_gpios >> 5) + 1;
If number of gpios are 32 then it should be only 1 bank, as per above it is 2
bank.
gpio->dcache = devm_kcalloc(>dev,
banks, sizeof(u32), GFP_KERNEL);
if (!gpio->dcache)
--
2.20.1
Re: [PATCH] ftgmac100: Disable HW checksum generation on AST2500
On 9/11/19, 7:49 AM, "Joel Stanley" wrote: Hi Ben, On Tue, 10 Sep 2019 at 22:05, Florian Fainelli wrote: > > On 9/10/19 2:37 PM, Vijay Khemka wrote: > > HW checksum generation is not working for AST2500, specially with IPV6 > > over NCSI. All TCP packets with IPv6 get dropped. By disabling this > > it works perfectly fine with IPV6. > > > > Verified with IPV6 enabled and can do ssh. > > How about IPv4, do these packets have problem? If not, can you continue > advertising NETIF_F_IP_CSUM but take out NETIF_F_IPV6_CSUM? > > > > > Signed-off-by: Vijay Khemka > > --- > > drivers/net/ethernet/faraday/ftgmac100.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c > > index 030fed65393e..591c9725002b 100644 > > --- a/drivers/net/ethernet/faraday/ftgmac100.c > > +++ b/drivers/net/ethernet/faraday/ftgmac100.c > > @@ -1839,8 +1839,9 @@ static int ftgmac100_probe(struct platform_device *pdev) > > if (priv->use_ncsi) > > netdev->hw_features |= NETIF_F_HW_VLAN_CTAG_FILTER; > > > > - /* AST2400 doesn't have working HW checksum generation */ > > - if (np && (of_device_is_compatible(np, "aspeed,ast2400-mac"))) > > + /* AST2400 and AST2500 doesn't have working HW checksum generation */ > > + if (np && (of_device_is_compatible(np, "aspeed,ast2400-mac") || > > +of_device_is_compatible(np, "aspeed,ast2500-mac"))) Do you recall under what circumstances we need to disable hardware checksumming? Mainly, TCP packets over IPV6 getting dropped. After disabling it was working. Cheers, Joel > > netdev->hw_features &= ~NETIF_F_HW_CSUM; > > if (np && of_get_property(np, "no-hw-checksum", NULL)) > > netdev->hw_features &= ~(NETIF_F_HW_CSUM | NETIF_F_RXCSUM); > > > > > -- > Florian
Re: [PATCH] ftgmac100: Disable HW checksum generation on AST2500
On 9/10/19, 3:50 PM, "Linux-aspeed on behalf of Vijay Khemka"
wrote:
On 9/10/19, 3:05 PM, "Florian Fainelli" wrote:
On 9/10/19 2:37 PM, Vijay Khemka wrote:
> HW checksum generation is not working for AST2500, specially with IPV6
> over NCSI. All TCP packets with IPv6 get dropped. By disabling this
> it works perfectly fine with IPV6.
>
> Verified with IPV6 enabled and can do ssh.
How about IPv4, do these packets have problem? If not, can you continue
advertising NETIF_F_IP_CSUM but take out NETIF_F_IPV6_CSUM?
I changed code from (netdev->hw_features &= ~NETIF_F_HW_CSUM) to
(netdev->hw_features &= ~NETIF_F_ IPV6_CSUM). And it is not working.
Don't know why. IPV4 works without any change but IPv6 needs HW_CSUM
Disabled.
Now I changed to
netdev->hw_features &= (~NETIF_F_HW_CSUM) | NETIF_F_IP_CSUM;
And it works.
>
> Signed-off-by: Vijay Khemka
> ---
> drivers/net/ethernet/faraday/ftgmac100.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/faraday/ftgmac100.c
b/drivers/net/ethernet/faraday/ftgmac100.c
> index 030fed65393e..591c9725002b 100644
> --- a/drivers/net/ethernet/faraday/ftgmac100.c
> +++ b/drivers/net/ethernet/faraday/ftgmac100.c
> @@ -1839,8 +1839,9 @@ static int ftgmac100_probe(struct
platform_device *pdev)
> if (priv->use_ncsi)
> netdev->hw_features |= NETIF_F_HW_VLAN_CTAG_FILTER;
>
> - /* AST2400 doesn't have working HW checksum generation */
> - if (np && (of_device_is_compatible(np, "aspeed,ast2400-mac")))
> + /* AST2400 and AST2500 doesn't have working HW checksum
generation */
> + if (np && (of_device_is_compatible(np, "aspeed,ast2400-mac") ||
> +of_device_is_compatible(np, "aspeed,ast2500-mac")))
> netdev->hw_features &= ~NETIF_F_HW_CSUM;
> if (np && of_get_property(np, "no-hw-checksum", NULL))
> netdev->hw_features &= ~(NETIF_F_HW_CSUM |
NETIF_F_RXCSUM);
>
--
Florian
Re: [PATCH] ftgmac100: Disable HW checksum generation on AST2500
On 9/10/19, 3:05 PM, "Florian Fainelli" wrote:
On 9/10/19 2:37 PM, Vijay Khemka wrote:
> HW checksum generation is not working for AST2500, specially with IPV6
> over NCSI. All TCP packets with IPv6 get dropped. By disabling this
> it works perfectly fine with IPV6.
>
> Verified with IPV6 enabled and can do ssh.
How about IPv4, do these packets have problem? If not, can you continue
advertising NETIF_F_IP_CSUM but take out NETIF_F_IPV6_CSUM?
I changed code from (netdev->hw_features &= ~NETIF_F_HW_CSUM) to
(netdev->hw_features &= ~NETIF_F_ IPV6_CSUM). And it is not working.
Don't know why. IPV4 works without any change but IPv6 needs HW_CSUM
Disabled.
>
> Signed-off-by: Vijay Khemka
> ---
> drivers/net/ethernet/faraday/ftgmac100.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/faraday/ftgmac100.c
b/drivers/net/ethernet/faraday/ftgmac100.c
> index 030fed65393e..591c9725002b 100644
> --- a/drivers/net/ethernet/faraday/ftgmac100.c
> +++ b/drivers/net/ethernet/faraday/ftgmac100.c
> @@ -1839,8 +1839,9 @@ static int ftgmac100_probe(struct platform_device
*pdev)
> if (priv->use_ncsi)
> netdev->hw_features |= NETIF_F_HW_VLAN_CTAG_FILTER;
>
> - /* AST2400 doesn't have working HW checksum generation */
> - if (np && (of_device_is_compatible(np, "aspeed,ast2400-mac")))
> + /* AST2400 and AST2500 doesn't have working HW checksum generation */
> + if (np && (of_device_is_compatible(np, "aspeed,ast2400-mac") ||
> +of_device_is_compatible(np, "aspeed,ast2500-mac")))
> netdev->hw_features &= ~NETIF_F_HW_CSUM;
> if (np && of_get_property(np, "no-hw-checksum", NULL))
> netdev->hw_features &= ~(NETIF_F_HW_CSUM | NETIF_F_RXCSUM);
>
--
Florian
Re: [PATCH] ftgmac100: Disable HW checksum generation on AST2500
On 9/10/19, 3:05 PM, "Florian Fainelli" wrote:
On 9/10/19 2:37 PM, Vijay Khemka wrote:
> HW checksum generation is not working for AST2500, specially with IPV6
> over NCSI. All TCP packets with IPv6 get dropped. By disabling this
> it works perfectly fine with IPV6.
>
> Verified with IPV6 enabled and can do ssh.
How about IPv4, do these packets have problem? If not, can you continue
advertising NETIF_F_IP_CSUM but take out NETIF_F_IPV6_CSUM?
Yes IPv4 works. Let me test with your suggestion and will update patch.
>
> Signed-off-by: Vijay Khemka
> ---
> drivers/net/ethernet/faraday/ftgmac100.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/faraday/ftgmac100.c
b/drivers/net/ethernet/faraday/ftgmac100.c
> index 030fed65393e..591c9725002b 100644
> --- a/drivers/net/ethernet/faraday/ftgmac100.c
> +++ b/drivers/net/ethernet/faraday/ftgmac100.c
> @@ -1839,8 +1839,9 @@ static int ftgmac100_probe(struct platform_device
*pdev)
> if (priv->use_ncsi)
> netdev->hw_features |= NETIF_F_HW_VLAN_CTAG_FILTER;
>
> - /* AST2400 doesn't have working HW checksum generation */
> - if (np && (of_device_is_compatible(np, "aspeed,ast2400-mac")))
> + /* AST2400 and AST2500 doesn't have working HW checksum generation */
> + if (np && (of_device_is_compatible(np, "aspeed,ast2400-mac") ||
> +of_device_is_compatible(np, "aspeed,ast2500-mac")))
> netdev->hw_features &= ~NETIF_F_HW_CSUM;
> if (np && of_get_property(np, "no-hw-checksum", NULL))
> netdev->hw_features &= ~(NETIF_F_HW_CSUM | NETIF_F_RXCSUM);
>
--
Florian
Re: [PATCH v2 1/4] gpio/aspeed: Fix incorrect number of banks
On 9/4/19, 6:17 PM, "Linux-aspeed on behalf of Rashmica Gupta"
wrote:
The current calculation for the number of GPIO banks is only correct if
the number of GPIOs is a multiple of 32 (if there were 31 GPIOs we would
currently say there are 0 banks, which is incorrect).
Fixes: 361b79119a4b7 ('gpio: Add Aspeed driver')
Signed-off-by: Rashmica Gupta
Reviewed-by: Vijay Khemka
---
drivers/gpio/gpio-aspeed.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpio/gpio-aspeed.c b/drivers/gpio/gpio-aspeed.c
index 9defe25d4721..b83e23aecd18 100644
--- a/drivers/gpio/gpio-aspeed.c
+++ b/drivers/gpio/gpio-aspeed.c
@@ -1165,7 +1165,7 @@ static int __init aspeed_gpio_probe(struct
platform_device *pdev)
gpio->chip.base = -1;
/* Allocate a cache of the output registers */
- banks = gpio->config->nr_gpios >> 5;
+ banks = DIV_ROUND_UP(gpio->config->nr_gpios, 32);
Good catch
gpio->dcache = devm_kcalloc(>dev,
banks, sizeof(u32), GFP_KERNEL);
if (!gpio->dcache)
--
2.20.1
Re: [PATCH] net/ncsi: Fix the payload copying for the request coming from Netlink
LGTM.
On 8/21/19, 2:26 PM, "openbmc on behalf of justin.l...@dell.com"
wrote:
The request coming from Netlink should use the OEM generic handler.
The standard command handler expects payload in bytes/words/dwords
but the actual payload is stored in data if the request is coming from
Netlink.
Signed-off-by: Justin Lee
Reviewed-by: Vijay Khemka
---
net/ncsi/ncsi-cmd.c | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/net/ncsi/ncsi-cmd.c b/net/ncsi/ncsi-cmd.c
index eab4346..0187e65 100644
--- a/net/ncsi/ncsi-cmd.c
+++ b/net/ncsi/ncsi-cmd.c
@@ -309,14 +309,21 @@ static struct ncsi_request *ncsi_alloc_command(struct
ncsi_cmd_arg *nca)
int ncsi_xmit_cmd(struct ncsi_cmd_arg *nca)
{
+ struct ncsi_cmd_handler *nch = NULL;
struct ncsi_request *nr;
+ unsigned char type;
struct ethhdr *eh;
- struct ncsi_cmd_handler *nch = NULL;
int i, ret;
+ /* Use OEM generic handler for Netlink request */
+ if (nca->req_flags == NCSI_REQ_FLAG_NETLINK_DRIVEN)
+ type = NCSI_PKT_CMD_OEM;
+ else
+ type = nca->type;
+
/* Search for the handler */
for (i = 0; i < ARRAY_SIZE(ncsi_cmd_handlers); i++) {
- if (ncsi_cmd_handlers[i].type == nca->type) {
+ if (ncsi_cmd_handlers[i].type == type) {
if (ncsi_cmd_handlers[i].handler)
nch = _cmd_handlers[i];
else
--
2.9.3
Re: [PATCH v5 2/2] hwmon: pmbus: Add Inspur Power System power supply driver
On 8/16/19, 3:20 AM, "openbmc on behalf of John Wang"
wrote:
Add the driver to monitor Inspur Power System power supplies
with hwmon over pmbus.
This driver adds sysfs attributes for additional power supply data,
including vendor, model, part_number, serial number,
firmware revision, hardware revision, and psu mode(active/standby).
Signed-off-by: John Wang
---
v5:
- Align sysfs attrs description in inspur-ipsps1.rst
(Use tab instead of space to sperate names and values)
v4:
- Remove the additional tabs in the Makefile
- Rebased on 5.3-rc4, not 5.2
v3:
- Sort kconfig/makefile entries alphabetically
- Remove unnecessary initialization
- Use ATTRIBUTE_GROUPS instead of expanding directly
- Use memscan to avoid reimplementation
v2:
- Fix typos in commit message
- Invert Christmas tree
- Configure device with sysfs attrs, not debugfs entries
- Fix errno in fw_version_read, ENODATA to EPROTO
- Change the print format of fw-version
- Use sysfs_streq instead of strcmp("xxx" "\n", "xxx")
- Document sysfs attributes
---
Documentation/hwmon/inspur-ipsps1.rst | 79 +
drivers/hwmon/pmbus/Kconfig | 9 +
drivers/hwmon/pmbus/Makefile | 1 +
drivers/hwmon/pmbus/inspur-ipsps.c| 226 ++
4 files changed, 315 insertions(+)
create mode 100644 Documentation/hwmon/inspur-ipsps1.rst
create mode 100644 drivers/hwmon/pmbus/inspur-ipsps.c
diff --git a/Documentation/hwmon/inspur-ipsps1.rst
b/Documentation/hwmon/inspur-ipsps1.rst
new file mode 100644
index ..2b871ae3448f
--- /dev/null
+++ b/Documentation/hwmon/inspur-ipsps1.rst
@@ -0,0 +1,79 @@
+Kernel driver inspur-ipsps1
+===
+
+Supported chips:
+
+ * Inspur Power System power supply unit
+
+Author: John Wang
+
+Description
+---
+
+This driver supports Inspur Power System power supplies. This driver
+is a client to the core PMBus driver.
+
+Usage Notes
+---
+
+This driver does not auto-detect devices. You will have to instantiate the
+devices explicitly. Please see Documentation/i2c/instantiating-devices for
+details.
+
+Sysfs entries
+-
+
+The following attributes are supported:
+
+===
==
+curr1_inputMeasured input current
+curr1_label"iin"
+curr1_max Maximum current
+curr1_max_alarmCurrent high alarm
+curr2_inputMeasured output current in mA.
+curr2_label"iout1"
+curr2_crit Critical maximum current
+curr2_crit_alarm Current critical high alarm
+curr2_max Maximum current
+curr2_max_alarmCurrent high alarm
+
+fan1_alarm Fan 1 warning.
+fan1_fault Fan 1 fault.
+fan1_input Fan 1 speed in RPM.
+
+in1_alarm Input voltage under-voltage alarm.
+in1_input Measured input voltage in mV.
+in1_label "vin"
+in2_input Measured output voltage in mV.
+in2_label "vout1"
+in2_lcrit Critical minimum output voltage
+in2_lcrit_alarmOutput voltage critical low alarm
+in2_maxMaximum output voltage
+in2_max_alarm Output voltage high alarm
+in2_minMinimum output voltage
+in2_min_alarm Output voltage low alarm
+
+power1_alarm Input fault or alarm.
+power1_input Measured input power in uW.
+power1_label "pin"
+power1_max Input power limit
+power2_max_alarm Output power high alarm
+power2_max Output power limit
+power2_input Measured output power in uW.
+power2_label "pout"
+
+temp[1-3]_inputMeasured temperature
+temp[1-2]_max Maximum temperature
+temp[1-3]_max_alarmTemperature high alarm
+
+vendor Manufacturer name
+model Product model
+part_numberProduct part number
+serial_number Product serial number
+fw_version Firmware version
+hw_version Hardware version
+mode Work mode. Can be set to active or
+ standby, when set to standby, PSU will
+ automatically switch between standby
+ and redundancy mode.
I don't think it is aligned yet. Please use space only instead of tabs.
+===
Re: [Potential Spoof] Re: [PATCH net-next] net/ncsi: allow to customize BMC MAC Address offset
On 8/8/19, 3:27 PM, "openbmc on behalf of Tao Ren"
wrote:
On 8/8/19 2:16 PM, Andrew Lunn wrote:
> On Thu, Aug 08, 2019 at 07:02:54PM +, Tao Ren wrote:
>> Hi Andrew,
>>
>> On 8/8/19 6:32 AM, Andrew Lunn wrote:
Let me prepare patch v2 using device tree. I'm not sure if standard
"mac-address" fits this situation because all we need is an offset
(integer) and BMC MAC is calculated by adding the offset to NIC's
MAC address. Anyways, let me work out v2 patch we can discuss more
then.
>>>
>>> Hi Tao
>>>
>>> I don't know BMC terminology. By NICs MAC address, you are referring
>>> to the hosts MAC address? The MAC address the big CPU is using for its
>>> interface? Where does this NIC get its MAC address from? If the BMCs
>>> bootloader has access to it, it can set the mac-address property in
>>> the device tree.
>>
>> Sorry for the confusion and let me clarify more:
>>
>
>> The NIC here refers to the Network controller which provide network
>> connectivity for both BMC (via NC-SI) and Host (for example, via
>> PCIe).
>>
>
>> On Facebook Yamp BMC, BMC sends NCSI_OEM_GET_MAC command (as an
>> ethernet packet) to the Network Controller while bringing up eth0,
>> and the (Broadcom) Network Controller replies with the Base MAC
>> Address reserved for the platform. As for Yamp, Base-MAC and
>> Base-MAC+1 are used by Host (big CPU) and Base-MAC+2 are assigned to
>> BMC. In my opinion, Base MAC and MAC address assignments are
>> controlled by Network Controller, which is transparent to both BMC
>> and Host.
>
> Hi Tao
>
> I've not done any work in the BMC field, so thanks for explaining
> this.
>
> In a typical embedded system, each network interface is assigned a MAC
> address by the vendor. But here, things are different. The BMC SoC
> network interface has not been assigned a MAC address, it needs to ask
> the network controller for its MAC address, and then do some magical
> transformation on the answer to derive a MAC address for
> itself. Correct?
Yes. It's correct.
> It seems like a better design would of been, the BMC sends a
> NCSI_OEM_GET_BMC_MAC and the answer it gets back is the MAC address
> the BMC should use. No magic involved. But i guess it is too late to
> do that now.
Some NCSI Network Controllers support such OEM command (Get Provisioned BMC
MAC Address), but unfortunately it's not supported on Yamp.
>> I'm not sure if I understand your suggestion correctly: do you mean
>> we should move the logic (GET_MAC from Network Controller, adding
>> offset and configuring BMC MAC) from kernel to boot loader?
>
> In general, the kernel is generic. It probably boots on any ARM system
> which is has the needed modules for. The bootloader is often much more
> specific. It might not be fully platform specific, but it will be at
> least specific to the general family of BMC SoCs. If you consider the
> combination of the BMC bootloader and the device tree blob, you have
> something specific to the platform. This magical transformation of
> adding 2 seems to be very platform specific. So having this magic in
> the bootloader+DT seems like the best place to put it.
I understand your concern now. Thank you for the explanation.
> However, how you pass the resulting MAC address to the kernel should
> be as generic as possible. The DT "mac-address" property is very
> generic, many MAC drivers understand it. Using it also allows for
> vendors which actually assign a MAC address to the BMC to pass it to
> the BMC, avoiding all this NCSI_OEM_GET_MAC handshake. Having an API
> which just passing '2' is not generic at all.
After giving it more thought, I'm thinking about adding ncsi dt node with
following structure (mac/ncsi similar to mac/mdio/phy):
{
/* MAC properties... */
use-ncsi;
ncsi {
/* ncsi level properties if any */
Tao, I like this idea but keep this only hardware specific.
package@0 {
/* package level properties if any */
channel@0 {
/* channel level properties if any */
bmc-mac-offset = <2>;
Every NIC vendor doesn't need this offset so it is specific to BCM card only as
you see this
increment has been done only for BCM card so you may want to pass bcm specific
only.
};
channel@1 {
/* channel #1 properties */
};
};
/* package #1 properties start here.. */
};
};
The reasons behind this are:
1) mac driver doesn't need to parse "mac-offset" stuff: these
Re:[PATCH net-next] net/ncsi: allow to customize BMC MAC Address offset
Lgtm except one small comment below.
On 8/6/19, 5:22 PM, "openbmc on behalf of Tao Ren"
wrote:
Currently BMC's MAC address is calculated by adding 1 to NCSI NIC's base
MAC address when CONFIG_NCSI_OEM_CMD_GET_MAC option is enabled. The logic
doesn't work for platforms with different BMC MAC offset: for example,
Facebook Yamp BMC's MAC address is calculated by adding 2 to NIC's base
MAC address ("BaseMAC + 1" is reserved for Host use).
This patch adds NET_NCSI_MC_MAC_OFFSET config option to customize offset
between NIC's Base MAC address and BMC's MAC address. Its default value is
set to 1 to avoid breaking existing users.
Signed-off-by: Tao Ren
---
net/ncsi/Kconfig| 8
net/ncsi/ncsi-rsp.c | 15 +--
2 files changed, 21 insertions(+), 2 deletions(-)
diff --git a/net/ncsi/Kconfig b/net/ncsi/Kconfig
index 2f1e5756c03a..be8efe1ed99e 100644
--- a/net/ncsi/Kconfig
+++ b/net/ncsi/Kconfig
@@ -17,3 +17,11 @@ config NCSI_OEM_CMD_GET_MAC
---help---
This allows to get MAC address from NCSI firmware and set them back to
controller.
+config NET_NCSI_MC_MAC_OFFSET
+ int
+ prompt "Offset of Management Controller's MAC Address"
+ depends on NCSI_OEM_CMD_GET_MAC
+ default 1
+ help
+ This defines the offset between Network Controller's (base) MAC
+ address and Management Controller's MAC address.
diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
index 7581bf919885..24a791f9ebf5 100644
--- a/net/ncsi/ncsi-rsp.c
+++ b/net/ncsi/ncsi-rsp.c
@@ -656,6 +656,11 @@ static int ncsi_rsp_handler_oem_bcm_gma(struct
ncsi_request *nr)
struct ncsi_rsp_oem_pkt *rsp;
struct sockaddr saddr;
int ret = 0;
+#ifdef CONFIG_NET_NCSI_MC_MAC_OFFSET
+ int mac_offset = CONFIG_NET_NCSI_MC_MAC_OFFSET;
+#else
+ int mac_offset = 1;
+#endif
/* Get the response header */
rsp = (struct ncsi_rsp_oem_pkt *)skb_network_header(nr->rsp);
@@ -663,8 +668,14 @@ static int ncsi_rsp_handler_oem_bcm_gma(struct
ncsi_request *nr)
saddr.sa_family = ndev->type;
ndev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
memcpy(saddr.sa_data, >data[BCM_MAC_ADDR_OFFSET], ETH_ALEN);
- /* Increase mac address by 1 for BMC's address */
- eth_addr_inc((u8 *)saddr.sa_data);
+
+ /* Management Controller's MAC address is calculated by adding
+* the offset to Network Controller's (base) MAC address.
+* Note: negative offset is "ignored", and BMC will use the Base
Just mention negative and zero offset is ignored. As you are ignoring 0 as well.
+* MAC address in this case.
+*/
+ while (mac_offset-- > 0)
+ eth_addr_inc((u8 *)saddr.sa_data);
if (!is_valid_ether_addr((const u8 *)saddr.sa_data))
return -ENXIO;
--
2.17.1
Re: [PATCH] dt-bindings: Add pxe1610 as a trivial device
Joel, I have added all 3 id in the documentation patch and I am not sure if that patch has been applied or not. Regards -Vijay On 8/1/19, 11:31 PM, "Joel Stanley" wrote: Add pxe1610 as a trivial device On Tue, 23 Jul 2019 at 17:14, Vijay Khemka wrote: > > On 7/23/19, 7:53 AM, "Rob Herring" wrote: > > On Tue, Jul 23, 2019 at 8:50 AM Rob Herring wrote: > > > > On Mon, Jul 22, 2019 at 6:46 PM Vijay Khemka wrote: > > > > > > The pxe1610 is a voltage regulator from Infineon. It also supports > > > other VRs pxe1110 and pxm1310 from Infineon. > > What happened to the other compatibles? S/w doesn't need to know the > differences? > As far as driver is concerned, it doesn't need to know differences. You have these three IDs in the driver: pxm1310 pxm1310 pxe1610 So all three could be listed in the documentation? Rob, is this what you wanted Vijay to do?
Re: [PATCH] dt-bindings: Add pxe1610 as a trivial device
On 7/23/19, 7:53 AM, "Rob Herring" wrote:
On Tue, Jul 23, 2019 at 8:50 AM Rob Herring wrote:
>
> On Mon, Jul 22, 2019 at 6:46 PM Vijay Khemka wrote:
> >
> > The pxe1610 is a voltage regulator from Infineon. It also supports
> > other VRs pxe1110 and pxm1310 from Infineon.
What happened to the other compatibles? S/w doesn't need to know the
differences?
As far as driver is concerned, it doesn't need to know differences.
> >
> > Signed-off-by: Vijay Khemka
> > ---
> > Documentation/devicetree/bindings/trivial-devices.yaml | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml
b/Documentation/devicetree/bindings/trivial-devices.yaml
> > index 2e742d399e87..1be648828a31 100644
> > --- a/Documentation/devicetree/bindings/trivial-devices.yaml
> > +++ b/Documentation/devicetree/bindings/trivial-devices.yaml
> > @@ -99,6 +99,8 @@ properties:
> > # Infineon IR38064 Voltage Regulator
> >- infineon,ir38064
> > # Infineon SLB9635 (Soft-) I2C TPM (old protocol, max
100khz)
> > + - infineon,pxe1610
> > +# Infineon PXE1610, PXE1110 and PXM1310 Voltage Regulators
>
> The comment goes above the entry.
>
> >- infineon,slb9635tt
> > # Infineon SLB9645 I2C TPM (new protocol, max 400khz)
> >- infineon,slb9645tt
> > --
> > 2.17.1
> >
Re: [PATCH] soc: aspeed: lpc-ctrl: Fix probe error handling
On 6/20/19, 2:17 AM, "joel.s...@gmail.com on behalf of Joel Stanley" wrote: gcc warns that a mising "flash" phandle node leads to undefined behavior later: drivers/soc/aspeed/aspeed-lpc-ctrl.c: In function 'aspeed_lpc_ctrl_probe': drivers/soc/aspeed/aspeed-lpc-ctrl.c:201:18: error: '*((void *)+8)' may be used uninitialized in this function [-Werror=maybe-uninitialized] Only set the flash base and size if we find a phandle in the device tree. Thanks for fixing this. Reviewed-by: Vijay Khemka Reported-by: Arnd Bergmann Signed-off-by: Joel Stanley --- drivers/soc/aspeed/aspeed-lpc-ctrl.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/soc/aspeed/aspeed-lpc-ctrl.c b/drivers/soc/aspeed/aspeed-lpc-ctrl.c index aca13779764a..eee26c2d8b52 100644 --- a/drivers/soc/aspeed/aspeed-lpc-ctrl.c +++ b/drivers/soc/aspeed/aspeed-lpc-ctrl.c @@ -223,10 +223,11 @@ static int aspeed_lpc_ctrl_probe(struct platform_device *pdev) dev_err(dev, "Couldn't address to resource for flash\n"); return rc; } + + lpc_ctrl->pnor_size = resource_size(); + lpc_ctrl->pnor_base = resm.start; } - lpc_ctrl->pnor_size = resource_size(); - lpc_ctrl->pnor_base = resm.start; dev_set_drvdata(>dev, lpc_ctrl); -- 2.20.1
Re: [PATCH] soc: aspeed: fix probe error handling
On 6/20/19, 1:29 AM, "Joel Stanley" wrote:
On Wed, 19 Jun 2019 at 12:56, Arnd Bergmann wrote:
>
> gcc warns that a mising "flash" phandle node leads to undefined
> behavior later:
>
> drivers/soc/aspeed/aspeed-lpc-ctrl.c: In function 'aspeed_lpc_ctrl_probe':
> drivers/soc/aspeed/aspeed-lpc-ctrl.c:201:18: error: '*((void *)+8)'
may be used uninitialized in this function [-Werror=maybe-uninitialized]
>
> The device cannot work without this node, so just error out here.
>
> Signed-off-by: Arnd Bergmann
Thanks Arnd. This looks like it applies on top of Vijay's recent patch?
The intent of that change was to make the driver usable for systems
that do not want to depend on the flash phandle. I think the fix we
want looks like this:
--- a/drivers/soc/aspeed/aspeed-lpc-ctrl.c
+++ b/drivers/soc/aspeed/aspeed-lpc-ctrl.c
@@ -224,10 +224,11 @@ static int aspeed_lpc_ctrl_probe(struct
platform_device *pdev)
dev_err(dev, "Couldn't address to resource for
flash\n");
return rc;
}
+
+ lpc_ctrl->pnor_size = resource_size();
+ lpc_ctrl->pnor_base = resm.start;
}
- lpc_ctrl->pnor_size = resource_size();
- lpc_ctrl->pnor_base = resm.start;
Vijay, do you agree?
Yes Joel. Thanks for this.
Cheers,
Joel
> ---
> drivers/soc/aspeed/aspeed-lpc-ctrl.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/soc/aspeed/aspeed-lpc-ctrl.c
b/drivers/soc/aspeed/aspeed-lpc-ctrl.c
> index 239520bb207e..81109d22af6a 100644
> --- a/drivers/soc/aspeed/aspeed-lpc-ctrl.c
> +++ b/drivers/soc/aspeed/aspeed-lpc-ctrl.c
> @@ -212,6 +212,7 @@ static int aspeed_lpc_ctrl_probe(struct
platform_device *pdev)
> node = of_parse_phandle(dev->of_node, "flash", 0);
> if (!node) {
> dev_dbg(dev, "Didn't find host pnor flash node\n");
> + return -ENXIO;
> } else {
> rc = of_address_to_resource(node, 1, );
> of_node_put(node);
> --
> 2.20.0
>
Re: [PATCH v2] soc: aspeed: lpc-ctrl: make parameter optional
Hi Andrew,
Any update on this patch or do I need any more rework.
Regards
-Vijay
On 6/6/19, 11:51 AM, "Vijay Khemka" wrote:
Please update on this patch.
On 5/30/19, 1:37 PM, "Vijay Khemka" wrote:
Making memory-region and flash as optional parameter in device
tree if user needs to use these parameter through ioctl then
need to define in devicetree.
Signed-off-by: Vijay Khemka
Reviewed-by: Andrew Jeffery
---
drivers/soc/aspeed/aspeed-lpc-ctrl.c | 58 +---
1 file changed, 36 insertions(+), 22 deletions(-)
diff --git a/drivers/soc/aspeed/aspeed-lpc-ctrl.c
b/drivers/soc/aspeed/aspeed-lpc-ctrl.c
index a024f8042259..aca13779764a 100644
--- a/drivers/soc/aspeed/aspeed-lpc-ctrl.c
+++ b/drivers/soc/aspeed/aspeed-lpc-ctrl.c
@@ -68,6 +68,7 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file,
unsigned int cmd,
unsigned long param)
{
struct aspeed_lpc_ctrl *lpc_ctrl = file_aspeed_lpc_ctrl(file);
+ struct device *dev = file->private_data;
void __user *p = (void __user *)param;
struct aspeed_lpc_ctrl_mapping map;
u32 addr;
@@ -90,6 +91,12 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file,
unsigned int cmd,
if (map.window_id != 0)
return -EINVAL;
+ /* If memory-region is not described in device tree */
+ if (!lpc_ctrl->mem_size) {
+ dev_dbg(dev, "Didn't find reserved memory\n");
+ return -ENXIO;
+ }
+
map.size = lpc_ctrl->mem_size;
return copy_to_user(p, , sizeof(map)) ? -EFAULT : 0;
@@ -126,9 +133,18 @@ static long aspeed_lpc_ctrl_ioctl(struct file
*file, unsigned int cmd,
return -EINVAL;
if (map.window_type == ASPEED_LPC_CTRL_WINDOW_FLASH) {
+ if (!lpc_ctrl->pnor_size) {
+ dev_dbg(dev, "Didn't find host pnor
flash\n");
+ return -ENXIO;
+ }
addr = lpc_ctrl->pnor_base;
size = lpc_ctrl->pnor_size;
} else if (map.window_type ==
ASPEED_LPC_CTRL_WINDOW_MEMORY) {
+ /* If memory-region is not described in device
tree */
+ if (!lpc_ctrl->mem_size) {
+ dev_dbg(dev, "Didn't find reserved
memory\n");
+ return -ENXIO;
+ }
addr = lpc_ctrl->mem_base;
size = lpc_ctrl->mem_size;
} else {
@@ -196,17 +212,17 @@ static int aspeed_lpc_ctrl_probe(struct
platform_device *pdev)
if (!lpc_ctrl)
return -ENOMEM;
+ /* If flash is described in device tree then store */
node = of_parse_phandle(dev->of_node, "flash", 0);
if (!node) {
- dev_err(dev, "Didn't find host pnor flash node\n");
- return -ENODEV;
- }
-
- rc = of_address_to_resource(node, 1, );
- of_node_put(node);
- if (rc) {
- dev_err(dev, "Couldn't address to resource for
flash\n");
- return rc;
+ dev_dbg(dev, "Didn't find host pnor flash node\n");
+ } else {
+ rc = of_address_to_resource(node, 1, );
+ of_node_put(node);
+ if (rc) {
+ dev_err(dev, "Couldn't address to resource for
flash\n");
+ return rc;
+ }
}
lpc_ctrl->pnor_size = resource_size();
@@ -214,22 +230,22 @@ static int aspeed_lpc_ctrl_probe(struct
platform_device *pdev)
dev_set_drvdata(>dev, lpc_ctrl);
+ /* If memory-region is described in device tree then store */
node = of_parse_phandle(dev->of_node, "memory-region", 0);
if (!node) {
- dev_err(dev, "Didn't find reserved memory\n");
- return -EINVAL;
- }
+ dev_dbg(dev, &quo
Re: [PATCH v2] soc: aspeed: lpc-ctrl: make parameter optional
Please update on this patch.
On 5/30/19, 1:37 PM, "Vijay Khemka" wrote:
Making memory-region and flash as optional parameter in device
tree if user needs to use these parameter through ioctl then
need to define in devicetree.
Signed-off-by: Vijay Khemka
Reviewed-by: Andrew Jeffery
---
drivers/soc/aspeed/aspeed-lpc-ctrl.c | 58 +---
1 file changed, 36 insertions(+), 22 deletions(-)
diff --git a/drivers/soc/aspeed/aspeed-lpc-ctrl.c
b/drivers/soc/aspeed/aspeed-lpc-ctrl.c
index a024f8042259..aca13779764a 100644
--- a/drivers/soc/aspeed/aspeed-lpc-ctrl.c
+++ b/drivers/soc/aspeed/aspeed-lpc-ctrl.c
@@ -68,6 +68,7 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file,
unsigned int cmd,
unsigned long param)
{
struct aspeed_lpc_ctrl *lpc_ctrl = file_aspeed_lpc_ctrl(file);
+ struct device *dev = file->private_data;
void __user *p = (void __user *)param;
struct aspeed_lpc_ctrl_mapping map;
u32 addr;
@@ -90,6 +91,12 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file,
unsigned int cmd,
if (map.window_id != 0)
return -EINVAL;
+ /* If memory-region is not described in device tree */
+ if (!lpc_ctrl->mem_size) {
+ dev_dbg(dev, "Didn't find reserved memory\n");
+ return -ENXIO;
+ }
+
map.size = lpc_ctrl->mem_size;
return copy_to_user(p, , sizeof(map)) ? -EFAULT : 0;
@@ -126,9 +133,18 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file,
unsigned int cmd,
return -EINVAL;
if (map.window_type == ASPEED_LPC_CTRL_WINDOW_FLASH) {
+ if (!lpc_ctrl->pnor_size) {
+ dev_dbg(dev, "Didn't find host pnor flash\n");
+ return -ENXIO;
+ }
addr = lpc_ctrl->pnor_base;
size = lpc_ctrl->pnor_size;
} else if (map.window_type == ASPEED_LPC_CTRL_WINDOW_MEMORY) {
+ /* If memory-region is not described in device tree */
+ if (!lpc_ctrl->mem_size) {
+ dev_dbg(dev, "Didn't find reserved memory\n");
+ return -ENXIO;
+ }
addr = lpc_ctrl->mem_base;
size = lpc_ctrl->mem_size;
} else {
@@ -196,17 +212,17 @@ static int aspeed_lpc_ctrl_probe(struct
platform_device *pdev)
if (!lpc_ctrl)
return -ENOMEM;
+ /* If flash is described in device tree then store */
node = of_parse_phandle(dev->of_node, "flash", 0);
if (!node) {
- dev_err(dev, "Didn't find host pnor flash node\n");
- return -ENODEV;
- }
-
- rc = of_address_to_resource(node, 1, );
- of_node_put(node);
- if (rc) {
- dev_err(dev, "Couldn't address to resource for flash\n");
- return rc;
+ dev_dbg(dev, "Didn't find host pnor flash node\n");
+ } else {
+ rc = of_address_to_resource(node, 1, );
+ of_node_put(node);
+ if (rc) {
+ dev_err(dev, "Couldn't address to resource for
flash\n");
+ return rc;
+ }
}
lpc_ctrl->pnor_size = resource_size();
@@ -214,22 +230,22 @@ static int aspeed_lpc_ctrl_probe(struct
platform_device *pdev)
dev_set_drvdata(>dev, lpc_ctrl);
+ /* If memory-region is described in device tree then store */
node = of_parse_phandle(dev->of_node, "memory-region", 0);
if (!node) {
- dev_err(dev, "Didn't find reserved memory\n");
- return -EINVAL;
- }
+ dev_dbg(dev, "Didn't find reserved memory\n");
+ } else {
+ rc = of_address_to_resource(node, 0, );
+ of_node_put(node);
+ if (rc) {
+ dev_err(dev, "Couldn't address to resource for reserved
memory\n");
+ return -ENXIO;
+ }
- rc = of_address_to_resource(node, 0, );
- of_node_put(node);
- if (rc) {
- dev_err(dev, "Couldn't address to resource for reserved
memory\n");
- return -ENOMEM;
+ lpc_ctrl->mem_size = resource_size();
+ lpc_ctrl->mem_base = resm.start;
}
- lpc_ctrl->mem_size = resource_size();
- lpc_ctrl->mem_base = res
Re: [PATCH v2 1/2] hwmon: pmbus: Add Infineon PXE1610 VR driver
On 6/5/19, 1:47 PM, "Guenter Roeck" wrote:
On Thu, May 30, 2019 at 04:11:56PM -0700, Vijay Khemka wrote:
> Added pmbus driver for the new device Infineon pxe1610
> voltage regulator. It also supports similar family device
> PXE1110 and PXM1310.
>
> Signed-off-by: Vijay Khemka
Applied.
Thanks
Thanks,
Guenter
> ---
> Changes in v2:
> incorporated all the feedback from Guenter Roeck
>
> drivers/hwmon/pmbus/Kconfig | 9 +++
> drivers/hwmon/pmbus/Makefile | 1 +
> drivers/hwmon/pmbus/pxe1610.c | 139 ++
> 3 files changed, 149 insertions(+)
> create mode 100644 drivers/hwmon/pmbus/pxe1610.c
>
> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> index 30751eb9550a..338ef9b5a395 100644
> --- a/drivers/hwmon/pmbus/Kconfig
> +++ b/drivers/hwmon/pmbus/Kconfig
> @@ -154,6 +154,15 @@ config SENSORS_MAX8688
> This driver can also be built as a module. If so, the module will
> be called max8688.
>
> +config SENSORS_PXE1610
> + tristate "Infineon PXE1610"
> + help
> + If you say yes here you get hardware monitoring support for Infineon
> + PXE1610.
> +
> + This driver can also be built as a module. If so, the module will
> + be called pxe1610.
> +
> config SENSORS_TPS40422
> tristate "TI TPS40422"
> help
> diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
> index 2219b9300316..b0fbd017a91a 100644
> --- a/drivers/hwmon/pmbus/Makefile
> +++ b/drivers/hwmon/pmbus/Makefile
> @@ -18,6 +18,7 @@ obj-$(CONFIG_SENSORS_MAX20751) += max20751.o
> obj-$(CONFIG_SENSORS_MAX31785) += max31785.o
> obj-$(CONFIG_SENSORS_MAX34440) += max34440.o
> obj-$(CONFIG_SENSORS_MAX8688)+= max8688.o
> +obj-$(CONFIG_SENSORS_PXE1610)+= pxe1610.o
> obj-$(CONFIG_SENSORS_TPS40422) += tps40422.o
> obj-$(CONFIG_SENSORS_TPS53679) += tps53679.o
> obj-$(CONFIG_SENSORS_UCD9000)+= ucd9000.o
> diff --git a/drivers/hwmon/pmbus/pxe1610.c b/drivers/hwmon/pmbus/pxe1610.c
> new file mode 100644
> index ..ebe3f023f840
> --- /dev/null
> +++ b/drivers/hwmon/pmbus/pxe1610.c
> @@ -0,0 +1,139 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Hardware monitoring driver for Infineon PXE1610
> + *
> + * Copyright (c) 2019 Facebook Inc
> + *
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include "pmbus.h"
> +
> +#define PXE1610_NUM_PAGES 3
> +
> +/* Identify chip parameters. */
> +static int pxe1610_identify(struct i2c_client *client,
> + struct pmbus_driver_info *info)
> +{
> + if (pmbus_check_byte_register(client, 0, PMBUS_VOUT_MODE)) {
> + u8 vout_mode;
> + int ret;
> +
> + /* Read the register with VOUT scaling value.*/
> + ret = pmbus_read_byte_data(client, 0, PMBUS_VOUT_MODE);
> + if (ret < 0)
> + return ret;
> +
> + vout_mode = ret & GENMASK(4, 0);
> +
> + switch (vout_mode) {
> + case 1:
> + info->vrm_version = vr12;
> + break;
> + case 2:
> + info->vrm_version = vr13;
> + break;
> + default:
> + return -ENODEV;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static struct pmbus_driver_info pxe1610_info = {
> + .pages = PXE1610_NUM_PAGES,
> + .format[PSC_VOLTAGE_IN] = linear,
> + .format[PSC_VOLTAGE_OUT] = vid,
> + .format[PSC_CURRENT_IN] = linear,
> + .format[PSC_CURRENT_OUT] = linear,
> + .format[PSC_TEMPERATURE] = linear,
> + .format[PSC_POWER] = linear,
> + .func[0] = PMBUS_HAVE_VIN
> + | PMBUS_HAVE_VOUT | PMBUS_HAVE_IIN
> + | PMBUS_HAVE_IOUT | PMBUS_HAVE_PIN
> + | PMBUS_HAVE_POUT | PMBUS_HAVE_TEMP
> + | PMBUS_HAVE_STATUS_VOUT | PMBUS_HAVE_STATUS_IOUT
> + | PMBUS_HAVE_STATUS_INPUT | PMBUS_HAVE_STATUS_TEMP,
> + .func[1] = PMBUS_HAVE_VIN
> + | PMBUS_HAVE_VOUT | PMBUS_HAVE_IIN
> + | PMBUS_HAVE_IOUT | PMBUS_HAVE_PIN
> + | PMBUS_HAVE_POUT | PMBUS_HAVE_TEMP
> + | PMBUS_HAVE_STATUS_VOUT | PMBUS_HAVE_STATUS_IO
Re: [PATCH] soc: aspeed: lpc-ctrl: make parameter optional
On 5/29/19, 9:02 PM, "Andrew Jeffery" wrote:
On Thu, 30 May 2019, at 02:51, Vijay Khemka wrote:
> Makiing
Typo here, but I'd prefer to see this patch go in, so
Corrected in next version v2.
> memory-region and flash as optional parameter in device
> tree if user needs to use these parameter through ioctl then
> need to define in devicetree.
>
> Signed-off-by: Vijay Khemka
Reviewed-by: Andrew Jeffery
> ---
> drivers/soc/aspeed/aspeed-lpc-ctrl.c | 58 +---
> 1 file changed, 36 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/soc/aspeed/aspeed-lpc-ctrl.c
> b/drivers/soc/aspeed/aspeed-lpc-ctrl.c
> index a024f8042259..aca13779764a 100644
> --- a/drivers/soc/aspeed/aspeed-lpc-ctrl.c
> +++ b/drivers/soc/aspeed/aspeed-lpc-ctrl.c
> @@ -68,6 +68,7 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file,
> unsigned int cmd,
> unsigned long param)
> {
> struct aspeed_lpc_ctrl *lpc_ctrl = file_aspeed_lpc_ctrl(file);
> + struct device *dev = file->private_data;
> void __user *p = (void __user *)param;
> struct aspeed_lpc_ctrl_mapping map;
> u32 addr;
> @@ -90,6 +91,12 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file,
> unsigned int cmd,
> if (map.window_id != 0)
> return -EINVAL;
>
> + /* If memory-region is not described in device tree */
> + if (!lpc_ctrl->mem_size) {
> + dev_dbg(dev, "Didn't find reserved memory\n");
> + return -ENXIO;
> + }
> +
> map.size = lpc_ctrl->mem_size;
>
> return copy_to_user(p, , sizeof(map)) ? -EFAULT : 0;
> @@ -126,9 +133,18 @@ static long aspeed_lpc_ctrl_ioctl(struct file
> *file, unsigned int cmd,
> return -EINVAL;
>
> if (map.window_type == ASPEED_LPC_CTRL_WINDOW_FLASH) {
> + if (!lpc_ctrl->pnor_size) {
> + dev_dbg(dev, "Didn't find host pnor flash\n");
> + return -ENXIO;
> + }
> addr = lpc_ctrl->pnor_base;
> size = lpc_ctrl->pnor_size;
> } else if (map.window_type == ASPEED_LPC_CTRL_WINDOW_MEMORY) {
> + /* If memory-region is not described in device tree */
> + if (!lpc_ctrl->mem_size) {
> + dev_dbg(dev, "Didn't find reserved memory\n");
> + return -ENXIO;
> + }
> addr = lpc_ctrl->mem_base;
> size = lpc_ctrl->mem_size;
> } else {
> @@ -196,17 +212,17 @@ static int aspeed_lpc_ctrl_probe(struct
> platform_device *pdev)
> if (!lpc_ctrl)
> return -ENOMEM;
>
> + /* If flash is described in device tree then store */
> node = of_parse_phandle(dev->of_node, "flash", 0);
> if (!node) {
> - dev_err(dev, "Didn't find host pnor flash node\n");
> - return -ENODEV;
> - }
> -
> - rc = of_address_to_resource(node, 1, );
> - of_node_put(node);
> - if (rc) {
> - dev_err(dev, "Couldn't address to resource for flash\n");
> - return rc;
> + dev_dbg(dev, "Didn't find host pnor flash node\n");
> + } else {
> + rc = of_address_to_resource(node, 1, );
> + of_node_put(node);
> + if (rc) {
> + dev_err(dev, "Couldn't address to resource for
flash\n");
> + return rc;
> + }
> }
>
> lpc_ctrl->pnor_size = resource_size();
> @@ -214,22 +230,22 @@ static int aspeed_lpc_ctrl_probe(struct
> platform_device *pdev)
>
> dev_set_drvdata(>dev, lpc_ctrl);
>
> + /* If memory-region is described in device tree then store */
> node = of_parse_phandle(dev->of_node, "memory-region", 0);
> if (!node) {
> - dev_err(dev, "Didn't find reserved memory\n");
> - return -EINVAL;
> - }
> + dev_dbg(dev, "Didn't find reserved memory\n");
> + } else {
> + rc = of_address_to_resource(node, 0, );
> + of_node_put(node);
> + if (rc) {
> + dev_err(dev, "Coul
[PATCH v2] soc: aspeed: lpc-ctrl: make parameter optional
Making memory-region and flash as optional parameter in device
tree if user needs to use these parameter through ioctl then
need to define in devicetree.
Signed-off-by: Vijay Khemka
Reviewed-by: Andrew Jeffery
---
drivers/soc/aspeed/aspeed-lpc-ctrl.c | 58 +---
1 file changed, 36 insertions(+), 22 deletions(-)
diff --git a/drivers/soc/aspeed/aspeed-lpc-ctrl.c
b/drivers/soc/aspeed/aspeed-lpc-ctrl.c
index a024f8042259..aca13779764a 100644
--- a/drivers/soc/aspeed/aspeed-lpc-ctrl.c
+++ b/drivers/soc/aspeed/aspeed-lpc-ctrl.c
@@ -68,6 +68,7 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file, unsigned
int cmd,
unsigned long param)
{
struct aspeed_lpc_ctrl *lpc_ctrl = file_aspeed_lpc_ctrl(file);
+ struct device *dev = file->private_data;
void __user *p = (void __user *)param;
struct aspeed_lpc_ctrl_mapping map;
u32 addr;
@@ -90,6 +91,12 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file,
unsigned int cmd,
if (map.window_id != 0)
return -EINVAL;
+ /* If memory-region is not described in device tree */
+ if (!lpc_ctrl->mem_size) {
+ dev_dbg(dev, "Didn't find reserved memory\n");
+ return -ENXIO;
+ }
+
map.size = lpc_ctrl->mem_size;
return copy_to_user(p, , sizeof(map)) ? -EFAULT : 0;
@@ -126,9 +133,18 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file,
unsigned int cmd,
return -EINVAL;
if (map.window_type == ASPEED_LPC_CTRL_WINDOW_FLASH) {
+ if (!lpc_ctrl->pnor_size) {
+ dev_dbg(dev, "Didn't find host pnor flash\n");
+ return -ENXIO;
+ }
addr = lpc_ctrl->pnor_base;
size = lpc_ctrl->pnor_size;
} else if (map.window_type == ASPEED_LPC_CTRL_WINDOW_MEMORY) {
+ /* If memory-region is not described in device tree */
+ if (!lpc_ctrl->mem_size) {
+ dev_dbg(dev, "Didn't find reserved memory\n");
+ return -ENXIO;
+ }
addr = lpc_ctrl->mem_base;
size = lpc_ctrl->mem_size;
} else {
@@ -196,17 +212,17 @@ static int aspeed_lpc_ctrl_probe(struct platform_device
*pdev)
if (!lpc_ctrl)
return -ENOMEM;
+ /* If flash is described in device tree then store */
node = of_parse_phandle(dev->of_node, "flash", 0);
if (!node) {
- dev_err(dev, "Didn't find host pnor flash node\n");
- return -ENODEV;
- }
-
- rc = of_address_to_resource(node, 1, );
- of_node_put(node);
- if (rc) {
- dev_err(dev, "Couldn't address to resource for flash\n");
- return rc;
+ dev_dbg(dev, "Didn't find host pnor flash node\n");
+ } else {
+ rc = of_address_to_resource(node, 1, );
+ of_node_put(node);
+ if (rc) {
+ dev_err(dev, "Couldn't address to resource for
flash\n");
+ return rc;
+ }
}
lpc_ctrl->pnor_size = resource_size();
@@ -214,22 +230,22 @@ static int aspeed_lpc_ctrl_probe(struct platform_device
*pdev)
dev_set_drvdata(>dev, lpc_ctrl);
+ /* If memory-region is described in device tree then store */
node = of_parse_phandle(dev->of_node, "memory-region", 0);
if (!node) {
- dev_err(dev, "Didn't find reserved memory\n");
- return -EINVAL;
- }
+ dev_dbg(dev, "Didn't find reserved memory\n");
+ } else {
+ rc = of_address_to_resource(node, 0, );
+ of_node_put(node);
+ if (rc) {
+ dev_err(dev, "Couldn't address to resource for reserved
memory\n");
+ return -ENXIO;
+ }
- rc = of_address_to_resource(node, 0, );
- of_node_put(node);
- if (rc) {
- dev_err(dev, "Couldn't address to resource for reserved
memory\n");
- return -ENOMEM;
+ lpc_ctrl->mem_size = resource_size();
+ lpc_ctrl->mem_base = resm.start;
}
- lpc_ctrl->mem_size = resource_size();
- lpc_ctrl->mem_base = resm.start;
-
lpc_ctrl->regmap = syscon_node_to_regmap(
pdev->dev.parent->of_node);
if (IS_ERR(lpc_ctrl->regmap)) {
@@ -258,8 +274,6 @@ static int aspeed_lpc_ctrl_probe(struct platform_device
*pdev)
goto err;
}
- dev_info(dev, "Loaded at %pr\n", );
-
return 0;
err:
--
2.17.1
Re: [PATCH 1/2] hwmon: pmbus: Add Infineon PXE1610 VR driver
On 5/29/19, 6:01 PM, "Guenter Roeck" wrote:
On 5/29/19 3:35 PM, Vijay Khemka wrote:
> Added pmbus driver for the new device Infineon pxe1610
> voltage regulator. It also supports similar family device
> PXE1110 and PXM1310.
>
> Signed-off-by: Vijay Khemka
> ---
> drivers/hwmon/pmbus/Kconfig | 9 +++
> drivers/hwmon/pmbus/Makefile | 1 +
> drivers/hwmon/pmbus/pxe1610.c | 119 ++
> 3 files changed, 129 insertions(+)
> create mode 100644 drivers/hwmon/pmbus/pxe1610.c
>
> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> index 30751eb9550a..338ef9b5a395 100644
> --- a/drivers/hwmon/pmbus/Kconfig
> +++ b/drivers/hwmon/pmbus/Kconfig
> @@ -154,6 +154,15 @@ config SENSORS_MAX8688
> This driver can also be built as a module. If so, the module
will
> be called max8688.
>
> +config SENSORS_PXE1610
> + tristate "Infineon PXE1610"
> + help
> + If you say yes here you get hardware monitoring support for Infineon
> + PXE1610.
> +
> + This driver can also be built as a module. If so, the module will
> + be called pxe1610.
> +
> config SENSORS_TPS40422
> tristate "TI TPS40422"
> help
> diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
> index 2219b9300316..b0fbd017a91a 100644
> --- a/drivers/hwmon/pmbus/Makefile
> +++ b/drivers/hwmon/pmbus/Makefile
> @@ -18,6 +18,7 @@ obj-$(CONFIG_SENSORS_MAX20751) += max20751.o
> obj-$(CONFIG_SENSORS_MAX31785) += max31785.o
> obj-$(CONFIG_SENSORS_MAX34440) += max34440.o
> obj-$(CONFIG_SENSORS_MAX8688) += max8688.o
> +obj-$(CONFIG_SENSORS_PXE1610)+= pxe1610.o
> obj-$(CONFIG_SENSORS_TPS40422) += tps40422.o
> obj-$(CONFIG_SENSORS_TPS53679) += tps53679.o
> obj-$(CONFIG_SENSORS_UCD9000) += ucd9000.o
> diff --git a/drivers/hwmon/pmbus/pxe1610.c b/drivers/hwmon/pmbus/pxe1610.c
> new file mode 100644
> index ..01e267944df5
> --- /dev/null
> +++ b/drivers/hwmon/pmbus/pxe1610.c
> @@ -0,0 +1,119 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Hardware monitoring driver for Infineon PXE1610
> + *
> + * Copyright (c) 2019 Facebook Inc
> + *
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include "pmbus.h"
> +
> +/*
> + * Identify chip parameters.
> + */
> +static int pxe1610_identify(struct i2c_client *client,
> + struct pmbus_driver_info *info)
Please align continuation lines with '('.
Ack.
> +{
> + if (pmbus_check_byte_register(client, 0, PMBUS_VOUT_MODE)) {
> + int vout_mode;
> +
> + vout_mode = pmbus_read_byte_data(client, 0, PMBUS_VOUT_MODE);
pmbus_read_byte_data() can return an error. Calling
pmbus_check_byte_register()
doesn't really add any value here, since the second call can still fail,
which needs to be checked.
Ack.
> + switch (vout_mode & 0x1f) {
> + case 1:
> + info->vrm_version = vr12;
> + break;
Alignment is off.
Ack.
> + case 2:
> + info->vrm_version = vr13;
> + break;
Same here.
> + default:
> + return -ENODEV;
> + }
> + }
> + return 0;
> +}
> +
> +static int pxe1610_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct pmbus_driver_info *info;
> + u8 buf[I2C_SMBUS_BLOCK_MAX];
> + int ret;
> +
> + if (!i2c_check_functionality(client->adapter,
> + I2C_FUNC_SMBUS_READ_BYTE_DATA
> + | I2C_FUNC_SMBUS_READ_WORD_DATA
> + | I2C_FUNC_SMBUS_READ_BLOCK_DATA))
> + return -ENODEV;
> +
> + /* By default this device doesn't boot to page 0, so set page 0
> + * to access all pmbus registers.
> + */
Please use standard multi-line comments.
Ack.
> + i2c_smbus_write_byte_data(client, 0, 0);
> +
Please use the PMBUS_PAGE command definition.
Will use.
I wonder if it would make sense to initialize currpage in the core to an
unreasonable
number for multi-page chips, but I guess that is a different que
Re: [PATCH] misc: aspeed-lpc-ctrl: make parameter optional
On 5/24/19, 8:59 AM, "Greg Kroah-Hartman" wrote: On Wed, May 01, 2019 at 03:34:11PM -0700, Vijay Khemka wrote: > Makiing memory-region and flash as optional parameter in device > tree if user needs to use these parameter through ioctl then > need to define in devicetree. > > Signed-off-by: Vijay Khemka > --- > drivers/misc/aspeed-lpc-ctrl.c | 58 +- > 1 file changed, 36 insertions(+), 22 deletions(-) File is no longer at this location :( I have rebased to new location and combined both patches and submitted again. __ Regards -Vijay
[PATCH] soc: aspeed: lpc-ctrl: make parameter optional
Makiing memory-region and flash as optional parameter in device
tree if user needs to use these parameter through ioctl then
need to define in devicetree.
Signed-off-by: Vijay Khemka
---
drivers/soc/aspeed/aspeed-lpc-ctrl.c | 58 +---
1 file changed, 36 insertions(+), 22 deletions(-)
diff --git a/drivers/soc/aspeed/aspeed-lpc-ctrl.c
b/drivers/soc/aspeed/aspeed-lpc-ctrl.c
index a024f8042259..aca13779764a 100644
--- a/drivers/soc/aspeed/aspeed-lpc-ctrl.c
+++ b/drivers/soc/aspeed/aspeed-lpc-ctrl.c
@@ -68,6 +68,7 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file, unsigned
int cmd,
unsigned long param)
{
struct aspeed_lpc_ctrl *lpc_ctrl = file_aspeed_lpc_ctrl(file);
+ struct device *dev = file->private_data;
void __user *p = (void __user *)param;
struct aspeed_lpc_ctrl_mapping map;
u32 addr;
@@ -90,6 +91,12 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file,
unsigned int cmd,
if (map.window_id != 0)
return -EINVAL;
+ /* If memory-region is not described in device tree */
+ if (!lpc_ctrl->mem_size) {
+ dev_dbg(dev, "Didn't find reserved memory\n");
+ return -ENXIO;
+ }
+
map.size = lpc_ctrl->mem_size;
return copy_to_user(p, , sizeof(map)) ? -EFAULT : 0;
@@ -126,9 +133,18 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file,
unsigned int cmd,
return -EINVAL;
if (map.window_type == ASPEED_LPC_CTRL_WINDOW_FLASH) {
+ if (!lpc_ctrl->pnor_size) {
+ dev_dbg(dev, "Didn't find host pnor flash\n");
+ return -ENXIO;
+ }
addr = lpc_ctrl->pnor_base;
size = lpc_ctrl->pnor_size;
} else if (map.window_type == ASPEED_LPC_CTRL_WINDOW_MEMORY) {
+ /* If memory-region is not described in device tree */
+ if (!lpc_ctrl->mem_size) {
+ dev_dbg(dev, "Didn't find reserved memory\n");
+ return -ENXIO;
+ }
addr = lpc_ctrl->mem_base;
size = lpc_ctrl->mem_size;
} else {
@@ -196,17 +212,17 @@ static int aspeed_lpc_ctrl_probe(struct platform_device
*pdev)
if (!lpc_ctrl)
return -ENOMEM;
+ /* If flash is described in device tree then store */
node = of_parse_phandle(dev->of_node, "flash", 0);
if (!node) {
- dev_err(dev, "Didn't find host pnor flash node\n");
- return -ENODEV;
- }
-
- rc = of_address_to_resource(node, 1, );
- of_node_put(node);
- if (rc) {
- dev_err(dev, "Couldn't address to resource for flash\n");
- return rc;
+ dev_dbg(dev, "Didn't find host pnor flash node\n");
+ } else {
+ rc = of_address_to_resource(node, 1, );
+ of_node_put(node);
+ if (rc) {
+ dev_err(dev, "Couldn't address to resource for
flash\n");
+ return rc;
+ }
}
lpc_ctrl->pnor_size = resource_size();
@@ -214,22 +230,22 @@ static int aspeed_lpc_ctrl_probe(struct platform_device
*pdev)
dev_set_drvdata(>dev, lpc_ctrl);
+ /* If memory-region is described in device tree then store */
node = of_parse_phandle(dev->of_node, "memory-region", 0);
if (!node) {
- dev_err(dev, "Didn't find reserved memory\n");
- return -EINVAL;
- }
+ dev_dbg(dev, "Didn't find reserved memory\n");
+ } else {
+ rc = of_address_to_resource(node, 0, );
+ of_node_put(node);
+ if (rc) {
+ dev_err(dev, "Couldn't address to resource for reserved
memory\n");
+ return -ENXIO;
+ }
- rc = of_address_to_resource(node, 0, );
- of_node_put(node);
- if (rc) {
- dev_err(dev, "Couldn't address to resource for reserved
memory\n");
- return -ENOMEM;
+ lpc_ctrl->mem_size = resource_size();
+ lpc_ctrl->mem_base = resm.start;
}
- lpc_ctrl->mem_size = resource_size();
- lpc_ctrl->mem_base = resm.start;
-
lpc_ctrl->regmap = syscon_node_to_regmap(
pdev->dev.parent->of_node);
if (IS_ERR(lpc_ctrl->regmap)) {
@@ -258,8 +274,6 @@ static int aspeed_lpc_ctrl_probe(struct platform_device
*pdev)
goto err;
}
- dev_info(dev, "Loaded at %pr\n", );
-
return 0;
err:
--
2.17.1
Re: [PATCH v2] misc: aspeed-lpc-ctrl: Correct return values
On 5/5/19, 9:24 PM, "Andrew Jeffery" wrote:
On Sat, 4 May 2019, at 03:43, Vijay Khemka wrote:
> Corrected some of return values with appropriate meanings and reported
> relevant messages as debug information.
>
> Signed-off-by: Vijay Khemka
Thanks for the fixes, this looks okay now. However, was there a reason for
not squashing change into your previous patch that introduced the issues
this fixes? That hasn't been applied yet either as far as I'm aware. I'd
prefer
we do that and submit a single, good patch if we can.
First patch has already been applied to LF openbmc kernel and being used by many
people so I wanted to keep this clean.
Andrew
> ---
> drivers/misc/aspeed-lpc-ctrl.c | 14 +++---
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/misc/aspeed-lpc-ctrl.c
> b/drivers/misc/aspeed-lpc-ctrl.c
> index 332210e06e98..aca13779764a 100644
> --- a/drivers/misc/aspeed-lpc-ctrl.c
> +++ b/drivers/misc/aspeed-lpc-ctrl.c
> @@ -93,8 +93,8 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file,
> unsigned int cmd,
>
> /* If memory-region is not described in device tree */
> if (!lpc_ctrl->mem_size) {
> - dev_err(dev, "Didn't find reserved memory\n");
> - return -EINVAL;
> + dev_dbg(dev, "Didn't find reserved memory\n");
> + return -ENXIO;
> }
>
> map.size = lpc_ctrl->mem_size;
> @@ -134,16 +134,16 @@ static long aspeed_lpc_ctrl_ioctl(struct file
> *file, unsigned int cmd,
>
> if (map.window_type == ASPEED_LPC_CTRL_WINDOW_FLASH) {
> if (!lpc_ctrl->pnor_size) {
> - dev_err(dev, "Didn't find host pnor flash\n");
> - return -EINVAL;
> + dev_dbg(dev, "Didn't find host pnor flash\n");
> + return -ENXIO;
> }
> addr = lpc_ctrl->pnor_base;
> size = lpc_ctrl->pnor_size;
> } else if (map.window_type == ASPEED_LPC_CTRL_WINDOW_MEMORY) {
> /* If memory-region is not described in device tree */
> if (!lpc_ctrl->mem_size) {
> - dev_err(dev, "Didn't find reserved memory\n");
> - return -EINVAL;
> + dev_dbg(dev, "Didn't find reserved memory\n");
> + return -ENXIO;
> }
> addr = lpc_ctrl->mem_base;
> size = lpc_ctrl->mem_size;
> @@ -239,7 +239,7 @@ static int aspeed_lpc_ctrl_probe(struct
> platform_device *pdev)
> of_node_put(node);
> if (rc) {
> dev_err(dev, "Couldn't address to resource for reserved
memory\n");
> - return -ENOMEM;
> + return -ENXIO;
> }
>
> lpc_ctrl->mem_size = resource_size();
> --
> 2.17.1
>
>
[PATCH v2] misc: aspeed-lpc-ctrl: Correct return values
Corrected some of return values with appropriate meanings and reported
relevant messages as debug information.
Signed-off-by: Vijay Khemka
---
drivers/misc/aspeed-lpc-ctrl.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/misc/aspeed-lpc-ctrl.c b/drivers/misc/aspeed-lpc-ctrl.c
index 332210e06e98..aca13779764a 100644
--- a/drivers/misc/aspeed-lpc-ctrl.c
+++ b/drivers/misc/aspeed-lpc-ctrl.c
@@ -93,8 +93,8 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file, unsigned
int cmd,
/* If memory-region is not described in device tree */
if (!lpc_ctrl->mem_size) {
- dev_err(dev, "Didn't find reserved memory\n");
- return -EINVAL;
+ dev_dbg(dev, "Didn't find reserved memory\n");
+ return -ENXIO;
}
map.size = lpc_ctrl->mem_size;
@@ -134,16 +134,16 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file,
unsigned int cmd,
if (map.window_type == ASPEED_LPC_CTRL_WINDOW_FLASH) {
if (!lpc_ctrl->pnor_size) {
- dev_err(dev, "Didn't find host pnor flash\n");
- return -EINVAL;
+ dev_dbg(dev, "Didn't find host pnor flash\n");
+ return -ENXIO;
}
addr = lpc_ctrl->pnor_base;
size = lpc_ctrl->pnor_size;
} else if (map.window_type == ASPEED_LPC_CTRL_WINDOW_MEMORY) {
/* If memory-region is not described in device tree */
if (!lpc_ctrl->mem_size) {
- dev_err(dev, "Didn't find reserved memory\n");
- return -EINVAL;
+ dev_dbg(dev, "Didn't find reserved memory\n");
+ return -ENXIO;
}
addr = lpc_ctrl->mem_base;
size = lpc_ctrl->mem_size;
@@ -239,7 +239,7 @@ static int aspeed_lpc_ctrl_probe(struct platform_device
*pdev)
of_node_put(node);
if (rc) {
dev_err(dev, "Couldn't address to resource for reserved
memory\n");
- return -ENOMEM;
+ return -ENXIO;
}
lpc_ctrl->mem_size = resource_size();
--
2.17.1
Re: [PATCH] misc: aspeed-lpc-ctrl: Correct return values
On 5/1/19, 11:49 PM, "Andrew Jeffery" wrote:
On Thu, 2 May 2019, at 16:10, Greg Kroah-Hartman wrote:
> On Wed, May 01, 2019 at 03:38:36PM -0700, Vijay Khemka wrote:
> > Corrected some of return values with appropriate meanings.
> >
> > Signed-off-by: Vijay Khemka
> > ---
> > drivers/misc/aspeed-lpc-ctrl.c | 15 +++
> > 1 file changed, 7 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/misc/aspeed-lpc-ctrl.c
b/drivers/misc/aspeed-lpc-ctrl.c
> > index 332210e06e98..97ae341109d5 100644
> > --- a/drivers/misc/aspeed-lpc-ctrl.c
> > +++ b/drivers/misc/aspeed-lpc-ctrl.c
> > @@ -68,7 +68,6 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file,
unsigned int cmd,
> > unsigned long param)
> > {
> > struct aspeed_lpc_ctrl *lpc_ctrl = file_aspeed_lpc_ctrl(file);
> > - struct device *dev = file->private_data;
> > void __user *p = (void __user *)param;
> > struct aspeed_lpc_ctrl_mapping map;
> > u32 addr;
>
> This change is not reflected in your changelog text :(
>
> Please fix up, or break this up into multiple patches.
The return value fixes should also be squashed into the patch that
introduced those lines
given it hasn't yet been applied.
Further, IIRC I previously suggested removing the dev_err()s entirely, not
just switching
them to pr_err(). Returning an error code is enough IMO, there's no need to
pollute the
kernel logs with application-level errors. Or make them dev_dbg().
Alright, I will replace with dev_dbg(), information can still be extracted by
user with debug.
Andrew
>
> greg k-h
>
[PATCH] misc: aspeed-lpc-ctrl: Correct return values
Corrected some of return values with appropriate meanings.
Signed-off-by: Vijay Khemka
---
drivers/misc/aspeed-lpc-ctrl.c | 15 +++
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/drivers/misc/aspeed-lpc-ctrl.c b/drivers/misc/aspeed-lpc-ctrl.c
index 332210e06e98..97ae341109d5 100644
--- a/drivers/misc/aspeed-lpc-ctrl.c
+++ b/drivers/misc/aspeed-lpc-ctrl.c
@@ -68,7 +68,6 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file, unsigned
int cmd,
unsigned long param)
{
struct aspeed_lpc_ctrl *lpc_ctrl = file_aspeed_lpc_ctrl(file);
- struct device *dev = file->private_data;
void __user *p = (void __user *)param;
struct aspeed_lpc_ctrl_mapping map;
u32 addr;
@@ -93,8 +92,8 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file, unsigned
int cmd,
/* If memory-region is not described in device tree */
if (!lpc_ctrl->mem_size) {
- dev_err(dev, "Didn't find reserved memory\n");
- return -EINVAL;
+ pr_err("aspeed_lpc_ctrl: ioctl: Didn't find reserved
memory\n");
+ return -ENXIO;
}
map.size = lpc_ctrl->mem_size;
@@ -134,16 +133,16 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file,
unsigned int cmd,
if (map.window_type == ASPEED_LPC_CTRL_WINDOW_FLASH) {
if (!lpc_ctrl->pnor_size) {
- dev_err(dev, "Didn't find host pnor flash\n");
- return -EINVAL;
+ pr_err("aspeed_lpc_ctrl: ioctl: Didn't find
host pnor flash\n");
+ return -ENXIO;
}
addr = lpc_ctrl->pnor_base;
size = lpc_ctrl->pnor_size;
} else if (map.window_type == ASPEED_LPC_CTRL_WINDOW_MEMORY) {
/* If memory-region is not described in device tree */
if (!lpc_ctrl->mem_size) {
- dev_err(dev, "Didn't find reserved memory\n");
- return -EINVAL;
+ pr_err("aspeed_lpc_ctrl: ioctl: Didn't find
reserved memory\n");
+ return -ENXIO;
}
addr = lpc_ctrl->mem_base;
size = lpc_ctrl->mem_size;
@@ -239,7 +238,7 @@ static int aspeed_lpc_ctrl_probe(struct platform_device
*pdev)
of_node_put(node);
if (rc) {
dev_err(dev, "Couldn't address to resource for reserved
memory\n");
- return -ENOMEM;
+ return -ENXIO;
}
lpc_ctrl->mem_size = resource_size();
--
2.17.1
[PATCH] misc: aspeed-lpc-ctrl: make parameter optional
Makiing memory-region and flash as optional parameter in device
tree if user needs to use these parameter through ioctl then
need to define in devicetree.
Signed-off-by: Vijay Khemka
---
drivers/misc/aspeed-lpc-ctrl.c | 58 +-
1 file changed, 36 insertions(+), 22 deletions(-)
diff --git a/drivers/misc/aspeed-lpc-ctrl.c b/drivers/misc/aspeed-lpc-ctrl.c
index a024f8042259..332210e06e98 100644
--- a/drivers/misc/aspeed-lpc-ctrl.c
+++ b/drivers/misc/aspeed-lpc-ctrl.c
@@ -68,6 +68,7 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file, unsigned
int cmd,
unsigned long param)
{
struct aspeed_lpc_ctrl *lpc_ctrl = file_aspeed_lpc_ctrl(file);
+ struct device *dev = file->private_data;
void __user *p = (void __user *)param;
struct aspeed_lpc_ctrl_mapping map;
u32 addr;
@@ -90,6 +91,12 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file,
unsigned int cmd,
if (map.window_id != 0)
return -EINVAL;
+ /* If memory-region is not described in device tree */
+ if (!lpc_ctrl->mem_size) {
+ dev_err(dev, "Didn't find reserved memory\n");
+ return -EINVAL;
+ }
+
map.size = lpc_ctrl->mem_size;
return copy_to_user(p, , sizeof(map)) ? -EFAULT : 0;
@@ -126,9 +133,18 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file,
unsigned int cmd,
return -EINVAL;
if (map.window_type == ASPEED_LPC_CTRL_WINDOW_FLASH) {
+ if (!lpc_ctrl->pnor_size) {
+ dev_err(dev, "Didn't find host pnor flash\n");
+ return -EINVAL;
+ }
addr = lpc_ctrl->pnor_base;
size = lpc_ctrl->pnor_size;
} else if (map.window_type == ASPEED_LPC_CTRL_WINDOW_MEMORY) {
+ /* If memory-region is not described in device tree */
+ if (!lpc_ctrl->mem_size) {
+ dev_err(dev, "Didn't find reserved memory\n");
+ return -EINVAL;
+ }
addr = lpc_ctrl->mem_base;
size = lpc_ctrl->mem_size;
} else {
@@ -196,17 +212,17 @@ static int aspeed_lpc_ctrl_probe(struct platform_device
*pdev)
if (!lpc_ctrl)
return -ENOMEM;
+ /* If flash is described in device tree then store */
node = of_parse_phandle(dev->of_node, "flash", 0);
if (!node) {
- dev_err(dev, "Didn't find host pnor flash node\n");
- return -ENODEV;
- }
-
- rc = of_address_to_resource(node, 1, );
- of_node_put(node);
- if (rc) {
- dev_err(dev, "Couldn't address to resource for flash\n");
- return rc;
+ dev_dbg(dev, "Didn't find host pnor flash node\n");
+ } else {
+ rc = of_address_to_resource(node, 1, );
+ of_node_put(node);
+ if (rc) {
+ dev_err(dev, "Couldn't address to resource for
flash\n");
+ return rc;
+ }
}
lpc_ctrl->pnor_size = resource_size();
@@ -214,22 +230,22 @@ static int aspeed_lpc_ctrl_probe(struct platform_device
*pdev)
dev_set_drvdata(>dev, lpc_ctrl);
+ /* If memory-region is described in device tree then store */
node = of_parse_phandle(dev->of_node, "memory-region", 0);
if (!node) {
- dev_err(dev, "Didn't find reserved memory\n");
- return -EINVAL;
- }
+ dev_dbg(dev, "Didn't find reserved memory\n");
+ } else {
+ rc = of_address_to_resource(node, 0, );
+ of_node_put(node);
+ if (rc) {
+ dev_err(dev, "Couldn't address to resource for reserved
memory\n");
+ return -ENOMEM;
+ }
- rc = of_address_to_resource(node, 0, );
- of_node_put(node);
- if (rc) {
- dev_err(dev, "Couldn't address to resource for reserved
memory\n");
- return -ENOMEM;
+ lpc_ctrl->mem_size = resource_size();
+ lpc_ctrl->mem_base = resm.start;
}
- lpc_ctrl->mem_size = resource_size();
- lpc_ctrl->mem_base = resm.start;
-
lpc_ctrl->regmap = syscon_node_to_regmap(
pdev->dev.parent->of_node);
if (IS_ERR(lpc_ctrl->regmap)) {
@@ -258,8 +274,6 @@ static int aspeed_lpc_ctrl_probe(struct platform_device
*pdev)
goto err;
}
- dev_info(dev, "Loaded at %pr\n", );
-
return 0;
err:
--
2.17.1
Re: [PATCH v2] misc: aspeed-lpc-ctrl: make parameter optional
Let me send both patches. On 4/30/19, 11:45 PM, "Greg Kroah-Hartman" wrote: On Wed, May 01, 2019 at 05:55:07AM +, Joel Stanley wrote: > On Fri, 18 Jan 2019 at 20:12, Vijay Khemka wrote: > > > > Hi Andrew, > > Thanks for this review, I will have a follow up patch for this return values. > > Did you send a follow up patch to fix the return values? > > Greg, is there any reason why you did not merge this one? 5.2 will > have device trees that depend on this patch's behavior. No idea, if it needs to be applied, please resend. thanks, greg k-h
Re: [Potential Spoof] Re: [Potential Spoof] Re: [PATCH] ARM: dts: aspeed: tiogapass: Enable VUART
Hi Joel,
Please apply this patch.
Regards
-Vijay
On 3/18/19, 12:46 PM, "openbmc on behalf of Vijay Khemka"
wrote:
Hi Joel,
Please apply this patch.
Regards
-Vijay
On 3/5/19, 12:06 PM, "openbmc on behalf of Vijay Khemka"
wrote:
Please review below patch.
Regards
-Vijay
On 1/30/19, 10:14 AM, "Vijay Khemka" wrote:
Enabling vuart for Facebook tiogapass
Signed-off-by: Vijay Khemka
---
arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts | 5 +
1 file changed, 5 insertions(+)
diff --git a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
index 42e0d7a8e8d0..a058fb2985f7 100644
--- a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
@@ -64,6 +64,11 @@
status = "okay";
};
+ {
+ // VUART Host Console
+ status = "okay";
+};
+
{
// Host Console
status = "okay";
--
2.17.1
Re: [Potential Spoof] Re: [Potential Spoof] Re: [Potential Spoof] Re: [PATCH v2] misc: aspeed-lpc-ctrl: make parameter optional
Hi Joel,
Can you please apply this below patch to kernel 5.0.
Regards
-Vijay
On 3/18/19, 12:46 PM, "openbmc on behalf of Vijay Khemka"
wrote:
Hi Joel,
Can you please apply this patch as "
Documentation/devicetree/bindings/mfd/aspeed-lpc.txt" has already been applied
Regards
-Vijay
On 3/5/19, 4:15 PM, "Linux-aspeed on behalf of Vijay Khemka"
wrote:
Joel,
Did this patch apply upstream. Somehow I can't find this patch in linux
or linux-next or our obmc dev4.19.
Regards
-Vijay
On 1/17/19, 10:53 AM, "Linux-aspeed on behalf of Vijay Khemka"
wrote:
On 1/16/19, 10:17 PM, "Joel Stanley" wrote:
On Thu, 17 Jan 2019 at 09:02, Vijay Khemka
wrote:
>
> Makiing memory-region and flash as optional parameter in
device
> tree if user needs to use these parameter through ioctl then
> need to define in devicetree.
>
> Signed-off-by: Vijay Khemka
Thanks! This looks okay to me. I tested it on one of our
systems which
uses both flash and reserved memory and it was fine.
Reviewed-by: Joel Stanley
Can you also send a patch to update the bindings at
Documentation/devicetree/bindings/mfd/aspeed-lpc.txt ? I think
the
only change you need to make is to move the memory region and
flash
properties to optional (instead of required).
Sure I will do this.
Cheers,
Joel
> ---
> drivers/misc/aspeed-lpc-ctrl.c | 58
+-
> 1 file changed, 36 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/misc/aspeed-lpc-ctrl.c
b/drivers/misc/aspeed-lpc-ctrl.c
> index a024f8042259..332210e06e98 100644
> --- a/drivers/misc/aspeed-lpc-ctrl.c
> +++ b/drivers/misc/aspeed-lpc-ctrl.c
> @@ -68,6 +68,7 @@ static long aspeed_lpc_ctrl_ioctl(struct
file *file, unsigned int cmd,
> unsigned long param)
> {
> struct aspeed_lpc_ctrl *lpc_ctrl =
file_aspeed_lpc_ctrl(file);
> + struct device *dev = file->private_data;
> void __user *p = (void __user *)param;
> struct aspeed_lpc_ctrl_mapping map;
> u32 addr;
> @@ -90,6 +91,12 @@ static long aspeed_lpc_ctrl_ioctl(struct
file *file, unsigned int cmd,
> if (map.window_id != 0)
> return -EINVAL;
>
> + /* If memory-region is not described in
device tree */
> + if (!lpc_ctrl->mem_size) {
> + dev_err(dev, "Didn't find reserved
memory\n");
> + return -EINVAL;
> + }
> +
> map.size = lpc_ctrl->mem_size;
>
> return copy_to_user(p, , sizeof(map)) ?
-EFAULT : 0;
> @@ -126,9 +133,18 @@ static long aspeed_lpc_ctrl_ioctl(struct
file *file, unsigned int cmd,
> return -EINVAL;
>
> if (map.window_type ==
ASPEED_LPC_CTRL_WINDOW_FLASH) {
> + if (!lpc_ctrl->pnor_size) {
> + dev_err(dev, "Didn't find
host pnor flash\n");
> + return -EINVAL;
> + }
> addr = lpc_ctrl->pnor_base;
> size = lpc_ctrl->pnor_size;
> } else if (map.window_type ==
ASPEED_LPC_CTRL_WINDOW_MEMORY) {
> + /* If memory-region is not described
in device tree */
> + if (!lpc_ctrl->mem_size) {
> + dev_err(dev, "Didn't find
reserved memory\n");
> + return -EINVAL;
> + }
>
Re: [Potential Spoof] Re: [PATCH] ARM: dts: aspeed: tiogapass: Enable VUART
Hi Joel,
Please apply this patch.
Regards
-Vijay
On 3/5/19, 12:06 PM, "openbmc on behalf of Vijay Khemka"
wrote:
Please review below patch.
Regards
-Vijay
On 1/30/19, 10:14 AM, "Vijay Khemka" wrote:
Enabling vuart for Facebook tiogapass
Signed-off-by: Vijay Khemka
---
arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts | 5 +
1 file changed, 5 insertions(+)
diff --git a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
index 42e0d7a8e8d0..a058fb2985f7 100644
--- a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
@@ -64,6 +64,11 @@
status = "okay";
};
+ {
+ // VUART Host Console
+ status = "okay";
+};
+
{
// Host Console
status = "okay";
--
2.17.1
Re: [Potential Spoof] Re: [Potential Spoof] Re: [PATCH v2] misc: aspeed-lpc-ctrl: make parameter optional
Hi Joel,
Can you please apply this patch as "
Documentation/devicetree/bindings/mfd/aspeed-lpc.txt" has already been applied
Regards
-Vijay
On 3/5/19, 4:15 PM, "Linux-aspeed on behalf of Vijay Khemka"
wrote:
Joel,
Did this patch apply upstream. Somehow I can't find this patch in linux or
linux-next or our obmc dev4.19.
Regards
-Vijay
On 1/17/19, 10:53 AM, "Linux-aspeed on behalf of Vijay Khemka"
wrote:
On 1/16/19, 10:17 PM, "Joel Stanley" wrote:
On Thu, 17 Jan 2019 at 09:02, Vijay Khemka
wrote:
>
> Makiing memory-region and flash as optional parameter in device
> tree if user needs to use these parameter through ioctl then
> need to define in devicetree.
>
> Signed-off-by: Vijay Khemka
Thanks! This looks okay to me. I tested it on one of our systems
which
uses both flash and reserved memory and it was fine.
Reviewed-by: Joel Stanley
Can you also send a patch to update the bindings at
Documentation/devicetree/bindings/mfd/aspeed-lpc.txt ? I think the
only change you need to make is to move the memory region and flash
properties to optional (instead of required).
Sure I will do this.
Cheers,
Joel
> ---
> drivers/misc/aspeed-lpc-ctrl.c | 58
+-
> 1 file changed, 36 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/misc/aspeed-lpc-ctrl.c
b/drivers/misc/aspeed-lpc-ctrl.c
> index a024f8042259..332210e06e98 100644
> --- a/drivers/misc/aspeed-lpc-ctrl.c
> +++ b/drivers/misc/aspeed-lpc-ctrl.c
> @@ -68,6 +68,7 @@ static long aspeed_lpc_ctrl_ioctl(struct file
*file, unsigned int cmd,
> unsigned long param)
> {
> struct aspeed_lpc_ctrl *lpc_ctrl =
file_aspeed_lpc_ctrl(file);
> + struct device *dev = file->private_data;
> void __user *p = (void __user *)param;
> struct aspeed_lpc_ctrl_mapping map;
> u32 addr;
> @@ -90,6 +91,12 @@ static long aspeed_lpc_ctrl_ioctl(struct file
*file, unsigned int cmd,
> if (map.window_id != 0)
> return -EINVAL;
>
> + /* If memory-region is not described in device
tree */
> + if (!lpc_ctrl->mem_size) {
> + dev_err(dev, "Didn't find reserved
memory\n");
> + return -EINVAL;
> + }
> +
> map.size = lpc_ctrl->mem_size;
>
> return copy_to_user(p, , sizeof(map)) ?
-EFAULT : 0;
> @@ -126,9 +133,18 @@ static long aspeed_lpc_ctrl_ioctl(struct
file *file, unsigned int cmd,
> return -EINVAL;
>
> if (map.window_type ==
ASPEED_LPC_CTRL_WINDOW_FLASH) {
> + if (!lpc_ctrl->pnor_size) {
> + dev_err(dev, "Didn't find host
pnor flash\n");
> + return -EINVAL;
> + }
> addr = lpc_ctrl->pnor_base;
> size = lpc_ctrl->pnor_size;
> } else if (map.window_type ==
ASPEED_LPC_CTRL_WINDOW_MEMORY) {
> + /* If memory-region is not described in
device tree */
> + if (!lpc_ctrl->mem_size) {
> + dev_err(dev, "Didn't find
reserved memory\n");
> + return -EINVAL;
> + }
> addr = lpc_ctrl->mem_base;
> size = lpc_ctrl->mem_size;
> } else {
> @@ -196,17 +212,17 @@ static int aspeed_lpc_ctrl_probe(struct
platform_device *pdev)
> if (!lpc_ctrl)
> return -ENOMEM;
>
> + /* If flash is described in device tree then store */
> node = of_parse_phandle(dev->of_
Re: [Potential Spoof] Re: [PATCH v2] misc: aspeed-lpc-ctrl: make parameter optional
Joel,
Did this patch apply upstream. Somehow I can't find this patch in linux or
linux-next or our obmc dev4.19.
Regards
-Vijay
On 1/17/19, 10:53 AM, "Linux-aspeed on behalf of Vijay Khemka"
wrote:
On 1/16/19, 10:17 PM, "Joel Stanley" wrote:
On Thu, 17 Jan 2019 at 09:02, Vijay Khemka wrote:
>
> Makiing memory-region and flash as optional parameter in device
> tree if user needs to use these parameter through ioctl then
> need to define in devicetree.
>
> Signed-off-by: Vijay Khemka
Thanks! This looks okay to me. I tested it on one of our systems which
uses both flash and reserved memory and it was fine.
Reviewed-by: Joel Stanley
Can you also send a patch to update the bindings at
Documentation/devicetree/bindings/mfd/aspeed-lpc.txt ? I think the
only change you need to make is to move the memory region and flash
properties to optional (instead of required).
Sure I will do this.
Cheers,
Joel
> ---
> drivers/misc/aspeed-lpc-ctrl.c | 58
+-
> 1 file changed, 36 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/misc/aspeed-lpc-ctrl.c
b/drivers/misc/aspeed-lpc-ctrl.c
> index a024f8042259..332210e06e98 100644
> --- a/drivers/misc/aspeed-lpc-ctrl.c
> +++ b/drivers/misc/aspeed-lpc-ctrl.c
> @@ -68,6 +68,7 @@ static long aspeed_lpc_ctrl_ioctl(struct file
*file, unsigned int cmd,
> unsigned long param)
> {
> struct aspeed_lpc_ctrl *lpc_ctrl = file_aspeed_lpc_ctrl(file);
> + struct device *dev = file->private_data;
> void __user *p = (void __user *)param;
> struct aspeed_lpc_ctrl_mapping map;
> u32 addr;
> @@ -90,6 +91,12 @@ static long aspeed_lpc_ctrl_ioctl(struct file
*file, unsigned int cmd,
> if (map.window_id != 0)
> return -EINVAL;
>
> + /* If memory-region is not described in device tree */
> + if (!lpc_ctrl->mem_size) {
> + dev_err(dev, "Didn't find reserved memory\n");
> + return -EINVAL;
> + }
> +
> map.size = lpc_ctrl->mem_size;
>
> return copy_to_user(p, , sizeof(map)) ? -EFAULT :
0;
> @@ -126,9 +133,18 @@ static long aspeed_lpc_ctrl_ioctl(struct file
*file, unsigned int cmd,
> return -EINVAL;
>
> if (map.window_type == ASPEED_LPC_CTRL_WINDOW_FLASH) {
> + if (!lpc_ctrl->pnor_size) {
> + dev_err(dev, "Didn't find host pnor
flash\n");
> + return -EINVAL;
> + }
> addr = lpc_ctrl->pnor_base;
> size = lpc_ctrl->pnor_size;
> } else if (map.window_type ==
ASPEED_LPC_CTRL_WINDOW_MEMORY) {
> + /* If memory-region is not described in
device tree */
> + if (!lpc_ctrl->mem_size) {
> + dev_err(dev, "Didn't find reserved
memory\n");
> + return -EINVAL;
> + }
> addr = lpc_ctrl->mem_base;
> size = lpc_ctrl->mem_size;
> } else {
> @@ -196,17 +212,17 @@ static int aspeed_lpc_ctrl_probe(struct
platform_device *pdev)
> if (!lpc_ctrl)
> return -ENOMEM;
>
> + /* If flash is described in device tree then store */
> node = of_parse_phandle(dev->of_node, "flash", 0);
> if (!node) {
> - dev_err(dev, "Didn't find host pnor flash node\n");
> - return -ENODEV;
> - }
> -
> - rc = of_address_to_resource(node, 1, );
> - of_node_put(node);
> - if (rc) {
> - dev_err(dev, "Couldn't address to resource for
flash\n");
> - return rc;
> + dev_dbg(dev, "Didn't find host pno
Re: [PATCH] ARM: dts: aspeed: tiogapass: Enable VUART
Please review below patch.
Regards
-Vijay
On 1/30/19, 10:14 AM, "Vijay Khemka" wrote:
Enabling vuart for Facebook tiogapass
Signed-off-by: Vijay Khemka
---
arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts | 5 +
1 file changed, 5 insertions(+)
diff --git a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
index 42e0d7a8e8d0..a058fb2985f7 100644
--- a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
@@ -64,6 +64,11 @@
status = "okay";
};
+ {
+ // VUART Host Console
+ status = "okay";
+};
+
{
// Host Console
status = "okay";
--
2.17.1
Re: [PATCH] misc: aspeed-lpc-ctrl: Correct return values
On 2/10/19, 9:22 PM, "Andrew Jeffery" wrote:
On Fri, 25 Jan 2019, at 05:59, Vijay Khemka wrote:
>
>
> On 1/24/19, 12:16 AM, "Greg Kroah-Hartman"
wrote:
>
> On Wed, Jan 23, 2019 at 03:06:34PM -0800, Vijay Khemka wrote:
> > Corrected some of return values with appropriate meanings.
> >
> > Signed-off-by: Vijay Khemka
> > ---
> > drivers/misc/aspeed-lpc-ctrl.c | 15 +++
> > 1 file changed, 7 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/misc/aspeed-lpc-ctrl.c b/drivers/misc/aspeed-
> lpc-ctrl.c
> > index 332210e06e98..97ae341109d5 100644
> > --- a/drivers/misc/aspeed-lpc-ctrl.c
> > +++ b/drivers/misc/aspeed-lpc-ctrl.c
> > @@ -68,7 +68,6 @@ static long aspeed_lpc_ctrl_ioctl(struct file
> *file, unsigned int cmd,
> > unsigned long param)
> > {
> > struct aspeed_lpc_ctrl *lpc_ctrl = file_aspeed_lpc_ctrl(file);
> > - struct device *dev = file->private_data;
> > void __user *p = (void __user *)param;
> > struct aspeed_lpc_ctrl_mapping map;
> > u32 addr;
> > @@ -93,8 +92,8 @@ static long aspeed_lpc_ctrl_ioctl(struct file
> *file, unsigned int cmd,
> >
> > /* If memory-region is not described in device tree */
> > if (!lpc_ctrl->mem_size) {
> > - dev_err(dev, "Didn't find reserved memory\n");
> > - return -EINVAL;
> > + pr_err("aspeed_lpc_ctrl: ioctl: Didn't find
reserved memory\n");
>
> Why did you change from dev_err() to pr_err()? You just lost a lot of
> information that the user previously was getting from dev_err() :(
>
> I did this as per review comment from Andrew Jeffery, as we don't want
> to put this error for driver. It has to be handled by userspace. But I
> am still reporting some error here.
Sorry? What I was trying to suggest was removing the logging altogether and
just returning the error code. Not simply changing how the logging is done.
No issue, I can remove logging, will just return error code. Will send new
patch with update.
Andrew
[PATCH] ARM: dts: aspeed: tiogapass: Enable VUART
Enabling vuart for Facebook tiogapass
Signed-off-by: Vijay Khemka
---
arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts | 5 +
1 file changed, 5 insertions(+)
diff --git a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
index 42e0d7a8e8d0..a058fb2985f7 100644
--- a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
@@ -64,6 +64,11 @@
status = "okay";
};
+ {
+ // VUART Host Console
+ status = "okay";
+};
+
{
// Host Console
status = "okay";
--
2.17.1
Re: [PATCH 1/2] ARM: dts: aspeed: Add peci and lpc for Facebook
On 1/29/19, 7:27 PM, "Joel Stanley" wrote:
On Thu, 24 Jan 2019 at 10:07, Vijay Khemka wrote:
> index 73e58a821613..d60dbb019f82 100644
> --- a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
> +++ b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
>
> + {
> + status = "okay";
> +};
I don't think we have an upstream tree that contains a node called
peci0. Therefore this does not build.
I have dropped this hunk from your patch. Please make sure you test
before sending patches.
Thanks Joel, I will be careful.
Cheers,
Joel
Re: [PATCH v2] dt-bindings: aspeed-lpc: Make parameter optional
Thanks Lee,
On 1/29/19, 4:30 AM, "Lee Jones" wrote:
On Fri, 18 Jan 2019, Vijay Khemka wrote:
> Memory-region and flash phandle is not a required parameter, it is
> optional to describe in device tree and needed only use basis.
>
> Signed-off-by: Vijay Khemka
> ---
> Documentation/devicetree/bindings/mfd/aspeed-lpc.txt | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/mfd/aspeed-lpc.txt
b/Documentation/devicetree/bindings/mfd/aspeed-lpc.txt
> index 34dd89087cff..d28c4e117611 100644
> --- a/Documentation/devicetree/bindings/mfd/aspeed-lpc.txt
> +++ b/Documentation/devicetree/bindings/mfd/aspeed-lpc.txt
> @@ -135,6 +135,9 @@ Required properties:
> - clocks:contains a phandle to the syscon node describing the
clocks.
> There should then be one cell representing the clock to use
>
> +Optional properties:
> +These below properties are optional to define.
This line is superfluous. Please remove it.
I will remove it. I am assuming only 1 line to remove.
> +
> - memory-region: A phandle to a reserved_memory region to be used for
the LPC
> to AHB mapping
>
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
[PATCH v3] dt-bindings: aspeed-lpc: Make parameter optional
Memory-region and flash phandle is not a required parameter, it is optional to describe in device tree and needed only use basis. Signed-off-by: Vijay Khemka --- Documentation/devicetree/bindings/mfd/aspeed-lpc.txt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/devicetree/bindings/mfd/aspeed-lpc.txt b/Documentation/devicetree/bindings/mfd/aspeed-lpc.txt index 34dd89087cff..86446074e206 100644 --- a/Documentation/devicetree/bindings/mfd/aspeed-lpc.txt +++ b/Documentation/devicetree/bindings/mfd/aspeed-lpc.txt @@ -135,6 +135,8 @@ Required properties: - clocks: contains a phandle to the syscon node describing the clocks. There should then be one cell representing the clock to use +Optional properties: + - memory-region: A phandle to a reserved_memory region to be used for the LPC to AHB mapping -- 2.17.1
Re: [PATCH] misc: aspeed-lpc-ctrl: Correct return values
On 1/24/19, 12:16 AM, "Greg Kroah-Hartman" wrote:
On Wed, Jan 23, 2019 at 03:06:34PM -0800, Vijay Khemka wrote:
> Corrected some of return values with appropriate meanings.
>
> Signed-off-by: Vijay Khemka
> ---
> drivers/misc/aspeed-lpc-ctrl.c | 15 +++
> 1 file changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/misc/aspeed-lpc-ctrl.c
b/drivers/misc/aspeed-lpc-ctrl.c
> index 332210e06e98..97ae341109d5 100644
> --- a/drivers/misc/aspeed-lpc-ctrl.c
> +++ b/drivers/misc/aspeed-lpc-ctrl.c
> @@ -68,7 +68,6 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file,
unsigned int cmd,
> unsigned long param)
> {
> struct aspeed_lpc_ctrl *lpc_ctrl = file_aspeed_lpc_ctrl(file);
> - struct device *dev = file->private_data;
> void __user *p = (void __user *)param;
> struct aspeed_lpc_ctrl_mapping map;
> u32 addr;
> @@ -93,8 +92,8 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file,
unsigned int cmd,
>
> /* If memory-region is not described in device tree */
> if (!lpc_ctrl->mem_size) {
> - dev_err(dev, "Didn't find reserved memory\n");
> - return -EINVAL;
> + pr_err("aspeed_lpc_ctrl: ioctl: Didn't find reserved
memory\n");
Why did you change from dev_err() to pr_err()? You just lost a lot of
information that the user previously was getting from dev_err() :(
I did this as per review comment from Andrew Jeffery, as we don't want to put
this error for driver. It has to be handled by userspace. But I am still
reporting some error here.
> + return -ENXIO;
> }
>
> map.size = lpc_ctrl->mem_size;
> @@ -134,16 +133,16 @@ static long aspeed_lpc_ctrl_ioctl(struct file
*file, unsigned int cmd,
>
> if (map.window_type == ASPEED_LPC_CTRL_WINDOW_FLASH) {
> if (!lpc_ctrl->pnor_size) {
> - dev_err(dev, "Didn't find host pnor flash\n");
> - return -EINVAL;
> + pr_err("aspeed_lpc_ctrl: ioctl: Didn't find
host pnor flash\n");
Again, don't do that :(
thanks,
greg k-h
[PATCH 2/2] ARM: dts: aspeed: Add uarts for SoL in Facebook
Added uart2 and uart3 in Facebook Tiogapass for routing serial input
from Host to BMC for SoL via LPC.
Signed-off-by: Vijay Khemka
---
arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts | 10 ++
1 file changed, 10 insertions(+)
diff --git a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
index d60dbb019f82..42e0d7a8e8d0 100644
--- a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
@@ -72,6 +72,16 @@
_rxd1_default>;
};
+ {
+ // SoL Host Console
+ status = "okay";
+};
+
+ {
+ // SoL BMC Console
+ status = "okay";
+};
+
{
// BMC Console
status = "okay";
--
2.17.1
[PATCH 1/2] ARM: dts: aspeed: Add peci and lpc for Facebook
Added lpc control for enabling lpc clock, peci for cpu sensors and lpc
snoop devices to Facebook Tiogapass device tree.
Signed-off-by: Vijay Khemka
---
.../arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts | 14 ++
1 file changed, 14 insertions(+)
diff --git a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
index 73e58a821613..d60dbb019f82 100644
--- a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
@@ -54,6 +54,16 @@
};
};
+_snoop {
+ status = "okay";
+ snoop-ports = <0x80>;
+};
+
+_ctrl {
+ // Enable lpc clock
+ status = "okay";
+};
+
{
// Host Console
status = "okay";
@@ -67,6 +77,10 @@
status = "okay";
};
+ {
+ status = "okay";
+};
+
{
// BMC KCS channel 2
status = "okay";
--
2.17.1
[PATCH] misc: aspeed-lpc-ctrl: Correct return values
Corrected some of return values with appropriate meanings.
Signed-off-by: Vijay Khemka
---
drivers/misc/aspeed-lpc-ctrl.c | 15 +++
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/drivers/misc/aspeed-lpc-ctrl.c b/drivers/misc/aspeed-lpc-ctrl.c
index 332210e06e98..97ae341109d5 100644
--- a/drivers/misc/aspeed-lpc-ctrl.c
+++ b/drivers/misc/aspeed-lpc-ctrl.c
@@ -68,7 +68,6 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file, unsigned
int cmd,
unsigned long param)
{
struct aspeed_lpc_ctrl *lpc_ctrl = file_aspeed_lpc_ctrl(file);
- struct device *dev = file->private_data;
void __user *p = (void __user *)param;
struct aspeed_lpc_ctrl_mapping map;
u32 addr;
@@ -93,8 +92,8 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file, unsigned
int cmd,
/* If memory-region is not described in device tree */
if (!lpc_ctrl->mem_size) {
- dev_err(dev, "Didn't find reserved memory\n");
- return -EINVAL;
+ pr_err("aspeed_lpc_ctrl: ioctl: Didn't find reserved
memory\n");
+ return -ENXIO;
}
map.size = lpc_ctrl->mem_size;
@@ -134,16 +133,16 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file,
unsigned int cmd,
if (map.window_type == ASPEED_LPC_CTRL_WINDOW_FLASH) {
if (!lpc_ctrl->pnor_size) {
- dev_err(dev, "Didn't find host pnor flash\n");
- return -EINVAL;
+ pr_err("aspeed_lpc_ctrl: ioctl: Didn't find
host pnor flash\n");
+ return -ENXIO;
}
addr = lpc_ctrl->pnor_base;
size = lpc_ctrl->pnor_size;
} else if (map.window_type == ASPEED_LPC_CTRL_WINDOW_MEMORY) {
/* If memory-region is not described in device tree */
if (!lpc_ctrl->mem_size) {
- dev_err(dev, "Didn't find reserved memory\n");
- return -EINVAL;
+ pr_err("aspeed_lpc_ctrl: ioctl: Didn't find
reserved memory\n");
+ return -ENXIO;
}
addr = lpc_ctrl->mem_base;
size = lpc_ctrl->mem_size;
@@ -239,7 +238,7 @@ static int aspeed_lpc_ctrl_probe(struct platform_device
*pdev)
of_node_put(node);
if (rc) {
dev_err(dev, "Couldn't address to resource for reserved
memory\n");
- return -ENOMEM;
+ return -ENXIO;
}
lpc_ctrl->mem_size = resource_size();
--
2.17.1
[PATCH v2] dt-bindings: aspeed-lpc: Make parameter optional
Memory-region and flash phandle is not a required parameter, it is optional to describe in device tree and needed only use basis. Signed-off-by: Vijay Khemka --- Documentation/devicetree/bindings/mfd/aspeed-lpc.txt | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Documentation/devicetree/bindings/mfd/aspeed-lpc.txt b/Documentation/devicetree/bindings/mfd/aspeed-lpc.txt index 34dd89087cff..d28c4e117611 100644 --- a/Documentation/devicetree/bindings/mfd/aspeed-lpc.txt +++ b/Documentation/devicetree/bindings/mfd/aspeed-lpc.txt @@ -135,6 +135,9 @@ Required properties: - clocks: contains a phandle to the syscon node describing the clocks. There should then be one cell representing the clock to use +Optional properties: +These below properties are optional to define. + - memory-region: A phandle to a reserved_memory region to be used for the LPC to AHB mapping -- 2.17.1
Re: [PATCH v2] misc: aspeed-lpc-ctrl: make parameter optional
Hi Andrew,
Thanks for this review, I will have a follow up patch for this return values.
Regards
-Vijay
On 1/17/19, 8:58 PM, "Andrew Jeffery" wrote:
Hi Vijay,
Thanks for doing the work to fix the driver. Some minor queries/points
below.
On Thu, 17 Jan 2019, at 08:31, Vijay Khemka wrote:
> Makiing memory-region and flash as optional parameter in device
> tree if user needs to use these parameter through ioctl then
> need to define in devicetree.
>
> Signed-off-by: Vijay Khemka
> ---
> drivers/misc/aspeed-lpc-ctrl.c | 58 +-
> 1 file changed, 36 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/misc/aspeed-lpc-ctrl.c b/drivers/misc/aspeed-lpc-
> ctrl.c
> index a024f8042259..332210e06e98 100644
> --- a/drivers/misc/aspeed-lpc-ctrl.c
> +++ b/drivers/misc/aspeed-lpc-ctrl.c
> @@ -68,6 +68,7 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file,
> unsigned int cmd,
> unsigned long param)
> {
> struct aspeed_lpc_ctrl *lpc_ctrl = file_aspeed_lpc_ctrl(file);
> + struct device *dev = file->private_data;
> void __user *p = (void __user *)param;
> struct aspeed_lpc_ctrl_mapping map;
> u32 addr;
> @@ -90,6 +91,12 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file,
> unsigned int cmd,
> if (map.window_id != 0)
> return -EINVAL;
>
> + /* If memory-region is not described in device tree */
> + if (!lpc_ctrl->mem_size) {
> + dev_err(dev, "Didn't find reserved memory\n");
> + return -EINVAL;
I feel like EINVAL isn't quite right - it's pretty generic, and the
parameter
value changes its validity with the devicetree context. My gut instinct
would be to use EINVAL for parameter values that violate assumptions
of the driver rather than violate configuration of the driver. Maybe ENXIO
("No such device or address") is an improvement: "I can't map that device
because there's no such device or address"?
> + }
> +
> map.size = lpc_ctrl->mem_size;
>
> return copy_to_user(p, , sizeof(map)) ? -EFAULT : 0;
> @@ -126,9 +133,18 @@ static long aspeed_lpc_ctrl_ioctl(struct file
> *file, unsigned int cmd,
> return -EINVAL;
>
> if (map.window_type == ASPEED_LPC_CTRL_WINDOW_FLASH) {
> + if (!lpc_ctrl->pnor_size) {
> + dev_err(dev, "Didn't find host pnor flash\n");
> + return -EINVAL;
See the error code discussion above. Also, this is userspace's error not
the kernel's, so I think dev_err() is a bit harsh. Probably best to just let
userspace log the error if it thinks the it is concerning.
> + }
> addr = lpc_ctrl->pnor_base;
> size = lpc_ctrl->pnor_size;
> } else if (map.window_type == ASPEED_LPC_CTRL_WINDOW_MEMORY) {
> + /* If memory-region is not described in device tree */
> + if (!lpc_ctrl->mem_size) {
> + dev_err(dev, "Didn't find reserved memory\n");
> + return -EINVAL;
as above.
> + }
> addr = lpc_ctrl->mem_base;
> size = lpc_ctrl->mem_size;
> } else {
> @@ -196,17 +212,17 @@ static int aspeed_lpc_ctrl_probe(struct
> platform_device *pdev)
> if (!lpc_ctrl)
> return -ENOMEM;
>
> + /* If flash is described in device tree then store */
> node = of_parse_phandle(dev->of_node, "flash", 0);
> if (!node) {
> - dev_err(dev, "Didn't find host pnor flash node\n");
> - return -ENODEV;
> - }
> -
> - rc = of_address_to_resource(node, 1, );
> - of_node_put(node);
> - if (rc) {
> - dev_err(dev, "Couldn't address to resource for flash\n");
> - return rc;
> + dev_dbg(dev, "Didn't find host pnor flash node\n");
> + } else {
> + rc = of_address_to_resource(node, 1, );
> + of_node_put(node);
> + if (rc) {
> + dev_err(dev, "Couldn't address to resource for
flash\n");
> + return rc;
> + }
> }
>
> lpc_ctrl->pnor_size = resource_size();
[PATCH] dt-bindings: aspeed-lpc: Make parameter optional
Memory-region and flash phandle is not a required parameter, it is optional to describe in device tree and needed only use basis. Signed-off-by: Vijay Khemka --- Documentation/devicetree/bindings/mfd/aspeed-lpc.txt | 4 1 file changed, 4 insertions(+) diff --git a/Documentation/devicetree/bindings/mfd/aspeed-lpc.txt b/Documentation/devicetree/bindings/mfd/aspeed-lpc.txt index 34dd89087cff..ff0cb28903dd 100644 --- a/Documentation/devicetree/bindings/mfd/aspeed-lpc.txt +++ b/Documentation/devicetree/bindings/mfd/aspeed-lpc.txt @@ -135,6 +135,10 @@ Required properties: - clocks: contains a phandle to the syscon node describing the clocks. There should then be one cell representing the clock to use +Optional properties: +These below properties are optional and required only if one need to use it +through ioctl. + - memory-region: A phandle to a reserved_memory region to be used for the LPC to AHB mapping -- 2.17.1
Re: [PATCH v2 4/4] ARM: dts: aspeed: Add lpc ctrl for Facebook
On 1/16/19, 6:49 PM, "Joel Stanley" wrote:
On Fri, 21 Dec 2018 at 05:06, Vijay Khemka wrote:
>
> On 12/17/18, 12:04 PM, "Vijay Khemka" wrote:
>
> Added lpc ctrl device to enable LPC clock in Facebook
> Tiogapass device tree.
>
> Signed-off-by: Vijay Khemka
>
> Joel, Can you please take care of these patches merge.
I did not apply this one as I think the description will change once
we've merged your aspeed-lpc-ctrl patch.
Yes I will update and send new one.
Cheers,
Joel
Re: [PATCH v2] misc: aspeed-lpc-ctrl: make parameter optional
On 1/16/19, 10:17 PM, "Joel Stanley" wrote:
On Thu, 17 Jan 2019 at 09:02, Vijay Khemka wrote:
>
> Makiing memory-region and flash as optional parameter in device
> tree if user needs to use these parameter through ioctl then
> need to define in devicetree.
>
> Signed-off-by: Vijay Khemka
Thanks! This looks okay to me. I tested it on one of our systems which
uses both flash and reserved memory and it was fine.
Reviewed-by: Joel Stanley
Can you also send a patch to update the bindings at
Documentation/devicetree/bindings/mfd/aspeed-lpc.txt ? I think the
only change you need to make is to move the memory region and flash
properties to optional (instead of required).
Sure I will do this.
Cheers,
Joel
> ---
> drivers/misc/aspeed-lpc-ctrl.c | 58 +-
> 1 file changed, 36 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/misc/aspeed-lpc-ctrl.c
b/drivers/misc/aspeed-lpc-ctrl.c
> index a024f8042259..332210e06e98 100644
> --- a/drivers/misc/aspeed-lpc-ctrl.c
> +++ b/drivers/misc/aspeed-lpc-ctrl.c
> @@ -68,6 +68,7 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file,
unsigned int cmd,
> unsigned long param)
> {
> struct aspeed_lpc_ctrl *lpc_ctrl = file_aspeed_lpc_ctrl(file);
> + struct device *dev = file->private_data;
> void __user *p = (void __user *)param;
> struct aspeed_lpc_ctrl_mapping map;
> u32 addr;
> @@ -90,6 +91,12 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file,
unsigned int cmd,
> if (map.window_id != 0)
> return -EINVAL;
>
> + /* If memory-region is not described in device tree */
> + if (!lpc_ctrl->mem_size) {
> + dev_err(dev, "Didn't find reserved memory\n");
> + return -EINVAL;
> + }
> +
> map.size = lpc_ctrl->mem_size;
>
> return copy_to_user(p, , sizeof(map)) ? -EFAULT : 0;
> @@ -126,9 +133,18 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file,
unsigned int cmd,
> return -EINVAL;
>
> if (map.window_type == ASPEED_LPC_CTRL_WINDOW_FLASH) {
> + if (!lpc_ctrl->pnor_size) {
> + dev_err(dev, "Didn't find host pnor
flash\n");
> + return -EINVAL;
> + }
> addr = lpc_ctrl->pnor_base;
> size = lpc_ctrl->pnor_size;
> } else if (map.window_type ==
ASPEED_LPC_CTRL_WINDOW_MEMORY) {
> + /* If memory-region is not described in device
tree */
> + if (!lpc_ctrl->mem_size) {
> + dev_err(dev, "Didn't find reserved
memory\n");
> + return -EINVAL;
> + }
> addr = lpc_ctrl->mem_base;
> size = lpc_ctrl->mem_size;
> } else {
> @@ -196,17 +212,17 @@ static int aspeed_lpc_ctrl_probe(struct
platform_device *pdev)
> if (!lpc_ctrl)
> return -ENOMEM;
>
> + /* If flash is described in device tree then store */
> node = of_parse_phandle(dev->of_node, "flash", 0);
> if (!node) {
> - dev_err(dev, "Didn't find host pnor flash node\n");
> - return -ENODEV;
> - }
> -
> - rc = of_address_to_resource(node, 1, );
> - of_node_put(node);
> - if (rc) {
> - dev_err(dev, "Couldn't address to resource for flash\n");
> - return rc;
> + dev_dbg(dev, "Didn't find host pnor flash node\n");
> + } else {
> + rc = of_address_to_resource(node, 1, );
> + of_node_put(node);
> + if (rc) {
> + dev_err(dev, "Couldn't address to resource for
flash\n");
> + return rc;
> + }
> }
>
> lpc_ctrl->pnor_size = resource_size();
> @@ -214,22 +230,22 @@ static int aspeed_lpc_ctrl_probe(struct
platform_device *pdev)
>
> dev_set_drvdata(>dev, lpc_ctrl);
>
>
[PATCH v2] misc: aspeed-lpc-ctrl: make parameter optional
Makiing memory-region and flash as optional parameter in device
tree if user needs to use these parameter through ioctl then
need to define in devicetree.
Signed-off-by: Vijay Khemka
---
drivers/misc/aspeed-lpc-ctrl.c | 58 +-
1 file changed, 36 insertions(+), 22 deletions(-)
diff --git a/drivers/misc/aspeed-lpc-ctrl.c b/drivers/misc/aspeed-lpc-ctrl.c
index a024f8042259..332210e06e98 100644
--- a/drivers/misc/aspeed-lpc-ctrl.c
+++ b/drivers/misc/aspeed-lpc-ctrl.c
@@ -68,6 +68,7 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file, unsigned
int cmd,
unsigned long param)
{
struct aspeed_lpc_ctrl *lpc_ctrl = file_aspeed_lpc_ctrl(file);
+ struct device *dev = file->private_data;
void __user *p = (void __user *)param;
struct aspeed_lpc_ctrl_mapping map;
u32 addr;
@@ -90,6 +91,12 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file,
unsigned int cmd,
if (map.window_id != 0)
return -EINVAL;
+ /* If memory-region is not described in device tree */
+ if (!lpc_ctrl->mem_size) {
+ dev_err(dev, "Didn't find reserved memory\n");
+ return -EINVAL;
+ }
+
map.size = lpc_ctrl->mem_size;
return copy_to_user(p, , sizeof(map)) ? -EFAULT : 0;
@@ -126,9 +133,18 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file,
unsigned int cmd,
return -EINVAL;
if (map.window_type == ASPEED_LPC_CTRL_WINDOW_FLASH) {
+ if (!lpc_ctrl->pnor_size) {
+ dev_err(dev, "Didn't find host pnor flash\n");
+ return -EINVAL;
+ }
addr = lpc_ctrl->pnor_base;
size = lpc_ctrl->pnor_size;
} else if (map.window_type == ASPEED_LPC_CTRL_WINDOW_MEMORY) {
+ /* If memory-region is not described in device tree */
+ if (!lpc_ctrl->mem_size) {
+ dev_err(dev, "Didn't find reserved memory\n");
+ return -EINVAL;
+ }
addr = lpc_ctrl->mem_base;
size = lpc_ctrl->mem_size;
} else {
@@ -196,17 +212,17 @@ static int aspeed_lpc_ctrl_probe(struct platform_device
*pdev)
if (!lpc_ctrl)
return -ENOMEM;
+ /* If flash is described in device tree then store */
node = of_parse_phandle(dev->of_node, "flash", 0);
if (!node) {
- dev_err(dev, "Didn't find host pnor flash node\n");
- return -ENODEV;
- }
-
- rc = of_address_to_resource(node, 1, );
- of_node_put(node);
- if (rc) {
- dev_err(dev, "Couldn't address to resource for flash\n");
- return rc;
+ dev_dbg(dev, "Didn't find host pnor flash node\n");
+ } else {
+ rc = of_address_to_resource(node, 1, );
+ of_node_put(node);
+ if (rc) {
+ dev_err(dev, "Couldn't address to resource for
flash\n");
+ return rc;
+ }
}
lpc_ctrl->pnor_size = resource_size();
@@ -214,22 +230,22 @@ static int aspeed_lpc_ctrl_probe(struct platform_device
*pdev)
dev_set_drvdata(>dev, lpc_ctrl);
+ /* If memory-region is described in device tree then store */
node = of_parse_phandle(dev->of_node, "memory-region", 0);
if (!node) {
- dev_err(dev, "Didn't find reserved memory\n");
- return -EINVAL;
- }
+ dev_dbg(dev, "Didn't find reserved memory\n");
+ } else {
+ rc = of_address_to_resource(node, 0, );
+ of_node_put(node);
+ if (rc) {
+ dev_err(dev, "Couldn't address to resource for reserved
memory\n");
+ return -ENOMEM;
+ }
- rc = of_address_to_resource(node, 0, );
- of_node_put(node);
- if (rc) {
- dev_err(dev, "Couldn't address to resource for reserved
memory\n");
- return -ENOMEM;
+ lpc_ctrl->mem_size = resource_size();
+ lpc_ctrl->mem_base = resm.start;
}
- lpc_ctrl->mem_size = resource_size();
- lpc_ctrl->mem_base = resm.start;
-
lpc_ctrl->regmap = syscon_node_to_regmap(
pdev->dev.parent->of_node);
if (IS_ERR(lpc_ctrl->regmap)) {
@@ -258,8 +274,6 @@ static int aspeed_lpc_ctrl_probe(struct platform_device
*pdev)
goto err;
}
- dev_info(dev, "Loaded at %pr\n", );
-
return 0;
err:
--
2.17.1
Re: [PATCH] misc: aspeed-lpc-ctrl: make memory-region optional
Sure Joel,
I will do v2 and submit for review today.
Regards
-Vijay
On 1/15/19, 4:11 PM, "Joel Stanley" wrote:
Hi Vijay,
On Tue, 15 Jan 2019 at 12:51, Vijay Khemka wrote:
>
> Makiing memory-region as optional parameter in device tree if
> user needs to use memory-region then define in devicetree.
Thank you for finding the time to do this work. You're not the first
to be blocked by this limitation, but you are the first who found the
time to propose a patch.
I think we should complete the rework to make the flash device
optional too. Can you do a v2 with a proposal for that?
>
> Signed-off-by: Vijay Khemka
> ---
> drivers/misc/aspeed-lpc-ctrl.c | 30 ++
> 1 file changed, 18 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/misc/aspeed-lpc-ctrl.c
b/drivers/misc/aspeed-lpc-ctrl.c
> index a024f8042259..20507f0764fb 100644
> --- a/drivers/misc/aspeed-lpc-ctrl.c
> +++ b/drivers/misc/aspeed-lpc-ctrl.c
> @@ -90,6 +90,10 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file,
unsigned int cmd,
> if (map.window_id != 0)
> return -EINVAL;
>
> + /* If memory-region is not described in device tree */
> + if (!lpc_ctrl->mem_size)
> + return -EINVAL;
> +
> map.size = lpc_ctrl->mem_size;
>
> return copy_to_user(p, , sizeof(map)) ? -EFAULT : 0;
> @@ -129,6 +133,10 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file,
unsigned int cmd,
> addr = lpc_ctrl->pnor_base;
> size = lpc_ctrl->pnor_size;
> } else if (map.window_type ==
ASPEED_LPC_CTRL_WINDOW_MEMORY) {
> + /* If memory-region is not described in device
tree */
> + if (!lpc_ctrl->mem_size)
Should it print a message here? I think a dev_dbg makes sense.
> + return -EINVAL;
> +
> addr = lpc_ctrl->mem_base;
> size = lpc_ctrl->mem_size;
> } else {
> @@ -214,22 +222,20 @@ static int aspeed_lpc_ctrl_probe(struct
platform_device *pdev)
>
> dev_set_drvdata(>dev, lpc_ctrl);
>
> + /* If memory-region is described in device tree then store */
> node = of_parse_phandle(dev->of_node, "memory-region", 0);
> - if (!node) {
> - dev_err(dev, "Didn't find reserved memory\n");
> - return -EINVAL;
> - }
> + if (node) {
Print a debug message when we didn't find the node.
> + rc = of_address_to_resource(node, 0, );
> + of_node_put(node);
> + if (rc) {
> + dev_err(dev, "Couldn't address to resource for
reserved memory\n");
> + return -ENOMEM;
> + }
>
> - rc = of_address_to_resource(node, 0, );
> - of_node_put(node);
> - if (rc) {
> - dev_err(dev, "Couldn't address to resource for reserved
memory\n");
> - return -ENOMEM;
> + lpc_ctrl->mem_size = resource_size();
> + lpc_ctrl->mem_base = resm.start;
> }
>
> - lpc_ctrl->mem_size = resource_size();
> - lpc_ctrl->mem_base = resm.start;
> -
> lpc_ctrl->regmap = syscon_node_to_regmap(
> pdev->dev.parent->of_node);
> if (IS_ERR(lpc_ctrl->regmap)) {
Down below here is this line:
>dev_info(dev, "Loaded at %pr\n", );
As resm is first used for the flash resource and then the memory
region, it will display the location of the flash when the memory
region is not present. This is confusing. You should either print out
the regions separately, or consider removing this dev_info completely.
Yes it did confuse me too and I also thought of removing it. I was waiting for
your opinion __
> --
> 2.17.1
>
[PATCH] misc: aspeed-lpc-ctrl: make memory-region optional
Makiing memory-region as optional parameter in device tree if
user needs to use memory-region then define in devicetree.
Signed-off-by: Vijay Khemka
---
drivers/misc/aspeed-lpc-ctrl.c | 30 ++
1 file changed, 18 insertions(+), 12 deletions(-)
diff --git a/drivers/misc/aspeed-lpc-ctrl.c b/drivers/misc/aspeed-lpc-ctrl.c
index a024f8042259..20507f0764fb 100644
--- a/drivers/misc/aspeed-lpc-ctrl.c
+++ b/drivers/misc/aspeed-lpc-ctrl.c
@@ -90,6 +90,10 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file,
unsigned int cmd,
if (map.window_id != 0)
return -EINVAL;
+ /* If memory-region is not described in device tree */
+ if (!lpc_ctrl->mem_size)
+ return -EINVAL;
+
map.size = lpc_ctrl->mem_size;
return copy_to_user(p, , sizeof(map)) ? -EFAULT : 0;
@@ -129,6 +133,10 @@ static long aspeed_lpc_ctrl_ioctl(struct file *file,
unsigned int cmd,
addr = lpc_ctrl->pnor_base;
size = lpc_ctrl->pnor_size;
} else if (map.window_type == ASPEED_LPC_CTRL_WINDOW_MEMORY) {
+ /* If memory-region is not described in device tree */
+ if (!lpc_ctrl->mem_size)
+ return -EINVAL;
+
addr = lpc_ctrl->mem_base;
size = lpc_ctrl->mem_size;
} else {
@@ -214,22 +222,20 @@ static int aspeed_lpc_ctrl_probe(struct platform_device
*pdev)
dev_set_drvdata(>dev, lpc_ctrl);
+ /* If memory-region is described in device tree then store */
node = of_parse_phandle(dev->of_node, "memory-region", 0);
- if (!node) {
- dev_err(dev, "Didn't find reserved memory\n");
- return -EINVAL;
- }
+ if (node) {
+ rc = of_address_to_resource(node, 0, );
+ of_node_put(node);
+ if (rc) {
+ dev_err(dev, "Couldn't address to resource for reserved
memory\n");
+ return -ENOMEM;
+ }
- rc = of_address_to_resource(node, 0, );
- of_node_put(node);
- if (rc) {
- dev_err(dev, "Couldn't address to resource for reserved
memory\n");
- return -ENOMEM;
+ lpc_ctrl->mem_size = resource_size();
+ lpc_ctrl->mem_base = resm.start;
}
- lpc_ctrl->mem_size = resource_size();
- lpc_ctrl->mem_base = resm.start;
-
lpc_ctrl->regmap = syscon_node_to_regmap(
pdev->dev.parent->of_node);
if (IS_ERR(lpc_ctrl->regmap)) {
--
2.17.1
Re: [Potential Spoof] Re: [Potential Spoof] Re: [PATCH v2 4/4] ARM: dts: aspeed: Add lpc ctrl for Facebook
On 1/13/19, 2:39 PM, "Andrew Jeffery" wrote:
Hi Vijay,
Sorry for providing an opinion so late, however:
On Sat, 12 Jan 2019, at 11:03, Vijay Khemka wrote:
> Joel,
> Please merge these patches as it is required by facebook platform.
>
> Regards
> -Vijay
>
> On 1/7/19, 11:25 AM, "Linux-aspeed on behalf of Vijay Khemka" aspeed-bounces+vijaykhemka=fb@lists.ozlabs.org on behalf of
> vijaykhe...@fb.com> wrote:
>
> Please merge these patches in upstream kernel.
>
> Regards
> -Vijay
>
> On 12/20/18, 10:06 AM, "Linux-aspeed on behalf of Vijay Khemka"
> vijaykhe...@fb.com> wrote:
>
> Joel, Can you please take care of these patches merge.
>
> On 12/17/18, 12:04 PM, "Vijay Khemka" wrote:
>
> Added lpc ctrl device to enable LPC clock in Facebook
> Tiogapass device tree.
>
> Signed-off-by: Vijay Khemka
> ---
> .../boot/dts/aspeed-bmc-facebook-tiogapass.dts | 17 ++
> +++
> 1 file changed, 17 insertions(+)
>
> diff --git a/arch/arm/boot/dts/aspeed-bmc-facebook-
> tiogapass.dts b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
> index 73e58a821613..ef7875b54562 100644
> --- a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
> +++ b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
> @@ -22,6 +22,17 @@
> reg = <0x8000 0x2000>;
> };
>
> + reserved-memory {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges;
> +
> + flash_memory: region@9800 {
> + no-map;
> + reg = <0x9800 0x1000>; /* 4K */
> + };
> + };
> +
> iio-hwmon {
> compatible = "iio-hwmon";
> io-channels = < 0>, < 1>, < 2>,
< 3>,
> @@ -54,6 +65,12 @@
> };
> };
>
> +_ctrl {
> + status = "okay";
> + memory-region = <_memory>;
> + flash = <>;
> +};
> +
> {
> // Host Console
> status = "okay";
> --
> 2.17.1
I understand you just want to make your system work, but this is a
broken way to do it. And that's not your fault - we should really fix
the driver. I think the memory-region phandle should be optional
and the driver should simply limit the options available to the ioctl
to just the flash region if the memory node is not present (i.e. return
an error if the memory region is requested but not described in the
devicetree).
Let me fix driver itself first.
Andrew
Re: [Potential Spoof] Re: [Potential Spoof] Re: [PATCH v2 4/4] ARM: dts: aspeed: Add lpc ctrl for Facebook
Joel,
Please merge these patches as it is required by facebook platform.
Regards
-Vijay
On 1/7/19, 11:25 AM, "Linux-aspeed on behalf of Vijay Khemka"
wrote:
Please merge these patches in upstream kernel.
Regards
-Vijay
On 12/20/18, 10:06 AM, "Linux-aspeed on behalf of Vijay Khemka"
wrote:
Joel, Can you please take care of these patches merge.
On 12/17/18, 12:04 PM, "Vijay Khemka" wrote:
Added lpc ctrl device to enable LPC clock in Facebook
Tiogapass device tree.
Signed-off-by: Vijay Khemka
---
.../boot/dts/aspeed-bmc-facebook-tiogapass.dts | 17
+
1 file changed, 17 insertions(+)
diff --git a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
index 73e58a821613..ef7875b54562 100644
--- a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
@@ -22,6 +22,17 @@
reg = <0x8000 0x2000>;
};
+ reserved-memory {
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges;
+
+ flash_memory: region@9800 {
+ no-map;
+ reg = <0x9800 0x1000>; /* 4K */
+ };
+ };
+
iio-hwmon {
compatible = "iio-hwmon";
io-channels = < 0>, < 1>, < 2>, < 3>,
@@ -54,6 +65,12 @@
};
};
+_ctrl {
+ status = "okay";
+ memory-region = <_memory>;
+ flash = <>;
+};
+
{
// Host Console
status = "okay";
--
2.17.1
Re: [Potential Spoof] Re: [PATCH v2 4/4] ARM: dts: aspeed: Add lpc ctrl for Facebook
Please merge these patches in upstream kernel.
Regards
-Vijay
On 12/20/18, 10:06 AM, "Linux-aspeed on behalf of Vijay Khemka"
wrote:
Joel, Can you please take care of these patches merge.
On 12/17/18, 12:04 PM, "Vijay Khemka" wrote:
Added lpc ctrl device to enable LPC clock in Facebook
Tiogapass device tree.
Signed-off-by: Vijay Khemka
---
.../boot/dts/aspeed-bmc-facebook-tiogapass.dts | 17 +
1 file changed, 17 insertions(+)
diff --git a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
index 73e58a821613..ef7875b54562 100644
--- a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
@@ -22,6 +22,17 @@
reg = <0x8000 0x2000>;
};
+ reserved-memory {
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges;
+
+ flash_memory: region@9800 {
+ no-map;
+ reg = <0x9800 0x1000>; /* 4K */
+ };
+ };
+
iio-hwmon {
compatible = "iio-hwmon";
io-channels = < 0>, < 1>, < 2>, < 3>,
@@ -54,6 +65,12 @@
};
};
+_ctrl {
+ status = "okay";
+ memory-region = <_memory>;
+ flash = <>;
+};
+
{
// Host Console
status = "okay";
--
2.17.1
Re: [PATCH v2 4/4] ARM: dts: aspeed: Add lpc ctrl for Facebook
Joel, Can you please take care of these patches merge.
On 12/17/18, 12:04 PM, "Vijay Khemka" wrote:
Added lpc ctrl device to enable LPC clock in Facebook
Tiogapass device tree.
Signed-off-by: Vijay Khemka
---
.../boot/dts/aspeed-bmc-facebook-tiogapass.dts | 17 +
1 file changed, 17 insertions(+)
diff --git a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
index 73e58a821613..ef7875b54562 100644
--- a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
@@ -22,6 +22,17 @@
reg = <0x8000 0x2000>;
};
+ reserved-memory {
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges;
+
+ flash_memory: region@9800 {
+ no-map;
+ reg = <0x9800 0x1000>; /* 4K */
+ };
+ };
+
iio-hwmon {
compatible = "iio-hwmon";
io-channels = < 0>, < 1>, < 2>, < 3>,
@@ -54,6 +65,12 @@
};
};
+_ctrl {
+ status = "okay";
+ memory-region = <_memory>;
+ flash = <>;
+};
+
{
// Host Console
status = "okay";
--
2.17.1
[PATCH v2 3/4] ARM: dts: aspeed: Add KCS for Facebook
Added kcs device in Facebook Tiogapass device tree.
Signed-off-by: Vijay Khemka
---
arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts | 12
1 file changed, 12 insertions(+)
diff --git a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
index 64039853..73e58a821613 100644
--- a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
@@ -67,6 +67,18 @@
status = "okay";
};
+ {
+ // BMC KCS channel 2
+ status = "okay";
+ kcs_addr = <0xca8>;
+};
+
+ {
+ // BMC KCS channel 3
+ status = "okay";
+ kcs_addr = <0xca2>;
+};
+
{
status = "okay";
--
2.17.1
[PATCH v2 4/4] ARM: dts: aspeed: Add lpc ctrl for Facebook
Added lpc ctrl device to enable LPC clock in Facebook
Tiogapass device tree.
Signed-off-by: Vijay Khemka
---
.../boot/dts/aspeed-bmc-facebook-tiogapass.dts | 17 +
1 file changed, 17 insertions(+)
diff --git a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
index 73e58a821613..ef7875b54562 100644
--- a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
@@ -22,6 +22,17 @@
reg = <0x8000 0x2000>;
};
+ reserved-memory {
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges;
+
+ flash_memory: region@9800 {
+ no-map;
+ reg = <0x9800 0x1000>; /* 4K */
+ };
+ };
+
iio-hwmon {
compatible = "iio-hwmon";
io-channels = < 0>, < 1>, < 2>, < 3>,
@@ -54,6 +65,12 @@
};
};
+_ctrl {
+ status = "okay";
+ memory-region = <_memory>;
+ flash = <>;
+};
+
{
// Host Console
status = "okay";
--
2.17.1
[PATCH v2 1/4] ARM: dts: aspeed: Add sensors devices for Facebook
Added ADC and other sensor devices in Facebook Tiogapass device tree.
Signed-off-by: Vijay Khemka
---
.../dts/aspeed-bmc-facebook-tiogapass.dts | 23 +--
1 file changed, 21 insertions(+), 2 deletions(-)
diff --git a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
index f8e7b71af7e6..64039853 100644
--- a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
@@ -21,6 +21,17 @@
memory@8000 {
reg = <0x8000 0x2000>;
};
+
+ iio-hwmon {
+ compatible = "iio-hwmon";
+ io-channels = < 0>, < 1>, < 2>, < 3>,
+ < 4>, < 5>, < 6>;
+ };
+
+ iio-hwmon-battery {
+ compatible = "iio-hwmon";
+ io-channels = < 7>;
+ };
};
{
@@ -64,6 +75,10 @@
use-ncsi;
};
+ {
+ status = "okay";
+};
+
{
status = "okay";
//Airmax Conn B, CPU0 PIROM, CPU1 PIROM
@@ -122,6 +137,10 @@
{
status = "okay";
+ tmp421@1f {
+ compatible = "ti,tmp421";
+ reg = <0x1f>;
+ };
//Mezz Sensor SMBus
};
@@ -140,7 +159,7 @@
};
fan@1 {
- reg = <0x00>;
- aspeed,fan-tach-ch = /bits/ 8 <0x01>;
+ reg = <0x01>;
+ aspeed,fan-tach-ch = /bits/ 8 <0x02>;
};
};
--
2.17.1
[PATCH v2 2/4] ARM: dts: aspeed: Add KCS support for LPC BMC
Added kcs device support for lpc BMC.
Signed-off-by: Vijay Khemka
---
arch/arm/boot/dts/aspeed-g5.dtsi | 33 +++-
1 file changed, 32 insertions(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi b/arch/arm/boot/dts/aspeed-g5.dtsi
index d107459fc0f8..2743f400aa29 100644
--- a/arch/arm/boot/dts/aspeed-g5.dtsi
+++ b/arch/arm/boot/dts/aspeed-g5.dtsi
@@ -330,8 +330,32 @@
ranges = <0x0 0x1e789000 0x1000>;
lpc_bmc: lpc-bmc@0 {
- compatible = "aspeed,ast2500-lpc-bmc";
+ compatible = "aspeed,ast2500-lpc-bmc",
"simple-mfd", "syscon";
reg = <0x0 0x80>;
+ reg-io-width = <4>;
+
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges = <0x0 0x0 0x80>;
+
+ kcs1: kcs1@0 {
+ compatible =
"aspeed,ast2500-kcs-bmc";
+ interrupts = <8>;
+ kcs_chan = <1>;
+ status = "disabled";
+ };
+ kcs2: kcs2@0 {
+ compatible =
"aspeed,ast2500-kcs-bmc";
+ interrupts = <8>;
+ kcs_chan = <2>;
+ status = "disabled";
+ };
+ kcs3: kcs3@0 {
+ compatible =
"aspeed,ast2500-kcs-bmc";
+ interrupts = <8>;
+ kcs_chan = <3>;
+ status = "disabled";
+ };
};
lpc_host: lpc-host@80 {
@@ -343,6 +367,13 @@
#size-cells = <1>;
ranges = <0x0 0x80 0x1e0>;
+ kcs4: kcs4@0 {
+ compatible =
"aspeed,ast2500-kcs-bmc";
+ interrupts = <8>;
+ kcs_chan = <4>;
+ status = "disabled";
+ };
+
lpc_ctrl: lpc-ctrl@0 {
compatible =
"aspeed,ast2500-lpc-ctrl";
reg = <0x0 0x80>;
--
2.17.1
Re: [Potential Spoof] Re: [PATCH 1/4] ARM: dts: aspeed: Add sensors devices for Facebook
On 12/14/18, 12:42 PM, "Linux-aspeed on behalf of Vijay Khemka" wrote: On 12/14/18, 11:22 AM, "openbmc on behalf of Jae Hyun Yoo" wrote: Hi Vijay, On 12/14/2018 10:11 AM, Vijay Khemka wrote: > On 12/13/18, 2:56 PM, "Joel Stanley" wrote: > > + oemname0 = "MB_P3V3"; > > + oemname1 = "MB_P5V"; > > + oemname2 = "MB_P12V"; > > + oemname3 = "MB_P1V05"; > > + oemname4 = "MB_PVNN_PCH_STBY"; > > + oemname5 = "MB_P3V3_STBY"; > > + oemname6 = "MB_P5V_STBY"; > > "oemname" isn't part of the upstream bindings. Is this something you > have patches for? > This is a workaround field used by dbus-sensors application to avoid overlay for dynamic detection of devices based on json file definition. > > Can you please also review other 3 patches in this series. > > These oemname settings should not be added into here. You can add these information into configuration of entity manager which uses overlay templates for dbus-sensors. Also, as Joel said, "oemname" isn't part of the upstream bindings. Overlay templates from entity manager is not working for fan and adc sensors that's why it is a workaround suggested by James Feist (Author of dbus-sensors). @james.fe...@linux.intel.com, Please comment on above patch, please advise if I need to fix this. Cheers, Jae
Re: [PATCH 1/4] ARM: dts: aspeed: Add sensors devices for Facebook
On 12/14/18, 11:22 AM, "openbmc on behalf of Jae Hyun Yoo" wrote: Hi Vijay, On 12/14/2018 10:11 AM, Vijay Khemka wrote: > On 12/13/18, 2:56 PM, "Joel Stanley" wrote: > > + oemname0 = "MB_P3V3"; > > + oemname1 = "MB_P5V"; > > + oemname2 = "MB_P12V"; > > + oemname3 = "MB_P1V05"; > > + oemname4 = "MB_PVNN_PCH_STBY"; > > + oemname5 = "MB_P3V3_STBY"; > > + oemname6 = "MB_P5V_STBY"; > > "oemname" isn't part of the upstream bindings. Is this something you > have patches for? > This is a workaround field used by dbus-sensors application to avoid overlay for dynamic detection of devices based on json file definition. > > Can you please also review other 3 patches in this series. > > These oemname settings should not be added into here. You can add these information into configuration of entity manager which uses overlay templates for dbus-sensors. Also, as Joel said, "oemname" isn't part of the upstream bindings. Overlay templates from entity manager is not working for fan and adc sensors that's why it is a workaround suggested by James Feist (Author of dbus-sensors). Cheers, Jae
Re: [PATCH 1/4] ARM: dts: aspeed: Add sensors devices for Facebook
On 12/13/18, 2:56 PM, "Joel Stanley" wrote:
On Fri, 14 Dec 2018 at 06:23, Vijay Khemka wrote:
>
> Added ADC and other sensor devices in Facebook Tiogapass device tree.
>
> Signed-off-by: Vijay Khemka
> ---
> .../dts/aspeed-bmc-facebook-tiogapass.dts | 33 +--
> 1 file changed, 31 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
> index f8e7b71af7e6..58bbe08d3ba7 100644
> --- a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
> +++ b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
> @@ -21,6 +21,25 @@
> memory@8000 {
> reg = <0x8000 0x2000>;
> };
> +
> + iio-hwmon {
> + compatible = "iio-hwmon";
> + oemname0 = "MB_P3V3";
> + oemname1 = "MB_P5V";
> + oemname2 = "MB_P12V";
> + oemname3 = "MB_P1V05";
> + oemname4 = "MB_PVNN_PCH_STBY";
> + oemname5 = "MB_P3V3_STBY";
> + oemname6 = "MB_P5V_STBY";
"oemname" isn't part of the upstream bindings. Is this something you
have patches for?
This is a workaround field used by dbus-sensors application to avoid overlay
for dynamic detection of devices based on json file definition.
Can you please also review other 3 patches in this series.
[PATCH 2/4] ARM: dts: aspeed: Add KCS support for LPC BMC
Added kcs device support for lpc BMC.
Signed-off-by: Vijay Khemka
---
arch/arm/boot/dts/aspeed-g5.dtsi | 33 +++-
1 file changed, 32 insertions(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi b/arch/arm/boot/dts/aspeed-g5.dtsi
index d107459fc0f8..2743f400aa29 100644
--- a/arch/arm/boot/dts/aspeed-g5.dtsi
+++ b/arch/arm/boot/dts/aspeed-g5.dtsi
@@ -330,8 +330,32 @@
ranges = <0x0 0x1e789000 0x1000>;
lpc_bmc: lpc-bmc@0 {
- compatible = "aspeed,ast2500-lpc-bmc";
+ compatible = "aspeed,ast2500-lpc-bmc",
"simple-mfd", "syscon";
reg = <0x0 0x80>;
+ reg-io-width = <4>;
+
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges = <0x0 0x0 0x80>;
+
+ kcs1: kcs1@0 {
+ compatible =
"aspeed,ast2500-kcs-bmc";
+ interrupts = <8>;
+ kcs_chan = <1>;
+ status = "disabled";
+ };
+ kcs2: kcs2@0 {
+ compatible =
"aspeed,ast2500-kcs-bmc";
+ interrupts = <8>;
+ kcs_chan = <2>;
+ status = "disabled";
+ };
+ kcs3: kcs3@0 {
+ compatible =
"aspeed,ast2500-kcs-bmc";
+ interrupts = <8>;
+ kcs_chan = <3>;
+ status = "disabled";
+ };
};
lpc_host: lpc-host@80 {
@@ -343,6 +367,13 @@
#size-cells = <1>;
ranges = <0x0 0x80 0x1e0>;
+ kcs4: kcs4@0 {
+ compatible =
"aspeed,ast2500-kcs-bmc";
+ interrupts = <8>;
+ kcs_chan = <4>;
+ status = "disabled";
+ };
+
lpc_ctrl: lpc-ctrl@0 {
compatible =
"aspeed,ast2500-lpc-ctrl";
reg = <0x0 0x80>;
--
2.17.1
[PATCH 4/4] ARM: dts: aspeed: Add lpc ctrl for Facebook
Added lpc ctrl device to enable LPC clock in Facebook
Tiogapass device tree.
Signed-off-by: Vijay Khemka
---
.../boot/dts/aspeed-bmc-facebook-tiogapass.dts | 17 +
1 file changed, 17 insertions(+)
diff --git a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
index b99751b3e080..084110007f5d 100644
--- a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
@@ -22,6 +22,17 @@
reg = <0x8000 0x2000>;
};
+ reserved-memory {
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges;
+
+ flash_memory: region@9800 {
+ no-map;
+ reg = <0x9800 0x1000>; /* 4K */
+ };
+ };
+
iio-hwmon {
compatible = "iio-hwmon";
oemname0 = "MB_P3V3";
@@ -62,6 +73,12 @@
};
};
+_ctrl {
+ status = "okay";
+ memory-region = <_memory>;
+ flash = <>;
+};
+
{
// Host Console
status = "okay";
--
2.17.1
[PATCH 3/4] ARM: dts: aspeed: Add KCS for Facebook
Added kcs device in Facebook Tiogapass device tree.
Signed-off-by: Vijay Khemka
---
arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts | 12
1 file changed, 12 insertions(+)
diff --git a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
index 58bbe08d3ba7..b99751b3e080 100644
--- a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
@@ -75,6 +75,18 @@
status = "okay";
};
+ {
+ // BMC KCS channel 2
+ status = "okay";
+ kcs_addr = <0xca8>;
+};
+
+ {
+ // BMC KCS channel 3
+ status = "okay";
+ kcs_addr = <0xca2>;
+};
+
{
status = "okay";
--
2.17.1
[PATCH 1/4] ARM: dts: aspeed: Add sensors devices for Facebook
Added ADC and other sensor devices in Facebook Tiogapass device tree.
Signed-off-by: Vijay Khemka
---
.../dts/aspeed-bmc-facebook-tiogapass.dts | 33 +--
1 file changed, 31 insertions(+), 2 deletions(-)
diff --git a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
index f8e7b71af7e6..58bbe08d3ba7 100644
--- a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
@@ -21,6 +21,25 @@
memory@8000 {
reg = <0x8000 0x2000>;
};
+
+ iio-hwmon {
+ compatible = "iio-hwmon";
+ oemname0 = "MB_P3V3";
+ oemname1 = "MB_P5V";
+ oemname2 = "MB_P12V";
+ oemname3 = "MB_P1V05";
+ oemname4 = "MB_PVNN_PCH_STBY";
+ oemname5 = "MB_P3V3_STBY";
+ oemname6 = "MB_P5V_STBY";
+ io-channels = < 0>, < 1>, < 2>, < 3>,
+ < 4>, < 5>, < 6>;
+ };
+
+ iio-hwmon-battery {
+ oemname0 = "MB_P3V_BAT";
+ compatible = "iio-hwmon";
+ io-channels = < 7>;
+ };
};
{
@@ -64,6 +83,10 @@
use-ncsi;
};
+ {
+ status = "okay";
+};
+
{
status = "okay";
//Airmax Conn B, CPU0 PIROM, CPU1 PIROM
@@ -122,6 +145,10 @@
{
status = "okay";
+ tmp421@1f {
+ compatible = "ti,tmp421";
+ reg = <0x1f>;
+ };
//Mezz Sensor SMBus
};
@@ -134,13 +161,15 @@
status = "okay";
pinctrl-names = "default";
pinctrl-0 = <_pwm0_default _pwm1_default>;
+ oemname0 = "System_Fan_Connector_1";
+ oemname1 = "System_Fan_Connector_3";
fan@0 {
reg = <0x00>;
aspeed,fan-tach-ch = /bits/ 8 <0x00>;
};
fan@1 {
- reg = <0x00>;
- aspeed,fan-tach-ch = /bits/ 8 <0x01>;
+ reg = <0x01>;
+ aspeed,fan-tach-ch = /bits/ 8 <0x02>;
};
};
--
2.17.1
Re: [PATCH v4 1/2] ARM: dts: aspeed: Add KCS & lpc ctrl for Facebook
On 12/12/18, 5:04 PM, "Joel Stanley" wrote:
On Thu, 13 Dec 2018 at 07:34, Vijay Khemka wrote:
>
> Added kcs device and lpc ctrl device to enable LPC clock in Facebook
> Tiogapass device tree.
As we're going to re-spin, it's a good idea to have the KCS and LPC
changes in separate patches. I will take the KCS one straight away,
but the LPC one might need more work (see my comments below).
Thanks Joel,
I will create different patches and resubmit again.
>
> Signed-off-by: Vijay Khemka
> ---
> .../dts/aspeed-bmc-facebook-tiogapass.dts | 30 +++
> 1 file changed, 30 insertions(+)
>
> diff --git a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
> index d9d7f208788a..9544fa2c1822 100644
> --- a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
> +++ b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
> @@ -21,6 +21,18 @@
> memory@8000 {
> reg = <0x8000 0x2000>;
> };
> +
> + reserved-memory {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges;
> +
> + flash_memory: region@9800 {
> + no-map;
> + reg = <0x9800 0x0400>; /* 64M */
> + };
Are you using this to expose some RAM over LPC? If not, we don't want this
here.
No, we are not using currently.
It is a flaw in the design of the driver that the reserved memory
region is required. We should change the driver to make it optional.
I agree with you we should make this change in driver.
As a workaround, we could add this patch to the tiogapass device tree
but make it smaller so you're not losing 64MB of RAM.
I will make it to 4K.
> + };
> +
> iio-hwmon {
> compatible = "iio-hwmon";
> oemname0 = "MB_P3V3";
> @@ -62,6 +74,12 @@
> };
> };
>
> +_ctrl {
> + status = "okay";
> + memory-region = <_memory>;
> + flash = <>;
> +};
> +
> {
> // Host Console
> status = "okay";
> @@ -75,6 +93,18 @@
> status = "okay";
> };
>
> + {
> + // BMC KCS channel 2
> + status = "okay";
> + kcs_addr = <0xca8>;
> +};
> +
> + {
> + // BMC KCS channel 3
> + status = "okay";
> + kcs_addr = <0xca2>;
> +};
> +
> {
> status = "okay";
>
> --
> 2.17.1
>
[PATCH v4 1/2] ARM: dts: aspeed: Add KCS & lpc ctrl for Facebook
Added kcs device and lpc ctrl device to enable LPC clock in Facebook
Tiogapass device tree.
Signed-off-by: Vijay Khemka
---
.../dts/aspeed-bmc-facebook-tiogapass.dts | 30 +++
1 file changed, 30 insertions(+)
diff --git a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
index d9d7f208788a..9544fa2c1822 100644
--- a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
@@ -21,6 +21,18 @@
memory@8000 {
reg = <0x8000 0x2000>;
};
+
+ reserved-memory {
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges;
+
+ flash_memory: region@9800 {
+ no-map;
+ reg = <0x9800 0x0400>; /* 64M */
+ };
+ };
+
iio-hwmon {
compatible = "iio-hwmon";
oemname0 = "MB_P3V3";
@@ -62,6 +74,12 @@
};
};
+_ctrl {
+ status = "okay";
+ memory-region = <_memory>;
+ flash = <>;
+};
+
{
// Host Console
status = "okay";
@@ -75,6 +93,18 @@
status = "okay";
};
+ {
+ // BMC KCS channel 2
+ status = "okay";
+ kcs_addr = <0xca8>;
+};
+
+ {
+ // BMC KCS channel 3
+ status = "okay";
+ kcs_addr = <0xca2>;
+};
+
{
status = "okay";
--
2.17.1
[PATCH v4 2/2] ARM: dts: aspeed: Add KCS support for LPC BMC
Added kcs device support for lpc BMC.
Signed-off-by: Vijay Khemka
---
arch/arm/boot/dts/aspeed-g5.dtsi | 33 +++-
1 file changed, 32 insertions(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi b/arch/arm/boot/dts/aspeed-g5.dtsi
index d107459fc0f8..2743f400aa29 100644
--- a/arch/arm/boot/dts/aspeed-g5.dtsi
+++ b/arch/arm/boot/dts/aspeed-g5.dtsi
@@ -330,8 +330,32 @@
ranges = <0x0 0x1e789000 0x1000>;
lpc_bmc: lpc-bmc@0 {
- compatible = "aspeed,ast2500-lpc-bmc";
+ compatible = "aspeed,ast2500-lpc-bmc",
"simple-mfd", "syscon";
reg = <0x0 0x80>;
+ reg-io-width = <4>;
+
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges = <0x0 0x0 0x80>;
+
+ kcs1: kcs1@0 {
+ compatible =
"aspeed,ast2500-kcs-bmc";
+ interrupts = <8>;
+ kcs_chan = <1>;
+ status = "disabled";
+ };
+ kcs2: kcs2@0 {
+ compatible =
"aspeed,ast2500-kcs-bmc";
+ interrupts = <8>;
+ kcs_chan = <2>;
+ status = "disabled";
+ };
+ kcs3: kcs3@0 {
+ compatible =
"aspeed,ast2500-kcs-bmc";
+ interrupts = <8>;
+ kcs_chan = <3>;
+ status = "disabled";
+ };
};
lpc_host: lpc-host@80 {
@@ -343,6 +367,13 @@
#size-cells = <1>;
ranges = <0x0 0x80 0x1e0>;
+ kcs4: kcs4@0 {
+ compatible =
"aspeed,ast2500-kcs-bmc";
+ interrupts = <8>;
+ kcs_chan = <4>;
+ status = "disabled";
+ };
+
lpc_ctrl: lpc-ctrl@0 {
compatible =
"aspeed,ast2500-lpc-ctrl";
reg = <0x0 0x80>;
--
2.17.1
[PATCH v3 1/2] ARM: dts: aspeed: Add KCS & lpc ctrl for Facebook
Added kcs device and lpc ctrl device to enable LPC clock in Facebook
Tiogapass device tree.
Signed-off-by: Vijay Khemka
---
.../dts/aspeed-bmc-facebook-tiogapass.dts | 30 +++
1 file changed, 30 insertions(+)
diff --git a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
index d9d7f208788a..9544fa2c1822 100644
--- a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
@@ -21,6 +21,18 @@
memory@8000 {
reg = <0x8000 0x2000>;
};
+
+ reserved-memory {
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges;
+
+ flash_memory: region@9800 {
+ no-map;
+ reg = <0x9800 0x0400>; /* 64M */
+ };
+ };
+
iio-hwmon {
compatible = "iio-hwmon";
oemname0 = "MB_P3V3";
@@ -62,6 +74,12 @@
};
};
+_ctrl {
+ status = "okay";
+ memory-region = <_memory>;
+ flash = <>;
+};
+
{
// Host Console
status = "okay";
@@ -75,6 +93,18 @@
status = "okay";
};
+ {
+ // BMC KCS channel 2
+ status = "okay";
+ kcs_addr = <0xca8>;
+};
+
+ {
+ // BMC KCS channel 3
+ status = "okay";
+ kcs_addr = <0xca2>;
+};
+
{
status = "okay";
--
2.17.1
[PATCH v3 2/2] ARM: dts: aspeed: Add KCS support for LPC BMC
Added kcs device support for lpc BMC.
Signed-off-by: Vijay Khemka
---
arch/arm/boot/dts/aspeed-g5.dtsi | 33 +++-
1 file changed, 32 insertions(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi b/arch/arm/boot/dts/aspeed-g5.dtsi
index d107459fc0f8..53e6ce4092dd 100644
--- a/arch/arm/boot/dts/aspeed-g5.dtsi
+++ b/arch/arm/boot/dts/aspeed-g5.dtsi
@@ -330,8 +330,32 @@
ranges = <0x0 0x1e789000 0x1000>;
lpc_bmc: lpc-bmc@0 {
- compatible = "aspeed,ast2500-lpc-bmc";
+ compatible = "aspeed,ast2500-lpc-bmc,
"simple-mfd", "syscon"";
reg = <0x0 0x80>;
+ reg-io-width = <4>;
+
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges = <0x0 0x0 0x80>;
+
+ kcs1: kcs1@0 {
+ compatible =
"aspeed,ast2500-kcs-bmc";
+ interrupts = <8>;
+ kcs_chan = <1>;
+ status = "disabled";
+ };
+ kcs2: kcs2@0 {
+ compatible =
"aspeed,ast2500-kcs-bmc";
+ interrupts = <8>;
+ kcs_chan = <2>;
+ status = "disabled";
+ };
+ kcs3: kcs3@0 {
+ compatible =
"aspeed,ast2500-kcs-bmc";
+ interrupts = <8>;
+ kcs_chan = <3>;
+ status = "disabled";
+ };
};
lpc_host: lpc-host@80 {
@@ -343,6 +367,13 @@
#size-cells = <1>;
ranges = <0x0 0x80 0x1e0>;
+ kcs4: kcs4@0 {
+ compatible =
"aspeed,ast2500-kcs-bmc";
+ interrupts = <8>;
+ kcs_chan = <4>;
+ status = "disabled";
+ };
+
lpc_ctrl: lpc-ctrl@0 {
compatible =
"aspeed,ast2500-lpc-ctrl";
reg = <0x0 0x80>;
--
2.17.1
[PATCH v2 1/2] ARM: dts: aspeed: Add KCS & lpc ctrl for Facebook
Added kcs device and lpc ctrl device to enable LPC clock in Facebook
Tiogapass device tree.
Signed-off-by: Vijay Khemka
---
.../dts/aspeed-bmc-facebook-tiogapass.dts | 30 +++
1 file changed, 30 insertions(+)
diff --git a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
index d9d7f208788a..9544fa2c1822 100644
--- a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
@@ -21,6 +21,18 @@
memory@8000 {
reg = <0x8000 0x2000>;
};
+
+ reserved-memory {
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges;
+
+ flash_memory: region@9800 {
+ no-map;
+ reg = <0x9800 0x0400>; /* 64M */
+ };
+ };
+
iio-hwmon {
compatible = "iio-hwmon";
oemname0 = "MB_P3V3";
@@ -62,6 +74,12 @@
};
};
+_ctrl {
+ status = "okay";
+ memory-region = <_memory>;
+ flash = <>;
+};
+
{
// Host Console
status = "okay";
@@ -75,6 +93,18 @@
status = "okay";
};
+ {
+ // BMC KCS channel 2
+ status = "okay";
+ kcs_addr = <0xca8>;
+};
+
+ {
+ // BMC KCS channel 3
+ status = "okay";
+ kcs_addr = <0xca2>;
+};
+
{
status = "okay";
--
2.17.1
[PATCH v2 2/2] ARM: dts: aspeed: Add KCS support for LPC BMC
Added kcs device support for lpc BMC.
Signed-off-by: Vijay Khemka
---
arch/arm/boot/dts/aspeed-g5.dtsi | 33 +++-
1 file changed, 32 insertions(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi b/arch/arm/boot/dts/aspeed-g5.dtsi
index d107459fc0f8..cb35a3ab580a 100644
--- a/arch/arm/boot/dts/aspeed-g5.dtsi
+++ b/arch/arm/boot/dts/aspeed-g5.dtsi
@@ -330,8 +330,32 @@
ranges = <0x0 0x1e789000 0x1000>;
lpc_bmc: lpc-bmc@0 {
- compatible = "aspeed,ast2500-lpc-bmc";
+ compatible = "aspeed,ast2500-lpc-bmc,
"simple-mfd", "syscon"";
reg = <0x0 0x80>;
+ reg-io-width = <4>;
+
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges = <0x0 0x0 0x80>;
+
+ kcs1: kcs1@0 {
+ compatible =
"aspeed,ast2500-kcs-bmc";
+ interrupts = <8>;
+ kcs_chan = <1>;
+ status = "disabled";
+ };
+ kcs2: kcs2@0 {
+ compatible =
"aspeed,ast2500-kcs-bmc";
+ interrupts = <8>;
+ kcs_chan = <2>;
+ status = "disabled";
+ };
+ kcs3: kcs3@0 {
+ compatible =
"aspeed,ast2500-kcs-bmc";
+ interrupts = <8>;
+ kcs_chan = <3>;
+ status = "disabled";
+ };
};
lpc_host: lpc-host@80 {
@@ -343,6 +367,13 @@
#size-cells = <1>;
ranges = <0x0 0x80 0x1e0>;
+ kcs4: kcs4@0 {
+ compatible =
"aspeed,ast2500-kcs-bmc";
+ interrupts = <8>;
+ kcs_chan = <4>;
+ status = "disabled";
+ };
+
lpc_ctrl: lpc-ctrl@0 {
compatible =
"aspeed,ast2500-lpc-ctrl";
reg = <0x0 0x80>;
--
2.17.1
Re: [PATCH 2/2] ARM: dts: aspeed: Add KCS support for LPC BMC
Thanks Jay,
I will take care of your comments and send next version.
Regards
-Vijay
On 12/11/18, 10:40 AM, "Jae Hyun Yoo" wrote:
Hi Vijay,
On 12/10/2018 12:07 PM, Vijay Khemka wrote:
> Added kcs device support for lpc BMC.
>
> Signed-off-by: Vijay Khemka
> ---
> arch/arm/boot/dts/aspeed-g5.dtsi | 29 -
> 1 file changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi
b/arch/arm/boot/dts/aspeed-g5.dtsi
> index d107459fc0f8..1bd48655bacb 100644
> --- a/arch/arm/boot/dts/aspeed-g5.dtsi
> +++ b/arch/arm/boot/dts/aspeed-g5.dtsi
> @@ -330,8 +330,35 @@
> ranges = <0x0 0x1e789000 0x1000>;
>
> lpc_bmc: lpc-bmc@0 {
> - compatible = "aspeed,ast2500-lpc-bmc";
> + compatible = "aspeed,ast2500-lpc-bmc,
"simple-mfd", "syscon"";
> reg = <0x0 0x80>;
> + reg-io-width = <4>;
> +
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges = <0x0 0x0 0x80>;
> +
> + kcs1: kcs1@0 {
> + compatible =
"aspeed,ast2500-kcs-bmc";
> + reg = <0x0 0x80>;
This reg property isn't needed at here because kcs_bmc_aspeed module
reads regmap from the parent node. Please check below code in the
driver code.
priv->map = syscon_node_to_regmap(dev->parent->of_node);
> + interrupts = <8>;
> + kcs_chan = <1>;
> + status = "disabled";
> + };
> + kcs2: kcs2@0 {
> + compatible =
"aspeed,ast2500-kcs-bmc";
> + reg = <0x0 0x80>;
ditto.
> + interrupts = <8>;
> + kcs_chan = <2>;
> + status = "disabled";
> + };
> + kcs3: kcs3@0 {
> + compatible =
"aspeed,ast2500-kcs-bmc";
> + reg = <0x0 0x80>;
ditto.
> + interrupts = <8>;
> + kcs_chan = <3>;
> + status = "disabled";
> + };
> };
>
> lpc_host: lpc-host@80 {
>
You might need to add kcs4 as a sub-node under lpc_host like below:
kcs4: kcs4@0 {
compatible = "aspeed,ast2500-kcs-bmc";
interrupts = <8>;
kcs_chan = <4>;
status = "disabled";
};
This would be uses as an SMM channel.
-Jae
[PATCH] ARM: dts: aspeed: Add sensors devices for Facebook
Added ADC and other sensor devices in Facebook Tiogapass device tree.
Signed-off-by: Vijay Khemka
---
.../dts/aspeed-bmc-facebook-tiogapass.dts | 33 +--
1 file changed, 31 insertions(+), 2 deletions(-)
diff --git a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
index f8e7b71af7e6..d9d7f208788a 100644
--- a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
@@ -21,6 +21,25 @@
memory@8000 {
reg = <0x8000 0x2000>;
};
+ iio-hwmon {
+ compatible = "iio-hwmon";
+ oemname0 = "MB_P3V3";
+ oemname1 = "MB_P5V";
+ oemname2 = "MB_P12V";
+ oemname3 = "MB_P1V05";
+ oemname4 = "MB_PVNN_PCH_STBY";
+ oemname5 = "MB_P3V3_STBY";
+ oemname6 = "MB_P5V_STBY";
+ io-channels = < 0>, < 1>, < 2>, < 3>,
+ < 4>, < 5>, < 6>;
+ };
+
+ iio-hwmon-battery {
+ oemname0 = "MB_P3V_BAT";
+ compatible = "iio-hwmon";
+ io-channels = < 7>;
+ };
+
};
{
@@ -64,6 +83,10 @@
use-ncsi;
};
+ {
+ status = "okay";
+};
+
{
status = "okay";
//Airmax Conn B, CPU0 PIROM, CPU1 PIROM
@@ -122,6 +145,10 @@
{
status = "okay";
+ tmp421@1f {
+ compatible = "ti,tmp421";
+ reg = <0x1f>;
+ };
//Mezz Sensor SMBus
};
@@ -134,13 +161,15 @@
status = "okay";
pinctrl-names = "default";
pinctrl-0 = <_pwm0_default _pwm1_default>;
+ oemname0 = "System_Fan_Connector_1";
+ oemname1 = "System_Fan_Connector_3";
fan@0 {
reg = <0x00>;
aspeed,fan-tach-ch = /bits/ 8 <0x00>;
};
fan@1 {
- reg = <0x00>;
- aspeed,fan-tach-ch = /bits/ 8 <0x01>;
+ reg = <0x01>;
+ aspeed,fan-tach-ch = /bits/ 8 <0x02>;
};
};
--
2.17.1
Re: [PATCH] ARM: dts: aspeed: Add KCS support for LPC BMC
Please ignore this patch as it is duplicate. It is included in list of 2
patches I have sent just before.
On 12/10/18, 12:08 PM, "Vijay Khemka" wrote:
Added kcs device support for lpc BMC.
Signed-off-by: Vijay Khemka
---
arch/arm/boot/dts/aspeed-g5.dtsi | 29 -
1 file changed, 28 insertions(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi
b/arch/arm/boot/dts/aspeed-g5.dtsi
index d107459fc0f8..1bd48655bacb 100644
--- a/arch/arm/boot/dts/aspeed-g5.dtsi
+++ b/arch/arm/boot/dts/aspeed-g5.dtsi
@@ -330,8 +330,35 @@
ranges = <0x0 0x1e789000 0x1000>;
lpc_bmc: lpc-bmc@0 {
- compatible = "aspeed,ast2500-lpc-bmc";
+ compatible = "aspeed,ast2500-lpc-bmc,
"simple-mfd", "syscon"";
reg = <0x0 0x80>;
+ reg-io-width = <4>;
+
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges = <0x0 0x0 0x80>;
+
+ kcs1: kcs1@0 {
+ compatible =
"aspeed,ast2500-kcs-bmc";
+ reg = <0x0 0x80>;
+ interrupts = <8>;
+ kcs_chan = <1>;
+ status = "disabled";
+ };
+ kcs2: kcs2@0 {
+ compatible =
"aspeed,ast2500-kcs-bmc";
+ reg = <0x0 0x80>;
+ interrupts = <8>;
+ kcs_chan = <2>;
+ status = "disabled";
+ };
+ kcs3: kcs3@0 {
+ compatible =
"aspeed,ast2500-kcs-bmc";
+ reg = <0x0 0x80>;
+ interrupts = <8>;
+ kcs_chan = <3>;
+ status = "disabled";
+ };
};
lpc_host: lpc-host@80 {
--
2.17.1
[PATCH 1/2] ARM: dts: aspeed: Add KCS & lpc ctrl for Facebook
Added kcs device and lpc ctrl device to enable LPC clock in Facebook
Tiogapass device tree.
Signed-off-by: Vijay Khemka
---
.../dts/aspeed-bmc-facebook-tiogapass.dts | 30 +++
1 file changed, 30 insertions(+)
diff --git a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
index d9d7f208788a..9544fa2c1822 100644
--- a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
@@ -21,6 +21,18 @@
memory@8000 {
reg = <0x8000 0x2000>;
};
+
+ reserved-memory {
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges;
+
+ flash_memory: region@9800 {
+ no-map;
+ reg = <0x9800 0x0400>; /* 64M */
+ };
+ };
+
iio-hwmon {
compatible = "iio-hwmon";
oemname0 = "MB_P3V3";
@@ -62,6 +74,12 @@
};
};
+_ctrl {
+ status = "okay";
+ memory-region = <_memory>;
+ flash = <>;
+};
+
{
// Host Console
status = "okay";
@@ -75,6 +93,18 @@
status = "okay";
};
+ {
+ // BMC KCS channel 2
+ status = "okay";
+ kcs_addr = <0xca8>;
+};
+
+ {
+ // BMC KCS channel 3
+ status = "okay";
+ kcs_addr = <0xca2>;
+};
+
{
status = "okay";
--
2.17.1
[PATCH 2/2] ARM: dts: aspeed: Add KCS support for LPC BMC
Added kcs device support for lpc BMC.
Signed-off-by: Vijay Khemka
---
arch/arm/boot/dts/aspeed-g5.dtsi | 29 -
1 file changed, 28 insertions(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi b/arch/arm/boot/dts/aspeed-g5.dtsi
index d107459fc0f8..1bd48655bacb 100644
--- a/arch/arm/boot/dts/aspeed-g5.dtsi
+++ b/arch/arm/boot/dts/aspeed-g5.dtsi
@@ -330,8 +330,35 @@
ranges = <0x0 0x1e789000 0x1000>;
lpc_bmc: lpc-bmc@0 {
- compatible = "aspeed,ast2500-lpc-bmc";
+ compatible = "aspeed,ast2500-lpc-bmc,
"simple-mfd", "syscon"";
reg = <0x0 0x80>;
+ reg-io-width = <4>;
+
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges = <0x0 0x0 0x80>;
+
+ kcs1: kcs1@0 {
+ compatible =
"aspeed,ast2500-kcs-bmc";
+ reg = <0x0 0x80>;
+ interrupts = <8>;
+ kcs_chan = <1>;
+ status = "disabled";
+ };
+ kcs2: kcs2@0 {
+ compatible =
"aspeed,ast2500-kcs-bmc";
+ reg = <0x0 0x80>;
+ interrupts = <8>;
+ kcs_chan = <2>;
+ status = "disabled";
+ };
+ kcs3: kcs3@0 {
+ compatible =
"aspeed,ast2500-kcs-bmc";
+ reg = <0x0 0x80>;
+ interrupts = <8>;
+ kcs_chan = <3>;
+ status = "disabled";
+ };
};
lpc_host: lpc-host@80 {
--
2.17.1
[PATCH] ARM: dts: aspeed: Add KCS support for LPC BMC
Added kcs device support for lpc BMC.
Signed-off-by: Vijay Khemka
---
arch/arm/boot/dts/aspeed-g5.dtsi | 29 -
1 file changed, 28 insertions(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi b/arch/arm/boot/dts/aspeed-g5.dtsi
index d107459fc0f8..1bd48655bacb 100644
--- a/arch/arm/boot/dts/aspeed-g5.dtsi
+++ b/arch/arm/boot/dts/aspeed-g5.dtsi
@@ -330,8 +330,35 @@
ranges = <0x0 0x1e789000 0x1000>;
lpc_bmc: lpc-bmc@0 {
- compatible = "aspeed,ast2500-lpc-bmc";
+ compatible = "aspeed,ast2500-lpc-bmc,
"simple-mfd", "syscon"";
reg = <0x0 0x80>;
+ reg-io-width = <4>;
+
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges = <0x0 0x0 0x80>;
+
+ kcs1: kcs1@0 {
+ compatible =
"aspeed,ast2500-kcs-bmc";
+ reg = <0x0 0x80>;
+ interrupts = <8>;
+ kcs_chan = <1>;
+ status = "disabled";
+ };
+ kcs2: kcs2@0 {
+ compatible =
"aspeed,ast2500-kcs-bmc";
+ reg = <0x0 0x80>;
+ interrupts = <8>;
+ kcs_chan = <2>;
+ status = "disabled";
+ };
+ kcs3: kcs3@0 {
+ compatible =
"aspeed,ast2500-kcs-bmc";
+ reg = <0x0 0x80>;
+ interrupts = <8>;
+ kcs_chan = <3>;
+ status = "disabled";
+ };
};
lpc_host: lpc-host@80 {
--
2.17.1
Re: [PATCH net-next] net/ncsi: Add NCSI Mellanox OEM command
Thanks David On 10/26/18, 10:36 AM, "David Miller" wrote: From: Vijay Khemka Date: Fri, 26 Oct 2018 17:19:49 + > Do you have any timeline when it is going to open next or how do I > know. I always announce net-next openning and closing here on the list. There is also a web site: http://vger.kernel.org/~davem/net-next.html Thanks.
Re: [PATCH net-next] net/ncsi: Add NCSI Mellanox OEM command
Thanks David On 10/26/18, 10:36 AM, "David Miller" wrote: From: Vijay Khemka Date: Fri, 26 Oct 2018 17:19:49 + > Do you have any timeline when it is going to open next or how do I > know. I always announce net-next openning and closing here on the list. There is also a web site: http://vger.kernel.org/~davem/net-next.html Thanks.
[PATCH v2 1/2] ARM: dt-bindings: Add vendor prefix for Facebook
Initial introduction of Facebook TiogaPass family equipped with Aspeed 2500 BMC SoC. TiogaPass is a x86 server development kit with a ASPEED ast2500 BMC manufactured by Facebook. This adds an entry of Facebook in Documentation for vendor prefix Signed-off-by: Vijay Khemka --- Documentation/devicetree/bindings/vendor-prefixes.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt index 2c3fc512e746..0520303ee5b8 100644 --- a/Documentation/devicetree/bindings/vendor-prefixes.txt +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt @@ -127,6 +127,7 @@ everspinEverspin Technologies, Inc. exar Exar Corporation excito Excito ezchip EZchip Semiconductor +facebook Facebook fairphone Fairphone B.V. faradayFaraday Technology Corporation fastraxFastrax Oy -- 2.17.1
[PATCH v2 1/2] ARM: dt-bindings: Add vendor prefix for Facebook
Initial introduction of Facebook TiogaPass family equipped with Aspeed 2500 BMC SoC. TiogaPass is a x86 server development kit with a ASPEED ast2500 BMC manufactured by Facebook. This adds an entry of Facebook in Documentation for vendor prefix Signed-off-by: Vijay Khemka --- Documentation/devicetree/bindings/vendor-prefixes.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt index 2c3fc512e746..0520303ee5b8 100644 --- a/Documentation/devicetree/bindings/vendor-prefixes.txt +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt @@ -127,6 +127,7 @@ everspinEverspin Technologies, Inc. exar Exar Corporation excito Excito ezchip EZchip Semiconductor +facebook Facebook fairphone Fairphone B.V. faradayFaraday Technology Corporation fastraxFastrax Oy -- 2.17.1
