| Message ID | 1539885344-18974-2-git-send-email-talho@nvidia.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | BPMP-FW version query ("tag") update | expand |
On Thu, Oct 18, 2018 at 08:55:40PM +0300, Timo Alho wrote: > Add a helper function to check that firmware is supporting a given MRQ > command. > > Signed-off-by: Timo Alho <talho@nvidia.com> > --- > drivers/firmware/tegra/bpmp.c | 28 ++++++++++++++++++++++++++++ > include/soc/tegra/bpmp.h | 7 +++++++ > 2 files changed, 35 insertions(+) Acked-by: Sivaram Nair <sivaramn@nvidia.com>
On Thu, Oct 18, 2018 at 08:55:40PM +0300, Timo Alho wrote: > Add a helper function to check that firmware is supporting a given MRQ > command. > > Signed-off-by: Timo Alho <talho@nvidia.com> > --- > drivers/firmware/tegra/bpmp.c | 28 ++++++++++++++++++++++++++++ > include/soc/tegra/bpmp.h | 7 +++++++ > 2 files changed, 35 insertions(+) > > diff --git a/drivers/firmware/tegra/bpmp.c b/drivers/firmware/tegra/bpmp.c > index 41448ba..79859ab 100644 > --- a/drivers/firmware/tegra/bpmp.c > +++ b/drivers/firmware/tegra/bpmp.c > @@ -470,6 +470,34 @@ void tegra_bpmp_free_mrq(struct tegra_bpmp *bpmp, unsigned int mrq, void *data) > } > EXPORT_SYMBOL_GPL(tegra_bpmp_free_mrq); > > +bool tegra_bpmp_mrq_is_supported(struct tegra_bpmp *bpmp, unsigned int mrq) > +{ > + struct mrq_query_abi_request req = { .mrq = cpu_to_le32(mrq) }; > + struct mrq_query_abi_response resp; > + struct tegra_bpmp_message msg = { > + .mrq = MRQ_QUERY_ABI, > + .tx = { > + .data = &req, > + .size = sizeof(req), > + }, > + .rx = { > + .data = &resp, > + .size = sizeof(resp), > + }, > + }; > + int ret; > + > + ret = tegra_bpmp_transfer(bpmp, &msg); > + if (ret != 0 || msg.rx.ret != 0) { Can somebody remind me of the difference between these two? It looks like we don't have a description of msg.rx.ret anywhere. It comes from tegra_bpmp_channel.ib->code, which in turn is set directly by BPMP as part of a transfer. What are the values that it can take? I think we could use some kerneldoc for struct tegra_bpmp_mb_data in include/soc/tegra/bpmp.h. > + /* something went wrong; assume not supported */ > + dev_warn(bpmp->dev, "tegra_bpmp_transfer failed\n"); This should probably contain an error code of some sort to help narrow down why it fails. Also, please use a more human readable error message for consistency within the driver. Something along these lines perhaps? dev_warn(bpmp->dev, "transfer failed: %d\n", ret); Or perhaps more specifically: dev_warn(bpmp->dev, "ABI query MRQ failed: %d\n", ret); Thierry
On 22.10.2018 12.12, Thierry Reding wrote: > On Thu, Oct 18, 2018 at 08:55:40PM +0300, Timo Alho wrote: >> Add a helper function to check that firmware is supporting a given MRQ >> command. >> >> Signed-off-by: Timo Alho <talho@nvidia.com> >> --- >> drivers/firmware/tegra/bpmp.c | 28 ++++++++++++++++++++++++++++ >> include/soc/tegra/bpmp.h | 7 +++++++ >> 2 files changed, 35 insertions(+) >> >> diff --git a/drivers/firmware/tegra/bpmp.c b/drivers/firmware/tegra/bpmp.c >> index 41448ba..79859ab 100644 >> --- a/drivers/firmware/tegra/bpmp.c >> +++ b/drivers/firmware/tegra/bpmp.c >> @@ -470,6 +470,34 @@ void tegra_bpmp_free_mrq(struct tegra_bpmp *bpmp, unsigned int mrq, void *data) >> } >> EXPORT_SYMBOL_GPL(tegra_bpmp_free_mrq); >> >> +bool tegra_bpmp_mrq_is_supported(struct tegra_bpmp *bpmp, unsigned int mrq) >> +{ >> + struct mrq_query_abi_request req = { .mrq = cpu_to_le32(mrq) }; >> + struct mrq_query_abi_response resp; >> + struct tegra_bpmp_message msg = { >> + .mrq = MRQ_QUERY_ABI, >> + .tx = { >> + .data = &req, >> + .size = sizeof(req), >> + }, >> + .rx = { >> + .data = &resp, >> + .size = sizeof(resp), >> + }, >> + }; >> + int ret; >> + >> + ret = tegra_bpmp_transfer(bpmp, &msg); >> + if (ret != 0 || msg.rx.ret != 0) { > > Can somebody remind me of the difference between these two? It looks > like we don't have a description of msg.rx.ret anywhere. It comes from > tegra_bpmp_channel.ib->code, which in turn is set directly by BPMP as > part of a transfer. This is correct. 'ret' is nonzero if the "wire-level" communication between CPU and BPMP fails. In turn, 'msg.rx.ret' is the first word of the response payload from bpmp. A nonzero value indicates that the request made to BPMP was somehow ill-formatted. Possible reasons could be e.g.: * Use of non-existing MRQ code * Use of non-existing resource identifier (clk, reset, power-domain, ...) > What are the values that it can take? Error codes are listed in very end of bpmp-abi.h > I think we could use some kerneldoc for struct tegra_bpmp_mb_data in > include/soc/tegra/bpmp.h. > >> + /* something went wrong; assume not supported */ >> + dev_warn(bpmp->dev, "tegra_bpmp_transfer failed\n"); > > This should probably contain an error code of some sort to help narrow > down why it fails. Also, please use a more human readable error message > for consistency within the driver. Something along these lines perhaps? > > dev_warn(bpmp->dev, "transfer failed: %d\n", ret); > > Or perhaps more specifically: > > dev_warn(bpmp->dev, "ABI query MRQ failed: %d\n", ret); Sure, I'll make the change.
diff --git a/drivers/firmware/tegra/bpmp.c b/drivers/firmware/tegra/bpmp.c index 41448ba..79859ab 100644 --- a/drivers/firmware/tegra/bpmp.c +++ b/drivers/firmware/tegra/bpmp.c @@ -470,6 +470,34 @@ void tegra_bpmp_free_mrq(struct tegra_bpmp *bpmp, unsigned int mrq, void *data) } EXPORT_SYMBOL_GPL(tegra_bpmp_free_mrq); +bool tegra_bpmp_mrq_is_supported(struct tegra_bpmp *bpmp, unsigned int mrq) +{ + struct mrq_query_abi_request req = { .mrq = cpu_to_le32(mrq) }; + struct mrq_query_abi_response resp; + struct tegra_bpmp_message msg = { + .mrq = MRQ_QUERY_ABI, + .tx = { + .data = &req, + .size = sizeof(req), + }, + .rx = { + .data = &resp, + .size = sizeof(resp), + }, + }; + int ret; + + ret = tegra_bpmp_transfer(bpmp, &msg); + if (ret != 0 || msg.rx.ret != 0) { + /* something went wrong; assume not supported */ + dev_warn(bpmp->dev, "tegra_bpmp_transfer failed\n"); + return false; + } + + return resp.status == 0; +} +EXPORT_SYMBOL_GPL(tegra_bpmp_mrq_is_supported); + static void tegra_bpmp_mrq_handle_ping(unsigned int mrq, struct tegra_bpmp_channel *channel, void *data) diff --git a/include/soc/tegra/bpmp.h b/include/soc/tegra/bpmp.h index e69e4c4..b02f926 100644 --- a/include/soc/tegra/bpmp.h +++ b/include/soc/tegra/bpmp.h @@ -129,6 +129,7 @@ int tegra_bpmp_request_mrq(struct tegra_bpmp *bpmp, unsigned int mrq, tegra_bpmp_mrq_handler_t handler, void *data); void tegra_bpmp_free_mrq(struct tegra_bpmp *bpmp, unsigned int mrq, void *data); +bool tegra_bpmp_mrq_is_supported(struct tegra_bpmp *bpmp, unsigned int mrq); #else static inline struct tegra_bpmp *tegra_bpmp_get(struct device *dev) { @@ -164,6 +165,12 @@ static inline void tegra_bpmp_free_mrq(struct tegra_bpmp *bpmp, unsigned int mrq, void *data) { } + +static inline bool tegra_bpmp_mrq_is_supported(struct tegra_bpmp *bpmp, + unsigned int mrq) +{ + return false; +} #endif #if IS_ENABLED(CONFIG_CLK_TEGRA_BPMP)
Add a helper function to check that firmware is supporting a given MRQ command. Signed-off-by: Timo Alho <talho@nvidia.com> --- drivers/firmware/tegra/bpmp.c | 28 ++++++++++++++++++++++++++++ include/soc/tegra/bpmp.h | 7 +++++++ 2 files changed, 35 insertions(+)