* Re: [RFC] dt-bindings: mailbox: add doorbell support to ARM MHU
From: Jassi Brar @ 2020-06-04 15:15 UTC (permalink / raw)
To: Sudeep Holla
Cc: Viresh Kumar, Rob Herring, Arnd Bergmann, Frank Rowand,
Bjorn Andersson, Vincent Guittot, linux-arm-kernel,
Devicetree List, Linux Kernel Mailing List
In-Reply-To: <20200604092052.GD8814@bogus>
On Thu, Jun 4, 2020 at 4:20 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Wed, Jun 03, 2020 at 01:32:42PM -0500, Jassi Brar wrote:
> > On Wed, Jun 3, 2020 at 1:04 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
> > >
> > > On Fri, May 29, 2020 at 09:37:58AM +0530, Viresh Kumar wrote:
> > > > On 28-05-20, 13:20, Rob Herring wrote:
> > > > > Whether Linux
> > > > > requires serializing mailbox accesses is a separate issue. On that side,
> > > > > it seems silly to not allow driving the h/w in the most efficient way
> > > > > possible.
> > > >
> > > > That's exactly what we are trying to say. The hardware allows us to
> > > > write all 32 bits in parallel, without any hardware issues, why
> > > > shouldn't we do that ? The delay (which Sudeep will find out, he is
> > > > facing issues with hardware access because of lockdown right now)
> > >
> > > OK, I was able to access the setup today. I couldn't reach a point
> > > where I can do measurements as the system just became unusable with
> > > one physical channel instead of 2 virtual channels as in my patches.
> > >
> > > My test was simple. Switch to schedutil and read sensors periodically
> > > via sysfs.
> > >
> > > arm-scmi firmware:scmi: message for 1 is not expected!
> > >
> > This sounds like you are not serialising requests on a shared channel.
> > Can you please also share the patch?
>
> OK, I did try with a small patch initially and then realised we must hit
> issue with mainline as is. Tried and the behaviour is exact same. All
> I did is removed my patches and use bit[0] as the signal. It doesn't
> matter as writes to the register are now serialised. Oh, the above
> message comes when OS times out in advance while firmware continues to
> process the old request and respond.
>
> The trace I sent gives much better view of what's going on.
>
BTW, you didn't even share 'good' vs 'bad' log for me to actually see
if the api lacks.
Let us look closely ...
>> bash-1019 [005] 1149.452340: scmi_xfer_begin:
transfer_id=1537 msg_id=7 protocol_id=19 seq=0 poll=1
>> bash-1019 [005] 1149.452407: scmi_xfer_end:
transfer_id=1537 msg_id=7 protocol_id=19 seq=0 status=0
>
This round trip took 67usecs. (log shows some at even 3usecs)
That includes mailbox api overhead, memcpy and the time taken by
remote to respond.
So the api is definitely capable of much faster transfers than you require.
>> bash-1526 [000] 1149.472553: scmi_xfer_begin: transfer_id=1538 msg_id=6 protocol_id=21 seq=0 poll=0
>> <idle>-0 [001] 1149.472733: scmi_xfer_begin: transfer_id=1539 msg_id=7 protocol_id=19 seq=1 poll=1
>
Here another request is started before the first is finished.
If you want this to work you have to serialise access to the single
physical channel and/or run the remote firmware in asynchronous mode -
that is, the firmware ack the bit as soon as it sees and starts
working in the background, so that we return in ~3usec always, while
the data returns whenever it is ready. Again, I don't have the code
or the 'good' run log to compare.
PS: I feel it is probably less effort for me to simply let the patch
through, than use my reiki powers to imagine how your test code and
firmware looks like.
^ permalink raw reply
* Re: [PATCH v7 5/5] drivers/tty/serial: add LiteUART driver
From: Randy Dunlap @ 2020-06-04 15:14 UTC (permalink / raw)
To: Mateusz Holenko, Rob Herring, Mark Rutland, Greg Kroah-Hartman,
Jiri Slaby, devicetree, linux-serial
Cc: Stafford Horne, Karol Gugala, Mauro Carvalho Chehab,
David S. Miller, Paul E. McKenney, Filip Kokosinski,
Pawel Czarnecki, Joel Stanley, Jonathan Cameron, Maxime Ripard,
Shawn Guo, Heiko Stuebner, Sam Ravnborg, Icenowy Zheng,
Laurent Pinchart, linux-kernel, Gabriel L. Somlo
In-Reply-To: <20200604121142.2964437-5-mholenko@antmicro.com>
Hi--
On 6/4/20 3:14 AM, Mateusz Holenko wrote:
> +config SERIAL_LITEUART
> + tristate "LiteUART serial port support"
> + depends on HAS_IOMEM
> + depends on OF || COMPILE_TEST
> + depends on LITEX_SOC_CONTROLLER
> + select SERIAL_CORE
> + help
> + This driver is for the FPGA-based LiteUART serial controller from LiteX
> + SoC builder.
> +
> + Say 'Y' here if you wish to use the LiteUART serial controller.
> + Otherwise, say 'N'.
That last paragraph seems to say that Y and N are the only choices here.
It can also be set to M ...
--
~Randy
^ permalink raw reply
* Re: [PATCH 10/12] of/irq: Make of_msi_map_rid() PCI bus agnostic
From: Lorenzo Pieralisi @ 2020-06-04 15:08 UTC (permalink / raw)
To: Rob Herring
Cc: devicetree, Sudeep Holla, Catalin Marinas, Will Deacon,
Diana Craciun, Marc Zyngier, Joerg Roedel, Hanjun Guo,
Rafael J. Wysocki, Makarand Pawagi, linux-acpi, Linux IOMMU, PCI,
Bjorn Helgaas, Robin Murphy,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
Laurentiu Tudor
In-Reply-To: <CAL_JsqLTBxX_3KjiEqMfw0qMaTmj_DdPD3j-yMUvrvONPBSvjg@mail.gmail.com>
On Thu, May 21, 2020 at 05:17:27PM -0600, Rob Herring wrote:
> On Thu, May 21, 2020 at 7:00 AM Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
> >
> > There is nothing PCI bus specific in the of_msi_map_rid()
> > implementation other than the requester ID tag for the input
> > ID space. Rename requester ID to a more generic ID so that
> > the translation code can be used by all busses that require
> > input/output ID translations.
> >
> > Leave a wrapper function of_msi_map_rid() in place to keep
> > existing PCI code mapping requester ID syntactically unchanged.
> >
> > No functional change intended.
> >
> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Marc Zyngier <maz@kernel.org>
> > ---
> > drivers/of/irq.c | 28 ++++++++++++++--------------
> > include/linux/of_irq.h | 14 ++++++++++++--
> > 2 files changed, 26 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> > index 48a40326984f..25d17b8a1a1a 100644
> > --- a/drivers/of/irq.c
> > +++ b/drivers/of/irq.c
> > @@ -576,43 +576,43 @@ void __init of_irq_init(const struct of_device_id *matches)
> > }
> > }
> >
> > -static u32 __of_msi_map_rid(struct device *dev, struct device_node **np,
> > - u32 rid_in)
> > +static u32 __of_msi_map_id(struct device *dev, struct device_node **np,
> > + u32 id_in)
> > {
> > struct device *parent_dev;
> > - u32 rid_out = rid_in;
> > + u32 id_out = id_in;
> >
> > /*
> > * Walk up the device parent links looking for one with a
> > * "msi-map" property.
> > */
> > for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent)
> > - if (!of_map_rid(parent_dev->of_node, rid_in, "msi-map",
> > - "msi-map-mask", np, &rid_out))
> > + if (!of_map_id(parent_dev->of_node, id_in, "msi-map",
> > + "msi-map-mask", np, &id_out))
> > break;
> > - return rid_out;
> > + return id_out;
> > }
> >
> > /**
> > - * of_msi_map_rid - Map a MSI requester ID for a device.
> > + * of_msi_map_id - Map a MSI ID for a device.
> > * @dev: device for which the mapping is to be done.
> > * @msi_np: device node of the expected msi controller.
> > - * @rid_in: unmapped MSI requester ID for the device.
> > + * @id_in: unmapped MSI ID for the device.
> > *
> > * Walk up the device hierarchy looking for devices with a "msi-map"
> > - * property. If found, apply the mapping to @rid_in.
> > + * property. If found, apply the mapping to @id_in.
> > *
> > - * Returns the mapped MSI requester ID.
> > + * Returns the mapped MSI ID.
> > */
> > -u32 of_msi_map_rid(struct device *dev, struct device_node *msi_np, u32 rid_in)
> > +u32 of_msi_map_id(struct device *dev, struct device_node *msi_np, u32 id_in)
> > {
> > - return __of_msi_map_rid(dev, &msi_np, rid_in);
> > + return __of_msi_map_id(dev, &msi_np, id_in);
> > }
> >
> > /**
> > * of_msi_map_get_device_domain - Use msi-map to find the relevant MSI domain
> > * @dev: device for which the mapping is to be done.
> > - * @rid: Requester ID for the device.
> > + * @id: Device ID.
> > * @bus_token: Bus token
> > *
> > * Walk up the device hierarchy looking for devices with a "msi-map"
> > @@ -625,7 +625,7 @@ struct irq_domain *of_msi_map_get_device_domain(struct device *dev, u32 id,
> > {
> > struct device_node *np = NULL;
> >
> > - __of_msi_map_rid(dev, &np, id);
> > + __of_msi_map_id(dev, &np, id);
> > return irq_find_matching_host(np, bus_token);
> > }
> >
> > diff --git a/include/linux/of_irq.h b/include/linux/of_irq.h
> > index 7142a3722758..cf9cb1e545ce 100644
> > --- a/include/linux/of_irq.h
> > +++ b/include/linux/of_irq.h
> > @@ -55,7 +55,12 @@ extern struct irq_domain *of_msi_map_get_device_domain(struct device *dev,
> > u32 id,
> > u32 bus_token);
> > extern void of_msi_configure(struct device *dev, struct device_node *np);
> > -u32 of_msi_map_rid(struct device *dev, struct device_node *msi_np, u32 rid_in);
> > +u32 of_msi_map_id(struct device *dev, struct device_node *msi_np, u32 id_in);
> > +static inline u32 of_msi_map_rid(struct device *dev,
> > + struct device_node *msi_np, u32 rid_in)
> > +{
> > + return of_msi_map_id(dev, msi_np, rid_in);
> > +}
> > #else
> > static inline int of_irq_count(struct device_node *dev)
> > {
> > @@ -93,10 +98,15 @@ static inline struct irq_domain *of_msi_map_get_device_domain(struct device *dev
> > static inline void of_msi_configure(struct device *dev, struct device_node *np)
> > {
> > }
> > +static inline u32 of_msi_map_id(struct device *dev,
> > + struct device_node *msi_np, u32 id_in)
> > +{
> > + return id_in;
> > +}
> > static inline u32 of_msi_map_rid(struct device *dev,
> > struct device_node *msi_np, u32 rid_in)
>
> Move this out of the ifdef and you only need it declared once.
>
> But again, I think I'd just kill of_msi_map_rid.
Yes I don't think there is a clear benefit in keeping the _rid
interface.
Thanks,
Lorenzo
^ permalink raw reply
* Re: [PATCH 5/7] arm64: dts: qcom: pm8150x: add thermal alarms and thermal zones
From: Dmitry Baryshkov @ 2020-06-04 15:03 UTC (permalink / raw)
To: Vinod Koul
Cc: Andy Gross, Bjorn Andersson, Rob Herring, linux-arm-msm,
devicetree, patches, linaro-kernel
In-Reply-To: <20200604104701.GG3521@vkoul-mobl>
On 04/06/2020 13:47, Vinod Koul wrote:
> On 04-06-20, 03:43, Dmitry Baryshkov wrote:
>> Add temperature alarm and thermal zone configuration to all three
>> pm8150 instances. Configuration is largely based on the msm-4.19 tree.
>> These alarms use main adc of the pmic. Separate temperature adc is not
>> supported yet.
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>> arch/arm64/boot/dts/qcom/pm8150.dtsi | 41 +++++++++++++++++++++++--
>> arch/arm64/boot/dts/qcom/pm8150b.dtsi | 43 +++++++++++++++++++++++++--
>> arch/arm64/boot/dts/qcom/pm8150l.dtsi | 43 +++++++++++++++++++++++++--
>> 3 files changed, 119 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/pm8150.dtsi b/arch/arm64/boot/dts/qcom/pm8150.dtsi
>> index c0b197458665..fee2db42f4cb 100644
>> --- a/arch/arm64/boot/dts/qcom/pm8150.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/pm8150.dtsi
>> @@ -30,6 +30,15 @@ pwrkey {
>> };
>> };
>>
>> + pm8150_temp: temp-alarm@2400 {
>> + compatible = "qcom,spmi-temp-alarm";
>> + reg = <0x2400>;
>> + interrupts = <0x0 0x24 0x0 IRQ_TYPE_EDGE_BOTH>;
>> + io-channels = <&pm8150_adc ADC5_DIE_TEMP>;
>> + io-channel-names = "thermal";
>> + #thermal-sensor-cells = <0>;
>> + };
>> +
>> pm8150_adc: adc@3100 {
>> compatible = "qcom,spmi-adc5";
>> reg = <0x3100>;
>> @@ -38,8 +47,6 @@ pm8150_adc: adc@3100 {
>> #io-channel-cells = <1>;
>> interrupts = <0x0 0x31 0x0 IRQ_TYPE_EDGE_RISING>;
>>
>> - status = "disabled";
>> -
>
> This should not be removed, rather than this please add enabled in you
> board dts file
>
>> ref-gnd@0 {
>> reg = <ADC5_REF_GND>;
>> qcom,pre-scaling = <1 1>;
>> @@ -85,3 +92,33 @@ pmic@1 {
>> #size-cells = <0>;
>> };
>> };
>> +
>> +&thermal_zones {
>> + pm8150_temp {
>> + polling-delay-passive = <0>;
>> + polling-delay = <0>;
>> +
>> + thermal-sensors = <&pm8150_temp>;
>> +
>> + trips {
>> + trip0 {
>> + temperature = <95000>;
>> + hysteresis = <0>;
>> + type = "passive";
>> + };
>> +
>> + trip1 {
>> + temperature = <115000>;
>> + hysteresis = <0>;
>> + type = "passive";
>> + };
>> +
>> + trip2 {
>> + temperature = <145000>;
>> + hysteresis = <0>;
>> + type = "passive";
>> + };
>> + };
>> +
>> + };
>
> Not sure about this, Amit..? Should this also not be in board dts?
>
> Similar comments on similar ones for rest of the patch as well..
I'm not so sure. This part of the configuration seems generic to me.
Unlike adc-tm config, which definitely goes to the board file.
I can split this into a separate pm8150-temp.dtsi file. Does that sound
better?
--
With best wishes
Dmitry
^ permalink raw reply
* Re: [PATCH 07/12] of/device: Add input id to of_dma_configure()
From: Lorenzo Pieralisi @ 2020-06-04 14:49 UTC (permalink / raw)
To: Rob Herring
Cc: moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
Robin Murphy, Joerg Roedel, Laurentiu Tudor, Linux IOMMU,
linux-acpi, devicetree, PCI, Rafael J. Wysocki, Hanjun Guo,
Bjorn Helgaas, Sudeep Holla, Catalin Marinas, Will Deacon,
Marc Zyngier, Makarand Pawagi, Diana Craciun
In-Reply-To: <CAL_JsqJw3wyiUrbd1AekwDc5+uqhHi9BwoB-rYpypUEGNgzCtw@mail.gmail.com>
On Thu, May 21, 2020 at 05:02:20PM -0600, Rob Herring wrote:
> On Thu, May 21, 2020 at 7:00 AM Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
> >
> > Devices sitting on proprietary busses have a device ID space that
> > is owned by the respective bus and related firmware bindings. In order
> > to let the generic OF layer handle the input translations to
> > an IOMMU id, for such busses the current of_dma_configure() interface
> > should be extended in order to allow the bus layer to provide the
> > device input id parameter - that is retrieved/assigned in bus
> > specific code and firmware.
> >
> > Augment of_dma_configure() to add an optional input_id parameter,
> > leaving current functionality unchanged.
> >
> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Robin Murphy <robin.murphy@arm.com>
> > Cc: Joerg Roedel <joro@8bytes.org>
> > Cc: Laurentiu Tudor <laurentiu.tudor@nxp.com>
> > ---
> > drivers/bus/fsl-mc/fsl-mc-bus.c | 4 ++-
> > drivers/iommu/of_iommu.c | 53 +++++++++++++++++++++------------
> > drivers/of/device.c | 8 +++--
> > include/linux/of_device.h | 16 ++++++++--
> > include/linux/of_iommu.h | 6 ++--
> > 5 files changed, 60 insertions(+), 27 deletions(-)
> >
> > diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c b/drivers/bus/fsl-mc/fsl-mc-bus.c
> > index 40526da5c6a6..8ead3f0238f2 100644
> > --- a/drivers/bus/fsl-mc/fsl-mc-bus.c
> > +++ b/drivers/bus/fsl-mc/fsl-mc-bus.c
> > @@ -118,11 +118,13 @@ static int fsl_mc_bus_uevent(struct device *dev, struct kobj_uevent_env *env)
> > static int fsl_mc_dma_configure(struct device *dev)
> > {
> > struct device *dma_dev = dev;
> > + struct fsl_mc_device *mc_dev = to_fsl_mc_device(dev);
> > + u32 input_id = mc_dev->icid;
> >
> > while (dev_is_fsl_mc(dma_dev))
> > dma_dev = dma_dev->parent;
> >
> > - return of_dma_configure(dev, dma_dev->of_node, 0);
> > + return of_dma_configure_id(dev, dma_dev->of_node, 0, &input_id);
> > }
> >
> > static ssize_t modalias_show(struct device *dev, struct device_attribute *attr,
> > diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> > index ad96b87137d6..4516d5bf6cc9 100644
> > --- a/drivers/iommu/of_iommu.c
> > +++ b/drivers/iommu/of_iommu.c
> > @@ -139,25 +139,53 @@ static int of_pci_iommu_init(struct pci_dev *pdev, u16 alias, void *data)
> > return err;
> > }
> >
> > -static int of_fsl_mc_iommu_init(struct fsl_mc_device *mc_dev,
> > - struct device_node *master_np)
> > +static int of_iommu_configure_dev_id(struct device_node *master_np,
> > + struct device *dev,
> > + const u32 *id)
>
> Should have read this patch before #6. I guess you could still make
> of_pci_iommu_init() call
> of_iommu_configure_dev_id.
Yes that makes sense, I will update it.
Thanks,
Lorenzo
^ permalink raw reply
* Re: [PATCH 06/12] of/iommu: Make of_map_rid() PCI agnostic
From: Lorenzo Pieralisi @ 2020-06-04 14:27 UTC (permalink / raw)
To: Rob Herring
Cc: moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
Joerg Roedel, Robin Murphy, Marc Zyngier, Linux IOMMU, linux-acpi,
devicetree, PCI, Rafael J. Wysocki, Hanjun Guo, Bjorn Helgaas,
Sudeep Holla, Catalin Marinas, Will Deacon, Makarand Pawagi,
Diana Craciun, Laurentiu Tudor
In-Reply-To: <CAL_JsqK5aiEMAZpqgTmrOq=HPRSFEoQWJrpR2YA0hziEtLMwrg@mail.gmail.com>
On Thu, May 21, 2020 at 04:47:19PM -0600, Rob Herring wrote:
> On Thu, May 21, 2020 at 7:00 AM Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
> >
> > There is nothing PCI specific (other than the RID - requester ID)
> > in the of_map_rid() implementation, so the same function can be
> > reused for input/output IDs mapping for other busses just as well.
> >
> > Rename the RID instances/names to a generic "id" tag and provide
> > an of_map_rid() wrapper function so that we can leave the existing
> > (and legitimate) callers unchanged.
>
> It's not all that clear to a casual observer that RID is a PCI thing,
> so I don't know that keeping it buys much. And there's only 3 callers.
Yes I agree - I think we can remove the _rid interface.
> > No functionality change intended.
> >
> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Joerg Roedel <joro@8bytes.org>
> > Cc: Robin Murphy <robin.murphy@arm.com>
> > Cc: Marc Zyngier <maz@kernel.org>
> > ---
> > drivers/iommu/of_iommu.c | 2 +-
> > drivers/of/base.c | 42 ++++++++++++++++++++--------------------
> > include/linux/of.h | 17 +++++++++++++++-
> > 3 files changed, 38 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> > index 20738aacac89..ad96b87137d6 100644
> > --- a/drivers/iommu/of_iommu.c
> > +++ b/drivers/iommu/of_iommu.c
> > @@ -145,7 +145,7 @@ static int of_fsl_mc_iommu_init(struct fsl_mc_device *mc_dev,
> > struct of_phandle_args iommu_spec = { .args_count = 1 };
> > int err;
> >
> > - err = of_map_rid(master_np, mc_dev->icid, "iommu-map",
> > + err = of_map_id(master_np, mc_dev->icid, "iommu-map",
>
> I'm not sure this is an improvement because I'd refactor this function
> and of_pci_iommu_init() into a single function:
>
> of_bus_iommu_init(struct device *dev, struct device_node *np, u32 id)
>
> Then of_pci_iommu_init() becomes:
>
> of_pci_iommu_init()
> {
> return of_bus_iommu_init(info->dev, info->np, alias);
> }
>
> And replace of_fsl_mc_iommu_init call with:
> err = of_bus_iommu_init(dev, master_np, to_fsl_mc_device(dev)->icid);
I will follow up on this on patch 7.
> > "iommu-map-mask", &iommu_spec.np,
> > iommu_spec.args);
> > if (err)
> > diff --git a/drivers/of/base.c b/drivers/of/base.c
> > index ae03b1218b06..e000e17bd602 100644
> > --- a/drivers/of/base.c
> > +++ b/drivers/of/base.c
> > @@ -2201,15 +2201,15 @@ int of_find_last_cache_level(unsigned int cpu)
> > }
> >
> > /**
> > - * of_map_rid - Translate a requester ID through a downstream mapping.
> > + * of_map_id - Translate a requester ID through a downstream mapping.
>
> Still a requester ID?
Fixed, thanks.
Lorenzo
^ permalink raw reply
* Re: [PATCH 0/6] arm64: dts: qcom: smmu/USB nodes and HDK855/HDK865 dts
From: Jonathan Marek @ 2020-06-04 14:06 UTC (permalink / raw)
To: Manivannan Sadhasivam
Cc: linux-arm-msm, Andy Gross, Bjorn Andersson,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list, Rob Herring
In-Reply-To: <20200604135221.GH16719@Mani-XPS-13-9360>
On 6/4/20 9:52 AM, Manivannan Sadhasivam wrote:
> Hi,
>
> On Sat, May 23, 2020 at 10:38:06PM -0400, Jonathan Marek wrote:
>> Add dts nodes for apps_smmu and USB for both sm8150 and sm8250.
>>
>
> I've tested this series on an SM8250 based board and able to get Type C (USB0)
> working. There are also couple of Type A ports (USB1) on that board behind a
> USB hub. It is probing fine but I don't see any activity while connecting a
> USB device. Will continue to debug and once I get them working, I'll add my
> Tested-by tag.
>
HDK865 also has a couple Type A ports, I am using them with devices
already plugged in during boot and I haven't hit a problem like that,
but I think I've seen the same issue when hotplugging. IIRC the behavior
was a bit weird, like plugging a device in the Type A port (USB1)
nothing would happen, but unplugging/replugging the type C port (USB0)
would cause the Type A port device to start working..
Have you tried with the devices already plugged in before booting?
> Thanks,
> Mani
>
>> Also add initial dts files for HDK855 and HDK865, based on mtp dts, with a
>> few changes. Notably, the HDK865 dts has regulator config changed a bit based
>> on downstream (I think sm8250-mtp.dts is wrong and copied too much from sm8150).
>>
>> Jonathan Marek (6):
>> arm64: dts: qcom: sm8150: add apps_smmu node
>> arm64: dts: qcom: sm8250: add apps_smmu node
>> arm64: dts: qcom: sm8150: Add secondary USB and PHY nodes
>> arm64: dts: qcom: sm8250: Add USB and PHY device nodes
>> arm64: dts: qcom: add sm8150 hdk dts
>> arm64: dts: qcom: add sm8250 hdk dts
>>
>> arch/arm64/boot/dts/qcom/Makefile | 2 +
>> arch/arm64/boot/dts/qcom/sm8150-hdk.dts | 461 ++++++++++++++++++++++++
>> arch/arm64/boot/dts/qcom/sm8150.dtsi | 180 +++++++++
>> arch/arm64/boot/dts/qcom/sm8250-hdk.dts | 454 +++++++++++++++++++++++
>> arch/arm64/boot/dts/qcom/sm8250.dtsi | 287 +++++++++++++++
>> 5 files changed, 1384 insertions(+)
>> create mode 100644 arch/arm64/boot/dts/qcom/sm8150-hdk.dts
>> create mode 100644 arch/arm64/boot/dts/qcom/sm8250-hdk.dts
>>
>> --
>> 2.26.1
>>
^ permalink raw reply
* Re: [PATCH 2/3] ARM: dts: bcm2711: Update SPI nodes compatible strings
From: Mark Brown @ 2020-06-04 14:05 UTC (permalink / raw)
To: Lukas Wunner
Cc: Florian Fainelli, linux-kernel, Rob Herring,
Nicolas Saenz Julienne, Ray Jui, Scott Branden,
maintainer:BROADCOM BCM281XX/BCM11XXX/BCM216XX ARM ARCHITE...,
open list:SPI SUBSYSTEM,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
Martin Sperl
In-Reply-To: <20200604112112.b3k4wrftckndscu6@wunner.de>
[-- Attachment #1: Type: text/plain, Size: 922 bytes --]
On Thu, Jun 04, 2020 at 01:21:12PM +0200, Lukas Wunner wrote:
> On Thu, Jun 04, 2020 at 12:13:25PM +0100, Mark Brown wrote:
> > Regardless of what's going on with the interrupts the compatible string
> > should reflect the IP version so unless for some reason someone taped
> > out two different versions of the IP it seems odd that the compatible
> > strings would vary within a given SoC.
> Hm. I guess it may be possible to search the DT for other devices
> sharing the same interrupt line and thereby determine whether
> IRQF_SHARED is necessary. The helper to perform this search could
> live in drivers/of/irq.c as I imagine it might be useful in general.
That's another option, yeah - it'd be DT specific but it seems neater
than a property and much more tractable than trying to dance around
doing this in genirq (where we'd end up with callbacks when the second
device registers or something else horrible).
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH V3 5/8] phy: tegra: xusb: Add soc ops API to enable UTMI PAD protection
From: Thierry Reding @ 2020-06-04 13:58 UTC (permalink / raw)
To: Kishon Vijay Abraham I
Cc: Nagarjuna Kristam, balbi, gregkh, jonathanh, mark.rutland,
robh+dt, devicetree, linux-tegra, linux-usb, linux-kernel
In-Reply-To: <db698a53-c5f6-d03f-edf0-f4fb38963e1f@ti.com>
[-- Attachment #1: Type: text/plain, Size: 594 bytes --]
On Mon, May 18, 2020 at 05:54:03PM +0530, Kishon Vijay Abraham I wrote:
> Thierry,
>
> On 5/14/2020 11:52 AM, Nagarjuna Kristam wrote:
> > When USB charger is enabled, UTMI PAD needs to be protected according
> > to the direction and current level. Add support for the same on Tegra210
> > and Tegra186.
> >
> > Signed-off-by: Nagarjuna Kristam <nkristam@nvidia.com>
>
> Can you give your Acked by for pending patches in this series?
I went through these patches one more time and I think they're all good
now. I've acked the ones that didn't have my Acked-by yet.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH V3 6/8] phy: tegra: xusb: Add support for charger detect
From: Thierry Reding @ 2020-06-04 13:56 UTC (permalink / raw)
To: Nagarjuna Kristam
Cc: balbi, gregkh, jonathanh, mark.rutland, robh+dt, kishon,
devicetree, linux-tegra, linux-usb, linux-kernel
In-Reply-To: <1589437363-16727-7-git-send-email-nkristam@nvidia.com>
[-- Attachment #1: Type: text/plain, Size: 1083 bytes --]
On Thu, May 14, 2020 at 11:52:41AM +0530, Nagarjuna Kristam wrote:
> Perform charger-detect operation if corresponding dt property is enabled.
> Update usb-phy with the detected charger state and max current values.
> Register charger-detect API's of usb-phy to provide needed functionalities.
>
> Signed-off-by: Nagarjuna Kristam <nkristam@nvidia.com>
> ---
> V3:
> - Allighed functions and its arguments.
> - replaced spaced by tabs for MACRO definition allignments.
> - Unified primary and secondary charger detect API's.
> - Used readl_poll_timeout instead of while loop condition check for register.
> - Fixed other comments as per inputs from Thierry.
> ---
> V2:
> - Patch re-based.
> ---
> drivers/phy/tegra/Makefile | 2 +-
> drivers/phy/tegra/cd.c | 283 +++++++++++++++++++++++++++++++++++++++++++++
> drivers/phy/tegra/xusb.c | 80 +++++++++++++
> drivers/phy/tegra/xusb.h | 7 ++
> 4 files changed, 371 insertions(+), 1 deletion(-)
> create mode 100644 drivers/phy/tegra/cd.c
Acked-by: Thierry Reding <treding@nvidia.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH V3 5/8] phy: tegra: xusb: Add soc ops API to enable UTMI PAD protection
From: Thierry Reding @ 2020-06-04 13:55 UTC (permalink / raw)
To: Nagarjuna Kristam
Cc: balbi, gregkh, jonathanh, mark.rutland, robh+dt, kishon,
devicetree, linux-tegra, linux-usb, linux-kernel
In-Reply-To: <1589437363-16727-6-git-send-email-nkristam@nvidia.com>
[-- Attachment #1: Type: text/plain, Size: 771 bytes --]
On Thu, May 14, 2020 at 11:52:40AM +0530, Nagarjuna Kristam wrote:
> When USB charger is enabled, UTMI PAD needs to be protected according
> to the direction and current level. Add support for the same on Tegra210
> and Tegra186.
>
> Signed-off-by: Nagarjuna Kristam <nkristam@nvidia.com>
> ---
> V3:
> - Alligned function and its arguments.
> - Fixed other comments from Thierry.
> ---
> V2:
> - Commit message coorected.
> - Patch re-based.
> ---
> drivers/phy/tegra/xusb-tegra186.c | 40 +++++++++++++++++++++++++++++++++++++++
> drivers/phy/tegra/xusb-tegra210.c | 32 +++++++++++++++++++++++++++++++
> drivers/phy/tegra/xusb.h | 13 +++++++++++++
> 3 files changed, 85 insertions(+)
Acked-by: Thierry Reding <treding@nvidia.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH V3 2/8] usb: gadget: tegra-xudc: Add vbus_draw support
From: Thierry Reding @ 2020-06-04 13:54 UTC (permalink / raw)
To: Nagarjuna Kristam
Cc: balbi, gregkh, jonathanh, mark.rutland, robh+dt, kishon,
devicetree, linux-tegra, linux-usb, linux-kernel
In-Reply-To: <1589437363-16727-3-git-send-email-nkristam@nvidia.com>
[-- Attachment #1: Type: text/plain, Size: 493 bytes --]
On Thu, May 14, 2020 at 11:52:37AM +0530, Nagarjuna Kristam wrote:
> Register vbus_draw to gadget ops and update corresponding vbus
> draw current to usb_phy.
>
> Signed-off-by: Nagarjuna Kristam <nkristam@nvidia.com>
> ---
> V3:
> - Propogated usb_phy->set_power error to vbus_draw caller.
> ---
> V2:
> - Patch re-based.
> ---
> drivers/usb/gadget/udc/tegra-xudc.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
Acked-by: Thierry Reding <treding@nvidia.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 0/6] arm64: dts: qcom: smmu/USB nodes and HDK855/HDK865 dts
From: Manivannan Sadhasivam @ 2020-06-04 13:52 UTC (permalink / raw)
To: Jonathan Marek
Cc: linux-arm-msm, Andy Gross, Bjorn Andersson,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list, Rob Herring
In-Reply-To: <20200524023815.21789-1-jonathan@marek.ca>
Hi,
On Sat, May 23, 2020 at 10:38:06PM -0400, Jonathan Marek wrote:
> Add dts nodes for apps_smmu and USB for both sm8150 and sm8250.
>
I've tested this series on an SM8250 based board and able to get Type C (USB0)
working. There are also couple of Type A ports (USB1) on that board behind a
USB hub. It is probing fine but I don't see any activity while connecting a
USB device. Will continue to debug and once I get them working, I'll add my
Tested-by tag.
Thanks,
Mani
> Also add initial dts files for HDK855 and HDK865, based on mtp dts, with a
> few changes. Notably, the HDK865 dts has regulator config changed a bit based
> on downstream (I think sm8250-mtp.dts is wrong and copied too much from sm8150).
>
> Jonathan Marek (6):
> arm64: dts: qcom: sm8150: add apps_smmu node
> arm64: dts: qcom: sm8250: add apps_smmu node
> arm64: dts: qcom: sm8150: Add secondary USB and PHY nodes
> arm64: dts: qcom: sm8250: Add USB and PHY device nodes
> arm64: dts: qcom: add sm8150 hdk dts
> arm64: dts: qcom: add sm8250 hdk dts
>
> arch/arm64/boot/dts/qcom/Makefile | 2 +
> arch/arm64/boot/dts/qcom/sm8150-hdk.dts | 461 ++++++++++++++++++++++++
> arch/arm64/boot/dts/qcom/sm8150.dtsi | 180 +++++++++
> arch/arm64/boot/dts/qcom/sm8250-hdk.dts | 454 +++++++++++++++++++++++
> arch/arm64/boot/dts/qcom/sm8250.dtsi | 287 +++++++++++++++
> 5 files changed, 1384 insertions(+)
> create mode 100644 arch/arm64/boot/dts/qcom/sm8150-hdk.dts
> create mode 100644 arch/arm64/boot/dts/qcom/sm8250-hdk.dts
>
> --
> 2.26.1
>
^ permalink raw reply
* [PATCH v10 4/8] tpm: tpm_tis: Rewrite "tpm_tis_req_canceled()"
From: amirmizi6 @ 2020-06-04 13:47 UTC (permalink / raw)
To: Eyal.Cohen, jarkko.sakkinen, oshrialkoby85, alexander.steffen,
robh+dt, "benoit.houyere, peterhuewe, christophe-h.richard,
jgg, arnd, gregkh
Cc: devicetree, linux-kernel, linux-integrity, oshri.alkoby,
tmaimon77, gcwilson, kgoldman, Dan.Morav, oren.tanami,
shmulik.hager, amir.mizinski, Amir Mizinski
In-Reply-To: <20200604134713.157951-1-amirmizi6@gmail.com>
From: Amir Mizinski <amirmizi6@gmail.com>
Using this function while reading/writing data resulted in an aborted
operation.
After investigating the issue according to the TCG TPM Profile (PTP)
Specifications, I found that "request to cancel" should occur only if
TPM_STS.commandReady bit is lit.
Note that i couldn't find a case where the present condition
(in the linux kernel) is valid, so I'm removing the case for
"TPM_VID_WINBOND" since we have no need for it.
Also, the default comparison is wrong. Only cmdReady bit needs to be
compared instead of the full lower status register byte.
Fixes: 1f866057291f (tpm: Fix cancellation of TPM commands (polling mode))
Signed-off-by: Amir Mizinski <amirmizi6@gmail.com>
---
drivers/char/tpm/tpm_tis_core.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 202714d..9d90b9a 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -687,13 +687,11 @@ static bool tpm_tis_req_canceled(struct tpm_chip *chip, u8 status)
struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
switch (priv->manufacturer_id) {
- case TPM_VID_WINBOND:
- return ((status == TPM_STS_VALID) ||
- (status == (TPM_STS_VALID | TPM_STS_COMMAND_READY)));
case TPM_VID_STM:
return (status == (TPM_STS_VALID | TPM_STS_COMMAND_READY));
default:
- return (status == TPM_STS_COMMAND_READY);
+ return ((status & TPM_STS_COMMAND_READY) ==
+ TPM_STS_COMMAND_READY);
}
}
--
2.7.4
^ permalink raw reply related
* [PATCH v10 8/8] tpm: tpm_tis: add tpm_tis_i2c driver
From: amirmizi6 @ 2020-06-04 13:47 UTC (permalink / raw)
To: Eyal.Cohen, jarkko.sakkinen, oshrialkoby85, alexander.steffen,
robh+dt, "benoit.houyere, peterhuewe, christophe-h.richard,
jgg, arnd, gregkh
Cc: devicetree, linux-kernel, linux-integrity, oshri.alkoby,
tmaimon77, gcwilson, kgoldman, Dan.Morav, oren.tanami,
shmulik.hager, amir.mizinski, Amir Mizinski, Eddie James,
Joel Stanley
In-Reply-To: <20200604134713.157951-1-amirmizi6@gmail.com>
From: Amir Mizinski <amirmizi6@gmail.com>
Implements the functionality needed to communicate with an I2C TPM
according to the TCG TPM I2C Interface Specification.
Signed-off-by: Amir Mizinski <amirmizi6@gmail.com>
Tested-by: Eddie James <eajames@linux.ibm.com>
Tested-by: Joel Stanley <joel@jms.id.au>
---
drivers/char/tpm/Kconfig | 12 ++
drivers/char/tpm/Makefile | 1 +
drivers/char/tpm/tpm_tis_i2c.c | 292 +++++++++++++++++++++++++++++++++++++++++
3 files changed, 305 insertions(+)
create mode 100644 drivers/char/tpm/tpm_tis_i2c.c
diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
index aacdeed..2116d94 100644
--- a/drivers/char/tpm/Kconfig
+++ b/drivers/char/tpm/Kconfig
@@ -74,6 +74,18 @@ config TCG_TIS_SPI_CR50
If you have a H1 secure module running Cr50 firmware on SPI bus,
say Yes and it will be accessible from within Linux.
+config TCG_TIS_I2C
+ tristate "TPM I2C Interface Specification"
+ depends on I2C
+ select CRC_CCITT
+ select TCG_TIS_CORE
+ help
+ If you have a TPM security chip which is connected to a regular
+ I2C master (i.e. most embedded platforms) that is compliant with the
+ TCG TPM I2C Interface Specification say Yes and it will be accessible from
+ within Linux. To compile this driver as a module, choose M here;
+ the module will be called tpm_tis_i2c.
+
config TCG_TIS_I2C_ATMEL
tristate "TPM Interface Specification 1.2 Interface (I2C - Atmel)"
depends on I2C
diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index 9567e51..97999cf 100644
--- a/drivers/char/tpm/Makefile
+++ b/drivers/char/tpm/Makefile
@@ -26,6 +26,7 @@ obj-$(CONFIG_TCG_TIS_SPI) += tpm_tis_spi.o
tpm_tis_spi-y := tpm_tis_spi_main.o
tpm_tis_spi-$(CONFIG_TCG_TIS_SPI_CR50) += tpm_tis_spi_cr50.o
+obj-$(CONFIG_TCG_TIS_I2C) += tpm_tis_i2c.o
obj-$(CONFIG_TCG_TIS_I2C_ATMEL) += tpm_i2c_atmel.o
obj-$(CONFIG_TCG_TIS_I2C_INFINEON) += tpm_i2c_infineon.o
obj-$(CONFIG_TCG_TIS_I2C_NUVOTON) += tpm_i2c_nuvoton.o
diff --git a/drivers/char/tpm/tpm_tis_i2c.c b/drivers/char/tpm/tpm_tis_i2c.c
new file mode 100644
index 0000000..4c9bad0
--- /dev/null
+++ b/drivers/char/tpm/tpm_tis_i2c.c
@@ -0,0 +1,292 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2014-2019 Nuvoton Technology corporation
+ *
+ * TPM TIS I2C
+ *
+ * TPM TIS I2C Device Driver Interface for devices that implement the TPM I2C
+ * Interface defined by TCG PC Client Platform TPM Profile (PTP) Specification
+ * Revision 01.03 v22 at www.trustedcomputinggroup.org
+ */
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/slab.h>
+#include <linux/interrupt.h>
+#include <linux/wait.h>
+#include <linux/acpi.h>
+#include <linux/freezer.h>
+#include <linux/crc-ccitt.h>
+
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/gpio.h>
+#include <linux/of_irq.h>
+#include <linux/of_gpio.h>
+#include <linux/tpm.h>
+#include "tpm.h"
+#include "tpm_tis_core.h"
+
+#define TPM_LOC_SEL 0x04
+#define TPM_I2C_INTERFACE_CAPABILITY 0x30
+#define TPM_I2C_DEVICE_ADDRESS 0x38
+#define TPM_DATA_CSUM_ENABLE 0x40
+#define TPM_DATA_CSUM 0x44
+#define TPM_I2C_DID_VID 0x48
+#define TPM_I2C_RID 0x4C
+
+//#define I2C_IS_TPM2 1
+
+struct tpm_tis_i2c_phy {
+ struct tpm_tis_data priv;
+ struct i2c_client *i2c_client;
+ bool data_csum;
+ u8 *iobuf;
+};
+
+static inline struct tpm_tis_i2c_phy *to_tpm_tis_i2c_phy(struct tpm_tis_data
+ *data)
+{
+ return container_of(data, struct tpm_tis_i2c_phy, priv);
+}
+
+static u8 address_to_register(u32 addr)
+{
+ addr &= 0xFFF;
+
+ switch (addr) {
+ // adapt register addresses that have changed compared to
+ // older TIS versions
+ case TPM_ACCESS(0):
+ return 0x04;
+ case TPM_LOC_SEL:
+ return 0x00;
+ case TPM_DID_VID(0):
+ return 0x48;
+ case TPM_RID(0):
+ return 0x4C;
+ default:
+ return addr;
+ }
+}
+
+static int tpm_tis_i2c_read_bytes(struct tpm_tis_data *data, u32 addr,
+ u16 len, u8 *result)
+{
+ struct tpm_tis_i2c_phy *phy = to_tpm_tis_i2c_phy(data);
+ int ret = 0;
+ int i = 0;
+ u8 reg = address_to_register(addr);
+ struct i2c_msg msgs[] = {
+ {
+ .addr = phy->i2c_client->addr,
+ .len = sizeof(reg),
+ .buf = ®,
+ },
+ {
+ .addr = phy->i2c_client->addr,
+ .len = len,
+ .buf = result,
+ .flags = I2C_M_RD,
+ },
+ };
+
+ do {
+ ret = i2c_transfer(phy->i2c_client->adapter, msgs,
+ ARRAY_SIZE(msgs));
+ usleep_range(250, 300); // wait default GUARD_TIME of 250µs
+
+ } while (ret < 0 && i++ < TPM_RETRY);
+
+ if (ret < 0)
+ return ret;
+
+ return 0;
+}
+
+static int tpm_tis_i2c_write_bytes(struct tpm_tis_data *data, u32 addr,
+ u16 len, const u8 *value)
+{
+ struct tpm_tis_i2c_phy *phy = to_tpm_tis_i2c_phy(data);
+ int ret = 0;
+ int i = 0;
+
+ if (phy->iobuf) {
+ if (len > TPM_BUFSIZE - 1)
+ return -EIO;
+
+ phy->iobuf[0] = address_to_register(addr);
+ memcpy(phy->iobuf + 1, value, len);
+
+ {
+ struct i2c_msg msgs[] = {
+ {
+ .addr = phy->i2c_client->addr,
+ .len = len + 1,
+ .buf = phy->iobuf,
+ },
+ };
+
+ do {
+ ret = i2c_transfer(phy->i2c_client->adapter,
+ msgs, ARRAY_SIZE(msgs));
+ // wait default GUARD_TIME of 250µs
+ usleep_range(250, 300);
+ } while (ret < 0 && i++ < TPM_RETRY);
+ }
+ } else {
+ u8 reg = address_to_register(addr);
+
+ struct i2c_msg msgs[] = {
+ {
+ .addr = phy->i2c_client->addr,
+ .len = sizeof(reg),
+ .buf = ®,
+ },
+ {
+ .addr = phy->i2c_client->addr,
+ .len = len,
+ .buf = (u8 *)value,
+ .flags = I2C_M_NOSTART,
+ },
+ };
+ do {
+ ret = i2c_transfer(phy->i2c_client->adapter, msgs,
+ ARRAY_SIZE(msgs));
+ // wait default GUARD_TIME of 250µs
+ usleep_range(250, 300);
+ } while (ret < 0 && i++ < TPM_RETRY);
+ }
+
+ if (ret < 0)
+ return ret;
+
+ return 0;
+}
+
+static bool tpm_tis_i2c_verify_data_integrity(struct tpm_tis_data *data,
+ const u8 *buf, size_t len)
+{
+ struct tpm_tis_i2c_phy *phy = to_tpm_tis_i2c_phy(data);
+ u16 crc, crc_tpm;
+ int rc;
+
+ if (phy->data_csum) {
+ crc = crc_ccitt(0x0000, buf, len);
+ rc = tpm_tis_read16(data, TPM_DATA_CSUM, &crc_tpm);
+ if (rc < 0)
+ return false;
+
+ crc_tpm = be16_to_cpu(crc_tpm);
+ return crc == crc_tpm;
+ }
+
+ return true;
+}
+
+static SIMPLE_DEV_PM_OPS(tpm_tis_pm, tpm_pm_suspend, tpm_tis_resume);
+
+static int csum_state_store(struct tpm_tis_data *data, u8 new_state)
+{
+ struct tpm_tis_i2c_phy *phy = to_tpm_tis_i2c_phy(data);
+ u8 cur_state;
+ int rc;
+
+ rc = tpm_tis_i2c_write_bytes(&phy->priv, TPM_DATA_CSUM_ENABLE,
+ 1, &new_state);
+ if (rc < 0)
+ return rc;
+
+ rc = tpm_tis_i2c_read_bytes(&phy->priv, TPM_DATA_CSUM_ENABLE,
+ 1, &cur_state);
+ if (rc < 0)
+ return rc;
+
+ if (new_state == cur_state)
+ phy->data_csum = (bool)new_state;
+
+ return rc;
+}
+
+static const struct tpm_tis_phy_ops tpm_i2c_phy_ops = {
+ .read_bytes = tpm_tis_i2c_read_bytes,
+ .write_bytes = tpm_tis_i2c_write_bytes,
+ .verify_data_integrity = tpm_tis_i2c_verify_data_integrity,
+};
+
+static int tpm_tis_i2c_probe(struct i2c_client *dev,
+ const struct i2c_device_id *id)
+{
+ struct tpm_tis_i2c_phy *phy;
+ int rc;
+ int crc_checksum = 0;
+ const u8 loc_init = 0;
+ struct device_node *np;
+
+ phy = devm_kzalloc(&dev->dev, sizeof(struct tpm_tis_i2c_phy),
+ GFP_KERNEL);
+ if (!phy)
+ return -ENOMEM;
+
+ phy->i2c_client = dev;
+
+ if (!i2c_check_functionality(dev->adapter, I2C_FUNC_NOSTART)) {
+ phy->iobuf = devm_kmalloc(&dev->dev, TPM_BUFSIZE, GFP_KERNEL);
+ if (!phy->iobuf)
+ return -ENOMEM;
+ }
+
+ // select locality 0 (the driver will access only via locality 0)
+ rc = tpm_tis_i2c_write_bytes(&phy->priv, TPM_LOC_SEL, 1, &loc_init);
+ if (rc < 0)
+ return rc;
+
+ // set CRC checksum calculation enable
+ np = dev->dev.of_node;
+ if (of_property_read_bool(np, "crc-checksum"))
+ crc_checksum = 1;
+
+ rc = csum_state_store(&phy->priv, crc_checksum);
+ if (rc < 0)
+ return rc;
+
+ return tpm_tis_core_init(&dev->dev, &phy->priv, -1, &tpm_i2c_phy_ops,
+ NULL);
+}
+
+static const struct i2c_device_id tpm_tis_i2c_id[] = {
+ {"tpm_tis_i2c", 0},
+ {}
+};
+MODULE_DEVICE_TABLE(i2c, tpm_tis_i2c_id);
+
+static const struct of_device_id of_tis_i2c_match[] = {
+ { .compatible = "nuvoton,npct75x", },
+ { .compatible = "tcg,tpm-tis-i2c", },
+ {}
+};
+MODULE_DEVICE_TABLE(of, of_tis_i2c_match);
+
+static const struct acpi_device_id acpi_tis_i2c_match[] = {
+ {"SMO0768", 0},
+ {}
+};
+MODULE_DEVICE_TABLE(acpi, acpi_tis_i2c_match);
+
+static struct i2c_driver tpm_tis_i2c_driver = {
+ .driver = {
+ .owner = THIS_MODULE,
+ .name = "tpm_tis_i2c",
+ .pm = &tpm_tis_pm,
+ .of_match_table = of_match_ptr(of_tis_i2c_match),
+ .acpi_match_table = ACPI_PTR(acpi_tis_i2c_match),
+ },
+ .probe = tpm_tis_i2c_probe,
+ .id_table = tpm_tis_i2c_id,
+};
+
+module_i2c_driver(tpm_tis_i2c_driver);
+
+MODULE_DESCRIPTION("TPM Driver");
+MODULE_LICENSE("GPL");
--
2.7.4
^ permalink raw reply related
* [PATCH v10 7/8] tpm: Add YAML schema for TPM TIS I2C options
From: amirmizi6 @ 2020-06-04 13:47 UTC (permalink / raw)
To: Eyal.Cohen, jarkko.sakkinen, oshrialkoby85, alexander.steffen,
robh+dt, "benoit.houyere, peterhuewe, christophe-h.richard,
jgg, arnd, gregkh
Cc: devicetree, linux-kernel, linux-integrity, oshri.alkoby,
tmaimon77, gcwilson, kgoldman, Dan.Morav, oren.tanami,
shmulik.hager, amir.mizinski, Amir Mizinski, Rob Herring
In-Reply-To: <20200604134713.157951-1-amirmizi6@gmail.com>
From: Amir Mizinski <amirmizi6@gmail.com>
Added a YAML schema to support tpm tis i2c related dt-bindings for the I2c
PTP based physical layer.
This patch adds the documentation for corresponding device tree bindings of
I2C based Physical TPM.
Refer to the 'I2C Interface Definition' section in
'TCG PC Client PlatformTPMProfile(PTP) Specification' publication
for specification.
Signed-off-by: Amir Mizinski <amirmizi6@gmail.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
.../bindings/security/tpm/tpm-tis-i2c.yaml | 50 ++++++++++++++++++++++
1 file changed, 50 insertions(+)
create mode 100644 Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml
diff --git a/Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml b/Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml
new file mode 100644
index 0000000..68b13d5
--- /dev/null
+++ b/Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml
@@ -0,0 +1,50 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/security/tpm/tpm-tis-i2c.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: I2C PTP based TPM Device Tree Bindings
+
+maintainers:
+ - Amir Mizinski <amirmizi6@gmail.com>
+
+description:
+ Device Tree Bindings for I2C based Trusted Platform Module(TPM).
+
+properties:
+ compatible:
+ items:
+ - enum:
+ # Nuvoton's Trusted Platform Module (TPM) (NPCT75x)
+ - nuvoton,npct75x
+ - const: tcg,tpm-tis-i2c
+
+ reg:
+ maxItems: 1
+
+ interrupt:
+ maxItems: 1
+
+ crc-checksum:
+ $ref: /schemas/types.yaml#/definitions/flag
+ description:
+ Set this flag to enable CRC checksum.
+
+required:
+ - compatible
+ - reg
+
+examples:
+ - |
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ tpm@2e {
+ compatible = "nuvoton,npct75x", "tcg,tpm-tis-i2c";
+ reg = <0x2e>;
+ crc-checksum;
+ };
+ };
+...
--
2.7.4
^ permalink raw reply related
* [PATCH v10 0/8] Add tpm i2c ptp driver
From: amirmizi6 @ 2020-06-04 13:47 UTC (permalink / raw)
To: Eyal.Cohen, jarkko.sakkinen, oshrialkoby85, alexander.steffen,
robh+dt, "benoit.houyere, peterhuewe, christophe-h.richard,
jgg, arnd, gregkh
Cc: devicetree, linux-kernel, linux-integrity, oshri.alkoby,
tmaimon77, gcwilson, kgoldman, Dan.Morav, oren.tanami,
shmulik.hager, amir.mizinski, Amir Mizinski
From: Amir Mizinski <amirmizi6@gmail.com>
This patch set adds support for TPM devices that implement the I2C.
Interface defined by TCG PTP specification:
https://trustedcomputinggroup.org/wp-content/uploads/TCG_PC_Client_Platform_TPM_Profile_PTP_2.0_r1.03_v22.pdf
The driver was tested on Raspberry-Pie 3, using Nuvoton NPCT75X TPM.
Interrupts are not implemented yet, preparing it for the next patch.
This patch is based on initial work by oshri Alkoby, Alexander Steffen and Christophe Ricard
Changes since version 1:
-"char:tpm:Add check_data handle to tpm_tis_phy_ops in order to check data integrity"
- Fixed and extended commit description.
- Fixed an issue regarding handling max retries.
-"dt-bindings: tpm: Add YAML schema for TPM TIS I2C options":
-Converted "tpm_tis_i2c.txt" to "tpm-tis-i2c.yaml".
- Renamed "tpm_tis-i2c" to "tpm-tis-i2c".
- Removed interrupts properties.
-"char: tpm: add tpm_tis_i2c driver"
- Replaced "tpm_tis-i2c" with "tpm-tis-i2c" in "tpm_tis_i2c.c".
Addressed comments from:
- Jarkko Sakkinen: https://patchwork.kernel.org/patch/11236257/
- Rob Herring: https://patchwork.kernel.org/patch/11236253/
Changes since version 2:
- Added 2 new commits with improvements suggested by Benoit Houyere.
-"Fix expected bit handling and send all bytes in one shot without last byte in exception"
-"Handle an exception for TPM Firmware Update mode."
- Updated patch to latest v5.5
-"dt-bindings: tpm: Add YAML schema for TPM TIS I2C options"
- Added "interrupts" and "crc-checksum" to properties.
- Updated binding description and commit info.
-"char: tpm: add tpm_tis_i2c driver" (suggested by Benoit Houyere)
- Added repeat I2C frame after NACK.
- Checksum I2C feature activation in DTS file configuration.
Addressed comments from:
- Rob Herring: https://lore.kernel.org/patchwork/patch/1161287/
Changes since version 3:
- Updated patch to latest v5.6
- Updated commits headlines and development credit format by Jarkko Sakkinen suggestion
-"tpm: tpm_tis: Make implementation of read16 read32 write32 optional"
- Updated commit description.
-"dt-bindings: tpm: Add YAML schema for TPM TIS I2C options"
- Fixed 'make dt_binding_check' errors on YAML file.
- Removed interrupts from required and examples since there is no use for them in current patch.
Addressed comments from:
- Jarkko Sakkinen: https://lore.kernel.org/patchwork/patch/1192101/
- Rob Herring: https://lore.kernel.org/patchwork/patch/1192099/
Changes since version 4:
-"tpm: tpm_tis: Make implementation of read16 read32 write32 optional"
-Added a "Reviewed-by" tag:
-"tpm: tpm_tis: Add check_data handle to tpm_tis_phy_ops in order to check data integrity"
-Fixed credit typos.
-"tpm: tpm_tis: rewrite "tpm_tis_req_canceled()""
-Added fixes tag and removed changes for STM.
-"tpm: tpm_tis: Fix expected bit handling and send all bytes in one shot without last byte in exception"
-Fixed typos, edited description to be clearer, and added a "Suggested-by" tag.
-"tpm: Handle an exception for TPM Firmware Update mode."
-Added a "Suggested-by" tag.
-"dt-bindings: tpm: Add YAML schema for TPM TIS I2C options"
-Fixed 'make dt_binding_check' errors.
-"tpm: tpm_tis: add tpm_tis_i2c driver"
-Added tested-by tag by Eddie James.
-Fixed indent in Kconfig file.
-Fixed 'MODULE_DESCRIPTION'.
Addressed comments from:
- Jarkko Sakkinen: https://patchwork.kernel.org/patch/11467645/
https://patchwork.kernel.org/patch/11467655/
https://patchwork.kernel.org/patch/11467643/
https://patchwork.kernel.org/patch/11467659/
https://patchwork.kernel.org/patch/11467651/
- Rob Herring: https://patchwork.kernel.org/patch/11467653/
- Randy Dunlap: https://patchwork.kernel.org/patch/11467651/
- Eddie James: https://lore.kernel.org/patchwork/patch/1192104/
Changes since version 5:
-"tpm: tpm_tis: Add check_data handle to tpm_tis_phy_ops"
-Updated short description and fixed long description to be more clear.
Addressed comments from:
- Jarkko Sakkinen: https://lkml.org/lkml/2020/4/6/748
Changes since version 6:
-"tpm: tpm_tis: Make implementation of read16, read32 and write32 optional"
-Fixed short description.
-fixed long description proofreading issues.
-"tpm: tpm_tis: Add check_data handle to tpm_tis_phy_ops"
-Fixed long description by Jarkko comments and proofreading issues.
-Replaced "check_data" with verify_data_integrity".
-New line before return statement.
-"tpm: tpm_tis: rewrite "tpm_tis_req_canceled()"
-Fixed line over 80 characters.
-fixed long description proofreading issues.
-" tpm: tpm_tis: Fix expected bit handling and send all bytes in one shot"
-fixed long description proofreading issues.
-"dt-bindings: tpm: Add YAML schema for TPM TIS I2C option"
-Replaced "tpm-tis-i2c@2e" with "tpm_tis@2e".
-Fixed CRC_Checksum description.
-"tpm: tpm_tis: add tpm_tis_i2c driver"
-Replaced "depends on CRC_CCIT" with "select CRC_CCIT".
-Added tested-by tag by Joel Stanley.
-Fixed checkpatch.pl warnings.
Addressed comments from:
- Jarkko Sakkinen:
https://lore.kernel.org/patchwork/patch/1221336/
https://lore.kernel.org/patchwork/patch/1221337/
https://lore.kernel.org/patchwork/patch/1221339/
- Joel Stanley:
https://lore.kernel.org/patchwork/patch/1220543/
- Rob Herring:
https://lore.kernel.org/patchwork/patch/1221334/
Changes since version 7:
- Added a new commit with improvements suggested by Benoit Houyere.
-"tpm: tpm_tis: verify TPM_STS register is valid after locality request"
-"tpm: tpm_tis: Rewrite "tpm_tis_req_canceled()""
-Fixed Hash for Fixes tag.
-"tpm: Add YAML schema for TPM TIS I2C options"
-Added a compatible string specific to the nuvoton npct75x chip.
-"tpm: tpm_tis: add tpm_tis_i2c driver"
-added a compatible string according to yaml file.
Addressed comments from:
- Jarkko Sakkinen:
https://lore.kernel.org/patchwork/patch/1231524/
- Rob Herring:
https://lore.kernel.org/patchwork/patch/1231526/
Changes since version 8:
- "tpm: tpm_tis: Make implementation of read16, read32 and write32 optional"
-Fixed a compile error conflicting CR50
- "tpm: tpm_tis: Fix expected bit handling and send all bytes in one shot without last byte in exception"
-Moved commit backwards from 4/8 to 2/8 for a better flow with new data integrity check design
- "tpm: tpm_tis: Add retry in case of protocol failure or data integrity (on I2C only) failure."
-Renamed from "tpm: tpm_tis: Add check_data handle to tpm_tis_phy_ops"
-Redesign and added a retry for additional error cases.
- "tpm: Add YAML schema for TPM TIS I2C options"
-Fixed Dual-license new binding
-Removed "oneOf"
-Fixed tpm_tis@2e to tpm@2e
Addressed comments from:
- Jarkko Sakkinen:
https://lore.kernel.org/patchwork/patch/1240728/
https://lore.kernel.org/patchwork/patch/1240736/
- Rob Herring:
https://lore.kernel.org/patchwork/patch/1240733/
Changes since version 9:
- "tpm: Make read{16, 32}() and write32() in tpm_tis_phy_ops optional"
-Fixed short description
- "tpm: tpm_tis: Fix expected bit handling and send all bytes in one shot without last byte in exception"
-Canceled wait_for_tpm_stat() function renaming.
-Fixed long description
- "tpm: Add YAML schema for TPM TIS I2C options"
-Added a reviewed-by tag.
Addressed comments from:
- Jarkko Sakkinen:
https://lore.kernel.org/patchwork/patch/1247163/
https://lore.kernel.org/patchwork/patch/1247164/
- Rob Herring:
https://lore.kernel.org/patchwork/patch/1247161/
Amir Mizinski (8):
tpm: Make read{16, 32}() and write32() in tpm_tis_phy_ops optional
tpm: tpm_tis: Fix expected bit handling and send all bytes in one shot
without last byte in exception
tpm: tpm_tis: Add retry in case of protocol failure or data integrity
(on I2C only) failure.
tpm: tpm_tis: Rewrite "tpm_tis_req_canceled()"
tpm: Handle an exception for TPM Firmware Update mode.
tpm: tpm_tis: verify TPM_STS register is valid after locality request
tpm: Add YAML schema for TPM TIS I2C options
tpm: tpm_tis: add tpm_tis_i2c driver
.../bindings/security/tpm/tpm-tis-i2c.yaml | 50 ++++
drivers/char/tpm/Kconfig | 12 +
drivers/char/tpm/Makefile | 1 +
drivers/char/tpm/tpm2-cmd.c | 4 +
drivers/char/tpm/tpm_tis_core.c | 182 ++++++-------
drivers/char/tpm/tpm_tis_core.h | 41 ++-
drivers/char/tpm/tpm_tis_i2c.c | 292 +++++++++++++++++++++
drivers/char/tpm/tpm_tis_spi.h | 4 -
drivers/char/tpm/tpm_tis_spi_cr50.c | 3 -
drivers/char/tpm/tpm_tis_spi_main.c | 41 ---
include/linux/tpm.h | 1 +
11 files changed, 491 insertions(+), 140 deletions(-)
create mode 100644 Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml
create mode 100644 drivers/char/tpm/tpm_tis_i2c.c
--
2.7.4
^ permalink raw reply
* [PATCH v10 1/8] tpm: Make read{16, 32}() and write32() in tpm_tis_phy_ops optional
From: amirmizi6 @ 2020-06-04 13:47 UTC (permalink / raw)
To: Eyal.Cohen, jarkko.sakkinen, oshrialkoby85, alexander.steffen,
robh+dt, "benoit.houyere, peterhuewe, christophe-h.richard,
jgg, arnd, gregkh
Cc: devicetree, linux-kernel, linux-integrity, oshri.alkoby,
tmaimon77, gcwilson, kgoldman, Dan.Morav, oren.tanami,
shmulik.hager, amir.mizinski, Amir Mizinski, Alexander Steffen
In-Reply-To: <20200604134713.157951-1-amirmizi6@gmail.com>
From: Amir Mizinski <amirmizi6@gmail.com>
Only tpm_tis can use memory-mapped I/O, which is truly mapped into
the kernel's memory space. Therefore, using ioread16/ioread32/iowrite32
turns into a straightforward pointer dereference.
Every other driver requires more complicated operations to read more than
one byte at a time and will just fall back to read_bytes/write_bytes.
Therefore, move this common code out of tpm_tis_spi and into tpm_tis_core
so that it is used automatically when low-level drivers do not implement
the specialized methods.
Co-developed-by: Alexander Steffen <Alexander.Steffen@infineon.com>
Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>
Signed-off-by: Amir Mizinski <amirmizi6@gmail.com>
---
drivers/char/tpm/tpm_tis_core.h | 38 +++++++++++++++++++++++++++++++---
drivers/char/tpm/tpm_tis_spi.h | 4 ----
drivers/char/tpm/tpm_tis_spi_cr50.c | 3 ---
drivers/char/tpm/tpm_tis_spi_main.c | 41 -------------------------------------
4 files changed, 35 insertions(+), 51 deletions(-)
diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
index 7337819..d06c65b 100644
--- a/drivers/char/tpm/tpm_tis_core.h
+++ b/drivers/char/tpm/tpm_tis_core.h
@@ -122,13 +122,35 @@ static inline int tpm_tis_read8(struct tpm_tis_data *data, u32 addr, u8 *result)
static inline int tpm_tis_read16(struct tpm_tis_data *data, u32 addr,
u16 *result)
{
- return data->phy_ops->read16(data, addr, result);
+ __le16 result_le;
+ int rc;
+
+ if (data->phy_ops->read16)
+ return data->phy_ops->read16(data, addr, result);
+
+ rc = data->phy_ops->read_bytes(data, addr, sizeof(u16),
+ (u8 *)&result_le);
+ if (!rc)
+ *result = le16_to_cpu(result_le);
+
+ return rc;
}
static inline int tpm_tis_read32(struct tpm_tis_data *data, u32 addr,
u32 *result)
{
- return data->phy_ops->read32(data, addr, result);
+ __le32 result_le;
+ int rc;
+
+ if (data->phy_ops->read32)
+ return data->phy_ops->read32(data, addr, result);
+
+ rc = data->phy_ops->read_bytes(data, addr, sizeof(u32),
+ (u8 *)&result_le);
+ if (!rc)
+ *result = le32_to_cpu(result_le);
+
+ return rc;
}
static inline int tpm_tis_write_bytes(struct tpm_tis_data *data, u32 addr,
@@ -145,7 +167,17 @@ static inline int tpm_tis_write8(struct tpm_tis_data *data, u32 addr, u8 value)
static inline int tpm_tis_write32(struct tpm_tis_data *data, u32 addr,
u32 value)
{
- return data->phy_ops->write32(data, addr, value);
+ __le32 value_le;
+ int rc;
+
+ if (data->phy_ops->write32)
+ return data->phy_ops->write32(data, addr, value);
+
+ value_le = cpu_to_le32(value);
+ rc = data->phy_ops->write_bytes(data, addr, sizeof(u32),
+ (u8 *)&value_le);
+
+ return rc;
}
static inline bool is_bsw(void)
diff --git a/drivers/char/tpm/tpm_tis_spi.h b/drivers/char/tpm/tpm_tis_spi.h
index bba7397..d0f66f6 100644
--- a/drivers/char/tpm/tpm_tis_spi.h
+++ b/drivers/char/tpm/tpm_tis_spi.h
@@ -31,10 +31,6 @@ extern int tpm_tis_spi_init(struct spi_device *spi, struct tpm_tis_spi_phy *phy,
extern int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
u8 *in, const u8 *out);
-extern int tpm_tis_spi_read16(struct tpm_tis_data *data, u32 addr, u16 *result);
-extern int tpm_tis_spi_read32(struct tpm_tis_data *data, u32 addr, u32 *result);
-extern int tpm_tis_spi_write32(struct tpm_tis_data *data, u32 addr, u32 value);
-
#ifdef CONFIG_TCG_TIS_SPI_CR50
extern int cr50_spi_probe(struct spi_device *spi);
#else
diff --git a/drivers/char/tpm/tpm_tis_spi_cr50.c b/drivers/char/tpm/tpm_tis_spi_cr50.c
index 37d72e8..f339d20 100644
--- a/drivers/char/tpm/tpm_tis_spi_cr50.c
+++ b/drivers/char/tpm/tpm_tis_spi_cr50.c
@@ -215,9 +215,6 @@ static int tpm_tis_spi_cr50_write_bytes(struct tpm_tis_data *data, u32 addr,
static const struct tpm_tis_phy_ops tpm_spi_cr50_phy_ops = {
.read_bytes = tpm_tis_spi_cr50_read_bytes,
.write_bytes = tpm_tis_spi_cr50_write_bytes,
- .read16 = tpm_tis_spi_read16,
- .read32 = tpm_tis_spi_read32,
- .write32 = tpm_tis_spi_write32,
};
static void cr50_print_fw_version(struct tpm_tis_data *data)
diff --git a/drivers/char/tpm/tpm_tis_spi_main.c b/drivers/char/tpm/tpm_tis_spi_main.c
index d1754fd..95fef9d 100644
--- a/drivers/char/tpm/tpm_tis_spi_main.c
+++ b/drivers/char/tpm/tpm_tis_spi_main.c
@@ -152,44 +152,6 @@ static int tpm_tis_spi_write_bytes(struct tpm_tis_data *data, u32 addr,
return tpm_tis_spi_transfer(data, addr, len, NULL, value);
}
-int tpm_tis_spi_read16(struct tpm_tis_data *data, u32 addr, u16 *result)
-{
- __le16 result_le;
- int rc;
-
- rc = data->phy_ops->read_bytes(data, addr, sizeof(u16),
- (u8 *)&result_le);
- if (!rc)
- *result = le16_to_cpu(result_le);
-
- return rc;
-}
-
-int tpm_tis_spi_read32(struct tpm_tis_data *data, u32 addr, u32 *result)
-{
- __le32 result_le;
- int rc;
-
- rc = data->phy_ops->read_bytes(data, addr, sizeof(u32),
- (u8 *)&result_le);
- if (!rc)
- *result = le32_to_cpu(result_le);
-
- return rc;
-}
-
-int tpm_tis_spi_write32(struct tpm_tis_data *data, u32 addr, u32 value)
-{
- __le32 value_le;
- int rc;
-
- value_le = cpu_to_le32(value);
- rc = data->phy_ops->write_bytes(data, addr, sizeof(u32),
- (u8 *)&value_le);
-
- return rc;
-}
-
int tpm_tis_spi_init(struct spi_device *spi, struct tpm_tis_spi_phy *phy,
int irq, const struct tpm_tis_phy_ops *phy_ops)
{
@@ -205,9 +167,6 @@ int tpm_tis_spi_init(struct spi_device *spi, struct tpm_tis_spi_phy *phy,
static const struct tpm_tis_phy_ops tpm_spi_phy_ops = {
.read_bytes = tpm_tis_spi_read_bytes,
.write_bytes = tpm_tis_spi_write_bytes,
- .read16 = tpm_tis_spi_read16,
- .read32 = tpm_tis_spi_read32,
- .write32 = tpm_tis_spi_write32,
};
static int tpm_tis_spi_probe(struct spi_device *dev)
--
2.7.4
^ permalink raw reply related
* [PATCH v10 2/8] tpm: tpm_tis: Fix expected bit handling and send all bytes in one shot without last byte in exception
From: amirmizi6 @ 2020-06-04 13:47 UTC (permalink / raw)
To: Eyal.Cohen, jarkko.sakkinen, oshrialkoby85, alexander.steffen,
robh+dt, "benoit.houyere, peterhuewe, christophe-h.richard,
jgg, arnd, gregkh
Cc: devicetree, linux-kernel, linux-integrity, oshri.alkoby,
tmaimon77, gcwilson, kgoldman, Dan.Morav, oren.tanami,
shmulik.hager, amir.mizinski, Amir Mizinski, Benoit Houyere
In-Reply-To: <20200604134713.157951-1-amirmizi6@gmail.com>
From: Amir Mizinski <amirmizi6@gmail.com>
Detected the following incorrect implementation of the send command:
polling on the TPM_STS.stsValid field followed by checking the
TPM_STS.expect field only once. Since TPM_STS.stsValid represents the
TPM_STS.expect validity, both fields should be polled at the same time.
This fix modifies the signature of wait_for_tpm_stat(), adding an
additional "mask_result" parameter to its call. wait_for_tpm_stat() is now
polling the TPM_STS with a mask and waits for the value in mask_result.
The fix adds the ability to check if certain TPM_STS bits have been
cleared.
This change is also aligned to verifying the CRC on I2C TPM. The CRC
verification should be done after the TPM_STS.expect field is cleared
(TPM received all expected command bytes and set the calculated CRC value
in the register).
In addition, the send command was changed to comply with
TCG_DesignPrinciples_TPM2p0Driver_vp24_pubrev.pdf as follows:
- send all command bytes in one loop
- remove special handling of the last byte
Suggested-by: Benoit Houyere <benoit.houyere@st.com>
Signed-off-by: Amir Mizinski <amirmizi6@gmail.com>
---
drivers/char/tpm/tpm_tis_core.c | 66 ++++++++++++++---------------------------
1 file changed, 22 insertions(+), 44 deletions(-)
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 27c6ca0..6ea70ea 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -44,9 +44,9 @@ static bool wait_for_tpm_stat_cond(struct tpm_chip *chip, u8 mask,
return false;
}
-static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask,
- unsigned long timeout, wait_queue_head_t *queue,
- bool check_cancel)
+static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, u8 mask_result,
+ unsigned long timeout, wait_queue_head_t *queue,
+ bool check_cancel)
{
unsigned long stop;
long rc;
@@ -55,7 +55,7 @@ static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask,
/* check current status */
status = chip->ops->status(chip);
- if ((status & mask) == mask)
+ if ((status & mask) == mask_result)
return 0;
stop = jiffies + timeout;
@@ -83,7 +83,7 @@ static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask,
usleep_range(TPM_TIMEOUT_USECS_MIN,
TPM_TIMEOUT_USECS_MAX);
status = chip->ops->status(chip);
- if ((status & mask) == mask)
+ if ((status & mask) == mask_result)
return 0;
} while (time_before(jiffies, stop));
}
@@ -281,10 +281,10 @@ static int recv_data(struct tpm_chip *chip, u8 *buf, size_t count)
int size = 0, burstcnt, rc;
while (size < count) {
- rc = wait_for_tpm_stat(chip,
- TPM_STS_DATA_AVAIL | TPM_STS_VALID,
- chip->timeout_c,
- &priv->read_queue, true);
+ rc = wait_for_tpm_stat(chip, TPM_STS_DATA_AVAIL | TPM_STS_VALID,
+ TPM_STS_DATA_AVAIL | TPM_STS_VALID,
+ chip->timeout_c, &priv->read_queue,
+ true);
if (rc < 0)
return rc;
burstcnt = get_burstcount(chip);
@@ -337,8 +337,8 @@ static int tpm_tis_recv(struct tpm_chip *chip, u8 *buf, size_t count)
goto out;
}
- if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
- &priv->int_queue, false) < 0) {
+ if (wait_for_tpm_stat(chip, TPM_STS_VALID, TPM_STS_VALID,
+ chip->timeout_c, &priv->int_queue, false) < 0) {
size = -ETIME;
goto out;
}
@@ -364,61 +364,39 @@ static int tpm_tis_send_data(struct tpm_chip *chip, const u8 *buf, size_t len)
struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
int rc, status, burstcnt;
size_t count = 0;
- bool itpm = priv->flags & TPM_TIS_ITPM_WORKAROUND;
status = tpm_tis_status(chip);
if ((status & TPM_STS_COMMAND_READY) == 0) {
tpm_tis_ready(chip);
- if (wait_for_tpm_stat
- (chip, TPM_STS_COMMAND_READY, chip->timeout_b,
- &priv->int_queue, false) < 0) {
+ if (wait_for_tpm_stat(chip, TPM_STS_COMMAND_READY,
+ TPM_STS_COMMAND_READY, chip->timeout_b,
+ &priv->int_queue, false) < 0) {
rc = -ETIME;
goto out_err;
}
}
- while (count < len - 1) {
+ while (count < len) {
burstcnt = get_burstcount(chip);
if (burstcnt < 0) {
dev_err(&chip->dev, "Unable to read burstcount\n");
rc = burstcnt;
goto out_err;
}
- burstcnt = min_t(int, burstcnt, len - count - 1);
+ burstcnt = min_t(int, burstcnt, len - count);
rc = tpm_tis_write_bytes(priv, TPM_DATA_FIFO(priv->locality),
burstcnt, buf + count);
if (rc < 0)
goto out_err;
count += burstcnt;
-
- if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
- &priv->int_queue, false) < 0) {
- rc = -ETIME;
- goto out_err;
- }
- status = tpm_tis_status(chip);
- if (!itpm && (status & TPM_STS_DATA_EXPECT) == 0) {
- rc = -EIO;
- goto out_err;
- }
}
-
- /* write last byte */
- rc = tpm_tis_write8(priv, TPM_DATA_FIFO(priv->locality), buf[count]);
- if (rc < 0)
- goto out_err;
-
- if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c,
- &priv->int_queue, false) < 0) {
+ if (wait_for_tpm_stat(chip, TPM_STS_VALID | TPM_STS_DATA_EXPECT,
+ TPM_STS_VALID, chip->timeout_a, &priv->int_queue,
+ false) < 0) {
rc = -ETIME;
goto out_err;
}
- status = tpm_tis_status(chip);
- if (!itpm && (status & TPM_STS_DATA_EXPECT) != 0) {
- rc = -EIO;
- goto out_err;
- }
return 0;
@@ -470,9 +448,9 @@ static int tpm_tis_send_main(struct tpm_chip *chip, const u8 *buf, size_t len)
ordinal = be32_to_cpu(*((__be32 *) (buf + 6)));
dur = tpm_calc_ordinal_duration(chip, ordinal);
- if (wait_for_tpm_stat
- (chip, TPM_STS_DATA_AVAIL | TPM_STS_VALID, dur,
- &priv->read_queue, false) < 0) {
+ if (wait_for_tpm_stat(chip, TPM_STS_DATA_AVAIL | TPM_STS_VALID,
+ TPM_STS_DATA_AVAIL | TPM_STS_VALID, dur,
+ &priv->read_queue, false) < 0) {
rc = -ETIME;
goto out_err;
}
--
2.7.4
^ permalink raw reply related
* [PATCH v10 3/8] tpm: tpm_tis: Add retry in case of protocol failure or data integrity (on I2C only) failure.
From: amirmizi6 @ 2020-06-04 13:47 UTC (permalink / raw)
To: Eyal.Cohen, jarkko.sakkinen, oshrialkoby85, alexander.steffen,
robh+dt, "benoit.houyere, peterhuewe, christophe-h.richard,
jgg, arnd, gregkh
Cc: devicetree, linux-kernel, linux-integrity, oshri.alkoby,
tmaimon77, gcwilson, kgoldman, Dan.Morav, oren.tanami,
shmulik.hager, amir.mizinski, Amir Mizinski, Christophe Ricard
In-Reply-To: <20200604134713.157951-1-amirmizi6@gmail.com>
From: Amir Mizinski <amirmizi6@gmail.com>
The FIFO protocol described in the TCG PC Client Device Driver Design
Principles for TPM 2.0 advises retrying sending a command or receiving
a response using the FIFO protocol in case of any error in the protocol.
Add a retry mechanism on any protocol error. In addition, in case of a data
integrity issue in the I2C bus protocol, check after sending a command
completion or receiving a response from the TPM.
Co-developed-by: Christophe Ricard <christophe-h.ricard@st.com>
Signed-off-by: Christophe Ricard <christophe-h.ricard@st.com>
Signed-off-by: Amir Mizinski <amirmizi6@gmail.com>
---
drivers/char/tpm/tpm_tis_core.c | 106 ++++++++++++++++++++++++----------------
drivers/char/tpm/tpm_tis_core.h | 3 ++
2 files changed, 67 insertions(+), 42 deletions(-)
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 6ea70ea..202714d 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -308,7 +308,7 @@ static int tpm_tis_recv(struct tpm_chip *chip, u8 *buf, size_t count)
{
struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
int size = 0;
- int status;
+ int status, i;
u32 expected;
if (count < TPM_HEADER_SIZE) {
@@ -316,39 +316,53 @@ static int tpm_tis_recv(struct tpm_chip *chip, u8 *buf, size_t count)
goto out;
}
- size = recv_data(chip, buf, TPM_HEADER_SIZE);
- /* read first 10 bytes, including tag, paramsize, and result */
- if (size < TPM_HEADER_SIZE) {
- dev_err(&chip->dev, "Unable to read header\n");
- goto out;
- }
+ for (i = 0; i < TPM_RETRY; i++) {
+ size = recv_data(chip, buf, TPM_HEADER_SIZE);
+ /* read first 10 bytes, including tag, paramsize, and result */
+ if (size < TPM_HEADER_SIZE) {
+ dev_err(&chip->dev, "Unable to read header\n");
+ goto retry;
+ }
- expected = be32_to_cpu(*(__be32 *) (buf + 2));
- if (expected > count || expected < TPM_HEADER_SIZE) {
- size = -EIO;
- goto out;
- }
+ expected = be32_to_cpu(*(__be32 *) (buf + 2));
+ if (expected > count || expected < TPM_HEADER_SIZE) {
+ size = -EIO;
+ goto retry;
+ }
- size += recv_data(chip, &buf[TPM_HEADER_SIZE],
- expected - TPM_HEADER_SIZE);
- if (size < expected) {
- dev_err(&chip->dev, "Unable to read remainder of result\n");
- size = -ETIME;
- goto out;
- }
+ size += recv_data(chip, &buf[TPM_HEADER_SIZE],
+ expected - TPM_HEADER_SIZE);
+ if (size < expected) {
+ dev_err(&chip->dev, "Unable to read remainder of result\n");
+ size = -ETIME;
+ goto retry;
+ }
- if (wait_for_tpm_stat(chip, TPM_STS_VALID, TPM_STS_VALID,
- chip->timeout_c, &priv->int_queue, false) < 0) {
- size = -ETIME;
- goto out;
- }
- status = tpm_tis_status(chip);
- if (status & TPM_STS_DATA_AVAIL) { /* retry? */
- dev_err(&chip->dev, "Error left over data\n");
- size = -EIO;
- goto out;
- }
+ if (wait_for_tpm_stat(chip, TPM_STS_VALID, TPM_STS_VALID,
+ chip->timeout_c, &priv->int_queue,
+ false) < 0) {
+ size = -ETIME;
+ goto retry;
+ }
+
+ status = tpm_tis_status(chip);
+ if (status & TPM_STS_DATA_AVAIL) { /* retry? */
+ dev_err(&chip->dev, "Error left over data\n");
+ size = -EIO;
+ goto retry;
+ }
+ if (priv->phy_ops->verify_data_integrity)
+ if (!priv->phy_ops->verify_data_integrity(priv, buf,
+ size))
+ size = -EIO;
+retry:
+ if (size <= 0)
+ tpm_tis_write8(priv, TPM_STS(priv->locality),
+ TPM_STS_RESPONSE_RETRY);
+ else
+ goto out;
+ }
out:
tpm_tis_ready(chip);
return size;
@@ -372,7 +386,7 @@ static int tpm_tis_send_data(struct tpm_chip *chip, const u8 *buf, size_t len)
TPM_STS_COMMAND_READY, chip->timeout_b,
&priv->int_queue, false) < 0) {
rc = -ETIME;
- goto out_err;
+ return rc;
}
}
@@ -381,13 +395,13 @@ static int tpm_tis_send_data(struct tpm_chip *chip, const u8 *buf, size_t len)
if (burstcnt < 0) {
dev_err(&chip->dev, "Unable to read burstcount\n");
rc = burstcnt;
- goto out_err;
+ return rc;
}
burstcnt = min_t(int, burstcnt, len - count);
rc = tpm_tis_write_bytes(priv, TPM_DATA_FIFO(priv->locality),
burstcnt, buf + count);
if (rc < 0)
- goto out_err;
+ return rc;
count += burstcnt;
}
@@ -395,14 +409,10 @@ static int tpm_tis_send_data(struct tpm_chip *chip, const u8 *buf, size_t len)
TPM_STS_VALID, chip->timeout_a, &priv->int_queue,
false) < 0) {
rc = -ETIME;
- goto out_err;
+ return rc;
}
return 0;
-
-out_err:
- tpm_tis_ready(chip);
- return rc;
}
static void disable_interrupts(struct tpm_chip *chip)
@@ -431,13 +441,25 @@ static void disable_interrupts(struct tpm_chip *chip)
static int tpm_tis_send_main(struct tpm_chip *chip, const u8 *buf, size_t len)
{
struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
- int rc;
+ int rc, i;
u32 ordinal;
unsigned long dur;
- rc = tpm_tis_send_data(chip, buf, len);
- if (rc < 0)
- return rc;
+ for (i = 0; i < TPM_RETRY; i++) {
+ rc = tpm_tis_send_data(chip, buf, len);
+ if (rc < 0)
+ continue;
+ if (priv->phy_ops->verify_data_integrity) {
+ if (!priv->phy_ops->verify_data_integrity(priv, buf,
+ len)){
+ rc = -EIO;
+ continue;
+ }
+ }
+ break;
+ }
+ if (i == TPM_RETRY)
+ goto out_err;
/* go and do it */
rc = tpm_tis_write8(priv, TPM_STS(priv->locality), TPM_STS_GO);
diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
index d06c65b..cd97c01 100644
--- a/drivers/char/tpm/tpm_tis_core.h
+++ b/drivers/char/tpm/tpm_tis_core.h
@@ -34,6 +34,7 @@ enum tis_status {
TPM_STS_GO = 0x20,
TPM_STS_DATA_AVAIL = 0x10,
TPM_STS_DATA_EXPECT = 0x08,
+ TPM_STS_RESPONSE_RETRY = 0x02,
};
enum tis_int_flags {
@@ -106,6 +107,8 @@ struct tpm_tis_phy_ops {
int (*read16)(struct tpm_tis_data *data, u32 addr, u16 *result);
int (*read32)(struct tpm_tis_data *data, u32 addr, u32 *result);
int (*write32)(struct tpm_tis_data *data, u32 addr, u32 src);
+ bool (*verify_data_integrity)(struct tpm_tis_data *data, const u8 *buf,
+ size_t len);
};
static inline int tpm_tis_read_bytes(struct tpm_tis_data *data, u32 addr,
--
2.7.4
^ permalink raw reply related
* [PATCH v10 6/8] tpm: tpm_tis: verify TPM_STS register is valid after locality request
From: amirmizi6 @ 2020-06-04 13:47 UTC (permalink / raw)
To: Eyal.Cohen, jarkko.sakkinen, oshrialkoby85, alexander.steffen,
robh+dt, "benoit.houyere, peterhuewe, christophe-h.richard,
jgg, arnd, gregkh
Cc: devicetree, linux-kernel, linux-integrity, oshri.alkoby,
tmaimon77, gcwilson, kgoldman, Dan.Morav, oren.tanami,
shmulik.hager, amir.mizinski, Amir Mizinski, Benoit Houyere
In-Reply-To: <20200604134713.157951-1-amirmizi6@gmail.com>
From: Amir Mizinski <amirmizi6@gmail.com>
Issue could result when the TPM does not update TPM_STS register after
a locality request (TPM_STS Initial value = 0xFF) and a TPM_STS register
read occurs (tpm_tis_status(chip)).
Checking the next condition("if ((status & TPM_STS_COMMAND_READY) == 0)"),
the status will be at 0xFF and will be considered, wrongly, in "Ready"
state (by checking only one bit). However, at this moment the TPM is, in
fact, in "Idle" state and remains in "Idle" state because
"tpm_tis_ready(chip);" was not executed.
Suggested-by: Benoit Houyere <benoit.houyere@st.com>
Signed-off-by: Amir Mizinski <amirmizi6@gmail.com>
---
drivers/char/tpm/tpm_tis_core.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 9d90b9a..8868fe0 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -221,8 +221,14 @@ static int request_locality(struct tpm_chip *chip, int l)
} else {
/* wait for burstcount */
do {
- if (check_locality(chip, l))
+ if (check_locality(chip, l)) {
+ if (wait_for_tpm_stat(chip, TPM_STS_GO, 0,
+ chip->timeout_c,
+ &priv->int_queue,
+ false) < 0)
+ return -ETIME;
return l;
+ }
tpm_msleep(TPM_TIMEOUT);
} while (time_before(jiffies, stop));
}
--
2.7.4
^ permalink raw reply related
* [PATCH v10 5/8] tpm: Handle an exception for TPM Firmware Update mode.
From: amirmizi6 @ 2020-06-04 13:47 UTC (permalink / raw)
To: Eyal.Cohen, jarkko.sakkinen, oshrialkoby85, alexander.steffen,
robh+dt, "benoit.houyere, peterhuewe, christophe-h.richard,
jgg, arnd, gregkh
Cc: devicetree, linux-kernel, linux-integrity, oshri.alkoby,
tmaimon77, gcwilson, kgoldman, Dan.Morav, oren.tanami,
shmulik.hager, amir.mizinski, Amir Mizinski, Benoit Houyere
In-Reply-To: <20200604134713.157951-1-amirmizi6@gmail.com>
From: Amir Mizinski <amirmizi6@gmail.com>
An extra precaution for TPM Firmware Update Mode.
For example if TPM power was cut while in Firmware update, platform
should ignore "selftest" failure and skip TPM initialization sequence.
Suggested-by: Benoit Houyere <benoit.houyere@st.com>
Signed-off-by: Amir Mizinski <amirmizi6@gmail.com>
---
drivers/char/tpm/tpm2-cmd.c | 4 ++++
include/linux/tpm.h | 1 +
2 files changed, 5 insertions(+)
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 7603295..6e42946 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -727,6 +727,10 @@ int tpm2_auto_startup(struct tpm_chip *chip)
goto out;
rc = tpm2_do_selftest(chip);
+
+ if (rc == TPM2_RC_UPGRADE || rc == TPM2_RC_COMMAND_CODE)
+ return 0;
+
if (rc && rc != TPM2_RC_INITIALIZE)
goto out;
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 03e9b18..5a2e031 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -199,6 +199,7 @@ enum tpm2_return_codes {
TPM2_RC_INITIALIZE = 0x0100, /* RC_VER1 */
TPM2_RC_FAILURE = 0x0101,
TPM2_RC_DISABLED = 0x0120,
+ TPM2_RC_UPGRADE = 0x012D,
TPM2_RC_COMMAND_CODE = 0x0143,
TPM2_RC_TESTING = 0x090A, /* RC_WARN */
TPM2_RC_REFERENCE_H0 = 0x0910,
--
2.7.4
^ permalink raw reply related
* Re: [RFC PATCH 0/3] firmware: Add support for PSA FF-A interface
From: Will Deacon @ 2020-06-04 13:37 UTC (permalink / raw)
To: Sudeep Holla
Cc: devicetree, linux-arm-kernel, linux-kernel, Marc Zyngier, tabba,
qwandor, ardb
In-Reply-To: <20200601094512.50509-1-sudeep.holla@arm.com>
Hi Sudeep, [+Fuad, Andrew and Ard]
(To other interested readers: if you haven't seen it, the FF-A spec is here:
https://static.docs.arm.com/den0077/a/DEN0077A_PSA_Firmware_Framework_Arm_v8-A_1.0_EAC.pdf
since this discussion makes no sense without that, and a tiny bit of sense
with it. It used to be called "SPCI" but it was recently renamed.)
On Mon, Jun 01, 2020 at 10:45:09AM +0100, Sudeep Holla wrote:
> Sorry for posting in the middle of merge window and I must have done
> this last week itself. This is not the driver I had thought about posting
> last week. After I started cleaning up and looking at Will's KVM prototype[1]
> for PSA FF-A (previously known as SPCI),
Yes, I need to do the Big Rename at some point. Joy.
> I got more doubts on alignment and dropped huge chunk of interface APIs in
> the driver in order to keep it simple, and get aligned more with that
> prototype and avoid scanning lots of code unnecessary.
You also dropped most of the code, so this doesn't really do anything in
its current form ;)
> Here are few things to clarify:
>
> 1. DT bindings
> ---------------
> I was initially against adding bindings for Tx/Rx buffers for
> partitions. As per the spec, an endpoint could allocate the
> buffer pair and use the FFA_RXTX_MAP interface to map it with the
> Hypervisor(KVM here). However looking at the prototype and also
> I remember you mentioning that it is not possible to manage buffers
> in that way. Please confirm if you plan to add the buffer details
> fetcthing them through ioctls in KVM and adding them to VM DT nodes
> in KVM userspace. I will update the bindings accordingly.
I think it's useful to have a mode of operation where the hypervisor
allocates the RX/TX buffers and advertises them in the DT. However, we
can always add this later, so there's no need to have it in the binding
from the start. Best start as simple as possible, I reckon.
Setting the static RX/TX buffer allocation aside, why is a DT node needed
at all for the case where Linux is running purely as an FF-A client? I
thought everything should be discoverable via FFA_VERSION, FFA_FEATURES,
FFA_PARTITION_INFO_GET and FFA_ID_GET? That should mean we can get away
without a binding at all for the client case.
> 2. Driver
> ---------
> a. Support for multiple partitions in a VM
> ------------------------------------------
> I am not sure if there is need for supporting multiple partitions
> within a VM. It should be possible to do so as I expect to create
> device for each partition entry under arm-psa-ffa devicetree node.
> However, I don't want to assume something that will never be a
> usecase. However I don't think this will change must of the
> abstraction as we need to keep the interface API implementation
> separate to support different partitions on various platforms.
I think Ard has a case for something like this, where a VM actually consists
of multiple partitions so that S-EL0 services can be provided from NS-EL0.
However, he probably wants that for a dynamically created VM, so we'd
need a way to instantiate an FFA namespace for the VM. Maybe that can be
done entirely in userspace by the VMM...
> b. SMCCC interface
> ------------------
> This is something I messed up completely while trying to add
> support for SMCCC v1.2. It now supports x0-x17 as parameter
> registers(input) and return registers(output). I started simple
> with x0-x7 as both input and output as PSA FF-A needs that at
> most. But extending to x0-x17 then became with messy in my
> implementation. That's the reason I dropped it completely
> here and thought of checking it first.
>
> Do we need to extend the optimisations that were done to handle
> ARCH_WORKAROUND_{1,2}. Or should be just use a version with x0-x7
> as both input and ouput. Hyper-V guys need full x0-x17 support.
>
> I need some guidance as what is the approach preferred ?
I think we can start off with x0-x7 and extend if later if we need to.
> 3. Partitions
> -------------
> I am not sure if we have a full define partition that we plan to
> push upstream. Without one, we can have a sample/example partition
> to test all the interface APIs, but is that fine with respect to
> what we want upstream ? Any other thoughts that helps to test the
> driver ?
I think that's the best you can do for now. We can probably help with
testing as our stuff gets off the ground.
> Sorry for long email and too many questions, but I thought it is easier
> this way to begin with than throwing huge code implementing loads of APIs
> with no users(expect example partition) especially that I am posting this
> during merge window.
No problem. Maybe it would help if I described roughly what we were thinking
of doing for KVM (this is open for discussion, of course):
1. Describe KVM-managed partitions in the DT, along the lines of [1]
2. Expose each partition as a file to userspace. E.g.:
/dev/spci/:
self
e3a48fa5-dc54-4a8b-898b-bdc4dfeeb7b8
49f65057-d002-4ae2-b4ee-d31c7940a13d
Here, self would be a symlink to the host uuid. The host uuid file
would implement FFA_MEM operations using an ioctl(), so you could,
for example, share a user buffer with multiple partitions by issuing
a MEM_SHARE ioctl() on self, passing the fds for the borrower partitions
as arguments. Messaging would be implemented as ioctl()s on the
partition uuid files themselves.
3. We'll need some (all?) of these patches to unmap memory from the host
when necessary:
https://lwn.net/Articles/821215/
(for nVHE, we'll have a stage-2 for the host so we can unmap there as
well)
For communicating with partitions that are not managed by KVM (e.g. trusted
applications), it's not clear to me how much of that will be handled in
kernel or user. I think it would still be worth exposing the partitions as
files, but perhaps having them root only or just returning -EPERM for the
ioctl() if a kernel driver has claimed the partition as its own? Ideally,
FF-A would allow us to transition some of the Trusted OS interfacing code
out to userspace, but I don't know how realistic that is.
Anyway, to enable this, I think we need a clear separation in the kernel
between the FF-A code and the users: KVM will want to expose things as
above, but if drivers need to use this stuff as well then they can plug in
as additional users and we don't have to worry about tripping over the
RX/TX buffers etc.
What do you think, and do you reckon you can spin a cut-down driver that
implements the common part of the logic (since I know you've written much
of this code already)?
Cheers,
Will
[1] https://android-kvm.googlesource.com/linux/+/8632a5723ef106017c4ab57e95d9ce7630d35522%5E%21/#F0
^ permalink raw reply
* [PATCH v6] drm/msm/dpu: ensure device suspend happens during PM sleep
From: Kalyan Thota @ 2020-06-04 13:19 UTC (permalink / raw)
To: dri-devel, linux-arm-msm, freedreno, devicetree
Cc: Kalyan Thota, linux-kernel, robdclark, seanpaul, hoegsberg,
dianders, jsanka, mkrishn, travitej, nganji
"The PM core always increments the runtime usage counter
before calling the ->suspend() callback and decrements it
after calling the ->resume() callback"
DPU and DSI are managed as runtime devices. When
suspend is triggered, PM core adds a refcount on all the
devices and calls device suspend, since usage count is
already incremented, runtime suspend was not getting called
and it kept the clocks on which resulted in target not
entering into XO shutdown.
Add changes to force suspend on runtime devices during pm sleep.
Changes in v1:
- Remove unnecessary checks in the function
_dpu_kms_disable_dpu (Rob Clark).
Changes in v2:
- Avoid using suspend_late to reset the usagecount
as suspend_late might not be called during suspend
call failures (Doug).
Changes in v3:
- Use force suspend instead of managing device usage_count
via runtime put and get API's to trigger callbacks (Doug).
Changes in v4:
- Check the return values of pm_runtime_force_suspend and
pm_runtime_force_resume API's and pass appropriately (Doug).
Changes in v5:
- With v4 patch, test cycle has uncovered issues in device resume.
On bubs: cmd tx failures were seen as SW is sending panel off
commands when the dsi resources are turned off.
Upon suspend, DRM driver will issue a NULL composition to the
dpu, followed by turning off all the HW blocks.
v5 changes will serialize the NULL commit and resource unwinding
by handling them under PM prepare and PM complete phases there by
ensuring that clks are on when panel off commands are being
processed.
Changes in v6:
- Use drm_mode_config_helper_suspend/resume() instead of legacy API
drm_atomic_helper_suspend/resume() (Doug).
Trigger runtime callbacks from the suspend/resume call to turn
off the resources.
Signed-off-by: Kalyan Thota <kalyan_t@codeaurora.org>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 2 +
drivers/gpu/drm/msm/dsi/dsi.c | 2 +
drivers/gpu/drm/msm/msm_drv.c | 67 ++++++++++++++++-----------------
3 files changed, 37 insertions(+), 34 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index ce19f1d..b886d9d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -1123,6 +1123,8 @@ static int __maybe_unused dpu_runtime_resume(struct device *dev)
static const struct dev_pm_ops dpu_pm_ops = {
SET_RUNTIME_PM_OPS(dpu_runtime_suspend, dpu_runtime_resume, NULL)
+ SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+ pm_runtime_force_resume)
};
static const struct of_device_id dpu_dt_match[] = {
diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
index 55ea4bc2..62704885 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.c
+++ b/drivers/gpu/drm/msm/dsi/dsi.c
@@ -161,6 +161,8 @@ static int dsi_dev_remove(struct platform_device *pdev)
static const struct dev_pm_ops dsi_pm_ops = {
SET_RUNTIME_PM_OPS(msm_dsi_runtime_suspend, msm_dsi_runtime_resume, NULL)
+ SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+ pm_runtime_force_resume)
};
static struct platform_driver dsi_driver = {
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 7d985f8..da42ff7 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -1035,75 +1035,74 @@ static int msm_ioctl_submitqueue_close(struct drm_device *dev, void *data,
.patchlevel = MSM_VERSION_PATCHLEVEL,
};
-#ifdef CONFIG_PM_SLEEP
-static int msm_pm_suspend(struct device *dev)
+#ifdef CONFIG_PM
+static int msm_runtime_suspend(struct device *dev)
{
struct drm_device *ddev = dev_get_drvdata(dev);
struct msm_drm_private *priv = ddev->dev_private;
+ struct msm_mdss *mdss = priv->mdss;
- if (WARN_ON(priv->pm_state))
- drm_atomic_state_put(priv->pm_state);
+ DBG("");
- priv->pm_state = drm_atomic_helper_suspend(ddev);
- if (IS_ERR(priv->pm_state)) {
- int ret = PTR_ERR(priv->pm_state);
- DRM_ERROR("Failed to suspend dpu, %d\n", ret);
- return ret;
- }
+ if (mdss && mdss->funcs)
+ return mdss->funcs->disable(mdss);
return 0;
}
-static int msm_pm_resume(struct device *dev)
+static int msm_runtime_resume(struct device *dev)
{
struct drm_device *ddev = dev_get_drvdata(dev);
struct msm_drm_private *priv = ddev->dev_private;
- int ret;
+ struct msm_mdss *mdss = priv->mdss;
- if (WARN_ON(!priv->pm_state))
- return -ENOENT;
+ DBG("");
- ret = drm_atomic_helper_resume(ddev, priv->pm_state);
- if (!ret)
- priv->pm_state = NULL;
+ if (mdss && mdss->funcs)
+ return mdss->funcs->enable(mdss);
- return ret;
+ return 0;
}
#endif
-#ifdef CONFIG_PM
-static int msm_runtime_suspend(struct device *dev)
+#ifdef CONFIG_PM_SLEEP
+static int msm_pm_suspend(struct device *dev)
{
- struct drm_device *ddev = dev_get_drvdata(dev);
- struct msm_drm_private *priv = ddev->dev_private;
- struct msm_mdss *mdss = priv->mdss;
- DBG("");
+ if (pm_runtime_suspended(dev))
+ return 0;
- if (mdss && mdss->funcs)
- return mdss->funcs->disable(mdss);
+ return msm_runtime_suspend(dev);
+}
- return 0;
+static int msm_pm_resume(struct device *dev)
+{
+ if (pm_runtime_suspended(dev))
+ return 0;
+
+ return msm_runtime_resume(dev);
}
-static int msm_runtime_resume(struct device *dev)
+static int msm_pm_prepare(struct device *dev)
{
struct drm_device *ddev = dev_get_drvdata(dev);
- struct msm_drm_private *priv = ddev->dev_private;
- struct msm_mdss *mdss = priv->mdss;
- DBG("");
+ return drm_mode_config_helper_suspend(ddev);
+}
- if (mdss && mdss->funcs)
- return mdss->funcs->enable(mdss);
+static void msm_pm_complete(struct device *dev)
+{
+ struct drm_device *ddev = dev_get_drvdata(dev);
- return 0;
+ drm_mode_config_helper_resume(ddev);
}
#endif
static const struct dev_pm_ops msm_pm_ops = {
SET_SYSTEM_SLEEP_PM_OPS(msm_pm_suspend, msm_pm_resume)
SET_RUNTIME_PM_OPS(msm_runtime_suspend, msm_runtime_resume, NULL)
+ .prepare = msm_pm_prepare,
+ .complete = msm_pm_complete,
};
/*
--
1.9.1
^ permalink raw reply related
* Re: [PATCH v1] dt-bindings: leds: fix macro names for pca955x
From: Pavel Machek @ 2020-06-04 13:17 UTC (permalink / raw)
To: Flavio Suligoi
Cc: Jacek Anaszewski, Dan Murphy, Rob Herring, linux-leds, devicetree,
linux-kernel
In-Reply-To: <20200603091516.31907-1-f.suligoi@asem.it>
[-- Attachment #1: Type: text/plain, Size: 426 bytes --]
On Wed 2020-06-03 11:15:16, Flavio Suligoi wrote:
> The documentation reports the wrong macro names
> related to the pca9532 instead of the pca955x
>
> Signed-off-by: Flavio Suligoi <f.suligoi@asem.it>
> Acked-by: Rob Herring <robh@kernel.org>
Thanks, applied.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox