Re: [PATCH v3 05/12] firmware: tegra: Add BPMP support

[Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 6048 bytes --]

On Mon, Aug 22, 2016 at 10:26:50AM +0100, Jon Hunter wrote:
> 
> On 19/08/16 18:32, Thierry Reding wrote:
> > From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > 
> > The Boot and Power Management Processor (BPMP) is a co-processor found
> > on Tegra SoCs. It is designed to handle the early stages of the boot
> > process and offload power management tasks (such as clocks, resets,
> > powergates, ...) as well as system control services.
> > 
> > Compared to the ARM SCPI, the services provided by BPMP are message-
> > based rather than method-based. The BPMP firmware driver provides the
> > services to transmit data to and receive data from the BPMP. Users can
> > also register an MRQ, for which a service routine will be run when a
> > corresponding event is received from the firmware.
> 
> MRQ?

I think that means "Message ReQuest", which is sort of like an IRQ but
the user will receive a message (with potentially payload) instead. Do
you want me to spell that out in the commit message, or what would you
suggest?

> > diff --git a/drivers/firmware/tegra/Makefile b/drivers/firmware/tegra/Makefile
> > index 92e2153e8173..e34a2f79e1ad 100644
> > --- a/drivers/firmware/tegra/Makefile
> > +++ b/drivers/firmware/tegra/Makefile
> > @@ -1 +1,2 @@
> > +obj-$(CONFIG_TEGRA_BPMP)	+= bpmp.o
> >  obj-$(CONFIG_TEGRA_IVC)		+= ivc.o
> > diff --git a/drivers/firmware/tegra/bpmp.c b/drivers/firmware/tegra/bpmp.c
> > new file mode 100644
> > index 000000000000..a09043b1be05
> > --- /dev/null
> > +++ b/drivers/firmware/tegra/bpmp.c
> > @@ -0,0 +1,880 @@
> > +/*
> > + * Copyright (c) 2016, NVIDIA CORPORATION.  All rights reserved.
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms and conditions of the GNU General Public License,
> > + * version 2, as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope it will be useful, but WITHOUT
> > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> > + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> > + * more details.
> > + */
> > +
> > +#define DEBUG
> 
> I don't think we want DEBUG by default, right?

Yes, that's left-over from debugging.

> > +static int tegra_bpmp_ping(struct tegra_bpmp *bpmp)
> > +{
> > +	struct mrq_ping_response response;
> > +	struct mrq_ping_request request;
> > +	struct tegra_bpmp_message msg;
> > +	ktime_t start, delta;
> > +	unsigned long flags;
> > +	int err;
> > +
> > +	memset(&request, 0, sizeof(request));
> > +	request.challenge = 1;
> > +
> > +	memset(&response, 0, sizeof(response));
> > +
> > +	memset(&msg, 0, sizeof(msg));
> > +	msg.mrq = MRQ_PING;
> > +	msg.tx.data = &request;
> > +	msg.tx.size = sizeof(request);
> > +	msg.rx.data = &response;
> > +	msg.rx.size = sizeof(response);
> > +
> > +	start = ktime_get();
> > +
> > +	local_irq_save(flags);
> > +	err = tegra_bpmp_transfer_atomic(bpmp, &msg);
> > +	local_irq_restore(flags);
> > +
> > +	delta = ktime_sub(ktime_get(), start);
> > +
> > +	if (!err)
> > +		dev_info(bpmp->dev,
> > +			 "ping ok: challenge: %u, response: %u, time: %lld\n",
> > +			 request.challenge, response.reply,
> > +			 ktime_to_us(delta));
> 
> Should this be a dev_dbg? I guess this only happens on probe.

I suppose you could use this anywhere else, too, just to check that the
BPMP is still responding. But yes, I think making this DEBUG level will
be enough.

> 
> > +	return err;
> > +}
> 
> [snip]
> 
> > +static int tegra_bpmp_probe(struct platform_device *pdev)
> > +{
> > +	struct tegra_bpmp_channel *channel;
> > +	struct tegra_bpmp *bpmp;
> > +	struct device_node *np;
> > +	struct resource res;
> > +	unsigned int i;
> > +	char tag[32];
> > +	size_t size;
> > +	int err;
> > +
> > +	bpmp = devm_kzalloc(&pdev->dev, sizeof(*bpmp), GFP_KERNEL);
> > +	if (!bpmp)
> > +		return -ENOMEM;
> > +
> > +	bpmp->soc = of_device_get_match_data(&pdev->dev);
> > +	bpmp->dev = &pdev->dev;
> > +
> > +	np = of_parse_phandle(pdev->dev.of_node, "shmem", 0);
> > +	if (!np)
> > +		return -ENOENT;
> > +
> > +	of_address_to_resource(np, 0, &res);
> > +	of_node_put(np);
> > +
> > +	bpmp->tx_base = devm_ioremap_resource(&pdev->dev, &res);
> > +	if (IS_ERR(bpmp->tx_base))
> > +		return PTR_ERR(bpmp->tx_base);
> > +
> > +	np = of_parse_phandle(pdev->dev.of_node, "shmem", 1);
> > +	if (!np)
> > +		return -ENOENT;
> > +
> > +	of_address_to_resource(np, 0, &res);
> > +	of_node_put(np);
> > +
> > +	bpmp->rx_base = devm_ioremap_resource(&pdev->dev, &res);
> > +	if (IS_ERR(bpmp->rx_base))
> > +		return PTR_ERR(bpmp->rx_base);
> > +
> > +	bpmp->num_channels = bpmp->soc->channels.cpu_tx.count +
> > +			     bpmp->soc->channels.thread.count +
> > +			     bpmp->soc->channels.cpu_rx.count;
> > +
> > +	bpmp->channels = devm_kcalloc(&pdev->dev, bpmp->num_channels,
> > +				      sizeof(*channel), GFP_KERNEL);
> > +	if (!bpmp->channels)
> > +		return -ENOMEM;
> > +
> > +	/* mbox registration */
> > +	bpmp->mbox.client.dev = &pdev->dev;
> > +	bpmp->mbox.client.rx_callback = tegra_bpmp_handle_rx;
> > +	bpmp->mbox.client.tx_block = false;
> > +	bpmp->mbox.client.knows_txdone = false;
> > +
> > +	bpmp->mbox.channel = mbox_request_channel(&bpmp->mbox.client, 0);
> > +	if (IS_ERR(bpmp->mbox.channel)) {
> > +		err = PTR_ERR(bpmp->mbox.channel);
> > +		dev_err(&pdev->dev, "failed to get HSP mailbox: %d\n", err);
> > +		return err;
> > +	}
> > +
> > +	/* message channel initialization */
> > +	for (i = 0; i < bpmp->num_channels; i++) {
> > +		struct tegra_bpmp_channel *channel = &bpmp->channels[i];
> > +
> > +		err = tegra_bpmp_channel_init(channel, bpmp, i);
> > +		if (err)
> > +			return err;
> > +	}
> 
> We should make sure we free the mbox if we fail after requesting it.

Yes, will do.

Thanks,
Thierry

> 
> Cheers
> Jon
> 
> -- 
> nvpublic

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]